address pr feedback

This commit is contained in:
galz10
2026-03-11 10:03:39 -07:00
parent abf29df57c
commit 64648ee721
8 changed files with 519 additions and 143 deletions
+90 -1
View File
@@ -295,13 +295,102 @@ describe('loadSandboxConfig', () => {
it.each([false, 'false', '0', undefined, null, ''])(
'should disable sandbox for value: %s',
async (value) => {
// \`null\` is not a valid type for the arg, but good to test falsiness
// `null` is not a valid type for the arg, but good to test falsiness
const config = await loadSandboxConfig({}, { sandbox: value });
expect(config).toBeUndefined();
},
);
});
describe('with SandboxConfig object in settings', () => {
beforeEach(() => {
mockedOsPlatform.mockReturnValue('linux');
mockedCommandExistsSync.mockImplementation((cmd) => cmd === 'docker');
});
it('should support object structure with enabled: true', async () => {
const config = await loadSandboxConfig(
{
tools: {
sandbox: {
enabled: true,
allowedPaths: ['/tmp'],
networkAccess: true,
},
},
},
{},
);
expect(config).toEqual({
enabled: true,
allowedPaths: ['/tmp'],
networkAccess: true,
command: 'docker',
image: 'default/image',
});
});
it('should support object structure with explicit command', async () => {
mockedCommandExistsSync.mockImplementation((cmd) => cmd === 'podman');
const config = await loadSandboxConfig(
{
tools: {
sandbox: {
enabled: true,
command: 'podman',
},
},
},
{},
);
expect(config?.command).toBe('podman');
});
it('should support object structure with custom image', async () => {
const config = await loadSandboxConfig(
{
tools: {
sandbox: {
enabled: true,
image: 'custom/image',
},
},
},
{},
);
expect(config?.image).toBe('custom/image');
});
it('should return undefined if enabled is false in object', async () => {
const config = await loadSandboxConfig(
{
tools: {
sandbox: {
enabled: false,
},
},
},
{},
);
expect(config).toBeUndefined();
});
it('should prioritize CLI flag over settings object', async () => {
const config = await loadSandboxConfig(
{
tools: {
sandbox: {
enabled: true,
allowedPaths: ['/settings-path'],
},
},
},
{ sandbox: false },
);
expect(config).toBeUndefined();
});
});
describe('with sandbox: runsc (gVisor)', () => {
beforeEach(() => {
mockedOsPlatform.mockReturnValue('linux');
+23 -2
View File
@@ -118,15 +118,36 @@ export async function loadSandboxConfig(
argv: SandboxCliArgs,
): Promise<SandboxConfig | undefined> {
const sandboxOption = argv.sandbox ?? settings.tools?.sandbox;
const command = getSandboxCommand(sandboxOption);
let sandboxValue: boolean | string | null | undefined;
let allowedPaths: string[] = [];
let networkAccess = false;
let customImage: string | undefined;
if (
typeof sandboxOption === 'object' &&
sandboxOption !== null &&
!Array.isArray(sandboxOption)
) {
const config = sandboxOption;
sandboxValue = config.enabled ? (config.command ?? true) : false;
allowedPaths = config.allowedPaths ?? [];
networkAccess = config.networkAccess ?? false;
customImage = config.image;
} else if (typeof sandboxOption !== 'object' || sandboxOption === null) {
sandboxValue = sandboxOption;
}
const command = getSandboxCommand(sandboxValue);
const packageJson = await getPackageJson(__dirname);
const image =
process.env['GEMINI_SANDBOX_IMAGE'] ??
process.env['GEMINI_SANDBOX_IMAGE_DEFAULT'] ??
customImage ??
packageJson?.config?.sandboxImageUri;
return command && image
? { enabled: true, allowedPaths: [], networkAccess: false, command, image }
? { enabled: true, allowedPaths, networkAccess, command, image }
: undefined;
}
+41 -5
View File
@@ -18,6 +18,7 @@ import {
type AuthType,
type AgentOverride,
type CustomTheme,
type SandboxConfig,
} from '@google/gemini-cli-core';
import type { SessionRetentionSettings } from './settings.js';
import { DEFAULT_MIN_RETENTION } from '../utils/sessionCleanup.js';
@@ -1252,8 +1253,8 @@ const SETTINGS_SCHEMA = {
label: 'Sandbox',
category: 'Tools',
requiresRestart: true,
default: undefined as boolean | string | undefined,
ref: 'BooleanOrString',
default: undefined as boolean | string | SandboxConfig | undefined,
ref: 'BooleanOrStringOrObject',
description: oneLine`
Sandbox execution environment.
Set to a boolean to enable or disable the sandbox, provide a string path to a sandbox profile,
@@ -2585,9 +2586,44 @@ export const SETTINGS_SCHEMA_DEFINITIONS: Record<
description: 'Accepts either a single string or an array of strings.',
anyOf: [{ type: 'string' }, { type: 'array', items: { type: 'string' } }],
},
BooleanOrString: {
description: 'Accepts either a boolean flag or a string command name.',
anyOf: [{ type: 'boolean' }, { type: 'string' }],
BooleanOrStringOrObject: {
description:
'Accepts either a boolean flag, a string command name, or a configuration object.',
anyOf: [
{ type: 'boolean' },
{ type: 'string' },
{
type: 'object',
description: 'Sandbox configuration object.',
additionalProperties: false,
properties: {
enabled: {
type: 'boolean',
description: 'Enables or disables the sandbox.',
},
command: {
type: 'string',
description:
'The sandbox command to use (docker, podman, sandbox-exec, runsc, lxc).',
enum: ['docker', 'podman', 'sandbox-exec', 'runsc', 'lxc'],
},
image: {
type: 'string',
description: 'The sandbox image to use.',
},
allowedPaths: {
type: 'array',
description:
'A list of absolute host paths that should be accessible within the sandbox.',
items: { type: 'string' },
},
networkAccess: {
type: 'boolean',
description: 'Whether the sandbox should have internet access.',
},
},
},
],
},
HookDefinitionArray: {
type: 'array',
+119
View File
@@ -395,6 +395,125 @@ describe('sandbox', () => {
);
});
it('should handle allowedPaths in Docker', async () => {
const config: SandboxConfig = createMockSandboxConfig({
command: 'docker',
image: 'gemini-cli-sandbox',
allowedPaths: ['/extra/path'],
});
vi.mocked(fs.existsSync).mockReturnValue(true);
// Mock image check to return true
interface MockProcessWithStdout extends EventEmitter {
stdout: EventEmitter;
}
const mockImageCheckProcess = new EventEmitter() as MockProcessWithStdout;
mockImageCheckProcess.stdout = new EventEmitter();
vi.mocked(spawn).mockImplementationOnce(() => {
setTimeout(() => {
mockImageCheckProcess.stdout.emit('data', Buffer.from('image-id'));
mockImageCheckProcess.emit('close', 0);
}, 1);
return mockImageCheckProcess as unknown as ReturnType<typeof spawn>;
});
const mockSpawnProcess = new EventEmitter() as unknown as ReturnType<
typeof spawn
>;
mockSpawnProcess.on = vi.fn().mockImplementation((event, cb) => {
if (event === 'close') {
setTimeout(() => cb(0), 10);
}
return mockSpawnProcess;
});
vi.mocked(spawn).mockImplementationOnce(() => mockSpawnProcess);
await start_sandbox(config);
expect(spawn).toHaveBeenCalledWith(
'docker',
expect.arrayContaining(['--volume', '/extra/path:/extra/path:ro']),
expect.any(Object),
);
});
it('should handle networkAccess: false in Docker', async () => {
const config: SandboxConfig = createMockSandboxConfig({
command: 'docker',
image: 'gemini-cli-sandbox',
networkAccess: false,
});
// Mock image check
interface MockProcessWithStdout extends EventEmitter {
stdout: EventEmitter;
}
const mockImageCheckProcess = new EventEmitter() as MockProcessWithStdout;
mockImageCheckProcess.stdout = new EventEmitter();
vi.mocked(spawn).mockImplementationOnce(() => {
setTimeout(() => {
mockImageCheckProcess.stdout.emit('data', Buffer.from('image-id'));
mockImageCheckProcess.emit('close', 0);
}, 1);
return mockImageCheckProcess as unknown as ReturnType<typeof spawn>;
});
const mockSpawnProcess = new EventEmitter() as unknown as ReturnType<
typeof spawn
>;
mockSpawnProcess.on = vi.fn().mockImplementation((event, cb) => {
if (event === 'close') {
setTimeout(() => cb(0), 10);
}
return mockSpawnProcess;
});
vi.mocked(spawn).mockImplementationOnce(() => mockSpawnProcess);
await start_sandbox(config);
expect(execSync).toHaveBeenCalledWith(
expect.stringContaining('network create --internal gemini-cli-sandbox'),
expect.any(Object),
);
expect(spawn).toHaveBeenCalledWith(
'docker',
expect.arrayContaining(['--network', 'gemini-cli-sandbox']),
expect.any(Object),
);
});
it('should handle allowedPaths in macOS seatbelt', async () => {
vi.mocked(os.platform).mockReturnValue('darwin');
const config: SandboxConfig = createMockSandboxConfig({
command: 'sandbox-exec',
image: 'some-image',
allowedPaths: ['/Users/user/extra'],
});
vi.mocked(fs.existsSync).mockReturnValue(true);
interface MockProcess extends EventEmitter {
stdout: EventEmitter;
stderr: EventEmitter;
}
const mockSpawnProcess = new EventEmitter() as MockProcess;
mockSpawnProcess.stdout = new EventEmitter();
mockSpawnProcess.stderr = new EventEmitter();
vi.mocked(spawn).mockReturnValue(
mockSpawnProcess as unknown as ReturnType<typeof spawn>,
);
const promise = start_sandbox(config);
setTimeout(() => mockSpawnProcess.emit('close', 0), 10);
await promise;
// Check that the extra path is passed as an INCLUDE_DIR_X argument
expect(spawn).toHaveBeenCalledWith(
'sandbox-exec',
expect.arrayContaining(['INCLUDE_DIR_0=/Users/user/extra']),
expect.any(Object),
);
});
it('should pass through GOOGLE_GEMINI_BASE_URL and GOOGLE_VERTEX_BASE_URL', async () => {
const config: SandboxConfig = createMockSandboxConfig({
command: 'docker',
+209 -129
View File
@@ -114,6 +114,22 @@ export async function start_sandbox(
}
}
// Add custom allowed paths from config
if (config.allowedPaths) {
for (const hostPath of config.allowedPaths) {
if (
hostPath &&
path.isAbsolute(hostPath) &&
fs.existsSync(hostPath)
) {
const realDir = fs.realpathSync(hostPath);
if (!includedDirs.includes(realDir) && realDir !== targetDir) {
includedDirs.push(realDir);
}
}
}
}
for (let i = 0; i < MAX_INCLUDE_DIRS; i++) {
let dirPath = '/dev/null'; // Default to a safe path that won't cause issues
@@ -232,7 +248,8 @@ export async function start_sandbox(
const image = config.image;
if (!image) throw new FatalSandboxError('Sandbox image is required');
if (!/^[a-zA-Z0-9_.:/-]+$/.test(image)) throw new FatalSandboxError('Invalid sandbox image name');
if (!/^[a-zA-Z0-9_.:/-]+$/.test(image))
throw new FatalSandboxError('Invalid sandbox image name');
const workdir = path.resolve(process.cwd());
const containerWorkdir = getContainerPath(workdir);
@@ -395,6 +412,19 @@ export async function start_sandbox(
}
}
// mount paths listed in config.allowedPaths
if (config.allowedPaths) {
for (const hostPath of config.allowedPaths) {
if (hostPath && path.isAbsolute(hostPath) && fs.existsSync(hostPath)) {
const containerPath = getContainerPath(hostPath);
debugLogger.log(
`Config allowedPath: ${hostPath} -> ${containerPath} (ro)`,
);
args.push('--volume', `${hostPath}:${containerPath}:ro`);
}
}
}
// expose env-specified ports on the sandbox
ports().forEach((p) => args.push('--publish', `${p}:${p}`));
@@ -428,21 +458,27 @@ export async function start_sandbox(
args.push('--env', `NO_PROXY=${noProxy}`);
args.push('--env', `no_proxy=${noProxy}`);
}
}
// if using proxy, switch to internal networking through proxy
if (proxy) {
execSync(
`${command} network inspect ${SANDBOX_NETWORK_NAME} || ${command} network create --internal ${SANDBOX_NETWORK_NAME}`,
);
args.push('--network', SANDBOX_NETWORK_NAME);
// handle network access and proxy configuration
if (!config.networkAccess || proxyCommand) {
const isInternal = !config.networkAccess || !!proxyCommand;
const networkFlags = isInternal ? '--internal' : '';
execSync(
`${command} network inspect ${SANDBOX_NETWORK_NAME} || ${command} network create ${networkFlags} ${SANDBOX_NETWORK_NAME}`,
{ stdio: 'ignore' },
);
args.push('--network', SANDBOX_NETWORK_NAME);
if (proxyCommand) {
// if proxy command is set, create a separate network w/ host access (i.e. non-internal)
// we will run proxy in its own container connected to both host network and internal network
// this allows proxy to work even on rootless podman on macos with host<->vm<->container isolation
if (proxyCommand) {
execSync(
`${command} network inspect ${SANDBOX_PROXY_NAME} || ${command} network create ${SANDBOX_PROXY_NAME}`,
);
}
execSync(
`${command} network inspect ${SANDBOX_PROXY_NAME} || ${command} network create ${SANDBOX_PROXY_NAME}`,
{ stdio: 'ignore' },
);
}
}
@@ -836,136 +872,180 @@ async function start_lxc_sandbox(
);
}
// Bind-mount the working directory into the container at the same path.
// Using "lxc config device add" is idempotent when the device name matches.
const deviceName = `gemini-workspace-${randomBytes(4).toString('hex')}`;
const devicesToRemove: string[] = [];
const removeDevices = () => {
for (const deviceName of devicesToRemove) {
try {
execFileSync(
'lxc',
['config', 'device', 'remove', containerName, deviceName],
{ timeout: 2000 },
);
} catch {
// Best-effort cleanup; ignore errors on exit.
}
}
};
try {
await execFileAsync('lxc', [
'config',
'device',
'add',
containerName,
deviceName,
'disk',
`source=${workdir}`,
`path=${workdir}`,
]);
debugLogger.log(
`mounted workspace '${workdir}' into container as device '${deviceName}'`,
);
} catch (err) {
throw new FatalSandboxError(
`Failed to mount workspace into LXC container '${containerName}': ${err instanceof Error ? err.message : String(err)}`,
);
}
// Bind-mount the working directory into the container at the same path.
// Using "lxc config device add" is idempotent when the device name matches.
const workspaceDeviceName = `gemini-workspace-${randomBytes(4).toString(
'hex',
)}`;
devicesToRemove.push(workspaceDeviceName);
// Remove the workspace device from the container when the process exits.
// Only the 'exit' event is needed — the CLI's cleanup.ts already handles
// SIGINT and SIGTERM by calling process.exit(), which fires 'exit'.
const removeDevice = () => {
try {
execFileSync(
'lxc',
['config', 'device', 'remove', containerName, deviceName],
{ timeout: 2000 },
await execFileAsync('lxc', [
'config',
'device',
'add',
containerName,
workspaceDeviceName,
'disk',
`source=${workdir}`,
`path=${workdir}`,
]);
debugLogger.log(
`mounted workspace '${workdir}' into container as device '${workspaceDeviceName}'`,
);
} catch (err) {
throw new FatalSandboxError(
`Failed to mount workspace into LXC container '${containerName}': ${err instanceof Error ? err.message : String(err)}`,
);
} catch {
// Best-effort cleanup; ignore errors on exit.
}
};
process.on('exit', removeDevice);
// Build the environment variable arguments for `lxc exec`.
const envArgs: string[] = [];
const envVarsToForward: Record<string, string | undefined> = {
GEMINI_API_KEY: process.env['GEMINI_API_KEY'],
GOOGLE_API_KEY: process.env['GOOGLE_API_KEY'],
GOOGLE_GEMINI_BASE_URL: process.env['GOOGLE_GEMINI_BASE_URL'],
GOOGLE_VERTEX_BASE_URL: process.env['GOOGLE_VERTEX_BASE_URL'],
GOOGLE_GENAI_USE_VERTEXAI: process.env['GOOGLE_GENAI_USE_VERTEXAI'],
GOOGLE_GENAI_USE_GCA: process.env['GOOGLE_GENAI_USE_GCA'],
GOOGLE_CLOUD_PROJECT: process.env['GOOGLE_CLOUD_PROJECT'],
GOOGLE_CLOUD_LOCATION: process.env['GOOGLE_CLOUD_LOCATION'],
GEMINI_MODEL: process.env['GEMINI_MODEL'],
TERM: process.env['TERM'],
COLORTERM: process.env['COLORTERM'],
GEMINI_CLI_IDE_SERVER_PORT: process.env['GEMINI_CLI_IDE_SERVER_PORT'],
GEMINI_CLI_IDE_WORKSPACE_PATH: process.env['GEMINI_CLI_IDE_WORKSPACE_PATH'],
TERM_PROGRAM: process.env['TERM_PROGRAM'],
};
for (const [key, value] of Object.entries(envVarsToForward)) {
if (value) {
envArgs.push('--env', `${key}=${value}`);
}
}
// Forward SANDBOX_ENV key=value pairs
if (process.env['SANDBOX_ENV']) {
for (let env of process.env['SANDBOX_ENV'].split(',')) {
if ((env = env.trim())) {
if (env.includes('=')) {
envArgs.push('--env', env);
} else {
throw new FatalSandboxError(
'SANDBOX_ENV must be a comma-separated list of key=value pairs',
);
// Add custom allowed paths from config
if (config.allowedPaths) {
for (const hostPath of config.allowedPaths) {
if (hostPath && path.isAbsolute(hostPath) && fs.existsSync(hostPath)) {
const allowedDeviceName = `gemini-allowed-${randomBytes(4).toString(
'hex',
)}`;
devicesToRemove.push(allowedDeviceName);
try {
await execFileAsync('lxc', [
'config',
'device',
'add',
containerName,
allowedDeviceName,
'disk',
`source=${hostPath}`,
`path=${hostPath}`,
'readonly=true',
]);
debugLogger.log(
`mounted allowed path '${hostPath}' into container as device '${allowedDeviceName}' (ro)`,
);
} catch (err) {
debugLogger.warn(
`Failed to mount allowed path '${hostPath}' into LXC container: ${err instanceof Error ? err.message : String(err)}`,
);
}
}
}
}
}
// Forward NODE_OPTIONS (e.g. from --inspect flags)
const existingNodeOptions = process.env['NODE_OPTIONS'] || '';
const allNodeOptions = [
...(existingNodeOptions ? [existingNodeOptions] : []),
...nodeArgs,
].join(' ');
if (allNodeOptions.length > 0) {
envArgs.push('--env', `NODE_OPTIONS=${allNodeOptions}`);
}
// Remove the devices from the container when the process exits.
// Only the 'exit' event is needed — the CLI's cleanup.ts already handles
// SIGINT and SIGTERM by calling process.exit(), which fires 'exit'.
process.on('exit', removeDevices);
// Mark that we're running inside an LXC sandbox.
envArgs.push('--env', `SANDBOX=${containerName}`);
// Build the command entrypoint (same logic as Docker path).
const finalEntrypoint = entrypoint(workdir, cliArgs);
// Build the full lxc exec command args.
const args = [
'exec',
containerName,
'--cwd',
workdir,
...envArgs,
'--',
...finalEntrypoint,
];
debugLogger.log(`lxc exec args: ${args.join(' ')}`);
process.stdin.pause();
const sandboxProcess = spawn('lxc', args, {
stdio: 'inherit',
});
return new Promise<number>((resolve, reject) => {
sandboxProcess.on('error', (err) => {
coreEvents.emitFeedback('error', 'LXC sandbox process error', err);
reject(err);
});
sandboxProcess.on('close', (code, signal) => {
process.stdin.resume();
process.off('exit', removeDevice);
removeDevice();
if (code !== 0 && code !== null) {
debugLogger.log(
`LXC sandbox process exited with code: ${code}, signal: ${signal}`,
);
// Build the environment variable arguments for `lxc exec`.
const envArgs: string[] = [];
const envVarsToForward: Record<string, string | undefined> = {
GEMINI_API_KEY: process.env['GEMINI_API_KEY'],
GOOGLE_API_KEY: process.env['GOOGLE_API_KEY'],
GOOGLE_GEMINI_BASE_URL: process.env['GOOGLE_GEMINI_BASE_URL'],
GOOGLE_VERTEX_BASE_URL: process.env['GOOGLE_VERTEX_BASE_URL'],
GOOGLE_GENAI_USE_VERTEXAI: process.env['GOOGLE_GENAI_USE_VERTEXAI'],
GOOGLE_GENAI_USE_GCA: process.env['GOOGLE_GENAI_USE_GCA'],
GOOGLE_CLOUD_PROJECT: process.env['GOOGLE_CLOUD_PROJECT'],
GOOGLE_CLOUD_LOCATION: process.env['GOOGLE_CLOUD_LOCATION'],
GEMINI_MODEL: process.env['GEMINI_MODEL'],
TERM: process.env['TERM'],
COLORTERM: process.env['COLORTERM'],
GEMINI_CLI_IDE_SERVER_PORT: process.env['GEMINI_CLI_IDE_SERVER_PORT'],
GEMINI_CLI_IDE_WORKSPACE_PATH:
process.env['GEMINI_CLI_IDE_WORKSPACE_PATH'],
TERM_PROGRAM: process.env['TERM_PROGRAM'],
};
for (const [key, value] of Object.entries(envVarsToForward)) {
if (value) {
envArgs.push('--env', `${key}=${value}`);
}
resolve(code ?? 1);
}
// Forward SANDBOX_ENV key=value pairs
if (process.env['SANDBOX_ENV']) {
for (let env of process.env['SANDBOX_ENV'].split(',')) {
if ((env = env.trim())) {
if (env.includes('=')) {
envArgs.push('--env', env);
} else {
throw new FatalSandboxError(
'SANDBOX_ENV must be a comma-separated list of key=value pairs',
);
}
}
}
}
// Forward NODE_OPTIONS (e.g. from --inspect flags)
const existingNodeOptions = process.env['NODE_OPTIONS'] || '';
const allNodeOptions = [
...(existingNodeOptions ? [existingNodeOptions] : []),
...nodeArgs,
].join(' ');
if (allNodeOptions.length > 0) {
envArgs.push('--env', `NODE_OPTIONS=${allNodeOptions}`);
}
// Mark that we're running inside an LXC sandbox.
envArgs.push('--env', `SANDBOX=${containerName}`);
// Build the command entrypoint (same logic as Docker path).
const finalEntrypoint = entrypoint(workdir, cliArgs);
// Build the full lxc exec command args.
const args = [
'exec',
containerName,
'--cwd',
workdir,
...envArgs,
'--',
...finalEntrypoint,
];
debugLogger.log(`lxc exec args: ${args.join(' ')}`);
process.stdin.pause();
const sandboxProcess = spawn('lxc', args, {
stdio: 'inherit',
});
});
return await new Promise<number>((resolve, reject) => {
sandboxProcess.on('error', (err) => {
coreEvents.emitFeedback('error', 'LXC sandbox process error', err);
reject(err);
});
sandboxProcess.on('close', (code, signal) => {
process.stdin.resume();
if (code !== 0 && code !== null) {
debugLogger.log(
`LXC sandbox process exited with code: ${code}, signal: ${signal}`,
);
}
resolve(code ?? 1);
});
});
} finally {
process.off('exit', removeDevices);
removeDevices();
}
}
// Helper functions to ensure sandbox image is present