Skip to content

Commit 185abc4

Browse files
EhabYEhabY
committed
fix(cliExec): refresh credentials per CLI call instead of env-forwarding
Forwarding CODER_SESSION_TOKEN via the child env exposed it to any sibling process via /proc/<pid>/environ on Linux and similar interfaces elsewhere. Drop all env injection from cliExec and instead refresh the file or keyring once via cliManager.configure inside resolveCliEnv, mirroring the connection-time write in remote.ts. The CLI reads the fresh token from the file (or keyring on supported systems) via the existing --global-config / --url flags. mTLS still works since the refresh accepts an empty token. Also drop the keyringOnly option from storeToken (no longer needed now that we always refresh) and update the matching tests. Add writeStdoutJs / writeStderrJs helpers in test/utils/platform.ts that generate fs.writeSync snippets, and use them in the cliExec and platform tests. process.stdout/stderr.write is async on POSIX pipes and can be lost on exit (nodejs/node#4112), which was making the version fallback test flaky.
1 parent 357814b commit 185abc4

9 files changed

Lines changed: 49 additions & 152 deletions

File tree

‎src/commands.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -930,11 +930,13 @@ export class Commands {
930930
const configDir = this.pathResolver.getGlobalConfigDir(safeHost);
931931
const configs = vscode.workspace.getConfiguration();
932932
const auth = resolveCliAuth(configs, featureSet, baseUrl, configDir);
933-
const childCredentials = {
934-
url: baseUrl,
935-
token: client.getSessionToken() ?? "",
936-
};
937-
return { binary, configs, auth, childCredentials, featureSet };
933+
// Same threat model as the connection-time write in remote.ts: token
934+
// goes to the file (or keyring on supported systems), never env, since
935+
// child env is sibling-readable on most platforms.
936+
await this.cliManager.configure(baseUrl, client.getSessionToken() ?? "", {
937+
silent: true,
938+
});
939+
return { binary, configs, auth, featureSet };
938940
}
939941

940942
/**

‎src/core/cliCredentialManager.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,26 +56,20 @@ export class CliCredentialManager {
5656
* setting is enabled and the CLI supports it; otherwise writes plaintext
5757
* files under --global-config.
5858
*
59-
* Keyring and files are mutually exclusive — never both.
60-
*
61-
* When `keyringOnly` is set, silently returns if the keyring is unavailable
62-
* instead of falling back to file storage.
59+
* Keyring and files are mutually exclusive, never both.
6360
*/
6461
public async storeToken(
6562
url: string,
6663
token: string,
6764
configs: Pick<WorkspaceConfiguration, "get">,
68-
options?: { signal?: AbortSignal; keyringOnly?: boolean },
65+
options?: { signal?: AbortSignal },
6966
): Promise<void> {
7067
const binPath = await this.resolveKeyringBinary(
7168
url,
7269
configs,
7370
"keyringAuth",
7471
);
7572
if (!binPath) {
76-
if (options?.keyringOnly) {
77-
return;
78-
}
7973
await this.writeCredentialFiles(url, token);
8074
return;
8175
}

‎src/core/cliExec.ts‎

Lines changed: 3 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@ export interface CliEnv {
1515
binary: string;
1616
auth: CliAuth;
1717
configs: Pick<vscode.WorkspaceConfiguration, "get">;
18-
/** Forwarded to the child as `CODER_URL` / `CODER_SESSION_TOKEN`. Empty token is valid for mTLS. */
19-
childCredentials: { url: string; token: string };
2018
}
2119

2220
/**
@@ -74,10 +72,7 @@ export async function speedtest(
7472
args.push("-t", duration);
7573
}
7674
try {
77-
const result = await execFileAsync(env.binary, args, {
78-
signal,
79-
env: execFileEnv(env),
80-
});
75+
const result = await execFileAsync(env.binary, args, { signal });
8176
return result.stdout;
8277
} catch (error) {
8378
throw cliError(error);
@@ -104,10 +99,7 @@ export async function supportBundle(
10499
"--yes",
105100
];
106101
try {
107-
await execFileAsync(env.binary, args, {
108-
signal,
109-
env: execFileEnv(env),
110-
});
102+
await execFileAsync(env.binary, args, { signal });
111103
} catch (error) {
112104
throw cliError(error);
113105
}
@@ -123,7 +115,6 @@ export function ping(env: CliEnv, workspaceName: string): vscode.Terminal {
123115
binary: env.binary,
124116
args: [...globalFlags, "ping", escapeCommandArg(workspaceName)],
125117
banner: ["Press Ctrl+C (^C) to stop.", "─".repeat(40)],
126-
env: execFileEnv(env),
127118
});
128119
}
129120

@@ -139,11 +130,7 @@ export async function openAppStatusTerminal(
139130
},
140131
): Promise<void> {
141132
const globalFlags = getGlobalShellFlags(env.configs, env.auth);
142-
// Pass only the delta; createTerminal merges it on top of the user's terminal env.
143-
const terminal = vscode.window.createTerminal({
144-
name: app.name,
145-
env: authEnv(env),
146-
});
133+
const terminal = vscode.window.createTerminal({ name: app.name });
147134
terminal.sendText(
148135
`${escapeCommandArg(env.binary)} ${globalFlags.join(" ")} ssh ${escapeCommandArg(app.workspace_name)}`,
149136
);
@@ -154,22 +141,6 @@ export async function openAppStatusTerminal(
154141

155142
const execFileAsync = promisify(execFile);
156143

157-
/**
158-
* Full env for `execFile`/`spawn`, which replace the child env entirely:
159-
* inherit `process.env` and overlay auth on top.
160-
*/
161-
function execFileEnv(env: CliEnv): NodeJS.ProcessEnv {
162-
return { ...process.env, ...authEnv(env) };
163-
}
164-
165-
/** Auth env vars to forward to a coder child process. */
166-
function authEnv(env: CliEnv): NodeJS.ProcessEnv {
167-
return {
168-
CODER_URL: env.childCredentials.url,
169-
CODER_SESSION_TOKEN: env.childCredentials.token,
170-
};
171-
}
172-
173144
/** Prefer stderr over the default message which includes the full command line. */
174145
function cliError(error: unknown): Error {
175146
// Pass aborts through; wrapping erases the AbortError name and would surface stale CLI warnings as the failure.
@@ -197,7 +168,6 @@ function spawnCliInTerminal(options: {
197168
binary: string;
198169
args: string[];
199170
banner: string[];
200-
env: NodeJS.ProcessEnv;
201171
}): vscode.Terminal {
202172
const writeEmitter = new vscode.EventEmitter<string>();
203173
const closeEmitter = new vscode.EventEmitter<number | void>();
@@ -211,7 +181,6 @@ function spawnCliInTerminal(options: {
211181
const proc = spawn(cmd, {
212182
shell: true,
213183
detached: useProcessGroup,
214-
env: options.env,
215184
});
216185

217186
let closed = false;

‎src/login/loginCoordinator.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -153,12 +153,9 @@ export class LoginCoordinator implements vscode.Disposable {
153153
});
154154
await this.mementoManager.addToUrlHistory(url);
155155

156-
// Fire-and-forget: sync token to OS keyring for the CLI.
157156
if (result.token) {
158157
this.cliCredentialManager
159-
.storeToken(url, result.token, vscode.workspace.getConfiguration(), {
160-
keyringOnly: true,
161-
})
158+
.storeToken(url, result.token, vscode.workspace.getConfiguration())
162159
.catch((error) => {
163160
this.logger.warn(
164161
"Failed to store token in keyring at login:",

‎test/unit/core/cliCredentialManager.test.ts‎

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -269,42 +269,6 @@ describe("CliCredentialManager", () => {
269269
}),
270270
).rejects.toThrow("The operation was aborted");
271271
});
272-
273-
it.each([
274-
{ scenario: "keyring disabled", keyringEnabled: false },
275-
{ scenario: "CLI version too old", keyringEnabled: true },
276-
])(
277-
"never writes files when keyringOnly and $scenario",
278-
async ({ keyringEnabled }) => {
279-
vi.mocked(isKeyringEnabled).mockReturnValue(keyringEnabled);
280-
if (keyringEnabled) {
281-
vi.mocked(cliExec.version).mockResolvedValueOnce("2.28.0");
282-
}
283-
const { manager } = setup();
284-
285-
await manager.storeToken(TEST_URL, "token", configs, {
286-
keyringOnly: true,
287-
});
288-
289-
expect(execFile).not.toHaveBeenCalled();
290-
expect(memfs.existsSync(URL_FILE)).toBe(false);
291-
expect(memfs.existsSync(SESSION_FILE)).toBe(false);
292-
},
293-
);
294-
295-
it("uses keyring without writing files when keyringOnly and keyring available", async () => {
296-
vi.mocked(isKeyringEnabled).mockReturnValue(true);
297-
stubExecFile({ stdout: "" });
298-
const { manager } = setup();
299-
300-
await manager.storeToken(TEST_URL, "my-token", configs, {
301-
keyringOnly: true,
302-
});
303-
304-
expect(execFile).toHaveBeenCalled();
305-
expect(memfs.existsSync(URL_FILE)).toBe(false);
306-
expect(memfs.existsSync(SESSION_FILE)).toBe(false);
307-
});
308272
});
309273

310274
describe("readToken", () => {

‎test/unit/core/cliExec.test.ts‎

Lines changed: 17 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,13 @@ import path from "path";
44
import { beforeAll, describe, expect, it, vi } from "vitest";
55

66
import { MockConfigurationProvider } from "../../mocks/testHelpers";
7-
import { isWindows, quoteCommand, writeExecutable } from "../../utils/platform";
7+
import {
8+
isWindows,
9+
quoteCommand,
10+
writeExecutable,
11+
writeStderrJs,
12+
writeStdoutJs,
13+
} from "../../utils/platform";
814

915
import type { CliEnv } from "@/core/cliExec";
1016

@@ -30,18 +36,13 @@ describe("cliExec", () => {
3036

3137
function setup(auth: CliEnv["auth"], binary = echoArgsBin) {
3238
const configs = new MockConfigurationProvider();
33-
const env: CliEnv = {
34-
binary,
35-
auth,
36-
configs,
37-
childCredentials: { url: "http://localhost:3000", token: "test-token" },
38-
};
39+
const env: CliEnv = { binary, auth, configs };
3940
return { configs, env };
4041
}
4142

4243
/** JS code for a fake CLI that writes a fixed string to stdout. */
4344
function echoBin(output: string): string {
44-
return `process.stdout.write(${JSON.stringify(output)});`;
45+
return writeStdoutJs(output);
4546
}
4647

4748
/**
@@ -51,10 +52,11 @@ describe("cliExec", () => {
5152
function oldCliBin(stderr: string, stdout: string): string {
5253
return [
5354
`if (process.argv.includes("--output")) {`,
54-
` process.stderr.write(${JSON.stringify(stderr)});`,
55-
` process.exitCode = 1;`,
55+
` ${writeStderrJs(stderr)}`,
56+
` process.exit(1);`,
5657
`} else {`,
57-
` process.stdout.write(${JSON.stringify(stdout)});`,
58+
` ${writeStdoutJs(stdout)}`,
59+
` process.exit(0);`,
5860
`}`,
5961
].join("\n");
6062
}
@@ -164,8 +166,8 @@ describe("cliExec", () => {
164166

165167
it("surfaces stderr instead of full command line on failure", async () => {
166168
const code = [
167-
`process.stderr.write("invalid argument for -t flag\\n");`,
168-
`process.exitCode = 1;`,
169+
writeStderrJs("invalid argument for -t flag\n"),
170+
`process.exit(1);`,
169171
].join("\n");
170172
const bin = await writeExecutable(tmp, "speedtest-err", code);
171173
const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin);
@@ -174,26 +176,6 @@ describe("cliExec", () => {
174176
).rejects.toThrow("invalid argument for -t flag");
175177
});
176178

177-
it("forwards CODER_URL and CODER_SESSION_TOKEN to the child", async () => {
178-
const code = [
179-
`process.stdout.write(JSON.stringify({`,
180-
` url: process.env.CODER_URL || "",`,
181-
` token: process.env.CODER_SESSION_TOKEN || "",`,
182-
`}));`,
183-
].join("\n");
184-
const bin = await writeExecutable(tmp, "speedtest-env", code);
185-
const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin);
186-
env.childCredentials = {
187-
url: "http://localhost:3000",
188-
token: "secret-token",
189-
};
190-
const out = await cliExec.speedtest(env, "owner/workspace");
191-
expect(JSON.parse(out)).toEqual({
192-
url: "http://localhost:3000",
193-
token: "secret-token",
194-
});
195-
});
196-
197179
it("preserves AbortError name when cancelled via signal", async () => {
198180
// Hangs forever so the only way out is the abort signal.
199181
const code = `setInterval(() => {}, 1000);`;
@@ -205,23 +187,6 @@ describe("cliExec", () => {
205187
cliExec.speedtest(env, "owner/workspace", undefined, ac.signal),
206188
).rejects.toMatchObject({ name: "AbortError" });
207189
});
208-
209-
it("forwards an empty CODER_SESSION_TOKEN for mTLS", async () => {
210-
const code = [
211-
`process.stdout.write(JSON.stringify({`,
212-
` url: process.env.CODER_URL,`,
213-
` token: process.env.CODER_SESSION_TOKEN,`,
214-
`}));`,
215-
].join("\n");
216-
const bin = await writeExecutable(tmp, "speedtest-env-mtls", code);
217-
const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin);
218-
env.childCredentials = { url: "http://localhost:3000", token: "" };
219-
const out = await cliExec.speedtest(env, "owner/workspace");
220-
expect(JSON.parse(out)).toEqual({
221-
url: "http://localhost:3000",
222-
token: "",
223-
});
224-
});
225190
});
226191

227192
describe("supportBundle", () => {
@@ -258,8 +223,8 @@ describe("cliExec", () => {
258223

259224
it("surfaces stderr on failure", async () => {
260225
const code = [
261-
`process.stderr.write("workspace not found\\n");`,
262-
`process.exitCode = 1;`,
226+
writeStderrJs("workspace not found\n"),
227+
`process.exit(1);`,
263228
].join("\n");
264229
const bin = await writeExecutable(tmp, "sb-err", code);
265230
const { env } = setup({ mode: "global-config", configDir: "/tmp" }, bin);

‎test/unit/login/loginCoordinator.test.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -500,7 +500,7 @@ describe("LoginCoordinator", () => {
500500
return { ...ctx, user, login };
501501
}
502502

503-
it("calls storeToken with keyringOnly after successful login", async () => {
503+
it("calls storeToken after successful login", async () => {
504504
const { mockCredentialManager, login } = await loginWithStoredToken();
505505

506506
await login();
@@ -509,7 +509,6 @@ describe("LoginCoordinator", () => {
509509
TEST_URL,
510510
"stored-token",
511511
expect.anything(),
512-
{ keyringOnly: true },
513512
);
514513
});
515514

‎test/utils/platform.test.ts‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
printEnvCommand,
1414
shimExecFile,
1515
writeExecutable,
16+
writeStdoutJs,
1617
} from "./platform";
1718

1819
describe("platform utils", () => {
@@ -123,11 +124,7 @@ describe("platform utils", () => {
123124
});
124125

125126
it("runs .js files through node", async () => {
126-
const script = await writeExecutable(
127-
tmp,
128-
"echo",
129-
'process.stdout.write("ok");',
130-
);
127+
const script = await writeExecutable(tmp, "echo", writeStdoutJs("ok"));
131128
const { stdout } = await execFileAsync(script);
132129
expect(stdout).toBe("ok");
133130
});
@@ -136,7 +133,7 @@ describe("platform utils", () => {
136133
const script = await writeExecutable(
137134
tmp,
138135
"echo-args",
139-
"process.stdout.write(process.argv.slice(2).join(','));",
136+
`require("fs").writeSync(1, process.argv.slice(2).join(","));`,
140137
);
141138
const { stdout } = await execFileAsync(script, ["a", "b", "c"]);
142139
expect(stdout).toBe("a,b,c");
@@ -149,11 +146,7 @@ describe("platform utils", () => {
149146
});
150147

151148
it("preserves the callback form", async () => {
152-
const script = await writeExecutable(
153-
tmp,
154-
"cb-echo",
155-
'process.stdout.write("cb");',
156-
);
149+
const script = await writeExecutable(tmp, "cb-echo", writeStdoutJs("cb"));
157150
const stdout = await new Promise<string>((resolve, reject) => {
158151
mod.execFile(script, (err, out) =>
159152
err ? reject(new Error(err.message)) : resolve(out),

0 commit comments

Comments
 (0)