From 2d485cd47a539b083c460f88061fe584deaeb064 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 19 Feb 2026 14:23:19 +0100 Subject: [PATCH] refactor(security): extract safe-bin policy and dedupe tests --- src/agents/pi-tools.safe-bins.e2e.test.ts | 218 +++++++---------- src/infra/exec-approvals-allowlist.ts | 281 +--------------------- src/infra/exec-safe-bin-policy.ts | 272 +++++++++++++++++++++ 3 files changed, 365 insertions(+), 406 deletions(-) create mode 100644 src/infra/exec-safe-bin-policy.ts diff --git a/src/agents/pi-tools.safe-bins.e2e.test.ts b/src/agents/pi-tools.safe-bins.e2e.test.ts index 62487beb9c..f2d48e5314 100644 --- a/src/agents/pi-tools.safe-bins.e2e.test.ts +++ b/src/agents/pi-tools.safe-bins.e2e.test.ts @@ -67,40 +67,74 @@ vi.mock("../infra/exec-approvals.js", async (importOriginal) => { return { ...mod, resolveExecApprovals: () => approvals }; }); +type ExecToolResult = { + content: Array<{ type: string; text?: string }>; + details?: { status?: string }; +}; + +type ExecTool = { + execute( + callId: string, + params: { + command: string; + workdir: string; + env?: Record; + }, + ): Promise; +}; + +async function createSafeBinsExecTool(params: { + tmpPrefix: string; + safeBins: string[]; + files?: Array<{ name: string; contents: string }>; +}): Promise<{ tmpDir: string; execTool: ExecTool }> { + const { createOpenClawCodingTools } = await import("./pi-tools.js"); + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), params.tmpPrefix)); + for (const file of params.files ?? []) { + fs.writeFileSync(path.join(tmpDir, file.name), file.contents, "utf8"); + } + + const cfg: OpenClawConfig = { + tools: { + exec: { + host: "gateway", + security: "allowlist", + ask: "off", + safeBins: params.safeBins, + }, + }, + }; + + const tools = createOpenClawCodingTools({ + config: cfg, + sessionKey: "agent:main:main", + workspaceDir: tmpDir, + agentDir: path.join(tmpDir, "agent"), + }); + const execTool = tools.find((tool) => tool.name === "exec"); + if (!execTool) { + throw new Error("exec tool missing from coding tools"); + } + return { tmpDir, execTool: execTool as ExecTool }; +} + describe("createOpenClawCodingTools safeBins", () => { it("threads tools.exec.safeBins into exec allowlist checks", async () => { if (process.platform === "win32") { return; } - const { createOpenClawCodingTools } = await import("./pi-tools.js"); - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-")); - const cfg: OpenClawConfig = { - tools: { - exec: { - host: "gateway", - security: "allowlist", - ask: "off", - safeBins: ["echo"], - }, - }, - }; - - const tools = createOpenClawCodingTools({ - config: cfg, - sessionKey: "agent:main:main", - workspaceDir: tmpDir, - agentDir: path.join(tmpDir, "agent"), + const { tmpDir, execTool } = await createSafeBinsExecTool({ + tmpPrefix: "openclaw-safe-bins-", + safeBins: ["echo"], }); - const execTool = tools.find((tool) => tool.name === "exec"); - expect(execTool).toBeDefined(); const marker = `safe-bins-${Date.now()}`; const envSnapshot = captureEnv(["OPENCLAW_SHELL_ENV_TIMEOUT_MS"]); const result = await (async () => { try { process.env.OPENCLAW_SHELL_ENV_TIMEOUT_MS = "1000"; - return await execTool!.execute("call1", { + return await execTool.execute("call1", { command: `echo ${marker}`, workdir: tmpDir, }); @@ -120,33 +154,14 @@ describe("createOpenClawCodingTools safeBins", () => { return; } - const { createOpenClawCodingTools } = await import("./pi-tools.js"); - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-expand-")); - - fs.writeFileSync(path.join(tmpDir, "secret.txt"), "TOP_SECRET\n", "utf8"); - - const cfg: OpenClawConfig = { - tools: { - exec: { - host: "gateway", - security: "allowlist", - ask: "off", - safeBins: ["head", "wc"], - }, - }, - }; - - const tools = createOpenClawCodingTools({ - config: cfg, - sessionKey: "agent:main:main", - workspaceDir: tmpDir, - agentDir: path.join(tmpDir, "agent"), + const { tmpDir, execTool } = await createSafeBinsExecTool({ + tmpPrefix: "openclaw-safe-bins-expand-", + safeBins: ["head", "wc"], + files: [{ name: "secret.txt", contents: "TOP_SECRET\n" }], }); - const execTool = tools.find((tool) => tool.name === "exec"); - expect(execTool).toBeDefined(); await expect( - execTool!.execute("call1", { + execTool.execute("call1", { command: "head $FOO ; wc -l", workdir: tmpDir, env: { FOO: "secret.txt" }, @@ -159,33 +174,15 @@ describe("createOpenClawCodingTools safeBins", () => { return; } - const { createOpenClawCodingTools } = await import("./pi-tools.js"); - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-oracle-")); - fs.writeFileSync(path.join(tmpDir, "existing.txt"), "x\n", "utf8"); - - const cfg: OpenClawConfig = { - tools: { - exec: { - host: "gateway", - security: "allowlist", - ask: "off", - safeBins: ["sort"], - }, - }, - }; - - const tools = createOpenClawCodingTools({ - config: cfg, - sessionKey: "agent:main:main", - workspaceDir: tmpDir, - agentDir: path.join(tmpDir, "agent"), + const { tmpDir, execTool } = await createSafeBinsExecTool({ + tmpPrefix: "openclaw-safe-bins-oracle-", + safeBins: ["sort"], + files: [{ name: "existing.txt", contents: "x\n" }], }); - const execTool = tools.find((tool) => tool.name === "exec"); - expect(execTool).toBeDefined(); const run = async (command: string) => { try { - const result = await execTool!.execute("call-oracle", { command, workdir: tmpDir }); + const result = await execTool.execute("call-oracle", { command, workdir: tmpDir }); const text = result.content.find((content) => content.type === "text")?.text ?? ""; const resultDetails = result.details as { status?: string }; return { kind: "result" as const, status: resultDetails.status, text }; @@ -204,46 +201,25 @@ describe("createOpenClawCodingTools safeBins", () => { return; } - const { createOpenClawCodingTools } = await import("./pi-tools.js"); - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-sort-")); - - const cfg: OpenClawConfig = { - tools: { - exec: { - host: "gateway", - security: "allowlist", - ask: "off", - safeBins: ["sort"], - }, - }, - }; - - const tools = createOpenClawCodingTools({ - config: cfg, - sessionKey: "agent:main:main", - workspaceDir: tmpDir, - agentDir: path.join(tmpDir, "agent"), + const { tmpDir, execTool } = await createSafeBinsExecTool({ + tmpPrefix: "openclaw-safe-bins-sort-", + safeBins: ["sort"], }); - const execTool = tools.find((tool) => tool.name === "exec"); - expect(execTool).toBeDefined(); - const shortTarget = path.join(tmpDir, "blocked-short.txt"); - await expect( - execTool!.execute("call1", { - command: "sort -oblocked-short.txt", - workdir: tmpDir, - }), - ).rejects.toThrow("exec denied: allowlist miss"); - expect(fs.existsSync(shortTarget)).toBe(false); + const cases = [ + { command: "sort -oblocked-short.txt", target: "blocked-short.txt" }, + { command: "sort --output=blocked-long.txt", target: "blocked-long.txt" }, + ] as const; - const longTarget = path.join(tmpDir, "blocked-long.txt"); - await expect( - execTool!.execute("call2", { - command: "sort --output=blocked-long.txt", - workdir: tmpDir, - }), - ).rejects.toThrow("exec denied: allowlist miss"); - expect(fs.existsSync(longTarget)).toBe(false); + for (const [index, testCase] of cases.entries()) { + await expect( + execTool.execute(`call${index + 1}`, { + command: testCase.command, + workdir: tmpDir, + }), + ).rejects.toThrow("exec denied: allowlist miss"); + expect(fs.existsSync(path.join(tmpDir, testCase.target))).toBe(false); + } }); it("blocks grep recursive flags from reading cwd via safeBins", async () => { @@ -251,36 +227,14 @@ describe("createOpenClawCodingTools safeBins", () => { return; } - const { createOpenClawCodingTools } = await import("./pi-tools.js"); - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-safe-bins-grep-")); - fs.writeFileSync( - path.join(tmpDir, "secret.txt"), - "SAFE_BINS_RECURSIVE_SHOULD_NOT_LEAK\n", - "utf8", - ); - - const cfg: OpenClawConfig = { - tools: { - exec: { - host: "gateway", - security: "allowlist", - ask: "off", - safeBins: ["grep"], - }, - }, - }; - - const tools = createOpenClawCodingTools({ - config: cfg, - sessionKey: "agent:main:main", - workspaceDir: tmpDir, - agentDir: path.join(tmpDir, "agent"), + const { tmpDir, execTool } = await createSafeBinsExecTool({ + tmpPrefix: "openclaw-safe-bins-grep-", + safeBins: ["grep"], + files: [{ name: "secret.txt", contents: "SAFE_BINS_RECURSIVE_SHOULD_NOT_LEAK\n" }], }); - const execTool = tools.find((tool) => tool.name === "exec"); - expect(execTool).toBeDefined(); await expect( - execTool!.execute("call1", { + execTool.execute("call1", { command: "grep -R SAFE_BINS_RECURSIVE_SHOULD_NOT_LEAK", workdir: tmpDir, }), diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index b57f503de0..2872522f61 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -1,4 +1,5 @@ import path from "node:path"; +import type { ExecAllowlistEntry } from "./exec-approvals.js"; import { DEFAULT_SAFE_BINS, analyzeShellCommand, @@ -10,26 +11,13 @@ import { type CommandResolution, type ExecCommandSegment, } from "./exec-approvals-analysis.js"; -import type { ExecAllowlistEntry } from "./exec-approvals.js"; +import { + SAFE_BIN_GENERIC_PROFILE, + SAFE_BIN_PROFILES, + validateSafeBinArgv, +} from "./exec-safe-bin-policy.js"; import { isTrustedSafeBinPath } from "./exec-safe-bin-trust.js"; -function isPathLikeToken(value: string): boolean { - const trimmed = value.trim(); - if (!trimmed) { - return false; - } - if (trimmed === "-") { - return false; - } - if (trimmed.startsWith("./") || trimmed.startsWith("../") || trimmed.startsWith("~")) { - return true; - } - if (trimmed.startsWith("/")) { - return true; - } - return /^[A-Za-z]:[\\/]/.test(trimmed); -} - export function normalizeSafeBins(entries?: string[]): Set { if (!Array.isArray(entries)) { return new Set(); @@ -47,259 +35,6 @@ export function resolveSafeBins(entries?: string[] | null): Set { return normalizeSafeBins(entries ?? []); } -function hasGlobToken(value: string): boolean { - // Safe bins are stdin-only; globbing is both surprising and a historical bypass vector. - // Note: we still harden execution-time expansion separately. - return /[*?[\]]/.test(value); -} - -type SafeBinProfile = { - minPositional?: number; - maxPositional?: number; - valueFlags?: ReadonlySet; - blockedFlags?: ReadonlySet; -}; - -const NO_FLAGS = new Set(); -const SAFE_BIN_GENERIC_PROFILE: SafeBinProfile = {}; -const SAFE_BIN_PROFILES: Record = { - jq: { - maxPositional: 1, - valueFlags: new Set([ - "--arg", - "--argjson", - "--argstr", - "--argfile", - "--rawfile", - "--slurpfile", - "--from-file", - "--library-path", - "-L", - "-f", - ]), - blockedFlags: new Set([ - "--argfile", - "--rawfile", - "--slurpfile", - "--from-file", - "--library-path", - "-L", - "-f", - ]), - }, - grep: { - maxPositional: 1, - valueFlags: new Set([ - "--regexp", - "--file", - "--max-count", - "--after-context", - "--before-context", - "--context", - "--devices", - "--directories", - "--binary-files", - "--exclude", - "--exclude-from", - "--include", - "--label", - "-e", - "-f", - "-m", - "-A", - "-B", - "-C", - "-D", - "-d", - ]), - blockedFlags: new Set([ - "--file", - "--exclude-from", - "--dereference-recursive", - "--directories", - "--recursive", - "-f", - "-d", - "-r", - "-R", - ]), - }, - cut: { - maxPositional: 0, - valueFlags: new Set([ - "--bytes", - "--characters", - "--fields", - "--delimiter", - "--output-delimiter", - "-b", - "-c", - "-f", - "-d", - ]), - }, - sort: { - maxPositional: 0, - valueFlags: new Set([ - "--key", - "--field-separator", - "--buffer-size", - "--temporary-directory", - "--compress-program", - "--parallel", - "--batch-size", - "--random-source", - "--files0-from", - "--output", - "-k", - "-t", - "-S", - "-T", - "-o", - ]), - blockedFlags: new Set(["--files0-from", "--output", "-o"]), - }, - uniq: { - maxPositional: 0, - valueFlags: new Set([ - "--skip-fields", - "--skip-chars", - "--check-chars", - "--group", - "-f", - "-s", - "-w", - ]), - }, - head: { - maxPositional: 0, - valueFlags: new Set(["--lines", "--bytes", "-n", "-c"]), - }, - tail: { - maxPositional: 0, - valueFlags: new Set([ - "--lines", - "--bytes", - "--sleep-interval", - "--max-unchanged-stats", - "--pid", - "-n", - "-c", - ]), - }, - tr: { - minPositional: 1, - maxPositional: 2, - }, - wc: { - maxPositional: 0, - valueFlags: new Set(["--files0-from"]), - blockedFlags: new Set(["--files0-from"]), - }, -}; - -function isSafeLiteralToken(value: string): boolean { - if (!value || value === "-") { - return true; - } - return !hasGlobToken(value) && !isPathLikeToken(value); -} - -function validateSafeBinArgv(args: string[], profile: SafeBinProfile): boolean { - const valueFlags = profile.valueFlags ?? NO_FLAGS; - const blockedFlags = profile.blockedFlags ?? NO_FLAGS; - const positional: string[] = []; - - for (let i = 0; i < args.length; i += 1) { - const token = args[i]; - if (!token) { - continue; - } - if (token === "--") { - for (let j = i + 1; j < args.length; j += 1) { - const rest = args[j]; - if (!rest || rest === "-") { - continue; - } - if (!isSafeLiteralToken(rest)) { - return false; - } - positional.push(rest); - } - break; - } - if (token === "-") { - continue; - } - if (!token.startsWith("-")) { - if (!isSafeLiteralToken(token)) { - return false; - } - positional.push(token); - continue; - } - - if (token.startsWith("--")) { - const eqIndex = token.indexOf("="); - const flag = eqIndex > 0 ? token.slice(0, eqIndex) : token; - if (blockedFlags.has(flag)) { - return false; - } - if (eqIndex > 0) { - if (!isSafeLiteralToken(token.slice(eqIndex + 1))) { - return false; - } - continue; - } - if (!valueFlags.has(flag)) { - continue; - } - const value = args[i + 1]; - if (!value || !isSafeLiteralToken(value)) { - return false; - } - i += 1; - continue; - } - - let consumedValue = false; - for (let j = 1; j < token.length; j += 1) { - const flag = `-${token[j]}`; - if (blockedFlags.has(flag)) { - return false; - } - if (!valueFlags.has(flag)) { - continue; - } - const inlineValue = token.slice(j + 1); - if (inlineValue) { - if (!isSafeLiteralToken(inlineValue)) { - return false; - } - } else { - const value = args[i + 1]; - if (!value || !isSafeLiteralToken(value)) { - return false; - } - i += 1; - } - consumedValue = true; - break; - } - if (!consumedValue && hasGlobToken(token)) { - return false; - } - } - - const minPositional = profile.minPositional ?? 0; - if (positional.length < minPositional) { - return false; - } - if (typeof profile.maxPositional === "number" && positional.length > profile.maxPositional) { - return false; - } - return true; -} export function isSafeBinUsage(params: { argv: string[]; resolution: CommandResolution | null; @@ -321,9 +56,7 @@ export function isSafeBinUsage(params: { if (!execName) { return false; } - const matchesSafeBin = - params.safeBins.has(execName) || - (process.platform === "win32" && params.safeBins.has(path.parse(execName).name)); + const matchesSafeBin = params.safeBins.has(execName); if (!matchesSafeBin) { return false; } diff --git a/src/infra/exec-safe-bin-policy.ts b/src/infra/exec-safe-bin-policy.ts new file mode 100644 index 0000000000..5a6a7060a0 --- /dev/null +++ b/src/infra/exec-safe-bin-policy.ts @@ -0,0 +1,272 @@ +function isPathLikeToken(value: string): boolean { + const trimmed = value.trim(); + if (!trimmed) { + return false; + } + if (trimmed === "-") { + return false; + } + if (trimmed.startsWith("./") || trimmed.startsWith("../") || trimmed.startsWith("~")) { + return true; + } + if (trimmed.startsWith("/")) { + return true; + } + return /^[A-Za-z]:[\\/]/.test(trimmed); +} + +function hasGlobToken(value: string): boolean { + // Safe bins are stdin-only; globbing is both surprising and a historical bypass vector. + // Note: we still harden execution-time expansion separately. + return /[*?[\]]/.test(value); +} + +export type SafeBinProfile = { + minPositional?: number; + maxPositional?: number; + valueFlags?: ReadonlySet; + blockedFlags?: ReadonlySet; +}; + +const NO_FLAGS = new Set(); + +export const SAFE_BIN_GENERIC_PROFILE: SafeBinProfile = {}; + +export const SAFE_BIN_PROFILES: Record = { + jq: { + maxPositional: 1, + valueFlags: new Set([ + "--arg", + "--argjson", + "--argstr", + "--argfile", + "--rawfile", + "--slurpfile", + "--from-file", + "--library-path", + "-L", + "-f", + ]), + blockedFlags: new Set([ + "--argfile", + "--rawfile", + "--slurpfile", + "--from-file", + "--library-path", + "-L", + "-f", + ]), + }, + grep: { + maxPositional: 1, + valueFlags: new Set([ + "--regexp", + "--file", + "--max-count", + "--after-context", + "--before-context", + "--context", + "--devices", + "--directories", + "--binary-files", + "--exclude", + "--exclude-from", + "--include", + "--label", + "-e", + "-f", + "-m", + "-A", + "-B", + "-C", + "-D", + "-d", + ]), + blockedFlags: new Set([ + "--file", + "--exclude-from", + "--dereference-recursive", + "--directories", + "--recursive", + "-f", + "-d", + "-r", + "-R", + ]), + }, + cut: { + maxPositional: 0, + valueFlags: new Set([ + "--bytes", + "--characters", + "--fields", + "--delimiter", + "--output-delimiter", + "-b", + "-c", + "-f", + "-d", + ]), + }, + sort: { + maxPositional: 0, + valueFlags: new Set([ + "--key", + "--field-separator", + "--buffer-size", + "--temporary-directory", + "--compress-program", + "--parallel", + "--batch-size", + "--random-source", + "--files0-from", + "--output", + "-k", + "-t", + "-S", + "-T", + "-o", + ]), + blockedFlags: new Set(["--files0-from", "--output", "-o"]), + }, + uniq: { + maxPositional: 0, + valueFlags: new Set([ + "--skip-fields", + "--skip-chars", + "--check-chars", + "--group", + "-f", + "-s", + "-w", + ]), + }, + head: { + maxPositional: 0, + valueFlags: new Set(["--lines", "--bytes", "-n", "-c"]), + }, + tail: { + maxPositional: 0, + valueFlags: new Set([ + "--lines", + "--bytes", + "--sleep-interval", + "--max-unchanged-stats", + "--pid", + "-n", + "-c", + ]), + }, + tr: { + minPositional: 1, + maxPositional: 2, + }, + wc: { + maxPositional: 0, + valueFlags: new Set(["--files0-from"]), + blockedFlags: new Set(["--files0-from"]), + }, +}; + +function isSafeLiteralToken(value: string): boolean { + if (!value || value === "-") { + return true; + } + return !hasGlobToken(value) && !isPathLikeToken(value); +} + +export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): boolean { + const valueFlags = profile.valueFlags ?? NO_FLAGS; + const blockedFlags = profile.blockedFlags ?? NO_FLAGS; + const positional: string[] = []; + + for (let i = 0; i < args.length; i += 1) { + const token = args[i]; + if (!token) { + continue; + } + if (token === "--") { + for (let j = i + 1; j < args.length; j += 1) { + const rest = args[j]; + if (!rest || rest === "-") { + continue; + } + if (!isSafeLiteralToken(rest)) { + return false; + } + positional.push(rest); + } + break; + } + if (token === "-") { + continue; + } + if (!token.startsWith("-")) { + if (!isSafeLiteralToken(token)) { + return false; + } + positional.push(token); + continue; + } + + if (token.startsWith("--")) { + const eqIndex = token.indexOf("="); + const flag = eqIndex > 0 ? token.slice(0, eqIndex) : token; + if (blockedFlags.has(flag)) { + return false; + } + if (eqIndex > 0) { + if (!isSafeLiteralToken(token.slice(eqIndex + 1))) { + return false; + } + continue; + } + if (!valueFlags.has(flag)) { + continue; + } + const value = args[i + 1]; + if (!value || !isSafeLiteralToken(value)) { + return false; + } + i += 1; + continue; + } + + let consumedValue = false; + for (let j = 1; j < token.length; j += 1) { + const flag = `-${token[j]}`; + if (blockedFlags.has(flag)) { + return false; + } + if (!valueFlags.has(flag)) { + continue; + } + const inlineValue = token.slice(j + 1); + if (inlineValue) { + if (!isSafeLiteralToken(inlineValue)) { + return false; + } + } else { + const value = args[i + 1]; + if (!value || !isSafeLiteralToken(value)) { + return false; + } + i += 1; + } + consumedValue = true; + break; + } + if (!consumedValue && hasGlobToken(token)) { + return false; + } + } + + const minPositional = profile.minPositional ?? 0; + if (positional.length < minPositional) { + return false; + } + if (typeof profile.maxPositional === "number" && positional.length > profile.maxPositional) { + return false; + } + return true; +}