feat: sign in to access backend data with JWT#5
Conversation
❌ Deploy Preview for gnosis-split failed.
|
WalkthroughImplements JWT-based wallet authentication: client signing utilities and Svelte store; new server signin endpoint that issues JWTs; server handle hook that verifies Bearer tokens and populates Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser as Client
participant AuthStore
participant SignInAPI as /api/auth/signin
participant ServerHook as hooks.server
participant SplitsAPI as /api/splits
participant DB
User->>Browser: connect wallet & sign message
Browser->>SignInAPI: POST (message, signature, address, ts)
SignInAPI->>SignInAPI: verify signature & timestamp
SignInAPI->>SignInAPI: issue JWT (HS256, exp=1d)
SignInAPI->>Browser: return token
Browser->>AuthStore: store token, set isAuthenticated
Browser->>Browser: save token to localStorage
Browser->>SplitsAPI: GET /api/splits (Authorization: Bearer {token})
SplitsAPI->>ServerHook: request enters (hook runs)
ServerHook->>ServerHook: verify JWT, decode user
ServerHook->>SplitsAPI: attach user to locals
SplitsAPI->>SplitsAPI: authorize (creator/participant)
SplitsAPI->>DB: query/modify splits
DB->>SplitsAPI: return result
SplitsAPI->>Browser: return data/status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/storage.ts (1)
45-57: Error is thrown then caught and swallowed.At line 51, you throw an error on non-OK responses, but the outer catch block (lines 54-57) catches it and returns an empty array. This inconsistency means errors are logged but callers won't know authentication failed vs. a network error.
Consider distinguishing auth errors from other failures:
if (!response.ok) { const errorData = await response.json().catch(() => ({})); console.error('API error:', response.status, errorData); + if (response.status === 401) { + // Could trigger re-auth or clear invalid token + } throw new Error(`Failed to fetch splits: ${response.status}`); }
🧹 Nitpick comments (8)
src/hooks.server.ts (1)
11-15: Consider adding proper typing and error logging.The
as anycast loses type safety. Also, silently swallowing JWT verification errors may complicate debugging authentication issues in production.- const decodedPayload = jwt.verify(token, SUPABASE_JWT_SECRET, { algorithms: ['HS256'] }) as any; + const decodedPayload = jwt.verify(token, SUPABASE_JWT_SECRET, { algorithms: ['HS256'] }) as App.Locals['user']; event.locals.user = decodedPayload; } catch (error) { + console.warn('JWT verification failed:', error instanceof Error ? error.message : 'Unknown error'); event.locals.user = null;package.json (1)
54-54: Move@types/jsonwebtokento devDependencies.Type definitions are only needed at build time and shouldn't be shipped to production.
Move the dependency:
"devDependencies": { + "@types/jsonwebtoken": "^9.0.10", "@eslint/compat": "^1.4.0", ... }, "dependencies": { - "@types/jsonwebtoken": "^9.0.10", "@reown/appkit": "^1.8.14",src/lib/supabase.ts (1)
17-17:Content-Typeheader is unnecessary for GET requests.GET requests typically don't have a body, so
Content-Type: application/jsonis redundant here. While harmless, it could be removed for cleanliness.src/routes/+layout.svelte (1)
6-6: Add a comment to clarify the side-effect import.This bare import initializes the auth store and sets up authentication checks at app load. A brief comment will clarify intent for future maintainers.
- import '$lib/stores/auth'; + // Initialize auth store and check authentication status on app load + import '$lib/stores/auth';src/lib/auth.ts (3)
6-6: Typeconfigparameter properly instead of usingany.The
configparameter has typeany, which loses type safety. Consider importing and using the proper Wagmi config type.+import type { Config } from '@wagmi/core'; + -export async function signInWithWallet(address: Address, config: any): Promise<string> { +export async function signInWithWallet(address: Address, config: Config): Promise<string> {
42-44: Remove redundant try-catch that only re-throws.The catch block at lines 42-44 catches the error and immediately re-throws it without any transformation or logging, which is redundant.
- } catch (error) { - throw error; - } + }
37-41: Missing SSR guard before accessinglocalStorage.Unlike
getAuthTokenandclearAuth,signInWithWalletdoesn't check fortypeof window === 'undefined'before accessinglocalStorage. While this function is unlikely to be called server-side, adding the guard would be consistent.const { token } = await response.json(); + if (typeof window !== 'undefined') { localStorage.setItem(AUTH_TOKEN_KEY, token); + } return token;src/routes/api/splits/[id]/+server.ts (1)
28-37: Extract duplicated authorization logic into a helper function.The same authorization check pattern (fetch split, checksum address, check isCreator/isParticipant) is repeated in all four handlers. Consider extracting to a shared utility.
async function checkAuthorization( supabase: SupabaseClient, splitId: string, userAddress: string ): Promise<{ authorized: boolean; isCreator: boolean; split?: SplitData; error?: Response }> { const checksumUserAddress = getAddress(userAddress); const { data, error } = await supabase .from('splits') .select('*') .eq('id', splitId) .single(); if (error || !data) { return { authorized: false, isCreator: false, error: json({ error: 'Split not found' }, { status: 404 }) }; } const isCreator = data.payer_address === checksumUserAddress; const participants = data.participants as unknown as Participant[]; const isParticipant = participants?.some(p => { try { return getAddress(p.address) === checksumUserAddress; } catch { return false; } }); return { authorized: isCreator || isParticipant, isCreator, split: data }; }Also applies to: 89-98, 167-176, 216-225
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
package.json(1 hunks)src/app.d.ts(1 hunks)src/hooks.server.ts(1 hunks)src/lib/auth.ts(1 hunks)src/lib/storage.ts(7 hunks)src/lib/stores/auth.ts(1 hunks)src/lib/supabase.ts(2 hunks)src/routes/+layout.svelte(1 hunks)src/routes/api/auth/signin/+server.ts(1 hunks)src/routes/api/splits/+server.ts(3 hunks)src/routes/api/splits/[id]/+server.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/lib/supabase.ts (1)
src/lib/auth.ts (1)
getAuthToken(47-50)
src/lib/auth.ts (1)
src/lib/stores/wallet.ts (1)
address(41-41)
src/routes/api/splits/+server.ts (3)
src/routes/api/splits/[id]/+server.ts (1)
GET(8-56)src/lib/server/supabase.ts (1)
getSupabase(7-25)src/routes/api/auth/signin/+server.ts (1)
POST(10-54)
src/lib/stores/auth.ts (2)
src/lib/auth.ts (2)
signInWithWallet(6-45)getAuthToken(47-50)src/lib/stores/wallet.ts (1)
address(41-41)
src/routes/api/splits/[id]/+server.ts (3)
src/routes/api/splits/+server.ts (1)
GET(8-56)src/lib/types.ts (1)
Participant(34-38)src/lib/server/supabase.ts (1)
getSupabase(7-25)
src/lib/storage.ts (1)
src/lib/auth.ts (1)
getAuthToken(47-50)
🪛 GitHub Actions: Netlify Deploy
src/routes/api/auth/signin/+server.ts
[error] 5-5: "SUPABASE_JWT_SECRET" is not exported by "virtual:env/static/private", imported by "src/routes/api/auth/signin/+server.ts".
🔇 Additional comments (5)
src/app.d.ts (1)
6-15: LGTM!The
Localsinterface correctly defines the JWT payload structure with proper nullable typing for unauthenticated requests. The inclusion ofiatandexpat the top level aligns with standard JWT claims.src/hooks.server.ts (1)
5-21: Protected routes already enforce authentication with 401 responses.The current implementation correctly delegates authorization to individual routes, and the protected API endpoints (
/api/splits/*) all validatelocals.userand return401status with "Unauthorized" error for missing or invalid tokens. The pattern is working as intended.package.json (1)
56-56: No action required—jsonwebtoken 9.0.3 has no known security vulnerabilities. Previous CVEs (CVE-2022-23529, CVE-2022-23541, etc.) were fixed in the v9.0.0 release; this version constraint is secure.src/lib/storage.ts (1)
9-25: LGTM!The
getAuthHeaders()andgetHeaders()utility functions correctly compose authorization headers. The pattern of returning an empty object when no token exists allows unauthenticated requests to proceed gracefully.src/routes/api/splits/+server.ts (1)
8-56: LGTM on authentication integration.The GET handler correctly derives the user address from
locals.userpopulated by the auth hook, returns 401 for unauthenticated requests, and properly filters splits by the authenticated user's address.
|
🚀 Deployed on https://gnosis-split.netlify.app |
|
🚀 Deployed on https://gnosis-split.netlify.app |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/routes/api/splits/[id]/+server.ts (2)
58-100: Restrict PUT updates so participants cannot change core split fields (includingpayer_address).The PUT handler currently treats creators and participants the same:
- If the user is either
isCreatororisParticipant, they can update all fields, includingpayer_address,description,totalAmount, anddate.That lets any participant reassign
payer_addressto themselves (or someone else), effectively taking ownership of the split and gaining creator‑level privileges (including delete), which is a serious authorization bug.A straightforward fix is to reserve the full update for creators only and route participant‑level changes through the more specific PATCH endpoint:
- const isCreator = splitData.payer_address === checksumUserAddress; + const isCreator = splitData.payer_address === checksumUserAddress; @@ - if (!isCreator && !isParticipant) { - return json({ error: 'Forbidden' }, { status: 403 }); - } + if (!isCreator) { + return json({ error: 'Forbidden' }, { status: 403 }); + }If you do want participants to update some fields via PUT, introduce a role‑based whitelist and reject payloads containing forbidden keys for non‑creators rather than applying them blindly.
28-33: Guard againstgetAddress(p.address)throwing for malformed participant data.All three participant checks:
- GET:
participants?.some((p) => getAddress(p.address) === checksumUserAddress)- PUT: same pattern on
splitData.participants- PATCH: same pattern on
splitData.participantswill throw if any stored participant address is malformed, turning a data issue into a 500.
A minimal defensive pattern:
- const isParticipant = participants?.some( - (p: Participant) => getAddress(p.address) === checksumUserAddress - ); + const isParticipant = participants?.some((p: Participant) => { + try { + return getAddress(p.address) === checksumUserAddress; + } catch { + return false; + } + });Apply the same change in the PUT and PATCH handlers where this check is duplicated.
According to the viem documentation, does `getAddress` throw an exception when called with an invalid Ethereum address (e.g., bad checksum or wrong format), or does it return a falsy value?Also applies to: 92-94, 212-215
src/routes/api/splits/+server.ts (1)
10-23: Defensively handlegetAddress()failures for user and payer addresses.Both usages:
const checksumAddress = getAddress(userAddress);if (getAddress(split.payerAddress) !== getAddress(userAddress)) { ... }will throw if either
userAddress(from the token) orsplit.payerAddress(from the request body) is malformed. Today that ends up as a generic 500 rather than a clear client error.Consider wrapping these in a small guard and returning a 400 instead:
- const checksumAddress = getAddress(userAddress); + let checksumAddress: string; + try { + checksumAddress = getAddress(userAddress); + } catch { + return json({ error: 'Invalid user address' }, { status: 400 }); + }and for the POST check:
- if (getAddress(split.payerAddress) !== getAddress(userAddress)) { + let payerChecksum: string; + let userChecksum: string; + try { + payerChecksum = getAddress(split.payerAddress); + userChecksum = getAddress(userAddress); + } catch { + return json({ error: 'Invalid address in request' }, { status: 400 }); + } + + if (payerChecksum !== userChecksum) { return json({ error: 'Forbidden: can only create splits for yourself' }, { status: 403 }); }This keeps authorization logic while avoiding treating bad input as a server error.
In viem, what does `getAddress` do when passed an invalid or malformed Ethereum address string (e.g., wrong length or non-hex)? Does it throw an error or return a fallback?Also applies to: 77-79
🧹 Nitpick comments (1)
src/lib/stores/auth.ts (1)
38-64: Guard wallet‑driven auth side effects to the browser and avoid double clears.Right now the
address.subscribeside effects (callingclearAuth()andattemptSignIn) can run during SSR if the wallet store ever emits a non‑null value server‑side, andclearAuth()is invoked twice when transitioning from a real address to a falsy$address.Consider:
- Wrapping the subscription in the same browser guard as
checkAuthStatus():-if (typeof window !== 'undefined') { - checkAuthStatus(); -} - -address.subscribe(async ($address) => { +if (typeof window !== 'undefined') { + checkAuthStatus(); + + address.subscribe(async ($address) => { if (lastAddress && lastAddress !== $address) { clearAuth(); isAuthenticated.set(false); signInError.set(null); } if (!$address) { clearAuth(); isAuthenticated.set(false); signInError.set(null); lastAddress = null; return; } lastAddress = $address; const currentlyAuthenticated = get(isAuthenticated); if (!currentlyAuthenticated) { await attemptSignIn($address); } -}); + }); +}
- (Optional) Removing the first
clearAuth()branch for the “address becomes falsy” case, since the!$addressblock already clears state.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/storage.ts(7 hunks)src/lib/stores/auth.ts(1 hunks)src/lib/supabase.ts(2 hunks)src/routes/api/auth/signin/+server.ts(1 hunks)src/routes/api/splits/+server.ts(3 hunks)src/routes/api/splits/[id]/+server.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/storage.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/routes/api/auth/signin/+server.ts (2)
src/routes/api/splits/+server.ts (1)
POST(58-118)src/lib/stores/wallet.ts (1)
address(41-41)
src/lib/supabase.ts (1)
src/lib/storage.ts (1)
getHeaders(19-25)
src/lib/stores/auth.ts (2)
src/lib/auth.ts (3)
signInWithWallet(6-45)getAuthToken(47-50)clearAuth(52-55)src/lib/stores/wallet.ts (1)
address(41-41)
src/routes/api/splits/+server.ts (3)
src/routes/api/splits/[id]/+server.ts (1)
GET(8-56)src/lib/server/supabase.ts (1)
getSupabase(7-25)src/routes/api/auth/signin/+server.ts (1)
POST(40-83)
🔇 Additional comments (2)
src/lib/supabase.ts (1)
3-15: UsinggetHeaders()here is a solid reuse of the shared auth/header helper.This keeps header construction consistent with the rest of the API calls and removes duplication from this store loader.
src/routes/api/auth/signin/+server.ts (1)
11-82: Signin flow now correctly validates message, timestamp, and signature before issuing tokens.The added
validateMessage()plus checksum normalization of the address and explicit error responses make this handler robust; no further issues stand out.
1 participant
This PR includes:
Summary by CodeRabbit
New Features
Improvements
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.