diff --git a/AGENTS.md b/AGENTS.md index 7ce91d68d3..f24fe89ab1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,8 +75,8 @@ deployment. Treat them as untrusted: - Validate names with `src/common/validation.ts` before they reach SSH hosts, paths, or the CLI. - In SSH `ProxyCommand` (the one shell context), build flags with - `getGlobalShellFlags`, which validates the URL and quotes it with - `escapeShellArg`. + `getGlobalShellFlags`, which validates the URL and quotes values with + `escapeProxyCommandArg` (open-remote-ssh splits it without a shell). ## Code style diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e351eb68f..9a86404f09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,17 @@ generally available. Existing values remain effective until the new setting is explicitly set in user settings. OAuth remains disabled by default. +### Fixed + +- Fix "Premature close" since v1.17.0 in Positron and other editors that use + the open source Remote - SSH extension (`open-remote-ssh`): the SSH + `ProxyCommand` single-quoted `--url` and `--global-config`, which these + editors pass to the CLI, and `--token=` merged with the next argument. +- `coder.headerCommand` now works with `open-remote-ssh` unless it contains `$`, + a backtick, `\`, `%` or a line break. +- On Windows, a `coder.headerCommand` containing `"` was split into several + arguments before it reached the CLI. + ## [v1.17.0](https://github.com/coder/vscode-coder/releases/tag/v1.17.0) 2026-10-13 ### Added diff --git a/src/common/strings.ts b/src/common/strings.ts index 61b13d768d..78ee991046 100644 --- a/src/common/strings.ts +++ b/src/common/strings.ts @@ -15,11 +15,14 @@ export function lowercase(value: T): Lowercase { * Not a universal shell-escape: `$VAR` / `$(...)` / `%VAR%` still expand * inside `"..."`. For untrusted values use {@link escapeShellArg}. * + * A leading, trailing or doubled `=` is quoted too: open-remote-ssh's + * ssh-config@5 treats an unquoted `=` as a separator. + * * @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 { - if (arg !== "" && /^[\w@%+,=:./-]+$/.test(arg)) { + if (/^[\w@%+,:./-]+(?:=[\w@%+,:./-]+)*$/.test(arg)) { return arg; } return `"${arg.replaceAll('"', String.raw`\"`)}"`; @@ -28,14 +31,29 @@ export function escapeCommandArg(arg: string): string { /** * Cross-platform shell quoting that blocks variable expansion. Use for * values from outside the user's local settings (e.g. server-controlled). - * The win32 branch targets Windows OpenSSH, which runs `ProxyCommand` - * without a shell and turns `%%` back into `%`. A shell such as cmd.exe or - * Git Bash's `sh` still expands inside those quotes. + * The win32 branch targets Windows OpenSSH, which starts `ProxyCommand` + * without a shell: `%%` becomes `%` and the CLI splits the line with the + * Windows argv rules (`\"`). Git Bash's `sh` still expands `$` inside. */ export function escapeShellArg(arg: string): string { if (os.platform() === "win32") { - const escaped = arg.replace(/"/g, '""').replace(/%/g, "%%"); + // `\\+` consumes backslash runs not before a quote, keeping this linear. + const escaped = arg + .replace(/(\\*)("|$)|\\+/g, (run, slashes?: string, quote?: string) => + slashes === undefined + ? run + : `${slashes}${slashes}${quote ? '\\"' : ""}`, + ) + .replace(/%/g, "%%"); return `"${escaped}"`; } return `'${arg.replace(/'/g, "'\\''")}'`; } + +/** + * Double quotes read the same in a shell and in open-remote-ssh, which splits + * `ProxyCommand` itself, unless the value needs escaping there. + */ +export function escapeProxyCommandArg(arg: string): string { + return /[$`\\%\r\n]/.test(arg) ? escapeShellArg(arg) : escapeCommandArg(arg); +} diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 86f4d3b12e..9cfe766596 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -1,7 +1,7 @@ import os from "node:os"; import { expandPath } from "../common/fs"; -import { escapeCommandArg, escapeShellArg } from "../common/strings"; +import { escapeCommandArg, escapeProxyCommandArg } from "../common/strings"; import { validateDeploymentUrl } from "../common/validation"; import { getHeaderArgs } from "./headers"; @@ -51,8 +51,8 @@ export function getGlobalShellFlags( // covers URLs that reach storage some other way. validateDeploymentUrl(auth.url); return [ - "--token=", - ...buildGlobalFlags(configs, auth, escapeCommandArg, escapeShellArg), + escapeCommandArg("--token="), + ...buildGlobalFlags(configs, auth, escapeCommandArg, escapeProxyCommandArg), ]; } diff --git a/src/settings/headers.ts b/src/settings/headers.ts index 7abbdd10ff..96defd7167 100644 --- a/src/settings/headers.ts +++ b/src/settings/headers.ts @@ -11,7 +11,7 @@ export function getHeaderCommand( return cmd || undefined; } -/** Returns `--header-command` args; shell callers pass `escapeShellArg` as `esc`. */ +/** Returns `--header-command` args, quoted with `esc`. */ export function getHeaderArgs( config: Pick, esc: (s: string) => string = (s) => s, diff --git a/test/unit/common/strings.test.ts b/test/unit/common/strings.test.ts index 9085096690..735cfc81ec 100644 --- a/test/unit/common/strings.test.ts +++ b/test/unit/common/strings.test.ts @@ -1,7 +1,11 @@ import os from "node:os"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { escapeCommandArg, escapeShellArg } from "@/common/strings"; +import { + escapeCommandArg, + escapeProxyCommandArg, + escapeShellArg, +} from "@/common/strings"; describe("escapeCommandArg", () => { it("returns simple strings unquoted", () => { @@ -17,6 +21,13 @@ describe("escapeCommandArg", () => { expect(escapeCommandArg("")).toBe('""'); }); + it.each([["--token="], ["=x"], ["x==y"]])( + "quotes a leading, trailing or doubled `=`: %j", + (input) => { + expect(escapeCommandArg(input)).toBe(`"${input}"`); + }, + ); + it("escapes double quotes and wraps in quotes", () => { expect(escapeCommandArg('say "hello"')).toBe(String.raw`"say \"hello\""`); }); @@ -85,20 +96,30 @@ describe("escapeShellArg", () => { expect(escapeShellArg("env=dev")).toBe('"env=dev"'); }); - it('doubles embedded `"`', () => { - expect(escapeShellArg('regions=["us","eu"]')).toBe( - '"regions=[""us"",""eu""]"', - ); + it.each([ + ['regions=["us","eu"]', String.raw`"regions=[\"us\",\"eu\"]"`], + ['C:\\a b\\ \\"x\\', '"C:\\a b\\ \\\\\\"x\\\\"'], + ])('escapes `"` and the backslashes before it in %j', (value, expected) => { + expect(escapeShellArg(value)).toBe(expected); }); it("doubles `%` so OpenSSH token expansion keeps it literal", () => { expect(escapeShellArg("%PATH%")).toBe('"%%PATH%%"'); }); + }); +}); - it("keeps cmd metachars inside the quoted region", () => { - expect(escapeShellArg('foo" & calc.exe & "x')).toBe( - '"foo"" & calc.exe & ""x"', - ); - }); +describe("escapeProxyCommandArg", () => { + it.each([ + ["--token=", escapeCommandArg], + ['say "hi"', escapeCommandArg], + ["get-headers --profile 'prod' | jq", escapeCommandArg], + ["$TOKEN", escapeShellArg], + ["`whoami`", escapeShellArg], + [String.raw`a\b`, escapeShellArg], + ["%h", escapeShellArg], + ["line\nbreak", escapeShellArg], + ])("quotes %j", (value, escape) => { + expect(escapeProxyCommandArg(value)).toBe(escape(value)); }); }); diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts index fcde9182a6..ab21ab784a 100644 --- a/test/unit/remote/remote.test.ts +++ b/test/unit/remote/remote.test.ts @@ -1,5 +1,6 @@ import { Api } from "coder/site/src/api/api"; import { vol } from "memfs"; +import { spawnSync } from "node:child_process"; import * as semver from "semver"; import { beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; @@ -28,7 +29,7 @@ import { MockUserInteraction, useEditor, } from "../../mocks/testHelpers"; -import { quoteCommand } from "../../utils/platform"; +import { isWindows } from "../../utils/platform"; import type { CliManager } from "@/cli/cliManager"; import type { Commands } from "@/commands"; @@ -395,9 +396,61 @@ describe("Remote", () => { }); expect(elideNetworkInfoDir(proxyCommand)).toBe( - `/mock/coder --token= --global-config ${quoteCommand("/mock/global")} --url ${quoteCommand("https://coder.example.com")} ${expected}`, + `/mock/coder "--token=" --global-config /mock/global --url https://coder.example.com ${expected}`, ); }); + + // OpenSSH runs the command with `sh -c` on Unix; Win32-OpenSSH passes it + // to CreateProcessW, so the program's argv parser splits it. + it("reaches the CLI unchanged through this platform's OpenSSH", () => { + const configDir = "/Users/me/Library/Application Support/coder"; + const headerCommand = 'echo "X: $TOKEN" "C:\\a b\\" `id`'; + const pathResolver = new PathResolver("/mock/global", "/mock/log"); + new MockConfigurationProvider().set("coder.headerCommand", headerCommand); + const proxyCommand = buildSshProxyCommand({ + pathResolver, + binaryPath: "coder", + cliAuth: { ...CLI_AUTH, configDir }, + logArgs: [], + sessionApp: "vscode", + hostPrefix: "coder-vscode.coder.example.com--", + }); + + // Swap the CLI for Node printing the arguments it receives. + const printArgv = proxyCommand.replace( + /^coder /, + `-p "JSON.stringify(process.argv.slice(1))" -- `, + ); + const result = isWindows() + ? spawnSync(process.execPath, [printArgv], { + argv0: "node", + encoding: "utf8", + windowsVerbatimArguments: true, + }) + : spawnSync("sh", ["-c", `"$NODE" ${printArgv}`], { + encoding: "utf8", + env: { ...process.env, NODE: process.execPath }, + }); + + expect(JSON.parse(result.stdout)).toStrictEqual([ + "--token=", + "--global-config", + configDir, + "--url", + CLI_AUTH.url, + "--header-command", + headerCommand, + "ssh", + "--disable-autostart", + "--stdio", + "--usage-app=vscode", + "--network-info-dir", + pathResolver.getNetworkInfoPath(), + "--ssh-host-prefix", + "coder-vscode.coder.example.com--", + "%h", + ]); + }); }); }); diff --git a/test/unit/settings/cli.test.ts b/test/unit/settings/cli.test.ts index 2304376996..3be9a8ce99 100644 --- a/test/unit/settings/cli.test.ts +++ b/test/unit/settings/cli.test.ts @@ -19,7 +19,7 @@ import { INVALID_DEPLOYMENT_URLS, MockConfigurationProvider, } from "../../mocks/testHelpers"; -import { isWindows, quoteCommand } from "../../utils/platform"; +import { isWindows } from "../../utils/platform"; vi.mock("node:os"); @@ -43,14 +43,6 @@ const cliStoreAuth: CliAuth = { const EXTENSION_FLAGS = ["--global-config", EXT_DIR, "--url", URL] as const; const CLI_FLAGS = ["--url", URL] as const; -// Shell contexts quote the auth values literally. -const EXTENSION_SHELL_FLAGS = [ - "--global-config", - `'${EXT_DIR}'`, - "--url", - `'${URL}'`, -] as const; -const CLI_SHELL_FLAGS = ["--url", `'${URL}'`] as const; describe("cliConfig", () => { describe("getGlobalShellFlags", () => { @@ -64,23 +56,23 @@ describe("cliConfig", () => { { scenario: "extension store", auth: extensionStoreAuth, - expected: EXTENSION_SHELL_FLAGS, + expected: EXTENSION_FLAGS, }, - { scenario: "CLI store", auth: cliStoreAuth, expected: CLI_SHELL_FLAGS }, + { scenario: "CLI store", auth: cliStoreAuth, expected: CLI_FLAGS }, { scenario: "extension store with keyring off", auth: { ...extensionStoreAuth, useKeyring: false }, - expected: [...EXTENSION_SHELL_FLAGS, "--use-keyring=false"], + expected: [...EXTENSION_FLAGS, "--use-keyring=false"], }, { scenario: "CLI store with keyring on", auth: { ...cliStoreAuth, useKeyring: true }, - expected: [...CLI_SHELL_FLAGS, "--use-keyring=true"], + expected: [...CLI_FLAGS, "--use-keyring=true"], }, ])("emits auth flags for a $scenario", ({ auth, expected }) => { const config = new MockConfigurationProvider(); expect(getGlobalShellFlags(config, auth)).toStrictEqual([ - "--token=", + '"--token="', ...expected, ]); }); @@ -101,10 +93,10 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose", "--global-configs"]); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ - "--token=", + '"--token="', "--verbose", "--global-configs", // similar prefixes are not managed flags - ...EXTENSION_SHELL_FLAGS, + ...EXTENSION_FLAGS, ]); }); @@ -113,9 +105,9 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose", "--use-keyring=false"]); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ - "--token=", + '"--token="', "--verbose", - ...EXTENSION_SHELL_FLAGS, + ...EXTENSION_FLAGS, ]); }); @@ -124,9 +116,9 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--token=abc"]); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ - "--token=", + '"--token="', "--token=abc", - ...CLI_SHELL_FLAGS, + ...CLI_FLAGS, ]); }); @@ -145,9 +137,9 @@ describe("cliConfig", () => { config.set("coder.globalFlags", flags); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ - "--token=", + '"--token="', ...flags, - ...CLI_SHELL_FLAGS, + ...CLI_FLAGS, ]); }, ); @@ -165,9 +157,9 @@ describe("cliConfig", () => { config.set("coder.globalFlags", flags); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ - "--token=", + '"--token="', "-v", - ...EXTENSION_SHELL_FLAGS, + ...EXTENSION_FLAGS, ]); }, ); @@ -179,12 +171,12 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["-v", "--header-command custom"]); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ - "--token=", + '"--token="', "-v", '"--header-command custom"', // ignored by CLI - ...CLI_SHELL_FLAGS, + ...CLI_FLAGS, "--header-command", - quoteCommand(headerCommand), + `"${headerCommand}"`, ]); }); @@ -215,12 +207,12 @@ describe("cliConfig", () => { configDir: "/home/$USER/%APPDATA%/coder", }), ).toStrictEqual([ - "--token=", + '"--token="', '"--header=Authorization=$TOKEN"', "--global-config", configDir, "--url", - quoteCommand(URL), + URL, "--header-command", headerCommand, ]); @@ -234,9 +226,9 @@ describe("cliConfig", () => { // Without per-entry escaping the space splits the shell command. expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ - "--token=", + '"--token="', '"--cfg=C:\\Users\\John Doe/coder"', - ...EXTENSION_SHELL_FLAGS, + ...EXTENSION_FLAGS, ]); }); }); diff --git a/test/utils/platform.ts b/test/utils/platform.ts index 4157bfc2d4..bea0e9994f 100644 --- a/test/utils/platform.ts +++ b/test/utils/platform.ts @@ -115,15 +115,6 @@ export function shimExecFile< return Object.assign({}, mod, { execFile }); } -/** - * Wraps a value in the platform-appropriate quote character, - * matching the escaping in {@link getHeaderArgs} from src/settings/headers.ts. - */ -export function quoteCommand(value: string): string { - const quote = isWindows() ? '"' : "'"; - return `${quote}${value}${quote}`; -} - export function expectPathsEqual(actual: string | undefined, expected: string) { expect(actual).toBeDefined(); expect(normalizePath(actual!)).toBe(normalizePath(expected));