fix: harden connect auth flow and exec policy diagnostics

src/node-host/exec-policy.test.ts

@@ -0,0 +1,148 @@
+import { describe, expect, it } from "vitest";
+import {
+  evaluateSystemRunPolicy,
+  formatSystemRunAllowlistMissMessage,
+  resolveExecApprovalDecision,
+} from "./exec-policy.js";
+
+describe("resolveExecApprovalDecision", () => {
+  it("accepts known approval decisions", () => {
+    expect(resolveExecApprovalDecision("allow-once")).toBe("allow-once");
+    expect(resolveExecApprovalDecision("allow-always")).toBe("allow-always");
+  });
+
+  it("normalizes unknown approval decisions to null", () => {
+    expect(resolveExecApprovalDecision("deny")).toBeNull();
+    expect(resolveExecApprovalDecision(undefined)).toBeNull();
+  });
+});
+
+describe("formatSystemRunAllowlistMissMessage", () => {
+  it("returns legacy allowlist miss message by default", () => {
+    expect(formatSystemRunAllowlistMissMessage()).toBe("SYSTEM_RUN_DENIED: allowlist miss");
+  });
+
+  it("adds Windows shell-wrapper guidance when blocked by cmd.exe policy", () => {
+    expect(
+      formatSystemRunAllowlistMissMessage({
+        windowsShellWrapperBlocked: true,
+      }),
+    ).toContain("Windows shell wrappers like cmd.exe /c require approval");
+  });
+});
+
+describe("evaluateSystemRunPolicy", () => {
+  it("denies when security mode is deny", () => {
+    const decision = evaluateSystemRunPolicy({
+      security: "deny",
+      ask: "off",
+      analysisOk: true,
+      allowlistSatisfied: true,
+      approvalDecision: null,
+      approved: false,
+      isWindows: false,
+      cmdInvocation: false,
+    });
+    expect(decision.allowed).toBe(false);
+    if (decision.allowed) {
+      throw new Error("expected denied decision");
+    }
+    expect(decision.eventReason).toBe("security=deny");
+    expect(decision.errorMessage).toBe("SYSTEM_RUN_DISABLED: security=deny");
+  });
+
+  it("requires approval when ask policy requires it", () => {
+    const decision = evaluateSystemRunPolicy({
+      security: "allowlist",
+      ask: "always",
+      analysisOk: true,
+      allowlistSatisfied: true,
+      approvalDecision: null,
+      approved: false,
+      isWindows: false,
+      cmdInvocation: false,
+    });
+    expect(decision.allowed).toBe(false);
+    if (decision.allowed) {
+      throw new Error("expected denied decision");
+    }
+    expect(decision.eventReason).toBe("approval-required");
+    expect(decision.requiresAsk).toBe(true);
+  });
+
+  it("allows allowlist miss when explicit approval is provided", () => {
+    const decision = evaluateSystemRunPolicy({
+      security: "allowlist",
+      ask: "on-miss",
+      analysisOk: false,
+      allowlistSatisfied: false,
+      approvalDecision: "allow-once",
+      approved: false,
+      isWindows: false,
+      cmdInvocation: false,
+    });
+    expect(decision.allowed).toBe(true);
+    if (!decision.allowed) {
+      throw new Error("expected allowed decision");
+    }
+    expect(decision.approvedByAsk).toBe(true);
+  });
+
+  it("denies allowlist misses without approval", () => {
+    const decision = evaluateSystemRunPolicy({
+      security: "allowlist",
+      ask: "off",
+      analysisOk: false,
+      allowlistSatisfied: false,
+      approvalDecision: null,
+      approved: false,
+      isWindows: false,
+      cmdInvocation: false,
+    });
+    expect(decision.allowed).toBe(false);
+    if (decision.allowed) {
+      throw new Error("expected denied decision");
+    }
+    expect(decision.eventReason).toBe("allowlist-miss");
+    expect(decision.errorMessage).toBe("SYSTEM_RUN_DENIED: allowlist miss");
+  });
+
+  it("treats Windows cmd.exe wrappers as allowlist misses", () => {
+    const decision = evaluateSystemRunPolicy({
+      security: "allowlist",
+      ask: "off",
+      analysisOk: true,
+      allowlistSatisfied: true,
+      approvalDecision: null,
+      approved: false,
+      isWindows: true,
+      cmdInvocation: true,
+    });
+    expect(decision.allowed).toBe(false);
+    if (decision.allowed) {
+      throw new Error("expected denied decision");
+    }
+    expect(decision.windowsShellWrapperBlocked).toBe(true);
+    expect(decision.errorMessage).toContain("Windows shell wrappers like cmd.exe /c");
+  });
+
+  it("allows execution when policy checks pass", () => {
+    const decision = evaluateSystemRunPolicy({
+      security: "allowlist",
+      ask: "on-miss",
+      analysisOk: true,
+      allowlistSatisfied: true,
+      approvalDecision: null,
+      approved: false,
+      isWindows: false,
+      cmdInvocation: false,
+    });
+    expect(decision.allowed).toBe(true);
+    if (!decision.allowed) {
+      throw new Error("expected allowed decision");
+    }
+    expect(decision.requiresAsk).toBe(false);
+    expect(decision.analysisOk).toBe(true);
+    expect(decision.allowlistSatisfied).toBe(true);
+  });
+});

src/node-host/exec-policy.ts

@@ -0,0 +1,116 @@
+import { requiresExecApproval, type ExecAsk, type ExecSecurity } from "../infra/exec-approvals.js";
+
+export type ExecApprovalDecision = "allow-once" | "allow-always" | null;
+
+export type SystemRunPolicyDecision = {
+  analysisOk: boolean;
+  allowlistSatisfied: boolean;
+  windowsShellWrapperBlocked: boolean;
+  requiresAsk: boolean;
+  approvalDecision: ExecApprovalDecision;
+  approvedByAsk: boolean;
+} & (
+  | {
+      allowed: true;
+    }
+  | {
+      allowed: false;
+      eventReason: "security=deny" | "approval-required" | "allowlist-miss";
+      errorMessage: string;
+    }
+);
+
+export function resolveExecApprovalDecision(value: unknown): ExecApprovalDecision {
+  if (value === "allow-once" || value === "allow-always") {
+    return value;
+  }
+  return null;
+}
+
+export function formatSystemRunAllowlistMissMessage(params?: {
+  windowsShellWrapperBlocked?: boolean;
+}): string {
+  if (params?.windowsShellWrapperBlocked) {
+    return (
+      "SYSTEM_RUN_DENIED: allowlist miss " +
+      "(Windows shell wrappers like cmd.exe /c require approval; " +
+      "approve once/always or run with --ask on-miss|always)"
+    );
+  }
+  return "SYSTEM_RUN_DENIED: allowlist miss";
+}
+
+export function evaluateSystemRunPolicy(params: {
+  security: ExecSecurity;
+  ask: ExecAsk;
+  analysisOk: boolean;
+  allowlistSatisfied: boolean;
+  approvalDecision: ExecApprovalDecision;
+  approved?: boolean;
+  isWindows: boolean;
+  cmdInvocation: boolean;
+}): SystemRunPolicyDecision {
+  const windowsShellWrapperBlocked =
+    params.security === "allowlist" && params.isWindows && params.cmdInvocation;
+  const analysisOk = windowsShellWrapperBlocked ? false : params.analysisOk;
+  const allowlistSatisfied = windowsShellWrapperBlocked ? false : params.allowlistSatisfied;
+  const approvedByAsk = params.approvalDecision !== null || params.approved === true;
+
+  if (params.security === "deny") {
+    return {
+      allowed: false,
+      eventReason: "security=deny",
+      errorMessage: "SYSTEM_RUN_DISABLED: security=deny",
+      analysisOk,
+      allowlistSatisfied,
+      windowsShellWrapperBlocked,
+      requiresAsk: false,
+      approvalDecision: params.approvalDecision,
+      approvedByAsk,
+    };
+  }
+
+  const requiresAsk = requiresExecApproval({
+    ask: params.ask,
+    security: params.security,
+    analysisOk,
+    allowlistSatisfied,
+  });
+  if (requiresAsk && !approvedByAsk) {
+    return {
+      allowed: false,
+      eventReason: "approval-required",
+      errorMessage: "SYSTEM_RUN_DENIED: approval required",
+      analysisOk,
+      allowlistSatisfied,
+      windowsShellWrapperBlocked,
+      requiresAsk,
+      approvalDecision: params.approvalDecision,
+      approvedByAsk,
+    };
+  }
+
+  if (params.security === "allowlist" && (!analysisOk || !allowlistSatisfied) && !approvedByAsk) {
+    return {
+      allowed: false,
+      eventReason: "allowlist-miss",
+      errorMessage: formatSystemRunAllowlistMissMessage({ windowsShellWrapperBlocked }),
+      analysisOk,
+      allowlistSatisfied,
+      windowsShellWrapperBlocked,
+      requiresAsk,
+      approvalDecision: params.approvalDecision,
+      approvedByAsk,
+    };
+  }
+
+  return {
+    allowed: true,
+    analysisOk,
+    allowlistSatisfied,
+    windowsShellWrapperBlocked,
+    requiresAsk,
+    approvalDecision: params.approvalDecision,
+    approvedByAsk,
+  };
+}

src/node-host/invoke-system-run.ts

@@ -8,7 +8,6 @@ import {
   evaluateExecAllowlist,
   evaluateShellAllowlist,
   recordAllowlistUse,
-  requiresExecApproval,
   resolveAllowAlwaysPatterns,
   resolveExecApprovals,
   type ExecAllowlistEntry,
@@ -20,6 +19,7 @@ import type { ExecHostRequest, ExecHostResponse, ExecHostRunResult } from "../in
 import { resolveExecSafeBinRuntimePolicy } from "../infra/exec-safe-bin-runtime-policy.js";
 import { sanitizeSystemRunEnvOverrides } from "../infra/host-env-security.js";
 import { resolveSystemRunCommand } from "../infra/system-run-command.js";
+import { evaluateSystemRunPolicy, resolveExecApprovalDecision } from "./exec-policy.js";
 import type {
   ExecEventPayload,
   RunResult,
@@ -32,19 +32,7 @@ type SystemRunInvokeResult = {
   payloadJSON?: string | null;
   error?: { code?: string; message?: string } | null;
 };
-
-export function formatSystemRunAllowlistMissMessage(params?: {
-  windowsShellWrapperBlocked?: boolean;
-}): string {
-  if (params?.windowsShellWrapperBlocked) {
-    return (
-      "SYSTEM_RUN_DENIED: allowlist miss " +
-      "(Windows shell wrappers like cmd.exe /c require approval; " +
-      "approve once/always or run with --ask on-miss|always)"
-    );
-  }
-  return "SYSTEM_RUN_DENIED: allowlist miss";
-}
+export { formatSystemRunAllowlistMissMessage } from "./exec-policy.js";
 
 export async function handleSystemRunInvoke(opts: {
   client: GatewayClient;
@@ -122,6 +110,7 @@ export async function handleSystemRunInvoke(opts: {
   const autoAllowSkills = approvals.agent.autoAllowSkills;
   const sessionKey = opts.params.sessionKey?.trim() || "node";
   const runId = opts.params.runId?.trim() || crypto.randomUUID();
+  const approvalDecision = resolveExecApprovalDecision(opts.params.approvalDecision);
   const envOverrides = sanitizeSystemRunEnvOverrides({
     overrides: opts.params.env ?? undefined,
     shellWrapper: shellCommand !== null,
@@ -176,19 +165,9 @@ export async function handleSystemRunInvoke(opts: {
   const cmdInvocation = shellCommand
     ? opts.isCmdExeInvocation(segments[0]?.argv ?? [])
     : opts.isCmdExeInvocation(argv);
-  const windowsShellWrapperBlocked = security === "allowlist" && isWindows && cmdInvocation;
-  if (windowsShellWrapperBlocked) {
-    analysisOk = false;
-    allowlistSatisfied = false;
-  }
 
   const useMacAppExec = process.platform === "darwin";
   if (useMacAppExec) {
-    const approvalDecision =
-      opts.params.approvalDecision === "allow-once" ||
-      opts.params.approvalDecision === "allow-always"
-        ? opts.params.approvalDecision
-        : null;
     const execRequest: ExecHostRequest = {
       command: argv,
       rawCommand: rawCommand || shellCommand || null,
@@ -252,38 +231,19 @@ export async function handleSystemRunInvoke(opts: {
     }
   }
 
-  if (security === "deny") {
-    await opts.sendNodeEvent(
-      opts.client,
-      "exec.denied",
-      opts.buildExecEventPayload({
-        sessionKey,
-        runId,
-        host: "node",
-        command: cmdText,
-        reason: "security=deny",
-      }),
-    );
-    await opts.sendInvokeResult({
-      ok: false,
-      error: { code: "UNAVAILABLE", message: "SYSTEM_RUN_DISABLED: security=deny" },
-    });
-    return;
-  }
-
-  const requiresAsk = requiresExecApproval({
-    ask,
+  const policy = evaluateSystemRunPolicy({
     security,
+    ask,
     analysisOk,
     allowlistSatisfied,
+    approvalDecision,
+    approved: opts.params.approved === true,
+    isWindows,
+    cmdInvocation,
   });
-
-  const approvalDecision =
-    opts.params.approvalDecision === "allow-once" || opts.params.approvalDecision === "allow-always"
-      ? opts.params.approvalDecision
-      : null;
-  const approvedByAsk = approvalDecision !== null || opts.params.approved === true;
-  if (requiresAsk && !approvedByAsk) {
+  analysisOk = policy.analysisOk;
+  allowlistSatisfied = policy.allowlistSatisfied;
+  if (!policy.allowed) {
     await opts.sendNodeEvent(
       opts.client,
       "exec.denied",
@@ -292,17 +252,18 @@ export async function handleSystemRunInvoke(opts: {
         runId,
         host: "node",
         command: cmdText,
-        reason: "approval-required",
+        reason: policy.eventReason,
       }),
     );
     await opts.sendInvokeResult({
       ok: false,
-      error: { code: "UNAVAILABLE", message: "SYSTEM_RUN_DENIED: approval required" },
+      error: { code: "UNAVAILABLE", message: policy.errorMessage },
     });
     return;
   }
-  if (approvalDecision === "allow-always" && security === "allowlist") {
-    if (analysisOk) {
+
+  if (policy.approvalDecision === "allow-always" && security === "allowlist") {
+    if (policy.analysisOk) {
       const patterns = resolveAllowAlwaysPatterns({
         segments,
         cwd: opts.params.cwd ?? undefined,
@@ -317,28 +278,6 @@ export async function handleSystemRunInvoke(opts: {
     }
   }
 
-  if (security === "allowlist" && (!analysisOk || !allowlistSatisfied) && !approvedByAsk) {
-    await opts.sendNodeEvent(
-      opts.client,
-      "exec.denied",
-      opts.buildExecEventPayload({
-        sessionKey,
-        runId,
-        host: "node",
-        command: cmdText,
-        reason: "allowlist-miss",
-      }),
-    );
-    await opts.sendInvokeResult({
-      ok: false,
-      error: {
-        code: "UNAVAILABLE",
-        message: formatSystemRunAllowlistMissMessage({ windowsShellWrapperBlocked }),
-      },
-    });
-    return;
-  }
-
   if (allowlistMatches.length > 0) {
     const seen = new Set<string>();
     for (const match of allowlistMatches) {
@@ -379,10 +318,10 @@ export async function handleSystemRunInvoke(opts: {
   if (
     security === "allowlist" &&
     isWindows &&
-    !approvedByAsk &&
+    !policy.approvedByAsk &&
     shellCommand &&
-    analysisOk &&
-    allowlistSatisfied &&
+    policy.analysisOk &&
+    policy.allowlistSatisfied &&
     segments.length === 1 &&
     segments[0]?.argv.length > 0
   ) {