From ea3a70a40d9a75daa144ec404a00346619b2eaea Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 20 Oct 2026 17:00:17 +0300 Subject: [PATCH] fix(oauth): keep OAuth endpoints and requests on the deployment origin - Reject OAuth metadata endpoints off the deployment origin or under a workspace app path, without echoing them. - Send OAuth POSTs with maxRedirects: 0. - Show OAuth failures in a modal's plain detail, not a notification that renders command links. - Clean up superseded or disposed flows, and open the browser without awaiting it so early rejections reach the caller. - Ignore a coder.alternativeWebUrl that is not http(s). --- CHANGELOG.md | 5 ++ src/common/url.ts | 7 ++- src/login/loginCoordinator.ts | 11 +++- src/oauth/authorizer.ts | 32 ++++++---- src/oauth/metadataClient.ts | 27 +++++++++ src/oauth/revocation.ts | 1 + src/oauth/sessionManager.ts | 1 + test/unit/common/url.test.ts | 2 + test/unit/login/loginCoordinator.test.ts | 23 +++++++ test/unit/oauth/authorizer.test.ts | 76 +++++++++++++++++++++++- test/unit/oauth/metadataClient.test.ts | 23 +++++++ test/unit/oauth/sessionManager.test.ts | 19 ++++++ 12 files changed, 205 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4dd9f0827..c6fdfc59ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,11 @@ ``!#$%&'*+-.^_`|~``, so whitespace (including tabs and carriage returns) and `:` are not allowed. An invisible byte order mark at the start of the output, which some Windows editors and PowerShell add, is ignored. +- OAuth endpoints off the deployment's origin or on a workspace app are now + refused, and OAuth requests no longer follow redirects. +- OAuth sign-in errors are now shown in a dialog, where server text cannot + become a link. +- A `coder.alternativeWebUrl` that is not an http(s) URL is now ignored. ## [v1.17.2](https://github.com/coder/vscode-coder/releases/tag/v1.17.2) 2026-10-28 diff --git a/src/common/url.ts b/src/common/url.ts index f342698d9c..acdcc1d98a 100644 --- a/src/common/url.ts +++ b/src/common/url.ts @@ -54,8 +54,8 @@ export function normalizeUrl(value: string): string { /** * Return the URL for opening Coder pages in the browser. Uses the - * `coder.alternativeWebUrl` setting when configured, otherwise returns - * the connection URL unchanged. + * `coder.alternativeWebUrl` setting when it is an http(s) URL, otherwise + * returns the connection URL unchanged. */ export function resolveCoderDashboardUrl(connectionUrl: string): string { const alt = normalizeUrl( @@ -63,7 +63,8 @@ export function resolveCoderDashboardUrl(connectionUrl: string): string { .getConfiguration("coder") .get("alternativeWebUrl") ?? "", ); - return alt || connectionUrl; + const protocol = URL.parse(alt)?.protocol; + return protocol === "http:" || protocol === "https:" ? alt : connectionUrl; } /** diff --git a/src/login/loginCoordinator.ts b/src/login/loginCoordinator.ts index 3dd0c94b2d..1b11de023f 100644 --- a/src/login/loginCoordinator.ts +++ b/src/login/loginCoordinator.ts @@ -685,10 +685,15 @@ export class LoginCoordinator implements vscode.Disposable { this.logger.error(title, error); if (error instanceof CertificateError) { void error.showNotification(title); + } else if (error instanceof CrossOriginRedirectError) { + void error.showNotification(title); } else { - vscode.window.showErrorMessage( - `${title}: ${getErrorMessage(error, "Unknown error")}`, - ); + // Notifications render `[text](command:...)` links from server text. + void vscodeProposed.window.showErrorMessage(title, { + detail: getErrorMessage(error, "Unknown error"), + modal: true, + useCustom: true, + }); } return { success: false, reason: "auth_failed" }; } diff --git a/src/oauth/authorizer.ts b/src/oauth/authorizer.ts index b456c3ee7d..f81554f980 100644 --- a/src/oauth/authorizer.ts +++ b/src/oauth/authorizer.ts @@ -181,6 +181,7 @@ export class OAuthAuthorizer implements vscode.Disposable { const response = await axiosInstance.post( metadata.registration_endpoint, registrationRequest, + { maxRedirects: 0 }, ); const registrationResponse = parseOAuthResponse( @@ -281,7 +282,10 @@ export class OAuthAuthorizer implements vscode.Disposable { if (this.pendingAuthReject) { this.pendingAuthReject(new Error("New OAuth flow started")); } - this.pendingAuthReject = reject; + this.pendingAuthReject = (error) => { + cleanup(); + reject(error); + }; const timeoutMins = 5; const timeoutHandle = setTimeout( @@ -297,10 +301,7 @@ export class OAuthAuthorizer implements vscode.Disposable { const listener = this.oauthCallback.onReceive( ({ state: callbackState, code, error }) => { if (callbackState !== state) { - this.logger.warn( - "Ignoring OAuth callback with mismatched state", - { expected: state, received: callbackState }, - ); + this.logger.warn("Ignoring OAuth callback with mismatched state"); return; } @@ -329,17 +330,21 @@ export class OAuthAuthorizer implements vscode.Disposable { listener.dispose(); cancellationListener.dispose(); }; + + // Not awaited, so a rejection while the browser opens reaches the caller. + Promise.resolve( + vscode.env.openExternal(vscode.Uri.parse(authUrl)), + ).catch((error: unknown) => { + cleanup(); + reject( + error instanceof Error + ? error + : new Error("Failed to open browser"), + ); + }); }, ); - try { - await vscode.env.openExternal(vscode.Uri.parse(authUrl)); - } catch (error) { - throw error instanceof Error - ? error - : new Error("Failed to open browser"); - } - return callbackPromise; } @@ -370,6 +375,7 @@ export class OAuthAuthorizer implements vscode.Disposable { metadata.token_endpoint, tokenRequest, { + maxRedirects: 0, headers: { "Content-Type": "application/x-www-form-urlencoded", }, diff --git a/src/oauth/metadataClient.ts b/src/oauth/metadataClient.ts index a8adc9e30e..4751f8a2c1 100644 --- a/src/oauth/metadataClient.ts +++ b/src/oauth/metadataClient.ts @@ -1,5 +1,6 @@ import { CoderApi } from "../api/coderApi"; import { parseApiResponse } from "../api/responseValidation"; +import { isSameOrigin } from "../common/url"; import { AUTH_GRANT_TYPE, @@ -22,6 +23,9 @@ import type { Logger } from "../logging/logger"; const OAUTH_DISCOVERY_ENDPOINT = "/.well-known/oauth-authorization-server"; +/** Path-based workspace apps (`/@user/...`) serve user content on our origin. */ +const WORKSPACE_APP_PATH = /\/(?:@|%40)/i; + const REQUIRED_GRANT_TYPES: readonly OAuth2ProviderGrantType[] = [ AUTH_GRANT_TYPE, REFRESH_GRANT_TYPE, @@ -97,6 +101,7 @@ export class OAuthMetadataClient { this.axiosInstance.defaults.baseURL, ); + this.validateEndpoints(metadata); this.validateGrantTypes(metadata); this.validateResponseTypes(metadata); this.validateAuthMethods(metadata); @@ -112,6 +117,28 @@ export class OAuthMetadataClient { return metadata; } + /** Endpoints receive secrets, so they must belong to the deployment. */ + private validateEndpoints(metadata: OAuth2AuthorizationServerMetadata): void { + const deploymentUrl = this.axiosInstance.defaults.baseURL ?? ""; + for (const name of [ + "authorization_endpoint", + "token_endpoint", + "registration_endpoint", + "revocation_endpoint", + ] as const) { + const endpoint = metadata[name]; + if ( + endpoint && + (!isSameOrigin(endpoint, deploymentUrl) || + WORKSPACE_APP_PATH.test(new URL(endpoint).pathname)) + ) { + throw new Error( + `OAuth server metadata ${name} is outside the deployment`, + ); + } + } + } + private validateGrantTypes( metadata: OAuth2AuthorizationServerMetadata, ): void { diff --git a/src/oauth/revocation.ts b/src/oauth/revocation.ts index 935ba3c959..630ceb8b8b 100644 --- a/src/oauth/revocation.ts +++ b/src/oauth/revocation.ts @@ -52,6 +52,7 @@ export async function revokeOAuthTokens( }; try { await axiosInstance.post(endpoint, toUrlSearchParams(params), { + maxRedirects: 0, headers: { "Content-Type": "application/x-www-form-urlencoded" }, }); logger.debug(`Revoked ${token_type_hint}`); diff --git a/src/oauth/sessionManager.ts b/src/oauth/sessionManager.ts index 044987e069..6875614b80 100644 --- a/src/oauth/sessionManager.ts +++ b/src/oauth/sessionManager.ts @@ -380,6 +380,7 @@ export class OAuthSessionManager implements vscode.Disposable { metadata.token_endpoint, toUrlSearchParams(params), { + maxRedirects: 0, headers: { "Content-Type": "application/x-www-form-urlencoded", }, diff --git a/test/unit/common/url.test.ts b/test/unit/common/url.test.ts index f5525b56b2..01fccee47f 100644 --- a/test/unit/common/url.test.ts +++ b/test/unit/common/url.test.ts @@ -133,6 +133,8 @@ describe("resolveCoderDashboardUrl", () => { it.each([ { name: "empty", value: "" }, { name: "whitespace", value: " " }, + { name: "not http(s)", value: "vscode://some.extension/path" }, + { name: "missing a scheme", value: "coder.example.com" }, ])( "returns the connection URL when the alternative is $name", ({ value }) => { diff --git a/test/unit/login/loginCoordinator.test.ts b/test/unit/login/loginCoordinator.test.ts index 1227f60a14..71e7f1e82b 100644 --- a/test/unit/login/loginCoordinator.test.ts +++ b/test/unit/login/loginCoordinator.test.ts @@ -432,6 +432,29 @@ describe("LoginCoordinator", () => { ); }); + it("shows OAuth errors only in a modal's plain detail", async () => { + const { coordinator } = createTestContext(); + const payload = "[Retry](command:workbench.action.terminal.new)"; + vi.mocked(maybeAskAuthMethod).mockResolvedValue("oauth"); + vi.spyOn(OAuthAuthorizer.prototype, "login").mockRejectedValue( + new Error(`OAuth error: ${payload}`), + ); + + const result = await coordinator.ensureLoggedIn({ + url: TEST_URL, + safeHostname: TEST_HOSTNAME, + }); + + expect(result.success).toBe(false); + expect(vscode.window.showErrorMessage).toHaveBeenCalledExactlyOnceWith( + "OAuth authentication failed", + expect.objectContaining({ + modal: true, + detail: `OAuth error: ${payload}`, + }), + ); + }); + it("shows error and returns failure when mTLS fails", async () => { const { mockConfig, coordinator, mockAuthFailure } = createTestContext(); mockConfig.set("coder.tlsCertFile", "/path/to/cert.pem"); diff --git a/test/unit/oauth/authorizer.test.ts b/test/unit/oauth/authorizer.test.ts index ba5afac99e..e971ac5d9b 100644 --- a/test/unit/oauth/authorizer.test.ts +++ b/test/unit/oauth/authorizer.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { describe, expect, it, onTestFinished, vi } from "vitest"; import * as vscode from "vscode"; import { InvalidApiResponseError } from "@/api/responseValidation"; @@ -121,8 +121,13 @@ async function waitForBrowserToOpen(): Promise<{ describe("OAuthAuthorizer", () => { describe("login flow", () => { it("completes full OAuth login flow successfully", async () => { - const { setupOAuthRoutes, oauthCallback, secretsManager, authorizer } = - createTestContext(); + const { + mockAdapter, + setupOAuthRoutes, + oauthCallback, + secretsManager, + authorizer, + } = createTestContext(); setupOAuthRoutes(undefined, { "/oauth2/register": createMockClientRegistration({ @@ -162,6 +167,15 @@ describe("OAuthAuthorizer", () => { const storedRegistration = await secretsManager.getOAuthClientRegistration(TEST_HOSTNAME); expect(storedRegistration?.client_id).toBe("registered-client-id"); + + for (const path of ["/oauth2/register", "/oauth2/token"]) { + expect(mockAdapter).toHaveBeenCalledWith( + expect.objectContaining({ + url: `${TEST_URL}${path}`, + maxRedirects: 0, + }), + ); + } }); it("uses existing client registration when redirect URI matches", async () => { @@ -524,6 +538,44 @@ describe("OAuthAuthorizer", () => { await expect(loginPromise).rejects.toThrow("OAuthAuthorizer disposed"); }); + it("rejects when disposed while the browser is opening", async () => { + const { authorizer, setupOAuthRoutes, startLogin } = createTestContext(); + setupOAuthRoutes(); + vi.mocked(vscode.env.openExternal).mockReturnValueOnce( + new Promise(() => undefined), + ); + + const { loginPromise } = await startLogin(); + authorizer.dispose(); + + await expect(loginPromise).rejects.toThrow("OAuthAuthorizer disposed"); + }); + + it("keeps a newer flow cancellable after the one it replaced times out", async () => { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + onTestFinished(() => { + vi.useRealTimers(); + }); + const { authorizer, setupOAuthRoutes, startLogin } = createTestContext(); + setupOAuthRoutes(); + + const first = await startLogin(); + vi.advanceTimersByTime(60_000); + const second = authorizer.login( + createTestDeployment(), + new MockProgress(), + new MockCancellationToken(), + ); + await expect(first.loginPromise).rejects.toThrow( + "New OAuth flow started", + ); + // Past the first flow's timeout, before the second's. + vi.advanceTimersByTime(4.5 * 60_000); + authorizer.dispose(); + + await expect(second).rejects.toThrow("OAuthAuthorizer disposed"); + }); + it("does nothing when disposed without pending auth", () => { const { authorizer } = createTestContext(); expect(() => authorizer.dispose()).not.toThrow(); @@ -531,6 +583,24 @@ describe("OAuthAuthorizer", () => { }); describe("error handling", () => { + it("rejects when the browser fails to open", async () => { + const { authorizer, setupOAuthRoutes } = createTestContext(); + setupOAuthRoutes(); + vi.mocked(vscode.env.openExternal).mockRejectedValueOnce( + new Error("No browser available"), + ); + + await expect( + authorizer.login( + createTestDeployment(), + new MockProgress(), + new MockCancellationToken(), + ), + ).rejects.toThrow("No browser available"); + // Vitest fails on the unhandled rejection a leftover flow would cause. + authorizer.dispose(); + }); + it("throws when server does not support dynamic client registration", async () => { const { mockAdapter, authorizer } = createTestContext(); diff --git a/test/unit/oauth/metadataClient.test.ts b/test/unit/oauth/metadataClient.test.ts index 587cc665af..08aded6bcb 100644 --- a/test/unit/oauth/metadataClient.test.ts +++ b/test/unit/oauth/metadataClient.test.ts @@ -102,6 +102,29 @@ describe("OAuthMetadataClient", () => { ); }); + it.each([ + ["authorization_endpoint", "https://someone.example/oauth2/authorize"], + ["token_endpoint", "https://someone.example/oauth2/token"], + ["registration_endpoint", "https://someone.example/oauth2/register"], + ["revocation_endpoint", "https://someone.example/oauth2/revoke"], + ["token_endpoint", `${TEST_URL}/@someone/ws/apps/app/token`], + ["token_endpoint", `${TEST_URL}/%40someone/ws/apps/app/token`], + ])("rejects %s %s", async (field, endpoint) => { + const { mockAdapter, client } = createTestContext(); + setupAxiosMockRoutes(mockAdapter, { + "/.well-known/oauth-authorization-server": createMockOAuthMetadata( + TEST_URL, + { [field]: endpoint }, + ), + }); + + const result = client.getMetadata(); + await expect(result).rejects.toThrow( + `${field} is outside the deployment`, + ); + await expect(result).rejects.not.toThrow("someone"); + }); + describe("grant type validation", () => { it("accepts metadata with required grant types", async () => { const { mockAdapter, client } = createTestContext(); diff --git a/test/unit/oauth/sessionManager.test.ts b/test/unit/oauth/sessionManager.test.ts index 3a5a599205..01036e8ece 100644 --- a/test/unit/oauth/sessionManager.test.ts +++ b/test/unit/oauth/sessionManager.test.ts @@ -560,6 +560,25 @@ describe("OAuthSessionManager", () => { expect(revoke).toHaveBeenCalledTimes(2); }); + it("refreshes and revokes without following redirects", async () => { + const { manager, setupForOAuthOperation } = createTestContext(); + const token = vi.fn(() => createMockTokenResponse()); + const revoke = vi.fn(() => ({})); + await setupForOAuthOperation({ + "/oauth2/token": token, + "/oauth2/revoke": revoke, + }); + + await manager.refreshToken(); + await manager.revokeTokens(); + + for (const route of [token, revoke]) { + expect(route).toHaveBeenCalledWith( + expect.objectContaining({ maxRedirects: 0 }), + ); + } + }); + it("does not throw when revocation fails", async () => { const { manager, setupForOAuthOperation } = createTestContext();