feat: implement authentication

[enko]

Refs #7

[claude]

PR Review: Authentication Implementation (Updated Review)

Overview

This PR implements a comprehensive authentication system including user registration, login, logout, session management, and password reset functionality. The implementation shows strong security practices, excellent test coverage, and proper TypeScript usage.

Changes: 57 files changed, 6,706 additions, 204 deletions

---

🚨 Critical Issues (MUST FIX Before Merge)

1. Password Reset Token Exposure - SEVERE SECURITY VULNERABILITY

Location: apps/backend/src/routes/auth.ts:280-284
Severity: CRITICAL ⚠️

return c.json({
  message: 'If the email exists, a password reset link has been sent',
  // REMOVE THIS IN PRODUCTION - only for testing
  resetToken,  // ⚠️ TOKEN EXPOSED IN API RESPONSE
});

Issue: The actual password reset token is returned in the API response, completely bypassing the intended security mechanism where tokens should only be delivered via email.

Attack Scenario:

1. Attacker calls POST /api/auth/forgot-password with victim's email
2. Receives valid reset token directly in JSON response (no email access needed!)
3. Uses token to reset password via POST /api/auth/reset-password
4. Gains full account access

Fix Required:

// RECOMMENDED: Remove entirely for production
return c.json({
  message: 'If the email exists, a password reset link has been sent',
});

// ALTERNATIVE: Dev-only (acceptable for MVP/testing phase)
return c.json({
  message: 'If the email exists, a password reset link has been sent',
  ...(process.env.NODE_ENV === 'development' && { resetToken }),
});

---

2. Raw SQL Bypasses Type Safety - VIOLATES PROJECT STANDARDS

Location: apps/backend/src/services/auth.service.ts:347-350
Severity: HIGH

await this.db.query(
  'UPDATE auth.users SET password_hash = $1, updated_at = CURRENT_TIMESTAMP WHERE external_id = $2',
  [passwordHash, user.external_id],
);

Issue: Direct SQL query violates CLAUDE.md requirement: "All queries must be written in .sql files for PgTyped type generation". This is the only instance of raw SQL in the entire codebase - all other queries properly use PgTyped.

Risks:

• No compile-time type checking
• Inconsistent with established codebase patterns
• Harder to maintain and refactor

Fix Required: Create PgTyped query in apps/backend/src/models/queries/users.sql:

/* @name UpdateUserPassword */
UPDATE auth.users
SET password_hash = :passwordHash,
    updated_at = CURRENT_TIMESTAMP
WHERE external_id = :externalId
RETURNING external_id, email, created_at, updated_at;

Then use:

await updateUserPassword.run(
  { externalId: user.external_id, passwordHash },
  this.db
);

---

3. Passwords Logged in Plaintext - SECURITY/PRIVACY VIOLATION

Locations: Multiple files (6+ instances)
Severity: HIGH

Found in:

• apps/backend/src/routes/auth.ts:38 - Registration (logs password)
• apps/backend/src/routes/auth.ts:105 - Login (logs password)
• apps/backend/src/routes/auth.ts:200 - Refresh request
• apps/backend/src/routes/auth.ts:262 - Forgot password request
• apps/backend/src/routes/auth.ts:311 - Reset password (logs new password)
• apps/backend/src/routes/users.ts:66 - Update profile request

Example:

logger.warn({ body, errors: validated }, 'Invalid registration request');
// Logs: {"body":{"email":"user@example.com","password":"SuperSecret123"},"msg":"..."}

Issue: Application logs will contain plaintext passwords, which violates security best practices and privacy regulations (GDPR, etc.). Log files are often:

• Stored indefinitely
• Backed up to multiple locations
• Accessible by operations teams
• Potentially exposed in error tracking systems (Sentry, DataDog, etc.)

Fix Required: Sanitize sensitive fields before logging:

// Add to apps/backend/src/utils/logger.ts
export function sanitizeBody(body: unknown): unknown {
  if (typeof body \!== 'object' || body === null) return body;
  const sanitized = { ...body };
  const sensitiveFields = ['password', 'newPassword', 'oldPassword'];
  for (const field of sensitiveFields) {
    if (field in sanitized) {
      (sanitized as Record<string, unknown>)[field] = '[REDACTED]';
    }
  }
  return sanitized;
}

// Then use everywhere:
logger.warn({ body: sanitizeBody(body), errors: validated }, 'Invalid registration request');

---

⚠️ High Priority Issues (SHOULD FIX)

4. Missing Rate Limiting

Severity: MEDIUM-HIGH - Security

Issue: No rate limiting on authentication endpoints exposes the application to:

• Brute force attacks on /api/auth/login (unlimited password guessing)
• Account enumeration via timing attacks
• Mass account creation on /api/auth/register
• Email bombing via /api/auth/forgot-password
• Resource exhaustion attacks

Recommendation: Implement rate limiting. Example using hono-rate-limiter or similar:

import { rateLimiter } from 'hono-rate-limiter'

// In routes/auth.ts
app.use('/login', rateLimiter({
  windowMs: 15 * 60 * 1000, // 15 minutes
  max: 5, // 5 attempts per window
  message: 'Too many login attempts, please try again later'
}))

app.use('/register', rateLimiter({
  windowMs: 60 * 60 * 1000, // 1 hour
  max: 3, // 3 registrations per IP/hour
}))

app.use('/forgot-password', rateLimiter({
  windowMs: 60 * 60 * 1000, // 1 hour
  max: 3, // 3 requests per IP/hour
}))

---

5. Weak Password Requirements

Location: packages/shared/src/auth.ts:11
Severity: MEDIUM

export const PasswordSchema = type('string >= 8');

Issue: Only validates minimum length of 8 characters. Allows weak passwords like:

• "aaaaaaaa" (all same character)
• "password" (common dictionary word)
• "12345678" (sequential numbers)

According to NIST guidelines (SP 800-63B), passwords should be at least 12 characters OR require complexity if shorter.

Recommendation: Strengthen password validation:

// Option 1: Increase minimum length (NIST recommended)
export const PasswordSchema = type('string >= 12');

// Option 2: Add complexity requirements for 8-char minimum
export const PasswordSchema = type('string >= 8').narrow((password): password is string => {
  const hasLowercase = /[a-z]/.test(password);
  const hasUppercase = /[A-Z]/.test(password);
  const hasDigit = /[0-9]/.test(password);
  return hasLowercase && hasUppercase && hasDigit;
});

// Option 3: Use password strength library
import zxcvbn from 'zxcvbn';
export const PasswordSchema = type('string >= 8').narrow((password): password is string => {
  return zxcvbn(password).score >= 3; // Strong password
});

---

6. No Cleanup Job for Expired Tokens/Sessions

Severity: MEDIUM - Resource Management

Issue: The deleteExpiredPasswordResetTokens query exists (password-reset-tokens.sql) but is never called. Database will accumulate:

• Expired password reset tokens
• Expired/invalidated sessions
• Orphaned data

Over time, this degrades database performance and wastes storage.

Recommendation: Implement scheduled cleanup job:

// apps/backend/src/jobs/cleanup.ts
import cron from 'node-cron';
import { deleteExpiredPasswordResetTokens } from '../models/queries/password-reset-tokens.queries.js';
import type { Pool } from 'pg';
import type { Logger } from 'pino';

export function startCleanupJobs(db: Pool, logger: Logger) {
  // Run daily at 2 AM
  cron.schedule('0 2 * * *', async () => {
    try {
      await deleteExpiredPasswordResetTokens.run(undefined, db);
      // TODO: Add session cleanup when query is implemented
      logger.info('Cleaned up expired tokens and sessions');
    } catch (error) {
      logger.error({ error }, 'Cleanup job failed');
    }
  });
  
  logger.info('Cleanup jobs scheduled');
}

// Call in src/index.ts after app initialization

---

7. Hardcoded Configuration Values

Severity: LOW-MEDIUM - Maintainability

Issue: Session duration (7 days), cookie settings, and token expiry times are hardcoded in multiple locations, making configuration changes error-prone and inconsistent.

