fix: bind CIMD provenance to the issuer it was discovered for
Copilot review (#2287): the active-issuer fallback added last round could copy issuer A's CIMD provenance onto a real dynamic registration for issuer B, since RFC 7591 §3.2 permits B to mint the same opaque URL as its `client_id`. Fix it at the source rather than in the resolver. `ensureCimdClientRegistration` now runs discovery first and records the registration against the issuer it just discovered, having confirmed *that* AS advertises `client_id_metadata_document_supported`. Its "already registered?" check moves after discovery and is keyed by that issuer — read ctx-less it resolved through the active issuer and early-returned for every later one, which is what forced the cross-issuer fallback in the first place. `resolveSdkRegistrationKind` is therefore issuer-scoped again, with no fallback. A second AS behind one resource now gets its own determination: CIMD when it advertises CIMD, `dcr` otherwise — including when it mints the metadata URL as its own `client_id`. The cost is a discovery round trip per connect attempt rather than only the first; noted at the call site. 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
parent
f3f68c731b
commit
4c6a9fb376
4 changed files with 185 additions and 66 deletions
|
|
@ -62,12 +62,15 @@ describe("ensureCimdClientRegistration", () => {
|
|||
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(
|
||||
SERVER_URL,
|
||||
{
|
||||
client_id: METADATA_URL,
|
||||
},
|
||||
{ registrationKind: "cimd" },
|
||||
{ registrationKind: "cimd", issuer: "http://127.0.0.1:9999" },
|
||||
);
|
||||
});
|
||||
|
||||
|
|
@ -154,22 +157,52 @@ describe("ensureCimdClientRegistration", () => {
|
|||
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||
SERVER_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 () => {
|
||||
storage.getClientInformation = vi.fn(async () => ({
|
||||
client_id: "existing-client",
|
||||
}));
|
||||
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" },
|
||||
);
|
||||
|
||||
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);
|
||||
await ensureCimdClientRegistration({
|
||||
serverUrl: SERVER_URL,
|
||||
provider,
|
||||
fetchFn: vi.fn(),
|
||||
fetchFn,
|
||||
});
|
||||
|
||||
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",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ 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 {
|
||||
BrowserNavigation,
|
||||
BrowserOAuthClientProvider,
|
||||
|
|
@ -778,9 +779,12 @@ 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).
|
||||
// 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";
|
||||
|
||||
|
|
@ -792,38 +796,71 @@ describe("OAuthNavigation", () => {
|
|||
return new OAuthStorageBase(new OAuthMemoryStore(), backend);
|
||||
}
|
||||
|
||||
async function bindFirstIssuer(storage: OAuthStorage) {
|
||||
/**
|
||||
* 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) {
|
||||
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}`);
|
||||
}) as unknown as typeof fetch;
|
||||
}
|
||||
|
||||
/** 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,
|
||||
});
|
||||
// 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 ensureCimdClientRegistration({
|
||||
serverUrl: SERVER,
|
||||
provider,
|
||||
fetchFn: discoveryFetch(ISSUER, true),
|
||||
});
|
||||
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();
|
||||
return provider;
|
||||
}
|
||||
|
||||
// The SDK's own CIMD branch, saving under the second issuer.
|
||||
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 provider.saveClientInformation(
|
||||
{ client_id: METADATA_URL },
|
||||
{ issuer: ISSUER_B },
|
||||
|
|
@ -837,9 +874,35 @@ describe("OAuthNavigation", () => {
|
|||
).toBe("cimd");
|
||||
});
|
||||
|
||||
it("still records dcr for a second issuer that mints its own client_id", async () => {
|
||||
it("records dcr when a second issuer without CIMD mints the same URL as its client_id", async () => {
|
||||
const storage = makeRealStorage();
|
||||
const provider = await bindFirstIssuer(storage);
|
||||
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),
|
||||
});
|
||||
// ...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");
|
||||
});
|
||||
|
||||
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" },
|
||||
|
|
|
|||
|
|
@ -27,9 +27,6 @@ export async function ensureCimdClientRegistration(params: {
|
|||
const clientMetadataUrl = params.provider.clientMetadataUrl?.trim();
|
||||
if (!clientMetadataUrl) return;
|
||||
|
||||
const existing = await params.provider.clientInformation();
|
||||
if (existing?.client_id) return;
|
||||
|
||||
let resourceMetadata;
|
||||
try {
|
||||
resourceMetadata = await discoverOAuthProtectedResourceMetadata(
|
||||
|
|
@ -56,10 +53,37 @@ export async function ensureCimdClientRegistration(params: {
|
|||
);
|
||||
if (!metadata?.client_id_metadata_document_supported) return;
|
||||
|
||||
// SEP-2352 keys a registration to the authorization server that issued it, so
|
||||
// the record this writes is bound to the issuer we just discovered rather than
|
||||
// to the server as a whole. That binding is what makes the provenance
|
||||
// trustworthy later: `BaseOAuthClientProvider.saveClientInformation` preserves
|
||||
// `cimd` only for an issuer this function recorded it for, having first
|
||||
// confirmed *that* AS advertises `client_id_metadata_document_supported`
|
||||
// (#2242, Copilot). A second AS behind the same resource therefore gets its own
|
||||
// determination — pre-registered here when it too supports CIMD, and left to
|
||||
// dynamic registration when it does not.
|
||||
//
|
||||
// ⚠️ This is why the "do we already have a client?" check below sits *after*
|
||||
// discovery rather than short-circuiting it, at the cost of a discovery round
|
||||
// trip on each connect attempt rather than only the first. Read ctx-less — as
|
||||
// it was — it resolves through the *active* issuer and so early-returns for
|
||||
// every subsequent issuer, leaving them with no CIMD record at all. It still
|
||||
// answers the static case first, since `clientInformation` checks the
|
||||
// preregistered slot before any issuer slot.
|
||||
const issuer = metadata.issuer;
|
||||
const existing = await params.provider.clientInformation(
|
||||
issuer ? { issuer } : undefined,
|
||||
);
|
||||
if (existing?.client_id) return;
|
||||
|
||||
const clientInformation: OAuthClientInformation = {
|
||||
client_id: clientMetadataUrl,
|
||||
};
|
||||
await params.provider.saveClientInformation(clientInformation, {
|
||||
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 }),
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -326,27 +326,33 @@ 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. 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
|
||||
* 3. the registration stored **for this issuer** under that same `client_id`
|
||||
* is recorded as `cimd` — written by `ensureCimdClientRegistration`, which
|
||||
* reaches that line only after confirming *that* AS advertises
|
||||
* `client_id_metadata_document_supported`.
|
||||
*
|
||||
* RFC 7591 §3.2 makes a dynamically issued `client_id` opaque, so a client may
|
||||
* not assume its format — which is why (2) is not load-bearing on its own. For
|
||||
* a `registerClient` result to be mislabeled here, the AS would have to mint an
|
||||
* identifier byte-identical to the HTTPS URL we configured *and* we would have
|
||||
* to already hold a CIMD registration recorded under it. Anything else — a
|
||||
* fresh DCR, a different id, CIMD switched off, no prior CIMD registration —
|
||||
* identifier byte-identical to the HTTPS URL we configured *and* be an AS we
|
||||
* had already recorded a CIMD registration for — that is, one that advertises
|
||||
* CIMD and then dynamically registers anyway. Anything else — a fresh DCR, a
|
||||
* different id, CIMD switched off, no prior CIMD registration for this issuer —
|
||||
* 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).
|
||||
* ⚠️ (3) is scoped to the issuer on purpose, and the lookup deliberately does
|
||||
* **not** fall back to the server's active issuer. SEP-2352 keys registrations
|
||||
* per AS, so a second AS behind the same resource is a separate determination:
|
||||
* it may well not support CIMD and register dynamically, and RFC 7591 permits
|
||||
* it to mint the very URL the first AS uses as a CIMD `client_id` (Copilot).
|
||||
* `ensureCimdClientRegistration` binds the record to the issuer it discovered,
|
||||
* which is what lets this stay issuer-scoped without losing a genuine
|
||||
* second-issuer CIMD registration.
|
||||
*
|
||||
* The read is still issuer-*keyed* rather than issuer-*only*: `getClientInformation`
|
||||
* falls back to the unkeyed slot when no `byIssuer` entry exists, which is how a
|
||||
* pre-registration written before an issuer was known is still found on the save
|
||||
* that first binds one.
|
||||
*/
|
||||
private async resolveSdkRegistrationKind(
|
||||
clientInformation: OAuthClientInformation,
|
||||
|
|
@ -359,26 +365,19 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
|||
) {
|
||||
return "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";
|
||||
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";
|
||||
}
|
||||
|
||||
async saveScope(scope: string | undefined): Promise<void> {
|
||||
|
|
|
|||
Loading…
Reference in a new issue