fix(logging): redact credentials from every logged value

The logger summarized only a top-level Axios error. Every logged
argument now goes through one redaction walk, for both the output
channel and the flight-recorder buffer. It redacts credential fields
with string values in plain objects and errors, and summarizes Axios
errors at any depth, including error causes. The walk reads only data
properties, never throws, and skips entries the channel drops.

Author: EhabY

CHANGELOG.md

@@ -33,8 +33,8 @@
   in a future release, so ask your Coder administrator to enable dynamic client
   registration. For instructions, see
   [OAuth2 provider](https://coder.com/docs/admin/integrations/oauth2-provider).
-- Keep session tokens, OAuth tokens, and client secrets from failed requests out
-  of the buffered logs that the Coder output channel replays.
+- Keep session tokens, OAuth tokens, and client secrets out of the Coder output
+  channel and its buffered logs, including from failed requests.
 - CLI commands and SSH connections that the extension starts ignore
   `CODER_SESSION_TOKEN` from the environment, so they no longer fail or run as a
   different user inside a Coder workspace.

src/logging/flightRecorder.ts

@@ -1,4 +1,4 @@
-import { safeStringify } from "./formatters";
+import { redactSecrets, safeStringify } from "./formatters";
 
 import type { LogSink, Logger } from "./logger";
 
@@ -109,8 +109,13 @@ export class FlightRecorder implements Logger {
 	/** Pass a call through to the sink and buffer it when below the level. */
 	private wrap(level: Level): (message: string, ...args: unknown[]) => void {
 		return (message, ...args) => {
-			this.record(level, message, args);
-			this.inner[level](message, ...args);
+			if (SEVERITY[level] < this.channel.logLevel) {
+				// The channel drops this entry, so only the buffered copy is formatted.
+				this.record(level, message, args);
+				this.inner[level](message, ...args);
+			} else {
+				this.inner[level](message, ...args.map(redactSecrets));
+			}
 		};
 	}
 
@@ -130,7 +135,7 @@ export class FlightRecorder implements Logger {
 	}
 
 	private record(level: Level, message: string, args: unknown[]): void {
-		if (this.capacity === 0 || SEVERITY[level] >= this.channel.logLevel) {
+		if (this.capacity === 0) {
 			return;
 		}
 		const text = [message, ...args.map((arg) => safeStringify(arg) ?? "")].join(

src/logging/formatters.ts

@@ -14,8 +14,8 @@ const SENSITIVE_HEADERS: ReadonlySet<Lowercase<string>> = new Set([
 	"x-api-key",
 ]);
 
-/** Credential fields from OAuth token requests/responses, logged at BODY level. */
-const SENSITIVE_BODY_FIELDS: ReadonlySet<Lowercase<string>> = new Set([
+/** Credential fields, such as those in OAuth requests, responses, and sessions. */
+const SENSITIVE_FIELDS: ReadonlySet<Lowercase<string>> = new Set([
 	"access_token",
 	"client_secret",
 	"code",
@@ -78,88 +78,132 @@ export function formatHeaders(
 
 export function formatBody(body: unknown): string {
 	if (body) {
-		return safeStringify(redactBodyFields(body)) ?? "<invalid body>";
+		return stringify(redactBody(body)) ?? "<invalid body>";
 	} else {
 		return "<no body>";
 	}
 }
 
 /**
- * Redact known credential fields, copying only what changes; untouched
- * values keep their original reference. util.inspect has no replacer
- * hook, so the value is walked before stringifying.
+ * Copy of `value` with credentials redacted and Axios errors summarized at any
+ * depth; unchanged values keep their reference. Never throws.
  */
-function redactBodyFields(
-	value: unknown,
-	seen = new WeakSet<object>(),
-): unknown {
-	if (typeof value === "string") {
-		return redactStringBody(value, seen);
-	}
-	if (value instanceof URLSearchParams) {
-		const keys = [...value.keys()];
-		if (!keys.some((key) => SENSITIVE_BODY_FIELDS.has(lowercase(key)))) {
-			return value;
-		}
-		return new URLSearchParams(
-			[...value].map(([key, entry]) => [
-				key,
-				SENSITIVE_BODY_FIELDS.has(lowercase(key)) ? REDACTED : entry,
-			]),
-		);
+export function redactSecrets(value: unknown): unknown {
+	try {
+		return redact(value, new WeakSet());
+	} catch {
+		return REDACTED;
 	}
+}
+
+function redact(value: unknown, seen: WeakSet<object>): unknown {
 	if (typeof value !== "object" || value === null || seen.has(value)) {
 		return value;
 	}
 	seen.add(value);
+	if (isAxiosError(value)) {
+		return summarizeAxiosError(value);
+	}
+	if (value instanceof URLSearchParams) {
+		return redactParams(value);
+	}
 	if (Array.isArray(value)) {
-		const entries: readonly unknown[] = value;
-		let copy: unknown[] | undefined;
-		entries.forEach((entry, index) => {
-			const redacted = redactBodyFields(entry, seen);
-			if (redacted !== entry) {
-				copy ??= [...entries];
-				copy[index] = redacted;
-			}
-		});
-		return copy ?? value;
+		return redactArray(value, seen);
 	}
-	// Rebuilding a Date or Buffer from its entries would mangle its output.
+	// Copying a Date, Buffer, or class instance property by property would mangle it.
 	const proto: unknown = Object.getPrototypeOf(value);
-	if (proto !== Object.prototype && proto !== null) {
-		return value;
+	return value instanceof Error || proto === Object.prototype || proto === null
+		? redactProperties(value, seen)
+		: value;
+}
+
+function redactArray(
+	array: readonly unknown[],
+	seen: WeakSet<object>,
+): readonly unknown[] {
+	let copy: unknown[] | undefined;
+	for (const [index, entry] of array.entries()) {
+		const redacted = redact(entry, seen);
+		if (redacted !== entry) {
+			copy ??= [...array];
+			copy[index] = redacted;
+		}
 	}
-	let copy: Record<string, unknown> | undefined;
-	for (const [key, entry] of Object.entries(value)) {
-		const redacted = SENSITIVE_BODY_FIELDS.has(lowercase(key))
-			? REDACTED
-			: redactBodyFields(entry, seen);
+	return copy ?? array;
+}
+
+/** Reads data properties only, so getters never run. */
+function redactProperties(value: object, seen: WeakSet<object>): object {
+	let descriptors: PropertyDescriptorMap | undefined;
+	for (const key of Reflect.ownKeys(value)) {
+		const descriptor = Object.getOwnPropertyDescriptor(value, key);
+		if (!descriptor || !("value" in descriptor)) {
+			continue;
+		}
+		const entry: unknown = descriptor.value;
+		const redacted = isCredential(key, entry) ? REDACTED : redact(entry, seen);
 		if (redacted !== entry) {
-			copy ??= { ...value };
-			copy[key] = redacted;
+			descriptors ??= Object.getOwnPropertyDescriptors(value);
+			descriptors[key] = { ...descriptor, value: redacted };
 		}
 	}
-	return copy ?? value;
+	if (!descriptors) {
+		return value;
+	}
+	if (value instanceof Error) {
+		// V8's `stack` accessor reads the original error's internal slot.
+		descriptors.stack = {
+			value: value.stack,
+			writable: true,
+			configurable: true,
+		};
+	}
+	return Object.create(
+		Object.getPrototypeOf(value) as object | null,
+		descriptors,
+	) as object;
 }
 
-/**
- * Axios error paths expose only the serialized body, so JSON and
- * form-encoded strings are parsed too. Clean strings pass through as-is.
- */
-function redactStringBody(body: string, seen: WeakSet<object>): unknown {
+function redactParams(params: URLSearchParams): URLSearchParams {
+	const entries = [...params];
+	if (!entries.some(([key, entry]) => isCredential(key, entry))) {
+		return params;
+	}
+	return new URLSearchParams(
+		entries.map(([key, entry]) => [
+			key,
+			isCredential(key, entry) ? REDACTED : entry,
+		]),
+	);
+}
+
+/** Credentials are strings, so numeric fields such as a close `code` stay readable. */
+function isCredential(key: PropertyKey, value: unknown): boolean {
+	return (
+		typeof key === "string" &&
+		typeof value === "string" &&
+		SENSITIVE_FIELDS.has(lowercase(key))
+	);
+}
+
+/** Axios error paths expose only the serialized body, so JSON and form-encoded strings are parsed too. */
+function redactBody(body: unknown): unknown {
+	if (typeof body !== "string") {
+		return redactSecrets(body);
+	}
 	const trimmed = body.trim();
 	if (trimmed.startsWith("{") || trimmed.startsWith("[")) {
 		try {
 			const parsed: unknown = JSON.parse(trimmed);
-			const redacted = redactBodyFields(parsed, seen);
+			const redacted = redactSecrets(parsed);
 			return redacted === parsed ? body : redacted;
 		} catch {
 			// Not JSON; fall through.
 		}
 	}
 	if (trimmed.includes("=")) {
 		const params = new URLSearchParams(trimmed);
-		const redacted = redactBodyFields(params, seen);
+		const redacted = redactParams(params);
 		return redacted === params ? body : redacted;
 	}
 	return body;
@@ -208,13 +252,17 @@ function summarizeAxiosError(error: AxiosError) {
 		method: formatMethod(error.config?.method),
 		url: formatUri(error.config),
 		status: error.response?.status,
-		data: redactBodyFields(error.response?.data),
+		data: redactBody(error.response?.data),
 	};
 }
 
 export function safeStringify(data: unknown): string | null {
+	return stringify(redactSecrets(data));
+}
+
+function stringify(data: unknown): string | null {
 	try {
-		return util.inspect(isAxiosError(data) ? summarizeAxiosError(data) : data, {
+		return util.inspect(data, {
 			showHidden: false,
 			// Bounded so a single log line cannot balloon in size.
 			depth: 8,
@@ -225,7 +273,6 @@ export function safeStringify(data: unknown): string | null {
 			getters: false, // avoid side-effects
 		});
 	} catch {
-		// Should rarely happen but just in case
 		return null;
 	}
 }

test/unit/logging/flightRecorder.test.ts

@@ -70,6 +70,17 @@ describe("FlightRecorder", () => {
 		]);
 	});
 
+	it("redacts credentials from forwarded and buffered args", () => {
+		const { buffer, calls, flush } = setup(INFO, 10);
+		const auth = { url: "https://coder.example.com", token: "secret" };
+
+		buffer.info("shown", auth);
+		buffer.debug("hidden", auth);
+
+		expect(calls[0].args).toEqual([{ ...auth, token: "<redacted>" }]);
+		expect(flush().join(" ")).not.toContain("secret");
+	});
+
 	interface LevelCase {
 		level: number;
 		hidden: LogMethod;

test/unit/logging/formatters.test.ts

@@ -1,5 +1,5 @@
 import { AxiosError, AxiosHeaders } from "axios";
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 
 import {
 	formatBody,
@@ -8,6 +8,7 @@ import {
 	formatSize,
 	formatTime,
 	formatUri,
+	redactSecrets,
 	safeStringify,
 	sizeOf,
 } from "@/logging/formatters";
@@ -205,6 +206,40 @@ describe("Logging formatters", () => {
 	});
 });
 
+describe("redactSecrets", () => {
+	it("returns unchanged values by reference", () => {
+		const value = { nested: { code: 1006 }, list: ["a"] };
+		expect(redactSecrets(value)).toBe(value);
+	});
+
+	it("never runs getters and never throws", () => {
+		const getter = vi.fn(() => "secret");
+		const value = Object.defineProperty({ token: "secret" }, "lazy", {
+			get: getter,
+			enumerable: true,
+		});
+		const { proxy, revoke } = Proxy.revocable({}, {});
+		revoke();
+
+		expect(redactSecrets(value)).toMatchObject({ token: "<redacted>" });
+		expect(getter).not.toHaveBeenCalled();
+		expect(redactSecrets(proxy)).toBe("<redacted>");
+	});
+
+	it("redacts an error's fields on a copy that keeps its stack", () => {
+		const error = Object.assign(new TypeError("failed"), { token: "secret" });
+
+		const redacted = redactSecrets(error);
+
+		expect(redacted).toBeInstanceOf(TypeError);
+		expect(redacted).toMatchObject({
+			message: "failed",
+			stack: error.stack,
+			token: "<redacted>",
+		});
+	});
+});
+
 describe("sizeOf", () => {
 	type SizeOfTestCase = [data: unknown, bytes: number | undefined];
 	it.each<SizeOfTestCase>([
@@ -293,7 +328,17 @@ describe("safeStringify", () => {
 		expect(safeStringify(nested)).not.toContain("bottom");
 	});
 
-	it("keeps request credentials out of Axios errors", () => {
+	it("redacts credential fields in any logged object", () => {
+		const result = safeStringify({
+			token: "access-secret",
+			oauth: { refresh_token: "refresh-secret", scope: "coder:all" },
+		});
+
+		expect(result).toContain("coder:all");
+		expect(result).not.toMatch(/access-secret|refresh-secret/);
+	});
+
+	it("keeps request credentials out of Axios errors at any depth", () => {
 		const config = {
 			method: "post",
 			url: "https://coder.example.com/oauth2/revoke",
@@ -314,8 +359,10 @@ describe("safeStringify", () => {
 			},
 		);
 
-		const result = safeStringify(error);
+		const wrapped = new Error("Revocation failed", { cause: error });
+		const result = safeStringify({ attempts: [wrapped] });
 
+		expect(result).toContain("Revocation failed");
 		expect(result).toContain("POST");
 		expect(result).toContain("https://coder.example.com/oauth2/revoke");
 		expect(result).toContain("status: 400");