Skip to content

Commit 5aeed4f

Browse files
edelaunaedelauna
committed
feat(McpHub): cache whether server required OAuth or not
1 parent b6dfa75 commit 5aeed4f

5 files changed

Lines changed: 186 additions & 23 deletions

File tree

‎src/services/mcp/McpHub.ts‎

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -795,24 +795,32 @@ export class McpHub {
795795
throw new Error("SecretStorageService not initialized — call setSecretStorage() before connecting")
796796
}
797797

798-
// Create an OAuth provider for this server.
798+
// Decide whether to perform OAuth discovery (RFC 9728 + RFC 8414)
799+
// upfront or skip it to avoid a wasted network round-trip for
800+
// non-OAuth servers.
799801
//
800-
// McpOAuthClientProvider.create() performs OAuth discovery (RFC 9728 +
801-
// RFC 8414) once and starts the local callback server so the redirect
802-
// URI port is stable before any connect attempt.
803-
//
804-
// If the server already has a stored token the SDK will use it
805-
// transparently; the browser is only opened when a 401 forces a new
806-
// authorization flow.
807-
const authProvider = await McpOAuthClientProvider.create(configInjected.url, this.secretStorage, name)
802+
// Three cases:
803+
// 1. SecretStorage has OAuth data → known OAuth server → discover
804+
// 2. In-memory negative cache says non-OAuth → skip discovery
805+
// 3. Unknown server (first connection) → discover once, cache result
806+
const hasOAuthData = await this.secretStorage.hasOAuthData(configInjected.url)
807+
const knownNonOAuth = McpOAuthClientProvider.isKnownNonOAuth(configInjected.url)
808+
const skipDiscovery = !hasOAuthData && knownNonOAuth
809+
810+
const authProvider = await McpOAuthClientProvider.create(configInjected.url, this.secretStorage, name, {
811+
skipDiscovery,
812+
})
808813

