fix: require the passcode to disable two-factor authentication - #39168
Open
Se1foo wants to merge 1 commit into
Open
fix: require the passcode to disable two-factor authentication#39168Se1foo wants to merge 1 commit into
Se1foo wants to merge 1 commit into
Conversation
Disabling TOTP performed no re-authentication: the handler checked the feature flag, loaded the enrollment and deleted it. Reaching an already authenticated session was enough to strip MFA from an account. Verify the current passcode, accepting the single-use recovery key as well so that a lost authenticator does not lock the user out. Assisted-by: Claude:claude-opus-5
GiteaBot
added
the
lgtm/need 2
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new disable-2FA verification path should cap/validate passcode length before PBKDF2 scratch-token verification to avoid unnecessary CPU amplification on large inputs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR closes #27690 by requiring a TOTP passcode (or the single-use recovery key) to disable two-factor authentication, preventing MFA removal from an already-authenticated session without re-verification.
Changes:
- Add server-side passcode (TOTP) verification before deleting a user’s 2FA enrollment, with recovery-key fallback.
- Update the security settings UI to request the passcode/recovery key when disabling 2FA.
- Extend the integration test suite to cover the new “disable 2FA requires passcode” behavior.
File summaries
| File | Description |
|---|---|
| tests/integration/user_settings_test.go | Adds an integration test ensuring disabling TOTP requires a valid TOTP code or recovery key. |
| templates/user/settings/security/twofa.tmpl | Adds a required passcode/recovery-key input to the disable-2FA form. |
| routers/web/user/setting/security/2fa.go | Enforces passcode validation (TOTP or recovery key) prior to deleting 2FA enrollment. |
| options/locale/locale_en-US.json | Adds new localized strings for the disable-2FA passcode label and error message. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+86
to
+95
| passcode := ctx.FormString("passcode") | ||
| valid, err := t.ValidateAndConsumeTOTP(ctx, passcode) | ||
| if err != nil { | ||
| ctx.ServerError("SettingsTwoFactor: Failed to ValidateAndConsumeTOTP", err) | ||
| return | ||
| } | ||
| if !valid && !t.VerifyScratchToken(passcode) { // recovery key, for a lost authenticator | ||
| ctx.JSONError(ctx.Tr("settings.twofa_disable_passcode_incorrect")) | ||
| return | ||
| } |
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.
Disabling TOTP performed no re-authentication: the handler checked the feature flag, loaded the enrollment and deleted it. Reaching an already authenticated session — an unattended browser, a stolen session cookie — was enough to strip MFA from an account.
The passcode is now verified before the enrollment is deleted, the way password reset already does it, and the settings form gains a field for it. The single-use recovery key is accepted too, so a lost authenticator does not lock the user out. A password is deliberately not used: OAuth2/LDAP accounts may not have one set, while every account reaching this handler is enrolled in TOTP. Admins can still reset a user's 2FA.
Extends
TestUserSettingsSecurity; the added assertions fail onmain.regenerate_scratchhas the same missing check, left out to keep this focused.Closes #27690