History

fix oauth discovery metadata validation

Rhys Sullivan committed 3 months ago

Commit 7a4abc01 parent 8926d665 files · +194 −27Browse files

Unified · Split

5 files
  1. packages/core/sdk/src/oauth-discovery.test.ts +70 −13
  2. packages/core/sdk/src/oauth-discovery.ts +28 −7
  3. packages/core/sdk/src/oauth-register-dynamic.test.ts +68 −1
  4. packages/core/sdk/src/oauth-service.ts +22 −4
  5. packages/core/sdk/src/testing/oauth-test-server.ts +6 −2
packages/core/sdk/src/oauth-discovery.test.ts+70−13
@@ -79,14 +79,14 @@ const withOAuthFixture = <A, E>( · 78 unmodified lines
7979 );
8080
8181 describe("canonicalResourceUrl", () => {
82− it("lowercases scheme + host, drops trailing slash, fragment, and query", () => {
82+ it("lowercases scheme + host, keeps query + trailing slash, and drops fragment", () => {
8383 expect(canonicalResourceUrl("https://API.Example.com/v1/mcp/")).toBe(
84− "https://api.example.com/v1/mcp",
84+ "https://api.example.com/v1/mcp/",
8585 );
8686 expect(canonicalResourceUrl("HTTPS://api.example.com/v1/mcp?x=1#frag")).toBe(
87− "https://api.example.com/v1/mcp",
87+ "https://api.example.com/v1/mcp?x=1",
8888 );
89− expect(canonicalResourceUrl("https://api.example.com/")).toBe("https://api.example.com");
89+ expect(canonicalResourceUrl("https://api.example.com/")).toBe("https://api.example.com/");
9090 });
9191 });
9292
@@ -99,7 +99,7 @@ describe("discoverProtectedResourceMetadata", () => { · 6 unmodified lines
9999 }
100100 if (request.url === "/.well-known/oauth-protected-resource") {
101101 return sendJson({
102− resource: baseUrl,
102+ resource: `${baseUrl}/graphql`,
103103 authorization_servers: [baseUrl],
104104 scopes_supported: ["read"],
105105 });
@@ -170,6 +170,22 @@ describe("discoverAuthorizationServerMetadata", () => { · 64 unmodified lines
170170 ),
171171 );
172172
173+ it.effect("treats a trailing slash as the same authorization-server issuer", () =>
174+ withOAuthFixture(
175+ (_request, baseUrl) =>
176+ sendJson({
177+ issuer: `${baseUrl}/`,
178+ authorization_endpoint: `${baseUrl}/authorize`,
179+ token_endpoint: `${baseUrl}/token`,
180+ }),
181+ ({ baseUrl }) =>
182+ Effect.gen(function* () {
183+ const result = yield* discoverAuthorizationServerMetadata(baseUrl);
184+ expect(result?.metadata.issuer).toBe(`${baseUrl}/`);
185+ }),
186+ ),
187+ );
188+
173189 it.effect("requires issuer + authorize + token endpoints", () =>
174190 withOAuthFixture(
175191 () => sendJson({ issuer: "http://127.0.0.1" }),
@@ -183,9 +199,9 @@ describe("discoverAuthorizationServerMetadata", () => { · 7 unmodified lines
183199
184200 it.effect("rejects unsupported authorization server endpoint URLs", () =>
185201 withOAuthFixture(
186− () =>
202+ (_request, baseUrl) =>
187203 sendJson({
188− issuer: "http://example.com",
204+ issuer: baseUrl,
189205 authorization_endpoint: "javascript:alert(1)",
190206 token_endpoint: "http://example.com/token",
191207 response_types_supported: ["code"],
@@ -208,9 +224,9 @@ describe("discoverAuthorizationServerMetadata", () => { · 16 unmodified lines
208224
209225 it.effect("allows HTTP authorization server endpoints when the caller opts into local HTTP", () =>
210226 withOAuthFixture(
211− () =>
227+ (_request, baseUrl) =>
212228 sendJson({
213− issuer: "http://example.com",
229+ issuer: baseUrl,
214230 authorization_endpoint: "http://example.com/authorize",
215231 token_endpoint: "http://example.com/token",
216232 response_types_supported: ["code"],
@@ -329,7 +345,7 @@ describe("beginDynamicAuthorization", () => { · 112 unmodified lines
329345 }
330346 if (request.url === "/.well-known/oauth-protected-resource") {
331347 return sendJson({
332− resource: baseUrl,
348+ resource: `${baseUrl}/graphql/v2`,
333349 authorization_servers: [baseUrl],
334350 scopes_supported: ["openid", "profile", "email", "offline_access", "workspace:member"],
335351 });
@@ -372,11 +388,11 @@ describe("beginDynamicAuthorization", () => { · 36 unmodified lines
372388 expect(url.searchParams.get("response_type")).toBe("code");
373389 expect(url.searchParams.get("state")).toBe("state-xyz");
374390 expect(url.searchParams.get("code_challenge_method")).toBe("S256");
375− expect(url.searchParams.get("resource")).toBe(baseUrl);
391+ expect(url.searchParams.get("resource")).toBe(`${baseUrl}/graphql/v2`);
376392 expect(result.state.authorizationServerMetadata.token_endpoint).toBe(
377393 `${baseUrl}/oauth/token`,
378394 );
379− expect(result.state.resourceMetadata?.resource).toBe(baseUrl);
395+ expect(result.state.resourceMetadata?.resource).toBe(`${baseUrl}/graphql/v2`);
380396 }),
381397 ),
382398 );
@@ -621,7 +637,7 @@ describe("beginDynamicAuthorization", () => { · 238 unmodified lines
621637 });
622638
623639 const authUrl = new URL(result.authorizationUrl);
624− expect(authUrl.searchParams.get("resource")).toBe(`${baseUrl}/v1/mcp`);
640+ expect(authUrl.searchParams.get("resource")).toBe(`${baseUrl}/v1/mcp/`);
625641 }),
626642 ),
627643 );
@@ -900,6 +916,47 @@ describe("beginDynamicAuthorization", () => { · 272 unmodified lines
900916 ),
901917 );
902918
919+ it.effect("rejects an ancestor resource indicator from previous discovery state", () =>
920+ withOAuthFixture(
921+ () => notFound(),
922+ ({ baseUrl, requests }) =>
923+ Effect.gen(function* () {
924+ const exit = yield* Effect.exit(
925+ beginDynamicAuthorization({
926+ endpoint: `${baseUrl}/mcp`,
927+ redirectUrl: "https://app/cb",
928+ state: "s",
929+ previousState: {
930+ authorizationServerUrl: baseUrl,
931+ authorizationServerMetadataUrl: `${baseUrl}/.well-known/oauth-authorization-server`,
932+ authorizationServerMetadata: {
933+ issuer: baseUrl,
934+ authorization_endpoint: `${baseUrl}/authorize`,
935+ token_endpoint: `${baseUrl}/token`,
936+ },
937+ resourceMetadata: null,
938+ clientInformation: {
939+ client_id: "cached-client",
940+ },
941+ resource: baseUrl,
942+ },
943+ }),
944+ );
945+
946+ expect(Exit.isFailure(exit)).toBe(true);
947+ if (!Exit.isFailure(exit)) return;
948+ const reason = exit.cause.reasons.find(Cause.isFailReason);
949+ expect(reason?.error).toEqual(
950+ expect.objectContaining({
951+ _tag: "OAuthDiscoveryError",
952+ message: expect.stringMatching(/resource does not match requested endpoint/),
953+ }),
954+ );
955+ expect((yield* requests).length).toBe(0);
956+ }),
957+ ),
958+ );
959+
903960 it.effect("rejects servers that don't support PKCE S256", () =>
904961 withOAuthFixture(
905962 (request, baseUrl) => {
packages/core/sdk/src/oauth-discovery.ts+28−7
@@ -164,10 +164,26 @@ const validateEndpointUrl = ( · 163 unmodified lines
164164
165165 const validateAuthorizationServerMetadata = (
166166 metadata: OAuthAuthorizationServerMetadata,
167+ expectedIssuer: string,
167168 policy: OAuthEndpointUrlPolicy = {},
168169 ): Effect.Effect<void, OAuthDiscoveryError> =>
169170 Effect.gen(function* () {
170171 yield* validateEndpointUrl(metadata.issuer, "issuer", policy);
172+ const canonicalIssuer = (value: string): string => {
173+ const url = new URL(value);
174+ const path = url.pathname.replace(/\/+$/, "");
175+ return `${url.protocol.toLowerCase()}//${url.host.toLowerCase()}${path}`;
176+ };
177+ // RFC 8414 issuer identity compares scheme, host, port, and path after URL
178+ // normalization; a trailing slash alone does not create a different issuer.
179+ if (canonicalIssuer(metadata.issuer) !== canonicalIssuer(expectedIssuer)) {
180+ return yield* new OAuthDiscoveryError({
181+ message:
182+ `Authorization server metadata issuer ${metadata.issuer} does not match the requested ` +
183+ `issuer ${expectedIssuer}. Correct the server's issuer metadata or use the matching ` +
184+ `authorization server URL.`,
185+ });
186+ }
171187 yield* validateEndpointUrl(metadata.authorization_endpoint, "authorization_endpoint", policy);
172188 yield* validateEndpointUrl(metadata.token_endpoint, "token_endpoint", policy);
173189 if (metadata.registration_endpoint) {
@@ -301,6 +317,9 @@ export const discoverProtectedResourceMetadata = ( · 127 unmodified lines
301317 }),
302318 ),
303319 );
320+ if (metadata.resource) {
321+ yield* validateResourceIndicator(metadata.resource, canonicalResourceUrl(resourceUrl));
322+ }
304323 return { metadataUrl: url, metadata };
305324 }
306325 return null;
@@ -394,7 +413,7 @@ export const discoverAuthorizationServerMetadata = ( · 87 unmodified lines
394413 }),
395414 ),
396415 );
397− yield* validateAuthorizationServerMetadata(metadata, options.endpointUrlPolicy);
416+ yield* validateAuthorizationServerMetadata(metadata, issuer, options.endpointUrlPolicy);
398417 return { metadataUrl, metadata };
399418 }
400419 return null;
@@ -583,16 +602,15 @@ export const registerDynamicClient = ( · 182 unmodified lines
583602 //
584603 // MCP Authorization 2025-06-18 requires `resource` on /authorize and /token
585604 // requests. RFC 8707 §2 says the value is "an absolute URI" identifying the
586−// protected resource — same scheme + host + (optional) path, no fragment,
587−// no query, lowercased scheme/host.
605+// protected resource. Query components and path trailing slashes are part of
606+// the identifier; fragments are not sent, and scheme/host compare lowercase.
588607 // ---------------------------------------------------------------------------
589608
590609 export const canonicalResourceUrl = (value: string): string => {
591610 const url = new URL(value);
592611 const scheme = url.protocol.toLowerCase();
593612 const host = url.host.toLowerCase();
594− const path = url.pathname.replace(/\/+$/, "");
595− return `${scheme}//${host}${path}`;
613+ return `${scheme}//${host}${url.pathname}${url.search}`;
596614 };
597615
598616 const validateResourceIndicator = (
@@ -608,11 +626,14 @@ const validateResourceIndicator = ( · 9 unmodified lines
608626 }),
609627 }).pipe(
610628 Effect.flatMap((actual) =>
611− actual === expected || expected.startsWith(`${actual}/`)
629+ actual === expected
612630 ? Effect.succeed(actual)
613631 : Effect.fail(
614632 new OAuthDiscoveryError({
615− message: "Protected resource metadata resource does not match requested endpoint",
633+ message:
634+ `Protected resource metadata resource does not match requested endpoint: ` +
635+ `advertised ${actual}; requested ${expected}. Correct the server's resource ` +
636+ `metadata or use the matching resource URL.`,
616637 cause: { expected, actual },
617638 }),
618639 ),
packages/core/sdk/src/oauth-register-dynamic.test.ts+68−1
@@ -8,7 +8,7 @@ import { · 7 unmodified lines
88 OAuthClientSlug,
99 ToolName,
1010 } from "./ids";
11−import { OAuthRegisterDynamicError } from "./oauth-client";
11+import { OAuthProbeError, OAuthRegisterDynamicError } from "./oauth-client";
1212 import { definePlugin } from "./plugin";
1313 import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config";
1414 import { serveOAuthTestServer } from "./testing/oauth-test-server";
@@ -58,6 +58,73 @@ const oauthPlugin = definePlugin(() => ({ · 43 unmodified lines
5858 const plugins = [memoryCredentialsPlugin(), oauthPlugin] as const;
5959
6060 describe("oauth.registerDynamicClient", () => {
61+ it.effect("probe rejects authorization-server metadata with a mismatched issuer", () =>
62+ Effect.scoped(
63+ Effect.gen(function* () {
64+ const advertisedIssuer = "https://issuer.example/mismatched";
65+ const server = yield* serveOAuthTestServer({ advertisedIssuer });
66+ const { executor } = yield* makeTestWorkspaceHarness({ plugins });
67+
68+ const error = yield* Effect.flip(executor.oauth.probe({ url: server.mcpResourceUrl }));
69+ expect(error).toBeInstanceOf(OAuthProbeError);
70+ if (!Predicate.isTagged(error, "OAuthProbeError")) return;
71+ // oxlint-disable-next-line executor/no-unknown-error-message -- test: the tag guard narrows to the public OAuthProbeError contract
72+ const message = error.message;
73+ expect(message).toContain(server.issuerUrl);
74+ expect(message).toContain(advertisedIssuer);
75+ }),
76+ ),
77+ );
78+
79+ it.effect("probe rejects protected-resource metadata for a different resource", () =>
80+ Effect.scoped(
81+ Effect.gen(function* () {
82+ const advertisedResource = "https://resource.example/mismatched";
83+ const server = yield* serveOAuthTestServer({ advertisedResource });
84+ const { executor } = yield* makeTestWorkspaceHarness({ plugins });
85+
86+ const error = yield* Effect.flip(executor.oauth.probe({ url: server.mcpResourceUrl }));
87+ expect(error).toBeInstanceOf(OAuthProbeError);
88+ if (!Predicate.isTagged(error, "OAuthProbeError")) return;
89+ // oxlint-disable-next-line executor/no-unknown-error-message -- test: the tag guard narrows to the public OAuthProbeError contract
90+ const message = error.message;
91+ expect(message).toContain(server.mcpResourceUrl);
92+ expect(message).toContain(advertisedResource);
93+ }),
94+ ),
95+ );
96+
97+ it.effect("canonicalizes the RFC 8707 resource before persisting a DCR client", () =>
98+ Effect.scoped(
99+ Effect.gen(function* () {
100+ const server = yield* serveOAuthTestServer({ scopes: ["read"] });
101+ const { executor } = yield* makeTestWorkspaceHarness({ plugins });
102+ yield* executor.acme.seed();
103+ const rawResource = `${server.issuerUrl.replace("http://", "HTTP://")}/mcp/?tenant=Acme#fragment`;
104+ const expectedResource = `${server.issuerUrl}/mcp/?tenant=Acme`;
105+
106+ const slug = yield* executor.oauth.registerDynamicClient({
107+ owner: "org",
108+ slug: CLIENT,
109+ issuer: server.issuerUrl,
110+ registrationEndpoint: server.registrationEndpoint,
111+ authorizationUrl: server.authorizationEndpoint,
112+ tokenUrl: server.tokenEndpoint,
113+ resource: rawResource,
114+ scopes: ["read"],
115+ tokenEndpointAuthMethodsSupported: ["none"],
116+ clientName: "Executor",
117+ redirectUri: FLOW_REDIRECT_URI,
118+ originIntegration: INTEG,
119+ });
120+
121+ const clients = yield* executor.oauth.listClients();
122+ const minted = clients.find((client) => String(client.slug) === String(slug));
123+ expect(minted?.resource).toBe(expectedResource);
124+ }),
125+ ),
126+ );
127+
61128 it.effect("DCR mints + persists a public (no-secret) client that lists + connects", () =>
62129 Effect.scoped(
63130 Effect.gen(function* () {
packages/core/sdk/src/oauth-service.ts+22−4
@@ -50,6 +50,7 @@ import { · 49 unmodified lines
5050 import type { OwnerBinding } from "./plugin";
5151 import type { CredentialProvider } from "./provider";
5252 import {
53+ canonicalResourceUrl,
5354 discoverAuthorizationServerMetadata,
5455 discoverProtectedResourceMetadata,
5556 OAuthDiscoveryError,
@@ -792,6 +793,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 736 unmodified lines
792793 input: RegisterDynamicClientInput,
793794 issuer: string | null,
794795 flowRedirectUri: string | null,
796+ resource: string | null,
795797 ): Effect.Effect<
796798 {
797799 readonly existingSlug: OAuthClientSlug | null;
@@ -801,7 +803,6 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 3 unmodified lines
801803 > =>
802804 Effect.gen(function* () {
803805 const candidates = yield* dcrCandidatesForIssuer(input.owner, issuer);
804− const resource = input.resource ?? null;
805806 // A candidate is reusable only when the callback it registered with the
806807 // AS still matches the current flow's callback — strict servers reject an
807808 // authorize request whose redirect_uri differs from the registration
@@ -859,11 +860,22 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 51 unmodified lines
859860 ): Effect.Effect<OAuthClientSlug, OAuthRegisterDynamicError | StorageFailure> =>
860861 Effect.gen(function* () {
861862 const issuer = canonicalDcrIssuer(input.issuer, input.registrationEndpoint);
863+ const requestedResource = input.resource ?? null;
864+ const resource =
865+ requestedResource === null
866+ ? null
867+ : yield* Effect.try({
868+ try: () => canonicalResourceUrl(requestedResource),
869+ catch: () =>
870+ new OAuthRegisterDynamicError({
871+ message: `OAuth resource must be an absolute URL: ${requestedResource}`,
872+ }),
873+ });
862874 // Resolved before the reuse decision: a persisted client registered with
863875 // a DIFFERENT callback must not be reused (strict servers 400 the
864876 // authorize request), so the reuse lookup compares against this value.
865877 const flowRedirectUri = input.redirectUri ?? redirectUri ?? null;
866− const reuse = yield* decideDcrClientReuse(input, issuer, flowRedirectUri);
878+ const reuse = yield* decideDcrClientReuse(input, issuer, flowRedirectUri, resource);
867879 if (reuse.existingSlug !== null) return reuse.existingSlug;
868880
869881 const slug = reuse.registrationSlug;
@@ -920,7 +932,7 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 50 unmodified lines
920932 slug,
921933 authorizationUrl: input.authorizationUrl,
922934 tokenUrl: input.tokenUrl,
923− resource: input.resource ?? null,
935+ resource,
924936 grant: "authorization_code",
925937 clientId: information.client_id,
926938 clientSecret: information.client_secret ?? "",
@@ -1427,7 +1439,13 @@ export const makeOAuthService = (deps: OAuthServiceDeps): OAuthService => { · 500 unmodified lines
14271439 const options = { endpointUrlPolicy: deps.endpointUrlPolicy };
14281440 // Try protected-resource metadata first (RFC 9728), then the AS issuer.
14291441 const resource = yield* discoverProtectedResourceMetadata(input.url, options).pipe(
1430− Effect.catch(() => Effect.succeed(null)),
1442+ Effect.mapError(
1443+ (cause) =>
1444+ new OAuthProbeError({
1445+ // oxlint-disable-next-line executor/no-unknown-error-message -- boundary: OAuthDiscoveryError carries a typed `message` field
1446+ message: `OAuth discovery failed: ${cause.message}`,
1447+ }),
1448+ ),
14311449 );
14321450 // EXPLICIT discovery order: when the protected-resource metadata advertises
14331451 // an authorization server, probe that; otherwise probe the input endpoint
packages/core/sdk/src/testing/oauth-test-server.ts+6−2
@@ -61,6 +61,10 @@ export interface OAuthTestServerOptions { · 60 unmodified lines
6161 readonly invalidRefreshTokenErrorCode?: string;
6262 readonly idTokenClaims?: Readonly<Record<string, unknown>>;
6363 readonly refreshIdTokenClaims?: Readonly<Record<string, unknown>>;
64+ /** Override the RFC 8414 `issuer` advertised by authorization-server metadata. */
65+ readonly advertisedIssuer?: string;
66+ /** Override the RFC 9728 `resource` advertised by protected-resource metadata. */
67+ readonly advertisedResource?: string;
6468 /** Gate Dynamic Client Registration on the requested redirect URIs. When set,
6569 * `/register` returns `400 invalid_redirect_uri` unless every requested
6670 * `redirect_uris` entry is approved. Mirrors authorization servers (e.g.
@@ -487,7 +491,7 @@ export const serveOAuthTestServer = ( · 420 unmodified lines
487491
488492 if (requestUrl.pathname.startsWith("/.well-known/oauth-protected-resource")) {
489493 const suffix = requestUrl.pathname.slice("/.well-known/oauth-protected-resource".length);
490− const resource = `${currentIssuerUrl}${suffix}`;
494+ const resource = options.advertisedResource ?? `${currentIssuerUrl}${suffix}`;
491495 return jsonResponse(200, {
492496 resource,
493497 authorization_servers: [currentIssuerUrl],
@@ -501,7 +505,7 @@ export const serveOAuthTestServer = ( · 7 unmodified lines
501505 requestUrl.pathname === "/.well-known/openid-configuration"
502506 ) {
503507 return jsonResponse(200, {
504− issuer: currentIssuerUrl,
508+ issuer: options.advertisedIssuer ?? currentIssuerUrl,
505509 authorization_endpoint: `${currentIssuerUrl}/authorize`,
506510 token_endpoint: `${currentIssuerUrl}/token`,
507511 registration_endpoint: `${currentIssuerUrl}/register`,

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t