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

Refreshing session can result in duplicate cookies (chunked and non chunked versions) #704

Open
astonfuture opened this issue Dec 13, 2023 · 8 comments
Labels
bug Something isn't working

Comments

@astonfuture
Copy link

astonfuture commented Dec 13, 2023

Describe the bug

In the supabase/ssr library, when an auth session gets refreshed, if the new auth cookie generated does not exceed the cookie chunking size, it will create a new, non-chunked cookie that does not overwrite the chunked cookie resulting in multiple auth cookies (some chunked auth cookies + a non-chunked auth cookie) OR VICE VERSA. This behaviour creates multiple cookies on the client which can cause issues in the following situation:

If there existed a non-chunked cookie and then the server returned a new chunked-cookie after refresh, the SupabaseAuthClient will always default to reading the non-chunked cookie which would be old/expired. It will also not delete the chunked cookies on signOut as it always defaults to reading and returning the non-chunked cookie first.

I've been through the GoTrue and createServerClient code and as far as I can see, there are no checks when a new auth cookie is created to make sure that it's not doing this doubling.

To Reproduce

Can be difficult to reproduce as it's an edge case when the cookie length is very close to the the cookie chunk size cutoff.

  1. Sign in a user using the supabase/ssr library
  2. Wait until the access token expires
  3. Call some server code that uses supabase.auth.getSession() to refresh the token
  4. Server might return a Set-Cookie header that sets a non chunked cookie when client previously was using chunked cookie (or vice versa) resulting in doubling up of cookies.

Expected behavior

Should only ever be a single cookie (or group of chunked cookies) representing the session.

System information

  • OS: Vercel Serverless Environment
  • Version of supabase-js: 2.32.0
  • Version of Node.js: 18.x
@astonfuture astonfuture added the bug Something isn't working label Dec 13, 2023
@chriscarrollsmith
Copy link

Interesting. Could this be why I'm exceeding my auth rate limit when testing my application? I am seeing several token requests per second in my database logs, and I'm unsure why it's happening.

@astonfuture
Copy link
Author

Interesting. Could this be why I'm exceeding my auth rate limit when testing my application? I am seeing several token requests per second in my database logs, and I'm unsure why it's happening.

Potentially. Are you seeing both chunked and non chunked cookies in your application? One possible scenario would be:

  • you have a non chunked cookie that is expired
  • you send it in a request to your server
  • supabse/ssr library refreshes the token but for some arbitary reason, the refreshed token is chunked
  • your client receives the new refreshed chunked cookie but retains the non chunked cookie as it wasn't overwritten.
  • on a subsequent request, your client sends all the cookies (chunked and non chunked)
  • supabase/ssr package defaults to reading the expired non chunked cookie and tries to refresh it again.

@ahockersten
Copy link

ahockersten commented Feb 6, 2024

I can confirm this issue.

I also have a (somewhat hack-ish) workaround for anyone affected by this. In your code, when setting a chunked cookie, first clear any non-chunked version:

  const supabase = createServerClient(process.env.SUPABASE_URL!, process.env.SUPABASE_ANON_KEY!, {
    cookies: {
      get(key) {
        return cookies[key]
      },
      set(key, value, options) {
          if (key.endsWith(".0")) {
            headers.append(
              "Set-Cookie",
              serialize(key.split(".0")[0], "", options),
            );
          }
        headers.append('Set-Cookie', serialize(key, value, options))
      },
      remove(key, options) {
        headers.append('Set-Cookie', serialize(key, '', options))
      },
    },
  })

Note that this workaround only really solves the issue in one direction, it won't delete the chunked cookie if Supabase decides to write a non-chunked version. However, it seems the Supabase code will always prefer the non-chunked version, so I don't think it is necessary to do anything about that.

As @astonfuture pointed out, this issue can be hard to verify and debug. If you have a scenario where the cookie is well below the limit for chunking, you can easily make it larger by adding user metadata, e.g.:

