supabase / auth-js

An isomorphic Javascript library for Supabase Auth.
MIT License
373 stars 165 forks source link

Still having getSession warning whenever _saveSession is called #912

Open larbish opened 6 months ago

larbish commented 6 months ago

Bug report

Describe the bug

Maintainer of the nuxt/supabase module here.

We have a PR to migrate on the @supabase/ssr package and we're still experiencing this issue with the latest released version including your PR.

The module is now using the @supabase/ssr package and we're still experiencing this issue with the latest released version.

I've removed all occurrences of getSession() in the module and I still have the warning.

Any help on this would be appreciate 🙏

To Reproduce

Clone the nuxt/supabase repository and follow the development readme to run the playground.

Notice the Using the user object as returned from supabase.auth.getSession() or from some supabase.auth.onAuthStateChange() events could be insecure warning.

Expected behavior

Do not display this warning.

j4w8n commented 6 months ago

Have you heard back about this?

regg00 commented 5 months ago

@larbish, you got any news about this issue?

larbish commented 5 months ago

No news about it so far... @thorwebdev could someone have a check at this please? I'd love to merge my PR in the module and use @supabase/ssr package 🙏

hf commented 5 months ago

We'll check this as soon as we can. Any external help in tracking down where the use comes from would help speed it up.

j4w8n commented 5 months ago

Looks like the code calls session.user a couple of times. And it json stringifies the session as well, which would also trigger the warning.

scottandrew commented 5 months ago

I am seeing this constantly right now ever since updating to SSR 0.4.0 and following the documentation. Going back to SSR 0.3.0 this seems to git rid of the messages.

chbert commented 4 months ago

Same here, running:

"@supabase/ssr": "^0.4.0",
"@supabase/supabase-js": "^2.44.3",

Getting:

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.

Please let me know how I can support to resolve this!

fnimick commented 4 months ago

If you are calling getUser on requests and want to silence the warning as your session is guaranteed to be safe, see my approach in https://github.com/fnimick/sveltekit-supabase-auth-starter - I replace the user proxy object that triggers the warning with the user instance returned from getUser. I'm not sure what the nuxt equivalent is, but look at hooks.server.ts for my solution.

jdgamble555 commented 4 months ago

I'm not calling or using session anywhere and it still gets called randomly.

A great example is to update a user or email:

const { error } = await supabase
    .auth
    .updateUser({
        email
    });

Nothing I do will get rid of the message in this case. Any fixes? I can call getUser() right before that line, still doesn't fix it.

J

j4w8n commented 4 months ago

I'm not calling or using session anywhere and it still gets called randomly.

A great example is to update a user or email:

const { error } = await supabase
    .auth
    .updateUser({
        email
    });

Nothing I do will get rid of the message in this case. Any fixes? I can call getUser() right before that line, still doesn't fix it.

J

Only suppressing the warning will stop that right now. I know there are some issues where people have posted how to do that. I have not implemented any.

jdgamble555 commented 4 months ago

I don't understand why this isn't fixed in the core code... this warning has created millions of wasted hours by supabase users.

J

j4w8n commented 4 months ago

I don't understand why this isn't fixed in the core code... this warning has created millions of wasted hours by supabase users.

J

I don't either. I created #888 for this, and they tried to cut down on the repetitiveness, but I'm not sure how much effort there has been to eliminate or deprecate the warning altogether.

fnimick commented 4 months ago

Until a fix is out, the only way to eliminate warnings is replace the .user property on the session with one returned from .getUser, as I do in my example.

    // workaround for user object use message
    const newSession: Omit<Session, "user"> & { user?: Session["user"] } = session;
    delete newSession.user;
    return { session: Object.assign({}, newSession, { user }), user };
jdgamble555 commented 4 months ago

@fnimick - I don't even use the session variable. I believe most people don't need it.

import {
    PUBLIC_SUPABASE_ANON_KEY,
    PUBLIC_SUPABASE_URL,
} from "$env/static/public";
import type { Database } from "$lib/database.types";
import { createServerClient } from "@supabase/ssr";
import type { Handle } from "@sveltejs/kit";

export const handle: Handle = async ({ event, resolve }) => {

    event.locals.supabase = createServerClient<Database>(
        PUBLIC_SUPABASE_URL,
        PUBLIC_SUPABASE_ANON_KEY,
        {
            cookies: {
                getAll: () => event.cookies.getAll(),
                setAll: (cookiesToSet) => {
                    cookiesToSet.forEach(({ name, value, options }) => {
                        event.cookies.set(name, value, {
                            ...options,
                            path: "/",
                        });
                    });
                },
            },
        },
    );

    event.locals.safeGetUser = 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) {
            return { user: null };
        }

        return { user };
    };

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

And it still creates the error message.

J

fnimick commented 4 months ago

Ah, I didn't realize that. If supabase functions fetch the session using _useSession and then use it, the error will appear and there's no way to avoid it. My solution fixes it only for uses of session.user on the server side when that is safe (the user is fetched separately).

kvetoslavnovak commented 4 months ago

Hi, I am using quite the same stragtegy in SvelteKit to repopulate the user in the session received via supabase.auth.getSession() wtih the user from supabase.auth.getUser().

But as mentioned above already this does not solve e.g. the case of updateUser(). I consider the silecing of the logs rather to be a hack., but this is the option as well at the moment

It is really a shame Supabase have not solved this issue for more the 3 or 4 month.

Anyways here is my hooks.server.js with user object substitution as well as silincing of the logs.

import { PUBLIC_SUPABASE_URL, PUBLIC_SUPABASE_ANON_KEY } from '$env/static/public'
import { createServerClient } from '@supabase/ssr'
import addUserprofileToUser from './utils/addUserprofileToUser'

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: '/' })
              )
          },
      },
  })

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

  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
      await addUserprofileToUser(session, event.locals.supabaseServerClient, 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'
      },
  })
}
joeldegerman99 commented 2 months ago

Ping! Still an issue, why isn't this fixed? Just remove the warning and add it as a warning in the docs.....

J0 commented 1 month ago

Thanks for raising the issue, the team is aware of this and actively working towards a fix.

The team hopes to release asymmetric keys soon which will allow for session verification client side without a round trip to Supabase Auth. This should hopefully significantly alleviate or fix the issue.

We are getting close to the date of release - please watch our changelog for further updates.

Let us know if there are further questions.

Thank you

curiosbasant commented 1 month ago

I'm doing this to suppress the annoying warnings.

async function getSupabaseAuthSession(supabase: SupabaseClient) {
  const [s, u] = await Promise.all([supabase.auth.getSession(), supabase.auth.getUser()])
  if (!s.data?.session || !u.data?.user) return null

  // @ts-expect-error
  delete s.data.session.user
  return { ...s.data.session, user: u.data.user } as Session
}