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

Multiple accept headers' specificity is misinterpreted #9940

Closed
ekampp opened this Issue Mar 26, 2013 · 81 comments

Comments

Projects
None yet
@ekampp
Copy link

ekampp commented Mar 26, 2013

The HTTP standard clearly states that higher specificity mime-types should take precedence, when multiple mimetypes is supplied. This is not the case with the current version of rails (cloned of master branch).

curl http://localhost:3000/entries --header "Accept:application/json, text/plain"

should return json, and in fact does so. While

curl http://localhost:3000/entries --header "Accept:application/json, text/plain, */*"

should still return json, since the */* is less specific, but this returns html.

Here is a test application where this can be tested.

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 26, 2013

It seems that Mime::Type.parse("Accept: application/json, text/plain, */*") simply parses the mime types in accordance to their position in the string, not their individual specificity.

=> [ #<Mime::Type:0x007fdabc15d7d8 @synonyms=[], @symbol=nil, @string="Accept: application/json">, 
     #<Mime::Type:0x007fdabc42be38 @synonyms=[], @symbol=:text, @string="text/plain">, 
     #<Mime::Type:0x007fdabc06e458 @synonyms=[], @symbol=nil, @string="*/*">]
@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 26, 2013

It seems that mime negotiation completely ignores any previously, valid, available mimetype, and skips any check, if there is a */* in the accept header.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Mar 27, 2013

Thanks! I agree this needs fixed.

@neerajdotname

This comment has been minimized.

Copy link
Member

neerajdotname commented Mar 27, 2013

@ekampp You are right . Rails completely ignores mime negotiation if */* is present in accept header. And here is the reason why http://blog.bigbinary.com/2010/11/23/mime-type-resolution-in-rails.html .

This is by design.

I don't think it's a bug . I'll let @josevalim confirm it .

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 27, 2013

I read that blog as well. The problem is that opting out of mime type negotiation simply because there is a catch-all in there is wrong according to the specifications of HTTP.

I'm thinking that we could check if any valid mime types has been found in the accept header, and if so use them, else opt out and return html. Allthough, I'm still a little unsure as to how we should determine which of the found, valid mime types that's more specific than others. The simple approach seems to be to negotiate all non-star (*) types before any star types.

@neerajdotname

This comment has been minimized.

Copy link
Member

neerajdotname commented Mar 27, 2013

@ekampp you are right we should follow HTTP specification. However the problem is the browser vendors are not following it very strictly. Rails team has gone through a number of iterations on accept header issue.

As suggested in the blog if things are in your control then please use .json in your requests.

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 27, 2013

How so, do you mean "[...] not following very strictly"?

@neerajdotname

This comment has been minimized.

Copy link
Member

neerajdotname commented Mar 27, 2013

Safari sends following order

application/xml (q is 1)
application/xhtml+xml (q is 1)
image/png (q is 1)
text/html (q is 0.9)
text/plain (q is 0.8)
\*/\* (q is 0.5)

So you visit www.myappp.com in safari and if the app supports .xml then Rails should render .xml file. This is not what user wants to see. User wants to see .html page not .xml page.

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 27, 2013

All right. So how could we approach the issue to allow clients that do know what they are talking about, to negotiate the right content with the server?

@neerajdotname

This comment has been minimized.

Copy link
Member

neerajdotname commented Mar 27, 2013

If you lay out the core issue that you are having then we can work towards a solution.

I'm assuming that all the app works fine in all the browsers.

Issue could be you are sending a specific curl or API request. In that case I would suggest to use .xml or .json format to eliminate accept header parsing issue.

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 27, 2013

The specific problem is that angularjs sends correct content negotiation when trying to grab content, which rails picks up wrong. So i'm getting html back, when I should be getting json back.

I could add .json on the end, but that would mean hacking away at angularjs, which is doing the right thing. I would rather find a good solution and hack away at rails, which is doing the wrong thing :)

@steveklabnik do you have a good idea for a solution here?

@neerajdotname

This comment has been minimized.

Copy link
Member

neerajdotname commented Mar 27, 2013

