mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-05-15 06:12:50 -07:00
fix(sandbox): proactively register plans directory for write validation
- Enhance WorkspaceContext with addWritablePath and isPathWritable to support proactive registration of paths that do not yet exist. - Update Config and enter_plan_mode to proactively register the plans directory in WorkspaceContext. - Refactor SandboxedFileSystemService to use isPathWritable for granular write validation. - Add comprehensive unit tests for the new WorkspaceContext methods.
This commit is contained in:
@@ -1398,14 +1398,13 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
// Add plans directory to workspace context for plan file storage
|
||||
if (this.planEnabled) {
|
||||
const plansDir = this.storage.getPlansDir();
|
||||
this.workspaceContext.addWritablePath(plansDir);
|
||||
try {
|
||||
await fs.promises.access(plansDir);
|
||||
this.workspaceContext.addDirectory(plansDir);
|
||||
} catch {
|
||||
// Directory does not exist yet, so we don't add it to the workspace context.
|
||||
// It will be created when the first plan is written. Since custom plan
|
||||
// directories must be within the project root, they are automatically
|
||||
// covered by the project-wide file discovery once created.
|
||||
// Directory does not exist yet, so we only add it as a writable path
|
||||
// for validation, but not as a workspace directory for discovery.
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -40,7 +40,7 @@ export class SandboxedFileSystemService implements FileSystemService {
|
||||
const isAllowed =
|
||||
checkType === 'read'
|
||||
? this.workspaceContext.isPathReadable(resolvedPath)
|
||||
: this.workspaceContext.isPathWithinWorkspace(resolvedPath);
|
||||
: this.workspaceContext.isPathWritable(resolvedPath);
|
||||
|
||||
if (!isAllowed) {
|
||||
throw new Error(
|
||||
|
||||
@@ -128,6 +128,7 @@ export class EnterPlanModeInvocation extends BaseToolInvocation<
|
||||
// In sandboxed environments, the plans directory must exist on the host
|
||||
// before it can be bound/allowed in the sandbox.
|
||||
const plansDir = this.config.storage.getPlansDir();
|
||||
this.config.getWorkspaceContext().addWritablePath(plansDir);
|
||||
if (!fs.existsSync(plansDir)) {
|
||||
try {
|
||||
fs.mkdirSync(plansDir, { recursive: true });
|
||||
|
||||
@@ -162,6 +162,47 @@ describe('WorkspaceContext with real filesystem', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('should support granular writable paths that do not exist yet', () => {
|
||||
const workspaceContext = new WorkspaceContext(cwd);
|
||||
const plansDir = path.join(tempDir, 'plans');
|
||||
const planFile = path.join(plansDir, 'my-plan.md');
|
||||
|
||||
// Initially rejected
|
||||
expect(workspaceContext.isPathWritable(planFile)).toBe(false);
|
||||
|
||||
// Register as writable
|
||||
workspaceContext.addWritablePath(plansDir);
|
||||
|
||||
// Now accepted even though plansDir does not exist on disk yet
|
||||
expect(workspaceContext.isPathWritable(planFile)).toBe(true);
|
||||
expect(workspaceContext.isPathWritable(plansDir)).toBe(true);
|
||||
|
||||
// Other paths still rejected
|
||||
const otherFile = path.join(tempDir, 'other', 'file.txt');
|
||||
expect(workspaceContext.isPathWritable(otherFile)).toBe(false);
|
||||
});
|
||||
|
||||
it('should support workspace paths in isPathWritable', () => {
|
||||
const workspaceContext = new WorkspaceContext(cwd);
|
||||
const projectFile = path.join(cwd, 'src', 'index.ts');
|
||||
|
||||
expect(workspaceContext.isPathWritable(projectFile)).toBe(true);
|
||||
});
|
||||
|
||||
it('should support read-only paths in isPathReadable', () => {
|
||||
const workspaceContext = new WorkspaceContext(cwd);
|
||||
const readOnlyDir = path.join(tempDir, 'readonly');
|
||||
const readOnlyFile = path.join(readOnlyDir, 'data.json');
|
||||
|
||||
fs.mkdirSync(readOnlyDir);
|
||||
fs.writeFileSync(readOnlyFile, '{}');
|
||||
|
||||
workspaceContext.addReadOnlyPath(readOnlyDir);
|
||||
|
||||
expect(workspaceContext.isPathReadable(readOnlyFile)).toBe(true);
|
||||
expect(workspaceContext.isPathWritable(readOnlyFile)).toBe(false);
|
||||
});
|
||||
|
||||
describe.skipIf(os.platform() === 'win32')('with symbolic link', () => {
|
||||
describe('in the workspace', () => {
|
||||
let realDir: string;
|
||||
|
||||
@@ -25,6 +25,7 @@ export class WorkspaceContext {
|
||||
private directories = new Set<string>();
|
||||
private initialDirectories: Set<string>;
|
||||
private readOnlyPaths = new Set<string>();
|
||||
private writablePaths = new Set<string>();
|
||||
private onDirectoriesChangedListeners = new Set<() => void>();
|
||||
|
||||
/**
|
||||
@@ -132,6 +133,18 @@ export class WorkspaceContext {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Adds a path that is allowed to be written to, even if it's not within the workspace.
|
||||
* This is useful for temporary directories or specific files that need to be
|
||||
* writable even in restricted modes. Unlike addDirectory, this does not
|
||||
* require the path to exist.
|
||||
*/
|
||||
addWritablePath(pathToAdd: string): void {
|
||||
// Resolve to absolute path, but don't use realpath as it might not exist
|
||||
const resolved = path.resolve(this.targetDir, pathToAdd);
|
||||
this.writablePaths.add(resolved);
|
||||
}
|
||||
|
||||
private resolveAndValidateDir(directory: string): string {
|
||||
const absolutePath = path.resolve(this.targetDir, directory);
|
||||
|
||||
@@ -221,6 +234,34 @@ export class WorkspaceContext {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Checks if a path is allowed to be written to.
|
||||
* This includes workspace paths and explicitly added writable paths.
|
||||
* @param pathToCheck The path to validate
|
||||
* @returns True if the path is writable, false otherwise
|
||||
*/
|
||||
isPathWritable(pathToCheck: string): boolean {
|
||||
if (this.isPathWithinWorkspace(pathToCheck)) {
|
||||
return true;
|
||||
}
|
||||
try {
|
||||
const fullyResolvedPath = this.fullyResolvedPath(pathToCheck);
|
||||
|
||||
for (const allowedPath of this.writablePaths) {
|
||||
// Allow exact matches or subpaths (if allowedPath is a directory)
|
||||
if (
|
||||
fullyResolvedPath === allowedPath ||
|
||||
this.isPathWithinRoot(fullyResolvedPath, allowedPath)
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Fully resolves a path, including symbolic links.
|
||||
* If the path does not exist, it returns the fully resolved path as it would be
|
||||
|
||||
Reference in New Issue
Block a user