Security and performance risk with getUser and getSession

[Zanzofily]

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

When using the getUser function from the Supabase auth client in Next.js server components, excessive database queries are generated upon each page refresh. When trying to use getUser to go around that, it solely checks JWT format and expiry without verifying authenticity.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

1. use getUser() in multiple components -> database overload
2. use getSession()-> unsafe

Expected behavior

Please refer to the class I wrote, and let me know if it's correct, it should be proper Jwt verification.

An alternative solution would be endpoint that only returns decoded and verified Jwt

Screenshots

System information

• OS: macOS
• Browser: none (server side)
• Version of [email protected]
• Version of Node.js: 20

Additional context

While implementing authentication with Next.js server components using this package, I observed an excessive number of database queries being generated with each page refresh. Initially, I was using the getUser function, which retrieves user data from the database on every invocation. This approach led to the performance issue noted.

Investigation

Upon reviewing the documentation, I realized that getSession should have been used instead of getUser for maintaining sessions without repeatedly hitting the database. However, further investigation revealed that getSession does not provide proper security checks, as it only verifies the format and expiry of JWTs without validating their authenticity.

Relevant Discussions and Changes

I came across a discussion in GitHub issue #873 from the supabase/auth-js repository, which suggested using getUser initially to eliminate invalid cookies before getSession. This method was flawed as it triggered warnings and was inefficient.

A recent commit (59ec9aff) addressed these warnings but introduced a potential security risk by failing to properly verify JWTs. If this new implementation applied to getSession as well it would only check the format and expiry of the tokens, which are vulnerable to spoofing.

Temporary Workaround

I have developed a workaround supabase-safesession that ensures more secure session handling in my application. Feel free to use it for now if the maintainers verified its correctness.

[j4w8n]

Be careful here. The code doesn't seem to take into account when cookies are chunked for storage.

[Zanzofily]

Be careful here. The code doesn't seem to take into account when cookies are chunked for storage.

@j4w8n The supabase client is a @supabase/ssr's server client, it should be rewriting storage functions to handle this, might need some more type safety here though in case someone misses utils/supabase/server.ts.

[j4w8n]

Be careful here. The code doesn't seem to take into account when cookies are chunked for storage.

@j4w8n The supabase client is a @supabase/ssr's server client, it should be rewriting storage functions to handle this, might need some more type safety here though in case someone misses utils/supabase/server.ts.

@Zanzofily ok. I was just going by grabbing the cookie with next stuff here and thought that might be an issue if the the session is broken into two cookies.

[Zanzofily]

@Zanzofily ok. I was just going by grabbing the cookie with next stuff here and thought that might be an issue if the the session is broken into two cookies.

@j4w8n You are right, I see what you mean now.

createChunks would definitely mess up getAuthTokensFromCookies function in my implementation. It seems that user_metadata can cause the cookie to grow, this field doesn't belong to cookies imo.

[bombillazo]

Both app_metadata and user_metadata can grow the JWT, it has happened to us already.

[Zanzofily]

Both app_metadata and user_metadata can grow the JWT, it has happened to us already.

I see. Someone opened a PR to handle this on the workaround package I created, would merge it once it's ready.