I will have to look at code to be sure but the rule that */* trumps all is applied only when the request is not an ajax request. So if you are making ajax request then you should be good.

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 27, 2013

Yes. I saw that. But that seems to be not quite right either.

I would love to look into it my self, but I'm unsure on how I test those deeper parts of rails. So if you have some pointers, I will get on hacking away at it my self :)

@neerajdotname

This comment has been minimized.

Copy link
Member

neerajdotname commented Mar 27, 2013

Rails guide has a section on contributing to rails. http://guides.rubyonrails.org/contributing_to_ruby_on_rails.html . You can change/add tests and see how it behaves.

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 27, 2013

Oh. What I meant was I'm having a hard time isolating the mime type stuff, so I can manipulate that specifically. The guide doesn't offer much help in that section. How do I instantiate the part of rails that does the content negoitation.

I'm guessing that curling the application isn't good enough a test. I need to test the individual methods of the content negotiation modules, right?

@LTe

This comment has been minimized.

Copy link
Contributor

LTe commented Mar 27, 2013

Related? #7464

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 27, 2013

Yes. They are indeed related — thank you!

The code attached there solves the issue in parts: It solves the problem of opting out when */* appears, which is an immediate solution to my problem.

But another aspect of the problem still persists, that rails doesn't take into consideration the relative specificity of the accept headers.

@pawelpacana

This comment has been minimized.

Copy link

pawelpacana commented Mar 27, 2013

@neerajdotname As mentioned in #7464 and https://bugs.webkit.org/show_bug.cgi?id=27267 that accept preference has been fixed 2 years ago.

My Safari sends:

HTTP_CONNECTION:keep-alive
HTTP_CONTENT_LENGTH:0
HTTP_ACCEPT:text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
HTTP_ACCEPT_ENCODING:gzip, deflate
HTTP_ACCEPT_LANGUAGE:pl-pl
HTTP_USER_AGENT:Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/536.26.17 (KHTML, like Gecko) Version/6.0.2 Safari/536.26.17

My Chrome sends:

HTTP_CONNECTION:keep-alive
HTTP_CONTENT_LENGTH:0
HTTP_ACCEPT:text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
HTTP_ACCEPT_CHARSET:UTF-8,*;q=0.5
HTTP_ACCEPT_ENCODING:gzip,deflate,sdch
HTTP_ACCEPT_LANGUAGE:pl-PL,pl;q=0.8,en-US;q=0.6,en;q=0.4
HTTP_USER_AGENT:Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/537.22 (KHTML, like Gecko) Chrome/25.0.1364.172 Safari/537.22
@neerajdotname

This comment has been minimized.

Copy link
Member

neerajdotname commented Mar 27, 2013

@pawelpacana indeed looks like the newer version of browsers have this problem fixed.

I guess the question is do we want to break newer Rails application for people using older versions of browser.

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 27, 2013

So the discussion becomes how many browsers, and how far back rails should support.

I don't really have a good way of testing ie, but my Opera sends this:

User-Agent: Opera/9.80 (Macintosh; Intel Mac OS X 10.8.3) Presto/2.12.388 Version/12.14
Accept: text/html, application/xml;q=0.9, application/xhtml+xml, image/png, image/webp, image/jpeg, image/gif, image/x-xbitmap, */*;q=0.1
Accept-Language: en,en-US;q=0.9,ja;q=0.8,fr;q=0.7,de;q=0.6,es;q=0.5,it;q=0.4,pt;q=0.3,pt-PT;q=0.2,nl;q=0.1
Accept-Encoding: gzip, deflate
Connection: Keep-Alive
@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 27, 2013

As @pawelpacana noted, the problem has been fixed for over two years on webkit, i suggest that we begin to look at a way to then properly support the correct implementation of the accept headers.

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 27, 2013

I'm thinking that we could determine which of the supplied, accepted mime types that doesn't include a star, parse those first according to their weighting, then parse all with a single star according to their weighting, and then, if nothing else is given, fall back to html.

@LTe

This comment has been minimized.

Copy link
Contributor

LTe commented Mar 29, 2013

I used browserstack and @pawelpacana tool to create list of accept-headers. I received 160 answers from browsers

https://gist.github.com/LTe/5270348

Uniq headers:

"text/html, application/xml;q=0.9",
"text/html, application/xhtml+xml, */*",
"text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8",
"text/html, application/xml;q=0.9, application/xhtml+xml, image/png, image/webp, image/jpeg, image/gif, image/x-xbitmap, */*;q=0.1",
"text/html, application/xml;q=0.9, application/xhtml+xml, image/png, image/jpeg, image/gif, image/x-xbitmap, */*;q=0.1",
"application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5",
"*/*"

I also updated my branch click

So you visit www.myappp.com in safari and if the app supports .xml then Rails should render .xml file. This is not what user wants to see. User wants to see .html page not .xml page.

@neerajdotname when safari send

application/xml (q is 1)
application/xhtml+xml (q is 1)
image/png (q is 1)
text/html (q is 0.9)
text/plain (q is 0.8)
\*/\* (q is 0.5)

