Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Cerarin
force-pushed
the
fix/totp-lifecycle
branch
from
September 15, 2026 09:07
0a43c13 to
db170b7
Compare
Make the authenticator key rotatable and disableable, unify its management UI, and let users hand-edit the setup key. Prove the current authenticator before minting a replacement key and refine the recovery code lifecycle. Enforce step-up authorization from server-side state: - Replacing an existing secret requires a current second factor (via=totp or via=recovery), never the master password alone. - Minting a recovery code for an account with an active TOTP requires a current second factor, closing the first-factor-only mint path.
Cerarin
force-pushed
the
fix/totp-lifecycle
branch
from
September 15, 2026 09:13
db170b7 to
b91a72f
Compare
1 task
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.
Summary
Make the authenticator (TOTP) key rotatable and disableable, unify its management UI, and let users hand-edit the setup key. Most importantly, enforce step-up authorization from server-side state:
RecoveryCodeConsumedflag so the UI can distinguish a fresh mint from a consumed-and-replaced code.Root cause
A security audit of the existing TOTP lifecycle found that first-factor proof (master password + session) was sufficient on several state-changing paths once an authenticator was already active:
PUT /api/two-factor/authenticatornever checked that replacing an existing secret carried a second-factor proof. With the verification token no longer bound to the key (a change made so the key can be hand-edited),masterPasswordHash → via-less token → submit attacker-chosen key + its codewould replace the victim's authenticator and lock them out.PUT /api/accounts/totp) minted a recovery code for accounts with an active TOTP but no stored recovery code — a legacy data shape that predates the TOTP feature — and returned it in plaintext to the caller. Combined with the recovery login, this allowed a full second-factor wipe from the first factor alone.handleGetTotpRecoveryCodeminted and returned a recovery code with only the master password, which is the same attack via a more direct endpoint.The fix derives everything from server-side state via focused helpers (
isTotpRotation,totpRotationRequiresStepUp,recoveryCodeMintRequiresStepUp): whether a commit is a rotation, whether step-up is required, and which second factor authorized it (viais embedded in the HMAC-signeduserVerificationToken, bound to user + securityStamp + 10-minute TTL).Change Type
Cross-File Checklist
CONTRIBUTING.md.migrations/0001_init.sql— N/A, no database schema changes (no new tables/columns; the newRotating/RecoveryCode/RecoveryCodeConsumedfields are response-only extensions).npm run i18n:validatepasses with every locale at 1553 keys.Checks
npx tsc -p tsconfig.json --noEmitnpx tsc -p webapp/tsconfig.json --noEmitnpm run i18n:validate— 10 locales, 1553 keys each, 0 errorsnpm run test:totp-rotation— 22 tests passed (new suite: token integrity, rotation step-up rejection, same-secret mint gating, legacy API paths, recovery-code endpoint)npx tsx --test scripts/totp-secret-validation.test.ts— 5 tests passed (new suite for hand-edited keys)npm run test:config-compatibilitynpm run test:web-cryptonpm run test:notifications-securitynpm run buildNotes
Intentional behavior change (needs maintainer sign-off): clients that disable or replace an active authenticator with the master password alone now receive a 400. Affected endpoints:
POST /api/two-factor/disable,DELETE /api/two-factor/authenticator(both branches of the legacyPUT /api/accounts/totp), and the replacement path ofPUT /api/two-factor/authenticator. They must also send a current authenticator code or the recovery code. First-time enable is unchanged: the master password is still accepted when no authenticator is active. This matches the spirit of the official server, which requires the current 2FA code to disable 2FA, and closes a path where a leaked first factor alone could strip every second factor.Rotation safety: the replacement key never touches storage until
PUT /api/two-factor/authenticatorverifies its code.POST /api/two-factor/get-authenticatorwithregenerate: truemints a key only aftermatchTotpSecondFactor— a read-only comparison that deliberately does not consume the login replay counter (provisioning is not a login). Cancelling the dialog or mistyping the new code leaves the old secret and the recovery code fully usable.Audit trail: replacement commits log
account.totp.rotate(with arecoveryCodeConsumedflag when authorized via recovery code) instead ofaccount.totp.enable, so the log distinguishes first enable from rotation.This PR addresses the issue.
Closes #290