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

improve quoted parameters in mime types #48397

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

ThunderKey
Copy link
Contributor

Motivation / Background

This Pull Request has been created because Mime::Type does not correctly handle quoted parameters. They can contain most characters, including a comma. Additionally, the current implementation ignored the parameters when loading the type, but not when registering it.

Fixes #48052

Detail

This Pull Request changes Mime::Type to allow every character in a quoted parameter value, except \r, \ and " as defined in RFC 822 3.3, except for escaped characters (see Current Limitations).

Additionally, the different parts of the Accept header are not simply split by ,, but now factor in quotes.

In the current implementation Mime::Type.register kept the parameters, whereas Mime::Type.parse removed them. This PR now keeps all parameters before the q parameter (as defined in RFC 7231 5.3.2). If during the lookup no type with the parameter could be found, a type is searched for without parameters as a fallback. This is useful for example for multipart/form-data; boundary="simple boundary" and keeps the current behaviour.

Current Limitations

Currently the following limitations exist, but I'm not sure if they are worth the effort:

  • Quoted strings could contain escaped characters, including \". My implementation does not allow a backslash to simplify the parsing and keeping it efficient.
  • If there are whitespace-differences around the parameters they would not be considered as matching. E.g. my/type;test="value" would not match my/type; test="value"
  • According to RFC 7231 5.3.2, the types with parameters have precedence over types without parameters. But sorting them according to this information would mean to split the types from the parameters, which introduces additional computational effort.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Accept headers allow parameters to be passed. They can contain quotes
that need to be handled differently. These quoted strings can contain
commas, which are not considered as delimiters of accept headers.

Additionally, all parameters before the q-parameter should be used to
lookup the media-type as well. If no media-type with the parameters is
found, a fallback is introduced to the media-type without any parameters
to keep the same functionality as before.

Fix rails#48052
@rails-bot rails-bot bot added the actionpack label Jun 5, 2023
@ThunderKey
Copy link
Contributor Author

I'm confused by the error I get in the CI. I cannot reproduce this locally.

Failure:
TranslationHelperTest#test_raise_arg_overrides_raise_config_option [/rails/actionview/test/template/translation_helper_test.rb:134]:
--- expected
+++ actual
@@ -1 +1 @@
-"translation missing: en.translations.missing"
+"Translation missing: en.translations.missing"

I do get an error locally for the following test:

def test_should_render_formatted_html_erb_template_with_bad_accepts_header
@request.env["HTTP_ACCEPT"] = "; a=dsf"
get :formatted_xml_erb
assert_equal "<test>passed formatted HTML erb</test>", @response.body
end

Is this intended behaviour? This worked before specifically for strings that start with ;. All other invalid values (like a=dsf) did raise the same error it raises now (e.g. ActionDispatch::Http::MimeNegotiation::InvalidType: "a=dsf" is not a valid MIME type).

@guilleiguaran
Copy link
Member

The error is unrelated to your changes, you can rebase your branch with the main branch and it will disappear.

@rafaelfranca rafaelfranca merged commit 0e18003 into rails:main Jun 5, 2023
8 of 9 checks passed
@skipkayhil
Copy link
Member

skipkayhil commented Jun 5, 2023

FYI there are real test failures from this change:

https://buildkite.com/rails/rails/builds/96949#01888c7e-e00d-439f-8363-fe831c318682/1059-1133

RenderTest#test_should_render_formatted_html_erb_template_with_bad_accepts_header:
ActionDispatch::Http::MimeNegotiation::InvalidType: "; a=dsf" is not a valid MIME type
    /rails/actionpack/lib/action_dispatch/http/mime_negotiation.rb:65:in `rescue in block in accepts'
    /rails/actionpack/lib/action_dispatch/http/mime_negotiation.rb:56:in `block in accepts'
    /usr/local/bundle/gems/rack-2.2.7/lib/rack/request.rb:69:in `fetch'
    /usr/local/bundle/gems/rack-2.2.7/lib/rack/request.rb:69:in `fetch_header'
    /rails/actionpack/lib/action_dispatch/http/mime_negotiation.rb:55:in `accepts'
    /rails/actionpack/lib/action_dispatch/http/mime_negotiation.rb:84:in `block in formats'
    /usr/local/bundle/gems/rack-2.2.7/lib/rack/request.rb:69:in `fetch'
    /usr/local/bundle/gems/rack-2.2.7/lib/rack/request.rb:69:in `fetch_header'
    /rails/actionpack/lib/action_dispatch/http/mime_negotiation.rb:80:in `formats'
    /rails/actionpack/lib/action_dispatch/http/mime_negotiation.rb:76:in `format'
    /rails/actionpack/lib/action_controller/metal/instrumentation.rb:66:in `process_action'
    /rails/actionpack/lib/action_controller/metal/params_wrapper.rb:261:in `process_action'
    /rails/actionpack/lib/abstract_controller/base.rb:158:in `process'
    /rails/actionview/lib/action_view/rendering.rb:40:in `process'
    /rails/actionpack/lib/action_controller/metal.rb:227:in `dispatch'
    /rails/actionpack/lib/action_controller/test_case.rb:611:in `block in process_controller_response'
    /rails/actionpack/lib/action_controller/test_case.rb:603:in `wrap_execution'
    /rails/actionpack/lib/action_controller/test_case.rb:611:in `process_controller_response'
    /rails/actionpack/lib/action_controller/test_case.rb:521:in `process'
    /rails/actionpack/lib/action_controller/test_case.rb:414:in `get'
    test/actionpack/controller/render_test.rb:1087:in `test_should_render_formatted_html_erb_template_with_bad_accepts_header'

The failure is due to Mime::Type.parse("; a=dsf") now raising, whereas it previously returned an empty array.

rafaelfranca added a commit that referenced this pull request Jun 5, 2023
This test was failing because the parameter `a` in the accept header
is now being correctly extracted as a mime type.

This test was expecting that Rails would extract it an an empty accept
header, that will only happen now if the `q` parameter that has special
meaning in used.
@ThunderKey ThunderKey deleted the improve-mime-regex branch June 6, 2023 06:35
@ThunderKey
Copy link
Contributor Author

Sorry, that I've missed this.

Thank you, @rafaelfranca, for fixing it!

notchairmk added a commit to notchairmk/rails that referenced this pull request Apr 20, 2024
Fixes MIME parsing raising errors on valid parameters rails#51594.

Mime type lookups were updated to handle custom registered types as part of rails#48397.

This fix the strips out custom media range parameters before falling back to the default type creation.
notchairmk added a commit to notchairmk/rails that referenced this pull request Apr 22, 2024
Fixes MIME parsing raising errors on valid parameters rails#51594.

Mime type lookups were updated to handle custom registered types as part of rails#48397.

This fix the strips out custom media range parameters before falling back to the default type creation.
fractaledmind pushed a commit to fractaledmind/rails that referenced this pull request May 13, 2024
Fixes MIME parsing raising errors on valid parameters rails#51594.

Mime type lookups were updated to handle custom registered types as part of rails#48397.

This fix the strips out custom media range parameters before falling back to the default type creation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIME type validation too strict
4 participants