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.59%. Comparing base (e70a6d8) to head (8cd08bd).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #232   +/-   ##
=======================================
  Coverage   76.59%   76.59%           
=======================================
  Files          71       71           
  Lines       20505    20505           
=======================================
  Hits        15706    15706           
  Misses       4799     4799           

☔ 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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 918b181556

ℹ️ 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 multi-key findOneByScan call that throws at runtime

This query passes two keys into findOneByScan, but FirestoreTable.findByScan only accepts a single key and throws findByScan: expected single query key. In production (non-mocked) tables this will cause Google/Apple OAuth logins to fail before any fallback logic runs, because the exception is not handled in a way that recovers. Consider adding a table API that supports multi-field queries (or a dedicated indexed field for provider+id) and updating this lookup to use it.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[claude]

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

This is a substantial and well-executed authentication refactor. The migration from client-side Firebase SDK to server-side endpoints is thorough, with excellent test coverage and security considerations. Below is my detailed review.

---

✅ Strengths

1. Security Improvements

• Strong CSRF protection via Firestore-backed OAuth state storage with 10-minute TTL
• Comprehensive URL validation in url-validation.ts prevents open redirect attacks (checks dangerous protocols, protocol/host matching, backslash/newline injection)
• Cryptographic verification of Apple ID tokens using JWKS with 1-hour caching
• Proper disabled account checking across all auth flows
• Password whitespace preservation (commit 19f5cb3) - important security fix

2. Test Coverage

• Excellent test coverage across 7 test files with ~2,500+ lines of tests
• Tests are well-structured with proper mocking and comprehensive scenarios
• Edge cases covered: disabled users, missing emails, returning Apple users, email normalization
• Tests verify both happy paths and error conditions

3. Code Quality

• Clean separation of concerns (auth-handlers, oauth-handlers, token-exchange, state management)
• Proper TypeScript typing throughout
• Good error handling with custom FirebaseAuthError class
• User-friendly error messages mapped from Firebase error codes

4. Progressive Enhancement

• Maintains backward compatibility with deprecated /session endpoint for existing mobile apps
• Migration path for pre-providerUserId users via Firebase provider fallback (commit db27462)
• Graceful degradation in error scenarios

---

🔴 Critical Issues

1. Race Condition in User Creation (authn.ts:119-168)

The getOrCreateUserFromIdToken and getOrCreateUserFromVerifiedInfo functions have a potential race condition:

user = await users.findOneByScan({ email });
if (!user) {
  user = new User();
  user.setId(`temp-${uuidV4()}`);
  // ...
  await users.create(user.getId(), user);
}

If two requests for the same new user arrive simultaneously, both could pass the findOneByScan check and attempt to create the user, causing a conflict. While the subsequent error handling at line 145 tries to clean this up, this is a band-aid fix rather than a proper solution.

Recommendation: Use a transaction or Firestore's create() with a unique constraint to make the check-and-create atomic. Alternatively, use the email as the document ID (with proper escaping) to leverage Firestore's native document-level consistency.

2. Insecure Dependency Injection Pattern (oauth-token-exchange.ts:74)

const crypto = require('crypto');

This require() inside a function is unusual and potentially problematic:

• Not type-safe (TypeScript can't verify the crypto import)
• Could theoretically be monkey-patched at runtime
• Inconsistent with the ES6 import style used everywhere else

Recommendation: Move to top-level import: import * as crypto from 'crypto';

3. Missing Index Warning (table-firestore.ts:76-80)

The multi-key query implementation chains .where() clauses without composite index support:

let firestoreQuery: Query = this.collection;
for (const key of keys) {
  firestoreQuery = firestoreQuery.where(key, '==', query[key]);
}

This will fail at runtime with a missing index error for queries like { providerUserId: '...', provider: 'apple' } unless composite indexes are manually created.

Recommendation: Add a comment explaining that composite indexes must be created manually for multi-field queries, or include index configuration in firestore.indexes.json.

---

⚠️ Potential Issues

4. Silent Password Reset Errors (auth-handlers.ts:199)

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

This hides actual errors from users. While this prevents email enumeration attacks, it could cause confusion if the reset fails for legitimate reasons (API quota, network issues, etc.).

Recommendation: Consider a middle ground - log the specific error internally but still return success to the user. Add monitoring alerts for persistent failures.

5. Incomplete Type Safety (oauth-handlers.ts:129)

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

This type assertion is unsafe. Firebase Admin SDK errors have a well-defined structure.

Recommendation: Import and use proper Firebase error types:

import { FirebaseError } from 'firebase-admin';
// ...
if (err instanceof FirebaseError && err.code === 'auth/user-not-found')

6. Memory Leak Risk (oauth-token-exchange.ts:135-137)

The JWKS cache uses module-level variables without bounds:

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

While the 1-hour TTL prevents stale data, there's no mechanism to clear very old cache entries if keys rotate.

Recommendation: This is likely fine for now, but consider using a proper cache library like node-cache if you add more cached resources.

7. Hardcoded 10-Minute Timeout (oauth-state.ts:21)

const DEFAULT_TTL_MS = 10 * 60 * 1000; // 10 minutes

This may be too short for users who are interrupted during OAuth flow (e.g., switching between password managers, 2FA delays).

Recommendation: Consider increasing to 15-20 minutes or making it configurable.

8. Insufficient Email Validation

The handlers only check for truthy email after trimming. They don't validate email format, which could lead to poor user experience or unexpected Firebase errors.

Recommendation: Add basic email format validation (regex) before calling Firebase API.

---

🟡 Code Quality Suggestions

9. Inconsistent Error Handling Pattern

Some handlers use explicit status codes with descriptive error objects:

res.status(400).json({ error: 'Missing state parameter' });

While others use a switch statement for error code mapping (auth-handlers.ts:21-38). Consider standardizing.

10. Magic Numbers

• expiresIn = 15777000 (oauth-token-exchange.ts:76) - Use a constant like APPLE_CLIENT_SECRET_TTL_SECONDS
• 100 * 1024 (table-firestore.ts:122) - Define as MAX_JSON_DEBUG_SIZE_BYTES

11. Console.log Debugging (authn.ts:278, Login.tsx:122, 140)

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

These should use the winston logger for consistency and proper log levels.

12. Commented-out Code (table-firestore.ts:130-138)

The hoistColumns implementation is commented out. Either implement it or remove it entirely.

---

🔒 Security Considerations

13. Jose Downgrade Justification (commit 42ab764)

The downgrade from jose v6 to v4 for CommonJS compatibility is reasonable but worth monitoring:

• v4 may have known vulnerabilities that v6 patches
• Consider migrating the server to ESM in the future to use latest jose

Recommendation: Add a TODO comment and check v4 CVE status.

14. Provider Enumeration Risk (auth-handlers.ts:161-183)

The /auth/providers endpoint reveals which OAuth providers a user has connected. This is a minor information disclosure that could aid targeted phishing.

Mitigation: This is probably acceptable given the UX benefits, but consider rate limiting this endpoint.

---

📊 Performance Considerations

15. Sequential Firebase Calls

In oauth-handlers.ts, the flow makes sequential calls:

1. Get/create Firebase user
2. Get/create local user
3. Update session

Recommendation: Consider batching where possible, though the current approach is clearer and errors are easier to handle sequentially.

16. Firestore Scan Performance (authn.ts:121)

user = await users.findOneByScan({ email });

Without an index on email, this could be slow. Ensure the email field is indexed in Firestore.

---

🧪 Test Coverage Gaps

17. Missing Integration Tests for Full OAuth Flow

While auth-integration.test.ts exists (108 lines), it's unclear if it covers the full OAuth flow end-to-end with real Firestore emulator and session management.

Recommendation: Add tests that verify the entire flow including state creation, callback, and session establishment.

18. Missing Error Scenario Tests

Some scenarios lack test coverage:

• Network failures during token exchange
• Firestore unavailability during state storage
• Session storage failures
• Concurrent user creation race conditions

---

📝 Documentation

19. Missing API Documentation

The new auth endpoints lack OpenAPI/Swagger documentation. This would help frontend developers and future maintainers.

20. Configuration Documentation

The config/default.json changes aren't documented. Add comments explaining required OAuth config fields.

---

✨ Positive Highlights Worth Calling Out

1. Excellent commit hygiene - Each commit has a clear atomic purpose with detailed commit messages explaining the "why"
2. Progressive bug fixing - The series of follow-up commits (af73774, 67b46c8, 84e3ef7, etc.) shows thorough testing and edge case discovery
3. Client bundle reduction - The 80-200KB savings is significant for initial page load
4. Type safety - Comprehensive TypeScript usage throughout
5. Provider abstraction - The AuthProvider type and provider scoping makes future provider additions easier

---

🎯 Summary & Recommendation

Overall Assessment: APPROVE with minor fixes recommended

This is high-quality work that significantly improves the architecture and security posture of the authentication system. The test coverage is exemplary, and the progressive bug fixes show thorough validation.

Must Fix Before Merge:

1. Issue editor overlay #2: Move crypto import to top-level
2. Issue server-side support #3: Add Firestore index documentation/configuration

Should Fix Soon After Merge:

1. Issue edit variable names #1: Race condition in user creation (needs architectural discussion)
2. Issue when moving objects w/ links, maintain relative link angle #8: Add email format validation
3. Issue detach + move links #11: Replace console.log with winston logger
4. Issue create clouds, ordinarys, stocks #13: Check jose v4 for CVEs, add migration TODO

Nice to Have:

• Increase OAuth state TTL to 15-20 minutes
• Add OpenAPI documentation for new endpoints
• Add comprehensive integration tests for OAuth flows

Great work on this migration! The code quality and attention to security detail are excellent.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 30ef1ccc72

ℹ️ 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 disabled Apple accounts in no-email path

When Apple omits email (common on subsequent logins), this branch logs the local user in immediately based on providerUserId without checking Firebase’s disabled flag. That means an account disabled in Firebase can still authenticate via Apple as long as it has a stored providerUserId, which bypasses the fbUser?.disabled guard used in the email-present path. Consider fetching the Firebase user by provider UID (or UID stored on the local user) and rejecting if disabled before calling loginUser.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[claude]

Code Review: PR #232 - Firebase Authentication Migration

Summary

This PR successfully migrates Firebase authentication from client-side SDK to server-side REST API endpoints, implementing email/password auth and OAuth (Google/Apple) flows with proper security controls. The implementation includes comprehensive test coverage and follows security best practices for CSRF protection, token handling, and open redirect prevention.

Overall Assessment: This is a well-architected implementation with strong security foundations and excellent test coverage. However, there are several critical issues and important suggestions that should be addressed before merging.

---

Strengths

1. Comprehensive Security Controls

   • CSRF protection via cryptographically random state tokens stored in Firestore
   • Open redirect prevention with strict URL validation
   • Apple ID token verification using JWKS with proper signature validation
   • Proper handling of disabled accounts across all authentication paths

2. Excellent Test Coverage

   • Unit tests cover happy paths, error cases, edge cases, and security scenarios
   • Tests validate security properties (e.g., disabled user rejection, state validation)
   • Mock implementations are thorough and well-structured
   • Apple-specific quirks are tested (email_verified as string, omitted email on repeat logins)

3. Code Quality

   • Clean separation of concerns (handlers, token exchange, state management, validation)
   • Dependency injection pattern makes code testable
   • TypeScript types are well-defined
   • Error handling is consistent with appropriate HTTP status codes

4. OAuth Migration Path

   • Thoughtful handling of existing users via providerUserId migration
   • Fallback logic for pre-migration Apple users
   • Cross-provider collision prevention (providerUserId + provider composite key)

---

Critical Issues

1. Race Condition in OAuth State Stores (CRITICAL)

File: src/server/auth/auth-router.ts

Lines 56 and 71 create separate state stores for Google and Apple handlers:

// Line 56 - creates NEW state store for Google
const stateStore = createFirestoreStateStore(deps.firestore);

// Line 71 - creates ANOTHER state store for Apple
const stateStore = createFirestoreStateStore(deps.firestore);

Problem: If a user initiates Google OAuth but an attacker intercepts and attempts to replay with Apple OAuth callback (or vice versa), the state validation could succeed because both use the same Firestore collection. While this wouldn't allow account takeover (email validation would fail), it's a defense-in-depth violation.

Recommendation: Either:

• Share a single state store instance between providers, OR
• Use different Firestore collections per provider (oauth_state_google, oauth_state_apple)

2. Missing Firestore Composite Index (CRITICAL)

File: src/server/models/table-firestore.ts:76-80

The code correctly passes { providerUserId: claims.sub, provider: 'apple' } to prevent cross-provider collisions, but this requires a Firestore composite index on (providerUserId, provider) to work efficiently. Without this index:

• Queries will fail or be very slow
• The security property (preventing collisions) may not work as intended

Recommendation:

• Add Firestore composite index configuration to your infrastructure
• Document index requirements in deployment guide
• Consider adding an integration test that verifies the index exists

3. Information Disclosure Prevention (HIGH - Actually Correct!)

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

Password reset always returns 200, even when email doesn't exist. This is correct behavior to prevent email enumeration, but should be documented.

Recommendation: Add a comment explaining why we return 200 even on error to prevent future "fixes" that reintroduce the vulnerability:

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

4. Password Handling Documentation (MEDIUM)

File: src/server/auth/auth-handlers.ts:55, 107

While the test at line 183 validates that passwords are NOT trimmed (which is correct), there's no explicit comment warning future developers.

Recommendation: Add inline comment:

// DO NOT trim - leading/trailing spaces are valid in passwords
const password = typeof req.body?.password === 'string' ? req.body.password : '';

---

Important Suggestions

1. Missing Rate Limiting (HIGH)

Issue: No rate limiting on authentication endpoints. This allows:

• Brute force attacks on /auth/login
• OAuth state exhaustion attacks on initiate endpoints
• Email enumeration via /auth/providers timing

Recommendation: Implement rate limiting middleware (e.g., express-rate-limit) on:

• /auth/login - 5 attempts per 15 minutes per IP
• /auth/signup - 3 attempts per hour per IP
• /auth/providers - 10 attempts per minute per IP
• OAuth initiate endpoints - 20 attempts per minute per IP

2. Session Security Configuration (MEDIUM)

File: src/server/auth/auth-handlers.ts:40-50

Recommendation: Verify that session cookies have:

• httpOnly: true (prevent XSS)
• secure: true (HTTPS only in production)
• sameSite: 'lax' or 'strict' (CSRF protection)
• Reasonable maxAge (e.g., 7 days)

3. OAuth State Cleanup (MEDIUM)

Issue: No automated cleanup of expired state documents in Firestore. Current approach may accumulate garbage over time.

Recommendation: Consider:

• Firestore TTL policy (if available)
• Scheduled Cloud Function to delete expired states

4. Token Exchange Error Handling (MEDIUM)

Files: src/server/auth/oauth-token-exchange.ts:50-53, 128-130

Raw error text from OAuth provider is included in error message, which may be logged or exposed.

Recommendation:

• Parse and sanitize error responses
• Define typed error classes
• Never include raw provider error details in user-facing responses

5. Apple User Name Parsing (LOW)

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

Silent failure on JSON parse errors could hide bugs.

Recommendation: Log parse errors at debug level for troubleshooting.

---

Test Coverage Assessment

Excellent coverage overall. Tests validate:

• ✅ Input validation (missing/empty fields)
• ✅ Authentication flows (login, signup, OAuth)
• ✅ Error handling (Firebase errors, network errors)
• ✅ Security controls (disabled users, state validation, URL validation)
• ✅ Edge cases (email trimming, Apple email_verified as string)
• ✅ Migration paths (providerUserId backfilling)

Missing test coverage:

1. Concurrent state access - Test that OAuth state can't be reused across providers
2. Session persistence - Test that req.login() actually creates a valid session
3. Firestore index requirements - Integration test for composite queries
4. Token expiration - Test handling of expired ID tokens
5. Network timeouts - Test behavior when OAuth providers are slow/unresponsive

---

Performance Considerations

1. JWKS Caching: 1-hour cache is reasonable. The code already exports clearJwksCache() which is good for testing.

2. Firestore Queries: The composite index on (providerUserId, provider) is essential for performance at scale.

3. OAuth State Cleanup: Consider adding automated cleanup to prevent accumulation.

---

Security Summary

Strengths:

• CSRF protection via state tokens
• Open redirect prevention
• Cryptographic token verification
• Disabled account checking

Vulnerabilities to Address:

1. Missing Firestore composite index (could cause auth failures)
2. Potential OAuth state reuse across providers (defense in depth)
3. No rate limiting (enables brute force)
4. Missing session security configuration verification

Risk Level: Medium - The core auth flows are secure, but production deployment requires additional hardening (rate limiting, indexes, monitoring).

---

Final Recommendations

Before merging:

1. Fix OAuth state store isolation (use provider-specific collections OR share a single instance)
2. Document Firestore index requirements in deployment guide
3. Add inline comments for password trimming and email enumeration prevention
4. Verify session cookie security configuration

Post-merge (high priority):

1. Add rate limiting middleware
2. Implement OAuth state cleanup (TTL or scheduled job)
3. Add integration tests for database queries
4. Set up monitoring/alerting for auth failures

Post-merge (medium priority):

1. Add typed errors for OAuth token exchange
2. Add debug logging for Apple user name parsing failures
3. Document Apple/Google OAuth setup in deployment guide

---

The implementation demonstrates strong engineering practices and security awareness. With the recommended fixes, this will be a production-ready authentication system. Excellent work on the comprehensive test coverage and thoughtful handling of edge cases!

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 31789a753c

ℹ️ 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]

Preserve current page in OAuth redirect

The OAuth initiation links drop the current location, so Google/Apple logins always return to / (see create*OAuthInitiateHandler, which defaults to returnUrl undefined and validateReturnUrl returns /). This is a regression versus the prior Firebase signInWithRedirect behavior, which returns to the same URL, and it breaks flows like starting login from /new (user ends up at / instead of continuing project creation). Consider including a returnUrl query param (e.g., window.location.pathname + window.location.search) when navigating to /auth/google or /auth/apple.

Useful? React with 👍 / 👎.

[bpowers]

@codex review

[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".