Skip to content

feat: response decoration api with cookie support - #152

Merged
markshust merged 5 commits into
developfrom
feature/response-decoration
Aug 29, 2026
Merged

feat: response decoration api with cookie support#152
markshust merged 5 commits into
developfrom
feature/response-decoration

Conversation

@markshust

Copy link
Copy Markdown
Collaborator

Gives Response a decoration API and first-class cookie support, and makes request-scoped singleton state explicitly clearable.

The live bug this fixes

StreamingResponse extends Response carries its payload in an SseStream with body = ''. Five middleware added headers by constructing a brand-new base Response from the previous response's getters:

// packages/security/src/Middleware/SecurityHeadersMiddleware.php — before
return new Response(
    body:       $response->body(),
    statusCode: $response->statusCode(),
    headers:    array_merge($response->headers(), $securityHeaders),
);

Any of them in front of an SSE route downgraded the StreamingResponse to a plain Response — the stream was discarded, the overridden send() never ran, and the client got an empty 200 with correct headers. SecurityHeadersMiddleware is exactly the kind of middleware apps register globally.

All six rebuild sites across five files now decorate instead, and tests/MiddlewareDecorationTest.php fails the build if the pattern reappears in any middleware.

Why Response is no longer readonly

Verified empirically on PHP 8.5.1 before committing to the design:

Approach Result
clone $this with { ... } Not available in 8.5.1 — parse error
readonly class + clone then assign Fails: Cannot modify readonly property
readonly class + ReflectionProperty::setValue Fails: same
Plain class, private non-readonly props, clone + assign Works — preserves subclass, subclass state, original untouched

clone is required because StreamingResponse::__construct(SseStream, int) has a different signature from its parent, so new static(...) cannot reconstruct it. Immutability is now enforced by API design — private properties, no setters, with*() returns copies. json()/html()/redirect() deliberately keep new self() for the same reason.

Session cookie moves onto the Response

setcookie() is gone from the codebase. The cookie now travels on the Response, which makes it visible to middleware, assertable without superglobal fixtures, and able to reach a client under a SAPI where setcookie() is a no-op.

Three subtleties worth reviewing closely:

  • session.use_cookies = 0 disables session ID reading, not just writing, so SessionMiddleware seeds the ID from the inbound request cookie itself. Without this, every request would silently start a fresh empty session.
  • A tampered inbound cookie fails setId()'s validation. That value is attacker-controlled, so it is ignored in favour of a fresh session rather than surfacing as a 500.
  • The cookie attaches only when the ID changed (new, regenerated, or expired-on-destroy), matching what session_start() did before. Attaching unconditionally would have silently disabled the page cache for every session-enabled app, since SessionMiddleware runs inside PageCacheMiddleware and cookie-bearing responses are never cached. tests/Integration/PageCacheSessionMiddlewareTest.php guards this.

Cross-user leaks closed

Three verified leaks that only manifest in a booted-once process, each reproduced by a failing test before being fixed:

  1. Session reused the previous request's ID. save() left $this->id populated and start() fed it back to session_id(). On a singleton, an anonymous visitor following an authenticated one resumed the previous user's session.
  2. SessionGuard::$cachedUser persisted across requests on a singleton guard, cleared only by the destructive logout().
  3. session_set_save_handler($handler, true) ran on every start(), registering a shutdown function per request — unbounded growth in a worker.

SessionInterface and GuardInterface are byte-identical; every marko/testing fake still satisfies them unedited. ResettableInterface is implemented on the concrete classes for exactly that reason.

Also

  • Cookie value object — rawurlencode()d values so a; b cannot inject a second attribute, SameSite=None without Secure throws rather than emitting a cookie browsers silently drop, and lifetime => 0 omits Expires entirely rather than sending an epoch
  • Request::cookie() plus $_COOKIE capture in fromGlobals()
  • Response::headerLines() — SAPI-free header emission, consumed by both send() implementations
  • Page cache refuses to store cookie-bearing responses — a security boundary, since a cached Set-Cookie would be replayed to every later visitor
  • ResettableInterface in core, implemented by Session, SessionGuard and ReadWriteConnection; Container::resolvedInstances() discovers them without forcing instantiation

Verification

6972 tests passing, 0 failures. PHPStan: no errors. PHPCS: clean.

Known follow-up

Container::resolvedInstances() is on the concrete Container, but Application::$container is typed ContainerInterface, so it is not reachable through the interface. Container is the only implementor, so adding it there is low-risk — but it is a BC-breaking interface change and belongs before 1.0 rather than in the follow-up worker PR. Deliberately left out of this PR's scope.

Closes #150

🤖 Generated with Claude Code

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLREGwAgqDnHQANKShZ7qL
@github-actions github-actions Bot added the enhancement New feature or request label Aug 28, 2026
markshust and others added 2 commits August 28, 2026 18:32
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLREGwAgqDnHQANKShZ7qL
…ring

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLREGwAgqDnHQANKShZ7qL
markshust and others added 2 commits August 29, 2026 08:36
…test

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLREGwAgqDnHQANKShZ7qL
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NLREGwAgqDnHQANKShZ7qL
@markshust
markshust merged commit 4181f81 into develop Aug 29, 2026
4 checks passed
@markshust
markshust deleted the feature/response-decoration branch August 29, 2026 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Response cannot be decorated — middleware rebuild silently downgrades StreamingResponse

1 participant