Rails will send html as response. Because application/xhtml+xml at this point is the most important. For rails application/xhtml+xml == text/html

If more than one media range applies to a given type, the most specific reference has precedence.
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.1

@neerajdotname

This comment has been minimized.

Copy link
Member

neerajdotname commented Mar 29, 2013

@LTe clearly it looks like browsers have gotten their act together. As mentioned earlier if Rails starts doing the right thing then it will break older browsers.

@LTe

This comment has been minimized.

Copy link
Contributor

LTe commented Mar 29, 2013

@neerajdotname can you give example of older browser with very-wrong accept-header?

@neerajdotname

This comment has been minimized.

Copy link
Member

neerajdotname commented Mar 29, 2013

older version of safari had image/png at higher priority over text/html.

@neerajdotname

This comment has been minimized.

Copy link
Member

neerajdotname commented Mar 29, 2013

@LTe

application/xml (q is 1)
application/xhtml+xml (q is 1)
image/png (q is 1)
text/html (q is 0.9)
text/plain (q is 0.8)
\*/\* (q is 0.5)

You mentioned that in that above case the application/xhtml+xml is the most important one. Is it because that one has + sign. In the RFC I do not see anywhere being mentioned that one with + is even more specific.

May be I am wrong but I think all the top three are equally specific.

@LTe

This comment has been minimized.

Copy link
Contributor

LTe commented Mar 29, 2013

When a new media type is introduced for an XML-based format, the name of the media type SHOULD end with '+xml'.
http://tools.ietf.org/html/rfc3023#section-7

In this case, you're right. The question is what the author had in mind here? 6972313#L0R108

@dhh do this: 06c2b43

Commit message linked to: http://www.xml.com/pub/a/2004/07/21/dive.html

According to RFC 3023, if the media type given in the Content-Type HTTP header is application/xml, application/xml-dtd, application/xml-external-parsed-entity, or any one of the subtypes of application/xml such as application/atom+xml or application/rss+xml or even application/rdf+xml

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Apr 1, 2013

Again, @LTe that solution looks promising to me.

@JonRowe

This comment has been minimized.

Copy link
Contributor

JonRowe commented Apr 30, 2013

What was the consensus here? Should Rails change to respect the mimetype order? Or is there still an issue with older browsers? Do we support those older browsers?

@LTe

This comment has been minimized.

Copy link
Contributor

LTe commented Jul 31, 2015

Hi @demsullivan! I think issue only appear when you make non-ajax HTTP request (like API does) with application/json accept header.

@djpate

This comment has been minimized.

Copy link

djpate commented Aug 12, 2015

I have the same issue as @demsullivan (on 3.2.8).
@LTe not sure what you mean by non-ajax request.
Are CORS Ajax not considered ajax?

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Aug 20, 2015

@demsullivan I believe you're right. But this is also quite an old issue. And they removed the implicit mime-type response out of Rails, and into the responders gem. So in your example you're explicitly responding with json.

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Aug 20, 2015

I have updated the initial example to rails 4.2.3, which supports the results found by @demsullivan

@rails-bot rails-bot closed this Aug 25, 2015

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Aug 25, 2015

This issue has been automatically closed because of inactivity.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

MaciejGawel added a commit to aghinz2015/foram-api that referenced this issue Nov 22, 2015

MaciejGawel added a commit to aghinz2015/foram-api that referenced this issue Nov 22, 2015

@jcoyne

This comment has been minimized.

Copy link
Contributor

jcoyne commented Feb 10, 2016

This is still an issue. If */* is present in the Accept header, it will skip content negotiation, even if more suitable mime-types precede the */* Tested with 4-2-stable.

@josevalim

This comment has been minimized.

Copy link
Contributor

josevalim commented Feb 10, 2016

We unfortunately still have browsers to thank for this behaviour. In IE10 (iirc), any software installed on your system can still prepend something to the accepts query. In other words, we cannot trust accept headers if they have / in them because chances would be that you would serve the wrong page for anyone running on IE.

@jcoyne

This comment has been minimized.

Copy link
Contributor

jcoyne commented Feb 10, 2016

