diff --git a/CHANGELOG.md b/CHANGELOG.md index 77f15c1..6d80654 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and ### Added +- `ForbidUntimedHttpClientRule` — new rule enforcing war-room **Architectural Principle #8** (explicit timeouts on outbound HTTP) at analysis time, the AST-aware successor to the per-territory `ExternalHttpTimeoutTest` named-list Pest tests (kendo / emmie). The named-list tests detect wrong-shape on *enrolled* classes but are blind to **omission** — a new untimed call nobody adds to the list; this rule closes the omission gap for the tractable call shapes. Registered in `extension.neon` (no parameters). Identifier: `forbidUntimedHttpClient.missingTimeout`. Doctrine: war-room §Architectural Principles #8. Seed: war-room enforcement queue #58 (spike branch `spike/wr-queue58-untimed-http-client`). **Review follow-up (bus #57 findings):** the `withOptions()` check is TYPE-aware, not AST-literal — a constant array type provably lacking `'timeout'` still fires (including through a variable holding a literal array, a widening over the inline-`Array_`-only first cut), while a computed/helper-built options expression (not a constant array type) is POSSIBLY timed and the chain DECLINES (the Major: flagging it was a false positive). A chain member outside the known `PendingRequest` builder surface — a Macroable extension (`Http::github()`, root or intermediate) or `when()`/`unless()` with their opaque closures — likewise declines (the Minor: a macro may return a pre-timed request); a genuine builder missing from the list costs only a false negative. + + **Detection (type-anchored, two entry points):** fires on a terminal send verb (`get`/`post`/`put`/`patch`/`delete`/`head`/`send`) reached without an explicit request timeout, where the entry point is either (1) the `Http` facade (`Illuminate\Support\Facades\Http` static-call root), or (2) an **injected `Illuminate\Http\Client\Factory`** receiver (`$this->http->…->get()`, anchored by TYPE so the property alias is irrelevant — the dominant fleet idiom, established by field survey of kendo/emmie/ublgenie/BIO). A timeout counts when the visible chain contains `->timeout(...)` **or** `->withOptions([... 'timeout' => ...])`; `connectTimeout()` alone does NOT (it bounds the handshake, not the response). + + **CONSERVATIVE BY DESIGN — biased to zero false positives.** Fires only when the ENTIRE chain from an entry point to the send verb is visible in a single expression. It DECLINES (never a false positive) on: split chains where the `PendingRequest` is built on one statement / helper and sent on another (`$req = $this->http->timeout(5); $req->get()`, or `$this->apiClient()->post()` returning a pre-timed request — a large fraction of the real fleet surface); raw `GuzzleHttp\Client` construction / per-call `['timeout' => N]` options; vendor SDKs that configure the timeout via their own `setConfig([...])`; and DI-bound pre-timed clients. **Consequence: this rule COMPLEMENTS the per-territory named-list tests; it does NOT replace them** — the helper-built split (which the named-list's whole-file scan does cover) is out of AST reach for a single-expression rule. The builder-side enforcement that would actually retire the named-lists is a separate follow-up (queue #58 continuation). + + **Field-validated (spike, 2026-07-21):** run against kendo / emmie / ublgenie / BIO backends — **zero false positives** (compliant chains short-circuit on the timeout; split/helper chains decline); a synthetic positive control fires on exactly the untimed method against the *real* Illuminate `Factory`. **Versioning: MINOR** (new rule; zero violations on every swept consumer ⇒ no baseline on kendo/emmie/ublgenie/BIO). A not-yet-swept consumer carrying an untimed *direct* facade/Factory chain would see a new error and adopt on its own bump PR; per the pre-1.0 caret convention `^0.8` excludes the next minor, so tagging auto-adopts nobody. **NOT tagged** (release is ally-gated). + - `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..e3d422f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,6 +35,7 @@ Composer package distributing war-room-doctrine PHPStan rules across `script-dev | `EnforceResourceDataValidatorOptInRule` | ADR-0009 §EAGER_LOAD validator opt-in | `enforceResourceDataValidatorOptIn.missingValidatorCall` | | `EnforceFormRequestToDtoRule` | ADR-0012 §FormRequest → DTO Flow | `enforceFormRequestToDto.missingToDtoMethod` | | `EnforceCurrentUserAttributeRule` | War-room §Explicit over implicit | `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser` | +| `ForbidUntimedHttpClientRule` | War-room §Explicit HTTP timeouts (#8) | `forbidUntimedHttpClient.missingTimeout` (type-aware; flags an `Http` facade OR injected `Illuminate\Http\Client\Factory` chain that reaches a send verb with no explicit `->timeout()` / `withOptions(['timeout'])`. Conservative — fires only on fully-visible single-expression chains; declines split/helper-built chains + Guzzle/SDK surfaces to hold FP at zero. COMPLEMENTS, does not replace, the per-territory `ExternalHttpTimeoutTest`. on `main`, `[Unreleased]`) | | `EnforceAuditModelProtectionsRule` | ADR-0001 §Append-only | `enforceAuditModelProtections.hasFactoryForbidden` / `.softDeletesForbidden` / `.updatedAtNotDisabled` (denylist-inversion; discovers audit models by shape — `auditModelNameSuffixes` default `AuditLog` OR `auditModelNamespacePrefixes` default `App\Models\Audit` — and flags `HasFactory` / `SoftDeletes` / missing `const UPDATED_AT = null`. shipped v0.7.0) | | `EnforceActionResultDtoRule` | ADR-0020 + ADR-0011 | `enforceActionResultDto.arrayReturnFromExecute` (signature-only; flags an `array` / `?array` / `array\|Dto` union / `iterable` native return type on `App\Actions\*` `execute()`. Phpdoc-only `@return array{...}` is a deliberate miss; no `list` carve-out. Seed kendo PR #1653. on `main`, `[Unreleased]` — pending v0.8.0 tag (release PR #53)) | | `ConnectionTransactionReturnTypeExtension` | (type extension, no rule) | — | @@ -113,6 +114,7 @@ SemVer per ADR-0021: ### War-room Architectural Principle rules (no published ADR) - **Explicit over implicit** — package distributes `ForbidAbortHelperRule` (bans `abort()` / `abort_if()` / `abort_unless()`; shipped), `EnforceCurrentUserAttributeRule` (flags `Request::user()` / `Auth::user()` / `auth()->user()` in `App\Http\Controllers`, steering to the `#[CurrentUser]` container attribute per Architectural Principle #9; shipped v0.4.0), `ForbidHttpExceptionInActionsRule` (type-aware sibling of `ForbidAbortHelperRule` — bans throwing the `Symfony\…\HttpException` family from `App\Actions\*`; HTTP status concerns belong to the HTTP layer per Principles #1 + #3; `ValidationException` deliberately out of scope; shipped v0.5.0), `ForbidResourceWrappedInJsonResponseRule` (bans wrapping a `JsonResource` in `response()->json()` / `new JsonResponse()` inside controllers per Principle #1 + ADR-0009; shipped v0.5.0), and `ForbidRawExceptionMessageInResponseRule` (bans a raw `Throwable::getMessage()` — or the `Throwable` itself — reaching a client-facing response sink per Principle #1 + information-disclosure hardening for the ISO 27001 / AVG / NEN 7510 consumers; default sink `Laravel\Mcp\Response::error`, configurable via `rawExceptionMessageSinks`; server-side logging never flags; `// @leak-safe:` exemption; on `main`, `[Unreleased]`). These enforce war-room §Architectural Principles (some also touching numbered ADRs) — each rule's docblock "Doctrine source" line names its authority. +- **Explicit HTTP timeouts (#8)** — package distributes `ForbidUntimedHttpClientRule` (flags an `Http` facade / injected `Illuminate\Http\Client\Factory` chain reaching a send verb without an explicit request timeout, per Architectural Principle #8; the AST-aware, omission-closing successor to the per-territory `ExternalHttpTimeoutTest` named-list Pest tests — conservative single-expression detection, declines split/helper-built chains + Guzzle/SDK surfaces; COMPLEMENTS the named-lists rather than replacing them; on `main`, `[Unreleased]`). Seed: war-room enforcement queue #58. ### War-room internal ADRs diff --git a/extension.neon b/extension.neon index a9137f1..9f94230 100644 --- a/extension.neon +++ b/extension.neon @@ -142,6 +142,9 @@ services: auditModelNamespacePrefixes: %auditModelNamespacePrefixes% auditModelNameSuffixes: %auditModelNameSuffixes% tags: [phpstan.rules.rule] + - + class: ScriptDevelopment\PhpstanWarroomRules\Rules\ForbidUntimedHttpClientRule + tags: [phpstan.rules.rule] - class: ScriptDevelopment\PhpstanWarroomRules\Rules\ForbidRawExceptionMessageInResponseRule arguments: diff --git a/src/Rules/ForbidUntimedHttpClientRule.php b/src/Rules/ForbidUntimedHttpClientRule.php new file mode 100644 index 0000000..81932e0 --- /dev/null +++ b/src/Rules/ForbidUntimedHttpClientRule.php @@ -0,0 +1,323 @@ +get($url)`. + * 2. An injected client factory (`Illuminate\Http\Client\Factory`) — the + * dominant fleet idiom, e.g. `$this->http->withToken($t)->get($url)` + * where `$this->http` is a promoted `Factory` property. Anchored by TYPE, + * so the alias (`$http` / `$httpClient` / `$client`) is irrelevant. + * + * A timeout is considered present when the visible chain contains a + * `->timeout(...)` call OR a `->withOptions(...)` whose options provably carry + * a `'timeout'` key. The options check is TYPE-aware, not AST-literal: a + * variable holding a literal array resolves to a constant array type and is + * seen through; an options expression whose type is NOT a constant array (a + * computed array, a helper/config() return) is treated as POSSIBLY timed and + * the chain DECLINES — absence is unprovable, and a false positive is the one + * unacceptable outcome. `connectTimeout()` alone is NOT sufficient — it bounds + * the handshake, not the response, so a hung server still stalls the caller; + * Doctrine #8 wants the request timeout. + * + * Doctrine source: war-room §Architectural Principles #8 — Explicit timeouts + * on external HTTP calls. Promotion candidate for war-room enforcement queue + * #58 (the AST-aware successor to the per-territory `ExternalHttpTimeoutTest` + * named-list Pest tests on kendo / emmie, which detect wrong-shape on enrolled + * classes but are blind to OMISSION — a new untimed call nobody enrolls). + * + * CONSERVATIVE BY DESIGN — fires only when the ENTIRE chain from an entry point + * (facade static call, or a `Factory`-typed receiver) to the send verb is + * visible in a single expression. It deliberately DECLINES (never a false + * positive) on: + * + * - Split chains — the `PendingRequest` is built on one statement / helper + * and sent on another (`$req = $this->http->timeout(5); $req->get(...)`, or + * `$this->apiClient()->post(...)` where `apiClient()` returns a pre-timed + * request). The send-site expression cannot see the builder, so a + * `PendingRequest`-typed (as opposed to `Factory`-typed) root is NOT an + * anchor — the timeout may have been set upstream. + * - Raw `GuzzleHttp\Client` construction / per-call `['timeout' => N]` request + * options (a distinct AST surface — the timeout rides an options array). + * - Vendor SDKs that wrap Guzzle and configure the timeout via their own + * `setConfig([...])` — invisible to any HTTP-client scan; flagging them + * would be a false positive. + * - Timeouts configured at a DI binding (a provider binds a pre-timed client). + * - `->withOptions($computed)` where the options type is not a constant + * array — the key set is unknowable statically (see above). + * - Chain members outside the known `PendingRequest` builder surface — a + * `Macroable` extension (`Http::github()->get(...)`, or an intermediate + * `->github()`) may return a PRE-TIMED request, so an unknown method + * anywhere in the chain (root or intermediate) declines. `when()` / + * `unless()` decline for the same reason: their closures can set the + * timeout invisibly. + * + * These exclusions are the deliberate consequence of biasing a first-cut rule + * toward zero false positives (a false positive reddens a compliant consumer's + * whole `phpstan` run and forces a coordinated baseline; a false negative is + * absorbed by the surviving named-list test). The excluded surfaces remain the + * responsibility of the per-territory named-list Pest test until a follow-up + * widens this rule. + * + * Suppression: standard PHPStan inline-ignore on the identifier + * `forbidUntimedHttpClient.missingTimeout`. + * + * @implements Rule + */ +final class ForbidUntimedHttpClientRule implements Rule +{ + private const string HTTP_FACADE = Http::class; + + /** + * Literal FQCN, not `Factory::class` — `illuminate/http` is not in this + * package's own dev tree (only `illuminate/support` is), so a class-const + * fetch is a `class.notFound` under self-analysis. `ObjectType` takes the + * string happily; in a consumer tree without Laravel the anchor simply + * never matches (correct: nothing to enforce). Same pattern as the + * sibling rule's `DEFAULT_SINK`. + */ + private const string CLIENT_FACTORY = Factory::class; + + /** Terminal HTTP send verbs on the facade / factory / `PendingRequest`. */ + private const array SEND_VERBS = ['get', 'post', 'put', 'patch', 'delete', 'head', 'send']; + + /** + * The known `PendingRequest` fluent-builder surface (lowercased). A chain + * member OUTSIDE this set — a Macroable extension, or `when()`/`unless()` + * whose closures are opaque — may have set the timeout internally, so the + * chain declines rather than risk a false positive. A genuine builder + * missing from this list costs only a false negative (ADR-0021 posture). + */ + private const array KNOWN_BUILDERS = [ + 'accept', 'acceptjson', 'asform', 'asjson', 'asmultipart', 'async', 'attach', + 'baseurl', 'beforesending', 'bodyformat', 'connecttimeout', 'contenttype', + 'dd', 'dump', 'maxredirects', 'replaceheaders', 'retry', 'sink', + 'throw', 'throwif', 'throwunless', 'timeout', + 'withbasicauth', 'withbody', 'withcookies', 'withdigestauth', + 'withheader', 'withheaders', 'withmiddleware', 'withoptions', + 'withqueryparameters', 'withrequestmiddleware', 'withresponsemiddleware', + 'withtoken', 'withurlparameters', 'withuseragent', + 'withoutredirecting', 'withoutverifying', + ]; + + public function getNodeType(): string + { + return CallLike::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if ($node instanceof StaticCall) { + return $this->processStaticSend($node); + } + + if ($node instanceof MethodCall) { + return $this->processChainedSend($node, $scope); + } + + return []; + } + + /** + * A bare static send with no chain at all — `Http::get($url)` — can carry + * no timeout, so it is always a violation when rooted at the Http facade. + * + * @return list + */ + private function processStaticSend(StaticCall $node): array + { + if (!$this->isHttpFacade($node->class) || !$this->isSendVerb($node->name)) { + return []; + } + + return [$this->buildError($node)]; + } + + /** + * A chained send — `->...->get($url)`. Fires only when the receiver + * chain is fully visible back to a recognised entry point (the `Http` + * facade static call, or a `Factory`-typed receiver) AND carries no + * timeout. Any other / unresolved root ⇒ DECLINE (return []). + * + * @return list + */ + private function processChainedSend(MethodCall $node, Scope $scope): array + { + if (!$this->isSendVerb($node->name)) { + return []; + } + + $cursor = $node->var; + + while ($cursor instanceof MethodCall) { + if ($this->declaresTimeout($cursor, $scope)) { + return []; + } + + // An unknown chain member — a Macroable extension (`->github()`) + // or anything outside the known builder surface — may have set the + // timeout internally. DECLINE, never a false positive. + if (!$this->isKnownBuilder($cursor->name)) { + return []; + } + + $cursor = $cursor->var; + } + + if ($cursor instanceof StaticCall) { + // Chain root is a static call. Only the Http facade is an entry we + // can fully account for; the root call may itself set the timeout. + if (!$this->isHttpFacade($cursor->class)) { + return []; + } + + if ($this->declaresTimeout($cursor, $scope)) { + return []; + } + + // Same macro guard at the static root — `Http::github()->get(...)` + // may be a macro returning a pre-timed request. + if (!$this->isKnownBuilder($cursor->name)) { + return []; + } + + return [$this->buildError($node)]; + } + + // Chain root is an expression (property / variable). It anchors ONLY if + // its type is the client Factory — the entry point, where the whole + // chain is guaranteed visible. A `PendingRequest`-typed root may carry + // an upstream timeout we cannot see, so it is NOT an anchor. + if ($this->isClientFactory($scope->getType($cursor))) { + return [$this->buildError($node)]; + } + + return []; + } + + /** + * True when a builder call in the chain establishes (or MAY establish) a + * request timeout: a `->timeout(...)` method, or a `->withOptions(...)` + * whose options are not provably timeout-free. + */ + private function declaresTimeout(MethodCall|StaticCall $call, Scope $scope): bool + { + $name = $this->methodName($call->name); + + if ($name === 'timeout') { + return true; + } + + if ($name === 'withoptions') { + return $this->withOptionsMayCarryTimeout($call, $scope); + } + + return false; + } + + /** + * Tri-state collapse over the `withOptions()` argument, by TYPE: + * + * - constant array type carrying a `'timeout'` key (any union variant) — + * timed, chain is compliant; + * - constant array type(s) all provably WITHOUT the key — untimed, the + * chain stays a candidate; + * - anything else (a computed array, a helper/config() return, unknown) — + * POSSIBLY timed, so the chain declines: absence is unprovable and a + * false positive is the one unacceptable outcome (ADR-0021). + * + * The type path subsumes the old literal-`Array_` AST check (a literal + * resolves to a constant array type) and additionally sees through a + * variable holding a literal array. + */ + private function withOptionsMayCarryTimeout(MethodCall|StaticCall $call, Scope $scope): bool + { + $first = $call->getArgs()[0]->value ?? null; + + if ($first === null) { + // `withOptions()` with no argument adds nothing to the request. + return false; + } + + $constantArrays = $scope->getType($first)->getConstantArrays(); + + if ($constantArrays === []) { + return true; + } + + foreach ($constantArrays as $constantArray) { + foreach ($constantArray->getKeyTypes() as $keyType) { + foreach ($keyType->getConstantStrings() as $keyString) { + if (mb_strtolower($keyString->getValue()) === 'timeout') { + return true; + } + } + } + } + + return false; + } + + private function isKnownBuilder(mixed $name): bool + { + return in_array($this->methodName($name), self::KNOWN_BUILDERS, true); + } + + private function isClientFactory(Type $type): bool + { + return (new ObjectType(self::CLIENT_FACTORY))->isSuperTypeOf($type)->yes(); + } + + private function isHttpFacade(mixed $class): bool + { + return $class instanceof Name && $class->toString() === self::HTTP_FACADE; + } + + private function isSendVerb(mixed $name): bool + { + return in_array($this->methodName($name), self::SEND_VERBS, true); + } + + private function methodName(mixed $name): string + { + return $name instanceof Identifier ? mb_strtolower($name->toString()) : ''; + } + + private function buildError(MethodCall|StaticCall $node): IdentifierRuleError + { + return RuleErrorBuilder::message( + 'Outbound HTTP request declares no explicit timeout. ' + . 'Add ->timeout(seconds) to the chain — external calls must not rely on the framework default (Doctrine Principle #8).', + ) + ->identifier('forbidUntimedHttpClient.missingTimeout') + ->line($node->getStartLine()) + ->build(); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/CompliantFactoryChainTimeout.php b/tests/Fixtures/UntimedHttpClient/CompliantFactoryChainTimeout.php new file mode 100644 index 0000000..65031f0 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/CompliantFactoryChainTimeout.php @@ -0,0 +1,19 @@ +http->withToken($token)->timeout(30)->get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/CompliantTimeoutMidChain.php b/tests/Fixtures/UntimedHttpClient/CompliantTimeoutMidChain.php new file mode 100644 index 0000000..606d0a3 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/CompliantTimeoutMidChain.php @@ -0,0 +1,18 @@ + $data + */ + public function push(string $url, string $token, array $data): void + { + Http::withToken($token)->timeout(5)->post($url, $data); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/CompliantTimeoutStatic.php b/tests/Fixtures/UntimedHttpClient/CompliantTimeoutStatic.php new file mode 100644 index 0000000..3647909 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/CompliantTimeoutStatic.php @@ -0,0 +1,15 @@ +get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/CompliantWithOptionsTimeout.php b/tests/Fixtures/UntimedHttpClient/CompliantWithOptionsTimeout.php new file mode 100644 index 0000000..d44c319 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/CompliantWithOptionsTimeout.php @@ -0,0 +1,15 @@ + 5])->get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/CompliantWithOptionsVariableTimeout.php b/tests/Fixtures/UntimedHttpClient/CompliantWithOptionsVariableTimeout.php new file mode 100644 index 0000000..b6b8605 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/CompliantWithOptionsVariableTimeout.php @@ -0,0 +1,18 @@ + 5]; + + Http::withOptions($options)->get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/DeclinedLocalPendingRequestVar.php b/tests/Fixtures/UntimedHttpClient/DeclinedLocalPendingRequestVar.php new file mode 100644 index 0000000..e109339 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/DeclinedLocalPendingRequestVar.php @@ -0,0 +1,27 @@ +http->withToken($token)->timeout(30); + + $request->get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/DeclinedMacroMidChain.php b/tests/Fixtures/UntimedHttpClient/DeclinedMacroMidChain.php new file mode 100644 index 0000000..71a6248 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/DeclinedMacroMidChain.php @@ -0,0 +1,17 @@ +github()->get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/DeclinedMacroStaticRoot.php b/tests/Fixtures/UntimedHttpClient/DeclinedMacroStaticRoot.php new file mode 100644 index 0000000..ba3307a --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/DeclinedMacroStaticRoot.php @@ -0,0 +1,18 @@ +get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/DeclinedSplitChainProperty.php b/tests/Fixtures/UntimedHttpClient/DeclinedSplitChainProperty.php new file mode 100644 index 0000000..16900d1 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/DeclinedSplitChainProperty.php @@ -0,0 +1,25 @@ +client->get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/DeclinedWithOptionsComputed.php b/tests/Fixtures/UntimedHttpClient/DeclinedWithOptionsComputed.php new file mode 100644 index 0000000..c6c3d67 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/DeclinedWithOptionsComputed.php @@ -0,0 +1,27 @@ +options())->get($url); + } + + /** + * @return array + */ + private function options(): array + { + return ['timeout' => 5, 'verify' => false]; + } +} diff --git a/tests/Fixtures/UntimedHttpClient/IgnoredNonHttpReceiver.php b/tests/Fixtures/UntimedHttpClient/IgnoredNonHttpReceiver.php new file mode 100644 index 0000000..a2a15fa --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/IgnoredNonHttpReceiver.php @@ -0,0 +1,25 @@ +get()` on an unrelated object whose chain root is not the Http facade. + * The rule must not fire — `get` as a method name is not exclusive to HTTP. + */ +final class LocalRepository +{ + public function get(string $id): string + { + return $id; + } +} + +final class IgnoredNonHttpReceiver +{ + public function fetch(LocalRepository $repo): string + { + return $repo->get('x'); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/ViolationBareStaticGet.php b/tests/Fixtures/UntimedHttpClient/ViolationBareStaticGet.php new file mode 100644 index 0000000..d14608f --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/ViolationBareStaticGet.php @@ -0,0 +1,15 @@ +get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/ViolationConnectTimeoutOnly.php b/tests/Fixtures/UntimedHttpClient/ViolationConnectTimeoutOnly.php new file mode 100644 index 0000000..0ef288e --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/ViolationConnectTimeoutOnly.php @@ -0,0 +1,17 @@ +get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/ViolationFactoryChainNoTimeout.php b/tests/Fixtures/UntimedHttpClient/ViolationFactoryChainNoTimeout.php new file mode 100644 index 0000000..097854d --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/ViolationFactoryChainNoTimeout.php @@ -0,0 +1,19 @@ +http->withToken($token)->get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/ViolationWithOptionsVariableNoTimeout.php b/tests/Fixtures/UntimedHttpClient/ViolationWithOptionsVariableNoTimeout.php new file mode 100644 index 0000000..cf7b227 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/ViolationWithOptionsVariableNoTimeout.php @@ -0,0 +1,21 @@ + false]; + + Http::withOptions($options)->get($url); + } +} diff --git a/tests/Fixtures/UntimedHttpClient/_stubs.php b/tests/Fixtures/UntimedHttpClient/_stubs.php new file mode 100644 index 0000000..b814c23 --- /dev/null +++ b/tests/Fixtures/UntimedHttpClient/_stubs.php @@ -0,0 +1,229 @@ + $options + */ + public function withOptions(array $options): PendingRequest + { + return new PendingRequest; + } + + public function baseUrl(string $url): PendingRequest + { + return new PendingRequest; + } + + /** + * @param array $query + */ + public function get(string $url, array $query = []): Response + { + return new Response; + } + + /** + * @param array $data + */ + public function post(string $url, array $data = []): Response + { + return new Response; + } + } + + class PendingRequest + { + public function timeout(int $seconds): static + { + return $this; + } + + public function connectTimeout(int $seconds): static + { + return $this; + } + + public function withToken(string $token): static + { + return $this; + } + + /** + * @param array $options + */ + public function withOptions(array $options): static + { + return $this; + } + + public function baseUrl(string $url): static + { + return $this; + } + + /** + * @param array $query + */ + public function get(string $url, array $query = []): Response + { + return new Response; + } + + /** + * @param array $data + */ + public function post(string $url, array $data = []): Response + { + return new Response; + } + + /** + * @param array $data + */ + public function put(string $url, array $data = []): Response + { + return new Response; + } + + /** + * @param array $data + */ + public function patch(string $url, array $data = []): Response + { + return new Response; + } + + /** + * @param array $data + */ + public function delete(string $url, array $data = []): Response + { + return new Response; + } + + public function head(string $url): Response + { + return new Response; + } + + /** + * @param array $options + */ + public function send(string $method, string $url, array $options = []): Response + { + return new Response; + } + } +} + +namespace Illuminate\Support\Facades { + use Illuminate\Http\Client\PendingRequest; + use Illuminate\Http\Client\Response; + + class Http + { + public static function timeout(int $seconds): PendingRequest + { + return new PendingRequest; + } + + public static function connectTimeout(int $seconds): PendingRequest + { + return new PendingRequest; + } + + public static function withToken(string $token): PendingRequest + { + return new PendingRequest; + } + + /** + * @param array $options + */ + public static function withOptions(array $options): PendingRequest + { + return new PendingRequest; + } + + public static function baseUrl(string $url): PendingRequest + { + return new PendingRequest; + } + + /** + * @param array $query + */ + public static function get(string $url, array $query = []): Response + { + return new Response; + } + + /** + * @param array $data + */ + public static function post(string $url, array $data = []): Response + { + return new Response; + } + + /** + * @param array $data + */ + public static function put(string $url, array $data = []): Response + { + return new Response; + } + + /** + * @param array $data + */ + public static function patch(string $url, array $data = []): Response + { + return new Response; + } + + /** + * @param array $data + */ + public static function delete(string $url, array $data = []): Response + { + return new Response; + } + + public static function head(string $url): Response + { + return new Response; + } + } +} diff --git a/tests/Rules/ForbidUntimedHttpClientRuleTest.php b/tests/Rules/ForbidUntimedHttpClientRuleTest.php new file mode 100644 index 0000000..cf50c05 --- /dev/null +++ b/tests/Rules/ForbidUntimedHttpClientRuleTest.php @@ -0,0 +1,172 @@ + + */ +final class ForbidUntimedHttpClientRuleTest extends RuleTestCase +{ + private const string MESSAGE = 'Outbound HTTP request declares no explicit timeout. ' + . 'Add ->timeout(seconds) to the chain — external calls must not rely on the framework default (Doctrine Principle #8).'; + + public function testFlagsBareStaticSend(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/ViolationBareStaticGet.php'], + [ + [self::MESSAGE, 13], + ], + ); + } + + public function testFlagsChainWithoutTimeout(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/ViolationChainNoTimeout.php'], + [ + [self::MESSAGE, 13], + ], + ); + } + + public function testFlagsConnectTimeoutOnly(): void + { + // connectTimeout() is not a request timeout — the rule must still fire. + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/ViolationConnectTimeoutOnly.php'], + [ + [self::MESSAGE, 15], + ], + ); + } + + public function testFlagsInjectedFactoryChainWithoutTimeout(): void + { + // The dominant fleet idiom: an injected Illuminate\Http\Client\Factory. + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/ViolationFactoryChainNoTimeout.php'], + [ + [self::MESSAGE, 17], + ], + ); + } + + public function testIgnoresInjectedFactoryChainWithTimeout(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/CompliantFactoryChainTimeout.php'], + [], + ); + } + + public function testDeclinesLocalPendingRequestVariable(): void + { + // Timeout set upstream on a PendingRequest-typed local — out of view. + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/DeclinedLocalPendingRequestVar.php'], + [], + ); + } + + public function testIgnoresStaticTimeoutChain(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/CompliantTimeoutStatic.php'], + [], + ); + } + + public function testIgnoresTimeoutMidChain(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/CompliantTimeoutMidChain.php'], + [], + ); + } + + public function testIgnoresWithOptionsTimeoutKey(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/CompliantWithOptionsTimeout.php'], + [], + ); + } + + public function testDeclinesSplitChainBuiltOnAProperty(): void + { + // Builder assembled out of view — deliberate conservative miss. + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/DeclinedSplitChainProperty.php'], + [], + ); + } + + public function testIgnoresNonHttpReceiver(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/IgnoredNonHttpReceiver.php'], + [], + ); + } + + public function testDeclinesComputedWithOptions(): void + { + // The #57 review's Major: a computed options array (helper return) is + // not a constant array type — absence of 'timeout' is unprovable, so + // the chain DECLINES instead of false-positive flagging. + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/DeclinedWithOptionsComputed.php'], + [], + ); + } + + public function testFlagsWithOptionsVariableProvablyWithoutTimeout(): void + { + // The type path sees through a variable holding a literal array — a + // provably timeout-free options set still fires (a widening over the + // old inline-Array_-only check). + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/ViolationWithOptionsVariableNoTimeout.php'], + [[self::MESSAGE, 19]], + ); + } + + public function testIgnoresWithOptionsVariableWithTimeout(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/CompliantWithOptionsVariableTimeout.php'], + [], + ); + } + + public function testDeclinesMacroStaticRoot(): void + { + // The #57 review's Minor: `Http::github()` — a Macroable entry outside + // the known builder surface — may return a pre-timed request. Declines. + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/DeclinedMacroStaticRoot.php'], + [], + ); + } + + public function testDeclinesMacroMidChain(): void + { + // Same guard for an intermediate member — PendingRequest is Macroable. + $this->analyse( + [__DIR__ . '/../Fixtures/UntimedHttpClient/DeclinedMacroMidChain.php'], + [], + ); + } + + protected function getRule(): Rule + { + return new ForbidUntimedHttpClientRule; + } +}