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

Token refresh logs out the user in nextjs #691

Open
2 tasks done
imownbey opened this issue Nov 17, 2023 · 12 comments
Open
2 tasks done

Token refresh logs out the user in nextjs #691

imownbey opened this issue Nov 17, 2023 · 12 comments
Labels
bug Something isn't working

Comments

@imownbey
Copy link

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

Once a token needs to be refreshed in nextjs the client will start to throw Invalid Refresh Token errors and will log out the user.

To Reproduce

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

  1. git clone https://github.com/imownbey/supabase-auth-repro
  2. supabase start
  3. go to http://localhost:3000/
  4. register a user
  5. log in
  6. wait 60 seconds and refresh the page
  7. wait another 60 seconds and refresh the page again

You will now be logged out

Expected behavior

Tokens should refresh and not log a user out everytime

Screenshots

Sessions DB before initial refresh (after log in, I have logged in with 2 users is why there are two rows)
image
After first refresh:
image
Refreshing after that will not change the session table and user is logged out.

Debug log from refresh which logs the user out:
debugger_output.txt

System information

  • OS: [e.g. macOS, Windows]
  • Browser (if applies) [e.g. chrome, safari]
  • Version of supabase-js: latest
  • Version of Node.js: v20.5.1

Additional context

I am also seeing this in prod and local in my production app, it is a new behavior after switching to ssr from auth-helpers/next

@imownbey imownbey added the bug Something isn't working label Nov 17, 2023
@imownbey imownbey changed the title Token refresh is completely broken in nextjs Token refresh is logs out the user in nextjs Nov 17, 2023
@imownbey imownbey changed the title Token refresh is logs out the user in nextjs Token refresh logs out the user in nextjs Nov 17, 2023
@imownbey
Copy link
Author

imownbey commented Nov 17, 2023

To try to make it a bit easier to understand whats going on I changed the middleware to be:

  if (!request.nextUrl.pathname.startsWith("/_next") && !request.nextUrl.pathname.startsWith("/favicon")) {
    console.log({path: request.nextUrl.pathname})
    const { supabase, response } = createClient(request)

    // Refresh session if expired - required for Server Components
    // https://supabase.com/docs/guides/auth/auth-helpers/nextjs#managing-session-with-middleware
    await supabase.auth.getSession()

    return response
  }

so it doesnt try to refresh for every request just the main one.

Here are the logs from the first refresh (which generated 3 new refresh tokens in the DB):
first-refresh.txt
and the second refresh (where the user is logged out):
second-refresh.txt

It seems to refresh the token 3 times (even though we are only calling getSession once? Although that is maybe server components also?), and then on the second refresh still is trying to use the original token from before the second refresh again.

@imownbey
Copy link
Author

imownbey commented Nov 17, 2023

Ok I am 99% sure that the cookie set middleware client code just does not work.

@imownbey
Copy link
Author

Ok I figured out the bug, the middleware uses assigning as if javascript had pointers or something. Instead it needs to do something like this:

export const createClient = (request: NextRequest) => {
  // Create an unmodified response
  const response = {ref: NextResponse.next({
    request: {
      headers: request.headers,
    },
  })}

  const supabase = createServerClient(
    process.env.NEXT_PUBLIC_SUPABASE_URL!,
    process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!,
    {
      cookies: {
        get(name: string) {
          return request.cookies.get(name)?.value
        },
        set(name: string, value: string, options: CookieOptions) {
          console.log({name, setValue: decodeURI(value)})
          // If the cookie is updated, update the cookies for the request and response
          request.cookies.set({
            name,
            value,
            ...options,
          })
          response.ref = NextResponse.next({
            request: {
              headers: request.headers,
            },
          })
          response.ref.cookies.set({
            name,
            value,
            ...options,
          })
        },
        remove(name: string, options: CookieOptions) {
          // If the cookie is removed, update the cookies for the request and response
          request.cookies.set({
            name,
            value: '',
            ...options,
          })
          response.ref = NextResponse.next({
            request: {
              headers: request.headers,
            },
          })
          response.ref.cookies.set({
            name,
            value: '',
            ...options,
          })
        },
      },
      auth: {
        debug: true
      }
    }
  )

  return { supabase, response }
}

@eposha
Copy link

eposha commented Nov 18, 2023

