fix(cache): don't mutate immutable response headers when applying cache headers - #17588
Open
ondraulehla wants to merge 3 commits into
Open
fix(cache): don't mutate immutable response headers when applying cache headers#17588ondraulehla wants to merge 3 commits into
ondraulehla wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 55f73a1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 419 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
I hit this while following up on #17481. The same header mutation that crashed the Cloudflare adapter on cached image responses also lives in core:
applyCacheHeaders()callsresponse.headers.set()directly on the response a route returns, andCacheHandlerdeletesCDN-Cache-ControlandCache-Tagfrom the response a provider returns. Both throwTypeError: immutablewhen the response headers can't be modified, which is the case for any response returned straight fromfetch(). A route that proxies an upstream (return fetch(...)pluscache.set({ maxAge })) gets a 500 on every request. A user already reported this error class when combiningcache.set()with aVaryheader on Cloudflare (comment on #17408). I haven't verified that path, it needs the Cloudflare dev runtime, but the repro here is adapter free and fails on plain Node.The fix mirrors what #17481 did in the adapter: try the mutation, and when it throws, rebuild the response with
new Response(response.body, response)and apply the headers to the copy. The first.set()throws before changing anything, so applying them again can't duplicate headers. Header stripping now also checkshas()first, so provider responses without CDN headers aren't rebuilt for nothing.One heads up:
pnpm installwith the pinned pnpm also dropped a staletriage/gh-17583importer from the lockfile that #17584 left behind. I can split that out if you'd rather keep it.Testing
Two new fixtures. Without the fix all four new tests fail with a 500 instead of a 200: a
memoryCache()route returning afetch()proxied response, and a provider whoseonRequestserves responses with immutable headers, where the CDN header stripping used to crash even when those headers weren't present. With the fix they pass and the second request is served as a cache HIT. The cache unit suite (162 tests) and the neighbouring integration tests still pass. I also reproduced the crash end to end against published astro 7.1.6 with@astrojs/nodebefore writing the fix.Docs
No docs changes. The changeset describes the fix.