fix: read the SDK's own discovery state instead of a marker of our own
Copilot review (#2287), two findings — and taken together they said the marker was the wrong mechanism, so this removes it rather than patching it again: - A transient failure in our CIMD preflight recorded no marker, so a *successful* SDK fallback discovery that advertised CIMD was still stored as `dcr`. - `existing` and `existingKind` could describe different registrations, because `clientInformation()` answers preregistered-first while `clientRegistrationKind()` prefers the issuer's dynamic slot. SDK v2 `auth()` persists the authorization-server metadata via `saveDiscoveryState` *before* it reads or writes client information, and it takes its URL-based-client-ID branch — rather than `registerClient` — exactly when that metadata advertises `client_id_metadata_document_supported` and a `clientMetadataUrl` is configured. So the branch is not inferred at all now: it is read back from the state the SDK itself just wrote. `resolveSdkRegistrationKind` becomes two cases. A save for an issuer that already has a registration under this `client_id` is a back-stamp, answered by that registration's recorded kind — so an existing DCR on the metadata URL stays `dcr`. A save for an issuer with none is a new registration, answered by the discovery state, guarded on the metadata describing that same issuer. This drops `cimdClientMetadataUrl` and its storage accessors entirely, and restores `ensureCimdClientRegistration` to a plain issuer-bound pre-registration. Invalid-client recovery, second-issuer CIMD, second-issuer DCR minting the metadata URL, and preflight failure are all consequences of the two cases rather than separate mechanisms. 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
abb6a47f7d
commit
6273790077
5 changed files with 183 additions and 306 deletions
|
|
@ -68,50 +68,19 @@ export async function ensureCimdClientRegistration(params: {
|
|||
}
|
||||
|
||||
const issuer = metadata?.issuer;
|
||||
const supportsCimd = metadata?.client_id_metadata_document_supported === true;
|
||||
|
||||
/**
|
||||
* The marker records that *this* AS accepts this URL as a `client_id` **and**
|
||||
* that the registration standing for it got there through CIMD. It is written
|
||||
* only where both are established, and actively withdrawn otherwise, so it
|
||||
* cannot go stale: discovery runs on every connect.
|
||||
*/
|
||||
const setMarker = async (url: string | undefined) => {
|
||||
if (issuer) await params.provider.saveCimdClientMetadataUrl(issuer, url);
|
||||
};
|
||||
|
||||
if (!supportsCimd) {
|
||||
// Withdrawn, not merely left alone — an AS that stops advertising CIMD
|
||||
// stops being treated as one.
|
||||
await setMarker(undefined);
|
||||
return;
|
||||
}
|
||||
if (!metadata?.client_id_metadata_document_supported) return;
|
||||
|
||||
// ⚠️ Keyed by the issuer just resolved, not read ctx-less. A ctx-less read
|
||||
// resolves through the *active* issuer, so it early-returns for every
|
||||
// subsequent issuer and leaves them with no CIMD record at all. It answers the
|
||||
// static case first, since `clientInformation` checks the preregistered slot
|
||||
// before any issuer slot.
|
||||
// subsequent issuer and leaves them with no CIMD registration at all. It
|
||||
// answers the static case first, since `clientInformation` checks the
|
||||
// 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) {
|
||||
// Something is already registered for this AS, so this call establishes
|
||||
// nothing — and AS support for CIMD is not on its own evidence that *that*
|
||||
// registration is a CIMD one. RFC 7591 §3.2 leaves a dynamically issued
|
||||
// `client_id` opaque, so an existing DCR may carry this very URL; marking it
|
||||
// would relabel a real dynamic registration (Copilot). Reaffirm the marker
|
||||
// only for a registration already recorded as `cimd` under this exact URL,
|
||||
// and withdraw it otherwise — which also covers a static client.
|
||||
const existingKind = await params.provider.clientRegistrationKind(issuer);
|
||||
const isCimdRegistration =
|
||||
existingKind === "cimd" && existing.client_id === clientMetadataUrl;
|
||||
await setMarker(isCimdRegistration ? clientMetadataUrl : undefined);
|
||||
return;
|
||||
}
|
||||
if (existing?.client_id) return;
|
||||
|
||||
// From here this call *is* the CIMD registration, so the marker is earned.
|
||||
await setMarker(clientMetadataUrl);
|
||||
await params.provider.saveClientInformation(
|
||||
{ client_id: clientMetadataUrl },
|
||||
{
|
||||
|
|
|
|||
|
|
@ -186,34 +186,6 @@ export class OAuthStorageBase implements OAuthStorage {
|
|||
);
|
||||
}
|
||||
|
||||
async getCimdClientMetadataUrl(
|
||||
serverUrl: string,
|
||||
issuer?: string,
|
||||
): Promise<string | undefined> {
|
||||
await this.ensureLoaded();
|
||||
const state = this.memory.getState().getServerState(serverUrl);
|
||||
return this.issuerSlot(state, issuer)?.cimdClientMetadataUrl;
|
||||
}
|
||||
|
||||
async saveCimdClientMetadataUrl(
|
||||
serverUrl: string,
|
||||
issuer: string,
|
||||
clientMetadataUrl: string | undefined,
|
||||
): Promise<void> {
|
||||
await this.ensureLoaded();
|
||||
// Not a save of credentials, so it must not promote this issuer to
|
||||
// `activeIssuer` — the marker is written during discovery, before anything
|
||||
// has been authorized against this AS.
|
||||
this.updateIssuerSlot(
|
||||
serverUrl,
|
||||
issuer,
|
||||
{ cimdClientMetadataUrl: clientMetadataUrl },
|
||||
{},
|
||||
false,
|
||||
);
|
||||
await this.persist();
|
||||
}
|
||||
|
||||
async saveClientInformation(
|
||||
serverUrl: string,
|
||||
clientInformation: OAuthClientInformation,
|
||||
|
|
|
|||
|
|
@ -9,11 +9,7 @@ import type {
|
|||
OAuthMetadata,
|
||||
OAuthDiscoveryState,
|
||||
} from "@modelcontextprotocol/client";
|
||||
import type {
|
||||
OAuthStorage,
|
||||
SaveClientInformationOptions,
|
||||
OAuthClientRegistrationKind,
|
||||
} from "./storage.js";
|
||||
import type { OAuthStorage, SaveClientInformationOptions } from "./storage.js";
|
||||
import { generateOAuthState } from "./utils.js";
|
||||
import { applyAuthorizationParams } from "./authorizationParams.js";
|
||||
import { scopeForDeclinedRefreshGrant } from "./scopes.js";
|
||||
|
|
@ -327,36 +323,40 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
|||
* save as DCR is what relabeled a CIMD registration `Dynamic (DCR)` in
|
||||
* Connection Info the moment the SDK bound it to an issuer (#2242).
|
||||
*
|
||||
* The claim is deliberately narrow — three conditions must all hold, and the
|
||||
* decisive one is a fact *we recorded about this authorization server*, not an
|
||||
* inference about what it returned:
|
||||
* Two cases reach here, and they are told apart by whether a registration
|
||||
* already exists for this issuer:
|
||||
*
|
||||
* 1. CIMD is configured for this connection right now, and
|
||||
* 2. the incoming `client_id` is exactly that metadata-document URL, and
|
||||
* 3. `ensureCimdClientRegistration` recorded that same URL as the CIMD marker
|
||||
* **for this issuer**, having read `client_id_metadata_document_supported`
|
||||
* from *that* AS's own metadata.
|
||||
* - **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.
|
||||
*
|
||||
* 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* be an AS that
|
||||
* currently advertises CIMD and dynamically registered anyway. Anything else —
|
||||
* a fresh DCR, a different id, CIMD switched off, an AS that does not advertise
|
||||
* CIMD — falls through to `"dcr"`.
|
||||
* 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).
|
||||
*
|
||||
* ⚠️ (3) reads the **marker**, not the stored registration kind, and the two
|
||||
* differ in exactly one place that matters: `invalidateCredentials("client")`
|
||||
* clears the credential and its kind, and SDK v2 `auth()` calls it on an
|
||||
* `invalid_client` / `unauthorized_client` response before retrying. The
|
||||
* retry's URL-based client-ID save would then find no kind and be recorded as
|
||||
* DCR. The marker describes the AS rather than the credential, so it survives
|
||||
* that clear (#2242, Copilot).
|
||||
* Consequences worth stating, since each was a defect on the way here:
|
||||
*
|
||||
* ⚠️ (3) is issuer-scoped with no fallback to the server's active issuer. A
|
||||
* second AS behind one resource is a separate determination: it may not support
|
||||
* CIMD and may register dynamically, and RFC 7591 permits it to mint the very
|
||||
* URL the first AS uses as its CIMD `client_id`.
|
||||
* - 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,
|
||||
|
|
@ -369,35 +369,32 @@ export class BaseOAuthClientProvider implements OAuthClientProvider {
|
|||
) {
|
||||
return "dcr";
|
||||
}
|
||||
const marker = await this.storage.getCimdClientMetadataUrl(
|
||||
|
||||
// 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,
|
||||
);
|
||||
return marker === clientMetadataUrl ? "cimd" : "dcr";
|
||||
}
|
||||
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";
|
||||
}
|
||||
|
||||
/** @see OAuthStorage.getCimdClientMetadataUrl */
|
||||
async cimdClientMetadataUrl(issuer?: string): Promise<string | undefined> {
|
||||
return await this.storage.getCimdClientMetadataUrl(this.serverUrl, issuer);
|
||||
}
|
||||
|
||||
/** @see OAuthStorage.getClientRegistrationKind */
|
||||
async clientRegistrationKind(
|
||||
issuer?: string,
|
||||
): Promise<OAuthClientRegistrationKind | undefined> {
|
||||
return await this.storage.getClientRegistrationKind(this.serverUrl, issuer);
|
||||
}
|
||||
|
||||
/** @see OAuthStorage.saveCimdClientMetadataUrl */
|
||||
async saveCimdClientMetadataUrl(
|
||||
issuer: string,
|
||||
clientMetadataUrl: string | undefined,
|
||||
): Promise<void> {
|
||||
await this.storage.saveCimdClientMetadataUrl(
|
||||
this.serverUrl,
|
||||
issuer,
|
||||
clientMetadataUrl,
|
||||
);
|
||||
// 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> {
|
||||
|
|
|
|||
|
|
@ -81,23 +81,6 @@ export interface OAuthStorage {
|
|||
issuer?: string,
|
||||
): Promise<OAuthClientRegistrationKind | undefined>;
|
||||
|
||||
/**
|
||||
* The CIMD client-metadata URL this authorization server was confirmed to
|
||||
* accept as a `client_id`. Survives {@link clearClientInformation}, because it
|
||||
* records a property of the AS rather than a credential (#2242).
|
||||
*/
|
||||
getCimdClientMetadataUrl(
|
||||
serverUrl: string,
|
||||
issuer?: string,
|
||||
): Promise<string | undefined>;
|
||||
|
||||
/** Write (or, with `undefined`, clear) the marker above for one issuer. */
|
||||
saveCimdClientMetadataUrl(
|
||||
serverUrl: string,
|
||||
issuer: string,
|
||||
clientMetadataUrl: string | undefined,
|
||||
): Promise<void>;
|
||||
|
||||
/**
|
||||
* Save client information (dynamically registered)
|
||||
*/
|
||||
|
|
|
|||
|
|
@ -30,21 +30,6 @@ export interface IssuerBoundOAuthState {
|
|||
/** Set when {@link clientInformation} is saved — DCR vs CIMD. */
|
||||
clientRegistrationKind?: OAuthClientRegistrationKind;
|
||||
tokens?: OAuthTokens;
|
||||
/**
|
||||
* The CIMD client-metadata URL this AS was confirmed to accept as a `client_id`
|
||||
* — written by `ensureCimdClientRegistration` after reading
|
||||
* `client_id_metadata_document_supported` from *this* issuer's metadata, and
|
||||
* refreshed (or cleared) on every connect because that check now runs each time.
|
||||
*
|
||||
* Deliberately **not** a credential, and so deliberately **not** cleared by
|
||||
* {@link OAuthStorage.clearClientInformation}. It records a property of the
|
||||
* authorization server and our own configuration, which an `invalid_client`
|
||||
* response says nothing about: SDK v2 `auth()` answers that error by calling
|
||||
* `invalidateCredentials("client")` and retrying, and the retry's URL-based
|
||||
* client-ID save would otherwise land with no provenance and be recorded as
|
||||
* DCR (#2242, Copilot).
|
||||
*/
|
||||
cimdClientMetadataUrl?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Reference in a new issue