fix(gateway): guard node approval policy writes

src/gateway/server-methods/exec-approvals.test.ts

@@ -0,0 +1,183 @@
+import { describe, expect, it, vi } from "vitest";
+import { ErrorCodes } from "../../../packages/gateway-protocol/src/index.js";
+import type { ExecApprovalsFile } from "../../infra/exec-approvals.js";
+import { execApprovalsHandlers } from "./exec-approvals.js";
+import type { GatewayRequestContext, RespondFn } from "./types.js";
+
+const FILE: ExecApprovalsFile = {
+  version: 1,
+  defaults: {
+    security: "allowlist",
+    ask: "on-miss",
+  },
+  agents: {},
+};
+
+function createContext(invoke: ReturnType<typeof vi.fn>): GatewayRequestContext {
+  return {
+    nodeRegistry: {
+      invoke,
+    },
+  } as unknown as GatewayRequestContext;
+}
+
+async function callNodeSet(params: Record<string, unknown>, invoke: ReturnType<typeof vi.fn>) {
+  const respond = vi.fn() as unknown as RespondFn;
+  const handler = execApprovalsHandlers["exec.approvals.node.set"];
+  if (!handler) {
+    throw new Error("exec.approvals.node.set handler missing");
+  }
+  await handler({
+    req: { type: "req", id: "req-1", method: "exec.approvals.node.set", params },
+    params,
+    client: null,
+    isWebchatConnect: () => false,
+    respond,
+    context: createContext(invoke),
+  });
+  return respond as unknown as ReturnType<typeof vi.fn>;
+}
+
+describe("exec approval node methods", () => {
+  it("rejects stale node approval writes before forwarding the set command", async () => {
+    const invoke = vi.fn(async () => ({
+      ok: true,
+      payload: {
+        path: "node://node-1/exec-approvals",
+        exists: true,
+        hash: "fresh-hash",
+        file: FILE,
+      },
+    }));
+
+    const respond = await callNodeSet(
+      {
+        nodeId: "node-1",
+        file: FILE,
+        baseHash: "stale-hash",
+      },
+      invoke,
+    );
+
+    expect(invoke).toHaveBeenCalledTimes(1);
+    expect(invoke).toHaveBeenCalledWith({
+      nodeId: "node-1",
+      command: "system.execApprovals.get",
+      params: {},
+    });
+    expect(respond).toHaveBeenCalledWith(
+      false,
+      undefined,
+      expect.objectContaining({
+        code: ErrorCodes.INVALID_REQUEST,
+        message: expect.stringContaining("exec.approvals.node.get"),
+      }),
+    );
+  });
+
+  it("rejects stale native node approval writes that return baseHash without file metadata", async () => {
+    const invoke = vi.fn(async () => ({
+      ok: true,
+      payload: {
+        enabled: true,
+        defaultAction: "deny",
+        hash: "fresh-native-hash",
+        baseHash: "fresh-native-hash",
+        rules: [],
+      },
+    }));
+
+    const respond = await callNodeSet(
+      {
+        nodeId: "node-1",
+        file: FILE,
+        baseHash: "stale-native-hash",
+      },
+      invoke,
+    );
+
+    expect(invoke).toHaveBeenCalledTimes(1);
+    expect(respond).toHaveBeenCalledWith(
+      false,
+      undefined,
+      expect.objectContaining({
+        code: ErrorCodes.INVALID_REQUEST,
+        message: expect.stringContaining("exec.approvals.node.get"),
+      }),
+    );
+  });
+
+  it("requires a base hash when the node approval file already exists", async () => {
+    const invoke = vi.fn(async () => ({
+      ok: true,
+      payload: {
+        path: "node://node-1/exec-approvals",
+        exists: true,
+        hash: "fresh-hash",
+        file: FILE,
+      },
+    }));
+
+    const respond = await callNodeSet(
+      {
+        nodeId: "node-1",
+        file: FILE,
+      },
+      invoke,
+    );
+
+    expect(invoke).toHaveBeenCalledTimes(1);
+    expect(respond).toHaveBeenCalledWith(
+      false,
+      undefined,
+      expect.objectContaining({
+        code: ErrorCodes.INVALID_REQUEST,
+        message: expect.stringContaining("base hash required"),
+      }),
+    );
+  });
+
+  it("forwards matching node approval writes after a fresh snapshot read", async () => {
+    const nextPayload = {
+      path: "node://node-1/exec-approvals",
+      exists: true,
+      hash: "next-hash",
+      file: FILE,
+    };
+    const invoke = vi
+      .fn()
+      .mockResolvedValueOnce({
+        ok: true,
+        payload: {
+          path: "node://node-1/exec-approvals",
+          exists: true,
+          hash: "fresh-hash",
+          file: FILE,
+        },
+      })
+      .mockResolvedValueOnce({
+        ok: true,
+        payloadJSON: JSON.stringify(nextPayload),
+      });
+
+    const respond = await callNodeSet(
+      {
+        nodeId: "node-1",
+        file: FILE,
+        baseHash: "fresh-hash",
+      },
+      invoke,
+    );
+
+    expect(invoke).toHaveBeenCalledTimes(2);
+    expect(invoke).toHaveBeenNthCalledWith(2, {
+      nodeId: "node-1",
+      command: "system.execApprovals.set",
+      params: {
+        file: FILE,
+        baseHash: "fresh-hash",
+      },
+    });
+    expect(respond).toHaveBeenCalledWith(true, nextPayload, undefined);
+  });
+});

src/gateway/server-methods/exec-approvals.ts

@@ -28,6 +28,7 @@ function requireApprovalsBaseHash(
   params: unknown,
   snapshot: ExecApprovalsSnapshot,
   respond: RespondFn,
+  getMethod = "exec.approvals.get",
 ): boolean {
   // Approval allowlists are admin-editable state. Require the caller's last
   // observed hash before writing so stale UI tabs cannot overwrite changes.
@@ -40,7 +41,7 @@ function requireApprovalsBaseHash(
       undefined,
       errorShape(
         ErrorCodes.INVALID_REQUEST,
-        "exec approvals base hash unavailable; re-run exec.approvals.get and retry",
+        `exec approvals base hash unavailable; re-run ${getMethod} and retry`,
       ),
     );
     return false;
@@ -52,7 +53,7 @@ function requireApprovalsBaseHash(
       undefined,
       errorShape(
         ErrorCodes.INVALID_REQUEST,
-        "exec approvals base hash required; re-run exec.approvals.get and retry",
+        `exec approvals base hash required; re-run ${getMethod} and retry`,
       ),
     );
     return false;
@@ -63,7 +64,7 @@ function requireApprovalsBaseHash(
       undefined,
       errorShape(
         ErrorCodes.INVALID_REQUEST,
-        "exec approvals changed since last load; re-run exec.approvals.get and retry",
+        `exec approvals changed since last load; re-run ${getMethod} and retry`,
       ),
     );
     return false;
@@ -90,6 +91,44 @@ function toExecApprovalsPayload(snapshot: ExecApprovalsSnapshot) {
   };
 }
 
+function isRecord(value: unknown): value is Record<string, unknown> {
+  return typeof value === "object" && value !== null && !Array.isArray(value);
+}
+
+function readNodeExecApprovalsPayload(response: {
+  payload?: unknown;
+  payloadJSON?: string | null;
+}): unknown {
+  return response.payloadJSON ? safeParseJson(response.payloadJSON) : response.payload;
+}
+
+function toNodeExecApprovalsSnapshot(payload: unknown): ExecApprovalsSnapshot | null {
+  if (!isRecord(payload)) {
+    return null;
+  }
+  const payloadHash =
+    typeof payload.hash === "string"
+      ? payload.hash
+      : typeof payload.baseHash === "string"
+        ? payload.baseHash
+        : "";
+  const exists = typeof payload.exists === "boolean" ? payload.exists : Boolean(payloadHash);
+  if (!exists && !("exists" in payload)) {
+    return null;
+  }
+  const path = typeof payload.path === "string" && payload.path.trim() ? payload.path : "<node>";
+  const file: ExecApprovalsFile = isRecord(payload.file)
+    ? (payload.file as ExecApprovalsFile)
+    : { version: 1 };
+  return {
+    path,
+    exists,
+    raw: null,
+    file,
+    hash: payloadHash,
+  };
+}
+
 async function respondWithExecApprovalsNodePayload<TParams extends { nodeId: string }>(params: {
   method: string;
   rawParams: unknown;
@@ -167,24 +206,64 @@ export const execApprovalsHandlers: GatewayRequestHandlers = {
       commandParams: () => ({}),
       // Node invocations can return structured payloads or JSON strings
       // depending on the transport; normalize before echoing the RPC response.
-      readPayload: (res) => (res.payloadJSON ? safeParseJson(res.payloadJSON) : res.payload),
+      readPayload: readNodeExecApprovalsPayload,
     });
   },
   "exec.approvals.node.set": async ({ params, respond, context }) => {
-    await respondWithExecApprovalsNodePayload({
-      method: "exec.approvals.node.set",
-      rawParams: params,
-      validate: validateExecApprovalsNodeSetParams,
-      context,
-      respond,
-      command: "system.execApprovals.set",
-      commandParams: (parsedParams) => ({
-        file: parsedParams.file,
-        baseHash: parsedParams.baseHash,
-      }),
+    if (
+      !assertValidParams(
+        params,
+        validateExecApprovalsNodeSetParams,
+        "exec.approvals.node.set",
+        respond,
+      )
+    ) {
+      return;
+    }
+    const parsedParams = params;
+    const nodeId = parsedParams.nodeId.trim();
+    if (!nodeId) {
+      respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, "nodeId required"));
+      return;
+    }
+    await respondUnavailableOnThrow(respond, async () => {
+      const getResponse = await context.nodeRegistry.invoke({
+        nodeId,
+        command: "system.execApprovals.get",
+        params: {},
+      });
+      if (!respondUnavailableOnNodeInvokeError(respond, getResponse)) {
+        return;
+      }
+      const snapshot = toNodeExecApprovalsSnapshot(readNodeExecApprovalsPayload(getResponse));
+      if (!snapshot) {
+        respond(
+          false,
+          undefined,
+          errorShape(
+            ErrorCodes.UNAVAILABLE,
+            "node exec approvals snapshot unavailable; re-run exec.approvals.node.get and retry",
+          ),
+        );
+        return;
+      }
+      if (!requireApprovalsBaseHash(params, snapshot, respond, "exec.approvals.node.get")) {
+        return;
+      }
+      const setResponse = await context.nodeRegistry.invoke({
+        nodeId,
+        command: "system.execApprovals.set",
+        params: {
+          file: parsedParams.file,
+          baseHash: parsedParams.baseHash,
+        },
+      });
+      if (!respondUnavailableOnNodeInvokeError(respond, setResponse)) {
+        return;
+      }
       // node.set returns JSON on the command channel; keep the gateway response
       // shape aligned with local exec.approvals.set.
-      readPayload: (res) => safeParseJson(res.payloadJSON ?? null),
+      respond(true, safeParseJson(setResponse.payloadJSON ?? null), undefined);
     });
   },
 };