fix(auth): floor the shared revocation budget instead of racing zero
The shared teardown deadline was decided on `remainingMs > 0` against `Date.now()`. Both halves of that are noise-sensitive: the deadline is enforced by a `setTimeout`, and a wall clock at millisecond resolution can still read a hair short of it when the loop comes round, so the next grant inherits a fractional budget and spends it on a request that cannot possibly complete. The same run then issues one request or two depending on scheduling — which is what makes the "shares one deadline across grants" test intermittent on CI (#2252). Measure the deadline with `performance.now()` (monotonic, so an NTP step or a suspend cannot expire or extend the budget, and sub-millisecond, so none is lost to rounding), and skip any grant reaching the loop with less than MIN_REVOCATION_REQUEST_BUDGET_MS left. The skipped grant is already reported as `failed`, so nothing is silently dropped — the needless call to the authorization server is. The CLI's outer per-plan budget in `sendPlans` shares both the shape and the defect, so it gets the same treatment. One consequence worth naming: a monotonic clock hands `revokeToken` a fractional budget, and Node's `AbortSignal.timeout` throws `ERR_OUT_OF_RANGE` on a non-integer delay — before the fetch, so the request would never be sent and the revocation would report that as its failure. `revokeToken` now rounds its own budget to whole milliseconds, which also covers any other caller passing a fractional one. Tests: the epsilon skip is pinned by a grant that lands inside the floor but above zero (removing the floor fails it, verified); the fractional budget by a `revokeToken` call at 19.996ms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017wEXtbs8UEHUMxDEAxbs99 Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
eb44e6d238
commit
84a65f28d8
2 changed files with 110 additions and 7 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in a new issue