Merge PR #8989: fix(mcp): run oauth discovery on reauth when handshake needs no auth (@roboomp)

2026-08-19 11:59:55 +02:00
parent 46c019fd6c 8ea64a6a8d
commit f2e11a3d72
2 changed files with 60 additions and 4 deletions
@@ -1198,11 +1198,20 @@ export class MCPCommandController {
 			connectionError = error as Error;
 		}

-		// Server connected fine without auth — reauth is not needed. A tool-level
-		// challenge overrides this: servers may allow the anonymous handshake yet
-		// protect individual tool calls with `_meta["mcp/www_authenticate"]`.
+		// Server connected fine without auth. A tool-level challenge overrides
+		// this: servers may allow the anonymous handshake yet protect individual
+		// tool calls with `_meta["mcp/www_authenticate"]`. Even without such a
+		// challenge, a clean `initialize` is only weak evidence — per the MCP
+		// spec a server MAY permit unauthenticated `initialize` while requiring a
+		// bearer token for `tools/call`. The user explicitly asked to reauth, so
+		// honor it when the server advertises OAuth discovery metadata; only
+		// refuse when there is genuinely no OAuth endpoint to acquire.
 		if (connectionSucceeded && !authChallenge) {
-			throw new Error("Server connection succeeded without OAuth; reauthorization is not required.");
+			const discovered = "url" in config && config.url ? await discoverOAuthEndpoints(config.url) : null;
+			if (!discovered) {
+				throw new Error("Server connection succeeded without OAuth; reauthorization is not required.");
+			}
+			return discovered;
 		}

 		// Tool calls can carry richer RFC 6750/RFC 9728 hints than the original
@@ -353,6 +353,53 @@ describe("/mcp auth commands", () => {
 		expect(showError).not.toHaveBeenCalled();
 	});

+	test("/mcp reauth acquires a credential when the handshake succeeds but tools need OAuth", async () => {
+		const authStorage = freshAuthStorage();
+		await authStorage.reload();
+		// The anonymous handshake (`initialize`) succeeds; only `tools/call` is
+		// gated behind OAuth. The unauthenticated probe must not treat this as
+		// proof that reauthorization is unnecessary.
+		vi.spyOn(mcpClient, "connectToServer").mockResolvedValue({} as never);
+		vi.spyOn(mcpClient, "disconnectServer").mockResolvedValue(undefined as never);
+
+		const fetchMock = Object.assign(
+			async (input: string | URL | Request): Promise<Response> => {
+				const url = String(input);
+				if (url === "https://mcp.example.com/.well-known/oauth-authorization-server") {
+					return new Response(
+						JSON.stringify({
+							issuer: "https://mcp.example.com",
+							authorization_endpoint: "https://auth.example.com/authorize",
+							token_endpoint: "https://auth.example.com/token",
+							client_id: "advertised-client",
+						}),
+						{ status: 200, headers: { "Content-Type": "application/json" } },
+					);
+				}
+				return new Response("not found", { status: 404 });
+			},
+			{ preconnect: globalThis.fetch.preconnect },
+		);
+		vi.spyOn(globalThis, "fetch").mockImplementation(fetchMock);
+
+		vi.spyOn(oauthFlow.MCPOAuthFlow.prototype, "login").mockResolvedValue({
+			access: "fresh-access",
+			refresh: "fresh-refresh",
+			expires: Date.now() + 3_600_000,
+		});
+
+		const { controller, showError } = createController(authStorage);
+
+		await controller.handle("/mcp reauth envserver");
+
+		expect(showError).not.toHaveBeenCalled();
+		expect(authStorage.get(oauthFlow.mcpOAuthCredentialId(EXPANDED_SERVER_URL))).toMatchObject({
+			type: "oauth",
+			access: "fresh-access",
+			tokenUrl: "https://auth.example.com/token",
+		});
+	});
+
 	test("reuses embedded DCR client secret during reauth token exchange", async () => {
 		const authStorage = freshAuthStorage();
 		await authStorage.reload();