F176: idempotent refresh grace — close the #169 tab-death residual

docs/superpowers/specs/2026-07-23-idempotent-refresh-design.md

@@ -0,0 +1,117 @@
+# #176 — Idempotent refresh (grace-window mint-on-retry)
+
+## Problem
+
+#169 serialised refresh across browser tabs with the Web Locks API, closing the
+common multi-tab race. One residual a page-owned lock cannot close remains: if a
+tab dies in the sub-second between **sending** a refresh and **receiving** the
+rotated cookie, the lock auto-releases while the cookie still holds the *old*
+token. The next tab presents that already-rotated token, and today's
+reuse-detection reads it as a replay → revokes the whole token family → both
+tabs are logged out.
+
+## Goal & the unavoidable trade-off
+
+Make refresh **idempotent** for the immediately-previous rotation so a benign
+dead-tab / concurrent retry succeeds instead of nuking the session — while
+keeping theft-detection strict for every realistic replay.
+
+Reuse-detection works by catching *divergence* (someone presents a token derived
+from an already-rotated ancestor). Any server grace window that lets the racing
+tab succeed must accept a just-revoked token, so **within that window a stolen
+token is also accepted**. This is inherent and unavoidable; the mitigation is to
+keep the window tiny (default 10s, vs the ~15-min refresh cadence) and to fail
+strict everywhere else. Chosen deliberately (design decision, 2026-07-23).
+
+## Design — mint-on-retry grace
+
+In `IdentityProvider.RefreshAsync`, replace the "revoked token → immediately
+revoke family" branch with:
+
+```
+present token T; stored = lookup(hash(T))
+if stored is null            -> invalid            (unchanged)
+if stored.RevokedAt is not null:
+    # #176 grace: a token rotated within GraceWindow whose replacement is still
+    # the live tip is a benign retry (the #169 residual), not a replay.
+    if stored.ReplacedByTokenHash is not null
+       and (now - stored.RevokedAt) <= GraceWindow
+       and replacement = lookup(stored.ReplacedByTokenHash)
+       and replacement.IsActive(now):
+          stored = replacement        # advance the LIVE token; fall through to normal rotation
+    else:
+          RevokeAllActiveForUserAsync(...)          # genuine theft (unchanged)
+          return invalid
+if stored.ExpiresAt <= now   -> expired            (unchanged)
+rotate stored -> new pair    (unchanged)
+```
+
+- For the actual tab-death case the replacement was never delivered to anyone,
+  so advancing it does **not** fork the chain — there is one live token after.
+- The grace path re-enters the *normal* rotation of the still-active replacement,
+  so single-use + expiry semantics are preserved unchanged.
+
+## Config
+
+Add `JwtOptions.RefreshReuseGraceSeconds` (default **10**). Bound by config so a
+deployment can tune or disable it; tests set it to `0` to exercise the strict
+path deterministically.
+
+## Theft-detection remains strict (all deterministic, no time-mocking)
+
+- **Replacement already consumed:** rotate twice (`R0→R1→R2`), then replay `R0`.
+  `R0`'s replacement `R1` is now revoked (not the live tip) → grace fails →
+  revoke family. Catches an attacker whose stolen token's chain has moved on.
+- **Grace disabled (factory `RefreshReuseGraceSeconds=0`):** immediate replay →
+  `now - RevokedAt > 0` → expired → revoke family. Proves the grace gate is
+  load-bearing.
+- **Benign retry (default grace):** rotate `R0→R1`, immediately replay `R0` →
+  within grace, `R1` active → succeeds with a fresh token; family NOT revoked.
+
+## Test changes
+
+- Update `Refresh_WithRotatedToken_IsRejected` and
+  `Refresh_ReuseDetection_RevokesTheWholeFamily` (both assert *immediate* replay
+  → 401, which is now the benign grace path) to the consumed-replacement /
+  grace-0 theft scenarios.
+- Add the benign-retry success test and a grace-0 factory theft test.
+
+## Docs
+
+- GLOSSARY "Session tokens": note the short idempotency grace on reuse-detection
+  that closes the #169 residual.
+- `web/README` auth section: the residual note now points here.
+
+## Hardening (post 4-way review)
+
+The 4-way review (codex + pi + 2 agents) + a reproduction test found the naive
+grace above has two HIGH holes; both are now closed (user chose "full hardening"):
+
+1. **Leap-frog / chain extension.** The grace advanced the replacement by minting
+   a new token, which revoked the replacement — and that revoked link was itself
+   grace-eligible, so a stolen token could be stepped down the chain (`T0→grace→T2`,
+   then `T1→grace→T3`, …) to extend a session indefinitely within the window.
+   Fix: `RefreshToken.RevokedByGrace` marks a link revoked BY a grace-advance;
+   `TryGraceReplacementAsync` refuses to grace such a link, bounding the grace to
+   a SINGLE hop off a normal rotation. A second-hop attempt hits the strict
+   theft path → family revoked.
+
+2. **Concurrent fork + backstop suppression.** `RefreshToken` had no concurrency
+   control, so two parallel presentations of the same token both read it active
+   and each minted a live child — a fork that, on the grace path, spawned
+   multiple sessions AND skipped the family-revoke backstop entirely. Fix: a
+   regenerated `ConcurrencyStamp` optimistic-concurrency token makes consuming a
+   token an atomic compare-and-swap; the losing writer's `SaveChanges` throws
+   `DbUpdateConcurrencyException` and `RefreshAsync` fails it closed. The two
+   revoke paths (`RevokeRefreshTokenAsync`, `RevokeAllActiveForUserAsync`) became
+   bulk `ExecuteUpdateAsync` so they don't fight the stamp and are safe to call
+   from the rotation fail path. Invariant: two concurrent presentations of one
+   token never leave more than one live tip.
+
+Also: a clock-skew guard (`elapsed >= 0`) so a future `RevokedAt` from a lagging
+node can't widen the window.
+
+## Out of scope
+
+- The client Web Lock (#169) stays as the first line of defence; this grace is
+  the server-side safety net for the tab-death residual only.

specs/product/GLOSSARY.md

@@ -443,7 +443,17 @@ second would trip theft-detection, logging both out; refresh is therefore
 **serialised across tabs** via the browser Web Locks API (#169) so only one tab
 refreshes at a time and the next presents the freshly-rotated cookie. Server
 theft-detection stays strict; browsers without the API fall back to per-tab
-coordination only.
+coordination only. As the server-side safety net for the case a lock can't cover
+(a tab dying between sending a refresh and receiving the rotated cookie),
+reuse-detection carries a short **idempotency grace** (#176): a token rotated in
+the last few seconds whose replacement is still the live tip is treated as a
+benign retry — the caller gets a fresh token instead of the family being revoked.
+The grace is deliberately tiny (default 10s) vs the ~15-min refresh cadence, and
+**bounded to a single hop** (the link revoked by a grace-advance can't itself be
+graced), so a stolen token can't be leap-frogged down the chain and a genuinely
+replayed/stale token is still caught and revokes the whole family. Consuming a
+token is an atomic compare-and-swap (a per-token concurrency stamp), so concurrent
+replays can never fork one token into two live sessions.
 
 **Version (concurrency token)** — every mutable aggregate carries a `Version`
 that each mutation bumps. Two concurrent edits: first save wins, second gets

src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs

@@ -94,12 +94,28 @@ public async Task<Result<TokenPair>> RefreshAsync(string refreshToken, Cancellat
         if (stored is null)
             return Result.Failure<TokenPair>(Error.Validation("Identity.InvalidRefreshToken", "Refresh token is invalid."));
 
-        // Presenting an already-rotated/revoked token means it was replayed — treat as a
-        // possible theft and revoke every active token for the user (breaks the chain).
+        // Presenting an already-rotated/revoked token normally means it was replayed —
+        // treat as a possible theft and revoke every active token for the user.
+        var viaGrace = false;
         if (stored.RevokedAt is not null)
         {
-            await RevokeAllActiveForUserAsync(stored.UserId, now, ct);
-            return Result.Failure<TokenPair>(Error.Validation("Identity.InvalidRefreshToken", "Refresh token is invalid."));
+            // #176 — idempotency grace: a token rotated within the last
+            // RefreshReuseGraceSeconds whose replacement is still the live tip is a
+            // benign concurrent/dead-tab retry (the #169 residual), not a replay.
+            // Advance the still-active replacement (fall through to the normal
+            // rotation below) and hand the caller a fresh token instead of revoking
+            // the family. For the actual tab-death case the replacement was never
+            // delivered, so this does not fork the chain. Anything else — a stale
+            // token, an expired grace, or a replacement already gone — is a genuine
+            // replay and still burns the family down.
+            var graced = await TryGraceReplacementAsync(stored, now, ct);
+            if (graced is null)
+            {
+                await RevokeAllActiveForUserAsync(stored.UserId, now, ct);
+                return Result.Failure<TokenPair>(Error.Validation("Identity.InvalidRefreshToken", "Refresh token is invalid."));
+            }
+            stored = graced;
+            viaGrace = true;
         }
 
         if (stored.ExpiresAt <= now)
@@ -109,12 +125,28 @@ public async Task<Result<TokenPair>> RefreshAsync(string refreshToken, Cancellat
         if (user is null)
             return Result.Failure<TokenPair>(Error.Validation("Identity.InvalidRefreshToken", "Refresh token is invalid."));
 
-        // Rotate: revoke the presented token and issue a fresh one.
+        // Rotate: revoke the presented token and issue a fresh one. Marking the
+        // revocation as grace-sourced (#176) bounds the grace to a single hop: the
+        // token minted here can be rotated normally, but this just-revoked link
+        // can never itself be grace-advanced, so a stolen token can't be
+        // leap-frogged down the chain.
         var (rawToken, newHash) = GenerateRefreshToken();
         stored.RevokedAt = now;
         stored.ReplacedByTokenHash = newHash;
+        stored.RevokedByGrace = viaGrace;
+        stored.ConcurrencyStamp = Guid.NewGuid().ToString(); // rotate the CAS token (#176)
         db.RefreshTokens.Add(NewToken(user, newHash));
-        await db.SaveChangesAsync(ct);
+        try
+        {
+            await db.SaveChangesAsync(ct);
+        }
+        catch (DbUpdateConcurrencyException)
+        {
+            // #176 — another request consumed this exact token first (concurrent
+            // presentation of the same token). The winner already minted the one
+            // live child; fail this one closed rather than fork a second session.
+            return Result.Failure<TokenPair>(Error.Validation("Identity.InvalidRefreshToken", "Refresh token is invalid."));
+        }
 
         // Roles re-read on every refresh so a demotion takes effect within one
         // access-token lifetime, not at next login.
@@ -225,14 +257,14 @@ private static string Describe(IdentityResult result) =>
 
     public async Task RevokeRefreshTokenAsync(string refreshToken, CancellationToken ct = default)
     {
+        // Bulk conditional update, not a tracked read-modify-save: the #176 xmin
+        // concurrency token would otherwise make this throw if the token was
+        // rotated concurrently. WHERE RevokedAt == null makes it idempotent.
         var presentedHash = Hash(refreshToken);
-        var stored = await db.RefreshTokens
-            .FirstOrDefaultAsync(t => t.TokenHash == presentedHash && t.RevokedAt == null, ct);
-
-        if (stored is null) return;
-
-        stored.RevokedAt = timeProvider.GetUtcNow();
-        await db.SaveChangesAsync(ct);
+        var now = timeProvider.GetUtcNow();
+        await db.RefreshTokens
+            .Where(t => t.TokenHash == presentedHash && t.RevokedAt == null)
+            .ExecuteUpdateAsync(s => s.SetProperty(t => t.RevokedAt, now), ct);
     }
 
     private RefreshToken NewToken(ApplicationUser user, string tokenHash)
@@ -249,17 +281,35 @@ private RefreshToken NewToken(ApplicationUser user, string tokenHash)
         };
     }
 
+    // #176 — returns the live replacement to rotate when `revoked` is a benign
+    // grace retry (rotated within the grace window and its replacement is still
+    // active), or null when it is a genuine replay that must revoke the family.
+    private async Task<RefreshToken?> TryGraceReplacementAsync(
+        RefreshToken revoked, DateTimeOffset now, CancellationToken ct)
+    {
+        var graceSeconds = jwtOptions.Value.RefreshReuseGraceSeconds;
+        var elapsed = now - (revoked.RevokedAt ?? now);
+        if (graceSeconds <= 0                       // grace disabled → strict replay
+            || revoked.RevokedByGrace               // already a grace hop → don't chain (one-hop bound)
+            || revoked.ReplacedByTokenHash is null
+            || revoked.RevokedAt is null
+            || elapsed < TimeSpan.Zero               // clock-skew guard: a future RevokedAt must not widen the window
+            || elapsed > TimeSpan.FromSeconds(graceSeconds))
+            return null;
+
+        var replacement = await db.RefreshTokens
+            .FirstOrDefaultAsync(t => t.TokenHash == revoked.ReplacedByTokenHash, ct);
+        return replacement is not null && replacement.IsActive(now) ? replacement : null;
+    }
+
     private async Task RevokeAllActiveForUserAsync(Guid userId, DateTimeOffset now, CancellationToken ct)
     {
-        var active = await db.RefreshTokens
+        // Bulk update rather than tracked read-modify-save: it never trips the
+        // #176 xmin concurrency token (so it is safe to call from the rotation
+        // fail path) and revokes the whole family in one statement.
+        await db.RefreshTokens
             .Where(t => t.UserId == userId && t.RevokedAt == null)
-            .ToListAsync(ct);
-
-        foreach (var token in active)
-            token.RevokedAt = now;
-
-        if (active.Count > 0)
-            await db.SaveChangesAsync(ct);
+            .ExecuteUpdateAsync(s => s.SetProperty(t => t.RevokedAt, now), ct);
     }
 
     // 256-bit random token; only the SHA-256 hash is persisted.

src/Cluckwork.Infrastructure/Identity/JwtOptions.cs

@@ -10,4 +10,12 @@ public sealed class JwtOptions
     public string PrivateKeyPem { get; init; } = string.Empty;
     public int AccessTokenMinutes { get; init; } = 15;
     public int RefreshTokenDays { get; init; } = 30;
+
+    // #176 — idempotency grace for refresh-token reuse-detection. A token rotated
+    // within this many seconds whose replacement is still the live tip is treated
+    // as a benign concurrent/dead-tab retry (the #169 residual), not a replay, so
+    // the racing tab is handed a fresh token instead of the whole family being
+    // revoked. Kept short vs the ~15-min refresh cadence to bound the inherent
+    // in-window relaxation of theft-detection; set 0 to disable (strict replay).
+    public int RefreshReuseGraceSeconds { get; init; } = 10;
 }

src/Cluckwork.Infrastructure/Identity/RefreshToken.cs

@@ -20,6 +20,19 @@ public sealed class RefreshToken
     public DateTimeOffset? RevokedAt { get; set; }
     public string? ReplacedByTokenHash { get; set; }
 
+    // #176 — set when this token was revoked BY a grace-advance (not a normal
+    // rotation). Reuse-detection refuses to grace such a token again, so the
+    // idempotency grace is bounded to a SINGLE hop off a normal rotation and
+    // cannot be leap-frogged down the chain to extend a stolen session.
+    public bool RevokedByGrace { get; set; }
+
+    // #176 — optimistic-concurrency token, regenerated on every rotation. Two
+    // concurrent presentations of the same token both load the same stamp; only
+    // the first UPDATE (WHERE ConcurrencyStamp = old) matches a row, so the loser
+    // throws DbUpdateConcurrencyException and RefreshAsync fails it closed —
+    // preventing a fork into two live sessions from one token.
+    public string ConcurrencyStamp { get; set; } = Guid.NewGuid().ToString();
+
     public bool IsActive(DateTimeOffset now) => RevokedAt is null && ExpiresAt > now;
 }
 
@@ -33,7 +46,17 @@ public void Configure(EntityTypeBuilder<RefreshToken> builder)
         builder.Property(e => e.AccountId).IsRequired();
         builder.Property(e => e.TokenHash).HasMaxLength(64).IsRequired();
         builder.Property(e => e.ReplacedByTokenHash).HasMaxLength(64);
+        builder.Property(e => e.RevokedByGrace).HasDefaultValue(false);
         builder.HasIndex(e => e.TokenHash).IsUnique();
         builder.HasIndex(e => e.UserId);
+
+        // #176 — consuming a refresh token (revoke-and-rotate) must be an atomic
+        // compare-and-swap: two concurrent presentations of the same token would
+        // otherwise both read it active and each mint a live child (a fork that,
+        // on the grace path, spawns multiple sessions AND skips theft-detection).
+        // The stamp is regenerated on every rotation, so a concurrent writer's
+        // UPDATE matches no row and throws DbUpdateConcurrencyException, which
+        // RefreshAsync fails closed.
+        builder.Property(e => e.ConcurrencyStamp).IsConcurrencyToken().HasMaxLength(36);
     }
 }