From a47ea3f7a1fbdc4229fde1d96687a6a6b9c1881f Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 5 Jun 2026 01:05:51 -0700 Subject: [PATCH] fix(review): strip approval-bypass flags in agent workflow authoring for chat/planning lanes --- .../strip-approval-bypass-flags.test.ts | 79 ++++++++++++++++ packages/core/src/index.ts | 1 + packages/core/src/workflow-ir.ts | 36 ++++++++ packages/dashboard/src/chat.ts | 2 +- packages/dashboard/src/planning.ts | 4 +- .../src/routes/register-workflow-routes.ts | 31 ++----- .../agent-workflow-tools-exposure.test.ts | 90 +++++++++++++++++++ packages/engine/src/agent-tools.ts | 56 +++++++++--- packages/engine/src/executor.ts | 12 ++- 9 files changed, 269 insertions(+), 42 deletions(-) create mode 100644 packages/core/src/__tests__/strip-approval-bypass-flags.test.ts diff --git a/packages/core/src/__tests__/strip-approval-bypass-flags.test.ts b/packages/core/src/__tests__/strip-approval-bypass-flags.test.ts new file mode 100644 index 0000000000..b17e3b26fd --- /dev/null +++ b/packages/core/src/__tests__/strip-approval-bypass-flags.test.ts @@ -0,0 +1,79 @@ +import { describe, it, expect } from "vitest"; +import { stripApprovalBypassFlags } from "../workflow-ir.js"; +import type { WorkflowIr } from "../workflow-ir-types.js"; + +/** + * P0 security helper: removes the CLI-approval-bypass flags + * (`cliSkipApproval`/`autoApprove`) from every node config, recursing into + * foreach `config.template.nodes` at any nesting depth. + */ +describe("stripApprovalBypassFlags", () => { + it("removes both flags from a top-level node config and reports stripped:true", () => { + const ir = { + version: "v1", + name: "wf", + nodes: [{ id: "n1", kind: "prompt", config: { cliSkipApproval: true, autoApprove: true, name: "x" } }], + edges: [], + } as unknown as WorkflowIr; + const { ir: out, stripped } = stripApprovalBypassFlags(ir); + expect(stripped).toBe(true); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const cfg = (out as any).nodes[0].config; + expect(cfg.cliSkipApproval).toBeUndefined(); + expect(cfg.autoApprove).toBeUndefined(); + expect(cfg.name).toBe("x"); // unrelated config preserved + }); + + it("strips nested foreach-in-foreach template nodes (arbitrary depth)", () => { + const ir = { + version: "v1", + name: "wf", + nodes: [ + { + id: "outer", + kind: "foreach", + config: { + template: { + nodes: [ + { + id: "inner-foreach", + kind: "foreach", + config: { + template: { + nodes: [ + { id: "deep", kind: "step-execute", config: { autoApprove: true } }, + ], + edges: [], + }, + }, + }, + ], + edges: [], + }, + }, + }, + ], + edges: [], + } as unknown as WorkflowIr; + const { stripped } = stripApprovalBypassFlags(ir); + expect(stripped).toBe(true); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const deep = (ir as any).nodes[0].config.template.nodes[0].config.template.nodes[0]; + expect(deep.config.autoApprove).toBeUndefined(); + }); + + it("returns stripped:false when no flags present", () => { + const ir = { + version: "v1", + name: "wf", + nodes: [{ id: "n1", kind: "prompt", config: { name: "x" } }], + edges: [], + } as unknown as WorkflowIr; + expect(stripApprovalBypassFlags(ir).stripped).toBe(false); + }); + + it("tolerates a non-array nodes field", () => { + const ir = { version: "v1", name: "wf" } as unknown as WorkflowIr; + expect(stripApprovalBypassFlags(ir).stripped).toBe(false); + }); +}); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b1b0dc3c78..23e23c4a63 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -48,6 +48,7 @@ export { export { parseWorkflowIr, serializeWorkflowIr, + stripApprovalBypassFlags, WorkflowIrError, DEFAULT_WORKFLOW_COLUMN_IDS, } from "./workflow-ir.js"; diff --git a/packages/core/src/workflow-ir.ts b/packages/core/src/workflow-ir.ts index 6a3cc5eff8..577169eb14 100644 --- a/packages/core/src/workflow-ir.ts +++ b/packages/core/src/workflow-ir.ts @@ -906,3 +906,39 @@ export function downgradeIrToV1IfPure(ir: WorkflowIr): WorkflowIr { export function serializeWorkflowIr(ir: WorkflowIr): string { return JSON.stringify(ir, null, 2); } + +/** + * Strip the trust-escalating `cliSkipApproval`/`autoApprove` flags from every + * node config in an IR, recursing into foreach `config.template.nodes` at any + * nesting depth (foreach-in-foreach). Mutates the passed IR in place and returns + * it alongside a `stripped` flag indicating whether anything was removed. + * + * These flags bypass the CLI first-run approval gate (see executor.ts). They are + * legitimate only for workflows authored through the trusted dashboard editor / + * executor lane; on prompt-injectable surfaces (chat/planning authoring tools, + * import, AI design) they must be removed at the write boundary. + */ +export function stripApprovalBypassFlags(ir: WorkflowIr): { ir: WorkflowIr; stripped: boolean } { + const nodes = (ir as { nodes?: WorkflowIrNode[] }).nodes; + if (!Array.isArray(nodes)) return { ir, stripped: false }; + let stripped = false; + const stripNode = (node: WorkflowIrNode): void => { + const cfg = node.config as Record | undefined; + if (cfg && typeof cfg === "object") { + if ("cliSkipApproval" in cfg) { + delete cfg.cliSkipApproval; + stripped = true; + } + if ("autoApprove" in cfg) { + delete cfg.autoApprove; + stripped = true; + } + const template = (cfg as { template?: { nodes?: unknown } }).template; + if (template && Array.isArray(template.nodes)) { + for (const inner of template.nodes as WorkflowIrNode[]) stripNode(inner); + } + } + }; + for (const node of nodes) stripNode(node); + return { ir, stripped }; +} diff --git a/packages/dashboard/src/chat.ts b/packages/dashboard/src/chat.ts index 6afb0cd1ec..cf8e10b04b 100644 --- a/packages/dashboard/src/chat.ts +++ b/packages/dashboard/src/chat.ts @@ -1616,7 +1616,7 @@ export class ChatManager { // is available. The chat lane has no ambient task, so fn_workflow_select // has no default target — an agent must pass an explicit task_id. const workflowTools = this.taskStore - ? createWorkflowAuthoringTools(this.taskStore, "") + ? createWorkflowAuthoringTools(this.taskStore, "", { stripApprovalFlags: true }) : []; const customTools = [...messagingTools, ...workflowTools]; diff --git a/packages/dashboard/src/planning.ts b/packages/dashboard/src/planning.ts index 6f62e51cad..6ccf89f44c 100644 --- a/packages/dashboard/src/planning.ts +++ b/packages/dashboard/src/planning.ts @@ -848,7 +848,7 @@ export async function createSession( builtinToolsAllowlist: [...PLANNING_BUILTIN_WEB_TOOLS], customTools: [ ...createPlanningBoardTools(store), - ...createWorkflowAuthoringTools(store, PLANNING_NO_AMBIENT_TASK_ID), + ...createWorkflowAuthoringTools(store, PLANNING_NO_AMBIENT_TASK_ID, { stripApprovalFlags: true }), ], onThinking: () => { // Non-streaming path ignores thinking output @@ -1392,7 +1392,7 @@ async function createPlanningAgent( builtinToolsAllowlist: [...PLANNING_BUILTIN_WEB_TOOLS], customTools: [ ...createPlanningBoardTools(store), - ...createWorkflowAuthoringTools(store, PLANNING_NO_AMBIENT_TASK_ID), + ...createWorkflowAuthoringTools(store, PLANNING_NO_AMBIENT_TASK_ID, { stripApprovalFlags: true }), ], ...(modelProvider && modelId ? { diff --git a/packages/dashboard/src/routes/register-workflow-routes.ts b/packages/dashboard/src/routes/register-workflow-routes.ts index 1f01dad672..4fc4e0b205 100644 --- a/packages/dashboard/src/routes/register-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-workflow-routes.ts @@ -1,5 +1,5 @@ import type { WorkflowDefinition, WorkflowDefinitionKind, WorkflowIr, WorkflowIrNode } from "@fusion/core"; -import { ColumnTraitValidationError, OccupiedColumnsError, InvalidRehomeTargetError, WorkflowCompileError, WorkflowIrError, SCHEMA_VERSION, assertColumnTraitsValid, compileWorkflowToSteps, layoutForIr, listTraits, listStepParsers, parseWorkflowIr, resolvePlanningSettingsModel } from "@fusion/core"; +import { ColumnTraitValidationError, OccupiedColumnsError, InvalidRehomeTargetError, WorkflowCompileError, WorkflowIrError, SCHEMA_VERSION, assertColumnTraitsValid, compileWorkflowToSteps, layoutForIr, listTraits, listStepParsers, parseWorkflowIr, resolvePlanningSettingsModel, stripApprovalBypassFlags } from "@fusion/core"; import { createFnAgent as engineCreateFnAgent, validateCodeNodeSources } from "@fusion/engine"; import { ApiError, badRequest, conflict, notFound, rateLimited } from "../api-error.js"; import { emitWorkflowSseEvent } from "../sse.js"; @@ -755,31 +755,12 @@ function assertImportTraitsValid(ir: WorkflowIr): void { } /** Strip `cliSkipApproval`/`autoApprove` from every node config in the IR, - * including configs nested inside foreach `template.nodes`. Returns true when - * anything was removed so the response can flag it (R10 trust boundary). */ + * including configs nested inside foreach `template.nodes` (any depth). Returns + * true when anything was removed so the response can flag it (R10 trust + * boundary). Delegates to the shared @fusion/core helper so the route and the + * chat/planning authoring tools cannot diverge. */ function stripApprovalFlags(ir: WorkflowIr): boolean { - const nodes = (ir as { nodes?: WorkflowIrNode[] }).nodes; - if (!Array.isArray(nodes)) return false; - let stripped = false; - const stripNode = (node: WorkflowIrNode): void => { - const cfg = node.config as Record | undefined; - if (cfg && typeof cfg === "object") { - if ("cliSkipApproval" in cfg) { - delete cfg.cliSkipApproval; - stripped = true; - } - if ("autoApprove" in cfg) { - delete cfg.autoApprove; - stripped = true; - } - const template = (cfg as { template?: { nodes?: unknown } }).template; - if (template && Array.isArray(template.nodes)) { - for (const inner of template.nodes as WorkflowIrNode[]) stripNode(inner); - } - } - }; - for (const node of nodes) stripNode(node); - return stripped; + return stripApprovalBypassFlags(ir).stripped; } /** Collect non-blocking warnings for script nodes (and any config carrying a diff --git a/packages/engine/src/__tests__/agent-workflow-tools-exposure.test.ts b/packages/engine/src/__tests__/agent-workflow-tools-exposure.test.ts index c757b01c65..6f90de23c7 100644 --- a/packages/engine/src/__tests__/agent-workflow-tools-exposure.test.ts +++ b/packages/engine/src/__tests__/agent-workflow-tools-exposure.test.ts @@ -54,3 +54,93 @@ describe("workflow tool exposure (engine factories)", () => { expect(createWorkflowDeleteTool(fakeStore).name).toBe("fn_workflow_delete"); }); }); + +/** + * P0 security: the chat/planning lanes pass {stripApprovalFlags:true} so the + * create/update tools cannot persist a CLI-approval-bypass smuggled through a + * prompt-injectable agent lane. The executor lane omits the option (project- + * owner escape hatch) and the flags pass through unchanged. + */ +describe("workflow authoring tools approval-flag stripping", () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + function captureStore(): { store: TaskStore; captured: { ir?: any } } { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const captured: { ir?: any } = {}; + const store = { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + createWorkflowDefinition: async (input: any) => { + captured.ir = input.ir; + return { id: "wf-1", name: input.name }; + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + updateWorkflowDefinition: async (_id: string, input: any) => { + captured.ir = input.ir; + return { id: "wf-1", name: input.name ?? "wf" }; + }, + } as unknown as TaskStore; + return { store, captured }; + } + + // The ToolDefinition.execute signature requires (id, params, signal, onUpdate, + // ctx); tests only need id+params, so pass undefined for the rest and cast the + // result to read the optional `isError`/`content` shape these tools return. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const run = (tool: { execute: (...a: any[]) => Promise }, params: unknown) => + // eslint-disable-next-line @typescript-eslint/no-explicit-any + tool.execute("call-1", params, undefined, undefined, undefined) as Promise<{ + // eslint-disable-next-line @typescript-eslint/no-explicit-any + isError?: boolean; content: { type: string; text?: string }[]; + }>; + + const irWithFlags = () => ({ + version: "v1" as const, + name: "wf", + nodes: [ + { id: "n1", kind: "prompt", config: { cliSkipApproval: true, name: "x" } }, + { + id: "fe", + kind: "foreach", + config: { + template: { + nodes: [ + { id: "inner", kind: "step-execute", config: { autoApprove: true } }, + ], + edges: [], + }, + }, + }, + ], + edges: [], + }); + + it("strips both flags (incl. foreach template) on create with stripApprovalFlags:true", async () => { + const { store, captured } = captureStore(); + const tool = createWorkflowCreateTool(store, { stripApprovalFlags: true }); + const res = await run(tool, { name: "wf", ir: irWithFlags() }); + expect(res.isError).toBeFalsy(); + expect(captured.ir.nodes[0].config.cliSkipApproval).toBeUndefined(); + expect(captured.ir.nodes[1].config.template.nodes[0].config.autoApprove).toBeUndefined(); + const text = res.content.map((c) => c.text ?? "").join(""); + expect(text).toContain("approval-bypass flags removed"); + }); + + it("strips flags on update with stripApprovalFlags:true", async () => { + const { store, captured } = captureStore(); + const tool = createWorkflowUpdateTool(store, { stripApprovalFlags: true }); + const res = await run(tool, { workflow_id: "wf-1", ir: irWithFlags() }); + expect(res.isError).toBeFalsy(); + expect(captured.ir.nodes[0].config.cliSkipApproval).toBeUndefined(); + expect(captured.ir.nodes[1].config.template.nodes[0].config.autoApprove).toBeUndefined(); + }); + + it("executor lane (no option) passes both flags through unchanged", async () => { + const { store, captured } = captureStore(); + const tool = createWorkflowCreateTool(store); + const res = await run(tool, { name: "wf", ir: irWithFlags() }); + expect(res.isError).toBeFalsy(); + expect(captured.ir.nodes[0].config.cliSkipApproval).toBe(true); + expect(captured.ir.nodes[1].config.template.nodes[0].config.autoApprove).toBe(true); + const text = res.content.map((c) => c.text ?? "").join(""); + expect(text).not.toContain("approval-bypass flags removed"); + }); +}); diff --git a/packages/engine/src/agent-tools.ts b/packages/engine/src/agent-tools.ts index 13cd5c63ad..2c16e6285c 100644 --- a/packages/engine/src/agent-tools.ts +++ b/packages/engine/src/agent-tools.ts @@ -12,7 +12,7 @@ import { existsSync } from "node:fs"; import { createHash } from "node:crypto"; import { join, relative, resolve } from "node:path"; import type { AgentStore, AgentState, AgentCapability, AgentUpdateInput, TaskDocument, TaskDocumentCreateInput, TaskStore, RunMutationContext, MessageStore, Message, SourceType, Settings, ResearchRun, ResearchRunStatus, TaskCreateInput, ReflectionStore, ApprovalRequestStore, ProjectSettings, ChatStore } from "@fusion/core"; -import { listTraits, isBuiltinWorkflowId } from "@fusion/core"; +import { listTraits, isBuiltinWorkflowId, stripApprovalBypassFlags } from "@fusion/core"; import { promoteHeldTask } from "./hold-release.js"; import { DASHBOARD_USER_ID, canAgentTakeImplementationTaskForExplicitRouting, dailyMemoryPath, ensureOpenClawMemoryFiles, extractAgentProvisioningRequest, formatRoleMismatchReason, getMemoryBackendCapabilities, getProjectMemory, isEphemeralAgent, memoryLongTermPath, normalizeMessageParticipant, reconcileDeterministicDuplicate, resolveAgentProvisioningPolicy, resolveMemoryBackend, resolveResearchSettings, resolveTaskGithubTracking, resolveTitleSummarizerSettingsModel, runDeterministicDuplicateGuard, scheduleQmdProjectMemoryRefresh, searchProjectMemory, shouldSkipBackgroundQmdRefresh, summarizeTitle } from "@fusion/core"; import { ResearchOrchestrator } from "./research-orchestrator.js"; @@ -1201,7 +1201,10 @@ export function createTaskPromoteTool(store: TaskStore, currentTaskId: string): * Create a `fn_workflow_create` tool — a thin wrapper over the store's workflow * definition create. The IR is validated server-side; a malformed graph rejects. */ -export function createWorkflowCreateTool(store: TaskStore): ToolDefinition { +export function createWorkflowCreateTool( + store: TaskStore, + opts?: WorkflowAuthoringToolOptions, +): ToolDefinition { return { name: "fn_workflow_create", label: "Create Workflow", @@ -1226,16 +1229,23 @@ export function createWorkflowCreateTool(store: TaskStore): ToolDefinition { parameters: workflowCreateParams, execute: async (_id: string, params: Static) => { try { + let approvalNote = ""; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let ir = params.ir as any; + if (opts?.stripApprovalFlags) { + const result = stripApprovalBypassFlags(ir); + ir = result.ir; + if (result.stripped) approvalNote = " (approval-bypass flags removed)"; + } const created = await store.createWorkflowDefinition({ name: params.name, description: params.description, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ir: params.ir as any, + ir, // eslint-disable-next-line @typescript-eslint/no-explicit-any layout: params.layout as any, }); return { - content: [{ type: "text" as const, text: `Created workflow ${created.id} (${created.name}).` }], + content: [{ type: "text" as const, text: `Created workflow ${created.id} (${created.name}).${approvalNote}` }], details: { workflowId: created.id, name: created.name }, }; // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -1256,7 +1266,10 @@ export function createWorkflowCreateTool(store: TaskStore): ToolDefinition { * throws an OccupiedColumnsError; we surface it as a structured response carrying * the per-column occupant counts so the agent can retry with `rehome_to`. */ -export function createWorkflowUpdateTool(store: TaskStore): ToolDefinition { +export function createWorkflowUpdateTool( + store: TaskStore, + opts?: WorkflowAuthoringToolOptions, +): ToolDefinition { return { name: "fn_workflow_update", label: "Update Workflow", @@ -1270,17 +1283,24 @@ export function createWorkflowUpdateTool(store: TaskStore): ToolDefinition { parameters: workflowUpdateParams, execute: async (_id: string, params: Static) => { try { + let approvalNote = ""; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let ir = params.ir as any; + if (opts?.stripApprovalFlags && ir !== undefined && ir !== null) { + const result = stripApprovalBypassFlags(ir); + ir = result.ir; + if (result.stripped) approvalNote = " (approval-bypass flags removed)"; + } const updated = await store.updateWorkflowDefinition(params.workflow_id, { name: params.name, description: params.description, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ir: params.ir as any, + ir, // eslint-disable-next-line @typescript-eslint/no-explicit-any layout: params.layout as any, rehomeTo: params.rehome_to, }); return { - content: [{ type: "text" as const, text: `Updated workflow ${updated.id} (${updated.name}).` }], + content: [{ type: "text" as const, text: `Updated workflow ${updated.id} (${updated.name}).${approvalNote}` }], details: { workflowId: updated.id, name: updated.name }, }; // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -1410,16 +1430,30 @@ export function createTraitListTool(): ToolDefinition { * the default task for `fn_workflow_select`; lanes with no ambient task pass a * placeholder (an agent can still target any task via the `task_id` param). */ +/** + * Options for the workflow authoring tool set. + * + * `stripApprovalFlags` removes the `cliSkipApproval`/`autoApprove` approval- + * bypass flags from every node config (incl. nested foreach templates) before + * the create/update tools persist the IR. Chat and planning lanes are prompt- + * injectable, so they pass `true`; the executor lane (project-owner escape + * hatch) omits it so the dashboard editor can deliberately set those flags. + */ +export interface WorkflowAuthoringToolOptions { + stripApprovalFlags?: boolean; +} + export function createWorkflowAuthoringTools( store: TaskStore, currentTaskId: string, + opts?: WorkflowAuthoringToolOptions, ): ToolDefinition[] { return [ createWorkflowListTool(store), createWorkflowGetTool(store), createWorkflowSelectTool(store, currentTaskId), - createWorkflowCreateTool(store), - createWorkflowUpdateTool(store), + createWorkflowCreateTool(store, opts), + createWorkflowUpdateTool(store, opts), createWorkflowDeleteTool(store), createTraitListTool(), ]; diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 3a29414187..70a63311d3 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -4575,9 +4575,15 @@ export class TaskExecutor { // // SECURITY: both flags are intentional project-owner-only escape hatches. // They are only reachable by someone who can author/edit a workflow - // definition for this project — the same trust boundary that already - // lets them add named scripts. They are NOT untrusted-input surfaces, - // and neither is enforced at the IR-validation layer. + // definition for this project through the trusted dashboard editor / + // executor lane — the same trust boundary that already lets them add + // named scripts. They are NOT enforced at the IR-validation layer. + // Prompt-injectable surfaces strip these flags at the write boundary + // before persisting: the import / AI-design routes (stripApprovalFlags + // in register-workflow-routes.ts) and the chat/planning workflow + // authoring tools (createWorkflowAuthoringTools(..., {stripApprovalFlags: + // true}) in chat.ts / planning.ts) — all via stripApprovalBypassFlags in + // @fusion/core. Only the executor lane keeps these flags intact. const skipApproval = cfg.cliSkipApproval === true || cfg.autoApprove === true; if (!skipApproval && !(await this.store.isWorkflowCliCommandApproved(rawCommand))) { return this.pauseForCliApproval(node, live, rawCommand);