search

opennextjs / opennextjs-aws

Open-source Next.js adapter for AWS
https://opennext.js.org
MIT License
4.14k stars 126 forks source link

fix 500 error page being cached #590

Closed conico974 closed 3 weeks ago

conico974 commented 3 weeks ago

It seems that sometimes the 500 error page gets cached for some reason. We don't want that to happen.

Update:

This can happen for page router if your 500 (or 404) page use getStaticProps. For every route in page router that throws an exception, the cache-control gets overwritten and set as if it was an SSG route. The problem is that it doesn't remove the previous header that was set (including set-cookie). This could lead to session leakage.

Haven't tested in Next 15, this is probably fixed there since they don't override your cache-control anymore ( It might still happen if you don't set any cache-control for the route )

I added an env variable OPEN_NEXT_DANGEROUSLY_SET_ERROR_HEADERS that if set to true will disable the cache-control override that we do

changeset-bot[bot] commented 3 weeks ago

⚠️ No Changeset found

Latest commit: 09e5bd9df58d517cdb9ffd63a7052c29cda46119

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

pkg-pr-new[bot] commented 3 weeks ago

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/aws@590

commit: 09e5bd9

conico974 commented 3 weeks ago

Would this be related at all w/ the one user on discord having a random session leak?

It's probably it's error yeah. I need to investigate this more closely but based on what he describes it is likely the issue. If the set-cookie headers has been set and an error occurs after that then the 500 could be cached. Still not sure why next would set cache-control on a 500 though, seems very weird

khuezy commented 3 weeks ago

Yea I'm not sure what the use case would be for errors to be cached. The HTTP spec says not to cache 500's.

conico974 commented 3 weeks ago

Yea I'm not sure what the use case would be for errors to be cached. The HTTP spec says not to cache 500's.

There is a use case that i can think of. Just to reduce the load on the server when you know that something is broken, you could cache the error for a small amount of time. Still i don't think it's a good idea, and it might just be that since they load the 500 page from the incremental cache, they assume it is ISR/SSG and apply the cache-control headers.

sommeeeer commented 3 weeks ago

this is interesting, probably what that guy was experiencing. i like the idea of having an environment variable to opt out of it. would be nice to know why this.setHeader() is not working tho.