fix: exempt MCP traffic by origin, not path (#2319 round 17)
exemptMcpEndpoint compared origin + pathname, which misses legacy SSE entirely: SSEClientTransport opens the configured URL and is handed a SEPARATE message endpoint by the server, then POSTs every JSON-RPC message to that second pathname. Those were bounded and body-buffered as OAuth traffic, so a slow tool call over SSE would be severed at 30s and reported as an OAuthRequestTimeoutError. It compares origin alone now. That is exact rather than approximate: the SDK enforces that the SSE message endpoint shares the stream URL's origin and rejects one that does not, so every MCP request this chain carries is same-origin with the configured URL by construction. The cost, stated in the docblock and pinned by a test: an OAuth endpoint on the MCP server's own origin is exempt here and so unbounded on this chain. The OAuth chain still bounds the Inspector's own requests to it, so only the SDK's transport-internal OAuth against a same-origin server falls back to where it was before #2319. Caller cancellation was also still swallowed in two later places in the compat probe loop — `probe.text()` has its own catch, and `fetch` resolves on headers, so an abort mid-body landed there and was answered with the preceding 404; and an abort during releaseBody's await would have carried on probing. Both recheck the signal now, and all three sites throw the caller's own `reason` rather than whatever shape the layer beneath rejected with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFnM6RqB4nXqNysgUYmjmq Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
e234343702
commit
5a1c157a79
2 changed files with 111 additions and 13 deletions
|
|
@ -504,6 +504,45 @@ describe("probe cancellation (#2319)", () => {
|
|||
expect(inner).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("rethrows an abort that lands while the probe body is pending", async () => {
|
||||
// `fetch` resolves on headers, so a late abort rejects `probe.text()`
|
||||
// rather than the fetch — a separate catch, which used to answer with the
|
||||
// preceding 404.
|
||||
const caller = new AbortController();
|
||||
const inner = vi.fn<typeof fetch>((input, init) => {
|
||||
if (String(input) === RFC8414) {
|
||||
return Promise.resolve(new Response(null, { status: 404 }));
|
||||
}
|
||||
// Headers now, body never.
|
||||
return Promise.resolve(
|
||||
new Response(
|
||||
new ReadableStream<Uint8Array>({
|
||||
start(ctrl) {
|
||||
ctrl.enqueue(new TextEncoder().encode('{"issuer":'));
|
||||
init?.signal?.addEventListener("abort", () =>
|
||||
ctrl.error(init.signal?.reason as Error),
|
||||
);
|
||||
},
|
||||
}),
|
||||
{ headers: { "content-type": "application/json" } },
|
||||
),
|
||||
);
|
||||
});
|
||||
const wrapped = withRfc8414OidcCompat(inner);
|
||||
|
||||
const settled = wrapped(RFC8414, { signal: caller.signal }).then(
|
||||
(r) => r,
|
||||
(e: unknown) => e,
|
||||
);
|
||||
await vi.waitFor(() => {
|
||||
expect(inner).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
const reason = new Error("caller gave up mid-body");
|
||||
caller.abort(reason);
|
||||
|
||||
expect(await settled).toBe(reason);
|
||||
});
|
||||
|
||||
it("still falls back to the original response on an ordinary probe failure", async () => {
|
||||
const inner = vi.fn<typeof fetch>((input) => {
|
||||
if (String(input) === RFC8414) {
|
||||
|
|
|
|||
|
|
@ -506,17 +506,46 @@ describe("withOAuthRequestTimeout", () => {
|
|||
expect(isExempt(`${SERVER}#frag`)).toBe(true);
|
||||
});
|
||||
|
||||
it("bounds OAuth work on the same chain", () => {
|
||||
it("exempts legacy SSE's separate message endpoint", () => {
|
||||
// The case a path rule gets wrong: `SSEClientTransport` opens the
|
||||
// configured URL and is handed a *different* pathname to POST every
|
||||
// JSON-RPC message to. Bounding those would sever a slow tool call over
|
||||
// SSE and report it as an OAuth timeout. The SDK enforces that the
|
||||
// message endpoint shares the stream URL's origin, which is what makes
|
||||
// an origin rule exact rather than approximate.
|
||||
const isExempt = exemptMcpEndpoint(() => "https://srv.example.com/sse");
|
||||
|
||||
expect(isExempt("https://srv.example.com/messages?sessionId=abc")).toBe(
|
||||
true,
|
||||
);
|
||||
expect(isExempt("https://srv.example.com/")).toBe(true);
|
||||
});
|
||||
|
||||
it("bounds OAuth work on a different origin", () => {
|
||||
const isExempt = exemptMcpEndpoint(() => SERVER);
|
||||
|
||||
expect(isExempt("https://as.example.com/token")).toBe(false);
|
||||
expect(
|
||||
isExempt("https://as.example.com/.well-known/openid-configuration"),
|
||||
).toBe(false);
|
||||
// Origin is scheme + host + port, so any of the three differing bounds it.
|
||||
expect(isExempt("http://srv.example.com/token")).toBe(false);
|
||||
expect(isExempt("https://srv.example.com:8443/token")).toBe(false);
|
||||
});
|
||||
|
||||
it("exempts a same-origin OAuth endpoint, which is the cost of the rule", () => {
|
||||
// Stated as a test rather than left implicit: protected-resource metadata
|
||||
// lives on the resource's own origin, so it is exempt here. The OAuth
|
||||
// chain still bounds the Inspector's own requests to it; only the SDK's
|
||||
// transport-internal OAuth against a same-origin server falls back to the
|
||||
// SDK's per-request timeout.
|
||||
const isExempt = exemptMcpEndpoint(() => SERVER);
|
||||
|
||||
expect(
|
||||
isExempt(
|
||||
"https://srv.example.com/.well-known/oauth-protected-resource/mcp",
|
||||
),
|
||||
).toBe(false);
|
||||
expect(isExempt("https://as.example.com/token")).toBe(false);
|
||||
// Same origin, different path — still not the endpoint.
|
||||
expect(isExempt("https://srv.example.com/mcp/register")).toBe(false);
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("fails open when the server URL is unknown or unparseable", () => {
|
||||
|
|
@ -540,6 +569,7 @@ describe("withOAuthRequestTimeout", () => {
|
|||
expect(isExempt("https://other.example.com/mcp")).toBe(false);
|
||||
current = "https://other.example.com/mcp";
|
||||
expect(isExempt("https://other.example.com/mcp")).toBe(true);
|
||||
expect(isExempt(SERVER)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue