Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nikwithak wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be5a7a1
PM-41073: Consume breaking changes from SDK making all children of Ci…
nikwithak Aug 5, 2026
33b19b4
Consume SDK changes for decrypted cipher/attachment/fido2/totp keys
nikwithak Aug 7, 2026
5da5fc5
Consume new SymmetricKey type for CipherEditREquest
nikwithak Aug 10, 2026
55982bc
Merge branch 'main' of https://github.com/bitwarden/clients into vaul…
nikwithak Aug 11, 2026
f8d0e2a
Address PR feedback
nikwithak Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions ...common/src/key-management/encrypted-migrator/migrations/v2-key-rotation-migration.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import { UserKey } from "../../../types/key";
import { CipherService } from "../../../vault/abstractions/cipher.service";
import { AttachmentView } from "../../../vault/models/view/attachment.view";
import { CipherView } from "../../../vault/models/view/cipher.view";
import { EncString } from "../../crypto/models/enc-string";
import { MasterPasswordServiceAbstraction } from "../../master-password/abstractions/master-password.service.abstraction";

import { V2KeyRotationMigration } from "./v2-key-rotation-migration";
Expand Down Expand Up @@ -44,11 +43,9 @@ describe("V2KeyRotationMigration", () => {
return cipher;
};

const makeAttachment = (hasEncryptedKey: boolean): AttachmentView => {
const makeAttachment = (hasKey: boolean): AttachmentView => {
const a = new AttachmentView();
a.encryptedKey = hasEncryptedKey
? new EncString("2.abc|def|ghi")
: (undefined as unknown as EncString);
a.key = hasKey ? mock<SymmetricCryptoKey>() : undefined;
return a;
};

Expand Down
8 changes: 6 additions & 2 deletions libs/common/src/models/export/cipher.export.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { EncString } from "../../key-management/crypto/models/enc-string";
import { SymmetricCryptoKey } from "../../platform/models/domain/symmetric-crypto-key";
import { CipherRepromptType } from "../../vault/enums/cipher-reprompt-type";
import { CipherType } from "../../vault/enums/cipher-type";
import { Cipher as CipherDomain } from "../../vault/models/domain/cipher";
Expand Down Expand Up @@ -43,7 +44,7 @@ export class CipherExport {
view.notes = req.notes;
view.favorite = req.favorite;
view.reprompt = req.reprompt ?? CipherRepromptType.None;
view.key = req.key != null ? new EncString(req.key) : undefined;
view.key = req.key != null ? SymmetricCryptoKey.fromString(req.key) : undefined;

@shane-melton shane-melton Aug 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.


if (req.fields != null) {
view.fields = req.fields.map((f) => FieldExport.toView(f));
Expand Down Expand Up @@ -208,7 +209,10 @@ export class CipherExport {
this.name = safeGetString(o.name) ?? "";
this.notes = safeGetString(o.notes);
if ("key" in o) {
this.key = o.key?.encryptedString;
this.key =
o.key instanceof SymmetricCryptoKey
? o.key.toBase64()
: (o.key as EncString | undefined)?.encryptedString;
Comment on lines +217 to +220

@claude claude Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

}

this.favorite = o.favorite;
Expand Down
1 change: 0 additions & 1 deletion libs/common/src/vault/models/domain/attachment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ export class Attachment extends Domain {

if (this.key != null) {
view.key = await this.decryptAttachmentKey(decryptionKey);

@claude claude Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

view.encryptedKey = this.key; // Keep the encrypted key for the view

// When the attachment key couldn't be decrypted, mark a decryption error
// The file won't be able to be downloaded in these cases
Expand Down
9 changes: 2 additions & 7 deletions libs/common/src/vault/models/view/attachment.view.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { AttachmentView as SdkAttachmentView } from "@bitwarden/sdk-internal";

import { mockFromJson } from "../../../../spec";
import { EncString } from "../../../key-management/crypto/models/enc-string";
import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key";

import { AttachmentView } from "./attachment.view";
Expand Down Expand Up @@ -34,8 +33,7 @@ describe("AttachmentView", () => {
size: "size",
sizeName: "sizeName",
fileName: "fileName",
key: "encKeyB64_fromString",
decryptedKey: "decryptedKey_B64",
key: "decryptedKey_B64",
} as SdkAttachmentView;

const result = AttachmentView.fromSdkAttachmentView(sdkAttachmentView);
Expand All @@ -47,7 +45,6 @@ describe("AttachmentView", () => {
sizeName: "sizeName",
fileName: "fileName",
key: "mockKey",
encryptedKey: new EncString(sdkAttachmentView.key as string),
});

expect(SymmetricCryptoKey.fromString).toHaveBeenCalledWith("decryptedKey_B64");
Expand All @@ -66,7 +63,6 @@ describe("AttachmentView", () => {
attachmentView.size = "size";
attachmentView.sizeName = "sizeName";
attachmentView.fileName = "fileName";
attachmentView.encryptedKey = new EncString("encKeyB64");
attachmentView.key = mockKey;

const result = attachmentView.toSdkAttachmentView();
Expand All @@ -77,8 +73,7 @@ describe("AttachmentView", () => {
size: "size",
sizeName: "sizeName",
fileName: "fileName",
key: "encKeyB64",
decryptedKey: "keyB64",
key: "keyB64",
});
});
});
Expand Down
31 changes: 6 additions & 25 deletions libs/common/src/vault/models/view/attachment.view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { Jsonify } from "type-fest";

import { AttachmentView as SdkAttachmentView } from "@bitwarden/sdk-internal";
import { AttachmentView as SdkAttachmentView, SymmetricKey } from "@bitwarden/sdk-internal";

import { DECRYPT_ERROR, EncString } from "../../../key-management/crypto/models/enc-string";
import { DECRYPT_ERROR } from "../../../key-management/crypto/models/enc-string";
import { View } from "../../../models/view/view";
import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key";
import { Attachment } from "../domain/attachment";
Expand All @@ -14,10 +14,6 @@ export class AttachmentView implements View {
sizeName?: string;
fileName?: string;
key?: SymmetricCryptoKey;
/**
* The SDK returns an encrypted key for the attachment.
*/
encryptedKey: EncString | undefined;
private _hasDecryptionError?: boolean;

constructor(a?: Attachment) {
Expand Down Expand Up @@ -52,18 +48,7 @@ export class AttachmentView implements View {

static fromJSON(obj: Partial<Jsonify<AttachmentView>>): AttachmentView {
const key = obj.key == null ? null : SymmetricCryptoKey.fromJSON(obj.key);

let encryptedKey: EncString | undefined;
if (obj.encryptedKey != null) {
if (typeof obj.encryptedKey === "string") {
// If the key is a string, we need to parse it as EncString
encryptedKey = EncString.fromJSON(obj.encryptedKey);
} else if ((obj.encryptedKey as any) instanceof EncString) {
// If the key is already an EncString instance, we can use it directly
encryptedKey = obj.encryptedKey;
}
}
return Object.assign(new AttachmentView(), obj, { key: key, encryptedKey: encryptedKey });
return Object.assign(new AttachmentView(), obj, { key: key });
}

/**
Expand All @@ -76,9 +61,7 @@ export class AttachmentView implements View {
size: this.size,
sizeName: this.sizeName,
fileName: this.fileName,
key: this.encryptedKey?.toSdk(),
// TODO: PM-23005 - Temporary field, should be removed when encrypted migration is complete
decryptedKey: this.key ? this.key.toBase64() : undefined,
key: (this.key?.toBase64() ?? undefined) as SymmetricKey | undefined,
};
}

Expand All @@ -99,9 +82,7 @@ export class AttachmentView implements View {
view.size = obj.size;
view.sizeName = obj.sizeName;
view.fileName = obj.fileName;
// TODO: PM-23005 - Temporary field, should be removed when encrypted migration is complete
view.key = obj.decryptedKey ? SymmetricCryptoKey.fromString(obj.decryptedKey) : undefined;
view.encryptedKey = obj.key ? new EncString(obj.key) : undefined;
view.key = obj.key ? SymmetricCryptoKey.fromString(obj.key) : undefined;
view._hasDecryptionError = failure;

return view;
Expand All @@ -112,6 +93,6 @@ export class AttachmentView implements View {
* In this case, the attachment is encrypted with the user's user-key
*/
isLegacyAttachment(): boolean {
return this.key == null && this.encryptedKey == null;
return this.key == null;

@claude claude Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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;
}

}
}
34 changes: 11 additions & 23 deletions libs/common/src/vault/models/view/cipher.view.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import { Jsonify } from "type-fest";

import {
CiphersClient,
CipherView as SdkCipherView,
Expand All @@ -13,8 +11,8 @@ import {
} from "@bitwarden/sdk-internal";

import { mockFromJson, mockFromSdk } from "../../../../spec";
import { EncString } from "../../../key-management/crypto/models/enc-string";
import { asUuid } from "../../../platform/abstractions/sdk/sdk.service";
import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key";
import { CipherRepromptType } from "../../enums";
import { CipherType } from "../../enums/cipher-type";
import { CipherPermissionsApi } from "../api/cipher-permissions.api";
Expand Down Expand Up @@ -94,23 +92,13 @@ describe("CipherView", () => {
expect(actual).toMatchObject(expected);
});

it("handle both string and object inputs for the cipher key", () => {
const cipherKeyString = "cipherKeyString";
const cipherKeyObject = new EncString("cipherKeyObject");
it("deserializes the cipher key to a SymmetricCryptoKey", () => {
const mockKey = { keyB64: "c29tZS1iYXNlNjQta2V5" };
jest.spyOn(SymmetricCryptoKey, "fromJSON").mockReturnValue(mockKey as any);

// Test with string input
let actual = CipherView.fromJSON({
key: cipherKeyString,
});
expect(actual.key).toBeInstanceOf(EncString);
expect(actual.key?.toJSON()).toBe(cipherKeyString);

// Test with object input (which can happen when cipher view is stored in an InMemory state provider)
actual = CipherView.fromJSON({
key: cipherKeyObject,
} as Jsonify<CipherView>);
expect(actual.key).toBeInstanceOf(EncString);
expect(actual.key?.toJSON()).toBe(cipherKeyObject.toJSON());
const actual = CipherView.fromJSON({ key: mockKey as any });
expect(actual.key).toBe(mockKey);
expect(SymmetricCryptoKey.fromJSON).toHaveBeenCalledWith(mockKey);
});

it("fromJSON should always restore top-level CipherView properties", () => {
Expand Down Expand Up @@ -143,7 +131,7 @@ describe("CipherView", () => {
original.deletedDate = new Date("2022-01-03");
original.archivedDate = new Date("2022-01-04");
original.reprompt = CipherRepromptType.Password;
original.key = new EncString("test-key");
original.key = new SymmetricCryptoKey(new Uint8Array(64));
original.decryptionFailure = true;

// Serialize and deserialize
Expand Down Expand Up @@ -304,7 +292,7 @@ describe("CipherView", () => {
cipherView.organizationId = "000f2a6e-da5e-4726-87ed-1c5c77322c3c";
cipherView.folderId = "41b22db4-8e2a-4ed2-b568-f1186c72922f";
cipherView.collectionIds = ["b0473506-3c3c-4260-a734-dfaaf833ab6f"];
cipherView.key = new EncString("some-key");
cipherView.key = { toBase64: () => "some-key-b64" } as any;
cipherView.name = "name";
cipherView.notes = "notes";
cipherView.type = CipherType.Login;
Expand Down Expand Up @@ -334,7 +322,7 @@ describe("CipherView", () => {
organizationId: asUuid("000f2a6e-da5e-4726-87ed-1c5c77322c3c"),
folderId: asUuid("41b22db4-8e2a-4ed2-b568-f1186c72922f"),
collectionIds: [asUuid("b0473506-3c3c-4260-a734-dfaaf833ab6f")],
key: "some-key" as any,
key: "some-key-b64" as any,
name: "name",
notes: "notes",
type: SdkCipherType.Login,
Expand Down Expand Up @@ -516,7 +504,7 @@ describe("CipherView", () => {
cipherView.reprompt = CipherRepromptType.Password;
cipherView.revisionDate = new Date("2022-01-02T12:00:00.000Z");
cipherView.archivedDate = new Date("2022-01-03T12:00:00.000Z");
cipherView.key = new EncString("cipher-key");
cipherView.key = { toBase64: () => "cipher-key-b64" } as any;

const mockField = new RealFieldView();
mockField.name = "testField";
Expand Down
42 changes: 11 additions & 31 deletions libs/common/src/vault/models/view/cipher.view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ import {
CiphersClient,
CipherViewType,
CipherView as SdkCipherView,
SymmetricKey,
} from "@bitwarden/sdk-internal";

import { EncString } from "../../../key-management/crypto/models/enc-string";
import { View } from "../../../models/view/view";
import { asUuid, uuidAsString } from "../../../platform/abstractions/sdk/sdk.service";
import { InitializerMetadata } from "../../../platform/interfaces/initializer-metadata.interface";
import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key";
import { InitializerKey } from "../../../platform/services/cryptography/initializer-key";
import { DeepJsonify } from "../../../types/deep-jsonify";
import { CipherType, LinkedIdType } from "../../enums";
Expand Down Expand Up @@ -65,9 +66,7 @@ export class CipherView implements View, InitializerMetadata {
deletedDate?: Date;
archivedDate?: Date;
reprompt: CipherRepromptType = CipherRepromptType.None;
// We need a copy of the encrypted key so we can pass it to
// the SdkCipherView during encryption
key?: EncString;
key?: SymmetricCryptoKey;

/**
* Flag to indicate if the cipher decryption failed.
Expand Down Expand Up @@ -97,7 +96,6 @@ export class CipherView implements View, InitializerMetadata {
this.archivedDate = c.archivedDate;
// Old locally stored ciphers might have reprompt == null. If so set it to None.
this.reprompt = c.reprompt ?? CipherRepromptType.None;

@claude claude Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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;

this.key = c.key;
}

private get item(): ItemView | undefined {
Expand Down Expand Up @@ -252,17 +250,7 @@ export class CipherView implements View, InitializerMetadata {
view.passwordHistory =
obj.passwordHistory?.map((ph: any) => PasswordHistoryView.fromJSON(ph)) ?? [];

if (obj.key != null) {
let key: EncString | undefined;
if (typeof obj.key === "string") {
// If the key is a string, we need to parse it as EncString
key = EncString.fromJSON(obj.key);
} else if ((obj.key as any) instanceof EncString) {
// If the key is already an EncString instance, we can use it directly
key = obj.key;
}
view.key = key;
}
view.key = obj.key != null ? SymmetricCryptoKey.fromJSON(obj.key) : undefined;

switch (obj.type) {
case CipherType.Card:
Expand Down Expand Up @@ -299,7 +287,7 @@ export class CipherView implements View, InitializerMetadata {
/**
* Creates a CipherView from the SDK CipherView.
*/
static fromSdkCipherView(obj: SdkCipherView, sdk?: CiphersClient): CipherView | undefined {
static fromSdkCipherView(obj: SdkCipherView): CipherView | undefined {
if (obj == null) {
return undefined;
}
Expand Down Expand Up @@ -340,7 +328,7 @@ export class CipherView implements View, InitializerMetadata {
cipherView.deletedDate = obj.deletedDate == null ? undefined : new Date(obj.deletedDate);
cipherView.archivedDate = obj.archivedDate == null ? undefined : new Date(obj.archivedDate);
cipherView.reprompt = obj.reprompt ?? CipherRepromptType.None;
cipherView.key = obj.key ? EncString.fromJSON(obj.key) : undefined;
cipherView.key = obj.key ? SymmetricCryptoKey.fromString(obj.key) : undefined;

switch (obj.type) {
case CipherType.Card:
Expand All @@ -353,17 +341,9 @@ export class CipherView implements View, InitializerMetadata {
break;
case CipherType.Login:
cipherView.login = obj.login ? LoginView.fromSdkLoginView(obj.login) : new LoginView();
if (sdk && obj.login?.fido2Credentials?.length) {
const fido2CredentialViews = sdk.decrypt_fido2_credentials(obj);
const decryptedKeyValue = sdk.decrypt_fido2_private_key(obj);
cipherView.login.fido2Credentials = fido2CredentialViews
.map((cred) => {
const view = Fido2CredentialView.fromSdkFido2CredentialView(cred);
if (view) {
view.keyValue = decryptedKeyValue;
}
return view;
})
if (obj.login?.fido2Credentials?.length) {
cipherView.login.fido2Credentials = obj.login.fido2Credentials
.map((cred) => Fido2CredentialView.fromSdkFido2CredentialView(cred))
.filter((cred): cred is Fido2CredentialView => !!cred);
}
break;
Expand Down Expand Up @@ -447,7 +427,7 @@ export class CipherView implements View, InitializerMetadata {
revisionDate: this.revisionDate?.toISOString(),
archivedDate: this.archivedDate?.toISOString(),
attachments: this.attachments?.map((a) => a.toSdkAttachmentView()),
key: this.key?.toSdk(),
key: (this.key?.toBase64() ?? undefined) as SymmetricKey | undefined,

@claude claude Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 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.

};

// If the cipher has FIDO2 credentials, we need to set them on the SDK edit request
Expand Down Expand Up @@ -552,7 +532,7 @@ export class CipherView implements View, InitializerMetadata {
deletedDate: this.deletedDate?.toISOString(),
archivedDate: this.archivedDate?.toISOString(),
reprompt: this.reprompt ?? CipherRepromptType.None,
key: this.key?.toSdk(),
key: (this.key?.toBase64() ?? undefined) as any,
// Cipher type specific properties are set in the switch statement below
// CipherView initializes each with default constructors (undefined values)
// The SDK does not expect those undefined values and will throw exceptions
Expand Down
Loading
Loading