fix(auth): revoke with the registration bound to the grant, not the current one (#2144)

Copilot review round 17.

`resolveClientInformation` mirrored `BaseOAuthClientProvider`, preferring the
preregistered entry. That order answers "who should I authenticate as now";
revocation asks "who minted this token", and the two diverge — the store lets a
preregistered client and an issuer-bound dynamic registration coexist, so after
a server is switched from DCR to a configured `oauth.clientId` an older DCR
grant was revoked with the configured client's credentials. RFC 7009 §2.2
answers 200 for a token the server does not recognise as the caller's, so that
reported `revoked` while the grant stayed live and the local record was already
gone — the worst available outcome.

The registration bound to the grant's issuer now wins, with the preregistered
entry as the fallback, which covers both directions: a token minted with the
configured client leaves the issuer slot empty, since the SDK writes it only
after DCR.

Stated in the comment rather than implied: this is best available evidence, not
proof. The store records client information per issuer, not per token, so
binding identity at save time remains the real fix and is a storage-shape
change beyond this PR.

Also fixed a CLI test comment still describing the pre-round-13 ordering.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>

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, unknown> = {}): string {
       dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-revoke-"));

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<typeof fetch>(
+      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",

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<OAuthClientInformation | undefined> {
   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))
   );
 }