feat: fix ssr cookie handling in all edge cases

[hf]

Fixes many issues with @supabase/ssr when handling edge-cases for reading and setting cookies especially in NextJS.

Some of these edge-cases are as follows:

There's no such thing as removing a cookie.

When cookies are "touched" by the Supabase client, they're either set to a non-empty value or they need to be set to an empty value. When set to an empty value, the max-age property must be set to 0 so the browser receiving the Set-Cookie header knows to delete it from its store.

For example, this delete method does not remove the cookie from the response but it just removes the Set-Cookie header that would have been sent. But that's not what's wanted -- the Supabase client wants to send a Set-Cookie header. It's therefore not very useful and should not be used.

Because storage items are often chunked into multiple cookies, existing cookie chunks must be managed.

There are 4 cases that need to be dealt with.

1. Non-chunked to chunked. (Currently broken.)
   Suppose the storage item with key xyz is below the cookie chunk limit, so it's encoded as one cookie under the name xyz. But in the session refresh step, the storage item has grown above the cookie chunk limit, so it now needs to be chunked into 2 cookies xyz.0 and xyz.1. In this case, the xyz cookie must be set to '' and max-age to 0 so it's removed from the browser cookie store.
2. Fewer chunks to more chunks. (Currently OK.)
   Suppose the storage item with key xyz is encoded as two cookie chunks xyz.0 and xyz.1. After the session refresh step, its size has grown and 3 cookie chunks are needed xyz.0, xyz.1 and xyz.2. Obviously the first two chunks need to be set to a value and in case they were set to max-age 0 now need to be set to the indefinite max-age.
3. More chunks to fewer chunks. (Currently broken.)
   Suppose the storage item with key xyz is encoded as three cookie chunks xyz.0 through .2. After the session refresh step, the size has shrunk down to only need xyz.0 and xyz.1. This means that chunk xyz.2 needs to be set to '' value and max-age of 0.
4. Chunked to non-chunked. (Currently broken.)
   Suppose the storage item with key xyz is encoded as two cookie chunks xyz.0 and xyz.1. After the session refresh step, its size has shrunk below the cookie size so it's now encoded as one cookie xyz. This means that xyz.0 and xyz.1 must be set to '' and max-age of 0.

Because (1), (3) and (4) are not handled properly at this time, they can leave chunk artifacts that float around on every request and can cause in the future problems such as:

• JSON.parse() failing, for example chunk xyz.1 ending like ..." and chunk xyz.2 starting like { (which would fail because a , is missing.
• Reading garbage, for example xyz.1 and xyz.2 align properly to form fully valid JSON, but with content that is unrecognizable.

Cookies don't always need to be set (server-side).

There are only 3 situations when cookies should be touched:

1. The session was actually refreshed, i.e. onAuthStateChange event with TOKEN_REFRESHED.
2. The user object was updated, i.e. onAuthStateChange event with USER_UPDATED.
   This is because the user object is encoded in the cookies, and the flow server -> browser is secure.
3. The user was signed out, most likely because the session refresh identified that the session has ended (user signed out from another device, the session reached its maximum lifetime or inactivity timeout). This is the SIGNED_OUT event.

If these events have not occurred on server side, Set-Cookie must not be present on the response. A missing Set-Cookie header tells the browser to obey the cookie store it already has.

Cookies should be set in one go instead of partially (server-side).

The NextResponse.next() API is truly awful in middleware.ts. It should only be called once after all session processing has been completed by the Supabase client.

This is because:

• The request headers also contain the cookies. To propagate the correct cookies down to the server-rendered pages and components, NextResponse.next({ request: { headers: request.headers } }) must be called, but it can only be called after the Supabase client has finished processing the session.
• The response headers need to be set on the response object, which from the previous case must be created after the Supabase client has finished processing the session.

So the following pattern, as seen in the Supabase guide for middleware.ts:

export async function middleware(request: NextRequest) {
  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) {
          request.cookies.set({
            name,
            value,
            ...options,
          })
          response = NextResponse.next({
            request: {
              headers: request.headers,
            },
          })
          response.cookies.set({
            name,
            value,
            ...options,
          })
        },

Is INCORRECT because a new response object is created on each cookie set/removal, meaning it will only contain always only one Set-Cookie header and not the multiple needed to encode chunked cookies or the PKCE state.

Finally, combining it all together

To solve these multiple issues, this PR modifies the cookies option to use a new pattern like so:

export async function middleware(request: NextRequest) {
  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: {
        getAll() {
          return request.cookies.getAll()
        },
        setAll(cookies: { name: string, value: string, options: CookieOptions }[]) {
          cookies.forEach(({ name, value, options }) => request.cookies.set({ name, value, ...options }))
 
          response = NextResponse.next({
            request: {
              headers: request.headers,
            },
          })

          cookies.forEach(({ name, value, options }) => response.cookies.set({ name, value, ...options }))
        },
      },
  )

  await supabase.auth.getUser()

  // either the session needed refreshing so `TOKEN_REFRESHED` was called and the `setAll` above was called, 
  // changing the response object; or it wasn't so the original response is being used

  return response

Cookies are only set once if the onAuthStateChange events TOKEN_REFRESHED, USER_UPDATE, SIGNED_OUT were emitted. If these events do not occur, cookies must not be touched, so the initial response will be used with the valid cookie chunks.

If cookies need to be set, all the cookie chunks are managed properly, so that there are no chunk artifacts floating around. This means the setAll call will include both setting '' on certain cookie names and setting actual values.

In the browser

The browser client will also use getAll and setAll similarly, but because this is the browser it's fine to use them directly with the document.cookie API -- meaning cookies will always be gotten and set -- not only on events.

This is still a WIP.

[ahosny752]

Does this fix address the issue of cookies intermittently not being set after a login -> sign out -> login on next js when using supabase-ssr?

[felixgabler]

It would be so awesome to get some of these issues resolved and I really hope your PR is the way to get there. What is still WIP about this, or rather, how can we help in getting this ready to merge? Thanks for your effort!

[hf]

This PR was one step towards what we describe here: https://github.com/orgs/supabase/discussions/27037

It's superseded by supabase/ssr#1