Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route protection not working in SvelteKit v2 #9454

Closed
WhyAsh5114 opened this issue Dec 24, 2023 · 11 comments
Closed

Route protection not working in SvelteKit v2 #9454

WhyAsh5114 opened this issue Dec 24, 2023 · 11 comments
Labels
bug Something isn't working triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime.

Comments

@WhyAsh5114
Copy link

WhyAsh5114 commented Dec 24, 2023

Environment

(also tried in Windows 11 with similar config)
System:
OS: Linux 6.2 Zorin OS 17 17
CPU: (12) x64 AMD Ryzen 5 3600 6-Core Processor
Memory: 10.69 GB / 15.52 GB
Container: Yes
Shell: 5.1.16 - /bin/bash
Binaries:
Node: 20.10.0 - ~/.nvm/versions/node/v20.10.0/bin/node
npm: 10.2.5 - ~/.nvm/versions/node/v20.10.0/bin/npm
npmPackages:
@auth/core: ^0.19.0 => 0.19.0
@auth/sveltekit: ^0.5.0 => 0.5.0

Reproduction URL

https://github.com/WhyAsh5114/authjs-sveltekitv2

Describe the issue

Using a sequence hook with SvelteKitAuth and an authorization function is not working as intended.
Protected route can still be accessed with no session, need to reload to redirect as expected.

How to reproduce

  1. Setup with SvelteKit 2 barebones
  2. Install @auth/core and @auth/sveltekit
  3. Basic home page and sign in page
  4. A protected route
  5. In hooks, use a sequence with authorization which redirects if route is protected
  6. Run dev server
  7. Go to protected page (still works with no session)

Expected behavior

Instead of being able to see the protected route, should redirect as defined in the authorization middleware function

@WhyAsh5114 WhyAsh5114 added bug Something isn't working triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. labels Dec 24, 2023
@lknlknim
Copy link

lknlknim commented Dec 24, 2023

I recently met the same situation but usually it's no harm and the route will be automatically protected when you do server side data fetching by adding +page.server.ts
It's because when /authenticated can be both server side(init page load) and client side(user navigation by pressing links) rendered. When it's running on client side the hooks.server.ts isn't even running at all. If you try to F5, it'll be server again and do the redirect properly.
For smallest effort fix, add an empty +page.server.ts can trigger the hooks to run and protect it.

@ndom91
Copy link
Member

ndom91 commented Dec 29, 2023

Thanks for the quick tip! Makes sense to me. Closing 🙏

@ndom91 ndom91 closed this as completed Dec 29, 2023
@WhyAsh5114
Copy link
Author

It is a temporary fix though, adding a +page.server.ts to each and every route just for re-running the hooks doesn't seem like the permanent right thing to do. And not doing that leads to a confusing user experience where they see an error code 500 without any explanation, which is to simply login, which should be done automatically by redirecting from the hooks

@ndom91
Copy link
Member

ndom91 commented Dec 29, 2023

@WhyAsh5114 I'm not very familiar with Svelte/SvelteKit, could you elaborate more on your recommended solution?

@WhyAsh5114
Copy link
Author

WhyAsh5114 commented Dec 29, 2023

@ndom91 okay, so the TLDR is, if a route (page) needs some data, say from an external API, you'd use either a +page.ts or +page.server.ts file to load the needed data.

Difference is that +page.server.ts is typically used when you directly interact with a database or some environment variables are being used, and it runs on server only.
+page.ts is typically used when you have your own API, aren't interacting directly with a DB, and runs on both server and client, reusing fetch responses.

So the proposed solution is adding a +page.server.ts to each and every route that should be protected in order to force the hooks to rerun.
But not every page needs data, also some might use a +page.ts, and adding an empty +page.server.ts file to every route is doable but highly redundant and make the project structure unnecessarily convoluted (especially for projects with many routes).

I know the hooks wouldn't run unless a server-side event occurs, so I doubt there's anything we can do from the project developer's point of view to fix it.
I'm trying to make it work using hooks.client.ts which is not the default way to do things from the docs.

@ndom91
Copy link
Member

ndom91 commented Dec 29, 2023

@WhyAsh5114 yeah so I get the redundancy of adding an unnecessary +page.server.ts to each route is not ideal, but do you have a suggestion as to what we could do from a next-auth perspective to make this work as intended on SvelteKit client-only routes/pages.ts? 🤔

@WhyAsh5114
Copy link
Author

I'll try to think up something, I was getting decent results by putting the auth logic in +layout.server.ts, but I need to test it a bit since it's explicitly not recommended in the docs.

I'll get back to you after I get a decent solution.

@ndom91
Copy link
Member

ndom91 commented Dec 29, 2023

Ah gotcha, I thought yuo had something in mind already. Well thanks and keep me posted, I'm up to help ship something for this 👍

@lknlknim
Copy link

I'm not 100% sure but I believe you can set ssr to true, csr to false, in a specific layout, to make child routes server side only.
Just like if we want to prerender all pages, we set prerender to true at root layout. Then optionally opt out at page level.
However, protection in +layout.server.ts load function is discouraged because layout load won't rerun when you navigate under the same layout.

@WhyAsh5114
Copy link
Author

if we use a layout load function like so:

export const load = async ({ locals, url }) => {
	const session = await locals.getSession();
	if (!session && url.pathname.startsWith('/authenticated')) {
		throw redirect(303, '/unauthorized');
	}
	return { session };
};

I do get all the desired results, since we are using url.pathname directly in the load function, SvelteKit will mark it as a dependency and re-run the load function whenever it changes, which is whenever we navigate to a new page. So this does seem to solve all the problems.

The reason this was not recommended in the first place by AuthJS docs was because:

You should NOT put authorization logic in a +layout.server.ts as the logic is not guaranteed to propagate to leafs in the tree. Prefer to manually protect each route through the +page.server.ts file to avoid mistakes. It is possible to force the layout file to run the load function on all routes, however that relies certain behaviours that can change and are not easily checked. For more information about these caveats make sure to read this issue in the SvelteKit repository: sveltejs/kit#6315

That issue is still open and being discussed, I guess there's not much to do from your side, other than maybe updating the documentation, because IMO, the +layout.server.ts approach with url.pathname as a dependency will at least work as we expect it to unlike the recommended approach, which is to add one more authorization function as a middleware, which doesn't produce the desired result since it only runs on server requests and not client-side navigation.

Ofc this isn't perfect either, there's other problems with the layout approach mentioned in sveltejs/kit#6315, so the decision of whether or not to update the docs is subjective. I don't think there's much to do from your side since this issue is still open on SvelteKit's repo.

@ndom91
Copy link
Member

ndom91 commented Dec 30, 2023

Thanks for the deep dive on this and links! I think including the path param in the load function, to get it to run on each navigation, at least makes sense for now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime.
Projects
None yet
Development

No branches or pull requests

3 participants