Prevent `RequestEncoder#encode_params` to parse falsey params #33392

al3rez · 2018-07-19T03:42:30Z

Fixes #33388 - When a get method called with as: :json and params: nil
(explicitly or implicitly) RequestEncoder#encode_params converts it
into a null value which includes a unexpected null= query string
into request URL. From now on RequestEncoder#encode_params checks
whether params is nil or not otherwise returns.

rails-bot · 2018-07-19T03:42:39Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

al3rez · 2018-07-19T03:44:18Z

@rafaelfranca

bogdanvlviv · 2018-07-19T11:55:50Z

actionpack/test/controller/integration_test.rb

@@ -4,6 +4,7 @@
 require "controller/fake_controllers"
 require "rails/engine"

+


Could you remove this extra line?

bogdanvlviv · 2018-07-19T12:05:24Z

actionpack/lib/action_dispatch/testing/request_encoder.rb

@@ -34,7 +34,7 @@ def accept_header
    end

    def encode_params(params)
-      @param_encoder.call(params)
+      @param_encoder.call(params) unless params.nil?


I think it should be fixed in actionpack/lib/action_dispatch/testing/integration.rb See diff:

diff --git a/actionpack/lib/action_dispatch/testing/integration.rb b/actionpack/lib/action_dispatch/testing/integration.rb index 7637febd1c..9c6075ad0c 100644 --- a/actionpack/lib/action_dispatch/testing/integration.rb +++ b/actionpack/lib/action_dispatch/testing/integration.rb @@ -233,7 +233,7 @@ def process(method, path, params: nil, headers: nil, env: nil, xhr: false, as: n request_env = { :method => method, - :params => request_encoder.encode_params(params), + :params => params.nil? ? nil : request_encoder.encode_params(params),

@bogdanvlviv why shouldn't it be fixed in here? Isn't it a special case of the request encoder to handle?

I'd write it as @param_encoder.call(params) if params though.

Should we update some documentation too? E.g. to say that a request encoder will only be asked to encode params when they aren't nil or similar?

Yeah, probably would be better just to write @param_encoder.call(params) if params in ActionDispatch::RequestEncoder, Since it is private api we haven't written any documentation.
By the way if

get "/foos_json", as: :json, params: false request.url # => http://www.example.com/foos_json?false

condition if params would fix it too.

@azbshiri My apologies for confusion. Could you apply @param_encoder.call(params) if params, and update the commit message accordance this change?

I didn't commit and push the changes of Yesterday, it's ready to be merged. @bogdanvlviv

bogdanvlviv · 2018-07-19T12:39:10Z

@azbshiri Could you squash commits into one commit?

al3rez · 2018-07-19T12:55:13Z

done

silva96 · 2018-07-19T18:39:33Z

actionpack/lib/action_dispatch/testing/integration.rb

@@ -233,7 +233,7 @@ def process(method, path, params: nil, headers: nil, env: nil, xhr: false, as: n

        request_env = {
          :method => method,
-          :params => request_encoder.encode_params(params),
+          :params => params.nil? ? nil : request_encoder.encode_params(params),


I would remove this change since this is a specific usage of the encode_params method which already handles the issue.

as i mentioned in tsolar reply that was the case, but i forgot to commit.

tsolar · 2018-07-19T18:40:14Z

actionpack/lib/action_dispatch/testing/integration.rb

@@ -233,7 +233,7 @@ def process(method, path, params: nil, headers: nil, env: nil, xhr: false, as: n

        request_env = {
          :method => method,
-          :params => request_encoder.encode_params(params),
+          :params => params.nil? ? nil : request_encoder.encode_params(params),


if encode_params returns nil if params not present, then this conditional is not necessary

@tsolar yeah it was the case, but i didn't commit yesterday.

When a `get` method called with `as: :json` and `params: nil` or `params: false` (explicitly or implicitly) `RequestEncoder#encode_params` converts it into a `null` or `false` value which includes a unexpected `null=` or `false` query string into request URL. From now on `RequestEncoder#encode_params` checks whether `params` is nil or not otherwise returns. Move down `nil` conversion guard Update CHANGELOG.md

kaspth · 2018-07-20T19:06:28Z

Thanks!

…coder-to-parse-nil-params Prevent `RequestEncoder#encode_params` to parse falsey params

rails-bot assigned kaspth Jul 19, 2018

bogdanvlviv reviewed Jul 19, 2018

View reviewed changes

al3rez force-pushed the actionpack/prevent-request-encoder-to-parse-nil-params branch from 385a14e to e38128c Compare July 19, 2018 12:33

al3rez force-pushed the actionpack/prevent-request-encoder-to-parse-nil-params branch from e38128c to c3ed3e2 Compare July 19, 2018 12:46

al3rez force-pushed the actionpack/prevent-request-encoder-to-parse-nil-params branch 2 times, most recently from 4f420bf to 34c5472 Compare July 19, 2018 15:33

al3rez changed the title ~~Prevent RequestEncoder#encode_params to parse nil params~~ Prevent RequestEncoder#encode_params to parse falsey params Jul 19, 2018

silva96 reviewed Jul 19, 2018

View reviewed changes

tsolar reviewed Jul 19, 2018

View reviewed changes

al3rez force-pushed the actionpack/prevent-request-encoder-to-parse-nil-params branch from 34c5472 to 0e59a32 Compare July 20, 2018 04:55

bogdanvlviv approved these changes Jul 20, 2018

View reviewed changes

al3rez force-pushed the actionpack/prevent-request-encoder-to-parse-nil-params branch from 0e59a32 to 9b51ee9 Compare July 20, 2018 10:49

kaspth merged commit e7fe07e into rails:master Jul 20, 2018

kaspth added a commit that referenced this pull request Jul 20, 2018

Merge pull request #33392 from azbshiri/actionpack/prevent-request-en…

ccfd927

…coder-to-parse-nil-params Prevent `RequestEncoder#encode_params` to parse falsey params

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent `RequestEncoder#encode_params` to parse falsey params #33392

Prevent `RequestEncoder#encode_params` to parse falsey params #33392

al3rez commented Jul 19, 2018 •

edited

rails-bot commented Jul 19, 2018

al3rez commented Jul 19, 2018

bogdanvlviv Jul 19, 2018

al3rez Jul 19, 2018

bogdanvlviv Jul 19, 2018 •

edited

al3rez Jul 19, 2018

kaspth Jul 19, 2018

bogdanvlviv Jul 19, 2018 •

edited

al3rez Jul 19, 2018

al3rez Jul 20, 2018 •

edited

bogdanvlviv commented Jul 19, 2018

al3rez commented Jul 19, 2018

silva96 Jul 19, 2018 •

edited

al3rez Jul 20, 2018

tsolar Jul 19, 2018

al3rez Jul 20, 2018

kaspth commented Jul 20, 2018

		@@ -4,6 +4,7 @@
		require "controller/fake_controllers"
		require "rails/engine"

Prevent RequestEncoder#encode_params to parse falsey params #33392

Prevent RequestEncoder#encode_params to parse falsey params #33392

Conversation

al3rez commented Jul 19, 2018 • edited

rails-bot commented Jul 19, 2018

al3rez commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanvlviv Jul 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanvlviv Jul 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

al3rez Jul 20, 2018 • edited

Choose a reason for hiding this comment

bogdanvlviv commented Jul 19, 2018

al3rez commented Jul 19, 2018

silva96 Jul 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspth commented Jul 20, 2018

Prevent `RequestEncoder#encode_params` to parse falsey params #33392

Prevent `RequestEncoder#encode_params` to parse falsey params #33392

al3rez commented Jul 19, 2018 •

edited

bogdanvlviv Jul 19, 2018 •

edited

bogdanvlviv Jul 19, 2018 •

edited

al3rez Jul 20, 2018 •

edited

silva96 Jul 19, 2018 •

edited