supabase / supabase-js

An isomorphic Javascript client for Supabase. Query your Supabase database, subscribe to realtime events, upload and download files, browse typescript examples, invoke postgres functions via rpc, invoke supabase edge functions, query pgvector.
https://supabase.com
MIT License
2.83k stars 219 forks source link

Code in supabase-js calls `getSession()`, which now throws a warning when using in combo with ssr #1010

Open j4w8n opened 3 weeks ago

j4w8n commented 3 weeks ago

Bug report

Describe the bug

When using @supabase/supabase-js and @supabase/ssr, a console warning is now thrown whenever you fetch from the DB on the server-side. e.g. await supabase.from('profiles').select('*'). Based on what I'm seeing in supabase-js code, this would throw during other invocations as well - like calling an edge function or storage. This landed with auth-js@2.63.0.

The warning is: "Using supabase.auth.getSession() is potentially insecure as it loads data directly from the storage medium (typically cookies) which may not be authentic. Prefer using supabase.auth.getUser() instead. To suppress this warning call supabase.auth.getUser() before you call supabase.auth.getSession()."

As you'll see from my repro code, I'm not calling getSession() at all. The underlying issue is that supabase-js code calls getSession(), and therefore the warning is outside of a dev's control.

In supabase-js, it's called here, which is invoked here.

Supabase uses getSession() to grab the user's access_token, in order to authenticate fetches. This is understandable, and I suspect this issue is an unintended side effect from the auth-js code.

To Reproduce

https://github.com/j4w8n/getsession-warning

Expected behavior

The console warning should not throw when the getSession() call is outside a dev's control.

System information

Additional context

There have been issues created on other repos. https://github.com/supabase/auth-js/issues/873 https://github.com/supabase/auth-helpers/issues/755

jdgamble555 commented 3 weeks ago

To reiterate, we should not be making an extra fetch to verify a session, unless a user is logged in. A non-logged in user should not have to call getUser() at all, as it is an unnecessary fetch that will slow down all apps.

J

julien-blanchon commented 3 weeks ago

How can we even disable this !

j4w8n commented 3 weeks ago

To reiterate, we should not be making an extra fetch to verify a session, unless a user is logged in. A non-logged in user should not have to call getUser() at all, as it is an unnecessary fetch that will slow down all apps.

J

https://github.com/supabase/auth-js/pull/876 should at least partially address your concern, once it lands. It won't fire the network request for getUser if there's no logged-in user. It does return an error in that case, so I'm not sure how that might affect apps, depending on if/how people handle an error for that call.

jdgamble555 commented 3 weeks ago

I don't want to

To reiterate, we should not be making an extra fetch to verify a session, unless a user is logged in. A non-logged in user should not have to call getUser() at all, as it is an unnecessary fetch that will slow down all apps. J

supabase/auth-js#876 should at least partially address your concern, once it lands. It won't fire the network request for getUser if there's no logged-in user. It does return an error in that case, so I'm not sure how that might affect apps, depending on if/how people handle an error for that call.

I don't want to call getUser() at all, so that is not really addressing anything. I should be able to call getSession() to see if there is a local session at all, and then all getUser() to verify it or revalidate it if and only if there is a session. So, that is unrelated IMO.

J

julien-blanchon commented 3 weeks ago

I'm very concerned with this design choose, I don't understand why you did that ...

j4w8n commented 3 weeks ago

I don't want to

To reiterate, we should not be making an extra fetch to verify a session, unless a user is logged in. A non-logged in user should not have to call getUser() at all, as it is an unnecessary fetch that will slow down all apps. J

supabase/auth-js#876 should at least partially address your concern, once it lands. It won't fire the network request for getUser if there's no logged-in user. It does return an error in that case, so I'm not sure how that might affect apps, depending on if/how people handle an error for that call.

