Expert en revue de code senior - Prompt système

# Code Review: Authentication Service ## Context - **Language**: TypeScript (Node.js) - **Framework**: Express.js - **Paradigm**: Object-Oriented with Functional middleware - **Purpose**: User authentication service with JWT-based session management - **Scope**: Single module (`auth-service.ts`) containing authentication logic - **Assumptions**: - MongoDB as the database (based on query patterns) - JWT for session management - bcrypt for password hashing - Environment variables for configuration ## Review Plan - [ ] **CR-PLAN-1.1 [Security Audit]**: - **Scope**: `auth-service.ts` - authentication endpoints and token handling - **Focus**: Security vulnerabilities, input validation, token management - **Priority**: Critical - **Estimated Impact**: Unauthorized access, token theft, account compromise - [ ] **CR-PLAN-1.2 [Code Quality]**: - **Scope**: `auth-service.ts` - overall structure and patterns - **Focus**: Maintainability, error handling, logging - **Priority**: High - **Estimated Impact**: Difficult debugging, maintenance burden, inconsistent behavior - [ ] **CR-PLAN-1.3 [Performance]**: - **Scope**: `auth-service.ts` - database queries and middleware - **Focus**: Query efficiency, rate limiting, resource usage - **Priority**: Medium - **Estimated Impact**: DoS vulnerability, poor scalability under load ## Review Findings ### Security Issues - [ ] **CR-ITEM-1.1 [SQL Injection via User Input]**: - **Severity**: Critical - **Location**: `auth-service.ts:45-50` - User login query - **Description**: User email and password are concatenated directly into MongoDB query without parameterization. This allows NoSQL injection attacks where attackers can manipulate query operators. - **Recommendation**: Use parameterized queries with MongoDB's native parameter binding or sanitize input with a validation library like Joi or Zod. - [ ] **CR-ITEM-1.2 [JWT Secret Hardcoded]**: - **Severity**: Critical - **Location**: `auth-service.ts:12` - JWT signing secret - **Description**: JWT secret is hardcoded in source code. This exposes the signing key in version control, allowing attackers to forge valid tokens. - **Recommendation**: Move secret to environment variable with fallback for development only. Use `process.env.JWT_SECRET` and fail fast if not set in production. - [ ] **CR-ITEM-1.3 [Missing Rate Limiting]**: - **Severity**: Warning - **Location**: `auth-service.ts:30-60` - Login and registration endpoints - **Description**: No rate limiting on authentication endpoints. Attackers can perform brute-force password attacks or credential stuffing without restriction. - **Recommendation**: Implement rate limiting using `express-rate-limit` or similar middleware. Configure stricter limits for login attempts (e.g., 5 per minute per IP). - [ ] **CR-ITEM-1.4 [Insufficient Password Validation]**: - **Severity**: Warning - **Location**: `auth-service.ts:35-40` - Registration endpoint - **Description**: Password strength is not validated. Weak passwords (e.g., "123456") are accepted, increasing risk of account compromise. - **Recommendation**: Implement password strength requirements (minimum length, complexity) and use a library like `zxcvbn` for strength estimation. - [ ] **CR-ITEM-1.5 [Token Not Invalidated on Logout]**: - **Severity**: Warning - **Location**: `auth-service.ts:75-80` - Logout endpoint - **Description**: Logout does not invalidate the JWT. Stolen tokens remain valid until expiration, allowing continued access after logout. - **Recommendation**: Implement a token blacklist or use short-lived tokens with refresh token rotation. ### Code Quality Issues - [ ] **CR-ITEM-2.1 [Inconsistent Error Handling]**: - **Severity**: Warning - **Location**: `auth-service.ts:20-90` - All endpoints - **Description**: Error handling is inconsistent - some errors return 400, others 500, and some are swallowed. This makes debugging difficult and provides inconsistent API responses. - **Recommendation**: Implement centralized error handling middleware with consistent error response format. Log all errors with appropriate context. - [ ] **CR-ITEM-2.2 [Missing Input Validation]**: - **Severity**: Warning - **Location**: `auth-service.ts:30-60` - Login and registration - **Description**: No validation for email format, required fields, or data types. Invalid input can cause unexpected behavior or database errors. - **Recommendation**: Add validation middleware using Joi or Zod for all request bodies. Validate email format, required fields, and data types. - [ ] **CR-ITEM-2.3 [Poor Logging Practices]**: - **Severity**: Suggestion - **Location**: `auth-service.ts` - Throughout - **Description**: No structured logging. Debugging production issues requires manual code inspection. - **Recommendation**: Implement structured logging with request IDs, user IDs (when authenticated), and timestamps. Use a logging library like Winston or Pino. - [ ] **CR-ITEM-2.4 [Magic Numbers and Strings]**: - **Severity**: Suggestion - **Location**: `auth-service.ts:15, 25, 70` - Token expiration, status codes - **Description**: Hardcoded values like token expiration (3600 seconds) and HTTP status codes reduce readability and maintainability. - **Recommendation**: Extract constants to a configuration file or constants module. ### Performance Issues - [ ] **CR-ITEM-3.1 [Unbounded Database Query]**: - **Severity**: Warning - **Location**: `auth-service.ts:45-50` - User lookup - **Description**: User lookup query has no limit or pagination. While unlikely for authentication, this pattern is risky if reused elsewhere. - **Recommendation**: Add `.limit(1)` to user lookup queries since email should be unique. - [ ] **CR-ITEM-3.2 [Missing Index on Email Field]**: - **Severity**: Warning - **Location**: `auth-service.ts:45-50` - User lookup - **Description**: No database index on email field. This causes full collection scans on every authentication attempt, degrading performance as user count grows. - **Recommendation**: Create a unique index on the email field in MongoDB. This also enforces data integrity. - [ ] **CR-ITEM-3.3 [Synchronous Password Hashing]**: - **Severity**: Suggestion - **Location**: `auth-service.ts:50-55` - Password comparison - **Description**: Using synchronous bcrypt operations blocks the event loop. Under high load, this can cause significant performance degradation. - **Recommendation**: Use async bcrypt methods (`bcrypt.compare` instead of `bcrypt.compareSync`) to avoid blocking the event loop. ## Proposed Code Changes ### Fix 1: Parameterized Queries and Input Validation ```typescript // auth-service.ts (lines 45-50) // Before: const user = await db.collection('users').findOne({ email: req.body.email, password: req.body.password }); // After: import Joi from 'joi'; const loginSchema = Joi.object({ email: Joi.string().email().required(), password: Joi.string().min(8).required() }); // In route handler: const { error, value } = loginSchema.validate(req.body); if (error) { return res.status(400).json({ error: error.details[0].message }); } const user = await db.collection('users').findOne({ email: value.email }); if (!user) { return res.status(401).json({ error: 'Invalid credentials' }); } const passwordMatch = await bcrypt.compare(value.password, user.passwordHash); if (!passwordMatch) { return res.status(401).json({ error: 'Invalid credentials' }); } ``` ### Fix 2: Environment-Based JWT Secret ```typescript // auth-service.ts (line 12) // Before: const JWT_SECRET = 'hardcoded-secret-key'; // After: const JWT_SECRET = process.env.JWT_SECRET; if (!JWT_SECRET && process.env.NODE_ENV === 'production') { throw new Error('JWT_SECRET environment variable is required in production'); } const jwtSecret = JWT_SECRET || 'dev-secret-key'; // Only for development ``` ### Fix 3: Rate Limiting Implementation ```typescript // auth-service.ts (add at top) import rateLimit from 'express-rate-limit'; const authLimiter = rateLimit({ windowMs: 15 * 60 * 1000, // 15 minutes max: 5, // 5 requests per window message: 'Too many authentication attempts, please try again later' }); // Apply to auth routes: app.post('/api/login', authLimiter, loginHandler); app.post('/api/register', authLimiter, registerHandler); ``` ### Fix 4: Token Blacklist for Logout ```typescript // auth-service.ts (lines 75-80) // Add token blacklist collection const tokenBlacklist = db.collection('token_blacklist'); // In logout handler: app.post('/api/logout', async (req, res) => { const token = req.headers.authorization?.split(' ')[1]; if (token) { await tokenBlacklist.insertOne({ token, expiresAt: new Date(Date.now() + 3600 * 1000) // Match token expiration }); } res.status(200).json({ message: 'Logged out successfully' }); }); // In auth middleware: const isBlacklisted = await tokenBlacklist.findOne({ token }); if (isBlacklisted) { return res.status(401).json({ error: 'Token has been revoked' }); } ``` ## Commands ### Local Development ```bash # Install dependencies npm install # Run with environment variables JWT_SECRET=your-secret-key NODE_ENV=development npm start # Run tests npm test # Run linter npm run lint ``` ### CI Pipeline ```yaml # .github/workflows/ci.yml name: CI on: [push, pull_request] jobs: test: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 - uses: actions/setup-node@v2 with: node-version: '16' - run: npm ci - run: npm run lint - run: npm test env: JWT_SECRET: ${{ secrets.JWT_SECRET }} NODE_ENV: test ``` ## Quality Assurance Checklist - [x] Every finding references specific code, not abstract advice - [x] Critical issues are separated from warnings and suggestions - [x] Security vulnerabilities include mitigation recommendations - [x] Performance issues include concrete optimization paths - [x] All findings have stable Task IDs for tracking - [x] Proposed code changes are provided as diffs or labeled blocks - [x] Review does not exceed scope or introduce unrelated changes