diff --git a/CHANGELOG.md b/CHANGELOG.md index 77f15c1..fb34132 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and ### Added +- `ForbidSentinelFallbackOnNarrowingHelperRule` — new rule that flags a **literal sentinel fallback** (`?? ''`, `?? 0`, `?: 'unknown'`, `?? false`, `?? []`, `?? Foo::BAR`) on the result of a **narrowing helper**: a first-party boundary helper that takes `mixed` external data and returns a nullable scalar, where `null` is the helper's way of saying "this input was unreadable". The sentinel converts that failure signal into a plausible-looking value, which then gets persisted, compared, or used to unlock a branch — and the original unreadable input is unrecoverable. **Confirmed damage (tc-api PR #360):** `?? ''` seeded `['']`, a one-element array holding an empty string that reads as non-empty and unlocked a `forceDelete()` wipe; a gender was written to the database as an empty `SET` string. A third bug in the same PR — `Carbon::parse(text($leaf) ?? null)` parsing a missing node as `now()` and writing today's date as a person's date of birth — is the **motivation** for this rule but explicitly **out of its scope**: there is no literal sentinel there (`?? null` is the correct half of the shape), and the damage happens one call later, in a sink that swallows the preserved `null`. Catching it needs sink-aware analysis (which callees are null-swallowing), not this rule's fallback-shape matcher. The remediation is always one of: skip the write, fail closed, or handle `null` explicitly. **Detection is SHAPE-KEYED, never a helper-name list** (a territory's helper inventory changes, the shape does not) — a left-hand `MethodCall` / `NullsafeMethodCall` / `StaticCall` / `FuncCall` (the `?->` arm is fixture-pinned and needs no null-stripping of its own: PHPStan analyses the left operand of `??` / `?:` in isset-ish context, so a nullable receiver — promoted property, parameter, method-call receiver, chained `?->…?->`, array offset — is already narrowed to the bare class by the time the rule resolves the method) fires only when its resolved reflection satisfies ALL of: (1) the declaring class (method/static call) or function FQN (plain function) sits under a configured namespace prefix — the new optional `narrowingHelperNamespacePrefixes` parameter (`listOf(string())`, default `['App']`), matched on a namespace BOUNDARY so `App` and `App\` behave identically and `Application\Foo` is never swept in; (2) at least one **required** parameter resolves to `MixedType` — explicit `mixed` or untyped, both being the same boundary contract from the caller's side, while an OPTIONAL mixed parameter (`ParameterReflection::isOptional()`) is skipped because a lookup helper's `get(string $key, mixed $default = null): ?string` takes the caller's own default there, not unvalidated external data, and its `?? ''` is idiomatic; (3) the return type is a nullable scalar or nullable union of scalars (`?string`, `?int`, `?float`, `?bool`, `string|int|null`), checked via the PHPStan Type API (`TypeCombinator::containsNull()` + a per-member scalar probe on the non-null remainder, with an explicit `NeverType` reject so a `null`-only return cannot match through the bottom type), never by string-comparing type names. The namespace gate is also the vendor gate: `filter_var($x, FILTER_VALIDATE_EMAIL) ?? ''` and every framework call are structurally out of scope, because a vendor helper's null contract is not ours to reason about. **Never flagged:** `?? null` (it PRESERVES the failure signal — the remediation), coalesces on properties / array offsets (`$row['city'] ?? ''` — the idiomatic absent-key default, no helper call), helpers with no `mixed` parameter, non-nullable returns, and nullable non-scalar returns (a null-object fallback is a different, legitimate pattern). **Deliberate misses:** a non-literal fallback (`?? $default`, `?? $this->fetch($leaf)`) — the false-positive-rich half of the shape, where the fallback may itself carry the failure forward; the long ternary (`text($leaf) !== null ? … : ''` — an explicit null test IS the remediation); and a helper result laundered through a variable before the coalesce (provenance is gone at the coalesce; closing it needs data-flow tracking, not a wider matcher). **Known residual false positive:** a first-party helper taking a REQUIRED `mixed` (or untyped) key AND returning a nullable scalar still matches even when it is a lookup rather than a boundary reader — `get(mixed $key, string $default = ''): ?string` is indistinguishable from a narrowing helper by signature alone; the required/optional split removes the common lookup shape, and for the rest the escape hatch is an inline `@phpstan-ignore forbidSentinelFallbackOnNarrowingHelper.sentinelFallback`. ADR-0021 posture — false negatives acceptable, false positives are not. `getNodeType()` is `Expr` because `Coalesce` (a `BinaryOp`) and the short `Ternary` have no common ancestor below it; both are matched by an instanceof guard on the first line and the AST-only sentinel check runs before any reflection, so the per-node cost on non-matching expressions is one instanceof. Identifier: `forbidSentinelFallbackOnNarrowingHelper.sentinelFallback`. Doctrine: war-room §Architectural Principles — Explicit over implicit (a swallowed parse failure is the implicit path); fail-closed data-integrity posture for the boundary/import surface. **Versioning: candidate MAJOR** (surfaces new errors in already-clean consumer code wherever an import/boundary helper is followed by a sentinel). Per the pre-1.0 caret convention the next minor auto-adopts nobody — each consumer remediates and goes green on its own pin bump. **NOT tagged** (release is ally-gated). Seed: tc-api PR #360. - `ForbidRawExceptionMessageInResponseRule` — new rule (war-room enforcement queue #140) that flags a raw `Throwable::getMessage()` — or a `Throwable` expression itself — flowing into a **client-facing response sink**. A raw exception message is internal detail (stack-trace fragments, SQL, file paths, driver errors); when it reaches an API response it is an information-disclosure leak (ISO 27001 A.5.33 / general defence-in-depth for the ISO 27001 / AVG / NEN 7510 consumer territories). The remediation is always the same: **log** the raw message server-side (`Log::`, `report()`) and hand the client a stable, app-authored message. The rule is the durable Level-2 backstop for the raw-exception-message leak family (the confirmed point-fix sites — ublgenie's 8 MCP tools + codebook `DeleteChapterTool`, each returning `Response::error('...' . $e->getMessage())` — and each consumer's release-pin adoption are separate, separately-tracked steps). **Sink model:** a sink is a `FQCN::method` signature, matched in BOTH call forms — a `StaticCall` whose resolved class equals the FQCN (`Response::error(...)`) and a `MethodCall` whose receiver type is a subtype of the FQCN (an injected persist-sink service). The built-in default sink is `Laravel\Mcp\Response::error` (the confirmed dominant shape, always armed); a consumer adds its own PERSIST sinks (an invoice-log setter, a `MarkInvoiceFailed` Action) via the new optional `rawExceptionMessageSinks` PHPStan parameter (`listOf(string())`, default `[]` — so the rule is safe to adopt with only the MCP shape armed). **Argument detection:** a matched sink call is flagged when any argument is, directly OR via string concatenation (`'context: ' . $e->getMessage()`), a `->getMessage()` call on an expression whose type is a subtype of `\Throwable`, or a `\Throwable` expression passed directly. **Type-aware discrimination is load-bearing:** `$validator->getMessage()` on a non-`Throwable` receiver does NOT fire — only a message pulled off an actual exception is a leak. **Mandatory false-positive exclusions (the remediation pattern, never the violation):** `Log::` / `logger()->` / PSR `LoggerInterface` log-level calls (`info` / `warning` / `error` / `critical` / `debug` / `log` / `notice` / `alert` / `emergency`) and `report()` — server-side logging of the raw message is exactly where it is *supposed* to go. Because a sink is keyed on `FQCN::method` a logger is never a sink under the default config; the exclusion additionally short-circuits BEFORE sink matching, so a consumer that adds a broad sink can never turn a logger into a false positive (pinned by tests that configure a logger method AS a sink and assert it still stays silent). **Exemptions (narrowest first):** the `safeMessageExceptionClasses` parameter (`listOf(string())`, default `[]`) lists exception FQCNs whose message discipline is proven app-authored — arch-test-pinned in the consuming territory, the codebook `DependentModelRelationException` shape — so a prove-safe class costs ONE config line, not a per-call-site annotation (type-aware, subtypes inherit; covers the **message only** — the Throwable itself still fires, `__toString` carries class/file/trace regardless of message discipline); a `// @leak-safe: ` comment on the sink call line (or in the contiguous comment block directly above it) suppresses a proven-safe call site the class list cannot express (the codebook `SendCodyReportAction` shape); the standard PHPStan inline-ignore mechanism on the identifier is the alternative. `$e?->getMessage()` (a `NullsafeMethodCall` — a distinct AST node) is matched like its unconditional sibling. Identifier: `forbidRawExceptionMessageInResponse.rawMessageInResponse`. **Deliberate misses (v1 scope):** `getTraceAsString()` / `__toString()` and other Throwable accessors (a future minor can widen the accessor set), a Throwable laundered through a helper/formatter call whose return type is no longer `Throwable`, and plain local-variable extraction (`$msg = $e->getMessage(); Response::error($msg);` — the type at the sink is `string`, provenance gone; closing it needs data-flow tracking) (ADR-0021 posture — false negatives acceptable, false positives are not). Doctrine: war-room §Architectural Principles — Explicit over implicit (#1); information-disclosure hardening. **Versioning: candidate MAJOR** (surfaces new errors in already-clean consumer code wherever a raw exception message reaches a response sink — the confirmed ublgenie/codebook MCP-tool sites). Per the pre-1.0 caret convention `^0.7` excludes the next minor, so tagging auto-adopts nobody — each consumer remediates and goes green on its own bump PR (a suppress-only / baseline-absorb posture). **NOT tagged** (release is ally-gated). Seed: war-room enforcement queue #140. ### Changed diff --git a/CLAUDE.md b/CLAUDE.md index d06919e..fba38bc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,6 +27,7 @@ Composer package distributing war-room-doctrine PHPStan rules across `script-dev | `ForbidResourceWrappedInJsonResponseRule` | War-room §Explicit over implicit + ADR-0009 | `forbidResourceWrappedInJsonResponse.resourceWrapped` (type-aware; bans wrapping a `JsonResource` in `response()->json()` / `new JsonResponse()` in `App\Http\Controllers\*`. Named-envelope nesting excluded. shipped v0.5.0) | | `ForbidInlineArrayJsonResponseInControllersRule` | ADR-0009 | `forbidInlineArrayJsonResponseInControllers.arrayPayload` (type-aware; bans constructing the base `JsonResponse` (exact-FQCN, NOT subclasses) / `response()->json()` with an ARRAY payload in `App\Http\Controllers\*`. Inverse of `ForbidResourceWrappedInJsonResponseRule`. `fromJsonString` a deliberate miss. Seed kendo PR #1653. on `main`, `[Unreleased]` — pending v0.8.0 tag (release PR #53)) | | `ForbidRawExceptionMessageInResponseRule` | War-room §Explicit over implicit + info-disclosure hardening | `forbidRawExceptionMessageInResponse.rawMessageInResponse` (flags a raw `Throwable::getMessage()` — directly or via string concat — or the `Throwable` itself flowing into a client-facing response sink. Default sink `Laravel\Mcp\Response::error`; additional `FQCN::method` sinks via the `rawExceptionMessageSinks` param, default `[]`. Type-aware — only a `getMessage()` on a `\Throwable` receiver fires. Server-side logging (`Log::`/`logger()`/PSR `LoggerInterface`/`report()`) never flags. `// @leak-safe:` comment exemption. on `main`, `[Unreleased]`) | +| `ForbidSentinelFallbackOnNarrowingHelperRule` | War-room §Explicit over implicit + fail-closed data integrity | `forbidSentinelFallbackOnNarrowingHelper.sentinelFallback` (shape-keyed; flags a literal sentinel fallback — `?? ''` / `?? 0` / `?: 'unknown'` / `?? false` / `?? []` / `?? Foo::BAR` — on a first-party helper that takes a `mixed` param and returns a nullable scalar. Namespace prefixes via `narrowingHelperNamespacePrefixes`, default `['App']`. `?? null` never fires; non-literal fallbacks and the long ternary are deliberate misses. Seed tc-api PR #360. on `main`, `[Unreleased]`) | | `LogRule` | ADR-0001 §Append-only | `logRule.logModification` (covers instance `update`/`delete`/`forceDelete`/`forceDeleteQuietly`; static `Model::destroy()` / `Model::forceDestroy()` shipped in v0.3.0) | | `LogBuilderTruncateRule` | ADR-0001 §Append-only | `logRule.logModification` (shared with `LogRule`; covers `Builder->truncate()` on Log-named tables — shipped in v0.3.0) | | `EnforceAuditSnapshotOnRetryRule` | ADR-0001 §Snapshot-on-Retry Safety | `enforceAuditSnapshotOnRetry.firstStatementMustResetState` | diff --git a/README.md b/README.md index ea09b80..4615d49 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,7 @@ includes: | `ForbidEloquentMutationInControllersRule` | `forbidEloquentMutationInControllers.eloquentMutationInController` | `App\Http\Controllers\*` (including sub-namespaces; configurable via `controllerNamespacePrefixes`) | Calling Eloquent persistence APIs (`save`, `update`, `delete`, `create`, `destroy`, `forceDelete`, `forceFill`, `push`, `restore`, `touch`, and their `*OrFail` / `*Quietly` / `*OrCreate` variants — 24-method blocklist) on `Illuminate\Database\Eloquent\Model` subclasses or `Illuminate\Database\Eloquent\Builder` chains is an error. Reads (`find`, `where`, `get`, `first`, `paginate`, `pluck`, `count`, `exists`, `query`) are permitted. Delegate mutations to an Action. Doctrine: ADR-0011 (Action Class Architecture) + ADR-0019 (Explicit Model Hydration). | | `ForbidInlineArrayJsonResponseInControllersRule` | `forbidInlineArrayJsonResponseInControllers.arrayPayload` | `App\Http\Controllers\*` (including sub-namespaces; configurable via `controllerNamespacePrefixes`) | Constructing the base `Illuminate\Http\JsonResponse` — exact-FQCN, **NOT subclasses** — or its `response()->json(...)` factory twin with an **array** payload is an error. Type-aware: fires when the first argument's resolved type `isArray()->yes()`, catching both inline literals (`new JsonResponse(['enabled' => …])`) and array-typed variables (`new JsonResponse($result)` — the same violation laundered through a variable). Passes on Resource / DTO / `JsonSerializable` / mixed / unknown payloads, `null` (`new JsonResponse(null, 204)`), no-args, and any JsonResponse **subclass** (`NoContentResponse`, … — the compliant fix; matching by supertype would criminalize it). Response shapes belong to a Resource/ResourceData or a dedicated JsonResponse subclass. Deliberate miss: `JsonResponse::fromJsonString(...)`. Sibling/inverse of `ForbidResourceWrappedInJsonResponseRule` (same JsonResponse × payload boundary, opposite direction — that rule fires on a Resource payload, this one on an array). Doctrine: ADR-0009 (Unified ResourceData Pattern). Seed: kendo PR #1653. | | `ForbidRawExceptionMessageInResponseRule` | `forbidRawExceptionMessageInResponse.rawMessageInResponse` | Calls to a configured client-facing response sink (default `Laravel\Mcp\Response::error`; add more via `rawExceptionMessageSinks`) | Passing a raw `Throwable::getMessage()` — directly or via string concat (`'x: ' . $e->getMessage()`) — or the `Throwable` itself into a response sink is an error: it leaks internal detail (stack traces, SQL, file paths) to the API client. Log the raw message server-side (`Log::` / `report()`) and return a stable, app-authored message. **Type-aware:** only a `getMessage()` on an actual `\Throwable` receiver fires (`$validator->getMessage()` is silent). **Never flags** server-side logging — `Log::` / `logger()->` / PSR `LoggerInterface` log-level calls and `report()` are the remediation, not the leak. Exempt a proven-safe app-authored message per exception CLASS via `safeMessageExceptionClasses` (arch-test-pinned; message only — the Throwable itself still fires) or per call site with a `// @leak-safe: ` comment on/above the sink line. Doctrine: war-room §Explicit over implicit (#1); information-disclosure hardening. | +| `ForbidSentinelFallbackOnNarrowingHelperRule` | `forbidSentinelFallbackOnNarrowingHelper.sentinelFallback` | `??` / short-`?:` fallbacks whose left side is a **narrowing helper** call — a first-party method / static method / function (namespace prefix, default `App\`, configurable via `narrowingHelperNamespacePrefixes`) that takes at least one **required** `mixed` (or untyped) parameter and returns a nullable scalar (`?string`, `?int`, `?float`, `?bool`, `string\|int\|null`) | Following such a call with a **literal sentinel** (`?? ''`, `?? 0`, `?: 'unknown'`, `?? false`, `?? []`, `?? Foo::BAR`) is an error: `null` means "this input was unreadable", and the sentinel converts that failure into a plausible value that then gets persisted or unlocks a branch. Skip the write, fail closed, or handle `null` explicitly. **`?? null` is never flagged** — it preserves the failure signal. Shape-keyed, not a helper-name list; the return-type check runs on the PHPStan Type API (`TypeCombinator::containsNull` + a per-member scalar probe), never on type-name strings. Vendor/builtin callees (`filter_var(...) ?? ''`), coalesces on properties/array offsets, helpers with no required `mixed` parameter (a lookup's optional `mixed $default = null` is the caller's own default, not external data), and non-nullable or non-scalar returns are all silent. A **non-literal** fallback (variable, call) is a deliberate miss (the false-positive-rich half of the shape), as is the long ternary (an explicit null test IS the remediation). Residual FP: a helper with a required `mixed` key plus a default parameter still fires — inline-ignore the identifier. Doctrine: war-room §Explicit over implicit; fail-closed data integrity. Seed: tc-api PR #360 (`?? ''` seeding `['']` that unlocked a `forceDelete()` wipe, empty-string gender; the `now()`-as-date-of-birth bug in the same PR motivated the rule but is out of scope — no literal sentinel, the `null` is swallowed by a downstream sink). | | `EnforceResourceDataValidatorOptInRule` | `enforceResourceDataValidatorOptIn.missingValidatorCall` | Classes extending `App\Http\Resources\ResourceData` | If the class declares a non-empty `EAGER_LOAD_COUNT` / `EAGER_LOAD_SUM` constant but never calls `validateRelationsLoaded()` in any method, error. | | `EnforceFormRequestToDtoRule` | `enforceFormRequestToDto.missingToDtoMethod` | Concrete classes extending `Illuminate\Foundation\Http\FormRequest` | If the class neither declares nor inherits a `toDto()` method, error. Abstract intermediates (`BaseFormRequest`) are exempt. Hand Actions a typed DTO, not `$request->validated()` arrays. Doctrine: ADR-0012 (FormRequest → DTO Flow). | | `EnforceCurrentUserAttributeRule` | `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser` | `Request::user()` / `Auth::user()` / `auth()->user()` calls inside `App\Http\Controllers\*` classes (namespace prefix, incl. sub-namespaces; configurable via `controllerNamespacePrefixes`) | Use `#[\Illuminate\Container\Attributes\CurrentUser] User $user` on the method parameter. Scope is decided by namespace, not class ancestry — a base-less `final` controller in `App\Http\Controllers` fires; FormRequests (`App\Http\Requests`), middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs, and console commands are silent because their namespaces do not start with the controller prefix (container-attribute injection does not apply to FormRequest methods regardless). | @@ -271,6 +272,46 @@ 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. +### `ForbidSentinelFallbackOnNarrowingHelperRule` — configurable first-party namespaces + +A **narrowing helper** is a boundary helper: `mixed` external data in (an XML leaf, a CSV cell, a decoded JSON node), a nullable scalar out, where `null` means "this input was unreadable". A sentinel fallback throws that signal away: + +```php +$gender = $this->text($leaf) ?? ''; // ERROR — empty SET value gets persisted +$names = [$this->text($leaf) ?? '']; // ERROR — [''] reads as non-empty + +$text = $this->text($leaf); + +if ($text === null) { + return; // fine — skip the write +} + +// never flagged: +$dob = Carbon::parse($this->text($leaf) ?? null); // `?? null` KEEPS the failure signal — the + // now()-as-date-of-birth damage happens in the + // sink that swallows the null, one call later +$city = $this->lookup->get('city') ?? ''; // a lookup helper: its only `mixed` parameter + // is the OPTIONAL $default, not external data +``` + +The `Carbon::parse(… ?? null)` case is the bug that motivated this rule, and it is deliberately out of its scope: there is no literal sentinel to match, so catching it needs sink-aware analysis (which callees swallow `null`) rather than a wider fallback matcher. + +Detection is keyed on the SHAPE, never on a helper-name list: the callee must be declared under a configured first-party namespace, take at least one **required** `mixed` (or untyped) parameter, and return a nullable scalar. The namespace prefixes are configurable, default `App`: + +```neon +parameters: + narrowingHelperNamespacePrefixes: + # single backslashes — NEON keeps them literal outside double quotes + - 'App' + - 'Domain\Import' +``` + +Matching is namespace-BOUNDARY aware — `App` and `App\` behave identically, and `Application\Foo` is never swept in by the `App` default. The prefix gate is also what keeps builtins and vendor code out of scope: `filter_var($x, FILTER_VALIDATE_EMAIL) ?? ''` never fires, because a vendor helper's null contract is not ours to reason about. + +`?? null` is never flagged — it hands the failure signal to the caller, which is the point. A non-literal fallback (`?? $default`, `?? $this->fetch($leaf)`) is a deliberate miss, as is a helper result laundered through a variable before the coalesce. + +The boundary tell is a **required** `mixed` parameter. An optional one is the caller's own default flowing in, so `get(string $key, mixed $default = null): ?string` is a lookup helper and its `?? ''` stays silent. Known residual false positive: a helper with a **required** `mixed` key plus a default parameter (`get(mixed $key, string $default = ''): ?string`) is indistinguishable from a narrowing helper by signature alone and still fires — the escape hatch is an inline `@phpstan-ignore forbidSentinelFallbackOnNarrowingHelper.sentinelFallback`. + ### 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. diff --git a/extension.neon b/extension.neon index a9137f1..6a2cf2d 100644 --- a/extension.neon +++ b/extension.neon @@ -67,6 +67,18 @@ parameters: rawExceptionMessageSinks: [] safeMessageExceptionClasses: [] + # `ForbidSentinelFallbackOnNarrowingHelperRule`: namespace prefixes whose + # classes / functions count as FIRST-PARTY, i.e. helpers whose nullable + # return contract we own and can reason about. A left-hand call is only + # considered a narrowing helper when its declaring class (method / static + # call) or its function FQN (plain function) sits under one of these — which + # is what keeps builtins and vendor code (`filter_var(...) ?? ''`) out of + # scope. Matching is namespace-BOUNDARY aware, so `App` and `App\` behave + # identically and `Application\Foo` is never swept in by the `App` default. + # Each prefix uses single backslashes — see the NEON-quoting note above. + narrowingHelperNamespacePrefixes: + - 'App' + parametersSchema: resourceDataBaseClass: string() formRequestBaseClass: string() @@ -76,6 +88,7 @@ parametersSchema: auditModelNameSuffixes: listOf(string()) rawExceptionMessageSinks: listOf(string()) safeMessageExceptionClasses: listOf(string()) + narrowingHelperNamespacePrefixes: listOf(string()) services: - @@ -148,6 +161,11 @@ services: rawExceptionMessageSinks: %rawExceptionMessageSinks% safeMessageExceptionClasses: %safeMessageExceptionClasses% tags: [phpstan.rules.rule] + - + class: ScriptDevelopment\PhpstanWarroomRules\Rules\ForbidSentinelFallbackOnNarrowingHelperRule + arguments: + narrowingHelperNamespacePrefixes: %narrowingHelperNamespacePrefixes% + tags: [phpstan.rules.rule] - class: ScriptDevelopment\PhpstanWarroomRules\Type\ConnectionTransactionReturnTypeExtension tags: [phpstan.broker.dynamicMethodReturnTypeExtension] diff --git a/src/Rules/ForbidSentinelFallbackOnNarrowingHelperRule.php b/src/Rules/ForbidSentinelFallbackOnNarrowingHelperRule.php new file mode 100644 index 0000000..3791552 --- /dev/null +++ b/src/Rules/ForbidSentinelFallbackOnNarrowingHelperRule.php @@ -0,0 +1,395 @@ + + */ +final class ForbidSentinelFallbackOnNarrowingHelperRule implements Rule +{ + /** + * @param list $narrowingHelperNamespacePrefixes namespace prefixes + * whose classes / + * functions are + * first-party enough to + * reason about (default + * `App\`). A trailing + * namespace separator is + * optional — `App` and + * `App\` behave + * identically, and both + * match on a namespace + * boundary so + * `Application\Foo` is + * never swept in. + */ + public function __construct( + private ReflectionProvider $reflectionProvider, + private array $narrowingHelperNamespacePrefixes = ['App\\'], + ) {} + + public function getNodeType(): string + { + return Expr::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if ($node instanceof Coalesce) { + $call = $node->left; + $fallback = $node->right; + $operator = '??'; + } elseif ($node instanceof Ternary && $node->if === null) { + $call = $node->cond; + $fallback = $node->else; + $operator = '?:'; + } else { + return []; + } + + // AST-only, and cheapest of the two gates — run it before any reflection. + $sentinel = $this->describeSentinel($fallback); + + if ($sentinel === null) { + return []; + } + + $helper = $this->resolveNarrowingHelper($call, $scope); + + if ($helper === null) { + return []; + } + + return [ + RuleErrorBuilder::message(sprintf( + 'Narrowing helper %s() returns null for unreadable input; the `%s %s` fallback hides that failure ' + . 'by turning it into a plausible value that gets persisted or unlocks a branch. ' + . 'Skip the write, fail closed, or handle null explicitly (`?? null` preserves the failure signal).', + $helper, + $operator, + $sentinel, + )) + ->identifier('forbidSentinelFallbackOnNarrowingHelper.sentinelFallback') + ->build(), + ]; + } + + /** + * Render the fallback expression when it is a literal sentinel, else null. + * `null` itself is NOT a sentinel — it preserves the failure signal — and a + * non-empty array literal is out of scope (its emptiness, not its content, + * is what makes `[]` a plausible-looking value). + */ + private function describeSentinel(Expr $expr): ?string + { + if ($expr instanceof String_ || $expr instanceof Int_ || $expr instanceof Float_) { + return var_export($expr->value, true); + } + + if ($expr instanceof ConstFetch) { + return $expr->name->toLowerString() === 'null' ? null : $expr->name->toString(); + } + + if ($expr instanceof ClassConstFetch && $expr->class instanceof Name && $expr->name instanceof Identifier) { + return $expr->class->toString() . '::' . $expr->name->toString(); + } + + if ($expr instanceof Array_) { + return $expr->items === [] ? '[]' : null; + } + + return null; + } + + /** + * Resolve the left-hand call to its reflection and return a display name + * (`App\Support\LeafReader::text`, `App\Support\text`) when it is a + * narrowing helper; null otherwise. + */ + private function resolveNarrowingHelper(Expr $expr, Scope $scope): ?string + { + if ($expr instanceof MethodCall || $expr instanceof NullsafeMethodCall) { + if (!$expr->name instanceof Identifier) { + return null; + } + + $methodName = $expr->name->toString(); + + // No `TypeCombinator::removeNull()` on the receiver, deliberately: + // this rule only ever runs on the left operand of `??` / `?:`, and + // PHPStan analyses that operand in isset-ish / truthy context, so a + // nullable receiver is ALREADY narrowed to non-null in this scope + // (verified on phpstan 2.2.7 for a nullable promoted property, a + // nullable parameter, a nullable method-call receiver, a chained + // `?->…?->`, and a nullable array offset — every one resolves to the + // bare class). Stripping null again would be an unreachable branch. + $calledOnType = $scope->getType($expr->var); + + if (!$calledOnType->hasMethod($methodName)->yes()) { + return null; + } + + $method = $calledOnType->getMethod($methodName, $scope); + $owner = $method->getDeclaringClass()->getName(); + + return $this->isNarrowingHelper($owner, $method->getVariants()) + ? $owner . '::' . $methodName + : null; + } + + if ($expr instanceof StaticCall) { + if (!$expr->name instanceof Identifier || !$expr->class instanceof Name) { + return null; + } + + $className = $scope->resolveName($expr->class); + + if (!$this->reflectionProvider->hasClass($className)) { + return null; + } + + $classReflection = $this->reflectionProvider->getClass($className); + $methodName = $expr->name->toString(); + + if (!$classReflection->hasMethod($methodName)) { + return null; + } + + $method = $classReflection->getMethod($methodName, $scope); + $owner = $method->getDeclaringClass()->getName(); + + return $this->isNarrowingHelper($owner, $method->getVariants()) + ? $owner . '::' . $methodName + : null; + } + + if ($expr instanceof FuncCall) { + if (!$expr->name instanceof Name || !$this->reflectionProvider->hasFunction($expr->name, $scope)) { + return null; + } + + $function = $this->reflectionProvider->getFunction($expr->name, $scope); + $owner = $function->getName(); + + return $this->isNarrowingHelper($owner, $function->getVariants()) ? $owner : null; + } + + return null; + } + + /** + * The three-part shape gate: first-party namespace, a `mixed` (or untyped) + * parameter, and a nullable-scalar return. + * + * @param array $variants + */ + private function isNarrowingHelper(string $owner, array $variants): bool + { + if (!$this->isFirstParty($owner)) { + return false; + } + + $variant = $variants[0] ?? null; + + if ($variant === null) { + return false; + } + + return $this->hasMixedParameter($variant) && $this->returnsNullableScalar($variant->getReturnType()); + } + + /** + * Namespace-boundary match — `App` and `App\` both accept `App\Support\Foo` + * and both reject `Application\Foo`. + */ + private function isFirstParty(string $owner): bool + { + foreach ($this->narrowingHelperNamespacePrefixes as $prefix) { + if (str_starts_with($owner, mb_rtrim($prefix, '\\') . '\\')) { + return true; + } + } + + return false; + } + + /** + * A REQUIRED `mixed` parameter is the boundary tell. An UNTYPED parameter + * also resolves to `MixedType` (implicit mixed), which is the same contract + * from the caller's side, so both count. + * + * An OPTIONAL mixed parameter does not: `get(string $key, mixed $default = + * null): ?string` is a lookup helper, and its `mixed` is the CALLER's own + * default value flowing in, not unvalidated external data. Counting it made + * every such lookup a narrowing helper and its idiomatic `?? ''` an error. + */ + private function hasMixedParameter(ParametersAcceptor $variant): bool + { + foreach ($variant->getParameters() as $parameter) { + if ($parameter->isOptional()) { + continue; + } + + if ($parameter->getType() instanceof MixedType) { + return true; + } + } + + return false; + } + + /** + * True for `?string` / `?int` / `?float` / `?bool` and nullable unions of + * those. Type-API only — never a string comparison on a type name. + */ + private function returnsNullableScalar(Type $returnType): bool + { + if (!TypeCombinator::containsNull($returnType)) { + return false; + } + + $nonNull = TypeCombinator::removeNull($returnType); + + // A `null`-only return leaves NeverType behind. NeverType is the bottom + // type, so every `is*()` probe answers yes — it must be rejected BEFORE + // the scalar loop or a `: ?null` helper would match everything. + if ($nonNull instanceof NeverType) { + return false; + } + + $members = $nonNull instanceof UnionType ? $nonNull->getTypes() : [$nonNull]; + + foreach ($members as $member) { + if (!$this->isScalar($member)) { + return false; + } + } + + return true; + } + + private function isScalar(Type $type): bool + { + return $type->isString()->yes() + || $type->isInteger()->yes() + || $type->isFloat()->yes() + || $type->isBoolean()->yes(); + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/ForeignNamespaceHelper.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/ForeignNamespaceHelper.php new file mode 100644 index 0000000..1d2d83d --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/ForeignNamespaceHelper.php @@ -0,0 +1,21 @@ +reader->text($leaf) ?? ''; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/MethodCallCoalesceEmptyString.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/MethodCallCoalesceEmptyString.php new file mode 100644 index 0000000..498a782 --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/MethodCallCoalesceEmptyString.php @@ -0,0 +1,21 @@ +reader->text($leaf) ?? ''; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/MethodCallCoalesceZero.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/MethodCallCoalesceZero.php new file mode 100644 index 0000000..2cb1abf --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/MethodCallCoalesceZero.php @@ -0,0 +1,20 @@ +reader->number($leaf) ?? 0; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NarrowedParameterHelper.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NarrowedParameterHelper.php new file mode 100644 index 0000000..975e66b --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NarrowedParameterHelper.php @@ -0,0 +1,21 @@ +reader->label($leaf) ?? ''; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NonLiteralFallback.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NonLiteralFallback.php new file mode 100644 index 0000000..e8d4037 --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NonLiteralFallback.php @@ -0,0 +1,27 @@ +reader->text($leaf) ?? $fallback; + } + + public function fromCall(mixed $leaf): string + { + // Same for a call: the second read is a decision, not a literal sentinel. + return $this->reader->text($leaf) ?? $this->reader->required($leaf); + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NonNullableReturnHelper.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NonNullableReturnHelper.php new file mode 100644 index 0000000..84eab6f --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NonNullableReturnHelper.php @@ -0,0 +1,21 @@ +reader->required($leaf) ?? ''; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NonScalarReturnHelper.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NonScalarReturnHelper.php new file mode 100644 index 0000000..d623a39 --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NonScalarReturnHelper.php @@ -0,0 +1,21 @@ +reader->reader($leaf) ?? ''; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NullFallback.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NullFallback.php new file mode 100644 index 0000000..10991f9 --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NullFallback.php @@ -0,0 +1,20 @@ +reader->text($leaf) ?? null; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NullsafeCallCoalesce.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NullsafeCallCoalesce.php new file mode 100644 index 0000000..eb5bfbb --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/NullsafeCallCoalesce.php @@ -0,0 +1,23 @@ +` on a NULLABLE receiver. Pins that the NullsafeMethodCall arm is + // live: PHPStan analyses the left operand of `??` in isset-ish context, + // so `$this->reader` is already narrowed to `LeafReader` here and the + // method resolves without the rule stripping null itself. Fires. + return $this->reader?->text($leaf) ?? ''; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/OptionalMixedDefaultHelper.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/OptionalMixedDefaultHelper.php new file mode 100644 index 0000000..4b41274 --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/OptionalMixedDefaultHelper.php @@ -0,0 +1,22 @@ +lookup->get($key) ?? ''; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/PlainAccessFallback.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/PlainAccessFallback.php new file mode 100644 index 0000000..7e6ec68 --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/PlainAccessFallback.php @@ -0,0 +1,28 @@ + + */ + private array $config = []; + + private ?string $label = null; + + /** + * @param array $row + */ + public function values(array $row): string + { + // Coalesce on an array offset / a property is the idiomatic + // absent-key default — no helper call, nothing to hide. + $city = $row['city'] ?? ''; + $label = $this->label ?? ''; + + return $this->config['name'] ?? $city . $label; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/PlainFunctionCoalesce.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/PlainFunctionCoalesce.php new file mode 100644 index 0000000..34c9307 --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/PlainFunctionCoalesce.php @@ -0,0 +1,23 @@ +reader->text($leaf) ?? LeafReader::UNKNOWN; + } + + /** + * @return list + */ + public function fromEmptyArray(mixed $leaf): array + { + // `[]` reads as "nothing found" rather than "input unreadable". Fires. + return $this->reader->scalar($leaf) ?? []; + } + + public function fromFalse(mixed $leaf): bool + { + // `false` is a decided answer; null was an undecided one. Fires. + return $this->reader->flag($leaf) ?? false; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/ShortTernaryUnknown.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/ShortTernaryUnknown.php new file mode 100644 index 0000000..3cc6967 --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/ShortTernaryUnknown.php @@ -0,0 +1,20 @@ +reader->text($leaf) ?: 'unknown'; + } +} diff --git a/tests/Fixtures/SentinelFallbackOnNarrowingHelper/StaticCallCoalesce.php b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/StaticCallCoalesce.php new file mode 100644 index 0000000..a894f5e --- /dev/null +++ b/tests/Fixtures/SentinelFallbackOnNarrowingHelper/StaticCallCoalesce.php @@ -0,0 +1,16 @@ + + */ +final class ForbidSentinelFallbackOnNarrowingHelperRuleTest extends RuleTestCase +{ + private const string MESSAGE = 'Narrowing helper %s() returns null for unreadable input; the `%s %s` fallback hides that failure ' + . 'by turning it into a plausible value that gets persisted or unlocks a branch. ' + . 'Skip the write, fail closed, or handle null explicitly (`?? null` preserves the failure signal).'; + + private const string STUBS = __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/_stubs.php'; + + private const string READER_TEXT = 'App\Support\LeafReader::text'; + + /** + * Override hook: when set, `getRule()` returns this instance instead of the + * default. Lets a single test reconfigure `narrowingHelperNamespacePrefixes` + * or pull the rule out of the NEON-configured container. + */ + private ?Rule $ruleOverride = null; + + public function testFlagsEmptyStringFallbackOnMethodCall(): void + { + // The tc-api #360 shape — `?? ''` on a nullable-scalar boundary helper. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/MethodCallCoalesceEmptyString.php'], + [[sprintf(self::MESSAGE, self::READER_TEXT, '??', "''"), 19]], + ); + } + + public function testFlagsZeroFallbackOnMethodCall(): void + { + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/MethodCallCoalesceZero.php'], + [[sprintf(self::MESSAGE, 'App\Support\LeafReader::number', '??', '0'), 18]], + ); + } + + public function testFlagsShortTernaryFallback(): void + { + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/ShortTernaryUnknown.php'], + [[sprintf(self::MESSAGE, self::READER_TEXT, '?:', "'unknown'"), 18]], + ); + } + + public function testFlagsStaticCallFallback(): void + { + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/StaticCallCoalesce.php'], + [[sprintf(self::MESSAGE, 'App\Support\LeafReader::staticText', '??', "''"), 14]], + ); + } + + public function testFlagsNullsafeCallOnNullableReceiver(): void + { + // `$this->reader?->text($leaf) ?? ''` — pins the NullsafeMethodCall arm + // as live. PHPStan analyses the left operand of `??` in isset-ish + // context, so the nullable receiver is already narrowed to the bare + // class and the method resolves. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/NullsafeCallCoalesce.php'], + [[sprintf(self::MESSAGE, self::READER_TEXT, '??', "''"), 21]], + ); + } + + public function testFlagsPlainFirstPartyFunctionFallback(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/PlainFunctionCoalesce.php'], + [[sprintf(self::MESSAGE, 'App\Support\leafText', '??', "''"), 21]], + ); + } + + public function testFlagsClassConstantEmptyArrayAndBooleanSentinels(): void + { + // The non-string sentinel shapes: a class constant, `[]`, and `false`. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/SentinelShapes.php'], + [ + // PHPStan resolves the `Name` node, so the constant renders + // fully qualified even though the source writes `LeafReader::`. + [sprintf(self::MESSAGE, self::READER_TEXT, '??', 'App\Support\LeafReader::UNKNOWN'), 18], + [sprintf(self::MESSAGE, 'App\Support\LeafReader::scalar', '??', '[]'), 27], + [sprintf(self::MESSAGE, 'App\Support\LeafReader::flag', '??', 'false'), 33], + ], + ); + } + + public function testIgnoresNullFallback(): void + { + // `?? null` preserves the failure signal — the remediation, not the violation. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/NullFallback.php'], + [], + ); + } + + public function testIgnoresHelperWithoutMixedParameter(): void + { + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/NarrowedParameterHelper.php'], + [], + ); + } + + public function testIgnoresHelperWhoseOnlyMixedParameterIsOptional(): void + { + // `get(string $key, mixed $default = null): ?string` — a lookup helper. + // Its `mixed` is the CALLER's default flowing in, not unvalidated + // external data, so `?? ''` on it is idiomatic and must stay silent. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/OptionalMixedDefaultHelper.php'], + [], + ); + } + + public function testIgnoresHelperWithNonNullableReturn(): void + { + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/NonNullableReturnHelper.php'], + [], + ); + } + + public function testIgnoresHelperWithNonScalarNullableReturn(): void + { + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/NonScalarReturnHelper.php'], + [], + ); + } + + public function testIgnoresVendorFunction(): void + { + // `filter_var(...) ?? ''` — a builtin is outside the first-party namespaces. + $this->analyse( + [__DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/VendorFunctionFallback.php'], + [], + ); + } + + public function testIgnoresPlainPropertyAndArrayAccessFallback(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/PlainAccessFallback.php'], + [], + ); + } + + public function testIgnoresNonLiteralFallback(): void + { + // A variable / call fallback is deliberately left alone (FP-prone half). + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/NonLiteralFallback.php'], + [], + ); + } + + public function testIgnoresHelperOutsideConfiguredNamespace(): void + { + // `Application\Support` must not match the `App\` default — the prefix + // comparison is namespace-boundary aware, not a bare string prefix. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/ForeignNamespaceHelper.php'], + [], + ); + } + + public function testFlagsHelperInAdditionallyConfiguredNamespace(): void + { + // Same fixture, prefixes reconfigured — pins that the namespace gate is + // the only thing keeping it silent above. + $this->ruleOverride = new ForbidSentinelFallbackOnNarrowingHelperRule( + $this->createReflectionProvider(), + ['Application\Support'], + ); + + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/ForeignNamespaceHelper.php'], + [[sprintf(self::MESSAGE, 'Application\Support\ImposterReader::text', '??', "''"), 19]], + ); + } + + public function testRuleResolvesFromExtensionNeonAndFires(): void + { + // Container-resolved: exercises the SHIPPED + // `narrowingHelperNamespacePrefixes` default and its `%param%` wiring, + // not the PHP constructor default. A NEON quoting regression silently + // no-ops the rule while every direct-instantiation test stays green; + // this is the only gate that catches it. + $this->ruleOverride = self::getContainer()->getByType(ForbidSentinelFallbackOnNarrowingHelperRule::class); + + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/SentinelFallbackOnNarrowingHelper/MethodCallCoalesceEmptyString.php'], + [[sprintf(self::MESSAGE, self::READER_TEXT, '??', "''"), 19]], + ); + } + + /** + * Load the shipped extension.neon so testRuleResolvesFromExtensionNeonAndFires + * can pull the rule out of the container with its NEON-configured + * `narrowingHelperNamespacePrefixes` parameter applied. + * + * @return array + */ + public static function getAdditionalConfigFiles(): array + { + return [ + __DIR__ . '/../../extension.neon', + ]; + } + + protected function getRule(): Rule + { + return $this->ruleOverride ?? new ForbidSentinelFallbackOnNarrowingHelperRule($this->createReflectionProvider()); + } +}