diff --git a/CHANGELOG.md b/CHANGELOG.md index 302b533..719fa02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ +## Unreleased + +### Fixed +- **A mutable `default:` was shared by every request.** Field declarations are frozen data, but the *value* an author wrote for `default:` was not, and `HashWithIndifferentAccess` hands a non-frozen Array — and any String — to the result **by reference**. So `array :tags, of: :string, default: []` gave every request the same Array: one request appending to `permitted_params[:tags]` corrupted the default for every later request in the process, and `Model.new(tags: …)` assigns that same object, so `record.tags << x` was enough to trigger it. The corruption outlived the request and lasted for the life of the process. A `default:` (and a documentation `example:`) is now **deep-copied and frozen at class load**, so the contract cannot be corrupted, and each request is handed its own copy. The copy is the point: the object the host app passed in is never frozen, in case it is still using it. +- **`normalize:` could manufacture an empty value that walked past `required`.** `""` is documented as absent, so a required field must violate — but normalization ran *after* absence had already been decided. `required :name, :string, normalize: :squish` therefore rejected `""` as `missing` and **accepted `" "` as `""`**, writing an empty string into the column: exactly the silent corruption strict coercion exists to refuse, delivered by the gem's own preset. `normalize:` now runs **first**, before the absence rule, so a value that normalizes to empty is absent like any other empty value — it takes the `nullable:` / `default:` / `missing` branch. There is still exactly one reading of absence, and a `normalize:` Proc is still called exactly once per value. Relatedly, a `default:` is now **stored** in the form it was validated in: `default: " free "` with `normalize: :squish` was checked as `"free"` and used to be handed to requests as `" free "`. +- **`unknown: :error` rejected ordinary form submissions.** Only the router's `controller`/`action`/`format` were exempt from the top-level unknown-key check, but Rails also merges `authenticity_token`, `_method`, `utf8` and `commit` into a form POST — so the strictest setting was unusable outside a JSON API, and every browser form failed on four of the framework's own keys rather than on anything the client got wrong. Those four are now exempt at the top level too. The exemption covers the *check* only, and only at the top level: a form key smuggled inside a `root:` or a nested hash is still `unknown`, a standalone `Permittable::Contract` still exempts nothing (it has neither a router nor a form), and monitor mode still passes the form keys through in its raw hash, where behaving exactly like the pre-contract app is the whole promise and a legacy action may read `_method` itself. + +Contracts that declare no `default:`, no `normalize:`, and no `unknown: :error` are unaffected. + ## 0.6.0 (2026-09-08) diff --git a/README.md b/README.md index c42e216..b7670b3 100644 --- a/README.md +++ b/README.md @@ -210,7 +210,7 @@ Adopting on an existing API with live traffic? Skip ahead to [Adopting on a live request params │ ├─ 1 unwrap root: params[:user] missing or not a hash → 400 - ├─ 2 each field normalize → cast → validate → transform + ├─ 2 each field normalize → absent? → cast → validate → transform ├─ 3 unknown-key check at every nesting level (unknown: :ignore | :log | :error) ├─ 4 finalize only when nothing violated │ @@ -290,8 +290,8 @@ Which options are legal depends on the field kind — anything else raises at cl | `in:` | ✅ | — | — | Allowed values: a `Range` (bounds-checked with `cover?`) or an `Array` | | `format:` | ✅¹ | — | — | Regexp the value must match | | `length:` | ✅¹ | ✅ | — | `Range` or `Integer`. Character count on strings, **element count** on arrays | -| `normalize:` | ✅¹ | — | — | `:squish`, `:strip`, `:downcase`, `:upcase`, `:email`, or a Proc. Runs **before** the cast | -| `default:` | ✅ | ✅ | — | Value used when the field is absent. Validated against the field's own contract at class load | +| `normalize:` | ✅¹ | — | — | `:squish`, `:strip`, `:downcase`, `:upcase`, `:email`, or a Proc. Runs **first** — before the absence rule, so a value that normalizes to `""` is absent | +| `default:` | ✅ | ✅ | — | Value used when the field is absent. Validated against the field's own contract at class load, then stored normalized and frozen (each request gets its own copy) | | `validate:` | ✅ | ✅ | — | Callable. Falsy fails as `"invalid"`; a returned `Symbol` becomes the violation code | | `transform:` | ✅ | ✅ | — | Callable applied **after** cast and validation — see [output reshaping](#output-reshaping-transform-and-finalize) | | `virtual:` | ✅ | ✅ | ✅ | Exempt this field from the schema-drift guard | @@ -365,7 +365,7 @@ In [exported OpenAPI](#exporting-openapi-docs-that-cannot-drift) the field is `{ ### Absence, defaults, and partial updates -`nil` and `""` are **both treated as absent** — the query-parameter convention, where an untouched form field arrives as an empty string. Boolean `false` is present. +`nil` and `""` are **both treated as absent** — the query-parameter convention, where an untouched form field arrives as an empty string. Boolean `false` is present. `normalize:` runs *before* this rule, so a field declared `normalize: :squish` treats `" "` as absent too: whitespace cannot satisfy a `required` field by becoming `""`. That single rule produces the behaviour you want from a `PATCH`: @@ -487,7 +487,9 @@ Resolution order per violation: the field's own `message:` (String, or the Hash | `:log` | Dropped, with a `logger.warn` naming the full paths | | `:error` | Each undeclared key becomes an `unknown` violation | -Rails merges `controller`, `action`, and `format` into `params`; these are exempt at the top level so `unknown: :error` doesn't flag the router's own bookkeeping. Inside a `root:` or a nested hash there is no such exemption, because nothing legitimately injects keys there. +Rails merges its own keys into `params`: `controller`, `action`, and `format` from the router, plus `authenticity_token`, `_method`, `utf8`, and `commit` from an ordinary form POST. All seven are exempt at the top level, so `unknown: :error` flags what the *client* got wrong rather than what the framework added. Inside a `root:` or a nested hash there is no such exemption, because nothing legitimately injects keys there — and a standalone `Contract` exempts nothing at all, having neither a router nor a form. + +The exemption covers the *check* only. Monitor mode still hands back the form keys in its raw pass-through, where behaving exactly like the pre-contract app is the whole promise and a legacy action may read `_method` itself; only the router's three are dropped there. ### Output reshaping (`transform:` and `finalize`) diff --git a/lib/permittable.rb b/lib/permittable.rb index 31973a6..15c9a75 100644 --- a/lib/permittable.rb +++ b/lib/permittable.rb @@ -4,6 +4,7 @@ require "active_support/hash_with_indifferent_access" require "active_support/core_ext/hash/indifferent_access" # nested plain Hashes inside HWIA.new require "active_support/core_ext/class/attribute" +require "active_support/core_ext/object/deep_dup" # authored default:/example: values are copied before freezing require "active_support/core_ext/string/inflections" require "active_support/core_ext/string/filters" require "bigdecimal" @@ -98,7 +99,12 @@ # guess. nil and "" are both treated as ABSENT (the query-param convention): # absent optional fields are OMITTED from the result (so partial updates never # nil-out columns), absent required fields violate, and `default:` fills -# absence. +# absence. `normalize:` runs BEFORE that rule rather than inside the cast, so +# there is exactly one reading of absence and a value that normalizes to empty +# (" " under :squish) cannot satisfy a required field by becoming "". An +# authored `default:`/`example:` is stored normalized — the form it was +# validated in — and deep-frozen on a copy, so no request can corrupt it for +# the next. # # `nullable: true` splits that rule in two for one field, which is how a PATCH # clears a column: a key the client never sent stays absent (defaults apply, @@ -162,6 +168,16 @@ module Permittable # Rails merges routing bookkeeping into params; a top-level (root: false) # unknown-keys check must not flag them. ROUTING_KEYS = %w[controller action format].freeze + # Nor the keys an ordinary form POST carries — the CSRF token, the verb + # override, the encoding probe, and the submit button's name. Without this + # `unknown: :error` was unusable outside a JSON API: every browser form + # failed on the framework's own keys rather than on anything the client got + # wrong. Exempt from the CHECK only: unlike ROUTING_KEYS these are NOT + # stripped from monitor mode's raw pass-through, where handing back an + # untouched params hash is the whole promise and a legacy action may well + # read `_method` itself. + FORM_KEYS = %w[authenticity_token _method utf8 commit].freeze + UNCHECKED_TOP_LEVEL_KEYS = (ROUTING_KEYS + FORM_KEYS).freeze NORMALIZERS = { squish: ->(v) { v.squish }, @@ -250,10 +266,12 @@ module Coercion TRUE_VALUES = [true, "true", "1", 1].freeze FALSE_VALUES = [false, "false", "0", 0].freeze - # Full pipeline for one scalar field: normalize → cast → in / format / - # length / validate. + # Pipeline for one scalar field: cast → in / format / length / validate. + # `normalize:` is NOT applied here — it is its own stage, run by the + # caller before the absence rule (a value that normalizes to "" is absent + # like any other empty value), so normalizing again here would call a + # host's `normalize:` proc twice per value. def check_scalar(field, value) - value = apply_normalize(field[:normalize], value) status, value = cast(field[:type], value) return [status, value] unless status == :ok @@ -618,9 +636,9 @@ def validate_json_authored_value!(field, opt) raise ArgumentError, "#{LABEL}: :#{opt} for :#{field[:name]} must be a Hash" unless field[opt].is_a?(Hash) status, code = Coercion.check_json(field, field[opt]) - return if status == :ok + raise ArgumentError, "#{LABEL}: :#{opt} for field :#{field[:name]} violates its own contract (#{code})" unless status == :ok - raise ArgumentError, "#{LABEL}: :#{opt} for field :#{field[:name]} violates its own contract (#{code})" + field[opt] = freeze_authored(field[opt]) end # format / length / normalize reason about characters; on any other @@ -661,22 +679,30 @@ def resolve_normalizer!(field) # An authored value (`default:`, or a documentation `example:`) must # satisfy the field's own contract — catching a lie at class load beats # shipping it to every request (or publishing it in generated docs). + # The authored value is STORED normalized, because that is the form it was + # validated in: `default: " free "` with `normalize: :squish` was + # checked as "free" and used to be handed to requests as " free ". def validate_authored_value!(field, opt) return unless field.key?(opt) return if authored_nil!(field, opt) - status, code = Coercion.check_scalar(field, field[opt]) - return if status == :ok + value = Coercion.apply_normalize(field[:normalize], field[opt]) + status, code = Coercion.check_scalar(field, value) + raise ArgumentError, "#{LABEL}: :#{opt} for field :#{field[:name]} violates its own contract (#{code})" unless status == :ok - raise ArgumentError, "#{LABEL}: :#{opt} for field :#{field[:name]} violates its own contract (#{code})" + field[opt] = freeze_authored(value) end def validate_array_authored_value!(field, opt) value = field[opt] return if authored_nil!(field, opt) raise ArgumentError, "#{LABEL}: :#{opt} for array :#{field[:name]} must be an Array" unless value.is_a?(Array) - return unless field[:of] + validate_array_elements!(field, opt, value) if field[:of] + field[opt] = freeze_authored(value) + end + + def validate_array_elements!(field, opt, value) value.each do |element| status, code = Coercion.cast(field[:of], element) next if status == :ok @@ -685,6 +711,27 @@ def validate_array_authored_value!(field, opt) end end + # A contract is frozen data, but `@fields.map(&:freeze)` freezes only the + # field hashes — an authored `default:` or `example:` value stayed + # mutable, and HashWithIndifferentAccess hands a non-frozen Array (and + # any String) to the result BY REFERENCE. So one request appending to + # `permitted_params[:tags]` corrupted the default for every later request + # in the process. Freezing a COPY fixes that without freezing an object + # the host app passed in and may still be using. + def freeze_authored(value) + deep_freeze(value.deep_dup) + end + + def deep_freeze(value) + case value + when Hash + value.each_key { |key| deep_freeze(key) } + value.each_value { |element| deep_freeze(element) } + when Array then value.each { |element| deep_freeze(element) } + end + value.freeze + end + # An authored nil is only meaningful on a nullable field, where it says # "absent means clear" (PUT semantics) rather than "no default". On any # other field it is a value nil could never satisfy, so it fails at class @@ -1039,13 +1086,13 @@ def permittable_check_hash(fields, hash, path:, unknown:, top_level:, violations fields.each do |field| key = field[:name].to_s full = permittable_path(path, key) - value = hash[key] + value = permittable_normalized(field, hash[key]) if permittable_absent?(value, hash, key) if permittable_explicit_null?(field, hash, key) result[key] = nil elsif field.key?(:default) - result[key] = field[:default] + result[key] = permittable_default(field) elsif field[:required] violations << permittable_violation(field, full, "missing") end @@ -1062,6 +1109,7 @@ def permittable_check_field(field, value, full, result, unknown:, violations:) key = field[:name].to_s case field[:kind] when :scalar + # Already normalized by permittable_normalized, before the absence rule. permittable_check_whole(field, Coercion.check_scalar(field, value), full, result, violations: violations) when :json permittable_check_whole(field, Coercion.check_json(field, value), full, result, violations: violations) @@ -1127,6 +1175,26 @@ def permittable_check_element(field, element, path, unknown:, violations:) nil end + # `normalize:` runs BEFORE the absence rule, not inside the cast, so there + # stays exactly ONE reading of absence. Otherwise a value that normalizes to + # empty walked straight past it: `required :name, :string, normalize: + # :squish` rejected "" as missing but accepted " " as "" — the silent + # corruption strict coercion exists to refuse, delivered by the gem's own + # preset. Only scalars take normalize:. + def permittable_normalized(field, value) + field[:normalize] ? Coercion.apply_normalize(field[:normalize], value) : value + end + + # An authored default belongs to the contract, which is frozen data (see + # ContractBuilder#freeze_authored). HashWithIndifferentAccess copies a + # frozen Array or Hash as it assigns it, but stores a String as-is — so + # that one is copied here, leaving every value in the result the app's own + # to mutate. + def permittable_default(field) + value = field[:default] + value.is_a?(String) ? value.dup : value + end + # nil and "" are both ABSENT — see the module comment. def permittable_absent?(value, hash, key) !hash.key?(key) || value.nil? || (value.is_a?(String) && value.empty?) @@ -1146,7 +1214,7 @@ def permittable_check_unknown(fields, hash, path:, unknown:, top_level:, violati declared = fields.map { |f| f[:name].to_s } extra = hash.keys.map(&:to_s) - declared - extra -= ROUTING_KEYS if top_level + extra -= UNCHECKED_TOP_LEVEL_KEYS if top_level return if extra.empty? if unknown == :error diff --git a/spec/contract_spec.rb b/spec/contract_spec.rb index d77400a..a73807f 100644 --- a/spec/contract_spec.rb +++ b/spec/contract_spec.rb @@ -86,12 +86,13 @@ expect(c.call(starts_on: "2026-01-01", ends_on: "2026-01-02")).to be_valid end - it "does not exempt the router's bookkeeping keys — standalone input has no router" do + it "does not exempt the router's or a form's bookkeeping keys — standalone input has neither" do c = described_class.define(unknown: :error) { optional :name, :string } - result = c.call(name: "x", action: "boom", controller: "hax") + result = c.call(name: "x", action: "boom", controller: "hax", authenticity_token: "tok") expect(result.violations).to contain_exactly( { param: "action", code: "unknown" }, - { param: "controller", code: "unknown" } + { param: "controller", code: "unknown" }, + { param: "authenticity_token", code: "unknown" } ) end diff --git a/spec/permittable_spec.rb b/spec/permittable_spec.rb index 887ffaf..2e46388 100644 --- a/spec/permittable_spec.rb +++ b/spec/permittable_spec.rb @@ -337,6 +337,32 @@ def violations_for(params, action: "create", &declaration) expect(custom[:sku]).to eq("AB-1") end + it "treats a value that NORMALIZES to empty as absent, like any other empty value" do + decl = proc do + permit_params(:create) do + required :name, :string, normalize: :squish + optional :plan, :string, normalize: :strip, default: "free" + optional :note, :string, normalize: :strip, nullable: true + end + end + expect(violations_for({ name: " " }, &decl).details).to eq([{ param: "name", code: "missing" }]) + expect(permit({ name: "a", plan: " " }, &decl)[:plan]).to eq("free") + expect(permit({ name: "a", note: " " }, &decl)[:note]).to be_nil + expect(permit({ name: " a b " }, &decl)[:name]).to eq("a b") + end + + it "normalizes exactly once per value" do + calls = 0 + counting = lambda do |v| + calls += 1 + v.strip + end + result = permit({ name: " a " }) { permit_params(:create) { required :name, :string, normalize: counting } } + + expect(result[:name]).to eq("a") + expect(calls).to eq(1) + end + it "runs a custom validate: — falsy fails as 'invalid', a Symbol fails as that code, truthy passes" do falsy = proc { permit_params(:create) { required :n, :integer, validate: ->(v) { v.even? } } } expect(permit({ n: "4" }, &falsy)[:n]).to eq(4) @@ -477,6 +503,43 @@ def violations_for(params, action: "create", &declaration) result = permit({ ok: false }) { permit_params(:create) { required :ok, :boolean } } expect(result[:ok]).to be(false) end + + it "hands each request its own copy of a mutable default:" do + klass = permittable_class do + permit_params(:create) do + array :tags, of: :string, default: ["a"] + optional :plan, :string, default: "free" + optional :meta, :json, default: { "k" => "v" } + end + end + first = controller(klass).permitted_params + first[:tags] << "leak" + first[:plan] << "!" + first[:meta]["leak"] = true + + second = controller(klass).permitted_params + expect(second[:tags]).to eq(["a"]) + expect(second[:plan]).to eq("free") + expect(second[:meta].to_h).to eq("k" => "v") + expect(second[:tags]).not_to be(first[:tags]) + end + + it "freezes its own copy of an authored value, never the caller's object" do + authored = ["a"] + klass = permittable_class { permit_params(:create) { array :tags, of: :string, default: authored } } + stored = klass.permittable_contracts.last[:fields].first[:default] + + expect(stored).to be_frozen + expect(stored).not_to be(authored) + expect(authored).not_to be_frozen + end + + it "delivers a default: in the normalized form it was validated in" do + result = permit({}) do + permit_params(:create) { optional :plan, :string, normalize: :squish, default: " free " } + end + expect(result[:plan]).to eq("free") + end end describe "nullable:" do @@ -606,6 +669,27 @@ def violations_for(params, action: "create", &declaration) expect(e.details).to eq([{ param: "extra", code: "unknown" }]) end + it "skips the form bookkeeping keys Rails merges into a POST, flagging only the real stray" do + e = violations_for({ name: "a", extra: "x", authenticity_token: "tok", + _method: "patch", utf8: "✓", commit: "Save" }) do + permit_params(:create, unknown: :error) { required :name, :string } + end + expect(e.details).to eq([{ param: "extra", code: "unknown" }]) + end + + it "still flags a form key smuggled inside a root (the exemption is top-level only)" do + e = violations_for({ user: { name: "a", authenticity_token: "smuggled" } }) do + permit_params(:create, root: :user, unknown: :error) { required :name, :string } + end + expect(e.details).to eq([{ param: "user.authenticity_token", code: "unknown" }]) + end + + it "passes the form keys through in monitor mode — only the router's own keys are dropped" do + klass = permittable_class { permit_params(:create, mode: :monitor) { required :n, :integer } } + passed = controller(klass, params: { n: "x", _method: "patch", controller: "users" }).permitted_params + expect(passed.to_h).to eq("n" => "x", "_method" => "patch") + end + it "flags undeclared keys inside root and nested hashes (routing keys are only top-level)" do e = violations_for({ user: { name: "a", controller: "smuggled" } }) do permit_params(:create, root: :user, unknown: :error) { required :name, :string }