Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks done
j4w8n opened this issue Apr 10, 2024 · 28 comments
Closed
2 tasks done
Labels
bug Something isn't working

Comments

@j4w8n
Copy link
Contributor

j4w8n commented Apr 10, 2024

Bug report

  • I confirm this is a bug with Supabase, not with my own application.
  • 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.
supabase/auth-js#873
supabase/auth-helpers#755

@j4w8n j4w8n added the bug Something isn't working label Apr 10, 2024
@jdgamble555
Copy link

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
Copy link

How can we even disable this !

@j4w8n
Copy link
Contributor Author

j4w8n commented Apr 10, 2024

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.

@jdgamble555
Copy link

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
Copy link

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

@j4w8n
Copy link
Contributor Author

j4w8n commented Apr 10, 2024

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
Copy link

Any update?? :3

@Gbuomprisco
Copy link

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
Copy link

ElectricCodeGuy commented Apr 13, 2024

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
Copy link

any update 🥲 im using SvelteKit

@j4w8n
Copy link
Contributor Author

j4w8n commented Apr 15, 2024

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
Copy link

Hope, this gets fixed sooner. 😭

kangmingtay added a commit to supabase/auth-js that referenced this issue Apr 18, 2024
## What kind of change does this PR introduce?
* removes the warning from being logged everytime `getSession` is called
* only log the warning if the user property is being accessed from the
session
* addresses #873, supabase/supabase-js#1010
@kangmingtay
Copy link
Member

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 supabase/auth-js#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
Copy link
Contributor Author

j4w8n commented Apr 19, 2024

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.

@chbert
Copy link

chbert commented Apr 23, 2024

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

@j4w8n
Copy link
Contributor Author

j4w8n commented Apr 23, 2024

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

@eluchsinger
Copy link

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

@j4w8n
Copy link
Contributor Author

j4w8n commented Apr 24, 2024

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 - supabase/auth-js#888

@eluchsinger
Copy link

@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
Copy link

chbert commented Apr 27, 2024

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

@j4w8n
Copy link
Contributor Author

j4w8n commented Apr 27, 2024

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

@ixxie
Copy link

ixxie commented May 5, 2024

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

@hussam3h
Copy link

hussam3h commented May 7, 2024

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.

@j4w8n
Copy link
Contributor Author

j4w8n commented May 8, 2024

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.

@hussam3h
Copy link

hussam3h commented May 8, 2024

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

@j4w8n
Copy link
Contributor Author

j4w8n commented May 8, 2024

@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
Copy link
Contributor Author

j4w8n commented May 8, 2024

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 supabase/auth-js#873 and supabase/auth-js#888 may be relevant for you if seeing the "Using the user object as returned from supabase.auth.getSession()..." warning log.

@ODreelist
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests