Skip to content

Commit d22be3f

Browse files
authored
fix(oauth): revoke replaced sessions and keep credentials out of the CLI and logs (#1147)
Clean up replaced OAuth sessions without forcing a re-login when required scopes change, offer OAuth only when the deployment can register clients, and stop host credentials and secrets from leaking into CLI calls and logs. - Keep tokens missing required scopes until they expire, but stop refreshing them; normal 401 recovery applies after expiry. - Revoke the replaced OAuth session in the background after a successful sign-in; failed or canceled sign-ins and same-token reuse revoke nothing. - Record the registration each session's tokens were issued to, so refresh and revocation keep working when another window re-registers; save a new registration only after login succeeds. - Preserve refresh credentials, scopes, and registration when reusing the same token; a session rotated by a concurrent refresh wins. - Request `user:update_personal` so `coder start` can refresh expired external-auth links. - Offer OAuth only when discovery advertises `registration_endpoint`, which Coder omits while dynamic client registration is off (the default from 2.37); a notice explains the session-token fallback. - Spawn CLI processes without `CODER_SESSION_TOKEN` in any letter case or with the `HOMEBREW_` prefix; the SSH ProxyCommand with an empty `--token=`, since the extension can't set its environment. - Redact credentials from every logged value, in the output channel and the connection log buffer, and summarize Axios errors at any depth. Closes #1140
1 parent b738dbe commit d22be3f

33 files changed

Lines changed: 1021 additions & 314 deletions

‎AGENTS.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,8 @@ deployment. Treat them as untrusted:
6868

6969
- Run the CLI with `execFile`/`spawn` argv (or a terminal's `shellPath` and
7070
`shellArgs`), never through a shell or `terminal.sendText`.
71+
- Pass `env: getCliProcessEnv()` (plus `strictEnv: true` for a terminal) so
72+
a host `CODER_SESSION_TOKEN` cannot replace the stored session.
7173
- Put flags first, then `--`, then deployment-provided operands
7274
(`coder ping -- owner/ws`) so a value like `--foo` is never read as a flag.
7375
- Validate names with `src/common/validation.ts` before they reach SSH hosts,

‎CHANGELOG.md‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,24 @@
2020

2121
### Fixed
2222

23-
- OAuth sign-in now works on deployments that enforce token scopes. Users signed
24-
in with OAuth are asked to sign in again once.
25-
- Revoke OAuth tokens on logout even when their scopes are outdated.
23+
- OAuth sign-in now works on deployments that enforce token scopes. Sessions
24+
missing required scopes stop refreshing and require sign-in when they expire.
25+
- Revoke OAuth tokens on logout and after successful session replacement, even
26+
when their scopes are outdated. Failed or canceled sign-ins do not revoke
27+
the stored session.
28+
- Preserve OAuth refresh credentials when reusing a stored token, and request
29+
the permissions needed to refresh workspace external-auth links.
30+
- OAuth sign-in is offered only when the deployment allows dynamic client
31+
registration, which is off by default from Coder 2.37; otherwise a notice
32+
says sign-in uses a session token. OAuth becomes the default sign-in method
33+
in a future release, so ask your Coder administrator to enable dynamic client
34+
registration. For instructions, see
35+
[OAuth2 provider](https://coder.com/docs/admin/integrations/oauth2-provider).
36+
- Keep session tokens, OAuth tokens, and client secrets out of the Coder output
37+
channel and its buffered logs, including from failed requests.
38+
- CLI commands and SSH connections that the extension starts ignore
39+
`CODER_SESSION_TOKEN` from the environment, so they no longer fail or run as a
40+
different user inside a Coder workspace.
2641

2742
### Fixed
2843

‎src/api/authInterceptor.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ export class AuthInterceptor implements vscode.Disposable {
102102

103103
// 1) OAuth refresh path.
104104
const isOAuth =
105-
await this.oauthSessionManager.isLoggedInWithOAuth(hostname);
105+
await this.oauthSessionManager.canRefreshOAuthSession(hostname);
106106
recorder.setRefreshAttempted(isOAuth);
107107
if (isOAuth) {
108108
const newToken = await this.tryOAuthRefresh();

‎src/api/workspace.ts‎

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

44
import { errToStr } from "../error/normalize";
5-
import { getGlobalFlags, type CliAuth } from "../settings/cli";
5+
import {
6+
getCliProcessEnv,
7+
getGlobalFlags,
8+
type CliAuth,
9+
} from "../settings/cli";
610

711
import { createWorkspaceIdentifier } from "./models";
812

@@ -81,7 +85,7 @@ function runCliCommand(ctx: CliContext, args: string[]): Promise<void> {
8185
"--",
8286
createWorkspaceIdentifier(ctx.workspace),
8387
];
84-
const proc = spawn(ctx.binPath, fullArgs);
88+
const proc = spawn(ctx.binPath, fullArgs, { env: getCliProcessEnv() });
8589
// Unexpected prompts EOF instead of hanging forever.
8690
proc.stdin.end();
8791

‎src/cli/cliCredentialManager.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
import { recordError } from "../instrumentation/outcomes";
1515
import {
1616
type CliAuth,
17+
getCliProcessEnv,
1718
getGlobalFlags,
1819
mayUseCliStore,
1920
resolveCliAuth,
@@ -76,7 +77,7 @@ export class CliCredentialManager {
7677
}
7778
span.setProperty("store", cli.auth.store);
7879
await this.exec(cli, ["login", "--use-token-as-session", url], {
79-
env: { ...process.env, CODER_SESSION_TOKEN: token },
80+
env: { ...getCliProcessEnv(), CODER_SESSION_TOKEN: token },
8081
signal: options?.signal,
8182
});
8283
span.setProperty("outcome", "stored");
@@ -234,6 +235,7 @@ export class CliCredentialManager {
234235
}, EXEC_LOG_INTERVAL_MS);
235236
try {
236237
return await execFileAsync(cli.binPath, [...cli.flags, ...args], {
238+
env: getCliProcessEnv(),
237239
...options,
238240
timeout: EXEC_TIMEOUT_MS,
239241
});

‎src/cli/cliExec.ts‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,11 @@ import { validateIdentifier } from "../common/validation";
77
import { isAbortError } from "../error/abort";
88
import { toError } from "../error/normalize";
99
import { type CliFeatureSet } from "../featureSet";
10-
import { type CliAuth, getGlobalFlags } from "../settings/cli";
10+
import {
11+
type CliAuth,
12+
getCliProcessEnv,
13+
getGlobalFlags,
14+
} from "../settings/cli";
1115

1216
/** Resolved CLI environment for invoking the coder binary. */
1317
export interface CliEnv {
@@ -86,7 +90,10 @@ export async function speedtest(
8690
}
8791
args.push("--", workspaceName);
8892
try {
89-
const result = await execFileAsync(env.binary, args, { signal });
93+
const result = await execFileAsync(env.binary, args, {
94+
env: getCliProcessEnv(),
95+
signal,
96+
});
9097
return result.stdout;
9198
} catch (error) {
9299
throw cliError(error);
@@ -105,7 +112,7 @@ export async function netcheck(
105112
const result = await execFileAsync(
106113
env.binary,
107114
[...globalFlags, "netcheck"],
108-
{ signal },
115+
{ env: getCliProcessEnv(), signal },
109116
);
110117
return result.stdout;
111118
} catch (error) {
@@ -141,7 +148,7 @@ export async function supportBundle(
141148
...(agentName ? [agentName] : []),
142149
];
143150
try {
144-
await execFileAsync(env.binary, args, { signal });
151+
await execFileAsync(env.binary, args, { env: getCliProcessEnv(), signal });
145152
} catch (error) {
146153
throw cliError(error);
147154
}
@@ -185,7 +192,13 @@ export function openAppStatusTerminal(
185192
app.command,
186193
];
187194
vscode.window
188-
.createTerminal({ name: app.name, shellPath: env.binary, shellArgs: args })
195+
.createTerminal({
196+
name: app.name,
197+
shellPath: env.binary,
198+
shellArgs: args,
199+
env: getCliProcessEnv(),
200+
strictEnv: true,
201+
})
189202
.show(false);
190203
}
191204

@@ -228,6 +241,7 @@ function spawnCliInTerminal(options: {
228241
// fall back to proc.kill() there.
229242
const useProcessGroup = process.platform !== "win32";
230243
const proc = spawn(options.binary, options.args, {
244+
env: getCliProcessEnv(),
231245
detached: useProcessGroup,
232246
});
233247

‎src/logging/flightRecorder.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { safeStringify } from "./formatters";
1+
import { redactSecrets, safeStringify } from "./formatters";
22

33
import type { LogSink, Logger } from "./logger";
44

@@ -109,8 +109,13 @@ export class FlightRecorder implements Logger {
109109
/** Pass a call through to the sink and buffer it when below the level. */
110110
private wrap(level: Level): (message: string, ...args: unknown[]) => void {
111111
return (message, ...args) => {
112-
this.record(level, message, args);
113-
this.inner[level](message, ...args);
112+
if (SEVERITY[level] < this.channel.logLevel) {
113+
// The channel drops this entry, so only the buffered copy is formatted.
114+
this.record(level, message, args);
115+
this.inner[level](message, ...args);
116+
} else {
117+
this.inner[level](message, ...args.map(redactSecrets));
118+
}
114119
};
115120
}
116121

@@ -130,7 +135,7 @@ export class FlightRecorder implements Logger {
130135
}
131136

132137
private record(level: Level, message: string, args: unknown[]): void {
133-
if (this.capacity === 0 || SEVERITY[level] >= this.channel.logLevel) {
138+
if (this.capacity === 0) {
134139
return;
135140
}
136141
const text = [message, ...args.map((arg) => safeStringify(arg) ?? "")].join(

0 commit comments

Comments
 (0)