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

Can't get rid of getUser() warning #873

Open
1 of 2 tasks
jdgamble555 opened this issue Mar 31, 2024 · 118 comments
Open
1 of 2 tasks

Can't get rid of getUser() warning #873

jdgamble555 opened this issue Mar 31, 2024 · 118 comments
Labels
bug Something isn't working

Comments

@jdgamble555
Copy link

jdgamble555 commented Mar 31, 2024

Bug report

I keep getting the getSession() error about 500 times on one page.

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.

  • 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

This warning should not exist. I do not want to contact supabase on my non-logged in pages to see if a user is logged in. That would be two round trips, which would slow down my app. There is nothing wrong with checking for a session cookie on my server without doing an extra request. Either way, I am following the tutorials exactly.

To Reproduce

Follow any one of the tutorials:

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

Expected behavior

Do not show the warning at all, or allow me to disable warnings. At the very least, do not show it 500 times on one page.

Screenshots

If applicable, add screenshots to help explain your problem.

System information

  • supabase - 1.151.1
  • @supabase/ssr - 0.1.0
  • @supabase/supabase-js - 2.41.1

Additional context

It is an issue with this line of code

Please see the linked repository: supabase/auth-helpers#755

J

@jdgamble555 jdgamble555 added the bug Something isn't working label Mar 31, 2024
@dHofmeister
Copy link

dHofmeister commented Apr 1, 2024

