refactor(security): enforce v1 node exec approval binding

This commit is contained in:
Peter Steinberger
2026-02-26 18:08:51 +01:00
parent f4391c1725
commit 10481097f8
19 changed files with 447 additions and 184 deletions

View File

@@ -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");
});
});

View File

@@ -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,
});
}

View File

@@ -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,

View File

@@ -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: {

View File

@@ -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,

View File

@@ -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();

View File

@@ -75,9 +75,11 @@ async function requestAllowOnceApproval(
nodeId: string,
): Promise<string> {
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",

View File

@@ -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<string, string>;
};
envHashFrom?: Record<string, string>;
};
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,

View File

@@ -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", () => {

View File

@@ -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;

View File

@@ -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<ExecApprovalRequestPayload, "envHash">): 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,
});
}

View File

@@ -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<SystemRunApprovalMatchResult, { ok: false }>;
expected: {
ok: false;
message: string;
details: Record<string, unknown>;
};
};
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);
});
}
});