Closed Bug 1141513 Opened 9 years ago Closed 9 years ago

deal with avast requests that have bad query args

Categories

(Release Engineering Graveyard :: Applications: Balrog (backend), defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bhearsum, Assigned: bhearsum)

References

Details

Attachments

(1 file, 1 obsolete file)

We end up with tracebacks like:


ValueError: invalid literal for int() with base 10: '1?avast=1'

Stacktrace (most recent call last):

  File "flask/app.py", line 1475, in full_dispatch_request
    rv = self.dispatch_request()
  File "flask/app.py", line 1461, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "newrelic/hooks/framework_flask.py", line 40, in _nr_wrapper_handler_
    return wrapped(*args, **kwargs)
  File "flask/views.py", line 84, in view
    return self.dispatch_request(*args, **kwargs)
  File "flask/views.py", line 149, in dispatch_request
    return meth(*args, **kwargs)
  File "auslib/web/views/client.py", line 40, in get
    query = self.getQueryFromURL(url)
  File "auslib/web/views/client.py", line 36, in getQueryFromURL
    query['force'] = (int(request.args.get('force', 0)) == 1)



Where the incoming URLs are things like https://aus4.mozilla.org/update/3/Firefox/28.0/20140314220517/WINNT_x86-msvc/es-ES/release/Windows_NT%206.1.7600%20(x86)/default/default/update.xml?force=1%3Favast=1
OK, this seems to do the trick. I had originally put this code in a before_request handler (https://github.com/bhearsum/balrog/commit/8ae8249e90a025bb8c9502af55361747ba2a1cc1), but it seemed to make sense to put it near the other Avast handling code after self review.
Assignee: nobody → bhearsum
Status: NEW → ASSIGNED
Attachment #8578046 - Flags: review?(nthomas)
Comment on attachment 8578214 [details] [diff] [review]
support ?avast=1 and %3favast=1

Review of attachment 8578214 [details] [diff] [review]:
-----------------------------------------------------------------

r+, just one suggestion ....

::: auslib/web/views/client.py
@@ +28,5 @@
> +        qs = request.environ.get("QUERY_STRING", "")
> +        if "force" in qs and "avast" in qs:
> +            qs = qs.replace("?avast=1", "&avast=1")
> +            qs = qs.replace("%3Favast=1", "&avast=1")
> +        request.environ["QUERY_STRING"] = qs

Slightly safer & a smidgen less work to only re-set the env var inside the if block.
Attachment #8578214 - Flags: review?(nthomas) → review+
Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/808ae444437bccd52cf752ca9527cc4ff0a5cc17
bug 1141513: deal with avast requests that have bad query args. r=nthomas
Comment on attachment 8578214 [details] [diff] [review]
support ?avast=1 and %3favast=1

(In reply to Nick Thomas [:nthomas] from comment #3)
> Comment on attachment 8578214 [details] [diff] [review]
> support ?avast=1 and %3favast=1
> 
> Review of attachment 8578214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, just one suggestion ....
> 
> ::: auslib/web/views/client.py
> @@ +28,5 @@
> > +        qs = request.environ.get("QUERY_STRING", "")
> > +        if "force" in qs and "avast" in qs:
> > +            qs = qs.replace("?avast=1", "&avast=1")
> > +            qs = qs.replace("%3Favast=1", "&avast=1")
> > +        request.environ["QUERY_STRING"] = qs
> 
> Slightly safer & a smidgen less work to only re-set the env var inside the
> if block.

Good point. I landed this with that change.
Attachment #8578214 - Flags: checked-in+
Had to land a follow-up to fix pyflakes warnings.
We did not know about this problem with "?force=1?avast=1" instead of "?force=1&avast=1" until today - sorry.
After checking the code we found no obvious reason why this might happen. To make sure we changed the code sequence completely. The change will be deployed from today evening 20:00 (CET). Please let me know if this really fixes the problem. There's a small percentage of users with old and outdated Avast 8 which still might have that problem, however.
We did not know about this problem with "?force=1?avast=1" instead of "?force=1&avast=1" until today - sorry.
After checking the code we found no obvious reason why this might happen. To make sure we changed the code sequence completely. The change will be deployed from today evening 20:00 (CET). Please let me know if this really fixes the problem. There's a small percentage of users with old and outdated Avast 8 which still might have that problem, however.
(In reply to Thomas Salomon from comment #7)
> We did not know about this problem with "?force=1?avast=1" instead of
> "?force=1&avast=1" until today - sorry.
> After checking the code we found no obvious reason why this might happen. To
> make sure we changed the code sequence completely. The change will be
> deployed from today evening 20:00 (CET). Please let me know if this really
> fixes the problem. There's a small percentage of users with old and outdated
> Avast 8 which still might have that problem, however.

Thanks Thomas! Sounds like your change will be made before ours gets deployed, so I should be able get a good indication whether or not yours worked.
Depends on: 1145144
This was pushed to prod today and appears to be working fine.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Release Engineering → Release Engineering Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: