only include specified headers in server-side fetch requests and serialized responses

[Rich-Harris]

Closes #5253
Closes #5195
Closes #1971

This is a topic that has been discussed a lot on this repo without an adequate resolution: when making server-side fetch calls within a load function, how which (if any) of the original request headers do we include?

In the interests of making things 'just work' as far as possible, we've been including almost all headers ever since #3631. But this causes its own problems, such as #5253. A PR from @johnnysprinkles (#5195) removes all header forwarding, but has sat unmerged because it depends on adding the event object to the load inputs, which we definitely don't want.

This PR takes a different approach (basically the one suggested by @nhunzaker in #5253 (comment)) — it allows app developers to create an allowlist of headers that should be forwarded in server-side fetches.

At the same time, we add a list of headers that should be included when we serialize the response, which means that things like Date don't mess up content hashing (which prevents us from responding with 304s — #1971):

// svelte.config.js
export default {
  kit: {
    fetch: {
      forwardedRequestHeaders: ['x-my-custom-auth-header'],
      serializedResponseHeaders: ['x-my-custom-response-header']
    }
  }
};

Questions:

• Are these the right names for the options?
• What should the defaults be? Right now I included content-type in serializedResponseHeaders to get the tests to pass, but presumably there are other sensible defaults. To some degree it doesn't matter, but it would be nice if most people didn't need to touch this option
• Should we go even further and remove the special handling for cookie and authorization headers when credentials is something other than omit?

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: f97dcff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

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

[netlify]

✅ Deploy Preview for kit-demo ready!

Name	Link
🔨 Latest commit	f97dcff
🔍 Latest deploy log	https://app.netlify.com/sites/kit-demo/deploys/6313398bf07e4a000950ed6b
😎 Deploy Preview	https://deploy-preview-6541--kit-demo.netlify.app/build
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[benmccann]

I wonder if it'd be worth having a test ensuring these aren't set by default just so that if we ever decide we need to go back to a blacklist we don't accidentally add these back or to make sure we don't accidentally add these to a default whitelist in the future

[dummdidumm]

I'm wondering if a static list is enough. Could there be use cases for "I want this forwarded for urls from X but not those from Y", which would hint at making this a hook somehow (which I don't know if it's possible)

[Rich-Harris]

Yeah, I think a hook is the way to go. @Conduitry had a suggestion I really like, which solves the 'I want this forwarded for urls from X but not those from Y' problem while also providing a solution to #4750 — we get rid of externalFetch and add a new handleFetch hook that gets passed the default fetch implementation, like so:

export function handleFetch({ event, request, fetch }) {
  // `event` is the underlying `RequestEvent`
  // `request` is a `Request` object the normalizes the arguments to `fetch` in `load`
  // `fetch` is our normal fetch logic — get data from our own routes, or delegate to undici/whatever
}

This would allow us to get rid of forwardedRequestHeaders, though we'd still need serializedResponseHeaders

[Rich-Harris]

closing in favour of #6565 and #6569