Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Response Content-Type potentially malformed #4253

Closed
4 of 6 tasks
lsowen opened this issue Jul 11, 2016 · 6 comments
Closed
4 of 6 tasks

Response Content-Type potentially malformed #4253

lsowen opened this issue Jul 11, 2016 · 6 comments
Labels
Milestone

Comments

@lsowen
Copy link

lsowen commented Jul 11, 2016

Checklist

  • I have verified that that issue exists against the master branch of Django REST framework.
  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • This is not a usage question. (Those should be directed to the discussion group instead.)
  • This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
  • I have reduced the issue to the simplest possible case.
  • I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Problem description

The default request header 'Accept' on many browsers is something like Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8. This means that currently, if the response is not text/html, then the response Content-Type incorrectly contains the q= parameter. (According to this, the q parameter specifies which content-types the browser prefers).

It looks like it is occurring because:

  1. DefaultContentNegotiation.select_renderer returns the full_media_type (which includes the extra Accept "parameters" like q)
  2. ApiView.perform_content_negotiation returns the renderer, full_media_type tuple
  3. ApiView.initial sets request.accepted_media_type equal to that full_media_type
  4. ApiView.finalize_response sets response.accepted_media_type equal to request.accepted_media_type
  5. Response.rendered_content is setting the response Content-Type to the value of response.accepted_media_type.

This only came to light because I'm running nginx in front of django, and it was not recognizing the content-type (as application/json is different from application/json;q=0.9). As far as I can tell, this is actually an invalid Content-Type (https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.17).

Steps to reproduce

curl --verbose http://example.com/api/model?format=json --H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,/;q=0.8' > /dev/null

Expected behavior

Content-Type: application/json; charset=utf-8

Actual behavior

Content-Type: application/json;q=0.9; charset=utf-8

@tomchristie
Copy link
Member

tomchristie commented Jul 11, 2016

Closing this as I couldn't replicate the described behavior...

$ curl --verbose http://restframework.herokuapp.com/?format=json -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,/;q=0.8'

--> 406 {"detail":"Could not satisfy the request Accept header."}

Or this, without the format=json...

$ curl --verbose http://restframework.herokuapp.com/ -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,/;q=0.8'
HTTP/1.1 200 OK
Content-Type: text/html; charset=utf-8
...

More than happy to reopen this if you can demo an example against http://restframework.herokuapp.com/ that I can replicate, or if you can demo that the behavior has changed, and that the latest version of REST framework running the tutorial does demonstrate the issue, or if you can provide an example pull request that demonstrates that we do have an issue.

@lsowen
Copy link
Author

lsowen commented Jul 11, 2016

They are different.

In master, the return value is the full_media_type (e.g. includes q and indent).

In 3.1, the return value is renderer.media_type, which does not include the extra parameters passed in by the Accept header.

@tomchristie tomchristie reopened this Jul 11, 2016
@tomchristie
Copy link
Member

tomchristie commented Jul 11, 2016

Thanks for confirming.

@tomchristie
Copy link
Member

tomchristie commented Jul 11, 2016

Now resolved, thanks again @lsowen!

@tomchristie tomchristie added this to the 3.4.0 Release milestone Jul 11, 2016
@lsowen
Copy link
Author

lsowen commented Jul 11, 2016

@tomchristie I appreciate the quick response. Thanks for your help!

@tomchristie
Copy link
Member

tomchristie commented Jul 11, 2016

Most welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants