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

user object warning logged, even when not touching session.user #888

Open
2 tasks done
j4w8n opened this issue Apr 24, 2024 · 34 comments
Open
2 tasks done

user object warning logged, even when not touching session.user #888

j4w8n opened this issue Apr 24, 2024 · 34 comments
Labels
bug Something isn't working

Comments

@j4w8n
Copy link
Contributor

j4w8n commented Apr 24, 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

With [email protected] and the ssr auth helper package, after a user logs in, the below warning is logged to the server console five times, with a fairly minimal SvelteKit app. This happens despite the fact that none of my code is calling session.user, nor am I destructuring the user property from session, or destructuring with ...rest-type syntax.

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.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

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

Expected behavior

The warning should not log when dev code doesn't explicitly call session.user.

Root Cause

I figured out why the first three logs happen: the Supabase client's server-side storage, from +layout.ts, is returning stringified JSON. You can see this in the docs. JSON.stringify() causes each property of session to be touched during the stringify process, which inevitably calls session.user. So, each time _useSession() is called, whether implicitly through dev code like getSession() or explicitly with internal auth-js processes, the warning is being logged.

I can't figure out where the final two warning logs are coming from. Could be from the same client initialization as above, but perhaps whatever triggers it is getting run asynchronously, so it's logging later.

Even if this warning wasn't an issue in the context of ssr, it could still be problematic, as the auth-js client itself calls session.user when updating a user, setting a session, refreshing a session, and more - at least some of which could theoretically happen server-side.

System information

  • OS: Windows, with or without WSL.
  • Browser N/A
  • Version of supabase-js: 2.42.5
  • Version of ssr: 0.3.0, but it should happen on earlier versions too I'd think.
  • Version of Node.js: N/A

Additional context

#873
#873 (comment)
https://tc39.es/ecma262/multipage/structured-data.html#sec-json.stringify
https://tc39.es/ecma262/multipage/structured-data.html#sec-serializejsonproperty
https://github.com/supabase/auth-js/blob/master/src/GoTrueClient.ts#L1111-L1124

@j4w8n
Copy link
Contributor Author

j4w8n commented Apr 27, 2024

@j4w8n
Copy link
Contributor Author

j4w8n commented Apr 28, 2024

It's likely the other two warnings, that I couldn't figure out how they are triggering, are triggered when SvelteKit checks if passed values are POJOs, in certain situations. See #873 (comment)

@kangmingtay
Copy link
Member

hey @j4w8n, we made another attempt in this PR to further cut down on the repeated warning logs returned - basically everytime we detect that a new session is saved, we set a flag to suppress the warning internally

@j4w8n
Copy link
Contributor Author

j4w8n commented May 2, 2024

Thanks @kangmingtay. I suspect that PR will resolve issues for some people - specifically the ones exclusively experiencing this when calling things like updateUser(). However, this has basically no effect for SvelteKit users - at least not on initial login and hard refreshes - because of the nature of what I explained in the first paragraph of the Root Cause section.

As an aside, how do I test an RC? I added an override in my package.json, and after doing bun install it claimed it installed one thing. When I go to the auth-js package.json the version is 2.64.2-rc.1, but none of the code from the release is in there - I had to add it manually to test. I was looking in dist/main/GoTrueClient.js, but I even glanced at the .ts version in src and saw nothing. I had this experience with pnpm as well, in another demo app.

@j4w8n
Copy link
Contributor Author

j4w8n commented May 2, 2024

@kangmingtay looks like there is something wrong with the build process for RCs. If you checkout the code on npm, you'll see the PR code is not there.
https://www.npmjs.com/package/@supabase/auth-js/v/2.64.2-rc.1?activeTab=code

@kangmingtay
Copy link
Member

@j4w8n ah good point, not sure why the release workflow got skipped in the first attempt but i reran it and it's published to npm so you should be able to test it out

@kangmingtay
Copy link
Member

@j4w8n yeah your analysis is right, any method that implicitly accesses the user property in the session object will trigger the warning log.

i think we can implement your suggestion here so the warning is only logged once per proxy session, but will need some time to test this out since we're currently quite tight on bandwidth

@vehm
Copy link

vehm commented May 3, 2024

Confirming that we are continuing to receive the warning in SvelteKit after #895 as mentioned here.

Will follow.

@j4w8n
Copy link
Contributor Author

j4w8n commented May 3, 2024

Until this issue is resolved, and perhaps even after that, I've tweaked my demo app - v0.3.0 - to where I no longer receive any warnings.

My solution is done in a legitimate way, which checks all of the security boxes. The code in hooks.server.ts verifies the JWT and uses it's decoded data to craft a validated session. This session will pass any internal auth-js checks, as well as type checks.

https://github.com/j4w8n/sveltekit-supabase-ssr

@silentworks
Copy link
Contributor

I think the suggestion here from @j4w8n is great but I'd also like a way to suppress this warning completely in production build. I don't want this to be logging inside of my application terminal when its being run in production mode.

kangmingtay pushed a commit that referenced this issue May 9, 2024
## What kind of change does this PR introduce?

Bug fix.

## What is the current behavior?

A call to `getSession()`, when using server storage, logs a warning
every time `session.user` is accessed. This is causing a lot of people
to see multiple consecutive logs to the console - especially for
SvelteKit users.

## What is the new behavior?

Ensures that accessing `session.user`, from a `getSession()` call, only
logs the warning once per Proxy session instance. In other words, once
per server request.

## Additional context

#888
@itsmikesharescode
Copy link

any update?

scosman added a commit to CriticalMoments/CMSaasStarter that referenced this issue May 22, 2024
scosman added a commit to CriticalMoments/CMSaasStarter that referenced this issue May 22, 2024
* Sveltekit 2 migration!

Mostly just running `npx svelte-migrate@latest sveltekit-2` and fixing package conflicts.

Use @supabase/auth-helpers-sveltekit to ^0.11 to avoid supabase/auth-js#888
@Salman2301
Copy link

Still logging, try different way to suppress it too, seems nothing works vite, sveltekit onwarn .... Svelte should somehow allow us to suppress certain log. Just incase if any of the lib are abusing it.

@itsmikesharescode
Copy link

any update??

@CropWatchDevelopment
Copy link

CropWatchDevelopment commented Jun 11, 2024

Any Updates?
This is getting really annoying. I am not looking to supress the issue, I want a fix.
Is there any repo that shows an example of the CORRECT way to implement supabase auth in sveltekit?
It seems like new examples from Supabase are all for Nextjs

@Hugo-Dz
Copy link

Hugo-Dz commented Jun 11, 2024

For all folks using SvelteKit, here's a robust solution which both removes the annoying logs and avoid a network call to get user's data.

@jdgamble555
Copy link

I just followed the official SvelteKit example, but without the session object. This gets rid of the log. 99% of the time, you just need the user object if there is one.

J

@vehm
Copy link

vehm commented Jun 11, 2024

I just followed the official SvelteKit example...

I think part of the issue is that the docs have multiple examples with SvelteKit and they are not all updated to reflect the current "correct" methods - and, meaning no offense, comments like this just add to the confusion because it all depends on which example you followed.

...but without the session object. This gets rid of the log. 99% of the time, you just need the user object if there is one.

Can you elaborate on this?

@jdgamble555
Copy link

@vehm - I'm not sure what you mean. There is only one example page that I'm aware of:

https://supabase.com/docs/guides/getting-started/tutorials/with-sveltekit

But to be clear, here is my hooks code. You usually only need user anyway and not session.

  event.locals.safeGetSession = async () => {

    const {
      data: { session },
    } = await event.locals.supabase.auth.getSession();

    if (!session) {
      return { user: null };
    }

    const {
      data: { user },
      error,
    } = await event.locals.supabase.auth.getUser();

    if (error) {
      // JWT validation has failed
      return { user: null };
    }

    return { user };
  };

That being said there is still the stupid log even when using session correctly, and needs to get fixed for people who do use session variable.

J

@vehm
Copy link

vehm commented Jun 12, 2024

I'm not sure what you mean. There is only one example page that I'm aware of

Here are the three I was referring to (including the one you linked to):

Essentially, there are a few guides that all approach things a little differently and it's difficult to know which of these is the "correct" or "up-to-date" method.

@jdgamble555
Copy link

They all have the same hooks file unless I'm missing something?

J

@vehm
Copy link

vehm commented Jun 12, 2024

They all have the same hooks file unless I'm missing something?

The first linked guide utilizes an authGuard + sequence and the app.d.ts is also different, but yes there are currently only minor differences between them. However, even if there are minor differences now, that is not true for prior versions nor for the future.

They were most inconsistent when they made the change from getSession to safeGetSession a while back. A couple of the guides were still using getSession even though safeGetSession was the "correct" method. This is the issue I'm referring to - inconsistent updates throughout the docs for what is "correct", and the resulting confusion/frustration that could occur when one might have followed an outdated guide.

@wiesson
Copy link

wiesson commented Jun 15, 2024

Which one should I use so that I don't see the error? I'm also using #888 (comment) approach and thought this is the way (checking the user, then the session)

After seeing https://supabase.com/docs/guides/getting-started/tutorials/with-sveltekit example, session and user is used reversed (calling session first, then obtain the user)

The error is so annoying 🫣

@scosman
Copy link

scosman commented Jun 15, 2024

Could we just get a "markUserAsVerifed" method as a interm solution if this fix is going to take much longer?

