perf(auth): switch to claim-based user lookups#1498
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughMigrates authentication from Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Frontend Component
participant Claims as Supabase Auth
participant Service as Component Logic
Client->>Claims: getClaims()
Claims-->>Client: claimsData (claims.sub)
Client->>Service: Extract userId from claims.sub
Service->>Service: Validate userId presence
Service-->>Client: Authorization state (hasAuth)
Client->>Service: Use userId for API operations
sequenceDiagram
participant Request as HTTP Request
participant Middleware as middlewareAuth
participant JWT as getClaimsFromJWT()
participant Handler as Edge Function Handler
participant DB as Database
Request->>Middleware: Bearer token
Middleware->>JWT: Decode JWT (local)
JWT-->>Middleware: JWTClaims {sub, exp, ...}
Middleware->>Middleware: Validate claims.sub
Middleware->>Middleware: Set auth.userId from claims.sub
Middleware->>Handler: auth context {userId}
Handler->>DB: Query with userId
DB-->>Handler: Results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3779a8a66d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Decode JWT claims without network call (much faster than getUser()) | ||
| const claims = getClaimsFromJWT(authorization) | ||
| if (!claims || !claims.sub) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'Invalid JWT claims' }) | ||
| throw simpleError('invalid_jwt', 'Invalid JWT') |
There was a problem hiding this comment.
The new middlewareAuth now derives userId solely from getClaimsFromJWT, which only base64-decodes the payload and checks exp without verifying the JWT signature. Because checkPermission later trusts auth.userId directly in rbac_check_permission_direct (no Supabase auth validation), an attacker can forge a Bearer token with any sub and bypass RBAC-protected endpoints. Previously supabase.auth.getUser() verified the token. This creates an authorization bypass whenever a request includes an untrusted JWT string. Please restore signature verification (e.g., getUser() or explicit JWT verification) before setting auth.userId.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR implements a performance optimization by replacing getUser() API calls with local JWT claim decoding across the backend and frontend.
Changes:
- Added
getClaimsFromJWT()function to decode JWT claims locally without network calls - Updated backend middleware (
middlewareAuth,foundJWT) to use claim-based authentication - Updated backend endpoints (stripe_portal, stripe_checkout, invite_new_user_to_org, download_link) to use auth context instead of
getUser() - Updated frontend pages and components to use
getClaims()instead ofgetUser()
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| supabase/functions/_backend/utils/hono.ts | Added getClaimsFromJWT() function and JWTClaims interface; updated middlewareAuth to decode claims locally |
| supabase/functions/_backend/utils/hono_middleware.ts | Updated foundJWT() to use getClaimsFromJWT() instead of getUser() |
| supabase/functions/_backend/private/stripe_portal.ts | Changed to use auth context (c.get('auth')) instead of getUser() |
| supabase/functions/_backend/private/stripe_checkout.ts | Changed to use auth context instead of getUser() |
| supabase/functions/_backend/private/invite_new_user_to_org.ts | Changed to use auth context instead of getUser() |
| supabase/functions/_backend/private/download_link.ts | Changed to use auth context instead of getUser() |
| src/pages/settings/account/index.vue | Changed to use getClaims() instead of getUser() |
| src/pages/login.vue | Changed to use getClaims() and getSession() instead of getUser() |
| src/pages/invitation.vue | Changed to use getClaims() to check authentication status |
| src/pages/delete_account.vue | Changed to use getClaims() instead of getUser() |
| src/pages/ApiKeys.vue | Changed to use getClaims() instead of getUser() for API key operations |
| src/modules/auth.ts | Updated guard function to use getClaims() and getSession() instead of getUser() |
| src/components/dashboard/StepsBundle.vue | Changed to use getClaims() for API key creation |
| src/components/dashboard/StepsBuild.vue | Changed to use getClaims() for API key creation |
| src/components/dashboard/StepsApp.vue | Changed to use getClaims() for API key creation |
riderx
force-pushed
the
riderx/getuser-to-getclaims
branch
from
3115087 to
b62a9c3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
supabase/functions/_backend/utils/hono.ts (1)
163-176: Unverified JWT claims trusted for authentication context.The
middlewareAuthmiddleware now derivesuserIdfromgetClaimsFromJWT, which doesn't verify the JWT signature. Previously,supabase.auth.getUser()verified the token server-side. This change means any endpoint usingmiddlewareAuthwill accept forged tokens.Endpoints that only use
c.get('auth').userIdfor authorization decisions (without subsequent RLS-protected Supabase operations) are now vulnerable to identity spoofing.supabase/functions/_backend/utils/hono_middleware.ts (1)
460-481: Same JWT verification gap applies tomiddlewareV2.The
foundJWTfunction now uses the unverifiedgetClaimsFromJWTto deriveuserId. This affects all endpoints protected bymiddlewareV2(rights)that rely on JWT authentication. The same authorization bypass risk identified inhono.tsapplies here.Since
middlewareV2is likely used across many endpoints (based on learnings indicating it's the standard middleware for external API keys and JWT auth), this significantly expands the attack surface.
🤖 Fix all issues with AI agents
In `@src/modules/auth.ts`:
- Around line 157-159: In the route-guard inside src/modules/auth.ts (the auth
middleware/guard that uses from.path and to.path), fix the path comparison that
currently reads from.path !== 'login' to include the leading slash: use
from.path !== '/login' so it matches Vue Router's path format and avoids
redirect loops when already on the login page; update the condition in the same
guard where hasAuth is checked to use '/login' for consistency with other
comparisons like to.path !== '/accountDisabled' and
to.path.startsWith('/admin').
In `@supabase/functions/_backend/private/invite_new_user_to_org.ts`:
- Around line 152-155: The guard that checks authContext in
invite_new_user_to_org.ts returns a 500 on missing auth (const authContext =
c.get('auth'); if (!authContext?.userId) ...); update the response to return
status: 401 instead of 500 to reflect an authentication failure—modify the block
that returns { message: 'Failed to get current user', error: 'Not authorized',
status: 500 } so it returns status: 401.
♻️ Duplicate comments (1)
supabase/functions/_backend/utils/hono.ts (1)
30-62: JWT signature not verified - authorization bypass risk.The
getClaimsFromJWTfunction only base64-decodes the JWT payload and checks expiration. It does not verify the cryptographic signature. The comment on line 33 stating "The JWT signature is still verified by Supabase when used with RLS policies" is incorrect for this context—RLS policies verify the JWT when Supabase client operations occur, butmiddlewareAuthsetsauth.userIddirectly from the unverifiedclaims.subbefore any RLS-protected operation.An attacker can craft a Bearer token with an arbitrary
subclaim and bypass RBAC checks in endpoints that rely onc.get('auth').userIdwithout making subsequent RLS-protected Supabase calls.Consider one of:
- Use
supabase.auth.getUser()for JWT validation (original behavior)- Implement proper JWT signature verification using the Supabase JWT secret
- Use Supabase's
getClaims()server-side if available
🧹 Nitpick comments (1)
src/modules/auth.ts (1)
114-116: Minor redundancy:sessionUsercheck is implied byhasAuth.Since
hasAuthis defined as!!claimsData?.claims?.sub && !!sessionUser(line 90), the explicitsessionUsercheck on line 114 is redundant. This doesn't affect correctness but could be simplified.♻️ Optional simplification
- if (hasAuth && sessionUser && !main.auth) { + if (hasAuth && !main.auth) {
| else if (from.path !== 'login' && !hasAuth) { | ||
| main.auth = undefined | ||
| next(`/login?to=${to.path}`) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the auth.ts file to understand the context
rg -n "from\.path" src/modules/auth.ts -B 3 -A 3Repository: Cap-go/capgo
Length of output: 226
🏁 Script executed:
# Also check other usages of from.path in the codebase
rg -n "from\.path" --type=ts --type=vue -C 2Repository: Cap-go/capgo
Length of output: 83
🏁 Script executed:
# Check the entire auth.ts file to understand its purpose and structure
wc -l src/modules/auth.tsRepository: Cap-go/capgo
Length of output: 79
🏁 Script executed:
cat -n src/modules/auth.tsRepository: Cap-go/capgo
Length of output: 7577
🏁 Script executed:
# Search for path comparisons more broadly
rg "\.path\s*[!=]" src/modules/auth.ts -B 2 -A 2Repository: Cap-go/capgo
Length of output: 440
🏁 Script executed:
# Search for path string patterns to understand what format is used
rg "'/(login|auth)" src/modules/ -nRepository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Verify vue-router documentation about path format by checking types or usage patterns
rg "path.*==|path.*!=|path.*startsWith" src/modules/ -n | head -20Repository: Cap-go/capgo
Length of output: 257
🏁 Script executed:
# Check if there are any route definitions that show how paths are used
fd -e ts -e js "router|route" src/ --exec grep -l "path:" {} \;Repository: Cap-go/capgo
Length of output: 38
Fix path comparison on line 157: add leading slash to match vue-router path format
The condition from.path !== 'login' is missing the leading slash. Vue Router paths always include a leading slash (e.g., /login), so this comparison will always evaluate to true even when the user is already on the login page. This could cause redirect loops.
Change line 157 to: else if (from.path !== '/login' && !hasAuth) {
This aligns with path comparisons on lines 164 (to.path !== '/accountDisabled') and 183 (to.path.startsWith('/admin')).
🤖 Prompt for AI Agents
In `@src/modules/auth.ts` around lines 157 - 159, In the route-guard inside
src/modules/auth.ts (the auth middleware/guard that uses from.path and to.path),
fix the path comparison that currently reads from.path !== 'login' to include
the leading slash: use from.path !== '/login' so it matches Vue Router's path
format and avoids redirect loops when already on the login page; update the
condition in the same guard where hasAuth is checked to use '/login' for
consistency with other comparisons like to.path !== '/accountDisabled' and
to.path.startsWith('/admin').
| const authContext = c.get('auth') | ||
| if (!authContext?.userId) { | ||
| return { message: 'Failed to get current user', error: 'Not authorized', status: 500 } | ||
| } |
There was a problem hiding this comment.
Error status code should be 401, not 500.
When authContext?.userId is missing, returning status 500 ("Internal Server Error") is misleading. This is an authentication failure, not a server error. Use 401 for consistency with other auth failures in the codebase.
Suggested fix
const authContext = c.get('auth')
if (!authContext?.userId) {
- return { message: 'Failed to get current user', error: 'Not authorized', status: 500 }
+ return { message: 'Failed to get current user', error: 'Not authorized', status: 401 }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const authContext = c.get('auth') | |
| if (!authContext?.userId) { | |
| return { message: 'Failed to get current user', error: 'Not authorized', status: 500 } | |
| } | |
| const authContext = c.get('auth') | |
| if (!authContext?.userId) { | |
| return { message: 'Failed to get current user', error: 'Not authorized', status: 401 } | |
| } |
🤖 Prompt for AI Agents
In `@supabase/functions/_backend/private/invite_new_user_to_org.ts` around lines
152 - 155, The guard that checks authContext in invite_new_user_to_org.ts
returns a 500 on missing auth (const authContext = c.get('auth'); if
(!authContext?.userId) ...); update the response to return status: 401 instead
of 500 to reflect an authentication failure—modify the block that returns {
message: 'Failed to get current user', error: 'Not authorized', status: 500 } so
it returns status: 401.
|
* perf(auth): use claims for user context * docs: note claim-based auth guidance * fix(backend): drop unused supabase import
* perf(auth): use claims for user context * docs: note claim-based auth guidance * fix(backend): drop unused supabase import
* perf(auth): use claims for user context * docs: note claim-based auth guidance * fix(backend): drop unused supabase import
Summary (AI generated)
Test plan (AI generated)
Screenshots (AI generated)
Checklist (AI generated)
/Users/martindonadieu/conductor/workspaces/capgo/sacramento-v1/supabase/functions/_backend/utils/hono_middleware.ts
9:49 error 'supabaseClient' is defined but never used unused-imports/no-unused-imports
✖ 1 problem (1 error, 0 warnings)
1 error and 0 warnings potentially fixable with the
--fixoption..accordingly.
my tests
Generated with AI
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.