From d9eda97abeac28ef7e6528af280e65ef62e6eef1 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 4 May 2026 06:16:48 +0100 Subject: [PATCH] fix: fail oauth when authorization url is missing --- CHANGELOG.md | 1 + docs/known-issues.md | 1 + src/cli/auth-command.ts | 4 +++ src/oauth.ts | 10 +++++++ src/runtime/oauth.ts | 26 +++++++++++++++-- tests/cli-auth.test.ts | 30 ++++++++++++++++++++ tests/runtime-oauth-connect.test.ts | 43 ++++++++++++++++++++++++++++- 7 files changed, 112 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72e6dd2..7a95d08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ - Let generated CLIs use `--raw` without also passing required flags, and parse array flags containing JSON object items. (Issues #102 and #103) - Preserve `auth: "oauth"` when an ad-hoc HTTP server is OAuth-promoted and saved with `--persist`. (Issue #82) - Let non-interactive `mcporter list` use existing OAuth token caches for HTTP servers even when older configs are missing `auth: "oauth"`. (Issue #137) +- Fail OAuth flows immediately when the server never creates an authorization URL, instead of waiting for a browser callback that cannot arrive. (Issue #115) ### Config diff --git a/docs/known-issues.md b/docs/known-issues.md index 50feb59..a4d4ea5 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -15,6 +15,7 @@ This file tracks limitations that users regularly run into. Most of these requir - Self-host their MCP server and configure PAT headers / custom OAuth. - Ask Supabase to accept the MCP scope or publish their scope list. - GitHub’s MCP endpoint (`https://api.githubcopilot.com/mcp/`) returns “does not support dynamic client registration” when mcporter attempts to connect. Copilot’s backend expects pre-registered client credentials. Until GitHub publishes a dynamic-registration API (or client secrets), mcporter cannot interact with their hosted server. +- Some hosted servers reject dynamic client registration before returning any authorization URL. mcporter now fails those flows immediately instead of waiting for a browser callback that cannot arrive. If the provider supports a pre-registered client name, set `clientName` in config; otherwise use the provider's supported client or token/header workaround. ## Output schemas missing/buggy on many servers diff --git a/src/cli/auth-command.ts b/src/cli/auth-command.ts index 4583036..ed6d907 100644 --- a/src/cli/auth-command.ts +++ b/src/cli/auth-command.ts @@ -3,6 +3,7 @@ import type { ServerDefinition } from '../config-schema.js'; import { analyzeConnectionError } from '../error-classifier.js'; import { clearOAuthCaches } from '../oauth-persistence.js'; import type { createRuntime } from '../runtime.js'; +import { isOAuthFlowError } from '../runtime/oauth.js'; import type { EphemeralServerSpec } from './adhoc-server.js'; import { extractEphemeralServerFlags } from './ephemeral-flags.js'; import { persistPreparedEphemeralServer, prepareEphemeralServerTarget } from './ephemeral-target.js'; @@ -114,6 +115,9 @@ async function runStdioAuth(definition: ServerDefinition): Promise { } function shouldRetryAuthError(error: unknown): boolean { + if (isOAuthFlowError(error)) { + return false; + } return analyzeConnectionError(error).kind === 'auth'; } diff --git a/src/oauth.ts b/src/oauth.ts index 62114fd..653447f 100644 --- a/src/oauth.ts +++ b/src/oauth.ts @@ -67,6 +67,7 @@ class PersistentOAuthClientProvider implements OAuthClientProvider { private readonly persistence: OAuthPersistence; private redirectUrlValue: URL; private authorizationDeferred: Deferred | null = null; + private authorizationRedirectStarted = false; private server?: http.Server; private constructor( @@ -254,11 +255,16 @@ class PersistentOAuthClientProvider implements OAuthClientProvider { async redirectToAuthorization(authorizationUrl: URL): Promise { this.logger.info(`Authorization required for ${this.definition.name}. Opening browser...`); + this.authorizationRedirectStarted = true; this.ensureAuthorizationDeferred(); __oauthInternals.openExternal(authorizationUrl.toString()); this.logger.warn(`If the browser did not open, visit ${authorizationUrl.toString()} manually.`); } + hasAuthorizationRedirectStarted(): boolean { + return this.authorizationRedirectStarted; + } + async saveCodeVerifier(codeVerifier: string): Promise { await this.persistence.saveCodeVerifier(codeVerifier); } @@ -309,8 +315,10 @@ class PersistentOAuthClientProvider implements OAuthClientProvider { export interface OAuthSession { provider: OAuthClientProvider & { waitForAuthorizationCode: () => Promise; + hasAuthorizationRedirectStarted?: () => boolean; }; waitForAuthorizationCode: () => Promise; + hasAuthorizationRedirectStarted?: () => boolean; close: () => Promise; } @@ -318,9 +326,11 @@ export interface OAuthSession { export async function createOAuthSession(definition: ServerDefinition, logger: OAuthLogger): Promise { const { provider, close } = await PersistentOAuthClientProvider.create(definition, logger); const waitForAuthorizationCode = () => provider.waitForAuthorizationCode(); + const hasAuthorizationRedirectStarted = () => provider.hasAuthorizationRedirectStarted(); return { provider, waitForAuthorizationCode, + hasAuthorizationRedirectStarted, close, }; } diff --git a/src/runtime/oauth.ts b/src/runtime/oauth.ts index b15d31f..aaababb 100644 --- a/src/runtime/oauth.ts +++ b/src/runtime/oauth.ts @@ -39,6 +39,19 @@ export class OAuthTimeoutError extends Error { } } +export class OAuthAuthorizationNotStartedError extends Error { + public readonly serverName: string; + + constructor(serverName: string, cause?: unknown) { + const detail = cause instanceof Error && cause.message ? ` Last error: ${cause.message}` : ''; + super( + `OAuth authorization for '${serverName}' did not produce an authorization URL; aborting instead of waiting for a browser callback.${detail}` + ); + this.name = 'OAuthAuthorizationNotStartedError'; + this.serverName = serverName; + } +} + export function markOAuthFlowError(error: unknown): unknown { return markError(error, OAUTH_FLOW_ERROR); } @@ -104,7 +117,9 @@ export async function connectWithAuth( await closeReplacementTransport(transport, state.activeTransport); throw state.hasCompletedAuthFlow ? markPostAuthConnectError(error) : error; } - logger.warn(`OAuth authorization required for '${serverName ?? 'unknown'}'. Waiting for browser approval...`); + if (session.hasAuthorizationRedirectStarted?.() !== false) { + logger.warn(`OAuth authorization required for '${serverName ?? 'unknown'}'. Waiting for browser approval...`); + } try { state.activeTransport = await completeAuthorizationChallenge(state.activeTransport, session, logger, error, { serverName, @@ -114,7 +129,11 @@ export async function connectWithAuth( state.hasCompletedAuthFlow = true; logger.info('Authorization code accepted. Retrying connection...'); } catch (authError) { - logger.error('OAuth authorization failed while waiting for callback.', authError); + const message = + authError instanceof OAuthAuthorizationNotStartedError + ? 'OAuth authorization could not start.' + : 'OAuth authorization failed while waiting for callback.'; + logger.error(message, authError); await closeReplacementTransport(transport, state.activeTransport); throw markOAuthFlowError(authError); } @@ -155,6 +174,9 @@ async function completeAuthorizationChallenge( connectError: unknown, options: Pick ): Promise { + if (session.hasAuthorizationRedirectStarted?.() === false) { + throw new OAuthAuthorizationNotStartedError(options.serverName ?? 'unknown', connectError); + } const code = await waitForAuthorizationCodeWithTimeout( session, logger, diff --git a/tests/cli-auth.test.ts b/tests/cli-auth.test.ts index b40d1b9..6985c89 100644 --- a/tests/cli-auth.test.ts +++ b/tests/cli-auth.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from 'vitest'; import type { ServerDefinition } from '../src/config.js'; +import { markOAuthFlowError } from '../src/runtime/oauth.js'; process.env.MCPORTER_DISABLE_AUTORUN = '1'; const cliModulePromise = import('../src/cli.js'); @@ -93,4 +94,33 @@ describe('mcporter auth ad-hoc support', () => { errorSpy.mockRestore(); process.exitCode = undefined; }); + + it('does not retry OAuth flow errors that already reached the browser-flow path', async () => { + const { handleAuth } = await cliModulePromise; + const definition = { + name: 'figma', + command: { kind: 'http', url: new URL('https://mcp.figma.com/mcp') }, + } as ServerDefinition; + const oauthError = markOAuthFlowError( + new Error('OAuth authorization for figma did not produce an authorization URL. Last error: HTTP 403') + ); + const listTools = vi.fn().mockRejectedValue(oauthError); + const runtime = { + getDefinitions: () => [definition], + registerDefinition: vi.fn(), + listTools, + getDefinition: () => definition, + } as unknown as Awaited>; + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await handleAuth(runtime, ['figma', '--json']); + + expect(listTools).toHaveBeenCalledTimes(1); + expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining('Retrying with browser flow')); + + logSpy.mockRestore(); + warnSpy.mockRestore(); + process.exitCode = undefined; + }); }); diff --git a/tests/runtime-oauth-connect.test.ts b/tests/runtime-oauth-connect.test.ts index 82bbc06..7fed07f 100644 --- a/tests/runtime-oauth-connect.test.ts +++ b/tests/runtime-oauth-connect.test.ts @@ -1,7 +1,13 @@ import type { Client } from '@modelcontextprotocol/sdk/client'; import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'; import { describe, expect, it, vi } from 'vitest'; -import { connectWithAuth, isOAuthFlowError, isPostAuthConnectError } from '../src/runtime/oauth.js'; +import type { OAuthSession } from '../src/oauth.js'; +import { + connectWithAuth, + isOAuthFlowError, + isPostAuthConnectError, + OAuthAuthorizationNotStartedError, +} from '../src/runtime/oauth.js'; import { createLogger, createPendingAuthorizationSession, @@ -187,4 +193,39 @@ describe('connectWithAuth', () => { await expect(promise).rejects.toSatisfy((error: unknown) => error === finishAuthError && isOAuthFlowError(error)); }); + + it('fails immediately when OAuth never produced an authorization URL', async () => { + const connectError = new UnauthorizedError('dynamic client registration rejected'); + const connect = vi.fn().mockRejectedValueOnce(connectError); + const client = { connect } as unknown as Client; + const waitForAuthorizationCode = vi.fn(() => new Promise(() => {})); + const session = { + provider: { + waitForAuthorizationCode, + hasAuthorizationRedirectStarted: () => false, + }, + waitForAuthorizationCode, + hasAuthorizationRedirectStarted: () => false, + close: vi.fn(async () => {}), + } as unknown as OAuthSession; + const transport = new MockTransport(); + const logger = createLogger(); + + await expect( + connectWithAuth(client, transport, session, logger, { + serverName: 'figma', + maxAttempts: 1, + oauthTimeoutMs: 5000, + }) + ).rejects.toSatisfy( + (error: unknown) => error instanceof OAuthAuthorizationNotStartedError && isOAuthFlowError(error) + ); + + expect(waitForAuthorizationCode).not.toHaveBeenCalled(); + expect(logger.warn).not.toHaveBeenCalledWith(expect.stringContaining('Waiting for browser approval')); + expect(logger.error).toHaveBeenCalledWith( + 'OAuth authorization could not start.', + expect.any(OAuthAuthorizationNotStartedError) + ); + }); });