Improve sandbox error matching and caching (#24550)

This commit is contained in:
David Pierce
2026-04-07 21:08:18 +00:00
committed by GitHub
parent 9637fb3990
commit adf7b3b717
10 changed files with 324 additions and 51 deletions

View File

@@ -27,11 +27,16 @@ import {
verifySandboxOverrides,
getCommandName,
} from '../utils/commandUtils.js';
import { assertValidPathString } from '../../utils/paths.js';
import {
isKnownSafeCommand,
isDangerousCommand,
} from '../utils/commandSafety.js';
import { parsePosixSandboxDenials } from '../utils/sandboxDenialUtils.js';
import {
parsePosixSandboxDenials,
createSandboxDenialCache,
type SandboxDenialCache,
} from '../utils/sandboxDenialUtils.js';
import { handleReadWriteCommands } from '../utils/sandboxReadWriteUtils.js';
import { buildBwrapArgs } from './bwrapArgsBuilder.js';
@@ -108,6 +113,7 @@ function getSeccompBpfPath(): string {
* Ensures a file or directory exists.
*/
function touch(filePath: string, isDirectory: boolean) {
assertValidPathString(filePath);
try {
// If it exists (even as a broken symlink), do nothing
if (fs.lstatSync(filePath)) return;
@@ -129,6 +135,7 @@ function touch(filePath: string, isDirectory: boolean) {
export class LinuxSandboxManager implements SandboxManager {
private static maskFilePath: string | undefined;
private readonly denialCache: SandboxDenialCache = createSandboxDenialCache();
constructor(private readonly options: GlobalSandboxOptions) {}
@@ -141,7 +148,7 @@ export class LinuxSandboxManager implements SandboxManager {
}
parseDenials(result: ShellExecutionResult): ParsedSandboxDenial | undefined {
return parsePosixSandboxDenials(result);
return parsePosixSandboxDenials(result, this.denialCache);
}
getWorkspace(): string {

View File

@@ -32,10 +32,16 @@ import {
getCommandName as getFullCommandName,
isStrictlyApproved,
} from '../utils/commandUtils.js';
import { parsePosixSandboxDenials } from '../utils/sandboxDenialUtils.js';
import {
parsePosixSandboxDenials,
createSandboxDenialCache,
type SandboxDenialCache,
} from '../utils/sandboxDenialUtils.js';
import { handleReadWriteCommands } from '../utils/sandboxReadWriteUtils.js';
export class MacOsSandboxManager implements SandboxManager {
private readonly denialCache: SandboxDenialCache = createSandboxDenialCache();
constructor(private readonly options: GlobalSandboxOptions) {}
isKnownSafeCommand(args: string[]): boolean {
@@ -52,7 +58,7 @@ export class MacOsSandboxManager implements SandboxManager {
}
parseDenials(result: ShellExecutionResult): ParsedSandboxDenial | undefined {
return parsePosixSandboxDenials(result);
return parsePosixSandboxDenials(result, this.denialCache);
}
getWorkspace(): string {

View File

@@ -0,0 +1,52 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
import fs from 'node:fs';
import path from 'node:path';
import os from 'node:os';
import { tryRealpath } from './fsUtils.js';
describe('fsUtils', () => {
let tempDir: string;
let realTempDir: string;
beforeAll(() => {
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'fs-utils-test-'));
realTempDir = fs.realpathSync(tempDir);
});
afterAll(() => {
fs.rmSync(tempDir, { recursive: true, force: true });
});
describe('tryRealpath', () => {
it('should throw error for paths with null bytes', () => {
expect(() => tryRealpath(path.join(tempDir, 'foo\0bar'))).toThrow(
'Invalid path',
);
});
it('should resolve existing paths', () => {
const resolved = tryRealpath(tempDir);
expect(resolved).toBe(realTempDir);
});
it('should handle non-existent paths by resolving parent', () => {
const nonExistentPath = path.join(tempDir, 'non-existent-file-12345');
const expected = path.join(realTempDir, 'non-existent-file-12345');
const resolved = tryRealpath(nonExistentPath);
expect(resolved).toBe(expected);
});
it('should handle nested non-existent paths', () => {
const nonExistentPath = path.join(tempDir, 'dir1', 'dir2', 'file');
const expected = path.join(realTempDir, 'dir1', 'dir2', 'file');
const resolved = tryRealpath(nonExistentPath);
expect(resolved).toBe(expected);
});
});
});

View File

@@ -6,12 +6,14 @@
import fs from 'node:fs';
import path from 'node:path';
import { assertValidPathString } from '../../utils/paths.js';
export function isErrnoException(e: unknown): e is NodeJS.ErrnoException {
return e instanceof Error && 'code' in e;
}
export function tryRealpath(p: string): string {
assertValidPathString(p);
try {
return fs.realpathSync(p);
} catch (e) {

View File

@@ -5,7 +5,10 @@
*/
import { describe, it, expect } from 'vitest';
import { parsePosixSandboxDenials } from './sandboxDenialUtils.js';
import {
parsePosixSandboxDenials,
createSandboxDenialCache,
} from './sandboxDenialUtils.js';
import type { ShellExecutionResult } from '../../services/shellExecutionService.js';
describe('parsePosixSandboxDenials', () => {
@@ -116,4 +119,109 @@ EACCES: permission denied, mkdir '/Users/galzahavi/.pnpm-store/v3'
expect(parsed).toBeDefined();
expect(parsed?.filePaths).toContain('/Users/galzahavi/.pnpm-store/v3');
});
it('should detect Python PermissionError and extract path accurately', () => {
const output = `Caught exception: [Errno 13] Permission denied: '/etc/test_sandbox_denial'
Traceback (most recent call last):
File "/usr/local/google/home/davidapierce/gemini-cli/repro_sandbox.py", line 9, in <module>
raise e
File "/usr/local/google/home/davidapierce/gemini-cli/repro_sandbox.py", line 5, in <module>
with open('/etc/test_sandbox_denial', 'w') as f:
~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
PermissionError: [Errno 13] Permission denied: '/etc/test_sandbox_denial'`;
const parsed = parsePosixSandboxDenials({
output,
exitCode: 1,
error: null,
} as unknown as ShellExecutionResult);
expect(parsed?.filePaths).toEqual(['/etc/test_sandbox_denial']);
});
it('should detect new keywords like "access denied" and "forbidden"', () => {
const parsed1 = parsePosixSandboxDenials({
output: 'Access denied to /var/log/syslog',
exitCode: 1,
error: null,
} as unknown as ShellExecutionResult);
expect(parsed1?.filePaths).toContain('/var/log/syslog');
const parsed2 = parsePosixSandboxDenials({
output: 'Forbidden: access to /root/secret is not allowed',
exitCode: 1,
error: null,
} as unknown as ShellExecutionResult);
expect(parsed2?.filePaths).toContain('/root/secret');
});
it('should detect read-only file system error', () => {
const parsed = parsePosixSandboxDenials({
output: 'rm: cannot remove /mnt/usb/test: Read-only file system',
exitCode: 1,
error: null,
} as unknown as ShellExecutionResult);
expect(parsed?.filePaths).toContain('/mnt/usb/test');
});
it('should reject paths with directory traversal', () => {
const output = 'ls: /etc/shadow/../../etc/passwd: Operation not permitted';
const parsed = parsePosixSandboxDenials({
output,
} as unknown as ShellExecutionResult);
expect(parsed?.filePaths || []).not.toContain(
'/etc/shadow/../../etc/passwd',
);
});
it('should reject home-relative paths with directory traversal', () => {
const output = "Operation not permitted, open '~/../../etc/shadow'";
const parsed = parsePosixSandboxDenials({
output,
} as unknown as ShellExecutionResult);
expect(parsed?.filePaths || []).not.toContain('~/../../etc/shadow');
});
it('should reject paths with null bytes', () => {
const output = "Operation not permitted, open '/etc/passwd\0/foo'";
const parsed = parsePosixSandboxDenials({
output,
} as unknown as ShellExecutionResult);
expect(parsed?.filePaths || []).not.toContain('/etc/passwd\0/foo');
});
it('should reject paths with internal tildes', () => {
const output = "Operation not permitted, open '/home/user/~/config'";
const parsed = parsePosixSandboxDenials({
output,
} as unknown as ShellExecutionResult);
expect(parsed?.filePaths || []).not.toContain('/home/user/~/config');
});
it('should suppress redundant denials if cache is provided', () => {
const cache = createSandboxDenialCache();
const result = {
output: 'ls: /root: Operation not permitted',
} as unknown as ShellExecutionResult;
// First call: should process
const parsed1 = parsePosixSandboxDenials(result, cache);
expect(parsed1).toBeDefined();
// Second call: should be suppressed
const parsed2 = parsePosixSandboxDenials(result, cache);
expect(parsed2).toBeUndefined();
});
it('should not suppress denials if no cache is provided', () => {
const result = {
output: 'ls: /root: Operation not permitted',
} as unknown as ShellExecutionResult;
const parsed1 = parsePosixSandboxDenials(result);
expect(parsed1).toBeDefined();
const parsed2 = parsePosixSandboxDenials(result);
expect(parsed2).toBeDefined();
});
});

View File

@@ -4,8 +4,58 @@
* SPDX-License-Identifier: Apache-2.0
*/
import { LRUCache } from 'mnemonist';
import { type ParsedSandboxDenial } from '../../services/sandboxManager.js';
import type { ShellExecutionResult } from '../../services/shellExecutionService.js';
import { isValidPathString } from '../../utils/paths.js';
/**
* Type for the sandbox denial error cache.
* Stores normalized error output to prevent redundant processing.
*/
export type SandboxDenialCache = LRUCache<string, boolean>;
/**
* Creates a new sandbox denial cache with a standard LRU policy.
*/
export function createSandboxDenialCache(maxSize = 10): SandboxDenialCache {
return new LRUCache<string, boolean>(maxSize);
}
/**
* Sanitizes extracted paths to prevent path traversal vulnerabilities.
* Filters out paths containing '..' or null bytes.
*/
export function sanitizeExtractedPath(p: string): string | undefined {
if (!isValidPathString(p)) return undefined;
// Reject paths with directory traversal components
const parts = p.split(/[/\\]/);
if (parts.includes('..')) {
return undefined;
}
// Reject paths with internal tildes (tilde should only be at the beginning)
if (p.indexOf('~') > 0) {
return undefined;
}
// Basic normalization without resolving symlinks or accessing the file system
let normalized = p;
// Collapse multiple slashes
normalized = normalized.replace(/\/+/g, '/');
// Remove single dot segments
normalized = normalized.replace(/\/\.\//g, '/');
// Remove trailing slashes (unless it's exactly '/')
if (normalized.length > 1 && normalized.endsWith('/')) {
normalized = normalized.slice(0, -1);
}
return normalized;
}
/**
* Common POSIX-style sandbox denial detection.
@@ -13,10 +63,18 @@ import type { ShellExecutionResult } from '../../services/shellExecutionService.
*/
export function parsePosixSandboxDenials(
result: ShellExecutionResult,
cache?: SandboxDenialCache,
): ParsedSandboxDenial | undefined {
const output = result.output || '';
const errorOutput = result.error?.message;
const combined = (output + ' ' + (errorOutput || '')).toLowerCase();
const fullText = output + '\n' + (errorOutput || '');
const combined = fullText.toLowerCase();
// Cache by the first 200 characters of the error to handle variable data (timestamps, PIDs)
const cacheKey = combined.trim().slice(0, 200);
if (cacheKey && cache?.has(cacheKey)) {
return undefined;
}
const isFileDenial = [
'operation not permitted',
@@ -27,6 +85,12 @@ export function parsePosixSandboxDenials(
'should be read/write',
'sandbox_apply',
'sandbox: ',
'access denied',
'read-only file system',
'permissionerror',
'fs.permissiondenied',
'forbidden',
'system.unauthorizedaccessexception',
].some((keyword) => combined.includes(keyword));
const isNetworkDenial = [
@@ -46,6 +110,8 @@ export function parsePosixSandboxDenials(
'err_pnpm_fetch',
'err_pnpm_no_matching_version',
"syscall: 'listen'",
'socketexception',
'networkaccessdenied',
].some((keyword) => combined.includes(keyword));
if (!isFileDenial && !isNetworkDenial) {
@@ -57,27 +123,28 @@ export function parsePosixSandboxDenials(
// Extract denied paths (POSIX absolute paths or home-relative paths starting with ~)
const regexes = [
// format: /path: operation not permitted
/(?:^|\s)['"]?((?:\/|~)[\w.\-/:~]+)['"]?:\s*[Oo]peration not permitted/gi,
/(?:^|\s)['"]?((?:\/|~)(?:[\w.\-/:~]*[\w.\-/~])?)['"]?[\s:,'"[\]]*operation not permitted/gi,
// format: operation not permitted, open '/path'
/[Oo]peration not permitted,\s*open\s*['"]?((?:\/|~)[\w.\-/:~]+)['"]?/gi,
/operation not permitted[\s:,'"[\]]*open[\s:,'"[\]]*['"]?((?:\/|~)(?:[\w.\-/:~]*[\w.\-/~])?)['"]?/gi,
// format: permission denied, open '/path'
/[Pp]ermission denied,\s*open\s*['"]?((?:\/|~)[\w.\-/:~]+)['"]?/gi,
/permission denied[\s:,'"[\]]*open[\s:,'"[\]]*['"]?((?:\/|~)(?:[\w.\-/:~]*[\w.\-/~])?)['"]?/gi,
// format: npm error path /path or npm ERR! path /path
/npm\s+(?:error|ERR!)\s+path\s+((?:\/|~)[\w.\-/:~]+)/gi,
// format: EACCES: permission denied, mkdir '/path'
/EACCES:\s*permission denied,\s*\w+\s*['"]?((?:\/|~)[\w.\-/:~]+)['"]?/gi,
/npm[\s!]*[A-Za-z]*err[A-Za-z!]*[\s!]+path[\s!]*((?:\/|~)(?:[\w.\-/:~]*[\w.\-/~])?)/gi,
// format: eacces: permission denied, mkdir '/path'
/eacces[\s:,'"[\]]*permission denied[\s:,'"[\]]*\w+[\s:,'"[\]]*['"]?((?:\/|~)[\w.\-/:~]*[\w.\-/~])?/gi,
// format: PermissionError: [Errno 13] Permission denied: '/path'
/permissionerror[\s:,'"[\]]*(?:[^'"]*)['"]((?:\/|~)[\w.\-/:~]*[\w.\-/~])?['"]/gi,
// format: FileNotFoundError: [Errno 2] No such file or directory: '/path' (sometimes returned in sandbox denials if directory is hidden)
/filenotfounderror[\s:,'"[\]]*(?:[^'"]*)['"]((?:\/|~)[\w.\-/:~]*[\w.\-/~])?['"]/gi,
// format: Error: EACCES: permission denied, open '/path'
/error[\s:,'"[\]]*eacces[\s:,'"[\]]*permission denied[\s:,'"[\]]*(?:[^'"]*)['"]((?:\/|~)[\w.\-/:~]*[\w.\-/~])?['"]/gi,
];
for (const regex of regexes) {
let match;
while ((match = regex.exec(output)) !== null) {
filePaths.add(match[1]);
}
if (errorOutput) {
regex.lastIndex = 0; // Reset for next use
while ((match = regex.exec(errorOutput)) !== null) {
filePaths.add(match[1]);
}
while ((match = regex.exec(fullText)) !== null) {
const sanitized = sanitizeExtractedPath(match[1]);
if (sanitized) filePaths.add(sanitized);
}
}
@@ -86,22 +153,16 @@ export function parsePosixSandboxDenials(
const fallbackRegex =
/(?:^|[\s"'[\]])(\/[a-zA-Z0-9_.-]+(?:\/[a-zA-Z0-9_.-]+)+)(?:$|[\s"'[\]:])/gi;
let m;
while ((m = fallbackRegex.exec(output)) !== null) {
const p = m[1];
if (p && !p.startsWith('/bin/') && !p.startsWith('/usr/bin/')) {
filePaths.add(p);
}
}
if (errorOutput) {
while ((m = fallbackRegex.exec(errorOutput)) !== null) {
const p = m[1];
if (p && !p.startsWith('/bin/') && !p.startsWith('/usr/bin/')) {
filePaths.add(p);
}
}
while ((m = fallbackRegex.exec(fullText)) !== null) {
const sanitized = sanitizeExtractedPath(m[1]);
if (sanitized) filePaths.add(sanitized);
}
}
if (cacheKey && cache) {
cache.set(cacheKey, true);
}
return {
network: isNetworkDenial || undefined,
filePaths: filePaths.size > 0 ? Array.from(filePaths) : undefined,

View File

@@ -8,6 +8,7 @@ import {
type SandboxPermissions,
type SandboxRequest,
} from '../../services/sandboxManager.js';
import { isValidPathString } from '../../utils/paths.js';
/**
* Validates if the requested paths are within the allowed workspace or allowed paths.
@@ -18,6 +19,9 @@ function validatePaths(
allowedPaths: string[],
): boolean {
for (const p of paths) {
if (!isValidPathString(p)) {
return false; // Reject malicious paths
}
const resolvedPath = path.resolve(p);
const resolvedWorkspace = path.resolve(workspace);
const isInsideWorkspace =

View File

@@ -35,7 +35,15 @@ import {
} from './commandSafety.js';
import { verifySandboxOverrides } from '../utils/commandUtils.js';
import { parseWindowsSandboxDenials } from './windowsSandboxDenialUtils.js';
import { isSubpath, resolveToRealPath } from '../../utils/paths.js';
import {
isSubpath,
resolveToRealPath,
assertValidPathString,
} from '../../utils/paths.js';
import {
type SandboxDenialCache,
createSandboxDenialCache,
} from '../utils/sandboxDenialUtils.js';
const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
@@ -54,6 +62,7 @@ export class WindowsSandboxManager implements SandboxManager {
private initialized = false;
private readonly allowedCache = new Set<string>();
private readonly deniedCache = new Set<string>();
private readonly denialCache: SandboxDenialCache = createSandboxDenialCache();
constructor(private readonly options: GlobalSandboxOptions) {
this.helperPath = path.resolve(__dirname, WindowsSandboxManager.HELPER_EXE);
@@ -73,7 +82,7 @@ export class WindowsSandboxManager implements SandboxManager {
}
parseDenials(result: ShellExecutionResult): ParsedSandboxDenial | undefined {
return parseWindowsSandboxDenials(result);
return parseWindowsSandboxDenials(result, this.denialCache);
}
getWorkspace(): string {
@@ -88,6 +97,7 @@ export class WindowsSandboxManager implements SandboxManager {
* Ensures a file or directory exists.
*/
private touch(filePath: string, isDirectory: boolean): void {
assertValidPathString(filePath);
try {
// If it exists (even as a broken symlink), do nothing
if (fs.lstatSync(filePath)) return;

View File

@@ -6,6 +6,10 @@
import { type ParsedSandboxDenial } from '../../services/sandboxManager.js';
import type { ShellExecutionResult } from '../../services/shellExecutionService.js';
import {
type SandboxDenialCache,
sanitizeExtractedPath,
} from '../utils/sandboxDenialUtils.js';
/**
* Windows-specific sandbox denial detection.
@@ -13,10 +17,18 @@ import type { ShellExecutionResult } from '../../services/shellExecutionService.
*/
export function parseWindowsSandboxDenials(
result: ShellExecutionResult,
cache?: SandboxDenialCache,
): ParsedSandboxDenial | undefined {
const output = result.output || '';
const errorOutput = result.error?.message;
const combined = (output + ' ' + (errorOutput || '')).toLowerCase();
const fullText = output + '\n' + (errorOutput || '');
const combined = fullText.toLowerCase();
// Cache by the first 200 characters of the error to handle variable data (timestamps, PIDs)
const cacheKey = combined.trim().slice(0, 200);
if (cacheKey && cache?.has(cacheKey)) {
return undefined;
}
const isFileDenial = [
'access is denied',
@@ -46,30 +58,24 @@ export function parseWindowsSandboxDenials(
// 1. Quoted paths: 'C:\Foo Bar' or "C:\Foo Bar"
const quotedRegex = /['"]((?:\\\\(?:\?|\.)\\)?[a-zA-Z]:[\\/][^'"]+)['"]/g;
for (const match of output.matchAll(quotedRegex)) {
filePaths.add(match[1]);
}
if (errorOutput) {
for (const match of errorOutput.matchAll(quotedRegex)) {
filePaths.add(match[1]);
}
for (const match of fullText.matchAll(quotedRegex)) {
const sanitized = sanitizeExtractedPath(match[1]);
if (sanitized) filePaths.add(sanitized);
}
// 2. Unquoted paths or paths in PowerShell error format: PermissionDenied: (C:\path:String)
const generalRegex =
/(?:^|[\s(])((?:\\\\(?:\?|\.)\\)?[a-zA-Z]:[\\/][^"'\s()<>|?*]+)/g;
for (const match of output.matchAll(generalRegex)) {
for (const match of fullText.matchAll(generalRegex)) {
// Clean up trailing colon which might be part of the error message rather than the path
let p = match[1];
if (p.endsWith(':')) p = p.slice(0, -1);
filePaths.add(p);
const sanitized = sanitizeExtractedPath(p);
if (sanitized) filePaths.add(sanitized);
}
if (errorOutput) {
for (const match of errorOutput.matchAll(generalRegex)) {
let p = match[1];
if (p.endsWith(':')) p = p.slice(0, -1);
filePaths.add(p);
}
if (cacheKey && cache) {
cache.set(cacheKey, true);
}
return {

View File

@@ -369,6 +369,22 @@ export function isSubpath(parentPath: string, childPath: string): boolean {
);
}
/**
* Type guard to verify a value is a string and does not contain null bytes.
*/
export function isValidPathString(p: unknown): p is string {
return typeof p === 'string' && !p.includes('\0');
}
/**
* Asserts that a value is a valid path string, throwing an Error otherwise.
*/
export function assertValidPathString(p: unknown): asserts p is string {
if (!isValidPathString(p)) {
throw new Error(`Invalid path: ${String(p)}`);
}
}
/**
* Resolves a path to its real path, sanitizing it first.
* - Removes 'file://' protocol if present.
@@ -379,6 +395,7 @@ export function isSubpath(parentPath: string, childPath: string): boolean {
* @returns The resolved real path.
*/
export function resolveToRealPath(pathStr: string): string {
assertValidPathString(pathStr);
let resolvedPath = pathStr;
try {