fix(auth): never read a stream, and say what the issuer override does not do
Two review findings on the fetch wrapper: The content-type test was a substring match for "json", which also matches `application/x-ndjson` — how the streamable-HTTP transport can serve its long-lived server-push channel (`isLongLivedStreamResponse`). Awaiting `.json()` on a clone of an unbounded stream never resolves, so the MCP connection would hang for as long as an override was configured. It now matches the media type exactly (`application/json` or a `+json` suffix), with a regression test driven by a stream that never closes. The overrides also leave `issuer` as discovery returned it, so pointing one at an authorization server that advertises a different issuer is rejected on callback by the RFC 9207 / SEP-2352 mix-up defense. That check is deliberately not weakened; the limit is now stated on the module, in the field description, and in the configuration guide. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
ac8a08e42f
commit
e5c20608bf
4 changed files with 78 additions and 4 deletions
|
|
@ -875,7 +875,7 @@ export function ServerSettingsForm({
|
||||||
</Stack>
|
</Stack>
|
||||||
<ClearableTextInput
|
<ClearableTextInput
|
||||||
label="Authorization URL override"
|
label="Authorization URL override"
|
||||||
description={`Leave blank to use the authorization_endpoint the authorization server's metadata advertises. Set it to point this server at a development or staging authorization server instead.${endpointOverrideEmaNote}`}
|
description={`Leave blank to use the authorization_endpoint the authorization server's metadata advertises. Set it to point this server at a development or staging authorization server instead. Endpoints only — the discovered issuer is unchanged, so an authorization server advertising a different issuer is rejected on callback (RFC 9207).${endpointOverrideEmaNote}`}
|
||||||
placeholder="https://staging.auth.example.com/authorize"
|
placeholder="https://staging.auth.example.com/authorize"
|
||||||
value={settings.oauthAuthorizationUrl ?? ""}
|
value={settings.oauthAuthorizationUrl ?? ""}
|
||||||
error={oauthEndpointUrlError(
|
error={oauthEndpointUrlError(
|
||||||
|
|
|
||||||
|
|
@ -282,6 +282,46 @@ describe("withOAuthEndpointOverrides", () => {
|
||||||
await expect(response.text()).resolves.toBe("not json at all");
|
await expect(response.text()).resolves.toBe("not json at all");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// `application/x-ndjson` satisfies a naive `includes("json")` test, and the
|
||||||
|
// streamable-HTTP transport serves its long-lived server-push channel that way
|
||||||
|
// (see `isLongLivedStreamResponse`). Awaiting `.json()` on a clone of an
|
||||||
|
// unbounded stream never resolves, so the MCP connection would hang for as
|
||||||
|
// long as an override was configured. Regression test for PR #2037 review.
|
||||||
|
it("does not read an unbounded NDJSON stream", async () => {
|
||||||
|
// A body that never closes: if the wrapper reads it, this test times out.
|
||||||
|
const stream = new ReadableStream<Uint8Array>({
|
||||||
|
start(controller) {
|
||||||
|
controller.enqueue(new TextEncoder().encode('{"jsonrpc":"2.0"}\n'));
|
||||||
|
// Deliberately never closed.
|
||||||
|
},
|
||||||
|
});
|
||||||
|
const original = new Response(stream, {
|
||||||
|
headers: { "content-type": "application/x-ndjson" },
|
||||||
|
});
|
||||||
|
const wrapped = withOAuthEndpointOverrides(passThrough(original), () => ({
|
||||||
|
tokenUrl: STAGING_TOKEN,
|
||||||
|
}));
|
||||||
|
await expect(wrapped("https://mcp.example.com/mcp")).resolves.toBe(
|
||||||
|
original,
|
||||||
|
);
|
||||||
|
await original.body?.cancel();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("ignores content-type parameters when deciding a body is JSON", async () => {
|
||||||
|
const wrapped = withOAuthEndpointOverrides(
|
||||||
|
passThrough(
|
||||||
|
new Response(JSON.stringify(METADATA), {
|
||||||
|
headers: { "content-type": "application/json; charset=utf-8" },
|
||||||
|
}),
|
||||||
|
),
|
||||||
|
() => ({ tokenUrl: STAGING_TOKEN }),
|
||||||
|
);
|
||||||
|
const response = await wrapped("https://as.example.com/x");
|
||||||
|
await expect(response.json()).resolves.toMatchObject({
|
||||||
|
token_endpoint: STAGING_TOKEN,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
// Fetch forbids a body on 204/205, so rebuilding one would throw. A JSON
|
// Fetch forbids a body on 204/205, so rebuilding one would throw. A JSON
|
||||||
// content-type on an empty response is unusual but legal, and it must not
|
// content-type on an empty response is unusual but legal, and it must not
|
||||||
// take down an unrelated transport request.
|
// take down an unrelated transport request.
|
||||||
|
|
|
||||||
|
|
@ -22,7 +22,7 @@
|
||||||
* sees both is the `fetchFn` the SDK uses for discovery, so the override is
|
* sees both is the `fetchFn` the SDK uses for discovery, so the override is
|
||||||
* applied to the metadata document in flight.
|
* applied to the metadata document in flight.
|
||||||
*
|
*
|
||||||
* Three consequences worth knowing:
|
* Four consequences worth knowing:
|
||||||
*
|
*
|
||||||
* - **A metadata document is required.** When discovery returns nothing, the SDK
|
* - **A metadata document is required.** When discovery returns nothing, the SDK
|
||||||
* falls back to `/authorize` and `/token` on the authorization server's origin
|
* falls back to `/authorize` and `/token` on the authorization server's origin
|
||||||
|
|
@ -36,6 +36,17 @@
|
||||||
* suppresses the overrides when the server is enterprise-managed. This mirrors
|
* suppresses the overrides when the server is enterprise-managed. This mirrors
|
||||||
* `redirectToExternalAuthorization` skipping the custom authorization
|
* `redirectToExternalAuthorization` skipping the custom authorization
|
||||||
* parameters (#2018).
|
* parameters (#2018).
|
||||||
|
* - **They redirect endpoints, they do not re-point the issuer.** `issuer` is
|
||||||
|
* left exactly as discovery returned it, deliberately: it is the authorization
|
||||||
|
* server's *identity*, and it is what RFC 9207 / SEP-2352 check the callback's
|
||||||
|
* `iss` against to defend against an authorization-server mix-up. So these
|
||||||
|
* overrides fit alternate endpoints of the same logical issuer (a staging
|
||||||
|
* deployment fronting the same issuer, a local proxy). Aim one at an
|
||||||
|
* authorization server that advertises a *different* issuer and the callback
|
||||||
|
* is rejected as an issuer mismatch before the code is redeemed — which is the
|
||||||
|
* mix-up defense working. Suppressing that check to make the override "work"
|
||||||
|
* would trade a real security property for a debugging convenience, so the
|
||||||
|
* limit is documented rather than removed.
|
||||||
* - **The Network tab shows the patched document.** The wrapper is applied to the
|
* - **The Network tab shows the patched document.** The wrapper is applied to the
|
||||||
* base fetch, inside the request tracker, so what the tab renders is the
|
* base fetch, inside the request tracker, so what the tab renders is the
|
||||||
* metadata as the flow consumed it. That is the useful reading: it explains why
|
* metadata as the flow consumed it. That is the useful reading: it explains why
|
||||||
|
|
@ -185,6 +196,27 @@ export function applyOAuthEndpointOverrides(
|
||||||
return patched;
|
return patched;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Whether a `content-type` names a **whole JSON document** — something that can
|
||||||
|
* be read to completion.
|
||||||
|
*
|
||||||
|
* This must be an exact media-type match, not a substring test for "json". The
|
||||||
|
* streamable-HTTP transport's long-lived server-push channel can be served as
|
||||||
|
* `application/x-ndjson` (see `isLongLivedStreamResponse` in
|
||||||
|
* `core/mcp/fetchTracking.ts`), which is an unbounded stream: awaiting `.json()`
|
||||||
|
* on a clone of it would never resolve, and since this wrapper awaits before
|
||||||
|
* returning, the MCP connection would hang for as long as an override is
|
||||||
|
* configured. Metadata documents are plain `application/json`, so the narrow
|
||||||
|
* test loses nothing. The `+json` structured suffix is accepted for the same
|
||||||
|
* reason it exists — it names a concrete document, not a stream.
|
||||||
|
*/
|
||||||
|
function isJsonDocumentResponse(contentType: string | null): boolean {
|
||||||
|
if (!contentType) return false;
|
||||||
|
// Strip parameters (`; charset=utf-8`) and compare the media type itself.
|
||||||
|
const mediaType = contentType.split(";")[0].trim().toLowerCase();
|
||||||
|
return mediaType === "application/json" || mediaType.endsWith("+json");
|
||||||
|
}
|
||||||
|
|
||||||
/** Rebuild a response around a replacement body, preserving status/headers. */
|
/** Rebuild a response around a replacement body, preserving status/headers. */
|
||||||
function responseWithBody(response: Response, body: string): Response {
|
function responseWithBody(response: Response, body: string): Response {
|
||||||
const headers = new Headers(response.headers);
|
const headers = new Headers(response.headers);
|
||||||
|
|
@ -237,8 +269,8 @@ export function withOAuthEndpointOverrides(
|
||||||
const overrides = resolveOverrides();
|
const overrides = resolveOverrides();
|
||||||
if (!overrides) return response;
|
if (!overrides) return response;
|
||||||
if (!response.ok) return response;
|
if (!response.ok) return response;
|
||||||
const contentType = response.headers.get("content-type") ?? "";
|
if (!isJsonDocumentResponse(response.headers.get("content-type")))
|
||||||
if (!contentType.toLowerCase().includes("json")) return response;
|
return response;
|
||||||
|
|
||||||
// Read through a clone so every path that does NOT patch can return the
|
// Read through a clone so every path that does NOT patch can return the
|
||||||
// caller's own `Response`, untouched. Rebuilding unconditionally would drop
|
// caller's own `Response`, untouched. Rebuilding unconditionally would drop
|
||||||
|
|
|
||||||
|
|
@ -181,6 +181,8 @@ These have no analog in the broader `mcp.json` ecosystem. Each is **omitted on w
|
||||||
|
|
||||||
`oauth.authorizationUrl` and `oauth.tokenUrl` override the `authorization_endpoint` and `token_endpoint` that authorization-server metadata discovery resolved. The Inspector deliberately has no such fields by default — it resolves both from the AS's metadata document, exactly as a real MCP host does — but a server under development often advertises its *production* authorization server while you want to hit staging. Set either (they are independent) to an absolute `http(s)` URL and it replaces what the metadata returned, for both the authorization request and the token request; leave blank to use discovery. A malformed value is flagged inline in the form and dropped with a warning at connect time rather than failing the connection. Edit them in Server Settings → Authorization ("Authorization URL override" / "Token URL override").
|
`oauth.authorizationUrl` and `oauth.tokenUrl` override the `authorization_endpoint` and `token_endpoint` that authorization-server metadata discovery resolved. The Inspector deliberately has no such fields by default — it resolves both from the AS's metadata document, exactly as a real MCP host does — but a server under development often advertises its *production* authorization server while you want to hit staging. Set either (they are independent) to an absolute `http(s)` URL and it replaces what the metadata returned, for both the authorization request and the token request; leave blank to use discovery. A malformed value is flagged inline in the form and dropped with a warning at connect time rather than failing the connection. Edit them in Server Settings → Authorization ("Authorization URL override" / "Token URL override").
|
||||||
|
|
||||||
|
They redirect the **endpoints**, not the authorization server's identity: `issuer` is left exactly as discovery returned it, because that is what RFC 9207 / SEP-2352 validate the callback's `iss` against to defend against an authorization-server mix-up. So they fit alternate endpoints of the *same* logical issuer — a staging deployment fronting the same issuer, a local proxy. Point one at an authorization server advertising a **different** issuer and the callback is rejected as an issuer mismatch before the code is redeemed; that is the mix-up defense working, and it is not disabled to make the override succeed.
|
||||||
|
|
||||||
They are **not applied under enterprise-managed authorization** (`oauth.enterpriseManaged`), for the same reason `oauth.authorizationParams` isn't: that flow authorizes against the enterprise IdP — a different authorization server, whose OIDC discovery would otherwise be rewritten too, pointing the IdP login (or the IdP code exchange) at the resource server's authorization server.
|
They are **not applied under enterprise-managed authorization** (`oauth.enterpriseManaged`), for the same reason `oauth.authorizationParams` isn't: that flow authorizes against the enterprise IdP — a different authorization server, whose OIDC discovery would otherwise be rewritten too, pointing the IdP login (or the IdP code exchange) at the resource server's authorization server.
|
||||||
|
|
||||||
> Because the override is applied to the *discovered metadata document*, a server whose authorization server publishes no metadata at all is unaffected — there the SDK falls back to `/authorize` and `/token` on the AS origin, and there is nothing to override.
|
> Because the override is applied to the *discovered metadata document*, a server whose authorization server publishes no metadata at all is unaffected — there the SDK falls back to `/authorize` and `/token` on the AS origin, and there is nothing to override.
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue