fix(auth): cover the transport's own discovery, and only rewrite GETs
Two more review findings, both real. The shim wrapped `effectiveAuthFetch` only, but the transport is constructed with the base `this.fetchFn`, and the SDK runs its own discovery from inside the transport on the 401/refresh leg — so a reconnect with an existing auth provider could still hit the upstream failure. It now wraps the base fetch, the same seam `withOAuthEndpointOverrides` uses and the only one that reaches both paths. That trades away the previous claim that a captured Network entry shows the real 404: below the trackers, what they record is the substitution. Rather than leave that silently misleading, a substituted response now carries `x-inspector-oauth-metadata-source` naming the URL its body was actually fetched from — the transport builds its tracker internally, so there is no seam above it to install into instead. An integration test captures the fetch the transport is handed and drives it against a real server, since nothing else proves that wiring. The wrapper also keyed on URL shape alone, but nothing stops an authorization or token endpoint from living under `/.well-known/oauth-authorization-server/`; a POST there returning an ordinary OAuth 4xx would have had its error replaced by a metadata document. Metadata discovery is a GET, so anything else is now left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
1db1b9be1f
commit
28c7a9ff8a
2 changed files with 141 additions and 18 deletions
|
|
@ -1,6 +1,7 @@
|
|||
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||
import { buildDiscoveryUrls } from "@modelcontextprotocol/client";
|
||||
import {
|
||||
COMPAT_SOURCE_HEADER,
|
||||
oidcDiscoveryCandidates,
|
||||
isRfc8414OnlyMetadata,
|
||||
withRfc8414OidcCompat,
|
||||
|
|
@ -144,11 +145,34 @@ describe("withRfc8414OidcCompat", () => {
|
|||
expect(response.status).toBe(200);
|
||||
expect(response.headers.get("content-type")).toBe("application/json");
|
||||
await expect(response.json()).resolves.toEqual(RFC8414_DOC);
|
||||
// The substitution names where its body came from, so a captured Network
|
||||
// entry does not read as a 200 from the RFC 8414 path.
|
||||
expect(response.headers.get(COMPAT_SOURCE_HEADER)).toBe(OIDC_APPENDED);
|
||||
// The failed original, then both OIDC candidates.
|
||||
expect(base).toHaveBeenCalledTimes(3);
|
||||
expect(warn).toHaveBeenCalledWith(expect.stringContaining(OIDC_APPENDED));
|
||||
});
|
||||
|
||||
it("does not touch a non-GET request to a matching path", async () => {
|
||||
// An authorization or token endpoint may legally live under the RFC 8414
|
||||
// well-known prefix; its ordinary OAuth error must reach the caller intact
|
||||
// rather than being replaced with a metadata document (Copilot).
|
||||
const original = new Response(JSON.stringify({ error: "invalid_grant" }), {
|
||||
status: 400,
|
||||
headers: { "content-type": "application/json" },
|
||||
});
|
||||
const base = vi.fn<typeof fetch>().mockResolvedValue(original);
|
||||
const wrapped = withRfc8414OidcCompat(base);
|
||||
|
||||
await expect(
|
||||
wrapped(`${RFC8414_URL}/token`, { method: "post" }),
|
||||
).resolves.toBe(original);
|
||||
await expect(
|
||||
wrapped(new Request(`${RFC8414_URL}/token`, { method: "POST" })),
|
||||
).resolves.toBe(original);
|
||||
expect(base).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("serves an RFC 8414 document found on the path-suffixed OIDC path", async () => {
|
||||
vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const base = vi.fn<typeof fetch>(async (input) => {
|
||||
|
|
|
|||
|
|
@ -17,7 +17,12 @@ import {
|
|||
createOAuthTestServerConfig,
|
||||
waitForOAuthWellKnown,
|
||||
} from "@modelcontextprotocol/inspector-test-server";
|
||||
import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js";
|
||||
import {
|
||||
COMPAT_SOURCE_HEADER,
|
||||
withRfc8414OidcCompat,
|
||||
} from "@inspector/core/auth/oidcDiscoveryCompat.js";
|
||||
import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js";
|
||||
import { createTransportNode } from "@inspector/core/mcp/node/transport.js";
|
||||
|
||||
const OIDC_PATH = "/.well-known/openid-configuration";
|
||||
|
||||
|
|
@ -37,7 +42,9 @@ describe("RFC 8414 metadata at the OIDC well-known path (#2172)", () => {
|
|||
...getDefaultServerConfig(),
|
||||
serverType: "streamable-http" as const,
|
||||
...createOAuthTestServerConfig({
|
||||
requireAuth: true,
|
||||
// Metadata routes are served either way; leaving auth unrequired lets
|
||||
// the transport-wiring test below actually connect.
|
||||
requireAuth: false,
|
||||
asMetadataPath: OIDC_PATH,
|
||||
}),
|
||||
});
|
||||
|
|
@ -97,4 +104,43 @@ describe("RFC 8414 metadata at the OIDC well-known path (#2172)", () => {
|
|||
warn.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it("hands the transport a fetch that carries the shim", async () => {
|
||||
// The SDK also runs discovery from *inside* the transport, which receives
|
||||
// `InspectorClient`'s base fetch directly — so a reconnect with an existing
|
||||
// auth provider must not bypass the shim (Copilot). Capturing the fetch the
|
||||
// transport was handed and driving it against this server is what proves
|
||||
// the wiring, without depending on the SDK to trigger that leg.
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
let transportFetch: typeof fetch | undefined;
|
||||
const capturingTransport: typeof createTransportNode = (
|
||||
config,
|
||||
options,
|
||||
) => {
|
||||
transportFetch = options?.fetchFn;
|
||||
return createTransportNode(config, options);
|
||||
};
|
||||
const client = new InspectorClient(
|
||||
{ type: "streamable-http", url: `${serverUrl}/mcp` },
|
||||
{ environment: { transport: capturingTransport } },
|
||||
);
|
||||
try {
|
||||
await client.connect();
|
||||
expect(transportFetch).toBeDefined();
|
||||
|
||||
const response = await transportFetch!(
|
||||
`${serverUrl}/.well-known/oauth-authorization-server`,
|
||||
);
|
||||
expect(response.status).toBe(200);
|
||||
expect(response.headers.get(COMPAT_SOURCE_HEADER)).toBe(
|
||||
`${serverUrl}${OIDC_PATH}`,
|
||||
);
|
||||
await expect(response.json()).resolves.toMatchObject({
|
||||
issuer: serverUrl,
|
||||
});
|
||||
} finally {
|
||||
await client.disconnect();
|
||||
warn.mockRestore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue