Merge remote-tracking branch 'origin/main'

Author: code-yeongyu

changes/unreleased/4211.md

@@ -0,0 +1,28 @@
+<!-- changes-log: packages/coding-agent/src/core/extensions/builtin/mcp/changes.md -->
+## 2026-10-24 - Validate OAuth discovery issuers before credential use (senpi#4194)
+
+### What changed
+
+- `auth/oauth-issuer.ts` validates metadata against the discovered authorization server, preserving exact PRM identifiers and the SDK's origin fallback for path-served resources without PRM.
+- `auth/oauth-provider.ts` validates both cached and new discovery, deletes credentials bound to another issuer before re-registration, stamps newly stored credentials with the validated issuer, and declares native registration for loopback redirects.
+- `auth/oauth.ts` checks the issuer of pasted error responses before exposing their contents and persists injected discovery through the same validation boundary.
+- `auth/oauth-refresh.ts` checks the client registration's issuer as well as the token's issuer before sending a refresh request.
+- Credential stamps use one canonical form that removes only an origin-only URL's final slash. Existing root-fallback sign-ins refresh without registering again, and grants without AS metadata are stamped from the validated discovery issuer.
+- Invalid cached metadata is rediscovered rather than requiring logout. A callback without trusted discovery is rejected before exchange, and redirect-less clients omit `application_type`.
+
+### Why
+
+- SDK discovery parsed metadata without comparing its issuer. Direct client-credentials grants and refresh could retain or use registrations belonging to a different issuer.
+
+### Why an extension could not handle it
+
+- Discovery persistence and credential use are owned by the builtin's SDK OAuth provider.
+
+### Expected merge conflict zones
+
+- LOW: discovery persistence in `auth/oauth-provider.ts`, pasted redirect parsing in `auth/oauth.ts`, and grant validation in `auth/oauth-refresh.ts`.
+
+<!-- changes-log: packages/coding-agent/CHANGELOG.md -->
+### Fixed
+
+- MCP OAuth validates metadata and response issuers before using credentials, discards registrations and tokens bound to another issuer, and declares native loopback registration. Metadata and callback issuers remain exact identifiers; credential stamps tolerate only the final slash of an origin-only URL, preserving existing root-fallback sign-ins and grants without AS metadata. Configured client IDs remain reusable configuration ([#4211](https://github.com/code-yeongyu/senpi/pull/4211)).

packages/coding-agent/src/core/extensions/builtin/mcp/auth/oauth-issuer.ts

@@ -0,0 +1,37 @@
+import type { OAuthServerInfo } from "@modelcontextprotocol/sdk/client/auth.js";
+import { OAuthFlowError } from "./oauth-errors.ts";
+import type { McpStoredAuth } from "./token-store.ts";
+
+// Credential stamps tolerate only the slash spelling of an origin-only URL, not URL normalization.
+export function canonicalAuthorizationServerIssuer(issuer: string): string {
+	return issuer.replace(/^(https?:\/\/[^/?#]+)\/$/, "$1");
+}
+
+// RFC 8414 section 3.3: identifiers from metadata are compared verbatim, not as normalized URLs.
+export function validatedAuthorizationServerIssuer(info: OAuthServerInfo, serverName: string): string {
+	// The SDK's legacy fallback discovers root metadata and spells the origin with a trailing slash.
+	// Only that configured-resource fallback is normalized; PRM-provided identifiers remain exact.
+	const origin = new URL(info.authorizationServerUrl).origin;
+	const issuer =
+		!info.resourceMetadata?.authorization_servers?.length && info.authorizationServerUrl === `${origin}/`
+			? canonicalAuthorizationServerIssuer(info.authorizationServerUrl)
+			: info.authorizationServerUrl;
+	if (info.authorizationServerMetadata !== undefined && info.authorizationServerMetadata.issuer !== issuer) {
+		throw new OAuthFlowError(
+			"needs_auth",
+			`MCP server ${serverName} authorization server metadata issuer mismatch; restart authorization after correcting the server metadata.`,
+			{ serverName },
+		);
+	}
+	return issuer;
+}
+
+export function credentialsMatchIssuer(record: McpStoredAuth | undefined, issuer: string): boolean {
+	const grantIssuer = record?.issuer ?? record?.discoveryState?.authorizationServerUrl;
+	const clientIssuer = record?.clientInfo?.issuer ?? grantIssuer;
+	return [grantIssuer, clientIssuer].every(
+		(stamp) =>
+			stamp === undefined ||
+			canonicalAuthorizationServerIssuer(stamp) === canonicalAuthorizationServerIssuer(issuer),
+	);
+}

packages/coding-agent/src/core/extensions/builtin/mcp/auth/oauth-provider.ts

@@ -9,6 +9,11 @@ import type {
 import type { McpLogger } from "../log.ts";
 import { fingerprintSecret } from "../log.ts";
 import { OAuthFlowError } from "./oauth-errors.ts";
+import {
+	canonicalAuthorizationServerIssuer,
+	credentialsMatchIssuer,
+	validatedAuthorizationServerIssuer,
+} from "./oauth-issuer.ts";
 import type { McpStoredAuth, McpTokenStore } from "./token-store.ts";
 
 // Buffer before absolute token expiry at which a refresh is considered due.
@@ -31,19 +36,6 @@ export function tokenExpiresAt(tokens: OAuthTokens, now = Date.now()): number |
 	return now + tokens.expires_in * 1000;
 }
 
-/** Compares authorization server identifiers the way the SDK does: parsed, tolerating one trailing `/`. */
-export function sameAuthorizationServer(a: string, b: string): boolean {
-	const normalize = (value: string): string => {
-		try {
-			return new URL(value).href.replace(/\/$/, "");
-		} catch (error) {
-			if (error instanceof TypeError) return value.replace(/\/$/, "");
-			throw error;
-		}
-	};
-	return normalize(a) === normalize(b);
-}
-
 /**
  * The authorization server a stored grant belongs to (senpi#2940, GHSA-6qxp-vccf-f47h): its `issuer` stamp, or
  * for a record saved before the stamp existed, the authorization server its sign-in discovered. A record with
@@ -127,9 +119,18 @@ export class McpOAuthProvider implements OAuthClientProvider {
 		return this.#options.clientMetadataUrl;
 	}
 
-	get clientMetadata(): OAuthClientMetadata {
+	get clientMetadata(): OAuthClientMetadata & { application_type?: "native" | "web" } {
 		const redirect = this.#options.redirectUrl;
 		return {
+			...(redirect === undefined
+				? {}
+				: {
+						application_type:
+							!["http:", "https:"].includes(new URL(redirect).protocol) ||
+							["127.0.0.1", "localhost", "[::1]"].includes(new URL(redirect).hostname)
+								? "native"
+								: "web",
+					}),
 			redirect_uris: redirect === undefined ? [] : [String(redirect)],
 			grant_types: ["authorization_code", "refresh_token"],
 			response_types: ["code"],
@@ -158,24 +159,59 @@ export class McpOAuthProvider implements OAuthClientProvider {
 	}
 
 	async saveClientInformation(info: OAuthClientInformationFull): Promise<void> {
-		// The SDK stamps `info.issuer`; it is persisted with the rest so the binding survives a restart.
-		await this.#store.update((current) => ({ ...(current ?? {}), clientInfo: info }));
+		const discovery = this.discoveryState();
+		const issuer =
+			discovery === undefined
+				? info.issuer && canonicalAuthorizationServerIssuer(info.issuer)
+				: validatedAuthorizationServerIssuer(discovery, this.serverName);
+		await this.#store.update((current) => ({ ...(current ?? {}), clientInfo: { ...info, issuer } }));
 	}
 
 	async saveDiscoveryState(state: OAuthDiscoveryState): Promise<void> {
-		await this.#store.update((current) => ({ ...freezeGrantAttribution(current), discoveryState: state }));
+		const issuer = validatedAuthorizationServerIssuer(state, this.serverName);
+		const discoveryState = { ...state, authorizationServerUrl: issuer };
+		await this.#store.update((current) => {
+			const attributed = freezeGrantAttribution(current);
+			if (!credentialsMatchIssuer(attributed, issuer)) return { discoveryState };
+			const clientIssuer = attributed.clientInfo?.issuer ?? storedGrantIssuer(attributed);
+			return {
+				...attributed,
+				// Unattributed legacy registrations cannot be sent to the server discovered now.
+				clientInfo:
+					clientIssuer === undefined || attributed.clientInfo === undefined
+						? undefined
+						: { ...attributed.clientInfo, issuer },
+				discoveryState,
+			};
+		});
 	}
 
 	discoveryState(): OAuthDiscoveryState | undefined {
-		return this.#store.read()?.discoveryState;
+		const current = this.#store.read();
+		const state = current?.discoveryState;
+		if (state !== undefined) {
+			try {
+				const issuer = validatedAuthorizationServerIssuer(state, this.serverName);
+				if (!credentialsMatchIssuer(current, issuer)) return undefined;
+			} catch (error) {
+				if (error instanceof OAuthFlowError) return undefined;
+				throw error;
+			}
+		}
+		return state;
 	}
 
 	tokens(): OAuthTokens | undefined {
 		return storedAuthToTokens(this.#store.read());
 	}
 
 	async saveTokens(tokens: OAuthTokens): Promise<void> {
-		await this.#store.update((current) => mergeTokensIntoStoredAuth(current, tokens, this.serverUrl));
+		const discovery = this.discoveryState();
+		const issuer =
+			discovery === undefined
+				? tokens.issuer && canonicalAuthorizationServerIssuer(tokens.issuer)
+				: validatedAuthorizationServerIssuer(discovery, this.serverName);
+		await this.#store.update((current) => mergeTokensIntoStoredAuth(current, { ...tokens, issuer }, this.serverUrl));
 		this.#logFingerprint("saveTokens", tokens.access_token);
 	}
 

packages/coding-agent/src/core/extensions/builtin/mcp/auth/oauth-refresh.ts

@@ -5,11 +5,11 @@ import { loadMcpSdkAuth } from "../sdk.lazy.ts";
 import { safeDelay } from "../wrap.ts";
 import { isInvalidGrant, isTransientTokenError, OAuthFlowError } from "./oauth-errors.ts";
 import { oauthFetch } from "./oauth-fetch.ts";
+import { credentialsMatchIssuer, validatedAuthorizationServerIssuer } from "./oauth-issuer.ts";
 import {
 	type McpOAuthProvider,
 	mergeTokensIntoStoredAuth,
 	REFRESH_LEEWAY_MS,
-	sameAuthorizationServer,
 	storedAuthToTokens,
 	storedGrantIssuer,
 } from "./oauth-provider.ts";
@@ -91,8 +91,8 @@ export class McpRefreshManager {
 		// elsewhere, or one that cannot be attributed at all, is a continuity break: drop it and require a fresh
 		// sign-in rather than refreshing silently against whatever server discovery now names.
 		const issuer = storedGrantIssuer(current);
-		const authorizationServer = String(info.authorizationServerUrl);
-		if (issuer === undefined || !sameAuthorizationServer(issuer, authorizationServer)) {
+		const authorizationServer = validatedAuthorizationServerIssuer(info, this.#provider.serverName);
+		if (issuer === undefined || !credentialsMatchIssuer(current, authorizationServer)) {
 			this.#provider.store.writeUnlocked(undefined);
 			throw new OAuthFlowError(
 				"needs_auth",

packages/coding-agent/src/core/extensions/builtin/mcp/auth/oauth.ts

@@ -8,6 +8,7 @@ import type { FetchLike } from "@modelcontextprotocol/sdk/shared/transport.js";
 import { loadMcpSdkAuth } from "../sdk.lazy.ts";
 import { isInvalidGrant, OAuthFlowError } from "./oauth-errors.ts";
 import { oauthFetch } from "./oauth-fetch.ts";
+import { validatedAuthorizationServerIssuer } from "./oauth-issuer.ts";
 import type { McpOAuthProvider } from "./oauth-provider.ts";
 import { assertS256Supported } from "./oauth-refresh.ts";
 
@@ -51,7 +52,7 @@ export async function completeAuthorization(
 	redirectInput: string,
 	options: OAuthFlowOptions = {},
 ): Promise<void> {
-	const { code, state, iss } = parseRedirect(redirectInput, provider.serverName);
+	const { code, state, iss } = parseRedirect(redirectInput, provider);
 	if (!provider.consumeState(state)) {
 		throw new OAuthFlowError(
 			"state_mismatch",
@@ -101,14 +102,23 @@ export async function finishAuthorization(
 // RFC 9207: an authorization response that names another authorization server, or that omits `iss`
 // although this server promises to send it, may have been injected by a mix-up attack; never exchange its code.
 function assertAuthorizationResponseIssuer(provider: McpOAuthProvider, iss: string | undefined): void {
-	const metadata = provider.discoveryState()?.authorizationServerMetadata;
-	if (metadata === undefined) return;
+	const discovery = provider.discoveryState();
+	if (discovery === undefined) {
+		throw new OAuthFlowError(
+			"needs_auth",
+			`MCP server ${provider.serverName} has no trusted authorization discovery; restart with /mcp auth-start ${provider.serverName}.`,
+			{ serverName: provider.serverName },
+		);
+	}
+	const metadata = discovery.authorizationServerMetadata;
+	const issuer = validatedAuthorizationServerIssuer(discovery, provider.serverName);
 	// The SDK keeps unknown metadata fields (loose schema) but does not type this RFC 9207 one.
 	const promised =
-		(metadata as { authorization_response_iss_parameter_supported?: unknown })
-			.authorization_response_iss_parameter_supported === true;
+		metadata !== undefined &&
+		"authorization_response_iss_parameter_supported" in metadata &&
+		metadata.authorization_response_iss_parameter_supported === true;
 	if (iss === undefined && !promised) return;
-	if (iss !== undefined && iss === metadata.issuer) return;
+	if (iss !== undefined && iss === issuer) return;
 	throw new OAuthFlowError(
 		"needs_auth",
 		iss === undefined
@@ -165,19 +175,21 @@ export async function clientCredentialsGrant(
 		resource: new URL(provider.serverUrl),
 		fetchFn: oauthFetch(options.fetchFn),
 	});
-	await provider.saveTokens({ ...tokens, issuer: String(info.authorizationServerUrl) });
+	await provider.saveTokens({ ...tokens, issuer: validatedAuthorizationServerIssuer(info, provider.serverName) });
 }
 
 export async function logout(provider: McpOAuthProvider): Promise<void> {
 	await provider.store.clear();
 }
 
 async function discover(provider: McpOAuthProvider, options: OAuthFlowOptions): Promise<OAuthServerInfo> {
-	if (options.discover !== undefined) return options.discover(provider.serverUrl);
 	const cached = provider.discoveryState();
 	if (cached !== undefined) return cached;
 	const { discoverOAuthServerInfo } = await loadMcpSdkAuth();
-	const info = await discoverOAuthServerInfo(provider.serverUrl, { fetchFn: oauthFetch(options.fetchFn) });
+	const info =
+		options.discover !== undefined
+			? await options.discover(provider.serverUrl)
+			: await discoverOAuthServerInfo(provider.serverUrl, { fetchFn: oauthFetch(options.fetchFn) });
 	await provider.saveDiscoveryState(toDiscoveryState(info));
 	return info;
 }
@@ -192,8 +204,9 @@ function toDiscoveryState(info: OAuthServerInfo): OAuthDiscoveryState {
 
 function parseRedirect(
 	input: string,
-	serverName: string,
+	provider: McpOAuthProvider,
 ): { code: string; state: string | undefined; iss: string | undefined } {
+	const serverName = provider.serverName;
 	let url: URL;
 	try {
 		url = new URL(input.trim());
@@ -204,6 +217,7 @@ function parseRedirect(
 			{ serverName },
 		);
 	}
+	assertAuthorizationResponseIssuer(provider, url.searchParams.get("iss") ?? undefined);
 	const error = url.searchParams.get("error");
 	if (error !== null) {
 		throw new OAuthFlowError("needs_auth", `MCP server ${serverName} authorization failed: ${error}`, { serverName });