fix: issuer validation, callback error propagation, and cleanup DRY

- Issuer check: re-register when storedIssuer is absent or non-string
  instead of silently reusing. Narrows unknown type with typeof guard
  and inverts condition so missing issuer → fresh DCR (safer default).
- OAuth callback route: call failFlow with the OAuth error when the
  authorization server redirects back with error= parameter, so the
  waiting flow receives the actual rejection instead of timing out.
  This lets isClientRejection match stale-client errors correctly.
- Extract duplicated cleanup block to clearStaleClientIfRejected()
  private method, called from both returnOnOAuth and blocking paths.
- Test fixes: add issuer to stored metadata in reuse tests, reset
  server to undefined in afterEach to prevent double-close.
This commit is contained in:
Danny Avila 2026-04-03 18:48:06 -04:00
parent 02a064ffb1
commit 68ea22813c
5 changed files with 55 additions and 34 deletions

View file

@ -149,6 +149,22 @@ router.get('/:serverName/oauth/callback', async (req, res) => {
if (oauthError) {
logger.error('[MCP OAuth] OAuth error received', { error: oauthError });
if (state && typeof state === 'string') {
try {
const flowsCache = getLogStores(CacheKeys.FLOWS);
const flowManager = getFlowStateManager(flowsCache);
const flowId = await MCPOAuthHandler.resolveStateToFlowId(state, flowManager);
if (flowId) {
await flowManager.failFlow(flowId, 'mcp_oauth', String(oauthError));
logger.debug('[MCP OAuth] Marked flow as FAILED with OAuth error', {
flowId,
error: oauthError,
});
}
} catch (err) {
logger.debug('[MCP OAuth] Could not mark flow as failed', err);
}
}
return res.redirect(
`${basePath}/oauth/error?error=${encodeURIComponent(String(oauthError))}`,
);

View file

@ -373,19 +373,7 @@ export class MCPConnectionFactory {
this.flowManager!.createFlow(newFlowId, 'mcp_oauth', {}, this.signal).catch(
async (error) => {
logger.debug(`${this.logPrefix} OAuth flow monitor ended`, error);
if (
flowMetadata.reusedStoredClient &&
this.tokenMethods?.deleteTokens &&
MCPConnectionFactory.isClientRejection(error)
) {
await MCPTokenStorage.deleteClientRegistration({
userId: this.userId!,
serverName: this.serverName,
deleteTokens: this.tokenMethods.deleteTokens,
}).catch((err) => {
logger.warn(`${this.logPrefix} Failed to clear stale client registration`, err);
});
}
await this.clearStaleClientIfRejected(flowMetadata.reusedStoredClient, error);
},
);
@ -429,21 +417,7 @@ export class MCPConnectionFactory {
if (result?.tokens) {
connection.emit('oauthHandled');
} else {
// OAuth failed — if we reused a stored client registration and the failure
// indicates client rejection, clear it so the next attempt does fresh DCR
if (
result?.reusedStoredClient &&
this.tokenMethods?.deleteTokens &&
MCPConnectionFactory.isClientRejection(result.error)
) {
await MCPTokenStorage.deleteClientRegistration({
userId: this.userId!,
serverName: this.serverName,
deleteTokens: this.tokenMethods.deleteTokens,
}).catch((err) => {
logger.warn(`${this.logPrefix} Failed to clear stale client registration`, err);
});
}
await this.clearStaleClientIfRejected(result?.reusedStoredClient, result?.error);
logger.warn(`${this.logPrefix} OAuth failed, emitting oauthFailed event`);
connection.emit('oauthFailed', new Error('OAuth authentication failed'));
}
@ -497,6 +471,26 @@ export class MCPConnectionFactory {
}
}
/** Clears stored client registration if the error indicates client rejection */
private async clearStaleClientIfRejected(
reusedStoredClient: boolean | undefined,
error: unknown,
): Promise<void> {
if (!reusedStoredClient || !this.tokenMethods?.deleteTokens) {
return;
}
if (!MCPConnectionFactory.isClientRejection(error)) {
return;
}
await MCPTokenStorage.deleteClientRegistration({
userId: this.userId!,
serverName: this.serverName,
deleteTokens: this.tokenMethods.deleteTokens,
}).catch((err) => {
logger.warn(`${this.logPrefix} Failed to clear stale client registration`, err);
});
}
/** Determines if an error indicates the OAuth client registration was rejected */
static isClientRejection(error: unknown): boolean {
if (!error || typeof error !== 'object') {

View file

@ -54,7 +54,7 @@ jest.mock('~/mcp/mcpConfig', () => ({
}));
describe('MCPOAuthHandler - client registration reuse on reconnection', () => {
let server: OAuthTestServer;
let server: OAuthTestServer | undefined;
let originalDomainServer: string | undefined;
beforeEach(() => {
@ -70,6 +70,7 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => {
}
if (server) {
await server.close();
server = undefined;
}
jest.clearAllMocks();
});
@ -289,7 +290,9 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => {
server = await createOAuthMCPServer({ tokenTTLMs: 60000 });
const tokenStore = new InMemoryTokenStore();
// Seed a stored client with a client_id that the OAuth server doesn't recognize
// Seed a stored client with a client_id that the OAuth server doesn't recognize,
// but with matching issuer and redirect_uri so reuse logic accepts it
const serverIssuer = `http://127.0.0.1:${server.port}`;
await MCPTokenStorage.storeTokens({
userId: 'user-1',
serverName: 'test-server',
@ -302,6 +305,11 @@ describe('MCPOAuthHandler - client registration reuse on reconnection', () => {
client_secret: 'stale-secret',
redirect_uris: ['http://localhost:3080/api/mcp/test-server/oauth/callback'],
} as OAuthClientInformation & { redirect_uris: string[] },
metadata: {
issuer: serverIssuer,
authorization_endpoint: `${serverIssuer}/authorize`,
token_endpoint: `${serverIssuer}/token`,
},
});
// First attempt: reuses the stale client (this is expected — we don't know it's stale yet)

View file

@ -1429,7 +1429,7 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => {
mockGetClientInfoAndMetadata.mockResolvedValueOnce({
clientInfo: existingClientInfo,
clientMetadata: {},
clientMetadata: { issuer: 'https://example.com' },
});
// Mock resource metadata discovery to fail

View file

@ -512,16 +512,19 @@ export class MCPOAuthHandler {
if (existing?.clientInfo?.client_id) {
const storedRedirectUri = (existing.clientInfo as OAuthClientInformation)
.redirect_uris?.[0];
const storedIssuer = existing.clientMetadata?.issuer;
const storedIssuer =
typeof existing.clientMetadata?.issuer === 'string'
? existing.clientMetadata.issuer
: null;
const currentIssuer = metadata.issuer ?? authServerUrl.toString();
if (!storedRedirectUri || storedRedirectUri !== redirectUri) {
logger.debug(
`[MCPOAuth] Stored redirect_uri "${storedRedirectUri}" differs from current "${redirectUri}", will re-register`,
);
} else if (storedIssuer && storedIssuer !== currentIssuer) {
} else if (!storedIssuer || storedIssuer !== currentIssuer) {
logger.debug(
`[MCPOAuth] Stored issuer "${storedIssuer}" differs from current "${currentIssuer}", will re-register`,
`[MCPOAuth] Issuer mismatch (stored: ${storedIssuer ?? 'none'}, current: ${currentIssuer}), will re-register`,
);
} else {
logger.debug(