mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
128 stars 41 forks source link

Don't return HTML in the API #1587

Closed andymckay closed 8 years ago

andymckay commented 9 years ago

If a request comes into something on the API its asking for json, don't return HTML on an error. Example:

>>> res = requests.get('https://addons-dev.allizom.org/en-US/firefox/api/v3/does-not-exist', headers={'Accept': 'application/json'})
send: 'GET /en-US/firefox/api/v3/does-not-exist HTTP/1.1\r\nHost: addons-dev.allizom.org\r\nConnection: keep-alive\r\nAccept-Encoding: gzip, deflate\r\nAccept: application/json\r\nUser-Agent: python-requests/2.5.3 CPython/2.7.10 Darwin/14.5.0\r\n\r\n'
reply: 'HTTP/1.1 404 NOT FOUND\r\n'
header: Content-Encoding: gzip
header: Content-Security-Policy-Report-Only: script-src 'self' https://www.google.com https://www.paypalobjects.com https://ssl.google-analytics.com https://addons-dev-cdn.allizom.org/user-media; default-src * data:; style-src * 'unsafe-inline'; frame-src https://ssl.google-analytics.com https://sandbox.paypal.com; object-src 'none'; report-uri /services/csp/report
header: Content-Type: text/xml; charset=utf-8
header: Date: Sun, 01 Nov 2015 17:41:53 GMT
header: Server: nginx
header: Strict-Transport-Security: max-age=31536000
header: Vary: Accept-Encoding
header: Vary: X-Mobile, User-Agent
header: X-Frame-Options: DENY
header: Content-Length: 90
header: Connection: keep-alive
>>> res.content
'<?xml version="1.0" encoding="utf-8" ?>\n      <error>Not Found</error>\n  '

I'm requesting JSON and get back XML.

andymckay commented 9 years ago

So I found out what causes this, but didn't get a satisfactory answer.

handle500 is set to this: https://github.com/mozilla/olympia/blob/master/apps/amo/views.py#L112-L117

Which looks to see if it starts with /api/ and then bounces you over to /apps/api/ which returns XML because the main API on olympia is version check and block list. Change this at your peril.

So the next obvious thing to do is override the exception handler in DRF: http://www.django-rest-framework.org/api-guide/exceptions/, but if you do that and its a 500, nothing gets sent to sentry. So then if you try to add in sentry logging in the exception handler, you'll note that you don't actually get the request.

So then I thought I'd just change the handler500 in /apps/api/ and we can do that by simply checking request.path_info.startswith('/api/v3/'), but then ideally you want to create a Response that adapts to the Accept headers sent by the client. Which a standard rest framework Response won't do.

At that point I headed for a beer. There's probably something simple here I'm missing.

andymckay commented 8 years ago

Duplicate of mozilla/addons#3137, but it looks like @diox is working on that one 👍

diox commented 8 years ago

Ah yeah, had not seen your issue. I'll look into playing with the DRF exception handler, I did that in Marketplace and had the request working : https://github.com/mozilla/zamboni/blob/master/mkt/api/exceptions.py#L38