Merge pull request #2256 from modelcontextprotocol/v2/fix/2252-revocation-deadline-epsilon

fix(auth): floor the shared revocation budget instead of racing zero

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

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<TokenRevocationOutcome | undefined> {
   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.

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

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,

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<TokenRevocationOutcome> {
-  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,