sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.43k stars 1.89k forks source link

SvelteKit security model doesn't cover +page.svelte content #12541

Open ckiee opened 1 month ago

ckiee commented 1 month ago

Describe the problem

SvelteKit allows you to visually deny access to a route using server hooks, but the compiled +page.svelte's filename is always public as .svelte-kit/output/client/_app/immutable/entry has import references to all routes' bundles.

This can leak internal API calls and DOM content on the supposedly-private route which is harmful for my blog use-case where I'd like to have truly private posts[^1]. It is also misleading for the developer who may not telepathically intuit the SvelteKit hydration model.

[^1]: Which I want to be able to just write in Svelte so I can have little experiments inline and so on using all the tailwind tooling goodness I have on the rest of the site.

Describe the proposed solution

[^2]: What about _app/immutable/nodes/? Do we wanna go that far and also try to prune dependencies that are only used on private pages?

Alternatives considered

Importance

nice to have (workaround probably possible, otherwise it'd be critical)

Related issues

https://github.com/sveltejs/kit/issues/10072

Conduitry commented 1 month ago

There have been other issues opened in the past that I can't find right now about the route manifest being public. This is by design. We don't want the browser reaching out to the server on every single request in case there's some secret route that that might match. (This would also be difficult when using the static adapter, because there is no server.) We also don't want to be encouraging that sort of security-through-obscurity approach.

I'll leave this open with the 'documentation' label because this is something we could maybe be more explicit about in the docs.

ckiee commented 1 month ago

We don't want the browser reaching out to the server on every single request in case there's some secret route that that might match. […] We also don't want to be encouraging that sort of security-through-obscurity approach.

That makes sense, and I, too, don't believe in security-through-obscurity -- but I also believe in security-in-depth. And besides, my blog usecase[^1] is completely different and I think it is legitimate.

I don't want to hide the route manifest, I want to extend the security boundary from +page.server.ts to also include +page.svelte, so files in output/client/_app/immutable/nodes can be denied access using hooks with the same auxillary semantics as /__data.json. Preload can still opportunistically work.

[^1]: where there are public/private posts and they are just routes with component includes that'd be static except for the auth hook

ckiee commented 1 month ago

I think export const private = true; may be extraneous with fast, idempotent hooks.

ckiee commented 1 month ago

So I ended up spending all of today learning enough SvelteKit internals and I wrote https://github.com/ckiee/svelte-adapter-bun/commit/94f6b0b236e3d114f875cb0f860aa1abf68a1452 as a functional PoC which avoids the most painful parts. There's no live demo because I've been testing it against my closed codebase but that can come a bit later.

This obviously cheats a ton but it does work: the server hooks restrict access to the JS bundle based on the user's session cookie which is passed through to the fake /__data.json request (which we'd want to remove and replace with a clean non-Request way to call into the Server's hooks-and-layout-and-load.ts chain logic.

ckie@cookiemonster ~ -> curl 'http://localhost:4142/_app/immutable/nodes/6.Cc_FuweP.js' ; echo
403
ckie@cookiemonster ~ -> curl 'http://localhost:4142/_app/immutable/nodes/6.Cc_FuweP.js' --cookie "authjs.session-token=eb742c19-[uuid redacted]" ; echo
import{s as m,n as c,c as d}from"../chunks/5.uHuZ-gue.js";import{S as f,i as _,e as u,t as i,c as w,a as h,d as p,f as l,j as g,k as O,l as x}from"../chunks/index.DEzwPv4q.js";import{p as v}from"../chunks/stores.N-ec_Nrk.js";function $(r){let t,e=r[0].data.content+" [trimmed]
david-plugge commented 1 month ago

Sounds like disabling client side routing is what you want to do here

ckiee commented 1 month ago

It would have to be a global client-side-app-wide toggle to make sure they cannot recover it from the route manifest, and that just completely degrades the UX site-wide.

My PoC has preloading on the secured page (and everywhere else) & doesn't treat a filename in the build as a secret so I think it is still the better solution.

Maybe it can be easier to understand as a video: (pay attention to the latest requests in DevTools) ![gif of the site working as i described](https://github.com/user-attachments/assets/59182751-f636-456f-8976-04bfe80982ca)