Fix: Password Change/Reset Credential Preservation #383
Reference in New Issue
Block a user
Delete Branch "fix/password-change-credential-wipe"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fix: Password Change/Reset Credential Preservation
🐛 Problem
When users changed their password or reset their password (while logged in), all SSH credentials (private keys, public keys, passwords) appeared to be wiped out and became inaccessible.
🔍 Root Cause
The application uses a two-layer encryption system:
The bug: After changing password,
logoutUser()was called which cleared the DEK from the session memory, making all encrypted credentials inaccessible even though they weren't actually deleted.✅ Solution
Password Change Flow
logoutUser()callPassword Reset Flow
Enhanced to intelligently handle two scenarios:
When logged in (resetting from settings):
resetUserPasswordWithPreservedDEK()methodWhen not logged in (forgot password):
📝 Changes
Modified Files
src/backend/utils/user-crypto.tschangeUserPassword()to maintain sessionresetUserPasswordWithPreservedDEK()methodBuffer.allocUnsafe()to saferBuffer.from()(code review fix)src/backend/utils/auth-manager.tsresetUserPasswordWithPreservedDEK()methodisUserUnlocked()helpersrc/backend/database/routes/users.ts/users/change-passwordroute/users/complete-resetto detect active sessionsDocumentation
PASSWORD_CHANGE_FIX.md- Comprehensive documentation of the fix🔧 Code Review Improvements
Three additional fixes were applied based on code review:
Session cleanup on DEK preservation failure (High Priority)
logoutUser()when DEK preservation fails and new DEK is createdSafer Buffer copying (Medium Priority)
Buffer.allocUnsafe() + copy()toBuffer.from()Removed dead code (Medium Priority)
getKEKSalt()check inresetUserPasswordWithPreservedDEK()🧪 Testing
Verified that:
🔒 Security Notes
📊 Impact
Before
After
Summary of Changes
Hello @thorved, I'm Gemini Code Assist1 ! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves a critical issue where user credentials became inaccessible after a password change or reset. The fix ensures that the Data Encryption Key (DEK) is preserved across password updates by re-encrypting it with the new password's Key Encryption Key (KEK), rather than clearing it from memory. This prevents data loss for logged-in users during password changes and intelligently handles password resets to preserve data when possible, while still maintaining expected behavior for 'forgot password' scenarios where a new DEK is generated. The changes involve modifications to
user-crypto.ts,auth-manager.ts, andusers.tsto implement this preservation logic and update relevant API routes, along with new documentation.Highlights
user-crypto.tsfile was updated to modifychangeUserPassword()to keep the DEK in the active session and re-encrypt it with the new KEK, including proper deep copying of the DEK buffer. A new method,resetUserPasswordWithPreservedDEK(), was added to handle password resets when a user is logged in, ensuring data preservation.auth-manager.tsnow exposes this new reset method and includes anisUserUnlocked()helper. Theusers.tsroutes for/change-passwordand/complete-resetwere updated to leverage these new functionalities, detecting active sessions for resets and triggering database saves for persistence. A new documentation file,PASSWORD_CHANGE_FIX.md, was also added.Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either
/gemini <command>or@gemini-code-assist <command>. Below is a summary of the supported commands on the current page./gemini review/gemini summary/gemini helpCustomization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a
.gemini/folder in the base of the repository. Detailed instructions can be found here.Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩︎
Code Review
This is a great pull request that addresses a critical bug in the password change/reset flow. The logic for preserving the user's Data Encryption Key (DEK) is well-thought-out, and the separation of concerns between logged-in and not-logged-in password resets is a solid approach. The added logging and error handling significantly improve the robustness of the crypto operations.
I've identified a few areas for improvement to enhance correctness and security. My comments focus on ensuring consistent session state management, removing redundant code, and adhering to security best practices for buffer handling. Overall, excellent work on this complex fix!
@@ -1323,0 +1344,4 @@userId,username,},);In the failure scenario for preserving the DEK, the user's session is not terminated after re-registering the user with a new DEK. This leaves the user in an inconsistent state, where their active session contains a stale DEK that no longer corresponds to the encrypted data on disk. This could lead to unexpected errors.
To ensure a clean state, the user should be logged out, similar to the flow for a non-logged-in password reset.
While
Buffer.allocUnsafe()followed byDEK.copy()is functionally correct, usingBuffer.allocUnsafe()is generally discouraged in security-sensitive code as it can be a source of vulnerabilities (like leaking old data from memory) if not handled with extreme care.A safer and more readable approach is to use
Buffer.from(buffer), which creates a secure copy of the buffer.This block of code fetches and validates the old
kekSalt, but it's never used. The purpose of this function is to re-encrypt an existing DEK from an active session using a new KEK derived from the new password. The old password's KEK and salt are not required.This is not just dead code; it performs an unnecessary database query and could cause the function to fail unexpectedly for certain user types (e.g., OIDC users) who might not have a
kekSalt.@@ -1323,0 +1344,4 @@userId,username,},);File: users.ts
Fix: Added authManager.logoutUser(userId) after registerUser() in the failure scenario
Why: When DEK preservation fails and a new DEK is created, the old session with stale DEK must be terminated to avoid inconsistent state
File: user-crypto.ts
Fix: Changed from Buffer.allocUnsafe() + copy() to Buffer.from(DEK)
Why: Buffer.from() is safer and more secure - no risk of leaking old memory data, plus it's more readable
File: user-crypto.ts
Fix: Removed unused getKEKSalt() check in resetUserPasswordWithPreservedDEK()
Why: The old KEK salt is not needed - we're using an existing DEK from the session and only need to create a new KEK salt for the new password. This also prevents failures for OIDC users who don't have a KEK salt.