Skip to content

Fix three silent corruptions in the absence path - #39

Open
VSN2015 wants to merge 1 commit into
masterfrom
fix/absence-and-default-integrity
Open

Fix three silent corruptions in the absence path#39
VSN2015 wants to merge 1 commit into
masterfrom
fix/absence-and-default-integrity

Conversation

@VSN2015

@VSN2015 VSN2015 commented Sep 12, 2026

Copy link
Copy Markdown
Owner

A second-pass audit of the absence/default/unknown-key path — the places the 21 open PRs don't reach. All three findings are the same shape: the contract accepted something it documents as impossible. Each was reproduced before it was fixed, and each spec is red on master.

1. A mutable default: was shared by every request

Field declarations are frozen data, but the value an author wrote was not — and HashWithIndifferentAccess hands a non-frozen Array (and any String) to the result by reference.

c = Permittable::Contract.define { array :tags, of: :string, default: ["a"] }
c.call!({})[:tags] << "leak"
c.call!({})[:tags]          # => ["a", "leak"]   ← a LATER, unrelated request

array :tags, default: [] is the common spelling, 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.

Authored default:/example: values are now deep-copied and frozen at class load. The copy is the point: the object the host app passed in is never frozen behind its back, in case it is still using it. A String default is copied again on delivery, so every value in the result stays the app's own to mutate.

2. 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:

c = Permittable::Contract.define { required :name, :string, normalize: :squish }
c.call({ "name" => "" })     # [{param: "name", code: "missing"}]     ✓
c.call({ "name" => "   " })  # params: {"name" => ""}, valid? true    ✗

An empty string went into the column — the silent corruption strict coercion exists to refuse, delivered by the gem's own preset.

normalize: is now its own stage, running before the absence rule, so a value that normalizes to empty takes the nullable: / default: / missing branch like any other empty value. There is still exactly one reading of absence, and a normalize: Proc is still called exactly once per value (spec'd, by counting). Relatedly, a default: is now stored in the form it was validated in: default: " free " with normalize: :squish was checked as "free" and handed to requests as " free ".

3. unknown: :error rejected ordinary form submissions

Only the router's controller/action/format were exempt from the top-level check, but Rails also merges authenticity_token, _method, utf8 and commit into a form POST:

[{param: "authenticity_token", code: "unknown"}, {param: "_method", code: "unknown"},
 {param: "utf8", code: "unknown"}, {param: "commit", code: "unknown"}]

The strictest setting was unusable outside a JSON API, and it failed on four of the framework's own keys rather than on anything the client got wrong.

Those four are now exempt from the check, at the top level only. Deliberately unchanged: 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 them 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. The two lists answer different questions, so FORM_KEYS sits beside ROUTING_KEYS rather than joining it.


Verification

  • 264 examples, 0 failures; rubocop clean; coverage 97.09%.
  • The five new/changed specs fail on master for the right reasons.
  • benchmark/overhead.rb unchanged at 1.64x faster than params.permit — normalization moved, it did not get duplicated.
  • Contracts that declare no default:, no normalize:, and no unknown: :error are unaffected.

Also found, not in this PR

Five more findings came out of the same audit and are reproduced but deliberately left out to keep this reviewable: sensitive: silently fails to redact non-String values (a proc filter can only mutate Strings — stack behind #33); exported OpenAPI documents omit the parameters entry for the path templates they emit, so member-route documents fail 3.1 validation (behind #29); ERROR_SCHEMA has drifted from the message: key the server actually sends and omits the depth code; a PATCH|PUT route is documented for one verb; and impossible declarations still pass class load (length: 0 on a required field, reversed ranges like in: 65..18, and a block array's default:, which skips element validation entirely). Two further members of this same family also remain: a default: is delivered uncast (default: "42" on an :integer yields the String "42") and untransformed where a sent value is transformed.

🤖 Generated with Claude Code

A second-pass audit of the absence/default/unknown-key path, in the
places the open PRs don't reach. All three are the same shape: the
contract accepted something it documents as impossible.

A mutable `default:` was shared by every request. Field declarations
are frozen data, but the value an author wrote was not, and
HashWithIndifferentAccess hands a non-frozen Array — and any String —
to the result by reference. So `array :tags, default: []` gave every
request the same Array, and one request appending to
`permitted_params[:tags]` corrupted the default for the life of the
process. Authored `default:`/`example:` values are now deep-copied and
frozen at class load; the copy is the point, so an object the host app
passed in is never frozen behind its back.

`normalize:` could manufacture an empty value that walked past
`required`. "" is documented as absent, but normalization ran after
absence had been decided, so `normalize: :squish` rejected "" as
missing and accepted "   " as "". It now runs first, as its own stage,
which keeps exactly one reading of absence and still calls a host's
proc once per value. A `default:` is also stored in the form it was
validated in.

`unknown: :error` rejected ordinary form submissions. Only the router's
controller/action/format were exempt, but Rails also merges
authenticity_token, `_method`, utf8 and commit into a form POST, so the
strictest setting failed on four of the framework's own keys. Those are
now exempt from the check at the top level only — still flagged inside a
root: or a nested hash, still flagged by a standalone Contract, and
still passed through by monitor mode, whose promise is an untouched
params hash.

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