supabase / auth-js

An isomorphic Javascript library for Supabase Auth.
MIT License
366 stars 164 forks source link

Security and performance risk with `getUser` and `getSession` #898

Closed Zanzofily closed 1 month ago

Zanzofily commented 6 months ago

Bug report

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

supabase-queries

System information

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

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

Zanzofily commented 6 months ago

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

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

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

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

Zanzofily commented 6 months ago

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.

jkhaui commented 2 months ago

Can we use react 19's cache function to wrap supabase.auth.getUser?

If we then call this function in multiple RSCs it should only make a single request to the auth server, right?

Zanzofily commented 2 months ago

Can we use react 19's cache function to wrap supabase.auth.getUser?

If we then call this function in multiple RSCs it should only make a single request to the auth server, right?

lucia-auth suggests that in their docs, I haven't personally tested it though, as I have seen caching go wrong with cookies before.

pppdns commented 2 weeks ago

When trying to use getUser to go around that, it solely checks JWT format and expiry without verifying authenticity.

I think you meant getSession, not getUser here @Zanzofily