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

Rails ignoring provided content-type parameters other than charset #35709

Closed
McPolemic opened this issue Mar 22, 2019 · 15 comments · Fixed by #36034
Closed

Rails ignoring provided content-type parameters other than charset #35709

McPolemic opened this issue Mar 22, 2019 · 15 comments · Fixed by #36034
Milestone

Comments

@McPolemic
Copy link

@McPolemic McPolemic commented Mar 22, 2019

Steps to reproduce

In a controller, render something with a specified content-type:

render body: "test content",
       content_type: "text/html; sandwiches=tasty; charset=utf-8; birds=flying"

See https://gist.github.com/85892fccc12d05d60c3d60c76c14500e for a single-file reproduction or https://github.com/leequarella/content-type-rails-bug for a rails app reproducing the problem.

Expected behavior

It includes all key/value pairs sandwiches=tasty and birds=flying in the Content-Type header returned to the browser.

Actual behavior

It strips out all key/value pairs and returns a content-type of `text/html; charset=utf-8" to the browser.

System configuration

Rails version:
Current master (4c8a333)

Ruby version:
2.5.1

Related PRs

#35549 - This seems to have started the behavior when it fixed certain issues with the character set portion of the content-type
#35706 - This fixed the issue in Rails 5.2 but not Rails 6.0

/cc @leequarella, @cpruitt, @jhawthorn

@rafaelfranca rafaelfranca added this to the 6.0.0 milestone Mar 22, 2019
@McPolemic McPolemic changed the title Rails 6 is ignoring all content-type parameters other than charset Rails ignoring provided content-type parameters other than charset Mar 22, 2019
@themilkman

This comment has been minimized.

Copy link

@themilkman themilkman commented Apr 3, 2019

ActionDispatch#parse_content_type drops required parts since 5.2.3

Now, after (! ... sad face) diving deep inside this I found this issue/revert commit. It's a bit sad that the revert didn't make it in the 5.2.3 release. But since I spent much time in this (and wrote the text) I do at least want to add my scenario here to acknowledge that the new behaviour was erroneous.


We generate multipart files in one of our APIs to deliver bundles of files.
Therefore in the response we set a HTTP header with the boundary in the Content-Type field ([0]) like that for years:

Content-Type: multipart/mixed; boundary=-----------b071530f6845c0da64f16186e351be07aaa0b958

After upgrading from 5.2.2.1 => 5.2.3 (thus a tiny level upgrade) and quite a few hours of searching an debugging reproducable crashes in our client app I found that the header now looks like:
Content-Type: multipart/mixed

This is a result of changes done in rails/actionpack/lib/action_dispatch/http/response.rb at commit 29b42f5 .

Old behaviour:

content_type = 'multipart/mixed;' + " boundary=-----42"
type, charset = content_type.split(/;\s*charset=/)
=> ["multipart/mixed; boundary=-----42"]

New behaviour:

CONTENT_TYPE_PARSER = /\A(?<type>[^;\s]+)?(?:.*;\s*charset=(?<quote>"?)(?<charset>[^;\s]+)\k<quote>)?/ # :nodoc:
content_type && match = CONTENT_TYPE_PARSER.match(content_type)

match[:type]
=> "multipart/mixed"

match[:charset]
=> nil

[0]Some RFC/Standards for reference:
https://tools.ietf.org/html/rfc2046#section-5.1.1
https://www.ietf.org/rfc/rfc2045.txt

@mileslane

This comment has been minimized.

Copy link

@mileslane mileslane commented Apr 3, 2019

In our application, we use "application/fhir+json; fhirVersion=#{fhir_version}" to specify the version of the FHIR protocol we are supporting for a transaction.

https://www.hl7.org/fhir/http.html#version-parameter

@armilam

This comment has been minimized.

Copy link

@armilam armilam commented Apr 5, 2019

This change in how content-type is parsed for a response also affected us: lessonly/scim_rails#9 (comment)

We discovered we were technically doing something wrong and that we technically should not be comma-separating content-type values, but this does technically constitute a change in how the ActionDispatch::Request object behaves. Because of that, I'm not sure whether it would be considered that #35709 fixed a bug or whether it introduced a regression.

In our case, we saw that if the Content-Type was formatted with a comma and space separating two values, then the second value is now getting cut off as of #35709. In our case, this is how we are rendering:

render content_type: "application/scim+json, application/json"

This was sending a Content-Type header of "application/scim+json, application/json" in the response before. Now, the value of the Content-Type header is "application/scim+json," (notice the trailing comma and missing second value).

@kaspth kaspth modified the milestones: 6.0.0, 6.x Apr 17, 2019
y-yagi added a commit to y-yagi/rails that referenced this issue Apr 19, 2019
Since rails#35709, Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return Content-Type
that does not contain charset. This seems to be breaking changes.

We can change this behavior with the deprecate cycle.
However, in that case, a method needs that include Content type with
additional parameters. And that method is probably the `content_type`
seems to properly.

So I changed the new behavior to another method(`mime_type`) and restored
`Response#conten_type` to the original behavior

The original `conten_type` method seems to have some problems with
charset parsing (it implicitly expects charset to be at the end). So I
think we should consider the appropriate behavior again later.

Fixes rails#35709.
y-yagi added a commit to y-yagi/rails that referenced this issue Apr 19, 2019
Since rails#35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.

We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.

So I changed the new behavior to more appropriate another
method(`mime_type`) and restored `Response#conten_type` to the original
behavior.

The original `conten_type` method seems to have some problems with
charset parsing (it implicitly expects charset to be at the end). So I
think we should consider the appropriate behavior again later.

Fixes rails#35709.
y-yagi added a commit to y-yagi/rails that referenced this issue Apr 19, 2019
Since rails#35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.

We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.

So I changed the new behavior to more appropriate another
method(`mime_type`) and restored `Response#conten_type` to the original
behavior.

The original `conten_type` method seems to have some problems with
charset parsing (it implicitly expects charset to be at the end). So I
think we should consider the appropriate behavior again later.

Fixes rails#35709.
y-yagi added a commit to y-yagi/rails that referenced this issue Apr 28, 2019
Since rails#35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.

We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.

So I changed the new behavior to more appropriate another
method(`mime_type`) and restored `Response#conten_type` to the original
behavior.

The original `conten_type` method seems to have some problems with
charset parsing (it implicitly expects charset to be at the end). So I
think we should consider the appropriate behavior again later.

Fixes rails#35709.
y-yagi added a commit to y-yagi/rails that referenced this issue May 2, 2019
… header as it is

Since rails#35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.

We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.

So I changed the new behavior to more appropriate `media_type` method.
And `Response#content_type` changed (as the method name) to return Content-Type
header as it is.

Fixes rails#35709.

[Rafael Mendonça França & Yuuji Yaginuma ]
y-yagi added a commit to y-yagi/rails that referenced this issue May 2, 2019
… header as it is

Since rails#35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.

We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.

So I changed the new behavior to more appropriate `media_type` method.
And `Response#content_type` changed (as the method name) to return Content-Type
header as it is.

Fixes rails#35709.

[Rafael Mendonça França & Yuuji Yaginuma ]
y-yagi added a commit to y-yagi/rails that referenced this issue May 2, 2019
… header as it is

Since rails#35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.

We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.

So I changed the new behavior to more appropriate `media_type` method.
And `Response#content_type` changed (as the method name) to return Content-Type
header as it is.

Fixes rails#35709.

[Rafael Mendonça França & Yuuji Yaginuma ]
y-yagi added a commit to y-yagi/rails that referenced this issue May 2, 2019
… header as it is

Since rails#35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.

We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.

So I changed the new behavior to more appropriate `media_type` method.
And `Response#content_type` changed (as the method name) to return Content-Type
header as it is.

Fixes rails#35709.

[Rafael Mendonça França & Yuuji Yaginuma ]
y-yagi added a commit to y-yagi/rails that referenced this issue Jun 1, 2019
… header as it is

Since rails#35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.

We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.

So I changed the new behavior to more appropriate `media_type` method.
And `Response#content_type` changed (as the method name) to return Content-Type
header as it is.

Fixes rails#35709.

[Rafael Mendonça França & Yuuji Yaginuma ]
@y-yagi y-yagi closed this in #36034 Jun 1, 2019
yoones added a commit to yoones/rails that referenced this issue Jul 16, 2019
… header as it is

Since rails#35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
https://github.com/rails/rails/blob/39de7fac0507070e3c5f8b33fbad6fced84d97ed/actionpack/lib/action_dispatch/http/response.rb#L245-L249

But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.

We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.

So I changed the new behavior to more appropriate `media_type` method.
And `Response#content_type` changed (as the method name) to return Content-Type
header as it is.

