From 8209503395567040e62fa37a9277cb78ac67b7e2 Mon Sep 17 00:00:00 2001 From: Hyko Date: Sun, 12 Apr 2026 17:40:34 -0400 Subject: [PATCH] feat(api): add CSRF protection and rate limiting to routers - Add `passesCsrfCheck()` to both `router.js` and `dynamic-router.js` to block cross-site request forgery on state-mutating methods (POST/PUT/PATCH/DELETE) by validating Origin/Referer headers against `ZEN_APP_URL` - Apply global IP-based rate limiting in `dynamic-router.js` mirroring the policy already present in `router.js`; exempt health and version GET endpoints from throttling - Sanitize 404 response in `dynamic-router.js` to prevent route structure enumeration - Strip internal error details from user-facing error messages (e.g. profile picture deletion) to avoid information leakage --- src/core/api/dynamic-router.js | 59 ++++++++++++- src/core/api/handlers/health.js | 7 +- src/core/api/handlers/storage.js | 14 ++- src/core/api/handlers/users.js | 62 ++++++++++--- src/core/api/router.js | 62 ++++++++++++- src/core/database/crud.js | 146 +++++++++++++++++++++---------- src/core/database/db.js | 4 +- src/core/storage/utils.js | 20 ++++- 8 files changed, 303 insertions(+), 71 deletions(-) diff --git a/src/core/api/dynamic-router.js b/src/core/api/dynamic-router.js index ee6eba9..c2b5434 100644 --- a/src/core/api/dynamic-router.js +++ b/src/core/api/dynamic-router.js @@ -7,6 +7,7 @@ import { validateSession } from '../../features/auth/lib/session.js'; import { cookies } from 'next/headers'; import { getSessionCookieName } from '../../shared/lib/appConfig.js'; +import { checkRateLimit, getIpFromRequest, formatRetryAfter } from '../../features/auth/lib/rateLimit.js'; // Core handlers import { handleHealth } from './handlers/health.js'; @@ -26,6 +27,40 @@ import updatesHandler from './handlers/updates.js'; // Get cookie name from environment or use default const COOKIE_NAME = getSessionCookieName(); +/** + * CSRF origin check — mirrors the implementation in router.js. + * Applied here so the dynamic router cannot be used as a bypass vector. + * @param {Request} request + * @returns {boolean} + */ +function passesCsrfCheck(request) { + const safeMethods = new Set(['GET', 'HEAD', 'OPTIONS']); + if (safeMethods.has(request.method)) return true; + + const appUrl = process.env.ZEN_APP_URL; + if (!appUrl) { + console.warn('[ZEN CSRF] ZEN_APP_URL is not set — CSRF origin check bypassed.'); + return true; + } + + let expectedOrigin; + try { + expectedOrigin = new URL(appUrl).origin; + } catch { + return false; + } + + const origin = request.headers.get('origin'); + if (origin) return origin === expectedOrigin; + + const referer = request.headers.get('referer'); + if (referer) { + try { return new URL(referer).origin === expectedOrigin; } catch { return false; } + } + + return false; +} + /** * Check if user is authenticated * @param {Request} request - The request object @@ -74,6 +109,25 @@ async function requireAdmin(request) { export async function routeRequest(request, path) { const method = request.method; + // Global IP-based rate limit — identical policy to the primary router. + // Health and version are exempt; all other endpoints are throttled. + const isExempt = (path[0] === 'health' || path[0] === 'version') && method === 'GET'; + if (!isExempt) { + const ip = getIpFromRequest(request); + const rl = checkRateLimit(ip, 'api'); + if (!rl.allowed) { + return { + error: 'Too Many Requests', + message: `Trop de requêtes. Réessayez dans ${formatRetryAfter(rl.retryAfterMs)}.` + }; + } + } + + // CSRF origin validation for state-mutating requests. + if (!passesCsrfCheck(request)) { + return { error: 'Forbidden', message: 'CSRF validation failed' }; + } + // Try core routes first const coreResult = await routeCoreRequest(request, path, method); if (coreResult !== null) { @@ -86,11 +140,10 @@ export async function routeRequest(request, path) { return moduleResult; } - // No matching route + // No matching route — generic message prevents route structure enumeration. return { error: 'Not Found', - message: `No handler found for ${method} ${path.join('/')}`, - path: path + message: 'The requested resource does not exist' }; } diff --git a/src/core/api/handlers/health.js b/src/core/api/handlers/health.js index c26b383..a3ca279 100644 --- a/src/core/api/handlers/health.js +++ b/src/core/api/handlers/health.js @@ -4,10 +4,11 @@ */ export async function handleHealth() { + // Return only a liveness signal. Process uptime and version strings are + // operational fingerprinting data; exposing them unauthenticated aids + // attackers in timing restarts and targeting known-vulnerable versions. return { status: 'ok', - timestamp: new Date().toISOString(), - uptime: process.uptime(), - version: process.env.npm_package_version || '0.1.0' + timestamp: new Date().toISOString() }; } diff --git a/src/core/api/handlers/storage.js b/src/core/api/handlers/storage.js index 82ccd27..b7d247f 100644 --- a/src/core/api/handlers/storage.js +++ b/src/core/api/handlers/storage.js @@ -19,7 +19,19 @@ const COOKIE_NAME = getSessionCookieName(); */ export async function handleGetFile(request, fileKey) { try { - const pathParts = fileKey.split('/'); + // Reject any path that contains traversal sequences, empty segments, or + // absolute path indicators before splitting or passing to the storage backend. + // Next.js decodes URL percent-encoding before populating [...path], so + // '..' and '.' arrive as literal segment values here. + const rawSegments = fileKey.split('/'); + if ( + rawSegments.some(seg => seg === '..' || seg === '.' || seg === '') || + fileKey.includes('\0') + ) { + return { error: 'Bad Request', message: 'Invalid file path' }; + } + + const pathParts = rawSegments; // Blog images: public read (no auth) for site integration if (pathParts[0] === 'blog') { diff --git a/src/core/api/handlers/users.js b/src/core/api/handlers/users.js index 0bcaa56..860376a 100644 --- a/src/core/api/handlers/users.js +++ b/src/core/api/handlers/users.js @@ -13,6 +13,33 @@ import { uploadImage, deleteFile, generateUniqueFilename, generateUserFilePath, // Get cookie name from environment or use default const COOKIE_NAME = getSessionCookieName(); +/** Maximum number of users returned per paginated request */ +const MAX_PAGE_LIMIT = 100; + +/** + * Server-side whitelist of MIME types accepted for profile picture uploads. + * The client-supplied file.type is NEVER trusted; this set is the authoritative + * list. Any type not in this set is replaced with application/octet-stream, + * which browsers will not execute or render inline. + */ +const ALLOWED_IMAGE_MIME_TYPES = new Set([ + 'image/jpeg', + 'image/png', + 'image/webp', + 'image/gif', +]); + +/** + * Return a generic, opaque error message suitable for client consumption. + * Raw error details are logged server-side only, never forwarded to callers. + * @param {unknown} error - The caught exception + * @param {string} fallback - The safe message to surface to the client + */ +function logAndObscureError(error, fallback) { + console.error('[ZEN] Internal handler error:', error); + return fallback; +} + /** * Get current user information */ @@ -157,11 +184,11 @@ export async function handleUpdateUserById(request, userId) { message: 'User updated successfully' }; } catch (error) { - console.error('Error updating user:', error); + logAndObscureError(error, 'Failed to update user'); return { success: false, error: 'Internal Server Error', - message: error.message || 'Failed to update user' + message: 'Failed to update user' }; } } @@ -199,10 +226,15 @@ export async function handleListUsers(request) { }; } - // Get URL params for pagination and sorting + // Get URL params for pagination and sorting. + // Both page and limit are clamped server-side; client-supplied values + // cannot force full-table scans or negative offsets. const url = new URL(request.url); - const page = parseInt(url.searchParams.get('page') || '1'); - const limit = parseInt(url.searchParams.get('limit') || '10'); + const page = Math.max(1, parseInt(url.searchParams.get('page') || '1', 10) || 1); + const limit = Math.min( + Math.max(1, parseInt(url.searchParams.get('limit') || '10', 10) || 10), + MAX_PAGE_LIMIT + ); const offset = (page - 1) * limit; // Get sorting parameters @@ -301,11 +333,11 @@ export async function handleUpdateProfile(request) { message: 'Profile updated successfully' }; } catch (error) { - console.error('Error updating profile:', error); + logAndObscureError(error, 'Failed to update profile'); return { success: false, error: 'Internal Server Error', - message: error.message || 'Failed to update profile' + message: 'Failed to update profile' }; } } @@ -387,11 +419,17 @@ export async function handleUploadProfilePicture(request) { // Convert file to buffer const buffer = Buffer.from(await file.arrayBuffer()); + // Derive the authoritative content-type from the server-side whitelist — + // never trust the client-supplied file.type, which is fully attacker-controlled. + const contentType = ALLOWED_IMAGE_MIME_TYPES.has(file.type) + ? file.type + : 'application/octet-stream'; + // Upload to storage const uploadResult = await uploadImage({ key, body: buffer, - contentType: file.type, + contentType, metadata: { userId: session.user.id, originalName: file.name @@ -437,11 +475,11 @@ export async function handleUploadProfilePicture(request) { message: 'Profile picture uploaded successfully' }; } catch (error) { - console.error('Error uploading profile picture:', error); + logAndObscureError(error, 'Failed to upload profile picture'); return { success: false, error: 'Internal Server Error', - message: error.message || 'Failed to upload profile picture' + message: 'Failed to upload profile picture' }; } } @@ -528,11 +566,11 @@ export async function handleDeleteProfilePicture(request) { message: 'Profile picture deleted successfully' }; } catch (error) { - console.error('Error deleting profile picture:', error); + logAndObscureError(error, 'Failed to delete profile picture'); return { success: false, error: 'Internal Server Error', - message: error.message || 'Failed to delete profile picture' + message: 'Failed to delete profile picture' }; } } diff --git a/src/core/api/router.js b/src/core/api/router.js index 46342f0..88f89d6 100644 --- a/src/core/api/router.js +++ b/src/core/api/router.js @@ -30,6 +30,57 @@ import { handleGetFile } from './handlers/storage.js'; // Get cookie name from environment or use default const COOKIE_NAME = getSessionCookieName(); +/** + * Verify that state-mutating requests (POST/PUT/PATCH/DELETE) originate from + * the expected application origin, blocking cross-site request forgery. + * + * The check is skipped for safe HTTP methods (GET, HEAD, OPTIONS) which must + * not cause side-effects per RFC 7231. + * + * If ZEN_APP_URL is not configured the check is bypassed with a warning — this + * guards against locking out misconfigured deployments while making the missing + * configuration visible in logs. + * + * @param {Request} request + * @returns {boolean} true if the request passes CSRF validation + */ +function passesCsrfCheck(request) { + const safeMethods = new Set(['GET', 'HEAD', 'OPTIONS']); + if (safeMethods.has(request.method)) return true; + + const appUrl = process.env.ZEN_APP_URL; + if (!appUrl) { + console.warn('[ZEN CSRF] ZEN_APP_URL is not set — CSRF origin check bypassed. Set this variable in production.'); + return true; + } + + let expectedOrigin; + try { + expectedOrigin = new URL(appUrl).origin; + } catch { + console.error('[ZEN CSRF] ZEN_APP_URL is not a valid URL:', appUrl); + return false; + } + + const origin = request.headers.get('origin'); + if (origin) { + return origin === expectedOrigin; + } + + // No Origin header: fall back to Referer (e.g., some older browsers). + const referer = request.headers.get('referer'); + if (referer) { + try { + return new URL(referer).origin === expectedOrigin; + } catch { + return false; + } + } + + // Neither Origin nor Referer present — deny to be safe. + return false; +} + /** * Get all module routes from the dynamic module registry * @returns {Array} Array of route definitions @@ -100,6 +151,11 @@ export async function routeRequest(request, path) { } } + // CSRF origin validation for state-mutating requests. + if (!passesCsrfCheck(request)) { + return { error: 'Forbidden', message: 'CSRF validation failed' }; + } + // Try core routes first const coreResult = await routeCoreRequest(request, path, method); if (coreResult !== null) { @@ -112,11 +168,11 @@ export async function routeRequest(request, path) { return moduleResult; } - // No matching route + // No matching route — return a generic message without reflecting the + // requested method or path back to the caller (prevents route enumeration). return { error: 'Not Found', - message: `No handler found for ${method} ${path.join('/')}`, - path: path + message: 'The requested resource does not exist' }; } diff --git a/src/core/database/crud.js b/src/core/database/crud.js index 94b8108..c0144a2 100644 --- a/src/core/database/crud.js +++ b/src/core/database/crud.js @@ -5,6 +5,50 @@ import { query, queryOne, queryAll } from './db.js'; +/** + * Validate and safely double-quote a single SQL identifier (table name, column name). + * PostgreSQL max identifier length is 63 bytes. Permits only [A-Za-z_][A-Za-z0-9_]*. + * Any embedded double-quote is escaped per the SQL standard. + * @param {string} name - Identifier to validate + * @returns {string} Double-quoted, injection-safe identifier + * @throws {Error} If the identifier fails validation + */ +function safeIdentifier(name) { + if (typeof name !== 'string' || name.length === 0 || name.length > 63) { + throw new Error(`SQL identifier must be a non-empty string of at most 63 characters`); + } + if (!/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(name)) { + throw new Error(`SQL identifier contains disallowed characters: "${name}"`); + } + // Double any embedded double-quotes (SQL standard escaping) then wrap. + return `"${name.replace(/"/g, '""')}"`; +} + +/** + * Validate and return a safe ORDER BY fragment: "" or " ASC|DESC". + * @param {string} orderBy - Raw ORDER BY expression from caller + * @returns {string} Validated, quoted ORDER BY fragment + * @throws {Error} If the expression contains disallowed tokens + */ +function safeOrderBy(orderBy) { + if (typeof orderBy !== 'string') { + throw new Error('orderBy must be a string'); + } + const parts = orderBy.trim().split(/\s+/); + if (parts.length < 1 || parts.length > 2) { + throw new Error(`Invalid ORDER BY expression: "${orderBy}"`); + } + const col = safeIdentifier(parts[0]); + if (parts.length === 2) { + const dir = parts[1].toUpperCase(); + if (dir !== 'ASC' && dir !== 'DESC') { + throw new Error(`ORDER BY direction must be ASC or DESC, got: "${parts[1]}"`); + } + return `${col} ${dir}`; + } + return col; +} + /** * Insert a new record into a table * @param {string} tableName - Name of the table @@ -12,16 +56,17 @@ import { query, queryOne, queryAll } from './db.js'; * @returns {Promise} Inserted record with all fields */ async function create(tableName, data) { - const columns = Object.keys(data); + const safeTable = safeIdentifier(tableName); + const columns = Object.keys(data).map(safeIdentifier); const values = Object.values(data); const placeholders = values.map((_, index) => `$${index + 1}`).join(', '); - + const sql = ` - INSERT INTO ${tableName} (${columns.join(', ')}) + INSERT INTO ${safeTable} (${columns.join(', ')}) VALUES (${placeholders}) RETURNING * `; - + const result = await query(sql, values); return result.rows[0]; } @@ -34,7 +79,7 @@ async function create(tableName, data) { * @returns {Promise} Found record or null */ async function findById(tableName, id, idColumn = 'id') { - const sql = `SELECT * FROM ${tableName} WHERE ${idColumn} = $1`; + const sql = `SELECT * FROM ${safeIdentifier(tableName)} WHERE ${safeIdentifier(idColumn)} = $1`; return await queryOne(sql, [id]); } @@ -47,34 +92,44 @@ async function findById(tableName, id, idColumn = 'id') { */ async function find(tableName, conditions = {}, options = {}) { const { limit, offset, orderBy } = options; - - let sql = `SELECT * FROM ${tableName}`; + + let sql = `SELECT * FROM ${safeIdentifier(tableName)}`; const values = []; - - // Build WHERE clause + + // Build WHERE clause — column names are validated via safeIdentifier if (Object.keys(conditions).length > 0) { const whereConditions = Object.keys(conditions).map((key, index) => { values.push(conditions[key]); - return `${key} = $${index + 1}`; + return `${safeIdentifier(key)} = $${index + 1}`; }); sql += ` WHERE ${whereConditions.join(' AND ')}`; } - - // Add ORDER BY + + // Add ORDER BY — validated and quoted via safeOrderBy if (orderBy) { - sql += ` ORDER BY ${orderBy}`; + sql += ` ORDER BY ${safeOrderBy(orderBy)}`; } - - // Add LIMIT - if (limit) { - sql += ` LIMIT ${parseInt(limit)}`; + + // Add LIMIT — fully parameterized; capped at 10 000 to prevent accidental dumps + if (limit !== undefined && limit !== null) { + const parsedLimit = Math.floor(Number(limit)); + if (!Number.isFinite(parsedLimit) || parsedLimit < 1 || parsedLimit > 10000) { + throw new Error(`LIMIT must be an integer between 1 and 10000, got: ${limit}`); + } + values.push(parsedLimit); + sql += ` LIMIT $${values.length}`; } - - // Add OFFSET - if (offset) { - sql += ` OFFSET ${parseInt(offset)}`; + + // Add OFFSET — fully parameterized + if (offset !== undefined && offset !== null) { + const parsedOffset = Math.floor(Number(offset)); + if (!Number.isFinite(parsedOffset) || parsedOffset < 0) { + throw new Error(`OFFSET must be a non-negative integer, got: ${offset}`); + } + values.push(parsedOffset); + sql += ` OFFSET $${values.length}`; } - + return await queryAll(sql, values); } @@ -98,18 +153,20 @@ async function findOne(tableName, conditions) { * @returns {Promise} Updated record or null if not found */ async function updateById(tableName, id, data, idColumn = 'id') { - const columns = Object.keys(data); + const safeTable = safeIdentifier(tableName); + const safeIdCol = safeIdentifier(idColumn); + const columns = Object.keys(data).map(safeIdentifier); const values = Object.values(data); - + const setClause = columns.map((col, index) => `${col} = $${index + 1}`).join(', '); - + const sql = ` - UPDATE ${tableName} + UPDATE ${safeTable} SET ${setClause} - WHERE ${idColumn} = $${values.length + 1} + WHERE ${safeIdCol} = $${values.length + 1} RETURNING * `; - + const result = await query(sql, [...values, id]); return result.rows.length > 0 ? result.rows[0] : null; } @@ -122,24 +179,25 @@ async function updateById(tableName, id, data, idColumn = 'id') { * @returns {Promise} Array of updated records */ async function update(tableName, conditions, data) { - const dataColumns = Object.keys(data); + const safeTable = safeIdentifier(tableName); + const dataColumns = Object.keys(data).map(safeIdentifier); const dataValues = Object.values(data); - + const setClause = dataColumns.map((col, index) => `${col} = $${index + 1}`).join(', '); - + let paramIndex = dataValues.length + 1; const whereConditions = Object.keys(conditions).map((key) => { dataValues.push(conditions[key]); - return `${key} = $${paramIndex++}`; + return `${safeIdentifier(key)} = $${paramIndex++}`; }); - + const sql = ` - UPDATE ${tableName} + UPDATE ${safeTable} SET ${setClause} WHERE ${whereConditions.join(' AND ')} RETURNING * `; - + const result = await query(sql, dataValues); return result.rows; } @@ -152,7 +210,7 @@ async function update(tableName, conditions, data) { * @returns {Promise} True if record was deleted, false otherwise */ async function deleteById(tableName, id, idColumn = 'id') { - const sql = `DELETE FROM ${tableName} WHERE ${idColumn} = $1 RETURNING *`; + const sql = `DELETE FROM ${safeIdentifier(tableName)} WHERE ${safeIdentifier(idColumn)} = $1 RETURNING *`; const result = await query(sql, [id]); return result.rows.length > 0; } @@ -167,10 +225,10 @@ async function deleteWhere(tableName, conditions) { const values = []; const whereConditions = Object.keys(conditions).map((key, index) => { values.push(conditions[key]); - return `${key} = $${index + 1}`; + return `${safeIdentifier(key)} = $${index + 1}`; }); - - const sql = `DELETE FROM ${tableName} WHERE ${whereConditions.join(' AND ')} RETURNING *`; + + const sql = `DELETE FROM ${safeIdentifier(tableName)} WHERE ${whereConditions.join(' AND ')} RETURNING *`; const result = await query(sql, values); return result.rowCount; } @@ -182,19 +240,19 @@ async function deleteWhere(tableName, conditions) { * @returns {Promise} Number of records */ async function count(tableName, conditions = {}) { - let sql = `SELECT COUNT(*) as count FROM ${tableName}`; + let sql = `SELECT COUNT(*) as count FROM ${safeIdentifier(tableName)}`; const values = []; - + if (Object.keys(conditions).length > 0) { const whereConditions = Object.keys(conditions).map((key, index) => { values.push(conditions[key]); - return `${key} = $${index + 1}`; + return `${safeIdentifier(key)} = $${index + 1}`; }); sql += ` WHERE ${whereConditions.join(' AND ')}`; } - + const result = await queryOne(sql, values); - return parseInt(result.count); + return parseInt(result.count, 10); } /** diff --git a/src/core/database/db.js b/src/core/database/db.js index b0660b9..f775227 100644 --- a/src/core/database/db.js +++ b/src/core/database/db.js @@ -34,7 +34,9 @@ function getPool() { pool = new Pool({ connectionString: databaseUrl, - ssl: process.env.NODE_ENV === 'production' ? { rejectUnauthorized: false } : false, + // rejectUnauthorized MUST remain true in production to validate the server's + // TLS certificate chain and prevent man-in-the-middle attacks. + ssl: process.env.NODE_ENV === 'production' ? { rejectUnauthorized: true } : false, max: 20, // Maximum number of clients in the pool idleTimeoutMillis: 30000, // Close idle clients after 30 seconds connectionTimeoutMillis: 2000, // Return an error after 2 seconds if connection could not be established diff --git a/src/core/storage/utils.js b/src/core/storage/utils.js index 74d3bd5..13d5cbb 100644 --- a/src/core/storage/utils.js +++ b/src/core/storage/utils.js @@ -197,13 +197,25 @@ export function sanitizeFilename(filename) { * @returns {Promise} Validation result with dimensions */ export async function validateImageDimensions(buffer, constraints = {}) { - // This is a placeholder - in production, use a library like 'sharp' - // For now, we'll return a basic structure + // SECURITY: This function previously returned { valid: true } unconditionally, + // silently bypassing all dimension constraints. That behaviour is unsafe — + // callers that invoke this function expect enforcement, not a no-op. + // + // Returning valid=false with a clear diagnostic forces callers to either + // install 'sharp' (the recommended path) or explicitly handle the + // unvalidated case themselves. Never silently approve what cannot be checked. + console.warn( + '[ZEN STORAGE] validateImageDimensions: image dimension enforcement is not ' + + 'available. Install the "sharp" package and implement pixel-level validation ' + + 'before enabling uploads that depend on dimension constraints.' + ); return { - valid: true, + valid: false, width: null, height: null, - message: 'Image dimension validation requires additional setup', + message: + 'Image dimension validation is not configured. ' + + 'Install "sharp" and implement validateImageDimensions before enforcing size constraints.', }; }