[PM-16098] Improved cipher decryption error handling

[shane-melton]

🎟️ Tracking

PM-16098

📔 Objective

Improve error handling when we unexpectedly fail to decrypt a cipher to avoid crashing the entire vault.

• Adds a new decryptionFailure flag to the CipherView that will be set when decryption fails along with setting the cipher name to [error: cannot decrypt].
• Adjust the Vaults on all 3 web clients to show a new decryption error dialog that lists the cipher Ids that are failing and encourages users to contact CS for assistance.
• Ciphers with decryption errors will have their edit / clone options disabled. Only the delete action will be available on Web/Desktop. (Browser does not currently have a menu option for deleting)

📸 Screenshots

Web

Screen.Recording.2024-12-18.at.12.18.03.PM.mov

Browser

Screen.Recording.2024-12-18.at.12.22.16.PM.mov

Desktop

Screen.Recording.2024-12-18.at.12.02.29.PM.mov

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions]

Checkmarx One – Scan Summary & Details – 6f6c7a48-81df-496e-ae1c-b107bf0ef3d5

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Client_Privacy_Violation	/libs/vault/src/components/decryption-failure-dialog/decryption-failure-dialog.component.html: 17	Attack Vector

[codecov]

Codecov Report

Attention: Patch coverage is 16.26016% with 103 lines in your changes missing coverage. Please review.

Project coverage is 34.27%. Comparing base (9ca3d06) to head (8a2f615).
Report is 16 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...pps/desktop/src/vault/app/vault/vault.component.ts	0.00%	15 Missing ⚠️
.../src/app/vault/individual-vault/vault.component.ts	0.00%	14 Missing ⚠️
...lt/popup/components/vault-v2/vault-v2.component.ts	0.00%	13 Missing ⚠️
libs/common/src/vault/services/cipher.service.ts	35.00%	13 Missing ⚠️
...lure-dialog/decryption-failure-dialog.component.ts	35.00%	13 Missing ⚠️
...-container/vault-list-items-container.component.ts	0.00%	5 Missing ⚠️
...-container/trash-list-items-container.component.ts	0.00%	4 Missing ⚠️
apps/desktop/src/vault/app/vault/view.component.ts	0.00%	4 Missing ⚠️
...s/vault-item-dialog/vault-item-dialog.component.ts	0.00%	4 Missing ⚠️
...pps/web/src/app/vault/org-vault/vault.component.ts	0.00%	4 Missing ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12468      +/-   ##
==========================================
- Coverage   34.28%   34.27%   -0.01%     
==========================================
  Files        2901     2902       +1     
  Lines       89696    89798     +102     
  Branches    16846    16864      +18     
==========================================
+ Hits        30754    30780      +26     
- Misses      56523    56599      +76     
  Partials     2419     2419              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[danielleflinn]

Screen recordings LGTM!

[gbubemismith]

Looks good!

[quexten]

⚠️ Please verify that the failed ciphers are not included when rotating keys. Otherwise they might be re-encrypted and saved as properly decryptable ciphers with the error message as the - validly encrypted - contents.

I'm not sure whether this is something that this PR needs to ensure, or a follow-up task that key-management has to do (cc @jlf0dev), but re-encrypting incorrectly decrypted items needs to be prevented.

(We should block rotation entirely while there are failed ciphers).

[shane-melton]

Thanks for the callout @quexten!

I added a check in the CipherService to throw an error when attempting to retrieve cipher data for key rotation. See e2cffbd

[shane-melton]

Also tweaked the Trash items in the Browser extension to behave similarly to the Vault - They show the error dialog upon clicking a corrupted item and only show the "delete forever" option in the 3dot menu.

Screen.Recording.2024-12-19.at.3.50.26.PM.mov

cc: @danielleflinn

[quexten]

Thanks for the callout @quexten!

I added a check in the CipherService to throw an error when attempting to retrieve cipher data for key rotation. See e2cffbd

Nice, thank you!

Another potential issue (I did not investigate, just want to point it out):
Export grabs ciphers like this:

clients/libs/tools/export/vault-export/vault-export-core/src/services/individual-vault-export.service.ts

Line 72 in 0619ef5

this.cipherService.getAllDecrypted().then((ciphers) => {

If I understand this PR correctly, the above would still graph the failed ciphers, and export them, so we would have an export with failed ciphers, that then get re-imported as "good" ciphers.
So we need to filter it out there too (again only had a quick look here, in case this was already caught somewhere and I did not see it).

Would it be an option to make the interfaces for failed ciphers and good ciphers completely separated, and explicit, so that there is no confusion and no accidental oversights?

[shane-melton]

@quexten Another great point and I like the idea of creating separate interfaces for successfully decrypted and failed to decrypt ciphers. I went ahead and updated CipherService.cipherViews$ and CipherService.getAllDecrypted() to exclude any ciphers that fail to decrypt, so consumers will not mistakenly mix corrupted ciphers with valid ciphers. 01bd2b4

I also left the added logic to throw an exception if a user attempts a key rotation with corrupted ciphers in their vault. As for exporting, that will now only export ciphers that were successfully decrypted.

Thanks for the feedback and suggestion!

[quexten]

Nice work!

[gbubemismith]

This looks cleaner 👍🏼

[gbubemismith]

Changes look good