diff --git a/AGENTS.md b/AGENTS.md index d31f9d85..82863633 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -103,18 +103,24 @@ v2/main/ │ │ # token request is made against — #2110; │ │ # revocation.ts RFC 7009 token revocation — │ │ # the request the three clear paths send. -│ │ # TWO halves — planOAuthRevocation then -│ │ # executeOAuthRevocation — and the order -│ │ # between them is the contract: plan → -│ │ # storage.clear() → execute. The snapshot -│ │ # must precede the clear (the token, the -│ │ # client credentials and the discovered -│ │ # `revocation_endpoint` all live in the store -│ │ # it empties); the CLEAR must precede the -│ │ # network, or a fresh authorization +│ │ # TWO halves — clearAndPlanRevocation then +│ │ # executeOAuthRevocation — and the split is +│ │ # the contract. The first TAKES the stored +│ │ # state and CLEARS it in one atomic storage +│ │ # step (OAuthStorage.takeRevocationSnapshot); +│ │ # the second sends from what was taken. +│ │ # There is no separate `storage.clear()` in +│ │ # any caller, deliberately — the clear cannot +│ │ # be forgotten or reordered. Both halves of +│ │ # that matter: the requests need state the +│ │ # clear destroys, and the clear must not wait +│ │ # on the network, or a fresh authorization │ │ # completing during a 5s request is deleted │ │ # by a clear reasoning about the grant it -│ │ # replaced. Names the +│ │ # replaced. NOT cross-process atomic — no +│ │ # OAuth-store mutation takes a lock, and +│ │ # `clear` alone always had that property. +│ │ # Names the │ │ # REFRESH token when there is one (§2.1 asks │ │ # the AS to invalidate the access tokens │ │ # under the same grant, so one request 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 d067a2f1..bccf5e4c 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -333,6 +333,30 @@ describe("clearStoredAuthForRelogin", () => { // A grant still live at the authorization server is what the user needs to // hear about; reporting the earlier success would print no warning at all. + // One budget across both keys, so a first request that outlives it fails + // rather than each key getting a fresh five seconds. + it("bounds both keys with one shared budget", async () => { + seedBothSpellings("live-r", "stale-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockImplementation(async () => { + // Outlives the injected budget, so the second key is never attempted. + await new Promise((r) => setTimeout(r, 40)); + return new Response(null, { status: 200 }); + }); + try { + const outcome = await clearStoredAuthForRelogin("https://example.com", { + budgetMs: 20, + }); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome?.status === "failed" ? outcome.detail : "").toContain( + "timed out after 20ms", + ); + } 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 12b17725..a02c865e 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -32,7 +32,15 @@ function normalizeServerUrl(serverUrl: string): string { */ export async function clearStoredAuthForRelogin( serverUrl: string | undefined, - options?: { revoke?: boolean }, + options?: { + revoke?: boolean; + /** + * Total wall-clock budget for the revocation requests across both key + * spellings. Defaults to {@link DEFAULT_REVOCATION_TIMEOUT_MS}; injectable + * so the exhaustion path can be exercised without a five-second test. + */ + budgetMs?: number; + }, ): Promise { if (!serverUrl?.trim()) return undefined; const raw = serverUrl.trim(); @@ -68,7 +76,9 @@ export async function clearStoredAuthForRelogin( // entry from the NodeOAuthStorage cache. resetNodeOAuthStorageCache(); - return options?.revoke === false ? undefined : sendPlans(plans); + return options?.revoke === false + ? undefined + : sendPlans(plans, options?.budgetMs ?? DEFAULT_REVOCATION_TIMEOUT_MS); } /** @@ -100,9 +110,10 @@ export async function clearStoredAuthForRelogin( */ async function sendPlans( plans: OAuthRevocationPlan[], + budgetMs: number, ): Promise { const fetchFn = createProxyFetch() ?? fetch; - const deadlineAt = Date.now() + DEFAULT_REVOCATION_TIMEOUT_MS; + const deadlineAt = Date.now() + budgetMs; let reported: TokenRevocationOutcome | undefined; let lastSkip: TokenRevocationOutcome | undefined; for (const plan of plans) { @@ -112,11 +123,19 @@ 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) { - reported ??= { + // 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 + // thing the user needs to hear about. Same failure-first rule as below. + const exhausted: TokenRevocationOutcome = { status: "failed", - detail: `the ${DEFAULT_REVOCATION_TIMEOUT_MS}ms revocation budget was exhausted before "${plan.serverUrl}" was attempted`, + detail: `the ${budgetMs}ms revocation budget was exhausted before "${plan.serverUrl}" was attempted`, }; + if (reported?.status !== "failed") reported = exhausted; continue; } const outcome = await executeOAuthRevocation(plan, { diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index 888c9637..1abeeea1 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -932,6 +932,54 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { expect(outcome).toMatchObject({ status: "failed" }); }); + // The preconfigured registration is only a *fallback*, so a malformed one + // must not abort grants that carry their own valid credentials. + it("still revokes when the preconfigured client registration is malformed", async () => { + stubSnapshot(storage, { + byIssuer: { + "https://as.example.com": { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r-good", + }, + clientInformation: { client_id: "dcr-cid" }, + }, + }, + // Not a valid `OAuthClientInformation` — no `client_id`. + preregisteredClientInformation: { client_secret: "orphan" }, + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: REVOKE_URL, + response_types_supported: ["code"], + }, + }); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + // The grant was still revoked, with its OWN registration... + expect(fetchFn).toHaveBeenCalledTimes(1); + expect( + new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get( + "client_id", + ), + ).toBe("dcr-cid"); + // ...and the malformed fallback is reported rather than swallowed. + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "preconfigured client registration", + ); + }); + // A corrupt slot must not abandon the grants that are still revocable — the // state is gone either way, so the failure has to be reported BESIDE the // successes rather than instead of them. diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index ae36917d..aa2bd473 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -438,7 +438,7 @@ interface CachedMetadata { } /** - * Send the requests a {@link planOAuthRevocation} snapshot describes. + * Send the requests a {@link clearAndPlanRevocation} snapshot describes. * * Safe to run **after** `storage.clear(serverUrl)` — that is the point: the * plan already holds the tokens, the credentials and the endpoint, so nothing @@ -539,9 +539,20 @@ async function collectGrants( const failures: TokenRevocationOutcome[] = []; const seenKeys = new Set(); - const preregistered = await parseClient( - snapshot.preregisteredClientInformation, - ); + // Parsed defensively: it is only a *fallback*, so a malformed preconfigured + // registration must not abort grants that carry their own valid credentials. + // Recorded and skipped, matching the per-slot salvage below. + let preregistered: OAuthClientInformation | undefined; + try { + preregistered = await parseClient(snapshot.preregisteredClientInformation); + } catch (err) { + failures.push({ + status: "failed", + detail: `could not read the preconfigured client registration: ${ + err instanceof Error ? err.message : String(err) + }`, + }); + } const add = async ( issuer: string | undefined, @@ -643,7 +654,7 @@ async function runPlan( ): Promise { if (plan.outcome) return plan.outcome; const endpoint = plan.endpoint; - /* v8 ignore next -- `planOAuthRevocation` always sets `outcome` when it sets + /* v8 ignore next -- `clearAndPlanRevocation` always sets `outcome` when it sets no endpoint, so this is unreachable; the guard exists to narrow the type. */ if (!endpoint) return { status: "skipped", reason: "no_endpoint" };