supabase / auth-js

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

getAuthenticatorAssuranceLevel() triggers "getSession() could be insecure" warnings #910

Open sleepdotexe opened 6 months ago

sleepdotexe commented 6 months ago

Bug report

Describe the bug

Despite not using getSession() anywhere in my code, I am still receiving (numerous) console warnings stating the following:

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.

After doing some investigation, this issue seems to only occur when checking the user's MFA status via supabase.auth.mfa.getAuthenticatorAssuranceLevel(). If I remove or replace this call with a static return of 'aal2' values, the warnings disappear in console.

I am using Next.js and server-side rendering with the App router + supabase/ssr.

To Reproduce

  1. Follow the guide to set up supabase in a Next.js project as per the docs here.
  2. Add a function that performs server-side MFA validation as per the docs here.
    • This function should be run on the server (eg. Server component) and call supabase.auth.mfa.getAuthenticatorAssuranceLevel().
    • In my implementation, the currentLevel property of this call is compared against the defined/expected AAL level for that route, then - if they don't match the expected level` redirects them to the appropriate page (login page, enter MFA code page, dashboard page)
  3. Call/await this function in a page or layout.
  4. Run the app and then visit a page where this function is called.

Expected behavior

Since I have not called/used getSession() anywhere, I should not be seeing warnings in the console about getSession().

System information

Additional context

I did some investigation and it seems like getAuthenticatorAssuranceLevel() is referencing the session.user object in this line:

private async _getAuthenticatorAssuranceLevel(): Promise<AuthMFAGetAuthenticatorAssuranceLevelResponse> {
    return this._acquireLock(-1, async () => {
      return await this._useSession(async (result) => {
        // ...

        const verifiedFactors =
          session.user.factors?.filter((factor: Factor) => factor.status === 'verified') ?? []
               // ^----

Some suggestions/thoughts:

I haven't explored too deeply into the source code so hopefully these thoughts make sense.

kizivat commented 6 months ago

There is a PR for that #909

Collin0810 commented 5 months ago

Are there any updates on this? We are also using Next.js with the latest SSR package and implementing getAuthenticatorAssuranceLevel() in the middleware to check the users' AAL level. Our logs are flooded with warnings:

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

Which makes it really hard to debug and read console logs for anything (un)related to Supabase in our app.

j4w8n commented 4 months ago

No updates on this that I'm aware of. There are several places in the code where this happens, besides this method: some examples are updating a user, setting a session, and refreshing a session - if executed on the server side of course. I believe there might be a couple of others.

I'm hoping that after they get some more auth things completed in the next couple of months, that they decide this warning is unneeded and just remove it.

k-thornton commented 4 months ago

omg thank you for posting this. I've been chasing down my particular source of this incessant warning, and eventually traced it to this same getAuthenticatorAssuranceLevel() function.

I suppose unless the maintainers switch this function to use getUser() the correct course of action would be to only use getAuthenticatorAssuranceLevel() on the client side... has anyone gotten this working on their end before I go down that route?

If it's the case that getAuthenticatorAssuranceLevel() Is only intended to be called on the client side, it would be nice for that to be added to the docs, and have a more explicit warning emitted

k-thornton commented 4 months ago

Ok I reimplemented my code to do this check on the client side and the warnings went away. In my case I have a layout file covering an entire Dashboard section of my website, and inside that layout I have a useEffect that checks assurance level and either shows the MFA screen, or the dashboard as appropriate:

export function DashboardWithMFA({ user, userDetails, entitlements, children }: DashboardProps) {
    const [readyToShow, setReadyToShow] = useState(false)
    const [showMFAScreen, setShowMFAScreen] = useState(false)
    const supabase = createClient()

    useEffect(() => {
        ; (async () => {
            try {
                const { data, error } = await supabase.auth.mfa.getAuthenticatorAssuranceLevel()
                if (error) {
                    throw error
                }

                if (data.nextLevel === 'aal2' && data.nextLevel !== data.currentLevel) {
                    setShowMFAScreen(true)
                }
            } finally {
                setReadyToShow(true)
            }
        })()
    }, [])

    return (
        <>
            {readyToShow && showMFAScreen ?
                (
                    <AuthMFA onSuccess={() => setShowMFAScreen(false)} />
                ) : (
                    <DashboardContainer user={user} userDetails={userDetails} entitlements={entitlements}>{children}</DashboardContainer>
                )
            }
        </>
    )
}

Hope this helps someone out there!

abhishekyelley commented 3 months ago

Hey @k-thornton, the warning was to remind us that the token was retrieved from a storage medium (cookies) which could be tampered by the user, right? Which means that there is no secure way of getting the user's aal with the getAuthenticatorAssuranceLevel() function?

antonsarg commented 3 months ago

I hope that the problem will be solved. I can't imagine that moving the query to the client is the best solution when middleware should be the cleanest solution

abhishekyelley commented 3 months ago

@antonsarg I just verify the jwt token with the jwt_secret and then decode the payload like suggested here

J0 commented 3 months ago

Hey folks,

Thanks for your patience with us. An update here - A fix is in the works. We'll soon move to an asymmetric model where getSession will be lower risk since we'll only be releasing the public key.

antonsarg commented 3 months ago

@J0 So what is the best way to go for now especially for production?

k-thornton commented 3 months ago

@J0 So what is the best way to go for now especially for production?

https://supabase.com/docs/guides/auth/auth-mfa#server-side-rendering Based on the docs I'm reasonably convinced the intended method is to do all of the Multi-Factor AAL checking on the client side, in "use client" code. No security warnings that way, and my code more or less matches the docs.

That feels like the way it was intended to be implemented.

groud commented 2 months ago

Hey, while waiting for a cleaner version of a fix for this, I wanted to share I how dealt with the issue. It was happening when using getAuthenticatorAssuranceLevel in a NextJS middleware. I am now using this, and it does not trigger and issue, despite supabase.auth.getSession() being used there :shrug: :

   const {
    data: { user },
  } = await supabase.auth.getUser()

  let isHighestAssuranceLevel = true
  if (user) {
    // Check if they have the highest authentication level.
    // Since getAuthenticatorAssuranceLevel triggers errors. We copy-pasted and
    // modified it's internal implementation here.
    // See https://github.com/supabase/auth-js/issues/910.
    const res = await supabase.auth.getSession()
    if (res.error || !res.data.session?.access_token) {
      throw new Error(`Error fetching session: ${res.error}`)
    }
    const payload = decodeJWTPayload(res.data.session?.access_token)

    let currentLevel: AuthenticatorAssuranceLevels | null = null
    if (payload.aal) {
      currentLevel = payload.aal
    }

    let nextLevel: AuthenticatorAssuranceLevels | null = currentLevel
    const verifiedFactors = user.factors?.filter((factor: Factor) => factor.status === 'verified') ?? []
    if (verifiedFactors.length > 0) {
      nextLevel = 'aal2'
    }

    isHighestAssuranceLevel = !(nextLevel === 'aal2' && nextLevel !== currentLevel)
  }

This chunk of code is inserted in the middleware's code, as shown in the step 4 of this documentation page : https://supabase.com/docs/guides/auth/server-side/nextjs?queryGroups=router&router=app (then I do some additional redirect according to the isHighestAssuranceLevel value, but this is out of context for this issue)