From 92eb3dfc9d25ca2600d6687cd025922193d3dba5 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 03:54:27 +0100 Subject: [PATCH] refactor(security): unify exec approval request matching --- src/gateway/exec-approval-manager.ts | 18 +--- ...e-invoke-system-run-approval-match.test.ts | 94 +++++++++++++++++++ .../node-invoke-system-run-approval-match.ts | 51 ++++++++++ .../node-invoke-system-run-approval.test.ts | 4 +- .../node-invoke-system-run-approval.ts | 61 +++--------- src/gateway/protocol/schema/exec-approvals.ts | 2 +- src/gateway/server-methods/exec-approval.ts | 4 +- src/infra/exec-approvals.ts | 26 ++--- 8 files changed, 182 insertions(+), 78 deletions(-) create mode 100644 src/gateway/node-invoke-system-run-approval-match.test.ts create mode 100644 src/gateway/node-invoke-system-run-approval-match.ts diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index 127d5feae09..320b4da0b1f 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -1,21 +1,13 @@ import { randomUUID } from "node:crypto"; -import type { ExecApprovalDecision } from "../infra/exec-approvals.js"; +import type { + ExecApprovalDecision, + ExecApprovalRequestPayload as InfraExecApprovalRequestPayload, +} from "../infra/exec-approvals.js"; // Grace period to keep resolved entries for late awaitDecision calls const RESOLVED_ENTRY_GRACE_MS = 15_000; -export type ExecApprovalRequestPayload = { - command: string; - commandArgv?: string[] | null; - cwd?: string | null; - nodeId?: string | null; - host?: string | null; - security?: string | null; - ask?: string | null; - agentId?: string | null; - resolvedPath?: string | null; - sessionKey?: string | null; -}; +export type ExecApprovalRequestPayload = InfraExecApprovalRequestPayload; export type ExecApprovalRecord = { id: string; diff --git a/src/gateway/node-invoke-system-run-approval-match.test.ts b/src/gateway/node-invoke-system-run-approval-match.test.ts new file mode 100644 index 00000000000..f5f093426c6 --- /dev/null +++ b/src/gateway/node-invoke-system-run-approval-match.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, test } from "vitest"; +import { approvalMatchesSystemRunRequest } from "./node-invoke-system-run-approval-match.js"; + +describe("approvalMatchesSystemRunRequest", () => { + test("matches legacy command text when binding fields match", () => { + const result = approvalMatchesSystemRunRequest({ + 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).toBe(true); + }); + + test("rejects legacy command mismatch", () => { + const result = approvalMatchesSystemRunRequest({ + cmdText: "echo PWNED", + argv: ["echo", "PWNED"], + request: { + host: "node", + command: "echo SAFE", + }, + binding: { + cwd: null, + agentId: null, + sessionKey: null, + }, + }); + expect(result).toBe(false); + }); + + test("enforces exact argv binding when commandArgv is set", () => { + const result = approvalMatchesSystemRunRequest({ + cmdText: "echo SAFE", + argv: ["echo", "SAFE"], + request: { + host: "node", + command: "echo SAFE", + commandArgv: ["echo", "SAFE"], + }, + binding: { + cwd: null, + agentId: null, + sessionKey: null, + }, + }); + expect(result).toBe(true); + }); + + test("rejects argv mismatch even when command text matches", () => { + const result = approvalMatchesSystemRunRequest({ + cmdText: "echo SAFE", + argv: ["echo", "SAFE"], + request: { + host: "node", + command: "echo SAFE", + commandArgv: ["echo SAFE"], + }, + binding: { + cwd: null, + agentId: null, + sessionKey: null, + }, + }); + expect(result).toBe(false); + }); + + test("rejects non-node host requests", () => { + const result = approvalMatchesSystemRunRequest({ + cmdText: "echo SAFE", + argv: ["echo", "SAFE"], + request: { + host: "gateway", + command: "echo SAFE", + }, + binding: { + cwd: null, + agentId: null, + sessionKey: null, + }, + }); + expect(result).toBe(false); + }); +}); diff --git a/src/gateway/node-invoke-system-run-approval-match.ts b/src/gateway/node-invoke-system-run-approval-match.ts new file mode 100644 index 00000000000..3dccc9b793d --- /dev/null +++ b/src/gateway/node-invoke-system-run-approval-match.ts @@ -0,0 +1,51 @@ +import type { ExecApprovalRequestPayload } from "../infra/exec-approvals.js"; + +export type SystemRunApprovalBinding = { + cwd: string | null; + agentId: string | null; + sessionKey: string | null; +}; + +function argvMatchesRequest(requestedArgv: string[], argv: string[]): boolean { + if (requestedArgv.length === 0 || requestedArgv.length !== argv.length) { + return false; + } + for (let i = 0; i < requestedArgv.length; i += 1) { + if (requestedArgv[i] !== argv[i]) { + return false; + } + } + return true; +} + +export function approvalMatchesSystemRunRequest(params: { + cmdText: string; + argv: string[]; + request: ExecApprovalRequestPayload; + binding: SystemRunApprovalBinding; +}): boolean { + if (params.request.host !== "node") { + return false; + } + + const requestedArgv = params.request.commandArgv; + if (Array.isArray(requestedArgv)) { + if (!argvMatchesRequest(requestedArgv, params.argv)) { + return false; + } + } else if (!params.cmdText || params.request.command !== params.cmdText) { + return false; + } + + if ((params.request.cwd ?? null) !== params.binding.cwd) { + return false; + } + if ((params.request.agentId ?? null) !== params.binding.agentId) { + return false; + } + if ((params.request.sessionKey ?? null) !== params.binding.sessionKey) { + return false; + } + + return true; +} diff --git a/src/gateway/node-invoke-system-run-approval.test.ts b/src/gateway/node-invoke-system-run-approval.test.ts index c2d3cbe1dfa..833bbf6f3cf 100644 --- a/src/gateway/node-invoke-system-run-approval.test.ts +++ b/src/gateway/node-invoke-system-run-approval.test.ts @@ -13,14 +13,14 @@ describe("sanitizeSystemRunParamsForForwarding", () => { }, }; - function makeRecord(command: string, commandArgv?: string[] | null): ExecApprovalRecord { + function makeRecord(command: string, commandArgv?: string[]): ExecApprovalRecord { return { id: "approval-1", request: { host: "node", nodeId: "node-1", command, - commandArgv: commandArgv ?? null, + commandArgv, 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 9623eb1b518..35cd18c66b9 100644 --- a/src/gateway/node-invoke-system-run-approval.ts +++ b/src/gateway/node-invoke-system-run-approval.ts @@ -1,5 +1,6 @@ import { resolveSystemRunCommand } from "../infra/system-run-command.js"; import type { ExecApprovalRecord } from "./exec-approval-manager.js"; +import { approvalMatchesSystemRunRequest } from "./node-invoke-system-run-approval-match.js"; type SystemRunParamsLike = { command?: unknown; @@ -53,53 +54,6 @@ function clientHasApprovals(client: ApprovalClient | null): boolean { return scopes.includes("operator.admin") || scopes.includes("operator.approvals"); } -function approvalMatchesRequest( - cmdText: string, - argv: string[], - params: SystemRunParamsLike, - record: ExecApprovalRecord, -): boolean { - if (record.request.host !== "node") { - return false; - } - - const requestedArgv = Array.isArray(record.request.commandArgv) - ? record.request.commandArgv - : null; - if (requestedArgv) { - if (requestedArgv.length === 0 || requestedArgv.length !== argv.length) { - return false; - } - for (let i = 0; i < requestedArgv.length; i += 1) { - if (requestedArgv[i] !== argv[i]) { - return false; - } - } - } else if (!cmdText || record.request.command !== cmdText) { - return false; - } - - const reqCwd = record.request.cwd ?? null; - const runCwd = normalizeString(params.cwd) ?? null; - if (reqCwd !== runCwd) { - return false; - } - - const reqAgentId = record.request.agentId ?? null; - const runAgentId = normalizeString(params.agentId) ?? null; - if (reqAgentId !== runAgentId) { - return false; - } - - const reqSessionKey = record.request.sessionKey ?? null; - const runSessionKey = normalizeString(params.sessionKey) ?? null; - if (reqSessionKey !== runSessionKey) { - return false; - } - - return true; -} - function pickSystemRunParams(raw: Record): Record { // Defensive allowlist: only forward fields that the node-host `system.run` handler understands. // This prevents future internal control fields from being smuggled through the gateway. @@ -250,7 +204,18 @@ export function sanitizeSystemRunParamsForForwarding(opts: { }; } - if (!approvalMatchesRequest(cmdText, cmdTextResolution.argv, p, snapshot)) { + if ( + !approvalMatchesSystemRunRequest({ + cmdText, + argv: cmdTextResolution.argv, + request: snapshot.request, + binding: { + cwd: normalizeString(p.cwd) ?? null, + agentId: normalizeString(p.agentId) ?? null, + sessionKey: normalizeString(p.sessionKey) ?? null, + }, + }) + ) { return { ok: false, message: "approval id does not match request", diff --git a/src/gateway/protocol/schema/exec-approvals.ts b/src/gateway/protocol/schema/exec-approvals.ts index 1482ae4cfef..083a445a4cf 100644 --- a/src/gateway/protocol/schema/exec-approvals.ts +++ b/src/gateway/protocol/schema/exec-approvals.ts @@ -89,7 +89,7 @@ export const ExecApprovalRequestParamsSchema = Type.Object( { id: Type.Optional(NonEmptyString), command: NonEmptyString, - commandArgv: Type.Optional(Type.Union([Type.Array(Type.String()), Type.Null()])), + commandArgv: Type.Optional(Type.Array(Type.String())), cwd: Type.Optional(Type.Union([Type.String(), Type.Null()])), nodeId: Type.Optional(Type.Union([NonEmptyString, Type.Null()])), host: Type.Optional(Type.Union([Type.String(), Type.Null()])), diff --git a/src/gateway/server-methods/exec-approval.ts b/src/gateway/server-methods/exec-approval.ts index 555348bc777..a9b3db150ce 100644 --- a/src/gateway/server-methods/exec-approval.ts +++ b/src/gateway/server-methods/exec-approval.ts @@ -43,7 +43,7 @@ export function createExecApprovalHandlers( const p = params as { id?: string; command: string; - commandArgv?: string[] | null; + commandArgv?: string[]; cwd?: string; nodeId?: string; host?: string; @@ -63,7 +63,7 @@ export function createExecApprovalHandlers( const nodeId = typeof p.nodeId === "string" ? p.nodeId.trim() : ""; const commandArgv = Array.isArray(p.commandArgv) ? p.commandArgv.map((entry) => String(entry)) - : null; + : undefined; if (host === "node" && !nodeId) { respond( false, diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index 688972d8361..d78f3d137e9 100644 --- a/src/infra/exec-approvals.ts +++ b/src/infra/exec-approvals.ts @@ -11,20 +11,22 @@ export type ExecHost = "sandbox" | "gateway" | "node"; export type ExecSecurity = "deny" | "allowlist" | "full"; export type ExecAsk = "off" | "on-miss" | "always"; +export type ExecApprovalRequestPayload = { + command: string; + commandArgv?: string[]; + cwd?: string | null; + nodeId?: string | null; + host?: string | null; + security?: string | null; + ask?: string | null; + agentId?: string | null; + resolvedPath?: string | null; + sessionKey?: string | null; +}; + export type ExecApprovalRequest = { id: string; - request: { - command: string; - commandArgv?: string[] | null; - cwd?: string | null; - nodeId?: string | null; - host?: string | null; - security?: string | null; - ask?: string | null; - agentId?: string | null; - resolvedPath?: string | null; - sessionKey?: string | null; - }; + request: ExecApprovalRequestPayload; createdAtMs: number; expiresAtMs: number; };