server: move Firebase auth to server-side endpoints

[bpowers]

Summary

• Migrate Firebase authentication from client-side SDK to server-side endpoints
• Add email/password auth endpoints (/auth/login, /auth/signup, /auth/providers, /auth/reset-password, /auth/logout)
• Implement OAuth handlers for Google and Apple Sign-In with server-side token exchange
• Remove Firebase SDK from client, reducing bundle size (~80-200KB savings)
• Add Firestore-backed OAuth state storage for CSRF protection
• Add cryptographic verification of Apple ID tokens using JWKS
• Add return URL validation to prevent open redirect attacks
• Extend User schema with providerUserId for stable OAuth identity

Test plan

• Run pnpm --filter @system-dynamics/server test to verify all new auth tests pass
• Test email/password signup flow
• Test email/password login flow
• Test password reset flow
• Test Google OAuth flow end-to-end
• Test Apple OAuth flow end-to-end
• Verify client bundle size reduction after build

Generated with Claude Code

[claude]

PR Review: Server-side Firebase Authentication Migration

Summary

This PR successfully migrates Firebase authentication from client-side SDK to server-side endpoints. The implementation is well-structured with excellent test coverage. I've identified several areas for improvement around security, error handling, and code quality.

Code Quality & Architecture ✅

Strengths:

• Clean separation of concerns with dedicated modules for auth handlers, OAuth, token exchange, and URL validation
• Comprehensive test coverage with ~2,100 lines of tests across 6 test files
• Good use of TypeScript types and interfaces
• Proper implementation of CSRF protection via OAuth state management
• Cryptographic verification of Apple ID tokens using JWKS

Suggestions:

• Consider extracting magic numbers to named constants (e.g., JWKS_CACHE_TTL_MS, DEFAULT_TTL_MS)
• The loginUser helper is duplicated in both auth-handlers.ts:40-50 and oauth-handlers.ts:52-62 - consider extracting to a shared utility module

Security Concerns 🔒

Critical Issues:

1. Apple Client Secret Generation (oauth-token-exchange.ts:73-110)

   • Using deprecated Node.js crypto API with manual JWT construction
   • You already have the jose library (v6.1.3) imported - use jose.SignJWT instead for safer, battle-tested JWT generation
   • Current implementation manually concatenates signature components which is error-prone

   // Recommended approach:
   import { SignJWT } from 'jose';
   const privateKey = await jose.importPKCS8(privateKeyPem, 'ES256');
   const jwt = await new SignJWT({ ... }).setProtectedHeader({ ... }).sign(privateKey);

2. Insufficient Rate Limiting

   • The PR adds authentication endpoints but I don't see rate limiting middleware
   • Auth endpoints (/auth/login, /auth/signup, /auth/reset-password) are prime targets for brute force attacks
   • Consider adding rate limiting per IP/email, especially for login and password reset

3. Password Reset Information Disclosure (auth-handlers.ts:185-202)

   • Always returns { success: true } even on error - this is good for preventing email enumeration
   • However, legitimate errors are swallowed silently (line 198-199), making debugging difficult
   • Consider logging errors with appropriate detail while still returning generic success response to client

4. Session Security

   • I don't see explicit session configuration (secure cookies, httpOnly, sameSite attributes) in the diff
   • Verify that session cookies have appropriate security flags set in production

Medium Priority:

5. URL Validation (url-validation.ts)

   • Good basic validation, but consider additional checks:
   • Check for encoded characters that could bypass filters (%2F%2F for //)
   • The newline/carriage return checks (line 27) are good but could be more comprehensive
   • Consider using a URL allowlist pattern if return URLs are limited to specific paths

6. Apple OAuth - User without Email (oauth-handlers.ts:264-294)

   • When Apple user has no email, the code does a full scan with listUsers(1000) (line 267)
   • This is inefficient and won't scale beyond 1000 users
   • Consider using Firebase Admin SDK's getUserByProviderUid() or storing provider UIDs in a queryable index

Performance Considerations ⚡

1. Firebase Admin API Calls (oauth-handlers.ts:131-144, 296-309)

   • Multiple sequential Firebase Admin API calls during OAuth callback
   • getUserByEmail() → createUser() pattern is fine, but consider batch operations if this becomes a bottleneck

2. OAuth State Cleanup

   • OAuth states expire but aren't actively cleaned up from Firestore
   • Consider implementing a background job or TTL index to remove expired state documents
   • Firestore supports TTL policies - utilize them to automatically clean up oauth_state collection

3. JWKS Caching (oauth-token-exchange.ts:140-158)

   • Good implementation with 1-hour cache
   • Consider adding error handling for cache staleness (e.g., retry with fresh JWKS if verification fails)

Potential Bugs 🐛

1. User Creation Race Condition (authn.ts:177-243)

   • In getOrCreateUserFromVerifiedInfo, there's a potential race condition between checking for user existence and creation
   • If two requests for the same new user arrive simultaneously, both could attempt creation
   • The eventual consistency fix (lines 211-235) handles duplicates after the fact, but prevention would be better
   • Consider using Firestore transactions or unique constraints on email/providerUserId

2. Error Type Assertions (oauth-handlers.ts:133, 299)

   • const adminErr = err as { code?: string } is unsafe type assertion
   • If Firebase Admin SDK changes error format, this could fail silently
   • Use type guard: if (err && typeof err === 'object' && 'code' in err && err.code === 'auth/user-not-found')

3. Missing Null Checks

   • authn.ts:131 sets providerUserId to decodedToken.uid which should always exist, but not validated
   • Consider adding explicit validation before use

4. Logout Session Cleanup (auth-handlers.ts:211)

   • Manual session key deletion could miss framework-managed properties
   • Consider using req.session.regenerate() or proper session destroy pattern

Test Coverage 🧪

Excellent test coverage overall, but some gaps:

1. Missing edge case tests:

   • Token expiration scenarios for OAuth state
   • Concurrent user creation (race conditions)
   • JWKS cache expiration and refresh during Apple token verification
   • Network failures during OAuth token exchange

2. Integration tests (auth-integration.test.ts) exist but are limited (108 lines)

   • Consider adding end-to-end flow tests for OAuth
   • Test interaction between state store, token exchange, and user creation

3. Mock quality:

   • Good use of Jest mocks
   • Consider adding some tests with actual Firestore emulator for state store

Best Practices & Recommendations 📋

1. Error Messages

   • User-facing error messages are good (friendly, non-technical)
   • Consider adding error codes for client-side handling
   • Example: { error: "Invalid password", code: "INVALID_PASSWORD" }

2. Logging

   • Good use of Winston logger
   • Consider adding structured logging with correlation IDs for request tracing
   • Add timing logs for OAuth flows to detect performance issues

3. Configuration

   • Apple OAuth config requires teamID, keyID, privateKey (config/default.json:28-33)
   • Document these requirements and setup process
   • Consider validating required config on server startup

4. Client-Side Changes (app/Login.tsx)

   • Well-structured React component
   • Good error handling and user feedback
   • Minor: Consider extracting OAuth redirect URLs to config/constants

5. Backward Compatibility

   • Good that the deprecated /session POST endpoint is maintained (authn.ts:291-293)
   • Consider adding deprecation warning in logs and setting sunset date

Minor Issues & Nitpicks

1. Console.log (authn.ts:266, Login.tsx:122,140,188,230)

   • Replace console.log with proper logger
   • Some are in production code paths

2. Type Safety (firebase-rest-client.ts:80)

   • return data as T - unchecked cast
   • Consider runtime validation for critical response shapes

3. Comments (authn.ts:296)

   • TODO comment about unsetting cookie should be addressed or filed as issue

4. Bundle Size Claims

   • PR description claims 80-200KB savings - verify this claim with actual build size measurements before/after

Recommendations Summary

Before Merge:

1. ⚠️ Fix Apple client secret generation to use jose.SignJWT
2. ⚠️ Add rate limiting to auth endpoints
3. ⚠️ Fix type assertions to use proper type guards
4. Replace console.log with logger

Post-Merge:

1. Implement OAuth state cleanup (Firestore TTL or background job)
2. Add request correlation IDs for better debugging
3. Consider extracting duplicate loginUser helper
4. Optimize Apple OAuth user lookup (avoid listUsers scan)
5. Add end-to-end integration tests for complete OAuth flows
6. Document Apple OAuth setup requirements

Conclusion

This is a solid PR that achieves its goals well. The migration to server-side auth is well-architected with good test coverage. The security issues identified are addressable, and most are standard concerns for authentication systems. With the recommended fixes, particularly around the Apple JWT generation and rate limiting, this will be production-ready.

Great work on comprehensive testing and clean separation of concerns! 🎉

[bpowers]

@codex review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.39%. Comparing base (007d3fb) to head (fec5fc6).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #232   +/-   ##
=======================================
  Coverage   76.39%   76.39%           
=======================================
  Files         143      143           
  Lines       36233    36233           
=======================================
+ Hits        27680    27682    +2     
+ Misses       8553     8551    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 11bbdf8823

ℹ️ 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".

[chatgpt-codex-connector]

Generate a valid ES256 signature for Apple client secret

The Apple client secret generator treats crypto.sign() output as a raw 64‑byte r||s signature, but Node returns a DER‑encoded ECDSA signature by default. Slicing the DER blob into 32‑byte chunks produces an invalid JWS signature, so the resulting client secret will be rejected during Apple token exchange (Apple OAuth fails even with correct keys). Use sign.sign({ key: privateKeyObj, dsaEncoding: 'ieee-p1363' }) or a JWS helper (e.g., jose.SignJWT) to produce a proper ES256 signature.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[claude]

Code Review - PR #232: Server-side Firebase Auth Migration

Summary

This PR successfully migrates Firebase authentication from client-side SDK to server-side endpoints. The implementation is generally well-structured with comprehensive test coverage. Below are my findings organized by category.

---

🟢 Strengths

1. Security Improvements

• CSRF Protection: Proper OAuth state management with Firestore-backed storage and 10-minute TTL
• Open Redirect Prevention: Strong URL validation in url-validation.ts prevents common redirect attacks
• Token Verification: Proper JWT verification for Apple Sign-In using JWKS with caching
• Input Sanitization: All user inputs are trimmed and validated

2. Code Quality

• Test Coverage: Excellent test coverage with 2,215 lines of tests across multiple files
• Error Handling: Proper error mapping from Firebase error codes to HTTP status codes
• Separation of Concerns: Well-organized modules (auth handlers, OAuth handlers, token exchange, etc.)
• Type Safety: Good TypeScript typing throughout

3. Architecture

• Bundle Size Reduction: Removing Firebase SDK from client saves ~80-200KB
• Dependency Injection: Clean dependency injection pattern makes code testable
• Backwards Compatibility: Kept /session endpoint for mobile app compatibility

---

🟡 Issues & Recommendations

High Priority

1. Security: require('crypto') usage in oauth-token-exchange.ts:79

File: src/server/auth/oauth-token-exchange.ts:79

Issue: Using require() instead of ES6 imports is inconsistent and could mask import issues.

Current code:

const crypto = require('crypto');

Recommendation: Change to:

import { createPrivateKey, sign } from 'crypto';

This also provides better tree-shaking and type safety.

---

2. Race Condition: Apple OAuth User Lookup by sub

File: src/server/auth/oauth-handlers.ts:267-277

Issue: When Apple doesn't provide an email, the code searches all users by iterating through listUsers(1000). This has several problems:

• Won't scale beyond 1000 users
• Inefficient O(n) search through all users
• No proper pagination handling

Current code:

const listResult = await deps.firebaseAdmin.listUsers(1000);
for (const user of listResult.users) {
  const appleProviderData = user.providerData.find((p) => p.providerId === 'apple.com');
  if (appleProviderData && appleProviderData.uid === claims.sub) {
    fbUser = user;
    break;
  }
}

Recommendation:

1. Use the existing users.findOneByScan({ providerUserId: claims.sub }) which is what you do on line 285
2. Move this check earlier before the Firebase lookup
3. Remove the inefficient listUsers iteration entirely

Suggested fix:

if (!email) {
  // Try to find user by providerUserId first
  const existingUser = await deps.users.findOneByScan({ providerUserId: claims.sub });
  if (existingUser) {
    await loginUser(req, existingUser);
    res.redirect(returnUrl);
    return;
  }
  
  logger.error('Apple user has no email and could not be found by sub');
  res.redirect('/?error=apple_no_email');
  return;
}

---

3. Error Handling: Password Reset Always Returns Success

File: src/server/auth/auth-handlers.ts:198-200

Issue: The password reset handler returns success even on error. While this prevents email enumeration attacks, it should at least log unexpected errors.

Current code:

} catch (err) {
  logger.error('Error sending password reset email:', err);
  res.status(200).json({ success: true }); // Returns 200 even on error
}

Recommendation: This is actually correct behavior for security (prevents email enumeration), but consider adding a comment explaining why errors return success:

} catch (err) {
  // Return success even on error to prevent email enumeration attacks
  logger.error('Error sending password reset email:', err);
  res.status(200).json({ success: true });
}

---

Medium Priority

4. Code Duplication: loginUser Function

Files:

• src/server/auth/auth-handlers.ts:40-50
• src/server/auth/oauth-handlers.ts:52-62

Issue: The loginUser helper function is duplicated in two files.

Recommendation: Extract to a shared utilities file (e.g., src/server/auth/utils.ts)

---

5. Console.log Usage in Production Code

File: src/server/authn.ts:266

Issue: Using console.log instead of the logger:

console.log(`serialize user: ${user.getId()}`);

Recommendation: Change to:

logger.debug(`serialize user: ${user.getId()}`);

---

6. JWKS Cache Management

File: src/server/auth/oauth-token-exchange.ts:140-142

Issue: The JWKS cache uses module-level state which could cause issues in testing and doesn't handle concurrent requests well.

Current code:

let cachedJwks: jose.JSONWebKeySet | undefined;
let jwksCacheTime = 0;
const JWKS_CACHE_TTL_MS = 60 * 60 * 1000; // 1 hour

Recommendation: Consider using a proper cache library like node-cache or at minimum add a mutex to prevent cache stampede during concurrent requests.

---

7. Type Safety: Any Types in Error Handling

File: src/server/auth/oauth-handlers.ts:133, 299

Issue: Using type assertions for Firebase Admin errors:

const adminErr = err as { code?: string };

Recommendation: Create a proper type guard:

function isFirebaseError(err: unknown): err is { code: string } {
  return typeof err === 'object' && err !== null && 'code' in err;
}

---

Low Priority

8. Magic Numbers

File: src/server/auth/oauth-token-exchange.ts:81

const expiresIn = 15777000; // ~6 months

Recommendation: Use a named constant:

const APPLE_CLIENT_SECRET_EXPIRY_SECONDS = 6 * 30 * 24 * 60 * 60; // 6 months

---

9. Empty Catch Blocks

Files:

• src/server/auth/oauth-handlers.ts:257 - Apple user JSON parsing
• src/server/auth/oauth-handlers.ts:165, 329 - State invalidation

Current code:

} catch {
  // Ignore JSON parse errors
}

Recommendation: At minimum log the error:

} catch (err) {
  logger.debug('Failed to parse Apple user JSON', err);
}

---

10. URL Validation Edge Case

File: src/server/auth/url-validation.ts:27-30

Issue: The newline/carriage return check is only on relative URLs but should also apply to absolute URLs.

Recommendation: Move this check earlier or apply to both paths.

---

🔵 Test Coverage Assessment

Excellent Coverage:

• ✅ Auth handlers (897 lines of tests)
• ✅ OAuth handlers (520 lines of tests)
• ✅ Firebase REST client (281 lines of tests)
• ✅ URL validation (85 lines of tests)
• ✅ OAuth state management (145 lines of tests)

Could Use More Tests:

• ⚠️ Integration tests (only 108 lines) - could use more end-to-end scenarios
• ⚠️ Error recovery paths (e.g., Firebase down, network timeouts)
• ⚠️ Concurrent request handling
• ⚠️ JWKS cache invalidation edge cases

---

🟣 Performance Considerations

1. OAuth State Cleanup: The Firestore collection oauth_state will accumulate expired records. Consider adding a cleanup job or TTL index.

2. JWKS Fetching: The cache prevents excessive requests to Apple's JWKS endpoint, but consider handling the case where multiple requests arrive during initial fetch.

3. User Lookups: Multiple database scans in getOrCreateUserFromVerifiedInfo could be optimized with a composite index on providerUserId and email.

---

🔴 Missing Elements

1. Rate Limiting: No rate limiting on auth endpoints (especially important for /auth/login to prevent brute force attacks)

2. Metrics/Monitoring: No instrumentation for auth success/failure rates, which would be valuable for detecting attacks or issues

3. Session Security: No mention of secure cookie configuration (httpOnly, secure, sameSite flags)

---

✅ Checklist Before Merge

• Add import statement instead of require('crypto')
• Fix Apple OAuth user lookup inefficiency
• Add comment explaining password reset error handling
• Replace console.log with logger.debug
• Consider extracting shared loginUser function
• Add cleanup job for expired OAuth states OR configure Firestore TTL
• Verify secure cookie configuration for sessions
• Consider adding rate limiting middleware

---

Overall Assessment

This is a well-implemented PR that successfully achieves its goals of migrating auth to server-side while improving security and reducing bundle size. The test coverage is excellent, and the architecture is clean. The issues identified are mostly minor improvements, with the Apple OAuth user lookup being the most critical to address before merge.

Recommendation: Approve with requested changes for the high-priority items.

[bpowers]

@codex review

[bpowers]

@codex review

[claude]

Code Review Summary

This is an excellent migration that moves Firebase authentication from client-side to server-side endpoints. The implementation is comprehensive with strong test coverage (2,134 lines of tests added). Here's my detailed feedback:

✅ Strengths

1. Security improvements: Proper CSRF protection via OAuth state storage, return URL validation to prevent open redirect attacks, and cryptographic verification of Apple ID tokens using JWKS
2. Excellent test coverage: Comprehensive unit tests for all auth handlers, OAuth flows, URL validation, and integration tests
3. Clean architecture: Good separation of concerns with dedicated modules for REST client, OAuth handlers, state management, and URL validation
4. Bundle size reduction: Removing Firebase SDK (~80-200KB) significantly improves client bundle size
5. Error handling: Proper error mapping from Firebase error codes to HTTP status codes

🔍 Issues & Recommendations

High Priority

1. Security: Apple OAuth user enumeration vulnerability (oauth-handlers.ts:266-277)

   • When email is not provided, the code lists ALL users (listUsers(1000)) and iterates through them to find matching providerUserId
   • This is a serious performance and privacy issue that could allow user enumeration
   • Fix: Use providerUserId as a direct lookup key in the User table rather than scanning all users
   • Consider adding an index on providerUserId in Firestore for efficient lookups

2. Race condition in OAuth state store (oauth-handlers.ts:118, 224)

   • State is invalidated BEFORE completing the authentication flow
   • If authentication fails after invalidation, the state cannot be retried
   • Fix: Move invalidate(state) to AFTER successful loginUser(req, user) and redirect

3. Duplicate state store instances (auth-router.ts:56, 71)

   • Google and Apple handlers create separate state store instances for the same collection
   • This is wasteful and could cause issues with shared state
   • Fix: Create a single state store instance and share it between both OAuth handlers

Medium Priority

4. Hardcoded require() instead of import (oauth-token-exchange.ts:79)

   • Using require('crypto') instead of ES6 import at the top
   • Fix: Add import { createPrivateKey, sign } from 'crypto'; at the top of the file

5. Inconsistent error handling in password reset (auth-handlers.ts:195-200)

   • Returns success (200) even on error to prevent email enumeration - this is actually GOOD for security
   • However, the comment should explain this is intentional to prevent timing attacks
   • Add comment: // Always return success to prevent email enumeration via timing attacks

6. Missing rate limiting

   • No rate limiting on auth endpoints (/auth/login, /auth/signup, etc.)
   • Firebase has built-in rate limiting, but the REST API might behave differently
   • Recommendation: Add rate limiting middleware to prevent brute force attacks

7. Session fixation concerns (auth-handlers.ts:40-50)

   • req.login() is promisified but session is not regenerated before login
   • Fix: Add session regeneration before req.login():

   await new Promise((resolve, reject) => {
     req.session.regenerate((err) => err ? reject(err) : resolve(undefined));
   });
   await loginUser(req, user);

Low Priority

8. Debug console.log in production code (authn.ts:266)

   • console.log(\serialize user: ${user.getId()}`)` should use winston logger instead
   • Fix: Replace with logger.debug(\serialize user: ${user.getId()}`)`

9. Type safety in error handling (Login.tsx:189-193)

   • Narrowing from unknown to Error could be improved
   • Consider a type guard: function isError(err: unknown): err is Error { return err instanceof Error; }

10. Potential JWKS cache race condition (oauth-token-exchange.ts:140-148)

    • Multiple concurrent requests could all fetch JWKS simultaneously before cache is set
    • Fix: Consider using a promise-based cache or mutex to prevent thundering herd

11. User consistency cleanup logic (authn.ts:142-168)

    • Complex logic to handle duplicate temp users - this suggests an underlying bug in the signup flow
    • The root cause should be investigated and fixed rather than working around it

🧪 Test Quality

The test coverage is excellent with comprehensive scenarios covered:

• Input validation (empty/whitespace trimming)
• Success and error paths
• OAuth state validation
• URL validation (including tricky cases like backslash tricks)
• Firebase error mapping

📊 Performance Considerations

1. Database scans: findOneByScan is used frequently - ensure Firestore indexes exist for email and providerUserId fields
2. Apple user lookup: The listUsers(1000) call (mentioned above) is a serious performance bottleneck

🔒 Security Considerations

Overall security is strong with:

• ✅ CSRF protection via state tokens
• ✅ Open redirect prevention
• ✅ Proper token verification
• ⚠️ Rate limiting should be added
• ⚠️ Session fixation mitigation should be improved
• ⚠️ Apple OAuth user enumeration should be fixed

📝 Recommendations for Follow-up

1. Add rate limiting middleware to all auth endpoints
2. Add Firestore indexes for email and providerUserId fields
3. Investigate and fix the root cause of duplicate temp users
4. Consider adding audit logging for authentication events
5. Add metrics/monitoring for authentication success/failure rates

Conclusion

This is a well-architected PR with strong test coverage and good security practices. The main concerns are the Apple OAuth user enumeration issue and the race condition in state invalidation. Once those are addressed, this will be production-ready.

Great work! 🎉

[bpowers]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: af737742cf

ℹ️ 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".

[chatgpt-codex-connector]

Allow Apple logins when email is missing

When Apple omits the email claim (which is common after the first authorization), this branch aborts unless a Firebase user with providerData for apple.com is found. But users created in this flow are created via firebaseAdmin.createUser without linking the Apple provider, so providerData will not contain apple.com and fbUser stays undefined. That means returning Apple users will be redirected to /?error=apple_no_email even though they already exist in your DB (you can match them by providerUserId). Consider falling back to the local providerUserId lookup even when no Firebase providerData entry is found, or explicitly link the Apple provider in Firebase.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[claude]

Code Review Summary

This is a well-executed migration from client-side Firebase SDK to server-side authentication endpoints. The implementation demonstrates strong security awareness with CSRF protection, URL validation, and proper token verification. However, there are several issues that should be addressed before merging.

---

Critical Issues (Must Fix Before Merge)

1. Hardcoded Firebase API Key

Location: src/server/app.ts:217

The Firebase API key is hardcoded:

const firebaseRestClient = createFirebaseRestClient({
  apiKey: 'AIzaSyConH72HQl9xOtjmYJO9o2kQ9nZZzl96G8',
  emulatorHost: process.env.FIREBASE_AUTH_EMULATOR_HOST,
});

Fix: Move to environment variable or config:

apiKey: process.env.FIREBASE_API_KEY || config.get('authentication.firebase.apiKey'),

2. Incorrect Module Import (crypto)

Location: src/server/auth/oauth-token-exchange.ts:74

Using require() instead of ES6 import is inconsistent with the rest of the codebase and can cause TypeScript issues:

const crypto = require('crypto');

Fix: Add at top of file:

import * as crypto from 'crypto';

3. Missing JWKS Refresh on Verification Failure

Location: src/server/auth/oauth-token-exchange.ts:135-153

The Apple JWKS cache has a 1-hour TTL with no fallback mechanism. If Apple rotates their signing keys, all Sign-In attempts will fail for up to an hour.

Fix: Add a retry mechanism that clears cache and refetches JWKS on verification failure.

---

High Priority Issues

4. URL Validation Missing Newline Checks for Absolute URLs

Location: src/server/auth/url-validation.ts:27

Newline checks only apply to relative paths, not absolute URLs. While modern Express protects against HTTP response splitting, defense-in-depth suggests checking all URL types.

Fix: Perform security checks before branching on URL type:

// Check for dangerous characters first
if (trimmedUrl.includes('\\') || trimmedUrl.includes('\n') || trimmedUrl.includes('\r')) {
  return '/';
}

// Then check URL type
if (trimmedUrl.startsWith('/') && !trimmedUrl.startsWith('//')) {
  return trimmedUrl;
}

5. Code Duplication: loginUser Helper

Location: src/server/auth/auth-handlers.ts:40 and src/server/auth/oauth-handlers.ts:52

The same helper function appears in two files.

Fix: Extract to src/server/auth/utils.ts for reuse.

6. Console.log in Production Code

Location: src/server/authn.ts:266

console.log(`serialize user: ${user.getId()}`);

Fix: Replace with logger.debug() for consistency.

---

Medium Priority Issues

7. Missing Runtime Validation for External API Responses

Location: src/server/auth/oauth-token-exchange.ts:55,70

Type assertions assume external APIs return the expected schema:

return (await response.json()) as TokenResponse;

Recommendation: Add basic field validation or use a schema validation library like Zod.

8. Magic Numbers in Apple Client Secret

Location: src/server/auth/oauth-token-exchange.ts:76

const expiresIn = 15777000; // ~6 months

Fix: Use named constant:

const APPLE_CLIENT_SECRET_EXPIRY_SECONDS = 6 * 30 * 24 * 60 * 60; // 6 months

9. Multiple OAuth State Store Instances

Location: src/server/auth/auth-router.ts:56,71

A separate state store is created for Google and Apple OAuth, but they could share the same Firestore collection.

Fix: Create once and reuse:

const stateStore = deps.firestore ? createFirestoreStateStore(deps.firestore) : null;

if (stateStore && deps.googleConfig) {
  // use stateStore
}

if (stateStore && deps.appleConfig) {
  // use same stateStore
}

---

Low Priority / Nice to Have

10. Type Safety: Passport Callbacks Use any

Location: src/server/authn.ts:264,273

Define proper types for passport serialize/deserialize callbacks.

11. Improve Error Logging Security Comment

Location: src/server/auth/auth-handlers.ts:198-200

The password reset endpoint correctly prevents email enumeration, but the comment could be clearer:

res.status(200).json({ success: true }); // Always return success to prevent user enumeration

---

Test Coverage Observations

Strengths:

• Comprehensive unit tests for all authentication flows
• Excellent edge case coverage (expired states, missing emails, invalid parameters)
• Security-focused tests for CSRF and URL validation
• Apple client secret generation includes cryptographic verification

Gaps:

• No integration tests for full auth flow
• Missing tests for JWKS cache expiration/refresh behavior
• No tests for concurrent OAuth state creation scenarios
• Missing test for race conditions in getOrCreateUserFromVerifiedInfo

---

Overall Assessment

This is solid work with good security practices. The code is well-structured, testable, and the commit messages follow the project conventions excellently. Once the critical issues are addressed (hardcoded API key, crypto import, and JWKS refresh), this will be ready to merge.

Recommendation: Fix critical issues #1-3 before merging. Other improvements can be addressed in follow-up PRs if needed.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 67b46c8708

ℹ️ 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".

[chatgpt-codex-connector]

Persist providerUserId when matching by email

When getOrCreateUserFromVerifiedInfo finds an existing user by email, it returns without updating providerUserId (or provider). This breaks Apple Sign In for users who first match by email: Apple only sends email on the first authorization, so subsequent Apple logins arrive without email and the callback tries to look up by providerUserId, which was never saved. That leads to /auth/apple/callback rejecting returning users with apple_no_email even though the account exists. Consider backfilling providerUserId/provider on the email-matched user so future email-less logins succeed.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[claude]

Pull Request Review: Server-side Firebase Authentication Migration

Overview

This is a substantial and well-architected PR that successfully migrates Firebase authentication from client-side SDK to server-side endpoints. The implementation is comprehensive with excellent test coverage (2,762 additions across 8 new test files). The code quality is high with proper error handling, security considerations, and clean separation of concerns.

Strengths

Architecture & Design

• Clean separation of concerns: Auth handlers, OAuth handlers, token exchange, and state management are properly isolated
• Dependency injection pattern: Handlers use dependency injection making them testable and maintainable
• Comprehensive test coverage: ~2,762 lines of tests covering unit, integration, and edge cases
• Security-first approach: CSRF protection via state tokens, URL validation, cryptographic verification of Apple ID tokens

Security Highlights

• ✅ OAuth state management with expiration (10 min TTL) prevents CSRF attacks
• ✅ Return URL validation prevents open redirect vulnerabilities (url-validation.ts:5-49)
• ✅ Apple ID token verification using JWKS with proper issuer/audience checks (oauth-token-exchange.ts:155-170)
• ✅ Proper error handling that doesn't leak sensitive information
• ✅ Firebase ID token verification on server-side

Code Quality

• Well-structured error handling with custom FirebaseAuthError class
• Proper TypeScript typing throughout
• Good use of interfaces for contracts
• Consistent error response patterns

Issues & Concerns

🔴 Critical: Rate Limiting Missing

Priority: HIGH

The new authentication endpoints lack rate limiting, making them vulnerable to:

• Brute force password attacks on /auth/login
• Account enumeration via /auth/providers
• Email bombing via /auth/reset-password

Recommendation: Implement rate limiting middleware (e.g., express-rate-limit) for all auth endpoints:

import rateLimit from 'express-rate-limit';

const authLimiter = rateLimit({
  windowMs: 15 * 60 * 1000, // 15 minutes
  max: 5, // 5 requests per window for login/signup
  message: 'Too many attempts, please try again later'
});

router.post('/login', authLimiter, createLoginHandler(handlerDeps));
router.post('/signup', authLimiter, createSignupHandler(handlerDeps));

🟡 Security: Information Disclosure in Provider Check

File: src/server/auth/auth-handlers.ts:161-183

The /auth/providers endpoint reveals whether an email is registered, enabling account enumeration. While this is a common tradeoff for better UX, consider:

1. Adding rate limiting (see above)
2. Adding a delay to responses to make enumeration slower
3. Logging suspicious patterns for monitoring

The current implementation is acceptable for internal use but should be documented.

🟡 Bug: Missing Error Handling in Client Code

Files: src/app/Login.tsx:99-105, 134-142, 208-225

Client-side auth calls lack proper error handling for non-200 responses:

// Current - doesn't check response.ok
const response = await fetch('/auth/providers', { ... });
const { providers, registered } = await response.json();

Fix: Add response status checks:

const response = await fetch('/auth/providers', { ... });
if (!response.ok) {
  const { error } = await response.json();
  this.setState({ emailError: error || 'Failed to check email' });
  return;
}
const { providers, registered } = await response.json();

This affects all client auth calls (providers, reset-password, login, signup).

🟡 Code Quality: Hardcoded require() Statement

File: src/server/auth/oauth-token-exchange.ts:74

const crypto = require('crypto');

Issue: Uses CommonJS require() instead of ES6 import in a TypeScript file. Should be:

import { createPrivateKey, sign } from 'crypto';

This is inconsistent with the rest of the codebase and can cause bundling issues.

🟠 Potential Race Condition in JWKS Cache

File: src/server/auth/oauth-token-exchange.ts:135-153

The JWKS cache implementation has a potential race condition:

async function fetchAppleJwks(): Promise<jose.JSONWebKeySet> {
  const now = Date.now();
  if (cachedJwks && now - jwksCacheTime < JWKS_CACHE_TTL_MS) {
    return cachedJwks;  // ← Multiple concurrent requests may bypass cache
  }
  // fetch...
}

If multiple requests arrive simultaneously after cache expiration, they'll all fetch the JWKS. While not critical (just inefficient), consider using a promise-based lock pattern or accepting this as acceptable overhead.

🟠 Database Query Pattern Concerns

File: src/server/authn.ts:188-202

The getOrCreateUserFromVerifiedInfo function performs sequential scans:

if (info.providerUserId) {
  user = await users.findOneByScan({ providerUserId: info.providerUserId });
}
if (!user && info.email) {
  user = await users.findOneByScan({ email: info.email });
  // ...
}

Performance consideration: Two sequential scans on every OAuth login. Consider:

1. Adding indexes on providerUserId and email fields in Firestore
2. Documenting the expected performance characteristics
3. Monitoring query latency in production

The logic correctly handles the Apple email omission case (lines 188-203, oauth-handlers.ts:255-267), which is good.

🟠 Password Reset Information Disclosure

File: src/server/auth/auth-handlers.ts:185-202

The password reset endpoint silently succeeds regardless of whether the email exists:

try {
  await deps.firebaseRestClient.sendPasswordResetEmail(email);
  res.status(200).json({ success: true });
} catch (err) {
  logger.error('Error sending password reset email:', err);
  res.status(200).json({ success: true });  // ← Always succeeds
}

Analysis: This is actually correct behavior to prevent email enumeration. However:

1. The catch block logs errors but returns success even for real errors (network failures, Firebase API issues)
2. Consider different handling for EMAIL_NOT_FOUND (silent success) vs actual errors (maybe retry or alert)

🟢 Minor: Deprecated Endpoint Not Removed

File: src/server/authn.ts:259-273

The old /session endpoint is marked deprecated but still present:

// DEPRECATED: Use /auth/login instead. This endpoint exists for backward
// compatibility with existing mobile apps and will be removed in a future release.

Recommendation:

1. Add a deprecation date/version
2. Add metrics to track usage
3. Set a removal date once mobile apps update

This is fine for now but should have a removal plan.

🟢 Minor: Console.log in Production Code

File: src/app/Login.tsx:122, 141, 200

Multiple console.log(err) statements in error handlers. While not critical, consider:

1. Using a proper logging library
2. Adding user-facing error messages
3. Removing console.logs before production deployment

Testing Observations

Excellent Coverage

• ✅ Unit tests for all auth handlers (897 lines)
• ✅ OAuth flow tests including edge cases (651 lines)
• ✅ URL validation tests (85 lines)
• ✅ Firebase REST client tests (281 lines)
• ✅ Integration tests (108 lines)

Edge Cases Well Covered

• ✅ Apple email omission on subsequent logins (oauth-handlers.test.ts:564-611)
• ✅ Provider user ID migration logic (authn.test.ts:54-122)
• ✅ Expired OAuth state (oauth-state.test.ts)
• ✅ Invalid return URLs (url-validation.test.ts)

Performance Considerations

1. Bundle size reduction: Removes ~80-200KB of Firebase SDK - excellent
2. Server-side token exchange: Adds one additional round trip for OAuth flows - acceptable tradeoff
3. OAuth state storage: Uses Firestore - ensure proper indexes exist
4. JWKS caching: 1-hour TTL is reasonable

Recommendations Summary

Must Fix Before Merge

1. ❗ Add rate limiting to auth endpoints (critical security issue)
2. ❗ Fix client-side error handling for non-200 responses
3. ❗ Replace require('crypto') with ES6 import

Should Fix Soon

4. Document the account enumeration risk in /auth/providers
5. Add Firestore indexes for email and providerUserId fields
6. Improve password reset error handling to distinguish between enumeration-safe errors and real failures

Nice to Have

7. Add deprecation timeline for /session endpoint
8. Replace console.log with proper logging
9. Consider JWKS cache mutex for efficiency

Conclusion

This is high-quality work that successfully achieves its goals. The architecture is solid, security considerations are mostly well-handled, and test coverage is exemplary. The critical issue is the lack of rate limiting on auth endpoints, which should be addressed before merging to production. The other issues are relatively minor and can be addressed in follow-up PRs if needed.

Recommendation: ✅ Approve with required changes (add rate limiting, fix client error handling, fix require() import)

---

Great work overall! The migration is well-designed and the test coverage gives confidence in the implementation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 84e3ef714e

ℹ️ 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".

[chatgpt-codex-connector]

Block OAuth login for disabled Firebase users

The new OAuth callbacks fetch or create a Firebase user but never check whether the fetched user is disabled, then proceed to create a local session. That means a user that has been disabled in Firebase Auth can still sign in via Google/Apple OAuth and get a valid app session, which is a regression from the previous client‑SDK flow that would reject disabled accounts. You likely want to inspect the UserRecord.disabled flag (here and in the Apple handler) and fail the login before loginUser when it’s true.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[claude]

Code Review - PR #232: Server-side Firebase Authentication Migration

This is a significant and well-executed refactoring that moves Firebase authentication from client-side to server-side. The implementation is thorough with excellent test coverage. Here's my detailed review:

✅ Strengths

1. Strong Security Implementation

   • CSRF protection via OAuth state storage in Firestore with proper TTL (10 min)
   • URL validation prevents open redirect attacks (url-validation.ts)
   • Apple ID token cryptographic verification using JWKS
   • Proper handling of sensitive data (passwords, tokens never logged)
   • Client secret generation for Apple using correct ES256 signature encoding

2. Excellent Test Coverage

   • Comprehensive unit tests for all auth handlers (~900 lines in auth-handlers.test.ts)
   • OAuth flow tests covering edge cases (~745 lines in oauth-handlers.test.ts)
   • Integration tests for full auth workflows
   • Tests for error conditions, disabled users, and state validation
   • Token exchange and URL validation thoroughly tested

3. Good Architecture

   • Clean separation of concerns (handlers, token exchange, state management)
   • Dependency injection pattern makes testing easy
   • Proper error handling with typed errors (FirebaseAuthError)
   • Generic OAuthStateStore interface enables different storage backends

4. Bundle Size Reduction

   • Removing Firebase SDK from client (~80-200KB savings) is a significant win
   • Server-side approach enables future migration away from Firebase

🔍 Issues & Concerns

Critical Security Issue

Location: oauth-token-exchange.ts:74 - Dynamic require inside function

export function generateAppleClientSecret(...) {
  const crypto = require('crypto');  // ⚠️ Dynamic require
  // ...
}

Problem: Using require() inside a function is unnecessary and inconsistent with TypeScript/ES6 patterns used elsewhere in the codebase.

Fix: Move to top-level import:

import * as crypto from 'crypto';

Security Consideration: Password Reset Timing Attack

Location: auth-handlers.ts:185-202

export function createResetPasswordHandler(deps: AuthHandlerDeps): RequestHandler {
  // ...
  try {
    await deps.firebaseRestClient.sendPasswordResetEmail(email);
    res.status(200).json({ success: true });
  } catch (err) {
    logger.error('Error sending password reset email:', err);
    res.status(200).json({ success: true });  // Always returns success
  }
}

Analysis: This is actually correct behavior - always returning success prevents email enumeration attacks. However, the implementation in firebase-rest-client.ts:127-139 already handles EMAIL_NOT_FOUND silently, so the outer try/catch may be redundant. Consider adding a comment explaining the security rationale.

Potential Race Condition

Location: authn.ts:197-203

if (user && matchedByEmail && info.providerUserId && user.getProviderUserId() !== info.providerUserId) {
  user.setProviderUserId(info.providerUserId);
  user.setProvider(info.provider);
  await users.update(user.getId(), {}, user);  // ⚠️ Not awaited before reuse
}

Problem: The updated user object is used immediately after this block without ensuring the update completed. If there's an error or race condition, subsequent operations may use stale data.

Recommendation: Ensure proper error handling and consider whether the update should complete before proceeding.

Minor: Inconsistent Error Handling

Location: oauth-handlers.ts:164-168

} catch (err) {
  logger.error('Error in Google OAuth callback:', err);
  await deps.stateStore.invalidate(state).catch(() => {});  // Silently ignore
  res.redirect('/?error=oauth_callback_failed');
}

Issue: State invalidation errors are silently ignored. While this might be intentional (best-effort cleanup), consider logging if invalidation fails to help debug state store issues.

Type Safety Issue

Location: oauth-handlers.ts:198

const { code, state, error, id_token: bodyIdToken, user: appleUserJson } = req.body;

Problem: No runtime validation that req.body fields are the expected types before destructuring. While type checks happen later, malformed data could cause issues.

Recommendation: Consider using a validation library (zod, yup) or add explicit type guards earlier in the handler.

📝 Code Quality Observations

1. Good: Consistent error messages and HTTP status codes
2. Good: Proper use of TypeScript interfaces for configuration
3. Good: JWKS caching with TTL to avoid excessive requests to Apple
4. Minor: Some code duplication between Google and Apple OAuth handlers could be extracted into shared helpers (DRY principle)

🧪 Testing Observations

1. Excellent: Mocking strategy is clean and comprehensive
2. Excellent: Tests verify both success and failure paths
3. Good: Tests for disabled user handling across all auth methods
4. Minor: Consider adding tests for concurrent state validation (race conditions)

🎯 Performance Considerations

1. Good: JWKS caching prevents repeated fetches to Apple's servers
2. Good: State TTL prevents unbounded growth in Firestore
3. Consider: Add indices on providerUserId field in Firestore for efficient lookups (mentioned in authn.ts:262)

📋 Recommendations

Must Fix:

1. Remove dynamic require() in generateAppleClientSecret - use top-level import

Should Consider:

1. Add comment explaining security rationale for password reset always returning success
2. Review race condition in providerUserId update flow
3. Add logging (not error) when state invalidation fails in cleanup
4. Consider request body validation middleware

Nice to Have:

1. Extract common OAuth handler logic to reduce duplication
2. Add Firestore index documentation for providerUserId lookups
3. Consider adding rate limiting for auth endpoints

✨ Overall Assessment

This is a high-quality PR that demonstrates excellent security awareness and thorough testing. The architecture is clean, the code is well-organized, and the migration path is clear. The critical issue (dynamic require) is easy to fix, and the other observations are minor improvements.

Recommendation: Approve with minor changes requested (fix the dynamic require).

---

Review generated by Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a756486e3

ℹ️ 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".

[chatgpt-codex-connector]

Stop trimming passwords before auth calls

Trimming the password changes its value before it is sent to Firebase. That means users whose passwords include leading/trailing spaces (which Firebase treats as significant) will be unable to sign in, and sign-ups will create a different password than the user entered. This is a behavior regression compared to the client SDK flow, and it will surface only for passwords with whitespace at either end.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 19f5cb3b77

ℹ️ 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".

[chatgpt-codex-connector]

Normalize Apple email_verified before use

Apple’s ID token encodes email_verified as the string "true"/"false", but verifyAppleIdToken currently casts it to boolean without converting. This means claims.email_verified can remain a string, and when createAppleOAuthCallbackHandler passes it into firebaseAdmin.createUser (emailVerified: claims.email_verified ?? false), it will either be treated as truthy even when the string is "false" or be rejected by the Admin SDK for a non-boolean value. Either outcome can break Apple sign‑ups or incorrectly mark emails as verified. Consider explicitly mapping string values to booleans (e.g., payload.email_verified === 'true').

Useful? React with 👍 / 👎.

[claude]

Code Review

This is a substantial and well-structured PR that migrates Firebase authentication from client-side to server-side. Overall, the implementation is solid with excellent test coverage. Here are my findings:

✅ Strengths

1. Excellent Test Coverage: Comprehensive tests covering auth handlers, OAuth flows, URL validation, and integration scenarios.

2. Security Best Practices:

   • CSRF protection via Firestore-backed OAuth state storage with 10-minute TTL
   • Strong URL validation preventing open redirects
   • Proper Apple ID token verification using JWKS with 1-hour caching
   • Secure Apple client secret generation using ES256 signatures

3. Proper Error Handling: Auth errors mapped to appropriate HTTP status codes (401, 403, 409, 429, etc.)

4. Bundle Size Reduction: Removing Firebase SDK from client saves 80-200KB - significant improvement.

---

🔴 Critical Issues

1. Security: Password Field Trimming

Location: src/app/Login.tsx:163, 210

Trimming passwords changes the actual password. If a user intentionally includes leading/trailing spaces, they won't be able to log in. Remove .trim() from password handling.

2. Security: Manual JWT Generation

Location: src/server/auth/oauth-token-exchange.ts:73-104

Manual JWT generation is error-prone. Use the jose library (already imported) with jose.SignJWT instead of manual crypto operations.

3. Bug: Type Assertions Without Validation

Locations: oauth-token-exchange.ts:55, 70, 88

Using as type assertions without validation. Add runtime checks before type assertions to prevent runtime errors if APIs change.

---

⚠️ High Priority Issues

4. Race Condition: JWKS Cache

Location: oauth-token-exchange.ts:135-152

Module-level cache has a race condition. Implement in-flight request deduplication to prevent multiple simultaneous fetches.

5. Password Reset Timing Attack

Location: auth-handlers.ts:185-202

While preventing email enumeration by returning 200 for both cases, timing differences may leak information. Consider adding small random delays.

6. User Provider Migration

Location: authn.ts:197-202

Updates provider when logging in with OAuth after password auth, but doesn't handle reverse case or log provider changes for auditing.

---

💡 Medium Priority Issues

7. Inconsistent Error Handling: OAuth redirects vs JSON responses - document the distinction
8. Memory Leak in State Store: Implement Firestore TTL or cleanup job for abandoned OAuth states
9. UX Issue: Add user-friendly error messages for OAuth error codes like apple_no_email
10. Silenced Errors: Log errors in .catch(() => {}) blocks instead of silently swallowing them

---

📝 Minor Issues

11. Use import crypto instead of require('crypto') in oauth-token-exchange.ts:74
12. Remove debug console.log in authn.ts:277
13. Add JSDoc comments for public functions explaining security considerations

---

Summary

This is high-quality work with excellent architecture and test coverage. Address the 🔴 Critical Issues and ⚠️ High Priority items before merge. The security foundations are solid, and the migration strategy is well thought out.

Great job on this migration! 🎉

[bpowers]

@codex review

[claude]

Code Review for PR #232: Server-Side Firebase Authentication Migration

Overview

This is a substantial and well-executed refactoring that migrates Firebase authentication from client-side SDK to server-side endpoints. The implementation is thorough with excellent test coverage (~2,600+ lines of tests across 7 new test files). The commit messages are detailed and follow the project's style guide well.

Strengths

1. Security Improvements ✅

• CSRF Protection: Proper OAuth state management with Firestore-backed storage and expiration (10-min TTL)
• Open Redirect Prevention: Comprehensive URL validation in url-validation.ts checking for dangerous protocols, double slashes, backslashes, and host/protocol matching
• Cryptographic Verification: Proper Apple ID token verification using JWKS with caching
• Password Handling: Correctly preserves password whitespace (auth-handlers.ts:55) - critical detail!
• Disabled Account Checks: Properly blocks disabled Firebase users from OAuth flows

2. Code Quality ✅

• Excellent Test Coverage: Comprehensive unit tests for all major components with proper mocking
• Clean Architecture: Good separation of concerns with dedicated modules for different auth flows
• Error Handling: Appropriate HTTP status codes and user-friendly error messages
• Type Safety: Strong TypeScript typing throughout

3. OAuth Implementation ✅

• Provider User ID Persistence: Smart handling of providerUserId for returning users (authn.ts:197-202)
• Apple's Email Quirks: Correctly handles Apple omitting email on subsequent logins
• Token Exchange: Proper implementation for both Google and Apple flows

Issues & Concerns

1. CRITICAL: Hardcoded Secrets Exposure Risk 🔴

Location: oauth-token-exchange.ts:73-105

The generateAppleClientSecret() function uses require('crypto') at runtime (line 74) instead of importing at the top of the file. While not inherently insecure, this is unconventional.

More importantly: The Apple private key is passed as a parameter and likely comes from config. Ensure:

• Private keys are NEVER committed to the repository
• Keys are loaded from environment variables or secure secret management
• The config/default.json file shows empty strings for sensitive fields (good!), but verify your deployment process

2. Security: JWKS Cache Invalidation 🟡

Location: oauth-token-exchange.ts:135-153

The JWKS cache has a 1-hour TTL but no mechanism for manual invalidation if Apple rotates their keys early. While rare, this could cause verification failures.

Recommendation:

• Consider adding retry logic that clears cache on verification failure
• Or reduce TTL to 15-30 minutes

3. Code Quality: Type Assertions Without Validation 🟡

Location: oauth-token-exchange.ts:55, 70, 132

Multiple type assertions (as TokenResponse, as GoogleUserInfo) without runtime validation:

return (await response.json()) as TokenResponse;

If Google/Apple changes their API response format, this could fail silently or cause runtime errors.

Recommendation: Add Zod or similar runtime validation, or at minimum check for required fields.

4. Potential Race Condition 🟡

Location: authn.ts:145-167

The "eventual consistency" workaround for duplicate temp users is concerning. The code acknowledges this is a band-aid:

// we have some eventual consistency problem where sometimes we don't
// delete the temp user when completing the sign-up flow.

This suggests a deeper issue with the user creation flow that should be properly investigated and fixed rather than worked around.

Recommendation: Root cause analysis - why are temp users not being deleted? Is there a missing transaction or error path?

5. Error Handling: Silent Failures 🟡

Location: auth-handlers.ts:198-200

Password reset always returns success, even on error:

} catch (err) {
  logger.error('Error sending password reset email:', err);
  res.status(200).json({ success: true });  // Returns success even on error!
}

While this prevents email enumeration attacks (good!), the error is only logged. Consider also:

• Alerting on repeated failures (possible Firebase API issues)
• Rate limiting this endpoint

6. Performance: Redundant Firebase Admin Calls 🟡

Location: oauth-handlers.ts:125-140, 275-289

Both Google and Apple OAuth handlers fetch or create the Firebase user, then separately fetch it in getOrCreateUserFromVerifiedInfo. This could be optimized by:

• Passing the Firebase user to the verification function
• Or combining these operations

7. Testing: Missing Integration Test Coverage 🟡

While unit tests are excellent, I don't see end-to-end integration tests for:

• Full OAuth flows (initiate → callback → session creation)
• CSRF token lifecycle
• Error recovery paths

The auth-integration.test.ts file exists but is only 108 lines. Consider expanding.

8. Code Style: Inconsistent Async/Promise Patterns 🟢

Location: auth-handlers.ts:40-49, 204-218

loginUser() and createLogoutHandler() wrap callbacks in Promises manually, while other code uses async/await. Consider promisifying passport methods once at module level for consistency.

9. Minor: Magic Numbers 🟢

Location: oauth-token-exchange.ts:76

const expiresIn = 15777000; // ~6 months

Use a named constant: const APPLE_CLIENT_SECRET_TTL_SECONDS = 180 * 24 * 60 * 60; // 180 days

10. Documentation: Missing JSDoc 🟢

Public functions lack JSDoc comments explaining parameters, return values, and potential errors. This would improve maintainability.

Performance Considerations

• Bundle Size: Achieves stated goal of reducing client bundle by ~80-200KB ✅
• JWKS Caching: Good optimization for Apple token verification ✅
• State Storage: Firestore reads/writes for OAuth state could be expensive at scale - consider Redis for better performance if needed

Test Coverage Analysis

Excellent coverage across:

• ✅ auth-handlers.test.ts (900 lines) - Email/password flows
• ✅ oauth-handlers.test.ts (745 lines) - Google/Apple OAuth
• ✅ firebase-rest-client.test.ts (281 lines) - REST API client
• ✅ oauth-token-exchange.test.ts (194 lines) - Token exchange
• ✅ authn.test.ts (172 lines) - User creation/lookup
• ✅ oauth-state.test.ts (145 lines) - State management
• ✅ url-validation.test.ts (85 lines) - URL validation

Missing tests:

• Error recovery paths (network failures, malformed responses)
• Rate limiting scenarios
• Concurrent OAuth state requests
• Apple's name parsing edge cases

Recommendations Priority

High Priority

1. Verify secret management - Ensure Apple private keys are never in version control
2. Add JWKS cache invalidation on verification failure
3. Investigate temp user race condition - fix root cause

Medium Priority

4. Add runtime validation for external API responses
5. Expand integration test coverage for full OAuth flows
6. Consider rate limiting for password reset endpoint
7. Add monitoring/alerting for auth errors

Low Priority

8. Refactor redundant Firebase Admin calls in OAuth handlers
9. Add JSDoc documentation to public APIs
10. Use named constants instead of magic numbers

Security Checklist

• ✅ CSRF protection via state tokens
• ✅ Open redirect prevention
• ✅ Password whitespace preservation
• ✅ Disabled account checks
• ✅ Cryptographic signature verification
• ⚠️ Secret management (verify deployment config)
• ⚠️ Type safety for external APIs (runtime validation needed)
• ✅ Email enumeration prevention

Conclusion

This is high-quality work that significantly improves the application's architecture and security posture. The migration is well-thought-out with excellent test coverage. The main concerns are around production readiness (secret management verification, external API validation, and investigating the temp user race condition).

Recommendation: Approve with minor revisions to address the high-priority items above.

Great job on the detailed commit messages and comprehensive testing! 🎉

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dd1713c9f7

ℹ️ 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".

[chatgpt-codex-connector]

Handle returning Apple users without email

When Apple omits the email on subsequent sign-ins, this path only attempts a providerUserId lookup and otherwise rejects the login. Existing Apple users created before this change won’t have providerUserId populated, so their first post-migration login will hit this branch and be locked out with apple_no_email. This is a regression for returning Apple users; you likely need a backfill/migration or a fallback path (e.g., link by Firebase user record or allow a one-time email-based match before requiring providerUserId).

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[claude]

Code Review: Server-side Firebase Authentication Migration

This is a substantial and well-structured PR that successfully migrates Firebase authentication from client-side SDK to server-side endpoints. Overall, the code quality is high with comprehensive test coverage. Below are my detailed findings:

---

✅ Strengths

1. Excellent Test Coverage: ~434 test cases across new auth modules demonstrate thorough testing
2. Security-First Design:
   • CSRF protection via OAuth state tokens with Firestore-backed storage
   • Open redirect prevention through URL validation
   • Cryptographic verification of Apple ID tokens using JWKS
   • Proper error handling that doesn't leak sensitive information
3. Clean Architecture: Well-organized separation of concerns across multiple modules
4. Good Error Handling: Custom error types with appropriate HTTP status codes

---

🐛 Potential Issues

1. Race Condition in Apple OAuth - No Email Path (oauth-handlers.ts:260-296)

The Apple OAuth handler has a complex fallback path when email is missing on subsequent logins. There's a potential race condition:

// Line 262: First lookup by providerUserId
let existingUser = await deps.users.findOneByScan({ providerUserId: claims.sub });

// Lines 273-291: If not found, try Firebase lookup and update providerUserId
const fbUser = await deps.firebaseAdmin.getUserByProviderUid('apple.com', claims.sub);
existingUser = await deps.users.findOneByScan({ email: fbUser.email });
if (existingUser) {
  existingUser.setProviderUserId(claims.sub);
  await deps.users.update(existingUser.getId(), {}, existingUser);
}

Problem: Between the update and login, another request could see an inconsistent state. Consider wrapping this in a transaction or using atomic updates.

2. Inconsistent Error Responses (auth-handlers.ts:198-200)

} catch (err) {
  logger.error('Error sending password reset email:', err);
  res.status(200).json({ success: true }); // ⚠️ Returns success even on error
}

Issue: Password reset always returns success (even on errors) to prevent email enumeration attacks. This is intentional for security, but:

• The comment explaining this security decision is missing
• This differs from the Firebase REST client which already suppresses EMAIL_NOT_FOUND (line 134 of firebase-rest-client.ts)

Recommendation: Add a comment explaining why errors are suppressed for security.

3. Hard-coded Crypto Require (oauth-token-exchange.ts:74)

export function generateAppleClientSecret(...): string {
  const crypto = require('crypto'); // ⚠️ Should be at module level
  // ...
}

Issue: require('crypto') is called every time the function executes instead of at module initialization.

Fix: Move to top of file with other imports.

4. Memory Leak Risk in JWKS Cache (oauth-token-exchange.ts:135-153)

let cachedJwks: jose.JSONWebKeySet | undefined;
let jwksCacheTime = 0;
const JWKS_CACHE_TTL_MS = 60 * 60 * 1000; // 1 hour

Issue: Module-level cache with no max size. While Apple's JWKS is small, this pattern could be problematic if extended.

Recommendation: Consider using a proper cache library (like lru-cache) for production readiness, especially if this pattern is replicated for other providers.

5. Weak Type Safety in Firebase REST Client (firebase-rest-client.ts:88)

return data as T;

Issue: Type assertion without runtime validation. If Firebase changes response format, this will fail silently.

Recommendation: Add runtime validation using a library like zod or at least validate required fields exist.

---

⚠️ Security Concerns

1. URL Validation Edge Cases (url-validation.ts)

The URL validator is good but could be more robust:

if (trimmedUrl.includes('\\')) {
  return '/';
}

Missing checks:

• %2f and other URL-encoded characters that could bypass / checks
• Case variations like JavaScript: (already handled via toLowerCase() ✓)
• Tab and other whitespace characters beyond \n and \r

Test case to add:

validateReturnUrl('/%2f/evil.com', baseUrl) // Should return '/'
validateReturnUrl('/\t/path', baseUrl) // Should return '/'

2. Session Security (authn.ts:211-213)

Object.keys(req.session as Record<string, unknown>).forEach((key) => {
  delete (req.session as Record<string, unknown>)[key];
});

Issue: Manual session clearing instead of using req.session.destroy(). While this works, it's less idiomatic and could miss session store cleanup.

Recommendation: Use req.session.destroy() unless there's a specific reason not to.

---

🔧 Code Quality Issues

1. Inconsistent Error Handling Patterns

Some handlers use tuple returns [data, error] (authn.ts) while others throw exceptions (oauth-handlers.ts). Consider standardizing on one pattern.

2. Magic Numbers

const DEFAULT_TTL_MS = 10 * 60 * 1000; // 10 minutes (oauth-state.ts:21)
const expiresIn = 15777000; // ~6 months (oauth-token-exchange.ts:76)
const JWKS_CACHE_TTL_MS = 60 * 60 * 1000; // 1 hour (oauth-token-exchange.ts:137)

These are well-commented, but consider extracting to a configuration file for easier tuning.

3. Duplicate Code: loginUser Function

The loginUser function is duplicated in both auth-handlers.ts:40-50 and oauth-handlers.ts:52-62. Extract to a shared utility.

4. Console.log in Production Code (authn.ts:277, 307)

console.log(`serialize user: ${user.getId()}`);
console.log(`TODO: unset cookie`);

Issue: Should use the logger (winston) like the rest of the codebase. The TODO on line 307 appears to be completed by the logout handler above it.

---

🚀 Performance Considerations

1. N+1 Query Pattern in User Lookup (authn.ts:145-165)

When handling duplicate temp users, the code makes multiple sequential database calls:

const userDocs = await users.findByScan({ email });
// Then loops through and deletes each one individually
for (const user of userDocs) {
  await users.deleteOne(userId);
}

Recommendation: If the DB supports batch deletes, use that. Otherwise, this is acceptable for handling edge cases.

2. Sequential API Calls in Google OAuth

const tokens = await exchangeGoogleCode(...); // Call 1
const userInfo = await fetchGoogleUserInfo(tokens.access_token); // Call 2

This is correct (Call 2 depends on Call 1), but consider adding timeouts for external API calls to prevent hanging requests.

---

✨ Missing Features / Enhancements

1. Rate Limiting: Consider adding rate limiting for auth endpoints to prevent brute force attacks
2. Token Refresh: No refresh token handling implemented yet (future work?)
3. Audit Logging: Consider logging auth events (login, logout, failed attempts) for security monitoring
4. MFA Support: No multi-factor authentication (likely out of scope for this PR)

---

📋 Test Coverage Assessment

Excellent coverage with 434 test cases including:

• Unit tests for all auth handlers ✓
• OAuth flow tests ✓
• Error condition tests ✓
• URL validation tests ✓
• Firebase REST client tests ✓

Gap: Integration tests are present but could be expanded to cover:

• Full OAuth round-trips with state management
• Session persistence across requests
• Concurrent login attempts

---

🎯 Summary & Recommendations

Overall Assessment: This is high-quality, production-ready code with strong security practices and excellent test coverage. The migration strategy is sound.

Priority Fixes (before merge):

1. Fix the hard-coded require('crypto') in oauth-token-exchange.ts:74
2. Remove console.log statements and use proper logger
3. Add comment explaining password reset error suppression for security
4. Extract duplicate loginUser function

Consider for Follow-up:

1. Add transaction support for Apple OAuth user updates
2. Implement rate limiting on auth endpoints
3. Add URL validation tests for encoded characters
4. Replace manual session clearing with req.session.destroy()
5. Add runtime validation for Firebase API responses
6. Consider using a proper cache library for JWKS

Bundle Size: The PR description mentions 80-200KB savings from removing Firebase SDK - excellent outcome! ✅

---

Style Adherence

Code follows repository conventions well:

• Proper copyright headers ✓
• TypeScript strict mode ✓
• Clear separation of concerns ✓
• No emoji in code ✓

One minor note: Based on CLAUDE.md commit style, the PR title should be lowercase first letter: "server: move firebase auth to server-side endpoints" (currently capitalized "Firebase")

---

Great work on this migration! The architecture is clean, security is solid, and test coverage is comprehensive. The issues raised are relatively minor and mostly about polish and future-proofing.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: db27462980

ℹ️ 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".

[chatgpt-codex-connector]

Avoid ESM-only jose import in CommonJS build

The server is compiled as CommonJS (src/server/tsconfig.json sets module: "commonjs"), but jose@6 is ESM-only. This import * as jose from 'jose' will compile to require('jose') and throw ERR_REQUIRE_ESM at runtime, preventing the auth module from loading and breaking Apple OAuth flows. Consider pinning to a CJS-compatible jose version or switching this module to dynamic import() / ESM build so the server can start.

Useful? React with 👍 / 👎.

[claude]

PR Review: Server-Side Firebase Authentication Migration

This is a comprehensive migration that moves Firebase authentication from client-side SDK to server-side endpoints. The implementation is thorough with excellent test coverage (2,760 lines of tests across 8 test files). Here's my detailed feedback:

Code Quality & Architecture

Strengths

1. Excellent separation of concerns - The code is well-organized into focused modules (auth-handlers.ts, oauth-handlers.ts, oauth-token-exchange.ts, url-validation.ts, oauth-state.ts)
2. Comprehensive test coverage - Every module has corresponding tests with good edge case coverage
3. Security-conscious design - CSRF protection via state tokens, URL validation, proper cryptographic verification
4. Clear commit history - Each commit addresses a specific issue with detailed explanations in commit messages

Areas for Improvement

1. Error Handling & Information Disclosure

Location: auth-handlers.ts:199

} catch (err) {
  logger.error('Error sending password reset email:', err);
  res.status(200).json({ success: true });  // ⚠️ Silently swallows all errors
}

While this prevents email enumeration attacks (good!), it also hides legitimate errors (network issues, config problems). Consider:

• Logging with more context (email domain, error type)
• Internal monitoring/alerting for repeated failures
• Rate limiting to prevent abuse while still surfacing real errors

2. Race Condition in Apple OAuth

Location: oauth-handlers.ts:225 and oauth-handlers.ts:294

await deps.stateStore.invalidate(state);  // Line 225
// ... lots of async operations ...
await users.update(existingUser.getId(), {}, existingUser);  // Line 294

If the callback processing fails after invalidating state (e.g., database error at line 294), users can't retry without restarting OAuth flow. Consider invalidating state only after successful completion or implementing idempotency.

3. Hardcoded Crypto Module Import

Location: oauth-token-exchange.ts:74

const crypto = require('crypto');  // ⚠️ Should be at top of file

This should be imported at the module level for consistency and testability.

4. Potential Memory Leak in JWKS Cache

Location: oauth-token-exchange.ts:135-137

let cachedJwks: jose.JSONWebKeySet | undefined;
let jwksCacheTime = 0;
const JWKS_CACHE_TTL_MS = 60 * 60 * 1000;

Module-level caching works but could grow unbounded if Apple's JWKS changes frequently. Consider:

• Using a proper cache library with size limits
• Adding cache invalidation on verification failures
• The clearJwksCache() export is good but appears unused - consider automated invalidation

5. Password Whitespace Handling

Location: auth-handlers.ts:55 vs auth-handlers.ts:107

// Login preserves password whitespace (correct)
const password = typeof req.body?.password === 'string' ? req.body.password : '';

// Signup preserves too, but displayName is trimmed
const displayName = typeof req.body?.displayName === 'string' ? req.body.displayName.trim() : '';

This is correct, but consider adding validation to reject passwords that are only whitespace (security best practice).

6. Multi-Key Query Implementation

Location: table-firestore.ts changes
The PR adds multi-key query support but the implementation details aren't visible in the review. Ensure:

• Composite indexes are documented (Firestore requires them)
• Query performance is acceptable (Firestore has query limitations)
• The findByScan name is misleading if it now uses indexed queries

Security Considerations

Good Practices Observed ✅

1. CSRF protection via cryptographically random state tokens
2. URL validation prevents open redirects
3. Apple ID token verification using JWKS from Apple
4. Disabled account checks across all auth paths
5. Provider-scoped user lookups prevents cross-provider collisions

Security Concerns ⚠️

1. Session Fixation Risk

Location: auth-handlers.ts:40-50 and multiple uses

function loginUser(req: Request, user: User): Promise<void> {
  return new Promise((resolve, reject) => {
    req.login(user, (err) => {  // ⚠️ No session regeneration

After authentication, you should regenerate the session ID to prevent session fixation attacks:

req.session.regenerate((err) => {
  if (err) return reject(err);
  req.login(user, (err) => {
    if (err) reject(err);
    else resolve();
  });
});

2. Insufficient Rate Limiting

The code lacks rate limiting on authentication endpoints. Consider adding:

• Per-IP rate limits on /auth/login, /auth/signup
• Account-level lockout after N failed attempts
• CAPTCHA after suspicious activity

3. Token Expiration Not Enforced Client-Side

After moving to server-side auth, ensure session cookies have appropriate:

• httpOnly flag (prevents XSS)
• secure flag (HTTPS only)
• sameSite attribute (CSRF protection)
• Short maxAge with refresh token mechanism

4. Secrets in Configuration

Location: config/default.json

"privateKey": ""  // ⚠️ Will contain Apple private key in production

Ensure this file is in .gitignore and secrets are loaded from environment variables or secret management service.

Performance Considerations

Good Decisions ✅

1. JWKS caching reduces Apple API calls
2. State cleanup prevents Firestore bloat
3. Bundle size reduction (~80-200KB) is significant

Optimization Opportunities

1. Redundant Database Queries

Location: oauth-handlers.ts:263-280

let existingUser = await deps.users.findOneByScan({ providerUserId: claims.sub, provider: 'apple' });
if (existingUser) {
  // Check if Firebase account is disabled
  const fbUser = await deps.firebaseAdmin.getUserByProviderUid('apple.com', claims.sub);

You're querying both local DB and Firebase. Consider caching disabled status in local DB to reduce Firebase calls.

2. State Store Cleanup

No automatic cleanup mechanism for expired OAuth states. Consider:

• Firestore TTL policy (if available)
• Periodic cleanup job
• Cleanup on validate (delete expired states when encountered)

Test Coverage Analysis

The test suite is impressive with 2,760+ lines across 8 files. Test quality is high with:

• Unit tests for each module
• Integration tests for full flows
• Edge case coverage (disabled users, missing emails, migration paths)
• Proper mocking of external dependencies

Test Coverage Gaps

1. No tests for concurrent OAuth flows (race conditions)
2. No tests for session regeneration (because it's not implemented)
3. No performance/load tests for database queries
4. Email validation tests missing (what happens with user@domain..com?)

Migration & Backwards Compatibility

Well Handled ✅

1. Deprecation path - Old /session endpoint kept for mobile apps
2. Data migration - providerUserId backfill logic for existing users
3. Firebase fallback - Handles users created before migration

Potential Issues ⚠️

1. Breaking change for API clients - Any scripts/tools using client-SDK auth will break
2. Session invalidation - Existing sessions might not work after deployment
3. Mobile app compatibility - Ensure old mobile apps can still authenticate during transition

Recommendations

Must Fix (Security)

1. Implement session regeneration after authentication
2. Add rate limiting to auth endpoints
3. Verify session cookie security attributes
4. Ensure secrets are loaded from environment (not config files)

Should Fix (Reliability)

1. Move state invalidation to after successful auth completion
2. Move crypto import to module top
3. Add JWKS cache invalidation on verification errors
4. Add automatic OAuth state cleanup

Nice to Have (Quality)

1. Add password whitespace validation (reject all-whitespace passwords)
2. Add monitoring/alerting for password reset errors
3. Document required Firestore composite indexes
4. Add integration tests for race conditions
5. Consider caching Firebase disabled status in local DB

Summary

This is a well-executed migration with strong security foundations and excellent test coverage. The code is clean, well-organized, and thoroughly tested. The main concerns are:

• Session fixation vulnerability (high priority)
• Missing rate limiting (high priority)
• Race condition in OAuth state management (medium priority)

The migration path for existing users is thoughtful, and the bundle size reduction is a nice win. With the security fixes, this would be production-ready.

Overall Assessment: ⭐⭐⭐⭐☆ (4/5)

• Code Quality: 5/5
• Test Coverage: 5/5
• Security: 3/5 (fixable issues)
• Performance: 4/5
• Migration Strategy: 5/5

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 792266b3c3

ℹ️ 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".

[chatgpt-codex-connector]

Bind OAuth state to session to prevent login CSRF

stateStore.create(returnUrl) generates a valid state that isn’t tied to the initiating user/session. Because the callback only checks that the state exists in Firestore, an attacker can initiate OAuth, capture their own code + state, and then trick a victim into hitting the callback URL, which logs the victim into the attacker’s account. To prevent login CSRF, persist the state in the user’s session (or a signed, HttpOnly cookie) and verify it in the callback; the same issue applies to the Apple flow.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[claude]

Code Review for PR #232: Server-side Firebase Authentication Migration

Summary

This PR migrates Firebase authentication from client-side SDK to server-side endpoints. The implementation is comprehensive and well-tested, with 4,279 additions across authentication handlers, OAuth flows, and extensive test coverage.

---

✅ Strengths

1. Excellent Security Practices

• CSRF Protection: Dual-layer OAuth state validation using both Firestore-backed state store AND session binding (lines 70-71, 119-124 in oauth-handlers.ts) - this is excellent defense-in-depth
• Open Redirect Prevention: Comprehensive URL validation in url-validation.ts with checks for dangerous protocols, backslashes, protocol/host matching
• Cryptographic Verification: Proper Apple ID token verification using JWKS with caching (oauth-token-exchange.ts:155-179)
• Disabled Account Checks: Consistently checking Firebase disabled status across all auth flows

2. Comprehensive Test Coverage

• 8 new test files with substantial coverage
• Tests cover edge cases like:
  • Apple OAuth without email (returning users)
  • Email_verified string/boolean normalization
  • Multi-key Firestore queries
  • Password whitespace preservation
  • Provider-scoped lookups

3. Thoughtful Migration Path

• Firebase provider fallback for pre-migration Apple users (oauth-handlers.ts:308-330)
• Backfilling providerUserId when matching by email (authn.ts:198-204)
• Graceful handling of eventual consistency issues

4. Good Code Organization

• Clear separation of concerns (handlers, token exchange, state management, URL validation)
• Dependency injection pattern for testability
• Proper error handling with user-friendly redirects

---

🔴 Critical Issues

1. Password Reset Information Disclosure (auth-handlers.ts:185-202)

export function createResetPasswordHandler(deps: AuthHandlerDeps): RequestHandler {
  return async (req: Request, res: Response): Promise<void> => {
    // ...
    try {
      await deps.firebaseRestClient.sendPasswordResetEmail(email);
      res.status(200).json({ success: true });
    } catch (err) {
      logger.error("Error sending password reset email:", err);
      res.status(200).json({ success: true });  // ⚠️ SECURITY ISSUE
    }
  };
}

Problem: Always returning success prevents email enumeration attacks, which is good. However, the log message logger.error could expose which emails do not exist to anyone with log access.

Recommendation: Use logger.debug instead of logger.error, or log without the email address.

---

2. Missing Rate Limiting

None of the auth endpoints have rate limiting implemented. This is critical for:

• /auth/login - credential stuffing attacks
• /auth/signup - account creation spam
• /auth/reset-password - email bombing
• /auth/providers - email enumeration

Recommendation: Add express-rate-limit middleware to all auth routes, especially login/signup.

---

3. Hardcoded mock-client-secret in Tests (oauth-handlers.test.ts:14)

generateAppleClientSecret: jest.fn(() => "mock-client-secret"),

While this is in test code, it is a potential security smell if this value ever leaked into production code. Consider using a more obviously fake value like TEST_SECRET_DO_NOT_USE.

---

🟡 High Priority Issues

4. Potential Timing Attack in State Validation (oauth-handlers.ts:119-124)

const sessionState = (req.session as Record<string, unknown>)?.oauthState;
if (sessionState \!== state) {
  res.status(400).json({ error: "State mismatch - possible CSRF attack" });
  return;
}

Problem: String comparison using \!== is vulnerable to timing attacks. An attacker could potentially determine the valid state character-by-character.

Recommendation: Use crypto.timingSafeEqual for constant-time comparison.

---

5. Session State Not Cleared on Error Paths

In both Google and Apple OAuth callbacks, if an error occurs after state validation but before the delete (req.session as Record<string, unknown>).oauthState line (129, 249), the session state remains. This could theoretically allow state reuse.

Recommendation: Move the session cleanup to a finally block or earlier in the flow.

---

6. Apple Client Secret Long Expiration (oauth-token-exchange.ts:76)

const expiresIn = 15777000; // ~6 months

Problem: 6 months is quite long. If the secret is compromised, it could be used for an extended period.

Recommendation: Consider reducing to 24-48 hours with automatic regeneration.

---

7. Missing Input Validation

Several handlers do not validate input beyond basic type checking:

• Email format validation (regex check for valid email)
• Display name length limits
• Password complexity requirements (if not enforced by Firebase)

Recommendation: Add validation using a library like validator or zod.

---

8. Insufficient Error Information for Debugging

When errors occur, users are redirected with generic error codes like ?error=oauth_callback_failed. The actual error is only logged server-side, making it difficult to debug user-reported issues.

Recommendation: Consider adding error IDs that can be correlated with logs for debugging.

---

🟢 Medium Priority Issues

9. JWKS Cache Never Invalidates on Error (oauth-token-exchange.ts:135-153)

If fetching JWKS fails, the cached JWKS could be stale. The cache only checks TTL, not if the previous fetch failed.

Recommendation: Clear cache on fetch failure or add retry logic.

---

10. Inconsistent Error Handling Between Providers

Google OAuth uses firebaseAdmin.getUserByEmail while Apple uses getUserByProviderUid in the no-email path. This asymmetry could lead to maintenance issues.

Recommendation: Consider extracting common user lookup/creation logic into a shared function.

---

11. Magic Strings for Provider Names

Provider names are scattered as string literals (google, apple, password).

Recommendation: Define constants or use the AuthProvider type more consistently.

---

12. Firestore Multi-Key Query Limitation (table-firestore.ts:70-87)

The implementation chains multiple .where() clauses, which works but is not optimized for Firestore. Without composite indexes, these queries could be slow or fail at scale.

Recommendation: Document the need for composite indexes in deployment docs, or add index configuration files.

---

13. Session Type Safety

Multiple places cast req.session as Record<string, unknown>. This bypasses TypeScript type safety.

Recommendation: Extend the session type properly using module augmentation.

---

🔵 Minor Issues / Nitpicks

14. Inconsistent Error Logging

Some errors use logger.error, others use logger.debug. Establish a consistent policy.

---

15. Bundle Size Claims Not Verified

The PR description claims 80-200KB savings but does not include measurement evidence. Consider adding bundle size reporting to CI.

---

16. Missing JSDoc Comments

Public functions lack documentation. Consider adding JSDoc for better IDE support.

---

🧪 Test Coverage Assessment

Excellent coverage with tests for:

• ✅ OAuth flows (Google & Apple)
• ✅ Email/password auth
• ✅ Edge cases (no email, disabled users, email_verified normalization)
• ✅ CSRF protection
• ✅ URL validation
• ✅ State management
• ✅ Token exchange
• ✅ Multi-key queries

Missing tests:

• ❌ Rate limiting (not implemented)
• ❌ Email validation edge cases
• ❌ Session cleanup on error paths
• ❌ Concurrent request handling
• ❌ Apple client secret expiration/rotation

---

📊 Performance Considerations

Positive:

• JWKS caching reduces external requests
• Firestore queries are reasonably optimized
• Removed client-side Firebase SDK (bundle size win)

Concerns:

• N+1 Queries: The user lookup logic (oauth-handlers.ts:288-330) could make multiple Firebase calls in the Apple no-email path
• Firestore Scans: findByScan could be slow on large user tables without proper indexes
• Session Storage: Storing OAuth state in sessions + Firestore is redundant. Consider using only session storage for the CSRF binding.

---

🔒 Security Summary

Category	Rating	Notes
Authentication	✅ Strong	Proper token verification, disabled account checks
CSRF Protection	✅ Strong	Dual-layer validation (state + session)
Open Redirect	✅ Strong	Comprehensive URL validation
Rate Limiting	❌ Missing	Critical gap for production
Input Validation	🟡 Partial	Basic checks, needs email/name validation
Error Information	🟡 Adequate	Generic errors prevent enumeration, but could leak via logs
Session Security	🟡 Good	Minor timing attack concern in state comparison

---

🎯 Recommendations Priority

Must Fix Before Merge:

1. Add rate limiting to all auth endpoints
2. Fix password reset logging (use debug instead of error)
3. Clear session state in error paths (use try-finally)

Should Fix Soon:

4. Use constant-time comparison for state validation
5. Reduce Apple client secret expiration time
6. Add email format validation
7. Document Firestore composite index requirements

Nice to Have:

8. Add JSDoc comments
9. Improve error correlation with unique IDs
10. Extract common user lookup logic
11. Type-safe session extensions
12. JWKS cache error handling

---

✅ Conclusion

This is a high-quality PR with excellent security fundamentals, comprehensive test coverage, and thoughtful edge case handling. The authentication migration is well-architected and demonstrates strong security awareness.

The critical issues identified (rate limiting, timing attacks, session cleanup) should be addressed before production deployment, but they do not block the core functionality from working correctly.

Recommendation: ✅ Approve with required changes - Address the "Must Fix" items, then this is ready to merge.

Great work on the thorough implementation and test coverage! 🎉

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 926822e389

ℹ️ 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".

[chatgpt-codex-connector]

Avoid state checks that require same-site cookies

These callbacks require req.session.oauthState to match the incoming state, but the session cookie is configured as sameSite: "strict" in production (config/production.json) and defaults to Lax otherwise. Browsers won’t send that cookie on cross-site OAuth redirects (Apple’s form_post and Google’s redirect), so sessionState will be undefined and the handler will return 400 even when the Firestore state is valid. That makes OAuth login fail in production unless SameSite is relaxed or the CSRF check is moved entirely to the Firestore state record.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve existing provider IDs when linking accounts

When a user is matched by email, this code overwrites the stored providerUserId and provider with the latest OAuth provider. That breaks Apple re-login: Apple often omits email after the first login, and the Apple callback path only searches by providerUserId/provider. If the user later signs in with Google, this overwrite removes the Apple sub, so subsequent Apple logins (without email) won’t find the account and will fail with apple_no_email despite the account existing.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e0f947411

ℹ️ 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".

[chatgpt-codex-connector]

Update providerUserId on email match even if set

When an existing user is matched by email, this code only updates providerUserId if it is empty. For users created via password signup, providerUserId is already set to the Firebase UID, so an Apple OAuth login (first time with email) will not store the Apple sub. On the next Apple login, Apple often omits email and the callback searches by providerUserId + provider (oauth-handlers.ts lines 260-264), which will miss and end in apple_no_email. This breaks Apple re-login for users who signed up with password then use Apple. Consider updating the provider link when the existing provider differs or storing multiple provider IDs.

Useful? React with 👍 / 👎.

[claude]

Pull Request Review

Summary

This PR migrates Firebase authentication from client-side SDK to server-side endpoints, which is a significant architectural improvement. The implementation adds comprehensive auth endpoints, OAuth handlers, and extensive test coverage. Overall, this is well-structured code, but I've identified several areas for improvement.

Strengths

✅ Excellent Test Coverage: 2,795 lines of tests across 8 test files demonstrates strong commitment to quality
✅ Security Best Practices: Proper CSRF protection with OAuth state tokens, URL validation to prevent open redirects, and cryptographic verification of Apple ID tokens
✅ Clean Architecture: Good separation of concerns with dedicated modules for handlers, token exchange, state management, and URL validation
✅ Error Handling: Comprehensive error handling with proper HTTP status codes mapped to Firebase error codes
✅ Bundle Size Win: Removing Firebase SDK from client (~80-200KB savings) is a meaningful performance improvement

Critical Issues

1. Security: Apple Client Secret Generation Uses Node.js crypto Instead of jose (oauth-token-exchange.ts:74)

The generateAppleClientSecret function uses require('crypto') instead of the jose library that's already in use. This is inconsistent and potentially problematic.

Recommendation: Use the jose library's SignJWT for consistency and better maintainability.

2. Race Condition in OAuth State Invalidation (oauth-handlers.ts:118, 225)

Both Google and Apple OAuth callbacks invalidate the state BEFORE the token exchange completes. If the token exchange fails, the state is already consumed, preventing retry.

Recommendation: Invalidate state only AFTER successful completion of the entire flow, or in the catch block to prevent replay attacks even on failure.

3. Inconsistent Error Handling in Password Reset (auth-handlers.ts:185-202)

The password reset handler swallows errors and always returns success. While this prevents email enumeration attacks (good!), the logged error still occurs for non-existent emails. The firebase-rest-client already handles this (line 134), so the outer catch is redundant.

Recommendation: Remove the outer try-catch since the client already handles EMAIL_NOT_FOUND gracefully.

Major Issues

4. Missing Input Validation on OAuth returnUrl Query Parameter (oauth-handlers.ts:67, 175)

The returnUrl is accepted from query params without size limits. An attacker could provide an extremely long URL that gets stored in Firestore, potentially causing DoS or excessive storage costs.

Recommendation: Add length validation (e.g., max 2048 characters) before storing.

5. Apple OAuth: Complex providerUserId Lookup Logic (oauth-handlers.ts:261-309)

The fallback logic for handling users without providerUserId is quite complex with nested try-catches and multiple database lookups.

Recommendation: Consider adding a data migration script to backfill providerUserId for existing Apple users, then simplify this logic. Document the migration path in code comments if this must remain.

6. Console.log Used for Debugging (authn.ts:281, Login.tsx:126, 145, 235)

Multiple console.log statements remain in the code.

Recommendation: Replace with proper logger calls (winston) on the server and a proper error reporting mechanism on the client.

7. JWKS Cache Never Cleared on Failure (oauth-token-exchange.ts:135-153)

If fetching Apple's JWKS fails, the cached JWKS could become stale indefinitely.

Recommendation: Clear cache on verification failures or implement a shorter TTL for failed refreshes.

Minor Issues

8. Type Safety: Any Types in Passport Integration (authn.ts:279-300)

Passport callbacks use any types extensively. Consider defining proper types for serialized user objects.

9. Potential Memory Leak: JWKS Cache Never Expires (oauth-token-exchange.ts:135)

The JWKS cache is module-level and never cleared, even after hours of inactivity. Consider using a proper cache library with TTL support.

10. Missing Rate Limiting on Auth Endpoints

The new auth endpoints lack rate limiting, which could be abused for brute force attacks, email enumeration, and DoS.

Recommendation: Add rate limiting middleware (e.g., express-rate-limit) to all auth endpoints.

11. Duplicate loginUser Function (auth-handlers.ts:40, oauth-handlers.ts:52)

The same loginUser helper appears in both files with identical implementation.

Recommendation: Extract to a shared utility module.

12. URL Validation: Backslash Check (url-validation.ts:22-24)

Add a comment explaining why both checks are needed, or consolidate if one is sufficient.

Code Quality Observations

Good Patterns:

• Dependency injection via factory functions
• Proper use of TypeScript interfaces for dependencies
• URL validation with comprehensive edge case handling
• Tests cover edge cases well

Suggestions:

• Consider extracting magic numbers to constants
• Some functions are quite long (e.g., createAppleOAuthCallbackHandler at 160 lines)
• The getOrCreateUserFromIdToken and getOrCreateUserFromVerifiedInfo functions have nearly identical eventual consistency workarounds

Testing

The test coverage is excellent overall. A few suggestions:

1. Test Apple OAuth without email - the fallback logic deserves dedicated coverage
2. Test JWKS cache expiration
3. Test concurrent OAuth state validation
4. Expand integration tests

Performance Considerations

✅ Good: Removing Firebase SDK from client bundle is a significant win
✅ Good: JWKS caching reduces external calls to Apple
⚠️ Concern: Multiple database lookups in Apple OAuth fallback logic could be slow
⚠️ Concern: No database indexes mentioned for providerUserId + provider lookups

Security Assessment

Overall security posture is strong:

• ✅ CSRF protection via state tokens
• ✅ Open redirect prevention
• ✅ Apple ID token verification with JWKS
• ✅ Proper error message sanitization
• ⚠️ Missing rate limiting
• ⚠️ OAuth state invalidation timing issue

Verdict

This is solid work with good architecture and excellent test coverage. The critical issues should be addressed before merge, particularly:

1. Switch Apple client secret generation to use jose
2. Fix OAuth state invalidation timing
3. Add rate limiting to auth endpoints

The other issues are important but could be addressed in follow-up PRs if time is constrained.

Recommendation: Approve with required changes to items 1-3 above.

---

Great work on this migration! The move to server-side auth is the right architectural decision, and the implementation quality is high overall.

[bpowers]

@codex review

[claude]

Code Review - PR #232: Server-side Firebase Auth Migration

This is a substantial and well-executed refactoring that migrates Firebase authentication from client-side SDK to server-side endpoints. The PR demonstrates strong engineering practices with comprehensive test coverage (2800+ lines of tests) and thoughtful iterative improvements through 16 commits.

✅ Strengths

1. Excellent Test Coverage

• Comprehensive unit tests for all auth modules (900 lines for auth-handlers, 875 for OAuth handlers)
• Tests cover edge cases: disabled users, missing emails, providerUserId migration, email_verified normalization
• Integration tests verify end-to-end flows
• This exceeds expectations for production-ready code

2. Strong Security Implementation

• ✅ CSRF protection via cryptographically random 32-byte OAuth state tokens with 10-minute TTL
• ✅ Open redirect prevention through URL validation (url-validation.ts)
• ✅ Cryptographic verification of Apple ID tokens using JWKS with proper caching (1-hour TTL)
• ✅ Apple ES256 signature generation correctly uses ieee-p1363 encoding (fixed in commit af73774)
• ✅ Proper password handling - passwords NOT trimmed (fixed in commit 19f5cb3)
• ✅ Disabled account checks for both email/password and OAuth flows

3. Well-Structured Code

• Clear separation of concerns: handlers, token exchange, state management, URL validation
• Dependency injection pattern makes code testable
• Proper error handling with custom FirebaseAuthError class
• User-friendly error messages mapped from Firebase error codes

4. Thoughtful Migration Path

• Firebase provider fallback for pre-migration Apple users (commit db27462)
• Provider ID preservation when switching between OAuth providers (commit 5e0f947)
• Password users can upgrade to OAuth with proper provider tracking (commit ff3f786)
• Backward compatibility maintained via deprecated /session endpoint

5. Performance Improvements

• 80-200KB bundle size reduction by removing Firebase SDK from client
• JWKS caching reduces Apple auth latency
• Efficient providerUserId lookups prevent O(n) user scans

🔍 Issues & Recommendations

Critical Issues

1. Password Trimming Inconsistency in Client ⚠️

File: src/app/Login.tsx:167,214

The server correctly preserves password whitespace (commit 19f5cb3), but the client still trims passwords in signup/login. Users cannot create/use passwords with leading/trailing spaces, creating an inconsistency with the server's handling.

Recommendation: Remove .trim() from password handling in Login.tsx (but keep it for email).

High Priority Issues

2. Race Condition in OAuth State Invalidation

File: src/server/auth/oauth-handlers.ts:118,225

State is invalidated after validation but before the user is logged in. If token exchange or user creation fails after state invalidation, the state is consumed but the user isn't logged in.

Recommendation: Move invalidate() to after successful login, or use a two-phase invalidation (mark as "in-use" then delete on success).

3. Session Fixation Vulnerability ⚠️

File: src/server/auth/auth-handlers.ts:42-49,82

req.login() is called without regenerating the session ID. If an attacker can set a user's session ID before login, they could hijack the authenticated session.

Recommendation: Regenerate the session before calling req.login().

4. Email Verification Not Enforced

File: src/server/authn.ts:199-209

The email_verified claim from OAuth providers is normalized but never checked. Users could potentially create accounts with unverified emails (though this is mitigated by OAuth provider verification).

Recommendation: Either enforce email_verified === true or document why unverified emails are acceptable from OAuth providers.

Medium Priority Issues

5. Missing Rate Limiting

The auth endpoints don't appear to have rate limiting beyond Firebase's built-in TOO_MANY_ATTEMPTS_TRY_LATER.

Recommendation: Add express-rate-limit middleware to prevent brute force attacks on /auth/login, /auth/signup, etc.

6. Insufficient Logging for Security Events

Failed logins, disabled account attempts, and suspicious OAuth callbacks should be logged for security monitoring.

Recommendation: Add structured logging for failed login attempts, disabled account access attempts, OAuth state validation failures, and suspicious redirect URLs.

7. Type Safety Issue with any

File: src/server/authn.ts:282-288

Passport serialization uses any types. Define proper types for serialized user data.

8. Apple Client Secret Regeneration Overhead

File: src/server/auth/oauth-handlers.ts:227-232

The Apple client secret is regenerated on every callback (valid for 6 months but recreated each time). Performance impact is negligible, but unnecessarily re-signing.

Recommendation: Cache the client secret with a TTL (e.g., 1 day).

9. Inconsistent Error Handling in Password Reset

File: src/server/auth/auth-handlers.ts:195-200

Password reset always returns success even on error (for security), but logs the error. This is correct for preventing email enumeration, but the try-catch should only catch expected errors.

Recommendation: Only catch FirebaseAuthError, let unexpected errors surface.

Low Priority / Nitpicks

10. Magic Strings for Providers

AuthProvider type is good, but string literals like 'apple.com' and 'google.com' are used throughout without constants. Consider defining constants.

11. Unclear Variable Name

File: src/server/auth/oauth-handlers.ts:244-256

appleUserJson is confusing - it's a string that gets parsed to JSON. Rename to appleUserString or rawAppleUser.

12. Duplicate Code in OAuth Handlers

Google and Apple handlers share significant logic for Firebase user lookup/creation and disabled checks. Extract shared logic into helper functions to reduce duplication and maintain consistency.

13. Missing JSDoc Comments

Public functions lack documentation explaining parameters, return values, and error conditions. Add JSDoc for public APIs, especially getOrCreateUserFromVerifiedInfo.

🎯 Best Practices & Architecture

Positive:

• ✅ Dependency injection throughout
• ✅ Proper error typing with custom error classes
• ✅ Test-driven development evident from iterative commits
• ✅ No backwards-compatibility shims (following CLAUDE.md guidance)
• ✅ Commit messages follow project style perfectly

Suggestions:

• Consider adding OpenAPI/Swagger docs for the new auth endpoints
• Add metrics/monitoring for auth success/failure rates
• Document the OAuth callback flow in a sequence diagram

📊 Performance Considerations

Positive:

• JWKS caching reduces latency
• Bundle size reduction is significant
• Firestore queries are indexed appropriately

Watch Out For:

• Potential N+1 queries in getOrCreateUserFromVerifiedInfo if findOneByScan falls back to findByScan
• OAuth state cleanup - consider TTL-based Firestore expiration instead of manual invalidation

🔐 Security Summary

Category	Status	Notes
Authentication	✅ Strong	Proper token verification
CSRF Protection	✅ Strong	Random state tokens with TTL
Open Redirects	✅ Protected	URL validation implemented
Session Security	⚠️ Needs Work	Session fixation vulnerability
Input Validation	⚠️ Needs Work	Password trimming inconsistency
Rate Limiting	❌ Missing	Should add
Security Logging	⚠️ Incomplete	Needs structured logging

📝 Recommendation

Overall Assessment: This is high-quality work with excellent test coverage and security fundamentals. The migration is well-thought-out and handles edge cases thoroughly.

Status: ✅ Approve with Required Changes

Required Before Merge:

1. Fix session fixation vulnerability (regenerate session on login)
2. Fix password trimming in client to match server behavior
3. Move OAuth state invalidation after successful login (or use two-phase invalidation)

Recommended for Follow-up PR:

• Add rate limiting middleware
• Add security event logging
• Enforce email verification or document why it's not required
• Reduce code duplication in OAuth handlers

Great work on this complex migration! The iterative approach through 16 commits shows careful attention to edge cases and security concerns.

---

Note: Review based on CLAUDE.md guidelines and TypeScript/Rust development standards for the Simlin project.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ff3f786a73

ℹ️ 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".

[chatgpt-codex-connector]

Prevent disabled Apple users from bypassing checks

In the no-email Apple path, a failed getUserByProviderUid lookup is silently ignored and the handler proceeds to log the user in. This means an account that was disabled in Firebase but lacks an Apple provider link (e.g., created via createUser during first Apple login) can still authenticate on subsequent logins when Apple omits the email. Consider falling back to getUserByEmail(existingUser.getEmail()) or otherwise enforcing the disabled check before calling loginUser.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[claude]

Code Review: Server-Side Firebase Authentication Migration

This is a well-executed migration from client-side to server-side Firebase authentication. The implementation demonstrates strong security practices and comprehensive test coverage. Below are my findings organized by category.

---

✅ Strengths

Security Architecture

• CSRF Protection: Excellent use of cryptographically random 32-byte state tokens with single-use invalidation and 10-minute TTL for OAuth flows
• Open Redirect Prevention: Comprehensive URL validation in url-validation.ts covers dangerous protocols, protocol-relative URLs, backslash tricks, and cross-origin redirects
• Cryptographic Verification: Proper Apple ID token verification using JWKS with issuer and audience validation
• Disabled Account Checks: Consistently checks Firebase disabled flag across all auth paths (password, Google OAuth, Apple OAuth with/without email)
• Password Preservation: Correctly preserves password whitespace while trimming email addresses

Code Quality

• Test Coverage: Outstanding test coverage with ~2,700+ lines of tests across 8 test files covering:
  • Auth handlers (900 lines)
  • OAuth handlers (933 lines)
  • Token exchange (194 lines)
  • URL validation (85 lines)
  • Edge cases like Apple no-email flows, disabled users, provider migrations
• Error Handling: Proper error types with FirebaseAuthError class and user-friendly error messages
• Type Safety: Strong TypeScript typing throughout with clear interfaces

Implementation Details

• Provider Migration Logic: Thoughtful handling of users created before providerUserId migration with fallback to Firebase provider lookup
• Provider ID Scoping: Correctly scopes providerUserId lookups by provider to prevent cross-provider collisions (commit 918b181)
• Account Linking: Intelligent provider ID updates - only updates when existing provider is 'password', preserving OAuth provider IDs

---

⚠️ Issues & Recommendations

1. CRITICAL: Multi-Key Firestore Query Support ✅ FIXED

Status: This was identified and fixed in commit 30ef1cc

The findByScan method previously only supported single-key queries. The fix properly chains multiple .where() clauses:

// Now supports queries like:
{ providerUserId: '...', provider: 'apple' }

This was essential for the provider-scoped lookups to work correctly.

---

2. Security: Email Verification Normalization

File: src/server/auth/oauth-token-exchange.ts:164-171

Apple may send email_verified as string "true"/"false" instead of boolean. The current implementation handles this:

const rawEmailVerified = payload.email_verified;
let emailVerified: boolean | undefined;
if (rawEmailVerified === 'true' || rawEmailVerified === true) {
  emailVerified = true;
} else if (rawEmailVerified === 'false' || rawEmailVerified === false) {
  emailVerified = false;
}

Recommendation: This is good defensive programming, but consider what happens if rawEmailVerified is undefined - should it default to false for security? Currently it remains undefined which may be passed to Firebase.

---

3. Code Quality: Require Statement in ES Module

File: src/server/auth/oauth-token-exchange.ts:74

const crypto = require('crypto');

Issue: Using CommonJS require() instead of ES6 import at function scope.

Recommendation: Move to top-level import:

import * as crypto from 'crypto';

This is more consistent with the rest of the codebase and provides better type safety.

---

4. Potential Race Condition: OAuth State Cleanup

File: src/server/auth/oauth-handlers.ts:118, 225

await deps.stateStore.invalidate(state);

State is invalidated after validation but before the Firebase/DB operations complete. If those operations fail, the user can't retry with the same OAuth code.

Current behavior:

1. State validated ✓
2. State invalidated ✓
3. Token exchange (could fail) ✗
4. User creation (could fail) ✗
5. Login (could fail) ✗

Recommendation: Move state invalidation to after successful user login, or use a try-finally pattern:

try {
  // ... all auth operations ...
  await loginUser(req, user);
  await deps.stateStore.invalidate(state);
  res.redirect(returnUrl);
} catch (err) {
  // State remains valid, user can retry
  logger.error('Error in OAuth callback:', err);
  res.redirect('/?error=oauth_callback_failed');
}

---

5. Error Handling: Inconsistent Catch Block

File: src/server/auth/oauth-handlers.ts:164-168

} catch (err) {
  logger.error('Error in Google OAuth callback:', err);
  await deps.stateStore.invalidate(state).catch(() => {});
  res.redirect('/?error=oauth_callback_failed');
}

The catch block attempts to invalidate state even on errors. This is inconsistent with the recommendation in #4, but also the nested .catch(() => {}) silently swallows any invalidation errors.

Recommendation: Decide on a consistent error recovery strategy. Either:

• Option A: Don't invalidate on error (let users retry)
• Option B: Invalidate but log failures: .catch((e) => logger.warn('Failed to invalidate state:', e))

---

6. Type Safety: Unsafe Type Casts

File: Multiple locations

// oauth-token-exchange.ts:55, 70
return (await response.json()) as TokenResponse;
return (await response.json()) as GoogleUserInfo;

// oauth-handlers.ts:129
const adminErr = err as { code?: string };

Issue: Unsafe as casts without runtime validation.

Recommendation: Add runtime validation or use a schema validation library like Zod for external API responses:

const data = await response.json();
if (!data.access_token || !data.expires_in) {
  throw new Error('Invalid token response from Google');
}
return data as TokenResponse;

---

7. Code Smell: Duplicate Logic

Files:

• src/server/auth/oauth-handlers.ts:125-140 (Google)
• src/server/auth/oauth-handlers.ts:322-336 (Apple)

Both Google and Apple handlers have nearly identical Firebase user creation logic:

let fbUser: admin.auth.UserRecord | undefined;
try {
  fbUser = await deps.firebaseAdmin.getUserByEmail(email);
} catch (err: unknown) {
  const adminErr = err as { code?: string };
  if (adminErr.code === 'auth/user-not-found') {
    fbUser = await deps.firebaseAdmin.createUser({ /* ... */ });
  } else {
    throw err;
  }
}

Recommendation: Extract to shared helper function:

async function getOrCreateFirebaseUser(
  auth: admin.auth.Auth,
  userInfo: { email: string; displayName: string; emailVerified: boolean; photoURL?: string }
): Promise<admin.auth.UserRecord> {
  // Shared implementation
}

---

8. Performance: JWKS Cache Implementation

File: src/server/auth/oauth-token-exchange.ts:135-153

The JWKS cache uses module-level variables:

let cachedJwks: jose.JSONWebKeySet | undefined;
let jwksCacheTime = 0;

Potential Issues:

• Not safe for concurrent requests during cache expiration
• No memory cleanup mechanism
• Module-level state makes testing harder

Recommendation: Consider using a proper caching library or implement a more robust cache with:

• Mutex/lock for concurrent requests
• LRU eviction policy
• Testable cache interface

---

9. Logging: Sensitive Data Exposure

File: src/server/authn.ts:284

console.log(`serialize user: ${user.getId()}`);

Issues:

• Uses console.log instead of winston logger (inconsistent with rest of codebase)
• Left over debug logging in production code

Recommendation: Remove or convert to proper logging:

logger.debug('Serializing user session', { userId: user.getId() });

---

10. Testing: Mock Verification

File: src/server/tests/oauth-handlers.test.ts

The tests use mocks extensively, which is good for unit testing, but consider:

• Integration tests that verify the full OAuth flow end-to-end
• Tests that verify the actual token exchange with Google/Apple (maybe against test/staging environments)
• Tests for the session serialization/deserialization flow

The existing auth-integration.test.ts is a good start but only has 108 lines.

---

📊 Metrics

• Lines Added: 4,325
• Lines Deleted: 246
• Net Change: +4,079
• Test Files: 8 new test files
• Test Lines: ~2,700+
• Test/Code Ratio: ~0.62 (excellent coverage)

---

🎯 Summary

This is a high-quality PR with excellent security practices and test coverage. The migration successfully reduces client bundle size while maintaining security and adding robust server-side controls.

Must Fix Before Merge:

• ✅ Multi-key Firestore queries (already fixed in 30ef1cc)

Should Fix:

• server-side support #3: Move crypto require() to import
• drag selection and bulk move/edit #9: Remove console.log debug statement

Consider for Follow-up:

• editor overlay #2: Email verification default value
• show simulation results #4, view + edit equation #5: OAuth state invalidation strategy
• enlarge clouds #6: Add runtime validation for external API responses
• keep better track of if a model is simulatable, errors, and update stoplight #7: Extract duplicate Firebase user creation logic
• when moving objects w/ links, maintain relative link angle #8: Improve JWKS caching
• moving clouds #10: Add more integration tests

Overall: Approve with minor recommendations ✅

Great work on the comprehensive test coverage and security-conscious implementation!

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. You're on a roll.

ℹ️ 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".

[claude]

Code Review

Finding 1: [P2] Missing response.ok check in onSubmitEmail causes TypeError on server errors

File: src/app/Login.tsx, lines 103-124

When the /auth/providers endpoint returns a non-2xx response (e.g., 500 with { error: "An unexpected error occurred" }), the code destructures providers and registered from the response body without checking response.ok first. Since the error response doesn't contain a providers field, providers will be undefined, and providers.includes('password') on line 112 will throw TypeError: Cannot read properties of undefined. The catch block handles it gracefully with a user-facing message, but the actual server error information is lost and a confusing TypeError is logged to the console instead. Adding a if (!response.ok) guard before destructuring would let you surface the server's error message to the user (similar to how onEmailLogin and onSubmitNewUser handle it).

---

Overall Correctness

Correct. The core auth migration is well-structured: OAuth state management has proper CSRF protection, URL validation prevents open redirects, Apple ID token verification uses proper JWKS, and the pre-migration fallback for Apple users without providerUserId is handled thoughtfully. The findByScan compound query change is backward-compatible with the findOneByScan contract. The one finding above is a minor error-handling gap that won't affect the happy path.

[claude]

Code Review

This is a well-structured migration from client-side Firebase auth to server-side endpoints. The iterative commit history shows thoughtful handling of edge cases (Apple no-email, disabled users, provider-scoped lookups, password whitespace preservation). The security measures — Firestore-backed state tokens, URL validation, JWKS verification, anti-enumeration on password reset — are solid.

After thorough review of all 18 commits and the final state of all changed files, I found no blocking bugs. Here are the findings:

---

[P3] validateReturnUrl returns untrimmed returnUrl for absolute URLs

File: src/server/auth/url-validation.ts, lines 38-48 (final state)

For relative paths starting with /, the function correctly returns trimmedUrl. But for absolute URLs that pass the new URL() host/protocol check, it returns the original returnUrl (not trimmedUrl). If the input contained leading/trailing whitespace like " https://app.simlin.com/foo ", the return value would include that whitespace.

This is low-severity because: (1) query string values from Express rarely contain meaningful whitespace, (2) res.redirect() handles it gracefully, and (3) the security checks are all performed on trimmedUrl so no bypass is possible. Still, returning trimmedUrl (or the reconstructed URL) would be more consistent with the relative-path branch.

    return trimmedUrl;

(replacing return returnUrl; on the absolute URL success path)

---

Verdict: Correct

The patch is well-tested, handles edge cases carefully, and introduces no regressions. The disabled-user checks cover all OAuth paths (Google, Apple with email, Apple without email with both provider and email fallback). The provider update logic correctly distinguishes password-to-OAuth upgrades from OAuth-to-OAuth overwrites. The URL validation is comprehensive. The CSRF protection via single-use 32-byte Firestore-backed tokens is adequate even without session binding (which was correctly removed since SameSite cookies break cross-origin OAuth redirects).

[claude]

Code Review: server: move Firebase auth to server-side endpoints

I reviewed the full diff covering auth handlers, OAuth flows (Google + Apple), Firebase REST client, URL validation, OAuth state management, protobuf schema changes, Firestore query updates, and client-side migration.

Findings

No blocking bugs found.

The auth flows are correctly implemented: input validation is consistent, error codes map to appropriate HTTP statuses, the password-reset handler correctly returns 200 on all paths to prevent email enumeration, and the URL validation covers the standard open-redirect attack vectors (protocol-relative URLs, javascript:/data: schemes, backslash tricks, CRLF injection, host mismatch).

One observation worth noting:

---

[P3] Cross-provider login edge case with Apple email omission
src/server/auth/oauth-handlers.ts (lines ~598-617) / src/server/authn.ts (lines ~224-235)

If a user originally signed up via Google (old or new flow) and later logs in with Apple, the first Apple login succeeds (Apple provides email). On subsequent Apple logins, Apple may omit the email. At that point resolveAppleUserWithoutEmail tries findOneByScan({ providerUserId: appleSub, provider: 'apple' }) — which won't match because providerUserId was never updated from the Google/Firebase UID to the Apple sub (the guard at line ~231 skips the update when existingProvider is an OAuth provider). The Firebase-provider-link fallback also won't find a match because Apple was never linked as a Firebase provider for that user. This means the user gets apple_no_email and can't complete login via Apple. This only affects the unusual case of switching OAuth providers and is not a regression from the old code, but it may be worth a comment or a TODO so it doesn't surprise anyone later.

---

Overall Correctness Verdict

Correct. The patch is well-structured with clean dependency injection, comprehensive tests, and appropriate error handling. Existing code and tests should not break, and the migration path (deprecated /session endpoint preserved) is sound. The one edge case noted above is P3 and not blocking.

[claude]

Code Review

[P2] onSubmitEmail does not check response.ok, causing a TypeError on server errors

File: src/app/Login.tsx, lines 101–110

In onSubmitEmail, the response from /auth/providers is destructured without first checking response.ok. If the server returns a non-2xx status (e.g., 500), the response body will be { error: "..." } rather than { providers, registered }, so providers will be undefined. The subsequent providers.includes('password') then throws a TypeError. The outer catch block does handle this, but the user sees a generic "Failed to check email" message instead of the server's actual error, and the real failure is masked. The other two similar handlers in this file (onEmailLogin, onSubmitNewUser) correctly check response.ok before parsing the body — this one should follow the same pattern.

---

Overall correctness: Correct. The patch is well-structured, handles edge cases thoughtfully (Apple's email omission on subsequent logins, providerUserId migration fallback, fail-closed disabled-account checks), and the URL validation is sound. The one bug above is low-impact since it only surfaces on unexpected server errors and is caught by existing error handling. Existing code and tests should not break.

[bpowers]

Review cycle summary

Four iterations of automated review (codex + Claude) identified and resolved several improvements to the auth migration:

Fixes applied:

• Password trimming bug: Client-side Login.tsx was trimming password whitespace before sending to the server, contradicting the server-side design that preserves whitespace (Firebase treats it as significant). Fixed in both login and signup flows.
• Fail-closed on Apple disabled status: When Firebase is temporarily unreachable during Apple no-email logins, the code now rejects login rather than allowing potentially disabled accounts through. Added test coverage for this edge case.
• JWKS retry on key rotation: Apple ID token verification now retries with a fresh JWKS fetch when the first attempt fails, preventing transient verification failures during Apple key rotations.
• Duplicate code extraction: Deduplicated the loginUser helper (was identical in auth-handlers and oauth-handlers) into auth-utils.ts. Extracted the duplicate error-recovery logic for eventual consistency issues into recoverFromDuplicateUsers.
• URL validation consistency: validateReturnUrl now consistently returns the trimmed URL for absolute same-origin URLs (was returning untrimmed input).
• Shared OAuth state store: Google and Apple OAuth now share a single Firestore state store instance instead of creating separate duplicates.
• Code quality: Replaced require('crypto') with ES import, removed redundant backslash check in URL validation, fixed copyright headers, fixed jest config for ESM packages under pnpm, removed unused clsx import, restored deleted concurrency comments per project policy, added explanatory comments for singleton JWKS cache and defense-in-depth state invalidation.

Issues tracked for follow-up: #380 (email enumeration rate limiting), #381 (Firebase API key to config), #382 (deprecated passport removal), #383 (CSP frame-src cleanup), #384 (OAuth button visibility), #385 (Apple OAuth nonce), #386 (Firestore composite index).

[claude]

Code Review

[P1] TOCTOU race in OAuth state validation allows state token replay

File: src/server/auth/oauth-handlers.ts, lines 99-107 (Google) and 278-286 (Apple)

validate() and invalidate() are separate Firestore operations. Between the validate read returning and the invalidate delete completing, a concurrent request with the same state token can also pass validation. This is a classic time-of-check-time-of-use (TOCTOU) race. The fix is to consume the state atomically — e.g., use a Firestore transaction that reads and deletes in one step, or perform the delete first (with a precondition that the document exists) and treat a successful delete as validation. The current two-step approach leaves a narrow but real window for state token replay, which undermines the CSRF protection the state parameter provides.

---

[P2] Unrecognized Firebase error codes leak raw error messages to the client

File: src/server/auth/firebase-rest-client.ts, line 54; src/server/auth/auth-handlers.ts, line 84

parseErrorMessage falls back to the raw Firebase error string for unrecognized codes: const message = ERROR_MESSAGES[code] ?? rawMessage;. The auth handlers then send this directly to the client: res.status(status).json({ error: err.message }). When Firebase returns an error code not in the ERROR_MESSAGES map, internal details (e.g., quota info, internal service names) may be exposed. The fallback should use a generic message like 'An unexpected error occurred' instead of rawMessage.

---

[P2] returnUrl stored in Firestore without validation

File: src/server/auth/oauth-handlers.ts, lines 56-57 and 236-237

The returnUrl query parameter is persisted to the Firestore state store without any validation — validateReturnUrl() is only called later in the callback handler. While the current code paths do validate before use, storing an unvalidated attacker-controlled URL creates a latent open-redirect risk if any future code path reads returnUrl from the state document without going through validation. Calling validateReturnUrl() before stateStore.create() would provide defense-in-depth at minimal cost.

---

[P3] Google OAuth callback does not check email_verified

File: src/server/auth/oauth-handlers.ts, lines 112-124

The Google callback uses userInfo.email to look up or create a Firebase user without checking userInfo.email_verified. While Google typically returns verified emails, if it ever returns email_verified: false, the code would create a user with an unverified email. Adding an explicit check (or at least passing it through to Firebase's emailVerified field consistently) would be a simple defense-in-depth improvement.

---

Overall Correctness Verdict

The patch is mostly correct but has a real security gap in the OAuth state validation (TOCTOU race). The email/password auth flow, URL validation, Apple ID token verification, and session management are implemented reasonably. The TOCTOU issue should be fixed before merging to ensure the CSRF protection is effective, and the error message leakage should be addressed to avoid exposing internals. The remaining issues are defense-in-depth improvements.

[claude]

Code Review

I've reviewed the full diff for this PR (moving Firebase auth from client-side SDK to server-side endpoints). The changes span the client-side login flow, new server auth handlers, OAuth flows for Google and Apple, URL validation, Firestore-backed OAuth state, and protobuf schema extension.

Findings

No bugs found that meet the review criteria.

Notes

The implementation looks solid:

• URL validation (url-validation.ts) properly handles dangerous protocols, protocol-relative URLs, backslash tricks, CRLF injection, and host-matching for absolute URLs.
• OAuth state management uses Firestore with TTL-based expiration and single-use invalidation. The validate-then-invalidate pattern has a small TOCTOU window, but it's not exploitable because authorization codes are single-use at the provider level.
• Apple no-email flow (resolveAppleUserWithoutEmail) correctly handles Apple's behavior of omitting email on subsequent logins, with a migration fallback for users created before providerUserId was stored. The fail-closed pattern for disabled account checks is a good security practice.
• Password handling correctly stopped trimming passwords (Firebase treats whitespace as significant).
• Body parsing (bodyParser.urlencoded) is already configured in app.ts, so Apple's form_post callback will be properly parsed.
• Protobuf changes are consistent — field 10 for provider_user_id uses readStringRequireUtf8 matching all other string fields, and the field number correctly follows the existing sequence (with field 8 intentionally skipped).
• findByScan multi-key support cleanly extends the Firestore query builder for the new { providerUserId, provider } compound lookups.
• The createResetPasswordHandler correctly returns 200 even on failure to prevent email enumeration, and the Firebase REST client already silently handles EMAIL_NOT_FOUND.

Overall Correctness Verdict

Correct. The patch is free of bugs and blocking issues. Existing code and tests should not break.

[claude]

Code Review

[P1] TOCTOU race in OAuth state validation allows token replay

File: src/server/auth/oauth-handlers.ts, lines 99-107 (Google) and 278-286 (Apple)

validate() and invalidate() are separate non-atomic operations on the Firestore state document. Between the validate check returning { valid: true } and the invalidate deletion, a concurrent request replaying the same callback URL can also pass validation. The state store should use an atomic validate-and-delete (e.g., a Firestore transaction that reads, checks, and deletes the document in one shot, returning the stored returnUrl only on success). This matters because the OAuth callback URL — containing the one-time code and state — can appear in browser history, server logs, or referrer headers.

---

[P2] /auth/providers endpoint enables email enumeration

File: src/server/auth/auth-handlers.ts, lines 150-171

The /auth/providers endpoint returns { registered: true/false, providers: [...] } for any email, allowing an attacker to enumerate which emails have accounts and which auth provider they use. The login and reset-password handlers already avoid this (login returns generic 401, reset always returns 200), but this endpoint undoes that protection. If this endpoint is needed for the client UX, consider rate-limiting it or requiring an authenticated session.

---

[P2] No email_verified guard before account lookup/creation in OAuth flows

File: src/server/auth/oauth-handlers.ts, lines 112-142 (Google) and 319-366 (Apple)

In both OAuth callback handlers, the code proceeds to look up or create a user by email without checking that the email is verified. For Google, userInfo.email_verified is available but unchecked before getUserByEmail / createUser. For Apple, claims.email_verified is similarly available but not gated on. If an OAuth provider returns an unverified email that matches an existing user's email, getOrCreateUserFromVerifiedInfo will link the OAuth identity to that user, potentially enabling account takeover. Adding an if (!email_verified) { redirect with error } guard before the email-based user lookup would close this.

---

[P2] Hardcoded Firebase API key in server source

File: src/server/app.ts, line 248

The Firebase API key AIzaSyConH72HQl9xOtjmYJO9o2kQ9nZZzl96G8 is hardcoded directly in the server source. While Firebase API keys are designed to be client-visible, hardcoding them server-side makes rotation harder and mixes deployment configuration with code. This key was not present in the base branch — it should come from the existing config/environment infrastructure (e.g., config/default.json or an environment variable), matching how other secrets like OAuth clientSecret are handled.

---

Overall Correctness Verdict

Not correct. The TOCTOU race on state validation (P1) is a real security gap that should be fixed before merging — an atomic validate-and-delete is straightforward with Firestore transactions. The email enumeration and missing email_verified checks (P2) are also worth addressing. The hardcoded API key is lower priority but should use the config system for consistency.

[claude]

Code Review: server: move Firebase auth to server-side endpoints

I reviewed all 31 changed files (~4,500 additions) covering the migration from client-side Firebase SDK authentication to server-side auth endpoints.

Architecture

The migration is well-structured:

• New src/server/auth/ module with clean separation: REST client, handlers, OAuth handlers, state management, URL validation
• Dependency injection via typed *Deps interfaces makes the handlers testable
• The client is simplified significantly — Firebase SDK removed, login flows now use same-origin fetch() calls to the server

Security

The security posture is solid:

• CSRF protection: Firestore-backed OAuth state tokens with 10-minute TTL
• Open redirect prevention: validateReturnUrl rejects dangerous protocols, protocol-relative URLs, backslashes, and enforces same-host for absolute URLs
• Apple ID token verification: Proper JWKS-based cryptographic verification via jose, with key rotation retry
• Email enumeration prevention: Password reset returns 200 regardless of whether the email exists
• Disabled account checks: Consistently checked across all auth paths, including the Apple no-email fallback with a "fail closed" strategy
• Defense-in-depth: State invalidation in catch blocks, body ID token only used after cryptographic verification

Edge Cases

The Apple Sign-In "no email on subsequent logins" case is handled well via resolveAppleUserWithoutEmail, with a reasonable fallback chain (providerUserId → Firebase provider link → reject).

The provider migration logic in getOrCreateUserFromVerifiedInfo correctly allows OAuth providers to subsume password accounts while protecting existing OAuth provider links from being overwritten.

Findings

No bugs found. The code is correct, well-tested, and handles the numerous edge cases in OAuth authentication appropriately.

Overall Correctness Verdict

Correct. Existing code and tests will not break. The migration is clean — existing server sessions (Passport + seshcookie) remain the source of truth, so users with valid session cookies will stay logged in after deployment. The new protobuf field (provider_user_id = 10) is additive and backwards-compatible with existing serialized data.

[claude]

Code Review

I reviewed this PR thoroughly, focusing on security, correctness, and the interaction between new and existing code.

Summary

This is a well-structured migration of Firebase authentication from client-side SDK to server-side endpoints. The implementation demonstrates careful attention to security:

• CSRF protection: Double-layer defense using Firestore-backed state tokens paired with binding cookies (timing-safe comparison via timingSafeEqual)
• Open redirect prevention: validateReturnUrl correctly handles dangerous protocols, protocol-relative URLs, backslash tricks, host mismatches, and header injection via newlines
• Apple ID token verification: Proper JWKS-based JWT verification with cache-refresh retry on key rotation
• Fail-closed design: resolveAppleUserWithoutEmail rejects login when Firebase status cannot be verified rather than defaulting to allow
• Middleware ordering: cookieParser, seshcookie, and body parsers are all mounted before the auth router, so cookies and form POST bodies are available when needed

Findings

No bugs found. The code handles the security-sensitive OAuth and authentication flows correctly. Edge cases like Apple omitting email on subsequent logins, duplicate temp-user recovery, and provider migration (password → OAuth) are handled with appropriate care.

Overall Correctness Verdict

Correct. The patch is free of bugs and blocking issues. Existing code and tests should not break — the old client-side Firebase SDK usage and the POST /session endpoint are removed in lockstep with the new server-side endpoints, and session serialization/deserialization via Passport remains intact.