mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-05-14 22:02:59 -07:00
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.
This commit is contained in:
@@ -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);
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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');
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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');
|
||||
|
||||
|
||||
@@ -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<string | null> {
|
||||
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;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user