diff --git a/extensions/cli/src/permissions/permissionChecker.test.ts b/extensions/cli/src/permissions/permissionChecker.test.ts index 7fa1cb62d92..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" }, @@ -597,7 +600,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 +608,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 +762,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) @@ -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 = { @@ -860,7 +929,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..8185120b65e 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. @@ -152,23 +183,41 @@ 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", + "allowedWithPermission", + "disabled", + ]; - // If dynamic evaluation says disabled, that ALWAYS takes precedence - if (evaluatedPolicy === "disabled") { + if (!validPolicies.includes(evaluatedPolicy)) { return { permission: "exclude", 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)) {