From 604d77a1f2eeec65b0d4bedd95c9049a9a40c03e Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 12 May 2026 23:08:10 +0300 Subject: [PATCH 1/4] 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 9e506fa8aa..3998967bf3 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 34dc39b155..f6cde17ffc 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 a0e24dd64d..3ff8271108 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 7f92d86ea7..3748fbeea8 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 2fe70826ec..a22bf0b9f8 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 2b67b2fa7b..f8d83f2301 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 4bc4eff0a6..2a1efb96b0 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 9481827c04..6256f7e86b 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 9d9d530724..35bbc86647 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 d48273351e..5edb858700 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)); } From e6ef3572c6c4870db6a658fce6ccd0f46c344530 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Fri, 15 May 2026 13:20:46 +0300 Subject: [PATCH 2/4] feat: apply ${env:VAR} substitution across path-like settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds `${env:VAR}` substitution into `expandPath`, so every setting that goes through it (TLS paths, binary destination, proxy log directory, global flags, SSH config file) gets the same env-var resolution. Env substitution runs before tilde/`${userHome}` expansion, so env values may themselves contain those tokens. For `coder.globalFlags`, the inline regex in `getUserGlobalFlags` is dropped — env substitution now flows through `expandPath` via the existing `--flag=value` value-half scoping, matching how tilde works. Adds env-var tests to `expandPath` and a substitution note to the `coder.tlsCertFile`/`tlsKeyFile`/`tlsCaFile`/`tlsAltHost`/ `binaryDestination`/`proxyLogDirectory` setting descriptions; tightens the `coder.globalFlags` description. --- package.json | 14 +++++++------- src/settings/cli.ts | 31 +++++++++---------------------- src/util.ts | 17 ++++++++++++----- test/unit/util.test.ts | 40 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 34 deletions(-) diff --git a/package.json b/package.json index 3998967bf3..9ecbe7172d 100644 --- a/package.json +++ b/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\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#`.", + "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\nItems run without a shell, so `$VAR` and `%VAR%` are **not** expanded. For `--flag=value` items the substitutions below apply to the value half only (so `--cfg=~/coder` works):\n- `${env:VAR}` becomes the value of `VAR` in the extension host's environment (unset becomes empty).\n- `${userHome}` (anywhere) or a leading `~` becomes your home directory.\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" diff --git a/src/settings/cli.ts b/src/settings/cli.ts index a22bf0b9f8..d43a103347 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -12,32 +12,19 @@ export type CliAuth = | { mode: "url"; url: string }; /** - * 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. + * 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 getUserGlobalFlags( configs: Pick, ): string[] { - 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)); + return configs.get("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`, SSH `ProxyCommand`). */ diff --git a/src/util.ts b/src/util.ts index 22a63800a3..594b66d213 100644 --- a/src/util.ts +++ b/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 detilded = expanded.startsWith("~") + ? userHome + expanded.substring("~".length) + : expanded; + return detilded.replaceAll("${userHome}", userHome); } /** diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index 745d7e9788..13ae433413 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -291,6 +291,46 @@ describe("expandPath", () => { it("expands both tilde and ${userHome}", () => { expect(expandPath("~/${userHome}/foo")).toBe(`${home}/${home}/foo`); }); + + describe("${env:VAR}", () => { + const envKey = "CODER_EXPAND_PATH_TEST"; + const ref = "${env:" + envKey + "}"; + let original: string | undefined; + beforeEach(() => { + original = process.env[envKey]; + }); + afterEach(() => { + if (original === undefined) { + delete process.env[envKey]; + } else { + process.env[envKey] = original; + } + }); + + it("substitutes a present env var", () => { + process.env[envKey] = "/data"; + expect(expandPath(`${ref}/foo`)).toBe("/data/foo"); + }); + + it("replaces a missing env var with an empty string", () => { + delete process.env[envKey]; + expect(expandPath(`prefix-${ref}-suffix`)).toBe("prefix--suffix"); + }); + + it("substitutes multiple occurrences in one string", () => { + process.env[envKey] = "data"; + expect(expandPath(`${ref}/${ref}`)).toBe("data/data"); + }); + + it("expands tilde or ${userHome} that appears inside the env value", () => { + process.env[envKey] = "~/projects"; + expect(expandPath(`${ref}/x`)).toBe(`${home}/projects/x`); + }); + + it("ignores ${env:...} with invalid names", () => { + expect(expandPath("${env:1BAD}/x")).toBe("${env:1BAD}/x"); + }); + }); }); describe("findPort", () => { From 1064297c1355f876b7a6d533333698cb3b76acb3 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Fri, 15 May 2026 14:36:08 +0300 Subject: [PATCH 3/4] fix: address PR #949 review feedback - Apply per-entry escaping to user globalFlags in shell context so expansion-introduced whitespace (`${userHome}`, `${env:VAR}`) stays in one shell token through buildProxyCommand and openAppStatusTerminal. - Switch escapeCommandArg to a Python shlex.quote-style whitelist so shell metacharacters force quoting while flag-only tokens stay raw. - Suppress the duplicate "Process exited" line in spawnCliInTerminal when error fires before close (ENOENT/EACCES path). - Restructure coder.globalFlags markdownDescription so the substitution rules read top-down instead of being gated behind --flag=value. - Add ping spawn tests covering raw argv, user flag pass-through, and the ENOENT no-duplicate-output guarantee. - Model real Node `error` then `close(null, null)` sequence in the updateWorkspace ENOENT test. - Rename getUserGlobalFlags to getExpandedUserGlobalFlags to surface the expandPath transformation. - Rename detilded to tildeExpanded. - Migrate env-substitution tests to vi.stubEnv / vi.unstubAllEnvs. --- package.json | 2 +- src/core/cliExec.ts | 7 ++- src/remote/remote.ts | 4 +- src/settings/cli.ts | 8 ++- src/util.ts | 23 ++++++-- test/mocks/vscode.runtime.ts | 1 + test/unit/api/workspace.test.ts | 2 + test/unit/cliConfig.test.ts | 70 +++++++++++++---------- test/unit/core/cliExec.test.ts | 99 +++++++++++++++++++++++++++++++-- test/unit/util.test.ts | 62 ++++++++++++++------- 10 files changed, 214 insertions(+), 64 deletions(-) diff --git a/package.json b/package.json index 9ecbe7172d..757c560b95 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, in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nItems run without a shell, so `$VAR` and `%VAR%` are **not** expanded. For `--flag=value` items the substitutions below apply to the value half only (so `--cfg=~/coder` works):\n- `${env:VAR}` becomes the value of `VAR` in the extension host's environment (unset becomes empty).\n- `${userHome}` (anywhere) or a leading `~` becomes your home directory.\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#`.", + "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\nItems run without a shell, so `$VAR` and `%VAR%` are **not** expanded. The following substitutions are applied to each item:\n- `${env:VAR}` becomes the value of `VAR` in the extension host's environment (unset becomes empty).\n- `${userHome}` (anywhere) or a leading `~` becomes your home directory.\n\nFor `--flag=value` items these substitutions apply to the value half only, 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" diff --git a/src/core/cliExec.ts b/src/core/cliExec.ts index 3748fbeea8..f372400799 100644 --- a/src/core/cliExec.ts +++ b/src/core/cliExec.ts @@ -264,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)"; diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 15adbfd9de..0b3607859e 100644 --- a/src/remote/remote.ts +++ b/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", diff --git a/src/settings/cli.ts b/src/settings/cli.ts index d43a103347..5086e6d628 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -16,7 +16,7 @@ export type CliAuth = * `--flag=value` entries the substitution is scoped to the value half so * `--cfg=~/coder` works without rewriting the flag name. */ -export function getUserGlobalFlags( +export function getExpandedUserGlobalFlags( configs: Pick, ): string[] { return configs.get("coder.globalFlags", []).map((flag) => { @@ -56,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)]; } diff --git a/src/util.ts b/src/util.ts index 594b66d213..ed9b6f648b 100644 --- a/src/util.ts +++ b/src/util.ts @@ -142,10 +142,10 @@ export function expandPath(input: string): string { (_, name: string) => process.env[name] ?? "", ); const userHome = os.homedir(); - const detilded = expanded.startsWith("~") + const tildeExpanded = expanded.startsWith("~") ? userHome + expanded.substring("~".length) : expanded; - return detilded.replaceAll("${userHome}", userHome); + return tildeExpanded.replaceAll("${userHome}", userHome); } /** @@ -209,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`\"`)}"`; } /** diff --git a/test/mocks/vscode.runtime.ts b/test/mocks/vscode.runtime.ts index b22a86f0bf..5b2d6ca45b 100644 --- a/test/mocks/vscode.runtime.ts +++ b/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() })), }; diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index 2a1efb96b0..ec51c51b51 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -235,6 +235,8 @@ describe("updateWorkspace", () => { const result = updateWorkspace(ctx); await sp.error(new Error("spawn /usr/bin/coder ENOENT")); + // Real Node fires `error` then `close(null, null)` on ENOENT. + await sp.close(null); await expect(result).rejects.toThrow(/ENOENT/); expect(restClient.getWorkspace).not.toHaveBeenCalled(); diff --git a/test/unit/cliConfig.test.ts b/test/unit/cliConfig.test.ts index 6256f7e86b..2d6be51c4b 100644 --- a/test/unit/cliConfig.test.ts +++ b/test/unit/cliConfig.test.ts @@ -1,14 +1,14 @@ import * as os from "node:os"; import * as semver from "semver"; -import { it, expect, describe, vi } from "vitest"; +import { afterEach, beforeEach, it, expect, describe, vi } from "vitest"; import { featureSetForVersion } from "@/featureSet"; import { type CliAuth, + getExpandedUserGlobalFlags, getGlobalFlags, getGlobalShellFlags, getSshFlags, - getUserGlobalFlags, isKeyringEnabled, resolveCliAuth, } from "@/settings/cli"; @@ -37,12 +37,12 @@ describe("cliConfig", () => { { scenario: "global-config mode", auth: globalConfigAuth, - expectedAuthFlags: ["--global-config", '"/config/dir"'], + expectedAuthFlags: ["--global-config", "/config/dir"], }, { scenario: "url mode", auth: urlAuth, - expectedAuthFlags: ["--url", '"https://dev.coder.com"'], + expectedAuthFlags: ["--url", "https://dev.coder.com"], }, ])( "should return auth flags for $scenario", @@ -65,7 +65,7 @@ describe("cliConfig", () => { "--verbose", "--disable-direct-connections", "--global-config", - '"/config/dir"', + "/config/dir", ]); }); @@ -83,7 +83,7 @@ describe("cliConfig", () => { "--verbose", "--disable-direct-connections", "--global-config", - '"/config/dir"', + "/config/dir", ]); }, ); @@ -118,12 +118,12 @@ describe("cliConfig", () => { expect(getGlobalShellFlags(config, globalConfigAuth)).toStrictEqual([ "-v", "--global-config", - '"/config/dir"', + "/config/dir", ]); expect(getGlobalShellFlags(config, urlAuth)).toStrictEqual([ "-v", "--url", - '"https://dev.coder.com"', + "https://dev.coder.com", ]); }, ); @@ -136,7 +136,7 @@ describe("cliConfig", () => { "--global-configs", "--use-keyrings", "--global-config", - '"/config/dir"', + "/config/dir", ]); }); @@ -144,12 +144,12 @@ describe("cliConfig", () => { { scenario: "global-config mode", auth: globalConfigAuth, - expectedAuthFlags: ["--global-config", '"/config/dir"'], + expectedAuthFlags: ["--global-config", "/config/dir"], }, { scenario: "url mode", auth: urlAuth, - expectedAuthFlags: ["--url", '"https://dev.coder.com"'], + expectedAuthFlags: ["--url", "https://dev.coder.com"], }, ])( "should not filter header-command flags ($scenario)", @@ -165,7 +165,7 @@ describe("cliConfig", () => { expect(getGlobalShellFlags(config, auth)).toStrictEqual([ "-v", - "--header-command custom", // ignored by CLI + '"--header-command custom"', // ignored by CLI "--no-feature-warning", ...expectedAuthFlags, "--header-command", @@ -173,6 +173,19 @@ describe("cliConfig", () => { ]); }, ); + + it("quotes flags whose expanded value contains whitespace", () => { + vi.mocked(os.homedir).mockReturnValue("C:\\Users\\John Doe"); + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", ["--cfg=${userHome}/coder"]); + + // Without per-entry escaping the space splits the shell command. + expect(getGlobalShellFlags(config, globalConfigAuth)).toStrictEqual([ + '"--cfg=C:\\Users\\John Doe/coder"', + "--global-config", + "/config/dir", + ]); + }); }); describe("getGlobalFlags", () => { @@ -212,11 +225,11 @@ describe("cliConfig", () => { }); }); - describe("getUserGlobalFlags", () => { + describe("getExpandedUserGlobalFlags", () => { it("returns empty array when no global flags configured", () => { const config = new MockConfigurationProvider(); - expect(getUserGlobalFlags(config)).toStrictEqual([]); + expect(getExpandedUserGlobalFlags(config)).toStrictEqual([]); }); it("returns global flags from config", () => { @@ -226,33 +239,34 @@ describe("cliConfig", () => { "--disable-direct-connections", ]); - expect(getUserGlobalFlags(config)).toStrictEqual([ + expect(getExpandedUserGlobalFlags(config)).toStrictEqual([ "--verbose", "--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 { + describe("env substitution", () => { + beforeEach(() => { + vi.stubEnv("CODER_TEST_VAR", "from-env"); + vi.stubEnv("CODER_MISSING_VAR", undefined); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("substitutes ${env:VAR} from process.env", () => { const config = new MockConfigurationProvider(); config.set("coder.globalFlags", [ "--prefix=${env:CODER_TEST_VAR}", "${env:CODER_MISSING_VAR}-suffix", ]); - expect(getUserGlobalFlags(config)).toStrictEqual([ + expect(getExpandedUserGlobalFlags(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", () => { @@ -265,7 +279,7 @@ describe("cliConfig", () => { "--literal=value~with~tildes", ]); - expect(getUserGlobalFlags(config)).toStrictEqual([ + expect(getExpandedUserGlobalFlags(config)).toStrictEqual([ "/home/coder/bare", "--cfg=/home/coder/coder", "--state=/home/coder/state", diff --git a/test/unit/core/cliExec.test.ts b/test/unit/core/cliExec.test.ts index bec8767ade..823f951b9f 100644 --- a/test/unit/core/cliExec.test.ts +++ b/test/unit/core/cliExec.test.ts @@ -1,7 +1,17 @@ -import fs from "fs/promises"; -import os from "os"; -import path from "path"; -import { beforeAll, describe, expect, it, vi } from "vitest"; +import { EventEmitter } from "node:events"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; +import * as vscode from "vscode"; import { MockConfigurationProvider } from "../../mocks/testHelpers"; import { @@ -14,13 +24,16 @@ import { import type { CliEnv } from "@/core/cliExec"; // Shim execFile so .js test scripts are run through node cross-platform. +// Spawn is replaced with vi.fn() so the ping tests can intercept it without +// starting real child processes. vi.mock("node:child_process", async (importOriginal) => { const { shimExecFile } = await import("../../utils/platform"); - return shimExecFile(await importOriginal()); + return { ...shimExecFile(await importOriginal()), spawn: vi.fn() }; }); // Import after mock so the module picks up the shimmed execFile. const cliExec = await import("@/core/cliExec"); +const { spawn } = await import("node:child_process"); describe("cliExec", () => { const tmp = path.join(os.tmpdir(), "vscode-coder-tests-cliExec"); @@ -232,4 +245,80 @@ describe("cliExec", () => { ).rejects.toThrow("workspace not found"); }); }); + + describe("ping", () => { + let writeListeners: string[]; + let proc: EventEmitter & { + stdout: EventEmitter; + stderr: EventEmitter; + pid: number; + kill: ReturnType; + }; + + beforeEach(() => { + vi.clearAllMocks(); + proc = Object.assign(new EventEmitter(), { + stdout: new EventEmitter(), + stderr: new EventEmitter(), + pid: 12345, + kill: vi.fn(), + }); + vi.mocked(spawn).mockReturnValue(proc as never); + + writeListeners = []; + vi.mocked(vscode.window.createTerminal).mockImplementation( + (opts: vscode.ExtensionTerminalOptions) => { + opts.pty.onDidWrite((s) => { + writeListeners.push(s); + }); + opts.pty.open?.(undefined); + return { show: vi.fn() } as unknown as vscode.Terminal; + }, + ); + }); + + afterEach(() => { + proc.removeAllListeners(); + }); + + it("spawns coder ping with raw argv (no shell, unescaped workspace name)", () => { + const { env } = setup({ mode: "url", url: "https://test.coder.com" }); + cliExec.ping(env, "owner/my workspace"); + + expect(spawn).toHaveBeenCalledWith( + env.binary, + ["--url", "https://test.coder.com", "ping", "owner/my workspace"], + expect.objectContaining({ detached: process.platform !== "win32" }), + ); + }); + + it("includes user global flags raw in the spawn argv", () => { + const { configs, env } = setup({ + mode: "global-config", + configDir: "/cfg", + }); + configs.set("coder.globalFlags", ["--verbose"]); + + cliExec.ping(env, "owner/ws"); + + expect(spawn).toHaveBeenCalledWith( + env.binary, + ["--verbose", "--global-config", "/cfg", "ping", "owner/ws"], + expect.objectContaining({ detached: process.platform !== "win32" }), + ); + }); + + it("reports ENOENT once even when `close` fires after `error`", () => { + const { env } = setup({ mode: "url", url: "https://test.coder.com" }); + cliExec.ping(env, "owner/ws"); + + // Real Node emits `error` then `close(null, null)` on missing binary. + proc.emit("error", new Error("spawn /missing/coder ENOENT")); + proc.emit("close", null, null); + + const output = writeListeners.join(""); + expect(output).toContain("Failed to start: spawn /missing/coder ENOENT"); + expect(output).not.toContain("Process exited"); + }); + }); }); diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index 13ae433413..2138832408 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -190,27 +190,58 @@ describe("countSubstring", () => { }); describe("escapeCommandArg", () => { - it("wraps simple string in quotes", () => { - expect(escapeCommandArg("hello")).toBe('"hello"'); + it("returns simple strings unquoted", () => { + expect(escapeCommandArg("hello")).toBe("hello"); }); - it("handles empty string", () => { + it("returns flag-style strings unquoted", () => { + expect(escapeCommandArg("--verbose")).toBe("--verbose"); + expect(escapeCommandArg("--cfg=/etc/coder")).toBe("--cfg=/etc/coder"); + }); + + it("quotes the empty string so it survives as a token", () => { expect(escapeCommandArg("")).toBe('""'); }); - it("escapes double quotes", () => { + it("escapes double quotes and wraps in quotes", () => { expect(escapeCommandArg('say "hello"')).toBe(String.raw`"say \"hello\""`); }); - it("preserves backslashes", () => { + it("quotes backslashes (POSIX escape char)", () => { expect(escapeCommandArg(String.raw`path\to\file`)).toBe( String.raw`"path\to\file"`, ); }); - it("handles string with spaces", () => { + it("quotes strings containing spaces", () => { expect(escapeCommandArg("hello world")).toBe('"hello world"'); }); + + it.each([["tab\there"], ["line\nbreak"], ["vtab\vhere"]])( + "quotes strings containing other whitespace: %j", + (input) => { + expect(escapeCommandArg(input)).toBe(`"${input}"`); + }, + ); + + it.each([ + ["foo&bar"], + ["foo;bar"], + ["foo|bar"], + ["foo(bar)"], + ["foobar"], + ["foo*bar"], + ["foo?bar"], + ["foo$bar"], + ["foo`bar"], + ["foo~bar"], + ["foo!bar"], + ["foo#bar"], + ["https://x.com?a=1&b=2"], + ])("quotes strings containing shell metacharacter: %j", (input) => { + expect(escapeCommandArg(input)).toBe(`"${input}"`); + }); }); describe("escapeShellArg", () => { @@ -295,35 +326,28 @@ describe("expandPath", () => { describe("${env:VAR}", () => { const envKey = "CODER_EXPAND_PATH_TEST"; const ref = "${env:" + envKey + "}"; - let original: string | undefined; - beforeEach(() => { - original = process.env[envKey]; - }); + afterEach(() => { - if (original === undefined) { - delete process.env[envKey]; - } else { - process.env[envKey] = original; - } + vi.unstubAllEnvs(); }); it("substitutes a present env var", () => { - process.env[envKey] = "/data"; + vi.stubEnv(envKey, "/data"); expect(expandPath(`${ref}/foo`)).toBe("/data/foo"); }); it("replaces a missing env var with an empty string", () => { - delete process.env[envKey]; + vi.stubEnv(envKey, undefined); expect(expandPath(`prefix-${ref}-suffix`)).toBe("prefix--suffix"); }); it("substitutes multiple occurrences in one string", () => { - process.env[envKey] = "data"; + vi.stubEnv(envKey, "data"); expect(expandPath(`${ref}/${ref}`)).toBe("data/data"); }); it("expands tilde or ${userHome} that appears inside the env value", () => { - process.env[envKey] = "~/projects"; + vi.stubEnv(envKey, "~/projects"); expect(expandPath(`${ref}/x`)).toBe(`${home}/projects/x`); }); From 40060708b574c044ce872f7172565b30170b428d Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 19 May 2026 11:34:43 +0300 Subject: [PATCH 4/4] fix changelog --- CHANGELOG.md | 15 +++++++++++++++ package.json | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e52bcd579..1cde23a473 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/package.json b/package.json index 757c560b95..9e3f781138 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, in order. Do **not** include the `coder` command itself. See the [CLI reference](https://coder.com/docs/reference/cli) for available global flags.\n\nItems run without a shell, so `$VAR` and `%VAR%` are **not** expanded. The following substitutions are applied to each item:\n- `${env:VAR}` becomes the value of `VAR` in the extension host's environment (unset becomes empty).\n- `${userHome}` (anywhere) or a leading `~` becomes your home directory.\n\nFor `--flag=value` items these substitutions apply to the value half only, 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#`.", + "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"