THREESCALE-12434: Migrate from protected attributes to strong parameters - Part 1

[mayorova]

What this PR does / why we need it:

This is part 1 of the migration from protected attributes to strong parameters.
Protected attributes is an old Rails feature which was deprecated a long time ago. We were using protected_attributes_continued gem to keep it working, but now it's also discontinued and does not support Rails 7+, so it's a blocker for upgrading to Rails 7.2 for us.

This Part 1 handles all simple models that don't have custom attributes (so, excluding Account, User, Cinstance).

Which issue(s) this PR fixes

https://redhat.atlassian.net/browse/THREESCALE-12434

Verification steps

All tests should pass, and all features should work as before.

Special notes for your reviewer:

[jlledom]

If tests pass, approved 👍

[jlledom]

Still a lot of references to without_protection, maybe this should be the PR to remove them

[qltysh]

❌ 11 blocking issues (11 total)

Tool	Category	Rule	Count
rubocop	Lint	Assignment Branch Condition size for hosted\_success is too high. [<4, 20, 6> 21.26/20]	3	❌
reek	Lint	DeveloperPortal::Admin::Messages::OutboxControllerIntegrationTest#test_create_with_valid_message has approx 12 statements	2	❌
reek	Lint	DeveloperPortal::Admin::Messages::OutboxControllerIntegrationTest#test_create_with_invalid_message calls 'flash[:error]' 2 times	2	❌
rubocop	Style	Incorrect formatting, autoformat by running qlty fmt.	1	❌
rubocop	Style	Trailing whitespace detected.	1	❌
rubocop	Lint	Action create should appear before destroy.	1	❌
rubocop	Style	1 trailing blank lines detected.	1	❌

@qltysh one-click actions:

• Auto-fix formatting (qlty fmt && git push)

[jlledom]

I left a few comments. The best catches are from Claude.

[jlledom]

not setting this issuer_id anywhere? Is this not breaking any test?

[mayorova]

Oh, yes, I forgot to leave a comment about this one.

Actually leaving this line as it is does break these tests: https://app.circleci.com/pipelines/github/3scale/porta/33827/workflows/e9c35096-8c14-455a-a3c3-af6dbd456f28/jobs/396661/tests#failed-test-1

The reason is - this .update actually never did anything, because issuer_id was a protected attribute, see here.

After removing protected attributes, this .update does change the issuer_id, which makes the application plan kind of "disconnected" from its issuer, I guess, for this reason it is not detected by the deletion logic, and is left as an orphan in the test (troubleshooting showed that this was the plan that appears in the failed test expectation).

I am not sure now whether this object still makes sense though, or whether its name is correct - I mean, I'm not sure the plan is "broken" anymore. The issuer_id is correct now, but not sure if original_id is the one that's expected. Maybe @akostadinov will remember what was the intention here?...

[akostadinov]

Actually perhaps I wanted to test that the service will be deleted anyway because it is anyways in the account background deletion list. And also test that a broken reference will not kill the background deletion.

Apparently I was fooled by the protected attributes gem 😿

Also apparently I didn't realize that I'm just disconnecting the ApplicationPlan. So this change is fine.

[akostadinov]

I am very lost on this. What is the concept of verifying correctness?

Before this PR we had protected attributes AND strong parameters. But some controllers used blanket permitting the parameters and relying on the protected attributes gem to actually do the work.

Now I see a lot of changes but I have no idea how did you search for controllers which update those kind of models to tidy the way they permit the parameters.

Could you share your approach so I can think about potential edge cases?

[mayorova]

This fix probably shouldn't be here, but this was just so wrong...

[mayorova]

I am very lost on this. What is the concept of verifying correctness?

Well, the concept is that no test should fail after the changes:

• remove attr_protected or attr_accessible from the model
• permit an explicit list of attributes on params in the corresponding controller

This is mostly respected, rather than changing the tests, I just added additional tests that I found missing.

Before this PR we had protected attributes AND strong parameters. But some controllers used blanket permitting the parameters and relying on the protected attributes gem to actually do the work.

Now I see a lot of changes but I have no idea how did you search for controllers which update those kind of models to tidy the way they permit the parameters.

Could you share your approach so I can think about potential edge cases?

Well, the approach is quite simple/stupid:

