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 bccf5e4c..9b5e587d 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -357,6 +357,28 @@ describe("clearStoredAuthForRelogin", () => { } }); + // A plan reached with no budget left is reported rather than firing a + // request it would immediately abandon — and that report must outrank an + // earlier success, since the unattempted key's grant may still be live. + it("reports exhaustion for a key it never attempted", async () => { + seedBothSpellings("live-r", "stale-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 200 })); + try { + const outcome = await clearStoredAuthForRelogin("https://example.com", { + budgetMs: 0, + }); + expect(fetchSpy).not.toHaveBeenCalled(); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome?.status === "failed" ? outcome.detail : "").toContain( + "budget was exhausted", + ); + } finally { + fetchSpy.mockRestore(); + } + }); + it("reports a later failure over an earlier success", async () => { seedBothSpellings("live-r", "stale-r"); const fetchSpy = vi diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index a02c865e..6ec64e6e 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -123,10 +123,6 @@ async function sendPlans( // may still be live when the key held no grant at all — a false alarm, and // one that outranks the real outcome under the failure-first rule below. const needsNetwork = plan.outcome === undefined; - /* v8 ignore next 10 -- A bound, not a path: `withDeadline` fails any plan - that reaches the deadline, so a preceding plan cannot both succeed and - leave zero budget. This exists so a plan reached with nothing left is - reported rather than firing a request it would immediately abandon. */ if (needsNetwork && remainingMs <= 0) { // Overrides an earlier success rather than deferring to it: this key's // grant may still be live at the authorization server, and that is the diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index bb25547f..f4fc9c45 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1321,9 +1321,12 @@ function App() { const settingsModalIsStdio = settingsModalServerType === "stdio"; /** - * In-flight lock shared by both clear controls (#2144). See `runClear`. + * Servers whose clear is in flight (#2144). Keyed by id, not a single flag: + * the callback explicitly supports clearing a server other than the active + * one, so a global lock would silently drop B's click while A's revocation + * was still out. See `runClear`. */ - const clearOAuthInFlightRef = useRef(false); + const clearOAuthInFlightRef = useRef>(new Set()); /** * Run a clear from a key/click handler, which cannot await. @@ -1337,16 +1340,17 @@ function App() { const runClear = useCallback( (server: Parameters[0]) => { // Shared by BOTH clear controls (Connection Info and Server Settings), - // because they drive the same client and the same store entry. A ref - // rather than state: a double click delivers both events before React - // re-renders, so a state-based guard would let both through — and with - // revocation taking up to five seconds, that means concurrent RFC 7009 - // requests, concurrent store writes, and two contradictory toasts. - if (clearOAuthInFlightRef.current) return; - clearOAuthInFlightRef.current = true; + // because for one server they drive the same client and the same store + // entry. A ref rather than state: a double click delivers both events + // before React re-renders, so a state-based guard would let both through + // — and with revocation taking up to five seconds, that means concurrent + // RFC 7009 requests, concurrent store writes, and two contradictory + // toasts. Keyed by server so a *different* server's clear is unaffected. + if (clearOAuthInFlightRef.current.has(server.id)) return; + clearOAuthInFlightRef.current.add(server.id); clearServerOAuthAndDisconnect(server) .finally(() => { - clearOAuthInFlightRef.current = false; + clearOAuthInFlightRef.current.delete(server.id); }) .catch((err: unknown) => { notifications.show({ diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 13779d56..df532f0e 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -1304,7 +1304,13 @@ export function useOAuthRecovery({ finalizeExplicitDisconnect(); } } - } else if (!isActive) { + } else if (!isActive || stillTargetsActiveSession()) { + // No client to disconnect — either this is a stored-only clear, or the + // active session has none yet (it is being built or torn down). Either + // way the resume snapshot is stale and must go; skipping it would leave + // OAuth recovery state pointing at credentials that no longer exist. + // Guarded so a clear whose session has moved on still publishes + // nothing. clearOAuthResumeOnExplicitDisconnect(); } diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index 1abeeea1..d0dd7ae1 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -101,16 +101,21 @@ describe("revocationAuthMethods", () => { token_endpoint_auth_methods_supported: ["client_secret_post"], }), ), - ).toEqual([]); + ).toEqual(["client_secret_basic"]); }); - it("yields nothing when the revocation list is absent", () => { - expect(revocationAuthMethods(metadata())).toEqual([]); + // Named literally, not left empty. An empty list means "the metadata said + // nothing" to `selectClientAuthMethod`, which then honors whatever the + // client's own registration declares — so a client registered + // `client_secret_post` could put credentials in the body of a request to an + // endpoint that promised only Basic. (`OAuthClientInformation` does not carry + // that field, so this path cannot construct the case today; naming the + // default is what keeps it true if the type widens.) + it("names the RFC 8414 default when the revocation list is absent", () => { + expect(revocationAuthMethods(metadata())).toEqual(["client_secret_basic"]); }); - // The empty list is not "no authentication": it is what makes the SDK apply - // RFC 8414's actual default. - it("an empty list resolves to the RFC 8414 default", () => { + it("the default resolves to Basic for a confidential client", () => { const { init } = buildRevocationRequest({ endpoint: REVOKE_URL, token: "r", @@ -121,7 +126,7 @@ describe("revocationAuthMethods", () => { expect(headerOf(init, "Authorization")).toBe(`Basic ${btoa("cid:sec")}`); }); - it("an empty list leaves a public client unauthenticated", () => { + it("the default leaves a public client unauthenticated", () => { const { init } = buildRevocationRequest({ endpoint: REVOKE_URL, token: "r", diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index aa2bd473..8a438ccd 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -104,14 +104,20 @@ export function selectRevocableToken( * there send POST credentials to a revocation endpoint that never advertised * that method, which a strict server rejects. * - * An empty result is returned rather than a literal `["client_secret_basic"]` - * because that is what makes {@link selectClientAuthMethod} apply exactly the - * RFC's default — `client_secret_basic` when the client holds a secret, `none` - * when it does not — while still honoring a `token_endpoint_auth_method` the - * client's own registration declares. + * The default is returned **literally** rather than as an empty list. An empty + * list does not mean "apply the RFC default" to `selectClientAuthMethod`: it + * means "the metadata said nothing", and the SDK then honors whatever + * `token_endpoint_auth_method` the client's own registration declares. A DCR + * client registered `client_secret_post` would therefore put its credentials + * in the body of a request to an endpoint that promised only Basic, and the + * revocation would fail. Naming the default is what actually enforces it. */ export function revocationAuthMethods(metadata: CachedMetadata): string[] { - return metadata.revocation_endpoint_auth_methods_supported ?? []; + return ( + metadata.revocation_endpoint_auth_methods_supported ?? [ + "client_secret_basic", + ] + ); } export interface RevocationRequestParams {