Skip to content

Fix issues with rip grep #18756

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gundermanc merged 3 commits into from
Feb 10, 2026
Merged

Fix issues with rip grep #18756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
709c3d1
Fix context, before, and after params.
gundermanc Feb 10, 2026
55679c2
fix(core): skip regex validation for fixed strings in ripgrep
gundermanc Feb 10, 2026
57a906a
Revert.
gundermanc Feb 10, 2026
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
23 changes: 22 additions & 1 deletion integration-tests/ripgrep-real.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as os from 'node:os';
import { RipGrepTool } from '../packages/core/src/tools/ripGrep.js';
import { Config } from '../packages/core/src/config/config.js';
import { WorkspaceContext } from '../packages/core/src/utils/workspaceContext.js';
import { createMockMessageBus } from '../packages/core/src/test-utils/mock-message-bus.js';

// Mock Config to provide necessary context
class MockConfig {
Expand Down Expand Up @@ -66,7 +67,7 @@ describe('ripgrep-real-direct', () => {
await fs.writeFile(path.join(tempDir, 'file3.txt'), 'goodbye moon\n');

const config = new MockConfig(tempDir) as unknown as Config;
tool = new RipGrepTool(config);
tool = new RipGrepTool(config, createMockMessageBus());
});

afterAll(async () => {
Expand Down Expand Up @@ -108,4 +109,24 @@ describe('ripgrep-real-direct', () => {
expect(result.llmContent).toContain('script.js');
expect(result.llmContent).not.toContain('file1.txt');
});

it('should support context parameters', async () => {
// Create a file with multiple lines
await fs.writeFile(
path.join(tempDir, 'context.txt'),
'line1\nline2\nline3 match\nline4\nline5\n',
);

const invocation = tool.build({
pattern: 'match',
context: 1,
});
const result = await invocation.execute(new AbortController().signal);

expect(result.llmContent).toContain('Found 1 match');
expect(result.llmContent).toContain('context.txt');
expect(result.llmContent).toContain('L2- line2');
expect(result.llmContent).toContain('L3: line3 match');
expect(result.llmContent).toContain('L4- line4');
});
});
113 changes: 70 additions & 43 deletions packages/core/src/tools/ripGrep.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1408,42 +1408,45 @@ describe('RipGrepTool', () => {
expect(result.llmContent).toContain('L1: HELLO world');
});

it.each([
{
name: 'fixed_strings parameter',
params: { pattern: 'hello.world', fixed_strings: true },
mockOutput: {
path: { text: 'fileA.txt' },
line_number: 1,
lines: { text: 'hello.world\n' },
},
expectedArgs: ['--fixed-strings'],
expectedPattern: 'hello.world',
},
])(
'should handle $name',
async ({ params, mockOutput, expectedArgs, expectedPattern }) => {
mockSpawn.mockImplementationOnce(
createMockSpawn({
outputData:
JSON.stringify({ type: 'match', data: mockOutput }) + '\n',
exitCode: 0,
}),
);
it('should handle fixed_strings parameter', async () => {
mockSpawn.mockImplementationOnce(
createMockSpawn({
outputData:
JSON.stringify({
type: 'match',
data: {
path: { text: 'fileA.txt' },
line_number: 1,
lines: { text: 'hello.world\n' },
},
}) + '\n',
exitCode: 0,
}),
);

const invocation = grepTool.build(params);
const result = await invocation.execute(abortSignal);

expect(mockSpawn).toHaveBeenLastCalledWith(
expect.anything(),
expect.arrayContaining(expectedArgs),
expect.anything(),
);
expect(result.llmContent).toContain(
`Found 1 match for pattern "${expectedPattern}"`,
);
},
);
const invocation = grepTool.build({
pattern: 'hello.world',
fixed_strings: true,
});
const result = await invocation.execute(abortSignal);

expect(mockSpawn).toHaveBeenLastCalledWith(
expect.anything(),
expect.arrayContaining(['--fixed-strings']),
expect.anything(),
);
expect(result.llmContent).toContain(
'Found 1 match for pattern "hello.world"',
);
});

it('should allow invalid regex patterns when fixed_strings is true', () => {
const params: RipGrepToolParams = {
pattern: '[[',
fixed_strings: true,
};
expect(grepTool.validateToolParams(params)).toBeNull();
});

it('should handle no_ignore parameter', async () => {
mockSpawn.mockImplementationOnce(
Expand Down Expand Up @@ -1681,19 +1684,42 @@ describe('RipGrepTool', () => {
mockSpawn.mockImplementationOnce(
createMockSpawn({
outputData:
JSON.stringify({
type: 'context',
data: {
path: { text: 'fileA.txt' },
line_number: 1,
lines: { text: 'hello world\n' },
},
}) +
'\n' +
JSON.stringify({
type: 'match',
data: {
path: { text: 'fileA.txt' },
line_number: 2,
lines: { text: 'second line with world\n' },
lines_before: [{ text: 'hello world\n' }],
lines_after: [
{ text: 'third line\n' },
{ text: 'fourth line\n' },
],
},
}) + '\n',
}) +
'\n' +
JSON.stringify({
type: 'context',
data: {
path: { text: 'fileA.txt' },
line_number: 3,
lines: { text: 'third line\n' },
},
}) +
'\n' +
JSON.stringify({
type: 'context',
data: {
path: { text: 'fileA.txt' },
line_number: 4,
lines: { text: 'fourth line\n' },
},
}) +
'\n',
exitCode: 0,
}),
);
Expand Down Expand Up @@ -1721,9 +1747,10 @@ describe('RipGrepTool', () => {
);
expect(result.llmContent).toContain('Found 1 match for pattern "world"');
expect(result.llmContent).toContain('File: fileA.txt');
expect(result.llmContent).toContain('L1- hello world');
expect(result.llmContent).toContain('L2: second line with world');
// Note: Ripgrep JSON output for context lines doesn't include line numbers for context lines directly
// The current parsing only extracts the matched line, so we only assert on that.
expect(result.llmContent).toContain('L3- third line');
expect(result.llmContent).toContain('L4- fourth line');
});
});

Expand Down
41 changes: 25 additions & 16 deletions packages/core/src/tools/ripGrep.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ interface GrepMatch {
filePath: string;
lineNumber: number;
line: string;
isContext?: boolean;
}

class GrepToolInvocation extends BaseToolInvocation<
Expand Down Expand Up @@ -267,8 +268,6 @@ class GrepToolInvocation extends BaseToolInvocation<
return { llmContent: noMatchMsg, returnDisplay: `No matches found` };
}

const wasTruncated = allMatches.length >= totalMaxMatches;

const matchesByFile = allMatches.reduce(
(acc, match) => {
const fileKey = match.filePath;
Expand All @@ -282,16 +281,19 @@ class GrepToolInvocation extends BaseToolInvocation<
{} as Record<string, GrepMatch[]>,
);

const matchCount = allMatches.length;
const matchesOnly = allMatches.filter((m) => !m.isContext);
const matchCount = matchesOnly.length;
const matchTerm = matchCount === 1 ? 'match' : 'matches';

const wasTruncated = matchCount >= totalMaxMatches;

let llmContent = `Found ${matchCount} ${matchTerm} for pattern "${this.params.pattern}" ${searchLocationDescription}${this.params.include ? ` (filter: "${this.params.include}")` : ''}${wasTruncated ? ` (results limited to ${totalMaxMatches} matches for performance)` : ''}:\n---\n`;

for (const filePath in matchesByFile) {
llmContent += `File: ${filePath}\n`;
matchesByFile[filePath].forEach((match) => {
const trimmedLine = match.line.trim();
llmContent += `L${match.lineNumber}: ${trimmedLine}\n`;
const separator = match.isContext ? '-' : ':';
llmContent += `L${match.lineNumber}${separator} ${match.line}\n`;
});
llmContent += '---\n';
}
Expand Down Expand Up @@ -402,11 +404,15 @@ class GrepToolInvocation extends BaseToolInvocation<
allowedExitCodes: [0, 1],
});

let matchesFound = 0;
for await (const line of generator) {
const match = this.parseRipgrepJsonLine(line, absolutePath);
if (match) {
results.push(match);
if (results.length >= maxMatches) {
if (!match.isContext) {
matchesFound++;
}
if (matchesFound >= maxMatches) {
break;
}
}
Expand All @@ -425,11 +431,11 @@ class GrepToolInvocation extends BaseToolInvocation<
): GrepMatch | null {
try {
const json = JSON.parse(line);
if (json.type === 'match') {
const match = json.data;
if (json.type === 'match' || json.type === 'context') {
const data = json.data;
// Defensive check: ensure text properties exist (skips binary/invalid encoding)
if (match.path?.text && match.lines?.text) {
const absoluteFilePath = path.resolve(basePath, match.path.text);
if (data.path?.text && data.lines?.text) {
const absoluteFilePath = path.resolve(basePath, data.path.text);
const relativeCheck = path.relative(basePath, absoluteFilePath);
if (
relativeCheck === '..' ||
Expand All @@ -443,8 +449,9 @@ class GrepToolInvocation extends BaseToolInvocation<

return {
filePath: relativeFilePath || path.basename(absoluteFilePath),
lineNumber: match.line_number,
line: match.lines.text.trimEnd(),
lineNumber: data.line_number,
line: data.lines.text.trimEnd(),
isContext: json.type === 'context',
};
}
}
Expand Down Expand Up @@ -573,10 +580,12 @@ export class RipGrepTool extends BaseDeclarativeTool<
protected override validateToolParamValues(
params: RipGrepToolParams,
): string | null {
try {
new RegExp(params.pattern);
} catch (error) {
return `Invalid regular expression pattern provided: ${params.pattern}. Error: ${getErrorMessage(error)}`;
if (!params.fixed_strings) {
try {
new RegExp(params.pattern);
} catch (error) {
return `Invalid regular expression pattern provided: ${params.pattern}. Error: ${getErrorMessage(error)}`;
}
}
Comment on lines +583 to 589

@gemini-code-assist gemini-code-assist Bot Feb 10, 2026

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.

security-critical critical

Skipping validation for pattern when fixed_strings is true allows for argument injection in the underlying ripgrep command. Since the pattern is passed as a positional argument at line 351 (rgArgs.push(pattern)), an attacker could provide a pattern starting with - to inject arbitrary flags. For example, injecting flags like --pre or --config-path could lead to Remote Code Execution (RCE) or unauthorized file access.

To remediate this, you should either add validation to ensure the pattern does not start with a hyphen when fixed_strings is true, or better, update the command construction in performRipgrepSearch to always use the --regexp (or -e) flag to pass the pattern safely.

Suggested change
if (!params.fixed_strings) {
try {
new RegExp(params.pattern);
} catch (error) {
return `Invalid regular expression pattern provided: ${params.pattern}. Error: ${getErrorMessage(error)}`;
}
}
if (!params.fixed_strings) {
try {
new RegExp(params.pattern);
} catch (error) {
return `Invalid regular expression pattern provided: ${params.pattern}. Error: ${getErrorMessage(error)}`;
}
} else if (params.pattern.startsWith('-')) {
return "Pattern cannot start with '-' when fixed_strings is true to prevent argument injection.";
}


// Only validate path if one is provided
Expand Down
Loading