From 9f4e6fe7b7339c1781f29109b6d22c90f8cfde78 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Jul 2026 09:00:59 +0000 Subject: [PATCH 1/9] fix(logging): keep command output out of header command errors and logs --- src/command/exec.ts | 17 +++-------------- src/headers.ts | 5 +++-- test/unit/command/exec.test.ts | 23 ++++++++++++++++++++++- test/unit/headers.test.ts | 14 ++++++++++++++ 4 files changed, 42 insertions(+), 17 deletions(-) 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/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/test/unit/command/exec.test.ts b/test/unit/command/exec.test.ts index 3041ebf2b8..67b9d9964f 100644 --- a/test/unit/command/exec.test.ts +++ b/test/unit/command/exec.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { execCommand } from "@/command/exec"; @@ -58,4 +58,25 @@ 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 = createMockLogger(); + await execCommand(printCommand("quiet-output-value"), quietLogger, { + title: "Test", + }); + await execCommand( + `${printCommand("quiet-output-value")} && ${exitCommand(3)}`, + quietLogger, + { title: "Test" }, + ); + + const logged = [ + ...vi.mocked(quietLogger.debug).mock.calls, + ...vi.mocked(quietLogger.warn).mock.calls, + ] + .flat() + .map(String) + .join("\n"); + expect(logged).not.toContain("quiet-output-value"); + }); }); diff --git a/test/unit/headers.test.ts b/test/unit/headers.test.ts index bb35350b1a..9f3dcf54c4 100644 --- a/test/unit/headers.test.ts +++ b/test/unit/headers.test.ts @@ -87,6 +87,20 @@ describe("Headers", () => { ).rejects.toThrow(/Malformed/); }); + it("should not include command output in parse errors", async () => { + const command = printCommand("Authorization=Bearer SECRET-VALUE\nbad line"); + const error = await getHeaders("localhost", command, logger).then( + () => { + throw new Error("expected getHeaders to reject"); + }, + (e: Error) => e, + ); + expect(error.message).toMatch(/Malformed line 2/); + expect(error.message).not.toContain("SECRET-VALUE"); + expect(error.message).not.toContain("Authorization"); + expect(error.message).not.toContain("bad line"); + }); + it("should have access to environment variables", async () => { const coderUrl = "dev.coder.com"; await expect( From 53bed3e8a571e0763e53d85f488ee9d3ebcbbbeb Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Jul 2026 10:07:04 +0000 Subject: [PATCH 2/9] fix(logging): broaden HTTP log redaction for headers and bodies --- src/logging/formatters.ts | 103 +++++++++++++++++++++++++-- src/logging/httpLogger.ts | 15 +++- src/logging/utils.ts | 7 +- test/unit/logging/formatters.test.ts | 88 +++++++++++++++++++++++ test/unit/logging/httpLogger.test.ts | 65 +++++++++++++++++ test/unit/logging/utils.test.ts | 16 +++++ 6 files changed, 282 insertions(+), 12 deletions(-) diff --git a/src/logging/formatters.ts b/src/logging/formatters.ts index df262100e3..bb258a7b84 100644 --- a/src/logging/formatters.ts +++ b/src/logging/formatters.ts @@ -5,10 +5,27 @@ import { safeStringify } from "./utils"; import type { AxiosRequestConfig } from "axios"; const SENSITIVE_HEADERS = new Set([ - "Coder-Session-Token", - "Proxy-Authorization", + "authorization", + "coder-session-token", + "cookie", + "proxy-authorization", + "set-cookie", + "x-api-key", ]); +const SENSITIVE_BODY_FIELDS = 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 +51,16 @@ 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 = new Set(extraSensitiveNames.map((name) => name.toLowerCase())); const formattedHeaders = Object.entries(headers) .map(([key, value]) => { - if (SENSITIVE_HEADERS.has(key)) { - return `${key}: `; + const name = key.toLowerCase(); + if (SENSITIVE_HEADERS.has(name) || extra.has(name)) { + return `${key}: ${REDACTED}`; } const strValue = typeof value === "string" ? value : safeStringify(value); return `${key}: ${strValue}`; @@ -51,8 +73,77 @@ export function formatHeaders(headers: Record): string { export function formatBody(body: unknown): string { if (body) { - return safeStringify(body) ?? ""; + return safeStringify(redactBodyFields(body)) ?? ""; } else { return ""; } } + +function isSensitiveField(name: string): boolean { + return SENSITIVE_BODY_FIELDS.has(name.toLowerCase()); +} + +/** Returns a copy of the body with known credential fields redacted (objects, params, JSON/form strings). */ +function redactBodyFields( + value: unknown, + seen = new WeakSet(), +): unknown { + if (typeof value === "string") { + return redactStringBody(value, seen); + } + if (value instanceof URLSearchParams) { + const redacted = new URLSearchParams(); + for (const [key, entry] of value) { + redacted.append(key, isSensitiveField(key) ? REDACTED : entry); + } + return redacted; + } + if (Array.isArray(value)) { + if (seen.has(value)) { + return value; + } + seen.add(value); + return value.map((entry) => redactBodyFields(entry, seen)); + } + if (isPlainObject(value)) { + if (seen.has(value)) { + return value; + } + seen.add(value); + return Object.fromEntries( + Object.entries(value).map(([key, entry]) => [ + key, + isSensitiveField(key) ? REDACTED : redactBodyFields(entry, seen), + ]), + ); + } + return value; +} + +function redactStringBody(body: string, seen: WeakSet): unknown { + const trimmed = body.trim(); + if (trimmed.startsWith("{") || trimmed.startsWith("[")) { + try { + return redactBodyFields(JSON.parse(trimmed), seen); + } catch { + // Not JSON; fall through. + } + } + if (trimmed.includes("=")) { + const params = new URLSearchParams(trimmed); + for (const key of params.keys()) { + if (isSensitiveField(key)) { + return redactBodyFields(params, seen); + } + } + } + return body; +} + +function isPlainObject(value: unknown): value is Record { + if (typeof value !== "object" || value === null) { + return false; + } + const proto: unknown = Object.getPrototypeOf(value); + return proto === Object.prototype || proto === null; +} diff --git a/src/logging/httpLogger.ts b/src/logging/httpLogger.ts index 3c5d782294..f6178eb642 100644 --- a/src/logging/httpLogger.ts +++ b/src/logging/httpLogger.ts @@ -46,7 +46,7 @@ export function logRequest( const msg = [ `→ ${shortId(requestId)} ${method} ${url} ${requestSize}`, - ...buildExtraLogs(config.headers, config.data, logLevel), + ...buildExtraLogs(config.headers, config.data, logLevel, config), ]; logger.trace(msg.join("\n")); } @@ -69,7 +69,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, + ), ]; logger.trace(msg.join("\n")); } @@ -110,6 +115,7 @@ export function logError( error.response.headers, error.response.data, logLevel, + config, ); } else { if (errorParts.length === 0) { @@ -120,6 +126,7 @@ export function logError( error?.config?.headers ?? {}, error.config?.data, logLevel, + config, ); } @@ -134,10 +141,12 @@ function buildExtraLogs( headers: Record, body: unknown, logLevel: HttpClientLogLevel, + config: RequestConfigWithMeta | 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, config?.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/test/unit/logging/formatters.test.ts b/test/unit/logging/formatters.test.ts index 1cd4fedfa6..eb60fd1977 100644 --- a/test/unit/logging/formatters.test.ts +++ b/test/unit/logging/formatters.test.ts @@ -81,6 +81,35 @@ describe("Logging formatters", () => { }); }); + it("redacts sensitive headers regardless of casing", () => { + const sensitiveHeaders = [ + "authorization", + "AUTHORIZATION", + "coder-session-token", + "cookie", + "set-cookie", + "SET-COOKIE", + "X-Api-Key", + "proxy-authorization", + ]; + + sensitiveHeaders.forEach((header) => { + const result = formatHeaders({ [header]: "secret-value" }); + expect(result).toContain(`${header}: `); + expect(result).not.toContain("secret-value"); + }); + }); + + 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 +147,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).not.toContain("secret-"); + expect(result).toContain("access_token: ''"); + expect(result).toContain("token_type: 'bearer'"); + }); + + it("redacts sensitive fields in nested objects and arrays", () => { + const result = formatBody({ + data: { session: { TOKEN: "secret-value" } }, + items: [{ password: "secret-value" }], + }); + expect(result).not.toContain("secret-value"); + expect(result).toContain("TOKEN: ''"); + expect(result).toContain("password: ''"); + }); + + it("redacts sensitive fields in URLSearchParams", () => { + const params = new URLSearchParams({ + grant_type: "refresh_token", + refresh_token: "secret-value", + }); + const result = formatBody(params); + expect(result).not.toContain("secret-value"); + expect(result).toContain("refresh_token"); + expect(result).toContain(""); + }); + + it("redacts sensitive fields in serialized bodies", () => { + const json = formatBody( + JSON.stringify({ access_token: "secret-value", expires_in: 3600 }), + ); + expect(json).not.toContain("secret-value"); + expect(json).toContain("expires_in"); + + const form = formatBody( + "grant_type=authorization_code&code=secret-value", + ); + expect(form).not.toContain("secret-value"); + expect(form).toContain("grant_type"); + }); + + 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..974d54ae1e 100644 --- a/test/unit/logging/httpLogger.test.ts +++ b/test/unit/logging/httpLogger.test.ts @@ -109,4 +109,69 @@ describe("REST HTTP Logger", () => { expect(logger.error).toHaveBeenCalledWith("Request error", error); }); }); + + describe("redaction", () => { + function makeConfig(): RequestConfigWithMeta { + return { + 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; + } + + function loggedText(fn: ReturnType): string { + return fn.mock.calls.flat().map(String).join("\n"); + } + + it("redacts sensitive request headers and body fields", () => { + const logger = createMockLogger(); + + logRequest(logger, makeConfig(), HttpClientLogLevel.BODY); + + const logged = loggedText(vi.mocked(logger.trace)); + expect(logged).not.toContain("request-secret"); + expect(logged).not.toContain("command-secret"); + expect(logged).not.toContain("body-secret"); + expect(logged).toContain("authorization: "); + expect(logged).toContain("X-From-Command: "); + }); + + it("redacts sensitive headers and body fields on error paths", () => { + const logger = createMockLogger(); + const error = new AxiosError("Bad Request"); + error.config = makeConfig(); + 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 = loggedText(vi.mocked(logger.error)); + expect(logged).not.toContain("response-secret"); + expect(logged).not.toContain("body-secret"); + expect(logged).toContain("set-cookie: "); + expect(logged).toContain("invalid_grant"); + }); + + it("redacts header-command headers on network error paths", () => { + const logger = createMockLogger(); + const error = new AxiosError("Network Error", "ECONNREFUSED"); + error.config = makeConfig(); + + logError(logger, error, HttpClientLogLevel.BODY); + + const logged = loggedText(vi.mocked(logger.error)); + expect(logged).not.toContain("request-secret"); + expect(logged).not.toContain("command-secret"); + expect(logged).not.toContain("body-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", () => { From 7bac1e4bfc060c01ecd14396d1e7ad1752ed6b3a Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Jul 2026 10:08:52 +0000 Subject: [PATCH 3/9] fix(auth): revoke OAuth tokens at the server during logout --- src/deployment/deploymentManager.ts | 4 ++ src/oauth/sessionManager.ts | 19 ++++++++ test/mocks/testHelpers.ts | 1 + .../unit/deployment/deploymentManager.test.ts | 31 +++++++++++++ test/unit/oauth/sessionManager.test.ts | 44 +++++++++++++++++++ 5 files changed, 99 insertions(+) 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/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index b8ef0ee003..38b781606b 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -455,6 +455,25 @@ export class OAuthSessionManager implements vscode.Disposable { ); } + /** 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; + } + const revoke = (token: string, hint: "access_token" | "refresh_token") => + this.revokeToken(storedTokens.access_token, token, hint).catch((error) => + this.logger.warn(`Best-effort ${hint} revocation failed:`, error), + ); + if (storedTokens.refresh_token) { + await revoke(storedTokens.refresh_token, "refresh_token"); + } + await revoke(storedTokens.access_token, "access_token"); + } + /** * Revoke a token using the OAuth server's revocation endpoint. * diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index b03d3daa14..b8a01b5b54 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -874,6 +874,7 @@ export class MockOAuthSessionManager { .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/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/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index 6c25d9cad7..a4e7c7e2dc 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -441,6 +441,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 {}; + }, + }); + + 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(); + + await expect(manager.revokeTokens()).resolves.toBeUndefined(); + expect(mockAdapter).not.toHaveBeenCalled(); + }); + }); + describe("scope validation", () => { it("rejects tokens with insufficient scopes", async () => { const { secretsManager, manager } = createTestContext(); From b392eb93cd51c8daf0f73a3bbc3e42dc4d6a78e9 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Jul 2026 10:10:19 +0000 Subject: [PATCH 4/9] feat(commands): confirm support bundle contents before collecting --- src/commands.ts | 26 ++++++++++++++++++++ test/unit/commands.supportBundle.test.ts | 31 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/src/commands.ts b/src/commands.ts index e058baf2ab..896cd60590 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 logs and workspace connection logs from recent VS Code windows", + "\u2022 Remote SSH extension logs", + "\u2022 Locally recorded telemetry entries", + "\u2022 A snapshot of the 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), diff --git a/test/unit/commands.supportBundle.test.ts b/test/unit/commands.supportBundle.test.ts index 1cee6dadd6..91aeaffbcd 100644 --- a/test/unit/commands.supportBundle.test.ts +++ b/test/unit/commands.supportBundle.test.ts @@ -66,6 +66,10 @@ function setup(options: { cliVersion?: string } = {}) { vi.mocked(vscode.window.showSaveDialog).mockResolvedValue( vscode.Uri.file(OUTPUT_PATH), ); + // Accept the collection disclosure dialog by default. + vi.mocked(vscode.window.showInformationMessage).mockResolvedValue( + "Continue" as unknown as vscode.MessageItem, + ); vi.mocked(cliExec.version).mockResolvedValue(options.cliVersion ?? "v2.36.0"); vi.mocked(cliExec.supportBundle).mockResolvedValue(undefined); vi.mocked(getRemoteServerDataPath).mockResolvedValue({ @@ -234,4 +238,31 @@ describe("Commands.supportBundle", () => { expect.objectContaining({ workspaceFiles: [] }), ); }); + + it("describes the collected data before creating the bundle", async () => { + const { commands } = setup(); + + await commands.supportBundle(agentItem("dev")); + + expect(vscode.window.showInformationMessage).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ + modal: true, + detail: expect.stringContaining("telemetry"), + }), + "Continue", + ); + }); + + it("does not create a bundle when the disclosure dialog is dismissed", async () => { + const { commands } = setup(); + vi.mocked(vscode.window.showInformationMessage).mockResolvedValue( + undefined, + ); + + await commands.supportBundle(agentItem("dev")); + + expect(vscode.window.showSaveDialog).not.toHaveBeenCalled(); + expect(cliExec.supportBundle).not.toHaveBeenCalled(); + }); }); From a94df28d0e5317927370d3d14c409468e9c17a86 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Jul 2026 10:11:28 +0000 Subject: [PATCH 5/9] fix(remote): delete file-based auth after migrating to secret storage --- src/remote/remote.ts | 13 +++ test/unit/remote/remote.migration.test.ts | 130 ++++++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 test/unit/remote/remote.migration.test.ts diff --git a/src/remote/remote.ts b/src/remote/remote.ts index c799ef8aef..0499c22dfd 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -832,6 +832,19 @@ export class Remote { } catch (error) { this.logger.warn("Failed to migrate session auth from files:", error); } + // Drop the plaintext copies even on failure: a rejected pair names + // another deployment, and the CLI config is rewritten on connect. + await Promise.all( + [urlPath, tokenPath].map((filePath) => + fs.rm(filePath, { force: true }).catch((error) => { + this.logger.warn( + "Failed to remove migrated auth file", + filePath, + error, + ); + }), + ), + ); } } diff --git a/test/unit/remote/remote.migration.test.ts b/test/unit/remote/remote.migration.test.ts new file mode 100644 index 0000000000..2b7c5098b8 --- /dev/null +++ b/test/unit/remote/remote.migration.test.ts @@ -0,0 +1,130 @@ +import { vol } from "memfs"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { PathResolver } from "@/core/pathResolver"; +import { Remote } from "@/remote/remote"; + +import { createTestTelemetryService } from "../../mocks/telemetry"; +import { + createMockLogger, + MockConfigurationProvider, +} from "../../mocks/testHelpers"; + +import type * as nodeFs from "node:fs"; +import type * as vscode from "vscode"; + +import type { Commands } from "@/commands"; +import type { ServiceContainer } from "@/core/container"; +import type { SecretsManager, 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`; + +interface MigratableRemote { + migrateToSecretsStorage(safeHostname: string): Promise; +} + +function setup(options: { existingAuth?: SessionAuth } = {}) { + vi.clearAllMocks(); + vol.reset(); + new MockConfigurationProvider(); + + const logger = createMockLogger(); + const pathResolver = new PathResolver(BASE_PATH, "/logs/code"); + const secretsManager: Pick< + SecretsManager, + "getSessionAuth" | "setSessionAuth" + > = { + getSessionAuth: vi.fn(() => Promise.resolve(options.existingAuth)), + setSessionAuth: vi.fn(() => Promise.resolve()), + }; + + const serviceContainer = { + getLogger: () => logger, + getPathResolver: () => pathResolver, + getCliManager: () => ({}), + getContextManager: () => ({}), + getSecretsManager: () => secretsManager, + getLoginCoordinator: () => ({}), + getTelemetryService: () => createTestTelemetryService(), + } as unknown as ServiceContainer; + + const remote = new Remote( + serviceContainer, + {} as Commands, + {} as vscode.ExtensionContext, + ) as unknown as MigratableRemote; + + return { remote, secretsManager }; +} + +function writeLegacyFiles(): void { + vol.fromJSON({ + [URL_PATH]: "https://dep.example.com\n", + [TOKEN_PATH]: "legacy-token\n", + }); +} + +describe("Remote session auth migration", () => { + beforeEach(() => { + vol.reset(); + }); + + it("moves file-based auth into secret storage and deletes the files", async () => { + const { remote, secretsManager } = setup(); + writeLegacyFiles(); + + await remote.migrateToSecretsStorage(HOSTNAME); + + 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 { remote, secretsManager } = setup(); + vi.mocked(secretsManager.setSessionAuth).mockRejectedValue( + new Error("Session auth hostname mismatch"), + ); + writeLegacyFiles(); + + await remote.migrateToSecretsStorage(HOSTNAME); + + 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 { remote, secretsManager } = setup({ + existingAuth: { url: "https://dep.example.com", token: "current" }, + }); + writeLegacyFiles(); + + await remote.migrateToSecretsStorage(HOSTNAME); + + 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 { remote, secretsManager } = setup(); + + await remote.migrateToSecretsStorage(HOSTNAME); + + expect(secretsManager.setSessionAuth).not.toHaveBeenCalled(); + }); +}); From fba60480770eff365370bd9ee6fed2cf0bea9425 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Jul 2026 10:16:57 +0000 Subject: [PATCH 6/9] fix(auth): surface credential cleanup failures during logout --- src/commands.ts | 18 ++++++- src/core/cliCredentialManager.ts | 52 +++++++++++++++------ src/core/cliManager.ts | 17 ++++--- src/instrumentation/auth.ts | 3 +- src/instrumentation/credentials.ts | 16 ++++--- test/mocks/testHelpers.ts | 2 +- test/unit/commands.telemetry.test.ts | 38 ++++++++++++++- test/unit/core/cliCredentialManager.test.ts | 15 +++--- test/unit/core/cliManager.test.ts | 29 ++++++++---- 9 files changed, 143 insertions(+), 47 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 896cd60590..297d5a26f8 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -67,6 +67,7 @@ import type { } from "coder/site/src/api/typesGenerated"; import type { CoderApi } from "./api/coderApi"; +import type { CredentialStore } from "./core/cliCredentialManager"; import type { CliManager } from "./core/cliManager"; import type { ServiceContainer } from "./core/container"; import type { MementoManager } from "./core/mementoManager"; @@ -622,8 +623,12 @@ export class Commands { await this.deploymentManager.clearDeployment("logout"); if (deployment) { - await this.cliManager.clearCredentials(deployment.url); + const cleanup = await this.cliManager.clearCredentials(deployment.url); await this.secretsManager.clearAllAuthData(deployment.safeHostname); + if (cleanup.failed.length > 0) { + this.showLogoutCleanupWarning(cleanup.failed); + return { success: false, reason: "cleanup_incomplete" }; + } } this.showLogoutMessage(); @@ -631,6 +636,17 @@ export class Commands { return { success: true }; } + private showLogoutCleanupWarning(failed: readonly CredentialStore[]): void { + const labels: Record = { + cli: "CLI session", + files: "credential files", + }; + const stores = failed.map((store) => labels[store]).join(", "); + vscode.window.showWarningMessage( + `You've been logged out of Coder, but some stored credentials could not be removed: ${stores}. Log out again to retry, or run "coder logout" with the Coder CLI.`, + ); + } + private showLogoutMessage(): void { vscode.window .showInformationMessage("You've been logged out of Coder!", "Login") diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 763d12d65f..df6e0f4076 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -38,6 +38,14 @@ export interface CliCredential { source: "keyring" | "files"; } +/** Credential stores that logout cleanup can fail to clear. */ +export type CredentialStore = "cli" | "files"; + +export interface CredentialClearResult { + /** Stores that could not be confirmed cleared. Empty on full success. */ + failed: CredentialStore[]; +} + const EXEC_TIMEOUT_MS = 60_000; const EXEC_LOG_INTERVAL_MS = 5_000; @@ -171,31 +179,41 @@ 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). + * Reports which stores could not be 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 }), ]); + const failed: CredentialStore[] = []; + if (!cliCleared) { + failed.push("cli"); + } + if (!filesCleared) { + failed.push("files"); + } + return { failed }; }); } /** * 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 +221,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 +233,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 +241,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 +331,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..1f218e52e5 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -39,7 +39,10 @@ import type { Logger } from "../logging/logger"; import type { TelemetryService } from "../telemetry/service"; import type { Span } from "../telemetry/span"; -import type { CliCredentialManager } from "./cliCredentialManager"; +import type { + CliCredentialManager, + CredentialClearResult, +} from "./cliCredentialManager"; import type { PathResolver } from "./pathResolver"; type ResolvedBinary = @@ -1061,9 +1064,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; reports which + * stores could not be 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 +1080,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 { failed: ["cli"] }; } + this.output.warn("Failed to remove credentials:", result.error); + return { failed: ["cli", "files"] }; } private handleStoreError(error: unknown): void { 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/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index b8a01b5b54..b3026fe49a 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -472,7 +472,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({ failed: [] }), } as unknown as CliCredentialManager; } diff --git a/test/unit/commands.telemetry.test.ts b/test/unit/commands.telemetry.test.ts index 629efdf144..b9e6d8e5bc 100644 --- a/test/unit/commands.telemetry.test.ts +++ b/test/unit/commands.telemetry.test.ts @@ -45,6 +45,9 @@ interface SetupOptions { readonly authenticated?: boolean; readonly loginResult?: LoginResultForTest; readonly clearAllAuthDataError?: Error; + readonly clearCredentialsResult?: Awaited< + ReturnType + >; } function setup(options: SetupOptions = {}) { @@ -83,7 +86,9 @@ function setup(options: SetupOptions = {}) { }; const cliManager: Pick = { - clearCredentials: vi.fn(() => Promise.resolve()), + clearCredentials: vi.fn(() => + Promise.resolve(options.clearCredentialsResult ?? { failed: [] }), + ), }; const secretsManager: Pick< @@ -273,5 +278,36 @@ describe("Commands", () => { error: { message: "secret clear failed" }, }); }); + + it("reports incomplete credential cleanup instead of success", async () => { + const { commands, sink } = setup({ + authenticated: true, + clearCredentialsResult: { failed: ["cli"] }, + }); + // Recreated after setup so this instance records the dialog calls. + const interaction = new MockUserInteraction(); + + await commands.logout(); + + expect(sink.expectOne("auth.logout")).toMatchObject({ + properties: { + result: "aborted", + reason: "cleanup_incomplete", + }, + }); + const messages = interaction.getMessageCalls(); + expect( + messages.some( + (call) => + call.message.includes("could not be removed") && + call.message.includes("CLI session"), + ), + ).toBe(true); + expect( + messages.some((call) => + call.message.includes("You've been logged out of Coder!"), + ), + ).toBe(false); + }); }); }); diff --git a/test/unit/core/cliCredentialManager.test.ts b/test/unit/core/cliCredentialManager.test.ts index 15b90bc311..e714c29949 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).toEqual({ failed: [] }); expect(resolver).toHaveBeenCalledWith(TEST_URL); const exec = lastExecArgs(); expect(exec.bin).toBe(TEST_BIN); @@ -580,9 +581,9 @@ 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.toEqual({ + failed: ["cli"], + }); expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { "error.type": "cli", @@ -595,9 +596,9 @@ 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.toEqual({ + failed: ["cli"], + }); 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..3ae912ecab 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, failed: [] }, + { + scenario: "fails", + error: new Error("unexpected failure"), + failed: ["cli", "files"], + }, + { scenario: "is cancelled", error: makeAbortError(), failed: ["cli"] }, + ])( + "should report cleanup state when deleteToken $scenario", + async ({ error, failed }) => { + const { manager, mockCredManager } = setupCliManager(); + if (error) { + vi.mocked(mockCredManager.deleteToken).mockRejectedValueOnce(error); + } + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ + failed, + }); + }, + ); }); describe("Binary Version Validation", () => { From beb2a58cc2108d1d15c1ac93c81c8456a016dcc8 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Jul 2026 19:58:57 +0300 Subject: [PATCH 7/9] refactor: simplify credential cleanup reporting and test helpers Consolidate OAuth revocation into a single revokeTokens() operation (one discovery fetch and client instead of two) and drop the now-unused revokeRefreshToken(). Replace the per-store CredentialClearResult with a boolean since the only consumer checks overall success and the store lists were fabricated on cancel/error paths; the failing store remains in the logs. Narrow buildExtraLogs to take headerCommandKeys directly. Also DRY the new tests: shared loggedOutput() helper for scanning mock logger output, merged duplicate header-redaction cases, and simpler rejection/message assertions. --- src/commands.ts | 20 +--- src/core/cliCredentialManager.ts | 21 +--- src/core/cliManager.ts | 17 ++- src/logging/httpLogger.ts | 17 +-- src/oauth/sessionManager.ts | 110 +++++++------------- test/mocks/testHelpers.ts | 20 ++-- test/unit/command/exec.test.ts | 15 +-- test/unit/commands.telemetry.test.ts | 37 +++---- test/unit/core/cliCredentialManager.test.ts | 10 +- test/unit/core/cliManager.test.ts | 14 +-- test/unit/headers.test.ts | 12 +-- test/unit/logging/formatters.test.ts | 13 +-- test/unit/logging/httpLogger.test.ts | 48 ++++----- test/unit/oauth/sessionManager.test.ts | 19 ---- test/unit/remote/remote.migration.test.ts | 6 +- test/unit/remote/remote.test.ts | 4 +- 16 files changed, 136 insertions(+), 247 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 297d5a26f8..31ecb17674 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -67,7 +67,6 @@ import type { } from "coder/site/src/api/typesGenerated"; import type { CoderApi } from "./api/coderApi"; -import type { CredentialStore } from "./core/cliCredentialManager"; import type { CliManager } from "./core/cliManager"; import type { ServiceContainer } from "./core/container"; import type { MementoManager } from "./core/mementoManager"; @@ -623,10 +622,12 @@ export class Commands { await this.deploymentManager.clearDeployment("logout"); if (deployment) { - const cleanup = await this.cliManager.clearCredentials(deployment.url); + const cleared = await this.cliManager.clearCredentials(deployment.url); await this.secretsManager.clearAllAuthData(deployment.safeHostname); - if (cleanup.failed.length > 0) { - this.showLogoutCleanupWarning(cleanup.failed); + if (!cleared) { + vscode.window.showWarningMessage( + 'You\'ve been logged out of Coder, but some stored credentials could not be removed. Log out again to retry, or run "coder logout" with the Coder CLI.', + ); return { success: false, reason: "cleanup_incomplete" }; } } @@ -636,17 +637,6 @@ export class Commands { return { success: true }; } - private showLogoutCleanupWarning(failed: readonly CredentialStore[]): void { - const labels: Record = { - cli: "CLI session", - files: "credential files", - }; - const stores = failed.map((store) => labels[store]).join(", "); - vscode.window.showWarningMessage( - `You've been logged out of Coder, but some stored credentials could not be removed: ${stores}. Log out again to retry, or run "coder logout" with the Coder CLI.`, - ); - } - private showLogoutMessage(): void { vscode.window .showInformationMessage("You've been logged out of Coder!", "Login") diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index df6e0f4076..f6299f58fc 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -38,14 +38,6 @@ export interface CliCredential { source: "keyring" | "files"; } -/** Credential stores that logout cleanup can fail to clear. */ -export type CredentialStore = "cli" | "files"; - -export interface CredentialClearResult { - /** Stores that could not be confirmed cleared. Empty on full success. */ - failed: CredentialStore[]; -} - const EXEC_TIMEOUT_MS = 60_000; const EXEC_LOG_INTERVAL_MS = 5_000; @@ -180,27 +172,20 @@ 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). - * Reports which stores could not be cleared instead of throwing, except + * 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) => { const [filesCleared, cliCleared] = await Promise.all([ this.deleteCredentialFiles(url), this.cliLogout(url, configs, { signal: options?.signal, span }), ]); - const failed: CredentialStore[] = []; - if (!cliCleared) { - failed.push("cli"); - } - if (!filesCleared) { - failed.push("files"); - } - return { failed }; + return filesCleared && cliCleared; }); } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 1f218e52e5..19b54cebce 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -39,10 +39,7 @@ import type { Logger } from "../logging/logger"; import type { TelemetryService } from "../telemetry/service"; import type { Span } from "../telemetry/span"; -import type { - CliCredentialManager, - CredentialClearResult, -} from "./cliCredentialManager"; +import type { CliCredentialManager } from "./cliCredentialManager"; import type { PathResolver } from "./pathResolver"; type ResolvedBinary = @@ -1064,10 +1061,10 @@ export class CliManager { /** * Remove credentials for a deployment. Clears both file-based credentials - * and keyring entries (via `coder logout`). Never throws; reports which - * stores could not be cleared. + * 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 }) => @@ -1084,10 +1081,10 @@ export class CliManager { } if (result.cancelled) { this.output.info("Credential removal cancelled by user"); - return { failed: ["cli"] }; + } else { + this.output.warn("Failed to remove credentials:", result.error); } - this.output.warn("Failed to remove credentials:", result.error); - return { failed: ["cli", "files"] }; + return false; } private handleStoreError(error: unknown): void { diff --git a/src/logging/httpLogger.ts b/src/logging/httpLogger.ts index f6178eb642..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, config), + ...buildExtraLogs( + config.headers, + config.data, + logLevel, + config.headerCommandKeys, + ), ]; logger.trace(msg.join("\n")); } @@ -73,7 +78,7 @@ export function logResponse( response.headers, response.data, logLevel, - response.config, + response.config.headerCommandKeys, ), ]; logger.trace(msg.join("\n")); @@ -115,7 +120,7 @@ export function logError( error.response.headers, error.response.data, logLevel, - config, + config?.headerCommandKeys, ); } else { if (errorParts.length === 0) { @@ -126,7 +131,7 @@ export function logError( error?.config?.headers ?? {}, error.config?.data, logLevel, - config, + config?.headerCommandKeys, ); } @@ -141,12 +146,12 @@ function buildExtraLogs( headers: Record, body: unknown, logLevel: HttpClientLogLevel, - config: RequestConfigWithMeta | undefined, + headerCommandKeys: readonly string[] | undefined, ) { const msg = []; if (logLevel >= HttpClientLogLevel.HEADERS) { // Headers applied by the header command are treated as sensitive too. - msg.push(formatHeaders(headers, config?.headerCommandKeys ?? [])); + msg.push(formatHeaders(headers, headerCommandKeys ?? [])); } if (logLevel >= HttpClientLogLevel.BODY) { msg.push(formatBody(body)); diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 38b781606b..2ff4353d43 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -441,20 +441,6 @@ 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"); - return; - } - - await this.revokeToken( - storedTokens.access_token, - storedTokens.refresh_token, - "refresh_token", - ); - } - /** 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) => { @@ -464,64 +450,48 @@ export class OAuthSessionManager implements vscode.Disposable { if (!storedTokens) { return; } - const revoke = (token: string, hint: "access_token" | "refresh_token") => - this.revokeToken(storedTokens.access_token, token, hint).catch((error) => - this.logger.warn(`Best-effort ${hint} revocation failed:`, 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 available, skipping revocation", + ); + return; + } + const revoke = async ( + token: string, + hint: "access_token" | "refresh_token", + ) => { + const params: OAuth2TokenRevocationRequest = { + token, + client_id: registration.client_id, + client_secret: registration.client_secret, + token_type_hint: hint, + }; + try { + await axiosInstance.post(endpoint, toUrlSearchParams(params), { + headers: { + "Content-Type": "application/x-www-form-urlencoded", + }, + }); + this.logger.debug("Token revocation successful"); + } catch (error) { + this.logger.warn(`Best-effort ${hint} revocation failed:`, error); + } + }; + // Revoke the refresh token while the access token still authenticates. + if (storedTokens.refresh_token) { + await revoke(storedTokens.refresh_token, "refresh_token"); + } + await revoke(storedTokens.access_token, "access_token"); + }, ); - if (storedTokens.refresh_token) { - await revoke(storedTokens.refresh_token, "refresh_token"); + } catch (error) { + this.logger.warn("Token revocation failed:", error); } - await revoke(storedTokens.access_token, "access_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, - }; - - 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; - } - }, - ); } /** diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index b3026fe49a..f3ffee2a95 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -472,7 +472,7 @@ export function createMockCliCredentialManager(): CliCredentialManager { return { storeToken: vi.fn().mockResolvedValue(undefined), readToken: vi.fn().mockResolvedValue(undefined), - deleteToken: vi.fn().mockResolvedValue({ failed: [] }), + deleteToken: vi.fn().mockResolvedValue(true), } as unknown as CliCredentialManager; } @@ -493,12 +493,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 +535,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 +880,6 @@ 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); diff --git a/test/unit/command/exec.test.ts b/test/unit/command/exec.test.ts index 67b9d9964f..3f6ab751cc 100644 --- a/test/unit/command/exec.test.ts +++ b/test/unit/command/exec.test.ts @@ -1,8 +1,8 @@ -import { describe, expect, it, vi } from "vitest"; +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, @@ -60,7 +60,7 @@ describe("execCommand", () => { }); it("should not log the command or its output", async () => { - const quietLogger = createMockLogger(); + const quietLogger = new LogCollector(); await execCommand(printCommand("quiet-output-value"), quietLogger, { title: "Test", }); @@ -70,13 +70,6 @@ describe("execCommand", () => { { title: "Test" }, ); - const logged = [ - ...vi.mocked(quietLogger.debug).mock.calls, - ...vi.mocked(quietLogger.warn).mock.calls, - ] - .flat() - .map(String) - .join("\n"); - expect(logged).not.toContain("quiet-output-value"); + expect(quietLogger.text).not.toContain("quiet-output-value"); }); }); diff --git a/test/unit/commands.telemetry.test.ts b/test/unit/commands.telemetry.test.ts index b9e6d8e5bc..8f1022dcf1 100644 --- a/test/unit/commands.telemetry.test.ts +++ b/test/unit/commands.telemetry.test.ts @@ -45,14 +45,12 @@ interface SetupOptions { readonly authenticated?: boolean; readonly loginResult?: LoginResultForTest; readonly clearAllAuthDataError?: Error; - readonly clearCredentialsResult?: Awaited< - ReturnType - >; + 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(); @@ -87,7 +85,7 @@ function setup(options: SetupOptions = {}) { const cliManager: Pick = { clearCredentials: vi.fn(() => - Promise.resolve(options.clearCredentialsResult ?? { failed: [] }), + Promise.resolve(options.clearCredentialsResult ?? true), ), }; @@ -130,6 +128,7 @@ function setup(options: SetupOptions = {}) { return { commands, sink, + interaction, mocks: { cliManager, deploymentManager, loginCoordinator, secretsManager }, }; } @@ -280,12 +279,10 @@ describe("Commands", () => { }); it("reports incomplete credential cleanup instead of success", async () => { - const { commands, sink } = setup({ + const { commands, sink, interaction } = setup({ authenticated: true, - clearCredentialsResult: { failed: ["cli"] }, + clearCredentialsResult: false, }); - // Recreated after setup so this instance records the dialog calls. - const interaction = new MockUserInteraction(); await commands.logout(); @@ -295,19 +292,15 @@ describe("Commands", () => { reason: "cleanup_incomplete", }, }); - const messages = interaction.getMessageCalls(); - expect( - messages.some( - (call) => - call.message.includes("could not be removed") && - call.message.includes("CLI session"), - ), - ).toBe(true); - expect( - messages.some((call) => - call.message.includes("You've been logged out of Coder!"), - ), - ).toBe(false); + const messages = interaction + .getMessageCalls() + .map((call) => call.message); + expect(messages).toContainEqual( + expect.stringContaining("could not be removed"), + ); + 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 e714c29949..e02d45f4b4 100644 --- a/test/unit/core/cliCredentialManager.test.ts +++ b/test/unit/core/cliCredentialManager.test.ts @@ -543,7 +543,7 @@ describe("CliCredentialManager", () => { const result = await manager.deleteToken(TEST_URL, configs); - expect(result).toEqual({ failed: [] }); + expect(result).toBe(true); expect(resolver).toHaveBeenCalledWith(TEST_URL); const exec = lastExecArgs(); expect(exec.bin).toBe(TEST_BIN); @@ -581,9 +581,7 @@ describe("CliCredentialManager", () => { stubExecFile({ error: "logout failed" }); const { manager, sink } = setup(); - await expect(manager.deleteToken(TEST_URL, configs)).resolves.toEqual({ - failed: ["cli"], - }); + await expect(manager.deleteToken(TEST_URL, configs)).resolves.toBe(false); expect(sink.expectOne("auth.credential.clear")).toMatchObject({ properties: { "error.type": "cli", @@ -596,9 +594,7 @@ describe("CliCredentialManager", () => { vi.mocked(isKeyringEnabled).mockReturnValue(true); const { manager, sink } = setup(failingResolver()); - await expect(manager.deleteToken(TEST_URL, configs)).resolves.toEqual({ - failed: ["cli"], - }); + 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 3ae912ecab..c150c1649c 100644 --- a/test/unit/core/cliManager.test.ts +++ b/test/unit/core/cliManager.test.ts @@ -340,23 +340,23 @@ describe("CliManager", () => { }); it.each([ - { scenario: "succeeds", error: undefined, failed: [] }, + { scenario: "succeeds", error: undefined, cleared: true }, { scenario: "fails", error: new Error("unexpected failure"), - failed: ["cli", "files"], + cleared: false, }, - { scenario: "is cancelled", error: makeAbortError(), failed: ["cli"] }, + { scenario: "is cancelled", error: makeAbortError(), cleared: false }, ])( "should report cleanup state when deleteToken $scenario", - async ({ error, failed }) => { + async ({ error, cleared }) => { const { manager, mockCredManager } = setupCliManager(); if (error) { vi.mocked(mockCredManager.deleteToken).mockRejectedValueOnce(error); } - await expect(manager.clearCredentials(CLEAR_URL)).resolves.toEqual({ - failed, - }); + await expect(manager.clearCredentials(CLEAR_URL)).resolves.toBe( + cleared, + ); }, ); }); diff --git a/test/unit/headers.test.ts b/test/unit/headers.test.ts index 9f3dcf54c4..ec00fc5ce6 100644 --- a/test/unit/headers.test.ts +++ b/test/unit/headers.test.ts @@ -89,16 +89,10 @@ describe("Headers", () => { it("should not include command output in parse errors", async () => { const command = printCommand("Authorization=Bearer SECRET-VALUE\nbad line"); - const error = await getHeaders("localhost", command, logger).then( - () => { - throw new Error("expected getHeaders to reject"); - }, - (e: Error) => e, + // 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$/, ); - expect(error.message).toMatch(/Malformed line 2/); - expect(error.message).not.toContain("SECRET-VALUE"); - expect(error.message).not.toContain("Authorization"); - expect(error.message).not.toContain("bad line"); }); it("should have access to environment variables", async () => { diff --git a/test/unit/logging/formatters.test.ts b/test/unit/logging/formatters.test.ts index eb60fd1977..81ae8f1dee 100644 --- a/test/unit/logging/formatters.test.ts +++ b/test/unit/logging/formatters.test.ts @@ -71,26 +71,17 @@ describe("Logging formatters", () => { expect(result).toContain("accept: text/html"); }); - it("redacts sensitive headers", () => { - const sensitiveHeaders = ["Coder-Session-Token", "Proxy-Authorization"]; - - sensitiveHeaders.forEach((header) => { - const result = formatHeaders({ [header]: "secret-value" }); - expect(result).toContain(`${header}: `); - expect(result).not.toContain("secret-value"); - }); - }); - 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", + "Proxy-Authorization", ]; sensitiveHeaders.forEach((header) => { diff --git a/test/unit/logging/httpLogger.test.ts b/test/unit/logging/httpLogger.test.ts index 974d54ae1e..ba6b8d6445 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", () => { @@ -111,30 +111,24 @@ describe("REST HTTP Logger", () => { }); describe("redaction", () => { - function makeConfig(): RequestConfigWithMeta { - return { - 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; - } - - function loggedText(fn: ReturnType): string { - return fn.mock.calls.flat().map(String).join("\n"); - } + 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 = createMockLogger(); + const logger = new LogCollector(); - logRequest(logger, makeConfig(), HttpClientLogLevel.BODY); + logRequest(logger, config, HttpClientLogLevel.BODY); - const logged = loggedText(vi.mocked(logger.trace)); + const logged = logger.text; expect(logged).not.toContain("request-secret"); expect(logged).not.toContain("command-secret"); expect(logged).not.toContain("body-secret"); @@ -143,9 +137,9 @@ describe("REST HTTP Logger", () => { }); it("redacts sensitive headers and body fields on error paths", () => { - const logger = createMockLogger(); + const logger = new LogCollector(); const error = new AxiosError("Bad Request"); - error.config = makeConfig(); + error.config = config; error.response = { status: 400, headers: { "set-cookie": ["session=response-secret"] }, @@ -154,7 +148,7 @@ describe("REST HTTP Logger", () => { logError(logger, error, HttpClientLogLevel.BODY); - const logged = loggedText(vi.mocked(logger.error)); + const logged = logger.text; expect(logged).not.toContain("response-secret"); expect(logged).not.toContain("body-secret"); expect(logged).toContain("set-cookie: "); @@ -162,13 +156,13 @@ describe("REST HTTP Logger", () => { }); it("redacts header-command headers on network error paths", () => { - const logger = createMockLogger(); + const logger = new LogCollector(); const error = new AxiosError("Network Error", "ECONNREFUSED"); - error.config = makeConfig(); + error.config = config; logError(logger, error, HttpClientLogLevel.BODY); - const logged = loggedText(vi.mocked(logger.error)); + const logged = logger.text; expect(logged).not.toContain("request-secret"); expect(logged).not.toContain("command-secret"); expect(logged).not.toContain("body-secret"); diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index a4e7c7e2dc..ac6887076a 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -422,25 +422,6 @@ describe("OAuthSessionManager", () => { }); }); - describe("revokeRefreshToken", () => { - it("revokes token via revocation endpoint", async () => { - const { manager, setupForOAuthOperation } = createTestContext(); - - let revokedToken: string | undefined; - await setupForOAuthOperation({ - "/oauth2/revoke": (config: InternalAxiosRequestConfig) => { - const params = new URLSearchParams(config.data as string); - revokedToken = params.get("token") ?? undefined; - return {}; - }, - }); - - await manager.revokeRefreshToken(); - - expect(revokedToken).toBe("refresh-token"); - }); - }); - describe("revokeTokens", () => { it("revokes the refresh and access tokens", async () => { const { manager, setupForOAuthOperation } = createTestContext(); diff --git a/test/unit/remote/remote.migration.test.ts b/test/unit/remote/remote.migration.test.ts index 2b7c5098b8..4f34e51398 100644 --- a/test/unit/remote/remote.migration.test.ts +++ b/test/unit/remote/remote.migration.test.ts @@ -1,5 +1,5 @@ import { vol } from "memfs"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { PathResolver } from "@/core/pathResolver"; import { Remote } from "@/remote/remote"; @@ -76,10 +76,6 @@ function writeLegacyFiles(): void { } describe("Remote session auth migration", () => { - beforeEach(() => { - vol.reset(); - }); - it("moves file-based auth into secret storage and deletes the files", async () => { const { remote, secretsManager } = setup(); writeLegacyFiles(); 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: [ From c0bce07a465a1c0a7892fb21d6e33503a10563e2 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Jul 2026 21:38:04 +0300 Subject: [PATCH 8/9] refactor: address review feedback on redaction, revocation, and tests Type the sensitive-name sets as ReadonlySet> with a shared lowercase() helper so wrong-cased entries fail to compile, and make body redaction copy-on-write: clean bodies (the common case) pass through without allocation and clean strings are returned as-is after a single parse. Flatten revokeTokens into one operation with a token loop, extract the session-auth migration out of Remote into src/remote/migration.ts so its test needs no Remote or ServiceContainer stubbing, and tighten the new user-facing messages. Tests: MockUserInteraction now records dialog options and backs the support-bundle disclosure tests, redaction tests assert positives before one collapsed negative, and the migration test is renamed to match the extracted module. --- src/commands.ts | 8 +- src/logging/formatters.ts | 103 ++++++++++-------- src/oauth/sessionManager.ts | 28 +++-- src/remote/migration.ts | 93 ++++++++++++++++ src/remote/remote.ts | 74 ++----------- src/util.ts | 5 + test/mocks/testHelpers.ts | 8 +- test/unit/commands.supportBundle.test.ts | 28 ++--- test/unit/commands.telemetry.test.ts | 1 + test/unit/logging/formatters.test.ts | 10 +- test/unit/logging/httpLogger.test.ts | 15 +-- ...te.migration.test.ts => migration.test.ts} | 73 ++++--------- 12 files changed, 237 insertions(+), 209 deletions(-) create mode 100644 src/remote/migration.ts rename test/unit/remote/{remote.migration.test.ts => migration.test.ts} (52%) diff --git a/src/commands.ts b/src/commands.ts index 31ecb17674..8c38b42312 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -534,10 +534,10 @@ export class Commands { "A support bundle may contain sensitive information. It collects:", "", "\u2022 Deployment and workspace diagnostics", - "\u2022 Coder extension logs and workspace connection logs from recent VS Code windows", + "\u2022 Coder extension and connection logs from recent VS Code windows", "\u2022 Remote SSH extension logs", - "\u2022 Locally recorded telemetry entries", - "\u2022 A snapshot of the Coder extension settings", + "\u2022 Locally recorded telemetry", + "\u2022 Coder extension settings", "", "Review the bundle before sharing it.", ].join("\n"); @@ -626,7 +626,7 @@ export class Commands { await this.secretsManager.clearAllAuthData(deployment.safeHostname); if (!cleared) { vscode.window.showWarningMessage( - 'You\'ve been logged out of Coder, but some stored credentials could not be removed. Log out again to retry, or run "coder logout" with the Coder CLI.', + '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" }; } diff --git a/src/logging/formatters.ts b/src/logging/formatters.ts index bb258a7b84..b962b1dec1 100644 --- a/src/logging/formatters.ts +++ b/src/logging/formatters.ts @@ -1,10 +1,12 @@ import prettyBytes from "pretty-bytes"; +import { lowercase } from "../util"; + import { safeStringify } from "./utils"; import type { AxiosRequestConfig } from "axios"; -const SENSITIVE_HEADERS = new Set([ +const SENSITIVE_HEADERS: ReadonlySet> = new Set([ "authorization", "coder-session-token", "cookie", @@ -13,7 +15,8 @@ const SENSITIVE_HEADERS = new Set([ "x-api-key", ]); -const SENSITIVE_BODY_FIELDS = new Set([ +/** Credential fields from OAuth token requests/responses, logged at BODY level. */ +const SENSITIVE_BODY_FIELDS: ReadonlySet> = new Set([ "access_token", "client_secret", "code", @@ -55,10 +58,12 @@ export function formatHeaders( headers: Record, extraSensitiveNames: readonly string[] = [], ): string { - const extra = new Set(extraSensitiveNames.map((name) => name.toLowerCase())); + const extra: ReadonlySet> = new Set( + extraSensitiveNames.map(lowercase), + ); const formattedHeaders = Object.entries(headers) .map(([key, value]) => { - const name = key.toLowerCase(); + const name = lowercase(key); if (SENSITIVE_HEADERS.has(name) || extra.has(name)) { return `${key}: ${REDACTED}`; } @@ -79,11 +84,11 @@ export function formatBody(body: unknown): string { } } -function isSensitiveField(name: string): boolean { - return SENSITIVE_BODY_FIELDS.has(name.toLowerCase()); -} - -/** Returns a copy of the body with known credential fields redacted (objects, params, JSON/form strings). */ +/** + * 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(), @@ -92,58 +97,70 @@ function redactBodyFields( return redactStringBody(value, seen); } if (value instanceof URLSearchParams) { - const redacted = new URLSearchParams(); - for (const [key, entry] of value) { - redacted.append(key, isSensitiveField(key) ? REDACTED : entry); - } - return redacted; - } - if (Array.isArray(value)) { - if (seen.has(value)) { + const keys = [...value.keys()]; + if (!keys.some((key) => SENSITIVE_BODY_FIELDS.has(lowercase(key)))) { return value; } - seen.add(value); - return value.map((entry) => redactBodyFields(entry, seen)); - } - if (isPlainObject(value)) { - if (seen.has(value)) { - return value; - } - seen.add(value); - return Object.fromEntries( - Object.entries(value).map(([key, entry]) => [ + return new URLSearchParams( + [...value].map(([key, entry]) => [ key, - isSensitiveField(key) ? REDACTED : redactBodyFields(entry, seen), + SENSITIVE_BODY_FIELDS.has(lowercase(key)) ? REDACTED : entry, ]), ); } - return value; + 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 { - return redactBodyFields(JSON.parse(trimmed), seen); + 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); - for (const key of params.keys()) { - if (isSensitiveField(key)) { - return redactBodyFields(params, seen); - } - } + const redacted = redactBodyFields(params, seen); + return redacted === params ? body : redacted; } return body; } - -function isPlainObject(value: unknown): value is Record { - if (typeof value !== "object" || value === null) { - return false; - } - const proto: unknown = Object.getPrototypeOf(value); - return proto === Object.prototype || proto === null; -} diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 2ff4353d43..27fddeb121 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -450,26 +450,29 @@ export class OAuthSessionManager implements vscode.Disposable { if (!storedTokens) { 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 available, skipping revocation", - ); + this.logger.debug("No revocation endpoint; skipping revocation"); return; } - const revoke = async ( - token: string, - hint: "access_token" | "refresh_token", - ) => { + 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: hint, + token_type_hint, }; try { await axiosInstance.post(endpoint, toUrlSearchParams(params), { @@ -477,16 +480,11 @@ export class OAuthSessionManager implements vscode.Disposable { "Content-Type": "application/x-www-form-urlencoded", }, }); - this.logger.debug("Token revocation successful"); + this.logger.debug(`Revoked ${token_type_hint}`); } catch (error) { - this.logger.warn(`Best-effort ${hint} revocation failed:`, error); + this.logger.warn(`Failed to revoke ${token_type_hint}:`, error); } - }; - // Revoke the refresh token while the access token still authenticates. - if (storedTokens.refresh_token) { - await revoke(storedTokens.refresh_token, "refresh_token"); } - await revoke(storedTokens.access_token, "access_token"); }, ); } catch (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 0499c22dfd..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,72 +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); - } - // Drop the plaintext copies even on failure: a rejected pair names - // another deployment, and the CLI config is rewritten on connect. - await Promise.all( - [urlPath, tokenPath].map((filePath) => - fs.rm(filePath, { force: true }).catch((error) => { - this.logger.warn( - "Failed to remove migrated auth file", - filePath, - 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 f3ffee2a95..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)); }; diff --git a/test/unit/commands.supportBundle.test.ts b/test/unit/commands.supportBundle.test.ts index 91aeaffbcd..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"; @@ -67,9 +68,8 @@ function setup(options: { cliVersion?: string } = {}) { vscode.Uri.file(OUTPUT_PATH), ); // Accept the collection disclosure dialog by default. - vi.mocked(vscode.window.showInformationMessage).mockResolvedValue( - "Continue" as unknown as vscode.MessageItem, - ); + 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({ @@ -111,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 { @@ -240,25 +240,25 @@ describe("Commands.supportBundle", () => { }); it("describes the collected data before creating the bundle", async () => { - const { commands } = setup(); + const { commands, interaction } = setup(); await commands.supportBundle(agentItem("dev")); - expect(vscode.window.showInformationMessage).toHaveBeenCalledWith( - expect.any(String), + expect(interaction.getMessageCalls()).toContainEqual( expect.objectContaining({ - modal: true, - detail: expect.stringContaining("telemetry"), + message: "Create a support bundle?", + items: ["Continue"], + options: expect.objectContaining({ + modal: true, + detail: expect.stringContaining("telemetry"), + }), }), - "Continue", ); }); it("does not create a bundle when the disclosure dialog is dismissed", async () => { - const { commands } = setup(); - vi.mocked(vscode.window.showInformationMessage).mockResolvedValue( - undefined, - ); + const { commands, interaction } = setup(); + interaction.setResponse("Create a support bundle?", undefined); await commands.supportBundle(agentItem("dev")); diff --git a/test/unit/commands.telemetry.test.ts b/test/unit/commands.telemetry.test.ts index 8f1022dcf1..17b59d6f9f 100644 --- a/test/unit/commands.telemetry.test.ts +++ b/test/unit/commands.telemetry.test.ts @@ -298,6 +298,7 @@ describe("Commands", () => { 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/logging/formatters.test.ts b/test/unit/logging/formatters.test.ts index 81ae8f1dee..8eb699dd66 100644 --- a/test/unit/logging/formatters.test.ts +++ b/test/unit/logging/formatters.test.ts @@ -151,9 +151,9 @@ describe("Logging formatters", () => { token: "secret-token", token_type: "bearer", }); - expect(result).not.toContain("secret-"); 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", () => { @@ -161,9 +161,9 @@ describe("Logging formatters", () => { data: { session: { TOKEN: "secret-value" } }, items: [{ password: "secret-value" }], }); - expect(result).not.toContain("secret-value"); expect(result).toContain("TOKEN: ''"); expect(result).toContain("password: ''"); + expect(result).not.toContain("secret-value"); }); it("redacts sensitive fields in URLSearchParams", () => { @@ -172,23 +172,23 @@ describe("Logging formatters", () => { refresh_token: "secret-value", }); const result = formatBody(params); - expect(result).not.toContain("secret-value"); 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).not.toContain("secret-value"); expect(json).toContain("expires_in"); + expect(json).not.toContain("secret-value"); const form = formatBody( "grant_type=authorization_code&code=secret-value", ); - expect(form).not.toContain("secret-value"); expect(form).toContain("grant_type"); + expect(form).not.toContain("secret-value"); }); it("leaves non-sensitive strings unchanged", () => { diff --git a/test/unit/logging/httpLogger.test.ts b/test/unit/logging/httpLogger.test.ts index ba6b8d6445..116c24fe42 100644 --- a/test/unit/logging/httpLogger.test.ts +++ b/test/unit/logging/httpLogger.test.ts @@ -129,11 +129,10 @@ describe("REST HTTP Logger", () => { logRequest(logger, config, HttpClientLogLevel.BODY); const logged = logger.text; - expect(logged).not.toContain("request-secret"); - expect(logged).not.toContain("command-secret"); - expect(logged).not.toContain("body-secret"); 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", () => { @@ -149,10 +148,10 @@ describe("REST HTTP Logger", () => { logError(logger, error, HttpClientLogLevel.BODY); const logged = logger.text; - expect(logged).not.toContain("response-secret"); - expect(logged).not.toContain("body-secret"); 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", () => { @@ -162,10 +161,8 @@ describe("REST HTTP Logger", () => { logError(logger, error, HttpClientLogLevel.BODY); - const logged = logger.text; - expect(logged).not.toContain("request-secret"); - expect(logged).not.toContain("command-secret"); - expect(logged).not.toContain("body-secret"); + // Every planted value contains "secret"; none may survive. + expect(logger.text).not.toContain("secret"); }); }); }); diff --git a/test/unit/remote/remote.migration.test.ts b/test/unit/remote/migration.test.ts similarity index 52% rename from test/unit/remote/remote.migration.test.ts rename to test/unit/remote/migration.test.ts index 4f34e51398..0eadd0c27b 100644 --- a/test/unit/remote/remote.migration.test.ts +++ b/test/unit/remote/migration.test.ts @@ -2,20 +2,13 @@ import { vol } from "memfs"; import { describe, expect, it, vi } from "vitest"; import { PathResolver } from "@/core/pathResolver"; -import { Remote } from "@/remote/remote"; +import { migrateAuthToSecretsStorage } from "@/remote/migration"; -import { createTestTelemetryService } from "../../mocks/telemetry"; -import { - createMockLogger, - MockConfigurationProvider, -} from "../../mocks/testHelpers"; +import { createMockLogger } from "../../mocks/testHelpers"; import type * as nodeFs from "node:fs"; -import type * as vscode from "vscode"; -import type { Commands } from "@/commands"; -import type { ServiceContainer } from "@/core/container"; -import type { SecretsManager, SessionAuth } from "@/core/secretsManager"; +import type { SessionAuth } from "@/core/secretsManager"; vi.mock("fs/promises", async () => { const memfs: { fs: typeof nodeFs } = await vi.importActual("memfs"); @@ -30,42 +23,20 @@ const HOSTNAME = "dep.example.com"; const URL_PATH = `${BASE_PATH}/${HOSTNAME}/url`; const TOKEN_PATH = `${BASE_PATH}/${HOSTNAME}/session`; -interface MigratableRemote { - migrateToSecretsStorage(safeHostname: string): Promise; -} - function setup(options: { existingAuth?: SessionAuth } = {}) { - vi.clearAllMocks(); vol.reset(); - new MockConfigurationProvider(); - - const logger = createMockLogger(); - const pathResolver = new PathResolver(BASE_PATH, "/logs/code"); - const secretsManager: Pick< - SecretsManager, - "getSessionAuth" | "setSessionAuth" - > = { + const secretsManager = { getSessionAuth: vi.fn(() => Promise.resolve(options.existingAuth)), setSessionAuth: vi.fn(() => Promise.resolve()), }; - - const serviceContainer = { - getLogger: () => logger, - getPathResolver: () => pathResolver, - getCliManager: () => ({}), - getContextManager: () => ({}), - getSecretsManager: () => secretsManager, - getLoginCoordinator: () => ({}), - getTelemetryService: () => createTestTelemetryService(), - } as unknown as ServiceContainer; - - const remote = new Remote( - serviceContainer, - {} as Commands, - {} as vscode.ExtensionContext, - ) as unknown as MigratableRemote; - - return { remote, secretsManager }; + const migrate = () => + migrateAuthToSecretsStorage( + HOSTNAME, + new PathResolver(BASE_PATH, "/logs/code"), + secretsManager, + createMockLogger(), + ); + return { migrate, secretsManager }; } function writeLegacyFiles(): void { @@ -75,12 +46,12 @@ function writeLegacyFiles(): void { }); } -describe("Remote session auth migration", () => { +describe("Session auth migration", () => { it("moves file-based auth into secret storage and deletes the files", async () => { - const { remote, secretsManager } = setup(); + const { migrate, secretsManager } = setup(); writeLegacyFiles(); - await remote.migrateToSecretsStorage(HOSTNAME); + await migrate(); expect(secretsManager.setSessionAuth).toHaveBeenCalledWith(HOSTNAME, { url: "https://dep.example.com", @@ -91,25 +62,25 @@ describe("Remote session auth migration", () => { }); it("deletes the files even when the migration is rejected", async () => { - const { remote, secretsManager } = setup(); - vi.mocked(secretsManager.setSessionAuth).mockRejectedValue( + const { migrate, secretsManager } = setup(); + secretsManager.setSessionAuth.mockRejectedValue( new Error("Session auth hostname mismatch"), ); writeLegacyFiles(); - await remote.migrateToSecretsStorage(HOSTNAME); + 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 { remote, secretsManager } = setup({ + const { migrate, secretsManager } = setup({ existingAuth: { url: "https://dep.example.com", token: "current" }, }); writeLegacyFiles(); - await remote.migrateToSecretsStorage(HOSTNAME); + await migrate(); expect(secretsManager.setSessionAuth).not.toHaveBeenCalled(); expect(vol.existsSync(URL_PATH)).toBe(true); @@ -117,9 +88,9 @@ describe("Remote session auth migration", () => { }); it("does nothing when the legacy files are missing", async () => { - const { remote, secretsManager } = setup(); + const { migrate, secretsManager } = setup(); - await remote.migrateToSecretsStorage(HOSTNAME); + await migrate(); expect(secretsManager.setSessionAuth).not.toHaveBeenCalled(); }); From 8830725b89230583bd540df85d3f8639ce43c11a Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Wed, 5 Aug 2026 16:55:22 +0300 Subject: [PATCH 9/9] docs: add changelog entries, with the support bundle disclosure as its own Per review, give the support bundle confirmation dialog a separate changelog entry alongside entries for the credential and log hygiene changes. --- CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) 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