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 ee675121a1..606d61f93e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,9 +20,24 @@ ### 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. +- 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 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. ### 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/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 16bc140026..90950b852e 100644 --- a/src/cli/cliCredentialManager.ts +++ b/src/cli/cliCredentialManager.ts @@ -14,6 +14,7 @@ import { import { recordError } from "../instrumentation/outcomes"; import { type CliAuth, + getCliProcessEnv, getGlobalFlags, mayUseCliStore, resolveCliAuth, @@ -76,7 +77,7 @@ export class CliCredentialManager { } span.setProperty("store", cli.auth.store); await this.exec(cli, ["login", "--use-token-as-session", url], { - env: { ...process.env, CODER_SESSION_TOKEN: token }, + env: { ...getCliProcessEnv(), CODER_SESSION_TOKEN: token }, signal: options?.signal, }); span.setProperty("outcome", "stored"); @@ -234,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, }); 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/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 d3389b7358..f50dd79788 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", @@ -15,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", @@ -79,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. @@ -160,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; @@ -200,7 +243,24 @@ 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: 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(data, { showHidden: false, @@ -213,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/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 98263562cf..bdb7cc0418 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -8,6 +8,7 @@ import { HttpStatusCode } from "../api/httpStatusCode"; import { isSameOrigin, openInBrowser } from "../common/url"; import { CertificateError } from "../error/certificateError"; import { 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 +16,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 +113,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 +170,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 +194,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 replaced the stored session during this login; keep 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), + ); } } @@ -275,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, @@ -286,15 +294,24 @@ export class LoginCoordinator implements vscode.Disposable { providedToken?: string, tokenSignInConfirmed = false, ): Promise { + // Read before OAuth login replaces it; it revokes a replaced session saved + // without its own registration. + 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(); } @@ -475,7 +492,7 @@ 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": return withLoginMethod( @@ -645,25 +662,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) { const title = "OAuth authentication failed"; diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index c3fcb7db75..b456c3ee7d 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -37,6 +37,12 @@ import type { SecretsManager } from "../storage/secretsManager"; import type { OAuthCallback } from "./oauthCallback"; +interface OAuthLoginResult { + tokenResponse: OAuth2TokenResponse; + user: User; + registration: OAuth2ClientRegistrationResponse; +} + /** * Handles the OAuth authorization code flow for authenticating with Coder deployments. * Encapsulates client registration, PKCE challenge, and token exchange. @@ -51,16 +57,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 registration on success, never 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( @@ -79,7 +81,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"); @@ -122,9 +124,14 @@ 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 }; + return { tokenResponse, user, registration }; } /** @@ -182,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/constants.ts b/src/oauth/constants.ts index 32c314fa1a..7d48893f78 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,9 @@ 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 b6b5b4e24f..a8adc9e30e 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 { @@ -37,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. */ @@ -47,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"; } } @@ -142,6 +160,29 @@ export class OAuthMetadataClient { } } +/** Runs `fn` with the server's OAuth metadata and a client disposed afterward. */ +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..935ba3c959 --- /dev/null +++ b/src/oauth/revocation.ts @@ -0,0 +1,67 @@ +import { withOAuthMetadata } from "./metadataClient"; +import { type OAuthClientRegistration, toUrlSearchParams } from "./tokens"; + +import type { OAuth2TokenRevocationRequest } from "coder/site/src/api/typesGenerated"; + +import type { Logger } from "../logging/logger"; +import type { SessionAuth } from "../storage/secretsManager"; + +/** + * Best-effort revocation of the token pair in `auth`; never throws. Uses the + * registration the tokens were issued to, else `storedRegistration`. + */ +export async function revokeOAuthTokens( + auth: SessionAuth, + storedRegistration: OAuthClientRegistration | undefined, + logger: Logger, +): Promise { + if (!auth.oauth) { + return; + } + const registration = auth.oauth.registration ?? storedRegistration; + 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..4b8c834572 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,20 @@ export class OAuthSessionManager implements vscode.Disposable { } const refreshToken = storedTokens.refresh_token; - const accessToken = storedTokens.access_token; + const registration = + storedTokens.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 = { @@ -444,7 +414,7 @@ export class OAuthSessionManager implements vscode.Disposable { deployment, tokenResponse.access_token, ), - oauth: buildOAuthTokenData(tokenResponse), + oauth: buildOAuthTokenData(tokenResponse, registration), }); return tokenResponse; @@ -484,64 +454,31 @@ 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. + * 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). */ - public async isLoggedInWithOAuth(hostname?: string): Promise { + public async canRefreshOAuthSession(hostname?: string): Promise { if (hostname && hostname !== this.deployment?.safeHostname) { return false; } diff --git a/src/oauth/tokens.ts b/src/oauth/tokens.ts index 1f0ade9165..6560bb150f 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 ID and secret that refresh and revocation authenticate with. */ +export type OAuthClientRegistration = Pick< + OAuth2ClientRegistrationResponse, + "client_id" | "client_secret" +>; + /** - * Build OAuthTokenData from a token response. + * Build OAuthTokenData from a token response issued to `registration`. * Prefers the `expiry` timestamp over calculating from `expires_in`. */ export function buildOAuthTokenData( tokenResponse: OAuth2TokenResponse, + registration: OAuthClientRegistration, ): 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), + registration: { + client_id: registration.client_id, + client_secret: registration.client_secret, + }, }; } diff --git a/src/settings/cli.ts b/src/settings/cli.ts index 68f4510d51..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,6 +66,23 @@ export function getGlobalFlags( const identity = (s: string) => s; +/** + * `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, auth: CliAuth, diff --git a/src/storage/secretsManager.ts b/src/storage/secretsManager.ts index 82e6f5e9d3..22301e580c 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(), + /** 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(), }); export type OAuthTokenData = z.infer; 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/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..d7772476d6 100644 --- a/test/unit/api/workspace.test.ts +++ b/test/unit/api/workspace.test.ts @@ -1,5 +1,5 @@ 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"; @@ -21,6 +21,11 @@ vi.mock(import("node:child_process"), async (importOriginal) => ({ })); const { spawn } = await import("node:child_process"); +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, proxyLogDirectory: true, @@ -217,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(); @@ -225,9 +234,8 @@ describe("updateWorkspace", () => { await sp.close(0); await expect(result).resolves.toBe(finalWorkspace); - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + expect(spawnedArgs()).toEqual([ + ...GLOBAL_FLAGS, "update", "--", "testuser/test-workspace", @@ -236,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(); @@ -247,9 +269,8 @@ describe("updateWorkspace", () => { await sp.close(0); await result; - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + expect(spawnedArgs()).toEqual([ + ...GLOBAL_FLAGS, "update", "--parameter", "region=us-east", @@ -424,9 +445,8 @@ describe("startWorkspace", () => { await sp.close(0); await expect(result).resolves.toBe(finalWorkspace); - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + expect(spawnedArgs()).toEqual([ + ...GLOBAL_FLAGS, "start", "--yes", "--reason", @@ -455,9 +475,8 @@ describe("startWorkspace", () => { await sp.close(0); await result; - expect(spawn).toHaveBeenCalledWith("/usr/bin/coder", [ - "--url", - "https://test.coder.com", + expect(spawnedArgs()).toEqual([ + ...GLOBAL_FLAGS, "start", "--yes", "--", diff --git a/test/unit/cli/cliCredentialManager.test.ts b/test/unit/cli/cliCredentialManager.test.ts index 10b6ba4f03..54f2731787 100644 --- a/test/unit/cli/cliCredentialManager.test.ts +++ b/test/unit/cli/cliCredentialManager.test.ts @@ -204,11 +204,13 @@ 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"); }); @@ -257,6 +259,16 @@ describe("CliCredentialManager", () => { 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 { scenario: string; token: ExecResult; diff --git a/test/unit/cli/cliExec.test.ts b/test/unit/cli/cliExec.test.ts index 62100bcb79..d04821db6c 100644 --- a/test/unit/cli/cliExec.test.ts +++ b/test/unit/cli/cliExec.test.ts @@ -66,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 }; @@ -215,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"), @@ -406,6 +421,15 @@ describe("cliExec", () => { ); }); + 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"), @@ -465,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, { @@ -485,6 +510,10 @@ describe("cliExec", () => { "--", command, ], + env: expect.not.objectContaining({ + CODER_SESSION_TOKEN: "host-token", + }), + strictEnv: true, }); }, ); 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 ded822f45e..3437d8aa8b 100644 --- a/test/unit/logging/formatters.test.ts +++ b/test/unit/logging/formatters.test.ts @@ -1,4 +1,5 @@ -import { describe, expect, it } from "vitest"; +import { AxiosError, AxiosHeaders } from "axios"; +import { describe, expect, it, vi } from "vitest"; import { formatBody, @@ -7,6 +8,7 @@ import { formatSize, formatTime, formatUri, + redactSecrets, safeStringify, sizeOf, } from "@/logging/formatters"; @@ -204,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([ @@ -291,4 +327,48 @@ describe("safeStringify", () => { } expect(safeStringify(nested)).not.toContain("bottom"); }); + + 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", + 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 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"); + 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 ab413003ce..24cf8db954 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -6,9 +6,11 @@ import { HttpStatusCode } from "@/api/httpStatusCode"; import { getHeaders } from "@/headers"; import { AuthTelemetry } from "@/instrumentation/auth"; import { LoginCoordinator, type LoginMethod } from "@/login/loginCoordinator"; +import { 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 +25,10 @@ import { MockProgressReporter, MockUserInteraction, } from "../../mocks/testHelpers"; +import { + createMockClientRegistration, + createMockTokenResponse, +} from "../oauth/testUtils"; import type { User } from "coder/site/src/api/typesGenerated"; @@ -75,6 +81,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 +861,128 @@ 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, + registration: createMockClientRegistration({ + client_id: "new-client", + }), + }); + 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("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..ba5afac99e 100644 --- a/test/unit/oauth/authorizer.test.ts +++ b/test/unit/oauth/authorizer.test.ts @@ -240,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(); 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 71d38f1d50..2e9df6c6f4 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 { OAuthClientRegistration } 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; + registration?: OAuthClientRegistration; } = {}, ) => { 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, + registration: overrides.registration, }, }); }; @@ -136,14 +139,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 +180,7 @@ describe("OAuthSessionManager", () => { await secretsManager.setSessionAuth(TEST_HOSTNAME, auth); } - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(expected); }); }); @@ -213,6 +216,34 @@ describe("OAuthSessionManager", () => { }); }); + it("authenticates as the registration that issued the tokens and keeps it", async () => { + const { secretsManager, manager, setupForOAuthOperation } = + createTestContext(); + const registration = { + 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(), + }, + { registration }, + ); + + 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({ registration }); + }); + it("keeps the stored username when the user lookup fails", async () => { const { secretsManager, manager, setupForOAuthOperation } = createTestContext(); @@ -264,7 +295,7 @@ describe("OAuthSessionManager", () => { }, }); - expect(await manager.isLoggedInWithOAuth()).toBe(false); + expect(await manager.canRefreshOAuthSession()).toBe(false); }); }); @@ -279,7 +310,7 @@ describe("OAuthSessionManager", () => { await manager.setDeployment(newDeployment); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); }); @@ -290,7 +321,7 @@ describe("OAuthSessionManager", () => { manager.clearDeployment(); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); }); @@ -461,28 +492,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; + registration?: OAuthClientRegistration; + clientId: string; + } + it.each([ + { + name: "the registration that issued the tokens", + registration: { 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 ({ registration, 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 {}; + }, + }, + { registration }, + ); - 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(); @@ -518,7 +571,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 +584,7 @@ describe("OAuthSessionManager", () => { }, }); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(false); }); @@ -550,7 +603,7 @@ describe("OAuthSessionManager", () => { }, }); - const result = await manager.isLoggedInWithOAuth(); + const result = await manager.canRefreshOAuthSession(); expect(result).toBe(true); }); @@ -558,7 +611,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/oauth/tokens.test.ts b/test/unit/oauth/tokens.test.ts index 34092685a3..6e92759978 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 REGISTRATION = { 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 }), + REGISTRATION, ); 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 }), + REGISTRATION, ); 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 }), + REGISTRATION, ); 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 }), + REGISTRATION, ); 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 }), + REGISTRATION, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -81,6 +87,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: futureExpiry.toISOString(), }), + REGISTRATION, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( futureExpiry.getTime() - 100, @@ -97,6 +104,7 @@ describe("buildOAuthTokenData", () => { expires_in: 3600, expiry: pastExpiry.toISOString(), }), + REGISTRATION, ); 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", }), + REGISTRATION, ); 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", }), + REGISTRATION, ); expect(result.expiry_timestamp).toBeGreaterThanOrEqual( before + ACCESS_TOKEN_DEFAULT_EXPIRY_MS, @@ -138,6 +148,7 @@ describe("buildOAuthTokenData", () => { expires_in: 7200, expiry: undefined, }), + REGISTRATION, ); const expectedExpiry = Date.now() + 7200 * 1000; expect(result.expiry_timestamp).toBeGreaterThanOrEqual( @@ -147,16 +158,28 @@ describe("buildOAuthTokenData", () => { }); }); + 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 })).scope, + buildOAuthTokenData( + createTokenResponse({ scope: undefined }), + REGISTRATION, + ).scope, ).toBe("coder:all"); }); describe("token_type validation", () => { it("accepts Bearer tokens", () => { expect(() => - buildOAuthTokenData(createTokenResponse({ token_type: "Bearer" })), + buildOAuthTokenData( + createTokenResponse({ token_type: "Bearer" }), + REGISTRATION, + ), ).not.toThrow(); }); @@ -166,6 +189,7 @@ describe("buildOAuthTokenData", () => { expect(() => buildOAuthTokenData( createTokenResponse({ token_type: tokenType as "Bearer" }), + REGISTRATION, ), ).toThrow(`Unsupported token type: ${tokenType}`); }, 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..2304376996 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, + getCliProcessEnv, 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([ + "--token=", + ...expected, + ]); }); it.each(INVALID_DEPLOYMENT_URLS)( @@ -97,6 +101,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose", "--global-configs"]); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + "--token=", "--verbose", "--global-configs", // similar prefixes are not managed flags ...EXTENSION_SHELL_FLAGS, @@ -108,11 +113,23 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["--verbose", "--use-keyring=false"]); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + "--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}`] }, { @@ -128,6 +145,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", flags); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ + "--token=", ...flags, ...CLI_SHELL_FLAGS, ]); @@ -147,6 +165,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", flags); expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + "--token=", "-v", ...EXTENSION_SHELL_FLAGS, ]); @@ -160,6 +179,7 @@ describe("cliConfig", () => { config.set("coder.globalFlags", ["-v", "--header-command custom"]); expect(getGlobalShellFlags(config, cliStoreAuth)).toStrictEqual([ + "--token=", "-v", '"--header-command custom"', // ignored by CLI ...CLI_SHELL_FLAGS, @@ -195,6 +215,7 @@ describe("cliConfig", () => { configDir: "/home/$USER/%APPDATA%/coder", }), ).toStrictEqual([ + "--token=", '"--header=Authorization=$TOKEN"', "--global-config", configDir, @@ -213,6 +234,7 @@ describe("cliConfig", () => { // Without per-entry escaping the space splits the shell command. expect(getGlobalShellFlags(config, extensionStoreAuth)).toStrictEqual([ + "--token=", '"--cfg=C:\\Users\\John Doe/coder"', ...EXTENSION_SHELL_FLAGS, ]); @@ -234,6 +256,24 @@ describe("cliConfig", () => { }); }); + describe("getCliProcessEnv", () => { + afterEach(() => { + vi.unstubAllEnvs(); + }); + + 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"); + }); + }); + describe("getExpandedUserGlobalFlags", () => { it("returns empty array when no global flags configured", () => { const config = new MockConfigurationProvider(); 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());