Exposing event to load(), and removing the automatic header forwarding

[johnnysprinkles]

Fixes #3503 Fixes #696
Reverts #3631 Reverts #835

Okay this is the last one of my SvelteKit wishlist items. Not sure what you'll think but it certainly makes sense to me.

This does two things, each in its own commit.

1. Exposes event to the load() function
2. Reverts and deletes all the logic related to passing headers and cookies to the upstream service calls.

#3631 was clearly a mistake. If we have a network call graph that looks something like this:

It doesn't make any sense for the Node service to pretend to be a browser. Some of the reasons:

• You're lying about the user agent. If you passed one at all it should be, perhaps, node/sveltekit or something
• You're passing headers unidirectionally, sending them out but but not sending the response headers back to the browser, because of course that wouldn't make any sense. The calls are many to one so what should you do, merge them?
• The type is different (HTML vs most likely JSON), so the accept header could be wrong
• Some of the headers are nonsensical for service-to-service calls, like referer which is a browser concern

There just seems to be some confusion and it seems like people are considering calls A-C in the graph to be the Z call proxied onward. When in fact those calls are unrelated to call Z, except that call Z is the reason calls A-C are being made.

But then even with all the complexity, we still can't pass along the most important information: the auth cookies. To get the full details of the incoming request you have to use an endpoint (or a hook). Endpoints should be a feature you can use if you want but shouldn't be mandatory.

In a more ops-driven, "close to the metal" kind of enterprise setup, our frontend services live among a constellation of other services, both in front as reverse proxies, and behind as upstream services. In this world things like SSL termination and endpoints are simply not wanted or needed. In this world if we want to avoid the moving target of CORS and serve our data on the same domain as our HTML, we simply put a reverse proxy in front and split traffic.

What this change does is restores control. If you want to call your backend services and spoof like you're a browser, you can do so! Copy all your headers over. If you want to forward a backend service's "set-cookie" directive to the browser, go ahead. But if you do nothing the calls to the backends get no headers, just as if you made a curl call, which seems like a pretty reasonable thing to do by default.

Here's some examples:

export default async function load({ event, params, fetch }) {
  let response = await fetch('https://a.backend.com/data/' + event.id, {
    headers: {
      // Use end user credentials
      // You could parse the cookie string and slice out a specific auth cookie, or
      // or just pass the whole darn thing.
      cookie: event?.request.headers.get('cookie'), // only relevant server-side
      credentials: 'include', // only relevant client-side
    },
  });
  ...
}

If your backend sets a cookie and you want to set the same cookie in the browser, there's no way to do that explicitly, and I stripped out the automatic logic for that because, hey, making service calls shouldn't have side effects. You get the data, you do what you want with the data, end of story. So I extended some interfaces with a set_cookies string array.

export default async function load({ event, params, fetch }) {
  let response = await fetch('https://a.backend.com/data/' + event.id, {
    headers: {
      cookie: event?.request.headers.get('cookie'), // only relevant server-side
      credentials: 'include', // only relevant client-side
    },
  });
  let data = await response.json();
  let set_cookie = response.headers.get('set-cookie');
  return {
    props: {
      widgets: data.widgets
    },
    set_cookies: set_cookie && [set_cookie],
}

More rationale: Even in the demo TODO app, you need access to the user's cookies to make the backend call. I know that backend is slated for removal in #4264 but as long as it remains it does prove a point. In my repro case for #4902 I have code like this:

https://github.com/johnnysprinkles/sveltekit_after_navigate_bug/commit/53dd01ad7f4f07cd1e5cfc31fb2782048ff258d1#diff-87b424ffe45157388b79be6ab8b5f706a521128cf102a15a3b58f480f4411a85R27

where I have to put the userid in session because there's no way to read it from the request in load(). If you don't want to put it in session and you don't want to use an endpoint, you're currently out of luck.

Anyway, I hope this might at least provoke some discussion. I know this goes against a SvelteKit value of abstracting away the differences between client and server, but that abstraction is kind of a fiction anyway.

Also, I have no idea how much this would break. Shadow endpoints? Various adapters?

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: ff4e342

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	Minor

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

[Conduitry]

As I mentioned in #3503 (comment), exposing event to load is bad because you completely lose the security of HTTP-only cookies.

[johnnysprinkles]

I guess I never quite understood that point... this is a change inside the running NodeJS code, not sure how that could be a security issue. I looked back at your comment and see that the server-side fetch currently tries to simulate what the browser's fetch would do, but why impose those kind of elective limitations on your own code in the serverside?

[johnnysprinkles]

Speaking of HTTP only cookie though, I was too aggressive in pruning and should have left in the code that strips out set-cookie and etag from the fetched headers. Because that information might not want to be exposed to client logic. We could look at the cookie value itself and look for HttpOnly but for now I just reverted that file back to how it was, so no set-cookie value ever appears on the client via that "serialized fetch result" data passing mechanism. (See update.)

[johnnysprinkles]

More thoughts about this... if right now we're implementing the browser's XHR security model in very broad strokes, it's interesting to think about what would be involved in implementing it exactly correctly:

• Respect the various "credentials" options, not just "omit"
• Understand and respect all the various Access-Control-* headers, and there's a lot of them
• Know if the request should be preflighted or not
• Change all the rules depending on if it's HTTP or HTTPS
• Keep this all up to date as things change (isn't Chrome always threatening to remove third party cookies altogether?)
• Maybe even use browser sniffing to make the behavior match the user's UA
• Ideally we'd even match older browser versions, keeping a historical record of how CORS works in all browsers across all versions

This doesn't seem like a business we want to be in.

I did a quick search and we have 79 open SvelteKit issues containing "fetch". Just on the first page I see a couple that this PR might resolve. Would be pretty nice to be able to close all those out.

[johnnysprinkles]

Nice!