Ok I am 99% sure that the cookie set middleware client code just does not work.

It's worked for me

@imownbey
Copy link
Author

Ah yeah sorry, I mean the middleware code described in the docs. Seems like you wrote your own

@toniengelhardt
Copy link

toniengelhardt commented Dec 13, 2023

@imownbey what exactly was the fix?

This is how my middleware.ts file looks like (same as yours, only without the .ref thingy bc that gives me errors) and I still get the error:

import { createServerClient, type CookieOptions } from '@supabase/ssr';
import { type NextRequest, NextResponse } from 'next/server';

export const createClient = (request: NextRequest) => {
	// Create an unmodified response
	let response = NextResponse.next({
		request: {
			headers: request.headers,
		},
	});

	const supabase = createServerClient(
		process.env.NEXT_PUBLIC_SUPABASE_URL!,
		process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!,
		{
			cookies: {
				get(name: string) {
					return request.cookies.get(name)?.value;
				},
				set(name: string, value: string, options: CookieOptions) {
					// If the cookie is updated, update the cookies for the request and response
					request.cookies.set({
						name,
						value,
						...options,
					});
					response = NextResponse.next({
						request: {
							headers: request.headers,
						},
					});
					response.cookies.set({
						name,
						value,
						...options,
					});
				},
				remove(name: string, options: CookieOptions) {
					// If the cookie is removed, update the cookies for the request and response
					request.cookies.set({
						name,
						value: '',
						...options,
					});
					response = NextResponse.next({
						request: {
							headers: request.headers,
						},
					});
					response.cookies.set({
						name,
						value: '',
						...options,
					});
				},
			},
			auth: {
				autoRefreshToken: true,
				// debug: true,
			},
		},
	);

	return { supabase, response };
};

@nmauersberg
Copy link

Thank you for investigating this.

I stumbled on your Issue, because I have a similar problem. I get logged out occasionally on using the supabase.auth.refreshSession() function. I use that, to detect if a user is deleted, on page load.

Using Next 14, with server components and custom middleware.

Not sure if it's related to your find though...

@hmnd
Copy link

hmnd commented May 3, 2024

I spent a few hours debugging this and tracked it down to having already been fixed in #760. What was happening (in my case at least) is the server-set cookie would be chunked into 3 cookies, whereas the browser-set cookie would only require 2 cookie chunks. The browser would only set those 2 chunks with the new cookie data, and then attempt to combine those chunks with the third one leftover from the server, resulting in an invalid session and the cookies being all cleared.

Unfortunately, it seems to be taking a little longer than usual for Supabase to release an update here, and this is causing a major headache for us.

@hf
Copy link
Contributor

hf commented Jun 5, 2024

We've identified this as a symptom of a few issues. This is what we're doing about it: https://github.com/orgs/supabase/discussions/27037

Linking the PR for reference: supabase/ssr#1

@hf
Copy link
Contributor

hf commented Jun 5, 2024

Unfortunately, it seems to be taking a little longer than usual for Supabase to release an update here, and this is causing a major headache for us.

I'm very sorry about taking so long, but we were basically 100% focused on this for about a month. We did not want to release any more partial fixes and then have to do another cycle of debugging to understand what's going on.

@hmnd
Copy link

hmnd commented Jun 5, 2024

@hf thank you for the clarification and I'm glad this is being worked on! The main thing causing confusion and frustration from my end was the lack of communication on what was happening, so this public explanation is very welcome :)

@firstian
Copy link

I ran into a variant of this bug, but not sure if it's the same. However, I'm pretty sure the middleware code in the doc for set() doesn't work when the cookie is chunked. The middleware code in question from the doc is:

      set(name: string, value: string, options: CookieOptions) {
          request.cookies.set({
            name,
            value,
            ...options,
          })
          response = NextResponse.next({
            request: {
              headers: request.headers,
            },
          })
          response.cookies.set({
            name,
            value,
            ...options,
          })
        },

The symptom I see is that my cookie is split into two chunks. The chunks are set individually in this callback. However, because the call to NextResponse.next() replaces response, the first chunk that was set is now lost. In the end, the only chunk that is sent to the client is the last chunk. The cookie is now broken, with a stale first chunk, which includes the old expires_at timestamp. In subsequent trips, the user is logged out because of that. When I commented out that NextResponse.next() line, it works correctly.

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

7 participants