Skip to content

Commit cfdd03d

Browse files
SoccerField24x7SoccerField24x7
committed
fix(mcp): keep OAuth requests consistent (#2096)
1 parent b75d677 commit cfdd03d

5 files changed

Lines changed: 111 additions & 53 deletions

File tree

‎src/eslint-suppressions.json‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1501,19 +1501,14 @@
15011501
},
15021502
"services/mcp/__tests__/McpOAuthClientProvider.spec.ts": {
15031503
"@typescript-eslint/no-explicit-any": {
1504-
"count": 20
1504+
"count": 19
15051505
}
15061506
},
15071507
"services/mcp/__tests__/SecretStorageService.spec.ts": {
15081508
"@typescript-eslint/no-explicit-any": {
15091509
"count": 1
15101510
}
15111511
},
1512-
"services/mcp/utils/__tests__/callbackServer.spec.ts": {
1513-
"@typescript-eslint/no-explicit-any": {
1514-
"count": 4
1515-
}
1516-
},
15171512
"services/mcp/utils/__tests__/oauth.spec.ts": {
15181513
"@typescript-eslint/no-explicit-any": {
15191514
"count": 9

‎src/services/mcp/McpOAuthClientProvider.ts‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,8 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
8989
private _refreshPromise: Promise<OAuthTokens> | null = null
9090
/** Stored by redirectToAuthorization(); opened on-demand via openBrowser(). */
9191
private _pendingAuthorizationUrl: URL | null = null
92+
/** Resource used by the authorization request and subsequent token requests. */
93+
private _effectiveResourceIndicator: string | null
9294
/** Deduplicates concurrent _ensureCallbackServer() calls. */
9395
private _ensureServerPromise: Promise<void> | null = null
9496

@@ -109,7 +111,9 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
109111
private readonly _clientName: string,
110112
private readonly _configuredRedirectUri?: string,
111113
private readonly _configuredClientId?: string,
112-
) {}
114+
) {
115+
this._effectiveResourceIndicator = _resourceIndicator
116+
}
113117

114118
/**
115119
* Factory — discovers OAuth Authorization Server metadata once (RFC 9728 +
@@ -184,7 +188,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
184188

185189
if (options?.clientId) {
186190
if (options.redirectUri) {
187-
getCallbackPort(options.redirectUri)
191+
getCallbackAddress(options.redirectUri)
188192
} else {
189193
// A static client skips DCR, which normally starts the callback server.
190194
// Start it here so the SDK never receives localhost:0 as redirect_uri.
@@ -226,9 +230,11 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
226230

227231
private async _doStartCallbackServer(): Promise<void> {
228232
this._closed = false
233+
const { port: configuredPort, host } = getCallbackAddress(this._configuredRedirectUri)
229234
const { server, port, result, cancel } = await startCallbackServer(
230-
getCallbackPort(this._configuredRedirectUri) ?? this._port,
235+
configuredPort ?? this._port,
231236
this._state,
237+
host,
232238
)
233239
this._server = server
234240
this._port = port
@@ -395,8 +401,12 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
395401
// an SDK-generated value unless the configuration disables it.
396402
if (this._resourceIndicator) {
397403
authorizationUrl.searchParams.set("resource", this._resourceIndicator)
404+
this._effectiveResourceIndicator = this._resourceIndicator
398405
} else if (this._resourceDisabled) {
399406
authorizationUrl.searchParams.delete("resource")
407+
this._effectiveResourceIndicator = null
408+
} else {
409+
this._effectiveResourceIndicator = authorizationUrl.searchParams.get("resource")
400410
}
401411
let correctedUrl = authorizationUrl
402412
if (this._authServerMeta?.authorization_endpoint) {
@@ -508,8 +518,8 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
508518

509519
// RFC 8707: include resource indicator so servers that bind token requests
510520
// to a specific resource can validate the exchange.
511-
if (this._resourceIndicator) {
512-
params.resource = this._resourceIndicator
521+
if (this._effectiveResourceIndicator) {
522+
params.resource = this._effectiveResourceIndicator
513523
}
514524

515525
// Include client_secret in the body when the auth method is client_secret_post.
@@ -560,8 +570,8 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
560570
}
561571

562572
// RFC 8707: include resource indicator in refresh requests too.
563-
if (this._resourceIndicator) {
564-
params.resource = this._resourceIndicator
573+
if (this._effectiveResourceIndicator) {
574+
params.resource = this._effectiveResourceIndicator
565575
}
566576

567577
if (this._tokenEndpointAuthMethod === "client_secret_post" && this._clientInfo?.client_secret) {
@@ -610,8 +620,8 @@ function resolveResourceIndicator(configured: McpOAuthResource | undefined, disc
610620
return discovered ?? null
611621
}
612622

613-
function getCallbackPort(redirectUri?: string): number | undefined {
614-
if (!redirectUri) return undefined
623+
function getCallbackAddress(redirectUri?: string): { port?: number; host: "localhost" | "127.0.0.1" } {
624+
if (!redirectUri) return { host: "localhost" }
615625
const url = new URL(redirectUri)
616626
if (url.hostname !== "localhost" && url.hostname !== "127.0.0.1") {
617627
throw new Error("MCP OAuth redirectUri must use a loopback host")
@@ -629,5 +639,5 @@ function getCallbackPort(redirectUri?: string): number | undefined {
629639
if (!Number.isInteger(port) || port < 1 || port > 65535) {
630640
throw new Error("MCP OAuth redirectUri must specify a valid port")
631641
}
632-
return port
642+
return { port, host: url.hostname }
633643
}

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

Lines changed: 67 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -168,36 +168,42 @@ describe("McpOAuthClientProvider", () => {
168168
expect(provider.hasMetadata).toBe(false)
169169
expect(McpOAuthClientProvider.isKnownNonOAuth("https://no-oauth.example.com/mcp")).toBe(true)
170170
})
171-
it("should initialize a static client with configured scopes and callback", async () => {
172-
setupCallbackServerMock()
173-
const provider = await McpOAuthClientProvider.create(
174-
"https://example.com/mcp",
175-
createMockSecretStorage(),
176-
undefined,
177-
{
178-
clientId: "registered-client",
179-
scope: "openid profile offline_access",
180-
redirectUri: "http://localhost:33418/callback",
181-
resource: false,
182-
},
183-
)
171+
it.each([
172+
["http://localhost:33418/callback", "localhost"],
173+
["http://127.0.0.1:33418/callback", "127.0.0.1"],
174+
] as const)(
175+
"should initialize a static client with configured scopes and callback %s",
176+
async (redirectUri, callbackHost) => {
177+
setupCallbackServerMock()
178+
const provider = await McpOAuthClientProvider.create(
179+
"https://example.com/mcp",
180+
createMockSecretStorage(),
181+
undefined,
182+
{
183+
clientId: "registered-client",
184+
scope: "openid profile offline_access",
185+
redirectUri,
186+
resource: false,
187+
},
188+
)
184189

185-
expect(await provider.clientInformation()).toMatchObject({
186-
client_id: "registered-client",
187-
redirect_uris: ["http://localhost:33418/callback"],
188-
token_endpoint_auth_method: "none",
189-
})
190-
expect(provider.redirectUrl).toBe("http://localhost:33418/callback")
190+
expect(await provider.clientInformation()).toMatchObject({
191+
client_id: "registered-client",
192+
redirect_uris: [redirectUri],
193+
token_endpoint_auth_method: "none",
194+
})
195+
expect(provider.redirectUrl).toBe(redirectUri)
191196

192-
await provider.redirectToAuthorization(
193-
new URL("https://auth.example.com/authorize?client_id=registered-client"),
194-
)
195-
await provider.openBrowser()
196-
const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString()
197-
expect(new URL(openedUri).searchParams.get("scope")).toBe("openid profile offline_access")
198-
expect(startCallbackServer).toHaveBeenCalledWith(33418, expect.any(String))
199-
await provider.close()
200-
})
197+
await provider.redirectToAuthorization(
198+
new URL("https://auth.example.com/authorize?client_id=registered-client"),
199+
)
200+
await provider.openBrowser()
201+
const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString()
202+
expect(new URL(openedUri).searchParams.get("scope")).toBe("openid profile offline_access")
203+
expect(startCallbackServer).toHaveBeenCalledWith(33418, expect.any(String), callbackHost)
204+
await provider.close()
205+
},
206+
)
201207

202208
it.each([
203209
"https://example.com:33418/callback",
@@ -603,22 +609,51 @@ describe("McpOAuthClientProvider", () => {
603609
})
604610

605611
it("should preserve an SDK resource when provider discovery finds no resource", async () => {
606-
;(discoverOAuthProtectedResourceMetadata as any).mockResolvedValueOnce({
612+
vi.mocked(discoverOAuthProtectedResourceMetadata).mockResolvedValueOnce({
613+
resource: "",
607614
authorization_servers: ["https://auth.example.com"],
608615
})
616+
mockFetch
617+
.mockResolvedValueOnce({
618+
ok: true,
619+
json: () =>
620+
Promise.resolve({
621+
issuer: "https://auth.example.com",
622+
authorization_endpoint: "https://auth.example.com/authorize",
623+
token_endpoint: "https://auth.example.com/token",
624+
token_endpoint_auth_methods_supported: ["none"],
625+
grant_types_supported: ["authorization_code"],
626+
}),
627+
})
628+
.mockResolvedValueOnce({
629+
ok: true,
630+
json: () => Promise.resolve({ access_token: "access-token", token_type: "Bearer" }),
631+
})
609632
setupCallbackServerMock()
610-
const provider = await McpOAuthClientProvider.create("https://example.com/mcp", createMockSecretStorage())
633+
const provider = await McpOAuthClientProvider.create(
634+
"https://example.com/mcp",
635+
createMockSecretStorage(),
636+
undefined,
637+
{ clientId: "registered-client" },
638+
)
611639

640+
const resource = "https://example.com/"
612641
const sdkUrl = new URL(
613-
"https://auth.example.com/authorize?client_id=abc&resource=https%3A%2F%2Fexample.com%2F&state=123",
642+
`https://auth.example.com/authorize?client_id=registered-client&resource=${encodeURIComponent(resource)}&state=123`,
614643
)
615644
await provider.redirectToAuthorization(sdkUrl)
616645
await provider.openBrowser()
646+
await provider.saveCodeVerifier("verifier")
647+
await provider.exchangeCodeForTokens("authorization-code")
617648

618649
const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString()
619650
const openedUrl = new URL(openedUri)
620-
expect(openedUrl.searchParams.get("resource")).toBe("https://example.com/")
651+
expect(openedUrl.searchParams.get("resource")).toBe(resource)
621652
expect(openedUrl.searchParams.getAll("resource")).toHaveLength(1)
653+
654+
const tokenCall = mockFetch.mock.calls.at(-1)
655+
const tokenBody = new URLSearchParams(tokenCall?.[1]?.body as string)
656+
expect(tokenBody.get("resource")).toBe(resource)
622657
await provider.close()
623658
})
624659

‎src/services/mcp/utils/__tests__/callbackServer.spec.ts‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,14 @@ describe("startCallbackServer", () => {
3939
it("should start server and resolve with callback result", async () => {
4040
const mockServer = createMockServer()
4141

42-
;(http.createServer as any).mockReturnValue(mockServer)
42+
vi.mocked(http.createServer).mockReturnValue(mockServer as unknown as http.Server)
4343

4444
const promise = startCallbackServer()
4545
const { server, port, result } = await promise
4646

4747
expect(port).toBe(3000)
4848
expect(server).toBe(mockServer)
49+
expect(mockServer.listen).toHaveBeenCalledWith(0, "localhost", expect.any(Function))
4950

5051
// Simulate callback request
5152
const requestCall = mockServer.on.mock.calls.find((call) => call[0] === "request")
@@ -70,10 +71,23 @@ describe("startCallbackServer", () => {
7071
expect(mockRes.end).toHaveBeenCalledWith(expect.stringContaining('<meta charset="utf-8">'))
7172
})
7273

74+
it("should bind to the configured loopback host", async () => {
75+
const mockServer = createMockServer()
76+
77+
vi.mocked(http.createServer).mockReturnValue(mockServer as unknown as http.Server)
78+
79+
const { result, cancel } = await startCallbackServer(33418, undefined, "127.0.0.1")
80+
const cancellation = expect(result).rejects.toThrow("Callback cancelled")
81+
cancel()
82+
83+
expect(mockServer.listen).toHaveBeenCalledWith(33418, "127.0.0.1", expect.any(Function))
84+
await cancellation
85+
})
86+
7387
it("should reject invalid state", async () => {
7488
const mockServer = createMockServer()
7589

76-
;(http.createServer as any).mockReturnValue(mockServer)
90+
vi.mocked(http.createServer).mockReturnValue(mockServer as unknown as http.Server)
7791

7892
const promise = startCallbackServer(undefined, "expected-state")
7993
const { result } = await promise
@@ -106,7 +120,7 @@ describe("stopCallbackServer", () => {
106120
close: vi.fn((callback) => callback()),
107121
}
108122

109-
await stopCallbackServer(mockServer as any, () => {})
123+
await stopCallbackServer(mockServer as unknown as http.Server, () => {})
110124
expect(mockServer.close).toHaveBeenCalled()
111125
})
112126

@@ -116,7 +130,7 @@ describe("stopCallbackServer", () => {
116130
}
117131
const cancel = vi.fn()
118132

119-
await stopCallbackServer(mockServer as any, cancel)
133+
await stopCallbackServer(mockServer as unknown as http.Server, cancel)
120134
expect(cancel).toHaveBeenCalledTimes(1)
121135
expect(mockServer.close).toHaveBeenCalled()
122136
})

‎src/services/mcp/utils/callbackServer.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,19 @@ export interface CallbackResult {
1212
state?: string
1313
}
1414

15+
type CallbackHost = "localhost" | "127.0.0.1"
16+
1517
/**
1618
* Starts a local HTTP server to handle OAuth callback.
1719
* @param port Optional port to use (defaults to random available port)
1820
* @param expectedState Optional expected state for CSRF protection
21+
* @param host Loopback host used by the OAuth redirect URI
1922
* @returns Promise<{server: http.Server, port: number, result: Promise<CallbackResult>}>
2023
*/
2124
export function startCallbackServer(
2225
port?: number,
2326
expectedState?: string,
27+
host: CallbackHost = "localhost",
2428
): Promise<{
2529
server: http.Server
2630
port: number
@@ -43,7 +47,7 @@ export function startCallbackServer(
4347
return new Promise((resolve, reject) => {
4448
const server = http.createServer()
4549

46-
server.listen(port || 0, "127.0.0.1", () => {
50+
server.listen(port || 0, host, () => {
4751
const address = server.address()
4852
if (!address || typeof address === "string") {
4953
reject(new Error("Failed to get server address"))

0 commit comments

Comments
 (0)