Secrets exposed in server only load fetch dependencies

[malviyaritesh]

Describe the bug

Thanks for the awesome open source work.

I'm quite new to Svelte and SvelteKit and recently decided to use in a project to learn it more. I think, I've discovered some bug or issue in SvelteKit with my very limited experience using it.

I'm loading some data in +layout.server.ts from an external url which requires sending api key as query param. I've stored the api key in .env file. I'm loading the data in +layout.server.ts specifically because I don't want to expose api key to the client. But even this file is also exposing it to client as part of url dependency for the fetch.

I tried calling depends inside load but it adds additional dependencies along with fetch url. I couldn't find any way to prevent SvelteKit using url as dependency.

As an alternate solution to this, I could've created an internal api route which calls the external api with api key but I think +layout.server.ts shouldn't expose the url (and secrets certainly not) to client as it guarantees to run the code only server side. I don't see any benefit of using server only load of +layout.server.ts over universal load in +layout.ts, if it exposes the url (and secrets) to client.

Reproduction

1. Create an .env file and add a secret such as API_KEY=top-secret-key.
2. Create a load function in +layout.server.ts which uses fetch to load data from external api. Import and pass API_KEY environment variable to fetch url as query param.
3. Use the server fetched data in +layout.svelte.
4. Open the browser dev tools and view source. At the bottom in script tag, you'll find that your secret API_KEY is exposed to the client.

const data = [
	{
		type: 'data',
		data: {
			todos: [
				{ id: 1, title: 'one' },
				{ id: 2, name: 'two' }
			]
		},
		uses: {
			dependencies: [
				'https://jsonplaceholder.typicode.com/todos?api_key=top-secret-key'        // <---- Secret exposed
			]
		}
	},
	null
]

Here's a demo project I've created to reproduce this issue:
https://stackblitz.com/edit/sveltejs-kit-template-default-eeb3rv?file=src/routes/+layout.server.js

Also note that in my testing with @sveltejs/kit@next version the dependencies array is empty which is what I was expecting.

Logs

No response

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
Binaries:
    Node: 16.14.2 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 7.17.0 - /usr/local/bin/npm
npmPackages:
    @sveltejs/adapter-auto: ^2.0.0 => 2.0.1 
    @sveltejs/kit: ^1.5.0 => 1.15.9
    svelte: ^3.54.0 => 3.58.0
    vite: ^4.3.0 => 4.3.3

Severity

blocking all usage of SvelteKit

Additional Information

No response

[gtm-nayan]

Dependencies have to be sent to the client to be able to invalidate data properly.

You can use the handleFetch hook to rewrite any requests that are sent from your server. So your +layout.server.js load function can just call the URL without the key, and the dependencies will have that link instead, then inside handleFetch you can add your API key to the url.

https://stackblitz.com/edit/sveltejs-kit-template-default-iad1ko?file=src%2Froutes%2F%2Blayout.server.js

I suppose we could document this behaviour better to prevent users leaking their keys. Feel free to send a PR or point out where in the documentation you would expect it.

[david-plugge]

Put your actual fetch inside +server and call that from your load function

[malviyaritesh]

@gtm-nayan
Thanks for your reply.
I understand that dependencies are required to invalidate the data.
I'm aware of handleFetch hook. As I've mentioned that I was not looking for a workaround (or an alternative way to fetch from external api) to prevent leaking the secret to client.

What I'm expecting is that there should be some way to override the invalidation key (which I guess is by default the fetch url). There's already a depends function provided by ServerLoadEvent in load but it doesn't facilitates the override behaviour. Other solution which I can think of is that SvelteKit should allow hashing the url (or by default hash all if not a performance issue) before using as invalidation key, so that urls are not exposed.

[ramonmalcolm10]

I was not aware of this behavior. I don’t think any value or code that is added to +layout.server.js or +server.js file should be exposed to the client under no circumstances. This is going to cause a lot of secret leak even if it’s documented.

[machak]

I still need to digest this, but what's the point of "private", "server" and "public" if everything is exposed to the client by default?

[malviyaritesh]

@ramonmalcolm10 @machak
Exactly. I came to SvelteKit from NextJS 13 because I wanted to use some React 18 exclusive features like Streaming, Suspense, Server components etc. but they are only available as part of app router in NextJS 13 which is in beta for a long time.
I noticed that SvelteKit provides similar features with elegance of Svelte's reactivity and other good things but these kind of issues (which are completely unacceptable from some production framework) putting me off.

