THREESCALE-12434: Migrate from protected attributes to strong parameters - Part 1#4248
THREESCALE-12434: Migrate from protected attributes to strong parameters - Part 1#4248
Conversation
jlledom
left a comment
There was a problem hiding this comment.
If tests pass, approved 👍
|
Still a lot of references to |
ead0bc4 to
29161ea
Compare
ae221f6 to
0b8ead4
Compare
0b8ead4 to
6c69623
Compare
❌ 9 blocking issues (11 total)
@qltysh one-click actions:
|
6bd96d4 to
f3227e4
Compare
a08b299 to
d3304a9
Compare
jlledom
left a comment
There was a problem hiding this comment.
I left a few comments. The best catches are from Claude.
| FactoryBot.create(:limit_alert, account: provider, cinstance: provider.application_contracts.take) | ||
| app_plan = FactoryBot.create(:application_plan, issuer: service) | ||
| app_plan.customize | ||
| FactoryBot.create(:application_plan, name: "somehow_broken", original_id: app_plan.id, issuer: service).update(issuer_id: service.id + 100) |
There was a problem hiding this comment.
not setting this issuer_id anywhere? Is this not breaking any test?
There was a problem hiding this comment.
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?...
There was a problem hiding this comment.
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.
|
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? |
| <tr role="row"> | ||
| <td td role="cell" data-label="From"><%= link_to h(redirect.source), edit_provider_admin_cms_redirect_path(redirect) %></> | ||
| <td td role="cell" data-label="To"><%= redirect.target %></> | ||
| <td role="cell" data-label="From"><%= link_to h(redirect.source), edit_provider_admin_cms_redirect_path(redirect) %></td> |
There was a problem hiding this comment.
This fix probably shouldn't be here, but this was just so wrong...
Well, the concept is that no test should fail after the changes:
This is mostly respected, rather than changing the tests, I just added additional tests that I found missing.
Well, the approach is quite simple/stupid:
When writing new tests, I sometimes would do this:
What I did not do in the tests was including some attribute in Anyway, I think we can rely on the |
|
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 ReportDate: 2026-04-16 Summary
CRITICAL FINDINGS (Action Required)1. FAIL: Settings model - premature
|
| 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_paramsreturnsparams.require(:topic)with NO.permit()call- Used in
build(topic_params)(line 36) and@topic.attributes = topic_params(line 52) - Old
attr_accessiblewhitelisted::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 inbuild()andupdate() - Old
attr_protectedblacklisted::forum_id, :tenant_id :forum_idand:tenant_idare now mass-assignable
c) Fields::Provider#for (app/lib/fields/provider.rb:14)
- Calls
klass.protected_attributeswhich changes behavior whenattr_protectedis 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 |
It has some good finds, thank you! ❤️ I'll review it carefully. |
Addressed in f9bd9d6
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.
More forum-related stuff, removed in #4281
This is a legit find, fixed in 9e03105
These are dead code (no views, no tests), removed in #4281 |
This and other
This error should not happen, because in the UI controller
True, but
Changed to |
- AuthenticationProvider::RedhatCustomerPortal - AuthenticationProvider::ServiceDiscoveryProvider - CMS::GroupSection - CMS::Permission - ApplicationKey - BackendEvent - LineItem - Onboarding - Partner - Post - ReferrerFilter
- Switches - SystemName - Finance::BillingStrategy - Alert - Forum - Invitation - MessageRecipient - Metric - Moderatorship - PaymentDetail - PaymentTransaction - PlanMetric - Profile - Topic - TopicCategory - UserTopic
c255696 to
4b5523f
Compare
|
wrt I think it makes more sense to avoid the permit and just have 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. I noticed several times AI suggesting to use permit where it is not needed because it (IMO wrongly) assumes that this is safer. I've run another grep for Also any Further analysis: |
|
Here is the except grep for completeness :) |
|
Basically to be covered for security issues I think we need to grep for:
|
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: