kit

kit copied to clipboard

Describe the bug

tldr;

• The existence of the +layout.server.js tree will lead folks to put logic there rather than in +page.server.js.
• Because SvelteKit decides on the client what to fetch from the tree, there are edge cases where important (e.g., authorization) logic in +layout.server.js is skipped.
• This may seem a contrived edge case, but it is an attack vector that needs to be addressed through documentation.

---

Consider the following case. (Repo link below)

src
└── routes
    └── launch-codes
        ├── +layout.server.js
        ├── +page.server.js
        └── +page.svelte

Let's say I naively (but I think quite naturally) do the following things:

• I place authentication/authorization logic in the +layout.server.js file. Perhaps I plan to elaborate this section of the site with launch-codes/[id].
• Meanwhile, I use +page.server.js to 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:

• The user goes to /launch-codes. This first page view runs the authorization logic in +layout.server.ts.
• They are signed out, say on another tab, or because the cookie expires, or just by deleting the cookie in the console.
• In the original tab, now signed out, they go to /launch-codes?page=2. The authorization logic in +layout.server.ts is 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:

• It's not that far-fetched for a developer to assume that separating authorization concerns is partly what +layout.server.js is for.
• It's not that far-fetched to assume that some apps may require quick cookie expiration. Think your bank.
• It exists. It's an attack vector.

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 dev

• Go to http://127.0.0.1:5173/launch-codes
• Assuming you are not signed in, you'll be redirected to a sign in page, and redirected back after clicking the button
• Navigate through the paginated list
• In the tab's console, delete the signedIn cookie. (I.e. the cookie has expired or you've been signed out on another tab.)
• Without refreshing the page, click to another page in the list. You will be shown that page, rather than being redirected to /sign-in

I made a separate route with a "non-naive" implementation, getting rid of layout.server.js and doing the authorization in +page.server.ts.

• Go to http://127.0.0.1:5173/launch-codes-fixed
• Replicate the steps above. This time, after deleting the cookie, subsequent navigations through the list at /launch-codes-fixed will be redirected to /sign-in

Logs

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.9

Severity

serious, but I can work around it

Additional Information

No response

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 await parent()

It sounds like what you want is a way for a load function to say 'always run me, even if you think you don't need to'. Straw man:

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. event.alwaysRun() could just as well be omitted as await parent() (or however it is now recommended to ensure the entire tree is invalidated.) Granted, to footgun the omission is in one +layout.server.js rather than in each leaf. Might make sense.

Ack, I see I wrote this:

Granted, to footgun the omission is in one +layout.server.js rather than in each leaf. Might make sense.

Ungarbled: For the vector to be in play, the developer would only have to forget to do event.alwaysRun() in the one +layout.server.js where the logic exists, rather than forgetting to repeat the logic in any one of many +page.server.jss.

So yes, I'd be on board with event.alwaysRun() if it's guaranteed that the client router couldn't override it.

The footgun would still exist, but event.alwaysRun() affords the opportunity to explain how to avoid it in a concrete. memorable way.

Why not +auth.server.js since authentication is among the basics of apps.

@TranscendentalRide Adding an +auth.server.js filename would light a dumpster fire 🔥. (See the discussions in the repo.) But I see where you're going. Right now the +layout.server load tree is all about caching, a fundamentally different concern from auth. So, maybe something like this in +layout.server.js...

// 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 guard, if it exists, would (1) always be called and (2) always beawaited before load (if load is called.) There's also a case where one would want guard without load. Note that LayoutServerGuard deliberately doesn't return anything, maintaining its independence & simplicity.

But maybe that's gilding the lily. I'd be happy with alwaysRun if that's simpler to implement.

@cdcarson Maybe... but that shouldn't matter

I believe the declarative benefits of this "plus paradigm", ie +function.location.js affords us not to need to dig into the code to see how it renders. Back pedaling on it give more power to the idea that this is the wrong direction.

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 +layout.server.ts to rerun every time we navigate to and between the child routes of a layout group.

This issue was referenced in this video demonstrating the current problem of protecting routes by way of only checking the auth in +layout.server.ts files, without awaiting the parent on each route.

Check out this repo that I forked from @huntabyte to see it in action. To accomplish this, I'm adding url.href here as an almost equivalent event.alwaysRun() function that Rich presented, but not quite.

Adding a url.href anywhere in the load function works because url.href has now become a dependency of said load function. If any dependency of the load function changes, the load function will rerun, as specified here.

By adding a url.href in the layout load function, we no longer have to add an await parent() in each child +page.server.ts load function.

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.

1. The main issue was already described above:
   If you don't explicitly run all checks on each request, someone could access data he isn't allowed to see. Unexperienced users probably won't notice the issue. They will think if they protect a node inside this tree structure, each leave will also be protected.
   Having to manually add await parent() is too easy to forget. Even if you are paying attention, you will probably mess it up at some point in the future and you won't spot the issue until it's to late.

2. Another smaller issue is: because all load functions per default run in parallel, data could get fetched from the DB before a parent load function detects that the user is not authorized. Not a big issue since the data will not get passed to the user, but also not ideal, since it could make an expensive call to the DB.

3. And yet another issue I can see is that regular endpoints must also be protected individually. Which is fine, but I guess this is not clear to all developers.

You will end up with a lot of duplicated code because you need to check authorization in each exported function of every +*.server.js file. And also easy to mess up when you alter the check somewhere, but forget to change it everywhere else.

There were already described a few solutions in the comments above:

• event.alwaysRun(): I don't think this is a good solution as it only solves issue 1
• export const guard inside +*.server.js files: would solve issue 1 and 2
• +auth.server.js: would solve all 3 issues

In my opinion adding +auth.server.js would be a great addition to SvelteKit

• Before a load or an endpoint function get's called, all +auth.server.js files get checked. If one of those checks throw an error object, nothing gets executed and the error Response get's returned.
• Inside the auth file, someone could implement advanced auth checks, handling POST and DELETE independently. As those files probably tend to be just a few lines short, it would be really easy to understand. In contrast to a load function where you need to find out where auth code stops and where the actual data fetching happens.
• Having auth defined as a file inside a directory makes it obvious that the route is protected somehow.
• I also really like that the order matches the flow in which those files get called:

   /route
   	/+auth.server.js
    	/+layout.server.js
    	/+page.server.js
   	/+page.svelte
  

  auth happens before layout which happens before page and at the end the .svelte file gets rendered.

I think this would fit really well into the current concepts of SvelteKit, would make thinks more obvious and easier to maintain.

Would a +auth.js (without server) also make sense? For my use-cases probably not, but maybe someone could need it.

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 kittens/[id]/dashboard, all of which share the same authorization logic...

.
└── routes
    └── kittens
        └── [id]
            └── dashboard
                ├── +auth.server.ts                
                ├── +layout.server.ts
                ├── +page.server.ts
                ├── revenue
                │   └── +page.server.ts
                └── settings
                    └── +page.server.ts

Would whatever is exported from routes/kittens/[id]/dashboard/+auth.server.ts run before all the descendant loads? For example, would it run before the load in routes/kittens/[id]/dashboard/revenue/+page.server.ts? Or is is just run before the layout and page loads in its own directory?

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?

Would whatever is exported from routes/kittens/[id]/dashboard/+auth.server.ts run before all the descendant loads? For example, would it run before the load in routes/kittens/[id]/dashboard/revenue/+page.server.ts? Or is is just run before the layout and page loads in its own directory?

Yes, or else you would not really gain any benefits except from splitting auth into a separate file. So the order would be:

1. run all +auth.server.js
2. run all +layout.server.js
3. run all +page.server.js
4. render the page tree (.svelte files)

For simplicity reasons I have omitted the non-server files.

So basically everything stays the same, just the auth concept get's executed first. It is basically the same as +layout.server.js with the small addition that the full tree get's executed each time a request gets made.

As I'm writing this I realize that I haven't thought of the distinction between layout and page for the auth perspective.

• Is it enough to let the user decide inside that file via if-else if he wants to run the check just for this page or the full tree?
• Maybe the +auth.server.js file could export a export const layout = () => ... and a export const page = () => ... function?
• Do we need +layout.auth.server.js and +page.auth.server.js? please not 😅

---

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?

So you are basically talking about #6731?

Maybe the existing +*.server.js files could optionally expose some functions

• doBefore: runs before all load and endpoint functions
• doAfter: runs after all load and endpoint functions
• other ?

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 load function. Authorization is a big concern that probably most bigger applications will need. So having a distinct auth feature makes sense in my opinion.

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

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. title and href for each path segment and combine them into an array with the correct order at the end of a request. This information can be passed as a data prop to the layout. Currently this also requires you to use await parent() for each path segment or else you would not be able to combine the information.

