From bbfd7c301484017d8a1249adc342dd47d411cbb2 Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Fri, 28 Aug 2026 13:35:33 -0400 Subject: [PATCH 1/5] feat: response decoration api with cookie support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a decoration API to Response (withHeader, withHeaders, withStatus, withCookie) that preserves the concrete subclass, plus first-class cookie support, and make request-scoped singleton state explicitly clearable. Fixes a live bug: five middleware added headers by constructing a new base Response from the previous response's getters, silently downgrading a StreamingResponse to a plain Response — the SSE stream was discarded and the client received an empty 200. All six rebuild sites now decorate instead, and an architecture test fails the build if the pattern reappears. Response and StreamingResponse drop the readonly class modifier: verified on PHP 8.5.1 that `clone with` does not exist and readonly properties cannot be modified on a clone by assignment or reflection, so clone-then-assign is the only mechanism that preserves the subclass. Immutability is enforced by API design instead — private properties, no setters. The session cookie now travels on the Response rather than the SAPI, and attaches only when the session ID changed, matching what session_start() did before. Attaching unconditionally would have silently disabled the page cache, since cookie-bearing responses are never cached — a security boundary that now also covers Cookie objects, not just literal set-cookie headers. Adds ResettableInterface in core for services holding request-scoped state, implemented by Session, SessionGuard and ReadWriteConnection, plus Container::resolvedInstances() to discover them without forcing instantiation. Three verified cross-user leaks are closed: Session reusing the previous request's ID, SessionGuard caching the authenticated user, and session save handlers accumulating a shutdown function per request. Closes #150 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NLREGwAgqDnHQANKShZ7qL --- .claude/architecture.md | 17 + .../response-decoration/.hook-fingerprint | 1 + .../001-cookie-value-object.md | 48 +++ .../002-response-decoration-api.md | 72 ++++ .../003-header-line-emission.md | 38 ++ .../004-migrate-middleware.md | 61 ++++ .../005-page-cache-cookie-policy.md | 45 +++ .../006-session-cookie-on-response.md | 101 +++++ .../006a-request-cookie-access.md | 46 +++ .../007-architecture-test.md | 134 +++++++ .../008-resettable-interface.md | 35 ++ .../009-request-scoped-session-and-auth.md | 79 ++++ .../010-readwrite-resettable.md | 35 ++ .../011-container-resolved-instances.md | 36 ++ .../response-decoration/_devils_advocate.md | 326 +++++++++++++++++ .claude/plans/response-decoration/_plan.md | 123 +++++++ composer.json | 6 +- .../authentication/src/Guard/SessionGuard.php | 15 +- .../tests/Unit/Guard/SessionGuardTest.php | 79 ++++ packages/core/src/Container/Container.php | 19 + .../src/Contracts/ResettableInterface.php | 30 ++ .../tests/Unit/Container/ContainerTest.php | 70 +++- .../Contracts/ResettableInterfaceTest.php | 52 +++ .../cors/src/Middleware/CorsMiddleware.php | 31 +- packages/cors/tests/CorsMiddlewareTest.php | 136 ++----- packages/cors/tests/Helpers.php | 69 ++++ .../src/Connection/ReadWriteConnection.php | 10 +- .../Connection/ReadWriteConnectionTest.php | 57 ++- .../docs/packages/authentication.md | 12 + packages/docs-markdown/docs/packages/core.md | 54 +++ .../docs/packages/database-readwrite.md | 5 +- .../docs-markdown/docs/packages/page-cache.md | 4 +- .../docs-markdown/docs/packages/routing.md | 94 ++++- .../docs-markdown/docs/packages/session.md | 19 + packages/docs-markdown/docs/packages/sse.md | 2 + .../src/Middleware/InertiaMiddleware.php | 12 +- packages/inertia/tests/Helpers.php | 34 ++ .../Middleware/InertiaMiddlewareTest.php | 79 +++- .../Unit/Driver/FilePageCacheDriverTest.php | 38 ++ .../page-cache/src/CacheabilityChecker.php | 15 +- .../tests/Unit/CacheabilityCheckerTest.php | 22 ++ .../src/Middleware/RateLimitMiddleware.php | 12 +- packages/ratelimiter/tests/Helpers.php | 34 ++ .../tests/Unit/RateLimitMiddlewareTest.php | 40 +- .../src/Exceptions/CookieException.php | 30 ++ packages/routing/src/Http/Cookie.php | 80 ++++ packages/routing/src/Http/Request.php | 21 ++ packages/routing/src/Http/Response.php | 107 +++++- packages/routing/tests/Http/CookieTest.php | 81 ++++ packages/routing/tests/Http/RequestTest.php | 81 +++- packages/routing/tests/Http/ResponseTest.php | 162 +++++++- .../src/Middleware/CorsMiddleware.php | 20 +- .../Middleware/SecurityHeadersMiddleware.php | 24 +- packages/security/tests/Helpers.php | 123 +++++++ .../tests/Unit/CorsMiddlewareTest.php | 72 ++-- .../Unit/SecurityHeadersMiddlewareTest.php | 110 ++++-- packages/session-file/tests/ModuleTest.php | 7 +- packages/session/composer.json | 3 +- .../src/Middleware/SessionMiddleware.php | 96 ++++- packages/session/src/Session.php | 60 +-- .../session/tests/PackageStructureTest.php | 40 +- .../Unit/Middleware/SessionMiddlewareTest.php | 289 ++++++++++++++- .../tests/Unit/SessionShutdownHandlerTest.php | 108 ++++++ packages/session/tests/Unit/SessionTest.php | 109 +++++- packages/sse/src/StreamingResponse.php | 6 +- packages/sse/tests/StreamingResponseTest.php | 100 +++++ .../FreshResponseBeforeNextMiddleware.php | 30 ++ .../HelperMethodResponseMiddleware.php | 39 ++ .../RebuildAfterNextMiddleware.php | 33 ++ .../RebuildFromLocalVariableMiddleware.php | 36 ++ .../PageCacheSessionMiddlewareTest.php | 147 ++++++++ tests/MiddlewareDecorationTest.php | 84 +++++ .../MiddlewareDecorationDetector.php | 345 ++++++++++++++++++ .../MiddlewareDiscovery.php | 144 ++++++++ 74 files changed, 4460 insertions(+), 374 deletions(-) create mode 100644 .claude/plans/response-decoration/.hook-fingerprint create mode 100644 .claude/plans/response-decoration/001-cookie-value-object.md create mode 100644 .claude/plans/response-decoration/002-response-decoration-api.md create mode 100644 .claude/plans/response-decoration/003-header-line-emission.md create mode 100644 .claude/plans/response-decoration/004-migrate-middleware.md create mode 100644 .claude/plans/response-decoration/005-page-cache-cookie-policy.md create mode 100644 .claude/plans/response-decoration/006-session-cookie-on-response.md create mode 100644 .claude/plans/response-decoration/006a-request-cookie-access.md create mode 100644 .claude/plans/response-decoration/007-architecture-test.md create mode 100644 .claude/plans/response-decoration/008-resettable-interface.md create mode 100644 .claude/plans/response-decoration/009-request-scoped-session-and-auth.md create mode 100644 .claude/plans/response-decoration/010-readwrite-resettable.md create mode 100644 .claude/plans/response-decoration/011-container-resolved-instances.md create mode 100644 .claude/plans/response-decoration/_devils_advocate.md create mode 100644 .claude/plans/response-decoration/_plan.md create mode 100644 packages/core/src/Contracts/ResettableInterface.php create mode 100644 packages/core/tests/Unit/Contracts/ResettableInterfaceTest.php create mode 100644 packages/cors/tests/Helpers.php create mode 100644 packages/inertia/tests/Helpers.php create mode 100644 packages/ratelimiter/tests/Helpers.php create mode 100644 packages/routing/src/Exceptions/CookieException.php create mode 100644 packages/routing/src/Http/Cookie.php create mode 100644 packages/routing/tests/Http/CookieTest.php create mode 100644 packages/security/tests/Helpers.php create mode 100644 packages/session/tests/Unit/SessionShutdownHandlerTest.php create mode 100644 tests/Fixtures/MiddlewareDecoration/FreshResponseBeforeNextMiddleware.php create mode 100644 tests/Fixtures/MiddlewareDecoration/HelperMethodResponseMiddleware.php create mode 100644 tests/Fixtures/MiddlewareDecoration/RebuildAfterNextMiddleware.php create mode 100644 tests/Fixtures/MiddlewareDecoration/RebuildFromLocalVariableMiddleware.php create mode 100644 tests/Integration/PageCacheSessionMiddlewareTest.php create mode 100644 tests/MiddlewareDecorationTest.php create mode 100644 tests/Support/MiddlewareDecoration/MiddlewareDecorationDetector.php create mode 100644 tests/Support/MiddlewareDecoration/MiddlewareDiscovery.php diff --git a/.claude/architecture.md b/.claude/architecture.md index 4e56796e..914dc35a 100644 --- a/.claude/architecture.md +++ b/.claude/architecture.md @@ -680,6 +680,23 @@ Why not use key-value for `GuardInterface`? Because key-value (`GuardInterface:: **When singletons matter:** Any service that holds state across the request must be a singleton. For example, if a boot callback registers policies into a `PolicyRegistry`, that registry must be a singleton — otherwise boot writes to one instance and request handlers get a different, empty one. +### Resettable Singletons and Long-Running Processes + +A singleton that caches per-request state (the current session, the authenticated user, a sticky read/write routing flag) is harmless under PHP-FPM, where the process ends after each request. Under a long-running process — a worker or event loop reusing one PHP process across many requests — the same state leaks across requests unless something clears it, which is a cross-user data leak (one user's session or identity bleeding into another user's request). + +Any singleton with this shape should implement `Marko\Core\Contracts\ResettableInterface`: + +```php +interface ResettableInterface +{ + public function reset(): void; +} +``` + +`reset()` must be non-destructive — it clears the instance's in-memory per-request tracking without destroying anything persisted (e.g. resetting a session service forgets which session it was serving, it does not delete the stored session). A long-running process discovers what to reset via `Container::resolvedInstances(ResettableInterface::class)`, which returns only instances the container has already built — never triggering resolution — instead of requiring a hardcoded list. `resolvedInstances()` lives on the concrete `Container` class, not on `ContainerInterface`. + +Current implementors: `Session`, `SessionGuard` (`marko/authentication`), and `ReadWriteConnection` (`marko/database-readwrite`). + ### Preferences Preferences replace one concrete class with another globally. Unlike bindings (interface → implementation), preferences swap class → class. diff --git a/.claude/plans/response-decoration/.hook-fingerprint b/.claude/plans/response-decoration/.hook-fingerprint new file mode 100644 index 00000000..808acc56 --- /dev/null +++ b/.claude/plans/response-decoration/.hook-fingerprint @@ -0,0 +1 @@ +discover-hooks-fingerprint-v1 2864d776f7830845948617a7a7473e0749f16de8b38d6d84f3a5ab64ab406895 diff --git a/.claude/plans/response-decoration/001-cookie-value-object.md b/.claude/plans/response-decoration/001-cookie-value-object.md new file mode 100644 index 00000000..c1918dff --- /dev/null +++ b/.claude/plans/response-decoration/001-cookie-value-object.md @@ -0,0 +1,48 @@ +# Task 001: Cookie Value Object + +**Status**: completed +**Depends on**: none +**Retry count**: 0 + +## Description +Create `Marko\Routing\Http\Cookie`, an immutable value object describing a single HTTP cookie and rendering itself as a `Set-Cookie` header value. This is the foundation for first-class cookie support on `Response`; nothing else in the plan can proceed without it. + +## Context +- New file: `packages/routing/src/Http/Cookie.php` +- New test: `packages/routing/tests/Http/CookieTest.php` +- Attributes needed by `Session`: name, value, expires, path, domain, secure, httpOnly, sameSite +- Follow the existing style in `packages/routing/src/Http/` — constructor property promotion, `declare(strict_types=1)`, full type declarations, no final classes +- This class MAY be `readonly` — the readonly restriction only blocks `Response`, which needs `clone`-based decoration. `Cookie` is constructed whole and never decorated. +- Loud errors: a malformed cookie name must throw, not silently emit a broken header. Add a `Cookie`-specific exception in `packages/routing/src/Exceptions/` following the existing exception style. + +**Three encoding/semantics decisions that must be made in this class, not left to callers:** + +1. **Value encoding.** A `Set-Cookie` value cannot contain `;`, `,`, whitespace or control characters. Session IDs happen to be safe but arbitrary application cookie values are not. Decide raw vs. `urlencode` here and cover it with a test — a caller passing `a; b` must not be able to inject a second cookie attribute. +2. **No-expiry (browser-session) cookies.** `SessionConfig::expireOnClose()` maps to `lifetime => 0`, which means a cookie with **no** `Expires` and **no** `Max-Age` attribute at all — not `Expires: `. The `expires` attribute needs an explicit representation for "omit entirely" (`null` or `0`). +3. **`SameSite=None` requires `Secure`.** Browsers silently drop a `SameSite=None` cookie that is not `Secure`. Per the loud-errors principle this must throw at construction rather than emit a cookie that vanishes. + +## Requirements (Test Descriptions) +- [x] `it renders name and value as a set-cookie string` +- [x] `it renders the path attribute when provided` +- [x] `it renders the domain attribute when provided` +- [x] `it renders expires as an rfc 7231 formatted date` +- [x] `it omits the expires attribute entirely for a browser session cookie` +- [x] `it renders secure and httponly flags only when enabled` +- [x] `it renders the samesite attribute when provided` +- [x] `it encodes a value containing characters that are illegal in a set-cookie header` +- [x] `it throws when the cookie name contains an invalid character` +- [x] `it throws when samesite is none without the secure flag` + +## Acceptance Criteria +- All requirements have passing tests +- Code follows code standards +- No decrease in test coverage + +## Implementation Notes +- Value encoding: `rawurlencode()` applied to the cookie value in `toSetCookieString()`. This strips `;`, `,`, whitespace, and control characters from the rendered header, preventing attribute injection from arbitrary application values while leaving safe values (e.g. session IDs) unchanged. +- No-expiry cookies: `expires` is `?int` (unix timestamp), defaulting to `null`. Both `null` and `0` are treated as "omit the `Expires` attribute entirely" — `0` is included so a raw `SessionConfig::lifetime()` value (which is `0` for expire-on-close) can be passed straight through by a future caller without conversion. +- `SameSite=None` without `secure: true` throws `CookieException::sameSiteNoneRequiresSecure()` at construction (loud errors, not a silently dropped cookie). +- Cookie name validation rejects control characters, whitespace, and RFC 2616 token separators (`()<>@,;:\"/[]?={}`) via `CookieException::invalidName()`; empty names are also rejected. +- Used a literal `gmdate()` format string (`'D, d M Y H:i:s \G\M\T'`) instead of the `DATE_RFC7231` constant, which is deprecated as of PHP 8.5. +- New exception `Marko\Routing\Exceptions\CookieException` added following the existing `MarkoException` static-factory style used by `RouteException`/`RouteConflictException`. +- Full suite (`composer test` equivalent: `pest -c phpunit.xml --parallel --exclude-group=integration-destructive`) passes: 6908 passed, 0 failed. `phpcs`, `php-cs-fixer`, and `phpstan` all clean on touched files. diff --git a/.claude/plans/response-decoration/002-response-decoration-api.md b/.claude/plans/response-decoration/002-response-decoration-api.md new file mode 100644 index 00000000..ee75131b --- /dev/null +++ b/.claude/plans/response-decoration/002-response-decoration-api.md @@ -0,0 +1,72 @@ +# Task 002: Response Decoration API + +**Status**: completed +**Depends on**: 001 +**Retry count**: 0 + +## Description +Give `Response` a decoration API (`withHeader()`, `withHeaders()`, `withStatus()`, `withCookie()`) that returns modified copies while preserving the concrete subclass, and add a cookie collection alongside the existing headers. This is the change that fixes the `StreamingResponse` downgrade at its root. + +## Context +- Modify: `packages/routing/src/Http/Response.php` +- Modify: `packages/sse/src/StreamingResponse.php` (must also drop the `readonly` class modifier so `clone` works through the hierarchy) +- Tests: `packages/routing/tests/Http/ResponseTest.php` + +**Verified on PHP 8.5.1 — read this before implementing.** `clone $this with { ... }` does not exist in 8.5.1 (parse error). Modifying a readonly property on a clone fails both by direct assignment and via `ReflectionProperty::setValue`. The only approach that works is: **plain class (no `readonly` class modifier), private non-readonly properties, `clone $this` then assign on the clone.** + +- MUST use `clone $this`, never `new static(...)`. `StreamingResponse::__construct(SseStream $stream, int $statusCode)` has a different signature from its parent, so `new static(...)` cannot reconstruct it. `clone` copies all subclass state automatically — this is the entire mechanism behind the SSE fix. +- Return type on `with*()` methods must be `static`, not `self`. +- Immutability is now enforced by API design rather than the `readonly` keyword: properties stay private and no setters are added. Every `with*()` needs a test asserting the original instance is unchanged. +- Cookies are a SEPARATE collection (`list`), NOT multi-valued headers. `headers()` must keep its `array` return type so every existing `->headers()` call site stays source-compatible (eight in production code, ~80 across the test suites). +- The 3-arg constructor and `json()` / `html()` / `redirect()` must remain source-compatible. + +### Required public surface (other tasks build against this — do not rename) + +```php +public function withHeader(string $name, string $value): static; +public function withHeaders(array $headers): static; // array, merged over existing +public function withStatus(int $statusCode): static; +public function withCookie(Cookie $cookie): static; +public function cookies(): array; // list +``` + +- **`withStatus()` is not optional.** `InertiaMiddleware.php:55` rebuilds the response specifically to change 302 → 303 for PUT/PATCH/DELETE. Without `withStatus()`, task 004 is hard-blocked on that branch. +- **`withHeaders()` is not optional either.** `SecurityHeadersMiddleware`, both `CorsMiddleware`s and `RateLimitMiddleware` all apply a *map* of headers via `array_merge`. A bulk method keeps task 004 a mechanical one-line change instead of a hand-unrolled chain. +- **`cookies()` is the accessor tasks 003 and 005 consume.** Task 003 needs it to build `Set-Cookie` lines; task 005 needs it to refuse caching. Name it exactly as above. +- **Duplicate-cookie semantics: replace, don't append.** `withCookie()` with a cookie matching an existing one on (name, path, domain) REPLACES it. This mirrors `withHeader()` and avoids emitting two competing `Set-Cookie` lines for the same name. Cookies differing in name, path, or domain accumulate. + +### Do NOT convert the named constructors + +`json()`, `html()` and `redirect()` stay `static ...: self` using `new self(...)`. Do **not** "improve" them to `new static(...)` for consistency with the `with*()` methods — `StreamingResponse`'s constructor cannot accept `(body:, statusCode:, headers:)`, so `StreamingResponse::json()` would fatal at runtime. That signature mismatch is precisely why `clone` is required. Late-static-bound named constructors are a separate follow-up, out of scope here. + +## Requirements (Test Descriptions) +- [x] `it returns a new instance from withHeader leaving the original unchanged` +- [x] `it merges the new header into the existing headers` +- [x] `it merges a map of headers with withHeaders` +- [x] `it returns a new instance from withStatus leaving the original unchanged` +- [x] `it preserves the concrete subclass when decorating with a header` +- [x] `it preserves the concrete subclass when decorating with a status` +- [x] `it preserves subclass state such as the streaming payload when decorating` +- [x] `it still streams from a decorated streaming response` +- [x] `it does not clobber the sse headers when adding a header to a streaming response` +- [x] `it returns a new instance from withCookie leaving the original unchanged` +- [x] `it accumulates cookies that differ in name path or domain` +- [x] `it replaces a cookie matching an existing name path and domain` +- [x] `it keeps cookies out of the headers collection` + +## Acceptance Criteria +- All requirements have passing tests +- `Response::json()`, `html()`, `redirect()` and the 3-arg constructor still work unchanged, and still use `new self()` +- `packages/sse/src/StreamingResponse.php` drops the `readonly` class modifier and its existing tests stay green +- Code follows code standards +- No decrease in test coverage + +## Implementation Notes + +- `Response` converted from `readonly class` to a plain `class` with a class-level docblock explaining why (clone-then-assign is required; readonly blocks it on PHP 8.5.1). Added `withHeader()`, `withHeaders()`, `withStatus()`, `withCookie()` (all returning `static`, all `clone $this` then assign on the clone) plus a private `list $cookies` property and `cookies(): array` accessor. +- `withCookie()` uses `array_find_key()` to locate a cookie matching on (name, path, domain) and replaces it in place; otherwise appends. +- `Cookie` (still `readonly class`, unchanged otherwise) gained narrow public accessors `name()`, `path()`, `domain()` — the minimum needed for `withCookie()`'s replace-matching logic. Added dedicated tests for these accessors in `CookieTest.php`, including a defaults-to-null case. +- `StreamingResponse` lost the `readonly` class modifier only; its constructor and `send()` are unchanged. `clone` now naturally carries over the private `$stream` property. +- Most requirements after the first (`withHeader` unchanged-original test) passed immediately on writing the test — the single structural change (clone-based `with*()` methods) needed for requirement 1 already implemented the full behavior for `withHeaders`, `withStatus`, `withCookie`, subclass preservation, and header non-clobbering. This was expected and noted per requirement rather than treated as a problem. +- `it still streams from a decorated streaming response` could not use `ob_start()`/`ob_get_clean()` to capture `send()`'s output: `StreamingResponse::send()` runs `while (ob_get_level() > 0) { ob_end_flush(); }` before echoing stream chunks, which forcibly closes any buffer the test itself started (including PHPUnit's), so chunks are echoed with no active buffer to capture them in-process. Test spawns a real PHP subprocess via `proc_open()` (using the project's `vendor/autoload.php`) and asserts on its actual stdout instead. +- Full suite: `composer test` → 6927 passed, 0 failed. `composer phpstan` → 0 errors project-wide. `phpcs` clean on all touched files after one `phpcbf` auto-fix (multi-line method signature) on `Response::withHeader()`. diff --git a/.claude/plans/response-decoration/003-header-line-emission.md b/.claude/plans/response-decoration/003-header-line-emission.md new file mode 100644 index 00000000..4f0048b2 --- /dev/null +++ b/.claude/plans/response-decoration/003-header-line-emission.md @@ -0,0 +1,38 @@ +# Task 003: Header Line Emission and Cookie-Aware send() + +**Status**: completed +**Depends on**: 002 +**Retry count**: 0 + +## Description +Extract response header emission into a testable method that returns the complete set of outbound header lines including one `Set-Cookie` per cookie, and reduce `send()` to a thin loop over it. `header()` is unobservable under the CLI SAPI, so emission logic cannot be tested while it lives inside `send()`. + +## Context +- Modify: `packages/routing/src/Http/Response.php` (`send()` at line 69) +- Modify: `packages/sse/src/StreamingResponse.php` (overrides `send()` and duplicates the header loop at lines 35-40 — it must consume the same method) +- Tests: `packages/routing/tests/Http/ResponseTest.php` +- This seam is also what the future RoadRunner bridge (#151) will consume to map a Marko response onto a PSR-7 response, so keep it free of any SAPI calls. +- `send()` keeps its existing `headers_sent()` guard and `http_response_code()` call; only the header-line construction moves. +- The extracted method reads `headers()` and `cookies()` (the accessor defined in task 002) and returns a `list` of complete header lines. Name it `headerLines(): array` — task 006's tests assert against it by name. +- Emit regular headers first, then one `Set-Cookie` line per cookie, in insertion order. + +## Requirements (Test Descriptions) +- [x] `it returns regular headers as name colon value lines` +- [x] `it returns a distinct set-cookie line for each cookie` +- [x] `it returns only regular header lines when the response has no cookies` +- [x] `it preserves cookie order in the emitted lines` +- [x] `it emits the same header lines for a streaming response subclass` + +## Acceptance Criteria +- All requirements have passing tests +- No SAPI functions (`header()`, `setcookie()`) are called by the extracted method +- Code follows code standards +- No decrease in test coverage + +## Implementation Notes +- Added `Response::headerLines(): array` (returns `list`) that emits regular headers as `"Name: value"` lines followed by one `"Set-Cookie: ..."` line per cookie (via `Cookie::toSetCookieString()`), in insertion order. Contains no SAPI calls. +- `Response::send()` and `StreamingResponse::send()` were reduced to loop `header($line)` over `headerLines()` instead of duplicating the header-construction logic. +- Requirements 3, 4, and 5 passed immediately once requirements 1-2 were implemented (order preservation and no-cookie behavior fall out naturally from the simple two-loop implementation; `headerLines()` is inherited unchanged by `StreamingResponse` since it is not overridden). Noted as expected, not over-implementation, since each test asserts a genuinely distinct requirement. +- The streaming-response test (`it emits the same header lines for a streaming response subclass`) was added to `packages/sse/tests/StreamingResponseTest.php` rather than `packages/routing/tests/Http/ResponseTest.php`, because `packages/routing` does not depend on `packages/sse` (dependency direction is the reverse) and `StreamingResponse` lives in the `sse` package. +- Verified: `composer test` (6932 passed), `composer phpstan` (0 errors), `./vendor/bin/phpcs` on touched files (0 errors), `php-cs-fixer fix` on touched files (only a pre-existing brace-placement fix unrelated to this change). + diff --git a/.claude/plans/response-decoration/004-migrate-middleware.md b/.claude/plans/response-decoration/004-migrate-middleware.md new file mode 100644 index 00000000..37c1b5ed --- /dev/null +++ b/.claude/plans/response-decoration/004-migrate-middleware.md @@ -0,0 +1,61 @@ +# Task 004: Migrate Rebuild-Pattern Middleware to Decoration + +**Status**: completed +**Depends on**: 002 +**Retry count**: 0 + +## Description +Replace the response-rebuilding pattern in all five middleware with decoration calls, so a response passing through them keeps its concrete subclass. This is where the live SSE bug from issue #150 actually stops reproducing. + +## Context +Five files, **six** rebuild sites. Every one of these builds a brand-new base `Response` from the previous response's getters, discarding subclass identity and any subclass state: + +| File | Line | What it rebuilds | Decoration replacement | +|---|---|---|---| +| `packages/security/src/Middleware/SecurityHeadersMiddleware.php` | 27 | headers merge | `withHeaders()` | +| `packages/security/src/Middleware/CorsMiddleware.php` | 44 | headers merge | `withHeader()` | +| `packages/cors/src/Middleware/CorsMiddleware.php` | 67 | headers merge | `withHeaders()` | +| `packages/ratelimiter/src/Middleware/RateLimitMiddleware.php` | 51 | headers merge | `withHeaders()` | +| `packages/inertia/src/Middleware/InertiaMiddleware.php` | 55 | headers merge **+ status 302 → 303** | `withHeaders()` then `withStatus()` | +| `packages/inertia/src/Middleware/InertiaMiddleware.php` | 64 | headers merge | `withHeaders()` | + +**`InertiaMiddleware` has two rebuild sites, not one.** Line 55 is the redirect branch and it also changes the status code (302 → 303 for PUT/PATCH/DELETE) — `withHeader()` alone cannot express it, which is why task 002 provides `withStatus()`. Line 64 is the normal branch. + +**`InertiaMiddleware.php:28` must be left alone.** That is a genuinely fresh 409 version-mismatch response constructed before `$next()` is ever called — it is not a rebuild. The same goes for the fresh preflight/limit responses at `cors/CorsMiddleware.php:48`, `security/CorsMiddleware.php:34` and `ratelimiter/RateLimitMiddleware.php:36`. Task 007's guard test is written to permit exactly these. + +- Depends on task 002 for `withHeader()`, `withHeaders()` and `withStatus()`. Do not start until those exist. +- The regression fixture MUST be a `StreamingResponse` (or an equivalent `Response` subclass carrying extra state), not a base `Response` — a base-class test would pass even with the bug present. +- Header values applied by each middleware must be unchanged; this is a mechanism change, not a behavior change. Existing middleware tests must stay green. +- Note that `cors/CorsMiddleware` and `security/CorsMiddleware` are two separate implementations in two packages. Migrate both. + +## Requirements (Test Descriptions) +- [x] `it preserves the response subclass through security headers middleware` +- [x] `it preserves the response subclass through the security package cors middleware` +- [x] `it preserves the response subclass through the cors package middleware` +- [x] `it preserves the response subclass through rate limit middleware` +- [x] `it preserves the response subclass through inertia middleware` +- [x] `it preserves the response subclass through the inertia redirect branch while upgrading 302 to 303` +- [x] `it preserves the streaming payload when a streaming response passes through security headers middleware` +- [x] `it still applies the same header values after migrating to decoration` + +## Acceptance Criteria +- All requirements have passing tests +- All pre-existing tests for these five middleware still pass unchanged +- No `new Response(` remains **after the `$next()` call** in any of the five middleware; the four pre-`$next()` fresh responses stay as they are +- Code follows code standards + +## Implementation Notes + +- Migrated all six rebuild sites to decoration: + - `security/SecurityHeadersMiddleware.php`: `new Response(...)` → `$response->withHeaders($this->buildSecurityHeaders())`. + - `security/CorsMiddleware.php`: `new Response(...)` → `$response->withHeader('Access-Control-Allow-Origin', $origin)`. + - `cors/CorsMiddleware.php`: `new Response(...)` → `$response->withHeaders($corsHeaders)` (removed the now-dead intermediate `$headers` variable). + - `ratelimiter/RateLimitMiddleware.php`: `new Response(...)` → `$response->withHeaders([...])`. + - `inertia/InertiaMiddleware.php` redirect branch (line 55): `new Response(...)` → `$response->withHeaders($headers)->withStatus($statusCode)`. + - `inertia/InertiaMiddleware.php` normal branch (line 64): `new Response(...)` → `$response->withHeaders($headers)`. + - The four pre-`$next()` fresh responses (409 version-mismatch, both preflight 204s, 429 rate-limited) were left untouched, per the task's explicit carve-out. +- Regression fixtures: added a per-package `tests/Helpers.php` (global-namespace-adjacent, namespaced to `Marko\{Package}\Tests`, loaded via root `composer.json` `autoload-dev.files`) in `security`, `cors`, `ratelimiter`, and `inertia`, each defining a `TaggedResponse extends Response` carrying an extra `tag` property plus a `createTaggedResponse()` factory. `security`'s Helpers.php additionally defines `StreamingLikeResponse` (carries a `chunks` list) for the streaming-payload requirement, avoiding a test-only cross-package dependency on `marko/sse` (per the task's explicit guidance). +- Confirmed RED authentically: for the security-headers middleware, verified via `git stash` on the production file alone that both `preserves the response subclass...` and `preserves the streaming payload...` tests genuinely fail against the pre-migration rebuild code (fails with `Failed asserting that an instance of class Marko\Routing\Http\Response is an instance of class ...`), then restored and re-verified GREEN. The other four middleware's subclass-preservation tests were confirmed RED via `--filter --bail` before their respective migrations. +- `it still applies the same header values after migrating to decoration` and the streaming-payload test were authored alongside the security-headers subclass test (same file, same edit) and consequently already passed once that single production fix landed — this is expected since both exercise the same `withHeaders()` call, not over-implementation of a separate requirement. +- Ran `composer phpstan` (0 errors) and the full suite (`vendor/bin/pest -c phpunit.xml --parallel --exclude-group=integration-destructive`): 6946 passed, 0 failed (pre-existing risky/notice/skipped counts unrelated to this change, e.g. a curl-extension-availability test). +- `phpcs`/`php-cs-fixer` run on all touched files; manually tidied import ordering in the four migrated test files after `php-cs-fixer` split `use function` imports into awkward single-line blocks. diff --git a/.claude/plans/response-decoration/005-page-cache-cookie-policy.md b/.claude/plans/response-decoration/005-page-cache-cookie-policy.md new file mode 100644 index 00000000..e79ee47f --- /dev/null +++ b/.claude/plans/response-decoration/005-page-cache-cookie-policy.md @@ -0,0 +1,45 @@ +# Task 005: Page-Cache Refuses Cookie-Bearing Responses + +**Status**: completed +**Depends on**: 002 +**Retry count**: 0 + +## Description +Make the page cache refuse to store any response carrying cookies. A cached `Set-Cookie` would be replayed to every later visitor, serving one user's session cookie to everybody else — so this is a security boundary, not an optimization. + +## Context +- **Primary (required):** `packages/page-cache/src/CacheabilityChecker.php` — `isResponseCacheable()` at line 34. This is already the central cacheability decision and it **already** rejects a response carrying a literal `set-cookie` header (line 40, via the case-insensitive `getHeader()` helper). Extending it to `$response->cookies() !== []` is one condition, benefits every driver, and needs no new package dependency. +- **Secondary (defence in depth):** `packages/page-cache-file/src/Driver/FilePageCacheDriver.php` — `store()` at line 69 refuses to write, `lookup()` at line 27 never hydrates cookies. +- Tests: existing page-cache and page-cache-file test locations. + +- The driver serializes `status_code`, `body`, and `headers` and rebuilds a base `Response` on read (line 59). It is a serialize/hydrate round-trip, NOT the middleware rebuild pattern — do not try to convert it to decoration. +- Hydrating a cached entry must never produce cookies, including for cache files written before this change. The stored payload has no `cookies` key, so `lookup()` must not start writing one. + +### Do not inject a logger into the driver +The original draft called for a debug log via `LoggerInterface` in `FilePageCacheDriver`. Don't. `packages/page-cache-file/composer.json` requires only core/config/routing/page-cache — adding `marko/log` pulls a new dependency into a driver package, and the constructor change breaks `packages/page-cache-file/tests/Unit/Driver/FilePageCacheDriverTest.php`, which instantiates the driver directly. If observability is wanted, it belongs in `PageCacheMiddleware` (which already has the container) and is a separate, optional change — not a blocker for this task. + +### Interaction with task 006 — read before writing the tests +`SessionMiddleware` is registered as **global middleware** by the session drivers (`packages/session-file/module.php:19-21`, same in `session-database`) and those modules declare `'sequence' => ['after' => ['marko/page-cache']]`. `Router::handle()` builds `[...$globalMiddleware, ...$routeMiddleware]` and `MiddlewarePipeline` peels from the front, so `SessionMiddleware` runs **inside** `PageCacheMiddleware` — its return value is what gets handed to `isResponseCacheable()`. + +If task 006 attached a session cookie to every response, this task would silently disable the page cache entirely for any app with sessions enabled, and no existing test would notice. Task 006 has been constrained to attach the cookie only when the session ID is new, regenerated, or destroyed — mirroring what `session_start()` does today. The last requirement below is the cross-task regression test for that. + +## Requirements (Test Descriptions) +- [x] `it does not cache a response that carries cookies` +- [x] `it still caches a response that carries no cookies` +- [x] `it still refuses to cache a response carrying a literal set-cookie header` +- [x] `it hydrates a cached response with no cookies` +- [x] `it hydrates a cache entry written before cookies existed without error` +- [x] `it still caches a repeat visit response that passed through session middleware without a new session` + +## Acceptance Criteria +- All requirements have passing tests +- Existing page-cache and page-cache-file tests still pass +- No new entries added to `packages/page-cache-file/composer.json` or `packages/page-cache/composer.json` +- Code follows code standards +- No decrease in test coverage + +## Implementation Notes +- `CacheabilityChecker::isResponseCacheable()` gained one new condition (`$response->cookies() !== []`) ahead of the existing literal `set-cookie` header check — one line, no new dependency, benefits every driver. +- `FilePageCacheDriver` needed no production change for the "defence in depth" behavior described in Context: `store()` never serializes `cookies()` (only `status_code`/`body`/`headers`/`tags`/timestamps), and `lookup()` rebuilds a plain `Response` from that payload, which defaults `cookies` to `[]`. Both new driver-level tests (`it hydrates a cached response with no cookies`, `it hydrates a cache entry written before cookies existed without error`) passed immediately against the existing code — this is expected and was verified before moving on, not an oversight. +- The cross-task regression test (`it still caches a repeat visit response that passed through session middleware without a new session`) lives in the new top-level `tests/Integration/PageCacheSessionMiddlewareTest.php`, not inside `packages/page-cache` or `packages/session`, because it exercises both `Marko\Session\Middleware\SessionMiddleware` and `Marko\PageCache\CacheabilityChecker` together and neither package may depend on the other. The root `composer.json` already requires both `marko/page-cache` and `marko/session` as dev dependencies with PSR-4 test autoloading, and an existing `tests/Integration/QueryBuilderRawConsistencyTest.php` established the pattern for this kind of cross-package test. It passed immediately since task 006 (which could regress this) has not run yet — it now stands as a tripwire. +- Full suite (`composer test` equivalent): 6941 passed, 0 failed. Touched files are clean under `php-cs-fixer` and `phpcs`. `phpstan.neon` only analyses `packages/core/src`, so it is out of scope for these files. diff --git a/.claude/plans/response-decoration/006-session-cookie-on-response.md b/.claude/plans/response-decoration/006-session-cookie-on-response.md new file mode 100644 index 00000000..e98400c9 --- /dev/null +++ b/.claude/plans/response-decoration/006-session-cookie-on-response.md @@ -0,0 +1,101 @@ +# Task 006: Session Cookie Travels on the Response + +**Status**: completed +**Depends on**: 002, 003, 006a +**Retry count**: 0 + +## Description +Move the session cookie off the SAPI and onto the `Response` object, for both the set path and the clear path. This makes the session cookie visible to middleware, assertable in tests without superglobal fixtures, and — critically — able to reach a client under a SAPI where `setcookie()` is a no-op. + +## Context +- Modify: `packages/session/src/Session.php` — `configure()` at line 233, `destroy()` at line 159 (the `setcookie()` at line 174) +- Modify: `packages/session/src/Middleware/SessionMiddleware.php` — currently returns `$response` untouched after calling `save()`; becomes the attach point via `withCookie()` +- Modify: `packages/session/composer.json` — see below +- Reference: `packages/session/src/Config/SessionConfig.php` for lifetime, path, domain, secure, httpOnly, sameSite values +- Tests: `packages/session/tests/Unit/Middleware/SessionMiddlewareTest.php` and the other existing session test locations + +**The subtle part — read carefully.** `Session.php:174` is the only `setcookie()` call in the repository, but it only *clears* the cookie in `destroy()`. The cookie is *set* implicitly by `session_start()` through the SAPI. Both paths must move onto the `Response`. + +### `session.use_cookies = 0` also disables session *reading* — you must seed the ID yourself + +`Session::configure()` currently sets both `session.use_cookies = 1` (line 240) and `session.use_only_cookies = 1` (line 241). `use_cookies` is **not** write-only: it also controls whether PHP reads the session ID from the request cookie. Setting it to `0` while `use_only_cookies` stays `1` leaves `session_start()` with no ID source at all, and **every request silently starts a brand-new empty session** — logins, flash messages and CSRF tokens all stop working. No existing test would catch this, because the current session tests never assert continuity across two requests. + +So the middleware must supply the ID explicitly: + +1. Read the inbound cookie by `SessionConfig::cookieName()` off the `Request` (task 006a's accessor). +2. If present, `$session->setId($value)` **before** `start()`. +3. `Session::setId()` validates against `^[a-zA-Z0-9-]{32,128}$` and throws `InvalidSessionIdException` on anything else. That value is attacker-controlled — an invalid or tampered cookie must be **ignored** (fall through to a fresh session), never surface as a 500. +4. Then `start()`. + +`regenerate()` (which calls `session_regenerate_id()`) updates `$this->id`, and the middleware reads the ID *after* `$next()` — so a mid-request regeneration is picked up correctly. + +### Attach the cookie only when it changed — otherwise the page cache dies + +`SessionMiddleware` is registered as **global middleware** by the session drivers (`packages/session-file/module.php:19-21`, same in `session-database`), sequenced `'after' => ['marko/page-cache']`. `Router::handle()` builds `[...$globalMiddleware, ...$routeMiddleware]` and `MiddlewarePipeline` peels from the front, so `SessionMiddleware` runs **inside** `PageCacheMiddleware` and its return value is what reaches `CacheabilityChecker::isResponseCacheable()` — which refuses to cache any cookie-bearing response (task 005, and it already rejects a literal `set-cookie` header today at `CacheabilityChecker.php:40`). + +If every response carried a session cookie, the page cache would silently stop caching anything on any app with sessions enabled. + +Today PHP only emits `Set-Cookie` at `session_start()` when it **creates** or **regenerates** an ID; repeat visitors get nothing. Replicate that exactly: + +- Attach the cookie when the outgoing `getId()` differs from the inbound cookie value (new session, or `regenerate()` ran). +- Attach an **expired** cookie when the session was destroyed. +- Attach **nothing** when the ID is unchanged. + +This is also what makes the "FPM behavior byte-identical" criterion literally true. + +### Detecting a destroyed session without changing `SessionInterface` + +`destroy()` sets `$this->id = ''` (line 169) and `started = false` (line 168). Comparing `getId()` before and after `$next()` therefore distinguishes all three cases with **no** addition to `SessionInterface` — which matters, because any interface change breaks both the anonymous-class fake in `SessionMiddlewareTest.php` and `marko/testing`'s `FakeSession` (`packages/testing/src/Fake/FakeSession.php`). + +Note that the existing `createFakeSession()` helper returns `''` from `getId()` unconditionally, so it will read as "destroyed" unless updated. Update the fake. + +### `destroy()`'s guard becomes dead code + +`Session.php:172` wraps the cookie clearing in `if (ini_get('session.use_cookies'))`. Once `configure()` sets that to `'0'`, this branch can never run. Remove the guard **and** the `setcookie()` call together — do not leave a dead `if` behind. + +### Ripple effects to handle in this task + +- `SessionMiddleware::__construct()` gains `SessionConfig`. All five tests in `packages/session/tests/Unit/Middleware/SessionMiddlewareTest.php` call `new SessionMiddleware($session)` and must be updated. +- `packages/session/composer.json` requires only `marko/core` and `marko/config`, yet `SessionMiddleware` already imports `Marko\Routing\Http\{Request,Response}`. Add `"marko/routing": "self.version"` — the `Cookie` type makes this undeclared dependency untenable. +- Config values must come from `SessionConfig`, never hardcoded. `expireOnClose()` maps to a browser-session cookie with **no** expiry attribute (see task 001), not an epoch timestamp. + +### The "exactly one Set-Cookie under FPM" claim is not directly testable + +`header()` is a no-op under the CLI SAPI — that is the whole reason task 003 exists. A test can never observe a duplicate emitted by `session_start()` through the SAPI. Assert the two things that *are* observable in-process: the response's `headerLines()` contains exactly one `Set-Cookie` for the configured cookie name, **and** `ini_get('session.use_cookies')` reads `'0'` after `start()`. The ini state is the only in-process proxy for "the SAPI will not emit its own". + +### Known behaviour change: the exception path + +`SessionMiddleware` calls `save()` in a `finally` and lets the exception propagate — there is no response to decorate, so an error response carries no session cookie. Today the SAPI emitted it at `session_start()`. With the "attach only when changed" rule this only affects a visitor's very first request, but it is a real change and needs a test pinning the behaviour rather than being discovered later. + +## Requirements (Test Descriptions) +- [x] `it reuses the session id from the inbound request cookie` +- [x] `it ignores an invalid inbound session cookie and starts a fresh session` +- [x] `it attaches the session cookie to the response when the session is new` +- [x] `it does not attach a session cookie when the id is unchanged` +- [x] `it attaches the new session cookie after the session id is regenerated` +- [x] `it emits exactly one session set-cookie line for the configured cookie name` +- [x] `it disables sapi cookie emission when the session starts` +- [x] `it applies the configured lifetime path and domain to the session cookie` +- [x] `it marks the session cookie httponly and applies the configured samesite value` +- [x] `it attaches an expired cookie to the response when the session is destroyed` +- [x] `it does not call setcookie directly` +- [x] `it still saves the session when the handler throws and attaches no cookie` + +## Acceptance Criteria +- All requirements have passing tests +- All pre-existing session tests still pass (five `new SessionMiddleware($session)` call sites updated for the new constructor) +- `marko/testing`'s `FakeSession` still satisfies `SessionInterface` — no interface changes +- No `setcookie()` call and no dead `ini_get('session.use_cookies')` guard remains in `packages/session/` +- `packages/session/composer.json` declares `marko/routing` +- Code follows code standards + +## Implementation Notes + +- `Session::configure()` now sets `session.use_cookies` to `'0'` and no longer calls `session_set_cookie_params()` (that call started emitting an E_WARNING once `use_cookies` is disabled, and is unnecessary now that `SessionMiddleware` owns every cookie attribute). `session.use_only_cookies` is left at `'1'` — harmless now that the ID is always seeded explicitly via `session_id()` before `session_start()`. +- `Session::destroy()` no longer clears a cookie itself; the dead `ini_get('session.use_cookies')` guard and the `setcookie()` call were removed together. Cookie expiry on logout is now `SessionMiddleware`'s job (an expired `Cookie` is attached when `getId()` comes back `''` after `$next()`). +- `SessionMiddleware` gained a `SessionConfig` constructor dependency and now: (1) reads the inbound cookie by `SessionConfig::cookieName()` and seeds `Session::setId()` before `start()`, silently ignoring `InvalidSessionIdException` (attacker-controlled cookie value — commented as a deliberate catch, never a 500); (2) compares the outgoing `getId()` after `$next()`/`save()` against the inbound cookie value to decide whether to attach a fresh cookie (new or regenerated ID), an expired cookie (`getId() === ''`, i.e. destroyed), or nothing (unchanged ID) — this is what keeps the page-cache tripwire test green. +- `packages/session/composer.json` now requires `marko/routing` (already an undeclared transitive dependency via `Request`/`Response`). +- Given the tight coupling between the id-seeding, cookie-attach, and cookie-attribute logic (all live in `SessionMiddleware::handle()` and its private helpers), the RED/GREEN cycle was done at the level of "add one test, confirm it exercises a real assertion against the already-converged implementation" rather than fully isolating each of the 12 requirements into its own minimal-code increment — most of the middleware logic had to exist before any single cookie-lifecycle test could pass. Each test was verified individually passing and the `--filter` RED check was used for the two Session-level tests (`it disables sapi cookie emission...`, structural `it does not call setcookie directly`) where the underlying production change was still separable. +- Five `new SessionMiddleware($session)` call sites in `SessionMiddlewareTest.php` were updated to `new SessionMiddleware($session, createMiddlewareSessionConfig())`. Two additional call sites outside the enumerated five also needed updating to keep the suite compiling: `packages/session-file/tests/ModuleTest.php` (real end-to-end test) and `tests/Integration/PageCacheSessionMiddlewareTest.php` (the page-cache tripwire test), both now construct a real `SessionConfig` and pass matching inbound-cookie request state. +- The local `createFakeSession()` helper in `SessionMiddlewareTest.php` was rewritten to track `setId()`/`getId()` state (with an injectable `onSetId` callback and a `rejectSetId` flag to simulate `InvalidSessionIdException`), rather than returning `''` unconditionally from `getId()`, per the task's callout. `marko/testing`'s `FakeSession` was left untouched — it is never combined with `SessionMiddleware` anywhere in the current codebase (confirmed via repo-wide grep), so updating its `destroy()`/`getId()` semantics was out of scope. +- Verification: `packages/session`, `packages/session-file`, `packages/session-database`, and `tests/Integration/PageCacheSessionMiddlewareTest.php` all pass (`--parallel`), including the page-cache tripwire test proving repeat visits stay cacheable. `composer phpstan` (scoped to `packages/core/src` per `phpstan.neon`) is unaffected and passes with 0 errors. `php-cs-fixer` and `phpcs` report no issues on any touched file. A pre-existing, out-of-scope failure in `tests/MiddlewareDecorationTest.php` (rebuild-after-`$next()` anti-pattern in `authentication`, `cors`, and `security` packages' middleware) was observed in the full-suite run; it predates this task, does not involve any file this task touches, and was confirmed (via `git status`) to be uncommitted work from other in-flight tasks in this same plan. diff --git a/.claude/plans/response-decoration/006a-request-cookie-access.md b/.claude/plans/response-decoration/006a-request-cookie-access.md new file mode 100644 index 00000000..d37d587e --- /dev/null +++ b/.claude/plans/response-decoration/006a-request-cookie-access.md @@ -0,0 +1,46 @@ +# Task 006a: Request Cookie Access + +**Status**: completed +**Depends on**: none +**Retry count**: 0 + +## Description +Give `Marko\Routing\Http\Request` first-class access to inbound cookies. Task 006 needs to read the session cookie off the request in order to seed the session ID; today there is no way to do that without reading `$_COOKIE` directly from middleware, which contradicts the plan's own superglobal stance and the worker-mode direction of issue #151. + +This task has no dependencies and can run in parallel with task 001. + +## Context +- Modify: `packages/routing/src/Http/Request.php` +- Tests: `packages/routing/tests/Http/RequestTest.php` + +**Verified gap.** `Request` has no `cookie()` accessor, and `Request::fromGlobals()` (line 23) captures `$_SERVER`, `$_GET`, `$_POST` and `php://input` but never `$_COOKIE`. Both halves need adding. + +- Follow the shape of the existing `query()` / `post()` accessors exactly: an optional `?string $key` returning the whole `array` when null, plus a `mixed $default`. Keep the same `@return ($key is null ? ... : ...)` conditional-type docblock style. +- `Request` is a `readonly class` and stays one — it is not part of the `Response` decoration change and nothing clones it. +- Cookies become a new constructor parameter. It **must be added last or as a named-only addition with a default**, because `Request::withRoute()` (line 104) reconstructs via `new self(...)` and `fromGlobals()` uses named arguments. Every existing `new Request(server: [...])` call across the test suites must keep working untouched — there are many. +- `withRoute()` must carry cookies through to the new instance. Missing this is a silent data-loss bug that only shows up after routing has matched, which is exactly when `SessionMiddleware` runs. + +## Requirements (Test Descriptions) +- [x] `it returns all cookies when no key is given` +- [x] `it returns a single cookie value by name` +- [x] `it returns the default when the cookie is absent` +- [x] `it defaults to an empty cookie collection` +- [x] `it captures cookies from globals` +- [x] `it preserves cookies through withRoute` + +## Acceptance Criteria +- All requirements have passing tests +- Every pre-existing `new Request(...)` call site across all packages still compiles and passes unchanged +- `Request` remains a `readonly class` +- Code follows code standards +- No decrease in test coverage + +## Implementation Notes +- Added `private array $cookies = []` as the last constructor parameter (named, defaulted) so every pre-existing `new Request(...)` call site keeps working unchanged. +- Added `cookie(?string $key = null, mixed $default = null): mixed` following the exact shape of `query()`/`post()`, including the `@return ($key is null ? array : mixed)` conditional docblock. +- `fromGlobals()` now passes `cookies: $_COOKIE`. +- `withRoute()` now forwards `cookies: $this->cookies` to the reconstructed instance. +- Requirements "returns a single cookie value by name", "returns the default when the cookie is absent", and "defaults to an empty cookie collection" passed immediately after the requirement-1 GREEN step, since the `cookie()` accessor (built to mirror `query()`/`post()` per the task's explicit instruction) already covered key lookup, default fallback, and the empty-array default in one pass. No extra code was written for these three; noting per TDD process rather than backfilling artificial failing tests. +- `Request` remains a `readonly class`; no clones anywhere. +- Verified with `phpstan analyse packages/routing/src/Http/Request.php` — no errors — and `php-cs-fixer fix` on both touched files — no changes needed. +- Full `packages/routing/tests/` run shows one pre-existing failure in `CookieTest.php` (untracked `Cookie.php`/`CookieException.php`/`CookieTest.php`, belonging to a different, unrelated task in this plan) — out of scope for 006a and not touched by this change. diff --git a/.claude/plans/response-decoration/007-architecture-test.md b/.claude/plans/response-decoration/007-architecture-test.md new file mode 100644 index 00000000..e1021c70 --- /dev/null +++ b/.claude/plans/response-decoration/007-architecture-test.md @@ -0,0 +1,134 @@ +# Task 007: Architecture Test Forbidding the Rebuild Pattern + +**Status**: completed +**Depends on**: 004 +**Retry count**: 0 + +## Description +Add a build-gating architecture test that fails when any middleware constructs a `Response` from another response's accessors. Without this, the rebuild pattern silently returns the first time someone adds a header the obvious-looking way, and the SSE bug comes back. + +## Context + +### Location: the root monorepo suite, NOT the routing package +- New test: `/Users/markshust/Sites/marko/tests/MiddlewareDecorationTest.php` +- Fixtures: `/Users/markshust/Sites/marko/tests/Fixtures/MiddlewareDecoration/` + +The original draft put this in `packages/routing/tests/`. That is wrong on two counts: `packages/routing/tests/` is published to the standalone read-only `marko/routing` repo (see `tests/SplitWorkflowTest.php` and `tests/PackagingTest.php`), where `packages/` does not exist — the test would either fail or, far worse, scan zero files and pass vacuously forever. And `marko/routing` has no business reaching into `marko/security` and `marko/inertia`. + +`phpunit.xml` already declares a `Monorepo` testsuite pointing at the root `tests/` directory. That is where cross-package architecture tests live (`tests/PackagingTest.php`, `tests/CiWorkflowTest.php`). + +### The detection rule: position relative to `$next()`, not argument provenance + +The original draft proposed detecting "a `new Response(` whose arguments are fed from another response's `body()` / `statusCode()` / `headers()`". **That heuristic returns a false negative on the exact code this test must catch.** `InertiaMiddleware` does: + +```php +$headers = $response->headers(); +$headers['Vary'] = $this->mergeVaryHeader($headers['Vary'] ?? null); +... +return new Response(body: $response->body(), statusCode: $statusCode, headers: $headers); +``` + +The variable indirection defeats argument matching for `headers`, and a body-less variant would be missed entirely. A guard test that goes green while the pattern is present is the worst possible outcome. + +**Use this rule instead: within a single method body, no `new *Response(` may appear after the first `$next(` call.** + +Verified against every middleware in the repo. Every legitimate fresh response is constructed either *before* `$next()` or in a separate helper method: + +| Site | Construct | Position | Verdict | +|---|---|---|---| +| `inertia/InertiaMiddleware.php:28` | fresh 409 | before `$next()` at :39 | allowed | +| `cors/CorsMiddleware.php:48` | fresh 204 preflight | before `$next()` | allowed | +| `security/CorsMiddleware.php:34` | fresh 204 preflight | before `$next()` at :42 | allowed | +| `ratelimiter/RateLimitMiddleware.php:36` | fresh 429 | before `$next()` at :49 | allowed | +| `authentication/AuthMiddleware.php:49` | fresh 401 | separate helper method | allowed | +| `admin-auth/AdminAuthMiddleware.php:96` | fresh 403 | separate helper method | allowed | +| `authorization/AuthorizationMiddleware.php:88,107` | fresh 401/403 | separate helper methods | allowed | +| the six sites migrated in task 004 | rebuild | after `$next()` | **caught** | + +`LayoutMiddleware` calls `$next()` then returns `$this->layoutProcessor->process(...)` — no `new Response`, passes cleanly. + +Keep the `->body()` / `->statusCode()` / `->headers()` argument check as a **secondary** signal that strengthens the failure message, not as the primary rule. + +### Scanning mechanics +- **There is no `nikic/php-parser` in this repo** (checked the root `composer.json`). Use `token_get_all()`, not regex — regex cannot reliably track method boundaries or distinguish `new Response(` in a comment or string. +- **Do not use a `**` glob.** PHP's `glob()` has no `**` support at all, so `packages/*/src/**/Middleware/*.php` silently misses `packages/security/src/Middleware/SecurityHeadersMiddleware.php`. Walk with `RecursiveDirectoryIterator` over `packages/*/src`. +- Key discovery off `implements MiddlewareInterface` rather than the directory name, so a middleware placed outside a `Middleware/` directory is not silently exempt. +- **The detector must be a class or callable taking an explicit list of files.** If it hardcodes the repo scan, the deliberately-bad fixture used by the negative-case test gets picked up by the repo-wide scan and the suite fails against itself. The repo-wide test passes the discovered file list; the fixture tests pass a fixture path. +- The fixtures directory must be excluded from the repo-wide scan (it lives under `tests/`, not `packages/*/src`, so this falls out naturally — but assert it). +- The failure message must name the offending file **and line** and explain the fix ("decorate with `withHeader()` / `withHeaders()` / `withStatus()` instead of rebuilding"), per the loud-errors principle. + +## Requirements (Test Descriptions) +- [x] `it passes for the current middleware in the repository` +- [x] `it discovers middleware that live directly under src slash middleware` +- [x] `it fails when a middleware constructs a response after calling next` +- [x] `it fails when a middleware rebuilds a response from headers held in a local variable` +- [x] `it allows a middleware to construct a genuinely new response before calling next` +- [x] `it allows a middleware to construct a response in a helper method` +- [x] `it reports the offending file and line and a suggested fix when it fails` + +## Acceptance Criteria +- All requirements have passing tests +- The test passes against the migrated middleware from task 004 +- The test lives in the root `Monorepo` testsuite and does not ship inside any package +- Detection uses `token_get_all()` and `RecursiveDirectoryIterator`, not regex and not `glob()` with `**` +- Code follows code standards + +## Implementation Notes + +- Test file: `tests/MiddlewareDecorationTest.php` (root `Monorepo` testsuite). +- Support classes (`require_once`'d directly from the test file — root `tests/` has + no PSR-4 autoload mapping in `composer.json`, unlike each package's own `tests/`): + - `tests/Support/MiddlewareDecoration/MiddlewareDiscovery.php` — walks `packages/*/src` + with `RecursiveDirectoryIterator`/`RecursiveIteratorIterator` (no `glob()`), then + tokenizes each file with `PhpToken::tokenize()` and looks for a real `implements` + clause naming `MiddlewareInterface` (or a qualified name ending in it). + - `tests/Support/MiddlewareDecoration/MiddlewareDecorationDetector.php` — tokenizes + each file with `PhpToken::tokenize()`, tracks brace depth to find method/closure + body boundaries, and flags any `new *Response(` whose class name ends in + `Response` that appears after the first "consuming" `$next(` call within the same + method body. A secondary signal (`->body()`/`->statusCode()`/`->headers()` calls + earlier in the same method) enriches the failure message but is not the trigger. +- Fixtures: `tests/Fixtures/MiddlewareDecoration/{RebuildAfterNextMiddleware, + RebuildFromLocalVariableMiddleware,FreshResponseBeforeNextMiddleware, + HelperMethodResponseMiddleware}.php`. These implement the real + `Marko\Routing\Middleware\MiddlewareInterface` (autoloaded via the `vendor/marko/routing` + path symlink) so the fixtures exercise realistic code, not stand-ins. + +### Bug found and fixed during implementation: bare `return $next(...)` must not "consume" $next() + +Initial implementation flagged the *first* `$next(` call textually in a method body as the +marker for "everything after this is a rebuild if it's `new Response(`." That produced false +positives on real repo code: `cors/CorsMiddleware.php`, `security/CorsMiddleware.php`, and +`authentication/AuthMiddleware.php` all have an **earlier** short-circuit +`return $next($request);` guard clause (e.g. "no Origin header, pass through") that occurs +*before* a later, unrelated, genuinely-fresh `new Response(` (the OPTIONS-preflight 204, or the +plain-401 fallback) — even though the task's own table classifies these as "before $next()" +positionally. + +Fix: a bare `return $next($request);` statement (nothing chained, nothing assigned) terminates +its branch immediately and has no bearing on sibling code reached only via a different branch +that never called `$next()`. The detector now only marks `$next()` as "consumed" for the rest of +the method when its result is kept (assigned to a variable, chained, or otherwise used) — +`isBareReturnOfNextCall()` in the detector. This matches every verified case in the task's table, +including the two that use an early-return `$next()` guard before their allowed fresh response. + +### Second bug found and fixed: discovery must tokenize, not text-search, for `implements` + +The first `MiddlewareDiscovery` implementation used a `preg_match('/\bimplements\b[^{]*\bMiddlewareInterface\b/')` +text search over raw file contents. This produced a false-positive match on +`packages/core/src/Exceptions/ModuleException.php`, which contains the string +`"...exists and implements " . MiddlewareInterface::class` inside an exception message — a +comment/string mention, not an actual `implements` clause. Discovery now tokenizes each file +with `PhpToken::tokenize()` and only counts a `T_IMPLEMENTS` keyword followed by a name token +(`T_STRING`/`T_NAME_QUALIFIED`/`T_NAME_FULLY_QUALIFIED`/`T_NAME_RELATIVE`) ending in +`MiddlewareInterface`, before the class body `{`. Discovery now returns exactly the 13 real +middleware classes in the repo (previously 14, including the false positive). + +### Verification +- `./vendor/bin/pest tests/MiddlewareDecorationTest.php` — 7/7 passing, 16 assertions. +- `./vendor/bin/pest tests/ --parallel` — full `Monorepo` testsuite green (103 passed). +- `composer test` (full monorepo, both testsuites) — 6965 passed, 0 failures. +- `./vendor/bin/php-cs-fixer fix` and `./vendor/bin/phpcbf`/`phpcs` run against all new files + (test file, both support classes, all four fixtures) — clean, no remaining violations. +- PHPStan's `phpstan.neon` scope is `packages/core/src` only, so it does not analyze root + `tests/` — no PHPStan action needed for these files. diff --git a/.claude/plans/response-decoration/008-resettable-interface.md b/.claude/plans/response-decoration/008-resettable-interface.md new file mode 100644 index 00000000..54c81307 --- /dev/null +++ b/.claude/plans/response-decoration/008-resettable-interface.md @@ -0,0 +1,35 @@ +# Task 008: ResettableInterface in Core + +**Status**: completed +**Depends on**: none +**Retry count**: 0 + +## Description +Introduce a `ResettableInterface` contract in `marko/core` that a service implements to declare it holds request-scoped state which must be cleared between requests. This is the contract a long-running worker calls; without it, any package that adds request-scoped singleton state breaks worker mode with no signal. + +## Context +This interface is being introduced now because the set of services needing it is **known**, not guessed: `Session`, `SessionGuard`, and `ReadWriteConnection` (which already ships `resetStickyState()` for exactly this purpose). Introducing it before that set was known would have been speculative; it no longer is. + +- New file in `packages/core/src/Contracts/` (or alongside existing core contracts — match where core keeps its interfaces). +- Purely additive: no existing class changes in this task, and no BC break. +- Keep the contract minimal — a single method to clear per-request state. Do NOT model it on `__destruct` or `logout()` semantics: reset must be **non-destructive**, meaning it clears in-memory per-request state without destroying persisted data. A `Session` reset must not destroy the stored session; it must forget which session this instance was serving. +- Document that reset runs between requests in a long-running process and is a no-op under PHP-FPM, where the process ends instead. +- The tasks that implement it are 009 (Session, SessionGuard) and 010 (ReadWriteConnection). + +## Requirements (Test Descriptions) +- [x] `it defines a contract for clearing request scoped state` +- [x] `it can be implemented by a class that clears its per request state` +- [x] `it documents that reset is non destructive` + +## Acceptance Criteria +- All requirements have passing tests +- Purely additive — no existing class or interface is modified +- Code follows code standards + +## Implementation Notes +- Added `Marko\Core\Contracts\ResettableInterface` at `packages/core/src/Contracts/ResettableInterface.php`, matching the `src/Contracts/` convention used by every other package in the repo (core had no interfaces of this kind yet, so no existing directory to match). +- Single method `reset(): void`. Docblock documents: (1) a long-running worker calls `reset()` between requests, (2) under PHP-FPM the process ends after each request instead, so `reset()` is effectively a no-op there, (3) `reset()` must be non-destructive — it clears in-memory per-request state without destroying persisted data (e.g. a `Session` reset forgets which session the instance was serving, it does not delete the stored session). +- Tests at `packages/core/tests/Unit/Contracts/ResettableInterfaceTest.php` (3 tests, reflection-based, following the `PluginInterceptedInterfaceTest` pattern already used in core). +- Requirements 2 and 3 passed on first run once requirement 1's interface was written — the interface is small enough that one implementation satisfied all three requirements' assertions; no over-implementation occurred, each test still asserts a distinct part of the contract (structure, implementability, documentation content). +- Purely additive: `git status` confirms only new files were added under `packages/core/src/Contracts/` and `packages/core/tests/Unit/Contracts/`; no existing class or interface was modified. +- Verified: `composer phpstan` (scoped to `packages/core/src`) — 0 errors. `phpcs` and `php-cs-fixer` — clean, no changes needed. Full `packages/core/tests/` suite — 562 passed. diff --git a/.claude/plans/response-decoration/009-request-scoped-session-and-auth.md b/.claude/plans/response-decoration/009-request-scoped-session-and-auth.md new file mode 100644 index 00000000..b3438ddc --- /dev/null +++ b/.claude/plans/response-decoration/009-request-scoped-session-and-auth.md @@ -0,0 +1,79 @@ +# Task 009: Make Session and Auth Guard State Request-Scoped + +**Status**: complete +**Depends on**: 006, 008 +**Retry count**: 0 + +## Description +Give `Marko\Session\Session` and `Marko\Authentication\Guard\SessionGuard` a way to drop their per-request state, and make both implement `ResettableInterface`. Both are container **singletons** that cache the current user's identity in instance properties, and neither can be cleared through its public interface. In any booted-once process this is a cross-user data leak. + +Both leaks below are **verified from source**, not hypotheses. + +## Context + +### Leak 1 — `Session` reuses the previous request's session ID + +`packages/session-file/module.php:16-18` (and `session-database`) bind `SessionInterface` as a **singleton** to `Marko\Session\Session`. + +- `Session::save()` (line 222) sets `started = false` but leaves `$this->id` and `$this->data` populated. +- `Session::start()` (line 52) does `if ($this->id !== '') { session_id($this->id); }`. +- Task 006's `SessionMiddleware` seeds the ID from the inbound request cookie — **but only when a cookie is present**. + +So request N is an authenticated user with a session cookie; request N+1 arrives from an anonymous visitor with **no** cookie; nothing calls `setId()`; `$this->id` still holds request N's ID; `session_id()` loads request N's session. The anonymous visitor is now logged in as the previous user. + +`SessionInterface` offers no escape: `setId('')` throws `InvalidSessionIdException` because `validateId()` requires `^[a-zA-Z0-9-]{32,128}$` (line 271). + +**Fix**: clear `$this->id`, `$this->data` and `$this->flashBag` in `Session::save()` after `session_write_close()`. Under PHP-FPM the process dies immediately afterwards, so this is a behavioural no-op there — prove that by running the existing `packages/session/tests/` suite unchanged. + +### Leak 2 — `SessionGuard` caches the resolved user forever + +`packages/authentication/module.php:25-28` marks `AuthManager` and `GuardInterface` as **singletons**, and `AuthManager::$guards` (line 18) memoizes guard instances. + +`SessionGuard::$cachedUser` (`packages/authentication/src/Guard/SessionGuard.php:24`) is set in `user()` (lines 60-75) and never cleared except by `logout()` — which is destructive and cannot serve as a reset. So request N+1 gets request N's `AuthenticatableInterface` back without ever touching the session. + +**Fix**: give the guard a non-destructive way to forget the cached user. + +### Leak 3 — shutdown functions accumulate per request + +`Session::configure()` (line 254) is called from `start()` on **every** request, and `session_set_save_handler($handler, true)`'s second argument makes PHP `register_shutdown_function()` a `session_write_close` callback each time. In a long-running process that grows unbounded and fires N times at exit. Confirm the behaviour, and if confirmed register the handler once rather than per request. + +### Constraints + +- **Do NOT add reset methods to `SessionInterface` or `GuardInterface`.** `marko/testing`'s `FakeSession`, `FakeUserProvider` and `FakeAuthenticatable`, plus the anonymous-class fakes in `SessionMiddlewareTest`, all implement against those contracts and must still satisfy them with **no edits**. Implement `ResettableInterface` (task 008) on the concrete classes instead — that is precisely why the contract is separate from the domain interfaces. +- Every pre-existing test in `packages/session/`, `packages/session-file/`, `packages/session-database/`, `packages/authentication/` and `packages/testing/` must pass untouched. If one needs changing, that is a signal the change is not FPM-neutral. +- **Prove each leak with a failing test before fixing it.** Simulate two sequential requests against a single `Session` / `SessionGuard` instance — no worker or RoadRunner binary is needed to demonstrate this. A test that only asserts a new method exists proves nothing. + +## Requirements (Test Descriptions) +- [x] `it clears the session id after saving` → renamed to `it clears the session id when reset` (clearing happens in `Session::reset()`, not `save()` — see Implementation Notes) +- [x] `it clears the session data after saving` → renamed to `it clears the session data when reset` (same rationale as above) +- [x] `it starts a fresh session when a second request arrives with no cookie` +- [x] `it resumes the same session when a second request arrives with the same cookie` +- [x] `it forgets the cached user without destroying the session` +- [x] `it does not return the previous requests user to an anonymous request` +- [x] `it registers the session save handler shutdown function only once` + +## Acceptance Criteria +- All requirements have passing tests +- Each leak reproduced by a failing test first, then fixed +- `SessionInterface` and `GuardInterface` are byte-identical to before +- `Session` and `SessionGuard` implement `ResettableInterface` +- All pre-existing session, authentication and testing package tests pass unmodified +- Code follows code standards + +## Implementation Notes + +- **Renamed two requirement names to match correct behaviour**, per the task file's own dependency-context note: `it clears the session id after saving` → `it clears the session id when reset`, and `it clears the session data after saving` → `it clears the session data when reset`. The clearing lives in `Session::reset()` (a new `ResettableInterface` method), not in `save()` — `save()` still only flips `started = false`. This preserves `SessionMiddleware::attachSessionCookie()`'s `getId()` read after `save()`, keeping the page-cache tripwire (`tests/Integration/PageCacheSessionMiddlewareTest.php`) green. + +- **A second, deeper root cause was discovered and fixed for Leak 1**, beyond what the task file anticipated. Clearing `Session::$id` alone (the fix implied by the task text) is *not* sufficient to stop the second visitor of a long-running process from resuming the first visitor's session. PHP's session extension retains the last-used session id in **process memory** (not exposed via any `$this` property) across `session_write_close()` calls. `Session::start()` previously only called `session_id($this->id)` when `$this->id !== ''`, so on the id-less path it silently fell through to PHP's internal state and resumed the previous request's id. Verified directly with a standalone PHP script before touching test code (see conversation — `session_id('')` explicitly forces a fresh id on the next `session_start()`, confirmed by comparing ids across two `session_start()`/`session_write_close()` cycles in the same process). Fixed by calling `session_id($this->id)` unconditionally in `start()`, so an empty `$this->id` explicitly clears PHP's internal state instead of leaving it untouched. This is the change that actually makes `it starts a fresh session when a second request arrives with no cookie` pass — the property-level `reset()` clearing from the first two requirements was necessary but not sufficient. + +- **Requirements 3, 4 and 6 passed immediately** once the preceding requirement's fix landed (integration-level tests confirming behaviour already delivered by the lower-level fix, not over-implementation introduced ahead of a test). Noted per the TDD workflow's guidance for this case. + +- **Test isolation bug found and fixed during requirement 4**: the first drafts of the two-sequential-request `Session` tests (`starts a fresh session...` / `resumes the same session...`) called `$session->start()` a second time without a closing `$session->save()`, leaving PHP's real session in the `PHP_SESSION_ACTIVE` state for the rest of that worker process under `--parallel`. This intermittently broke unrelated tests in the same file with `SessionException: A session is already active`. Fixed by wrapping the assertions in `try { ... } finally { $session->save(); }`, matching the existing `disables sapi cookie emission when the session starts` test's pattern. Verified stable across 5 repeated `--parallel` runs after the fix. + +- **`SessionGuard::reset()`** clears only `$cachedUser`; it never touches the session (non-destructive, per `ResettableInterface`'s contract), verified by `it forgets the cached user without destroying the session` using a spy `UserProviderInterface` that returns a fresh instance per call so the test can prove a re-fetch happened rather than returning the same cached object. + +- **`AuthManager` was deliberately left unchanged.** The task's Leak 2 section says "Consider whether `AuthManager` also needs to participate, given it memoizes guard instances" — but the Acceptance Criteria only require `Session` and `SessionGuard` to implement `ResettableInterface`, and there is no requirement/test asking for `AuthManager` participation. Left out of scope to avoid unrequested surface area; a long-running worker wiring resets together would need to resolve guard instances (e.g. via `AuthManager::guard()`) to reset them, which is a separate, unspecified concern. + +- **Leak 3 (`it registers the session save handler shutdown function only once`)** is proven with a dedicated test file, `packages/session/tests/Unit/SessionShutdownHandlerTest.php`, that defines `Marko\Session\session_set_save_handler()` as a counting wrapper forwarding to the real built-in. PHP resolves the unqualified call inside `Session::configure()` against the `Marko\Session` namespace first, so this intercepts the call without any production-code changes for testability. The fix is a `private bool $handlerRegistered` flag on `Session`, checked in `configure()` — deliberately **not** cleared by `reset()`, since handler registration is per-process state, not per-request state. This test file is self-contained (own inline `SessionConfig`/handler) rather than reusing `SessionTest.php`'s global helper functions, because under `--parallel` paratest can schedule this file in a worker process that never loaded `SessionTest.php`, so those global functions would be undefined — discovered as a real failure during the RED→GREEN cycle and fixed by inlining. + +- Full suite: `6972 passed` (`0 failed`) via `composer test` equivalent (`pest -c phpunit.xml --parallel --exclude-group=integration-destructive`), up from the pre-existing 6965 baseline (+7 new tests, matching the 7 requirements). `composer phpstan` reports `No errors` — its `phpstan.neon` scopes analysis to `packages/core/src` only, so it is unaffected by this task's `packages/session` / `packages/authentication` changes. All three named tripwires (`tests/Integration/PageCacheSessionMiddlewareTest.php`, `tests/MiddlewareDecorationTest.php`, `packages/session/tests/PackageStructureTest.php`) pass unmodified. diff --git a/.claude/plans/response-decoration/010-readwrite-resettable.md b/.claude/plans/response-decoration/010-readwrite-resettable.md new file mode 100644 index 00000000..448afd67 --- /dev/null +++ b/.claude/plans/response-decoration/010-readwrite-resettable.md @@ -0,0 +1,35 @@ +# Task 010: ReadWriteConnection Implements ResettableInterface + +**Status**: completed +**Depends on**: 008 +**Retry count**: 0 + +## Description +Make `ReadWriteConnection` implement `ResettableInterface`, delegating to its existing `resetStickyState()`. This converts the one service that already anticipated worker mode from a bespoke concrete method into the shared contract, so a worker can discover it uniformly. + +## Context +- Modify: `packages/database-readwrite/src/` — the `ReadWriteConnection` class +- `resetStickyState()` already exists and is already documented for exactly this scenario. From that package's own plan: *"Long-running processes (queue workers, Swoole, RoadRunner): `resetStickyState()` is a public method... v1 ships the method but does not auto-wire it."* This task is that wiring. +- Keep `resetStickyState()` as a public method — it is documented in `packages/docs-markdown/docs/packages/database-readwrite.md` and removing it would be a BC break for anyone already calling it. The interface method should delegate to it, not replace it. +- Behaviour must be unchanged: all pre-existing `database-readwrite` tests pass untouched. +- Update that package's docs page to mention it now satisfies the contract. + +## Requirements (Test Descriptions) +- [x] `it implements the resettable contract` +- [x] `it clears sticky write state when reset` +- [x] `it routes reads to a replica again after reset` +- [x] `it keeps the existing reset sticky state method available` + +## Acceptance Criteria +- All requirements have passing tests +- All pre-existing `database-readwrite` tests pass unmodified +- `resetStickyState()` remains public +- Code follows code standards + +## Implementation Notes +- `ReadWriteConnection` now implements `Marko\Core\Contracts\ResettableInterface` in addition to `ConnectionInterface`/`TransactionInterface`. +- New public `reset(): void` method delegates to the existing `resetStickyState()`, which is unchanged and remains public (BC preserved). +- `marko/database-readwrite`'s `composer.json` already required `marko/core`, so no dependency change was needed. +- The requirement `it keeps the existing reset sticky state method available` passed immediately once written (RED phase produced a pass, not a fail) — `resetStickyState()` already existed prior to this task from task 008's prerequisite work, so there was nothing new to implement for that specific test. Noted here per TDD process rather than silently skipped. +- Docs updated at `packages/docs-markdown/docs/packages/database-readwrite.md`: Long-Running Processes section now explains `ResettableInterface`/`reset()` wiring, the API reference "Implements" line now lists `ResettableInterface`, and a `reset(): void` row was added to the method table. +- Full package suite (`packages/database-readwrite/tests/`) — 83 tests passed, all pre-existing tests unmodified. `composer phpstan` and `phpcs`/`php-cs-fixer` clean on touched files. diff --git a/.claude/plans/response-decoration/011-container-resolved-instances.md b/.claude/plans/response-decoration/011-container-resolved-instances.md new file mode 100644 index 00000000..ff315771 --- /dev/null +++ b/.claude/plans/response-decoration/011-container-resolved-instances.md @@ -0,0 +1,36 @@ +# Task 011: Container Resolved-Instances Accessor + +**Status**: completed +**Depends on**: none +**Retry count**: 0 + +## Description +Add an accessor to `Container` that exposes the instances it has actually resolved, so a long-running process can find the request-scoped services it needs to reset instead of relying on a hand-maintained list. + +## Context +- Modify: `packages/core/src/Container/Container.php` +- The problem this solves: `Container::has()` returns true for any class that merely *exists*, and the internal `$instances` map is private. Without an accessor, a worker's reset list must be hardcoded — and any package that later adds request-scoped singleton state breaks worker mode undetectably, because nothing can discover it. +- **Must NOT force instantiation.** The accessor returns only what has already been resolved. Calling it must never construct a service as a side effect — that would change boot behaviour and could be catastrophic in a worker. +- Purely additive. No existing container behaviour changes. +- Consider the return shape carefully: consumers want to filter for services implementing `ResettableInterface` (task 008), so returning the resolved instances keyed by their binding identifier is more useful than returning identifiers alone. +- This is core, so it IS covered by `phpstan.neon` (which analyzes `packages/core/src`) — it must be clean at level 6. + +## Requirements (Test Descriptions) +- [x] `it returns instances that have already been resolved` +- [x] `it does not return bindings that have never been resolved` +- [x] `it does not instantiate anything when called` +- [x] `it returns an empty result for a fresh container` +- [x] `it allows filtering resolved instances by implemented interface` + +## Acceptance Criteria +- All requirements have passing tests +- No service is constructed as a side effect of calling the accessor +- PHPStan level 6 clean +- Code follows code standards + +## Implementation Notes +- Added `Container::resolvedInstances(?string $interface = null): array`. With no argument it returns the private `$instances` map (already-resolved singleton/`instance()` entries) directly — no resolution logic is invoked, so nothing is constructed as a side effect. With an interface argument it `array_filter`s that same map by `instanceof`. +- Only `Container.php` was touched; `ContainerInterface` was left unchanged since Container is the sole implementation and the task scope named only `Container.php`. +- Tests added under a new `resolvedInstances` describe block at the end of `packages/core/tests/Unit/Container/ContainerTest.php`, with fixtures (`CountingServiceInterface`, `CountingService`, `OtherResolvedService`, `InstantiationTrackingService`) placed just above the block, following the file's existing pattern of locating single-purpose fixtures near the describe block that uses them. +- Requirements 2-4 passed immediately once requirement 1's minimal implementation existed (returning the raw `$instances` array already satisfies "no bindings that were never resolved", "no instantiation as a side effect", and "empty for a fresh container") — no extra code was needed for those steps. +- Verified: `composer phpstan` (0 errors), `phpcs`/`php-cs-fixer` clean on touched files, full `composer test` run (6908 passed, 0 failures). diff --git a/.claude/plans/response-decoration/_devils_advocate.md b/.claude/plans/response-decoration/_devils_advocate.md new file mode 100644 index 00000000..e83ee1af --- /dev/null +++ b/.claude/plans/response-decoration/_devils_advocate.md @@ -0,0 +1,326 @@ +# Devil's Advocate Review: response-decoration + +Reviewed against the actual source, not just the plan text. The PHP 8.5.1 decoration finding in +`_plan.md` holds up and no task contradicts it — `readonly class StreamingResponse extends Response` +means dropping `readonly` on `Response` *forces* dropping it on `StreamingResponse` (PHP forbids a +readonly class extending a non-readonly one and vice versa), which the plan already accounts for. +`StreamingResponse` is the only subclass in the repo. `SseStream` is a separate `readonly class` in +an unrelated hierarchy and needs no change. + +The problems are elsewhere: a missing `withStatus()`, an undefined `cookies()` accessor, a session +approach that would break session continuity entirely, a page-cache interaction that would silently +disable the page cache, and an architecture test that is defeated by the exact code it is meant to catch. + +--- + +## Critical (Must fix before building) + +### C1 — Task 002 has no `withStatus()`, so task 004 cannot migrate Inertia +`packages/inertia/src/Middleware/InertiaMiddleware.php:55` rebuilds the response **with a changed +status code** (302 → 303 for PUT/PATCH/DELETE). Task 002 only specifies `withHeader()` and +`withCookie()`; task 004 says "Each becomes a chain of `withHeader()` calls". A worker on task 004 +would be hard-blocked with no way to change the status while preserving the subclass. + +`SecurityHeadersMiddleware`, both `CorsMiddleware`s and `RateLimitMiddleware` all apply a *map* of +headers via `array_merge`, so a `withHeaders(array $headers)` bulk method keeps the migration a +one-liner instead of a hand-unrolled chain. + +**Fix applied:** task 002 now requires `withHeaders(array): static` and +`withStatus(int): static` alongside `withHeader()`/`withCookie()`. Task 004 updated to reference them. + +### C2 — Task 004 undercounts InertiaMiddleware's rebuild sites +The plan lists `InertiaMiddleware.php:64` only. There are **two** rebuild sites — line 55 (redirect +branch, status-changing) and line 64 (normal branch). Line 28 (the 409 version-mismatch response) +is a genuinely fresh response and must be left alone. Verified: five files, six rebuild sites, and +`->body()` appears in exactly those five middleware plus the page-cache driver. + +**Fix applied:** task 004 context now lists both Inertia sites and explicitly exempts line 28. + +### C3 — `cookies()` accessor is never defined, but tasks 003 and 005 both consume it +Task 002 says cookies are "a SEPARATE collection (`list`)" and requires a test that they stay +out of `headers()` — but never names a public accessor. Task 003 must read the cookies to build +`Set-Cookie` lines; task 005 must read them to refuse caching. Two parallel workers would invent two +different names, or block. + +Duplicate-name semantics are also undefined: `withCookie()` called twice with the same name either +appends two `Set-Cookie` lines or replaces. Task 002's requirement literally says "accumulates +multiple cookies", which reads as append-always. + +**Fix applied:** task 002 now specifies `public function cookies(): array` returning `list`, +and pins replace-on-same-(name, path, domain) semantics with a test. + +### C4 — Disabling `session.use_cookies` also disables session *reading*, breaking every session +`_plan.md` and task 006 both say "the session must start with cookie emission disabled". But +`session.use_cookies` is not write-only — it controls whether PHP reads the session ID from the +request cookie as well. `Session::configure()` (`packages/session/src/Session.php:240-241`) currently +sets both `use_cookies=1` and `use_only_cookies=1`. Flipping `use_cookies` to `0` while +`use_only_cookies` stays `1` leaves `session_start()` with **no ID source at all** — every single +request starts a brand-new empty session. Logins, flash messages and CSRF tokens all stop working, +and no existing test would catch it because the current session tests never assert continuity across +two requests. + +The plan's own mitigation as written is the bug. + +**Fix applied:** task 006 now requires the middleware to seed the ID explicitly — read the inbound +cookie from the `Request`, `setId()` it before `start()` — and adds a continuity requirement +("it reuses the session id from the inbound request cookie"). `Session::setId()` throws +`InvalidSessionIdException` for anything failing `^[a-zA-Z0-9-]{32,128}$`, and that value is +attacker-controlled, so an invalid inbound cookie must be ignored (fresh session), never a 500. +Added as an explicit requirement. + +### C5 — `Request` has no cookie access at all; task 006 is blocked on `marko/routing` +`packages/routing/src/Http/Request.php` has no `cookie()` method and `fromGlobals()` (line 37) never +captures `$_COOKIE`. The fix for C4 needs the inbound session cookie, and reading `$_COOKIE` directly +from `SessionMiddleware` contradicts the plan's own superglobal stance and the worker-mode direction +of #151. + +**Fix applied:** new **task 006a — Request cookie access** in `marko/routing` (no dependencies, so it +parallelises with 001). Task 006 now depends on 002, 003 and 006a. + +### C6 — Tasks 005 + 006 together silently disable the page cache +`SessionMiddleware` is registered as **global middleware** by the driver packages +(`packages/session-file/module.php:19-21`, same in `session-database`), and those modules sequence +`'after' => ['marko/page-cache']`. `Router::handle()` builds `[...$globalMiddleware, ...routeMiddleware]` +and `MiddlewarePipeline` peels from the front — so a later-sequenced module runs *inside* an earlier +one. `SessionMiddleware` therefore runs inside `PageCacheMiddleware`, and its return value is what +`PageCacheMiddleware` hands to `isResponseCacheable()`. + +`CacheabilityChecker::isResponseCacheable()` (`packages/page-cache/src/CacheabilityChecker.php:40`) +**already** returns false for any response carrying a `set-cookie` header. Once task 006 makes every +response carry a session cookie and task 005 extends that check to the cookie collection, the page +cache refuses to store anything at all on any app with sessions enabled. Every existing page-cache +test would still pass — they don't run `SessionMiddleware`. + +Today PHP only emits `Set-Cookie` at `session_start()` when it *creates* or *regenerates* an ID; +repeat visitors get no cookie. Replicating that is both the correctness fix and the "FPM behavior +byte-identical" success criterion the plan already claims. + +**Fix applied:** task 006 now requires the cookie to be attached **only** when the outgoing ID differs +from the inbound cookie value (new session or `regenerate()`), or when the session was destroyed — +with requirements covering both the attach and the no-attach case. Task 005 gained a requirement that +a plain cacheable response passing through `SessionMiddleware` on a repeat visit is still cached, and +`_plan.md` gained a matching success criterion and risk entry. + +### C7 — Task 006's constructor change breaks five existing tests, the `SessionInterface` fake, and the manifest +`SessionMiddleware::__construct(SessionInterface $session)` gains `SessionConfig`. All five tests in +`packages/session/tests/Unit/Middleware/SessionMiddlewareTest.php` call `new SessionMiddleware($session)`, +and the file's `createFakeSession()` anonymous class implements the full `SessionInterface` — any +interface addition breaks it, and so does `marko/testing`'s `FakeSession` +(`packages/testing/src/Fake/FakeSession.php`). Note that fake's `getId()` returns `''` +unconditionally, which is exactly the sentinel the destroyed-session path wants to use. + +Separately, `packages/session/composer.json` requires only `marko/core` and `marko/config`, yet +`SessionMiddleware` already imports `Marko\Routing\Http\{Request,Response}`. Adding `Cookie` makes an +existing undeclared dependency worse. + +**Fix applied:** task 006 context now enumerates all of these — the five call sites, both fakes, and +the `marko/routing: self.version` composer requirement — and prefers a signal that needs **no** +`SessionInterface` change (compare `getId()` before/after; `destroy()` already zeroes it at +`Session.php:169`). + +### C8 — Task 007 lives in the wrong test suite and will break the split repos +Task 007 places a repo-wide scanner "in the routing package's test suite". `packages/routing/tests/` +is published to the standalone read-only `marko/routing` repo (see `tests/SplitWorkflowTest.php`, +`tests/PackagingTest.php`), where `packages/` does not exist — the test would either fail or, worse, +scan zero files and pass vacuously forever. It also has no business making `marko/routing` reach into +`marko/security` and `marko/inertia`. + +`phpunit.xml` already defines a `Monorepo` testsuite pointing at the root `tests/` directory, which is +exactly where cross-package architecture tests belong (`tests/PackagingTest.php`, `tests/CiWorkflowTest.php`). + +**Fix applied:** task 007 relocated to `/Users/markshust/Sites/marko/tests/MiddlewareDecorationTest.php`. + +### C9 — Task 007's heuristic returns a false negative on the very code it must catch +Task 007 describes the signal as "a `new Response(` whose arguments are fed from another response's +`body()`/`statusCode()`/`headers()`". `InertiaMiddleware` does: + +```php +$headers = $response->headers(); +$headers['Vary'] = $this->mergeVaryHeader($headers['Vary'] ?? null); +... +return new Response(body: $response->body(), statusCode: $statusCode, headers: $headers); +``` + +The variable indirection defeats argument-source matching for the `headers` argument, and a +body-less variant (`new Response(body: $body, ...)`) would be missed entirely. The test would go green +while the rebuild pattern is present — the single worst outcome for a guard test. + +There is also no `nikic/php-parser` in the repo (checked the root `composer.json`), so real AST +analysis is not available; `token_get_all()` is. + +A much more robust rule, which I verified against every middleware in the repo: **no `new *Response(` +may appear after the first `$next(` call within the same method body.** Every legitimate fresh +response is constructed *before* `$next()` or in a separate helper method: + +| Site | Construct | Position | Verdict | +|---|---|---|---| +| `inertia/InertiaMiddleware.php:28` | fresh 409 | before `$next()` at :39 | passes | +| `cors/CorsMiddleware.php:48` | fresh 204 preflight | before `$next()` | passes | +| `security/CorsMiddleware.php:34` | fresh 204 preflight | before `$next()` at :42 | passes | +| `ratelimiter/RateLimitMiddleware.php:36` | fresh 429 | before `$next()` at :49 | passes | +| `authentication/AuthMiddleware.php:49` | fresh 401 | separate helper method | passes | +| `admin-auth/AdminAuthMiddleware.php:96` | fresh 403 | separate helper method | passes | +| `authorization/AuthorizationMiddleware.php:88,107` | fresh 401/403 | separate helper methods | passes | +| the six rebuild sites | rebuild | after `$next()` | **caught** | + +`LayoutMiddleware` calls `$next()` and then returns `$this->layoutProcessor->process(...)` — no +`new Response`, passes cleanly. + +**Fix applied:** task 007 rewritten around the position rule (with the argument heuristic kept as a +secondary signal), `token_get_all()` mandated over regex, and the detector required to be a class +taking an explicit file list — otherwise the negative-case fixture gets picked up by the repo-wide +scan and the suite fails against itself. + +--- + +## Important (Should fix before building) + +### I1 — Task 007's glob misses half the middleware +`packages/*/src/**/Middleware/*.php` does not match `packages/security/src/Middleware/SecurityHeadersMiddleware.php` +under PHP's `glob()`, which has no `**` support whatsoever. Middleware in this repo sit at *both* +`src/Middleware/` (security, session, cors, inertia, ratelimiter, layout, page-cache) and +`src/Middleware/` nested variants. Discovery must be a recursive directory walk, and should key off +`implements MiddlewareInterface` rather than directory name so a middleware placed elsewhere is not +silently exempt. + +**Fix applied** in task 007. + +### I2 — Task 001 leaves cookie encoding, "no expiry", and SameSite=None undefined +Three gaps that will produce wrong headers: +- **Value encoding.** `Set-Cookie` values cannot contain `;`, `,`, whitespace or control characters. + Session IDs happen to be safe; arbitrary application cookies are not. Raw vs. `urlencode` must be + decided in the value object, not left to callers. +- **No-expiry cookies.** `SessionConfig::expireOnClose()` maps to `lifetime => 0`, i.e. a browser + session cookie with **no** `Expires`/`Max-Age` attribute at all. The task's `expires` attribute has + no stated representation for that. +- **`SameSite=None` requires `Secure`.** Browsers drop the cookie otherwise. Per "loud errors", this + should throw at construction. + +**Fix applied:** three requirements added to task 001. + +### I3 — Task 005's logger dependency adds a package dependency and breaks existing tests +Injecting `LoggerInterface` into `FilePageCacheDriver` requires adding `marko/log` to +`packages/page-cache-file/composer.json` (currently core/config/routing/page-cache only) and changes +a constructor that `packages/page-cache-file/tests/Unit/Driver/FilePageCacheDriverTest.php` +instantiates directly. + +More importantly the check is in the wrong place. `CacheabilityChecker::isResponseCacheable()` is +already the central cacheability decision and *already* rejects a literal `set-cookie` header at +line 40 — extending it to the cookie collection is one line, benefits every driver, and needs no new +dependency. The driver check is worth keeping as defence in depth, but it is the checker that must change. + +**Fix applied:** task 005 now names `CacheabilityChecker` as the required location, keeps the driver +guard as secondary, and demotes the debug logging to optional (relocated to `PageCacheMiddleware`, +which can already reach the container) so it does not drag `marko/log` into a driver package. + +### I4 — "Exactly one Set-Cookie under FPM" is not assertable from the test suite +`header()` is a no-op under the CLI SAPI — the plan says so itself in task 003's rationale — so +task 006's requirement `it emits exactly one session set-cookie header` can only ever observe the +`Response`'s own header lines. It structurally cannot detect a duplicate emitted by `session_start()` +through the SAPI, which is the actual risk being mitigated. As written the test gives false confidence. + +**Fix applied:** task 006's requirement is now split into two concrete, actually-observable +assertions: the response's header lines contain exactly one `Set-Cookie` for the session cookie name, +**and** `session.use_cookies` reads `'0'` after `start()` (the ini state is the only in-process proxy +for "the SAPI will not emit its own"). `_plan.md` success criterion reworded to match. + +### I5 — Task 006 is well over one TDD cycle +As written it spans two packages and covers: `Session::configure()` ini semantics, `Session::destroy()`, +a new `SessionMiddleware` constructor dependency, inbound-ID seeding, invalid-ID handling, +new-vs-existing attach logic, destroyed-session expiry, a composer.json change, five broken test call +sites, and two `SessionInterface` fakes. That is not one red-green-refactor loop. + +**Fix applied:** extracted task 006a (Request cookie access, `marko/routing`, zero dependencies) which +also improves parallelism — it can run alongside 001. Task 006 retains the session work. + +### I6 — Task 002 does not test that the decorated stream still streams +The requirement `it preserves subclass state such as the streaming payload when decorating` asserts +the property survives. It does not assert the *behaviour* survives — that `send()` on the clone still +runs `StreamingResponse::send()` and iterates the stream. `clone` is shallow, so the clone shares one +`SseStream` instance with the original; `SseStream::getIterator()` returns a fresh `Generator` per +call so this is safe, but it is an unstated invariant that a future `__clone()` could break. Also +untested: that `withHeader()` on a `StreamingResponse` does not clobber the four SSE headers set by +its constructor. + +**Fix applied:** two requirements added to task 002. + +### I7 — `Session::destroy()`'s clear path dies silently under the new ini setting +`Session.php:172` guards the cookie clearing with `if (ini_get('session.use_cookies'))`. Once +`configure()` sets that to `'0'`, the branch is permanently dead — the `setcookie()` disappears and +nothing replaces it unless the task explicitly says so. Task 006 says both paths must move, but the +guard is not mentioned and a worker deleting only the `setcookie()` line would leave a dead `if`. + +`destroy()` sets `$this->id = ''` at line 169, which is a usable "was destroyed" signal for the +middleware without touching `SessionInterface`. Note `createFakeSession()`'s `getId()` returns `''` +unconditionally, so the existing fakes need updating or those tests will see a phantom destroy. + +**Fix applied:** task 006 context and requirements updated. + +### I8 — Nothing forbids a worker from "fixing" the named constructors into `new static()` +`Response::json()`, `html()` and `redirect()` are `static ...: self` using `new self(...)`. A worker +reading task 002's "preserves the concrete subclass, return `static` not `self`" guidance could +reasonably decide the named constructors should be late-static-bound too. That would be a fatal +error: `StreamingResponse::__construct(SseStream $stream, int $statusCode)` cannot accept +`(body:, statusCode:, headers:)`, so `StreamingResponse::json()` would blow up at runtime — the exact +signature mismatch that makes `clone` necessary in the first place. + +**Fix applied:** task 002 now states explicitly that the three named constructors keep `self` / +`new self()` and are out of scope, with the reason. + +### I9 — The session cookie is lost on the exception path +`SessionMiddleware` calls `save()` in a `finally` and lets the exception propagate — there is no +response to decorate. Today the SAPI emitted the session cookie at `session_start()`, so even a +500 page carried it. After this change an error response carries no cookie, so a first-time visitor +who hits an error loses the session and any flash message or CSRF token queued for the error page. +The existing test `it saves session even when handler throws` covers `save()` but not the cookie. + +The C6 fix ("attach only when new") narrows this to first requests, but it is still a real behaviour +change and should be a conscious, tested decision rather than a discovery. + +**Fix applied:** noted in task 006 context with a requirement that the exception path is covered. + +--- + +## Minor (Nice to address — not applied) + +- **M1.** `_plan.md` and task 002 both cite "nine existing `->headers()` call sites". The real count + is eight production call sites on a Marko `Response` (`ratelimiter:54`, `security/SecurityHeaders:30`, + `security/Cors:47`, `cors:65`, `inertia:45`, `sse/StreamingResponse:38`, `page-cache-file:81`, + `page-cache/CacheabilityChecker:96`) plus roughly eighty in tests, plus an unrelated `headers()` on + `marko/http-guzzle`'s own response type. Harmless — the signature is unchanged, so all of them stay + source-compatible — but the number is wrong and reads as more precise than it is. +- **M2.** `phpstan.neon` analyses `packages/core/src` only. The success criterion "PHPStan at zero + errors" therefore provides no static-analysis coverage of any file this plan touches. True as + written, but weaker than it sounds. +- **M3.** `Session::validateId()` requires `^[a-zA-Z0-9-]{32,128}$`. PHP's default + `session.sid_bits_per_character=5` produces a matching alphabet, but an app that sets it to `6` + gets `,` in IDs and every inbound cookie would be rejected as invalid. Edge case, only reachable via + explicit ini tuning. +- **M4.** Nothing stops `withHeader('Set-Cookie', ...)` from bypassing the cookie collection entirely, + producing a cookie that `cookies()` cannot see. Per "loud errors", consider throwing and directing + the caller to `withCookie()`. +- **M5.** The shallow-`clone` sharing of `SseStream` between original and decorated response is + correct today but undocumented. Worth a class-level comment so nobody adds a deep-copying + `__clone()` later. + +## Questions for the Team + +1. **`withCookie()` with a duplicate name — replace or append?** I pinned *replace* on matching + (name, path, domain) in task 002, since that is what `Response::withHeader()` does for headers and + what browsers effectively do with duplicate `Set-Cookie` lines. Append is defensible if you want + the same name scoped to two paths. Confirm the choice. +2. **Should the session cookie always ride the response, or only when it changes?** I applied + "only when new/regenerated/destroyed" because always-attach silently kills the page cache (C6) and + diverges from today's FPM behaviour. The cost: the `Response` is not a complete picture of session + state on repeat requests, which slightly weakens the "session cookie travels on the Response" + framing. If you would rather always attach, task 005 needs a session-cookie carve-out instead — + which is a security decision, not a mechanical one. +3. **Should `Cookie` live in `marko/routing`?** It forces `marko/session` to declare a dependency on + `marko/routing` (which it already has undeclared). That is fine and probably correct, but it does + mean any package wanting to set a cookie now depends on the routing package. +4. **Follow-up: late-static-bound named constructors.** Making `json()`/`html()`/`redirect()` work + correctly for subclasses would need a different construction strategy than `new static()`. Worth an + issue, deliberately not touched here. +5. **Should `Request` become the sole source of cookies for the whole framework?** Task 006a adds + `Request::cookie()`. `Session` still reaches PHP's session machinery. Full superglobal removal is + #151 territory but the boundary is worth agreeing on now. diff --git a/.claude/plans/response-decoration/_plan.md b/.claude/plans/response-decoration/_plan.md new file mode 100644 index 00000000..b6eab26e --- /dev/null +++ b/.claude/plans/response-decoration/_plan.md @@ -0,0 +1,123 @@ +# Plan: Response Decoration API with Cookie Support + +## Created +2026-08-28 + +## Status +completed + +## Objective +Make every piece of request-scoped state in the framework explicit and clearable: give `Marko\Routing\Http\Response` a decoration API so middleware stop rebuilding responses (which silently downgrades `StreamingResponse` and discards SSE streams), and give responses first-class cookie support so the session cookie travels on the `Response` object instead of the SAPI. + +## Related Issues +Closes #150 + +## Discovery Notes + +**The live bug.** `StreamingResponse extends Response` (`packages/sse/src/StreamingResponse.php:11`) carries its payload in an `SseStream` with `body` = `''`, and overrides `send()` to stream it. Five middleware add headers by constructing a brand-new base `Response` from `body()`/`statusCode()`/`headers()`. Any of them in front of an SSE route downgrades the `StreamingResponse` to a plain `Response` — stream discarded, overridden `send()` never runs, client gets an empty 200. `SecurityHeadersMiddleware` is the kind of middleware apps register globally, so this is reachable in practice. + +**Verified on PHP 8.5.1 — the decoration mechanism.** Four approaches were tested empirically: + +| Approach | Result | +|---|---| +| `clone $this with { ... }` | Not available in 8.5.1 — parse error | +| `readonly class` + clone then assign in class scope | Fails: `Cannot modify readonly property` | +| `readonly class` + `ReflectionProperty::setValue` on the clone | Fails: same error | +| Plain class, private non-readonly props, `clone` + assign | **Works** — preserves concrete subclass, subclass state, and leaves the original untouched | + +`Response` must therefore drop the `readonly` **class modifier**. Immutability is preserved by API design: private properties, no setters, `with*()` returns modified clones. This is the PSR-7 implementation approach and is consistent with the project convention "readonly — use when appropriate for immutability, not as a blanket rule." + +**Cookie representation (decided).** Cookies live in a separate collection, not as multi-valued headers. `headers()` keeps its `array` type, so every existing `->headers()` call site stays source-compatible — eight in production code, roughly eighty across the test suites. `send()` merges cookies into `Set-Cookie` lines at emit time. + +**Decoration needs more than `withHeader()`.** `InertiaMiddleware.php:55` rebuilds specifically to change the status code (302 → 303 for PUT/PATCH/DELETE), and four of the five middleware apply a *map* of headers via `array_merge`. The API is therefore `withHeader()`, `withHeaders()`, `withStatus()`, `withCookie()`, plus a `cookies()` accessor that tasks 003 and 005 consume. `InertiaMiddleware` has **two** rebuild sites (lines 55 and 64), so the five files contain six rebuild sites. + +**Session is more involved than one call site.** `Session.php:174` is the only `setcookie()` in the repo, but it lives in `destroy()` (clearing the cookie). The *setting* of the session cookie is emitted implicitly by `session_start()` through the SAPI. Both paths must move onto the `Response`. `SessionMiddleware` (`packages/session/src/Middleware/SessionMiddleware.php`) already wraps the response and calls `save()`, so it is the natural attach point. + +Two traps here, both verified against the source: + +- **`session.use_cookies = 0` disables session *reading*, not just writing.** `Session::configure()` sets `use_cookies=1` and `use_only_cookies=1` (lines 240-241). Turning the first off leaves `session_start()` with no ID source and every request begins a brand-new empty session. The middleware must therefore seed the ID explicitly — read the inbound cookie off the `Request` and `setId()` it before `start()` — and ignore invalid attacker-supplied IDs rather than letting `InvalidSessionIdException` become a 500. This requires cookie access on `Request`, which does not exist today (`fromGlobals()` never captures `$_COOKIE`); hence task 006a. +- **Attaching the cookie unconditionally would silently disable the page cache.** See below. + +**Page-cache is a distinct case, and it collides with the session work.** `FilePageCacheDriver:59,81` is a serialize/hydrate round-trip, not middleware decoration. A cached `Set-Cookie` would serve one user's session cookie to every later visitor, so cookie-bearing responses must not be cached. `CacheabilityChecker::isResponseCacheable()` already enforces exactly this for a literal `set-cookie` header (line 40), and is the right place to extend. + +But `SessionMiddleware` is global middleware registered by the session drivers (`session-file/module.php:19-21`) and sequenced `after: marko/page-cache`, so it runs *inside* `PageCacheMiddleware`. If every response carried a session cookie, the page cache would refuse to store anything on any app with sessions enabled — and no existing test would notice. PHP itself only emits `Set-Cookie` when `session_start()` creates or regenerates an ID; the middleware must mirror that and attach only on new / regenerated / destroyed sessions. + +## Scope + +### In Scope +- `Cookie` value object with correct `Set-Cookie` rendering +- `Request` cookie access (`cookie()` accessor plus `$_COOKIE` capture in `fromGlobals()`) +- `Response` decoration API (`withHeader()`, `withHeaders()`, `withStatus()`, `withCookie()`, `cookies()`) preserving concrete subclass +- Testable header-line emission consumed by `send()`, emitting one `Set-Cookie` per cookie +- Migrating the five rebuild-pattern middleware to decoration +- Architecture test that fails the build if the rebuild pattern reappears in middleware +- Session cookie (set and clear) travelling on the `Response` +- Page-cache refusing to cache cookie-bearing responses +- `ResettableInterface` in `marko/core` — the contract for services holding request-scoped state +- `Session` and `SessionGuard` made request-scoped (three verified cross-user leaks) and implementing that contract +- `ReadWriteConnection` implementing that contract, wiring up its existing `resetStickyState()` +- `Container` accessor exposing already-resolved instances, without forcing instantiation + +### Out of Scope +- Worker-mode runtime, PSR-7 bridge, RoadRunner package (issue #151) +- Calling the reset lifecycle per request — the worker that does the calling is #151; this plan only supplies the contract and its implementors +- Refactoring debugbar's superglobal reads (dev-only tool) +- `errors-advanced/RequestDataCollector` (already constructor-injectable with superglobal fallback) +- Introducing PSR-7 anywhere — core has zero PSR-7 and that must stay true + +## Success Criteria +- [ ] A `StreamingResponse` passed through `SecurityHeadersMiddleware` retains its concrete subclass, its stream, and still streams when sent +- [ ] A response can carry multiple cookies, each emitted as its own `Set-Cookie` line +- [ ] `Response::json()`, `html()`, `redirect()` and the 3-arg constructor remain source-compatible and keep using `new self()` +- [ ] A request carrying a valid session cookie reuses that session — no new session per request +- [ ] The response carries exactly one session `Set-Cookie` line when the session is new or regenerated, and none when the ID is unchanged; `session.use_cookies` reads `'0'` after `start()` so the SAPI emits no duplicate +- [ ] Existing session tests still green; FPM behavior unchanged from the client's perspective +- [ ] Page-cache never stores a response carrying cookies, **and still caches repeat-visit responses that passed through `SessionMiddleware`** +- [ ] Architecture test fails when the rebuild pattern is reintroduced in middleware, including via a local `$headers` variable +- [ ] An anonymous request following an authenticated one starts a fresh session — the verified cross-user leak is reproduced by a failing test first, then fixed +- [ ] The cached authenticated user does not survive into a subsequent request +- [ ] The session save-handler shutdown function is registered once, not once per request +- [ ] `SessionInterface` and `GuardInterface` are byte-identical to before, and every `marko/testing` fake still satisfies them unedited +- [ ] The container accessor never constructs a service as a side effect of being called +- [ ] `composer ci` fully green (tests, lint, PHPStan at zero errors — note `phpstan.neon` analyses `packages/core/src` only, so it covers none of the files this plan touches) + +## Task Overview +| Task | Description | Depends On | Status | +|------|-------------|------------|--------| +| 001 | Cookie value object and Set-Cookie rendering | - | completed | +| 006a | Request cookie access | - | completed | +| 008 | ResettableInterface in core | - | completed | +| 011 | Container resolved-instances accessor | - | completed | +| 002 | Response decoration API preserving subclass | 001 | completed | +| 010 | ReadWriteConnection implements ResettableInterface | 008 | completed | +| 003 | Header line emission and cookie-aware send() | 002 | completed | +| 004 | Migrate rebuild-pattern middleware to decoration | 002 | completed | +| 005 | Page-cache refuses cookie-bearing responses | 002 | completed | +| 006 | Session cookie travels on the Response | 002, 003, 006a | completed | +| 007 | Architecture test forbidding the rebuild pattern | 004 | completed | +| 009 | Request-scoped Session and auth guard | 006, 008 | completed | + +Batches: **(1)** 001, 006a, 008, 011 → **(2)** 002, 010 → **(3)** 003, 004, 005 → **(4)** 006, 007 → **(5)** 009. + +### Later decisions (added after the RoadRunner plan's review) + +- **The session and auth request-scoping fixes live here, not in #151.** They were found while planning the worker, but they modify `marko/session` and `marko/authentication` and cannot be fixed from inside a driver package. Putting them here keeps #151 purely additive, which was the whole point of splitting the two. +- **`ResettableInterface` is introduced now.** The earlier decision to defer it rested on not knowing what needed resetting. The set is now known and verified — `Session`, `SessionGuard`, `ReadWriteConnection` — so the contract is no longer speculative. +- **`Container` gains a resolved-instances accessor.** Without it a worker's reset list must be hardcoded, and any package that later adds request-scoped singleton state breaks worker mode undetectably. + +## Architecture Notes +- `Response` loses the `readonly` class modifier; properties stay private with no setters. `StreamingResponse` **must** follow suit — PHP forbids a readonly class extending a non-readonly one — and it is the only `Response` subclass in the repo. `SseStream` is a separate `readonly class` in an unrelated hierarchy and is unaffected. +- `with*()` methods return `static` and use `clone` — never `new static(...)`, because `StreamingResponse`'s constructor signature differs from its parent's. This is precisely what makes the SSE fix work. The named constructors `json()` / `html()` / `redirect()` deliberately stay `self` / `new self()` for the same reason: `StreamingResponse::json()` would fatal under late static binding. +- `clone` is shallow, so a decorated `StreamingResponse` shares one `SseStream` with the original. That is correct — `SseStream::getIterator()` returns a fresh `Generator` per call — but it is an invariant a future `__clone()` could break. +- Cookies are a separate `list` collection; `headers()` keeps `array`. `withCookie()` replaces on matching (name, path, domain) rather than appending duplicates. +- Header emission is extracted into a testable `headerLines()` method so `send()` stays a thin loop — `header()` is unobservable under the CLI SAPI, so emission logic must not live inside `send()` itself. This also gives the future RoadRunner bridge (#151) a ready-made seam. +- Loud errors: invalid cookie names throw, and `SameSite=None` without `Secure` throws, rather than silently producing a header the browser discards. + +## Risks & Mitigations +- **Total session loss from disabling `session.use_cookies`**: the ini flag governs reading as well as writing, so flipping it without seeding the ID starts a fresh session on every request. Mitigation: `SessionMiddleware` reads the inbound cookie off the `Request` and calls `setId()` before `start()`; invalid IDs are ignored rather than thrown; a continuity test asserts the ID survives a round trip. +- **Duplicate session cookie under FPM**: `session_start()` emits its own `Set-Cookie`. Mitigation: disable SAPI cookie emission at session start and let `SessionMiddleware` own the cookie. Note this cannot be asserted directly — `header()` is a no-op under CLI — so the tests assert the ini state plus a single `Set-Cookie` in `headerLines()`. +- **Page cache silently stops caching**: `SessionMiddleware` runs inside `PageCacheMiddleware`, so an always-attached session cookie would make every response uncacheable. Mitigation: attach only on new / regenerated / destroyed sessions, mirroring PHP's own behavior; task 005 carries the cross-task regression test. +- **Dropping `readonly` weakens the immutability guarantee**: mitigated by keeping properties private with no setters and covering "original untouched" in tests for every `with*()` method. +- **Silent regression of the rebuild pattern**: mitigated by task 007's architecture test. The rule is positional (no `new *Response(` after `$next()` in the same method) rather than argument-provenance based, because the latter is defeated by `InertiaMiddleware`'s local `$headers` variable — the exact code it must catch. +- **Subclass state loss in decoration**: mitigated by testing decoration against `StreamingResponse` specifically, and by asserting the decorated clone still *streams*, not merely that it retains the property. +- **Session cookie lost on the exception path**: an exception in `$next()` leaves no response to decorate, so an error response carries no cookie where the SAPI previously supplied one. Narrowed to first requests by the attach-only-when-changed rule; pinned by a test rather than left to be discovered. diff --git a/composer.json b/composer.json index 6b558faf..ff1cd6b0 100644 --- a/composer.json +++ b/composer.json @@ -606,8 +606,12 @@ "Marko\\Skeleton\\Tests\\": "packages/skeleton/tests/" }, "files": [ + "packages/cors/tests/Helpers.php", "packages/database-mysql/tests/Connection/Helpers.php", - "packages/devai/tests/Helpers.php" + "packages/devai/tests/Helpers.php", + "packages/inertia/tests/Helpers.php", + "packages/ratelimiter/tests/Helpers.php", + "packages/security/tests/Helpers.php" ] } } diff --git a/packages/authentication/src/Guard/SessionGuard.php b/packages/authentication/src/Guard/SessionGuard.php index 371f01cb..50d9b88c 100644 --- a/packages/authentication/src/Guard/SessionGuard.php +++ b/packages/authentication/src/Guard/SessionGuard.php @@ -13,11 +13,13 @@ use Marko\Authentication\Event\LogoutEvent; use Marko\Authentication\Exceptions\AuthException; use Marko\Authentication\Token\RememberTokenManager; +use Marko\Core\Contracts\ResettableInterface; use Marko\Core\Event\EventDispatcherInterface; use Marko\Session\Contracts\SessionInterface; +use Override; use Random\RandomException; -class SessionGuard implements GuardInterface +class SessionGuard implements GuardInterface, ResettableInterface { private const int REMEMBER_COOKIE_MINUTES = 43200; // 30 days @@ -126,6 +128,8 @@ public function id(): int|string|null } /** + * @param array $credentials + * * @throws AuthException|RandomException */ public function attempt( @@ -150,6 +154,9 @@ public function attempt( return true; } + /** + * @param array $credentials + */ private function dispatchFailedLoginEvent( array $credentials, ): void { @@ -282,4 +289,10 @@ public function getName(): string { return $this->name; } + + #[Override] + public function reset(): void + { + $this->cachedUser = null; + } } diff --git a/packages/authentication/tests/Unit/Guard/SessionGuardTest.php b/packages/authentication/tests/Unit/Guard/SessionGuardTest.php index 271892bb..b6630689 100644 --- a/packages/authentication/tests/Unit/Guard/SessionGuardTest.php +++ b/packages/authentication/tests/Unit/Guard/SessionGuardTest.php @@ -718,6 +718,85 @@ public function updateRememberToken( ->and($session->get('auth_admin_user_id'))->toBe(2); }); +test('it forgets the cached user without destroying the session', function (): void { + $session = new FakeSession(); + $session->set('auth_web_user_id', 42); + + $provider = new class () implements UserProviderInterface + { + public int $retrieveCalls = 0; + + public function retrieveById(int|string $identifier): ?AuthenticatableInterface + { + $this->retrieveCalls++; + + return new FakeAuthenticatable(id: (int) $identifier); + } + + public function retrieveByCredentials(array $credentials): ?AuthenticatableInterface + { + return null; + } + + public function validateCredentials( + AuthenticatableInterface $user, + array $credentials, + ): bool { + return false; + } + + public function retrieveByRememberToken( + int|string $identifier, + string $token, + ): ?AuthenticatableInterface { + return null; + } + + public function updateRememberToken( + AuthenticatableInterface $user, + ?string $token, + ): void {} + }; + + $guard = new SessionGuard( + session: $session, + provider: $provider, + name: 'web', + ); + + $firstUser = $guard->user(); + $guard->reset(); + $secondUser = $guard->user(); + + expect($secondUser)->not->toBe($firstUser) + ->and($provider->retrieveCalls)->toBe(2) + ->and($session->has('auth_web_user_id'))->toBeTrue(); +}); + +test('it does not return the previous requests user to an anonymous request', function (): void { + $session = new FakeSession(); + $session->set('auth_web_user_id', 42); + $user = new FakeAuthenticatable(id: 42); + $provider = new FakeUserProvider([42 => $user]); + + $guard = new SessionGuard( + session: $session, + provider: $provider, + name: 'web', + ); + + // Request 1: an authenticated user is resolved and cached + expect($guard->user())->toBe($user); + + // Between requests, a long-running worker resets state and the next + // request is an anonymous visitor with no session key present + $guard->reset(); + $session->remove('auth_web_user_id'); + + // Request 2: the anonymous visitor must not receive the previous user + expect($guard->user())->toBeNull(); +}); + test('it defaults to auth_session_user_id when guard name is session', function (): void { $session = new FakeSession(); $session->start(); diff --git a/packages/core/src/Container/Container.php b/packages/core/src/Container/Container.php index 2fbfe94c..9ba9bc31 100644 --- a/packages/core/src/Container/Container.php +++ b/packages/core/src/Container/Container.php @@ -77,6 +77,25 @@ public function instance( $this->instances[$id] = $instance; } + /** + * Instances already resolved, keyed by binding identifier. Never + * triggers resolution — returns only what has already been built. + * Pass an interface to return only instances implementing it. + * + * @return array + */ + public function resolvedInstances(?string $interface = null): array + { + if ($interface === null) { + return $this->instances; + } + + return array_filter( + $this->instances, + fn (object $instance): bool => $instance instanceof $interface, + ); + } + /** * @throws BindingException|CircularDependencyException|ReflectionException|PluginException */ diff --git a/packages/core/src/Contracts/ResettableInterface.php b/packages/core/src/Contracts/ResettableInterface.php new file mode 100644 index 00000000..853d170b --- /dev/null +++ b/packages/core/src/Contracts/ResettableInterface.php @@ -0,0 +1,30 @@ + $container->get(Marko\TestFixtureNoDriver\SomeInterface::class)) + expect(fn () => $container->get(NoDriverSomeInterface::class)) ->toThrow(BindingException::class); }); @@ -749,3 +750,70 @@ public function afterCompute(string $result): string ->and($proxy1)->toBe($proxy2); }); }); + +interface CountingServiceInterface {} + +class CountingService implements CountingServiceInterface {} + +class OtherResolvedService {} + +class InstantiationTrackingService +{ + public static int $constructedCount = 0; + + public function __construct() + { + self::$constructedCount++; + } +} + +describe('resolvedInstances', function (): void { + it('returns instances that have already been resolved', function (): void { + $container = new Container(); + $container->singleton(SimpleClass::class); + $instance = $container->get(SimpleClass::class); + + $resolved = $container->resolvedInstances(); + + expect($resolved)->toHaveKey(SimpleClass::class) + ->and($resolved[SimpleClass::class])->toBe($instance); + }); + + it('does not return bindings that have never been resolved', function (): void { + $container = new Container(); + $container->singleton(SimpleClass::class); + + $resolved = $container->resolvedInstances(); + + expect($resolved)->toBeEmpty(); + }); + + it('does not instantiate anything when called', function (): void { + $container = new Container(); + $container->singleton(InstantiationTrackingService::class); + InstantiationTrackingService::$constructedCount = 0; + + $container->resolvedInstances(); + + expect(InstantiationTrackingService::$constructedCount)->toBe(0); + }); + + it('returns an empty result for a fresh container', function (): void { + $container = new Container(); + + expect($container->resolvedInstances())->toBeEmpty(); + }); + + it('allows filtering resolved instances by implemented interface', function (): void { + $container = new Container(); + $container->singleton(CountingService::class); + $container->singleton(OtherResolvedService::class); + $countingService = $container->get(CountingService::class); + $container->get(OtherResolvedService::class); + + $filtered = $container->resolvedInstances(CountingServiceInterface::class); + + expect($filtered)->toHaveCount(1) + ->and($filtered[CountingService::class])->toBe($countingService); + }); +}); diff --git a/packages/core/tests/Unit/Contracts/ResettableInterfaceTest.php b/packages/core/tests/Unit/Contracts/ResettableInterfaceTest.php new file mode 100644 index 00000000..7c9520f8 --- /dev/null +++ b/packages/core/tests/Unit/Contracts/ResettableInterfaceTest.php @@ -0,0 +1,52 @@ +isInterface()) + ->toBeTrue() + ->and($reflection->hasMethod('reset')) + ->toBeTrue(); +}); + +it('can be implemented by a class that clears its per request state', function (): void { + $service = new class () implements ResettableInterface + { + public string $state = 'request-scoped'; + + public function reset(): void + { + $this->state = ''; + } + }; + + $service->reset(); + + expect($service) + ->toBeInstanceOf(ResettableInterface::class) + ->and($service->state) + ->toBe(''); +}); + +it('documents that reset is non destructive', function (): void { + $classDoc = (new ReflectionClass(ResettableInterface::class))->getDocComment(); + $methodDoc = (new ReflectionClass(ResettableInterface::class))->getMethod('reset')->getDocComment(); + + expect($classDoc) + ->not->toBeFalse() + ->and($classDoc) + ->toContain('non-destructive') + ->and($classDoc) + ->toContain('PHP-FPM') + ->and($methodDoc) + ->not->toBeFalse() + ->and($methodDoc) + ->toContain('not destroy'); +}); diff --git a/packages/cors/src/Middleware/CorsMiddleware.php b/packages/cors/src/Middleware/CorsMiddleware.php index be95ba89..e79760df 100644 --- a/packages/cors/src/Middleware/CorsMiddleware.php +++ b/packages/cors/src/Middleware/CorsMiddleware.php @@ -4,6 +4,7 @@ namespace Marko\Cors\Middleware; +use Marko\Config\Exceptions\ConfigNotFoundException; use Marko\Cors\Config\CorsConfig; use Marko\Cors\Exceptions\CorsException; use Marko\Routing\Http\Request; @@ -13,11 +14,11 @@ readonly class CorsMiddleware implements MiddlewareInterface { public function __construct( - private CorsConfig $config, + private CorsConfig $corsConfig, ) {} /** - * @throws CorsException + * @throws ConfigNotFoundException|CorsException */ public function handle( Request $request, @@ -29,20 +30,20 @@ public function handle( return $next($request); } - if ($this->config->supportsCredentials() && in_array('*', $this->config->allowedOrigins(), true)) { + if ($this->corsConfig->supportsCredentials() && in_array('*', $this->corsConfig->allowedOrigins(), true)) { throw CorsException::wildcardWithCredentials(); } if ($request->method() === 'OPTIONS') { $preflightHeaders = [ 'Access-Control-Allow-Origin' => $origin, - 'Access-Control-Allow-Methods' => implode(', ', $this->config->allowedMethods()), - 'Access-Control-Allow-Headers' => implode(', ', $this->config->allowedHeaders()), + 'Access-Control-Allow-Methods' => implode(', ', $this->corsConfig->allowedMethods()), + 'Access-Control-Allow-Headers' => implode(', ', $this->corsConfig->allowedHeaders()), 'Vary' => 'Origin', ]; - if ($this->config->maxAge() > 0) { - $preflightHeaders['Access-Control-Max-Age'] = (string) $this->config->maxAge(); + if ($this->corsConfig->maxAge() > 0) { + $preflightHeaders['Access-Control-Max-Age'] = (string) $this->corsConfig->maxAge(); } return new Response( @@ -52,28 +53,26 @@ public function handle( ); } + /** @var Response $response */ $response = $next($request); $corsHeaders = [ 'Access-Control-Allow-Origin' => $origin, 'Vary' => 'Origin', ]; - if ($this->config->supportsCredentials()) { + if ($this->corsConfig->supportsCredentials()) { $corsHeaders['Access-Control-Allow-Credentials'] = 'true'; } - $headers = array_merge($response->headers(), $corsHeaders); - - return new Response( - body: $response->body(), - statusCode: $response->statusCode(), - headers: $headers, - ); + return $response->withHeaders($corsHeaders); } + /** + * @throws ConfigNotFoundException + */ private function isOriginAllowed(string $origin): bool { - $allowedOrigins = $this->config->allowedOrigins(); + $allowedOrigins = $this->corsConfig->allowedOrigins(); if (in_array('*', $allowedOrigins, true)) { return true; diff --git a/packages/cors/tests/CorsMiddlewareTest.php b/packages/cors/tests/CorsMiddlewareTest.php index 90378400..82804763 100644 --- a/packages/cors/tests/CorsMiddlewareTest.php +++ b/packages/cors/tests/CorsMiddlewareTest.php @@ -5,19 +5,14 @@ use Marko\Cors\Config\CorsConfig; use Marko\Cors\Exceptions\CorsException; use Marko\Cors\Middleware\CorsMiddleware; +use Marko\Cors\Tests\Helpers; +use Marko\Cors\Tests\TaggedResponse; use Marko\Routing\Http\Request; use Marko\Routing\Http\Response; use Marko\Testing\Fake\FakeConfigRepository; it('adds Access-Control-Allow-Origin header for allowed origins', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['https://example.com'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => false, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig(); $middleware = new CorsMiddleware($config); @@ -35,14 +30,7 @@ }); it('handles preflight OPTIONS requests with 204 No Content response', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['https://example.com'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => false, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig(); $middleware = new CorsMiddleware($config); @@ -70,14 +58,7 @@ }); it('rejects requests from origins not in allowed list', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['https://example.com'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => false, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig(); $middleware = new CorsMiddleware($config); @@ -121,14 +102,7 @@ }); it('supports wildcard star origin matching', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['*'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => false, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig(allowedOrigins: ['*']); $middleware = new CorsMiddleware($config); @@ -146,14 +120,7 @@ }); it('includes Access-Control-Allow-Credentials header when configured', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['https://example.com'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => true, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig(supportsCredentials: true); $middleware = new CorsMiddleware($config); @@ -171,14 +138,7 @@ }); it('sets Access-Control-Max-Age header for preflight caching', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['https://example.com'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => false, - 'cors.max_age' => 3600, - ])); + $config = Helpers::createCorsConfig(maxAge: 3600); $middleware = new CorsMiddleware($config); @@ -197,14 +157,7 @@ }); it('leaves the response unchanged and adds no CORS headers when the Origin is not allowed', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['https://trusted.com'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => false, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig(allowedOrigins: ['https://trusted.com']); $middleware = new CorsMiddleware($config); @@ -223,14 +176,7 @@ }); it('adds a Vary: Origin header on the preflight OPTIONS response when the origin is allowed', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['https://example.com'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => false, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig(); $middleware = new CorsMiddleware($config); @@ -248,14 +194,7 @@ }); it('adds a Vary: Origin header when reflecting an allowed origin on a normal request', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['https://example.com'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => false, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig(); $middleware = new CorsMiddleware($config); @@ -275,14 +214,10 @@ it( 'reflects an explicitly allowed origin and emits Access-Control-Allow-Credentials true when credentials are supported', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['https://trusted.com'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => true, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig( + allowedOrigins: ['https://trusted.com'], + supportsCredentials: true, + ); $middleware = new CorsMiddleware($config); @@ -305,14 +240,7 @@ function (): void { it( 'never emits Access-Control-Allow-Credentials together with a wildcard Access-Control-Allow-Origin', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['*'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => false, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig(allowedOrigins: ['*']); $middleware = new CorsMiddleware($config); @@ -332,14 +260,7 @@ function (): void { ); it('throws a CorsException when allowed origins contain a wildcard and credentials are supported', function (): void { - $config = new CorsConfig(new FakeConfigRepository([ - 'cors.allowed_origins' => ['*'], - 'cors.allowed_methods' => ['GET', 'POST'], - 'cors.allowed_headers' => ['Content-Type'], - 'cors.expose_headers' => [], - 'cors.supports_credentials' => true, - 'cors.max_age' => 0, - ])); + $config = Helpers::createCorsConfig(allowedOrigins: ['*'], supportsCredentials: true); $middleware = new CorsMiddleware($config); @@ -350,6 +271,27 @@ function (): void { $next = fn (Request $req): Response => new Response(body: 'OK'); - expect(fn () => $middleware->handle($request, $next)) + expect(fn (): Response => $middleware->handle($request, $next)) ->toThrow(CorsException::class); }); + +it('preserves the response subclass through the cors package middleware', function (): void { + $config = Helpers::createCorsConfig(); + + $middleware = new CorsMiddleware($config); + + $request = new Request(server: [ + 'REQUEST_METHOD' => 'GET', + 'HTTP_ORIGIN' => 'https://example.com', + ]); + + $next = fn (Request $req): TaggedResponse => Helpers::createTaggedResponse(tag: 'from-controller'); + + $response = $middleware->handle($request, $next); + + /** @var TaggedResponse $response */ + expect($response) + ->toBeInstanceOf(TaggedResponse::class) + ->and($response->tag)->toBe('from-controller') + ->and($response->headers()['Access-Control-Allow-Origin'])->toBe('https://example.com'); +}); diff --git a/packages/cors/tests/Helpers.php b/packages/cors/tests/Helpers.php new file mode 100644 index 00000000..b2a7f83f --- /dev/null +++ b/packages/cors/tests/Helpers.php @@ -0,0 +1,69 @@ + $headers + */ + public function __construct( + public readonly string $tag, + string $body = '', + int $statusCode = 200, + array $headers = [], + ) { + parent::__construct($body, $statusCode, $headers); + } +} + +final class Helpers +{ + /** + * @param array $headers + */ + public static function createTaggedResponse( + string $tag = 'tagged', + string $body = '', + int $statusCode = 200, + array $headers = [], + ): TaggedResponse { + return new TaggedResponse($tag, $body, $statusCode, $headers); + } + + /** + * @param list $allowedOrigins + * @param list $allowedMethods + * @param list $allowedHeaders + * @param list $exposeHeaders + */ + public static function createCorsConfig( + array $allowedOrigins = ['https://example.com'], + array $allowedMethods = ['GET', 'POST'], + array $allowedHeaders = ['Content-Type'], + array $exposeHeaders = [], + bool $supportsCredentials = false, + int $maxAge = 0, + ): CorsConfig { + return new CorsConfig(new FakeConfigRepository([ + 'cors.allowed_origins' => $allowedOrigins, + 'cors.allowed_methods' => $allowedMethods, + 'cors.allowed_headers' => $allowedHeaders, + 'cors.expose_headers' => $exposeHeaders, + 'cors.supports_credentials' => $supportsCredentials, + 'cors.max_age' => $maxAge, + ])); + } +} diff --git a/packages/database-readwrite/src/Connection/ReadWriteConnection.php b/packages/database-readwrite/src/Connection/ReadWriteConnection.php index 87f58735..ba9e8164 100644 --- a/packages/database-readwrite/src/Connection/ReadWriteConnection.php +++ b/packages/database-readwrite/src/Connection/ReadWriteConnection.php @@ -4,15 +4,17 @@ namespace Marko\Database\ReadWrite\Connection; +use Marko\Core\Contracts\ResettableInterface; use Marko\Core\Exceptions\MarkoException; use Marko\Database\Connection\ConnectionInterface; use Marko\Database\Connection\StatementInterface; use Marko\Database\Connection\TransactionInterface; use Marko\Database\ReadWrite\Exceptions\ReadException; use Marko\Database\ReadWrite\Replica\ReplicaSelectorInterface; +use Override; use PDOException; -class ReadWriteConnection implements ConnectionInterface, TransactionInterface +class ReadWriteConnection implements ConnectionInterface, TransactionInterface, ResettableInterface { private bool $stickyWrite = false; @@ -135,6 +137,12 @@ public function resetStickyState(): void $this->stickyWrite = false; } + #[Override] + public function reset(): void + { + $this->resetStickyState(); + } + /** * Detects whether a SQL statement is a write operation (INSERT, UPDATE, DELETE). * diff --git a/packages/database-readwrite/tests/Connection/ReadWriteConnectionTest.php b/packages/database-readwrite/tests/Connection/ReadWriteConnectionTest.php index e559e0b6..7fc1ff25 100644 --- a/packages/database-readwrite/tests/Connection/ReadWriteConnectionTest.php +++ b/packages/database-readwrite/tests/Connection/ReadWriteConnectionTest.php @@ -2,6 +2,7 @@ declare(strict_types=1); +use Marko\Core\Contracts\ResettableInterface; use Marko\Database\Connection\ConnectionInterface; use Marko\Database\Connection\StatementInterface; use Marko\Database\Connection\TransactionInterface; @@ -525,6 +526,57 @@ public function select(array $replicas): ConnectionInterface ->and($reflection->isPublic())->toBeTrue(); }); + it('implements the resettable contract', function (): void { + $write = makeConnection(); + $replica = makeConnection(); + $selector = makeSelector($replica); + + $conn = new ReadWriteConnection($write, [$replica], $selector); + + expect($conn)->toBeInstanceOf(ResettableInterface::class); + }); + + it('clears sticky write state when reset', function (): void { + $write = makeConnection(['query' => [['id' => 1]]]); + $replica = makeConnection(['query' => [['id' => 99]]]); + $selector = makeSelector($replica); + + $conn = new ReadWriteConnection($write, [$replica], $selector); + $conn->execute('INSERT INTO foo VALUES (1)'); + $conn->reset(); + $result = $conn->query('SELECT 1'); + + expect($result)->toBe([['id' => 99]]); + }); + + it('routes reads to a replica again after reset', function (): void { + $write = makeConnection(['query' => [['id' => 1]]]); + $replica = makeConnection(['query' => [['id' => 99]]]); + $selector = makeSelector($replica); + + $conn = new ReadWriteConnection($write, [$replica], $selector); + $conn->beginTransaction(); + $conn->reset(); + $result = $conn->query('SELECT 1'); + + expect($result)->toBe([['id' => 99]]) + ->and($replica->calls)->toContain(['query', 'SELECT 1', []]) + ->and($write->calls)->not->toContain(['query', 'SELECT 1', []]); + }); + + it('keeps the existing reset sticky state method available', function (): void { + $write = makeConnection(['query' => [['id' => 1]]]); + $replica = makeConnection(['query' => [['id' => 99]]]); + $selector = makeSelector($replica); + + $conn = new ReadWriteConnection($write, [$replica], $selector); + $conn->execute('INSERT INTO foo VALUES (1)'); + $conn->resetStickyState(); + $result = $conn->query('SELECT 1'); + + expect($result)->toBe([['id' => 99]]); + }); + it('tries the next replica when first replica throws PDOException', function (): void { $write = makeConnection(); $failing = makeThrowingConnection('replica1 down'); @@ -631,9 +683,8 @@ public function transaction(callable $callback): mixed $conn = new ReadWriteConnection($write, [$badQuery, $good], $selector); expect(fn () => $conn->query('INVALID SQL')) - ->toThrow(InvalidArgumentException::class, 'SQL syntax error'); - - expect($badQuery->calls)->toContain(['query', 'INVALID SQL', []]) + ->toThrow(InvalidArgumentException::class, 'SQL syntax error') + ->and($badQuery->calls)->toContain(['query', 'INVALID SQL', []]) ->and($good->calls)->toBeEmpty(); }); diff --git a/packages/docs-markdown/docs/packages/authentication.md b/packages/docs-markdown/docs/packages/authentication.md index 1ba8eacd..730f21c7 100644 --- a/packages/docs-markdown/docs/packages/authentication.md +++ b/packages/docs-markdown/docs/packages/authentication.md @@ -183,6 +183,18 @@ if ($guard->check()) { } ``` +`SessionGuard` also implements `Marko\Core\Contracts\ResettableInterface`. In a long-running worker (e.g. Swoole, RoadRunner), call `reset()` between requests to clear the guard's cached user so one request's authenticated user is never served to the next: + +```php +use Marko\Core\Contracts\ResettableInterface; + +if ($guard instanceof ResettableInterface) { + $guard->reset(); +} +``` + +`reset()` only clears the cached user --- it does not call `logout()` or otherwise touch the session. The next call to `user()` re-reads the authenticated user from the session as normal. + ### TokenGuard For API authentication via Bearer tokens in the `Authorization` header: diff --git a/packages/docs-markdown/docs/packages/core.md b/packages/docs-markdown/docs/packages/core.md index e0ae9489..9334ece5 100644 --- a/packages/docs-markdown/docs/packages/core.md +++ b/packages/docs-markdown/docs/packages/core.md @@ -166,6 +166,47 @@ return [ ]; ``` +### Resetting Request-Scoped State in Long-Running Processes + +PHP-FPM ends the process after every request, so any state a singleton accumulates disappears automatically. A long-running process --- a worker or event loop that reuses one PHP process across many requests --- has no such reset, so a singleton that caches per-request state (the current session, the authenticated user, a sticky database routing flag) leaks across requests once the process picks up a different user. + +Implement `ResettableInterface` on any singleton that holds this kind of state: + +```php +use Marko\Core\Contracts\ResettableInterface; + +class RequestScopedCache implements ResettableInterface +{ + private array $entries = []; + + public function remember(string $key, mixed $value): void + { + $this->entries[$key] = $value; + } + + public function reset(): void + { + $this->entries = []; + } +} +``` + +`reset()` must be non-destructive --- it clears the instance's in-memory tracking, not anything persisted. Resetting a session service forgets which session the instance was serving; it does not delete the stored session. + +A long-running process discovers what to reset via `Container::resolvedInstances()`, which returns only instances the container has already built --- never triggering resolution --- optionally filtered to those implementing an interface: + +```php +use Marko\Core\Contracts\ResettableInterface; + +foreach ($container->resolvedInstances(ResettableInterface::class) as $resettable) { + $resettable->reset(); +} +``` + +`resolvedInstances()` is declared on the concrete `Container` class, not on `ContainerInterface` --- code that needs it must type-hint `Container` or check `instanceof Container` rather than relying on the interface. + +Current implementors: `Session` ([marko/session](/docs/packages/session/)), `SessionGuard` ([marko/authentication](/docs/packages/authentication/)), and `ReadWriteConnection` ([marko/database-readwrite](/docs/packages/database-readwrite/)). + ### Discovery Cache On every boot, Marko scans all module PHP files to discover `#[Preference]`, `#[Plugin]`, `#[Observer]`, and `#[Command]` attributes. In production this scan can be eliminated by compiling its results into a single PHP file --- the discovery cache. @@ -296,6 +337,19 @@ interface ContainerInterface extends PsrContainerInterface } ``` +The concrete `Container` class additionally provides `resolvedInstances(?string $interface = null): array` --- not part of `ContainerInterface`. It returns only instances already built, optionally filtered to those implementing `$interface`, and never triggers resolution as a side effect. See [Resetting Request-Scoped State](#resetting-request-scoped-state-in-long-running-processes) above. + +### Contracts + +```php +interface ResettableInterface +{ + public function reset(): void; +} +``` + +Implemented by services that hold request-scoped state which must be cleared between requests in a long-running process. See [Resetting Request-Scoped State](#resetting-request-scoped-state-in-long-running-processes) above. + ### Events ```php diff --git a/packages/docs-markdown/docs/packages/database-readwrite.md b/packages/docs-markdown/docs/packages/database-readwrite.md index 97bc84b0..262134c3 100644 --- a/packages/docs-markdown/docs/packages/database-readwrite.md +++ b/packages/docs-markdown/docs/packages/database-readwrite.md @@ -181,6 +181,8 @@ Sticky writes (via `execute()` or `beginTransaction()`) bypass all replicas enti In PHP-FPM the sticky flag is cleared automatically at the end of each request because each request is a new process. In a queue worker or other long-running process the sticky flag persists for the lifetime of the process. Call `resetStickyState()` between jobs to restore replica routing: +`ReadWriteConnection` also implements `Marko\Core\Contracts\ResettableInterface`, so a worker that resets every registered `ResettableInterface` implementation between requests will clear the sticky flag automatically via `reset()`, which delegates to `resetStickyState()`. Calling `resetStickyState()` directly remains supported for callers that don't go through the contract. + ```php use Marko\Database\ReadWrite\Connection\ReadWriteConnection; use Marko\Database\Connection\ConnectionInterface; @@ -236,7 +238,7 @@ Your `CustomReadWriteConnection` must extend `ReadWriteConnection` (or independe ### ReadWriteConnection -Implements `ConnectionInterface` and `TransactionInterface`. Routes reads to replicas and writes to the primary. +Implements `ConnectionInterface`, `TransactionInterface`, and `ResettableInterface`. Routes reads to replicas and writes to the primary. | Method | Routes To | Description | |--------|-----------|-------------| @@ -254,6 +256,7 @@ Implements `ConnectionInterface` and `TransactionInterface`. Routes reads to rep | `transaction(callable $callback): mixed` | Write (sets sticky temporarily) | Run a callback inside an auto-managed transaction; sticky flag is set for the callback duration and cleared on completion | | `driverName(): string` | Write (delegates) | Return the write connection's driver name (e.g. `'mysql'`, `'pgsql'`) | | `resetStickyState(): void` | — | Clear the sticky flag; subsequent reads route to replicas again | +| `reset(): void` | — | `ResettableInterface` contract method; delegates to `resetStickyState()` | ### ReadException diff --git a/packages/docs-markdown/docs/packages/page-cache.md b/packages/docs-markdown/docs/packages/page-cache.md index baca7814..42379929 100644 --- a/packages/docs-markdown/docs/packages/page-cache.md +++ b/packages/docs-markdown/docs/packages/page-cache.md @@ -44,9 +44,9 @@ class ProductController `PageCacheMiddleware` is automatically registered as global middleware. On the first request the response is served from the controller and stored. Subsequent requests return the stored response without executing the controller. -### Known Limitation +### Cookies Are Never Cached -Responses with a `Set-Cookie` header are never cached in v1. This includes responses that set analytics or session cookies --- if your response sets any cookie, it bypasses the cache entirely. +Responses carrying any cookie --- whether attached via `Response::withCookie()` or set directly with a raw `Set-Cookie` header --- are never cached. This is a deliberate security boundary, not a limitation to work around: a cached `Set-Cookie` would be replayed to every later visitor, leaking one user's session (or any other cookie) to everybody else. This includes responses that set analytics or session cookies --- if your response carries any cookie, it bypasses the cache entirely. ### Extending Cacheability Rules diff --git a/packages/docs-markdown/docs/packages/routing.md b/packages/docs-markdown/docs/packages/routing.md index c5b2c8ca..fda51fe8 100644 --- a/packages/docs-markdown/docs/packages/routing.md +++ b/packages/docs-markdown/docs/packages/routing.md @@ -95,6 +95,63 @@ class AuthMiddleware implements MiddlewareInterface } ``` +### Decorating Responses + +Middleware that adds headers, cookies, or changes the status code should decorate the response returned by `$next()` rather than construct a new one: + +```php title="SecurityHeadersMiddleware.php" +use Marko\Routing\Http\Request; +use Marko\Routing\Http\Response; +use Marko\Routing\Middleware\MiddlewareInterface; + +class SecurityHeadersMiddleware implements MiddlewareInterface +{ + public function handle( + Request $request, + callable $next, + ): Response { + $response = $next($request); + + return $response->withHeader('X-Frame-Options', 'DENY'); + } +} +``` + +`withHeader()`, `withHeaders()`, `withStatus()`, and `withCookie()` each return a clone of the response, preserving its concrete class. Rebuilding a response instead, e.g. `new Response($response->body(), $response->statusCode(), $headers)`, silently discards subclass identity --- a `StreamingResponse` returned by an SSE endpoint would be downgraded to a plain `Response` and its stream would never send. Always decorate, never rebuild. + +### Setting Cookies + +Attach a cookie to a response with `withCookie()`: + +```php +use Marko\Routing\Attributes\Get; +use Marko\Routing\Http\Cookie; +use Marko\Routing\Http\Response; + +#[Get('/login')] +public function login(): Response +{ + return Response::json(['ok' => true]) + ->withCookie(new Cookie( + name: 'session', + value: $sessionId, + expires: time() + 3600, + path: '/', + secure: true, + httpOnly: true, + sameSite: 'Lax', + )); +} +``` + +`expires: null` or `expires: 0` omits the `Expires` attribute entirely, producing a browser-session cookie instead of a persistent one. Cookie values are `rawurlencode()`d automatically, so a value containing `;` cannot inject a second attribute. An invalid cookie name throws `CookieException`, as does `sameSite: 'None'` without `secure: true` --- browsers silently drop such cookies, so Marko fails loudly instead. + +Read cookies sent by the client with `Request::cookie()`, which mirrors `query()` and `post()`: + +```php +$sessionId = $request->cookie('session'); +``` + ### Overriding Vendor Routes Use [Preferences](/docs/packages/core/) to replace a vendor's controller: @@ -201,6 +258,7 @@ class Request public function path(): string; public function query(?string $key = null, mixed $default = null): mixed; public function post(?string $key = null, mixed $default = null): mixed; + public function cookie(?string $key = null, mixed $default = null): mixed; public function body(): string; public function header(string $name, ?string $default = null): ?string; public function headers(): array; @@ -213,7 +271,7 @@ class Request } ``` -`ip()` returns `REMOTE_ADDR` from the server bag (equivalent to `server('REMOTE_ADDR')`). `withRoute()` returns a new immutable `Request` with the matched controller class and action method attached; `controller()` and `action()` retrieve them. The router attaches route context before invoking middleware, which allows middleware (such as `AdminAuthMiddleware`) to inspect which controller method is handling the request. +`ip()` returns `REMOTE_ADDR` from the server bag (equivalent to `server('REMOTE_ADDR')`). `cookie()` reads from the request's `$_COOKIE` bag and mirrors the signature of `query()` and `post()`. `withRoute()` returns a new immutable `Request` with the matched controller class and action method attached; `controller()` and `action()` retrieve them. The router attaches route context before invoking middleware, which allows middleware (such as `AdminAuthMiddleware`) to inspect which controller method is handling the request. ### Response @@ -229,13 +287,47 @@ class Response public function body(): string; public function statusCode(): int; public function headers(): array; + public function cookies(): array; + public function headerLines(): array; public function send(): void; public static function json(mixed $data, int $statusCode = 200): self; public static function html(string $html, int $statusCode = 200): self; public static function redirect(string $url, int $statusCode = 302): self; + public function withHeader(string $name, string $value): static; + public function withHeaders(array $headers): static; + public function withStatus(int $statusCode): static; + public function withCookie(Cookie $cookie): static; } ``` +`cookies()` returns the `Cookie` instances attached to the response; `headerLines()` returns the raw `"Name: value"` lines followed by one `Set-Cookie:` line per cookie, without making any SAPI calls --- `send()` uses it internally, and it's also useful for testing. `withHeader()` and `withHeaders()` merge into the existing headers (`withHeaders()` merges its argument over the current set). `withCookie()` replaces an existing cookie that matches on `(name, path, domain)`, or appends a new one otherwise. Every `with*()` method is marked `#[\NoDiscard]` and returns a clone via PHP's `clone` operator rather than `new static(...)`, so a `Response` subclass such as `StreamingResponse` survives decoration intact --- see [Decorating Responses](#decorating-responses). `Response` is deliberately not a `readonly class` for this reason: immutability is enforced by API design (private properties, no setters) rather than the `readonly` keyword. + +`json()`, `html()`, and `redirect()` construct with `new self()`, not `new static()`, since a subclass such as `StreamingResponse` has a different constructor signature and cannot accept `(body:, statusCode:, headers:)`. + +### Cookie + +```php +use Marko\Routing\Http\Cookie; + +public function __construct( + string $name, + string $value = '', + ?int $expires = null, + ?string $path = null, + ?string $domain = null, + bool $secure = false, + bool $httpOnly = false, + ?string $sameSite = null, +) + +public function name(): string; +public function path(): ?string; +public function domain(): ?string; +public function toSetCookieString(): string; +``` + +`expires: null` or `expires: 0` omits the `Expires` attribute, producing a browser-session cookie. The value passed to `toSetCookieString()` is `rawurlencode()`d. The constructor throws `CookieException` for an invalid cookie name (control characters, whitespace, or separator characters such as `( ) < > @ , ; : \ " / [ ] ? = { }`), and also throws when `sameSite` is `'None'` without `secure: true`. + ### MiddlewareInterface ```php diff --git a/packages/docs-markdown/docs/packages/session.md b/packages/docs-markdown/docs/packages/session.md index 7b012c40..15e2d77d 100644 --- a/packages/docs-markdown/docs/packages/session.md +++ b/packages/docs-markdown/docs/packages/session.md @@ -130,6 +130,25 @@ $id = $this->session->getId(); The `SessionMiddleware` automatically starts the session at the beginning of a request and saves it when the response completes. It is registered globally by the session driver package (e.g., `marko/session-file`, `marko/session-database`) --- no manual registration is needed. Your controllers only need to inject `SessionInterface`; `start()` and `save()` are handled automatically. +The session cookie is attached to the `Response` rather than emitted directly by PHP --- `Session::configure()` disables PHP's built-in cookie handling, so `SessionMiddleware` reads the inbound cookie off the `Request`, seeds the session ID before `start()`, and attaches an outbound cookie only when the session ID changed (a new session, a regenerated ID, or an expired cookie after `destroy()`). A repeat visitor whose session ID is unchanged gets no `Set-Cookie` header. An invalid or tampered inbound cookie is ignored --- the middleware falls through to a fresh session rather than raising an error. + +This matters for [`marko/page-cache`](/docs/packages/page-cache/): responses carrying any cookie are never cached, so attaching the session cookie unconditionally would silently disable page caching on every session-enabled route. + +### Long-Running Processes + +`Session` implements `Marko\Core\Contracts\ResettableInterface`. In a long-running worker (e.g. Swoole, RoadRunner), call `reset()` between requests to clear the cached session ID, data, and flash bag so one request's session state is never reused for the next: + +```php +use Marko\Core\Contracts\ResettableInterface; +use Marko\Session\Contracts\SessionInterface; + +if ($this->session instanceof ResettableInterface) { + $this->session->reset(); +} +``` + +`save()` does not clear session state on its own --- `SessionMiddleware` reads `getId()` after `save()` runs to decide whether to attach a cookie, so clearing happens explicitly via `reset()` instead. + ### Garbage Collection Run expired session cleanup via CLI: diff --git a/packages/docs-markdown/docs/packages/sse.md b/packages/docs-markdown/docs/packages/sse.md index 7aa73bd0..6a73f7a8 100644 --- a/packages/docs-markdown/docs/packages/sse.md +++ b/packages/docs-markdown/docs/packages/sse.md @@ -128,6 +128,8 @@ return new StreamingResponse($stream); **Reconnection:** When the browser reconnects after a disconnect, it sends a `Last-Event-ID` header containing the last event ID it received. Read it with `$request->header('Last-Event-ID')` and pass it to your data source to resume from where the stream left off. +**Middleware compatibility:** Any middleware in front of an SSE route must [decorate the response rather than rebuild it](/docs/packages/routing/#decorating-responses) --- for example `$response->withHeader(...)` instead of `new Response($response->body(), ...)`. Rebuilding discards the concrete class, so a `StreamingResponse` would be silently downgraded to a plain `Response` and the stream would never send. + ## API Reference ### SseEvent diff --git a/packages/inertia/src/Middleware/InertiaMiddleware.php b/packages/inertia/src/Middleware/InertiaMiddleware.php index 85718bce..98618dc6 100644 --- a/packages/inertia/src/Middleware/InertiaMiddleware.php +++ b/packages/inertia/src/Middleware/InertiaMiddleware.php @@ -52,20 +52,12 @@ public function handle( $statusCode = 303; } - return new Response( - body: $response->body(), - statusCode: $statusCode, - headers: $headers, - ); + return $response->withHeaders($headers)->withStatus($statusCode); } $headers['X-Inertia'] = 'true'; - return new Response( - body: $response->body(), - statusCode: $response->statusCode(), - headers: $headers, - ); + return $response->withHeaders($headers); } /** diff --git a/packages/inertia/tests/Helpers.php b/packages/inertia/tests/Helpers.php new file mode 100644 index 00000000..ad48376e --- /dev/null +++ b/packages/inertia/tests/Helpers.php @@ -0,0 +1,34 @@ +config = new FakeConfigRepository([ +use function Marko\Inertia\Tests\createTaggedResponse; + +beforeEach(function (): void { + $this->middleware = new InertiaMiddleware(new FakeConfigRepository([ 'inertia.version' => '1.0', - ]); - $this->middleware = new InertiaMiddleware($this->config); + ])); }); -test('middleware passes through non-inertia requests unchanged', function () { +test('middleware passes through non-inertia requests unchanged', function (): void { $request = new Request(); $originalResponse = new Response(body: 'OK'); - $response = $this->middleware->handle($request, fn () => $originalResponse); + $response = $this->middleware->handle($request, fn (): Response => $originalResponse); expect($response->body())->toBe('OK') ->and($response->headers())->not->toHaveKey('X-Inertia'); }); -test('middleware adds inertia headers for inertia requests', function () { +test('middleware adds inertia headers for inertia requests', function (): void { $request = new Request(server: ['HTTP_X_INERTIA' => 'true']); $originalResponse = new Response(body: '{}', headers: ['Content-Type' => 'application/json']); - $response = $this->middleware->handle($request, fn () => $originalResponse); + $response = $this->middleware->handle($request, fn (): Response => $originalResponse); expect($response->headers()['X-Inertia'])->toBe('true') ->and($response->headers()['Vary'])->toBe('X-Inertia'); }); -test('middleware leaves redirects unchanged for inertia requests', function () { +test('middleware leaves redirects unchanged for inertia requests', function (): void { $request = new Request(server: ['HTTP_X_INERTIA' => 'true']); $originalResponse = Response::redirect('/other'); - $response = $this->middleware->handle($request, fn () => $originalResponse); + $response = $this->middleware->handle($request, fn (): Response => $originalResponse); expect($response->statusCode())->toBe(302) ->and($response->headers()['Location'])->toBe('/other') @@ -47,21 +49,21 @@ ->and($response->headers())->not->toHaveKey('X-Inertia-Location'); }); -test('middleware upgrades non-get inertia redirects to 303', function () { +test('middleware upgrades non-get inertia redirects to 303', function (): void { $request = new Request(server: [ 'REQUEST_METHOD' => 'PATCH', 'HTTP_X_INERTIA' => 'true', ]); $originalResponse = Response::redirect('/updated'); - $response = $this->middleware->handle($request, fn () => $originalResponse); + $response = $this->middleware->handle($request, fn (): Response => $originalResponse); expect($response->statusCode())->toBe(303) ->and($response->headers()['Location'])->toBe('/updated') ->and($response->headers()['Vary'])->toBe('X-Inertia'); }); -test('middleware returns 409 on version mismatch', function () { +test('middleware returns 409 on version mismatch', function (): void { $request = new Request(server: [ 'REQUEST_METHOD' => 'GET', 'HTTP_X_INERTIA' => 'true', @@ -69,7 +71,7 @@ ]); $originalResponse = new Response(body: '{}'); - $response = $this->middleware->handle($request, fn () => $originalResponse); + $response = $this->middleware->handle($request, fn (): Response => $originalResponse); expect($response->statusCode())->toBe(409) ->and($response->headers()['X-Inertia-Location'])->toBe('/'); @@ -83,7 +85,7 @@ 'HTTP_X_INERTIA_VERSION' => '0.9', ]); - $response = $this->middleware->handle($request, fn () => new Response(body: '{}')); + $response = $this->middleware->handle($request, fn (): Response => new Response(body: '{}')); expect($response->statusCode())->toBe(409) ->and($response->headers()['X-Inertia-Location'])->toBe('/users?page=2'); @@ -96,7 +98,7 @@ 'HTTP_X_INERTIA_VERSION' => '0.9', ]); - $response = $this->middleware->handle($request, fn () => new Response(body: '{}')); + $response = $this->middleware->handle($request, fn (): Response => new Response(body: '{}')); expect($response->statusCode())->toBe(409); }); @@ -121,7 +123,7 @@ ->and($controllerWasCalled)->toBeFalse(); }); -test('middleware does not return 409 for non-get version mismatches', function () { +test('middleware does not return 409 for non-get version mismatches', function (): void { $request = new Request(server: [ 'REQUEST_METHOD' => 'POST', 'HTTP_X_INERTIA' => 'true', @@ -129,22 +131,59 @@ ]); $originalResponse = new Response(body: '{}'); - $response = $this->middleware->handle($request, fn () => $originalResponse); + $response = $this->middleware->handle($request, fn (): Response => $originalResponse); expect($response->statusCode())->toBe(200) ->and($response->headers()['X-Inertia'])->toBe('true'); }); -test('middleware throws a loud exception for invalid version config', function () { +test('middleware throws a loud exception for invalid version config', function (): void { $middleware = new InertiaMiddleware(new FakeConfigRepository([ 'inertia.version' => ['invalid'], ])); $request = new Request(server: ['HTTP_X_INERTIA' => 'true']); - expect(fn () => $middleware->handle($request, fn () => new Response(body: '{}'))) + expect(fn (): Response => $middleware->handle($request, fn (): Response => new Response(body: '{}'))) ->toThrow( InertiaConfigurationException::class, 'Inertia configuration key "inertia.version" must be a string, number, or null.', ); }); + +it('preserves the response subclass through inertia middleware', function (): void { + $request = new Request(server: ['HTTP_X_INERTIA' => 'true']); + $originalResponse = createTaggedResponse(tag: 'from-controller', body: '{}'); + + $response = $this->middleware->handle($request, fn (): TaggedResponse => $originalResponse); + + /** @var TaggedResponse $response */ + expect($response) + ->toBeInstanceOf(TaggedResponse::class) + ->and($response->tag)->toBe('from-controller') + ->and($response->headers()['X-Inertia'])->toBe('true'); +}); + +it( + 'preserves the response subclass through the inertia redirect branch while upgrading 302 to 303', + function (): void { + $request = new Request(server: [ + 'REQUEST_METHOD' => 'PATCH', + 'HTTP_X_INERTIA' => 'true', + ]); + $originalResponse = createTaggedResponse( + tag: 'from-controller', + statusCode: 302, + headers: ['Location' => '/updated'], + ); + + $response = $this->middleware->handle($request, fn (): TaggedResponse => $originalResponse); + + /** @var TaggedResponse $response */ + expect($response) + ->toBeInstanceOf(TaggedResponse::class) + ->and($response->tag)->toBe('from-controller') + ->and($response->statusCode())->toBe(303) + ->and($response->headers()['Location'])->toBe('/updated'); + }, +); diff --git a/packages/page-cache-file/tests/Unit/Driver/FilePageCacheDriverTest.php b/packages/page-cache-file/tests/Unit/Driver/FilePageCacheDriverTest.php index df81b228..b735273f 100644 --- a/packages/page-cache-file/tests/Unit/Driver/FilePageCacheDriverTest.php +++ b/packages/page-cache-file/tests/Unit/Driver/FilePageCacheDriverTest.php @@ -228,6 +228,44 @@ function (): void { ->and($result->headers())->toBe(['X-Test' => 'value']); }); +it('hydrates a cached response with no cookies', function (): void { + $request = createTestRequest('GET', '/no-cookies'); + $response = new Response(body: 'no cookies here', statusCode: 200); + $policy = new CachePolicy(ttl: 3600, tags: []); + + $this->driver->store($request, $response, $policy); + $result = $this->driver->lookup($request); + + expect($result)->toBeInstanceOf(Response::class) + ->and($result->cookies())->toBeEmpty(); +}); + +it('hydrates a cache entry written before cookies existed without error', function (): void { + $request = createTestRequest('GET', '/legacy'); + $key = CacheKey::fromRequest($request); + $pagesDir = $this->tmpDir . '/pages'; + + if (!is_dir($pagesDir)) { + mkdir($pagesDir, 0755, true); + } + + $legacyPayload = serialize([ + 'status_code' => 200, + 'body' => 'legacy body', + 'headers' => ['X-Legacy' => 'yes'], + 'expires_at' => time() + 9999, + 'created_at' => time(), + ]); + + file_put_contents($pagesDir . '/' . $key->hash() . '.cache', $legacyPayload); + + $result = $this->driver->lookup($request); + + expect($result)->toBeInstanceOf(Response::class) + ->and($result->body())->toBe('legacy body') + ->and($result->cookies())->toBeEmpty(); +}); + it('does not instantiate a disallowed class when decoding a tampered page-cache payload', function (): void { $request = createTestRequest('GET', '/tampered'); $key = CacheKey::fromRequest($request); diff --git a/packages/page-cache/src/CacheabilityChecker.php b/packages/page-cache/src/CacheabilityChecker.php index ae626f54..6cb222f2 100644 --- a/packages/page-cache/src/CacheabilityChecker.php +++ b/packages/page-cache/src/CacheabilityChecker.php @@ -37,6 +37,10 @@ public function isResponseCacheable(Response $response): bool return false; } + if ($response->cookies() !== []) { + return false; + } + if ($this->getHeader($response, 'set-cookie') !== null) { return false; } @@ -93,12 +97,9 @@ private function getHeader( ): ?string { $name = strtolower($name); - foreach ($response->headers() as $key => $value) { - if (strtolower($key) === $name) { - return $value; - } - } - - return null; + return array_find( + $response->headers(), + fn (string $value, string $key): bool => strtolower($key) === $name, + ); } } diff --git a/packages/page-cache/tests/Unit/CacheabilityCheckerTest.php b/packages/page-cache/tests/Unit/CacheabilityCheckerTest.php index c149489b..9174e06b 100644 --- a/packages/page-cache/tests/Unit/CacheabilityCheckerTest.php +++ b/packages/page-cache/tests/Unit/CacheabilityCheckerTest.php @@ -7,6 +7,7 @@ use Marko\PageCache\Attributes\Cacheable; use Marko\PageCache\CacheabilityChecker; use Marko\PageCache\Config\PageCacheConfig; +use Marko\Routing\Http\Cookie; use Marko\Routing\Http\Request; use Marko\Routing\Http\Response; use Marko\Routing\MatchedRoute; @@ -230,6 +231,27 @@ function (): void { }, ); +it('does not cache a response that carries cookies', function (): void { + $checker = makeChecker(makeNullMatcher()); + $response = makeCacheCheckerResponse(200)->withCookie(new Cookie(name: 'session', value: 'abc123')); + + expect($checker->isResponseCacheable($response))->toBeFalse(); +}); + +it('still caches a response that carries no cookies', function (): void { + $checker = makeChecker(makeNullMatcher()); + $response = makeCacheCheckerResponse(200); + + expect($checker->isResponseCacheable($response))->toBeTrue(); +}); + +it('still refuses to cache a response carrying a literal set-cookie header', function (): void { + $checker = makeChecker(makeNullMatcher()); + $response = makeCacheCheckerResponse(200, ['Set-Cookie' => 'session=abc123']); + + expect($checker->isResponseCacheable($response))->toBeFalse(); +}); + // ─── getRouteAttribute ──────────────────────────────────────────────────────── it('returns the Cacheable attribute when the matched route declares it', function (): void { diff --git a/packages/ratelimiter/src/Middleware/RateLimitMiddleware.php b/packages/ratelimiter/src/Middleware/RateLimitMiddleware.php index 20a08aa7..b4edd6e9 100644 --- a/packages/ratelimiter/src/Middleware/RateLimitMiddleware.php +++ b/packages/ratelimiter/src/Middleware/RateLimitMiddleware.php @@ -48,13 +48,9 @@ public function handle( /** @var Response $response */ $response = $next($request); - return new Response( - body: $response->body(), - statusCode: $response->statusCode(), - headers: array_merge($response->headers(), [ - 'X-RateLimit-Limit' => (string) $this->maxAttempts, - 'X-RateLimit-Remaining' => (string) $result->remaining(), - ]), - ); + return $response->withHeaders([ + 'X-RateLimit-Limit' => (string) $this->maxAttempts, + 'X-RateLimit-Remaining' => (string) $result->remaining(), + ]); } } diff --git a/packages/ratelimiter/tests/Helpers.php b/packages/ratelimiter/tests/Helpers.php new file mode 100644 index 00000000..8a7da343 --- /dev/null +++ b/packages/ratelimiter/tests/Helpers.php @@ -0,0 +1,34 @@ + '192.168.1.1']); - $next = fn (Request $r) => new Response('OK', 200); + $next = fn (Request $r): Response => new Response('OK', 200); $response = $middleware->handle($request, $next); @@ -88,7 +91,7 @@ function createMiddlewareResolver(array $trustedProxies = []): ClientIpResolver $nextCalled = false; $middleware = new RateLimitMiddleware($limiter, createMiddlewareResolver()); $request = new Request(server: ['REMOTE_ADDR' => '10.0.0.1']); - $next = function (Request $r) use (&$nextCalled) { + $next = function (Request $r) use (&$nextCalled): Response { $nextCalled = true; return new Response('OK'); @@ -109,7 +112,7 @@ function createMiddlewareResolver(array $trustedProxies = []): ClientIpResolver $nextCalled = false; $middleware = new RateLimitMiddleware($limiter, createMiddlewareResolver()); $request = new Request(server: ['REMOTE_ADDR' => '10.0.0.1']); - $next = function (Request $r) use (&$nextCalled) { + $next = function (Request $r) use (&$nextCalled): Response { $nextCalled = true; return new Response('OK'); @@ -134,7 +137,7 @@ function createMiddlewareResolver(array $trustedProxies = []): ClientIpResolver maxAttempts: 100, ); $request = new Request(server: ['REMOTE_ADDR' => '10.0.0.1']); - $next = fn (Request $r) => new Response('OK'); + $next = fn (Request $r): Response => new Response('OK'); $response = $middleware->handle($request, $next); @@ -155,7 +158,7 @@ function createMiddlewareResolver(array $trustedProxies = []): ClientIpResolver $middleware = new RateLimitMiddleware($limiter, createMiddlewareResolver()); $request = new Request(server: ['REMOTE_ADDR' => '10.0.0.1']); - $next = fn (Request $r) => new Response('OK'); + $next = fn (Request $r): Response => new Response('OK'); $response = $middleware->handle($request, $next); @@ -202,7 +205,7 @@ public function clear( $middleware = new RateLimitMiddleware($limiter, createMiddlewareResolver()); $request = new Request(server: ['REMOTE_ADDR' => '203.0.113.50']); - $next = fn (Request $r) => new Response('OK'); + $next = fn (Request $r): Response => new Response('OK'); $middleware->handle($request, $next); @@ -246,7 +249,7 @@ public function clear( $middleware = new RateLimitMiddleware($limiter, createMiddlewareResolver()); $request = new Request(server: ['REMOTE_ADDR' => '10.0.0.99']); - $next = fn (Request $r) => new Response('OK'); + $next = fn (Request $r): Response => new Response('OK'); $middleware->handle($request, $next); @@ -261,9 +264,28 @@ public function clear( $middleware = new RateLimitMiddleware($limiter, createMiddlewareResolver()); $request = new Request(); - $next = fn (Request $r) => new Response('OK'); + $next = fn (Request $r): Response => new Response('OK'); - expect(fn () => $middleware->handle($request, $next)) + expect(fn (): Response => $middleware->handle($request, $next)) ->toThrow(ClientIpException::class); }); + + it('preserves the response subclass through rate limit middleware', function (): void { + $limiter = createMockLimiter(new RateLimitResult( + allowed: true, + remaining: 42, + )); + + $middleware = new RateLimitMiddleware($limiter, createMiddlewareResolver()); + $request = new Request(server: ['REMOTE_ADDR' => '10.0.0.1']); + $next = fn (Request $r): TaggedResponse => createTaggedResponse(tag: 'from-controller'); + + $response = $middleware->handle($request, $next); + + /** @var TaggedResponse $response */ + expect($response) + ->toBeInstanceOf(TaggedResponse::class) + ->and($response->tag)->toBe('from-controller') + ->and($response->headers())->toHaveKey('X-RateLimit-Limit'); + }); }); diff --git a/packages/routing/src/Exceptions/CookieException.php b/packages/routing/src/Exceptions/CookieException.php new file mode 100644 index 00000000..7d6bd5b2 --- /dev/null +++ b/packages/routing/src/Exceptions/CookieException.php @@ -0,0 +1,30 @@ + @ , ; : \\ " / [ ] ? = { }', + suggestion: 'Use a token-safe cookie name, e.g. letters, digits, and characters like - _ . ~', + ); + } + + public static function sameSiteNoneRequiresSecure( + string $name, + ): self { + return new self( + message: "Cookie '$name' uses SameSite=None but is not marked Secure", + context: 'Browsers silently drop a SameSite=None cookie that is not sent with the Secure attribute', + suggestion: "Set secure: true when using sameSite: 'None'", + ); + } +} diff --git a/packages/routing/src/Http/Cookie.php b/packages/routing/src/Http/Cookie.php new file mode 100644 index 00000000..fadbbb11 --- /dev/null +++ b/packages/routing/src/Http/Cookie.php @@ -0,0 +1,80 @@ +@,;:\\\\"\/\[\]?={}]/'; + + /** + * @throws CookieException + */ + public function __construct( + private string $name, + private string $value = '', + private ?int $expires = null, + private ?string $path = null, + private ?string $domain = null, + private bool $secure = false, + private bool $httpOnly = false, + private ?string $sameSite = null, + ) { + if ($this->name === '' || preg_match(self::INVALID_NAME_PATTERN, $this->name) === 1) { + throw CookieException::invalidName($this->name); + } + + if ($this->sameSite === 'None' && !$this->secure) { + throw CookieException::sameSiteNoneRequiresSecure($this->name); + } + } + + public function name(): string + { + return $this->name; + } + + public function path(): ?string + { + return $this->path; + } + + public function domain(): ?string + { + return $this->domain; + } + + public function toSetCookieString(): string + { + $parts = [$this->name . '=' . rawurlencode($this->value)]; + + if ($this->expires !== null && $this->expires !== 0) { + $parts[] = 'Expires=' . gmdate('D, d M Y H:i:s \G\M\T', $this->expires); + } + + if ($this->path !== null) { + $parts[] = 'Path=' . $this->path; + } + + if ($this->domain !== null) { + $parts[] = 'Domain=' . $this->domain; + } + + if ($this->secure) { + $parts[] = 'Secure'; + } + + if ($this->httpOnly) { + $parts[] = 'HttpOnly'; + } + + if ($this->sameSite !== null) { + $parts[] = 'SameSite=' . $this->sameSite; + } + + return implode('; ', $parts); + } +} diff --git a/packages/routing/src/Http/Request.php b/packages/routing/src/Http/Request.php index 3d8f9a58..fb75941c 100644 --- a/packages/routing/src/Http/Request.php +++ b/packages/routing/src/Http/Request.php @@ -4,12 +4,15 @@ namespace Marko\Routing\Http; +use NoDiscard; + readonly class Request { /** * @param array $server * @param array $query * @param array $post + * @param array $cookies */ public function __construct( private array $server = [], @@ -18,6 +21,7 @@ public function __construct( private string $body = '', private ?string $controller = null, private ?string $action = null, + private array $cookies = [], ) {} public static function fromGlobals(): self @@ -39,6 +43,7 @@ public static function fromGlobals(): self query: $_GET, post: $post, body: $body, + cookies: $_COOKIE, ); } @@ -83,6 +88,20 @@ public function post( return $this->post[$key] ?? $default; } + /** + * @return ($key is null ? array : mixed) + */ + public function cookie( + ?string $key = null, + mixed $default = null, + ): mixed { + if ($key === null) { + return $this->cookies; + } + + return $this->cookies[$key] ?? $default; + } + public function body(): string { return $this->body; @@ -101,6 +120,7 @@ public function ip(): ?string return $this->server('REMOTE_ADDR'); } + #[NoDiscard] public function withRoute( string $controller, string $action, @@ -112,6 +132,7 @@ public function withRoute( body: $this->body, controller: $controller, action: $action, + cookies: $this->cookies, ); } diff --git a/packages/routing/src/Http/Response.php b/packages/routing/src/Http/Response.php index 4ef0d44d..2697e760 100644 --- a/packages/routing/src/Http/Response.php +++ b/packages/routing/src/Http/Response.php @@ -4,8 +4,27 @@ namespace Marko\Routing\Http; -readonly class Response +use JsonException; +use NoDiscard; + +/** + * Not `readonly class`, and its properties are not individually `readonly`, + * because the decoration API (`with*()` methods) relies on `clone $this` + * followed by assignment on the clone. On PHP 8.5.1, modifying a readonly + * property on a clone fails both by direct assignment and via + * `ReflectionProperty::setValue()`, and `clone $this with { ... }` is not + * valid syntax. `clone` is required (rather than `new static(...)`) because + * subclasses such as `StreamingResponse` have constructors with a different + * signature than the parent. Immutability is enforced by API design instead: + * properties stay `private` and no setters are exposed. + */ +class Response { + /** + * @var list + */ + private array $cookies = []; + /** * @param array $headers */ @@ -33,6 +52,70 @@ public function headers(): array return $this->headers; } + /** + * @return list + */ + public function cookies(): array + { + return $this->cookies; + } + + #[NoDiscard] + public function withHeader( + string $name, + string $value, + ): static { + $clone = clone $this; + $clone->headers[$name] = $value; + + return $clone; + } + + /** + * @param array $headers + */ + #[NoDiscard] + public function withHeaders(array $headers): static + { + $clone = clone $this; + $clone->headers = [...$clone->headers, ...$headers]; + + return $clone; + } + + #[NoDiscard] + public function withStatus(int $statusCode): static + { + $clone = clone $this; + $clone->statusCode = $statusCode; + + return $clone; + } + + #[NoDiscard] + public function withCookie(Cookie $cookie): static + { + $clone = clone $this; + + $index = array_find_key( + $clone->cookies, + fn (Cookie $existing): bool => $existing->name() === $cookie->name() + && $existing->path() === $cookie->path() + && $existing->domain() === $cookie->domain(), + ); + + if ($index === null) { + $clone->cookies[] = $cookie; + } else { + $clone->cookies[$index] = $cookie; + } + + return $clone; + } + + /** + * @throws JsonException + */ public static function json( mixed $data, int $statusCode = 200, @@ -66,13 +149,31 @@ public static function redirect( ); } + /** + * @return list + */ + public function headerLines(): array + { + $lines = []; + + foreach ($this->headers as $name => $value) { + $lines[] = "$name: $value"; + } + + foreach ($this->cookies as $cookie) { + $lines[] = 'Set-Cookie: ' . $cookie->toSetCookieString(); + } + + return $lines; + } + public function send(): void { if (!headers_sent()) { http_response_code($this->statusCode); - foreach ($this->headers as $name => $value) { - header("$name: $value"); + foreach ($this->headerLines() as $line) { + header($line); } } diff --git a/packages/routing/tests/Http/CookieTest.php b/packages/routing/tests/Http/CookieTest.php new file mode 100644 index 00000000..9049920c --- /dev/null +++ b/packages/routing/tests/Http/CookieTest.php @@ -0,0 +1,81 @@ +toSetCookieString())->toBe('session_id=abc123'); +}); + +it('renders the path attribute when provided', function (): void { + $cookie = new Cookie(name: 'session_id', value: 'abc123', path: '/'); + + expect($cookie->toSetCookieString())->toBe('session_id=abc123; Path=/'); +}); + +it('renders the domain attribute when provided', function (): void { + $cookie = new Cookie(name: 'session_id', value: 'abc123', domain: 'example.com'); + + expect($cookie->toSetCookieString())->toBe('session_id=abc123; Domain=example.com'); +}); + +it('renders expires as an rfc 7231 formatted date', function (): void { + $cookie = new Cookie(name: 'session_id', value: 'abc123', expires: 1704067200); + + expect($cookie->toSetCookieString())->toBe('session_id=abc123; Expires=Mon, 01 Jan 2024 00:00:00 GMT'); +}); + +it('omits the expires attribute entirely for a browser session cookie', function (): void { + $cookie = new Cookie(name: 'session_id', value: 'abc123', expires: 0); + + expect($cookie->toSetCookieString())->toBe('session_id=abc123'); +}); + +it('renders secure and httponly flags only when enabled', function (): void { + $enabled = new Cookie(name: 'session_id', value: 'abc123', secure: true, httpOnly: true); + $disabled = new Cookie(name: 'session_id', value: 'abc123'); + + expect($enabled->toSetCookieString())->toBe('session_id=abc123; Secure; HttpOnly') + ->and($disabled->toSetCookieString())->toBe('session_id=abc123'); +}); + +it('renders the samesite attribute when provided', function (): void { + $cookie = new Cookie(name: 'session_id', value: 'abc123', sameSite: 'Lax'); + + expect($cookie->toSetCookieString())->toBe('session_id=abc123; SameSite=Lax'); +}); + +it('encodes a value containing characters that are illegal in a set-cookie header', function (): void { + $cookie = new Cookie(name: 'data', value: 'a; b'); + + expect($cookie->toSetCookieString())->toBe('data=a%3B%20b'); +}); + +it('throws when the cookie name contains an invalid character', function (): void { + expect(fn (): Cookie => new Cookie(name: 'sess ion', value: 'abc123')) + ->toThrow(CookieException::class); +}); + +it('throws when samesite is none without the secure flag', function (): void { + expect(fn (): Cookie => new Cookie(name: 'session_id', value: 'abc123', sameSite: 'None')) + ->toThrow(CookieException::class); +}); + +it('exposes the name path and domain used to identify a cookie', function (): void { + $cookie = new Cookie(name: 'session_id', value: 'abc123', path: '/admin', domain: 'example.com'); + + expect($cookie->name())->toBe('session_id') + ->and($cookie->path())->toBe('/admin') + ->and($cookie->domain())->toBe('example.com'); +}); + +it('defaults path and domain to null when not provided', function (): void { + $cookie = new Cookie(name: 'session_id', value: 'abc123'); + + expect($cookie->path())->toBeNull() + ->and($cookie->domain())->toBeNull(); +}); diff --git a/packages/routing/tests/Http/RequestTest.php b/packages/routing/tests/Http/RequestTest.php index 5d9e9e87..838a622b 100644 --- a/packages/routing/tests/Http/RequestTest.php +++ b/packages/routing/tests/Http/RequestTest.php @@ -4,7 +4,7 @@ use Marko\Routing\Http\Request; -it('creates request from PHP superglobals', function () { +it('creates request from PHP superglobals', function (): void { $_SERVER['REQUEST_METHOD'] = 'GET'; $_SERVER['REQUEST_URI'] = '/test'; $_GET = ['foo' => 'bar']; @@ -16,7 +16,7 @@ expect($request)->toBeInstanceOf(Request::class); }); -it('returns method (GET, POST, etc.) from server vars', function () { +it('returns method (GET, POST, etc.) from server vars', function (): void { $_SERVER['REQUEST_METHOD'] = 'POST'; $request = Request::fromGlobals(); @@ -24,7 +24,7 @@ expect($request->method())->toBe('POST'); }); -it('returns path without query string', function () { +it('returns path without query string', function (): void { $_SERVER['REQUEST_URI'] = '/users/123?page=1&sort=name'; $request = Request::fromGlobals(); @@ -32,7 +32,7 @@ expect($request->path())->toBe('/users/123'); }); -it('returns query parameters from GET', function () { +it('returns query parameters from GET', function (): void { $_GET = ['page' => '1', 'sort' => 'name']; $request = Request::fromGlobals(); @@ -43,7 +43,7 @@ ->and($request->query('missing', 'default'))->toBe('default'); }); -it('returns body parameters from POST', function () { +it('returns body parameters from POST', function (): void { $_POST = ['name' => 'John', 'email' => 'john@example.com']; $request = Request::fromGlobals(); @@ -54,7 +54,7 @@ ->and($request->post('missing', 'default'))->toBe('default'); }); -it('returns specific header by name', function () { +it('returns specific header by name', function (): void { $_SERVER['HTTP_CONTENT_TYPE'] = 'application/json'; $_SERVER['HTTP_ACCEPT'] = 'text/html'; $_SERVER['HTTP_X_CUSTOM_HEADER'] = 'custom-value'; @@ -68,7 +68,7 @@ ->and($request->header('Missing-Header', 'default'))->toBe('default'); }); -it('returns all headers', function () { +it('returns all headers', function (): void { $_SERVER = [ 'HTTP_CONTENT_TYPE' => 'application/json', 'HTTP_ACCEPT' => 'text/html', @@ -87,32 +87,32 @@ ->and($headers)->not->toHaveKey('Server-Name'); }); -it('returns a raw server param by key via server()', function () { +it('returns a raw server param by key via server()', function (): void { $request = new Request(server: ['SERVER_NAME' => 'example.com', 'SERVER_PORT' => '443']); expect($request->server('SERVER_NAME'))->toBe('example.com') ->and($request->server('SERVER_PORT'))->toBe('443'); }); -it('returns null from server() when the key is absent', function () { +it('returns null from server() when the key is absent', function (): void { $request = new Request(server: ['SERVER_NAME' => 'example.com']); expect($request->server('MISSING_KEY'))->toBeNull(); }); -it('returns the REMOTE_ADDR value via ip()', function () { +it('returns the REMOTE_ADDR value via ip()', function (): void { $request = new Request(server: ['REMOTE_ADDR' => '192.168.1.1']); expect($request->ip())->toBe('192.168.1.1'); }); -it('returns null from ip() when REMOTE_ADDR is absent', function () { +it('returns null from ip() when REMOTE_ADDR is absent', function (): void { $request = new Request(server: ['SERVER_NAME' => 'example.com']); expect($request->ip())->toBeNull(); }); -it('ignores X-Forwarded-For when resolving ip()', function () { +it('ignores X-Forwarded-For when resolving ip()', function (): void { $request = new Request(server: [ 'REMOTE_ADDR' => '10.0.0.1', 'HTTP_X_FORWARDED_FOR' => '203.0.113.1', @@ -121,7 +121,7 @@ expect($request->ip())->toBe('10.0.0.1'); }); -it('returns a new Request carrying the controller and action via withRoute()', function () { +it('returns a new Request carrying the controller and action via withRoute()', function (): void { $request = new Request(server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/']); $routed = $request->withRoute('App\\Controllers\\HomeController', 'index'); @@ -131,7 +131,7 @@ ->and($routed->action())->toBe('index'); }); -it('leaves the original Request unchanged after withRoute() (immutability)', function () { +it('leaves the original Request unchanged after withRoute()', function (): void { $request = new Request(server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/']); $routed = $request->withRoute('App\\Controllers\\HomeController', 'index'); @@ -141,39 +141,80 @@ ->and($routed)->not->toBe($request); }); -it('returns null from controller() and action() before withRoute() is called', function () { +it('returns null from controller() and action() before withRoute() is called', function (): void { $request = new Request(server: ['REQUEST_METHOD' => 'GET']); expect($request->controller())->toBeNull() ->and($request->action())->toBeNull(); }); -it('reads Content-Type from the CGI CONTENT_TYPE server key when HTTP_CONTENT_TYPE is absent', function () { +it('reads Content-Type from the CGI CONTENT_TYPE server key when HTTP_CONTENT_TYPE is absent', function (): void { $request = new Request(server: ['CONTENT_TYPE' => 'application/json']); expect($request->header('Content-Type'))->toBe('application/json'); }); -it('still reads Content-Type from HTTP_CONTENT_TYPE when present', function () { +it('still reads Content-Type from HTTP_CONTENT_TYPE when present', function (): void { $request = new Request(server: ['HTTP_CONTENT_TYPE' => 'text/html', 'CONTENT_TYPE' => 'application/json']); expect($request->header('Content-Type'))->toBe('text/html'); }); -it('reads Content-Length from the CGI CONTENT_LENGTH server key', function () { +it('reads Content-Length from the CGI CONTENT_LENGTH server key', function (): void { $request = new Request(server: ['CONTENT_LENGTH' => '42']); expect($request->header('Content-Length'))->toBe('42'); }); -it('does not read an un-prefixed key for a non-CGI header name', function () { +it('does not read an un-prefixed key for a non-CGI header name', function (): void { $request = new Request(server: ['X_CUSTOM' => 'should-not-appear']); expect($request->header('X-Custom'))->toBeNull(); }); -it('returns the default when neither header form is present', function () { +it('returns the default when neither header form is present', function (): void { $request = new Request(server: []); expect($request->header('Content-Type', 'text/plain'))->toBe('text/plain'); }); + +it('returns all cookies when no key is given', function (): void { + $request = new Request(cookies: ['session_id' => 'abc123', 'theme' => 'dark']); + + expect($request->cookie())->toBe(['session_id' => 'abc123', 'theme' => 'dark']); +}); + +it('returns a single cookie value by name', function (): void { + $request = new Request(cookies: ['session_id' => 'abc123']); + + expect($request->cookie('session_id'))->toBe('abc123'); +}); + +it('returns the default when the cookie is absent', function (): void { + $request = new Request(cookies: ['session_id' => 'abc123']); + + expect($request->cookie('missing'))->toBeNull() + ->and($request->cookie('missing', 'default'))->toBe('default'); +}); + +it('defaults to an empty cookie collection', function (): void { + $request = new Request(); + + expect($request->cookie())->toBeEmpty(); +}); + +it('captures cookies from globals', function (): void { + $_COOKIE = ['session_id' => 'abc123']; + + $request = Request::fromGlobals(); + + expect($request->cookie('session_id'))->toBe('abc123'); +}); + +it('preserves cookies through withRoute', function (): void { + $request = new Request(server: ['REQUEST_METHOD' => 'GET'], cookies: ['session_id' => 'abc123']); + + $routed = $request->withRoute('App\\Controllers\\HomeController', 'index'); + + expect($routed->cookie('session_id'))->toBe('abc123'); +}); diff --git a/packages/routing/tests/Http/ResponseTest.php b/packages/routing/tests/Http/ResponseTest.php index d128df11..13822e9a 100644 --- a/packages/routing/tests/Http/ResponseTest.php +++ b/packages/routing/tests/Http/ResponseTest.php @@ -2,9 +2,10 @@ declare(strict_types=1); +use Marko\Routing\Http\Cookie; use Marko\Routing\Http\Response; -it('accepts status code, headers, and body', function () { +it('accepts status code, headers, and body', function (): void { $response = new Response( body: 'Hello World', statusCode: 201, @@ -16,14 +17,14 @@ ->and($response->headers())->toBe(['X-Custom-Header' => 'custom-value']); }); -it('defaults to 200 status code', function () { +it('defaults to 200 status code', function (): void { $response = new Response(body: 'Hello'); expect($response->statusCode())->toBe(200) - ->and($response->headers())->toBe([]); + ->and($response->headers())->toBeEmpty(); }); -it('creates JSON response with correct content-type', function () { +it('creates JSON response with correct content-type', function (): void { $data = ['name' => 'John', 'age' => 30]; $response = Response::json($data); @@ -33,7 +34,7 @@ ->and($response->statusCode())->toBe(200); }); -it('creates JSON response with custom status code', function () { +it('creates JSON response with custom status code', function (): void { $data = ['error' => 'Not Found']; $response = Response::json($data, 404); @@ -41,7 +42,7 @@ ->and($response->headers()['Content-Type'])->toBe('application/json'); }); -it('creates HTML response with correct content-type', function () { +it('creates HTML response with correct content-type', function (): void { $html = 'Hello World'; $response = Response::html($html); @@ -51,7 +52,7 @@ ->and($response->statusCode())->toBe(200); }); -it('creates HTML response with custom status code', function () { +it('creates HTML response with custom status code', function (): void { $html = 'Not Found'; $response = Response::html($html, 404); @@ -59,7 +60,7 @@ ->and($response->headers()['Content-Type'])->toBe('text/html; charset=utf-8'); }); -it('creates redirect response with Location header', function () { +it('creates redirect response with Location header', function (): void { $response = Response::redirect('/dashboard'); expect($response->headers())->toHaveKey('Location') @@ -68,14 +69,104 @@ ->and($response->body())->toBe(''); }); -it('creates redirect response with custom status code', function () { +it('creates redirect response with custom status code', function (): void { $response = Response::redirect('/new-location', 301); expect($response->statusCode())->toBe(301) ->and($response->headers()['Location'])->toBe('/new-location'); }); -it('send outputs headers and body', function () { +it('returns a new instance from withHeader leaving the original unchanged', function (): void { + $response = new Response(body: 'Hello'); + + $decorated = $response->withHeader('X-Custom-Header', 'custom-value'); + + expect($decorated)->not->toBe($response) + ->and($response->headers())->toBeEmpty(); +}); + +it('merges the new header into the existing headers', function (): void { + $response = new Response(body: 'Hello', headers: ['X-Existing' => 'existing-value']); + + $decorated = $response->withHeader('X-Custom-Header', 'custom-value'); + + expect($decorated->headers())->toBe([ + 'X-Existing' => 'existing-value', + 'X-Custom-Header' => 'custom-value', + ]); +}); + +it('merges a map of headers with withHeaders', function (): void { + $response = new Response(body: 'Hello', headers: ['X-Existing' => 'existing-value']); + + $decorated = $response->withHeaders([ + 'X-First' => 'first-value', + 'X-Second' => 'second-value', + ]); + + expect($decorated->headers())->toBe([ + 'X-Existing' => 'existing-value', + 'X-First' => 'first-value', + 'X-Second' => 'second-value', + ]); +}); + +it('returns a new instance from withStatus leaving the original unchanged', function (): void { + $response = new Response(body: 'Hello', statusCode: 200); + + $decorated = $response->withStatus(404); + + expect($decorated)->not->toBe($response) + ->and($decorated->statusCode())->toBe(404) + ->and($response->statusCode())->toBe(200); +}); + +it('returns a new instance from withCookie leaving the original unchanged', function (): void { + $response = new Response(body: 'Hello'); + $cookie = new Cookie(name: 'session_id', value: 'abc123'); + + $decorated = $response->withCookie($cookie); + + expect($decorated)->not->toBe($response) + ->and($response->cookies())->toBeEmpty(); +}); + +it('accumulates cookies that differ in name path or domain', function (): void { + $response = new Response(body: 'Hello'); + $sessionCookie = new Cookie(name: 'session_id', value: 'abc123'); + $adminPathCookie = new Cookie(name: 'session_id', value: 'def456', path: '/admin'); + $apiDomainCookie = new Cookie(name: 'session_id', value: 'ghi789', domain: 'api.example.com'); + + $decorated = $response + ->withCookie($sessionCookie) + ->withCookie($adminPathCookie) + ->withCookie($apiDomainCookie); + + expect($decorated->cookies())->toBe([$sessionCookie, $adminPathCookie, $apiDomainCookie]); +}); + +it('replaces a cookie matching an existing name path and domain', function (): void { + $response = new Response(body: 'Hello'); + $original = new Cookie(name: 'session_id', value: 'abc123', path: '/', domain: 'example.com'); + $replacement = new Cookie(name: 'session_id', value: 'xyz999', path: '/', domain: 'example.com'); + + $decorated = $response + ->withCookie($original) + ->withCookie($replacement); + + expect($decorated->cookies())->toBe([$replacement]); +}); + +it('keeps cookies out of the headers collection', function (): void { + $response = new Response(body: 'Hello', headers: ['X-Existing' => 'existing-value']); + $cookie = new Cookie(name: 'session_id', value: 'abc123'); + + $decorated = $response->withCookie($cookie); + + expect($decorated->headers())->toBe(['X-Existing' => 'existing-value']); +}); + +it('outputs headers and body when sent', function (): void { $response = new Response( body: 'Hello World', statusCode: 201, @@ -88,3 +179,54 @@ expect($output)->toBe('Hello World'); }); + +it('returns regular headers as name colon value lines', function (): void { + $response = new Response( + body: 'Hello', + headers: ['X-Custom-Header' => 'custom-value', 'Content-Type' => 'text/plain'], + ); + + expect($response->headerLines())->toBe([ + 'X-Custom-Header: custom-value', + 'Content-Type: text/plain', + ]); +}); + +it('returns a distinct set-cookie line for each cookie', function (): void { + $response = new Response(body: 'Hello'); + $sessionCookie = new Cookie(name: 'session_id', value: 'abc123'); + $csrfCookie = new Cookie(name: 'csrf_token', value: 'def456'); + + $decorated = $response + ->withCookie($sessionCookie) + ->withCookie($csrfCookie); + + expect($decorated->headerLines())->toBe([ + 'Set-Cookie: ' . $sessionCookie->toSetCookieString(), + 'Set-Cookie: ' . $csrfCookie->toSetCookieString(), + ]); +}); + +it('returns only regular header lines when the response has no cookies', function (): void { + $response = new Response(body: 'Hello', headers: ['X-Custom-Header' => 'custom-value']); + + expect($response->headerLines())->toBe(['X-Custom-Header: custom-value']); +}); + +it('preserves cookie order in the emitted lines', function (): void { + $response = new Response(body: 'Hello'); + $thirdCookie = new Cookie(name: 'third', value: '3'); + $firstCookie = new Cookie(name: 'first', value: '1'); + $secondCookie = new Cookie(name: 'second', value: '2'); + + $decorated = $response + ->withCookie($thirdCookie) + ->withCookie($firstCookie) + ->withCookie($secondCookie); + + expect($decorated->headerLines())->toBe([ + 'Set-Cookie: ' . $thirdCookie->toSetCookieString(), + 'Set-Cookie: ' . $firstCookie->toSetCookieString(), + 'Set-Cookie: ' . $secondCookie->toSetCookieString(), + ]); +}); diff --git a/packages/security/src/Middleware/CorsMiddleware.php b/packages/security/src/Middleware/CorsMiddleware.php index 06d163bd..afc8722d 100644 --- a/packages/security/src/Middleware/CorsMiddleware.php +++ b/packages/security/src/Middleware/CorsMiddleware.php @@ -9,10 +9,10 @@ use Marko\Routing\Middleware\MiddlewareInterface; use Marko\Security\Config\SecurityConfig; -class CorsMiddleware implements MiddlewareInterface +readonly class CorsMiddleware implements MiddlewareInterface { public function __construct( - private readonly SecurityConfig $config, + private SecurityConfig $securityConfig, ) {} public function handle( @@ -41,19 +41,13 @@ public function handle( /** @var Response $response */ $response = $next($request); - return new Response( - body: $response->body(), - statusCode: $response->statusCode(), - headers: array_merge($response->headers(), [ - 'Access-Control-Allow-Origin' => $origin, - ]), - ); + return $response->withHeader('Access-Control-Allow-Origin', $origin); } private function isAllowedOrigin( string $origin, ): bool { - $allowedOrigins = $this->config->corsAllowedOrigins(); + $allowedOrigins = $this->securityConfig->corsAllowedOrigins(); if (in_array('*', $allowedOrigins, true)) { return true; @@ -70,9 +64,9 @@ private function buildPreflightHeaders( ): array { return [ 'Access-Control-Allow-Origin' => $origin, - 'Access-Control-Allow-Methods' => implode(', ', $this->config->corsAllowedMethods()), - 'Access-Control-Allow-Headers' => implode(', ', $this->config->corsAllowedHeaders()), - 'Access-Control-Max-Age' => (string) $this->config->corsMaxAge(), + 'Access-Control-Allow-Methods' => implode(', ', $this->securityConfig->corsAllowedMethods()), + 'Access-Control-Allow-Headers' => implode(', ', $this->securityConfig->corsAllowedHeaders()), + 'Access-Control-Max-Age' => (string) $this->securityConfig->corsMaxAge(), ]; } } diff --git a/packages/security/src/Middleware/SecurityHeadersMiddleware.php b/packages/security/src/Middleware/SecurityHeadersMiddleware.php index 89e4b243..9eaf1ed6 100644 --- a/packages/security/src/Middleware/SecurityHeadersMiddleware.php +++ b/packages/security/src/Middleware/SecurityHeadersMiddleware.php @@ -9,10 +9,10 @@ use Marko\Routing\Middleware\MiddlewareInterface; use Marko\Security\Config\SecurityConfig; -class SecurityHeadersMiddleware implements MiddlewareInterface +readonly class SecurityHeadersMiddleware implements MiddlewareInterface { public function __construct( - private readonly SecurityConfig $config, + private SecurityConfig $securityConfig, ) {} public function handle( @@ -22,13 +22,7 @@ public function handle( /** @var Response $response */ $response = $next($request); - $securityHeaders = $this->buildSecurityHeaders(); - - return new Response( - body: $response->body(), - statusCode: $response->statusCode(), - headers: array_merge($response->headers(), $securityHeaders), - ); + return $response->withHeaders($this->buildSecurityHeaders()); } /** @@ -37,12 +31,12 @@ public function handle( private function buildSecurityHeaders(): array { $headerMap = [ - 'X-Content-Type-Options' => $this->config->headerXContentTypeOptions(), - 'X-Frame-Options' => $this->config->headerXFrameOptions(), - 'X-XSS-Protection' => $this->config->headerXXssProtection(), - 'Strict-Transport-Security' => $this->config->headerStrictTransportSecurity(), - 'Referrer-Policy' => $this->config->headerReferrerPolicy(), - 'Content-Security-Policy' => $this->config->headerContentSecurityPolicy(), + 'X-Content-Type-Options' => $this->securityConfig->headerXContentTypeOptions(), + 'X-Frame-Options' => $this->securityConfig->headerXFrameOptions(), + 'X-XSS-Protection' => $this->securityConfig->headerXXssProtection(), + 'Strict-Transport-Security' => $this->securityConfig->headerStrictTransportSecurity(), + 'Referrer-Policy' => $this->securityConfig->headerReferrerPolicy(), + 'Content-Security-Policy' => $this->securityConfig->headerContentSecurityPolicy(), ]; return array_filter($headerMap, static fn (string $value): bool => $value !== ''); diff --git a/packages/security/tests/Helpers.php b/packages/security/tests/Helpers.php new file mode 100644 index 00000000..8255580e --- /dev/null +++ b/packages/security/tests/Helpers.php @@ -0,0 +1,123 @@ + $headers + */ + public function __construct( + public readonly string $tag, + string $body = '', + int $statusCode = 200, + array $headers = [], + ) { + parent::__construct($body, $statusCode, $headers); + } +} + +/** + * A `Response` subclass carrying a simulated streamed payload, used to prove + * that streaming state survives middleware that used to rebuild a base + * `Response`. + */ +class StreamingLikeResponse extends Response +{ + /** + * @param list $chunks + * @param array $headers + */ + public function __construct( + private readonly array $chunks, + int $statusCode = 200, + array $headers = [], + ) { + parent::__construct('', $statusCode, $headers); + } + + /** + * @return list + */ + public function chunks(): array + { + return $this->chunks; + } +} + +final class Helpers +{ + /** + * @param array $headers + */ + public static function createTaggedResponse( + string $tag = 'tagged', + string $body = '', + int $statusCode = 200, + array $headers = [], + ): TaggedResponse { + return new TaggedResponse($tag, $body, $statusCode, $headers); + } + + /** + * @param list $chunks + * @param array $headers + */ + public static function createStreamingLikeResponse( + array $chunks = ['event: message', 'data: hello'], + int $statusCode = 200, + array $headers = [], + ): StreamingLikeResponse { + return new StreamingLikeResponse($chunks, $statusCode, $headers); + } + + /** + * @param array $configData + */ + public static function createSecurityConfig(array $configData = []): SecurityConfig + { + return new SecurityConfig(new FakeConfigRepository($configData)); + } + + /** + * @param array $overrides + * @return array + */ + public static function defaultHeadersConfig(array $overrides = []): array + { + return array_merge([ + 'security.headers.x_content_type_options' => 'nosniff', + 'security.headers.x_frame_options' => 'SAMEORIGIN', + 'security.headers.x_xss_protection' => '1; mode=block', + 'security.headers.strict_transport_security' => 'max-age=31536000; includeSubDomains', + 'security.headers.referrer_policy' => 'strict-origin-when-cross-origin', + 'security.headers.content_security_policy' => "default-src 'self'", + ], $overrides); + } + + /** + * @param array $overrides + * @return array + */ + public static function defaultCorsConfig(array $overrides = []): array + { + return array_merge([ + 'security.cors.allowed_origins' => ['https://example.com'], + 'security.cors.allowed_methods' => ['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], + 'security.cors.allowed_headers' => ['Content-Type', 'X-Requested-With', 'X-CSRF-TOKEN'], + 'security.cors.max_age' => 86400, + ], $overrides); + } +} diff --git a/packages/security/tests/Unit/CorsMiddlewareTest.php b/packages/security/tests/Unit/CorsMiddlewareTest.php index 2bbf9c11..07e6de57 100644 --- a/packages/security/tests/Unit/CorsMiddlewareTest.php +++ b/packages/security/tests/Unit/CorsMiddlewareTest.php @@ -7,39 +7,24 @@ use Marko\Routing\Middleware\MiddlewareInterface; use Marko\Security\Config\SecurityConfig; use Marko\Security\Middleware\CorsMiddleware; +use Marko\Security\Tests\Helpers; +use Marko\Security\Tests\TaggedResponse; use Marko\Testing\Fake\FakeConfigRepository; -function createCorsConfig( - array $configData = [], -): SecurityConfig { - return new SecurityConfig(new FakeConfigRepository($configData)); -} - -function defaultCorsConfig( - array $overrides = [], -): array { - return array_merge([ - 'security.cors.allowed_origins' => ['https://example.com'], - 'security.cors.allowed_methods' => ['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], - 'security.cors.allowed_headers' => ['Content-Type', 'X-Requested-With', 'X-CSRF-TOKEN'], - 'security.cors.max_age' => 86400, - ], $overrides); -} - describe('CorsMiddleware', function (): void { it('implements MiddlewareInterface', function (): void { - $config = createCorsConfig(defaultCorsConfig()); + $config = Helpers::createSecurityConfig(Helpers::defaultCorsConfig()); $middleware = new CorsMiddleware($config); expect($middleware)->toBeInstanceOf(MiddlewareInterface::class); }); it('passes request through when no Origin header present', function (): void { - $config = createCorsConfig(defaultCorsConfig()); + $config = Helpers::createSecurityConfig(Helpers::defaultCorsConfig()); $middleware = new CorsMiddleware($config); $request = new Request(server: ['REQUEST_METHOD' => 'GET']); - $next = fn (Request $r) => new Response('OK', 200); + $next = fn (Request $r): Response => new Response('OK', 200); $response = $middleware->handle($request, $next); @@ -49,14 +34,14 @@ function defaultCorsConfig( }); it('adds CORS headers for allowed origin', function (): void { - $config = createCorsConfig(defaultCorsConfig()); + $config = Helpers::createSecurityConfig(Helpers::defaultCorsConfig()); $middleware = new CorsMiddleware($config); $request = new Request(server: [ 'REQUEST_METHOD' => 'GET', 'HTTP_ORIGIN' => 'https://example.com', ]); - $next = fn (Request $r) => new Response('OK', 200); + $next = fn (Request $r): Response => new Response('OK', 200); $response = $middleware->handle($request, $next); @@ -67,14 +52,14 @@ function defaultCorsConfig( }); it('rejects request from disallowed origin', function (): void { - $config = createCorsConfig(defaultCorsConfig()); + $config = Helpers::createSecurityConfig(Helpers::defaultCorsConfig()); $middleware = new CorsMiddleware($config); $request = new Request(server: [ 'REQUEST_METHOD' => 'GET', 'HTTP_ORIGIN' => 'https://evil.com', ]); - $next = fn (Request $r) => new Response('OK', 200); + $next = fn (Request $r): Response => new Response('OK', 200); $response = $middleware->handle($request, $next); @@ -84,7 +69,7 @@ function defaultCorsConfig( }); it('handles preflight OPTIONS request with 204 response', function (): void { - $config = createCorsConfig(defaultCorsConfig()); + $config = Helpers::createSecurityConfig(Helpers::defaultCorsConfig()); $middleware = new CorsMiddleware($config); $request = new Request(server: [ @@ -92,7 +77,7 @@ function defaultCorsConfig( 'HTTP_ORIGIN' => 'https://example.com', ]); $nextCalled = false; - $next = function (Request $r) use (&$nextCalled) { + $next = function (Request $r) use (&$nextCalled): Response { $nextCalled = true; return new Response('OK', 200); @@ -110,7 +95,7 @@ function defaultCorsConfig( }); it('supports wildcard origin', function (): void { - $config = createCorsConfig(defaultCorsConfig([ + $config = Helpers::createSecurityConfig(Helpers::defaultCorsConfig([ 'security.cors.allowed_origins' => ['*'], ])); $middleware = new CorsMiddleware($config); @@ -119,7 +104,7 @@ function defaultCorsConfig( 'REQUEST_METHOD' => 'GET', 'HTTP_ORIGIN' => 'https://any-site.com', ]); - $next = fn (Request $r) => new Response('OK', 200); + $next = fn (Request $r): Response => new Response('OK', 200); $response = $middleware->handle($request, $next); @@ -128,7 +113,7 @@ function defaultCorsConfig( }); it('includes configured allowed methods and headers in preflight response', function (): void { - $config = createCorsConfig(defaultCorsConfig([ + $config = Helpers::createSecurityConfig(Helpers::defaultCorsConfig([ 'security.cors.allowed_methods' => ['GET', 'POST'], 'security.cors.allowed_headers' => ['Content-Type', 'Authorization'], ])); @@ -138,7 +123,7 @@ function defaultCorsConfig( 'REQUEST_METHOD' => 'OPTIONS', 'HTTP_ORIGIN' => 'https://example.com', ]); - $next = fn (Request $r) => new Response('OK', 200); + $next = fn (Request $r): Response => new Response('OK', 200); $response = $middleware->handle($request, $next); @@ -148,12 +133,31 @@ function defaultCorsConfig( ->and($response->headers()['Access-Control-Allow-Headers'])->toBe('Content-Type, Authorization'); }); - it('uses FakeConfigRepository instead of inline config stub in CorsMiddlewareTest', function (): void { - $repo = new FakeConfigRepository(defaultCorsConfig()); - $config = new SecurityConfig($repo); + it('builds from a SecurityConfig backed by FakeConfigRepository', function (): void { + $repository = new FakeConfigRepository(Helpers::defaultCorsConfig()); + $config = new SecurityConfig($repository); $middleware = new CorsMiddleware($config); - expect($repo)->toBeInstanceOf(FakeConfigRepository::class) + expect($repository)->toBeInstanceOf(FakeConfigRepository::class) ->and($middleware)->toBeInstanceOf(MiddlewareInterface::class); }); + + it('preserves the response subclass through the security package cors middleware', function (): void { + $config = Helpers::createSecurityConfig(Helpers::defaultCorsConfig()); + $middleware = new CorsMiddleware($config); + + $request = new Request(server: [ + 'REQUEST_METHOD' => 'GET', + 'HTTP_ORIGIN' => 'https://example.com', + ]); + $next = fn (Request $r): TaggedResponse => Helpers::createTaggedResponse(tag: 'from-controller'); + + $response = $middleware->handle($request, $next); + + /** @var TaggedResponse $response */ + expect($response) + ->toBeInstanceOf(TaggedResponse::class) + ->and($response->tag)->toBe('from-controller') + ->and($response->headers()['Access-Control-Allow-Origin'])->toBe('https://example.com'); + }); }); diff --git a/packages/security/tests/Unit/SecurityHeadersMiddlewareTest.php b/packages/security/tests/Unit/SecurityHeadersMiddlewareTest.php index 5fcc562c..4b14296d 100644 --- a/packages/security/tests/Unit/SecurityHeadersMiddlewareTest.php +++ b/packages/security/tests/Unit/SecurityHeadersMiddlewareTest.php @@ -7,41 +7,25 @@ use Marko\Routing\Middleware\MiddlewareInterface; use Marko\Security\Config\SecurityConfig; use Marko\Security\Middleware\SecurityHeadersMiddleware; +use Marko\Security\Tests\Helpers; +use Marko\Security\Tests\StreamingLikeResponse; +use Marko\Security\Tests\TaggedResponse; use Marko\Testing\Fake\FakeConfigRepository; -function createHeadersConfig( - array $configData = [], -): SecurityConfig { - return new SecurityConfig(new FakeConfigRepository($configData)); -} - -function defaultHeadersConfig( - array $overrides = [], -): array { - return array_merge([ - 'security.headers.x_content_type_options' => 'nosniff', - 'security.headers.x_frame_options' => 'SAMEORIGIN', - 'security.headers.x_xss_protection' => '1; mode=block', - 'security.headers.strict_transport_security' => 'max-age=31536000; includeSubDomains', - 'security.headers.referrer_policy' => 'strict-origin-when-cross-origin', - 'security.headers.content_security_policy' => "default-src 'self'", - ], $overrides); -} - describe('SecurityHeadersMiddleware', function (): void { it('implements MiddlewareInterface', function (): void { - $config = createHeadersConfig(defaultHeadersConfig()); + $config = Helpers::createSecurityConfig(Helpers::defaultHeadersConfig()); $middleware = new SecurityHeadersMiddleware($config); expect($middleware)->toBeInstanceOf(MiddlewareInterface::class); }); it('adds all six security headers to response', function (): void { - $config = createHeadersConfig(defaultHeadersConfig()); + $config = Helpers::createSecurityConfig(Helpers::defaultHeadersConfig()); $middleware = new SecurityHeadersMiddleware($config); $request = new Request(server: ['REQUEST_METHOD' => 'GET']); - $next = fn (Request $r) => new Response('OK', 200); + $next = fn (Request $r): Response => new Response('OK', 200); $response = $middleware->handle($request, $next); @@ -62,14 +46,14 @@ function defaultHeadersConfig( }); it('uses configured header values from SecurityConfig', function (): void { - $config = createHeadersConfig(defaultHeadersConfig([ + $config = Helpers::createSecurityConfig(Helpers::defaultHeadersConfig([ 'security.headers.x_frame_options' => 'DENY', 'security.headers.referrer_policy' => 'no-referrer', ])); $middleware = new SecurityHeadersMiddleware($config); $request = new Request(server: ['REQUEST_METHOD' => 'GET']); - $next = fn (Request $r) => new Response('OK', 200); + $next = fn (Request $r): Response => new Response('OK', 200); $response = $middleware->handle($request, $next); @@ -80,14 +64,14 @@ function defaultHeadersConfig( }); it('omits headers with empty string config value', function (): void { - $config = createHeadersConfig(defaultHeadersConfig([ + $config = Helpers::createSecurityConfig(Helpers::defaultHeadersConfig([ 'security.headers.x_xss_protection' => '', 'security.headers.content_security_policy' => '', ])); $middleware = new SecurityHeadersMiddleware($config); $request = new Request(server: ['REQUEST_METHOD' => 'GET']); - $next = fn (Request $r) => new Response('OK', 200); + $next = fn (Request $r): Response => new Response('OK', 200); $response = $middleware->handle($request, $next); @@ -102,11 +86,11 @@ function defaultHeadersConfig( }); it('preserves existing response headers', function (): void { - $config = createHeadersConfig(defaultHeadersConfig()); + $config = Helpers::createSecurityConfig(Helpers::defaultHeadersConfig()); $middleware = new SecurityHeadersMiddleware($config); $request = new Request(server: ['REQUEST_METHOD' => 'GET']); - $next = fn (Request $r) => new Response('OK', 200, [ + $next = fn (Request $r): Response => new Response('OK', 200, [ 'Content-Type' => 'text/html', 'X-Custom' => 'value', ]); @@ -123,11 +107,11 @@ function defaultHeadersConfig( }); it('preserves response body and status code', function (): void { - $config = createHeadersConfig(defaultHeadersConfig()); + $config = Helpers::createSecurityConfig(Helpers::defaultHeadersConfig()); $middleware = new SecurityHeadersMiddleware($config); $request = new Request(server: ['REQUEST_METHOD' => 'GET']); - $next = fn (Request $r) => new Response('Hello World', 201, [ + $next = fn (Request $r): Response => new Response('Hello World', 201, [ 'Content-Type' => 'text/plain', ]); @@ -137,12 +121,70 @@ function defaultHeadersConfig( ->and($response->statusCode())->toBe(201); }); - it('uses FakeConfigRepository instead of inline config stub in SecurityHeadersMiddlewareTest', function (): void { - $repo = new FakeConfigRepository(defaultHeadersConfig()); - $config = new SecurityConfig($repo); + it('builds from a SecurityConfig backed by FakeConfigRepository', function (): void { + $repository = new FakeConfigRepository(Helpers::defaultHeadersConfig()); + $config = new SecurityConfig($repository); $middleware = new SecurityHeadersMiddleware($config); - expect($repo)->toBeInstanceOf(FakeConfigRepository::class) + expect($repository)->toBeInstanceOf(FakeConfigRepository::class) ->and($middleware)->toBeInstanceOf(MiddlewareInterface::class); }); + + it('preserves the response subclass through security headers middleware', function (): void { + $config = Helpers::createSecurityConfig(Helpers::defaultHeadersConfig()); + $middleware = new SecurityHeadersMiddleware($config); + + $request = new Request(server: ['REQUEST_METHOD' => 'GET']); + $next = fn (Request $r): TaggedResponse => Helpers::createTaggedResponse(tag: 'from-controller'); + + $response = $middleware->handle($request, $next); + + /** @var TaggedResponse $response */ + expect($response) + ->toBeInstanceOf(TaggedResponse::class) + ->and($response->tag)->toBe('from-controller') + ->and($response->headers())->toHaveKey('X-Frame-Options'); + }); + + it( + 'preserves the streaming payload when a streaming response passes through security headers middleware', + function (): void { + $config = Helpers::createSecurityConfig(Helpers::defaultHeadersConfig()); + $middleware = new SecurityHeadersMiddleware($config); + + $request = new Request(server: ['REQUEST_METHOD' => 'GET']); + $chunks = ['event: message', 'data: one', 'data: two']; + $next = fn (Request $r): StreamingLikeResponse => Helpers::createStreamingLikeResponse($chunks); + + $response = $middleware->handle($request, $next); + + /** @var StreamingLikeResponse $response */ + expect($response) + ->toBeInstanceOf(StreamingLikeResponse::class) + ->and($response->chunks())->toBe($chunks) + ->and($response->headers())->toHaveKey('X-Frame-Options'); + }, + ); + + it('still applies the same header values after migrating to decoration', function (): void { + $config = Helpers::createSecurityConfig(Helpers::defaultHeadersConfig([ + 'security.headers.x_frame_options' => 'DENY', + ])); + $middleware = new SecurityHeadersMiddleware($config); + + $request = new Request(server: ['REQUEST_METHOD' => 'GET']); + $next = fn (Request $r): Response => new Response('OK', 200, ['X-Custom' => 'value']); + + $response = $middleware->handle($request, $next); + + expect($response->headers())->toBe([ + 'X-Custom' => 'value', + 'X-Content-Type-Options' => 'nosniff', + 'X-Frame-Options' => 'DENY', + 'X-XSS-Protection' => '1; mode=block', + 'Strict-Transport-Security' => 'max-age=31536000; includeSubDomains', + 'Referrer-Policy' => 'strict-origin-when-cross-origin', + 'Content-Security-Policy' => "default-src 'self'", + ]); + }); }); diff --git a/packages/session-file/tests/ModuleTest.php b/packages/session-file/tests/ModuleTest.php index f294c541..e29b5ca6 100644 --- a/packages/session-file/tests/ModuleTest.php +++ b/packages/session-file/tests/ModuleTest.php @@ -44,9 +44,10 @@ 'session.gc_divisor' => 100, ]); - $handler = new FileSessionHandler(new SessionConfig($config)); - $session = new Session($handler, new SessionConfig($config)); - $middleware = new SessionMiddleware($session); + $sessionConfig = new SessionConfig($config); + $handler = new FileSessionHandler($sessionConfig); + $session = new Session($handler, $sessionConfig); + $middleware = new SessionMiddleware($session, $sessionConfig); $request = new Request(server: [ 'REQUEST_METHOD' => 'GET', diff --git a/packages/session/composer.json b/packages/session/composer.json index 97f04487..f3a46334 100644 --- a/packages/session/composer.json +++ b/packages/session/composer.json @@ -6,7 +6,8 @@ "require": { "php": "^8.5", "marko/core": "self.version", - "marko/config": "self.version" + "marko/config": "self.version", + "marko/routing": "self.version" }, "require-dev": { "marko/testing": "self.version", diff --git a/packages/session/src/Middleware/SessionMiddleware.php b/packages/session/src/Middleware/SessionMiddleware.php index 6f42fede..168e7a50 100644 --- a/packages/session/src/Middleware/SessionMiddleware.php +++ b/packages/session/src/Middleware/SessionMiddleware.php @@ -4,22 +4,37 @@ namespace Marko\Session\Middleware; +use Marko\Routing\Exceptions\CookieException; +use Marko\Routing\Http\Cookie; use Marko\Routing\Http\Request; use Marko\Routing\Http\Response; use Marko\Routing\Middleware\MiddlewareInterface; +use Marko\Session\Config\SessionConfig; use Marko\Session\Contracts\SessionInterface; +use Marko\Session\Exceptions\InvalidSessionIdException; readonly class SessionMiddleware implements MiddlewareInterface { + private const int SECONDS_PER_MINUTE = 60; + + private const int EXPIRED_COOKIE_OFFSET_SECONDS = 42000; + public function __construct( private SessionInterface $session, + private SessionConfig $sessionConfig, ) {} + /** + * @throws CookieException + */ public function handle( Request $request, callable $next, ): Response { + $inboundId = $this->inboundSessionId($request); + if (!$this->session->started) { + $this->seedSessionId($inboundId); $this->session->start(); } @@ -29,6 +44,85 @@ public function handle( $this->session->save(); } - return $response; + return $this->attachSessionCookie($response, $inboundId); + } + + private function inboundSessionId( + Request $request, + ): ?string { + $value = $request->cookie($this->sessionConfig->cookieName()); + + return is_string($value) && $value !== '' ? $value : null; + } + + private function seedSessionId( + ?string $inboundId, + ): void { + if ($inboundId === null) { + return; + } + + try { + $this->session->setId($inboundId); + } catch (InvalidSessionIdException) { + // Attacker-controlled cookie value — ignore and fall through to a fresh session + // rather than surfacing a 500 for a tampered or malformed inbound cookie. + } + } + + /** + * @throws CookieException + */ + private function attachSessionCookie( + Response $response, + ?string $inboundId, + ): Response { + $outgoingId = $this->session->getId(); + + if ($outgoingId === '') { + return $response->withCookie($this->expiredCookie()); + } + + if ($outgoingId === $inboundId) { + return $response; + } + + return $response->withCookie($this->freshCookie($outgoingId)); + } + + /** + * @throws CookieException + */ + private function freshCookie( + string $id, + ): Cookie { + return new Cookie( + name: $this->sessionConfig->cookieName(), + value: $id, + expires: $this->sessionConfig->expireOnClose() + ? null + : time() + $this->sessionConfig->lifetime() * self::SECONDS_PER_MINUTE, + path: $this->sessionConfig->cookiePath(), + domain: $this->sessionConfig->cookieDomain(), + secure: $this->sessionConfig->cookieSecure(), + httpOnly: $this->sessionConfig->cookieHttpOnly(), + sameSite: ucfirst($this->sessionConfig->cookieSameSite()), + ); + } + + /** + * @throws CookieException + */ + private function expiredCookie(): Cookie + { + return new Cookie( + name: $this->sessionConfig->cookieName(), + value: '', + expires: time() - self::EXPIRED_COOKIE_OFFSET_SECONDS, + path: $this->sessionConfig->cookiePath(), + domain: $this->sessionConfig->cookieDomain(), + secure: $this->sessionConfig->cookieSecure(), + httpOnly: $this->sessionConfig->cookieHttpOnly(), + ); } } diff --git a/packages/session/src/Session.php b/packages/session/src/Session.php index 9fcf7202..b69acc34 100644 --- a/packages/session/src/Session.php +++ b/packages/session/src/Session.php @@ -4,6 +4,7 @@ namespace Marko\Session; +use Marko\Core\Contracts\ResettableInterface; use Marko\Session\Config\SessionConfig; use Marko\Session\Contracts\SessionHandlerInterface; use Marko\Session\Contracts\SessionInterface; @@ -11,8 +12,9 @@ use Marko\Session\Exceptions\SessionException; use Marko\Session\Exceptions\SessionNotStartedException; use Marko\Session\Flash\FlashBag; +use Override; -class Session implements SessionInterface +class Session implements SessionInterface, ResettableInterface { public private(set) bool $started = false; @@ -20,6 +22,8 @@ class Session implements SessionInterface private ?FlashBag $flashBag = null; + private bool $handlerRegistered = false; + /** * @var array */ @@ -49,9 +53,12 @@ public function start(): void $this->configure(); - if ($this->id !== '') { - session_id($this->id); - } + // Always set the internal session id explicitly, even when $this->id is + // empty. PHP's session module keeps the last-used id in process memory + // after session_write_close(); without this call a subsequent start() + // in the same long-running process would silently resume the previous + // request's session instead of generating a fresh one. + session_id($this->id); if (!session_start()) { throw new SessionException( @@ -125,6 +132,8 @@ public function clear(): void } /** + * @return array + * * @throws SessionNotStartedException */ public function all(): array @@ -167,20 +176,6 @@ public function destroy(): void $this->started = false; $this->id = ''; - - // Clear session cookie - if (ini_get('session.use_cookies')) { - $params = session_get_cookie_params(); - setcookie( - session_name(), - '', - time() - 42000, - $params['path'], - $params['domain'], - $params['secure'], - $params['httponly'], - ); - } } public function getId(): string @@ -230,6 +225,14 @@ public function save(): void $this->started = false; } + #[Override] + public function reset(): void + { + $this->id = ''; + $this->data = []; + $this->flashBag = null; + } + private function configure(): void { // Note: session.save_handler is automatically set to 'user' by session_set_save_handler() @@ -237,21 +240,20 @@ private function configure(): void ini_set('session.gc_probability', (string) $this->config->gcProbability()); ini_set('session.gc_divisor', (string) $this->config->gcDivisor()); ini_set('session.use_strict_mode', '1'); - ini_set('session.use_cookies', '1'); + ini_set('session.use_cookies', '0'); ini_set('session.use_only_cookies', '1'); session_name($this->config->cookieName()); - session_set_cookie_params([ - 'lifetime' => $this->config->expireOnClose() ? 0 : $this->config->lifetime() * 60, - 'path' => $this->config->cookiePath(), - 'domain' => $this->config->cookieDomain() ?? '', - 'secure' => $this->config->cookieSecure(), - 'httponly' => $this->config->cookieHttpOnly(), - 'samesite' => ucfirst($this->config->cookieSameSite()), - ]); - - session_set_save_handler($this->handler, true); + // session_set_save_handler()'s $register_shutdown argument registers a + // register_shutdown_function() callback as a side effect. Registering + // it more than once per process would accumulate an unbounded number + // of shutdown callbacks in a long-running worker, so this must happen + // at most once, not on every start(). + if (!$this->handlerRegistered) { + session_set_save_handler($this->handler, true); + $this->handlerRegistered = true; + } } /** diff --git a/packages/session/tests/PackageStructureTest.php b/packages/session/tests/PackageStructureTest.php index 417c5535..2b3ca78a 100644 --- a/packages/session/tests/PackageStructureTest.php +++ b/packages/session/tests/PackageStructureTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -it('has a valid composer.json with correct package name marko/session', function () { +it('has a valid composer.json with correct package name marko/session', function (): void { $composerPath = dirname(__DIR__) . '/composer.json'; expect(file_exists($composerPath))->toBeTrue() @@ -10,28 +10,28 @@ ->and(json_decode(file_get_contents($composerPath), true)['name'])->toBe('marko/session'); }); -it('has correct description in composer.json', function () { +it('has correct description in composer.json', function (): void { $composerPath = dirname(__DIR__) . '/composer.json'; $composer = json_decode(file_get_contents($composerPath), true); expect($composer['description'])->toBe('Session interfaces and infrastructure for Marko Framework'); }); -it('has type marko-module in composer.json', function () { +it('has type marko-module in composer.json', function (): void { $composerPath = dirname(__DIR__) . '/composer.json'; $composer = json_decode(file_get_contents($composerPath), true); expect($composer['type'])->toBe('marko-module'); }); -it('has MIT license in composer.json', function () { +it('has MIT license in composer.json', function (): void { $composerPath = dirname(__DIR__) . '/composer.json'; $composer = json_decode(file_get_contents($composerPath), true); expect($composer['license'])->toBe('MIT'); }); -it('requires PHP 8.5 or higher', function () { +it('requires PHP 8.5 or higher', function (): void { $composerPath = dirname(__DIR__) . '/composer.json'; $composer = json_decode(file_get_contents($composerPath), true); @@ -39,7 +39,7 @@ ->and($composer['require']['php'])->toBe('^8.5'); }); -it('has PSR-4 autoloading configured for Marko\\Session namespace', function () { +it('has PSR-4 autoloading configured for Marko\\Session namespace', function (): void { $composerPath = dirname(__DIR__) . '/composer.json'; $composer = json_decode(file_get_contents($composerPath), true); @@ -59,25 +59,25 @@ expect($config)->toBeArray(); }); -it('has src directory for source code', function () { +it('has src directory for source code', function (): void { $srcPath = dirname(__DIR__) . '/src'; expect(is_dir($srcPath))->toBeTrue(); }); -it('has tests directory for tests', function () { +it('has tests directory for tests', function (): void { $testsPath = dirname(__DIR__) . '/tests'; expect(is_dir($testsPath))->toBeTrue(); }); -it('has config directory for default configuration', function () { +it('has config directory for default configuration', function (): void { $configPath = dirname(__DIR__) . '/config'; expect(is_dir($configPath))->toBeTrue(); }); -it('has default session.php config file', function () { +it('has default session.php config file', function (): void { $configPath = dirname(__DIR__) . '/config/session.php'; expect(file_exists($configPath))->toBeTrue(); @@ -101,3 +101,23 @@ expect(array_keys($module['singletons'] ?? []))->not->toContain('Marko\\Session\\Contracts\\SessionInterface'); }); + +it('does not call setcookie directly', function (): void { + $srcPath = dirname(__DIR__) . '/src'; + + $files = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($srcPath, FilesystemIterator::SKIP_DOTS), + ); + + $phpFiles = array_filter( + iterator_to_array($files), + fn (SplFileInfo $file): bool => $file->getExtension() === 'php', + ); + + $callsSetcookie = array_any( + $phpFiles, + fn (SplFileInfo $file): bool => str_contains((string) file_get_contents($file->getPathname()), 'setcookie('), + ); + + expect($callsSetcookie)->toBeFalse(); +}); diff --git a/packages/session/tests/Unit/Middleware/SessionMiddlewareTest.php b/packages/session/tests/Unit/Middleware/SessionMiddlewareTest.php index 5475a6a0..1c14ae24 100644 --- a/packages/session/tests/Unit/Middleware/SessionMiddlewareTest.php +++ b/packages/session/tests/Unit/Middleware/SessionMiddlewareTest.php @@ -4,9 +4,12 @@ use Marko\Routing\Http\Request; use Marko\Routing\Http\Response; +use Marko\Session\Config\SessionConfig; use Marko\Session\Contracts\SessionInterface; +use Marko\Session\Exceptions\InvalidSessionIdException; use Marko\Session\Flash\FlashBag; use Marko\Session\Middleware\SessionMiddleware; +use Marko\Testing\Fake\FakeConfigRepository; it('starts session before passing to next handler', function (): void { $sessionStarted = false; @@ -15,7 +18,7 @@ $sessionStarted = true; }); - $middleware = new SessionMiddleware($session); + $middleware = new SessionMiddleware($session, createMiddlewareSessionConfig()); $request = new Request(server: [ 'REQUEST_METHOD' => 'GET', @@ -34,7 +37,7 @@ $sessionSaved = true; }); - $middleware = new SessionMiddleware($session); + $middleware = new SessionMiddleware($session, createMiddlewareSessionConfig()); $request = new Request(server: [ 'REQUEST_METHOD' => 'GET', @@ -49,7 +52,7 @@ it('passes request through to next handler', function (): void { $session = createFakeSession(); - $middleware = new SessionMiddleware($session); + $middleware = new SessionMiddleware($session, createMiddlewareSessionConfig()); $request = new Request(server: [ 'REQUEST_METHOD' => 'GET', @@ -68,7 +71,7 @@ $sessionSaved = true; }); - $middleware = new SessionMiddleware($session); + $middleware = new SessionMiddleware($session, createMiddlewareSessionConfig()); $request = new Request(server: [ 'REQUEST_METHOD' => 'GET', @@ -96,7 +99,7 @@ }, ); - $middleware = new SessionMiddleware($session); + $middleware = new SessionMiddleware($session, createMiddlewareSessionConfig()); $request = new Request(server: [ 'REQUEST_METHOD' => 'GET', @@ -108,22 +111,253 @@ expect($startCount)->toBe(0); }); +it('reuses the session id from the inbound request cookie', function (): void { + $inboundId = 'abcdefghijklmnopqrstuvwxyz012345'; + $capturedSetId = null; + + $session = createFakeSession(onSetId: function (string $id) use (&$capturedSetId): void { + $capturedSetId = $id; + }); + $sessionConfig = createMiddlewareSessionConfig(); + $middleware = new SessionMiddleware($session, $sessionConfig); + + $request = new Request( + server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/'], + cookies: [$sessionConfig->cookieName() => $inboundId], + ); + + $middleware->handle($request, fn (Request $r) => new Response('OK')); + + expect($capturedSetId)->toBe($inboundId) + ->and($session->getId())->toBe($inboundId); +}); + +it('ignores an invalid inbound session cookie and starts a fresh session', function (): void { + $session = createFakeSession(rejectSetId: true); + $sessionConfig = createMiddlewareSessionConfig(); + $middleware = new SessionMiddleware($session, $sessionConfig); + + $request = new Request( + server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/'], + cookies: [$sessionConfig->cookieName() => 'not-a-valid-id'], + ); + + $response = $middleware->handle($request, fn (Request $r) => new Response('OK')); + + expect($response->body())->toBe('OK') + ->and($session->getId())->not->toBe('not-a-valid-id') + ->and($session->getId())->not->toBeEmpty(); +}); + +it('attaches the session cookie to the response when the session is new', function (): void { + $session = createFakeSession(); + $sessionConfig = createMiddlewareSessionConfig(); + $middleware = new SessionMiddleware($session, $sessionConfig); + + $request = new Request(server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/']); + + $response = $middleware->handle($request, fn (Request $r) => new Response('OK')); + + expect($response->cookies())->toHaveCount(1) + ->and($response->cookies()[0]->name())->toBe($sessionConfig->cookieName()); +}); + +it('does not attach a session cookie when the id is unchanged', function (): void { + $inboundId = 'abcdefghijklmnopqrstuvwxyz012345'; + + $session = createFakeSession(); + $sessionConfig = createMiddlewareSessionConfig(); + $middleware = new SessionMiddleware($session, $sessionConfig); + + $request = new Request( + server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/'], + cookies: [$sessionConfig->cookieName() => $inboundId], + ); + + $response = $middleware->handle($request, fn (Request $r) => new Response('OK')); + + expect($response->cookies())->toBeEmpty(); +}); + +it('attaches the new session cookie after the session id is regenerated', function (): void { + $inboundId = 'abcdefghijklmnopqrstuvwxyz012345'; + + $session = createFakeSession(); + $sessionConfig = createMiddlewareSessionConfig(); + $middleware = new SessionMiddleware($session, $sessionConfig); + + $request = new Request( + server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/'], + cookies: [$sessionConfig->cookieName() => $inboundId], + ); + + $response = $middleware->handle($request, function (Request $r) use ($session): Response { + $session->regenerate(); + + return new Response('OK'); + }); + + expect($response->cookies())->toHaveCount(1) + ->and($response->cookies()[0]->name())->toBe($sessionConfig->cookieName()) + ->and($session->getId())->not->toBe($inboundId); +}); + +it('emits exactly one session set-cookie line for the configured cookie name', function (): void { + $session = createFakeSession(); + $sessionConfig = createMiddlewareSessionConfig(); + $middleware = new SessionMiddleware($session, $sessionConfig); + + $request = new Request(server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/']); + + $response = $middleware->handle($request, fn (Request $r) => new Response('OK')); + + $setCookieLines = array_values(array_filter( + $response->headerLines(), + fn (string $line): bool => str_starts_with($line, 'Set-Cookie: ' . $sessionConfig->cookieName() . '='), + )); + + expect($setCookieLines)->toHaveCount(1); +}); + +it('applies the configured lifetime path and domain to the session cookie', function (): void { + $session = createFakeSession(); + $sessionConfig = createMiddlewareSessionConfig([ + 'session.lifetime' => 30, + 'session.cookie.path' => '/app', + 'session.cookie.domain' => 'example.test', + ]); + $middleware = new SessionMiddleware($session, $sessionConfig); + + $request = new Request(server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/']); + + $response = $middleware->handle($request, fn (Request $r) => new Response('OK')); + + $cookieLine = $response->cookies()[0]->toSetCookieString(); + preg_match('/Expires=([^;]+)/', $cookieLine, $matches); + $expiresAt = strtotime($matches[1]); + $expectedExpiresAt = time() + 30 * 60; + + expect($cookieLine)->toContain('Path=/app') + ->and($cookieLine)->toContain('Domain=example.test') + ->and(abs($expiresAt - $expectedExpiresAt))->toBeLessThan(5); +}); + +it('marks the session cookie httponly and applies the configured samesite value', function (): void { + $session = createFakeSession(); + $sessionConfig = createMiddlewareSessionConfig([ + 'session.cookie.httponly' => true, + 'session.cookie.samesite' => 'strict', + ]); + $middleware = new SessionMiddleware($session, $sessionConfig); + + $request = new Request(server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/']); + + $response = $middleware->handle($request, fn (Request $r) => new Response('OK')); + + $cookieLine = $response->cookies()[0]->toSetCookieString(); + + expect($cookieLine)->toContain('HttpOnly') + ->and($cookieLine)->toContain('SameSite=Strict'); +}); + +it('attaches an expired cookie to the response when the session is destroyed', function (): void { + $inboundId = 'abcdefghijklmnopqrstuvwxyz012345'; + + $session = createFakeSession(); + $sessionConfig = createMiddlewareSessionConfig(); + $middleware = new SessionMiddleware($session, $sessionConfig); + + $request = new Request( + server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/'], + cookies: [$sessionConfig->cookieName() => $inboundId], + ); + + $response = $middleware->handle($request, function (Request $r) use ($session): Response { + $session->destroy(); + + return new Response('OK'); + }); + + $cookieLine = $response->cookies()[0]->toSetCookieString(); + preg_match('/Expires=([^;]+)/', $cookieLine, $matches); + + expect($response->cookies())->toHaveCount(1) + ->and($response->cookies()[0]->name())->toBe($sessionConfig->cookieName()) + ->and(strtotime($matches[1]))->toBeLessThan(time()); +}); + +it('still saves the session when the handler throws and attaches no cookie', function (): void { + $sessionSaved = false; + + $session = createFakeSession(onSave: function () use (&$sessionSaved): void { + $sessionSaved = true; + }); + $sessionConfig = createMiddlewareSessionConfig(); + $middleware = new SessionMiddleware($session, $sessionConfig); + + $request = new Request(server: [ + 'REQUEST_METHOD' => 'GET', + 'REQUEST_URI' => '/', + ]); + + $thrown = null; + + try { + $middleware->handle($request, function () { + throw new RuntimeException('handler error'); + }); + } catch (RuntimeException $exception) { + $thrown = $exception; + } + + expect($thrown)->toBeInstanceOf(RuntimeException::class) + ->and($sessionSaved)->toBeTrue(); +}); + /** - * Create a fake session for testing middleware behavior. + * Create a SessionConfig backed by a FakeConfigRepository for middleware tests. * - * @return SessionInterface + * @param array $overrides + */ +function createMiddlewareSessionConfig(array $overrides = []): SessionConfig +{ + return new SessionConfig(new FakeConfigRepository([ + 'session.driver' => 'array', + 'session.lifetime' => 120, + 'session.expire_on_close' => false, + 'session.path' => '/tmp', + 'session.cookie.name' => 'marko_session', + 'session.cookie.path' => '/', + 'session.cookie.domain' => '', + 'session.cookie.secure' => true, + 'session.cookie.httponly' => true, + 'session.cookie.samesite' => 'lax', + 'session.gc_probability' => 2, + 'session.gc_divisor' => 100, + ...$overrides, + ])); +} + +/** + * Create a fake session for testing middleware behavior. */ function createFakeSession( bool $started = false, ?Closure $onStart = null, ?Closure $onSave = null, + bool $rejectSetId = false, + ?Closure $onSetId = null, ): SessionInterface { - return new class ($started, $onStart, $onSave) implements SessionInterface + return new class ($started, $onStart, $onSave, $rejectSetId, $onSetId) implements SessionInterface { + private string $id = ''; + public function __construct( public bool $started, private readonly ?Closure $onStart, private readonly ?Closure $onSave, + private readonly bool $rejectSetId, + private readonly ?Closure $onSetId, ) {} public function start(): void @@ -131,6 +365,11 @@ public function start(): void if ($this->onStart !== null) { ($this->onStart)(); } + + if ($this->id === '') { + $this->id = 'generated-session-id-1234567890123456'; + } + $this->started = true; } @@ -139,6 +378,8 @@ public function save(): void if ($this->onSave !== null) { ($this->onSave)(); } + + $this->started = false; } public function get( @@ -162,21 +403,45 @@ public function remove(string $key): void {} public function clear(): void {} + /** + * @return array + */ public function all(): array { return []; } - public function regenerate(bool $deleteOldSession = true): void {} + public function regenerate(bool $deleteOldSession = true): void + { + $this->id = 'regenerated-session-id-1234567890123456'; + } - public function destroy(): void {} + public function destroy(): void + { + $this->id = ''; + $this->started = false; + } public function getId(): string { - return ''; + return $this->id; } - public function setId(string $id): void {} + /** + * @throws InvalidSessionIdException + */ + public function setId(string $id): void + { + if ($this->onSetId !== null) { + ($this->onSetId)($id); + } + + if ($this->rejectSetId) { + throw InvalidSessionIdException::forId($id); + } + + $this->id = $id; + } public function flash(): FlashBag { diff --git a/packages/session/tests/Unit/SessionShutdownHandlerTest.php b/packages/session/tests/Unit/SessionShutdownHandlerTest.php new file mode 100644 index 00000000..466a5c08 --- /dev/null +++ b/packages/session/tests/Unit/SessionShutdownHandlerTest.php @@ -0,0 +1,108 @@ + 'array', + 'session.lifetime' => 120, + 'session.expire_on_close' => false, + 'session.path' => '/tmp', + 'session.cookie.name' => 'PHPSESSID', + 'session.cookie.path' => '/', + 'session.cookie.domain' => '', + 'session.cookie.secure' => false, + 'session.cookie.httponly' => true, + 'session.cookie.samesite' => 'lax', + 'session.gc_probability' => 1, + 'session.gc_divisor' => 100, + ])); + + $handler = new class () implements SessionHandlerInterface + { + /** @var array */ + public array $written = []; + + public function open( + string $path, + string $name, + ): bool { + return true; + } + + public function close(): bool + { + return true; + } + + public function read(string $id): string|false + { + return $this->written[$id] ?? ''; + } + + public function write( + string $id, + string $data, + ): bool { + $this->written[$id] = $data; + + return true; + } + + public function destroy(string $id): bool + { + unset($this->written[$id]); + + return true; + } + + public function gc(int $max_lifetime): int|false + { + return 0; + } + }; + + $session = new Session($handler, $sessionConfig); + + // Request 1 + $session->start(); + $session->save(); + $session->reset(); + + // Request 2, in the same long-running process + $session->start(); + $session->save(); + + expect($GLOBALS['sessionSetSaveHandlerCallCount'])->toBe(1); + }); +} diff --git a/packages/session/tests/Unit/SessionTest.php b/packages/session/tests/Unit/SessionTest.php index 3f8df77f..8a31f8fd 100644 --- a/packages/session/tests/Unit/SessionTest.php +++ b/packages/session/tests/Unit/SessionTest.php @@ -9,12 +9,11 @@ use Marko\Testing\Fake\FakeConfigRepository; /** - * Creates a Session with started=true via reflection so PHP session functions - * are not required to drive the write-after-close behaviour. + * Builds the SessionConfig shared by the tests in this file. */ -function createStartedSession(): Session +function createTestSessionConfig(): SessionConfig { - $config = new FakeConfigRepository([ + return new SessionConfig(new FakeConfigRepository([ 'session.driver' => 'array', 'session.lifetime' => 120, 'session.expire_on_close' => false, @@ -27,9 +26,16 @@ function createStartedSession(): Session 'session.cookie.samesite' => 'lax', 'session.gc_probability' => 1, 'session.gc_divisor' => 100, - ]); + ])); +} - $handler = new class () implements SessionHandlerInterface +/** + * An in-memory SessionHandlerInterface implementation used to drive a real + * Session without touching the filesystem. + */ +function createInMemorySessionHandler(): SessionHandlerInterface +{ + return new class () implements SessionHandlerInterface { /** @var array */ public array $written = []; @@ -72,9 +78,16 @@ public function gc(int $max_lifetime): int|false return 0; } }; +} - $sessionConfig = new SessionConfig($config); - $session = new Session($handler, $sessionConfig); +/** + * Creates a Session with started=true via reflection so PHP session functions + * are not required to drive the write-after-close behaviour. + */ +function createStartedSession(): Session +{ + $sessionConfig = createTestSessionConfig(); + $session = new Session(createInMemorySessionHandler(), $sessionConfig); // Use reflection to set started = true without triggering session_start() $reflection = new ReflectionProperty(Session::class, 'started'); @@ -99,3 +112,83 @@ public function gc(int $max_lifetime): int|false expect($session->get('key'))->toBe('value'); }); + +it('disables sapi cookie emission when the session starts', function (): void { + $session = new Session(createInMemorySessionHandler(), createTestSessionConfig()); + + $session->start(); + + try { + expect(ini_get('session.use_cookies'))->toBe('0'); + } finally { + $session->save(); + } +}); + +it('clears the session id when reset', function (): void { + $session = createStartedSession(); + $idProperty = new ReflectionProperty(Session::class, 'id'); + $idProperty->setValue($session, 'abcdefghijklmnopqrstuvwxyz012345'); + + $session->reset(); + + expect($session->getId())->toBe(''); +}); + +it('clears the session data when reset', function (): void { + $session = createStartedSession(); + $session->set('key', 'value'); + + $session->reset(); + + $dataProperty = new ReflectionProperty(Session::class, 'data'); + + expect($dataProperty->getValue($session))->toBeEmpty(); +}); + +it('starts a fresh session when a second request arrives with no cookie', function (): void { + $session = new Session(createInMemorySessionHandler(), createTestSessionConfig()); + + // Request 1: an authenticated visitor + $session->start(); + $firstRequestId = $session->getId(); + $session->set('user_id', 42); + $session->save(); + + // Worker resets state between requests + $session->reset(); + + // Request 2: an anonymous visitor with no session cookie — nothing calls setId() + $session->start(); + + try { + expect($session->getId())->not->toBe($firstRequestId) + ->and($session->get('user_id'))->toBeNull(); + } finally { + $session->save(); + } +}); + +it('resumes the same session when a second request arrives with the same cookie', function (): void { + $session = new Session(createInMemorySessionHandler(), createTestSessionConfig()); + + // Request 1: a visitor sets data and receives a session cookie + $session->start(); + $firstRequestId = $session->getId(); + $session->set('user_id', 42); + $session->save(); + + // Worker resets state between requests + $session->reset(); + + // Request 2: the same visitor returns with the session cookie from request 1 + $session->setId($firstRequestId); + $session->start(); + + try { + expect($session->getId())->toBe($firstRequestId) + ->and($session->get('user_id'))->toBe(42); + } finally { + $session->save(); + } +}); diff --git a/packages/sse/src/StreamingResponse.php b/packages/sse/src/StreamingResponse.php index 1b951546..95fdd6cc 100644 --- a/packages/sse/src/StreamingResponse.php +++ b/packages/sse/src/StreamingResponse.php @@ -8,7 +8,7 @@ use Marko\Routing\Http\Response; use Override; -readonly class StreamingResponse extends Response +class StreamingResponse extends Response { public function __construct( private SseStream $stream, @@ -35,8 +35,8 @@ public function send(): void if (!headers_sent()) { http_response_code($this->statusCode()); - foreach ($this->headers() as $name => $value) { - header("$name: $value"); + foreach ($this->headerLines() as $line) { + header($line); } } diff --git a/packages/sse/tests/StreamingResponseTest.php b/packages/sse/tests/StreamingResponseTest.php index 0089ed57..efa6d789 100644 --- a/packages/sse/tests/StreamingResponseTest.php +++ b/packages/sse/tests/StreamingResponseTest.php @@ -75,4 +75,104 @@ expect($response->body())->toBe(''); }); + + it('preserves the concrete subclass when decorating with a header', function (): void { + $response = new StreamingResponse( + stream: new SseStream(dataProvider: fn (): array => []), + ); + + $decorated = $response->withHeader('X-Custom-Header', 'custom-value'); + + expect($decorated)->toBeInstanceOf(StreamingResponse::class); + }); + + it('preserves the concrete subclass when decorating with a status', function (): void { + $response = new StreamingResponse( + stream: new SseStream(dataProvider: fn (): array => []), + ); + + $decorated = $response->withStatus(500); + + expect($decorated)->toBeInstanceOf(StreamingResponse::class) + ->and($decorated->statusCode())->toBe(500); + }); + + it('preserves subclass state such as the streaming payload when decorating', function (): void { + $stream = new SseStream(dataProvider: fn (): array => []); + $response = new StreamingResponse(stream: $stream); + + $decorated = $response->withHeader('X-Custom-Header', 'custom-value'); + + $property = new ReflectionProperty(StreamingResponse::class, 'stream'); + + expect($property->getValue($decorated))->toBe($stream); + }); + + it('still streams from a decorated streaming response', function (): void { + // send() forcibly closes every output buffer level before it echoes + // stream chunks, so ob_start()/ob_get_clean() cannot capture it in + // this process. Run it in a real subprocess and capture actual + // stdout instead. + $autoload = dirname(__DIR__, 3) . '/vendor/autoload.php'; + $script = << [new SseEvent(data: 'payload')], timeout: 0), + ); + + \$response->withHeader('X-Custom-Header', 'custom-value')->send(); + PHP; + + $scriptPath = tempnam(sys_get_temp_dir(), 'streaming_response_test_') . '.php'; + file_put_contents($scriptPath, $script); + + $process = proc_open( + [PHP_BINARY, $scriptPath], + [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], + $pipes, + ); + + $output = stream_get_contents($pipes[1]); + fclose($pipes[1]); + fclose($pipes[2]); + proc_close($process); + unlink($scriptPath); + + expect($output)->toContain('data: payload'); + }); + + it('does not clobber the sse headers when adding a header to a streaming response', function (): void { + $response = new StreamingResponse( + stream: new SseStream(dataProvider: fn (): array => []), + ); + + $decorated = $response->withHeader('X-Custom-Header', 'custom-value'); + + expect($decorated->headers())->toBe([ + 'Content-Type' => 'text/event-stream', + 'Cache-Control' => 'no-cache', + 'Connection' => 'keep-alive', + 'X-Accel-Buffering' => 'no', + 'X-Custom-Header' => 'custom-value', + ]); + }); + + it('emits the same header lines for a streaming response subclass', function (): void { + $response = new StreamingResponse( + stream: new SseStream(dataProvider: fn (): array => []), + ); + + expect($response->headerLines())->toBe([ + 'Content-Type: text/event-stream', + 'Cache-Control: no-cache', + 'Connection: keep-alive', + 'X-Accel-Buffering: no', + ]); + }); }); diff --git a/tests/Fixtures/MiddlewareDecoration/FreshResponseBeforeNextMiddleware.php b/tests/Fixtures/MiddlewareDecoration/FreshResponseBeforeNextMiddleware.php new file mode 100644 index 00000000..9663e343 --- /dev/null +++ b/tests/Fixtures/MiddlewareDecoration/FreshResponseBeforeNextMiddleware.php @@ -0,0 +1,30 @@ +header('X-Skip') !== null) { + return new Response( + body: '', + statusCode: 204, + ); + } + + return $next($request); + } +} diff --git a/tests/Fixtures/MiddlewareDecoration/HelperMethodResponseMiddleware.php b/tests/Fixtures/MiddlewareDecoration/HelperMethodResponseMiddleware.php new file mode 100644 index 00000000..9888848e --- /dev/null +++ b/tests/Fixtures/MiddlewareDecoration/HelperMethodResponseMiddleware.php @@ -0,0 +1,39 @@ +statusCode() >= 500) { + return $this->fallbackResponse(); + } + + return $response; + } + + private function fallbackResponse(): Response + { + return new Response( + body: 'Service Unavailable', + statusCode: 503, + ); + } +} diff --git a/tests/Fixtures/MiddlewareDecoration/RebuildAfterNextMiddleware.php b/tests/Fixtures/MiddlewareDecoration/RebuildAfterNextMiddleware.php new file mode 100644 index 00000000..1943e55a --- /dev/null +++ b/tests/Fixtures/MiddlewareDecoration/RebuildAfterNextMiddleware.php @@ -0,0 +1,33 @@ + 'value'], + ); + } +} diff --git a/tests/Fixtures/MiddlewareDecoration/RebuildFromLocalVariableMiddleware.php b/tests/Fixtures/MiddlewareDecoration/RebuildFromLocalVariableMiddleware.php new file mode 100644 index 00000000..dfd94b6a --- /dev/null +++ b/tests/Fixtures/MiddlewareDecoration/RebuildFromLocalVariableMiddleware.php @@ -0,0 +1,36 @@ +headers(); + $headers['Vary'] = 'X-Custom'; + + return new Response( + body: $response->body(), + statusCode: $response->statusCode(), + headers: $headers, + ); + } +} diff --git a/tests/Integration/PageCacheSessionMiddlewareTest.php b/tests/Integration/PageCacheSessionMiddlewareTest.php new file mode 100644 index 00000000..79592018 --- /dev/null +++ b/tests/Integration/PageCacheSessionMiddlewareTest.php @@ -0,0 +1,147 @@ + */ + private array $data = []; + + public function start(): void + { + $this->started = true; + } + + public function get( + string $key, + mixed $default = null, + ): mixed { + return $this->data[$key] ?? $default; + } + + public function set( + string $key, + mixed $value, + ): void { + $this->data[$key] = $value; + } + + public function has(string $key): bool + { + return isset($this->data[$key]); + } + + public function remove(string $key): void + { + unset($this->data[$key]); + } + + public function clear(): void + { + $this->data = []; + } + + /** + * @return array + */ + public function all(): array + { + return $this->data; + } + + public function regenerate(bool $deleteOldSession = true): void {} + + public function destroy(): void {} + + public function getId(): string + { + return 'existing-session-id'; + } + + public function setId(string $id): void {} + + public function flash(): FlashBag + { + return new FlashBag($this->data); + } + + public function save(): void {} + }; +} + +function makePageCacheabilityChecker(): CacheabilityChecker +{ + $matcher = new class () implements RouteMatcherInterface + { + public function match( + string $method, + string $path, + ): ?MatchedRoute { + return null; + } + }; + + $config = new PageCacheConfig(new FakeConfigRepository([ + 'page-cache.cacheable_methods' => ['GET', 'HEAD'], + 'page-cache.cacheable_status_codes' => [200], + ])); + + return new CacheabilityChecker($matcher, $config); +} + +function makeSessionConfig(): SessionConfig +{ + return new SessionConfig(new FakeConfigRepository([ + 'session.driver' => 'array', + 'session.lifetime' => 120, + 'session.expire_on_close' => false, + 'session.path' => '/tmp', + 'session.cookie.name' => 'marko_session', + 'session.cookie.path' => '/', + 'session.cookie.domain' => '', + 'session.cookie.secure' => true, + 'session.cookie.httponly' => true, + 'session.cookie.samesite' => 'lax', + 'session.gc_probability' => 2, + 'session.gc_divisor' => 100, + ])); +} + +it( + 'still caches a repeat visit response that passed through session middleware without a new session', + function (): void { + $session = makeRepeatVisitSession(); + $sessionConfig = makeSessionConfig(); + $middleware = new SessionMiddleware($session, $sessionConfig); + $request = new Request( + server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/'], + cookies: [$sessionConfig->cookieName() => 'existing-session-id'], + ); + + $response = $middleware->handle($request, fn (Request $request): Response => new Response( + body: 'cached page', + statusCode: 200, + )); + + $checker = makePageCacheabilityChecker(); + + expect($response->cookies())->toBeEmpty() + ->and($checker->isResponseCacheable($response))->toBeTrue(); + }, +); diff --git a/tests/MiddlewareDecorationTest.php b/tests/MiddlewareDecorationTest.php new file mode 100644 index 00000000..70cc51c4 --- /dev/null +++ b/tests/MiddlewareDecorationTest.php @@ -0,0 +1,84 @@ +discover($packagesRoot); + $violations = (new MiddlewareDecorationDetector())->scan($files); + + expect($files)->not->toBeEmpty() + ->and($violations)->toBeEmpty(); +}); + +it( + 'discovers middleware that live directly under src slash middleware', + function () use ($packagesRoot, $fixturesRoot): void { + $files = (new MiddlewareDiscovery())->discover($packagesRoot); + + // packages/security/src/Middleware/SecurityHeadersMiddleware.php has zero + // intermediate directories between src/ and Middleware/. A naive + // glob('packages/*/src/**/Middleware/*.php') silently misses it because + // glob() has no ** support; RecursiveDirectoryIterator must not. + expect($files)->toContain($packagesRoot . '/security/src/Middleware/SecurityHeadersMiddleware.php') + ->and(array_filter($files, fn (string $file): bool => str_starts_with($file, $fixturesRoot))) + ->toBeEmpty(); + }, +); + +it('fails when a middleware constructs a response after calling next', function () use ($fixturesRoot): void { + $violations = (new MiddlewareDecorationDetector())->scan([$fixturesRoot . '/RebuildAfterNextMiddleware.php']); + + expect($violations)->not->toBeEmpty(); +}); + +it( + 'fails when a middleware rebuilds a response from headers held in a local variable', + function () use ($fixturesRoot): void { + $violations = (new MiddlewareDecorationDetector())->scan( + [$fixturesRoot . '/RebuildFromLocalVariableMiddleware.php'], + ); + + expect($violations)->not->toBeEmpty(); + }, +); + +it( + 'allows a middleware to construct a genuinely new response before calling next', + function () use ($fixturesRoot): void { + $violations = (new MiddlewareDecorationDetector())->scan( + [$fixturesRoot . '/FreshResponseBeforeNextMiddleware.php'], + ); + + expect($violations)->toBeEmpty(); + }, +); + +it('allows a middleware to construct a response in a helper method', function () use ($fixturesRoot): void { + $violations = (new MiddlewareDecorationDetector())->scan([$fixturesRoot . '/HelperMethodResponseMiddleware.php']); + + expect($violations)->toBeEmpty(); +}); + +it('reports the offending file and line and a suggested fix when it fails', function () use ($fixturesRoot): void { + $file = $fixturesRoot . '/RebuildAfterNextMiddleware.php'; + $violations = (new MiddlewareDecorationDetector())->scan([$file]); + + expect($violations)->toHaveCount(1) + ->and($violations[0]['file'])->toBe($file) + ->and($violations[0]['line'])->toBe(27) + ->and($violations[0]['message']) + ->toContain($file) + ->toContain('27') + ->toContain('withHeader') + ->toContain('withHeaders') + ->toContain('withStatus'); +}); diff --git a/tests/Support/MiddlewareDecoration/MiddlewareDecorationDetector.php b/tests/Support/MiddlewareDecoration/MiddlewareDecorationDetector.php new file mode 100644 index 00000000..9fa7625a --- /dev/null +++ b/tests/Support/MiddlewareDecoration/MiddlewareDecorationDetector.php @@ -0,0 +1,345 @@ + + */ + private const array RESPONSE_ACCESSORS = ['body', 'statusCode', 'headers']; + + /** + * @param list $files + * @return list + */ + public function scan(array $files): array + { + $violations = []; + + foreach ($files as $file) { + $violations = [...$violations, ...$this->scanFile($file)]; + } + + return $violations; + } + + /** + * @return list + */ + private function scanFile(string $file): array + { + $code = file_get_contents($file); + + if ($code === false) { + return []; + } + + $tokens = PhpToken::tokenize($code); + $violations = []; + + $depth = 0; + /** @var list $contextStack */ + $contextStack = []; + + $awaitingFunctionBody = false; + $functionParenDepth = 0; + $seenParenForFunction = false; + + $count = count($tokens); + + for ($i = 0; $i < $count; $i++) { + $token = $tokens[$i]; + $text = $token->text; + + if ($token->id === T_FUNCTION) { + $awaitingFunctionBody = true; + $functionParenDepth = 0; + $seenParenForFunction = false; + continue; + } + + if ($awaitingFunctionBody) { + if ($text === '(') { + $functionParenDepth++; + $seenParenForFunction = true; + } elseif ($text === ')') { + $functionParenDepth--; + } elseif ($seenParenForFunction && $functionParenDepth === 0 && $text === '{') { + $depth++; + $contextStack[] = ['depth' => $depth, 'startIndex' => $i, 'sawNext' => false]; + $awaitingFunctionBody = false; + } elseif ($seenParenForFunction && $functionParenDepth === 0 && $text === ';') { + $awaitingFunctionBody = false; + } + + continue; + } + + if ($text === '{') { + $depth++; + continue; + } + + if ($text === '}') { + $topIndex = array_key_last($contextStack); + + if ($topIndex !== null && $contextStack[$topIndex]['depth'] === $depth) { + array_pop($contextStack); + } + + $depth--; + continue; + } + + $contextIndex = array_key_last($contextStack); + + if ($contextIndex === null) { + continue; + } + + if ($token->id === T_VARIABLE && $text === self::NEXT_VARIABLE) { + $lookahead = $this->nextSignificantToken($tokens, $i); + + if ($lookahead !== null && $lookahead->text === '(' && !$this->isBareReturnOfNextCall($tokens, $i)) { + $contextStack[$contextIndex]['sawNext'] = true; + } + + continue; + } + + if ($token->id === T_NEW && $contextStack[$contextIndex]['sawNext']) { + $className = $this->readClassName($tokens, $i); + + if ($className !== null && str_ends_with($className, 'Response')) { + $violations[] = [ + 'file' => $file, + 'line' => $token->line, + 'message' => $this->violationMessage( + file: $file, + line: $token->line, + tokens: $tokens, + startIndex: $contextStack[$contextIndex]['startIndex'], + newIndex: $i, + ), + ]; + } + } + } + + return $violations; + } + + /** + * @param list $tokens + */ + private function nextSignificantToken( + array $tokens, + int $index, + ): ?PhpToken { + $nextIndex = $this->nextSignificantIndex($tokens, $index); + + return $nextIndex === null ? null : $tokens[$nextIndex]; + } + + /** + * @param list $tokens + */ + private function nextSignificantIndex( + array $tokens, + int $index, + ): ?int { + $count = count($tokens); + + for ($j = $index + 1; $j < $count; $j++) { + if (!$tokens[$j]->isIgnorable()) { + return $j; + } + } + + return null; + } + + /** + * @param list $tokens + */ + private function previousSignificantToken( + array $tokens, + int $index, + ): ?PhpToken { + for ($j = $index - 1; $j >= 0; $j--) { + if (!$tokens[$j]->isIgnorable()) { + return $tokens[$j]; + } + } + + return null; + } + + /** + * A bare `return $next($request);` statement terminates its branch + * immediately and has no bearing on code reached only via a sibling + * branch that never called $next() — for example the common + * short-circuit `if (...) { return $next($request); }` guard clause + * that precedes an unrelated, genuinely fresh response later in the + * same method. Only count $next() as "consumed" for the rest of the + * method when its result is kept (assigned, chained, or otherwise + * used) rather than returned bare. + * + * @param list $tokens + */ + private function isBareReturnOfNextCall( + array $tokens, + int $variableIndex, + ): bool { + $previous = $this->previousSignificantToken($tokens, $variableIndex); + + if ($previous === null || $previous->id !== T_RETURN) { + return false; + } + + $openParenIndex = $this->nextSignificantIndex($tokens, $variableIndex); + + if ($openParenIndex === null) { + return false; + } + + $closingParenIndex = $this->matchingCloseParenIndex($tokens, $openParenIndex); + + if ($closingParenIndex === null) { + return false; + } + + $afterCall = $this->nextSignificantToken($tokens, $closingParenIndex); + + return $afterCall !== null && $afterCall->text === ';'; + } + + /** + * @param list $tokens + */ + private function matchingCloseParenIndex( + array $tokens, + int $openParenIndex, + ): ?int { + $count = count($tokens); + $depth = 0; + + for ($j = $openParenIndex; $j < $count; $j++) { + if ($tokens[$j]->text === '(') { + $depth++; + } elseif ($tokens[$j]->text === ')') { + $depth--; + + if ($depth === 0) { + return $j; + } + } + } + + return null; + } + + /** + * @param list $tokens + */ + private function readClassName( + array $tokens, + int $newIndex, + ): ?string { + $nameTokenIds = [T_STRING, T_NS_SEPARATOR, T_NAME_QUALIFIED, T_NAME_FULLY_QUALIFIED, T_NAME_RELATIVE]; + $count = count($tokens); + $name = ''; + + for ($j = $newIndex + 1; $j < $count; $j++) { + $token = $tokens[$j]; + + if ($token->isIgnorable()) { + continue; + } + + if (!in_array($token->id, $nameTokenIds, true)) { + break; + } + + $name .= $token->text; + } + + return $name === '' ? null : $name; + } + + /** + * @param list $tokens + */ + private function violationMessage( + string $file, + int $line, + array $tokens, + int $startIndex, + int $newIndex, + ): string { + $suffix = $this->referencesResponseAccessors($tokens, $startIndex, $newIndex) + ? ' It even copies fields off the response $next() returned before rebuilding' + . ' — decorate that response directly instead.' + : ''; + + return sprintf( + '%s:%d constructs a new Response after calling $next(). This is the rebuild anti-pattern.%s ' + . 'Decorate the response $next() returned with Response::withHeader(), withHeaders(), ' + . 'withStatus(), or withCookie() instead of rebuilding it from scratch.', + $file, + $line, + $suffix, + ); + } + + /** + * Secondary signal only, used to strengthen the failure message: does + * the enclosing method body call ->body(), ->statusCode(), or + * ->headers() anywhere before the offending `new Response(`? This + * catches the case where fields are copied into a local variable first + * (as the original InertiaMiddleware bug did) as well as direct + * argument provenance. + * + * @param list $tokens + */ + private function referencesResponseAccessors( + array $tokens, + int $startIndex, + int $newIndex, + ): bool { + for ($j = $startIndex; $j < $newIndex; $j++) { + if ($tokens[$j]->id !== T_OBJECT_OPERATOR) { + continue; + } + + $accessor = $this->nextSignificantToken($tokens, $j); + + if ($accessor !== null && in_array($accessor->text, self::RESPONSE_ACCESSORS, true)) { + return true; + } + } + + return false; + } +} diff --git a/tests/Support/MiddlewareDecoration/MiddlewareDiscovery.php b/tests/Support/MiddlewareDecoration/MiddlewareDiscovery.php new file mode 100644 index 00000000..9bb46a94 --- /dev/null +++ b/tests/Support/MiddlewareDecoration/MiddlewareDiscovery.php @@ -0,0 +1,144 @@ + + * + * @throws UnexpectedValueException + */ + public function discover(string $packagesRoot): array + { + $files = []; + + foreach ($this->packageSrcDirectories($packagesRoot) as $srcDirectory) { + foreach ($this->phpFilesUnder($srcDirectory) as $file) { + if ($this->implementsMiddlewareInterface($file)) { + $files[] = $file; + } + } + } + + sort($files); + + return $files; + } + + /** + * @return list + */ + private function packageSrcDirectories(string $packagesRoot): array + { + $entries = scandir($packagesRoot); + + if ($entries === false) { + return []; + } + + $srcDirectories = []; + + foreach ($entries as $entry) { + if ($entry === '.' || $entry === '..') { + continue; + } + + $srcDirectory = $packagesRoot . '/' . $entry . '/src'; + + if (is_dir($srcDirectory)) { + $srcDirectories[] = $srcDirectory; + } + } + + return $srcDirectories; + } + + /** + * @return list + * + * @throws UnexpectedValueException + */ + private function phpFilesUnder(string $directory): array + { + $iterator = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($directory, RecursiveDirectoryIterator::SKIP_DOTS), + ); + + $files = []; + + foreach ($iterator as $fileInfo) { + /** @var SplFileInfo $fileInfo */ + if ($fileInfo->isFile() && $fileInfo->getExtension() === 'php') { + $files[] = $fileInfo->getPathname(); + } + } + + return $files; + } + + /** + * Tokenizes the file and looks for a real `implements ... MiddlewareInterface` + * clause on a class declaration, rather than a text search — a plain + * substring or regex match would also fire on a string literal or + * comment that merely mentions `MiddlewareInterface`. + */ + private function implementsMiddlewareInterface(string $file): bool + { + $content = file_get_contents($file); + + if ($content === false) { + return false; + } + + $tokens = PhpToken::tokenize($content); + $inImplementsClause = false; + + foreach ($tokens as $token) { + if ($token->id === T_IMPLEMENTS) { + $inImplementsClause = true; + continue; + } + + if (!$inImplementsClause) { + continue; + } + + if ($token->text === '{') { + $inImplementsClause = false; + continue; + } + + $isNameToken = in_array( + $token->id, + [T_STRING, T_NAME_QUALIFIED, T_NAME_FULLY_QUALIFIED, T_NAME_RELATIVE], + true, + ); + + if ($isNameToken && str_ends_with($token->text, 'MiddlewareInterface')) { + return true; + } + } + + return false; + } +} From 47085eddeb7aa379ee50dbaab2193a8fa6a7adc5 Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Fri, 28 Aug 2026 18:32:05 -0400 Subject: [PATCH 2/5] feat: close inertia and transaction leaks found by the worker spike MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three additions driven by the RoadRunner state-leak spike, which audited every singleton, boot-time instance() binding, mutable static and process-global in the monorepo. Inertia::$shared is a container singleton whose shared props are merged into every subsequent render() and never cleared. In a booted-once process, middleware that shares the authenticated user once per request leaves that user visible to every following request's Inertia response. Inertia now implements ResettableInterface. ReadWriteConnection::reset() cleared the sticky-write flag but left a transaction open when a request threw before commit() or rollback(). That connection is pooled across requests, so the next request's writes were silently appended to the previous request's abandoned transaction. reset() now rolls back under an inTransaction() guard, with sticky-state clearing in a finally so a failing rollback cannot skip it. resolvedInstances() moves onto ContainerInterface. Application::$container is typed as the interface, so a worker could not reach the accessor added for exactly this purpose. Adding a method to the interface required updating container stubs in 14 test files across 8 packages — a BC break that belongs before 1.0 rather than in the follow-up worker PR. Closes #150 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NLREGwAgqDnHQANKShZ7qL --- ...-container-interface-resolved-instances.md | 37 ++++++++++ .../013-inertia-resettable.md | 44 ++++++++++++ .../014-readwrite-transaction-rollback.md | 39 +++++++++++ .claude/plans/response-decoration/_plan.md | 3 + .../core/src/Container/ContainerInterface.php | 9 +++ .../Unit/Container/ContainerInterfaceTest.php | 68 +++++++++++++++++++ .../Unit/Plugin/PluginInterceptionTest.php | 12 ++++ .../src/Connection/ReadWriteConnection.php | 18 ++++- .../Connection/ReadWriteConnectionTest.php | 63 +++++++++++++++++ .../tests/Integration/MySqlWiringTest.php | 8 +++ .../tests/Integration/PgSqlWiringTest.php | 8 +++ .../tests/Module/ModuleBootTest.php | 8 +++ .../docs/packages/database-readwrite.md | 6 +- .../tests/Unit/AdvancedErrorHandlerTest.php | 5 ++ packages/inertia/src/Inertia.php | 14 +++- packages/inertia/tests/InertiaTest.php | 48 +++++++++++++ packages/layout/tests/Unit/Helpers.php | 5 ++ .../Unit/SerializableNotificationJobTest.php | 10 +++ .../Middleware/PageCacheMiddlewareTest.php | 5 ++ packages/queue/tests/AsyncObserverJobTest.php | 10 +++ .../queue/tests/Feature/IntegrationTest.php | 20 ++++++ packages/queue/tests/WorkerTest.php | 10 +++ .../Jobs/DispatchWebhookJobRetryTest.php | 10 +++ .../tests/Jobs/DispatchWebhookJobTest.php | 5 ++ .../tests/Jobs/SerializableWebhookJobTest.php | 5 ++ 25 files changed, 466 insertions(+), 4 deletions(-) create mode 100644 .claude/plans/response-decoration/012-container-interface-resolved-instances.md create mode 100644 .claude/plans/response-decoration/013-inertia-resettable.md create mode 100644 .claude/plans/response-decoration/014-readwrite-transaction-rollback.md create mode 100644 packages/core/tests/Unit/Container/ContainerInterfaceTest.php diff --git a/.claude/plans/response-decoration/012-container-interface-resolved-instances.md b/.claude/plans/response-decoration/012-container-interface-resolved-instances.md new file mode 100644 index 00000000..f33c19ce --- /dev/null +++ b/.claude/plans/response-decoration/012-container-interface-resolved-instances.md @@ -0,0 +1,37 @@ +# Task 012: Expose resolvedInstances on ContainerInterface + +**Status**: completed +**Depends on**: 011 +**Retry count**: 0 + +## Description +Add `resolvedInstances()` to `ContainerInterface` so it is reachable through the type `Application::$container` is declared as. Without this, a long-running process cannot discover what to reset without reaching for the concrete `Container`. + +## Context +- Modify: `packages/core/src/Container/ContainerInterface.php` and confirm `packages/core/src/Container/Container.php` still satisfies it +- `Application::$container` is `public private(set) ContainerInterface` (`Application.php:64`), so `$app->container->resolvedInstances(...)` currently does not type-check — the accessor added in task 011 lives only on the concrete class. +- `Container` is the **only** implementor of `Marko\Core\Container\ContainerInterface` (verified). The interface extends `Psr\Container\ContainerInterface`. Adding a method is a BC break for any third-party implementor, which is why it belongs here, pre-1.0, rather than in the follow-up worker PR. +- Keep the signature identical to the concrete method so nothing else changes: `resolvedInstances(?string $interface = null): array`, returning already-resolved instances keyed by binding identifier, optionally filtered to those implementing a given interface. +- **It must never force instantiation** — that contract is the whole point and must be restated in the interface docblock. +- `packages/core/src` is covered by `phpstan.neon` at level 6 and must stay at zero errors. + +## Requirements (Test Descriptions) +- [x] `it declares resolved instances on the container contract` +- [x] `it resolves already built instances through the interface type` +- [x] `it filters resolved instances by interface through the interface type` +- [x] `it documents that the accessor never forces instantiation` + +## Acceptance Criteria +- All requirements have passing tests +- `Container` satisfies the extended interface with no signature change +- All pre-existing core tests pass unmodified +- PHPStan level 6 clean +- Code follows code standards + +## Implementation Notes +- Added `resolvedInstances(?string $interface = null): array` to `ContainerInterface` with a docblock restating the never-forces-instantiation contract (byte-identical signature to `Container::resolvedInstances()`, so `Container` satisfies it unchanged). +- New test file `packages/core/tests/Unit/Container/ContainerInterfaceTest.php` tests through the `ContainerInterface` type (not the concrete class) per the task's "test the contract" requirement — one test per requirement, using `assert($typed instanceof ContainerInterface)` to keep the static type as the interface when calling the method. +- `ContainerInterface` is implemented by test-double stubs across several other packages (anonymous/concrete classes in test files), which is a real BC break beyond `Container`. Added the minimal `resolvedInstances(): array { return []; }` (or equivalent, filtering/returning tracked instances where the stub already tracked them) method to each so the whole repo's test suite keeps compiling: `packages/core/tests/Unit/Plugin/PluginInterceptionTest.php`, `packages/webhook/tests/Jobs/{SerializableWebhookJobTest,DispatchWebhookJobRetryTest,DispatchWebhookJobTest}.php`, `packages/notification/tests/Unit/SerializableNotificationJobTest.php`, `packages/layout/tests/Unit/Helpers.php`, `packages/page-cache/tests/Unit/Middleware/PageCacheMiddlewareTest.php`, `packages/queue/tests/{WorkerTest,AsyncObserverJobTest,Feature/IntegrationTest}.php`, `packages/errors-advanced/tests/Unit/AdvancedErrorHandlerTest.php`. These are mechanical stub updates, not behavioral test changes. +- Deliberately left `packages/database-readwrite/tests/**` untouched — per task context, that package is being edited concurrently by a sibling worker and is out of scope; its `ContainerInterface` stubs will fail to compile until that worker (or a follow-up) adds the method. `packages/inertia` has no `ContainerInterface` implementors, so nothing to do there. +- Verified via a temporary scoped PHPUnit config (excluding `database-readwrite` and `inertia`) that the rest of the monorepo's test suite passes; the only other observed failures (`tests/PackagingTest.php`, `tests/IntegrationVerificationTest.php`) are pre-existing and caused by an unrelated, incomplete `packages/roadrunner` scaffold being built by a different concurrent process — unrelated to this change. +- Core suite: 571 tests passed. PHPStan (`packages/core/src`, level 6): no errors. PHPCS and php-cs-fixer: clean on all touched files. diff --git a/.claude/plans/response-decoration/013-inertia-resettable.md b/.claude/plans/response-decoration/013-inertia-resettable.md new file mode 100644 index 00000000..9d98c506 --- /dev/null +++ b/.claude/plans/response-decoration/013-inertia-resettable.md @@ -0,0 +1,44 @@ +# Task 013: Inertia Implements ResettableInterface + +**Status**: completed +**Depends on**: 008 +**Retry count**: 0 + +## Description +Make `Marko\Inertia\Inertia` implement `ResettableInterface`, clearing its shared props. This closes a verified cross-user data leak found by the roadrunner state-leak spike. + +## Context +- Modify: `packages/inertia/src/Inertia.php` + +**The leak, verified from source.** `Inertia::$shared` is `private array $shared = []` (line 22), written by the public `share()` API (line 36) and merged into every subsequent `render()` call's props. `packages/inertia/module.php` binds `Inertia` as a container **singleton**, and nothing ever clears `$shared`. + +In a booted-once process the typical usage pattern — middleware sharing the current authenticated user or flash data once per request — leaves that data visible to **every following request's** Inertia response until the same key is overwritten. If a later request shares a different key, both accumulate indefinitely. This is a cross-request, cross-user data leak, not merely memory growth. + +Under PHP-FPM the process ends after each request, so this is a behavioural no-op there — prove that by running the existing `packages/inertia/tests/` suite unchanged. + +- Implement `Marko\Core\Contracts\ResettableInterface` on the concrete class. Do **not** add a clearing method to any interface — follow the precedent set by `Session`, `SessionGuard` and `ReadWriteConnection`. +- `reset()` must be **non-destructive**: clear the in-memory per-request shared props, nothing else. +- Check whether `packages/inertia/composer.json` already requires `marko/core`; add it if not. +- Prove the leak with a failing test first: share a value, reset, then assert a subsequent `render()` does not carry it. + +## Requirements (Test Descriptions) +- [x] `it implements the resettable contract` +- [x] `it clears shared props when reset` +- [x] `it does not carry shared props into a later render after reset` +- [x] `it leaves shared props intact when not reset` + +## Acceptance Criteria +- All requirements have passing tests +- The leak is reproduced by a failing test before being fixed +- All pre-existing `packages/inertia/` tests pass unmodified +- No interface is modified +- Code follows code standards + +## Implementation Notes + +- `Marko\Inertia\Inertia` now `implements ResettableInterface` (concrete class only, per `Session`/`SessionGuard`/`ReadWriteConnection` precedent — no interface changed). +- `reset()` clears `$shared` to `[]`; nothing else is touched (non-destructive, request-scoped state only). +- The leak was proven first: "it does not carry shared props into a later render after reset" fails without `reset()` clearing `$shared` between two `render()` calls sharing the same `Inertia` instance — this reproduces the exact singleton cross-request leak described in the task. +- `packages/inertia/composer.json` already required `marko/core`; no change needed. +- All pre-existing `packages/inertia/tests/` (54 tests: 50 passed, 2 pre-existing risky, 1 pre-existing skipped — both environment-dependent SSR/curl tests, unrelated to this change) pass unmodified. +- `./vendor/bin/phpcs` clean on both touched files. `phpstan analyse packages/inertia` shows the same 22 pre-existing test-file errors with or without this change (verified via `git stash`) — none in `Inertia.php`; these are a subdirectory-scoping artifact bypassing root `phpstan.neon` exclusions, not caused by this task. diff --git a/.claude/plans/response-decoration/014-readwrite-transaction-rollback.md b/.claude/plans/response-decoration/014-readwrite-transaction-rollback.md new file mode 100644 index 00000000..3d1b0628 --- /dev/null +++ b/.claude/plans/response-decoration/014-readwrite-transaction-rollback.md @@ -0,0 +1,39 @@ +# Task 014: Roll Back Open Transactions on Reset + +**Status**: completed +**Depends on**: 010 +**Retry count**: 0 + +## Description +Extend `ReadWriteConnection::reset()` to roll back a transaction left open by a request that threw. Task 010 wired `reset()` to `resetStickyState()`, which clears the sticky-write flag but leaves an in-progress transaction alive on a pooled connection. + +## Context +- Modify: `packages/database-readwrite/src/Connection/ReadWriteConnection.php` + +**The leak, verified from source.** `reset()` currently delegates only to `resetStickyState()`. If a request calls `beginTransaction()` directly — not via the `transaction()` helper, which already wraps its work in `try/finally { $this->stickyWrite = false; }` — and then throws before `commit()`/`rollback()`, the underlying write connection is left with `inTransaction() === true`. That connection is pooled across requests in a long-running process, so **the next request's writes are silently appended to the previous request's abandoned transaction.** + +- Guard the rollback with `inTransaction()` so `reset()` stays safe to call when no transaction is open. +- Keep `resetStickyState()` public and its behaviour unchanged — it is documented in `packages/docs-markdown/docs/packages/database-readwrite.md` and removing or altering it is a BC break. +- A rollback that itself throws must not prevent the sticky-state reset. Decide the ordering deliberately and document it. +- Under PHP-FPM `reset()` is never called, so this is a no-op there — all pre-existing tests must pass unmodified. +- Update the package docs page to note that `reset()` also rolls back. + +## Requirements (Test Descriptions) +- [x] `it rolls back an open transaction when reset` +- [x] `it does not attempt a rollback when no transaction is open` +- [x] `it still clears sticky write state when reset` +- [x] `it clears sticky write state even when the rollback fails` + +## Acceptance Criteria +- All requirements have passing tests +- All pre-existing `packages/database-readwrite/` tests pass unmodified +- `resetStickyState()` remains public with unchanged behaviour +- Code follows code standards + +## Implementation Notes +- `reset()` now guards the rollback with `$this->write->inTransaction()`, calls `$this->write->rollback()` only when true, and always runs `resetStickyState()` in a `finally` block. If `rollback()` throws, the exception propagates after `resetStickyState()` has already run — deliberately not swallowed, per code standard rule 9 ("never silently catch and ignore"); the caller needs to know a rollback failed even though the connection's sticky flag is safely cleared. +- `resetStickyState()` is unchanged (still public, still just clears `$this->stickyWrite`) — no BC break. +- Added rollback-throw support to the `makeConnection()` test stub (`overrides['rollback']`) to test the "rollback fails" case; this is additive and does not change existing test behaviour, confirmed by the pre-existing `ReadWriteConnectionTest.php` suite (38 tests) still passing unmodified alongside the 4 new tests (42 total). +- `packages/database-readwrite/tests/Integration/MySqlWiringTest.php` currently fails independently of this change (`ContainerInterface::resolvedInstances` abstract-method error) — caused by a sibling worker's concurrent edit to `packages/core/src/Container/ContainerInterface.php`, not this task. +- Updated `packages/docs-markdown/docs/packages/database-readwrite.md`: the "Long-Running Processes" section now explains the rollback behaviour, and the API reference table row for `reset()` reflects it. +- Full-project `composer phpstan` — no errors. `phpcs` on touched files — clean. diff --git a/.claude/plans/response-decoration/_plan.md b/.claude/plans/response-decoration/_plan.md index b6eab26e..1cdc0c33 100644 --- a/.claude/plans/response-decoration/_plan.md +++ b/.claude/plans/response-decoration/_plan.md @@ -96,6 +96,9 @@ But `SessionMiddleware` is global middleware registered by the session drivers ( | 006 | Session cookie travels on the Response | 002, 003, 006a | completed | | 007 | Architecture test forbidding the rebuild pattern | 004 | completed | | 009 | Request-scoped Session and auth guard | 006, 008 | completed | +| 012 | Expose resolvedInstances on ContainerInterface | 011 | completed | +| 013 | Inertia implements ResettableInterface | 008 | completed | +| 014 | Roll back open transactions on reset | 010 | completed | Batches: **(1)** 001, 006a, 008, 011 → **(2)** 002, 010 → **(3)** 003, 004, 005 → **(4)** 006, 007 → **(5)** 009. diff --git a/packages/core/src/Container/ContainerInterface.php b/packages/core/src/Container/ContainerInterface.php index 99e80d2b..a7b94423 100644 --- a/packages/core/src/Container/ContainerInterface.php +++ b/packages/core/src/Container/ContainerInterface.php @@ -26,4 +26,13 @@ public function instance( * Invoke a callable with auto-resolved dependencies. */ public function call(Closure $callable): mixed; + + /** + * Instances already resolved, keyed by binding identifier. Never + * forces instantiation — returns only what has already been built. + * Pass an interface to return only instances implementing it. + * + * @return array + */ + public function resolvedInstances(?string $interface = null): array; } diff --git a/packages/core/tests/Unit/Container/ContainerInterfaceTest.php b/packages/core/tests/Unit/Container/ContainerInterfaceTest.php new file mode 100644 index 00000000..ebb4a181 --- /dev/null +++ b/packages/core/tests/Unit/Container/ContainerInterfaceTest.php @@ -0,0 +1,68 @@ +hasMethod('resolvedInstances'))->toBeTrue(); + + $method = $reflection->getMethod('resolvedInstances'); + $parameter = $method->getParameters()[0]; + + expect($parameter->getName())->toBe('interface') + ->and($parameter->allowsNull())->toBeTrue() + ->and($parameter->isDefaultValueAvailable())->toBeTrue() + ->and($parameter->getDefaultValue())->toBeNull(); +}); + +it('resolves already built instances through the interface type', function (): void { + $container = new Container(); + $container->singleton(ContainerInterfaceTestService::class); + $instance = $container->get(ContainerInterfaceTestService::class); + + $typed = $container; + assert($typed instanceof ContainerInterface); + + $resolved = $typed->resolvedInstances(); + + expect($resolved)->toHaveKey(ContainerInterfaceTestService::class) + ->and($resolved[ContainerInterfaceTestService::class])->toBe($instance); +}); + +it('filters resolved instances by interface through the interface type', function (): void { + $container = new Container(); + $container->singleton(ContainerInterfaceTestService::class); + $container->singleton(ContainerInterfaceTestOtherService::class); + $service = $container->get(ContainerInterfaceTestService::class); + $container->get(ContainerInterfaceTestOtherService::class); + + $typed = $container; + assert($typed instanceof ContainerInterface); + + $filtered = $typed->resolvedInstances(ContainerInterfaceTestServiceInterface::class); + + expect($filtered)->toHaveCount(1) + ->and($filtered[ContainerInterfaceTestService::class])->toBe($service); +}); + +it('documents that the accessor never forces instantiation', function (): void { + $method = (new ReflectionClass(ContainerInterface::class))->getMethod('resolvedInstances'); + $methodDoc = $method->getDocComment(); + + expect($methodDoc) + ->not->toBeFalse() + ->and($methodDoc) + ->toContain('Never') + ->and($methodDoc) + ->toContain('instantiation'); +}); diff --git a/packages/core/tests/Unit/Plugin/PluginInterceptionTest.php b/packages/core/tests/Unit/Plugin/PluginInterceptionTest.php index 11bca5b5..b1c63eb8 100644 --- a/packages/core/tests/Unit/Plugin/PluginInterceptionTest.php +++ b/packages/core/tests/Unit/Plugin/PluginInterceptionTest.php @@ -228,6 +228,18 @@ public function call(Closure $callable): mixed { return $callable(); } + + public function resolvedInstances(?string $interface = null): array + { + if ($interface === null) { + return $this->instances; + } + + return array_filter( + $this->instances, + fn (object $instance): bool => $instance instanceof $interface, + ); + } }; } diff --git a/packages/database-readwrite/src/Connection/ReadWriteConnection.php b/packages/database-readwrite/src/Connection/ReadWriteConnection.php index ba9e8164..45601d90 100644 --- a/packages/database-readwrite/src/Connection/ReadWriteConnection.php +++ b/packages/database-readwrite/src/Connection/ReadWriteConnection.php @@ -137,10 +137,26 @@ public function resetStickyState(): void $this->stickyWrite = false; } + /** + * Rolls back a transaction abandoned by a request that threw before + * commit()/rollback(), then clears the sticky-write flag. + * + * The rollback runs first (inside try) and the sticky-state reset + * runs in finally so it always happens, even if the rollback itself + * throws. The exception is intentionally not swallowed here: a + * failed rollback means the pooled connection may still be in an + * unknown transactional state, and the caller needs to know. + */ #[Override] public function reset(): void { - $this->resetStickyState(); + try { + if ($this->write->inTransaction()) { + $this->write->rollback(); + } + } finally { + $this->resetStickyState(); + } } /** diff --git a/packages/database-readwrite/tests/Connection/ReadWriteConnectionTest.php b/packages/database-readwrite/tests/Connection/ReadWriteConnectionTest.php index 7fc1ff25..ff21a56b 100644 --- a/packages/database-readwrite/tests/Connection/ReadWriteConnectionTest.php +++ b/packages/database-readwrite/tests/Connection/ReadWriteConnectionTest.php @@ -85,6 +85,10 @@ public function commit(): void public function rollback(): void { $this->calls[] = 'rollback'; + + if (isset($this->overrides['rollback'])) { + throw $this->overrides['rollback']; + } } public function inTransaction(): bool @@ -564,6 +568,65 @@ public function select(array $replicas): ConnectionInterface ->and($write->calls)->not->toContain(['query', 'SELECT 1', []]); }); + it('rolls back an open transaction when reset', function (): void { + $write = makeConnection(['inTransaction' => true]); + $replica = makeConnection(); + $selector = makeSelector($replica); + + $conn = new ReadWriteConnection($write, [$replica], $selector); + $conn->reset(); + + expect($write->calls)->toContain('rollback'); + }); + + it('does not attempt a rollback when no transaction is open', function (): void { + $write = makeConnection(['inTransaction' => false]); + $replica = makeConnection(); + $selector = makeSelector($replica); + + $conn = new ReadWriteConnection($write, [$replica], $selector); + $conn->reset(); + + expect($write->calls)->not->toContain('rollback'); + }); + + it('still clears sticky write state when reset', function (): void { + $write = makeConnection([ + 'inTransaction' => true, + 'query' => [['id' => 99]], + ]); + $replica = makeConnection(['query' => [['id' => 99]]]); + $selector = makeSelector($replica); + + $conn = new ReadWriteConnection($write, [$replica], $selector); + $conn->execute('INSERT INTO foo VALUES (1)'); + $conn->reset(); + $result = $conn->query('SELECT 1'); + + expect($result)->toBe([['id' => 99]]) + ->and($replica->calls)->toContain(['query', 'SELECT 1', []]); + }); + + it('clears sticky write state even when the rollback fails', function (): void { + $write = makeConnection([ + 'inTransaction' => true, + 'rollback' => new PDOException('rollback failed'), + 'query' => [['id' => 1]], + ]); + $replica = makeConnection(['query' => [['id' => 99]]]); + $selector = makeSelector($replica); + + $conn = new ReadWriteConnection($write, [$replica], $selector); + $conn->execute('INSERT INTO foo VALUES (1)'); + + expect(fn () => $conn->reset())->toThrow(PDOException::class, 'rollback failed'); + + $result = $conn->query('SELECT 1'); + + expect($result)->toBe([['id' => 99]]) + ->and($replica->calls)->toContain(['query', 'SELECT 1', []]); + }); + it('keeps the existing reset sticky state method available', function (): void { $write = makeConnection(['query' => [['id' => 1]]]); $replica = makeConnection(['query' => [['id' => 99]]]); diff --git a/packages/database-readwrite/tests/Integration/MySqlWiringTest.php b/packages/database-readwrite/tests/Integration/MySqlWiringTest.php index e531936e..6fa2036c 100644 --- a/packages/database-readwrite/tests/Integration/MySqlWiringTest.php +++ b/packages/database-readwrite/tests/Integration/MySqlWiringTest.php @@ -242,6 +242,14 @@ public function call(Closure $callable): mixed { return $callable($this); } + + /** + * @return array + */ + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; } diff --git a/packages/database-readwrite/tests/Integration/PgSqlWiringTest.php b/packages/database-readwrite/tests/Integration/PgSqlWiringTest.php index 0280d497..6f5149a5 100644 --- a/packages/database-readwrite/tests/Integration/PgSqlWiringTest.php +++ b/packages/database-readwrite/tests/Integration/PgSqlWiringTest.php @@ -225,6 +225,14 @@ public function call(Closure $callable): mixed { return $callable($this); } + + /** + * @return array + */ + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; } diff --git a/packages/database-readwrite/tests/Module/ModuleBootTest.php b/packages/database-readwrite/tests/Module/ModuleBootTest.php index 8601783b..45294d3d 100644 --- a/packages/database-readwrite/tests/Module/ModuleBootTest.php +++ b/packages/database-readwrite/tests/Module/ModuleBootTest.php @@ -206,6 +206,14 @@ public function call(Closure $callable): mixed { return $callable($this); } + + /** + * @return array + */ + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; } diff --git a/packages/docs-markdown/docs/packages/database-readwrite.md b/packages/docs-markdown/docs/packages/database-readwrite.md index 262134c3..26f84bfa 100644 --- a/packages/docs-markdown/docs/packages/database-readwrite.md +++ b/packages/docs-markdown/docs/packages/database-readwrite.md @@ -181,7 +181,9 @@ Sticky writes (via `execute()` or `beginTransaction()`) bypass all replicas enti In PHP-FPM the sticky flag is cleared automatically at the end of each request because each request is a new process. In a queue worker or other long-running process the sticky flag persists for the lifetime of the process. Call `resetStickyState()` between jobs to restore replica routing: -`ReadWriteConnection` also implements `Marko\Core\Contracts\ResettableInterface`, so a worker that resets every registered `ResettableInterface` implementation between requests will clear the sticky flag automatically via `reset()`, which delegates to `resetStickyState()`. Calling `resetStickyState()` directly remains supported for callers that don't go through the contract. +`ReadWriteConnection` also implements `Marko\Core\Contracts\ResettableInterface`, so a worker that resets every registered `ResettableInterface` implementation between requests will clear the sticky flag automatically via `reset()`. Calling `resetStickyState()` directly remains supported for callers that don't go through the contract. + +Beyond clearing the sticky flag, `reset()` also rolls back any transaction left open by a request that called `beginTransaction()` directly and then threw before `commit()`/`rollback()`. Without this, the underlying write connection stays mid-transaction on the pooled connection, and the next request's writes would silently land inside the previous request's abandoned transaction. The rollback only runs when a transaction is actually open; if the rollback itself throws, the sticky flag is still cleared before the exception propagates, so the connection is never left permanently sticky even when a reset only partially succeeds. ```php use Marko\Database\ReadWrite\Connection\ReadWriteConnection; @@ -256,7 +258,7 @@ Implements `ConnectionInterface`, `TransactionInterface`, and `ResettableInterfa | `transaction(callable $callback): mixed` | Write (sets sticky temporarily) | Run a callback inside an auto-managed transaction; sticky flag is set for the callback duration and cleared on completion | | `driverName(): string` | Write (delegates) | Return the write connection's driver name (e.g. `'mysql'`, `'pgsql'`) | | `resetStickyState(): void` | — | Clear the sticky flag; subsequent reads route to replicas again | -| `reset(): void` | — | `ResettableInterface` contract method; delegates to `resetStickyState()` | +| `reset(): void` | — | `ResettableInterface` contract method; rolls back an open transaction (if any) and clears the sticky flag | ### ReadException diff --git a/packages/errors-advanced/tests/Unit/AdvancedErrorHandlerTest.php b/packages/errors-advanced/tests/Unit/AdvancedErrorHandlerTest.php index fde2fe33..6989fb11 100644 --- a/packages/errors-advanced/tests/Unit/AdvancedErrorHandlerTest.php +++ b/packages/errors-advanced/tests/Unit/AdvancedErrorHandlerTest.php @@ -398,6 +398,11 @@ public function call(Closure $callable): mixed { return $callable($this); } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; ($module['boot'])($container); diff --git a/packages/inertia/src/Inertia.php b/packages/inertia/src/Inertia.php index dd5202c4..c857f36a 100644 --- a/packages/inertia/src/Inertia.php +++ b/packages/inertia/src/Inertia.php @@ -8,15 +8,17 @@ use JsonException; use Marko\Config\ConfigRepositoryInterface; use Marko\Config\Exceptions\ConfigException; +use Marko\Core\Contracts\ResettableInterface; use Marko\Inertia\Exceptions\InertiaConfigurationException; use Marko\Inertia\Ssr\SsrClient; use Marko\Routing\Http\Request; use Marko\Routing\Http\Response; use Marko\Session\Contracts\SessionInterface; use Marko\Vite\Vite; +use Override; use stdClass; -class Inertia +class Inertia implements ResettableInterface { /** @var array */ private array $shared = []; @@ -48,6 +50,16 @@ public function share( $this->shared[$key] = $value; } + /** + * Clear shared props so a long-running worker does not leak them + * into a subsequent request's Inertia response. + */ + #[Override] + public function reset(): void + { + $this->shared = []; + } + /** * Flash a message to the session for the next request. * diff --git a/packages/inertia/tests/InertiaTest.php b/packages/inertia/tests/InertiaTest.php index 80b68867..9e0f8c4a 100644 --- a/packages/inertia/tests/InertiaTest.php +++ b/packages/inertia/tests/InertiaTest.php @@ -2,6 +2,7 @@ declare(strict_types=1); +use Marko\Core\Contracts\ResettableInterface; use Marko\Core\Path\ProjectPaths; use Marko\Inertia\Exceptions\InertiaConfigurationException; use Marko\Inertia\Inertia; @@ -134,6 +135,53 @@ function createInertia(array $config = [], array $viteConfig = []): Inertia ->and($data['props']['user']['name'])->toBe('Test'); }); +test('it implements the resettable contract', function () { + $inertia = createInertia(); + + expect($inertia)->toBeInstanceOf(ResettableInterface::class); +}); + +test('it clears shared props when reset', function () { + $inertia = createInertia(); + $inertia->share('user', ['name' => 'Alice']); + + $inertia->reset(); + + $request = new Request(server: ['HTTP_X_INERTIA' => 'true']); + $response = $inertia->render($request, 'Dashboard'); + + $data = json_decode($response->body(), true); + expect($data['props'])->not->toHaveKey('user'); +}); + +test('it does not carry shared props into a later render after reset', function () { + $inertia = createInertia(); + $inertia->share('user', ['name' => 'Alice']); + + $request = new Request(server: ['HTTP_X_INERTIA' => 'true']); + $firstResponse = $inertia->render($request, 'Dashboard'); + $firstData = json_decode($firstResponse->body(), true); + + $inertia->reset(); + + $secondResponse = $inertia->render($request, 'Dashboard'); + $secondData = json_decode($secondResponse->body(), true); + + expect($firstData['props']['user']['name'])->toBe('Alice') + ->and($secondData['props'])->not->toHaveKey('user'); +}); + +test('it leaves shared props intact when not reset', function () { + $inertia = createInertia(); + $inertia->share('user', ['name' => 'Alice']); + + $request = new Request(server: ['HTTP_X_INERTIA' => 'true']); + $response = $inertia->render($request, 'Dashboard'); + + $data = json_decode($response->body(), true); + expect($data['props']['user']['name'])->toBe('Alice'); +}); + test('inertia location redirect returns x-inertia-location header', function () { $inertia = createInertia(); $response = $inertia->location('https://example.com'); diff --git a/packages/layout/tests/Unit/Helpers.php b/packages/layout/tests/Unit/Helpers.php index 866e140d..7f828a97 100644 --- a/packages/layout/tests/Unit/Helpers.php +++ b/packages/layout/tests/Unit/Helpers.php @@ -34,6 +34,11 @@ public function call(Closure $callable): mixed { return $callable(); } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; } } diff --git a/packages/notification/tests/Unit/SerializableNotificationJobTest.php b/packages/notification/tests/Unit/SerializableNotificationJobTest.php index bfd7beef..0c53f8c6 100644 --- a/packages/notification/tests/Unit/SerializableNotificationJobTest.php +++ b/packages/notification/tests/Unit/SerializableNotificationJobTest.php @@ -204,6 +204,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; $unserialized->setContainer($container); @@ -343,6 +348,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; $envelope = createNotificationTestEnvelope(); diff --git a/packages/page-cache/tests/Unit/Middleware/PageCacheMiddlewareTest.php b/packages/page-cache/tests/Unit/Middleware/PageCacheMiddlewareTest.php index 31730c1b..88bf0e0b 100644 --- a/packages/page-cache/tests/Unit/Middleware/PageCacheMiddlewareTest.php +++ b/packages/page-cache/tests/Unit/Middleware/PageCacheMiddlewareTest.php @@ -137,6 +137,11 @@ public function call(Closure $callable): mixed { return $callable(); } + + public function resolvedInstances(?string $interface = null): array + { + return $this->instances; + } } // ─── Helpers ────────────────────────────────────────────────────────────────── diff --git a/packages/queue/tests/AsyncObserverJobTest.php b/packages/queue/tests/AsyncObserverJobTest.php index 33811265..7daebd53 100644 --- a/packages/queue/tests/AsyncObserverJobTest.php +++ b/packages/queue/tests/AsyncObserverJobTest.php @@ -45,6 +45,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; } @@ -277,6 +282,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; $job = new AsyncObserverJob( diff --git a/packages/queue/tests/Feature/IntegrationTest.php b/packages/queue/tests/Feature/IntegrationTest.php index 2d6fdda4..ac47d932 100644 --- a/packages/queue/tests/Feature/IntegrationTest.php +++ b/packages/queue/tests/Feature/IntegrationTest.php @@ -52,6 +52,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; } @@ -372,6 +377,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; $poppedJob->setContainer($container); @@ -437,6 +447,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; // Simulate EventDispatcher: wraps serialized event and pushes the job @@ -510,6 +525,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; $job = new AsyncObserverJob( diff --git a/packages/queue/tests/WorkerTest.php b/packages/queue/tests/WorkerTest.php index 6282c1b1..2163726f 100644 --- a/packages/queue/tests/WorkerTest.php +++ b/packages/queue/tests/WorkerTest.php @@ -51,6 +51,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; } @@ -79,6 +84,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; } diff --git a/packages/webhook/tests/Jobs/DispatchWebhookJobRetryTest.php b/packages/webhook/tests/Jobs/DispatchWebhookJobRetryTest.php index 017de29b..771f4eb6 100644 --- a/packages/webhook/tests/Jobs/DispatchWebhookJobRetryTest.php +++ b/packages/webhook/tests/Jobs/DispatchWebhookJobRetryTest.php @@ -137,6 +137,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; // Attempt 1: first failure should re-queue with delay = 60 * 2^1 = 120 @@ -270,6 +275,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; // Attempt 3 = max_retries, should NOT re-queue diff --git a/packages/webhook/tests/Jobs/DispatchWebhookJobTest.php b/packages/webhook/tests/Jobs/DispatchWebhookJobTest.php index 507501f4..ccfadbd1 100644 --- a/packages/webhook/tests/Jobs/DispatchWebhookJobTest.php +++ b/packages/webhook/tests/Jobs/DispatchWebhookJobTest.php @@ -136,6 +136,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; $job = new DispatchWebhookJob($payload); diff --git a/packages/webhook/tests/Jobs/SerializableWebhookJobTest.php b/packages/webhook/tests/Jobs/SerializableWebhookJobTest.php index faac837e..06bf74b4 100644 --- a/packages/webhook/tests/Jobs/SerializableWebhookJobTest.php +++ b/packages/webhook/tests/Jobs/SerializableWebhookJobTest.php @@ -166,6 +166,11 @@ public function call(Closure $callable): mixed { return null; } + + public function resolvedInstances(?string $interface = null): array + { + return []; + } }; } } From bed06c511f1a01f57d6580337cdedee7af8f378a Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Fri, 28 Aug 2026 19:37:04 -0400 Subject: [PATCH 3/5] fix(session): make the shutdown-handler test independent of file ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SessionShutdownHandlerTest proves session_set_save_handler() is called once per process by defining a Marko\Session\session_set_save_handler() shim that PHP resolves ahead of the global function. The shim lived inside the test file itself, which made the test depend on load order. PHP caches the resolved target on a call site's opline the first time it executes. If any earlier test in the same process reached Session::configure() before the test file was loaded, that opline bound permanently to the global function, the spy never fired, and the assertion saw a count of 0 — a failure that depended entirely on how paratest distributed files across worker processes. It passed on this branch by luck of the current distribution and failed on roughly 83% of parallel runs on a branch that merely added unrelated test files elsewhere in the repo. Sequential runs were always clean, which is what made it look like cross-process interference rather than a load-order bug. Moving the shim into an autoload-dev files entry guarantees it is defined before any test can resolve that call site, in every process. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NLREGwAgqDnHQANKShZ7qL --- .gitignore | 6 + composer.json | 3 +- .../session/tests/SessionFunctionShim.php | 34 ++++ .../tests/Unit/SessionShutdownHandlerTest.php | 180 ++++++++---------- 4 files changed, 120 insertions(+), 103 deletions(-) create mode 100644 packages/session/tests/SessionFunctionShim.php diff --git a/.gitignore b/.gitignore index 4894e505..937d9750 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,9 @@ **/.phpunit.cache/ composer.lock vendor/ + +# `rr:serve` and `vendor/bin/rr get-binary` both write into the project root: +# a downloaded platform-specific binary and a generated default config. Local +# dev conveniences, never committed. +/rr +/.rr.yaml diff --git a/composer.json b/composer.json index ff1cd6b0..fb7e3186 100644 --- a/composer.json +++ b/composer.json @@ -611,7 +611,8 @@ "packages/devai/tests/Helpers.php", "packages/inertia/tests/Helpers.php", "packages/ratelimiter/tests/Helpers.php", - "packages/security/tests/Helpers.php" + "packages/security/tests/Helpers.php", + "packages/session/tests/SessionFunctionShim.php" ] } } diff --git a/packages/session/tests/SessionFunctionShim.php b/packages/session/tests/SessionFunctionShim.php new file mode 100644 index 00000000..f3f428ad --- /dev/null +++ b/packages/session/tests/SessionFunctionShim.php @@ -0,0 +1,34 @@ + 'array', + 'session.lifetime' => 120, + 'session.expire_on_close' => false, + 'session.path' => '/tmp', + 'session.cookie.name' => 'PHPSESSID', + 'session.cookie.path' => '/', + 'session.cookie.domain' => '', + 'session.cookie.secure' => false, + 'session.cookie.httponly' => true, + 'session.cookie.samesite' => 'lax', + 'session.gc_probability' => 1, + 'session.gc_divisor' => 100, + ])); + + $handler = new class () implements SessionHandlerInterface { - $GLOBALS['sessionSetSaveHandlerCallCount'] = ($GLOBALS['sessionSetSaveHandlerCallCount'] ?? 0) + 1; - - return \session_set_save_handler(...$arguments); - } -} - -namespace { - use Marko\Session\Config\SessionConfig; - use Marko\Session\Contracts\SessionHandlerInterface; - use Marko\Session\Session; - use Marko\Testing\Fake\FakeConfigRepository; - - it('registers the session save handler shutdown function only once', function (): void { - $GLOBALS['sessionSetSaveHandlerCallCount'] = 0; - - $sessionConfig = new SessionConfig(new FakeConfigRepository([ - 'session.driver' => 'array', - 'session.lifetime' => 120, - 'session.expire_on_close' => false, - 'session.path' => '/tmp', - 'session.cookie.name' => 'PHPSESSID', - 'session.cookie.path' => '/', - 'session.cookie.domain' => '', - 'session.cookie.secure' => false, - 'session.cookie.httponly' => true, - 'session.cookie.samesite' => 'lax', - 'session.gc_probability' => 1, - 'session.gc_divisor' => 100, - ])); - - $handler = new class () implements SessionHandlerInterface + /** @var array */ + public array $written = []; + + public function open( + string $path, + string $name, + ): bool { + return true; + } + + public function close(): bool + { + return true; + } + + public function read(string $id): string|false + { + return $this->written[$id] ?? ''; + } + + public function write( + string $id, + string $data, + ): bool { + $this->written[$id] = $data; + + return true; + } + + public function destroy(string $id): bool { - /** @var array */ - public array $written = []; - - public function open( - string $path, - string $name, - ): bool { - return true; - } - - public function close(): bool - { - return true; - } - - public function read(string $id): string|false - { - return $this->written[$id] ?? ''; - } - - public function write( - string $id, - string $data, - ): bool { - $this->written[$id] = $data; - - return true; - } - - public function destroy(string $id): bool - { - unset($this->written[$id]); - - return true; - } - - public function gc(int $max_lifetime): int|false - { - return 0; - } - }; - - $session = new Session($handler, $sessionConfig); - - // Request 1 - $session->start(); - $session->save(); - $session->reset(); - - // Request 2, in the same long-running process - $session->start(); - $session->save(); - - expect($GLOBALS['sessionSetSaveHandlerCallCount'])->toBe(1); - }); -} + unset($this->written[$id]); + + return true; + } + + public function gc(int $max_lifetime): int|false + { + return 0; + } + }; + + $session = new Session($handler, $sessionConfig); + + // Request 1 + $session->start(); + $session->save(); + $session->reset(); + + // Request 2, in the same long-running process + $session->start(); + $session->save(); + + expect($GLOBALS['sessionSetSaveHandlerCallCount'])->toBe(1); +}); From 61668d42059a60f9ee960b4b34f4c11e23e27f58 Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Sat, 29 Aug 2026 08:36:00 -0400 Subject: [PATCH 4/5] test(core): stop pinning incidental prose in the resettable contract test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docblock assertions included `toContain('PHP-FPM')`, which is an example runtime mentioned in passing rather than part of the contract. Rewording that sentence — changing no behaviour and no contract — would have failed the test. Keeps the two phrases that ARE the contract: an implementor reading nothing but the interface must not mistake reset() for a destructive teardown. A comment records why those two and not the rest, so the incidental assertions do not creep back. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NLREGwAgqDnHQANKShZ7qL --- .../Unit/Contracts/ResettableInterfaceTest.php | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/core/tests/Unit/Contracts/ResettableInterfaceTest.php b/packages/core/tests/Unit/Contracts/ResettableInterfaceTest.php index 7c9520f8..74130b98 100644 --- a/packages/core/tests/Unit/Contracts/ResettableInterfaceTest.php +++ b/packages/core/tests/Unit/Contracts/ResettableInterfaceTest.php @@ -35,16 +35,23 @@ public function reset(): void ->toBe(''); }); +/* + * Asserts only on the two phrases that ARE the contract — an implementor + * reading nothing but this interface must not mistake reset() for a + * destructive teardown. Deliberately does not assert on incidental prose + * such as the example runtimes named in the docblock: those are free to be + * reworded, and pinning them would make this test fail on edits that change + * no behaviour and no contract. + */ it('documents that reset is non destructive', function (): void { - $classDoc = (new ReflectionClass(ResettableInterface::class))->getDocComment(); - $methodDoc = (new ReflectionClass(ResettableInterface::class))->getMethod('reset')->getDocComment(); + $reflection = new ReflectionClass(ResettableInterface::class); + $classDoc = $reflection->getDocComment(); + $methodDoc = $reflection->getMethod('reset')->getDocComment(); expect($classDoc) ->not->toBeFalse() ->and($classDoc) ->toContain('non-destructive') - ->and($classDoc) - ->toContain('PHP-FPM') ->and($methodDoc) ->not->toBeFalse() ->and($methodDoc) From 1ad0bd5f6b3d399199bb08e921c8f015d880fe89 Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Sat, 29 Aug 2026 08:40:22 -0400 Subject: [PATCH 5/5] style: apply php-cs-fixer to the migrated middleware tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's Lint job runs `php-cs-fixer --dry-run`, which fails the build on any file the fixer would change. Two test files touched by the decoration migration had `use function` imports placed where the fixer wants them ordered differently. These were noted during the standards pass and left as "the pre-commit hook handles it" — true locally, but CI checks rather than fixes, so the build went red. Running the fixer is the whole remedy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NLREGwAgqDnHQANKShZ7qL --- packages/inertia/tests/Middleware/InertiaMiddlewareTest.php | 6 ++++-- packages/ratelimiter/tests/Unit/RateLimitMiddlewareTest.php | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/inertia/tests/Middleware/InertiaMiddlewareTest.php b/packages/inertia/tests/Middleware/InertiaMiddlewareTest.php index 1561ceac..2ad236ec 100644 --- a/packages/inertia/tests/Middleware/InertiaMiddlewareTest.php +++ b/packages/inertia/tests/Middleware/InertiaMiddlewareTest.php @@ -4,12 +4,14 @@ use Marko\Inertia\Exceptions\InertiaConfigurationException; use Marko\Inertia\Middleware\InertiaMiddleware; + +use function Marko\Inertia\Tests\createTaggedResponse; + use Marko\Inertia\Tests\TaggedResponse; use Marko\Routing\Http\Request; use Marko\Routing\Http\Response; -use Marko\Testing\Fake\FakeConfigRepository; -use function Marko\Inertia\Tests\createTaggedResponse; +use Marko\Testing\Fake\FakeConfigRepository; beforeEach(function (): void { $this->middleware = new InertiaMiddleware(new FakeConfigRepository([ diff --git a/packages/ratelimiter/tests/Unit/RateLimitMiddlewareTest.php b/packages/ratelimiter/tests/Unit/RateLimitMiddlewareTest.php index 47bc423f..eedb4b61 100644 --- a/packages/ratelimiter/tests/Unit/RateLimitMiddlewareTest.php +++ b/packages/ratelimiter/tests/Unit/RateLimitMiddlewareTest.php @@ -7,13 +7,15 @@ use Marko\RateLimiter\Exceptions\ClientIpException; use Marko\RateLimiter\Middleware\RateLimitMiddleware; use Marko\RateLimiter\RateLimitResult; + +use function Marko\RateLimiter\Tests\createTaggedResponse; + use Marko\RateLimiter\Tests\TaggedResponse; use Marko\Routing\Http\Request; use Marko\Routing\Http\Response; use Marko\Routing\Middleware\MiddlewareInterface; -use Marko\Testing\Fake\FakeConfigRepository; -use function Marko\RateLimiter\Tests\createTaggedResponse; +use Marko\Testing\Fake\FakeConfigRepository; function createMockLimiter( RateLimitResult $result,