diff --git a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts index 33133121..d067a2f1 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -68,9 +68,9 @@ describe("clearStoredAuthForRelogin", () => { expect(blob.servers["not a url"]).toBeUndefined(); }); - // #2144 — RFC 7009. The request has to go out *before* the delete, since it - // is built from the token, the client id and the cached metadata the delete - // removes. + // #2144 — RFC 7009. The request is *built* before the delete (from the token, + // the client id and the cached metadata it removes) and *sent* after it, so + // the local delete never waits on the network. describe("token revocation", () => { function seed(over: Record = {}): string { dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-revoke-")); diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index c992b763..1d9168c7 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -435,12 +435,41 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { expect(body(init!).get("token")).toBe("r"); }); - // A server configured with `oauth.clientId` stores its credentials in the - // preregistered slot, which is issuer-independent and is *not* what a plain - // `getClientInformation(serverUrl)` returns. Reading only the dynamic slot - // would send no client authentication at all for exactly the confidential - // clients most likely to require it. - it("authenticates with a preconfigured client, not just a dynamically registered one", async () => { + // A grant minted under DCR must be revoked with the registration that minted + // it, even after the server has since been switched to a configured + // `oauth.clientId`. RFC 7009 §2.2 answers 200 for a token the server does not + // recognise as the caller's, so using the wrong client reports `revoked` + // while the grant stays live — and the local record is already gone. + it("authenticates with the registration bound to the grant's issuer", async () => { + await seed(); + await storage.saveClientInformation( + SERVER_URL, + { client_id: "dcr-cid", client_secret: "dcr-sec" }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + // A configured client id was added later; it must NOT win here. + await storage.savePreregisteredClientInformation(SERVER_URL, { + client_id: "static-cid", + client_secret: "static-sec", + }); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }); + + const [, init] = fetchFn.mock.calls[0]!; + expect(headerOf(init!, "Authorization")).toBe( + `Basic ${btoa("dcr-cid:dcr-sec")}`, + ); + }); + + // The other direction: a token minted with the configured client leaves the + // issuer slot empty (the SDK writes it only after DCR), so the preregistered + // entry is the right fallback — and dropping it would send no client + // authentication at all for exactly the confidential clients most likely to + // require it. + it("falls back to a preconfigured client when the issuer slot has none", async () => { await seed(); await storage.savePreregisteredClientInformation(SERVER_URL, { client_id: "static-cid", diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index 4c96e6ef..3a6717e5 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -437,11 +437,28 @@ export async function executeOAuthRevocation( /** * The client credentials to authenticate the revocation request with. * - * Mirrors `BaseOAuthClientProvider.clientInformation`: the preregistered - * (static, issuer-independent) entry wins, then the registration bound to - * `issuer`. Reading only the second would silently drop client authentication - * for every server configured with an `oauth.clientId` — the confidential case, - * where an authorization server is most likely to *require* it and answer 401. + * Deliberately the **reverse** of `BaseOAuthClientProvider.clientInformation`, + * which prefers the preregistered (static) entry. That order answers "who + * should I authenticate as *now*"; revocation asks a different question — "who + * minted *this* token" — and the two diverge. The store lets a preregistered + * client and an issuer-bound dynamic registration coexist + * (`savePreregisteredClientInformation` does not clear the issuer slot), so + * after a server is switched from DCR to a configured `oauth.clientId`, an + * older DCR grant would be revoked with the configured client's credentials. + * RFC 7009 §2.2 makes a 200 the answer for a token the server does not + * recognise as the caller's, so that reports `revoked` while the grant stays + * live — the worst possible outcome, since the local record is gone. + * + * So the registration bound to this grant's `issuer` wins where the store has + * one, and the preregistered entry is the fallback. That covers both + * directions: a token minted with the configured client leaves the issuer slot + * empty (the SDK only writes it after DCR) and falls through correctly. + * + * This is best available evidence, not proof: the store records client + * information per issuer, not per token, so a server that re-registered + * dynamically under one issuer still cannot distinguish which registration + * minted which grant. Binding the client identity to the token at save time is + * the real fix and is a storage-shape change beyond this PR. */ async function resolveClientInformation( storage: OAuthStorage, @@ -449,8 +466,9 @@ async function resolveClientInformation( issuer?: string, ): Promise { return ( - (await storage.getClientInformation(serverUrl, true)) ?? - (await storage.getClientInformation(serverUrl, false, issuer)) + (issuer !== undefined + ? await storage.getClientInformation(serverUrl, false, issuer) + : undefined) ?? (await storage.getClientInformation(serverUrl, true)) ); }