fix(mcp): isolate OAuth request headers
This commit is contained in:
parent
4b83f5d8f0
commit
5823deb7c8
5 changed files with 396 additions and 35 deletions
|
|
@ -769,7 +769,10 @@ export const McpDebugCommand = effectCmd({
|
|||
prompts.log.info(`WWW-Authenticate: ${wwwAuth}`)
|
||||
}
|
||||
|
||||
if (response.status === 401) {
|
||||
if (
|
||||
response.status === 401 &&
|
||||
!Object.keys(serverConfig.headers ?? {}).some((key) => key.toLowerCase() === "authorization")
|
||||
) {
|
||||
prompts.log.warn("Server returned 401 Unauthorized")
|
||||
|
||||
// Try to discover OAuth metadata
|
||||
|
|
|
|||
|
|
@ -226,7 +226,8 @@ export const layer = Layer.effect(
|
|||
key: string,
|
||||
mcp: ConfigMCPV1.Info & { type: "remote" },
|
||||
) {
|
||||
const oauthDisabled = mcp.oauth === false
|
||||
const oauthDisabled =
|
||||
mcp.oauth === false || Object.keys(mcp.headers ?? {}).some((key) => key.toLowerCase() === "authorization")
|
||||
const oauthConfig = typeof mcp.oauth === "object" ? mcp.oauth : undefined
|
||||
const url = remoteURL(mcp.url)
|
||||
if (!url) {
|
||||
|
|
@ -797,6 +798,8 @@ export const layer = Layer.effect(
|
|||
const mcpConfig = yield* requireMcpConfig(mcpName)
|
||||
if (mcpConfig.type !== "remote") throw new Error(`MCP server ${mcpName} is not a remote server`)
|
||||
if (mcpConfig.oauth === false) throw new Error(`MCP server ${mcpName} has OAuth explicitly disabled`)
|
||||
if (Object.keys(mcpConfig.headers ?? {}).some((key) => key.toLowerCase() === "authorization"))
|
||||
throw new Error(`MCP server ${mcpName} uses static Authorization`)
|
||||
const url = remoteURL(mcpConfig.url)
|
||||
if (!url) throw new Error(`Invalid MCP URL for "${mcpName}"`)
|
||||
|
||||
|
|
|
|||
|
|
@ -49,7 +49,7 @@ const { MCP } = await import("../../src/mcp/index")
|
|||
const it = testEffect(MCP.defaultLayer)
|
||||
|
||||
describe("mcp.headers", () => {
|
||||
it.instance("headers are passed to transports when oauth is enabled (default)", () =>
|
||||
it.instance("static authorization is passed to transports without oauth", () =>
|
||||
Effect.gen(function* () {
|
||||
const mcp = yield* MCP.Service
|
||||
yield* mcp
|
||||
|
|
@ -72,8 +72,8 @@ describe("mcp.headers", () => {
|
|||
Authorization: "Bearer test-token",
|
||||
"X-Custom-Header": "custom-value",
|
||||
})
|
||||
// OAuth should be enabled by default, so authProvider should exist
|
||||
expect(call.options.authProvider).toBeDefined()
|
||||
// Static Authorization and OAuth are mutually exclusive.
|
||||
expect(call.options.authProvider).toBeUndefined()
|
||||
}
|
||||
}),
|
||||
)
|
||||
|
|
@ -105,6 +105,28 @@ describe("mcp.headers", () => {
|
|||
}),
|
||||
)
|
||||
|
||||
it.instance("case-insensitive authorization disables oauth", () =>
|
||||
Effect.gen(function* () {
|
||||
const mcp = yield* MCP.Service
|
||||
yield* mcp
|
||||
.add("test-server-lowercase-auth", {
|
||||
type: "remote",
|
||||
url: "https://example.com/mcp",
|
||||
headers: { authorization: "Bearer test-token", "X-Custom-Header": "custom-value" },
|
||||
})
|
||||
.pipe(Effect.catch(() => Effect.void))
|
||||
|
||||
expect(transportCalls.length).toBeGreaterThanOrEqual(1)
|
||||
for (const call of transportCalls) {
|
||||
expect(call.options.requestInit?.headers).toEqual({
|
||||
authorization: "Bearer test-token",
|
||||
"X-Custom-Header": "custom-value",
|
||||
})
|
||||
expect(call.options.authProvider).toBeUndefined()
|
||||
}
|
||||
}),
|
||||
)
|
||||
|
||||
it.instance("no requestInit when headers are not provided", () =>
|
||||
Effect.gen(function* () {
|
||||
const mcp = yield* MCP.Service
|
||||
|
|
|
|||
221
packages/opencode/test/mcp/oauth-header-isolation.test.ts
Normal file
221
packages/opencode/test/mcp/oauth-header-isolation.test.ts
Normal file
|
|
@ -0,0 +1,221 @@
|
|||
import { describe, expect, test } from "bun:test"
|
||||
import type { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js"
|
||||
import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"
|
||||
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"
|
||||
import type {
|
||||
OAuthClientInformationMixed,
|
||||
OAuthClientMetadata,
|
||||
OAuthTokens,
|
||||
} from "@modelcontextprotocol/sdk/shared/auth.js"
|
||||
|
||||
const canary = "resource-secret"
|
||||
const staticAuthorization = "Bearer static-resource-token"
|
||||
const basicAuthorization = `Basic ${btoa("client:secret")}`
|
||||
|
||||
describe("MCP OAuth header isolation", () => {
|
||||
test("keeps resource headers out of discovery, registration, and token exchange", async () => {
|
||||
using token = serve(() => Response.json({ access_token: "access", token_type: "Bearer", refresh_token: "refresh" }))
|
||||
using registration = serve(() =>
|
||||
Response.json({
|
||||
client_id: "client",
|
||||
client_secret: "secret",
|
||||
token_endpoint_auth_method: "client_secret_basic",
|
||||
redirect_uris: ["http://127.0.0.1/callback"],
|
||||
client_name: "OpenCode test",
|
||||
grant_types: ["authorization_code", "refresh_token"],
|
||||
response_types: ["code"],
|
||||
}),
|
||||
)
|
||||
using authorizationServer = serve(() =>
|
||||
Response.json({
|
||||
issuer: authorizationServer.origin,
|
||||
authorization_endpoint: `${authorizationServer.origin}/authorize`,
|
||||
token_endpoint: `${token.origin}/token`,
|
||||
registration_endpoint: `${registration.origin}/register`,
|
||||
response_types_supported: ["code"],
|
||||
code_challenge_methods_supported: ["S256"],
|
||||
token_endpoint_auth_methods_supported: ["client_secret_basic"],
|
||||
}),
|
||||
)
|
||||
using metadata = serve(() =>
|
||||
Response.json({
|
||||
resource: `${resource.origin}/mcp`,
|
||||
authorization_servers: [authorizationServer.origin],
|
||||
}),
|
||||
)
|
||||
using resource = serve(
|
||||
() =>
|
||||
new Response(null, {
|
||||
status: 401,
|
||||
headers: { "WWW-Authenticate": `Bearer resource_metadata="${metadata.origin}/prm"` },
|
||||
}),
|
||||
)
|
||||
const provider = createProvider()
|
||||
const transport = new StreamableHTTPClientTransport(new URL(`${resource.origin}/mcp`), {
|
||||
authProvider: provider,
|
||||
requestInit: { headers: { "X-Resource-Canary": canary, aUtHoRiZaTiOn: staticAuthorization } },
|
||||
})
|
||||
|
||||
await transport.start()
|
||||
await expect(transport.send(request())).rejects.toBeInstanceOf(UnauthorizedError)
|
||||
await transport.finishAuth("authorization-code")
|
||||
await transport.close()
|
||||
|
||||
expectResourceHeaders(resource.requests)
|
||||
expectNoResourceHeaders(metadata.requests)
|
||||
expectNoResourceHeaders(authorizationServer.requests)
|
||||
expectNoResourceHeaders(registration.requests)
|
||||
expectNoResourceHeaders(token.requests)
|
||||
expect(registration.requests).toHaveLength(1)
|
||||
expect(token.requests).toHaveLength(1)
|
||||
expectAuthorization(token.requests[0], basicAuthorization)
|
||||
expect(new URLSearchParams(token.requests[0].body).get("grant_type")).toBe("authorization_code")
|
||||
})
|
||||
|
||||
test("keeps resource headers out of token refresh", async () => {
|
||||
using refresh = serve(() => Response.json({ access_token: "fresh-access", token_type: "Bearer" }))
|
||||
using authorizationServer = serve(() =>
|
||||
Response.json({
|
||||
issuer: authorizationServer.origin,
|
||||
authorization_endpoint: `${authorizationServer.origin}/authorize`,
|
||||
token_endpoint: `${refresh.origin}/refresh`,
|
||||
response_types_supported: ["code"],
|
||||
code_challenge_methods_supported: ["S256"],
|
||||
token_endpoint_auth_methods_supported: ["client_secret_basic"],
|
||||
}),
|
||||
)
|
||||
using metadata = serve(() =>
|
||||
Response.json({
|
||||
resource: `${resource.origin}/mcp`,
|
||||
authorization_servers: [authorizationServer.origin],
|
||||
}),
|
||||
)
|
||||
using resource = serve((request) => {
|
||||
if (resource.requests.length === 1) {
|
||||
return new Response(null, {
|
||||
status: 401,
|
||||
headers: { "WWW-Authenticate": `Bearer resource_metadata="${metadata.origin}/prm"` },
|
||||
})
|
||||
}
|
||||
return Response.json({ jsonrpc: "2.0", id: 1, result: {} })
|
||||
})
|
||||
const transport = new StreamableHTTPClientTransport(new URL(`${resource.origin}/mcp`), {
|
||||
authProvider: createProvider({
|
||||
clientInformation: {
|
||||
client_id: "client",
|
||||
client_secret: "secret",
|
||||
token_endpoint_auth_method: "client_secret_basic",
|
||||
},
|
||||
tokens: { access_token: "expired-access", token_type: "Bearer", refresh_token: "refresh-token" },
|
||||
}),
|
||||
requestInit: { headers: { "X-Resource-Canary": canary, authorization: staticAuthorization } },
|
||||
})
|
||||
transport.onmessage = () => {}
|
||||
|
||||
await transport.start()
|
||||
await transport.send(request())
|
||||
await transport.close()
|
||||
|
||||
expect(resource.requests).toHaveLength(2)
|
||||
expectResourceHeaders(resource.requests, ["Bearer expired-access", "Bearer fresh-access"])
|
||||
expectNoResourceHeaders(metadata.requests)
|
||||
expectNoResourceHeaders(authorizationServer.requests)
|
||||
expectNoResourceHeaders(refresh.requests)
|
||||
expect(refresh.requests).toHaveLength(1)
|
||||
expectAuthorization(refresh.requests[0], basicAuthorization)
|
||||
expect(new URLSearchParams(refresh.requests[0].body).get("grant_type")).toBe("refresh_token")
|
||||
})
|
||||
})
|
||||
|
||||
function createProvider(initial?: {
|
||||
clientInformation?: OAuthClientInformationMixed
|
||||
tokens?: OAuthTokens
|
||||
}): OAuthClientProvider {
|
||||
let clientInformation = initial?.clientInformation
|
||||
let tokens = initial?.tokens
|
||||
let codeVerifier = ""
|
||||
return {
|
||||
redirectUrl: "http://127.0.0.1/callback",
|
||||
clientMetadata: {
|
||||
redirect_uris: ["http://127.0.0.1/callback"],
|
||||
client_name: "OpenCode test",
|
||||
grant_types: ["authorization_code", "refresh_token"],
|
||||
response_types: ["code"],
|
||||
} satisfies OAuthClientMetadata,
|
||||
clientInformation: () => clientInformation,
|
||||
saveClientInformation: (value) => {
|
||||
clientInformation = value
|
||||
},
|
||||
tokens: () => tokens,
|
||||
saveTokens: (value) => {
|
||||
tokens = value
|
||||
},
|
||||
redirectToAuthorization: () => {},
|
||||
saveCodeVerifier: (value) => {
|
||||
codeVerifier = value
|
||||
},
|
||||
codeVerifier: () => codeVerifier,
|
||||
state: () => "state",
|
||||
}
|
||||
}
|
||||
|
||||
function request() {
|
||||
return {
|
||||
jsonrpc: "2.0" as const,
|
||||
id: 1,
|
||||
method: "initialize",
|
||||
params: {
|
||||
protocolVersion: "2025-06-18",
|
||||
capabilities: {},
|
||||
clientInfo: { name: "opencode-test", version: "1" },
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
function expectResourceHeaders(requests: CapturedRequest[], authorization: string | string[] = staticAuthorization) {
|
||||
expect(requests.length).toBeGreaterThan(0)
|
||||
for (const [index, request] of requests.entries()) {
|
||||
expect(request.headers.get("x-resource-canary")).toBe(canary)
|
||||
expectAuthorization(request, Array.isArray(authorization) ? authorization[index] : authorization)
|
||||
}
|
||||
}
|
||||
|
||||
function expectNoResourceHeaders(requests: CapturedRequest[]) {
|
||||
expect(requests.length).toBeGreaterThan(0)
|
||||
for (const request of requests) {
|
||||
expect(request.headers.get("x-resource-canary")).toBeNull()
|
||||
expect(request.headers.get("authorization")).not.toBe(staticAuthorization)
|
||||
}
|
||||
}
|
||||
|
||||
function expectAuthorization(request: CapturedRequest, value: string) {
|
||||
expect([...request.headers].filter(([name]) => name === "authorization")).toEqual([["authorization", value]])
|
||||
}
|
||||
|
||||
interface CapturedRequest {
|
||||
url: string
|
||||
headers: Headers
|
||||
body: string
|
||||
}
|
||||
|
||||
function serve(handler: (request: CapturedRequest) => Response | Promise<Response>) {
|
||||
const requests: CapturedRequest[] = []
|
||||
const server = Bun.serve({
|
||||
hostname: "127.0.0.1",
|
||||
port: 0,
|
||||
async fetch(request) {
|
||||
const captured = {
|
||||
url: request.url,
|
||||
headers: new Headers(request.headers),
|
||||
body: await request.text(),
|
||||
}
|
||||
requests.push(captured)
|
||||
return handler(captured)
|
||||
},
|
||||
})
|
||||
return {
|
||||
origin: `http://127.0.0.1:${server.port}`,
|
||||
requests,
|
||||
[Symbol.dispose]: () => server.stop(true),
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue