Skip to content

Commit a5bd65d

Browse files
byapparovclaude
byapparov
and
claude
committed
fix(mcp): bind configured OAuth client credentials to their authorization server (#50)
Pre-registered credentials (`oauth.clientId`/`clientSecret`) were returned without an `issuer`, so the SDK treated them as unbound and would post the configured secret to any authorization server the MCP server advertised. That is the same disclosure GHSA-6qxp-vccf-f47h fixes for dynamically registered clients. - Bind configured credentials on first successful use. Only { clientId, issuer } is stored; the secret stays in config. - clientInformation() returns the stored binding as `issuer`, so the SDK refuses the secret to any other authorization server. - The binding survives SDK credential invalidation, which otherwise would let a hostile server reset it with an invalid_client error. `aictrl mcp logout` still clears it. - Tests compare origins instead of URL prefixes (CodeQL js/incomplete-url-substring-sanitization) and check both Basic auth headers and request bodies. Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 parent a0bfe2b commit a5bd65d

2 files changed

Lines changed: 122 additions & 32 deletions

File tree

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

Lines changed: 42 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,7 @@ export class McpOAuthProvider implements OAuthClientProvider {
4848

4949
async clientInformation(): Promise<OAuthClientInformation | undefined> {
5050
// Check config first (pre-registered client)
51-
if (this.config.clientId) {
52-
return {
53-
client_id: this.config.clientId,
54-
client_secret: this.config.clientSecret,
55-
}
56-
}
51+
if (this.config.clientId) return this.configuredClientInformation(this.config.clientId)
5752

5853
// Check stored client info (from dynamic registration)
5954
// Use getForUrl to validate credentials are for the current server URL
@@ -76,9 +71,7 @@ export class McpOAuthProvider implements OAuthClientProvider {
7671
}
7772

7873
async saveClientInformation(info: OAuthClientInformationFull): Promise<void> {
79-
// Configured credentials are the source of truth and are never read from storage, so
80-
// do not copy them to disk when the SDK binds them to the authorization server.
81-
if (this.config.clientId) return
74+
if (this.config.clientId) return this.bindConfiguredClient(this.config.clientId, info)
8275

8376
await McpAuth.updateClientInfo(
8477
this.mcpName,
@@ -97,6 +90,38 @@ export class McpOAuthProvider implements OAuthClientProvider {
9790
})
9891
}
9992

93+
/**
94+
* Configured credentials, stamped with the authorization server they were first accepted by.
95+
* The SDK refuses to present a stamped client secret to any other authorization server.
96+
*/
97+
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
100+
return {
101+
client_id: clientId,
102+
client_secret: this.config.clientSecret,
103+
issuer: binding,
104+
}
105+
}
106+
107+
/**
108+
* 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.
111+
*/
112+
private async bindConfiguredClient(clientId: string, info: OAuthClientInformationFull): Promise<void> {
113+
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
120+
}
121+
await McpAuth.updateClientInfo(this.mcpName, { clientId, issuer: info.issuer }, this.serverUrl)
122+
log.info("bound configured client to authorization server", { mcpName: this.mcpName, issuer: info.issuer })
123+
}
124+
100125
async tokens(): Promise<OAuthTokens | undefined> {
101126
// Use getForUrl to validate tokens are for the current server URL
102127
const entry = await McpAuth.getForUrl(this.mcpName, this.serverUrl)
@@ -165,11 +190,19 @@ export class McpOAuthProvider implements OAuthClientProvider {
165190
return
166191
}
167192

193+
// The binding of configured credentials outlives invalidation: the secret is still in
194+
// config, and dropping the binding would let the next authorization server receive it.
195+
const keepBinding = Boolean(this.config.clientId)
168196
switch (type) {
169197
case "all":
198+
if (keepBinding) {
199+
await McpAuth.set(this.mcpName, { clientInfo: entry.clientInfo, serverUrl: entry.serverUrl })
200+
break
201+
}
170202
await McpAuth.remove(this.mcpName)
171203
break
172204
case "client":
205+
if (keepBinding) break
173206
delete entry.clientInfo
174207
await McpAuth.set(this.mcpName, entry)
175208
break

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

Lines changed: 80 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,19 @@ const SERVER_URL = "https://mcp.example.com/mcp"
1919
const AS_A = "https://as-a.example.com"
2020
const AS_B = "https://as-b.example.com"
2121

22-
type Recorded = { url: string; method: string; body: string }
22+
const CONFIGURED = { clientId: "configured-client", clientSecret: "configured-secret" }
23+
24+
// `sent` is everything that could carry a credential: the body and a decoded Basic auth header.
25+
type Recorded = { url: URL; sent: string }
2326

2427
function authServer(advertised: () => string) {
2528
const requests: Recorded[] = []
2629
const fetchFn = async (input: string | URL | Request, init?: RequestInit) => {
2730
const url = new URL(input instanceof Request ? input.url : String(input))
2831
const body = init?.body ? String(init.body) : ""
29-
requests.push({ url: url.href, method: init?.method ?? "GET", body })
32+
const authorization = new Headers(init?.headers).get("authorization") ?? ""
33+
const basic = authorization.startsWith("Basic ") ? atob(authorization.slice(6)) : authorization
34+
requests.push({ url, sent: `${body}\n${basic}` })
3035

3136
if (url.origin === new URL(SERVER_URL).origin && url.pathname.startsWith("/.well-known/oauth-protected-resource")) {
3237
return Response.json({ resource: SERVER_URL, authorization_servers: [advertised()] })
@@ -54,13 +59,18 @@ function authServer(advertised: () => string) {
5459
}
5560
return new Response("not found", { status: 404 })
5661
}
57-
return { requests, fetchFn }
62+
const sentTo = (origin: string) => requests.filter((r) => r.url.origin === origin)
63+
return { requests, sentTo, fetchFn }
5864
}
5965

6066
function provider(redirects: URL[] = []) {
6167
return new McpOAuthProvider(MCP_NAME, SERVER_URL, {}, { onRedirect: (url) => void redirects.push(url) })
6268
}
6369

70+
function configuredProvider() {
71+
return new McpOAuthProvider(MCP_NAME, SERVER_URL, CONFIGURED, { onRedirect: () => {} })
72+
}
73+
6474
async function storeCredentials(issuer: string | undefined) {
6575
await McpAuth.set(
6676
MCP_NAME,
@@ -86,28 +96,14 @@ describe("McpOAuthProvider issuer binding", () => {
8696
expect((await p.tokens())?.issuer).toBe(AS_A)
8797
})
8898

89-
test("does not copy configured client credentials to storage", async () => {
90-
const p = new McpOAuthProvider(
91-
MCP_NAME,
92-
SERVER_URL,
93-
{ clientId: "configured", clientSecret: "s" },
94-
{
95-
onRedirect: () => {},
96-
},
97-
)
98-
await p.saveClientInformation({ client_id: "configured", client_secret: "s", redirect_uris: [], issuer: AS_A })
99-
100-
expect(await McpAuth.get(MCP_NAME)).toBeUndefined()
101-
})
102-
10399
test("refreshes with the authorization server that issued the tokens", async () => {
104100
await storeCredentials(AS_A)
105101
const server = authServer(() => AS_A)
106102

107103
expect(await auth(provider(), { serverUrl: SERVER_URL, fetchFn: server.fetchFn })).toBe("AUTHORIZED")
108104

109-
const refresh = server.requests.find((r) => r.url === `${AS_A}/token`)
110-
expect(refresh?.body).toContain("refresh_token=refresh-a")
105+
const refresh = server.sentTo(AS_A).find((r) => r.url.pathname === "/token")
106+
expect(refresh?.sent).toContain("refresh_token=refresh-a")
111107
const stored = await McpAuth.get(MCP_NAME)
112108
expect(stored?.tokens?.accessToken).toBe("access-from-as-a.example.com")
113109
expect(stored?.tokens?.issuer).toBe(AS_A)
@@ -120,10 +116,10 @@ describe("McpOAuthProvider issuer binding", () => {
120116

121117
expect(await auth(provider(redirects), { serverUrl: SERVER_URL, fetchFn: server.fetchFn })).toBe("REDIRECT")
122118

123-
const sentToB = server.requests.filter((r) => r.url.startsWith(AS_B)).map((r) => r.body)
124-
expect(sentToB.some((body) => body.includes("refresh-a"))).toBe(false)
125-
expect(sentToB.some((body) => body.includes("secret-a"))).toBe(false)
126-
expect(server.requests.some((r) => r.url === `${AS_B}/token`)).toBe(false)
119+
const sentToB = server.sentTo(AS_B)
120+
expect(sentToB.some((r) => r.sent.includes("refresh-a"))).toBe(false)
121+
expect(sentToB.some((r) => r.sent.includes("secret-a"))).toBe(false)
122+
expect(sentToB.some((r) => r.url.pathname === "/token")).toBe(false)
127123
// A fresh registration with the new authorization server replaces the old binding.
128124
expect(redirects[0]?.searchParams.get("client_id")).toBe("client-from-as-b.example.com")
129125
const stored = await McpAuth.get(MCP_NAME)
@@ -141,3 +137,64 @@ describe("McpOAuthProvider issuer binding", () => {
141137
expect(stored?.tokens?.issuer).toBe(AS_A)
142138
})
143139
})
140+
141+
describe("McpOAuthProvider configured client binding", () => {
142+
async function storeConfiguredBinding(issuer: string) {
143+
await McpAuth.set(
144+
MCP_NAME,
145+
{
146+
clientInfo: { clientId: CONFIGURED.clientId, issuer },
147+
tokens: { accessToken: "access-a", refreshToken: "refresh-a", expiresAt: Date.now() / 1000 - 60, issuer },
148+
oauthState: "state",
149+
},
150+
SERVER_URL,
151+
)
152+
}
153+
154+
test("binds configured credentials on first use without storing the secret", async () => {
155+
await McpAuth.set(
156+
MCP_NAME,
157+
{ tokens: { accessToken: "access-a", refreshToken: "refresh-a", expiresAt: Date.now() / 1000 - 60 } },
158+
SERVER_URL,
159+
)
160+
const server = authServer(() => AS_A)
161+
162+
expect(await auth(configuredProvider(), { serverUrl: SERVER_URL, fetchFn: server.fetchFn })).toBe("AUTHORIZED")
163+
164+
expect(server.sentTo(AS_A).find((r) => r.url.pathname === "/token")?.sent).toContain(CONFIGURED.clientSecret)
165+
const stored = await McpAuth.get(MCP_NAME)
166+
expect(stored?.clientInfo).toEqual({ clientId: CONFIGURED.clientId, issuer: AS_A })
167+
expect((await configuredProvider().clientInformation())?.issuer).toBe(AS_A)
168+
})
169+
170+
test("does not present configured credentials to a different authorization server", async () => {
171+
await storeConfiguredBinding(AS_A)
172+
const server = authServer(() => AS_B)
173+
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.
177+
await expect(
178+
auth(configuredProvider(), { serverUrl: SERVER_URL, authorizationCode: "code", fetchFn: server.fetchFn }),
179+
).rejects.toThrow()
180+
181+
const sentToB = server.sentTo(AS_B)
182+
expect(sentToB.some((r) => r.sent.includes(CONFIGURED.clientSecret))).toBe(false)
183+
expect(sentToB.some((r) => r.sent.includes("refresh-a"))).toBe(false)
184+
expect(sentToB.some((r) => r.url.pathname === "/token")).toBe(false)
185+
expect((await McpAuth.get(MCP_NAME))?.clientInfo).toEqual({ clientId: CONFIGURED.clientId, issuer: AS_A })
186+
})
187+
188+
test("keeps the configured binding when the SDK invalidates credentials", async () => {
189+
await storeConfiguredBinding(AS_A)
190+
const p = configuredProvider()
191+
192+
await p.invalidateCredentials("client")
193+
await p.invalidateCredentials("all")
194+
195+
const stored = await McpAuth.get(MCP_NAME)
196+
expect(stored?.clientInfo).toEqual({ clientId: CONFIGURED.clientId, issuer: AS_A })
197+
expect(stored?.tokens).toBeUndefined()
198+
expect((await p.clientInformation())?.issuer).toBe(AS_A)
199+
})
200+
})

0 commit comments

Comments
 (0)