It is a bit painful to implement this currently, but I'm not sure if such a feature should be offered by SvelteKit directly. On the other hand an easier solution to the authorization problem should definitely be offered by SvelteKit.

I'm of two minds about this.

If SvelteKit keeps the notion of layout.server.ts it should definitely provide a way to guard routes hierarchically (i.e. what @ivanhofer is talking about, where a guard defined in a layout node runs before all the child loads. Importantly, the guards should operate separately from the load functions.

But after messing around with layout.server.ts, I've abandoned it (at least for now) except at the root level, to read an auth cookie and fetch profile data. For my project caching other data on the user's browser doesn't make much sense. If Bob and Alice share a set of data, they should see the same thing, fetched fresh from the server. So for me at least the original concern of this issue -- i.e. assuming that auth logic in a layout load would run on each applicable request -- has gone away. So, maybe get rid of +layout.server.ts? Just a thought.

Interesting debate. I'll join @cdcarson case (the first at least 😅), it's likely more of a documentation/best practice thing. +layout.server.ts is still very useful for it's main purpose: load data at a layout level, which can be used by +layout.svelte or any of its childs. And responding to @ivanhofer, I consider that being able to load data while waiting for user authorization is quite a nice feature to have. An opt-in method, like export const guard or a specific file will be a nice addition.

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?

Agreed! Naming is hard and calling it +auth.server.ts will inevitably pigeonhole people's thinking about how they might use the feature.

What about using the word hook? If going with a file, something like +hook.server.js, or if exporting from +layout.server.js maybe it's a function called hook.

Of course the word hook is already used in src/hooks.client.js and src/hooks.server.js, but I think keeping it singular hook instead of hooks would help with the distinction.

FWIW, I think the simplest way to describe the correct way of handling authentication is to say, all authentication logic needs to go into hooks.server.ts before the call to resolve(event). Full stop. Every other options runs into complications like parallel loading, easily forgettable guard checks, server vs client loading, etc.

The issue I've found with putting it into hooks.server.ts is that you essentially have to do route pattern matching to determine if you want to guard the route. Which is made slightly easier with route groups. And since layout groups can be nested (who knew?!) you can essentially just sprinkle in (protected) into your route tree to protect disparate hierarchies of routes

e.g.

• src/routes/
  • (protected)
    • +page.svelte
  • login/
    • +page.svelte

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:

• src/routes
  • (app)
    • (requiresAdmin)
      • admin/
        • +page.svelte
    • (requiresUser)
      • dashboard/
        • +page.svelte
    • login/
      • +page.svelte
      • (requiresAdmin)
        • secret-admin-functionality/
          • +page.svelte
  • (marketing)
    • +page.svelte

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 Handle

I'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 SvelteKits current state, auth handling is best handled in hooks.server.js, if you really want to make sure to not accidentially introduce security vulnerabilities.

For static routes and just a few roles this can be done in a managable amount of code like you showed above. But things can get more complex really fast:

• what if you want to respond with an error page that has the same layout as the group the path is in? e.g. /dashboard should show the usual frame around the content (error message) and /settings should show the settings-menu. With the plain hooks.server.js approach, you will see a plain error page, without the possibility for users to "recover from their mistake". A simple "go back" button isn't always the best option.
• what if you want to show custom error messages depending on the route? e.g. (requiresUser) for an unathenticated on /settings should show a message that says "You need to login first", but for an admin it should show "you need to change your profile in Azure AD". If you have different outcomes for different roles an routes, you need to replicate parts of your folder structure in hooks.server.js. Rename a route, and forgetting to take a look at the auth logic and you will have an unprotected route.
• what if your authentication logic depends on a dynamic slug? e.g. user A is owner of the project with id 1 and should be able to access /projects/1 but not projects/2 because it belongs to user B. Inside hooks.server.js you can't access the params in a typesafe way (issue when refactoring) and you would need to add code that is specific to that part of the application because you need to load the project from the db to see if it belongs to that user. Having some similar routes with all those checks and you probably will not ever touch the auth code again because it grows in complexity really fast and therefore is not easy to maintain.

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 - https://github.com/sveltejs/kit/discussions/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.

Whatever happens, I really hope we avoid string-based pattern matching for routing logic anywhere, including auth logic in hooks.js. The whole point of file-based routing is to avoid the problems that come from that approach, and reintroducing it would be a step backwards, especially in a framework that strongly uses file-based routing everywhere else.

Kinda disagree about using hooks for auth guards. You have to add url pattern matching and it gets messy. While it's a bit more verbose, adding a server file to each protected route with explicit auth checks is the easiest and the most clear.

Adding an auth check function inside locals will clean up the code:

const session = await locals.validate():
if (!session) {
  // redirect
}

and you can memoize them to mitigate unnecessary waterfalls.

Example here: https://lucia-auth.vercel.app/sveltekit/basics/get-session-in-the-server

@pilcrowOnPaper No need for manual parsing of the url, you can recover route params from event.params

https://kit.svelte.dev/docs/types#public-types-handle https://kit.svelte.dev/docs/types#public-types-requestevent

Either way, you have to remember to add every route to the list - it's safer to apply something to each route to define what permission checks need to be passed. Too easy to add or rename something and then it loses it's association which is a weak string reference.

My normal goto approach would be some TS decorators to define & apply these, I'm not sure those will work with SK typing though.

e.g.

import type { RequestHandler } from './$types'

@roles('admin', 'moderator')
export const POST: RequestHandler = async ({ request, cookies }) => {
  //
}

Either way, you have to remember to add every route to the list - it's safer to apply something to each route to define what permission checks need to be passed. Too easy to add or rename something and then it loses it's association which is a weak string reference.

src
  hooks.server.ts
  routes
    +layout.svelte
    [lang]
      ... the rest of the site

if you have a site with the above folder structure than you should not have any problem with eventual refactorings.

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.

I'm not a fan of the directory/file-naming-as-an-api approach that Kit takes, but it does allow for some useful configurations.

I've just arrived here from watch this Video by Huntabyte: https://www.youtube.com/watch?v=UbhhJWV3bmI and i'm trying to implement this for Firebase. A bit of can of worms I see here, but can I confirm, is the implementation in Huntabytes video the best/safest option available now? Could it successfully work with the public/admin/user folder pattern you've described above (which at present makes a bit of sense to my mind)?

I don't claim that this is the right way, only a convenient, (albeit hacky) way to do things. I've implemented it in my project and it does indeed work.

I'd love to hear from a maintainer on the plan for auth guards. Or better yet, handle-like hooks for route trees.

Some thoughts. Usual caveats about IMO, etc.

The original issue was just about how +layout.server.js is presented. It's a browser cache, not middleware. The issue pointed out that it might be reasonable to assume that it was middleware -- executed on every applicable request -- more or less the exact opposite of what it's for.

As for authorization via actual middleware, to which this discussion has turned. One of the great things about SvelteKit is that it concentrates on the logic of the route at hand, not middleware. So I can do this...

// routes/special-dashboard/[id]/+page.server.ts
import { guardSpecialDashboard } from './shared.server.js';
export const load = async (event) => {

  // throws redirect if the user isn't authenticated, throws error if the user isn't authorized
  const { currentUser, permissions, specialDashboardData } = await guardSpecialDashboard(event);

  // get whatever else the particular route requires....
 const foo = await db.foo.findUnique({where: {id: specialDashboardData.id});

  return { currentUser, permissions, specialDashboardData, foo };
}

...which is way more straightforward, flexible and strongly-typed than relying on middleware (hooks, route functions, or whatever) to pass stuff down via locals. The guard is enforced by the pattern of the guard returning data. I can change the implementation of guardSpecialDashboard without messing anything else up. The return type of the guard is obvious.

My preference would be for SvelteKit to:

• Say (more explicitly) that +layout.server.js isn't middleware but a just way to cache data in the browser.
• Encourage the route-centric pattern for auth over middleware.

I have spent some hours to create a repository that should demonstrate the pain-points of using authorization in a current SvelteKit project: https://github.com/ivanhofer/sveltekit-auth It also shows how the +auth.server.ts proposal from above could eliminate those pain points. Feel free to share your feedback!

I think that example makes it appear way worse than it really is. Having repeated authGuard functions in each server load makes it look way more onerous to do auth than it needs to be. If instead you have a single authGuard function, each load can call it and pass a list of required roles so things become clearer to read and simpler to add to protect a load:

import { error } from '@sveltejs/kit'
import type { LayoutServerLoad, LayoutServerLoadEvent } from './$types'
import { authGuard } from '$lib/utils/authGuard'

export const load: LayoutServerLoad = (event) => {
  authGuard(event, 'manage-payments')
  // load & return data
}