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

cookieOptions and config.toml config for cookie MaxAge are ignored #40

Open
2 tasks done
Rudolf-Dudarev opened this issue Jul 14, 2024 · 8 comments
Open
2 tasks done
Labels
bug Something isn't working

Comments

@Rudolf-Dudarev
Copy link

Rudolf-Dudarev commented Jul 14, 2024

Bug report

  • I confirm this is a bug with Supabase, not with my own application.
  • I confirm I have searched the Docs, GitHub Discussions, and Discord.

Describe the bug

The supabase/ssr method createServerClient ignores cookieOptions property maxAge. Not only that but the config.toml property jwt_expiry is also ignored when setting the auth cookie expiry. So we get cookies set with Expires / Max-Age of about 1 year. I have a Next.js app where this is observed.

To Reproduce

  1. Open config.toml, set the jwt_expiry to a low value, e.g., 300 (5min)
  2. Initiate a supabase server client with vanilla config found in docs:
    https://supabase.com/docs/guides/auth/server-side/creating-a-client
  3. Add cokieOptions object with the maxAge property in the server client config object with a low value, e.g., 300..
  4. From client side initiate a request to an endpoint that handles server side auth. Use one of the sign-in options like signInAnonymousley() or signInWithPassword() on the server side.
  5. Inspect set auth cookie in your browsers dev tools - the Max Age / Expiry of the cookie is set to be around 1 year, which does not match configuration in the cookieOptions or the config.toml.

I observed this issue in my supabase server client utility function:

import { createServerClient, type CookieOptions } from '@supabase/ssr';
import { Database } from '@/lib/types/supabase';
import { cookies } from 'next/headers';

export default function supabaseServerClient() {
  const cookieStore = cookies();
  return createServerClient<Database>(
    process.env.NEXT_PUBLIC_SUPABASE_API_URL!,
    process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!,
    {
      cookieOptions: {
        // The maxAge property I would expect to be used in setting the cookie expiry and overriding the config.toml config value.
        maxAge: 300,
      },
      cookies: {
        getAll() {
          return cookieStore.getAll();
        },
        setAll(cookiesToSet) {
          try {
            cookiesToSet.forEach(({ name, value, options }) => {
              console.log('_@ OPTIONS', options);

              // The maxAge is not matching the maxAge set in cookieOptions. Nor is it matching the value in the config.toml  
              
              // _@ OPTIONS {
              //   path: '/',
              //   sameSite: 'lax',
              //   httpOnly: false,
              //   maxAge: 31536000000,
              //   expires: 2024-07-14T17:11:04.968Z
              // }

              return cookieStore.set(name, value, options);
            });
          } catch {}
        },
      },
    },
  );
}

Expected behavior

The set cookie has an expiry matching the jwt_expiry field in config.toml or is overriden and matches the maxAge property set in the createServerClient config cookieOptions.

Screenshots

Screenshot 2024-07-14 at 21 13 43

System information

  • OS: macOS
  • Browser: Chrome
  • Version of supabase-js: v2.38.4
  • Version supabase/ssr: v0.4.0
  • Version of Node.js: v20.9.0

Additional context

Using Next.js 14.

@Rudolf-Dudarev Rudolf-Dudarev added the bug Something isn't working label Jul 14, 2024
@j4w8n
Copy link
Contributor

j4w8n commented Jul 16, 2024

I'm pretty sure I know what's going on here.

The ssr library is overwriting the maxAge, set via options, to their default of 1000 years. However, Chrome is now adhering to RFC6265bis and lowering it to 400 days when setting the cookie.

There is a PR from a community member to get the default lowered to 365 or 400, but has not gotten any traction in the past.

Nonetheless, there is also an issue of the ssr library overriding what devs are setting. I assume this is by design, but I'm unsure why.

@encima encima transferred this issue from supabase/supabase Jul 16, 2024
@dmitriyrusnak1
Copy link

Is there any update when this issue might be handled? It's quite concerning that we can't set a shorter expiry time than 1 year for the access token.

@Rudolf-Dudarev
Copy link
Author

@j4w8n @encima Sorry to ping you, but it does seem like a meaningful security issue that the JWT doesn't expire for a year.
I read Supabase docs and everything related to JWTs, it seems that leaving that cookie for a year in client's browser makes it very vulnerable to token theft.
As this post hasn't received much attention, I am curious if it could be just an issue with our setup.
It's especially concerning for our company as we migrated our e-commerce backend to Supabase and are about to go live.
I see that it's not just a local development issue as the setting from our staging Supabase instance also sets the expiry of the cookie to 1 year, overriding the settings.

@j4w8n
Copy link
Contributor

j4w8n commented Aug 14, 2024

@Rudolf-Dudarev I think the auth team has been pretty busy with some high-priority features, and I'm guessing they've had to triage other items like this one. Hopefully they will chime-in by early September, if not sooner; but I have no idea.

@J0
Copy link
Contributor

J0 commented Sep 4, 2024

Thanks @j4w8n for jumping in here.

Here's more context around the fixed value and lack of configurability in the library

We recently updated it to 400 days

Main goal of the long time is to ensure Supabase Auth session expires before the max-age is reached, so as to guard against random logouts.

@j4w8n
Copy link
Contributor

j4w8n commented Sep 24, 2024

Thanks for the reference @J0. I still think the team should consider leaving it up to the developer as to whether they want to set a lower limit than 400 days for the cookie age.

@jkhaui
Copy link

jkhaui commented Oct 5, 2024

Sorry to leave a "+1"comment here, but this is something I've just stumbled across as well and think it's essential that we allow maxAge in cookieOptions to be configurable.

From what I can see in the source code this is the only cookie option that is hardcoded, don't really understand the rationale for this

@nac62116
Copy link

nac62116 commented Oct 9, 2024

Found a way to work arround this issue by extending the cookie options in the get() callback you can pass into createServerClient() "cookies" field.

const cookies = parse(request.headers.get("Cookie") ?? "");
const headers = new Headers();

const authClient = createServerClient(
      process.env.SUPABASE_URL,
      process.env.SUPABASE_ANON_KEY,
      {
        cookies: {
          get(key) {
            return cookies[key];
          },
          set(key, value, options) {
              // maxAge gets overwritten by supabase-ssr because its hard coded to 1000 years.
              // This is the way to work arround this issue.
            const enhancedOptions = {
              ...options,
              maxAge: 60 * 60,
            };
            headers.append(
              "Set-Cookie",
              serialize(key, value, enhancedOptions)
            );
          },
          remove(key, options) {
            headers.append("Set-Cookie", serialize(key, "", options));
          },
        },
        cookieOptions: {
          name: SESSION_NAME,
          secure: process.env.NODE_ENV === "production",
          sameSite: "lax",
          path: "/",
          httpOnly: true,
        },
        auth: {
          flowType: "pkce",
        },
      }
    );

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
Status: No status
Development

No branches or pull requests

6 participants