diff --git a/CHANGELOG.md b/CHANGELOG.md index f3c8a16..5675e2b 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 +- `ForbidAdHocDateParsingRule` — new rule (war-room enforcement queue #222) forbidding the construction of a date/time value from a **string** anywhere outside the namespaces where the boundary decode is allowed to live. Registered in `extension.neon` with one new parameter, `dateParsingNamespaces` (default `['App\Support\Time', 'App\Support\DateTime', 'App\Casts']`). Identifier: `forbidAdHocDateParsing.stringParsedOutsideBoundary`. Doctrine: **ADR-0020 Amendment 1** (Semantic Boundary Types — "decoding happens once, at the boundary") + **ADR-0031** (instant vs wall-clock), which supplies the semantics the boundary type is obliged to carry. + + **The bug class.** The failure is not a parse that throws — it is a parse that SUCCEEDS in fifty places with fifty slightly different readings of the same string: a month-only bound, a `24:00` end-of-day, a bare `T` separator, an IANA zone the next Action drops, a day treated as an instant. Each reading is local and defensible; the disagreement surfaces only in a report that does not balance, which is the most expensive place to find it. Measured seed: **30 of 354** crit review findings in one week were date/cursor-bound semantics, each re-implemented per Action, and lokalekeuze PR #190 drew **seven review rounds** on a single parser. emmie carries 77 files calling `CarbonImmutable::parse(` and 20 calling `createFromFormat(` across `app/` against **one** strict helper (`App\Support\DateTime\InstantParser`) used by four of them — the ratio the rule exists to invert. + + **Detection — three call shapes, one registration.** `getNodeType()` returns `CallLike`, so a single service sees all three (mirrors `EnforceCurrentUserAttributeRule`): (1) a **static call** whose class RESOLVES to a `DateTimeInterface` subtype or to the `Illuminate\Support\Facades\Date` facade, with a method in the sixteen-entry `PARSING_METHODS` list (`parse`, `rawParse`, `parseFromLocale`, the `createFromFormat` / `rawCreateFromFormat` / `createFromIsoFormat` / `createFromLocaleFormat` / `createFromLocaleIsoFormat` / `createFromTimeString` / `createFromDate` / `createMidnightDate` / `createFromTime` factories, `create`, `make`, `createStrict`, `createSafe`); (2) **`new` on a `DateTimeInterface` subtype**; (3) a **function call** whose callee RESOLVES to `strtotime`, `date_create`, `date_create_immutable`, `date_create_from_format`, `date_create_immutable_from_format`, `date_parse` or `date_parse_from_format` — resolved through PHPStan's `ReflectionProvider`, never read off the written token, because PHP resolves an unqualified call to a same-namespace declaration when one exists (an `App\Support\strtotime()` helper is not the global function and must not fire) and `use function strtotime as decode;` is the same question from the other side. A `FuncCall` whose name is an `Expr` — a variable function — has no resolvable callee and stays out of scope. **All three pass through one argument gate**: the call fires only when the argument in its DECODED SLOT is present and its type is not provably non-string. The slot is named per verb rather than assumed to be argument zero — `createFromFormat` decodes its SECOND parameter and the locale-aware pair their THIRD, so "argument zero" is the format string for six of the sixteen methods and three of the seven functions — and it is addressed by parameter NAME first and position second, because since PHP 8.0 any caller may name any argument: `create(timezone: 'Europe/Amsterdam')` hands the call no date input at all, while `create(month: 1, year: $raw)` puts a string at index 1 where a source-order read finds an integer. The accepted spellings are asserted against the real Carbon and native signatures by a test (`testEveryDecodedSlotMatchesTheParameterItNames`), in both directions — every slot names a parameter that exists at that index, and every accepted spelling is earned by a real signature — because a rename upstream would disable the named lookup in silence; teeth-proved against both a renamed slot and a shifted position. The names alone do not discriminate — Carbon's `create()` delegates to `parse()` exactly when `$year` is a non-numeric string, and the same `create`/`createFromDate`/`createFromTime`/`createStrict`/`createSafe` assemble a date from integers, `make`/`parse` re-wrap an existing value, and every factory is a clock read with no argument. The gate's **direction** is deliberate: `mixed`, `int|string` and `?string` fire, because an untyped value at a parse site (`$request->input('from')`) is exactly what the boundary type exists to pin down. + + **One anchor, not two.** `Carbon\CarbonInterface` extends `DateTimeInterface`, so a single supertype check covers `Carbon\Carbon`, `Carbon\CarbonImmutable`, `Illuminate\Support\Carbon`, `\DateTime`, `\DateTimeImmutable` and every subclass. A second, Carbon-specific condition would be redundant and — worse — **unfalsifiable**: no input could distinguish the two, so nothing could ever prove the Carbon half still worked. That inheritance is a fact about an upstream package rather than about this code, so it is **asserted in the suite** (`testCarbonInterfaceExtendsTheDateTimeAnchor`) rather than claimed in a docblock. A Carbon release that stopped extending `DateTimeInterface` would otherwise narrow the rule to the two PHP natives with every fixture still green. + + **The static method name is folded, the class is resolved by TYPE.** PHP dispatches `PARSE()` and `parse()` to the same method, so the written identifier is lower-cased before the lookup and reported back with the casing the source wrote — that is the token the reader has to change. **Class resolution is by TYPE, never by the written token.** An aliased import (`use Carbon\CarbonImmutable as C; C::parse(…)`) fires and reports under the real class; a local class merely NAMED `Carbon` that declares a static `parse()` does **not** — both pinned by fixtures, and the second one reds the moment the type check is swapped for a name match. + + **Deliberate misses, and the reason is the same one every time — nothing here decodes a string.** Any listed call whose decoded slot is absent or provably not a string — zero-argument construction and factories, a named call that fills no date slot (`Carbon::create(timezone: …)`) (`new DateTimeImmutable()`, `Carbon::create()`, `date_create()` are "now"), integer components (`Carbon::create(2026, 9, 7)`), `null`, an existing `DateTimeInterface` value (`Carbon::make($carbon)`); the clock reads `now` / `today` / `yesterday` / `tomorrow`; the `createFromTimestamp*` family plus `instance` and `fromSerialized`, because a timestamp is already an instant; instance calls on an already-decoded value (`->format()`, `->addDays()`, `->startOfDay()`), which are what a boundary type EXISTS to make safe; and a dynamic class expression (`$class::parse()`), where the receiver has no resolvable name and guessing one would be a false-positive source a boundary rule cannot afford; and a **first-class callable** (`CarbonImmutable::parse(...)`, `strtotime(...)`), which decodes nothing where it is written — the string arrives wherever the callable is later invoked. That last one is silent STRUCTURALLY rather than by exemption: PHPStan substitutes `StaticMethodCallableNode` / `FunctionCallableNode` / `MethodCallableNode` for a first-class callable, none of which extends `CallLike`, so the rule's registration cannot receive one. That is also why `decodedSlotMayBeAString()` may call `CallLike::getArgs()` unguarded even though it asserts `!isFirstClassCallable()`; being a fact about PHPStan rather than about this code, it is **asserted in the suite** (`testPhpstanFirstClassCallableNodesAreNotCallLike`) rather than claimed in a docblock — the same treatment as the Carbon anchor above — with fixture lines on both delivered shapes as the tripwire. A class in the **global namespace** is outside every configured prefix and therefore fires — consumers analyse `app/`, which is namespaced throughout. + + **The boundary is configuration, not a carve-out.** A namespace is inside `dateParsingNamespaces` when it EQUALS an entry or continues one across a namespace SEPARATOR, so `App\Casts\Money` is inside `App\Casts` and `App\CastsReport` — which merely shares an opening substring — is not. A configured prefix's trailing backslash is normalised once, so `App\Casts\` is not a second, never-matching spelling of the same boundary. The default names the two doors ADR-0020 Amd 1 recognises: a dedicated decode helper (emmie's `App\Support\DateTime\InstantParser`; lokalekeuze's lands under `App\Support\Time`) and the row-to-model cast (`App\Casts`), where a column genuinely arrives as a string from outside and must become a value object exactly once. A territory narrows or widens the list from its own `phpstan.neon`; the rule knows nothing about any territory's layout. There is deliberately **no per-call-site comment exemption** — a call site that would need one is a boundary namespace nobody has named yet. + + **Teeth, both directions.** Positive: six shape fixtures assert an exact message at an exact line (Action `parse`, FormRequest `createFromFormat`, aliased import, `Date` facade, `new \DateTimeImmutable($string)`, `strtotime` in a service). Negative: five assert `[]` — the two boundary namespaces, the clock/timestamp/instance-call set, the component-factory / re-wrap / zero-argument set, and the local class named `Carbon`; a sixth positive fixture pins the gate's direction (`mixed`, `int|string`, `?string` fire). Five teeth probes were run and each reddened exactly the assertion that pins it, then restored: disabling the namespace gate reds both boundary fixtures (4 failures); removing the argument gate reds the component-factory fixture; flipping the gate to demand a *proven* string reds the direction fixture; dropping the two `*_from_format` functions reds the denominator; replacing the type check with a name match reds the local-`Carbon` fixture. **Denominator:** `AllParsingMethods.php` names every entry of both lists exactly once, each handed a string in its decoded slot, and asserts 26 errors at 26 named lines, so an entry dropped by hand or by a mutation operator fails by line rather than silently narrowing what the rule can see. + + **The method table is no longer maintained by hand.** Four consecutive review rounds on the rule's pull request each found a true defect, and the fourth found two static factories `PARSING_METHODS` had never carried — `parseFromLocale` and `createMidnightDate` — with a reflection sweep over the same surface immediately turning up a third, `rawCreateFromFormat`, that nobody reading the list had seen. All three are now listed (`parseFromLocale` decodes `$time` at index 0; `rawCreateFromFormat` decodes `$time` at index 1, after the format; `createMidnightDate` decodes `$year` at index 0, because it delegates to `create()`, which delegates to `parse()` when `$year` is a non-numeric string). More importantly the list's completeness stopped being a claim: `testEveryCarbonStaticFactoryIsClassifiedAsDecodingOrNot` reflects every public static factory of `Carbon\Carbon`, `Carbon\CarbonImmutable` and `Illuminate\Support\Carbon` — 34 of them today — and fails BY NAME on any that is neither a key of `PARSING_METHODS` nor a key of the new `NON_DECODING_FACTORIES` constant, which carries a one-line reason per entry. A method whose returned shape cannot be read at all (no declared return type and no `@return`, as with `createFromImmutable` and `createFromMutable`) counts as a factory and must be classified, because an unreadable shape has to be decided rather than assumed inert. `Illuminate\Support\Carbon` is reflected alongside the two `Carbon\*` classes because the rule already fires on it and it is what the `Date` facade resolves to, so the facade path rides the same table; it contributes exactly one factory of its own, `createFromId`. A second test, `testNoAllowedFactoryTakesAStringInADecodedSlot`, stops the allowlist being used to silence a decoder: no entry may name a parameter with one of the slot spellings the rule reads (`$time`, `$datetime`, `$year`, `$hour`, `$var`) typed so a string fits. The discriminator has to be the parameter NAME — `now(DateTimeZone|string|int|null $timezone)` and `createFromTimestamp(string|int|float $timestamp)` both take a string in their first slot and both legitimately belong on the allowlist. Teeth: dropping `parsefromlocale` from `PARSING_METHODS` reds the completeness test naming `Carbon\Carbon::parseFromLocale()`, and moving `parse` into `NON_DECODING_FACTORIES` reds the allowlist gate on its `$time` parameter. Both constants also assert a non-empty denominator, disjointness, and that every allowlist entry names a method that actually exists on a reflected class. **Configuration is proved in both directions plus a control** — an override to `['App\Domain\Clock']` makes the default boundary fixture fire AND silences a fixture under the overridden prefix, and that same fixture fires under the shipped default, so the silence is the parameter talking rather than a fixture that could never have been flagged. `extension.neon` wiring is pinned end-to-end by resolving the rule from the PHPStan container, which exercises the shipped NEON default rather than the PHP constructor default. Four further fixtures pin the four things the written token cannot answer, each reverted to its pre-fix shape and seen to red exactly the assertion that pins it: a same-namespace `strtotime()` helper beside an aliased import of the global one; `create(timezone: …)` silent beside `create(month: 1, year: $raw)` firing; `CarbonImmutable::PARSE($raw)` firing; and `App\CastsReport` firing beside a control — `App\Casts\Reporting` — that stays exempt, so tightening the prefix test to an exact match reds rather than passing. + + **Versioning: MINOR** (a new rule; no existing rule changes and no option's default moves). It surfaces new errors wherever a consumer parses a date outside a boundary namespace — which today is most of them — so adoption is a baseline-or-drain exercise per territory. 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). + - `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(...)`, the Postgres-only `updateFrom` / `insertOrIgnoreReturning`, `incrementOrCreate`, the increment family loud and quiet, 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. @@ -83,7 +103,7 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and ### Security -- `league/commonmark` 2.9.0 → 2.10.0 in `composer.lock` (GHSA-8rr7-cvq3-gmfh; the 2.9.1 advisories GHSA-jjv6-8j6v-6j52, GHSA-f8fg-pg57-v4j8 and GHSA-j8pm-gj4c-rq4x close with it). A **runtime** dependency of this package: `illuminate/mail` is in `require` and pulls `league/commonmark ^2.7`, so `composer install --no-dev` installs it. This lock pins only what this repo's own CI and self-analysis tree install — a consumer territory resolves the range through its own lock and must bump there — but the stale pin here was reddening every check on every open PR, because `composer install` audits the lock on every job (WR-1256). +- `league/commonmark` 2.9.0 → 2.10.0 in `composer.lock` (GHSA-8rr7-cvq3-gmfh; the 2.9.1 advisories GHSA-jjv6-8j6v-6j52, GHSA-f8fg-pg57-v4j8 and GHSA-j8pm-gj4c-rq4x close with it). A **runtime** dependency of this package: `illuminate/mail` is in `require` and pulls `league/commonmark ^2.7`, so `composer install --no-dev` installs it. This lock pins only what this repo's own CI and self-analysis tree install — a consumer territory resolves the range through its own lock and must bump there — but the stale pin here was reddening every open PR, because the `check` job runs an explicit `composer audit` step on both of its PHP legs (WR-1256). It is that step alone: `check-production-tree` installs `--no-dev` and `check-lowest-laravel` runs `composer update`, and neither audits. ## [0.8.0] — 2026-08-11 diff --git a/CLAUDE.md b/CLAUDE.md index 2af98a3..3191d7b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,6 +39,7 @@ Composer package distributing war-room-doctrine PHPStan rules across `script-dev | `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` plus the increment family (`increment`/`decrement`/`incrementEach`/`decrementEach`, whose EXTRA payload reaches SQL via `update(array_merge($columns, $extra))`) 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, read PER UNION BRANCH; `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` — then resolved as PHP resolves them, NOT merged across the ancestry: `$casts` is a property so ONE declaration survives (most derived, replacing), `casts()` is a SINGLE virtual dispatch so only the nearest body runs and an ancestor contributes only through an explicit `parent::casts()` (carried by `array_merge(parent::casts(), …)`, a spread, or a bare pass-through). Laravel's `array_merge($this->casts, $this->casts())` makes the METHOD half win on a shared column regardless of source order. ⚠ The merge-everything reading is wrong on 9 of the 23 shapes in `CastDispatchShapes.php`, 8 of them FALSE POSITIVES inventing a cast the model does not have; a first-match trait walk is wrong on 2 others (`insteadof`, discarded `parent::casts()`). The shape test computes its expectation from PHP itself. 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]`) | +| `ForbidAdHocDateParsingRule` | ADR-0020 Amd 1 (Semantic Boundary Types) + ADR-0031 | `forbidAdHocDateParsing.stringParsedOutsideBoundary` (type-aware; flags date/time construction from a STRING outside the configured boundary namespaces — a static call on a `DateTimeInterface` subtype (covers the whole Carbon family, since `CarbonInterface` extends it) or the `Illuminate\Support\Facades\Date` facade with a method in the 13-entry parse/create set, `new` on a `DateTimeInterface` subtype, and the `strtotime` / `date_create` / `date_create_immutable` / `date_create_from_format` / `date_create_immutable_from_format` / `date_parse` / `date_parse_from_format` functions — each only when the argument in the verb's DECODED SLOT is present and not provably non-string (`mixed` and `int|string` fire; integer components, `null`, an existing value object and a zero-argument clock read do not). The slot is per verb rather than argument zero (`createFromFormat` decodes its second parameter, `create` its `$year`) and is read by argument NAME first, position second, so `create(timezone: …)` is silent and `create(month: 1, year: $raw)` fires; the spellings are pinned against the real Carbon and native signatures by a test. Allowed prefixes come from `dateParsingNamespaces`, default `App\Support\Time` / `App\Support\DateTime` / `App\Casts` — the two boundary doors of ADR-0020 Amd 1 — matched on a namespace SEPARATOR, so `App\Casts\Money` is inside and `App\CastsReport` is not. The static method name is folded (`Carbon::PARSE()` fires) and the function callee is RESOLVED through the `ReflectionProvider`, so a same-namespace `strtotime()` helper is silent and `use function strtotime as decode;` fires. Class resolution is by TYPE: an aliased `use Carbon\CarbonImmutable as C;` fires, a local class merely NAMED `Carbon` does not. Deliberate misses, each because no string is decoded: any listed call whose decoded slot is absent or provably not a string, the `now`/`today`/`yesterday`/`tomorrow` clock reads, the `createFromTimestamp*` family, `instance`, `fromSerialized`, instance calls on an already-decoded value, and a dynamic class expression. No per-call-site comment exemption — a site that needs one is an unnamed boundary namespace. Seed war-room enforcement queue #222; 30 of 354 crit findings in one week were date/cursor-bound semantics. on `main`, `[Unreleased]`) | | `ConnectionTransactionReturnTypeExtension` | (type extension, no rule) | — (resolves `ConnectionInterface::transaction()` to the closure's return type. **Load-bearing on `illuminate/* ^12` only** — it annotates `@return mixed`; Laravel 13 annotates `@template TReturn`/`@return TReturn` and infers the same type unaided. Its teeth are measured by the `check-lowest-laravel` CI job, not by the unit suite on a Laravel-13 tree. WR-0855, WR-0860) | Phase 2 expands the rule set: `EnforceAuditSnapshotOnRetryRule` (ADR-0001 §Snapshot-on-Retry Safety) was the first Phase 2 addition, promoted from cross-territory Pest arch tests (emmie PR #187, entreezuil PR #139, ublgenie PR #166, kendo PR #1029). `EnforceResourceDataValidatorOptInRule` (ADR-0009 §EAGER_LOAD validator opt-in) is the second Phase 2 addition, promoted from kendo PR #1084 under war-room enforcement queue #55. `EnforceFormRequestToDtoRule` (ADR-0012) is the third Phase 2 addition, promoted from entreezuil's `tests/Arch/FormRequestsTest.php` under the same queue #55 (instance 2). `EnforceExplicitHydrationRule` (ADR-0019) is the next Phase 2 candidate. diff --git a/README.md b/README.md index 2cfd52a..d18151f 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,7 @@ includes: | `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` | Write calls (`update`, `insert`, `insertOrIgnore`, `insertGetId`, `upsert`, `updateOrInsert`, `updateFrom`, `insertOrIgnoreReturning`, `incrementOrCreate`, and the increment family loud and quiet) 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 increment family is included because `Query\Builder::incrementEach()` is literally `update(array_merge($columns, $extra))` — its extra payload is an ordinary uncast write — and for that family a **`Model` receiver is in scope too**: `Model::__call()` re-exposes the protected increment methods, and `Model::incrementOrDecrement()` casts the in-memory attribute via `forceFill($extra)` while passing the same `$extra` uncast to the query builder. Payload arguments are addressed by parameter NAME as well as position, since `increment('votes', extra: [...])` puts the payload at index 1 rather than 2. The model comes from the builder's/relation's generic type argument, read per UNION branch so `Builder|Builder` is checked against both cast maps; 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 — and then resolved the way PHP resolves them rather than merged: `$casts` is a property, so ONE declaration survives (most derived, replacing not merging), while `casts()` is a single virtual dispatch, so only the nearest body runs and an ancestor contributes only through an explicit `parent::casts()`. Laravel merges the two as `array_merge($this->casts, $this->casts())`, so the method half wins on a shared column whatever order the file declares them in. 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. | +| `ForbidAdHocDateParsingRule` | `forbidAdHocDateParsing.stringParsedOutsideBoundary` | Construction of a date/time value from a STRING outside the configured boundary namespaces (`dateParsingNamespaces`, default `App\Support\Time` / `App\Support\DateTime` / `App\Casts`) | Three shapes are errors: a **static call** whose class resolves to a `DateTimeInterface` subtype (`Carbon\Carbon`, `Carbon\CarbonImmutable`, `Illuminate\Support\Carbon`, `\DateTime`, `\DateTimeImmutable`, any subclass) or to the `Illuminate\Support\Facades\Date` facade, with a method in `parse`, `rawParse`, `parseFromLocale`, `createFromFormat`, `rawCreateFromFormat`, `createFromIsoFormat`, `createFromLocaleFormat`, `createFromLocaleIsoFormat`, `createFromTimeString`, `createFromDate`, `createMidnightDate`, `createFromTime`, `create`, `make`, `createStrict`, `createSafe` — **matched case-insensitively**, because PHP dispatches `Carbon::PARSE()` to the same method; **`new` on a `DateTimeInterface` subtype**; and a **function call whose callee resolves to** `strtotime`, `date_create`, `date_create_immutable`, `date_create_from_format`, `date_create_immutable_from_format`, `date_parse` or `date_parse_from_format` — resolved, not matched on the written token, so a same-namespace helper of your own named `strtotime()` is silent while `use function strtotime as decode;` fires under the real name. **One argument gate on all three shapes:** the call fires only when the argument in its **decoded slot** is present and its type is not provably non-string — `Carbon::create(2026, 9, 7)`, `Carbon::make($carbon)`, `Carbon::create()` and `date_create()` are silent; `Carbon::create($raw)` with `$raw` a `string`, `mixed`, `int\|string` or `?string` fires. The slot is per verb, not argument zero: `createFromFormat` decodes its second parameter, `createFromLocaleFormat` its third, `create` its `$year`. It is read by **argument name first, position second**, so `Carbon::create(timezone: 'Europe/Amsterdam')` is silent (no date input at all) and `Carbon::create(month: 1, year: $raw)` fires. Decode the string ONCE at the boundary and pass the value object onward. **Type-aware** — an aliased import (`use Carbon\CarbonImmutable as C;`) fires and reports under the real class, while a local class merely NAMED `Carbon` does not. **Deliberate misses, none of which decodes a string:** a listed call whose decoded slot is absent or provably not a string (`new DateTimeImmutable()` is "now", `createFromDate(2026, 9, 7)` assembles from integers, `Carbon::create(timezone: …)` names no date slot at all); the clock reads `now` / `today` / `yesterday` / `tomorrow`; the `createFromTimestamp*` family, `instance` and `fromSerialized` (a timestamp is already an instant); instance calls on an already-decoded value (`->format()`, `->addDays()`, `->startOfDay()`); and a dynamic class expression (`$class::parse()`), where the receiver has no resolvable name. **The method table is reflection-checked, not hand-maintained:** every public static factory of `Carbon\Carbon`, `Carbon\CarbonImmutable` and `Illuminate\Support\Carbon` must be classified either as a decoder in the list above or as a non-decoder in the rule's `NON_DECODING_FACTORIES` constant, each with a stated reason, and a Carbon release that adds a factory fails the suite by name instead of opening a silent hole. A class in the GLOBAL namespace is outside every prefix and therefore fires. Doctrine: ADR-0020 Amendment 1 (Semantic Boundary Types) + ADR-0031 (instant vs wall-clock). Seed: war-room enforcement queue #222. | ### `EnforceActionTransactionsRule` — write-method list @@ -380,6 +381,29 @@ return Response::error('Report failed: ' . $e->getMessage()); The standard PHPStan inline-ignore on `forbidRawExceptionMessageInResponse.rawMessageInResponse` is the alternative. `getTraceAsString()` / `__toString()` and a Throwable laundered through a formatter call are deliberate v1 misses. +### `ForbidAdHocDateParsingRule` — configuring the boundary namespaces + +The rule is silent inside the namespaces where the decode is *supposed* to live, and fires everywhere else. Which namespaces those are is configuration, not a hardcoded carve-out: + +```neon +parameters: + dateParsingNamespaces: + - 'App\Support\Time' + - 'App\Support\DateTime' + - 'App\Casts' +``` + +A namespace matches when it **equals** a prefix or continues one across a namespace **separator**, so a prefix covers its sub-namespaces naturally — `App\Support\Time` exempts `App\Support\Time\Parsing\IsoBounds` without a second entry — while a namespace that merely shares an opening substring does not: `App\CastsReport` is outside `App\Casts` and fires. A trailing backslash in a configured prefix is normalised, so `App\Casts\` and `App\Casts` name the same boundary. **Single backslashes** — see the NEON-quoting note in `extension.neon`. + +The default names the two boundary doors ADR-0020 Amendment 1 recognises: + +- **A dedicated decode helper.** emmie ships `App\Support\DateTime\InstantParser`; lokalekeuze's lands under `App\Support\Time`. This is where a request string, a query parameter or a cursor becomes a value object, with the instant-vs-wall-clock question of ADR-0031 answered once and in one place. +- **The row-to-model cast** (`App\Casts`). A database column is genuinely a string arriving from outside, and the cast is the one place the application is entitled to interpret it. + +A territory narrows or widens the list from its own `phpstan.neon`; the rule itself knows nothing about any territory's layout. **Setting it to `[]` is legal and arms the rule everywhere** — useful on a territory whose decoding already lives behind a value object, but it will flag the value object's own constructor. + +Adoption on a tree with existing violations is a `phpstan.neon` baseline or an `ignoreErrors` entry on `forbidAdHocDateParsing.stringParsedOutsideBoundary`, drained as the parses are folded into the boundary type. The rule deliberately has **no per-call-site comment exemption**: a call site that needs one is a boundary namespace that has not been named yet. + ### Action namespace assumption `EnforceActionTransactionsRule` and `ForbidDatabaseManagerInActionsRule` only fire on classes whose namespace starts with `App\Actions`. This matches the Laravel convention used in every `script-development` territory. Territories using a different actions namespace should open a PR to make this configurable. @@ -400,7 +424,7 @@ Semantic versioning: - **Minor** — a new rule is added, or a rule gains an option that doesn't change defaults. - **Patch** — bug fixes, false-positive suppression, performance improvements. -Pin to a 0.x minor version today (`^0.2`); future 1.0 release will allow `^1.0` pinning. See `CLAUDE.md` § Versioning for the 0.x caret-semantics rationale. +Pin to a 0.x minor version today (`^0.8`, the current minor); future 1.0 release will allow `^1.0` pinning. See `CLAUDE.md` § Versioning for the 0.x caret-semantics rationale. ## License diff --git a/extension.neon b/extension.neon index dc3fa87..6171c22 100644 --- a/extension.neon +++ b/extension.neon @@ -77,6 +77,24 @@ parameters: # backslashes — see the NEON-quoting note above. credentialCastTableModels: [] + # `ForbidAdHocDateParsingRule`: boundary NAMESPACES inside which a date/time + # STRING may legitimately be decoded. A class is inside an entry when its + # namespace EQUALS it or continues it across a namespace separator — so + # `App\Casts\Money` is inside `App\Casts` and `App\CastsReport`, which merely + # shares an opening substring, is NOT (a trailing backslash on an entry is + # normalised away). Everywhere else, constructing a date from a string is an + # error. The default names the two boundary doors of ADR-0020 Amendment 1 — + # a dedicated decode helper (emmie ships `App\Support\DateTime\InstantParser`; + # lokalekeuze's lands under `App\Support\Time`) and the row-to-model cast + # (`App\Casts`), where the string genuinely arrives from outside and must + # become a value object exactly once. A territory narrows or widens this + # list; the rule itself knows nothing about any territory's layout. Each + # prefix uses single backslashes — see the NEON-quoting note above. + dateParsingNamespaces: + - 'App\Support\Time' + - 'App\Support\DateTime' + - 'App\Casts' + parametersSchema: resourceDataBaseClass: string() formRequestBaseClass: string() @@ -87,6 +105,7 @@ parametersSchema: rawExceptionMessageSinks: listOf(string()) safeMessageExceptionClasses: listOf(string()) credentialCastTableModels: arrayOf(string()) + dateParsingNamespaces: listOf(string()) services: - @@ -162,6 +181,11 @@ services: rawExceptionMessageSinks: %rawExceptionMessageSinks% safeMessageExceptionClasses: %safeMessageExceptionClasses% tags: [phpstan.rules.rule] + - + class: ScriptDevelopment\PhpstanWarroomRules\Rules\ForbidAdHocDateParsingRule + arguments: + dateParsingNamespaces: %dateParsingNamespaces% + tags: [phpstan.rules.rule] - class: ScriptDevelopment\PhpstanWarroomRules\Rules\ForbidCredentialCastBypassRule arguments: diff --git a/src/Rules/ForbidAdHocDateParsingRule.php b/src/Rules/ForbidAdHocDateParsingRule.php new file mode 100644 index 0000000..f29d4d6 --- /dev/null +++ b/src/Rules/ForbidAdHocDateParsingRule.php @@ -0,0 +1,554 @@ +input('from')`) is + * exactly the input the boundary type exists to pin down; a gate that demanded + * a PROVEN string would exempt every one of those. + * + * `Carbon\CarbonInterface` extends `DateTimeInterface`, so ONE supertype check + * covers the whole Carbon family as well as the two PHP natives — a second, + * Carbon-specific check would be redundant and, worse, unfalsifiable: no input + * could distinguish the two conditions, so nothing could ever prove the second + * one still worked. That inheritance is load-bearing rather than incidental, so + * it is asserted in the test suite (`testCarbonInterfaceExtendsTheDateTimeAnchor`) + * instead of claimed in this docblock — a Carbon release that stopped extending + * `DateTimeInterface` would silently narrow this rule to the two PHP natives, + * and the assertion is what turns that into a red build. + * + * WHAT DOES NOT FIRE, by design — none of these decodes a string: + * - `now()`, `today()`, `yesterday()`, `tomorrow()`, `instance()`, + * `fromSerialized()` and the `createFromTimestamp*` family. A timestamp is + * already an instant; there is nothing to interpret. Each of these is named + * in `NON_DECODING_FACTORIES` with its reason, so a factory belonging to + * neither constant is a failed test rather than a silent escape. + * - Any listed call whose decoded slot is absent or provably not a string + * (see the gate above): zero-argument construction and factories, integer + * components, `null`, an existing `DateTimeInterface` value. + * - Instance calls: `$date->format(...)`, `$date->addDays(1)`, + * `$date->startOfDay()`. The value object is already decoded; moving it + * around is what the boundary type exists FOR. + * - A `StaticCall` on a dynamic class expression (`$class::parse(...)`), and + * a `New_` on a dynamic class expression. An accepted false negative: the + * receiver has no resolvable name, and guessing one would be a + * false-positive source a boundary rule cannot afford. + * - A first-class callable (`Carbon::parse(...)`). Nothing is decoded at + * that site, and PHPStan does not hand the node to this rule. Accepted + * false negative, pinned by fixture. + * - A local class that merely happens to be NAMED `Carbon` and declares a + * static `parse()`. Resolution is by TYPE, never by string-matching the + * class name — pinned by a negative fixture. + * + * THE ALLOWED NAMESPACES ARE CONFIGURATION, not a hardcoded carve-out. A class + * whose namespace EQUALS an entry in `dateParsingNamespaces`, or continues one + * across a namespace SEPARATOR, is silent — so `App\Casts\Money` is inside + * `App\Casts` and `App\CastsReport` is not. The default + * `['App\Support\Time', 'App\Support\DateTime', 'App\Casts']` covers the two + * boundary doors ADR-0020 Amd 1 names: a dedicated decode helper (emmie ships + * `App\Support\DateTime\InstantParser`; lokalekeuze's lands under + * `App\Support\Time`) and the row-to-model cast (`App\Casts`), where the string + * genuinely arrives from outside and must become a value object exactly once. A + * territory narrows or widens by configuration; the rule itself knows nothing + * about any territory's layout. + * + * A class in the GLOBAL namespace (`$scope->getNamespace() === null`) is + * outside every configured prefix and therefore fires. That is deliberate: + * consumers analyse `app/`, which is namespaced throughout, and a + * global-namespace parse is by definition not inside a boundary namespace. + * + * @implements Rule + */ +final class ForbidAdHocDateParsingRule implements Rule +{ + /** + * The other half of Carbon's static factory surface: methods that return an + * instance without interpreting any argument as a date/time string. Listing + * them is what lets `testEveryCarbonStaticFactoryIsClassifiedAsDecodingOrNot` + * fail on a factory that is in NEITHER constant — the state a reviewer + * found by hand on the fourth consecutive round of PR #71. + * + * The reason is not decoration — it is the claim the entry makes, and the + * completeness test refuses any entry whose real signature names a decoded + * slot (`$time`, `$datetime`, `$year`, `$hour`, `$var`) with a type that + * admits a string. That is what stops a future author silencing a decoder + * by moving its name down here. + * + * Analysis never reads it: a name that is absent from `PARSING_METHODS` + * already returns no error, so consulting this list at analysis time would + * be a branch no input could distinguish. Its only reader is the + * completeness test, and `public` is what says so — a private constant + * nothing in this class consults is `classConstant.unused`, and the fix for + * that is to declare the external reader, not to invent a use. + * + * @var array + */ + public const array NON_DECODING_FACTORIES = [ + 'now' => 'Reads the clock. There is no argument to interpret.', + 'today' => 'Reads the clock, truncated to the day.', + 'tomorrow' => 'Reads the clock, offset by a day.', + 'yesterday' => 'Reads the clock, offset by a day.', + 'instance' => 'Re-wraps a DateTimeInterface that is already decoded.', + 'createfrominterface' => 'Re-wraps a DateTimeInterface that is already decoded.', + 'createfromimmutable' => 'Re-wraps a DateTimeImmutable that is already decoded.', + 'createfrommutable' => 'Re-wraps a DateTime that is already decoded.', + 'createfromid' => 'Reads the embedded timestamp of an ordered UUID or ULID, not a date string.', + 'fromserialized' => 'Restores a previously serialized instance.', + '__set_state' => 'Restores an instance from its var_export() form.', + 'createfromtimestamp' => 'A timestamp is already an instant; nothing is interpreted.', + 'createfromtimestamputc' => 'A timestamp is already an instant; nothing is interpreted.', + 'createfromtimestampms' => 'A timestamp is already an instant; nothing is interpreted.', + 'createfromtimestampmsutc' => 'A timestamp is already an instant; nothing is interpreted.', + 'startoftime' => 'The lowest representable instant. It takes no argument.', + 'endoftime' => 'The highest representable instant. It takes no argument.', + 'gettestnow' => 'Returns the configured test clock; it is a getter, not a factory over an argument.', + ]; + + private const string IDENTIFIER = 'forbidAdHocDateParsing.stringParsedOutsideBoundary'; + + /** + * The supertype every date/time class this rule cares about shares. + * `Carbon\CarbonInterface` extends it, so does `\DateTime` and + * `\DateTimeImmutable`, and userland cannot implement it directly — which + * makes it an exact fit for "is this a date/time class" with no ancestry + * list to maintain. + */ + private const string DATE_TIME_ANCHOR = DateTimeInterface::class; + + private const string DATE_FACADE = Date::class; + + /** + * Static factory methods that interpret a STRING, mapped to the SLOT that + * carries it — the accepted parameter spellings and the position. The names + * alone do not discriminate: `create`, `createFromDate`, `createFromTime`, + * `createStrict` and `createSafe` also accept integer components, `make` + * and `parse` also re-wrap an existing value, and every one of them is a + * clock read with no argument at all — Carbon's `create()` delegates to + * `parse()` exactly when its `$year` is a non-numeric string. The gate in + * `decodedSlotMayBeAString()` is what turns a name here into a finding, so + * the list stays wide and the gate stays narrow. + * + * The slot is NOT argument zero for the `*FromFormat` family: the format + * string sits there and the decoded value is the `$time` after it, two + * places along for the locale-aware pair. `createFromFormat` carries two + * spellings because Carbon names that parameter `$time` and the two PHP + * natives name it `$datetime`; every slot here is pinned against the real + * signatures in `testEveryDecodedSlotMatchesTheParameterItNames`. + * + * The list stays wide, and what keeps it COMPLETE is not care: + * `testEveryCarbonStaticFactoryIsClassifiedAsDecodingOrNot` reflects + * Carbon's real static surface and fails on any factory that is neither a + * key here nor a key of `NON_DECODING_FACTORIES`. Three methods were missing + * while the list was maintained by hand: a reviewer named `parseFromLocale` + * and `createMidnightDate`, and a reflection sweep over the same surface + * then found `rawCreateFromFormat`, which nobody reading the list had seen. + * That is what moved the completeness claim out of this docblock and into a + * test. + * + * Keys are lower-case because PHP dispatches a static method + * case-insensitively and the lookup folds the written identifier to match. + * + * @var array, 1: int}> + */ + private const array PARSING_METHODS = [ + 'parse' => [['time'], 0], + 'rawparse' => [['time'], 0], + 'parsefromlocale' => [['time'], 0], + 'createfromformat' => [['time', 'datetime'], 1], + 'rawcreatefromformat' => [['time'], 1], + 'createfromisoformat' => [['time'], 1], + 'createfromlocaleformat' => [['time'], 2], + 'createfromlocaleisoformat' => [['time'], 2], + 'createfromtimestring' => [['time'], 0], + 'createfromdate' => [['year'], 0], + 'createmidnightdate' => [['year'], 0], + 'createfromtime' => [['hour'], 0], + 'create' => [['year'], 0], + 'make' => [['var'], 0], + 'createstrict' => [['year'], 0], + 'createsafe' => [['year'], 0], + ]; + + /** + * The slot `new DateTimeImmutable(...)` / `new CarbonImmutable(...)` + * decodes. Carbon spells that constructor parameter `$time`, the two PHP + * natives spell it `$datetime`, and `new CarbonImmutable(timezone: $tz)` is + * "now in a zone" with nothing in the slot at all. + * + * @var array{0: list, 1: int} + */ + private const array CONSTRUCTOR_SLOT = [['time', 'datetime'], 0]; + + /** + * Procedural equivalents of the same decode, mapped to the same kind of + * slot — including the two `*_from_format` aliases of `createFromFormat`, + * the method the seed measured as the second-largest offender, which a list + * without them would have left a one-token escape hatch for. Every one of + * these spells its decoded parameter `$datetime`; the three `*_from_format` + * shapes carry it after the format, at index 1. + * + * @var array, 1: int}> + */ + private const array PARSING_FUNCTIONS = [ + 'strtotime' => [['datetime'], 0], + 'date_create' => [['datetime'], 0], + 'date_create_immutable' => [['datetime'], 0], + 'date_create_from_format' => [['datetime'], 1], + 'date_create_immutable_from_format' => [['datetime'], 1], + 'date_parse' => [['datetime'], 0], + 'date_parse_from_format' => [['datetime'], 1], + ]; + + /** @var list */ + private array $dateParsingNamespaces; + + /** + * @param list $dateParsingNamespaces namespace prefixes inside + * which a date/time string may + * legitimately be decoded. A + * namespace matches when it + * equals a prefix or continues + * it across a separator, so + * sub-namespaces match and + * `App\CastsReport` does not. + * The default names the two + * boundary doors of ADR-0020 + * Amd 1 — a dedicated decode + * helper and the row-to-model + * cast. + */ + public function __construct( + private ReflectionProvider $reflectionProvider, + array $dateParsingNamespaces = ['App\Support\Time', 'App\Support\DateTime', 'App\Casts'], + ) { + // A configured `App\Casts\` and `App\Casts` name the same boundary. + // Normalising once here keeps the separator test below from treating + // the trailing form as a second, never-matching spelling. + $this->dateParsingNamespaces = array_map( + static fn(string $prefix): string => mb_rtrim($prefix, '\\'), + $dateParsingNamespaces, + ); + } + + public function getNodeType(): string + { + return CallLike::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if ($this->insideBoundaryNamespace($scope)) { + return []; + } + + if ($node instanceof StaticCall) { + return $this->processStaticCall($node, $scope); + } + + if ($node instanceof New_) { + return $this->processNew($node, $scope); + } + + if ($node instanceof FuncCall) { + return $this->processFuncCall($node, $scope); + } + + return []; + } + + /** + * Containing-class gate. A class whose namespace starts with a configured + * prefix is where the decode BELONGS, so the rule stays silent there and + * nowhere else. + */ + private function insideBoundaryNamespace(Scope $scope): bool + { + $namespace = $scope->getNamespace(); + + if ($namespace === null) { + return false; + } + + foreach ($this->dateParsingNamespaces as $prefix) { + // The separator is what makes this a NAMESPACE test rather than a + // character-prefix test: without it `App\CastsReport` is inside + // `App\Casts` and every namespace sharing an opening substring + // with a boundary silently inherits its exemption. + if ($namespace === $prefix || str_starts_with($namespace, $prefix . '\\')) { + return true; + } + } + + return false; + } + + /** + * @return list + */ + private function processStaticCall(StaticCall $node, Scope $scope): array + { + if (!$node->name instanceof Identifier) { + return []; + } + + $method = $node->name->toString(); + + // PHP dispatches `PARSE()` and `parse()` to the same method, so the + // written identifier is folded before the lookup. It is still what the + // error reports — that is the token the reader has to go and change. + $folded = mb_strtolower($method); + + if (!array_key_exists($folded, self::PARSING_METHODS)) { + return []; + } + + [$names, $position] = self::PARSING_METHODS[$folded]; + + if (!$this->decodedSlotMayBeAString($node, $scope, $names, $position)) { + return []; + } + + $class = $this->resolveClassName($node->class, $scope); + + if ($class === null) { + return []; + } + + if ($class !== self::DATE_FACADE && !$this->isDateTimeClass($class)) { + return []; + } + + return [$this->buildError(sprintf('%s::%s()', $this->shortName($class), $method))]; + } + + /** + * @return list + */ + private function processNew(New_ $node, Scope $scope): array + { + [$names, $position] = self::CONSTRUCTOR_SLOT; + + if (!$this->decodedSlotMayBeAString($node, $scope, $names, $position)) { + return []; + } + + $class = $this->resolveClassName($node->class, $scope); + + if ($class === null || !$this->isDateTimeClass($class)) { + return []; + } + + return [$this->buildError(sprintf('new %s()', $this->shortName($class)))]; + } + + /** + * @return list + */ + private function processFuncCall(FuncCall $node, Scope $scope): array + { + if (!$node->name instanceof Name) { + return []; + } + + // The callee is RESOLVED, never read off the token. An unqualified + // call inside a namespace resolves to a same-namespace declaration + // when one exists, so a local `strtotime()` helper is not the global + // function and must not fire; `use function strtotime as decode;` + // is the same question from the other side. + if (!$this->reflectionProvider->hasFunction($node->name, $scope)) { + return []; + } + + $function = mb_strtolower($this->reflectionProvider->getFunction($node->name, $scope)->getName()); + + if (!array_key_exists($function, self::PARSING_FUNCTIONS)) { + return []; + } + + [$names, $position] = self::PARSING_FUNCTIONS[$function]; + + if (!$this->decodedSlotMayBeAString($node, $scope, $names, $position)) { + return []; + } + + return [$this->buildError(sprintf('%s()', $function))]; + } + + /** + * The argument gate shared by all three shapes: a call decodes only if the + * argument in its decoded slot is present and the analyser cannot prove it + * is NOT a string. Absent argument, integer components, `null` and an + * existing value object all resolve `isString()` to "no" and are silent; + * `string`, `mixed`, `int|string` and `?string` are not provably + * non-string and fire. + * + * `getArgs()` is safe to call unguarded even though it asserts + * `!isFirstClassCallable()`: PHPStan substitutes `StaticMethodCallableNode` + * / `FunctionCallableNode` / `MethodCallableNode` for a first-class + * callable, and none of those extends `CallLike`, so this rule's + * registration cannot receive one. A guard here would be unreachable code + * no test could pin and every mutation of it would escape. The upstream + * substitution is asserted in the rule test instead. + * + * @param list $names + */ + private function decodedSlotMayBeAString(CallLike $node, Scope $scope, array $names, int $position): bool + { + $argument = $this->argumentAt($node, $names, $position); + + if ($argument === null) { + return false; + } + + return !$scope->getType($argument->value)->isString()->no(); + } + + /** + * One argument, addressed by NAME first and by position second — the reader + * `ForbidCredentialCastBypassRule::argumentAt()` uses, for the same reason. + * + * A named argument does not sit at its parameter's position: + * `create(month: 1, year: $raw)` puts the decoded value at index 1, so + * reading index 0 finds an integer and the parse passes silently, while + * `create(timezone: 'Europe/Amsterdam')` puts a string at index 0 that is + * not a date input at all and is reported for it. + * + * @param list $names accepted spellings of the parameter, because + * Carbon and the two PHP natives disagree on + * `$time` versus `$datetime` + */ + private function argumentAt(CallLike $node, array $names, int $position): ?Arg + { + $args = $node->getArgs(); + + foreach ($args as $argument) { + if ($argument->name instanceof Identifier && in_array($argument->name->toString(), $names, true)) { + return $argument; + } + } + + // PHP requires every positional argument before the first named one, so + // positional slots are contiguous from zero and this index is only + // meaningful when the argument sitting there is itself positional. + // Checking the slot rather than refusing whenever ANY argument is named + // keeps `parse($raw, timezone: 'UTC')` covered. + $argument = $args[$position] ?? null; + + return $argument !== null && $argument->name === null ? $argument : null; + } + + /** + * Resolves a class-position node to an FQCN through the SCOPE, so an + * aliased import (`use Carbon\CarbonImmutable as C;`) resolves to the real + * class and a dynamic expression resolves to nothing. + */ + private function resolveClassName(Node $class, Scope $scope): ?string + { + if (!$class instanceof Name) { + return null; + } + + return $scope->resolveName($class); + } + + private function isDateTimeClass(string $class): bool + { + return (new ObjectType(self::DATE_TIME_ANCHOR))->isSuperTypeOf(new ObjectType($class))->yes(); + } + + private function shortName(string $class): string + { + $position = mb_strrpos($class, '\\'); + + return $position === false ? $class : mb_substr($class, $position + 1); + } + + private function buildError(string $call): IdentifierRuleError + { + return RuleErrorBuilder::message(sprintf( + 'Ad-hoc date parsing (%s) outside %s: decode the string once at the boundary and pass the value object (ADR-0020 Amd 1).', + $call, + implode(', ', $this->dateParsingNamespaces), + )) + ->identifier(self::IDENTIFIER) + ->build(); + } +} diff --git a/tests/Fixtures/AdHocDateParsing/AliasedCarbon.php b/tests/Fixtures/AdHocDateParsing/AliasedCarbon.php new file mode 100644 index 0000000..810d80d --- /dev/null +++ b/tests/Fixtures/AdHocDateParsing/AliasedCarbon.php @@ -0,0 +1,15 @@ + + */ + public function firstClassCallables(): array + { + return [ + CarbonImmutable::parse(...), + CarbonImmutable::createFromFormat(...), + strtotime(...), + date_create(...), + ]; + } +} diff --git a/tests/Fixtures/AdHocDateParsing/CreateFromFormatInRequest.php b/tests/Fixtures/AdHocDateParsing/CreateFromFormatInRequest.php new file mode 100644 index 0000000..4c04d2e --- /dev/null +++ b/tests/Fixtures/AdHocDateParsing/CreateFromFormatInRequest.php @@ -0,0 +1,18 @@ + + */ + public function toDto(string $raw): array + { + return ['from' => Carbon::createFromFormat('Y-m-d', $raw)]; + } +} diff --git a/tests/Fixtures/AdHocDateParsing/DateFacade.php b/tests/Fixtures/AdHocDateParsing/DateFacade.php new file mode 100644 index 0000000..e13bbc9 --- /dev/null +++ b/tests/Fixtures/AdHocDateParsing/DateFacade.php @@ -0,0 +1,15 @@ +getNamespace()` is null, which is outside +// every configured prefix, so the rule fires. Consumers analyse `app/`, which +// is namespaced throughout — a global-namespace parse is by definition not +// inside a boundary namespace. + +final class GlobalNamespaceParse +{ + public function execute(string $raw): CarbonImmutable + { + return CarbonImmutable::parse($raw); + } +} diff --git a/tests/Fixtures/AdHocDateParsing/LocalClassNamedCarbon.php b/tests/Fixtures/AdHocDateParsing/LocalClassNamedCarbon.php new file mode 100644 index 0000000..f95aa4c --- /dev/null +++ b/tests/Fixtures/AdHocDateParsing/LocalClassNamedCarbon.php @@ -0,0 +1,31 @@ +input()` parse. + */ +final class MaybeStringFirstArgument +{ + public function untypedInput(mixed $raw): void + { + CarbonImmutable::create($raw); + CarbonImmutable::createFromDate($raw); + CarbonImmutable::make($raw); + } + + public function unionInput(int|string $yearOrDate, ?string $maybe): void + { + CarbonImmutable::create($yearOrDate); + CarbonImmutable::parse($maybe); + + new CarbonImmutable($maybe); + } +} diff --git a/tests/Fixtures/AdHocDateParsing/NamedArguments.php b/tests/Fixtures/AdHocDateParsing/NamedArguments.php new file mode 100644 index 0000000..388cba5 --- /dev/null +++ b/tests/Fixtures/AdHocDateParsing/NamedArguments.php @@ -0,0 +1,37 @@ +addDays(1)->startOfDay()->format('Y-m-d'); + } +} diff --git a/tests/Fixtures/AdHocDateParsing/ParsesInAction.php b/tests/Fixtures/AdHocDateParsing/ParsesInAction.php new file mode 100644 index 0000000..ce4bef9 --- /dev/null +++ b/tests/Fixtures/AdHocDateParsing/ParsesInAction.php @@ -0,0 +1,15 @@ + + */ +final class ForbidAdHocDateParsingRuleTest extends RuleTestCase +{ + private const string DEFAULT_ALLOWED = 'App\Support\Time, App\Support\DateTime, App\Casts'; + + private const string CONFIGURED_ALLOWED = 'App\Domain\Clock'; + + /** + * The classes whose static surface the completeness gate must partition. + * `Illuminate\Support\Carbon` earns its place the same way the two + * `Carbon\*` classes do — the rule fires on it, because it is a + * `DateTimeInterface` subtype and it is what the `Date` facade resolves to, + * so a factory added there would evade the rule exactly as one added to + * Carbon would. + * + * @var list + */ + private const array FACTORY_DECLARERS = [ + Carbon::class, + CarbonImmutable::class, + IlluminateCarbon::class, + ]; + + /** + * Override hook: when set, `getRule()` returns this instance instead of the + * default, so one test can reconfigure `dateParsingNamespaces` or pull the + * rule out of the PHPStan container. + */ + private ?Rule $ruleOverride = null; + + public function testFlagsParseInsideAnAction(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/ParsesInAction.php'], + [[self::expected('CarbonImmutable::parse()'), 13]], + ); + } + + public function testFlagsCreateFromFormatInsideAFormRequest(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/CreateFromFormatInRequest.php'], + [[self::expected('Carbon::createFromFormat()'), 16]], + ); + } + + /** + * Resolution is through the SCOPE, so an aliased import reports under the + * class it actually names. A rule matching the written token would both + * miss this call and report a useless `C::parse()`. + */ + public function testFlagsAnAliasedCarbonImport(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/AliasedCarbon.php'], + [[self::expected('CarbonImmutable::parse()'), 13]], + ); + } + + public function testFlagsTheDateFacade(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/DateFacade.php'], + [[self::expected('Date::parse()'), 13]], + ); + } + + public function testFlagsNativeConstructionWithAnArgument(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/NewDateTimeWithArgument.php'], + [[self::expected('new DateTimeImmutable()'), 13]], + ); + } + + public function testFlagsStrtotimeInAService(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/StrtotimeInService.php'], + [[self::expected('strtotime()'), 11]], + ); + } + + public function testIgnoresParsingInsideTheSupportTimeBoundary(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/ParsesInSupportTime.php'], + [], + ); + } + + public function testIgnoresParsingInsideACast(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/ParsesInCast.php'], + [], + ); + } + + /** + * Clock reads, timestamp factories, zero-argument construction, instance + * calls on an already-decoded value and a non-listed static helper handed + * a string all sit in a NON-boundary namespace in this fixture, so what + * holds them back is the method set and the argument gate — not the + * namespace gate. The string-taking helper is what keeps the method set + * load-bearing: without it, the argument gate alone would silence every + * other call here and the name check could be deleted unnoticed. + */ + public function testIgnoresClockReadsTimestampFactoriesAndInstanceCalls(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/NowAndTimestamp.php'], + [], + ); + } + + /** + * A class in the global namespace is outside every configured prefix, so + * the rule fires. This also pins the `getNamespace() === null` branch, which + * a prefix-matching loop alone would never reach. + */ + public function testFlagsParsingInTheGlobalNamespace(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/GlobalNamespaceParse.php'], + [[self::expected('CarbonImmutable::parse()'), 15]], + ); + } + + /** + * The DECLINE paths, asserted in a NON-boundary namespace so the namespace + * gate is not what is keeping them quiet: a dynamic class expression + * (`$class::parse()`, `new $class()`), a dynamic method name + * (`CarbonImmutable::{$method}()`), a dynamic function name (`$fn()`), and + * ordinary calls on a class that is not a date at all. Each is an accepted + * false negative — the receiver has no resolvable name and guessing one + * would be a false-positive source a boundary rule cannot afford. + */ + public function testDeclinesDynamicExpressionsAndUnrelatedCalls(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/DynamicAndUnrelatedCalls.php'], + [], + ); + } + + /** + * The argument gate, negative half. Every call here names a method or + * function the rule lists, in a non-boundary namespace, and none of them + * receives a string or anything that could be one: integer components, + * an existing value object, a `null`, or no argument at all. Removing the + * gate reds this test; so does narrowing it back to a zero-argument check. + */ + public function testIgnoresComponentFactoriesRewrapsAndZeroArgumentCalls(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/ComponentFactoriesAndRewraps.php'], + [], + ); + } + + /** + * The argument gate, positive half and its DIRECTION: a first argument the + * analyser cannot prove is NOT a string (`mixed`, `int|string`, `?string`) + * fires. A gate that required a proven string instead would exempt every + * untyped `$request->input()` parse — this test is what makes that swap + * red rather than silent. + */ + public function testFlagsAFirstArgumentThatMayBeAString(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/MaybeStringFirstArgument.php'], + [ + [self::expected('CarbonImmutable::create()'), 19], + [self::expected('CarbonImmutable::createFromDate()'), 20], + [self::expected('CarbonImmutable::make()'), 21], + [self::expected('CarbonImmutable::create()'), 26], + [self::expected('CarbonImmutable::parse()'), 27], + [self::expected('new CarbonImmutable()'), 29], + ], + ); + } + + public function testIgnoresALocalClassMerelyNamedCarbon(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/LocalClassNamedCarbon.php'], + [], + ); + } + + /** + * Function names are resolved, not read off the token. PHP resolves an + * unqualified call inside a namespace to a same-namespace declaration when + * one exists, and `use function … as …` gives the global function a local + * spelling — so the written token both over- and under-reports which global + * function is being called. + */ + public function testResolvesFunctionNamesRatherThanMatchingTheWrittenToken(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/ResolvedFunctionNames.php'], + [[self::expected('strtotime()'), 34]], + ); + } + + /** + * The decoded slot is addressed by parameter NAME first and by position + * second. Reading argument zero in source order reports + * `create(timezone: …)`, which hands the call no date input at all, and + * stays silent on `create(month: 1, year: $raw)`, which hands it a string. + */ + public function testReadsTheDecodedSlotByParameterName(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/NamedArguments.php'], + [ + [self::expected('CarbonImmutable::create()'), 27], + [self::expected('CarbonImmutable::parse()'), 28], + [self::expected('strtotime()'), 30], + ], + ); + } + + /** + * PHP dispatches a static method case-insensitively, so the comparison + * folds case. The call is still reported with the casing the source wrote, + * which is where the reader has to go to fix it. + */ + public function testFoldsTheCaseOfTheStaticMethodName(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/UppercaseMethodName.php'], + [ + [self::expected('CarbonImmutable::PARSE()'), 18], + [self::expected('CarbonImmutable::CreateFromFormat()'), 19], + ], + ); + } + + /** + * The boundary prefix matches on a namespace SEPARATOR, so a namespace that + * merely starts with the same characters is not inside it. + */ + public function testANamespaceSharingAPrefixIsNotInsideTheBoundary(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/NamespacePrefixCollision.php'], + [[self::expected('CarbonImmutable::parse()'), 19]], + ); + } + + /** + * Control for the pair above: a real sub-namespace of a configured boundary + * keeps its exemption. Without this, tightening the prefix test to an exact + * match would pass the collision case and silently cost every consumer its + * documented sub-namespace behaviour. + */ + public function testARealSubNamespaceOfTheBoundaryStaysExempt(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/NestedCastNamespace.php'], + [], + ); + } + + /** + * The denominator assertion. Every entry of both lists appears exactly once + * in the fixture, so dropping one — by hand or by a mutation operator that + * removes an array item — fails here at a named line instead of silently + * narrowing what the rule can see. + */ + public function testFlagsEveryParsingMethodAndFunction(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/AllParsingMethods.php'], + [ + [self::expected('CarbonImmutable::parse()'), 31], + [self::expected('CarbonImmutable::rawParse()'), 32], + [self::expected('CarbonImmutable::parseFromLocale()'), 33], + [self::expected('CarbonImmutable::createFromFormat()'), 34], + [self::expected('CarbonImmutable::rawCreateFromFormat()'), 35], + [self::expected('CarbonImmutable::createFromIsoFormat()'), 36], + [self::expected('CarbonImmutable::createFromLocaleFormat()'), 37], + [self::expected('CarbonImmutable::createFromLocaleIsoFormat()'), 38], + [self::expected('CarbonImmutable::createFromTimeString()'), 39], + [self::expected('CarbonImmutable::createFromDate()'), 40], + [self::expected('CarbonImmutable::createMidnightDate()'), 41], + [self::expected('CarbonImmutable::createFromTime()'), 42], + [self::expected('CarbonImmutable::create()'), 43], + [self::expected('CarbonImmutable::make()'), 44], + [self::expected('CarbonImmutable::createStrict()'), 45], + [self::expected('CarbonImmutable::createSafe()'), 46], + [self::expected('strtotime()'), 51], + [self::expected('date_create()'), 52], + [self::expected('date_create_immutable()'), 53], + [self::expected('date_parse()'), 54], + [self::expected('date_parse_from_format()'), 55], + [self::expected('date_create_from_format()'), 56], + [self::expected('date_create_immutable_from_format()'), 57], + [self::expected('new DateTime()'), 62], + [self::expected('new DateTimeImmutable()'), 63], + [self::expected('DateTime::createFromFormat()'), 64], + ], + ); + } + + /** + * Configuration half 1: with `dateParsingNamespaces` pointed elsewhere, the + * default boundary namespace loses its exemption and every call in it + * fires. Without this the parameter could be ignored entirely and every + * other assertion in this file would still pass. + */ + public function testConfiguredNamespacesReplaceTheDefaultAndExposeIt(): void + { + $this->ruleOverride = new ForbidAdHocDateParsingRule(self::createReflectionProvider(), ['App\Domain\Clock']); + + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/ParsesInSupportTime.php'], + [ + [self::expected('CarbonImmutable::parse()', self::CONFIGURED_ALLOWED), 19], + [self::expected('CarbonImmutable::createFromFormat()', self::CONFIGURED_ALLOWED), 24], + [self::expected('new DateTimeImmutable()', self::CONFIGURED_ALLOWED), 29], + [self::expected('strtotime()', self::CONFIGURED_ALLOWED), 34], + ], + ); + } + + /** + * Configuration half 2: the same override silences the namespace it names. + * Half 1 alone is satisfied by a rule that ignores the parameter and simply + * has no exemptions at all. + */ + public function testConfiguredNamespaceSilencesItsOwnClasses(): void + { + $this->ruleOverride = new ForbidAdHocDateParsingRule(self::createReflectionProvider(), ['App\Domain\Clock']); + + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/ParsesInDomainClock.php'], + [], + ); + } + + /** + * Control for the pair above: under the SHIPPED default the very same + * fixture fires, so half 2's silence is the override talking and not a + * fixture that could never have been flagged. + */ + public function testTheConfigurationFixtureFiresUnderTheShippedDefault(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/ParsesInDomainClock.php'], + [[self::expected('CarbonImmutable::parse()'), 19]], + ); + } + + /** + * End-to-end pin on the `extension.neon` registration path consumers + * actually use: resolve the rule from the PHPStan container so the shipped + * `dateParsingNamespaces` default and the `%dateParsingNamespaces%` argument + * wiring are exercised, NOT the PHP constructor default. A NEON quoting + * regression in the shipped list silently un-exempts every boundary + * namespace for every default consumer; this catches it by asserting the + * boundary fixture is still silent while a non-boundary one still fires. + */ + public function testRuleResolvesFromExtensionNeonWithTheShippedDefault(): void + { + $this->ruleOverride = self::getContainer()->getByType(ForbidAdHocDateParsingRule::class); + + $this->analyse( + [ + __DIR__ . '/../Fixtures/AdHocDateParsing/ParsesInSupportTime.php', + __DIR__ . '/../Fixtures/AdHocDateParsing/ParsesInAction.php', + ], + [[self::expected('CarbonImmutable::parse()'), 13]], + ); + } + + /** + * Every decoded slot names a parameter that EXISTS on the real signature, + * at the position the rule reads, under one of the spellings it accepts. + * + * The rule addresses the slot by name first and position second, so each + * entry is three claims about `nesbot/carbon` and the two PHP natives: that + * a parameter sits at that index, that its name is one the rule would + * match, and — checked in the other direction — that every accepted + * spelling is earned by a real signature rather than left behind by an + * edit. A Carbon rename would break the named lookup silently: the rule + * would simply stop seeing named arguments, with no error anywhere, and + * every fixture here would stay green because they call these methods + * positionally. + * + * The slot positions are the reason this cannot be eyeballed: + * `createFromFormat` decodes its SECOND parameter and the locale-aware pair + * decode their THIRD, so "argument zero" is wrong for five of the thirteen + * methods and three of the seven functions. + */ + public function testEveryDecodedSlotMatchesTheParameterItNames(): void + { + $declarers = [CarbonImmutable::class, Carbon::class, DateTime::class, DateTimeImmutable::class]; + $covered = []; + + foreach (self::ruleSlots('PARSING_METHODS') as $method => [$names, $position]) { + $seen = []; + + foreach ($declarers as $class) { + $reflection = new ReflectionClass($class); + + if (!$reflection->hasMethod($method)) { + continue; + } + + $seen[$this->assertSlotParameter($reflection->getMethod($method)->getParameters(), $names, $position, sprintf('%s::%s()', $class, $method))] = true; + } + + self::assertNotSame( + [], + $seen, + sprintf('No date/time class declares %s(), so the rule reads a slot from a method that does not exist.', $method), + ); + + $this->assertEverySpellingIsEarned($names, array_keys($seen), $method); + + $covered[$method] = true; + } + + // Denominator: the loop above visited every entry, so an entry that + // silently stopped being iterated fails here rather than passing as a + // clean run over a shorter map. + self::assertSame(array_keys(self::ruleSlots('PARSING_METHODS')), array_keys($covered)); + + foreach (self::ruleSlots('PARSING_FUNCTIONS') as $function => [$names, $position]) { + self::assertTrue(function_exists($function), sprintf('The rule reads a slot from %s(), which does not exist.', $function)); + + $actual = $this->assertSlotParameter((new ReflectionFunction($function))->getParameters(), $names, $position, sprintf('%s()', $function)); + + $this->assertEverySpellingIsEarned($names, [$actual], $function); + } + + [$names, $position] = self::ruleConstant('CONSTRUCTOR_SLOT'); + $seen = []; + + foreach ($declarers as $class) { + $constructor = (new ReflectionClass($class))->getConstructor(); + + self::assertNotNull($constructor, sprintf('%s has no constructor, so the New_ slot reads nothing.', $class)); + + $seen[$this->assertSlotParameter($constructor->getParameters(), $names, $position, sprintf('new %s()', $class))] = true; + } + + $this->assertEverySpellingIsEarned($names, array_keys($seen), 'the constructor slot'); + } + + /** + * The generator gate. Every method list on this rule was maintained BY HAND, + * and four consecutive review rounds on its pull request each found a true + * defect in one — the fourth naming two static factories `PARSING_METHODS` + * had never carried (`parseFromLocale`, `createMidnightDate`), with a + * reflection sweep over the same surface immediately turning up a third + * (`rawCreateFromFormat`) that nobody reading the list had seen. Reading the + * list harder is not the fix. The fix is that the list stops being a claim. + * + * So: reflect Carbon's REAL static factory surface and require every method + * on it to be classified — either a key of `PARSING_METHODS`, meaning it + * interprets a string and the rule reports it, or a key of + * `NON_DECODING_FACTORIES`, meaning it does not and the rule is silent for a + * stated reason. A method in neither fails here BY NAME, with both places it + * could go. A Carbon release that adds a factory then reds this build + * instead of opening a silent hole in the rule. + * + * The classification is deliberately conservative in ONE direction: a method + * whose returned shape cannot be read at all — no declared return type and + * no `@return` — counts as a factory and must be classified. + * `createFromImmutable` and `createFromMutable` are exactly that case. Being + * wrong that way costs one allowlist row; being wrong the other way is the + * hole this test exists to close. + */ + public function testEveryCarbonStaticFactoryIsClassifiedAsDecodingOrNot(): void + { + $decoding = self::ruleSlots('PARSING_METHODS'); + $allowed = self::ruleAllowlist(); + + self::assertNotSame([], $decoding, 'PARSING_METHODS is empty, so this test partitions nothing.'); + self::assertNotSame([], $allowed, 'NON_DECODING_FACTORIES is empty, so every factory would have to be a decoder.'); + self::assertSame( + [], + array_intersect_key($decoding, $allowed), + 'A method is listed as BOTH decoding and non-decoding, so the two constants no longer partition the surface.', + ); + + $factories = []; + $unclassified = []; + + foreach (self::FACTORY_DECLARERS as $class) { + foreach ((new ReflectionClass($class))->getMethods(ReflectionMethod::IS_PUBLIC) as $method) { + if (!$method->isStatic() || !self::returnsAnInstance($method)) { + continue; + } + + $folded = mb_strtolower($method->getName()); + $factories[$folded] = true; + + if (array_key_exists($folded, $decoding) || array_key_exists($folded, $allowed)) { + continue; + } + + // First declarer wins, so the message names the class the + // method is declared on rather than the last one to inherit it. + $unclassified[$folded] ??= sprintf('%s::%s()', $class, $method->getName()); + } + } + + // Non-empty denominator. A reflection sweep that returned nothing would + // report a clean partition of an empty set — the exact output shape of a + // broken instrument. Carbon's real surface is comfortably over this + // floor, so the number only ever moves when the sweep breaks. + self::assertGreaterThanOrEqual( + 20, + count($factories), + sprintf( + 'Only %d static factories were reflected off %s, so the sweep is broken rather than the lists complete.', + count($factories), + implode(', ', self::FACTORY_DECLARERS), + ), + ); + + self::assertSame( + [], + array_values($unclassified), + 'A Carbon static factory is classified by neither constant on ForbidAdHocDateParsingRule. Add it to PARSING_METHODS with the slot that carries the decoded string if it interprets one, or to NON_DECODING_FACTORIES with the reason it does not.', + ); + } + + /** + * The allowlist's own gate, and the reason `NON_DECODING_FACTORIES` cannot + * be used to silence a decoder. The test above is satisfied by ANY + * classification, including a wrong one: moving `parse` down into the + * allowlist would make it pass while the rule stopped reporting the single + * most common decode in the fleet. + * + * The discriminator is the one the rule itself uses — the parameter NAME. A + * decoding factory names its input with one of the slot spellings + * `PARSING_METHODS` reads (`$time`, `$datetime`, `$year`, `$hour`, `$var`) + * and types it so a string fits. A type-only check could not be written: + * `now(DateTimeZone|string|int|null $timezone)` and + * `createFromTimestamp(string|int|float $timestamp)` both accept a string in + * their first slot and both belong on the allowlist, so a check that read + * types alone would have to reject them. + */ + public function testNoAllowedFactoryTakesAStringInADecodedSlot(): void + { + $spellings = self::decodedSlotSpellings(); + + self::assertNotSame([], $spellings, 'No slot spellings were read off the rule, so this check compares against nothing.'); + + $unmatched = []; + + foreach (array_keys(self::ruleAllowlist()) as $allowed) { + $found = false; + + foreach (self::FACTORY_DECLARERS as $class) { + $reflection = new ReflectionClass($class); + + if (!$reflection->hasMethod($allowed)) { + continue; + } + + $found = true; + + foreach ($reflection->getMethod($allowed)->getParameters() as $parameter) { + if (!in_array(mb_strtolower($parameter->getName()), $spellings, true)) { + continue; + } + + self::assertFalse( + self::admitsAString($parameter), + sprintf( + '%s::%s() is on NON_DECODING_FACTORIES, but its $%s parameter is a decoded slot that accepts a string. Either it decodes — move it to PARSING_METHODS — or the allowlist is being used to silence a decoder.', + $class, + $allowed, + $parameter->getName(), + ), + ); + } + } + + if (!$found) { + $unmatched[] = $allowed; + } + } + + // Per-ENTRY denominator, not a total: a count would stay satisfied while + // one entry matched nothing and another matched three classes, which is + // how a skipped loop passes for the wrong reason. + self::assertSame( + [], + $unmatched, + 'An entry of NON_DECODING_FACTORIES names no method on any reflected class, so it was never checked and is dead configuration.', + ); + } + + /** + * A configured prefix that already ends in a separator names the same + * boundary as one that does not. Without the normalisation the separator + * test appends a second backslash and the trailing spelling matches + * nothing — a silently disarmed exemption for every consumer that writes it + * that way. + */ + public function testATrailingSeparatorInAConfiguredPrefixNamesTheSameBoundary(): void + { + $this->ruleOverride = new ForbidAdHocDateParsingRule(self::createReflectionProvider(), ['App\Domain\Clock\\']); + + $this->analyse( + [__DIR__ . '/../Fixtures/AdHocDateParsing/ParsesInDomainClock.php'], + [], + ); + } + + /** + * The rule anchors on `DateTimeInterface` alone and relies on + * `CarbonInterface` extending it to cover the whole Carbon family. That is + * a fact about an upstream package, not about this code: a Carbon release + * that stopped extending it would narrow the rule to the two PHP natives + * with every fixture above still green, because each of them names a Carbon + * class the analyser would then simply not recognise. Asserting it here is + * what turns that into a red build. + */ + public function testCarbonInterfaceExtendsTheDateTimeAnchor(): void + { + self::assertTrue( + is_subclass_of(CarbonInterface::class, DateTimeInterface::class), + 'Carbon\CarbonInterface no longer extends DateTimeInterface, so ForbidAdHocDateParsingRule::DATE_TIME_ANCHOR no longer covers the Carbon family. Add an explicit Carbon anchor to the rule.', + ); + } + + /** + * `CallLike::getArgs()` asserts `!isFirstClassCallable()` and returns raw + * arguments otherwise, so reaching it with `Carbon::parse(...)` would be an + * AssertionError under `zend.assertions=1` and a read of an undefined + * property on a `VariadicPlaceholder` without it. Nothing in the rule + * guards against that, and nothing needs to: PHPStan substitutes a + * dedicated node for every first-class callable, and none of those nodes is + * a `CallLike`, so the rule's own registration is what makes the argument + * gate unreachable with one. That is a fact about PHPStan, not about this + * code — a release that made those nodes `CallLike` would hand the rule a + * shape it cannot read, with the fixture tripwire still green because the + * rule would crash before reporting anything. This is what turns that into + * a red build. + * + * @param class-string $node + */ + #[DataProvider('firstClassCallableNodes')] + public function testPhpstanFirstClassCallableNodesAreNotCallLike(string $node): void + { + self::assertTrue(class_exists($node), sprintf('%s no longer exists in PHPStan.', $node)); + + self::assertFalse( + is_a($node, CallLike::class, true), + sprintf( + '%s is now a CallLike, so PHPStan can hand a first-class callable to ForbidAdHocDateParsingRule and firstArgumentMayBeAString() will call getArgs() on it. Guard the choke point with isFirstClassCallable().', + $node, + ), + ); + } + + /** + * @return iterable + */ + public static function firstClassCallableNodes(): iterable + { + yield 'static method' => [StaticMethodCallableNode::class]; + + yield 'function' => [FunctionCallableNode::class]; + + yield 'instance method' => [MethodCallableNode::class]; + } + + /** + * Load the shipped `extension.neon` so the container-resolution test can + * pull the rule out with its NEON-configured parameter applied. + * + * @return array + */ + public static function getAdditionalConfigFiles(): array + { + return [ + __DIR__ . '/../../extension.neon', + ]; + } + + protected function getRule(): Rule + { + return $this->ruleOverride ?? new ForbidAdHocDateParsingRule(self::createReflectionProvider()); + } + + /** + * The parameter at one slot, asserted to exist and to carry a spelling the + * rule would match. Returns the spelling it found, so the caller can check + * the other direction. + * + * @param array $parameters + * @param list $names + */ + private function assertSlotParameter(array $parameters, array $names, int $position, string $subject): string + { + self::assertArrayHasKey($position, $parameters, sprintf('%s has no parameter at position %d.', $subject, $position)); + + $actual = $parameters[$position]->getName(); + + self::assertContains( + $actual, + $names, + sprintf( + '%s parameter %d is $%s, but the rule accepts only $%s there — a named argument would silently never match.', + $subject, + $position, + $actual, + implode(' / $', $names), + ), + ); + + return $actual; + } + + /** + * The other direction: a spelling the rule accepts that no real signature + * uses is dead configuration, and it hides the day the live spelling + * changed underneath it. + * + * @param list $names + * @param list $seen + */ + private function assertEverySpellingIsEarned(array $names, array $seen, string $subject): void + { + sort($names); + sort($seen); + + self::assertSame( + $names, + $seen, + sprintf('The rule accepts $%s for %s, but the real signatures only ever spell it $%s.', implode(' / $', $names), $subject, implode(' / $', $seen)), + ); + } + + /** + * A slot map read off the rule rather than restated — a copy here would + * drift and this test would then verify the copy. + * + * @return array, 1: int}> + */ + private static function ruleSlots(string $constant): array + { + $slots = (new ReflectionClass(ForbidAdHocDateParsingRule::class))->getConstant($constant); + + self::assertIsArray($slots); + + return $slots; + } + + /** + * @return array{0: list, 1: int} + */ + private static function ruleConstant(string $constant): array + { + $value = (new ReflectionClass(ForbidAdHocDateParsingRule::class))->getConstant($constant); + + self::assertIsArray($value); + + return $value; + } + + /** + * The classification allowlist, read off the rule rather than restated. + * + * @return array + */ + private static function ruleAllowlist(): array + { + $allowed = (new ReflectionClass(ForbidAdHocDateParsingRule::class))->getConstant('NON_DECODING_FACTORIES'); + + self::assertIsArray($allowed); + + return $allowed; + } + + /** + * Every parameter spelling the rule will accept as a decoded slot, unioned + * across all three of its slot maps and read off the rule, so a spelling + * added there is covered here without an edit. + * + * @return list + */ + private static function decodedSlotSpellings(): array + { + $spellings = []; + + foreach ([self::ruleSlots('PARSING_METHODS'), self::ruleSlots('PARSING_FUNCTIONS')] as $map) { + foreach ($map as [$names, $position]) { + foreach ($names as $name) { + $spellings[] = mb_strtolower($name); + } + } + } + + [$names, $position] = self::ruleConstant('CONSTRUCTOR_SLOT'); + + foreach ($names as $name) { + $spellings[] = mb_strtolower($name); + } + + $spellings = array_values(array_unique($spellings)); + sort($spellings); + + return $spellings; + } + + /** + * Whether a static method hands back an instance — the shape that makes it a + * factory this rule has to have an opinion about. The declared return type + * is read first; Carbon leaves two factories untyped, so the docblock + * `@return` is the fallback; and a method with NEITHER counts as a factory, + * because an unreadable shape must be classified rather than assumed inert. + */ + private static function returnsAnInstance(ReflectionMethod $method): bool + { + $declared = $method->getReturnType(); + + if ($declared !== null) { + return self::namesAnInstance(self::typeNames($declared)); + } + + $documented = self::documentedReturn($method); + + return $documented === null || self::namesAnInstance($documented); + } + + /** + * @param list $names + */ + private static function namesAnInstance(array $names): bool + { + foreach ($names as $name) { + $name = mb_ltrim($name, '?\\'); + + if (in_array(mb_strtolower($name), ['static', 'self', '$this'], true)) { + return true; + } + + // Carbon writes its own classes unqualified in docblocks, so the + // relative spelling is resolved against its namespace before the + // subtype question is asked. + foreach ([$name, 'Carbon\\' . $name] as $candidate) { + if ((class_exists($candidate) || interface_exists($candidate)) && is_a($candidate, DateTimeInterface::class, true)) { + return true; + } + } + } + + return false; + } + + /** + * @return list|null the union members of the docblock `@return`, or + * null when the method documents none + */ + private static function documentedReturn(ReflectionMethod $method): ?array + { + $doc = $method->getDocComment(); + + if ($doc === false || preg_match('/@return\s+(\S+)/', $doc, $matches) !== 1) { + return null; + } + + return explode('|', $matches[1]); + } + + /** + * Whether a string fits in this parameter. An untyped parameter and a + * `mixed` one both do — the same direction the rule's own argument gate + * takes, where "not provably non-string" is what fires. + */ + private static function admitsAString(ReflectionParameter $parameter): bool + { + $type = $parameter->getType(); + + if ($type === null) { + return true; + } + + $names = self::typeNames($type); + + if ($names === []) { + return true; + } + + foreach ($names as $name) { + if (in_array(mb_strtolower(mb_ltrim($name, '?\\')), ['string', 'mixed'], true)) { + return true; + } + } + + return false; + } + + /** + * Every named member of a type, flattening unions and intersections so a + * `Closure|CarbonInterface|null` is read for the Carbon in it. + * + * @return list + */ + private static function typeNames(ReflectionType $type): array + { + if ($type instanceof ReflectionNamedType) { + return [$type->getName()]; + } + + if ($type instanceof ReflectionUnionType || $type instanceof ReflectionIntersectionType) { + $names = []; + + foreach ($type->getTypes() as $inner) { + $names = [...$names, ...self::typeNames($inner)]; + } + + return $names; + } + + return []; + } + + private static function expected(string $call, string $allowed = self::DEFAULT_ALLOWED): string + { + return sprintf( + 'Ad-hoc date parsing (%s) outside %s: decode the string once at the boundary and pass the value object (ADR-0020 Amd 1).', + $call, + $allowed, + ); + } +}