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

migrate to new supabase ssr auth package #63

Merged
merged 7 commits into from
Aug 30, 2024

Conversation

kizivat
Copy link
Contributor

@kizivat kizivat commented May 9, 2024

As I commented on #29 there are still few

Using the user object as returned from supabase.auth.getSession() or from some supabase.auth.onAuthStateChange() events could be insecure! This value comes directly from the storage medium (usually cookies on the server) and many not be authentic. Use supabase.auth.getUser() instead which authenticates the data by contacting the Supabase Auth server.

warnings being logged. I'm unable to locate where they are coming from atm, but I'm opening this draft PR in case someone would like to have a look.

@scosman
Copy link
Contributor

scosman commented May 9, 2024

Very cool! Are you planning on updating once you find the remaining issues you described in #29? I'll hold off a detailed CR until then, probably.

@kizivat
Copy link
Contributor Author

kizivat commented May 9, 2024

Yes, I'll have a look at it in a few hours.

@kizivat
Copy link
Contributor Author

kizivat commented May 9, 2024

It looks like the warnings originate from @supabase/auth-ui-svelte. I'll be reimplementing the forms in my fork using form actions anyway.

I wanted to make PR fixing this into the supabase/auth-ui, but it says it's in "Maintenance mode" which I suppose means that the PR would go unmerged for a while.

So we have a few options here. I'd be interested in hearing which one would you prefer, @scosman

  1. go ahead with this PR and merge it anyway even with the annoying Supabase warnings; add custom auth forms in later PR
  2. add custom auth forms implementation to this PR
  3. close this PR; stick to auth-helpers for now

@daniellovera
Copy link

I'd been meaning to add those stripe_customers definitions back to this repo, so I noticed you also added an id prop. Should that also be added to the creation of that table?

create table stripe_customers (

@kizivat
Copy link
Contributor Author

kizivat commented May 9, 2024

I'd been meaning to add those stripe_customers definitions back to this repo, so I noticed you also added an id prop. Should that also be added to the creation of that table?

create table stripe_customers (

You're right, there should be no id. Neither in select nor update I'll be removing it soon. Thank you.

@kizivat kizivat marked this pull request as ready for review May 9, 2024 21:44
@kizivat
Copy link
Contributor Author

kizivat commented May 9, 2024

I made this ready for review so you could merge if needed. As you mentioned in #65 - it's a security concern. I believe #65 could have missed some spots (maybe not, not sure) though.

Weird supabase warnings can wait.

@scosman
Copy link
Contributor

scosman commented May 10, 2024

@kizivat can you check the security concern? I think I got them all. I completely removed the old method so would have caused build errors if not (I think). That's urgent because... security. Should split that out into the minimal PR if there is anything left.

For the rest: on vacation so won't be able to review till next week! But excited to get this in.

@kizivat
Copy link
Contributor Author

kizivat commented May 11, 2024

@kizivat can you check the security concern? I think I got them all. I completely removed the old method so would have caused build errors if not (I think). That's urgent because... security. Should split that out into the minimal PR if there is anything left.

For the rest: on vacation so won't be able to review till next week! But excited to get this in.

You're right, you got them all, just double checked. I didn't notice you've removed original getSession the first time I glanced the commit, sorry.

@scosman
Copy link
Contributor

scosman commented May 13, 2024

Thanks again for this PR. Looks good for quality (not final pass but initial looks good).

However: those errors sure are annoying!

I do think almost all are originating in our code. The fix suggested here (bit too much of a hack for my liking) does reduce the volume by 95%: supabase/auth-js#873 (comment) - now the code in question does come straight from the supabase guide....

Lots of other folks have the issue too, including after following latest official guides: supabase/auth-js#873

I'm hoping they update docs and/or lib with an approved approach, and am inclined to wait for that. Better old system than one firing lots of security warnings.

Also would take this if you can find a clean way to fix this. Potentially the forms you mention, and the a fix to getSafeSession like the ones suggested in the link above?

@scosman
Copy link
Contributor

scosman commented May 15, 2024

This seems to be the issue we're waiting on: supabase/auth-js#888

@scosman scosman mentioned this pull request May 22, 2024
@scosman
Copy link
Contributor

scosman commented May 22, 2024

@kizivat Does this change work with "@supabase/auth-helpers-sveltekit": "^0.11.0",? I found that worked around the blocker on another PR.

@kizivat
Copy link
Contributor Author

kizivat commented May 22, 2024

@kizivat Does this change work with "@supabase/auth-helpers-sveltekit": "^0.11.0",? I found that worked around the blocker on another PR.

well, this change removes the @supabase/auth-helpers-sveltekit dep altogether.

@scosman
Copy link
Contributor

scosman commented May 23, 2024

haha, of course.

Okay, let's wait until supabase/auth-js#888 is resolved then.

@scosman scosman mentioned this pull request May 23, 2024
@mez0ru
Copy link

mez0ru commented Jun 2, 2024

Just wanted to add something to this pull request.

I tried to migrate to the ssr auth myself, and I managed to suppress all warnings. By not using session.user, but instead using the user returned from the safeGetSession function from hooks directly whenever trying to access the user data.
using delete session.user; would help you with that if you don't know when you are actually calling the session to get the user data.
This solved the issue completely for me, so I think waiting for a fix is not good as it's actually considered a bad practice now from the vibe I get on the people commenting on this issue.

@scosman
Copy link
Contributor

scosman commented Jun 2, 2024

@mez0ru can you share the code? Is it really just a delete session.user?

I had played around with this approach, but found it broke things (supabase expects session to have a user). I can't recall what though. I've seem similar options where they delete it, then replace it with the user from getUser, but the code was ugly and didn't look robust to future issues.

@mez0ru
Copy link

mez0ru commented Jun 2, 2024

@scosman

I would like to, but my code is kinda messy and I already overwritten the template a bit. I'm still not 100% sure it supressed it, because there is a rare occasion of the log popping up after a while. When I'm sure I fixed it completely. I will try to replicate my setup on the template and share the code.

The key I think, is never using session, instead use user to check for session. Only use session when you are actually invalidate it, or check if it will expire. I tried logging out, and logging in, clicking all pages... but no logs are shown for now. I will keep you updated.

@scosman
Copy link
Contributor

scosman commented Jun 2, 2024

Try the change password functionality / settings too. I assume we need that there (but it's not fresh in my mind).

@mez0ru

This comment was marked as off-topic.

@analytically
Copy link

Is this functional?

@scosman
Copy link
Contributor

scosman commented Jul 11, 2024

@analytically it works. Just waiting a very long time on a fix to supabase so it doesn't log a ton of security warnings: supabase/auth-js#888

@kizivat looks like there are some work arounds to hide the log in the issue thread. Interested in taking this change in with one of those? Waiting for a real fix hasn't exactly been fruitful 😀

@NDruce
Copy link

NDruce commented Jul 17, 2024

Better to make auth via AuthJs

@kizivat
Copy link
Contributor Author

kizivat commented Jul 18, 2024

@analytically it works. Just waiting a very long time on a fix to supabase so it doesn't log a ton of security warnings: supabase/auth-js#888

@kizivat looks like there are some work arounds to hide the log in the issue thread. Interested in taking this change in with one of those? Waiting for a real fix hasn't exactly been fruitful 😀

Yeah, looks promising. I'll look at it and update this PR if it works out. Thanks for pointing it out.

@scosman scosman mentioned this pull request Aug 29, 2024
@scosman scosman merged commit a0fb518 into CriticalMoments:main Aug 30, 2024
3 of 4 checks passed
@scosman
Copy link
Contributor

scosman commented Aug 30, 2024

Merged this via: #135

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants