From d0d72d8cafc3418e6c03da2573bea03793dae4f4 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Sun, 16 Aug 2026 00:21:09 +0100 Subject: [PATCH] fix: refresh-token reuse grace 60s -> 20s (cross-tab coordinated rotation) The frontend's cross-tab coordination (auth.svelte.ts REFRESH_LOCK_TTL_MS=15s + 20s wait-for-timeout) guarantees only ONE tab rotates and every sibling adopts the rotated pair, so the only legitimately-arriving replays are same-tick races (sub-second). The old 60s window handed a stolen refresh token a full minute of freshness before reuse detection fired; 20s keeps comfortable margin over the coordination bound while cutting the undetected-theft window to a third. The ideal fix (kill only when the replay's IP/UA differs) still needs rotation-origin persistence the locked schema cannot express. --- backend/auth/jwt.go | 21 ++++++++------- backend/auth/jwt_test.go | 56 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 65 insertions(+), 12 deletions(-) diff --git a/backend/auth/jwt.go b/backend/auth/jwt.go index 71497cb..69fe152 100644 --- a/backend/auth/jwt.go +++ b/backend/auth/jwt.go @@ -41,16 +41,19 @@ const refreshTokenLifetimeDays = int64(RefreshTokenLifetime / (24 * time.Hour)) // does NOT raise the refresh_token_reuse alert — only a replay after the window // has elapsed is treated as theft (see VerifyRefreshToken). // -// LOW-2: 60s (was 30s) because a legitimately-rotated token can be replayed -// from the SAME device well after the old 30s window when the user's second -// tab refreshes on a slower return (e.g. the tab was suspended by the OS and -// wakes up >30s after the first tab rotated). The widened grace costs a stolen -// token up to an extra 30s of freshness before reuse detection fires — an -// acceptable trade-off for not killing a legitimate session. The ideal fix +// LOW-2: 20s (was 60s). The frontend's cross-tab coordination +// (frontend/src/lib/stores/auth.svelte.ts: REFRESH_LOCK_TTL_MS = 15s, plus a +// 20s wait-for-timeout) guarantees only ONE tab performs a rotation and every +// sibling tab adopts the rotated pair instead of replaying the old token, so +// the only legitimately-arriving replays are same-tick races — two in-flight +// fetches that crossed before the lock settled, sub-second. The old 60s window +// handed a stolen refresh token up to a full minute of freshness before reuse +// detection fired; 20s keeps comfortable margin over the cross-tab coordination +// bound while cutting the undetected-theft window to a third. The ideal fix // (only kill when the replay's IP/User-Agent differs from the rotation's) // would need the rotation origin persisted per family, which the locked schema -// cannot express today; the widened window is the safe minimum. -const refreshTokenReuseGrace = 60 * time.Second +// cannot express today. +const refreshTokenReuseGrace = 20 * time.Second // refreshTokenReuseGraceSecs is the grace window in whole seconds for the SQL // make_interval(secs => ...) comparison in VerifyRefreshToken's reuse branch. @@ -667,7 +670,7 @@ func VerifyRefreshToken(ctx context.Context, tokenString string) (userID string, // the GLOBAL cap (adminnotify.MaxUnacknowledgedCriticalLogs) bounds the // unacknowledged 'refresh_token_reuse' queue ATOMICALLY (Round 2 Loop B // finding 1): without it a register-botnet — N accounts, each rotated - // once and replayed past the 60s grace — could bury the single-operator + // once and replayed past the grace window — could bury the single-operator // notification centre under unbounded alerts. The cap is folded into // the INSERT's WHERE clause (count-then-insert is atomic, closing the // TOCTOU), and the pre-check logs the suppression for operator diff --git a/backend/auth/jwt_test.go b/backend/auth/jwt_test.go index cb96916..4cf2c27 100644 --- a/backend/auth/jwt_test.go +++ b/backend/auth/jwt_test.go @@ -519,8 +519,8 @@ func TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts(t *testing.T) { t.Fatalf("expected 2 refresh tokens in family, got %d", famCount) } - // Reuse grace window: backdate the original's used_at past the 60s reuse - // grace so the replay below is genuine theft. WITHOUT this, a replay + // Reuse grace window: backdate the original's used_at past the reuse grace + // so the replay below is genuine theft. WITHOUT this, a replay // moments after rotation is a benign two-tab concurrent refresh and the // family must NOT be killed. if _, err := tx.Exec(ctx, ` @@ -573,7 +573,7 @@ func TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts(t *testing.T) { } // TestVerifyRefreshToken_ReplayWithinGrace_IsBenign verifies the reuse -// hardening: a used-token replay WITHIN the 60s grace window (two tabs sharing +// hardening: a used-token replay WITHIN the grace window (two tabs sharing // one localStorage refresh token both refreshing on load) is a benign // concurrent refresh — the generic error is returned, but the rotation family // survives and no refresh_token_reuse alert is raised. @@ -634,6 +634,56 @@ func TestVerifyRefreshToken_ReplayWithinGrace_IsBenign(t *testing.T) { } } +// TestVerifyRefreshToken_GraceBoundary pins the reuse-grace boundary (LOW-2, +// reduced 60s → 20s): a used-token replay just INSIDE the grace window is a +// benign concurrent refresh — the family survives — while a replay just past +// the window is genuine theft and revokes the ENTIRE rotation family. The +// exact boundary is refreshTokenReuseGraceSecs (derived from +// refreshTokenReuseGrace), so this test holds the reduced value honest. +func TestVerifyRefreshToken_GraceBoundary(t *testing.T) { + ctx, tx := testtx.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + original, _, err := GenerateRefreshToken(ctx, userID, "verified_email") + require.NoError(t, err) + _, _, familyID, err := VerifyRefreshToken(ctx, original) + require.NoError(t, err) + require.NotEmpty(t, familyID) + _, err = GenerateRefreshTokenInFamily(ctx, userID, "verified_email", familyID) + require.NoError(t, err) + + backdate := func(secs int64) { + t.Helper() + _, err = tx.Exec(ctx, ` + UPDATE refresh_tokens + SET used_at = NOW() - make_interval(secs => $2) + WHERE token_hash = encode(sha256($1::bytea), 'hex') + `, original, secs) + require.NoError(t, err) + } + + // Replay just INSIDE the grace window (grace - 1s) → benign: the family + // (used original + descendant) survives and no theft alert is raised. + backdate(refreshTokenReuseGraceSecs - 1) + _, _, _, err = VerifyRefreshToken(ctx, original) + require.Error(t, err, "a within-grace replay must still return the generic error") + require.Contains(t, err.Error(), "invalid or expired") + var famCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM refresh_tokens WHERE family_id = $1`, familyID).Scan(&famCount)) + require.Equal(t, 2, famCount, "a within-grace replay must NOT kill the rotation family") + + // Replay just OUTSIDE the grace window (grace + 1s) → theft: the ENTIRE + // family is revoked (descendant included) and a critical alert is raised. + backdate(refreshTokenReuseGraceSecs + 1) + _, _, _, err = VerifyRefreshToken(ctx, original) + require.Error(t, err, "a post-grace replay must return the generic error") + require.Contains(t, err.Error(), "invalid or expired") + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM refresh_tokens WHERE family_id = $1`, familyID).Scan(&famCount)) + require.Equal(t, 0, famCount, "a post-grace replay must revoke the ENTIRE rotation family") +} + // TestAccessTokenKilledWithRotationFamily verifies the HIGH 1 fix: an access // token minted at rotation is bound (family_id claim) to the rotation family, // so when reuse detection DELETEs the family the access token — which the