From 10481097f8e6dd0346db9be0b5f27570e1bdfcfa Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 18:08:51 +0100 Subject: [PATCH] refactor(security): enforce v1 node exec approval binding --- CHANGELOG.md | 2 +- .../HostEnvSecurityPolicy.generated.swift | 2 +- package.json | 6 +- ...enerate-host-env-security-policy-swift.mjs | 37 +++- scripts/ghsa-patch.mjs | 168 ++++++++++++++++++ ...e-invoke-system-run-approval-match.test.ts | 86 ++------- .../node-invoke-system-run-approval-match.ts | 25 ++- .../node-invoke-system-run-approval.test.ts | 44 ++++- .../node-invoke-system-run-approval.ts | 2 - src/gateway/server-methods/exec-approval.ts | 30 ++-- .../server-methods/server-methods.test.ts | 22 ++- ...server.node-invoke-approval-bypass.test.ts | 2 + ...stem-run-approval-binding.contract.test.ts | 11 +- .../system-run-approval-binding.test.ts | 2 +- src/infra/exec-approvals.ts | 2 - .../system-run-approval-binding.ts | 55 +----- ...tem-run-approval-mismatch.contract.test.ts | 41 +++++ .../system-run-approval-binding-contract.json | 27 ++- ...system-run-approval-mismatch-contract.json | 67 +++++++ 19 files changed, 447 insertions(+), 184 deletions(-) create mode 100644 scripts/ghsa-patch.mjs rename src/{gateway => infra}/system-run-approval-binding.ts (76%) create mode 100644 src/infra/system-run-approval-mismatch.contract.test.ts create mode 100644 test/fixtures/system-run-approval-mismatch-contract.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 109d886fbfc..f442807f174 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,7 @@ Docs: https://docs.openclaw.ai - Security/Sandbox path alias guard: reject broken symlink targets by resolving through existing ancestors and failing closed on out-of-root targets, preventing workspace-only `apply_patch` writes from escaping sandbox/workspace boundaries via dangling symlinks. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting. - Security/Workspace FS boundary aliases: harden canonical boundary resolution for non-existent-leaf symlink aliases while preserving valid in-root aliases, preventing first-write workspace escapes via out-of-root symlink targets. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting. - Security/Config includes: harden `$include` file loading with verified-open reads, reject hardlinked include aliases, and enforce include file-size guardrails so config include resolution remains bounded to trusted in-root files. This ships in the next npm release (`2026.2.26`). Thanks @zpbrent for reporting. -- Security/Node exec approvals: bind `system.run` approvals to canonicalized env overrides (`envHash`/`envKeys`) and fail closed on env-binding mismatches/missing bindings, while adding `GIT_EXTERNAL_DIFF` to blocked host env keys. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting. +- Security/Node exec approvals: require structured `commandArgv` approvals for `host=node`, enforce versioned `systemRunBindingV1` matching for argv/cwd/session/agent/env context with fail-closed behavior on missing/mismatched bindings, and add `GIT_EXTERNAL_DIFF` to blocked host env keys. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting. - Security/Microsoft Teams media fetch: route Graph message/hosted-content/attachment fetches and auth-scope fallback attachment downloads through shared SSRF-guarded fetch paths, and centralize hostname-suffix allowlist policy helpers in the plugin SDK to remove channel/plugin drift. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting. - Microsoft Teams/File uploads: acknowledge `fileConsent/invoke` immediately (`invokeResponse` before upload + file card send) so Teams no longer shows false "Something went wrong" timeout banners while upload completion continues asynchronously; includes updated async regression coverage. Landed from contributor PR #27641 by @scz2011. - Security/Plugin channel HTTP auth: normalize protected `/api/channels` path checks against canonicalized request paths (case + percent-decoding + slash normalization), resolve encoded dot-segment traversal variants, and fail closed on malformed `%`-encoded channel prefixes so alternate-path variants cannot bypass gateway auth. This ships in the next npm release (`2026.2.26`). Thanks @zpbrent for reporting. diff --git a/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift b/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift index 0a94ce114f3..b126d03de21 100644 --- a/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift +++ b/apps/macos/Sources/OpenClaw/HostEnvSecurityPolicy.generated.swift @@ -1,6 +1,6 @@ // Generated file. Do not edit directly. // Source: src/infra/host-env-security-policy.json -// Regenerate: node scripts/generate-host-env-security-policy-swift.mjs +// Regenerate: node scripts/generate-host-env-security-policy-swift.mjs --write import Foundation diff --git a/package.json b/package.json index e9c9b4b796f..51c24aef7d5 100644 --- a/package.json +++ b/package.json @@ -54,8 +54,9 @@ "build": "pnpm canvas:a2ui:bundle && tsdown && pnpm build:plugin-sdk:dts && node --import tsx scripts/write-plugin-sdk-entry-dts.ts && node --import tsx scripts/canvas-a2ui-copy.ts && node --import tsx scripts/copy-hook-metadata.ts && node --import tsx scripts/copy-export-html-templates.ts && node --import tsx scripts/write-build-info.ts && node --import tsx scripts/write-cli-compat.ts", "build:plugin-sdk:dts": "tsc -p tsconfig.plugin-sdk.dts.json", "canvas:a2ui:bundle": "bash scripts/bundle-a2ui.sh", - "check": "pnpm format:check && pnpm tsgo && pnpm lint && pnpm lint:tmp:no-random-messaging && pnpm lint:tmp:channel-agnostic-boundaries && pnpm lint:tmp:no-raw-channel-fetch && pnpm lint:auth:no-pairing-store-group", + "check": "pnpm format:check && pnpm tsgo && pnpm lint && pnpm lint:tmp:no-random-messaging && pnpm lint:tmp:channel-agnostic-boundaries && pnpm lint:tmp:no-raw-channel-fetch && pnpm lint:auth:no-pairing-store-group && pnpm check:host-env-policy:swift", "check:docs": "pnpm format:docs:check && pnpm lint:docs && pnpm docs:check-links", + "check:host-env-policy:swift": "node scripts/generate-host-env-security-policy-swift.mjs --check", "check:loc": "node --import tsx scripts/check-ts-max-loc.ts --max 500", "deadcode:ci": "pnpm deadcode:report:ci:knip && pnpm deadcode:report:ci:ts-prune && pnpm deadcode:report:ci:ts-unused", "deadcode:knip": "pnpm dlx knip --no-progress", @@ -83,6 +84,8 @@ "gateway:dev": "OPENCLAW_SKIP_CHANNELS=1 CLAWDBOT_SKIP_CHANNELS=1 node scripts/run-node.mjs --dev gateway", "gateway:dev:reset": "OPENCLAW_SKIP_CHANNELS=1 CLAWDBOT_SKIP_CHANNELS=1 node scripts/run-node.mjs --dev gateway --reset", "gateway:watch": "node scripts/watch-node.mjs gateway --force", + "gen:host-env-policy:swift": "node scripts/generate-host-env-security-policy-swift.mjs --write", + "ghsa:patch": "node scripts/ghsa-patch.mjs", "ios:build": "bash -lc './scripts/ios-configure-signing.sh && cd apps/ios && xcodegen generate && xcodebuild -project OpenClaw.xcodeproj -scheme OpenClaw -destination \"${IOS_DEST:-platform=iOS Simulator,name=iPhone 17}\" -configuration Debug build'", "ios:gen": "bash -lc './scripts/ios-configure-signing.sh && cd apps/ios && xcodegen generate'", "ios:open": "bash -lc './scripts/ios-configure-signing.sh && cd apps/ios && xcodegen generate && open OpenClaw.xcodeproj'", @@ -133,6 +136,7 @@ "test:install:smoke": "bash scripts/test-install-sh-docker.sh", "test:live": "OPENCLAW_LIVE_TEST=1 CLAWDBOT_LIVE_TEST=1 vitest run --config vitest.live.config.ts", "test:macmini": "OPENCLAW_TEST_VM_FORKS=0 OPENCLAW_TEST_PROFILE=serial node scripts/test-parallel.mjs", + "test:sectriage": "pnpm exec vitest run --config vitest.gateway.config.ts && vitest run --config vitest.unit.config.ts --exclude src/daemon/launchd.integration.test.ts --exclude src/process/exec.test.ts", "test:ui": "pnpm lint:ui:no-raw-window-open && pnpm --dir ui test", "test:voicecall:closedloop": "vitest run extensions/voice-call/src/manager.test.ts extensions/voice-call/src/media-stream.test.ts src/plugins/voice-call.plugin.test.ts --maxWorkers=1", "test:watch": "vitest", diff --git a/scripts/generate-host-env-security-policy-swift.mjs b/scripts/generate-host-env-security-policy-swift.mjs index b8d496db6b8..4de64ad8d98 100644 --- a/scripts/generate-host-env-security-policy-swift.mjs +++ b/scripts/generate-host-env-security-policy-swift.mjs @@ -3,6 +3,15 @@ import fs from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; +const args = new Set(process.argv.slice(2)); +const checkOnly = args.has("--check"); +const writeMode = args.has("--write") || !checkOnly; + +if (checkOnly && args.has("--write")) { + console.error("Use either --check or --write, not both."); + process.exit(1); +} + const here = path.dirname(fileURLToPath(import.meta.url)); const repoRoot = path.resolve(here, ".."); const policyPath = path.join(repoRoot, "src", "infra", "host-env-security-policy.json"); @@ -20,9 +29,9 @@ const policy = JSON.parse(fs.readFileSync(policyPath, "utf8")); const renderSwiftStringArray = (items) => items.map((item) => ` "${item}"`).join(",\n"); -const swift = `// Generated file. Do not edit directly. +const generated = `// Generated file. Do not edit directly. // Source: src/infra/host-env-security-policy.json -// Regenerate: node scripts/generate-host-env-security-policy-swift.mjs +// Regenerate: node scripts/generate-host-env-security-policy-swift.mjs --write import Foundation @@ -41,5 +50,25 @@ ${renderSwiftStringArray(policy.blockedPrefixes)} } `; -fs.writeFileSync(outputPath, swift); -console.log(`Wrote ${path.relative(repoRoot, outputPath)}`); +const current = fs.existsSync(outputPath) ? fs.readFileSync(outputPath, "utf8") : null; + +if (checkOnly) { + if (current === generated) { + console.log(`OK ${path.relative(repoRoot, outputPath)}`); + process.exit(0); + } + console.error( + [ + `Out of date ${path.relative(repoRoot, outputPath)}.`, + "Run: node scripts/generate-host-env-security-policy-swift.mjs --write", + ].join("\n"), + ); + process.exit(1); +} + +if (writeMode) { + if (current !== generated) { + fs.writeFileSync(outputPath, generated); + } + console.log(`Wrote ${path.relative(repoRoot, outputPath)}`); +} diff --git a/scripts/ghsa-patch.mjs b/scripts/ghsa-patch.mjs new file mode 100644 index 00000000000..44e7daa2bee --- /dev/null +++ b/scripts/ghsa-patch.mjs @@ -0,0 +1,168 @@ +#!/usr/bin/env node +import { execFileSync, spawnSync } from "node:child_process"; +import crypto from "node:crypto"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +function usage() { + console.error( + [ + "Usage:", + " node scripts/ghsa-patch.mjs --ghsa [--repo owner/name]", + " --summary --severity ", + " --description-file ", + " --vulnerable-version-range ", + " --patched-versions ", + " [--package openclaw] [--ecosystem npm] [--cvss ]", + ].join("\n"), + ); +} + +function fail(message) { + console.error(message); + process.exit(1); +} + +function parseArgs(argv) { + const out = {}; + for (let i = 0; i < argv.length; i += 1) { + const arg = argv[i]; + if (!arg.startsWith("--")) { + fail(`Unexpected argument: ${arg}`); + } + const key = arg.slice(2); + const value = argv[i + 1]; + if (!value || value.startsWith("--")) { + fail(`Missing value for --${key}`); + } + out[key] = value; + i += 1; + } + return out; +} + +function runGh(args) { + const proc = spawnSync("gh", args, { encoding: "utf8" }); + if (proc.status !== 0) { + fail(proc.stderr.trim() || proc.stdout.trim() || `gh ${args.join(" ")} failed`); + } + return proc.stdout; +} + +function deriveRepoFromOrigin() { + const remote = execFileSync("git", ["remote", "get-url", "origin"], { encoding: "utf8" }).trim(); + const httpsMatch = remote.match(/github\.com[/:]([^/]+)\/([^/.]+)(?:\.git)?$/); + if (!httpsMatch) { + fail(`Could not parse origin remote: ${remote}`); + } + return `${httpsMatch[1]}/${httpsMatch[2]}`; +} + +function parseGhsaId(value) { + const match = value.match(/GHSA-[a-z0-9]{4}-[a-z0-9]{4}-[a-z0-9]{4}/i); + if (!match) { + fail(`Could not parse GHSA id from: ${value}`); + } + return match[0]; +} + +function writeTempJson(data) { + const file = path.join(os.tmpdir(), `ghsa-patch-${crypto.randomUUID()}.json`); + fs.writeFileSync(file, `${JSON.stringify(data, null, 2)}\n`); + return file; +} + +const args = parseArgs(process.argv.slice(2)); +if (!args.ghsa || !args.summary || !args.severity || !args["description-file"]) { + usage(); + process.exit(1); +} + +const repo = args.repo || deriveRepoFromOrigin(); +const ghsaId = parseGhsaId(args.ghsa); +const advisoryPath = `/repos/${repo}/security-advisories/${ghsaId}`; +const descriptionPath = path.resolve(args["description-file"]); + +if (!fs.existsSync(descriptionPath)) { + fail(`Description file does not exist: ${descriptionPath}`); +} + +const current = JSON.parse(runGh(["api", "-H", "X-GitHub-Api-Version: 2022-11-28", advisoryPath])); +const restoredCvss = args.cvss || current?.cvss?.vector_string || null; + +const ecosystem = args.ecosystem || "npm"; +const packageName = args.package || "openclaw"; +const vulnerableRange = args["vulnerable-version-range"]; +const patchedVersionsRaw = args["patched-versions"]; + +if (!vulnerableRange) { + fail("Missing --vulnerable-version-range"); +} +if (patchedVersionsRaw === undefined) { + fail("Missing --patched-versions"); +} + +const patchedVersions = patchedVersionsRaw === "null" ? null : patchedVersionsRaw; +const description = fs.readFileSync(descriptionPath, "utf8"); + +const payload = { + summary: args.summary, + severity: args.severity, + description, + vulnerabilities: [ + { + package: { + ecosystem, + name: packageName, + }, + vulnerable_version_range: vulnerableRange, + patched_versions: patchedVersions, + vulnerable_functions: [], + }, + ], +}; + +const patchFile = writeTempJson(payload); +runGh([ + "api", + "-H", + "X-GitHub-Api-Version: 2022-11-28", + "-X", + "PATCH", + advisoryPath, + "--input", + patchFile, +]); + +if (restoredCvss) { + runGh([ + "api", + "-H", + "X-GitHub-Api-Version: 2022-11-28", + "-X", + "PATCH", + advisoryPath, + "-f", + `cvss_vector_string=${restoredCvss}`, + ]); +} + +const refreshed = JSON.parse( + runGh(["api", "-H", "X-GitHub-Api-Version: 2022-11-28", advisoryPath]), +); +console.log( + JSON.stringify( + { + html_url: refreshed.html_url, + state: refreshed.state, + severity: refreshed.severity, + summary: refreshed.summary, + vulnerabilities: refreshed.vulnerabilities, + cvss: refreshed.cvss, + updated_at: refreshed.updated_at, + }, + null, + 2, + ), +); diff --git a/src/gateway/node-invoke-system-run-approval-match.test.ts b/src/gateway/node-invoke-system-run-approval-match.test.ts index 0312733f43a..9ba85d5350d 100644 --- a/src/gateway/node-invoke-system-run-approval-match.test.ts +++ b/src/gateway/node-invoke-system-run-approval-match.test.ts @@ -1,35 +1,11 @@ import { describe, expect, test } from "vitest"; +import { buildSystemRunApprovalBindingV1 } from "../infra/system-run-approval-binding.js"; import { evaluateSystemRunApprovalMatch } from "./node-invoke-system-run-approval-match.js"; -import { - buildSystemRunApprovalBindingV1, - buildSystemRunApprovalEnvBinding, -} from "./system-run-approval-binding.js"; describe("evaluateSystemRunApprovalMatch", () => { - test("matches legacy command text when binding fields match", () => { + test("rejects approvals that do not carry v1 binding", () => { const result = evaluateSystemRunApprovalMatch({ - cmdText: "echo SAFE", argv: ["echo", "SAFE"], - request: { - host: "node", - command: "echo SAFE", - cwd: "/tmp", - agentId: "agent-1", - sessionKey: "session-1", - }, - binding: { - cwd: "/tmp", - agentId: "agent-1", - sessionKey: "session-1", - }, - }); - expect(result).toEqual({ ok: true }); - }); - - test("rejects legacy command mismatch", () => { - const result = evaluateSystemRunApprovalMatch({ - cmdText: "echo PWNED", - argv: ["echo", "PWNED"], request: { host: "node", command: "echo SAFE", @@ -49,7 +25,6 @@ describe("evaluateSystemRunApprovalMatch", () => { test("enforces exact argv binding in v1 object", () => { const result = evaluateSystemRunApprovalMatch({ - cmdText: "echo SAFE", argv: ["echo", "SAFE"], request: { host: "node", @@ -72,7 +47,6 @@ describe("evaluateSystemRunApprovalMatch", () => { test("rejects argv mismatch in v1 object", () => { const result = evaluateSystemRunApprovalMatch({ - cmdText: "echo SAFE", argv: ["echo", "SAFE"], request: { host: "node", @@ -97,14 +71,18 @@ describe("evaluateSystemRunApprovalMatch", () => { expect(result.code).toBe("APPROVAL_REQUEST_MISMATCH"); }); - test("rejects env overrides when approval record lacks env binding", () => { + test("rejects env overrides when v1 binding has no env hash", () => { const result = evaluateSystemRunApprovalMatch({ - cmdText: "git diff", argv: ["git", "diff"], request: { host: "node", command: "git diff", - commandArgv: ["git", "diff"], + systemRunBindingV1: buildSystemRunApprovalBindingV1({ + argv: ["git", "diff"], + cwd: null, + agentId: null, + sessionKey: null, + }).binding, }, binding: { cwd: null, @@ -121,18 +99,18 @@ describe("evaluateSystemRunApprovalMatch", () => { }); test("accepts matching env hash with reordered keys", () => { - const envBinding = buildSystemRunApprovalEnvBinding({ - SAFE_A: "1", - SAFE_B: "2", - }); const result = evaluateSystemRunApprovalMatch({ - cmdText: "git diff", argv: ["git", "diff"], request: { host: "node", command: "git diff", - commandArgv: ["git", "diff"], - envHash: envBinding.envHash, + systemRunBindingV1: buildSystemRunApprovalBindingV1({ + argv: ["git", "diff"], + cwd: null, + agentId: null, + sessionKey: null, + env: { SAFE_A: "1", SAFE_B: "2" }, + }).binding, }, binding: { cwd: null, @@ -146,7 +124,6 @@ describe("evaluateSystemRunApprovalMatch", () => { test("rejects non-node host requests", () => { const result = evaluateSystemRunApprovalMatch({ - cmdText: "echo SAFE", argv: ["echo", "SAFE"], request: { host: "gateway", @@ -165,13 +142,11 @@ describe("evaluateSystemRunApprovalMatch", () => { expect(result.code).toBe("APPROVAL_REQUEST_MISMATCH"); }); - test("prefers v1 binding over legacy command text fields", () => { + test("uses v1 binding even when legacy command text diverges", () => { const result = evaluateSystemRunApprovalMatch({ - cmdText: "echo SAFE", argv: ["echo", "SAFE"], request: { host: "node", - // Intentionally stale legacy fields; v1 should be authoritative. command: "echo STALE", commandArgv: ["echo STALE"], systemRunBindingV1: buildSystemRunApprovalBindingV1({ @@ -189,31 +164,4 @@ describe("evaluateSystemRunApprovalMatch", () => { }); expect(result).toEqual({ ok: true }); }); - - test("rejects v1 mismatch even when legacy command text matches", () => { - const result = evaluateSystemRunApprovalMatch({ - cmdText: "echo SAFE", - argv: ["echo", "SAFE"], - request: { - host: "node", - command: "echo SAFE", - systemRunBindingV1: buildSystemRunApprovalBindingV1({ - argv: ["echo SAFE"], - cwd: null, - agentId: null, - sessionKey: null, - }).binding, - }, - binding: { - cwd: null, - agentId: null, - sessionKey: null, - }, - }); - expect(result.ok).toBe(false); - if (result.ok) { - throw new Error("unreachable"); - } - expect(result.code).toBe("APPROVAL_REQUEST_MISMATCH"); - }); }); diff --git a/src/gateway/node-invoke-system-run-approval-match.ts b/src/gateway/node-invoke-system-run-approval-match.ts index 567fd08e5b7..c67231f760c 100644 --- a/src/gateway/node-invoke-system-run-approval-match.ts +++ b/src/gateway/node-invoke-system-run-approval-match.ts @@ -1,10 +1,10 @@ import type { ExecApprovalRequestPayload } from "../infra/exec-approvals.js"; import { buildSystemRunApprovalBindingV1, - matchLegacySystemRunApprovalBinding, + missingSystemRunApprovalBindingV1, matchSystemRunApprovalBindingV1, type SystemRunApprovalMatchResult, -} from "./system-run-approval-binding.js"; +} from "../infra/system-run-approval-binding.js"; export type SystemRunApprovalBinding = { cwd: string | null; @@ -21,11 +21,10 @@ function requestMismatch(): SystemRunApprovalMatchResult { }; } -export { toSystemRunApprovalMismatchError } from "./system-run-approval-binding.js"; -export type { SystemRunApprovalMatchResult } from "./system-run-approval-binding.js"; +export { toSystemRunApprovalMismatchError } from "../infra/system-run-approval-binding.js"; +export type { SystemRunApprovalMatchResult } from "../infra/system-run-approval-binding.js"; export function evaluateSystemRunApprovalMatch(params: { - cmdText: string; argv: string[]; request: ExecApprovalRequestPayload; binding: SystemRunApprovalBinding; @@ -43,18 +42,14 @@ export function evaluateSystemRunApprovalMatch(params: { }); const expectedBinding = params.request.systemRunBindingV1; - if (expectedBinding) { - return matchSystemRunApprovalBindingV1({ - expected: expectedBinding, - actual: actualBinding.binding, + if (!expectedBinding) { + return missingSystemRunApprovalBindingV1({ actualEnvKeys: actualBinding.envKeys, }); } - - return matchLegacySystemRunApprovalBinding({ - request: params.request, - cmdText: params.cmdText, - argv: params.argv, - binding: params.binding, + return matchSystemRunApprovalBindingV1({ + expected: expectedBinding, + actual: actualBinding.binding, + actualEnvKeys: actualBinding.envKeys, }); } diff --git a/src/gateway/node-invoke-system-run-approval.test.ts b/src/gateway/node-invoke-system-run-approval.test.ts index 7437fb7ff58..1336e0fe009 100644 --- a/src/gateway/node-invoke-system-run-approval.test.ts +++ b/src/gateway/node-invoke-system-run-approval.test.ts @@ -1,7 +1,10 @@ import { describe, expect, test } from "vitest"; +import { + buildSystemRunApprovalBindingV1, + buildSystemRunApprovalEnvBinding, +} from "../infra/system-run-approval-binding.js"; import { ExecApprovalManager, type ExecApprovalRecord } from "./exec-approval-manager.js"; import { sanitizeSystemRunParamsForForwarding } from "./node-invoke-system-run-approval.js"; -import { buildSystemRunApprovalEnvBinding } from "./system-run-approval-binding.js"; describe("sanitizeSystemRunParamsForForwarding", () => { const now = Date.now(); @@ -14,7 +17,12 @@ describe("sanitizeSystemRunParamsForForwarding", () => { }, }; - function makeRecord(command: string, commandArgv?: string[]): ExecApprovalRecord { + function makeRecord( + command: string, + commandArgv?: string[], + bindingArgv?: string[], + ): ExecApprovalRecord { + const effectiveBindingArgv = bindingArgv ?? commandArgv ?? [command]; return { id: "approval-1", request: { @@ -22,6 +30,12 @@ describe("sanitizeSystemRunParamsForForwarding", () => { nodeId: "node-1", command, commandArgv, + systemRunBindingV1: buildSystemRunApprovalBindingV1({ + argv: effectiveBindingArgv, + cwd: null, + agentId: null, + sessionKey: null, + }).binding, cwd: null, agentId: null, sessionKey: null, @@ -97,7 +111,16 @@ describe("sanitizeSystemRunParamsForForwarding", () => { }, nodeId: "node-1", client, - execApprovalManager: manager(makeRecord("echo SAFE&&whoami")), + execApprovalManager: manager( + makeRecord("echo SAFE&&whoami", undefined, [ + "cmd.exe", + "/d", + "/s", + "/c", + "echo", + "SAFE&&whoami", + ]), + ), nowMs: now, }); expectAllowOnceForwardingResult(result); @@ -135,7 +158,13 @@ describe("sanitizeSystemRunParamsForForwarding", () => { nodeId: "node-1", client, execApprovalManager: manager( - makeRecord('/usr/bin/env BASH_ENV=/tmp/payload.sh bash -lc "echo SAFE"'), + makeRecord('/usr/bin/env BASH_ENV=/tmp/payload.sh bash -lc "echo SAFE"', undefined, [ + "/usr/bin/env", + "BASH_ENV=/tmp/payload.sh", + "bash", + "-lc", + "echo SAFE", + ]), ), nowMs: now, }); @@ -289,6 +318,13 @@ describe("sanitizeSystemRunParamsForForwarding", () => { host: "node", nodeId: "node-1", command: "echo SAFE", + commandArgv: ["echo", "SAFE"], + systemRunBindingV1: buildSystemRunApprovalBindingV1({ + argv: ["echo", "SAFE"], + cwd: null, + agentId: null, + sessionKey: null, + }).binding, cwd: null, agentId: null, sessionKey: null, diff --git a/src/gateway/node-invoke-system-run-approval.ts b/src/gateway/node-invoke-system-run-approval.ts index 6573025769e..fffca68574f 100644 --- a/src/gateway/node-invoke-system-run-approval.ts +++ b/src/gateway/node-invoke-system-run-approval.ts @@ -110,7 +110,6 @@ export function sanitizeSystemRunParamsForForwarding(opts: { details: cmdTextResolution.details, }; } - const cmdText = cmdTextResolution.cmdText; const approved = p.approved === true; const requestedDecision = normalizeApprovalDecision(p.approvalDecision); @@ -208,7 +207,6 @@ export function sanitizeSystemRunParamsForForwarding(opts: { } const approvalMatch = evaluateSystemRunApprovalMatch({ - cmdText, argv: cmdTextResolution.argv, request: snapshot.request, binding: { diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index 3898a07eee3..76806fb265d 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -3,6 +3,7 @@ import { DEFAULT_EXEC_APPROVAL_TIMEOUT_MS, type ExecApprovalDecision, } from "../../infra/exec-approvals.js"; +import { buildSystemRunApprovalBindingV1 } from "../../infra/system-run-approval-binding.js"; import type { ExecApprovalManager } from "../exec-approval-manager.js"; import { ErrorCodes, @@ -11,7 +12,6 @@ import { validateExecApprovalRequestParams, validateExecApprovalResolveParams, } from "../protocol/index.js"; -import { buildSystemRunApprovalBindingV1 } from "../system-run-approval-binding.js"; import type { GatewayRequestHandlers } from "./types.js"; export function createExecApprovalHandlers( @@ -70,16 +70,6 @@ export function createExecApprovalHandlers( const commandArgv = Array.isArray(p.commandArgv) ? p.commandArgv.map((entry) => String(entry)) : undefined; - const systemRunBindingV1 = - host === "node" && Array.isArray(commandArgv) && commandArgv.length > 0 - ? buildSystemRunApprovalBindingV1({ - argv: commandArgv, - cwd: p.cwd, - agentId: p.agentId, - sessionKey: p.sessionKey, - env: p.env, - }) - : null; if (host === "node" && !nodeId) { respond( false, @@ -88,6 +78,24 @@ export function createExecApprovalHandlers( ); return; } + if (host === "node" && (!Array.isArray(commandArgv) || commandArgv.length === 0)) { + respond( + false, + undefined, + errorShape(ErrorCodes.INVALID_REQUEST, "commandArgv is required for host=node"), + ); + return; + } + const systemRunBindingV1 = + host === "node" + ? buildSystemRunApprovalBindingV1({ + argv: commandArgv, + cwd: p.cwd, + agentId: p.agentId, + sessionKey: p.sessionKey, + env: p.env, + }) + : null; if (explicitId && manager.getSnapshot(explicitId)) { respond( false, diff --git a/src/gateway/server-methods/server-methods.test.ts b/src/gateway/server-methods/server-methods.test.ts index 3ed48c246c3..c43d9a5cdf5 100644 --- a/src/gateway/server-methods/server-methods.test.ts +++ b/src/gateway/server-methods/server-methods.test.ts @@ -6,10 +6,10 @@ import { fileURLToPath } from "node:url"; import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { emitAgentEvent } from "../../infra/agent-events.js"; import { formatZonedTimestamp } from "../../infra/format-time/format-datetime.js"; +import { buildSystemRunApprovalBindingV1 } from "../../infra/system-run-approval-binding.js"; import { resetLogger, setLoggerOverride } from "../../logging.js"; import { ExecApprovalManager } from "../exec-approval-manager.js"; import { validateExecApprovalRequestParams } from "../protocol/index.js"; -import { buildSystemRunApprovalBindingV1 } from "../system-run-approval-binding.js"; import { waitForAgentJob } from "./agent-job.js"; import { injectTimestamp, timestampOptsFromConfig } from "./agent-timestamp.js"; import { normalizeRpcAttachmentsToChatAttachments } from "./attachment-normalize.js"; @@ -248,6 +248,7 @@ describe("exec approval handlers", () => { const defaultExecApprovalRequestParams = { command: "echo ok", + commandArgv: ["echo", "ok"], cwd: "/tmp", nodeId: "node-1", host: "node", @@ -384,6 +385,25 @@ describe("exec approval handlers", () => { ); }); + it("rejects host=node approval requests without commandArgv", async () => { + const { handlers, respond, context } = createExecApprovalFixture(); + await requestExecApproval({ + handlers, + respond, + context, + params: { + commandArgv: undefined, + }, + }); + expect(respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ + message: "commandArgv is required for host=node", + }), + ); + }); + it("broadcasts request + resolve", async () => { const { handlers, broadcasts, respond, context } = createExecApprovalFixture(); diff --git a/src/gateway/server.node-invoke-approval-bypass.test.ts b/src/gateway/server.node-invoke-approval-bypass.test.ts index 26dc293789c..0e01a9619b9 100644 --- a/src/gateway/server.node-invoke-approval-bypass.test.ts +++ b/src/gateway/server.node-invoke-approval-bypass.test.ts @@ -75,9 +75,11 @@ async function requestAllowOnceApproval( nodeId: string, ): Promise { const approvalId = crypto.randomUUID(); + const commandArgv = command.split(/\s+/).filter((part) => part.length > 0); const requestP = rpcReq(ws, "exec.approval.request", { id: approvalId, command, + commandArgv, nodeId, cwd: null, host: "node", diff --git a/src/gateway/system-run-approval-binding.contract.test.ts b/src/gateway/system-run-approval-binding.contract.test.ts index ae29c258ae4..48976c3bdc5 100644 --- a/src/gateway/system-run-approval-binding.contract.test.ts +++ b/src/gateway/system-run-approval-binding.contract.test.ts @@ -3,11 +3,8 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { describe, expect, test } from "vitest"; import type { ExecApprovalRequestPayload } from "../infra/exec-approvals.js"; +import { buildSystemRunApprovalBindingV1 } from "../infra/system-run-approval-binding.js"; import { evaluateSystemRunApprovalMatch } from "./node-invoke-system-run-approval-match.js"; -import { - buildSystemRunApprovalBindingV1, - buildSystemRunApprovalEnvBinding, -} from "./system-run-approval-binding.js"; type FixtureCase = { name: string; @@ -25,10 +22,8 @@ type FixtureCase = { sessionKey?: string | null; env?: Record; }; - envHashFrom?: Record; }; invoke: { - cmdText: string; argv: string[]; binding: { cwd: string | null; @@ -71,9 +66,6 @@ function buildRequestPayload(entry: FixtureCase): ExecApprovalRequestPayload { env: entry.request.bindingV1.env, }).binding; } - if (entry.request.envHashFrom) { - payload.envHash = buildSystemRunApprovalEnvBinding(entry.request.envHashFrom).envHash; - } return payload; } @@ -81,7 +73,6 @@ describe("system-run approval binding contract fixtures", () => { for (const entry of fixture.cases) { test(entry.name, () => { const result = evaluateSystemRunApprovalMatch({ - cmdText: entry.invoke.cmdText, argv: entry.invoke.argv, request: buildRequestPayload(entry), binding: entry.invoke.binding, diff --git a/src/gateway/system-run-approval-binding.test.ts b/src/gateway/system-run-approval-binding.test.ts index 66dae690050..383b2895ffd 100644 --- a/src/gateway/system-run-approval-binding.test.ts +++ b/src/gateway/system-run-approval-binding.test.ts @@ -5,7 +5,7 @@ import { matchSystemRunApprovalBindingV1, matchSystemRunApprovalEnvHash, toSystemRunApprovalMismatchError, -} from "./system-run-approval-binding.js"; +} from "../infra/system-run-approval-binding.js"; describe("buildSystemRunApprovalEnvBinding", () => { test("normalizes keys and produces stable hash regardless of input order", () => { diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index 4bf6d62a4a8..f08998fc756 100644 --- a/src/infra/exec-approvals.ts +++ b/src/infra/exec-approvals.ts @@ -23,8 +23,6 @@ export type SystemRunApprovalBindingV1 = { export type ExecApprovalRequestPayload = { command: string; commandArgv?: string[]; - // Legacy env binding field (used for backward compatibility with old approvals). - envHash?: string | null; // Optional UI-safe env key preview for approval prompts. envKeys?: string[]; systemRunBindingV1?: SystemRunApprovalBindingV1 | null; diff --git a/src/gateway/system-run-approval-binding.ts b/src/infra/system-run-approval-binding.ts similarity index 76% rename from src/gateway/system-run-approval-binding.ts rename to src/infra/system-run-approval-binding.ts index d0cb59c7cd6..fbfb390167a 100644 --- a/src/gateway/system-run-approval-binding.ts +++ b/src/infra/system-run-approval-binding.ts @@ -1,9 +1,6 @@ import crypto from "node:crypto"; -import type { - ExecApprovalRequestPayload, - SystemRunApprovalBindingV1, -} from "../infra/exec-approvals.js"; -import { normalizeEnvVarKey } from "../infra/host-env-security.js"; +import type { SystemRunApprovalBindingV1 } from "./exec-approvals.js"; +import { normalizeEnvVarKey } from "./host-env-security.js"; type NormalizedSystemRunEnvEntry = [key: string, value: string]; @@ -89,14 +86,6 @@ function argvMatches(expectedArgv: string[], actualArgv: string[]): boolean { return true; } -function readExpectedEnvHash(request: Pick): string | null { - if (typeof request.envHash !== "string") { - return null; - } - const trimmed = request.envHash.trim(); - return trimmed ? trimmed : null; -} - export type SystemRunApprovalMatchResult = | { ok: true } | { @@ -180,42 +169,12 @@ export function matchSystemRunApprovalBindingV1(params: { }); } -export function matchLegacySystemRunApprovalBinding(params: { - request: Pick< - ExecApprovalRequestPayload, - "command" | "commandArgv" | "cwd" | "agentId" | "sessionKey" | "envHash" - >; - cmdText: string; - argv: string[]; - binding: { - cwd: string | null; - agentId: string | null; - sessionKey: string | null; - env?: unknown; - }; +export function missingSystemRunApprovalBindingV1(params: { + actualEnvKeys: string[]; }): SystemRunApprovalMatchResult { - const requestedArgv = params.request.commandArgv; - if (Array.isArray(requestedArgv)) { - if (!argvMatches(requestedArgv, params.argv)) { - return requestMismatch(); - } - } else if (!params.cmdText || params.request.command !== params.cmdText) { - return requestMismatch(); - } - if ((params.request.cwd ?? null) !== params.binding.cwd) { - return requestMismatch(); - } - if ((params.request.agentId ?? null) !== params.binding.agentId) { - return requestMismatch(); - } - if ((params.request.sessionKey ?? null) !== params.binding.sessionKey) { - return requestMismatch(); - } - const actualEnvBinding = buildSystemRunApprovalEnvBinding(params.binding.env); - return matchSystemRunApprovalEnvHash({ - expectedEnvHash: readExpectedEnvHash(params.request), - actualEnvHash: actualEnvBinding.envHash, - actualEnvKeys: actualEnvBinding.envKeys, + return requestMismatch({ + requiredBindingVersion: 1, + envKeys: params.actualEnvKeys, }); } diff --git a/src/infra/system-run-approval-mismatch.contract.test.ts b/src/infra/system-run-approval-mismatch.contract.test.ts new file mode 100644 index 00000000000..890e0de1bf9 --- /dev/null +++ b/src/infra/system-run-approval-mismatch.contract.test.ts @@ -0,0 +1,41 @@ +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, test } from "vitest"; +import { + toSystemRunApprovalMismatchError, + type SystemRunApprovalMatchResult, +} from "./system-run-approval-binding.js"; + +type FixtureCase = { + name: string; + runId: string; + match: Extract; + expected: { + ok: false; + message: string; + details: Record; + }; +}; + +type Fixture = { + cases: FixtureCase[]; +}; + +const fixturePath = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../test/fixtures/system-run-approval-mismatch-contract.json", +); +const fixture = JSON.parse(fs.readFileSync(fixturePath, "utf8")) as Fixture; + +describe("system-run approval mismatch contract fixtures", () => { + for (const entry of fixture.cases) { + test(entry.name, () => { + const result = toSystemRunApprovalMismatchError({ + runId: entry.runId, + match: entry.match, + }); + expect(result).toEqual(entry.expected); + }); + } +}); diff --git a/test/fixtures/system-run-approval-binding-contract.json b/test/fixtures/system-run-approval-binding-contract.json index b296898d06f..2a5a5ad55c2 100644 --- a/test/fixtures/system-run-approval-binding-contract.json +++ b/test/fixtures/system-run-approval-binding-contract.json @@ -14,7 +14,6 @@ } }, "invoke": { - "cmdText": "git diff", "argv": ["git", "diff"], "binding": { "cwd": null, @@ -39,7 +38,6 @@ } }, "invoke": { - "cmdText": "git diff", "argv": ["git", "diff"], "binding": { "cwd": null, @@ -63,7 +61,6 @@ } }, "invoke": { - "cmdText": "git diff", "argv": ["git", "diff"], "binding": { "cwd": null, @@ -75,14 +72,13 @@ "expected": { "ok": false, "code": "APPROVAL_ENV_BINDING_MISSING" } }, { - "name": "legacy rejects argv mismatch", + "name": "missing binding rejects requests even with matching argv", "request": { "host": "node", "command": "echo SAFE", - "commandArgv": ["echo SAFE"] + "commandArgv": ["echo", "SAFE"] }, "invoke": { - "cmdText": "echo SAFE", "argv": ["echo", "SAFE"], "binding": { "cwd": null, @@ -93,21 +89,24 @@ "expected": { "ok": false, "code": "APPROVAL_REQUEST_MISMATCH" } }, { - "name": "legacy accepts matching env hash", + "name": "v1 stays authoritative when legacy command text diverges", "request": { "host": "node", - "command": "git diff", - "commandArgv": ["git", "diff"], - "envHashFrom": { "SAFE_A": "1", "SAFE_B": "2" } + "command": "echo STALE", + "commandArgv": ["echo", "STALE"], + "bindingV1": { + "argv": ["echo", "SAFE"], + "cwd": null, + "agentId": null, + "sessionKey": null + } }, "invoke": { - "cmdText": "git diff", - "argv": ["git", "diff"], + "argv": ["echo", "SAFE"], "binding": { "cwd": null, "agentId": null, - "sessionKey": null, - "env": { "SAFE_B": "2", "SAFE_A": "1" } + "sessionKey": null } }, "expected": { "ok": true } diff --git a/test/fixtures/system-run-approval-mismatch-contract.json b/test/fixtures/system-run-approval-mismatch-contract.json new file mode 100644 index 00000000000..138751c68fb --- /dev/null +++ b/test/fixtures/system-run-approval-mismatch-contract.json @@ -0,0 +1,67 @@ +{ + "cases": [ + { + "name": "request mismatch preserves base details", + "runId": "approval-req-1", + "match": { + "ok": false, + "code": "APPROVAL_REQUEST_MISMATCH", + "message": "approval id does not match request" + }, + "expected": { + "ok": false, + "message": "approval id does not match request", + "details": { + "code": "APPROVAL_REQUEST_MISMATCH", + "runId": "approval-req-1" + } + } + }, + { + "name": "missing env binding keeps env key details", + "runId": "approval-env-missing", + "match": { + "ok": false, + "code": "APPROVAL_ENV_BINDING_MISSING", + "message": "approval id missing env binding for requested env overrides", + "details": { + "envKeys": ["GIT_EXTERNAL_DIFF"] + } + }, + "expected": { + "ok": false, + "message": "approval id missing env binding for requested env overrides", + "details": { + "code": "APPROVAL_ENV_BINDING_MISSING", + "runId": "approval-env-missing", + "envKeys": ["GIT_EXTERNAL_DIFF"] + } + } + }, + { + "name": "env mismatch preserves hash diagnostics", + "runId": "approval-env-mismatch", + "match": { + "ok": false, + "code": "APPROVAL_ENV_MISMATCH", + "message": "approval id env binding mismatch", + "details": { + "envKeys": ["SAFE_A"], + "expectedEnvHash": "expected-hash", + "actualEnvHash": "actual-hash" + } + }, + "expected": { + "ok": false, + "message": "approval id env binding mismatch", + "details": { + "code": "APPROVAL_ENV_MISMATCH", + "runId": "approval-env-mismatch", + "envKeys": ["SAFE_A"], + "expectedEnvHash": "expected-hash", + "actualEnvHash": "actual-hash" + } + } + } + ] +}