Skip to content

Bind the filter proc to the registry at filter time - #33

Open
VSN2015 wants to merge 1 commit into
masterfrom
fix/late-bound-filter-proc
Open

Bind the filter proc to the registry at filter time#33
VSN2015 wants to merge 1 commit into
masterfrom
fix/late-bound-filter-proc

Conversation

@VSN2015

@VSN2015 VSN2015 commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Second silent-redaction failure of the same family as #21. Worth reading alongside it.

The bug

Permittable::Railtie appended filter_parameter_registry.to_proc — a proc bound to whichever registry instance existed at boot.

Rails runs railtie initializers before config/initializers, so a host gem or app that swaps the registry necessarily does so afterwards. Rails then goes on filtering through the old instance:

registered in the new registry?  true
what the boot-time filter sees:  {"ssn"=>"111-22-3333"}

The sensitive: field registered itself in the new registry. The appended proc consulted an empty one. The parameter was logged in the clear, with nothing to indicate it.

Why this is the normal case, not an exotic one

That swap is the entire reason filter_parameter_registry= exists. The gem's own comment says so:

The whole object is duck-typed (#add, #include?, #to_proc, #reset!) so a host can swap in its own registry via Permittable.filter_parameter_registry= and pool registrations. (concerns_on_rails does exactly this)

So anyone following the documented pooling recipe lost redaction — and there was no way to notice short of reading production logs.

The fix

The Railtie now appends Permittable.filter_parameter_proc, which resolves the registry at filter time:

def filter_parameter_proc
  @filter_parameter_proc ||= lambda do |key, value|
    filter_parameter_registry.to_proc.call(key, value)
  end
end
registered in the new registry?  true
what the boot-time filter sees:  {"ssn"=>"[FILTERED]"}

It's a stable object, so the Railtie's idempotence check (include? before <<) still holds across repeated initializer runs — there's a spec pinning that, because the obvious implementation (building a fresh lambda each call) would break it.

filter_parameter_registry.to_proc still works for anyone calling it directly; only what the Railtie appends changed.

Verification

  • 202 examples, 0 failures (3 new, written before the fix): redaction through a registry swapped after boot, proc stability across a swap, and the un-swapped default path still redacting
  • RuboCop clean

@VSN2015

VSN2015 commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Code review — #33 Late-bound filter proc

Verdict: not ready; one Critical regression. The diagnosis is correct and I reproduced both the bug and the fix. The Rails proc contract, idempotence and per-request cost are all sound. But the fix is one-directional: after a swap, nothing consults the default registry any more, so sensitive: fields registered before the swap are now logged in the clear. That is the mirror image of the bug being fixed, and concerns_on_rails documents relying on the old behaviour.

Verified

  • bundle exec rspec at 6d40764: 202 examples, 0 failures. rubocop: no offenses.
  • Read activesupport 8.1.3.1 parameter_filter.rb: the new lambda has arity 2, is called with (key, value), and forwards to the registry's inner lambda which does value.replace(FILTERED). Mutate-in-place contract preserved end to end.
  • Base reproduction: ParameterFilter.new([registry.to_proc]), swap registry, load controller → {"ssn"=>"111-22-3333"} while the new registry includes ssn. Bug reproduced exactly as the PR body shows.
  • Head: same steps → {"ssn"=>"[FILTERED]"}. Fix confirmed.
  • Orphaning: load controller A (sensitive: :ssn) BEFORE swap, swap to a fresh registry, load controller B (sensitive: :pin) AFTER swap, filter both keys:
    • base proc → {"ssn"=>"[FILTERED]", "pin"=>"1234"}
    • this PR → {"ssn"=>"111-22-3333", "pin"=>"[FILTERED]"}
  • Cost: allocations identical (2004 vs 2003 for 1001 keys), time within noise. filter_parameter_proc is the same object across calls and across a swap.
  • Registry swapped to a Set (no to_proc) → NoMethodError at filter time on every request. Host to_proc with arity 3 → ArgumentError through the wrapper.
  • concerns_on_rails (local checkout): swaps the registry from an autoloaded file, so the swap runs at first constant reference, not from config/initializers. Its bridge comment states that contracts declared before the bridge loads "registered on the gem's default registry — that one stays appended by Permittable::Railtie, so nothing is un-filtered". This PR removes the behaviour that comment depends on.

Strengths

  • lib/permittable.rb:177-181 is the minimal correct shape: arity-2 lambda, works identically on AS 5.x and 6+, verified against the AS source.
  • Memoizing the lambda (:178) keeps the Railtie's include?-before-<< idempotence (lib/permittable/railtie.rb:15-16), and spec/permittable_spec.rb:634-638 pins identity across a swap.
  • Because the same proc object is what ActiveRecord snapshots into filter_attributes, the fix reaches #inspect redaction too.
  • Specs drive the real ActiveSupport::ParameterFilter with the real DSL (spec/permittable_spec.rb:620-644), with a "nothing swapped" control case.

Critical

lib/permittable.rb:163 (writer) and :177-181 (proc): registrations made before a swap are silently orphaned. The proc consults only the current registry, so anything registered on the default registry before filter_parameter_registry= runs is never redacted again. Permittable::Contract.define also registers sensitive: fields (spec/contract_spec.rb:131), so contracts defined in files loaded before the swap are affected in any host. README.md:399 now promises "The swap works at any point", which is not true. Fix: make the writer carry the previous registry's names into the new one via the duck-typed #add, e.g. expose FilterParameterRegistry#fields and in the writer do previous.fields.each { |n| registry.add(n) } when swapping away from a FilterParameterRegistry. Add a spec: "registered before the swap, still redacted after". Then reword README:399 and the CHANGELOG.

Important

  1. lib/permittable.rb:163: the writer accepts anything, and duck-type failures now surface per request. Before, a swapped-in object lacking to_proc was silently never consulted; now every request raises NoMethodError inside process_action. Loud beats silent, but fail at the swap: raise ArgumentError unless registry.nil? || registry.respond_to?(:to_proc), as the mode= setter already does for its values. Also either forward by arity (declare the wrapper ->(key, value, original = nil) and call the inner by its arity) or state "arity-2 callable" in the duck-type comment at filter_parameter_registry.rb:12-14 and README.md:397.
  2. The Railtie wiring is untested here and the regression above has no spec. Nothing under spec/ at this head references Railtie or config.filter_parameters. The missing "registered before swap" example is what let the Critical through. Add it next to spec/permittable_spec.rb:624, and once Cover the Railtie and rake tasks with a real Rails boot #36's real-boot spec/railtie_spec.rb lands, add the swap scenario there.

Minor

  • CHANGELOG.md:6 and the PR body overstate the concerns_on_rails exposure and omit the real one. That host appends its own pooled proc, so post-swap registrations were already redacted through it; the case it explicitly relied on (pre-swap) is the one this PR regresses. With both, the pooled proc runs twice per key (idempotent, negligible, worth a sentence).
  • README.md:603-604: the Module API table has no row for Permittable.filter_parameter_proc, though README:399 and the code comment present it as public.
  • lib/permittable.rb:178: @filter_parameter_proc ||= is not under @registry_mutex, unlike the getter at :157-161. Benign at boot, but a frozen module-level constant lambda would remove the memo, the race, and the need for the identity spec.
  • Pre-existing, in the proc this PR wraps (filter_parameter_registry.rb:27): non-String values under a sensitive key pass through unredacted ("ssn" => 111223333), because the inner lambda only handles Strings. Inherent to Rails proc filters; deserves a README caveat in "Sensitive parameters".

@VSN2015 VSN2015 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of PR #33: Late-binding the parameter filter proc fixes a real-world security issue where swapping Permittable.filter_parameter_registry in initializers orphaned the proc appended during railtie boot.

Comment thread lib/permittable.rb Outdated
# A stable object, so the Railtie's idempotence check (include? before <<)
# still holds across repeated initializer runs.
def filter_parameter_proc
@filter_parameter_proc ||= lambda do |key, value|

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolving filter_parameter_registry.to_proc dynamically within filter_parameter_proc ensures that whichever registry instance is active at filter time is the one performing redaction, regardless of initializer load order.

# Late-bound on purpose — see Permittable.filter_parameter_proc. This
# initializer runs before config/initializers, so a registry swapped
# there must still be the one consulted at filter time.
filter = ::Permittable.filter_parameter_proc

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appending ::Permittable.filter_parameter_proc maintains a stable object identity for the Railtie's idempotency check while deferring registry lookup.

Railtie appended filter_parameter_registry.to_proc — a proc bound to
whichever registry INSTANCE existed at boot. Rails runs railtie
initializers before config/initializers, so a host gem or app that
swaps the registry necessarily does so afterwards, and Rails went on
filtering through the old instance:

  registered in the new registry?  true
  what the boot-time filter sees:  {"ssn"=>"111-22-3333"}

The sensitive: field registered itself in the new registry, the
appended proc consulted an empty one, and the parameter was logged in
the clear with nothing to indicate it.

That swap is the entire reason the writer exists — the gem's own
comment names concerns_on_rails as doing exactly this — so the broken
ordering was the normal case, not an exotic one. Anyone following the
documented pooling recipe lost redaction.

Railtie now appends Permittable.filter_parameter_proc, which resolves
the registry at filter time. It is a stable object, so the Railtie's
idempotence check (include? before <<) still holds across repeated
initializer runs, and a spec pins that too.

Second silent-redaction failure of the same family as the nested
sensitive: cascade: the declaration looked right, the registry looked
right, and nothing was redacted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant