From b295c8a44f9b1a5a0655ba127bd4dee476453307 Mon Sep 17 00:00:00 2001 From: Fabio Caldas Date: Wed, 14 Oct 2026 20:37:27 -0300 Subject: [PATCH] Start the CopilotKit CLI through cmd.exe on Windows On Windows, spawning npx.cmd without a shell throws a synchronous EINVAL (Node/Bun hardening after CVE-2024-27980), so scripts/setup-learning.ts stopped with "spawn npx.cmd EINVAL" and the server's Learning setup at boot could not run the CLI either. - npxCommand() starts npx.cmd with shell: true on Windows only. - cmdLineSafe() refuses an argument cmd.exe would run or expand (the project id read from .copilotkit/project.json is not a literal), and setup-learning.ts names such an id instead of failing later. - stopProcessTree() ends the whole tree with taskkill /T /F on timeout; child.kill() ended only cmd.exe and left the CLI holding the pipes. Fixes #792 --- CHANGELOG.md | 9 +++ scripts/setup-learning.test.ts | 15 ++++ scripts/setup-learning.ts | 8 ++ server/src/learning/ensure-container.ts | 81 ++++++++++++++++--- .../tests/learning-ensure-container.test.ts | 79 ++++++++++++++++++ 5 files changed, 179 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2aa59af7d..5715666b8 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/scripts/setup-learning.test.ts b/scripts/setup-learning.test.ts index ad73a2582..f30a5a6a0 100644 --- a/scripts/setup-learning.test.ts +++ b/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({ diff --git a/scripts/setup-learning.ts b/scripts/setup-learning.ts index be3bbd7f0..406566abb 100644 --- a/scripts/setup-learning.ts +++ b/scripts/setup-learning.ts @@ -4,6 +4,7 @@ import { createInterface } from "node:readline/promises"; import { parseEnv } from "node:util"; import { cliLoginPresent, + cmdLineSafe, containerMissing, ensureLearningContainer, json, @@ -11,6 +12,7 @@ import { 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( diff --git a/server/src/learning/ensure-container.ts b/server/src/learning/ensure-container.ts index 916389e57..56566745a 100644 --- a/server/src/learning/ensure-container.ts +++ b/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,18 +120,69 @@ export function cliEnvironment( return kept; } -/** Run `npx --yes copilotkit@latest ` 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 ` in `directory`, for at most two minutes. */ +export const runNpx = (args: string[], directory: string): Promise => 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) => { @@ -139,7 +190,7 @@ export const runCli: RunCli = (args, directory) => }); // 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 ` 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. * diff --git a/server/tests/learning-ensure-container.test.ts b/server/tests/learning-ensure-container.test.ts index 6a572be5d..6687bc22a 100644 --- a/server/tests/learning-ensure-container.test.ts +++ b/server/tests/learning-ensure-container.test.ts @@ -1,4 +1,5 @@ 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"; @@ -6,10 +7,14 @@ 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', + "ab", + "(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); + }, + ); });