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)) {