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)
Release Engineering Graveyard
Applications: Balrog (backend)
x86_64
Linux
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bhearsum, Assigned: bhearsum)
References
Details
Attachments
(1 file, 1 obsolete file)
3.90 KB,
patch
|
nthomas
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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 | ||
Comment 2•9 years ago
|
||
Turns out most (maybe all?) of these requests come in with an escaped "?" instead of unescaped. We should support both to be safe. I tested this out in Vagrant, all of these returned the same thing: http://balrog.mozilla.dev:9000/update/3/Firefox/33.0a1/20120222174716/WINNT_x86-msvc/en-US/nightly/default/default/default/update.xml?force=1%3Favast=1 http://balrog.mozilla.dev:9000/update/3/Firefox/33.0a1/20120222174716/WINNT_x86-msvc/en-US/nightly/default/default/default/update.xml?force=1?avast=1 http://balrog.mozilla.dev:9000/update/3/Firefox/33.0a1/20120222174716/WINNT_x86-msvc/en-US/nightly/default/default/default/update.xml?force=1&avast=1 http://balrog.mozilla.dev:9000/update/3/Firefox/33.0a1/20120222174716/WINNT_x86-msvc/en-US/nightly/default/default/default/update.xml?force=1
Attachment #8578046 -
Attachment is obsolete: true
Attachment #8578046 -
Flags: review?(nthomas)
Attachment #8578214 -
Flags: review?(nthomas)
Comment 3•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
Had to land a follow-up to fix pyflakes warnings.
Comment 7•9 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•9 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•9 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 | ||
Comment 10•9 years ago
|
||
This was pushed to prod today and appears to be working fine.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Product: Release Engineering → Release Engineering Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•