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
4 changes: 2 additions & 2 deletions integration-tests/globalSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ if (process.env['NO_COLOR'] !== undefined) {
import { mkdir, readdir, rm, readFile } from 'node:fs/promises';
import { join, dirname, extname } from 'node:path';
import { fileURLToPath } from 'node:url';
import { canUseRipgrep } from '../packages/core/src/tools/ripGrep.js';
import { resolveRipgrepPath } from '../packages/core/src/tools/ripGrep.js';
import { disableMouseTracking } from '@google/gemini-cli-core';
import { isolateTestEnv } from '../packages/test-utils/src/env-setup.js';
import { createServer, type Server } from 'node:http';
Expand Down Expand Up @@ -93,7 +93,7 @@ export async function setup() {
isolateTestEnv(runDir);

// Download ripgrep to avoid race conditions in parallel tests
const available = await canUseRipgrep();
const available = await resolveRipgrepPath();
if (!available) {
throw new Error('Failed to download ripgrep binary');
}
Expand Down
9 changes: 8 additions & 1 deletion integration-tests/ripgrep-real.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest';
import * as path from 'node:path';
import * as fs from 'node:fs/promises';
import * as os from 'node:os';
import { RipGrepTool } from '../packages/core/src/tools/ripGrep.js';
import {
RipGrepTool,
resolveRipgrepPath,
} 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';
Expand Down Expand Up @@ -48,6 +51,10 @@ class MockConfig {
validatePathAccess() {
return null;
}

async getRipgrepPath() {
return resolveRipgrepPath();
}
}

describe('ripgrep-real-direct', () => {
Expand Down
4 changes: 2 additions & 2 deletions memory-tests/globalSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { mkdir, readdir, rm } from 'node:fs/promises';
import { join, dirname } from 'node:path';
import { fileURLToPath } from 'node:url';
import { canUseRipgrep } from '../packages/core/src/tools/ripGrep.js';
import { resolveRipgrepPath } from '../packages/core/src/tools/ripGrep.js';

const __dirname = dirname(fileURLToPath(import.meta.url));
const rootDir = join(__dirname, '..');
Expand All @@ -27,7 +27,7 @@ export async function setup() {
process.env['GEMINI_CONFIG_DIR'] = join(runDir, '.gemini');

// Download ripgrep to avoid race conditions
const available = await canUseRipgrep();
const available = await resolveRipgrepPath();
if (!available) {
throw new Error('Failed to download ripgrep binary');
}
Expand Down
28 changes: 22 additions & 6 deletions packages/core/src/config/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ import { ShellTool } from '../tools/shell.js';
import { AgentTool } from '../agents/agent-tool.js';
import { ReadFileTool } from '../tools/read-file.js';
import { GrepTool } from '../tools/grep.js';
import { RipGrepTool, canUseRipgrep } from '../tools/ripGrep.js';
import { RipGrepTool, resolveRipgrepPath } from '../tools/ripGrep.js';
import {
logRipgrepFallback,
logApprovalModeDuration,
Expand Down Expand Up @@ -89,6 +89,22 @@ vi.mock('fs', async (importOriginal) => {
};
});

vi.mock('../utils/paths.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('../utils/paths.js')>();
return {
...actual,
resolveToRealPath: vi.fn((p) => p),
};
});

vi.mock('../utils/fileUtils.js', () => ({
fileExists: vi.fn(),
}));

vi.mock('../utils/shell-utils.js', () => ({
resolveExecutable: vi.fn(),
}));

// Mock dependencies that might be called during Config construction or createServerConfig
vi.mock('../tools/tool-registry', () => {
const ToolRegistryMock = vi.fn();
Expand Down Expand Up @@ -120,7 +136,7 @@ vi.mock('../tools/ls');
vi.mock('../tools/read-file');
vi.mock('../tools/grep.js');
vi.mock('../tools/ripGrep.js', () => ({
canUseRipgrep: vi.fn(),
resolveRipgrepPath: vi.fn(),
RipGrepTool: class MockRipGrepTool {},
}));
vi.mock('../tools/glob');
Expand Down Expand Up @@ -2288,7 +2304,7 @@ describe('setApprovalMode with folder trust', () => {
});

it('should register RipGrepTool when useRipgrep is true and it is available', async () => {
vi.mocked(canUseRipgrep).mockResolvedValue(true);
vi.mocked(resolveRipgrepPath).mockResolvedValue('/mock/rg');
const config = new Config({ ...baseParams, useRipgrep: true });
await config.initialize();

Expand All @@ -2306,7 +2322,7 @@ describe('setApprovalMode with folder trust', () => {
});

it('should register GrepTool as a fallback when useRipgrep is true but it is not available', async () => {
vi.mocked(canUseRipgrep).mockResolvedValue(false);
vi.mocked(resolveRipgrepPath).mockResolvedValue(null);
const config = new Config({ ...baseParams, useRipgrep: true });
await config.initialize();

Expand All @@ -2330,7 +2346,7 @@ describe('setApprovalMode with folder trust', () => {

it('should register GrepTool as a fallback when canUseRipgrep throws an error', async () => {
const error = new Error('ripGrep check failed');
vi.mocked(canUseRipgrep).mockRejectedValue(error);
vi.mocked(resolveRipgrepPath).mockRejectedValue(error);
const config = new Config({ ...baseParams, useRipgrep: true });
await config.initialize();

Expand Down Expand Up @@ -2366,7 +2382,7 @@ describe('setApprovalMode with folder trust', () => {

expect(wasRipGrepRegistered).toBe(false);
expect(wasGrepRegistered).toBe(true);
expect(canUseRipgrep).not.toHaveBeenCalled();
expect(resolveRipgrepPath).not.toHaveBeenCalled();
expect(logRipgrepFallback).not.toHaveBeenCalled();
});
});
Expand Down
31 changes: 29 additions & 2 deletions packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import { ReadFileTool } from '../tools/read-file.js';
import { ReadMcpResourceTool } from '../tools/read-mcp-resource.js';
import { ListMcpResourcesTool } from '../tools/list-mcp-resources.js';
import { GrepTool } from '../tools/grep.js';
import { canUseRipgrep, RipGrepTool } from '../tools/ripGrep.js';
import { RipGrepTool, resolveRipgrepPath } from '../tools/ripGrep.js';
import { GlobTool } from '../tools/glob.js';
import { ActivateSkillTool } from '../tools/activate-skill.js';
import { EditTool } from '../tools/edit.js';
Expand Down Expand Up @@ -773,6 +773,7 @@ export class Config implements McpContext, AgentLoopContext {
private readonly sandbox: SandboxConfig | undefined;
private _sandboxForbiddenPaths: string[] | undefined;
private readonly targetDir: string;
private _ripgrepPathPromise?: Promise<string | null>;
private workspaceContext: WorkspaceContext;
private readonly debugMode: boolean;
private readonly question: string | undefined;
Expand Down Expand Up @@ -2134,6 +2135,32 @@ export class Config implements McpContext, AgentLoopContext {
return this.targetDir;
}

/**
* Returns the path to the ripgrep binary, or null if not found or unsafe.
* Uses Promise-based caching to prevent race conditions and redundant I/O.
*/
async getRipgrepPath(): Promise<string | null> {
if (!this._ripgrepPathPromise) {
this._ripgrepPathPromise = resolveRipgrepPath();
}
return this._ripgrepPathPromise;
}

/**
* Checks if ripgrep is available.
*/
async canUseRipgrep(): Promise<boolean> {
return (await this.getRipgrepPath()) !== null;
}

/**
* Resets the cached ripgrep path. Used for testing.
* @internal
*/
__resetRipgrepPathCache(): void {
this._ripgrepPathPromise = undefined;
}

getWorkspaceContext(): WorkspaceContext {
return getWorkspaceContextOverride() ?? this.workspaceContext;
}
Expand Down Expand Up @@ -3866,7 +3893,7 @@ export class Config implements McpContext, AgentLoopContext {
let useRipgrep = false;
let errorString: undefined | string = undefined;
try {
useRipgrep = await canUseRipgrep();
useRipgrep = await this.canUseRipgrep();
} catch (error: unknown) {
errorString = String(error);
}
Expand Down
129 changes: 129 additions & 0 deletions packages/core/src/sandbox/utils/commandSafety.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, it, expect, vi, beforeEach } from 'vitest';
import {
isStrictlyApproved,
isKnownSafeCommand,
isDangerousCommand,
} from './commandSafety.js';
import * as paths from '../../utils/paths.js';

vi.mock('../../utils/paths.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('../../utils/paths.js')>();
return {
...actual,
resolveToRealPath: vi.fn((p: string) => p),
isTrustedSystemPath: vi.fn(() => false),
};
});

describe('commandSafety', () => {
beforeEach(() => {
vi.resetAllMocks();
});

describe('rg specific logic', () => {
it('should consider rg safe without unsafe args if path is trusted', () => {
vi.mocked(paths.resolveToRealPath).mockReturnValue('/usr/bin/rg');
vi.mocked(paths.isTrustedSystemPath).mockReturnValue(true);

// Using isKnownSafeCommand which calls isSafeToCallWithExec under the hood
expect(isKnownSafeCommand(['/usr/bin/rg', 'pattern', 'file.txt'])).toBe(
true,
);
expect(paths.resolveToRealPath).toHaveBeenCalledWith('/usr/bin/rg');
expect(paths.isTrustedSystemPath).toHaveBeenCalledWith('/usr/bin/rg');
});

it('should not consider bare rg safe (Search Path Interruption prevention)', () => {
// Bare 'rg' is not an absolute path, so it fails `isTrustedCommandPath`
expect(isKnownSafeCommand(['rg', 'pattern', 'file.txt'])).toBe(false);
});

it('should not consider rg safe with unsafe args even if path is trusted', () => {
vi.mocked(paths.resolveToRealPath).mockReturnValue('/usr/bin/rg');
vi.mocked(paths.isTrustedSystemPath).mockReturnValue(true);

expect(
isKnownSafeCommand(['/usr/bin/rg', '--search-zip', 'pattern']),
).toBe(false);
expect(isKnownSafeCommand(['/usr/bin/rg', '-z', 'pattern'])).toBe(false);
expect(isKnownSafeCommand(['/usr/bin/rg', '--pre=cat', 'pattern'])).toBe(
false,
);
});

it('should consider rg dangerous with unsafe args', () => {
vi.mocked(paths.resolveToRealPath).mockReturnValue('/usr/bin/rg');
vi.mocked(paths.isTrustedSystemPath).mockReturnValue(true);

expect(
isDangerousCommand(['/usr/bin/rg', '--search-zip', 'pattern']),
).toBe(true);
expect(isDangerousCommand(['/usr/bin/rg', '--pre=cat', 'pattern'])).toBe(
true,
);
});

it('should not consider rg safe if path is untrusted', () => {
vi.mocked(paths.resolveToRealPath).mockReturnValue('/tmp/malicious/rg');
vi.mocked(paths.isTrustedSystemPath).mockReturnValue(false);

expect(isKnownSafeCommand(['/tmp/malicious/rg', 'pattern'])).toBe(false);
expect(paths.resolveToRealPath).toHaveBeenCalledWith('/tmp/malicious/rg');
});

it('should not consider rg safe if path resolution throws', () => {
vi.mocked(paths.resolveToRealPath).mockImplementation(() => {
throw new Error('Resolution failed');
});
vi.mocked(paths.isTrustedSystemPath).mockReturnValue(true);

expect(isKnownSafeCommand(['/some/path/rg', 'pattern'])).toBe(false);
});

it('should flag untrusted rg as dangerous if it has unsafe args (Paranoid validation)', () => {
vi.mocked(paths.resolveToRealPath).mockReturnValue('/tmp/malicious/rg');
vi.mocked(paths.isTrustedSystemPath).mockReturnValue(false);

// isDangerousCommand relies on isRipgrepCommand, which strictly identifies intent (name)
// and doesn't care about path safety. So even an untrusted rg will be flagged if it has unsafe args.
expect(isDangerousCommand(['/tmp/malicious/rg', '--search-zip'])).toBe(
true,
);
});
});

describe('isStrictlyApproved', () => {
it('should approve rg if explicitly in approved tools regardless of path', async () => {
// In this case, isStrictlyApproved relies on `tools.includes(command)`
expect(
await isStrictlyApproved(
'/tmp/malicious/rg',
['pattern'],
['/tmp/malicious/rg'],
),
).toBe(true);
});

it('should approve rg if path is trusted', async () => {
vi.mocked(paths.resolveToRealPath).mockReturnValue('/usr/bin/rg');
vi.mocked(paths.isTrustedSystemPath).mockReturnValue(true);

expect(await isStrictlyApproved('/usr/bin/rg', ['pattern'])).toBe(true);
});

it('should reject rg if path is untrusted and not explicitly approved', async () => {
vi.mocked(paths.resolveToRealPath).mockReturnValue('/tmp/malicious/rg');
vi.mocked(paths.isTrustedSystemPath).mockReturnValue(false);

expect(await isStrictlyApproved('/tmp/malicious/rg', ['pattern'])).toBe(
false,
);
});
});
});
25 changes: 23 additions & 2 deletions packages/core/src/sandbox/utils/commandSafety.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,32 @@
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import path from 'node:path';
import { parse as shellParse } from 'shell-quote';
import {
extractStringFromParseEntry,
initializeShellParsers,
splitCommands,
stripShellWrapper,
} from '../../utils/shell-utils.js';
import { isTrustedSystemPath, resolveToRealPath } from '../../utils/paths.js';
Comment thread
cocosheng-g marked this conversation as resolved.

function isRipgrepCommand(cmd: string): boolean {
const cmdBasename = path.basename(cmd);
return cmdBasename === 'rg' || cmdBasename === 'rg.exe';
}

function isTrustedCommandPath(cmd: string): boolean {
if (!path.isAbsolute(cmd)) {
return false;
}
try {
const realPath = resolveToRealPath(cmd);
return isTrustedSystemPath(realPath);
} catch {
return false;
}
}

/**
* Determines if a command is strictly approved for execution on macOS.
Expand Down Expand Up @@ -191,7 +210,9 @@ function isSafeToCallWithExec(args: string[]): boolean {
return !args.some((arg) => unsafeOptions.has(arg));
}

if (cmd === 'rg') {
if (isRipgrepCommand(cmd)) {
if (!isTrustedCommandPath(cmd)) return false;

const unsafeWithArgs = new Set(['--pre', '--hostname-bin']);
const unsafeWithoutArgs = new Set(['--search-zip', '-z']);

Expand Down Expand Up @@ -453,7 +474,7 @@ export function isDangerousCommand(args: string[]): boolean {
return args.some((arg) => unsafeOptions.has(arg));
}

if (cmd === 'rg') {
if (isRipgrepCommand(cmd)) {
const unsafeWithArgs = new Set(['--pre', '--hostname-bin']);
const unsafeWithoutArgs = new Set(['--search-zip', '-z']);

Expand Down
Loading
Loading