From c8f5fe48a7cdeaeaf651ec439137f11c34e04acb Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 3 Aug 2026 10:52:09 +0000 Subject: [PATCH 1/4] fix: enforce session auth hostname invariant --- src/core/secretsManager.ts | 46 +++++++++- src/login/loginCoordinator.ts | 32 ++++++- src/remote/remote.ts | 23 +++-- test/unit/core/secretsManager.test.ts | 102 ++++++++++++++++++---- test/unit/login/loginCoordinator.test.ts | 37 ++++++++ test/unit/oauth/sessionManager.test.ts | 10 +-- test/unit/remote/remote.test.ts | 103 +++++++++++++++++++++++ 7 files changed, 323 insertions(+), 30 deletions(-) create mode 100644 test/unit/remote/remote.test.ts diff --git a/src/core/secretsManager.ts b/src/core/secretsManager.ts index 6d40840dc2..2bfc4a3099 100644 --- a/src/core/secretsManager.ts +++ b/src/core/secretsManager.ts @@ -51,6 +51,31 @@ const SessionAuthSchema = z.object({ export type SessionAuth = z.infer; +export class SessionAuthHostnameError extends Error { + public constructor( + public readonly safeHostname: string, + public readonly authHostname?: string, + ) { + super("Session auth URL does not match its deployment hostname"); + this.name = "SessionAuthHostnameError"; + } +} + +export function assertSessionAuthHostname( + safeHostname: string, + url: string, +): void { + let authHostname: string; + try { + authHostname = toSafeHost(url); + } catch { + throw new SessionAuthHostnameError(safeHostname); + } + if (authHostname !== safeHostname) { + throw new SessionAuthHostnameError(safeHostname, authHostname); + } +} + export class SecretsManager { constructor( private readonly secrets: SecretStorage, @@ -189,7 +214,25 @@ export class SecretsManager { return undefined; } const result = SessionAuthSchema.safeParse(data); - return result.success ? result.data : undefined; + if (!result.success) { + return undefined; + } + try { + assertSessionAuthHostname(safeHostname, result.data.url); + } catch (error) { + if (!(error instanceof SessionAuthHostnameError)) { + throw error; + } + this.logger.warn( + "Ignoring session auth with invalid deployment hostname", + { + safeHostname: error.safeHostname, + authHostname: error.authHostname ?? "(invalid URL)", + }, + ); + return undefined; + } + return result.data; } public async setSessionAuth( @@ -198,6 +241,7 @@ export class SecretsManager { ): Promise { // Parse through schema to strip any extra fields const state = SessionAuthSchema.parse(auth); + assertSessionAuthHostname(safeHostname, state.url); await this.setSecret(SESSION_KEY_PREFIX, safeHostname, state); } diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 0023b95dbc..b42a6f692b 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -4,6 +4,12 @@ import * as vscode from "vscode"; import { CoderApi } from "../api/coderApi"; import { needToken } from "../api/utils"; +import { + assertSessionAuthHostname, + SessionAuthHostnameError, + type OAuthTokenData, + type SecretsManager, +} from "../core/secretsManager"; import { CertificateError } from "../error/certificateError"; import { OAuthAuthorizer } from "../oauth/authorizer"; import { buildOAuthTokenData } from "../oauth/utils"; @@ -17,7 +23,6 @@ import type { User } from "coder/site/src/api/typesGenerated"; import type { CliCredentialManager } from "../core/cliCredentialManager"; import type { MementoManager } from "../core/mementoManager"; -import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; import type { AuthLoginPromptTrigger, @@ -88,6 +93,9 @@ export class LoginCoordinator implements vscode.Disposable { options: LoginOptions & { url: string }, ): Promise { const { safeHostname, url } = options; + if (!this.hasValidSessionAuthHostname(safeHostname, url)) { + return Promise.resolve({ success: false, reason: "auth_failed" }); + } return this.executeWithGuard(async () => { const result = await this.attemptLogin( { safeHostname, url }, @@ -147,6 +155,9 @@ export class LoginCoordinator implements vscode.Disposable { if (!newUrl) { return { success: false, reason: "no_url_provided" }; } + if (!this.hasValidSessionAuthHostname(safeHostname, newUrl)) { + return { success: false, reason: "auth_failed" }; + } const result = await this.attemptLogin( { url: newUrl, safeHostname }, @@ -175,6 +186,25 @@ export class LoginCoordinator implements vscode.Disposable { }); } + private hasValidSessionAuthHostname( + safeHostname: string, + url: string, + ): boolean { + try { + assertSessionAuthHostname(safeHostname, url); + return true; + } catch (error) { + if (!(error instanceof SessionAuthHostnameError)) { + throw error; + } + this.logger.warn("Ignoring login with invalid deployment hostname", { + safeHostname: error.safeHostname, + authHostname: error.authHostname ?? "(invalid URL)", + }); + return false; + } + } + private async persistSessionAuth( result: LoginAttemptResult, safeHostname: string, diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 36d8687dcb..fc2d0fb69b 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -20,6 +20,10 @@ import { watchConfigurationChanges, } from "../configWatcher"; import { version as cliVersion } from "../core/cliExec"; +import { + SessionAuthHostnameError, + type SecretsManager, +} from "../core/secretsManager"; import { toError } from "../error/errorUtils"; import { featureSetForVersion, type FeatureSet } from "../featureSet"; import { Inbox } from "../inbox"; @@ -72,7 +76,6 @@ import type { ServiceContainer } from "../core/container"; import type { ContextManager } from "../core/contextManager"; import type { StartupMode } from "../core/mementoManager"; import type { PathResolver } from "../core/pathResolver"; -import type { SecretsManager } from "../core/secretsManager"; import type { Logger } from "../logging/logger"; import type { LoginCoordinator } from "../login/loginCoordinator"; @@ -823,10 +826,20 @@ 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) { + if (!(error instanceof SessionAuthHostnameError)) { + throw error; + } + this.logger.warn("Ignoring invalid session auth migration", { + safeHostname: error.safeHostname, + authHostname: error.authHostname ?? "(invalid URL)", + }); + } } } diff --git a/test/unit/core/secretsManager.test.ts b/test/unit/core/secretsManager.test.ts index 1166bb774c..6e770a7368 100644 --- a/test/unit/core/secretsManager.test.ts +++ b/test/unit/core/secretsManager.test.ts @@ -16,6 +16,7 @@ describe("SecretsManager", () => { let secretStorage: InMemorySecretStorage; let memento: InMemoryMemento; let mementoManager: MementoManager; + let logger: ReturnType; let secretsManager: SecretsManager; beforeEach(() => { @@ -23,11 +24,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 +46,75 @@ 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" }, + { + name: "mismatched hostname", + url: "https://other.example.com", + }, + ])("should reject a write with a $name", async ({ url }) => { + await expect( + secretsManager.setSessionAuth("example.com", { + url, + token: "secret-token", + }), + ).rejects.toThrow( + "Session auth URL does not match its deployment hostname", + ); + + expect( + await secretStorage.get("coder.session.example.com"), + ).toBeUndefined(); + expect(mementoManager.getDeploymentAccess("example.com")).toBeUndefined(); + }); + + it.each([ + { + name: "malformed URL", + url: "not a URL", + expectedHostname: "(invalid URL)", + }, + { + name: "mismatched hostname", + url: "https://other.example.com/private?token=secret", + expectedHostname: "other.example.com", + }, + ])( + "should ignore stored auth with a $name", + async ({ url, expectedHostname }) => { + await secretStorage.store( + "coder.session.example.com", + JSON.stringify({ url, token: "secret-token" }), + ); + + expect( + await secretsManager.getSessionAuth("example.com"), + ).toBeUndefined(); + expect(logger.warn).toHaveBeenCalledWith( + "Ignoring session auth with invalid deployment hostname", + { + safeHostname: "example.com", + authHostname: expectedHostname, + }, + ); + expect(JSON.stringify(vi.mocked(logger.warn).mock.calls)).not.toContain( + "secret-token", + ); + }, + ); + it("should clear session auth", async () => { await secretsManager.setSessionAuth("example.com", { url: "https://example.com", @@ -85,7 +152,7 @@ describe("SecretsManager", () => { "example.com", ); - await secretsManager.setSessionAuth("other-com", { + await secretsManager.setSessionAuth("other.com", { url: "https://other.com", token: "other-token", }); @@ -93,7 +160,7 @@ describe("SecretsManager", () => { "example.com", ); expect(await secretsManager.getKnownSafeHostnames()).toContain( - "other-com", + "other.com", ); }); @@ -327,9 +394,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 +414,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 +486,6 @@ describe("SecretsManager", () => { describe("backwards compatibility", () => { interface BackwardsCompatTestCase { name: string; - key: string; data: Record; expected: unknown; } @@ -427,13 +493,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 +513,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/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 0dc86b1b30..ab44a658c4 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -178,6 +178,28 @@ function createTestContext(telemetry?: TelemetryService) { } describe("LoginCoordinator", () => { + describe("hostname validation", () => { + it.each([ + { name: "malformed URL", url: "not a URL" }, + { name: "mismatched hostname", url: "https://other.example.com" }, + ])("rejects a $name before authentication", async ({ url }) => { + const { coordinator, logger } = createTestContext(); + + const result = await coordinator.ensureLoggedIn({ + url, + safeHostname: TEST_HOSTNAME, + token: "provided-token", + }); + + expect(result).toEqual({ success: false, reason: "auth_failed" }); + expect(mockGetAuthenticatedUser).not.toHaveBeenCalled(); + expect(logger.warn).toHaveBeenCalledWith( + "Ignoring login with invalid deployment hostname", + expect.objectContaining({ safeHostname: TEST_HOSTNAME }), + ); + }); + }); + describe("token authentication", () => { it("authenticates with stored token on success", async () => { const { secretsManager, coordinator, mockSuccessfulAuth } = @@ -413,6 +435,21 @@ describe("LoginCoordinator", () => { }); describe("ensureLoggedInWithDialog", () => { + it("returns auth failure when the selected URL hostname mismatches", async () => { + const { userInteraction, coordinator } = createTestContext(); + userInteraction.setResponse("Authentication Required", "Login"); + vi.mocked(maybeAskUrl).mockResolvedValue("https://other.example.com"); + + const result = await coordinator.ensureLoggedInWithDialog({ + url: undefined, + safeHostname: TEST_HOSTNAME, + trigger: "missing_session", + }); + + expect(result).toEqual({ success: false, reason: "auth_failed" }); + expect(mockGetAuthenticatedUser).not.toHaveBeenCalled(); + }); + it("returns success false when user dismisses dialog", async () => { const { mockConfig, userInteraction, coordinator } = createTestContext(); // Use mTLS for simpler dialog test 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..0b124f15c7 --- /dev/null +++ b/test/unit/remote/remote.test.ts @@ -0,0 +1,103 @@ +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; + +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, + MockConfigurationProvider, +} from "../../mocks/testHelpers"; + +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 { LoginCoordinator } from "@/login/loginCoordinator"; + +const SAFE_HOSTNAME = "coder.example.com"; +const REMOTE_AUTHORITY = + "ssh-remote+coder-vscode.coder.example.com--testuser--test-workspace.main"; + +describe("Remote", () => { + let testDir: string; + + beforeEach(async () => { + vi.clearAllMocks(); + new MockConfigurationProvider(); + testDir = await fs.mkdtemp(path.join(os.tmpdir(), "remote-test-")); + }); + + afterEach(async () => { + await fs.rm(testDir, { recursive: true, force: true }); + }); + + it("ignores mismatched file auth and falls through to login", async () => { + const pathResolver = new PathResolver(testDir, "/code/log"); + await fs.mkdir(pathResolver.getGlobalConfigDir(SAFE_HOSTNAME), { + recursive: true, + }); + await Promise.all([ + fs.writeFile( + pathResolver.getUrlPath(SAFE_HOSTNAME), + "https://cursor.example.com", + ), + fs.writeFile(pathResolver.getSessionTokenPath(SAFE_HOSTNAME), "token"), + ]); + + const logger = createMockLogger(); + 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; + const remote = new Remote( + serviceContainer, + {} as Commands, + {} as vscode.ExtensionContext, + ); + + await expect( + remote.setup(REMOTE_AUTHORITY, "none", "anysphere.remote-ssh"), + ).resolves.toBeUndefined(); + + expect(await secretsManager.getSessionAuth(SAFE_HOSTNAME)).toBeUndefined(); + expect(logger.warn).toHaveBeenCalledWith( + "Ignoring invalid session auth migration", + { + safeHostname: SAFE_HOSTNAME, + authHostname: "cursor.example.com", + }, + ); + expect(ensureLoggedInWithDialog).toHaveBeenCalledWith( + expect.objectContaining({ + safeHostname: SAFE_HOSTNAME, + trigger: "missing_session", + }), + ); + expect(vscode.commands.executeCommand).toHaveBeenCalledWith( + "workbench.action.remote.close", + ); + }); +}); From a5e09d6d50d3d420d285944f60c9794c54c866d9 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 3 Aug 2026 12:18:16 +0000 Subject: [PATCH 2/4] refactor: simplify session auth validation --- src/core/secretsManager.ts | 53 ++++++++---------------- src/login/loginCoordinator.ts | 32 +------------- src/remote/remote.ts | 15 ++----- test/unit/core/secretsManager.test.ts | 45 ++++++++------------ test/unit/login/loginCoordinator.test.ts | 37 ----------------- test/unit/remote/remote.test.ts | 7 +--- 6 files changed, 42 insertions(+), 147 deletions(-) diff --git a/src/core/secretsManager.ts b/src/core/secretsManager.ts index 2bfc4a3099..24684ca1e0 100644 --- a/src/core/secretsManager.ts +++ b/src/core/secretsManager.ts @@ -51,31 +51,6 @@ const SessionAuthSchema = z.object({ export type SessionAuth = z.infer; -export class SessionAuthHostnameError extends Error { - public constructor( - public readonly safeHostname: string, - public readonly authHostname?: string, - ) { - super("Session auth URL does not match its deployment hostname"); - this.name = "SessionAuthHostnameError"; - } -} - -export function assertSessionAuthHostname( - safeHostname: string, - url: string, -): void { - let authHostname: string; - try { - authHostname = toSafeHost(url); - } catch { - throw new SessionAuthHostnameError(safeHostname); - } - if (authHostname !== safeHostname) { - throw new SessionAuthHostnameError(safeHostname, authHostname); - } -} - export class SecretsManager { constructor( private readonly secrets: SecretStorage, @@ -87,6 +62,20 @@ export class SecretsManager { return `${prefix}${safeHostname}`; } + private assertSessionAuthHostname(safeHostname: string, url: string): void { + let authHostname: string | undefined; + try { + authHostname = toSafeHost(url); + } catch { + authHostname = undefined; + } + if (authHostname !== safeHostname) { + throw new Error( + "Session auth URL does not match its deployment hostname", + ); + } + } + private async getSecret( prefix: SecretKeyPrefix, safeHostname: string, @@ -218,17 +207,11 @@ export class SecretsManager { return undefined; } try { - assertSessionAuthHostname(safeHostname, result.data.url); - } catch (error) { - if (!(error instanceof SessionAuthHostnameError)) { - throw error; - } + this.assertSessionAuthHostname(safeHostname, result.data.url); + } catch { this.logger.warn( "Ignoring session auth with invalid deployment hostname", - { - safeHostname: error.safeHostname, - authHostname: error.authHostname ?? "(invalid URL)", - }, + { safeHostname }, ); return undefined; } @@ -241,7 +224,7 @@ export class SecretsManager { ): Promise { // Parse through schema to strip any extra fields const state = SessionAuthSchema.parse(auth); - assertSessionAuthHostname(safeHostname, state.url); + this.assertSessionAuthHostname(safeHostname, state.url); await this.setSecret(SESSION_KEY_PREFIX, safeHostname, state); } diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index b42a6f692b..0023b95dbc 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -4,12 +4,6 @@ import * as vscode from "vscode"; import { CoderApi } from "../api/coderApi"; import { needToken } from "../api/utils"; -import { - assertSessionAuthHostname, - SessionAuthHostnameError, - type OAuthTokenData, - type SecretsManager, -} from "../core/secretsManager"; import { CertificateError } from "../error/certificateError"; import { OAuthAuthorizer } from "../oauth/authorizer"; import { buildOAuthTokenData } from "../oauth/utils"; @@ -23,6 +17,7 @@ import type { User } from "coder/site/src/api/typesGenerated"; import type { CliCredentialManager } from "../core/cliCredentialManager"; import type { MementoManager } from "../core/mementoManager"; +import type { OAuthTokenData, SecretsManager } from "../core/secretsManager"; import type { Deployment } from "../deployment/types"; import type { AuthLoginPromptTrigger, @@ -93,9 +88,6 @@ export class LoginCoordinator implements vscode.Disposable { options: LoginOptions & { url: string }, ): Promise { const { safeHostname, url } = options; - if (!this.hasValidSessionAuthHostname(safeHostname, url)) { - return Promise.resolve({ success: false, reason: "auth_failed" }); - } return this.executeWithGuard(async () => { const result = await this.attemptLogin( { safeHostname, url }, @@ -155,9 +147,6 @@ export class LoginCoordinator implements vscode.Disposable { if (!newUrl) { return { success: false, reason: "no_url_provided" }; } - if (!this.hasValidSessionAuthHostname(safeHostname, newUrl)) { - return { success: false, reason: "auth_failed" }; - } const result = await this.attemptLogin( { url: newUrl, safeHostname }, @@ -186,25 +175,6 @@ export class LoginCoordinator implements vscode.Disposable { }); } - private hasValidSessionAuthHostname( - safeHostname: string, - url: string, - ): boolean { - try { - assertSessionAuthHostname(safeHostname, url); - return true; - } catch (error) { - if (!(error instanceof SessionAuthHostnameError)) { - throw error; - } - this.logger.warn("Ignoring login with invalid deployment hostname", { - safeHostname: error.safeHostname, - authHostname: error.authHostname ?? "(invalid URL)", - }); - return false; - } - } - private async persistSessionAuth( result: LoginAttemptResult, safeHostname: string, diff --git a/src/remote/remote.ts b/src/remote/remote.ts index fc2d0fb69b..17b2140133 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -20,10 +20,6 @@ import { watchConfigurationChanges, } from "../configWatcher"; import { version as cliVersion } from "../core/cliExec"; -import { - SessionAuthHostnameError, - type SecretsManager, -} from "../core/secretsManager"; import { toError } from "../error/errorUtils"; import { featureSetForVersion, type FeatureSet } from "../featureSet"; import { Inbox } from "../inbox"; @@ -76,6 +72,7 @@ import type { ServiceContainer } from "../core/container"; import type { ContextManager } from "../core/contextManager"; import type { StartupMode } from "../core/mementoManager"; import type { PathResolver } from "../core/pathResolver"; +import type { SecretsManager } from "../core/secretsManager"; import type { Logger } from "../logging/logger"; import type { LoginCoordinator } from "../login/loginCoordinator"; @@ -831,13 +828,9 @@ export class Remote { url: url.value.trim(), token: token.value.trim(), }); - } catch (error) { - if (!(error instanceof SessionAuthHostnameError)) { - throw error; - } - this.logger.warn("Ignoring invalid session auth migration", { - safeHostname: error.safeHostname, - authHostname: error.authHostname ?? "(invalid URL)", + } catch { + this.logger.warn("Failed to migrate session auth from files", { + safeHostname, }); } } diff --git a/test/unit/core/secretsManager.test.ts b/test/unit/core/secretsManager.test.ts index 6e770a7368..0baa803e75 100644 --- a/test/unit/core/secretsManager.test.ts +++ b/test/unit/core/secretsManager.test.ts @@ -81,39 +81,28 @@ describe("SecretsManager", () => { }); it.each([ - { - name: "malformed URL", - url: "not a URL", - expectedHostname: "(invalid URL)", - }, + { name: "malformed URL", url: "not a URL" }, { name: "mismatched hostname", url: "https://other.example.com/private?token=secret", - expectedHostname: "other.example.com", }, - ])( - "should ignore stored auth with a $name", - async ({ url, expectedHostname }) => { - await secretStorage.store( - "coder.session.example.com", - JSON.stringify({ url, token: "secret-token" }), - ); + ])("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(); - expect(logger.warn).toHaveBeenCalledWith( - "Ignoring session auth with invalid deployment hostname", - { - safeHostname: "example.com", - authHostname: expectedHostname, - }, - ); - expect(JSON.stringify(vi.mocked(logger.warn).mock.calls)).not.toContain( - "secret-token", - ); - }, - ); + expect( + await secretsManager.getSessionAuth("example.com"), + ).toBeUndefined(); + expect(logger.warn).toHaveBeenCalledWith( + "Ignoring session auth with invalid deployment hostname", + { safeHostname: "example.com" }, + ); + expect(JSON.stringify(vi.mocked(logger.warn).mock.calls)).not.toContain( + "secret-token", + ); + }); it("should clear session auth", async () => { await secretsManager.setSessionAuth("example.com", { diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index ab44a658c4..0dc86b1b30 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -178,28 +178,6 @@ function createTestContext(telemetry?: TelemetryService) { } describe("LoginCoordinator", () => { - describe("hostname validation", () => { - it.each([ - { name: "malformed URL", url: "not a URL" }, - { name: "mismatched hostname", url: "https://other.example.com" }, - ])("rejects a $name before authentication", async ({ url }) => { - const { coordinator, logger } = createTestContext(); - - const result = await coordinator.ensureLoggedIn({ - url, - safeHostname: TEST_HOSTNAME, - token: "provided-token", - }); - - expect(result).toEqual({ success: false, reason: "auth_failed" }); - expect(mockGetAuthenticatedUser).not.toHaveBeenCalled(); - expect(logger.warn).toHaveBeenCalledWith( - "Ignoring login with invalid deployment hostname", - expect.objectContaining({ safeHostname: TEST_HOSTNAME }), - ); - }); - }); - describe("token authentication", () => { it("authenticates with stored token on success", async () => { const { secretsManager, coordinator, mockSuccessfulAuth } = @@ -435,21 +413,6 @@ describe("LoginCoordinator", () => { }); describe("ensureLoggedInWithDialog", () => { - it("returns auth failure when the selected URL hostname mismatches", async () => { - const { userInteraction, coordinator } = createTestContext(); - userInteraction.setResponse("Authentication Required", "Login"); - vi.mocked(maybeAskUrl).mockResolvedValue("https://other.example.com"); - - const result = await coordinator.ensureLoggedInWithDialog({ - url: undefined, - safeHostname: TEST_HOSTNAME, - trigger: "missing_session", - }); - - expect(result).toEqual({ success: false, reason: "auth_failed" }); - expect(mockGetAuthenticatedUser).not.toHaveBeenCalled(); - }); - it("returns success false when user dismisses dialog", async () => { const { mockConfig, userInteraction, coordinator } = createTestContext(); // Use mTLS for simpler dialog test diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts index 0b124f15c7..ae2854aaac 100644 --- a/test/unit/remote/remote.test.ts +++ b/test/unit/remote/remote.test.ts @@ -84,11 +84,8 @@ describe("Remote", () => { expect(await secretsManager.getSessionAuth(SAFE_HOSTNAME)).toBeUndefined(); expect(logger.warn).toHaveBeenCalledWith( - "Ignoring invalid session auth migration", - { - safeHostname: SAFE_HOSTNAME, - authHostname: "cursor.example.com", - }, + "Failed to migrate session auth from files", + { safeHostname: SAFE_HOSTNAME }, ); expect(ensureLoggedInWithDialog).toHaveBeenCalledWith( expect.objectContaining({ From 966d11866c1c2bfd0241a3835500611dee27f813 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 3 Aug 2026 15:18:25 +0000 Subject: [PATCH 3/4] test: focus session auth coverage on outputs --- src/core/secretsManager.ts | 14 ++- test/mocks/testHelpers.ts | 45 +++++++++ test/unit/core/secretsManager.test.ts | 65 +++++++++--- test/unit/remote/remote.test.ts | 136 ++++++++++++++------------ 4 files changed, 177 insertions(+), 83 deletions(-) diff --git a/src/core/secretsManager.ts b/src/core/secretsManager.ts index 24684ca1e0..3ff095a7d1 100644 --- a/src/core/secretsManager.ts +++ b/src/core/secretsManager.ts @@ -63,15 +63,17 @@ export class SecretsManager { } private assertSessionAuthHostname(safeHostname: string, url: string): void { - let authHostname: string | undefined; + let authHostname: string; try { authHostname = toSafeHost(url); } catch { - authHostname = undefined; + throw new Error( + `Session auth hostname mismatch: expected "${safeHostname}", got an invalid URL`, + ); } if (authHostname !== safeHostname) { throw new Error( - "Session auth URL does not match its deployment hostname", + `Session auth hostname mismatch: expected "${safeHostname}", got "${authHostname}"`, ); } } @@ -218,6 +220,12 @@ export class SecretsManager { 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, 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 0baa803e75..3eb329a5b7 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"; @@ -59,25 +60,35 @@ describe("SecretsManager", () => { }); it.each([ - { name: "malformed URL", url: "not a URL" }, + { + name: "malformed URL", + url: "not a URL", + error: + 'Session auth hostname mismatch: expected "example.com", got an invalid 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 }) => { + ])("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( - "Session auth URL does not match its deployment hostname", - ); + ).rejects.toThrow(error); - expect( - await secretStorage.get("coder.session.example.com"), - ).toBeUndefined(); - expect(mementoManager.getDeploymentAccess("example.com")).toBeUndefined(); + expect(await secretsManager.getSessionAuth("example.com")).toEqual( + existingAuth, + ); }); it.each([ @@ -95,13 +106,35 @@ describe("SecretsManager", () => { expect( await secretsManager.getSessionAuth("example.com"), ).toBeUndefined(); - expect(logger.warn).toHaveBeenCalledWith( - "Ignoring session auth with invalid deployment hostname", - { safeHostname: "example.com" }, - ); - expect(JSON.stringify(vi.mocked(logger.warn).mock.calls)).not.toContain( - "secret-token", - ); + }); + + describe("logging", () => { + it.each([ + { name: "malformed URL", url: "not a URL" }, + { + name: "mismatched hostname", + url: "https://other.example.com/private?token=secret", + }, + ])("logs a sanitized warning for a $name", async ({ url }) => { + 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 session auth with invalid deployment hostname", + args: [{ safeHostname: "example.com" }], + }, + ]); + expect(JSON.stringify(logs.entries)).not.toContain("secret-token"); + expect(JSON.stringify(logs.entries)).not.toContain(url); + }); }); it("should clear session auth", async () => { diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts index ae2854aaac..8a53cc14dc 100644 --- a/test/unit/remote/remote.test.ts +++ b/test/unit/remote/remote.test.ts @@ -1,8 +1,5 @@ -import * as fs from "node:fs/promises"; -import * as os from "node:os"; -import * as path from "node:path"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import * as vscode from "vscode"; +import { vol } from "memfs"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import { MementoManager } from "@/core/mementoManager"; import { PathResolver } from "@/core/pathResolver"; @@ -14,87 +11,98 @@ 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"; -describe("Remote", () => { - let testDir: string; - - beforeEach(async () => { - vi.clearAllMocks(); - new MockConfigurationProvider(); - testDir = await fs.mkdtemp(path.join(os.tmpdir(), "remote-test-")); +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; - afterEach(async () => { - await fs.rm(testDir, { recursive: true, force: true }); - }); - - it("ignores mismatched file auth and falls through to login", async () => { - const pathResolver = new PathResolver(testDir, "/code/log"); - await fs.mkdir(pathResolver.getGlobalConfigDir(SAFE_HOSTNAME), { - recursive: true, - }); - await Promise.all([ - fs.writeFile( - pathResolver.getUrlPath(SAFE_HOSTNAME), - "https://cursor.example.com", - ), - fs.writeFile(pathResolver.getSessionTokenPath(SAFE_HOSTNAME), "token"), - ]); - - const logger = createMockLogger(); - 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; - const remote = new Remote( + 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(); - expect(logger.warn).toHaveBeenCalledWith( - "Failed to migrate session auth from files", - { safeHostname: SAFE_HOSTNAME }, - ); - expect(ensureLoggedInWithDialog).toHaveBeenCalledWith( - expect.objectContaining({ - safeHostname: SAFE_HOSTNAME, - trigger: "missing_session", - }), - ); - expect(vscode.commands.executeCommand).toHaveBeenCalledWith( - "workbench.action.remote.close", - ); + }); + + describe("logging", () => { + it("sanitizes the file auth migration warning", async () => { + const logs = new LogCollector(); + const { remote } = createRemote(logs); + + await remote.setup(REMOTE_AUTHORITY, "none", "anysphere.remote-ssh"); + + const warning = logs.entries.find( + (entry) => + entry.level === "warn" && + entry.message === "Failed to migrate session auth from files", + ); + expect(warning).toEqual({ + level: "warn", + message: "Failed to migrate session auth from files", + args: [{ safeHostname: SAFE_HOSTNAME }], + }); + expect(JSON.stringify(warning)).not.toContain(MISMATCHED_URL); + expect(JSON.stringify(warning)).not.toContain(SESSION_TOKEN); + }); }); }); From f9800272859fcb65eb378007f99b25e9b5575260 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 4 Aug 2026 11:47:12 +0300 Subject: [PATCH 4/4] fix: log why session auth was rejected Include the offending URL in the invalid-URL error and log the error itself at both call sites instead of a generic message. The mismatch branch still reports only hostnames, so a stored URL carrying credentials does not reach the log. --- src/core/secretsManager.ts | 9 +++------ src/remote/remote.ts | 6 ++---- test/unit/core/secretsManager.test.ts | 20 +++++++++++++------- test/unit/remote/remote.test.ts | 22 +++++++++++----------- 4 files changed, 29 insertions(+), 28 deletions(-) diff --git a/src/core/secretsManager.ts b/src/core/secretsManager.ts index 3ff095a7d1..093a469ac9 100644 --- a/src/core/secretsManager.ts +++ b/src/core/secretsManager.ts @@ -68,7 +68,7 @@ export class SecretsManager { authHostname = toSafeHost(url); } catch { throw new Error( - `Session auth hostname mismatch: expected "${safeHostname}", got an invalid URL`, + `Session auth hostname mismatch: expected "${safeHostname}", got an invalid URL "${url}"`, ); } if (authHostname !== safeHostname) { @@ -210,11 +210,8 @@ export class SecretsManager { } try { this.assertSessionAuthHostname(safeHostname, result.data.url); - } catch { - this.logger.warn( - "Ignoring session auth with invalid deployment hostname", - { safeHostname }, - ); + } catch (error) { + this.logger.warn("Ignoring stored session auth:", error); return undefined; } return result.data; diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 17b2140133..00c855c529 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -828,10 +828,8 @@ export class Remote { url: url.value.trim(), token: token.value.trim(), }); - } catch { - this.logger.warn("Failed to migrate session auth from files", { - safeHostname, - }); + } catch (error) { + this.logger.warn("Failed to migrate session auth from files:", error); } } } diff --git a/test/unit/core/secretsManager.test.ts b/test/unit/core/secretsManager.test.ts index 3eb329a5b7..1ab29e5451 100644 --- a/test/unit/core/secretsManager.test.ts +++ b/test/unit/core/secretsManager.test.ts @@ -64,7 +64,7 @@ describe("SecretsManager", () => { name: "malformed URL", url: "not a URL", error: - 'Session auth hostname mismatch: expected "example.com", got an invalid URL', + 'Session auth hostname mismatch: expected "example.com", got an invalid URL "not a URL"', }, { name: "mismatched hostname", @@ -110,12 +110,20 @@ describe("SecretsManager", () => { describe("logging", () => { it.each([ - { name: "malformed URL", url: "not a URL" }, { + 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 a sanitized warning for a $name", async ({ url }) => { + ])("logs why a $name was ignored", async ({ url, error }) => { const logs = new LogCollector(); const manager = new SecretsManager(secretStorage, mementoManager, logs); await secretStorage.store( @@ -128,12 +136,10 @@ describe("SecretsManager", () => { expect(logs.entries).toEqual([ { level: "warn", - message: "Ignoring session auth with invalid deployment hostname", - args: [{ safeHostname: "example.com" }], + message: "Ignoring stored session auth:", + args: [new Error(error)], }, ]); - expect(JSON.stringify(logs.entries)).not.toContain("secret-token"); - expect(JSON.stringify(logs.entries)).not.toContain(url); }); }); diff --git a/test/unit/remote/remote.test.ts b/test/unit/remote/remote.test.ts index 8a53cc14dc..835240fc92 100644 --- a/test/unit/remote/remote.test.ts +++ b/test/unit/remote/remote.test.ts @@ -85,24 +85,24 @@ describe("Remote", () => { }); describe("logging", () => { - it("sanitizes the file auth migration warning", async () => { + 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"); - const warning = logs.entries.find( - (entry) => - entry.level === "warn" && - entry.message === "Failed to migrate session auth from files", - ); - expect(warning).toEqual({ + // 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: [{ safeHostname: SAFE_HOSTNAME }], + message: "Failed to migrate session auth from files:", + args: [ + new Error( + `Session auth hostname mismatch: expected "${SAFE_HOSTNAME}", got "cursor.example.com"`, + ), + ], }); - expect(JSON.stringify(warning)).not.toContain(MISMATCHED_URL); - expect(JSON.stringify(warning)).not.toContain(SESSION_TOKEN); }); }); });