Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix MIME::Type.parse handling of single media with a q value #4918

Merged
merged 2 commits into from
@scottwb

An HTTP request header like this:

Accept: text/html;q=0.9

Is valid according to the HTTP spec, but is not handled well by Mime::Type.parse. It is not uncommon to receive requests like this from bots and, according to one source, the PSP's browser. The failed parsing typically manifests itself as a MissingTemplate error.

This change fixes issue #736, which is not currently open, but should be.

The fix should be easy to also apply to 3.x branches. I'd be happy to help with that if necessary.

@masterkain

Related: #4127 #701

@josevalim josevalim commented on the diff
actionpack/test/dispatch/mime_type_test.rb
@@ -69,6 +69,12 @@ class MimeTypeTest < ActiveSupport::TestCase
assert_equal expect, Mime::Type.parse(accept)
end
+ test "parse single media range with q" do
@josevalim Owner

Could you please also add a test that handles multiple types with q=? Your changes also updated this part of the code, but you haven't added any tests for it. Thanks.

@scottwb
scottwb added a note

@josevalim Just above that line (line 66) there is a pre-existing test that handles multiple types with q=:

  test "parse with q" do
    accept = "text/xml,application/xhtml+xml,text/yaml; q=0.3,application/xml,text/html; q=0.8,image/png,text/plain; q=0.5,application/pdf,*/*; q=0.2"
    expect = [Mime::HTML, Mime::XML, Mime::PNG, Mime::PDF, Mime::TEXT, Mime::YAML, Mime::ALL]
    assert_equal expect, Mime::Type.parse(accept)
  end

This test passes before and after my change. Or am I misunderstanding your request?

@josevalim Owner

That was it, thanks! Merging.

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

Just wanted to say thanks for taking the time and doing the effort scottwb! I hope your work fixes it once and for all :)

@josevalim josevalim merged commit dd9b428 into from
@pheld

Here's a freedom patch that works for this for now:

  class << Mime::Type
    Q_SEPARATOR_REGEXP = /;\s*q=/

    def parse_with_q_fix(accept_header)
      if accept_header !~ /,/
        accept_header = accept_header.split(Q_SEPARATOR_REGEXP).first
      end

      parse_without_q_fix accept_header
    end

    alias_method_chain :parse, :q_fix
  end
@jeremywadsack

@scottwb Thanks for this fix and getting it merged. GoogleBot hits us on this every day now. @pheld Thanks for the monkey patch for the meantime.

@asanghi

This fix is in Rails master (4.0) or Rails stable (3.2.x)? If not, can this please be merged into 3.2.x?

@scottwb
@asanghi

Fine. In the mood for taking on some stabbing.

@asanghi asanghi referenced this pull request from a commit
@asanghi asanghi backporting #4918 to 3.2 stable; adding extra test for accept header …
…given by googlebot
dffd85a
@steveklabnik steveklabnik referenced this pull request from a commit in steveklabnik/rails
@steveklabnik steveklabnik Added parsing of arbitrary media type parameters.
Based on #4918.

Related to #4127.
8c4b3a4
@steveklabnik steveklabnik referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@joshuap joshuap referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@andhapp andhapp referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@andhapp andhapp referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@bokor

what version was this merged into. I'm still on 3.0.9

@scottwb

@designwaves I am pretty sure it was merged into 3.2.x. Not sure what 'x' is, but when I discussed it with some of the maintainers they said they did not plan any functional updates to 3.0 and 3.1 at the time so it would only be relevant to apply it to 3.2 and later.

@bokor

Scott,

Is there a monkey patch that I can cut and paste into 3.0.9?

@steveklabnik
Collaborator

@scottwb @designwaves Above, you can see this was merged into 3.2 stable.

@scottwb

@designwaves I posted a while back about some monkey-patches I was using on 3.0.7, which should probably work for you on 3.0.9: https://gist.github.com/1754727, however I think I like the one pasted above by @pheld better.

@bokor

sweet thank you so much for helping out with this. We are getting enough traffic now that this is becoming an issue and wanted to patch it. Awesome work man!

@bokor

I used the Gist and it fixed all my issues. The monkey patch above only fixed one but your code fixed all. thank you for that. It really helped out.

@strzalek strzalek referenced this pull request from a commit in square/rails
@strzalek strzalek Backport mime-type parsing fixes 1c1ee5d
@strzalek strzalek referenced this pull request from a commit in square/rails
@strzalek strzalek Backport mime-type parsing fixes a31adba
@GT565K

Has this been fixed for 3.2?

I saw that it was merged into master, but I'm using 3.2.13 and I'm having the same issue.

