Skip to content

Commit 9ec6cf4

Browse files
byapparovclaude
byapparov
and
claude
committed
fix(mcp): harden the configured OAuth client binding (#169 review)
- Read the configured-client binding regardless of the stored MCP server URL. It pins the authorization server, not the resource URL. - Abort with an actionable error, instead of redirecting to an authorization whose code exchange the SDK would refuse, when the configured client is bound to another authorization server. - On invalidation, keep only { clientId, issuer } for the configured client. Remove the entry when there is no binding, so no empty entry or stale dynamic client secret is left behind. - Pin @babel/core through the workspace catalog, so the root override and the packages/cli dependency cannot drift apart. Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 parent a5bd65d commit 9ec6cf4

5 files changed

Lines changed: 61 additions & 23 deletions

File tree

‎bun.lock‎

Lines changed: 3 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
"packages/*"
1919
],
2020
"catalog": {
21+
"@babel/core": "7.29.7",
2122
"hono": "4.10.7",
2223
"hono-openapi": "1.1.2",
2324
"@hono/zod-validator": "0.4.2",
@@ -96,7 +97,7 @@
9697
"web-tree-sitter"
9798
],
9899
"overrides": {
99-
"@babel/core": "7.29.7",
100+
"@babel/core": "catalog:",
100101
"@types/bun": "catalog:",
101102
"@types/node": "catalog:"
102103
},

‎packages/cli/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
"@ai-sdk/vercel": "1.0.33",
5353
"@ai-sdk/xai": "2.0.51",
5454
"@aws-sdk/credential-providers": "3.993.0",
55-
"@babel/core": "7.29.7",
55+
"@babel/core": "catalog:",
5656
"@gitlab/gitlab-ai-provider": "3.6.0",
5757
"@octokit/webhooks-types": "7.6.1",
5858
"@openrouter/ai-sdk-provider": "1.5.4",

‎packages/cli/src/mcp/oauth-provider.ts‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -95,28 +95,36 @@ export class McpOAuthProvider implements OAuthClientProvider {
9595
* The SDK refuses to present a stamped client secret to any other authorization server.
9696
*/
9797
private async configuredClientInformation(clientId: string): Promise<OAuthClientInformation> {
98-
const entry = await McpAuth.getForUrl(this.mcpName, this.serverUrl)
99-
const binding = entry?.clientInfo?.clientId === clientId ? entry.clientInfo.issuer : undefined
10098
return {
10199
client_id: clientId,
102100
client_secret: this.config.clientSecret,
103-
issuer: binding,
101+
issuer: (await this.configuredBinding(clientId))?.issuer,
104102
}
105103
}
106104

105+
/**
106+
* The stored binding of the configured client. It pins the authorization server, not the MCP
107+
* server URL, so it is read regardless of the URL the entry was saved for.
108+
*/
109+
private async configuredBinding(clientId: string): Promise<McpAuth.ClientInfo | undefined> {
110+
const clientInfo = (await McpAuth.get(this.mcpName))?.clientInfo
111+
if (clientInfo?.clientId !== clientId) return undefined
112+
return { clientId, issuer: clientInfo.issuer }
113+
}
114+
107115
/**
108116
* Stores only the authorization-server binding of configured credentials; the secret stays
109-
* in config. A different client_id is a registration with another authorization server,
110-
* which the configured client never uses, so it is not stored.
117+
* in config. A different client_id means the SDK registered with another authorization server
118+
* because the configured client is bound elsewhere. Abort instead of starting an authorization
119+
* whose code exchange would be refused.
111120
*/
112121
private async bindConfiguredClient(clientId: string, info: OAuthClientInformationFull): Promise<void> {
113122
if (info.client_id !== clientId) {
114-
log.warn("configured OAuth client is bound to another authorization server; not using it", {
115-
mcpName: this.mcpName,
116-
advertisedIssuer: info.issuer,
117-
hint: `run \`aictrl mcp logout ${this.mcpName}\` if the authorization server has moved`,
118-
})
119-
return
123+
const bound = (await this.configuredBinding(clientId))?.issuer
124+
throw new Error(
125+
`MCP server "${this.mcpName}" advertises authorization server ${info.issuer}, but its configured OAuth client ` +
126+
`is bound to ${bound}. Run \`aictrl mcp logout ${this.mcpName}\` if the authorization server has moved.`,
127+
)
120128
}
121129
await McpAuth.updateClientInfo(this.mcpName, { clientId, issuer: info.issuer }, this.serverUrl)
122130
log.info("bound configured client to authorization server", { mcpName: this.mcpName, issuer: info.issuer })
@@ -192,18 +200,18 @@ export class McpOAuthProvider implements OAuthClientProvider {
192200

193201
// The binding of configured credentials outlives invalidation: the secret is still in
194202
// config, and dropping the binding would let the next authorization server receive it.
195-
const keepBinding = Boolean(this.config.clientId)
203+
const binding = this.config.clientId ? await this.configuredBinding(this.config.clientId) : undefined
196204
switch (type) {
197205
case "all":
198-
if (keepBinding) {
199-
await McpAuth.set(this.mcpName, { clientInfo: entry.clientInfo, serverUrl: entry.serverUrl })
206+
if (binding) {
207+
await McpAuth.set(this.mcpName, { clientInfo: binding, serverUrl: entry.serverUrl })
200208
break
201209
}
202210
await McpAuth.remove(this.mcpName)
203211
break
204212
case "client":
205-
if (keepBinding) break
206-
delete entry.clientInfo
213+
if (binding) entry.clientInfo = binding
214+
else delete entry.clientInfo
207215
await McpAuth.set(this.mcpName, entry)
208216
break
209217
case "tokens":

‎packages/cli/test/mcp/oauth-issuer-binding.test.ts‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -171,9 +171,11 @@ describe("McpOAuthProvider configured client binding", () => {
171171
await storeConfiguredBinding(AS_A)
172172
const server = authServer(() => AS_B)
173173

174-
// The SDK registers a throwaway client with the new server instead of using the configured one,
175-
expect(await auth(configuredProvider(), { serverUrl: SERVER_URL, fetchFn: server.fetchFn })).toBe("REDIRECT")
176-
// and the code exchange that would carry the configured secret is refused.
174+
// Aborts before redirecting to an authorization whose code exchange would be refused,
175+
await expect(auth(configuredProvider(), { serverUrl: SERVER_URL, fetchFn: server.fetchFn })).rejects.toThrow(
176+
`aictrl mcp logout ${MCP_NAME}`,
177+
)
178+
// and a code exchange that would carry the configured secret is refused by the SDK.
177179
await expect(
178180
auth(configuredProvider(), { serverUrl: SERVER_URL, authorizationCode: "code", fetchFn: server.fetchFn }),
179181
).rejects.toThrow()
@@ -185,6 +187,19 @@ describe("McpOAuthProvider configured client binding", () => {
185187
expect((await McpAuth.get(MCP_NAME))?.clientInfo).toEqual({ clientId: CONFIGURED.clientId, issuer: AS_A })
186188
})
187189

190+
test("keeps the configured binding when the MCP server URL changes", async () => {
191+
await McpAuth.set(
192+
MCP_NAME,
193+
{ clientInfo: { clientId: CONFIGURED.clientId, issuer: AS_A } },
194+
"https://old-mcp.example.com/mcp",
195+
)
196+
const server = authServer(() => AS_B)
197+
198+
expect((await configuredProvider().clientInformation())?.issuer).toBe(AS_A)
199+
await expect(auth(configuredProvider(), { serverUrl: SERVER_URL, fetchFn: server.fetchFn })).rejects.toThrow()
200+
expect(server.sentTo(AS_B).some((r) => r.sent.includes(CONFIGURED.clientSecret))).toBe(false)
201+
})
202+
188203
test("keeps the configured binding when the SDK invalidates credentials", async () => {
189204
await storeConfiguredBinding(AS_A)
190205
const p = configuredProvider()
@@ -197,4 +212,17 @@ describe("McpOAuthProvider configured client binding", () => {
197212
expect(stored?.tokens).toBeUndefined()
198213
expect((await p.clientInformation())?.issuer).toBe(AS_A)
199214
})
215+
216+
test("invalidating all removes the entry when there is no configured binding to keep", async () => {
217+
const p = configuredProvider()
218+
219+
await McpAuth.set(MCP_NAME, { tokens: { accessToken: "access-a" } }, SERVER_URL)
220+
await p.invalidateCredentials("all")
221+
expect(await McpAuth.get(MCP_NAME)).toBeUndefined()
222+
223+
// A client registered dynamically before `oauth.clientId` was configured is not a binding.
224+
await McpAuth.set(MCP_NAME, { clientInfo: { clientId: "old-dynamic", clientSecret: "old-secret" } }, SERVER_URL)
225+
await p.invalidateCredentials("all")
226+
expect(await McpAuth.get(MCP_NAME)).toBeUndefined()
227+
})
200228
})

0 commit comments

Comments
 (0)