multimeric / flapison

Flask extension to build REST APIs around JSONAPI 1.0 specification.
http://flask-rest-jsonapi.readthedocs.io
MIT License
13 stars 4 forks source link

Prevent False Positives in MIMEtypes #2

Closed srhinos closed 5 years ago

srhinos commented 5 years ago

This fixes a bug I'm experiencing integrating in Flapison. I don't want to force the front end to do a restructuring of their code to support strict accept types so this will help prevent any false positives when throwing out the error

srhinos commented 5 years ago

Figured out what I was doing dumb, nvm about this

multimeric commented 5 years ago

Out of interest, what were you doing wrong?

srhinos commented 5 years ago

So my team is split into frontend and backend and before swapping to flapison, we didn't have any enforced standards when it came to the Accept or Content-Type headers.

I do think a variation of this PR needs to exist because as is since request.accept_mimetypes.best will prioritize the wrong stuff if provided a list like: application/json text/html */* produces text/html

Another edge case I found: Content-Type: application/json;charset=utf-8 throws an error despite application/json being an accepted type.

This PR wouldn't solve that latter issue though since Content-Type is a get only method so resetting it and even if you did in this code set, we're attempting to use that encode type as a key in request.content_type further on.

I've just kept the PR above as a monkeypatch for now in my prod code base (for now) and asked front end to make changes to standardize their practices because thats prolly the best thing in the long run regardless.

multimeric commented 5 years ago

Okay cool! I'm happy you're using this in your project.

Thanks for spelling all this out. Really I'm relying on werkzeug's content parsing behaviour here, and I would be surprised if it made any serious mistakes (if it does, you should raise an issue over at https://github.com/pallets/werkzeug).

In general the behaviour of Werkzeug in parsing the Accept header is documented here. best is supposed to return the first content type in the list, assuming they have all the same quality score, so I'd be surprised if it returns text/html in your example? But in any case, I think it's good practise not to pass in multiple content types anyway, because otherwise you'd have to write code to handle all content types.

However you might be onto something with regards to the content type issue. Having looked it up, I should perhaps be using the mimetype field instead, which seems to strip out the encoding parameter. I'll have a look into this.

multimeric commented 5 years ago

I investigated the Accept header, and you're right about how it prioritizes content types:

>>> from werkzeug.http import parse_accept_header
>>> parse_accept_header('application/json text/html */*')
Accept([('text/html', 1), ('application/json', 1), ('*/*', 1)])
>>> parse_accept_header('application/json text/html */*').best
'text/html'

However, I'm not convinced this is bad behaviour by werkzeug, because the spec doesn't place any importance on the order of mimetypes. Only the q value, and the specificity of the content type matters (e.g. text/html will win over text/*). I think if you want to prioritize application/json, you should give the other types lower q values.