Skip to content

Commit 5c0ba1d

Browse files
authored
fix(ssh): double-quote ProxyCommand values for open-remote-ssh (#1156)
Editors built on open-remote-ssh (Positron, VSCodium) split the SSH ProxyCommand themselves instead of running it through a shell. Since 1.17.0 they passed the single-quoted --url and --global-config to the CLI literally, and ssh-config@5 merged `--token=` with the next argument, so every connection failed with "Premature close". - Quote ProxyCommand values with double quotes unless they contain `$`, a backtick, `\`, `%` or a line break; those keep single quotes. - Quote a leading, trailing or doubled `=` in escapeCommandArg. - On Windows, escape `"` in escapeShellArg as `\"` with doubled preceding backslashes, so the Go CLI reads a header command containing quotes as one argument. Closes #1153
1 parent 61d0c48 commit 5c0ba1d

9 files changed

Lines changed: 149 additions & 63 deletions

File tree

‎AGENTS.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ deployment. Treat them as untrusted:
7575
- Validate names with `src/common/validation.ts` before they reach SSH hosts,
7676
paths, or the CLI.
7777
- In SSH `ProxyCommand` (the one shell context), build flags with
78-
`getGlobalShellFlags`, which validates the URL and quotes it with
79-
`escapeShellArg`.
78+
`getGlobalShellFlags`, which validates the URL and quotes values with
79+
`escapeProxyCommandArg` (open-remote-ssh splits it without a shell).
8080

8181
## Code style
8282

‎CHANGELOG.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,17 @@
1313
generally available. Existing values remain effective until the new setting is
1414
explicitly set in user settings. OAuth remains disabled by default.
1515

16+
### Fixed
17+
18+
- Fix "Premature close" since v1.17.0 in Positron and other editors that use
19+
the open source Remote - SSH extension (`open-remote-ssh`): the SSH
20+
`ProxyCommand` single-quoted `--url` and `--global-config`, which these
21+
editors pass to the CLI, and `--token=` merged with the next argument.
22+
- `coder.headerCommand` now works with `open-remote-ssh` unless it contains `$`,
23+
a backtick, `\`, `%` or a line break.
24+
- On Windows, a `coder.headerCommand` containing `"` was split into several
25+
arguments before it reached the CLI.
26+
1627
## [v1.17.0](https://github.com/coder/vscode-coder/releases/tag/v1.17.0) 2026-10-13
1728

1829
### Added

‎src/common/strings.ts‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,14 @@ export function lowercase<T extends string>(value: T): Lowercase<T> {
1515
* Not a universal shell-escape: `$VAR` / `$(...)` / `%VAR%` still expand
1616
* inside `"..."`. For untrusted values use {@link escapeShellArg}.
1717
*
18+
* A leading, trailing or doubled `=` is quoted too: open-remote-ssh's
19+
* ssh-config@5 treats an unquoted `=` as a separator.
20+
*
1821
* @see https://docs.python.org/3/library/shlex.html#shlex.quote
1922
* @see https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way
2023
*/
2124
export function escapeCommandArg(arg: string): string {
22-
if (arg !== "" && /^[\w@%+,=:./-]+$/.test(arg)) {
25+
if (/^[\w@%+,:./-]+(?:=[\w@%+,:./-]+)*$/.test(arg)) {
2326
return arg;
2427
}
2528
return `"${arg.replaceAll('"', String.raw`\"`)}"`;
@@ -28,14 +31,29 @@ export function escapeCommandArg(arg: string): string {
2831
/**
2932
* Cross-platform shell quoting that blocks variable expansion. Use for
3033
* values from outside the user's local settings (e.g. server-controlled).
31-
* The win32 branch targets Windows OpenSSH, which runs `ProxyCommand`
32-
* without a shell and turns `%%` back into `%`. A shell such as cmd.exe or
33-
* Git Bash's `sh` still expands inside those quotes.
34+
* The win32 branch targets Windows OpenSSH, which starts `ProxyCommand`
35+
* without a shell: `%%` becomes `%` and the CLI splits the line with the
36+
* Windows argv rules (`\"`). Git Bash's `sh` still expands `$` inside.
3437
*/
3538
export function escapeShellArg(arg: string): string {
3639
if (os.platform() === "win32") {
37-
const escaped = arg.replace(/"/g, '""').replace(/%/g, "%%");
40+
// `\\+` consumes backslash runs not before a quote, keeping this linear.
41+
const escaped = arg
42+
.replace(/(\\*)("|$)|\\+/g, (run, slashes?: string, quote?: string) =>
43+
slashes === undefined
44+
? run
45+
: `${slashes}${slashes}${quote ? '\\"' : ""}`,
46+
)
47+
.replace(/%/g, "%%");
3848
return `"${escaped}"`;
3949
}
4050
return `'${arg.replace(/'/g, "'\\''")}'`;
4151
}
52+
53+
/**
54+
* Double quotes read the same in a shell and in open-remote-ssh, which splits
55+
* `ProxyCommand` itself, unless the value needs escaping there.
56+
*/
57+
export function escapeProxyCommandArg(arg: string): string {
58+
return /[$`\\%\r\n]/.test(arg) ? escapeShellArg(arg) : escapeCommandArg(arg);
59+
}

‎src/settings/cli.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import os from "node:os";
22

33
import { expandPath } from "../common/fs";
4-
import { escapeCommandArg, escapeShellArg } from "../common/strings";
4+
import { escapeCommandArg, escapeProxyCommandArg } from "../common/strings";
55
import { validateDeploymentUrl } from "../common/validation";
66

77
import { getHeaderArgs } from "./headers";
@@ -51,8 +51,8 @@ export function getGlobalShellFlags(
5151
// covers URLs that reach storage some other way.
5252
validateDeploymentUrl(auth.url);
5353
return [
54-
"--token=",
55-
...buildGlobalFlags(configs, auth, escapeCommandArg, escapeShellArg),
54+
escapeCommandArg("--token="),
55+
...buildGlobalFlags(configs, auth, escapeCommandArg, escapeProxyCommandArg),
5656
];
5757
}
5858

‎src/settings/headers.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export function getHeaderCommand(
1111
return cmd || undefined;
1212
}
1313

14-
/** Returns `--header-command` args; shell callers pass `escapeShellArg` as `esc`. */
14+
/** Returns `--header-command` args, quoted with `esc`. */
1515
export function getHeaderArgs(
1616
config: Pick<WorkspaceConfiguration, "get">,
1717
esc: (s: string) => string = (s) => s,

‎test/unit/common/strings.test.ts‎

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
import os from "node:os";
22
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
33

4-
import { escapeCommandArg, escapeShellArg } from "@/common/strings";
4+
import {
5+
escapeCommandArg,
6+
escapeProxyCommandArg,
7+
escapeShellArg,
8+
} from "@/common/strings";
59

610
describe("escapeCommandArg", () => {
711
it("returns simple strings unquoted", () => {
@@ -17,6 +21,13 @@ describe("escapeCommandArg", () => {
1721
expect(escapeCommandArg("")).toBe('""');
1822
});
1923

24+
it.each([["--token="], ["=x"], ["x==y"]])(
25+
"quotes a leading, trailing or doubled `=`: %j",
26+
(input) => {
27+
expect(escapeCommandArg(input)).toBe(`"${input}"`);
28+
},
29+
);
30+
2031
it("escapes double quotes and wraps in quotes", () => {
2132
expect(escapeCommandArg('say "hello"')).toBe(String.raw`"say \"hello\""`);
2233
});
@@ -85,20 +96,30 @@ describe("escapeShellArg", () => {
8596
expect(escapeShellArg("env=dev")).toBe('"env=dev"');
8697
});
8798

88-
it('doubles embedded `"`', () => {
89-
expect(escapeShellArg('regions=["us","eu"]')).toBe(
90-
'"regions=[""us"",""eu""]"',
91-
);
99+
it.each([
100+
['regions=["us","eu"]', String.raw`"regions=[\"us\",\"eu\"]"`],
101+
['C:\\a b\\ \\"x\\', '"C:\\a b\\ \\\\\\"x\\\\"'],
102+
])('escapes `"` and the backslashes before it in %j', (value, expected) => {
103+
expect(escapeShellArg(value)).toBe(expected);
92104
});
93105

94106
it("doubles `%` so OpenSSH token expansion keeps it literal", () => {
95107
expect(escapeShellArg("%PATH%")).toBe('"%%PATH%%"');
96108
});
109+
});
110+
});
97111

98-
it("keeps cmd metachars inside the quoted region", () => {
99-
expect(escapeShellArg('foo" & calc.exe & "x')).toBe(
100-
'"foo"" & calc.exe & ""x"',
101-
);
102-
});
112+
describe("escapeProxyCommandArg", () => {
113+
it.each([
114+
["--token=", escapeCommandArg],
115+
['say "hi"', escapeCommandArg],
116+
["get-headers --profile 'prod' | jq", escapeCommandArg],
117+
["$TOKEN", escapeShellArg],
118+
["`whoami`", escapeShellArg],
119+
[String.raw`a\b`, escapeShellArg],
120+
["%h", escapeShellArg],
121+
["line\nbreak", escapeShellArg],
122+
])("quotes %j", (value, escape) => {
123+
expect(escapeProxyCommandArg(value)).toBe(escape(value));
103124
});
104125
});

‎test/unit/remote/remote.test.ts‎

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { Api } from "coder/site/src/api/api";
22
import { vol } from "memfs";
3+
import { spawnSync } from "node:child_process";
34
import * as semver from "semver";
45
import { beforeEach, describe, expect, it, vi } from "vitest";
56
import * as vscode from "vscode";
@@ -28,7 +29,7 @@ import {
2829
MockUserInteraction,
2930
useEditor,
3031
} from "../../mocks/testHelpers";
31-
import { quoteCommand } from "../../utils/platform";
32+
import { isWindows } from "../../utils/platform";
3233

3334
import type { CliManager } from "@/cli/cliManager";
3435
import type { Commands } from "@/commands";
@@ -395,9 +396,61 @@ describe("Remote", () => {
395396
});
396397

397398
expect(elideNetworkInfoDir(proxyCommand)).toBe(
398-
`/mock/coder --token= --global-config ${quoteCommand("/mock/global")} --url ${quoteCommand("https://coder.example.com")} ${expected}`,
399+
`/mock/coder "--token=" --global-config /mock/global --url https://coder.example.com ${expected}`,
399400
);
400401
});
402+
403+
// OpenSSH runs the command with `sh -c` on Unix; Win32-OpenSSH passes it
404+
// to CreateProcessW, so the program's argv parser splits it.
405+
it("reaches the CLI unchanged through this platform's OpenSSH", () => {
406+
const configDir = "/Users/me/Library/Application Support/coder";
407+
const headerCommand = 'echo "X: $TOKEN" "C:\\a b\\" `id`';
408+
const pathResolver = new PathResolver("/mock/global", "/mock/log");
409+
new MockConfigurationProvider().set("coder.headerCommand", headerCommand);
410+
const proxyCommand = buildSshProxyCommand({
411+
pathResolver,
412+
binaryPath: "coder",
413+
cliAuth: { ...CLI_AUTH, configDir },
414+
logArgs: [],
415+
sessionApp: "vscode",
416+
hostPrefix: "coder-vscode.coder.example.com--",
417+
});
418+
419+
// Swap the CLI for Node printing the arguments it receives.
420+
const printArgv = proxyCommand.replace(
421+
/^coder /,
422+
`-p "JSON.stringify(process.argv.slice(1))" -- `,
423+
);
424+
const result = isWindows()
425+
? spawnSync(process.execPath, [printArgv], {
426+
argv0: "node",
427+
encoding: "utf8",
428+
windowsVerbatimArguments: true,
429+
})
430+
: spawnSync("sh", ["-c", `"$NODE" ${printArgv}`], {
431+
encoding: "utf8",
432+
env: { ...process.env, NODE: process.execPath },
433+
});
434+
435+
expect(JSON.parse(result.stdout)).toStrictEqual([
436+
"--token=",
437+
"--global-config",
438+
configDir,
439+
"--url",
440+
CLI_AUTH.url,
441+
"--header-command",
442+
headerCommand,
443+
"ssh",
444+
"--disable-autostart",
445+
"--stdio",
446+
"--usage-app=vscode",
447+
"--network-info-dir",
448+
pathResolver.getNetworkInfoPath(),
449+
"--ssh-host-prefix",
450+
"coder-vscode.coder.example.com--",
451+
"%h",
452+
]);
453+
});
401454
});
402455
});
403456

0 commit comments

Comments
 (0)