1. mainly I checked in the corresponding controller which attributes were expected: for the UI controllers it's easy to see on submitting the form, and for the API controllers, I was following the OpenAPI spec.
2. I also did a cross-check with the attr_accessible and attr_protected lists - were the expected attributes identified in the previous step 1) in attr_accessible (they should be), or in attr_protected (should not be there).

When writing new tests, I sometimes would do this:

• in the controller, removed .permit and verified that without it ActiveModel::ForbiddenAttributesError was being raised
• then added .permit again, and verified that the test passes successfully.

What I did not do in the tests was including some attribute in params that is not in the permitted list, and verifying that this attribute is not being updated on the model. But honestly, now thinking about it - there are hardly such attributes, especially in the models included in this Part 1, most "non-objects" attributes are expected to be updated. And it's not even that easy to figure out which are the "protected" attributes.

Anyway, I think we can rely on the .permit method and be confident that it does remove anything that is not in the list.

[akostadinov]

I iterated over the commits with AI help. This is iteration over each commit and comparing between two models. But I couldn't validate the claims by myself yet. Decided to paste as but will be looking into this more tomorrow.

---

Strong Parameters Migration Review Report

Date: 2026-04-16
Scope: All commits since git merge-base HEAD upstream/master (93a6f03..1fd4a0f)
Purpose: Verify that all model updates previously relying on protected_attributes gem are now properly covered by strong parameters whitelists. Flag any blacklist/except patterns and suspicious usages.

---

Summary

Verdict	Count	Commits
PASS	11	51b916b, 07fccfe, fe53472, 9fa3385, 3052471, 237f612, ef62096, 878199b, c849220, 5b0412a, 1fd4a0f
NEEDS_REVIEW	5	813c08d, 3099a22, 57d8d40, b03a0f6, 4407db4
FAIL	2	3af98a8, d3304a9
NEEDS_REVIEW (batch)	1	8b64609

---

CRITICAL FINDINGS (Action Required)

1. FAIL: Settings model - premature attr_protected removal (3af98a8)

Severity: CRITICAL

The attr_protected :account_id, :tenant_id, :product, :audit_ids, :sso_key was removed from the Settings model, but 4 controllers still pass raw params[:settings] without permit():

Controller	File	Line
Sites::SpamProtectionsController	app/controllers/sites/spam_protections_controller.rb	10
Sites::ForumsController	app/controllers/sites/forums_controller.rb	11
Provider::Admin::BotProtectionsController	app/controllers/provider/admin/bot_protections_controller.rb	12
Sites::DeveloperPortalsController	app/controllers/sites/developer_portals_controller.rb	9, 19-21

Additionally, Sites::DocumentationsController uses params[:settings].slice(...) instead of permit(), which will raise ForbiddenAttributesError in standard Rails.

Risk: An attacker could mass-assign account_id, tenant_id, product, sso_key, or any other Settings attribute through these unprotected endpoints.

Recommendation: Add settings_params methods with proper permit() whitelists to all 5 controllers before removing attr_protected.

---

2. FAIL: Forum models - missing strong parameters (d3304a9)

Severity: CRITICAL

This commit removes attr_protected/attr_accessible from 16 models. Two controllers have no permit() calls at all:

a) ForumSupport::Topics (app/lib/forum_support/topics.rb:98-99)

• topic_params returns params.require(:topic) with NO .permit() call
• Used in build(topic_params) (line 36) and @topic.attributes = topic_params (line 52)
• Old attr_accessible whitelisted: :markup_type, :title, :body, :quiz, :quiz_id, :first_name, :last_name, :email, :tag_list, :anonymous_user
• All Topic attributes are now mass-assignable

b) ForumSupport::Categories (app/lib/forum_support/categories.rb:26,39)

• Uses params[:topic_category] directly in build() and update()
• Old attr_protected blacklisted: :forum_id, :tenant_id
• :forum_id and :tenant_id are now mass-assignable

c) Fields::Provider#for (app/lib/fields/provider.rb:14)

• Calls klass.protected_attributes which changes behavior when attr_protected is removed
• May expose internal/sensitive attributes via the provider fields API

Note: The commit message says "Remove attr_protected from multiple models" but Topic actually had attr_accessible (a whitelist) removed, which is a more dangerous operation.

---

3. NEEDS_REVIEW: Post model - missing strong parameters (8b64609)

Severity: CRITICAL

attr_accessible :body, :markup_type, :anonymous_user was removed from Post, but ForumSupport::Posts (app/lib/forum_support/posts.rb) still uses raw params[:post]:

