diff --git a/CHANGELOG.md b/CHANGELOG.md index 3658404b3..7c09f6607 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ Docs: https://docs.clawd.bot - Cron: cap reminder context history to 10 messages and honor `contextMessages`. (#1103) Thanks @mkbehr. - Exec approvals: treat main as the default agent + migrate legacy default allowlists. (#1417) Thanks @czekaj. - Exec: avoid defaulting to elevated mode when elevated is not allowed. +- Exec approvals: align node/gateway allowlist prechecks and approval gating; avoid null optional params in approval requests. (#1425) Thanks @czekaj. - UI: refresh debug panel on route-driven tab changes. (#1373) Thanks @yazinsai. ## 2026.1.21 diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index 54f6d7150..bd810ab3f 100644 --- a/src/agents/bash-tools.exec.approval-id.test.ts +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -68,4 +68,70 @@ describe("exec approvals", () => { const runId = (invokeParams as { params?: { runId?: string } } | undefined)?.params?.runId; expect(runId).toBe(approvalId); }); + + it("skips approval when node allowlist is satisfied", async () => { + const { callGatewayTool } = await import("./tools/gateway.js"); + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "clawdbot-test-bin-")); + const binDir = path.join(tempDir, "bin"); + await fs.mkdir(binDir, { recursive: true }); + const exeName = process.platform === "win32" ? "tool.cmd" : "tool"; + const exePath = path.join(binDir, exeName); + await fs.writeFile(exePath, ""); + if (process.platform !== "win32") { + await fs.chmod(exePath, 0o755); + } + const prevPath = process.env.PATH; + const prevPathExt = process.env.PATHEXT; + process.env.PATH = binDir; + if (process.platform === "win32") { + process.env.PATHEXT = ".CMD"; + } + + try { + const approvalsFile = { + version: 1, + defaults: { security: "allowlist", ask: "on-miss", askFallback: "deny" }, + agents: { + main: { + allowlist: [{ pattern: exePath }], + }, + }, + }; + + const calls: string[] = []; + vi.mocked(callGatewayTool).mockImplementation(async (method) => { + calls.push(method); + if (method === "exec.approvals.node.get") { + return { file: approvalsFile }; + } + if (method === "node.invoke") { + return { payload: { success: true, stdout: "ok" } }; + } + if (method === "exec.approval.request") { + return { decision: "allow-once" }; + } + return { ok: true }; + }); + + const { createExecTool } = await import("./bash-tools.exec.js"); + const tool = createExecTool({ + host: "node", + ask: "on-miss", + approvalRunningNoticeMs: 0, + }); + + const result = await tool.execute("call2", { command: `${exeName} --help` }); + expect(result.details.status).toBe("completed"); + expect(calls).toContain("exec.approvals.node.get"); + expect(calls).toContain("node.invoke"); + expect(calls).not.toContain("exec.approval.request"); + } finally { + process.env.PATH = prevPath; + if (prevPathExt === undefined) { + delete process.env.PATHEXT; + } else { + process.env.PATHEXT = prevPathExt; + } + } + }); }); diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index 79082e321..75f6b1927 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -8,16 +8,17 @@ import { type ExecAsk, type ExecHost, type ExecSecurity, - type ExecAllowlistEntry, + type ExecApprovalsFile, addAllowlistEntry, analyzeShellCommand, - isSafeBinUsage, - matchAllowlist, + evaluateExecAllowlist, maxAsk, minSecurity, + requiresExecApproval, resolveSafeBins, recordAllowlistUse, resolveExecApprovals, + resolveExecApprovalsFromFile, } from "../infra/exec-approvals.js"; import { requestHeartbeatNow } from "../infra/heartbeat-wake.js"; import { buildNodeShellCommand } from "../infra/node-shell.js"; @@ -870,7 +871,43 @@ export function createExecTool( if (nodeEnv) { applyPathPrepend(nodeEnv, defaultPathPrepend, { requireExisting: true }); } - const requiresAsk = hostAsk === "always" || hostAsk === "on-miss"; + const analysis = analyzeShellCommand({ command: params.command, cwd: workdir, env }); + let allowlistSatisfied = false; + if (hostAsk === "on-miss" && hostSecurity === "allowlist") { + try { + const approvalsSnapshot = (await callGatewayTool( + "exec.approvals.node.get", + { timeoutMs: 10_000 }, + { nodeId }, + )) as { file?: unknown } | null; + const approvalsFile = + approvalsSnapshot && typeof approvalsSnapshot === "object" + ? approvalsSnapshot.file + : undefined; + if (approvalsFile && typeof approvalsFile === "object") { + const resolved = resolveExecApprovalsFromFile({ + file: approvalsFile as ExecApprovalsFile, + agentId, + overrides: { security: "allowlist" }, + }); + // Allowlist-only precheck; safe bins are node-local and may diverge. + allowlistSatisfied = evaluateExecAllowlist({ + analysis, + allowlist: resolved.allowlist, + safeBins: new Set(), + cwd: workdir, + }).allowlistSatisfied; + } + } catch { + // Fall back to requiring approval if node approvals cannot be fetched. + } + } + const requiresAsk = requiresExecApproval({ + ask: hostAsk, + security: hostSecurity, + analysisOk: analysis.ok, + allowlistSatisfied, + }); const commandText = params.command; const invokeTimeoutMs = Math.max( 10_000, @@ -921,8 +958,8 @@ export function createExecTool( security: hostSecurity, ask: hostAsk, agentId, - resolvedPath: null, - sessionKey: defaults?.sessionKey ?? null, + resolvedPath: undefined, + sessionKey: defaults?.sessionKey, timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS, }, )) as { decision?: string } | null; @@ -1059,33 +1096,28 @@ export function createExecTool( throw new Error("exec denied: host=gateway security=deny"); } const analysis = analyzeShellCommand({ command: params.command, cwd: workdir, env }); - const allowlistMatches: ExecAllowlistEntry[] = []; - let allowlistSatisfied = false; - if (hostSecurity === "allowlist" && analysis.ok && analysis.segments.length > 0) { - allowlistSatisfied = analysis.segments.every((segment) => { - const match = matchAllowlist(approvals.allowlist, segment.resolution); - if (match) allowlistMatches.push(match); - const safe = isSafeBinUsage({ - argv: segment.argv, - resolution: segment.resolution, - safeBins, - cwd: workdir, - }); - return Boolean(match || safe); - }); - } - const requiresAsk = - hostAsk === "always" || - (hostAsk === "on-miss" && - hostSecurity === "allowlist" && - (!analysis.ok || !allowlistSatisfied)); + const allowlistEval = evaluateExecAllowlist({ + analysis, + allowlist: approvals.allowlist, + safeBins, + cwd: workdir, + }); + const allowlistMatches = allowlistEval.allowlistMatches; + const allowlistSatisfied = + hostSecurity === "allowlist" && analysis.ok ? allowlistEval.allowlistSatisfied : false; + const requiresAsk = requiresExecApproval({ + ask: hostAsk, + security: hostSecurity, + analysisOk: analysis.ok, + allowlistSatisfied, + }); if (requiresAsk) { const approvalId = crypto.randomUUID(); const approvalSlug = createApprovalSlug(approvalId); const expiresAtMs = Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS; const contextKey = `exec:${approvalId}`; - const resolvedPath = analysis.segments[0]?.resolution?.resolvedPath ?? null; + const resolvedPath = analysis.segments[0]?.resolution?.resolvedPath; const noticeSeconds = Math.max(1, Math.round(approvalRunningNoticeMs / 1000)); const commandText = params.command; const effectiveTimeout = @@ -1107,7 +1139,7 @@ export function createExecTool( ask: hostAsk, agentId, resolvedPath, - sessionKey: defaults?.sessionKey ?? null, + sessionKey: defaults?.sessionKey, timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS, }, )) as { decision?: string } | null; diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index f6d77b2f1..191cc08bc 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -7,11 +7,13 @@ import { describe, expect, it, vi } from "vitest"; import { analyzeArgvCommand, analyzeShellCommand, + evaluateExecAllowlist, isSafeBinUsage, matchAllowlist, maxAsk, minSecurity, normalizeSafeBins, + requiresExecApproval, resolveCommandResolution, resolveExecApprovals, resolveExecApprovalsFromFile, @@ -183,6 +185,85 @@ describe("exec approvals safe bins", () => { }); }); +describe("exec approvals allowlist evaluation", () => { + it("satisfies allowlist on exact match", () => { + const analysis = { + ok: true, + segments: [ + { + raw: "tool", + argv: ["tool"], + resolution: { + rawExecutable: "tool", + resolvedPath: "/usr/bin/tool", + executableName: "tool", + }, + }, + ], + }; + const allowlist: ExecAllowlistEntry[] = [{ pattern: "/usr/bin/tool" }]; + const result = evaluateExecAllowlist({ + analysis, + allowlist, + safeBins: new Set(), + cwd: "/tmp", + }); + expect(result.allowlistSatisfied).toBe(true); + expect(result.allowlistMatches.map((entry) => entry.pattern)).toEqual(["/usr/bin/tool"]); + }); + + it("satisfies allowlist via safe bins", () => { + const analysis = { + ok: true, + segments: [ + { + raw: "jq .foo", + argv: ["jq", ".foo"], + resolution: { + rawExecutable: "jq", + resolvedPath: "/usr/bin/jq", + executableName: "jq", + }, + }, + ], + }; + const result = evaluateExecAllowlist({ + analysis, + allowlist: [], + safeBins: normalizeSafeBins(["jq"]), + cwd: "/tmp", + }); + expect(result.allowlistSatisfied).toBe(true); + expect(result.allowlistMatches).toEqual([]); + }); + + it("satisfies allowlist via auto-allow skills", () => { + const analysis = { + ok: true, + segments: [ + { + raw: "skill-bin", + argv: ["skill-bin", "--help"], + resolution: { + rawExecutable: "skill-bin", + resolvedPath: "/opt/skills/skill-bin", + executableName: "skill-bin", + }, + }, + ], + }; + const result = evaluateExecAllowlist({ + analysis, + allowlist: [], + safeBins: new Set(), + skillBins: new Set(["skill-bin"]), + autoAllowSkills: true, + cwd: "/tmp", + }); + expect(result.allowlistSatisfied).toBe(true); + }); +}); + describe("exec approvals policy helpers", () => { it("minSecurity returns the more restrictive value", () => { expect(minSecurity("deny", "full")).toBe("deny"); @@ -193,6 +274,49 @@ describe("exec approvals policy helpers", () => { expect(maxAsk("off", "always")).toBe("always"); expect(maxAsk("on-miss", "off")).toBe("on-miss"); }); + + it("requiresExecApproval respects ask mode and allowlist satisfaction", () => { + expect( + requiresExecApproval({ + ask: "always", + security: "allowlist", + analysisOk: true, + allowlistSatisfied: true, + }), + ).toBe(true); + expect( + requiresExecApproval({ + ask: "off", + security: "allowlist", + analysisOk: true, + allowlistSatisfied: false, + }), + ).toBe(false); + expect( + requiresExecApproval({ + ask: "on-miss", + security: "allowlist", + analysisOk: true, + allowlistSatisfied: true, + }), + ).toBe(false); + expect( + requiresExecApproval({ + ask: "on-miss", + security: "allowlist", + analysisOk: false, + allowlistSatisfied: false, + }), + ).toBe(true); + expect( + requiresExecApproval({ + ask: "on-miss", + security: "full", + analysisOk: false, + allowlistSatisfied: false, + }), + ).toBe(false); + }); }); describe("exec approvals wildcard agent", () => { @@ -229,6 +353,82 @@ describe("exec approvals wildcard agent", () => { }); }); +describe("exec approvals node host allowlist check", () => { + // These tests verify the allowlist satisfaction logic used by the node host path + // The node host checks: matchAllowlist() || isSafeBinUsage() for each command segment + // Using hardcoded resolution objects for cross-platform compatibility + + it("satisfies allowlist when command matches exact path pattern", () => { + const resolution = { + rawExecutable: "python3", + resolvedPath: "/usr/bin/python3", + executableName: "python3", + }; + const entries: ExecAllowlistEntry[] = [{ pattern: "/usr/bin/python3" }]; + const match = matchAllowlist(entries, resolution); + expect(match).not.toBeNull(); + expect(match?.pattern).toBe("/usr/bin/python3"); + }); + + it("satisfies allowlist when command matches ** wildcard pattern", () => { + // Simulates symlink resolution: /opt/homebrew/bin/python3 -> /opt/homebrew/opt/python@3.14/bin/python3.14 + const resolution = { + rawExecutable: "python3", + resolvedPath: "/opt/homebrew/opt/python@3.14/bin/python3.14", + executableName: "python3.14", + }; + // Pattern with ** matches across multiple directories + const entries: ExecAllowlistEntry[] = [{ pattern: "/opt/**/python*" }]; + const match = matchAllowlist(entries, resolution); + expect(match?.pattern).toBe("/opt/**/python*"); + }); + + it("does not satisfy allowlist when command is not in allowlist", () => { + const resolution = { + rawExecutable: "unknown-tool", + resolvedPath: "/usr/local/bin/unknown-tool", + executableName: "unknown-tool", + }; + // Allowlist has different commands + const entries: ExecAllowlistEntry[] = [ + { pattern: "/usr/bin/python3" }, + { pattern: "/opt/**/node" }, + ]; + const match = matchAllowlist(entries, resolution); + expect(match).toBeNull(); + + // Also not a safe bin + const safe = isSafeBinUsage({ + argv: ["unknown-tool", "--help"], + resolution, + safeBins: normalizeSafeBins(["jq", "curl"]), + cwd: "/tmp", + }); + expect(safe).toBe(false); + }); + + it("satisfies via safeBins even when not in allowlist", () => { + const resolution = { + rawExecutable: "jq", + resolvedPath: "/usr/bin/jq", + executableName: "jq", + }; + // Not in allowlist + const entries: ExecAllowlistEntry[] = [{ pattern: "/usr/bin/python3" }]; + const match = matchAllowlist(entries, resolution); + expect(match).toBeNull(); + + // But is a safe bin with non-file args + const safe = isSafeBinUsage({ + argv: ["jq", ".foo"], + resolution, + safeBins: normalizeSafeBins(["jq"]), + cwd: "/tmp", + }); + expect(safe).toBe(true); + }); +}); + describe("exec approvals default agent migration", () => { it("migrates legacy default agent entries to main", () => { const file = { diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index b6d3549f4..7047abf89 100644 --- a/src/infra/exec-approvals.ts +++ b/src/infra/exec-approvals.ts @@ -727,6 +727,56 @@ export function isSafeBinUsage(params: { return true; } +export type ExecAllowlistEvaluation = { + allowlistSatisfied: boolean; + allowlistMatches: ExecAllowlistEntry[]; +}; + +export function evaluateExecAllowlist(params: { + analysis: ExecCommandAnalysis; + allowlist: ExecAllowlistEntry[]; + safeBins: Set; + cwd?: string; + skillBins?: Set; + autoAllowSkills?: boolean; +}): ExecAllowlistEvaluation { + const allowlistMatches: ExecAllowlistEntry[] = []; + if (!params.analysis.ok || params.analysis.segments.length === 0) { + return { allowlistSatisfied: false, allowlistMatches }; + } + const allowSkills = params.autoAllowSkills === true && (params.skillBins?.size ?? 0) > 0; + const allowlistSatisfied = params.analysis.segments.every((segment) => { + const match = matchAllowlist(params.allowlist, segment.resolution); + if (match) allowlistMatches.push(match); + const safe = isSafeBinUsage({ + argv: segment.argv, + resolution: segment.resolution, + safeBins: params.safeBins, + cwd: params.cwd, + }); + const skillAllow = + allowSkills && segment.resolution?.executableName + ? params.skillBins?.has(segment.resolution.executableName) + : false; + return Boolean(match || safe || skillAllow); + }); + return { allowlistSatisfied, allowlistMatches }; +} + +export function requiresExecApproval(params: { + ask: ExecAsk; + security: ExecSecurity; + analysisOk: boolean; + allowlistSatisfied: boolean; +}): boolean { + return ( + params.ask === "always" || + (params.ask === "on-miss" && + params.security === "allowlist" && + (!params.analysisOk || !params.allowlistSatisfied)) + ); +} + export function recordAllowlistUse( approvals: ExecApprovalsFile, agentId: string | undefined, diff --git a/src/node-host/runner.ts b/src/node-host/runner.ts index 0d4892e35..a66aa63f1 100644 --- a/src/node-host/runner.ts +++ b/src/node-host/runner.ts @@ -4,12 +4,11 @@ import fs from "node:fs"; import path from "node:path"; import { - type ExecAllowlistEntry, addAllowlistEntry, analyzeArgvCommand, analyzeShellCommand, - isSafeBinUsage, - matchAllowlist, + evaluateExecAllowlist, + requiresExecApproval, normalizeExecApprovals, recordAllowlistUse, resolveExecApprovals, @@ -592,26 +591,18 @@ async function handleInvoke( const cfg = loadConfig(); const agentExec = agentId ? resolveAgentConfig(cfg, agentId)?.tools?.exec : undefined; const safeBins = resolveSafeBins(agentExec?.safeBins ?? cfg.tools?.exec?.safeBins); - const allowlistMatches: ExecAllowlistEntry[] = []; const bins = autoAllowSkills ? await skillBins.current() : new Set(); + const allowlistEval = evaluateExecAllowlist({ + analysis, + allowlist: approvals.allowlist, + safeBins, + cwd: params.cwd ?? undefined, + skillBins: bins, + autoAllowSkills, + }); + const allowlistMatches = allowlistEval.allowlistMatches; const allowlistSatisfied = - security === "allowlist" && analysis.ok && analysis.segments.length > 0 - ? analysis.segments.every((segment) => { - const match = matchAllowlist(approvals.allowlist, segment.resolution); - if (match) allowlistMatches.push(match); - const safe = isSafeBinUsage({ - argv: segment.argv, - resolution: segment.resolution, - safeBins, - cwd: params.cwd ?? undefined, - }); - const skillAllow = - autoAllowSkills && segment.resolution?.executableName - ? bins.has(segment.resolution.executableName) - : false; - return Boolean(match || safe || skillAllow); - }) - : false; + security === "allowlist" && analysis.ok ? allowlistEval.allowlistSatisfied : false; const useMacAppExec = process.platform === "darwin"; if (useMacAppExec) { @@ -715,9 +706,12 @@ async function handleInvoke( return; } - const requiresAsk = - ask === "always" || - (ask === "on-miss" && security === "allowlist" && (!analysis.ok || !allowlistSatisfied)); + const requiresAsk = requiresExecApproval({ + ask, + security, + analysisOk: analysis.ok, + allowlistSatisfied, + }); const approvalDecision = params.approvalDecision === "allow-once" || params.approvalDecision === "allow-always"