fix(oauth): address review on CLI token env and client registrations

- Spawned CLI processes get `process.env` without `CODER_SESSION_TOKEN`,
  matched as the CLI reads it: any letter case, with or without the
  `HOMEBREW_` prefix. Only shell command strings (the SSH ProxyCommand
  and `terminal.sendText`) keep an inline `--token=`, since the
  extension can't set their environment.
- Save a client registration only after the OAuth login succeeds, so a
  failed login keeps the registration a stored session was issued to.
- Rename the recorded OAuth client to `registration` to tell the
  registered app apart from the HTTP client.
- Offer OAuth only when discovery advertises `registration_endpoint`,
  which Coder omits while dynamic client registration is off (the
  default from 2.37), instead of failing after the user picks OAuth.

Author: EhabY

CHANGELOG.md

@@ -27,11 +27,12 @@
   the stored session.
 - Preserve OAuth refresh credentials when reusing a stored token, and request
   the permissions needed to refresh workspace external-auth links.
-- If a deployment disables dynamic client registration (the default from Coder
-  2.38), OAuth sign-in now names the setting and offers session-token sign-in.
-  OAuth becomes the default sign-in method in a future release, so ask your
-  Coder administrator to enable dynamic client registration. For instructions,
-  see [OAuth2 provider](https://coder.com/docs/admin/integrations/oauth2-provider).
+- OAuth sign-in is offered only when the deployment allows dynamic client
+  registration, which is off by default from Coder 2.37; otherwise sign-in uses
+  a session token. OAuth becomes the default sign-in method in a future
+  release, so ask your Coder administrator to enable dynamic client
+  registration. For instructions, see
+  [OAuth2 provider](https://coder.com/docs/admin/integrations/oauth2-provider).
 - Keep session tokens, OAuth tokens, and client secrets from failed requests out
   of the buffered logs that the Coder output channel replays.
 - CLI commands and SSH connections that the extension starts ignore

src/api/workspace.ts

@@ -2,7 +2,11 @@ import { spawn } from "node:child_process";
 import * as vscode from "vscode";
 
 import { errToStr } from "../error/normalize";
-import { getGlobalFlags, type CliAuth } from "../settings/cli";
+import {
+	getCliProcessEnv,
+	getGlobalFlags,
+	type CliAuth,
+} from "../settings/cli";
 
 import { createWorkspaceIdentifier } from "./models";
 
@@ -80,7 +84,7 @@ function runCliCommand(ctx: CliContext, args: string[]): Promise<void> {
 			...args,
 			createWorkspaceIdentifier(ctx.workspace),
 		];
-		const proc = spawn(ctx.binPath, fullArgs);
+		const proc = spawn(ctx.binPath, fullArgs, { env: getCliProcessEnv() });
 		// Unexpected prompts EOF instead of hanging forever.
 		proc.stdin.end();
 

src/cli/cliCredentialManager.ts

@@ -14,7 +14,7 @@ import {
 import { recordError } from "../instrumentation/outcomes";
 import {
 	type CliAuth,
-	EMPTY_TOKEN_FLAG,
+	getCliProcessEnv,
 	getGlobalFlags,
 	mayUseCliStore,
 	resolveCliAuth,
@@ -76,16 +76,10 @@ export class CliCredentialManager {
 				return;
 			}
 			span.setProperty("store", cli.auth.store);
-			// `login` reads the token from the environment, so drop the empty flag.
-			const flags = cli.flags.filter((flag) => flag !== EMPTY_TOKEN_FLAG);
-			await this.exec(
-				{ ...cli, flags },
-				["login", "--use-token-as-session", url],
-				{
-					env: envWithSessionToken(token),
-					signal: options?.signal,
-				},
-			);
+			await this.exec(cli, ["login", "--use-token-as-session", url], {
+				env: { ...getCliProcessEnv(), CODER_SESSION_TOKEN: token },
+				signal: options?.signal,
+			});
 			span.setProperty("outcome", "stored");
 			this.logger.info("Stored token via CLI for", url);
 		});
@@ -241,6 +235,7 @@ export class CliCredentialManager {
 		}, EXEC_LOG_INTERVAL_MS);
 		try {
 			return await execFileAsync(cli.binPath, [...cli.flags, ...args], {
+				env: getCliProcessEnv(),
 				...options,
 				timeout: EXEC_TIMEOUT_MS,
 			});
@@ -275,13 +270,3 @@ export class CliCredentialManager {
 		return results.every(Boolean);
 	}
 }
-
-/** `process.env` with `token` as its only `CODER_SESSION_TOKEN`, in any case. */
-function envWithSessionToken(token: string): NodeJS.ProcessEnv {
-	const env = Object.fromEntries(
-		Object.entries(process.env).filter(
-			([key]) => key.toUpperCase() !== "CODER_SESSION_TOKEN",
-		),
-	);
-	return { ...env, CODER_SESSION_TOKEN: token };
-}

src/cli/cliExec.ts

@@ -8,6 +8,7 @@ import { isAbortError } from "../error/abort";
 import { toError } from "../error/normalize";
 import {
 	type CliAuth,
+	getCliProcessEnv,
 	getGlobalFlags,
 	getGlobalShellFlags,
 } from "../settings/cli";
@@ -88,7 +89,10 @@ export async function speedtest(
 		args.push("-t", duration);
 	}
 	try {
-		const result = await execFileAsync(env.binary, args, { signal });
+		const result = await execFileAsync(env.binary, args, {
+			env: getCliProcessEnv(),
+			signal,
+		});
 		return result.stdout;
 	} catch (error) {
 		throw cliError(error);
@@ -107,7 +111,7 @@ export async function netcheck(
 		const result = await execFileAsync(
 			env.binary,
 			[...globalFlags, "netcheck"],
-			{ signal },
+			{ env: getCliProcessEnv(), signal },
 		);
 		return result.stdout;
 	} catch (error) {
@@ -142,7 +146,7 @@ export async function supportBundle(
 		...workspaceFiles.flatMap((file) => ["--workspace-file", file]),
 	];
 	try {
-		await execFileAsync(env.binary, args, { signal });
+		await execFileAsync(env.binary, args, { env: getCliProcessEnv(), signal });
 	} catch (error) {
 		throw cliError(error);
 	}
@@ -221,6 +225,7 @@ function spawnCliInTerminal(options: {
 	// fall back to proc.kill() there.
 	const useProcessGroup = process.platform !== "win32";
 	const proc = spawn(options.binary, options.args, {
+		env: getCliProcessEnv(),
 		detached: useProcessGroup,
 	});
 

src/login/loginCoordinator.ts

@@ -7,10 +7,7 @@ import { needToken } from "../api/httpAgent";
 import { HttpStatusCode } from "../api/httpStatusCode";
 import { isSameOrigin, openInBrowser } from "../common/url";
 import { CertificateError } from "../error/certificateError";
-import {
-	ClientRegistrationDisabledError,
-	OAuthAuthorizer,
-} from "../oauth/authorizer";
+import { OAuthAuthorizer } from "../oauth/authorizer";
 import { revokeOAuthTokens } from "../oauth/revocation";
 import { buildOAuthTokenData } from "../oauth/tokens";
 import { withCancellableProgress } from "../ui/progress";
@@ -289,18 +286,19 @@ export class LoginCoordinator implements vscode.Disposable {
 
 	/**
 	 * Attempt to authenticate using OAuth, token, or mTLS. If necessary, prompts
-	 * for authentication method and credentials. Returns the token, user, and
-	 * method on success, or a bounded reason when the user aborts or
-	 * authentication fails.
+	 * for authentication method and credentials. On success, persists the
+	 * session, revokes the OAuth session it replaces, and returns the token,
+	 * user, and method. Otherwise returns a bounded reason when the user aborts
+	 * or authentication fails.
 	 */
 	private async attemptLogin(
 		deployment: Deployment,
 		isAutoLogin: boolean,
 		providedToken?: string,
 		tokenSignInConfirmed = false,
 	): Promise<LoginResult> {
-		// Revokes sessions that don't record their client. Read it before login can
-		// re-register the client.
+		// Read before OAuth login replaces it; it revokes a replaced session saved
+		// without its own registration.
 		const registration = await this.secretsManager.getOAuthClientRegistration(
 			deployment.safeHostname,
 		);
@@ -499,16 +497,11 @@ export class LoginCoordinator implements vscode.Disposable {
 	private async askAuthMethod(ctx: LoginAttemptContext): Promise<LoginResult> {
 		const authMethod = await maybeAskAuthMethod(ctx.client);
 		switch (authMethod) {
-			case "oauth": {
-				const result = await this.loginWithOAuth(ctx.deployment);
-				if (result !== "use_token") {
-					return withLoginMethod("oauth", result);
-				}
+			case "oauth":
 				return withLoginMethod(
-					"cli_token",
-					await this.loginWithToken(ctx.client),
+					"oauth",
+					await this.loginWithOAuth(ctx.deployment),
 				);
-			}
 			case "legacy":
 				return withLoginMethod(
 					"cli_token",
@@ -663,10 +656,12 @@ export class LoginCoordinator implements vscode.Disposable {
 		return { success: false, reason: "user_dismissed" };
 	}
 
-	/** OAuth flow; "use_token" means the user chose session-token sign-in instead. */
+	/**
+	 * OAuth authentication flow.
+	 */
 	private async loginWithOAuth(
 		deployment: Deployment,
-	): Promise<LoginAttemptResult | "use_token"> {
+	): Promise<LoginAttemptResult> {
 		try {
 			this.logger.debug("Starting OAuth authentication");
 
@@ -692,21 +687,6 @@ export class LoginCoordinator implements vscode.Disposable {
 				oauth: buildOAuthTokenData(tokenResponse, registration),
 			};
 		} catch (error) {
-			if (error instanceof ClientRegistrationDisabledError) {
-				this.logger.warn("OAuth sign-in unavailable:", error.message);
-				const action = await vscodeProposed.window.showWarningMessage(
-					"OAuth sign-in is unavailable",
-					{
-						useCustom: true,
-						modal: true,
-						detail: `${error.message}. To use OAuth, ask an administrator to set dynamic_client_registration_enabled.`,
-					},
-					"Sign In with Token",
-				);
-				return action === "Sign In with Token"
-					? "use_token"
-					: { success: false, reason: "user_dismissed" };
-			}
 			const title = "OAuth authentication failed";
 			this.logger.error(title, error);
 			if (error instanceof CertificateError) {

src/oauth/authorizer.ts

@@ -1,9 +1,7 @@
-import { type AxiosInstance, isAxiosError } from "axios";
 import { createHash, randomBytes } from "node:crypto";
 import * as vscode from "vscode";
 
 import { CoderApi } from "../api/coderApi";
-import { HttpStatusCode } from "../api/httpStatusCode";
 import { resolveCoderDashboardUrl } from "../common/url";
 
 import {
@@ -23,6 +21,7 @@ import {
 	parseOAuthResponse,
 } from "./validation";
 
+import type { AxiosInstance } from "axios";
 import type {
 	OAuth2AuthorizationServerMetadata,
 	OAuth2ClientRegistrationRequest,
@@ -41,18 +40,10 @@ import type { OAuthCallback } from "./oauthCallback";
 interface OAuthLoginResult {
 	tokenResponse: OAuth2TokenResponse;
 	user: User;
-	/** Client the tokens were issued to. */
+	/** Registration the tokens were issued to. */
 	registration: OAuth2ClientRegistrationResponse;
 }
 
-/** The server refuses client registration, which is off by default from Coder 2.38. */
-export class ClientRegistrationDisabledError extends Error {
-	constructor() {
-		super("Dynamic client registration is disabled on this deployment");
-		this.name = "ClientRegistrationDisabledError";
-	}
-}
-
 /**
  * Handles the OAuth authorization code flow for authenticating with Coder deployments.
  * Encapsulates client registration, PKCE challenge, and token exchange.
@@ -67,7 +58,7 @@ export class OAuthAuthorizer implements vscode.Disposable {
 		private readonly extensionId: string,
 	) {}
 
-	/** Runs the OAuth login flow. Saves the client registration, not the tokens. */
+	/** Runs the OAuth login flow. On success, saves the client registration but not the tokens. */
 	public async login(
 		deployment: Deployment,
 		progress: vscode.Progress<{ message?: string; increment?: number }>,
@@ -134,6 +125,11 @@ export class OAuthAuthorizer implements vscode.Disposable {
 		reportProgress("fetching user...", 20);
 		const user = await client.getAuthenticatedUser();
 
+		// Save only on success so a failed login keeps the stored session's registration.
+		await this.secretsManager.setOAuthClientRegistration(
+			deployment.safeHostname,
+			registration,
+		);
 		this.logger.debug("OAuth login flow completed successfully");
 
 		return { tokenResponse, user, registration };
@@ -183,33 +179,19 @@ export class OAuthAuthorizer implements vscode.Disposable {
 			token_endpoint_auth_method: TOKEN_ENDPOINT_AUTH_METHOD,
 		};
 
-		const response = await axiosInstance
-			.post<OAuth2ClientRegistrationResponse>(
-				metadata.registration_endpoint,
-				registrationRequest,
-			)
-			.catch((error: unknown) => {
-				if (
-					isAxiosError(error) &&
-					error.response?.status === HttpStatusCode.FORBIDDEN
-				) {
-					throw new ClientRegistrationDisabledError();
-				}
-				throw error;
-			});
+		const response = await axiosInstance.post<OAuth2ClientRegistrationResponse>(
+			metadata.registration_endpoint,
+			registrationRequest,
+		);
 
 		const registrationResponse = parseOAuthResponse(
 			OAuth2ClientRegistrationResponseSchema,
 			response.data,
 			metadata.registration_endpoint,
 		);
 
-		await this.secretsManager.setOAuthClientRegistration(
-			deployment.safeHostname,
-			registrationResponse,
-		);
 		this.logger.debug(
-			"Saved OAuth client registration:",
+			"Registered OAuth client:",
 			registrationResponse.client_id,
 		);
 

src/oauth/metadataClient.ts

@@ -48,14 +48,17 @@ export class OAuthMetadataClient {
 	) {}
 
 	/**
-	 * Check if a server supports OAuth by attempting to fetch the well-known endpoint.
+	 * True when the server advertises OAuth client registration. Coder omits it
+	 * while dynamic client registration is off, the default from 2.37.
 	 */
 	public static async checkOAuthSupport(
 		axiosInstance: AxiosInstance,
 	): Promise<boolean> {
 		try {
-			await axiosInstance.get(OAUTH_DISCOVERY_ENDPOINT);
-			return true;
+			const response = await axiosInstance.get<
+				Partial<OAuth2AuthorizationServerMetadata>
+			>(OAUTH_DISCOVERY_ENDPOINT);
+			return Boolean(response.data.registration_endpoint);
 		} catch {
 			return false;
 		}

src/oauth/revocation.ts

@@ -1,25 +1,25 @@
 import { withOAuthMetadata } from "./metadataClient";
-import { type OAuthClient, toUrlSearchParams } from "./tokens";
+import { type OAuthClientRegistration, toUrlSearchParams } from "./tokens";
 
 import type { OAuth2TokenRevocationRequest } from "coder/site/src/api/typesGenerated";
 
 import type { Logger } from "../logging/logger";
 import type { SessionAuth } from "../storage/secretsManager";
 
 /**
- * Best-effort revocation of the token pair in `auth`; never throws. Signs in as
- * the client that issued the tokens, else `fallbackClient`.
+ * Best-effort revocation of the token pair in `auth`; never throws. Authenticates
+ * as the registration the tokens were issued to, else `storedRegistration`.
  */
 export async function revokeOAuthTokens(
 	auth: SessionAuth,
-	fallbackClient: OAuthClient | undefined,
+	storedRegistration: OAuthClientRegistration | undefined,
 	logger: Logger,
 ): Promise<void> {
 	if (!auth.oauth) {
 		return;
 	}
-	const client = auth.oauth.client ?? fallbackClient;
-	if (!client) {
+	const registration = auth.oauth.registration ?? storedRegistration;
+	if (!registration) {
 		logger.warn("Skipped token revocation: no client registration");
 		return;
 	}
@@ -46,8 +46,8 @@ export async function revokeOAuthTokens(
 					}
 					const params: OAuth2TokenRevocationRequest = {
 						token,
-						client_id: client.client_id,
-						client_secret: client.client_secret,
+						client_id: registration.client_id,
+						client_secret: registration.client_secret,
 						token_type_hint,
 					};
 					try {