Skip to content

Commit 268831a

Browse files
committed
feat: comprehensive security hardening
- Add ReDoS prevention for regex patterns in symbol/AST search - Fix UNC extended-length path bypass on Windows - Fix symlink attack vulnerability in ctags temp file creation - Add 30-second timeout to external process calls - Add 100MB file size limit for AST parsing - Add recursion depth limits (100) for AST traversal - Sanitize error messages to prevent path leakage - Add 61 new security tests across 3 test suites
1 parent 0e02f8b commit 268831a

13 files changed

Lines changed: 1038 additions & 22 deletions

.claude/commands/new-command.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
Generate a new command
2+
3+
You must assist the user with creating a new command, based on their input: #$ARGUMENTS
4+
5+
If the user specifies it needs an input, you must use #$ ARGUMENTS (without the space). This is how you provide the user's input to yourself (when this command you're making is ran)
6+
7+
Commands are located here : .claude\commands
8+
9+
You must create them like command-name.md, and structure them like optimized instructions for yourself

.claude/commands/security-audit.md

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
# Security Audit Command for Code Search MCP
2+
3+
You are performing a comprehensive security audit of the Code Search MCP server - a Node.js application that provides code search capabilities via the Model Context Protocol. This is a security-sensitive tool that accepts path inputs and spawns external processes.
4+
5+
## Project Context
6+
7+
This project:
8+
- Is a Node.js MCP server accepting workspace paths from external clients
9+
- Spawns external processes (ctags, ripgrep) via `child_process.execFile`
10+
- Writes temporary files to user-controlled directories
11+
- Implements an allow-list-based workspace access control system
12+
- Caches symbol indices to disk in `~/.code-search-mcp-cache/`
13+
14+
## Phase 1: Parallel Exploration
15+
16+
Launch multiple Explore agents in parallel to review different focused security areas:
17+
18+
### Agent 1: Path Traversal & Workspace Access Control
19+
Focus: `src/utils/workspace-path.ts`, `src/mcp/server.ts`
20+
- Check for path traversal bypasses in `validateAllowedPath()`
21+
- Look for symlink-based escape vulnerabilities
22+
- Verify Windows vs Unix path separator handling
23+
- Check if `..` or path separator edge cases are handled
24+
- Look for cases where workspace validation is bypassed
25+
26+
### Agent 2: Command Injection & Process Spawning
27+
Focus: `src/symbol-search/ctags-integration.ts`, `src/symbol-search/text-search-service.ts`, `src/ast-search/`
28+
- Verify all `execFile` calls use safe argument passing
29+
- Check if workspace paths passed as `cwd` are properly validated
30+
- Look for shell command injection via pattern/regex inputs
31+
- Verify ripgrep glob patterns can't escape the workspace
32+
33+
### Agent 3: Temporary File & Symlink Attacks
34+
Focus: `src/symbol-search/ctags-integration.ts`, `src/cache/cache-manager.ts`
35+
- Check `.code-search-tags` file creation for symlink vulnerabilities
36+
- Verify cache directory creation is safe from TOCTOU races
37+
- Check if temporary files use secure permissions
38+
- Look for arbitrary file write vulnerabilities
39+
40+
### Agent 4: Input Validation & Injection
41+
Focus: `src/mcp/server.ts` - all tool handlers
42+
- Verify all MCP tool inputs are validated before use
43+
- Check regex/pattern injection points (ripgrep, AST search)
44+
- Verify glob pattern sanitization in file/text search
45+
- Look for unsafe JSON parsing
46+
47+
### Agent 5: Cache Security & Information Disclosure
48+
Focus: `src/cache/cache-manager.ts`
49+
- Check if cached data contains sensitive file contents
50+
- Verify cache files are not world-readable
51+
- Check for information disclosure in error messages
52+
- Look for workspace path leakage in responses
53+
54+
### Agent 6: Dependency Vulnerabilities
55+
Focus: `package.json`, all `src/dependency-analysis/parsers/*.ts`
56+
- Check for known vulnerabilities in dependencies
57+
- Verify dependency manifests are parsed safely
58+
- Look for malicious package detection capabilities
59+
- Check if `analyze_dependencies` has network exposure
60+
61+
### Agent 7: Denial of Service & Resource Limits
62+
Focus: All services
63+
- Check for missing timeout constraints on operations
64+
- Verify search result limits are enforced
65+
- Look for memory exhaustion via large file inputs
66+
- Check if unbounded loops exist in parsers
67+
68+
### Agent 8: Access Control Bypasses
69+
Focus: `src/mcp/server.ts`, `src/utils/workspace-path.ts`
70+
- Verify all file operations go through workspace validation
71+
- Check for direct file reads bypassing `validateAllowedPath()`
72+
- Look for cases where `normalizeSearchPathFilters` can be bypassed
73+
- Verify cache operations can't access arbitrary workspaces
74+
75+
## Phase 2: Collect and Analyze
76+
77+
Wait for all agents to complete. Organize findings by severity:
78+
- **Critical**: Path traversal, arbitrary file read/write, command execution
79+
- **High**: Symlink attacks, significant DoS vectors, information disclosure
80+
- **Medium**: DoS resource exhaustion, minor injection risks
81+
- **Low**: Best practice violations, minor issues
82+
- **Info**: Security considerations
83+
84+
For each finding, gather:
85+
- File path and line number
86+
- Vulnerability type (e.g., CWE-22, CWE-78, CWE-20)
87+
- Severity level
88+
- Brief description with exploit scenario
89+
- Recommended fix with code snippet
90+
91+
## Phase 3: Present Results
92+
93+
If NO issues are found:
94+
```
95+
╔══════════════════════════════════════════════════════════════╗
96+
║ SECURITY AUDIT PASSED ║
97+
║ ║
98+
║ No critical security issues detected in code-search-mcp. ║
99+
╚══════════════════════════════════════════════════════════════╝
100+
```
101+
102+
If issues are found, present an ASCII table:
103+
```
104+
╔══════════════════╤════════════════════════════╤═══════════════════════════╤════════════╗
105+
║ Severity │ Issue Type (CWE) │ Location │ Description ║
106+
╠══════════════════╪════════════════════════════╪═══════════════════════════╪════════════╣
107+
║ CRITICAL │ Path Traversal (CWE-22) │ workspace-path.ts:68 │ Symlink ║
108+
║ │ │ │ bypass via ║
109+
║ │ │ │ junction ║
110+
╠══════════════════╪════════════════════════════╪═══════════════════════════╪════════════╣
111+
║ HIGH │ Symlink Attack (CWE-59) │ ctags-integration.ts:19 │ .code- ║
112+
║ │ │ │ search-tags ║
113+
║ │ │ │ link target ║
114+
╚══════════════════╧════════════════════════════╧═══════════════════════════╧════════════╝
115+
```
116+
117+
## Phase 4: Remediation Planning
118+
119+
After presenting findings, gather fix details ahead of time, then use AskUserQuestion to confirm:
120+
121+
1. **Fix scope**: Critical only? Critical+High? All issues?
122+
2. **Fix approach**: Implement fixes directly, create PR, or review together?
123+
3. **Testing**: Add security tests? Verify existing tests pass?
124+
125+
Proceed with implementation based on user responses.
126+
127+
## Key Security Considerations for This Project
128+
129+
1. **Path Validation is Critical**: This tool's primary security boundary is `validateAllowedPath()`. Any bypass allows reading arbitrary files.
130+
131+
2. **Process Spinning**: Every `execFile` call with user-controlled `cwd` is a potential vulnerability.
132+
133+
3. **MCP Protocol**: The server accepts input from external MCP clients - assume all input is hostile.
134+
135+
4. **Temporary Files**: Files written to user-controlled directories are symlink attack targets.
136+
137+
5. **Workspace Enumeration**: Error messages should not leak valid workspace paths.

.claude/settings.local.json

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@
1515
"mcp__code-search-mcp__search_ast_rule",
1616
"Bash(npm test:*)",
1717
"Skill(readme-generator)",
18-
"Skill(repo-sweep)"
18+
"Skill(repo-sweep)",
19+
"Bash(gh pr view:*)",
20+
"Bash(gh pr diff:*)",
21+
"Bash(npx jest:*)"
1922
],
2023
"deny": [],
2124
"ask": []

src/ast-search/ast-search-service.ts

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,11 @@ import langYaml = require('@ast-grep/lang-yaml');
2222
import { promises as fs } from 'fs';
2323
import path from 'path';
2424
import fastGlob from 'fast-glob';
25+
import {
26+
safeRegex,
27+
MAX_AST_FILE_SIZE,
28+
MAX_AST_RECURSION_DEPTH,
29+
} from '../utils/security.js';
2530
import type {
2631
ASTPatternSearchOptions,
2732
ASTRuleSearchOptions,
@@ -154,6 +159,28 @@ export class ASTSearchService {
154159
}
155160
}
156161

162+
/**
163+
* Read file with size limit to prevent memory exhaustion.
164+
* Throws if file exceeds maximum size.
165+
*/
166+
private async readFileWithSizeLimit(filePath: string): Promise<string> {
167+
const stats = await fs.stat(filePath);
168+
if (stats.size > MAX_AST_FILE_SIZE) {
169+
throw new Error(
170+
`File ${path.basename(filePath)} exceeds size limit (${Math.round(stats.size / 1024 / 1024)}MB > ${Math.round(MAX_AST_FILE_SIZE / 1024 / 1024)}MB)`
171+
);
172+
}
173+
return fs.readFile(filePath, 'utf-8');
174+
}
175+
176+
/**
177+
* Validate regex pattern for security (prevent ReDoS).
178+
* Returns null if pattern is unsafe.
179+
*/
180+
private validatePattern(pattern: string): RegExp | null {
181+
return safeRegex(pattern);
182+
}
183+
157184
/**
158185
* Search using a simple pattern
159186
*/
@@ -184,14 +211,19 @@ export class ASTSearchService {
184211
// Search each file
185212
for (const file of files) {
186213
try {
187-
const content = await fs.readFile(file, 'utf-8');
214+
const content = await this.readFileWithSizeLimit(file);
188215
const ast = parse(astLang, content);
189216
const root = ast.root();
190217

191-
// Find all matches
218+
// Find all matches with iteration limit
192219
const nodes = root.findAll(options.pattern);
193-
220+
let matchCount = 0;
194221
for (const node of nodes) {
222+
if (matchCount >= MAX_AST_RECURSION_DEPTH) {
223+
break;
224+
}
225+
matchCount++;
226+
195227
const range = node.range();
196228
const fullText = node.text();
197229
const { truncated, totalLines } = this.truncateText(fullText, options.maxLines ?? 3);
@@ -267,14 +299,19 @@ export class ASTSearchService {
267299
// Search each file
268300
for (const file of files) {
269301
try {
270-
const content = await fs.readFile(file, 'utf-8');
302+
const content = await this.readFileWithSizeLimit(file);
271303
const ast = parse(astLang, content);
272304
const root = ast.root();
273305

274-
// Apply rule
306+
// Apply rule with iteration limit
275307
const nodes = this.applyRule(root, options.rule);
276-
308+
let matchCount = 0;
277309
for (const node of nodes) {
310+
if (matchCount >= MAX_AST_RECURSION_DEPTH) {
311+
break;
312+
}
313+
matchCount++;
314+
278315
const range = node.range();
279316
const fullText = node.text();
280317
const { truncated, totalLines } = this.truncateText(fullText, options.maxLines ?? 3);

src/cache/cache-manager.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import path from 'path';
88
import os from 'os';
99
import type { SymbolIndex, SymbolResult, SupportedLanguage } from '../types/index.js';
1010
import { createHash } from 'crypto';
11+
import { CACHE_DIR_PERMISSIONS, CACHE_FILE_PERMISSIONS } from '../utils/security.js';
1112

1213
const CACHE_VERSION = '1.0.0';
1314
const CACHE_DIR_NAME = '.code-search-mcp-cache';
@@ -55,15 +56,18 @@ export class CacheManager {
5556
}
5657

5758
/**
58-
* Initialize the cache directory.
59+
* Initialize the cache directory with secure permissions.
5960
*/
6061
async initialize(): Promise<void> {
6162
if (!this.enableCache) {
6263
return;
6364
}
6465

6566
try {
66-
await fs.mkdir(this.cacheDir, { recursive: true });
67+
await fs.mkdir(this.cacheDir, {
68+
recursive: true,
69+
mode: CACHE_DIR_PERMISSIONS,
70+
});
6771
} catch {
6872
this.enableCache = false;
6973
}
@@ -281,7 +285,11 @@ export class CacheManager {
281285
};
282286

283287
const cacheFilePath = this.getCacheFilePath(workspaceId);
284-
await fs.writeFile(cacheFilePath, JSON.stringify(cached, null, 2), 'utf-8');
288+
await fs.writeFile(
289+
cacheFilePath,
290+
JSON.stringify(cached, null, 2),
291+
{ mode: CACHE_FILE_PERMISSIONS }
292+
);
285293
} catch {
286294
// Don't throw - caching is optional
287295
}

src/symbol-search/ctags-integration.ts

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,19 +6,58 @@ import { execFile } from 'child_process';
66
import { promisify } from 'util';
77
import { promises as fs } from 'fs';
88
import path from 'path';
9+
import os from 'os';
10+
import { createHash } from 'crypto';
911
import { ctagsPath } from '@LLMTooling/universal-ctags-node';
1012
import type { CTagsTag, SupportedLanguage } from '../types/index.js';
13+
import { PROCESS_TIMEOUT } from '../utils/security.js';
1114

1215
const execFileAsync = promisify(execFile);
1316

17+
/**
18+
* Generate a unique temp filename for ctags output.
19+
* Uses a hash of the workspace path to ensure consistency.
20+
*/
21+
function getTagsFilePath(workspaceRoot: string): string {
22+
const hash = createHash('sha256').update(workspaceRoot).digest('hex').substring(0, 16);
23+
return path.join(os.tmpdir(), `code-search-tags-${hash}.tmp`);
24+
}
25+
26+
/**
27+
* Check if a file exists and is a symlink.
28+
* Returns true if the path is a symlink.
29+
*/
30+
async function isSymlink(filePath: string): Promise<boolean> {
31+
try {
32+
const stats = await fs.lstat(filePath);
33+
return stats.isSymbolicLink();
34+
} catch {
35+
return false;
36+
}
37+
}
38+
1439
/**
1540
* Run universal-ctags on a workspace directory.
41+
* Uses a secure temporary file to prevent symlink attacks.
1642
*/
1743
export async function runCTags(workspaceRoot: string): Promise<CTagsTag[]> {
18-
// Create a temporary tags file path
19-
const tagsFile = path.join(workspaceRoot, '.code-search-tags');
44+
// Use system temp directory instead of workspace root to prevent symlink attacks
45+
const tagsFile = getTagsFilePath(workspaceRoot);
2046

2147
try {
48+
// Ensure the tags file doesn't exist or is not a symlink (TOCTOU protection)
49+
try {
50+
const existingIsSymlink = await isSymlink(tagsFile);
51+
if (existingIsSymlink) {
52+
throw new Error('Security: Refusing to overwrite symlink at tags file location');
53+
}
54+
} catch (error) {
55+
if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
56+
throw error;
57+
}
58+
// File doesn't exist, which is fine
59+
}
60+
2261
// Run ctags with appropriate options
2362
const args = [
2463
'--languages=Java,Python,JavaScript,TypeScript,C#,Go,Rust,C,C++,PHP,Ruby,Kotlin',
@@ -34,6 +73,7 @@ export async function runCTags(workspaceRoot: string): Promise<CTagsTag[]> {
3473
await execFileAsync(ctagsPath, args, {
3574
cwd: workspaceRoot,
3675
maxBuffer: 50 * 1024 * 1024, // 50MB buffer for large projects
76+
timeout: PROCESS_TIMEOUT, // Add timeout to prevent hangs
3777
});
3878

3979
// Read and parse the tags file

src/symbol-search/symbol-search-service.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import type {
1010
} from '../types/index.js';
1111
import { SymbolIndexer } from './symbol-indexer.js';
1212
import { getDefaultKinds } from './language-profiles.js';
13+
import { safeRegex } from '../utils/security.js';
1314

1415
export class SymbolSearchService {
1516
constructor(private indexer: SymbolIndexer) {}
@@ -86,13 +87,13 @@ export class SymbolSearchService {
8687
case 'substring':
8788
return symbolName.toLowerCase().includes(searchTerm.toLowerCase());
8889
case 'regex': {
89-
try {
90-
const regex = new RegExp(searchTerm);
90+
// Use safeRegex to prevent ReDoS attacks
91+
const regex = safeRegex(searchTerm);
92+
if (regex) {
9193
return regex.test(symbolName);
92-
} catch {
93-
// Invalid regex, treat as literal substring
94-
return symbolName.toLowerCase().includes(searchTerm.toLowerCase());
9594
}
95+
// Invalid or unsafe regex, treat as literal substring
96+
return symbolName.toLowerCase().includes(searchTerm.toLowerCase());
9697
}
9798
default: {
9899
const _exhaustive: never = mode;

0 commit comments

Comments
 (0)