diff --git a/bun.lock b/bun.lock index bc5ca40ea..77798579a 100644 --- a/bun.lock +++ b/bun.lock @@ -343,7 +343,7 @@ "version": "0.0.32", "dependencies": { "@executor-js/api": "workspace:*", - "@executor-js/emulate": "^0.13.6", + "@executor-js/emulate": "^0.13.7", "@executor-js/mcporter": "^0.11.4", "@executor-js/plugin-graphql": "workspace:*", "@executor-js/plugin-mcp": "workspace:*", @@ -1741,7 +1741,7 @@ "@executor-js/e2e": ["@executor-js/e2e@workspace:e2e"], - "@executor-js/emulate": ["@executor-js/emulate@0.13.6", "", { "dependencies": { "@aws-sdk/client-s3": "^3.1031.0", "@aws-sdk/client-sqs": "^3.1075.0", "@azure/msal-node": "^5.3.0", "@clerk/backend": "^3.8.4", "@octokit/rest": "^22.0.1", "@okta/okta-auth-js": "^8.0.1", "@slack/web-api": "^7.16.0", "@vercel/sdk": "^1.28.4", "@workos-inc/node": "^8.13.0", "atlas-api-client": "^0.3.0", "autumn-js": "^1.2.8", "commander": "^14", "googleapis": "^173.0.0", "graphql": "^16.9.0", "graphql-request": "^7.4.0", "openid-client": "^6.8.4", "picocolors": "^1.1.1", "resend": "^6.16.0", "spotify-web-api-node": "^5.0.2", "stripe": "^22.3.0", "twitter-api-v2": "^1.29.0", "yaml": "^2" }, "bin": { "emulate": "dist/index.js" } }, "sha512-FwR2RvO5DnwYgGO2w3JKPCpUx8o+AzSPJeei/oYMIrb4PYg2bQceucxiw52698t9SDEDwAOHkdC/Vv1Tv1+MJQ=="], + "@executor-js/emulate": ["@executor-js/emulate@0.13.7", "", { "dependencies": { "@aws-sdk/client-s3": "^3.1031.0", "@aws-sdk/client-sqs": "^3.1075.0", "@azure/msal-node": "^5.3.0", "@clerk/backend": "^3.8.4", "@octokit/rest": "^22.0.1", "@okta/okta-auth-js": "^8.0.1", "@slack/web-api": "^7.16.0", "@vercel/sdk": "^1.28.4", "@workos-inc/node": "^8.13.0", "atlas-api-client": "^0.3.0", "autumn-js": "^1.2.8", "commander": "^14", "googleapis": "^173.0.0", "graphql": "^16.9.0", "graphql-request": "^7.4.0", "openid-client": "^6.8.4", "picocolors": "^1.1.1", "resend": "^6.16.0", "spotify-web-api-node": "^5.0.2", "stripe": "^22.3.0", "twitter-api-v2": "^1.29.0", "yaml": "^2" }, "bin": { "emulate": "dist/index.js" } }, "sha512-119UmWRSAtagLWdH8a53X/X1/q1YjM4O3Gkp2STy4/DeSNg7CTIF4XV7gNiNTAed90iLfte1vQzTwh1o/O5aWg=="], "@executor-js/example-all-plugins": ["@executor-js/example-all-plugins@workspace:examples/all-plugins"], diff --git a/e2e/package.json b/e2e/package.json index ca937fc19..1840ee116 100644 --- a/e2e/package.json +++ b/e2e/package.json @@ -22,7 +22,7 @@ }, "dependencies": { "@executor-js/api": "workspace:*", - "@executor-js/emulate": "^0.13.6", + "@executor-js/emulate": "^0.13.7", "@executor-js/mcporter": "^0.11.4", "@executor-js/plugin-graphql": "workspace:*", "@executor-js/plugin-mcp": "workspace:*", diff --git a/e2e/scenarios/oauth-dcr-compliance.test.ts b/e2e/scenarios/oauth-dcr-compliance.test.ts new file mode 100644 index 000000000..bf4aefb77 --- /dev/null +++ b/e2e/scenarios/oauth-dcr-compliance.test.ts @@ -0,0 +1,328 @@ +// Cross-target: DCR (RFC 7591) compliance against a real emulated MCP +// authorization server — the probe → registerDynamic → start → complete +// journey driven through the product's typed API, with the MCP emulator +// seeded into each misbehavior/variation an RFC-compliant client must handle. +// +// Each scenario is one gap from the 2026-07 DCR spec audit: +// 1. RFC 8414 §3.3 — a metadata document whose `issuer` does not match the +// URL it was fetched from must be REJECTED at probe time. +// 2. RFC 9728 §3.3 — protected-resource metadata whose `resource` is not +// the requested identifier must be rejected, never adopted as the +// RFC 8707 token audience. +// 3. RFC 8414 §2 — omitted `token_endpoint_auth_methods_supported` means +// client_secret_basic, NOT `none`; a registration + token exchange +// against such a server must authenticate with HTTP Basic. +// 4. RFC 7591 §3.2.1 — the server may substitute registration metadata; +// the client must honor the RETURNED token_endpoint_auth_method (a +// server answering client_secret_basic gets Basic on /token, even when +// the client asked for `none`). +import { randomBytes } from "node:crypto"; +import { createServer } from "node:net"; + +import { expect } from "@effect/vitest"; +import { Effect, Predicate } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { createEmulator, type Emulator } from "@executor-js/emulate"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import type { OAuthProbeError } from "@executor-js/sdk"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, +} from "@executor-js/sdk/shared"; + +import { scenario } from "../src/scenario"; +import { Api, Target } from "../src/services"; + +const api = composePluginApi([mcpHttpPlugin()] as const); + +const unique = (prefix: string) => `${prefix}_${randomBytes(4).toString("hex")}`; + +/** The slug `normalizeMcpAuthMethods` assigns a slug-less oauth2 method. */ +const OAUTH2_TEMPLATE = AuthTemplateSlug.make("oauth2"); + +const availablePort = Effect.callback((resume) => { + const server = createServer(); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address ? address.port : 0; + server.close(() => { + resume(Effect.succeed(port)); + }); + }); +}); + +/** A private MCP emulator seeded with `oauth` misbehavior knobs plus one + * authorizable user. Local + in-process so each scenario owns its instance. */ +const mcpEmulator = (oauth: Record) => + Effect.acquireRelease( + Effect.gen(function* () { + const port = yield* availablePort; + const emulator = yield* Effect.promise(() => createEmulator({ service: "mcp", port })); + yield* Effect.promise(() => emulator.seed({ users: [{ login: "octocat" }], oauth })); + return emulator; + }), + (emulator: Emulator) => Effect.promise(() => emulator.close()).pipe(Effect.ignore), + ); + +/** Follow the emulator's consent screen headlessly: GET /authorize renders a + * form per seeded user; POST the approval and capture the redirect back to + * the executor callback. Returns the `code` + `state` from that location. */ +const approveConsent = (authorizationUrl: string) => + Effect.promise(async () => { + const authorize = new URL(authorizationUrl); + const page = await fetch(authorize, { redirect: "manual" }); + if (!page.ok) throw new Error(`authorize page failed: ${page.status}`); + const approval = new URL("/authorize/approve", authorize.origin); + const body = new URLSearchParams({ + client_id: authorize.searchParams.get("client_id") ?? "", + redirect_uri: authorize.searchParams.get("redirect_uri") ?? "", + state: authorize.searchParams.get("state") ?? "", + scope: authorize.searchParams.get("scope") ?? "", + code_challenge: authorize.searchParams.get("code_challenge") ?? "", + code_challenge_method: authorize.searchParams.get("code_challenge_method") ?? "", + resource: authorize.searchParams.get("resource") ?? "", + login: "octocat", + }); + const response = await fetch(approval, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: body.toString(), + redirect: "manual", + }); + const location = response.headers.get("location"); + if (response.status !== 302 || !location) { + throw new Error(`consent approval did not redirect: ${response.status}`); + } + const redirected = new URL(location); + const code = redirected.searchParams.get("code"); + const state = redirected.searchParams.get("state"); + if (!code || !state) throw new Error(`callback carried no code/state: ${location}`); + return { code, state }; + }); + +// --------------------------------------------------------------------------- +// 1. RFC 8414 issuer validation: metadata lying about its issuer is rejected. +// --------------------------------------------------------------------------- + +scenario( + "OAuth DCR · probe rejects authorization-server metadata whose issuer mismatches", + {}, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const emulator = yield* mcpEmulator({ issuerOverride: "https://evil.example.com" }); + + // The metadata document at /.well-known/... claims an issuer + // it does not live at. RFC 8414 §3.3: such metadata MUST NOT be used. + const failure = yield* Effect.flip( + client.oauth.probe({ payload: { url: `${emulator.url}/mcp` } }), + ); + expect(Predicate.isTagged("OAuthProbeError")(failure)).toBe(true); + const probeError = failure as OAuthProbeError; + expect(probeError.message).toContain("issuer"); + expect(probeError.message).toContain("https://evil.example.com"); + }), + ), +); + +// --------------------------------------------------------------------------- +// 2. RFC 9728 resource validation: PRM naming a foreign resource is rejected. +// --------------------------------------------------------------------------- + +scenario( + "OAuth DCR · probe rejects protected-resource metadata naming a foreign resource", + {}, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const emulator = yield* mcpEmulator({ + resourceOverride: "https://other.example.com/mcp", + }); + + // The PRM's `resource` is not the identifier we asked about. Accepting + // it would make the foreign URL the RFC 8707 token audience. + const failure = yield* Effect.flip( + client.oauth.probe({ payload: { url: `${emulator.url}/mcp` } }), + ); + expect(Predicate.isTagged("OAuthProbeError")(failure)).toBe(true); + const probeError = failure as OAuthProbeError; + expect(probeError.message).toContain("resource"); + expect(probeError.message).toContain("https://other.example.com/mcp"); + }), + ), +); + +// --------------------------------------------------------------------------- +// 3. RFC 8414 default auth method: omitted advertisement = client_secret_basic. +// The emulator omits token_endpoint_auth_methods_supported entirely and its +// token endpoint REQUIRES HTTP Basic for basic clients — a client that +// wrongly assumes `none` (or posts the secret in the form body) fails. +// --------------------------------------------------------------------------- + +scenario( + "OAuth DCR · omitted auth-methods metadata negotiates client_secret_basic end to end", + { + skip: "red until DCR auth-method negotiation honors the RFC 8414 client_secret_basic default (needs the oauth_client token_endpoint_auth_method column)", + }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const emulator = yield* mcpEmulator({ tokenEndpointAuthMethods: "omit" }); + + const integration = IntegrationSlug.make(unique("dcr-basic")); + yield* client.mcp.addServer({ + payload: { + transport: "remote", + name: "DCR compliance MCP", + endpoint: `${emulator.url}/mcp`, + slug: String(integration), + authenticationTemplate: [{ kind: "oauth2" }], + }, + }); + const probe = yield* client.oauth.probe({ payload: { url: `${emulator.url}/mcp` } }); + expect(probe.registrationEndpoint, "the emulator advertises DCR").toBeTruthy(); + // RFC 8414 §2: omitted means client_secret_basic — the probe must not + // fabricate a `none` capability the server never advertised. + expect(probe.tokenEndpointAuthMethodsSupported ?? null).toBeNull(); + + const registered = yield* client.oauth.registerDynamic({ + payload: { + owner: "user", + slug: OAuthClientSlug.make(unique("dcr-basic")), + issuer: probe.issuer ?? null, + registrationEndpoint: probe.registrationEndpoint!, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: probe.resource ?? null, + scopes: [...(probe.scopesSupported ?? [])], + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, + clientName: "Executor", + originIntegration: integration, + }, + }); + + const started = yield* client.oauth.start({ + payload: { + client: registered.client, + clientOwner: "user", + owner: "user", + name: ConnectionName.make("main"), + integration, + template: OAUTH2_TEMPLATE, + }, + }); + expect(started.status).toBe("redirect"); + if (started.status !== "redirect") return; + + // The token exchange must authenticate with HTTP Basic (the emulator + // rejects form-body secrets for basic clients) — completing proves the + // negotiated AND executed method is client_secret_basic. + const { code } = yield* approveConsent(started.authorizationUrl); + const connection = yield* client.oauth.complete({ + payload: { state: started.state, code }, + }); + expect(String(connection.name)).toBe("main"); + + // Completion alone is not proof: the emulator honors whatever method the + // client REQUESTED at registration, so a wrongly-negotiated `none` also + // completes (public client + PKCE). The on-wire /token request is the + // authoritative check: a Basic client sends an Authorization header + // (redacted in the ledger, but its presence survives), a `none` client + // sends none at all. + const ledger = yield* Effect.promise(() => emulator.ledger.list()); + const tokenExchange = ledger.find( + (entry) => + entry.method === "POST" && + entry.path.endsWith("/token") && + JSON.stringify(entry.request.body ?? "").includes("authorization_code"), + ); + expect(tokenExchange, "the token exchange is in the ledger").toBeDefined(); + expect( + Object.keys(tokenExchange!.request.headers).map((h) => h.toLowerCase()), + "the token exchange authenticated with HTTP Basic", + ).toContain("authorization"); + }), + ), +); + +// --------------------------------------------------------------------------- +// 4. RFC 7591 metadata substitution: the server upgrades the requested `none` +// to client_secret_basic in the registration RESPONSE; the client must use +// the returned method on /token, not the one it asked for. +// --------------------------------------------------------------------------- + +scenario( + "OAuth DCR · the server-substituted token auth method is honored on the token exchange", + { + skip: "red until the DCR response's token_endpoint_auth_method is persisted and used on /token (needs the oauth_client token_endpoint_auth_method column)", + }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const emulator = yield* mcpEmulator({ dcrAuthMethodOverride: "client_secret_basic" }); + + const integration = IntegrationSlug.make(unique("dcr-subst")); + yield* client.mcp.addServer({ + payload: { + transport: "remote", + name: "DCR substitution MCP", + endpoint: `${emulator.url}/mcp`, + slug: String(integration), + authenticationTemplate: [{ kind: "oauth2" }], + }, + }); + const probe = yield* client.oauth.probe({ payload: { url: `${emulator.url}/mcp` } }); + + // The server advertises `none` (so the client requests it) but the DCR + // response substitutes client_secret_basic + mints a secret. + const registered = yield* client.oauth.registerDynamic({ + payload: { + owner: "user", + slug: OAuthClientSlug.make(unique("dcr-subst")), + issuer: probe.issuer ?? null, + registrationEndpoint: probe.registrationEndpoint!, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: probe.resource ?? null, + scopes: [...(probe.scopesSupported ?? [])], + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, + clientName: "Executor", + originIntegration: integration, + }, + }); + + const started = yield* client.oauth.start({ + payload: { + client: registered.client, + clientOwner: "user", + owner: "user", + name: ConnectionName.make("main"), + integration, + template: OAUTH2_TEMPLATE, + }, + }); + expect(started.status).toBe("redirect"); + if (started.status !== "redirect") return; + + const { code } = yield* approveConsent(started.authorizationUrl); + const connection = yield* client.oauth.complete({ + payload: { state: started.state, code }, + }); + expect(String(connection.name)).toBe("main"); + }), + ), +); diff --git a/packages/core/sdk/src/oauth-discovery.test.ts b/packages/core/sdk/src/oauth-discovery.test.ts index 7e30d1977..5badf0b6b 100644 --- a/packages/core/sdk/src/oauth-discovery.test.ts +++ b/packages/core/sdk/src/oauth-discovery.test.ts @@ -79,14 +79,14 @@ const withOAuthFixture = ( ); describe("canonicalResourceUrl", () => { - it("lowercases scheme + host, drops trailing slash, fragment, and query", () => { + it("lowercases scheme + host, keeps query + trailing slash, and drops fragment", () => { expect(canonicalResourceUrl("https://API.Example.com/v1/mcp/")).toBe( - "https://api.example.com/v1/mcp", + "https://api.example.com/v1/mcp/", ); expect(canonicalResourceUrl("HTTPS://api.example.com/v1/mcp?x=1#frag")).toBe( - "https://api.example.com/v1/mcp", + "https://api.example.com/v1/mcp?x=1", ); - expect(canonicalResourceUrl("https://api.example.com/")).toBe("https://api.example.com"); + expect(canonicalResourceUrl("https://api.example.com/")).toBe("https://api.example.com/"); }); }); @@ -99,7 +99,7 @@ describe("discoverProtectedResourceMetadata", () => { } if (request.url === "/.well-known/oauth-protected-resource") { return sendJson({ - resource: baseUrl, + resource: `${baseUrl}/graphql`, authorization_servers: [baseUrl], scopes_supported: ["read"], }); @@ -170,6 +170,22 @@ describe("discoverAuthorizationServerMetadata", () => { ), ); + it.effect("treats a trailing slash as the same authorization-server issuer", () => + withOAuthFixture( + (_request, baseUrl) => + sendJson({ + issuer: `${baseUrl}/`, + authorization_endpoint: `${baseUrl}/authorize`, + token_endpoint: `${baseUrl}/token`, + }), + ({ baseUrl }) => + Effect.gen(function* () { + const result = yield* discoverAuthorizationServerMetadata(baseUrl); + expect(result?.metadata.issuer).toBe(`${baseUrl}/`); + }), + ), + ); + it.effect("requires issuer + authorize + token endpoints", () => withOAuthFixture( () => sendJson({ issuer: "http://127.0.0.1" }), @@ -183,9 +199,9 @@ describe("discoverAuthorizationServerMetadata", () => { it.effect("rejects unsupported authorization server endpoint URLs", () => withOAuthFixture( - () => + (_request, baseUrl) => sendJson({ - issuer: "http://example.com", + issuer: baseUrl, authorization_endpoint: "javascript:alert(1)", token_endpoint: "http://example.com/token", response_types_supported: ["code"], @@ -208,9 +224,9 @@ describe("discoverAuthorizationServerMetadata", () => { it.effect("allows HTTP authorization server endpoints when the caller opts into local HTTP", () => withOAuthFixture( - () => + (_request, baseUrl) => sendJson({ - issuer: "http://example.com", + issuer: baseUrl, authorization_endpoint: "http://example.com/authorize", token_endpoint: "http://example.com/token", response_types_supported: ["code"], @@ -329,7 +345,7 @@ describe("beginDynamicAuthorization", () => { } if (request.url === "/.well-known/oauth-protected-resource") { return sendJson({ - resource: baseUrl, + resource: `${baseUrl}/graphql/v2`, authorization_servers: [baseUrl], scopes_supported: ["openid", "profile", "email", "offline_access", "workspace:member"], }); @@ -372,11 +388,11 @@ describe("beginDynamicAuthorization", () => { expect(url.searchParams.get("response_type")).toBe("code"); expect(url.searchParams.get("state")).toBe("state-xyz"); expect(url.searchParams.get("code_challenge_method")).toBe("S256"); - expect(url.searchParams.get("resource")).toBe(baseUrl); + expect(url.searchParams.get("resource")).toBe(`${baseUrl}/graphql/v2`); expect(result.state.authorizationServerMetadata.token_endpoint).toBe( `${baseUrl}/oauth/token`, ); - expect(result.state.resourceMetadata?.resource).toBe(baseUrl); + expect(result.state.resourceMetadata?.resource).toBe(`${baseUrl}/graphql/v2`); }), ), ); @@ -621,7 +637,7 @@ describe("beginDynamicAuthorization", () => { }); const authUrl = new URL(result.authorizationUrl); - expect(authUrl.searchParams.get("resource")).toBe(`${baseUrl}/v1/mcp`); + expect(authUrl.searchParams.get("resource")).toBe(`${baseUrl}/v1/mcp/`); }), ), ); @@ -900,6 +916,47 @@ describe("beginDynamicAuthorization", () => { ), ); + it.effect("rejects an ancestor resource indicator from previous discovery state", () => + withOAuthFixture( + () => notFound(), + ({ baseUrl, requests }) => + Effect.gen(function* () { + const exit = yield* Effect.exit( + beginDynamicAuthorization({ + endpoint: `${baseUrl}/mcp`, + redirectUrl: "https://app/cb", + state: "s", + previousState: { + authorizationServerUrl: baseUrl, + authorizationServerMetadataUrl: `${baseUrl}/.well-known/oauth-authorization-server`, + authorizationServerMetadata: { + issuer: baseUrl, + authorization_endpoint: `${baseUrl}/authorize`, + token_endpoint: `${baseUrl}/token`, + }, + resourceMetadata: null, + clientInformation: { + client_id: "cached-client", + }, + resource: baseUrl, + }, + }), + ); + + expect(Exit.isFailure(exit)).toBe(true); + if (!Exit.isFailure(exit)) return; + const reason = exit.cause.reasons.find(Cause.isFailReason); + expect(reason?.error).toEqual( + expect.objectContaining({ + _tag: "OAuthDiscoveryError", + message: expect.stringMatching(/resource does not match requested endpoint/), + }), + ); + expect((yield* requests).length).toBe(0); + }), + ), + ); + it.effect("rejects servers that don't support PKCE S256", () => withOAuthFixture( (request, baseUrl) => { diff --git a/packages/core/sdk/src/oauth-discovery.ts b/packages/core/sdk/src/oauth-discovery.ts index 25786012c..c6e95b8be 100644 --- a/packages/core/sdk/src/oauth-discovery.ts +++ b/packages/core/sdk/src/oauth-discovery.ts @@ -162,12 +162,32 @@ const validateEndpointUrl = ( }), }); +/** RFC 8414 issuer identity: scheme, host, port, and path after URL + * normalization; a trailing slash alone does not create a different issuer. + * Deliberately NOT `canonicalResourceUrl`: an issuer never carries a query, + * and its trailing slash is insignificant (RFC 8414 §2), whereas a resource + * identifier's query and trailing slash are part of the identity (RFC 8707). */ +const canonicalIssuer = (value: string): string => { + const url = new URL(value); + const path = url.pathname.replace(/\/+$/, ""); + return `${url.protocol.toLowerCase()}//${url.host.toLowerCase()}${path}`; +}; + const validateAuthorizationServerMetadata = ( metadata: OAuthAuthorizationServerMetadata, + expectedIssuer: string, policy: OAuthEndpointUrlPolicy = {}, ): Effect.Effect => Effect.gen(function* () { yield* validateEndpointUrl(metadata.issuer, "issuer", policy); + if (canonicalIssuer(metadata.issuer) !== canonicalIssuer(expectedIssuer)) { + return yield* new OAuthDiscoveryError({ + message: + `Authorization server metadata issuer ${metadata.issuer} does not match the requested ` + + `issuer ${expectedIssuer}. Correct the server's issuer metadata or use the matching ` + + `authorization server URL.`, + }); + } yield* validateEndpointUrl(metadata.authorization_endpoint, "authorization_endpoint", policy); yield* validateEndpointUrl(metadata.token_endpoint, "token_endpoint", policy); if (metadata.registration_endpoint) { @@ -301,6 +321,9 @@ export const discoverProtectedResourceMetadata = ( }), ), ); + if (metadata.resource) { + yield* validateResourceIndicator(metadata.resource, canonicalResourceUrl(resourceUrl)); + } return { metadataUrl: url, metadata }; } return null; @@ -394,7 +417,7 @@ export const discoverAuthorizationServerMetadata = ( }), ), ); - yield* validateAuthorizationServerMetadata(metadata, options.endpointUrlPolicy); + yield* validateAuthorizationServerMetadata(metadata, issuer, options.endpointUrlPolicy); return { metadataUrl, metadata }; } return null; @@ -583,16 +606,15 @@ export const registerDynamicClient = ( // // MCP Authorization 2025-06-18 requires `resource` on /authorize and /token // requests. RFC 8707 §2 says the value is "an absolute URI" identifying the -// protected resource — same scheme + host + (optional) path, no fragment, -// no query, lowercased scheme/host. +// protected resource. Query components and path trailing slashes are part of +// the identifier; fragments are not sent, and scheme/host compare lowercase. // --------------------------------------------------------------------------- export const canonicalResourceUrl = (value: string): string => { const url = new URL(value); const scheme = url.protocol.toLowerCase(); const host = url.host.toLowerCase(); - const path = url.pathname.replace(/\/+$/, ""); - return `${scheme}//${host}${path}`; + return `${scheme}//${host}${url.pathname}${url.search}`; }; const validateResourceIndicator = ( @@ -608,11 +630,14 @@ const validateResourceIndicator = ( }), }).pipe( Effect.flatMap((actual) => - actual === expected || expected.startsWith(`${actual}/`) + actual === expected ? Effect.succeed(actual) : Effect.fail( new OAuthDiscoveryError({ - message: "Protected resource metadata resource does not match requested endpoint", + message: + `Protected resource metadata resource does not match requested endpoint: ` + + `advertised ${actual}; requested ${expected}. Correct the server's resource ` + + `metadata or use the matching resource URL.`, cause: { expected, actual }, }), ), diff --git a/packages/core/sdk/src/oauth-register-dynamic.test.ts b/packages/core/sdk/src/oauth-register-dynamic.test.ts index 266c7cc4d..6f69badbb 100644 --- a/packages/core/sdk/src/oauth-register-dynamic.test.ts +++ b/packages/core/sdk/src/oauth-register-dynamic.test.ts @@ -8,7 +8,7 @@ import { OAuthClientSlug, ToolName, } from "./ids"; -import { OAuthRegisterDynamicError } from "./oauth-client"; +import { OAuthProbeError, OAuthRegisterDynamicError } from "./oauth-client"; import { definePlugin } from "./plugin"; import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config"; import { serveOAuthTestServer } from "./testing/oauth-test-server"; @@ -58,6 +58,73 @@ const oauthPlugin = definePlugin(() => ({ const plugins = [memoryCredentialsPlugin(), oauthPlugin] as const; describe("oauth.registerDynamicClient", () => { + it.effect("probe rejects authorization-server metadata with a mismatched issuer", () => + Effect.scoped( + Effect.gen(function* () { + const advertisedIssuer = "https://issuer.example/mismatched"; + const server = yield* serveOAuthTestServer({ advertisedIssuer }); + const { executor } = yield* makeTestWorkspaceHarness({ plugins }); + + const error = yield* Effect.flip(executor.oauth.probe({ url: server.mcpResourceUrl })); + expect(error).toBeInstanceOf(OAuthProbeError); + if (!Predicate.isTagged(error, "OAuthProbeError")) return; + // oxlint-disable-next-line executor/no-unknown-error-message -- test: the tag guard narrows to the public OAuthProbeError contract + const message = error.message; + expect(message).toContain(server.issuerUrl); + expect(message).toContain(advertisedIssuer); + }), + ), + ); + + it.effect("probe rejects protected-resource metadata for a different resource", () => + Effect.scoped( + Effect.gen(function* () { + const advertisedResource = "https://resource.example/mismatched"; + const server = yield* serveOAuthTestServer({ advertisedResource }); + const { executor } = yield* makeTestWorkspaceHarness({ plugins }); + + const error = yield* Effect.flip(executor.oauth.probe({ url: server.mcpResourceUrl })); + expect(error).toBeInstanceOf(OAuthProbeError); + if (!Predicate.isTagged(error, "OAuthProbeError")) return; + // oxlint-disable-next-line executor/no-unknown-error-message -- test: the tag guard narrows to the public OAuthProbeError contract + const message = error.message; + expect(message).toContain(server.mcpResourceUrl); + expect(message).toContain(advertisedResource); + }), + ), + ); + + it.effect("canonicalizes the RFC 8707 resource before persisting a DCR client", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); + const { executor } = yield* makeTestWorkspaceHarness({ plugins }); + yield* executor.acme.seed(); + const rawResource = `${server.issuerUrl.replace("http://", "HTTP://")}/mcp/?tenant=Acme#fragment`; + const expectedResource = `${server.issuerUrl}/mcp/?tenant=Acme`; + + const slug = yield* executor.oauth.registerDynamicClient({ + owner: "org", + slug: CLIENT, + issuer: server.issuerUrl, + registrationEndpoint: server.registrationEndpoint, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + resource: rawResource, + scopes: ["read"], + tokenEndpointAuthMethodsSupported: ["none"], + clientName: "Executor", + redirectUri: FLOW_REDIRECT_URI, + originIntegration: INTEG, + }); + + const clients = yield* executor.oauth.listClients(); + const minted = clients.find((client) => String(client.slug) === String(slug)); + expect(minted?.resource).toBe(expectedResource); + }), + ), + ); + it.effect("DCR mints + persists a public (no-secret) client that lists + connects", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/core/sdk/src/oauth-service.ts b/packages/core/sdk/src/oauth-service.ts index 65227a0f9..71bb2690e 100644 --- a/packages/core/sdk/src/oauth-service.ts +++ b/packages/core/sdk/src/oauth-service.ts @@ -50,6 +50,7 @@ import { import type { OwnerBinding } from "./plugin"; import type { CredentialProvider } from "./provider"; import { + canonicalResourceUrl, discoverAuthorizationServerMetadata, discoverProtectedResourceMetadata, OAuthDiscoveryError, @@ -792,6 +793,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { input: RegisterDynamicClientInput, issuer: string | null, flowRedirectUri: string | null, + resource: string | null, ): Effect.Effect< { readonly existingSlug: OAuthClientSlug | null; @@ -801,7 +803,6 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { > => Effect.gen(function* () { const candidates = yield* dcrCandidatesForIssuer(input.owner, issuer); - const resource = input.resource ?? null; // A candidate is reusable only when the callback it registered with the // AS still matches the current flow's callback — strict servers reject an // authorize request whose redirect_uri differs from the registration @@ -859,11 +860,27 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { ): Effect.Effect => Effect.gen(function* () { const issuer = canonicalDcrIssuer(input.issuer, input.registrationEndpoint); + const requestedResource = input.resource ?? null; + // Canonicalized ONCE here; reuse-matching, persistence, and every later + // authorize/token request all see this exact value. Rows persisted + // before canonicalization may hold a raw string that no longer matches + // (case, trailing slash) — those clients simply re-register, which is + // safe and self-healing, so no backfill migration is warranted. + const resource = + requestedResource === null + ? null + : yield* Effect.try({ + try: () => canonicalResourceUrl(requestedResource), + catch: () => + new OAuthRegisterDynamicError({ + message: `OAuth resource must be an absolute URL: ${requestedResource}`, + }), + }); // Resolved before the reuse decision: a persisted client registered with // a DIFFERENT callback must not be reused (strict servers 400 the // authorize request), so the reuse lookup compares against this value. const flowRedirectUri = input.redirectUri ?? redirectUri ?? null; - const reuse = yield* decideDcrClientReuse(input, issuer, flowRedirectUri); + const reuse = yield* decideDcrClientReuse(input, issuer, flowRedirectUri, resource); if (reuse.existingSlug !== null) return reuse.existingSlug; const slug = reuse.registrationSlug; @@ -920,7 +937,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { slug, authorizationUrl: input.authorizationUrl, tokenUrl: input.tokenUrl, - resource: input.resource ?? null, + resource, grant: "authorization_code", clientId: information.client_id, clientSecret: information.client_secret ?? "", @@ -1427,7 +1444,13 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { const options = { endpointUrlPolicy: deps.endpointUrlPolicy }; // Try protected-resource metadata first (RFC 9728), then the AS issuer. const resource = yield* discoverProtectedResourceMetadata(input.url, options).pipe( - Effect.catch(() => Effect.succeed(null)), + Effect.mapError( + (cause) => + new OAuthProbeError({ + // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OAuthDiscoveryError carries a typed `message` field + message: `OAuth discovery failed: ${cause.message}`, + }), + ), ); // EXPLICIT discovery order: when the protected-resource metadata advertises // an authorization server, probe that; otherwise probe the input endpoint diff --git a/packages/core/sdk/src/testing/oauth-test-server.ts b/packages/core/sdk/src/testing/oauth-test-server.ts index 95ef31ec0..aa6b6799a 100644 --- a/packages/core/sdk/src/testing/oauth-test-server.ts +++ b/packages/core/sdk/src/testing/oauth-test-server.ts @@ -61,6 +61,10 @@ export interface OAuthTestServerOptions { readonly invalidRefreshTokenErrorCode?: string; readonly idTokenClaims?: Readonly>; readonly refreshIdTokenClaims?: Readonly>; + /** Override the RFC 8414 `issuer` advertised by authorization-server metadata. */ + readonly advertisedIssuer?: string; + /** Override the RFC 9728 `resource` advertised by protected-resource metadata. */ + readonly advertisedResource?: string; /** Gate Dynamic Client Registration on the requested redirect URIs. When set, * `/register` returns `400 invalid_redirect_uri` unless every requested * `redirect_uris` entry is approved. Mirrors authorization servers (e.g. @@ -487,7 +491,7 @@ export const serveOAuthTestServer = ( if (requestUrl.pathname.startsWith("/.well-known/oauth-protected-resource")) { const suffix = requestUrl.pathname.slice("/.well-known/oauth-protected-resource".length); - const resource = `${currentIssuerUrl}${suffix}`; + const resource = options.advertisedResource ?? `${currentIssuerUrl}${suffix}`; return jsonResponse(200, { resource, authorization_servers: [currentIssuerUrl], @@ -501,7 +505,7 @@ export const serveOAuthTestServer = ( requestUrl.pathname === "/.well-known/openid-configuration" ) { return jsonResponse(200, { - issuer: currentIssuerUrl, + issuer: options.advertisedIssuer ?? currentIssuerUrl, authorization_endpoint: `${currentIssuerUrl}/authorize`, token_endpoint: `${currentIssuerUrl}/token`, registration_endpoint: `${currentIssuerUrl}/register`,