fix: TOTP clock-drift tolerance with replay prevention#3601
Open
fix: TOTP clock-drift tolerance with replay prevention#3601
Conversation
Expand 2FA validation to accept codes from the previous and next 30-second time windows in addition to the current one. This handles clock drift between the user's device and the server, which can otherwise cause confusing validation failures when the user's time is off by a few seconds or when entering a code near a time-step boundary. ±1 tolerance is the RFC 6238 recommendation and matches the behavior of all major 2FA providers.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds time-step-aware TOTP handling with replay protection and atomic last-used tracking, persists a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as UserMfaController
participant Service as UserAccountService
participant Mfa as MfaService
participant Repo as UserAccountRepository
participant DB as Database
Client->>Controller: POST /mfa/enable (initialOtp, key)
Controller->>Service: enableMfaTotp(user, key, initialOtp)
Service->>Mfa: findMatchingTimeStep(key, initialOtp)
Mfa-->>Service: matchedTimeStep or null
alt matched
Service->>Repo: save totpKey + totpLastUsedTimeStep = matchedTimeStep
Repo->>DB: INSERT/UPDATE row
DB-->>Repo: OK
Repo-->>Service: persisted UserAccount
Service-->>Controller: success
Controller-->>Client: 200 OK
else not matched
Service-->>Controller: ValidationException
Controller-->>Client: 400 Invalid OTP
end
Client->>Controller: POST /mfa/consume (code)
Controller->>Service: consumeTotpCode(user, code)
Service->>Mfa: findMatchingTimeStep(key, code, minExclusive = user.totpLastUsedTimeStep)
Mfa-->>Service: matchedStep or null
alt matchedStep and > lastUsed
Service->>Repo: updateTotpLastUsedTimeStepIfGreater(id, matchedStep)
Repo->>DB: atomic UPDATE WHERE totp_last_used_time_step IS NULL OR < matchedStep
DB-->>Repo: rowsAffected (1 or 0)
alt rowsAffected == 1
Repo-->>Service: success -> allowed
Service-->>Controller: consumed OK
Controller-->>Client: 200 OK
else rowsAffected == 0
Repo-->>Service: conflict -> AuthenticationException (replay/race)
Service-->>Controller: reject
Controller-->>Client: 401 Replay detected
end
else not matched
Service-->>Controller: AuthenticationException
Controller-->>Client: 401 Invalid OTP
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/data/src/main/kotlin/io/tolgee/service/security/MfaService.kt (1)
145-159: Optional: flip the two TOTP helpers.
generateStringCode()is the caller but currently sits belowgenerateCode(), so the file reads bottom-up at the end. Swapping them would match the Stepdown Rule used for Kotlin files here.As per coding guidelines, "Functions should be ordered so that a caller appears before the functions it calls."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/data/src/main/kotlin/io/tolgee/service/security/MfaService.kt` around lines 145 - 159, Move the TOTP helper functions so the caller appears before the callee: place generateStringCode(...) above generateCode(...). Update their order within the MfaService class so generateStringCode calls generateCode and then generates/pads the string, followed by generateCode which computes the Int TOTP using totpGenerator and SecretKeySpec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/data/src/main/kotlin/io/tolgee/service/security/MfaService.kt`:
- Around line 137-150: The TOTP validation currently calls generateCode(key,
offset) three times which each calls Instant.now(), causing flakiness near
time-step boundaries; update validateTotpCode to read Instant.now() once into a
local Instant and pass that fixed timestamp into generateCode (add an overload
or parameter like timestamp: Instant to generateCode) so all three comparisons
use the same reference time; update generateCode to call
totpGenerator.generateOneTimePassword(SecretKeySpec(key,
SecurityConfiguration.OTP_ALGORITHM), timestamp) with the passed Instant and
adjust tests to use the injectable timestamp path.
---
Nitpick comments:
In `@backend/data/src/main/kotlin/io/tolgee/service/security/MfaService.kt`:
- Around line 145-159: Move the TOTP helper functions so the caller appears
before the callee: place generateStringCode(...) above generateCode(...). Update
their order within the MfaService class so generateStringCode calls generateCode
and then generates/pads the string, followed by generateCode which computes the
Int TOTP using totpGenerator and SecretKeySpec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b33d0458-35d8-4f26-bf0f-b21c83e60c1b
📒 Files selected for processing (3)
backend/app/build.gradlebackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/UserMfaControllerTest.ktbackend/data/src/main/kotlin/io/tolgee/service/security/MfaService.kt
💤 Files with no reviewable changes (1)
- backend/app/build.gradle
The previous commit widened TOTP validation to the ±1 time step window to tolerate clock drift, which also widened the theoretical replay window of a captured code. Track the last accepted absolute time step counter per user and reject any authentication whose matched counter is not strictly greater, using a conditional atomic UPDATE to stay safe under concurrent requests. The enable flow seeds the counter from the matched step so the enable-time code cannot be replayed against the login endpoint, and the recovery-code path burns the current TOTP window so a captured code cannot be replayed after a recovery-code authentication.
Anty0
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.kt (1)
341-363: Consider movingnotifySelfto follow the Stepdown Rule.The private helper
notifySelfis placed between public methods (disableMfaTotpandconsumeTotpCode). Per the Stepdown Rule, private helpers should appear after the public methods that call them.Suggested reorder
Move
notifySelfto afterupdatePassword(around line 564) or group it with other private methods likeresetTokensValidNotBefore. This keeps high-level public API at the top with implementation details below.As per coding guidelines: "Functions should be ordered so that a caller appears before the functions it calls."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.kt` around lines 341 - 363, The private helper notifySelf is currently interleaved between public methods (it sits between disableMfaTotp and consumeTotpCode); move notifySelf so it follows the Stepdown Rule by placing it after the public API methods that call it (for example after updatePassword or grouped with other private helpers like resetTokensValidNotBefore), ensuring all public methods (disableMfaTotp, consumeTotpCode, updatePassword, etc.) appear before notifySelf and other private helper functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@backend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.kt`:
- Around line 341-363: The private helper notifySelf is currently interleaved
between public methods (it sits between disableMfaTotp and consumeTotpCode);
move notifySelf so it follows the Stepdown Rule by placing it after the public
API methods that call it (for example after updatePassword or grouped with other
private helpers like resetTokensValidNotBefore), ensuring all public methods
(disableMfaTotp, consumeTotpCode, updatePassword, etc.) appear before notifySelf
and other private helper functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db63eb80-1d02-4e44-b587-b3fe3338eba9
📒 Files selected for processing (6)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/UserMfaControllerTest.ktbackend/data/src/main/kotlin/io/tolgee/model/UserAccount.ktbackend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.ktbackend/data/src/main/kotlin/io/tolgee/service/security/MfaService.ktbackend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.ktbackend/data/src/main/resources/db/changelog/schema.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/UserMfaControllerTest.kt
Two tests started failing in CI after replay prevention landed: - `UserMfaControllerTest.it regenerates MFA recovery codes` hit a pre-existing JWT/`tokens_valid_not_before` race — the test helper `enableMfa()` bumps `tokens_valid_not_before` via `resetTokensValidNotBefore`, and when the wall clock crossed a second boundary between the earlier `loginAsAdmin` and that bump, the JWT's `iat` became strictly less than the new `tokens_valid_not_before` and the next `performAuth*` call 401-ed. Refresh the JWT defensively inside the helper. - The `Asks for OTP before operation` Cypress spec called the internal `get-totp` helper twice per run — once for pepa's login and once for the sensitive-operation dialog — and within a single 30-second window both calls returned the same 6-digit code. Replay prevention now correctly rejects the second use. Reset pepa's `totpLastUsedTimeStep` inside the test-only helper so each `get-totp` call yields a code that will be accepted regardless of what the previous call burned.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/UserMfaControllerTest.kt (2)
398-399: Minor: Comment referencesenableMfabutenableMfaAtFixedTimewas called.The comment is technically correct since
enableMfaAtFixedTime()delegates toenableMfa(), but for clarity:📝 Suggested clarification
- // `enableMfa` cleared the JWT-valid-from marker, so refresh the test's auth token. + // `enableMfaAtFixedTime` (via `enableMfa`) cleared the JWT-valid-from marker, so refresh the test's auth token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/UserMfaControllerTest.kt` around lines 398 - 399, Update the inline comment to reference enableMfaAtFixedTime instead of enableMfa for clarity: change the comment above the loginAsUser(persistedUser()) call to mention that enableMfaAtFixedTime cleared the JWT-valid-from marker, so the test's auth token is refreshed; reference enableMfaAtFixedTime and loginAsUser/persistedUser in the comment.
336-336: Consider clarifying or removing the sanity-check assertion.The assertion
assertThat(advancedStep).isLessThan(postReEnableStep + 1L)is mathematically equivalent toadvancedStep <= postReEnableStep. Given thatpostReEnableStepis exactly one time step afteradvancedStepdue to the 30-second advancement, this will always trivially pass.If the intent is to verify the counter reset actually allowed forward progress past the old high-water mark, consider a clearer assertion:
♻️ Suggested clarification
- assertThat(advancedStep).isLessThan(postReEnableStep + 1L) // sanity: values are comparable + // Verify that after disable/re-enable, the counter can advance past where it was before + assertThat(postReEnableStep).isGreaterThanOrEqualTo(advancedStep)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/UserMfaControllerTest.kt` at line 336, The current sanity-check assertion using assertThat(advancedStep).isLessThan(postReEnableStep + 1L) is effectively tautological; replace it with a clearer intent check in UserMfaControllerTest.kt that uses the existing variables (advancedStep, postReEnableStep). Either assert forward progress explicitly (e.g., assert that postReEnableStep is greater than advancedStep) to prove the counter moved forward, or assert the exact expected increment (e.g., postReEnableStep equals advancedStep + 1) if the step advance is deterministic; remove the original tautological assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/SensitiveOperationProtectionE2eDataController.kt`:
- Line 45: Typo in the inline comment inside
SensitiveOperationProtectionE2eDataController: replace "cadency then" with
"cadence than" (or better "cadence than once every 30s") so the comment reads
clearly; locate the comment mentioning "Reset OTP replay protection" and update
the wording to "cadence than" to fix the spelling and comparative usage.
- Line 46: The current safe-call on userAccountRepository.findActive("pepa")
makes the totpLastUsedTimeStep reset a silent no-op if the "pepa" account is
missing; change this to fail fast by asserting presence and throwing when null
(e.g., use requireNotNull(userAccountRepository.findActive("pepa")) or check
result and throw IllegalStateException) before setting totpLastUsedTimeStep =
null so tests error immediately and not later with replay failures.
---
Nitpick comments:
In
`@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/UserMfaControllerTest.kt`:
- Around line 398-399: Update the inline comment to reference
enableMfaAtFixedTime instead of enableMfa for clarity: change the comment above
the loginAsUser(persistedUser()) call to mention that enableMfaAtFixedTime
cleared the JWT-valid-from marker, so the test's auth token is refreshed;
reference enableMfaAtFixedTime and loginAsUser/persistedUser in the comment.
- Line 336: The current sanity-check assertion using
assertThat(advancedStep).isLessThan(postReEnableStep + 1L) is effectively
tautological; replace it with a clearer intent check in UserMfaControllerTest.kt
that uses the existing variables (advancedStep, postReEnableStep). Either assert
forward progress explicitly (e.g., assert that postReEnableStep is greater than
advancedStep) to prove the counter moved forward, or assert the exact expected
increment (e.g., postReEnableStep equals advancedStep + 1) if the step advance
is deterministic; remove the original tautological assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b37df762-0054-4ba5-9954-d7bd1f116563
📒 Files selected for processing (2)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/UserMfaControllerTest.ktbackend/development/src/main/kotlin/io/tolgee/controllers/internal/e2eData/SensitiveOperationProtectionE2eDataController.kt
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…en user not found
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
1 participant
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.
This PR was made in preparation for implementing support for WebAuthn (HW MFA tokens).
Summary
UserAccountand rejecting any authentication whose matched counter is not strictly greater.updateTotpLastUsedTimeStepIfGreater) so concurrent requests with the same code are handled without row locks: the first commit wins and the loser is rejected as a replay.enableMfaTotpso the same code used to enable MFA cannot be replayed against the login endpoint immediately afterwards.consumeMfaRecoveryCodeso a previously captured TOTP cannot be replayed after a recovery-code authentication.disableMfaTotpso re-enabling starts from a clean slate./api/public/generatetokenand/v2/user/generate-super-token.Summary by CodeRabbit