ForbidCredentialCastBypassRule — hashed/encrypted cast columns may only be written through the model path (#217) by Goosterhof · Pull Request #68 · script-development/phpstan-warroom-rules

Goosterhof · 2026-08-30T14:32:31Z

New rule: ForbidCredentialCastBypassRule (war-room enforcement #217). A column declared with a hashed / encrypted / encrypted:* cast may not appear as a payload key in a query-builder or Relation write (update, insert, insertOrIgnore, insertGetId, upsert, updateOrInsert): those writes bypass Eloquent casts and ship the raw value to SQL — no hash, no error, green tests, plaintext credential at rest. Seed: a lokalekeuze Builder writing a password through the model by choice while the builder idiom sat one file away (Back-to-code/lokalekeuze#65).

Design decisions, deliberate:

The model path is silent structurally (a Model receiver never matches the type gate), and create/updateOrCreate/firstOrCreate/createOrFirst are excluded from the verb list because they save a model and the casts fire — they are the remediation the error message names.
DB::table('…') writes resolve only through a new opt-in credentialCastTableModels config map (empty default): PHPStan 2.x exposes no class-enumeration API, so "which model owns this table" is not computable, and inflecting it from the table name is a false-positive generator this rule cannot afford. Silent-until-opt-in, per the package's empty-default precedent.
Identifier: forbidCredentialCastBypass.castBypassedByBuilderWrite.

Verification. 11 violating fixture sites → 12 expected errors; 11 clean sites; 5 raw-table sites (2 firing under a configured map, all silent by default). Four negative controls with captured failure signatures — two of them added because the mutation run left mutants alive on exactly those lines. Gates as CI runs them: format, self-analysis level max, 248 tests / 372 assertions, coverage 90.07% (gate 83), MSI 85.68% (gate 75), plus the --no-dev production-tree and illuminate/* ^12 lowest-Laravel legs on scratch copies; extension.neon smoked through the real phpstan analyse entry point.

Known bounds, documented not assumed: on plain PHPStan the static-magic Model::where(...) form resolves to an error type and the rule declines — consumers on larastan get Builder<TModel> there too, so consumer coverage is wider than the package fixtures demonstrate; spot-check during adoption. Fixture FQCNs live in a rule-scoped namespace with a resolution guard (testEveryFixtureModelResolvesToThisRuleSOwnFixtureDirectory) — this is the package's first rule that resolves an FQCN to a file, and a corpus scan found nine pre-existing duplicate FQCNs that are harmless for every other rule; a package-hygiene follow-up is proposed in the war-room report rather than folded in here.

Versioning: candidate MAJOR — surfaces new errors wherever a consumer writes a credential column through a builder; consumer-impact sizing deliberately not performed here (^0.8 caret means tagging auto-adopts nobody). CHANGELOG under [Unreleased] says so.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VdioobCZRV7dVBrpJ2f5nC

…passed by builder writes Laravel attribute casts fire on the MODEL path only. `Builder::update()` delegates to `toBase()->update()`, so a query-builder write ships the payload straight to SQL: a `hashed` / `encrypted` column receives the raw value with no hash, no encryption, no exception, and a green test suite (a test that reads the column back gets exactly what it wrote). The failure is silent at every layer. Seed: lokalekeuze PR #65 — `ReissueVoucherAction` wrote through the model by CHOICE while `BlockVoucherAction`'s builder idiom sat one file away. The rule flags a `hashed` / `encrypted` / `encrypted:*` cast column appearing as a key in a builder write payload (`update`, `insert`, `insertOrIgnore`, `insertGetId`, `upsert`, `updateOrInsert`) on an Eloquent Builder, query Builder or Relation receiver. - The model path is silent STRUCTURALLY, not by exemption: a `Model` receiver never matches the type gate, and `create` / `updateOrCreate` / `firstOrCreate` / `createOrFirst` are absent from the verb list because they save a model and the casts do fire — they are the remediation. - Model resolution comes from the builder's / relation's generic argument, so the dominant idiom needs no configuration. `DB::table('…')` carries no model and resolves only through the new opt-in `credentialCastTableModels` map (default `[]`); a model is never inferred from a table name. - Cast maps are read from model SOURCE via the injected `@defaultAnalysisParser` — both the `casts()` method and a `$casts` property, merged across the ancestry with the child winning. Neither shape is reachable through reflection alone, and invoking `casts()` would mean instantiating a model in the analyser. - Payload keys come from the resolved constant array type rather than the AST, so a payload hoisted into a variable is caught and a dynamic one is silent. Teeth proved in both directions: dropping the model's `hashed` cast removed exactly the six `password` findings and left every other cast firing; injecting a credential column into a clean-fixture builder write turned that assertion red. Two further fixtures pin claims that were otherwise unchecked — the child-wins merge DIRECTION (an escaped `array_reverse` mutant found it) and the `encrypted:` prefix boundary against near-miss cast names. `testEveryFixtureModelResolvesToThisRuleSOwnFixtureDirectory` guards a new constraint this rule introduces: it is the first rule in the package to resolve an FQCN to a FILE and parse it, so a duplicated fixture FQCN makes it read the wrong model. The models were first written in `App\Models`, where `User` collided with three other rules' stubs, and the suite passed on one installed tree while failing on another purely because `composer update` reordered the classmap. Gates: format:check, phpstan (self), 248 tests, coverage 90.07% (threshold 83), mutation MSI 85.68% (threshold 75). Additionally verified on the two CI legs a local run does not cover — `--no-dev` production tree with its fixture-unreachable control, and `illuminate/* ^12` (resolved v12.68.0), both green. Not tagged; release is a General/Commander step.

jasperboerhof

Crit review

3 issues · 1 nitpick · head 3c978418a6

Crit requests changes — 3 issues.

Issues

declaredCasts() misses credential casts declared inside a trait, not in the class body
src/Rules/ForbidCredentialCastBypassRule.php:656 — see inline

Hoisting a DB::table() builder into a variable defeats table-chain model resolution
src/Rules/ForbidCredentialCastBypassRule.php:458 — see inline

Unparsable or unreadable model files make the rule silently skip all credential checks
src/Rules/ForbidCredentialCastBypassRule.php:664 — see inline

1 nitpick

Expected error order for the two-column fixture may not match the rule's emission order
tests/Rules/ForbidCredentialCastBypassRuleTest.php:1354 — The fixture at line 74 writes ['password' => 'p', 'api_token' => 't'], and keysOfConstantArray() walks getKeyTypes() in that declaration order, so the rule should emit password before api_token for that call site. The test asserts the opposite order, api_token then password, at lines 54-55. Whether this makes the test fail depends on how PHPStan\Testing\RuleTestCase compares same-line errors, and that source is not available in this vendored tree to confirm either way.

nitpick because pre-existing — this pull request did not write those lines; unconfirmed — proof gap: Need to run the test suite or inspect PHPStan\Testing\RuleTestCase's error-comparison logic (only phpstan.phar is vendored here, no plain-PHP source, and no test runner is available) to see whether same-line errors are compared in emission order or re-sorted before assertion.

…l-open on unreadable model source Addresses crit's three findings on PR #68. 1. Trait-declared casts were invisible. Laravel models compose cast maps from traits routinely, and resolution walked only the class ancestry — so a `hashed` cast declared in a trait silently exempted every model using it, on the one rule whose whole value is catching a silent plaintext write. The walk now covers the trait use-chain via `getTraits(true)`, which flattens traits-of-traits, in PHP's own member-resolution order: per ancestor, oldest first, traits then the class's own declarations. Class-declared beats trait-imported beats inherited. 2. A `DB::table()` builder hoisted into a variable defeats the chain walk. Fixed as far as it goes — `DB::connection('…')->table('…')` and arbitrary intermediate hops now resolve — but the hoisted form is not resolvable in principle: the variable's type is a bare `Query\Builder` carrying no table name, so there is nothing left to read. Now documented and pinned by a fixture instead of left as an unstated gap. 3. An unparsable or unlocatable model source made the rule fail OPEN. It returned the same empty cast set as a model that genuinely declares none, so every write to that model passed silently — MISSING arriving as FAILED, which our own doctrine forbids. `declaredCasts()` now returns null for "could not look", distinct from `[]` for "looked, found none", and the call site is reported under a separate identifier `forbidCredentialCastBypass.modelSourceUnreadable`. Reported regardless of the payload, deliberately: with an incomplete map the rule cannot claim the payload is clean. A consumer can suppress that identifier alone without disarming the real check. Teeth, each breaking exactly the logic it pins: - Removing the trait walk dropped exactly the two trait-declared findings and left every other cast firing. - Applying traits after the class's own declarations turned `TraitOverriddenCastModel` red, proving the precedence DIRECTION is checked rather than merely that traits are read. - Reverting `declaredCasts()` to return `[]` on a parse failure made the diagnostic vanish — the fail-open reproduced, then fixed. The unreadable-source branch is tested through an injected `ThrowingParser` that fails for one named file. A syntactically broken fixture would break the suite's classmap rather than the branch under test, and the same fixture is asserted silent under the real parser, so the test carries both directions. Not changed: the expected-error order at the top of the flagged-writes test. Measured — the assertion passes in payload-declaration order too, so the comparison normalizes and the ordering is not load-bearing. Gates: format:check, phpstan (level max), 250 tests / 387 assertions, coverage 90.47% (gate 83), mutation MSI 85.76% (gate 75) with all 7 new mutants killed (total 1264, escaped held at 180). Also re-run on the two legs a local pass misses: `--no-dev` production tree with its fixture-unreachable control, and `illuminate/* ^12` (v12.68.0), both green.

Goosterhof · 2026-08-30T15:30:46Z

crit round closed at d958a29 (follow-up, no amend). Issue 1 FIXED: cast resolution now walks the trait use-chain (getTraits(true), traits-of-traits flattened) in PHP's member-resolution order — precedence DIRECTION is mutation-pinned, not just presence (traits-after-class flips a fixture red). Issue 2 PARTIALLY CLOSED, remainder declined with analysis: a hoisted DB::table() builder is a bare non-generic Query\Builder whose type carries no table name — covering it needs intra-statement def-use tracking, a materially different rule shape; instead the resolvable chain forms were WIDENED (DB::connection()->table(), intermediate hops now fire) and the hoisted form is pinned as documented, tested silence. If you want def-use coverage it deserves its own decision, not a fold-in. Issue 3 FIXED per MISSING≠FAILED doctrine: unreadable model source now reports under a second, independently-suppressible identifier (forbidCredentialCastBypass.modelSourceUnreadable) instead of silently exempting the write; both directions in one fixture (silent under the real parser, loud under a ThrowingParser). Nitpick disposed by MEASUREMENT: both orderings pass (flipped and re-ran), so RuleTestCase normalizes — ordering is not load-bearing; fixture untouched per your guidance. All 7 new mutants killed (1257→1264 total, escaped held at 180); gates green as CI runs them.

jasperboerhof

Crit review

2 issues · 0 nitpicks · head d958a29d87

Crit requests changes — 2 issues.

Issues

declaredCasts() only reads literal cast arrays, missing array_merge or constant-composed casts
src/Rules/ForbidCredentialCastBypassRule.php:601 — see inline

A mistyped credentialCastTableModels FQCN is silently treated as an unmapped table, not an error
src/Rules/ForbidCredentialCastBypassRule.php:522 — see inline

Goosterhof

Two majors anchored inline, confirming crit's round-2 findings via independent code read.

Round 1's modelSourceUnreadable fix (settled) drew a hard line: a read/parse failure must surface as a diagnostic, never collapse into "no casts". Both open findings are the same failure shape recurring in code that fix didn't reach — composed cast declarations (declaredCasts) and a mistyped config FQCN (castResolutionFor) both fail silently instead.

Neither is documented in the class docblock's Out-of-scope list, unlike every other accepted false negative here (hoisted-variable DB::table(), static-magic builder entry, etc.) — that list is the right home if these are accepted rather than fixed.

Blocking pending a fix or an explicit Out-of-scope entry for each.

… shapes crit found crit round 2 on PR #68 — two live issues, both fail-open false negatives on the one rule whose whole value is catching a silent plaintext credential write. Both confirmed first-hand at HEAD with a positive control before touching anything. (1) `declaredCasts()` read only LITERAL cast arrays. `returnedArrays()` accepted a `return` only when its expression WAS an `Expr\Array_`, so `return array_merge(parent::casts(), ['password' => 'hashed']);` — Laravel's own documented way to extend a parent's cast map — contributed nothing. Measured before the fix on a leaf composing that way: the write naming the INHERITED `passphrase` fired and the write naming the leaf's own `composed_secret` was silent, so the same model was half-enforced with no diagnostic anywhere. Array literals are now collected from anywhere inside a returned expression, which covers `array_merge`, a ternary over two literals and the spread form. (2) A `casts()` return or `$casts` default carrying no array literal at all (`return self::CASTS;`, `protected $casts = self::CASTS;`) is perfectly readable, so `modelSourceUnreadable` never fired — yet the map is incomplete. Now its own identifier, `forbidCredentialCastBypass.castMapIncomplete`, reported regardless of payload, with its own remediation. An abstract `casts()` with no body is NOT flagged: no return statement, nothing uninterpretable. (3) A mistyped `credentialCastTableModels` FQCN was answered with the "table not mapped" result — `hasClass()` false returned the same empty cast set as an absent mapping, so a typo or a stale rename permanently and silently disarmed the rule for that table. Now `forbidCredentialCastBypass.configuredModelMissing`, reachable only from the config map (an FQCN off a resolved generic always exists). False-positive direction, because widening what gets collected is where a credential-flavoured rule spends its authority: literals are never collected from inside an already-collected array (a nested cast value must not become a second cast map) nor from inside a function-like — the closure guard is widened from `Expr\Closure` to `FunctionLike`, which also covers arrow functions. Both pinned by a clean assertion on a model that carries a real cast alongside a nested and a callback literal. Teeth-proved by mutation, five guards, each reverted individually: every one reds exactly the test that pins it. Reverting the composed-literal collection also reds the CLEAN assertion, because the two mechanisms interlock — a map that can no longer be read reports `castMapIncomplete` rather than passing. New accepted false negative, documented not assumed: a composition mixing a literal with a DYNAMIC contributor (`array_merge($this->dynamicCasts(), [...])`) reads the literal half and stays silent about the rest. Reporting there would flag every model that composes at all, including the ones read in full. Gates as CI runs them: format, self-analysis level max, 252 tests / 403 assertions, coverage 90.85% (gate 83), MSI 86.08% (gate 75), plus the `--no-dev` production-tree and `illuminate/* ^12` lowest-Laravel legs on scratch copies. `extension.neon` smoked through the real `phpstan analyse` entry point: all three new diagnostics surface with their identifiers on a synthetic consumer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TzmX4F7niaC3Y411TcgFkh

Goosterhof · 2026-08-31T13:09:15Z

Round 2 dispositioned — head `9cc3a11`

@jasperboerhof / crit: both issues confirmed and fixed, each with a positive control run before the fix so the finding was measured rather than inferred. Detail in the two inline replies; the short version:

Finding	Disposition	Identifier
`declaredCasts()` only reads literal cast arrays	fixed — literals now harvested from anywhere inside a returned expression, so `array_merge(parent::casts(), [...])`, spread and ternary composition all contribute	—
…the class-constant half of the same finding	fixed separately — readable source, uninterpretable declaration, so it needed its own outcome	`forbidCredentialCastBypass.castMapIncomplete`
mistyped `credentialCastTableModels` FQCN treated as unmapped	fixed	`forbidCredentialCastBypass.configuredModelMissing`

The first one was the sharp one. On a leaf composing with array_merge(parent::casts(), …), the write naming the inherited cast fired and the write naming the leaf's own cast was silent — the same model half-enforced, with no diagnostic anywhere. That is this rule's own failure mode occurring inside the rule, and it would have shipped as "green".

Round 1's nitpick is answered with a measurement, not a shrug. You flagged the two-column fixture's expected error order and named the proof gap honestly ("no test runner available"). Ran it both ways: password, api_token and api_token, password on the same line both pass, so PHPStan\Testing\RuleTestCase does not compare same-line errors positionally. Nothing to change, and the assertion order is not load-bearing — worth recording so the next reader does not re-derive it.

One residual, declined for this PR and documented rather than left unstated: a composition mixing a literal with a dynamic contributor (array_merge($this->dynamicCasts(), [...])) reads the literal half and stays silent about the rest. Flagging it would mean flagging every model that composes at all, including the ones read in full — the false-positive cost is larger than the coverage gain on a credential-flavoured rule. It is in the class docblock's Out-of-scope list and the README.

Where I pushed the fix beyond what you asked, and why. Widening what gets harvested is exactly where this rule spends its authority, so the harvest stops at two boundaries: never inside an already-collected array (a nested cast value must not become a second cast map), and never inside a function-like. Your finding is what surfaced that the closure guard was Expr\Closure only — arrow functions were not covered, so a literal inside static fn (): array => [...] would have been read as a cast pair once the harvest widened. Now FunctionLike. Both boundaries are pinned by a clean assertion on a model carrying a real cast alongside a nested and a callback literal, because the false-positive direction is the one that costs the gate its credibility.

Teeth — five guards, each reverted individually, each redding exactly the test that pins it. One interaction worth naming: reverting the literal collection also reds the clean assertion, because the two mechanisms interlock — a map that can no longer be read reports castMapIncomplete rather than passing silently. That is the intended shape (MISSING must not read as CLEAN), not a coupling accident.

Gates as CI runs them, not as the manifest declares them: format · self-analysis level max · 252 tests / 403 assertions · coverage 90.85% (gate 83) · MSI 86.08% (gate 75) · --no-dev production-tree phpstan and illuminate/* ^12 lowest-Laravel phpstan + tests, both on scratch copies. extension.neon smoked through the real phpstan analyse entry point on a synthetic consumer: all three new diagnostics surface with their identifiers, which RuleTestCase alone would not have proved.

Over to you for round 3.

— General 🎖️

Goosterhof

prsweep:9cc3a111dd692047000c96d61a6ab9154b7bb3e6

Round 3 — both round-2 majors (#4237 composed-cast blind spot, #4238 mistyped-FQCN silent no-op) are fixed in 9cc3a111dd69 with dedicated tests (testCastDeclarationsCarryingNoArrayLiteralAreReportedRatherThanReadAsCastless, testAMistypedConfiguredModelIsReportedRatherThanTreatedAsUnmapped) and new fixtures (ComposedCastModel, SpreadCastModel, ConstantCastModel, ConstantCastPropertyModel, NestedLiteralCastModel, UninterpretableCastWrites).

Read the diff independently, not the commit message's own account:

collectArrayLiterals stops descending the moment it captures an Array_ node, so a nested cast VALUE (e.g. ['b' => 'c'] inside a composed map) can't become a second cast pair — the concern the docblock raises is actually guarded.
The three failure modes now carry distinct identifiers (modelSourceUnreadable / castMapIncomplete / configuredModelMissing) with different remediations, matching the MISSING-vs-FAILED-vs-MISCONFIGURED distinction the rule already draws elsewhere in the package.
ComposedCastModel's fixture pins that a composed map contributes only its OWN pairs (line 93/98 fire, the nested/callback literals stay clean) — closes the exact double-count risk the mechanism could have introduced.
Uzer-mapped raw-table fixture (testAMistypedConfiguredModelIsReportedRatherThanTreatedAsUnmapped) confirms the mistyped-FQCN case now reports on all 4 configured sites rather than going silent.

No new findings from this pass. Own-PR — capped at COMMENT.

jasperboerhof

Crit review

1 issue · 5 nitpicks · head 9cc3a111dd

Crit requests changes — 1 issue.

Issues

Replacing casts overrides retain inherited credential casts and report violations
src/Rules/ForbidCredentialCastBypassRule.php:596 — see inline

5 nitpicks

Increment and decrement extra payloads bypass casts without rule coverage
src/Rules/ForbidCredentialCastBypassRule.php:243 — WRITE_METHODS excludes increment, decrement, incrementEach, and decrementEach. Laravel passes each method's extra array to Query\Builder::update() without assigning model attributes. Plaintext credential values in those extra arrays evade the rule. Consumers can store plaintext credentials without a PHPStan diagnostic.