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

multipart POST - utf8 param name not encoded #5606

Merged
merged 1 commit into from Mar 15, 2013

Conversation

Projects
None yet
9 participants
@teohm
Contributor

teohm commented Mar 27, 2012

Issue

When submit a multipart form that contains a unicode param name e.g.

<form method="post" enctype="multipart/form-data" action="..">
  <input name="Iñtërnâtiônàlizætiøn_name" value="Iñtërnâtiônàlizætiøn_value" />
</form>

Controller receives the param value as "Iñtërnâtiônàlizætiøn_value", as expected. But the param name becomes "I\xC3\xB1t\xC3\xABrn\xC3\xA2ti\xC3\xB4n\xC3\xA0liz\xC3\xA6ti\xC3\xB8n_name".

Failed Tests

2 failed tests are added in the pull request.

Test results:

$ cd rails/actionpack
$ rake test TEST=test/dispatch/request/multipart_params_parsing_test.rb 

# Running tests:
.FF..........

Finished tests in 0.542967s, 23.9425 tests/s, 88.4032 assertions/s.

  1) Failure:
test_parse_bracketed_utf8_parameter(MultipartParamsParsingTest) [rails/actionpack/test/dispatch/request/multipart_params_parsing_test.rb:41]:
--- expected
+++ actual
@@ -1 +1 @@
-{"Iñtërnâtiônàlizætiøn_name"=>{"Iñtërnâtiônàlizætiøn_nested_name"=>"Iñtërnâtiônàlizætiøn_value"}}
+{"I\xC3\xB1t\xC3\xABrn\xC3\xA2ti\xC3\xB4n\xC3\xA0liz\xC3\xA6ti\xC3\xB8n_name"=>{"I\xC3\xB1t\xC3\xABrn\xC3\xA2ti\xC3\xB4n\xC3\xA0liz\xC3\xA6ti\xC3\xB8n_nested_name"=>"Iñtërnâtiônàlizætiøn_value"}}

  2) Failure:
test_parse_single_utf8_parameter(MultipartParamsParsingTest) [rails/actionpack/test/dispatch/request/multipart_params_parsing_test.rb:35]:
--- expected
+++ actual
@@ -1 +1 @@
-{"Iñtërnâtiônàlizætiøn_name"=>"Iñtërnâtiônàlizætiøn_value"}
+{"I\xC3\xB1t\xC3\xABrn\xC3\xA2ti\xC3\xB4n\xC3\xA0liz\xC3\xA6ti\xC3\xB8n_name"=>"Iñtërnâtiônàlizætiøn_value"}

Cause

I spent some time troubleshoot the issue, and found that ActionDispatch::Http::Parameters#encode_params (in actionpack/lib/action_dispatch/http/parameters.rb) only encodes param values, but not param keys. Is such behavior intended?

@krsmurata

This comment has been minimized.

Show comment
Hide comment
@krsmurata

krsmurata Mar 27, 2012

Interesting, it only happens for multipart forms, I can confirm this bug on my end.

krsmurata commented Mar 27, 2012

Interesting, it only happens for multipart forms, I can confirm this bug on my end.

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Mar 27, 2012

Contributor

FYI, it only happens for multipart form because this is what Rack returns to Rails:

{ "I\xC3\xB1t\xC3\xABrn\xC3\xA2ti\xC3\xB4n\xC3\xA0liz\xC3\xA6ti\xC3\xB8n_name" =>   
  "I\xC3\xB1t\xC3\xABrn\xC3\xA2ti\xC3\xB4n\xC3\xA0liz\xC3\xA6ti\xC3\xB8n_value" }

and in Rails, ActionDispatch::Http::Parameters#encode_params decided to only encode param value, but not param name. As a result we got:

{ "I\xC3\xB1t\xC3\xABrn\xC3\xA2ti\xC3\xB4n\xC3\xA0liz\xC3\xA6ti\xC3\xB8n_name" =>
  "Iñtërnâtiônàlizætiøn_value" }

Hope this helps to clarify the root cause.

Contributor

teohm commented Mar 27, 2012

FYI, it only happens for multipart form because this is what Rack returns to Rails:

{ "I\xC3\xB1t\xC3\xABrn\xC3\xA2ti\xC3\xB4n\xC3\xA0liz\xC3\xA6ti\xC3\xB8n_name" =>   
  "I\xC3\xB1t\xC3\xABrn\xC3\xA2ti\xC3\xB4n\xC3\xA0liz\xC3\xA6ti\xC3\xB8n_value" }

and in Rails, ActionDispatch::Http::Parameters#encode_params decided to only encode param value, but not param name. As a result we got:

{ "I\xC3\xB1t\xC3\xABrn\xC3\xA2ti\xC3\xB4n\xC3\xA0liz\xC3\xA6ti\xC3\xB8n_name" =>
  "Iñtërnâtiônàlizætiøn_value" }

Hope this helps to clarify the root cause.

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Mar 27, 2012

Contributor

The pull request was a bit messed up with an accident rebase, sorry.

Anyway, I prepared a separate branch, along with a proof-of-concept fix that passes failing tests.

https://github.com/teohm/rails/compare/multipart_utf8_param

Contributor

teohm commented Mar 27, 2012

The pull request was a bit messed up with an accident rebase, sorry.

Anyway, I prepared a separate branch, along with a proof-of-concept fix that passes failing tests.

https://github.com/teohm/rails/compare/multipart_utf8_param

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Mar 27, 2012

Member

@teohm you can try rebasing again from current master, and push forcing to your pull request branch. It should cleanup the current state.

Member

carlosantoniodasilva commented Mar 27, 2012

@teohm you can try rebasing again from current master, and push forcing to your pull request branch. It should cleanup the current state.

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Mar 27, 2012

Contributor

@carlosantoniodasilva I'm not sure how to do that, can you guide me through the steps? Or is it easier if I re-submit the pull request from the new branch I prepared?

Contributor

teohm commented Mar 27, 2012

@carlosantoniodasilva I'm not sure how to do that, can you guide me through the steps? Or is it easier if I re-submit the pull request from the new branch I prepared?

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Mar 27, 2012

Member

@teohm I think it should look something like:

# in rails dir - grab current master - considering `origin` is your rails remote
git checkout master
git pull origin master
# back to this branch locally
git checkout multipart_unicode_param_name 
git rebase master
# should be clean now - you can check with `git log` or `git diff master`

# update your remote branch - should reflect in this pr in a few minutes
git push --force teohm multipart_unicode_param_name

I think no conflict should happen with you run the rebase, but it it happens, you'll have to solve them, add with git add . and run git rebase --continue to finish the rebase. If that does not solve, just close this and open a new one :)

Member

carlosantoniodasilva commented Mar 27, 2012

@teohm I think it should look something like:

# in rails dir - grab current master - considering `origin` is your rails remote
git checkout master
git pull origin master
# back to this branch locally
git checkout multipart_unicode_param_name 
git rebase master
# should be clean now - you can check with `git log` or `git diff master`

# update your remote branch - should reflect in this pr in a few minutes
git push --force teohm multipart_unicode_param_name

I think no conflict should happen with you run the rebase, but it it happens, you'll have to solve them, add with git add . and run git rebase --continue to finish the rebase. If that does not solve, just close this and open a new one :)

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Mar 28, 2012

Contributor

Thanks @carlosantoniodasilva! I believe the branch is fixed now. :)

Contributor

teohm commented Mar 28, 2012

Thanks @carlosantoniodasilva! I believe the branch is fixed now. :)

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Mar 28, 2012

Member

No problem :). I'll try to take a look later at the fix, thanks.

At.
Carlos Antonio

On Tuesday, March 27, 2012 at 9:31 PM, Huiming Teo wrote:

Thanks @carlosantoniodasilva! I believe the branch is fixed now. :)


Reply to this email directly or view it on GitHub:
#5606 (comment)

Member

carlosantoniodasilva commented Mar 28, 2012

No problem :). I'll try to take a look later at the fix, thanks.

At.
Carlos Antonio

On Tuesday, March 27, 2012 at 9:31 PM, Huiming Teo wrote:

Thanks @carlosantoniodasilva! I believe the branch is fixed now. :)


Reply to this email directly or view it on GitHub:
#5606 (comment)

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Apr 7, 2012

Contributor

Hi, any updates :)

Contributor

teohm commented Apr 7, 2012

Hi, any updates :)

@steveklabnik

View changes

Show outdated Hide outdated actionpack/lib/action_dispatch/http/parameters.rb
@steveklabnik

View changes

Show outdated Hide outdated actionpack/lib/action_dispatch/http/parameters.rb
@steveklabnik

View changes

Show outdated Hide outdated actionpack/lib/action_dispatch/http/request.rb
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 14, 2012

Member

I think this is something that needs to be fixed, but I'm not sure about the implementation here.

Member

steveklabnik commented Sep 14, 2012

I think this is something that needs to be fixed, but I'm not sure about the implementation here.

@michaeleconomy

This comment has been minimized.

Show comment
Hide comment
@michaeleconomy

michaeleconomy Sep 27, 2012

I'm having a similar issue, or the same issue, I can't tell. The fix provided above didn't work for me.

What i see is that on multipart posts, the parameters are coming in as "ascii-8bit" when they should be "utf8".

Not sure why rails doesn't do this for me.

Heres a fix that does work:

https://gist.github.com/3796400

I'm not sure how to write tests for it.

michaeleconomy commented Sep 27, 2012

I'm having a similar issue, or the same issue, I can't tell. The fix provided above didn't work for me.

What i see is that on multipart posts, the parameters are coming in as "ascii-8bit" when they should be "utf8".

Not sure why rails doesn't do this for me.

Heres a fix that does work:

https://gist.github.com/3796400

I'm not sure how to write tests for it.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 18, 2012

Member

@teohm any interest in keeping this pull request up to date? What do you think about my feedback? This needs rebased as well.

Member

steveklabnik commented Nov 18, 2012

@teohm any interest in keeping this pull request up to date? What do you think about my feedback? This needs rebased as well.

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Nov 18, 2012

Contributor

@steveklabnik Ya, thanks for the feedbacks! Sorry for late reply, I wasn't aware of the updates. Let me work on it in a while. Wish to try out @michaeleconomy implementation as well.

Contributor

teohm commented Nov 18, 2012

@steveklabnik Ya, thanks for the feedbacks! Sorry for late reply, I wasn't aware of the updates. Let me work on it in a while. Wish to try out @michaeleconomy implementation as well.

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Nov 19, 2012

Contributor

@steveklabnik, I have updated the branch with the following changes:

  • rebased to rails/master (12d32dd)
  • fixed broken unit test fixtures & updated the unit tests
  • merged the implementation into encode_params()

I have simplified the implementation based on your feedbacks and @michaeleconomy's idea.

Here's the rational behind this commit:

ActionDispatch::Http::Parameters#encode_params() will now utf8-encode both keys and values in params if the key/value is a String.

The keys in params hash are frozen. The goal is to utf8-encode the keys while keeping the implementation simple to read. So instead of replacing the frozen keys in existing params hash, @steveklabnik as you suggested, encode_params() will now create and return new hash.

As a result, it should not be called in:

ActionDispatch::Http::Parameters#parameters

Instead, it has to be called in:

ActionDispatch::Http::Request#GET
ActionDispatch::Http::Request#POST

So that the utf8 encoding is applied to all 3 @env vars:

@env["action_dispatch.request.query_parameters"]
@env["action_dispatch.request.request_parameters"]
@env["action_dispatch.request.parameters"]

If it's only called in ActionDispatch::Http::Parameters#parameters, the encoding will only be applied to @env["action_dispatch.request.parameters"].

What do you guys think?

Contributor

teohm commented Nov 19, 2012

@steveklabnik, I have updated the branch with the following changes:

  • rebased to rails/master (12d32dd)
  • fixed broken unit test fixtures & updated the unit tests
  • merged the implementation into encode_params()

I have simplified the implementation based on your feedbacks and @michaeleconomy's idea.

Here's the rational behind this commit:

ActionDispatch::Http::Parameters#encode_params() will now utf8-encode both keys and values in params if the key/value is a String.

The keys in params hash are frozen. The goal is to utf8-encode the keys while keeping the implementation simple to read. So instead of replacing the frozen keys in existing params hash, @steveklabnik as you suggested, encode_params() will now create and return new hash.

As a result, it should not be called in:

ActionDispatch::Http::Parameters#parameters

Instead, it has to be called in:

ActionDispatch::Http::Request#GET
ActionDispatch::Http::Request#POST

So that the utf8 encoding is applied to all 3 @env vars:

@env["action_dispatch.request.query_parameters"]
@env["action_dispatch.request.request_parameters"]
@env["action_dispatch.request.parameters"]

If it's only called in ActionDispatch::Http::Parameters#parameters, the encoding will only be applied to @env["action_dispatch.request.parameters"].

What do you guys think?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Nov 19, 2012

Member

It is good to me. But I want to get feedback from some Rack expert.

@josevalim @tenderlove mind to review this pull request?

Member

rafaelfranca commented Nov 19, 2012

It is good to me. But I want to get feedback from some Rack expert.

@josevalim @tenderlove mind to review this pull request?

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Nov 29, 2012

Contributor

Hi, anything I can help?

Contributor

teohm commented Nov 29, 2012

Hi, anything I can help?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 19, 2012

Contributor

This looks good but I would suggest folding the normalize_parameters loop into the encode_params so we avoid iterating all parameters twice and creating the hashes twice.

Contributor

josevalim commented Dec 19, 2012

This looks good but I would suggest folding the normalize_parameters loop into the encode_params so we avoid iterating all parameters twice and creating the hashes twice.

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Dec 19, 2012

Contributor

@josevalim Thanks for the feedback. I have merged normalize_parameters and encode_params into single function i.e. normalize_encode_params (any better name?)

Now it just iterates and re-creates the params hash once.

I have rebased it to latest master branch and ran the actionpack test suite.

Contributor

teohm commented Dec 19, 2012

@josevalim Thanks for the feedback. I have merged normalize_parameters and encode_params into single function i.e. normalize_encode_params (any better name?)

Now it just iterates and re-creates the params hash once.

I have rebased it to latest master branch and ran the actionpack test suite.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 19, 2012

Member

@teohm also please squash your commits and push force to your branch, to update the pull request. Thanks.

Member

carlosantoniodasilva commented Dec 19, 2012

@teohm also please squash your commits and push force to your branch, to update the pull request. Thanks.

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Dec 19, 2012

Contributor

@carlosantoniodasilva Squashed and pushed. :)

Contributor

teohm commented Dec 19, 2012

@carlosantoniodasilva Squashed and pushed. :)

@goshakkk

This comment has been minimized.

Show comment
Hide comment
@goshakkk

goshakkk Jan 10, 2013

Contributor

Is it still alive?

Contributor

goshakkk commented Jan 10, 2013

Is it still alive?

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Jan 10, 2013

Contributor

Yeah, I'm actually waiting for review. Anything I can help to close this
issue?

On Friday, January 11, 2013, Gosha Arinich wrote:

Is it still alive?


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/5606#issuecomment-12115773.

Huiming

Contributor

teohm commented Jan 10, 2013

Yeah, I'm actually waiting for review. Anything I can help to close this
issue?

On Friday, January 11, 2013, Gosha Arinich wrote:

Is it still alive?


Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/pull/5606#issuecomment-12115773.

Huiming

@goshakkk

This comment has been minimized.

Show comment
Hide comment
@goshakkk

goshakkk Jan 10, 2013

Contributor

It seems good to me.

I think the only thing you can do is cc someone from Rails Core to remind them of/introduce them to this PR. It's been hanging there for about an year, still isn't merged.

Contributor

goshakkk commented Jan 10, 2013

It seems good to me.

I think the only thing you can do is cc someone from Rails Core to remind them of/introduce them to this PR. It's been hanging there for about an year, still isn't merged.

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Jan 11, 2013

Contributor

Rebased on latest rails master branch.

@carlosantoniodasilva, do you think we can merge this PR and close the issue?

Contributor

teohm commented Jan 11, 2013

Rebased on latest rails master branch.

@carlosantoniodasilva, do you think we can merge this PR and close the issue?

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Mar 14, 2013

Member

2 months later, I guess it need to be again rebased. @teohm can you do this? Thanks.

Looks like this one is ready. @carlosantoniodasilva what do you think? Can we merge it?

Member

lukaszx0 commented Mar 14, 2013

2 months later, I guess it need to be again rebased. @teohm can you do this? Thanks.

Looks like this one is ready. @carlosantoniodasilva what do you think? Can we merge it?

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Mar 15, 2013

Contributor

@strzalek @carlosantoniodasilva Hey guys, rebase done, and rake test passed. Anyone from core team is kind enough to review and merge this back to master branch?

Contributor

teohm commented Mar 15, 2013

@strzalek @carlosantoniodasilva Hey guys, rebase done, and rake test passed. Anyone from core team is kind enough to review and merge this back to master branch?

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Mar 15, 2013

Member

@teohm thanks for taking quick action.

I've reviewed code, test also passes for me. I think it's good to go. @steveklabnik what do you think? Can you merge it?

Member

lukaszx0 commented Mar 15, 2013

@teohm thanks for taking quick action.

I've reviewed code, test also passes for me. I think it's good to go. @steveklabnik what do you think? Can you merge it?

steveklabnik added a commit that referenced this pull request Mar 15, 2013

Merge pull request #5606 from teohm/multipart_unicode_param_name
multipart POST - utf8 param name not encoded

@steveklabnik steveklabnik merged commit 29030d3 into rails:master Mar 15, 2013

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Mar 15, 2013

Member

Looks good. Thank you for working through multiple rounds of feedback on this one, i18n is a big deal. ❤️

Member

steveklabnik commented Mar 15, 2013

Looks good. Thank you for working through multiple rounds of feedback on this one, i18n is a big deal. ❤️

@teohm

This comment has been minimized.

Show comment
Hide comment
@teohm

teohm Mar 15, 2013

Contributor

Indeed, thanks for everyone's help and feedback over the year 👍 :-)

Contributor

teohm commented Mar 15, 2013

Indeed, thanks for everyone's help and feedback over the year 👍 :-)

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