Conversation
Pull Request Test Coverage Report for Build 21705052308Details
💛 - 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" { |
There was a problem hiding this comment.
🟠 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.
| if claim.GetAuthenticationMethod() == "otp" || claim.GetAuthenticationMethod() == "magiclink" { | |
| if claim.GetAuthenticationMethod() == "recovery" { |
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_passwordWhen set, the currentPassword must be sent in the user update request.