feat(tests): add key rotation and sync tests - #1459
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import { | ||
| SecureNoteType, | ||
| isDecryptError, | ||
| type CipherViewType, | ||
| type PasswordManagerClient, | ||
| } from "@bitwarden/sdk-internal"; |
| import type { ClientEmulator } from "../../client-emulator/client-emulator"; | ||
| import { testHarness, type TestHarness } from "../../test-harness"; | ||
| import { MASTER_PASSWORD_ACCOUNT } from "../../vectors/accounts"; | ||
| import { rejection, TEST_EMAIL, TEST_PASSWORD } from "../utils"; |
| }); | ||
| } | ||
|
|
||
| /** An item to carry across the rotation, which re-encrypts the vault under the new key. */ |
There was a problem hiding this comment.
Will be replaced by vectors
🔍 SDK Breaking Change DetectionSDK Version:
Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## km/integration-model-server #1459 +/- ##
============================================================
Coverage 86.23% 86.23%
============================================================
Files 555 555
Lines 80085 80085
============================================================
Hits 69060 69060
Misses 11025 11025 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7e69d33 to
6dda325
Compare
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR is test infrastructure only — no production Rust or binding code changes. It moves the client emulator's Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| for (const cipher of posted.accountData.ciphers ?? []) { | ||
| const stored = this.db.ciphers.get(cipher.id); | ||
| if (stored === undefined) { | ||
| return error(HTTP_NOT_FOUND, `no cipher ${cipher.id} to re-encrypt`); | ||
| } |
There was a problem hiding this comment.
♻️ DEBT: The rotation's cipher and folder writes skip the ownership check every other write in this file applies.
Details and fix
rotateUserKeys resolves items with this.db.ciphers.get(...) and this.db.folders.get(...), while updateCipher goes through reachableCipher and updateFolder compares stored.userId !== user.userId. So a rotation payload that carries an organization cipher — which crates/bitwarden-user-crypto-management/src/key_rotation/data.rs documents must never happen ("Ciphers must be filtered to just contain the user's ciphers, not organization ciphers") — is accepted here and rewrites the org-owned entity under the new user key. The real server refuses that, so a regression in the SDK's filter would still pass this suite.
Suggested fix: resolve each posted cipher with this.reachableCipher(user, cipher.id) and reject one whose userId !== user.userId; apply the same userId check to folders.
While in there: the key, unlock and upgrade-token writes above happen before these lookups, so a 404 on a posted item leaves the account rotated with a partially re-encrypted vault. Validating every referenced item first keeps a rejected rotation from mutating anything.
1 participant
Adds integration tests for key rotation and sync.