fix: re-register stale dynamic OAuth clients

Author: pmor

src/lib/node-oauth-client-provider.test.ts

@@ -1,4 +1,5 @@
 import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
+import open from 'open'
 import { NodeOAuthClientProvider } from './node-oauth-client-provider'
 import * as mcpAuthConfig from './mcp-auth-config'
 import type { OAuthProviderOptions } from './types'
@@ -22,6 +23,7 @@ describe('NodeOAuthClientProvider - OAuth Scope Handling', () => {
   let mockReadJsonFile: any
   let mockWriteJsonFile: any
   let mockDeleteConfigFile: any
+  let mockFetch: ReturnType<typeof vi.fn>
 
   const defaultOptions: OAuthProviderOptions = {
     serverUrl: 'https://example.com',
@@ -41,6 +43,7 @@ describe('NodeOAuthClientProvider - OAuth Scope Handling', () => {
   })
 
   afterEach(() => {
+    vi.unstubAllGlobals()
     vi.clearAllMocks()
   })
 
@@ -96,14 +99,133 @@ describe('NodeOAuthClientProvider - OAuth Scope Handling', () => {
       expect(authUrl.searchParams.get('scope')).toBe('github read:user')
     })
 
-    it('should include default scope in authorization URL when none specified', async () => {
+    it('should replace an existing authorization URL scope with the default scope when none is specified', async () => {
       provider = new NodeOAuthClientProvider(defaultOptions)
 
-      const authUrl = new URL('https://auth.example.com/authorize')
+      const authUrl = new URL('https://auth.example.com/authorize?scope=existing')
       await provider.redirectToAuthorization(authUrl)
 
       expect(authUrl.searchParams.get('scope')).toBe('openid email profile')
     })
+
+    it('invalidates a cached dynamic client when authorization reports it is no longer registered', async () => {
+      provider = new NodeOAuthClientProvider(defaultOptions)
+      mockReadJsonFile.mockResolvedValueOnce({
+        client_id: 'stale-client',
+        redirect_uris: ['http://localhost:8080/oauth/callback'],
+      })
+      await provider.clientInformation()
+      mockFetch = vi.fn().mockResolvedValue({
+        status: 400,
+        json: async () => ({
+          registration_endpoint: 'https://auth.example.com/register',
+          error: 'invalid_request',
+          error_description: "Client ID 'stale-client' is not registered with this server",
+        }),
+      })
+      vi.stubGlobal('fetch', mockFetch)
+
+      await expect(
+        provider.redirectToAuthorization(new URL('https://auth.example.com/authorize?client_id=stale-client')),
+      ).rejects.toMatchObject({
+        name: 'StaleClientRegistrationError',
+        message: 'Cached OAuth client registration is no longer valid',
+      })
+
+      expect(mockFetch).toHaveBeenCalledWith(
+        'https://auth.example.com/authorize?client_id=stale-client&scope=openid+email+profile',
+        expect.objectContaining({
+          redirect: 'manual',
+          headers: { Accept: 'application/json' },
+          signal: expect.any(AbortSignal),
+        }),
+      )
+      expect(mockDeleteConfigFile).toHaveBeenCalledTimes(3)
+      expect(mockDeleteConfigFile.mock.calls.map(([, fileName]: [string, string]) => fileName)).toEqual(
+        expect.arrayContaining(['client_info.json', 'tokens.json', 'code_verifier.txt']),
+      )
+      expect(open).not.toHaveBeenCalled()
+    })
+
+    it('retains cached credentials and opens the browser when authorization redirects', async () => {
+      provider = new NodeOAuthClientProvider(defaultOptions)
+      mockReadJsonFile.mockResolvedValueOnce({
+        client_id: 'active-client',
+        redirect_uris: ['http://localhost:8080/oauth/callback'],
+      })
+      await provider.clientInformation()
+      mockFetch = vi.fn().mockResolvedValue({ status: 302 })
+      vi.stubGlobal('fetch', mockFetch)
+
+      await provider.redirectToAuthorization(new URL('https://auth.example.com/authorize?client_id=active-client'))
+
+      expect(mockFetch).toHaveBeenCalledWith(
+        'https://auth.example.com/authorize?client_id=active-client&scope=openid+email+profile',
+        expect.objectContaining({
+          redirect: 'manual',
+          headers: { Accept: 'application/json' },
+          signal: expect.any(AbortSignal),
+        }),
+      )
+      expect(mockDeleteConfigFile).not.toHaveBeenCalled()
+      expect(open).toHaveBeenCalledOnce()
+    })
+
+    it('does not invalidate a cached client when its redirect URI is described as not registered', async () => {
+      provider = new NodeOAuthClientProvider(defaultOptions)
+      mockReadJsonFile.mockResolvedValueOnce({
+        client_id: 'active-client',
+        redirect_uris: ['http://localhost:8080/oauth/callback'],
+      })
+      await provider.clientInformation()
+      mockFetch = vi.fn().mockResolvedValue({
+        status: 400,
+        json: async () => ({
+          registration_endpoint: 'https://auth.example.com/register',
+          error: 'invalid_request',
+          error_description: 'The client redirect URI is not registered',
+        }),
+      })
+      vi.stubGlobal('fetch', mockFetch)
+
+      await provider.redirectToAuthorization(new URL('https://auth.example.com/authorize?client_id=active-client'))
+
+      expect(mockDeleteConfigFile).not.toHaveBeenCalled()
+      expect(open).toHaveBeenCalledOnce()
+    })
+
+    it('does not preflight a freshly dynamically registered client', async () => {
+      provider = new NodeOAuthClientProvider(defaultOptions)
+      await provider.saveClientInformation({
+        client_id: 'fresh-client',
+        redirect_uris: ['http://localhost:8080/oauth/callback'],
+      })
+      mockFetch = vi.fn()
+      vi.stubGlobal('fetch', mockFetch)
+
+      await provider.redirectToAuthorization(new URL('https://auth.example.com/authorize?client_id=fresh-client'))
+
+      expect(mockFetch).not.toHaveBeenCalled()
+      expect(open).toHaveBeenCalledOnce()
+    })
+
+    it('does not preflight a static client registration', async () => {
+      provider = new NodeOAuthClientProvider({
+        ...defaultOptions,
+        staticOAuthClientInfo: {
+          client_id: 'static-client',
+          redirect_uris: ['http://localhost:8080/oauth/callback'],
+        },
+      })
+      mockFetch = vi.fn()
+      vi.stubGlobal('fetch', mockFetch)
+
+      await provider.clientInformation()
+      await provider.redirectToAuthorization(new URL('https://auth.example.com/authorize?client_id=static-client'))
+
+      expect(mockFetch).not.toHaveBeenCalled()
+      expect(open).toHaveBeenCalledOnce()
+    })
   })
 
   describe('backward compatibility', () => {

src/lib/node-oauth-client-provider.ts

@@ -14,6 +14,22 @@ import { sanitizeUrl } from 'strict-url-sanitise'
 import { randomUUID } from 'node:crypto'
 import { fetchAuthorizationServerMetadata, type AuthorizationServerMetadata } from './authorization-server-metadata'
 import type { ProtectedResourceMetadata } from './protected-resource-metadata'
+import { StaleClientRegistrationError } from './stale-client-registration-error'
+
+type ClientRegistrationSource = 'cached-dynamic' | 'fresh-dynamic' | 'static' | undefined
+
+function isStaleClientRegistrationResponse(response: unknown): boolean {
+  if (!response || typeof response !== 'object') {
+    return false
+  }
+
+  const { registration_endpoint: registrationEndpoint, error_description: errorDescription } = response as Record<string, unknown>
+  return (
+    typeof registrationEndpoint === 'string' &&
+    typeof errorDescription === 'string' &&
+    /\bclient(?:\s+id)?\b\s+(?:['"][^'"]+['"]\s+)?is\s+not[\s-]+registered\b/i.test(errorDescription)
+  )
+}
 
 /**
  * Implements the OAuthClientProvider interface for Node.js environments.
@@ -31,6 +47,7 @@ export class NodeOAuthClientProvider implements OAuthClientProvider {
   private authorizeResource: string | undefined
   private _state: string
   private _clientInfo: OAuthClientInformationFull | undefined
+  private clientRegistrationSource: ClientRegistrationSource
   private authorizationServerMetadata: AuthorizationServerMetadata | undefined
   private protectedResourceMetadata: ProtectedResourceMetadata | undefined
   private wwwAuthenticateScope: string | undefined
@@ -51,6 +68,7 @@ export class NodeOAuthClientProvider implements OAuthClientProvider {
     this.authorizeResource = options.authorizeResource
     this._state = randomUUID()
     this._clientInfo = undefined
+    this.clientRegistrationSource = undefined
     this.authorizationServerMetadata = options.authorizationServerMetadata
     this.protectedResourceMetadata = options.protectedResourceMetadata
     this.wwwAuthenticateScope = options.wwwAuthenticateScope
@@ -159,6 +177,7 @@ export class NodeOAuthClientProvider implements OAuthClientProvider {
     if (this.staticOAuthClientInfo) {
       debugLog('Returning static client info')
       this._clientInfo = this.staticOAuthClientInfo
+      this.clientRegistrationSource = 'static'
       return this.staticOAuthClientInfo
     }
     const clientInfo = await readJsonFile<OAuthClientInformationFull>(
@@ -169,6 +188,9 @@ export class NodeOAuthClientProvider implements OAuthClientProvider {
 
     if (clientInfo) {
       this._clientInfo = clientInfo
+      if (this.clientRegistrationSource !== 'fresh-dynamic') {
+        this.clientRegistrationSource = 'cached-dynamic'
+      }
     }
 
     debugLog('Client info result:', clientInfo ? 'Found' : 'Not found')
@@ -182,6 +204,7 @@ export class NodeOAuthClientProvider implements OAuthClientProvider {
   async saveClientInformation(clientInformation: OAuthClientInformationFull): Promise<void> {
     debugLog('Saving client info', { client_id: clientInformation.client_id })
     this._clientInfo = clientInformation
+    this.clientRegistrationSource = 'fresh-dynamic'
     await writeJsonFile(this.serverUrlHash, 'client_info.json', clientInformation)
   }
 
@@ -270,6 +293,8 @@ export class NodeOAuthClientProvider implements OAuthClientProvider {
 
     debugLog('Redirecting to authorization URL', authorizationUrl.toString())
 
+    await this.preflightCachedDynamicClientRegistration(authorizationUrl)
+
     try {
       await open(sanitizeUrl(authorizationUrl.toString()))
       log('Browser opened automatically.')
@@ -279,6 +304,43 @@ export class NodeOAuthClientProvider implements OAuthClientProvider {
     }
   }
 
+  private async preflightCachedDynamicClientRegistration(authorizationUrl: URL): Promise<void> {
+    if (this.clientRegistrationSource !== 'cached-dynamic') {
+      return
+    }
+
+    let response: Response
+    try {
+      response = await fetch(authorizationUrl.toString(), {
+        redirect: 'manual',
+        headers: { Accept: 'application/json' },
+        signal: AbortSignal.timeout(5_000),
+      })
+    } catch (error) {
+      debugLog('Authorization preflight failed; continuing to browser authorization', error)
+      return
+    }
+
+    if (response.status !== 400 && response.status !== 401) {
+      return
+    }
+
+    let errorResponse: unknown
+    try {
+      errorResponse = await response.json()
+    } catch (error) {
+      debugLog('Authorization preflight returned invalid JSON; continuing to browser authorization', error)
+      return
+    }
+
+    if (!isStaleClientRegistrationResponse(errorResponse)) {
+      return
+    }
+
+    await this.invalidateCredentials('all')
+    throw new StaleClientRegistrationError()
+  }
+
   /**
    * Saves the PKCE code verifier
    * @param codeVerifier The code verifier to save
@@ -314,12 +376,14 @@ export class NodeOAuthClientProvider implements OAuthClientProvider {
           deleteConfigFile(this.serverUrlHash, 'code_verifier.txt'),
         ])
         this._clientInfo = undefined
+        this.clientRegistrationSource = undefined
         debugLog('All credentials invalidated')
         break
 
       case 'client':
         await deleteConfigFile(this.serverUrlHash, 'client_info.json')
         this._clientInfo = undefined
+        this.clientRegistrationSource = undefined
         debugLog('Client information invalidated')
         break
 

src/lib/stale-client-registration-error.ts

@@ -0,0 +1,6 @@
+export class StaleClientRegistrationError extends Error {
+  constructor() {
+    super('Cached OAuth client registration is no longer valid')
+    this.name = 'StaleClientRegistrationError'
+  }
+}

src/lib/utils.test.ts

@@ -1,11 +1,40 @@
 import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
-import { parseCommandLineArgs, shouldIncludeTool, mcpProxy, setupOAuthCallbackServerWithLongPoll, getServerUrlHash } from './utils'
+import {
+  connectToRemoteServer,
+  parseCommandLineArgs,
+  shouldIncludeTool,
+  mcpProxy,
+  setupOAuthCallbackServerWithLongPoll,
+  getServerUrlHash,
+} from './utils'
 import { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'
+import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
 import { EventEmitter } from 'events'
 import express from 'express'
+import { StaleClientRegistrationError } from './stale-client-registration-error'
 
 // All sanitizeUrl tests have been moved to the strict-url-sanitise package
 
+describe('connectToRemoteServer', () => {
+  it('rethrows a stale client registration error after one reconnect attempt', async () => {
+    const error = new StaleClientRegistrationError()
+    const startSpy = vi.spyOn(StreamableHTTPClientTransport.prototype, 'start').mockRejectedValue(error)
+
+    try {
+      await expect(
+        connectToRemoteServer(null, 'https://example.com/mcp', {} as any, {}, async () => ({
+          waitForAuthCode: async () => 'unused',
+          skipBrowserAuth: false,
+        })),
+      ).rejects.toBe(error)
+
+      expect(startSpy).toHaveBeenCalledTimes(2)
+    } finally {
+      startSpy.mockRestore()
+    }
+  })
+})
+
 describe('Feature: Command Line Arguments Parsing', () => {
   it('Scenario: Parse basic server URL', async () => {
     // Given command line arguments with only a server URL

src/lib/utils.ts

@@ -14,6 +14,7 @@ import {
   type ProtectedResourceMetadata,
 } from './protected-resource-metadata'
 import { fetchAuthorizationServerMetadata, type AuthorizationServerMetadata } from './authorization-server-metadata'
+import { StaleClientRegistrationError } from './stale-client-registration-error'
 import express from 'express'
 import net from 'net'
 import crypto from 'crypto'
@@ -31,6 +32,7 @@ declare global {
 // Connection constants
 export const REASON_AUTH_NEEDED = 'authentication-needed'
 export const REASON_TRANSPORT_FALLBACK = 'falling-back-to-alternate-transport'
+export const REASON_STALE_CLIENT_REGISTRATION = 'stale-client-registration'
 
 // Transport strategy types
 export type TransportStrategy = 'sse-only' | 'http-only' | 'sse-first' | 'http-first'
@@ -460,6 +462,16 @@ export async function connectToRemoteServer(
 
     return transport
   } catch (error: any) {
+    if (error instanceof StaleClientRegistrationError) {
+      if (recursionReasons.has(REASON_STALE_CLIENT_REGISTRATION)) {
+        throw error
+      }
+
+      recursionReasons.add(REASON_STALE_CLIENT_REGISTRATION)
+      log(`Recursively reconnecting for reason: ${REASON_STALE_CLIENT_REGISTRATION}`)
+      return connectToRemoteServer(client, serverUrl, authProvider, headers, authInitializer, transportStrategy, recursionReasons)
+    }
+
     // Check if it's a protocol error and we should attempt fallback
     // StreamableHTTPError has a `code` property with the HTTP status code
     const isStreamableHTTPError = error instanceof StreamableHTTPError