From ded474c2d00d35462649d9fb09d2a82e07e33081 Mon Sep 17 00:00:00 2001 From: Mahima Shanware Date: Mon, 6 Apr 2026 22:10:19 +0000 Subject: [PATCH] fix(core): fail-closed security for plan directory TOCTOU 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'. --- packages/core/src/config/config.test.ts | 60 +++++++++++-------------- packages/core/src/config/config.ts | 19 ++++---- 2 files changed, 36 insertions(+), 43 deletions(-) diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index ba3a936e7e..89c41150bf 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -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 () => { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 684f9a93ef..f7f985323b 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -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;