[PM-24978] Pass along Attachment decryption errors #644
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #644 +/- ##
=======================================
Coverage 78.88% 78.89%
=======================================
Files 291 291
Lines 31689 31701 +12
=======================================
+ Hits 24998 25009 +11
- Misses 6691 6692 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| let (file_name, decryption_failure) = match self.file_name.decrypt(ctx, key) { | ||
| Ok(name) => (name, false), | ||
| Err(..) => (None, true), | ||
| }; |
There was a problem hiding this comment.
Currently the SDK swallows any decryption errors for Attachments and thus hides all attachments from the user.
Where is the actual error being swallowed? I would suggest adding the handling logic there, maybe following the pattern of decrypt_list_with_failures. with the change here, you end up throwing away the CryptoError and the function returns as a "successful" decryption, breaking the contract of "This function returns a decrypted object, or an Err(CryptoError)"
🎟️ Tracking
PM-24978
Clients PR: bitwarden/clients#17790
📔 Objective
Currently the SDK swallows any decryption errors for Attachments and thus hides all attachments from the user. Even when the user has multiple attachments on a cipher.
The clients repository currently uses a magic string as the value to signify for decryption failures this.decryptedValue = DECRYPT_ERROR;. This approach does not allow for easy changes in the UI from design and is not localized.
Here I added a flag to the Attachment that will be set if the attachment fails to decrypt which is then consumed by the clients to present the respective UI.
🚨 Breaking Changes
❓ This could be a breaking change for the mobile clients if they expect attachment decryptions to be ignored. I do not know if that is the case yet. I'll reach out.
Client UI
🦮 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 confirmedissue 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