Rate limiter: an idle-pruned bucket can split one identity into two live buckets

[pnocera]

Found by an independent Codex review (round 6) of the Bun migration branch, with a reproducing probe. Deferred from that branch deliberately: the fix is a lease/reference-count redesign, not a patch.

What's wrong

RateLimiter.isIdle() means only "full and currently unqueued". It does not mean "no client can use this object again".

Every BookStackClient stores the limiter it was given permanently in this.rateLimiter, and the HTTP readiness path deliberately caches one such client in healthServer(). So when pruneIdle() deletes a full bucket from the registry, the existing client still holds that object. A later client for the same identity is handed a new, full bucket.

The registry bounds map entries, not usable buckets per identity.

Reproduction (Codex's probe, current code)

Retain the first identity's client, insert 256 other idle identities, then look the first identity up again:

• the retained object and the replacement are different objects
• sharedRateLimiterCount() reports 256 (the cap is respected)
• both objects can make an immediate request

With burstLimit: 1, a single shared bucket could not have served both.

Why it matters

An authenticated HTTP caller can seed the cached health client, rotate enough x-bookstack-token identities to prune its full bucket, then cause normal tool traffic to construct a replacement for the configured identity. Health and tool traffic thereafter spend two independent burst allowances against the same BookStack token. The same flaw applies to any long-lived client.

This is the remaining half of a finding first raised as R4-W1: URL alias collapse, cursor progress and non-idle preservation are all fixed and tested; this ownership case is not.

Locations

• src/utils/rateLimit.ts — pruneIdle() / isIdle() candidate rule
• src/api/client.ts — client stores the limiter permanently
• src/server.ts — healthServer() caches a client

Recommended fix

Don't let clients retain evictable limiter objects. Either:

• store the rate-limit identity on BookStackClient and resolve the current shared bucket at each request, immediately before acquire(); or
• introduce explicit leases / reference counts and prune only once no client can reuse the object.

Tests to add

• Deterministic regression: retain an old client across an idle prune, construct a second client for the same identity, and assert two simultaneous requests with burstLimit: 1 cannot both leave immediately.
• Transport equivalent: the cached health server plus rotated override identities.

Note the existing tests miss this: the LRU test retains secondOldest after proving the registry replaced it, but never tries to spend the old and new objects; the live-prefix test proves non-idle objects retain identity but says nothing about an idle object still held by a reusable client.