Attack vector arising from naive developer use of the +layout.server.js tree
#6315
Comments
|
For security you need to check auth on every request. Every route can be fetched directly, without using the app UI at all, and each fetch is independent of anything that happened previously (although they can "collaborate" with the new |
|
It sounds like what you want is a way for a import { gateAuthenticated } from '$lib/auth';
import type { LayoutServerLoad } from '../../../.svelte-kit/types/src/routes/launch-codes/$types';
import type { LaunchCodeLayoutData } from './shared';
export const load: LayoutServerLoad = (event): LaunchCodeLayoutData => {
+ event.alwaysRun();
const signedIn = gateAuthenticated(event);
return { signedIn };
}; |
|
@Rich-Harris My gut is that this is a "more documentation" issue. |
|
Ack, I see I wrote this:
Ungarbled: For the vector to be in play, the developer would only have to forget to do So yes, I'd be on board with The footgun would still exist, but |
|
Why not |
|
@TranscendentalRide Adding an // SvelteKit type...
export type LayoutServerGuard = (event: RequestEvent) => MaybePromise<void>;
// src/routes/kittens/[kittenId]/dashboard/+layout.server.ts...
// as now, refetched only when invalidated
export const load: LayoutServerLoad = async (event): Promise<KittenDashboardData> => {
const { kittenId } = event.params
const kitten = await db.findUnique({where: {id: kittenId}});
return { kitten };
}
// called for every navigation to page in `/kittens/[id]/dashboard/*`
export const guard: LayoutServerGuard = async (event): Promise<void> => {
// authorize the current user in relation to the kitten dashboard
// could be just reading a cookie in many cases -- i.e. quick
// if the user isn't authorized, throw error or redirect, otherwise void
}...where But maybe that's gilding the lily. I'd be happy with |
|
@cdcarson Maybe... but that shouldn't matter I believe the declarative benefits of this "plus paradigm", ie |
|
Today I learned about an interesting quirk of SvelteKit's invalidation that I think we can use to our advantage for this exact scenario. We can force the load function of This issue was referenced in this video demonstrating the current problem of protecting routes by way of only checking the auth in Check out this repo that I forked from @huntabyte to see it in action. To accomplish this, I'm adding Adding a By adding a Obviously, this isn't an ideal solution by any means, but it's an interesting one that I stumbled upon. |
|
Adding my two cents here: Handling authorization is currently easy to mess up.
You will end up with a lot of duplicated code because you need to check authorization in each exported function of every There were already described a few solutions in the comments above:
In my opinion adding
I think this would fit really well into the current concepts of
And If someone does not like the additional file, he does not have to use it and can implement auth like we currently have to do. |
|
@ivanhofer Question. Let's say I have a whole bunch of routes for .
└── routes
└── kittens
└── [id]
└── dashboard
├── +auth.server.ts
├── +layout.server.ts
├── +page.server.ts
├── revenue
│ └── +page.server.ts
└── settings
└── +page.server.tsWould whatever is exported from |
|
As a fundamental rule, authorization has to happen on each request to the server before data is loaded and returned (and making the load incorporate the user identity or role is significantly better than "did they reach this load function somehow"). If you don't do it in the page load, and don't make that load conditional on the layout load by awaiting it, then all you have is a veneer of security ... as long as people navigate your app correctly. There's nothing stopping anyone requesting the __data.json files directly without even going through your app UI. I like the concept of the +auth file that applies to everything below it, but IMO there's no reason to make it specific to auth - what it effectively becomes is middleware that can be defined to run at any point in the routing tree and might often be used for auth, but could be useful for other things. So why not something like the current hooks, which is running at the root? |
Yes, or else you would not really gain any benefits except from splitting auth into a separate file.
So basically everything stays the same, just the auth concept get's executed first. It is basically the same as As I'm writing this I realize that I haven't thought of the distinction between
So you are basically talking about #6731? Maybe the existing
I'm currently not aware of any usecase where something would need to run in a similar way for the full tree as authorization requires it. If it just involves a single page, you can simply call a function at the top of the |
Found another use-case where something needs to run for all path segments: page metadata or more precisely breadcrumb navigation. But in this case, it just needs to run a single time and not for every request. For such a feature you probably would need to get an object with metadata e.g. It is a bit painful to implement this currently, but I'm not sure if such a feature should be offered by |
|
I'm of two minds about this. If SvelteKit keeps the notion of But after messing around with |
This authorization section was added to make sure a few caveats with SvelteKit were well documented to anyone using the library. The problem is documented here: sveltejs/kit#6315 Essentially, propagation of data between leafs is not guaranteed when using the +layout.server.ts file as its load function is not guaranteed to rerun every page change. The current approach to solve this is to do authorization in each +page.server.ts file and additionally make sure to grab the session data by awaiting the parent instead of directly accessing the $page store, to make sure the information there is current.
|
Interesting debate. I'll join @cdcarson case (the first at least 😅), it's likely more of a documentation/best practice thing. |
* chore(docs): Session management sample for Svelte Added a code sample for managing the session through the $page store. The sample demonstrates how to retrieve the session data in the root +page.server.ts file and make it globally accessible through the $page store, simplifying state management in the application. The previous examples already used the data available in this store but did not show how to set it. * docs: Add authorization section to SvelteKit docs This authorization section was added to make sure a few caveats with SvelteKit were well documented to anyone using the library. The problem is documented here: sveltejs/kit#6315 Essentially, propagation of data between leafs is not guaranteed when using the +layout.server.ts file as its load function is not guaranteed to rerun every page change. The current approach to solve this is to do authorization in each +page.server.ts file and additionally make sure to grab the session data by awaiting the parent instead of directly accessing the $page store, to make sure the information there is current. * docs: Fix small typesafety mistake in SvelteKit PageLoad type should actually be PageServerLoad. Not setting this does not actually generate any problems other than TypeScript complaining that this type is not actually exported. * docs: Add handle hook authorization management Another way to handle authorization is through a path-based method. This added part of the documentation uses the handle hook to protect certain routes based on their path. The previous method which is per-component is still present. * docs: Simplify component approach for Svelte auth Using event.locals.getSession() exposed by SvelteKitAuth instead of relying in the root layout file making that available in the $page store. * docs: Complete SvelteKit authorization docs Finalize the explanation for the URI-based approach and also clarify interactions with the component-based approach. * docs: Add formatting to vars in the SvelteKit docs Format the variables like this: `var` so that it appears clearly as code when reading the documentation. Co-authored-by: Thang Vu <[email protected]>
Agreed! Naming is hard and calling it What about using the word Of course the word hook is already used in |
|
FWIW, I think the simplest way to describe the correct way of handling authentication is to say, all authentication logic needs to go into The issue I've found with putting it into e.g.
export const handle = (async ({ event, resolve }) => {
const user = await getUserFromCookieOrHeader(event)
if (shouldProtectRoute(event.route.id) && !user) {
throw error(401) // render the error.svelte page where you check for 401 and optionally render a login component
}
return resolve(event)
}) satisfies Handle
function shouldProtectRoue(routeId: string) {
return routeId.startsWith('/(protected)/')
}It's not pretty but seems to work. Looking into the Next.js Auth framework for Kit it appears this is what they do. https://github.com/nextauthjs/next-auth/blob/main/packages/frameworks-sveltekit/src/lib/index.ts#L143 In fact, the comment linked to above describes the issue this thread is addressing pretty succinctly. |
|
Noodling on the above for a second... You could also appropriate layout groups for role based access control. This opens up some interesting possibilities like nested ACL checks. E.g. "/" is public, "/dashboard" requires a user, "/dashboard/settings" requires an admin role. etc. Simple example:
export const handle = (async ({ event, resolve }) => {
const user = await getUserFromCookieOrHeader(event)
const role = user?.role
// Look for the layout group "requiresUser" in the route tree and use that to determine if everything
// in the tree hierarchy requires a user is present in the cookie/header
if (event.route.id.includes('/(requiresUser)/') && !user) {
throw error(401, 'requires authentication')
}
// Check for the "requiresAdmin" layout group and run the appropriate RBAC logic
if (event.route.id.includes('/(requiresAdmin)/') && role !== 'ADMIN') {
throw error(401, 'requires admin')
}
return resolve(event)
}) satisfies HandleI'm not a fan of the directory/file-naming-as-an-api approach that Kit takes, but it does allow for some useful configurations. |
|
@coryvirok I agree, in For static routes and just a few roles this can be done in a managable amount of code like you showed above.
In my opinion having a folder-based auth solution really benefits when handling with complex use-cases. Autorization is not just black or white and therefore needs strong support from the metaframework. It should not be left to user-land. At least not in it's current state where it is far too easy to mess up. |
|
Ya I'm with you on each point. I don't think the solution I added above is a good solution, just one that works with the framework as it is. Regarding the error component Chrome, I opened a discussion about this yesterday when I realized errors in hooks.server.ts were rendered with the fallback error.html file - #8393 Regarding folder based auth strategy, I know @benmccann is/was interested in creating a way to configure routes that didn't rely on folder/filenames and could be more programmatic. That's the method that other frameworks use to enable complex auth guards like the ones you mention. Eg. Routes.js would define the url path + params + param validation + before/after hooks, etc. IMO Adding more and more logic into the folder names or file names diverges the framework further and further from convention that devs are used to. Which is why I'm not a big fan and I'd consider the solution I posted above to be a hack. |
|
As I mention here (#6315 (comment)) just do it all in hooks.server.js. It's not pretty, but it works to secure all server-side routes. This is what Next Auth for SvelteKit does - https://github.com/nextauthjs/next-auth/blob/d094c6f4d9881af5b803fa707aadd8819e2427cd/packages/frameworks-sveltekit/src/lib/index.ts#L138 |
The problem is not that there's is not a way to do route-level hooks, authentication, guards (whatever one wants to call them). There are ways to do it. The problem is that Fixing the issue means fixing the footgun. This could mean making a new feature be the more obvious place to do it, or it could mean fixing the docs to explicitly provide advice on how to do it. I will say that matching on something like |
True, an app usually fetches data "given the user's authentication". For example But any time you have a reference to some shared asset, and that reference isn't stored in the user schema, the app will have to fetch data "for an authorization check". Concrete example: the layout route is
For the basic (and more critical) authentication case, adding a more intuitive place would remove the footgun, but as soon as someone wants to pass data around used by a permission check, they'll think, "Oh, I'll just do it in Whether or not either scenario is addressed by the framework, the hazards of both would at least be mitigated by updating the docs and tutorial. Theres a PR open for the docs, but not the tutorial AFAIK. |
|
I support the idea of non root +hooks. The first thing I tried when moving on to sveltekit was creating a hook for a specific route and all sub routes as that felt like the most natural thing to do. Was quickly forced on to docs and had to put in the root. Some times I just want to create Middleware and separate it from the main hooks, isolating it only for selected endpoints. It just feels the closest to every http framework out there to me. Adding to this, as a raw idea, if hooks were scopable and made an utility prop like use(req,res,next) available, it could potentially unlock the whole express ecosystem and make writing sveltekit Middleware easy. (things like rate limiter, redis sessions, helmet etc) |
|
Given the upcoming changes with Svelte 5 and the potential for streamlining the code structure, I'd like to suggest a breaking change: removing +hooks.server.js and incorporating the handle function directly into +layout.server.js, and the handle function would run on every request made to the child pages of the +layout.server.js. This could offer a more straightforward approach while maintaining the integrity of the load function. What are your thoughts on this idea? |
|
FWIW, what I find also confusing: A rerun of Is there a reason that a rerun of |
|
That's got a separate issue at #9355. It's unintuitive (to me) that although the load function reruns and the fresh data is available to children via export const load = async ({ url }) => {
url.pathname;
|
|
Since SvelteKit works around folder based routing, what if hooks worked in the same way? That way, if you need auth middle ware code, just throw a "hooks" file at the root level of where you want anything below it protected. |
|
@timothycohen Thanks! Re the general discussion, I use And why need both |
|
Follow-up: This is more tricky than I thought. Using the So, how can we let |
@mostrecent I'm using an array of unprotected routes (or prerendered routes), in my project. |
|
I know I’m late to this conversation but I’ve been spending the last few days trying to figure out best approaches for protecting pages from being rendered whether doing it via SSR or CSR. I get that hooks.server.ts seems to be the method for SSR redirects but there doesn’t seem to be a great way for hooking into client side navigation. In my react apps I write my own router that had an onBeforeNavigation callback that would return a promise Boolean that I could stop navigating happening or return a URL that would redirect. I think if sveltekit would update its hooks.client.ts with something to this effect it would solve client side navigation blocking in one location instead of the suggested approach of put an empty page.server.ts load function in every single page which seems prone to user error. Also I’m pretty sure that would mean we are doing a server request before navigating. |
SvelteKit has
It's simpler and safer to just use an empty +page.server.ts. Yes, it requires a server request before navigating, but I think that is safer and more secure. Ideally, SvelteKit would provide some mechanism so all routes/sub-routes are protected unless there is an explicit opt-out. |
That's exactly what i have thought so many times: |
Isn't this doable via onNavigate() in layout.svelte? https://kit.svelte.dev/docs/types#public-types-onnavigate Also, the new reroute functionality might allow you to do this. But I haven't tested this use case for it. |
|
Really hope something ends up happening I don't want to have +page.server.ts for all my protected routes |
If your routes are protected, I presume it's because they access sensible data. In that case, do you not already have a If you protect routes only to keep some parts of your UI private without them containing specially sensible data, then using layout-based protection is completely fine. |
|
This is how I am checking session state in the application Server Sidehooks.server.ts const userSessionInterceptor = (async ({ event, resolve }) => {
const sessionId = event.cookies.get(lucia.sessionCookieName);
if (!sessionId) {
event.locals.user = null;
event.locals.session = null;
redirect(302, `${LOGOUT_PAGE}`);
} else {
// continue with the session
}
}) satisfies Handle;Client Sidesrc/routes/+layout.svelte const checkUserSessionValidity = debounce(async () => {
const request = await fetch(
`${window.location.origin}${BASE_PATH}/api/get-session-status`
);
const response = await request.json();
const isSessionValid = !!response?.status;
if (!isSessionValid) {
goto(LOGOUT_PAGE);
}
}, 1 * 1000);
beforeNavigate(() => {
checkUserSessionValidity();
return true;
});src/routes/api/get-session-data/+server.ts export const GET = (async (event) => {
const sessionId = event.cookies.get(lucia.sessionCookieName);
return new Response(JSON.stringify({ status: sessionId }), { status: 200 });
}) satisfies RequestHandler;So far things are working great! |
Now that I think about it though I may not need to protect the routes really because the data on them is gonna be blocked unless you have the permission to view it/modify it but it still would be nice for a clean way to just block pages under a route I don't really store anything sensitive anyway because I have oauth login and I just have basically username and discord user id and then some things that are public anyway |
I've been doing it this way for over a year now. See the comment for pros and cons. |
I've created a new sveltekit-2 project and moved the files over from your repo. Adding |
|
I wrote a blog article to try and describe this issue and suggest an approach that I've found useful in my own apps: |
and others
Describe the bug
tldr;
+layout.server.jstree will lead folks to put logic there rather than in+page.server.js.+layout.server.jsis skipped.Consider the following case. (Repo link below)
Let's say I naively (but I think quite naturally) do the following things:
+layout.server.jsfile. Perhaps I plan to elaborate this section of the site withlaunch-codes/[id].+page.server.jsto fetch a paged list of codes from the database.Essentially I have separated the concerns of authorization and data collection.
Now this is totally secure as long as my user remains signed in. But let's say the following happens:
/launch-codes. This first page view runs the authorization logic in+layout.server.ts./launch-codes?page=2. The authorization logic in+layout.server.tsis skipped, and the signed out user is shown the launch codes.I don't think there's a way to reliably fix this in the framework. The client side router decides what to fetch, and that AFAICT will always be manipulable. The only way to "fix" it is by documentation.
(Or we could get rid of the layout data tree notion entirely. Not a bad option, IMO, but a separate losing argument.)
I know all this may seem contrived, but:
+layout.server.jsis for.Reproduction
Reproduction here, as minimal as I could make it: https://github.com/cdcarson/yellow-submarine.git
git clone https://github.com/cdcarson/yellow-submarine.git cd yellow-submarine npm i npm run devsignedIncookie. (I.e. the cookie has expired or you've been signed out on another tab.)/sign-inI made a separate route with a "non-naive" implementation, getting rid of
layout.server.jsand doing the authorization in+page.server.ts./launch-codes-fixedwill be redirected to/sign-inLogs
No response
System Info
System: OS: macOS 12.4 CPU: (8) arm64 Apple M1 Memory: 70.06 MB / 8.00 GB Shell: 5.8.1 - /bin/zsh Binaries: Node: 16.13.2 - ~/.nvm/versions/node/v16.13.2/bin/node npm: 8.1.2 - ~/.nvm/versions/node/v16.13.2/bin/npm Browsers: Chrome: 104.0.5112.101 Safari: 15.5 npmPackages: @sveltejs/adapter-auto: next => 1.0.0-next.66 @sveltejs/kit: next => 1.0.0-next.442 svelte: ^3.44.0 => 3.49.0 vite: ^3.0.4 => 3.0.9Severity
serious, but I can work around it
Additional Information
No response
The text was updated successfully, but these errors were encountered: