stacklok / toolhive / 28232691304

67%

Build Type

push

github

Committed by web-flow

Commit Message

Fail closed on unpersisted rotated refresh token (#5636)

RefreshAndStore redeemed the stored refresh token at the IdP, then
persisted the result, but swallowed any storage-write failure and
returned the refreshed tokens anyway. With refresh-token rotation
(the common case) that strands a dead token in storage and gets the
user's whole token family revoked:

  1. read stored row            -> RT(v1)
  2. provider.RefreshTokens(RT v1)
        -> IdP rotates: RT(v1) now DEAD, returns AT', RT(v2)
           (irreversible -- it happened at the IdP)
  3. StoreUpstreamTokens(RT v2) FAILS   (Redis blip, timeout, ...)
  4. old code: log + `return updated, nil`  -- swallows the failure
        caller gets AT'/RT(v2), request succeeds,
        but STORAGE STILL HOLDS RT(v1)        -- poisoned
        ----- time passes -----
  5. next refresh: read stored row -> RT(v1)
  6. provider.RefreshTokens(RT v1)
        -> IdP: "RT(v1) already used -> REPLAY -> breach"
           revokes the whole family (v1, v2, ...)
  7. user hard-logged-out of the upstream; full re-auth required

The damage is done at step 2 (rotation is irreversible) but only
detonates at step 6 when the stale row is replayed. No concurrency
is needed -- one failed write plus any later refresh.

Fix, replacing steps 3-4:
  - Detect rotation (provider returned a new, non-empty refresh token
    that differs from the stored one).
  - Not rotated: keep today's behavior -- the stored token is still
    valid, so proceed and let the next request retry.
  - Rotated + store failed: retry the store briefly; if it still
    fails, best-effort delete the stale per-provider row so the next
    read misses and forces a clean re-auth instead of replaying a
    dead token, then return an error so callers fail closed.

Add DeleteUpstreamTokensForProvider to the storage interface (memory
+ redis); deleting an absent row is a no-op and siblings are
untouched.

Residual window: a crash between the IdP redemption and t... (continued)

Coverage Stats

76 of 107 new or added lines in 4 files covered. (71.03%)

24 existing lines in 6 files now uncovered.

69578 of 103343 relevant lines covered (67.33%)

65.82 hits per line