809814
// Pre-register the OAuth client so the SDK can skip its own
810815
// registration step (broken for path-prefixed issuers — see
811816
// utils/oauth.ts for upstream issue links).
812-
try {
813-
await authProvider.registerClientIfNeeded()
814-
} catch {
815-
// Registration may not be supported — the SDK will attempt its own.
817+
// Skip when discovery was skipped — there's no metadata to register with.
818+
if (!skipDiscovery) {
819+
try {
820+
await authProvider.registerClientIfNeeded()
821+
} catch {
822+
// Registration may not be supported — the SDK will attempt its own.
823+
}
816824
}
817825

818826
transport = new StreamableHTTPClientTransport(new URL(configInjected.url), {
@@ -959,10 +967,23 @@ export class McpHub {
959967
await client.connect(transport)
960968
} catch (connectError) {
961969
if (connectError instanceof UnauthorizedError && streamableHttpAuthProvider && configInjected.url) {
962-
// The server requires OAuth. Mark this connection as "connecting" and
963-
// detach the toast + browser flow from the initialization path so that
970+
// The server requires OAuth.
971+
McpOAuthClientProvider.clearNonOAuthCache(configInjected.url)
972+
973+
// If discovery was skipped (provider has no metadata), we need to
974+
// tear down and reconnect with full discovery now that we know
975+
// this is an OAuth server. The reconnect will do discovery since
976+
// the negative cache was cleared and no SecretStorage data exists yet.
977+
if (!streamableHttpAuthProvider.hasMetadata) {
978+
await streamableHttpAuthProvider.close()
979+
await this.deleteConnection(name, source)
980+
void this.connectToServer(name, config, source)
981+
return
982+
}
983+
984+
// Mark this connection as "connecting" and detach the toast +
985+
// browser flow from the initialization path so that
964986
// waitUntilReady() resolves immediately and the MCP panel can load.
965-
const serverUrl = configInjected.url
966987
connection.server.status = "connecting"
967988

968989
void this._initiateOAuthFlow(

‎src/services/mcp/McpOAuthClientProvider.ts‎

Lines changed: 52 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,35 @@ import { fetchOAuthAuthServerMetadata } from "./utils/oauth"
3131
* 6. `await authProvider.close()` when done (success or permanent failure)
3232
*/
3333
export class McpOAuthClientProvider implements OAuthClientProvider {
34+
// ── Static negative cache ────────────────────────────────────────────────
35+
// Remembers servers that returned no OAuth metadata so we can skip the
36+
// discovery probe on subsequent connection attempts (reconnect, restart).
37+
private static _nonOAuthCache = new Map<string, number>() // serverUrl → timestamp
38+
private static NON_OAUTH_TTL_MS = 30 * 60 * 1000 // 30 minutes
39+
40+
static isKnownNonOAuth(serverUrl: string): boolean {
41+
const ts = McpOAuthClientProvider._nonOAuthCache.get(serverUrl)
42+
if (ts === undefined) return false
43+
if (Date.now() - ts > McpOAuthClientProvider.NON_OAUTH_TTL_MS) {
44+
McpOAuthClientProvider._nonOAuthCache.delete(serverUrl)
45+
return false
46+
}
47+
return true
48+
}
49+
50+
static markNonOAuth(serverUrl: string): void {
51+
McpOAuthClientProvider._nonOAuthCache.set(serverUrl, Date.now())
52+
}
53+
54+
static clearNonOAuthCache(serverUrl?: string): void {
55+
if (serverUrl) {
56+
McpOAuthClientProvider._nonOAuthCache.delete(serverUrl)
57+
} else {
58+
McpOAuthClientProvider._nonOAuthCache.clear()
59+
}
60+
}
61+
62+
// ── Instance fields ──────────────────────────────────────────────────────
3463
private _codeVerifier?: string
3564
// Client info is kept in-memory only (not persisted) to avoid stale registrations
3665
// when the redirect URI port changes between sessions.
@@ -71,14 +100,25 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
71100
serverUrl: string,
72101
secretStorage: SecretStorageService,
73102
serverName?: string,
103+
options?: { skipDiscovery?: boolean },
74104
): Promise<McpOAuthClientProvider> {
75-
// Fetch auth server metadata once. Reused for:
76-
// - selecting token_endpoint_auth_method / grant_types / scopes
77-
// - pre-registering the client (registration_endpoint)
78-
// - RFC 8707 resource indicator (injected into authorization URL)
79-
const discovery = await fetchOAuthAuthServerMetadata(serverUrl)
80-
const authServerMeta = discovery?.authServerMeta ?? null
81-
const resourceIndicator = discovery?.resourceIndicator ?? null
105+
let authServerMeta: Record<string, any> | null = null
106+
let resourceIndicator: string | null = null
107+
108+
if (!options?.skipDiscovery) {
109+
// Fetch auth server metadata once. Reused for:
110+
// - selecting token_endpoint_auth_method / grant_types / scopes
111+
// - pre-registering the client (registration_endpoint)
112+
// - RFC 8707 resource indicator (injected into authorization URL)
113+
const discovery = await fetchOAuthAuthServerMetadata(serverUrl)
114+
authServerMeta = discovery?.authServerMeta ?? null
115+
resourceIndicator = discovery?.resourceIndicator ?? null
116+
117+
// Cache the result so subsequent connections can skip the probe.
118+
if (!authServerMeta) {
119+
McpOAuthClientProvider.markNonOAuth(serverUrl)
120+
}
121+
}
82122

83123
// Extract auth-method preferences.
84124
// Prefer "none" → first supported → "client_secret_post"
@@ -113,6 +153,11 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
113153

114154
// ── OAuthClientProvider interface ────────────────────────────────────────
115155

156+
/** Whether this provider was created with OAuth metadata (discovery succeeded). */
157+
get hasMetadata(): boolean {
158+
return this._authServerMeta !== null
159+
}
160+
116161
get redirectUrl(): string {
117162
return `http://localhost:${this._port}/callback`
118163
}

‎src/services/mcp/SecretStorageService.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,11 @@ export class SecretStorageService {
4545
await this._storage.store(this._key(serverUrl), JSON.stringify(data))
4646
}
4747

48+
async hasOAuthData(serverUrl: string): Promise<boolean> {
49+
const raw = await this._storage.get(this._key(serverUrl))
50+
return raw !== undefined
51+
}
52+
4853
async deleteOAuthData(serverUrl: string): Promise<void> {
4954
await this._storage.delete(this._key(serverUrl))
5055
}

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

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,43 @@ function setupCallbackServerMock(code = "test-auth-code", state?: string) {
8282
describe("McpOAuthClientProvider", () => {
8383
beforeEach(() => {
8484
vi.clearAllMocks()
85+
McpOAuthClientProvider.clearNonOAuthCache()
86+
})
87+
88+
describe("static negative cache", () => {
89+
it("isKnownNonOAuth returns false for unknown servers", () => {
90+
expect(McpOAuthClientProvider.isKnownNonOAuth("https://unknown.com/mcp")).toBe(false)
91+
})
92+
93+
it("markNonOAuth makes isKnownNonOAuth return true", () => {
94+
McpOAuthClientProvider.markNonOAuth("https://example.com/mcp")
95+
expect(McpOAuthClientProvider.isKnownNonOAuth("https://example.com/mcp")).toBe(true)
96+
})
97+
98+
it("clearNonOAuthCache(url) clears a specific entry", () => {
99+
McpOAuthClientProvider.markNonOAuth("https://a.com/mcp")
100+
McpOAuthClientProvider.markNonOAuth("https://b.com/mcp")
101+
McpOAuthClientProvider.clearNonOAuthCache("https://a.com/mcp")
102+
expect(McpOAuthClientProvider.isKnownNonOAuth("https://a.com/mcp")).toBe(false)
103+
expect(McpOAuthClientProvider.isKnownNonOAuth("https://b.com/mcp")).toBe(true)
104+
})
105+
106+
it("clearNonOAuthCache() with no arg clears all entries", () => {
107+
McpOAuthClientProvider.markNonOAuth("https://a.com/mcp")
108+
McpOAuthClientProvider.markNonOAuth("https://b.com/mcp")
109+
McpOAuthClientProvider.clearNonOAuthCache()
110+
expect(McpOAuthClientProvider.isKnownNonOAuth("https://a.com/mcp")).toBe(false)
111+
expect(McpOAuthClientProvider.isKnownNonOAuth("https://b.com/mcp")).toBe(false)
112+
})
113+
114+
it("entries expire after the TTL", () => {
115+
const realNow = Date.now()
116+
McpOAuthClientProvider.markNonOAuth("https://example.com/mcp")
117+
// Advance time past the 30-minute TTL
118+
const spy = vi.spyOn(Date, "now").mockReturnValue(realNow + 31 * 60 * 1000)
119+
expect(McpOAuthClientProvider.isKnownNonOAuth("https://example.com/mcp")).toBe(false)
120+
spy.mockRestore()
121+
})
85122
})
86123

87124
describe("create", () => {
@@ -95,6 +132,36 @@ describe("McpOAuthClientProvider", () => {
95132
expect(provider.redirectUrl).toBe("http://localhost:0/callback")
96133
await provider.close()
97134
})
135+
136+
it("should skip discovery when skipDiscovery option is true", async () => {
137+
const secretStorage = createMockSecretStorage()
138+
const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage, undefined, {
139+
skipDiscovery: true,
140+
})
141+
142+
expect(discoverOAuthProtectedResourceMetadata).not.toHaveBeenCalled()
143+
expect(provider.hasMetadata).toBe(false)
144+
})
145+
146+
it("should have hasMetadata true when discovery succeeds", async () => {
147+
const secretStorage = createMockSecretStorage()
148+
const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage)
149+
150+
expect(discoverOAuthProtectedResourceMetadata).toHaveBeenCalled()
151+
expect(provider.hasMetadata).toBe(true)
152+
})
153+
154+
it("should cache server as non-OAuth when discovery fails", async () => {
155+
// Use mockImplementationOnce to override just this call
156+
;(discoverOAuthProtectedResourceMetadata as any).mockImplementationOnce(() => {
157+
throw new Error("not found")
158+
})
159+
const secretStorage = createMockSecretStorage()
160+
const provider = await McpOAuthClientProvider.create("https://no-oauth.example.com/mcp", secretStorage)
161+
162+
expect(provider.hasMetadata).toBe(false)
163+
expect(McpOAuthClientProvider.isKnownNonOAuth("https://no-oauth.example.com/mcp")).toBe(true)
164+
})
98165
})
99166

100167
describe("redirectUrl (pre-server-start)", () => {

‎src/services/mcp/__tests__/SecretStorageService.spec.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,31 @@ describe("SecretStorageService", () => {
8989
})
9090
})
9191

92+
describe("hasOAuthData", () => {
93+
it("should return false when no data stored", async () => {
94+
expect(await service.hasOAuthData("https://example.com/mcp")).toBe(false)
95+
})
96+
97+
it("should return true when data is stored", async () => {
98+
const data: StoredMcpOAuthData = {
99+
tokens: { access_token: "tok", token_type: "Bearer" },
100+
expires_at: Date.now() + 3600_000,
101+
}
102+
await service.saveOAuthData("https://example.com/mcp", data)
103+
expect(await service.hasOAuthData("https://example.com/mcp")).toBe(true)
104+
})
105+
106+
it("should return false after data is deleted", async () => {
107+
const data: StoredMcpOAuthData = {
108+
tokens: { access_token: "tok", token_type: "Bearer" },
109+
expires_at: Date.now() + 3600_000,
110+
}
111+
await service.saveOAuthData("https://example.com/mcp", data)
112+
await service.deleteOAuthData("https://example.com/mcp")
113+
expect(await service.hasOAuthData("https://example.com/mcp")).toBe(false)
114+
})
115+
})
116+
92117
describe("deleteOAuthData", () => {
93118
it("should delete stored data", async () => {
94119
const data: StoredMcpOAuthData = {

0 commit comments

Comments
 (0)