PM-41073: Consume breaking changes from SDK making all children of CipherView decrypted

[nikwithak]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-41073

📔 Objective

Consumes breaking SDK changes from bitwarden/sdk-internal#1353.

With these changes, a decrypted CipherView object no longer stores Fido2 creds, cipher keys, attachment keys, or TOTP secrets in a still-encrypted state.

Note: CI builds are failing because the SDK PR hasn't merged yet.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

This revision consumes the breaking SDK change so CipherView.key, AttachmentView.key, FIDO2 credentials, and TOTP secrets arrive fully decrypted, removing the temporary encryptedKey/decryptedKey fields and the manual decrypt_fido2_credentials / decrypt_fido2_private_key calls. The previously reported findings on isLegacyAttachment(), the Cipher.decrypt() item key, the CLI key exposure, the as SymmetricKey casts, and the stale login.view.ts doc comments have all been addressed, and CipherExport.toView now tolerates legacy EncString keys. This pass focused on the fallout of the new CLI redaction and on remaining test alignment. The @bitwarden/sdk-internal pin is still unchanged, which the PR description already calls out as the reason CI is red.

Code Review Details

• ⚠️ : Redacting key in CipherResponse combined with the unconditional view.key overwrite in CipherExport.toView makes bw edit item silently strip a cipher's item key on every edit
  • apps/cli/src/vault/models/cipher.response.ts:25-26
• ⚠️ : attachment.spec.ts still asserts encryptedKey: attachment.key on the decrypted view, which Attachment.decrypt() no longer sets
  • libs/common/src/vault/models/domain/attachment.spec.ts:100

[claude]

⚠️ IMPORTANT: Attachments whose key fails to decrypt are now classified as legacy (pre-key) attachments.

Details and fix

CipherView.fromSdkCipherView builds views for attachmentDecryptionFailures with failure = true. Previously those views still carried encryptedKey (set from obj.key), so isLegacyAttachment() returned false. Now key == null for the same views, so isLegacyAttachment() — and therefore CipherView.hasOldAttachments — returns true for corrupt-key attachments.

Downstream effects for a user with one undecryptable attachment key:

• "Fix old attachments" prompt shows on the vault row (vault-cipher-row.component.ts)
• Key rotation is blocked (key-rotation-dialog.service.ts:140) and the automatic V2 migration is skipped (v2-key-rotation-migration.ts:167)
• transferPersonalItems throws (default-vault-items-transfer.service.ts:234), because upgradeOldCipherAttachments can never clear the flag — the attachment key can't be unwrapped, so the download/re-encrypt step fails

Suggested fix:

isLegacyAttachment(): boolean {
  return this.key == null && !this.hasDecryptionError;
}

[claude]

⚠️ IMPORTANT: Dropping the key copy leaves views from the legacy Cipher.decrypt() path without an item key, which is then lost on re-encryption.

Details and fix

Removing this.key = c.key is correct for the new SymmetricCryptoKey semantics (the wrapped key must not be assigned here), but nothing replaces it on the deprecated decrypt path:

1. Cipher.decrypt() (models/domain/cipher.ts:152) builds new CipherView(this) and unwraps the cipher key into a local cipherKey, but never assigns it to the view — so view.key is now always undefined.
2. That path is still live: CipherService.decryptOrganizationCiphersResponse (cipher.service.ts:657, admin console org ciphers) and the admin attachment refresh in vault-item-dialog.component.ts:553.
3. Saving such a view goes through DefaultCipherEncryptionService.encrypt → toSdkCipherView() → key: undefined, so the SDK no longer receives the existing item key.

Suggested fix in Cipher.decrypt(), where the unwrapped key is already available:

const cipherKey = await encryptService.unwrapSymmetricKey(this.key, userKeyOrOrgKey);
cipherDecryptionKey = cipherKey;
model.key = cipherKey;

[claude]

⚠️ IMPORTANT: The CLI will now print the decrypted item key in bw get item / bw list items output.

Details and fix

CipherResponse (apps/cli/src/vault/models/cipher.response.ts) extends CipherWithIdExport and calls build(cipherView), then serializes the whole object to stdout. Previously this.key was the wrapped key (EncString.encryptedString); with CipherView.key now a SymmetricCryptoKey, this emits the raw base64 item key — key material that can decrypt the item's attachments, written to terminals, shell history, and script logs.

The export services already guard against this (delete cipher.key in individual-vault-export.service.ts:354 and org-vault-export.service.ts:384); the CLI response has no equivalent.

Consider redacting key in CipherResponse's constructor, mirroring the existing viewPassword redaction.

[claude]

🎨 SUGGESTED: SymmetricCryptoKey.toSdk() already returns SymmetricKey, so the cast isn't needed here.

Details

key: this.key?.toSdk(),

toSdk() is toBase64() as SymmetricKey, so this removes the manual assertion and keeps the type checked at the boundary. Same applies at cipher.view.ts:535 (which currently uses as any) and attachment.view.ts:64.

[shane-melton]

Partial review, but Claude has a few good points above.

Also, I think we need to update the LoginView as well now that Fido2Credentials no longer remain encrypted in the Views.

[shane-melton]

⚠️ Any exports that had the EncString version of the key will now fail to import. (The EncString was removed in f167f06, but existed for exports before then).

We'll want to add some type of try/catch or format checking to avoid breaking the entire import just because an old key value is present.

[claude]

⚠️ IMPORTANT: Redacting key here makes bw edit item silently strip the cipher's item key.

Details and fix

The documented CLI edit flow round-trips bw get item output back through bw edit item. EditCommand passes the freshly decrypted view as the target of CipherExport.toView (apps/cli/src/commands/edit.command.ts:143), and toView overwrites key unconditionally:

view.key = req.key != null ? SymmetricCryptoKey.fromString(req.key) : undefined;

Since req.key is now always absent, the item key already present on cipherView is discarded, and toSdkUpdateCipherRequest / toSdkCipherView then send key: undefined. Every CLI edit of a cipher that has an item key drops it. Previously CipherResponse.key carried the wrapped EncString, so the round trip preserved it.

The redaction itself is correct — the unconditional overwrite in toView is the problem. Consider only replacing the key when the request actually supplies one:

if (req.key != null) {
  try {
    view.key = SymmetricCryptoKey.fromString(req.key);
  } catch {
    // Old exports stored the wrapped EncString key, which cannot be used on import
  }
}

BitwardenJsonImporter already sets cipher.key = null immediately after toView, so import behavior is unaffected by the change.

[claude]

⚠️ IMPORTANT: attachment.spec.ts still asserts the removed encryptedKey field, so this test fails.

Details and fix

libs/common/src/vault/models/domain/attachment.spec.ts:93-101 expects:

expect(view).toEqual({
  ...
  key: expect.any(SymmetricCryptoKey),
  encryptedKey: attachment.key,
});

AttachmentView no longer declares encryptedKey and decrypt() no longer assigns it, so the actual view has no such property while the expectation is a defined EncString mock. toEqual only ignores properties that are undefined on both sides, so this assertion fails. Removing the encryptedKey line from the expectation resolves it.