migrate to new supabase ssr auth package

[kizivat]

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]

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]

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

[kizivat]

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]

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?

CMSaasStarter/database_migration.sql

Line 27 in 7904d41

create table stripe_customers (

[kizivat]

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?

CMSaasStarter/database_migration.sql

Line 27 in 7904d41

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]

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]

@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]

@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]

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]

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

[scosman]

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

[kizivat]

@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]

haha, of course.

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