fix(auth): carry resource_metadata into first-time legacy authorization
The challenge-driven plumbing did not reach the default CLI/TUI flow. With no stored token in the legacy era, InspectorClient deliberately builds the transport with no authProvider (so the SDK cannot open a browser before the callback server is listening), which also leaves challenge interception off; the 401 then surfaces as the SDK's headerless UnauthorizedError and the runner calls authenticate() with no challenge in hand. So a server serving metadata only at the advertised path still failed there — including for the legacy showcase the README instructs. Add a passive counterpart to the challenge interceptor: an observer fetch that reports every 401/403 without touching control flow or the body. The OAuthManager records the advertised URL from it and uses it in authenticate() and its CIMD pre-registration. Latest-wins, including clearing when a later challenge advertises none. Also: the fixture helper dropped an explicitly supplied empty resourceMetadataPath by truthiness, bypassing validation and silently reverting to the well-known route. Covered by a direct-client integration test that drives connect -> authenticate() against the real server and asserts the advertised path was requested and the well-known one never was; verified failing without the fix. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
8beba25b58
commit
597e1bd25f
2 changed files with 242 additions and 5 deletions
|
|
@ -1777,6 +1777,61 @@ describe("OAuthManager", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("uses the last observed challenge on the plain authenticate() path", async () => {
|
||||
// The legacy first-authorization path: no challenge is passed in, so the
|
||||
// one the transport observed is the only source (#2071).
|
||||
mockedMcpAuth.mockResolvedValue("REDIRECT");
|
||||
const params = createMockParams();
|
||||
const manager = new OAuthManager(params);
|
||||
const captureSpy = vi
|
||||
.spyOn(
|
||||
(await import("@inspector/core/auth/providers.js"))
|
||||
.BaseOAuthClientProvider.prototype,
|
||||
"getCapturedAuthUrl",
|
||||
)
|
||||
.mockReturnValue(new URL("https://auth.example.com/authorize?state=x"));
|
||||
|
||||
manager.noteObservedAuthChallenge({
|
||||
reason: "unauthorized",
|
||||
resourceMetadataUrl: METADATA_URL,
|
||||
});
|
||||
await manager.authenticate();
|
||||
|
||||
expect(mockedMcpAuth).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
expect.objectContaining({
|
||||
resourceMetadataUrl: new URL(METADATA_URL),
|
||||
}),
|
||||
);
|
||||
captureSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("clears the observed URL when a later challenge advertises none", async () => {
|
||||
mockedMcpAuth.mockResolvedValue("REDIRECT");
|
||||
const params = createMockParams();
|
||||
const manager = new OAuthManager(params);
|
||||
const captureSpy = vi
|
||||
.spyOn(
|
||||
(await import("@inspector/core/auth/providers.js"))
|
||||
.BaseOAuthClientProvider.prototype,
|
||||
"getCapturedAuthUrl",
|
||||
)
|
||||
.mockReturnValue(new URL("https://auth.example.com/authorize?state=x"));
|
||||
|
||||
manager.noteObservedAuthChallenge({
|
||||
reason: "unauthorized",
|
||||
resourceMetadataUrl: METADATA_URL,
|
||||
});
|
||||
manager.noteObservedAuthChallenge({ reason: "unauthorized" });
|
||||
await manager.authenticate();
|
||||
|
||||
expect(mockedMcpAuth).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
expect.objectContaining({ resourceMetadataUrl: undefined }),
|
||||
);
|
||||
captureSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("forwards it on the forced step-up reauthorization leg too", async () => {
|
||||
mockedMcpAuth.mockResolvedValue("AUTHORIZED");
|
||||
const params = createMockParams();
|
||||
|
|
|
|||
|
|
@ -10,8 +10,10 @@
|
|||
* plumbing is load-bearing rather than merely present.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeAll, afterAll } from "vitest";
|
||||
import { describe, it, expect, beforeAll, afterAll, vi } from "vitest";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { discoverOAuthProtectedResourceMetadata } from "@modelcontextprotocol/client";
|
||||
import {
|
||||
TestServerHttp,
|
||||
|
|
@ -26,8 +28,19 @@ import {
|
|||
challengeResourceMetadataUrl,
|
||||
parseAuthChallengeFromResponse,
|
||||
} from "@inspector/core/auth/challenge.js";
|
||||
import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js";
|
||||
import { createTransportNode } from "@inspector/core/mcp/node/transport.js";
|
||||
import {
|
||||
ConsoleNavigation,
|
||||
type RedirectUrlProvider,
|
||||
} from "@inspector/core/auth/providers.js";
|
||||
import { NodeOAuthStorage } from "@inspector/core/auth/node/storage-node.js";
|
||||
import type { MCPServerConfig } from "@inspector/core/mcp/types.js";
|
||||
|
||||
const METADATA_PATH = "/custom/protected-resource";
|
||||
const STATIC_CLIENT_ID = "test-2071-resource-metadata";
|
||||
const STATIC_CLIENT_SECRET = "test-2071-secret";
|
||||
const REDIRECT_URL = "http://localhost:3000/oauth/callback";
|
||||
|
||||
describe("OAuth challenge resource_metadata (RFC 9728)", () => {
|
||||
let mcpServer: TestServerHttp | null = null;
|
||||
|
|
@ -40,6 +53,16 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => {
|
|||
...createOAuthTestServerConfig({
|
||||
requireAuth: true,
|
||||
resourceMetadataPath: METADATA_PATH,
|
||||
// Static rather than DCR: this suite is about *where the metadata
|
||||
// document is found*, so registration should not be a second thing
|
||||
// that can fail.
|
||||
staticClients: [
|
||||
{
|
||||
clientId: STATIC_CLIENT_ID,
|
||||
clientSecret: STATIC_CLIENT_SECRET,
|
||||
redirectUris: [REDIRECT_URL],
|
||||
},
|
||||
],
|
||||
}),
|
||||
});
|
||||
const port = await mcpServer.start();
|
||||
|
|
@ -89,6 +112,68 @@ describe("OAuth challenge resource_metadata (RFC 9728)", () => {
|
|||
).rejects.toThrow();
|
||||
});
|
||||
|
||||
it("drives first-time legacy authorization off the advertised URL (#2071)", async () => {
|
||||
// The gap the challenge-driven plumbing alone does not close: with no
|
||||
// stored token in the default legacy era, `InspectorClient` builds the
|
||||
// transport with no `authProvider` and no challenge interception, so the
|
||||
// 401 reaches the client as the SDK's headerless `UnauthorizedError` and
|
||||
// the CLI/TUI runner calls `authenticate()` with nothing in hand. The
|
||||
// observed-challenge path is what carries the URL across that gap.
|
||||
const requested: string[] = [];
|
||||
const spyFetch = vi.fn(
|
||||
async (input: RequestInfo | URL, init?: RequestInit) => {
|
||||
requested.push(String(input instanceof Request ? input.url : input));
|
||||
return fetch(input as RequestInfo, init);
|
||||
},
|
||||
) as unknown as typeof fetch;
|
||||
|
||||
const redirectUrlProvider: RedirectUrlProvider = {
|
||||
getRedirectUrl: () => REDIRECT_URL,
|
||||
};
|
||||
|
||||
const client = new InspectorClient(
|
||||
{
|
||||
type: "streamable-http",
|
||||
url: `${serverUrl}/mcp`,
|
||||
} as MCPServerConfig,
|
||||
{
|
||||
environment: {
|
||||
transport: createTransportNode,
|
||||
fetch: spyFetch,
|
||||
oauth: {
|
||||
storage: new NodeOAuthStorage(
|
||||
join(tmpdir(), `mcp-oauth-${process.pid}-2071.json`),
|
||||
),
|
||||
navigation: new ConsoleNavigation(),
|
||||
redirectUrlProvider,
|
||||
},
|
||||
},
|
||||
oauth: {
|
||||
clientId: STATIC_CLIENT_ID,
|
||||
clientSecret: STATIC_CLIENT_SECRET,
|
||||
scope: "mcp",
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
await expect(client.connect()).rejects.toThrow();
|
||||
|
||||
// Reaching an authorization URL at all means discovery resolved — which it
|
||||
// can only have done through the advertised path, since the well-known one
|
||||
// 404s on this server.
|
||||
const authUrl = await client.authenticate();
|
||||
expect(authUrl?.href).toContain("/oauth/authorize");
|
||||
|
||||
expect(requested).toContain(`${serverUrl}${METADATA_PATH}`);
|
||||
expect(
|
||||
requested.filter((url) =>
|
||||
url.includes("/.well-known/oauth-protected-resource"),
|
||||
),
|
||||
).toEqual([]);
|
||||
|
||||
await client.disconnect();
|
||||
}, 30_000);
|
||||
|
||||
it("keeps the showcase config valid and carrying the custom path", () => {
|
||||
const configPath = fileURLToPath(
|
||||
new URL(
|
||||
|
|
|
|||
Loading…
Reference in a new issue