feat: unify CLI execution and never spawn a shell

CHANGELOG.md

@@ -25,6 +25,21 @@
   discovery/loss/recovery with sampled network info, and reconnecting
   WebSocket open/drop/reconnect/state transitions.
 - Local telemetry now records authentication refresh and recovery prompts.
+- Path-like settings (`coder.binaryDestination`, `coder.tlsCertFile`,
+  `coder.tlsKeyFile`, `coder.tlsCaFile`, `coder.tlsAltHost`,
+  `coder.proxyLogDirectory`) and items in `coder.globalFlags` now support
+  `${env:VAR}`, `${userHome}`, and a leading `~`. For `--flag=value` items
+  in `coder.globalFlags`, the expansion applies to the value half, so
+  `--cfg=~/coder` works.
+
+### Changed
+
+- The Coder CLI is now spawned directly instead of through a shell, so
+  arguments reach the binary as-is. The extension no longer has to
+  shell-escape values by hand. That escaping was error-prone (especially
+  around `cmd.exe` on Windows) and a recurring command-injection risk
+  when deployment-supplied values like workspace names or template
+  parameters contained spaces, quotes, or shell metacharacters.
 
 ### Fixed
 

package.json

@@ -67,7 +67,7 @@
 					"default": ""
 				},
 				"coder.binaryDestination": {
-					"markdownDescription": "The path to the Coder CLI binary or the directory containing it. When set to a file path (e.g., `/usr/bin/coder`), the extension checks its version and downloads a replacement if it does not match the server (and downloads are enabled). When set to a directory, the extension looks for the CLI inside it (downloading if enabled). Defaults to the value of `CODER_BINARY_DESTINATION` if not set, otherwise the extension's global storage directory.",
+					"markdownDescription": "The path to the Coder CLI binary or the directory containing it. When set to a file path (e.g., `/usr/bin/coder`), the extension checks its version and downloads a replacement if it does not match the server (and downloads are enabled). When set to a directory, the extension looks for the CLI inside it (downloading if enabled). Defaults to the value of `CODER_BINARY_DESTINATION` if not set, otherwise the extension's global storage directory.\n\nSupports `${env:VAR}`, `${userHome}`, and a leading `~`.",
 					"type": "string",
 					"default": ""
 				},
@@ -82,22 +82,22 @@
 					"default": ""
 				},
 				"coder.tlsCertFile": {
-					"markdownDescription": "Path to file for TLS client cert. When specified, token authorization will be skipped. `http.proxySupport` must be set to `on` or `off`, otherwise VS Code will override the proxy agent set by the plugin.",
+					"markdownDescription": "Path to file for TLS client cert. When specified, token authorization will be skipped. `http.proxySupport` must be set to `on` or `off`, otherwise VS Code will override the proxy agent set by the plugin.\n\nSupports `${env:VAR}`, `${userHome}`, and a leading `~`.",
 					"type": "string",
 					"default": ""
 				},
 				"coder.tlsKeyFile": {
-					"markdownDescription": "Path to file for TLS client key. When specified, token authorization will be skipped. `http.proxySupport` must be set to `on` or `off`, otherwise VS Code will override the proxy agent set by the plugin.",
+					"markdownDescription": "Path to file for TLS client key. When specified, token authorization will be skipped. `http.proxySupport` must be set to `on` or `off`, otherwise VS Code will override the proxy agent set by the plugin.\n\nSupports `${env:VAR}`, `${userHome}`, and a leading `~`.",
 					"type": "string",
 					"default": ""
 				},
 				"coder.tlsCaFile": {
-					"markdownDescription": "Path to file for TLS certificate authority. `http.proxySupport` must be set to `on` or `off`, otherwise VS Code will override the proxy agent set by the plugin.",
+					"markdownDescription": "Path to file for TLS certificate authority. `http.proxySupport` must be set to `on` or `off`, otherwise VS Code will override the proxy agent set by the plugin.\n\nSupports `${env:VAR}`, `${userHome}`, and a leading `~`.",
 					"type": "string",
 					"default": ""
 				},
 				"coder.tlsAltHost": {
-					"markdownDescription": "Alternative hostname to use for TLS verification. This is useful when the hostname in the certificate does not match the hostname used to connect.",
+					"markdownDescription": "Alternative hostname to use for TLS verification. This is useful when the hostname in the certificate does not match the hostname used to connect.\n\nSupports `${env:VAR}` substitution.",
 					"type": "string",
 					"default": ""
 				},
@@ -107,7 +107,7 @@
 					"default": ""
 				},
 				"coder.proxyLogDirectory": {
-					"markdownDescription": "Directory where the Coder CLI outputs SSH connection logs for debugging. Defaults to the value of `CODER_SSH_LOG_DIR` if not set, otherwise the extension's global storage directory.",
+					"markdownDescription": "Directory where the Coder CLI outputs SSH connection logs for debugging. Defaults to the value of `CODER_SSH_LOG_DIR` if not set, otherwise the extension's global storage directory.\n\nSupports `${env:VAR}`, `${userHome}`, and a leading `~`.",
 					"type": "string",
 					"default": ""
 				},
@@ -160,7 +160,7 @@
 					]
 				},
 				"coder.globalFlags": {
-					"markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item; values are passed verbatim and in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nNote that for `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` and `--use-keyring` flags are silently ignored as the extension manages them via `#coder.useKeyring#`.",
+					"markdownDescription": "Global flags to pass to every Coder CLI invocation. Enter each flag as a separate array item, in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nSupports `${env:VAR}`, `${userHome}`, and a leading `~`. For `--flag=value` items the expansion applies to the value half, so `--cfg=~/coder` works.\n\nFor `--header-command`, precedence is: `#coder.headerCommand#` setting, then `CODER_HEADER_COMMAND` environment variable, then the value specified here. The `--global-config` and `--use-keyring` flags are silently ignored as the extension manages them via `#coder.useKeyring#`.",
 					"type": "array",
 					"items": {
 						"type": "string"

src/api/updateParameters.ts

@@ -1,7 +1,5 @@
 import * as vscode from "vscode";
 
-import { escapeShellArg } from "../util";
-
 import type { Api } from "coder/site/src/api/api";
 import type {
 	TemplateVersionParameter,
@@ -44,7 +42,7 @@ export async function collectUpdateParameters(
 		if (value === undefined) {
 			throw new WorkspaceUpdateCancelledError();
 		}
-		args.push("--parameter", escapeShellArg(`${param.name}=${value}`));
+		args.push("--parameter", `${param.name}=${value}`);
 	}
 	return args;
 }

src/api/workspace.ts

@@ -1,8 +1,7 @@
 import { spawn } from "node:child_process";
 import * as vscode from "vscode";
 
-import { getGlobalShellFlags, type CliAuth } from "../settings/cli";
-import { escapeCommandArg, escapeShellArg } from "../util";
+import { getGlobalFlags, type CliAuth } from "../settings/cli";
 
 import { errToStr, createWorkspaceIdentifier } from "./api-helper";
 import { collectUpdateParameters } from "./updateParameters";
@@ -62,12 +61,11 @@ interface CliContext {
 function runCliCommand(ctx: CliContext, args: string[]): Promise<void> {
 	return new Promise((resolve, reject) => {
 		const fullArgs = [
-			...getGlobalShellFlags(vscode.workspace.getConfiguration(), ctx.auth),
+			...getGlobalFlags(vscode.workspace.getConfiguration(), ctx.auth),
 			...args,
-			escapeShellArg(createWorkspaceIdentifier(ctx.workspace)),
+			createWorkspaceIdentifier(ctx.workspace),
 		];
-		const cmd = `${escapeCommandArg(ctx.binPath)} ${fullArgs.join(" ")}`;
-		const proc = spawn(cmd, { shell: true });
+		const proc = spawn(ctx.binPath, fullArgs);
 		// Unexpected prompts EOF instead of hanging forever.
 		proc.stdin.end();
 
@@ -82,6 +80,9 @@ function runCliCommand(ctx: CliContext, args: string[]): Promise<void> {
 			capturedStderr += text;
 		});
 
+		// Settle on ENOENT/EACCES; later `close` rejects are then no-ops.
+		proc.on("error", reject);
+
 		proc.on("close", (code: number | null, signal: NodeJS.Signals | null) => {
 			if (code === 0) {
 				resolve();

src/core/cliExec.ts

@@ -109,11 +109,11 @@ export async function supportBundle(
  * Run `coder ping` in a PTY terminal with Ctrl+C support.
  */
 export function ping(env: CliEnv, workspaceName: string): vscode.Terminal {
-	const globalFlags = getGlobalShellFlags(env.configs, env.auth);
+	const globalFlags = getGlobalFlags(env.configs, env.auth);
 	return spawnCliInTerminal({
 		name: `Coder Ping: ${workspaceName}`,
 		binary: env.binary,
-		args: [...globalFlags, "ping", escapeCommandArg(workspaceName)],
+		args: [...globalFlags, "ping", workspaceName],
 		banner: ["Press Ctrl+C (^C) to stop.", "─".repeat(40)],
 	});
 }
@@ -172,14 +172,12 @@ function spawnCliInTerminal(options: {
 	const writeEmitter = new vscode.EventEmitter<string>();
 	const closeEmitter = new vscode.EventEmitter<number | void>();
 
-	const cmd = `${escapeCommandArg(options.binary)} ${options.args.join(" ")}`;
-	// On Unix, spawn in a new process group so we can signal the
-	// entire group (shell + coder binary) on Ctrl+C. On Windows,
-	// detached opens a visible console window and negative-PID kill
-	// is unsupported, so we fall back to proc.kill().
+	// On Unix, `detached` puts the child in its own process group so
+	// Ctrl+C can signal the whole subtree. On Windows it would open a
+	// visible console window and negative-PID kill is unsupported, so we
+	// fall back to proc.kill() there.
 	const useProcessGroup = process.platform !== "win32";
-	const proc = spawn(cmd, {
-		shell: true,
+	const proc = spawn(options.binary, options.args, {
 		detached: useProcessGroup,
 	});
 
@@ -266,11 +264,12 @@ function spawnCliInTerminal(options: {
 		writeEmitter.fire("Press any key to close.\r\n");
 	});
 	proc.on("close", (code, signal) => {
-		exited = true;
-		clearTimeout(forceKillTimer);
-		if (closed) {
+		// On ENOENT/EACCES, `error` fires first and already wrote the failure
+		if (exited || closed) {
 			return;
 		}
+		exited = true;
+		clearTimeout(forceKillTimer);
 		let reason: string;
 		if (signal === "SIGKILL") {
 			reason = "Process force killed (SIGKILL)";

src/remote/remote.ts

@@ -41,9 +41,9 @@ import { type LoginCoordinator } from "../login/loginCoordinator";
 import { OAuthSessionManager } from "../oauth/sessionManager";
 import {
 	type CliAuth,
+	getExpandedUserGlobalFlags,
 	getGlobalShellFlags,
 	getSshFlags,
-	getUserGlobalFlags,
 	resolveCliAuth,
 } from "../settings/cli";
 import { getHeaderCommand } from "../settings/headers";
@@ -436,7 +436,7 @@ export class Remote {
 					setting: "coder.globalFlags",
 					title: "Global Flags",
 					getValue: () =>
-						getUserGlobalFlags(vscode.workspace.getConfiguration()),
+						getExpandedUserGlobalFlags(vscode.workspace.getConfiguration()),
 				},
 				{
 					setting: "coder.headerCommand",

src/settings/cli.ts

@@ -1,5 +1,5 @@
 import { isKeyringSupported } from "../core/cliCredentialManager";
-import { escapeCommandArg, escapeShellArg } from "../util";
+import { escapeCommandArg, escapeShellArg, expandPath } from "../util";
 
 import { getHeaderArgs } from "./headers";
 
@@ -11,14 +11,23 @@ export type CliAuth =
 	| { mode: "global-config"; configDir: string }
 	| { mode: "url"; url: string };
 
-/** Returns the user's `coder.globalFlags` as configured, with no expansion. */
-export function getUserGlobalFlags(
+/**
+ * Returns the user's `coder.globalFlags` with `expandPath` applied. For
+ * `--flag=value` entries the substitution is scoped to the value half so
+ * `--cfg=~/coder` works without rewriting the flag name.
+ */
+export function getExpandedUserGlobalFlags(
 	configs: Pick<WorkspaceConfiguration, "get">,
 ): string[] {
-	return configs.get<string[]>("coder.globalFlags", []);
+	return configs.get<string[]>("coder.globalFlags", []).map((flag) => {
+		const eq = flag.indexOf("=");
+		return eq === -1
+			? expandPath(flag)
+			: flag.slice(0, eq + 1) + expandPath(flag.slice(eq + 1));
+	});
 }
 
-/** Flags for shell contexts (`terminal.sendText`, `spawn({ shell: true })`). */
+/** Flags for shell contexts (`terminal.sendText`, SSH `ProxyCommand`). */
 export function getGlobalShellFlags(
 	configs: Pick<WorkspaceConfiguration, "get">,
 	auth: CliAuth,
@@ -47,7 +56,11 @@ function buildGlobalFlags(
 			? ["--url", escAuth(auth.url)]
 			: ["--global-config", escAuth(auth.configDir)];
 
-	const filtered = stripManagedFlags(getUserGlobalFlags(configs));
+	// Escape each user flag so expansion-introduced whitespace stays inside
+	// one shell token. `escAuth` is `identity` on the array path.
+	const filtered = stripManagedFlags(getExpandedUserGlobalFlags(configs)).map(
+		escAuth,
+	);
 
 	return [...filtered, ...authFlags, ...getHeaderArgs(configs, escHeader)];
 }

src/util.ts

@@ -131,14 +131,21 @@ export function toSafeHost(rawUrl: string): string {
 }
 
 /**
- * Expand a path if it starts with tilde (~) or contains ${userHome}.
+ * Substitute `${env:VAR}` with `process.env.VAR` (unset → empty string),
+ * `${userHome}` (anywhere) with `os.homedir()`, and a leading `~` with
+ * `os.homedir()`. Env substitution runs first so env values can themselves
+ * contain `~` or `${userHome}`.
  */
 export function expandPath(input: string): string {
+	const expanded = input.replace(
+		/\$\{env:([A-Za-z_][A-Za-z0-9_]*)\}/g,
+		(_, name: string) => process.env[name] ?? "",
+	);
 	const userHome = os.homedir();
-	if (input.startsWith("~")) {
-		input = userHome + input.substring("~".length);
-	}
-	return input.replaceAll("${userHome}", userHome);
+	const tildeExpanded = expanded.startsWith("~")
+		? userHome + expanded.substring("~".length)
+		: expanded;
+	return tildeExpanded.replaceAll("${userHome}", userHome);
 }
 
 /**
@@ -202,9 +209,24 @@ export async function renameWithRetry(
 	}
 }
 
+/**
+ * Wraps `arg` in `"..."` unless every character is in the shell-safe
+ * whitelist (matching Python `shlex.quote`'s set: alphanumerics plus
+ * `@%+,=:./-`). Anything else (whitespace, `"`, `&|;()<>*?[~#!^\` `$`)
+ * forces quoting so the output is a single token in POSIX `sh`, cmd.exe,
+ * and PowerShell.
+ *
+ * Not a universal shell-escape: `$VAR` / `$(...)` / `%VAR%` still expand
+ * inside `"..."`. For untrusted values use {@link escapeShellArg}.
+ *
+ * @see https://docs.python.org/3/library/shlex.html#shlex.quote
+ * @see https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way
+ */
 export function escapeCommandArg(arg: string): string {
-	const escapedString = arg.replaceAll('"', String.raw`\"`);
-	return `"${escapedString}"`;
+	if (arg !== "" && /^[\w@%+,=:./-]+$/.test(arg)) {
+		return arg;
+	}
+	return `"${arg.replaceAll('"', String.raw`\"`)}"`;
 }
 
 /**

test/mocks/vscode.runtime.ts

@@ -141,6 +141,7 @@ export const window = {
 	createInputBox: vi.fn(),
 	createQuickPick: vi.fn(),
 	createStatusBarItem: vi.fn(),
+	createTerminal: vi.fn(),
 	createWebviewPanel: vi.fn(),
 	registerUriHandler: vi.fn(() => ({ dispose: vi.fn() })),
 };

test/unit/api/updateParameters.test.ts

@@ -8,8 +8,6 @@ import {
 
 import { workspace as createWorkspace } from "@repo/mocks";
 
-import { shellQuote } from "../../utils/platform";
-
 import type { Api } from "coder/site/src/api/api";
 import type { TemplateVersionParameter } from "coder/site/src/api/typesGenerated";
 
@@ -145,14 +143,14 @@ describe("collectUpdateParameters", () => {
 			param: { name: "environment" },
 			mock: mockCreateInputBox,
 			accept: { value: "dev" },
-			expected: ["--parameter", shellQuote("environment=dev")],
+			expected: ["--parameter", "environment=dev"],
 		},
 		{
 			kind: "bool quick pick",
 			param: { name: "enabled", type: "bool" },
 			mock: mockCreateQuickPick,
 			accept: { selectedItems: [{ value: "true" }] },
-			expected: ["--parameter", shellQuote("enabled=true")],
+			expected: ["--parameter", "enabled=true"],
 		},
 		{
 			kind: "options quick pick",
@@ -165,7 +163,7 @@ describe("collectUpdateParameters", () => {
 			},
 			mock: mockCreateQuickPick,
 			accept: { selectedItems: [{ value: "l" }] },
-			expected: ["--parameter", shellQuote("size=l")],
+			expected: ["--parameter", "size=l"],
 		},
 		{
 			kind: "multi-select quick pick (JSON array)",
@@ -179,7 +177,7 @@ describe("collectUpdateParameters", () => {
 			},
 			mock: mockCreateQuickPick,
 			accept: { selectedItems: [{ value: "us" }, { value: "eu" }] },
-			expected: ["--parameter", shellQuote('regions=["us","eu"]')],
+			expected: ["--parameter", 'regions=["us","eu"]'],
 		},
 	])(
 		"collects the value via $kind",
@@ -195,18 +193,15 @@ describe("collectUpdateParameters", () => {
 		},
 	);
 
-	it("escapes shell metacharacters in server-controlled values", async () => {
+	it("passes server-controlled values through verbatim (no shell expansion path)", async () => {
 		const { restClient, workspace } = createCollectCtx([{ name: "evil" }]);
 		const qi = mockCreateInputBox();
 
 		const result = collectUpdateParameters(restClient, workspace);
 		await waitShown(qi);
 		qi.accept({ value: "$(rm -rf /)" });
 
-		await expect(result).resolves.toEqual([
-			"--parameter",
-			shellQuote("evil=$(rm -rf /)"),
-		]);
+		await expect(result).resolves.toEqual(["--parameter", "evil=$(rm -rf /)"]);
 	});
 
 	it("skips parameters that already have a value or default", async () => {