modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

perf(auth): gate the override wrapper on the discovery request, and guard disk values

Browse source 
Two suppressed review findings.

With an override configured, the wrapper cloned and parsed every
successful application/json response — every tools/call result and every
resource payload on the direct CLI/TUI transports — only to conclude it
was not a metadata document. It now tests the request URL for the RFC
8414 / OIDC well-known paths first, so ordinary traffic costs one string
test and is returned untouched.

The read path also trusted StoredMCPServer's `string` type for the two
override fields, which a hand-edited mcp.json does not honor: the CLI and
TUI read the file directly and then `.trim()` these, so a number would
crash the server load. Guarded with cleanEndpointOverride, mirroring
cleanAuthorizationParams.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-16 22:11:16 -04:00
commit 8d6803953c
4 changed files with 162 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

64
clients/web/src/test/core/auth/endpointOverrides.test.ts
View file

@ -14,6 +14,8 @@ const METADATA = {
response_types_supported: ["code"],
};
const AS_METADATA_URL =
"https://as.example.com/.well-known/oauth-authorization-server";
const STAGING_AUTHORIZE = "https://staging.example.com/authorize";
const STAGING_TOKEN = "https://staging.example.com/token";
@ -190,7 +192,7 @@ describe("withOAuthEndpointOverrides", () => {
tokenUrl: STAGING_TOKEN,
}));
const response = await wrapped("https://as.example.com/.well-known/x");
const response = await wrapped(AS_METADATA_URL);
await expect(response.json()).resolves.toEqual({
...METADATA,
authorization_endpoint: STAGING_AUTHORIZE,
@ -206,18 +208,52 @@ describe("withOAuthEndpointOverrides", () => {
const config: { overrides?: { tokenUrl?: string } } = {};
const wrapped = withOAuthEndpointOverrides(base, () => config.overrides);
const before = await wrapped("https://as.example.com/.well-known/x");
const before = await wrapped(AS_METADATA_URL);
await expect(before.json()).resolves.toMatchObject({
token_endpoint: METADATA.token_endpoint,
});
config.overrides = { tokenUrl: STAGING_TOKEN };
const after = await wrapped("https://as.example.com/.well-known/x");
const after = await wrapped(AS_METADATA_URL);
await expect(after.json()).resolves.toMatchObject({
token_endpoint: STAGING_TOKEN,
});
});
// The gate that keeps this wrapper off the hot path: an ordinary JSON-RPC
// response is not even cloned, let alone parsed. Regression test for PR #2037.
it("ignores a response to a request that is not metadata discovery", async () => {
const original = jsonResponse(METADATA);
const clone = vi.spyOn(original, "clone");
const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({
tokenUrl: STAGING_TOKEN,
}));
await expect(wrapped("https://mcp.example.com/mcp")).resolves.toBe(
original,
);
expect(clone).not.toHaveBeenCalled();
});
it("recognizes discovery through every request-input form", async () => {
const wrapped = withOAuthEndpointOverrides(
// A fresh response per call: each patched one consumes its own clone.
async () => jsonResponse(METADATA),
() => ({ tokenUrl: STAGING_TOKEN }),
);
const oidcUrl = "https://as.example.com/.well-known/openid-configuration";
for (const input of [
AS_METADATA_URL,
new URL(oidcUrl),
new Request(`${AS_METADATA_URL}/tenant-a`),
]) {
const response = await wrapped(input);
await expect(response.json()).resolves.toMatchObject({
token_endpoint: STAGING_TOKEN,
});
}
});
it("warns once about a malformed override, not on every request", async () => {
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
const base: typeof fetch = async () => jsonResponse(METADATA);
@ -225,9 +261,9 @@ describe("withOAuthEndpointOverrides", () => {
tokenUrl: "not a url",
}));
await wrapped("https://as.example.com/x");
await wrapped("https://as.example.com/x");
await wrapped("https://as.example.com/x");
await wrapped(AS_METADATA_URL);
await wrapped(AS_METADATA_URL);
await wrapped(AS_METADATA_URL);
expect(warn).toHaveBeenCalledTimes(1);
});
@ -238,7 +274,7 @@ describe("withOAuthEndpointOverrides", () => {
passThrough(original),
() => undefined,
);
await expect(wrapped("https://as.example.com/x")).resolves.toBe(original);
await expect(wrapped(AS_METADATA_URL)).resolves.toBe(original);
});
it("leaves an error response alone", async () => {
@ -246,7 +282,7 @@ describe("withOAuthEndpointOverrides", () => {
const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({
tokenUrl: STAGING_TOKEN,
}));
await expect(wrapped("https://as.example.com/x")).resolves.toBe(original);
await expect(wrapped(AS_METADATA_URL)).resolves.toBe(original);
});
it("leaves a non-JSON response alone", async () => {
@ -256,7 +292,7 @@ describe("withOAuthEndpointOverrides", () => {
const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({
tokenUrl: STAGING_TOKEN,
}));
await expect(wrapped("https://as.example.com/x")).resolves.toBe(original);
await expect(wrapped(AS_METADATA_URL)).resolves.toBe(original);
});
// Everything that is not a metadata document comes back as the caller's own
@ -267,7 +303,7 @@ describe("withOAuthEndpointOverrides", () => {
const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({
tokenUrl: STAGING_TOKEN,
}));
const response = await wrapped("https://as.example.com/token");
const response = await wrapped(AS_METADATA_URL);
expect(response).toBe(original);
await expect(response.json()).resolves.toEqual({ access_token: "abc" });
});
@ -277,7 +313,7 @@ describe("withOAuthEndpointOverrides", () => {
const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({
tokenUrl: STAGING_TOKEN,
}));
const response = await wrapped("https://as.example.com/x");
const response = await wrapped(AS_METADATA_URL);
expect(response).toBe(original);
await expect(response.text()).resolves.toBe("not json at all");
});
@ -316,7 +352,7 @@ describe("withOAuthEndpointOverrides", () => {
),
() => ({ tokenUrl: STAGING_TOKEN }),
);
const response = await wrapped("https://as.example.com/x");
const response = await wrapped(AS_METADATA_URL);
await expect(response.json()).resolves.toMatchObject({
token_endpoint: STAGING_TOKEN,
});
@ -333,7 +369,7 @@ describe("withOAuthEndpointOverrides", () => {
const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({
tokenUrl: STAGING_TOKEN,
}));
await expect(wrapped("https://as.example.com/x")).resolves.toBe(original);
await expect(wrapped(AS_METADATA_URL)).resolves.toBe(original);
});
it("drops the stale content-length and content-encoding of a rewritten body", async () => {
@ -350,7 +386,7 @@ describe("withOAuthEndpointOverrides", () => {
),
() => ({ tokenUrl: STAGING_TOKEN }),
);
const response = await wrapped("https://as.example.com/x");
const response = await wrapped(AS_METADATA_URL);
expect(response.headers.get("content-length")).toBeNull();
expect(response.headers.get("content-encoding")).toBeNull();
expect(response.headers.get("content-type")).toBe("application/json");

23
clients/web/src/test/core/mcp/serverList.test.ts
View file

@ -1304,6 +1304,29 @@ describe("oauthOnInsufficientScope (SEP-2350)", () => {
expect(settings?.oauthTokenUrl).toBe("https://staging.example.com/token");
});
// Hand-edited `mcp.json` reaches the CLI/TUI unvalidated, and
// `oauthEndpointOverridesFromSettings` calls `.trim()` on these — so a
// non-string would crash the server load rather than being ignored.
it("drops a non-string endpoint override from disk", () => {
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
const settings = storedFieldsToInspectorSettings({
oauth: {
clientId: "cid",
// Types the field as a string; disk does not honor that.
authorizationUrl: 42 as unknown as string,
tokenUrl: "https://staging.example.com/token",
},
});
expect(settings?.oauthAuthorizationUrl).toBeUndefined();
expect(settings?.oauthTokenUrl).toBe("https://staging.example.com/token");
expect(warn).toHaveBeenCalledWith(
expect.stringContaining("oauth.authorizationUrl"),
);
// Restored here rather than left to a global teardown: a later describe
// spies on `console.warn` again and would inherit this spy's recorded call.
warn.mockRestore();
});
it("reads a blank endpoint override back as unset", () => {
const settings = storedFieldsToInspectorSettings({
oauth: { clientId: "cid", authorizationUrl: "", tokenUrl: "" },

48
core/auth/endpointOverrides.ts
View file

@ -196,6 +196,51 @@ export function applyOAuthEndpointOverrides(
return patched;
}
/**
* The well-known paths authorization-server metadata is discovered at: RFC 8414
* (§3.1, including the path-suffixed and path-prefixed variants the SDK's
* `buildDiscoveryUrls` emits) and OpenID Connect Discovery. Matched as a
* substring of the pathname so every variant is covered by two literals.
*/
const AS_METADATA_WELL_KNOWN_PATHS = [
"/.well-known/oauth-authorization-server",
"/.well-known/openid-configuration",
] as const;
/** The request URL a `fetch` call was made with, in any of its three forms. */
function requestUrlOf(input: RequestInfo | URL): string {
if (typeof input === "string") return input;
if (input instanceof URL) return input.href;
return input.url;
}
/**
* Whether a request is an authorization-server metadata discovery request.
*
* This is what keeps the wrapper off the hot path. Without it, every successful
* `application/json` response on the connection — every `tools/call` result, and
* every resource payload on the direct CLI/TUI transports — would be buffered
* and parsed a second time before the SDK could consume it, purely to discover
* that it is not a metadata document. Gating on the discovery URL first means
* ordinary traffic is untouched: one string test and the response is returned.
*
* A URL that will not parse falls back to testing the raw string, which is the
* conservative direction — worst case a request is *considered*, and the
* metadata predicate then rejects it.
*/
function isAuthorizationServerMetadataRequest(
input: RequestInfo | URL,
): boolean {
const raw = requestUrlOf(input);
let pathname: string;
try {
pathname = new URL(raw).pathname;
} catch {
pathname = raw;
}
return AS_METADATA_WELL_KNOWN_PATHS.some((path) => pathname.includes(path));
}
/**
* Whether a `content-type` names a **whole JSON document** — something that can
* be read to completion.
@ -268,6 +313,9 @@ export function withOAuthEndpointOverrides(
const response = await fetchFn(input, init);
const overrides = resolveOverrides();
if (!overrides) return response;
// Cheapest tests first: only a discovery request can carry the document
// this wrapper rewrites, so ordinary traffic never reaches the clone below.
if (!isAuthorizationServerMetadataRequest(input)) return response;
if (!response.ok) return response;
if (!isJsonDocumentResponse(response.headers.get("content-type")))
return response;

45
core/mcp/serverList.ts
View file

@ -248,6 +248,33 @@ export function oauthAuthorizationParamsFromSettings(
return Object.keys(record).length > 0 ? record : undefined;
}
/**
* Validate one of a server's `oauth` endpoint overrides as it comes off disk,
* or `undefined` when there is nothing usable. (#1906)
*
* Same reasoning as `cleanAuthorizationParams` above: `StoredMCPServer` types
* these as `string`, but that is a compile-time promise a hand-edited file does
* not keep, and the CLI/TUI read `mcp.json` directly. A non-string reaches
* `oauthEndpointOverridesFromSettings`, which calls `.trim()` on it — so without
* this the whole server list fails to load. URL *validity* is not checked here;
* that happens where the override is applied, so a typo drops one field with a
* warning instead of anything louder.
*/
export function cleanEndpointOverride(
value: string | undefined,
field: "authorizationUrl" | "tokenUrl",
): string | undefined {
if (value === undefined) return undefined;
// Keep: unreachable per the parameter type, reachable from disk.
if (typeof value !== "string") {
console.warn(
`Ignoring \`oauth.${field}\`: expected a string, got ${typeof value}.`,
);
return undefined;
}
return value.trim() || undefined;
}
/**
* Collapse a server's endpoint-override fields into the
* `InspectorClientOptions.oauth` sub-shape, or `undefined` when neither is set.
@ -381,10 +408,20 @@ export function storedFieldsToInspectorSettings(
settings.oauthAuthorizationParams = envRecordToPairs(authorizationParams);
}
// Truthiness drops empty strings like the credential fields above, so a
// cleared field reads back as unset rather than as an empty override. (#1906)
if (stored.oauth?.authorizationUrl)
settings.oauthAuthorizationUrl = stored.oauth.authorizationUrl;
if (stored.oauth?.tokenUrl) settings.oauthTokenUrl = stored.oauth.tokenUrl;
// cleared field reads back as unset rather than as an empty override. The
// `string` type is a compile-time promise a hand-edited `mcp.json` does not
// keep, and only the web client's `/api/servers` route checks it — the CLI and
// TUI read the file directly and then call `.trim()` on these in
// `oauthEndpointOverridesFromSettings`, so a non-string would crash the server
// load. Sanitized like `authorizationParams` above rather than trusted.
// (#1906)
const authorizationUrl = cleanEndpointOverride(
stored.oauth?.authorizationUrl,
"authorizationUrl",
);
if (authorizationUrl) settings.oauthAuthorizationUrl = authorizationUrl;
const tokenUrl = cleanEndpointOverride(stored.oauth?.tokenUrl, "tokenUrl");
if (tokenUrl) settings.oauthTokenUrl = tokenUrl;
if (stored.oauth?.onInsufficientScope) {
settings.oauthOnInsufficientScope = stored.oauth.onInsufficientScope;
}