From a725928baf11e69a873e177535c47c6ad71545f8 Mon Sep 17 00:00:00 2001 From: Gerard Oosterhof Date: Wed, 22 Jul 2026 12:58:09 +0200 Subject: [PATCH 1/2] feat(rules): add ForbidRawExceptionMessageInResponseRule (queue #140) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Level-2 durable backstop for the raw-exception-message info-disclosure family. 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 new `rawExceptionMessageSinks` param (default `[]`, safe to adopt). Type-aware: only a getMessage() on a `\Throwable` receiver fires. Mandatory false-positive exclusions: `Log::` / `logger()->` / PSR `LoggerInterface` log-level calls and `report()` — server-side logging is the remediation, never the leak (exclusion short-circuits before sink match, pinned by tests that configure a logger method AS a sink and assert silence). `// @leak-safe: ` comment exemption (same-line or block-above) for proven-safe app-authored messages. 16 fixtures + RuleTestCase (green), extension.neon registration + param, plus README / CLAUDE.md / CHANGELOG (candidate MAJOR, [Unreleased] ### Added). All 6 gates green on the tracked lock: 192 tests / 277 assertions, phpstan [OK] 18/18, pint clean, audit clean, coverage 89.83%, mutation:ci MSI ~85.5% (new rule covered-MSI 81%). NOT tagged — release + consumer pins are separate. Seed: war-room enforcement queue #140. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_0197TgxPcfkCqd3yLuaxCgJ3 --- CHANGELOG.md | 4 + CLAUDE.md | 3 +- README.md | 34 ++ extension.neon | 19 + ...orbidRawExceptionMessageInResponseRule.php | 391 ++++++++++++++++++ .../AppAuthoredLiteralMessage.php | 19 + .../ErrorSinkConcatGetMessage.php | 18 + .../ErrorSinkDirectGetMessage.php | 18 + .../LeakSafeExempted.php | 18 + .../LeakSafeSameLineExempted.php | 16 + .../LogFacadeDirectGetMessage.php | 19 + .../LoggerHelperGetMessage.php | 18 + .../LogsGetMessage.php | 21 + .../NonThrowableGetMessage.php | 18 + .../PersistSinkGetMessage.php | 23 ++ .../PsrLoggerGetMessage.php | 24 ++ .../ThrowableItselfIntoSink.php | 18 + .../RawExceptionMessageInResponse/_stubs.php | 107 +++++ ...dRawExceptionMessageInResponseRuleTest.php | 225 ++++++++++ 19 files changed, 1012 insertions(+), 1 deletion(-) create mode 100644 src/Rules/ForbidRawExceptionMessageInResponseRule.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/AppAuthoredLiteralMessage.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/ErrorSinkConcatGetMessage.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/ErrorSinkDirectGetMessage.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/LeakSafeExempted.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/LeakSafeSameLineExempted.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/LogFacadeDirectGetMessage.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/LoggerHelperGetMessage.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/LogsGetMessage.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/NonThrowableGetMessage.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/PersistSinkGetMessage.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/PsrLoggerGetMessage.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/ThrowableItselfIntoSink.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/_stubs.php create mode 100644 tests/Rules/ForbidRawExceptionMessageInResponseRuleTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 289efa2..c56eb8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and ## [Unreleased] +### Added + +- `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). **Exemption:** a `// @leak-safe: ` comment on the sink call line (or in the contiguous comment block directly above it) suppresses the rule for a proven-safe case — an app-authored exception message carrying no raw payload (the codebook `SendCodyReportAction` shape); the standard PHPStan inline-ignore mechanism on the identifier is the alternative. Identifier: `forbidRawExceptionMessageInResponse.rawMessageInResponse`. **Deliberate misses (v1 scope):** `getTraceAsString()` / `__toString()` and other Throwable accessors (a future minor can widen the accessor set), and a Throwable laundered through a helper/formatter call whose return type is no longer `Throwable` (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 - `EnforceResourceDataValidatorOptInRule` + `EnforceFormRequestToDtoRule` + `EnforceAuditModelProtectionsRule` — migrated the internal inheritance gate off the deprecated `ClassReflection::isSubclassOf(string)` API onto `isSubclassOfClass(ClassReflection)` (war-room enforcement queue #112). `isSubclassOf(string)` is `@deprecated Use isSubclassOfClass instead.` in PHPStan 2.2+ and **removed in PHPStan 3.x** — a latent break for every consumer's static analysis the day this package (the fleet's canonical static-analysis backbone) targets PHPStan 3. Each of the three rules now injects `PHPStan\Reflection\ReflectionProvider` (constructor DI, autowired by the PHPStan/Nette container — no `extension.neon` argument wiring needed, proven by each rule's container-resolution test) and resolves its configured/known base FQCN via `reflectionProvider->hasClass()/getClass()` before calling `isSubclassOfClass()`. **Behaviour is byte-for-byte preserved:** the migration inlines the deprecated method's own body (`if (!hasClass($fqcn)) return false; return isSubclassOfClass(getClass($fqcn));`), so the load-bearing unknown-base-class no-op — a tree lacking the configured base class (or `Illuminate\Database\Eloquent\Model` for the audit rule) silently does not fire, the "consumers analysing non-Laravel trees are unaffected" guarantee — is reproduced exactly. Pinned by a new base-class-absent no-op test per rule (a configured base FQCN absent from the analysed tree ⇒ zero errors); all existing positive tests stay green. **Versioning: PATCH per ADR-0021 §Versioning** (internal API migration / future-proofing — adds no errors, removes none, no consumer-visible behaviour change). Seed: war-room enforcement queue #112. diff --git a/CLAUDE.md b/CLAUDE.md index cb50ae0..d06919e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -26,6 +26,7 @@ Composer package distributing war-room-doctrine PHPStan rules across `script-dev | `ForbidHttpExceptionInActionsRule` | War-room §Explicit over implicit + §FormRequest → DTO → Action | `forbidHttpExceptionInActions.httpExceptionInAction` (type-aware sibling of `ForbidAbortHelperRule`; bans throwing the `Symfony\…\HttpException` family from `App\Actions\*`. `Illuminate\Validation\ValidationException` out of scope. shipped v0.5.0) | | `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]`) | | `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` | @@ -111,7 +112,7 @@ SemVer per ADR-0021: ### War-room Architectural Principle rules (no published ADR) -- **Explicit over implicit** — package distributes `ForbidAbortHelperRule` (bans `abort()` / `abort_if()` / `abort_unless()`; shipped), `EnforceCurrentUserAttributeRule` (flags `Request::user()` / `Auth::user()` / `auth()->user()` in `App\Http\Controllers`, steering to the `#[CurrentUser]` container attribute per Architectural Principle #9; shipped v0.4.0), `ForbidHttpExceptionInActionsRule` (type-aware sibling of `ForbidAbortHelperRule` — bans throwing the `Symfony\…\HttpException` family from `App\Actions\*`; HTTP status concerns belong to the HTTP layer per Principles #1 + #3; `ValidationException` deliberately out of scope; shipped v0.5.0), and `ForbidResourceWrappedInJsonResponseRule` (bans wrapping a `JsonResource` in `response()->json()` / `new JsonResponse()` inside controllers per Principle #1 + ADR-0009; shipped v0.5.0). These enforce war-room §Architectural Principles (the last two also touching numbered ADRs) — each rule's docblock "Doctrine source" line names its authority. +- **Explicit over implicit** — package distributes `ForbidAbortHelperRule` (bans `abort()` / `abort_if()` / `abort_unless()`; shipped), `EnforceCurrentUserAttributeRule` (flags `Request::user()` / `Auth::user()` / `auth()->user()` in `App\Http\Controllers`, steering to the `#[CurrentUser]` container attribute per Architectural Principle #9; shipped v0.4.0), `ForbidHttpExceptionInActionsRule` (type-aware sibling of `ForbidAbortHelperRule` — bans throwing the `Symfony\…\HttpException` family from `App\Actions\*`; HTTP status concerns belong to the HTTP layer per Principles #1 + #3; `ValidationException` deliberately out of scope; shipped v0.5.0), `ForbidResourceWrappedInJsonResponseRule` (bans wrapping a `JsonResource` in `response()->json()` / `new JsonResponse()` inside controllers per Principle #1 + ADR-0009; shipped v0.5.0), and `ForbidRawExceptionMessageInResponseRule` (bans a raw `Throwable::getMessage()` — or the `Throwable` itself — reaching a client-facing response sink per Principle #1 + information-disclosure hardening for the ISO 27001 / AVG / NEN 7510 consumers; default sink `Laravel\Mcp\Response::error`, configurable via `rawExceptionMessageSinks`; server-side logging never flags; `// @leak-safe:` exemption; on `main`, `[Unreleased]`). These enforce war-room §Architectural Principles (some also touching numbered ADRs) — each rule's docblock "Doctrine source" line names its authority. ### War-room internal ADRs diff --git a/README.md b/README.md index a9fe262..e43ceaf 100644 --- a/README.md +++ b/README.md @@ -48,6 +48,7 @@ includes: | `EnforceAuditTransactionScopeRule` | `enforceAuditTransactionScope.nonTransactionalMutationInClosure` | `App\Actions\*` whose `execute()` calls `transaction(...)` with a literal closure | Mutating `StatefulGuard` / `Session` / `Cache` / `Bus` / `Queue` / `Mailer` / `Notification` / `Broadcaster` / `Filesystem` state (or their `Illuminate\Support\Facades\*` counterparts) inside the closure is an error. Reads (`Auth::user()`, `Session::get()`, `Cache::get()`) are permitted. Doctrine: ADR-0029 (Audit Row Durability Contract) §Decision rule 3. | | `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 with a `// @leak-safe: ` comment on/above the sink line. Doctrine: war-room §Explicit over implicit (#1); information-disclosure hardening. | | `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). | @@ -229,6 +230,39 @@ parameters: path: app/Models/Audit/SomeProjectionLog.php ``` +### `ForbidRawExceptionMessageInResponseRule` — configurable sinks + `@leak-safe` exemption + +The rule flags a raw `Throwable::getMessage()` (or the `Throwable` itself) reaching a **client-facing response sink** — an information-disclosure leak. The built-in default sink `Laravel\Mcp\Response::error` is always armed; a consumer adds more (a persist-error setter, a `MarkInvoiceFailed` Action) via the `rawExceptionMessageSinks` parameter — a list of `FQCN::method` signatures, default `[]`: + +```neon +parameters: + rawExceptionMessageSinks: + # single backslashes — NEON keeps them literal outside double quotes + - 'App\Support\InvoiceLog::recordError' + - 'App\Actions\Invoice\MarkInvoiceFailed::execute' +``` + +A signature matches BOTH call forms — a static call whose resolved class equals the FQCN, and an instance call whose receiver is a subtype of the FQCN — so an injected persist sink is caught without a rule change. + +**Server-side logging is never flagged** — `Log::`, `logger()->`, PSR `LoggerInterface` log-level calls, and `report()` are the remediation. Log the raw message; return a stable, app-authored message to the client: + +```php +} catch (\Throwable $e) { + logger()->error('invoice.show failed', ['exception' => $e->getMessage()]); // fine — server-side + return Response::error('Could not load the invoice.'); // fine — app-authored + // return Response::error('Failed: ' . $e->getMessage()); // ERROR — raw leak +} +``` + +**Exempting a proven-safe sink** — when the exception message is app-authored and carries no raw payload (the codebook `SendCodyReportAction` shape), mark it with a `// @leak-safe: ` comment on the sink line or in the comment block directly above it: + +```php +// @leak-safe: SendCodyReportException carries only an app-authored, payload-free message +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. + ### 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 866a555..caa6960 100644 --- a/extension.neon +++ b/extension.neon @@ -53,6 +53,19 @@ parameters: auditModelNameSuffixes: - 'AuditLog' + # `ForbidRawExceptionMessageInResponseRule`: ADDITIONAL client-facing sink + # signatures, each in `FQCN::method` form, that must not receive a raw + # `Throwable::getMessage()` (or the Throwable itself). These are ADDED to the + # always-armed built-in default sink `Laravel\Mcp\Response::error`; the + # default is empty so the rule is safe to adopt with only the MCP shape + # armed. A signature matches BOTH call forms — a static call whose resolved + # class equals the FQCN, and an instance call whose receiver is a subtype of + # the FQCN — so a consumer names its persist-error sink (an invoice-log + # setter, a MarkInvoiceFailed Action) here without a rule change. Logger / + # report() calls are excluded structurally and never need listing. Each FQCN + # uses single backslashes — see the NEON-quoting note above. + rawExceptionMessageSinks: [] + parametersSchema: resourceDataBaseClass: string() formRequestBaseClass: string() @@ -60,6 +73,7 @@ parametersSchema: controllerNamespacePrefixes: listOf(string()) auditModelNamespacePrefixes: listOf(string()) auditModelNameSuffixes: listOf(string()) + rawExceptionMessageSinks: listOf(string()) services: - @@ -126,6 +140,11 @@ services: auditModelNamespacePrefixes: %auditModelNamespacePrefixes% auditModelNameSuffixes: %auditModelNameSuffixes% tags: [phpstan.rules.rule] + - + class: ScriptDevelopment\PhpstanWarroomRules\Rules\ForbidRawExceptionMessageInResponseRule + arguments: + rawExceptionMessageSinks: %rawExceptionMessageSinks% + tags: [phpstan.rules.rule] - class: ScriptDevelopment\PhpstanWarroomRules\Type\ConnectionTransactionReturnTypeExtension tags: [phpstan.broker.dynamicMethodReturnTypeExtension] diff --git a/src/Rules/ForbidRawExceptionMessageInResponseRule.php b/src/Rules/ForbidRawExceptionMessageInResponseRule.php new file mode 100644 index 0000000..b77d0e4 --- /dev/null +++ b/src/Rules/ForbidRawExceptionMessageInResponseRule.php @@ -0,0 +1,391 @@ +getMessage())` (ublgenie's 8 MCP tools + + * codebook `DeleteChapterTool` — every one concatenates the raw message into + * the error response). `Laravel\Mcp\Response::error` is therefore the built-in + * default sink; a consumer adds its own PERSIST sinks (an invoice-log setter, + * a `MarkInvoiceFailed` Action) via the `rawExceptionMessageSinks` parameter, + * default `[]`, so the rule is safe to adopt with only the MCP shape armed. + * + * A sink is a `FQCN::method` signature. It is matched in BOTH call forms: + * + * - a `StaticCall` whose resolved class equals the sink FQCN + * (`Response::error(...)`), and + * - a `MethodCall` whose receiver type is a subtype of the sink FQCN + * (`$this->response->error(...)`, an injected persist-sink service). + * + * A matched sink call is flagged when ANY argument is, directly OR via string + * concatenation (`'context: ' . $e->getMessage()`): + * + * - a `->getMessage()` `MethodCall` on an expression whose type is a subtype + * of `\Throwable`, or + * - a `\Throwable` expression passed directly into the sink. + * + * Type-aware discrimination is load-bearing: `$validator->getMessage()` (a + * non-`Throwable` receiver) does NOT fire — only a message pulled off an actual + * exception is a leak. + * + * NEVER flagged (mandatory false-positive exclusions — the remediation pattern, + * not 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 CORRECT — it is where the raw message is *supposed* to go. + * Because a sink is keyed on `FQCN::method`, a logger is never a sink under + * the default config; this exclusion additionally short-circuits BEFORE + * sink matching, so a consumer that adds a broad sink can never turn a + * logger into a false positive. + * + * Exemption: a `// @leak-safe: ` comment on the sink call line (or + * in the contiguous comment block directly above it) suppresses the rule for a + * proven-safe case — an app-authored exception message carrying no raw payload + * (the codebook `SendCodyReportAction` shape). The standard PHPStan inline- + * ignore mechanism on the identifier + * `forbidRawExceptionMessageInResponse.rawMessageInResponse` is the alternative. + * + * Out of scope (deliberately, for v1): + * + * - `getTraceAsString()` / `__toString()` / other Throwable accessors — the + * confirmed leak surface is `getMessage()` and the Throwable itself; a + * future minor can widen the accessor set. + * - Sinks passed a Throwable through a helper/formatter call + * (`Response::error($this->format($e))`) — the type at the sink boundary is + * the formatter's return, not a Throwable; a false negative is accepted + * (ADR-0021 posture: false negatives acceptable, false positives are not). + * + * @implements Rule + */ +final class ForbidRawExceptionMessageInResponseRule implements Rule +{ + /** + * Built-in default sink — the Laravel-MCP `Response::error(...)` shape that + * dominates the confirmed leak surface. Always armed; consumer-configured + * sinks are added to it, never replace it. + */ + private const string DEFAULT_SINK = 'Laravel\Mcp\Response::error'; + + private const string THROWABLE = Throwable::class; + + /** Log-level method names — a call to any of these on a logger is the remediation, never a leak. */ + private const array LOGGER_METHODS = [ + 'info', 'warning', 'error', 'critical', 'debug', 'log', 'notice', 'alert', 'emergency', + ]; + + /** Logger receivers whose log-level calls are excluded. */ + private const string LOG_FACADE = Log::class; + + private const string PSR_LOGGER = LoggerInterface::class; + + private const string LOGGER_HELPER = 'logger'; + + /** + * Parsed sink signatures — each `['class' => FQCN, 'method' => name]`. + * + * @var list + */ + private array $sinks; + + /** + * @param list $rawExceptionMessageSinks additional client-facing + * sink signatures in + * `FQCN::method` form (e.g. a + * consumer's persist-error + * setter). Merged with the + * built-in `Response::error` + * default; empty by default so + * the rule is safe to adopt. + */ + public function __construct( + array $rawExceptionMessageSinks = [], + ) { + $this->sinks = []; + + foreach ([self::DEFAULT_SINK, ...$rawExceptionMessageSinks] as $signature) { + $parsed = $this->parseSinkSignature($signature); + + if ($parsed !== null) { + $this->sinks[] = $parsed; + } + } + } + + public function getNodeType(): string + { + return CallLike::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if (!$node instanceof StaticCall && !$node instanceof MethodCall) { + return []; + } + + // Mandatory exclusion — a logger call is the remediation, not a leak. + // Short-circuits before sink matching so a broad consumer sink can never + // criminalize server-side logging. (`report()` is a FuncCall, never + // examined here, so it is structurally out of scope already.) + if ($this->isLoggerCall($node, $scope)) { + return []; + } + + if (!$this->isConfiguredSink($node, $scope)) { + return []; + } + + if ($this->hasLeakSafeMarker($node, $scope)) { + return []; + } + + foreach ($node->getArgs() as $arg) { + if ($this->exprCarriesRawExceptionMessage($arg->value, $scope)) { + return [$this->buildError($node)]; + } + } + + return []; + } + + /** + * @return array{class: string, method: string}|null + */ + private function parseSinkSignature(string $signature): ?array + { + $parts = explode('::', $signature); + + if (count($parts) !== 2 || $parts[0] === '' || $parts[1] === '') { + return null; + } + + return ['class' => $parts[0], 'method' => $parts[1]]; + } + + /** + * True when the call matches a configured sink in either form: a static + * call whose resolved class equals the sink FQCN, or an instance call whose + * receiver type is a subtype of the sink FQCN. Method name must match. + */ + private function isConfiguredSink(MethodCall|StaticCall $node, Scope $scope): bool + { + if (!$node->name instanceof Identifier) { + return false; + } + + $methodName = $node->name->toString(); + + foreach ($this->sinks as $sink) { + if ($sink['method'] !== $methodName) { + continue; + } + + if ($node instanceof StaticCall) { + if ($node->class instanceof Name && $scope->resolveName($node->class) === $sink['class']) { + return true; + } + + continue; + } + + if ((new ObjectType($sink['class']))->isSuperTypeOf($scope->getType($node->var))->yes()) { + return true; + } + } + + return false; + } + + /** + * True when `$expr` is, directly or through string concatenation, a raw + * exception message: a `Throwable::getMessage()` call or a `Throwable` + * expression itself. + */ + private function exprCarriesRawExceptionMessage(Expr $expr, Scope $scope): bool + { + if ($expr instanceof Concat) { + return $this->exprCarriesRawExceptionMessage($expr->left, $scope) + || $this->exprCarriesRawExceptionMessage($expr->right, $scope); + } + + if ($this->isThrowableGetMessageCall($expr, $scope)) { + return true; + } + + // The Throwable itself passed into the sink (a `getMessage()` call + // returns string, so this branch never double-counts the call above). + return $this->typeIsThrowable($scope->getType($expr)); + } + + private function isThrowableGetMessageCall(Expr $expr, Scope $scope): bool + { + return $expr instanceof MethodCall + && $expr->name instanceof Identifier + && $expr->name->toString() === 'getMessage' + && $this->typeIsThrowable($scope->getType($expr->var)); + } + + private function typeIsThrowable(Type $type): bool + { + return (new ObjectType(self::THROWABLE))->isSuperTypeOf($type)->yes(); + } + + /** + * Recognise a logger call so it is never treated as a leak. Covers + * `Log::error(...)`, `logger()->error(...)`, and a PSR `LoggerInterface` + * instance call. (`report(...)` is a FuncCall, already out of scope.). + */ + private function isLoggerCall(MethodCall|StaticCall $node, Scope $scope): bool + { + if (!$node->name instanceof Identifier) { + return false; + } + + $methodName = $node->name->toString(); + + if ($node instanceof StaticCall) { + return in_array($methodName, self::LOGGER_METHODS, true) + && $node->class instanceof Name + && $scope->resolveName($node->class) === self::LOG_FACADE; + } + + if (!in_array($methodName, self::LOGGER_METHODS, true)) { + return false; + } + + // `logger()->error(...)` — receiver is the `logger()` helper FuncCall. + if ( + $node->var instanceof FuncCall + && $node->var->name instanceof Name + && $node->var->name->toString() === self::LOGGER_HELPER + ) { + return true; + } + + // `$this->logger->error(...)` — receiver typed as a PSR logger. + return (new ObjectType(self::PSR_LOGGER))->isSuperTypeOf($scope->getType($node->var))->yes(); + } + + /** + * Honour a `// @leak-safe: ` marker on the sink call line or in + * the contiguous comment block directly above it. Mirrors + * `EnforceAuditSnapshotOnRetryRule::hasExemptionMarker()` — PHPStan does not + * propagate a `parent` attribute onto nodes, so the raw-source scan is the + * reliable path. + */ + private function hasLeakSafeMarker(MethodCall|StaticCall $node, Scope $scope): bool + { + foreach ($node->getComments() as $comment) { + if (str_contains($comment->getText(), '@leak-safe')) { + return true; + } + } + + $file = $scope->getFile(); + + if ($file === '' || !is_file($file)) { + return false; + } + + $source = @file_get_contents($file); + + if ($source === false) { + return false; + } + + $lines = explode("\n", $source); + $startLine = $node->getStartLine(); + + // Same-line trailing comment (`Response::error(...); // @leak-safe: ...`). + if (isset($lines[$startLine - 1]) && str_contains($lines[$startLine - 1], '@leak-safe')) { + return true; + } + + // Contiguous comment block immediately above the sink call. + $idx = $startLine - 2; + + while ($idx >= 0) { + $line = mb_trim($lines[$idx]); + + if ($line === '') { + $idx--; + + continue; + } + + $isCommentLine = str_starts_with($line, '//') + || str_starts_with($line, '*') + || str_starts_with($line, '/*'); + + if (!$isCommentLine) { + return false; + } + + if (str_contains($line, '@leak-safe')) { + return true; + } + + $idx--; + } + + return false; + } + + private function buildError(MethodCall|StaticCall $node): IdentifierRuleError + { + return RuleErrorBuilder::message( + 'Raw exception message reaches a client-facing response sink. ' + . 'Passing Throwable::getMessage() (or the Throwable itself) to a response leaks internal detail ' + . '(stack-trace fragments, SQL, file paths) to the API client. Log the raw message server-side ' + . '(Log::/report()) and return a stable, app-authored message. ' + . 'Suppress a proven-safe app-authored message with a `// @leak-safe: ` comment on the sink line.', + ) + ->identifier('forbidRawExceptionMessageInResponse.rawMessageInResponse') + ->line($node->getStartLine()) + ->build(); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/AppAuthoredLiteralMessage.php b/tests/Fixtures/RawExceptionMessageInResponse/AppAuthoredLiteralMessage.php new file mode 100644 index 0000000..8057d82 --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/AppAuthoredLiteralMessage.php @@ -0,0 +1,19 @@ +getMessage()); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/ErrorSinkDirectGetMessage.php b/tests/Fixtures/RawExceptionMessageInResponse/ErrorSinkDirectGetMessage.php new file mode 100644 index 0000000..fb6610c --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/ErrorSinkDirectGetMessage.php @@ -0,0 +1,18 @@ +getMessage()); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/LeakSafeExempted.php b/tests/Fixtures/RawExceptionMessageInResponse/LeakSafeExempted.php new file mode 100644 index 0000000..fcb918c --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/LeakSafeExempted.php @@ -0,0 +1,18 @@ +getMessage()); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/LeakSafeSameLineExempted.php b/tests/Fixtures/RawExceptionMessageInResponse/LeakSafeSameLineExempted.php new file mode 100644 index 0000000..3e147e7 --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/LeakSafeSameLineExempted.php @@ -0,0 +1,16 @@ +getMessage()); // @leak-safe: app-authored, payload-free + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/LogFacadeDirectGetMessage.php b/tests/Fixtures/RawExceptionMessageInResponse/LogFacadeDirectGetMessage.php new file mode 100644 index 0000000..5298869 --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/LogFacadeDirectGetMessage.php @@ -0,0 +1,19 @@ +getMessage())` — direct raw message to the Log facade. + // Server-side logging; the static-logger exclusion must hold it silent + // EVEN when Log::error is configured as a sink. + Log::error($e->getMessage()); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/LoggerHelperGetMessage.php b/tests/Fixtures/RawExceptionMessageInResponse/LoggerHelperGetMessage.php new file mode 100644 index 0000000..7c86e46 --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/LoggerHelperGetMessage.php @@ -0,0 +1,18 @@ +error(...)` — the helper form of server-side logging. The + // logger exclusion must hold this silent EVEN when the logger method is + // configured as a sink (the exclusion short-circuits before sink match). + logger()->error($e->getMessage()); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/LogsGetMessage.php b/tests/Fixtures/RawExceptionMessageInResponse/LogsGetMessage.php new file mode 100644 index 0000000..3737d65 --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/LogsGetMessage.php @@ -0,0 +1,21 @@ + $e->getMessage()]); + logger()->error($e->getMessage()); + report($e); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/NonThrowableGetMessage.php b/tests/Fixtures/RawExceptionMessageInResponse/NonThrowableGetMessage.php new file mode 100644 index 0000000..bc11df3 --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/NonThrowableGetMessage.php @@ -0,0 +1,18 @@ +getMessage()); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/PersistSinkGetMessage.php b/tests/Fixtures/RawExceptionMessageInResponse/PersistSinkGetMessage.php new file mode 100644 index 0000000..675febe --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/PersistSinkGetMessage.php @@ -0,0 +1,23 @@ +log->recordError('failed: ' . $e->getMessage()); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/PsrLoggerGetMessage.php b/tests/Fixtures/RawExceptionMessageInResponse/PsrLoggerGetMessage.php new file mode 100644 index 0000000..6ca6467 --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/PsrLoggerGetMessage.php @@ -0,0 +1,24 @@ +reporter->logger->error($e->getMessage()); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/ThrowableItselfIntoSink.php b/tests/Fixtures/RawExceptionMessageInResponse/ThrowableItselfIntoSink.php new file mode 100644 index 0000000..17b5767 --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/ThrowableItselfIntoSink.php @@ -0,0 +1,18 @@ +` / PSR `LoggerInterface` +// log-level call, or `report()`, is never a leak. +// +// composer.json requires none of laravel/mcp, psr/log, or illuminate/support, +// so the sink base, the PSR logger contract, the Log facade, the `logger()` / +// `report()` helpers, and a non-Throwable object carrying a `getMessage()` +// method are stubbed here with their real FQCNs — enough for PHPStan's FQCN +// resolution + subtype inference without pulling the real packages. `Throwable` +// and its exception hierarchy (`Exception`, `RuntimeException`, +// `InvalidArgumentException`) are PHP core, so fixtures use them directly with +// no stub. Mirrors the sibling rules' fixture-stub approach. + +namespace Laravel\Mcp { + // The MCP tool response. `error()` is the confirmed client-facing sink — + // ublgenie's 8 MCP tools + codebook DeleteChapterTool all return + // `Response::error('...' . $e->getMessage())`. + final class Response + { + public static function error(string $message): self + { + return new self; + } + } +} + +namespace Psr\Log { + // Minimal PSR-3 logger contract — the instance-logger exclusion resolves a + // receiver's subtype against this. + interface LoggerInterface + { + public function error(string $message, array $context = []): void; + + public function info(string $message, array $context = []): void; + } +} + +namespace Illuminate\Support\Facades { + // The Log facade — static log-level calls resolve against this FQCN. + class Log + { + public static function error(string $message, array $context = []): void {} + + public static function info(string $message, array $context = []): void {} + } +} + +namespace App\Support { + use Psr\Log\LoggerInterface; + + // A consumer-side PERSIST sink — records a failure message to a store. Not + // a logger; a configured `App\Support\InvoiceLog::recordError` sink. + final class InvoiceLog + { + public function recordError(string $message): void {} + } + + // A non-Throwable object that happens to expose getMessage() — the type + // gate must NOT treat its getMessage() as a leak. + final class ValidatorBag + { + public function getMessage(): string + { + return 'app-authored validation summary'; + } + } + + // A class holding an injected PSR logger, for the instance-logger exclusion. + final class ReportsToLogger + { + public function __construct( + public LoggerInterface $logger, + ) {} + } +} + +namespace { + use Psr\Log\LoggerInterface; + + // The `logger()` helper — normally in illuminate/foundation's helpers file. + if (!\function_exists('logger')) { + function logger(): LoggerInterface + { + return new class implements LoggerInterface { + public function error(string $message, array $context = []): void {} + + public function info(string $message, array $context = []): void {} + }; + } + } + + // The `report()` helper — normally in illuminate/foundation's helpers file. + if (!\function_exists('report')) { + function report(Throwable $throwable): void {} + } +} diff --git a/tests/Rules/ForbidRawExceptionMessageInResponseRuleTest.php b/tests/Rules/ForbidRawExceptionMessageInResponseRuleTest.php new file mode 100644 index 0000000..b7a7f45 --- /dev/null +++ b/tests/Rules/ForbidRawExceptionMessageInResponseRuleTest.php @@ -0,0 +1,225 @@ + + */ +final class ForbidRawExceptionMessageInResponseRuleTest extends RuleTestCase +{ + private const string MESSAGE = 'Raw exception message reaches a client-facing response sink. ' + . 'Passing Throwable::getMessage() (or the Throwable itself) to a response leaks internal detail ' + . '(stack-trace fragments, SQL, file paths) to the API client. Log the raw message server-side ' + . '(Log::/report()) and return a stable, app-authored message. ' + . 'Suppress a proven-safe app-authored message with a `// @leak-safe: ` comment on the sink line.'; + + private const string STUBS = __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/_stubs.php'; + + /** + * Override hook: when set, `getRule()` returns this instance instead of the + * default. Lets a single test reconfigure `rawExceptionMessageSinks`. + */ + private ?Rule $ruleOverride = null; + + public function testFlagsConcatGetMessageIntoResponseError(): void + { + // The dominant MCP-tool shape: Response::error('x: ' . $e->getMessage()). + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/ErrorSinkConcatGetMessage.php'], + [[self::MESSAGE, 16]], + ); + } + + public function testFlagsDirectGetMessageIntoResponseError(): void + { + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/ErrorSinkDirectGetMessage.php'], + [[self::MESSAGE, 16]], + ); + } + + public function testFlagsThrowableItselfIntoSink(): void + { + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/ThrowableItselfIntoSink.php'], + [[self::MESSAGE, 16]], + ); + } + + public function testFlagsConfiguredPersistSink(): void + { + // The persist sink fires ONLY when its signature is configured. + $this->ruleOverride = new ForbidRawExceptionMessageInResponseRule( + ['App\Support\InvoiceLog::recordError'], + ); + + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/PersistSinkGetMessage.php'], + [[self::MESSAGE, 21]], + ); + } + + public function testIgnoresPersistSinkWhenNotConfigured(): void + { + // Same fixture under the default config — the persist sink is not armed, + // so the raw message flowing into it is silent. Pins "empty param = + // only Response::error built-in". + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/PersistSinkGetMessage.php'], + [], + ); + } + + public function testIgnoresLoggingOfRawMessage(): void + { + // Log::error / logger()->error / report() — the remediation, never a leak. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/LogsGetMessage.php'], + [], + ); + } + + public function testIgnoresAppAuthoredLiteralMessage(): void + { + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/AppAuthoredLiteralMessage.php'], + [], + ); + } + + public function testIgnoresLeakSafeExemptedSink(): void + { + // `// @leak-safe:` marker in the comment block above the sink call. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/LeakSafeExempted.php'], + [], + ); + } + + public function testIgnoresGetMessageOnNonThrowable(): void + { + // getMessage() on a non-Throwable receiver — the type gate keeps it silent. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/NonThrowableGetMessage.php'], + [], + ); + } + + public function testIgnoresInstancePsrLoggerUnderDefaultConfig(): void + { + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/PsrLoggerGetMessage.php'], + [], + ); + } + + public function testLoggerExclusionOverridesAConfiguredLoggerSink(): void + { + // Even if a consumer (mis)configures a logger method as a sink, the + // logger/report exclusion short-circuits first — server-side logging is + // never criminalized. Gives the mandatory exclusion real teeth. + $this->ruleOverride = new ForbidRawExceptionMessageInResponseRule( + ['Psr\Log\LoggerInterface::error'], + ); + + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/PsrLoggerGetMessage.php'], + [], + ); + } + + public function testStaticLoggerExclusionOverridesAConfiguredLogFacadeSink(): void + { + // Configure the Log facade method AS a sink; the static-logger exclusion + // must still hold `Log::error($e->getMessage())` silent. Gives the + // static-call branch of the exclusion real teeth. + $this->ruleOverride = new ForbidRawExceptionMessageInResponseRule( + ['Illuminate\Support\Facades\Log::error'], + ); + + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/LogFacadeDirectGetMessage.php'], + [], + ); + } + + public function testHelperLoggerExclusionOverridesAConfiguredLoggerSink(): void + { + // Configure the PSR logger method AS a sink; `logger()->error(...)` (the + // helper's return type IS LoggerInterface, so it would match the sink) + // must still be held silent by the logger()-helper exclusion branch. + $this->ruleOverride = new ForbidRawExceptionMessageInResponseRule( + ['Psr\Log\LoggerInterface::error'], + ); + + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/LoggerHelperGetMessage.php'], + [], + ); + } + + public function testIgnoresSameLineLeakSafeMarker(): void + { + // `// @leak-safe:` trailing comment on the sink call line itself. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/LeakSafeSameLineExempted.php'], + [], + ); + } + + public function testMalformedSinkSignaturesAreSkippedNotFatal(): void + { + // Garbage sink signatures (no `::`, too many `::`, empty class, empty + // method) are skipped by the signature parser — they neither crash nor + // arm a bogus sink, and the always-armed Response::error default still + // fires. Pins the parser's guard. + $this->ruleOverride = new ForbidRawExceptionMessageInResponseRule( + ['NoSeparatorHere', 'A::b::c', '::emptyClass', 'EmptyMethod::'], + ); + + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/ErrorSinkConcatGetMessage.php'], + [[self::MESSAGE, 16]], + ); + } + + public function testRuleResolvesFromExtensionNeonAndFiresOnDefaultSink(): void + { + // End-to-end pin on the extension.neon registration path consumers + // actually use: resolve the rule from the PHPStan container so the + // shipped `rawExceptionMessageSinks` default ([]) + the built-in + // Response::error sink wiring are exercised — NOT the PHP constructor + // default. A NEON regression would silently no-op the rule; this asserts + // the canonical Response::error leak still flags under the shipped wiring. + $this->ruleOverride = self::getContainer()->getByType(ForbidRawExceptionMessageInResponseRule::class); + + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/ErrorSinkConcatGetMessage.php'], + [[self::MESSAGE, 16]], + ); + } + + /** + * Load the shipped extension.neon so the container-resolved test can pull + * the rule out with its NEON-configured `rawExceptionMessageSinks`. + * + * @return array + */ + public static function getAdditionalConfigFiles(): array + { + return [ + __DIR__ . '/../../extension.neon', + ]; + } + + protected function getRule(): Rule + { + return $this->ruleOverride ?? new ForbidRawExceptionMessageInResponseRule; + } +} From 6896414bf6cae2e8ac4185532eb4fae2412d9f3a Mon Sep 17 00:00:00 2001 From: Gerard Oosterhof Date: Thu, 23 Jul 2026 09:40:36 +0200 Subject: [PATCH 2/2] feat(rules): safeMessageExceptionClasses allowlist + nullsafe matching (bus review follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses all three findings on PR #59: Major — the rule flagged its own motivating example: codebook DeleteChapterTool's DependentModelRelationException::getMessage() passthrough is arch-test-PINNED as app-authored (prove-safe), yet the only escape was a per-call-site @leak-safe annotation. New safeMessageExceptionClasses parameter (listOf(string()), default []): exception FQCNs whose message discipline the consuming territory pins with an arch test are exempt at CONFIG level. Type-aware (subtypes inherit); covers the MESSAGE only — passing the Throwable itself still fires (__toString carries class/file/trace regardless of message discipline), pinned by test. Minor — the docblock's Out-of-scope section now names the mundane gap alongside the formatter one: plain local-variable extraction ($msg = $e->getMessage(); sink($msg)) — type at the sink is string, provenance gone; same accepted-false-negative posture (ADR-0021). Nit — $e?->getMessage() is a NullsafeMethodCall, a distinct AST node the MethodCall-only matcher missed. Now matched; typeIsThrowable strips null first (the nullsafe receiver types as Throwable|null) with a NeverType guard so a pure-null receiver stays silent. 4 new tests / 3 fixtures: nullsafe flagged, safe class flagged under default config, safe class silent when configured, Throwable-itself still flagged under the safe config. README + CHANGELOG updated. Gates: 196 tests / 281 assertions, phpstan OK, pint clean, coverage 89.68% (>=83). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01APYknX78PNXAChnD17Dayx --- CHANGELOG.md | 2 +- README.md | 12 +- extension.neon | 3 + ...orbidRawExceptionMessageInResponseRule.php | 110 +++++++++++++++--- .../NullsafeGetMessage.php | 19 +++ .../SafeMessageExceptionPassthrough.php | 20 ++++ .../SafeMessageExceptionThrowableItself.php | 19 +++ .../RawExceptionMessageInResponse/_stubs.php | 8 ++ ...dRawExceptionMessageInResponseRuleTest.php | 55 ++++++++- 9 files changed, 225 insertions(+), 23 deletions(-) create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/NullsafeGetMessage.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionPassthrough.php create mode 100644 tests/Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionThrowableItself.php diff --git a/CHANGELOG.md b/CHANGELOG.md index c56eb8c..8a6022d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and ### Added -- `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). **Exemption:** a `// @leak-safe: ` comment on the sink call line (or in the contiguous comment block directly above it) suppresses the rule for a proven-safe case — an app-authored exception message carrying no raw payload (the codebook `SendCodyReportAction` shape); the standard PHPStan inline-ignore mechanism on the identifier is the alternative. Identifier: `forbidRawExceptionMessageInResponse.rawMessageInResponse`. **Deliberate misses (v1 scope):** `getTraceAsString()` / `__toString()` and other Throwable accessors (a future minor can widen the accessor set), and a Throwable laundered through a helper/formatter call whose return type is no longer `Throwable` (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. +- `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/README.md b/README.md index e43ceaf..ea09b80 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,7 @@ includes: | `EnforceAuditTransactionScopeRule` | `enforceAuditTransactionScope.nonTransactionalMutationInClosure` | `App\Actions\*` whose `execute()` calls `transaction(...)` with a literal closure | Mutating `StatefulGuard` / `Session` / `Cache` / `Bus` / `Queue` / `Mailer` / `Notification` / `Broadcaster` / `Filesystem` state (or their `Illuminate\Support\Facades\*` counterparts) inside the closure is an error. Reads (`Auth::user()`, `Session::get()`, `Cache::get()`) are permitted. Doctrine: ADR-0029 (Audit Row Durability Contract) §Decision rule 3. | | `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 with a `// @leak-safe: ` comment on/above the sink line. Doctrine: war-room §Explicit over implicit (#1); information-disclosure hardening. | +| `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. | | `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). | @@ -254,7 +254,15 @@ A signature matches BOTH call forms — a static call whose resolved class equal } ``` -**Exempting a proven-safe sink** — when the exception message is app-authored and carries no raw payload (the codebook `SendCodyReportAction` shape), mark it with a `// @leak-safe: ` comment on the sink line or in the comment block directly above it: +**Exempting a proven-safe exception CLASS** — when a domain exception's message discipline is proven app-authored (the codebook `DependentModelRelationException` shape, pinned by an arch test in the consuming territory), list it in `safeMessageExceptionClasses` (default `[]`) instead of annotating every call site. Type-aware — subtypes inherit the allowance; the exemption covers the **message only** (passing the Throwable itself still fires: `__toString` carries class, file, and trace regardless of message discipline). List a class here only when an arch test pins its message discipline — config without the pin is a hole, not an exemption: + +```neon +parameters: + safeMessageExceptionClasses: + - 'App\Exceptions\DependentModelRelationException' +``` + +**Exempting a proven-safe call site** — when the exception message is app-authored and carries no raw payload but a class-level listing does not fit (the codebook `SendCodyReportAction` shape), mark it with a `// @leak-safe: ` comment on the sink line or in the comment block directly above it: ```php // @leak-safe: SendCodyReportException carries only an app-authored, payload-free message diff --git a/extension.neon b/extension.neon index caa6960..a9137f1 100644 --- a/extension.neon +++ b/extension.neon @@ -65,6 +65,7 @@ parameters: # report() calls are excluded structurally and never need listing. Each FQCN # uses single backslashes — see the NEON-quoting note above. rawExceptionMessageSinks: [] + safeMessageExceptionClasses: [] parametersSchema: resourceDataBaseClass: string() @@ -74,6 +75,7 @@ parametersSchema: auditModelNamespacePrefixes: listOf(string()) auditModelNameSuffixes: listOf(string()) rawExceptionMessageSinks: listOf(string()) + safeMessageExceptionClasses: listOf(string()) services: - @@ -144,6 +146,7 @@ services: class: ScriptDevelopment\PhpstanWarroomRules\Rules\ForbidRawExceptionMessageInResponseRule arguments: rawExceptionMessageSinks: %rawExceptionMessageSinks% + safeMessageExceptionClasses: %safeMessageExceptionClasses% tags: [phpstan.rules.rule] - class: ScriptDevelopment\PhpstanWarroomRules\Type\ConnectionTransactionReturnTypeExtension diff --git a/src/Rules/ForbidRawExceptionMessageInResponseRule.php b/src/Rules/ForbidRawExceptionMessageInResponseRule.php index b77d0e4..b6ff84f 100644 --- a/src/Rules/ForbidRawExceptionMessageInResponseRule.php +++ b/src/Rules/ForbidRawExceptionMessageInResponseRule.php @@ -11,6 +11,7 @@ use PhpParser\Node\Expr\CallLike; use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\MethodCall; +use PhpParser\Node\Expr\NullsafeMethodCall; use PhpParser\Node\Expr\StaticCall; use PhpParser\Node\Identifier; use PhpParser\Node\Name; @@ -18,8 +19,10 @@ use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; +use PHPStan\Type\NeverType; use PHPStan\Type\ObjectType; use PHPStan\Type\Type; +use PHPStan\Type\TypeCombinator; use Psr\Log\LoggerInterface; use Throwable; @@ -83,12 +86,25 @@ * sink matching, so a consumer that adds a broad sink can never turn a * logger into a false positive. * - * Exemption: a `// @leak-safe: ` comment on the sink call line (or - * in the contiguous comment block directly above it) suppresses the rule for a - * proven-safe case — an app-authored exception message carrying no raw payload - * (the codebook `SendCodyReportAction` shape). The standard PHPStan inline- - * ignore mechanism on the identifier - * `forbidRawExceptionMessageInResponse.rawMessageInResponse` is the alternative. + * Exemptions, narrowest first: + * + * - `safeMessageExceptionClasses` (config, the CLASS-level path): exception + * FQCNs whose messages are proven app-authored (the codebook + * `DependentModelRelationException` shape — its message discipline is + * pinned by an arch test in the consuming territory). A `getMessage()` + * whose receiver is a subtype of a listed class never fires, so a + * prove-safe class needs ONE config line, not an annotation at every call + * site. The allowlist covers the MESSAGE only — passing the Throwable + * itself into a sink still fires (`__toString` carries class, file, and + * trace regardless of message discipline). List a class here only when the + * consuming territory pins its message discipline with an arch test; + * config without the pin is a hole, not an exemption. + * - `// @leak-safe: ` comment on the sink call line (or in the + * contiguous comment block directly above it) — the per-call-site path for + * a proven-safe case the class-level list cannot express (the codebook + * `SendCodyReportAction` shape). The standard PHPStan inline-ignore on the + * identifier `forbidRawExceptionMessageInResponse.rawMessageInResponse` is + * the alternative. * * Out of scope (deliberately, for v1): * @@ -99,6 +115,11 @@ * (`Response::error($this->format($e))`) — the type at the sink boundary is * the formatter's return, not a Throwable; a false negative is accepted * (ADR-0021 posture: false negatives acceptable, false positives are not). + * - Plain local-variable extraction (`$msg = $e->getMessage(); + * Response::error($msg);`) — the mundane sibling of the formatter gap: the + * type at the sink is `string`, the provenance is gone. Same accepted- + * false-negative posture; closing it needs data-flow tracking, not a + * wider matcher. * * @implements Rule */ @@ -133,17 +154,26 @@ final class ForbidRawExceptionMessageInResponseRule implements Rule private array $sinks; /** - * @param list $rawExceptionMessageSinks additional client-facing - * sink signatures in - * `FQCN::method` form (e.g. a - * consumer's persist-error - * setter). Merged with the - * built-in `Response::error` - * default; empty by default so - * the rule is safe to adopt. + * @param list $rawExceptionMessageSinks additional client-facing + * sink signatures in + * `FQCN::method` form (e.g. a + * consumer's persist-error + * setter). Merged with the + * built-in `Response::error` + * default; empty by default so + * the rule is safe to adopt. + * @param list $safeMessageExceptionClasses exception FQCNs whose + * messages are proven + * app-authored (arch-test- + * pinned in the consuming + * territory) — their + * `getMessage()` is exempt; + * the Throwable itself never + * is. Empty by default. */ public function __construct( array $rawExceptionMessageSinks = [], + private readonly array $safeMessageExceptionClasses = [], ) { $this->sinks = []; @@ -263,14 +293,55 @@ private function exprCarriesRawExceptionMessage(Expr $expr, Scope $scope): bool private function isThrowableGetMessageCall(Expr $expr, Scope $scope): bool { - return $expr instanceof MethodCall - && $expr->name instanceof Identifier - && $expr->name->toString() === 'getMessage' - && $this->typeIsThrowable($scope->getType($expr->var)); + // NullsafeMethodCall is a distinct node — `$e?->getMessage()` leaks + // exactly as its unconditional sibling does when `$e` is non-null. + if (!$expr instanceof MethodCall && !$expr instanceof NullsafeMethodCall) { + return false; + } + + if (!$expr->name instanceof Identifier || $expr->name->toString() !== 'getMessage') { + return false; + } + + $receiverType = $scope->getType($expr->var); + + return $this->typeIsThrowable($receiverType) + && !$this->isSafeMessageException($receiverType); + } + + /** + * True when the receiver's type is a subtype of a configured + * safe-message exception class — its `getMessage()` is proven + * app-authored, so the message (and ONLY the message) is exempt. + */ + private function isSafeMessageException(Type $type): bool + { + $type = TypeCombinator::removeNull($type); + + if ($type instanceof NeverType) { + return false; + } + + foreach ($this->safeMessageExceptionClasses as $class) { + if ((new ObjectType($class))->isSuperTypeOf($type)->yes()) { + return true; + } + } + + return false; } private function typeIsThrowable(Type $type): bool { + // Strip null so a nullsafe receiver (`$e?->getMessage()` — the receiver + // types as `Throwable|null` inside the NullsafeMethodCall) still + // resolves; a pure-null type (NeverType after the strip) never is one. + $type = TypeCombinator::removeNull($type); + + if ($type instanceof NeverType) { + return false; + } + return (new ObjectType(self::THROWABLE))->isSuperTypeOf($type)->yes(); } @@ -382,7 +453,8 @@ private function buildError(MethodCall|StaticCall $node): IdentifierRuleError . 'Passing Throwable::getMessage() (or the Throwable itself) to a response leaks internal detail ' . '(stack-trace fragments, SQL, file paths) to the API client. Log the raw message server-side ' . '(Log::/report()) and return a stable, app-authored message. ' - . 'Suppress a proven-safe app-authored message with a `// @leak-safe: ` comment on the sink line.', + . 'Suppress a proven-safe app-authored message with a `// @leak-safe: ` comment on the sink line, ' + . 'or list an arch-test-pinned exception class in `safeMessageExceptionClasses`.', ) ->identifier('forbidRawExceptionMessageInResponse.rawMessageInResponse') ->line($node->getStartLine()) diff --git a/tests/Fixtures/RawExceptionMessageInResponse/NullsafeGetMessage.php b/tests/Fixtures/RawExceptionMessageInResponse/NullsafeGetMessage.php new file mode 100644 index 0000000..96de279 --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/NullsafeGetMessage.php @@ -0,0 +1,19 @@ +` is a distinct + // AST node (NullsafeMethodCall) but leaks identically when non-null. + // Fires. + return Response::error('Invalid input: ' . $e?->getMessage()); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionPassthrough.php b/tests/Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionPassthrough.php new file mode 100644 index 0000000..7a33f50 --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionPassthrough.php @@ -0,0 +1,20 @@ +getMessage()); + } +} diff --git a/tests/Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionThrowableItself.php b/tests/Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionThrowableItself.php new file mode 100644 index 0000000..0cc3c3b --- /dev/null +++ b/tests/Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionThrowableItself.php @@ -0,0 +1,19 @@ +` comment on the sink line.'; + . 'Suppress a proven-safe app-authored message with a `// @leak-safe: ` comment on the sink line, ' + . 'or list an arch-test-pinned exception class in `safeMessageExceptionClasses`.'; private const string STUBS = __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/_stubs.php'; @@ -173,6 +174,58 @@ public function testIgnoresSameLineLeakSafeMarker(): void ); } + public function testFlagsNullsafeGetMessageIntoResponseError(): void + { + // `$e?->getMessage()` is a NullsafeMethodCall — a distinct AST node the + // MethodCall-only matcher missed (the #59 review's Nit). Same leak. + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/NullsafeGetMessage.php'], + [[self::MESSAGE, 17]], + ); + } + + public function testFlagsSafeMessageExceptionUnderDefaultConfig(): void + { + // No allowlist configured — a prove-safe class is still an exception + // like any other. Pins "empty param = no class-level exemption". + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionPassthrough.php'], + [[self::MESSAGE, 18]], + ); + } + + public function testIgnoresConfiguredSafeMessageExceptionGetMessage(): void + { + // The #59 review's Major: the rule's own motivating example (codebook + // DeleteChapterTool passing DependentModelRelationException::getMessage(), + // arch-test-pinned app-authored) needed a config-level exemption, not a + // per-call-site annotation. Type-aware: subtypes inherit the allowance. + $this->ruleOverride = new ForbidRawExceptionMessageInResponseRule( + [], + ['App\Exceptions\DependentModelRelationException'], + ); + + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionPassthrough.php'], + [], + ); + } + + public function testSafeMessageExceptionDoesNotExemptTheThrowableItself(): void + { + // The allowlist covers the MESSAGE only — the Throwable stringifies + // with class, file, and trace regardless of message discipline. + $this->ruleOverride = new ForbidRawExceptionMessageInResponseRule( + [], + ['App\Exceptions\DependentModelRelationException'], + ); + + $this->analyse( + [self::STUBS, __DIR__ . '/../Fixtures/RawExceptionMessageInResponse/SafeMessageExceptionThrowableItself.php'], + [[self::MESSAGE, 17]], + ); + } + public function testMalformedSinkSignaturesAreSkippedNotFatal(): void { // Garbage sink signatures (no `::`, too many `::`, empty class, empty