Strong params fix for Rails forms

[georgepalmer]

Fix for part of #1287

As a Rails form will include other params such as a commit, utf_8 and authenticity_token we need to pull out only the params we require or we'll get a strong params exception when running Rails with the setting:

config.action_controller.action_on_unpermitted_parameters = :raise

[doorkeeper-bot]

	1 Warning
⚠️	There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by 🚫 Danger

[nbulaj]

Could you please also add a changelog entry as bot suggested? Also it would be great to have a test that checks if not required params are slliced

[georgepalmer]

Ok all checks now passing

[nbulaj]

Could you please resolve the conflict? Thanks!

[georgepalmer]

@nbulaj sorted

[linhdangduy]

@georgepalmer Sorry to jump into the middle.
Does the change to params.slice().permit() is just a refactor? or because current code still raise exception when passing client_id?

[georgepalmer]

@linhdangduy it's not a refactor. Rails form contain hidden fields such as utf_8 and authenticity token. We need to filter those out or Rails will raise an exception if the following setting is on:

config.action_controller.action_on_unpermitted_parameters = :raise

[linhdangduy]

@georgepalmer I see that current params.permit line already permit client_id parameter and filter only needed parameters, it fixed the issue problem.
Why do we need to change params.permit to params.slice().permit()?

[georgepalmer]

permit doesn't filter the parameters - it just marks them as acceptable for a mass update. So if extra parameters are sent through we get a ActionController::UnpermittedParameters exception. From the Rails docs:

action_on_unpermitted_parameters - Allow to control the behavior when parameters that are not explicitly permitted are found. The values can be false to just filter them out, :log to additionally write a message on the logger, or :raise to raise ActionController::UnpermittedParameters exception.

[linhdangduy]

Thank for the explaining.

Originally, I think that you want to use this configure to be able to do :log or :raise if unpermitted params is sent.

But seem that with this changes, you want to disable the action_on_unpermitted_parameters configuration at authorization_controller. (even you sent parameters other than accepted params, it will occur nothing, because slice filtered needed params already). (Correct me if I'm wrong)

As a Rails form will include other params such as a commit, utf_8 and authenticity_token..

At rails controller, strong params doesn't include 👆these extra form's params.

[georgepalmer]

To be honest I'm a bit confused by what you're getting at but I don't think action_on_unpermitted_parameters can, or should be, disabled at a controller level. That would be something somebody would set in their app and is outside the remit of a gem imho.

Rails forms do normally include extra params such as utf_8 etc - you're just probably not used to seeing these as a more usual approach would be:

params.require(:model).permit(:attributes)

That won't work here though as the authorizations/new template is a form_tag rather than based around a model.

[linhdangduy]

Thank for your clarification.

disable the action_on_unpermitted_parameters configuration at authorization_controller

By disable configure, I mean that:

• params.slice(*field).permit(*field) pull out only params in field, then set permitted=true for params in field.
• So even you send a random params to this controller, the configure will not raise unpermitted error, because with slice, params other than field are filtered out, and will not be considered whether they are permitted or not.

So, if we still want to use this configure at authorization_controller, I think we should do as below:

default_rails_form_params = %i[utf8 authenticity_token commit]
permitted_params = %i[
  client_id response_type redirect_uri scope
  state code_challenge code_challenge_method
]
params.except(*default_rails_form_params).permit(*permitted_params)

With 👆code, if you sent params other than default_rails_form_params and permitted_params, the configure will be triggered.

[georgepalmer]

Ah I see what you were getting at now! To be honest I think this really comes down to a stylistic thing. I've always used slice under the philosophy that if somebody injects extra parameters into the view and submits a form it'll be quietly swallowed rather than cause an error (which then leads to me scratching my head in Sentry working out who would ever do that). I work on high traffic sites where people are trying this kind of thing all the time though. You could quite legitimately argue the opposite that knowing is better and take an approach like you have.

@nbulaj thoughts on best style for this repos?

[nbulaj]

Actually it's a hard question of design, at least because somebody can override the controller and got some issue with the custom behavior and params. But I'm OK with slicing only params we really need. And it must be an overridable method with the list of such params so everybody can reuse and extend it.

[georgepalmer]

@nbulaj to be clear are you saying this is ok or you'd like additional changes to be made?

[nbulaj]

Hi @georgepalmer . Sorry, I was on vacation, but now I'm back :)

Let's move the list of params to a method, say oauth_params. And reuse it with slice / permit. Also don't forget to add a changelog entry and squash your commits. Thanks!

[georgepalmer]

@nbulaj ok I think we're good now

[nbulaj]

LGTM