supabase-js icon indicating copy to clipboard operation
supabase-js copied to clipboard

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

Open j4w8n opened this issue 10 months ago • 22 comments

Bug report

  • [x] I confirm this is a bug with Supabase, not with my own application.
  • [x] I confirm I have searched the Docs, GitHub Discussions, and Discord.

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 [email protected].

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

  • clone, install, create your .env with supabase url and key, npm run dev, visit localhost on port 5173

Expected behavior

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

System information

  • Version of ssr: [^0.3.0]
  • Version of supabase-js: [^2.42.0]

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

j4w8n avatar Apr 10 '24 11:04 j4w8n

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

jdgamble555 avatar Apr 10 '24 11:04 jdgamble555

How can we even disable this !

julien-blanchon avatar Apr 10 '24 13:04 julien-blanchon

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.

j4w8n avatar Apr 10 '24 15:04 j4w8n

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

jdgamble555 avatar Apr 10 '24 15:04 jdgamble555

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

julien-blanchon avatar Apr 10 '24 16:04 julien-blanchon

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.

j4w8n avatar Apr 10 '24 17:04 j4w8n

Any update?? :3

itsmikesharescode avatar Apr 11 '24 06:04 itsmikesharescode

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)

Gbuomprisco avatar Apr 13 '24 03:04 Gbuomprisco

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

ElectricCodeGuy avatar Apr 13 '24 20:04 ElectricCodeGuy

any update 🥲 im using SvelteKit

itsmikesharescode avatar Apr 14 '24 18:04 itsmikesharescode

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.

j4w8n avatar Apr 15 '24 01:04 j4w8n

Hope, this gets fixed sooner. 😭

itsmikesharescode avatar Apr 17 '24 19:04 itsmikesharescode

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.

kangmingtay avatar Apr 18 '24 17:04 kangmingtay

I can confirm that @supabase/[email protected] resolves this issue. Thanks!

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

j4w8n avatar Apr 19 '24 01:04 j4w8n

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

chbert avatar Apr 23 '24 11:04 chbert

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

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

j4w8n avatar Apr 23 '24 13:04 j4w8n

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

eluchsinger avatar Apr 24 '24 14:04 eluchsinger

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

j4w8n avatar Apr 24 '24 14:04 j4w8n

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

eluchsinger avatar Apr 24 '24 19:04 eluchsinger

@j4w8n I'm using "@supabase/supabase-js": "^2.42.5", which should have @supabase/[email protected] 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.

chbert avatar Apr 27 '24 19:04 chbert

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

j4w8n avatar Apr 27 '24 20:04 j4w8n

@kangmingtay the team's decision is understandable, but it would make sense if the tutorials like this one for SvelteKit show us a way to correctly manage server-side authentication without raising this warning.

ixxie avatar May 05 '24 09:05 ixxie

Joining this party, unfortunately :/ I am getting this warning when trying to upload a file. I am not calling getSession() anywhere in my Next.js app, and almost always, my auth calls are done server-side with getUser(). In this new situation, I am trying to upload a file via a server action, while the user is logged in, and supabase.storage.from('bucket').upload('path/to/file', file) is throwing this warning for me.

I am using supabase-js's latest v2.43.1 and supabase/ssr v0.3.0.

hussam3h avatar May 07 '24 23:05 hussam3h

Joining this party, unfortunately :/ I am getting this warning when trying to upload a file. I am not calling getSession() anywhere in my Next.js app, and almost always, my auth calls are done server-side with getUser(). In this new situation, I am trying to upload a file via a server action, while the user is logged in, and supabase.storage.from('bucket').upload('path/to/file', file) is throwing this warning for me.

I am using supabase-js's latest v2.43.1 and supabase/ssr v0.3.0

I wouldn't think you'd be getting this warning with the latest supabase-js - it's been removed entirely. Do you have any overrides set in your package.json?

There is still the warning when accessing session.user though, which, to my knowledge, does not happen with supabase.storage.

j4w8n avatar May 08 '24 00:05 j4w8n

@j4w8n You're right, I am sorry! I did find a side effect triggering a user update elsewhere:

await supabase.auth.updateUser({
    data: {
      first_name,
      last_name,
      full_name: `${first_name} ${last_name}`,
      avatar_url: avatarUrl ? avatarUrl : undefined,
    },
  });

and I do not have an override set.

hussam3h avatar May 08 '24 01:05 hussam3h

@hussam3h ok, cool. Yeah, updateUser is definitely an issue, but unrelated to this one. I mention it at the end of the Root Cause section on another issue.

j4w8n avatar May 08 '24 10:05 j4w8n

The fix for the "Using supabase.auth.getSession()..." warning log has been out for a few weeks now, so I'm going to close this issue.

For future visitors, issues https://github.com/supabase/auth-js/issues/873 and https://github.com/supabase/auth-js/issues/888 may be relevant for you if seeing the "Using the user object as returned from supabase.auth.getSession()..." warning log.

j4w8n avatar May 08 '24 10:05 j4w8n

this warning when trying to upload a file. I am not calling getSession() anywhere in my Next.js app, and almost always, my auth calls are done server-side with getUser(). In this new situation, I am trying to upload a file via a server action, while the user is logged in, and supabase.storage.from('bucket').upload('path/to/file', file) is throwing this warning for me.

I am using supabase-js's latest v2.43.1 and supabase/ssr v0.3.0.

Forgive me if I'm doing something wrong but I'm unable to suppress the warning in next (latest/app router):

export const getUserSessionInSSR = cache(async () => {
  const { auth } = serverClient()
  const { data, error } = await auth.getSession()
  if (error) {
    return null
  }
  return data?.session?.user ?? null
})

The warning is well understood, we only use this augment client side presentation elements as this call is made in the root layout, all actual api calls are verified server side by getUser() which is checking cookies so there are no security issues, I'd just like to suppress the warning so any help or tailored example would be appreciated. Thanks.

ODreelist avatar Jul 22 '24 17:07 ODreelist