diff --git a/CHANGELOG.md b/CHANGELOG.md index dc12edd..88ea633 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,26 @@ 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`. + + **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. **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..0095172 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` / `.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. @@ -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..b3bc7aa 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 **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 @@ -230,6 +231,68 @@ 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. + +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 cast map cannot be read in full + +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: + +| 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 | + +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/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..603bb51 --- /dev/null +++ b/src/Rules/ForbidCredentialCastBypassRule.php @@ -0,0 +1,991 @@ +…->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. + * + * 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 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`, + * `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. + * - **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 + * 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 + */ +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'; + + /** + * 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 + * 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 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, incomplete: list, missing: bool}> + */ + 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 []; + } + + $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['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; + } + + $errors[] = $this->buildError($node, $modelFqcn, $column, $resolution['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 AND the trait use-chain, plus the list of declaring sources + * whose PHP could not be read. Memoized per FQCN. + * + * 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, incomplete: list, missing: bool} + */ + private function castResolutionFor(string $modelFqcn): array + { + if (array_key_exists($modelFqcn, $this->castCache)) { + return $this->castCache[$modelFqcn]; + } + + $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)) { + $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, $incomplete); + } + + $this->mergeDeclaredCasts($ancestor, $casts, $unreadable, $incomplete); + } + + $credentialCasts = []; + + foreach ($casts as $column => $cast) { + if ($this->isCredentialCast($cast)) { + $credentialCasts[$column] = $cast; + } + } + + $resolution = [ + 'casts' => $credentialCasts, + 'unreadable' => $unreadable, + 'incomplete' => $incomplete, + 'missing' => false, + ]; + $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 + * @param list $incomplete + */ + private function mergeDeclaredCasts( + ClassReflection $source, + array &$casts, + array &$unreadable, + array &$incomplete, + ): void { + $declared = $this->declaredCasts($source); + + if ($declared === null) { + $unreadable[] = $source->getName(); + + return; + } + + if (!$declared['complete']) { + $incomplete[] = $source->getName(); + } + + foreach ($declared['casts'] as $column => $cast) { + $casts[$column] = $cast; + } + } + + 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. + * + * 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. + * + * `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 + { + $file = $classReflection->getFileName(); + + if ($file === null) { + return null; + } + + try { + $stmts = $this->parser->parseFile($file); + } catch (ParserErrorsException) { + return null; + } + + $classNode = $this->findClassNode($stmts, $classReflection->getName()); + + if ($classNode === null) { + return null; + } + + $casts = []; + $complete = true; + + foreach ($classNode->stmts as $stmt) { + if ($stmt instanceof ClassMethod && $stmt->name->toString() === 'casts') { + foreach ($this->returnedArrays($stmt, $complete) 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; + } + + if (!$prop->default instanceof Expr\Array_) { + $complete = false; + + continue; + } + + foreach ($this->stringPairs($prop->default) as $column => $cast) { + $casts[$column] = $cast; + } + } + } + + return ['casts' => $casts, 'complete' => $complete]; + } + + /** + * 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): ?ClassLike + { + foreach ($nodes as $node) { + if ($node instanceof ClassLike && $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 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, bool &$complete): array + { + $arrays = []; + + $this->collectReturnedArrays($this->childNodes($method), $arrays, $complete); + + return $arrays; + } + + /** + * @param list $nodes + * @param list $arrays + */ + private function collectReturnedArrays(array $nodes, array &$arrays, bool &$complete): void + { + foreach ($nodes as $node) { + 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 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, $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); + } + } + + /** + * `'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; + } + + /** + * @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(); + } + + /** + * @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, + 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..3ab9982 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/BuilderWrites.php @@ -0,0 +1,110 @@ +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 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']); + } + + 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 new file mode 100644 index 0000000..e4a497a --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/CleanWrites.php @@ -0,0 +1,143 @@ +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']); + } + + /** + * 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. + */ + 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/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 @@ + */ + 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/HasEncryptedNotesProperty.php b/tests/Fixtures/CredentialCastBypass/HasEncryptedNotesProperty.php new file mode 100644 index 0000000..3208ee8 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/HasEncryptedNotesProperty.php @@ -0,0 +1,17 @@ + */ + 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/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/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/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..229b2c4 --- /dev/null +++ b/tests/Fixtures/CredentialCastBypass/RawTableWrites.php @@ -0,0 +1,57 @@ +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 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']); + } + + public function nonLiteralTableName(string $table): void + { + DB::table($table)->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/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/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/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/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'; + + 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.'; + + 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. + * + * @var array|null + */ + 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'), 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], + ]); + } + + 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], + [sprintf(self::MESSAGE, 'password', 'App\Models\CredentialCastBypass\User', 'hashed', 'update'), 33], + ]); + } + + // -------------------------------------------------------------- GREEN --- + + public function testModelPathAndNonCredentialWritesAreClean(): void + { + $this->analyse([self::CLEAN_WRITES], []); + } + + 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], []); + } + + /** + * 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 --- + + /** + * § 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(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.'); + } + + /** + * 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', + 'App\Models\CredentialCastBypass\TraitCastModel', + 'App\Models\CredentialCastBypass\TraitOverriddenCastModel', + '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(); + $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); + + if ($this->unparsableFileSuffix !== null) { + $parser = new ThrowingParser($parser, $this->unparsableFileSuffix); + } + + return new ForbidCredentialCastBypassRule( + self::createReflectionProvider(), + $parser, + $this->tableModelOverride ?? [], + ); + } +} 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); + } +}