mirror of
https://github.com/bitwarden/browser
synced 2025-12-16 00:03:56 +00:00
[PM-16098] Improved cipher decryption error handling (#12468)
* [PM-16098] Add decryptionFailure flag to CipherView * [PM-16098] Add failedToDecryptCiphers$ observable to CipherService * [PM-16098] Introduce decryption-failure-dialog.component * [PM-16098] Disable cipher rows for the Web Vault * [PM-16098] Show decryption error dialog on vault load or when attempting to view/edit a corrupted cipher * [PM-16098] Browser - Show decryption error dialog on vault load or when attempting to view/edit a corrupted cipher * [PM-16098] Desktop - Show decryption error dialog on vault load or when attempting to view a corrupted cipher. Remove edit/clone context menu options and footer actions. * [PM-16098] Add CS link to decryption failure dialog * [PM-16098] Return cipherViews and move filtering of isDeleted to consumers * [PM-16098] Throw an error when retrieving cipher data for key rotation when a decryption failure is present * [PM-16098] Properly filter out deleted, corrupted ciphers when showing dialog within the Vault * [PM-16098] Show the decryption error dialog when attempting to view a cipher in trash and disable the restore option * [PM-16098] Exclude failed to decrypt ciphers from getAllDecrypted method and cipherViews$ observable * [PM-16098] Avoid re-sorting remainingCiphers$ as it was redundant * [PM-16098] Update tests * [PM-16098] Prevent opening view dialog in AC for corrupted ciphers * [PM-16098] Remove withLatestFrom operator that was causing race conditions when navigating away from the individual vault * [PM-16098] Ensure decryption error dialog is only shown once on Desktop when switching accounts
This commit is contained in:
@@ -26,6 +26,12 @@ export abstract class CipherService implements UserKeyRotationDataProvider<Ciphe
|
||||
* An observable monitoring the add/edit cipher info saved to memory.
|
||||
*/
|
||||
addEditCipherInfo$: Observable<AddEditCipherInfo>;
|
||||
/**
|
||||
* Observable that emits an array of cipherViews that failed to decrypt. Does not emit until decryption has completed.
|
||||
*
|
||||
* An empty array indicates that all ciphers were successfully decrypted.
|
||||
*/
|
||||
failedToDecryptCiphers$: Observable<CipherView[]>;
|
||||
clearCache: (userId?: string) => Promise<void>;
|
||||
encrypt: (
|
||||
model: CipherView,
|
||||
|
||||
@@ -136,7 +136,13 @@ export class Cipher extends Domain implements Decryptable<CipherView> {
|
||||
|
||||
if (this.key != null) {
|
||||
const encryptService = Utils.getContainerService().getEncryptService();
|
||||
encKey = new SymmetricCryptoKey(await encryptService.decryptToBytes(this.key, encKey));
|
||||
const keyBytes = await encryptService.decryptToBytes(this.key, encKey);
|
||||
if (keyBytes == null) {
|
||||
model.name = "[error: cannot decrypt]";
|
||||
model.decryptionFailure = true;
|
||||
return model;
|
||||
}
|
||||
encKey = new SymmetricCryptoKey(keyBytes);
|
||||
bypassValidation = false;
|
||||
}
|
||||
|
||||
|
||||
@@ -46,6 +46,11 @@ export class CipherView implements View, InitializerMetadata {
|
||||
deletedDate: Date = null;
|
||||
reprompt: CipherRepromptType = CipherRepromptType.None;
|
||||
|
||||
/**
|
||||
* Flag to indicate if the cipher decryption failed.
|
||||
*/
|
||||
decryptionFailure = false;
|
||||
|
||||
constructor(c?: Cipher) {
|
||||
if (!c) {
|
||||
return;
|
||||
|
||||
@@ -359,6 +359,7 @@ describe("Cipher Service", () => {
|
||||
const originalUserKey = new SymmetricCryptoKey(new Uint8Array(32)) as UserKey;
|
||||
const newUserKey = new SymmetricCryptoKey(new Uint8Array(32)) as UserKey;
|
||||
let decryptedCiphers: BehaviorSubject<Record<CipherId, CipherView>>;
|
||||
let failedCiphers: BehaviorSubject<CipherView[]>;
|
||||
let encryptedKey: EncString;
|
||||
|
||||
beforeEach(() => {
|
||||
@@ -385,6 +386,7 @@ describe("Cipher Service", () => {
|
||||
Cipher2: cipher2,
|
||||
});
|
||||
cipherService.cipherViews$ = decryptedCiphers.pipe(map((ciphers) => Object.values(ciphers)));
|
||||
cipherService.failedToDecryptCiphers$ = failedCiphers = new BehaviorSubject<CipherView[]>([]);
|
||||
|
||||
encryptService.decryptToBytes.mockResolvedValue(new Uint8Array(32));
|
||||
encryptedKey = new EncString("Re-encrypted Cipher Key");
|
||||
@@ -413,5 +415,16 @@ describe("Cipher Service", () => {
|
||||
"New user key is required to rotate ciphers",
|
||||
);
|
||||
});
|
||||
|
||||
it("throws if the user has any failed to decrypt ciphers", async () => {
|
||||
const badCipher = new CipherView(cipherObj);
|
||||
badCipher.id = "Cipher 3";
|
||||
badCipher.organizationId = null;
|
||||
badCipher.decryptionFailure = true;
|
||||
failedCiphers.next([badCipher]);
|
||||
await expect(
|
||||
cipherService.getRotatedData(originalUserKey, newUserKey, mockUserId),
|
||||
).rejects.toThrow("Cannot rotate ciphers when decryption failures are present");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -7,6 +7,7 @@ import {
|
||||
map,
|
||||
merge,
|
||||
Observable,
|
||||
of,
|
||||
shareReplay,
|
||||
Subject,
|
||||
switchMap,
|
||||
@@ -79,6 +80,7 @@ import {
|
||||
ADD_EDIT_CIPHER_INFO_KEY,
|
||||
DECRYPTED_CIPHERS,
|
||||
ENCRYPTED_CIPHERS,
|
||||
FAILED_DECRYPTED_CIPHERS,
|
||||
LOCAL_DATA_KEY,
|
||||
} from "./key-state/ciphers.state";
|
||||
|
||||
@@ -109,9 +111,17 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
cipherViews$: Observable<CipherView[] | null>;
|
||||
addEditCipherInfo$: Observable<AddEditCipherInfo>;
|
||||
|
||||
/**
|
||||
* Observable that emits an array of cipherViews that failed to decrypt. Does not emit until decryption has completed.
|
||||
*
|
||||
* An empty array indicates that all ciphers were successfully decrypted.
|
||||
*/
|
||||
failedToDecryptCiphers$: Observable<CipherView[]>;
|
||||
|
||||
private localDataState: ActiveUserState<Record<CipherId, LocalData>>;
|
||||
private encryptedCiphersState: ActiveUserState<Record<CipherId, CipherData>>;
|
||||
private decryptedCiphersState: ActiveUserState<Record<CipherId, CipherView>>;
|
||||
private failedToDecryptCiphersState: ActiveUserState<CipherView[]>;
|
||||
private addEditCipherInfoState: ActiveUserState<AddEditCipherInfo>;
|
||||
|
||||
constructor(
|
||||
@@ -132,6 +142,7 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
this.localDataState = this.stateProvider.getActive(LOCAL_DATA_KEY);
|
||||
this.encryptedCiphersState = this.stateProvider.getActive(ENCRYPTED_CIPHERS);
|
||||
this.decryptedCiphersState = this.stateProvider.getActive(DECRYPTED_CIPHERS);
|
||||
this.failedToDecryptCiphersState = this.stateProvider.getActive(FAILED_DECRYPTED_CIPHERS);
|
||||
this.addEditCipherInfoState = this.stateProvider.getActive(ADD_EDIT_CIPHER_INFO_KEY);
|
||||
|
||||
this.localData$ = this.localDataState.state$.pipe(map((data) => data ?? {}));
|
||||
@@ -143,6 +154,13 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
switchMap(() => merge(this.forceCipherViews$, this.getAllDecrypted())),
|
||||
shareReplay({ bufferSize: 1, refCount: true }),
|
||||
);
|
||||
|
||||
this.failedToDecryptCiphers$ = this.failedToDecryptCiphersState.state$.pipe(
|
||||
filter((ciphers) => ciphers != null),
|
||||
switchMap((ciphers) => merge(this.forceCipherViews$, of(ciphers))),
|
||||
shareReplay({ bufferSize: 1, refCount: true }),
|
||||
);
|
||||
|
||||
this.addEditCipherInfo$ = this.addEditCipherInfoState.state$;
|
||||
}
|
||||
|
||||
@@ -162,6 +180,10 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
}
|
||||
}
|
||||
|
||||
async setFailedDecryptedCiphers(cipherViews: CipherView[], userId: UserId) {
|
||||
await this.stateProvider.setUserState(FAILED_DECRYPTED_CIPHERS, cipherViews, userId);
|
||||
}
|
||||
|
||||
private async setDecryptedCiphers(value: CipherView[], userId: UserId) {
|
||||
const cipherViews: { [id: string]: CipherView } = {};
|
||||
value?.forEach((c) => {
|
||||
@@ -378,7 +400,7 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
*/
|
||||
@sequentialize(() => "getAllDecrypted")
|
||||
async getAllDecrypted(): Promise<CipherView[]> {
|
||||
let decCiphers = await this.getDecryptedCiphers();
|
||||
const decCiphers = await this.getDecryptedCiphers();
|
||||
if (decCiphers != null && decCiphers.length !== 0) {
|
||||
await this.reindexCiphers();
|
||||
return await this.getDecryptedCiphers();
|
||||
@@ -390,10 +412,15 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
return [];
|
||||
}
|
||||
|
||||
decCiphers = await this.decryptCiphers(await this.getAll(), activeUserId);
|
||||
const [newDecCiphers, failedCiphers] = await this.decryptCiphers(
|
||||
await this.getAll(),
|
||||
activeUserId,
|
||||
);
|
||||
|
||||
await this.setDecryptedCipherCache(decCiphers, activeUserId);
|
||||
return decCiphers;
|
||||
await this.setDecryptedCipherCache(newDecCiphers, activeUserId);
|
||||
await this.setFailedDecryptedCiphers(failedCiphers, activeUserId);
|
||||
|
||||
return newDecCiphers;
|
||||
}
|
||||
|
||||
private async getDecryptedCiphers() {
|
||||
@@ -402,7 +429,17 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
);
|
||||
}
|
||||
|
||||
private async decryptCiphers(ciphers: Cipher[], userId: UserId) {
|
||||
/**
|
||||
* Decrypts the provided ciphers using the provided user's keys.
|
||||
* @param ciphers
|
||||
* @param userId
|
||||
* @returns Two cipher arrays, the first containing successfully decrypted ciphers and the second containing ciphers that failed to decrypt.
|
||||
* @private
|
||||
*/
|
||||
private async decryptCiphers(
|
||||
ciphers: Cipher[],
|
||||
userId: UserId,
|
||||
): Promise<[CipherView[], CipherView[]]> {
|
||||
const keys = await firstValueFrom(this.keyService.cipherDecryptionKeys$(userId, true));
|
||||
|
||||
if (keys == null || (keys.userKey == null && Object.keys(keys.orgKeys).length === 0)) {
|
||||
@@ -420,7 +457,7 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
{} as Record<string, Cipher[]>,
|
||||
);
|
||||
|
||||
const decCiphers = (
|
||||
const allCipherViews = (
|
||||
await Promise.all(
|
||||
Object.entries(grouped).map(async ([orgId, groupedCiphers]) => {
|
||||
if (await this.configService.getFeatureFlag(FeatureFlag.PM4154_BulkEncryptionService)) {
|
||||
@@ -440,7 +477,18 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
.flat()
|
||||
.sort(this.getLocaleSortingFunction());
|
||||
|
||||
return decCiphers;
|
||||
// Split ciphers into two arrays, one for successfully decrypted ciphers and one for ciphers that failed to decrypt
|
||||
return allCipherViews.reduce(
|
||||
(acc, c) => {
|
||||
if (c.decryptionFailure) {
|
||||
acc[1].push(c);
|
||||
} else {
|
||||
acc[0].push(c);
|
||||
}
|
||||
return acc;
|
||||
},
|
||||
[[], []] as [CipherView[], CipherView[]],
|
||||
);
|
||||
}
|
||||
|
||||
private async reindexCiphers() {
|
||||
@@ -1272,10 +1320,15 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
let encryptedCiphers: CipherWithIdRequest[] = [];
|
||||
|
||||
const ciphers = await firstValueFrom(this.cipherViews$);
|
||||
const failedCiphers = await firstValueFrom(this.failedToDecryptCiphers$);
|
||||
if (!ciphers) {
|
||||
return encryptedCiphers;
|
||||
}
|
||||
|
||||
if (failedCiphers.length > 0) {
|
||||
throw new Error("Cannot rotate ciphers when decryption failures are present");
|
||||
}
|
||||
|
||||
const userCiphers = ciphers.filter((c) => c.organizationId == null);
|
||||
if (userCiphers.length === 0) {
|
||||
return encryptedCiphers;
|
||||
@@ -1636,6 +1689,7 @@ export class CipherService implements CipherServiceAbstraction {
|
||||
|
||||
private async clearDecryptedCiphersState(userId: UserId) {
|
||||
await this.setDecryptedCiphers(null, userId);
|
||||
await this.setFailedDecryptedCiphers(null, userId);
|
||||
this.clearSortedCiphers();
|
||||
}
|
||||
|
||||
|
||||
@@ -28,6 +28,15 @@ export const DECRYPTED_CIPHERS = UserKeyDefinition.record<CipherView>(
|
||||
},
|
||||
);
|
||||
|
||||
export const FAILED_DECRYPTED_CIPHERS = UserKeyDefinition.array<CipherView>(
|
||||
CIPHERS_MEMORY,
|
||||
"failedDecryptedCiphers",
|
||||
{
|
||||
deserializer: (cipher: Jsonify<CipherView>) => CipherView.fromJSON(cipher),
|
||||
clearOn: ["logout", "lock"],
|
||||
},
|
||||
);
|
||||
|
||||
export const LOCAL_DATA_KEY = new UserKeyDefinition<Record<CipherId, LocalData>>(
|
||||
CIPHERS_DISK_LOCAL,
|
||||
"localData",
|
||||
|
||||
Reference in New Issue
Block a user