add security fix in oauth.callback

[rphlmr]

Change what happens in oauth.callback and verifyAuthSession Linked to https://github.com/rphlmr/supa-fly-stack/issues/45 from @Benjamin-Dobell

What's new :

• oauth.callback action no more trust authSession it receives from the client
• oauth.callback (client side) only sends refresh_token found in onAuthStateChange
• oauth.callback action uses authSession returned by refreshAccessToken, called with the refresh_token received
• verifyAuthSession compares authSession user id and user id returned by getAuthAccountByAccessToken. It's only a last guard against session cookie secret leak

[rphlmr]

@Benjamin-Dobell Do you think it reduces the risk?

[Benjamin-Dobell]

This does look like it will address the OAuth sign-up issue. Great work! 👍

I do think verifyAuthSession looks a bit dubious though. Because it's validating that the access token corresponds with a Supabase Auth user, but it's not necessarily the access token for the user in the AuthSession. It could be any user. That said, now you've made this change, the AuthSession is generated securely server-side, in which case you don't actually need to check the Supabase auth token - you already have, and you signed your AuthSession after doing so. You may want to check to see if the Supabase auth user has since been deleted though.

My approach was to keep the AuthSession, but I'm not storing Supabase data in there, just extra domain model stuff specific to my app. I'm using the sb-auth-token as it turns out the JWT is a JWE, which allows it to be validated without a round trip to Supabase or requiring your Supabase auth secret be included in the project. Basically it's the same approach Supabase themselves are taking in their new auth helpers library (Next.js equivalent https://github.com/supabase/auth-helpers/blob/main/packages/nextjs/src/middleware/withMiddlewareAuth.ts). I've actually added @supabase/auth-helpers-shared as a dependency.

[rphlmr]

Ah, yes I see.

I said to myself that if my session cookie is compromised (someone finds the secret to sign it), I couldn't do anything more than renew this secret :/ But maybe it's a last "guardian" before an attacker compromise this access_token ?

Should I add this? 👇

async function verifyAuthSession(authSession: AuthSession) {
  const authAccount = await getAuthAccountByAccessToken(
    authSession.accessToken
  );

  if (!authAccount) return false;

  // Here we verify that what we have in the session is the same as what we have in the auth database.
  if (
    authAccount.id !== authSession.userId
  ) {
    return false;
  }

  return true;
}

[Benjamin-Dobell]

Yep, that looks good 👍

Aside: I've realised my approach I've described above will in fact still require a single round trip to Supabase. I'll then store the signature in my AuthSession so I don't need to round trip for future request. I'd previously assumed the Supabase JWT was using either asymmetric encryption or a per session symmetric key - it's not. Unless I'm missing something, it looks like Supabase's own auth helpers library (the one I mentioned) isn't taking this into consideration. I'll come back to this later though. I've gone far enough down the rabbit hole for now 😆