fix: fail oauth when authorization url is missing
This commit is contained in:
parent
88237703e2
commit
d9eda97abe
@ -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
|
||||
|
||||
|
||||
@ -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
|
||||
|
||||
|
||||
@ -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<void> {
|
||||
}
|
||||
|
||||
function shouldRetryAuthError(error: unknown): boolean {
|
||||
if (isOAuthFlowError(error)) {
|
||||
return false;
|
||||
}
|
||||
return analyzeConnectionError(error).kind === 'auth';
|
||||
}
|
||||
|
||||
|
||||
10
src/oauth.ts
10
src/oauth.ts
@ -67,6 +67,7 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
|
||||
private readonly persistence: OAuthPersistence;
|
||||
private redirectUrlValue: URL;
|
||||
private authorizationDeferred: Deferred<string> | null = null;
|
||||
private authorizationRedirectStarted = false;
|
||||
private server?: http.Server;
|
||||
|
||||
private constructor(
|
||||
@ -254,11 +255,16 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
|
||||
|
||||
async redirectToAuthorization(authorizationUrl: URL): Promise<void> {
|
||||
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<void> {
|
||||
await this.persistence.saveCodeVerifier(codeVerifier);
|
||||
}
|
||||
@ -309,8 +315,10 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
|
||||
export interface OAuthSession {
|
||||
provider: OAuthClientProvider & {
|
||||
waitForAuthorizationCode: () => Promise<string>;
|
||||
hasAuthorizationRedirectStarted?: () => boolean;
|
||||
};
|
||||
waitForAuthorizationCode: () => Promise<string>;
|
||||
hasAuthorizationRedirectStarted?: () => boolean;
|
||||
close: () => Promise<void>;
|
||||
}
|
||||
|
||||
@ -318,9 +326,11 @@ export interface OAuthSession {
|
||||
export async function createOAuthSession(definition: ServerDefinition, logger: OAuthLogger): Promise<OAuthSession> {
|
||||
const { provider, close } = await PersistentOAuthClientProvider.create(definition, logger);
|
||||
const waitForAuthorizationCode = () => provider.waitForAuthorizationCode();
|
||||
const hasAuthorizationRedirectStarted = () => provider.hasAuthorizationRedirectStarted();
|
||||
return {
|
||||
provider,
|
||||
waitForAuthorizationCode,
|
||||
hasAuthorizationRedirectStarted,
|
||||
close,
|
||||
};
|
||||
}
|
||||
|
||||
@ -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<ConnectWithAuthOptions, 'serverName' | 'oauthTimeoutMs' | 'recreateTransport'>
|
||||
): Promise<OAuthCapableTransport> {
|
||||
if (session.hasAuthorizationRedirectStarted?.() === false) {
|
||||
throw new OAuthAuthorizationNotStartedError(options.serverName ?? 'unknown', connectError);
|
||||
}
|
||||
const code = await waitForAuthorizationCodeWithTimeout(
|
||||
session,
|
||||
logger,
|
||||
|
||||
@ -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<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
|
||||
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;
|
||||
});
|
||||
});
|
||||
|
||||
@ -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<string>(() => {}));
|
||||
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)
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Loading…
Reference in New Issue
Block a user