diff --git a/CHANGELOG.md b/CHANGELOG.md index b3506cd456..462c2b77fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ `coder.disableNotifications`; suppressed announcements highlight the status bar item instead). A new **Coder: View Announcements** command opens the full messages in a markdown preview. +- Ask for confirmation before creating a support bundle, with a summary of the + data it collects. ### Changed @@ -22,6 +24,12 @@ workspaces are fetched and the view loads faster. Deployments too old to support the new filter now show a message explaining why instead of an empty list. +- Logging out now revokes the OAuth tokens at the server and warns when locally + stored credentials could not be fully removed. +- Redact more sensitive data from HTTP logs: authorization and cookie headers + regardless of casing, OAuth credential fields in request and response bodies, + and headers produced by `coder.headerCommand`. Shell command output and the + header command's output no longer appear in logs or error messages. ### Fixed @@ -41,6 +49,8 @@ keeps workspace/folder `settings.json` from overriding them (the original SEC-200 goal) while fixing #1032, where a `machine`-scoped value could revert to its default in a remote window. +- Delete the legacy file-based credentials after migrating them to secret + storage, instead of leaving plaintext copies behind. ## [v1.15.2](https://github.com/coder/vscode-coder/releases/tag/v1.15.2) 2026-06-30 diff --git a/src/command/exec.ts b/src/command/exec.ts index 6e43d0408c..e4edf6a627 100644 --- a/src/command/exec.ts +++ b/src/command/exec.ts @@ -33,19 +33,14 @@ export async function execCommand( options?: ExecCommandOptions, ): Promise { const title = options?.title ?? "Command"; - logger.debug(`Executing ${title}: ${command}`); + // The command string and its output can carry credentials, so log neither. + logger.debug(`Executing ${title}`); try { const result = await util.promisify(cp.exec)(command, { env: options?.env, }); logger.debug(`${title} completed successfully`); - if (result.stdout) { - logger.debug(`${title} stdout:`, result.stdout); - } - if (result.stderr) { - logger.debug(`${title} stderr:`, result.stderr); - } return { success: true, stdout: result.stdout, @@ -54,12 +49,6 @@ export async function execCommand( } catch (error) { if (isExecException(error)) { logger.warn(`${title} failed with exit code ${error.code}`); - if (error.stdout) { - logger.warn(`${title} stdout:`, error.stdout); - } - if (error.stderr) { - logger.warn(`${title} stderr:`, error.stderr); - } return { success: false, stdout: error.stdout, @@ -68,7 +57,7 @@ export async function execCommand( }; } - logger.warn(`${title} failed:`, error); + logger.warn(`${title} failed to execute`); return { success: false }; } } diff --git a/src/commands.ts b/src/commands.ts index e058baf2ab..8c38b42312 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -434,6 +434,11 @@ export class Commands { const { agentName, client, workspaceId, remoteAuthority } = resolved; + if (!(await this.confirmSupportBundleCollection())) { + telemetry.abort("prompt"); + return; + } + const outputUri = await this.promptSupportBundlePath(); if (!outputUri) { telemetry.abort("save_dialog"); @@ -523,6 +528,27 @@ export class Commands { }); } + /** Modal disclosure of what a support bundle collects; the CLI's own prompt is suppressed. */ + private async confirmSupportBundleCollection(): Promise { + const detail = [ + "A support bundle may contain sensitive information. It collects:", + "", + "\u2022 Deployment and workspace diagnostics", + "\u2022 Coder extension and connection logs from recent VS Code windows", + "\u2022 Remote SSH extension logs", + "\u2022 Locally recorded telemetry", + "\u2022 Coder extension settings", + "", + "Review the bundle before sharing it.", + ].join("\n"); + const choice = await vscode.window.showInformationMessage( + "Create a support bundle?", + { modal: true, detail }, + "Continue", + ); + return choice === "Continue"; + } + public async exportTelemetry(): Promise { await this.diagnosticTelemetry.trace("export_telemetry", (telemetry) => this.runExportTelemetry(telemetry), @@ -596,8 +622,14 @@ export class Commands { await this.deploymentManager.clearDeployment("logout"); if (deployment) { - await this.cliManager.clearCredentials(deployment.url); + const cleared = await this.cliManager.clearCredentials(deployment.url); await this.secretsManager.clearAllAuthData(deployment.safeHostname); + if (!cleared) { + vscode.window.showWarningMessage( + 'You\'ve been logged out of Coder, but some credentials could not be removed. Log out again to retry, or run "coder logout" in a terminal.', + ); + return { success: false, reason: "cleanup_incomplete" }; + } } this.showLogoutMessage(); diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 763d12d65f..f6299f58fc 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -171,31 +171,34 @@ export class CliCredentialManager { /** * Delete credentials for a deployment. Removes the default-dir files and - * logs out of the active store (keyring or file via --global-config), both - * best-effort. Throws AbortError when the signal is aborted. + * logs out of the active store (keyring or file via --global-config). + * Returns whether every store was cleared instead of throwing, except + * for AbortError when the signal is aborted. */ public deleteToken( url: string, configs: Pick, options?: { signal?: AbortSignal }, - ): Promise { + ): Promise { return this.credentialTelemetry.traceClear(configs, async (span) => { - await Promise.all([ + const [filesCleared, cliCleared] = await Promise.all([ this.deleteCredentialFiles(url), this.cliLogout(url, configs, { signal: options?.signal, span }), ]); + return filesCleared && cliCleared; }); } /** * Log out via `coder logout`, keyring or file (--global-config). Records - * failures on the span instead of throwing (except on abort). + * failures on the span instead of throwing (except on abort) and returns + * whether the logout succeeded. */ private async cliLogout( url: string, configs: Pick, { signal, span }: { signal?: AbortSignal; span: Span }, - ): Promise { + ): Promise { let transport: CliTransport; try { transport = await this.resolveWriteTransport(url, configs); @@ -203,7 +206,7 @@ export class CliCredentialManager { this.logger.warn("Could not resolve CLI binary for logout:", error); span.setProperty("error.type", "binary"); span.markError(); - return; + return false; } const args = [ ...this.credentialGlobalFlags(transport, url, configs), @@ -215,6 +218,7 @@ export class CliCredentialManager { try { await this.execWithTimeout(transport.binPath, args, { signal }); this.logger.info("Deleted token via CLI for", url); + return true; } catch (error) { if (isAbortError(error)) { throw error; @@ -222,6 +226,7 @@ export class CliCredentialManager { this.logger.warn("Failed to delete token via CLI:", error); span.setProperty("error.type", "cli"); span.markError(); + return false; } } @@ -311,21 +316,27 @@ export class CliCredentialManager { } /** - * Delete URL and token files. Best-effort: never throws. + * Delete URL and token files. Returns whether all removals succeeded; + * never throws. */ - private async deleteCredentialFiles(url: string): Promise { + private async deleteCredentialFiles(url: string): Promise { const safeHostname = toSafeHost(url); const paths = [ this.pathResolver.getSessionTokenPath(safeHostname), this.pathResolver.getUrlPath(safeHostname), ]; - await Promise.all( + const results = await Promise.all( paths.map((p) => - fs.rm(p, { force: true }).catch((error) => { - this.logger.warn("Failed to remove credential file", p, error); - }), + fs.rm(p, { force: true }).then( + () => true, + (error) => { + this.logger.warn("Failed to remove credential file", p, error); + return false; + }, + ), ), ); + return results.every(Boolean); } } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 765e0a2dde..19b54cebce 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -1061,9 +1061,10 @@ export class CliManager { /** * Remove credentials for a deployment. Clears both file-based credentials - * and keyring entries (via `coder logout`). All cleanup is best-effort. + * and keyring entries (via `coder logout`). Never throws; returns whether + * every store was cleared. */ - public async clearCredentials(url: string): Promise { + public async clearCredentials(url: string): Promise { const configs = vscode.workspace.getConfiguration(); const result = await withOptionalProgress( ({ signal }) => @@ -1076,13 +1077,14 @@ export class CliManager { }, ); if (result.ok) { - return; + return result.value; } if (result.cancelled) { this.output.info("Credential removal cancelled by user"); } else { this.output.warn("Failed to remove credentials:", result.error); } + return false; } private handleStoreError(error: unknown): void { diff --git a/src/deployment/deploymentManager.ts b/src/deployment/deploymentManager.ts index 40a8718f2f..7c0c47e3e2 100644 --- a/src/deployment/deploymentManager.ts +++ b/src/deployment/deploymentManager.ts @@ -201,6 +201,10 @@ export class DeploymentManager implements vscode.Disposable { "Clearing deployment", this.#sessionStore.current.deployment?.safeHostname, ); + if (reason === "logout") { + // Best-effort server-side revocation before local state is cleared. + await this.oauthSessionManager.revokeTokens(); + } const wasAuthenticated = this.isAuthenticated(); this.#authListenerDisposable?.dispose(); this.#authListenerDisposable = undefined; diff --git a/src/headers.ts b/src/headers.ts index ac46c95cf3..b2fcd12cf9 100644 --- a/src/headers.ts +++ b/src/headers.ts @@ -38,13 +38,14 @@ export async function getHeaders( return headers; } const lines = result.stdout.replace(/\r?\n$/, "").split(/\r?\n/); - for (const line of lines) { + for (const [index, line] of lines.entries()) { const [key, value] = line.split(/=(.*)/); // Header names cannot be blank or contain whitespace and the Coder CLI // requires that there be an equals sign (the value can be blank though). if (key.length === 0 || key.includes(" ") || value === undefined) { + // The output can carry credentials; reference the line by number only. throw new Error( - `Malformed line from header command: [${line}] (out: ${result.stdout})`, + `Malformed line ${index + 1} from header command output`, ); } headers[key] = value; diff --git a/src/instrumentation/auth.ts b/src/instrumentation/auth.ts index abae2d2048..bc21c588ff 100644 --- a/src/instrumentation/auth.ts +++ b/src/instrumentation/auth.ts @@ -17,7 +17,8 @@ export type AuthLoginOutcome = | { success: true; method: LoginMethod } | { success: false; method?: LoginMethod; reason: LoginPromptReason }; export type AuthLogoutOutcome = - { success: true } | { success: false; reason: "not_authenticated" }; + | { success: true } + | { success: false; reason: "not_authenticated" | "cleanup_incomplete" }; interface AuthLoginTrace { setMethod: (method: LoginMethod) => void; diff --git a/src/instrumentation/credentials.ts b/src/instrumentation/credentials.ts index c921e3837a..c193f0dc1f 100644 --- a/src/instrumentation/credentials.ts +++ b/src/instrumentation/credentials.ts @@ -27,25 +27,26 @@ export class CredentialTelemetry { return this.trace("auth.credential.store", configs, fn); } - public traceClear( + public traceClear( configs: Pick, - fn: (span: Span) => Promise, - ): Promise { + fn: (span: Span) => Promise, + ): Promise { return this.trace("auth.credential.clear", configs, fn); } - private async trace( + private async trace( eventName: CredentialEvent, configs: Pick, - fn: (span: Span) => Promise, - ): Promise { + fn: (span: Span) => Promise, + ): Promise { const keyringEnabled = isKeyringEnabled(configs); let aborted: Error | undefined; + let result: T | undefined; await this.telemetry.trace( eventName, async (span) => { try { - await fn(span); + result = await fn(span); } catch (error) { if (isAbortError(error)) { span.markAborted(); @@ -64,6 +65,7 @@ export class CredentialTelemetry { if (aborted) { throw aborted; } + return result as T; } } diff --git a/src/logging/formatters.ts b/src/logging/formatters.ts index df262100e3..b962b1dec1 100644 --- a/src/logging/formatters.ts +++ b/src/logging/formatters.ts @@ -1,14 +1,34 @@ import prettyBytes from "pretty-bytes"; +import { lowercase } from "../util"; + import { safeStringify } from "./utils"; import type { AxiosRequestConfig } from "axios"; -const SENSITIVE_HEADERS = new Set([ - "Coder-Session-Token", - "Proxy-Authorization", +const SENSITIVE_HEADERS: ReadonlySet> = new Set([ + "authorization", + "coder-session-token", + "cookie", + "proxy-authorization", + "set-cookie", + "x-api-key", +]); + +/** Credential fields from OAuth token requests/responses, logged at BODY level. */ +const SENSITIVE_BODY_FIELDS: ReadonlySet> = new Set([ + "access_token", + "client_secret", + "code", + "code_verifier", + "id_token", + "password", + "refresh_token", + "token", ]); +const REDACTED = ""; + export function formatTime(ms: number): string { if (ms < 1000) { return `${ms}ms`; @@ -34,11 +54,18 @@ export function formatUri(config: AxiosRequestConfig | undefined): string { return config?.url || ""; } -export function formatHeaders(headers: Record): string { +export function formatHeaders( + headers: Record, + extraSensitiveNames: readonly string[] = [], +): string { + const extra: ReadonlySet> = new Set( + extraSensitiveNames.map(lowercase), + ); const formattedHeaders = Object.entries(headers) .map(([key, value]) => { - if (SENSITIVE_HEADERS.has(key)) { - return `${key}: `; + const name = lowercase(key); + if (SENSITIVE_HEADERS.has(name) || extra.has(name)) { + return `${key}: ${REDACTED}`; } const strValue = typeof value === "string" ? value : safeStringify(value); return `${key}: ${strValue}`; @@ -51,8 +78,89 @@ export function formatHeaders(headers: Record): string { export function formatBody(body: unknown): string { if (body) { - return safeStringify(body) ?? ""; + return safeStringify(redactBodyFields(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. + */ +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, + ]), + ); + } + if (typeof value !== "object" || value === null || seen.has(value)) { + return value; + } + seen.add(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; + } + // Rebuilding a Date or Buffer from its entries would mangle its output. + const proto: unknown = Object.getPrototypeOf(value); + if (proto !== Object.prototype && proto !== null) { + return value; + } + let copy: Record | undefined; + for (const [key, entry] of Object.entries(value)) { + const redacted = SENSITIVE_BODY_FIELDS.has(lowercase(key)) + ? REDACTED + : redactBodyFields(entry, seen); + if (redacted !== entry) { + copy ??= { ...value }; + copy[key] = redacted; + } + } + return copy ?? value; +} + +/** + * 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 { + const trimmed = body.trim(); + if (trimmed.startsWith("{") || trimmed.startsWith("[")) { + try { + const parsed: unknown = JSON.parse(trimmed); + const redacted = redactBodyFields(parsed, seen); + return redacted === parsed ? body : redacted; + } catch { + // Not JSON; fall through. + } + } + if (trimmed.includes("=")) { + const params = new URLSearchParams(trimmed); + const redacted = redactBodyFields(params, seen); + return redacted === params ? body : redacted; + } + return body; +} diff --git a/src/logging/httpLogger.ts b/src/logging/httpLogger.ts index 3c5d782294..424fd591be 100644 --- a/src/logging/httpLogger.ts +++ b/src/logging/httpLogger.ts @@ -46,7 +46,12 @@ export function logRequest( const msg = [ `→ ${shortId(requestId)} ${method} ${url} ${requestSize}`, - ...buildExtraLogs(config.headers, config.data, logLevel), + ...buildExtraLogs( + config.headers, + config.data, + logLevel, + config.headerCommandKeys, + ), ]; logger.trace(msg.join("\n")); } @@ -69,7 +74,12 @@ export function logResponse( const msg = [ `← ${shortId(requestId)} ${response.status} ${method} ${url} ${responseSize} ${time}`, - ...buildExtraLogs(response.headers, response.data, logLevel), + ...buildExtraLogs( + response.headers, + response.data, + logLevel, + response.config.headerCommandKeys, + ), ]; logger.trace(msg.join("\n")); } @@ -110,6 +120,7 @@ export function logError( error.response.headers, error.response.data, logLevel, + config?.headerCommandKeys, ); } else { if (errorParts.length === 0) { @@ -120,6 +131,7 @@ export function logError( error?.config?.headers ?? {}, error.config?.data, logLevel, + config?.headerCommandKeys, ); } @@ -134,10 +146,12 @@ function buildExtraLogs( headers: Record, body: unknown, logLevel: HttpClientLogLevel, + headerCommandKeys: readonly string[] | undefined, ) { const msg = []; if (logLevel >= HttpClientLogLevel.HEADERS) { - msg.push(formatHeaders(headers)); + // Headers applied by the header command are treated as sensitive too. + msg.push(formatHeaders(headers, headerCommandKeys ?? [])); } if (logLevel >= HttpClientLogLevel.BODY) { msg.push(formatBody(body)); diff --git a/src/logging/utils.ts b/src/logging/utils.ts index 5deadaaff4..b72765997a 100644 --- a/src/logging/utils.ts +++ b/src/logging/utils.ts @@ -48,9 +48,10 @@ export function safeStringify(data: unknown): string | null { try { return util.inspect(data, { showHidden: false, - depth: Infinity, - maxArrayLength: Infinity, - maxStringLength: Infinity, + // Bounded so a single log line cannot balloon in size. + depth: 8, + maxArrayLength: 100, + maxStringLength: 10_000, breakLength: Infinity, compact: true, getters: false, // avoid side-effects diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index b8ef0ee003..27fddeb121 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -441,68 +441,55 @@ export class OAuthSessionManager implements vscode.Disposable { } } - public async revokeRefreshToken(): Promise { - const storedTokens = await this.getStoredTokens(); - if (!storedTokens?.refresh_token) { - this.logger.debug("No refresh token to revoke"); + /** Best-effort server-side revocation of the stored refresh and access tokens; never throws. */ + public async revokeTokens(): Promise { + const storedTokens = await this.getStoredTokens().catch((error) => { + this.logger.warn("Failed to read stored tokens for revocation:", error); + return undefined; + }); + if (!storedTokens) { return; } - await this.revokeToken( - storedTokens.access_token, - storedTokens.refresh_token, - "refresh_token", - ); - } - - /** - * Revoke a token using the OAuth server's revocation endpoint. - * - * @param authToken - Token for authenticating the revocation request - * @param tokenToRevoke - The token to be revoked - * @param tokenTypeHint - Hint about the token type being revoked - */ - private async revokeToken( - authToken: string, - tokenToRevoke: string, - tokenTypeHint: "access_token" | "refresh_token" = "refresh_token", - ): Promise { - await this.withOAuthOperation( - authToken, - async ({ axiosInstance, metadata, registration }) => { - if (!metadata.revocation_endpoint) { - this.logger.debug( - "No revocation endpoint available, skipping revocation", - ); - return; - } - - this.logger.debug("Revoking refresh token"); - - const params: OAuth2TokenRevocationRequest = { - token: tokenToRevoke, - client_id: registration.client_id, - client_secret: registration.client_secret, - token_type_hint: tokenTypeHint, - }; + // 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 axiosInstance.post( - metadata.revocation_endpoint, - toUrlSearchParams(params), - { - headers: { - "Content-Type": "application/x-www-form-urlencoded", - }, - }, - ); - this.logger.debug("Token revocation successful"); - } catch (error) { - this.logger.error("Token revocation failed:", error); - throw error; - } - }, - ); + 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); + } } /** diff --git a/src/remote/migration.ts b/src/remote/migration.ts new file mode 100644 index 0000000000..c18a68fde7 --- /dev/null +++ b/src/remote/migration.ts @@ -0,0 +1,93 @@ +import * as fs from "node:fs/promises"; + +import type { PathResolver } from "../core/pathResolver"; +import type { SecretsManager } from "../core/secretsManager"; +import type { Logger } from "../logging/logger"; + +type SessionAuthStore = Pick< + SecretsManager, + "getSessionAuth" | "setSessionAuth" +>; + +/** + * Migrate legacy file-based auth to secrets storage: rename the old + * "session_token" file to "session", then move the url/session file + * contents into secret storage. + */ +export async function migrateAuthToSecretsStorage( + safeHostname: string, + pathResolver: PathResolver, + secretsManager: SessionAuthStore, + logger: Logger, +): Promise { + await migrateSessionTokenFile(safeHostname, pathResolver); + await migrateSessionAuthFromFiles( + safeHostname, + pathResolver, + secretsManager, + logger, + ); +} + +/** + * Migrate the session token file from "session_token" to "session". + */ +async function migrateSessionTokenFile( + safeHostname: string, + pathResolver: PathResolver, +): Promise { + const oldTokenPath = pathResolver.getLegacySessionTokenPath(safeHostname); + const newTokenPath = pathResolver.getSessionTokenPath(safeHostname); + try { + await fs.rename(oldTokenPath, newTokenPath); + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") { + throw error; + } + } +} + +/** + * Migrate URL and session token from files to the multi-deployment secrets + * storage. + */ +async function migrateSessionAuthFromFiles( + safeHostname: string, + pathResolver: PathResolver, + secretsManager: SessionAuthStore, + logger: Logger, +): Promise { + const existingAuth = await secretsManager.getSessionAuth(safeHostname); + if (existingAuth) { + return; + } + + const urlPath = pathResolver.getUrlPath(safeHostname); + const tokenPath = pathResolver.getSessionTokenPath(safeHostname); + const [url, token] = await Promise.allSettled([ + fs.readFile(urlPath, "utf8"), + fs.readFile(tokenPath, "utf8"), + ]); + + if (url.status === "fulfilled" && token.status === "fulfilled") { + logger.info("Migrating session auth from files for", safeHostname); + try { + await secretsManager.setSessionAuth(safeHostname, { + url: url.value.trim(), + token: token.value.trim(), + }); + } catch (error) { + logger.warn("Failed to migrate session auth from files:", error); + } + // Drop the plaintext copies even on failure: a rejected pair names + // another deployment, and connect rewrites the CLI credentials in its + // cli_configure phase right after this migration runs. + await Promise.all( + [urlPath, tokenPath].map((filePath) => + fs.rm(filePath, { force: true }).catch((error) => { + logger.warn("Failed to remove migrated auth file", filePath, error); + }), + ), + ); + } +} diff --git a/src/remote/remote.ts b/src/remote/remote.ts index c799ef8aef..8c7189c8de 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -48,6 +48,7 @@ import { vscodeProposed } from "../vscodeProposed"; import { WorkspaceMonitor } from "../workspace/workspaceMonitor"; import { applySshEnvironment, SSH_PROXY_SETTINGS } from "./environment"; +import { migrateAuthToSecretsStorage } from "./migration"; import { SshConfig, type SshValues, @@ -163,7 +164,12 @@ export class Remote { // Both run before `remote.setup` so an auth-required retry doesn't nest // traces, and migration is kept out of `auth.session_lookup` so a slow // first-run migration doesn't pollute that signal. - await this.migrateToSecretsStorage(parts.safeHostname); + await migrateAuthToSecretsStorage( + parts.safeHostname, + this.pathResolver, + this.secretsManager, + this.logger, + ); const telemetry = this.serviceContainer.getTelemetryService(); const auth = await this.authTelemetry.traceSessionLookup(() => this.secretsManager.getSessionAuth(parts.safeHostname), @@ -782,59 +788,6 @@ export class Remote { ); } - /** - * Migrate legacy file-based auth to secrets storage. - */ - private async migrateToSecretsStorage(safeHostname: string) { - await this.migrateSessionTokenFile(safeHostname); - await this.migrateSessionAuthFromFiles(safeHostname); - } - - /** - * Migrate the session token file from "session_token" to "session". - */ - private async migrateSessionTokenFile(safeHostname: string) { - const oldTokenPath = - this.pathResolver.getLegacySessionTokenPath(safeHostname); - const newTokenPath = this.pathResolver.getSessionTokenPath(safeHostname); - try { - await fs.rename(oldTokenPath, newTokenPath); - } catch (error) { - if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") { - throw error; - } - } - } - - /** - * Migrate URL and session token from files to the mutli-deployment secrets storage. - */ - private async migrateSessionAuthFromFiles(safeHostname: string) { - const existingAuth = await this.secretsManager.getSessionAuth(safeHostname); - if (existingAuth) { - return; - } - - const urlPath = this.pathResolver.getUrlPath(safeHostname); - const tokenPath = this.pathResolver.getSessionTokenPath(safeHostname); - const [url, token] = await Promise.allSettled([ - fs.readFile(urlPath, "utf8"), - fs.readFile(tokenPath, "utf8"), - ]); - - if (url.status === "fulfilled" && token.status === "fulfilled") { - this.logger.info("Migrating session auth from files for", safeHostname); - try { - await this.secretsManager.setSessionAuth(safeHostname, { - url: url.value.trim(), - token: token.value.trim(), - }); - } catch (error) { - this.logger.warn("Failed to migrate session auth from files:", error); - } - } - } - /** * Return the --log-dir argument value for the ProxyCommand, or an empty * string when the CLI does not support it. diff --git a/src/util.ts b/src/util.ts index 33510981c5..979700c614 100644 --- a/src/util.ts +++ b/src/util.ts @@ -51,6 +51,11 @@ export function expandPath(input: string): string { return tildeExpanded.replaceAll("${userHome}", userHome); } +/** `toLowerCase` typed for indexing `Lowercase`-keyed records without a cast. */ +export function lowercase(value: T): Lowercase { + return value.toLowerCase() as Lowercase; +} + /** * Return the number of times a substring appears in a string. */ diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index b03d3daa14..f89a0a3162 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -240,6 +240,7 @@ export interface MessageCall { level: "information" | "warning" | "error"; message: string; items: string[]; + options?: vscode.MessageOptions; } /** @@ -327,7 +328,12 @@ export class MockUserInteraction { const items = rest.filter( (arg): arg is string => typeof arg === "string", ); - this._messageCalls.push({ level, message, items }); + // Options object, as opposed to a MessageItem (which has a title). + const options = rest.find( + (arg): arg is vscode.MessageOptions => + typeof arg === "object" && arg !== null && !("title" in arg), + ); + this._messageCalls.push({ level, message, items, options }); return Promise.resolve(getResponse(message)); }; @@ -472,7 +478,7 @@ export function createMockCliCredentialManager(): CliCredentialManager { return { storeToken: vi.fn().mockResolvedValue(undefined), readToken: vi.fn().mockResolvedValue(undefined), - deleteToken: vi.fn().mockResolvedValue(undefined), + deleteToken: vi.fn().mockResolvedValue(true), } as unknown as CliCredentialManager; } @@ -493,12 +499,19 @@ export interface LogEntry { args: readonly unknown[]; } -/** Logger that records structured entries for tests of logging behavior. */ +/** + * Logger that records what was logged. Assert on `entries` for exact output, + * or search `text` when checking that a secret never reached the log. + */ export class LogCollector implements Logger { - private readonly _entries: LogEntry[] = []; + readonly entries: LogEntry[] = []; - get entries(): readonly LogEntry[] { - return this._entries; + /** Every message and argument logged, as one searchable string. */ + get text(): string { + return this.entries + .flatMap((entry) => [entry.message, ...entry.args]) + .map(String) + .join("\n"); } trace(message: string, ...args: unknown[]): void { @@ -528,7 +541,7 @@ export class LogCollector implements Logger { message: string, args: readonly unknown[], ): void { - this._entries.push({ level, message, args }); + this.entries.push({ level, message, args }); } } @@ -873,7 +886,7 @@ export class MockOAuthSessionManager { readonly refreshToken = vi .fn() .mockResolvedValue({ access_token: "test-token" }); - readonly revokeRefreshToken = vi.fn().mockResolvedValue(undefined); + readonly revokeTokens = vi.fn().mockResolvedValue(undefined); readonly isLoggedInWithOAuth = vi.fn().mockResolvedValue(false); readonly clearOAuthState = vi.fn().mockResolvedValue(undefined); readonly dispose = vi.fn(); diff --git a/test/unit/command/exec.test.ts b/test/unit/command/exec.test.ts index 3041ebf2b8..3f6ab751cc 100644 --- a/test/unit/command/exec.test.ts +++ b/test/unit/command/exec.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it } from "vitest"; import { execCommand } from "@/command/exec"; -import { createMockLogger } from "../../mocks/testHelpers"; +import { createMockLogger, LogCollector } from "../../mocks/testHelpers"; import { exitCommand, printCommand, @@ -58,4 +58,18 @@ describe("execCommand", () => { const result = await execCommand(printCommand("test"), logger); expect(result.success).toBe(true); }); + + it("should not log the command or its output", async () => { + const quietLogger = new LogCollector(); + await execCommand(printCommand("quiet-output-value"), quietLogger, { + title: "Test", + }); + await execCommand( + `${printCommand("quiet-output-value")} && ${exitCommand(3)}`, + quietLogger, + { title: "Test" }, + ); + + expect(quietLogger.text).not.toContain("quiet-output-value"); + }); }); diff --git a/test/unit/commands.supportBundle.test.ts b/test/unit/commands.supportBundle.test.ts index 1cee6dadd6..d473b2f764 100644 --- a/test/unit/commands.supportBundle.test.ts +++ b/test/unit/commands.supportBundle.test.ts @@ -17,6 +17,7 @@ import { config, createMockLogger, MockProgressReporter, + MockUserInteraction, } from "../mocks/testHelpers"; import type { CoderApi } from "@/api/coderApi"; @@ -66,6 +67,9 @@ function setup(options: { cliVersion?: string } = {}) { vi.mocked(vscode.window.showSaveDialog).mockResolvedValue( vscode.Uri.file(OUTPUT_PATH), ); + // Accept the collection disclosure dialog by default. + const interaction = new MockUserInteraction(); + interaction.setResponse("Create a support bundle?", "Continue"); vi.mocked(cliExec.version).mockResolvedValue(options.cliVersion ?? "v2.36.0"); vi.mocked(cliExec.supportBundle).mockResolvedValue(undefined); vi.mocked(getRemoteServerDataPath).mockResolvedValue({ @@ -107,7 +111,7 @@ function setup(options: { cliVersion?: string } = {}) { {} as DeploymentManager, ); - return { commands, client, logger }; + return { commands, client, logger, interaction }; } function setRemoteAuthority(value: string | undefined): void { @@ -234,4 +238,31 @@ describe("Commands.supportBundle", () => { expect.objectContaining({ workspaceFiles: [] }), ); }); + + it("describes the collected data before creating the bundle", async () => { + const { commands, interaction } = setup(); + + await commands.supportBundle(agentItem("dev")); + + expect(interaction.getMessageCalls()).toContainEqual( + expect.objectContaining({ + message: "Create a support bundle?", + items: ["Continue"], + options: expect.objectContaining({ + modal: true, + detail: expect.stringContaining("telemetry"), + }), + }), + ); + }); + + it("does not create a bundle when the disclosure dialog is dismissed", async () => { + const { commands, interaction } = setup(); + interaction.setResponse("Create a support bundle?", undefined); + + await commands.supportBundle(agentItem("dev")); + + expect(vscode.window.showSaveDialog).not.toHaveBeenCalled(); + expect(cliExec.supportBundle).not.toHaveBeenCalled(); + }); }); diff --git a/test/unit/commands.telemetry.test.ts b/test/unit/commands.telemetry.test.ts index 629efdf144..17b59d6f9f 100644 --- a/test/unit/commands.telemetry.test.ts +++ b/test/unit/commands.telemetry.test.ts @@ -45,11 +45,12 @@ interface SetupOptions { readonly authenticated?: boolean; readonly loginResult?: LoginResultForTest; readonly clearAllAuthDataError?: Error; + readonly clearCredentialsResult?: boolean; } function setup(options: SetupOptions = {}) { vi.clearAllMocks(); - new MockUserInteraction(); + const interaction = new MockUserInteraction(); vi.mocked(maybeAskUrl).mockResolvedValue(TEST_URL); const { sink, service } = createTelemetryHarness(); @@ -83,7 +84,9 @@ function setup(options: SetupOptions = {}) { }; const cliManager: Pick = { - clearCredentials: vi.fn(() => Promise.resolve()), + clearCredentials: vi.fn(() => + Promise.resolve(options.clearCredentialsResult ?? true), + ), }; const secretsManager: Pick< @@ -125,6 +128,7 @@ function setup(options: SetupOptions = {}) { return { commands, sink, + interaction, mocks: { cliManager, deploymentManager, loginCoordinator, secretsManager }, }; } @@ -273,5 +277,31 @@ describe("Commands", () => { error: { message: "secret clear failed" }, }); }); + + it("reports incomplete credential cleanup instead of success", async () => { + const { commands, sink, interaction } = setup({ + authenticated: true, + clearCredentialsResult: false, + }); + + await commands.logout(); + + expect(sink.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "aborted", + reason: "cleanup_incomplete", + }, + }); + const messages = interaction + .getMessageCalls() + .map((call) => call.message); + expect(messages).toContainEqual( + expect.stringContaining("could not be removed"), + ); + // The success toast must not appear alongside the warning. + expect(messages).not.toContainEqual( + expect.stringContaining("You've been logged out of Coder!"), + ); + }); }); }); diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 15b90bc311..e02d45f4b4 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -541,8 +541,9 @@ describe("CliCredentialManager", () => { writeCredentialFiles(TEST_URL, "old-token"); const { manager, resolver, sink } = setup(); - await manager.deleteToken(TEST_URL, configs); + const result = await manager.deleteToken(TEST_URL, configs); + expect(result).toBe(true); expect(resolver).toHaveBeenCalledWith(TEST_URL); const exec = lastExecArgs(); expect(exec.bin).toBe(TEST_BIN); @@ -580,9 +581,7 @@ describe("CliCredentialManager", () => { stubExecFile({ error: "logout failed" }); const { manager, sink } = setup(); - await expect( - manager.deleteToken(TEST_URL, configs), - ).resolves.not.toThrow(); + await expect(manager.deleteToken(TEST_URL, configs)).resolves.toBe(false); expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { "error.type": "cli", @@ -595,9 +594,7 @@ describe("CliCredentialManager", () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); const { manager, sink } = setup(failingResolver()); - await expect( - manager.deleteToken(TEST_URL, configs), - ).resolves.toBeUndefined(); + await expect(manager.deleteToken(TEST_URL, configs)).resolves.toBe(false); expect(execFile).not.toHaveBeenCalled(); expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { diff --git a/test/unit/core/cliManager.test.ts b/test/unit/core/cliManager.test.ts index e28854ae47..c150c1649c 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -340,16 +340,25 @@ describe("CliManager", () => { }); it.each([ - { scenario: "succeeds", error: undefined }, - { scenario: "fails", error: new Error("unexpected failure") }, - { scenario: "is cancelled", error: makeAbortError() }, - ])("should not throw when deleteToken $scenario", async ({ error }) => { - const { manager, mockCredManager } = setupCliManager(); - if (error) { - vi.mocked(mockCredManager.deleteToken).mockRejectedValueOnce(error); - } - await expect(manager.clearCredentials(CLEAR_URL)).resolves.not.toThrow(); - }); + { scenario: "succeeds", error: undefined, cleared: true }, + { + scenario: "fails", + error: new Error("unexpected failure"), + cleared: false, + }, + { scenario: "is cancelled", error: makeAbortError(), cleared: false }, + ])( + "should report cleanup state when deleteToken $scenario", + async ({ error, cleared }) => { + const { manager, mockCredManager } = setupCliManager(); + if (error) { + vi.mocked(mockCredManager.deleteToken).mockRejectedValueOnce(error); + } + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toBe( + cleared, + ); + }, + ); }); describe("Binary Version Validation", () => { diff --git a/test/unit/deployment/deploymentManager.test.ts b/test/unit/deployment/deploymentManager.test.ts index 1ee2d3064b..1a45bfa596 100644 --- a/test/unit/deployment/deploymentManager.test.ts +++ b/test/unit/deployment/deploymentManager.test.ts @@ -153,6 +153,37 @@ describe("DeploymentManager", () => { expect(currentUserId(manager)).toBeUndefined(); expect(manager.isAuthenticated()).toBe(false); }); + + it("revokes OAuth tokens when clearing for logout", async () => { + const { manager, mockOAuthSessionManager } = createTestContext(); + + await manager.setDeployment({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + token: "test-token", + user: createMockUser(), + }); + + await manager.clearDeployment("logout"); + + expect(mockOAuthSessionManager.revokeTokens).toHaveBeenCalledTimes(1); + expect(manager.isAuthenticated()).toBe(false); + }); + + it("does not revoke OAuth tokens for other clear reasons", async () => { + const { manager, mockOAuthSessionManager } = createTestContext(); + + await manager.setDeployment({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + token: "test-token", + user: createMockUser(), + }); + + await manager.clearDeployment("credentials_removed"); + + expect(mockOAuthSessionManager.revokeTokens).not.toHaveBeenCalled(); + }); }); describe("setDeployment", () => { diff --git a/test/unit/headers.test.ts b/test/unit/headers.test.ts index bb35350b1a..ec00fc5ce6 100644 --- a/test/unit/headers.test.ts +++ b/test/unit/headers.test.ts @@ -87,6 +87,14 @@ describe("Headers", () => { ).rejects.toThrow(/Malformed/); }); + it("should not include command output in parse errors", async () => { + const command = printCommand("Authorization=Bearer SECRET-VALUE\nbad line"); + // Anchored match: the message must carry the line number and nothing else. + await expect(getHeaders("localhost", command, logger)).rejects.toThrow( + /^Malformed line 2 from header command output$/, + ); + }); + it("should have access to environment variables", async () => { const coderUrl = "dev.coder.com"; await expect( diff --git a/test/unit/logging/formatters.test.ts b/test/unit/logging/formatters.test.ts index 1cd4fedfa6..8eb699dd66 100644 --- a/test/unit/logging/formatters.test.ts +++ b/test/unit/logging/formatters.test.ts @@ -71,8 +71,18 @@ describe("Logging formatters", () => { expect(result).toContain("accept: text/html"); }); - it("redacts sensitive headers", () => { - const sensitiveHeaders = ["Coder-Session-Token", "Proxy-Authorization"]; + it("redacts sensitive headers regardless of casing", () => { + const sensitiveHeaders = [ + "authorization", + "AUTHORIZATION", + "Coder-Session-Token", + "coder-session-token", + "cookie", + "set-cookie", + "SET-COOKIE", + "X-Api-Key", + "Proxy-Authorization", + ]; sensitiveHeaders.forEach((header) => { const result = formatHeaders({ [header]: "secret-value" }); @@ -81,6 +91,16 @@ describe("Logging formatters", () => { }); }); + it("redacts extra header names case-insensitively", () => { + const result = formatHeaders( + { "X-Custom-Auth": "secret-value", accept: "text/html" }, + ["x-custom-auth"], + ); + expect(result).toContain("X-Custom-Auth: "); + expect(result).not.toContain("secret-value"); + expect(result).toContain("accept: text/html"); + }); + it("returns placeholder for empty headers", () => { expect(formatHeaders({})).toBe(""); }); @@ -118,5 +138,64 @@ describe("Logging formatters", () => { expect(formatBody(value)).toContain("no body"); }); }); + + it("redacts sensitive fields in objects", () => { + const result = formatBody({ + access_token: "secret-access", + refresh_token: "secret-refresh", + client_secret: "secret-client", + code: "secret-code", + code_verifier: "secret-verifier", + id_token: "secret-id", + password: "secret-password", + token: "secret-token", + token_type: "bearer", + }); + expect(result).toContain("access_token: ''"); + expect(result).toContain("token_type: 'bearer'"); + expect(result).not.toContain("secret-"); + }); + + it("redacts sensitive fields in nested objects and arrays", () => { + const result = formatBody({ + data: { session: { TOKEN: "secret-value" } }, + items: [{ password: "secret-value" }], + }); + expect(result).toContain("TOKEN: ''"); + expect(result).toContain("password: ''"); + expect(result).not.toContain("secret-value"); + }); + + it("redacts sensitive fields in URLSearchParams", () => { + const params = new URLSearchParams({ + grant_type: "refresh_token", + refresh_token: "secret-value", + }); + const result = formatBody(params); + expect(result).toContain("refresh_token"); + expect(result).toContain(""); + expect(result).not.toContain("secret-value"); + }); + + it("redacts sensitive fields in serialized bodies", () => { + const json = formatBody( + JSON.stringify({ access_token: "secret-value", expires_in: 3600 }), + ); + expect(json).toContain("expires_in"); + expect(json).not.toContain("secret-value"); + + const form = formatBody( + "grant_type=authorization_code&code=secret-value", + ); + expect(form).toContain("grant_type"); + expect(form).not.toContain("secret-value"); + }); + + it("leaves non-sensitive strings unchanged", () => { + expect(formatBody("plain response text")).toContain( + "plain response text", + ); + expect(formatBody("a=b&c=d")).toContain("a=b&c=d"); + }); }); }); diff --git a/test/unit/logging/httpLogger.test.ts b/test/unit/logging/httpLogger.test.ts index 81cfbed877..116c24fe42 100644 --- a/test/unit/logging/httpLogger.test.ts +++ b/test/unit/logging/httpLogger.test.ts @@ -12,7 +12,7 @@ import { type RequestConfigWithMeta, } from "@/logging/types"; -import { createMockLogger } from "../../mocks/testHelpers"; +import { createMockLogger, LogCollector } from "../../mocks/testHelpers"; describe("REST HTTP Logger", () => { describe("log level behavior", () => { @@ -109,4 +109,60 @@ describe("REST HTTP Logger", () => { expect(logger.error).toHaveBeenCalledWith("Request error", error); }); }); + + describe("redaction", () => { + const config = { + method: "POST", + url: "https://api.example.com/endpoint", + headers: { + authorization: "Bearer request-secret", + "X-From-Command": "command-secret", + } as unknown as AxiosHeaders, + data: { refresh_token: "body-secret" }, + headerCommandKeys: ["X-From-Command"], + metadata: createRequestMeta(), + } as RequestConfigWithMeta; + + it("redacts sensitive request headers and body fields", () => { + const logger = new LogCollector(); + + logRequest(logger, config, HttpClientLogLevel.BODY); + + const logged = logger.text; + expect(logged).toContain("authorization: "); + expect(logged).toContain("X-From-Command: "); + // Every planted value contains "secret"; none may survive. + expect(logged).not.toContain("secret"); + }); + + it("redacts sensitive headers and body fields on error paths", () => { + const logger = new LogCollector(); + const error = new AxiosError("Bad Request"); + error.config = config; + error.response = { + status: 400, + headers: { "set-cookie": ["session=response-secret"] }, + data: { access_token: "body-secret", error: "invalid_grant" }, + } as unknown as AxiosResponse; + + logError(logger, error, HttpClientLogLevel.BODY); + + const logged = logger.text; + expect(logged).toContain("set-cookie: "); + expect(logged).toContain("invalid_grant"); + // Every planted value contains "secret"; none may survive. + expect(logged).not.toContain("secret"); + }); + + it("redacts header-command headers on network error paths", () => { + const logger = new LogCollector(); + const error = new AxiosError("Network Error", "ECONNREFUSED"); + error.config = config; + + logError(logger, error, HttpClientLogLevel.BODY); + + // Every planted value contains "secret"; none may survive. + expect(logger.text).not.toContain("secret"); + }); + }); }); diff --git a/test/unit/logging/utils.test.ts b/test/unit/logging/utils.test.ts index 989a23e1f2..2a3b8ca263 100644 --- a/test/unit/logging/utils.test.ts +++ b/test/unit/logging/utils.test.ts @@ -95,6 +95,22 @@ describe("Logging utils", () => { const result = safeStringify(deep); expect(result).toContain("level4: { value: 'deep' }"); }); + + it("bounds output size for large inputs", () => { + const longString = safeStringify("a".repeat(50_000)); + expect(longString?.length).toBeLessThan(20_000); + + const bigArray = safeStringify( + Array.from({ length: 10_000 }, (_, i) => i), + ); + expect(bigArray).toContain("more items"); + + let nested: Record = { value: "bottom" }; + for (let i = 0; i < 30; i++) { + nested = { child: nested }; + } + expect(safeStringify(nested)).not.toContain("bottom"); + }); }); describe("createRequestId", () => { diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index 6c25d9cad7..ac6887076a 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -422,22 +422,47 @@ describe("OAuthSessionManager", () => { }); }); - describe("revokeRefreshToken", () => { - it("revokes token via revocation endpoint", async () => { + describe("revokeTokens", () => { + it("revokes the refresh and access tokens", async () => { const { manager, setupForOAuthOperation } = createTestContext(); - let revokedToken: string | undefined; + const revoked: Array<{ token: string | null; hint: string | null }> = []; await setupForOAuthOperation({ "/oauth2/revoke": (config: InternalAxiosRequestConfig) => { const params = new URLSearchParams(config.data as string); - revokedToken = params.get("token") ?? undefined; + revoked.push({ + token: params.get("token"), + hint: params.get("token_type_hint"), + }); return {}; }, }); - await manager.revokeRefreshToken(); + await manager.revokeTokens(); + + expect(revoked).toEqual([ + { token: "refresh-token", hint: "refresh_token" }, + { token: "access-token", hint: "access_token" }, + ]); + }); + + it("does not throw when revocation fails", async () => { + const { manager, setupForOAuthOperation } = createTestContext(); + + await setupForOAuthOperation({ + "/oauth2/revoke": () => { + throw new Error("revocation endpoint unavailable"); + }, + }); + + await expect(manager.revokeTokens()).resolves.toBeUndefined(); + }); + + it("is a no-op without stored tokens", async () => { + const { manager, mockAdapter } = createTestContext(); - expect(revokedToken).toBe("refresh-token"); + await expect(manager.revokeTokens()).resolves.toBeUndefined(); + expect(mockAdapter).not.toHaveBeenCalled(); }); }); diff --git a/test/unit/remote/migration.test.ts b/test/unit/remote/migration.test.ts new file mode 100644 index 0000000000..0eadd0c27b --- /dev/null +++ b/test/unit/remote/migration.test.ts @@ -0,0 +1,97 @@ +import { vol } from "memfs"; +import { describe, expect, it, vi } from "vitest"; + +import { PathResolver } from "@/core/pathResolver"; +import { migrateAuthToSecretsStorage } from "@/remote/migration"; + +import { createMockLogger } from "../../mocks/testHelpers"; + +import type * as nodeFs from "node:fs"; + +import type { SessionAuth } from "@/core/secretsManager"; + +vi.mock("fs/promises", async () => { + const memfs: { fs: typeof nodeFs } = await vi.importActual("memfs"); + return { + ...memfs.fs.promises, + default: memfs.fs.promises, + }; +}); + +const BASE_PATH = "/base"; +const HOSTNAME = "dep.example.com"; +const URL_PATH = `${BASE_PATH}/${HOSTNAME}/url`; +const TOKEN_PATH = `${BASE_PATH}/${HOSTNAME}/session`; + +function setup(options: { existingAuth?: SessionAuth } = {}) { + vol.reset(); + const secretsManager = { + getSessionAuth: vi.fn(() => Promise.resolve(options.existingAuth)), + setSessionAuth: vi.fn(() => Promise.resolve()), + }; + const migrate = () => + migrateAuthToSecretsStorage( + HOSTNAME, + new PathResolver(BASE_PATH, "/logs/code"), + secretsManager, + createMockLogger(), + ); + return { migrate, secretsManager }; +} + +function writeLegacyFiles(): void { + vol.fromJSON({ + [URL_PATH]: "https://dep.example.com\n", + [TOKEN_PATH]: "legacy-token\n", + }); +} + +describe("Session auth migration", () => { + it("moves file-based auth into secret storage and deletes the files", async () => { + const { migrate, secretsManager } = setup(); + writeLegacyFiles(); + + await migrate(); + + expect(secretsManager.setSessionAuth).toHaveBeenCalledWith(HOSTNAME, { + url: "https://dep.example.com", + token: "legacy-token", + }); + expect(vol.existsSync(URL_PATH)).toBe(false); + expect(vol.existsSync(TOKEN_PATH)).toBe(false); + }); + + it("deletes the files even when the migration is rejected", async () => { + const { migrate, secretsManager } = setup(); + secretsManager.setSessionAuth.mockRejectedValue( + new Error("Session auth hostname mismatch"), + ); + writeLegacyFiles(); + + await migrate(); + + expect(vol.existsSync(URL_PATH)).toBe(false); + expect(vol.existsSync(TOKEN_PATH)).toBe(false); + }); + + it("does not migrate or delete files when auth already exists", async () => { + const { migrate, secretsManager } = setup({ + existingAuth: { url: "https://dep.example.com", token: "current" }, + }); + writeLegacyFiles(); + + await migrate(); + + expect(secretsManager.setSessionAuth).not.toHaveBeenCalled(); + expect(vol.existsSync(URL_PATH)).toBe(true); + expect(vol.existsSync(TOKEN_PATH)).toBe(true); + }); + + it("does nothing when the legacy files are missing", async () => { + const { migrate, secretsManager } = setup(); + + await migrate(); + + expect(secretsManager.setSessionAuth).not.toHaveBeenCalled(); + }); +}); diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts index 835240fc92..e9286edbbe 100644 --- a/test/unit/remote/remote.test.ts +++ b/test/unit/remote/remote.test.ts @@ -92,9 +92,7 @@ describe("Remote", () => { await remote.setup(REMOTE_AUTHORITY, "none", "anysphere.remote-ssh"); // The mismatched URL carries a token, so only its hostname is logged. - expect( - logs.entries.filter((entry) => entry.level === "warn"), - ).toContainEqual({ + expect(logs.entries).toContainEqual({ level: "warn", message: "Failed to migrate session auth from files:", args: [