Bind the filter proc to the registry at filter time - #33
Conversation
Code review — #33 Late-bound filter procVerdict: 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 Verified
Strengths
Criticallib/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 Important
Minor
|
| # 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| |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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>
6d40764 to
894784d
Compare
The bug
Permittable::Railtieappendedfilter_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: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: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: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_procstill works for anyone calling it directly; only what the Railtie appends changed.Verification