fix: address R3 review feedback

- Drop `shell: true` from `runCliCommand`; pass args as an array via
  `getGlobalFlags` (raw). Eliminates Windows cmd.exe quoting concerns
  entirely (DEREM-17) and lets `--parameter` values pass through verbatim.
- Close child stdin after spawn so unexpected interactive CLI prompts
  see EOF and error out instead of hanging (DEREM-18).
- Guard the running case of the state machine against post-update builds
  that aren't running yet (DEREM-19).
- Drop client-side regex validation (server validates with RE2, ReDoS-safe)
  to prevent extension-host freezes on hostile patterns (DEREM-22).
- Expand `${env:VAR}` in `coder.globalFlags` so users can reference
  environment variables now that there's no outer shell expansion.
- Document the new globalFlags expansion semantics in package.json.

Author: EhabY

package.json

@@ -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; 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\nValues are passed directly to the CLI without a shell, so `$VAR` and `%VAR%` are **not** expanded. Use `${env:VAR}` to reference environment variables (e.g. `--cfg=${env:HOME}/cfg`); missing variables resolve to an empty string.\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#`.",
 					"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,
@@ -25,15 +23,15 @@ export async function collectUpdateParameters(
 	restClient: Api,
 	workspace: Workspace,
 ): Promise<string[]> {
-	const newParams = await restClient.getTemplateVersionRichParameters(
-		workspace.template_active_version_id,
-	);
+	const [newParams, currentValues] = await Promise.all([
+		restClient.getTemplateVersionRichParameters(
+			workspace.template_active_version_id,
+		),
+		restClient.getWorkspaceBuildParameters(workspace.latest_build.id),
+	]);
 	const candidates = newParams.filter((p) => p.required && !p.default_value);
 	if (candidates.length === 0) return [];
 
-	const currentValues = await restClient.getWorkspaceBuildParameters(
-		workspace.latest_build.id,
-	);
 	const existing = new Set(currentValues.map((p) => p.name));
 	const toPrompt = candidates.filter((p) => !existing.has(p.name));
 
@@ -44,8 +42,7 @@ export async function collectUpdateParameters(
 		if (value === undefined) {
 			throw new WorkspaceUpdateCancelledError();
 		}
-		// Server-controlled values; block shell expansion under `shell: true`.
-		args.push("--parameter", escapeShellArg(`${param.name}=${value}`));
+		args.push("--parameter", `${param.name}=${value}`);
 	}
 	return args;
 }
@@ -178,20 +175,12 @@ function substituteTemplate(
 }
 
 /**
- * Returns `{ ok, message }`; invalid RE2 regexes fall through to server-side
- * validation.
+ * Returns `{ ok, message }`. Regex constraints are intentionally not tested
+ * client-side; server validates with RE2 (linear-time, ReDoS-safe).
  */
 function makeValidator(
 	param: TemplateVersionParameter,
 ): (input: string) => { ok: boolean; message?: string } {
-	let re: RegExp | undefined;
-	if (param.validation_regex) {
-		try {
-			re = new RegExp(param.validation_regex);
-		} catch {
-			re = undefined;
-		}
-	}
 	return (input) => {
 		if (!input) return { ok: !param.required };
 		if (param.type === "number") {
@@ -216,14 +205,6 @@ function makeValidator(
 				};
 			}
 		}
-		if (re && !re.test(input)) {
-			return {
-				ok: false,
-				message:
-					substituteTemplate(param.validation_error, param, input) ||
-					"Invalid format",
-			};
-		}
 		return { ok: true };
 	};
 }

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 } from "../util";
+import { getGlobalFlags, type CliAuth } from "../settings/cli";
 
 import { errToStr, createWorkspaceIdentifier } from "./api-helper";
 import { collectUpdateParameters } from "./updateParameters";
@@ -62,12 +61,13 @@ 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,
 			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();
 
 		proc.stdout.on("data", (data: Buffer) => {
 			ctx.write(data.toString());

src/remote/remote.ts

@@ -41,9 +41,9 @@ import { type LoginCoordinator } from "../login/loginCoordinator";
 import { OAuthSessionManager } from "../oauth/sessionManager";
 import {
 	type CliAuth,
-	getGlobalFlagsRaw,
 	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: () =>
-						getGlobalFlagsRaw(vscode.workspace.getConfiguration()),
+						getUserGlobalFlags(vscode.workspace.getConfiguration()),
 				},
 				{
 					setting: "coder.headerCommand",

src/remote/workspaceStateMachine.ts

@@ -78,6 +78,7 @@ export class WorkspaceStateMachine implements vscode.Disposable {
 					workspace = updated;
 					// Agent IDs may have changed after an update.
 					this.agent = undefined;
+					if (workspace.latest_build.status !== "running") return false;
 				}
 				break;
 			}
@@ -106,8 +107,8 @@ export class WorkspaceStateMachine implements vscode.Disposable {
 					workspace = updated;
 					// Agent IDs may have changed after an update.
 					this.agent = undefined;
-					if (workspace.latest_build.status === "running") break;
-					return false;
+					if (workspace.latest_build.status !== "running") return false;
+					break;
 				}
 				// Either we weren't in update mode, or the update failed: start.
 				await this.triggerStart(workspace, workspaceName, progress);

src/settings/cli.ts

@@ -12,12 +12,21 @@ export type CliAuth =
 	| { mode: "url"; url: string };
 
 /**
- * Returns the raw global flags from user configuration.
+ * Returns the user's `coder.globalFlags` with `${env:VAR}` references
+ * substituted from `process.env`. Missing variables resolve to an empty
+ * string, matching VS Code's behaviour for built-in `${env:VAR}` sites.
  */
-export function getGlobalFlagsRaw(
+export function getUserGlobalFlags(
 	configs: Pick<WorkspaceConfiguration, "get">,
 ): string[] {
-	return configs.get<string[]>("coder.globalFlags", []);
+	return configs
+		.get<string[]>("coder.globalFlags", [])
+		.map((flag) =>
+			flag.replace(
+				/\$\{env:([A-Za-z_][A-Za-z0-9_]*)\}/g,
+				(_, name: string) => process.env[name] ?? "",
+			),
+		);
 }
 
 /**
@@ -52,26 +61,25 @@ function buildGlobalFlags(
 			? ["--url", esc(auth.url)]
 			: ["--global-config", esc(auth.configDir)];
 
-	const raw = getGlobalFlagsRaw(configs);
-	const filtered = stripManagedFlags(raw);
+	const filtered = stripManagedFlags(getUserGlobalFlags(configs));
 
 	return [...filtered, ...authFlags, ...getHeaderArgs(configs)];
 }
 
-function stripManagedFlags(rawFlags: string[]): string[] {
+function stripManagedFlags(flags: string[]): string[] {
 	const filtered: string[] = [];
-	for (let i = 0; i < rawFlags.length; i++) {
-		if (isFlag(rawFlags[i], "--use-keyring")) {
+	for (let i = 0; i < flags.length; i++) {
+		if (isFlag(flags[i], "--use-keyring")) {
 			continue;
 		}
-		if (isFlag(rawFlags[i], "--global-config")) {
+		if (isFlag(flags[i], "--global-config")) {
 			// Skip the next item too when the value is a separate entry.
-			if (rawFlags[i] === "--global-config") {
+			if (flags[i] === "--global-config") {
 				i++;
 			}
 			continue;
 		}
-		filtered.push(rawFlags[i]);
+		filtered.push(flags[i]);
 	}
 	return filtered;
 }

src/util.ts

@@ -212,7 +212,8 @@ export function escapeCommandArg(arg: string): string {
  */
 export function escapeShellArg(arg: string): string {
 	if (os.platform() === "win32") {
-		return escapeCommandArg(arg).replace(/%/g, "%%");
+		const escaped = arg.replace(/"/g, '""').replace(/%/g, "%%");
+		return `"${escaped}"`;
 	}
 	return `'${arg.replace(/'/g, "'\\''")}'`;
 }

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 () => {
@@ -312,21 +307,6 @@ describe("parameter prompt validation", () => {
 			input: "abc",
 			expected: "Must be a number",
 		},
-		{
-			kind: "regex mismatch with default message",
-			param: { validation_regex: "^x" },
-			input: "y",
-			expected: "Invalid format",
-		},
-		{
-			kind: "regex mismatch with {value} substitution",
-			param: {
-				validation_regex: "^x",
-				validation_error: "Value {value} is not allowed",
-			},
-			input: "y",
-			expected: "Value y is not allowed",
-		},
 		{
 			kind: "number out-of-range with {min}/{max} substitution",
 			param: {
@@ -345,6 +325,13 @@ describe("parameter prompt validation", () => {
 		});
 	});
 
+	it("does not evaluate validation_regex client-side (ReDoS guard)", async () => {
+		await withInputBox({ name: "r", validation_regex: "^(a+)+$" }, (qi) => {
+			qi.change("aaaaaaaaaaaaaaaaaaaaaaaaaaa!");
+			expect(qi.mock.validationMessage).toBe("");
+		});
+	});
+
 	it("treats JSON null on validation_min/max as unset", async () => {
 		await withInputBox(
 			{

test/unit/api/workspace.test.ts

@@ -106,9 +106,11 @@ function controlSpawn() {
 	const proc = new EventEmitter() as EventEmitter & {
 		stdout: EventEmitter;
 		stderr: EventEmitter;
+		stdin: { end: ReturnType<typeof vi.fn> };
 	};
 	proc.stdout = new EventEmitter();
 	proc.stderr = new EventEmitter();
+	proc.stdin = { end: vi.fn() };
 	const { promise: spawned, resolve: resolveSpawned } =
 		Promise.withResolvers<void>();
 	vi.mocked(spawn).mockImplementation(() => {
@@ -191,6 +193,23 @@ describe("updateWorkspace", () => {
 		vi.clearAllMocks();
 	});
 
+	it("runs coder update and resolves with the refreshed workspace", async () => {
+		const { ctx, restClient, finalWorkspace } = createUpdateCtx();
+		const sp = controlSpawn();
+
+		const result = updateWorkspace(ctx);
+		await sp.close(0);
+
+		await expect(result).resolves.toBe(finalWorkspace);
+		expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [
+			"--url",
+			"https://test.coder.com",
+			"update",
+			"testuser/test-workspace",
+		]);
+		expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id);
+	});
+
 	it("rejects when the process exits non-zero", async () => {
 		const { ctx, restClient } = createUpdateCtx();
 		const sp = controlSpawn();
@@ -264,10 +283,15 @@ describe("startWorkspace", () => {
 		await sp.close(0);
 
 		await expect(result).resolves.toBe(finalWorkspace);
-		expect(spawn).toHaveBeenCalledWith(
-			'"/usr/bin/coder" --url "https://test.coder.com" start --yes --reason vscode_connection testuser/test-workspace',
-			expect.objectContaining({ shell: true }),
-		);
+		expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [
+			"--url",
+			"https://test.coder.com",
+			"start",
+			"--yes",
+			"--reason",
+			"vscode_connection",
+			"testuser/test-workspace",
+		]);
 		expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id);
 	});
 
@@ -289,9 +313,12 @@ describe("startWorkspace", () => {
 		await sp.close(0);
 		await result;
 
-		expect(spawn).toHaveBeenCalledWith(
-			'"/usr/bin/coder" --url "https://test.coder.com" start --yes testuser/test-workspace',
-			expect.objectContaining({ shell: true }),
-		);
+		expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [
+			"--url",
+			"https://test.coder.com",
+			"start",
+			"--yes",
+			"testuser/test-workspace",
+		]);
 	});
 });