fix(McpOAuthClientProvider): refresh token race condition between client_id

Author: edelauna

src/services/mcp/McpOAuthClientProvider.ts

@@ -174,7 +174,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
 
 		// Check if we have a cached client_id from previous registration
 		const cachedData = await this._secretStorage.getOAuthData(this._serverUrl)
-		if (cachedData?.client_id && cachedData.redirect_uri === this.redirectUrl) {
+		if (cachedData?.client_id) {
 			this._clientInfo = {
 				client_id: cachedData.client_id,
 				redirect_uris: [this.redirectUrl],
@@ -219,9 +219,17 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
 				return this._refreshPromise
 			}
 
-			this._refreshPromise = this.refreshAccessToken(data.tokens.refresh_token).finally(() => {
-				this._refreshPromise = null
-			})
+			// Use the client_id stored alongside the tokens — it is the one the
+			// auth server bound the refresh token to.  `this._clientInfo.client_id`
+			// may differ if a fresh DCR was performed (e.g. after stale token
+			// cleanup removed the cached data).
+			const clientIdForRefresh = data.client_id ?? this._clientInfo?.client_id
+
+			this._refreshPromise = this.refreshAccessToken(data.tokens.refresh_token, clientIdForRefresh).finally(
+				() => {
+					this._refreshPromise = null
+				},
+			)
 
 			try {
 				return await this._refreshPromise
@@ -236,13 +244,12 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
 		return undefined
 	}
 
-	async saveTokens(tokens: OAuthTokens): Promise<void> {
+	async saveTokens(tokens: OAuthTokens, clientIdOverride?: string): Promise<void> {
 		const expires_at = tokens.expires_in ? Date.now() + tokens.expires_in * 1000 : Date.now() + 3600 * 1000 // default 1 hour when server omits expires_in
 		await this._secretStorage.saveOAuthData(this._serverUrl, {
 			tokens,
 			expires_at,
-			client_id: this._clientInfo?.client_id,
-			redirect_uri: this.redirectUrl,
+			client_id: clientIdOverride ?? this._clientInfo?.client_id,
 		})
 	}
 
@@ -374,24 +381,30 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
 
 	/**
 	 * Refreshes the access token using a refresh token.
+	 *
 	 * @param refreshToken The refresh token to use.
+	 * @param clientIdOverride Optional client_id to use instead of `this._clientInfo.client_id`.
+	 *   This is used when the stored tokens were issued to a different client_id than the
+	 *   current in-memory registration (e.g. after a port change caused a new DCR).
 	 * @returns The new tokens.
 	 */
-	async refreshAccessToken(refreshToken: string): Promise<OAuthTokens> {
+	async refreshAccessToken(refreshToken: string, clientIdOverride?: string): Promise<OAuthTokens> {
 		if (!this._authServerMeta?.token_endpoint) {
 			throw new Error("No token_endpoint in auth server metadata — cannot refresh token")
 		}
-		if (!this._clientInfo) {
+
+		const clientId = clientIdOverride ?? this._clientInfo?.client_id
+		if (!clientId) {
 			throw new Error("No client information — registerClientIfNeeded() must be called first")
 		}
 
 		const params: Record<string, string> = {
 			grant_type: "refresh_token",
 			refresh_token: refreshToken,
-			client_id: this._clientInfo.client_id,
+			client_id: clientId,
 		}
 
-		if (this._tokenEndpointAuthMethod === "client_secret_post" && this._clientInfo.client_secret) {
+		if (this._tokenEndpointAuthMethod === "client_secret_post" && this._clientInfo?.client_secret) {
 			params.client_secret = this._clientInfo.client_secret
 		}
 
@@ -405,11 +418,12 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
 		})
 
 		if (!response.ok) {
-			throw new Error(`Token refresh failed: HTTP ${response.status}`)
+			const errorBody = await response.text().catch(() => "")
+			throw new Error(`Token refresh failed: HTTP ${response.status} ${errorBody}`)
 		}
 
 		const tokens = (await response.json()) as OAuthTokens
-		await this.saveTokens(tokens)
+		await this.saveTokens(tokens, clientId)
 		return tokens
 	}
 

src/services/mcp/SecretStorageService.ts

@@ -7,8 +7,6 @@ export interface StoredMcpOAuthData {
 	expires_at: number
 	/** The client_id used to obtain these tokens (for token reuse without re-registration). */
 	client_id?: string
-	/** The redirect_uri used during client registration (to detect port changes). */
-	redirect_uri?: string
 }
 
 /**

src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts

@@ -585,7 +585,7 @@ describe("McpOAuthClientProvider", () => {
 	})
 
 	describe("registerClientIfNeeded", () => {
-		it("should reuse cached client_id when redirect_uri matches", async () => {
+		it("should reuse cached client_id from previous registration", async () => {
 			setupCallbackServerMock()
 			const secretStorage = createMockSecretStorage()
 
@@ -594,7 +594,6 @@ describe("McpOAuthClientProvider", () => {
 				tokens: { access_token: "cached-token", token_type: "Bearer" },
 				expires_at: Date.now() + 3600000,
 				client_id: "cached-client-id",
-				redirect_uri: "http://localhost:12345/callback",
 			})
 
 			const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage)
@@ -604,7 +603,28 @@ describe("McpOAuthClientProvider", () => {
 			await provider.close()
 		})
 
-		it("should not reuse cached client_id when redirect_uri does not match", async () => {
+		it("should reuse cached client_id even when callback server port has changed", async () => {
+			setupCallbackServerMock()
+			const secretStorage = createMockSecretStorage()
+
+			// Pre-populate storage with cached data — the callback server port (12345)
+			// may differ from the port used in the original registration, but we still
+			// reuse the client_id to avoid "refresh token not issued to this client" errors.
+			await secretStorage.saveOAuthData("https://example.com/mcp", {
+				tokens: { access_token: "cached-token", token_type: "Bearer" },
+				expires_at: Date.now() + 3600000,
+				client_id: "cached-client-id",
+			})
+
+			const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage)
+			await provider.registerClientIfNeeded()
+
+			// Should still reuse the cached client_id, NOT perform a new DCR
+			expect((await provider.clientInformation())?.client_id).toBe("cached-client-id")
+			await provider.close()
+		})
+
+		it("should perform DCR when no cached client_id exists", async () => {
 			setupCallbackServerMock()
 			const secretStorage = createMockSecretStorage()
 
@@ -636,12 +656,10 @@ describe("McpOAuthClientProvider", () => {
 					}),
 			})
 
-			// Pre-populate storage with cached data with different redirect_uri
+			// No cached client_id in storage
 			await secretStorage.saveOAuthData("https://example.com/mcp", {
 				tokens: { access_token: "cached-token", token_type: "Bearer" },
 				expires_at: Date.now() + 3600000,
-				client_id: "cached-client-id",
-				redirect_uri: "http://localhost:99999/callback", // different port
 			})
 
 			const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage)