fix: preserve CIMD registration provenance across SDK issuer binding (#2242)
`BaseOAuthClientProvider.saveClientInformation` treated every save without an explicit `registrationKind` as DCR. The SDK reaches it from three places in `auth()` and only one is a dynamic registration, so the issuer-binding write overwrote the `cimd` provenance our own pre-registration had just stored — and Connection Info reported `Dynamic (DCR)` for a connection that never issued a `POST /oauth/register`. Recover the kind instead: a `client_id` equal to the configured `clientMetadataUrl` is CIMD (which also covers the SDK's own CIMD write), and otherwise a stored registration with the same `client_id` carries its recorded kind forward. Matching on `client_id` keeps stale CIMD provenance from leaking onto a later DCR registration for the same server. 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:
parent
aab6475527
commit
4789cb954e
3 changed files with 204 additions and 4 deletions
|
|
@ -216,6 +216,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 +239,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 +628,147 @@ 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";
|
||||||
|
|
||||||
|
it("keeps cimd when the client_id is the configured metadata document URL", async () => {
|
||||||
|
const storage = makeStorage();
|
||||||
|
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("still records dcr for a server-minted client_id while CIMD is configured", async () => {
|
||||||
|
const storage = makeStorage();
|
||||||
|
const provider = makeProvider(storage, vi.fn(), {
|
||||||
|
clientMetadataUrl: METADATA_URL,
|
||||||
|
});
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: "dcr-minted-id" },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: "dcr-minted-id" },
|
||||||
|
{ registrationKind: "dcr", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("preserves the stored kind when the stored client_id matches", 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(
|
||||||
|
"cimd",
|
||||||
|
);
|
||||||
|
// No `clientMetadataUrl` on the provider — the provenance comes from
|
||||||
|
// storage alone, so a config cleared since the registration was made
|
||||||
|
// does not silently demote it.
|
||||||
|
const provider = makeProvider(storage);
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.getClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
false,
|
||||||
|
ISSUER,
|
||||||
|
);
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: METADATA_URL },
|
||||||
|
{ registrationKind: "cimd", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not leak a stored cimd kind onto a different client_id", 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(
|
||||||
|
"cimd",
|
||||||
|
);
|
||||||
|
const provider = makeProvider(storage);
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: "freshly-registered" },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.getClientRegistrationKind).not.toHaveBeenCalled();
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: "freshly-registered" },
|
||||||
|
{ registrationKind: "dcr", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("falls back to dcr when storage has a matching id but no recorded kind", async () => {
|
||||||
|
const storage = makeStorage();
|
||||||
|
vi.mocked(storage.getClientInformation).mockImplementation(
|
||||||
|
async (_url: string, preregistered?: boolean) =>
|
||||||
|
preregistered ? undefined : { client_id: "legacy-id" },
|
||||||
|
);
|
||||||
|
const provider = makeProvider(storage);
|
||||||
|
|
||||||
|
await provider.saveClientInformation(
|
||||||
|
{ client_id: "legacy-id" },
|
||||||
|
{ issuer: ISSUER },
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||||
|
SERVER,
|
||||||
|
{ client_id: "legacy-id" },
|
||||||
|
{ registrationKind: "dcr", issuer: ISSUER },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("an explicit registrationKind wins and consults no storage reads", async () => {
|
||||||
|
const storage = makeStorage();
|
||||||
|
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,15 @@ 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.
|
||||||
|
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 () => {
|
||||||
|
|
|
||||||
|
|
@ -294,15 +294,15 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
||||||
// `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 no kind — `resolveSdkRegistrationKind` recovers it —
|
||||||
// the registration kind and no issuer yet.
|
// while our callers supply the registration kind and no issuer yet.
|
||||||
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 +313,53 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Resolve the registration kind for a save that carries no explicit one — that
|
||||||
|
* is, one the SDK made. Three SDK call sites reach here: back-stamping an
|
||||||
|
* existing registration with its `issuer`, the SDK's own CIMD write (whose
|
||||||
|
* `client_id` *is* the metadata-document URL), and a real dynamic
|
||||||
|
* registration. Only the last is `"dcr"`, so defaulting every unstamped save
|
||||||
|
* to it relabels a CIMD registration `Dynamic (DCR)` in Connection Info the
|
||||||
|
* moment the SDK binds it to an issuer — reported as #2242, where no
|
||||||
|
* `POST /register` was ever made.
|
||||||
|
*
|
||||||
|
* `client_id` is what tells the cases apart, so match on it rather than on the
|
||||||
|
* stored kind alone: a DCR `client_id` is minted by the authorization server,
|
||||||
|
* so a later DCR registration for the same server cannot inherit the earlier
|
||||||
|
* CIMD provenance.
|
||||||
|
*/
|
||||||
|
private async resolveSdkRegistrationKind(
|
||||||
|
clientInformation: OAuthClientInformation,
|
||||||
|
issuer: string | undefined,
|
||||||
|
): Promise<SaveClientInformationOptions["registrationKind"]> {
|
||||||
|
const clientMetadataUrl = this.clientMetadataUrl?.trim();
|
||||||
|
if (
|
||||||
|
clientMetadataUrl &&
|
||||||
|
clientInformation.client_id === clientMetadataUrl
|
||||||
|
) {
|
||||||
|
return "cimd";
|
||||||
|
}
|
||||||
|
// Falls back to the unkeyed slot our own pre-registration wrote, since the
|
||||||
|
// issuer slot does not exist yet on the save that creates it. Reading it
|
||||||
|
// covers a CIMD registration whose `clientMetadataUrl` config has since
|
||||||
|
// been cleared, so the provenance is not silently demoted.
|
||||||
|
const stored = await this.storage.getClientInformation(
|
||||||
|
this.serverUrl,
|
||||||
|
false,
|
||||||
|
issuer,
|
||||||
|
);
|
||||||
|
if (!stored || stored.client_id !== clientInformation.client_id) {
|
||||||
|
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> {
|
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