modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: keep CIMD provenance when a resource resolves to a second issuer

Browse source 
Copilot review (#2287): scoping the stored-CIMD check to the incoming issuer
alone mislabels the SDK's own CIMD write for a second authorization server.
SEP-2352 keys registrations per AS, so the first binding promotes the unkeyed
CIMD entry into issuer A's slot and clears the fallback;
`ensureCimdClientRegistration` then early-returns on its ctx-less read, and the
save under issuer B finds nothing recorded for B.

Check the issuer slot and then the server's active registration, so a second
issuer stays CIMD while a `client_id` the AS minted itself still falls through
to `dcr`. Covered by two tests driving a real `OAuthStorageBase` through the
A → B sequence, since the behaviour under test is how storage promotes and
clears slots rather than anything a mock would express.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-07 00:44:49 -04:00
commit f3f68c731b
2 changed files with 109 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

77
clients/web/src/test/core/auth/providers.test.ts
View file

@ -7,6 +7,9 @@ import {
type OAuthProviderConfig,
} from "@inspector/core/auth/providers.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 {
BrowserNavigation,
BrowserOAuthClientProvider,
@ -775,6 +778,80 @@ describe("OAuthNavigation", () => {
);
});
// SEP-2352 keys registrations per authorization server. Driven against a
// real `OAuthStorageBase` rather than mocks, because the bug is in how
// the *storage* promotes and clears slots across issuers (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);
}
async function bindFirstIssuer(storage: OAuthStorage) {
const provider = makeProvider(storage, vi.fn(), {
clientMetadataUrl: METADATA_URL,
});
// Our own pre-registration writes the unkeyed slot...
await provider.saveClientInformation(
{ client_id: METADATA_URL },
{ registrationKind: "cimd" },
);
// ...which the SDK's first issuer-stamped save promotes into
// issuer A's slot, clearing the unkeyed fallback.
await provider.saveClientInformation(
{ client_id: METADATA_URL },
{ issuer: ISSUER },
);
return provider;
}
it("keeps cimd when the resource resolves to a second issuer", async () => {
const storage = makeRealStorage();
const provider = await bindFirstIssuer(storage);
expect(
await storage.getClientRegistrationKind(SERVER, ISSUER),
).toBe("cimd");
// The precondition that made this go wrong: nothing is stored for
// issuer B, and the unkeyed fallback is gone.
expect(
await storage.getClientInformation(SERVER, false, ISSUER_B),
).toBeUndefined();
// The SDK's own CIMD branch, saving under the second issuer.
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("still records dcr for a second issuer that mints its own client_id", async () => {
const storage = makeRealStorage();
const provider = await bindFirstIssuer(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(), {

50
core/auth/providers.ts
View file

@ -326,9 +326,9 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
*
* 1. CIMD is configured for this connection right now, and
* 2. the incoming `client_id` is exactly that metadata-document URL, and
* 3. the registration already stored under that same `client_id` is recorded
* as `cimd` — written by `ensureCimdClientRegistration`, which reaches that
* line only after confirming the AS advertises
* 3. a registration stored for this server under that same `client_id` is
* recorded as `cimd` — written by `ensureCimdClientRegistration`, which
* reaches that line only after confirming the AS advertises
* `client_id_metadata_document_supported`.
*
* RFC 7591 §3.2 makes a dynamically issued `client_id` opaque, so a client may
@ -338,6 +338,15 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
* to already hold a CIMD registration recorded under it. Anything else — a
* fresh DCR, a different id, CIMD switched off, no prior CIMD registration —
* falls through to `"dcr"`.
*
* ⚠️ (3) is deliberately **not** scoped to the incoming issuer alone. SEP-2352
* keys registrations per authorization server, so a resource that resolves to a
* second issuer legitimately has no record under it yet: the first binding
* promotes the unkeyed CIMD entry into issuer A's slot and clears the fallback,
* `ensureCimdClientRegistration` then early-returns on the ctx-less read, and
* the SDK's own CIMD branch saves under issuer B with nothing stored for B.
* Checking the issuer slot and then the server's active registration keeps that
* second issuer labeled CIMD (Copilot).
*/
private async resolveSdkRegistrationKind(
clientInformation: OAuthClientInformation,
@ -350,21 +359,26 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
) {
return "dcr";
}
// Reads through to the unkeyed slot our own pre-registration wrote, since
// the issuer slot does not exist yet on the save that creates it.
const stored = await this.storage.getClientInformation(
this.serverUrl,
false,
issuer,
);
if (stored?.client_id !== clientMetadataUrl) return "dcr";
const storedKind = await this.storage.getClientRegistrationKind(
this.serverUrl,
issuer,
);
// `"static"` lives in the preregistered slot, never this one, so `"cimd"`
// is the only kind worth carrying forward.
return storedKind === "cimd" ? "cimd" : "dcr";
// `undefined` resolves to the server's active issuer, falling back to the
// unkeyed slot our own pre-registration wrote — which is where the record
// still lives on the save that first binds an issuer.
const lookupKeys = issuer === undefined ? [undefined] : [issuer, undefined];
for (const key of lookupKeys) {
const stored = await this.storage.getClientInformation(
this.serverUrl,
false,
key,
);
if (stored?.client_id !== clientMetadataUrl) continue;
const storedKind = await this.storage.getClientRegistrationKind(
this.serverUrl,
key,
);
// `"static"` lives in the preregistered slot, never this one, so `"cimd"`
// is the only kind worth carrying forward.
if (storedKind === "cimd") return "cimd";
}
return "dcr";
}
async saveScope(scope: string | undefined): Promise<void> {