fix: MCP OAuth exact redirect URIs for Slack-style providers (#454)

* Add exact MCP OAuth redirect URI support * Allow proxied HTTPS loopback MCP redirects * Fix MCP OAuth busy-port test determinism * Make MCP OAuth redirect test deterministic * fix(coding-agent): honor exact loopback redirect ports * fix(coding-agent): expand env vars in standalone MCP oauth config --------- Co-authored-by: can1357 <me@can.ac>
2026-03-17 14:49:23 +01:00
parent d3efe5e111
commit 571510bdf3
10 changed files with 560 additions and 34 deletions
@@ -31,11 +31,17 @@ export interface MCPServer {
 	auth?: {
 		type: "oauth" | "apikey";
 		credentialId?: string;
+		tokenUrl?: string;
+		clientId?: string;
+		clientSecret?: string;
 	};
-	/** OAuth configuration (clientId, callbackPort) for servers requiring explicit client credentials */
+	/** OAuth configuration (clientId, clientSecret, redirectUri, callbackPort, callbackPath) for servers requiring explicit client credentials */
 	oauth?: {
 		clientId?: string;
+		clientSecret?: string;
+		redirectUri?: string;
 		callbackPort?: number;
+		callbackPath?: string;
 	};
 	/** Transport type */
 	transport?: "stdio" | "sse" | "http";
@@ -160,8 +160,24 @@ async function loadMCPServers(ctx: LoadContext): Promise<LoadResult<MCPServer>>
 				env: serverConfig.env as Record<string, string> | undefined,
 				url: serverConfig.url as string | undefined,
 				headers: serverConfig.headers as Record<string, string> | undefined,
-				auth: serverConfig.auth as { type: "oauth" | "apikey"; credentialId?: string } | undefined,
-				oauth: serverConfig.oauth as { clientId?: string; callbackPort?: number } | undefined,
+				auth: serverConfig.auth as
+					| {
+							type: "oauth" | "apikey";
+							credentialId?: string;
+							tokenUrl?: string;
+							clientId?: string;
+							clientSecret?: string;
+					  }
+					| undefined,
+				oauth: serverConfig.oauth as
+					| {
+							clientId?: string;
+							clientSecret?: string;
+							redirectUri?: string;
+							callbackPort?: number;
+							callbackPath?: string;
+					  }
+					| undefined,
 				transport: serverConfig.type as "stdio" | "sse" | "http" | undefined,
 				_source: createSourceMeta(PROVIDER_ID, path, level),
 			});
@@ -34,9 +34,18 @@ interface MCPConfigFile {
 			auth?: {
 				type: "oauth" | "apikey";
 				credentialId?: string;
+				tokenUrl?: string;
+				clientId?: string;
+				clientSecret?: string;
 			};
 			type?: "stdio" | "sse" | "http";
-			oauth?: { clientId?: string; callbackPort?: number };
+			oauth?: {
+				clientId?: string;
+				clientSecret?: string;
+				redirectUri?: string;
+				callbackPort?: number;
+				callbackPath?: string;
+			};
 		}
 	>;
 }
@@ -93,7 +102,8 @@ function transformMCPConfig(config: MCPConfigFile, source: SourceMeta): MCPServe
 			if (server.env) server.env = expandEnvVarsDeep(server.env);
 			if (server.url) server.url = expandEnvVarsDeep(server.url);
 			if (server.headers) server.headers = expandEnvVarsDeep(server.headers);
-
+			if (server.auth) server.auth = expandEnvVarsDeep(server.auth);
+			if (server.oauth) server.oauth = expandEnvVarsDeep(server.oauth);
 			servers.push(server);
 		}
 	}
@@ -6,11 +6,93 @@
 */

 import type { OAuthController, OAuthCredentials } from "@oh-my-pi/pi-ai";
+import type { OAuthCallbackFlowOptions } from "@oh-my-pi/pi-ai/utils/oauth/callback-server";
 import { OAuthCallbackFlow } from "@oh-my-pi/pi-ai/utils/oauth/callback-server";

 const DEFAULT_PORT = 3000;
 const CALLBACK_PATH = "/callback";

