vvo / iron-session

🛠 Secure, stateless, and cookie-based session library for JavaScript
https://get-iron-session.vercel.app
MIT License
3.62k stars 249 forks source link

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

Open Emrin opened 9 months ago

Emrin commented 9 months ago

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 commented 8 months ago

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

romeobravo commented 6 months ago

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 commented 4 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@vvo, will submit a PR tomorrow.

llegoelkelo commented 3 months ago

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

xava-dev commented 2 months ago

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 commented 1 month ago

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 commented 4 weeks ago

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.