From 8d584e4d9649968daad31e4f19ec0534202a3c52 Mon Sep 17 00:00:00 2001 From: Mahima Shanware Date: Tue, 7 Apr 2026 21:20:32 +0000 Subject: [PATCH] fix(core): handle nested plan files by resolving paths correctly This fixes a bug where path.basename was incorrectly stripping directory structures from plan files (e.g., trying to write to plans/nested/file.md would incorrectly write to plans/file.md). By using path.resolve and verifying with isSubpath, nested files are now handled securely and correctly. --- evals/plan_mode.eval.ts | 37 +++++++++++++++ packages/core/src/tools/edit.test.ts | 4 +- packages/core/src/tools/edit.ts | 18 ++++++-- packages/core/src/tools/write-file.ts | 21 +++++++-- packages/core/src/utils/planUtils.test.ts | 10 ++--- packages/core/src/utils/planUtils.ts | 55 +++++++++++++++++------ 6 files changed, 118 insertions(+), 27 deletions(-) diff --git a/evals/plan_mode.eval.ts b/evals/plan_mode.eval.ts index 799fb6acb1..0580c6acb7 100644 --- a/evals/plan_mode.eval.ts +++ b/evals/plan_mode.eval.ts @@ -372,4 +372,41 @@ describe('plan_mode', () => { assertModelHasOutput(result); }, }); + + evalTest('ALWAYS_PASSES', { + name: 'should handle nested plan directories correctly', + approvalMode: ApprovalMode.PLAN, + params: { + settings, + }, + prompt: + 'Please create a new architectural plan in a nested folder called "architecture/frontend-v2.md" within the plans directory. The plan should contain the text "# Frontend V2 Plan". Do not ask for user approval, just create the plan.', + assert: async (rig, result) => { + await rig.waitForTelemetryReady(); + const toolLogs = rig.readToolLogs(); + + const writeCalls = toolLogs.filter((log) => + ['write_file', 'replace'].includes(log.toolRequest.name), + ); + + const wroteToNestedPath = writeCalls.some((log) => { + try { + const args = JSON.parse(log.toolRequest.args); + return ( + args.file_path && + args.file_path.includes('architecture/frontend-v2.md') + ); + } catch { + return false; + } + }); + + expect( + wroteToNestedPath, + 'Expected model to successfully target the nested plan file path', + ).toBe(true); + + assertModelHasOutput(result); + }, + }); }); diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index d6d65da238..7d437c4fde 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -1338,8 +1338,8 @@ function doIt() { vi.mocked(mockConfig.getPlansDir).mockReturnValue(plansDir); vi.mocked(mockConfig.storage.getPlansDir).mockReturnValue(plansDir); - const filePath = path.join(rootDir, 'test-file.txt'); - const planFilePath = path.join(plansDir, 'test-file.txt'); + const filePath = 'test-file.txt'; + const planFilePath = path.join(plansDir, filePath); const initialContent = 'some initial content'; fs.writeFileSync(planFilePath, initialContent, 'utf8'); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index a0da6cb7ff..9ed696cbd2 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -58,6 +58,7 @@ import { EDIT_DEFINITION } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; import { detectOmissionPlaceholders } from './omissionPlaceholderDetector.js'; import { discoverJitContext, appendJitContext } from './jit-context.js'; +import { resolveAndValidatePlanPath } from '../utils/planUtils.js'; const ENABLE_FUZZY_MATCH_RECOVERY = true; const FUZZY_MATCH_THRESHOLD = 0.1; // Allow up to 10% weighted difference @@ -465,8 +466,10 @@ class EditToolInvocation () => this.config.getApprovalMode(), ); if (this.config.isPlanMode()) { - const safeFilename = path.basename(this.params.file_path); - this.resolvedPath = path.join(this.config.getPlansDir(), safeFilename); + this.resolvedPath = resolveAndValidatePlanPath( + this.params.file_path, + this.config.getPlansDir(), + ); } else if (!path.isAbsolute(this.params.file_path)) { const result = correctPath(this.params.file_path, this.config); if (result.success) { @@ -1051,7 +1054,16 @@ export class EditTool } let resolvedPath: string; - if (!path.isAbsolute(params.file_path)) { + if (this.config.isPlanMode()) { + try { + resolvedPath = resolveAndValidatePlanPath( + params.file_path, + this.config.getPlansDir(), + ); + } catch (err) { + return err instanceof Error ? err.message : String(err); + } + } else if (!path.isAbsolute(params.file_path)) { const result = correctPath(params.file_path, this.config); if (result.success) { resolvedPath = result.correctedPath; diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 5348fbe27e..4f972d8cf1 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -49,6 +49,7 @@ import { debugLogger } from '../utils/debugLogger.js'; import { WRITE_FILE_DEFINITION } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; import { detectOmissionPlaceholders } from './omissionPlaceholderDetector.js'; +import { resolveAndValidatePlanPath } from '../utils/planUtils.js'; import { isGemini3Model } from '../config/models.js'; import { discoverJitContext, appendJitContext } from './jit-context.js'; @@ -168,8 +169,10 @@ class WriteFileToolInvocation extends BaseToolInvocation< ); if (this.config.isPlanMode()) { - const safeFilename = path.basename(this.params.file_path); - this.resolvedPath = path.join(this.config.getPlansDir(), safeFilename); + this.resolvedPath = resolveAndValidatePlanPath( + this.params.file_path, + this.config.getPlansDir(), + ); } else { this.resolvedPath = path.resolve( this.config.getTargetDir(), @@ -496,7 +499,19 @@ export class WriteFileTool return `Missing or empty "file_path"`; } - const resolvedPath = path.resolve(this.config.getTargetDir(), filePath); + let resolvedPath: string; + if (this.config.isPlanMode()) { + try { + resolvedPath = resolveAndValidatePlanPath( + filePath, + this.config.getPlansDir(), + ); + } catch (err) { + return err instanceof Error ? err.message : String(err); + } + } else { + resolvedPath = path.resolve(this.config.getTargetDir(), filePath); + } const validationError = this.config.validatePathAccess(resolvedPath); if (validationError) { diff --git a/packages/core/src/utils/planUtils.test.ts b/packages/core/src/utils/planUtils.test.ts index e7d953b41a..f70b5f16e0 100644 --- a/packages/core/src/utils/planUtils.test.ts +++ b/packages/core/src/utils/planUtils.test.ts @@ -31,8 +31,8 @@ describe('planUtils', () => { describe('validatePlanPath', () => { it('should return null for a valid path within plans directory', async () => { - const planPath = path.join('plans', 'test.md'); - const fullPath = path.join(tempRootDir, planPath); + const planPath = 'test.md'; + const fullPath = path.join(plansDir, planPath); fs.writeFileSync(fullPath, '# My Plan'); const result = await validatePlanPath(planPath, plansDir); @@ -40,14 +40,14 @@ describe('planUtils', () => { }); it('should return error for non-existent file', async () => { - const planPath = path.join('plans', 'ghost.md'); + const planPath = 'ghost.md'; const result = await validatePlanPath(planPath, plansDir); expect(result).toContain('Plan file does not exist'); }); it('should detect path traversal via symbolic links', async () => { - const maliciousPath = path.join('plans', 'malicious.md'); - const fullMaliciousPath = path.join(tempRootDir, maliciousPath); + const maliciousPath = 'malicious.md'; + const fullMaliciousPath = path.join(plansDir, maliciousPath); const outsideFile = path.join(tempRootDir, 'outside.txt'); fs.writeFileSync(outsideFile, 'secret content'); diff --git a/packages/core/src/utils/planUtils.ts b/packages/core/src/utils/planUtils.ts index 559434b1e3..c0ae78b613 100644 --- a/packages/core/src/utils/planUtils.ts +++ b/packages/core/src/utils/planUtils.ts @@ -22,31 +22,58 @@ export const PlanErrorMessages = { READ_FAILURE: (detail: string) => `Failed to read plan file: ${detail}`, } as const; +/** + * Resolves a plan file path and strictly validates it against the plans directory boundary. + * Useful for tools that need to write or read plans. + * @param planPath The untrusted file path provided by the model. + * @param plansDir The authorized project plans directory. + * @returns The safely resolved path string. + * @throws Error if the path is empty, malicious, or escapes boundaries. + */ +export function resolveAndValidatePlanPath( + planPath: string, + plansDir: string, +): string { + const trimmedPath = planPath.trim(); + if (!trimmedPath) { + throw new Error('Plan file path must be non-empty.'); + } + + const resolvedPath = path.resolve(plansDir, trimmedPath); + const realPath = resolveToRealPath(resolvedPath); + const realPlansDir = resolveToRealPath(plansDir); + + if (!isSubpath(realPlansDir, realPath)) { + throw new Error( + `Security violation: plan path (${trimmedPath}) must be within the designated plans directory (${plansDir}).`, + ); + } + + return resolvedPath; +} + /** * Validates a plan file path for safety (traversal) and existence. * @param planPath The untrusted path to the plan file. * @param plansDir The authorized project plans directory. - * @param targetDir The current working directory (project root). * @returns An error message if validation fails, or null if successful. */ export async function validatePlanPath( planPath: string, plansDir: string, ): Promise { - const safeFilename = path.basename(planPath); - const resolvedPath = path.join(plansDir, safeFilename); - const realPath = resolveToRealPath(resolvedPath); - const realPlansDir = resolveToRealPath(plansDir); - - if (!isSubpath(realPlansDir, realPath)) { - return PlanErrorMessages.PATH_ACCESS_DENIED(planPath, realPlansDir); + try { + const resolvedPath = resolveAndValidatePlanPath(planPath, plansDir); + if (!(await fileExists(resolvedPath))) { + return PlanErrorMessages.FILE_NOT_FOUND(planPath); + } + return null; + } catch { + return PlanErrorMessages.PATH_ACCESS_DENIED( + planPath, + resolveToRealPath(plansDir), + ); } - - if (!(await fileExists(resolvedPath))) { - return PlanErrorMessages.FILE_NOT_FOUND(planPath); - } - - return null; } /**