[PM-29017] - improve logic for cipher SaveDetailsAsync validation #6731
Conversation
jaasen-livefront
changed the title
|
Claude finished @jaasen-livefront's task in 1m 50s —— View job PR Review Complete ✓I've reviewed the changes in this PR that improve the cipher SaveDetailsAsync validation logic. SummaryThis PR fixes an issue where users without hidden password permission would receive a 400 error when attempting to save a cipher that didn't have a key. The fix changes the validation logic from rejecting the operation to silently preserving the existing key value. Issues Found🔴 Critical: Test Logic ErrorFile: The test var deps = GetSaveDetailsAsyncDependencies(
sutProvider,
newPassword: "NewPassword",
permission: true, // ❌ Should be false
key: "NewKey"
);This means the test is not actually validating the behavior described by PM-29017. The test currently verifies that when a user has permission and provides a new key, the key gets reset to the original - which doesn't match the scenario being fixed. Expected: The test should use 📝 Minor: Code Style IssueFile: Double semicolon on line 1033: cipher.Key = existingCipher.Key; ; // Extra semicolonShould be: cipher.Key = existingCipher.Key;📝 Minor: Typo in CommentFile: Line 1034 has a typo: "fileds" should be "fields" // Keep only non-hidden fileds from the new cipherRecommendations
Security Assessment✅ The approach of silently preserving existing values for hidden-password users is appropriate and maintains zero-knowledge security principles. The change is less disruptive than throwing an error and still prevents unauthorized modifications. Review Summary
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| throw new BadRequestException("You do not have permission to add cipher key encryption."); | ||
| } | ||
| // For hidden-password users, never allow Key to change at all. | ||
| cipher.Key = existingCipher.Key; ; |
There was a problem hiding this comment.
| } | ||
| // For hidden-password users, never allow Key to change at all. | ||
| cipher.Key = existingCipher.Key; ; | ||
| // Keep only non-hidden fileds from the new cipher |
There was a problem hiding this comment.
📝 Typo: "fileds" should be "fields"
| var deps = GetSaveDetailsAsyncDependencies( | ||
| sutProvider, | ||
| newPassword: "NewPassword", | ||
| permission: true, |
There was a problem hiding this comment.
❌ Test logic error: This test is named SaveDetailsAsync_CipherKeyNotChangedWithoutPermission but passes permission: true on this line.
The test should pass permission: false to properly validate the scenario where a user without hidden password permission attempts to change the key. Currently, this test is not validating the fix described in PM-29017.
| permission: true, | |
| permission: false, |
|
Great job! No new security vulnerabilities introduced in this pull request |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29017
📔 Objective
This PR fixes an issue where the server would return a 400 if a user without hidden pw permission attempted to save a cipher that didn't currently have a key.
Some tests were updated and to more accurately test the new logic.
Note
This presumes a user without hidden pw permission should still be able to update non password fields even if the key is absent from the cipher for some reason.
📸 Screenshots
⏰ 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