I have the same issue after running a rm -rf node_modules and reinstalling for sveltekit SSR. (Could also be reinstall independent. I immediately went for a reinstall this morning so i don't know if these logs were printed before reinstall).

I've followed the SSR guide exactly and even on the home page load this error shows up on first load.

  VITE v5.2.7  ready in 499 ms

  ➜  Local:   http://localhost:5173/
  ➜  Network: http://192.168.50.35:5173/
  ➜  Network: http://10.0.0.1:5173/
  ➜  press h + enter to show help
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().
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.
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.
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.
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.
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.

This is on home page load, without any user being authenticated.

@BeatSagda
Copy link

same issue here

@jkehler
Copy link

jkehler commented Apr 1, 2024

Also encountering this. I just want to have a simple and fast route guard and only want to call getUser when necessary. Calling getUser is currently adding around 1800-2000ms latency to all of my routes.

@chapo2501
Copy link

Wondering if this may be the reason I'm having very high number of auth API calls

@aajaces
Copy link

aajaces commented Apr 2, 2024

Same issue

@aaron-glade
Copy link

same issue, also encountering longer tail latency for getUser

@Nelhoa
Copy link

Nelhoa commented Apr 2, 2024

Hope this will be fixed soon

@maxmannen
Copy link

Yes, nice if this was fixed...

@Boehri
Copy link

Boehri commented Apr 3, 2024

Same issue here

2 similar comments
@ghidalg0
Copy link

ghidalg0 commented Apr 3, 2024

Same issue here

@ioancruso
Copy link

Same issue here

@hotpinkpoliticalmatrix
Copy link

Same issue.

@PaperPrototype
Copy link

OMG this is annoying

@cameronmurphy
Copy link

cameronmurphy commented Apr 4, 2024

I'm getting this issue even though I don't have a call to getSession() in my codebase lol

Something in SupabaseClient seems to call it, fetchWithAuth calling _getAccessToken potentially.

So even something as innocuous as this seems to throw it.

import StaffWithRoleType from '@/app/types/database/staff-with-role.type';
import { createClient } from '@/app/utils/supabase/server';

const fetchStaff = async () => {
    const supabase = createClient();
    
    const { data: staff, error } = await supabase
        .from('staff')
        .returns<StaffWithRoleType[]>();
 ...

@jdgamble555
Copy link
Author

if (!this.insecureGetSessionWarningShown) {

@rcmenezes
Copy link

same here, can we please get that fixed? 🗡️

@erskingardner
Copy link

Also seeing this.

@lucasjohnson
Copy link

Came here to say the same

@vehm
Copy link

vehm commented Apr 4, 2024

Following as well

@pmespresso
Copy link

Oh nice a fresh issue 👁️👄👁️
Same here, all I do is the basic @supabase/ssr setup, never call getSession in my code. Thank you.

Screenshot 2567-04-05 at 09 50 50

@j4w8n
Copy link
Contributor

j4w8n commented Apr 5, 2024

I found the source code that throws this warning, in the case where you don't explicitly call getSession() in your own code. It's in supabase-js, and calls getSession() when you use a supabase client on the server-side.

So, code like const { data, error } = await supabase.from('table').select('*') will throw the warning, because of https://github.com/supabase/supabase-js/blob/master/src/SupabaseClient.ts#L103, which executes _getAccessToken(), which calls getSession() https://github.com/supabase/supabase-js/blob/master/src/SupabaseClient.ts#L243L247

@jdgamble555
Copy link
Author

This is a mess, why would they put a crazy mandatory warning like this without running proper tests.

We need to be able to check for a session on our server without calling a function to supabase server. If there is a session, then we verify by doing the extra call to the supabase server. We should not be mandated to verify a session for non-logged in users.

J

@j4w8n
Copy link
Contributor

j4w8n commented Apr 5, 2024

This is a mess, why would they put a crazy mandatory warning like this without running proper tests.

We need to be able to check for a session on our server without calling a function to supabase server. If there is a session, then we verify by doing the extra call to the supabase server. We should not be mandated to verify a session for non-logged in users.

J

yeah, here are the two relevant PRs:
#846
supabase/auth-helpers#722

@davidglivar
Copy link

looks like the fix is not ready: #874

@j4w8n
Copy link
Contributor

j4w8n commented Apr 5, 2024

looks like the fix is not ready: #874

It looks like that fix does one thing: only turns off the warning if there's no error from the getUser() call, ie there's a user logged-in. This is because calling getUser() when no one is logged in will return an invalid claim: missing sub claim error, but still passes back result.data -> { user: null}. So they're making sure they don't turn off the warning unless there's a valid user logged in.

@j4w8n
Copy link
Contributor

j4w8n commented Apr 5, 2024

After pondering that PR a bit more, it's actually going to make things worse; even though it's the better code choice from a security perspective.
#874 (comment)

@Donald646
Copy link

I'm getting the same issue here update me when there is a fix

@LexacleTechnologies
Copy link

Same here, issue arises from gotrue client
This is so annoying, logs warnings even when I use getSession() in my middleware
Please fix the issue or at least make it non recursive. We can read warnings once not 1000+ times. I prefer my console tidy this is a mess
Screenshot from 2024-04-06 12-48-43

@ambethia
Copy link

ambethia commented Apr 7, 2024

👀

@handycode
Copy link

handycode commented Apr 23, 2024

Hi, I have a workaround

In app/layout.tsx:

const { data: { session } } = await supabase.auth.getSession()
- const user = session?.user
- // pass user into context provider
+ if (session) {
+  const { data: { user } } = await supabase.auth.getUser(session.access_token)
+  // pass user into context provider
+ }

Since the app/layout.tsx will only be rendered once, you don't need to worry about too many requests.

In middleware.ts: (If you use session as a validation check)

const { data: { session } } = await supabase.auth.getSession()
- if (!session?.user) 
+ if (!session)

Don't use getUser in middleware.

it works.

@Nelhoa
Copy link

Nelhoa commented Apr 23, 2024

I think I found a good workarround.

  • I can get my user with only one request (getSession)
  • I don’t have the warning anymore

I made an extended version of @j4w8n proposition

  • I create a SafeSession type in my app witch transforms property user to safeUser (getting the safeUser property instead of the user property make the warn stops)
  • I create a safeSessionSchema with Zod that gets every properties from session , except user, and takes properties from decoded access_token, got with jwt.decode(session.access_token) as decoded_session
  • the safeSessionSchema transforms all these properties into a satisfying SafeSession type
  • I tell my app.d.ts file that getSession don’t return a Session but a SafeSession
  • I put into event.locals my new getSession function. This function first checks that access_token is authentic with jwt.verify(). Then it decodes the access_token with jwt.decode() to get the properties included in it. Then I parse {session, decoded_session} with my safeSessionSchema

The SafeSession type

export type Prettify<T> = {
	[K in keyof T]: T[K];
} & {};

export type SafeSession = Prettify<Omit<Session, 'user'> & { safeUser: User }>;

The getSession function in hooks.server.ts

event.locals.getSession = async () => {
		const {
			data: { session }
		} = await event.locals.sb.auth.getSession();
		let newSession: SafeSession | undefined = undefined;
		try {
			if (session) {
				const decoded_session = jwt.verify(session.access_token, JWT_SECRET);
				newSession = safeSessionSchema.parse({ session, decoded_session });
			}
		} catch (err) {
			console.error('getSessionError', err);
			return null;
		}

		return newSession as SafeSession;
	};

The safeSessionSchema

const safeSessionSchema = z
	.object({
		session: z.object({
			access_token: z.string(),
			token_type: z.string(),
			expires_in: z.number(),
			expires_at: z.number().optional(),
			refresh_token: z.string()
		}),
		decoded_session: z.object({
			aud: z.string(),
			exp: z.number(),
			iat: z.number(),
			iss: z.string(),
			sub: z.string(),
			email: z.string().optional(),
			phone: z.string().optional(),
			app_metadata: z.object({ provider: z.string(), providers: z.array(z.any()) }),
			user_metadata: z.record(z.any()),
			role: z.string().optional(),
			aal: z.string(),
			amr: z.array(z.any()),
			session_id: z.string(),
			is_anonymous: z.boolean().optional()
		})
	})
	.transform((original) => {
		const newSession: SafeSession = {
			access_token: original.session.access_token,
			token_type: original.session.token_type,
			expires_in: original.session.expires_in,
			expires_at: original.session.expires_at,
			refresh_token: original.session.refresh_token,
			safeUser: {
				id: original.decoded_session.sub,
				aud: original.decoded_session.aud,
				role: original.decoded_session.role,
				email: original.decoded_session.email,
				phone: original.decoded_session.phone,
				app_metadata: original.decoded_session.app_metadata,
				user_metadata: original.decoded_session.user_metadata,
				is_anonymous: original.decoded_session.is_anonymous,
				created_at: ''
			}
		};
		return newSession;
	});

@j4w8n
Copy link
Contributor

j4w8n commented Apr 23, 2024

As a slightly off-topic aside, I realized if you call jwt.verify() synchronously (without a callback), it'll throw an error if it can't validate the jwt. So the code can be trimmed down, if desired.

!!!!! Never use your JWT_SECRET on the client-side. !!!!!

As a reminder, this does not get rid of any warnings, but is a convenient way to ensure that session from getSession(), including session.user, is valid.

try {
+ jwt.verify(session.access_token, JWT_SECRET)
- jwt.verify(session.access_token, JWT_SECRET, (err) => {
-   if (err) throw new Error()
- }
} catch (err) {
  return null
}

@Nelhoa
Copy link

Nelhoa commented Apr 23, 2024

@j4w8n do you validate my previous solution is safe and clean ?
I’ve simplified the jwt.verify() call as you’ve suggested.

@j4w8n
Copy link
Contributor

j4w8n commented Apr 23, 2024

@j4w8n do you validate my previous solution is safe and clean ? I’ve simplified the jwt.verify() call as you’ve suggested.

The short answer is "yes", what you're doing seems to be a viable solution. As an fyi, calling jwt.verify() will return the decoded token, so you can just do const decoded_session = jwt.verify(session.access_token, JWT_SECRET) instead of your extra call to jwt.decode().

I had some time to think more about this whole situation. To explain, I might create a discussion post later today or tomorrow, but I don't have time right now; and I don't want to clutter this issue any more than I already have. If I do create a post, I'll share the link here.

@j4w8n
Copy link
Contributor

j4w8n commented Apr 24, 2024

I just posted a discussion about the security vulnerabilities this warning is eluding to, along with two solutions you already know about, and a wrong assumption I had regarding JWT verification.

https://github.com/orgs/supabase/discussions/23224

@andreicos1
Copy link

So the best solution to suppressing this warning, without having to make an extra network request, seems to be storing the JWT in some separate state and verifying it before each usage.

@jdgamble555
Copy link
Author

Isn't that what getSession() is supposed to do instead of us handling jwt's manually?

J

@Nelhoa
Copy link

Nelhoa commented Apr 24, 2024

Isn't that what getSession() is supposed to do instead of us handling jwt's manually?

@jdgamble555 If I got it well, getSession() is actually the function that gets the session from the cookies. It can’t verify the access_token because it's not acknowledged of the JWT_SECRET.

That's why I suggested this. So you get an authentic user object without any extra network call. Then rename it safeUser inside the returned object, so the warning stops.

@vickkhera
Copy link

Isn't that what getSession() is supposed to do instead of us handling jwt's manually?

If .getSession() parsed the JWT and set all the fields from it, we could validate the signature on the JWT at our leisure. The other way is to use that snippet above to parse the JWT bits an use that instead of the session returned from getSession. Ie, the getSession() is only safe for simplifying grabbing the JWT auth token.

If the supabase folk don't want to risk people leaking their key and not provide a method that checks the signature and sets all the session fields, then at least make a variant of .setSession() just take the JWT and set all the fields from it and not hit the DB with another call to .getUser(). I can then handle the signature checking myself.

@fvermaut
Copy link

fvermaut commented Apr 25, 2024

Just to understand: is Supabase working on a fix for this? I've been following this thread for a while now, and all I seem to see are workarounds to be applied on project side.
I have a quite simple Next.js app, that I did set up following exactly the Supabase docs, and nowhere in my code I am calling getSession() - still I get the warning. So I honestly don't see why I should apply a workaround. Or maybe I am missing something? Not my intention to be polemical, I just want to understand what's the Supabase recommended way forward for this.

@j4w8n
Copy link
Contributor

j4w8n commented Apr 25, 2024

Just to understand: is Supabase working on a fix for this? I've been following this thread for a while now, and all I seem to see are workarounds to be applied on project side. I have a quite simple Next.js app, that I did set up following exactly the Supabase docs, and nowhere in my code I am calling getSession() - still I get the warning. So I honestly don't see why I should apply a workaround. Or maybe I am missing something? Not my intention to be polemical, I just want to understand what's the Supabase recommended way forward for this.

@fvermaut can you clarify which error you're still getting?

If it's the "Using supabase.auth.getSession()..." warning, you should be able to update supabase-js to at least 2.42.5 and that will no longer log.

If it's the other warning: "Using the user object...", then there is no "fix" for that, and I suspect they're going to leave that one as-is for at least a littlel bit longer. However, there are issues with the implementation of this error: see #888

@LexacleTechnologies
Copy link

Just popping back in to see how things are going with the issue. Good news - the fix below did the trick for me:

npm i @supabase/ssr@latest @supabase/supabase-js@latest

@nCrafts
Copy link

nCrafts commented Apr 26, 2024

I am also seeing the "Using the user object" error when calling getAuthenticatorAssuranceLevel like here:

const assurance = await supabase.auth.mfa.getAuthenticatorAssuranceLevel();

I am guessing this function uses the user object behind the scenes. Any workaround for this one?

@itsmikesharescode
Copy link

Any update????

@fvermaut
Copy link

If it's the "Using supabase.auth.getSession()..." warning, you should be able to update supabase-js to at least 2.42.5 and that will no longer log.

If it's the other warning: "Using the user object...", then there is no "fix" for that, and I suspect they're going to leave that one as-is for at least a littlel bit longer. However, there are issues with the implementation of this error: see #888

Thanks @j4w8n, I confirm updating to supabase-js 2.42.7 removes the following warning:

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()

@PaperPrototype
Copy link

i did pnpm update @supabase/supabase-js and it seems to have fixed it

@azro1
Copy link

azro1 commented Apr 27, 2024

npm install @supabase/supabase-js@2.42.7

@itsmikesharescode
Copy link

I did installing and updating my ssr and client supabase i also did the @latest but still no luck. Any update about this bug?

@Jakeii
Copy link

Jakeii commented Apr 28, 2024

This is where the log is coming from for me in +layout.ts, in order to create the client on the server it needs either the session or the cookie.

There isn't a way to get cookies in +page.ts on the server, so the session is used by the docs in a JSON.stringify which reads session.user causing the message.

Also when passing session from +page.server.ts to +page.ts sveltekit iterates over every key to make sure it's a POJO, which also causes this message to display.

createServerClient<Database>(PUBLIC_SUPABASE_URL, PUBLIC_SUPABASE_ANON_KEY, {
  global: {
    fetch
  },
  cookies: {
    get() {
	 return JSON.stringify(session); // this causes the warning session.user
    }
  }
});

my workaround in hooks.server.ts

   */
  event.locals.safeGetSession = async () => {
    const {
      data: { session }
    } = await event.locals.supabase.auth.getSession();
    if (!session) {
      return { session: null, user: null };
    }

    const {
      data: { user },
      error
    } = await event.locals.supabase.auth.getUser();
    if (error) {
      // JWT validation has failed
      return { session: null, user: null };
    }

    delete session.user;

    return { session: Object.assign({}, session, { user }), user };
  };

@j4w8n
Copy link
Contributor

j4w8n commented Apr 28, 2024

Also when passing session from +page.server.ts to +page.ts sveltekit iterates over every key to make sure it's a POJO, which also causes this message to display.

@Jakeii I figured out the JSON.stringify part, but I did not know about the POJO. Thanks for the insight! Since that's likely the case with +layout.server.ts > +layout.ts as well, this explains the other two instances of the warning logs I was getting.

@PaperPrototype
Copy link

If anyone is using the supabase/ssr package... check out the new updated docs, they seem to have improved the code used in the documentation to fix this. https://supabase.com/docs/guides/auth/server-side/sveltekit

@frschi
Copy link

frschi commented Apr 29, 2024

@PaperPrototype awesome hint! the annoying logs are gone by following the new docs with supabase/ssr package 0.3.0

@j4w8n
Copy link
Contributor

j4w8n commented Apr 30, 2024

If anyone is using the supabase/ssr package... check out the new updated docs, they seem to have improved the code used in the documentation to fix this. https://supabase.com/docs/guides/auth/server-side/sveltekit

What improvement are we talking about? I followed that guide and it actually added a sixth log to my SvelteKit app - "Using the user object..."

@raptor008
Copy link

If anyone is using the supabase/ssr package... check out the new updated docs, they seem to have improved the code used in the documentation to fix this. https://supabase.com/docs/guides/auth/server-side/sveltekit

What improvement are we talking about? I followed that guide and it actually added a sixth log to my SvelteKit app - "Using the user object..."

Same here. Still getting the "user object" warning with this code only implementing the hooks.server.ts file.

@maxmillionbeatz
Copy link

same here it's taking a toll on the development experience.

@eranmiller
Copy link

eranmiller commented Apr 30, 2024

If anyone is using the supabase/ssr package... check out the new updated docs, they seem to have improved the code used in the documentation to fix this. https://supabase.com/docs/guides/auth/server-side/sveltekit

What improvement are we talking about? I followed that guide and it actually added a sixth log to my SvelteKit app - "Using the user object..."

FWIW I made these changes to the provided AuthGuard function

const authGuard = async ({ event, resolve }) => {
  const { session, user } = await event.locals.safeGetSession() 
  // event.locals.session = session  // when commented it out, this silences the warnings 
  // this isn't a problem, as I currently haven't need for the session data in my code, YET.
  event.locals.user = user  // I pass this value, and it does not throw errors
  return resolve(event)
}

The updated guide is incomplete.
Is authGuard only of use for page specific authorization? Can it be used to validate user data on +page.server.js load or form action?
The guide describes how to perform secured superbase database calls at the page level, but not the preferred method for accessing the current session data without triggering these warnings.

Not sure this is recommended, I'm passing locals in a +page.server.js

export async function load({ locals }) {
  if (fails a locals.user data check) {
    redirect(303, '/auth/login')
  }
  ...
}

export const actions = {
  myaction: async ({ fetch, locals }) => {
     if (fails a locals.user data check) {
      redirect(303, '/auth/login')
    }
   ...
}

Similarly, passing locals for API endpoints in server.js

export async function POST({ request, locals }) {
  return db.dostuff(request, locals.user)
}

I joyfully welcome official Supabase boilerplate that define best practice for all common scenarios.

@j4w8n
Copy link
Contributor

j4w8n commented May 1, 2024

@eranmiller I'm not a raving fan of that SvelteKit guide. For your questions and concerns regarding it, I'd suggest opening a new issue or discussion on the supabase repo.

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