Fixes rails#35709.

[Rafael Mendonça França & Yuuji Yaginuma ]
@themilkman

This comment has been minimized.

Copy link

@themilkman themilkman commented Aug 21, 2019

Will this fix be included in a 5.2.4 release? (Any ETA?) Currently, I am still stuck with 5.2.2.1.
Thanks for any response!

@themilkman

This comment has been minimized.

Copy link

@themilkman themilkman commented Oct 8, 2019

@y-yagi Sorry for pushing, but this is important. Any plans? Rails team?

@y-yagi

This comment has been minimized.

Copy link
Member

@y-yagi y-yagi commented Oct 8, 2019

@themilkman The issue fixed by #35706. It is going to include in 5.2.4.
But we already released 6.0.0. Please consider upgrading to that. Thanks.

@themilkman

This comment has been minimized.

Copy link

@themilkman themilkman commented Oct 9, 2019

@themilkman The issue fixed by #35706. It is going to include in 5.2.4.

Thanks for your response, @y-yagi. Looking forward to it.

But we already released 6.0.0. Please consider upgrading to that. Thanks.

Yes, but as mentioned in 36034 #36034 (comment) I observe the same behavior in Rails 6 which is problematic for our software. Could you please question in the referenced comment, to be honest I am confused by all the commits around this topic. Thanks in advance!

After that I'll happily finish the upgrade of our apps :)

@y-yagi

This comment has been minimized.

Copy link
Member

@y-yagi y-yagi commented Oct 9, 2019

I changed that behavior is configured by #36490.
You can use old behavior when set Rails.application.config.action_dispatch.return_only_media_type_on_content_type .

@themilkman

This comment has been minimized.

Copy link

@themilkman themilkman commented Oct 9, 2019

Thank you for your fast reply, @y-yagi !
I just ran a test with my rails 6.0.0 branch and set
config.action_dispatch.return_only_media_type_on_content_type = true
in my application.rb (and to be sure also in my environment file), and even set it to false. But the behaviour does not change at all, I always receive a
Content-Type →multipart/mixed
response header instead of the intended full content-type including my boundary as it worked in Rails 5.2.2.1 and below.

Am I missing something? :(

@y-yagi

This comment has been minimized.

Copy link
Member

@y-yagi y-yagi commented Oct 10, 2019

@themilkman Could you try to set ActionDispatch::Response.return_only_media_type_on_content_type = true?

@themilkman

This comment has been minimized.

Copy link

@themilkman themilkman commented Oct 11, 2019

ActionDispatch::Response.return_only_media_type_on_content_type = true?

Unfortunately no change, I did even insert your line to the start of the called action. Still, the Content-Type is cut. :-(

@DylanReile

This comment has been minimized.

Copy link
Contributor

@DylanReile DylanReile commented Oct 15, 2019

@y-yagi

If I set Rails.application.config.action_dispatch.return_only_media_type_on_content_type = true

then ActionDispatch::Response#content_type returns "text/html; as expected.

However, cURL still shows the full header Content-Type: text/html; charset=utf-8.

Is that the expected behavior? Shouldn't the response header be limited to the media type as well?

@Zyndoras

This comment has been minimized.

Copy link

@Zyndoras Zyndoras commented Oct 22, 2019

@y-yagi the return_only_media_type_on_content_type-option has no effect because when using

send_file(file, type: 'multipart/mixed; boundary=-----------42')
  1. The content_type method (the only place where i found that option) does not get invoked at all.
  2. If invoking it anyway, you can see that other parts of the Content-Type header are already stripped off in super.

So, for anyone using 5.2.3 or 6.0.0, here is my workaround to get the old behavior:

send_file(file)
response.set_header('Content-Type', 'multipart/mixed; boundary=-----------42')

Setting the return_only_media_type_on_content_type-flag is not required, as it has no impact.

@themilkman

This comment has been minimized.

Copy link

@themilkman themilkman commented Nov 24, 2019

@rafaelfranca @y-yagi I don't see any note for this on the 5.2.4 release notes, nevertheless I think (or at least hope but my quick look in the source seemed good) that this regression was fixed, right? For me (since it costed me so many hours) it would definitely worth of noting. Thanks for any feedback!

@y-yagi

This comment has been minimized.

Copy link
Member

@y-yagi y-yagi commented Nov 24, 2019

@themilkman Yes, the fix is included in 5.2.4. Ref: cc26fc0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.