Conversation
Adds ChittyID single sign-on alongside existing email/password auth. Backend: - New /api/auth/chittyid/authorize + /callback routes (OAuth 2.0 + PKCE S256) - getUserByChittyId + linkChittyId storage methods for user resolution - PKCE verifier stored in KV with 10-min TTL - Auto-links ChittyID to existing users matched by email - Session response now includes chittyId field - CHITTYAUTH_CLIENT_ID + CHITTYAUTH_CLIENT_SECRET env bindings Frontend: - "Sign in with ChittyID" button on Login page (above email/password form) - OAuth error codes mapped to user-friendly messages - URL param error display for callback failures Activation requires: 1. Register OAuth client with ChittyAuth (POST /v1/oauth/register) 2. Set CHITTYAUTH_CLIENT_ID + CHITTYAUTH_CLIENT_SECRET as Worker secrets Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
chittyfinance | bf931e9 | Mar 28 2026, 05:06 AM |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review Please evaluate:
|
📝 WalkthroughWalkthroughThis pull request introduces ChittyID OAuth authentication support. The changes include client-side UI updates to the login page offering ChittyID sign-in, a complete server-side OAuth authorize and callback flow with PKCE and state validation, new database methods for user lookup and linking by ChittyID, environment configuration for OAuth credentials, and session response updates including the chittyId field. Changes
Sequence DiagramsequenceDiagram
participant User as User/Browser
participant Client as Login Page
participant Backend as Backend Server
participant KV as KV Store
participant ChittyID as ChittyID OAuth
participant DB as Database
User->>Client: Click "Sign in with ChittyID"
Client->>Backend: GET /api/auth/chittyid/authorize
Backend->>Backend: Generate PKCE verifier/challenge
Backend->>Backend: Generate OAuth state token
Backend->>KV: Store PKCE verifier (10 min TTL)
Backend->>User: Redirect to ChittyID auth endpoint
User->>ChittyID: Authenticate & authorize
ChittyID->>User: Redirect to callback with code & state
User->>Backend: GET /api/auth/chittyid/callback?code=...&state=...
Backend->>Backend: Validate state token
Backend->>KV: Retrieve PKCE verifier
Backend->>ChittyID: Exchange code for tokens (PKCE)
ChittyID->>Backend: Return access token + identity data
Backend->>DB: Lookup user by chittyId
alt User found
Backend->>DB: Retrieve user
else User not found by chittyId
Backend->>DB: Lookup user by email
alt Email match found
Backend->>DB: Link chittyId to existing user
else No match
Backend->>DB: Create new user with chittyId
end
end
Backend->>KV: Store session (SESSION_TTL)
Backend->>User: Set session cookie & redirect to /
User->>Client: Redirected to home
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Code Review — ChittyID SSO (OAuth 2.0 PKCE)Overall this is a clean implementation. PKCE is the right choice here, the KV-backed verifier storage is appropriate for Workers, and the graceful 503 degradation is good practice. A few things worth addressing before merge: Bugs / Correctness1. // server/routes/chittyid-auth.ts:100
const clientId = c.env.CHITTYAUTH_CLIENT_ID!;The callback route can be hit directly (e.g. by a crafted request) without passing through const clientId = c.env.CHITTYAUTH_CLIENT_ID;
if (!clientId) return c.redirect('/login?error=server_config');2. Identity extracted from token response body — non-standard // lines 126–127
const chittyId = tokenData.chitty_id || tokenData.sub;
const email = tokenData.email;Standard OAuth 2.0 token endpoints return 3. Two error codes not mapped in // Login.tsx — missing entries
return c.redirect('/login?error=missing_params'); // not in map → shows raw string
return c.redirect('/login?error=server_config'); // not in map → shows raw stringEither add them to Security4. OAuth provider error codes forwarded verbatim to users // lines 70–72
if (error) {
return c.redirect(`/login?error=${encodeURIComponent(error)}`);
}An error string returned by ChittyAuth (or a malicious redirect) is displayed directly if it isn't in Schema Gap5. Minor6. 7. Token not persisted Summary
Items 1, 2, and 5 should be resolved before merge. The rest are low-risk but easy to address. |
There was a problem hiding this comment.
Pull request overview
Adds ChittyID single sign-on (OAuth 2.0 Authorization Code + PKCE) to ChittyFinance, including backend routes to initiate/complete the OAuth flow and frontend login UI updates to expose “Sign in with ChittyID”.
Changes:
- Introduces
/api/auth/chittyid/authorizeand/api/auth/chittyid/callbackroutes implementing PKCE + session creation. - Adds storage helpers to resolve/link users by
chittyId, and includeschittyIdin session responses. - Updates the login page to add a ChittyID sign-in button and improved error rendering.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| server/storage/system.ts | Adds getUserByChittyId and linkChittyId helpers for account linking. |
| server/routes/session.ts | Includes chittyId in session JSON responses. |
| server/routes/chittyid-auth.ts | New public OAuth PKCE authorize/callback flow; creates KV-backed sessions. |
| server/env.ts | Adds ChittyID OAuth env bindings (CHITTYAUTH_CLIENT_ID/SECRET). |
| server/app.ts | Mounts the new ChittyID auth routes. |
| client/src/pages/Login.tsx | Adds “Sign in with ChittyID” entry point and error-code mapping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
server/storage/system.ts (1)
195-201: Consider settingupdatedAtfor consistency with other update methods.All other
update*methods in this class setupdatedAt: new Date(). This omission could lead to staleupdatedAtvalues when ChittyID linking occurs.♻️ Suggested fix
async linkChittyId(userId: string, chittyId: string) { const [row] = await this.db.update(schema.users) - .set({ chittyId }) + .set({ chittyId, updatedAt: new Date() }) .where(eq(schema.users.id, userId)) .returning(); return row; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/storage/system.ts` around lines 195 - 201, The linkChittyId method omits updating updatedAt, causing inconsistent timestamps; modify linkChittyId to include updatedAt: new Date() in the .set(...) call (i.e., set both { chittyId, updatedAt: new Date() }) when performing the this.db.update(schema.users) so it matches other update* methods and returns the updated row as before.client/src/pages/Login.tsx (2)
5-5: Unused import:setLocationis destructured but never used.The component uses
window.location.href = "/"for navigation instead ofsetLocation.♻️ Suggested fix
-import { useLocation } from "wouter"; +// Remove if setLocation is truly unused, or replace window.location.href with setLocation("/")If programmatic navigation via Wouter is preferred:
- window.location.href = "/"; + setLocation("/");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/Login.tsx` at line 5, The import and destructure from useLocation includes setLocation but it's unused while the component navigates with window.location.href = "/", so either remove setLocation from the useLocation destructure or replace direct window.location.href navigation with programmatic Wouter navigation by calling setLocation("/") where the redirect happens (update the Login component to use the setLocation symbol from useLocation instead of window.location.href, or simply remove setLocation if you prefer native navigation).
22-26: Consider defaulting unmapped error codes to a generic message.Line 25 falls back to displaying the raw error code (
err) if it's not inERROR_MESSAGES. This could expose internal error identifiers to users.♻️ Suggested fix
const [error, setError] = useState(() => { const params = new URLSearchParams(window.location.search); const err = params.get("error"); - return err ? (ERROR_MESSAGES[err] || err) : ""; + return err ? (ERROR_MESSAGES[err] || "An error occurred — try again") : ""; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/Login.tsx` around lines 22 - 26, The initializer for the error state currently returns the raw error code when it's not found in ERROR_MESSAGES (useState -> error/setError), which can leak internal identifiers; change the logic so when err exists but ERROR_MESSAGES[err] is undefined you return a generic user-facing message (e.g., ERROR_MESSAGES.UNKNOWN or "An unexpected error occurred") instead of err; update the initializer that uses URLSearchParams(window.location.search) to prefer mapped messages and fallback to the generic constant, and optionally add a DEFAULT_ERROR/UNKNOWN key to ERROR_MESSAGES for consistency.server/routes/chittyid-auth.ts (1)
11-15: DuplicategenerateSessionIdfunction exists insession.ts.This function is identical to the one in
server/routes/session.ts. Consider extracting it to a shared location likeserver/lib/session.ts.♻️ Suggested refactor
In
server/lib/session.ts:export function generateSessionId(): string { const bytes = new Uint8Array(32); crypto.getRandomValues(bytes); return Array.from(bytes, (b) => b.toString(16).padStart(2, '0')).join(''); }Then import and use in both
session.tsandchittyid-auth.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/chittyid-auth.ts` around lines 11 - 15, The duplicate generateSessionId function should be extracted to a shared module and reused: move the implementation of generateSessionId() into a single exported function in a common module, export it, then replace the local implementations in both session.ts and chittyid-auth.ts with imports that call the shared generateSessionId; ensure the signature and behavior remain identical and update imports where the function was used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/chittyid-auth.ts`:
- Around line 97-98: Remove the non-null assertion on c.env.CHITTYAUTH_CLIENT_ID
and add a guard that validates both CHITTYAUTH_CLIENT_ID (and optionally
CHITTYAUTH_CLIENT_SECRET) before using them; specifically, replace the forced
access to clientId with a safe check (e.g., if (!c.env.CHITTYAUTH_CLIENT_ID) {
log error and return an appropriate HTTP error/redirect) and only proceed to use
clientId and clientSecret after they've been validated, updating any code paths
that assume clientId is present (the variables clientId and clientSecret in
server/routes/chittyid-auth.ts) to handle the missing-config case gracefully.
- Around line 147-153: The current flow that calls storage.getUserByEmail(...)
then storage.linkChittyId(user.id, chittyId) can race: another request may
already link the same chittyId and cause linkChittyId to throw a
unique-constraint DB error; modify the code around the fallback block so
linkChittyId is wrapped in a try/catch that detects the unique constraint /
conflict on chittyId and handles it gracefully (e.g., treat as a controlled
failure path that redirects back to login with an error key like
"chittyid_conflict" rather than letting the exception bubble to a 500). Update
the client error mapping by adding chittyid_conflict to ERROR_MESSAGES in
Login.tsx so the frontend shows "This ChittyID is already linked to another
account".
- Around line 116-129: The fetch to exchange the token can hang because it has
no timeout; update the POST to `${CHITTYAUTH_BASE}/v1/oauth/token` to include an
AbortSignal timeout (e.g. add signal:
AbortSignal.timeout(TOKEN_FETCH_TIMEOUT_MS) to the fetch options) and define a
sensible TOKEN_FETCH_TIMEOUT_MS constant near the handler; also handle aborts in
the catch block (detect AbortError and redirect with a specific timeout error)
so tokenRes/tokenData handling no longer blocks indefinitely.
---
Nitpick comments:
In `@client/src/pages/Login.tsx`:
- Line 5: The import and destructure from useLocation includes setLocation but
it's unused while the component navigates with window.location.href = "/", so
either remove setLocation from the useLocation destructure or replace direct
window.location.href navigation with programmatic Wouter navigation by calling
setLocation("/") where the redirect happens (update the Login component to use
the setLocation symbol from useLocation instead of window.location.href, or
simply remove setLocation if you prefer native navigation).
- Around line 22-26: The initializer for the error state currently returns the
raw error code when it's not found in ERROR_MESSAGES (useState ->
error/setError), which can leak internal identifiers; change the logic so when
err exists but ERROR_MESSAGES[err] is undefined you return a generic user-facing
message (e.g., ERROR_MESSAGES.UNKNOWN or "An unexpected error occurred") instead
of err; update the initializer that uses URLSearchParams(window.location.search)
to prefer mapped messages and fallback to the generic constant, and optionally
add a DEFAULT_ERROR/UNKNOWN key to ERROR_MESSAGES for consistency.
In `@server/routes/chittyid-auth.ts`:
- Around line 11-15: The duplicate generateSessionId function should be
extracted to a shared module and reused: move the implementation of
generateSessionId() into a single exported function in a common module, export
it, then replace the local implementations in both session.ts and
chittyid-auth.ts with imports that call the shared generateSessionId; ensure the
signature and behavior remain identical and update imports where the function
was used.
In `@server/storage/system.ts`:
- Around line 195-201: The linkChittyId method omits updating updatedAt, causing
inconsistent timestamps; modify linkChittyId to include updatedAt: new Date() in
the .set(...) call (i.e., set both { chittyId, updatedAt: new Date() }) when
performing the this.db.update(schema.users) so it matches other update* methods
and returns the updated row as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 661595ec-5683-480e-8282-5852b549e9c3
📒 Files selected for processing (6)
client/src/pages/Login.tsxserver/app.tsserver/env.tsserver/routes/chittyid-auth.tsserver/routes/session.tsserver/storage/system.ts
Summary
chittyIdfield for frontend identity displayArchitecture
GET /api/auth/chittyid/authorize— redirects toauth.chitty.cc/v1/oauth/authorizewith PKCE challengeGET /api/auth/chittyid/callback— exchanges code for token, resolves/links user, creates KV sessionFINANCE_KVwith 10-min TTL, keyed by CSRF state tokenCHITTYAUTH_CLIENT_IDnot configuredActivation
POST auth.chitty.cc/v1/oauth/registerwrangler secret put CHITTYAUTH_CLIENT_ID+CHITTYAUTH_CLIENT_SECRETTest plan
npm run check)npx vitest run)/api/auth/chittyid/authorizereturns 503 when unconfiguredGenerated with Claude Code
Summary by CodeRabbit
New Features