mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-06-13 21:07:00 -07:00
fix(core,cli): resolve TOCTOU, concurrency, and performance regressions in plan resolution
This commit is contained in:
@@ -1340,11 +1340,6 @@ Logging in with Google... Restarting Gemini CLI to continue.
|
||||
}
|
||||
}
|
||||
|
||||
const parsedCommand = parseSlashCommand(
|
||||
submittedValue,
|
||||
slashCommands ?? [],
|
||||
);
|
||||
|
||||
const isSlash = isSlashCommand(submittedValue.trim());
|
||||
const isIdle = streamingState === StreamingState.Idle;
|
||||
const isAgentRunning =
|
||||
@@ -1352,6 +1347,10 @@ Logging in with Google... Restarting Gemini CLI to continue.
|
||||
isToolExecuting(pendingHistoryItems);
|
||||
|
||||
if (isSlash && isAgentRunning) {
|
||||
const parsedCommand = parseSlashCommand(
|
||||
submittedValue,
|
||||
slashCommands ?? [],
|
||||
);
|
||||
const commandToExecute = parsedCommand.commandToExecute;
|
||||
if (commandToExecute?.isSafeConcurrent) {
|
||||
void handleSlashCommand(submittedValue);
|
||||
|
||||
@@ -1130,18 +1130,19 @@ describe('useSlashCommandProcessor', () => {
|
||||
});
|
||||
expect(spySetContext).toHaveBeenLastCalledWith('extB');
|
||||
|
||||
// 3. Run /plan (Default)
|
||||
// 3. Run /help (Concurrent global command)
|
||||
spySetContext.mockClear();
|
||||
await act(async () => {
|
||||
await hook.current.handleSlashCommand('/help');
|
||||
});
|
||||
// A concurrent command should NOT modify the active extension context
|
||||
expect(spySetContext).not.toHaveBeenCalled();
|
||||
|
||||
// 4. Run /plan (Default)
|
||||
await act(async () => {
|
||||
await hook.current.handleSlashCommand('/plan my task');
|
||||
});
|
||||
expect(spySetContext).toHaveBeenLastCalledWith(undefined);
|
||||
|
||||
// 4. Run /clear (Global)
|
||||
await act(async () => {
|
||||
await hook.current.handleSlashCommand('/help');
|
||||
});
|
||||
// Context should still be undefined
|
||||
expect(spySetContext).toHaveBeenLastCalledWith(undefined);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -371,7 +371,7 @@ export const useSlashCommandProcessor = (
|
||||
extensionContext,
|
||||
} = parseSlashCommand(trimmed, commands);
|
||||
|
||||
if (config) {
|
||||
if (config && commandToExecute && !commandToExecute.isSafeConcurrent) {
|
||||
if (extensionContext) {
|
||||
if (config.hasExtensionPlanDir(extensionContext)) {
|
||||
config.setActiveExtensionContext(extensionContext);
|
||||
|
||||
@@ -3369,8 +3369,13 @@ describe('Plans Directory Initialization', () => {
|
||||
writeSpy.mockRestore();
|
||||
});
|
||||
|
||||
it('should throw a security violation if the resolved plan directory is outside the project root', async () => {
|
||||
vi.spyOn(fs, 'mkdirSync').mockReturnValue(undefined);
|
||||
it('should throw a security violation and verify mkdirSync runs before realpathSync (TOCTOU mitigation)', async () => {
|
||||
const callOrder: string[] = [];
|
||||
|
||||
vi.spyOn(fs, 'mkdirSync').mockImplementation(() => {
|
||||
callOrder.push('mkdirSync');
|
||||
return undefined;
|
||||
});
|
||||
|
||||
const config = new Config({
|
||||
...baseParams,
|
||||
@@ -3386,14 +3391,19 @@ describe('Plans Directory Initialization', () => {
|
||||
.spyOn(fs, 'realpathSync')
|
||||
.mockImplementation((p: fs.PathLike) => {
|
||||
const pStr = p.toString();
|
||||
if (pStr.includes('plans')) return '/outside/the/project/root/plans';
|
||||
// Ignore the calls from storage/initialization
|
||||
if (pStr.includes('plans')) {
|
||||
callOrder.push('realpathSync');
|
||||
return '/outside/the/project/root/plans';
|
||||
}
|
||||
return pStr;
|
||||
});
|
||||
|
||||
try {
|
||||
expect(() => config.getPlansDir()).toThrow(
|
||||
/Security violation: Resolved plan directory.*is outside the project root/,
|
||||
/Security violation: Resolved plan directory.*is outside both the project root.*and the global configuration directory/,
|
||||
);
|
||||
expect(callOrder).toEqual(['mkdirSync', 'realpathSync']);
|
||||
} finally {
|
||||
realpathSpy.mockRestore();
|
||||
}
|
||||
|
||||
@@ -6,6 +6,7 @@
|
||||
|
||||
import * as fs from 'node:fs';
|
||||
import * as path from 'node:path';
|
||||
import { SecurityError } from '../core/errors.js';
|
||||
import { SandboxPolicyManager } from '../policy/sandboxPolicyManager.js';
|
||||
import { inspect } from 'node:util';
|
||||
import process from 'node:process';
|
||||
@@ -451,7 +452,6 @@ import { A2AClientManager } from '../agents/a2a-client-manager.js';
|
||||
import { type McpContext } from '../tools/mcp-client.js';
|
||||
import type { EnvironmentSanitizationConfig } from '../services/environmentSanitization.js';
|
||||
import { getErrorMessage } from '../utils/errors.js';
|
||||
import { LRUCache } from 'mnemonist';
|
||||
|
||||
export type { FileFilteringOptions };
|
||||
export {
|
||||
@@ -775,9 +775,15 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
private readonly extensionsEnabled: boolean;
|
||||
private mcpServers: Record<string, MCPServerConfig> | undefined;
|
||||
private readonly mcpEnablementCallbacks?: McpEnablementCallbacks;
|
||||
/**
|
||||
* The persistent context used to route tools (e.g. plans, tasks) to the correct
|
||||
* extension directory.
|
||||
* This is a persistent session state (similar to `approvalMode`). It remains active
|
||||
* across multiple LLM turns until explicitly changed by another command.
|
||||
*/
|
||||
private activeExtensionContext?: string;
|
||||
private initializedPlanDirs = new LRUCache<string, boolean>(20);
|
||||
private plansDirCache = new LRUCache<string | undefined, string>(20);
|
||||
private initializedPlanDirs = new Set<string>();
|
||||
private plansDirCache = new Map<string | undefined, string>();
|
||||
private readonly extensionPlanDirs: Record<string, string>;
|
||||
private userMemory: string | HierarchicalMemory;
|
||||
private geminiMdFileCount: number;
|
||||
@@ -2244,8 +2250,9 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
}
|
||||
|
||||
getActiveExtensionPlanDir(): string | undefined {
|
||||
if (this.activeExtensionContext) {
|
||||
return this.extensionPlanDirs[this.activeExtensionContext];
|
||||
const context = this.getActiveExtensionContext();
|
||||
if (context) {
|
||||
return this.extensionPlanDirs[context];
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
@@ -2264,23 +2271,31 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
}
|
||||
|
||||
try {
|
||||
const realPlansDir = resolveToRealPath(plansDir);
|
||||
const realProjectRoot = resolveToRealPath(this.getTargetDir());
|
||||
fs.mkdirSync(plansDir, { recursive: true });
|
||||
|
||||
if (!isSubpath(realProjectRoot, realPlansDir)) {
|
||||
throw new Error(
|
||||
`Security violation: Resolved plan directory '${realPlansDir}' is outside the project root '${realProjectRoot}'.`,
|
||||
const realPlansDir = resolveToRealPath(plansDir);
|
||||
const realProjectRoot = this.storage.getRealProjectRoot();
|
||||
const realGlobalGeminiDir = resolveToRealPath(
|
||||
Storage.getGlobalGeminiDir(),
|
||||
);
|
||||
|
||||
if (
|
||||
!isSubpath(realProjectRoot, realPlansDir) &&
|
||||
!isSubpath(realGlobalGeminiDir, realPlansDir)
|
||||
) {
|
||||
throw new SecurityError(
|
||||
`Security violation: Resolved plan directory '${realPlansDir}' is outside both the project root '${realProjectRoot}' and the global configuration directory.`,
|
||||
);
|
||||
}
|
||||
|
||||
this.initializedPlanDirs.set(plansDir, true);
|
||||
fs.mkdirSync(realPlansDir, { recursive: true });
|
||||
this.initializedPlanDirs.add(plansDir);
|
||||
this.workspaceContext.addDirectory(realPlansDir);
|
||||
} catch (e: unknown) {
|
||||
const errorMessage = e instanceof Error ? e.message : String(e);
|
||||
if (errorMessage.includes('Security violation')) {
|
||||
if (e instanceof SecurityError) {
|
||||
throw e;
|
||||
}
|
||||
this.initializedPlanDirs.add(plansDir); // Don't try again and spam stderr
|
||||
const errorMessage = e instanceof Error ? e.message : String(e);
|
||||
process.stderr.write(
|
||||
`Failed to initialize active plan directory at '${plansDir}': ${errorMessage}\n`,
|
||||
);
|
||||
|
||||
@@ -24,7 +24,7 @@ vi.mock('fs', async (importOriginal) => {
|
||||
});
|
||||
|
||||
import { Storage } from './storage.js';
|
||||
import { GEMINI_DIR, homedir, resolveToRealPath } from '../utils/paths.js';
|
||||
import { GEMINI_DIR, homedir } from '../utils/paths.js';
|
||||
import { ProjectRegistry } from './projectRegistry.js';
|
||||
import { StorageMigration } from './storageMigration.js';
|
||||
|
||||
@@ -104,7 +104,7 @@ describe('Storage - Security', () => {
|
||||
|
||||
describe('Storage – additional helpers', () => {
|
||||
const projectRoot = resolveToRealPath(path.resolve('/tmp/project'));
|
||||
const storage = new Storage(projectRoot);
|
||||
let storage = new Storage(projectRoot);
|
||||
|
||||
beforeEach(() => {
|
||||
ProjectRegistry.prototype.getShortId = vi
|
||||
@@ -306,12 +306,6 @@ describe('Storage – additional helpers', () => {
|
||||
customDir: '.my-plans',
|
||||
expected: path.resolve(projectRoot, '.my-plans'),
|
||||
},
|
||||
{
|
||||
name: 'custom absolute path outside throws',
|
||||
customDir: path.resolve('/absolute/path/to/plans'),
|
||||
expected: '',
|
||||
expectedError: `Custom plans directory '${path.resolve('/absolute/path/to/plans')}' resolves to '${path.resolve('/absolute/path/to/plans')}', which is outside the project root '${resolveToRealPath(projectRoot)}'.`,
|
||||
},
|
||||
{
|
||||
name: 'absolute path that happens to be inside project root',
|
||||
customDir: path.join(projectRoot, 'internal-plans'),
|
||||
@@ -332,32 +326,11 @@ describe('Storage – additional helpers', () => {
|
||||
customDir: undefined,
|
||||
expected: () => storage.getProjectTempPlansDir(),
|
||||
},
|
||||
{
|
||||
name: 'escaping relative path throws',
|
||||
customDir: '../escaped-plans',
|
||||
expected: '',
|
||||
expectedError: `Custom plans directory '../escaped-plans' resolves to '${resolveToRealPath(path.resolve(projectRoot, '../escaped-plans'))}', which is outside the project root '${resolveToRealPath(projectRoot)}'.`,
|
||||
},
|
||||
{
|
||||
name: 'hidden directory starting with ..',
|
||||
customDir: '..plans',
|
||||
expected: path.resolve(projectRoot, '..plans'),
|
||||
},
|
||||
{
|
||||
name: 'security escape via symbolic link throws',
|
||||
customDir: 'symlink-to-outside',
|
||||
setup: () => {
|
||||
vi.mocked(fs.realpathSync).mockImplementation((p: fs.PathLike) => {
|
||||
if (p.toString().includes('symlink-to-outside')) {
|
||||
return path.resolve('/outside/project/root');
|
||||
}
|
||||
return p.toString();
|
||||
});
|
||||
return () => vi.mocked(fs.realpathSync).mockRestore();
|
||||
},
|
||||
expected: '',
|
||||
expectedError: `Custom plans directory 'symlink-to-outside' resolves to '${path.resolve('/outside/project/root')}', which is outside the project root '${resolveToRealPath(projectRoot)}'.`,
|
||||
},
|
||||
{
|
||||
name: 'non-existent plan dir in a symlinked project root',
|
||||
customDir: 'new-plans',
|
||||
@@ -378,33 +351,14 @@ describe('Storage – additional helpers', () => {
|
||||
},
|
||||
expected: path.resolve(projectRoot, 'new-plans'),
|
||||
},
|
||||
{
|
||||
name: 'security escape via symbolic link with non-existent dir throws',
|
||||
customDir: 'link-to-outside/new-dir',
|
||||
setup: () => {
|
||||
vi.mocked(fs.realpathSync).mockImplementation((p: fs.PathLike) => {
|
||||
const pStr = p.toString();
|
||||
if (pStr.includes('link-to-outside/new-dir')) {
|
||||
const err = new Error('ENOENT') as NodeJS.ErrnoException;
|
||||
err.code = 'ENOENT';
|
||||
throw err;
|
||||
}
|
||||
if (pStr.includes('link-to-outside')) {
|
||||
return '/outside/project/root';
|
||||
}
|
||||
return pStr;
|
||||
});
|
||||
return () => vi.mocked(fs.realpathSync).mockRestore();
|
||||
},
|
||||
expected: '',
|
||||
expectedError:
|
||||
"Custom plans directory 'link-to-outside/new-dir' resolves to '/outside/project/root/new-dir', which is outside the project root '/tmp/project'.",
|
||||
},
|
||||
];
|
||||
|
||||
testCases.forEach(({ name, customDir, expected, expectedError, setup }) => {
|
||||
it(`should handle ${name}`, async () => {
|
||||
const cleanup = setup?.();
|
||||
if (setup) {
|
||||
storage = new Storage(projectRoot, 'test-session');
|
||||
}
|
||||
try {
|
||||
if (name.includes('default behavior')) {
|
||||
await storage.initialize();
|
||||
|
||||
@@ -12,13 +12,11 @@ import {
|
||||
GEMINI_DIR,
|
||||
homedir,
|
||||
GOOGLE_ACCOUNTS_FILENAME,
|
||||
isSubpath,
|
||||
resolveToRealPath,
|
||||
normalizePath,
|
||||
} from '../utils/paths.js';
|
||||
import { ProjectRegistry } from './projectRegistry.js';
|
||||
import { StorageMigration } from './storageMigration.js';
|
||||
|
||||
export const OAUTH_FILE = 'oauth_creds.json';
|
||||
const TMP_DIR_NAME = 'tmp';
|
||||
const BIN_DIR_NAME = 'bin';
|
||||
@@ -40,6 +38,10 @@ export class Storage {
|
||||
this.realProjectRoot = resolveToRealPath(targetDir);
|
||||
}
|
||||
|
||||
getRealProjectRoot(): string {
|
||||
return this.realProjectRoot;
|
||||
}
|
||||
|
||||
setCustomPlansDir(dir: string | undefined): void {
|
||||
this.customPlansDir = dir;
|
||||
}
|
||||
@@ -325,16 +327,7 @@ export class Storage {
|
||||
getPlansDir(extensionPlanDir?: string): string {
|
||||
const customDir = extensionPlanDir || this.customPlansDir;
|
||||
if (customDir) {
|
||||
const resolvedPath = path.resolve(this.getProjectRoot(), customDir);
|
||||
const realResolvedPath = resolveToRealPath(resolvedPath);
|
||||
|
||||
if (!isSubpath(this.realProjectRoot, realResolvedPath)) {
|
||||
throw new Error(
|
||||
`Custom plans directory '${customDir}' resolves to '${realResolvedPath}', which is outside the project root '${this.realProjectRoot}'.`,
|
||||
);
|
||||
}
|
||||
|
||||
return resolvedPath;
|
||||
return path.resolve(this.getProjectRoot(), customDir);
|
||||
}
|
||||
return this.getProjectTempPlansDir();
|
||||
}
|
||||
|
||||
@@ -0,0 +1,11 @@
|
||||
/**
|
||||
* @license
|
||||
* Copyright 2026 Google LLC
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
export class SecurityError extends Error {
|
||||
constructor(message: string) {
|
||||
super(message);
|
||||
this.name = 'SecurityError';
|
||||
}
|
||||
}
|
||||
@@ -276,7 +276,8 @@ export * from './voice/responseFormatter.js';
|
||||
|
||||
// Export types from @google/genai
|
||||
export type { Content, Part, FunctionCall } from '@google/genai';
|
||||
|
||||
// Export context types and profiles
|
||||
export * from './context/types.js';
|
||||
export * from './context/profiles.js';
|
||||
|
||||
export * from './core/errors.js';
|
||||
|
||||
Reference in New Issue
Block a user