feat!: Migrate to @supabase/ssr

[felixgabler]

Started using @supabase/ssr to create clients, simplifying cookie handling.

https://supabase.com/docs/guides/auth/server-side/creating-a-client

Types of changes

• Bug fix (a non-breaking change which fixes an issue)
• New feature (a non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

Resolves: #301

I replaced the plain usage of createClient (in serverSupabaseClient, supabase.server.ts, and supabase.client.ts) with the more advanced createServerClient and createBrowserClient from @supabase/ssr. In the process, I removed some of the manual cookie handling since it should be taken care of by Supabase's auth.storage setting. This is based on my limited understanding though, so would appreciate a confirmation.

This was because we were encountering quite a few issues of cookies getting lost somewhere. Now everything seems to work fine again.

I was not entirely sure what to do about the clientOptions since they should be managed by @supabase/ssr now. Also, the cookieName is now also quite irrelevant and I did not find a good way to include it in the new setup for the other cookies. One idea would be to prefix them all again through the cookies option functions?

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes (if not applicable, please state why): No testing structure available

[netlify]

👷 Deploy request for n3-supabase pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	92bf923

[felixgabler]

There is another issue that comes up now:

WARN 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 may not be authentic. Use supabase.auth.getUser() instead which authenticates the data by contacting the Supabase Auth server.

This is actually something that should be changed in the library regardless of whether this PR is merged. getSession is not as secure as getUser, so one should assess where it makes sense to switch to the additional network request.

EDIT: After upgrading supabase-js (see supabase/auth-js#873 (comment)), they don't seem to come up anymore. However, I still think it would be important to think about whether the current approach is insecure at times.

[larbish]

Hello @felixgabler, I' don't have a lot of time for this module since I've created it for a side project that do not need evolution no more and I'm so happy when people help me to maintain it and make it evolve, thanks for that!

Concerning the PR, I faced two blocking points:

1. Server side session/user was not up to date on first load. Since you removed this line, the session was not set on server side on first load.
2. I am still facing the insecure warn about getSession

After reading the Supabase docs concerning the getSession method and taking into account your integration of supabase/user (the removal of the legacy custom server side synchronisation), I told my self that it could be the time for a refactor. So I decided to transform useSupabaseUser and useSupabaseSession into async composable. It ensures data are always up to date when called on purpose. I've updated the useSupabaseUser to be consistent with Supabase docs and avoid insecure call of getSession on server side.

Those changes include breaking changes but it make sense to release a major version once this PR is merged to be sure users make some complete test to upgrade the module.

Tell me what you think of this and if it's working well on your project.

Also I want to get rid of the getSession warning before merging this, you talked about upgrading supabase-js but I still have it.

[felixgabler]

Those changes include breaking changes but it make sense to release a major version once this PR is merged to be sure users make some complete test to upgrade the module.

That all sounds very reasonable and I welcome the changes you made to keep things more secure! Very cool.

Tell me what you think of this and if it's working well on your project.

We deployed my branch and have not faced issues so far. It actually fixed some bugs (we set cookies on the top-level domain which the subdomains inherit and I suspect something broke around this but works now with @supabase/ssr).

Also I want to get rid of the getSession warning before merging this, you talked about upgrading supabase-js but I still have it.

That is weird.. I don't get them anymore after the upgrade. Perhaps you need to delete the node_modules and .nuxt?

[felixgabler]

If useSupabaseUser is now async as well, will this lead to even more warnings like the following because there might be sequential promises in a component setup? Stumbling upon this and cannot find clear guidelines of how to deal with it in Nuxt.

[nuxt] [useLazyAsyncData] Component is already mounted, please use $fetch instead

A simple example would be fetching the user profile with useLazyAsyncData after getting the user ID from useSupabaseUser. How would you go about this?

[larbish]

I pushed two new fix:

• I was facing this issue after releasing and using the module on another project: supabase/ssr export error in browser supabase/auth-helpers#725 so i fixed it by optimising this lib
• Use getUser instead of getSession for serverSupabaseUser

[larbish]

I tried to play with useLazyAsyncData and the async resolution of the user and I'm not facing any issue/warning. Could you provide an example in the playground of the module?

[felixgabler]

I created a branch with a repro of the warning: https://github.com/felixgabler/supabase-nuxt/tree/warning-repro

The warning is shown when navigating between /login and /unprotected using the buttons.

However, I might have figured out what the problem is. Does "useAsyncData is a composable meant to be called directly in the Nuxt context" (Nuxt Docs) mean that these composables can't be reused in other composables? That would be somewhat annoying but would at least explain the warning.. If this is indeed true, do you know what the "right" way would be to share this user state?

[larbish]

Indeed I don't think that calling useAsyncData inside a composable makes sense. I don't exactly understand what you are trying to achieve so maybe I need more info to propose something but one solution could be to create a state using the useState in your composable. Then you can update this state from the page where it makes sense (using useAsyncData). Each time you need to access this state from another page/component, you can call this composable and access the state.

[larbish]

Let's closely follow this issue. Once it's fixed, we can merge this PR . Wdyt?

[felixgabler]

Indeed I don't think that calling useAsyncData inside a composable makes sense. I don't exactly understand what you are trying to achieve so maybe I need more info to propose something but one solution could be to create a state using the useState in your composable. Then you can update this state from the page where it makes sense (using useAsyncData). Each time you need to access this state from another page/component, you can call this composable and access the state.

We put useAsyncData in a composable because we have many pages/components in our app that work on the same underlying data. The problem is that there is not really one component that is always there which could become the owner of the useAsyncData, where we could update a shared state. This is where the reusability of composables really comes in handy. Does that make sense?

[felixgabler]

Let's closely follow this issue. Once it's fixed, we can merge this PR . Wdyt?

Sounds good to me!

[larbish]

Maybe you can try with this experimental feature: https://nuxt.com/docs/guide/going-further/experimental-features#asynccontext

Or what about calling the useAsyncData inside a root page or even in a plugin on app load?

[felixgabler]

Maybe you can try with this experimental feature: https://nuxt.com/docs/guide/going-further/experimental-features#asynccontext

Unfortunately, this does not remove the warnings. The good thing right now is that at least it works in SPA mode. It is only that the warnings are annoying but they can be lived with.

Or what about calling the useAsyncData inside a root page or even in a plugin on app load?

The issue here is that we also don't want to load all data if it won't be used. So calling it inside a root page or plugin is also not optimal. The only way I can think of making the useAsyncData reusable would be to extract and share its arguments, i.e., the fetching function and options. But this is also an ugly solution in my opinion.

Especially given that the convention is that all composables (i.e., anything with use... as oppose to define...) can again be composed in Vue, I think this could be an area of improvement either in docs or making it possible to compose useAsyncData.

[larbish]

Still facing the warning even after upgrading to the latest supabase-js release.

To discuss with Supabase team here: supabase/auth-js#895

[felixgabler]

Still facing the warning even after upgrading to the latest supabase-js release.

To discuss with Supabase team here: supabase/auth-js#895

I wonder if it fell off the plate. Perhaps best to create a new issue?

[felixgabler]

Maybe you can try with this experimental feature: https://nuxt.com/docs/guide/going-further/experimental-features#asynccontext

Unfortunately, this does not remove the warnings. The good thing right now is that at least it works in SPA mode. It is only that the warnings are annoying but they can be lived with.

Or what about calling the useAsyncData inside a root page or even in a plugin on app load?

The issue here is that we also don't want to load all data if it won't be used. So calling it inside a root page or plugin is also not optimal. The only way I can think of making the useAsyncData reusable would be to extract and share its arguments, i.e., the fetching function and options. But this is also an ugly solution in my opinion.

Especially given that the convention is that all composables (i.e., anything with use... as oppose to define...) can again be composed in Vue, I think this could be an area of improvement either in docs or making it possible to compose useAsyncData.

@Atinux could I perhaps also get your thoughts on this? I was quite surprised to learn that useAsyncData is not composable

[larbish]

Still facing the warning even after upgrading to the latest supabase-js release.
To discuss with Supabase team here: supabase/auth-js#895

I wonder if it fell off the plate. Perhaps best to create a new issue?

Indeed I've create a new one: supabase/auth-js#912

[felixgabler]

If we already get the user info for the server in the plugin, does it make sense to also do the same for the client plugin? This would clean up this composable a bit and we also have the onAuthStateChange listener there already, so it would be a fitting place.

Cannot really think of reasons why not to do it. I am not sure if there is value in refreshing the userState each time useSupabaseUser is called. It might even be bad for performance.

[larbish]

From my point of view, onAuthStateChange is only triggered when Supabase API is called to refresh the session either when getSession or refreshSession are called or when user is logged out. But on client side, when navigating, we want to ensure that the session (and the user) are still connected (token not expired) that's why calling getSession ensures this.

Important to note that getSession is not calling Supabase API when token are not expired (most of the time) but only reading local storage, this is not a lack of performance thus.

However, if you want to make some tests and prove that onAuthStateChange is triggered when token is expired and not refresh, we could remove it.

[felixgabler]

From what I can tell, the onAuthStateChange is called on events that would cover token expiration because SIGNED_OUT is also called when the token expires. Reading the docs, it definitely looks to me like one can rely on these auth events to track sessions. We also might not even need to call a getSession to initialize since the INITIAL_SESSION auth event triggers after client creation.

I can do more testing later today if you'd like

[larbish]

In this case we can indeed remove the async call to useSupabaseSession and get useSupabaseUser back to a synchronous composable. Which is a good thing meaning no breaking changes!

You can update the code then we'll both update our personal projects to ensure it works as expected.

[larbish]

useSupabaseUser will be back to the old version only returning the state. State being muted by either the server plugin or the onAuthStateChange. It makes sense to me. useSupabaseSession stays as a standalone composable.

[felixgabler]

You are right that we need to make sure the session is 100% valid. I've gone over the code again and here are some thoughts:

1. We want the behavior to be the same regardless of whether it is called on the client or the server. I am wondering whether there could be a case where the onAuthStateChange fires after the useSupabaseUser is already accessed somewhere? Maybe we should initialize it with data from the session too here?
2. I dislike that session and user are not handled uniformly. Right now, on the server, the user is read via getUser() in the server plugin but the session is not initialized there. On the client, both are managed through the onAuthStateChange in the client plugin.

To me, the cleanest and probably safest approach would be to make useSupabaseSession also only hold state that is managed by the plugin. I would then move the session initialization logic into supabase.server.ts. Additionally, I think we might want to also run the same logic in supabase.client.ts and initialize currentSession and currentUser with it to be safe. This way, we always ensure that the session and user are read on startup. Does that sound reasonable to you?

The only drawback I see is that the session and the user are not updated on the server during SSR. This, however, should not really happen very often because SSR is so fast and the odds of the token expiring during render are almost non-existent in my opinion. As long as we refresh the session and user in our Nitro server routes (which we are doing in serverSupabaseSession and serverSupabaseUser, I think we are fine with the approach defined above. Otherwise we could also use onAuthStateChange in the server plugin..

[felixgabler]

I made the changes on a separate branch (felixgabler@0895ccc). Will test it out in my local project because I prefer the general approach and cleaner code :)

[larbish]

@felixgabler If you confirm everything works smoothly with your project, let's push your changes in this PR. It looks all good to me! I'll test it on mine once you push your changes.

[felixgabler]

We've been running it for the last few days without issues related to the changes. We do get the supabase useSession warnings which are annoying but not a problem. Looking forward to hearing how it works on your project :)

EDIT: One issue we did encounter is a header overflow:

 ERROR  [h3] [unhandled] Parse Error: Header overflow                                                                                               9:40:44 AM

  at Socket.socketOnData (node:_http_client:540:22)
  at Socket.emit (node:events:519:28)
  at addChunk (node:internal/streams/readable:559:12)
  at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
  at Readable.push (node:internal/streams/readable:390:5)
  at Pipe.onStreamRead (node:internal/stream_base_commons:190:23)

This is most likely due to large auth and refresh tokens in the Supabase headers.
I've since increased the max header size (NODE_OPTIONS=--max-http-header-size=32384) and hopefully it is resolved now. Did you encounter this too? If so, we might have to add something to the docs about increasing the header size.

[larbish]

I've pushed the new version on my side project. I'll tell you if everything's ok :)

This warning is really annoying, they do not seem to be in a hurry with it...