Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions src/tools/search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,17 @@ export async function searchCode(options: {
}

if (filePattern) {
args.push('-g', filePattern);
const patterns = filePattern
.split('|')
.map(p => p.trim()) // remove surrounding spaces
.filter(Boolean); // drop empty tokens

// If all patterns were empty, return no results
if (patterns.length === 0) {
return [];
}

patterns.forEach(p => args.push('-g', p));
}

// Add pattern and path
Expand Down Expand Up @@ -116,6 +126,12 @@ export async function searchCode(options: {
});
}
} catch (error) {






const errorMessage = error instanceof Error ? error.message : String(error);
capture('server_request_error', {error: `Error parsing ripgrep output: ${errorMessage}`});
console.error(`Error parsing ripgrep output: ${errorMessage}`);
Expand Down Expand Up @@ -152,7 +168,26 @@ export async function searchCodeFallback(options: {
const validPath = await validatePath(rootPath);
const results: SearchResult[] = [];
const regex = new RegExp(pattern, ignoreCase ? 'i' : '');
const fileRegex = filePattern ? new RegExp(filePattern) : null;

// Handle filePattern similarly to main implementation
let fileRegex: RegExp | null = null;
if (filePattern) {
const patterns = filePattern
.split('|')
.map(p => p.trim())
.filter(Boolean);

// If all patterns were empty, return no results
if (patterns.length === 0) {
return [];
}

// Create a regex that matches any of the patterns
const combinedPattern = patterns.map(p =>
p.replace(/\./g, '\\.').replace(/\*/g, '.*').replace(/\?/g, '.')
).join('|');
fileRegex = new RegExp(`^(${combinedPattern})$`);
}
Comment on lines +171 to +190

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fallback glob handling: ReDoS risk and mismatch with ripgrep path semantics

The fallback currently synthesizes a single regex from user-supplied globs. Two problems:

  • Security/DoS: Variable-driven regex with partial escaping can be exploited for catastrophic backtracking (ReDoS), and allows unintended regex metacharacters (e.g., +, {}, [], ()), as flagged by static analysis.
  • Behavior mismatch: ripgrep’s -g matches against the full relative path. The fallback only tests entry.name, so patterns like src/**/*.ts won’t work consistently when the fallback is used.

Prefer using a robust glob matcher (e.g., picomatch/micromatch) and match against the file’s relative path from rootPath. This removes the need for hand-rolled regex and aligns behavior with rg.

Apply this focused refactor within this hunk:

-  // Handle filePattern similarly to main implementation
-  let fileRegex: RegExp | null = null;
+  // Handle filePattern similarly to main implementation
+  // Use a safe glob matcher instead of constructing a regex from user input
+  let fileMatcher: ((relPath: string) => boolean) | null = null;
   if (filePattern) {
     const patterns = filePattern
       .split('|')
       .map(p => p.trim())
       .filter(Boolean);
 
     // If all patterns were empty, return no results
     if (patterns.length === 0) {
       return [];
     }
 
-    // Create a regex that matches any of the patterns
-    const combinedPattern = patterns.map(p => 
-      p.replace(/\./g, '\\.').replace(/\*/g, '.*').replace(/\?/g, '.')
-    ).join('|');
-    fileRegex = new RegExp(`^(${combinedPattern})$`);
+    // Defer to a glob engine for correctness and safety
+    // Note: match against normalized relative paths to mimic ripgrep -g behavior
+    const picomatch = (await import('picomatch')).default;
+    const normalized = patterns.map(p => p.replaceAll('\\', '/'));
+    const isMatch = picomatch(normalized, { nocase: false }); // keep case-sensitive unless you intentionally want otherwise
+    fileMatcher = (relPath: string) => isMatch(relPath.replaceAll('\\', '/'));
   }

And update the usage a few lines below to test the relative path (example snippet; adjust exact placement accordingly):

// near lines ~201-213 where files are handled
const relPath = path.relative(validPath, fullPath);
// If no matcher, accept all; else match by relative path (mimics rg -g)
if (!fileMatcher || fileMatcher(relPath)) {
  const content = await fs.readFile(fullPath, 'utf-8');
  // ...
}

Optionally, if you want parity with the includeHidden flag, plumb it into searchCodeFallback and pass { dot: includeHidden === true } to picomatch.

This removes the ReDoS vector, aligns behavior with ripgrep, and supports path-aware globs like src/**/*.ts.

🧰 Tools
🪛 ast-grep (0.38.6)

[warning] 188-188: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^(${combinedPattern})$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html

(regexp-from-variable)


async function searchDir(dirPath: string) {
if (results.length >= maxResults) return;
Expand Down Expand Up @@ -239,4 +274,4 @@ export async function searchTextInFiles(options: {
excludeDirs: ['node_modules', '.git', 'dist']
});
}
}
}
65 changes: 64 additions & 1 deletion test/test-search-code-edge-cases.js
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,68 @@ async function testManySmallFiles() {
}
}

/**
* Test filePattern with multiple values, including whitespace and empty tokens
*/
async function testFilePatternWithMultipleValues() {
console.log(`${colors.yellow}Testing filePattern with multiple values...${colors.reset}`);

// Create test files
await fs.writeFile(path.join(EDGE_CASE_TEST_DIR, 'file1.ts'), 'export const myTsVar = "patternTs";');
await fs.writeFile(path.join(EDGE_CASE_TEST_DIR, 'file2.js'), 'const myJsVar = "patternJs";');
await fs.writeFile(path.join(EDGE_CASE_TEST_DIR, 'file3.py'), 'my_py_var = "patternPy"');
await fs.writeFile(path.join(EDGE_CASE_TEST_DIR, 'file4.java'), 'String myJavaVar = "patternJava";');
await fs.writeFile(path.join(EDGE_CASE_TEST_DIR, 'file5.go'), 'var myGoVar = "patternGo"');
await fs.writeFile(path.join(EDGE_CASE_TEST_DIR, 'file6.txt'), 'This is a text file.');

// Test with valid multiple patterns
let result = await handleSearchCode({
path: EDGE_CASE_TEST_DIR,
pattern: 'pattern',
filePattern: '*.ts|*.js|*.py'
});
let text = result.content[0].text;
assert(text.includes('file1.ts'), 'Should find match in file1.ts');
assert(text.includes('file2.js'), 'Should find match in file2.js');
assert(text.includes('file3.py'), 'Should find match in file3.py');
assert(!text.includes('file4.java'), 'Should not find match in file4.java');
assert(!text.includes('file5.go'), 'Should not find match in file5.go');

// Test with patterns including whitespace
result = await handleSearchCode({
path: EDGE_CASE_TEST_DIR,
pattern: 'pattern',
filePattern: ' *.ts | *.js '
});
text = result.content[0].text;
assert(text.includes('file1.ts'), 'Should find match with whitespace-padded patterns (file1.ts)');
assert(text.includes('file2.js'), 'Should find match with whitespace-padded patterns (file2.js)');
assert(!text.includes('file3.py'), 'Should not find match with whitespace-padded patterns (file3.py)');

// Test with patterns including empty tokens (e.g., || or leading/trailing |)
result = await handleSearchCode({
path: EDGE_CASE_TEST_DIR,
pattern: 'pattern',
filePattern: '|*.ts||*.py|'
});
text = result.content[0].text;
assert(text.includes('file1.ts'), 'Should find match with empty tokens (file1.ts)');
assert(text.includes('file3.py'), 'Should find match with empty tokens (file3.py)');
assert(!text.includes('file2.js'), 'Should not find match with empty tokens (file2.js)');

// Test with only empty tokens
result = await handleSearchCode({
path: EDGE_CASE_TEST_DIR,
pattern: 'pattern',
filePattern: '|||'
});
text = result.content[0].text;
assert(!text.includes('file1.ts'), 'Should not find any matches with only empty patterns');
assert(!text.includes('file2.js'), 'Should not find any matches with only empty patterns');

console.log(`${colors.green}✓ FilePattern with multiple values test passed${colors.reset}`);
}

/**
* Main test runner for edge cases
*/
Expand All @@ -386,7 +448,8 @@ export async function testSearchCodeEdgeCases() {
await testLargeContextLines();
await testPathTraversalSecurity();
await testManySmallFiles();

await testFilePatternWithMultipleValues();

console.log(`${colors.green}✅ All handleSearchCode edge case tests passed!${colors.reset}`);
return true;

Expand Down