security: path validation, ReDoS prevention, picomatch, PID-based file lock

- Add validateProjectPath() to reject sensitive system directories
- Add isPathWithinRoot/isPathWithinRootReal for symlink-aware path checks
- Replace hand-rolled glob-to-regex with picomatch to prevent ReDoS
- Add isSafeRegex() to reject custom patterns with nested quantifiers
- Replace FileLock with PID-tracking version that detects stale locks
- Add symlink detection in removeDirectory/listDirectoryContents
- Add subdirectory name validation in ensureSubdirectory
- Add atomicWriteFileSync and corrupted file backup in config-writer
- Add MCP input validation (validateString) for all tool handlers
- Fix CLAUDE.md section replacement to handle ### subsections correctly
This commit is contained in:
Martin Oehlert
2026-02-10 11:22:04 +01:00
parent 4825661e02
commit 399d78b938
11 changed files with 1072 additions and 357 deletions
+31 -9
View File
@@ -6,6 +6,7 @@
import * as fs from 'fs';
import * as path from 'path';
import picomatch from 'picomatch';
import { CodeGraphConfig, DEFAULT_CONFIG, Language, NodeKind } from './types';
/**
@@ -20,6 +21,31 @@ export function getConfigPath(projectRoot: string): string {
return path.join(projectRoot, '.codegraph', CONFIG_FILENAME);
}
/**
* Check if a regex pattern is safe from ReDoS attacks.
*
* Rejects patterns with nested quantifiers (e.g., (a+)+, (a*)*) which
* are the primary source of catastrophic backtracking. Also rejects
* excessively long patterns and validates compilability.
*/
function isSafeRegex(pattern: string): boolean {
// Reject excessively long patterns
if (pattern.length > 500) return false;
// Reject nested quantifiers: (...)+ followed by +, *, or {
// These are the primary cause of catastrophic backtracking
if (/([+*}])\s*[+*{]/.test(pattern)) return false;
if (/\([^)]*[+*][^)]*\)[+*{]/.test(pattern)) return false;
// Verify the pattern is a valid regex
try {
new RegExp(pattern);
return true;
} catch {
return false;
}
}
/**
* Validate a configuration object
*/
@@ -75,6 +101,9 @@ export function validateConfig(config: unknown): config is CodeGraphConfig {
if (typeof p.name !== 'string') return false;
if (typeof p.pattern !== 'string') return false;
if (typeof p.kind !== 'string') return false;
// Validate regex is compilable and reject patterns with known ReDoS risks
if (!isSafeRegex(p.pattern)) return false;
}
}
@@ -243,15 +272,8 @@ export function shouldIncludeFile(filePath: string, config: CodeGraphConfig): bo
// Simple glob matching (for now, just check if any pattern matches)
// A full implementation would use a proper glob library
const matchesPattern = (pattern: string, path: string): boolean => {
// Convert glob to regex (simplified)
const regexStr = pattern
.replace(/\./g, '\\.')
.replace(/\*\*/g, '.*')
.replace(/\*/g, '[^/]*')
.replace(/\?/g, '.');
const regex = new RegExp(`^${regexStr}$`);
return regex.test(path);
const matchesPattern = (pattern: string, filePath: string): boolean => {
return picomatch.isMatch(filePath, pattern, { dot: true });
};
// Check exclude patterns first
+28
View File
@@ -115,6 +115,20 @@ export function removeDirectory(projectRoot: string): void {
return;
}
// Verify .codegraph is a real directory, not a symlink pointing elsewhere
const lstat = fs.lstatSync(codegraphDir);
if (lstat.isSymbolicLink()) {
// Only remove the symlink itself, never follow it for recursive delete
fs.unlinkSync(codegraphDir);
return;
}
if (!lstat.isDirectory()) {
// Not a directory - remove the single file
fs.unlinkSync(codegraphDir);
return;
}
// Recursively remove directory
fs.rmSync(codegraphDir, { recursive: true, force: true });
}
@@ -137,6 +151,11 @@ export function listDirectoryContents(projectRoot: string): string[] {
for (const entry of entries) {
const relativePath = prefix ? `${prefix}/${entry.name}` : entry.name;
// Skip symlinks to prevent following links outside .codegraph
if (entry.isSymbolicLink()) {
continue;
}
if (entry.isDirectory()) {
walkDir(path.join(dir, entry.name), relativePath);
} else {
@@ -165,6 +184,11 @@ export function getDirectorySize(projectRoot: string): number {
const entries = fs.readdirSync(dir, { withFileTypes: true });
for (const entry of entries) {
// Skip symlinks to prevent following links outside .codegraph
if (entry.isSymbolicLink()) {
continue;
}
const fullPath = path.join(dir, entry.name);
if (entry.isDirectory()) {
@@ -184,6 +208,10 @@ export function getDirectorySize(projectRoot: string): number {
* Ensure a subdirectory exists within .codegraph
*/
export function ensureSubdirectory(projectRoot: string, subdirName: string): string {
if (subdirName.includes('..') || subdirName.includes(path.sep) || subdirName.includes('/')) {
throw new Error(`Invalid subdirectory name: ${subdirName}`);
}
const subdirPath = path.join(getCodeGraphDir(projectRoot), subdirName);
if (!fs.existsSync(subdirPath)) {
+2 -20
View File
@@ -68,26 +68,8 @@ export function hashContent(content: string): string {
* Check if a path matches any glob pattern (simplified)
*/
function matchesGlob(filePath: string, pattern: string): boolean {
// Convert glob to regex using placeholders to avoid conflicts
let regexStr = pattern;
// Replace glob patterns with placeholders first
regexStr = regexStr.replace(/\*\*\//g, '\x00GLOBSTAR_SLASH\x00');
regexStr = regexStr.replace(/\*\*/g, '\x00GLOBSTAR\x00');
regexStr = regexStr.replace(/\*/g, '\x00STAR\x00');
regexStr = regexStr.replace(/\?/g, '\x00QUESTION\x00');
// Escape regex special characters
regexStr = regexStr.replace(/[.+^${}()|[\]\\]/g, '\\$&');
// Replace placeholders with regex equivalents
regexStr = regexStr.replace(/\x00GLOBSTAR_SLASH\x00/g, '(?:.*/)?'); // **/ = zero or more dirs
regexStr = regexStr.replace(/\x00GLOBSTAR\x00/g, '.*'); // ** = anything
regexStr = regexStr.replace(/\x00STAR\x00/g, '[^/]*'); // * = anything except /
regexStr = regexStr.replace(/\x00QUESTION\x00/g, '.'); // ? = single char
const regex = new RegExp(`^${regexStr}$`);
return regex.test(filePath);
const picomatch = require('picomatch');
return picomatch.isMatch(filePath, pattern, { dot: true });
}
/**
+12 -7
View File
@@ -149,7 +149,9 @@ export class CodeGraph {
this.queries = queries;
this.config = config;
this.projectRoot = projectRoot;
this.fileLock = new FileLock(db.getPath());
this.fileLock = new FileLock(
path.join(projectRoot, '.codegraph', 'codegraph.lock')
);
this.orchestrator = new ExtractionOrchestrator(projectRoot, config, queries);
this.resolver = createResolver(projectRoot, queries);
this.graphManager = new GraphQueryManager(queries);
@@ -375,8 +377,9 @@ export class CodeGraph {
*/
async indexAll(options: IndexOptions = {}): Promise<IndexResult> {
return this.indexMutex.withLock(async () => {
const locked = await this.fileLock.acquire();
if (!locked) {
try {
this.fileLock.acquire();
} catch {
return { success: false, filesIndexed: 0, filesSkipped: 0, nodesCreated: 0, edgesCreated: 0, errors: [{ message: 'Could not acquire file lock - another process may be indexing', severity: 'error' as const }], durationMs: 0 };
}
try {
@@ -416,8 +419,9 @@ export class CodeGraph {
*/
async indexFiles(filePaths: string[]): Promise<IndexResult> {
return this.indexMutex.withLock(async () => {
const locked = await this.fileLock.acquire();
if (!locked) {
try {
this.fileLock.acquire();
} catch {
return { success: false, filesIndexed: 0, filesSkipped: 0, nodesCreated: 0, edgesCreated: 0, errors: [{ message: 'Could not acquire file lock - another process may be indexing', severity: 'error' as const }], durationMs: 0 };
}
try {
@@ -435,8 +439,9 @@ export class CodeGraph {
*/
async sync(options: IndexOptions = {}): Promise<SyncResult> {
return this.indexMutex.withLock(async () => {
const locked = await this.fileLock.acquire();
if (!locked) {
try {
this.fileLock.acquire();
} catch {
return { filesChecked: 0, filesAdded: 0, filesModified: 0, filesRemoved: 0, nodesUpdated: 0, durationMs: 0 };
}
try {
+47 -20
View File
@@ -46,29 +46,55 @@ function getSettingsJsonPath(location: InstallLocation): string {
}
/**
* Read a JSON file, returning an empty object if it doesn't exist
* Read a JSON file, returning an empty object if it doesn't exist.
* Distinguishes between missing files (returns {}) and corrupted
* files (logs warning, returns {}).
*/
function readJsonFile(filePath: string): Record<string, any> {
try {
if (fs.existsSync(filePath)) {
const content = fs.readFileSync(filePath, 'utf-8');
return JSON.parse(content);
}
} catch {
// Ignore parse errors, return empty object
if (!fs.existsSync(filePath)) {
return {};
}
try {
const content = fs.readFileSync(filePath, 'utf-8');
return JSON.parse(content);
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
console.warn(` Warning: Could not parse ${path.basename(filePath)}: ${msg}`);
console.warn(` A backup will be created before overwriting.`);
// Create a backup of the corrupted file
try {
const backupPath = filePath + '.backup';
fs.copyFileSync(filePath, backupPath);
} catch { /* ignore backup failure */ }
return {};
}
}
/**
* Write a file atomically by writing to a temp file then renaming.
* Prevents corruption if the process crashes mid-write.
*/
function atomicWriteFileSync(filePath: string, content: string): void {
const dir = path.dirname(filePath);
if (!fs.existsSync(dir)) {
fs.mkdirSync(dir, { recursive: true });
}
const tmpPath = filePath + '.tmp.' + process.pid;
try {
fs.writeFileSync(tmpPath, content);
fs.renameSync(tmpPath, filePath);
} catch (err) {
// Clean up temp file on failure
try { fs.unlinkSync(tmpPath); } catch { /* ignore */ }
throw err;
}
return {};
}
/**
* Write a JSON file, creating parent directories if needed
*/
function writeJsonFile(filePath: string, data: Record<string, any>): void {
const dir = path.dirname(filePath);
if (!fs.existsSync(dir)) {
fs.mkdirSync(dir, { recursive: true });
}
fs.writeFileSync(filePath, JSON.stringify(data, null, 2) + '\n');
atomicWriteFileSync(filePath, JSON.stringify(data, null, 2) + '\n');
}
/**
@@ -306,7 +332,7 @@ export function writeClaudeMd(location: InstallLocation): { created: boolean; up
// Check if file exists
if (!fs.existsSync(claudeMdPath)) {
// Create new file with just the CodeGraph section
fs.writeFileSync(claudeMdPath, CLAUDE_MD_TEMPLATE + '\n');
atomicWriteFileSync(claudeMdPath, CLAUDE_MD_TEMPLATE + '\n');
return { created: true, updated: false };
}
@@ -324,7 +350,7 @@ export function writeClaudeMd(location: InstallLocation): { created: boolean; up
const before = content.substring(0, startIdx);
const after = content.substring(endIdx + CODEGRAPH_SECTION_END.length);
content = before + CLAUDE_MD_TEMPLATE + after;
fs.writeFileSync(claudeMdPath, content);
atomicWriteFileSync(claudeMdPath, content);
return { created: false, updated: true };
}
}
@@ -334,10 +360,11 @@ export function writeClaudeMd(location: InstallLocation): { created: boolean; up
const match = content.match(codegraphHeaderRegex);
if (match && match.index !== undefined) {
// Find the end of the CodeGraph section (next ## header or end of file)
// Find the end of the CodeGraph section (next h2 header or end of file)
// Use negative lookahead (?!#) to match "## X" but not "### X"
const sectionStart = match.index;
const afterSection = content.substring(sectionStart + 1);
const nextHeaderMatch = afterSection.match(/\n## [^#]/);
const nextHeaderMatch = afterSection.match(/\n## (?!#)/);
let sectionEnd: number;
if (nextHeaderMatch && nextHeaderMatch.index !== undefined) {
@@ -350,12 +377,12 @@ export function writeClaudeMd(location: InstallLocation): { created: boolean; up
const before = content.substring(0, sectionStart);
const after = content.substring(sectionEnd);
content = before + '\n' + CLAUDE_MD_TEMPLATE + after;
fs.writeFileSync(claudeMdPath, content);
atomicWriteFileSync(claudeMdPath, content);
return { created: false, updated: true };
}
// No existing section, append to end
content = content.trimEnd() + '\n\n' + CLAUDE_MD_TEMPLATE + '\n';
fs.writeFileSync(claudeMdPath, content);
atomicWriteFileSync(claudeMdPath, content);
return { created: false, updated: false };
}
+30 -7
View File
@@ -331,6 +331,16 @@ export class ToolHandler {
this.projectCache.clear();
}
/**
* Validate that a value is a non-empty string
*/
private validateString(value: unknown, name: string): string | ToolResult {
if (typeof value !== 'string' || value.length === 0) {
return this.errorResult(`${name} must be a non-empty string`);
}
return value;
}
/**
* Execute a tool by name
*/
@@ -366,10 +376,13 @@ export class ToolHandler {
* Handle codegraph_search
*/
private async handleSearch(args: Record<string, unknown>): Promise<ToolResult> {
const query = this.validateString(args.query, 'query');
if (typeof query !== 'string') return query;
const cg = this.getCodeGraph(args.projectPath as string | undefined);
const query = args.query as string;
const kind = args.kind as string | undefined;
const limit = clamp((args.limit as number) || 10, 1, 100);
const rawLimit = Number(args.limit) || 10;
const limit = clamp(rawLimit, 1, 100);
const results = cg.searchNodes(query, {
limit,
@@ -388,6 +401,9 @@ export class ToolHandler {
* Handle codegraph_context
*/
private async handleContext(args: Record<string, unknown>): Promise<ToolResult> {
const task = this.validateString(args.task, 'task');
if (typeof task !== 'string') return task;
// Mark session as consulted (enables Grep/Glob/Bash)
const sessionId = process.env.CLAUDE_SESSION_ID;
if (sessionId) {
@@ -395,7 +411,6 @@ export class ToolHandler {
}
const cg = this.getCodeGraph(args.projectPath as string | undefined);
const task = args.task as string;
const maxNodes = (args.maxNodes as number) || 20;
const includeCode = args.includeCode !== false;
@@ -452,8 +467,10 @@ export class ToolHandler {
* Handle codegraph_callers
*/
private async handleCallers(args: Record<string, unknown>): Promise<ToolResult> {
const symbol = this.validateString(args.symbol, 'symbol');
if (typeof symbol !== 'string') return symbol;
const cg = this.getCodeGraph(args.projectPath as string | undefined);
const symbol = args.symbol as string;
const limit = clamp((args.limit as number) || 20, 1, 100);
const match = this.findSymbol(cg, symbol);
@@ -476,8 +493,10 @@ export class ToolHandler {
* Handle codegraph_callees
*/
private async handleCallees(args: Record<string, unknown>): Promise<ToolResult> {
const symbol = this.validateString(args.symbol, 'symbol');
if (typeof symbol !== 'string') return symbol;
const cg = this.getCodeGraph(args.projectPath as string | undefined);
const symbol = args.symbol as string;
const limit = clamp((args.limit as number) || 20, 1, 100);
const match = this.findSymbol(cg, symbol);
@@ -500,8 +519,10 @@ export class ToolHandler {
* Handle codegraph_impact
*/
private async handleImpact(args: Record<string, unknown>): Promise<ToolResult> {
const symbol = this.validateString(args.symbol, 'symbol');
if (typeof symbol !== 'string') return symbol;
const cg = this.getCodeGraph(args.projectPath as string | undefined);
const symbol = args.symbol as string;
const depth = clamp((args.depth as number) || 2, 1, 10);
const match = this.findSymbol(cg, symbol);
@@ -519,8 +540,10 @@ export class ToolHandler {
* Handle codegraph_node
*/
private async handleNode(args: Record<string, unknown>): Promise<ToolResult> {
const symbol = this.validateString(args.symbol, 'symbol');
if (typeof symbol !== 'string') return symbol;
const cg = this.getCodeGraph(args.projectPath as string | undefined);
const symbol = args.symbol as string;
// Default to false to minimize context usage
const includeCode = args.includeCode === true;
+179 -37
View File
@@ -29,13 +29,23 @@
* ```
*/
import * as path from 'path';
import * as fs from 'fs';
import * as path from 'path';
// ============================================================
// SECURITY UTILITIES
// ============================================================
/**
* Sensitive system directories that should never be used as project roots.
* Checked on all platforms; non-applicable paths are harmlessly skipped.
*/
const SENSITIVE_PATHS = new Set([
'/', '/etc', '/usr', '/bin', '/sbin', '/var', '/tmp', '/dev', '/proc', '/sys',
'/root', '/boot', '/lib', '/lib64', '/opt',
'C:\\', 'C:\\Windows', 'C:\\Windows\\System32',
]);
/**
* Validate that a resolved file path stays within the project root.
* Prevents path traversal attacks (e.g. node.filePath = "../../etc/passwd").
@@ -54,6 +64,88 @@ export function validatePathWithinRoot(projectRoot: string, filePath: string): s
return resolved;
}
/**
* Validate that a path is a safe project root directory.
*
* Rejects sensitive system directories and ensures the path is
* a real, existing directory. Used at MCP and API entry points
* to prevent arbitrary directory access.
*
* @param dirPath - The path to validate
* @returns An error message if invalid, or null if valid
*/
export function validateProjectPath(dirPath: string): string | null {
const resolved = path.resolve(dirPath);
// Block sensitive system directories
if (SENSITIVE_PATHS.has(resolved) || SENSITIVE_PATHS.has(resolved.toLowerCase())) {
return `Refusing to operate on sensitive system directory: ${resolved}`;
}
// Also block common sensitive home subdirectories
const homeDir = require('os').homedir();
const sensitiveHomeDirs = ['.ssh', '.gnupg', '.aws', '.config'];
for (const dir of sensitiveHomeDirs) {
const sensitivePath = path.join(homeDir, dir);
if (resolved === sensitivePath || resolved.startsWith(sensitivePath + path.sep)) {
return `Refusing to operate on sensitive directory: ${resolved}`;
}
}
// Verify it's a real directory
try {
const stats = fs.statSync(resolved);
if (!stats.isDirectory()) {
return `Path is not a directory: ${resolved}`;
}
} catch {
return `Path does not exist or is not accessible: ${resolved}`;
}
return null;
}
/**
* Check if a file path resolves to a location within the given root directory.
*
* Prevents path traversal attacks by ensuring the resolved absolute path
* starts with the resolved root path. Handles '..' sequences, symlink-like
* relative paths, and platform-specific separators.
*
* @param filePath - The path to check (can be relative or absolute)
* @param rootDir - The root directory that filePath must stay within
* @returns true if filePath resolves to a location within rootDir
*/
export function isPathWithinRoot(filePath: string, rootDir: string): boolean {
const resolvedPath = path.resolve(rootDir, filePath);
const resolvedRoot = path.resolve(rootDir);
return resolvedPath.startsWith(resolvedRoot + path.sep) || resolvedPath === resolvedRoot;
}
/**
* Like isPathWithinRoot but also resolves symlinks via fs.realpathSync.
*
* This catches symlink escapes where the logical path appears to be within
* root but the real path on disk points elsewhere. Falls back to logical
* path checking if realpath resolution fails (e.g. broken symlink).
*/
export function isPathWithinRootReal(filePath: string, rootDir: string): boolean {
// First do the cheap logical check
if (!isPathWithinRoot(filePath, rootDir)) {
return false;
}
// Then verify with realpath to catch symlink escapes
try {
const realPath = fs.realpathSync(path.resolve(rootDir, filePath));
const realRoot = fs.realpathSync(rootDir);
return realPath.startsWith(realRoot + path.sep) || realPath === realRoot;
} catch {
// If realpath fails (broken symlink, permissions), fall back to logical check
return true;
}
}
/**
* Safely parse JSON with a fallback value.
* Prevents crashes from corrupted database metadata.
@@ -75,63 +167,113 @@ export function clamp(value: number, min: number, max: number): number {
}
/**
* Cross-process file lock using lock files.
* Prevents concurrent database writes from CLI, MCP server, and git hooks.
* Cross-process file lock using a lock file with PID tracking.
*
* Prevents multiple processes (e.g., git hooks, CLI, MCP server) from
* writing to the same database simultaneously.
*/
export class FileLock {
private lockPath: string;
private acquired = false;
private held = false;
constructor(resourcePath: string) {
this.lockPath = resourcePath + '.lock';
constructor(lockPath: string) {
this.lockPath = lockPath;
}
/**
* Acquire the file lock. Waits up to timeoutMs for the lock.
* Cleans up stale locks older than staleLockMs.
* Acquire the lock. Throws if the lock is held by another live process.
*/
async acquire(timeoutMs: number = 10000, staleLockMs: number = 30000): Promise<boolean> {
const start = Date.now();
while (Date.now() - start < timeoutMs) {
acquire(): void {
// Check for existing lock
if (fs.existsSync(this.lockPath)) {
try {
// Try to create lock file exclusively
fs.writeFileSync(this.lockPath, String(process.pid), { flag: 'wx' });
this.acquired = true;
return true;
} catch {
// Lock file exists - check if stale
try {
const stat = fs.statSync(this.lockPath);
if (Date.now() - stat.mtimeMs > staleLockMs) {
// Stale lock - remove and retry
fs.unlinkSync(this.lockPath);
continue;
}
} catch {
// Lock file disappeared between check and stat - retry
continue;
const content = fs.readFileSync(this.lockPath, 'utf-8').trim();
const pid = parseInt(content, 10);
if (!isNaN(pid) && this.isProcessAlive(pid)) {
throw new Error(
`CodeGraph database is locked by another process (PID ${pid}). ` +
`If this is stale, delete ${this.lockPath}`
);
}
// Wait and retry
await new Promise(resolve => setTimeout(resolve, 100));
// Stale lock - remove it
fs.unlinkSync(this.lockPath);
} catch (err) {
if (err instanceof Error && err.message.includes('locked by another')) {
throw err;
}
// Other errors reading lock file - try to remove it
try { fs.unlinkSync(this.lockPath); } catch { /* ignore */ }
}
}
return false;
// Write our PID to the lock file using exclusive create flag
try {
fs.writeFileSync(this.lockPath, String(process.pid), { flag: 'wx' });
this.held = true;
} catch (err: any) {
if (err.code === 'EEXIST') {
// Race condition: another process grabbed the lock between our check and write
throw new Error(
'CodeGraph database is locked by another process. ' +
`If this is stale, delete ${this.lockPath}`
);
}
throw err;
}
}
/**
* Release the file lock
* Release the lock
*/
release(): void {
if (this.acquired) {
try {
if (!this.held) return;
try {
// Only remove if we still own it (check PID)
const content = fs.readFileSync(this.lockPath, 'utf-8').trim();
if (parseInt(content, 10) === process.pid) {
fs.unlinkSync(this.lockPath);
} catch {
// Lock file already removed - that's fine
}
this.acquired = false;
} catch {
// Lock file already gone - that's fine
}
this.held = false;
}
/**
* Execute a function while holding the lock
*/
withLock<T>(fn: () => T): T {
this.acquire();
try {
return fn();
} finally {
this.release();
}
}
/**
* Execute an async function while holding the lock
*/
async withLockAsync<T>(fn: () => Promise<T>): Promise<T> {
this.acquire();
try {
return await fn();
} finally {
this.release();
}
}
/**
* Check if a process is still running
*/
private isProcessAlive(pid: number): boolean {
try {
process.kill(pid, 0);
return true;
} catch {
return false;
}
}
}