• Line 19: @post = @topic.posts.build(params[:post])
• Line 34: @post.attributes = params[:post]

Risk: All Post attributes (including user_id, topic_id, created_at, etc.) are now mass-assignable.

Recommendation: Add post_params method with params.require(:post).permit(:body, :markup_type, :anonymous_user).

---

4. NEEDS_REVIEW: Message model - params.permit! in outbox (b03a0f6)

Severity: HIGH

DeveloperPortal::Admin::Messages::OutboxController (lib/developer_portal/app/controllers/developer_portal/admin/messages/outbox_controller.rb, line 63) uses params.permit! which whitelists ALL parameters. This is then used to build Message objects, allowing mass-assignment of sender_id, state, system_operation_id, tenant_id, type, or any other column.

Recommendation: Replace params.permit! with a proper message_params method permitting only :subject, :body.

---

5. NEEDS_REVIEW: Service model - unprotected controllers (4407db4)

Severity: HIGH

attr_protected :account_id, :tenant_id, :audit_ids was removed from Service, but three controllers still pass raw params without permit():

Controller	File	Line	Pattern
Api::TermsController	app/controllers/api/terms_controller.rb	14	@edited_service.update(params[:edited_service])
Api::SupportsController	app/controllers/api/supports_controller.rb	11	@edited_service.update(params[:service])
Api::ContentsController	app/controllers/api/contents_controller.rb	11	@service.update(params[:service])

Risk: account_id, tenant_id, or other sensitive attributes could be mass-assigned through these endpoints.

---

MODERATE FINDINGS (Should Fix)

6. ApiDocs::Service - without_protection: true in production code (3099a22)

app/services/service_discovery/import_cluster_definitions_service.rb:94 still uses:

service.api_docs_services.build({ ... }, without_protection: true)

This will break at runtime once the protected_attributes gem is removed. The , without_protection: true should be removed.

3 test files also still use without_protection: true for this model.

---

7. CMS Groups - section_ids authorization bypass (813c08d, fixed in de59eef)

The initial migration (813c08d) removed the per-section authorization check when assigning section_ids, allowing a user to assign sections from other providers. This was fixed in de59eef with a validate_section_ids before_action. These commits must always be deployed together.

Remaining issue in the fix (de59eef): validate_section_ids will raise NoMethodError if section_ids is absent from params (e.g., updating only name). A return if section_ids.blank? guard is needed.

---

8. Field Definitions - hint attribute possibly missing (57d8d40)

The hint column exists in the DB and was not previously protected, but it is not included in the controller's permit() list. Verify this is intentional (i.e., hint is not exposed in the UI form).

Also, sort_params does not call permit() -- it fetches raw params but only uses them as array of IDs for find(), so it's not a mass-assignment vector. Still, could be hardened.

---

REPO-WIDE BLACKLIST/EXCEPT AND permit! AUDIT

The following is a full sweep of the entire repository for params.except(...), params.reject(...), and params.permit! patterns in production code. Test files are excluded unless they reveal a pattern concern.

Blacklist/Except Patterns (production code using .except() on params)

HIGH RISK