Using handleFetch to rewrite your secrets in response is really a hack not a solution. Imagine using more than 5 private external apis in your project and you've to apply this hack for each of them. We can't expect from users to apply this hack to prevent their secrets from being exposed which are by default exposed anyway.

[machak]

It would be nice if this could be automatically done/intercepted by compiler itself and replace any secret with a unique placeholder and automatically map and rewrite URLs in a similar way as in handleFetch, instead of just documenting it and when major security leak happens pointing to people and telling them to RTFM

[dominikg]

This was added in #8420

Invalidation by url for urls passed into server only fetch calls seems like a rare usecase that should be opt-in rather than default, especially if it leaks information about service urls to the client.

This is not only problematic if the url contains a secret token, but in general, as the url itself gives an attacker additional information that could be used in a chained attack.

[Rich-Harris]

This was an oversight, thanks for bringing it to our attention.

I think there are a few possible approaches:

1. Don't include implicit dependencies at all when serializing data. This would be a breaking change, but a reasonable one under the circumstances
2. Don't include implicit dependencies for external URLs. That is, fetch('/api/foo') would continue to add /api/foo to dependencies, allowing for easy invalidation, but fetch('https://example.com/api/foo?key=<SECRET>') would not. This assumes that you're not passing secrets to your own endpoints, which isn't bulletproof but seems like a fair assumption. This would still be a breaking change, but less so.
3. Strip out query parameters. Again, not bulletproof, but would prevent leakage in all realistic cases, though it would (to @dominikg's point above) still leak information about the URLs being used
4. Hash URLs. In other words, fetch('https://example.com/api/foo?key=<SECRET>') creates a abc123 dependency, while fetch('/api/foo') creates a def456 dependency. In the client, calling invalidate('/api/foo') would cause def456 to be invalidated, because we'd use the same hashing function. However, invalidating with a function, as in invalidate((url) => url.startsWith('/api')), would not work with this approach
5. Some combination of the above

Thoughts?

In #9850 @malviyaritesh adds a new svelte key to the fetch options. I don't think we should do this — adding arbitrary stuff to standard interfaces rarely ages well in my experience, and I don't think there's any reason to prefer this...

import { PRIVATE_STATIC } from '$env/static/private';

export async function load({ fetch }) {
  return {
    time: fetch(`http://worldtimeapi.org/api/ip?PRIVATE=${encodeURIComponent(PRIVATE_STATIC)}`, {
      svelte: { depends: 'app:hidden-time-api' }
    }).then((res) => res.json())
  };
}

...over this:

import { PRIVATE_STATIC } from '$env/static/private';

export async function load({ fetch, depends }) {
  depends('app:hidden-time-api');
  return {
    time: fetch(`http://worldtimeapi.org/api/ip?PRIVATE=${encodeURIComponent(PRIVATE_STATIC)}`).then((res) => res.json())
  };
}

[dummdidumm]

I would make this an option like dangerouslyTrackServerFetches which you can opt in to but it's removed in 2.0 and then we also print some kind of warning so people know they can opt into it - so the breaking change could easily be reverted for people who need the current behavior.

We could also make it opt in to track so we don't break people but warn that people should opt into it.

[Rich-Harris]

Opting people in by default would defeat the object, I think. Any thoughts on what sort of warning would guide people towards the option without annoying people who don't need it?

I remember we (half-jokingly?) suggested a dangerZone config section in case we need more things like this in future:

export default {
  kit: {
    dangerZone: {
      trackServerFetches: true
    }
  }
};

[malviyaritesh]

@Rich-Harris
My thoughts align well with @dominikg and IMHO we should adopt approach # 1.

adding arbitrary stuff to standard interfaces rarely ages well in my experience

Yes, I also don't like to poison the standard interfaces, but there were two reasons for this:

1. Initially, this issue was labelled for documentation related fixes only. But clearly, it needed changes in the code. So, in order to avoid creating breaking changes while providing at least overriding behaviour this was the only API, I could think of.
   But now as this issue is being properly acknowledged for its severity and maintainers agree to create breaking changes, this sounds good and we can think about better API design.
2. Providing dependency in fetch interface can allow invalidating individual requests without throwing away all the other valid responses and reloading everything in load. But I think currently SvelteKit doesn't support this behaviour, so this isn't valid.

Also, completely unrelated, I don't expect this to be considered (as it is another breaking change) but just sharing the thought.
The invalidate functions should be renamed to revalidate, as behaviour of invalidate functions is not only to invalidate or mark data as stale, but also calling these functions will re-fetch fresh data automatically.