+function isLoopbackHostname(hostname: string): boolean {
+	return hostname === "localhost" || hostname === "127.0.0.1";
+}
+
+function resolveRedirectUri(redirectUri: string | undefined): string | undefined {
+	const trimmed = redirectUri?.trim();
+	if (!trimmed) return undefined;
+
+	const parsed = new URL(trimmed);
+	if (parsed.protocol !== "http:" && parsed.protocol !== "https:") {
+		throw new Error("OAuth redirect URI must use http or https");
+	}
+	return parsed.toString();
+}
+
+function parseRedirectUri(redirectUri: string | undefined): URL | undefined {
+	return redirectUri ? new URL(redirectUri) : undefined;
+}
+
+function getUriPort(uri: URL): number {
+	if (uri.port !== "") return Number(uri.port);
+	return uri.protocol === "https:" ? 443 : 80;
+}
+
+function validateRedirectConfig(config: MCPOAuthConfig, redirectUri: string | undefined): void {
+	const parsed = parseRedirectUri(redirectUri);
+	if (!parsed || parsed.protocol !== "https:" || !isLoopbackHostname(parsed.hostname)) {
+		return;
+	}
+
+	if (config.callbackPort === undefined) {
+		throw new Error(
+			"HTTPS loopback redirect URIs require oauth.callbackPort to point at the local HTTP callback listener behind your TLS terminator",
+		);
+	}
+
+	if (config.callbackPort === getUriPort(parsed)) {
+		throw new Error(
+			"HTTPS loopback redirect URIs cannot reuse the same local port; terminate TLS separately and forward to oauth.callbackPort",
+		);
+	}
+}
+
+function resolveCallbackPort(callbackPort: number | undefined, redirectUri: string | undefined): number {
+	if (callbackPort !== undefined) return callbackPort;
+
+	const parsed = parseRedirectUri(redirectUri);
+	if (!parsed || parsed.protocol !== "http:" || !isLoopbackHostname(parsed.hostname)) {
+		return DEFAULT_PORT;
+	}
+
+	const port = getUriPort(parsed);
+	return Number.isFinite(port) && port > 0 ? port : DEFAULT_PORT;
+}
+
+function resolveCallbackPath(callbackPath: string | undefined, redirectUri: string | undefined): string {
+	const trimmed = callbackPath?.trim();
+	if (trimmed) return trimmed.startsWith("/") ? trimmed : `/${trimmed}`;
+
+	const parsed = parseRedirectUri(redirectUri);
+	if (parsed?.pathname) return parsed.pathname;
+	return CALLBACK_PATH;
+}
+
+function resolveCallbackHostname(redirectUri: string | undefined): string | undefined {
+	const parsed = parseRedirectUri(redirectUri);
+	if (!parsed || !isLoopbackHostname(parsed.hostname)) return undefined;
+	return parsed.hostname;
+}
+
+function resolveCallbackOptions(config: MCPOAuthConfig): OAuthCallbackFlowOptions {
+	const redirectUri = resolveRedirectUri(config.redirectUri);
+	validateRedirectConfig(config, redirectUri);
+	return {
+		preferredPort: resolveCallbackPort(config.callbackPort, redirectUri),
+		callbackPath: resolveCallbackPath(config.callbackPath, redirectUri),
+		callbackHostname: resolveCallbackHostname(redirectUri),
+		redirectUri,
+	};
+}
+
 export interface MCPOAuthConfig {
 	/** Authorization endpoint URL */
 	authorizationUrl: string;
@@ -22,8 +104,12 @@ export interface MCPOAuthConfig {
 	clientSecret?: string;
 	/** OAuth scopes (space-separated) */
 	scopes?: string;
+	/** Exact redirect URI to advertise to the provider */
+	redirectUri?: string;
 	/** Custom callback port (default: 3000) */
 	callbackPort?: number;
+	/** Custom callback path (default: /callback or redirectUri pathname) */
+	callbackPath?: string;
 }

 /**
@@ -39,7 +125,7 @@ export class MCPOAuthFlow extends OAuthCallbackFlow {
 		private config: MCPOAuthConfig,
 		ctrl: OAuthController,
 	) {
-		super(ctrl, config.callbackPort ?? DEFAULT_PORT, CALLBACK_PATH);
+		super(ctrl, resolveCallbackOptions(config));
 		this.#resolvedClientId = this.#resolveClientId(config);
 	}

@@ -68,7 +68,10 @@ interface MCPServerConfigBase {
 	/** OAuth configuration for servers requiring explicit client credentials */
 	oauth?: {
 		clientId?: string;
+		clientSecret?: string;
+		redirectUri?: string;
 		callbackPort?: number;
+		callbackPath?: string;
 	};
 }

@@ -32,7 +32,7 @@ import {
 	searchSmitheryRegistry,
 	toConfigName,
 } from "../../mcp/smithery-registry";
-import type { MCPServerConfig, MCPServerConnection } from "../../mcp/types";
+import type { MCPAuthConfig, MCPServerConfig, MCPServerConnection } from "../../mcp/types";
 import type { OAuthCredential } from "../../session/auth-storage";
 import { shortenPath } from "../../tools/render-utils";
 import { openPath } from "../../utils/open";
@@ -400,13 +400,16 @@ export class MCPCommandController {
 						}

 						try {
+							const oauthClientSecret = finalConfig.oauth?.clientSecret ?? "";
 							const credentialId = await this.#handleOAuthFlow(
 								oauth.authorizationUrl,
 								oauth.tokenUrl,
 								oauth.clientId ?? finalConfig.oauth?.clientId ?? "",
-								"",
+								oauthClientSecret,
 								oauth.scopes ?? "",
 								finalConfig.oauth?.callbackPort,
+								finalConfig.oauth?.callbackPath,
+								finalConfig.oauth?.redirectUri,
 							);
 							finalConfig = {
 								...finalConfig,
@@ -415,7 +418,7 @@ export class MCPCommandController {
 									credentialId,
 									tokenUrl: oauth.tokenUrl,
 									clientId: oauth.clientId ?? finalConfig.oauth?.clientId,
-									clientSecret: undefined,
+									clientSecret: finalConfig.oauth?.clientSecret,
 								},
 							};
 						} catch (oauthError) {
@@ -478,6 +481,8 @@ export class MCPCommandController {
 		clientSecret: string,
 		scopes: string,
 		callbackPort?: number,
+		callbackPath?: string,
+		redirectUri?: string,
 	): Promise<string> {
 		const authStorage = this.ctx.session.modelRegistry.authStorage;
 		let parsedAuthUrl: URL;
@@ -493,6 +498,7 @@ export class MCPCommandController {
 		}

 		const resolvedClientId = clientId.trim() || parsedAuthUrl.searchParams.get("client_id") || undefined;
+		const resolvedClientSecret = clientSecret.trim() || undefined;

 		try {
 			// Create OAuth flow
@@ -501,9 +507,11 @@ export class MCPCommandController {
 					authorizationUrl: authUrl,
 					tokenUrl: tokenUrl,
 					clientId: resolvedClientId,
-					clientSecret: clientSecret || undefined,
+					clientSecret: resolvedClientSecret,
 					scopes: scopes || undefined,
+					redirectUri,
 					callbackPort,
+					callbackPath,
 				},
 				{
 					onAuth: (info: { url: string; instructions?: string }) => {
@@ -653,7 +661,7 @@ export class MCPCommandController {
 	}

 	#stripOAuthAuth(config: MCPServerConfig): MCPServerConfig {
-		const next = { ...config } as MCPServerConfig & { auth?: { type: "oauth" | "apikey"; credentialId?: string } };
+		const next = { ...config } as MCPServerConfig & { auth?: MCPAuthConfig };
 		delete next.auth;
 		return next;
 	}
@@ -1261,9 +1269,7 @@ export class MCPCommandController {
 				return;
 			}

-			const currentAuth = (
-				found.config as MCPServerConfig & { auth?: { type: "oauth" | "apikey"; credentialId?: string } }
-			).auth;
+			const currentAuth = (found.config as MCPServerConfig & { auth?: MCPAuthConfig }).auth;
 			if (currentAuth?.type === "oauth") {
 				await this.#removeManagedOAuthCredential(currentAuth.credentialId);
 			}
@@ -1298,17 +1304,14 @@ export class MCPCommandController {
 				return;
 			}

-			const currentAuth = (
-				found.config as MCPServerConfig & {
-					auth?: { type: "oauth" | "apikey"; credentialId?: string; clientSecret?: string };
-				}
-			).auth;
+			const currentAuth = (found.config as MCPServerConfig & { auth?: MCPAuthConfig }).auth;
 			if (currentAuth?.type === "oauth") {
 				await this.#removeManagedOAuthCredential(currentAuth.credentialId);
 			}

 			const baseConfig = this.#stripOAuthAuth(found.config);
 			const oauth = await this.#resolveOAuthEndpointsFromServer(baseConfig);
+			const oauthClientSecret = found.config.oauth?.clientSecret ?? currentAuth?.clientSecret ?? "";

 			this.#showMessage(["", theme.fg("muted", `Reauthorizing "${name}"...`), ""].join("\n"));

@@ -1316,9 +1319,11 @@ export class MCPCommandController {
 				oauth.authorizationUrl,
 				oauth.tokenUrl,
 				oauth.clientId ?? found.config.oauth?.clientId ?? "",
-				"",
+				oauthClientSecret,
 				oauth.scopes ?? "",
 				found.config.oauth?.callbackPort,
+				found.config.oauth?.callbackPath,
+				found.config.oauth?.redirectUri,
 			);

 			const updated: MCPServerConfig = {
@@ -1328,7 +1333,7 @@ export class MCPCommandController {
 					credentialId,
 					tokenUrl: oauth.tokenUrl,
 					clientId: oauth.clientId ?? found.config.oauth?.clientId,
-					clientSecret: currentAuth?.clientSecret,
+					clientSecret: oauthClientSecret || undefined,
 				},
 			};
 			await updateMCPServer(found.filePath, name, updated);