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

await session.save() fails to set cookie if cookies.set() is used after #684

Open
Emrin opened this issue Jan 6, 2024 · 10 comments
Open

Comments

@Emrin
Copy link

Emrin commented Jan 6, 2024

I tested this in middleware and any invocation of response.cookies.set() on a NextResponse after saving the session with await session.save() will not set the new session as a cookie.

@calebjoshuapaul
Copy link

Same here, cookie won't update on session.save() if another cookie has been set using document.setCookie()

@romeobravo
Copy link

romeobravo commented Mar 18, 2024

I can confirm this issue. We are chaining middleware functions together.

Simplified examples:

function setCookieMiddleware(req, res, ctx) {
  res.cookies.set('foo', 'bar', { httpOnly: false })
  return res
}

async function saveSessionMiddleware(req, res, ctx) {
  ctx.session.foo = 'bar'
  await ctx.session.save()

  return response
}


// DOES NOT WORK: session cookie is not set in response headers, only foo cookie is present
const middlewares = [
  saveSessionMiddleware,
  setCookieMiddleware,
]

// DOES WORK: both session cookie and foo cookie Set Cookie header present in response headers
const middlewares = [
  setCookieMiddleware,
  saveSessionMiddleware,
]

@vvo
Copy link
Owner

vvo commented Jun 14, 2024

Hey there, can someone create a very minimal example that I can run? It would be the best way to solve this issue, thanks!

@kevva
Copy link

kevva commented Jul 4, 2024

@vvo, I don't have an example, but I encountered a somewhat similar issue recently. In fact, if you update the session in the middleware and try to read it in a server component, it's not in sync and you have to refresh the page again for it to work. However, I solved it by doing this:

import { parseSetCookie } from 'next/dist/compiled/@edge-runtime/cookies'

const options = {
  cookieName: '_mySessionCookie',
}

await session.save()

// `parseSetCookie` only reads the first cookie so probably not 100% safe to use
const { name, value } = parseSetCookie(response.headers.get('set-cookie')) ?? {}

if (name === options.cookieName) {
  response.cookies.set(name, value)
}

return response

Now when updating the session in middleware, it's reflected in the server components as well. This might be Next.js doing some magic behind the scenes (looks like it) when response.cookies.set(…) is called because it doesn't feel like they're taking the set-cookie header into account fully. Even though it's a Next.js bug, this feels like something that we should handle in this module.

@vvo
Copy link
Owner

vvo commented Jul 11, 2024

@kevva it looks like you found the root cause, do you think there's a way to fix this in iron-session? If so send a PR, thanks a lot 🙏

@kevva
Copy link

kevva commented Jul 11, 2024

@vvo, will submit a PR tomorrow.

@llegoelkelo
Copy link

I'm getting this issue as well. @kevva I'll be on the lookout for your PR.

@xava-dev
Copy link

xava-dev commented Jul 23, 2024

The suggested solution from @kevva did not seem to work for us. The session cookie was still not set on the first load within the server component using cookies() from 'next/headers'.

However, we managed to read the session cookie on the first load by looking at the set-cookie header in the server component:

import { getIronSession, unsealData } from 'iron-session'
import { cookies, headers } from 'next/headers'

const sessionOptions = {
  cookieName: '_mySessionCookie',
}

async function getSession() {
  const sessionSetCookie = parseCookieHeader(headers().get('set-cookie'), sessionOptions.cookieName)

  if (!sessionSetCookie) return getIronSession<Session>(cookies(), sessionOptions)

  return unsealData<Session>(sessionSetCookie, sessionOptions)
}

Our custom parser which finds the last cookie in the set-cookie header with the latest (session) changes:

function parseCookieHeader(header: string | null, key: string) {
  const cookieHeader = header
    ?.split(',')
    .findLast(c => c.includes(key))
    ?.split(';')[0]
    .split('=')[1]

  return cookieHeader ? decodeURIComponent(cookieHeader) : null
}

@olafurns7
Copy link

Here's how supabase are doing some cookie setting voodoo in the suggested middleware function for supabase auth, take note on the cookie setting, that's probably to handle this exact problem being discussed here above

middleware.ts:

import { type NextRequest } from 'next/server'
import { updateSession } from '@/utils/supabase/middleware'

export async function middleware(request: NextRequest) {
  return await updateSession(request)
}

export const config = {
  matcher: [
    /*
     * Match all request paths except for the ones starting with:
     * - _next/static (static files)
     * - _next/image (image optimization files)
     * - favicon.ico (favicon file)
     * Feel free to modify this pattern to include more paths.
     */
    '/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).*)',
  ],
}

supabase/middleware:

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

export async function updateSession(request: NextRequest) {
  let supabaseResponse = NextResponse.next({
    request,
  })

  const supabase = createServerClient(
    process.env.NEXT_PUBLIC_SUPABASE_URL!,
    process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!,
    {
      cookies: {
        getAll() {
          return request.cookies.getAll()
        },
        setAll(cookiesToSet) {
          cookiesToSet.forEach(({ name, value, options }) => request.cookies.set(name, value))
          supabaseResponse = NextResponse.next({
            request,
          })
          cookiesToSet.forEach(({ name, value, options }) =>
            supabaseResponse.cookies.set(name, value, options)
          )
        },
      },
    }
  )

  // IMPORTANT: Avoid writing any logic between createServerClient and
  // supabase.auth.getUser(). A simple mistake could make it very hard to debug
  // issues with users being randomly logged out.

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

  if (
    !user &&
    !request.nextUrl.pathname.startsWith('/login') &&
    !request.nextUrl.pathname.startsWith('/auth')
  ) {
    // no user, potentially respond by redirecting the user to the login page
    const url = request.nextUrl.clone()
    url.pathname = '/login'
    return NextResponse.redirect(url)
  }

  // IMPORTANT: You *must* return the supabaseResponse object as it is. If you're
  // creating a new response object with NextResponse.next() make sure to:
  // 1. Pass the request in it, like so:
  //    const myNewResponse = NextResponse.next({ request })
  // 2. Copy over the cookies, like so:
  //    myNewResponse.cookies.setAll(supabaseResponse.cookies.getAll())
  // 3. Change the myNewResponse object to fit your needs, but avoid changing
  //    the cookies!
  // 4. Finally:
  //    return myNewResponse
  // If this is not done, you may be causing the browser and server to go out
  // of sync and terminate the user's session prematurely!

  return supabaseResponse
}

@HiroakiLion
Copy link

Having the same issue.
await session.save() seems to do nothing.
I can access the session object and show the content inside by console.log, but it just fails to set it in cookie. And also Set-Cookie is not in the headers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants