Start the CopilotKit CLI through cmd.exe on Windows

CHANGELOG.md

@@ -32,6 +32,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.
   `code_required` (below). A server and a computer must be upgraded together: an older computer has
   no `/sign-in/code`.
 
+### `setup-learning.ts` runs on Windows
+
+On Windows, `bun scripts/setup-learning.ts` stopped at once with `spawn npx.cmd EINVAL` and wrote
+nothing to `.env`. Since CVE-2024-27980, Node and Bun refuse to start a `.cmd` without a shell, and
+`npx` is one there. The CopilotKit CLI is now started through `cmd.exe` on Windows. An argument the
+shell would run or expand, such as a project id containing `&` or `%`, is refused instead of passed.
+A CLI that outlives its two minutes now has its whole process tree stopped. The server's own
+Learning setup at boot calls the CLI the same way, and is fixed with it.
+
 ### A reply's tool calls fold into one line
 
 A Bot that searched, read two pages and checked Drive before answering drew four lines above its

scripts/setup-learning.test.ts

@@ -144,6 +144,21 @@ test("fresh managed setup creates and verifies the selected project's container
   expect(f.calls).toHaveLength(3);
 });
 
+test.if(process.platform === "win32")(
+  "a project id the Windows shell would run is named, and the CLI is not called",
+  async () => {
+    const f = await fixture();
+    await writeFile(
+      join(f.directory, ".copilotkit/project.json"),
+      JSON.stringify({ projectId: "12&calc", projectSlug: "chosen" }),
+    );
+    await expect(
+      setupLearning({ ...f.options, runCli: f.runCli }),
+    ).rejects.toThrow("cannot be passed safely");
+    expect(f.calls).toEqual([]);
+  },
+);
+
 test("reuses an existing verified container without creating or editing it", async () => {
   const f = await fixture();
   await setupLearning({

scripts/setup-learning.ts

@@ -4,13 +4,15 @@ import { createInterface } from "node:readline/promises";
 import { parseEnv } from "node:util";
 import {
   cliLoginPresent,
+  cmdLineSafe,
   containerMissing,
   ensureLearningContainer,
   json,
   type KeyIntelligence,
   keyIntelligence,
   learningSetupInstructions,
   MANAGED_API,
+  npxCommand,
   object,
   type RunCli,
   runCli,
@@ -224,6 +226,12 @@ export async function setupLearning(
       "The selected project is invalid. Run npx --yes copilotkit@latest project select again.",
     );
   }
+  // Through cmd.exe, `runCli` refuses such an id without running anything; say so here instead.
+  if (npxCommand().shell && !cmdLineSafe([projectId])) {
+    throw new Error(
+      "The selected project id has characters that cannot be passed safely to the CopilotKit CLI on Windows. Run npx --yes copilotkit@latest project select again.",
+    );
+  }
   // A saved record alone does not prove a stale CPK key belongs to it. Re-provision here.
   // CLI project select removes INTELLIGENCE_API_KEY, hence the precondition above.
   const provision = await run(

server/src/learning/ensure-container.ts

@@ -21,7 +21,7 @@
  * Shared by `scripts/setup-learning.ts` (run by `scripts/start.sh`), the server (at boot and from
  * Admin → Automatic Learning), and the Helm chart's setup Job, which runs this file directly.
  */
-import { spawn } from "node:child_process";
+import { type ChildProcess, spawn } from "node:child_process";
 import { existsSync } from "node:fs";
 import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
 import { homedir, tmpdir } from "node:os";
@@ -120,26 +120,77 @@ export function cliEnvironment(
   return kept;
 }
 
-/** Run `npx --yes copilotkit@latest <args>` in `directory`, for at most two minutes. */
-export const runCli: RunCli = (args, directory) =>
+/**
+ * How to start `npx` on a platform. Windows ships it as `npx.cmd`, and since CVE-2024-27980 Node and
+ * Bun refuse to spawn a `.cmd` without a shell: `spawn` throws EINVAL before anything runs.
+ */
+export function npxCommand(platform: NodeJS.Platform = process.platform): {
+  command: string;
+  shell: boolean;
+} {
+  return platform === "win32"
+    ? { command: "npx.cmd", shell: true }
+    : { command: "npx", shell: false };
+}
+
+/**
+ * Whether every argument reaches the program as written through `cmd.exe`. The shell joins them into
+ * one unquoted command line, so `&` or `|` in one would start another command and `%` would expand a
+ * variable. Not every argument is a literal: `setup-learning.ts` passes the project id it reads from
+ * `.copilotkit/project.json`.
+ */
+export function cmdLineSafe(args: string[]): boolean {
+  return args.every((arg) => /^[A-Za-z0-9@._:/+-]+$/.test(arg));
+}
+
+/**
+ * `child.kill()` on Windows ends `cmd.exe` and leaves the command under it running, holding the
+ * pipes, so `close` waits for that command however long it takes. `taskkill /T` ends the tree.
+ * Once `cmd.exe` has exited its id may belong to another process, so it is not used after that.
+ */
+export function stopProcessTree(child: ChildProcess, shell: boolean): void {
+  if (child.exitCode !== null || child.signalCode !== null) return;
+  if (!shell || child.pid === undefined) {
+    child.kill();
+    return;
+  }
+  // By full path: a bare name is looked up in the working directory first.
+  const taskkill = join(
+    process.env.SystemRoot ?? "C:\\Windows",
+    "System32",
+    "taskkill.exe",
+  );
+  spawn(taskkill, ["/pid", String(child.pid), "/T", "/F"], {
+    stdio: "ignore",
+  })
+    .on("error", () => child.kill())
+    .on("exit", (code) => {
+      if (code !== 0) child.kill();
+    });
+}
+
+/** Run `npx <args>` in `directory`, for at most two minutes. */
+export const runNpx = (args: string[], directory: string): Promise<CliResult> =>
   new Promise((resolvePromise) => {
-    const child = spawn(
-      process.platform === "win32" ? "npx.cmd" : "npx",
-      ["--yes", "copilotkit@latest", ...args],
-      {
-        cwd: directory,
-        env: cliEnvironment(),
-        stdio: ["ignore", "pipe", "pipe"],
-      },
-    );
+    const { command, shell } = npxCommand();
+    if (shell && !cmdLineSafe(args)) {
+      resolvePromise({ exitCode: 1, stdout: "" });
+      return;
+    }
+    const child = spawn(command, args, {
+      cwd: directory,
+      env: cliEnvironment(),
+      shell,
+      stdio: ["ignore", "pipe", "pipe"],
+    });
     let stdout = "";
     child.stdout.setEncoding("utf8");
     child.stdout.on("data", (chunk: string) => {
       if (stdout.length < 1_048_576) stdout += chunk;
     });
     // CLI output on stderr can carry account or credential details. Never relay it.
     child.stderr.resume();
-    const timeout = setTimeout(() => child.kill(), 120_000);
+    const timeout = setTimeout(() => stopProcessTree(child, shell), 120_000);
     const finish = (exitCode: number) => {
       clearTimeout(timeout);
       resolvePromise({ exitCode, stdout });
@@ -148,6 +199,10 @@ export const runCli: RunCli = (args, directory) =>
     child.on("close", (code) => finish(code ?? 1));
   });
 
+/** Run `npx --yes copilotkit@latest <args>` in `directory`, for at most two minutes. */
+export const runCli: RunCli = (args, directory) =>
+  runNpx(["--yes", "copilotkit@latest", ...args], directory);
+
 /**
  * Whether a CopilotKit CLI login is on this machine, without running the CLI.
  *

server/tests/learning-ensure-container.test.ts

@@ -1,15 +1,20 @@
 import { afterEach, describe, expect, test } from "bun:test";
+import { spawn } from "node:child_process";
 import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import type { ContainerProbe } from "../src/learning/auto-assign";
 import {
   cliEnvironment,
   cliLoginPresent,
+  cmdLineSafe,
   ensureLearningContainer,
   type KeyIntelligence,
   learningSetupInstructions,
   MANAGED_API,
+  npxCommand,
+  runNpx,
+  stopProcessTree,
 } from "../src/learning/ensure-container";
 
 const SELF_HOSTED = "https://intelligence.customer.example";
@@ -231,4 +236,78 @@ describe("the CopilotKit CLI", () => {
       COPILOTKIT_TELEMETRY_DISABLED: "1",
     });
   });
+
+  test("is started through a shell on Windows, where npx is a .cmd", () => {
+    expect(npxCommand("win32")).toEqual({ command: "npx.cmd", shell: true });
+    expect(npxCommand("darwin")).toEqual({ command: "npx", shell: false });
+    expect(npxCommand("linux")).toEqual({ command: "npx", shell: false });
+  });
+
+  test("passes only arguments cmd.exe would not run or expand", () => {
+    expect(
+      cmdLineSafe([
+        "--yes",
+        "copilotkit@latest",
+        "project",
+        "select",
+        "--project",
+        "a1B2c",
+        "--json",
+      ]),
+    ).toBe(true);
+    for (const unsafe of [
+      "abc&calc",
+      "abc|calc",
+      "%PATH%",
+      "a^b",
+      "a b",
+      'a"b',
+      "a<b",
+      "a>b",
+      "(a)",
+      "a=b",
+      "",
+    ])
+      expect(cmdLineSafe(["project", "select", "--project", unsafe])).toBe(
+        false,
+      );
+  });
+
+  // Since CVE-2024-27980, spawning a .cmd without a shell throws EINVAL before anything runs.
+  test.if(process.platform === "win32")(
+    "starts on this Windows machine instead of throwing EINVAL",
+    async () => {
+      const result = await runNpx(["--version"], tmpdir());
+      expect(result.exitCode).toBe(0);
+      expect(result.stdout.trim()).toMatch(/^\d+\.\d+\.\d+$/);
+    },
+  );
+
+  test.if(process.platform === "win32")(
+    "refuses an argument the Windows shell would run, without starting it",
+    async () => {
+      const result = await runNpx(["--version", "x&echo INJECTED"], tmpdir());
+      expect(result).toEqual({ exitCode: 1, stdout: "" });
+    },
+  );
+
+  // Killing cmd.exe alone leaves the command under it holding the pipes, so `close` waits for it.
+  test.if(process.platform === "win32")(
+    "a stopped command takes the processes under its shell with it",
+    async () => {
+      const child = spawn("ping", ["-n", "30", "127.0.0.1"], {
+        shell: true,
+        stdio: ["ignore", "pipe", "pipe"],
+      });
+      child.stdout.resume();
+      child.stderr.resume();
+      const closed = new Promise((resolve) => child.on("close", resolve));
+      // Stopped at once, cmd.exe goes before it has started ping, and there is no tree to leave.
+      await new Promise((resolve) => setTimeout(resolve, 1_000));
+      const started = Date.now();
+      stopProcessTree(child, true);
+      await closed;
+      expect(Date.now() - started).toBeLessThan(4_000);
+    },
+  );
 });