fix(compat): harden cmd shim quoting and PATH parsing for review

Address Greptile review on #1045:

- Quote cmd.exe metacharacters (& | < > ^ ! parens) in shim arguments
  so cmd treats them literally; a bare 'a&b' previously let cmd chain
  a second command (verified as injection on Windows).
- Throw on arguments containing % or control characters instead of
  letting cmd.exe reinterpret them: % expands variables even inside
  quotes and has no cmd-line escape. Mirrors Node.js EINVAL after
  CVE-2024-27980.
- Wrap the whole command line in one outer quote pair: cmd's /s
  handling strips the first and last quote of the /c payload, which
  broke spaced shim paths like "C:\Program Files\...\x.cmd".
- Split PATH the way cmd.exe reads it: quoted entries may contain ';',
  surrounding quotes are stripped, and empty entries mean the current
  directory. Plain split(';') shredded the first and dropped the last.

Author: JoJohanse

src/utils/compat.test.ts

@@ -3,7 +3,11 @@ import { mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
 import * as os from 'node:os';
 import * as path from 'node:path';
 import { runInNewContext } from 'node:vm';
-import { crossWrite, resolveWindowsCommand } from './compat';
+import {
+  buildWindowsCommandLine,
+  crossWrite,
+  resolveWindowsCommand,
+} from './compat';
 
 const TEST_DIR = path.join(os.tmpdir(), `compat-test-${process.pid}`);
 
@@ -149,4 +153,88 @@ describe('resolveWindowsCommand', () => {
     );
     expect(resolved).toBeUndefined();
   });
+
+  it('treats an empty PATH component as the current directory', () => {
+    // cmd.exe semantics: a PATH entry that is empty after splitting
+    // points at the cwd, so `dir1;;dir2` searches cwd between them.
+    const marker = `omos-cwd-probe-${process.pid}.cmd`;
+    writeFileSync(marker, '');
+    try {
+      const resolved = resolveWindowsCommand(
+        marker.replace(/\.cmd$/, ''),
+        ['', ''].join(path.delimiter),
+        '.CMD',
+      );
+      expect(resolved?.file).toBe(marker);
+      expect(resolved?.viaCmdShell).toBe(true);
+    } finally {
+      rmSync(marker, { force: true });
+    }
+  });
+
+  it('keeps a quoted PATH component containing separators intact', () => {
+    // Quoted entries may contain ';'; splitting on raw ';' would shred
+    // the directory name and miss the shim cmd.exe finds.
+    const dir = fixtureDir('semi;colon', ['bun.cmd']);
+    const resolved = resolveWindowsCommand('bun', `"${dir}"`, '.CMD');
+    expect(resolved?.file).toBe(path.join(dir, 'bun.cmd'));
+  });
+
+  it('strips surrounding quotes from individual PATH components', () => {
+    const quotedDir = fixtureDir('quoted-entry', ['bun.cmd']);
+    const plainDir = fixtureDir('plain-entry', []);
+    const resolved = resolveWindowsCommand(
+      'bun',
+      `"${quotedDir}"${path.delimiter}${plainDir}`,
+      '.CMD',
+    );
+    expect(resolved?.file).toBe(path.join(quotedDir, 'bun.cmd'));
+  });
+});
+
+describe('buildWindowsCommandLine', () => {
+  it('wraps the whole line in one outer quote pair for cmd /s /c', () => {
+    // cmd's /s handling strips the first and the last quote of the /c
+    // payload; the outer pair absorbs that so per-argument quotes keep
+    // their meaning.
+    expect(buildWindowsCommandLine('bun', ['install'])).toBe('"bun install"');
+  });
+
+  it('quotes arguments containing spaces with Windows escaping', () => {
+    expect(buildWindowsCommandLine('tar', ['-xf', 'C:\\my file.zip'])).toBe(
+      '"tar -xf "C:\\my file.zip""',
+    );
+  });
+
+  it('double-escapes trailing backslashes inside quoted arguments', () => {
+    expect(buildWindowsCommandLine('bun', ['C:\\my dir\\'])).toBe(
+      '"bun "C:\\my dir\\\\""',
+    );
+  });
+
+  it('escapes embedded quotes in quoted arguments', () => {
+    expect(buildWindowsCommandLine('bun', ['a"b'])).toBe('"bun "a\\"b""');
+  });
+
+  it('quotes cmd metacharacters so cmd.exe treats them literally', () => {
+    // Unquoted, `&` would let cmd chain a second command — the argument
+    // must end up inside double quotes on the final command line.
+    expect(buildWindowsCommandLine('bun', ['run', 'a&b'])).toBe(
+      '"bun run "a&b""',
+    );
+    expect(buildWindowsCommandLine('bun', ['run', 'a|b', 'c^d'])).toBe(
+      '"bun run "a|b" "c^d""',
+    );
+  });
+
+  it('rejects percent signs that cmd.exe would expand even quoted', () => {
+    expect(() => buildWindowsCommandLine('bun', ['100%'])).toThrow(/'%'/);
+    expect(() => buildWindowsCommandLine('bun', ['a%PATH%b'])).toThrow();
+  });
+
+  it('rejects control characters that corrupt the cmd line', () => {
+    expect(() => buildWindowsCommandLine('bun', ['a\nb'])).toThrow();
+    expect(() => buildWindowsCommandLine('bun', ['a\rb'])).toThrow();
+    expect(() => buildWindowsCommandLine('bun', ['a\u0000b'])).toThrow();
+  });
 });

src/utils/compat.ts

@@ -64,6 +64,33 @@ function splitList(value: string, separator: string): string[] {
     .filter((entry) => entry.length > 0);
 }
 
+/**
+ * Splits a Windows PATH the way cmd.exe reads it: a `;` inside a quoted
+ * component does not separate entries, the surrounding quotes are
+ * stripped, and an empty component stands for the current directory.
+ * Plain `String.split(';')` would shred quoted entries whose directory
+ * names contain `;` and silently drop current-directory entries.
+ */
+function splitWindowsPath(pathEnv: string): string[] {
+  const parts: string[] = [];
+  let current = '';
+  let inQuotes = false;
+  for (const char of pathEnv) {
+    if (char === '"') {
+      inQuotes = !inQuotes;
+      continue;
+    }
+    if (char === path.delimiter && !inQuotes) {
+      parts.push(current);
+      current = '';
+      continue;
+    }
+    current += char;
+  }
+  parts.push(current);
+  return parts.map((part) => (part === '' ? '.' : part));
+}
+
 /**
  * Resolve a bare command name against PATH and PATHEXT the way cmd.exe
  * does, so spawn() can launch it on Windows.
@@ -76,9 +103,12 @@ function splitList(value: string, separator: string): string[] {
  * shell, which silently breaks bun-based flows such as the auto-updater.
  *
  * Walks PATH entries in order; within each entry, tries PATHEXT
- * extensions in declared order. The first directory containing any match
- * wins, and the matched extension decides whether the file is directly
- * spawnable (`.exe`/`.com`) or must run through cmd.exe (`.cmd`/`.bat`).
+ * extensions in declared order. PATH entries are split the way cmd.exe
+ * reads them (quoted entries may contain `;`, empty entries mean the
+ * current directory — see splitWindowsPath). The first directory
+ * containing any match wins, and the matched extension decides whether
+ * the file is directly spawnable (`.exe`/`.com`) or must run through
+ * cmd.exe (`.cmd`/`.bat`).
  */
 export function resolveWindowsCommand(
   command: string,
@@ -98,7 +128,7 @@ export function resolveWindowsCommand(
     candidate.toLowerCase(),
   );
 
-  for (const dir of splitList(pathEnv, path.delimiter)) {
+  for (const dir of splitWindowsPath(pathEnv)) {
     let entries: string[];
     try {
       entries = readdirSync(dir);
@@ -125,22 +155,62 @@ export function resolveWindowsCommand(
   return undefined;
 }
 
+/**
+ * cmd.exe metacharacters neutralised by double-quoting the argument.
+ * Inside double quotes, `& | < > ( ) ^ !` are literal to cmd; unquoted
+ * they split or chain commands (verified on Windows: passing `a&echo x`
+ * as a bare token makes cmd execute the second command).
+ */
+const CMD_METACHARACTERS = /[\s"&|<>()^!]/;
+
+/**
+ * Detects characters that cannot be passed through `cmd.exe /c` at
+ * all: `%` expands environment variables even inside double quotes and
+ * has no escape on the cmd command line, and control characters corrupt
+ * the line. Node.js rejects the same inputs with EINVAL when spawning
+ * .cmd/.bat files (CVE-2024-27980 hardening); we throw instead of
+ * letting cmd.exe reinterpret the argument.
+ */
+function isCmdUnsafeArgument(arg: string): boolean {
+  if (arg.includes('%')) return true;
+  for (let i = 0; i < arg.length; i++) {
+    if (arg.charCodeAt(i) <= 0x1f) return true;
+  }
+  return false;
+}
+
 /**
  * Quotes one argument for a `cmd.exe /c` command line. Arguments that
- * contain no spaces or quotes are passed through untouched — cmd's /s
- * stripping mangles gratuitously quoted tokens. Callers must not pass
- * untrusted input — cmd.exe expands `%VAR%` even inside quotes.
+ * contain no metacharacters are passed through untouched — cmd's /s
+ * stripping mangles gratuitously quoted tokens.
+ *
+ * Throws on arguments that cmd.exe cannot represent faithfully (`%`,
+ * control characters) so callers fail loudly instead of executing an
+ * altered command line.
  */
 function escapeWindowsArgument(arg: string): string {
-  if (!/[\s"]/.test(arg)) {
+  if (isCmdUnsafeArgument(arg)) {
+    throw new Error(
+      `cannot pass ${JSON.stringify(arg)} through a .cmd shim: cmd.exe reinterprets '%' and control characters even inside quotes`,
+    );
+  }
+  if (!CMD_METACHARACTERS.test(arg)) {
     return arg;
   }
   const escaped = arg.replace(/(\\*)"/g, '$1$1\\"').replace(/(\\*)$/, '$1$1');
   return `"${escaped}"`;
 }
 
-function buildWindowsCommandLine(file: string, args: string[]): string {
-  return [file, ...args].map(escapeWindowsArgument).join(' ');
+/**
+ * Builds the full command line handed to `cmd.exe /d /s /c`. The whole
+ * line is wrapped in one outer pair of quotes because cmd's /s
+ * processing strips the first and the last quote of the /c payload:
+ * without the outer pair, a spaced path like `"C:\Program
+ * Files\...\bun.cmd"` loses its quotes and the spawn fails (verified on
+ * Windows). Exported for unit tests only.
+ */
+export function buildWindowsCommandLine(file: string, args: string[]): string {
+  return `"${[file, ...args].map(escapeWindowsArgument).join(' ')}"`;
 }
 
 /**