Coderabbit test - #9
Conversation
- Add users table migration for Bluesky-authenticated users - Implement user service with getOrCreateUser and getUserByHandle - Complete verify-challenge endpoint to create users and sessions - Add session management helpers (getSession, clearSession) - Add logout endpoint - Enhance accounts used display on match and compare pages - Improve methodology page with account selection transparency - Add health check endpoint (/api/health) - Improve error messages throughout with actionable guidance - Add loading states and spinners for async operations - Add better error styling and user feedback
Co-authored-by: anitabansal.flights <anitabansal.flights@gmail.com>
Co-authored-by: anitabansal.flights <anitabansal.flights@gmail.com>
Co-authored-by: anitabansal.flights <anitabansal.flights@gmail.com>
WalkthroughImplements user authentication and session management for Bluesky integration, adding a new users database table, cookie-based session handling, user service with Bluesky API calls, updated auth endpoints, logout and health check endpoints, and documentation updates describing platform-specific eligibility criteria. Changes
Sequence DiagramsequenceDiagram
participant Client
participant VerifyAPI as Verify Challenge API
participant Bluesky as Bluesky API
participant Supabase as Supabase DB
participant SessionMgmt as Session Manager
Client->>VerifyAPI: POST /api/auth/bsky/verify-challenge<br/>(handle, challenge, proof)
VerifyAPI->>VerifyAPI: Validate handle present
VerifyAPI->>Bluesky: Verify challenge proof
alt Verification Failed
Bluesky-->>VerifyAPI: Invalid proof
VerifyAPI-->>Client: 400 verification_failed
else Verification Success
Bluesky-->>VerifyAPI: ✓ Valid
VerifyAPI->>Bluesky: Resolve handle to DID<br/>+ fetch profile data
Bluesky-->>VerifyAPI: DID, displayName, avatar
VerifyAPI->>Supabase: Upsert user (bsky_handle,<br/>bsky_did, display_name,<br/>avatar_url, verified_at)
alt Upsert Fails
Supabase-->>VerifyAPI: Error
VerifyAPI-->>Client: 500 user_creation_failed
else Upsert Success
Supabase-->>VerifyAPI: User { id, ... }
VerifyAPI->>SessionMgmt: Set session cookie<br/>(userId, handle, verifiedAt)
SessionMgmt-->>VerifyAPI: Cookie set
VerifyAPI-->>Client: 200 { ok: true,<br/>user: { id, handle,<br/>displayName, avatarUrl,<br/>verifiedAt } }
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
src/routes/api/health/+server.ts (2)
60-62: Duplicated/xrpcsuffix stripping logic.This logic is duplicated from
src/lib/services/userService.ts(getServiceBasefunction at lines 19-23). Consider extracting to a shared utility in$lib/config/bsky.ts.
81-81: Consider returning 503 for degraded status.Load balancers and monitoring systems often use HTTP status codes to determine service health. Returning 200 for
degradedmay cause traffic to continue routing to a partially unhealthy instance.- const statusCode = health.status === 'down' ? 503 : health.status === 'degraded' ? 200 : 200; + const statusCode = health.status === 'ok' ? 200 : 503;src/lib/services/userService.ts (2)
38-46: Redundant API calls togetProfile.
resolveDidFromHandlecallsgetProfileto get the DID, thengetOrCreateUsercallsgetProfileagain with the same DID. The first call already returns the full profile includingdisplayNameandavatar. Consider refactoring to return the profile data fromresolveDidFromHandle.-async function resolveDidFromHandle(handle: string): Promise<string | null> { +async function resolveProfileFromHandle(handle: string): Promise<{ did: string; displayName?: string; avatar?: string } | null> { try { const agent = await getAgent(); const res: any = await (agent as any).getProfile?.({ actor: handle }); - return res?.data?.did ?? null; + if (!res?.data?.did) return null; + return { + did: res.data.did, + displayName: res.data.displayName ?? undefined, + avatar: res.data.avatar ?? undefined + }; } catch { return null; } }Then in
getOrCreateUser:- const did = await resolveDidFromHandle(normalizedHandle); - let displayName: string | null = null; - let avatarUrl: string | null = null; - - if (did) { - try { - const agent = await getAgent(); - const profile: any = await (agent as any).getProfile?.({ actor: did }); - displayName = profile?.data?.displayName ?? null; - avatarUrl = profile?.data?.avatar ?? null; - } catch { - // Ignore profile fetch errors - } - } + const profile = await resolveProfileFromHandle(normalizedHandle); + const did = profile?.did ?? null; + const displayName = profile?.displayName ?? null; + const avatarUrl = profile?.avatar ?? null;Also applies to: 64-73
41-41: Use the properly typedagent.getProfile()method instead of casting toany.The
@atproto/apiBskyAgent exposes a properly typedgetProfilemethod with QueryParams{ actor: string }and OutputSchemaProfileViewDetailed. Removing the(agent as any)cast and using the typed method directly preserves type safety without any functional change.bugbot.md (1)
11-11: Consider rephrasing for improved readability.The static analysis tool suggests that "e.g., due to database errors" may make the sentence harder to follow.
Consider this alternative phrasing:
-**Rationale**: In this codebase, we intentionally clear verification challenges immediately upon verification, before attempting user creation. If user creation fails (e.g., due to database errors), users will need to request a new verification challenge and post a new code. This is an acceptable UX trade-off for this application. +**Rationale**: In this codebase, we intentionally clear verification challenges immediately upon verification, before attempting user creation. If user creation fails (for example, because of database errors), users will need to request a new verification challenge and post a new code. This is an acceptable UX trade-off for this application.src/lib/auth/session.ts (1)
2-2: Remove unused import.The
getUserByHandleimport is not used anywhere in this file.Apply this diff:
-import { getUserByHandle } from '$lib/services/userService';supabase/migrations/005_create_users.sql (1)
23-35: RLS policy is permissive but currently unused—clarify intent or remove.The SELECT policy uses
using (true), which theoretically allows public reads of all user records. However, the application architecture doesn't enforce this RLS policy: all server-side queries use the admin client (service role), which bypasses RLS entirely. Access control is implemented via session cookies, not RLS.The "for now; can restrict later" comment indicates this was intentional, but the current state creates confusion:
- The RLS policy is dead code (never enforced)
- It's undocumented (unlike the intentional RLS strategy on the comments table, which is explained in README)
- Future developers might mistakenly assume RLS protects user data
Recommendation: Either remove the unused RLS policy, or if client-side user data access is planned for future phases, implement a proper auth strategy (e.g., Supabase Auth or JWT) with appropriate RLS policies documented in README alongside the rationale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
bugbot.md(1 hunks)src/lib/auth/session.ts(1 hunks)src/lib/services/userService.ts(1 hunks)src/routes/api/auth/bsky/create-challenge/+server.ts(2 hunks)src/routes/api/auth/bsky/verify-challenge/+server.ts(1 hunks)src/routes/api/auth/logout/+server.ts(1 hunks)src/routes/api/health/+server.ts(1 hunks)src/routes/compare/[matchId]/+page.svelte(1 hunks)src/routes/match/[id]/+page.svelte(9 hunks)src/routes/methodology/+page.svelte(2 hunks)supabase/migrations/005_create_users.sql(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/routes/api/health/+server.ts (3)
src/lib/supabaseAdmin.ts (1)
getSupabaseAdmin(10-33)src/lib/supabaseClient.ts (1)
getSupabaseClient(5-22)src/lib/config/bsky.ts (1)
BSKY_APPVIEW_BASE(1-1)
src/routes/api/auth/logout/+server.ts (1)
src/lib/auth/session.ts (1)
clearSession(30-32)
src/lib/services/userService.ts (2)
src/lib/config/bsky.ts (1)
BSKY_APPVIEW_BASE(1-1)src/lib/supabaseAdmin.ts (1)
getSupabaseAdmin(10-33)
src/routes/api/auth/bsky/verify-challenge/+server.ts (2)
src/lib/auth/bskyVerifyStore.ts (3)
getChallenge(46-55)verifyPostContainsCode(66-93)clearChallenge(57-60)src/lib/services/userService.ts (1)
getOrCreateUser(53-110)
🪛 LanguageTool
bugbot.md
[style] ~11-~11: This wording can make your sentence hard to follow. Try rephrasing for improved clarity.
Context: ...r creation. If user creation fails (e.g., due to database errors), users will need to re...
(DUE_TO_BECAUSE)
🔇 Additional comments (17)
src/routes/methodology/+page.svelte (2)
45-54: LGTM! Eligibility rules are well documented.The updated eligibility criteria are clear with platform-specific thresholds and appropriate disclosure about API limitations and admin overrides.
64-76: LGTM! Transparency section provides good user guidance.The new transparency subsection clearly documents how account lists are displayed across different analysis windows, supporting the PR's goal of user verification capabilities.
src/routes/match/[id]/+page.svelte (4)
94-107: LGTM! Improved error handling for snapshot loading.Good extraction of error details from response with appropriate fallbacks. The error message hierarchy (JSON message → text → default) provides better user feedback.
147-162: LGTM! Comprehensive error handling for summary loading.The differentiated error messages for rate limiting, timeouts, and missing API key significantly improve user experience by providing actionable feedback.
225-239: LGTM! Enhanced live accounts section.The added border separator, explanatory paragraph, and DID tooltips on account chips improve transparency and align with the methodology documentation.
350-358: LGTM! Loading spinner and error styling improvements.The CSS spinner animation and styled error messages provide clear visual feedback during loading and error states.
src/routes/compare/[matchId]/+page.svelte (1)
105-113: LGTM! Consistent UI updates for account transparency.The explanatory paragraph and DID tooltips on account chips are consistent with the changes in the match page, maintaining UI coherence across the application.
src/routes/api/auth/bsky/create-challenge/+server.ts (2)
9-20: LGTM!The structured error response for missing handle is clear and provides helpful guidance to the user.
51-62: LGTM!The enhanced error handling with structured JSON responses provides better debugging information and consistency with other error responses in the file.
src/routes/api/auth/logout/+server.ts (1)
1-34: LGTM!The logout endpoint implementation is clean and follows best practices with structured error responses and appropriate HTTP status codes. The idempotent nature of the operation (clearing a non-existent cookie is safe) makes this a robust implementation.
src/lib/auth/session.ts (1)
30-41: LGTM!The session clearing and handle retrieval functions are implemented correctly. However, note the critical security issue flagged in the
getSessionfunction regarding unsigned session cookies.src/routes/api/auth/bsky/verify-challenge/+server.ts (6)
11-21: LGTM!The addition of the
cookiesparameter enables session management, and the enhanced error response is clear and user-friendly.
23-35: LGTM!The enhanced error response provides clear guidance to users when no active challenge is found.
37-51: LGTM!The verification error handling provides context-specific feedback that helps users understand and resolve issues.
53-69: LGTM!The challenge lifecycle and user creation flow are implemented correctly. Note that clearing the challenge before user creation (line 54) is an intentional design decision documented in bugbot.md, where users must request a new challenge if user creation fails. The error handling appropriately returns a 500 status with a clear message.
71-84: Good cookie security flags; session signing addressed in session.ts.The cookie security configuration is solid with
httpOnly,securein production, andsameSite: 'lax'. The critical issue of unsigned session cookies is already flagged in thesrc/lib/auth/session.tsreview.The 30-day session lifetime is reasonable for this use case.
86-101: The response structure does return complete user information, but this concern cannot be fully verified. The code snippet shows the final state of lines 86–101 without context for what changed. Since no internal code in the repository consumes this endpoint, there are no internal clients requiring updates. If this endpoint serves external consumers, coordinate the API change with them separately, but it's not addressed within this codebase.
| export function getSession(cookies: Cookies): SessionData | null { | ||
| try { | ||
| const sessionCookie = cookies.get('session'); | ||
| if (!sessionCookie) return null; | ||
| const session = JSON.parse(sessionCookie) as SessionData; | ||
| // Basic validation | ||
| if (!session.userId || !session.handle || !session.verifiedAt) return null; | ||
| return session; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
CRITICAL: Unsigned session cookies enable authentication bypass.
The session data is stored as plain JSON without any signing or encryption. An attacker can:
- Read their session cookie
- Modify the
userIdorhandlefields - Impersonate any user
This is a severe authentication vulnerability that allows complete account takeover.
Recommended solutions:
Option 1: Use signed cookies with SvelteKit's built-in signing
export function getSession(cookies: Cookies): SessionData | null {
try {
const sessionCookie = cookies.get('session', { signed: true });
if (!sessionCookie) return null;
const session = JSON.parse(sessionCookie) as SessionData;
if (!session.userId || !session.handle || !session.verifiedAt) return null;
return session;
} catch {
return null;
}
}Then in verify-challenge/+server.ts, set the cookie with signing:
cookies.set('session', JSON.stringify(sessionData), {
path: '/',
httpOnly: true,
secure: process.env.NODE_ENV === 'production',
sameSite: 'lax',
maxAge: 60 * 60 * 24 * 30,
signed: true // Add this
});Option 2: Use JWT tokens (as mentioned in the verify-challenge comment at line 71)
Option 3: Migrate to Supabase Auth for full session management
Note: If using SvelteKit's signed cookies, ensure you have a strong secret configured in your environment.
🤖 Prompt for AI Agents
In src/lib/auth/session.ts around lines 14 to 25, the session cookie is parsed
from plain JSON which allows tampering; update the code to read a signed cookie
(use cookies.get('session', { signed: true })) and ensure the cookie is set
elsewhere with signing enabled (cookies.set must include signed: true plus
httpOnly, secure, sameSite, and maxAge), or replace this scheme with JWTs or an
external auth provider; also verify a strong cookie secret is configured in your
environment.
There was a problem hiding this comment.
@CodeRabbit ai - what are the pros and cons of the three options?
There was a problem hiding this comment.
@smb060606 Great question! Here's a detailed comparison of the three authentication approaches:
Option 1: SvelteKit Signed Cookies
Pros:
- Minimal code changes - Just add
signed: trueto your existing cookie operations - Built-in to SvelteKit - No additional dependencies needed
- Lightweight - No external auth service or token parsing overhead
- Server-side control - Session data stored server-side, cookies just contain signed data
- Quick to implement - Could fix the vulnerability in minutes
Cons:
- Requires strong secret management - Must configure and protect a cookie signing secret
- Limited scalability - Cookie size constraints (typically 4KB limit)
- Session invalidation complexity - No built-in way to revoke sessions without additional infrastructure
- Multi-server challenges - All servers need the same secret; rotation is tricky
- Basic security - Signing prevents tampering but cookies are still readable (not encrypted)
Best for:
Single-server or small-scale applications needing a quick security fix.
Option 2: JWT Tokens
Pros:
- Stateless - No server-side session storage needed
- Scalable - Works seamlessly across multiple servers
- Industry standard - Well-understood, many libraries available
- Flexible - Can include custom claims, set expiration, support refresh tokens
- Revocation possible - With short expiry + refresh token pattern or token blacklist
Cons:
- More complex - Requires JWT library, token generation/validation logic, refresh token flow
- Token size - JWTs are larger than simple session IDs, increasing bandwidth
- Cannot revoke easily - Once issued, a JWT is valid until expiration (unless using blacklist)
- More code to maintain - Need to implement token refresh, rotation, storage patterns
- Clock synchronization - Expiry validation requires server time accuracy
Best for:
Multi-server deployments, API-first architectures, or when you need fine-grained control over authentication.
Option 3: Supabase Auth
Pros:
- Fully managed - Supabase handles sessions, tokens, refresh, security best practices
- Feature-rich - Built-in OAuth providers, magic links, password reset, email verification
- Secure by default - Industry-standard JWT implementation with automatic refresh
- Row-level security integration - Native integration with Supabase RLS policies
- Less code - Offload authentication complexity to a battle-tested service
- Multi-device support - Built-in session management across devices
Cons:
- Significant refactoring - Would require rewriting your auth flow and data structures
- Lock-in - Creates dependency on Supabase Auth service
- Loss of custom flow - Your current Bluesky verification flow would need adaptation
- Learning curve - Team needs to learn Supabase Auth patterns and APIs
- Overkill? - May be more than needed if you just want Bluesky-only auth
Best for:
Greenfield projects, apps needing multiple auth providers, or when you want production-grade auth without maintaining it.
My Recommendation:
Given your current codebase and Bluesky-specific auth flow:
Short-term (immediate fix): Use Option 1 (signed cookies) to quickly patch the security vulnerability. It's a 10-minute fix that makes your current system secure.
Long-term (if scaling): Consider Option 2 (JWTs) if you expect to scale horizontally or build APIs. Consider Option 3 (Supabase Auth) if you want to add other auth providers (email, OAuth) or need enterprise-grade features.
For a simple Bluesky-authenticated app with one or two servers, signed cookies are perfectly adequate and maintainable.
Would you like me to help implement any of these options?
| // Basic handle validation | ||
| if (!handle.includes('.') && !handle.includes('@')) { | ||
| return new Response( | ||
| JSON.stringify({ | ||
| error: 'invalid_handle', | ||
| message: 'Invalid Bluesky handle format. Handles should be in the format "example.bsky.social" or "@example.bsky.social".' | ||
| }), | ||
| { | ||
| status: 400, | ||
| headers: { 'Content-Type': 'application/json' } | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Strengthen handle validation to match Bluesky handle format.
The current validation only checks for the presence of '.' or '@', which is too permissive and would accept invalid handles like "a.b", "@x", or handles with special characters. Consider using a more robust validation pattern.
Apply this diff to add more thorough validation:
- // Basic handle validation
- if (!handle.includes('.') && !handle.includes('@')) {
+ // Validate handle format (domain-like structure or @-prefixed)
+ const handlePattern = /^@?[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)+$/;
+ if (!handlePattern.test(handle)) {
return new Response(
JSON.stringify({
error: 'invalid_handle',
- message: 'Invalid Bluesky handle format. Handles should be in the format "example.bsky.social" or "@example.bsky.social".'
+ message: 'Invalid Bluesky handle format. Handles must be valid domain names (e.g., "example.bsky.social") or prefixed with @ (e.g., "@example.bsky.social").'
}),🤖 Prompt for AI Agents
In src/routes/api/auth/bsky/create-challenge/+server.ts around lines 22-34, the
current check only looks for '.' or '@' and is too permissive; replace it with
stricter validation that (1) trims the input and optionally strips a leading
'@', (2) ensures the local username part is 2–64 characters long and contains
only letters, digits, underscores or hyphens, (3) ensures the host part contains
at least two dot-separated labels, each label only letters/digits/hyphens and
the final label is at least 2 characters long, and (4) returns the same 400 JSON
response when validation fails; implement this using a single
regular-expression-based test or equivalent segmented checks and normalize the
handle (e.g., toLowerCase) before further processing.
| } else { | ||
| // Fallback to anon client | ||
| const client = getSupabaseClient(); | ||
| if (!client) { | ||
| health.services.database = 'down'; | ||
| health.status = 'degraded'; | ||
| hasErrors = true; | ||
| } |
There was a problem hiding this comment.
Anon client fallback doesn't verify database connectivity.
When admin client is unavailable, the code only checks if getSupabaseClient() returns non-null but doesn't actually query the database to verify connectivity. This could report ok status when the database is actually unreachable.
} else {
// Fallback to anon client
const client = getSupabaseClient();
if (!client) {
health.services.database = 'down';
health.status = 'degraded';
hasErrors = true;
+ } else {
+ // Verify connectivity with anon client
+ const { error } = await client.from('users').select('id').limit(1);
+ if (error) {
+ health.services.database = 'degraded';
+ hasErrors = true;
+ }
}
}📝 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.
| } else { | |
| // Fallback to anon client | |
| const client = getSupabaseClient(); | |
| if (!client) { | |
| health.services.database = 'down'; | |
| health.status = 'degraded'; | |
| hasErrors = true; | |
| } | |
| } else { | |
| // Fallback to anon client | |
| const client = getSupabaseClient(); | |
| if (!client) { | |
| health.services.database = 'down'; | |
| health.status = 'degraded'; | |
| hasErrors = true; | |
| } else { | |
| // Verify connectivity with anon client | |
| const { error } = await client.from('users').select('id').limit(1); | |
| if (error) { | |
| health.services.database = 'degraded'; | |
| hasErrors = true; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/routes/api/health/+server.ts around lines 41 to 48, the anon Supabase
client fallback only checks for a non-null client but does not verify actual DB
connectivity; update this branch to perform a lightweight database call (e.g.,
run a simple SELECT 1 or equivalent minimal query via the Supabase client) and
treat any failure or timeout as the DB being down: set health.services.database
= 'down', health.status = 'degraded', and hasErrors = true when the query fails
or throws; otherwise leave the database status as healthy.
| await (agent as any).getProfile?.({ actor: 'bsky.app' }).catch(() => { | ||
| throw new Error('Bluesky unreachable'); | ||
| }); |
There was a problem hiding this comment.
Unsafe optional chaining on dynamically resolved method.
Using (agent as any).getProfile?.({ actor: 'bsky.app' }).catch(...) relies on the method existing. If getProfile is undefined, the .catch() won't execute, and the code silently succeeds. Consider using a more explicit check.
- await (agent as any).getProfile?.({ actor: 'bsky.app' }).catch(() => {
- throw new Error('Bluesky unreachable');
- });
+ const getProfile = (agent as any).getProfile;
+ if (typeof getProfile !== 'function') {
+ throw new Error('getProfile method not available');
+ }
+ await getProfile.call(agent, { actor: 'bsky.app' });
health.services.bluesky = 'ok';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/routes/api/health/+server.ts around lines 65 to 67, the current use of
optional chaining with (agent as any).getProfile?.(...).catch(...) is unsafe
because if getProfile is undefined the .catch will never run and the health
check will silently pass; instead, explicitly verify that getProfile exists and
is a function, and if not throw an error, then invoke it inside a try/catch (or
await its Promise) so any rejection is caught and mapped to the 'Bluesky
unreachable' error; ensure you do not rely on optional chaining on the call
expression but perform a clear runtime check before calling.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.