IE10 is EOL as of January and has less than 3.4% of the market share according to netmarketshare.com. Shouldn't Rails be concerned about the API use case? It's important to me that I give my clients the best matching content, rather than something that is merely adequate for them: */*

jcoyne added a commit to curationexperts/alexandria-legacy that referenced this issue Feb 10, 2016

Don't send */* to rails in the accept header
This is a workaround for Rails
rails/rails#9940.  Rails doesn't do content
negotiation if you pass */*

jcoyne added a commit to curationexperts/alexandria-legacy that referenced this issue Feb 10, 2016

Don't send */* to rails in the accept header
This is a workaround for Rails
rails/rails#9940.  Rails doesn't do content
negotiation if you pass */*
@josevalim

This comment has been minimized.

Copy link
Contributor

josevalim commented Feb 10, 2016

IIRC It is IE10 and older, which means we are looking at least at 15% market share. I haven't checked if they finally fixed the issue on IE11.

@kreynolds

This comment has been minimized.

Copy link
Contributor

kreynolds commented Feb 10, 2016

What exactly is the accept header causing bad behavior that we're concerned about? When I went through the various browsers previously, everything seemed to Do The Right Thing™. Is our problem here when :format is unspecified, the browser prefers XML over HTML, and the endpoint implements both XML and HTML?

@bradgessler

This comment has been minimized.

Copy link

bradgessler commented Feb 10, 2016

We ran into this problem and wrote a middleware that rewrites the headers to deal with Internet Explorer mime type issues. I just pulled this chunk of code out of our rails repo, MIT licensed it, and am sharing it with you at https://gist.github.com/bradgessler/0958e3f8a17c358263c5 (pardon some of the IE rage in there). Feel free to use that in your own project (or better yet turn it into a gem to share with the rest of the community and post al link on here).

Overall I believe this should belong in a middleware and should not be a rails core team concern.

@jcoyne

This comment has been minimized.

Copy link
Contributor

jcoyne commented Feb 11, 2016

@bradgessler Nice work. I too prefer that approach.

@kreynolds

This comment has been minimized.

Copy link
Contributor

kreynolds commented Feb 11, 2016

@bradgessler I agree, rails core should behave correctly and any goofiness belongs elsewhere.

@david50407

This comment has been minimized.

Copy link

david50407 commented Jan 15, 2018

Still can re-produced on Rails 5.0.0.1.

Requests with header Accept: x-any-type/x-any-subtype, */* has been rewrited as text/html here (ActionDispatch::Http::MimeNegotiation):

https://github.com/rails/rails/blob/v5.0.0.1/actionpack/lib/action_dispatch/http/mime_negotiation.rb#L77
https://github.com/rails/rails/blob/v5.0.0.1/actionpack/lib/action_dispatch/http/mime_negotiation.rb#L159

It's very annoying while using api-mode with requesting Accept: application/json, */* and return 406 (not acceptable).

@rails-bot rails-bot bot removed the stale label Jan 15, 2018

@kreynolds

This comment has been minimized.

Copy link
Contributor

kreynolds commented Jan 15, 2018

As a reminder, I have this and several other issues fixed in #14540. I'd be happy to spend the time updating it to merge it cleanly into the main branch if I knew that rails core would accept it. There is no need for a separate gem or middleware or any nonsense like that, the parsing just needs to be correct.

@mb21

This comment has been minimized.

Copy link

mb21 commented Mar 21, 2018

Cannot believe this is still not fixed in Rails 5.0.6. The following doesn't return JSON:

curl 'http://localhost:3000/article/1' -H 'Accept: application/json, */*'

with this in the controller:

respond_to do |format|
  format.html { render }
  format.json { render json: @article }
end

Please at least reopen the issue.

@david50407

This comment has been minimized.

Copy link

david50407 commented Mar 21, 2018

I monkey-patched ActionDispatch::Http::MimeNegotiation, to skip browser-like accepts check:

# This monkey patch applies to:
#     Rails acts '*/*' in multi-accept types as 'text/html' directly.
# Related to: https://github.com/rails/rails/issues/9940
ActionDispatch::Http::MimeNegotiation.send :remove_const, :BROWSER_LIKE_ACCEPTS
ActionDispatch::Http::MimeNegotiation.const_set :BROWSER_LIKE_ACCEPTS, /NEVER_MATCHED_MIME_TYPE_LOL/
@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 23, 2018

@mb21 there's no need to reopen this issue. The bug is confirmed -- also by core team members. There is already a branch that fixes this problem, which is open: #14540

@mb21

This comment has been minimized.

Copy link

mb21 commented Mar 23, 2018

@ekampp I assumed issues would be closed only when they are fixed in master, i.e. when the branch that fixes the bug is merged? Would love to see #14540 merged, but it's been there since 2014 :S

@ekampp

This comment has been minimized.

Copy link
Author

ekampp commented Mar 23, 2018

Yep, and this issue has been here since 2013. Don't get your hopes up 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment