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

UnpermittedParameters not raised #30025

Closed
KjellMorgenstern opened this issue Aug 1, 2017 · 10 comments
Closed

UnpermittedParameters not raised #30025

KjellMorgenstern opened this issue Aug 1, 2017 · 10 comments
Milestone

Comments

@KjellMorgenstern
Copy link

@KjellMorgenstern KjellMorgenstern commented Aug 1, 2017

Steps to reproduce

In config/environments/test.rb (and development.rb) set
config.action_controller.action_on_unpermitted_parameters = :raise

Run a test that tries to pass a non whitelisted parameter to an action.

Rails 5.1.2 behaves fine, the moment we upgrade to 5.1.3RC3, the unpermitted parameter is logged instead of raised.

Expected behavior

ActionController::UnpermittedParameters should be raised.

Actual behavior

Nothing is raised, instead the unpermitted parameter ist logged and the transaction is executed.

System configuration

Rails version:
5.1.3RC3

Ruby version:
2.4.1

@sgrif
Copy link
Contributor

@sgrif sgrif commented Aug 1, 2017

Please provide a sample application that demonstrates your issue.

@sgrif sgrif added the needs work label Aug 1, 2017
@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Aug 1, 2017

Looks like it's related to this change: 6b0606a

@JakariaBlaine
Copy link

@JakariaBlaine JakariaBlaine commented Aug 1, 2017

I also tried to pass unpermitted parameters in the request. No exception was thrown.
The rails version was 5.1.3RC3

This is the sample application with tests - https://github.com/JakariaBlaine/rails-test

@albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Aug 1, 2017

I could reproduce it with Rails 5.1.3RC3

@sgrif @pixeltrix May I try to fix it?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 1, 2017

@albertoalmagro yes

@albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Aug 2, 2017

Created PR #30045

kaspth added a commit that referenced this issue Aug 3, 2017
Caused the regression in #30025.

We didn't have the right fix in time for the final release, so this
get's reverted instead.

This reverts commit 6b0606a.
@kaspth
Copy link
Member

@kaspth kaspth commented Aug 3, 2017

Thanks for helping out with the RCs! Much appreciated ❤️

We're on the third RC for 5.1.3 and didn't want to delay that release further. Instead we've reverted the change causing this for now.

Hopefully #30045 is ready for 5.1.4.rc1 💪

@albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Aug 4, 2017

@kaspth yesterday I updated #30045 please feel free to review it 😊

@rafaelfranca rafaelfranca modified the milestones: 5.1.3, 5.1.4 Aug 4, 2017
@albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Aug 15, 2017

PR is now merged ❤️

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 15, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants