feat: Implement API key authentication with rate limiting and secure comparison

- Added rate limiting to the authentication middleware to prevent brute-force attacks. - Introduced a secure comparison function to mitigate timing attacks during API key validation. - Created a new rate limiter class to track failed authentication attempts and block requests after exceeding the maximum allowed failures. - Updated the authentication middleware to handle rate limiting and secure key comparison. - Enhanced error handling for rate-limited requests, providing appropriate responses to clients.
2026-02-01 20:23:36 +00:00 · 2025-12-24 14:49:47 -05:00
parent 97af998066
commit c7ebdb1f80
22 changed files with 1439 additions and 99 deletions
--- 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<void> {
-  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<string | Buffer> {
-  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<void> {
-  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<string | undefined> {
-  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<string[] | Dirent[]> {
-  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<any> {
-  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<void> {
-  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<void> {
-  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<void> {
-  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<void> {
-  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<void> {
-  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<any> {
+  // Use basic validation since lstat is for inspecting symlinks
  const validatedPath = validatePath(filePath);
  return fs.lstat(validatedPath);
 }
--- 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
--- 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';