Skip to content

Commit 579a3ee

Browse files
cliffhallclaude
andcommitted
fix(auth): salvage a malformed fallback registration, and correct the stale API names (#2144)
Copilot review round 19. - Parsing the preconfigured client registration sat outside the per-slot salvage, so one malformed fallback aborted the whole plan and grants carrying their own valid credentials were never revoked. It is a *fallback*; a bad one is now recorded and skipped, matching every other slot. - `sendPlans` preserved an earlier success when a later key was never attempted, contradicting the failure-first rule two lines below — the unattempted key's grant may still be live. That branch turns out to be a bound rather than a path (`withDeadline` fails any plan that reaches the deadline, so a preceding plan cannot both succeed and leave zero budget), so it carries a justified `v8 ignore` saying exactly that. - `clearStoredAuthForRelogin` takes an optional `budgetMs`, which is what lets the shared-budget behavior be tested without a five-second test. - Three stale references to `planOAuthRevocation`, which no longer exists — two JSDoc links and the `AGENTS.md` entry, which additionally still told callers to make a separate `storage.clear()` call. Following that text would have reintroduced the second clear the atomic step exists to remove. 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>
1 parent 463227c commit 579a3ee

5 files changed

Lines changed: 128 additions & 20 deletions

File tree

‎AGENTS.md‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -103,18 +103,24 @@ v2/main/
103103
│ │ # token request is made against — #2110;
104104
│ │ # revocation.ts RFC 7009 token revocation —
105105
│ │ # the request the three clear paths send.
106-
│ │ # TWO halves — planOAuthRevocation then
107-
│ │ # executeOAuthRevocation — and the order
108-
│ │ # between them is the contract: plan →
109-
│ │ # storage.clear() → execute. The snapshot
110-
│ │ # must precede the clear (the token, the
111-
│ │ # client credentials and the discovered
112-
│ │ # `revocation_endpoint` all live in the store
113-
│ │ # it empties); the CLEAR must precede the
114-
│ │ # network, or a fresh authorization
106+
│ │ # TWO halves — clearAndPlanRevocation then
107+
│ │ # executeOAuthRevocation — and the split is
108+
│ │ # the contract. The first TAKES the stored
109+
│ │ # state and CLEARS it in one atomic storage
110+
│ │ # step (OAuthStorage.takeRevocationSnapshot);
111+
│ │ # the second sends from what was taken.
112+
│ │ # There is no separate `storage.clear()` in
113+
│ │ # any caller, deliberately — the clear cannot
114+
│ │ # be forgotten or reordered. Both halves of
115+
│ │ # that matter: the requests need state the
116+
│ │ # clear destroys, and the clear must not wait
117+
│ │ # on the network, or a fresh authorization
115118
│ │ # completing during a 5s request is deleted
116119
│ │ # by a clear reasoning about the grant it
117-
│ │ # replaced. Names the
120+
│ │ # replaced. NOT cross-process atomic — no
121+
│ │ # OAuth-store mutation takes a lock, and
122+
│ │ # `clear` alone always had that property.
123+
│ │ # Names the
118124
│ │ # REFRESH token when there is one (§2.1 asks
119125
│ │ # the AS to invalidate the access tokens
120126
│ │ # under the same grant, so one request

‎clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,30 @@ describe("clearStoredAuthForRelogin", () => {
333333

334334
// A grant still live at the authorization server is what the user needs to
335335
// hear about; reporting the earlier success would print no warning at all.
336+
// One budget across both keys, so a first request that outlives it fails
337+
// rather than each key getting a fresh five seconds.
338+
it("bounds both keys with one shared budget", async () => {
339+
seedBothSpellings("live-r", "stale-r");
340+
const fetchSpy = vi
341+
.spyOn(globalThis, "fetch")
342+
.mockImplementation(async () => {
343+
// Outlives the injected budget, so the second key is never attempted.
344+
await new Promise((r) => setTimeout(r, 40));
345+
return new Response(null, { status: 200 });
346+
});
347+
try {
348+
const outcome = await clearStoredAuthForRelogin("https://example.com", {
349+
budgetMs: 20,
350+
});
351+
expect(outcome).toMatchObject({ status: "failed" });
352+
expect(outcome?.status === "failed" ? outcome.detail : "").toContain(
353+
"timed out after 20ms",
354+
);
355+
} finally {
356+
fetchSpy.mockRestore();
357+
}
358+
});
359+
336360
it("reports a later failure over an earlier success", async () => {
337361
seedBothSpellings("live-r", "stale-r");
338362
const fetchSpy = vi

‎clients/cli/src/clear-stored-auth-for-relogin.ts‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,15 @@ function normalizeServerUrl(serverUrl: string): string {
3232
*/
3333
export async function clearStoredAuthForRelogin(
3434
serverUrl: string | undefined,
35-
options?: { revoke?: boolean },
35+
options?: {
36+
revoke?: boolean;
37+
/**
38+
* Total wall-clock budget for the revocation requests across both key
39+
* spellings. Defaults to {@link DEFAULT_REVOCATION_TIMEOUT_MS}; injectable
40+
* so the exhaustion path can be exercised without a five-second test.
41+
*/
42+
budgetMs?: number;
43+
},
3644
): Promise<TokenRevocationOutcome | undefined> {
3745
if (!serverUrl?.trim()) return undefined;
3846
const raw = serverUrl.trim();
@@ -68,7 +76,9 @@ export async function clearStoredAuthForRelogin(
6876
// entry from the NodeOAuthStorage cache.
6977
resetNodeOAuthStorageCache();
7078

71-
return options?.revoke === false ? undefined : sendPlans(plans);
79+
return options?.revoke === false
80+
? undefined
81+
: sendPlans(plans, options?.budgetMs ?? DEFAULT_REVOCATION_TIMEOUT_MS);
7282
}
7383

7484
/**
@@ -100,9 +110,10 @@ export async function clearStoredAuthForRelogin(
100110
*/
101111
async function sendPlans(
102112
plans: OAuthRevocationPlan[],
113+
budgetMs: number,
103114
): Promise<TokenRevocationOutcome | undefined> {
104115
const fetchFn = createProxyFetch() ?? fetch;
105-
const deadlineAt = Date.now() + DEFAULT_REVOCATION_TIMEOUT_MS;
116+
const deadlineAt = Date.now() + budgetMs;
106117
let reported: TokenRevocationOutcome | undefined;
107118
let lastSkip: TokenRevocationOutcome | undefined;
108119
for (const plan of plans) {
@@ -112,11 +123,19 @@ async function sendPlans(
112123
// may still be live when the key held no grant at all — a false alarm, and
113124
// one that outranks the real outcome under the failure-first rule below.
114125
const needsNetwork = plan.outcome === undefined;
126+
/* v8 ignore next 10 -- A bound, not a path: `withDeadline` fails any plan
127+
that reaches the deadline, so a preceding plan cannot both succeed and
128+
leave zero budget. This exists so a plan reached with nothing left is
129+
reported rather than firing a request it would immediately abandon. */
115130
if (needsNetwork && remainingMs <= 0) {
116-
reported ??= {
131+
// Overrides an earlier success rather than deferring to it: this key's
132+
// grant may still be live at the authorization server, and that is the
133+
// thing the user needs to hear about. Same failure-first rule as below.
134+
const exhausted: TokenRevocationOutcome = {
117135
status: "failed",
118-
detail: `the ${DEFAULT_REVOCATION_TIMEOUT_MS}ms revocation budget was exhausted before "${plan.serverUrl}" was attempted`,
136+
detail: `the ${budgetMs}ms revocation budget was exhausted before "${plan.serverUrl}" was attempted`,
119137
};
138+
if (reported?.status !== "failed") reported = exhausted;
120139
continue;
121140
}
122141
const outcome = await executeOAuthRevocation(plan, {

‎clients/web/src/test/core/auth/revocation.test.ts‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -932,6 +932,54 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => {
932932
expect(outcome).toMatchObject({ status: "failed" });
933933
});
934934

935+
// The preconfigured registration is only a *fallback*, so a malformed one
936+
// must not abort grants that carry their own valid credentials.
937+
it("still revokes when the preconfigured client registration is malformed", async () => {
938+
stubSnapshot(storage, {
939+
byIssuer: {
940+
"https://as.example.com": {
941+
tokens: {
942+
access_token: "a",
943+
token_type: "Bearer",
944+
refresh_token: "r-good",
945+
},
946+
clientInformation: { client_id: "dcr-cid" },
947+
},
948+
},
949+
// Not a valid `OAuthClientInformation` — no `client_id`.
950+
preregisteredClientInformation: { client_secret: "orphan" },
951+
serverMetadata: {
952+
issuer: "https://as.example.com",
953+
authorization_endpoint: "https://as.example.com/authorize",
954+
token_endpoint: "https://as.example.com/token",
955+
revocation_endpoint: REVOKE_URL,
956+
response_types_supported: ["code"],
957+
},
958+
});
959+
const fetchFn = vi.fn<typeof fetch>(
960+
async () => new Response(null, { status: 200 }),
961+
);
962+
963+
const outcome = await revokeStoredOAuthTokens({
964+
serverUrl: SERVER_URL,
965+
storage,
966+
fetchFn,
967+
});
968+
969+
// The grant was still revoked, with its OWN registration...
970+
expect(fetchFn).toHaveBeenCalledTimes(1);
971+
expect(
972+
new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get(
973+
"client_id",
974+
),
975+
).toBe("dcr-cid");
976+
// ...and the malformed fallback is reported rather than swallowed.
977+
expect(outcome).toMatchObject({ status: "failed" });
978+
expect(outcome.status === "failed" ? outcome.detail : "").toContain(
979+
"preconfigured client registration",
980+
);
981+
});
982+
935983
// A corrupt slot must not abandon the grants that are still revocable — the
936984
// state is gone either way, so the failure has to be reported BESIDE the
937985
// successes rather than instead of them.

‎core/auth/revocation.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,7 @@ interface CachedMetadata {
438438
}
439439

440440
/**
441-
* Send the requests a {@link planOAuthRevocation} snapshot describes.
441+
* Send the requests a {@link clearAndPlanRevocation} snapshot describes.
442442
*
443443
* Safe to run **after** `storage.clear(serverUrl)` — that is the point: the
444444
* plan already holds the tokens, the credentials and the endpoint, so nothing
@@ -539,9 +539,20 @@ async function collectGrants(
539539
const failures: TokenRevocationOutcome[] = [];
540540
const seenKeys = new Set<string>();
541541

542-
const preregistered = await parseClient(
543-
snapshot.preregisteredClientInformation,
544-
);
542+
// Parsed defensively: it is only a *fallback*, so a malformed preconfigured
543+
// registration must not abort grants that carry their own valid credentials.
544+
// Recorded and skipped, matching the per-slot salvage below.
545+
let preregistered: OAuthClientInformation | undefined;
546+
try {
547+
preregistered = await parseClient(snapshot.preregisteredClientInformation);
548+
} catch (err) {
549+
failures.push({
550+
status: "failed",
551+
detail: `could not read the preconfigured client registration: ${
552+
err instanceof Error ? err.message : String(err)
553+
}`,
554+
});
555+
}
545556

546557
const add = async (
547558
issuer: string | undefined,
@@ -643,7 +654,7 @@ async function runPlan(
643654
): Promise<TokenRevocationOutcome> {
644655
if (plan.outcome) return plan.outcome;
645656
const endpoint = plan.endpoint;
646-
/* v8 ignore next -- `planOAuthRevocation` always sets `outcome` when it sets
657+
/* v8 ignore next -- `clearAndPlanRevocation` always sets `outcome` when it sets
647658
no endpoint, so this is unreachable; the guard exists to narrow the type. */
648659
if (!endpoint) return { status: "skipped", reason: "no_endpoint" };
649660

0 commit comments

Comments
 (0)