fix: false positive on unpermitted parameters

[Paul-Bob]

Description

Fixes #3095

TLDR

This PR ensures that unnecessary and "false positive" warnings about unpermitted parameters are not raised by using only the permit strong parameters method where we're permitting. We were using the same permit method in places where permitting params weren't necessary and we could use slice or extract!

Context

When configuring self.extra_params on a resource and submitting a form, the permit method is utilized in two distinct stages:

1. In the Base Controller: All parameters, including both base and extra params, are processed through Strong Parameters, ensuring they are permitted.

2. On the Resource: When updating the record, permit is called again but only for the extra params. This step isolates these extra params from the combined base and extra params hash.

Actual approach

Instead of using permit on params to select the extra parameters, we now use slice, which returns only the specified extra parameters. In order to use slice we need to pass only the keys from extra_params. That's safe since the params at this point are already permitted (by using also the hashes values from extra_params) and there is no chance to pick unpermitted ones.

Previous approach

We temporarily disable the action on unpermitted parameters during this second call. This is necessary because permit is applied exclusively to the extra params, which means any base params are excluded and would otherwise trigger errors or logs for being unpermitted.

This approach does not impact the rest of the application because the action on unpermitted parameters is only temporarily disabled within this specific context. After handling the extra parameters, we restore the original setting for handling unpermitted parameters.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works

[codeclimate]

Code Climate has analyzed commit 949121d and detected 0 issues on this pull request.

View more on Code Climate.

[binarygit]

Thanks for working on this Paul. I spent a little time trying to understand this and this is tricky. Kudos to you for coming up with the solution. I also have a small idea I want to share:

Could we do something like this?

 #base.rb
 def fill_record(record, params, extra_params_keys: [], fields: nil)
         ...
         if extra_params.present?
          # get_key_value returns a hash of objects like permit does...but without permitting
          record.assign_attributes params.get_key_value(extra_params_keys)
        end

We'll need to code up the get_key_value method (I checked the API and couldn't find anything similar) which essentially replicates permit but without the permitting part. It just takes our array of keys (eg: [:fish_type, :something_else, {:properties=>[], :information=>[:name, :history], :reviews_attributes=>[:body, :user_id]}]) and then returns us the key_value pair like permit does.

Do you think something like this would work?

[Paul-Bob]

We'll need to code up the get_key_value method

I thought about it but changed my mind after checking the permit source code.

PS: I can think about 3 possible solutions to this:

1 - Maintaining a method that behaves like permit without raising any log / error ( not ideal )
2 - The actual changes ( not ideal but IMO better than 1 )
3 - Make a PR to rails that allows to specify action_on_unpermitted_parameters, something like:

record.assign_attributes params.permit(extra_params, action_on_unpermitted_parameters: false)

@adrianthedev and @binarygit WDYT about making a PR to rails with these changes? (need to be tested and improved, it's just a DRAFT commit)

[adrianthedev]

To-do:

• let's fill out this PR with what we spoke on the call
• let's PR to Rails to suport permit_without_warnings

[binarygit]

.

[Paul-Bob]

Just added a couple of comments with a brief change explanation

[Paul-Bob]

Memoizing and permitting id.

The id is present when an action is clicked from a show page.

[Paul-Bob]

require and permit is not really necessary here, we just want to access the related_id

[Paul-Bob]

Applying permit only on the attach_fields would raise the error of the unpermitted parameters for the remaining parameters.

permit is unnecessary here because fill_record enures that only params that match attach_fields ids will be sent to the record.

[Paul-Bob]

Using a tags field will always raise the unpermitted error since it is generating a dummy param that isn't permitted anywhere.

[binarygit]

LGTM 😄 . Thanks Paul!

[adrianthedev]

Let's sanitize the src_url below where we display it. There's a security advisory there.

[adrianthedev]

How about we params to permitted_params so it's clear to anyone who's reading this and wondering why we are slicing instead of permitting.

[adrianthedev]

I left a couple of pieces of feedback.

GG! Looking forward to not showing unpermitted params useless to users.

Thanks!

[github-actions]

This PR has been merged into main. The functionality will be available in the next release.

Please check the release guide for more information.