From e0e2cf56f53f890c854d713f448caebd254d5f92 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Mon, 29 Dec 2025 18:31:15 +0100 Subject: [PATCH] [PM-30285] Add soundness check to cipher and folder recovery step (#18120) * Add soundness check to cipher and folder recovery step * fix tests --------- Co-authored-by: Maciej Zieniuk (cherry picked from commit f689fd88b76789ddf8a98951f0062a54f611ac88) --- .../data-recovery/steps/cipher-step.spec.ts | 36 ++++++++++++++++--- .../data-recovery/steps/cipher-step.ts | 6 +++- .../data-recovery/steps/folder-step.ts | 6 +++- 3 files changed, 42 insertions(+), 6 deletions(-) diff --git a/apps/web/src/app/key-management/data-recovery/steps/cipher-step.spec.ts b/apps/web/src/app/key-management/data-recovery/steps/cipher-step.spec.ts index a894fce0c41..9ae0600fb2a 100644 --- a/apps/web/src/app/key-management/data-recovery/steps/cipher-step.spec.ts +++ b/apps/web/src/app/key-management/data-recovery/steps/cipher-step.spec.ts @@ -132,7 +132,10 @@ describe("CipherStep", () => { userKey: null, encryptedPrivateKey: null, isPrivateKeyCorrupt: false, - ciphers: [{ id: "cipher-1", organizationId: null } as Cipher], + ciphers: [ + { id: "cipher-1", organizationId: null } as Cipher, + { id: "cipher-2", organizationId: null } as Cipher, + ], folders: [], }; @@ -144,14 +147,39 @@ describe("CipherStep", () => { expect(result).toBe(false); }); - it("returns true when there are undecryptable ciphers", async () => { + it("returns true when there are undecryptable ciphers but at least one decryptable cipher", async () => { const userId = "user-id" as UserId; const workingData: RecoveryWorkingData = { userId, userKey: null, encryptedPrivateKey: null, isPrivateKeyCorrupt: false, - ciphers: [{ id: "cipher-1", organizationId: null } as Cipher], + ciphers: [ + { id: "cipher-1", organizationId: null } as Cipher, + { id: "cipher-2", organizationId: null } as Cipher, + ], + folders: [], + }; + + cipherEncryptionService.decrypt.mockRejectedValueOnce(new Error("Decryption failed")); + + await cipherStep.runDiagnostics(workingData, logger); + const result = cipherStep.canRecover(workingData); + + expect(result).toBe(true); + }); + + it("returns false when all ciphers are undecryptable", async () => { + const userId = "user-id" as UserId; + const workingData: RecoveryWorkingData = { + userId, + userKey: null, + encryptedPrivateKey: null, + isPrivateKeyCorrupt: false, + ciphers: [ + { id: "cipher-1", organizationId: null } as Cipher, + { id: "cipher-2", organizationId: null } as Cipher, + ], folders: [], }; @@ -160,7 +188,7 @@ describe("CipherStep", () => { await cipherStep.runDiagnostics(workingData, logger); const result = cipherStep.canRecover(workingData); - expect(result).toBe(true); + expect(result).toBe(false); }); }); diff --git a/apps/web/src/app/key-management/data-recovery/steps/cipher-step.ts b/apps/web/src/app/key-management/data-recovery/steps/cipher-step.ts index b44e8afc54d..01c2d9bc2a1 100644 --- a/apps/web/src/app/key-management/data-recovery/steps/cipher-step.ts +++ b/apps/web/src/app/key-management/data-recovery/steps/cipher-step.ts @@ -10,6 +10,7 @@ export class CipherStep implements RecoveryStep { title = "recoveryStepCipherTitle"; private undecryptableCipherIds: string[] = []; + private decryptableCipherIds: string[] = []; constructor( private apiService: ApiService, @@ -31,18 +32,21 @@ export class CipherStep implements RecoveryStep { for (const cipher of userCiphers) { try { await this.cipherService.decrypt(cipher, workingData.userId); + this.decryptableCipherIds.push(cipher.id); } catch { logger.record(`Cipher ID ${cipher.id} was undecryptable`); this.undecryptableCipherIds.push(cipher.id); } } logger.record(`Found ${this.undecryptableCipherIds.length} undecryptable ciphers`); + logger.record(`Found ${this.decryptableCipherIds.length} decryptable ciphers`); return this.undecryptableCipherIds.length == 0; } canRecover(workingData: RecoveryWorkingData): boolean { - return this.undecryptableCipherIds.length > 0; + // If everything fails to decrypt, it's a deeper issue and we shouldn't offer recovery here. + return this.undecryptableCipherIds.length > 0 && this.decryptableCipherIds.length > 0; } async runRecovery(workingData: RecoveryWorkingData, logger: LogRecorder): Promise { diff --git a/apps/web/src/app/key-management/data-recovery/steps/folder-step.ts b/apps/web/src/app/key-management/data-recovery/steps/folder-step.ts index bc0ae31efba..90e252ce6c3 100644 --- a/apps/web/src/app/key-management/data-recovery/steps/folder-step.ts +++ b/apps/web/src/app/key-management/data-recovery/steps/folder-step.ts @@ -11,6 +11,7 @@ export class FolderStep implements RecoveryStep { title = "recoveryStepFoldersTitle"; private undecryptableFolderIds: string[] = []; + private decryptableFolderIds: string[] = []; constructor( private folderService: FolderApiServiceAbstraction, @@ -36,18 +37,21 @@ export class FolderStep implements RecoveryStep { folder.name.encryptedString, workingData.userKey.toEncoded(), ); + this.decryptableFolderIds.push(folder.id); } catch { logger.record(`Folder name for folder ID ${folder.id} was undecryptable`); this.undecryptableFolderIds.push(folder.id); } } logger.record(`Found ${this.undecryptableFolderIds.length} undecryptable folders`); + logger.record(`Found ${this.decryptableFolderIds.length} decryptable folders`); return this.undecryptableFolderIds.length == 0; } canRecover(workingData: RecoveryWorkingData): boolean { - return this.undecryptableFolderIds.length > 0; + // If everything fails to decrypt, it's a deeper issue and we shouldn't offer recovery here. + return this.undecryptableFolderIds.length > 0 && this.decryptableFolderIds.length > 0; } async runRecovery(workingData: RecoveryWorkingData, logger: LogRecorder): Promise {