Closed malviyaritesh closed 1 year ago
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.
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.
Put your actual fetch inside +server and call that from your load function
@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.
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.
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?
@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.
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
This was added in https://github.com/sveltejs/kit/pull/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.
This was an oversight, thanks for bringing it to our attention.
I think there are a few possible approaches:
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.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 approachThoughts?
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())
};
}
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.
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
}
}
};
@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:
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.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.
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 thefetch
.I tried calling
depends
insideload
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
.env
file and add a secret such asAPI_KEY=top-secret-key
.+layout.server.ts
which usesfetch
to load data from external api. Import and passAPI_KEY
environment variable to fetch url as query param.+layout.svelte
.script
tag, you'll find that your secretAPI_KEY
is exposed to the client.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 thedependencies
array is empty which is what I was expecting.Logs
No response
System Info
Severity
blocking all usage of SvelteKit
Additional Information
No response