Bug in Mime::Type#parse for single media range with accept params #736

Closed
lighthouse-import opened this Issue May 16, 2011 · 14 comments

Projects

None yet

7 participants

@lighthouse-import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/5833
Created by Joel Chippindale - 2010-10-18 10:54:21 UTC

Mime::Type#parse doesn't split out the accept params from the media range when there is a single media range.

I.e. it incorrectly parses the valid HTTP_ACCEPTS header "/;0.9" to be "/;0.9" rather than "/"

@lighthouse-import

Imported from Lighthouse.
Comment by Joel Chippindale - 2010-10-18 11:07:36 UTC

Attached patch for master

@lighthouse-import

Imported from Lighthouse.
Comment by Joel Chippindale - 2010-10-18 11:08:00 UTC

And patch for 2-3-stable

@lighthouse-import

Imported from Lighthouse.
Comment by Piotr Sarnacki - 2010-12-17 12:31:48 UTC

Joel: why patch for master uses /;/ and patch for 2.3 uses ';' ?

@lighthouse-import

Imported from Lighthouse.
Comment by Joel Chippindale - 2011-02-07 12:38:20 UTC

Piotr,

The patch for master does not use /;/, rather they both use ';'

If you look in the patch for master and for 2.3 the line is

[Mime::Type.lookup(accept_header.split(';').first)]

Or perhaps I have misunderstood your question

J.

@lighthouse-import

Imported from Lighthouse.
Comment by Joel Chippindale - 2011-02-07 12:57:04 UTC

This is still an issue with master, here is a new version of the patch which applies cleanly to current master.

@lighthouse-import

Attachments saved to Gist: http://gist.github.com/971715

@jake3030 jake3030 pushed a commit to jake3030/rails that referenced this issue Jun 28, 2011
@lifo Jim Remsik and Tim Pope + lifo Ensure has_many :through works with changed primary keys [#736 state:…
…resolved]

Signed-off-by: Pratik Naik <pratiknaik@gmail.com>
1e6c50e
@elliotcm

I would quite like to see this patch applied to master. Useless or not it is still valid HTTP, and is not just caused by bots. At least one of the PSP's browsers sends */*;q=0.01 as an accept header.

@scottwb
Contributor
scottwb commented Aug 17, 2011

+1 for applying this patch. I agree with @elliotcm that properly parsing valid HTTP is the right thing to do. For now, I am using this ugly monkey-patch, which is not as nice as the posted patch, but is easy and unintrusive:

module Mime
  class Type
    def self.lookup(string)
      LOOKUP[string.split(';').first]
    end
  end
end
@mhuggins

+1 for seeing this fixed. There's already a gist attached to this issue that looks like it does the trick: http://gist.github.com/971715

@bittersweet

+1 indeed, we have this problem with a lot of our applications as well, these are a few of the headers:

text/html;q=0.7
*/*, application/youtube-client
@dolzenko
Contributor

+1

@mhuggins

Is someone in Rails core able to reopen this issue?

@damncabbage

+1. Please reopen this issue; this is still relevant.

@scottwb
Contributor
scottwb commented Feb 6, 2012

I can see from inspecting the code, that this is still not fixed in master. The problem is that Mime::Type.parse treats the single content-type case differently from the multiple content-type case, and all the fixes go into the multiple content-type case. I think a quick refactoring of that code to share the logic for both cases would solve this. I am going to work up a pull request.

In the meantime, I have posted a gist similar to the monkey-patch I described above. This time it includes a test case and some version checking so you can detect when an upgrade to a later version of Rails pulls in a fix. (This gist also fixes a related issue #860).

https://gist.github.com/1754727

@scottwb scottwb added a commit to scottwb/rails that referenced this issue Feb 7, 2012
@scottwb scottwb Failing test case that shows issue #736 should still be open. 339c99b
@scottwb scottwb added a commit to scottwb/rails that referenced this issue Feb 7, 2012
@scottwb scottwb Correctly handle single media with q value. Fixes #736. 3b203f7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment