Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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
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 @@ -3805,7 +3832,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