Merge pull request #2287 from modelcontextprotocol/v2/fix/2242-cimd-registration-kind
fix: preserve CIMD registration provenance across SDK issuer binding
This commit is contained in:
commit
98911df74d
7 changed files with 871 additions and 48 deletions
|
|
@ -24,6 +24,7 @@ describe("ensureCimdClientRegistration", () => {
|
||||||
storage = {
|
storage = {
|
||||||
getClientInformation: vi.fn(async () => undefined),
|
getClientInformation: vi.fn(async () => undefined),
|
||||||
saveClientInformation: vi.fn(async () => {}),
|
saveClientInformation: vi.fn(async () => {}),
|
||||||
|
getDiscoveryState: vi.fn(async () => undefined),
|
||||||
getScope: vi.fn().mockResolvedValue(undefined),
|
getScope: vi.fn().mockResolvedValue(undefined),
|
||||||
getTokens: vi.fn(async () => undefined),
|
getTokens: vi.fn(async () => undefined),
|
||||||
saveTokens: vi.fn(async () => {}),
|
saveTokens: vi.fn(async () => {}),
|
||||||
|
|
@ -62,12 +63,15 @@ describe("ensureCimdClientRegistration", () => {
|
||||||
fetchFn,
|
fetchFn,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2242: the record is bound to the issuer just discovered, so a second AS
|
||||||
|
// behind the same resource gets its own CIMD determination rather than
|
||||||
|
// inheriting this one.
|
||||||
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
SERVER_URL,
|
SERVER_URL,
|
||||||
{
|
{
|
||||||
client_id: METADATA_URL,
|
client_id: METADATA_URL,
|
||||||
},
|
},
|
||||||
{ registrationKind: "cimd" },
|
{ registrationKind: "cimd", issuer: "http://127.0.0.1:9999" },
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|
@ -154,22 +158,104 @@ describe("ensureCimdClientRegistration", () => {
|
||||||
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
SERVER_URL,
|
SERVER_URL,
|
||||||
{ client_id: METADATA_URL },
|
{ client_id: METADATA_URL },
|
||||||
{ registrationKind: "cimd" },
|
{ registrationKind: "cimd", issuer: "http://127.0.0.1:9999" },
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("no-ops when client information is already stored", async () => {
|
// #2242 (Copilot): the existing-client check moved after discovery, so this
|
||||||
storage.getClientInformation = vi.fn(async () => ({
|
// helper must not turn a well-known outage into a failed reconnect. It reuses
|
||||||
client_id: "existing-client",
|
// the discovery state SDK `auth()` persists, and treats a discovery failure as
|
||||||
|
// "skip pre-registration" rather than an error.
|
||||||
|
it("reuses persisted discovery state instead of re-fetching", async () => {
|
||||||
|
storage.getDiscoveryState = vi.fn(async () => ({
|
||||||
|
authorizationServerUrl: "http://127.0.0.1:9999",
|
||||||
|
authorizationServerMetadata: {
|
||||||
|
issuer: "http://127.0.0.1:9999",
|
||||||
|
authorization_endpoint: "http://127.0.0.1:9999/oauth/authorize",
|
||||||
|
token_endpoint: "http://127.0.0.1:9999/oauth/token",
|
||||||
|
response_types_supported: ["code"],
|
||||||
|
client_id_metadata_document_supported: true,
|
||||||
|
},
|
||||||
}));
|
}));
|
||||||
|
const fetchFn = vi.fn(async () => {
|
||||||
|
throw new Error("discovery must not run when state is cached");
|
||||||
|
});
|
||||||
|
|
||||||
|
await ensureCimdClientRegistration({
|
||||||
|
serverUrl: SERVER_URL,
|
||||||
|
provider: createProvider(storage),
|
||||||
|
fetchFn,
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(fetchFn).not.toHaveBeenCalled();
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER_URL,
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "cimd", issuer: "http://127.0.0.1:9999" },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("skips pre-registration when discovery fails, rather than throwing", async () => {
|
||||||
|
const fetchFn = vi.fn(async () => {
|
||||||
|
throw new Error("well-known endpoint is down");
|
||||||
|
});
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
ensureCimdClientRegistration({
|
||||||
|
serverUrl: SERVER_URL,
|
||||||
|
provider: createProvider(storage),
|
||||||
|
fetchFn,
|
||||||
|
}),
|
||||||
|
).resolves.toBeUndefined();
|
||||||
|
|
||||||
|
expect(storage.saveClientInformation).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("no-ops when client information is already stored for the discovered issuer", async () => {
|
||||||
|
// Dynamic slot only — a preregistered hit would short-circuit
|
||||||
|
// `clientInformation()` before it ever reaches the issuer-keyed read.
|
||||||
|
storage.getClientInformation = vi.fn(
|
||||||
|
async (_url: string, preregistered?: boolean) =>
|
||||||
|
preregistered ? undefined : { client_id: "existing-client" },
|
||||||
|
);
|
||||||
|
storage.getClientRegistrationKind = vi.fn(
|
||||||
|
async (): Promise<"dcr"> => "dcr",
|
||||||
|
);
|
||||||
|
|
||||||
|
const fetchFn = vi.fn(async (input: RequestInfo | URL) => {
|
||||||
|
const url = String(input);
|
||||||
|
if (url.includes("/.well-known/oauth-protected-resource")) {
|
||||||
|
return new Response(JSON.stringify({ resource: SERVER_URL }));
|
||||||
|
}
|
||||||
|
if (url.includes("/.well-known/oauth-authorization-server")) {
|
||||||
|
return new Response(
|
||||||
|
JSON.stringify({
|
||||||
|
issuer: "http://127.0.0.1:9999",
|
||||||
|
authorization_endpoint: "http://127.0.0.1:9999/oauth/authorize",
|
||||||
|
token_endpoint: "http://127.0.0.1:9999/oauth/token",
|
||||||
|
response_types_supported: ["code"],
|
||||||
|
client_id_metadata_document_supported: true,
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
throw new Error(`unexpected fetch: ${url}`);
|
||||||
|
});
|
||||||
|
|
||||||
const provider = createProvider(storage);
|
const provider = createProvider(storage);
|
||||||
await ensureCimdClientRegistration({
|
await ensureCimdClientRegistration({
|
||||||
serverUrl: SERVER_URL,
|
serverUrl: SERVER_URL,
|
||||||
provider,
|
provider,
|
||||||
fetchFn: vi.fn(),
|
fetchFn,
|
||||||
});
|
});
|
||||||
|
|
||||||
expect(storage.saveClientInformation).not.toHaveBeenCalled();
|
expect(storage.saveClientInformation).not.toHaveBeenCalled();
|
||||||
|
// #2242: the existing-client check is keyed by the issuer discovery just
|
||||||
|
// resolved, not read ctx-less — a ctx-less read resolves through the
|
||||||
|
// *active* issuer and would early-return for every later issuer.
|
||||||
|
expect(storage.getClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER_URL,
|
||||||
|
false,
|
||||||
|
"http://127.0.0.1:9999",
|
||||||
|
);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -65,6 +65,8 @@ interface FakeInner {
|
||||||
clearCapturedAuthUrl: ReturnType<typeof vi.fn>;
|
clearCapturedAuthUrl: ReturnType<typeof vi.fn>;
|
||||||
saveCodeVerifier: ReturnType<typeof vi.fn>;
|
saveCodeVerifier: ReturnType<typeof vi.fn>;
|
||||||
codeVerifier: ReturnType<typeof vi.fn>;
|
codeVerifier: ReturnType<typeof vi.fn>;
|
||||||
|
saveDiscoveryState: ReturnType<typeof vi.fn>;
|
||||||
|
discoveryState: ReturnType<typeof vi.fn>;
|
||||||
}
|
}
|
||||||
|
|
||||||
function createInner(): FakeInner {
|
function createInner(): FakeInner {
|
||||||
|
|
@ -82,6 +84,8 @@ function createInner(): FakeInner {
|
||||||
clearCapturedAuthUrl: vi.fn(),
|
clearCapturedAuthUrl: vi.fn(),
|
||||||
saveCodeVerifier: vi.fn(),
|
saveCodeVerifier: vi.fn(),
|
||||||
codeVerifier: vi.fn(() => "verifier-xyz"),
|
codeVerifier: vi.fn(() => "verifier-xyz"),
|
||||||
|
saveDiscoveryState: vi.fn(),
|
||||||
|
discoveryState: vi.fn(),
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -118,14 +122,54 @@ describe("EmaTransportOAuthProvider", () => {
|
||||||
expect(await provider.codeVerifier()).toBe("verifier-xyz");
|
expect(await provider.codeVerifier()).toBe("verifier-xyz");
|
||||||
|
|
||||||
await provider.saveClientInformation({ client_id: "new" } as never);
|
await provider.saveClientInformation({ client_id: "new" } as never);
|
||||||
expect(inner.saveClientInformation).toHaveBeenCalledWith({
|
expect(inner.saveClientInformation).toHaveBeenCalledWith(
|
||||||
client_id: "new",
|
{ client_id: "new" },
|
||||||
});
|
undefined,
|
||||||
|
);
|
||||||
|
|
||||||
await provider.saveCodeVerifier("cv");
|
await provider.saveCodeVerifier("cv");
|
||||||
expect(inner.saveCodeVerifier).toHaveBeenCalledWith("cv");
|
expect(inner.saveCodeVerifier).toHaveBeenCalledWith("cv");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// SEP-2352: the wrapper used to drop the SDK's `ctx`, so every EMA read and
|
||||||
|
// write landed on the unkeyed slot — and, since #2242, the registration-kind
|
||||||
|
// resolver had no issuer to check and recorded a CIMD registration made over
|
||||||
|
// an EMA connection as DCR (Copilot).
|
||||||
|
it("forwards the SDK issuer context on client-information reads and writes", async () => {
|
||||||
|
const ctx = { issuer: "https://as.example.com" };
|
||||||
|
|
||||||
|
await provider.clientInformation(ctx);
|
||||||
|
expect(inner.clientInformation).toHaveBeenCalledWith(ctx);
|
||||||
|
|
||||||
|
await provider.saveClientInformation({ client_id: "new" } as never, ctx);
|
||||||
|
expect(inner.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
{ client_id: "new" },
|
||||||
|
ctx,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Without these the SDK persists no discovery state for an EMA connection, so
|
||||||
|
// it re-discovers every call, cannot run its callback-leg AS binding check,
|
||||||
|
// and leaves the registration-kind resolver nothing to read back.
|
||||||
|
it("delegates discovery state to the inner provider", async () => {
|
||||||
|
const state = {
|
||||||
|
authorizationServerUrl: "https://as.example.com",
|
||||||
|
authorizationServerMetadata: {
|
||||||
|
issuer: "https://as.example.com",
|
||||||
|
authorization_endpoint: "https://as.example.com/authorize",
|
||||||
|
token_endpoint: "https://as.example.com/token",
|
||||||
|
response_types_supported: ["code"],
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
await provider.saveDiscoveryState(state);
|
||||||
|
expect(inner.saveDiscoveryState).toHaveBeenCalledWith(state);
|
||||||
|
|
||||||
|
inner.discoveryState.mockReturnValue(state);
|
||||||
|
expect(await provider.discoveryState()).toEqual(state);
|
||||||
|
expect(inner.discoveryState).toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
it("tokens() returns stored tokens when the access token is still usable", async () => {
|
it("tokens() returns stored tokens when the access token is still usable", async () => {
|
||||||
const stored: OAuthTokens = {
|
const stored: OAuthTokens = {
|
||||||
access_token: VALID_ACCESS_TOKEN,
|
access_token: VALID_ACCESS_TOKEN,
|
||||||
|
|
|
||||||
|
|
@ -7,6 +7,10 @@ import {
|
||||||
type OAuthProviderConfig,
|
type OAuthProviderConfig,
|
||||||
} from "@inspector/core/auth/providers.js";
|
} from "@inspector/core/auth/providers.js";
|
||||||
import type { OAuthStorage } from "@inspector/core/auth/storage.js";
|
import type { OAuthStorage } from "@inspector/core/auth/storage.js";
|
||||||
|
import { OAuthStorageBase } from "@inspector/core/auth/oauth-storage.js";
|
||||||
|
import { OAuthMemoryStore } from "@inspector/core/auth/store.js";
|
||||||
|
import type { OAuthPersistBackend } from "@inspector/core/auth/oauth-persist.js";
|
||||||
|
import { ensureCimdClientRegistration } from "@inspector/core/auth/cimd.js";
|
||||||
import {
|
import {
|
||||||
BrowserNavigation,
|
BrowserNavigation,
|
||||||
BrowserOAuthClientProvider,
|
BrowserOAuthClientProvider,
|
||||||
|
|
@ -216,6 +220,7 @@ describe("OAuthNavigation", () => {
|
||||||
load: vi.fn().mockResolvedValue(undefined),
|
load: vi.fn().mockResolvedValue(undefined),
|
||||||
getScope: vi.fn().mockResolvedValue(undefined),
|
getScope: vi.fn().mockResolvedValue(undefined),
|
||||||
getClientInformation: vi.fn(async () => undefined),
|
getClientInformation: vi.fn(async () => undefined),
|
||||||
|
getClientRegistrationKind: vi.fn(async () => undefined),
|
||||||
saveClientInformation: vi.fn(async () => undefined),
|
saveClientInformation: vi.fn(async () => undefined),
|
||||||
savePreregisteredClientInformation: vi.fn(async () => undefined),
|
savePreregisteredClientInformation: vi.fn(async () => undefined),
|
||||||
saveScope: vi.fn(async () => undefined),
|
saveScope: vi.fn(async () => undefined),
|
||||||
|
|
@ -238,11 +243,13 @@ describe("OAuthNavigation", () => {
|
||||||
function makeProvider(
|
function makeProvider(
|
||||||
storage: OAuthStorage,
|
storage: OAuthStorage,
|
||||||
navCallback = vi.fn(),
|
navCallback = vi.fn(),
|
||||||
|
extraConfig: Partial<OAuthProviderConfig> = {},
|
||||||
): BaseOAuthClientProvider {
|
): BaseOAuthClientProvider {
|
||||||
const config: OAuthProviderConfig = {
|
const config: OAuthProviderConfig = {
|
||||||
storage,
|
storage,
|
||||||
redirectUrlProvider: new MutableRedirectUrlProvider(),
|
redirectUrlProvider: new MutableRedirectUrlProvider(),
|
||||||
navigation: new CallbackNavigation(navCallback),
|
navigation: new CallbackNavigation(navCallback),
|
||||||
|
...extraConfig,
|
||||||
};
|
};
|
||||||
return new BaseOAuthClientProvider(SERVER, config);
|
return new BaseOAuthClientProvider(SERVER, config);
|
||||||
}
|
}
|
||||||
|
|
@ -625,6 +632,448 @@ describe("OAuthNavigation", () => {
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2242: the SDK binds an existing registration to its issuer by calling
|
||||||
|
// `saveClientInformation(info, { issuer })` with no registration kind.
|
||||||
|
// Treating every such save as DCR relabeled a CIMD registration
|
||||||
|
// "Dynamic (DCR)" in Connection Info, even though no `POST /register`
|
||||||
|
// ever happened.
|
||||||
|
describe("registration kind on an unstamped (SDK) save", () => {
|
||||||
|
const ISSUER = "https://as.example.com";
|
||||||
|
const METADATA_URL = "https://app.example.com/client-metadata.json";
|
||||||
|
|
||||||
|
/** Discovery state as SDK `auth()` persists it before saving client info. */
|
||||||
|
function seedDiscovery(
|
||||||
|
storage: OAuthStorage,
|
||||||
|
issuer: string,
|
||||||
|
cimd: boolean,
|
||||||
|
) {
|
||||||
|
vi.mocked(storage.getDiscoveryState).mockResolvedValue({
|
||||||
|
authorizationServerUrl: issuer,
|
||||||
|
authorizationServerMetadata: {
|
||||||
|
issuer,
|
||||||
|
authorization_endpoint: `${issuer}/authorize`,
|
||||||
|
token_endpoint: `${issuer}/token`,
|
||||||
|
response_types_supported: ["code"],
|
||||||
|
...(cimd && { client_id_metadata_document_supported: true }),
|
||||||
|
},
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Storage whose issuer slot already holds a CIMD registration. */
|
||||||
|
function makeCimdStorage(): OAuthStorage {
|
||||||
|
const storage = makeStorage();
|
||||||
|
vi.mocked(storage.getClientInformation).mockImplementation(
|
||||||
|
async (_url: string, preregistered?: boolean) =>
|
||||||
|
preregistered ? undefined : { client_id: METADATA_URL },
|
||||||
|
);
|
||||||
|
vi.mocked(storage.getClientRegistrationKind).mockResolvedValue(
|
||||||
|
"cimd",
|
||||||
|
);
|
||||||
|
return storage;
|
||||||
|
}
|
||||||
|
|
||||||
|
it("keeps cimd when back-stamping a registration recorded as cimd", async () => {
|
||||||
|
const storage = makeCimdStorage();
|
||||||
|
const provider = makeProvider(storage, vi.fn(), {
|
||||||
|
clientMetadataUrl: METADATA_URL,
|
||||||
|
});
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "cimd", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// RFC 7591 §3.2 leaves a dynamically issued `client_id` opaque, so an
|
||||||
|
// existing DCR may carry the configured metadata URL. Back-stamping it
|
||||||
|
// must not relabel it (Copilot).
|
||||||
|
it("keeps dcr when back-stamping a DCR that uses the metadata URL", async () => {
|
||||||
|
const storage = makeStorage();
|
||||||
|
vi.mocked(storage.getClientInformation).mockImplementation(
|
||||||
|
async (_url: string, preregistered?: boolean) =>
|
||||||
|
preregistered ? undefined : { client_id: METADATA_URL },
|
||||||
|
);
|
||||||
|
vi.mocked(storage.getClientRegistrationKind).mockResolvedValue("dcr");
|
||||||
|
// Even with an AS that does advertise CIMD.
|
||||||
|
seedDiscovery(storage, ISSUER, true);
|
||||||
|
const provider = makeProvider(storage, vi.fn(), {
|
||||||
|
clientMetadataUrl: METADATA_URL,
|
||||||
|
});
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "dcr", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Nothing stored for this issuer, so the SDK is creating the
|
||||||
|
// registration. It reaches its URL-based-client-ID branch exactly when
|
||||||
|
// the AS advertises CIMD — read back from the discovery state it
|
||||||
|
// persisted moments earlier.
|
||||||
|
it("records cimd for a new registration when the AS advertises CIMD", async () => {
|
||||||
|
const storage = makeStorage();
|
||||||
|
seedDiscovery(storage, ISSUER, true);
|
||||||
|
const provider = makeProvider(storage, vi.fn(), {
|
||||||
|
clientMetadataUrl: METADATA_URL,
|
||||||
|
});
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "cimd", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("records dcr for a new registration when the AS does not advertise CIMD", async () => {
|
||||||
|
const storage = makeStorage();
|
||||||
|
seedDiscovery(storage, ISSUER, false);
|
||||||
|
const provider = makeProvider(storage, vi.fn(), {
|
||||||
|
clientMetadataUrl: METADATA_URL,
|
||||||
|
});
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "dcr", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("records dcr when the discovery state describes a different issuer", async () => {
|
||||||
|
const storage = makeStorage();
|
||||||
|
seedDiscovery(storage, "https://as-other.example.com", true);
|
||||||
|
const provider = makeProvider(storage, vi.fn(), {
|
||||||
|
clientMetadataUrl: METADATA_URL,
|
||||||
|
});
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "dcr", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("records dcr for a server-minted client_id while CIMD is configured", async () => {
|
||||||
|
const storage = makeCimdStorage();
|
||||||
|
seedDiscovery(storage, ISSUER, true);
|
||||||
|
const provider = makeProvider(storage, vi.fn(), {
|
||||||
|
clientMetadataUrl: METADATA_URL,
|
||||||
|
});
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: "dcr-minted-id" },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
// The id is not the metadata URL, so nothing is read at all.
|
||||||
|
expect(storage.getClientInformation).not.toHaveBeenCalled();
|
||||||
|
expect(storage.getDiscoveryState).not.toHaveBeenCalled();
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: "dcr-minted-id" },
|
||||||
|
{ registrationKind: "dcr", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("records dcr when CIMD is not configured for this connection", async () => {
|
||||||
|
const storage = makeCimdStorage();
|
||||||
|
seedDiscovery(storage, ISSUER, true);
|
||||||
|
const provider = makeProvider(storage);
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "dcr", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// SEP-2352 keys registrations per authorization server, so a second AS
|
||||||
|
// behind one resource is a separate determination. Driven against a real
|
||||||
|
// `OAuthStorageBase` and the real `ensureCimdClientRegistration`, because
|
||||||
|
// the behaviour under test is how the pre-registration binds provenance to
|
||||||
|
// a discovered issuer and how storage promotes and clears slots — neither
|
||||||
|
// of which a mock would express (Copilot).
|
||||||
|
describe("across two authorization servers", () => {
|
||||||
|
const ISSUER_B = "https://as-b.example.com";
|
||||||
|
|
||||||
|
function makeRealStorage(): OAuthStorage {
|
||||||
|
const backend: OAuthPersistBackend = {
|
||||||
|
read: async () => null,
|
||||||
|
write: async () => {},
|
||||||
|
};
|
||||||
|
return new OAuthStorageBase(new OAuthMemoryStore(), backend);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Discovery that points the resource at `issuer` as its authorization
|
||||||
|
* server and declares CIMD support per `cimd`. The RFC 9728 document
|
||||||
|
* has to name the AS, so that the RFC 8414 §3.3 issuer echo the SDK
|
||||||
|
* enforces resolves against the AS URL rather than the resource's.
|
||||||
|
*/
|
||||||
|
function discoveryFetch(issuer: string, cimd: boolean): typeof fetch {
|
||||||
|
return async (input: RequestInfo | URL) => {
|
||||||
|
const url = String(input);
|
||||||
|
if (url.includes("/.well-known/oauth-protected-resource")) {
|
||||||
|
return new Response(
|
||||||
|
JSON.stringify({
|
||||||
|
resource: SERVER,
|
||||||
|
authorization_servers: [issuer],
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
if (url.startsWith(issuer)) {
|
||||||
|
return new Response(
|
||||||
|
JSON.stringify({
|
||||||
|
issuer,
|
||||||
|
authorization_endpoint: `${issuer}/authorize`,
|
||||||
|
token_endpoint: `${issuer}/token`,
|
||||||
|
response_types_supported: ["code"],
|
||||||
|
...(cimd && {
|
||||||
|
client_id_metadata_document_supported: true,
|
||||||
|
}),
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
throw new Error(`unexpected fetch: ${url}`);
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Issuer A pre-registers via CIMD, then the SDK binds it. */
|
||||||
|
async function bindIssuerA(storage: OAuthStorage) {
|
||||||
|
const provider = makeProvider(storage, vi.fn(), {
|
||||||
|
clientMetadataUrl: METADATA_URL,
|
||||||
|
});
|
||||||
|
await ensureCimdClientRegistration({
|
||||||
|
serverUrl: SERVER,
|
||||||
|
provider,
|
||||||
|
fetchFn: discoveryFetch(ISSUER, true),
|
||||||
|
});
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
await storage.getClientRegistrationKind(SERVER, ISSUER),
|
||||||
|
).toBe("cimd");
|
||||||
|
return provider;
|
||||||
|
}
|
||||||
|
|
||||||
|
it("keeps cimd when a second CIMD-supporting issuer takes over", async () => {
|
||||||
|
const storage = makeRealStorage();
|
||||||
|
const provider = await bindIssuerA(storage);
|
||||||
|
|
||||||
|
// Issuer B also advertises CIMD, so the pre-registration records it
|
||||||
|
// for B too — it must not early-return on issuer A's client.
|
||||||
|
await ensureCimdClientRegistration({
|
||||||
|
serverUrl: SERVER,
|
||||||
|
provider,
|
||||||
|
fetchFn: discoveryFetch(ISSUER_B, true),
|
||||||
|
});
|
||||||
|
await storage.saveDiscoveryState(SERVER, {
|
||||||
|
authorizationServerUrl: ISSUER_B,
|
||||||
|
authorizationServerMetadata: {
|
||||||
|
issuer: ISSUER_B,
|
||||||
|
authorization_endpoint: `${ISSUER_B}/authorize`,
|
||||||
|
token_endpoint: `${ISSUER_B}/token`,
|
||||||
|
response_types_supported: ["code"],
|
||||||
|
client_id_metadata_document_supported: true,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER_B },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(
|
||||||
|
await storage.getClientRegistrationKind(SERVER, ISSUER_B),
|
||||||
|
).toBe("cimd");
|
||||||
|
expect(
|
||||||
|
await storage.getClientRegistrationKind(SERVER, ISSUER),
|
||||||
|
).toBe("cimd");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("records dcr when a second issuer without CIMD mints the same URL as its client_id", async () => {
|
||||||
|
const storage = makeRealStorage();
|
||||||
|
const provider = await bindIssuerA(storage);
|
||||||
|
|
||||||
|
// Issuer B does *not* advertise CIMD, so nothing is recorded for B...
|
||||||
|
await ensureCimdClientRegistration({
|
||||||
|
serverUrl: SERVER,
|
||||||
|
provider,
|
||||||
|
fetchFn: discoveryFetch(ISSUER_B, false),
|
||||||
|
});
|
||||||
|
// Discovery state as SDK `auth()` persists it for issuer B — which
|
||||||
|
// is what tells the save apart from a CIMD one. Seeded explicitly so
|
||||||
|
// the assertion rests on B's advertised capabilities rather than on
|
||||||
|
// discovery state merely being absent.
|
||||||
|
await storage.saveDiscoveryState(SERVER, {
|
||||||
|
authorizationServerUrl: ISSUER_B,
|
||||||
|
authorizationServerMetadata: {
|
||||||
|
issuer: ISSUER_B,
|
||||||
|
authorization_endpoint: `${ISSUER_B}/authorize`,
|
||||||
|
token_endpoint: `${ISSUER_B}/token`,
|
||||||
|
response_types_supported: ["code"],
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
// ...and RFC 7591 §3.2 lets it mint an opaque id that happens to be
|
||||||
|
// the very URL issuer A uses as its CIMD client_id.
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER_B },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(
|
||||||
|
await storage.getClientRegistrationKind(SERVER, ISSUER_B),
|
||||||
|
).toBe("dcr");
|
||||||
|
// Issuer A's own provenance is untouched.
|
||||||
|
expect(
|
||||||
|
await storage.getClientRegistrationKind(SERVER, ISSUER),
|
||||||
|
).toBe("cimd");
|
||||||
|
});
|
||||||
|
|
||||||
|
// Copilot: an AS advertising CIMD does not make an *existing* dynamic
|
||||||
|
// registration a CIMD one. RFC 7591 §3.2 leaves the id opaque, so a
|
||||||
|
// real DCR may carry the metadata URL; end to end, it must stay `dcr`.
|
||||||
|
it("does not relabel an existing DCR whose client_id is the metadata URL", async () => {
|
||||||
|
const storage = makeRealStorage();
|
||||||
|
const provider = makeProvider(storage, vi.fn(), {
|
||||||
|
clientMetadataUrl: METADATA_URL,
|
||||||
|
});
|
||||||
|
|
||||||
|
// A real dynamic registration that happens to use the same URL.
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "dcr", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
// The AS does advertise CIMD, so the pre-registration runs and finds
|
||||||
|
// that registration already in place.
|
||||||
|
await ensureCimdClientRegistration({
|
||||||
|
serverUrl: SERVER,
|
||||||
|
provider,
|
||||||
|
fetchFn: discoveryFetch(ISSUER, true),
|
||||||
|
});
|
||||||
|
// The SDK's issuer back-stamp of that same registration.
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(
|
||||||
|
await storage.getClientRegistrationKind(SERVER, ISSUER),
|
||||||
|
).toBe("dcr");
|
||||||
|
});
|
||||||
|
|
||||||
|
// SDK v2 `auth()` answers `invalid_client` with
|
||||||
|
// `invalidateCredentials("client")` and an immediate retry. That clear
|
||||||
|
// removes the registration *and* its kind, so the retry takes the
|
||||||
|
// new-registration path and must be answered from the discovery state
|
||||||
|
// — which the clear does not touch (Copilot). Asserted against real
|
||||||
|
// storage, since the point is what `clearClientInformation` does.
|
||||||
|
it("keeps cimd through invalid-client recovery, which clears the credential", async () => {
|
||||||
|
const storage = makeRealStorage();
|
||||||
|
const provider = await bindIssuerA(storage);
|
||||||
|
// Discovery state as SDK `auth()` persisted it for issuer A.
|
||||||
|
await storage.saveDiscoveryState(SERVER, {
|
||||||
|
authorizationServerUrl: ISSUER,
|
||||||
|
authorizationServerMetadata: {
|
||||||
|
issuer: ISSUER,
|
||||||
|
authorization_endpoint: `${ISSUER}/authorize`,
|
||||||
|
token_endpoint: `${ISSUER}/token`,
|
||||||
|
response_types_supported: ["code"],
|
||||||
|
client_id_metadata_document_supported: true,
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
await provider.invalidateCredentials("client");
|
||||||
|
|
||||||
|
// The credential and its recorded kind are both gone...
|
||||||
|
expect(
|
||||||
|
await storage.getClientInformation(SERVER, false, ISSUER),
|
||||||
|
).toBeUndefined();
|
||||||
|
expect(
|
||||||
|
await storage.getClientRegistrationKind(SERVER, ISSUER),
|
||||||
|
).toBeUndefined();
|
||||||
|
|
||||||
|
// ...so the SDK's retry re-runs its URL-based client-ID branch.
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(
|
||||||
|
await storage.getClientRegistrationKind(SERVER, ISSUER),
|
||||||
|
).toBe("cimd");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("records dcr for a second issuer that mints its own client_id", async () => {
|
||||||
|
const storage = makeRealStorage();
|
||||||
|
const provider = await bindIssuerA(storage);
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: "b-registered-id" },
|
||||||
|
{ issuer: ISSUER_B },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(
|
||||||
|
await storage.getClientRegistrationKind(SERVER, ISSUER_B),
|
||||||
|
).toBe("dcr");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("an explicit registrationKind wins and consults no storage reads", async () => {
|
||||||
|
const storage = makeCimdStorage();
|
||||||
|
const provider = makeProvider(storage, vi.fn(), {
|
||||||
|
clientMetadataUrl: METADATA_URL,
|
||||||
|
});
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "cimd" },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.getClientInformation).not.toHaveBeenCalled();
|
||||||
|
expect(storage.getClientRegistrationKind).not.toHaveBeenCalled();
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "cimd", issuer: undefined },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it("round-trips discovery state to storage", async () => {
|
it("round-trips discovery state to storage", async () => {
|
||||||
const storage = makeStorage();
|
const storage = makeStorage();
|
||||||
const provider = makeProvider(storage);
|
const provider = makeProvider(storage);
|
||||||
|
|
|
||||||
|
|
@ -370,6 +370,112 @@ describe("InspectorClient OAuth E2E", () => {
|
||||||
|
|
||||||
// Connection should now be successful
|
// Connection should now be successful
|
||||||
expect(client.getStatus()).toBe("connected");
|
expect(client.getStatus()).toBe("connected");
|
||||||
|
|
||||||
|
// #2242: the metadata-document URL is the client_id, and the stored
|
||||||
|
// provenance still says CIMD after the SDK bound the registration to
|
||||||
|
// the issuer — no `POST /register` ever happened.
|
||||||
|
//
|
||||||
|
// ⚠️ This assertion alone does not exercise `resolveSdkRegistrationKind`:
|
||||||
|
// the CIMD pre-registration now writes the issuer-keyed slot itself, so
|
||||||
|
// the SDK finds an already-stamped credential and never calls
|
||||||
|
// `saveClientInformation`. The test below covers the resolver across the
|
||||||
|
// integration boundary (Copilot).
|
||||||
|
const oauthState = await client.getOAuthState();
|
||||||
|
expect(oauthState?.client).toMatchObject({
|
||||||
|
clientId: metadataUrl,
|
||||||
|
registrationKind: "cimd",
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
// The reported #2242 shape: an unkeyed CIMD registration — what every
|
||||||
|
// pre-SEP-2352 install has on disk, and what the pre-registration wrote
|
||||||
|
// before it knew the issuer. The SDK back-stamps it, which is the save
|
||||||
|
// that used to relabel it `Dynamic (DCR)`. This is the case that puts
|
||||||
|
// `resolveSdkRegistrationKind` on the path end to end.
|
||||||
|
it("keeps CIMD provenance when the SDK issuer-stamps an unkeyed registration", async () => {
|
||||||
|
const testRedirectUrl = "http://localhost:3001/oauth/callback";
|
||||||
|
|
||||||
|
const clientMetadata: ClientMetadataDocument = {
|
||||||
|
redirect_uris: [testRedirectUrl],
|
||||||
|
token_endpoint_auth_method: "none",
|
||||||
|
grant_types: ["authorization_code", "refresh_token"],
|
||||||
|
response_types: ["code"],
|
||||||
|
client_name: "MCP Inspector Test Client",
|
||||||
|
client_uri: "https://github.com/modelcontextprotocol/inspector",
|
||||||
|
scope: "mcp",
|
||||||
|
};
|
||||||
|
|
||||||
|
metadataServer = await createClientMetadataServer(clientMetadata);
|
||||||
|
const metadataUrl = metadataServer.url;
|
||||||
|
|
||||||
|
const serverConfig = {
|
||||||
|
...getDefaultServerConfig(),
|
||||||
|
serverType: transport.serverType,
|
||||||
|
...createOAuthTestServerConfig({
|
||||||
|
requireAuth: true,
|
||||||
|
supportCIMD: true,
|
||||||
|
}),
|
||||||
|
};
|
||||||
|
|
||||||
|
server = new TestServerHttp(serverConfig);
|
||||||
|
const port = await server.start();
|
||||||
|
const serverUrl = `http://localhost:${port}`;
|
||||||
|
await waitForOAuthWellKnown(serverUrl);
|
||||||
|
|
||||||
|
const oauthConfig = createTestOAuthConfig({
|
||||||
|
mode: "cimd",
|
||||||
|
clientMetadataUrl: metadataUrl,
|
||||||
|
redirectUrl: testRedirectUrl,
|
||||||
|
});
|
||||||
|
|
||||||
|
const mcpUrl = `${serverUrl}${transport.endpoint}`;
|
||||||
|
// Seed the legacy unkeyed slot: a CIMD registration with no issuer.
|
||||||
|
await oauthConfig.storage.saveClientInformation(
|
||||||
|
mcpUrl,
|
||||||
|
{ client_id: metadataUrl },
|
||||||
|
{ registrationKind: "cimd" },
|
||||||
|
);
|
||||||
|
|
||||||
|
const clientConfig: InspectorClientOptions = {
|
||||||
|
environment: {
|
||||||
|
transport: createTransportNode,
|
||||||
|
oauth: {
|
||||||
|
storage: oauthConfig.storage,
|
||||||
|
navigation: oauthConfig.navigation,
|
||||||
|
redirectUrlProvider: oauthConfig.redirectUrlProvider,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
oauth: {
|
||||||
|
clientId: oauthConfig.clientId,
|
||||||
|
clientSecret: oauthConfig.clientSecret,
|
||||||
|
clientMetadataUrl: oauthConfig.clientMetadataUrl,
|
||||||
|
scope: oauthConfig.scope,
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
client = new InspectorClient(
|
||||||
|
{
|
||||||
|
type: transport.clientType,
|
||||||
|
url: mcpUrl,
|
||||||
|
} as MCPServerConfig,
|
||||||
|
clientConfig,
|
||||||
|
);
|
||||||
|
|
||||||
|
const authUrl = await client.authenticate();
|
||||||
|
if (!authUrl) throw new Error("Expected authorization URL");
|
||||||
|
|
||||||
|
const { code: authCode, iss: authCodeIss } =
|
||||||
|
await completeOAuthAuthorization(authUrl);
|
||||||
|
await client.completeOAuthFlow(authCode, authCodeIss);
|
||||||
|
await client.connect();
|
||||||
|
|
||||||
|
expect(client.getStatus()).toBe("connected");
|
||||||
|
|
||||||
|
const oauthState = await client.getOAuthState();
|
||||||
|
expect(oauthState?.client).toMatchObject({
|
||||||
|
clientId: metadataUrl,
|
||||||
|
registrationKind: "cimd",
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should retry original request after OAuth completion with CIMD", async () => {
|
it("should retry original request after OAuth completion with CIMD", async () => {
|
||||||
|
|
|
||||||
|
|
@ -1,5 +1,4 @@
|
||||||
import { discoverOAuthProtectedResourceMetadata } from "@modelcontextprotocol/client";
|
import { discoverOAuthProtectedResourceMetadata } from "@modelcontextprotocol/client";
|
||||||
import type { OAuthClientInformation } from "@modelcontextprotocol/client";
|
|
||||||
import { discoverAuthorizationServerMetadataForServer } from "./discovery.js";
|
import { discoverAuthorizationServerMetadataForServer } from "./discovery.js";
|
||||||
import type { BaseOAuthClientProvider } from "./providers.js";
|
import type { BaseOAuthClientProvider } from "./providers.js";
|
||||||
|
|
||||||
|
|
@ -27,39 +26,69 @@ export async function ensureCimdClientRegistration(params: {
|
||||||
const clientMetadataUrl = params.provider.clientMetadataUrl?.trim();
|
const clientMetadataUrl = params.provider.clientMetadataUrl?.trim();
|
||||||
if (!clientMetadataUrl) return;
|
if (!clientMetadataUrl) return;
|
||||||
|
|
||||||
const existing = await params.provider.clientInformation();
|
// Prefer the discovery state SDK `auth()` itself persists and reuses. Without
|
||||||
if (existing?.client_id) return;
|
// this, moving the existing-client check after discovery would turn a
|
||||||
|
// temporary well-known outage into a failed reconnect, even where the SDK
|
||||||
|
// could have proceeded from cache (Copilot).
|
||||||
|
let metadata = (await params.provider.discoveryState())
|
||||||
|
?.authorizationServerMetadata;
|
||||||
|
|
||||||
let resourceMetadata;
|
if (!metadata) {
|
||||||
try {
|
let resourceMetadata;
|
||||||
resourceMetadata = await discoverOAuthProtectedResourceMetadata(
|
try {
|
||||||
params.serverUrl,
|
resourceMetadata = await discoverOAuthProtectedResourceMetadata(
|
||||||
{ resourceMetadataUrl: params.resourceMetadataUrl },
|
params.serverUrl,
|
||||||
// The same fetch the AS-metadata leg below uses. On web that is
|
{ resourceMetadataUrl: params.resourceMetadataUrl },
|
||||||
// `createRemoteFetch`, which proxies through the backend to sidestep
|
// The same fetch the AS-metadata leg below uses. On web that is
|
||||||
// CORS — on the global `fetch` this leg would fail in the browser, be
|
// `createRemoteFetch`, which proxies through the backend to sidestep
|
||||||
// swallowed by the catch, and leave CIMD probing the wrong
|
// CORS — on the global `fetch` this leg would fail in the browser, be
|
||||||
// authorization server (Copilot).
|
// swallowed by the catch, and leave CIMD probing the wrong
|
||||||
params.fetchFn,
|
// authorization server (Copilot).
|
||||||
);
|
params.fetchFn,
|
||||||
} catch {
|
);
|
||||||
resourceMetadata = undefined;
|
} catch {
|
||||||
|
resourceMetadata = undefined;
|
||||||
|
}
|
||||||
|
|
||||||
|
try {
|
||||||
|
// Walks the path-scoped authorization-server URL before the bare origin, so
|
||||||
|
// a server hosted under a path is probed where it actually publishes its
|
||||||
|
// metadata rather than only at the domain root (#2110).
|
||||||
|
metadata = await discoverAuthorizationServerMetadataForServer(
|
||||||
|
params.serverUrl,
|
||||||
|
resourceMetadata,
|
||||||
|
params.fetchFn,
|
||||||
|
);
|
||||||
|
} catch {
|
||||||
|
// Pre-registration is an optimization over what SDK `auth()` does for
|
||||||
|
// itself, so a discovery failure here must never fail the connection: bail
|
||||||
|
// out and let `auth()` run its own discovery and error handling.
|
||||||
|
return;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Walks the path-scoped authorization-server URL before the bare origin, so a
|
const issuer = metadata?.issuer;
|
||||||
// server hosted under a path is probed where it actually publishes its
|
|
||||||
// metadata rather than only at the domain root (#2110).
|
|
||||||
const metadata = await discoverAuthorizationServerMetadataForServer(
|
|
||||||
params.serverUrl,
|
|
||||||
resourceMetadata,
|
|
||||||
params.fetchFn,
|
|
||||||
);
|
|
||||||
if (!metadata?.client_id_metadata_document_supported) return;
|
if (!metadata?.client_id_metadata_document_supported) return;
|
||||||
|
|
||||||
const clientInformation: OAuthClientInformation = {
|
// ⚠️ Keyed by the issuer just resolved, not read ctx-less. A ctx-less read
|
||||||
client_id: clientMetadataUrl,
|
// resolves through the *active* issuer, so it early-returns for every
|
||||||
};
|
// subsequent issuer and leaves them with no CIMD registration at all. It
|
||||||
await params.provider.saveClientInformation(clientInformation, {
|
// answers the static case first, since `clientInformation` checks the
|
||||||
registrationKind: "cimd",
|
// preregistered slot before any issuer slot — and a static client, like any
|
||||||
});
|
// existing registration, means there is nothing to pre-register.
|
||||||
|
const existing = await params.provider.clientInformation(
|
||||||
|
issuer ? { issuer } : undefined,
|
||||||
|
);
|
||||||
|
if (existing?.client_id) return;
|
||||||
|
|
||||||
|
await params.provider.saveClientInformation(
|
||||||
|
{ client_id: clientMetadataUrl },
|
||||||
|
{
|
||||||
|
registrationKind: "cimd",
|
||||||
|
// An AS metadata document without an `issuer` is malformed (RFC 8414 §2),
|
||||||
|
// but the type allows it; fall back to the unkeyed slot rather than
|
||||||
|
// inventing a key.
|
||||||
|
...(issuer && { issuer }),
|
||||||
|
},
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1,7 +1,9 @@
|
||||||
import type { OAuthClientProvider } from "@modelcontextprotocol/client";
|
import type { OAuthClientProvider } from "@modelcontextprotocol/client";
|
||||||
import type {
|
import type {
|
||||||
|
OAuthClientInformationContext,
|
||||||
OAuthClientInformationMixed,
|
OAuthClientInformationMixed,
|
||||||
OAuthClientMetadata,
|
OAuthClientMetadata,
|
||||||
|
OAuthDiscoveryState,
|
||||||
OAuthTokens,
|
OAuthTokens,
|
||||||
} from "@modelcontextprotocol/client";
|
} from "@modelcontextprotocol/client";
|
||||||
import type { BaseOAuthClientProvider } from "../providers.js";
|
import type { BaseOAuthClientProvider } from "../providers.js";
|
||||||
|
|
@ -50,17 +52,40 @@ export class EmaTransportOAuthProvider implements OAuthClientProvider {
|
||||||
return this.inner.state();
|
return this.inner.state();
|
||||||
}
|
}
|
||||||
|
|
||||||
clientInformation():
|
// SEP-2352: `ctx` carries the authorization-server `issuer` the SDK resolved,
|
||||||
|
// and the inner provider keys registrations by it. Dropping it here made every
|
||||||
|
// EMA read and write land on the unkeyed slot — and, since #2242, left
|
||||||
|
// `resolveSdkRegistrationKind` with no issuer to check, so a CIMD registration
|
||||||
|
// made over an EMA connection was recorded as DCR (Copilot).
|
||||||
|
clientInformation(
|
||||||
|
ctx?: OAuthClientInformationContext,
|
||||||
|
):
|
||||||
| OAuthClientInformationMixed
|
| OAuthClientInformationMixed
|
||||||
| undefined
|
| undefined
|
||||||
| Promise<OAuthClientInformationMixed | undefined> {
|
| Promise<OAuthClientInformationMixed | undefined> {
|
||||||
return this.inner.clientInformation();
|
return this.inner.clientInformation(ctx);
|
||||||
}
|
}
|
||||||
|
|
||||||
saveClientInformation(
|
saveClientInformation(
|
||||||
clientInformation: OAuthClientInformationMixed,
|
clientInformation: OAuthClientInformationMixed,
|
||||||
|
ctx?: OAuthClientInformationContext,
|
||||||
): void | Promise<void> {
|
): void | Promise<void> {
|
||||||
return this.inner.saveClientInformation(clientInformation);
|
return this.inner.saveClientInformation(clientInformation, ctx);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Without these the SDK persists no discovery state for an EMA connection, so
|
||||||
|
// it re-discovers on every call, cannot perform its SEP-2352 callback-leg
|
||||||
|
// authorization-server binding check (it warns as much), and — since #2242 —
|
||||||
|
// leaves the registration-kind resolver nothing to read back.
|
||||||
|
saveDiscoveryState(state: OAuthDiscoveryState): void | Promise<void> {
|
||||||
|
return this.inner.saveDiscoveryState(state);
|
||||||
|
}
|
||||||
|
|
||||||
|
discoveryState():
|
||||||
|
| OAuthDiscoveryState
|
||||||
|
| undefined
|
||||||
|
| Promise<OAuthDiscoveryState | undefined> {
|
||||||
|
return this.inner.discoveryState();
|
||||||
}
|
}
|
||||||
|
|
||||||
async tokens(): Promise<OAuthTokens | undefined> {
|
async tokens(): Promise<OAuthTokens | undefined> {
|
||||||
|
|
|
||||||
|
|
@ -293,16 +293,19 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
||||||
// SDK v2's `OAuthClientProvider.saveClientInformation` passes an
|
// SDK v2's `OAuthClientProvider.saveClientInformation` passes an
|
||||||
// `OAuthClientInformationContext` ({ issuer }); our own DCR/CIMD callers
|
// `OAuthClientInformationContext` ({ issuer }); our own DCR/CIMD callers
|
||||||
// pass `SaveClientInformationOptions` ({ registrationKind }). Accept either
|
// pass `SaveClientInformationOptions` ({ registrationKind }). Accept either
|
||||||
// and read whichever keys are present: the SDK supplies `issuer` (SEP-2352
|
// and read whichever keys are present. The SDK supplies `issuer` (SEP-2352
|
||||||
// per-AS keying) and defaults registration kind to DCR; our callers supply
|
// per-AS keying) and never a kind, so `resolveSdkRegistrationKind` recovers
|
||||||
// the registration kind and no issuer yet.
|
// one. Our own callers always supply the kind, and supply the `issuer` too
|
||||||
|
// when they know it — `ensureCimdClientRegistration` does, having just
|
||||||
|
// discovered it; the unkeyed slot is only for the case where AS metadata
|
||||||
|
// carried no `issuer` at all.
|
||||||
options?: SaveClientInformationOptions | OAuthClientInformationContext,
|
options?: SaveClientInformationOptions | OAuthClientInformationContext,
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
|
const issuer = options && "issuer" in options ? options.issuer : undefined;
|
||||||
const registrationKind =
|
const registrationKind =
|
||||||
options && "registrationKind" in options
|
options && "registrationKind" in options
|
||||||
? options.registrationKind
|
? options.registrationKind
|
||||||
: "dcr";
|
: await this.resolveSdkRegistrationKind(clientInformation, issuer);
|
||||||
const issuer = options && "issuer" in options ? options.issuer : undefined;
|
|
||||||
await this.storage.saveClientInformation(
|
await this.storage.saveClientInformation(
|
||||||
this.serverUrl,
|
this.serverUrl,
|
||||||
clientInformation,
|
clientInformation,
|
||||||
|
|
@ -313,6 +316,87 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Resolve the registration kind for a save that carries no explicit one — that
|
||||||
|
* is, one the SDK made. SDK v2's `saveClientInformation` contract passes only
|
||||||
|
* `{ issuer }`, so the mechanism cannot be handed to us; treating every such
|
||||||
|
* save as DCR is what relabeled a CIMD registration `Dynamic (DCR)` in
|
||||||
|
* Connection Info the moment the SDK bound it to an issuer (#2242).
|
||||||
|
*
|
||||||
|
* Two cases reach here, and they are told apart by whether a registration
|
||||||
|
* already exists for this issuer:
|
||||||
|
*
|
||||||
|
* - **A back-stamp.** A registration is already stored for this issuer under
|
||||||
|
* this `client_id`, and the SDK is only adding the `issuer` to it. Its
|
||||||
|
* recorded kind is the answer — kind and credential are written and cleared
|
||||||
|
* together, so a stored registration always has one.
|
||||||
|
* - **A new registration.** Nothing is stored for this issuer, so this save
|
||||||
|
* creates it. SDK v2 `auth()` reaches its URL-based-client-ID branch — rather
|
||||||
|
* than `registerClient` — exactly when the AS advertises
|
||||||
|
* `client_id_metadata_document_supported` and a `clientMetadataUrl` is
|
||||||
|
* configured, and it persists the AS metadata via `saveDiscoveryState`
|
||||||
|
* *before* it reads or writes client information. So the branch it took is
|
||||||
|
* not inferred here, it is read back from the state it just wrote.
|
||||||
|
*
|
||||||
|
* This is why the check is not "the `client_id` looks like our metadata URL".
|
||||||
|
* RFC 7591 §3.2 makes a dynamically issued `client_id` opaque, so an AS may
|
||||||
|
* mint that very URL from `POST /register`; the URL comparison only decides
|
||||||
|
* whether CIMD is *in play* for this connection, and the two cases above decide
|
||||||
|
* what actually happened (#2242, Copilot).
|
||||||
|
*
|
||||||
|
* Consequences worth stating, since each was a defect on the way here:
|
||||||
|
*
|
||||||
|
* - An existing DCR whose `client_id` happens to be the metadata URL stays
|
||||||
|
* `dcr` — it takes the back-stamp path and its recorded kind says so.
|
||||||
|
* - `invalidateCredentials("client")`, which SDK `auth()` calls on
|
||||||
|
* `invalid_client` before retrying, clears the registration and its kind. The
|
||||||
|
* retry therefore takes the new-registration path and is answered from
|
||||||
|
* discovery state, which that clear does not touch.
|
||||||
|
* - A second AS behind one resource gets its own answer, since discovery state
|
||||||
|
* describes the issuer the SDK actually resolved. One that does not advertise
|
||||||
|
* CIMD is `dcr` even when it mints the metadata URL as its `client_id`.
|
||||||
|
* - A transient failure in our own CIMD preflight costs nothing: the SDK's own
|
||||||
|
* discovery is what this reads.
|
||||||
|
*/
|
||||||
|
private async resolveSdkRegistrationKind(
|
||||||
|
clientInformation: OAuthClientInformation,
|
||||||
|
issuer: string | undefined,
|
||||||
|
): Promise<SaveClientInformationOptions["registrationKind"]> {
|
||||||
|
const clientMetadataUrl = this.clientMetadataUrl?.trim();
|
||||||
|
if (
|
||||||
|
!clientMetadataUrl ||
|
||||||
|
clientInformation.client_id !== clientMetadataUrl
|
||||||
|
) {
|
||||||
|
return "dcr";
|
||||||
|
}
|
||||||
|
|
||||||
|
// Issuer-keyed, with `getClientInformation`'s own fallback to the unkeyed
|
||||||
|
// slot covering a registration written before an issuer was known.
|
||||||
|
const stored = await this.storage.getClientInformation(
|
||||||
|
this.serverUrl,
|
||||||
|
false,
|
||||||
|
issuer,
|
||||||
|
);
|
||||||
|
if (stored?.client_id === clientMetadataUrl) {
|
||||||
|
const storedKind = await this.storage.getClientRegistrationKind(
|
||||||
|
this.serverUrl,
|
||||||
|
issuer,
|
||||||
|
);
|
||||||
|
// `"static"` lives in the preregistered slot, never this one.
|
||||||
|
return storedKind === "cimd" ? "cimd" : "dcr";
|
||||||
|
}
|
||||||
|
|
||||||
|
// A new registration: read back the branch the SDK took.
|
||||||
|
const discovery = await this.storage.getDiscoveryState(this.serverUrl);
|
||||||
|
const metadata = discovery?.authorizationServerMetadata;
|
||||||
|
// Require the metadata to describe *this* issuer, so a state left over from
|
||||||
|
// a previously resolved AS cannot answer for a different one.
|
||||||
|
if (issuer !== undefined && metadata?.issuer !== issuer) return "dcr";
|
||||||
|
return metadata?.client_id_metadata_document_supported === true
|
||||||
|
? "cimd"
|
||||||
|
: "dcr";
|
||||||
|
}
|
||||||
|
|
||||||
async saveScope(scope: string | undefined): Promise<void> {
|
async saveScope(scope: string | undefined): Promise<void> {
|
||||||
await this.storage.saveScope(this.serverUrl, scope);
|
await this.storage.saveScope(this.serverUrl, scope);
|
||||||
this.cachedScope = scope;
|
this.cachedScope = scope;
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue