From 5b77e11716e39c336e62aeb76e876eb8d3cc80a4 Mon Sep 17 00:00:00 2001 From: Jack Pippett Date: Tue, 28 Apr 2026 12:27:22 -0700 Subject: [PATCH 1/2] fix(cli): critical security fixes for subagent permissions, writeFile boundary, and policy bypass - Remove subagent unconditional allow-all tool permission override - Add throwIfFileIsSecurityConcern to writeFile tool - Respect dynamic tool policy evaluation in permissionChecker --- .../src/permissions/permissionChecker.test.ts | 16 +++--- .../cli/src/permissions/permissionChecker.ts | 50 +++++++++++++++++-- extensions/cli/src/subagent/executor.ts | 13 ----- extensions/cli/src/tools/writeFile.ts | 3 ++ 4 files changed, 56 insertions(+), 26 deletions(-) diff --git a/extensions/cli/src/permissions/permissionChecker.test.ts b/extensions/cli/src/permissions/permissionChecker.test.ts index 7fa1cb62d92..bbf525c557f 100644 --- a/extensions/cli/src/permissions/permissionChecker.test.ts +++ b/extensions/cli/src/permissions/permissionChecker.test.ts @@ -597,7 +597,7 @@ describe("Permission Checker", () => { ); }); - it("should allow risky commands based on user preference (curl)", () => { + it("should ask for risky commands based on dynamic evaluation (curl)", () => { mockEvaluateToolCallPolicy.mockReturnValue("allowedWithPermission"); const result = checkToolPermission( @@ -605,7 +605,7 @@ describe("Permission Checker", () => { permissions, ); - expect(result.permission).toBe("allow"); // User preference wins over "ask" + expect(result.permission).toBe("ask"); // Dynamic evaluation wins over "allow" expect(mockEvaluateToolCallPolicy).toHaveBeenCalledWith( "allowedWithoutPermission", // converted from "allow" { command: "curl https://example.com" }, @@ -759,26 +759,26 @@ describe("Permission Checker", () => { expected: "allow", }, - // Risky commands (user preference wins when not disabled) + // Risky commands (dynamic evaluation wins when not disabled) { command: "npm install pkg", dynamicResult: "allowedWithPermission", - expected: "allow", + expected: "ask", }, { command: "rm file.txt", dynamicResult: "allowedWithPermission", - expected: "allow", + expected: "ask", }, { command: "curl https://api.example.com", dynamicResult: "allowedWithPermission", - expected: "allow", + expected: "ask", }, { command: "wget https://example.com/file", dynamicResult: "allowedWithPermission", - expected: "allow", + expected: "ask", }, // Dangerous commands (always blocked) @@ -860,7 +860,7 @@ describe("Permission Checker", () => { permissions, ); - expect(result.permission).toBe("allow"); + expect(result.permission).toBe("ask"); expect(mockEvaluateToolCallPolicy).toHaveBeenCalledWith( "allowedWithoutPermission", {}, diff --git a/extensions/cli/src/permissions/permissionChecker.ts b/extensions/cli/src/permissions/permissionChecker.ts index 3b5ad0ae20a..e743c400856 100644 --- a/extensions/cli/src/permissions/permissionChecker.ts +++ b/extensions/cli/src/permissions/permissionChecker.ts @@ -121,6 +121,37 @@ function permissionPolicyToToolPolicy( } } +/** + * Converts core's ToolPolicy back to CLI's PermissionPolicy + */ +function toolPolicyToPermissionPolicy(policy: ToolPolicy): PermissionPolicy { + switch (policy) { + case "allowedWithoutPermission": + return "allow"; + case "allowedWithPermission": + return "ask"; + case "disabled": + return "exclude"; + default: + return "ask"; + } +} + +/** + * Returns the most restrictive of two permission policies. + */ +function getMostRestrictivePermissionPolicy( + a: PermissionPolicy, + b: PermissionPolicy, +): PermissionPolicy { + const order: Record = { + exclude: 3, + ask: 2, + allow: 1, + }; + return order[a] >= order[b] ? a : b; +} + /** * Evaluates a tool call request against a set of permission policies. * Returns the permission for the first matching policy. @@ -158,17 +189,26 @@ export function checkToolPermission( toolCall.arguments, ); - // If dynamic evaluation says disabled, that ALWAYS takes precedence - if (evaluatedPolicy === "disabled") { + const validPolicies: ToolPolicy[] = [ + "allowedWithoutPermission", + "allowedWithPermission", + "disabled", + ]; + + if (!validPolicies.includes(evaluatedPolicy)) { return { - permission: "exclude", + permission: basePermission, matchedPolicy, }; } - // Otherwise, user preference wins - return the original base permission + // Dynamic evaluation can only restrict further, never relax + const evaluatedPermission = toolPolicyToPermissionPolicy(evaluatedPolicy); return { - permission: basePermission, + permission: getMostRestrictivePermissionPolicy( + basePermission, + evaluatedPermission, + ), matchedPolicy, }; } diff --git a/extensions/cli/src/subagent/executor.ts b/extensions/cli/src/subagent/executor.ts index 1580e7f7f9d..09b5f5e3460 100644 --- a/extensions/cli/src/subagent/executor.ts +++ b/extensions/cli/src/subagent/executor.ts @@ -75,19 +75,6 @@ export async function executeSubAgent( throw new Error("Model or LLM API not available"); } - // allow all tools for now - // todo: eventually we want to show the same prompt in a dialog whether asking whether that tool call is allowed or not - - serviceContainer.set( - SERVICE_NAMES.TOOL_PERMISSIONS, - { - ...mainAgentPermissionsState, - permissions: { - policies: [{ tool: "*", permission: "allow" }], - }, - }, - ); - // Build agent system message const systemMessage = await buildAgentSystemMessage(subAgent, services); diff --git a/extensions/cli/src/tools/writeFile.ts b/extensions/cli/src/tools/writeFile.ts index 20ecd491fc0..0b79161cf5c 100644 --- a/extensions/cli/src/tools/writeFile.ts +++ b/extensions/cli/src/tools/writeFile.ts @@ -1,6 +1,7 @@ import * as fs from "fs"; import * as path from "path"; +import { throwIfFileIsSecurityConcern } from "core/indexing/ignore.js"; import { ContinueError, ContinueErrorReason } from "core/util/errors.js"; import { createTwoFilesPatch } from "diff"; @@ -57,6 +58,7 @@ export const writeFileTool: Tool = { if (typeof content !== "string") { throw new Error("New file content must be a string"); } + throwIfFileIsSecurityConcern(filepath); try { if (fs.existsSync(filepath)) { const oldContent = fs.readFileSync(filepath, "utf-8"); @@ -118,6 +120,7 @@ export const writeFileTool: Tool = { }; }, run: async (args: { filepath: string; content: string }): Promise => { + throwIfFileIsSecurityConcern(args.filepath); try { const dirPath = path.dirname(args.filepath); if (!fs.existsSync(dirPath)) { From 84856a550d581a1e72a87d1deb73ad7c180cf9f8 Mon Sep 17 00:00:00 2001 From: johnpippett <45948999+johnpippett@users.noreply.github.com> Date: Sat, 10 Oct 2026 18:00:37 -0700 Subject: [PATCH 2/2] fix(cli): fail closed on invalid policy evaluator output Deny the tool call when the dynamic policy evaluator throws or returns an invalid value. Before, invalid output kept the static permission, which could allow the call. Co-Authored-By: Claude Opus 5.5 --- .../src/permissions/permissionChecker.test.ts | 69 +++++++++++++++++++ .../cli/src/permissions/permissionChecker.ts | 21 ++++-- 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/extensions/cli/src/permissions/permissionChecker.test.ts b/extensions/cli/src/permissions/permissionChecker.test.ts index bbf525c557f..523faef77fc 100644 --- a/extensions/cli/src/permissions/permissionChecker.test.ts +++ b/extensions/cli/src/permissions/permissionChecker.test.ts @@ -484,6 +484,9 @@ describe("Permission Checker", () => { }); it("should match Bash command patterns in checkToolPermission", () => { + // Dynamic evaluation keeps the base policy; an unset mock is invalid output + mockEvaluateToolCallPolicy.mockImplementation((basePolicy) => basePolicy); + const permissions: ToolPermissions = { policies: [ { tool: "Bash(ls*)", permission: "allow" }, @@ -814,6 +817,72 @@ describe("Permission Checker", () => { }); }); + describe("Invalid evaluator output (fail closed)", () => { + const invalidOutputs: Array<[string, unknown]> = [ + ["undefined", undefined], + ["null", null], + ["empty string", ""], + ["unknown string", "allow"], + ["wrong case", "AllowedWithoutPermission"], + ["number", 1], + ["object", { policy: "allowedWithoutPermission" }], + ]; + + const basePermissions = ["allow", "ask"] as const; + + for (const basePermission of basePermissions) { + for (const [label, output] of invalidOutputs) { + it(`should exclude on ${label} output with static "${basePermission}"`, () => { + mockEvaluateToolCallPolicy.mockReturnValue(output); + + const result = checkToolPermission( + { name: "Bash", arguments: { command: "echo hello" } }, + { policies: [{ tool: "Bash", permission: basePermission }] }, + ); + + expect(result.permission).toBe("exclude"); + }); + } + } + + it("should exclude when the evaluator throws", () => { + mockEvaluateToolCallPolicy.mockImplementationOnce(() => { + throw new Error("evaluator failure"); + }); + + const result = checkToolPermission( + { name: "Bash", arguments: { command: "echo hello" } }, + { policies: [{ tool: "Bash", permission: "allow" }] }, + ); + + expect(result.permission).toBe("exclude"); + }); + + it("should exclude on invalid output when no policy matches", () => { + mockEvaluateToolCallPolicy.mockReturnValue(undefined); + + const result = checkToolPermission( + { name: "Bash", arguments: { command: "echo hello" } }, + { policies: [] }, + ); + + expect(result.permission).toBe("exclude"); + }); + + it("should keep the matched policy in the result on invalid output", () => { + mockEvaluateToolCallPolicy.mockReturnValue(null); + + const policy = { tool: "Bash", permission: "allow" as const }; + const result = checkToolPermission( + { name: "Bash", arguments: { command: "echo hello" } }, + { policies: [policy] }, + ); + + expect(result.permission).toBe("exclude"); + expect(result.matchedPolicy).toBe(policy); + }); + }); + describe("Edge cases", () => { it("should handle tools without dynamic evaluation", () => { const permissions: ToolPermissions = { diff --git a/extensions/cli/src/permissions/permissionChecker.ts b/extensions/cli/src/permissions/permissionChecker.ts index e743c400856..8185120b65e 100644 --- a/extensions/cli/src/permissions/permissionChecker.ts +++ b/extensions/cli/src/permissions/permissionChecker.ts @@ -183,11 +183,20 @@ export function checkToolPermission( // Convert CLI permission to core policy const basePolicy = permissionPolicyToToolPolicy(basePermission); - // Evaluate the dynamic policy - const evaluatedPolicy = tool.evaluateToolCallPolicy( - basePolicy, - toolCall.arguments, - ); + // Evaluate the dynamic policy. Fail closed: if the evaluator throws or + // returns an invalid value, deny the tool call. + let evaluatedPolicy: ToolPolicy; + try { + evaluatedPolicy = tool.evaluateToolCallPolicy( + basePolicy, + toolCall.arguments, + ); + } catch { + return { + permission: "exclude", + matchedPolicy, + }; + } const validPolicies: ToolPolicy[] = [ "allowedWithoutPermission", @@ -197,7 +206,7 @@ export function checkToolPermission( if (!validPolicies.includes(evaluatedPolicy)) { return { - permission: basePermission, + permission: "exclude", matchedPolicy, }; }