When an error page is "static", cache-control headers are overwritten

[nicholas-c]

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:

1. Setting a cookie with a value
2. Setting a Cache-Control: private, no-cache, no-store; header
3. Error is then thrown in getServerSideProps method
4. 500 page has a getStaticProps method, therefore is a static HTML page from S3
5. Cookie value is cached for all to see

In this scenario, any Cache-Control headers set by the page, will always get overwritten when the 500/error page does a getStaticProps method of it's own - In testing this issue in isolation, having no getStaticProps 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 a Set-Cookie header on the response, if the getServerSideProps method then errors and serves a "static" error page, open-next overwrites any Cache-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

// error-page.tsx
import { GetServerSideProps } from "next";

export const getServerSideProps = (async ({ res }) => {
 res.setHeader("Cache-Control", "private, no-cache, no-store");
 res.setHeader("Set-Cookie", "some-random-cookie=some-value;");
 throw new Error("Whoopsie");
}) satisfies GetServerSideProps;

export default function Page() {
 return (
   <>
     <h1>This page should error</h1>
   </>
 );
}

// 500.tsx
export default function Custom500() {
  return (
    <main>
      <h1>500 error page</h1>
    </main>
  );
}

export async function getStaticProps() {
  return {
    props: {
      foo: "bar",
    },
  };
}

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...

[khuezy]

Thanks for the report. Should fixCacheHeadersForHtmlPages always delete the set-cookie header to prevent any cookies from getting set?

[khuezy]

I'm not sure why the condition is if (HtmlPages.includes(rawPath) && headers[CommonHeaders.CACHE_CONTROL]) { It would make more sense for it to be !headers

[conico974]

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 s-maxage=31536000, stale-while-revalidate, when we encounter this header we modify it to what you see.

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

[khuezy]

Another workaround is to try/catch your gSSP and redirect it to an error page

[nicholas-c]

Just getting back onto this now, sorry for the delayed response.

The only thing for this is to make your 500 page SSR instead, this will fix your issue

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

That's next that is overwriting your cache-control headers the first time

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

 // WORKAROUND: `NextServer` does not set cache headers for HTML pages — https://github.com/serverless-stack/open-next#workaround-nextserver-does-not-set-cache-headers-for-html-pages

[conico974]

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

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

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

This is not the offending block, as you can look this is not the same header at all s-maxage=31536000, stale-while-revalidate=2592000 in your request vs public, max-age=0, s-maxage=31536000, must-revalidate in the block you showed.

This is where we override this header https://github.com/sst/open-next/blob/ba6e176fe30c3654de24c34a0269cbf1ac4d8ada/packages/open-next/src/adapters/plugins/routing/util.ts#L92 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 500.html file is present in the bundle, and next server might think that it is a static html file (which it is not, since you use getStaticProps it is SSG). I'm not sure how to handle this one, the way it is done in next seems incorrect since you could use an external cache handler (which is what we do) and serving the local 500.html is not really correct (You might have revalidated the 500 page using res.revalidate, or as we do removed the 500.html file from the bundle)

[conico974]

It should be fixed by this now https://github.com/opennextjs/opennextjs-aws/pull/426