#	File	Line	Pattern	Model	Risk	Notes
B1	app/controllers/buyers/accounts_controller.rb	36	account_params.except(:vat_rate)	Account	HIGH	account_params has NO permit() call (has a # TODO: using permit later comment). All non-attr_protected Account attrs are mass-assignable.
B2	app/controllers/provider/signups_controller.rb	58-63	params.require(:account).except(:user) / user_params	Account, User	HIGH	No permit() on account or user params in signup flow. Relies entirely on attr_protected/attr_accessible.
B3	lib/developer_portal/app/controllers/developer_portal/base_controller.rb	33	params.except(*read_only_fields)	Various	HIGH	Blacklist of read-only fields only. Multiple developer portal controllers (signup, accounts, users) chain this without adding permit(). Any non-read-only field passes through.
B4	app/lib/api_support/params.rb	13	params.except(:format, :controller, :action, :provider_key, :api_key, :access_token)	Various	HIGH	Used in Master::Api::ProvidersController#create_params for signup. Extremely broad -- everything except auth/routing keys reaches account/user creation.

LOW RISK (post-permit except or non-mass-assignment usage)

#	File	Line	Pattern	Risk	Notes
B5	app/controllers/admin/api/service_features_controller.rb	49	feature_params.except(:scope)	LOW	Post-permit except. Safe.
B6	app/controllers/provider/admin/user/personal_details_controller.rb	8	user_params.except(:current_password)	MODERATE	No permit() on user_params, but User has attr_accessible whitelist as secondary defense. Should still add permit().
B7	app/controllers/api/integrations_controller.rb	89	proxy_params.except(:lock_version)	SAFE	Post-permit except. proxy_params has a proper permit() whitelist.
B8	app/lib/finance/billing.rb	16	params.except(:type)	SAFE	Internal params (not user-facing ActionController::Parameters). Used for LineItem error handling.
B9	app/lib/authentication/strategy/oauth2_base.rb	52	params.except(:code, :system_name)	SAFE	Used for URL generation, not mass-assignment.
B10	app/lib/authentication/strategy/base.rb	78	permitted_params.except(:action, :controller)	SAFE	Used for URL generation.

params.permit! Patterns (permit-all)

#	File	Line	Model Affected	Risk	Notes
P1	lib/developer_portal/app/controllers/developer_portal/admin/messages/outbox_controller.rb	63	Message	HIGH	Already flagged as Finding #4 above. All message attrs mass-assignable.
P2	app/controllers/provider/signups_controller.rb	110	None directly	LOW	permit! result only used to read specific keys (plan_id, origin). But account_params/user_params in same controller lack permit() (see B2).
P3	app/controllers/master/redhat/auth_controller.rb	24	None	LOW	Used for url_for() redirect URL generation, not mass-assignment.
P4	app/controllers/admin/api/registry/policies_controller.rb	57	Policy	LOW	permit! applied after merging only :name, :version, :schema. Effectively a 3-field whitelist.
P5	app/controllers/provider/admin/account/payment_gateways/braintree_blue_controller.rb	36	External API	LOW	Passed to Braintree API, not ActiveRecord.
P6	lib/developer_portal/app/controllers/developer_portal/admin/account/braintree_blue_controller.rb	22	External API	LOW	Passed to Braintree API, not ActiveRecord.
P7	app/lib/three_scale/api/responder.rb	34	None	LOW	Used for url_for() only.
P8	app/lib/authentication/strategy/base.rb	77	None	SAFE	Used for signup_path() URL generation.
P9	config/initializers/protected_attributes_hacks.rb	11	Various	NOTE	Compatibility layer for protected_attributes_continued gem. Will need removal when gem is removed.

without_protection: true in Production Code

These will break at runtime when the protected_attributes gem is removed:

#	File	Line	Model	Notes
W1	app/controllers/sites/dns_controller.rb	11	Account	Has proper permit() -- without_protection only needed to bypass attr_protected for from_email/domain. Safe but needs cleanup.
W2	app/controllers/master/api/providers_controller.rb	51	Account	Has proper permit() on update_params. But line 52 calls assign_unflattened_attributes without permit().
W3	app/controllers/admin/api/providers_controller.rb	16	Account	Has proper permit(). Safe but needs without_protection removed.
W4	app/services/service_discovery/import_cluster_definitions_service.rb	94	ApiDocs::Service	Internal service, no user params. Remove without_protection: true.
W5	app/services/finance/stripe_payment_intent_update_service.rb	44	PaymentTransaction	Internal service. Remove without_protection: true.
W6	app/workers/backend_delete_service_worker.rb	14	Service	Constructing stub object. Remove without_protection: true.
W7	app/workers/audited_worker.rb	6	Audit	Gem integration. Review if still needed.
W8	app/queries/usage_limit_violations_query.rb	33	Account	Constructing stub object. Remove without_protection: true.
W9	app/lib/logic/cms.rb	63	CMS::Section	Internal creation. Remove without_protection: true.
W10	app/lib/finance/billing.rb	11, 21, 40	LineItem	Internal billing. Remove without_protection: true.
W11	app/lib/signup/impersonation_admin_builder.rb	16	User	Internal. Remove without_protection: true.
W12	app/lib/backend/model_extensions/service.rb	48	Account	Internal. Remove without_protection: true.
W13	app/events/zync_event.rb	73	Service	Constructing stub object. Remove without_protection: true.
W14	app/events/applications/application_deleted_event.rb	8	Service	Constructing stub object. Remove without_protection: true.

---

LOW-RISK OBSERVATIONS

Commit	Observation
fe53472	test/test_helpers/provider.rb:98-100 still uses without_protection: true for CMS::Redirect
9fa3385	test/integration/services/finance/billing_service_integration_test.rb:51 uses without_protection: true for Invoice
237f612	PricingRule model still has audited allow_mass_assignment: true (harmless but unnecessary)
c849220	Buyers::CustomPlansController and Buyers::CustomApplicationPlansController pass raw params to customize_plan! -- safe because downstream only reads individual keys, but not idiomatic
8b64609	Finance::Billing (app/lib/finance/billing.rb:11,21,40) uses without_protection: true -- will break when gem is removed
d3304a9	Multiple production files still use without_protection: true -- full gem removal is not yet possible

---

PASSED COMMITS (No Issues)

Commit	Description	Notes
51b916b	Usage limits	permit(:period, :value) -- clean
07fccfe	Access tokens	permit(:name, :permission, :expires_at, scopes: []) -- clean, :owner correctly excluded
fe53472	CMS redirects	permit(:source, :target) -- matches old attr_accessible exactly
9fa3385	Invoice	Multiple controllers with appropriate permit lists
3052471	SSOToken	permit(:user_id, :username, :expires_in, :redirect_url, :protocol) + ForbiddenAttributesProtection
237f612	Pricing rules	permit(:min, :max, :cost_per_unit) -- sensitive attrs excluded
ef62096	Plan features	permit(:name, :system_name, :description, :scope) -- protected attrs excluded
878199b	Webhook	permit(url, active, provider_actions, *switchable_attributes)
c849220	Plan	All controllers have proper permits; protected attrs excluded
5b0412a	Sites settings fix	Controller already had permit(), commit fixes error handling
1fd4a0f	Peer review fixes	Memoization fix + SSOToken test -- clean

[mayorova]

I iterated over the commits with AI help. This is iteration over each commit and comparing between two models. But I couldn't validate the claims by myself yet. Decided to paste as but will be looking into this more tomorrow.

It has some good finds, thank you! ❤️

I'll review it carefully.

[mayorova]

1. FAIL: Settings model - premature attr_protected removal (3af98a8)

Addressed in f9bd9d6

• Sites::SpamProtectionsController, Provider::Admin::BotProtectionsController - legit, and missed because of lack of tests.

• Sites::ForumsController is not needed, it's deprecated, but it was still working in Master portal (probably by error), so here I just fixed it. As it's known to be deprecated and pending removal, I didn't add the test.

• Sites::DeveloperPortalsController is a "hidden" page (not referenced from anywhere in the menu), so probably it's deprecated, but as it was still accessible I fixed it too.

2. FAIL: Forum models - missing strong parameters (d3304a9)

a) ForumSupport::Topics (app/lib/forum_support/topics.rb:98-99)

