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.
This commit is contained in:
+12
-9
@@ -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
|
// does NOT raise the refresh_token_reuse alert — only a replay after the window
|
||||||
// has elapsed is treated as theft (see VerifyRefreshToken).
|
// has elapsed is treated as theft (see VerifyRefreshToken).
|
||||||
//
|
//
|
||||||
// LOW-2: 60s (was 30s) because a legitimately-rotated token can be replayed
|
// LOW-2: 20s (was 60s). The frontend's cross-tab coordination
|
||||||
// from the SAME device well after the old 30s window when the user's second
|
// (frontend/src/lib/stores/auth.svelte.ts: REFRESH_LOCK_TTL_MS = 15s, plus a
|
||||||
// tab refreshes on a slower return (e.g. the tab was suspended by the OS and
|
// 20s wait-for-timeout) guarantees only ONE tab performs a rotation and every
|
||||||
// wakes up >30s after the first tab rotated). The widened grace costs a stolen
|
// sibling tab adopts the rotated pair instead of replaying the old token, so
|
||||||
// token up to an extra 30s of freshness before reuse detection fires — an
|
// the only legitimately-arriving replays are same-tick races — two in-flight
|
||||||
// acceptable trade-off for not killing a legitimate session. The ideal fix
|
// 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)
|
// (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
|
// would need the rotation origin persisted per family, which the locked schema
|
||||||
// cannot express today; the widened window is the safe minimum.
|
// cannot express today.
|
||||||
const refreshTokenReuseGrace = 60 * time.Second
|
const refreshTokenReuseGrace = 20 * time.Second
|
||||||
|
|
||||||
// refreshTokenReuseGraceSecs is the grace window in whole seconds for the SQL
|
// refreshTokenReuseGraceSecs is the grace window in whole seconds for the SQL
|
||||||
// make_interval(secs => ...) comparison in VerifyRefreshToken's reuse branch.
|
// 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
|
// the GLOBAL cap (adminnotify.MaxUnacknowledgedCriticalLogs) bounds the
|
||||||
// unacknowledged 'refresh_token_reuse' queue ATOMICALLY (Round 2 Loop B
|
// unacknowledged 'refresh_token_reuse' queue ATOMICALLY (Round 2 Loop B
|
||||||
// finding 1): without it a register-botnet — N accounts, each rotated
|
// 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
|
// notification centre under unbounded alerts. The cap is folded into
|
||||||
// the INSERT's WHERE clause (count-then-insert is atomic, closing the
|
// the INSERT's WHERE clause (count-then-insert is atomic, closing the
|
||||||
// TOCTOU), and the pre-check logs the suppression for operator
|
// TOCTOU), and the pre-check logs the suppression for operator
|
||||||
|
|||||||
@@ -519,8 +519,8 @@ func TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts(t *testing.T) {
|
|||||||
t.Fatalf("expected 2 refresh tokens in family, got %d", famCount)
|
t.Fatalf("expected 2 refresh tokens in family, got %d", famCount)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Reuse grace window: backdate the original's used_at past the 60s reuse
|
// Reuse grace window: backdate the original's used_at past the reuse grace
|
||||||
// grace so the replay below is genuine theft. WITHOUT this, a replay
|
// so the replay below is genuine theft. WITHOUT this, a replay
|
||||||
// moments after rotation is a benign two-tab concurrent refresh and the
|
// moments after rotation is a benign two-tab concurrent refresh and the
|
||||||
// family must NOT be killed.
|
// family must NOT be killed.
|
||||||
if _, err := tx.Exec(ctx, `
|
if _, err := tx.Exec(ctx, `
|
||||||
@@ -573,7 +573,7 @@ func TestVerifyRefreshToken_ReuseRevokesFamilyAndAlerts(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// TestVerifyRefreshToken_ReplayWithinGrace_IsBenign verifies the reuse
|
// 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
|
// one localStorage refresh token both refreshing on load) is a benign
|
||||||
// concurrent refresh — the generic error is returned, but the rotation family
|
// concurrent refresh — the generic error is returned, but the rotation family
|
||||||
// survives and no refresh_token_reuse alert is raised.
|
// 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
|
// TestAccessTokenKilledWithRotationFamily verifies the HIGH 1 fix: an access
|
||||||
// token minted at rotation is bound (family_id claim) to the rotation family,
|
// 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
|
// so when reuse detection DELETEs the family the access token — which the
|
||||||
|
|||||||
Reference in New Issue
Block a user