fix(core): resolve Plan Mode deadlock during plan file creation due to sandbox restrictions (#24047)

This commit is contained in:
David Pierce
2026-03-31 22:06:50 +00:00
committed by GitHub
parent 9364dd8a49
commit 94f9480a3a
13 changed files with 555 additions and 97 deletions

View File

@@ -68,6 +68,8 @@ export interface GlobalSandboxOptions {
* This directory is granted full read and write access.
*/
workspace: string;
/** Absolute paths to explicitly include in the workspace context. */
includeDirectories?: string[];
/** Absolute paths to explicitly deny read/write access to (overrides allowlists). */
forbiddenPaths?: string[];
/** The current sandbox mode behavior from config. */

View File

@@ -28,13 +28,13 @@ vi.mock('node:child_process', () => ({
}));
class MockSandboxManager implements SandboxManager {
async prepareCommand(req: SandboxRequest): Promise<SandboxedCommand> {
return {
prepareCommand = vi.fn(
async (req: SandboxRequest): Promise<SandboxedCommand> => ({
program: 'sandbox.exe',
args: ['0', req.cwd, req.command, ...req.args],
env: req.env || {},
};
}
}),
);
isKnownSafeCommand(): boolean {
return false;
@@ -73,7 +73,7 @@ describe('SandboxedFileSystemService', () => {
vi.mocked(spawn).mockReturnValue(mockChild);
const readPromise = service.readTextFile('/test/file.txt');
const readPromise = service.readTextFile('/test/cwd/file.txt');
// Use setImmediate to ensure events are emitted after the promise starts executing
setImmediate(() => {
@@ -83,9 +83,18 @@ describe('SandboxedFileSystemService', () => {
const content = await readPromise;
expect(content).toBe('file content');
expect(vi.mocked(sandboxManager.prepareCommand)).toHaveBeenCalledWith(
expect.objectContaining({
command: '__read',
args: ['/test/cwd/file.txt'],
policy: {
allowedPaths: ['/test/cwd/file.txt'],
},
}),
);
expect(spawn).toHaveBeenCalledWith(
'sandbox.exe',
['0', cwd, '__read', '/test/file.txt'],
['0', cwd, '__read', '/test/cwd/file.txt'],
expect.any(Object),
);
});
@@ -104,7 +113,10 @@ describe('SandboxedFileSystemService', () => {
vi.mocked(spawn).mockReturnValue(mockChild);
const writePromise = service.writeTextFile('/test/file.txt', 'new content');
const writePromise = service.writeTextFile(
'/test/cwd/file.txt',
'new content',
);
setImmediate(() => {
mockChild.emit('close', 0);
@@ -115,9 +127,23 @@ describe('SandboxedFileSystemService', () => {
(mockStdin as unknown as { write: Mock }).write,
).toHaveBeenCalledWith('new content');
expect((mockStdin as unknown as { end: Mock }).end).toHaveBeenCalled();
expect(vi.mocked(sandboxManager.prepareCommand)).toHaveBeenCalledWith(
expect.objectContaining({
command: '__write',
args: ['/test/cwd/file.txt'],
policy: {
allowedPaths: ['/test/cwd/file.txt'],
additionalPermissions: {
fileSystem: {
write: ['/test/cwd/file.txt'],
},
},
},
}),
);
expect(spawn).toHaveBeenCalledWith(
'sandbox.exe',
['0', cwd, '__write', '/test/file.txt'],
['0', cwd, '__write', '/test/cwd/file.txt'],
expect.any(Object),
);
});
@@ -131,7 +157,7 @@ describe('SandboxedFileSystemService', () => {
vi.mocked(spawn).mockReturnValue(mockChild);
const readPromise = service.readTextFile('/test/file.txt');
const readPromise = service.readTextFile('/test/cwd/file.txt');
setImmediate(() => {
mockChild.stderr!.emit('data', Buffer.from('access denied'));
@@ -139,7 +165,63 @@ describe('SandboxedFileSystemService', () => {
});
await expect(readPromise).rejects.toThrow(
"Sandbox Error: read_file failed for '/test/file.txt'. Exit code 1. Details: access denied",
"Sandbox Error: read_file failed for '/test/cwd/file.txt'. Exit code 1. Details: access denied",
);
});
it('should set ENOENT code when file does not exist', async () => {
const mockChild = new EventEmitter() as unknown as ChildProcess;
Object.assign(mockChild, {
stdout: new EventEmitter(),
stderr: new EventEmitter(),
});
vi.mocked(spawn).mockReturnValue(mockChild);
const readPromise = service.readTextFile('/test/cwd/missing.txt');
setImmediate(() => {
mockChild.stderr!.emit('data', Buffer.from('No such file or directory'));
mockChild.emit('close', 1);
});
try {
await readPromise;
expect.fail('Should have rejected');
} catch (err: unknown) {
// @ts-expect-error - Checking message and code on unknown error
expect(err.message).toContain('No such file or directory');
// @ts-expect-error - Checking message and code on unknown error
expect(err.code).toBe('ENOENT');
}
});
it('should set ENOENT code when file does not exist on Windows', async () => {
const mockChild = new EventEmitter() as unknown as ChildProcess;
Object.assign(mockChild, {
stdout: new EventEmitter(),
stderr: new EventEmitter(),
});
vi.mocked(spawn).mockReturnValue(mockChild);
const readPromise = service.readTextFile('/test/cwd/missing.txt');
setImmediate(() => {
mockChild.stderr!.emit(
'data',
Buffer.from('Could not find a part of the path'),
);
mockChild.emit('close', 1);
});
try {
await readPromise;
expect.fail('Should have rejected');
} catch (err: unknown) {
const error = err as { message: string; code?: string };
expect(error.message).toContain('Could not find a part of the path');
expect(error.code).toBe('ENOENT');
}
});
});

View File

@@ -9,6 +9,7 @@ import { type FileSystemService } from './fileSystemService.js';
import { type SandboxManager } from './sandboxManager.js';
import { debugLogger } from '../utils/debugLogger.js';
import { isNodeError } from '../utils/errors.js';
import { resolveToRealPath, isSubpath } from '../utils/paths.js';
/**
* A FileSystemService implementation that performs operations through a sandbox.
@@ -19,12 +20,26 @@ export class SandboxedFileSystemService implements FileSystemService {
private cwd: string,
) {}
private sanitizeAndValidatePath(filePath: string): string {
const resolvedPath = resolveToRealPath(filePath);
if (!isSubpath(this.cwd, resolvedPath) && this.cwd !== resolvedPath) {
throw new Error(
`Access denied: Path '${filePath}' is outside the workspace.`,
);
}
return resolvedPath;
}
async readTextFile(filePath: string): Promise<string> {
const safePath = this.sanitizeAndValidatePath(filePath);
const prepared = await this.sandboxManager.prepareCommand({
command: '__read',
args: [filePath],
args: [safePath],
cwd: this.cwd,
env: process.env,
policy: {
allowedPaths: [safePath],
},
});
return new Promise((resolve, reject) => {
@@ -50,11 +65,18 @@ export class SandboxedFileSystemService implements FileSystemService {
if (code === 0) {
resolve(output);
} else {
reject(
new Error(
`Sandbox Error: read_file failed for '${filePath}'. Exit code ${code}. ${error ? 'Details: ' + error : ''}`,
),
const isEnoent =
error.toLowerCase().includes('no such file or directory') ||
error.toLowerCase().includes('enoent') ||
error.toLowerCase().includes('could not find file') ||
error.toLowerCase().includes('could not find a part of the path');
const err = new Error(
`Sandbox Error: read_file failed for '${filePath}'. Exit code ${code}. ${error ? 'Details: ' + error : ''}`,
);
if (isEnoent) {
Object.assign(err, { code: 'ENOENT' });
}
reject(err);
}
});
@@ -69,11 +91,20 @@ export class SandboxedFileSystemService implements FileSystemService {
}
async writeTextFile(filePath: string, content: string): Promise<void> {
const safePath = this.sanitizeAndValidatePath(filePath);
const prepared = await this.sandboxManager.prepareCommand({
command: '__write',
args: [filePath],
args: [safePath],
cwd: this.cwd,
env: process.env,
policy: {
allowedPaths: [safePath],
additionalPermissions: {
fileSystem: {
write: [safePath],
},
},
},
});
return new Promise((resolve, reject) => {