Skip to content

Commit dd67164

Browse files
authored
Merge pull request #2136 from modelcontextprotocol/v2/fix/2110-oauth-path-scoped-discovery
fix(oauth): keep the server's path in the fallback authorization-server URL (#2110)
2 parents c06dc31 + 02cba73 commit dd67164

6 files changed

Lines changed: 437 additions & 27 deletions

File tree

‎AGENTS.md‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,34 @@ v2/main/
7373
│ │ # the advertised document rather than a
7474
│ │ # location derived from the MCP server
7575
│ │ # URL — #2071;
76+
│ │ # discovery.ts the authorization-server
77+
│ │ # URL POLICY — when protected-resource
78+
│ │ # metadata names no authorization
79+
│ │ # server, the MCP server URL stands in,
80+
│ │ # and its PATH is load-bearing: the
81+
│ │ # SDK's buildDiscoveryUrls derives the
82+
│ │ # path-scoped well-known locations from
83+
│ │ # that pathname, so reducing it to the
84+
│ │ # origin can only ever probe the domain
85+
│ │ # root. getAuthorizationServerUrlCandidates
86+
│ │ # therefore yields the path-scoped URL
87+
│ │ # FIRST and the bare origin second, and
88+
│ │ # discoverAuthorizationServerMetadataForServer
89+
│ │ # walks them — the second candidate is
90+
│ │ # what keeps a server that merely lives
91+
│ │ # under a path while publishing metadata
92+
│ │ # at the root working, so do not drop it
93+
│ │ # in favour of a straight swap. EVERY
94+
│ │ # consumer of the fallback must walk:
95+
│ │ # discoverScopes, the CIMD probe, and
96+
│ │ # the CLI's refreshStoredAuthToken,
97+
│ │ # which is why the walk is also exposed
98+
│ │ # as ...FromCandidates(candidates,
99+
│ │ # discover) — the CLI injects its own
100+
│ │ # discovery function as a test seam and
101+
│ │ # needs to know WHICH candidate
102+
│ │ # answered, since that is the base its
103+
│ │ # token request is made against — #2110;
76104
│ │ # scopes.ts SEP-2350 scope union, oauthUx.ts
77105
│ │ # shared copy, mcpAuth.ts force-reauthorization,
78106
│ │ # issuerBinding.ts SEP-2352 callback-leg failure

‎clients/cli/__tests__/stored-auth.test.ts‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,97 @@ describe("refreshStoredAuthToken", () => {
158158
}
159159
});
160160

161+
// #2110: with no stored metadata the MCP server URL stands in as the
162+
// authorization server. A path-hosted server has two plausible answers, and
163+
// both must keep working — the path-scoped URL for a server that publishes
164+
// its metadata under its own path, the bare origin for one that merely lives
165+
// under a path. The candidate that answers is also the base the token request
166+
// is made against, so the walk has to report *which* one it was.
167+
it("prefers the path-scoped authorization server for a path-hosted MCP server", async () => {
168+
const path = writeOAuthFixture({
169+
[SERVER]: {
170+
tokens: { refresh_token: "old-refresh", token_type: "Bearer" },
171+
clientInformation: { client_id: "cid" },
172+
},
173+
});
174+
try {
175+
const refresh = vi.fn().mockResolvedValue(freshTokens);
176+
const pathMetadata = {
177+
issuer: "https://api.example/mcp",
178+
token_endpoint: "https://api.example/mcp/token",
179+
};
180+
const discover = vi.fn().mockResolvedValue(pathMetadata);
181+
182+
await refreshStoredAuthToken(SERVER, path, { refresh, discover });
183+
184+
expect(discover).toHaveBeenCalledTimes(1);
185+
expect(discover).toHaveBeenCalledWith(new URL("https://api.example/mcp"));
186+
const [authServerUrl, opts] = refresh.mock.calls[0]!;
187+
expect(authServerUrl).toEqual(new URL("https://api.example/mcp"));
188+
expect(opts.metadata).toEqual(pathMetadata);
189+
} finally {
190+
rmSync(path, { force: true });
191+
}
192+
});
193+
194+
it("falls back to the origin when a path-hosted server publishes metadata at the root", async () => {
195+
const path = writeOAuthFixture({
196+
[SERVER]: {
197+
tokens: { refresh_token: "old-refresh", token_type: "Bearer" },
198+
clientInformation: { client_id: "cid" },
199+
},
200+
});
201+
try {
202+
const refresh = vi.fn().mockResolvedValue(freshTokens);
203+
const rootMetadata = {
204+
issuer: "https://api.example",
205+
token_endpoint: "https://api.example/token",
206+
};
207+
const discover = vi
208+
.fn()
209+
.mockResolvedValueOnce(undefined)
210+
.mockResolvedValueOnce(rootMetadata);
211+
212+
const token = await refreshStoredAuthToken(SERVER, path, {
213+
refresh,
214+
discover,
215+
});
216+
217+
expect(token).toBe("refreshed-access-token");
218+
expect(discover).toHaveBeenNthCalledWith(
219+
2,
220+
new URL("https://api.example/"),
221+
);
222+
// The candidate that answered is the one the token request targets.
223+
const [authServerUrl, opts] = refresh.mock.calls[0]!;
224+
expect(authServerUrl).toEqual(new URL("https://api.example/"));
225+
expect(opts.metadata).toEqual(rootMetadata);
226+
} finally {
227+
rmSync(path, { force: true });
228+
}
229+
});
230+
231+
it("keeps the path-scoped candidate as the token-request base when no candidate answers", async () => {
232+
const path = writeOAuthFixture({
233+
[SERVER]: {
234+
tokens: { refresh_token: "old-refresh", token_type: "Bearer" },
235+
clientInformation: { client_id: "cid" },
236+
},
237+
});
238+
try {
239+
const refresh = vi.fn().mockResolvedValue(freshTokens);
240+
const discover = vi.fn().mockResolvedValue(undefined);
241+
242+
await refreshStoredAuthToken(SERVER, path, { refresh, discover });
243+
244+
expect(discover).toHaveBeenCalledTimes(2);
245+
const [authServerUrl] = refresh.mock.calls[0]!;
246+
expect(authServerUrl).toEqual(new URL("https://api.example/mcp"));
247+
} finally {
248+
rmSync(path, { force: true });
249+
}
250+
});
251+
161252
it("uses the distinct no_client_information code when client info is missing", async () => {
162253
const path = writeOAuthFixture({
163254
[SERVER]: {

‎clients/cli/src/cli.ts‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,11 @@ import {
4747
serializeOAuthPersistBlob,
4848
type OAuthPersistSnapshot,
4949
} from "@inspector/core/auth/oauth-persist.js";
50-
import { getAuthorizationServerUrl } from "@inspector/core/auth/discovery.js";
50+
import {
51+
discoverAuthorizationServerMetadataFromCandidates,
52+
getAuthorizationServerUrl,
53+
getAuthorizationServerUrlCandidates,
54+
} from "@inspector/core/auth/discovery.js";
5155
import { writeStoreFile } from "@inspector/core/storage/store-io.js";
5256
import {
5357
refreshAuthorization,
@@ -347,11 +351,27 @@ export async function refreshStoredAuthToken(
347351
);
348352
}
349353

350-
const authServerUrl = found.state.serverMetadata?.issuer
351-
? new URL(found.state.serverMetadata.issuer)
352-
: getAuthorizationServerUrl(serverUrl);
353-
const metadata =
354-
found.state.serverMetadata ?? (await discover(authServerUrl)) ?? undefined;
354+
// With stored metadata the issuer settles it. Without, the MCP server URL
355+
// stands in as the authorization server — and a path-hosted server has two
356+
// plausible answers, so walk them rather than committing to the path-scoped
357+
// one: a server that merely lives under a path while publishing its metadata
358+
// at the domain root must keep working (#2110). The candidate that *answered*
359+
// becomes `authServerUrl`, since it is also the base the token request below
360+
// is made against.
361+
let authServerUrl: URL;
362+
let metadata = found.state.serverMetadata ?? undefined;
363+
if (found.state.serverMetadata?.issuer) {
364+
authServerUrl = new URL(found.state.serverMetadata.issuer);
365+
} else {
366+
const discovered = await discoverAuthorizationServerMetadataFromCandidates(
367+
getAuthorizationServerUrlCandidates(serverUrl),
368+
discover,
369+
);
370+
authServerUrl =
371+
discovered?.authorizationServerUrl ??
372+
getAuthorizationServerUrl(serverUrl);
373+
metadata = discovered?.metadata;
374+
}
355375

356376
let tokens: OAuthTokens;
357377
try {

‎clients/web/src/test/integration/auth/discovery.test.ts‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import { describe, it, expect, vi, beforeEach } from "vitest";
22
import {
3+
discoverAuthorizationServerMetadataForServer,
34
discoverScopes,
45
getAuthorizationServerUrl,
6+
getAuthorizationServerUrlCandidates,
57
} from "@inspector/core/auth/discovery.js";
68
import type { OAuthProtectedResourceMetadata } from "@modelcontextprotocol/client";
79

@@ -335,3 +337,172 @@ describe("getAuthorizationServerUrl", () => {
335337
expect(captured?.message).toMatch(/Invalid MCP server URL: "" \(.+\)/);
336338
});
337339
});
340+
341+
// #2110: an MCP server hosted under a path (Moodle, a WordPress plugin, any
342+
// PHP endpoint) publishes its OAuth metadata under that same path. Reducing the
343+
// fallback authorization-server URL to the origin threw that path away, so
344+
// discovery could only ever probe the domain root and 404'd.
345+
describe("path-hosted MCP servers (#2110)", () => {
346+
const pathServerUrl = "https://misc.example.com/mod/minilesson/mcp.php";
347+
348+
describe("getAuthorizationServerUrl", () => {
349+
it("keeps the server's path in the fallback URL", () => {
350+
expect(getAuthorizationServerUrl(pathServerUrl)).toEqual(
351+
new URL(pathServerUrl),
352+
);
353+
});
354+
355+
it("strips query and fragment, which an RFC 8414 issuer never carries", () => {
356+
expect(
357+
getAuthorizationServerUrl(`${pathServerUrl}?session=abc#frag`),
358+
).toEqual(new URL(pathServerUrl));
359+
});
360+
});
361+
362+
describe("getAuthorizationServerUrlCandidates", () => {
363+
it("tries the path-scoped URL before the bare origin", () => {
364+
expect(getAuthorizationServerUrlCandidates(pathServerUrl)).toEqual([
365+
new URL(pathServerUrl),
366+
new URL("https://misc.example.com/"),
367+
]);
368+
});
369+
370+
it("offers a single candidate for a root-hosted server", () => {
371+
expect(
372+
getAuthorizationServerUrlCandidates("https://mcp.example.com"),
373+
).toEqual([new URL("https://mcp.example.com/")]);
374+
});
375+
376+
it("does not probe past an authorization server named by resource metadata", () => {
377+
const resourceMetadata: OAuthProtectedResourceMetadata = {
378+
resource: pathServerUrl,
379+
authorization_servers: ["https://auth.example.com/realms/r"],
380+
};
381+
expect(
382+
getAuthorizationServerUrlCandidates(pathServerUrl, resourceMetadata),
383+
).toEqual([new URL("https://auth.example.com/realms/r")]);
384+
});
385+
});
386+
387+
describe("discoverAuthorizationServerMetadataForServer", () => {
388+
const metadata = {
389+
issuer: pathServerUrl,
390+
authorization_endpoint: "https://misc.example.com/mod/authorize.php",
391+
token_endpoint: "https://misc.example.com/mod/token.php",
392+
response_types_supported: ["code"],
393+
};
394+
395+
beforeEach(() => {
396+
vi.clearAllMocks();
397+
});
398+
399+
it("returns the path-scoped result without probing the origin", async () => {
400+
const { discoverAuthorizationServerMetadata } =
401+
await import("@modelcontextprotocol/client");
402+
vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue(
403+
metadata,
404+
);
405+
406+
await expect(
407+
discoverAuthorizationServerMetadataForServer(pathServerUrl),
408+
).resolves.toEqual(metadata);
409+
expect(discoverAuthorizationServerMetadata).toHaveBeenCalledTimes(1);
410+
expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith(
411+
new URL(pathServerUrl),
412+
{ fetchFn: undefined },
413+
);
414+
});
415+
416+
it("falls back to the origin when the path-scoped candidate has no metadata", async () => {
417+
const { discoverAuthorizationServerMetadata } =
418+
await import("@modelcontextprotocol/client");
419+
vi.mocked(discoverAuthorizationServerMetadata)
420+
.mockResolvedValueOnce(undefined)
421+
.mockResolvedValueOnce(metadata);
422+
423+
await expect(
424+
discoverAuthorizationServerMetadataForServer(pathServerUrl),
425+
).resolves.toEqual(metadata);
426+
expect(discoverAuthorizationServerMetadata).toHaveBeenNthCalledWith(
427+
2,
428+
new URL("https://misc.example.com/"),
429+
{ fetchFn: undefined },
430+
);
431+
});
432+
433+
it("survives a throwing candidate while another remains", async () => {
434+
const { discoverAuthorizationServerMetadata } =
435+
await import("@modelcontextprotocol/client");
436+
vi.mocked(discoverAuthorizationServerMetadata)
437+
.mockRejectedValueOnce(new Error("issuer mismatch"))
438+
.mockResolvedValueOnce(metadata);
439+
440+
await expect(
441+
discoverAuthorizationServerMetadataForServer(pathServerUrl),
442+
).resolves.toEqual(metadata);
443+
});
444+
445+
it("rethrows the first error when every candidate fails", async () => {
446+
const { discoverAuthorizationServerMetadata } =
447+
await import("@modelcontextprotocol/client");
448+
vi.mocked(discoverAuthorizationServerMetadata)
449+
.mockRejectedValueOnce(new Error("path-scoped failed"))
450+
.mockRejectedValueOnce(new Error("origin failed"));
451+
452+
await expect(
453+
discoverAuthorizationServerMetadataForServer(pathServerUrl),
454+
).rejects.toThrow("path-scoped failed");
455+
});
456+
457+
it("resolves undefined when every candidate answers with no metadata", async () => {
458+
const { discoverAuthorizationServerMetadata } =
459+
await import("@modelcontextprotocol/client");
460+
vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue(
461+
undefined,
462+
);
463+
464+
await expect(
465+
discoverAuthorizationServerMetadataForServer(pathServerUrl),
466+
).resolves.toBeUndefined();
467+
expect(discoverAuthorizationServerMetadata).toHaveBeenCalledTimes(2);
468+
});
469+
470+
it("passes the caller's fetch through to every candidate", async () => {
471+
const { discoverAuthorizationServerMetadata } =
472+
await import("@modelcontextprotocol/client");
473+
vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue(
474+
undefined,
475+
);
476+
const fetchFn = vi.fn<typeof fetch>();
477+
478+
await discoverAuthorizationServerMetadataForServer(
479+
pathServerUrl,
480+
undefined,
481+
fetchFn,
482+
);
483+
484+
expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith(
485+
new URL(pathServerUrl),
486+
{ fetchFn },
487+
);
488+
});
489+
});
490+
491+
it("discoverScopes reaches a path-hosted authorization server", async () => {
492+
const { discoverAuthorizationServerMetadata } =
493+
await import("@modelcontextprotocol/client");
494+
vi.mocked(discoverAuthorizationServerMetadata).mockResolvedValue({
495+
issuer: pathServerUrl,
496+
authorization_endpoint: "https://misc.example.com/mod/authorize.php",
497+
token_endpoint: "https://misc.example.com/mod/token.php",
498+
response_types_supported: ["code"],
499+
scopes_supported: ["read"],
500+
});
501+
502+
await expect(discoverScopes(pathServerUrl)).resolves.toBe("read");
503+
expect(discoverAuthorizationServerMetadata).toHaveBeenCalledWith(
504+
new URL(pathServerUrl),
505+
{ fetchFn: undefined },
506+
);
507+
});
508+
});

‎core/auth/cimd.ts‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,6 @@
1-
import {
2-
discoverAuthorizationServerMetadata,
3-
discoverOAuthProtectedResourceMetadata,
4-
} from "@modelcontextprotocol/client";
1+
import { discoverOAuthProtectedResourceMetadata } from "@modelcontextprotocol/client";
52
import type { OAuthClientInformation } from "@modelcontextprotocol/client";
6-
import { getAuthorizationServerUrl } from "./discovery.js";
3+
import { discoverAuthorizationServerMetadataForServer } from "./discovery.js";
74
import type { BaseOAuthClientProvider } from "./providers.js";
85

96
/**
@@ -49,14 +46,14 @@ export async function ensureCimdClientRegistration(params: {
4946
resourceMetadata = undefined;
5047
}
5148

52-
const authServerUrl = getAuthorizationServerUrl(
49+
// Walks the path-scoped authorization-server URL before the bare origin, so a
50+
// server hosted under a path is probed where it actually publishes its
51+
// metadata rather than only at the domain root (#2110).
52+
const metadata = await discoverAuthorizationServerMetadataForServer(
5353
params.serverUrl,
5454
resourceMetadata,
55+
params.fetchFn,
5556
);
56-
57-
const metadata = await discoverAuthorizationServerMetadata(authServerUrl, {
58-
...(params.fetchFn && { fetchFn: params.fetchFn }),
59-
});
6057
if (!metadata?.client_id_metadata_document_supported) return;
6158

6259
const clientInformation: OAuthClientInformation = {

0 commit comments

Comments
 (0)