From 4e7fb3769a11acbb482d917e9d645f354423bfea Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Sat, 10 Oct 2026 18:52:53 +0300 Subject: [PATCH 1/4] fix(oauth): revoke replaced sessions and preserve token metadata Clean up replaced OAuth sessions without forcing a re-login when the required scopes change. - Insufficient scopes prevent refresh but keep the stored access token, so normal 401 recovery applies after expiry. Rename the check to `canRefreshOAuthSession`. - Save a successful replacement first, then revoke the overwritten OAuth pair in the background with the client registration captured before login. Failed or canceled logins, same-token reuse, and stored-session adoption revoke nothing. - Keep refresh credentials and scopes when the same token is reused on the same origin, instead of turning it into a manual-token session. If a refresh rotates the session while its stored token is checked, keep the rotated session. - Share revocation with logout through `withOAuthMetadata`, which token refresh also uses, and request `user:update_personal` to refresh expired external-auth links. - If the server refuses dynamic client registration (off by default from Coder 2.38), name the setting and offer session-token sign-in. - Start every CLI invocation's global flags with an empty `--token=`, so `CODER_SESSION_TOKEN` in the extension host (in any case) no longer overrides the stored session. This covers the SSH ProxyCommand and terminals too. `coder login --use-token-as-session` omits it, since it reads the token from the environment, and gets an environment without the host's case variants of the variable. Closes #1140 --- CHANGELOG.md | 15 +- src/api/authInterceptor.ts | 2 +- src/cli/cliCredentialManager.ts | 25 ++- src/login/loginCoordinator.ts | 125 ++++++++++----- src/oauth/authorizer.ts | 29 +++- src/oauth/constants.ts | 5 +- src/oauth/metadataClient.ts | 28 ++++ src/oauth/revocation.ts | 66 ++++++++ src/oauth/sessionManager.ts | 122 ++++----------- src/settings/cli.ts | 7 +- test/mocks/testHelpers.ts | 2 +- test/unit/api/authInterceptor.test.ts | 8 +- test/unit/api/workspace.test.ts | 15 +- test/unit/cli/cliCredentialManager.test.ts | 11 +- test/unit/cli/cliExec.test.ts | 16 +- test/unit/login/loginCoordinator.test.ts | 173 ++++++++++++++++++++- test/unit/oauth/authorizer.test.ts | 21 ++- test/unit/oauth/sessionManager.test.ts | 22 +-- test/unit/remote/remote.test.ts | 2 +- test/unit/settings/cli.test.ts | 32 +++- 20 files changed, 540 insertions(+), 186 deletions(-) create mode 100644 src/oauth/revocation.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ee675121a1..6f8b0a6886 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,9 +20,18 @@ ### Fixed -- OAuth sign-in now works on deployments that enforce token scopes. Users signed - in with OAuth are asked to sign in again once. -- Revoke OAuth tokens on logout even when their scopes are outdated. +- OAuth sign-in now works on deployments that enforce token scopes. Sessions + missing required scopes stop refreshing and require sign-in when they expire. +- Revoke OAuth tokens on logout and after successful session replacement, even + when their scopes are outdated. Failed or canceled sign-ins do not revoke + 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. +- CLI commands and SSH connections that the extension starts ignore + `CODER_SESSION_TOKEN` from the environment, so they no longer fail or run as a + different user inside a Coder workspace. ### Fixed diff --git a/src/api/authInterceptor.ts b/src/api/authInterceptor.ts index 7ea7bc5cab..3925c7df70 100644 --- a/src/api/authInterceptor.ts +++ b/src/api/authInterceptor.ts @@ -102,7 +102,7 @@ export class AuthInterceptor implements vscode.Disposable { // 1) OAuth refresh path. const isOAuth = - await this.oauthSessionManager.isLoggedInWithOAuth(hostname); + await this.oauthSessionManager.canRefreshOAuthSession(hostname); recorder.setRefreshAttempted(isOAuth); if (isOAuth) { const newToken = await this.tryOAuthRefresh(); diff --git a/src/cli/cliCredentialManager.ts b/src/cli/cliCredentialManager.ts index 16bc140026..040e688c6f 100644 --- a/src/cli/cliCredentialManager.ts +++ b/src/cli/cliCredentialManager.ts @@ -14,6 +14,7 @@ import { import { recordError } from "../instrumentation/outcomes"; import { type CliAuth, + EMPTY_TOKEN_FLAG, getGlobalFlags, mayUseCliStore, resolveCliAuth, @@ -75,10 +76,16 @@ export class CliCredentialManager { return; } span.setProperty("store", cli.auth.store); - await this.exec(cli, ["login", "--use-token-as-session", url], { - env: { ...process.env, CODER_SESSION_TOKEN: token }, - signal: options?.signal, - }); + // `login` reads the token from the environment, which the empty flag would hide. + 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, + }, + ); span.setProperty("outcome", "stored"); this.logger.info("Stored token via CLI for", url); }); @@ -268,3 +275,13 @@ export class CliCredentialManager { return results.every(Boolean); } } + +/** `process.env` with `token` as the only session token, since the CLI reads the name 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 }; +} diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 98263562cf..929e49ac4d 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -7,7 +7,11 @@ import { needToken } from "../api/httpAgent"; import { HttpStatusCode } from "../api/httpStatusCode"; import { isSameOrigin, openInBrowser } from "../common/url"; import { CertificateError } from "../error/certificateError"; -import { OAuthAuthorizer } from "../oauth/authorizer"; +import { + ClientRegistrationDisabledError, + OAuthAuthorizer, +} from "../oauth/authorizer"; +import { revokeOAuthTokens } from "../oauth/revocation"; import { buildOAuthTokenData } from "../oauth/tokens"; import { withCancellableProgress } from "../ui/progress"; import { maybeAskAuthMethod, maybeAskUrl } from "../ui/prompts"; @@ -15,7 +19,10 @@ import { vscodeProposed } from "../vscode/proposed"; import { showStoreCredentialsError } from "./credentials"; -import type { User } from "coder/site/src/api/typesGenerated"; +import type { + User, + OAuth2ClientRegistrationResponse, +} from "coder/site/src/api/typesGenerated"; import type { CliCredentialManager } from "../cli/cliCredentialManager"; import type { Deployment } from "../deployment/types"; @@ -109,18 +116,14 @@ export class LoginCoordinator implements vscode.Disposable { options: LoginOptions & { url: string }, ): Promise { const { safeHostname, url } = options; - return this.executeWithGuard(async () => { - const result = await this.attemptLogin( + return this.executeWithGuard(() => + this.attemptLogin( { safeHostname, url }, options.autoLogin ?? false, options.token, options.tokenSignInConfirmed ?? false, - ); - - await this.persistSessionAuth(result, safeHostname, url); - - return result; - }); + ), + ); } /** @@ -170,15 +173,11 @@ export class LoginCoordinator implements vscode.Disposable { return { success: false, reason: "no_url_provided" }; } - const result = await this.attemptLogin( + return this.attemptLogin( { url: newUrl, safeHostname }, false, options.token, ); - - await this.persistSessionAuth(result, safeHostname, newUrl); - - return result; } // User cancelled return { success: false, reason: "user_dismissed" }; @@ -198,28 +197,42 @@ export class LoginCoordinator implements vscode.Disposable { } private async persistSessionAuth( - result: LoginResult, - safeHostname: string, - url: string, + result: Extract, + deployment: Deployment, + registration: OAuth2ClientRegistrationResponse | undefined, ): Promise { + const { safeHostname, url } = deployment; + const previous = await this.secretsManager.getSessionAuth(safeHostname); + const replaced = previous?.token === result.token ? undefined : previous; + // A refresh rotated the stored session while this login checked it. + if (result.method === "stored_token" && replaced) { + return; + } + const reusedOAuth = + !replaced && previous && isSameOrigin(previous.url, url) + ? previous.oauth + : undefined; + // Empty token is valid for mTLS - if (result.success) { - await this.secretsManager.setSessionAuth(safeHostname, { - url, - token: result.token, - username: result.user.username, - oauth: result.oauth, // undefined for non-OAuth logins - }); - await this.mementoManager.addToUrlHistory(url); - - if (result.token) { - const configs = vscode.workspace.getConfiguration(); - this.cliCredentialManager - .storeToken(url, result.token, configs) - .catch((error) => - showStoreCredentialsError(error, configs, this.logger), - ); - } + await this.secretsManager.setSessionAuth(safeHostname, { + url, + token: result.token, + username: result.user.username, + oauth: result.oauth ?? reusedOAuth, + }); + await this.mementoManager.addToUrlHistory(url); + + if (replaced?.oauth) { + void revokeOAuthTokens(replaced, registration, this.logger); + } + + if (result.token) { + const configs = vscode.workspace.getConfiguration(); + this.cliCredentialManager + .storeToken(url, result.token, configs) + .catch((error) => + showStoreCredentialsError(error, configs, this.logger), + ); } } @@ -286,15 +299,23 @@ export class LoginCoordinator implements vscode.Disposable { providedToken?: string, tokenSignInConfirmed = false, ): Promise { + // Read before an OAuth login can re-register the client. + const registration = await this.secretsManager.getOAuthClientRegistration( + deployment.safeHostname, + ); const client = CoderApi.create(deployment.url, "", this.logger); try { - return await this.runLoginAttempts( + const result = await this.runLoginAttempts( client, deployment, isAutoLogin, providedToken, tokenSignInConfirmed, ); + if (result.success) { + await this.persistSessionAuth(result, deployment, registration); + } + return result; } finally { client.dispose(); } @@ -477,11 +498,16 @@ export class LoginCoordinator implements vscode.Disposable { private async askAuthMethod(ctx: LoginAttemptContext): Promise { const authMethod = await maybeAskAuthMethod(ctx.client); switch (authMethod) { - case "oauth": + case "oauth": { + const result = await this.loginWithOAuth(ctx.deployment); + if (result !== "use_token") { + return withLoginMethod("oauth", result); + } return withLoginMethod( - "oauth", - await this.loginWithOAuth(ctx.deployment), + "cli_token", + await this.loginWithToken(ctx.client), ); + } case "legacy": return withLoginMethod( "cli_token", @@ -636,12 +662,10 @@ export class LoginCoordinator implements vscode.Disposable { return { success: false, reason: "user_dismissed" }; } - /** - * OAuth authentication flow. - */ + /** OAuth flow; "use_token" means the user chose session-token sign-in instead. */ private async loginWithOAuth( deployment: Deployment, - ): Promise { + ): Promise { try { this.logger.debug("Starting OAuth authentication"); @@ -666,6 +690,21 @@ export class LoginCoordinator implements vscode.Disposable { oauth: buildOAuthTokenData(tokenResponse), }; } 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) { diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index c3fcb7db75..b51ed572f3 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -1,7 +1,9 @@ +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 { @@ -21,7 +23,6 @@ import { parseOAuthResponse, } from "./validation"; -import type { AxiosInstance } from "axios"; import type { OAuth2AuthorizationServerMetadata, OAuth2ClientRegistrationRequest, @@ -37,6 +38,14 @@ import type { SecretsManager } from "../storage/secretsManager"; import type { OAuthCallback } from "./oauthCallback"; +/** 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. @@ -171,10 +180,20 @@ export class OAuthAuthorizer implements vscode.Disposable { token_endpoint_auth_method: TOKEN_ENDPOINT_AUTH_METHOD, }; - const response = await axiosInstance.post( - metadata.registration_endpoint, - registrationRequest, - ); + const response = await axiosInstance + .post( + metadata.registration_endpoint, + registrationRequest, + ) + .catch((error: unknown) => { + if ( + isAxiosError(error) && + error.response?.status === HttpStatusCode.FORBIDDEN + ) { + throw new ClientRegistrationDisabledError(); + } + throw error; + }); const registrationResponse = parseOAuthResponse( OAuth2ClientRegistrationResponseSchema, diff --git a/src/oauth/constants.ts b/src/oauth/constants.ts index 32c314fa1a..dee54fe768 100644 --- a/src/oauth/constants.ts +++ b/src/oauth/constants.ts @@ -3,8 +3,8 @@ export const AUTH_GRANT_TYPE = "authorization_code"; export const REFRESH_GRANT_TYPE = "refresh_token"; /** - * Scopes the extension and the CLI need. Stored sessions must have all of - * them, so adding one signs out users of servers that enforce scopes. + * Scopes the extension and the CLI need. Sessions missing one stop refreshing, + * so on servers that enforce scopes, adding one means signing in at expiry. */ export const DEFAULT_OAUTH_SCOPES = [ "coder:workspaces.operate", @@ -12,6 +12,7 @@ export const DEFAULT_OAUTH_SCOPES = [ "workspace:create", "user:read", "user:read_personal", + "user:update_personal", ].join(" "); /** diff --git a/src/oauth/metadataClient.ts b/src/oauth/metadataClient.ts index b6b5b4e24f..bb2d5fb31a 100644 --- a/src/oauth/metadataClient.ts +++ b/src/oauth/metadataClient.ts @@ -1,3 +1,4 @@ +import { CoderApi } from "../api/coderApi"; import { parseApiResponse } from "../api/responseValidation"; import { @@ -142,6 +143,33 @@ export class OAuthMetadataClient { } } +/** + * Run `fn` with a short-lived client for `url` and the server's OAuth + * metadata. The client is disposed on exit so its config-change + * subscriptions never outlive the operation. + */ +export async function withOAuthMetadata( + url: string, + token: string | undefined, + logger: Logger, + fn: (ctx: { + axiosInstance: AxiosInstance; + metadata: OAuth2AuthorizationServerMetadata; + }) => Promise, +): Promise { + const client = CoderApi.create(url, token, logger); + try { + const axiosInstance = client.getAxiosInstance(); + const metadata = await new OAuthMetadataClient( + axiosInstance, + logger, + ).getMetadata(); + return await fn({ axiosInstance, metadata }); + } finally { + client.dispose(); + } +} + /** * Check if an array includes all required types. */ diff --git a/src/oauth/revocation.ts b/src/oauth/revocation.ts new file mode 100644 index 0000000000..db177bb6d8 --- /dev/null +++ b/src/oauth/revocation.ts @@ -0,0 +1,66 @@ +import { withOAuthMetadata } from "./metadataClient"; +import { toUrlSearchParams } from "./tokens"; + +import type { + OAuth2ClientRegistrationResponse, + 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. */ +export async function revokeOAuthTokens( + auth: SessionAuth, + registration: OAuth2ClientRegistrationResponse | undefined, + logger: Logger, +): Promise { + if (!auth.oauth) { + return; + } + if (!registration) { + logger.warn("Skipped token revocation: no client registration"); + return; + } + // Refresh token first, while the access token still authenticates the call. + const targets = [ + [auth.oauth.refresh_token, "refresh_token"], + [auth.token, "access_token"], + ] as const; + + try { + await withOAuthMetadata( + auth.url, + auth.token, + logger, + async ({ axiosInstance, metadata }) => { + const endpoint = metadata.revocation_endpoint; + if (!endpoint) { + logger.debug("No revocation endpoint; skipping revocation"); + return; + } + for (const [token, token_type_hint] of targets) { + if (!token) { + continue; + } + const params: OAuth2TokenRevocationRequest = { + token, + client_id: registration.client_id, + client_secret: registration.client_secret, + token_type_hint, + }; + try { + await axiosInstance.post(endpoint, toUrlSearchParams(params), { + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + }); + logger.debug(`Revoked ${token_type_hint}`); + } catch (error) { + logger.warn(`Failed to revoke ${token_type_hint}:`, error); + } + } + }, + ); + } catch (error) { + logger.warn("Token revocation failed:", error); + } +} diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 843d4b3658..b26095b8c4 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -6,17 +6,14 @@ import { import { DEFAULT_OAUTH_SCOPES, REFRESH_GRANT_TYPE } from "./constants"; import { OAuthError, parseOAuthError } from "./errors"; -import { OAuthMetadataClient } from "./metadataClient"; +import { withOAuthMetadata } from "./metadataClient"; +import { revokeOAuthTokens } from "./revocation"; import { buildOAuthTokenData, toUrlSearchParams } from "./tokens"; import { OAuth2TokenResponseSchema, parseOAuthResponse } from "./validation"; -import type { AxiosInstance } from "axios"; import type { - OAuth2AuthorizationServerMetadata, - OAuth2ClientRegistrationResponse, OAuth2TokenRequest, OAuth2TokenResponse, - OAuth2TokenRevocationRequest, } from "coder/site/src/api/typesGenerated"; import type * as vscode from "vscode"; @@ -294,42 +291,6 @@ export class OAuthSessionManager implements vscode.Disposable { return true; } - /** - * Run `fn` with a per-call CoderApi configured for the current - * deployment. The client is disposed on exit so its config-change - * subscriptions never outlive the operation. - */ - private async withOAuthOperation( - token: string | undefined, - fn: (ctx: { - axiosInstance: AxiosInstance; - metadata: OAuth2AuthorizationServerMetadata; - registration: OAuth2ClientRegistrationResponse; - }) => Promise, - ): Promise { - const deployment = this.requireDeployment(); - const client = CoderApi.create(deployment.url, token, this.logger); - try { - const axiosInstance = client.getAxiosInstance(); - const metadataClient = new OAuthMetadataClient( - axiosInstance, - this.logger, - ); - const metadata = await metadataClient.getMetadata(); - - const registration = await this.secretsManager.getOAuthClientRegistration( - deployment.safeHostname, - ); - if (!registration) { - throw new Error("No client registration found"); - } - - return await fn({ axiosInstance, metadata, registration }); - } finally { - client.dispose(); - } - } - public async setDeployment(deployment: Deployment): Promise { if ( deployment.safeHostname === this.deployment?.safeHostname && @@ -400,11 +361,18 @@ export class OAuthSessionManager implements vscode.Disposable { } const refreshToken = storedTokens.refresh_token; - const accessToken = storedTokens.access_token; + const registration = await this.secretsManager.getOAuthClientRegistration( + deployment.safeHostname, + ); + if (!registration) { + throw new Error("No client registration found"); + } - return await this.withOAuthOperation( - accessToken, - async ({ axiosInstance, metadata, registration }) => { + return await withOAuthMetadata( + deployment.url, + storedTokens.access_token, + this.logger, + async ({ axiosInstance, metadata }) => { this.logger.debug("Refreshing access token"); const params: OAuth2TokenRequest = { @@ -484,64 +452,32 @@ export class OAuthSessionManager implements vscode.Disposable { /** Best-effort server-side revocation of the stored refresh and access tokens; never throws. */ public async revokeTokens(): Promise { - // Includes tokens lacking a required scope, which still work on the server. - const storedTokens = await this.getStoredTokens().catch((error) => { - this.logger.warn("Failed to read stored tokens for revocation:", error); - return undefined; - }); - if (!storedTokens) { + const deployment = this.deployment; + if (!deployment) { return; } - - // Refresh token first, while the access token still authenticates the call. - const targets: Array<[string, "access_token" | "refresh_token"]> = []; - if (storedTokens.refresh_token) { - targets.push([storedTokens.refresh_token, "refresh_token"]); - } - targets.push([storedTokens.access_token, "access_token"]); - - try { - await this.withOAuthOperation( - storedTokens.access_token, - async ({ axiosInstance, metadata, registration }) => { - const endpoint = metadata.revocation_endpoint; - if (!endpoint) { - this.logger.debug("No revocation endpoint; skipping revocation"); - return; - } - for (const [token, token_type_hint] of targets) { - const params: OAuth2TokenRevocationRequest = { - token, - client_id: registration.client_id, - client_secret: registration.client_secret, - token_type_hint, - }; - try { - await axiosInstance.post(endpoint, toUrlSearchParams(params), { - headers: { - "Content-Type": "application/x-www-form-urlencoded", - }, - }); - this.logger.debug(`Revoked ${token_type_hint}`); - } catch (error) { - this.logger.warn(`Failed to revoke ${token_type_hint}:`, error); - } - } - }, - ); - } catch (error) { - this.logger.warn("Token revocation failed:", error); + // Includes tokens lacking a required scope, which still work on the server. + const auth = await this.secretsManager.getSessionAuth( + deployment.safeHostname, + ); + if (auth?.url !== deployment.url) { + return; } + const registration = await this.secretsManager.getOAuthClientRegistration( + deployment.safeHostname, + ); + await revokeOAuthTokens(auth, registration, this.logger); } /** - * Returns true if OAuth tokens exist for the current deployment. - * Always reads fresh from secrets to ensure cross-window synchronization. + * Returns true if the current deployment has OAuth tokens that may be + * refreshed, i.e. they carry every required scope. Always reads fresh from + * secrets to ensure cross-window synchronization. * * @param hostname Optional hostname to validate against current deployment. * If provided and doesn't match, returns false (race-safety). */ - public async isLoggedInWithOAuth(hostname?: string): Promise { + public async canRefreshOAuthSession(hostname?: string): Promise { if (hostname && hostname !== this.deployment?.safeHostname) { return false; } diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 68f4510d51..0ef4cc7aa5 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -59,6 +59,9 @@ export function getGlobalFlags( const identity = (s: string) => s; +/** Overrides `CODER_SESSION_TOKEN` in any case variant, so the CLI reads its stored session. */ +export const EMPTY_TOKEN_FLAG = "--token="; + function buildGlobalFlags( configs: Pick, auth: CliAuth, @@ -68,10 +71,12 @@ function buildGlobalFlags( // Escape after stripping so expansion whitespace stays in one shell token. // User flags keep shell expansion (e.g. `$VAR`); the auth values and // header command are quoted literally. - const flags = stripManagedFlags( + const userFlags = stripManagedFlags( getExpandedUserGlobalFlags(configs), auth.store === "extension", ).map(escUserFlag); + // The last `--token` wins, so a user's own token still applies. + const flags = [EMPTY_TOKEN_FLAG, ...userFlags]; if (auth.store === "extension") { flags.push("--global-config", escValue(auth.configDir)); } diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 3878204764..1ad36bbc48 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -1089,7 +1089,7 @@ export class MockOAuthSessionManager { .fn() .mockResolvedValue({ access_token: "test-token" }); readonly revokeTokens = vi.fn().mockResolvedValue(undefined); - readonly isLoggedInWithOAuth = vi.fn().mockResolvedValue(false); + readonly canRefreshOAuthSession = vi.fn().mockResolvedValue(false); readonly clearOAuthState = vi.fn().mockResolvedValue(undefined); readonly dispose = vi.fn(); } diff --git a/test/unit/api/authInterceptor.test.ts b/test/unit/api/authInterceptor.test.ts index 94888450f9..4259e6561a 100644 --- a/test/unit/api/authInterceptor.test.ts +++ b/test/unit/api/authInterceptor.test.ts @@ -116,7 +116,7 @@ function createTestContext() { const mockOAuthManager = new MockOAuthSessionManager(); // Default: not logged in with OAuth - mockOAuthManager.isLoggedInWithOAuth.mockResolvedValue(false); + mockOAuthManager.canRefreshOAuthSession.mockResolvedValue(false); /** Sets up OAuth tokens in storage and configures mock */ const setupOAuthTokens = async () => { @@ -129,7 +129,7 @@ function createTestContext() { scope: "workspace:read", }, }); - mockOAuthManager.isLoggedInWithOAuth.mockImplementation( + mockOAuthManager.canRefreshOAuthSession.mockImplementation( async (hostname?: string) => { if (hostname && hostname !== TEST_HOSTNAME) { return false; @@ -556,8 +556,8 @@ describe("AuthInterceptor", () => { await setupOAuthTokens(); - // Make isLoggedInWithOAuth return false for different hostname - mockOAuthManager.isLoggedInWithOAuth.mockImplementation( + // Make canRefreshOAuthSession return false for different hostname + mockOAuthManager.canRefreshOAuthSession.mockImplementation( (hostname?: string) => { // Simulate hostname mismatch (deployment changed) if (hostname === TEST_HOSTNAME) { diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index 5801ccd225..006c67c435 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -3,6 +3,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; import { LazyStream, startWorkspace, updateWorkspace } from "@/api/workspace"; +import { EMPTY_TOKEN_FLAG } from "@/settings/cli"; import { workspace as createWorkspace } from "@repo/mocks"; @@ -21,6 +22,8 @@ vi.mock(import("node:child_process"), async (importOriginal) => ({ })); const { spawn } = await import("node:child_process"); +const GLOBAL_FLAGS = [EMPTY_TOKEN_FLAG, "--url", "https://test.coder.com"]; + const CLI_FEATURES: CliFeatureSet = { cliLogin: true, proxyLogDirectory: true, @@ -226,8 +229,7 @@ describe("updateWorkspace", () => { await expect(result).resolves.toBe(finalWorkspace); expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + ...GLOBAL_FLAGS, "update", "--", "testuser/test-workspace", @@ -248,8 +250,7 @@ describe("updateWorkspace", () => { await result; expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + ...GLOBAL_FLAGS, "update", "--parameter", "region=us-east", @@ -425,8 +426,7 @@ describe("startWorkspace", () => { await expect(result).resolves.toBe(finalWorkspace); expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + ...GLOBAL_FLAGS, "start", "--yes", "--reason", @@ -456,8 +456,7 @@ describe("startWorkspace", () => { await result; expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + ...GLOBAL_FLAGS, "start", "--yes", "--", diff --git a/test/unit/cli/cliCredentialManager.test.ts b/test/unit/cli/cliCredentialManager.test.ts index 10b6ba4f03..1e9367e86b 100644 --- a/test/unit/cli/cliCredentialManager.test.ts +++ b/test/unit/cli/cliCredentialManager.test.ts @@ -9,6 +9,7 @@ import { type BinaryResolver, } from "@/cli/cliCredentialManager"; import * as cliExec from "@/cli/cliExec"; +import { EMPTY_TOKEN_FLAG } from "@/settings/cli"; import { PathResolver } from "@/storage/pathResolver"; import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; @@ -41,6 +42,7 @@ const PATH_RESOLVER = new PathResolver("/mock/base", "/mock/log"); const CRED_DIR = path.join("/mock/base", "dev.coder.com"); const USER_DIR = "/custom/coderv2"; +// Flags for `login --use-token-as-session`; every other command gets EMPTY_TOKEN_FLAG first. const EXTENSION_FLAGS = [ "--global-config", CRED_DIR, @@ -204,12 +206,15 @@ describe("CliCredentialManager", () => { describe("storeToken", () => { it("passes the token through the environment only", async () => { stubExecFile(); + vi.stubEnv("coder_session_token", "host-token"); const { manager } = setup(); await manager.storeToken(TEST_URL, "my-secret-token", configs); expect(execOptions().env?.CODER_SESSION_TOKEN).toBe("my-secret-token"); + expect(execOptions().env).not.toHaveProperty("coder_session_token"); expect(execCalls()[0]).not.toContain("my-secret-token"); + expect(execCalls()[0]).not.toContain(EMPTY_TOKEN_FLAG); }); interface CliErrorCase { @@ -254,7 +259,9 @@ describe("CliCredentialManager", () => { const { manager } = setup(); expect(await manager.readToken(TEST_URL, configs)).toBe("my-token"); - expect(execCalls()).toEqual([[...KEYRING_FLAGS, "login", "token"]]); + expect(execCalls()).toEqual([ + [EMPTY_TOKEN_FLAG, ...KEYRING_FLAGS, "login", "token"], + ]); }); interface ReadTokenCase { @@ -408,7 +415,7 @@ describe("CliCredentialManager", () => { expect(result).toBe(true); expect(execCalls()).toEqual( - logout ? [[...logout, "logout", "--yes"]] : [], + logout ? [[EMPTY_TOKEN_FLAG, ...logout, "logout", "--yes"]] : [], ); expect(credentialFilesExist()).toBe(false); expect(sink.expectOne("auth.credential.clear").properties).toMatchObject({ diff --git a/test/unit/cli/cliExec.test.ts b/test/unit/cli/cliExec.test.ts index 62100bcb79..7696514c9f 100644 --- a/test/unit/cli/cliExec.test.ts +++ b/test/unit/cli/cliExec.test.ts @@ -15,6 +15,7 @@ import { import * as vscode from "vscode"; import { cliFeatureSet } from "@/featureSet"; +import { EMPTY_TOKEN_FLAG } from "@/settings/cli"; import { MockConfigurationProvider, @@ -201,6 +202,7 @@ describe("cliExec", () => { .trim() .split("\n"); expect(args).toEqual([ + EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "--header-command", @@ -246,6 +248,7 @@ describe("cliExec", () => { configs.set("coder.headerCommand", "my-header-cmd"); const args = (await cliExec.netcheck(env)).trim().split("\n"); expect(args).toEqual([ + EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "--header-command", @@ -302,6 +305,7 @@ describe("cliExec", () => { }); const args = (await fs.readFile(outputPath, "utf-8")).trim().split("\n"); expect(args).toEqual([ + EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "--header-command", @@ -333,6 +337,7 @@ describe("cliExec", () => { await cliExec.supportBundle(env, "owner/workspace", { outputPath }); const args = (await fs.readFile(outputPath, "utf-8")).trim().split("\n"); expect(args).toEqual([ + EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "support", @@ -401,7 +406,14 @@ describe("cliExec", () => { expect(spawn).toHaveBeenCalledWith( env.binary, - ["--url", "https://test.coder.com", "ping", "--", "owner/my workspace"], + [ + EMPTY_TOKEN_FLAG, + "--url", + "https://test.coder.com", + "ping", + "--", + "owner/my workspace", + ], expect.objectContaining({ detached: process.platform !== "win32" }), ); }); @@ -417,6 +429,7 @@ describe("cliExec", () => { expect(spawn).toHaveBeenCalledWith( env.binary, [ + EMPTY_TOKEN_FLAG, "--verbose", "--global-config", "/cfg", @@ -477,6 +490,7 @@ describe("cliExec", () => { name: "AI Agent", shellPath: env.binary, shellArgs: [ + EMPTY_TOKEN_FLAG, "--url", "https://test.coder.com", "ssh", diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index ab413003ce..9645259b28 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -6,9 +6,14 @@ import { HttpStatusCode } from "@/api/httpStatusCode"; import { getHeaders } from "@/headers"; import { AuthTelemetry } from "@/instrumentation/auth"; import { LoginCoordinator, type LoginMethod } from "@/login/loginCoordinator"; +import { + ClientRegistrationDisabledError, + OAuthAuthorizer, +} from "@/oauth/authorizer"; import { OAuthCallback } from "@/oauth/oauthCallback"; +import { revokeOAuthTokens } from "@/oauth/revocation"; import { MementoManager } from "@/storage/mementoManager"; -import { SecretsManager } from "@/storage/secretsManager"; +import { SecretsManager, type SessionAuth } from "@/storage/secretsManager"; import { maybeAskAuthMethod, maybeAskUrl } from "@/ui/prompts"; import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; @@ -23,6 +28,10 @@ import { MockProgressReporter, MockUserInteraction, } from "../../mocks/testHelpers"; +import { + createMockClientRegistration, + createMockTokenResponse, +} from "../oauth/testUtils"; import type { User } from "coder/site/src/api/typesGenerated"; @@ -75,6 +84,8 @@ vi.mock("@/ui/prompts", () => ({ maybeAskUrl: vi.fn(), })); +vi.mock("@/oauth/revocation", () => ({ revokeOAuthTokens: vi.fn() })); + // Mock CoderApi to control getAuthenticatedUser behavior const mockGetAuthenticatedUser = vi.hoisted(() => vi.fn()); vi.mock("@/api/coderApi", async (importOriginal) => { @@ -853,6 +864,166 @@ describe("LoginCoordinator", () => { }); }); + describe("replacing an OAuth session", () => { + const registration = createMockClientRegistration(); + const oldSession: SessionAuth = { + url: TEST_URL, + token: "old-token", + username: createMockUser().username, + oauth: { + refresh_token: "old-refresh-token", + scope: "coder:all", + expiry_timestamp: Date.now() + 60 * 60 * 1000, + }, + }; + + async function setup() { + const ctx = createTestContext(); + const user = ctx.mockSuccessfulAuth(); + await ctx.secretsManager.setSessionAuth(TEST_HOSTNAME, oldSession); + await ctx.secretsManager.setOAuthClientRegistration( + TEST_HOSTNAME, + registration, + ); + return { + ...ctx, + user, + login: (token?: string) => + ctx.coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + token, + }), + stored: () => ctx.secretsManager.getSessionAuth(TEST_HOSTNAME), + }; + } + + it.each([undefined, "old-token"])( + "keeps the OAuth data of a reused session (provided token: %s)", + async (token) => { + const t = await setup(); + + expect(await t.login(token)).toMatchObject({ success: true }); + expect(await t.stored()).toEqual(oldSession); + expect(revokeOAuthTokens).not.toHaveBeenCalled(); + }, + ); + + it.each<{ + name: string; + token: string | undefined; + method: "legacy" | "oauth"; + }>([ + { name: "a manual token", token: undefined, method: "legacy" }, + { name: "a provided token", token: "new-token", method: "legacy" }, + { name: "an OAuth login", token: undefined, method: "oauth" }, + ])( + "revokes the session replaced by $name, after saving it", + async ({ token, method }) => { + const t = await setup(); + const rotated = { ...oldSession, token: "rotated-token" }; + // While the first token is checked, a refresh rotates the session + // and a different client registers; the stored token has expired. + t.mockGetAuthenticatedUser.mockImplementationOnce(async () => { + await t.secretsManager.setSessionAuth(TEST_HOSTNAME, rotated); + await t.secretsManager.setOAuthClientRegistration( + TEST_HOSTNAME, + createMockClientRegistration({ client_id: "other-client" }), + ); + if (!token) { + throw createAxiosError(HttpStatusCode.UNAUTHORIZED, "Expired"); + } + return t.user; + }); + vi.mocked(maybeAskAuthMethod).mockResolvedValue(method); + t.userInteraction.setInputBoxValue("new-token"); + vi.spyOn(OAuthAuthorizer.prototype, "login").mockResolvedValue({ + tokenResponse: createMockTokenResponse({ access_token: "new-token" }), + user: t.user, + }); + let storedAtRevoke: Promise | undefined; + vi.mocked(revokeOAuthTokens).mockImplementation(() => { + storedAtRevoke = t.stored(); + return Promise.resolve(); + }); + + expect(await t.login(token)).toMatchObject({ token: "new-token" }); + expect((await storedAtRevoke)?.token).toBe("new-token"); + expect(revokeOAuthTokens).toHaveBeenCalledExactlyOnceWith( + rotated, + registration, + t.logger, + ); + }, + ); + + it("keeps a session a refresh rotates while its stored token is checked", async () => { + const t = await setup(); + const rotated = { ...oldSession, token: "rotated-token" }; + t.mockGetAuthenticatedUser.mockImplementationOnce(async () => { + await t.secretsManager.setSessionAuth(TEST_HOSTNAME, rotated); + return t.user; + }); + + expect(await t.login()).toMatchObject({ method: "stored_token" }); + expect(await t.stored()).toEqual(rotated); + expect(revokeOAuthTokens).not.toHaveBeenCalled(); + }); + + it.each([ + { name: "the provided token is invalid", token: "bad-token" }, + { name: "the manual token prompt is cancelled", token: undefined }, + ])("keeps the old session when $name", async ({ token }) => { + const t = await setup(); + t.mockAuthFailure(); + + expect(await t.login(token)).toMatchObject({ success: false }); + expect(await t.stored()).toEqual(oldSession); + expect(revokeOAuthTokens).not.toHaveBeenCalled(); + }); + }); + + describe("when client registration is disabled", () => { + function setup() { + const ctx = createTestContext(); + ctx.mockSuccessfulAuth(); + vi.mocked(maybeAskAuthMethod).mockResolvedValue("oauth"); + vi.spyOn(OAuthAuthorizer.prototype, "login").mockRejectedValue( + new ClientRegistrationDisabledError(), + ); + ctx.userInteraction.setInputBoxValue("session-token"); + return { + ...ctx, + login: () => + ctx.coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }), + }; + } + + it("signs in with a session token when the user chooses it", async () => { + const t = setup(); + t.userInteraction.setResponse( + "OAuth sign-in is unavailable", + "Sign In with Token", + ); + + expect(await t.login()).toMatchObject({ + method: "cli_token", + token: "session-token", + }); + }); + + it("cancels sign-in when the user dismisses the prompt", async () => { + const t = setup(); + t.userInteraction.setResponse("OAuth sign-in is unavailable", undefined); + + expect(await t.login()).toMatchObject({ reason: "user_dismissed" }); + expect(vscode.window.showInputBox).not.toHaveBeenCalled(); + }); + }); + describe("telemetry", () => { const dialogOptions = (trigger: "auth_required" | "missing_session") => ({ url: TEST_URL, diff --git a/test/unit/oauth/authorizer.test.ts b/test/unit/oauth/authorizer.test.ts index 2780ed6a68..3e464d4a4c 100644 --- a/test/unit/oauth/authorizer.test.ts +++ b/test/unit/oauth/authorizer.test.ts @@ -3,13 +3,17 @@ import * as vscode from "vscode"; import { InvalidApiResponseError } from "@/api/responseValidation"; import { getHeaders } from "@/headers"; -import { OAuthAuthorizer } from "@/oauth/authorizer"; +import { + ClientRegistrationDisabledError, + OAuthAuthorizer, +} from "@/oauth/authorizer"; import { DEFAULT_OAUTH_SCOPES, IF_SUPPORTED_OAUTH_SCOPES, } from "@/oauth/constants"; import { + createAxiosError, createMockUser, MockCancellationToken, MockProgress, @@ -525,6 +529,21 @@ describe("OAuthAuthorizer", () => { ).rejects.toThrow("Server does not support dynamic client registration"); }); + it("throws ClientRegistrationDisabledError when the server refuses registration", async () => { + const { setupOAuthRoutes, authorizer } = createTestContext(); + setupOAuthRoutes(undefined, { + "/oauth2/register": createAxiosError(403, "Forbidden"), + }); + + await expect( + authorizer.login( + createTestDeployment(), + new MockProgress(), + new MockCancellationToken(), + ), + ).rejects.toThrow(ClientRegistrationDisabledError); + }); + it("rejects a token response without an access token", async () => { const { setupOAuthRoutes, startLogin, completeLogin } = createTestContext(); diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index 71d38f1d50..58e1d8a3c7 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -136,14 +136,14 @@ function createTestContext(deployment: Deployment = createTestDeployment()) { } describe("OAuthSessionManager", () => { - describe("isLoggedInWithOAuth", () => { - interface IsLoggedInTestCase { + describe("canRefreshOAuthSession", () => { + interface CanRefreshTestCase { name: string; auth: SessionAuth | null; expected: boolean; } - it.each([ + it.each([ { name: "returns true when OAuth tokens exist", auth: { @@ -177,7 +177,7 @@ describe("OAuthSessionManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, auth); } - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(expected); }); }); @@ -264,7 +264,7 @@ describe("OAuthSessionManager", () => { }, }); - expect(await manager.isLoggedInWithOAuth()).toBe(false); + expect(await manager.canRefreshOAuthSession()).toBe(false); }); }); @@ -279,7 +279,7 @@ describe("OAuthSessionManager", () => { await manager.setDeployment(newDeployment); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); }); @@ -290,7 +290,7 @@ describe("OAuthSessionManager", () => { manager.clearDeployment(); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); }); @@ -518,7 +518,7 @@ describe("OAuthSessionManager", () => { }); describe("scope validation", () => { - it("rejects tokens with insufficient scopes", async () => { + it("cannot refresh tokens with insufficient scopes", async () => { const { secretsManager, manager } = createTestContext(); await secretsManager.setSessionAuth(TEST_HOSTNAME, { @@ -531,7 +531,7 @@ describe("OAuthSessionManager", () => { }, }); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); @@ -550,7 +550,7 @@ describe("OAuthSessionManager", () => { }, }); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(true); }); @@ -558,7 +558,7 @@ describe("OAuthSessionManager", () => { const { manager, setupOAuthSession } = createTestContext(); await setupOAuthSession({ scope: "coder:all" }); - expect(await manager.isLoggedInWithOAuth()).toBe(true); + expect(await manager.canRefreshOAuthSession()).toBe(true); }); }); diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts index 1fd3389e78..fcde9182a6 100644 --- a/test/unit/remote/remote.test.ts +++ b/test/unit/remote/remote.test.ts @@ -395,7 +395,7 @@ describe("Remote", () => { }); expect(elideNetworkInfoDir(proxyCommand)).toBe( - `/mock/coder --global-config ${quoteCommand("/mock/global")} --url ${quoteCommand("https://coder.example.com")} ${expected}`, + `/mock/coder --token= --global-config ${quoteCommand("/mock/global")} --url ${quoteCommand("https://coder.example.com")} ${expected}`, ); }); }); diff --git a/test/unit/settings/cli.test.ts b/test/unit/settings/cli.test.ts index d8e315afd9..a286a16068 100644 --- a/test/unit/settings/cli.test.ts +++ b/test/unit/settings/cli.test.ts @@ -5,6 +5,7 @@ import { afterEach, beforeEach, it, expect, describe, vi } from "vitest"; import { cliFeatureSet } from "@/featureSet"; import { type CliAuth, + EMPTY_TOKEN_FLAG, getExpandedUserGlobalFlags, getGlobalFlags, getGlobalShellFlags, @@ -78,7 +79,10 @@ describe("cliConfig", () => { }, ])("emits auth flags for a $scenario", ({ auth, expected }) => { const config = new MockConfigurationProvider(); - expect(getGlobalShellFlags(config, auth)).toStrictEqual(expected); + expect(getGlobalShellFlags(config, auth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, + ...expected, + ]); }); it.each(INVALID_DEPLOYMENT_URLS)( @@ -97,6 +101,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose", "--global-configs"]); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, "--verbose", "--global-configs", // similar prefixes are not managed flags ...EXTENSION_SHELL_FLAGS, @@ -108,6 +113,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose", "--use-keyring=false"]); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, "--verbose", ...EXTENSION_SHELL_FLAGS, ]); @@ -128,6 +134,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", flags); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, ...flags, ...CLI_SHELL_FLAGS, ]); @@ -147,6 +154,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", flags); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, "-v", ...EXTENSION_SHELL_FLAGS, ]); @@ -160,6 +168,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["-v", "--header-command custom"]); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, "-v", '"--header-command custom"', // ignored by CLI ...CLI_SHELL_FLAGS, @@ -195,6 +204,7 @@ describe("cliConfig", () => { configDir: "/home/$USER/%APPDATA%/coder", }), ).toStrictEqual([ + EMPTY_TOKEN_FLAG, '"--header=Authorization=$TOKEN"', "--global-config", configDir, @@ -213,6 +223,7 @@ describe("cliConfig", () => { // Without per-entry escaping the space splits the shell command. expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, '"--cfg=C:\\Users\\John Doe/coder"', ...EXTENSION_SHELL_FLAGS, ]); @@ -226,12 +237,24 @@ describe("cliConfig", () => { config.set("coder.headerCommand", "echo test"); expect(getGlobalFlags(config, extensionStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, "--verbose", ...EXTENSION_FLAGS, "--header-command", "echo test", ]); }); + + it("places a user --token after the empty token flag so it wins", () => { + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", ["--token=abc"]); + + expect(getGlobalFlags(config, cliStoreAuth)).toStrictEqual([ + EMPTY_TOKEN_FLAG, + "--token=abc", + ...CLI_FLAGS, + ]); + }); }); describe("getExpandedUserGlobalFlags", () => { @@ -491,9 +514,10 @@ describe("cliConfig", () => { } config.set("coder.globalFlags", userFlags); - expect(getGlobalFlags(config, resolve(config, version))).toStrictEqual( - expected, - ); + expect(getGlobalFlags(config, resolve(config, version))).toStrictEqual([ + EMPTY_TOKEN_FLAG, + ...expected, + ]); }); }); }); From a507d0a040116dd4509783c7c9b736865d259983 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 12 Oct 2026 17:46:42 +0300 Subject: [PATCH 2/4] fix(oauth): keep credentials out of logs and revoke as the issuing client - Log Axios errors as their request line and the server's reply. Their config and socket hold the session token, OAuth tokens, and client secret, which the buffered connection logs replayed into the output channel. - Record the client that issued each OAuth session, and refresh and revoke as that client, so another window re-registering the client no longer breaks either. Sessions without a recorded client use the stored registration. - Explain why the extension requests `user:update_personal`. - In the changelog, note that OAuth becomes the default sign-in method and link the OAuth2 provider docs. - Shorten comments. --- CHANGELOG.md | 5 ++ src/cli/cliCredentialManager.ts | 4 +- src/logging/formatters.ts | 18 ++++- src/login/loginCoordinator.ts | 34 ++++----- src/oauth/authorizer.ts | 19 ++--- src/oauth/constants.ts | 2 + src/oauth/metadataClient.ts | 6 +- src/oauth/revocation.ts | 21 +++--- src/oauth/sessionManager.ts | 21 +++--- src/oauth/tokens.ts | 18 ++++- src/settings/cli.ts | 4 +- src/storage/secretsManager.ts | 4 ++ test/unit/logging/formatters.test.ts | 33 +++++++++ test/unit/login/loginCoordinator.test.ts | 3 + test/unit/oauth/sessionManager.test.ts | 90 ++++++++++++++++++------ test/unit/oauth/tokens.test.ts | 26 ++++++- 16 files changed, 228 insertions(+), 80 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f8b0a6886..924d36723f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,11 @@ 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). +- 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 `CODER_SESSION_TOKEN` from the environment, so they no longer fail or run as a different user inside a Coder workspace. diff --git a/src/cli/cliCredentialManager.ts b/src/cli/cliCredentialManager.ts index 040e688c6f..989ea2692a 100644 --- a/src/cli/cliCredentialManager.ts +++ b/src/cli/cliCredentialManager.ts @@ -76,7 +76,7 @@ export class CliCredentialManager { return; } span.setProperty("store", cli.auth.store); - // `login` reads the token from the environment, which the empty flag would hide. + // `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 }, @@ -276,7 +276,7 @@ export class CliCredentialManager { } } -/** `process.env` with `token` as the only session token, since the CLI reads the name in any case. */ +/** `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( diff --git a/src/logging/formatters.ts b/src/logging/formatters.ts index d3389b7358..4181488276 100644 --- a/src/logging/formatters.ts +++ b/src/logging/formatters.ts @@ -1,11 +1,10 @@ +import { type AxiosError, type AxiosRequestConfig, isAxiosError } from "axios"; import { Buffer } from "node:buffer"; import util from "node:util"; import prettyBytes from "pretty-bytes"; import { lowercase } from "../common/strings"; -import type { AxiosRequestConfig } from "axios"; - const SENSITIVE_HEADERS: ReadonlySet> = new Set([ "authorization", "coder-session-token", @@ -200,9 +199,22 @@ export function sizeOf(data: unknown): number | undefined { return undefined; } +/** Drops the error's config and socket, which hold the request's credentials. */ +function summarizeAxiosError(error: AxiosError) { + return { + name: error.name, + message: error.message, + code: error.code, + method: formatMethod(error.config?.method), + url: formatUri(error.config), + status: error.response?.status, + data: redactBodyFields(error.response?.data), + }; +} + export function safeStringify(data: unknown): string | null { try { - return util.inspect(data, { + return util.inspect(isAxiosError(data) ? summarizeAxiosError(data) : data, { showHidden: false, // Bounded so a single log line cannot balloon in size. depth: 8, diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 929e49ac4d..877b8a95ba 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -204,7 +204,7 @@ export class LoginCoordinator implements vscode.Disposable { const { safeHostname, url } = deployment; const previous = await this.secretsManager.getSessionAuth(safeHostname); const replaced = previous?.token === result.token ? undefined : previous; - // A refresh rotated the stored session while this login checked it. + // A refresh replaced the stored session during this login; keep it. if (result.method === "stored_token" && replaced) { return; } @@ -299,7 +299,8 @@ export class LoginCoordinator implements vscode.Disposable { providedToken?: string, tokenSignInConfirmed = false, ): Promise { - // Read before an OAuth login can re-register the client. + // Revokes sessions that don't record their client. Read it before login can + // re-register the client. const registration = await this.secretsManager.getOAuthClientRegistration( deployment.safeHostname, ); @@ -669,25 +670,26 @@ export class LoginCoordinator implements vscode.Disposable { try { this.logger.debug("Starting OAuth authentication"); - const { tokenResponse, user } = await vscode.window.withProgress( - { - location: vscode.ProgressLocation.Notification, - title: "Authenticating", - cancellable: true, - }, - async (progress, cancellationToken) => - await this.oauthAuthorizer.login( - deployment, - progress, - cancellationToken, - ), - ); + const { tokenResponse, user, registration } = + await vscode.window.withProgress( + { + location: vscode.ProgressLocation.Notification, + title: "Authenticating", + cancellable: true, + }, + async (progress, cancellationToken) => + await this.oauthAuthorizer.login( + deployment, + progress, + cancellationToken, + ), + ); return { success: true, token: tokenResponse.access_token, user, - oauth: buildOAuthTokenData(tokenResponse), + oauth: buildOAuthTokenData(tokenResponse, registration), }; } catch (error) { if (error instanceof ClientRegistrationDisabledError) { diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index b51ed572f3..ec97220b70 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -38,6 +38,13 @@ import type { SecretsManager } from "../storage/secretsManager"; import type { OAuthCallback } from "./oauthCallback"; +interface OAuthLoginResult { + tokenResponse: OAuth2TokenResponse; + user: User; + /** Client 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() { @@ -60,16 +67,12 @@ export class OAuthAuthorizer implements vscode.Disposable { private readonly extensionId: string, ) {} - /** - * Perform complete OAuth login flow. - * Creates CoderApi internally from deployment. - * Returns the token response and user - does not persist tokens. - */ + /** Runs the OAuth login flow. Saves the client registration, not the tokens. */ public async login( deployment: Deployment, progress: vscode.Progress<{ message?: string; increment?: number }>, cancellationToken: vscode.CancellationToken, - ): Promise<{ tokenResponse: OAuth2TokenResponse; user: User }> { + ): Promise { const client = CoderApi.create(deployment.url, undefined, this.logger); try { return await this.runLoginFlow( @@ -88,7 +91,7 @@ export class OAuthAuthorizer implements vscode.Disposable { deployment: Deployment, progress: vscode.Progress<{ message?: string; increment?: number }>, cancellationToken: vscode.CancellationToken, - ): Promise<{ tokenResponse: OAuth2TokenResponse; user: User }> { + ): Promise { const reportProgress = (message?: string, increment?: number): void => { if (cancellationToken.isCancellationRequested) { throw new Error("OAuth login cancelled by user"); @@ -133,7 +136,7 @@ export class OAuthAuthorizer implements vscode.Disposable { this.logger.debug("OAuth login flow completed successfully"); - return { tokenResponse, user }; + return { tokenResponse, user, registration }; } /** diff --git a/src/oauth/constants.ts b/src/oauth/constants.ts index dee54fe768..7d48893f78 100644 --- a/src/oauth/constants.ts +++ b/src/oauth/constants.ts @@ -12,6 +12,8 @@ export const DEFAULT_OAUTH_SCOPES = [ "workspace:create", "user:read", "user:read_personal", + // Lets `coder start` refresh expired external-auth links. No narrower scope + // exists, so it also allows profile and Git SSH key changes. "user:update_personal", ].join(" "); diff --git a/src/oauth/metadataClient.ts b/src/oauth/metadataClient.ts index bb2d5fb31a..af33bc23db 100644 --- a/src/oauth/metadataClient.ts +++ b/src/oauth/metadataClient.ts @@ -143,11 +143,7 @@ export class OAuthMetadataClient { } } -/** - * Run `fn` with a short-lived client for `url` and the server's OAuth - * metadata. The client is disposed on exit so its config-change - * subscriptions never outlive the operation. - */ +/** Runs `fn` with the server's OAuth metadata and a client disposed afterward. */ export async function withOAuthMetadata( url: string, token: string | undefined, diff --git a/src/oauth/revocation.ts b/src/oauth/revocation.ts index db177bb6d8..ffa9c36acf 100644 --- a/src/oauth/revocation.ts +++ b/src/oauth/revocation.ts @@ -1,24 +1,25 @@ import { withOAuthMetadata } from "./metadataClient"; -import { toUrlSearchParams } from "./tokens"; +import { type OAuthClient, toUrlSearchParams } from "./tokens"; -import type { - OAuth2ClientRegistrationResponse, - OAuth2TokenRevocationRequest, -} from "coder/site/src/api/typesGenerated"; +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. */ +/** + * Best-effort revocation of the token pair in `auth`; never throws. Signs in as + * the client that issued the tokens, else `fallbackClient`. + */ export async function revokeOAuthTokens( auth: SessionAuth, - registration: OAuth2ClientRegistrationResponse | undefined, + fallbackClient: OAuthClient | undefined, logger: Logger, ): Promise { if (!auth.oauth) { return; } - if (!registration) { + const client = auth.oauth.client ?? fallbackClient; + if (!client) { logger.warn("Skipped token revocation: no client registration"); return; } @@ -45,8 +46,8 @@ export async function revokeOAuthTokens( } const params: OAuth2TokenRevocationRequest = { token, - client_id: registration.client_id, - client_secret: registration.client_secret, + client_id: client.client_id, + client_secret: client.client_secret, token_type_hint, }; try { diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index b26095b8c4..de3bbfbfb8 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -361,10 +361,12 @@ export class OAuthSessionManager implements vscode.Disposable { } const refreshToken = storedTokens.refresh_token; - const registration = await this.secretsManager.getOAuthClientRegistration( - deployment.safeHostname, - ); - if (!registration) { + const client = + storedTokens.client ?? + (await this.secretsManager.getOAuthClientRegistration( + deployment.safeHostname, + )); + if (!client) { throw new Error("No client registration found"); } @@ -378,8 +380,8 @@ export class OAuthSessionManager implements vscode.Disposable { const params: OAuth2TokenRequest = { grant_type: REFRESH_GRANT_TYPE, refresh_token: refreshToken, - client_id: registration.client_id, - client_secret: registration.client_secret, + client_id: client.client_id, + client_secret: client.client_secret, }; const response = await axiosInstance.post( @@ -412,7 +414,7 @@ export class OAuthSessionManager implements vscode.Disposable { deployment, tokenResponse.access_token, ), - oauth: buildOAuthTokenData(tokenResponse), + oauth: buildOAuthTokenData(tokenResponse, client), }); return tokenResponse; @@ -470,9 +472,8 @@ export class OAuthSessionManager implements vscode.Disposable { } /** - * Returns true if the current deployment has OAuth tokens that may be - * refreshed, i.e. they carry every required scope. Always reads fresh from - * secrets to ensure cross-window synchronization. + * Whether the deployment's OAuth tokens carry every required scope, so they + * may refresh. Reads secrets each time to stay in sync across windows. * * @param hostname Optional hostname to validate against current deployment. * If provided and doesn't match, returns false (race-safety). diff --git a/src/oauth/tokens.ts b/src/oauth/tokens.ts index 1f0ade9165..f635125658 100644 --- a/src/oauth/tokens.ts +++ b/src/oauth/tokens.ts @@ -1,4 +1,7 @@ -import type { OAuth2TokenResponse } from "coder/site/src/api/typesGenerated"; +import type { + OAuth2ClientRegistrationResponse, + OAuth2TokenResponse, +} from "coder/site/src/api/typesGenerated"; import type { OAuthTokenData } from "../storage/secretsManager"; @@ -22,12 +25,19 @@ export function toUrlSearchParams(obj: object): URLSearchParams { return new URLSearchParams(params); } +/** Client credentials that refresh and revocation authenticate with. */ +export type OAuthClient = Pick< + OAuth2ClientRegistrationResponse, + "client_id" | "client_secret" +>; + /** - * Build OAuthTokenData from a token response. + * Build OAuthTokenData from a token response issued to `client`. * Prefers the `expiry` timestamp over calculating from `expires_in`. */ export function buildOAuthTokenData( tokenResponse: OAuth2TokenResponse, + client: OAuthClient, ): OAuthTokenData { if (tokenResponse.token_type !== "Bearer") { throw new Error( @@ -40,6 +50,10 @@ export function buildOAuthTokenData( // Servers that ignore scopes return none and grant everything scope: tokenResponse.scope || "coder:all", expiry_timestamp: getExpiryTimestamp(tokenResponse), + client: { + client_id: client.client_id, + client_secret: client.client_secret, + }, }; } diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 0ef4cc7aa5..f17d58e181 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -59,7 +59,7 @@ export function getGlobalFlags( const identity = (s: string) => s; -/** Overrides `CODER_SESSION_TOKEN` in any case variant, so the CLI reads its stored session. */ +/** Hides `CODER_SESSION_TOKEN` (in any case) so the CLI uses its stored session. */ export const EMPTY_TOKEN_FLAG = "--token="; function buildGlobalFlags( @@ -75,7 +75,7 @@ function buildGlobalFlags( getExpandedUserGlobalFlags(configs), auth.store === "extension", ).map(escUserFlag); - // The last `--token` wins, so a user's own token still applies. + // A user's own `--token` comes later, so it wins. const flags = [EMPTY_TOKEN_FLAG, ...userFlags]; if (auth.store === "extension") { flags.push("--global-config", escValue(auth.configDir)); diff --git a/src/storage/secretsManager.ts b/src/storage/secretsManager.ts index 82e6f5e9d3..6587ed1922 100644 --- a/src/storage/secretsManager.ts +++ b/src/storage/secretsManager.ts @@ -39,6 +39,10 @@ const OAuthTokenDataSchema = z.object({ refresh_token: z.string().optional(), scope: z.string(), expiry_timestamp: z.number(), + /** Client that issued the tokens; if absent, the stored registration is used. */ + client: z + .object({ client_id: z.string(), client_secret: z.string().optional() }) + .optional(), }); export type OAuthTokenData = z.infer; diff --git a/test/unit/logging/formatters.test.ts b/test/unit/logging/formatters.test.ts index ded822f45e..4fe97cf25b 100644 --- a/test/unit/logging/formatters.test.ts +++ b/test/unit/logging/formatters.test.ts @@ -1,3 +1,4 @@ +import { AxiosError, AxiosHeaders } from "axios"; import { describe, expect, it } from "vitest"; import { @@ -291,4 +292,36 @@ describe("safeStringify", () => { } expect(safeStringify(nested)).not.toContain("bottom"); }); + + it("keeps request credentials out of Axios errors", () => { + const config = { + method: "post", + url: "https://coder.example.com/oauth2/revoke", + headers: new AxiosHeaders({ "Coder-Session-Token": "session-secret" }), + data: "token=token-secret&client_secret=client-secret", + }; + const error = new AxiosError( + "Bad request", + "ERR_BAD_REQUEST", + config, + {}, + { + status: 400, + statusText: "Bad Request", + headers: {}, + config, + data: { error: "invalid_client", refresh_token: "reply-secret" }, + }, + ); + + const result = safeStringify(error); + + expect(result).toContain("POST"); + expect(result).toContain("https://coder.example.com/oauth2/revoke"); + expect(result).toContain("status: 400"); + expect(result).toContain("invalid_client"); + expect(result).not.toMatch( + /session-secret|token-secret|client-secret|reply-secret/, + ); + }); }); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 9645259b28..bfe90948cb 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -940,6 +940,9 @@ describe("LoginCoordinator", () => { vi.spyOn(OAuthAuthorizer.prototype, "login").mockResolvedValue({ tokenResponse: createMockTokenResponse({ access_token: "new-token" }), user: t.user, + registration: createMockClientRegistration({ + client_id: "new-client", + }), }); let storedAtRevoke: Promise | undefined; vi.mocked(revokeOAuthTokens).mockImplementation(() => { diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index 58e1d8a3c7..3848fc20d4 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -33,6 +33,7 @@ import { } from "./testUtils"; import type { Deployment } from "@/deployment/types"; +import type { OAuthClient } from "@/oauth/tokens"; import type { TelemetryService } from "@/telemetry/service"; vi.mock("axios", async () => { @@ -85,6 +86,7 @@ function createTestContext(deployment: Deployment = createTestDeployment()) { refreshToken?: string; expiryMs?: number; scope?: string; + client?: OAuthClient; } = {}, ) => { await base.secretsManager.setSessionAuth(TEST_HOSTNAME, { @@ -95,6 +97,7 @@ function createTestContext(deployment: Deployment = createTestDeployment()) { refresh_token: overrides.refreshToken ?? "refresh-token", expiry_timestamp: Date.now() + (overrides.expiryMs ?? ONE_HOUR_MS), scope: overrides.scope ?? DEFAULT_OAUTH_SCOPES, + client: overrides.client, }, }); }; @@ -213,6 +216,31 @@ describe("OAuthSessionManager", () => { }); }); + it("authenticates as the client that issued the tokens and keeps it", async () => { + const { secretsManager, manager, setupForOAuthOperation } = + createTestContext(); + const client = { client_id: "issuer", client_secret: "issuer-secret" }; + let params: URLSearchParams | undefined; + await setupForOAuthOperation( + { + "/oauth2/token": (config: InternalAxiosRequestConfig) => { + params = new URLSearchParams(config.data as string); + return createMockTokenResponse(); + }, + "/api/v2/users/me": createMockUser(), + }, + { client }, + ); + + await manager.refreshToken(); + + expect(params?.get("client_id")).toBe("issuer"); + expect(params?.get("client_secret")).toBe("issuer-secret"); + expect( + (await secretsManager.getSessionAuth(TEST_HOSTNAME))?.oauth, + ).toMatchObject({ client }); + }); + it("keeps the stored username when the user lookup fails", async () => { const { secretsManager, manager, setupForOAuthOperation } = createTestContext(); @@ -461,28 +489,50 @@ describe("OAuthSessionManager", () => { }); describe("revokeTokens", () => { - it("revokes the refresh and access tokens", async () => { - const { manager, setupForOAuthOperation } = createTestContext(); - - const revoked: Array<{ token: string | null; hint: string | null }> = []; - await setupForOAuthOperation({ - "/oauth2/revoke": (config: InternalAxiosRequestConfig) => { - const params = new URLSearchParams(config.data as string); - revoked.push({ - token: params.get("token"), - hint: params.get("token_type_hint"), - }); - return {}; - }, - }); + interface RevokeCase { + name: string; + client?: OAuthClient; + clientId: string; + } + it.each([ + { + name: "the client that issued the tokens", + client: { client_id: "issuer", client_secret: "issuer-secret" }, + clientId: "issuer", + }, + { + name: "the stored registration without one", + clientId: "test-client-id", + }, + ])( + "revokes the refresh and access tokens as $name", + async ({ client, clientId }) => { + const { manager, setupForOAuthOperation } = createTestContext(); + + const revoked: Array> = []; + await setupForOAuthOperation( + { + "/oauth2/revoke": (config: InternalAxiosRequestConfig) => { + const params = new URLSearchParams(config.data as string); + revoked.push({ + token: params.get("token"), + hint: params.get("token_type_hint"), + client: params.get("client_id"), + }); + return {}; + }, + }, + { client }, + ); - await manager.revokeTokens(); + await manager.revokeTokens(); - expect(revoked).toEqual([ - { token: "refresh-token", hint: "refresh_token" }, - { token: "access-token", hint: "access_token" }, - ]); - }); + expect(revoked).toEqual([ + { token: "refresh-token", hint: "refresh_token", client: clientId }, + { token: "access-token", hint: "access_token", client: clientId }, + ]); + }, + ); it("revokes tokens with outdated scopes", async () => { const { manager, setupForOAuthOperation } = createTestContext(); diff --git a/test/unit/oauth/tokens.test.ts b/test/unit/oauth/tokens.test.ts index 34092685a3..85074d8dd3 100644 --- a/test/unit/oauth/tokens.test.ts +++ b/test/unit/oauth/tokens.test.ts @@ -5,6 +5,7 @@ import { buildOAuthTokenData } from "@/oauth/tokens"; import type { OAuth2TokenResponse } from "coder/site/src/api/typesGenerated"; const ACCESS_TOKEN_DEFAULT_EXPIRY_MS = 60 * 60 * 1000; +const CLIENT = { client_id: "client-id", client_secret: "client-secret" }; function createTokenResponse( overrides: Partial = {}, @@ -24,6 +25,7 @@ describe("buildOAuthTokenData", () => { it("uses expires_in when valid", () => { const result = buildOAuthTokenData( createTokenResponse({ expires_in: 7200 }), + CLIENT, ); const expectedExpiry = Date.now() + 7200 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -36,6 +38,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: 0 }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -46,6 +49,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: -100 }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -56,6 +60,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: undefined }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -66,6 +71,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: Infinity }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -81,6 +87,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: futureExpiry.toISOString(), }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( futureExpiry.getTime() - 100, @@ -97,6 +104,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: pastExpiry.toISOString(), }), + CLIENT, ); const expectedExpiry = Date.now() + 3600 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -111,6 +119,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: "not-a-valid-date", }), + CLIENT, ); const expectedExpiry = Date.now() + 3600 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -126,6 +135,7 @@ describe("buildOAuthTokenData", () => { expires_in: undefined, expiry: "not-a-valid-date", }), + CLIENT, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -138,6 +148,7 @@ describe("buildOAuthTokenData", () => { expires_in: 7200, expiry: undefined, }), + CLIENT, ); const expectedExpiry = Date.now() + 7200 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -147,16 +158,26 @@ describe("buildOAuthTokenData", () => { }); }); + it("records the client the tokens were issued to", () => { + expect(buildOAuthTokenData(createTokenResponse(), CLIENT).client).toEqual( + CLIENT, + ); + }); + it("stores coder:all when the server returns no scope", () => { expect( - buildOAuthTokenData(createTokenResponse({ scope: undefined })).scope, + buildOAuthTokenData(createTokenResponse({ scope: undefined }), CLIENT) + .scope, ).toBe("coder:all"); }); describe("token_type validation", () => { it("accepts Bearer tokens", () => { expect(() => - buildOAuthTokenData(createTokenResponse({ token_type: "Bearer" })), + buildOAuthTokenData( + createTokenResponse({ token_type: "Bearer" }), + CLIENT, + ), ).not.toThrow(); }); @@ -166,6 +187,7 @@ describe("buildOAuthTokenData", () => { expect(() => buildOAuthTokenData( createTokenResponse({ token_type: tokenType as "Bearer" }), + CLIENT, ), ).toThrow(`Unsupported token type: ${tokenType}`); }, From 40c11e66b00e3d4c9039abb9448c6036055c89b9 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 13 Oct 2026 13:39:40 +0300 Subject: [PATCH 3/4] fix(oauth): address review on CLI token env and client registrations - Spawned CLI processes, including the app-command terminal (with `strictEnv`), get `process.env` without `CODER_SESSION_TOKEN`, matched as the CLI reads it: any letter case, with or without the `HOMEBREW_` prefix. Only the SSH ProxyCommand keeps an inline `--token=`, since Remote-SSH sets its 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. A notice explains the session-token fallback, and both unavailable cases are logged. --- AGENTS.md | 2 + CHANGELOG.md | 11 ++-- src/api/workspace.ts | 8 ++- src/cli/cliCredentialManager.ts | 27 +++------- src/cli/cliExec.ts | 24 +++++++-- src/login/loginCoordinator.ts | 49 +++++------------- src/oauth/authorizer.ts | 43 +++++----------- src/oauth/metadataClient.ts | 29 ++++++++--- src/oauth/revocation.ts | 16 +++--- src/oauth/sessionManager.ts | 12 ++--- src/oauth/tokens.ts | 14 ++--- src/settings/cli.ts | 35 ++++++++++--- src/storage/secretsManager.ts | 4 +- src/ui/prompts.ts | 19 ++++--- test/unit/api/workspace.test.ts | 34 +++++++++--- test/unit/cli/cliCredentialManager.test.ts | 19 ++++--- test/unit/cli/cliExec.test.ts | 45 ++++++++++------ test/unit/login/loginCoordinator.test.ts | 46 +---------------- test/unit/oauth/authorizer.test.ts | 46 +++++++++-------- test/unit/oauth/metadataClient.test.ts | 26 +++++++--- test/unit/oauth/sessionManager.test.ts | 27 +++++----- test/unit/oauth/tokens.test.ts | 40 ++++++++------- test/unit/settings/cli.test.ts | 60 ++++++++++++++-------- test/unit/ui/prompts.test.ts | 55 +++++++++++++++++++- 24 files changed, 393 insertions(+), 298 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b2f8b18823..7ce91d68d3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,6 +68,8 @@ deployment. Treat them as untrusted: - Run the CLI with `execFile`/`spawn` argv (or a terminal's `shellPath` and `shellArgs`), never through a shell or `terminal.sendText`. +- Pass `env: getCliProcessEnv()` (plus `strictEnv: true` for a terminal) so + a host `CODER_SESSION_TOKEN` cannot replace the stored session. - Put flags first, then `--`, then deployment-provided operands (`coder ping -- owner/ws`) so a value like `--foo` is never read as a flag. - Validate names with `src/common/validation.ts` before they reach SSH hosts, diff --git a/CHANGELOG.md b/CHANGELOG.md index 924d36723f..73db892a26 100644 --- a/CHANGELOG.md +++ b/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 a notice + says 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 diff --git a/src/api/workspace.ts b/src/api/workspace.ts index d7735072ca..f1796473c5 100644 --- a/src/api/workspace.ts +++ b/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"; @@ -81,7 +85,7 @@ function runCliCommand(ctx: CliContext, args: string[]): Promise { "--", 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(); diff --git a/src/cli/cliCredentialManager.ts b/src/cli/cliCredentialManager.ts index 989ea2692a..90950b852e 100644 --- a/src/cli/cliCredentialManager.ts +++ b/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 }; -} diff --git a/src/cli/cliExec.ts b/src/cli/cliExec.ts index 9516665461..aa70baed41 100644 --- a/src/cli/cliExec.ts +++ b/src/cli/cliExec.ts @@ -7,7 +7,11 @@ import { validateIdentifier } from "../common/validation"; import { isAbortError } from "../error/abort"; import { toError } from "../error/normalize"; import { type CliFeatureSet } from "../featureSet"; -import { type CliAuth, getGlobalFlags } from "../settings/cli"; +import { + type CliAuth, + getCliProcessEnv, + getGlobalFlags, +} from "../settings/cli"; /** Resolved CLI environment for invoking the coder binary. */ export interface CliEnv { @@ -86,7 +90,10 @@ export async function speedtest( } args.push("--", workspaceName); 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); @@ -105,7 +112,7 @@ export async function netcheck( const result = await execFileAsync( env.binary, [...globalFlags, "netcheck"], - { signal }, + { env: getCliProcessEnv(), signal }, ); return result.stdout; } catch (error) { @@ -141,7 +148,7 @@ export async function supportBundle( ...(agentName ? [agentName] : []), ]; try { - await execFileAsync(env.binary, args, { signal }); + await execFileAsync(env.binary, args, { env: getCliProcessEnv(), signal }); } catch (error) { throw cliError(error); } @@ -185,7 +192,13 @@ export function openAppStatusTerminal( app.command, ]; vscode.window - .createTerminal({ name: app.name, shellPath: env.binary, shellArgs: args }) + .createTerminal({ + name: app.name, + shellPath: env.binary, + shellArgs: args, + env: getCliProcessEnv(), + strictEnv: true, + }) .show(false); } @@ -228,6 +241,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, }); diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 877b8a95ba..bdb7cc0418 100644 --- a/src/login/loginCoordinator.ts +++ b/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"; @@ -288,10 +285,8 @@ 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. + * Authenticate via OAuth, token, or mTLS, prompting as needed. On success, + * persists the session and revokes the OAuth session it replaces. */ private async attemptLogin( deployment: Deployment, @@ -299,8 +294,8 @@ export class LoginCoordinator implements vscode.Disposable { providedToken?: string, tokenSignInConfirmed = false, ): Promise { - // 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, ); @@ -497,18 +492,13 @@ export class LoginCoordinator implements vscode.Disposable { /** Last resort: ask the user how to authenticate. */ private async askAuthMethod(ctx: LoginAttemptContext): Promise { - const authMethod = await maybeAskAuthMethod(ctx.client); + const authMethod = await maybeAskAuthMethod(ctx.client, this.logger); 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 +653,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 { + ): Promise { try { this.logger.debug("Starting OAuth authentication"); @@ -692,21 +684,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) { diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index ec97220b70..b456c3ee7d 100644 --- a/src/oauth/authorizer.ts +++ b/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,9 @@ import type { OAuthCallback } from "./oauthCallback"; interface OAuthLoginResult { tokenResponse: OAuth2TokenResponse; user: User; - /** Client 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 +57,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. Saves the registration on success, never the tokens. */ public async login( deployment: Deployment, progress: vscode.Progress<{ message?: string; increment?: number }>, @@ -134,6 +124,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,20 +178,10 @@ export class OAuthAuthorizer implements vscode.Disposable { token_endpoint_auth_method: TOKEN_ENDPOINT_AUTH_METHOD, }; - const response = await axiosInstance - .post( - 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( + metadata.registration_endpoint, + registrationRequest, + ); const registrationResponse = parseOAuthResponse( OAuth2ClientRegistrationResponseSchema, @@ -204,12 +189,8 @@ export class OAuthAuthorizer implements vscode.Disposable { metadata.registration_endpoint, ); - await this.secretsManager.setOAuthClientRegistration( - deployment.safeHostname, - registrationResponse, - ); this.logger.debug( - "Saved OAuth client registration:", + "Registered OAuth client:", registrationResponse.client_id, ); diff --git a/src/oauth/metadataClient.ts b/src/oauth/metadataClient.ts index af33bc23db..a8adc9e30e 100644 --- a/src/oauth/metadataClient.ts +++ b/src/oauth/metadataClient.ts @@ -38,6 +38,12 @@ const DEFAULT_AUTH_METHODS: readonly OAuth2TokenEndpointAuthMethod[] = [ "client_secret_basic", ]; +/** Whether a server allows OAuth sign-in, or why not. */ +export type OAuthSupport = + | "supported" + | "registration_disabled" + | "unsupported"; + /** * Client for discovering and validating OAuth server metadata. */ @@ -48,16 +54,27 @@ export class OAuthMetadataClient { ) {} /** - * Check if a server supports OAuth by attempting to fetch the well-known endpoint. + * Checks OAuth discovery for client registration, which Coder omits while + * dynamic client registration is off (the default from 2.37). */ public static async checkOAuthSupport( axiosInstance: AxiosInstance, - ): Promise { + logger: Logger, + ): Promise { try { - await axiosInstance.get(OAUTH_DISCOVERY_ENDPOINT); - return true; - } catch { - return false; + const response = await axiosInstance.get< + Partial + >(OAUTH_DISCOVERY_ENDPOINT); + if (response.data.registration_endpoint) { + return "supported"; + } + logger.warn( + "OAuth sign-in unavailable: dynamic client registration is disabled", + ); + return "registration_disabled"; + } catch (error) { + logger.info("OAuth sign-in unavailable: discovery failed", error); + return "unsupported"; } } diff --git a/src/oauth/revocation.ts b/src/oauth/revocation.ts index ffa9c36acf..935ba3c959 100644 --- a/src/oauth/revocation.ts +++ b/src/oauth/revocation.ts @@ -1,5 +1,5 @@ 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"; @@ -7,19 +7,19 @@ 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. Uses the + * registration the tokens were issued to, else `storedRegistration`. */ export async function revokeOAuthTokens( auth: SessionAuth, - fallbackClient: OAuthClient | undefined, + storedRegistration: OAuthClientRegistration | undefined, logger: Logger, ): Promise { 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 { diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index de3bbfbfb8..4b8c834572 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -361,12 +361,12 @@ export class OAuthSessionManager implements vscode.Disposable { } const refreshToken = storedTokens.refresh_token; - const client = - storedTokens.client ?? + const registration = + storedTokens.registration ?? (await this.secretsManager.getOAuthClientRegistration( deployment.safeHostname, )); - if (!client) { + if (!registration) { throw new Error("No client registration found"); } @@ -380,8 +380,8 @@ export class OAuthSessionManager implements vscode.Disposable { const params: OAuth2TokenRequest = { grant_type: REFRESH_GRANT_TYPE, refresh_token: refreshToken, - client_id: client.client_id, - client_secret: client.client_secret, + client_id: registration.client_id, + client_secret: registration.client_secret, }; const response = await axiosInstance.post( @@ -414,7 +414,7 @@ export class OAuthSessionManager implements vscode.Disposable { deployment, tokenResponse.access_token, ), - oauth: buildOAuthTokenData(tokenResponse, client), + oauth: buildOAuthTokenData(tokenResponse, registration), }); return tokenResponse; diff --git a/src/oauth/tokens.ts b/src/oauth/tokens.ts index f635125658..6560bb150f 100644 --- a/src/oauth/tokens.ts +++ b/src/oauth/tokens.ts @@ -25,19 +25,19 @@ export function toUrlSearchParams(obj: object): URLSearchParams { return new URLSearchParams(params); } -/** Client credentials that refresh and revocation authenticate with. */ -export type OAuthClient = Pick< +/** Client ID and secret that refresh and revocation authenticate with. */ +export type OAuthClientRegistration = Pick< OAuth2ClientRegistrationResponse, "client_id" | "client_secret" >; /** - * Build OAuthTokenData from a token response issued to `client`. + * Build OAuthTokenData from a token response issued to `registration`. * Prefers the `expiry` timestamp over calculating from `expires_in`. */ export function buildOAuthTokenData( tokenResponse: OAuth2TokenResponse, - client: OAuthClient, + registration: OAuthClientRegistration, ): OAuthTokenData { if (tokenResponse.token_type !== "Bearer") { throw new Error( @@ -50,9 +50,9 @@ export function buildOAuthTokenData( // Servers that ignore scopes return none and grant everything scope: tokenResponse.scope || "coder:all", expiry_timestamp: getExpiryTimestamp(tokenResponse), - client: { - client_id: client.client_id, - client_secret: client.client_secret, + registration: { + client_id: registration.client_id, + client_secret: registration.client_secret, }, }; } diff --git a/src/settings/cli.ts b/src/settings/cli.ts index f17d58e181..86f4d3b12e 100644 --- a/src/settings/cli.ts +++ b/src/settings/cli.ts @@ -36,7 +36,11 @@ export function getExpandedUserGlobalFlags( }); } -/** Flags for shell contexts (SSH `ProxyCommand`). */ +/** + * Flags for the SSH `ProxyCommand`, whose environment the extension can't set. + * `--token=` hides a host `CODER_SESSION_TOKEN`; a user's `--token` comes + * later and wins. + */ export function getGlobalShellFlags( configs: Pick, auth: CliAuth, @@ -46,10 +50,13 @@ export function getGlobalShellFlags( // OpenSSH (no shell), and `sh`. `maybeAskUrl` rejects these at entry; this // covers URLs that reach storage some other way. validateDeploymentUrl(auth.url); - return buildGlobalFlags(configs, auth, escapeCommandArg, escapeShellArg); + return [ + "--token=", + ...buildGlobalFlags(configs, auth, escapeCommandArg, escapeShellArg), + ]; } -/** Raw flags for `execFile` or `spawn` without a shell. */ +/** Raw flags for `execFile` or `spawn` without a shell; pair them with `getCliProcessEnv`. */ export function getGlobalFlags( configs: Pick, auth: CliAuth, @@ -59,8 +66,22 @@ export function getGlobalFlags( const identity = (s: string) => s; -/** Hides `CODER_SESSION_TOKEN` (in any case) so the CLI uses its stored session. */ -export const EMPTY_TOKEN_FLAG = "--token="; +/** + * `process.env` without `CODER_SESSION_TOKEN` (any letter case, optionally + * `HOMEBREW_`-prefixed), which the CLI would use over its stored session. + */ +export function getCliProcessEnv(): NodeJS.ProcessEnv { + return Object.fromEntries( + Object.entries(process.env).filter(([name]) => !isSessionTokenEnv(name)), + ); +} + +function isSessionTokenEnv(name: string): boolean { + const upper = name.toUpperCase(); + return ( + upper === "CODER_SESSION_TOKEN" || upper === "HOMEBREW_CODER_SESSION_TOKEN" + ); +} function buildGlobalFlags( configs: Pick, @@ -71,12 +92,10 @@ function buildGlobalFlags( // Escape after stripping so expansion whitespace stays in one shell token. // User flags keep shell expansion (e.g. `$VAR`); the auth values and // header command are quoted literally. - const userFlags = stripManagedFlags( + const flags = stripManagedFlags( getExpandedUserGlobalFlags(configs), auth.store === "extension", ).map(escUserFlag); - // A user's own `--token` comes later, so it wins. - const flags = [EMPTY_TOKEN_FLAG, ...userFlags]; if (auth.store === "extension") { flags.push("--global-config", escValue(auth.configDir)); } diff --git a/src/storage/secretsManager.ts b/src/storage/secretsManager.ts index 6587ed1922..22301e580c 100644 --- a/src/storage/secretsManager.ts +++ b/src/storage/secretsManager.ts @@ -39,8 +39,8 @@ const OAuthTokenDataSchema = z.object({ refresh_token: z.string().optional(), scope: z.string(), expiry_timestamp: z.number(), - /** Client that issued the tokens; if absent, the stored registration is used. */ - client: z + /** Registration the tokens were issued to; if absent, the stored registration is used. */ + registration: z .object({ client_id: z.string(), client_secret: z.string().optional() }) .optional(), }); diff --git a/src/ui/prompts.ts b/src/ui/prompts.ts index 663bdc6945..8f0b2585f9 100644 --- a/src/ui/prompts.ts +++ b/src/ui/prompts.ts @@ -11,6 +11,8 @@ import { type MementoManager } from "../storage/mementoManager"; import { withProgress } from "./progress"; +import type { Logger } from "../logging/logger"; + type AuthMethod = "oauth" | "legacy"; /** @@ -146,6 +148,7 @@ export async function maybeAskUrl( export async function maybeAskAuthMethod( client: CoderApi, + logger: Logger, ): Promise { const experimentalOAuthEnabled = vscode.workspace .getConfiguration() @@ -155,20 +158,24 @@ export async function maybeAskAuthMethod( return "legacy"; } - // Check if server supports OAuth with progress indication - const supportsOAuth = await withProgress( + const support = await withProgress( { location: vscode.ProgressLocation.Notification, title: "Checking authentication methods", }, - () => OAuthMetadataClient.checkOAuthSupport(client.getAxiosInstance()), + () => + OAuthMetadataClient.checkOAuthSupport(client.getAxiosInstance(), logger), ); - if (supportsOAuth) { + if (support === "supported") { return await askAuthMethod(); - } else { - return "legacy"; } + if (support === "registration_disabled") { + void vscode.window.showWarningMessage( + "OAuth sign-in is unavailable: dynamic client registration is disabled on this deployment. Signing in with a session token instead.", + ); + } + return "legacy"; } /** diff --git a/test/unit/api/workspace.test.ts b/test/unit/api/workspace.test.ts index 006c67c435..d7772476d6 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -1,9 +1,8 @@ import { EventEmitter } from "node:events"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import * as vscode from "vscode"; import { LazyStream, startWorkspace, updateWorkspace } from "@/api/workspace"; -import { EMPTY_TOKEN_FLAG } from "@/settings/cli"; import { workspace as createWorkspace } from "@repo/mocks"; @@ -22,7 +21,10 @@ vi.mock(import("node:child_process"), async (importOriginal) => ({ })); const { spawn } = await import("node:child_process"); -const GLOBAL_FLAGS = [EMPTY_TOKEN_FLAG, "--url", "https://test.coder.com"]; +const GLOBAL_FLAGS = ["--url", "https://test.coder.com"]; + +/** Arguments of the first CLI invocation. */ +const spawnedArgs = () => vi.mocked(spawn).mock.calls[0][1]; const CLI_FEATURES: CliFeatureSet = { cliLogin: true, @@ -220,6 +222,10 @@ describe("updateWorkspace", () => { vi.clearAllMocks(); }); + afterEach(() => { + vi.unstubAllEnvs(); + }); + it("runs coder update on servers before 2.36", async () => { const { ctx, restClient, finalWorkspace } = createUpdateCtx(); const sp = controlSpawn(); @@ -228,7 +234,7 @@ describe("updateWorkspace", () => { await sp.close(0); await expect(result).resolves.toBe(finalWorkspace); - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ + expect(spawnedArgs()).toEqual([ ...GLOBAL_FLAGS, "update", "--", @@ -238,6 +244,20 @@ describe("updateWorkspace", () => { expect(restClient.getWorkspace).toHaveBeenCalledWith(ctx.workspace.id); }); + it("hides the host session token from the CLI", async () => { + vi.stubEnv("CODER_SESSION_TOKEN", "host-token"); + const { ctx } = createUpdateCtx(); + const sp = controlSpawn(); + + const result = updateWorkspace(ctx, []); + await sp.close(0); + await result; + + expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", expect.any(Array), { + env: expect.not.objectContaining({ CODER_SESSION_TOKEN: "host-token" }), + }); + }); + it("passes collected parameters as --parameter flags", async () => { const { ctx } = createUpdateCtx(); const sp = controlSpawn(); @@ -249,7 +269,7 @@ describe("updateWorkspace", () => { await sp.close(0); await result; - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ + expect(spawnedArgs()).toEqual([ ...GLOBAL_FLAGS, "update", "--parameter", @@ -425,7 +445,7 @@ describe("startWorkspace", () => { await sp.close(0); await expect(result).resolves.toBe(finalWorkspace); - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ + expect(spawnedArgs()).toEqual([ ...GLOBAL_FLAGS, "start", "--yes", @@ -455,7 +475,7 @@ describe("startWorkspace", () => { await sp.close(0); await result; - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ + expect(spawnedArgs()).toEqual([ ...GLOBAL_FLAGS, "start", "--yes", diff --git a/test/unit/cli/cliCredentialManager.test.ts b/test/unit/cli/cliCredentialManager.test.ts index 1e9367e86b..54f2731787 100644 --- a/test/unit/cli/cliCredentialManager.test.ts +++ b/test/unit/cli/cliCredentialManager.test.ts @@ -9,7 +9,6 @@ import { type BinaryResolver, } from "@/cli/cliCredentialManager"; import * as cliExec from "@/cli/cliExec"; -import { EMPTY_TOKEN_FLAG } from "@/settings/cli"; import { PathResolver } from "@/storage/pathResolver"; import { createTestTelemetryService, TestSink } from "../../mocks/telemetry"; @@ -42,7 +41,6 @@ const PATH_RESOLVER = new PathResolver("/mock/base", "/mock/log"); const CRED_DIR = path.join("/mock/base", "dev.coder.com"); const USER_DIR = "/custom/coderv2"; -// Flags for `login --use-token-as-session`; every other command gets EMPTY_TOKEN_FLAG first. const EXTENSION_FLAGS = [ "--global-config", CRED_DIR, @@ -214,7 +212,6 @@ describe("CliCredentialManager", () => { expect(execOptions().env?.CODER_SESSION_TOKEN).toBe("my-secret-token"); expect(execOptions().env).not.toHaveProperty("coder_session_token"); expect(execCalls()[0]).not.toContain("my-secret-token"); - expect(execCalls()[0]).not.toContain(EMPTY_TOKEN_FLAG); }); interface CliErrorCase { @@ -259,9 +256,17 @@ describe("CliCredentialManager", () => { const { manager } = setup(); expect(await manager.readToken(TEST_URL, configs)).toBe("my-token"); - expect(execCalls()).toEqual([ - [EMPTY_TOKEN_FLAG, ...KEYRING_FLAGS, "login", "token"], - ]); + expect(execCalls()).toEqual([[...KEYRING_FLAGS, "login", "token"]]); + }); + + it("hides the host session token from the CLI", async () => { + vi.mocked(os.platform).mockReturnValue("darwin"); + stubExecFile({ token: "my-token" }); + vi.stubEnv("CODER_SESSION_TOKEN", "host-token"); + + await setup().manager.readToken(TEST_URL, configs); + + expect(execOptions().env).not.toHaveProperty("CODER_SESSION_TOKEN"); }); interface ReadTokenCase { @@ -415,7 +420,7 @@ describe("CliCredentialManager", () => { expect(result).toBe(true); expect(execCalls()).toEqual( - logout ? [[EMPTY_TOKEN_FLAG, ...logout, "logout", "--yes"]] : [], + logout ? [[...logout, "logout", "--yes"]] : [], ); expect(credentialFilesExist()).toBe(false); expect(sink.expectOne("auth.credential.clear").properties).toMatchObject({ diff --git a/test/unit/cli/cliExec.test.ts b/test/unit/cli/cliExec.test.ts index 7696514c9f..d04821db6c 100644 --- a/test/unit/cli/cliExec.test.ts +++ b/test/unit/cli/cliExec.test.ts @@ -15,7 +15,6 @@ import { import * as vscode from "vscode"; import { cliFeatureSet } from "@/featureSet"; -import { EMPTY_TOKEN_FLAG } from "@/settings/cli"; import { MockConfigurationProvider, @@ -67,6 +66,10 @@ describe("cliExec", () => { echoArgsBin = await writeExecutable(tmp, "echo-args", code); }); + afterEach(() => { + vi.unstubAllEnvs(); + }); + function setup(auth: CliEnv["auth"], binary = echoArgsBin) { const configs = new MockConfigurationProvider(); const env: CliEnv = { binary, auth, configs }; @@ -202,7 +205,6 @@ describe("cliExec", () => { .trim() .split("\n"); expect(args).toEqual([ - EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "--header-command", @@ -217,6 +219,17 @@ describe("cliExec", () => { ]); }); + it("hides the host session token from the CLI", async () => { + vi.stubEnv("CODER_SESSION_TOKEN", "host-token"); + const code = `console.log(process.env.CODER_SESSION_TOKEN ?? "unset");`; + const bin = await writeExecutable(tmp, "speedtest-env", code); + const { env } = setup(sharedAuth("http://localhost:3000"), bin); + + const output = await cliExec.speedtest(env, "owner/workspace"); + + expect(output.trim()).toBe("unset"); + }); + it("surfaces stderr instead of full command line on failure", async () => { const code = [ writeStderrJs("invalid argument for -t flag\n"), @@ -248,7 +261,6 @@ describe("cliExec", () => { configs.set("coder.headerCommand", "my-header-cmd"); const args = (await cliExec.netcheck(env)).trim().split("\n"); expect(args).toEqual([ - EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "--header-command", @@ -305,7 +317,6 @@ describe("cliExec", () => { }); const args = (await fs.readFile(outputPath, "utf-8")).trim().split("\n"); expect(args).toEqual([ - EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "--header-command", @@ -337,7 +348,6 @@ describe("cliExec", () => { await cliExec.supportBundle(env, "owner/workspace", { outputPath }); const args = (await fs.readFile(outputPath, "utf-8")).trim().split("\n"); expect(args).toEqual([ - EMPTY_TOKEN_FLAG, "--url", "http://localhost:3000", "support", @@ -406,18 +416,20 @@ describe("cliExec", () => { expect(spawn).toHaveBeenCalledWith( env.binary, - [ - EMPTY_TOKEN_FLAG, - "--url", - "https://test.coder.com", - "ping", - "--", - "owner/my workspace", - ], + ["--url", "https://test.coder.com", "ping", "--", "owner/my workspace"], expect.objectContaining({ detached: process.platform !== "win32" }), ); }); + it("hides the host session token from the CLI", () => { + vi.stubEnv("CODER_SESSION_TOKEN", "host-token"); + const { env } = setup(sharedAuth("https://test.coder.com")); + cliExec.ping(env, "owner/ws"); + + const options = vi.mocked(spawn).mock.calls[0][2]; + expect(options.env).not.toHaveProperty("CODER_SESSION_TOKEN"); + }); + it("includes user global flags raw in the spawn argv", () => { const { configs, env } = setup( privateAuth("https://test.coder.com", "/cfg"), @@ -429,7 +441,6 @@ describe("cliExec", () => { expect(spawn).toHaveBeenCalledWith( env.binary, [ - EMPTY_TOKEN_FLAG, "--verbose", "--global-config", "/cfg", @@ -478,6 +489,7 @@ describe("cliExec", () => { ])( "runs the app command as one argv entry after `--` on CLI $cliVersion", ({ cliVersion, ttyArgs }) => { + vi.stubEnv("CODER_SESSION_TOKEN", "host-token"); const env = appEnv(cliVersion); cliExec.openAppStatusTerminal(env, { @@ -490,7 +502,6 @@ describe("cliExec", () => { name: "AI Agent", shellPath: env.binary, shellArgs: [ - EMPTY_TOKEN_FLAG, "--url", "https://test.coder.com", "ssh", @@ -499,6 +510,10 @@ describe("cliExec", () => { "--", command, ], + env: expect.not.objectContaining({ + CODER_SESSION_TOKEN: "host-token", + }), + strictEnv: true, }); }, ); diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index bfe90948cb..24cf8db954 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -6,10 +6,7 @@ import { HttpStatusCode } from "@/api/httpStatusCode"; import { getHeaders } from "@/headers"; import { AuthTelemetry } from "@/instrumentation/auth"; import { LoginCoordinator, type LoginMethod } from "@/login/loginCoordinator"; -import { - ClientRegistrationDisabledError, - OAuthAuthorizer, -} from "@/oauth/authorizer"; +import { OAuthAuthorizer } from "@/oauth/authorizer"; import { OAuthCallback } from "@/oauth/oauthCallback"; import { revokeOAuthTokens } from "@/oauth/revocation"; import { MementoManager } from "@/storage/mementoManager"; @@ -986,47 +983,6 @@ describe("LoginCoordinator", () => { }); }); - describe("when client registration is disabled", () => { - function setup() { - const ctx = createTestContext(); - ctx.mockSuccessfulAuth(); - vi.mocked(maybeAskAuthMethod).mockResolvedValue("oauth"); - vi.spyOn(OAuthAuthorizer.prototype, "login").mockRejectedValue( - new ClientRegistrationDisabledError(), - ); - ctx.userInteraction.setInputBoxValue("session-token"); - return { - ...ctx, - login: () => - ctx.coordinator.ensureLoggedIn({ - url: TEST_URL, - safeHostname: TEST_HOSTNAME, - }), - }; - } - - it("signs in with a session token when the user chooses it", async () => { - const t = setup(); - t.userInteraction.setResponse( - "OAuth sign-in is unavailable", - "Sign In with Token", - ); - - expect(await t.login()).toMatchObject({ - method: "cli_token", - token: "session-token", - }); - }); - - it("cancels sign-in when the user dismisses the prompt", async () => { - const t = setup(); - t.userInteraction.setResponse("OAuth sign-in is unavailable", undefined); - - expect(await t.login()).toMatchObject({ reason: "user_dismissed" }); - expect(vscode.window.showInputBox).not.toHaveBeenCalled(); - }); - }); - describe("telemetry", () => { const dialogOptions = (trigger: "auth_required" | "missing_session") => ({ url: TEST_URL, diff --git a/test/unit/oauth/authorizer.test.ts b/test/unit/oauth/authorizer.test.ts index 3e464d4a4c..ba5afac99e 100644 --- a/test/unit/oauth/authorizer.test.ts +++ b/test/unit/oauth/authorizer.test.ts @@ -3,17 +3,13 @@ import * as vscode from "vscode"; import { InvalidApiResponseError } from "@/api/responseValidation"; import { getHeaders } from "@/headers"; -import { - ClientRegistrationDisabledError, - OAuthAuthorizer, -} from "@/oauth/authorizer"; +import { OAuthAuthorizer } from "@/oauth/authorizer"; import { DEFAULT_OAUTH_SCOPES, IF_SUPPORTED_OAUTH_SCOPES, } from "@/oauth/constants"; import { - createAxiosError, createMockUser, MockCancellationToken, MockProgress, @@ -244,6 +240,31 @@ describe("OAuthAuthorizer", () => { expect(stored?.client_id).toBe("new-client-id"); }); + it("keeps the stored registration when login fails after re-registering", async () => { + const { setupOAuthRoutes, oauthCallback, secretsManager, startLogin } = + createTestContext(); + await secretsManager.setOAuthClientRegistration( + TEST_HOSTNAME, + createMockClientRegistration({ + client_id: "old-client-id", + redirect_uris: ["vscode://different-extension/oauth/callback"], + }), + ); + setupOAuthRoutes(undefined, { + "/oauth2/register": createMockClientRegistration({ + client_id: "new-client-id", + }), + }); + + const { loginPromise, state } = await startLogin(); + await oauthCallback.send({ state, code: null, error: "access_denied" }); + await expect(loginPromise).rejects.toThrow("OAuth error: access_denied"); + + const stored = + await secretsManager.getOAuthClientRegistration(TEST_HOSTNAME); + expect(stored?.client_id).toBe("old-client-id"); + }); + it("reports progress during login flow", async () => { const { setupOAuthRoutes, startLogin, completeLogin } = createTestContext(); @@ -529,21 +550,6 @@ describe("OAuthAuthorizer", () => { ).rejects.toThrow("Server does not support dynamic client registration"); }); - it("throws ClientRegistrationDisabledError when the server refuses registration", async () => { - const { setupOAuthRoutes, authorizer } = createTestContext(); - setupOAuthRoutes(undefined, { - "/oauth2/register": createAxiosError(403, "Forbidden"), - }); - - await expect( - authorizer.login( - createTestDeployment(), - new MockProgress(), - new MockCancellationToken(), - ), - ).rejects.toThrow(ClientRegistrationDisabledError); - }); - it("rejects a token response without an access token", async () => { const { setupOAuthRoutes, startLogin, completeLogin } = createTestContext(); diff --git a/test/unit/oauth/metadataClient.test.ts b/test/unit/oauth/metadataClient.test.ts index 866abd4887..587cc665af 100644 --- a/test/unit/oauth/metadataClient.test.ts +++ b/test/unit/oauth/metadataClient.test.ts @@ -244,7 +244,10 @@ describe("OAuthMetadataClient", () => { }); describe("checkOAuthSupport", () => { - it("returns true when endpoint exists", async () => { + const checkSupport = (axiosInstance: AxiosInstance) => + OAuthMetadataClient.checkOAuthSupport(axiosInstance, createMockLogger()); + + it("returns supported when registration is advertised", async () => { const { mockAdapter, axiosInstance } = createTestContext(); setupAxiosMockRoutes(mockAdapter, { @@ -252,8 +255,20 @@ describe("OAuthMetadataClient", () => { createMockOAuthMetadata(TEST_URL), }); - const result = await OAuthMetadataClient.checkOAuthSupport(axiosInstance); - expect(result).toBe(true); + expect(await checkSupport(axiosInstance)).toBe("supported"); + }); + + it("returns registration_disabled without a registration endpoint", async () => { + const { mockAdapter, axiosInstance } = createTestContext(); + + setupAxiosMockRoutes(mockAdapter, { + "/.well-known/oauth-authorization-server": createMockOAuthMetadata( + TEST_URL, + { registration_endpoint: undefined }, + ), + }); + + expect(await checkSupport(axiosInstance)).toBe("registration_disabled"); }); it.each([ @@ -264,7 +279,7 @@ describe("OAuthMetadataClient", () => { }), }, { name: "network error", error: new Error("Network Error") }, - ])("returns false on $name", async ({ error }) => { + ])("returns unsupported on $name", async ({ error }) => { const { mockAdapter, axiosInstance } = createTestContext(); mockAdapter.mockImplementation((config: InternalAxiosRequestConfig) => { @@ -280,8 +295,7 @@ describe("OAuthMetadataClient", () => { }); }); - const result = await OAuthMetadataClient.checkOAuthSupport(axiosInstance); - expect(result).toBe(false); + expect(await checkSupport(axiosInstance)).toBe("unsupported"); }); }); }); diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index 3848fc20d4..2e9df6c6f4 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -33,7 +33,7 @@ import { } from "./testUtils"; import type { Deployment } from "@/deployment/types"; -import type { OAuthClient } from "@/oauth/tokens"; +import type { OAuthClientRegistration } from "@/oauth/tokens"; import type { TelemetryService } from "@/telemetry/service"; vi.mock("axios", async () => { @@ -86,7 +86,7 @@ function createTestContext(deployment: Deployment = createTestDeployment()) { refreshToken?: string; expiryMs?: number; scope?: string; - client?: OAuthClient; + registration?: OAuthClientRegistration; } = {}, ) => { await base.secretsManager.setSessionAuth(TEST_HOSTNAME, { @@ -97,7 +97,7 @@ function createTestContext(deployment: Deployment = createTestDeployment()) { refresh_token: overrides.refreshToken ?? "refresh-token", expiry_timestamp: Date.now() + (overrides.expiryMs ?? ONE_HOUR_MS), scope: overrides.scope ?? DEFAULT_OAUTH_SCOPES, - client: overrides.client, + registration: overrides.registration, }, }); }; @@ -216,10 +216,13 @@ describe("OAuthSessionManager", () => { }); }); - it("authenticates as the client that issued the tokens and keeps it", async () => { + it("authenticates as the registration that issued the tokens and keeps it", async () => { const { secretsManager, manager, setupForOAuthOperation } = createTestContext(); - const client = { client_id: "issuer", client_secret: "issuer-secret" }; + const registration = { + client_id: "issuer", + client_secret: "issuer-secret", + }; let params: URLSearchParams | undefined; await setupForOAuthOperation( { @@ -229,7 +232,7 @@ describe("OAuthSessionManager", () => { }, "/api/v2/users/me": createMockUser(), }, - { client }, + { registration }, ); await manager.refreshToken(); @@ -238,7 +241,7 @@ describe("OAuthSessionManager", () => { expect(params?.get("client_secret")).toBe("issuer-secret"); expect( (await secretsManager.getSessionAuth(TEST_HOSTNAME))?.oauth, - ).toMatchObject({ client }); + ).toMatchObject({ registration }); }); it("keeps the stored username when the user lookup fails", async () => { @@ -491,13 +494,13 @@ describe("OAuthSessionManager", () => { describe("revokeTokens", () => { interface RevokeCase { name: string; - client?: OAuthClient; + registration?: OAuthClientRegistration; clientId: string; } it.each([ { - name: "the client that issued the tokens", - client: { client_id: "issuer", client_secret: "issuer-secret" }, + name: "the registration that issued the tokens", + registration: { client_id: "issuer", client_secret: "issuer-secret" }, clientId: "issuer", }, { @@ -506,7 +509,7 @@ describe("OAuthSessionManager", () => { }, ])( "revokes the refresh and access tokens as $name", - async ({ client, clientId }) => { + async ({ registration, clientId }) => { const { manager, setupForOAuthOperation } = createTestContext(); const revoked: Array> = []; @@ -522,7 +525,7 @@ describe("OAuthSessionManager", () => { return {}; }, }, - { client }, + { registration }, ); await manager.revokeTokens(); diff --git a/test/unit/oauth/tokens.test.ts b/test/unit/oauth/tokens.test.ts index 85074d8dd3..6e92759978 100644 --- a/test/unit/oauth/tokens.test.ts +++ b/test/unit/oauth/tokens.test.ts @@ -5,7 +5,7 @@ import { buildOAuthTokenData } from "@/oauth/tokens"; import type { OAuth2TokenResponse } from "coder/site/src/api/typesGenerated"; const ACCESS_TOKEN_DEFAULT_EXPIRY_MS = 60 * 60 * 1000; -const CLIENT = { client_id: "client-id", client_secret: "client-secret" }; +const REGISTRATION = { client_id: "client-id", client_secret: "client-secret" }; function createTokenResponse( overrides: Partial = {}, @@ -25,7 +25,7 @@ describe("buildOAuthTokenData", () => { it("uses expires_in when valid", () => { const result = buildOAuthTokenData( createTokenResponse({ expires_in: 7200 }), - CLIENT, + REGISTRATION, ); const expectedExpiry = Date.now() + 7200 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -38,7 +38,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: 0 }), - CLIENT, + REGISTRATION, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -49,7 +49,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: -100 }), - CLIENT, + REGISTRATION, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -60,7 +60,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: undefined }), - CLIENT, + REGISTRATION, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -71,7 +71,7 @@ describe("buildOAuthTokenData", () => { const before = Date.now(); const result = buildOAuthTokenData( createTokenResponse({ expires_in: Infinity }), - CLIENT, + REGISTRATION, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -87,7 +87,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: futureExpiry.toISOString(), }), - CLIENT, + REGISTRATION, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( futureExpiry.getTime() - 100, @@ -104,7 +104,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: pastExpiry.toISOString(), }), - CLIENT, + REGISTRATION, ); const expectedExpiry = Date.now() + 3600 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -119,7 +119,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: "not-a-valid-date", }), - CLIENT, + REGISTRATION, ); const expectedExpiry = Date.now() + 3600 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -135,7 +135,7 @@ describe("buildOAuthTokenData", () => { expires_in: undefined, expiry: "not-a-valid-date", }), - CLIENT, + REGISTRATION, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -148,7 +148,7 @@ describe("buildOAuthTokenData", () => { expires_in: 7200, expiry: undefined, }), - CLIENT, + REGISTRATION, ); const expectedExpiry = Date.now() + 7200 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -158,16 +158,18 @@ describe("buildOAuthTokenData", () => { }); }); - it("records the client the tokens were issued to", () => { - expect(buildOAuthTokenData(createTokenResponse(), CLIENT).client).toEqual( - CLIENT, - ); + it("records the registration the tokens were issued to", () => { + expect( + buildOAuthTokenData(createTokenResponse(), REGISTRATION).registration, + ).toEqual(REGISTRATION); }); it("stores coder:all when the server returns no scope", () => { expect( - buildOAuthTokenData(createTokenResponse({ scope: undefined }), CLIENT) - .scope, + buildOAuthTokenData( + createTokenResponse({ scope: undefined }), + REGISTRATION, + ).scope, ).toBe("coder:all"); }); @@ -176,7 +178,7 @@ describe("buildOAuthTokenData", () => { expect(() => buildOAuthTokenData( createTokenResponse({ token_type: "Bearer" }), - CLIENT, + REGISTRATION, ), ).not.toThrow(); }); @@ -187,7 +189,7 @@ describe("buildOAuthTokenData", () => { expect(() => buildOAuthTokenData( createTokenResponse({ token_type: tokenType as "Bearer" }), - CLIENT, + REGISTRATION, ), ).toThrow(`Unsupported token type: ${tokenType}`); }, diff --git a/test/unit/settings/cli.test.ts b/test/unit/settings/cli.test.ts index a286a16068..2304376996 100644 --- a/test/unit/settings/cli.test.ts +++ b/test/unit/settings/cli.test.ts @@ -5,7 +5,7 @@ import { afterEach, beforeEach, it, expect, describe, vi } from "vitest"; import { cliFeatureSet } from "@/featureSet"; import { type CliAuth, - EMPTY_TOKEN_FLAG, + getCliProcessEnv, getExpandedUserGlobalFlags, getGlobalFlags, getGlobalShellFlags, @@ -80,7 +80,7 @@ describe("cliConfig", () => { ])("emits auth flags for a $scenario", ({ auth, expected }) => { const config = new MockConfigurationProvider(); expect(getGlobalShellFlags(config, auth)).toStrictEqual([ - EMPTY_TOKEN_FLAG, + "--token=", ...expected, ]); }); @@ -101,7 +101,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose", "--global-configs"]); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ - EMPTY_TOKEN_FLAG, + "--token=", "--verbose", "--global-configs", // similar prefixes are not managed flags ...EXTENSION_SHELL_FLAGS, @@ -113,12 +113,23 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose", "--use-keyring=false"]); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ - EMPTY_TOKEN_FLAG, + "--token=", "--verbose", ...EXTENSION_SHELL_FLAGS, ]); }); + it("places a user --token after the empty token flag so it wins", () => { + const config = new MockConfigurationProvider(); + config.set("coder.globalFlags", ["--token=abc"]); + + expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ + "--token=", + "--token=abc", + ...CLI_SHELL_FLAGS, + ]); + }); + const userGlobalConfigCases = [ { scenario: "equals form", flags: ["-v", `--global-config=${USER_DIR}`] }, { @@ -134,7 +145,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", flags); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ - EMPTY_TOKEN_FLAG, + "--token=", ...flags, ...CLI_SHELL_FLAGS, ]); @@ -154,7 +165,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", flags); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ - EMPTY_TOKEN_FLAG, + "--token=", "-v", ...EXTENSION_SHELL_FLAGS, ]); @@ -168,7 +179,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["-v", "--header-command custom"]); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ - EMPTY_TOKEN_FLAG, + "--token=", "-v", '"--header-command custom"', // ignored by CLI ...CLI_SHELL_FLAGS, @@ -204,7 +215,7 @@ describe("cliConfig", () => { configDir: "/home/$USER/%APPDATA%/coder", }), ).toStrictEqual([ - EMPTY_TOKEN_FLAG, + "--token=", '"--header=Authorization=$TOKEN"', "--global-config", configDir, @@ -223,7 +234,7 @@ describe("cliConfig", () => { // Without per-entry escaping the space splits the shell command. expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ - EMPTY_TOKEN_FLAG, + "--token=", '"--cfg=C:\\Users\\John Doe/coder"', ...EXTENSION_SHELL_FLAGS, ]); @@ -237,23 +248,29 @@ describe("cliConfig", () => { config.set("coder.headerCommand", "echo test"); expect(getGlobalFlags(config, extensionStoreAuth)).toStrictEqual([ - EMPTY_TOKEN_FLAG, "--verbose", ...EXTENSION_FLAGS, "--header-command", "echo test", ]); }); + }); - it("places a user --token after the empty token flag so it wins", () => { - const config = new MockConfigurationProvider(); - config.set("coder.globalFlags", ["--token=abc"]); + describe("getCliProcessEnv", () => { + afterEach(() => { + vi.unstubAllEnvs(); + }); - expect(getGlobalFlags(config, cliStoreAuth)).toStrictEqual([ - EMPTY_TOKEN_FLAG, - "--token=abc", - ...CLI_FLAGS, - ]); + it("drops every session token variable the CLI reads", () => { + vi.stubEnv("coder_session_token", "host-token"); + vi.stubEnv("HOMEBREW_CODER_SESSION_TOKEN", "brew-token"); + vi.stubEnv("CODER_URL", "https://coder.example.com"); + + const env = getCliProcessEnv(); + + expect(env).not.toHaveProperty("coder_session_token"); + expect(env).not.toHaveProperty("HOMEBREW_CODER_SESSION_TOKEN"); + expect(env).toHaveProperty("CODER_URL", "https://coder.example.com"); }); }); @@ -514,10 +531,9 @@ describe("cliConfig", () => { } config.set("coder.globalFlags", userFlags); - expect(getGlobalFlags(config, resolve(config, version))).toStrictEqual([ - EMPTY_TOKEN_FLAG, - ...expected, - ]); + expect(getGlobalFlags(config, resolve(config, version))).toStrictEqual( + expected, + ); }); }); }); diff --git a/test/unit/ui/prompts.test.ts b/test/unit/ui/prompts.test.ts index 3801a72e72..91efe42437 100644 --- a/test/unit/ui/prompts.test.ts +++ b/test/unit/ui/prompts.test.ts @@ -1,13 +1,64 @@ -import { describe, expect, it } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; +import { OAuthMetadataClient, type OAuthSupport } from "@/oauth/metadataClient"; import { MementoManager } from "@/storage/mementoManager"; -import { maybeAskUrl } from "@/ui/prompts"; +import { maybeAskAuthMethod, maybeAskUrl } from "@/ui/prompts"; import { + createMockLogger, INVALID_DEPLOYMENT_URLS, InMemoryMemento, + MockConfigurationProvider, } from "../../mocks/testHelpers"; +import type { CoderApi } from "@/api/coderApi"; + +vi.mock("@/ui/progress", () => ({ + withProgress: (_options: unknown, fn: () => Promise) => fn(), +})); + +const client = { getAxiosInstance: vi.fn() } as unknown as CoderApi; + +describe("maybeAskAuthMethod", () => { + beforeEach(() => { + vi.clearAllMocks(); + new MockConfigurationProvider().set("coder.experimental.oauth", true); + }); + + function mockSupport(support: OAuthSupport) { + vi.spyOn(OAuthMetadataClient, "checkOAuthSupport").mockResolvedValue( + support, + ); + } + + it("signs in with a token and explains why when registration is disabled", async () => { + mockSupport("registration_disabled"); + + expect(await maybeAskAuthMethod(client, createMockLogger())).toBe("legacy"); + expect(vscode.window.showWarningMessage).toHaveBeenCalledWith( + expect.stringContaining("dynamic client registration is disabled"), + ); + }); + + it("signs in with a token without a notice when OAuth is unsupported", async () => { + mockSupport("unsupported"); + + expect(await maybeAskAuthMethod(client, createMockLogger())).toBe("legacy"); + expect(vscode.window.showWarningMessage).not.toHaveBeenCalled(); + }); + + it("asks for the method when OAuth is supported", async () => { + mockSupport("supported"); + vi.mocked(vscode.window.showQuickPick).mockResolvedValue({ + label: "OAuth (Recommended)", + value: "oauth", + } as vscode.QuickPickItem); + + expect(await maybeAskAuthMethod(client, createMockLogger())).toBe("oauth"); + }); +}); + describe("maybeAskUrl", () => { const mementoManager = new MementoManager(new InMemoryMemento()); From 1059ab7f876be07478a44dbceba2a60ea0df26dc Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 13 Oct 2026 15:03:50 +0300 Subject: [PATCH 4/4] fix(logging): redact credentials from every logged value The logger summarized only a top-level Axios error. Every logged argument now goes through one redaction walk, for both the output channel and the flight-recorder buffer. It redacts credential fields with string values in plain objects and errors, and summarizes Axios errors at any depth, including error causes. The walk reads only data properties, never throws, and skips entries the channel drops. --- CHANGELOG.md | 4 +- src/logging/flightRecorder.ts | 13 +- src/logging/formatters.ts | 157 +++++++++++++++-------- test/unit/logging/flightRecorder.test.ts | 11 ++ test/unit/logging/formatters.test.ts | 53 +++++++- 5 files changed, 174 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73db892a26..606d61f93e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,8 +33,8 @@ 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. +- Keep session tokens, OAuth tokens, and client secrets out of the Coder output + channel and its buffered logs, including from failed requests. - CLI commands and SSH connections that the extension starts ignore `CODER_SESSION_TOKEN` from the environment, so they no longer fail or run as a different user inside a Coder workspace. diff --git a/src/logging/flightRecorder.ts b/src/logging/flightRecorder.ts index dbc49906b5..c3cce7ba82 100644 --- a/src/logging/flightRecorder.ts +++ b/src/logging/flightRecorder.ts @@ -1,4 +1,4 @@ -import { safeStringify } from "./formatters"; +import { redactSecrets, safeStringify } from "./formatters"; import type { LogSink, Logger } from "./logger"; @@ -109,8 +109,13 @@ export class FlightRecorder implements Logger { /** Pass a call through to the sink and buffer it when below the level. */ private wrap(level: Level): (message: string, ...args: unknown[]) => void { return (message, ...args) => { - this.record(level, message, args); - this.inner[level](message, ...args); + if (SEVERITY[level] < this.channel.logLevel) { + // The channel drops this entry, so only the buffered copy is formatted. + this.record(level, message, args); + this.inner[level](message, ...args); + } else { + this.inner[level](message, ...args.map(redactSecrets)); + } }; } @@ -130,7 +135,7 @@ export class FlightRecorder implements Logger { } private record(level: Level, message: string, args: unknown[]): void { - if (this.capacity === 0 || SEVERITY[level] >= this.channel.logLevel) { + if (this.capacity === 0) { return; } const text = [message, ...args.map((arg) => safeStringify(arg) ?? "")].join( diff --git a/src/logging/formatters.ts b/src/logging/formatters.ts index 4181488276..f50dd79788 100644 --- a/src/logging/formatters.ts +++ b/src/logging/formatters.ts @@ -14,8 +14,8 @@ const SENSITIVE_HEADERS: ReadonlySet> = new Set([ "x-api-key", ]); -/** Credential fields from OAuth token requests/responses, logged at BODY level. */ -const SENSITIVE_BODY_FIELDS: ReadonlySet> = new Set([ +/** Credential fields, such as those in OAuth requests, responses, and sessions. */ +const SENSITIVE_FIELDS: ReadonlySet> = new Set([ "access_token", "client_secret", "code", @@ -78,80 +78,124 @@ export function formatHeaders( export function formatBody(body: unknown): string { if (body) { - return safeStringify(redactBodyFields(body)) ?? ""; + return stringify(redactBody(body)) ?? ""; } else { return ""; } } /** - * Redact known credential fields, copying only what changes; untouched - * values keep their original reference. util.inspect has no replacer - * hook, so the value is walked before stringifying. + * Copy of `value` with credentials redacted and Axios errors summarized at any + * depth; unchanged values keep their reference. Never throws. */ -function redactBodyFields( - value: unknown, - seen = new WeakSet(), -): unknown { - if (typeof value === "string") { - return redactStringBody(value, seen); - } - if (value instanceof URLSearchParams) { - const keys = [...value.keys()]; - if (!keys.some((key) => SENSITIVE_BODY_FIELDS.has(lowercase(key)))) { - return value; - } - return new URLSearchParams( - [...value].map(([key, entry]) => [ - key, - SENSITIVE_BODY_FIELDS.has(lowercase(key)) ? REDACTED : entry, - ]), - ); +export function redactSecrets(value: unknown): unknown { + try { + return redact(value, new WeakSet()); + } catch { + return REDACTED; } +} + +function redact(value: unknown, seen: WeakSet): unknown { if (typeof value !== "object" || value === null || seen.has(value)) { return value; } seen.add(value); + if (isAxiosError(value)) { + return summarizeAxiosError(value); + } + if (value instanceof URLSearchParams) { + return redactParams(value); + } if (Array.isArray(value)) { - const entries: readonly unknown[] = value; - let copy: unknown[] | undefined; - entries.forEach((entry, index) => { - const redacted = redactBodyFields(entry, seen); - if (redacted !== entry) { - copy ??= [...entries]; - copy[index] = redacted; - } - }); - return copy ?? value; + return redactArray(value, seen); } - // Rebuilding a Date or Buffer from its entries would mangle its output. + // Copying a Date, Buffer, or class instance property by property would mangle it. const proto: unknown = Object.getPrototypeOf(value); - if (proto !== Object.prototype && proto !== null) { - return value; + return value instanceof Error || proto === Object.prototype || proto === null + ? redactProperties(value, seen) + : value; +} + +function redactArray( + array: readonly unknown[], + seen: WeakSet, +): readonly unknown[] { + let copy: unknown[] | undefined; + for (const [index, entry] of array.entries()) { + const redacted = redact(entry, seen); + if (redacted !== entry) { + copy ??= [...array]; + copy[index] = redacted; + } } - let copy: Record | undefined; - for (const [key, entry] of Object.entries(value)) { - const redacted = SENSITIVE_BODY_FIELDS.has(lowercase(key)) - ? REDACTED - : redactBodyFields(entry, seen); + return copy ?? array; +} + +/** Reads data properties only, so getters never run. */ +function redactProperties(value: object, seen: WeakSet): object { + let descriptors: PropertyDescriptorMap | undefined; + for (const key of Reflect.ownKeys(value)) { + const descriptor = Object.getOwnPropertyDescriptor(value, key); + if (!descriptor || !("value" in descriptor)) { + continue; + } + const entry: unknown = descriptor.value; + const redacted = isCredential(key, entry) ? REDACTED : redact(entry, seen); if (redacted !== entry) { - copy ??= { ...value }; - copy[key] = redacted; + descriptors ??= Object.getOwnPropertyDescriptors(value); + descriptors[key] = { ...descriptor, value: redacted }; } } - return copy ?? value; + if (!descriptors) { + return value; + } + if (value instanceof Error) { + // V8's `stack` accessor reads the original error's internal slot. + descriptors.stack = { + value: value.stack, + writable: true, + configurable: true, + }; + } + return Object.create( + Object.getPrototypeOf(value) as object | null, + descriptors, + ) as object; } -/** - * Axios error paths expose only the serialized body, so JSON and - * form-encoded strings are parsed too. Clean strings pass through as-is. - */ -function redactStringBody(body: string, seen: WeakSet): unknown { +function redactParams(params: URLSearchParams): URLSearchParams { + const entries = [...params]; + if (!entries.some(([key, entry]) => isCredential(key, entry))) { + return params; + } + return new URLSearchParams( + entries.map(([key, entry]) => [ + key, + isCredential(key, entry) ? REDACTED : entry, + ]), + ); +} + +/** Credentials are strings, so numeric fields such as a close `code` stay readable. */ +function isCredential(key: PropertyKey, value: unknown): boolean { + return ( + typeof key === "string" && + typeof value === "string" && + SENSITIVE_FIELDS.has(lowercase(key)) + ); +} + +/** Axios error paths expose only the serialized body, so JSON and form-encoded strings are parsed too. */ +function redactBody(body: unknown): unknown { + if (typeof body !== "string") { + return redactSecrets(body); + } const trimmed = body.trim(); if (trimmed.startsWith("{") || trimmed.startsWith("[")) { try { const parsed: unknown = JSON.parse(trimmed); - const redacted = redactBodyFields(parsed, seen); + const redacted = redactSecrets(parsed); return redacted === parsed ? body : redacted; } catch { // Not JSON; fall through. @@ -159,7 +203,7 @@ function redactStringBody(body: string, seen: WeakSet): unknown { } if (trimmed.includes("=")) { const params = new URLSearchParams(trimmed); - const redacted = redactBodyFields(params, seen); + const redacted = redactParams(params); return redacted === params ? body : redacted; } return body; @@ -208,13 +252,17 @@ function summarizeAxiosError(error: AxiosError) { method: formatMethod(error.config?.method), url: formatUri(error.config), status: error.response?.status, - data: redactBodyFields(error.response?.data), + data: redactBody(error.response?.data), }; } export function safeStringify(data: unknown): string | null { + return stringify(redactSecrets(data)); +} + +function stringify(data: unknown): string | null { try { - return util.inspect(isAxiosError(data) ? summarizeAxiosError(data) : data, { + return util.inspect(data, { showHidden: false, // Bounded so a single log line cannot balloon in size. depth: 8, @@ -225,7 +273,6 @@ export function safeStringify(data: unknown): string | null { getters: false, // avoid side-effects }); } catch { - // Should rarely happen but just in case return null; } } diff --git a/test/unit/logging/flightRecorder.test.ts b/test/unit/logging/flightRecorder.test.ts index 4c0b822698..eadc8c1732 100644 --- a/test/unit/logging/flightRecorder.test.ts +++ b/test/unit/logging/flightRecorder.test.ts @@ -70,6 +70,17 @@ describe("FlightRecorder", () => { ]); }); + it("redacts credentials from forwarded and buffered args", () => { + const { buffer, calls, flush } = setup(INFO, 10); + const auth = { url: "https://coder.example.com", token: "secret" }; + + buffer.info("shown", auth); + buffer.debug("hidden", auth); + + expect(calls[0].args).toEqual([{ ...auth, token: "" }]); + expect(flush().join(" ")).not.toContain("secret"); + }); + interface LevelCase { level: number; hidden: LogMethod; diff --git a/test/unit/logging/formatters.test.ts b/test/unit/logging/formatters.test.ts index 4fe97cf25b..3437d8aa8b 100644 --- a/test/unit/logging/formatters.test.ts +++ b/test/unit/logging/formatters.test.ts @@ -1,5 +1,5 @@ import { AxiosError, AxiosHeaders } from "axios"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { formatBody, @@ -8,6 +8,7 @@ import { formatSize, formatTime, formatUri, + redactSecrets, safeStringify, sizeOf, } from "@/logging/formatters"; @@ -205,6 +206,40 @@ describe("Logging formatters", () => { }); }); +describe("redactSecrets", () => { + it("returns unchanged values by reference", () => { + const value = { nested: { code: 1006 }, list: ["a"] }; + expect(redactSecrets(value)).toBe(value); + }); + + it("never runs getters and never throws", () => { + const getter = vi.fn(() => "secret"); + const value = Object.defineProperty({ token: "secret" }, "lazy", { + get: getter, + enumerable: true, + }); + const { proxy, revoke } = Proxy.revocable({}, {}); + revoke(); + + expect(redactSecrets(value)).toMatchObject({ token: "" }); + expect(getter).not.toHaveBeenCalled(); + expect(redactSecrets(proxy)).toBe(""); + }); + + it("redacts an error's fields on a copy that keeps its stack", () => { + const error = Object.assign(new TypeError("failed"), { token: "secret" }); + + const redacted = redactSecrets(error); + + expect(redacted).toBeInstanceOf(TypeError); + expect(redacted).toMatchObject({ + message: "failed", + stack: error.stack, + token: "", + }); + }); +}); + describe("sizeOf", () => { type SizeOfTestCase = [data: unknown, bytes: number | undefined]; it.each([ @@ -293,7 +328,17 @@ describe("safeStringify", () => { expect(safeStringify(nested)).not.toContain("bottom"); }); - it("keeps request credentials out of Axios errors", () => { + it("redacts credential fields in any logged object", () => { + const result = safeStringify({ + token: "access-secret", + oauth: { refresh_token: "refresh-secret", scope: "coder:all" }, + }); + + expect(result).toContain("coder:all"); + expect(result).not.toMatch(/access-secret|refresh-secret/); + }); + + it("keeps request credentials out of Axios errors at any depth", () => { const config = { method: "post", url: "https://coder.example.com/oauth2/revoke", @@ -314,8 +359,10 @@ describe("safeStringify", () => { }, ); - const result = safeStringify(error); + const wrapped = new Error("Revocation failed", { cause: error }); + const result = safeStringify({ attempts: [wrapped] }); + expect(result).toContain("Revocation failed"); expect(result).toContain("POST"); expect(result).toContain("https://coder.example.com/oauth2/revoke"); expect(result).toContain("status: 400");