Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Prevent raising EOFError on multipart GET request. #8476

Merged
merged 1 commit into from Dec 10, 2012

Conversation

Projects
None yet
4 participants
Contributor

sheerun commented Dec 10, 2012

Such request can happen on Internet Explorer. When we redirect
after multipart form submission, the request type is changed
to GET, but Content-Type is preserved as multipart. GET request
cannot have multipart body and that caused Rails to fail.

It's similar fix to Rack's one.


This need to be back ported to older Rails. How to do it?

Contributor

sheerun commented Dec 10, 2012

I've checked. It can be backported without any merge problems. Just cherry-pick.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Dec 10, 2012

...est/dispatch/request/multipart_params_parsing_test.rb
@@ -123,6 +123,20 @@ def teardown
end
end
+ # This can happen in Internet Explorer
@carlosantoniodasilva

carlosantoniodasilva Dec 10, 2012

Owner

The comment can be in one line.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Dec 10, 2012

...est/dispatch/request/multipart_params_parsing_test.rb
@@ -123,6 +123,20 @@ def teardown
end
end
+ # This can happen in Internet Explorer
+ # when redirecting after multipart form submit.
+ test "does not raise EOFError on GET request with mulipart content-type" do

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Dec 10, 2012

...est/dispatch/request/multipart_params_parsing_test.rb
@@ -123,6 +123,20 @@ def teardown
end
end
+ # This can happen in Internet Explorer
+ # when redirecting after multipart form submit.
+ test "does not raise EOFError on GET request with mulipart content-type" do
+ with_routing do |set|
+ set.draw do
+ get ':action', :to => 'multipart_params_parsing_test/test'
@carlosantoniodasilva

carlosantoniodasilva Dec 10, 2012

Owner

Please use 1.9 hash style.

It's also going to need a changelog entry.

/cc @rafaelfranca wdyt?

Contributor

sheerun commented Dec 10, 2012

Do you add changelog or do I? What about backport changelog?

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Dec 10, 2012

...est/dispatch/request/multipart_params_parsing_test.rb
@@ -123,6 +123,19 @@ def teardown
end
end
+ # This can happen in Internet Explorer when redirecting after multipart form submit.
+ test "does not raise EOFError on GET request with multipart content-type" do
+ with_routing do |set|
+ set.draw do
+ get ':action', to: 'multipart_params_parsing_test/test'
+ end
+ headers = { "CONTENT_TYPE" => "multipart/form-data; boundary=AaB03x" }
+ get "/parse", {}, headers
+ assert_response :ok
+ TestController.last_request_parameters
@rafaelfranca

rafaelfranca Dec 10, 2012

Owner

Should not we doing an assertion here?

@sheerun

sheerun Dec 10, 2012

Contributor

Yeah... Stupid copy-paste mistake. I've pushed fix.

Owner

rafaelfranca commented Dec 10, 2012

@sheerun please add the changelog entry. When we backport we will use it.

@sheerun sheerun Prevent raising EOFError on multipart GET request.
Such request can happen on Internet Explorer. When we redirect
after multipart form submission, the request type is changed
to GET, but Content-Type is preserved as multipart. GET request
cannot have multipart body and that caused Rails to fail.

It's similar fix to Rack's one:
https://github.com/chneukirchen/rack/blob/8025a4ae9477d1e6231344c2b7d795aa9b3717b6/lib/rack/request.rb#L224
bc254cc
Contributor

sheerun commented Dec 10, 2012

done

@rafaelfranca rafaelfranca added a commit that referenced this pull request Dec 10, 2012

@rafaelfranca rafaelfranca Merge pull request #8476 from sheerun/fix/multipart-get
Prevent raising EOFError on multipart GET request.
b04fe4c

@rafaelfranca rafaelfranca merged commit b04fe4c into rails:master Dec 10, 2012

Owner

rafaelfranca commented Dec 10, 2012

Thanks

@frodsan frodsan commented on the diff Dec 10, 2012

actionpack/CHANGELOG.md
@@ -722,4 +722,6 @@
* `ActionView::Helpers::TextHelper#highlight` now defaults to the
HTML5 `mark` element. *Brian Cardarella*
+* Prevent raising EOFError on multipart GET request (IE issue). *Adam Stankiewicz*
@frodsan

frodsan Dec 10, 2012

Contributor

Move CHANGELOG entry to the top.

@rafaelfranca

rafaelfranca Dec 10, 2012

Owner

I'm doing this

@frodsan

frodsan Dec 10, 2012

Contributor

👍

Contributor

sheerun commented Jan 9, 2013

What about backport?

Contributor

sheerun commented Apr 3, 2013

@frodsan @rafaelfranca Is this going to be backported? I'd like to not to use my fork.

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