await supabase.auth.updateUser({
    data: {
      dummy: "add a really long string here, much longer than this"
    },
  });

By calling updateUser() with and without the large metadata, you should be able to easily cause the cookie to switch between being chunked and not chunked.

@ahockersten
Copy link

@chriscarrollsmith FWIW I have been seeing auth rate limit issues as well lately, and my cookies are obviously right at the edge of where chunking happens. I have not investigated whether the workaround above helps with these yet, but I'll try to get back to you on that.

@chriscarrollsmith
Copy link

@chriscarrollsmith FWIW I have been seeing auth rate limit issues as well lately, and my cookies are obviously right at the edge of where chunking happens. I have not investigated whether the workaround above helps with these yet, but I'll try to get back to you on that.

Fwiw, my auth rate limit error proved to be caused by my middleware running too often. Once I improved the pattern matcher, the issue went away. You can add cobsole logging in your middleware to see what routes it renders on to determine if this affects you.

@damianricobelli
Copy link

I can confirm this issue.

I also have a (somewhat hack-ish) workaround for anyone affected by this. In your code, when setting a chunked cookie, first clear any non-chunked version:

  const supabase = createServerClient(process.env.SUPABASE_URL!, process.env.SUPABASE_ANON_KEY!, {
    cookies: {
      get(key) {
        return cookies[key]
      },
      set(key, value, options) {
          if (key.endsWith(".0")) {
            headers.append(
              "Set-Cookie",
              serialize(key.split(".0")[0], "", options),
            );
          }
        headers.append('Set-Cookie', serialize(key, value, options))
      },
      remove(key, options) {
        headers.append('Set-Cookie', serialize(key, '', options))
      },
    },
  })

Note that this workaround only really solves the issue in one direction, it won't delete the chunked cookie if Supabase decides to write a non-chunked version. However, it seems the Supabase code will always prefer the non-chunked version, so I don't think it is necessary to do anything about that.

As @astonfuture pointed out, this issue can be hard to verify and debug. If you have a scenario where the cookie is well below the limit for chunking, you can easily make it larger by adding user metadata, e.g.:

await supabase.auth.updateUser({
    data: {
      dummy: "add a really long string here, much longer than this"
    },
  });

By calling updateUser() with and without the large metadata, you should be able to easily cause the cookie to switch between being chunked and not chunked.

Same problem here. I had a hard time debugging the behavior, but this issue explains exactly the same behavior I'm having and which is causing me problems. The good thing is that at least this hack that you have commented serves momentarily to resolve the duplicate cookies.

I hope the supabase team manages to solve this issue soon.

@Keagel
Copy link

Keagel commented Apr 28, 2024

Seeing as this is still an issue I've implemented @ahockersten's solution, though if you happen to refresh the session on the client side you also need the equivalent code for the +layout.ts file (SvelteKit):

import { combineChunks, createBrowserClient, isBrowser, parse, serialize } from '@supabase/ssr';

const supabase = createBrowserClient(PUBLIC_SUPABASE_URL, PUBLIC_SUPABASE_ANON_KEY, {
    global: {
        fetch
    },
    cookies: {
        get(key) {
            if (!isBrowser()) {
                return JSON.stringify(data.session);
            }

            const cookie = combineChunks(key, (name) => {
                const cookies = parse(document.cookie);
                return cookies[name];
            });

            return cookie;
        },
        set(key, value, options) {
            if (!isBrowser()) {
                return;
            }

            if (key.endsWith('.0')) {
                document.cookie = serialize(key.split('.0')[0], '', options);
            }

            document.cookie = serialize(key, value, options);
        }
    }
});

Feel free to tell me if something's wrong.

@hf
Copy link
Contributor

hf commented Jun 5, 2024

This is one of the main issues we identified and this discussion explains what we're doing about it: https://github.com/orgs/supabase/discussions/27037

Linking the PR just for reference: supabase/ssr#1

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

6 participants