From a7261598a86e2d3aa10fcbcc34b8ca0681f3fed9 Mon Sep 17 00:00:00 2001 From: Sang Date: Sat, 5 Sep 2026 00:23:41 +0700 Subject: [PATCH] Fix sensitive: on a nested block redacting nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A contract declaring optional :payment, sensitive: true do required :card_number, :string end printed the card number in the clear. sensitive: on a container was a complete no-op, and it looked correct in review. Rails' parameter filtering matches the leaf key it is currently looking at, never the path that led there. Registering only the container's own name therefore redacts nothing: the filter proc is handed ("payment", {...}), a Hash is not a String so nothing is replaced, and ParameterFilter then recurses and asks about "card_number" — which the container's name never matches. sensitive: now cascades to every field inside a nested or array container, at any depth. The spec proves it through ActiveSupport::ParameterFilter on a realistic payload rather than only asserting on the registry, which is what let the original behaviour pass: the existing spec marked the SUB-field sensitive, so it never exercised a marked container at all. A sub-field can opt out with sensitive: false. That escape hatch is load-bearing rather than decorative: matching is a case-insensitive SUBSTRING match, so cascading a generic name like :id would redact every parameter in the app containing "id" — user_id, valid, identity — which is occasionally a worse outcome than the leak it prevents. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 11 ++++ README.md | 16 ++++++ lib/permittable.rb | 63 ++++++++++++++++++++++- spec/json_schema_spec.rb | 11 ++++ spec/matchers_spec.rb | 16 ++++++ spec/permittable_spec.rb | 108 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 223 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 302b533..5b11d93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ +## Unreleased + +### Added +- **`sensitive: false` opts a sub-field out of an inherited cascade.** Matching is a case-insensitive **substring** match, so cascading a generic name like `:id` or `:name` would redact every parameter in the app that happens to contain it — occasionally a worse outcome than the leak it prevents. An explicit `sensitive: false` on a field (or on a container, for its whole subtree) keeps it readable. Only `false` opts out — `sensitive: nil` reads as "not stated" and still inherits. + +### Upgrading +- **A contract that already declares `sensitive: true` on a nested block or array will redact more than it did before.** That is the point of the fix, but the widening is app-wide and worth a look before deploying: every cascaded child's name is registered as a case-insensitive **substring** filter, so a child called `id`, `name`, `type`, `status` or `zip` starts redacting `user_id`, `company_name`, `content_type` and `gzip` in **every** controller's logs, not only in the contract that declared it. Run `grep -n "sensitive: true" app/controllers` and add `sensitive: false` to any child whose name is too generic to filter globally. + +### Fixed +- **`sensitive: true` on a nested block or array was a complete no-op, and logged the values it promised to redact.** Rails' parameter filtering walks into hashes and arrays itself and asks a proc filter about the **leaf values only**, handing it the leaf's own key and never the path that led there — so registering only the container's name redacted nothing: the filter descended and asked about `"card_number"`, which the container's name does not match. A contract declaring `optional :payment, sensitive: true do required :card_number, :string end` printed the card number in the clear. `sensitive:` now **cascades** to every field inside a nested or array container, at any depth, and a spec proves it through `ActiveSupport::ParameterFilter` rather than only asserting on the registry. The cascade is resolved onto the field data at class load, so every reader of a contract agrees with the redaction: the exported JSON Schema marks a cascaded child `writeOnly`, and the RSpec matcher's `.sensitive` chain passes for it. + ## 0.6.0 (2026-09-08) diff --git a/README.md b/README.md index c42e216..f129036 100644 --- a/README.md +++ b/README.md @@ -553,6 +553,22 @@ Mark a field `sensitive: true` and its name is registered with `Permittable.filt optional :ssn, :string, sensitive: true ``` +**On a nested block or an array, `sensitive:` cascades to everything inside it:** + +```ruby +optional :payment, sensitive: true do + required :card_number, :string # redacted + optional :cvv, :string # redacted + optional :id, :string, sensitive: false # NOT redacted — see below +end +``` + +It has to. Rails' parameter filtering walks into hashes and arrays itself and asks a proc filter about the **leaf values only**, handing it the leaf's own key and never the path that led there. So registering `payment` alone redacts nothing inside it: the filter descends and asks about `card_number`, which the container's name does not match. + +A sub-field opts out with an explicit `sensitive: false`. That exists because matching is a case-insensitive **substring** match, so cascading a generic name like `:id` or `:name` would redact every parameter in the app that happens to contain it — occasionally a worse outcome than the leak it prevents. Only `false` opts out; `sensitive: nil` reads as "not stated" and still inherits. + +The cascade is resolved onto the field when the contract loads, so everything that reads a contract agrees: the value is redacted from logs, the exported schema marks the child `writeOnly`, and `permit_param("payment.card_number").sensitive` passes. + The indirection is deliberate. Appending plain symbols to `config.filter_parameters` at class-load time misses every consumer that snapshots the list at boot — ActiveRecord's `filter_attributes` copy, lograge-style initializers, precompiled filters. A **single proc appended once at boot, consulting a live registry at filter time**, means fields registered when a controller loads later (lazy loading in development) are still redacted. The initializer runs before `active_record.set_filter_attributes`, so values are redacted from both request logs and `#inspect`. Matching mirrors Rails' own symbol-filter semantics: case-insensitive substring match on the parameter key. The registry is fully duck-typed (`#add`, `#include?`, `#to_proc`, `#reset!`) and swappable via `Permittable.filter_parameter_registry=`, so a host gem can pool registrations into its own. diff --git a/lib/permittable.rb b/lib/permittable.rb index 31973a6..c993890 100644 --- a/lib/permittable.rb +++ b/lib/permittable.rb @@ -127,7 +127,13 @@ # `sensitive: true` registers the field name with # Permittable.filter_parameter_registry (swappable — a host gem can point it # at its own registry), consulted at filter time by the proc -# Permittable::Railtie appends to `config.filter_parameters`. +# Permittable::Railtie appends to `config.filter_parameters`. On a nested or +# array field it CASCADES to every field inside, because Rails' filtering +# asks about the leaf key it is looking at rather than the path to it; a +# sub-field opts out with `sensitive: false`, since matching is a substring +# match and a generic cascaded name would redact half the app's logs. The +# cascade is resolved onto the field data at class load — see +# ContractBuilder#cascade_sensitive. # # OUTPUT RESHAPING — the safe replacement for params-mutating before_actions. # Two layers, both operating on the validated COPY (the request's `params` is @@ -495,7 +501,7 @@ def array(name, **opts, &block) if block raise ArgumentError, "#{LABEL}: array :#{name} takes of: OR a block, not both" if opts.key?(:of) - field[:fields] = nested_fields!(name, &block) + field[:fields] = cascade_sensitive(nested_fields!(name, &block), field[:sensitive]) field.delete(:of) else field[:of] = scalar_type!(name, opts[:of] || :string) @@ -519,6 +525,7 @@ def add_field(name, type, required:, opts:, &block) assert_opts!(name, opts, NESTED_OPTS) field = { name: name, kind: :nested, required: required, fields: nested_fields!(name, &block), **opts } + field[:fields] = cascade_sensitive(field[:fields], field[:sensitive]) validate_message!(field) elsif type&.to_sym == JSON_TYPE assert_opts!(name, opts, JSON_OPTS) @@ -570,6 +577,43 @@ def nested_fields!(name, &) fields end + # `sensitive: true` on a nested or array field CASCADES to every field + # inside it, and the cascade is resolved HERE, at class load, so that + # `field[:sensitive]` stays the single source of truth every reader + # consults: the filter registry, the exported schema's `writeOnly`, and + # the RSpec matcher's `.sensitive` chain. Resolving it privately inside + # the registry walk would have redacted a cascaded child at runtime + # while the schema and the matcher went on calling it public. + # + # It has to cascade: ActiveSupport::ParameterFilter recurses into Hash + # and Array values itself and consults proc filters only for the LEAVES, + # handing each one the leaf's own key and never the path that led there. + # So registering only `payment` is asked about `card_number`, which it + # does not match, and redacts nothing inside the container. + # + # A sub-field opts out with an explicit `sensitive: false`, because + # matching is a case-insensitive SUBSTRING match and cascading a generic + # name (:id, :name) would redact every parameter app-wide that contains + # it. Only `false` opts out; `sensitive: nil` reads as "not stated" and + # still inherits. + def cascade_sensitive(fields, inherited) + updated = fields.map { |field| cascade_field_sensitive(field, inherited) } + updated.zip(fields).all? { |new_field, old| new_field.equal?(old) } ? fields : updated.freeze + end + + def cascade_field_sensitive(field, inherited) + declared = field[:sensitive] + effective = declared.nil? ? inherited : declared + children = field[:fields] ? cascade_sensitive(field[:fields], effective) : nil + unchanged = (effective ? declared == true : declared == false || !field.key?(:sensitive)) && + (children.nil? || children.equal?(field[:fields])) + return field if unchanged + + updated = field.merge(sensitive: effective) + updated[:fields] = children if children + updated.freeze + end + def validate_scalar_opts!(field) name = field[:name] if field[:required] && field.key?(:default) @@ -846,6 +890,21 @@ def guard_contract_columns!(model_class, fields) end end + # `sensitive: true` on a nested or array field CASCADES to everything + # inside it, because Rails' parameter filtering matches the leaf key it is + # currently looking at — never the path that led there. Registering only + # the container's own name therefore redacted nothing it promised: the + # filter is handed ("payment", {...}), a Hash is not a String so nothing + # is replaced, and it then recurses and asks about "card_number", which + # was never registered. + # + # A sub-field opts out with an explicit `sensitive: false`. That escape + # hatch exists because matching is a case-insensitive SUBSTRING match, so + # cascading a generic name (:id, :name) would redact every parameter + # app-wide that happens to contain it — occasionally a worse outcome than + # the leak it prevents. + # The cascade is already resolved on the field data (see + # ContractBuilder#cascade_sensitive), so this only has to read it. def register_sensitive_params(fields) fields.each do |field| Permittable.filter_parameter_registry.add(field[:name]) if field[:sensitive] diff --git a/spec/json_schema_spec.rb b/spec/json_schema_spec.rb index 939befa..bf7613a 100644 --- a/spec/json_schema_spec.rb +++ b/spec/json_schema_spec.rb @@ -117,6 +117,17 @@ def property(name, **opts, &contract) expect(props["slug"]).not_to have_key("pattern") expect(props["tags"]["x-permittable-transformed"]).to be(true) end + + it "marks a child that inherited sensitive: from its container writeOnly too" do + props = schema_for do + optional :payment, sensitive: true do + required :card_number, :string + optional :id, :string, sensitive: false + end + end["properties"]["payment"]["properties"] + expect(props["card_number"]).to include("writeOnly" => true, "x-permittable-sensitive" => true) + expect(props["id"]).not_to have_key("writeOnly") + end end describe "nullable:" do diff --git a/spec/matchers_spec.rb b/spec/matchers_spec.rb index 01abb7a..0a0036d 100644 --- a/spec/matchers_spec.rb +++ b/spec/matchers_spec.rb @@ -77,6 +77,22 @@ def failure_of expect(controller).to permit_param(:ssn).for_action(:create).virtual.sensitive end + it "sees sensitive: on a child that inherited it from its container" do + cascaded = Class.new(FakeController) do + include Permittable + + permit_params(:create) do + optional :payment, sensitive: true do + required :card_number, :string + optional :id, :string, sensitive: false + end + end + end + expect(cascaded).to permit_param("payment.card_number").sensitive + expect(failure_of { expect(cascaded).to permit_param("payment.id").sensitive }) + .to include("expected the field to be sensitive") + end + it "checks the nullable flag" do nullable = Class.new(FakeController) do include Permittable diff --git a/spec/permittable_spec.rb b/spec/permittable_spec.rb index 887ffaf..c6d9c9d 100644 --- a/spec/permittable_spec.rb +++ b/spec/permittable_spec.rb @@ -904,6 +904,114 @@ def violations_for(params, action: "create", &declaration) expect(Permittable.filter_parameter_registry.include?("name")).to be(false) end + it "cascades sensitive: from a nested block to every field inside it" do + permittable_class do + permit_params(:create) do + optional :payment, sensitive: true do + required :card_number, :string + optional :cvv, :string + optional :billing do + optional :postcode, :string + end + end + end + end + registry = Permittable.filter_parameter_registry + %w[payment card_number cvv billing postcode].each do |name| + expect(registry.include?(name)).to be(true), "expected :#{name} to be registered" + end + end + + it "cascades sensitive: from an array block to its element fields" do + permittable_class do + permit_params(:create) do + array :cards, sensitive: true do + required :pan, :string + optional :expiry, :string + end + end + end + expect(Permittable.filter_parameter_registry.include?("pan")).to be(true) + expect(Permittable.filter_parameter_registry.include?("expiry")).to be(true) + end + + it "lets a sub-field opt OUT with sensitive: false, for a name too generic to redact app-wide" do + permittable_class do + permit_params(:create) do + optional :payment, sensitive: true do + required :card_number, :string + # "id" would match user_id, valid, identity... app-wide. + optional :id, :string, sensitive: false + optional :meta, sensitive: false do + optional :name, :string + end + end + end + end + registry = Permittable.filter_parameter_registry + expect(registry.include?("card_number")).to be(true) + expect(registry.include?("id")).to be(false) + expect(registry.include?("meta")).to be(false) + expect(registry.include?("name")).to be(false) + end + + it "does not register anything inside a container that is not sensitive" do + permittable_class do + permit_params(:create) do + optional :bank do + required :iban, :string, sensitive: true + optional :branch, :string + end + end + end + registry = Permittable.filter_parameter_registry + expect(registry.include?("iban")).to be(true) + expect(registry.include?("bank")).to be(false) + expect(registry.include?("branch")).to be(false) + end + + it "actually redacts the nested values a Rails log would print" do + permittable_class do + permit_params(:create) do + optional :ssn, :string, sensitive: true + optional :payment, sensitive: true do + required :card_number, :string + optional :cvv, :string + optional :billing do + optional :postcode, :string + end + end + array :cards, sensitive: true do + required :pan, :string + end + end + end + filter = ActiveSupport::ParameterFilter.new([Permittable.filter_parameter_registry.to_proc]) + expect(filter.filter("ssn" => "111-22-3333", + "payment" => { "card_number" => "4111111111111111", "cvv" => "123", + "billing" => { "postcode" => "SW1A 1AA" } }, + "cards" => [{ "pan" => "5555555555554444" }])) + .to eq("ssn" => "[FILTERED]", + "payment" => { "card_number" => "[FILTERED]", "cvv" => "[FILTERED]", + "billing" => { "postcode" => "[FILTERED]" } }, + "cards" => [{ "pan" => "[FILTERED]" }]) + end + + it "stamps the cascade onto the field, so every reader of the contract agrees" do + klass = permittable_class do + permit_params(:create) do + optional :payment, sensitive: true do + required :card_number, :string + optional :id, :string, sensitive: false + end + end + end + payment = klass.permit_rule_for(:create)[:fields].first + card_number, id = payment[:fields] + expect(card_number[:sensitive]).to be(true) + expect(id[:sensitive]).to be(false) + end + it "instruments invalid_parameters.permittable with the violation details" do events = [] subscription = ActiveSupport::Notifications.subscribe("invalid_parameters.permittable") do |*, payload|