fix: apply middleware headers to intercept route and server action responses#762
Conversation
…sponses Middleware response headers (_mwCtx.headers) were silently dropped on three code paths: intercepting route responses, server action re-render responses, and server action redirect responses. All three constructed Response objects directly without merging middleware headers, unlike the normal page path which goes through buildAppPageRscResponse(). Extract mergeMiddlewareResponseHeaders() from the inline logic in buildAppPageRscResponse() and call it in all three paths. This ensures auth cookies, CORS headers, and security headers set in middleware are applied consistently to all response types.
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90271239d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "x-action-redirect-type": actionRedirect.type, | ||
| "x-action-redirect-status": String(actionRedirect.status), | ||
| }); | ||
| __mergeMiddlewareResponseHeaders(redirectHeaders, _mwCtx.headers); |
There was a problem hiding this comment.
In the action-redirect branch, the code builds x-action-redirect* headers and then merges middleware headers afterward; because mergeMiddlewareResponseHeaders() uses set() for most keys, middleware can overwrite these control headers. If a middleware sets x-action-redirect, x-action-redirect-type, or x-action-redirect-status (intentionally or via header passthrough), the client can navigate to the wrong URL or fail to process the redirect correctly. Merge middleware headers before writing the action redirect control headers, or explicitly protect these keys from override.
Useful? React with 👍 / 👎.
Codex review correctly identified that mergeMiddlewareResponseHeaders() uses set() for most keys, meaning middleware headers could overwrite the x-action-redirect* control headers if a middleware happened to set them. Fix by merging middleware headers first, then writing the x-action-redirect* headers afterward so they are always authoritative. The framework's own redirect control signals must survive regardless of what middleware sets.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix. The three code paths (intercept route, action re-render, action redirect) were silently dropping middleware headers — auth cookies and security headers vanishing is a real production problem, and this is the right fix.
The extraction of mergeMiddlewareResponseHeaders() is clean, the merge ordering in the redirect path correctly protects x-action-redirect* control headers (addressed in d0bf61a), and the test coverage is solid (unit tests on the helper, integration test on intercept, generated-code assertions on all three paths).
Two minor observations:
-
Pre-existing HTML/RSC parity gap:
buildAppPageHtmlResponse(line 241) still usesappendfor all middleware headers, whilebuildAppPageRscResponsenow delegates to the extracted helper withset()-for-singular /append-for-Set-Cookie-and-Vary. This means middleware settingCache-Controlwill override on the RSC path but append (producing an invalid multi-value) on the HTML path. Not introduced by this PR, but worth a follow-up to alignbuildAppPageHtmlResponsewithmergeMiddlewareResponseHeaderstoo. -
Minor:
Headersiterator keys are already lowercase per spec, sokey.toLowerCase()on line 179 is technically redundant — but it makes intent clear, so fine to keep.
| return; | ||
| } | ||
|
|
||
| for (const [key, value] of middlewareHeaders) { |
There was a problem hiding this comment.
Nit: Headers iterator always yields lowercase keys per the Fetch spec, so key.toLowerCase() here is a no-op. Harmless and defensive — just noting for awareness.
|
Review submitted and approved. Here's a summary of the review: Approved — This is a solid bug fix addressing middleware headers being silently dropped on three code paths: intercepting route responses, server action re-render responses, and server action redirect responses. Key findings:
Two minor observations noted in the review:
|
|
5 approved in a row. God damn that hit the spot |
2 participants
Summary
_mwCtx.headers) were silently dropped on three code paths: intercepting route responses, server action re-render responses, and server action redirect responsesResponseobjects directly without merging middleware headers, unlike the normal page path which goes throughbuildAppPageRscResponse()mergeMiddlewareResponseHeaders()frombuildAppPageRscResponse()and applied it to all three paths, plus refactored the original caller to use the shared helperTest plan
mergeMiddlewareResponseHeadershelper (null-safe, Set-Cookie/Vary append, other headers set)mergeMiddlewareResponseHeadersis called in intercept, action re-render, and action redirect paths