ActionView::MissingTemplate: Missing template homes/show, application/show with {:locale=>[:en], :formats=>["text/html; chars...lder, :slim, :coffee]}. Searched in: * "/app/app/views" * "/app/vendor/bundle/ruby/1.9.1/gems/kaminari-0.14.1/app/views"
HTTP_ACCEPT
text/html; charset=utf-8; q=0.1

seems to be happening with bots (as mentioned above). Here's the user agent that triggered it

HTTP_USER_AGENT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.116 Safari/537.36 HubSpot Marketing Grader

replicated with

curl -H"Accept: text/html; charset=utf-8; q=0.1" http://localhost:3000/
@rafaelfranca

It seems to be included in 3-2-stable so maybe it is in one 3.2.x release.

@GT565K

@rafaelfranca doesn't seem so.

I just tried 3.2.18, which is the latest version as shown in the RAILS_VERSION file under the 3.2-stable branch and I still get the same error...

@strzalek strzalek referenced this pull request from a commit in square/rails
@strzalek strzalek Backport mime-type parsing fixes 5ac247c
@rafaelfranca

Maybe it was reverted because caused some regression.

@tamird tamird referenced this pull request from a commit in square/rails
@asanghi asanghi backporting #4918 to 3.2 stable; adding extra test for accept header …
…given by googlebot
9e9c852
@tamird tamird referenced this pull request from a commit in square/rails
@steveklabnik steveklabnik Added parsing of arbitrary media type parameters.
Based on #4918.

Related to #4127.

Conflicts:
	actionpack/test/dispatch/mime_type_test.rb
0413897
@tamird tamird referenced this pull request from a commit in square/rails
@steveklabnik steveklabnik Added parsing of arbitrary media type parameters.
Based on #4918.

Related to #4127.

Conflicts:
	actionpack/test/dispatch/mime_type_test.rb
816257d
@tamird tamird referenced this pull request from a commit in square/rails
@steveklabnik steveklabnik Added parsing of arbitrary media type parameters.
Based on #4918.

Related to #4127.

Conflicts:
	actionpack/test/dispatch/mime_type_test.rb
965358e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 actionpack/lib/action_dispatch/http/mime_type.rb
@@ -82,6 +82,7 @@ def ==(item)
class << self
TRAILING_STAR_REGEXP = /(text|application)\/\*/
+ Q_SEPARATOR_REGEXP = /;\s*q=/
def lookup(string)
LOOKUP[string]
@@ -108,6 +109,7 @@ def register(string, symbol, mime_type_synonyms = [], extension_synonyms = [], s
def parse(accept_header)
if accept_header !~ /,/
+ accept_header = accept_header.split(Q_SEPARATOR_REGEXP).first
if accept_header =~ TRAILING_STAR_REGEXP
parse_data_with_trailing_star($1)
else
@@ -117,7 +119,7 @@ def parse(accept_header)
# keep track of creation order to keep the subsequent sort stable
list, index = [], 0
accept_header.split(/,/).each do |header|
- params, q = header.split(/;\s*q=/)
+ params, q = header.split(Q_SEPARATOR_REGEXP)
if params.present?
params.strip!
View
6 actionpack/test/dispatch/mime_type_test.rb
@@ -69,6 +69,12 @@ class MimeTypeTest < ActiveSupport::TestCase
assert_equal expect, Mime::Type.parse(accept)
end
+ test "parse single media range with q" do
@josevalim Owner

Could you please also add a test that handles multiple types with q=? Your changes also updated this part of the code, but you haven't added any tests for it. Thanks.

@scottwb
scottwb added a note

@josevalim Just above that line (line 66) there is a pre-existing test that handles multiple types with q=:

  test "parse with q" do
    accept = "text/xml,application/xhtml+xml,text/yaml; q=0.3,application/xml,text/html; q=0.8,image/png,text/plain; q=0.5,application/pdf,*/*; q=0.2"
    expect = [Mime::HTML, Mime::XML, Mime::PNG, Mime::PDF, Mime::TEXT, Mime::YAML, Mime::ALL]
    assert_equal expect, Mime::Type.parse(accept)
  end

This test passes before and after my change. Or am I misunderstanding your request?

@josevalim Owner

That was it, thanks! Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ accept = "text/html;q=0.9"
+ expect = [Mime::HTML]
+ assert_equal expect, Mime::Type.parse(accept)
+ end
+
# Accept header send with user HTTP_USER_AGENT: Sunrise/0.42j (Windows XP)
test "parse broken acceptlines" do
accept = "text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/*,,*/*;q=0.5"
Something went wrong with that request. Please try again.