Closed Bug 565688 Opened 14 years ago Closed 14 years ago

int(GET['addons-author-addons-select']) needs a try-except

Categories

(addons.mozilla.org Graveyard :: Public Pages, defect)

Product:
addons.mozilla.org Graveyard
Component:
Public Pages
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: jbalogh, Assigned: wenzel)

References

(
URL
)

Details

(Whiteboard: [z][crash]) 

File "/data/virtualenvs/zamboni/lib/python2.6/site-packages/django/core/handlers/base.py", line 101, in get_response
   response = callback(request, *callback_args, **callback_kwargs)

 File "/data/amo_python/www/prod/zamboni/apps/addons/views.py", line 23, in decorated
   target_id = int(request.GET.get('addons-author-addons-select'))

ValueError: invalid literal for int() with base 10: "'271"

I've also seen a UnicodeError here, so it might be appropriate to catch and drop Exception here.


Oh, here's the UnicodeError, because I enjoy the GET params:

Traceback (most recent call last):

 File "/data/virtualenvs/zamboni/lib/python2.6/site-packages/django/core/handlers/base.py", line 101, in get_response
   response = callback(request, *callback_args, **callback_kwargs)

 File "/data/amo_python/www/prod/zamboni/apps/addons/views.py", line 23, in decorated
   target_id = int(request.GET.get('addons-author-addons-select'))

UnicodeEncodeError: 'decimal' codec can't encode character u'\x00' in position 14: invalid decimal Unicode string


<WSGIRequest
GET:<QueryDict: {u'addons-author-addons-select': [u'suckmydick.php\x00271']}>,
Related to bug 565581, or is there no shared code here at all?
(In reply to comment #1)
> Related to bug 565581, or is there no shared code here at all?

Related in that we both need to catch ValueErrors. No shared code, though.
Fix and test: http://github.com/jbalogh/zamboni/commits/c4c2f18
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Jeff/Fred: have a URL that trigger(s/ed) this exception?
https://preview.addons.mozilla.org/en-US/firefox/addon/1865/?addons-author-addons-select=stephend3000 is now a 404, rather than triggering a traceback (I'm assuming, since I still don't yet get them).

Verified?
(In reply to comment #5)
> Verified?

Yes, I implemented it as a 404 when you provide bad data. Come to think of it, perhaps that should be a 400 though.
(In reply to comment #6)
> (In reply to comment #5)
> > Verified?
> 
> Yes, I implemented it as a 404 when you provide bad data. Come to think of it,
> perhaps that should be a 400 though.

One line fix; do it now, while no-one's looking!
I'm on it :) waiting for our test suite to slug its way to success.
s/404/400/

http://github.com/jbalogh/zamboni/commits/5bd6838

Please verify this once it's landed: Bad input should now throw an error 400. If, however, you feed it a number that doesn't correspond to a valid add-on, that's a 404.
For compleness' sake:

https://preview.addons.mozilla.org/en-US/firefox/addon/1865/?addons-author-addons-select=348293472938

redirects, but then returns a 404, both as expected.
(In reply to comment #12)
> For compleness' sake:
> 
> https://preview.addons.mozilla.org/en-US/firefox/addon/1865/?addons-author-addons-select=348293472938
> 
> redirects, but then returns a 404, both as expected.

Aha; thanks, Fred.  Didn't feel quite right; appreciate the link for posterity.
(In reply to comment #10)
> https://preview.addons.mozilla.org/en-US/firefox/addon/1865/?addons-author-addons-select=stephend3000
> now returns a 400 error when it returns "Invalid add-on ID."

I don't think we should error out like that here.  From the logs, I don't think people are typing in these wonky links, they're following bad links from somewhere else.  We should ignore the bad query string or redirect to the same page with the bad part dropped.
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.