Security hardening: path validation, input clamping, safe JSON, file locking

__tests__/security.test.ts

@@ -0,0 +1,291 @@
+/**
+ * Security Hardening Tests
+ *
+ * Tests for path validation, safe JSON parsing, input clamping,
+ * file locking, and atomic config writes.
+ */
+
+import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import * as fs from 'fs';
+import * as path from 'path';
+import * as os from 'os';
+import { validatePathWithinRoot, safeJsonParse, clamp, FileLock } from '../src/utils';
+import { saveConfig, loadConfig, getConfigPath } from '../src/config';
+import { DEFAULT_CONFIG } from '../src/types';
+
+// Create a temporary directory for each test
+function createTempDir(): string {
+  return fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-security-test-'));
+}
+
+// Clean up temporary directory
+function cleanupTempDir(dir: string): void {
+  if (fs.existsSync(dir)) {
+    fs.rmSync(dir, { recursive: true, force: true });
+  }
+}
+
+describe('Security Hardening', () => {
+  let tempDir: string;
+
+  beforeEach(() => {
+    tempDir = createTempDir();
+  });
+
+  afterEach(() => {
+    cleanupTempDir(tempDir);
+  });
+
+  // ==========================================================================
+  // Path Validation
+  // ==========================================================================
+
+  describe('validatePathWithinRoot', () => {
+    it('should accept paths within the root', () => {
+      const result = validatePathWithinRoot(tempDir, 'src/index.ts');
+      expect(result).toBe(path.resolve(tempDir, 'src/index.ts'));
+    });
+
+    it('should accept nested paths within the root', () => {
+      const result = validatePathWithinRoot(tempDir, 'src/db/queries.ts');
+      expect(result).toBe(path.resolve(tempDir, 'src/db/queries.ts'));
+    });
+
+    it('should reject paths that traverse above the root', () => {
+      const result = validatePathWithinRoot(tempDir, '../../etc/passwd');
+      expect(result).toBeNull();
+    });
+
+    it('should reject paths with embedded traversal', () => {
+      const result = validatePathWithinRoot(tempDir, 'src/../../etc/passwd');
+      expect(result).toBeNull();
+    });
+
+    it('should accept the root directory itself', () => {
+      const result = validatePathWithinRoot(tempDir, '.');
+      expect(result).toBe(path.resolve(tempDir));
+    });
+
+    it('should reject absolute paths outside the root', () => {
+      const outsidePath = path.resolve(tempDir, '..', 'outside-file.txt');
+      const result = validatePathWithinRoot(tempDir, outsidePath);
+      expect(result).toBeNull();
+    });
+
+    it('should accept absolute paths within the root', () => {
+      const insidePath = path.join(tempDir, 'src', 'file.ts');
+      const result = validatePathWithinRoot(tempDir, insidePath);
+      expect(result).toBe(insidePath);
+    });
+  });
+
+  // ==========================================================================
+  // Safe JSON Parsing
+  // ==========================================================================
+
+  describe('safeJsonParse', () => {
+    it('should parse valid JSON', () => {
+      const result = safeJsonParse('{"key": "value"}', {});
+      expect(result).toEqual({ key: 'value' });
+    });
+
+    it('should parse valid JSON arrays', () => {
+      const result = safeJsonParse('["a", "b", "c"]', []);
+      expect(result).toEqual(['a', 'b', 'c']);
+    });
+
+    it('should return fallback for invalid JSON', () => {
+      const result = safeJsonParse('not valid json{{{', { default: true });
+      expect(result).toEqual({ default: true });
+    });
+
+    it('should return fallback for empty string', () => {
+      const result = safeJsonParse('', []);
+      expect(result).toEqual([]);
+    });
+
+    it('should return fallback for truncated JSON', () => {
+      const result = safeJsonParse('{"key": "val', undefined);
+      expect(result).toBeUndefined();
+    });
+
+    it('should parse valid primitives', () => {
+      expect(safeJsonParse('42', 0)).toBe(42);
+      expect(safeJsonParse('"hello"', '')).toBe('hello');
+      expect(safeJsonParse('true', false)).toBe(true);
+      expect(safeJsonParse('null', 'fallback')).toBeNull();
+    });
+  });
+
+  // ==========================================================================
+  // Input Clamping
+  // ==========================================================================
+
+  describe('clamp', () => {
+    it('should return value when within range', () => {
+      expect(clamp(5, 1, 10)).toBe(5);
+    });
+
+    it('should clamp to minimum', () => {
+      expect(clamp(-5, 1, 10)).toBe(1);
+      expect(clamp(0, 1, 10)).toBe(1);
+    });
+
+    it('should clamp to maximum', () => {
+      expect(clamp(100, 1, 10)).toBe(10);
+      expect(clamp(11, 1, 10)).toBe(10);
+    });
+
+    it('should handle exact boundary values', () => {
+      expect(clamp(1, 1, 10)).toBe(1);
+      expect(clamp(10, 1, 10)).toBe(10);
+    });
+
+    it('should handle negative ranges', () => {
+      expect(clamp(0, -10, -1)).toBe(-1);
+      expect(clamp(-20, -10, -1)).toBe(-10);
+      expect(clamp(-5, -10, -1)).toBe(-5);
+    });
+  });
+
+  // ==========================================================================
+  // File Lock
+  // ==========================================================================
+
+  describe('FileLock', () => {
+    it('should acquire and release a lock', async () => {
+      const lockTarget = path.join(tempDir, 'test.db');
+      const lock = new FileLock(lockTarget);
+
+      const acquired = await lock.acquire();
+      expect(acquired).toBe(true);
+      expect(fs.existsSync(lockTarget + '.lock')).toBe(true);
+
+      lock.release();
+      expect(fs.existsSync(lockTarget + '.lock')).toBe(false);
+    });
+
+    it('should fail to acquire when another lock is held', async () => {
+      const lockTarget = path.join(tempDir, 'test.db');
+      const lock1 = new FileLock(lockTarget);
+      const lock2 = new FileLock(lockTarget);
+
+      const acquired1 = await lock1.acquire();
+      expect(acquired1).toBe(true);
+
+      // Second lock should time out quickly
+      const acquired2 = await lock2.acquire(300);
+      expect(acquired2).toBe(false);
+
+      lock1.release();
+    });
+
+    it('should allow re-acquiring after release', async () => {
+      const lockTarget = path.join(tempDir, 'test.db');
+      const lock = new FileLock(lockTarget);
+
+      const acquired1 = await lock.acquire();
+      expect(acquired1).toBe(true);
+      lock.release();
+
+      const acquired2 = await lock.acquire();
+      expect(acquired2).toBe(true);
+      lock.release();
+    });
+
+    it('should clean up stale locks', async () => {
+      const lockTarget = path.join(tempDir, 'test.db');
+      const lockPath = lockTarget + '.lock';
+
+      // Create a stale lock file manually
+      fs.writeFileSync(lockPath, '99999', { flag: 'wx' });
+      // Set its mtime to 60 seconds ago
+      const pastTime = new Date(Date.now() - 60000);
+      fs.utimesSync(lockPath, pastTime, pastTime);
+
+      const lock = new FileLock(lockTarget);
+      // staleLockMs=1000 means locks older than 1s are considered stale
+      const acquired = await lock.acquire(5000, 1000);
+      expect(acquired).toBe(true);
+
+      lock.release();
+    });
+
+    it('should be safe to call release when not acquired', () => {
+      const lockTarget = path.join(tempDir, 'test.db');
+      const lock = new FileLock(lockTarget);
+
+      // Should not throw
+      expect(() => lock.release()).not.toThrow();
+    });
+
+    it('should be safe to call release multiple times', async () => {
+      const lockTarget = path.join(tempDir, 'test.db');
+      const lock = new FileLock(lockTarget);
+
+      await lock.acquire();
+      lock.release();
+      // Second release should not throw
+      expect(() => lock.release()).not.toThrow();
+    });
+
+    it('should write PID to lock file', async () => {
+      const lockTarget = path.join(tempDir, 'test.db');
+      const lock = new FileLock(lockTarget);
+
+      await lock.acquire();
+      const content = fs.readFileSync(lockTarget + '.lock', 'utf-8');
+      expect(content).toBe(String(process.pid));
+
+      lock.release();
+    });
+  });
+
+  // ==========================================================================
+  // Atomic Config Writes
+  // ==========================================================================
+
+  describe('Atomic Config Writes', () => {
+    it('should not leave .tmp files after save', () => {
+      const configDir = path.join(tempDir, '.codegraph');
+      fs.mkdirSync(configDir, { recursive: true });
+
+      const config = { ...DEFAULT_CONFIG, rootDir: tempDir };
+      saveConfig(tempDir, config);
+
+      const configPath = getConfigPath(tempDir);
+      expect(fs.existsSync(configPath)).toBe(true);
+      expect(fs.existsSync(configPath + '.tmp')).toBe(false);
+    });
+
+    it('should produce valid JSON after atomic save', () => {
+      const configDir = path.join(tempDir, '.codegraph');
+      fs.mkdirSync(configDir, { recursive: true });
+
+      const config = { ...DEFAULT_CONFIG, rootDir: tempDir };
+      saveConfig(tempDir, config);
+
+      const loaded = loadConfig(tempDir);
+      expect(loaded.version).toBe(DEFAULT_CONFIG.version);
+      expect(loaded.enableEmbeddings).toBe(DEFAULT_CONFIG.enableEmbeddings);
+      expect(loaded.rootDir).toBe(tempDir);
+    });
+
+    it('should overwrite existing config atomically', () => {
+      const configDir = path.join(tempDir, '.codegraph');
+      fs.mkdirSync(configDir, { recursive: true });
+
+      // Save initial config
+      const config1 = { ...DEFAULT_CONFIG, rootDir: tempDir, maxFileSize: 100000 };
+      saveConfig(tempDir, config1);
+
+      // Overwrite with new config
+      const config2 = { ...DEFAULT_CONFIG, rootDir: tempDir, maxFileSize: 200000 };
+      saveConfig(tempDir, config2);
+
+      const loaded = loadConfig(tempDir);
+      expect(loaded.maxFileSize).toBe(200000);
+      expect(fs.existsSync(getConfigPath(tempDir) + '.tmp')).toBe(false);
+    });
+  });
+});

src/config.ts

@@ -154,7 +154,11 @@ export function saveConfig(projectRoot: string, config: CodeGraphConfig): void {
   delete (toSave as Partial<CodeGraphConfig>).rootDir;
 
   const content = JSON.stringify(toSave, null, 2);
-  fs.writeFileSync(configPath, content, 'utf-8');
+
+  // Atomic write: write to temp file then rename to prevent partial/corrupt configs
+  const tmpPath = configPath + '.tmp';
+  fs.writeFileSync(tmpPath, content, 'utf-8');
+  fs.renameSync(tmpPath, configPath);
 }
 
 /**

src/context/index.ts

@@ -6,7 +6,6 @@
  */
 
 import * as fs from 'fs';
-import * as path from 'path';
 import {
   Node,
   Edge,
@@ -25,6 +24,7 @@ import { GraphTraverser } from '../graph';
 import { VectorManager } from '../vectors';
 import { formatContextAsMarkdown, formatContextAsJson } from './formatter';
 import { logDebug, logWarn } from '../errors';
+import { validatePathWithinRoot } from '../utils';
 
 /**
  * Extract likely symbol names from a natural language query
@@ -438,9 +438,9 @@ export class ContextBuilder {
    * Extract code from a node's source file
    */
   private async extractNodeCode(node: Node): Promise<string | null> {
-    const filePath = path.join(this.projectRoot, node.filePath);
+    const filePath = validatePathWithinRoot(this.projectRoot, node.filePath);
 
-    if (!fs.existsSync(filePath)) {
+    if (!filePath || !fs.existsSync(filePath)) {
       return null;
     }
 

src/db/queries.ts

@@ -17,6 +17,7 @@ import {
   SearchOptions,
   SearchResult,
 } from '../types';
+import { safeJsonParse } from '../utils';
 
 /**
  * Database row types (snake_case from SQLite)
@@ -97,8 +98,8 @@ function rowToNode(row: NodeRow): Node {
     isAsync: row.is_async === 1,
     isStatic: row.is_static === 1,
     isAbstract: row.is_abstract === 1,
-    decorators: row.decorators ? JSON.parse(row.decorators) : undefined,
-    typeParameters: row.type_parameters ? JSON.parse(row.type_parameters) : undefined,
+    decorators: row.decorators ? safeJsonParse(row.decorators, undefined) : undefined,
+    typeParameters: row.type_parameters ? safeJsonParse(row.type_parameters, undefined) : undefined,
     updatedAt: row.updated_at,
   };
 }
@@ -111,7 +112,7 @@ function rowToEdge(row: EdgeRow): Edge {
     source: row.source,
     target: row.target,
     kind: row.kind as EdgeKind,
-    metadata: row.metadata ? JSON.parse(row.metadata) : undefined,
+    metadata: row.metadata ? safeJsonParse(row.metadata, undefined) : undefined,
     line: row.line ?? undefined,
     column: row.col ?? undefined,
   };
@@ -129,7 +130,7 @@ function rowToFileRecord(row: FileRow): FileRecord {
     modifiedAt: row.modified_at,
     indexedAt: row.indexed_at,
     nodeCount: row.node_count,
-    errors: row.errors ? JSON.parse(row.errors) : undefined,
+    errors: row.errors ? safeJsonParse(row.errors, undefined) : undefined,
   };
 }
 
@@ -820,7 +821,7 @@ export class QueryBuilder {
       referenceKind: row.reference_kind as EdgeKind,
       line: row.line,
       column: row.col,
-      candidates: row.candidates ? JSON.parse(row.candidates) : undefined,
+      candidates: row.candidates ? safeJsonParse<string[]>(row.candidates, []) : undefined,
     }));
   }
 
@@ -835,7 +836,7 @@ export class QueryBuilder {
       referenceKind: row.reference_kind as EdgeKind,
       line: row.line,
       column: row.col,
-      candidates: row.candidates ? JSON.parse(row.candidates) : undefined,
+      candidates: row.candidates ? safeJsonParse<string[]>(row.candidates, []) : undefined,
     }));
   }