fix: exempt PKCE recovery sessions from require-current-password check#2497
Closed
oelmgren wants to merge 1 commit intosupabase:masterfrom
Closed
fix: exempt PKCE recovery sessions from require-current-password check#2497oelmgren wants to merge 1 commit intosupabase:masterfrom
oelmgren wants to merge 1 commit intosupabase:masterfrom
Conversation
staaldraad
requested changes
Apr 23, 2026
| desc: "Current password required for any other claim", | ||
| newPassword: "newpassword456", | ||
| recoveryType: models.EmailChange, | ||
| expected: expected{code: http.StatusBadRequest, isAuthenticated: true}, |
Member
There was a problem hiding this comment.
@oelmgren this isAuthenticated: true is incorrect, it works in the current tests because the step before had changed the password to newpassword456. The change password correctly fails, but the isAuthenticated check passes as true because the "newPassword" is the same as what is currently set in the database (bad test behaviour on my part, having side effects).
Suggested change
| expected: expected{code: http.StatusBadRequest, isAuthenticated: false}, |
Contributor
Contributor
Author
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
3 participants
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.
Bug
UpdatePasswordRequireCurrentPasswordinuser.goonly exempts sessions withotpormagiclinkAMR claims from the current-password requirement. The PKCE recovery flow (the default path for the JS client) issues sessions with therecoveryAMR claim:recover.go:60creates the flow state withmodels.Recovery.String()("recovery")token.go:256parses it viaParseAuthenticationMethod(flowState.AuthenticationMethod)token.go:265issues the refresh token withmodels.RecoveryThe resulting JWT has
"amr": [{"method": "recovery", ...}], which doesn't match"otp"or"magiclink". If the setting is enabled,updateUser({ password })rejects the request with "Current password required" even though the user just came from a password reset email.Fix
Add
"recovery"to the AMR exemption check inuser.go:176.Test
Added a test case for
models.RecoveryAMR confirming current password is not required in PKCE recovery flow.