ForbidCredentialCastBypassRule — cast resolution is PHP dispatch, not an ancestry merge (#217, supersedes #68) by Goosterhof · Pull Request #69 · script-development/phpstan-warroom-rules

Goosterhof · 2026-09-01T09:21:03Z

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

This supersedes #68, which is closed. Same remit, different core. #68's cast resolution merged every declaration in the ancestry and let the leaf win; that reading is wrong, and the fourth crit round is what exposed it. Rather than patch a fourth time, the resolution half was rebuilt around PHP's actual semantics and the fixture suite was rebuilt to measure against PHP rather than against anyone's reading of Laravel. The write-verb detection, receiver type gate, payload typing and table-map resolution carried over unchanged — they were sound.

What was wrong, measured

Laravel builds the effective cast map exactly once, in HasAttributes::initializeHasAttributes():

$this->casts = array_merge($this->casts, $this->casts());

Two halves, two different PHP resolution rules:

$casts is a property — exactly ONE declaration survives, the most derived, REPLACING an ancestor's default rather than merging with it (and a class default replacing a trait's).
casts() is a method read by a SINGLE virtual dispatch — only the nearest body runs. An ancestor's or a trait's body contributes NOTHING unless the body that runs calls parent::casts().

Merging every declaration therefore invents casts that do not exist at runtime. Against PHP's own answer over the eighteen shapes in CastDispatchShapes.php, the old reading is wrong on seven — six inventing a credential cast the model does not have, the seventh calling a readable declaration unreadable:

Shape	Runtime	Old reading
replacing `casts()` override, other column	no cast	flags — crit's blocking issue
`return parent::casts();`	cast present	flags + spurious `castMapIncomplete` — crit nitpick 4
`$casts` property redeclared in a child	no cast	flags — not in crit's list
`$casts` + `casts()` on one class	no cast (method wins)	depends on source order — crit nitpick 3
trait `casts()` + class `casts()`	no cast	flags — not in crit's list
3 levels, mid replaces grandparent, leaf composes	leaf only	flags the grandparent too — not in crit's list
`mergeCasts()` at construct	cast present	silent — undocumented false negative

Every one of those was masked in the previous fixtures by a key collision: the old OverridingVault fixture passed because it redeclared the same column, so key-overwrite hid the dispatch error. On a security rule a false positive spends the gate's authority faster than a missed catch, which is why this is a re-cut and not a documented limitation.

What the fix is

castResolutionFor() now walks a declaration chain (nearest ancestor first, class body before its imported traits) and takes the first surviving $casts property declaration, the first dispatched casts() body — continuing upward only through an explicit parent::casts() — then merges property-then-method. The ancestry merge, its precedence commentary and the source-order-dependent reader are gone; the resolution half is smaller than it was.

Three of crit's other findings rode along, all confirmed first-hand:

increment family added to the write verbs. Query\Builder::incrementEach() is literally update(array_merge($columns, $extra)), so a credential in $extra reached SQL uncast with no diagnostic.
Union receivers are read per branch. Builder<User>|Builder<AuditLog> was collapsed to its first member — wrong in both directions, and the same defect family as the merge: picking one where several apply.
insertOrIgnore / insertGetId gained fixtures. On the verb list with no site of their own, so a regression dropping either was invisible.

Why the fixture suite looks like this

CastDispatchShapes.php is a table of declaration shapes, deliberately in one file so they read side by side. Its test does not carry a hand-written expectation — a hand-written one would encode the same wrong reading of Laravel that caused the defect. For each shape it computes the expectation from PHP itself: the $casts default PHP resolved (getDefaultProperties(), which honours replacement), merged under a real virtual dispatch of casts(), in Laravel's own order.

It also asserts the two readings still disagree on at least six rows. That is a denominator, not decoration: without it the table could quietly stop measuring the difference and keep passing.

Verification

