Skip to content

Fix UserParameters#without being a no-op#7070

Open
dylangriffinshub wants to merge 1 commit into
rubyforgood:mainfrom
dylangriffinshub:fix-user-parameters-without-noop
Open

Fix UserParameters#without being a no-op#7070
dylangriffinshub wants to merge 1 commit into
rubyforgood:mainfrom
dylangriffinshub:fix-user-parameters-without-noop

Conversation

@dylangriffinshub

Copy link
Copy Markdown

Summary

Fixes #7067UserParameters#without compared symbol keys against ActionController::Parameters#reject!'s string keys, so its condition was always false and the method silently removed nothing. It also didn't return self, breaking the builder chain.

The only caller, CreateCasaAdminService#build, calls .without(:active, :type) expecting both to be stripped before CasaAdmin.new. Because the call was a no-op:

  • Any authenticated user could set active=false on a new admin via an extra field in the create request (DB default is true).
  • Passing type=<sibling class> raised an unrescued ActiveRecord::SubclassNotFound (500), since Rails' STI guard is the only thing preventing type escalation.

Fix

without now stringifies the incoming keys before comparing (matching how reject! yields keys), and returns self so it can still be chained.

Also replaces the pending stub in spec/values/user_parameters_spec.rb with real coverage for without, without_type, without_active, with_organization, and with_password.

Test plan

  • bundle exec rspec spec/values/user_parameters_spec.rb
  • Manually verify: as a volunteer/supervisor, POST /casa_admins with an extra type or active field no longer leaks through

#without compared symbol keys (its only caller, CreateCasaAdminService#build,
always passes symbols) against ActionController::Parameters#reject!'s
string keys, so the block condition was always false and nothing was
ever removed. It also didn't return self, breaking the builder chain.

This let :active and :type survive into CasaAdmin.new via
CreateCasaAdminService#build, letting any authenticated user set
active=false on a new admin, or trigger an unrescued
ActiveRecord::SubclassNotFound (500) by passing type=<sibling class>.

Fixes rubyforgood#7067

Also replaces the pending spec stub in spec/values/user_parameters_spec.rb
with real coverage of without, without_type, without_active,
with_organization, and with_password.
@fuentesjr

Copy link
Copy Markdown
Contributor

Nice, thanks for jumping on this. The fix looks right. One heads-up on overlap. I opened this issue while doing the spec/values stub replacements for #5563, and that spec work is up as #7068 (green, in review). It rewrites this same user_parameters_spec.rb from the stub, and covers more (permit allowlist, ParameterMissing, every builder, plus the supervisor/casa_admin/all_casa_admin sibling files). The catch: since that PR is test only, its #without specs pin down the current broken behavior (they assert the key is not removed and that self is not returned), so once your fix lands, those two specs will start failing and need updating.

Since the only overlap is this one file, maybe the cleanest split is this: let #7068 land the coverage first, then rebase this PR to keep the user_parameters.rb fix and just update those two #without specs to assert the corrected behavior (key removed, returns self), dropping the spec additions here since they'd already be covered. Your call and the maintainers'. Happy to go whichever way keeps it simplest for you. Either way, the user_parameters.rb fix here is the part that matters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ruby Touches Ruby code 🧪 Tests Tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: UserParameters#without is a no-op with symbol keys, leaking active/type into admin creation

2 participants