diff --git a/src/core/secretsManager.ts b/src/core/secretsManager.ts index 6d40840dc2..093a469ac9 100644 --- a/src/core/secretsManager.ts +++ b/src/core/secretsManager.ts @@ -62,6 +62,22 @@ export class SecretsManager { return `${prefix}${safeHostname}`; } + private assertSessionAuthHostname(safeHostname: string, url: string): void { + let authHostname: string; + try { + authHostname = toSafeHost(url); + } catch { + throw new Error( + `Session auth hostname mismatch: expected "${safeHostname}", got an invalid URL "${url}"`, + ); + } + if (authHostname !== safeHostname) { + throw new Error( + `Session auth hostname mismatch: expected "${safeHostname}", got "${authHostname}"`, + ); + } + } + private async getSecret( prefix: SecretKeyPrefix, safeHostname: string, @@ -189,15 +205,31 @@ export class SecretsManager { return undefined; } const result = SessionAuthSchema.safeParse(data); - return result.success ? result.data : undefined; + if (!result.success) { + return undefined; + } + try { + this.assertSessionAuthHostname(safeHostname, result.data.url); + } catch (error) { + this.logger.warn("Ignoring stored session auth:", error); + return undefined; + } + return result.data; } + /** + * Store session auth for a deployment. + * + * @throws If the auth URL is invalid or its hostname does not match the + * deployment. + */ public async setSessionAuth( safeHostname: string, auth: SessionAuth, ): Promise { // Parse through schema to strip any extra fields const state = SessionAuthSchema.parse(auth); + this.assertSessionAuthHostname(safeHostname, state.url); await this.setSecret(SESSION_KEY_PREFIX, safeHostname, state); } diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 36d8687dcb..00c855c529 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -823,10 +823,14 @@ export class Remote { if (url.status === "fulfilled" && token.status === "fulfilled") { this.logger.info("Migrating session auth from files for", safeHostname); - await this.secretsManager.setSessionAuth(safeHostname, { - url: url.value.trim(), - token: token.value.trim(), - }); + 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); + } } } diff --git a/test/mocks/testHelpers.ts b/test/mocks/testHelpers.ts index 90f2614d1c..b03d3daa14 100644 --- a/test/mocks/testHelpers.ts +++ b/test/mocks/testHelpers.ts @@ -487,6 +487,51 @@ export function createMockLogger(): Logger { }; } +export interface LogEntry { + level: "trace" | "debug" | "info" | "warn" | "error"; + message: string; + args: readonly unknown[]; +} + +/** Logger that records structured entries for tests of logging behavior. */ +export class LogCollector implements Logger { + private readonly _entries: LogEntry[] = []; + + get entries(): readonly LogEntry[] { + return this._entries; + } + + trace(message: string, ...args: unknown[]): void { + this.collect("trace", message, args); + } + + debug(message: string, ...args: unknown[]): void { + this.collect("debug", message, args); + } + + info(message: string, ...args: unknown[]): void { + this.collect("info", message, args); + } + + warn(message: string, ...args: unknown[]): void { + this.collect("warn", message, args); + } + + error(message: string, ...args: unknown[]): void { + this.collect("error", message, args); + } + + show(): void {} + + private collect( + level: LogEntry["level"], + message: string, + args: readonly unknown[], + ): void { + this._entries.push({ level, message, args }); + } +} + /** Resolve once pending microtasks and the macrotask queue have drained. */ export async function flush(): Promise { await new Promise((resolve) => setImmediate(resolve)); diff --git a/test/unit/core/secretsManager.test.ts b/test/unit/core/secretsManager.test.ts index 1166bb774c..1ab29e5451 100644 --- a/test/unit/core/secretsManager.test.ts +++ b/test/unit/core/secretsManager.test.ts @@ -9,6 +9,7 @@ import { import { InMemoryMemento, InMemorySecretStorage, + LogCollector, createMockLogger, } from "../../mocks/testHelpers"; @@ -16,6 +17,7 @@ describe("SecretsManager", () => { let secretStorage: InMemorySecretStorage; let memento: InMemoryMemento; let mementoManager: MementoManager; + let logger: ReturnType; let secretsManager: SecretsManager; beforeEach(() => { @@ -23,11 +25,8 @@ describe("SecretsManager", () => { secretStorage = new InMemorySecretStorage(); memento = new InMemoryMemento(); mementoManager = new MementoManager(memento); - secretsManager = new SecretsManager( - secretStorage, - mementoManager, - createMockLogger(), - ); + logger = createMockLogger(); + secretsManager = new SecretsManager(secretStorage, mementoManager, logger); }); describe("session auth", () => { @@ -48,6 +47,102 @@ describe("SecretsManager", () => { expect(newAuth?.token).toBe("new-token"); }); + it("should accept a URL port for a matching hostname", async () => { + await secretsManager.setSessionAuth("example.com", { + url: "https://example.com:8443", + token: "test-token", + }); + + expect(await secretsManager.getSessionAuth("example.com")).toEqual({ + url: "https://example.com:8443", + token: "test-token", + }); + }); + + it.each([ + { + name: "malformed URL", + url: "not a URL", + error: + 'Session auth hostname mismatch: expected "example.com", got an invalid URL "not a URL"', + }, + { + name: "mismatched hostname", + url: "https://other.example.com", + error: + 'Session auth hostname mismatch: expected "example.com", got "other.example.com"', + }, + ])("should reject a write with a $name", async ({ url, error }) => { + const existingAuth = { + url: "https://example.com", + token: "existing-token", + }; + await secretsManager.setSessionAuth("example.com", existingAuth); + + await expect( + secretsManager.setSessionAuth("example.com", { + url, + token: "secret-token", + }), + ).rejects.toThrow(error); + + expect(await secretsManager.getSessionAuth("example.com")).toEqual( + existingAuth, + ); + }); + + it.each([ + { name: "malformed URL", url: "not a URL" }, + { + name: "mismatched hostname", + url: "https://other.example.com/private?token=secret", + }, + ])("should ignore stored auth with a $name", async ({ url }) => { + await secretStorage.store( + "coder.session.example.com", + JSON.stringify({ url, token: "secret-token" }), + ); + + expect( + await secretsManager.getSessionAuth("example.com"), + ).toBeUndefined(); + }); + + describe("logging", () => { + it.each([ + { + name: "malformed URL", + url: "not a URL", + error: + 'Session auth hostname mismatch: expected "example.com", got an invalid URL "not a URL"', + }, + { + // A mismatched URL can carry credentials, so only its hostname is logged. + name: "mismatched hostname", + url: "https://other.example.com/private?token=secret", + error: + 'Session auth hostname mismatch: expected "example.com", got "other.example.com"', + }, + ])("logs why a $name was ignored", async ({ url, error }) => { + const logs = new LogCollector(); + const manager = new SecretsManager(secretStorage, mementoManager, logs); + await secretStorage.store( + "coder.session.example.com", + JSON.stringify({ url, token: "secret-token" }), + ); + + await manager.getSessionAuth("example.com"); + + expect(logs.entries).toEqual([ + { + level: "warn", + message: "Ignoring stored session auth:", + args: [new Error(error)], + }, + ]); + }); + }); + it("should clear session auth", async () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", @@ -85,7 +180,7 @@ describe("SecretsManager", () => { "example.com", ); - await secretsManager.setSessionAuth("other-com", { + await secretsManager.setSessionAuth("other.com", { url: "https://other.com", token: "other-token", }); @@ -93,7 +188,7 @@ describe("SecretsManager", () => { "example.com", ); expect(await secretsManager.getKnownSafeHostnames()).toContain( - "other-com", + "other.com", ); }); @@ -327,9 +422,9 @@ describe("SecretsManager", () => { extraField: "should be stripped", }; - await secretsManager.setSessionAuth("example.com", authWithExtra); + await secretsManager.setSessionAuth("coder.example.com", authWithExtra); - const raw = await secretStorage.get("coder.session.example.com"); + const raw = await secretStorage.get("coder.session.coder.example.com"); expect(JSON.parse(raw!)).toEqual({ url: "https://coder.example.com", token: "test-token", @@ -347,9 +442,9 @@ describe("SecretsManager", () => { }, }; - await secretsManager.setSessionAuth("example.com", authWithExtra); + await secretsManager.setSessionAuth("coder.example.com", authWithExtra); - const raw = await secretStorage.get("coder.session.example.com"); + const raw = await secretStorage.get("coder.session.coder.example.com"); expect(JSON.parse(raw!)).toEqual({ url: "https://coder.example.com", token: "test-token", @@ -419,7 +514,6 @@ describe("SecretsManager", () => { describe("backwards compatibility", () => { interface BackwardsCompatTestCase { name: string; - key: string; data: Record; expected: unknown; } @@ -427,13 +521,11 @@ describe("SecretsManager", () => { const sessionAuthCases: BackwardsCompatTestCase[] = [ { name: "without optional oauth field", - key: "coder.session.example.com", data: { url: "https://coder.example.com", token: "test-token" }, expected: { url: "https://coder.example.com", token: "test-token" }, }, { name: "with OAuth without optional fields", - key: "coder.session.example.com", data: { url: "https://coder.example.com", token: "test-token", @@ -449,9 +541,13 @@ describe("SecretsManager", () => { it.each(sessionAuthCases)( "handles SessionAuth $name", - async ({ key, data, expected }) => { - await secretStorage.store(key, JSON.stringify(data)); - const result = await secretsManager.getSessionAuth("example.com"); + async ({ data, expected }) => { + await secretStorage.store( + "coder.session.coder.example.com", + JSON.stringify(data), + ); + const result = + await secretsManager.getSessionAuth("coder.example.com"); expect(result).toEqual(expected); }, ); diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index 205e1c8228..6c25d9cad7 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -213,22 +213,20 @@ describe("OAuthSessionManager", () => { }); describe("getStoredTokens validation", () => { - it("returns undefined when URL mismatches", async () => { + it("returns undefined when the URL differs on the same hostname", async () => { const { secretsManager, manager } = createTestContext(); - // Manually set auth with different URL (can't use helper) await secretsManager.setSessionAuth(TEST_HOSTNAME, { - url: "https://different-coder.example.com", + url: `${TEST_URL}:8443`, token: "access-token", oauth: { refresh_token: "refresh-token", expiry_timestamp: Date.now() + ONE_HOUR_MS, - scope: "", + scope: DEFAULT_OAUTH_SCOPES, }, }); - const result = await manager.isLoggedInWithOAuth(); - expect(result).toBe(false); + expect(await manager.isLoggedInWithOAuth()).toBe(false); }); }); diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts new file mode 100644 index 0000000000..835240fc92 --- /dev/null +++ b/test/unit/remote/remote.test.ts @@ -0,0 +1,108 @@ +import { vol } from "memfs"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { MementoManager } from "@/core/mementoManager"; +import { PathResolver } from "@/core/pathResolver"; +import { SecretsManager } from "@/core/secretsManager"; +import { Remote } from "@/remote/remote"; + +import { createTestTelemetryService } from "../../mocks/telemetry"; +import { + createMockLogger, + InMemoryMemento, + InMemorySecretStorage, + LogCollector, + MockConfigurationProvider, +} from "../../mocks/testHelpers"; + +import type * as vscode from "vscode"; + +import type { Commands } from "@/commands"; +import type { CliManager } from "@/core/cliManager"; +import type { ServiceContainer } from "@/core/container"; +import type { ContextManager } from "@/core/contextManager"; +import type { Logger } from "@/logging/logger"; +import type { LoginCoordinator } from "@/login/loginCoordinator"; + +vi.mock("node:fs/promises", async () => (await import("memfs")).fs.promises); + +const SAFE_HOSTNAME = "coder.example.com"; +const REMOTE_AUTHORITY = + "ssh-remote+coder-vscode.coder.example.com--testuser--test-workspace.main"; +const MISMATCHED_URL = + "https://cursor.example.com/private?token=sensitive-url-token"; +const SESSION_TOKEN = "sensitive-session-token"; + +function createRemote(logger: Logger = createMockLogger()) { + const pathResolver = new PathResolver("/mock/global", "/mock/log"); + vol.fromJSON({ + [pathResolver.getUrlPath(SAFE_HOSTNAME)]: MISMATCHED_URL, + [pathResolver.getSessionTokenPath(SAFE_HOSTNAME)]: SESSION_TOKEN, + }); + const secretsManager = new SecretsManager( + new InMemorySecretStorage(), + new MementoManager(new InMemoryMemento()), + logger, + ); + const ensureLoggedInWithDialog = vi + .fn() + .mockResolvedValue({ success: false, reason: "user_dismissed" }); + const serviceContainer = { + getLogger: () => logger, + getPathResolver: () => pathResolver, + getCliManager: () => ({}) as CliManager, + getContextManager: () => ({}) as ContextManager, + getSecretsManager: () => secretsManager, + getLoginCoordinator: () => + ({ ensureLoggedInWithDialog }) as unknown as LoginCoordinator, + getTelemetryService: () => createTestTelemetryService(), + } as ServiceContainer; + + return { + remote: new Remote( + serviceContainer, + {} as Commands, + {} as vscode.ExtensionContext, + ), + secretsManager, + }; +} + +describe("Remote", () => { + beforeEach(() => { + vi.clearAllMocks(); + vol.reset(); + new MockConfigurationProvider(); + }); + + it("ignores mismatched file auth", async () => { + const { remote, secretsManager } = createRemote(); + + await expect( + remote.setup(REMOTE_AUTHORITY, "none", "anysphere.remote-ssh"), + ).resolves.toBeUndefined(); + expect(await secretsManager.getSessionAuth(SAFE_HOSTNAME)).toBeUndefined(); + }); + + describe("logging", () => { + it("logs why the file auth migration failed", async () => { + const logs = new LogCollector(); + const { remote } = createRemote(logs); + + 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({ + level: "warn", + message: "Failed to migrate session auth from files:", + args: [ + new Error( + `Session auth hostname mismatch: expected "${SAFE_HOSTNAME}", got "cursor.example.com"`, + ), + ], + }); + }); + }); +});