b) ForumSupport::Categories (app/lib/forum_support/categories.rb:26,39)

All forum-related stuff is deprecated since a long time ago, and pending to be removed, so I opened #4281 that removes forum-related (and some other) controllers.

c) Fields::Provider#for (app/lib/fields/provider.rb:14)

• Calls klass.protected_attributes which changes behavior when attr_protected is removed

app/lib/fields/provider.rb is updated to remove reference to protected_attributes in #4255

3. NEEDS_REVIEW: Post model - missing strong parameters (8b64609)

Severity: CRITICAL

More forum-related stuff, removed in #4281

4. NEEDS_REVIEW: Message model - params.permit! in outbox (b03a0f6)

Severity: HIGH

DeveloperPortal::Admin::Messages::OutboxController (lib/developer_portal/app/controllers/developer_portal/admin/messages/outbox_controller.rb, line 63) uses params.permit! which whitelists ALL parameters. This is then used to build Message objects, allowing mass-assignment of sender_id, state, system_operation_id, tenant_id, type, or any other column.

This is a legit find, fixed in 9e03105

5. NEEDS_REVIEW: Service model - unprotected controllers (4407db4)

Api::TermsController, Api::SupportsController, Api::ContentsController

These are dead code (no views, no tests), removed in #4281

[mayorova]

MODERATE FINDINGS (Should Fix)

6. ApiDocs::Service - without_protection: true in production code (3099a22)

app/services/service_discovery/import_cluster_definitions_service.rb:94 still uses:

service.api_docs_services.build({ ... }, without_protection: true)

This and other without_protection: true occurrences are addressed in

#4255

