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

Reset rack.input when the environment is scrubbed for the next request #25965

Merged
merged 2 commits into from Jul 28, 2016

Conversation

Projects
None yet
4 participants
@nicksieger
Contributor

nicksieger commented Jul 27, 2016

Summary

Before this change, posted parameters would leak across requests. The included test case failed like so:

  1) Failure:
TestCaseTest#test_multiple_mixed_method_process_should_scrub_rack_input:
--- expected
+++ actual
@@ -1 +1 @@
-{"bar"=>"an bar", "controller"=>"test_case_test/test", "action"=>"test_params"}
+{"foo"=>"an foo", "bar"=>"an bar", "controller"=>"test_case_test/test", "action"=>"test_params"}

An argument could be made that this situation isn't encountered often and that one should limit the number of requests per test case, but I still think the parameter leaking is an unexpected side-effect.

Reset rack.input when the environment is scrubbed for the next request
Before this change, posted parameters would leak across requests. The included
test case failed like so:

      1) Failure:
    TestCaseTest#test_multiple_mixed_method_process_should_scrub_rack_input:
    --- expected
    +++ actual
    @@ -1 +1 @@
    -{"bar"=>"an bar", "controller"=>"test_case_test/test", "action"=>"test_params"}
    +{"foo"=>"an foo", "bar"=>"an bar", "controller"=>"test_case_test/test", "action"=>"test_params"}

An argument could be made that this situation isn't encountered often and that
one should limit the number of requests per test case, but I still think the
parameter leaking is an unexpected side-effect.
@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Jul 27, 2016

Member

Should we be adding this behavior to ActionDispatch::IntegrationTest as well, if it doesn't already do this? 😬

Member

maclover7 commented Jul 27, 2016

Should we be adding this behavior to ActionDispatch::IntegrationTest as well, if it doesn't already do this? 😬

@nicksieger

This comment has been minimized.

Show comment
Hide comment
@nicksieger

nicksieger Jul 27, 2016

Contributor

I'd be happy to add that if it makes sense. I'll take a look shortly.

Contributor

nicksieger commented Jul 27, 2016

I'd be happy to add that if it makes sense. I'll take a look shortly.

@nicksieger

This comment has been minimized.

Show comment
Hide comment
@nicksieger

nicksieger Jul 28, 2016

Contributor

Looks like AD::IntegrationTest already works as expected. Adding a passing test case.

Contributor

nicksieger commented Jul 28, 2016

Looks like AD::IntegrationTest already works as expected. Adding a passing test case.

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Jul 28, 2016

Member

Thank you very much 👏

Member

guilleiguaran commented Jul 28, 2016

Thank you very much 👏

@guilleiguaran guilleiguaran merged commit 3916656 into rails:master Jul 28, 2016

2 checks passed

codeclimate Code Climate has skipped analysis of this commit.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicksieger

This comment has been minimized.

Show comment
Hide comment
@nicksieger

nicksieger Jul 29, 2016

Contributor

@rafaelfranca how many branches back should this go?

Contributor

nicksieger commented Jul 29, 2016

@rafaelfranca how many branches back should this go?

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Jul 29, 2016

Member

@nicksieger this should be backported to 5-0-stable.

Member

maclover7 commented Jul 29, 2016

@nicksieger this should be backported to 5-0-stable.

@nicksieger nicksieger referenced this pull request Jul 29, 2016

Merged

Backport for 25965 #25988

@nicksieger nicksieger deleted the nicksieger:ac_test_case_reset_rack_input branch Jul 29, 2016

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