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|