7. CMS Groups - section_ids authorization bypass ([813c08d]

Remaining issue in the fix (de59eef): validate_section_ids will raise NoMethodError if section_ids is absent from params (e.g., updating only name). A return if section_ids.blank? guard is needed.

This error should not happen, because in the UI controller section_ids[] is always passed, even if it's empty. But adding a check is still good. Fixed.

8. Field Definitions - hint attribute possibly missing (57d8d40)

The hint column exists in the DB and was not previously protected, but it is not included in the controller's permit() list. Verify this is intentional (i.e., hint is not exposed in the UI form).

True, but hint is not on the UI form for Fields Definitions, and seems to not be used anywhere.

Also, sort_params does not call permit() -- it fetches raw params but only uses them as array of IDs for find(), so it's not a mass-assignment vector. Still, could be hardened.

Changed to

  def sort_params
    @sort_params ||= params.permit(fields_definition: [])[:fields_definition] || []
  end

[mayorova]

Because when we do not permit, strong parameters will prevent assignment. If this thing is not used for mass assignment then it is safer not to permit anything so we assure it is not used for assignment purposes. Once we permit, it then can be used for unintended purposes.

Yeah, but in this case we permit a specific list of params (well, a single param, actually), so I think it's still safe.

But I agree that

@sort_params ||= params.permit(fields_definition: [])[:fields_definition] || []

is probably not any safer than

@sort_params ||= params[:fields_definition] || []

just harder to read. So, if you with, I can change it.

[akostadinov]

But I agree that

@sort_params ||= params.permit(fields_definition: [])[:fields_definition] || []

is probably not any safer than

@sort_params ||= params[:fields_definition] || []

just harder to read. So, if you with, I can change it.

You are right that this would only permit a simple scalar array and not any nested params. Still I think it makes more sense and is prettier not to perform the permit given it makes no difference as an end result. One would wonder later why is this expression there, was there some hidden meaning behind it. And given it is uglier like that, I will appreciate if you get rid of it.

Actually it makes a difference.

p = ActionController::Parameters.new(user_id: "15", expires_in: "tomorrow", fields_definition: {a: "1", b: "2"})
pp = p.permit(fields_definition: [])
pp[:fields_definition]
=> nil

So it is even better to keep the permit as you have it right now.

[mayorova]

Actually it makes a difference.

Yeah, right 😅

This complex expression does, actually, protect from unexpected format of the sort params. Clearer example:

Standard usage (both work the same way)

>> params = ActionController::Parameters.new({"fields_definition"=>["1", "2", "3"]})
=> #<ActionController::Parameters {"fields_definition"=>["1", "2", "3"]} permitted: false>
>> params.permit(fields_definition: [])[:fields_definition] || []
=> ["1", "2", "3"]
>> params[:fields_definition] || []
=> ["1", "2", "3"]

fields_definition is an object instead of array

>> params = ActionController::Parameters.new({"fields_definition"=>{ "a": "1", "b": "2"}})
=> #<ActionController::Parameters {"fields_definition"=>{"a"=>"1", "b"=>"2"}} permitted: false>
>> params.permit(fields_definition: [])[:fields_definition] || []
=> Unpermitted parameter: :fields_definition. Context: {  }
[]
>> params[:fields_definition] || []
=> #<ActionController::Parameters {"a"=>"1", "b"=>"2"} permitted: false>

fields_definition is an array of objects

>> params = ActionController::Parameters.new({"fields_definition"=>[{ "a": "1"}, {"b": "1" } ]})
=> #<ActionController::Parameters {"fields_definition"=>[{"a"=>"1"}, {"b"=>"1"}]} permitted: false>
>> params.permit(fields_definition: [])[:fields_definition] || []
=> Unpermitted parameter: :fields_definition. Context: {  }
[]
>> params[:fields_definition] || []
=> [#<ActionController::Parameters {"a"=>"1"} permitted: false>, #<ActionController::Parameters {"b"=>"1"} permitted: false>]

[akostadinov]

Suggested change

	def customer_params
	def customer_hash

If we want it to remain params, then remove to_h call here and call it wherever we need to provide a hash.

[akostadinov]

configuration I assume is never known especially custom policies will have an unknown configuration structure, so we permit anything, correct?

Should we try to limit to a single level of hierarchy or something? Or this is safe?

[mayorova]

That's correct. But actually, I am thinking - what if configuration is a string (and not a hash)? 🤔 then this change would break it.

[akostadinov]

can it be a string?

[mayorova]

Well, not sure to be honest... Trying to make sense of https://docs.redhat.com/en/documentation/red_hat_3scale_api_management/2.16/html-single/administering_the_api_gateway/index

It seems that either everything is JSON (like the whole policies_config parameter), or everything is a hash.
So, dunno, I guess like it is, it's fine. Hopefully 🙃

[akostadinov]

Maybe we need to look at the user of that configuration. I will be surprised if it can consume a string.

[mayorova]

So, this can be reproduced with this test:

test 'updates policies config when policies_config is not a JSON string' do
    config = [
      { name: 'apicast', version: 'builtin', configuration: '{"one": 1, "two": 2}', enabled: true }
    ]

(if we change `configuration: {}´ to the value above).

Indeed, I don't think it's a valid value, because it get serialized in the DB in a weird way:

[{"name":"apicast","version":"builtin","configuration":"{\"one\": 1, \"two\": 2]","enabled":"true"}]

And probably it will not work (i.e. that serialized JSON within configuration is not going to be deserialized, I think).

However, it is a change compared to the original permit! which does allow this value (or any other value for "configuration").

So, if we had this test in master, it would pass in master, but fail in the branch (because with permit(configuration: {}) a string-like configuration will be filtered out.

So, not sure, whether we want to keep the previous behavior (probably safer), or introduce the change which is conceptually more correct.

[akostadinov]

Same here, either

Suggested change

	def customer_params
	billing_address_params = %i[company street_address postal_code locality region country_name]
	params.require(:customer).permit(:first_name, :last_name, :phone, credit_card: { billing_address: billing_address_params }).to_h
	def customer_hash
	billing_address_params = %i[company street_address postal_code locality region country_name]
	params.require(:customer).permit(:first_name, :last_name, :phone, credit_card: { billing_address: billing_address_params }).to_h

or

Suggested change

	def customer_params
	billing_address_params = %i[company street_address postal_code locality region country_name]
	params.require(:customer).permit(:first_name, :last_name, :phone, credit_card: { billing_address: billing_address_params }).to_h
	def customer_params
	billing_address_params = %i[company street_address postal_code locality region country_name]
	params.require(:customer).permit(:first_name, :last_name, :phone, credit_card: { billing_address: billing_address_params })

[akostadinov]

A couple of nitpicks and a few questions but looks great!

[mayorova]

Regarding the issues from this comment: #4248 (comment)

#: 1
File: app/controllers/provider/signups_controller.rb
Line: 110
Reason: params.permit!.to_h — blanket permit on all request params

Addressed in #4249

#: 2
File: lib/developer_portal/.../braintree_blue_controller.rb
Line: 22
Reason: params.require(:customer).permit! — user form data
────────────────────────────────────────
#: 3
File: app/controllers/.../braintree_blue_controller.rb
Line: 36
Reason: params.require(:customer).permit! — user form data

Not a big concern, because it's not used to update models, but to send params to braintree through braintree gem.

But it is good to filter the params anyway. Done in 18e65cc

#: 4
File: app/services/policies_config_params.rb
Line: 15
Reason: policies_config.try(:permit!) — user-supplied policy config

Changed in 18e65cc

#: 5
File: app/controllers/admin/api/registry/policies_controller.rb
Line: 57
Reason: .permit! on merged params including user-supplied schema

Couldn't find a way to safely get rid of it, so just left a comment about why it's needed. In any case it only makes schema to accept anything inside, all other parameters that we are not accepting are filtered out.

#: 6
File: app/controllers/master/redhat/auth_controller.rb
Line: 24
Reason: params.except(...).merge(...).permit! — user OAuth params, open
redirect risk

It's only used for url_for, not for mass assignment. It's a bit tricky to figure out what should be the allowed list.
In any case, it was already there, this PR does not make it any worse.

#: 7
File: app/lib/three_scale/api/responder.rb
Line: 34
Reason: Merges query_parameters (user input) then permit!

I couldn't figure out how to reach #navigation_location method whether this permit! appears 🤷 In any case - not something that was affected by the PR changes.

#: 8
File: app/lib/authentication/strategy/base.rb
Line: 77
Reason: params.dup.permit! — user request params passed to signup_path

Not mass-assignment, so nothing new introduced in this PR.

#: 9
File: config/initializers/protected_attributes_hacks.rb
Line: 11
Reason: attributes.dup.permit! — can receive user-supplied attributes via
ActiveRecord

Addressed in #4255 (the file is deleted completely).

[akostadinov]

Awesome work!