diff --git a/packages/coding-agent/src/step/permissions.ts b/packages/coding-agent/src/step/permissions.ts index edfaf6f31..689315222 100644 --- a/packages/coding-agent/src/step/permissions.ts +++ b/packages/coding-agent/src/step/permissions.ts @@ -100,6 +100,18 @@ const WRITE_OR_EXECUTE_TOOLS = new Set([ "user_bash", ]); +/** + * Tools whose `script` argument is program source rather than a shell command. + * + * `workflow` runs JavaScript in a sandboxed VM with no process, require, or + * network, so its script never reaches a shell. Extracting it as a command made + * every ordinary multi-line script read as "unresolved" and a script that merely + * mentioned a dangerous command (inside a template literal, for example) read as + * a hazardous match. Under Approve for Me both outcomes pinned the call behind a + * confirmation the user never actually needed. + */ +const NON_SHELL_SCRIPT_TOOLS = new Set(["workflow"]); + /** Backoff shared with the previous Step gateway host (5s, 15s, 45s, 2m, 5m). */ const DEFAULT_AUTO_RESUME_DELAYS_MS = [5_000, 15_000, 45_000, 120_000, 300_000] as const; /** Unattended continuations are deliberately capped even when the ladder is extended. */ @@ -407,6 +419,10 @@ function combineThenRunDecisions( * conservative for unknown tools: ask mode confirms them, Approve for Me * permits them unless the call is hazardous or its analysis is incomplete, and * Full Access permits everything (see skipCommandPolicy). + * + * Command analysis only applies to fields that actually carry shell commands. + * A non-shell `script` field (see NON_SHELL_SCRIPT_TOOLS) skips it, so sandboxed + * source is never misread as an unanalyzable or dangerous command. */ export function decideStepToolCall( toolName: string, @@ -423,7 +439,7 @@ export function decideStepToolCall( return combineThenRunDecisions(mutation, verification, thenRun); } const normalizedName = toolName.trim().toLowerCase(); - const command = extractCommand(input); + const command = extractCommand(normalizedName, input); let analysis: CommandPolicyAnalysis | undefined; if (command !== undefined) { try { @@ -761,8 +777,12 @@ function findToolOverride( return undefined; } -function extractCommand(input: Record): string | undefined { +function extractCommand(toolName: string, input: Record): string | undefined { for (const key of ["command", "cmd", "script"]) { + // Exempt only the field known to carry non-shell source. Keeping command + // and cmd analyzable avoids turning a tool-name exception into a blanket + // exemption if the tool's input contract later grows a real shell field. + if (key === "script" && NON_SHELL_SCRIPT_TOOLS.has(toolName)) continue; const value = input[key]; if (typeof value === "string") return value; } diff --git a/packages/coding-agent/test/step-permissions.test.ts b/packages/coding-agent/test/step-permissions.test.ts index b29cb68cf..5e198388b 100644 --- a/packages/coding-agent/test/step-permissions.test.ts +++ b/packages/coding-agent/test/step-permissions.test.ts @@ -232,6 +232,156 @@ describe("Step permission presets", () => { ).toBe("confirm"); }); + describe("workflow tool calls", () => { + const inlineScript = [ + "const findings = [];", + "for (const item of items) {", + " findings.push(agent('review ' + item));", + "}", + "return findings.filter(Boolean);", + ].join("\n"); + /** + * A template literal is not a shell quote, so this is the shape the Bash + * analyzer used to parse and match a hazard rule on. Single- and + * double-quoted strings are read as shell quotes and come back `ordinary`, + * which Approve for Me already allowed, so they prove nothing here. + */ + const hazardousScript = ["const cleanup = `rm -rf ./tmp/cache`;", "return cleanup;"].join("\n"); + const wipe = "rm -rf /etc"; + const matrix = [ + { preset: "ask", action: "confirm" }, + { preset: "approve-for-me", action: "allow" }, + { preset: "full-access", action: "allow" }, + ] as const; + + it.each(matrix)("decides an inline script under $preset", ({ preset, action }) => { + const decision = decideStepToolCall( + "workflow", + { script: inlineScript }, + stepPermissionStateForPreset(preset), + ); + expect(decision).toMatchObject({ action, hazardous: false }); + expect(decision.analysisIncomplete).toBeUndefined(); + }); + + it.each(matrix)("honors explicit workflow overrides under $preset", ({ preset }) => { + expect( + decideStepToolCall("workflow", { script: inlineScript }, stepPermissionStateForPreset(preset), { + workflow: "deny", + }).action, + ).toBe("deny"); + expect( + decideStepToolCall("workflow", { script: inlineScript }, stepPermissionStateForPreset(preset), { + workflow: "allow", + }).action, + ).toBe("allow"); + }); + + it("does not read a sandboxed script as a dangerous shell command", () => { + const decision = decideStepToolCall( + "workflow", + { script: hazardousScript }, + stepPermissionStateForPreset("approve-for-me"), + ); + expect(decision).toMatchObject({ action: "allow", hazardous: false }); + expect(decision.analysisIncomplete).toBeUndefined(); + }); + + it.each(["command", "cmd"] as const)("keeps workflow.%s analyzable", (key) => { + const decision = decideStepToolCall( + "workflow", + { [key]: wipe }, + stepPermissionStateForPreset("approve-for-me"), + ); + expect(decision).toMatchObject({ action: "confirm", hazardous: true }); + }); + + it("allows the saved-name and scriptPath workflow forms under Approve for Me", () => { + for (const input of [{ name: "audit" }, { scriptPath: "./audit.js" }]) { + expect(decideStepToolCall("workflow", input, stepPermissionStateForPreset("approve-for-me")).action).toBe( + "allow", + ); + } + }); + + it("still analyzes real shell tools, so the workflow exemption cannot leak", () => { + const analyzed: [toolName: string, input: Record][] = [ + ["run_command", { command: wipe }], + ["bash", { script: wipe }], + ["workflow_helper", { command: wipe }], + ["workflow_helper", { script: wipe }], + ["my_script_runner", { script: wipe }], + ["workflow-runner", { script: wipe }], + ]; + for (const [toolName, input] of analyzed) { + const decision = decideStepToolCall(toolName, input, stepPermissionStateForPreset("ask")); + expect(decision, toolName).toMatchObject({ action: "confirm", hazardous: true }); + expect(decision.reason, toolName).toMatch(/Dangerous command requires confirmation/); + expect(decision.reason, toolName).not.toMatch(/can modify the workspace/); + } + }); + + it("exempts only workflow.script, whatever the casing or padding of the tool name", () => { + for (const toolName of ["workflow", "WORKFLOW", " WorkFlow "]) { + const exempt = decideStepToolCall( + toolName, + { script: hazardousScript }, + stepPermissionStateForPreset("ask"), + ); + expect(exempt, toolName).toMatchObject({ action: "confirm", hazardous: false }); + expect(exempt.reason, toolName).toMatch(/can modify the workspace/); + expect(exempt.analysisIncomplete, toolName).toBeUndefined(); + expect( + decideStepToolCall( + toolName, + { script: hazardousScript }, + stepPermissionStateForPreset("approve-for-me"), + ), + toolName, + ).toMatchObject({ action: "allow", hazardous: false }); + expect( + decideStepToolCall(`${toolName}_helper`, { script: wipe }, stepPermissionStateForPreset("ask")), + toolName, + ).toMatchObject({ action: "confirm", hazardous: true }); + } + }); + + it("runs the workflow hook without prompting under Approve for Me", async () => { + const controller = new StepPermissionController({ env: {}, initialPreset: "approve-for-me" }); + const confirm = vi.fn(async () => true); + const context = { hasUI: true, ui: { confirm } } as unknown as ExtensionContext; + const event = { + type: "tool_call", + toolName: "workflow", + toolCallId: "workflow-12345678", + input: { script: inlineScript }, + } as const; + + expect(await controller.handleToolCall(event, context)).toBeUndefined(); + expect(await controller.handleToolCall(event, { hasUI: false } as ExtensionContext)).toBeUndefined(); + expect(confirm).not.toHaveBeenCalled(); + }); + + it("still asks interactively under Ask and blocks when unattended", async () => { + const controller = new StepPermissionController({ env: {}, initialPreset: "ask" }); + const confirm = vi.fn(async () => false); + const context = { hasUI: true, ui: { confirm } } as unknown as ExtensionContext; + const event = { + type: "tool_call", + toolName: "workflow", + toolCallId: "workflow-87654321", + input: { script: inlineScript }, + } as const; + + expect(await controller.handleToolCall(event, context)).toMatchObject({ block: true }); + expect(confirm).toHaveBeenCalledTimes(1); + expect(await controller.handleToolCall(event, { hasUI: false } as ExtensionContext)).toMatchObject({ + block: true, + terminate: true, + }); + }); + }); + it.each([true, false])("identifies each approval without changing the decision (approved=%s)", async (approved) => { const controller = new StepPermissionController({ env: {},