Skip to content

feat: check current password on change#2364

Open
staaldraad wants to merge 4 commits intomasterfrom
sec-601-password-change-function
Open

feat: check current password on change#2364
staaldraad wants to merge 4 commits intomasterfrom
sec-601-password-change-function

Conversation

@staaldraad
Copy link
Member

What kind of change does this PR introduce?

feature - allows requiring current password when changing the password

What is the new behavior?

Adds config flag update_password_require_current_password
When set, the currentPassword must be sent in the user update request.

@staaldraad staaldraad requested a review from a team as a code owner February 4, 2026 12:01
@coveralls
Copy link

coveralls commented Feb 4, 2026

Pull Request Test Coverage Report for Build 21705052308

Details

  • 19 of 21 (90.48%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 68.869%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/user.go 19 21 90.48%
Totals Coverage Status
Change from base Build 21450104510: 0.02%
Covered Lines: 14915
Relevant Lines: 21657

💛 - Coveralls

for _, claim := range session.AMRClaims {
// password recovery flows can be via otp or a magic link, check if the current session
// was created with one of those
if claim.GetAuthenticationMethod() == "otp" || claim.GetAuthenticationMethod() == "magiclink" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Severity: HIGH

The AMR check to exempt recovery sessions is overly broad. Since 'otp' and 'magiclink' are used for standard passwordless sign-ins, any user authenticated via these methods can bypass the 'require current password' policy. This allow an attacker with access to a passwordless session to change the account password without knowing the existing one.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: Change the AMR check to specifically look for the 'recovery' authentication method instead of the overly broad 'otp' or 'magiclink' checks. The 'recovery' authentication method (models.Recovery) is the specific marker for password recovery flows, while 'otp' and 'magiclink' are used for standard passwordless sign-ins. This prevents users with regular OTP or MagicLink sessions from bypassing the current password requirement.

⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.

Suggested change
if claim.GetAuthenticationMethod() == "otp" || claim.GetAuthenticationMethod() == "magiclink" {
if claim.GetAuthenticationMethod() == "recovery" {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants