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
Fixes #22285 - Raise error when strong params filters out params #5183
Conversation
Issues: #22285 |
This currently Breaks All Things. setting to WoC to figure out. |
Previously had 636 failing tests, still WIP to fix all broken tests. |
@@ -314,7 +314,7 @@ def expect_attribute_modifier(modifier_class, args) | |||
|
|||
test "should update hostgroup_id of host" do | |||
@host = FactoryBot.create(:host, basic_attrs_with_hg) | |||
put :update, params: { :id => @host.to_param, :hostgroup_id => Hostgroup.last.id } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last hostgroup doesn't necessarily have an environment, which is required, and reset if the hg changes.
Could we wait with this change until 1.17 is out + a week or two: many plugin authors haven't had green tests for a while, so would be good to give them some time to actually do other things than catching up with upstream breakages? |
Down to 237 :) |
@iNecas I expect fixing all tests and reviewing it will take several days at best. I don't mind waiting a bit but it will be painful to keep up to date over a long period. Besides the impact on users and developers I outlined on discourse, this also exposes multiple tests which we thought were passing when in fact they were failing silently - I am still digging into it, but there may be other bugs discovered due to these tests. In other words - the sooner we fix this properly the better. |
0831607
to
e4c080b
Compare
refute_includes self.class.auth_source_ldap_params_filter.filter_params(params, context), 'type' | ||
assert_raises ActionController::UnpermittedParameters do | ||
self.class.auth_source_ldap_params_filter.filter_params(params, context) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end at 12, 3 is not aligned with assert_raises ActionController::UnpermittedParameters do at 10, 4.
@@ -7,6 +7,8 @@ class AuthSourceLdapParametersTest < ActiveSupport::TestCase | |||
|
|||
test "filters STI :type field" do | |||
params = ActionController::Parameters.new(:auth_source_ldap => {:type => AuthSourceHidden.name}) | |||
refute_includes self.class.auth_source_ldap_params_filter.filter_params(params, context), 'type' | |||
assert_raises ActionController::UnpermittedParameters do | |||
self.class.auth_source_ldap_params_filter.filter_params(params, context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use 2 (not 3) spaces for indentation.
b713bec
to
1960afd
Compare
@waldenraines FYI - This will require significant effort to fix and test the katello UI since when an item is edited its json is often sent back to server with extra fields. Product edit, for example, gets "An error occurred saving the Product: found unpermitted parameters: :cp_id, :created_at, :updated_at". This will be common on all of the pages, I think. |
@tbrisker please rebase :) |
Currently we silently filter out parameters (or log in production), causing unexpected results when passing an invalid param (either incorrect name or incorrect type). This can lead to unexpected results, since the user, seeing no error, assumes the request was successful when in fact some of the parameters were filtered out.
- fix users controller tests A user cannot change their own auth_source. Passing illegal parameters should raise ActionController::UnpermittedParameters - Fix auth_source_ldap controller tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
rebased, now lets see how many new test have broken in the last 2 months... |
@tbrisker this needs a rebase :) I think this should go in soon, especially now that 1.18 is branched. |
@tbrisker ping? |
Thank you for your contribution, @tbrisker! This PR has been inactive for 6 months, closing for now. |
😢 |
Currently we silently filter out parameters (or log in production),
causing unexpected results when passing an invalid param (either
incorrect name or incorrect type). This can lead to unexpected results,
since the user, seeing no error, assumes the request was successful when
in fact some of the parameters were filtered out.