diff --git a/apps/server/src/lib/auth.ts b/apps/server/src/lib/auth.ts index 145c7b9d..4cf761fd 100644 --- a/apps/server/src/lib/auth.ts +++ b/apps/server/src/lib/auth.ts @@ -2,9 +2,30 @@ * Authentication middleware for API security * * Supports API key authentication via header or environment variable. + * Includes rate limiting to prevent brute-force attacks. */ +import * as crypto from 'crypto'; import type { Request, Response, NextFunction } from 'express'; +import { apiKeyRateLimiter } from './rate-limiter.js'; + +/** + * Performs a constant-time string comparison to prevent timing attacks. + * Uses crypto.timingSafeEqual with proper buffer handling. + */ +function secureCompare(a: string, b: string): boolean { + const bufferA = Buffer.from(a, 'utf8'); + const bufferB = Buffer.from(b, 'utf8'); + + // If lengths differ, we still need to do a constant-time comparison + // to avoid leaking length information. We compare against bufferA twice. + if (bufferA.length !== bufferB.length) { + crypto.timingSafeEqual(bufferA, bufferA); + return false; + } + + return crypto.timingSafeEqual(bufferA, bufferB); +} // API key from environment (optional - if not set, auth is disabled) const API_KEY = process.env.AUTOMAKER_API_KEY; @@ -14,6 +35,7 @@ const API_KEY = process.env.AUTOMAKER_API_KEY; * * If AUTOMAKER_API_KEY is set, requires matching key in X-API-Key header. * If not set, allows all requests (development mode). + * Includes rate limiting to prevent brute-force attacks. */ export function authMiddleware(req: Request, res: Response, next: NextFunction): void { // If no API key is configured, allow all requests @@ -22,6 +44,22 @@ export function authMiddleware(req: Request, res: Response, next: NextFunction): return; } + const clientIp = apiKeyRateLimiter.getClientIp(req); + + // Check if client is rate limited + if (apiKeyRateLimiter.isBlocked(clientIp)) { + const retryAfterMs = apiKeyRateLimiter.getBlockTimeRemaining(clientIp); + const retryAfterSeconds = Math.ceil(retryAfterMs / 1000); + + res.setHeader('Retry-After', retryAfterSeconds.toString()); + res.status(429).json({ + success: false, + error: 'Too many failed authentication attempts. Please try again later.', + retryAfter: retryAfterSeconds, + }); + return; + } + // Check for API key in header const providedKey = req.headers['x-api-key'] as string | undefined; @@ -33,7 +71,10 @@ export function authMiddleware(req: Request, res: Response, next: NextFunction): return; } - if (providedKey !== API_KEY) { + if (!secureCompare(providedKey, API_KEY)) { + // Record failed attempt + apiKeyRateLimiter.recordFailure(clientIp); + res.status(403).json({ success: false, error: 'Invalid API key.', @@ -41,6 +82,9 @@ export function authMiddleware(req: Request, res: Response, next: NextFunction): return; } + // Successful authentication - reset rate limiter for this IP + apiKeyRateLimiter.reset(clientIp); + next(); } diff --git a/apps/server/src/lib/rate-limiter.ts b/apps/server/src/lib/rate-limiter.ts new file mode 100644 index 00000000..afde206b --- /dev/null +++ b/apps/server/src/lib/rate-limiter.ts @@ -0,0 +1,208 @@ +/** + * In-memory rate limiter for authentication endpoints + * + * Provides brute-force protection by tracking failed attempts per IP address. + * Blocks requests after exceeding the maximum number of failures within a time window. + */ + +import type { Request, Response, NextFunction } from 'express'; + +interface AttemptRecord { + count: number; + firstAttempt: number; + blockedUntil: number | null; +} + +interface RateLimiterConfig { + maxAttempts: number; + windowMs: number; + blockDurationMs: number; +} + +const DEFAULT_CONFIG: RateLimiterConfig = { + maxAttempts: 5, + windowMs: 15 * 60 * 1000, // 15 minutes + blockDurationMs: 15 * 60 * 1000, // 15 minutes +}; + +/** + * Rate limiter instance that tracks attempts by a key (typically IP address) + */ +export class RateLimiter { + private attempts: Map = new Map(); + private config: RateLimiterConfig; + + constructor(config: Partial = {}) { + this.config = { ...DEFAULT_CONFIG, ...config }; + } + + /** + * Extract client IP address from request + * Handles proxied requests via X-Forwarded-For header + */ + getClientIp(req: Request): string { + const forwarded = req.headers['x-forwarded-for']; + if (forwarded) { + const forwardedIp = Array.isArray(forwarded) ? forwarded[0] : forwarded.split(',')[0]; + return forwardedIp.trim(); + } + return req.socket.remoteAddress || 'unknown'; + } + + /** + * Check if a key is currently rate limited + */ + isBlocked(key: string): boolean { + const record = this.attempts.get(key); + if (!record) return false; + + const now = Date.now(); + + // Check if currently blocked + if (record.blockedUntil && now < record.blockedUntil) { + return true; + } + + // Clear expired block + if (record.blockedUntil && now >= record.blockedUntil) { + this.attempts.delete(key); + return false; + } + + return false; + } + + /** + * Get remaining time until block expires (in milliseconds) + */ + getBlockTimeRemaining(key: string): number { + const record = this.attempts.get(key); + if (!record?.blockedUntil) return 0; + + const remaining = record.blockedUntil - Date.now(); + return remaining > 0 ? remaining : 0; + } + + /** + * Record a failed authentication attempt + * Returns true if the key is now blocked + */ + recordFailure(key: string): boolean { + const now = Date.now(); + const record = this.attempts.get(key); + + if (!record) { + this.attempts.set(key, { + count: 1, + firstAttempt: now, + blockedUntil: null, + }); + return false; + } + + // If window has expired, reset the counter + if (now - record.firstAttempt > this.config.windowMs) { + this.attempts.set(key, { + count: 1, + firstAttempt: now, + blockedUntil: null, + }); + return false; + } + + // Increment counter + record.count += 1; + + // Check if should be blocked + if (record.count >= this.config.maxAttempts) { + record.blockedUntil = now + this.config.blockDurationMs; + return true; + } + + return false; + } + + /** + * Clear a key's record (e.g., on successful authentication) + */ + reset(key: string): void { + this.attempts.delete(key); + } + + /** + * Get the number of attempts remaining before block + */ + getAttemptsRemaining(key: string): number { + const record = this.attempts.get(key); + if (!record) return this.config.maxAttempts; + + const now = Date.now(); + + // If window expired, full attempts available + if (now - record.firstAttempt > this.config.windowMs) { + return this.config.maxAttempts; + } + + return Math.max(0, this.config.maxAttempts - record.count); + } + + /** + * Clean up expired records to prevent memory leaks + */ + cleanup(): void { + const now = Date.now(); + const keysToDelete: string[] = []; + + this.attempts.forEach((record, key) => { + // Mark for deletion if block has expired + if (record.blockedUntil && now >= record.blockedUntil) { + keysToDelete.push(key); + return; + } + // Mark for deletion if window has expired and not blocked + if (!record.blockedUntil && now - record.firstAttempt > this.config.windowMs) { + keysToDelete.push(key); + } + }); + + keysToDelete.forEach((key) => this.attempts.delete(key)); + } +} + +// Shared rate limiter instances for authentication endpoints +export const apiKeyRateLimiter = new RateLimiter(); +export const terminalAuthRateLimiter = new RateLimiter(); + +// Clean up expired records periodically (every 5 minutes) +setInterval( + () => { + apiKeyRateLimiter.cleanup(); + terminalAuthRateLimiter.cleanup(); + }, + 5 * 60 * 1000 +); + +/** + * Create rate limiting middleware for authentication endpoints + * This middleware checks if the request is rate limited before processing + */ +export function createRateLimitMiddleware(rateLimiter: RateLimiter) { + return (req: Request, res: Response, next: NextFunction): void => { + const clientIp = rateLimiter.getClientIp(req); + + if (rateLimiter.isBlocked(clientIp)) { + const retryAfterMs = rateLimiter.getBlockTimeRemaining(clientIp); + const retryAfterSeconds = Math.ceil(retryAfterMs / 1000); + + res.setHeader('Retry-After', retryAfterSeconds.toString()); + res.status(429).json({ + success: false, + error: 'Too many failed authentication attempts. Please try again later.', + retryAfter: retryAfterSeconds, + }); + return; + } + + next(); + }; +} diff --git a/apps/server/src/middleware/validate-paths.ts b/apps/server/src/middleware/validate-paths.ts index 51b8ccb1..ee1b8931 100644 --- a/apps/server/src/middleware/validate-paths.ts +++ b/apps/server/src/middleware/validate-paths.ts @@ -7,6 +7,29 @@ import type { Request, Response, NextFunction } from 'express'; import { validatePath, PathNotAllowedError } from '@automaker/platform'; +/** + * Custom error for invalid path type + */ +class InvalidPathTypeError extends Error { + constructor(paramName: string, expectedType: string, actualType: string) { + super(`Invalid type for '${paramName}': expected ${expectedType}, got ${actualType}`); + this.name = 'InvalidPathTypeError'; + } +} + +/** + * Validates that a value is a non-empty string suitable for path validation + * + * @param value - The value to check + * @param paramName - The parameter name for error messages + * @throws InvalidPathTypeError if value is not a valid string + */ +function assertValidPathString(value: unknown, paramName: string): asserts value is string { + if (typeof value !== 'string') { + throw new InvalidPathTypeError(paramName, 'string', typeof value); + } +} + /** * Creates a middleware that validates specified path parameters in req.body * @param paramNames - Names of parameters to validate (e.g., 'projectPath', 'worktreePath') @@ -27,7 +50,8 @@ export function validatePathParams(...paramNames: string[]) { if (paramName.endsWith('?')) { const actualName = paramName.slice(0, -1); const value = req.body[actualName]; - if (value) { + if (value !== undefined && value !== null) { + assertValidPathString(value, actualName); validatePath(value); } continue; @@ -37,17 +61,30 @@ export function validatePathParams(...paramNames: string[]) { if (paramName.endsWith('[]')) { const actualName = paramName.slice(0, -2); const values = req.body[actualName]; - if (Array.isArray(values) && values.length > 0) { - for (const value of values) { - validatePath(value); - } + + // Skip if not provided or empty + if (values === undefined || values === null) { + continue; + } + + // Validate that it's actually an array + if (!Array.isArray(values)) { + throw new InvalidPathTypeError(actualName, 'array', typeof values); + } + + // Validate each element in the array + for (let i = 0; i < values.length; i++) { + const value = values[i]; + assertValidPathString(value, `${actualName}[${i}]`); + validatePath(value); } continue; } // Handle regular parameters const value = req.body[paramName]; - if (value) { + if (value !== undefined && value !== null) { + assertValidPathString(value, paramName); validatePath(value); } } @@ -62,6 +99,14 @@ export function validatePathParams(...paramNames: string[]) { return; } + if (error instanceof InvalidPathTypeError) { + res.status(400).json({ + success: false, + error: error.message, + }); + return; + } + // Re-throw unexpected errors throw error; } diff --git a/apps/server/src/routes/settings/routes/get-credentials.ts b/apps/server/src/routes/settings/routes/get-credentials.ts index be15b04b..33c91450 100644 --- a/apps/server/src/routes/settings/routes/get-credentials.ts +++ b/apps/server/src/routes/settings/routes/get-credentials.ts @@ -10,7 +10,7 @@ import type { Request, Response } from 'express'; import type { SettingsService } from '../../../services/settings-service.js'; -import { getErrorMessage, logError } from '../common.js'; +import { logError } from '../common.js'; /** * Create handler factory for GET /api/settings/credentials @@ -29,7 +29,7 @@ export function createGetCredentialsHandler(settingsService: SettingsService) { }); } catch (error) { logError(error, 'Get credentials failed'); - res.status(500).json({ success: false, error: getErrorMessage(error) }); + res.status(500).json({ success: false, error: 'Failed to retrieve credentials' }); } }; } diff --git a/apps/server/src/routes/settings/routes/update-credentials.ts b/apps/server/src/routes/settings/routes/update-credentials.ts index c08b2445..22732c1a 100644 --- a/apps/server/src/routes/settings/routes/update-credentials.ts +++ b/apps/server/src/routes/settings/routes/update-credentials.ts @@ -11,7 +11,71 @@ import type { Request, Response } from 'express'; import type { SettingsService } from '../../../services/settings-service.js'; import type { Credentials } from '../../../types/settings.js'; -import { getErrorMessage, logError } from '../common.js'; +import { logError } from '../common.js'; + +/** Maximum allowed length for API keys to prevent abuse */ +const MAX_API_KEY_LENGTH = 512; + +/** Known API key provider names that are valid */ +const VALID_API_KEY_PROVIDERS = ['anthropic', 'google', 'openai'] as const; + +/** + * Validates that the provided updates object has the correct structure + * and all apiKeys values are strings within acceptable length limits. + * + * @param updates - The partial credentials update object to validate + * @returns An error message if validation fails, or null if valid + */ +function validateCredentialsUpdate(updates: unknown): string | null { + if (!updates || typeof updates !== 'object' || Array.isArray(updates)) { + return 'Invalid request body - expected credentials object'; + } + + const obj = updates as Record; + + // If apiKeys is provided, validate its structure + if ('apiKeys' in obj) { + const apiKeys = obj.apiKeys; + + if (apiKeys === null || apiKeys === undefined) { + // Allow null/undefined to clear + return null; + } + + if (typeof apiKeys !== 'object' || Array.isArray(apiKeys)) { + return 'Invalid apiKeys - expected object'; + } + + const keysObj = apiKeys as Record; + + // Validate each provided API key + for (const [provider, value] of Object.entries(keysObj)) { + // Check provider name is valid + if (!VALID_API_KEY_PROVIDERS.includes(provider as (typeof VALID_API_KEY_PROVIDERS)[number])) { + return `Invalid API key provider: ${provider}. Valid providers: ${VALID_API_KEY_PROVIDERS.join(', ')}`; + } + + // Check value is a string + if (typeof value !== 'string') { + return `Invalid API key for ${provider} - expected string`; + } + + // Check length limit + if (value.length > MAX_API_KEY_LENGTH) { + return `API key for ${provider} exceeds maximum length of ${MAX_API_KEY_LENGTH} characters`; + } + } + } + + // Validate version if provided + if ('version' in obj && obj.version !== undefined) { + if (typeof obj.version !== 'number' || !Number.isInteger(obj.version) || obj.version < 0) { + return 'Invalid version - expected non-negative integer'; + } + } + + return null; +} /** * Create handler factory for PUT /api/settings/credentials @@ -22,16 +86,19 @@ import { getErrorMessage, logError } from '../common.js'; export function createUpdateCredentialsHandler(settingsService: SettingsService) { return async (req: Request, res: Response): Promise => { try { - const updates = req.body as Partial; - - if (!updates || typeof updates !== 'object') { + // Validate the request body before type assertion + const validationError = validateCredentialsUpdate(req.body); + if (validationError) { res.status(400).json({ success: false, - error: 'Invalid request body - expected credentials object', + error: validationError, }); return; } + // Safe to cast after validation + const updates = req.body as Partial; + await settingsService.updateCredentials(updates); // Return masked credentials for confirmation @@ -43,7 +110,7 @@ export function createUpdateCredentialsHandler(settingsService: SettingsService) }); } catch (error) { logError(error, 'Update credentials failed'); - res.status(500).json({ success: false, error: getErrorMessage(error) }); + res.status(500).json({ success: false, error: 'Failed to update credentials' }); } }; } diff --git a/apps/server/src/routes/setup/common.ts b/apps/server/src/routes/setup/common.ts index 097d7a6c..e93edbe6 100644 --- a/apps/server/src/routes/setup/common.ts +++ b/apps/server/src/routes/setup/common.ts @@ -9,6 +9,17 @@ import { getErrorMessage as getErrorMessageShared, createLogError } from '../com const logger = createLogger('Setup'); +/** + * Escapes special regex characters in a string to prevent regex injection. + * This ensures user input can be safely used in RegExp constructors. + * + * @param str - The string to escape + * @returns The escaped string safe for use in RegExp + */ +export function escapeRegExp(str: string): string { + return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + // Storage for API keys (in-memory cache) - private const apiKeys: Record = {}; @@ -33,6 +44,32 @@ export function getAllApiKeys(): Record { return { ...apiKeys }; } +/** + * Escape a value for safe inclusion in a .env file. + * Handles special characters like quotes, newlines, dollar signs, and backslashes. + * Returns a properly quoted string if needed. + */ +function escapeEnvValue(value: string): string { + // Check if the value contains any characters that require quoting + const requiresQuoting = /[\s"'$`\\#\n\r]/.test(value) || value.includes('='); + + if (!requiresQuoting) { + return value; + } + + // Use double quotes and escape special characters within + // Escape backslashes first to avoid double-escaping + let escaped = value + .replace(/\\/g, '\\\\') // Escape backslashes + .replace(/"/g, '\\"') // Escape double quotes + .replace(/\$/g, '\\$') // Escape dollar signs (prevents variable expansion) + .replace(/`/g, '\\`') // Escape backticks + .replace(/\n/g, '\\n') // Escape newlines + .replace(/\r/g, '\\r'); // Escape carriage returns + + return `"${escaped}"`; +} + /** * Helper to persist API keys to .env file */ @@ -47,21 +84,24 @@ export async function persistApiKeyToEnv(key: string, value: string): Promise { if (keyRegex.test(line)) { found = true; - return `${key}=${value}`; + return `${key}=${escapedValue}`; } return line; }); if (!found) { // Add the key at the end - newLines.push(`${key}=${value}`); + newLines.push(`${key}=${escapedValue}`); } await fs.writeFile(envPath, newLines.join('\n')); diff --git a/apps/server/src/routes/setup/routes/api-keys.ts b/apps/server/src/routes/setup/routes/api-keys.ts index d052c187..90c5fde1 100644 --- a/apps/server/src/routes/setup/routes/api-keys.ts +++ b/apps/server/src/routes/setup/routes/api-keys.ts @@ -3,7 +3,7 @@ */ import type { Request, Response } from 'express'; -import { getApiKey, getErrorMessage, logError } from '../common.js'; +import { getApiKey, logError } from '../common.js'; export function createApiKeysHandler() { return async (_req: Request, res: Response): Promise => { @@ -14,7 +14,7 @@ export function createApiKeysHandler() { }); } catch (error) { logError(error, 'Get API keys failed'); - res.status(500).json({ success: false, error: getErrorMessage(error) }); + res.status(500).json({ success: false, error: 'Failed to retrieve API keys status' }); } }; } diff --git a/apps/server/src/routes/setup/routes/delete-api-key.ts b/apps/server/src/routes/setup/routes/delete-api-key.ts index e64ff6b7..32f6834e 100644 --- a/apps/server/src/routes/setup/routes/delete-api-key.ts +++ b/apps/server/src/routes/setup/routes/delete-api-key.ts @@ -11,7 +11,7 @@ const logger = createLogger('Setup'); // In-memory storage reference (imported from common.ts pattern) // We need to modify common.ts to export a deleteApiKey function -import { setApiKey } from '../common.js'; +import { setApiKey, escapeRegExp } from '../common.js'; /** * Remove an API key from the .env file @@ -30,7 +30,7 @@ async function removeApiKeyFromEnv(key: string): Promise { // Parse existing env content and remove the key const lines = envContent.split('\n'); - const keyRegex = new RegExp(`^${key}=`); + const keyRegex = new RegExp(`^${escapeRegExp(key)}=`); const newLines = lines.filter((line) => !keyRegex.test(line)); // Remove empty lines at the end @@ -68,9 +68,10 @@ export function createDeleteApiKeyHandler() { const envKey = envKeyMap[provider]; if (!envKey) { + logger.warn(`[Setup] Unknown provider requested for deletion: ${provider}`); res.status(400).json({ success: false, - error: `Unknown provider: ${provider}. Only anthropic is supported.`, + error: 'Unknown provider. Only anthropic is supported.', }); return; } @@ -94,7 +95,7 @@ export function createDeleteApiKeyHandler() { logger.error('[Setup] Delete API key error:', error); res.status(500).json({ success: false, - error: error instanceof Error ? error.message : 'Failed to delete API key', + error: 'Failed to delete API key', }); } }; diff --git a/apps/server/src/routes/setup/routes/store-api-key.ts b/apps/server/src/routes/setup/routes/store-api-key.ts index e77a697e..0c75d424 100644 --- a/apps/server/src/routes/setup/routes/store-api-key.ts +++ b/apps/server/src/routes/setup/routes/store-api-key.ts @@ -3,7 +3,7 @@ */ import type { Request, Response } from 'express'; -import { setApiKey, persistApiKeyToEnv, getErrorMessage, logError } from '../common.js'; +import { setApiKey, persistApiKeyToEnv, logError } from '../common.js'; import { createLogger } from '@automaker/utils'; const logger = createLogger('Setup'); @@ -30,9 +30,10 @@ export function createStoreApiKeyHandler() { await persistApiKeyToEnv('ANTHROPIC_API_KEY', apiKey); logger.info('[Setup] Stored API key as ANTHROPIC_API_KEY'); } else { + logger.warn(`[Setup] Unsupported provider requested: ${provider}`); res.status(400).json({ success: false, - error: `Unsupported provider: ${provider}. Only anthropic is supported.`, + error: 'Unsupported provider. Only anthropic is supported.', }); return; } @@ -40,7 +41,7 @@ export function createStoreApiKeyHandler() { res.json({ success: true }); } catch (error) { logError(error, 'Store API key failed'); - res.status(500).json({ success: false, error: getErrorMessage(error) }); + res.status(500).json({ success: false, error: 'Failed to store API key' }); } }; } diff --git a/apps/server/src/routes/setup/routes/verify-claude-auth.ts b/apps/server/src/routes/setup/routes/verify-claude-auth.ts index 5debc5c7..421ef237 100644 --- a/apps/server/src/routes/setup/routes/verify-claude-auth.ts +++ b/apps/server/src/routes/setup/routes/verify-claude-auth.ts @@ -10,6 +10,41 @@ import { getApiKey } from '../common.js'; const logger = createLogger('Setup'); +/** + * Simple mutex implementation to prevent race conditions when + * modifying process.env during concurrent verification requests. + * + * The Claude Agent SDK reads ANTHROPIC_API_KEY from process.env, + * so we must temporarily modify it for verification. This mutex + * ensures only one verification runs at a time. + */ +class VerificationMutex { + private locked = false; + private queue: Array<() => void> = []; + + async acquire(): Promise { + return new Promise((resolve) => { + if (!this.locked) { + this.locked = true; + resolve(); + } else { + this.queue.push(resolve); + } + }); + } + + release(): void { + if (this.queue.length > 0) { + const next = this.queue.shift(); + if (next) next(); + } else { + this.locked = false; + } + } +} + +const verificationMutex = new VerificationMutex(); + // Known error patterns that indicate auth failure const AUTH_ERROR_PATTERNS = [ 'OAuth token revoked', @@ -68,14 +103,79 @@ function containsAuthError(text: string): boolean { return AUTH_ERROR_PATTERNS.some((pattern) => lowerText.includes(pattern.toLowerCase())); } +/** Valid authentication method values */ +const VALID_AUTH_METHODS = ['cli', 'api_key'] as const; +type AuthMethod = (typeof VALID_AUTH_METHODS)[number]; + +/** + * Validates and extracts the authMethod from the request body. + * + * @param body - The request body to validate + * @returns The validated authMethod or undefined if not provided + * @throws Error if authMethod is provided but invalid + */ +function validateAuthMethod(body: unknown): AuthMethod | undefined { + if (!body || typeof body !== 'object') { + return undefined; + } + + const obj = body as Record; + + if (!('authMethod' in obj) || obj.authMethod === undefined || obj.authMethod === null) { + return undefined; + } + + const authMethod = obj.authMethod; + + if (typeof authMethod !== 'string') { + throw new Error(`Invalid authMethod type: expected string, got ${typeof authMethod}`); + } + + if (!VALID_AUTH_METHODS.includes(authMethod as AuthMethod)) { + throw new Error( + `Invalid authMethod value: '${authMethod}'. Valid values: ${VALID_AUTH_METHODS.join(', ')}` + ); + } + + return authMethod as AuthMethod; +} + export function createVerifyClaudeAuthHandler() { return async (req: Request, res: Response): Promise => { try { - // Get the auth method from the request body - const { authMethod } = req.body as { authMethod?: 'cli' | 'api_key' }; + // Validate and extract the auth method from the request body + let authMethod: AuthMethod | undefined; + try { + authMethod = validateAuthMethod(req.body); + } catch (validationError) { + res.status(400).json({ + success: false, + authenticated: false, + error: validationError instanceof Error ? validationError.message : 'Invalid request', + }); + return; + } logger.info(`[Setup] Verifying Claude authentication using method: ${authMethod || 'auto'}`); + // Early validation before acquiring mutex - check if API key is needed but missing + if (authMethod === 'api_key') { + const storedApiKey = getApiKey('anthropic'); + if (!storedApiKey && !process.env.ANTHROPIC_API_KEY) { + res.json({ + success: true, + authenticated: false, + error: 'No API key configured. Please enter an API key first.', + }); + return; + } + } + + // Acquire mutex to prevent race conditions when modifying process.env + // The SDK reads ANTHROPIC_API_KEY from environment, so concurrent requests + // could interfere with each other without this lock + await verificationMutex.acquire(); + // Create an AbortController with a 30-second timeout const abortController = new AbortController(); const timeoutId = setTimeout(() => abortController.abort(), 30000); @@ -84,7 +184,7 @@ export function createVerifyClaudeAuthHandler() { let errorMessage = ''; let receivedAnyContent = false; - // Save original env values + // Save original env values (inside mutex to ensure consistency) const originalAnthropicKey = process.env.ANTHROPIC_API_KEY; try { @@ -99,17 +199,8 @@ export function createVerifyClaudeAuthHandler() { if (storedApiKey) { process.env.ANTHROPIC_API_KEY = storedApiKey; logger.info('[Setup] Using stored API key for verification'); - } else { - // Check env var - if (!process.env.ANTHROPIC_API_KEY) { - res.json({ - success: true, - authenticated: false, - error: 'No API key configured. Please enter an API key first.', - }); - return; - } } + // Note: if no stored key, we use the existing env var (already validated above) } // Run a minimal query to verify authentication @@ -129,7 +220,8 @@ export function createVerifyClaudeAuthHandler() { for await (const msg of stream) { const msgStr = JSON.stringify(msg); allMessages.push(msgStr); - logger.info('[Setup] Stream message:', msgStr.substring(0, 500)); + // Debug log only message type to avoid leaking sensitive data + logger.debug('[Setup] Stream message type:', msg.type); // Check for billing errors FIRST - these should fail verification if (isBillingError(msgStr)) { @@ -221,7 +313,8 @@ export function createVerifyClaudeAuthHandler() { } else { // No content received - might be an issue logger.warn('[Setup] No content received from stream'); - logger.warn('[Setup] All messages:', allMessages.join('\n')); + // Log only message count to avoid leaking sensitive data + logger.warn('[Setup] Total messages received:', allMessages.length); errorMessage = 'No response received from Claude. Please check your authentication.'; } } catch (error: unknown) { @@ -277,6 +370,8 @@ export function createVerifyClaudeAuthHandler() { // If we cleared it and there was no original, keep it cleared delete process.env.ANTHROPIC_API_KEY; } + // Release the mutex so other verification requests can proceed + verificationMutex.release(); } logger.info('[Setup] Verification result:', { diff --git a/apps/server/src/routes/terminal/common.ts b/apps/server/src/routes/terminal/common.ts index 6121e345..e698aa8a 100644 --- a/apps/server/src/routes/terminal/common.ts +++ b/apps/server/src/routes/terminal/common.ts @@ -65,6 +65,18 @@ export function cleanupExpiredTokens(): void { // Clean up expired tokens every 5 minutes setInterval(cleanupExpiredTokens, 5 * 60 * 1000); +/** + * Extract Bearer token from Authorization header + * Returns undefined if header is missing or malformed + */ +export function extractBearerToken(req: Request): string | undefined { + const authHeader = req.headers.authorization; + if (!authHeader || !authHeader.startsWith('Bearer ')) { + return undefined; + } + return authHeader.slice(7); // Remove 'Bearer ' prefix +} + /** * Validate a terminal session token */ @@ -116,8 +128,9 @@ export function terminalAuthMiddleware(req: Request, res: Response, next: NextFu return; } - // Check for session token - const token = (req.headers['x-terminal-token'] as string) || (req.query.token as string); + // Extract token from Authorization header only (Bearer token format) + // Query string tokens are not supported due to security risks (URL logging, referrer leakage) + const token = extractBearerToken(req); if (!validateTerminalToken(token)) { res.status(401).json({ diff --git a/apps/server/src/routes/terminal/routes/auth.ts b/apps/server/src/routes/terminal/routes/auth.ts index 1d6156bd..3187a341 100644 --- a/apps/server/src/routes/terminal/routes/auth.ts +++ b/apps/server/src/routes/terminal/routes/auth.ts @@ -1,7 +1,9 @@ /** * POST /auth endpoint - Authenticate with password to get a session token + * Includes rate limiting to prevent brute-force attacks. */ +import * as crypto from 'crypto'; import type { Request, Response } from 'express'; import { getTerminalEnabledConfigValue, @@ -11,6 +13,25 @@ import { getTokenExpiryMs, getErrorMessage, } from '../common.js'; +import { terminalAuthRateLimiter } from '../../../lib/rate-limiter.js'; + +/** + * Performs a constant-time string comparison to prevent timing attacks. + * Uses crypto.timingSafeEqual with proper buffer handling. + */ +function secureCompare(a: string, b: string): boolean { + const bufferA = Buffer.from(a, 'utf8'); + const bufferB = Buffer.from(b, 'utf8'); + + // If lengths differ, we still need to do a constant-time comparison + // to avoid leaking length information. We compare against bufferA twice. + if (bufferA.length !== bufferB.length) { + crypto.timingSafeEqual(bufferA, bufferA); + return false; + } + + return crypto.timingSafeEqual(bufferA, bufferB); +} export function createAuthHandler() { return (req: Request, res: Response): void => { @@ -36,9 +57,28 @@ export function createAuthHandler() { return; } + const clientIp = terminalAuthRateLimiter.getClientIp(req); + + // Check if client is rate limited + if (terminalAuthRateLimiter.isBlocked(clientIp)) { + const retryAfterMs = terminalAuthRateLimiter.getBlockTimeRemaining(clientIp); + const retryAfterSeconds = Math.ceil(retryAfterMs / 1000); + + res.setHeader('Retry-After', retryAfterSeconds.toString()); + res.status(429).json({ + success: false, + error: 'Too many failed authentication attempts. Please try again later.', + retryAfter: retryAfterSeconds, + }); + return; + } + const { password } = req.body; - if (!password || password !== terminalPassword) { + if (!password || !secureCompare(password, terminalPassword)) { + // Record failed attempt + terminalAuthRateLimiter.recordFailure(clientIp); + res.status(401).json({ success: false, error: 'Invalid password', @@ -46,6 +86,9 @@ export function createAuthHandler() { return; } + // Successful authentication - reset rate limiter for this IP + terminalAuthRateLimiter.reset(clientIp); + // Generate session token const token = generateToken(); const now = new Date(); diff --git a/apps/server/src/routes/terminal/routes/logout.ts b/apps/server/src/routes/terminal/routes/logout.ts index 2af85713..dc0d0940 100644 --- a/apps/server/src/routes/terminal/routes/logout.ts +++ b/apps/server/src/routes/terminal/routes/logout.ts @@ -1,18 +1,36 @@ /** * POST /logout endpoint - Invalidate a session token + * + * Security: Only allows invalidating the token used for authentication. + * This ensures users can only log out their own sessions. */ import type { Request, Response } from 'express'; -import { deleteToken } from '../common.js'; +import { deleteToken, extractBearerToken, validateTerminalToken } from '../common.js'; export function createLogoutHandler() { return (req: Request, res: Response): void => { - const token = (req.headers['x-terminal-token'] as string) || req.body.token; + const token = extractBearerToken(req); - if (token) { - deleteToken(token); + if (!token) { + res.status(401).json({ + success: false, + error: 'Authorization header with Bearer token is required', + }); + return; } + if (!validateTerminalToken(token)) { + res.status(401).json({ + success: false, + error: 'Invalid or expired token', + }); + return; + } + + // Token is valid and belongs to the requester - safe to invalidate + deleteToken(token); + res.json({ success: true, }); diff --git a/apps/server/tests/unit/lib/auth.test.ts b/apps/server/tests/unit/lib/auth.test.ts index 91c1c461..3494d353 100644 --- a/apps/server/tests/unit/lib/auth.test.ts +++ b/apps/server/tests/unit/lib/auth.test.ts @@ -1,6 +1,16 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { createMockExpressContext } from '../../utils/mocks.js'; +/** + * Creates a mock Express context with socket properties for rate limiter support + */ +function createMockExpressContextWithSocket() { + const ctx = createMockExpressContext(); + ctx.req.socket = { remoteAddress: '127.0.0.1' } as any; + ctx.res.setHeader = vi.fn().mockReturnThis(); + return ctx; +} + /** * Note: auth.ts reads AUTOMAKER_API_KEY at module load time. * We need to reset modules and reimport for each test to get fresh state. @@ -29,7 +39,7 @@ describe('auth.ts', () => { process.env.AUTOMAKER_API_KEY = 'test-secret-key'; const { authMiddleware } = await import('@/lib/auth.js'); - const { req, res, next } = createMockExpressContext(); + const { req, res, next } = createMockExpressContextWithSocket(); authMiddleware(req, res, next); @@ -45,7 +55,7 @@ describe('auth.ts', () => { process.env.AUTOMAKER_API_KEY = 'test-secret-key'; const { authMiddleware } = await import('@/lib/auth.js'); - const { req, res, next } = createMockExpressContext(); + const { req, res, next } = createMockExpressContextWithSocket(); req.headers['x-api-key'] = 'wrong-key'; authMiddleware(req, res, next); @@ -62,7 +72,7 @@ describe('auth.ts', () => { process.env.AUTOMAKER_API_KEY = 'test-secret-key'; const { authMiddleware } = await import('@/lib/auth.js'); - const { req, res, next } = createMockExpressContext(); + const { req, res, next } = createMockExpressContextWithSocket(); req.headers['x-api-key'] = 'test-secret-key'; authMiddleware(req, res, next); @@ -113,4 +123,197 @@ describe('auth.ts', () => { }); }); }); + + describe('security - AUTOMAKER_API_KEY not set', () => { + it('should allow requests without any authentication when API key is not configured', async () => { + delete process.env.AUTOMAKER_API_KEY; + + const { authMiddleware } = await import('@/lib/auth.js'); + const { req, res, next } = createMockExpressContext(); + + authMiddleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + expect(res.json).not.toHaveBeenCalled(); + }); + + it('should allow requests even with invalid key header when API key is not configured', async () => { + delete process.env.AUTOMAKER_API_KEY; + + const { authMiddleware } = await import('@/lib/auth.js'); + const { req, res, next } = createMockExpressContext(); + req.headers['x-api-key'] = 'some-random-key'; + + authMiddleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + }); + + it('should report auth as disabled when no API key is configured', async () => { + delete process.env.AUTOMAKER_API_KEY; + + const { isAuthEnabled, getAuthStatus } = await import('@/lib/auth.js'); + + expect(isAuthEnabled()).toBe(false); + expect(getAuthStatus()).toEqual({ + enabled: false, + method: 'none', + }); + }); + }); + + describe('security - authentication correctness', () => { + it('should correctly authenticate with matching API key', async () => { + const testKey = 'correct-secret-key-12345'; + process.env.AUTOMAKER_API_KEY = testKey; + + const { authMiddleware } = await import('@/lib/auth.js'); + const { req, res, next } = createMockExpressContextWithSocket(); + req.headers['x-api-key'] = testKey; + + authMiddleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + }); + + it('should reject keys that differ by a single character', async () => { + process.env.AUTOMAKER_API_KEY = 'correct-secret-key'; + + const { authMiddleware } = await import('@/lib/auth.js'); + const { req, res, next } = createMockExpressContextWithSocket(); + req.headers['x-api-key'] = 'correct-secret-keY'; // Last char uppercase + + authMiddleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(403); + expect(next).not.toHaveBeenCalled(); + }); + + it('should reject keys with extra characters', async () => { + process.env.AUTOMAKER_API_KEY = 'secret-key'; + + const { authMiddleware } = await import('@/lib/auth.js'); + const { req, res, next } = createMockExpressContextWithSocket(); + req.headers['x-api-key'] = 'secret-key-extra'; + + authMiddleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(403); + expect(next).not.toHaveBeenCalled(); + }); + + it('should reject keys that are a prefix of the actual key', async () => { + process.env.AUTOMAKER_API_KEY = 'full-secret-key'; + + const { authMiddleware } = await import('@/lib/auth.js'); + const { req, res, next } = createMockExpressContextWithSocket(); + req.headers['x-api-key'] = 'full-secret'; + + authMiddleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(403); + expect(next).not.toHaveBeenCalled(); + }); + + it('should reject empty string API key header', async () => { + process.env.AUTOMAKER_API_KEY = 'secret-key'; + + const { authMiddleware } = await import('@/lib/auth.js'); + const { req, res, next } = createMockExpressContextWithSocket(); + req.headers['x-api-key'] = ''; + + authMiddleware(req, res, next); + + // Empty string is falsy, so should get 401 (no key provided) + expect(res.status).toHaveBeenCalledWith(401); + expect(next).not.toHaveBeenCalled(); + }); + + it('should handle keys with special characters correctly', async () => { + const specialKey = 'key-with-$pecial!@#chars_123'; + process.env.AUTOMAKER_API_KEY = specialKey; + + const { authMiddleware } = await import('@/lib/auth.js'); + const { req, res, next } = createMockExpressContextWithSocket(); + req.headers['x-api-key'] = specialKey; + + authMiddleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + }); + }); + + describe('security - rate limiting', () => { + it('should block requests after multiple failed attempts', async () => { + process.env.AUTOMAKER_API_KEY = 'correct-key'; + + const { authMiddleware } = await import('@/lib/auth.js'); + const { apiKeyRateLimiter } = await import('@/lib/rate-limiter.js'); + + // Reset the rate limiter for this test + apiKeyRateLimiter.reset('192.168.1.100'); + + // Simulate multiple failed attempts + for (let i = 0; i < 5; i++) { + const { req, res, next } = createMockExpressContextWithSocket(); + req.socket.remoteAddress = '192.168.1.100'; + req.headers['x-api-key'] = 'wrong-key'; + authMiddleware(req, res, next); + } + + // Next request should be rate limited + const { req, res, next } = createMockExpressContextWithSocket(); + req.socket.remoteAddress = '192.168.1.100'; + req.headers['x-api-key'] = 'correct-key'; // Even with correct key + + authMiddleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(429); + expect(next).not.toHaveBeenCalled(); + + // Cleanup + apiKeyRateLimiter.reset('192.168.1.100'); + }); + + it('should reset rate limit on successful authentication', async () => { + process.env.AUTOMAKER_API_KEY = 'correct-key'; + + const { authMiddleware } = await import('@/lib/auth.js'); + const { apiKeyRateLimiter } = await import('@/lib/rate-limiter.js'); + + // Reset the rate limiter for this test + apiKeyRateLimiter.reset('192.168.1.101'); + + // Simulate a few failed attempts (not enough to trigger block) + for (let i = 0; i < 3; i++) { + const { req, res, next } = createMockExpressContextWithSocket(); + req.socket.remoteAddress = '192.168.1.101'; + req.headers['x-api-key'] = 'wrong-key'; + authMiddleware(req, res, next); + } + + // Successful authentication should reset the counter + const { + req: successReq, + res: successRes, + next: successNext, + } = createMockExpressContextWithSocket(); + successReq.socket.remoteAddress = '192.168.1.101'; + successReq.headers['x-api-key'] = 'correct-key'; + + authMiddleware(successReq, successRes, successNext); + + expect(successNext).toHaveBeenCalled(); + + // After reset, we should have full attempts available again + expect(apiKeyRateLimiter.getAttemptsRemaining('192.168.1.101')).toBe(5); + + // Cleanup + apiKeyRateLimiter.reset('192.168.1.101'); + }); + }); }); diff --git a/apps/server/tests/unit/lib/rate-limiter.test.ts b/apps/server/tests/unit/lib/rate-limiter.test.ts new file mode 100644 index 00000000..ffc97c01 --- /dev/null +++ b/apps/server/tests/unit/lib/rate-limiter.test.ts @@ -0,0 +1,249 @@ +import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; +import { RateLimiter } from '../../../src/lib/rate-limiter.js'; +import type { Request } from 'express'; + +describe('RateLimiter', () => { + let rateLimiter: RateLimiter; + + beforeEach(() => { + rateLimiter = new RateLimiter({ + maxAttempts: 3, + windowMs: 60000, // 1 minute + blockDurationMs: 60000, // 1 minute + }); + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + describe('getClientIp', () => { + it('should extract IP from x-forwarded-for header', () => { + const req = { + headers: { 'x-forwarded-for': '192.168.1.100' }, + socket: { remoteAddress: '127.0.0.1' }, + } as unknown as Request; + + expect(rateLimiter.getClientIp(req)).toBe('192.168.1.100'); + }); + + it('should use first IP from x-forwarded-for with multiple IPs', () => { + const req = { + headers: { 'x-forwarded-for': '192.168.1.100, 10.0.0.1, 172.16.0.1' }, + socket: { remoteAddress: '127.0.0.1' }, + } as unknown as Request; + + expect(rateLimiter.getClientIp(req)).toBe('192.168.1.100'); + }); + + it('should fall back to socket remoteAddress when no x-forwarded-for', () => { + const req = { + headers: {}, + socket: { remoteAddress: '127.0.0.1' }, + } as unknown as Request; + + expect(rateLimiter.getClientIp(req)).toBe('127.0.0.1'); + }); + + it('should return "unknown" when no IP can be determined', () => { + const req = { + headers: {}, + socket: { remoteAddress: undefined }, + } as unknown as Request; + + expect(rateLimiter.getClientIp(req)).toBe('unknown'); + }); + }); + + describe('isBlocked', () => { + it('should return false for unknown keys', () => { + expect(rateLimiter.isBlocked('192.168.1.1')).toBe(false); + }); + + it('should return false after recording fewer failures than max', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + expect(rateLimiter.isBlocked('192.168.1.1')).toBe(false); + }); + + it('should return true after reaching max failures', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + expect(rateLimiter.isBlocked('192.168.1.1')).toBe(true); + }); + + it('should return false after block expires', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + expect(rateLimiter.isBlocked('192.168.1.1')).toBe(true); + + // Advance time past block duration + vi.advanceTimersByTime(60001); + + expect(rateLimiter.isBlocked('192.168.1.1')).toBe(false); + }); + }); + + describe('recordFailure', () => { + it('should return false when not yet blocked', () => { + expect(rateLimiter.recordFailure('192.168.1.1')).toBe(false); + expect(rateLimiter.recordFailure('192.168.1.1')).toBe(false); + }); + + it('should return true when threshold is reached', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + expect(rateLimiter.recordFailure('192.168.1.1')).toBe(true); + }); + + it('should reset counter after window expires', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + // Advance time past window + vi.advanceTimersByTime(60001); + + // Should start fresh + expect(rateLimiter.recordFailure('192.168.1.1')).toBe(false); + expect(rateLimiter.getAttemptsRemaining('192.168.1.1')).toBe(2); + }); + + it('should track different IPs independently', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + rateLimiter.recordFailure('192.168.1.2'); + + expect(rateLimiter.isBlocked('192.168.1.1')).toBe(true); + expect(rateLimiter.isBlocked('192.168.1.2')).toBe(false); + }); + }); + + describe('reset', () => { + it('should clear record for a key', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + rateLimiter.reset('192.168.1.1'); + + expect(rateLimiter.getAttemptsRemaining('192.168.1.1')).toBe(3); + }); + + it('should clear blocked status', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + expect(rateLimiter.isBlocked('192.168.1.1')).toBe(true); + + rateLimiter.reset('192.168.1.1'); + + expect(rateLimiter.isBlocked('192.168.1.1')).toBe(false); + }); + }); + + describe('getAttemptsRemaining', () => { + it('should return max attempts for unknown key', () => { + expect(rateLimiter.getAttemptsRemaining('192.168.1.1')).toBe(3); + }); + + it('should decrease as failures are recorded', () => { + rateLimiter.recordFailure('192.168.1.1'); + expect(rateLimiter.getAttemptsRemaining('192.168.1.1')).toBe(2); + + rateLimiter.recordFailure('192.168.1.1'); + expect(rateLimiter.getAttemptsRemaining('192.168.1.1')).toBe(1); + + rateLimiter.recordFailure('192.168.1.1'); + expect(rateLimiter.getAttemptsRemaining('192.168.1.1')).toBe(0); + }); + + it('should return max attempts after window expires', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + vi.advanceTimersByTime(60001); + + expect(rateLimiter.getAttemptsRemaining('192.168.1.1')).toBe(3); + }); + }); + + describe('getBlockTimeRemaining', () => { + it('should return 0 for non-blocked key', () => { + expect(rateLimiter.getBlockTimeRemaining('192.168.1.1')).toBe(0); + }); + + it('should return remaining block time for blocked key', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + vi.advanceTimersByTime(30000); // Advance 30 seconds + + const remaining = rateLimiter.getBlockTimeRemaining('192.168.1.1'); + expect(remaining).toBeGreaterThan(29000); + expect(remaining).toBeLessThanOrEqual(30000); + }); + + it('should return 0 after block expires', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + vi.advanceTimersByTime(60001); + + expect(rateLimiter.getBlockTimeRemaining('192.168.1.1')).toBe(0); + }); + }); + + describe('cleanup', () => { + it('should remove expired blocks', () => { + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + rateLimiter.recordFailure('192.168.1.1'); + + vi.advanceTimersByTime(60001); + + rateLimiter.cleanup(); + + // After cleanup, the record should be gone + expect(rateLimiter.getAttemptsRemaining('192.168.1.1')).toBe(3); + }); + + it('should remove expired windows', () => { + rateLimiter.recordFailure('192.168.1.1'); + + vi.advanceTimersByTime(60001); + + rateLimiter.cleanup(); + + expect(rateLimiter.getAttemptsRemaining('192.168.1.1')).toBe(3); + }); + + it('should preserve active records', () => { + rateLimiter.recordFailure('192.168.1.1'); + + vi.advanceTimersByTime(30000); // Half the window + + rateLimiter.cleanup(); + + expect(rateLimiter.getAttemptsRemaining('192.168.1.1')).toBe(2); + }); + }); + + describe('default configuration', () => { + it('should use sensible defaults', () => { + const defaultLimiter = new RateLimiter(); + + // Should have 5 max attempts by default + expect(defaultLimiter.getAttemptsRemaining('test')).toBe(5); + }); + }); +}); diff --git a/apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts b/apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts index cd0a4c9c..5ce82f67 100644 --- a/apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts +++ b/apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.ts @@ -59,8 +59,9 @@ export function useApiKeyManagement() { hasGoogleKey: status.hasGoogleKey, }); } - } catch (error) { - console.error('Failed to check API key status:', error); + } catch { + // Silently handle API key status check failures to avoid exposing + // sensitive error details in the console } } }; @@ -98,26 +99,29 @@ export function useApiKeyManagement() { }; // Test Google/Gemini connection - // TODO: Add backend endpoint for Gemini API key verification + // NOTE: Full API key validation requires a backend call to verify the key + // against Google's API. The current client-side validation only checks + // basic format requirements and cannot confirm the key is actually valid. const handleTestGeminiConnection = async () => { setTestingGeminiConnection(true); setGeminiTestResult(null); - // Basic validation - check key format + // Basic client-side format validation only + // This does NOT verify the key is valid with Google's API if (!googleKey || googleKey.trim().length < 10) { setGeminiTestResult({ success: false, - message: 'Please enter a valid API key.', + message: 'Please enter an API key with at least 10 characters.', }); setTestingGeminiConnection(false); return; } - // For now, just validate the key format (starts with expected prefix) - // Full verification requires a backend endpoint + // Client-side validation cannot confirm key validity. + // The key will be verified when first used with the Gemini API. setGeminiTestResult({ success: true, - message: 'API key saved. Connection test not yet available.', + message: 'API key format accepted. Key will be validated on first use with Gemini API.', }); setTestingGeminiConnection(false); }; diff --git a/apps/ui/src/components/views/terminal-view.tsx b/apps/ui/src/components/views/terminal-view.tsx index db79ce0f..13b2eea2 100644 --- a/apps/ui/src/components/views/terminal-view.tsx +++ b/apps/ui/src/components/views/terminal-view.tsx @@ -296,7 +296,7 @@ export function TerminalView() { const headers: Record = {}; if (terminalState.authToken) { - headers['X-Terminal-Token'] = terminalState.authToken; + headers['Authorization'] = `Bearer ${terminalState.authToken}`; } console.log(`[Terminal] Killing ${sessionIds.length} sessions on server`); @@ -459,7 +459,7 @@ export function TerminalView() { try { const headers: Record = {}; if (terminalState.authToken) { - headers['X-Terminal-Token'] = terminalState.authToken; + headers['Authorization'] = `Bearer ${terminalState.authToken}`; } const response = await fetch(`${serverUrl}/api/terminal/settings`, { headers }); const data = await response.json(); @@ -488,7 +488,7 @@ export function TerminalView() { 'Content-Type': 'application/json', }; if (terminalState.authToken) { - headers['X-Terminal-Token'] = terminalState.authToken; + headers['Authorization'] = `Bearer ${terminalState.authToken}`; } // Try to use the bulk delete endpoint if available, otherwise delete individually @@ -501,7 +501,7 @@ export function TerminalView() { const xhr = new XMLHttpRequest(); xhr.open('DELETE', url, false); // synchronous if (terminalState.authToken) { - xhr.setRequestHeader('X-Terminal-Token', terminalState.authToken); + xhr.setRequestHeader('Authorization', `Bearer ${terminalState.authToken}`); } xhr.send(); } catch { @@ -595,7 +595,7 @@ export function TerminalView() { // Get fresh auth token from store const authToken = useAppStore.getState().terminalState.authToken; if (authToken) { - headers['X-Terminal-Token'] = authToken; + headers['Authorization'] = `Bearer ${authToken}`; } // Helper to check if a session still exists on server @@ -833,7 +833,7 @@ export function TerminalView() { 'Content-Type': 'application/json', }; if (terminalState.authToken) { - headers['X-Terminal-Token'] = terminalState.authToken; + headers['Authorization'] = `Bearer ${terminalState.authToken}`; } const response = await fetch(`${serverUrl}/api/terminal/sessions`, { @@ -892,7 +892,7 @@ export function TerminalView() { 'Content-Type': 'application/json', }; if (terminalState.authToken) { - headers['X-Terminal-Token'] = terminalState.authToken; + headers['Authorization'] = `Bearer ${terminalState.authToken}`; } const response = await fetch(`${serverUrl}/api/terminal/sessions`, { @@ -952,7 +952,7 @@ export function TerminalView() { try { const headers: Record = {}; if (terminalState.authToken) { - headers['X-Terminal-Token'] = terminalState.authToken; + headers['Authorization'] = `Bearer ${terminalState.authToken}`; } const response = await fetch(`${serverUrl}/api/terminal/sessions/${sessionId}`, { @@ -998,7 +998,7 @@ export function TerminalView() { // Kill all sessions on the server const headers: Record = {}; if (terminalState.authToken) { - headers['X-Terminal-Token'] = terminalState.authToken; + headers['Authorization'] = `Bearer ${terminalState.authToken}`; } await Promise.all( diff --git a/apps/ui/src/components/views/terminal-view/terminal-panel.tsx b/apps/ui/src/components/views/terminal-view/terminal-panel.tsx index 56266db6..db2b33e3 100644 --- a/apps/ui/src/components/views/terminal-view/terminal-panel.tsx +++ b/apps/ui/src/components/views/terminal-view/terminal-panel.tsx @@ -940,7 +940,12 @@ export function TerminalPanel({ if (!terminal) return; const connect = () => { - // Build WebSocket URL with token + // Build WebSocket URL with token in query string + // Note: WebSocket API in browsers does not support custom headers during the upgrade handshake, + // so we must pass the token via query string. This is acceptable because: + // 1. WebSocket URLs are not exposed in HTTP Referer headers + // 2. The connection is upgraded to a secure WebSocket protocol immediately + // 3. Server-side logging should not log query parameters containing tokens let url = `${wsUrl}/api/terminal/ws?sessionId=${sessionId}`; if (authToken) { url += `&token=${encodeURIComponent(authToken)}`; diff --git a/apps/ui/src/store/app-store.ts b/apps/ui/src/store/app-store.ts index 978d67cc..6a846341 100644 --- a/apps/ui/src/store/app-store.ts +++ b/apps/ui/src/store/app-store.ts @@ -2676,7 +2676,10 @@ export const useAppStore = create()( kanbanCardDetailLevel: state.kanbanCardDetailLevel, boardViewMode: state.boardViewMode, // Settings - apiKeys: state.apiKeys, + // NOTE: apiKeys are intentionally NOT persisted to localStorage for security. + // API keys are stored server-side only via the storeApiKey API to prevent + // exposure through XSS attacks. The apiKeys state is populated on app load + // from the secure server-side storage. maxConcurrency: state.maxConcurrency, // Note: autoModeByProject is intentionally NOT persisted // Auto-mode should always default to OFF on app refresh diff --git a/libs/platform/src/secure-fs.ts b/libs/platform/src/secure-fs.ts index bf6099dd..263f09aa 100644 --- a/libs/platform/src/secure-fs.ts +++ b/libs/platform/src/secure-fs.ts @@ -4,29 +4,46 @@ * All file I/O operations must go through this adapter to enforce * ALLOWED_ROOT_DIRECTORY restrictions at the actual access point, * not just at the API layer. This provides defense-in-depth security. + * + * Security features: + * - Path validation: All paths are validated against allowed directories + * - Symlink protection: Operations on existing files resolve symlinks before validation + * to prevent directory escape attacks via symbolic links + * + * TOCTOU (Time-of-check to time-of-use) note: + * There is an inherent race condition between path validation and the actual file + * operation. To mitigate this, we use the validated realpath (symlinks resolved) + * for the actual operation wherever possible. However, this cannot fully prevent + * race conditions in a multi-process environment. For maximum security in + * high-risk scenarios, consider using file descriptor-based operations or + * additional locking mechanisms. */ import fs from 'fs/promises'; import type { Dirent } from 'fs'; import path from 'path'; -import { validatePath } from './security.js'; +import { validatePath, validatePathWithSymlinkCheck } from './security.js'; /** * Wrapper around fs.access that validates path first + * Uses symlink-aware validation to prevent directory escape attacks */ export async function access(filePath: string, mode?: number): Promise { - const validatedPath = validatePath(filePath); + // Use symlink check since we're checking an existing path + const validatedPath = validatePathWithSymlinkCheck(filePath); return fs.access(validatedPath, mode); } /** * Wrapper around fs.readFile that validates path first + * Uses symlink-aware validation to prevent reading files outside allowed directories */ export async function readFile( filePath: string, encoding?: BufferEncoding ): Promise { - const validatedPath = validatePath(filePath); + // Use symlink check since we're reading an existing file + const validatedPath = validatePathWithSymlinkCheck(filePath); if (encoding) { return fs.readFile(validatedPath, encoding); } @@ -35,29 +52,34 @@ export async function readFile( /** * Wrapper around fs.writeFile that validates path first + * Uses symlink-aware validation for existing files, or validates parent for new files */ export async function writeFile( filePath: string, data: string | Buffer, encoding?: BufferEncoding ): Promise { - const validatedPath = validatePath(filePath); + // Use symlink check with requireExists=false to handle both new and existing files + const validatedPath = validatePathWithSymlinkCheck(filePath, { requireExists: false }); return fs.writeFile(validatedPath, data, encoding); } /** * Wrapper around fs.mkdir that validates path first + * Uses symlink-aware validation for parent directory to prevent creating dirs via symlink escape */ export async function mkdir( dirPath: string, options?: { recursive?: boolean; mode?: number } ): Promise { - const validatedPath = validatePath(dirPath); + // Use symlink check with requireExists=false since directory may not exist yet + const validatedPath = validatePathWithSymlinkCheck(dirPath, { requireExists: false }); return fs.mkdir(validatedPath, options); } /** * Wrapper around fs.readdir that validates path first + * Uses symlink-aware validation to prevent listing directories outside allowed paths */ export async function readdir( dirPath: string, @@ -71,7 +93,8 @@ export async function readdir( dirPath: string, options?: { withFileTypes?: boolean; encoding?: BufferEncoding } ): Promise { - const validatedPath = validatePath(dirPath); + // Use symlink check since we're reading an existing directory + const validatedPath = validatePathWithSymlinkCheck(dirPath); if (options?.withFileTypes === true) { return fs.readdir(validatedPath, { withFileTypes: true }); } @@ -80,66 +103,85 @@ export async function readdir( /** * Wrapper around fs.stat that validates path first + * Uses symlink-aware validation to prevent stat on files outside allowed paths */ export async function stat(filePath: string): Promise { - const validatedPath = validatePath(filePath); + // Use symlink check since we're getting info about an existing file + const validatedPath = validatePathWithSymlinkCheck(filePath); return fs.stat(validatedPath); } /** * Wrapper around fs.rm that validates path first + * Uses symlink-aware validation to prevent deleting files/directories outside allowed paths */ export async function rm( filePath: string, options?: { recursive?: boolean; force?: boolean } ): Promise { - const validatedPath = validatePath(filePath); + // Use symlink check since we're removing an existing file/directory + const validatedPath = validatePathWithSymlinkCheck(filePath); return fs.rm(validatedPath, options); } /** * Wrapper around fs.unlink that validates path first + * Uses symlink-aware validation to prevent unlinking files outside allowed paths */ export async function unlink(filePath: string): Promise { - const validatedPath = validatePath(filePath); + // Use symlink check since we're unlinking an existing file + const validatedPath = validatePathWithSymlinkCheck(filePath); return fs.unlink(validatedPath); } /** * Wrapper around fs.copyFile that validates both paths first + * Uses symlink-aware validation for source, and parent validation for destination */ export async function copyFile(src: string, dest: string, mode?: number): Promise { - const validatedSrc = validatePath(src); - const validatedDest = validatePath(dest); + // Source must exist, use symlink check + const validatedSrc = validatePathWithSymlinkCheck(src); + // Destination may not exist, validate with parent fallback + const validatedDest = validatePathWithSymlinkCheck(dest, { requireExists: false }); return fs.copyFile(validatedSrc, validatedDest, mode); } /** * Wrapper around fs.appendFile that validates path first + * Uses symlink-aware validation for existing files, or validates parent for new files */ export async function appendFile( filePath: string, data: string | Buffer, encoding?: BufferEncoding ): Promise { - const validatedPath = validatePath(filePath); + // File may or may not exist, use symlink check with parent fallback + const validatedPath = validatePathWithSymlinkCheck(filePath, { requireExists: false }); return fs.appendFile(validatedPath, data, encoding); } /** * Wrapper around fs.rename that validates both paths first + * Uses symlink-aware validation for source, and parent validation for destination */ export async function rename(oldPath: string, newPath: string): Promise { - const validatedOldPath = validatePath(oldPath); - const validatedNewPath = validatePath(newPath); + // Source must exist, use symlink check + const validatedOldPath = validatePathWithSymlinkCheck(oldPath); + // Destination may not exist, validate with parent fallback + const validatedNewPath = validatePathWithSymlinkCheck(newPath, { requireExists: false }); return fs.rename(validatedOldPath, validatedNewPath); } /** * Wrapper around fs.lstat that validates path first * Returns file stats without following symbolic links + * + * Note: This intentionally uses validatePath (not validatePathWithSymlinkCheck) + * because lstat is used to inspect symlinks themselves. Using realpathSync + * would defeat the purpose of lstat. */ export async function lstat(filePath: string): Promise { + // Use basic validation since lstat is for inspecting symlinks const validatedPath = validatePath(filePath); return fs.lstat(validatedPath); } diff --git a/libs/platform/src/security.ts b/libs/platform/src/security.ts index ebb877de..749d0a16 100644 --- a/libs/platform/src/security.ts +++ b/libs/platform/src/security.ts @@ -1,9 +1,22 @@ /** * Security utilities for path validation * Enforces ALLOWED_ROOT_DIRECTORY constraint with appData exception + * + * Security considerations: + * - Symlink resolution: validatePathWithSymlinkCheck() resolves symlinks to prevent + * escaping the allowed directory via symbolic links + * - TOCTOU: There is an inherent race condition between path validation and file + * operation. Callers should use the resolved realpath for operations when possible. */ import path from 'path'; +import fs from 'fs'; + +/** + * Security mode: 'strict' fails closed when ALLOWED_ROOT_DIRECTORY is not set, + * 'permissive' allows all paths (legacy behavior, not recommended for production) + */ +let securityMode: 'strict' | 'permissive' = 'strict'; /** * Error thrown when a path is not allowed by security policy @@ -25,22 +38,43 @@ let dataDirectory: string | null = null; * Initialize security settings from environment variables * - ALLOWED_ROOT_DIRECTORY: main security boundary * - DATA_DIR: appData exception, always allowed + * - SECURITY_MODE: 'strict' (default, fail-closed) or 'permissive' (legacy, fail-open) */ export function initAllowedPaths(): void { + // Load security mode + const mode = process.env.SECURITY_MODE?.toLowerCase(); + if (mode === 'permissive') { + securityMode = 'permissive'; + console.warn( + '[Security] WARNING: Running in PERMISSIVE mode - all paths allowed when ALLOWED_ROOT_DIRECTORY is not set. ' + + 'This is not recommended for production environments.' + ); + } else { + securityMode = 'strict'; + } + // Load ALLOWED_ROOT_DIRECTORY const rootDir = process.env.ALLOWED_ROOT_DIRECTORY; if (rootDir) { allowedRootDirectory = path.resolve(rootDir); - console.log(`[Security] ✓ ALLOWED_ROOT_DIRECTORY configured: ${allowedRootDirectory}`); + console.log(`[Security] ALLOWED_ROOT_DIRECTORY configured: ${allowedRootDirectory}`); + } else if (securityMode === 'strict') { + console.error( + '[Security] CRITICAL: ALLOWED_ROOT_DIRECTORY not set in strict mode. ' + + 'All file operations outside DATA_DIR will be denied. ' + + 'Set ALLOWED_ROOT_DIRECTORY or use SECURITY_MODE=permissive to allow all paths.' + ); } else { - console.log('[Security] ⚠️ ALLOWED_ROOT_DIRECTORY not set - allowing access to all paths'); + console.warn( + '[Security] WARNING: ALLOWED_ROOT_DIRECTORY not set - allowing access to all paths' + ); } // Load DATA_DIR (appData exception - always allowed) const dataDir = process.env.DATA_DIR; if (dataDir) { dataDirectory = path.resolve(dataDir); - console.log(`[Security] ✓ DATA_DIR configured: ${dataDirectory}`); + console.log(`[Security] DATA_DIR configured: ${dataDirectory}`); } } @@ -49,7 +83,10 @@ export function initAllowedPaths(): void { * Returns true if: * - Path is within ALLOWED_ROOT_DIRECTORY, OR * - Path is within DATA_DIR (appData exception), OR - * - No restrictions are configured (backward compatibility) + * - No restrictions are configured AND security mode is 'permissive' + * + * In strict mode (default), paths are denied if ALLOWED_ROOT_DIRECTORY is not set, + * unless they are within DATA_DIR. */ export function isPathAllowed(filePath: string): boolean { const resolvedPath = path.resolve(filePath); @@ -59,24 +96,29 @@ export function isPathAllowed(filePath: string): boolean { return true; } - // If no ALLOWED_ROOT_DIRECTORY restriction is configured, allow all paths - // Note: DATA_DIR is checked above as an exception, but doesn't restrict other paths + // If no ALLOWED_ROOT_DIRECTORY restriction is configured: + // - In strict mode: deny (fail-closed) + // - In permissive mode: allow all paths (legacy behavior) if (!allowedRootDirectory) { - return true; + return securityMode === 'permissive'; } // Allow if within ALLOWED_ROOT_DIRECTORY - if (allowedRootDirectory && isPathWithinDirectory(resolvedPath, allowedRootDirectory)) { + if (isPathWithinDirectory(resolvedPath, allowedRootDirectory)) { return true; } - // If restrictions are configured but path doesn't match, deny + // Path doesn't match any allowed directory, deny return false; } /** * Validate a path - resolves it and checks permissions * Throws PathNotAllowedError if path is not allowed + * + * NOTE: This function uses path.resolve() which does NOT resolve symbolic links. + * For operations on existing files where symlink attacks are a concern, use + * validatePathWithSymlinkCheck() instead. */ export function validatePath(filePath: string): string { const resolvedPath = path.resolve(filePath); @@ -88,6 +130,74 @@ export function validatePath(filePath: string): string { return resolvedPath; } +/** + * Validate a path with symlink resolution for existing files + * This prevents symlink-based directory escape attacks by resolving the + * actual filesystem path before validation. + * + * @param filePath - The path to validate + * @param options.requireExists - If true (default), throws if path doesn't exist. + * If false, falls back to validatePath for non-existent paths. + * @returns The real path (symlinks resolved) if file exists, or resolved path if not + * @throws PathNotAllowedError if the real path escapes allowed directories + * + * Security note: There is still a TOCTOU race between this check and the actual + * file operation. For maximum security, callers should use the returned realpath + * for the subsequent operation, not the original path. + */ +export function validatePathWithSymlinkCheck( + filePath: string, + options: { requireExists?: boolean } = {} +): string { + const { requireExists = true } = options; + const resolvedPath = path.resolve(filePath); + + try { + // Check if path exists and get info without following symlinks + const lstats = fs.lstatSync(resolvedPath); + + // Get the real path (resolves all symlinks) + const realPath = fs.realpathSync(resolvedPath); + + // Validate the real path, not the symlink path + if (!isPathAllowed(realPath)) { + throw new PathNotAllowedError(`${filePath} (resolves to ${realPath} via symlink)`); + } + + // If it's a symlink, log for security auditing + if (lstats.isSymbolicLink()) { + console.log(`[Security] Symlink detected: ${resolvedPath} -> ${realPath}`); + } + + return realPath; + } catch (error) { + // Handle file not found + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + if (requireExists) { + throw error; + } + // For new files, validate the parent directory with symlink check if it exists + const parentDir = path.dirname(resolvedPath); + try { + const realParentPath = fs.realpathSync(parentDir); + if (!isPathAllowed(realParentPath)) { + throw new PathNotAllowedError(`${filePath} (parent resolves to ${realParentPath})`); + } + // Return the path within the real parent + return path.join(realParentPath, path.basename(resolvedPath)); + } catch (parentError) { + // Parent doesn't exist either, fall back to basic validation + if ((parentError as NodeJS.ErrnoException).code === 'ENOENT') { + return validatePath(filePath); + } + throw parentError; + } + } + // Re-throw PathNotAllowedError and other errors + throw error; + } +} + /** * Check if a path is within a directory, with protection against path traversal * Returns true only if resolvedPath is within directoryPath diff --git a/libs/platform/tests/security.test.ts b/libs/platform/tests/security.test.ts index dd2224c5..13613591 100644 --- a/libs/platform/tests/security.test.ts +++ b/libs/platform/tests/security.test.ts @@ -95,27 +95,57 @@ describe('security.ts', () => { expect(isPathAllowed('/app/data/credentials.json')).toBe(true); }); - it('should allow all paths when no restrictions configured', async () => { + it('should deny all paths in strict mode when no restrictions configured', async () => { delete process.env.ALLOWED_ROOT_DIRECTORY; delete process.env.DATA_DIR; + delete process.env.SECURITY_MODE; // Default to strict const { initAllowedPaths, isPathAllowed } = await import('../src/security'); initAllowedPaths(); + // In strict mode, paths are denied when no ALLOWED_ROOT_DIRECTORY is set + expect(isPathAllowed('/any/path')).toBe(false); + expect(isPathAllowed('/etc/passwd')).toBe(false); + }); + + it('should allow all paths in permissive mode when no restrictions configured', async () => { + delete process.env.ALLOWED_ROOT_DIRECTORY; + delete process.env.DATA_DIR; + process.env.SECURITY_MODE = 'permissive'; + + const { initAllowedPaths, isPathAllowed } = await import('../src/security'); + initAllowedPaths(); + + // In permissive mode, all paths are allowed when no restrictions configured expect(isPathAllowed('/any/path')).toBe(true); expect(isPathAllowed('/etc/passwd')).toBe(true); }); - it('should allow all paths when only DATA_DIR is configured', async () => { + it('should deny non-DATA_DIR paths in strict mode when only DATA_DIR is configured', async () => { delete process.env.ALLOWED_ROOT_DIRECTORY; process.env.DATA_DIR = '/data'; + delete process.env.SECURITY_MODE; // Default to strict const { initAllowedPaths, isPathAllowed } = await import('../src/security'); initAllowedPaths(); // DATA_DIR should be allowed expect(isPathAllowed('/data/file.txt')).toBe(true); - // And all other paths should be allowed since no ALLOWED_ROOT_DIRECTORY restriction + // Other paths should be denied in strict mode + expect(isPathAllowed('/any/path')).toBe(false); + }); + + it('should allow all paths in permissive mode when only DATA_DIR is configured', async () => { + delete process.env.ALLOWED_ROOT_DIRECTORY; + process.env.DATA_DIR = '/data'; + process.env.SECURITY_MODE = 'permissive'; + + const { initAllowedPaths, isPathAllowed } = await import('../src/security'); + initAllowedPaths(); + + // DATA_DIR should be allowed + expect(isPathAllowed('/data/file.txt')).toBe(true); + // Other paths should also be allowed in permissive mode expect(isPathAllowed('/any/path')).toBe(true); }); }); @@ -155,13 +185,28 @@ describe('security.ts', () => { expect(result).toBe(path.resolve(cwd, './file.txt')); }); - it('should not throw when no restrictions configured', async () => { + it('should throw in strict mode when no restrictions configured', async () => { delete process.env.ALLOWED_ROOT_DIRECTORY; delete process.env.DATA_DIR; + delete process.env.SECURITY_MODE; // Default to strict + + const { initAllowedPaths, validatePath, PathNotAllowedError } = + await import('../src/security'); + initAllowedPaths(); + + // In strict mode, paths are denied when no ALLOWED_ROOT_DIRECTORY is set + expect(() => validatePath('/any/path')).toThrow(PathNotAllowedError); + }); + + it('should not throw in permissive mode when no restrictions configured', async () => { + delete process.env.ALLOWED_ROOT_DIRECTORY; + delete process.env.DATA_DIR; + process.env.SECURITY_MODE = 'permissive'; const { initAllowedPaths, validatePath } = await import('../src/security'); initAllowedPaths(); + // In permissive mode, all paths are allowed when no restrictions configured expect(() => validatePath('/any/path')).not.toThrow(); }); }); @@ -212,6 +257,110 @@ describe('security.ts', () => { }); }); + describe('path traversal attack prevention', () => { + it('should block basic path traversal with ../', async () => { + process.env.ALLOWED_ROOT_DIRECTORY = '/allowed'; + delete process.env.DATA_DIR; + + const { initAllowedPaths, isPathAllowed } = await import('../src/security'); + initAllowedPaths(); + + expect(isPathAllowed('/allowed/../etc/passwd')).toBe(false); + expect(isPathAllowed('/allowed/subdir/../../etc/passwd')).toBe(false); + }); + + it('should block path traversal with multiple ../ sequences', async () => { + process.env.ALLOWED_ROOT_DIRECTORY = '/allowed/deep/nested'; + delete process.env.DATA_DIR; + + const { initAllowedPaths, isPathAllowed } = await import('../src/security'); + initAllowedPaths(); + + expect(isPathAllowed('/allowed/deep/nested/../../../etc/passwd')).toBe(false); + expect(isPathAllowed('/allowed/deep/nested/../../../../root')).toBe(false); + }); + + it('should block standalone .. in path components', async () => { + process.env.ALLOWED_ROOT_DIRECTORY = '/allowed'; + delete process.env.DATA_DIR; + + const { initAllowedPaths, isPathAllowed } = await import('../src/security'); + initAllowedPaths(); + + expect(isPathAllowed('/allowed/foo/..bar')).toBe(true); // This is a valid filename, not traversal + expect(isPathAllowed('/allowed/foo/../bar')).toBe(true); // Resolves within allowed + expect(isPathAllowed('/allowed/../notallowed')).toBe(false); + }); + + it('should handle edge case of path ending with /..', async () => { + process.env.ALLOWED_ROOT_DIRECTORY = '/allowed/subdir'; + delete process.env.DATA_DIR; + + const { initAllowedPaths, isPathAllowed } = await import('../src/security'); + initAllowedPaths(); + + expect(isPathAllowed('/allowed/subdir/..')).toBe(false); + expect(isPathAllowed('/allowed/subdir/../..')).toBe(false); + }); + + it('should properly resolve and block complex traversal attempts', async () => { + process.env.ALLOWED_ROOT_DIRECTORY = '/home/user/projects'; + delete process.env.DATA_DIR; + + const { initAllowedPaths, isPathAllowed } = await import('../src/security'); + initAllowedPaths(); + + // Attempt to escape via complex path + expect(isPathAllowed('/home/user/projects/app/../../../etc/shadow')).toBe(false); + + // Valid path that uses .. but stays within allowed + expect(isPathAllowed('/home/user/projects/app/../lib/file.ts')).toBe(true); + }); + + it('should validate path throws on traversal attacks', async () => { + process.env.ALLOWED_ROOT_DIRECTORY = '/allowed'; + delete process.env.DATA_DIR; + + const { initAllowedPaths, validatePath, PathNotAllowedError } = + await import('../src/security'); + initAllowedPaths(); + + expect(() => validatePath('/allowed/../etc/passwd')).toThrow(PathNotAllowedError); + expect(() => validatePath('/allowed/../../root/.ssh/id_rsa')).toThrow(PathNotAllowedError); + }); + + it('should handle paths with mixed separators (cross-platform)', async () => { + process.env.ALLOWED_ROOT_DIRECTORY = '/allowed'; + delete process.env.DATA_DIR; + + const { initAllowedPaths, isPathAllowed } = await import('../src/security'); + initAllowedPaths(); + + // Node's path.resolve handles these correctly on each platform + const maliciousPath = path.resolve('/allowed', '..', 'etc', 'passwd'); + expect(isPathAllowed(maliciousPath)).toBe(false); + }); + + it('should correctly identify paths at the boundary', async () => { + process.env.ALLOWED_ROOT_DIRECTORY = '/allowed'; + delete process.env.DATA_DIR; + + const { initAllowedPaths, isPathAllowed } = await import('../src/security'); + initAllowedPaths(); + + // The allowed directory itself should be allowed + expect(isPathAllowed('/allowed')).toBe(true); + expect(isPathAllowed('/allowed/')).toBe(true); + + // Parent of allowed should not be allowed + expect(isPathAllowed('/')).toBe(false); + + // Sibling directories should not be allowed + expect(isPathAllowed('/allowed2')).toBe(false); + expect(isPathAllowed('/allowedextra')).toBe(false); + }); + }); + describe('getDataDirectory', () => { it('should return the configured data directory', async () => { process.env.DATA_DIR = '/data';