Refs #22285 - Prepare for strong params enforcement #5330
Conversation
|
Issues: #22285 |
tbrisker
force-pushed
the
22285-pre
branch
2 times, most recently
from
9feae6a
to
3368162
Compare
|
Test failures seem to be just random timeouts not related. |
|
Only reviewed visually, and mainly the test changes, but this looks sensible. 👍 |
| @@ -60,12 +60,13 @@ def parameters | |||
|
|
|||
| def get_host_or_hostgroup | |||
| # params['host_id'] = 'undefined' if NEW since hosts/form and hostgroups/form has no data-id | |||
| if params['host_id'] == 'undefined' | |||
| host_id = params.delete(:host_id) | |||
There was a problem hiding this comment.
Is the .delete required and are other parts not relying on host_id?
There was a problem hiding this comment.
yes, the host_id is passed just for the lookup part and isn't permitted by strong params since its a top level param. there is also params[host][id] or params[hostgroup][id] that should be identical. TBH this code deserves a whole lot of love but that's outside the scope of this pr.
config/application.rb
Outdated
| controller action format locale utf8 _method authenticity_token commit redirect | ||
| page per_page paginate search order sort sort_by sort_order | ||
| _ie_support fakepassword apiv id organization_id location_id user_id | ||
| ) |
There was a problem hiding this comment.
From the security perspective, the following can be theoretically dangerous: id organization_id location_id user_id but I think it is worth taking the risk - will unlikely happen and your patch is a great security improvement.
There was a problem hiding this comment.
This doesn't actually permit these params, despite the naming. it just prevents raising an exception when these params are found and permit! is called on them.
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `S...
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead. (obsolete configuration found in .rubocop.yml, please update it) The `Performance/HashEachMethods` cop has been removed since it no longer provides performance benefits in modern rubies.
|
I had to rebase which caused the reviews to be marked as stale. this already has 2 👍's from @lzap and @mmoll, and i think also @bastilian reviewed but didn't mark as approved. |
|
@tbrisker I'd merge this, please squash the commits together 👍 |
This workaround was needed in Rails 5.0, but 5.1 already supports filtering on arbitrary hash params.
Despite the name, this list only prevents errors from being raised from these parameters which are still filtered.
- Allow $resource_id param on parameters controller - Permit user_id on access token controller - Allow params in templete combination controller Allows `config_template_id` and `:provisioning_template_id` - Fix compute attribute param filter Ignore top-level compute_resource_id and compute_profile_id that are passed from the url and aren't used for assignment. - Allow arbitrary compute_atrributes for nic
- Fix broken hosts api tests - Fix role cloning tests - Fix ssh key controller api test - Fix http proxies controller test - Fix lookup key override api controller test - Fix puppetclass controller api test - fix operatingsystem controller api test - Fix hostgroups controller test - Fix integration tests with template fields We remove the template fields using JS, so we have to test these forms with JS or they fail because the template fields are sent - Fix host instegration tests - fix hosts controller tests - Fix images controller parameter handling - Fix interface controller test - Fix puppetclass controller tests - Fix variable LK controller test
- Fix current_url_params helper for strong params Explicitly permit only the permitted parameters, don't try to permit all parameters (and raise exception for other parameters) - Don't send editor params to server This causes issues with strong params, and we don't need these fields anyways. - Fix host form for strong params - Disable cr fields on hosts form These fields are only used to determine in JS what capabilities the current compute resource allows, they don't need to be sent to the server. - Correctly calculate object type in host form Otherwise, discovered hosts add a field named `discovered_host[puppetclass_ids][]` that fails on smart params which expects the whole form to be submitted under the `host` hash. - Remove extranous role_id input on filter form
|
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
There was a problem hiding this comment.
Some files could not be reviewed due to errors:
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `S...
Error: The `Style/TrailingCommaInLiteral` cop no longer exists. Please use `Style/TrailingCommaInArrayLiteral` and/or `Style/TrailingCommaInHashLiteral` instead. (obsolete configuration found in .rubocop.yml, please update it) The `Performance/HashEachMethods` cop has been removed since it no longer provides performance benefits in modern rubies.
|
@mmoll Thanks, tried to combine them together into 6 logical blocks, i hope this makes sense :) |
|
test failures unrelated, merging... 💣 |
|
merged, thanks @tbrisker! |
These commits are all fixes needed to allow us to start raising errors on strong params errors, while (hopefully) still being compatible with the current code base so that the actual fix will be easier to merge. It is split into multiple PRs to enable easier reviews, once agreed on they could most likely be squashed into one (or a few) commits.
See #5183 and https://community.theforeman.org/t/strong-parameters-is-borked/8070/4 for more information.