Found in:

• apps/backend/src/routes/auth.ts:56 - session maxAge (7 days)
• apps/backend/src/routes/auth.ts:123 - session maxAge (7 days)
• apps/backend/src/routes/auth.ts:163 - clear cookie settings
• apps/backend/src/routes/auth.ts:230 - clear cookie settings
• Plus JWT expiry, token duration, etc.

Fix: Centralize in configuration:

// apps/backend/src/config/auth.config.ts
export const AUTH_CONFIG = {
  session: {
    durationDays: 7,
    cookieName: 'session_token',
  },
  passwordReset: {
    tokenExpiryHours: 1,
  },
  jwt: {
    expiresIn: '1h',
  },
  cookies: {
    httpOnly: true,
    secure: process.env.NODE_ENV === 'production',
    sameSite: 'Lax' as const,
    path: '/',
  },
} as const;

// Then use:
setCookie(c, AUTH_CONFIG.session.cookieName, result.sessionToken, {
  ...AUTH_CONFIG.cookies,
  maxAge: AUTH_CONFIG.session.durationDays * 24 * 60 * 60,
});

---

📋 Minor Suggestions (Nice to Have)

8. No Automatic Token Refresh in Frontend - The frontend auth store doesn't automatically refresh expired access tokens, potentially causing sudden logouts mid-session
9. Inconsistent Error Response Format - Some endpoints return { error, details }, others just { error } - standardize
10. No Account Lockout Mechanism - Consider temporary lockout after N failed login attempts (works well with rate limiting)
11. Missing Password Strength Indicator UI - RegisterForm.svelte could show visual password strength feedback to users
12. CSRF Token Consideration - While SameSite: 'Lax' provides basic protection, consider CSRF tokens for production
13. No API Documentation - Consider adding OpenAPI/Swagger specification for API endpoints
14. Security Event Logging - Log security events (failed logins, password changes, etc.) to separate audit log for compliance
15. Accessibility Improvements - Add ARIA labels and roles to form inputs for screen reader users
16. Email Verification - No email verification flow implemented (likely deferred to later phase)

---

✅ Strengths (What's Done Well)

1. Excellent Test Coverage - Comprehensive integration tests covering:

   • Happy path scenarios (auth-happy-path.test.ts)
   • Negative test cases (auth-negative.test.ts)
   • Edge cases (auth-edge-cases.test.ts)
   • Password reset flow (auth-password-reset.test.ts)

2. Security-First Approach:

   • ✅ HTTP-only cookies for session tokens
   • ✅ bcrypt for password hashing (10 rounds)
   • ✅ SHA-256 hashing for session/reset tokens stored in DB
   • ✅ Proper session invalidation after password reset
   • ✅ Prevention of user enumeration (forgot-password endpoint)
   • ✅ Database-level unique constraints handled

3. Clean Architecture:

   • Well-separated concerns: routes → services → models
   • Reusable service layer with clear responsibilities
   • Middleware properly implemented

4. Type Safety (mostly):

   • TypeScript strict mode enabled
   • ArkType for runtime validation at API boundaries
   • PgTyped for type-safe database queries (with 1 exception noted above)
   • Shared types package for frontend/backend consistency

5. Professional Code Quality:

   • Structured logging with Pino
   • Proper error handling and user-friendly messages
   • Good code comments and documentation
   • Follows project conventions (mostly)

6. Email Validation - Custom regex pattern prevents XSS via email field

7. Concurrent Registration Handling - Gracefully handles database-level unique constraint violations

---

📊 Architecture Compliance

Requirement	Status	Notes
Monorepo structure	✅ Pass	Correctly uses pnpm workspaces
PgTyped for all queries	⚠️ 1 Violation	One raw SQL in auth.service.ts:347
ArkType validation	✅ Pass	Used at all API boundaries
TypeScript strict mode	✅ Pass	No any types detected
Hono framework	✅ Pass	Properly structured routes
Test coverage	✅ Excellent	Comprehensive integration tests
Security best practices	⚠️ Issues	Token exposure, password logging, no rate limiting
Open standards compliance	⚠️ Partial	JWT used, but not vCard/CalDAV yet (planned for later)

---

🎯 Final Verdict

Recommendation: REQUEST CHANGES

This is a well-engineered authentication system with excellent foundations, comprehensive test coverage, and professional code quality. However, 3 critical security issues must be addressed before merging:

🚨 BLOCKING (Must Fix Before Merge):

1. Remove password reset token from API response - Critical security vulnerability allowing account takeover
2. Replace raw SQL with PgTyped query - Violates project standards and reduces type safety
3. Sanitize passwords from logs - Prevents sensitive data exposure in log files

Estimated Fix Time: 2-4 hours

⚠️ STRONGLY RECOMMENDED (Should Fix Soon):

4. Implement rate limiting on auth endpoints (prevents brute force)
5. Strengthen password requirements (NIST guidelines)
6. Add cleanup job for expired tokens/sessions (prevents DB bloat)
7. Centralize configuration values (maintainability)

Estimated Fix Time: 4-8 hours

📝 OPTIONAL (Future Improvements):

Items 8-16 can be addressed in follow-up PRs

---

📂 Key File References

Critical Security Issues:

• Password reset token exposure: apps/backend/src/routes/auth.ts:280-284
• Raw SQL query: apps/backend/src/services/auth.service.ts:347-350
• Password logging: apps/backend/src/routes/auth.ts:38,105,200,262,311 and apps/backend/src/routes/users.ts:66

Core Implementation:

• Auth routes: apps/backend/src/routes/auth.ts (337 lines)
• Auth service: apps/backend/src/services/auth.service.ts (360 lines)
• Validation schemas: packages/shared/src/auth.ts
• Database queries: apps/backend/src/models/queries/*.sql

Test Coverage:

• apps/backend/tests/integration/auth-happy-path.test.ts (542 lines)
• apps/backend/tests/integration/auth-negative.test.ts (798 lines)
• apps/backend/tests/integration/auth-password-reset.test.ts (794 lines)
• apps/backend/tests/integration/auth-edge-cases.test.ts (682 lines)

---

After addressing the 3 critical issues, this PR will provide a solid, secure, and production-ready authentication foundation for the Personal CRM application. The implementation demonstrates strong engineering practices and attention to detail - just needs those security fixes! 🛡️

---

🤖 Review by Claude Code - PR Review Bot
Review Date: 2025-10-26
Reviewed commit: 741b830

[github-actions]

Automated Code Review - PR #14: feat: implement authentication

Review Status: ⚠️ Issues Found
New Issues This Run: 4 critical, 2 important, 3 suggestions

---

Summary

This is an impressive authentication implementation with strong security fundamentals! The code demonstrates excellent practices: proper use of bcrypt for password hashing, JWT with jti for token uniqueness, PgTyped for SQL injection prevention, rate limiting, and comprehensive test coverage. However, there are several critical security and architecture issues that need attention before merging.

New Critical Issues 🚨

1. Security: Weak Session Token Hashing (SHA-256)

Location: apps/backend/src/utils/auth.ts:97-99

Session tokens are hashed using SHA-256, which is NOT a secure choice for password-like secrets:

export function hashSessionToken(token: string): string {
  return crypto.createHash("sha256").update(token).digest("hex");
}

Why this is critical: SHA-256 is fast, making it vulnerable to brute-force attacks if session token hashes are leaked. While the tokens are 32 random bytes (strong), you should use a slow hashing algorithm like bcrypt.

From CLAUDE.md:

Password hashing with bcrypt (line 184)

The same principle applies to session tokens stored in the database.

Suggested fix:

export async function hashSessionToken(token: string): Promise<string> {
  // Use bcrypt for slow hashing (same as passwords)
  return bcrypt.hash(token, SALT_ROUNDS);
}

Impact: All session token comparisons become await - update:

• apps/backend/src/services/auth.service.ts:103, 194, 288, 312
• Password reset token hashing (uses same function)

---

2. Security: Frontend Authentication Bypass Risk

Location: apps/frontend/src/hooks.server.ts:22-24

The frontend route protection only checks for the presence of a session cookie, without validating it. An attacker can set a fake session_token cookie and bypass frontend route protection.

Suggested fix: Add session validation by calling the backend refresh endpoint in the hook to verify the token is valid before allowing access to protected routes.

---

3. Security: Missing CSRF Protection

Location: All POST endpoints in apps/backend/src/routes/auth.ts

The authentication endpoints lack CSRF (Cross-Site Request Forgery) protection. While using SameSite=Lax cookies provides some protection, its not sufficient for state-changing operations.

From CLAUDE.md Security Principles:

Security by Default - All dependencies reviewed, input validated, SQL injection prevented (line 227)

Suggested fix: Implement CSRF tokens using Hono csrf middleware on all state-changing endpoints.

---

4. Architecture: Type Safety Violation - any Type Usage

Location: Multiple test files use any type

Examples:

• apps/backend/tests/integration/auth-happy-path.test.ts:30 - const body: any = await response.json();

From CLAUDE.md:

No any types - use unknown and narrow types (line 163)

This is explicitly forbidden by project standards.

Suggested fix: Define proper types for test responses using AuthResponse/ErrorResponse from shared package.

---

New Important Issues ⚠️

5. Performance: N+1 Query Pattern in Session Refresh

Location: apps/backend/src/services/auth.service.ts:206-221

The refresh flow makes two sequential database queries. Create a single SQL query with JOIN to reduce database round trips.

---

6. Observability: Sensitive Data in Logs

Location: Multiple places log user emails (e.g., auth.service.ts:63)

Email addresses are PII. GDPR compliance requires minimizing PII in logs. Use userId instead of email in logs, or obfuscate emails for security events.

---

New Suggestions 💡

7. Code Quality: Extract Magic Numbers to Constants

Rate limit values in apps/backend/src/middleware/rate-limit.ts:9-20 are hard-coded. Move to configuration for easier tuning.

---

8. UX: Password Reset Token Exposure

Instead of checking ENV !== production, use a dedicated EXPOSE_RESET_TOKENS config flag for clearer intent.

---

9. Testing: Document Session Limit Behavior

Add test/documentation on whether unlimited sessions per user is intended.

---

Review Statistics

• Files analyzed: 70 changed files
• Lines changed: +7,643 / -1,649
• Test files: 10 backend test files
• SQL Queries: All parameterized (PgTyped) ✓
• Input Validation: ArkType schemas on all endpoints ✓

CLAUDE.md Compliance

• ✅ TypeScript strict mode enabled
• ⚠️ any types found in tests (violation)
• ✅ ArkType runtime validation at API boundaries
• ✅ PgTyped for type-safe SQL queries
• ⚠️ Security gaps: CSRF protection missing, session hash algorithm weak

---

Recommendations Before Merge

Must Address:

1. Fix session token hashing (use bcrypt)
2. Add frontend session validation
3. Implement CSRF protection
4. Remove any types from tests

Should Address:
5. Optimize session refresh query
6. Minimize PII in logs

---

Positive Highlights ⭐

• Comprehensive test coverage
• Security-first design (rate limiting, bcrypt, JWT with jti)
• Type safety (PgTyped, ArkType)
• Clean architecture
• Proper session management

This is a strong foundation! Addressing the critical issues will make this production-ready.

---

Automated review - all issues are NEW findings not previously reported.

[github-actions]

Critical Issue Details:

1. Session Token Hashing (apps/backend/src/utils/auth.ts:97-99) - SHA-256 can compute 1 billion hashes/sec on GPUs. If database leaks, tokens can be brute-forced. Use bcrypt instead.

2. Frontend Session Validation (apps/frontend/src/hooks.server.ts:22) - Only checks cookie existence. Attacker can set fake cookie to access protected UI and leak info.

3. CSRF Protection Missing - State-changing operations vulnerable. SameSite=Lax is insufficient. Add Hono CSRF middleware.

See main review for full details and fixes.

[github-actions]

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