To current state of svelte+supabase is pretty frustrating. Non SSR auth has been deprecated, and SSR auth hits constant security warnings (even when doing things correctly).

@j4w8n
Copy link
Contributor Author

j4w8n commented Jun 15, 2024

@wiesson, there are no official examples which don't log the warning when using SvelteKit.

@scosman, yeah, bit of a mess right now, but I doubt they're going to introduce a new method before a fix happens. Maybe there will be one that comes along with the fixes?

I don't know the full roadmap of how they're addressing this, but the new ssr update is in RC right now; and asymmetric JWTs are coming, plus an augmented getUser() I believe.

However, in my opinion, more changes are needed to address the warning being logged when using things like updateUser() on the server side. I hope this part of the issue has not been overlooked. They'll either need to get rid of the warning entirely (if the above changes address the problem enough) or something else is going to have to change within code.

@igorlanko
Copy link

Is it okay to stay on 0.0.10 before the RC is shipped? I updated to 0.3.0, found this issue, and decided to rollback for now.

@j4w8n
Copy link
Contributor Author

j4w8n commented Jun 16, 2024

Is it okay to stay on 0.0.10 before the RC is shipped? I updated to 0.3.0, found this issue, and decided to rollback for now.

Since it's more of a logging nuisance than an actual issue, I'd consider staying on 0.3.0. But stick with whatever is working for you.

@jstjoe
Copy link

jstjoe commented Jun 19, 2024

@vehm - I'm not sure what you mean. There is only one example page that I'm aware of:

https://supabase.com/docs/guides/getting-started/tutorials/with-sveltekit

But to be clear, here is my hooks code. You usually only need user anyway and not session.

  event.locals.safeGetSession = async () => {

    const {
      data: { session },
    } = await event.locals.supabase.auth.getSession();

    if (!session) {
      return { user: null };
    }

    const {
      data: { user },
      error,
    } = await event.locals.supabase.auth.getUser();

    if (error) {
      // JWT validation has failed
      return { user: null };
    }

    return { user };
  };

That being said there is still the stupid log even when using session correctly, and needs to get fixed for people who do use session variable.

J

I'm in the same boat. I implemented safeGetSession() as per the documentation but I'm getting overrun with these logs. I hope the RC people are talking about will resolve that?

@thedelanyo
Copy link

Still logging, try different way to suppress it too, seems nothing works vite, sveltekit onwarn .... Svelte should somehow allow us to suppress certain log. Just incase if any of the lib are abusing it.

This is really a nightmare. I cannot even console.log on server to quickly look up a behavior.

@thedelanyo
Copy link

@wiesson, there are no official examples which don't log the warning when using SvelteKit.

@scosman, yeah, bit of a mess right now, but I doubt they're going to introduce a new method before a fix happens. Maybe there will be one that comes along with the fixes?

I don't know the full roadmap of how they're addressing this, but the new ssr update is in RC right now; and asymmetric JWTs are coming, plus an augmented getUser() I believe.

However, in my opinion, more changes are needed to address the warning being logged when using things like updateUser() on the server side. I hope this part of the issue has not been overlooked. They'll either need to get rid of the warning entirely (if the above changes address the problem enough) or something else is going to have to change within code.

The warning is unnecessary imho, could be a warning in the docs, alongside the guides or a comment alongside the code.

The warning makes server log debugging nightmare. I can't figure out what to read anymore. I presume it even makes the server slower over time as well.

@itsmikesharescode
Copy link

is supabase dont give a sht with these?

@itsmikesharescode
Copy link

this spam msg hit my productivity already

@victorforissier
Copy link

victorforissier commented Jun 24, 2024

For whomever that can help - while they fix this I setup a vite vite plugin that just suppresses this message.

In vite.config.ts

import { sveltekit } from '@sveltejs/kit/vite';
import { defineConfig } from 'vitest/config';

// myErrorFilterPlugin.ts
import type { Plugin } from 'vite';

export function myErrorFilterPlugin(): Plugin {
	return {
		name: 'error-filter-plugin',
		configureServer(server) {
			const filterMessage =
				'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.';

			// Intercept console.warn
			const originalWarn = console.warn;
			console.warn = (...args: any[]) => {
				if (args[0] && typeof args[0] === 'string' && args[0].includes(filterMessage)) {
					return;
				}
				originalWarn(...args);
			};

			// Intercept console.log
			const originalLog = console.log;
			console.log = (...args: any[]) => {
				if (args[0] && typeof args[0] === 'string' && args[0].includes(filterMessage)) {
					return;
				}
				originalLog(...args);
			};
		}
	};
}

export default defineConfig({
	plugins: [sveltekit(), myErrorFilterPlugin()],
	test: {
		include: ['src/**/*.{test,spec}.{js,ts}']
	}
});

craigkai added a commit to craigkai/volleyhub that referenced this issue Jun 24, 2024
@jacksteamdev
Copy link

jacksteamdev commented Jun 25, 2024

We can use #895 to suppress the warning by setting the protected property suppressGetSessionWarning on the AuthClient instance.

// @ts-expect-error - suppressGetSessionWarning is not part of the official API
event.locals.supabase.auth.suppressGetSessionWarning = true;

Here's my hooks.server.ts:

import { PUBLIC_SUPABASE_URL, PUBLIC_SUPABASE_ANON_KEY } from '$env/static/public';
import { createServerClient } from '@supabase/ssr';
import type { Handle } from '@sveltejs/kit';

export const handle: Handle = async ({ event, resolve }) => {
  event.locals.supabase = createServerClient(PUBLIC_SUPABASE_URL, PUBLIC_SUPABASE_ANON_KEY, {
    cookies: {
      get: (key) => event.cookies.get(key),
      /**
       * Note: You have to add the `path` variable to the
       * set and remove method due to sveltekit's cookie API
       * requiring this to be set, setting the path to `/`
       * will replicate previous/standard behaviour (https://kit.svelte.dev/docs/types#public-types-cookies)
       */
      set: (key, value, options) => {
        event.cookies.set(key, value, { ...options, path: '/' });
      },
      remove: (key, options) => {
        event.cookies.delete(key, { ...options, path: '/' });
      },
    },
  });

  if ('suppressGetSessionWarning' in event.locals.supabase.auth) {
    // @ts-expect-error - suppressGetSessionWarning is not part of the official API
    event.locals.supabase.auth.suppressGetSessionWarning = true;
  } else {
    console.warn(
      'SupabaseAuthClient#suppressGetSessionWarning was removed. See https://github.com/supabase/auth-js/issues/888.',
    );
  }

  /**
   * Unlike `supabase.auth.getSession()`, which returns the session _without_
   * validating the JWT, this function also calls `getUser()` to validate the
   * JWT before returning the session.
   */
  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 };
    }

    return { session, user };
  };

  return resolve(event, {
    filterSerializedResponseHeaders(name) {
      return name === 'content-range' || name === 'x-supabase-api-version';
    },
  });
};

@kvetoslavnovak
Copy link

kvetoslavnovak commented Jul 6, 2024

In SvelteKit I am using this setup and it seems to work ok without any warning logs:

// hooks.server.js
import { PUBLIC_SUPABASE_URL, PUBLIC_SUPABASE_ANON_KEY } from '$env/static/public'
import { createServerClient } from '@supabase/ssr'

export const handle = async ({ event, resolve }) => {
  event.locals.supabaseServerClient = createServerClient(PUBLIC_SUPABASE_URL, PUBLIC_SUPABASE_ANON_KEY, {
      cookies: {
          getAll() {
              return event.cookies.getAll()
          },
          setAll(cookiesToSet) {
              cookiesToSet.forEach(({ name, value, options }) =>
                  event.cookies.set(name, value, { ...options,path: '/' })
              )
          },
      },
  })


  const getSessionAndUser = async () => {
      const { data: { session } } = await event.locals.supabaseServerClient.auth.getSession()
      if (!session) {
          return {
              session: null,
              user: null
          }
      }

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

      delete session.user
      const sessionWithUserFromUser = { ...session, user: {...user} }

      return { session: sessionWithUserFromUser, user }
  }

  const { session, user } = await getSessionAndUser()

  event.locals.session = session
  event.locals.user = user

  return resolve(event, {
      filterSerializedResponseHeaders(name) {
          return name === 'content-range' || name === 'x-supabase-api-version'
      },
  })
}
// +layout.server.js
export const load = async (event) => {
// // clearing the cookie from a browser if the user logs out
//   if (event.locals.session == null) {
//   event.cookies.delete(event.locals.supabaseServerClient.storageKey, { path: '/' });
// }

  return {
      session: event.locals.session,
      user: event.locals.user
  };
};
// +layout.js
import { PUBLIC_SUPABASE_ANON_KEY, PUBLIC_SUPABASE_URL } from '$env/static/public'
import { createBrowserClient, createServerClient, isBrowser } from '@supabase/ssr'

export const load = async ({ fetch, data, depends }) => {
  depends('supabase:auth')

  const supabase = isBrowser()
    ? createBrowserClient(PUBLIC_SUPABASE_URL, PUBLIC_SUPABASE_ANON_KEY, {
        global: {
          fetch,
        },
      })
    : createServerClient(PUBLIC_SUPABASE_URL, PUBLIC_SUPABASE_ANON_KEY, {
        global: {
          fetch,
        },
        cookies: {
          getAll() {
            return data.cookies
          },
        },
      })

      const session = isBrowser()
      ? (await supabase.auth.getSession()).data.session 
      : data.session

  return {
    supabase,
    session,
    user: data.user
  }
}

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