deal with avast requests that have bad query args

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bhearsum, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created attachment 8578046 [details] [diff] [review]
support bad query strings that avast sends

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+

Comment 4

4 years ago
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
(Assignee)

Comment 5

4 years ago
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+
(Assignee)

Comment 6

4 years ago
Had to land a follow-up to fix pyflakes warnings.

Comment 7

4 years ago
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.

Comment 8

4 years ago
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.
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Updated

4 years ago
Depends on: 1145144
(Assignee)

Comment 10

3 years ago
This was pushed to prod today and appears to be working fine.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.