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:
KEK (Key Encryption Key): Derived from user password
DEK (Data Encryption Key): Encrypts actual user credentials
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
Removed the logoutUser() call
Maintain active session with the same DEK
Re-encrypt DEK with new password's KEK
Create proper deep copy of DEK buffer before cleanup
✅User data is ALWAYS preserved
Password Reset Flow
Enhanced to intelligently handle two scenarios:
When logged in (resetting from settings):
Detects active session with existing DEK
Uses new resetUserPasswordWithPreservedDEK() method
Re-encrypts same DEK with new password
✅User data is preserved
When not logged in (forgot password):
No access to old DEK (security by design)
Generates new DEK as before
⚠️Old data becomes inaccessible (expected behavior)
📝 Changes
Modified Files
src/backend/utils/user-crypto.ts
Fixed changeUserPassword() to maintain session
Added resetUserPasswordWithPreservedDEK() method
Improved Buffer handling: Changed from Buffer.allocUnsafe() to safer Buffer.from() (code review fix)
Removed unnecessary KEK salt check in password reset (prevents issues with OIDC users)
Password reset (forgot password) generates new DEK as expected
SSH keys remain accessible after password operations
Database persistence works correctly
🔒 Security Notes
The DEK never changes during password change - only how it's encrypted
Proper memory cleanup with Buffer.fill(0) to prevent leaks
Session management maintains security boundaries
Password reset without login still prevents unauthorized data access
📊 Impact
User Experience: ✅ No more credential loss after password changes
Security: ✅ Maintains encryption integrity
Breaking Changes: ❌ None - backward compatible
Database Migration: ❌ Not required
Before
Change Password → Logout → DEK cleared from memory → Credentials inaccessible ❌
After
Change Password → Session maintained → DEK stays in memory → Credentials accessible ✅
# 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:
- **KEK (Key Encryption Key)**: Derived from user password
- **DEK (Data Encryption Key)**: Encrypts actual user credentials
**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
- Removed the `logoutUser()` call
- Maintain active session with the same DEK
- Re-encrypt DEK with new password's KEK
- Create proper deep copy of DEK buffer before cleanup
- ✅ **User data is ALWAYS preserved**
### Password Reset Flow
Enhanced to intelligently handle two scenarios:
**When logged in (resetting from settings):**
- Detects active session with existing DEK
- Uses new `resetUserPasswordWithPreservedDEK()` method
- Re-encrypts same DEK with new password
- ✅ **User data is preserved**
**When not logged in (forgot password):**
- No access to old DEK (security by design)
- Generates new DEK as before
- ⚠️ **Old data becomes inaccessible** (expected behavior)
## 📝 Changes
### Modified Files
- `src/backend/utils/user-crypto.ts`
- Fixed `changeUserPassword()` to maintain session
- Added `resetUserPasswordWithPreservedDEK()` method
- Improved Buffer handling: Changed from `Buffer.allocUnsafe()` to safer `Buffer.from()` (code review fix)
- Removed unnecessary KEK salt check in password reset (prevents issues with OIDC users)
- `src/backend/utils/auth-manager.ts`
- Exposed `resetUserPasswordWithPreservedDEK()` method
- Added `isUserUnlocked()` helper
- `src/backend/database/routes/users.ts`
- Updated `/users/change-password` route
- Enhanced `/users/complete-reset` to detect active sessions
- Added database save triggers
- Added logout on DEK preservation failure to prevent stale session state (code review fix)
### Documentation
- `PASSWORD_CHANGE_FIX.md` - Comprehensive documentation of the fix
## 🔧 Code Review Improvements
Three additional fixes were applied based on code review:
1. **Session cleanup on DEK preservation failure** (High Priority)
- Added `logoutUser()` when DEK preservation fails and new DEK is created
- Prevents inconsistent state where active session has stale DEK
- Ensures clean state similar to non-logged-in password reset flow
2. **Safer Buffer copying** (Medium Priority)
- Changed from `Buffer.allocUnsafe() + copy()` to `Buffer.from()`
- Eliminates potential memory leak vulnerabilities
- More secure and readable code
3. **Removed dead code** (Medium Priority)
- Removed unused `getKEKSalt()` check in `resetUserPasswordWithPreservedDEK()`
- Old KEK salt not needed when using existing session DEK
- Prevents function failure for OIDC users who don't have KEK salt
## 🧪 Testing
Verified that:
- [x] Password change preserves all credentials
- [x] Password reset (while logged in) preserves credentials
- [x] Password reset (forgot password) generates new DEK as expected
- [x] SSH keys remain accessible after password operations
- [x] Database persistence works correctly
## 🔒 Security Notes
- The DEK never changes during password change - only how it's encrypted
- Proper memory cleanup with Buffer.fill(0) to prevent leaks
- Session management maintains security boundaries
- Password reset without login still prevents unauthorized data access
## 📊 Impact
- **User Experience**: ✅ No more credential loss after password changes
- **Security**: ✅ Maintains encryption integrity
- **Breaking Changes**: ❌ None - backward compatible
- **Database Migration**: ❌ Not required
---
### Before
```
Change Password → Logout → DEK cleared from memory → Credentials inaccessible ❌
```
### After
```
Change Password → Session maintained → DEK stays in memory → Credentials accessible ✅
```
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, and users.ts to implement this preservation logic and update relevant API routes, along with new documentation.
Highlights
Problem Addressed: Previously, changing or resetting a password (while logged in) would cause all encrypted user credentials (like SSH keys) to become inaccessible. This was due to the Data Encryption Key (DEK) being cleared from session memory after a password change.
Solution Overview: This PR introduces a fix to preserve user credentials during password changes and intelligent handling for password resets. The core idea is to maintain the DEK in memory and re-encrypt it with the new password's Key Encryption Key (KEK), rather than clearing it.
Key Changes: The user-crypto.ts file was updated to modify changeUserPassword() 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.ts now exposes this new reset method and includes an isUserUnlocked() helper. The users.ts routes for /change-password and /complete-reset were 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.
Impact: Users will no longer lose their encrypted credentials after changing their password or resetting it while logged in, significantly improving user experience. Security is maintained as the DEK itself doesn't change, only its encryption. There are no breaking changes or database migrations required.
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.
Feature
Command
Description
Code Review
/gemini review
Performs a code review for the current pull request in its current state.
Pull Request Summary
/gemini summary
Provides a summary of the current pull request in its current state.
Comment
@gemini-code-assist
Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help
/gemini help
Displays a list of available commands.
Customization
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.
## Summary of Changes
Hello @thorved, I'm Gemini Code Assist[^1]! 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`, and `users.ts` to implement this preservation logic and update relevant API routes, along with new documentation.
### Highlights
* **Problem Addressed**: Previously, changing or resetting a password (while logged in) would cause all encrypted user credentials (like SSH keys) to become inaccessible. This was due to the Data Encryption Key (DEK) being cleared from session memory after a password change.
* **Solution Overview**: This PR introduces a fix to preserve user credentials during password changes and intelligent handling for password resets. The core idea is to maintain the DEK in memory and re-encrypt it with the new password's Key Encryption Key (KEK), rather than clearing it.
* **Key Changes**: The `user-crypto.ts` file was updated to modify `changeUserPassword()` 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.ts` now exposes this new reset method and includes an `isUserUnlocked()` helper. The `users.ts` routes for `/change-password` and `/complete-reset` were 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.
* **Impact**: Users will no longer lose their encrypted credentials after changing their password or resetting it while logged in, significantly improving user experience. Security is maintained as the DEK itself doesn't change, only its encryption. There are no breaking changes or database migrations required.
<details>
<summary><b>Using Gemini Code Assist</b></summary>
<br>
The full guide for Gemini Code Assist can be found on our [documentation page](https://developers.google.com/gemini-code-assist/docs/review-github-code), here are some quick tips.
<b>Invoking Gemini</b>
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.
Feature | Command | Description
--- | --- | ---
Code Review | `/gemini review` | Performs a code review for the current pull request in its current state.
Pull Request Summary | `/gemini summary` | Provides a summary of the current pull request in its current state.
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help | `/gemini help` | Displays a list of available commands.
<b>Customization</b>
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](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github).
<b>Limitations & Feedback</b>
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with :thumbsup: and :thumbsdown: 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](https://google.qualtrics.com/jfe/form/SV_2cyuGuTWsEw84yG).
<b>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](https://cloud.google.com/products/gemini/code-assist).</b>
</details>
[^1]: Review the [Privacy Notices](https://policies.google.com/privacy), [Generative AI Prohibited Use Policy](https://policies.google.com/terms/generative-ai/use-policy), [Terms of Service](https://policies.google.com/terms), and learn how to configure Gemini Code Assist in GitHub [here](https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github). Gemini can make mistakes, so double check it and [use code with caution](https://support.google.com/legal/answer/13505487).
gemini-code-assist[bot]
(Migrated from github.com)
reviewed 2025-10-08 18:01:31 +11:00
gemini-code-assist[bot]
(Migrated from github.com)
left a comment
Copy Link
Copy Source
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!
## 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!
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.
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.
```suggestion
await authManager.registerUser(userId, newPassword);
authManager.logoutUser(userId);
```
While Buffer.allocUnsafe() followed by DEK.copy() is functionally correct, using Buffer.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.
const dekCopy = Buffer.from(DEK);
While `Buffer.allocUnsafe()` followed by `DEK.copy()` is functionally correct, using `Buffer.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.
```suggestion
const dekCopy = Buffer.from(DEK);
```
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.
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`.
thorved
(Migrated from github.com)
reviewed 2025-10-08 18:11:23 +11:00
Logout on DEK preservation failure (High Priority)
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
1. Logout on DEK preservation failure (High Priority)
File: [users.ts](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html)
Fix: Added [authManager.logoutUser(userId)](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) after [registerUser()](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) 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
thorved
(Migrated from github.com)
reviewed 2025-10-08 18:11:54 +11:00
2. Safer Buffer copying (Medium Priority)
File: [user-crypto.ts](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html)
Fix: Changed from [Buffer.allocUnsafe() + copy()](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) to [Buffer.from(DEK)](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html)
Why: [Buffer.from()](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) is safer and more secure - no risk of leaking old memory data, plus it's more readable
thorved
(Migrated from github.com)
reviewed 2025-10-08 18:12:08 +11:00
Removed dead code (Medium Priority)
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.
3. Removed dead code (Medium Priority)
File: [user-crypto.ts](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html)
Fix: Removed unused [getKEKSalt()](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) check in [resetUserPasswordWithPreservedDEK()](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html)
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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.