-
Notifications
You must be signed in to change notification settings - Fork 119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
When an error page is "static", cache-control headers are overwritten #326
Comments
Thanks for the report. Should |
I'm not sure why the condition is |
This is a tricky one because your 500 page is not static html, it is actually SSG (which from next POV is like ISR). That's next that is overwriting your cache-control headers the first time, when you set it as SSG next overwrite your cache-control to We can't really do something automatically here, if we don't override it, next would have, if we override it to not cache, it's not correct either because it might be the desired behavior. The only thing for this is to make your 500 page SSR instead, this will fix your issue |
Another workaround is to |
Just getting back onto this now, sorry for the delayed response.
The issue with making the 500 page SSR, is that Next.js then fails to build and throws this error - https://nextjs.org/docs/messages/404-get-initial-props A redirect also doesn't feel the "right" way to do it as we'd likely lose some of our datadog observability on them routes specifically throwing 500's
In the Open-next code the offending block has the following comment, which suggests Next isn't already setting any of the headers? We've not been able to replicate this issue running Next Server without open next via K8s or other alternatives
|
You probably need to use https://nextjs.org/docs/pages/building-your-application/routing/custom-error#more-advanced-error-page-customizing or even https://nextjs.org/docs/pages/building-your-application/routing/custom-error#reusing-the-built-in-error-page
This is not the offending block, as you can look this is not the same header at all This is where we override this header
And as you can see it is a replace, so we take the cache header from the next response and modify it. My guess as to why it works on k8s is that the |
Hey folks 👋
There's a potential "gotcha" we're experiencing, and just wanted some insight; We have a page where we're doing a few things:
Cache-Control: private, no-cache, no-store;
headergetServerSideProps
methodgetStaticProps
method, therefore is a static HTML page from S3In this scenario, any
Cache-Control
headers set by the page, will always get overwritten when the 500/error page does agetStaticProps
method of it's own - In testing this issue in isolation, having nogetStaticProps
method on the error page leads to the headers not being overwritten.Specifically for us, we discovered this when using Auth0's
withPageAuthRequired
method, and a "rolling session". This package automatically updates the users cookie/JWT and writes that as aSet-Cookie
header on the response, if thegetServerSideProps
method then errors and serves a "static" error page, open-next overwrites anyCache-Control
headers (Which Auth0 also write to the response) and will expose JWT/cookies for the entire world to see.I've setup an example with a clean Next.js app and new SST deployment (Can upload to Github if needed)
https://d27orfbdgu7alk.cloudfront.net/error-page
As you can see in the page response headers, our
private, no-cache, no-store
properties have been replaced, and therefore is caching the cookies previously set in the response.https://github.com/sst/open-next/blob/ba6e176fe30c3654de24c34a0269cbf1ac4d8ada/packages/open-next/src/adapters/plugins/routing/util.ts#L77
This snippet may be where that overwrite is caused, but one of two things needs to happen to prevent this, either the entire response is thrown out and a new one generated, or cache-control headers not overwritten when either set-cookie, or cache-control headers are already defined...
The text was updated successfully, but these errors were encountered: