fix: code verifier remains in storage during edge cases

[j4w8n]

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

In some scenarios, the sb-<project-id>-auth-token-code-verifier will get created in storage, but is either not used or an error will happen during calls like updateUser(), etc. In these cases, the code verifier remains in storage, which can cause issues if third-party code any non-Supabase code uses the code search param in a url later on. See issue #911 for an example.

This requires a client-side supabase client to be running on the page that recieves the code param.

What is the new behavior?

When the code verifier is set, and the function throws an error or returns an error (in one case), we now remove the code verifier from storage. To cover success scenarios, we remove any potential code verifier whenever a session is saved; as I wasn't sure where else to handle this.

This should resolve the issue when strictly using supabase-js, but this is not necessarily the case for the ssr library because it only enforces storage changes when a whitelisted auth event also occurs (e.g. SIGNED_IN, TOKEN_REFRESHED). Since none of the existing events seem relevant in this case, I created a new one called STORAGE_UPDATED, although this might be too generic. I tried to ensure that the session is passed along with the event; so we hopefully don't disrupt any action logic within onAuthStateChange() that a lot of devs put there. The ssr PR that coincides with this is supabase/ssr#82.

I'm not sure if this is how the team wants to resolve this issue or not. Feel free to delete this PR or slice and dice it; as it didn't spend too much time on it.

UPDATE: Noting this PR will no longer resolve this issue for users of the ssr library. I've also closed the related ssr PR.

Alternatives Considered

After some short discussion with @silentworks, one thing which could be done is renaming code; which would be breaking of course.

One other idea is to somehow introduce a new name while keeping the old name. Might be a bit complicated, even if it is possible. Then, deprecate the old usage and remove in auth-js v3.

Or, you could implement something like I've proposed here, as a temporary fix, then deprecate code in favor of something else in auth-js v3, and then roll my proposed changes back. Although, if one of the goals is to resolve the "orphaned" code verifier, then you may want to keep some of the changes.

Also, to make my proposed change a bit smaller, perhaps the ssr library can be tweaked to where storage changes are written immediately, instead of waiting for an auth state change event. That would allow you to remove the STORAGE_UPDATED event proposed here.

Additional context

fixes #911
fixes #961

https://discord.com/channels/839993398554656828/1312930639066169365

As seen here, https://github.com/supabase/auth-js/blob/master/src/GoTrueClient.ts#L1535, the _isPKCEFlow() check only returns true if there is a code search param in the url and the code verifier is in storage. So, if we can cleanup the code verifier once it's no longer needed in these edge cases, this resolves the issue.

[hf]

I highly suspect this is not auth-js but rather bad usage in a SSR library.

[j4w8n]

Can you define "bad usage"?

I can reproduce the logout behavior (see Discord for a user running into this) without the ssr library.

[kmalloy24]

I ran into a similar issue following the suggested usage in the Sveltekit SSR Auth Guide from the docs.

Calling signInWithOtp creates sb-<project-id>-auth-token-code-verifier even when not needed for OTP (vs magic link). Since there is no callback in this flow that calls exchangeCodeForSession the sb-<project-id>-auth-token-code-verifier cookie never gets cleared - even by signOut.

For now I am manually deleting it since it's not needed for email OTP.

const allCookies = cookies.getAll();
const codeVerifierCookie = allCookies.find((cookie) => cookie.name.endsWith('code-verifier'));

if (codeVerifierCookie?.name) {
	cookies.delete(codeVerifierCookie.name, { path: '/' });
}

[hf]

This actually looks good now that I have fresh eyes on it. Can you please just remove the STORAGE_UPDATED event?

I'm not sure we're comfortable adding new events at this point unless necessary -- but we'll consider it in v3 of auth-js for which some initial feedback collection is taking place on.

[j4w8n]

@hf done

[j4w8n]

Actually, no it's not. Just realizing I need to rollback some of the "session" changes, since we're dropping the new event.

[j4w8n]

Ok, @hf, NOW it's ready; after fully rolling back the changes you requested.