fix(cli): critical security fixes for subagent permissions, writeFile boundary, and policy bypass

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,15 +600,15 @@ 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(
           { name: "Bash", arguments: { command: "curl https://example.com" } },
           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",
           {},

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<PermissionPolicy, number> = {
+    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,
     };
   }

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<ToolPermissionServiceState>(
-      SERVICE_NAMES.TOOL_PERMISSIONS,
-      {
-        ...mainAgentPermissionsState,
-        permissions: {
-          policies: [{ tool: "*", permission: "allow" }],
-        },
-      },
-    );
-
     // Build agent system message
     const systemMessage = await buildAgentSystemMessage(subAgent, services);
 

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<string> => {
+    throwIfFileIsSecurityConcern(args.filepath);
     try {
       const dirPath = path.dirname(args.filepath);
       if (!fs.existsSync(dirPath)) {