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 9e506fa8aa..9e3f781138 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\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" 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..f372400799 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, }); @@ -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)"; 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 2fe70826ec..5086e6d628 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,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, ): string[] { - return configs.get("coder.globalFlags", []); + 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`, `spawn({ shell: true })`). */ +/** Flags for shell contexts (`terminal.sendText`, SSH `ProxyCommand`). */ export function getGlobalShellFlags( configs: Pick, 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)]; } diff --git a/src/util.ts b/src/util.ts index 22a63800a3..ed9b6f648b 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 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`\"`)}"`; } /** 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/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..ec51c51b51 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,19 @@ 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")); + // 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(); + }); + it("reports the terminating signal when the process is killed", async () => { const { ctx } = createUpdateCtx(); const sp = controlSpawn(); @@ -295,10 +312,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 +342,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..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,11 +239,55 @@ describe("cliConfig", () => { "--disable-direct-connections", ]); - expect(getUserGlobalFlags(config)).toStrictEqual([ + expect(getExpandedUserGlobalFlags(config)).toStrictEqual([ "--verbose", "--disable-direct-connections", ]); }); + + 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(getExpandedUserGlobalFlags(config)).toStrictEqual([ + "--prefix=from-env", + "-suffix", + ]); + }); + }); + + 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(getExpandedUserGlobalFlags(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/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 745d7e9788..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", () => { @@ -291,6 +322,39 @@ 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 + "}"; + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("substitutes a present env var", () => { + vi.stubEnv(envKey, "/data"); + expect(expandPath(`${ref}/foo`)).toBe("/data/foo"); + }); + + it("replaces a missing env var with an empty string", () => { + vi.stubEnv(envKey, undefined); + expect(expandPath(`prefix-${ref}-suffix`)).toBe("prefix--suffix"); + }); + + it("substitutes multiple occurrences in one string", () => { + vi.stubEnv(envKey, "data"); + expect(expandPath(`${ref}/${ref}`)).toBe("data/data"); + }); + + it("expands tilde or ${userHome} that appears inside the env value", () => { + vi.stubEnv(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", () => { 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)); }