From fe65639d25c61a51620a5586fb534720fdc5e462 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 12 May 2026 23:08:10 +0300 Subject: [PATCH] feat: unify CLI execution and never spawn a shell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drops the last Node-level \`shell: true\` from the extension and replaces the behaviours we relied on the shell for with explicit expansion. Spawns \`coder\` as an argv array (\`spawn(binary, args)\`) instead of a quoted command string through a shell. Args reach \`coder\` byte-for-byte: no Windows \`cmd.exe\` metacharacter escaping surface for server-supplied template parameter values, no surprise re-parsing on Linux/macOS, one code path across platforms. Also reinstates \`proc.on("error", reject)\` so a missing/unexecutable binary surfaces via the spawn error event (Node emits \`error\` for ENOENT/EACCES when there is no shell) instead of hanging the progress dialog. Same treatment: \`spawn(binary, argv, { detached })\` instead of \`spawn(cmdString, { shell: true, detached })\`. The process-group setup for Ctrl+C handling comes from \`detached\` alone — the shell wrapper was not contributing anything. \`ping\` now uses \`getGlobalFlags\` (raw) and passes the workspace name unescaped, matching the rest of the codebase. Two surfaces still feed a shell because the consumer runs one: - \`openAppStatusTerminal\` (VS Code \`Terminal\` → user's shell). - \`buildProxyCommand\` (string handed to OpenSSH's \`ProxyCommand\`). Both still use \`getGlobalShellFlags\` + the platform-appropriate escaper. That's correct — they're outside Node's \`shell: true\` and escaping is required by the consuming shell. With no shell to expand values, the extension now substitutes: - \`\${env:VAR}\` from \`process.env\` (missing → empty, matching VS Code's own \`\${env:VAR}\` semantics). - Leading \`~\` and \`\${userHome}\` from \`os.homedir()\`. For \`--flag=value\` entries the expansion is scoped to the value half so \`--cfg=~/coder\` works. Substitution lives in \`getUserGlobalFlags\`, which feeds both \`getGlobalFlags\` and \`getGlobalShellFlags\`, so every site that reads the user's flags gets the same expansion. The doc in package.json spells out exactly which forms are supported. - \`cliConfig.test.ts\`: existing \`\${env:VAR}\` test plus a new test covering tilde / \`\${userHome}\` expansion (bare \`~\` entry, \`--flag=~/value\`, \`\${userHome}\` mid-string, mid-value tildes left alone). - The test-side \`shellQuote\` mirror added during the temporary shell-revert is dropped along with its tests — no caller needs it now that no test asserts an \`escapeShellArg\` output. \`getGlobalShellFlags\`'s comment loses the \`spawn({ shell: true })\` mention since that no longer exists; only \`terminal.sendText\` and SSH \`ProxyCommand\` remain as shell contexts. --- package.json | 2 +- src/api/updateParameters.ts | 4 +- src/api/workspace.ts | 13 ++++--- src/core/cliExec.ts | 16 ++++---- src/settings/cli.ts | 30 +++++++++++++-- test/unit/api/updateParameters.test.ts | 17 +++------ test/unit/api/workspace.test.ts | 51 +++++++++++++++++++------- test/unit/cliConfig.test.ts | 43 ++++++++++++++++++++++ test/utils/platform.test.ts | 50 +------------------------ test/utils/platform.ts | 12 ------ 10 files changed, 129 insertions(+), 109 deletions(-) diff --git a/package.json b/package.json index 1e652166..dbb01c79 100644 --- a/package.json +++ b/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. The extension does expand:\n- `${env:VAR}` → the value of `VAR` in the extension host's environment (missing variables resolve to an empty string).\n- A leading `~` or `${userHome}` → your home directory. For `--flag=value` entries the path expansion applies to the value half so `--cfg=~/coder` works.\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" diff --git a/src/api/updateParameters.ts b/src/api/updateParameters.ts index 34dc39b1..f6cde17f 100644 --- a/src/api/updateParameters.ts +++ b/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; } diff --git a/src/api/workspace.ts b/src/api/workspace.ts index a0e24dd6..3ff82711 100644 --- a/src/api/workspace.ts +++ b/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 { 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 { 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(); diff --git a/src/core/cliExec.ts b/src/core/cliExec.ts index 7f92d86e..3748fbee 100644 --- a/src/core/cliExec.ts +++ b/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(); const closeEmitter = new vscode.EventEmitter(); - 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, }); diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 2fe70826..a22bf0b9 100644 --- a/src/settings/cli.ts +++ b/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,36 @@ export type CliAuth = | { mode: "global-config"; configDir: string } | { mode: "url"; url: string }; -/** Returns the user's `coder.globalFlags` as configured, with no expansion. */ +/** + * Returns the user's `coder.globalFlags` with `${env:VAR}` references + * substituted from `process.env` (missing vars become empty, matching + * VS Code's built-in `${env:VAR}` behaviour) and `~` / `${userHome}` + * expanded to the home directory. For `--flag=value` entries the path + * expansion applies to the value half so `--cfg=~/coder` works. + */ export function getUserGlobalFlags( configs: Pick, ): string[] { - return configs.get("coder.globalFlags", []); + return configs + .get("coder.globalFlags", []) + .map((flag) => + flag.replace( + /\$\{env:([A-Za-z_][A-Za-z0-9_]*)\}/g, + (_, name: string) => process.env[name] ?? "", + ), + ) + .map(expandFlagPath); +} + +/** Applies `expandPath` to the value half of `--flag=value`, or the whole entry. */ +function expandFlagPath(flag: string): string { + 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, auth: CliAuth, diff --git a/test/unit/api/updateParameters.test.ts b/test/unit/api/updateParameters.test.ts index 2b67b2fa..f8d83f23 100644 --- a/test/unit/api/updateParameters.test.ts +++ b/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,7 +193,7 @@ 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(); @@ -203,10 +201,7 @@ describe("collectUpdateParameters", () => { 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 () => { diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index 4bc4eff0..2a1efb96 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -6,8 +6,6 @@ import { LazyStream, startWorkspace, updateWorkspace } from "@/api/workspace"; import { workspace as createWorkspace } from "@repo/mocks"; -import { shellQuote } from "../../utils/platform"; - import type { Api } from "coder/site/src/api/api"; import type { Workspace, @@ -129,6 +127,10 @@ function controlSpawn() { await spawned; proc.emit("close", exitCode, signal ?? null); }, + async error(err: Error) { + await spawned; + proc.emit("error", err); + }, }; } @@ -204,10 +206,12 @@ describe("updateWorkspace", () => { await sp.close(0); await expect(result).resolves.toBe(finalWorkspace); - expect(spawn).toHaveBeenCalledWith( - `"/usr/bin/coder" --url "https://test.coder.com" update ${shellQuote("testuser/test-workspace")}`, - { shell: true }, - ); + expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ + "--url", + "https://test.coder.com", + "update", + "testuser/test-workspace", + ]); expect(sp.stdinEnd).toHaveBeenCalled(); expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id); }); @@ -225,6 +229,17 @@ describe("updateWorkspace", () => { expect(restClient.getWorkspace).not.toHaveBeenCalled(); }); + it("rejects when spawn emits an error (e.g. missing binary)", async () => { + const { ctx, restClient } = createUpdateCtx(); + const sp = controlSpawn(); + + const result = updateWorkspace(ctx); + await sp.error(new Error("spawn /usr/bin/coder ENOENT")); + + await expect(result).rejects.toThrow(/ENOENT/); + expect(restClient.getWorkspace).not.toHaveBeenCalled(); + }); + it("reports the terminating signal when the process is killed", async () => { const { ctx } = createUpdateCtx(); const sp = controlSpawn(); @@ -295,10 +310,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 ${shellQuote("testuser/test-workspace")}`, - { 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); }); @@ -320,9 +340,12 @@ describe("startWorkspace", () => { await sp.close(0); await result; - expect(spawn).toHaveBeenCalledWith( - `"/usr/bin/coder" --url "https://test.coder.com" start --yes ${shellQuote("testuser/test-workspace")}`, - { shell: true }, - ); + expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ + "--url", + "https://test.coder.com", + "start", + "--yes", + "testuser/test-workspace", + ]); }); }); diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index 9481827c..6256f7e8 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -231,6 +231,49 @@ describe("cliConfig", () => { "--disable-direct-connections", ]); }); + + it("substitutes ${env:VAR} from process.env", () => { + const restore = process.env.CODER_TEST_VAR; + process.env.CODER_TEST_VAR = "from-env"; + try { + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", [ + "--prefix=${env:CODER_TEST_VAR}", + "${env:CODER_MISSING_VAR}-suffix", + ]); + + expect(getUserGlobalFlags(config)).toStrictEqual([ + "--prefix=from-env", + "-suffix", + ]); + } finally { + if (restore === undefined) { + delete process.env.CODER_TEST_VAR; + } else { + process.env.CODER_TEST_VAR = restore; + } + } + }); + + it("expands ~ and ${userHome} in flag values", () => { + vi.mocked(os.homedir).mockReturnValue("/home/coder"); + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", [ + "~/bare", + "--cfg=~/coder", + "--state=${userHome}/state", + "--literal=value~with~tildes", + ]); + + expect(getUserGlobalFlags(config)).toStrictEqual([ + "/home/coder/bare", + "--cfg=/home/coder/coder", + "--state=/home/coder/state", + // Tildes mid-value are left alone (only ~ at the start of the + // value half is expanded). + "--literal=value~with~tildes", + ]); + }); }); describe("getSshFlags", () => { diff --git a/test/utils/platform.test.ts b/test/utils/platform.test.ts index 9d9d5307..35bbc866 100644 --- a/test/utils/platform.test.ts +++ b/test/utils/platform.test.ts @@ -3,15 +3,7 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { promisify } from "node:util"; -import { - afterEach, - beforeAll, - beforeEach, - describe, - expect, - it, - vi, -} from "vitest"; +import { beforeAll, describe, expect, it } from "vitest"; import { expectPathsEqual, @@ -19,7 +11,6 @@ import { isWindows, printCommand, printEnvCommand, - shellQuote, shimExecFile, writeExecutable, writeStdoutJs, @@ -168,43 +159,4 @@ describe("platform utils", () => { expect(mod.spawn).toBe(cp.spawn); }); }); - - describe("shellQuote", () => { - const platformSpy = vi.spyOn(os, "platform"); - afterEach(() => platformSpy.mockReset()); - - describe("on Unix", () => { - beforeEach(() => platformSpy.mockReturnValue("linux")); - - it("wraps in single quotes", () => { - expect(shellQuote("env=dev")).toBe("'env=dev'"); - }); - - it("escapes single quotes via the '\\'' sequence", () => { - expect(shellQuote("it's fine")).toBe("'it'\\''s fine'"); - }); - - it("keeps $VAR, $(...), and backticks literal inside the quotes", () => { - expect(shellQuote("$(echo pwned)")).toBe("'$(echo pwned)'"); - }); - }); - - describe("on Windows", () => { - beforeEach(() => platformSpy.mockReturnValue("win32")); - - it("wraps in double quotes", () => { - expect(shellQuote("env=dev")).toBe('"env=dev"'); - }); - - it("doubles embedded double quotes", () => { - expect(shellQuote('regions=["us","eu"]')).toBe( - '"regions=[""us"",""eu""]"', - ); - }); - - it("doubles percent signs to block %VAR% expansion", () => { - expect(shellQuote("%PATH%")).toBe('"%%PATH%%"'); - }); - }); - }); }); diff --git a/test/utils/platform.ts b/test/utils/platform.ts index d4827335..5edb8587 100644 --- a/test/utils/platform.ts +++ b/test/utils/platform.ts @@ -124,18 +124,6 @@ export function quoteCommand(value: string): string { return `${quote}${value}${quote}`; } -/** - * Test-side mirror of `escapeShellArg` from src/util.ts. Wraps in single - * quotes on Unix and double quotes (with `"` doubled and `%` doubled) on - * Windows. - */ -export function shellQuote(value: string): string { - if (isWindows()) { - return `"${value.replace(/"/g, '""').replace(/%/g, "%%")}"`; - } - return `'${value.replace(/'/g, "'\\''")}'`; -} - export function expectPathsEqual(actual: string, expected: string) { expect(normalizePath(actual)).toBe(normalizePath(expected)); }