Skip to content
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

Merged
merged 1 commit into from
Feb 5, 2024
Merged

feat: explicit cache: no-store in fetch #847

merged 1 commit into from
Feb 5, 2024

Conversation

hf
Copy link
Contributor

@hf hf commented Feb 3, 2024

Next.js polyfills the fetch on the server side to always cache (what's known as their Data Cache) responses. Quote:

Next.js has a built-in Data Cache that persists the result of data fetches across incoming server requests and deployments. This is possible because Next.js extends the native fetch API to allow each request on the server to set its own persistent caching semantics.

Good to know: In the browser, the cache option of fetch indicates how a request will interact with the browser's HTTP cache, in Next.js, the cache option indicates how a server-side request will interact with the server's Data Cache.

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

@kangmingtay
Copy link
Member

kangmingtay commented Feb 5, 2024

agreed that the default behaviour is dangerous for auth in nextjs but they also provide a way to specify this through the noStore and cookies functions, which will force a cache revalidation each time - i think we should add this to our nextjs guides too

src/lib/fetch.ts Outdated Show resolved Hide resolved
@hf hf changed the title feat: explicit cache: no-cache in fetch feat: explicit cache: no-store in fetch Feb 5, 2024
@hf hf merged commit 034bee0 into master Feb 5, 2024
2 checks passed
@hf hf deleted the hf/cache-no-cache branch February 5, 2024 13:26
kangmingtay pushed a commit that referenced this pull request Mar 28, 2024
🤖 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>
@elias123tre
Copy link

This has introduced a couple of errors regarding Cloudflare Workers, related:
cloudflare/workerd#698
supabase/supabase-js#1001
cloudflare/workers-sdk#2514
nuxt-modules/supabase#340

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

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?

Copy link
Contributor Author

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants