fix: stop 404s/redirects from being publicly CDN-cached#2526
Conversation
The catch-all getServerSideProps set 'public, max-age=3600' plus Surrogate-Control and the instance Surrogate-Key at the top of the function, before knowing the outcome. Non-200 exits (notFound, redirects) inherited those headers, so a transient 404 on a valid route (e.g. /instances/ during a deploy window) could be pinned at the Fastly edge and served to everyone -- including logged-in users -- until a manual instance purge. Default every response to 'private, no-store' and apply the cacheable headers only on the confirmed 200 page render. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| ); | ||
| // Default to non-cacheable; cache headers are set only on the 200 path below | ||
| // so 404s/redirects can't be pinned at the CDN edge and served to everyone. | ||
| res.setHeader('Cache-Control', 'private, no-store'); |
There was a problem hiding this comment.
Small behavior change worth calling out explicitly: previously Surrogate-Key was set unconditionally (outside the isProd && guards), so dev/stage responses also carried it. With this refactor, Surrogate-Key is only emitted on the successful isProd 200 path. That's very likely intentional (surrogate keys are only meaningful behind Fastly), but if any stage-facing tooling or purge script currently relies on the header being present in stage, it will silently stop working. Worth a quick confirm.
Also, per your own verification table, Next.js overrides the header on the notFound path to no-store, must-revalidate — so the private, no-store default here is really only observed on the redirect path and any thrown-500 path. Still correct, just noting the default set here isn't what actually goes out for 404s.
There was a problem hiding this comment.
Correct and intentional. As I mentiond in the PR desc: Surrogate-Control/Surrogate-Key are the operative headers here. On Fastly, Surrogate-Control controls the shared edge cache and overrides Cache-Control. Next already forced Cache-Control: no-store on the 404, but it doesn't touch the surrogate headers, so Fastly obeyed Surrogate-Control and edge-cached the 404 anyway. The fix's key effect is dropping the surrogate headers on non-200 paths, not the Cache-Control default.
On stage it's inert: isProd already gated the public Cache-Control, so stage responses were never shared-cacheable regardless of the key
| } | ||
|
|
||
| // Real 200 page: safe to cache and tag with the instance Surrogate-Key. | ||
| if (isProd) { |
There was a problem hiding this comment.
Nice side effect worth noting: the two auth-driven redirects above (/ → /dashboard/ for logged-in users, and /login/ → / for logged-in users) had the same bug as the 404 case described in the PR — under the old code those 307s went out public, max-age=3600 and tagged with the instance Surrogate-Key. In principle an anonymous user hitting / right after an authenticated user could have gotten the cached 307 to /dashboard/, or vice versa. Since they now sit above this isProd block, they inherit the private, no-store default and are non-cacheable. Might be worth a one-liner in the PR description so reviewers/on-call know the fix is broader than the /instances/ 404 symptom.
There was a problem hiding this comment.
Yea, same bug class as the 404. these 307s carried Surrogate-Control + the instance Surrogate-Key, so Fastly could edge-cache an auth-dependent redirect and serve it to the wrong user.
Review summaryCorrect diagnosis, minimal fix, and I like the inversion of the default ( Things I checked
Two small notes (left inline):
Out of scope but adjacent (worth a follow-up ticket, not this PR):
LGTM to ship as-is. |
https://www.zesty.io/instances/returned a 404 served from the Fastly edge to everyone and only a manual "refresh the marketing instance CDN" cleared it.Root cause: a
Surrogate-Controlheader on a non-200 responseThe main header responsible for this is
Surrogate-Control, notCache-Control. On Fastly,Surrogate-Controlcontrols the shared edge cache and overridesCache-ControlIn the catch-all
src/pages/[[...zesty]].js,getServerSidePropsset all three caching headers at the top of the function, before knowing the outcomeWhen
/instances/briefly 404'd through this catch-all, the 404 went out carryingSurrogate-Control: max-age=3600.Next.js technically overrides
Cache-Controltono-store, must-revalidateon that notFound response but that did nothing, because Next only touchesCache-Control(browser). It has no ideaSurrogate-Control/Surrogate-Keyexist, so it left them on the response.Fastly saw
Surrogate-Control: max-age=3600, ignored theno-store, and cached the 404 at the edge.Surrogate-Key: <instance_zuid>tagged it under the marketing instance's key, so it took a full instance purge to clear (and was served to everyone until then).I want to note that (
Cache-Control) was a red herring here for me. The bug was entirelySurrogate-Controlbeing present on a response that turned out to be a 404.