fix: strip Set-Cookie from App Route ISR cache entries#818
fix: strip Set-Cookie from App Route ISR cache entries#818southpolesteve wants to merge 1 commit intomainfrom
Conversation
App Route ISR cache values were persisting Set-Cookie headers from cacheable GET route handlers and replaying them on later HIT/STALE responses. Cached route handler responses are shared across requests, so Set-Cookie must never be stored in the route ISR cache. Fix: - drop Set-Cookie entirely in buildAppRouteCacheValue() - keep X-Vinext-Cache and Cache-Control excluded as before - add a dedicated static route handler fixture with revalidate=1 that sets Set-Cookie directly on the response - add a regression test proving the MISS response includes Set-Cookie but the cached HIT response does not
|
/bigbonk review this PR |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, correct fix. The bug was that buildAppRouteCacheValue() filtered set-cookie in the forEach loop but then immediately re-added it via the getSetCookie() block below. Removing that block is the right fix.
A few observations from tracing the call sites:
The cookies().set() API path was never affected. In app-route-handler-execution.ts:145-172, the clone is taken from the raw handler response before finalizeRouteHandlerResponse() adds pending cookies from cookies().set(). So only direct Set-Cookie headers on the Response constructor (like the new fixture) would have been cached. The cookies() API cookies were safe by accident of ordering. Good to know that this PR closes the remaining gap.
STALE regen path is also covered. The background regeneration in app-route-handler-cache.ts:116 calls buildAppRouteCacheValue(response) on a freshly-executed handler response, so the same filter applies there.
Test placement is reasonable given the order-dependent ISR test block. The new test creates its own cache key (/api/static-set-cookie) so it doesn't interfere with the /api/static-data tests around it.
LGTM.
|
Reviewed and approved PR #818. The fix is correct: the old code had a contradictory pattern where the Key findings from the review:
|
|
I think this might be a deviation from Next.js, albeit arguably a correct one. |
|
Yeah good catch @james-elicx. I'm honestly torn about how to proceed here. |
2 participants
Summary
App Route ISR cache values no longer persist
Set-Cookieheaders.Details
Cached App Route responses are shared across requests. Persisting
Set-Cookieinside the ISR cache would replay per-request cookies to later visitors on HIT/STALE responses.This change updates
buildAppRouteCacheValue()to keep dropping:set-cookiex-vinext-cachecache-controlTests
Adds a dedicated cacheable route handler fixture (
/api/static-set-cookie) withrevalidate = 1that setsSet-Cookiedirectly on the response.Regression coverage verifies:
MISSandSet-CookiepresentHITandSet-Cookieabsent