MediaMetz/gemini-cli
1
0
Fork 0
mirror of https://github.com/google-gemini/gemini-cli.git synced 2026-06-13 12:57:12 -07:00
Code Issues Packages Projects Releases Wiki Activity

fix(core): fail-closed security for plan directory TOCTOU

Browse Source 
Resolves security review findings:
- Reordered resolveToRealPath before mkdirSync to fully eliminate TOCTOU risks with symlink injection.
- Fail closed by re-throwing 'Security violation' errors instead of swallowing them.
- Replaced lint-disabler with process.stderr.write for legitimate fallback warnings.
- Used direct context string as LRUCache key to avoid collision with an extension potentially named 'default'.
This commit is contained in:
Mahima Shanware
2026-04-06 22:10:19 +00:00
parent 3256b16039
commit ded474c2d0
2 changed files with 36 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/core/src/config/config.test.ts
+27 -33
View File
@@ -3345,7 +3345,9 @@ describe('Plans Directory Initialization', () => {
});
it('should log a warning if the plan directory path is blocked by an existing file (EEXIST)', async () => {
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
const writeSpy = vi
.spyOn(process.stderr, 'write')
.mockImplementation(() => true);
vi.spyOn(fs, 'mkdirSync').mockImplementation(() => {
const err = new Error('File exists') as NodeJS.ErrnoException;
err.code = 'EEXIST';
@@ -3359,33 +3361,16 @@ describe('Plans Directory Initialization', () => {
await config.initialize();
config.getPlansDir();
expect(warnSpy).toHaveBeenCalledWith(
expect(writeSpy).toHaveBeenCalledWith(
expect.stringMatching(
/Failed to initialize active plan directory.*File exists/,
),
);
warnSpy.mockRestore();
writeSpy.mockRestore();
});
it('should log a warning if the resolved plan directory is outside the project root (TOCTOU security violation)', async () => {
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
let isMalicious = false;
vi.spyOn(fs, 'mkdirSync').mockImplementation(() => {
// Simulate an attacker creating a symlink right after the initial check but before/during creation
isMalicious = true;
return undefined;
});
// When Config.getPlansDir calls resolveToRealPath AFTER creation, it resolves to an outside path.
const realpathSpy = vi
.spyOn(fs, 'realpathSync')
.mockImplementation((p: fs.PathLike) => {
const pStr = p.toString();
if (isMalicious && pStr.includes('plans'))
return '/outside/the/project/root/plans';
return pStr;
});
it('should throw a security violation if the resolved plan directory is outside the project root', async () => {
vi.spyOn(fs, 'mkdirSync').mockReturnValue(undefined);
const config = new Config({
...baseParams,
@@ -3393,22 +3378,31 @@ describe('Plans Directory Initialization', () => {
});
await config.initialize();
// Bypass Storage check so we can specifically test Config's check
vi.spyOn(config.storage, 'getPlansDir').mockReturnValue('/tmp/test/plans');
const realpathSpy = vi
.spyOn(fs, 'realpathSync')
.mockImplementation((p: fs.PathLike) => {
const pStr = p.toString();
if (pStr.includes('plans')) return '/outside/the/project/root/plans';
return pStr;
});
try {
config.getPlansDir();
expect(() => config.getPlansDir()).toThrow(
/Security violation: Resolved plan directory.*is outside the project root/,
);
} finally {
realpathSpy.mockRestore();
}
expect(warnSpy).toHaveBeenCalledWith(
expect.stringMatching(
/Security violation: Resolved plan directory.*is outside the project root/,
),
);
warnSpy.mockRestore();
});
it('should log a warning if mkdirSync fails during getPlansDir (e.g. EACCES)', async () => {
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
const writeSpy = vi
.spyOn(process.stderr, 'write')
.mockImplementation(() => true);
vi.spyOn(fs, 'mkdirSync').mockImplementation(() => {
const err = new Error('Permission denied') as NodeJS.ErrnoException;
err.code = 'EACCES';
@@ -3422,12 +3416,12 @@ describe('Plans Directory Initialization', () => {
await config.initialize();
config.getPlansDir();
expect(warnSpy).toHaveBeenCalledWith(
expect(writeSpy).toHaveBeenCalledWith(
expect.stringMatching(
/Failed to initialize active plan directory.*Permission denied/,
),
);
warnSpy.mockRestore();
writeSpy.mockRestore();
});
it('should deduplicate and cache when multiple extensions (or default) use the same directory', async () => {
packages/core/src/config/config.ts
+9 -10
View File
@@ -2252,13 +2252,11 @@ export class Config implements McpContext, AgentLoopContext {
getPlansDir(): string {
const context = this.getActiveExtensionContext();
// Cache key: undefined means default context, string means extension context
const cacheKey = context === undefined ? 'default' : context;
let plansDir = this.plansDirCache.get(cacheKey);
let plansDir = this.plansDirCache.get(context);
if (plansDir === undefined) {
plansDir = this.storage.getPlansDir(this.getActiveExtensionPlanDir());
this.plansDirCache.set(cacheKey, plansDir);
this.plansDirCache.set(context, plansDir);
}
if (!this.planEnabled || this.initializedPlanDirs.has(plansDir)) {
@@ -2266,8 +2264,6 @@ export class Config implements McpContext, AgentLoopContext {
}
try {
fs.mkdirSync(plansDir, { recursive: true });
const realPlansDir = resolveToRealPath(plansDir);
const realProjectRoot = resolveToRealPath(this.getTargetDir());
@@ -2277,13 +2273,16 @@ export class Config implements McpContext, AgentLoopContext {
);
}
this.workspaceContext.addDirectory(realPlansDir);
this.initializedPlanDirs.set(plansDir, true);
fs.mkdirSync(realPlansDir, { recursive: true });
this.workspaceContext.addDirectory(realPlansDir);
} catch (e: unknown) {
const errorMessage = e instanceof Error ? e.message : String(e);
// eslint-disable-next-line no-console
console.warn(
`Failed to initialize active plan directory at '${plansDir}': ${errorMessage}`,
if (errorMessage.includes('Security violation')) {
throw e;
}
process.stderr.write(
`Failed to initialize active plan directory at '${plansDir}': ${errorMessage}\n`,
);
}
return plansDir;