fix: address final review feedback on context resets and pre-creation path validation

This commit is contained in:
Mahima Shanware
2026-04-07 04:39:15 +00:00
parent 35fd166bce
commit 432df7982c
2 changed files with 51 additions and 36 deletions
@@ -372,13 +372,11 @@ export const useSlashCommandProcessor = (
} = parseSlashCommand(trimmed, commands);
if (config && commandToExecute && !commandToExecute.isSafeConcurrent) {
if (extensionContext) {
if (config.hasExtensionPlanDir(extensionContext)) {
config.setActiveExtensionContext(extensionContext);
} else {
config.setActiveExtensionContext(undefined);
}
}
config.setActiveExtensionContext(
extensionContext && config.hasExtensionPlanDir(extensionContext)
? extensionContext
: undefined,
);
}
// If the input doesn't match any known command, check if MCP servers
+46 -29
View File
@@ -2266,25 +2266,10 @@ export class Config implements McpContext, AgentLoopContext {
this.plansDirCache.set(context, plansDir);
}
if (!this.planEnabled || this.initializedPlanDirs.has(plansDir)) {
if (this.initializedPlanDirs.has(plansDir)) {
return plansDir;
}
let mkdirError: unknown;
try {
fs.mkdirSync(plansDir, { recursive: true });
} catch (e: unknown) {
mkdirError = e;
}
let realPlansDir: string;
try {
realPlansDir = resolveToRealPath(plansDir);
} catch {
// Fallback to path.resolve if the directory doesn't exist yet (e.g. mkdirSync failed)
// so that the security check can still be performed on the absolute path.
realPlansDir = path.resolve(plansDir);
}
const realProjectRoot = this.storage.getRealProjectRoot();
let realGlobalGeminiDir: string;
try {
@@ -2292,26 +2277,58 @@ export class Config implements McpContext, AgentLoopContext {
} catch {
realGlobalGeminiDir = path.resolve(Storage.getGlobalGeminiDir());
}
// 1. Lexical security check (before any filesystem mutation or return)
const lexicalPlansDir = path.resolve(plansDir);
if (
!isSubpath(realProjectRoot, realPlansDir) &&
!isSubpath(realGlobalGeminiDir, realPlansDir)
!isSubpath(realProjectRoot, lexicalPlansDir) &&
!isSubpath(realGlobalGeminiDir, lexicalPlansDir)
) {
throw new SecurityError(
`Security violation: Resolved plan directory '${realPlansDir}' is outside both the project root '${realProjectRoot}' and the global configuration directory.`,
`Security violation: Plan directory '${lexicalPlansDir}' is outside both the project root '${realProjectRoot}' and the global configuration directory.`,
);
}
// 2. We only attempt to physically create the directory if plan mode is enabled
if (this.planEnabled) {
let mkdirError: unknown;
try {
fs.mkdirSync(plansDir, { recursive: true });
} catch (e: unknown) {
mkdirError = e;
}
// 3. Physical security check (after creation, to mitigate TOCTOU symlink attacks)
let realPlansDir: string;
try {
realPlansDir = resolveToRealPath(plansDir);
} catch {
// Fallback to path.resolve if the directory doesn't exist yet (e.g. mkdirSync failed)
// so that the security check can still be performed on the absolute path.
realPlansDir = path.resolve(plansDir);
}
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.`,
);
}
if (mkdirError) {
const errorMessage =
mkdirError instanceof Error ? mkdirError.message : String(mkdirError);
process.stderr.write(
`Failed to initialize active plan directory at '${plansDir}': ${errorMessage}\n`,
);
} else {
this.workspaceContext.addDirectory(realPlansDir);
}
}
this.initializedPlanDirs.add(plansDir);
if (mkdirError) {
const errorMessage =
mkdirError instanceof Error ? mkdirError.message : String(mkdirError);
process.stderr.write(
`Failed to initialize active plan directory at '${plansDir}': ${errorMessage}\n`,
);
} else {
this.workspaceContext.addDirectory(realPlansDir);
}
return plansDir;
}