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

feat: auth-helpers-nextjs total fix with createMiddlewareClientV2 #769

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hf
Copy link
Contributor

@hf hf commented Apr 16, 2024

createMiddlewareClient was basically completely broken for correctly syncing the session state between the server and browser parts of a NextJS application.

It works in most situations where the user is actively interacting with the site, but as soon as they leave the site for an extended period of time and come back, it's very likely that after the JWT expiry time they will receive an Invalid Refresh Token error from the server. Because these events are not time-correlated, it's impossible to debug what is causing them.

Why does this happen:

  1. NextResponse.next() is not enough. Pages, server-rendered components and route handlers do not see the Set-Cookie header set on the response.
  2. Because this runs in the middleware.ts file, the session is correctly refreshed in the middleware client and the Set-Cookie headers are set on the response so the browser syncs up with the state of the middleware client.
  3. However, the page or component client rendered on the server still only sees the cookie header from the request which is the old session and any use of getSession() or getUser() will refresh the session every time. If you have, say, 3 Data API calls to PostgREST, it's very likely that the session would be refreshed 3 times.
  4. Because the page or component cannot set headers on the response, these refreshed sessions just remain unused, but the Auth server already tracks them as the "later" refresh tokens for the user session and is expecting (for security reasons) the browser to know about them too.
  5. One hour after this has happened, the Supabase client on browser or server will try to refresh the session from step 2 (coming from middleware.ts) and be greeted with the "Invalid Refresh Token" error.

How is this being fixed:

The design of createMiddlewareClient does not care at all about what happens to the request, but this is incredibly important so the new refreshed session is passed down to the page or server-rendered component. Furthermore, this cannot "just be set" as for the propagation to take place, NextResponse.next({ request }) needs to be called.

Therefore, createMiddlewareClient is fully deprecated and replaced with its new counterpart createMiddlewareClient2. This works a bit differently (without reinventing the simplistic beauty of the @supabase/ssr package):

export function middleware(request: NextRequest) {
  const [supabaseClient, nextResponse] = createMiddlewareClient2(request, { /* standard client options */ })

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

  if (!user) {
    // there's no user session, ask them to log in
    return NextResponse.redirect(new URL('/sign-in-page-in-your-app'))
  }

  return nextResponse()
}

Firstly it returns two arguments, the Supabase client and a nextResponse function. It should be called towards the bottom of the middleware.ts file, ideally in the return statement.

The client is setup such that, it initially reads from the request's Cookie header, but writes the changes to local memory. It tracks what items (not cookies!) are being set or deleted.

When you call nextResponse() the requests's Cookie header is reset to reflect the new state of the Supabase client. Because this header will just be propagated down to the page or server-rendered component, no cookie chunking is necessary.

Then, it calls:

NextResponse.next({
  request
})

Which ensures that the request's changes will be actually propagated down to the page or component. This enables the clients there to pick up the Cookie header which now includes the refreshed session.

Finally, it sets the Set-Cookie header on the response, which eventually reaches the browser and the session state is finally synchronized between them.

@hf hf requested a review from a team as a code owner April 16, 2024 12:19
Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pnpm build:nextjs fails - also, if createMiddlewareClient is broken, why not just modify the implementation to call createMiddlewareClient2 under the hood instead?

@hf
Copy link
Contributor Author

hf commented Apr 20, 2024

pnpm build:nextjs fails - also, if createMiddlewareClient is broken, why not just modify the implementation to call createMiddlewareClient2 under the hood instead?

As described in the PR -- to forward the headers NextResponse.next() should be called after the client has processed the session.

packages/nextjs/src/middlewareClient.ts Show resolved Hide resolved
packages/nextjs/src/middlewareClient.ts Outdated Show resolved Hide resolved
packages/nextjs/src/middlewareClient.ts Show resolved Hide resolved
packages/nextjs/src/middlewareClient.ts Show resolved Hide resolved
packages/nextjs/src/middlewareClient.ts Show resolved Hide resolved
@kangmingtay
Copy link
Member

@hf you need to export the new client in the index.ts file too

@kangmingtay
Copy link
Member

@hf the return types don't show up correctly when the client is instantiated, which isn't great DX-wise

const [supabaseClient, nextResponse] = createMiddlewareClient2(req);

it seems to think that createMiddlewareClient2 returns (SupabaseClient<any, "public", any> | (() => NextResponse<...>))[] (note the |)

@hf hf changed the title feat: auth-helpers-nextjs total fix with createMiddlewareClient2 feat: auth-helpers-nextjs total fix with createMiddlewareClientV2 May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants