Repository navigation
fix(pages): append edge API route headers after middleware's - #4079
james-elicx wants to merge 1 commit into
Conversation
|
/bigbonk review for issues |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| } | ||
| for (const cookie of middlewareHeaders?.getSetCookie() ?? []) | ||
| headers.append("Set-Cookie", cookie); | ||
| for (const [name, value] of response.headers) { |
There was a problem hiding this comment.
Next.js does not actually emit an exact duplicate here: NodeNextResponse.appendHeader() reads the existing values and only adds value when it is not already present. Headers.append() always adds it, so if middleware and the edge handler both return (for example) x-custom: same or the same Set-Cookie, Workers/hybrid/prod now send same, same while Next.js sends it once. The dev implementation at api-handler.ts:452-455 has the same issue because native ServerResponse.appendHeader() also does not perform Next’s wrapper-level deduplication. Please preserve the existing-value check in both paths, including per-cookie values.
|
Review: 1 findings. Posted 1 actionable inline comment. |
Performance benchmarksCompared 1 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
e4fd6d5 to
417fc93
Compare
|
/bigbonk review for issues |
| // cookies are added ahead of the handler's by the pipeline. | ||
| const initialResponseHeaders = new Headers(stagedHeaders); | ||
| initialResponseHeaders.delete("Content-Length"); | ||
| initialResponseHeaders.delete("Set-Cookie"); |
There was a problem hiding this comment.
initialResponseHeaders drops Set-Cookie, so the duplicate check in appendEdgeApiResponseHeaders() never sees middleware cookies in the Pages-only production path. pages-request-pipeline.ts later prepends the staged cookies in mergeHeaders() without deduplicating them. Consequently, when middleware and the edge handler both emit the same cookie, this path still returns it twice even though Next.js returns it once. Keep the cookies available for the handler-level deduplication and strip the inherited copy before the pipeline recomposes them (as the response-stage path does), or deduplicate at this boundary.
| ]); | ||
| const append = (name: string, value: string) => { | ||
| const current = values.get(name) ?? []; | ||
| if (!current.includes(value)) values.set(name, [...current, value]); |
There was a problem hiding this comment.
This exact-value deduplication removes the evidence that the edge handler authored a CDN policy. If middleware and the handler both set the same provider cache header, the merged response equals middlewareResponseHeaders, so hasHandlerCdnResponsePolicy() returns false. The base implementation explicitly marked the handler policy before merging; without that marker, explicitProviderOutcome() can reject shared-cache admission for a handler-authored policy. Record the edge response policy before composition, including when its value duplicates middleware.
|
Review: 2 findings. Posted 2 actionable inline comments. Targeted tests passed: 130/130. |
417fc93 to
25ddebb
Compare
|
/bigbonk review for issues |
|
LGTM! |
Next.js runEdgeFunction() appends every header of an edge API route's Response onto the Node response that already holds next.config and middleware headers, so a header both set carries middleware's value and then the handler's. Its NodeNextResponse.appendHeader() skips a value the header already carries, so a value both sides set (or a repeated cookie) is sent once. vinext let middleware win in hybrid apps (dropping the handler's WWW-Authenticate), let the handler win in Pages-only production, and in dev also replaced middleware's cookie. The Pages API route now builds an edge response from the pre-handler headers with the handler's values appended unless already present, and the Pages-only production server hands it the staged middleware and next.config headers, as the Workers response stage already does. Dev ports the same existing-value check onto the Node response.
25ddebb to
1ca1856
Compare
|
💬 Approved with comments — I found no correctness, security or performance problems. Two INFO notes: the description says Set-Cookie is removed from the staged headers but the code keeps it, and the multi-stage test cannot confirm the Cache-Control append. Findings: 2 info The decision is advisory: this comment never approves, blocks or requests changes on the pull request. 🔧 Fix with your agentPaste the following into your coding agent to investigate and fix these issues. 📋 Code Quality: 💬 2 infoThe change is coherent and I found no correctness bugs. Two notes: the description doesn't match the code on Set-Cookie, and one test assertion is weaker than the others. Recommendation: 💬 Optional Nits
View full review
Also checked and correct:
✅ All other checks passed🔒 Security: ✅ LGTMNo security issues found in the header-merging changes. 📖 AGENTS.md: ✅ LGTMThe change follows the repository guidelines. The dev and production paths are both updated, and the fixtures and integration tests are included. ⚡ Performance: ✅ LGTMNo performance concerns. 📝 Documentation: ✅ LGTMNo documentation issues. Reviewed commit: |
1 participant
Summary
Next.js
runEdgeFunction()callsres.appendHeader()for every header of an edge API route'sResponse, andresalready holds the next.config and middleware headers. So when middleware and the handler set the same header, Next sends middleware's value and then the handler's, in dev and prod alike.NodeNextResponse.appendHeader()skips a value the header already carries, so a value that both sides set, or a repeated cookie, is sent only once. vinext's result depended on the path:Middleware sets cookie
mw=1,WWW-Authenticate/Proxy-Authenticate: Basic realm="mw",Cache-Control: private, max-age=7andx-custom: mw. The edge route returnsCache-Control: no-store,x-custom: handler,Set-Cookie: h=1,WWW-Authenticate: Bearer realm="h".private, max-age=7+no-storeprivate, max-age=7no-storeno-storeprivate, max-age=7, no-storemw+handlermwhandlerhandlermw, handlerBasic mw+Bearer hBasic mw(handler's dropped)Bearer hBearer hBasic mw, Bearer hBasic mwBasic mwBasic mwBasic mwBasic mwmw,hmw,hmw,hhmw,hRepeated non-cookie values are joined with
", ", which is how FetchHeadersrepresent repeated fields. A Fetch client reads the same value from Next's separate header lines.Other cases checked against
next start:Varyvalues are sent.x-middleware-foofrom both sides is sent twice.Content-Type, Next sends both values. vinext follows the same composition.x-custom: mw,Cache-Control: private, max-age=7andSet-Cookie: mw=1, each value is sent once.Fix
pages-api-route.ts: the edge branch now copies the pre-handler headers the way a Node handler'sresis seeded (next.config headers first, then middleware's on top, then middleware's cookies) and appends every value from the edgeResponsethat the header doesn't already carry, cookies included. This replaces fix(pages): let API route headers override middleware headers #4056's middleware-wins merge. The hybrid bridge, the hybrid multi-stage response stage and the Pages Workers/multi-stage response stage already pass these headers in, so all of them now compose the same way.prod-server.ts: the Pages-only Node server now passes the pipeline's staged headers, minusSet-CookieandContent-Length, to the API route, as the Workers response stage does. The pipeline still puts middleware's cookies ahead of the handler's. A Node handler now also sees those headers onresbefore it runs, as it does in Next.js and on Workers.api-handler.ts(dev): edge response headers are appended onto the Node response with the same existing-value check asNodeNextResponse.appendHeader(), instead of replacing middleware's withsetHeader().Tests
tests/pages-api-route.test.ts:tests/api-handler.test.ts: the dev edge path appends after middleware's headers and skips values already present. Theapi-handler-globalsmock response gainsgetHeader(), like a Node response.middleware-header-precedence-edgeAPI routes and middleware branches inpages-basicandapp-basic. The existingmulti-stage-httproute now also setsCache-Control.tests/pages-router.test.ts: Pages-only prod and devtests/app-router-production-server.test.ts: hybrid prodtests/multi-stage-http.test.ts: replaces fix(pages): let API route headers override middleware headers #4056's middleware-wins expectation. That fixture's CDN adapter turns any privateCache-Controlintono-store, so the test asserts that value.