Skip to content

Commit 7dbc60c

Browse files
committed
feat: unify CLI execution and never spawn a shell
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.
1 parent f2fbba5 commit 7dbc60c

10 files changed

Lines changed: 129 additions & 109 deletions

File tree

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@
160160
]
161161
},
162162
"coder.globalFlags": {
163-
"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#`.",
163+
"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#`.",
164164
"type": "array",
165165
"items": {
166166
"type": "string"

src/api/updateParameters.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
import * as vscode from "vscode";
22

3-
import { escapeShellArg } from "../util";
4-
53
import type { Api } from "coder/site/src/api/api";
64
import type {
75
TemplateVersionParameter,
@@ -44,7 +42,7 @@ export async function collectUpdateParameters(
4442
if (value === undefined) {
4543
throw new WorkspaceUpdateCancelledError();
4644
}
47-
args.push("--parameter", escapeShellArg(`${param.name}=${value}`));
45+
args.push("--parameter", `${param.name}=${value}`);
4846
}
4947
return args;
5048
}

src/api/workspace.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
import { spawn } from "node:child_process";
22
import * as vscode from "vscode";
33

4-
import { getGlobalShellFlags, type CliAuth } from "../settings/cli";
5-
import { escapeCommandArg, escapeShellArg } from "../util";
4+
import { getGlobalFlags, type CliAuth } from "../settings/cli";
65

76
import { errToStr, createWorkspaceIdentifier } from "./api-helper";
87
import { collectUpdateParameters } from "./updateParameters";
@@ -62,12 +61,11 @@ interface CliContext {
6261
function runCliCommand(ctx: CliContext, args: string[]): Promise<void> {
6362
return new Promise((resolve, reject) => {
6463
const fullArgs = [
65-
...getGlobalShellFlags(vscode.workspace.getConfiguration(), ctx.auth),
64+
...getGlobalFlags(vscode.workspace.getConfiguration(), ctx.auth),
6665
...args,
67-
escapeShellArg(createWorkspaceIdentifier(ctx.workspace)),
66+
createWorkspaceIdentifier(ctx.workspace),
6867
];
69-
const cmd = `${escapeCommandArg(ctx.binPath)} ${fullArgs.join(" ")}`;
70-
const proc = spawn(cmd, { shell: true });
68+
const proc = spawn(ctx.binPath, fullArgs);
7169
// Unexpected prompts EOF instead of hanging forever.
7270
proc.stdin.end();
7371

@@ -82,6 +80,9 @@ function runCliCommand(ctx: CliContext, args: string[]): Promise<void> {
8280
capturedStderr += text;
8381
});
8482

83+
// Settle on ENOENT/EACCES; later `close` rejects are then no-ops.
84+
proc.on("error", reject);
85+
8586
proc.on("close", (code: number | null, signal: NodeJS.Signals | null) => {
8687
if (code === 0) {
8788
resolve();

src/core/cliExec.ts

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -109,11 +109,11 @@ export async function supportBundle(
109109
* Run `coder ping` in a PTY terminal with Ctrl+C support.
110110
*/
111111
export function ping(env: CliEnv, workspaceName: string): vscode.Terminal {
112-
const globalFlags = getGlobalShellFlags(env.configs, env.auth);
112+
const globalFlags = getGlobalFlags(env.configs, env.auth);
113113
return spawnCliInTerminal({
114114
name: `Coder Ping: ${workspaceName}`,
115115
binary: env.binary,
116-
args: [...globalFlags, "ping", escapeCommandArg(workspaceName)],
116+
args: [...globalFlags, "ping", workspaceName],
117117
banner: ["Press Ctrl+C (^C) to stop.", "─".repeat(40)],
118118
});
119119
}
@@ -172,14 +172,12 @@ function spawnCliInTerminal(options: {
172172
const writeEmitter = new vscode.EventEmitter<string>();
173173
const closeEmitter = new vscode.EventEmitter<number | void>();
174174

175-
const cmd = `${escapeCommandArg(options.binary)} ${options.args.join(" ")}`;
176-
// On Unix, spawn in a new process group so we can signal the
177-
// entire group (shell + coder binary) on Ctrl+C. On Windows,
178-
// detached opens a visible console window and negative-PID kill
179-
// is unsupported, so we fall back to proc.kill().
175+
// On Unix, `detached` puts the child in its own process group so
176+
// Ctrl+C can signal the whole subtree. On Windows it would open a
177+
// visible console window and negative-PID kill is unsupported, so we
178+
// fall back to proc.kill() there.
180179
const useProcessGroup = process.platform !== "win32";
181-
const proc = spawn(cmd, {
182-
shell: true,
180+
const proc = spawn(options.binary, options.args, {
183181
detached: useProcessGroup,
184182
});
185183

src/settings/cli.ts

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { isKeyringSupported } from "../core/cliCredentialManager";
2-
import { escapeCommandArg, escapeShellArg } from "../util";
2+
import { escapeCommandArg, escapeShellArg, expandPath } from "../util";
33

44
import { getHeaderArgs } from "./headers";
55

@@ -11,14 +11,36 @@ export type CliAuth =
1111
| { mode: "global-config"; configDir: string }
1212
| { mode: "url"; url: string };
1313

14-
/** Returns the user's `coder.globalFlags` as configured, with no expansion. */
14+
/**
15+
* Returns the user's `coder.globalFlags` with `${env:VAR}` references
16+
* substituted from `process.env` (missing vars become empty, matching
17+
* VS Code's built-in `${env:VAR}` behaviour) and `~` / `${userHome}`
18+
* expanded to the home directory. For `--flag=value` entries the path
19+
* expansion applies to the value half so `--cfg=~/coder` works.
20+
*/
1521
export function getUserGlobalFlags(
1622
configs: Pick<WorkspaceConfiguration, "get">,
1723
): string[] {
18-
return configs.get<string[]>("coder.globalFlags", []);
24+
return configs
25+
.get<string[]>("coder.globalFlags", [])
26+
.map((flag) =>
27+
flag.replace(
28+
/\$\{env:([A-Za-z_][A-Za-z0-9_]*)\}/g,
29+
(_, name: string) => process.env[name] ?? "",
30+
),
31+
)
32+
.map(expandFlagPath);
33+
}
34+
35+
/** Applies `expandPath` to the value half of `--flag=value`, or the whole entry. */
36+
function expandFlagPath(flag: string): string {
37+
const eq = flag.indexOf("=");
38+
return eq === -1
39+
? expandPath(flag)
40+
: flag.slice(0, eq + 1) + expandPath(flag.slice(eq + 1));
1941
}
2042

21-
/** Flags for shell contexts (`terminal.sendText`, `spawn({ shell: true })`). */
43+
/** Flags for shell contexts (`terminal.sendText`, SSH `ProxyCommand`). */
2244
export function getGlobalShellFlags(
2345
configs: Pick<WorkspaceConfiguration, "get">,
2446
auth: CliAuth,

test/unit/api/updateParameters.test.ts

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ import {
88

99
import { workspace as createWorkspace } from "@repo/mocks";
1010

11-
import { shellQuote } from "../../utils/platform";
12-
1311
import type { Api } from "coder/site/src/api/api";
1412
import type { TemplateVersionParameter } from "coder/site/src/api/typesGenerated";
1513

@@ -145,14 +143,14 @@ describe("collectUpdateParameters", () => {
145143
param: { name: "environment" },
146144
mock: mockCreateInputBox,
147145
accept: { value: "dev" },
148-
expected: ["--parameter", shellQuote("environment=dev")],
146+
expected: ["--parameter", "environment=dev"],
149147
},
150148
{
151149
kind: "bool quick pick",
152150
param: { name: "enabled", type: "bool" },
153151
mock: mockCreateQuickPick,
154152
accept: { selectedItems: [{ value: "true" }] },
155-
expected: ["--parameter", shellQuote("enabled=true")],
153+
expected: ["--parameter", "enabled=true"],
156154
},
157155
{
158156
kind: "options quick pick",
@@ -165,7 +163,7 @@ describe("collectUpdateParameters", () => {
165163
},
166164
mock: mockCreateQuickPick,
167165
accept: { selectedItems: [{ value: "l" }] },
168-
expected: ["--parameter", shellQuote("size=l")],
166+
expected: ["--parameter", "size=l"],
169167
},
170168
{
171169
kind: "multi-select quick pick (JSON array)",
@@ -179,7 +177,7 @@ describe("collectUpdateParameters", () => {
179177
},
180178
mock: mockCreateQuickPick,
181179
accept: { selectedItems: [{ value: "us" }, { value: "eu" }] },
182-
expected: ["--parameter", shellQuote('regions=["us","eu"]')],
180+
expected: ["--parameter", 'regions=["us","eu"]'],
183181
},
184182
])(
185183
"collects the value via $kind",
@@ -195,18 +193,15 @@ describe("collectUpdateParameters", () => {
195193
},
196194
);
197195

198-
it("escapes shell metacharacters in server-controlled values", async () => {
196+
it("passes server-controlled values through verbatim (no shell expansion path)", async () => {
199197
const { restClient, workspace } = createCollectCtx([{ name: "evil" }]);
200198
const qi = mockCreateInputBox();
201199

202200
const result = collectUpdateParameters(restClient, workspace);
203201
await waitShown(qi);
204202
qi.accept({ value: "$(rm -rf /)" });
205203

206-
await expect(result).resolves.toEqual([
207-
"--parameter",
208-
shellQuote("evil=$(rm -rf /)"),
209-
]);
204+
await expect(result).resolves.toEqual(["--parameter", "evil=$(rm -rf /)"]);
210205
});
211206

212207
it("skips parameters that already have a value or default", async () => {

test/unit/api/workspace.test.ts

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@ import { LazyStream, startWorkspace, updateWorkspace } from "@/api/workspace";
66

77
import { workspace as createWorkspace } from "@repo/mocks";
88

9-
import { shellQuote } from "../../utils/platform";
10-
119
import type { Api } from "coder/site/src/api/api";
1210
import type {
1311
Workspace,
@@ -129,6 +127,10 @@ function controlSpawn() {
129127
await spawned;
130128
proc.emit("close", exitCode, signal ?? null);
131129
},
130+
async error(err: Error) {
131+
await spawned;
132+
proc.emit("error", err);
133+
},
132134
};
133135
}
134136

@@ -204,10 +206,12 @@ describe("updateWorkspace", () => {
204206
await sp.close(0);
205207

206208
await expect(result).resolves.toBe(finalWorkspace);
207-
expect(spawn).toHaveBeenCalledWith(
208-
`"/usr/bin/coder" --url "https://test.coder.com" update ${shellQuote("testuser/test-workspace")}`,
209-
{ shell: true },
210-
);
209+
expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [
210+
"--url",
211+
"https://test.coder.com",
212+
"update",
213+
"testuser/test-workspace",
214+
]);
211215
expect(sp.stdinEnd).toHaveBeenCalled();
212216
expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id);
213217
});
@@ -225,6 +229,17 @@ describe("updateWorkspace", () => {
225229
expect(restClient.getWorkspace).not.toHaveBeenCalled();
226230
});
227231

232+
it("rejects when spawn emits an error (e.g. missing binary)", async () => {
233+
const { ctx, restClient } = createUpdateCtx();
234+
const sp = controlSpawn();
235+
236+
const result = updateWorkspace(ctx);
237+
await sp.error(new Error("spawn /usr/bin/coder ENOENT"));
238+
239+
await expect(result).rejects.toThrow(/ENOENT/);
240+
expect(restClient.getWorkspace).not.toHaveBeenCalled();
241+
});
242+
228243
it("reports the terminating signal when the process is killed", async () => {
229244
const { ctx } = createUpdateCtx();
230245
const sp = controlSpawn();
@@ -295,10 +310,15 @@ describe("startWorkspace", () => {
295310
await sp.close(0);
296311

297312
await expect(result).resolves.toBe(finalWorkspace);
298-
expect(spawn).toHaveBeenCalledWith(
299-
`"/usr/bin/coder" --url "https://test.coder.com" start --yes --reason vscode_connection ${shellQuote("testuser/test-workspace")}`,
300-
{ shell: true },
301-
);
313+
expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [
314+
"--url",
315+
"https://test.coder.com",
316+
"start",
317+
"--yes",
318+
"--reason",
319+
"vscode_connection",
320+
"testuser/test-workspace",
321+
]);
302322
expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id);
303323
});
304324

@@ -320,9 +340,12 @@ describe("startWorkspace", () => {
320340
await sp.close(0);
321341
await result;
322342

323-
expect(spawn).toHaveBeenCalledWith(
324-
`"/usr/bin/coder" --url "https://test.coder.com" start --yes ${shellQuote("testuser/test-workspace")}`,
325-
{ shell: true },
326-
);
343+
expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [
344+
"--url",
345+
"https://test.coder.com",
346+
"start",
347+
"--yes",
348+
"testuser/test-workspace",
349+
]);
327350
});
328351
});

test/unit/cliConfig.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,49 @@ describe("cliConfig", () => {
231231
"--disable-direct-connections",
232232
]);
233233
});
234+
235+
it("substitutes ${env:VAR} from process.env", () => {
236+
const restore = process.env.CODER_TEST_VAR;
237+
process.env.CODER_TEST_VAR = "from-env";
238+
try {
239+
const config = new MockConfigurationProvider();
240+
config.set("coder.globalFlags", [
241+
"--prefix=${env:CODER_TEST_VAR}",
242+
"${env:CODER_MISSING_VAR}-suffix",
243+
]);
244+
245+
expect(getUserGlobalFlags(config)).toStrictEqual([
246+
"--prefix=from-env",
247+
"-suffix",
248+
]);
249+
} finally {
250+
if (restore === undefined) {
251+
delete process.env.CODER_TEST_VAR;
252+
} else {
253+
process.env.CODER_TEST_VAR = restore;
254+
}
255+
}
256+
});
257+
258+
it("expands ~ and ${userHome} in flag values", () => {
259+
vi.mocked(os.homedir).mockReturnValue("/home/coder");
260+
const config = new MockConfigurationProvider();
261+
config.set("coder.globalFlags", [
262+
"~/bare",
263+
"--cfg=~/coder",
264+
"--state=${userHome}/state",
265+
"--literal=value~with~tildes",
266+
]);
267+
268+
expect(getUserGlobalFlags(config)).toStrictEqual([
269+
"/home/coder/bare",
270+
"--cfg=/home/coder/coder",
271+
"--state=/home/coder/state",
272+
// Tildes mid-value are left alone (only ~ at the start of the
273+
// value half is expanded).
274+
"--literal=value~with~tildes",
275+
]);
276+
});
234277
});
235278

236279
describe("getSshFlags", () => {

0 commit comments

Comments
 (0)