From 3c978418a6fc0ec36d030350e18f467106243c18 Mon Sep 17 00:00:00 2001 From: Gerard Oosterhof Date: Sun, 30 Aug 2026 16:27:02 +0200 Subject: [PATCH 1/3] =?UTF-8?q?feat(queue-217):=20ForbidCredentialCastBypa?= =?UTF-8?q?ssRule=20=E2=80=94=20credential=20casts=20bypassed=20by=20build?= =?UTF-8?q?er=20writes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 16 + CLAUDE.md | 2 + README.md | 24 + extension.neon | 17 + src/Rules/ForbidCredentialCastBypassRule.php | 700 ++++++++++++++++++ .../AbstractCredentialHolder.php | 24 + .../Fixtures/CredentialCastBypass/ApiKey.php | 21 + .../Fixtures/CredentialCastBypass/Article.php | 25 + .../CredentialCastBypass/BuilderWrites.php | 76 ++ .../CredentialCastBypass/CleanWrites.php | 110 +++ .../NearMissCastModel.php | 28 + .../CredentialCastBypass/OverridingVault.php | 26 + .../CredentialCastBypass/RawTableWrites.php | 40 + tests/Fixtures/CredentialCastBypass/User.php | 38 + tests/Fixtures/CredentialCastBypass/Vault.php | 10 + .../ForbidCredentialCastBypassRuleTest.php | 162 ++++ 16 files changed, 1319 insertions(+) create mode 100644 src/Rules/ForbidCredentialCastBypassRule.php create mode 100644 tests/Fixtures/CredentialCastBypass/AbstractCredentialHolder.php create mode 100644 tests/Fixtures/CredentialCastBypass/ApiKey.php create mode 100644 tests/Fixtures/CredentialCastBypass/Article.php create mode 100644 tests/Fixtures/CredentialCastBypass/BuilderWrites.php create mode 100644 tests/Fixtures/CredentialCastBypass/CleanWrites.php create mode 100644 tests/Fixtures/CredentialCastBypass/NearMissCastModel.php create mode 100644 tests/Fixtures/CredentialCastBypass/OverridingVault.php create mode 100644 tests/Fixtures/CredentialCastBypass/RawTableWrites.php create mode 100644 tests/Fixtures/CredentialCastBypass/User.php create mode 100644 tests/Fixtures/CredentialCastBypass/Vault.php create mode 100644 tests/Rules/ForbidCredentialCastBypassRuleTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index dc12edd..20a1146 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,22 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and ### Added +- `ForbidCredentialCastBypassRule` — new rule (war-room enforcement queue #217) forbidding a `hashed` / `encrypted` / `encrypted:*` cast column from appearing as a key in a **query-builder** write payload: `Model::query()->…->update([...])`, `->insert(...)`, `->insertOrIgnore(...)`, `->insertGetId(...)`, `->upsert(...)`, `->updateOrInsert(...)`, a relation-derived builder write, or a `DB::table('…')` write on a mapped table. Registered in `extension.neon` with one new parameter, `credentialCastTableModels` (default `[]`). Identifier: `forbidCredentialCastBypass.castBypassedByBuilderWrite`. Doctrine: war-room §Architectural Principles #1 (Explicit over implicit) + #10; ISO 27001 A.5.33 and AVG on the compliance territories. Seed: lokalekeuze PR #65. + + **The bug class.** Attribute casts fire on the MODEL path only — `setAttribute()` runs the cast when you assign `$model->password = $plain` and `save()`. `Illuminate\Database\Eloquent\Builder::update()` delegates to `toBase()->update()`, which ships the array straight to SQL: the credential lands in the column verbatim, 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 and is normally discovered by reading the database. In the seed, `ReissueVoucherAction` wrote through the model by CHOICE while `BlockVoucherAction`'s builder idiom sat one file away — nothing but author preference separated the safe site from the unsafe one. + + **Model path stays silent STRUCTURALLY, not by exemption.** The receiver type must be an Eloquent `Builder`, a query `Builder`, or a `Relation`; a `Model` receiver never matches, so `$model->update([...])`, `$model->password = …; $model->save()` and `Model::create([...])` are silent because of what they are, not because they are listed. `create` / `updateOrCreate` / `firstOrCreate` / `createOrFirst` are deliberately absent from the write-verb list even on a builder receiver — they instantiate and `save()` a model, so casts fire and flagging them would criminalize the remediation. + + **Model resolution — generic first, config only for raw tables.** An Eloquent `Builder` or `Relation` yields the model from its generic argument (measured on plain PHPStan: `Model::query()->where(…)` resolves to `Builder`), so the dominant idiom needs no configuration. `DB::table('users')` resolves to a bare `Illuminate\Database\Query\Builder` carrying **no** model, so it is resolved only by walking the chain back to the `table('…')` string literal and looking it up in `credentialCastTableModels`. That map is **empty by default**, so raw-table writes are silent until a consumer opts in — inferring a model from a table name by inflection is exactly the false-positive source a credential-flavoured rule cannot afford, and queue #217 names the per-territory Level-1 arch test as the interim for what this rule cannot see. + + **Cast maps are read from model SOURCE.** Neither declaration shape is reachable through reflection alone: `protected function casts(): array` needs a method body, and invoking it would mean instantiating an Eloquent model inside the analyser. The rule injects PHPStan's own `@defaultAnalysisParser` (cached — a model file is parsed once per run), locates the class by resolved `namespacedName`, and reads `'column' => 'cast'` string pairs from **both** the `casts()` method's `return [...]` statements and a `$casts` property default, merged across the ancestry with the child winning (a cast declared on an abstract base still counts at the leaf). Payload keys come from the resolved **constant array type** rather than the AST literal, so `$p = ['password' => …]; $q->update($p);` is caught and a dynamic payload is silent. + + **Deliberate misses, each a false NEGATIVE and never a false positive:** class-based casts (`AsEncryptedArrayObject::class` and friends appear as `::class` constant fetches, not the string values matched here); dynamic payloads and computed keys; `upsert()`'s third argument (an update-COLUMN list whose names are values, and every column named there already appears in the row payload that is read); `Model::where(…)->update([...])` static-magic entry on plain PHPStan, where `__callStatic` is untypeable and the receiver resolves to an error type (consumers running larastan get `Builder` there and the rule fires normally — `Model::query()->…` resolves either way); and raw SQL, which has no payload array. + + **Teeth proved in both directions.** Flipping `'password' => 'hashed'` to `'string'` on the model fixture dropped exactly the six `password` findings and left every other cast firing; injecting `'password'` into a clean-fixture builder write turned the green assertion red at that line. A denominator test asserts each fixture still carries its write sites, so the two zero-expectation assertions cannot pass on an empty or unparsed file. The `extension.neon` wiring was additionally smoked through the real `phpstan analyse` entry point, not only `RuleTestCase`. + + **Versioning: candidate MAJOR** — it surfaces new errors in code that previously passed wherever a consumer writes a credential column through a builder. Per the pre-1.0 caret convention `^0.8` excludes the next minor, so tagging auto-adopts nobody; each consumer adopts on its own pin-bump PR. **NOT tagged** (release is a General/Commander step). + - `ForbidUntimedHttpClientRule` — new rule enforcing war-room **Architectural Principle #8** (explicit timeouts on outbound HTTP) at analysis time, the AST-aware successor to the per-territory `ExternalHttpTimeoutTest` named-list Pest tests (kendo / emmie). The named-list tests detect wrong-shape on *enrolled* classes but are blind to **omission** — a new untimed call nobody adds to the list; this rule closes the omission gap for the tractable call shapes. Registered in `extension.neon` (no parameters). Identifier: `forbidUntimedHttpClient.missingTimeout`. Doctrine: war-room §Architectural Principles #8. Seed: war-room enforcement queue #58 (spike branch `spike/wr-queue58-untimed-http-client`). **Review follow-up (bus #57 findings):** the `withOptions()` check is TYPE-aware, not AST-literal — a constant array type provably lacking `'timeout'` still fires (including through a variable holding a literal array, a widening over the inline-`Array_`-only first cut), while a computed/helper-built options expression (not a constant array type) is POSSIBLY timed and the chain DECLINES (the Major: flagging it was a false positive). A chain member outside the known `PendingRequest` builder surface — a Macroable extension (`Http::github()`, root or intermediate) or `when()`/`unless()` with their opaque closures — likewise declines (the Minor: a macro may return a pre-timed request); a genuine builder missing from the list costs only a false negative. **Detection (type-anchored, two entry points):** fires on a terminal send verb (`get`/`post`/`put`/`patch`/`delete`/`head`/`send`) reached without an explicit request timeout, where the entry point is either (1) the `Http` facade (`Illuminate\Support\Facades\Http` static-call root), or (2) an **injected `Illuminate\Http\Client\Factory`** receiver (`$this->http->…->get()`, anchored by TYPE so the property alias is irrelevant — the dominant fleet idiom, established by field survey of kendo/emmie/ublgenie/BIO). A timeout counts when the visible chain contains `->timeout(...)` **or** `->withOptions([... 'timeout' => ...])`; `connectTimeout()` alone does NOT (it bounds the handshake, not the response). diff --git a/CLAUDE.md b/CLAUDE.md index bf61980..f399295 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -38,6 +38,7 @@ Composer package distributing war-room-doctrine PHPStan rules across `script-dev | `ForbidUntimedHttpClientRule` | War-room §Explicit HTTP timeouts (#8) | `forbidUntimedHttpClient.missingTimeout` (type-aware; flags an `Http` facade OR injected `Illuminate\Http\Client\Factory` chain that reaches a send verb with no explicit `->timeout()` / `withOptions(['timeout'])`. Conservative — fires only on fully-visible single-expression chains; declines split/helper-built chains + Guzzle/SDK surfaces to hold FP at zero. COMPLEMENTS, does not replace, the per-territory `ExternalHttpTimeoutTest`. on `main`, `[Unreleased]`) | | `EnforceAuditModelProtectionsRule` | ADR-0001 §Append-only | `enforceAuditModelProtections.hasFactoryForbidden` / `.softDeletesForbidden` / `.updatedAtNotDisabled` (denylist-inversion; discovers audit models by shape — `auditModelNameSuffixes` default `AuditLog` OR `auditModelNamespacePrefixes` default `App\Models\Audit` — and flags `HasFactory` / `SoftDeletes` / missing `const UPDATED_AT = null`. shipped v0.7.0) | | `EnforceActionResultDtoRule` | ADR-0020 + ADR-0011 | `enforceActionResultDto.arrayReturnFromExecute` (signature-only; flags an `array` / `?array` / `array\|Dto` union / `iterable` native return type on `App\Actions\*` `execute()`. Phpdoc-only `@return array{...}` is a deliberate miss; no `list` carve-out. Seed kendo PR #1653. shipped v0.8.0) | +| `ForbidCredentialCastBypassRule` | War-room §Explicit over implicit (#1) + §Rotation-invariant credential handling (#10) | `forbidCredentialCastBypass.castBypassedByBuilderWrite` (flags a `hashed` / `encrypted` / `encrypted:*` cast column appearing as a key in a QUERY-BUILDER write payload — `update`/`insert`/`insertOrIgnore`/`insertGetId`/`upsert`/`updateOrInsert` on an Eloquent `Builder`, query `Builder` or `Relation`. Casts fire on the model path only, so a builder write stores the credential in plaintext with a green suite. The model path and the model-routing builder verbs (`create`/`updateOrCreate`/`firstOrCreate`/`createOrFirst`) are silent STRUCTURALLY — the receiver type gate, not an exemption list. Model comes from the builder/relation generic; `DB::table('…')` carries none and resolves only via the opt-in `credentialCastTableModels` map, default `[]`. Cast maps read from model SOURCE via the injected `@defaultAnalysisParser` — both `casts()` and `$casts`, merged across the ancestry. Payload keys read from the CONSTANT ARRAY TYPE, so a hoisted variable is caught. Seed lokalekeuze PR #65. on `main`, `[Unreleased]`) | | `ConnectionTransactionReturnTypeExtension` | (type extension, no rule) | — (resolves `ConnectionInterface::transaction()` to the closure's return type. **Load-bearing on `illuminate/* ^12` only** — it annotates `@return mixed`; Laravel 13 annotates `@template TReturn`/`@return TReturn` and infers the same type unaided. Its teeth are measured by the `check-lowest-laravel` CI job, not by the unit suite on a Laravel-13 tree. WR-0855, WR-0860) | Phase 2 expands the rule set: `EnforceAuditSnapshotOnRetryRule` (ADR-0001 §Snapshot-on-Retry Safety) was the first Phase 2 addition, promoted from cross-territory Pest arch tests (emmie PR #187, entreezuil PR #139, ublgenie PR #166, kendo PR #1029). `EnforceResourceDataValidatorOptInRule` (ADR-0009 §EAGER_LOAD validator opt-in) is the second Phase 2 addition, promoted from kendo PR #1084 under war-room enforcement queue #55. `EnforceFormRequestToDtoRule` (ADR-0012) is the third Phase 2 addition, promoted from entreezuil's `tests/Arch/FormRequestsTest.php` under the same queue #55 (instance 2). `EnforceExplicitHydrationRule` (ADR-0019) is the next Phase 2 candidate. @@ -135,6 +136,7 @@ SemVer per ADR-0021: ### War-room Architectural Principle rules (no published ADR) - **Explicit over implicit** — package distributes `ForbidAbortHelperRule` (bans `abort()` / `abort_if()` / `abort_unless()`; shipped), `EnforceCurrentUserAttributeRule` (flags `Request::user()` / `Auth::user()` / `auth()->user()` in `App\Http\Controllers`, steering to the `#[CurrentUser]` container attribute per Architectural Principle #9; shipped v0.4.0), `ForbidHttpExceptionInActionsRule` (type-aware sibling of `ForbidAbortHelperRule` — bans throwing the `Symfony\…\HttpException` family from `App\Actions\*`; HTTP status concerns belong to the HTTP layer per Principles #1 + #3; `ValidationException` deliberately out of scope; shipped v0.5.0), `ForbidResourceWrappedInJsonResponseRule` (bans wrapping a `JsonResource` in `response()->json()` / `new JsonResponse()` inside controllers per Principle #1 + ADR-0009; shipped v0.5.0), and `ForbidRawExceptionMessageInResponseRule` (bans a raw `Throwable::getMessage()` — or the `Throwable` itself — reaching a client-facing response sink per Principle #1 + information-disclosure hardening for the ISO 27001 / AVG / NEN 7510 consumers; default sink `Laravel\Mcp\Response::error`, configurable via `rawExceptionMessageSinks`; server-side logging never flags; `// @leak-safe:` exemption; shipped v0.8.0). These enforce war-room §Architectural Principles (some also touching numbered ADRs) — each rule's docblock "Doctrine source" line names its authority. +- **Rotation-invariant credential handling (#10) + Explicit over implicit (#1)** — package distributes `ForbidCredentialCastBypassRule` (flags a `hashed` / `encrypted` / `encrypted:*` cast column named as a key in a query-builder write payload, where the cast never fires and the raw credential reaches SQL; the model path is structurally silent because a `Model` receiver never matches the builder/relation type gate. `DB::table()` resolution is opt-in via `credentialCastTableModels` — a model is never inferred from a table name. ISO 27001 A.5.33 / AVG relevance on the compliance consumers. Seed lokalekeuze PR #65, war-room enforcement queue #217; on `main`, `[Unreleased]`). - **Explicit HTTP timeouts (#8)** — package distributes `ForbidUntimedHttpClientRule` (flags an `Http` facade / injected `Illuminate\Http\Client\Factory` chain reaching a send verb without an explicit request timeout, per Architectural Principle #8; the AST-aware, omission-closing successor to the per-territory `ExternalHttpTimeoutTest` named-list Pest tests — conservative single-expression detection, declines split/helper-built chains + Guzzle/SDK surfaces; COMPLEMENTS the named-lists rather than replacing them; on `main`, `[Unreleased]`). Seed: war-room enforcement queue #58. ### War-room internal ADRs diff --git a/README.md b/README.md index ea09b80..ebadff9 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,7 @@ includes: | `EnforceCurrentUserAttributeRule` | `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser` | `Request::user()` / `Auth::user()` / `auth()->user()` calls inside `App\Http\Controllers\*` classes (namespace prefix, incl. sub-namespaces; configurable via `controllerNamespacePrefixes`) | Use `#[\Illuminate\Container\Attributes\CurrentUser] User $user` on the method parameter. Scope is decided by namespace, not class ancestry — a base-less `final` controller in `App\Http\Controllers` fires; FormRequests (`App\Http\Requests`), middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs, and console commands are silent because their namespaces do not start with the controller prefix (container-attribute injection does not apply to FormRequest methods regardless). | | `EnforceCurrentUserAttributeRule` | `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser` | `Request::user()` / `Auth::user()` / `auth()->user()` calls inside `App\Http\Controllers\*` classes (namespace prefix, incl. sub-namespaces) | Use `#[\Illuminate\Container\Attributes\CurrentUser] User $user` on the method parameter. Scope is decided by namespace, not class ancestry — a base-less `final` controller in `App\Http\Controllers` fires; FormRequests (`App\Http\Requests`), middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs, and console commands are silent because their namespaces do not start with the controller prefix (container-attribute injection does not apply to FormRequest methods regardless). | | `EnforceAuditModelProtectionsRule` | `enforceAuditModelProtections.hasFactoryForbidden` / `.softDeletesForbidden` / `.updatedAtNotDisabled` | Eloquent models recognised as audit records by SHAPE — short name ends with a configured suffix (default `AuditLog`) OR FQCN sits under a configured namespace (default `App\Models\Audit`) | Three append-only protections, each firing independently: using `HasFactory` (a factory is a direct-insert path bypassing the hash-chained writer), using `SoftDeletes` (audit rows are never removed), or not disabling `updated_at` (an audit row is written once and never mutated — declare `public const UPDATED_AT = null;`) is an error. Discovery is by pattern, never a hand-maintained class list — a denylist inversion, so a newly-added audit model cannot escape the protections by omission. Abstract intermediates are exempt (their concrete leaves carry inherited violations). Non-model classes named `*AuditLog` are excluded by the Eloquent `Model` type gate. Doctrine: ADR-0001 §Append-only. | +| `ForbidCredentialCastBypassRule` | `forbidCredentialCastBypass.castBypassedByBuilderWrite` | Query-builder write calls (`update`, `insert`, `insertOrIgnore`, `insertGetId`, `upsert`, `updateOrInsert`) whose receiver is an `Illuminate\Database\Eloquent\Builder`, `Illuminate\Database\Query\Builder`, or `Illuminate\Database\Eloquent\Relations\Relation` | Naming a column that carries a `hashed`, `encrypted` or `encrypted:*` cast as a key in the write payload is an error. Casts fire on the MODEL path only; a builder write delegates to `toBase()->update()` and ships the raw value to SQL — no hash, no encryption, no exception, and a green test suite. The model path is **structurally** silent (a `Model` receiver never matches), and so are the builder methods that route through a model (`create`, `updateOrCreate`, `firstOrCreate`, `createOrFirst`) — they are the remediation. The model comes from the builder's/relation's generic type argument; a `DB::table('…')` chain carries none and resolves only through the opt-in `credentialCastTableModels` map (empty by default ⇒ silent, never inferred from the table name). The cast map is read from the model SOURCE — both the `casts()` method and a `$casts` property, merged across the ancestry. Payload keys are read from the resolved **constant array type**, so a payload hoisted into a variable is caught and a dynamic payload is silent. Deliberate misses: class-based casts (`AsEncryptedArrayObject::class`), dynamic keys, `upsert()`'s third argument, and `Model::where(...)` static-magic entry without larastan. Doctrine: war-room §Architectural Principles #1 + #10; ISO 27001 A.5.33 / AVG. Seed: lokalekeuze PR #65. | ### `EnforceActionTransactionsRule` — write-method list @@ -230,6 +231,29 @@ parameters: path: app/Models/Audit/SomeProjectionLog.php ``` +### `ForbidCredentialCastBypassRule` — resolving `DB::table()` writes + +A builder or relation write carries its model in its type — `Voucher::query()->…` +is a `Builder`, `$user->apiKeys()->…` is a `HasMany` — so the +rule finds the cast map on its own and needs no configuration. + +`DB::table('users')->update([...])` carries no model at all. The rule will not +guess one from the table name (an inflection guess is exactly the false-positive +source a credential-flavoured rule cannot afford), so raw-table writes are +**silent by default**. Opt a table in by mapping it: + +```neon +parameters: + credentialCastTableModels: + users: 'App\Models\User' + api_keys: 'App\Models\ApiKey' +``` + +Only tables whose model declares a `hashed` / `encrypted` / `encrypted:*` cast +are worth listing; a mapped table whose model has no credential cast changes +nothing. Single backslashes in the FQCN — NEON only unescapes `\\` inside +double quotes. + ### `ForbidRawExceptionMessageInResponseRule` — configurable sinks + `@leak-safe` exemption The rule flags a raw `Throwable::getMessage()` (or the `Throwable` itself) reaching a **client-facing response sink** — an information-disclosure leak. The built-in default sink `Laravel\Mcp\Response::error` is always armed; a consumer adds more (a persist-error setter, a `MarkInvoiceFailed` Action) via the `rawExceptionMessageSinks` parameter — a list of `FQCN::method` signatures, default `[]`: diff --git a/extension.neon b/extension.neon index 9f94230..dc3fa87 100644 --- a/extension.neon +++ b/extension.neon @@ -67,6 +67,16 @@ parameters: rawExceptionMessageSinks: [] safeMessageExceptionClasses: [] + # `ForbidCredentialCastBypassRule`: map of raw TABLE NAME to model FQCN, + # used ONLY to resolve a `DB::table('…')->update([...])` chain, which + # carries no model in its type the way `Model::query()` does. Empty by + # default, so `DB::table()` writes are silent until a consumer opts in — + # inferring a model from a table name would be a false-positive source this + # rule cannot afford. Builder and relation writes need no entry here; their + # model comes out of the generic type argument. Each FQCN uses single + # backslashes — see the NEON-quoting note above. + credentialCastTableModels: [] + parametersSchema: resourceDataBaseClass: string() formRequestBaseClass: string() @@ -76,6 +86,7 @@ parametersSchema: auditModelNameSuffixes: listOf(string()) rawExceptionMessageSinks: listOf(string()) safeMessageExceptionClasses: listOf(string()) + credentialCastTableModels: arrayOf(string()) services: - @@ -151,6 +162,12 @@ services: rawExceptionMessageSinks: %rawExceptionMessageSinks% safeMessageExceptionClasses: %safeMessageExceptionClasses% tags: [phpstan.rules.rule] + - + class: ScriptDevelopment\PhpstanWarroomRules\Rules\ForbidCredentialCastBypassRule + arguments: + parser: @defaultAnalysisParser + credentialCastTableModels: %credentialCastTableModels% + tags: [phpstan.rules.rule] - class: ScriptDevelopment\PhpstanWarroomRules\Type\ConnectionTransactionReturnTypeExtension tags: [phpstan.broker.dynamicMethodReturnTypeExtension] diff --git a/src/Rules/ForbidCredentialCastBypassRule.php b/src/Rules/ForbidCredentialCastBypassRule.php new file mode 100644 index 0000000..d1ca90d --- /dev/null +++ b/src/Rules/ForbidCredentialCastBypassRule.php @@ -0,0 +1,700 @@ +…->update([...])`, + * `->insert(...)`, `->upsert(...)`, a relation-derived builder write, or a + * `DB::table('…')` write on a table mapped to a model. + * + * Doctrine source: war-room §Architectural Principles #1 (Explicit over + * implicit) + #10 (credential handling / rotation-invariance); ISO 27001 A.5.33 + * and AVG on the compliance territories. + * + * **Why this is a security rule, not a style rule.** Attribute casts fire on + * the MODEL path only — `setAttribute()` runs the `hashed` / `encrypted` cast + * when you assign `$model->password = $plain` and `save()`. A query-builder + * write skips the model entirely: `Builder::update()` delegates to + * `toBase()->update()`, which ships the array straight to SQL. The value lands + * in the column verbatim — no hash, no encryption, no exception, and a green + * test suite, because a test that reads the column back gets exactly what it + * wrote. The failure is silent at every layer and is only ever discovered by + * reading the database. Seed: lokalekeuze PR #65, where `ReissueVoucherAction` + * wrote through the model by CHOICE while `BlockVoucherAction`'s builder idiom + * sat one file away — nothing but author preference separated the safe site + * from the unsafe one. + * + * Detection has three independent halves; all three must resolve or the rule + * stays SILENT (a credential-flavoured false positive spends the gate's + * authority faster than almost any other kind — ADR-0021 posture): + * + * 1. **Write verb + payload.** The call is one of `update`, `insert`, + * `insertOrIgnore`, `insertGetId`, `upsert`, `updateOrInsert`, and the + * payload argument resolves to a CONSTANT array type, so its keys are + * statically known. A payload built dynamically (`$data` of unknown + * shape, computed keys) is not a constant array type and is silent. + * Because the check is TYPE-based rather than AST-literal, a payload + * hoisted into a variable (`$p = ['password' => …]; $q->update($p);`) is + * still caught. A list-of-rows payload (`insert([[…], […]])`, `upsert`) + * is recognised by shape — all-integer keys over constant-array values — + * and each row's keys are collected. + * + * Deliberately ABSENT from the verb list: `create`, `updateOrCreate`, + * `firstOrCreate`, `createOrFirst`. Those are Eloquent-builder methods + * that instantiate a model and `save()` it, so casts DO fire — flagging + * them would criminalize the remediation. + * + * 2. **Receiver is a builder, never a model.** The receiver type must be a + * subtype of `Illuminate\Database\Eloquent\Builder`, + * `Illuminate\Database\Query\Builder`, or + * `Illuminate\Database\Eloquent\Relations\Relation`. A `Model` receiver is + * structurally excluded, which is what keeps the whole model path silent: + * `$model->update([...])` routes through `fill()` → `setAttribute()` and + * casts fire, so it is correct and must not fire here. + * + * 3. **Model resolution.** From an Eloquent `Builder` or a + * `Relation` the model comes out of the GENERIC + * argument — the first type argument that is a `Model` subtype. For a + * relation the first template parameter is `TRelatedModel`, which is the + * model whose table the write targets, so the same "first Model-subtype + * argument" reading is correct for both. A bare + * `Illuminate\Database\Query\Builder` (what `DB::table('x')` yields) + * carries NO model in its type, so it is resolved by walking the fluent + * chain back to the `table('…')` call with a string-literal argument and + * looking that table name up in the configured + * `credentialCastTableModels` map. That map is EMPTY by default, so + * `DB::table()` writes are silent until a consumer opts in — guessing a + * model from a table name by inflection would be exactly the + * false-positive source this rule cannot afford. + * + * **Reading the casts.** The model's cast map is read from SOURCE, because + * neither shape is reachable through reflection alone: the modern + * `protected function casts(): array` form needs a method body, and invoking it + * would mean instantiating an Eloquent model inside the analyser. The rule + * injects PHPStan's own analysis parser (`@defaultAnalysisParser` — cached, so + * a model file is parsed once per run), parses the file + * `ClassReflection::getFileName()` names, locates the class by resolved + * `namespacedName`, and collects `'column' => 'cast'` string pairs from BOTH + * the `casts()` method's `return [...]` statements and a `$casts` property + * default. Parents are walked and merged with the child winning, so a cast + * declared on an abstract base still counts at the leaf. A model whose file + * cannot be read, parsed, or located is treated as having no casts — silent. + * + * A cast counts as credential-bearing when its value is exactly `hashed`, + * exactly `encrypted`, or begins with `encrypted:` (`encrypted:array`, + * `encrypted:collection`, `encrypted:object`). + * + * Suppression: standard PHPStan inline-ignore mechanism on the rule's + * identifier `forbidCredentialCastBypass.castBypassedByBuilderWrite`. + * + * Out of scope — each an accepted false NEGATIVE, never a false positive: + * + * - **Class-based encrypted casts** — `AsEncryptedArrayObject::class`, + * `AsEncryptedCollection::class` and friends appear as `::class` constant + * fetches rather than the string values this rule matches. They carry the + * same bypass risk; a consumer needing them covered restates the column in + * string form or relies on the per-territory arch test. + * - **Dynamic payloads and dynamic keys** — not a constant array type, so + * the keys are not statically known. + * - **`upsert()`'s third argument** (the update-column list) — its column + * names are VALUES, not keys, and every column named there must already + * appear in the row payload this rule does read. + * - **Static-magic builder entry** (`Model::where(...)->update([...])` + * without larastan) — plain PHPStan cannot type `Model::__callStatic`, so + * the receiver resolves to an error type and the rule declines. Consumers + * running larastan get `Builder` there and the rule fires normally; + * `Model::query()->…` resolves on plain PHPStan either way. + * - **Raw SQL** (`DB::update('update users set …')`) — no payload array. + * + * @implements Rule + */ +final class ForbidCredentialCastBypassRule implements Rule +{ + private const string IDENTIFIER = 'forbidCredentialCastBypass.castBypassedByBuilderWrite'; + + /** + * Builder write verbs that ship their payload to SQL without routing + * through `Model::setAttribute()`, mapped to the argument positions + * carrying a `column => value` payload. + * + * `create` / `updateOrCreate` / `firstOrCreate` / `createOrFirst` are + * deliberately absent — they build and `save()` a model, so casts fire. + * + * @var array> + */ + private const array WRITE_METHODS = [ + 'update' => [0], + 'insert' => [0], + 'insertOrIgnore' => [0], + 'insertGetId' => [0], + 'upsert' => [0], + 'updateOrInsert' => [0, 1], + ]; + + /** The fluent-chain method whose string argument names the table. */ + private const string TABLE_SETTING_METHOD = 'table'; + + /** + * Cast values that mean "the model layer transforms this value on write". + * `encrypted:array` / `encrypted:collection` / `encrypted:object` are + * matched by the prefix entry. + * + * @var list + */ + private const array CREDENTIAL_CASTS = ['hashed', 'encrypted']; + + /** + * Cast maps already read this run, keyed by model FQCN. A model is parsed + * once even when a hundred call sites write to it. + * + * @var array> + */ + private array $castCache = []; + + /** + * @param array $credentialCastTableModels map of raw table + * name to model + * FQCN, used ONLY + * to resolve + * `DB::table('…')` + * chains, which + * carry no model in + * their type. Empty + * by default — an + * unmapped table is + * silent, never + * guessed. + */ + public function __construct( + private ReflectionProvider $reflectionProvider, + private Parser $parser, + private array $credentialCastTableModels = [], + ) {} + + public function getNodeType(): string + { + return MethodCall::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if (!$node->name instanceof Identifier) { + return []; + } + + $method = $node->name->toString(); + + if (!array_key_exists($method, self::WRITE_METHODS)) { + return []; + } + + $modelFqcn = $this->resolveModel($node, $scope); + + if ($modelFqcn === null) { + return []; + } + + $casts = $this->credentialCastsFor($modelFqcn); + + if ($casts === []) { + return []; + } + + $errors = []; + + foreach ($this->payloadColumns($node, $scope, self::WRITE_METHODS[$method]) as $column) { + if (!array_key_exists($column, $casts)) { + continue; + } + + $errors[] = $this->buildError($node, $modelFqcn, $column, $casts[$column], $method); + } + + return $errors; + } + + /** + * Resolve the model whose table this write targets, or null when it cannot + * be established statically. A `Model` receiver returns null on purpose — + * the model path fires casts and is the remediation, not the violation. + */ + private function resolveModel(MethodCall $node, Scope $scope): ?string + { + $receiverType = TypeCombinator::removeNull($scope->getType($node->var)); + + if ((new ObjectType(Model::class))->isSuperTypeOf($receiverType)->yes()) { + return null; + } + + $isEloquentBuilder = (new ObjectType(EloquentBuilder::class))->isSuperTypeOf($receiverType)->yes(); + $isRelation = (new ObjectType(Relation::class))->isSuperTypeOf($receiverType)->yes(); + + if ($isEloquentBuilder || $isRelation) { + $fromGenerics = $this->modelFromGenerics($receiverType); + + if ($fromGenerics !== null) { + return $fromGenerics; + } + } + + if (!(new ObjectType(QueryBuilder::class))->isSuperTypeOf($receiverType)->yes()) { + return null; + } + + return $this->modelFromChainTable($node->var); + } + + /** + * Pull the model out of a `Builder` / `Relation` + * generic argument list — the first argument that is a Model subtype. For a + * relation `TRelatedModel` comes first, and it is the model whose table the + * write targets, so one reading serves both shapes. + */ + private function modelFromGenerics(Type $receiverType): ?string + { + $modelType = new ObjectType(Model::class); + + foreach ($receiverType->getObjectClassReflections() as $classReflection) { + // The ACTIVE template map holds the arguments this particular + // instance was parameterized with, in template-declaration order — + // `TModel` for a Builder, `TRelatedModel` first for a Relation. + foreach ($classReflection->getActiveTemplateTypeMap()->getTypes() as $typeArgument) { + if (!$modelType->isSuperTypeOf($typeArgument)->yes()) { + continue; + } + + $referenced = $typeArgument->getReferencedClasses(); + + if ($referenced !== []) { + return $referenced[0]; + } + } + } + + return null; + } + + /** + * Walk the fluent chain back to the nearest `table('…')` call with a + * string-literal argument and look the table name up in the configured + * map. An unmapped table, a non-literal argument, or no `table()` call at + * all all yield null — the rule declines rather than guessing a model from + * a table name. + */ + private function modelFromChainTable(Expr $receiver): ?string + { + $current = $receiver; + + while ($current instanceof MethodCall || $current instanceof StaticCall) { + if ( + $current->name instanceof Identifier + && $current->name->toString() === self::TABLE_SETTING_METHOD + ) { + return $this->mappedModelForTableArg($current); + } + + if ($current instanceof MethodCall) { + $current = $current->var; + + continue; + } + + // A StaticCall is the chain root — its own arguments were inspected + // above, and there are no earlier hops to walk. + return null; + } + + return null; + } + + private function mappedModelForTableArg(MethodCall|StaticCall $call): ?string + { + if (!isset($call->args[0]) || !$call->args[0] instanceof Node\Arg) { + return null; + } + + $value = $call->args[0]->value; + + if (!$value instanceof String_) { + return null; + } + + return $this->credentialCastTableModels[$value->value] ?? null; + } + + /** + * Distinct column names appearing as keys in the payload arguments at + * `$argumentPositions`. Only constant array types are read; anything else + * contributes nothing. + * + * Deduplicated on purpose: a multi-row `insert([['password' => …], + * ['password' => …]])` names one offending column at one call site, and + * reporting it once per row would put N identical errors on one line. + * + * @param list $argumentPositions + * + * @return list + */ + private function payloadColumns(MethodCall $node, Scope $scope, array $argumentPositions): array + { + $seen = []; + + foreach ($argumentPositions as $position) { + if (!isset($node->args[$position]) || !$node->args[$position] instanceof Node\Arg) { + continue; + } + + foreach ($this->constantArrayKeys($scope->getType($node->args[$position]->value)) as $column) { + $seen[$column] = true; + } + } + + return array_keys($seen); + } + + /** + * String keys of a constant array type. A list of constant arrays (the + * `insert([[…], […]])` / `upsert` row shape) is recognised by having no + * string keys of its own while every value is itself a constant array, and + * is descended into. + * + * @return list + */ + private function constantArrayKeys(Type $type): array + { + $columns = []; + + foreach ($type->getConstantArrays() as $constantArray) { + foreach ($this->keysOfConstantArray($constantArray) as $column) { + $columns[] = $column; + } + } + + return $columns; + } + + /** + * @return list + */ + private function keysOfConstantArray(ConstantArrayType $type): array + { + $columns = []; + $nested = []; + + foreach ($type->getKeyTypes() as $index => $keyType) { + $constantStrings = $keyType->getConstantStrings(); + + if ($constantStrings !== []) { + $columns[] = $constantStrings[0]->getValue(); + + continue; + } + + $valueType = $type->getValueTypes()[$index] ?? null; + + if ($valueType !== null) { + foreach ($valueType->getConstantArrays() as $row) { + $nested[] = $row; + } + } + } + + // A payload is EITHER a `column => value` map or a list of such maps. + // Descending into rows only when the outer array contributed no column + // names of its own keeps a map with one array-valued column (a `json` + // cast written as an array) from being misread as a row list. + if ($columns !== []) { + return $columns; + } + + foreach ($nested as $row) { + foreach ($this->keysOfConstantArray($row) as $column) { + $columns[] = $column; + } + } + + return $columns; + } + + /** + * The model's credential-bearing casts as `column => cast`, merged across + * the ancestry with the child winning. Memoized per FQCN. + * + * @return array + */ + private function credentialCastsFor(string $modelFqcn): array + { + if (array_key_exists($modelFqcn, $this->castCache)) { + return $this->castCache[$modelFqcn]; + } + + $this->castCache[$modelFqcn] = []; + + if (!$this->reflectionProvider->hasClass($modelFqcn)) { + return []; + } + + $classReflection = $this->reflectionProvider->getClass($modelFqcn); + + $casts = []; + + // Ancestors first so a cast redeclared on the child overwrites the + // inherited one, matching how Laravel merges the maps at runtime. + foreach (array_reverse([$classReflection, ...$classReflection->getParents()]) as $ancestor) { + foreach ($this->declaredCasts($ancestor) as $column => $cast) { + $casts[$column] = $cast; + } + } + + $credentialCasts = []; + + foreach ($casts as $column => $cast) { + if ($this->isCredentialCast($cast)) { + $credentialCasts[$column] = $cast; + } + } + + $this->castCache[$modelFqcn] = $credentialCasts; + + return $credentialCasts; + } + + private function isCredentialCast(string $cast): bool + { + foreach (self::CREDENTIAL_CASTS as $credentialCast) { + if ($cast === $credentialCast || str_starts_with($cast, $credentialCast . ':')) { + return true; + } + } + + return false; + } + + /** + * `'column' => 'cast'` pairs declared on ONE class — read from the source + * file, since a `casts()` method body is not reachable through reflection + * and invoking it would mean instantiating an Eloquent model inside the + * analyser. Both declaration forms are read: the `casts()` method's + * `return [...]` statements and a `$casts` property default. + * + * @return array + */ + private function declaredCasts(ClassReflection $classReflection): array + { + $file = $classReflection->getFileName(); + + if ($file === null) { + return []; + } + + try { + $stmts = $this->parser->parseFile($file); + } catch (ParserErrorsException) { + return []; + } + + $classNode = $this->findClassNode($stmts, $classReflection->getName()); + + if ($classNode === null) { + return []; + } + + $casts = []; + + foreach ($classNode->stmts as $stmt) { + if ($stmt instanceof ClassMethod && $stmt->name->toString() === 'casts') { + foreach ($this->returnedArrays($stmt) as $array) { + foreach ($this->stringPairs($array) as $column => $cast) { + $casts[$column] = $cast; + } + } + + continue; + } + + if (!$stmt instanceof Property) { + continue; + } + + foreach ($stmt->props as $prop) { + if ($prop->name->toString() !== 'casts' || $prop->default === null) { + continue; + } + + foreach ($this->stringPairs($prop->default) as $column => $cast) { + $casts[$column] = $cast; + } + } + } + + return $casts; + } + + /** + * Locate the class declaration for `$fqcn` among parsed statements. The + * injected parser resolves names, so `namespacedName` is populated and the + * match is exact rather than by short name. + * + * @param array $nodes + */ + private function findClassNode(array $nodes, string $fqcn): ?Class_ + { + foreach ($nodes as $node) { + if ($node instanceof Class_ && $node->namespacedName?->toString() === $fqcn) { + return $node; + } + + $found = $this->findClassNode($this->childNodes($node), $fqcn); + + if ($found !== null) { + return $found; + } + } + + return null; + } + + /** + * A node's direct child NODES. Sub-node slots also hold strings, ints, + * booleans and nulls (`Class_::$flags`, `Identifier::$name`, …), so the + * slot values are filtered rather than assumed traversable. + * + * @return list + */ + private function childNodes(Node $node): array + { + $children = []; + + foreach ($node->getSubNodeNames() as $subNodeName) { + $subNode = $node->{$subNodeName}; + + foreach (is_array($subNode) ? $subNode : [$subNode] as $candidate) { + if ($candidate instanceof Node) { + $children[] = $candidate; + } + } + } + + return $children; + } + + /** + * Every array literal returned from a method body, including returns nested + * inside conditionals. + * + * @return list + */ + private function returnedArrays(ClassMethod $method): array + { + $arrays = []; + + $this->collectReturnedArrays($this->childNodes($method), $arrays); + + return $arrays; + } + + /** + * @param list $nodes + * @param list $arrays + */ + private function collectReturnedArrays(array $nodes, array &$arrays): void + { + foreach ($nodes as $node) { + if ($node instanceof Return_ && $node->expr instanceof Expr\Array_) { + $arrays[] = $node->expr; + + continue; + } + + // A nested closure or anonymous class carries its own returns, + // which are not this method's cast map. + if ($node instanceof Expr\Closure || $node instanceof Class_) { + continue; + } + + $this->collectReturnedArrays($this->childNodes($node), $arrays); + } + } + + /** + * `'key' => 'value'` pairs of an array literal. Non-string keys and + * non-string values (a `::class` constant fetch, a computed expression) are + * skipped — see the class docblock's out-of-scope list. + * + * @return array + */ + private function stringPairs(Expr $expr): array + { + if (!$expr instanceof Expr\Array_) { + return []; + } + + $pairs = []; + + foreach ($expr->items as $item) { + if ($item->key instanceof String_ && $item->value instanceof String_) { + $pairs[$item->key->value] = $item->value->value; + } + } + + return $pairs; + } + + private function buildError( + MethodCall $node, + string $modelFqcn, + string $column, + string $cast, + string $method, + ): IdentifierRuleError { + $message = sprintf( + "Attribute '%s' on %s carries the '%s' cast, but %s() is a query-builder write that bypasses the cast and stores the raw value. Write it through the model path instead (assign the attribute and save the model).", + $column, + $modelFqcn, + $cast, + $method, + ); + + return RuleErrorBuilder::message($message) + ->identifier(self::IDENTIFIER) + ->line($node->getStartLine()) + ->build(); + } +} diff --git a/tests/Fixtures/CredentialCastBypass/AbstractCredentialHolder.php b/tests/Fixtures/CredentialCastBypass/AbstractCredentialHolder.php new file mode 100644 index 0000000..6379ee0 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/AbstractCredentialHolder.php @@ -0,0 +1,24 @@ + + */ + protected function casts(): array + { + return [ + 'passphrase' => 'hashed', + ]; + } +} diff --git a/tests/Fixtures/CredentialCastBypass/ApiKey.php b/tests/Fixtures/CredentialCastBypass/ApiKey.php new file mode 100644 index 0000000..d260b2c --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/ApiKey.php @@ -0,0 +1,21 @@ + */ + protected $casts = [ + 'secret' => 'encrypted', + 'label' => 'string', + ]; +} diff --git a/tests/Fixtures/CredentialCastBypass/Article.php b/tests/Fixtures/CredentialCastBypass/Article.php new file mode 100644 index 0000000..5b047fa --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/Article.php @@ -0,0 +1,25 @@ + + */ + protected function casts(): array + { + return [ + 'published_at' => 'datetime', + ]; + } +} diff --git a/tests/Fixtures/CredentialCastBypass/BuilderWrites.php b/tests/Fixtures/CredentialCastBypass/BuilderWrites.php new file mode 100644 index 0000000..c121c38 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/BuilderWrites.php @@ -0,0 +1,76 @@ +where('id', 1)->update(['password' => 'plaintext']); + } + + public function encryptedColumn(): void + { + User::query()->where('id', 1)->update(['api_token' => 'raw-token']); + } + + public function encryptedVariantColumn(): void + { + User::query()->where('id', 1)->update(['recovery_codes' => ['a', 'b']]); + } + + public function castsPropertyForm(): void + { + ApiKey::query()->update(['secret' => 'raw']); + } + + public function inheritedFromAbstractBase(): void + { + Vault::query()->update(['passphrase' => 'raw']); + } + + public function payloadHoistedIntoVariable(): void + { + $payload = ['password' => 'plaintext']; + + User::query()->update($payload); + } + + public function insertRowList(): void + { + User::query()->insert([ + ['password' => 'one'], + ['password' => 'two'], + ]); + } + + public function upsertRows(): void + { + User::query()->upsert([['email' => 'a@b.c', 'password' => 'raw']], ['email']); + } + + public function updateOrInsertSecondArgument(): void + { + User::query()->updateOrInsert(['email' => 'a@b.c'], ['password' => 'raw']); + } + + public function relationDerivedWrite(User $user): void + { + $user->apiKeys()->update(['secret' => 'raw']); + } + + public function twoCredentialColumnsInOnePayload(): void + { + User::query()->update(['password' => 'p', 'api_token' => 't']); + } +} diff --git a/tests/Fixtures/CredentialCastBypass/CleanWrites.php b/tests/Fixtures/CredentialCastBypass/CleanWrites.php new file mode 100644 index 0000000..4eb53d0 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/CleanWrites.php @@ -0,0 +1,110 @@ +password = 'plaintext'; + $user->save(); + } + + /** + * `Model::update()` fills through `setAttribute()`, so casts fire. + */ + public function modelUpdate(User $user): void + { + $user->update(['password' => 'plaintext']); + } + + /** + * `Model::create()` instantiates and saves — casts fire. + */ + public function modelCreate(): void + { + User::create(['password' => 'plaintext']); + } + + /** + * Builder `create()` also builds and saves a model — casts fire. + */ + public function builderCreate(): void + { + User::query()->create(['password' => 'plaintext']); + } + + /** + * Builder `updateOrCreate()` routes through the model too. + */ + public function builderUpdateOrCreate(): void + { + User::query()->updateOrCreate(['email' => 'a@b.c'], ['password' => 'plaintext']); + } + + /** + * A builder write to columns carrying no credential cast. + */ + public function builderWriteToNonCastColumn(): void + { + User::query()->update(['login_count' => 3]); + } + + /** + * A model with no credential casts at all. + */ + public function builderWriteOnUncastModel(): void + { + Article::query()->update(['published_at' => 'now']); + } + + /** + * `DB::table()` carries no model in its type, and no table is mapped here. + */ + public function unmappedRawTableWrite(): void + { + DB::table('users')->update(['password' => 'plaintext']); + } + + /** + * The child REDECLARES the abstract parent's `passphrase` cast as `string`. + * Child wins, so this is no longer a credential column. Pins the merge + * DIRECTION, not merely the fact that a merge happens. + */ + public function childOverridesInheritedCredentialCast(): void + { + OverridingVault::query()->update(['passphrase' => 'plain']); + } + + /** + * Cast values that only look like credential casts (`encryptedish`). + */ + public function nearMissCastNames(): void + { + NearMissCastModel::query()->update(['blob' => 'x', 'digest' => 'y']); + } + + /** + * A payload of unknown shape — keys are not statically known. + */ + public function dynamicPayload(string $column, string $value): void + { + User::query()->update([$column => $value]); + } +} diff --git a/tests/Fixtures/CredentialCastBypass/NearMissCastModel.php b/tests/Fixtures/CredentialCastBypass/NearMissCastModel.php new file mode 100644 index 0000000..0eb571d --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/NearMissCastModel.php @@ -0,0 +1,28 @@ + + */ + protected function casts(): array + { + return [ + 'blob' => 'encryptedish', + 'digest' => 'hashedish', + ]; + } +} diff --git a/tests/Fixtures/CredentialCastBypass/OverridingVault.php b/tests/Fixtures/CredentialCastBypass/OverridingVault.php new file mode 100644 index 0000000..ed91740 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/OverridingVault.php @@ -0,0 +1,26 @@ + + */ + protected function casts(): array + { + return [ + 'passphrase' => 'string', + ]; + } +} diff --git a/tests/Fixtures/CredentialCastBypass/RawTableWrites.php b/tests/Fixtures/CredentialCastBypass/RawTableWrites.php new file mode 100644 index 0000000..ab1a76d --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/RawTableWrites.php @@ -0,0 +1,40 @@ +update(['password' => 'plaintext']); + } + + public function mappedTableWithIntermediateChainHops(): void + { + DB::table('users')->where('id', 1)->limit(1)->update(['api_token' => 'raw']); + } + + public function mappedTableNonCastColumn(): void + { + DB::table('users')->update(['login_count' => 1]); + } + + public function unmappedTable(): void + { + DB::table('articles')->update(['password' => 'plaintext']); + } + + public function nonLiteralTableName(string $table): void + { + DB::table($table)->update(['password' => 'plaintext']); + } +} diff --git a/tests/Fixtures/CredentialCastBypass/User.php b/tests/Fixtures/CredentialCastBypass/User.php new file mode 100644 index 0000000..fa8d80a --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/User.php @@ -0,0 +1,38 @@ + + */ + public function apiKeys(): HasMany + { + return $this->hasMany(ApiKey::class); + } + + /** + * @return array + */ + protected function casts(): array + { + return [ + 'password' => 'hashed', + 'api_token' => 'encrypted', + 'recovery_codes' => 'encrypted:array', + 'login_count' => 'integer', + ]; + } +} diff --git a/tests/Fixtures/CredentialCastBypass/Vault.php b/tests/Fixtures/CredentialCastBypass/Vault.php new file mode 100644 index 0000000..3ecec8e --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/Vault.php @@ -0,0 +1,10 @@ + + */ +final class ForbidCredentialCastBypassRuleTest extends RuleTestCase +{ + private const string MESSAGE = "Attribute '%s' on %s carries the '%s' cast, but %s() is a query-builder write that bypasses the cast and stores the raw value. Write it through the model path instead (assign the attribute and save the model)."; + + private const string BUILDER_WRITES = __DIR__ . '/../Fixtures/CredentialCastBypass/BuilderWrites.php'; + + private const string CLEAN_WRITES = __DIR__ . '/../Fixtures/CredentialCastBypass/CleanWrites.php'; + + private const string RAW_TABLE_WRITES = __DIR__ . '/../Fixtures/CredentialCastBypass/RawTableWrites.php'; + + /** + * Table-to-model map used by the `DB::table()` tests. Left null so the + * default (empty map, `DB::table()` silent) is what every other test sees. + * + * @var array|null + */ + private ?array $tableModelOverride = null; + + // ---------------------------------------------------------------- RED --- + + public function testHashedCastColumnInBuilderUpdateIsFlagged(): void + { + $this->analyse([self::BUILDER_WRITES], [ + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 19], + [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 24], + [sprintf(self::MESSAGE, 'recovery_codes', 'App\Models\CredentialCastBypass\User', 'encrypted:array', 'update'), 29], + [sprintf(self::MESSAGE, 'secret', 'App\Models\CredentialCastBypass\ApiKey', 'encrypted', 'update'), 34], + [sprintf(self::MESSAGE, 'passphrase', 'App\Models\CredentialCastBypass\Vault', 'hashed', 'update'), 39], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 46], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'insert'), 51], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'upsert'), 59], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'updateOrInsert'), 64], + [sprintf(self::MESSAGE, 'secret', 'App\Models\CredentialCastBypass\ApiKey', 'encrypted', 'update'), 69], + [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 74], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 74], + ]); + } + + public function testRawTableWriteIsFlaggedOnlyForMappedTables(): void + { + $this->tableModelOverride = ['users' => 'App\Models\CredentialCastBypass\User']; + + $this->analyse([self::RAW_TABLE_WRITES], [ + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 18], + [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 23], + ]); + } + + // -------------------------------------------------------------- GREEN --- + + public function testModelPathAndNonCredentialWritesAreClean(): void + { + $this->analyse([self::CLEAN_WRITES], []); + } + + public function testRawTableWritesAreSilentWithTheDefaultEmptyMap(): void + { + $this->analyse([self::RAW_TABLE_WRITES], []); + } + + // ---------------------------------------------------------- DENOMINATOR --- + + /** + * § Null-Result corollary 1 — the green fixtures above report zero both when + * the rule is correctly silent and when the fixture file is empty, renamed + * or stopped being parsed. Assert the population the clean assertions are + * measuring is non-zero, and that the red fixture really does carry a write + * per flagged line. + */ + public function testFixturePopulationIsNonZero(): void + { + $writeCalls = static function(string $file): int { + $source = file_get_contents($file); + + self::assertNotFalse($source, sprintf('Fixture %s could not be read.', $file)); + + return preg_match_all('/(->|::)(update|insert|insertOrIgnore|insertGetId|upsert|updateOrInsert|create|updateOrCreate|save)\(/', $source); + }; + + self::assertGreaterThanOrEqual(11, $writeCalls(self::BUILDER_WRITES), 'The violating fixture lost write sites.'); + self::assertGreaterThanOrEqual(9, $writeCalls(self::CLEAN_WRITES), 'The clean fixture lost write sites, so its zero proves nothing.'); + self::assertGreaterThanOrEqual(5, $writeCalls(self::RAW_TABLE_WRITES), 'The raw-table fixture lost write sites.'); + } + + /** + * This rule is the first in the package to resolve a class FQCN to a FILE + * and parse it, so it is uniquely sensitive to an FQCN declared twice in the + * fixture corpus: reflection picks one declaration, and which one is + * classmap-order dependent. Measured — the model fixtures were first written + * in `App\Models`, where `User` already collided with three other rules' + * stubs; the suite passed on one installed tree and failed on another purely + * because `composer update` reordered the classmap. + * + * A comment saying "do not collide" would not survive; this does. + */ + public function testEveryFixtureModelResolvesToThisRuleSOwnFixtureDirectory(): void + { + $models = [ + 'App\Models\CredentialCastBypass\User', + 'App\Models\CredentialCastBypass\ApiKey', + 'App\Models\CredentialCastBypass\Vault', + 'App\Models\CredentialCastBypass\Article', + 'App\Models\CredentialCastBypass\OverridingVault', + 'App\Models\CredentialCastBypass\NearMissCastModel', + 'App\Models\CredentialCastBypass\AbstractCredentialHolder', + ]; + + $reflectionProvider = self::createReflectionProvider(); + $expectedDirectory = realpath(__DIR__ . '/../Fixtures/CredentialCastBypass'); + + self::assertNotFalse($expectedDirectory, 'The fixture directory is missing.'); + + foreach ($models as $model) { + self::assertTrue( + $reflectionProvider->hasClass($model), + sprintf('Fixture model %s does not resolve at all.', $model), + ); + + self::assertSame( + $expectedDirectory, + dirname((string) $reflectionProvider->getClass($model)->getFileName()), + sprintf( + '%s resolves to a file outside this rule\'s fixture directory, so another fixture declares the same FQCN. The rule reads casts from the resolved FILE, so it would silently read the wrong model.', + $model, + ), + ); + } + } + + protected function getRule(): Rule + { + $parser = self::getContainer()->getService('defaultAnalysisParser'); + + self::assertInstanceOf(Parser::class, $parser); + + return new ForbidCredentialCastBypassRule( + self::createReflectionProvider(), + $parser, + $this->tableModelOverride ?? [], + ); + } +} From d958a29d87c5d698b99b896a289261e37ee9526e Mon Sep 17 00:00:00 2001 From: Gerard Oosterhof Date: Sun, 30 Aug 2026 17:28:29 +0200 Subject: [PATCH 2/3] fix(queue-217): trait-declared casts, chain-scoped DB::table, and fail-open on unreadable model source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 2 + CLAUDE.md | 2 +- README.md | 31 +++- src/Rules/ForbidCredentialCastBypassRule.php | 172 ++++++++++++++---- .../CredentialCastBypass/BuilderWrites.php | 11 ++ .../CredentialCastBypass/CleanWrites.php | 11 ++ .../ComposesHashedSecret.php | 14 ++ .../HasEncryptedNotesProperty.php | 17 ++ .../CredentialCastBypass/HasHashedSecret.php | 24 +++ .../CredentialCastBypass/RawTableWrites.php | 17 ++ .../SingleUncastWrite.php | 20 ++ .../CredentialCastBypass/TraitCastModel.php | 19 ++ .../TraitOverriddenCastModel.php | 30 +++ .../ForbidCredentialCastBypassRuleTest.php | 86 +++++++-- tests/Support/ThrowingParser.php | 46 +++++ 15 files changed, 451 insertions(+), 51 deletions(-) create mode 100644 tests/Fixtures/CredentialCastBypass/ComposesHashedSecret.php create mode 100644 tests/Fixtures/CredentialCastBypass/HasEncryptedNotesProperty.php create mode 100644 tests/Fixtures/CredentialCastBypass/HasHashedSecret.php create mode 100644 tests/Fixtures/CredentialCastBypass/SingleUncastWrite.php create mode 100644 tests/Fixtures/CredentialCastBypass/TraitCastModel.php create mode 100644 tests/Fixtures/CredentialCastBypass/TraitOverriddenCastModel.php create mode 100644 tests/Support/ThrowingParser.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 20a1146..a6342b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and **Teeth proved in both directions.** Flipping `'password' => 'hashed'` to `'string'` on the model fixture dropped exactly the six `password` findings and left every other cast firing; injecting `'password'` into a clean-fixture builder write turned the green assertion red at that line. A denominator test asserts each fixture still carries its write sites, so the two zero-expectation assertions cannot pass on an empty or unparsed file. The `extension.neon` wiring was additionally smoked through the real `phpstan analyse` entry point, not only `RuleTestCase`. + **Review round 1 (crit, PR #68 — three issues, all fixed in a follow-up commit).** (1) **Trait-declared casts were invisible.** Laravel models compose cast maps from traits routinely, and the resolution walked only the class ancestry — so a `hashed` cast declared in a trait silently exempted every model using it, on a rule whose whole purpose is catching a silent plaintext write. The walk now covers the trait use-chain via `getTraits(true)` (flattening traits-of-traits) in PHP's own member-resolution order: per ancestor, oldest first, traits then the class's own declarations, so class-declared beats trait-imported beats inherited. (2) **`DB::table()` hoisted into a variable** defeats the chain walk. Fixed as far as it can be — `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 `Illuminate\Database\Query\Builder` which carries no table name, so there is nothing to read. Now documented and pinned by a fixture rather than 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. `declaredCasts()` now returns `null` for "could not look" as distinct from `[]` for "looked, found none", and the call site is reported under a **separate identifier `forbidCredentialCastBypass.modelSourceUnreadable`** — regardless of the payload, because with an incomplete map the rule cannot claim the payload is clean. Each fix is teeth-proved in both directions, including a fixture pinning the trait-vs-class precedence DIRECTION and a test that injects a parser failing on exactly one model file (a syntactically broken fixture would break the suite's classmap rather than the branch under test). + **Versioning: candidate MAJOR** — it surfaces new errors in code that previously passed wherever a consumer writes a credential column through a builder. Per the pre-1.0 caret convention `^0.8` excludes the next minor, so tagging auto-adopts nobody; each consumer adopts on its own pin-bump PR. **NOT tagged** (release is a General/Commander step). - `ForbidUntimedHttpClientRule` — new rule enforcing war-room **Architectural Principle #8** (explicit timeouts on outbound HTTP) at analysis time, the AST-aware successor to the per-territory `ExternalHttpTimeoutTest` named-list Pest tests (kendo / emmie). The named-list tests detect wrong-shape on *enrolled* classes but are blind to **omission** — a new untimed call nobody adds to the list; this rule closes the omission gap for the tractable call shapes. Registered in `extension.neon` (no parameters). Identifier: `forbidUntimedHttpClient.missingTimeout`. Doctrine: war-room §Architectural Principles #8. Seed: war-room enforcement queue #58 (spike branch `spike/wr-queue58-untimed-http-client`). **Review follow-up (bus #57 findings):** the `withOptions()` check is TYPE-aware, not AST-literal — a constant array type provably lacking `'timeout'` still fires (including through a variable holding a literal array, a widening over the inline-`Array_`-only first cut), while a computed/helper-built options expression (not a constant array type) is POSSIBLY timed and the chain DECLINES (the Major: flagging it was a false positive). A chain member outside the known `PendingRequest` builder surface — a Macroable extension (`Http::github()`, root or intermediate) or `when()`/`unless()` with their opaque closures — likewise declines (the Minor: a macro may return a pre-timed request); a genuine builder missing from the list costs only a false negative. diff --git a/CLAUDE.md b/CLAUDE.md index f399295..0e041f1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -38,7 +38,7 @@ Composer package distributing war-room-doctrine PHPStan rules across `script-dev | `ForbidUntimedHttpClientRule` | War-room §Explicit HTTP timeouts (#8) | `forbidUntimedHttpClient.missingTimeout` (type-aware; flags an `Http` facade OR injected `Illuminate\Http\Client\Factory` chain that reaches a send verb with no explicit `->timeout()` / `withOptions(['timeout'])`. Conservative — fires only on fully-visible single-expression chains; declines split/helper-built chains + Guzzle/SDK surfaces to hold FP at zero. COMPLEMENTS, does not replace, the per-territory `ExternalHttpTimeoutTest`. on `main`, `[Unreleased]`) | | `EnforceAuditModelProtectionsRule` | ADR-0001 §Append-only | `enforceAuditModelProtections.hasFactoryForbidden` / `.softDeletesForbidden` / `.updatedAtNotDisabled` (denylist-inversion; discovers audit models by shape — `auditModelNameSuffixes` default `AuditLog` OR `auditModelNamespacePrefixes` default `App\Models\Audit` — and flags `HasFactory` / `SoftDeletes` / missing `const UPDATED_AT = null`. shipped v0.7.0) | | `EnforceActionResultDtoRule` | ADR-0020 + ADR-0011 | `enforceActionResultDto.arrayReturnFromExecute` (signature-only; flags an `array` / `?array` / `array\|Dto` union / `iterable` native return type on `App\Actions\*` `execute()`. Phpdoc-only `@return array{...}` is a deliberate miss; no `list` carve-out. Seed kendo PR #1653. shipped v0.8.0) | -| `ForbidCredentialCastBypassRule` | War-room §Explicit over implicit (#1) + §Rotation-invariant credential handling (#10) | `forbidCredentialCastBypass.castBypassedByBuilderWrite` (flags a `hashed` / `encrypted` / `encrypted:*` cast column appearing as a key in a QUERY-BUILDER write payload — `update`/`insert`/`insertOrIgnore`/`insertGetId`/`upsert`/`updateOrInsert` on an Eloquent `Builder`, query `Builder` or `Relation`. Casts fire on the model path only, so a builder write stores the credential in plaintext with a green suite. The model path and the model-routing builder verbs (`create`/`updateOrCreate`/`firstOrCreate`/`createOrFirst`) are silent STRUCTURALLY — the receiver type gate, not an exemption list. Model comes from the builder/relation generic; `DB::table('…')` carries none and resolves only via the opt-in `credentialCastTableModels` map, default `[]`. Cast maps read from model SOURCE via the injected `@defaultAnalysisParser` — both `casts()` and `$casts`, merged across the ancestry. Payload keys read from the CONSTANT ARRAY TYPE, so a hoisted variable is caught. Seed lokalekeuze PR #65. on `main`, `[Unreleased]`) | +| `ForbidCredentialCastBypassRule` | War-room §Explicit over implicit (#1) + §Rotation-invariant credential handling (#10) | `forbidCredentialCastBypass.castBypassedByBuilderWrite` / `.modelSourceUnreadable` (flags a `hashed` / `encrypted` / `encrypted:*` cast column appearing as a key in a QUERY-BUILDER write payload — `update`/`insert`/`insertOrIgnore`/`insertGetId`/`upsert`/`updateOrInsert` on an Eloquent `Builder`, query `Builder` or `Relation`. Casts fire on the model path only, so a builder write stores the credential in plaintext with a green suite. The model path and the model-routing builder verbs (`create`/`updateOrCreate`/`firstOrCreate`/`createOrFirst`) are silent STRUCTURALLY — the receiver type gate, not an exemption list. Model comes from the builder/relation generic; `DB::table('…')` carries none and resolves only via the opt-in `credentialCastTableModels` map, default `[]`. Cast maps read from model SOURCE via the injected `@defaultAnalysisParser` — both `casts()` and `$casts`, merged across the ancestry AND the trait use-chain (class-declared beats trait-imported beats inherited). A source whose PHP cannot be read is NOT treated as castless — that fails open — but reported under the second identifier `forbidCredentialCastBypass.modelSourceUnreadable`. Payload keys read from the CONSTANT ARRAY TYPE, so a hoisted variable is caught. Seed lokalekeuze PR #65. on `main`, `[Unreleased]`) | | `ConnectionTransactionReturnTypeExtension` | (type extension, no rule) | — (resolves `ConnectionInterface::transaction()` to the closure's return type. **Load-bearing on `illuminate/* ^12` only** — it annotates `@return mixed`; Laravel 13 annotates `@template TReturn`/`@return TReturn` and infers the same type unaided. Its teeth are measured by the `check-lowest-laravel` CI job, not by the unit suite on a Laravel-13 tree. WR-0855, WR-0860) | Phase 2 expands the rule set: `EnforceAuditSnapshotOnRetryRule` (ADR-0001 §Snapshot-on-Retry Safety) was the first Phase 2 addition, promoted from cross-territory Pest arch tests (emmie PR #187, entreezuil PR #139, ublgenie PR #166, kendo PR #1029). `EnforceResourceDataValidatorOptInRule` (ADR-0009 §EAGER_LOAD validator opt-in) is the second Phase 2 addition, promoted from kendo PR #1084 under war-room enforcement queue #55. `EnforceFormRequestToDtoRule` (ADR-0012) is the third Phase 2 addition, promoted from entreezuil's `tests/Arch/FormRequestsTest.php` under the same queue #55 (instance 2). `EnforceExplicitHydrationRule` (ADR-0019) is the next Phase 2 candidate. diff --git a/README.md b/README.md index ebadff9..3ea307e 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ includes: | `EnforceCurrentUserAttributeRule` | `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser` | `Request::user()` / `Auth::user()` / `auth()->user()` calls inside `App\Http\Controllers\*` classes (namespace prefix, incl. sub-namespaces; configurable via `controllerNamespacePrefixes`) | Use `#[\Illuminate\Container\Attributes\CurrentUser] User $user` on the method parameter. Scope is decided by namespace, not class ancestry — a base-less `final` controller in `App\Http\Controllers` fires; FormRequests (`App\Http\Requests`), middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs, and console commands are silent because their namespaces do not start with the controller prefix (container-attribute injection does not apply to FormRequest methods regardless). | | `EnforceCurrentUserAttributeRule` | `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser` | `Request::user()` / `Auth::user()` / `auth()->user()` calls inside `App\Http\Controllers\*` classes (namespace prefix, incl. sub-namespaces) | Use `#[\Illuminate\Container\Attributes\CurrentUser] User $user` on the method parameter. Scope is decided by namespace, not class ancestry — a base-less `final` controller in `App\Http\Controllers` fires; FormRequests (`App\Http\Requests`), middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs, and console commands are silent because their namespaces do not start with the controller prefix (container-attribute injection does not apply to FormRequest methods regardless). | | `EnforceAuditModelProtectionsRule` | `enforceAuditModelProtections.hasFactoryForbidden` / `.softDeletesForbidden` / `.updatedAtNotDisabled` | Eloquent models recognised as audit records by SHAPE — short name ends with a configured suffix (default `AuditLog`) OR FQCN sits under a configured namespace (default `App\Models\Audit`) | Three append-only protections, each firing independently: using `HasFactory` (a factory is a direct-insert path bypassing the hash-chained writer), using `SoftDeletes` (audit rows are never removed), or not disabling `updated_at` (an audit row is written once and never mutated — declare `public const UPDATED_AT = null;`) is an error. Discovery is by pattern, never a hand-maintained class list — a denylist inversion, so a newly-added audit model cannot escape the protections by omission. Abstract intermediates are exempt (their concrete leaves carry inherited violations). Non-model classes named `*AuditLog` are excluded by the Eloquent `Model` type gate. Doctrine: ADR-0001 §Append-only. | -| `ForbidCredentialCastBypassRule` | `forbidCredentialCastBypass.castBypassedByBuilderWrite` | Query-builder write calls (`update`, `insert`, `insertOrIgnore`, `insertGetId`, `upsert`, `updateOrInsert`) whose receiver is an `Illuminate\Database\Eloquent\Builder`, `Illuminate\Database\Query\Builder`, or `Illuminate\Database\Eloquent\Relations\Relation` | Naming a column that carries a `hashed`, `encrypted` or `encrypted:*` cast as a key in the write payload is an error. Casts fire on the MODEL path only; a builder write delegates to `toBase()->update()` and ships the raw value to SQL — no hash, no encryption, no exception, and a green test suite. The model path is **structurally** silent (a `Model` receiver never matches), and so are the builder methods that route through a model (`create`, `updateOrCreate`, `firstOrCreate`, `createOrFirst`) — they are the remediation. The model comes from the builder's/relation's generic type argument; a `DB::table('…')` chain carries none and resolves only through the opt-in `credentialCastTableModels` map (empty by default ⇒ silent, never inferred from the table name). The cast map is read from the model SOURCE — both the `casts()` method and a `$casts` property, merged across the ancestry. Payload keys are read from the resolved **constant array type**, so a payload hoisted into a variable is caught and a dynamic payload is silent. Deliberate misses: class-based casts (`AsEncryptedArrayObject::class`), dynamic keys, `upsert()`'s third argument, and `Model::where(...)` static-magic entry without larastan. Doctrine: war-room §Architectural Principles #1 + #10; ISO 27001 A.5.33 / AVG. Seed: lokalekeuze PR #65. | +| `ForbidCredentialCastBypassRule` | `forbidCredentialCastBypass.castBypassedByBuilderWrite` | Query-builder write calls (`update`, `insert`, `insertOrIgnore`, `insertGetId`, `upsert`, `updateOrInsert`) whose receiver is an `Illuminate\Database\Eloquent\Builder`, `Illuminate\Database\Query\Builder`, or `Illuminate\Database\Eloquent\Relations\Relation` | Naming a column that carries a `hashed`, `encrypted` or `encrypted:*` cast as a key in the write payload is an error. Casts fire on the MODEL path only; a builder write delegates to `toBase()->update()` and ships the raw value to SQL — no hash, no encryption, no exception, and a green test suite. The model path is **structurally** silent (a `Model` receiver never matches), and so are the builder methods that route through a model (`create`, `updateOrCreate`, `firstOrCreate`, `createOrFirst`) — they are the remediation. The model comes from the builder's/relation's generic type argument; a `DB::table('…')` chain carries none and resolves only through the opt-in `credentialCastTableModels` map (empty by default ⇒ silent, never inferred from the table name). The cast map is read from the model SOURCE — both the `casts()` method and a `$casts` property, merged across the ancestry **and the trait use-chain** (class-declared beats trait-imported beats inherited; `getTraits(true)` flattens traits-of-traits). A declaring source whose PHP cannot be located or parsed is NOT treated as "declares no casts" — that would fail open — and is reported under the separate identifier `forbidCredentialCastBypass.modelSourceUnreadable`. Payload keys are read from the resolved **constant array type**, so a payload hoisted into a variable is caught and a dynamic payload is silent. Deliberate misses: class-based casts (`AsEncryptedArrayObject::class`), dynamic keys, `upsert()`'s third argument, `Model::where(...)` static-magic entry without larastan, and a `DB::table()` builder hoisted into a variable (the variable's type carries no table name). Doctrine: war-room §Architectural Principles #1 + #10; ISO 27001 A.5.33 / AVG. Seed: lokalekeuze PR #65. | ### `EnforceActionTransactionsRule` — write-method list @@ -254,6 +254,35 @@ are worth listing; a mapped table whose model has no credential cast changes nothing. Single backslashes in the FQCN — NEON only unescapes `\\` inside double quotes. +Chain forms all resolve, including `DB::connection('mysql')->table('users')` and +any number of intermediate hops (`->where(...)->limit(...)`). What does **not** +resolve is a builder hoisted into a variable: + +```php +$query = DB::table('users'); +$query->update(['password' => $plain]); // silent — see below +``` + +The walk needs the `table('…')` string literal, and the variable's type is a bare +`Illuminate\Database\Query\Builder` carrying no table name, so there is nothing +left to read. This is a limitation of the query builder's type rather than of the +walk, and it is a false negative, never a false positive. + +### `ForbidCredentialCastBypassRule` — when the model source cannot be read + +If a declaring class or trait's PHP cannot be located or parsed, the cast map is +incomplete and the rule cannot vouch for the payload. Rather than pass silently — +which would fail open on exactly the models it exists to guard — it reports the +call site under its own identifier: + +``` +forbidCredentialCastBypass.modelSourceUnreadable +``` + +This is deliberately independent of the payload: with an incomplete map, a +credential column in that payload would go unreported. Suppress this identifier +alone (per file or per line) if a write is known safe; the real check stays armed. + ### `ForbidRawExceptionMessageInResponseRule` — configurable sinks + `@leak-safe` exemption The rule flags a raw `Throwable::getMessage()` (or the `Throwable` itself) reaching a **client-facing response sink** — an information-disclosure leak. The built-in default sink `Laravel\Mcp\Response::error` is always armed; a consumer adds more (a persist-error setter, a `MarkInvoiceFailed` Action) via the `rawExceptionMessageSinks` parameter — a list of `FQCN::method` signatures, default `[]`: diff --git a/src/Rules/ForbidCredentialCastBypassRule.php b/src/Rules/ForbidCredentialCastBypassRule.php index d1ca90d..7027352 100644 --- a/src/Rules/ForbidCredentialCastBypassRule.php +++ b/src/Rules/ForbidCredentialCastBypassRule.php @@ -15,6 +15,7 @@ use PhpParser\Node\Identifier; use PhpParser\Node\Scalar\String_; use PhpParser\Node\Stmt\Class_; +use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\Property; use PhpParser\Node\Stmt\Return_; @@ -34,6 +35,7 @@ use function array_key_exists; use function array_keys; use function array_reverse; +use function implode; use function is_array; use function sprintf; use function str_starts_with; @@ -113,9 +115,24 @@ * `ClassReflection::getFileName()` names, locates the class by resolved * `namespacedName`, and collects `'column' => 'cast'` string pairs from BOTH * the `casts()` method's `return [...]` statements and a `$casts` property - * default. Parents are walked and merged with the child winning, so a cast - * declared on an abstract base still counts at the leaf. A model whose file - * cannot be read, parsed, or located is treated as having no casts — silent. + * default. + * + * Both the ancestry AND the trait use-chain are walked — Laravel models compose + * cast maps from traits routinely, and a credential cast declared in a trait + * would otherwise silently exempt every model using it. `getTraits(true)` + * flattens traits-used-by-traits, so a cast two hops away still counts. The + * merge reproduces PHP's own member resolution: per ancestor, oldest first, + * traits then the class's own declarations — so a class-declared cast beats a + * trait-imported one, a trait-imported cast beats an inherited one, and the leaf + * beats everything. + * + * A declaring source whose PHP cannot be located or parsed does NOT silently + * count as "declares no casts". That would fail OPEN on exactly the models this + * rule exists to guard, and it makes MISSING indistinguishable from FAILED. The + * call site is reported under a separate identifier, + * `forbidCredentialCastBypass.modelSourceUnreadable`, saying the cast map is + * incomplete — regardless of the payload, because with an incomplete map the + * rule cannot claim the payload is clean. * * A cast counts as credential-bearing when its value is exactly `hashed`, * exactly `encrypted`, or begins with `encrypted:` (`encrypted:array`, @@ -136,6 +153,14 @@ * - **`upsert()`'s third argument** (the update-column list) — its column * names are VALUES, not keys, and every column named there must already * appear in the row payload this rule does read. + * - **A `DB::table('…')` builder hoisted into a variable** — `$q = + * DB::table('users'); $q->update([...]);`. The chain walk needs the + * `table('…')` literal, and the variable's TYPE is a bare + * `Illuminate\Database\Query\Builder` that carries no table name, so once + * the builder is behind a variable there is nothing left to resolve. Chain + * forms ARE covered, including `DB::connection('…')->table('…')` and any + * number of intermediate hops. This is a limitation of the query builder's + * type, not of the walk. * - **Static-magic builder entry** (`Model::where(...)->update([...])` * without larastan) — plain PHPStan cannot type `Model::__callStatic`, so * the receiver resolves to an error type and the rule declines. Consumers @@ -149,6 +174,15 @@ final class ForbidCredentialCastBypassRule implements Rule { private const string IDENTIFIER = 'forbidCredentialCastBypass.castBypassedByBuilderWrite'; + /** + * Reported when a declaring source could not be read, so the cast map is + * INCOMPLETE and the rule cannot vouch for the payload. A distinct + * identifier because MISSING and FAILED must not arrive as the same + * (silent) outcome — a consumer can suppress this one alone without + * disarming the real check. + */ + private const string UNREADABLE_IDENTIFIER = 'forbidCredentialCastBypass.modelSourceUnreadable'; + /** * Builder write verbs that ship their payload to SQL without routing * through `Model::setAttribute()`, mapped to the argument positions @@ -181,10 +215,10 @@ final class ForbidCredentialCastBypassRule implements Rule private const array CREDENTIAL_CASTS = ['hashed', 'encrypted']; /** - * Cast maps already read this run, keyed by model FQCN. A model is parsed - * once even when a hundred call sites write to it. + * Cast resolutions already computed this run, keyed by model FQCN. A model + * is parsed once even when a hundred call sites write to it. * - * @var array> + * @var array, unreadable: list}> */ private array $castCache = []; @@ -231,20 +265,25 @@ public function processNode(Node $node, Scope $scope): array return []; } - $casts = $this->credentialCastsFor($modelFqcn); - - if ($casts === []) { - return []; - } + $resolution = $this->castResolutionFor($modelFqcn); $errors = []; + // Reported REGARDLESS of the payload, and deliberately: when a + // declaring source could not be read, this rule does not know which + // columns carry a credential cast, so it cannot say the payload is + // clean. Staying silent here would be a fail-open on the one rule whose + // whole value is catching a silent plaintext write. + if ($resolution['unreadable'] !== []) { + $errors[] = $this->buildUnreadableSourceError($node, $modelFqcn, $resolution['unreadable']); + } + foreach ($this->payloadColumns($node, $scope, self::WRITE_METHODS[$method]) as $column) { - if (!array_key_exists($column, $casts)) { + if (!array_key_exists($column, $resolution['casts'])) { continue; } - $errors[] = $this->buildError($node, $modelFqcn, $column, $casts[$column], $method); + $errors[] = $this->buildError($node, $modelFqcn, $column, $resolution['casts'][$column], $method); } return $errors; @@ -455,32 +494,48 @@ private function keysOfConstantArray(ConstantArrayType $type): array /** * The model's credential-bearing casts as `column => cast`, merged across - * the ancestry with the child winning. Memoized per FQCN. + * the ancestry AND the trait use-chain, plus the list of declaring sources + * whose PHP could not be read. Memoized per FQCN. * - * @return array + * Merge order reproduces PHP's own member resolution: for each ancestor, + * oldest first, apply that ancestor's traits and then its own declarations. + * A class-declared cast therefore beats one imported from its traits, a + * trait-imported cast beats an inherited one, and the leaf beats everything. + * + * The `unreadable` list is the reason this returns a shape rather than a + * bare map: an ancestor whose source cannot be parsed yields the SAME empty + * cast set as an ancestor that genuinely declares none, and silently + * treating the two alike would make the rule fail OPEN on exactly the models + * it exists to guard. See `buildUnreadableSourceError()`. + * + * @return array{casts: array, unreadable: list} */ - private function credentialCastsFor(string $modelFqcn): array + private function castResolutionFor(string $modelFqcn): array { if (array_key_exists($modelFqcn, $this->castCache)) { return $this->castCache[$modelFqcn]; } - $this->castCache[$modelFqcn] = []; + $empty = ['casts' => [], 'unreadable' => []]; + $this->castCache[$modelFqcn] = $empty; if (!$this->reflectionProvider->hasClass($modelFqcn)) { - return []; + return $empty; } $classReflection = $this->reflectionProvider->getClass($modelFqcn); $casts = []; + $unreadable = []; - // Ancestors first so a cast redeclared on the child overwrites the - // inherited one, matching how Laravel merges the maps at runtime. foreach (array_reverse([$classReflection, ...$classReflection->getParents()]) as $ancestor) { - foreach ($this->declaredCasts($ancestor) as $column => $cast) { - $casts[$column] = $cast; + // `getTraits(true)` flattens traits-used-by-traits, so a cast + // declared two trait hops away still counts. + foreach ($ancestor->getTraits(true) as $trait) { + $this->mergeDeclaredCasts($trait, $casts, $unreadable); } + + $this->mergeDeclaredCasts($ancestor, $casts, $unreadable); } $credentialCasts = []; @@ -491,9 +546,32 @@ private function credentialCastsFor(string $modelFqcn): array } } - $this->castCache[$modelFqcn] = $credentialCasts; + $resolution = ['casts' => $credentialCasts, 'unreadable' => $unreadable]; + $this->castCache[$modelFqcn] = $resolution; + + return $resolution; + } + + /** + * Merge one declaring source's casts into `$casts`, recording the source's + * FQCN in `$unreadable` when its PHP could not be located or parsed. + * + * @param array $casts + * @param list $unreadable + */ + private function mergeDeclaredCasts(ClassReflection $source, array &$casts, array &$unreadable): void + { + $declared = $this->declaredCasts($source); + + if ($declared === null) { + $unreadable[] = $source->getName(); + + return; + } - return $credentialCasts; + foreach ($declared as $column => $cast) { + $casts[$column] = $cast; + } } private function isCredentialCast(string $cast): bool @@ -514,26 +592,30 @@ private function isCredentialCast(string $cast): bool * analyser. Both declaration forms are read: the `casts()` method's * `return [...]` statements and a `$casts` property default. * - * @return array + * Returns NULL — never an empty array — when the source cannot be located + * or parsed, so the caller can tell "declares no casts" from "we could not + * look". An empty array means the source was read and declares none. + * + * @return array|null */ - private function declaredCasts(ClassReflection $classReflection): array + private function declaredCasts(ClassReflection $classReflection): ?array { $file = $classReflection->getFileName(); if ($file === null) { - return []; + return null; } try { $stmts = $this->parser->parseFile($file); } catch (ParserErrorsException) { - return []; + return null; } $classNode = $this->findClassNode($stmts, $classReflection->getName()); if ($classNode === null) { - return []; + return null; } $casts = []; @@ -568,16 +650,17 @@ private function declaredCasts(ClassReflection $classReflection): array } /** - * Locate the class declaration for `$fqcn` among parsed statements. The - * injected parser resolves names, so `namespacedName` is populated and the - * match is exact rather than by short name. + * Locate the class-like declaration for `$fqcn` among parsed statements — + * a class OR a trait, since Laravel models routinely compose their cast map + * from traits. The injected parser resolves names, so `namespacedName` is + * populated and the match is exact rather than by short name. * * @param array $nodes */ - private function findClassNode(array $nodes, string $fqcn): ?Class_ + private function findClassNode(array $nodes, string $fqcn): ?ClassLike { foreach ($nodes as $node) { - if ($node instanceof Class_ && $node->namespacedName?->toString() === $fqcn) { + if ($node instanceof ClassLike && $node->namespacedName?->toString() === $fqcn) { return $node; } @@ -677,6 +760,27 @@ private function stringPairs(Expr $expr): array return $pairs; } + /** + * @param list $unreadable + */ + private function buildUnreadableSourceError( + MethodCall $node, + string $modelFqcn, + array $unreadable, + ): IdentifierRuleError { + $message = sprintf( + 'Cannot verify this write against %s: the PHP declaring %s could not be located or parsed, so the credential-cast map is incomplete and a hashed/encrypted column in this payload would go unreported. Fix the source, or suppress %s here if the write is known safe.', + $modelFqcn, + implode(', ', $unreadable), + self::UNREADABLE_IDENTIFIER, + ); + + return RuleErrorBuilder::message($message) + ->identifier(self::UNREADABLE_IDENTIFIER) + ->line($node->getStartLine()) + ->build(); + } + private function buildError( MethodCall $node, string $modelFqcn, diff --git a/tests/Fixtures/CredentialCastBypass/BuilderWrites.php b/tests/Fixtures/CredentialCastBypass/BuilderWrites.php index c121c38..2c03ee8 100644 --- a/tests/Fixtures/CredentialCastBypass/BuilderWrites.php +++ b/tests/Fixtures/CredentialCastBypass/BuilderWrites.php @@ -5,6 +5,7 @@ namespace App\Actions\CredentialCastBypass; use App\Models\CredentialCastBypass\ApiKey; +use App\Models\CredentialCastBypass\TraitCastModel; use App\Models\CredentialCastBypass\User; use App\Models\CredentialCastBypass\Vault; @@ -69,6 +70,16 @@ public function relationDerivedWrite(User $user): void $user->apiKeys()->update(['secret' => 'raw']); } + public function castDeclaredInATraitViaMethod(): void + { + TraitCastModel::query()->update(['trait_secret' => 'raw']); + } + + public function castDeclaredInATraitViaProperty(): void + { + TraitCastModel::query()->update(['trait_notes' => 'raw']); + } + public function twoCredentialColumnsInOnePayload(): void { User::query()->update(['password' => 'p', 'api_token' => 't']); diff --git a/tests/Fixtures/CredentialCastBypass/CleanWrites.php b/tests/Fixtures/CredentialCastBypass/CleanWrites.php index 4eb53d0..538270b 100644 --- a/tests/Fixtures/CredentialCastBypass/CleanWrites.php +++ b/tests/Fixtures/CredentialCastBypass/CleanWrites.php @@ -7,6 +7,7 @@ use App\Models\CredentialCastBypass\Article; use App\Models\CredentialCastBypass\NearMissCastModel; use App\Models\CredentialCastBypass\OverridingVault; +use App\Models\CredentialCastBypass\TraitOverriddenCastModel; use App\Models\CredentialCastBypass\User; use Illuminate\Support\Facades\DB; @@ -100,6 +101,16 @@ public function nearMissCastNames(): void NearMissCastModel::query()->update(['blob' => 'x', 'digest' => 'y']); } + /** + * The class REDECLARES the trait's `trait_secret` cast as `string`. PHP + * resolves a class-declared member over a trait-imported one, so this is no + * longer a credential column. + */ + public function classDeclarationBeatsTraitImportedCast(): void + { + TraitOverriddenCastModel::query()->update(['trait_secret' => 'plain']); + } + /** * A payload of unknown shape — keys are not statically known. */ diff --git a/tests/Fixtures/CredentialCastBypass/ComposesHashedSecret.php b/tests/Fixtures/CredentialCastBypass/ComposesHashedSecret.php new file mode 100644 index 0000000..41c9c25 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/ComposesHashedSecret.php @@ -0,0 +1,14 @@ + */ + protected $casts = [ + 'trait_notes' => 'encrypted', + ]; +} diff --git a/tests/Fixtures/CredentialCastBypass/HasHashedSecret.php b/tests/Fixtures/CredentialCastBypass/HasHashedSecret.php new file mode 100644 index 0000000..cfab058 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/HasHashedSecret.php @@ -0,0 +1,24 @@ + + */ + protected function casts(): array + { + return [ + 'trait_secret' => 'hashed', + ]; + } +} diff --git a/tests/Fixtures/CredentialCastBypass/RawTableWrites.php b/tests/Fixtures/CredentialCastBypass/RawTableWrites.php index ab1a76d..229b2c4 100644 --- a/tests/Fixtures/CredentialCastBypass/RawTableWrites.php +++ b/tests/Fixtures/CredentialCastBypass/RawTableWrites.php @@ -28,6 +28,23 @@ public function mappedTableNonCastColumn(): void DB::table('users')->update(['login_count' => 1]); } + public function connectionScopedTableWrite(): void + { + DB::connection('mysql')->table('users')->update(['password' => 'plaintext']); + } + + /** + * Hoisting the builder into a variable defeats the chain walk — the + * variable's TYPE is a bare `Query\Builder` carrying no table name, so + * there is nothing left to resolve. Documented, tested silence. + */ + public function hoistedTableBuilder(): void + { + $query = DB::table('users'); + + $query->update(['password' => 'plaintext']); + } + public function unmappedTable(): void { DB::table('articles')->update(['password' => 'plaintext']); diff --git a/tests/Fixtures/CredentialCastBypass/SingleUncastWrite.php b/tests/Fixtures/CredentialCastBypass/SingleUncastWrite.php new file mode 100644 index 0000000..cfe7bbc --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/SingleUncastWrite.php @@ -0,0 +1,20 @@ +update(['published_at' => 'now']); + } +} diff --git a/tests/Fixtures/CredentialCastBypass/TraitCastModel.php b/tests/Fixtures/CredentialCastBypass/TraitCastModel.php new file mode 100644 index 0000000..438239b --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/TraitCastModel.php @@ -0,0 +1,19 @@ + + */ + protected function casts(): array + { + return [ + 'trait_secret' => 'string', + ]; + } +} diff --git a/tests/Rules/ForbidCredentialCastBypassRuleTest.php b/tests/Rules/ForbidCredentialCastBypassRuleTest.php index 93280d9..d300082 100644 --- a/tests/Rules/ForbidCredentialCastBypassRuleTest.php +++ b/tests/Rules/ForbidCredentialCastBypassRuleTest.php @@ -8,6 +8,7 @@ use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; use ScriptDevelopment\PhpstanWarroomRules\Rules\ForbidCredentialCastBypassRule; +use ScriptDevelopment\PhpstanWarroomRules\Tests\Support\ThrowingParser; use function dirname; use function file_get_contents; @@ -28,6 +29,10 @@ final class ForbidCredentialCastBypassRuleTest extends RuleTestCase private const string RAW_TABLE_WRITES = __DIR__ . '/../Fixtures/CredentialCastBypass/RawTableWrites.php'; + private const string SINGLE_UNCAST_WRITE = __DIR__ . '/../Fixtures/CredentialCastBypass/SingleUncastWrite.php'; + + private const string UNREADABLE_MESSAGE = 'Cannot verify this write against %s: the PHP declaring %s could not be located or parsed, so the credential-cast map is incomplete and a hashed/encrypted column in this payload would go unreported. Fix the source, or suppress forbidCredentialCastBypass.modelSourceUnreadable here if the write is known safe.'; + /** * Table-to-model map used by the `DB::table()` tests. Left null so the * default (empty map, `DB::table()` silent) is what every other test sees. @@ -36,23 +41,33 @@ final class ForbidCredentialCastBypassRuleTest extends RuleTestCase */ private ?array $tableModelOverride = null; + /** + * When set, `getRule()` injects a parser that throws for any file whose path + * ends with this suffix — the only way to exercise the unreadable-source + * branch without shipping a syntactically broken fixture (which would break + * the classmap for the whole suite). + */ + private ?string $unparsableFileSuffix = null; + // ---------------------------------------------------------------- RED --- public function testHashedCastColumnInBuilderUpdateIsFlagged(): void { $this->analyse([self::BUILDER_WRITES], [ - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 19], - [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 24], - [sprintf(self::MESSAGE, 'recovery_codes', 'App\Models\CredentialCastBypass\User', 'encrypted:array', 'update'), 29], - [sprintf(self::MESSAGE, 'secret', 'App\Models\CredentialCastBypass\ApiKey', 'encrypted', 'update'), 34], - [sprintf(self::MESSAGE, 'passphrase', 'App\Models\CredentialCastBypass\Vault', 'hashed', 'update'), 39], - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 46], - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'insert'), 51], - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'upsert'), 59], - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'updateOrInsert'), 64], - [sprintf(self::MESSAGE, 'secret', 'App\Models\CredentialCastBypass\ApiKey', 'encrypted', 'update'), 69], - [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 74], - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 74], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 20], + [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 25], + [sprintf(self::MESSAGE, 'recovery_codes', 'App\Models\CredentialCastBypass\User', 'encrypted:array', 'update'), 30], + [sprintf(self::MESSAGE, 'secret', 'App\Models\CredentialCastBypass\ApiKey', 'encrypted', 'update'), 35], + [sprintf(self::MESSAGE, 'passphrase', 'App\Models\CredentialCastBypass\Vault', 'hashed', 'update'), 40], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 47], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'insert'), 52], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'upsert'), 60], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'updateOrInsert'), 65], + [sprintf(self::MESSAGE, 'secret', 'App\Models\CredentialCastBypass\ApiKey', 'encrypted', 'update'), 70], + [sprintf(self::MESSAGE, 'trait_secret', 'App\Models\CredentialCastBypass\TraitCastModel', 'hashed', 'update'), 75], + [sprintf(self::MESSAGE, 'trait_notes', 'App\Models\CredentialCastBypass\TraitCastModel', 'encrypted', 'update'), 80], + [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 85], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 85], ]); } @@ -63,6 +78,7 @@ public function testRawTableWriteIsFlaggedOnlyForMappedTables(): void $this->analyse([self::RAW_TABLE_WRITES], [ [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 18], [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 23], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 33], ]); } @@ -78,6 +94,37 @@ public function testRawTableWritesAreSilentWithTheDefaultEmptyMap(): void $this->analyse([self::RAW_TABLE_WRITES], []); } + /** + * crit round 1, issue 3. A declaring source that cannot be parsed yields the + * same empty cast set as a model that genuinely declares none, so treating + * the two alike makes the rule fail OPEN — silently passing every write to a + * model it can no longer read. Doctrine requires MISSING to be a distinct + * outcome from FAILED, so this reports under its own identifier. + * + * Both directions in ONE fixture: the same write site is silent under a + * working parser (below) and loud when the model's PHP cannot be read. + */ + public function testUnreadableModelSourceIsReportedRatherThanSilentlySkipped(): void + { + $this->unparsableFileSuffix = 'CredentialCastBypass/Article.php'; + + $this->analyse([self::SINGLE_UNCAST_WRITE], [ + [ + sprintf( + self::UNREADABLE_MESSAGE, + 'App\Models\CredentialCastBypass\Article', + 'App\Models\CredentialCastBypass\Article', + ), + 18, + ], + ]); + } + + public function testTheSameWriteIsSilentWhenTheModelSourceParses(): void + { + $this->analyse([self::SINGLE_UNCAST_WRITE], []); + } + // ---------------------------------------------------------- DENOMINATOR --- /** @@ -97,9 +144,9 @@ public function testFixturePopulationIsNonZero(): void return preg_match_all('/(->|::)(update|insert|insertOrIgnore|insertGetId|upsert|updateOrInsert|create|updateOrCreate|save)\(/', $source); }; - self::assertGreaterThanOrEqual(11, $writeCalls(self::BUILDER_WRITES), 'The violating fixture lost write sites.'); - self::assertGreaterThanOrEqual(9, $writeCalls(self::CLEAN_WRITES), 'The clean fixture lost write sites, so its zero proves nothing.'); - self::assertGreaterThanOrEqual(5, $writeCalls(self::RAW_TABLE_WRITES), 'The raw-table fixture lost write sites.'); + self::assertGreaterThanOrEqual(13, $writeCalls(self::BUILDER_WRITES), 'The violating fixture lost write sites.'); + self::assertGreaterThanOrEqual(12, $writeCalls(self::CLEAN_WRITES), 'The clean fixture lost write sites, so its zero proves nothing.'); + self::assertGreaterThanOrEqual(7, $writeCalls(self::RAW_TABLE_WRITES), 'The raw-table fixture lost write sites.'); } /** @@ -123,6 +170,11 @@ public function testEveryFixtureModelResolvesToThisRuleSOwnFixtureDirectory(): v 'App\Models\CredentialCastBypass\OverridingVault', 'App\Models\CredentialCastBypass\NearMissCastModel', 'App\Models\CredentialCastBypass\AbstractCredentialHolder', + 'App\Models\CredentialCastBypass\TraitCastModel', + 'App\Models\CredentialCastBypass\TraitOverriddenCastModel', + 'App\Models\CredentialCastBypass\HasHashedSecret', + 'App\Models\CredentialCastBypass\HasEncryptedNotesProperty', + 'App\Models\CredentialCastBypass\ComposesHashedSecret', ]; $reflectionProvider = self::createReflectionProvider(); @@ -153,6 +205,10 @@ protected function getRule(): Rule self::assertInstanceOf(Parser::class, $parser); + if ($this->unparsableFileSuffix !== null) { + $parser = new ThrowingParser($parser, $this->unparsableFileSuffix); + } + return new ForbidCredentialCastBypassRule( self::createReflectionProvider(), $parser, diff --git a/tests/Support/ThrowingParser.php b/tests/Support/ThrowingParser.php new file mode 100644 index 0000000..c915200 --- /dev/null +++ b/tests/Support/ThrowingParser.php @@ -0,0 +1,46 @@ + + */ + public function parseFile(string $file): array + { + if (str_ends_with($file, $this->failingFileSuffix)) { + throw new ParserErrorsException([new Error('Simulated parse failure')], $file); + } + + return $this->inner->parseFile($file); + } + + /** + * @return array + */ + public function parseString(string $sourceCode): array + { + return $this->inner->parseString($sourceCode); + } +} From 9cc3a111dd692047000c96d61a6ab9154b7bb3e6 Mon Sep 17 00:00:00 2001 From: Gerard Oosterhof Date: Mon, 31 Aug 2026 15:07:40 +0200 Subject: [PATCH 3/3] fix(queue-217): read composed cast maps, and report the two fail-open shapes crit found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01TzmX4F7niaC3Y411TcgFkh --- CHANGELOG.md | 2 + CLAUDE.md | 2 +- README.md | 32 ++- src/Rules/ForbidCredentialCastBypassRule.php | 255 +++++++++++++++--- .../CredentialCastBypass/BuilderWrites.php | 23 ++ .../CredentialCastBypass/CleanWrites.php | 22 ++ .../ComposedCastModel.php | 28 ++ .../ConstantCastModel.php | 32 +++ .../ConstantCastPropertyModel.php | 26 ++ .../NestedLiteralCastModel.php | 45 ++++ .../CredentialCastBypass/SpreadCastModel.php | 24 ++ .../UninterpretableCastWrites.php | 28 ++ .../ForbidCredentialCastBypassRuleTest.php | 114 ++++++-- 13 files changed, 571 insertions(+), 62 deletions(-) create mode 100644 tests/Fixtures/CredentialCastBypass/ComposedCastModel.php create mode 100644 tests/Fixtures/CredentialCastBypass/ConstantCastModel.php create mode 100644 tests/Fixtures/CredentialCastBypass/ConstantCastPropertyModel.php create mode 100644 tests/Fixtures/CredentialCastBypass/NestedLiteralCastModel.php create mode 100644 tests/Fixtures/CredentialCastBypass/SpreadCastModel.php create mode 100644 tests/Fixtures/CredentialCastBypass/UninterpretableCastWrites.php diff --git a/CHANGELOG.md b/CHANGELOG.md index a6342b2..88ea633 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and **Review round 1 (crit, PR #68 — three issues, all fixed in a follow-up commit).** (1) **Trait-declared casts were invisible.** Laravel models compose cast maps from traits routinely, and the resolution walked only the class ancestry — so a `hashed` cast declared in a trait silently exempted every model using it, on a rule whose whole purpose is catching a silent plaintext write. The walk now covers the trait use-chain via `getTraits(true)` (flattening traits-of-traits) in PHP's own member-resolution order: per ancestor, oldest first, traits then the class's own declarations, so class-declared beats trait-imported beats inherited. (2) **`DB::table()` hoisted into a variable** defeats the chain walk. Fixed as far as it can be — `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 `Illuminate\Database\Query\Builder` which carries no table name, so there is nothing to read. Now documented and pinned by a fixture rather than 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. `declaredCasts()` now returns `null` for "could not look" as distinct from `[]` for "looked, found none", and the call site is reported under a **separate identifier `forbidCredentialCastBypass.modelSourceUnreadable`** — regardless of the payload, because with an incomplete map the rule cannot claim the payload is clean. Each fix is teeth-proved in both directions, including a fixture pinning the trait-vs-class precedence DIRECTION and a test that injects a parser failing on exactly one model file (a syntactically broken fixture would break the suite's classmap rather than the branch under test). + **Review round 2 (crit, PR #68 — two issues, both fail-open false negatives, both fixed).** (1) **`declaredCasts()` read only LITERAL cast arrays.** `returnedArrays()` collected a `return` statement 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 with a positive control 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 (never from *inside* an already-collected array, so a nested-array cast value stays a value), 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;`, `return $this->buildCasts();`, `protected $casts = self::CASTS;`) is read, so `modelSourceUnreadable` never fired — yet the map is incomplete. That is now its own identifier, **`forbidCredentialCastBypass.castMapIncomplete`**, reported regardless of payload with a remediation of its own (restate the credential columns literally). 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()` returning 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 — the exact fail-open shape the unreadable identifier exists to prevent, arriving through configuration rather than source. Now **`forbidCredentialCastBypass.configuredModelMissing`**, reachable only from the config map (an FQCN taken from a resolved generic always exists). All three teeth-proved by mutation: reverting each guard individually reds exactly the test that pins it, and reverting the composed-literal collection additionally 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: a composition mixing a literal with a *dynamic* contributor reads the literal half and stays silent about the rest. + **Versioning: candidate MAJOR** — it surfaces new errors in code that previously passed wherever a consumer writes a credential column through a builder. Per the pre-1.0 caret convention `^0.8` excludes the next minor, so tagging auto-adopts nobody; each consumer adopts on its own pin-bump PR. **NOT tagged** (release is a General/Commander step). - `ForbidUntimedHttpClientRule` — new rule enforcing war-room **Architectural Principle #8** (explicit timeouts on outbound HTTP) at analysis time, the AST-aware successor to the per-territory `ExternalHttpTimeoutTest` named-list Pest tests (kendo / emmie). The named-list tests detect wrong-shape on *enrolled* classes but are blind to **omission** — a new untimed call nobody adds to the list; this rule closes the omission gap for the tractable call shapes. Registered in `extension.neon` (no parameters). Identifier: `forbidUntimedHttpClient.missingTimeout`. Doctrine: war-room §Architectural Principles #8. Seed: war-room enforcement queue #58 (spike branch `spike/wr-queue58-untimed-http-client`). **Review follow-up (bus #57 findings):** the `withOptions()` check is TYPE-aware, not AST-literal — a constant array type provably lacking `'timeout'` still fires (including through a variable holding a literal array, a widening over the inline-`Array_`-only first cut), while a computed/helper-built options expression (not a constant array type) is POSSIBLY timed and the chain DECLINES (the Major: flagging it was a false positive). A chain member outside the known `PendingRequest` builder surface — a Macroable extension (`Http::github()`, root or intermediate) or `when()`/`unless()` with their opaque closures — likewise declines (the Minor: a macro may return a pre-timed request); a genuine builder missing from the list costs only a false negative. diff --git a/CLAUDE.md b/CLAUDE.md index 0e041f1..0095172 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -38,7 +38,7 @@ Composer package distributing war-room-doctrine PHPStan rules across `script-dev | `ForbidUntimedHttpClientRule` | War-room §Explicit HTTP timeouts (#8) | `forbidUntimedHttpClient.missingTimeout` (type-aware; flags an `Http` facade OR injected `Illuminate\Http\Client\Factory` chain that reaches a send verb with no explicit `->timeout()` / `withOptions(['timeout'])`. Conservative — fires only on fully-visible single-expression chains; declines split/helper-built chains + Guzzle/SDK surfaces to hold FP at zero. COMPLEMENTS, does not replace, the per-territory `ExternalHttpTimeoutTest`. on `main`, `[Unreleased]`) | | `EnforceAuditModelProtectionsRule` | ADR-0001 §Append-only | `enforceAuditModelProtections.hasFactoryForbidden` / `.softDeletesForbidden` / `.updatedAtNotDisabled` (denylist-inversion; discovers audit models by shape — `auditModelNameSuffixes` default `AuditLog` OR `auditModelNamespacePrefixes` default `App\Models\Audit` — and flags `HasFactory` / `SoftDeletes` / missing `const UPDATED_AT = null`. shipped v0.7.0) | | `EnforceActionResultDtoRule` | ADR-0020 + ADR-0011 | `enforceActionResultDto.arrayReturnFromExecute` (signature-only; flags an `array` / `?array` / `array\|Dto` union / `iterable` native return type on `App\Actions\*` `execute()`. Phpdoc-only `@return array{...}` is a deliberate miss; no `list` carve-out. Seed kendo PR #1653. shipped v0.8.0) | -| `ForbidCredentialCastBypassRule` | War-room §Explicit over implicit (#1) + §Rotation-invariant credential handling (#10) | `forbidCredentialCastBypass.castBypassedByBuilderWrite` / `.modelSourceUnreadable` (flags a `hashed` / `encrypted` / `encrypted:*` cast column appearing as a key in a QUERY-BUILDER write payload — `update`/`insert`/`insertOrIgnore`/`insertGetId`/`upsert`/`updateOrInsert` on an Eloquent `Builder`, query `Builder` or `Relation`. Casts fire on the model path only, so a builder write stores the credential in plaintext with a green suite. The model path and the model-routing builder verbs (`create`/`updateOrCreate`/`firstOrCreate`/`createOrFirst`) are silent STRUCTURALLY — the receiver type gate, not an exemption list. Model comes from the builder/relation generic; `DB::table('…')` carries none and resolves only via the opt-in `credentialCastTableModels` map, default `[]`. Cast maps read from model SOURCE via the injected `@defaultAnalysisParser` — both `casts()` and `$casts`, merged across the ancestry AND the trait use-chain (class-declared beats trait-imported beats inherited). A source whose PHP cannot be read is NOT treated as castless — that fails open — but reported under the second identifier `forbidCredentialCastBypass.modelSourceUnreadable`. Payload keys read from the CONSTANT ARRAY TYPE, so a hoisted variable is caught. Seed lokalekeuze PR #65. on `main`, `[Unreleased]`) | +| `ForbidCredentialCastBypassRule` | War-room §Explicit over implicit (#1) + §Rotation-invariant credential handling (#10) | `forbidCredentialCastBypass.castBypassedByBuilderWrite` / `.modelSourceUnreadable` / `.castMapIncomplete` / `.configuredModelMissing` (flags a `hashed` / `encrypted` / `encrypted:*` cast column appearing as a key in a QUERY-BUILDER write payload — `update`/`insert`/`insertOrIgnore`/`insertGetId`/`upsert`/`updateOrInsert` on an Eloquent `Builder`, query `Builder` or `Relation`. Casts fire on the model path only, so a builder write stores the credential in plaintext with a green suite. The model path and the model-routing builder verbs (`create`/`updateOrCreate`/`firstOrCreate`/`createOrFirst`) are silent STRUCTURALLY — the receiver type gate, not an exemption list. Model comes from the builder/relation generic; `DB::table('…')` carries none and resolves only via the opt-in `credentialCastTableModels` map, default `[]`. Cast maps read from model SOURCE via the injected `@defaultAnalysisParser` — both `casts()` and `$casts`, merged across the ancestry AND the trait use-chain (class-declared beats trait-imported beats inherited). Composed maps are read as well — the literal inside `array_merge(parent::casts(), …)` and an array spread both contribute. THREE fail-open shapes are each reported under their own identifier rather than reading as castless: `.modelSourceUnreadable` (source cannot be located/parsed), `.castMapIncomplete` (read, but a declaration carries no array literal at all — `return self::CASTS;`), `.configuredModelMissing` (`credentialCastTableModels` names a nonexistent class). Payload keys read from the CONSTANT ARRAY TYPE, so a hoisted variable is caught. Seed lokalekeuze PR #65. on `main`, `[Unreleased]`) | | `ConnectionTransactionReturnTypeExtension` | (type extension, no rule) | — (resolves `ConnectionInterface::transaction()` to the closure's return type. **Load-bearing on `illuminate/* ^12` only** — it annotates `@return mixed`; Laravel 13 annotates `@template TReturn`/`@return TReturn` and infers the same type unaided. Its teeth are measured by the `check-lowest-laravel` CI job, not by the unit suite on a Laravel-13 tree. WR-0855, WR-0860) | Phase 2 expands the rule set: `EnforceAuditSnapshotOnRetryRule` (ADR-0001 §Snapshot-on-Retry Safety) was the first Phase 2 addition, promoted from cross-territory Pest arch tests (emmie PR #187, entreezuil PR #139, ublgenie PR #166, kendo PR #1029). `EnforceResourceDataValidatorOptInRule` (ADR-0009 §EAGER_LOAD validator opt-in) is the second Phase 2 addition, promoted from kendo PR #1084 under war-room enforcement queue #55. `EnforceFormRequestToDtoRule` (ADR-0012) is the third Phase 2 addition, promoted from entreezuil's `tests/Arch/FormRequestsTest.php` under the same queue #55 (instance 2). `EnforceExplicitHydrationRule` (ADR-0019) is the next Phase 2 candidate. diff --git a/README.md b/README.md index 3ea307e..b3bc7aa 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ includes: | `EnforceCurrentUserAttributeRule` | `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser` | `Request::user()` / `Auth::user()` / `auth()->user()` calls inside `App\Http\Controllers\*` classes (namespace prefix, incl. sub-namespaces; configurable via `controllerNamespacePrefixes`) | Use `#[\Illuminate\Container\Attributes\CurrentUser] User $user` on the method parameter. Scope is decided by namespace, not class ancestry — a base-less `final` controller in `App\Http\Controllers` fires; FormRequests (`App\Http\Requests`), middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs, and console commands are silent because their namespaces do not start with the controller prefix (container-attribute injection does not apply to FormRequest methods regardless). | | `EnforceCurrentUserAttributeRule` | `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser` | `Request::user()` / `Auth::user()` / `auth()->user()` calls inside `App\Http\Controllers\*` classes (namespace prefix, incl. sub-namespaces) | Use `#[\Illuminate\Container\Attributes\CurrentUser] User $user` on the method parameter. Scope is decided by namespace, not class ancestry — a base-less `final` controller in `App\Http\Controllers` fires; FormRequests (`App\Http\Requests`), middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs, and console commands are silent because their namespaces do not start with the controller prefix (container-attribute injection does not apply to FormRequest methods regardless). | | `EnforceAuditModelProtectionsRule` | `enforceAuditModelProtections.hasFactoryForbidden` / `.softDeletesForbidden` / `.updatedAtNotDisabled` | Eloquent models recognised as audit records by SHAPE — short name ends with a configured suffix (default `AuditLog`) OR FQCN sits under a configured namespace (default `App\Models\Audit`) | Three append-only protections, each firing independently: using `HasFactory` (a factory is a direct-insert path bypassing the hash-chained writer), using `SoftDeletes` (audit rows are never removed), or not disabling `updated_at` (an audit row is written once and never mutated — declare `public const UPDATED_AT = null;`) is an error. Discovery is by pattern, never a hand-maintained class list — a denylist inversion, so a newly-added audit model cannot escape the protections by omission. Abstract intermediates are exempt (their concrete leaves carry inherited violations). Non-model classes named `*AuditLog` are excluded by the Eloquent `Model` type gate. Doctrine: ADR-0001 §Append-only. | -| `ForbidCredentialCastBypassRule` | `forbidCredentialCastBypass.castBypassedByBuilderWrite` | Query-builder write calls (`update`, `insert`, `insertOrIgnore`, `insertGetId`, `upsert`, `updateOrInsert`) whose receiver is an `Illuminate\Database\Eloquent\Builder`, `Illuminate\Database\Query\Builder`, or `Illuminate\Database\Eloquent\Relations\Relation` | Naming a column that carries a `hashed`, `encrypted` or `encrypted:*` cast as a key in the write payload is an error. Casts fire on the MODEL path only; a builder write delegates to `toBase()->update()` and ships the raw value to SQL — no hash, no encryption, no exception, and a green test suite. The model path is **structurally** silent (a `Model` receiver never matches), and so are the builder methods that route through a model (`create`, `updateOrCreate`, `firstOrCreate`, `createOrFirst`) — they are the remediation. The model comes from the builder's/relation's generic type argument; a `DB::table('…')` chain carries none and resolves only through the opt-in `credentialCastTableModels` map (empty by default ⇒ silent, never inferred from the table name). The cast map is read from the model SOURCE — both the `casts()` method and a `$casts` property, merged across the ancestry **and the trait use-chain** (class-declared beats trait-imported beats inherited; `getTraits(true)` flattens traits-of-traits). A declaring source whose PHP cannot be located or parsed is NOT treated as "declares no casts" — that would fail open — and is reported under the separate identifier `forbidCredentialCastBypass.modelSourceUnreadable`. Payload keys are read from the resolved **constant array type**, so a payload hoisted into a variable is caught and a dynamic payload is silent. Deliberate misses: class-based casts (`AsEncryptedArrayObject::class`), dynamic keys, `upsert()`'s third argument, `Model::where(...)` static-magic entry without larastan, and a `DB::table()` builder hoisted into a variable (the variable's type carries no table name). Doctrine: war-room §Architectural Principles #1 + #10; ISO 27001 A.5.33 / AVG. Seed: lokalekeuze PR #65. | +| `ForbidCredentialCastBypassRule` | `forbidCredentialCastBypass.castBypassedByBuilderWrite` | Query-builder write calls (`update`, `insert`, `insertOrIgnore`, `insertGetId`, `upsert`, `updateOrInsert`) whose receiver is an `Illuminate\Database\Eloquent\Builder`, `Illuminate\Database\Query\Builder`, or `Illuminate\Database\Eloquent\Relations\Relation` | Naming a column that carries a `hashed`, `encrypted` or `encrypted:*` cast as a key in the write payload is an error. Casts fire on the MODEL path only; a builder write delegates to `toBase()->update()` and ships the raw value to SQL — no hash, no encryption, no exception, and a green test suite. The model path is **structurally** silent (a `Model` receiver never matches), and so are the builder methods that route through a model (`create`, `updateOrCreate`, `firstOrCreate`, `createOrFirst`) — they are the remediation. The model comes from the builder's/relation's generic type argument; a `DB::table('…')` chain carries none and resolves only through the opt-in `credentialCastTableModels` map (empty by default ⇒ silent, never inferred from the table name). The cast map is read from the model SOURCE — both the `casts()` method and a `$casts` property, merged across the ancestry **and the trait use-chain** (class-declared beats trait-imported beats inherited; `getTraits(true)` flattens traits-of-traits). Composed maps are read too: the literal inside `array_merge(parent::casts(), [...])` and an array spread both contribute. Three fail-open shapes each get their OWN identifier rather than reading as "declares no casts" — `forbidCredentialCastBypass.modelSourceUnreadable` (source cannot be located or parsed), `.castMapIncomplete` (source read, but a declaration carries no array literal at all — `return self::CASTS;`), and `.configuredModelMissing` (`credentialCastTableModels` names a class that does not exist). Payload keys are read from the resolved **constant array type**, so a payload hoisted into a variable is caught and a dynamic payload is silent. Deliberate misses: class-based casts (`AsEncryptedArrayObject::class`), dynamic keys, `upsert()`'s third argument, `Model::where(...)` static-magic entry without larastan, and a `DB::table()` builder hoisted into a variable (the variable's type carries no table name). Doctrine: war-room §Architectural Principles #1 + #10; ISO 27001 A.5.33 / AVG. Seed: lokalekeuze PR #65. | ### `EnforceActionTransactionsRule` — write-method list @@ -268,21 +268,31 @@ The walk needs the `table('…')` string literal, and the variable's type is a b left to read. This is a limitation of the query builder's type rather than of the walk, and it is a false negative, never a false positive. -### `ForbidCredentialCastBypassRule` — when the model source cannot be read +### `ForbidCredentialCastBypassRule` — when the cast map cannot be read in full -If a declaring class or trait's PHP cannot be located or parsed, the cast map is -incomplete and the rule cannot vouch for the payload. Rather than pass silently — -which would fail open on exactly the models it exists to guard — it reports the -call site under its own identifier: +Three different things can stop the rule from reading a complete cast map, and +each reports under its own identifier — MISSING, FAILED and MISCONFIGURED must +not arrive as the same silent outcome, and the remediation differs: -``` -forbidCredentialCastBypass.modelSourceUnreadable -``` +| Identifier | Cause | Fix | +|---|---|---| +| `forbidCredentialCastBypass.modelSourceUnreadable` | a declaring class or trait's PHP cannot be located or parsed | fix the source | +| `forbidCredentialCastBypass.castMapIncomplete` | the source was read, but a `casts()` return or a `$casts` default carries no array literal at all (`return self::CASTS;`, `return $this->buildCasts();`) | restate the credential columns as literal string pairs | +| `forbidCredentialCastBypass.configuredModelMissing` | `credentialCastTableModels` maps a table to a class that does not exist (a typo, or a stale rename) | fix the FQCN, or drop the mapping | -This is deliberately independent of the payload: with an incomplete map, a -credential column in that payload would go unreported. Suppress this identifier +All three are deliberately independent of the payload: with an incomplete map, a +credential column in that payload would go unreported. Suppress an identifier alone (per file or per line) if a write is known safe; the real check stays armed. +**Composed cast maps are read, not reported.** +`return array_merge(parent::casts(), ['password' => 'hashed']);` and +`return [...parent::casts(), 'password' => 'hashed'];` both contribute their +literal, and the ancestor being merged in is walked separately, so the merged map +is complete and neither form triggers `castMapIncomplete`. 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. + ### `ForbidRawExceptionMessageInResponseRule` — configurable sinks + `@leak-safe` exemption The rule flags a raw `Throwable::getMessage()` (or the `Throwable` itself) reaching a **client-facing response sink** — an information-disclosure leak. The built-in default sink `Laravel\Mcp\Response::error` is always armed; a consumer adds more (a persist-error setter, a `MarkInvoiceFailed` Action) via the `rawExceptionMessageSinks` parameter — a list of `FQCN::method` signatures, default `[]`: diff --git a/src/Rules/ForbidCredentialCastBypassRule.php b/src/Rules/ForbidCredentialCastBypassRule.php index 7027352..603bb51 100644 --- a/src/Rules/ForbidCredentialCastBypassRule.php +++ b/src/Rules/ForbidCredentialCastBypassRule.php @@ -12,6 +12,7 @@ use PhpParser\Node\Expr; use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\StaticCall; +use PhpParser\Node\FunctionLike; use PhpParser\Node\Identifier; use PhpParser\Node\Scalar\String_; use PhpParser\Node\Stmt\Class_; @@ -126,13 +127,31 @@ * trait-imported one, a trait-imported cast beats an inherited one, and the leaf * beats everything. * - * A declaring source whose PHP cannot be located or parsed does NOT silently - * count as "declares no casts". That would fail OPEN on exactly the models this - * rule exists to guard, and it makes MISSING indistinguishable from FAILED. The - * call site is reported under a separate identifier, - * `forbidCredentialCastBypass.modelSourceUnreadable`, saying the cast map is - * incomplete — regardless of the payload, because with an incomplete map the - * rule cannot claim the payload is clean. + * A cast map composed rather than returned literally is READ, not missed: + * `return array_merge(parent::casts(), ['password' => 'hashed']);` and + * `return [...parent::casts(), 'password' => 'hashed'];` both contribute their + * literal, and the ancestor being merged in is walked separately, so the merged + * map is complete. Array literals are collected from anywhere inside a returned + * expression — but never from inside an already-collected array, so a + * nested-array cast value stays a value rather than becoming a second cast map. + * + * Three failure modes are each reported under their OWN identifier, because + * MISSING, FAILED and MISCONFIGURED must not arrive as the same (silent) + * outcome, and each has a different remediation: + * + * - **`…modelSourceUnreadable`** — a declaring source whose PHP cannot be + * located or parsed. Fix the source. + * - **`…castMapIncomplete`** — the source WAS read, but a `casts()` return or + * a `$casts` default carries no array literal at all (`return self::CASTS;`, + * `return $this->buildCasts();`). Restate the credential columns literally. + * - **`…configuredModelMissing`** — `credentialCastTableModels` maps a table + * to a class that does not exist. Fix the parameter. Reachable only from the + * config map: an FQCN taken from a resolved generic type always exists. + * + * All three are reported REGARDLESS of the payload, because with an incomplete + * map the rule cannot claim the payload is clean. Treating any of them as + * "declares no casts" would fail OPEN on exactly the models this rule exists to + * guard, and would make MISSING indistinguishable from FAILED. * * A cast counts as credential-bearing when its value is exactly `hashed`, * exactly `encrypted`, or begins with `encrypted:` (`encrypted:array`, @@ -167,6 +186,12 @@ * running larastan get `Builder` there and the rule fires normally; * `Model::query()->…` resolves on plain PHPStan either way. * - **Raw SQL** (`DB::update('update users set …')`) — no payload array. + * - **A composition mixing a literal with a dynamic contributor** — + * `return array_merge($this->dynamicCasts(), ['password' => 'hashed']);`. + * The literal IS read, so the map is not reported as incomplete, and + * whatever the dynamic half contributes stays invisible. Reporting here + * would mean flagging every model that composes at all, including the ones + * read in full; the literal half being covered is the honest limit. * * @implements Rule */ @@ -183,6 +208,29 @@ final class ForbidCredentialCastBypassRule implements Rule */ private const string UNREADABLE_IDENTIFIER = 'forbidCredentialCastBypass.modelSourceUnreadable'; + /** + * Reported when a declaring source WAS read but one of its cast + * declarations could not be interpreted — a `casts()` return or a `$casts` + * default that contributes no array literal at all (`return self::CASTS;`, + * `return $this->buildCasts();`). Distinct from UNREADABLE_IDENTIFIER + * because the remediation is different: the file is fine, the declaration + * shape is what this rule cannot read, and restating the credential columns + * in literal form fixes it. Composition forms that DO carry a literal + * (`array_merge(parent::casts(), [...])`, `[...parent::casts(), ...]`) are + * read, not reported. + */ + private const string INCOMPLETE_IDENTIFIER = 'forbidCredentialCastBypass.castMapIncomplete'; + + /** + * Reported when `credentialCastTableModels` maps a table to a class that + * does not exist. A typo or a stale rename would otherwise be + * indistinguishable from "this table is not mapped", which silently and + * permanently disarms the rule for every write on that table — the same + * fail-open shape UNREADABLE_IDENTIFIER exists to prevent, arriving through + * the configuration instead of the source. + */ + private const string CONFIG_IDENTIFIER = 'forbidCredentialCastBypass.configuredModelMissing'; + /** * Builder write verbs that ship their payload to SQL without routing * through `Model::setAttribute()`, mapped to the argument positions @@ -218,7 +266,7 @@ final class ForbidCredentialCastBypassRule implements Rule * Cast resolutions already computed this run, keyed by model FQCN. A model * is parsed once even when a hundred call sites write to it. * - * @var array, unreadable: list}> + * @var array, unreadable: list, incomplete: list, missing: bool}> */ private array $castCache = []; @@ -274,10 +322,18 @@ public function processNode(Node $node, Scope $scope): array // columns carry a credential cast, so it cannot say the payload is // clean. Staying silent here would be a fail-open on the one rule whose // whole value is catching a silent plaintext write. + if ($resolution['missing']) { + $errors[] = $this->buildConfiguredModelMissingError($node, $modelFqcn); + } + if ($resolution['unreadable'] !== []) { $errors[] = $this->buildUnreadableSourceError($node, $modelFqcn, $resolution['unreadable']); } + if ($resolution['incomplete'] !== []) { + $errors[] = $this->buildIncompleteCastMapError($node, $modelFqcn, $resolution['incomplete']); + } + foreach ($this->payloadColumns($node, $scope, self::WRITE_METHODS[$method]) as $column) { if (!array_key_exists($column, $resolution['casts'])) { continue; @@ -508,7 +564,7 @@ private function keysOfConstantArray(ConstantArrayType $type): array * treating the two alike would make the rule fail OPEN on exactly the models * it exists to guard. See `buildUnreadableSourceError()`. * - * @return array{casts: array, unreadable: list} + * @return array{casts: array, unreadable: list, incomplete: list, missing: bool} */ private function castResolutionFor(string $modelFqcn): array { @@ -516,26 +572,35 @@ private function castResolutionFor(string $modelFqcn): array return $this->castCache[$modelFqcn]; } - $empty = ['casts' => [], 'unreadable' => []]; + $empty = ['casts' => [], 'unreadable' => [], 'incomplete' => [], 'missing' => false]; $this->castCache[$modelFqcn] = $empty; + // The class cannot be absent on the GENERIC path — that FQCN came out + // of a resolved type — so this branch is reachable only from the + // `credentialCastTableModels` map, where it means the configured class + // does not exist. Returning the "no mapping" answer here would let a + // typo disarm the rule permanently and silently. if (!$this->reflectionProvider->hasClass($modelFqcn)) { - return $empty; + $resolution = ['casts' => [], 'unreadable' => [], 'incomplete' => [], 'missing' => true]; + $this->castCache[$modelFqcn] = $resolution; + + return $resolution; } $classReflection = $this->reflectionProvider->getClass($modelFqcn); $casts = []; $unreadable = []; + $incomplete = []; foreach (array_reverse([$classReflection, ...$classReflection->getParents()]) as $ancestor) { // `getTraits(true)` flattens traits-used-by-traits, so a cast // declared two trait hops away still counts. foreach ($ancestor->getTraits(true) as $trait) { - $this->mergeDeclaredCasts($trait, $casts, $unreadable); + $this->mergeDeclaredCasts($trait, $casts, $unreadable, $incomplete); } - $this->mergeDeclaredCasts($ancestor, $casts, $unreadable); + $this->mergeDeclaredCasts($ancestor, $casts, $unreadable, $incomplete); } $credentialCasts = []; @@ -546,7 +611,12 @@ private function castResolutionFor(string $modelFqcn): array } } - $resolution = ['casts' => $credentialCasts, 'unreadable' => $unreadable]; + $resolution = [ + 'casts' => $credentialCasts, + 'unreadable' => $unreadable, + 'incomplete' => $incomplete, + 'missing' => false, + ]; $this->castCache[$modelFqcn] = $resolution; return $resolution; @@ -558,9 +628,14 @@ private function castResolutionFor(string $modelFqcn): array * * @param array $casts * @param list $unreadable + * @param list $incomplete */ - private function mergeDeclaredCasts(ClassReflection $source, array &$casts, array &$unreadable): void - { + private function mergeDeclaredCasts( + ClassReflection $source, + array &$casts, + array &$unreadable, + array &$incomplete, + ): void { $declared = $this->declaredCasts($source); if ($declared === null) { @@ -569,7 +644,11 @@ private function mergeDeclaredCasts(ClassReflection $source, array &$casts, arra return; } - foreach ($declared as $column => $cast) { + if (!$declared['complete']) { + $incomplete[] = $source->getName(); + } + + foreach ($declared['casts'] as $column => $cast) { $casts[$column] = $cast; } } @@ -592,11 +671,24 @@ private function isCredentialCast(string $cast): bool * analyser. Both declaration forms are read: the `casts()` method's * `return [...]` statements and a `$casts` property default. * - * Returns NULL — never an empty array — when the source cannot be located - * or parsed, so the caller can tell "declares no casts" from "we could not - * look". An empty array means the source was read and declares none. + * Returns NULL — never an empty map — when the source cannot be located or + * parsed, so the caller can tell "declares no casts" from "we could not + * look". An empty map with `complete: true` means the source was read and + * declares none. * - * @return array|null + * `complete` is FALSE when the source was read but a cast declaration could + * not be interpreted: a `casts()` return statement, or a `$casts` property + * default, from which no array literal can be extracted at all. Composition + * forms that DO carry a literal are read rather than reported — the array + * inside `array_merge(parent::casts(), [...])` is collected, and a spread + * (`[...parent::casts(), 'password' => 'hashed']`) is an array literal + * whose spread item simply carries no string key. In both, the contributor + * being merged in is an ancestor call, and the ancestry is walked + * separately, so the merged map is complete. What cannot be read is a + * declaration carrying no literal at all — `return self::CASTS;`, + * `return $this->buildCasts();`, `protected $casts = self::CASTS;`. + * + * @return array{casts: array, complete: bool}|null */ private function declaredCasts(ClassReflection $classReflection): ?array { @@ -619,10 +711,11 @@ private function declaredCasts(ClassReflection $classReflection): ?array } $casts = []; + $complete = true; foreach ($classNode->stmts as $stmt) { if ($stmt instanceof ClassMethod && $stmt->name->toString() === 'casts') { - foreach ($this->returnedArrays($stmt) as $array) { + foreach ($this->returnedArrays($stmt, $complete) as $array) { foreach ($this->stringPairs($array) as $column => $cast) { $casts[$column] = $cast; } @@ -640,13 +733,19 @@ private function declaredCasts(ClassReflection $classReflection): ?array continue; } + if (!$prop->default instanceof Expr\Array_) { + $complete = false; + + continue; + } + foreach ($this->stringPairs($prop->default) as $column => $cast) { $casts[$column] = $cast; } } } - return $casts; + return ['casts' => $casts, 'complete' => $complete]; } /** @@ -699,16 +798,21 @@ private function childNodes(Node $node): array } /** - * Every array literal returned from a method body, including returns nested - * inside conditionals. + * Every array literal contributed by a `return` in a method body, including + * returns nested inside conditionals and literals nested inside a + * composition expression (`return array_merge(parent::casts(), [...]);`). + * + * `$complete` is set to FALSE when a return statement contributes no array + * literal at all, so the caller can report an incomplete cast map instead + * of silently reading it as "declares nothing". * * @return list */ - private function returnedArrays(ClassMethod $method): array + private function returnedArrays(ClassMethod $method, bool &$complete): array { $arrays = []; - $this->collectReturnedArrays($this->childNodes($method), $arrays); + $this->collectReturnedArrays($this->childNodes($method), $arrays, $complete); return $arrays; } @@ -717,22 +821,71 @@ private function returnedArrays(ClassMethod $method): array * @param list $nodes * @param list $arrays */ - private function collectReturnedArrays(array $nodes, array &$arrays): void + private function collectReturnedArrays(array $nodes, array &$arrays, bool &$complete): void { foreach ($nodes as $node) { - if ($node instanceof Return_ && $node->expr instanceof Expr\Array_) { - $arrays[] = $node->expr; + if ($node instanceof Return_) { + if ($node->expr === null) { + continue; + } + + $returned = []; + + $this->collectArrayLiterals([$node->expr], $returned); + + if ($returned === []) { + $complete = false; + + continue; + } + + foreach ($returned as $array) { + $arrays[] = $array; + } continue; } - // A nested closure or anonymous class carries its own returns, - // which are not this method's cast map. - if ($node instanceof Expr\Closure || $node instanceof Class_) { + // A nested function-like (closure, arrow function, nested function + // declaration) or anonymous class carries its own returns, which + // are not this method's cast map. + if ($node instanceof FunctionLike || $node instanceof Class_) { continue; } - $this->collectReturnedArrays($this->childNodes($node), $arrays); + $this->collectReturnedArrays($this->childNodes($node), $arrays, $complete); + } + } + + /** + * Array literals reachable inside one expression WITHOUT descending into a + * collected array — so `['a' => ['b' => 'c']]` contributes the outer array + * only, and `'b' => 'c'` never becomes a cast pair of its own. Composition + * expressions DO contribute: `array_merge(parent::casts(), [...])` and a + * ternary over two literals both yield the literals they carry. + * + * A literal inside a function-like (a closure or arrow function passed as an + * argument, an anonymous class) is NOT collected — it is a callback's return + * value, not this cast map, and harvesting it would be a false positive on a + * column the model never casts. + * + * @param list $nodes + * @param list $arrays + */ + private function collectArrayLiterals(array $nodes, array &$arrays): void + { + foreach ($nodes as $node) { + if ($node instanceof FunctionLike || $node instanceof Class_) { + continue; + } + + if ($node instanceof Expr\Array_) { + $arrays[] = $node; + + continue; + } + + $this->collectArrayLiterals($this->childNodes($node), $arrays); } } @@ -781,6 +934,40 @@ private function buildUnreadableSourceError( ->build(); } + /** + * @param list $incomplete + */ + private function buildIncompleteCastMapError( + MethodCall $node, + string $modelFqcn, + array $incomplete, + ): IdentifierRuleError { + $message = sprintf( + 'Cannot verify this write against %s: %s declares casts in a form this rule cannot read (a casts() return or a $casts default carrying no array literal, such as a class constant or a helper call), so the credential-cast map is incomplete and a hashed/encrypted column in this payload would go unreported. Restate the credential columns as literal string pairs, or suppress %s here if the write is known safe.', + $modelFqcn, + implode(', ', $incomplete), + self::INCOMPLETE_IDENTIFIER, + ); + + return RuleErrorBuilder::message($message) + ->identifier(self::INCOMPLETE_IDENTIFIER) + ->line($node->getStartLine()) + ->build(); + } + + private function buildConfiguredModelMissingError(MethodCall $node, string $modelFqcn): IdentifierRuleError + { + $message = sprintf( + 'Cannot verify this write: credentialCastTableModels maps this table to %s, which does not exist, so no credential-cast map could be read and a hashed/encrypted column in this payload would go unreported. Fix the FQCN in the parameter, or remove the mapping if the table is no longer covered.', + $modelFqcn, + ); + + return RuleErrorBuilder::message($message) + ->identifier(self::CONFIG_IDENTIFIER) + ->line($node->getStartLine()) + ->build(); + } + private function buildError( MethodCall $node, string $modelFqcn, diff --git a/tests/Fixtures/CredentialCastBypass/BuilderWrites.php b/tests/Fixtures/CredentialCastBypass/BuilderWrites.php index 2c03ee8..3ab9982 100644 --- a/tests/Fixtures/CredentialCastBypass/BuilderWrites.php +++ b/tests/Fixtures/CredentialCastBypass/BuilderWrites.php @@ -5,6 +5,9 @@ namespace App\Actions\CredentialCastBypass; use App\Models\CredentialCastBypass\ApiKey; +use App\Models\CredentialCastBypass\ComposedCastModel; +use App\Models\CredentialCastBypass\NestedLiteralCastModel; +use App\Models\CredentialCastBypass\SpreadCastModel; use App\Models\CredentialCastBypass\TraitCastModel; use App\Models\CredentialCastBypass\User; use App\Models\CredentialCastBypass\Vault; @@ -84,4 +87,24 @@ public function twoCredentialColumnsInOnePayload(): void { User::query()->update(['password' => 'p', 'api_token' => 't']); } + + public function castComposedWithArrayMerge(): void + { + ComposedCastModel::query()->update(['composed_secret' => 'raw']); + } + + public function castInheritedThroughAComposingLeaf(): void + { + ComposedCastModel::query()->update(['passphrase' => 'raw']); + } + + public function castComposedWithArraySpread(): void + { + SpreadCastModel::query()->update(['spread_secret' => 'raw']); + } + + public function castDeclaredAlongsideANestedLiteral(): void + { + NestedLiteralCastModel::query()->update(['real_secret' => 'raw']); + } } diff --git a/tests/Fixtures/CredentialCastBypass/CleanWrites.php b/tests/Fixtures/CredentialCastBypass/CleanWrites.php index 538270b..e4a497a 100644 --- a/tests/Fixtures/CredentialCastBypass/CleanWrites.php +++ b/tests/Fixtures/CredentialCastBypass/CleanWrites.php @@ -5,7 +5,9 @@ namespace App\Actions\CredentialCastBypass; use App\Models\CredentialCastBypass\Article; +use App\Models\CredentialCastBypass\ComposedCastModel; use App\Models\CredentialCastBypass\NearMissCastModel; +use App\Models\CredentialCastBypass\NestedLiteralCastModel; use App\Models\CredentialCastBypass\OverridingVault; use App\Models\CredentialCastBypass\TraitOverriddenCastModel; use App\Models\CredentialCastBypass\User; @@ -118,4 +120,24 @@ public function dynamicPayload(string $column, string $value): void { User::query()->update([$column => $value]); } + + /** + * The non-credential entry from the SAME composed cast map. Reading a + * composed declaration must not turn every column on that model into a + * finding — this is the false-positive direction of crit round 2, issue 1. + */ + public function nonCredentialColumnOfAComposedCastMap(): void + { + ComposedCastModel::query()->update(['composed_count' => 3]); + } + + /** + * The two literals a composed cast map must NOT contribute: a value nested + * inside the map itself, and a literal inside a callback passed as an + * argument. Both would be findings on columns the model never casts. + */ + public function nestedAndCallbackLiteralsAreNotCastPairs(): void + { + NestedLiteralCastModel::query()->update(['nested_secret' => 'x', 'decoy_secret' => 'y']); + } } diff --git a/tests/Fixtures/CredentialCastBypass/ComposedCastModel.php b/tests/Fixtures/CredentialCastBypass/ComposedCastModel.php new file mode 100644 index 0000000..e57b592 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/ComposedCastModel.php @@ -0,0 +1,28 @@ + + */ + protected function casts(): array + { + return array_merge(parent::casts(), [ + 'composed_secret' => 'hashed', + 'composed_count' => 'integer', + ]); + } +} diff --git a/tests/Fixtures/CredentialCastBypass/ConstantCastModel.php b/tests/Fixtures/CredentialCastBypass/ConstantCastModel.php new file mode 100644 index 0000000..1ee8276 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/ConstantCastModel.php @@ -0,0 +1,32 @@ + */ + private const array CASTS = ['constant_secret' => 'hashed']; + + protected $table = 'constant_cast_models'; + + /** + * @return array + */ + protected function casts(): array + { + return self::CASTS; + } +} diff --git a/tests/Fixtures/CredentialCastBypass/ConstantCastPropertyModel.php b/tests/Fixtures/CredentialCastBypass/ConstantCastPropertyModel.php new file mode 100644 index 0000000..7a07210 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/ConstantCastPropertyModel.php @@ -0,0 +1,26 @@ + */ + private const array CASTS = ['constant_property_secret' => 'encrypted']; + + protected $table = 'constant_cast_property_models'; + + /** @var array */ + protected $casts = self::CASTS; +} diff --git a/tests/Fixtures/CredentialCastBypass/NestedLiteralCastModel.php b/tests/Fixtures/CredentialCastBypass/NestedLiteralCastModel.php new file mode 100644 index 0000000..d5c43dc --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/NestedLiteralCastModel.php @@ -0,0 +1,45 @@ + cast` pairs: + * + * - a literal nested INSIDE the cast map (`'meta' => ['nested_secret' => …]`). + * Collecting array literals from anywhere inside a returned expression must + * stop at an array it already collected, or a nested value becomes a second + * cast map and every column named in it turns into a finding. + * - a literal inside a callback passed as an argument. It is that callback's + * return value, not this model's cast map. + * + * `real_secret` IS a cast and must still fire, so the fixture cannot pass by the + * rule simply going blind on this model. + */ +class NestedLiteralCastModel extends AbstractCredentialHolder +{ + protected $table = 'nested_literal_cast_models'; + + /** + * @return array + */ + protected function casts(): array + { + return array_merge(parent::casts(), $this->decorate(static fn(): array => ['decoy_secret' => 'hashed']), [ + 'real_secret' => 'hashed', + 'meta' => ['nested_secret' => 'hashed'], + ]); + } + + /** + * @param callable(): array $callback + * + * @return array + */ + private function decorate(callable $callback): array + { + return []; + } +} diff --git a/tests/Fixtures/CredentialCastBypass/SpreadCastModel.php b/tests/Fixtures/CredentialCastBypass/SpreadCastModel.php new file mode 100644 index 0000000..0f5c2e3 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/SpreadCastModel.php @@ -0,0 +1,24 @@ + + */ + protected function casts(): array + { + return [...parent::casts(), 'spread_secret' => 'encrypted']; + } +} diff --git a/tests/Fixtures/CredentialCastBypass/UninterpretableCastWrites.php b/tests/Fixtures/CredentialCastBypass/UninterpretableCastWrites.php new file mode 100644 index 0000000..51c5cd4 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/UninterpretableCastWrites.php @@ -0,0 +1,28 @@ +update(['unrelated' => 'value']); + } + + public function propertyDefaultingToAClassConstant(): void + { + ConstantCastPropertyModel::query()->update(['unrelated' => 'value']); + } +} diff --git a/tests/Rules/ForbidCredentialCastBypassRuleTest.php b/tests/Rules/ForbidCredentialCastBypassRuleTest.php index d300082..6fff76a 100644 --- a/tests/Rules/ForbidCredentialCastBypassRuleTest.php +++ b/tests/Rules/ForbidCredentialCastBypassRuleTest.php @@ -33,6 +33,12 @@ final class ForbidCredentialCastBypassRuleTest extends RuleTestCase private const string UNREADABLE_MESSAGE = 'Cannot verify this write against %s: the PHP declaring %s could not be located or parsed, so the credential-cast map is incomplete and a hashed/encrypted column in this payload would go unreported. Fix the source, or suppress forbidCredentialCastBypass.modelSourceUnreadable here if the write is known safe.'; + private const string INCOMPLETE_MESSAGE = 'Cannot verify this write against %s: %s declares casts in a form this rule cannot read (a casts() return or a $casts default carrying no array literal, such as a class constant or a helper call), so the credential-cast map is incomplete and a hashed/encrypted column in this payload would go unreported. Restate the credential columns as literal string pairs, or suppress forbidCredentialCastBypass.castMapIncomplete here if the write is known safe.'; + + private const string CONFIGURED_MODEL_MISSING_MESSAGE = 'Cannot verify this write: credentialCastTableModels maps this table to %s, which does not exist, so no credential-cast map could be read and a hashed/encrypted column in this payload would go unreported. Fix the FQCN in the parameter, or remove the mapping if the table is no longer covered.'; + + private const string UNINTERPRETABLE_CAST_WRITES = __DIR__ . '/../Fixtures/CredentialCastBypass/UninterpretableCastWrites.php'; + /** * Table-to-model map used by the `DB::table()` tests. Left null so the * default (empty map, `DB::table()` silent) is what every other test sees. @@ -54,20 +60,31 @@ final class ForbidCredentialCastBypassRuleTest extends RuleTestCase public function testHashedCastColumnInBuilderUpdateIsFlagged(): void { $this->analyse([self::BUILDER_WRITES], [ - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 20], - [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 25], - [sprintf(self::MESSAGE, 'recovery_codes', 'App\Models\CredentialCastBypass\User', 'encrypted:array', 'update'), 30], - [sprintf(self::MESSAGE, 'secret', 'App\Models\CredentialCastBypass\ApiKey', 'encrypted', 'update'), 35], - [sprintf(self::MESSAGE, 'passphrase', 'App\Models\CredentialCastBypass\Vault', 'hashed', 'update'), 40], - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 47], - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'insert'), 52], - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'upsert'), 60], - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'updateOrInsert'), 65], - [sprintf(self::MESSAGE, 'secret', 'App\Models\CredentialCastBypass\ApiKey', 'encrypted', 'update'), 70], - [sprintf(self::MESSAGE, 'trait_secret', 'App\Models\CredentialCastBypass\TraitCastModel', 'hashed', 'update'), 75], - [sprintf(self::MESSAGE, 'trait_notes', 'App\Models\CredentialCastBypass\TraitCastModel', 'encrypted', 'update'), 80], - [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 85], - [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 85], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 23], + [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 28], + [sprintf(self::MESSAGE, 'recovery_codes', 'App\Models\CredentialCastBypass\User', 'encrypted:array', 'update'), 33], + [sprintf(self::MESSAGE, 'secret', 'App\Models\CredentialCastBypass\ApiKey', 'encrypted', 'update'), 38], + [sprintf(self::MESSAGE, 'passphrase', 'App\Models\CredentialCastBypass\Vault', 'hashed', 'update'), 43], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 50], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'insert'), 55], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'upsert'), 63], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'updateOrInsert'), 68], + [sprintf(self::MESSAGE, 'secret', 'App\Models\CredentialCastBypass\ApiKey', 'encrypted', 'update'), 73], + [sprintf(self::MESSAGE, 'trait_secret', 'App\Models\CredentialCastBypass\TraitCastModel', 'hashed', 'update'), 78], + [sprintf(self::MESSAGE, 'trait_notes', 'App\Models\CredentialCastBypass\TraitCastModel', 'encrypted', 'update'), 83], + [sprintf(self::MESSAGE, 'api_token', 'App\Models\CredentialCastBypass\User', 'encrypted', 'update'), 88], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 88], + // crit round 2, issue 1 — the leaf's OWN cast, added through + // `array_merge(parent::casts(), …)`. Silent before the fix while + // line 97's inherited cast fired, so the same model was + // half-enforced. + [sprintf(self::MESSAGE, 'composed_secret', 'App\Models\CredentialCastBypass\ComposedCastModel', 'hashed', 'update'), 93], + [sprintf(self::MESSAGE, 'passphrase', 'App\Models\CredentialCastBypass\ComposedCastModel', 'hashed', 'update'), 98], + [sprintf(self::MESSAGE, 'spread_secret', 'App\Models\CredentialCastBypass\SpreadCastModel', 'encrypted', 'update'), 103], + // Composed maps must contribute their OWN pairs and nothing else — + // the nested and callback literals on this model are pinned clean in + // testModelPathAndNonCredentialWritesAreClean. + [sprintf(self::MESSAGE, 'real_secret', 'App\Models\CredentialCastBypass\NestedLiteralCastModel', 'hashed', 'update'), 108], ]); } @@ -125,6 +142,66 @@ public function testTheSameWriteIsSilentWhenTheModelSourceParses(): void $this->analyse([self::SINGLE_UNCAST_WRITE], []); } + /** + * crit round 2, issue 1 — the OTHER direction. `declaredCasts()` requires an + * array literal; a `casts()` return or `$casts` default that carries none + * (`return self::CASTS;`) was read as "declares nothing", so the write + * passed silently. The source is readable, so `modelSourceUnreadable` never + * fired either: the declaration SHAPE is what cannot be read, and it gets + * its own identifier because the remediation is different. + * + * Reported regardless of payload, like the unreadable-source diagnostic — + * neither payload below names a credential column, which is the point: with + * an incomplete map the rule cannot claim the payload is clean. + */ + public function testCastDeclarationsCarryingNoArrayLiteralAreReportedRatherThanReadAsCastless(): void + { + $this->analyse([self::UNINTERPRETABLE_CAST_WRITES], [ + [ + sprintf( + self::INCOMPLETE_MESSAGE, + 'App\Models\CredentialCastBypass\ConstantCastModel', + 'App\Models\CredentialCastBypass\ConstantCastModel', + ), + 21, + ], + [ + sprintf( + self::INCOMPLETE_MESSAGE, + 'App\Models\CredentialCastBypass\ConstantCastPropertyModel', + 'App\Models\CredentialCastBypass\ConstantCastPropertyModel', + ), + 26, + ], + ]); + } + + /** + * crit round 2, issue 2. `hasClass()` returning false was answered with the + * same empty cast set as "this table is not mapped", so a typo or a stale + * rename in `credentialCastTableModels` silently and permanently disarmed + * the rule for that table — the exact fail-open shape the unreadable-source + * identifier exists to prevent, arriving through configuration instead of + * source. + * + * The three `users` sites fire; `articles` (unmapped) and the non-literal + * table argument stay silent, so this also pins that a bad mapping does not + * spray the diagnostic over tables it was never configured for. + */ + public function testAMistypedConfiguredModelIsReportedRatherThanTreatedAsUnmapped(): void + { + $this->tableModelOverride = ['users' => 'App\Models\CredentialCastBypass\Uzer']; + + $message = sprintf(self::CONFIGURED_MODEL_MISSING_MESSAGE, 'App\Models\CredentialCastBypass\Uzer'); + + $this->analyse([self::RAW_TABLE_WRITES], [ + [$message, 18], + [$message, 23], + [$message, 28], + [$message, 33], + ]); + } + // ---------------------------------------------------------- DENOMINATOR --- /** @@ -144,8 +221,8 @@ public function testFixturePopulationIsNonZero(): void return preg_match_all('/(->|::)(update|insert|insertOrIgnore|insertGetId|upsert|updateOrInsert|create|updateOrCreate|save)\(/', $source); }; - self::assertGreaterThanOrEqual(13, $writeCalls(self::BUILDER_WRITES), 'The violating fixture lost write sites.'); - self::assertGreaterThanOrEqual(12, $writeCalls(self::CLEAN_WRITES), 'The clean fixture lost write sites, so its zero proves nothing.'); + self::assertGreaterThanOrEqual(17, $writeCalls(self::BUILDER_WRITES), 'The violating fixture lost write sites.'); + self::assertGreaterThanOrEqual(14, $writeCalls(self::CLEAN_WRITES), 'The clean fixture lost write sites, so its zero proves nothing.'); self::assertGreaterThanOrEqual(7, $writeCalls(self::RAW_TABLE_WRITES), 'The raw-table fixture lost write sites.'); } @@ -175,6 +252,11 @@ public function testEveryFixtureModelResolvesToThisRuleSOwnFixtureDirectory(): v 'App\Models\CredentialCastBypass\HasHashedSecret', 'App\Models\CredentialCastBypass\HasEncryptedNotesProperty', 'App\Models\CredentialCastBypass\ComposesHashedSecret', + 'App\Models\CredentialCastBypass\ComposedCastModel', + 'App\Models\CredentialCastBypass\SpreadCastModel', + 'App\Models\CredentialCastBypass\ConstantCastModel', + 'App\Models\CredentialCastBypass\ConstantCastPropertyModel', + 'App\Models\CredentialCastBypass\NestedLiteralCastModel', ]; $reflectionProvider = self::createReflectionProvider();