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

Update fetch cache handling with POST requests #46856

Merged
merged 4 commits into from
Mar 7, 2023

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Mar 7, 2023

This updates to no longer skip caching POST or authed requests with the fetch cache and instead we bail when cookies() or headers() is used prior which is a better heuristic to signal user specific data would be related to the fetch request.

x-ref: slack thread

Bug

  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have a helpful link attached, see contributing.md

@ijjk ijjk merged commit 8f5ffed into vercel:canary Mar 7, 2023
@ijjk ijjk deleted the update/fetch-cache-post-handling branch March 7, 2023 05:30
mattrobenolt added a commit to planetscale/database-js that referenced this pull request Apr 5, 2023
For the sake of our customers, I don't feel it's worth us fighting that "this implementation is wrong" with our customers in the middle.

If this hack were specific to an implementation of fetch, I'd feel differently, but given this is in the spec, and we're just being overly explicit, rather than relying on a "default" behavior, I'm more ok with it.

I still firmly disagree with implementations that are choosing to cache POST, _especially_ also with an Authorization header, but there's only so much I/we can control.

See context: vercel/next.js#46856

Fixes #101, #99
mattrobenolt added a commit to planetscale/database-js that referenced this pull request Apr 5, 2023
For the sake of our customers, I don't feel it's worth us fighting that "this implementation is wrong" with our customers in the middle.

If this hack were specific to an implementation of fetch, I'd feel differently, but given this is in the spec, and we're just being overly explicit, rather than relying on a "default" behavior, I'm more ok with it.

I still firmly disagree with implementations that are choosing to cache POST, _especially_ also with an Authorization header, but there's only so much I/we can control.

See context: vercel/next.js#46856

Fixes: #101 #99
mattrobenolt added a commit to planetscale/database-js that referenced this pull request Apr 5, 2023
For the sake of our customers, I don't feel it's worth us fighting that "this implementation is wrong" with our customers in the middle.

If this hack were specific to an implementation of fetch, I'd feel differently, but given this is in the spec, and we're just being overly explicit, rather than relying on a "default" behavior, I'm more ok with it.

I still firmly disagree with implementations that are choosing to cache POST, _especially_ also with an Authorization header, but there's only so much I/we can control.

See context: vercel/next.js#46856

Fixes: #101 #99
@mattrobenolt
Copy link

mattrobenolt commented Apr 5, 2023

👋 hi @ijjk, is there any context that you may share publicly about why this change was done? This is a very interesting change and one that I suspect had a specific motivation behind, but has a major ramifications by deviating from the expected behaviors of HTTP caching by default.

This is already causing issues popping up on our end since our library uses fetch() with a POST and an Authorization header, which both should by any typical measure, be considered unsafe for caching. I was rather shocked to go down this rabbit hole and find that this behavior is newly intentional.

I'm also fine pulling this out into it's own issue, I was just first seeking some background on the motivation for this behavior, and I suspect it's going to cause a lot of confusion or bugs or security issues for your users.

To add to this, I'm going to refer to RFC7231:

Section 3 calls out criteria on which a response must not be cached. Both the POST method is defined as unsafe in section 4.2.1. Section 3.2 explicitly says Authorization header shouldn't be cached unless explicitly allowed.

Personally, I'm fine if this is a product choice by Vercel/Next.js, I just want to make sure this decision to go against the RFCs is intentional for a tangible reason, so we can work around it.

mattrobenolt added a commit to planetscale/database-js that referenced this pull request Apr 6, 2023
For the sake of our customers, I don't feel it's worth us fighting that "this implementation is wrong" with our customers in the middle.

If this hack were specific to an implementation of fetch, I'd feel differently, but given this is in the spec, and we're just being overly explicit, rather than relying on a "default" behavior, I'm more ok with it.

I still firmly disagree with implementations that are choosing to cache POST, _especially_ also with an Authorization header, but there's only so much I/we can control.

See context: vercel/next.js#46856

Fixes: #101 #99
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 6, 2023
@ijjk
Copy link
Member Author

ijjk commented May 9, 2023

@mattrobenolt This was changed as we noticed a lot of CMS requests and graphql requests failing to cache since they leverage POST and have authorization. Instead we went for a heuristic where if headers() or cookies() are read before a POST request or authorized request is done then we don't cache those by default as they can be request specific.

Note: the default can always be overridden on a per-fetch or per-page basis via cache: 'no-store', or export const fetchCache (related fetchCache documentation and related per-request documentation)

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

Successfully merging this pull request may close these issues.

2 participants