-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
feat: explicit cache: no-store
in fetch
#847
Conversation
cache: no-cache
in fetchcache: no-store
in fetch
🤖 I have created a release *beep* *boop* --- ## [2.63.0](v2.62.2...v2.63.0) (2024-03-26) ### Features * add method for anonymous sign-in ([#858](#858)) ([e8a1fc9](e8a1fc9)) * add support for error codes ([#855](#855)) ([99821f4](99821f4)) * explicit `cache: no-store` in fetch ([#847](#847)) ([034bee0](034bee0)) * warn use of `getSession()` when `isServer` on storage ([#846](#846)) ([9ea94fe](9ea94fe)) ### Bug Fixes * refactor all pkce code into a single method ([#860](#860)) ([860bffc](860bffc)) * remove data type ([#848](#848)) ([15c7c82](15c7c82)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This has introduced a couple of errors regarding Cloudflare Workers, related: |
result = await fetcher(url, { | ||
...requestParams, | ||
// UNDER NO CIRCUMSTANCE SHOULD THIS OPTION BE REMOVED, YOU MAY BE OPENING UP A SECURITY HOLE IN NEXT.JS APPS | ||
// https://nextjs.org/docs/app/building-your-application/caching#opting-out-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"YOU MAY BE OPENING UP A SECURITY HOLE IN NEXT.JS APPS"
there's nothing at all about that in the link provided. 🤔
maybe the comment can be expanded to actually explain what issue this would cause?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does in the context of NextJS, as the docs state:
The Data Cache is persistent across incoming requests and deployments unless you revalidate or opt-out.
So we have to forcefully opt-out of the cache, as it appears that GET /user
may be cached just because. There's no docs on whether it's cached by the JWT, or not. We reached out to Vercel -- 0 information on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add that to the comment? most users are likely going to do the same thing i did and look on that page you linked for something mentioning "security" directly.
Next.js polyfills the
fetch
on the server side to always cache (what's known as their Data Cache) responses. Quote:This is incredibly dangerous for Auth use cases. Thankfully, it appears that using the default client uses
@supabase/node-fetch
in such a circumstance which should not be affected by the issue.However, if developers do choose to switch back to the native
fetch
in Next.js the library must inform that HTTP client that there should be no caching under any circumstance on Auth responses.Opting out docs: https://nextjs.org/docs/app/building-your-application/caching#opting-out-1