Gates as CI runs them (composer audit, format:check, phpstan at level max, test, coverage:check, mutation:ci), plus the --no-dev production-tree leg with its dev-stub assertion and the illuminate/* ^12 lowest-Laravel leg on scratch copies. 254 tests / 610 assertions; coverage 91.25% (gate 83); MSI 85.57% (gate 75) with 0 errors, 0 timeouts, 0 uncovered. extension.neon smoked through the real phpstan analyse entry point, not only RuleTestCase.

Teeth, each observed to fire rather than asserted:

Against a merge-everything implementation the shape test reds on seven spurious errors.
Mutation controls, run individually: dropping a write verb, collapsing the union receiver to its first branch, removing the merge reversal, capping the ancestry walk at one parent, and keying the parent-call detector on the method name alone — each reds exactly the assertion that pins it.
The first probe of this whole investigation reported zero errors including its own positive control (scratch classes absent from composer's classmap). Worth stating because without that control the conclusion would have been "crit is wrong, nothing reproduces."

Two traps found on the way, both recorded in the fixtures

ClassReflection::getTraits(true) walks the PARENT CHAIN, not only traits-of-traits. A model importing zero traits reports twelve — Laravel's HasAttributes among them, which declares BOTH $casts = [] and casts(). Harmless under a merge; under stop-at-first-hit it answers "this model casts nothing" for every subclass. Caught by an existing fixture, not by reasoning. Trait flattening is now hand-rolled with a diamond guard. (Filed WR-1162 for the sibling rule's docblock, which documents only the traits-of-traits half — its behaviour there is correct and deliberate.)
Pint's ordered_class_elements collapsed a fixture into a byte-identical twin of its neighbour — the pair pinning "source order does not matter" — with every gate green, because both orders yield the same correct answer under the new rule. That distinction is unrepresentable in a Pint-formatted tree, so it now rides a trait boundary where no formatter can reorder it, and the fixture says why. The canonical Pint config was not touched.

Accepted false negatives

Documented and pinned, and the out-of-scope list's header now states that nothing may be parked there to excuse a false positive — it previously claimed "never a false positive" while three of its entries were exactly that. Class-based casts (AsEncryptedArrayObject::class), dynamic payloads and keys, upsert()'s third argument, a hoisted DB::table() builder, static-magic builder entry without larastan, raw SQL, a composition mixing a readable contributor with a dynamic one, and casts added at runtime via mergeCasts() / withCasts().

The last is a Commander ruling on measured grounds rather than a judgement call: across the war-room fleet mergeCasts() appears in application code once, inside a copy-pasted newInstance() override propagating a map this rule already reads, and withCasts() once, on a non-credential column, query-time only (control: 306 files carry protected function casts). A diagnostic keyed on those calls has no true positive to find today and exactly one false positive to produce. A test pins the silence, so changing it is a decision rather than a drift.

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].

Reasoning trail: campaigns/phpstan-warroom-rules/2026-09-01-credential-cast-dispatch-resolution.md (war-room).

🤖 Generated with Claude Code

https://claude.ai/code/session_01TKX1HbRNWdgbZVs5Wo9f9n

…umns may only be written through the model path 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`, and the increment family). Those writes bypass Eloquent casts and ship the raw value to SQL — no hash, no error, green tests, plaintext credential at rest. Seed: lokalekeuze#65, where one Action wrote through the model by choice while the builder idiom sat one file away. Cast resolution mirrors PHP's own member resolution rather than merging every declaration in the ancestry. Laravel builds the effective map once, as `array_merge($this->casts, $this->casts())`, and the two halves resolve differently: `$casts` is a property, so exactly one declaration survives and a redeclaration replaces its parent's; `casts()` is a single virtual dispatch, so only the nearest body runs and an ancestor contributes only through an explicit `parent::casts()`. The method half therefore wins on a shared column whatever order the file declares them in. That distinction is the whole point of this implementation. A merge-everything reading is wrong on seven of the eighteen shapes in `CastDispatchShapes.php` — six of them inventing a credential cast the model does not have, the seventh calling a readable declaration unreadable — and every one of those is masked in ordinary fixtures by a key collision. On a security rule a false positive spends the gate's authority faster than a missed catch, so the shape test computes its expectation from PHP itself (the property default PHP resolved, merged under a real virtual dispatch of `casts()`) instead of from anyone's reading of Laravel, and asserts the two readings still disagree on enough rows to be measuring something. Three fail-open shapes each report under their own identifier, because MISSING, FAILED and MISCONFIGURED must not arrive as the same silent outcome: `modelSourceUnreadable`, `castMapIncomplete`, `configuredModelMissing`. All three fire regardless of the payload — with an incomplete map the rule cannot claim the payload is clean. Model resolution reads the builder/relation generic per UNION branch; `DB::table('…')` carries no model and resolves only through the opt-in `credentialCastTableModels` map, empty by default, because inferring a model from a table name is exactly the false-positive source this rule cannot afford. Accepted false negatives are documented and pinned, and nothing is parked there to excuse a false positive: class-based casts, dynamic payloads and keys, `upsert()`'s third argument, a hoisted `DB::table()` builder, static-magic builder entry without larastan, raw SQL, a composition mixing a readable contributor with a dynamic one, and casts added at runtime via `mergeCasts()` / `withCasts()`. The last on measured grounds: across the fleet `mergeCasts()` appears in application code once, inside a `newInstance()` override propagating a map this rule already reads, and `withCasts()` once on a non-credential column — a diagnostic keyed on those calls has no true positive to find and one false positive to produce. Gates as CI runs them: format, self-analysis at level max, 254 tests / 610 assertions, coverage 91.25% (gate 83), MSI 85.57% (gate 75, 0 errors, 0 timeouts, 0 uncovered), plus the `--no-dev` production-tree leg with its dev-stub assertion and the `illuminate/* ^12` lowest-Laravel leg on scratch copies. `extension.neon` smoked through the real `phpstan analyse` entry point. Teeth: against a merge-everything implementation the shape test reds on seven spurious errors. Mutation controls kill a dropped write verb, a union receiver collapsed to its first branch, a removed merge reversal, an ancestry walk capped at one parent, and a parent-call detector keyed on the method name alone. Two traps worth recording. `ClassReflection::getTraits(true)` walks the PARENT CHAIN, not just traits-of-traits — a model importing no traits reports twelve, including Laravel's `HasAttributes`, which declares both `$casts` and `casts()`; harmless under a merge, fatal under stop-at-first-hit, so trait flattening is hand-rolled with a diamond guard. And the canonical Pint config's `ordered_class_elements` reformatted a fixture pinning source-order irrelevance into a byte-identical twin of its neighbour with every gate green, so that shape now crosses a trait boundary where no formatter can reorder it. Versioning: candidate MAJOR — surfaces new errors wherever a consumer writes a credential column through a builder. `^0.8` caret means tagging auto-adopts nobody. CHANGELOG under [Unreleased]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKX1HbRNWdgbZVs5Wo9f9n

…atal on PHP 8.4 The shape table's classes are LOADED rather than parsed, which is what makes its expectations PHP's own answer instead of a reading of Laravel — and constrains what may live there to shapes composable on the package's MINIMUM PHP. A trait declaring a non-empty `$casts` default is not. `Model` declares `protected $casts = []` through `HasAttributes`, and PHP 8.4 requires an inherited and a trait-imported property to agree on their default ("the definition differs and is considered incompatible"); 8.5 accepts it. Both 8.4 CI legs died on `TraitPropertyInherited` while both 8.5 legs passed. Controlled in isolation against both interpreters: 8.4 fatal, 8.5 composes. The shape stays covered by the analysis-only fixtures (`HasEncryptedNotesProperty` on `TraitCastModel`), which PHPStan parses and never composes — the reason the incompatibility went unnoticed there for four review rounds. Both fixtures now name the constraint, so nobody "fixes" one by loading it or deletes the other as redundant. Measured on BOTH interpreters this time, which is the actual lesson: the earlier verification ran on 8.5 only while 8.4 sat installed at /usr/bin/php8.4. 254 tests / 600 assertions and PHPStan level max green on 8.4 and 8.5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKX1HbRNWdgbZVs5Wo9f9n

jasperboerhof

Crit review

2 issues · 2 nitpicks · head 6fadd44507

Crit requests changes — 2 issues.

Issues

Trait insteadof adaptations can make the rule select an excluded casts() method
src/Rules/ForbidCredentialCastBypassRule.php:858 — see inline

Conditional casts() returns are merged into a cast map no runtime call can produce
src/Rules/ForbidCredentialCastBypassRule.php:981 — see inline

2 nitpicks

Discarded parent::casts() calls make the rule inherit unused credential casts
src/Rules/ForbidCredentialCastBypassRule.php:979 — containsParentCastsCall() matches discarded parent::casts() calls. dispatchedMethodCasts() then merges the parent's cast map. The child can return no credential cast for that column at runtime. Consumer PHPStan rejects safe builder writes.

nitpick because code change required — the harm needs an edit that has not happened

Named extra arguments bypass increment payload inspection when amount is omitted
src/Rules/ForbidCredentialCastBypassRule.php:563 — payloadColumns() reads the increment payload only at positional index two. PHP places a named extra argument after the first positional argument. The rule inspects no credential key when amount is omitted. Consumer PHPStan allows raw credential writes.

nitpick because code change required — the harm needs an edit that has not happened

jasperboerhof

Crit review

3 issues · 0 nitpicks · head 69f503df74

Crit requests changes — 3 issues, 2 open threads.

Issues

Model increment-family extra payloads bypass the rule despite carrying raw credential columns.
src/Rules/ForbidCredentialCastBypassRule.php:430 — see inline

Unlisted updateFrom and insertOrIgnoreReturning writes bypass credential-cast enforcement.
src/Rules/ForbidCredentialCastBypassRule.php:277 — see inline

incrementOrCreate() skips credential payloads in its existing-record increment branch.
src/Rules/ForbidCredentialCastBypassRule.php:277 — see inline

Still open

src/Rules/ForbidCredentialCastBypassRule.php — already filed, still open
src/Rules/ForbidCredentialCastBypassRule.php — already filed, still open

…bs and receivers that bypass casts crit's fifth round, verified finding by finding against PHP's own resolution rather than taken at face value. Two false positives, four false negatives, and one finding right about its mechanism and wrong about its harm. FALSE POSITIVES, fixed at the root rather than patched. Which `casts()` body runs is now resolved by REFLECTION — `getNativeReflection()->getMethod('casts')` plus its file and start line — not by a first-match walk over the imported traits. That walk gets `use A, B { B::casts insteadof A; }` wrong whenever the excluded trait is listed first, and an `as` alias the same way; reflection resolves the adaptation and locates the body exactly, trait included. Measured: on the new fixture it points at the trait `insteadof` selected. The PROPERTY half still walks the declaration chain, and that IS PHP's answer there — adaptations are method-only, and two sources declaring `$casts` with different defaults is a fatal error rather than an ambiguity — so the two halves resolve by different means for a reason, and the docblock says which and why. A `parent::casts()` call now extends the walk only when its RESULT IS CAPTURED. A bare `parent::casts();` statement changes nothing at runtime, so inheriting the parent's map on the strength of the call merely appearing in the body invented casts the child does not have. Returned, composed, spread, or assigned to a variable first all still count — the fixture for the variable form exists so this fix cannot quietly become a fail-open on a credential column. FALSE NEGATIVES. Payload slots now carry a parameter NAME as well as a position, because a named argument does not sit at its parameter's index once an earlier optional one is skipped: `increment('votes', extra: [...])` puts the payload at index 1, not 2. The first attempt refused positional reading whenever ANY argument was named, which would have dropped `upsert($values, uniqueBy: [...])`; PHP requires positionals before named ones, so the correct test is whether the argument at that slot is itself positional. The slot names are claims about `illuminate/database`, so a test asserts every one against Laravel's real signature — a rename upstream would otherwise disable the named lookup in total silence. Teeth-proved against both a renamed slot and a shifted position. A MODEL receiver is now in scope for the increment family, and this is the finding that falsified the rule's own prose. `Model::increment()` is `protected`, but `Model::__call()` names all eight increment methods and forwards to them, and `Model::incrementOrDecrement()` casts the in-memory attribute through `forceFill($extra)` while handing the SAME `$extra`, uncast, to the query builder: the object ends up right and the row ends up plaintext. The docblock claimed the model path was safe "structurally, not by exemption". It now says safe per VERB, and names `MODEL_BYPASSING_METHODS` as the whole exception. Three verbs added with verified signatures: `updateFrom` and `insertOrIgnoreReturning` (Postgres-only, on `Query\Builder`, forwarded by Eloquent's `__call`) and `incrementOrCreate`, whose `$attributes` go through `firstOrCreate()` — a model save, so casts fire and it is deliberately not read — while its `$extra` does not. RIGHT ABOUT MECHANISM, WRONG ABOUT HARM. crit called the union of conditional `casts()` returns "a cast map no runtime call can produce". True, and not a defect: a column some branch casts as a credential IS cast on that path, so the union is the bias a credential rule should have, and the probe showed the rule agreeing with PHP on the conditional shape. The real defect inside it was narrower — two branches disagreeing about the SAME column resolved by source order — so the CREDENTIAL cast now wins. Source order is not a fact about which branch runs. The dispatch walk is BOUNDED by ancestry depth, not merely guarded. Mutation testing made the case: turning the visited-guard `break` into `continue` spun forever, and an unbounded loop in an analyser hangs a consumer's pipeline with no error rather than reporting something wrong. Controlled — with the bound in place that same mutation terminates. Docblock audit, because two rounds have now falsified a guarantee it asserted. "A Model receiver is structurally excluded" was false for eight verbs. "All three must resolve or the rule stays SILENT" read as a no-false-positives promise and now says what it is: a disposition for what the rule does when it CANNOT resolve something, not a claim that what it resolves is right. The trait narrative still described the walk this commit deletes. Counts re-measured rather than carried forward: 23 shapes, and the merge-everything reading is wrong on nine of them. `insteadof` is NOT among those nine — it discriminates the intermediate first-match walk instead — so the table deliberately keeps shapes refuting BOTH wrong readings. A table that only refutes the reading you already abandoned measures nothing. PHPStan level max surfaced six real errors from the reworked constant, including an always-false comparison; all fixed. Gates on BOTH interpreters: level max clean, 255 tests / 700 assertions, coverage 91.00% (gate 83), MSI 86.01% (gate 75, 0 errors, 0 uncovered) on 8.4 and 8.5. Teeth against the previous implementation: 7 missing errors and 2 spurious ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKX1HbRNWdgbZVs5Wo9f9n

jasperboerhof

Crit review

0 issues · 4 nitpicks · head 3de488cfcc

Crit approves — nothing blocking at this head.

4 nitpicks

Reverse parent composition can hide an inherited credential cast
src/Rules/ForbidCredentialCastBypassRule.php:1061 — dispatchedMethodCasts() merges the parent map before the child map.
A child can return its literal map before parent::casts().
PHP lets the parent value replace the child value.
The rule misses builder writes that bypass the effective credential cast.

nitpick because code change required — the harm needs an edit that has not happened

Mixed model and builder unions bypass credential-write detection
src/Rules/ForbidCredentialCastBypassRule.php:511 — resolveModels() evaluates a mixed union as one receiver type.
The builder supertype gate returns maybe for that union.
The method returns no target model.
The rule misses raw credential writes through the builder branch.

nitpick because code change required — the harm needs an edit that has not happened

Unsupported hashed modifiers are treated as credential casts
src/Rules/ForbidCredentialCastBypassRule.php:1223 — isCredentialCast() accepts every hashed: suffix.
Laravel hashes only a cast whose type is exactly hashed.
An unsupported suffix leaves model assignments untransformed.
The rule blocks equivalent builder writes for consumer models.

nitpick because code change required — the harm needs an edit that has not happened

Captured parent maps survive child mutations to credential entries
src/Rules/ForbidCredentialCastBypassRule.php:1193 — capturesParentCastsCall() follows a parent call without tracing the returned map.
dispatchedMethodCasts() merges the entire parent map into the child map.
A child can replace or remove the inherited credential entry before returning.
The rule blocks builder writes to an uncast consumer column.

nitpick because code change required — the harm needs an edit that has not happened

…both supported Laravel majors The signature guard added in the previous commit failed CI on `check-lowest-laravel`, and it failed for the right reason: three of the verbs the rule reads postdate this package's MINIMUM Laravel. The package supports `illuminate/* ^12 || ^13`, and I measured the verbs on 13 only. Enumerated against both majors rather than probed and generalised — all 17 verbs resolve on `illuminate/database` 13.20, and exactly `insertOrIgnoreReturning`, `incrementEachQuietly` and `decrementEachQuietly` are absent on 12.68. My first pass declared only the first of those, because I checked `incrementQuietly` (which IS present on 12) and generalised from it to the whole quiet family. Enumerating the corpus and classifying it beats testing a hand-written candidate list. The gate tightens rather than loosens. A verb absent from every receiver class must be declared version-gated, or the test fails — so an upstream RENAME still reds. And on the newest supported Laravel nothing may be skipped at all, read from `InstalledVersions` rather than by probing for one of the methods under test, which would make the guard argue with itself. Teeth-proved three ways: un-declaring a genuinely gated verb reds the Laravel-12 leg and correctly stays green on 13; a gated verb absent on 13 too reds there. The slot count is now RECONCILED instead of a floor picked by hand. The previous `assertGreaterThanOrEqual(20)` was calibrated on whichever Laravel I happened to run and was silently wrong on the other — which is how it failed. Every slot of every present verb must be checked, so the assertion holds on both majors and still catches a map that stopped being read. Verified on all three legs this time: Laravel 13 on PHP 8.4 and 8.5 (255 tests / 686 assertions, level max clean, coverage 91.01%), and the Laravel 12 leg on both interpreters (255 tests / 678 assertions, level max clean). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKX1HbRNWdgbZVs5Wo9f9n

jasperboerhof

Crit review

0 issues · 3 nitpicks · head 1de5289ad6

Crit approves — nothing blocking at this head.

3 nitpicks

Raw table resolution ignores a later from() target override
src/Rules/ForbidCredentialCastBypassRule.php:600 — modelFromChainTable() only recognizes table() calls during its backward walk. Query\Builder::from() replaces the builder's update target. The rule can select User for an accounts update. Consumers receive false positives or miss users credential writes.

nitpick because code change required — the harm needs an edit that has not happened

Heterogeneous supported receiver unions bypass credential-write checking
src/Rules/ForbidCredentialCastBypassRule.php:528 — resolveModels() checks each supported receiver type against the complete union. No supported type covers both an Eloquent builder and a Relation branch. The method returns no models before it examines individual branches. Consumers can write plaintext through an unchecked credential path.

nitpick because code change required — the harm needs an edit that has not happened

Unused parent::casts() assignments incorrectly inherit credential casts
src/Rules/ForbidCredentialCastBypassRule.php:1193 — capturesParentCastsCall() accepts parent::casts() inside an assignment. dispatchedMethodCasts() then merges the parent cast map. Laravel uses only the overriding casts() return value. Consumers receive false credential-bypass errors.

nitpick because code change required — the harm needs an edit that has not happened

Goosterhof · 2026-09-01T10:29:47Z

Thanks — approving at 1de5289 with three nitpicks is the right call, and all three are captured as tracked tickets rather than left in a comment that evaporates on merge. I verified each first-hand before filing, with a positive control in the same probe, because a reviewer's confidence is not evidence:

Nitpick	Verdict	Ticket
`from()` overrides the `table()` target	confirmed false positive — `DB::table('users')->from('accounts')->update(['password' => …])` reports the `users` model against an `accounts` write	WR-1164
heterogeneous builder/relation union silently skipped	confirmed false negative — `Builder<Usr>\|HasMany<Usr, Usr>` reports nothing; the whole-union `isSuperTypeOf()` gates all answer `no`, so execution returns before the per-branch reading added in this PR ever runs	WR-1165
unused `parent::casts()` assignment inherits casts	confirmed false positive — PHP truth is `{"nickname":"string"}`, rule reports `password`	WR-1166

Two notes worth having in the thread rather than only in the tickets.

WR-1165 is ranked highest of the three despite being the rarest. It is the only fail-OPEN one: a missed credential write costs an incident, while a false positive costs the gate's authority. Your severity ordering put it alongside the other two; I am treating it as the most valuable of the set.

WR-1166 is the residue of a fix in this very PR, and the trade is deliberate. Moving the parent-call detector from "appears anywhere" to "result is captured" killed the discarded-statement false positive you filed earlier this round. Assignment counts as capture on purpose, because $c = parent::casts(); return array_merge($c, […]); must keep inheriting — treating assignment as non-capture would swap this false positive for a false negative on a credential column. The proper fix is the shallow dataflow step (is the assigned variable referenced in a return?), which is what the ticket specifies rather than the quick inversion.

Nothing here is being parked as out-of-scope: the rule's docblock now states that the out-of-scope list may only hold accepted false NEGATIVES and that nothing may be filed there to excuse a false positive, so both FP tickets stay open work.

dmooibroek

PR adds ForbidCredentialCastBypassRule (PHP-dispatch cast resolution replacing ancestry-merge logic); review flags coverage gaps — associative upsert() updates and static builder writes escape the rule, completeness diagnostics over-fire, extension.neon wiring is untested, and the cast-resolution invariant/magic count is duplicated four times in comments.

Goosterhof requested a review from a team as a code owner September 1, 2026 09:21

Goosterhof added the Agent Review Requested Requesting review of specialized AI review agents. label Sep 1, 2026

Goosterhof mentioned this pull request Sep 1, 2026

ForbidCredentialCastBypassRule — hashed/encrypted cast columns may only be written through the model path (#217) #68

Closed

jasperboerhof requested changes Sep 1, 2026

View reviewed changes

Comment thread src/Rules/ForbidCredentialCastBypassRule.php Outdated

Comment thread src/Rules/ForbidCredentialCastBypassRule.php Outdated

jasperboerhof requested changes Sep 1, 2026

View reviewed changes

Comment thread src/Rules/ForbidCredentialCastBypassRule.php

Comment thread src/Rules/ForbidCredentialCastBypassRule.php

Comment thread src/Rules/ForbidCredentialCastBypassRule.php

jasperboerhof previously approved these changes Sep 1, 2026

View reviewed changes

Goosterhof dismissed jasperboerhof’s stale review via 1de5289 September 1, 2026 10:16

jasperboerhof approved these changes Sep 1, 2026

View reviewed changes

dmooibroek approved these changes Sep 1, 2026

View reviewed changes

Goosterhof merged commit fa68466 into main Sep 1, 2026
7 checks passed

Goosterhof deleted the feature/queue217-credential-cast-dispatch-resolution branch September 1, 2026 11:08

Navigation Menu

Uh oh!

ForbidCredentialCastBypassRule — cast resolution is PHP dispatch, not an ancestry merge (#217, supersedes #68) - #69

ForbidCredentialCastBypassRule — cast resolution is PHP dispatch, not an ancestry merge (#217, supersedes #68)#69
Goosterhof merged 4 commits into
mainfrom
feature/queue217-credential-cast-dispatch-resolution

Goosterhof commented Sep 1, 2026

Uh oh!

jasperboerhof left a comment

Uh oh!

Uh oh!

Uh oh!

jasperboerhof left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasperboerhof left a comment

Uh oh!

jasperboerhof left a comment

Uh oh!

Goosterhof commented Sep 1, 2026

Uh oh!

dmooibroek left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Goosterhof commented Sep 1, 2026

What was wrong, measured

What the fix is

Why the fixture suite looks like this

Verification

Two traps found on the way, both recorded in the fixtures

Accepted false negatives

Versioning

Uh oh!

jasperboerhof left a comment

Choose a reason for hiding this comment

Crit review

Issues

Uh oh!

Uh oh!

Uh oh!

jasperboerhof left a comment

Choose a reason for hiding this comment

Crit review

Issues

Still open

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasperboerhof left a comment

Choose a reason for hiding this comment

Crit review

Uh oh!

jasperboerhof left a comment

Choose a reason for hiding this comment

Crit review

Uh oh!

Goosterhof commented Sep 1, 2026

Uh oh!

dmooibroek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmooibroek left a comment •

edited

Loading