[fix] Server-side fetch request should have headers (#696)

[aolose]

Server-side fetch in load function will include original request headers. （#696）

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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

⚠️ No Changeset found

Latest commit: 69e26ee

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[netlify]

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: 69e26ee

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61f966c261508400077ecb83

[benmccann]

It looks like there's another PR to fix this issue as well: #2911. I haven't looked at either in any detail yet. Can you comment on the differences between the two approaches?

[aolose]

It looks like there's another PR to fix this issue as well: #2911. I haven't looked at either in any detail yet. Can you comment on the differences between the two approaches?

the param of function load_node have changed after commit ee906a3b

* @param {{
- *   request: import('types/hooks').ServerRequest;
+ *   event: import('types/hooks').RequestEvent;

The code below is unable to achieve goals. 228f0e660

const headers = /** @type {import('types/helper').RequestHeaders} */ ({
    ...request.headers,
    ...opts.headers
});

Headers can't be merged.

I try to modify it 6f5308e7:

opts.headers = new Headers({
	...Object.fromEntries(event.request.headers),
	...opts.headers
});

It's working, but it can't pass tests.
I didn't look at the test. I guess opts.headers is a Headers object in the test.

[johnnysprinkles]

This also relates to my #3503 where I'm grasping around for ways to get at the incoming request headers in order to fulfill a server-side rendered response (and one that makes data fetch calls).

I suggested either making "event" available in externalFetch, or for a narrower scoped change, making "fetch" have the cookies the way it would if you did a credentials: "include" fetch on the client. Haven't heard if either way is preferred, or neither.

Question though -- should this behavior be gated by the presence of "credentials: include"? At the very least it seems like it should respect "credentials: omit" the same as they do here right?

kit/packages/kit/src/runtime/server/page/load_node.js

Line 179 in 3546170

// allow cookie passthrough for "same-origin"

[Rich-Harris]

Thanks @aolose. @johnnysprinkles yes, we do need to account for credentials — cookie and authorization need special treatment. Will update the PR

[benmccann]

were we only going to skip these for the omit case? right now it looks like it's always skipping them

[Conduitry]

We're skipping copying them over directly right here. These headers should already have gotten handled earlier, if it was appropriate to copy them over.

[bothness]

I seem to be getting issues from version 1.0.0-next.253 upwards whenever I make server-side fetch requests with an "authorization" header. I'm guessing it may relate to this code change?

See my comment here: #3631 (comment)

[Rich-Harris]

Realised there's at least a couple of headers that should get special casing — if-none-match should be removed (it's harmless, but can't possibly match), and referer should be set to the page URL.

Adding some tests alongside this behaviour

[bothness]

This fix appears to be causing problems when I'm using the fetch command in a route's load function within the module context.

I'm getting the following error "Unexpected token < in JSON at position 2", which seems to mean that I'm getting an HTML error page in place of the JSON file that I'm trying to request (it worked perfectly in 1.0.0-next.252 but breaks in 1.0.0-next.252).

I'm guessing this issue can be solved by explicitly adding certain headers to the request, eg. allowing CORS? It would be great if requests could be sent with suitable default headers to prevent this issue!

---

Additional info: It seems this issue may only affect requests that have an "Authorization" header.

[squelix]

I'm getting an error too. I think we need more doc to say when to use fetch from global and fetch from loadInput.

[benmccann]

@bothness comments on merged PRs will end up getting lost. If you have encountered a bug, please file a new issue

[bothness]

Thanks for spotting this @benmccann. I'll file a fresh issue.