I don't want to call getUser() at all, so that is not really addressing anything. I should be able to call getSession() to see if there is a local session at all, and then all getUser() to verify it or revalidate it if and only if there is a session. So, that is unrelated IMO.

J

I misunderstood then. I'm sorry.

itsmikesharescode commented 3 weeks ago

Any update?? :3

Gbuomprisco commented 3 weeks ago

I personally only use it on marketing pages to check if a user is logged in and display their profile avatar/name - and do so to avoid a request that would slow down important pages.

I'd personally either add a way to disable the log, or disable the log in production, or disable them altogether and simply mention this in the docs (which I can now see do so)

ElectricCodeGuy commented 2 weeks ago

Would it be possible to fix silly issue?? It have been reported one week ago and it seems like Supabase team are on vacation or something?

const originalWarn = console.warn.bind(console.warn); console.warn = (msg, ...params) => { if ( msg.includes('Using supabase.auth.getSession() is potentially insecure') ) { return; } originalWarn(msg, ...params); };

To suppress it

itsmikesharescode commented 2 weeks ago

any update 🥲 im using SvelteKit

j4w8n commented 2 weeks ago

any update 🥲 im using SvelteKit

No updates. I'm not sure how visible it is to the team at this point. I'd imagine they've been prepping for the "big announcement" next this week.

itsmikesharescode commented 2 weeks ago

Hope, this gets fixed sooner. 😭

kangmingtay commented 2 weeks ago

Hey everyone, apologies for not acting on this sooner - the team decided to add the warning log in getSession with good intention because we noticed that many folks were using it insecurely on the server-side to "verify" that a user is logged in. It was designed to be used safely on the client-side but with the move to react server components, this posed a problem when it's used on the server-side since the user details in the session can be faked by the client.

Our immediate priority was to ensure that no one uses it in an insecure fashion and we might have been over-zealous with logging it as a warning. Since this is security sensitive, we wanted to inform everyone in the quickest and most obvious manner. We have since made the fix https://github.com/supabase/auth-js/pull/879 to cut down on the noise, and only log the warning when the user object is accessed from the session returned in getSession .

We appreciate all of your feedback, and hopefully this makes this easier for you; however, if there are other concerns about this or the fix, please let us know.

j4w8n commented 2 weeks ago

I can confirm that @supabase/auth-js@2.63.1 resolves this issue. Thanks!

@kangmingtay, I can close this if you'd like, or we can leave it open for however long you want.

chbert commented 1 week ago

@j4w8n I'm using "@supabase/supabase-js": "^2.42.5", which should have @supabase/auth-js@2.63.1 afaik. But I still get the logs.

j4w8n commented 1 week ago

@j4w8n I'm using "@supabase/supabase-js": "^2.42.5", which should have @supabase/auth-js@2.63.1 afaik. But I still get the logs.

That should be the warning about the user object, correct? Not the getSession warning.

eluchsinger commented 1 week ago

I get this warning when calling the supabase.auth.updateUser function.

j4w8n commented 1 week ago

I get this warning when calling the supabase.auth.updateUser function.

Can you clarify the warning? If it's the "Using supabase.auth.getSession()..." warning, you should be able to update supabase-js and that will no longer log. If it's the other warning: "Using the user object...", then I briefly mentioned the updateUser() edge case on an issue I recently created - https://github.com/supabase/auth-js/issues/888

eluchsinger commented 1 week ago

@j4w8n you're right. It's the following error:

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.

chbert commented 6 days ago

@j4w8n I'm using "@supabase/supabase-js": "^2.42.5", which should have @supabase/auth-js@2.63.1 afaik. But I still get the logs.

That should be the warning about the user object, correct? Not the getSession warning.

@j4w8n Sorry, for my late reply! Yes, correct: 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.

j4w8n commented 6 days ago

Thanks @chbert. Then these two issues are going to be more relevant for you: https://github.com/supabase/auth-js/issues/873 https://github.com/supabase/auth-js/issues/888