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 9b5e587d5..09b02f18f 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -6,6 +6,7 @@ import { getStateFilePath, resetNodeOAuthStorageCache, } from "@inspector/core/auth/node/storage-node.js"; +import { MIN_REVOCATION_REQUEST_BUDGET_MS } from "@inspector/core/auth/revocation.js"; import { clearStoredAuthForRelogin } from "../src/clear-stored-auth-for-relogin.js"; const AS_ISSUER = "https://as.example.com"; @@ -379,6 +380,42 @@ describe("clearStoredAuthForRelogin", () => { } }); + // The zero-budget case above passes under the old `remainingMs <= 0` bound + // too, so it says nothing about the floor. This one is the floor's own + // detector: a budget that is genuinely positive, and genuinely too small to + // complete a request, must be spent on no request at all (#2252). The + // budget only shrinks as the loop runs, so a slow machine cannot flip it. + it("issues no request for a positive budget below the minimum", async () => { + seedBothSpellings("live-r", "stale-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 200 })); + // Frozen, so the remainder at the check is exactly the budget. Left to + // the real clock this is a one-sided detector: a worker preempted for + // more than the budget reaches the check with a NEGATIVE remainder, where + // the unfixed `remainingMs <= 0` bound takes the same branch and prints + // the same message — passing without the floor (Copilot). + const nowSpy = vi.spyOn(performance, "now").mockReturnValue(1_000); + try { + const outcome = await clearStoredAuthForRelogin("https://example.com", { + budgetMs: MIN_REVOCATION_REQUEST_BUDGET_MS - 1, + }); + expect(fetchSpy).not.toHaveBeenCalled(); + expect(outcome).toMatchObject({ status: "failed" }); + // The *plan* was never attempted, so the report has to be this loop's + // own — naming the server URL — and not the per-grant one from inside + // `executeOAuthRevocation`. Without that distinction the assertion + // passes on the old `<= 0` bound too: the plan would be handed a 4ms + // budget, and core's identical floor would decline it one level down. + expect(outcome?.status === "failed" ? outcome.detail : "").toContain( + 'budget was exhausted before "', + ); + } finally { + nowSpy.mockRestore(); + 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 6ec64e6e8..a67695d9d 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -4,6 +4,7 @@ import { } from "@inspector/core/auth/node/storage-node.js"; import { DEFAULT_REVOCATION_TIMEOUT_MS, + MIN_REVOCATION_REQUEST_BUDGET_MS, clearAndPlanRevocation, executeOAuthRevocation, type OAuthRevocationPlan, @@ -113,17 +114,19 @@ async function sendPlans( budgetMs: number, ): Promise { const fetchFn = createProxyFetch() ?? fetch; - const deadlineAt = Date.now() + budgetMs; + // Monotonic and epsilon-floored for the same reasons as the shared deadline + // inside `executeOAuthRevocation` — see MIN_REVOCATION_REQUEST_BUDGET_MS. + const deadlineAt = performance.now() + budgetMs; let reported: TokenRevocationOutcome | undefined; let lastSkip: TokenRevocationOutcome | undefined; for (const plan of plans) { - const remainingMs = deadlineAt - Date.now(); + const remainingMs = deadlineAt - performance.now(); // A plan that already knows its answer needs no network, so the budget is // irrelevant to it. Synthesising exhaustion here would warn that a grant // 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; - if (needsNetwork && remainingMs <= 0) { + if (needsNetwork && remainingMs <= MIN_REVOCATION_REQUEST_BUDGET_MS) { // 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. diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index fe5c29f15..e50fa1e96 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -3,6 +3,7 @@ import type { OAuthMetadata } from "@modelcontextprotocol/client"; import { BrowserOAuthStorage } from "@inspector/core/auth/browser/storage.js"; import { DEFAULT_REVOCATION_TIMEOUT_MS, + MIN_REVOCATION_REQUEST_BUDGET_MS, aggregateOutcomes, buildRevocationRequest, revocationAuthMethods, @@ -431,6 +432,27 @@ describe("revokeToken", () => { expect(seen?.signal).toBeInstanceOf(AbortSignal); expect(DEFAULT_REVOCATION_TIMEOUT_MS).toBeGreaterThan(0); }); + + // What is left of a shared deadline is measured with a sub-millisecond clock, + // so the budget handed down here is routinely fractional — and Node's + // `AbortSignal.timeout` throws `ERR_OUT_OF_RANGE` on a non-integer delay, + // before the fetch, turning a perfectly good request into a revocation failure + // that never left the process (#2252). + it("accepts a fractional budget and still sends the request", async () => { + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + const outcome = await revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + fetchFn, + timeoutMs: 19.996, + }); + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(outcome).toMatchObject({ status: "revoked" }); + }); }); /** @@ -838,6 +860,58 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { expect(fetchFn).toHaveBeenCalledTimes(1); }); + // The boundary the shared budget used to be decided on was `remainingMs > 0`, + // which timer resolution can land either side of: a grant that finished a + // hair before the deadline left a fractional budget behind, and the next grant + // spent it on a request that could not possibly complete (#2252). The floor + // makes that decision the same on every run. + it("does not issue a request with less than the minimum budget left", async () => { + const grant = (n: string) => ({ + issuer: "https://as.example.com", + token: `r-${n}`, + tokenTypeHint: "refresh_token" as const, + }); + const timeoutMs = 40; + // The clock is stubbed rather than slept against. A real sleep makes this a + // ONE-SIDED detector: under contention the first request overruns, the + // remainder goes negative, and the unfixed `remainingMs <= 0` bound skips + // the second grant for the wrong reason — so the test passes on an + // implementation that has no floor at all. Advancing a stub inside the + // fetch puts the second grant's remainder at exactly + // `MIN_REVOCATION_REQUEST_BUDGET_MS - 1` on every machine: positive, and + // under the floor, which is the only state that tells the two apart. + let now = 1_000; + const nowSpy = vi.spyOn(performance, "now").mockImplementation(() => now); + const fetchFn = vi.fn(async () => { + now += timeoutMs - MIN_REVOCATION_REQUEST_BUDGET_MS + 1; + return new Response(null, { status: 200 }); + }); + + try { + const outcome = await executeOAuthRevocation( + { + serverUrl: SERVER_URL, + grants: [grant("a"), grant("b")], + failures: [], + endpoint: REVOKE_URL, + supportedAuthMethods: [], + metadataIssuer: "https://as.example.com", + }, + { fetchFn, timeoutMs }, + ); + + expect(fetchFn).toHaveBeenCalledTimes(1); + // The unattempted grant outranks the first one's success, so the caller + // hears that a grant may still be live rather than that all was well. + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "budget was exhausted", + ); + } finally { + nowSpy.mockRestore(); + } + }); + // A grant bound to an issuer the cached metadata does not describe cannot be // revoked — that endpoint belongs to a different authorization server, and // sending it another AS's token would hand a credential to a server that diff --git a/core/auth/index.ts b/core/auth/index.ts index 2ba73e832..c74702ce3 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -142,6 +142,7 @@ export { discoverScopes } from "./discovery.js"; // RFC 7009 token revocation (#2144) export { DEFAULT_REVOCATION_TIMEOUT_MS, + MIN_REVOCATION_REQUEST_BUDGET_MS, aggregateOutcomes, buildRevocationRequest, revocationAuthMethods, diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index 3514e4a43..f2d16c0f9 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -47,6 +47,25 @@ import type { OAuthStorage, RevocationSnapshot } from "./storage.js"; */ export const DEFAULT_REVOCATION_TIMEOUT_MS = 5000; +/** + * The least budget worth spending a revocation request on. + * + * The shared deadline below is consumed sequentially, so the grant that follows + * a slow one can arrive with a sliver of budget left — a couple of milliseconds, + * which is less than a TCP handshake, let alone a round trip. Issuing that + * request buys nothing: it is guaranteed to time out, and its outcome is the + * same `failed` the exhausted-budget branch already reports, only after a + * needless call to the authorization server. + * + * It also removes a boundary that nothing can land on cleanly. `remainingMs > 0` + * is decided by timer resolution: the deadline is enforced by a `setTimeout`, + * and a clock that has not yet ticked past the deadline when the loop comes + * round leaves a fractional budget behind, so the same run issues one request or + * two depending on scheduling noise (#2252). A floor an order of magnitude above + * that noise makes the decision the same every time. + */ +export const MIN_REVOCATION_REQUEST_BUDGET_MS = 5; + /** Why a revocation request was not sent. */ export type TokenRevocationSkipReason = /** The caller turned revocation off for this server. */ @@ -231,7 +250,17 @@ export interface RevokeTokenParams extends RevocationRequestParams { export async function revokeToken( params: RevokeTokenParams, ): Promise { - const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS; + // Whole milliseconds, because `AbortSignal.timeout` takes an integer: Node + // throws `ERR_OUT_OF_RANGE` on a fractional delay — before the fetch, so the + // request is never sent and the caller gets that as the revocation's failure + // detail. What is left of a shared deadline is measured with a + // sub-millisecond clock, so a fractional budget does arrive here. Rounded + // rather than floored so a caller's own whole-millisecond timeout survives the + // trip through that clock and is still the number the timeout message names. + const timeoutMs = Math.max( + 0, + Math.round(params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS), + ); try { // Inside the try: `encodeURIComponent` throws on a lone UTF-16 surrogate, // which is valid JSON and so can reach here from a persisted client id or @@ -692,7 +721,11 @@ async function runPlan( // on purpose (a burst of parallel requests to one authorization server is // not a kindness), so the budget is shared instead. const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS; - const deadlineAt = Date.now() + timeoutMs; + // `performance.now()` rather than `Date.now()`: this is an elapsed-time + // measurement, and a wall clock can be stepped by NTP or a suspend/resume + // mid-teardown, which would either expire the budget early or extend it. It is + // also sub-millisecond, so a budget is not spent or preserved by rounding. + const deadlineAt = performance.now() + timeoutMs; for (const grant of plan.grants) { // Metadata is cached once per server, not per issuer, so it describes @@ -735,8 +768,10 @@ async function runPlan( continue; } - const remainingMs = deadlineAt - Date.now(); - if (remainingMs <= 0) { + const remainingMs = deadlineAt - performance.now(); + // Not `<= 0`: see MIN_REVOCATION_REQUEST_BUDGET_MS. A budget too small to + // complete a request is treated as no budget at all. + if (remainingMs <= MIN_REVOCATION_REQUEST_BUDGET_MS) { outcomes.push({ status: "failed", endpoint,