🍒 [PM-27902] Logout user after successful master password reset #6137
Conversation
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
aj-rosado
requested review from
a team,
brian-livefront and
david-livefront
as code owners
Contributor
|
Claude finished @aj-rosado's task —— View job PR Review: Master Password Reset Security FixThis cherry-pick addresses a critical security vulnerability where users remained logged in after forced password resets. SummaryChanges:
Security Impact: ✅ Critical security issue resolved - users can no longer bypass forced password reset requirements. Code Review✅ Architecture Compliance: Follows MVVM patterns, uses Result types correctly, proper DI usage Findings2 findings identified - see inline comments for details:
Review CompleteThis PR successfully addresses the security vulnerability with a clean, minimal implementation. The cherry-pick is appropriate for the release branch and maintains code quality standards. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| userLogoutManager.logout( | ||
| userId = ACCOUNT_1.profile.userId, | ||
| reason = LogoutReason.PasswordReset, | ||
| ) |
Contributor
There was a problem hiding this comment.
Consider using a matcher to explicitly verify the reason type and properties:
Suggested change
| ) | |
| verify { | |
| userLogoutManager.logout( | |
| userId = ACCOUNT_1.profile.userId, | |
| reason = match { it is LogoutReason.PasswordReset && it.source == null }, | |
| ) | |
| } |
This ensures the correct LogoutReason subtype is used and validates its properties.
|
|
||
| // Log out the user after successful password reset. | ||
| // This clears all user state including forcePasswordResetReason. | ||
| logout(reason = LogoutReason.PasswordReset) |
Contributor
There was a problem hiding this comment.
💭 Finding 2: Order of operations - storing password hash before logout.
The current implementation stores the new password hash (lines 1027-1039) before logging out. If logout were interrupted or failed, the user would have the new hash stored but remain logged in.
Is this ordering intentional to guarantee the new password is always persisted? Or should logout happen first to ensure clean state before any storage operations?
SaintPatrck
approved these changes
Nov 7, 2025
Contributor
|
New Issues (61)Checkmarx found the following issues in this Pull Request
|
david-livefront
approved these changes
Nov 7, 2025
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/2025.10-rc38 #6137 +/- ##
=======================================================
Coverage ? 84.82%
=======================================================
Files ? 721
Lines ? 52825
Branches ? 7667
=======================================================
Hits ? 44809
Misses ? 5328
Partials ? 2688 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
thanks @SaintPatrck and @david-livefront |
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.
🎟️ Tracking
PM-27902
Cherry-picked from #6133
📔 Objective
Fixes a critical security issue where users remained logged in after updating their master password due to policy requirements. This allowed users to immediately access their vault with the new password without re-authentication, bypassing the security intent of forced password resets.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes