feat(core,redis,types): refresh-token reuse detection + reset-time access-token invalidation#71
Open
msalvatti wants to merge 2 commits into
Open
feat(core,redis,types): refresh-token reuse detection + reset-time access-token invalidation#71msalvatti wants to merge 2 commits into
msalvatti wants to merge 2 commits into
Conversation
…ation Refresh rotation used a 30s grace window but never detected the replay of an already-consumed token: an in-grace replay forked a parallel session and a post-grace replay was rejected as a plain invalid, so a stolen refresh token stayed usable and theft was never surfaced (OWASP rotation with automatic reuse detection was not implemented). Track a refresh-token family (login lineage) minted at login and inherited unchanged across every rotation. On rotation the store now plants a consumed-token marker (`cf:`) that outlives the grace pointer and moves the hash in a family index (`fam:`). Presenting a consumed token past its grace window is therefore caught as a reuse: the store returns RotateOutcome::Reused carrying the compromised family, and the token manager revokes the entire family (every live descendant) before rejecting, forcing re-authentication. The atomic-rotation + grace-window concurrency semantics are preserved: an in-grace replay still recovers, and only a post-grace replay trips reuse. Legacy records with no family are handled gracefully (no marker, no index, never a reuse target). - SessionRecord gains `family_id` (serde-default, omitted when empty) - RotateOutcome gains `Reused(family_id)`; SessionStore gains `revoke_family` - refresh_rotate.lua plants the consumed marker + family index and detects reuse; new revoke_family.lua deletes a lineage atomically - in-memory test store mirrors the semantics for the core unit tier Tests prove: post-grace reuse revokes the live descendant (dashboard, platform, in-memory and Redis tiers); an in-grace concurrent rotation still succeeds; and a legacy no-family session never trips reuse. Coverage stays 100% on every changed file; clippy, cargo-deny, and the security-invariant gate pass.
There was a problem hiding this comment.
Pull request overview
Implements refresh-token rotation reuse detection via token families and adds a revoke_family primitive so that a post-grace replay of a consumed refresh token can revoke the entire login lineage across Redis and in-memory stores.
Changes:
- Add
SessionRecord.family_id(serde-defaulted, omitted when empty) and extend rotation results withRotateOutcome::Reused(String). - Implement Redis reuse detection (
cf:consumed markers +fam:family index) and family revocation (revoke_family.lua). - Update token manager + tests to revoke a family on reuse detection and validate behavior across tiers.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/bymax-auth-redis/tests/redis_stores.rs | Extends Redis store integration tests to cover reuse detection, family revocation, and legacy no-family behavior. |
| crates/bymax-auth-redis/src/stores/session.rs | Wires new prefixes, parses REUSED: replies, writes fam: membership on create, and exposes revoke_family. |
| crates/bymax-auth-redis/src/script.rs | Registers the new revoke_family.lua script. |
| crates/bymax-auth-redis/src/lua/revoke_family.lua | Adds atomic family revocation transaction over the family index. |
| crates/bymax-auth-redis/src/lua/refresh_rotate.lua | Extends rotation script to plant cf: markers, maintain fam: membership, and return REUSED: on post-grace replay. |
| crates/bymax-auth-redis/src/keys.rs | Adds new Redis prefixes: cf/fam and pcf/pfam. |
| crates/bymax-auth-core/src/traits/store.rs | Adds family_id to SessionRecord, adds RotateOutcome::Reused, and adds SessionStore::revoke_family. |
| crates/bymax-auth-core/src/testing/mod.rs | Mirrors reuse detection and family revocation in InMemoryStores and adds tests. |
| crates/bymax-auth-core/src/services/token_manager.rs | Mints a family at login, preserves it on rotation, and revokes the family on Reused. |
| crates/bymax-auth-core/src/services/session.rs | Updates test records / store stubs to satisfy the expanded trait. |
| crates/bymax-auth-core/src/services/platform.rs | Updates logout-related test to account for reuse detection semantics. |
| crates/bymax-auth-core/src/services/mfa/challenge.rs | Ensures hook/eviction projection records keep family_id empty (server-internal). |
| crates/bymax-auth-core/src/services/auth/password_reset.rs | Updates test record construction for new family_id field. |
| crates/bymax-auth-core/src/services/auth/mod.rs | Keeps family_id empty on display-only record used for hooks/evictions. |
| crates/bymax-auth-axum/tests/common/mod.rs | Updates failing-store adapter to implement revoke_family. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
49
to
52
| local grace = redis.call('GET', KEYS[3]) | ||
| if grace then | ||
| return 'GRACE:' .. grace | ||
| end |
Comment on lines
+163
to
+166
| // Register the session in its family index (skipped for a legacy record with no family), | ||
| // so the whole lineage can be revoked on reuse detection. The index carries the refresh | ||
| // TTL so it ages out with the sessions it tracks. | ||
| if !detail.family_id.is_empty() { |
Comment on lines
+496
to
+516
| async fn revoke_family(&self, kind: SessionKind, family_id: &str) -> Result<(), AuthError> { | ||
| // Idempotent: an empty, unknown, or already-cleared family drops nothing. | ||
| if family_id.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| let Some(hashes) = lock(&self.families).remove(&(kind, family_id.to_owned())) else { | ||
| return Ok(()); | ||
| }; | ||
| let mut sessions = lock(&self.sessions); | ||
| let mut index = lock(&self.session_index); | ||
| for hash in hashes { | ||
| // Every live descendant of the compromised login is deleted, and pruned from its | ||
| // owner's session index (all family members share one user). | ||
| if let Some(record) = sessions.remove(&(kind, hash.clone())) | ||
| && let Some(details) = index.get_mut(&(kind, record.user_id.clone())) | ||
| { | ||
| details.retain(|detail| detail.session_hash != hash); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
…er-user epoch A password reset revoked the refresh sessions but left already-issued, stateless access JWTs valid for the remainder of their (up to 15-minute) lifetime: the jti-blacklist used on logout can only revoke a token you hold, and a reset does not possess the user's active access-token jtis — there is no per-user registry of them. Add a per-user token **epoch** (generation counter): stamped into the Dashboard/Platform claims at issuance (and rotation), read on every verification, and bumped on a password reset. A token stamped below the user's current epoch was issued before the reset and is now rejected on its next request, so a reset takes effect immediately instead of lingering for the access-token lifetime. The refresh sessions are revoked as before. Backward-safe: a legacy token with no epoch claim and a user with no stored epoch both read as 0, so the mechanism is inert (0 < 0 is false) until a first bump — no existing session is locked out. The epoch is server-internal (never surfaced in AuthResult); the edge WASM verifier carries the claim but does not consult it (no store), exactly like the jti-blacklist. - DashboardClaims/PlatformClaims gain `epoch` (serde-default); TS bindings regenerated - SessionStore gains current_epoch/bump_epoch; keys ep/pep; redis + in-memory impls - the dashboard password-reset flow bumps the epoch after revoking sessions Scope note: MFA-state changes and session revoke-all keep their existing "revoke other refresh sessions, current session continues" semantics and do not bump the epoch; the platform epoch mechanism is complete and symmetric, awaiting a platform credential-reset flow to trigger it. Tests prove a pre-reset access token is rejected after the bump (dashboard and platform), that a post-bump token still verifies, that the reset flow advances the epoch, and that a legacy no-epoch token stays valid. Coverage stays 100% on every changed file; clippy, cargo-deny, ts-export staleness, and the security-invariant gate pass.
msalvatti
changed the title
Comment on lines
+163
to
+176
| // Register the session in its family index (skipped for a legacy record with no family), | ||
| // so the whole lineage can be revoked on reuse detection. The index carries the refresh | ||
| // TTL so it ages out with the sessions it tracks. | ||
| if !detail.family_id.is_empty() { | ||
| let fam_key = keys.key(prefixes.fam, &detail.family_id); | ||
| pipe.cmd("SADD") | ||
| .arg(&fam_key) | ||
| .arg(token_hash) | ||
| .ignore() | ||
| .cmd("EXPIRE") | ||
| .arg(&fam_key) | ||
| .arg(ttl_window) | ||
| .ignore(); | ||
| } |
Comment on lines
+475
to
+478
| /// TTL applied to a token-epoch key, in seconds (30 days). It must comfortably exceed the | ||
| /// longest an access token can live so an epoch bump stays in force for every pre-bump token's | ||
| /// remaining lifetime; a small fixed integer key per reset-affected user is negligible. | ||
| const EPOCH_TTL_SECS: u64 = 30 * 24 * 60 * 60; |
Comment on lines
160
to
+177
| @@ -168,6 +174,7 @@ | |||
| mfa_verified, | |||
| iat: now, | |||
| exp: now.saturating_add(self.access_ttl.as_secs().min(i64::MAX as u64) as i64), | |||
| epoch, | |||
Comment on lines
537
to
+552
| pub async fn verify_access(&self, token: &str) -> Result<DashboardClaims, AuthError> { | ||
| let claims = verify::<DashboardClaims>(token, &self.key, &VerifyOptions::default()) | ||
| .map_err(map_jwt_error)?; | ||
| if self.session_store.is_blacklisted(&claims.jti).await? { | ||
| return Err(AuthError::TokenRevoked); | ||
| } | ||
| // A token stamped below the user's current epoch predates an invalidating event (a | ||
| // password reset or sign-out-everywhere) and is revoked. | ||
| if claims.epoch | ||
| < self | ||
| .session_store | ||
| .current_epoch(SessionKind::Dashboard, &claims.sub) | ||
| .await? | ||
| { | ||
| return Err(AuthError::TokenRevoked); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two access-token-invalidation gaps found during a security assessment, closed in one branch. Both are additive, backward-safe, and keep every gate green.
1. Refresh-token reuse detection with family revocation
Refresh rotation used a 30s grace window but never detected the replay of an already-consumed token: an in-grace replay forked a parallel session and a post-grace replay was rejected as a plain
Invalid. A stolen refresh token therefore stayed usable and the theft was never surfaced.Design — token-family lineage. A refresh-token family id is minted at login and inherited unchanged across every rotation. On rotation the store plants a consumed-token marker
cf:{sha256(old)} = family(TTL = refresh lifetime, outliving the grace pointer) and moves the hash in a family indexfam:{family}. Rotation outcomes:RotatedGrace(unchanged concurrency tolerance)Reused(family)— a replayed stolen tokenInvalidOn
Reusedthe token manager callsrevoke_family(deletes every live descendant'srt:/sd:, prunessess:, dropsfam:) then rejects, forcing the whole lineage to re-authenticate. Only a post-grace replay trips detection; in-grace concurrency still recovers. Legacy records with no family are never a reuse target.New surface:
SessionRecord.family_id,RotateOutcome::Reused,SessionStore::revoke_family, Rediscf/fam(+pcf/pfam),revoke_family.lua, rewrittenrefresh_rotate.lua.2. Stateless access-token invalidation on reset via a per-user epoch
A password reset revoked the refresh sessions but left already-issued stateless access JWTs valid for the rest of their (≤15-minute) lifetime: the jti-blacklist used on logout can only revoke a token you hold, and a reset does not possess the user's active access-token jtis.
Design — per-user token epoch. A per-user generation counter is stamped into the Dashboard/Platform claims at issuance and rotation, read on every verification, and bumped by the password reset. A token stamped below the user's current epoch predates the reset and is rejected on its next request — so the reset takes effect immediately, not after the token expires.
Backward-safe: a legacy token with no
epochclaim and a user with no stored epoch both read as0, so the check (token.epoch < stored) is inert until a first bump — no existing session is locked out. The epoch is server-internal (never inAuthResult); the edge WASM verifier carries the claim but does not consult it, exactly like the jti-blacklist.New surface:
DashboardClaims.epoch/PlatformClaims.epoch(serde-default; TS bindings regenerated),SessionStore::current_epoch/bump_epoch, Redisep/pep. Scope note: MFA-state changes and session revoke-all keep their existing "revoke other refresh sessions, current session continues" semantics and do not bump the epoch; the platform epoch mechanism is complete and symmetric, awaiting a platform credential-reset flow to trigger it.Testing & gates (both changes)
cargo fmt·clippy --workspace --all-targets --all-features -D warnings· full workspace test (--all-features, incl. the testcontainers Redis tier) ·cargo deny check· ts-export staleness · the security-invariant gate — all green.cargo llvm-cov).Follow-ups
@bymax-one/nest-authneeds the parallel changes to keep shared-Redis contract parity.