Skip to content

Conversation

@kbrock
Copy link
Member

@kbrock kbrock commented Oct 29, 2025

pulled out of #1300 - it is unrelated

We always pass a few parameters. these parameters come from query_parameters.
These were triggering a warning in our logging.

Rails adds parameters from routes and actionpack. like :c_id, :s_id, :action, :controller, :format

We only care about a few of these. So only outputting the ones we care about and ignoring the rest

[----] D, [2025-10-28T23:00:25.289277#13035:5200] DEBUG -- : Unpermitted parameters: :expand, :attributes, :c_id. [...]

/cc @jrafanie not sure if you have an opinion on this

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of permitting everything and then slicing... Kind of goes against the right way to use action params. I preferred the original way.

That being said I thought params had a form of slice without the except 🤔

@kbrock
Copy link
Member Author

kbrock commented Oct 29, 2025

@Fryguy Yea, I don't like this either.

Original code

params.permit(:action, :controller, :format).to_h

Reads: Give me all parameters, and complain if there are any parameters besides action, controller, format.

Since there is almost always c_id, s_id. format, offset, expand, and attributes, it will always complain.

I wanted

Please give me action, controller, and format from ActionController and routes.rb.

params.slice(:action, :controller, :format)

This was an error.

Closest I could come

params.permit!.to_h.slice(:action, :controller, :format)

Punt on strong parameters. Just give me these 3 parameters..

log_request("Request", @req.to_hash)
unfiltered_params = request.query_parameters
.merge(params.permit(:action, :controller, :format).to_h)
.merge(params.permit!.to_h.slice(:action, :controller, :format))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think you can do

Suggested change
.merge(params.permit!.to_h.slice(:action, :controller, :format))
.merge(params.slice(:action, :controller, :format).permit!)

@Fryguy
Copy link
Member

Fryguy commented Oct 30, 2025

@kbrock How do you get the warning to appear for this with test code? I tried the following in rails console and I don't see anything:

vmdb(dev)> p = ActionController::Parameters.new(:a => "1", :b => "2", :c => 3)
=> #<ActionController::Parameters {"a"=>"1", "b"=>"2", "c"=>3} permitted: false>
vmdb(dev)> p.permit(:a, :b).to_h
=> {"a"=>"1", "b"=>"2"}

@Fryguy
Copy link
Member

Fryguy commented Oct 30, 2025

Oh i found a way after setting ActionController::Parameters.action_on_unpermitted_parameters = :raise

vmdb(dev)> p = ActionController::Parameters.new(:a => "1", :b => "2", :c => 3)
=> #<ActionController::Parameters {"a"=>"1", "b"=>"2", "c"=>3} permitted: false>

vmdb(dev)> p.permit(:a, :b).to_h
actionpack (7.2.3) lib/action_controller/metal/strong_parameters.rb:1112:in `unpermitted_parameters!': found unpermitted parameter: :c (ActionController::UnpermittedParameters)

vmdb(dev)> p.slice(:a, :b).permit!.to_h
=> {"a"=>"1", "b"=>"2"}

We always pass a few parameters. these parameters come from query_parameters.
These were triggering a warning in our logging.

Rails adds parameters from routes and actionpack. like :c_id, :s_id, :action, :controller, :format

We only care about a few of these. So only outputting the ones we care about and ignoring the rest

```
[----] D, [2025-10-28T23:00:25.289277#13035:5200] DEBUG -- : Unpermitted parameters: :expand, :attributes, :c_id. [...]
```
@kbrock kbrock force-pushed the unpermitted_parameters branch from 73e91a9 to e4eec5c Compare October 30, 2025 16:58
@kbrock
Copy link
Member Author

kbrock commented Oct 30, 2025

Update:

  • moved permit! to the end

This solution looked much closer to my preferred solution. Thanks for the help with a console reproducer.

@Fryguy
Copy link
Member

Fryguy commented Oct 30, 2025

@kbrock Is there a spec that covers this line?

@kbrock
Copy link
Member Author

kbrock commented Oct 31, 2025

@Fryguy So Post requests go through this. (Think Get requests do not - which is odd)
Most tests go through this code since it is called as a before_action :log_api_request, so introducing an error causes a bunch of specs to fail. So at least we have protection there.

I can't find a good way to test this.
Spent a day trying to add a spec and failing. Just can't find a way to green-red-green this.
Lets call this complete unless you have an idea how to test.

@Fryguy
Copy link
Member

Fryguy commented Oct 31, 2025

as long as tests would fail if an error was in there then I think we're covered. I was concerned specifically with the return unless api_log_info? line breaking out of the method early in test and thus this code not getting exercised. But if you says it's covered, then I'm good with it.

@Fryguy
Copy link
Member

Fryguy commented Oct 31, 2025

ugh I read api_log_info? as "Is the API configured to log detailed information", but when I dug down to the method defintion, it's just "is the $api_log at :info level" - why do we even have that method?! 😭

@Fryguy Fryguy merged commit 3dd66de into ManageIQ:master Oct 31, 2025
4 checks passed
@kbrock
Copy link
Member Author

kbrock commented Oct 31, 2025

Yes, return unless api_log_info? is the reason GET requests did not run that method

@kbrock kbrock deleted the unpermitted_parameters branch October 31, 2025 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants