Pull Request Regression and Security Review

SECURITY CODE REVIEW - AUTH REFACTOR PR

REPO: internal/auth-v2 | PR #487 | Files changed: 18 | +342 -156

CRITICAL ISSUES (must fix before merge):

1. TOKEN EXPIRY NOT ENFORCED
   File: lib/auth/jwt-verify.ts, line 78
   Issue: exp claim checked but not revoked on server
   Risk: Leaked token remains valid until TTL expires (7 days)
   Severity: CRITICAL
   Fix: Implement token blacklist (Redis cache) on logout
   Code pattern:
     Before: if (payload.exp > now) { validate }
     After: if (payload.exp > now && !blacklist.has(token)) { validate }
   Estimated fix time: 2 hours

2. PASSWORD RESET TOKEN UNENCRYPTED IN URL
   File: handlers/reset-password.ts, line 45
   Issue: Reset token passed as query param, visible in logs and browser history
   Risk: Account takeover if server logs compromised
   Severity: HIGH
   Recommendation: POST-only flow with encrypted short-lived cookie
   Example: POST /reset?code=abc123 (code expires in 1 hour)
   Estimated fix time: 3 hours

HIGH PRIORITY ISSUES:

3. MISSING RATE LIMITING ON LOGIN
   File: handlers/login.ts (entire endpoint)
   Issue: No brute force protection
   Current: Account guessable with 10K attempts/second
   Add: Token bucket (5 attempts per IP per 15 min)
   Estimated fix time: 1 hour

4. BCRYPT ROUNDS TOO LOW
   File: lib/crypto/hash.ts, line 12
   Current code: bcrypt.hash(password, 10)
   Issue: 10 rounds = 100ms per check, vulnerable to GPU attacks
   Recommended: 12 rounds (400ms per check, still user-acceptable)
   Estimated fix time: 15 minutes

5. SESSION HIJACKING RISK
   File: middleware/session.ts, line 67
   Issue: No user-agent/IP validation on session cookie
   Risk: Compromised token allows access from any device/location
   Add: Hash(user_agent + ip) stored in session, verified on request
   Estimated fix time: 90 minutes

MEDIUM PRIORITY ISSUES:

6. CSRF PROTECTION INCONSISTENT
   File: routes/api/* (multiple)
   Issue: Some endpoints use csrf middleware, others don't
   Pattern: POST /auth/verify missing token validation
   Fix: Audit all state-changing routes, add middleware globally
   Estimated fix time: 4 hours

7. MISSING SECRETS IN GITIGNORE
   File: .env.example
   Issue: JWT_SECRET present in example (should be docs only)
   Risk: Accidental commit of real secrets if team copies example
   Recommendation: Move to SECRETS.md (not in repo), use only variable names
   Estimated fix time: 20 minutes

LOW PRIORITY / BEST PRACTICES:

8. NO AUDIT LOGGING
   File: handlers/login.ts
   Note: Consider logging failed auth attempts for security monitoring
   Nice-to-have, not blocking

9. REDIRECT AFTER LOGIN UNVALIDATED
   File: handlers/callback.ts, line 92
   Issue: ?redirect=http://attacker.com not validated
   Risk: Phishing attacks
   Fix: Whitelist allowed domains before redirect
   Estimated fix time: 30 minutes

COMPLIANCE NOTES:
  ✓ WCAG auth flow compliant (keyboard navigable)
  ✓ No hardcoded secrets in code
  ✓ Uses industry-standard libraries (not custom crypto)
  ~ Missing GDPR audit trail for data deletion on account close

TEST COVERAGE AUDIT:
  Current: 62% for auth module
  Missing: Brute force tests, token expiry, session hijacking scenarios
  Recommendation: Add 8 security-specific test cases before deploy

SUMMARY:
  5 Critical/High issues blocking merge
  Estimated remediation: 11 hours of engineering
  Recommended: Do NOT merge without addressing items 1-3
  Timeline: 1-2 days to fix all issues

NEXT STEPS:
  1. Assign item 1 (token blacklist) highest priority
  2. Run OWASP dependency check (npm audit)
  3. Schedule follow-up security review after fixes
  4. Plan penetration test before production deploy