-
Notifications
You must be signed in to change notification settings - Fork 24
[PM-24978] Pass along Attachment decryption errors #644
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.
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
Changes from 2 commits
edcd0d3
3d5dfac
99be6b2
ce23472
40ac965
78b44cf
3acc313
732705a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -211,6 +211,9 @@ impl Decryptable<KeyIds, SymmetricKeyId, AttachmentView> for Attachment { | |
| ctx: &mut KeyStoreContext<KeyIds>, | ||
| key: SymmetricKeyId, | ||
| ) -> Result<AttachmentView, CryptoError> { | ||
| // Decrypt the file name or return an error if decryption fails | ||
| let file_name = self.file_name.decrypt(ctx, key)?; | ||
|
|
||
| #[cfg(feature = "wasm")] | ||
| let decrypted_key = if let Some(attachment_key) = &self.key { | ||
| let content_key_id = ctx.unwrap_symmetric_key(key, attachment_key)?; | ||
|
|
@@ -228,14 +231,47 @@ impl Decryptable<KeyIds, SymmetricKeyId, AttachmentView> for Attachment { | |
| url: self.url.clone(), | ||
| size: self.size.clone(), | ||
| size_name: self.size_name.clone(), | ||
| file_name: self.file_name.decrypt(ctx, key)?, | ||
| file_name, | ||
| key: self.key.clone(), | ||
| #[cfg(feature = "wasm")] | ||
| decrypted_key: decrypted_key.map(|k| k.to_string()), | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /// Decrypts a list of attachments, separating successful decryptions from failures. | ||
| /// | ||
| /// Returns a tuple of (successful_attachments, failed_attachments). | ||
| pub(crate) fn decrypt_attachments_with_failures( | ||
| attachments: &[Attachment], | ||
| ctx: &mut KeyStoreContext<KeyIds>, | ||
| key: SymmetricKeyId, | ||
| ) -> (Option<Vec<AttachmentView>>, Option<Vec<AttachmentView>>) { | ||
| let mut successes = Vec::new(); | ||
| let mut failures = Vec::new(); | ||
|
|
||
| for attachment in attachments { | ||
| match attachment.decrypt(ctx, key) { | ||
| Ok(decrypted) => successes.push(decrypted), | ||
| Err(_) => failures.push(AttachmentView { | ||
|
||
| id: attachment.id.clone(), | ||
| url: attachment.url.clone(), | ||
| size: attachment.size.clone(), | ||
| size_name: attachment.size_name.clone(), | ||
| file_name: None, | ||
| key: attachment.key.clone(), | ||
| #[cfg(feature = "wasm")] | ||
| decrypted_key: None, | ||
| }), | ||
| } | ||
| } | ||
|
|
||
| let successes = (!successes.is_empty()).then_some(successes); | ||
| let failures = (!failures.is_empty()).then_some(failures); | ||
|
|
||
| (successes, failures) | ||
| } | ||
|
|
||
| impl TryFrom<bitwarden_api_api::models::AttachmentResponseModel> for Attachment { | ||
| type Error = VaultParseError; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -347,6 +347,9 @@ pub struct CipherView { | |
| pub attachments: Option<Vec<attachment::AttachmentView>>, | ||
| pub fields: Option<Vec<field::FieldView>>, | ||
| pub password_history: Option<Vec<password_history::PasswordHistoryView>>, | ||
| /// Attachments that failed to decrypt. Only present when there are decryption failures. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub attachment_decryption_failures: Option<Vec<attachment::AttachmentView>>, | ||
nikwithak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| pub creation_date: DateTime<Utc>, | ||
| pub deleted_date: Option<DateTime<Utc>>, | ||
|
|
@@ -522,6 +525,19 @@ impl Decryptable<KeyIds, SymmetricKeyId, CipherView> for Cipher { | |
| ) -> Result<CipherView, CryptoError> { | ||
| let ciphers_key = Cipher::decrypt_cipher_key(ctx, key, &self.key)?; | ||
|
|
||
| // Separate successful and failed attachment decryptions | ||
| let (attachments, attachment_decryption_failures) = self | ||
|
||
| .attachments | ||
| .as_deref() | ||
| .map(|encrypted_attachments| { | ||
| attachment::decrypt_attachments_with_failures( | ||
| encrypted_attachments, | ||
| ctx, | ||
| ciphers_key, | ||
| ) | ||
| }) | ||
| .unwrap_or((None, None)); | ||
|
||
|
|
||
| let mut cipher = CipherView { | ||
| id: self.id, | ||
| organization_id: self.organization_id, | ||
|
|
@@ -543,7 +559,8 @@ impl Decryptable<KeyIds, SymmetricKeyId, CipherView> for Cipher { | |
| permissions: self.permissions, | ||
| view_password: self.view_password, | ||
| local_data: self.local_data.decrypt(ctx, ciphers_key).ok().flatten(), | ||
| attachments: self.attachments.decrypt(ctx, ciphers_key).ok().flatten(), | ||
| attachments, | ||
| attachment_decryption_failures, | ||
| fields: self.fields.decrypt(ctx, ciphers_key).ok().flatten(), | ||
| password_history: self | ||
| .password_history | ||
|
|
@@ -1264,6 +1281,7 @@ mod tests { | |
| view_password: true, | ||
| local_data: None, | ||
| attachments: None, | ||
| attachment_decryption_failures: None, | ||
| fields: None, | ||
| password_history: None, | ||
| creation_date: "2024-01-30T17:55:36.150Z".parse().unwrap(), | ||
|
|
@@ -2266,4 +2284,102 @@ mod tests { | |
|
|
||
| assert!(matches!(result, Err(VaultParseError::SerdeJson(_)))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_decrypt_cipher_with_mixed_attachments() { | ||
| let user_key: SymmetricCryptoKey = "w2LO+nwV4oxwswVYCxlOfRUseXfvU03VzvKQHrqeklPgiMZrspUe6sOBToCnDn9Ay0tuCBn8ykVVRb7PWhub2Q==".to_string().try_into().unwrap(); | ||
| let key_store = create_test_crypto_with_user_key(user_key); | ||
|
|
||
| // Create properly encrypted attachments | ||
| let mut ctx = key_store.context(); | ||
| let valid1 = "valid_file_1.txt" | ||
| .encrypt(&mut ctx, SymmetricKeyId::User) | ||
| .unwrap(); | ||
| let valid2 = "valid_file_2.txt" | ||
| .encrypt(&mut ctx, SymmetricKeyId::User) | ||
| .unwrap(); | ||
|
|
||
| // Create corrupted attachment by encrypting with a random different key | ||
| let wrong_key: SymmetricCryptoKey = "QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQQ==".to_string().try_into().unwrap(); | ||
| let wrong_key_store = create_test_crypto_with_user_key(wrong_key); | ||
| let mut wrong_ctx = wrong_key_store.context(); | ||
| let corrupted = "corrupted_file.txt" | ||
| .encrypt(&mut wrong_ctx, SymmetricKeyId::User) | ||
| .unwrap(); | ||
|
|
||
| let cipher = Cipher { | ||
| id: Some("090c19ea-a61a-4df6-8963-262b97bc6266".parse().unwrap()), | ||
| organization_id: None, | ||
| folder_id: None, | ||
| collection_ids: vec![], | ||
| key: None, | ||
| name: TEST_CIPHER_NAME.parse().unwrap(), | ||
| notes: None, | ||
| r#type: CipherType::Login, | ||
| login: None, | ||
| identity: None, | ||
| card: None, | ||
| secure_note: None, | ||
| ssh_key: None, | ||
| favorite: false, | ||
| reprompt: CipherRepromptType::None, | ||
| organization_use_totp: false, | ||
| edit: true, | ||
| permissions: None, | ||
| view_password: true, | ||
| local_data: None, | ||
| attachments: Some(vec![ | ||
| // Valid attachment | ||
| attachment::Attachment { | ||
| id: Some("valid-attachment".to_string()), | ||
| url: Some("https://example.com/valid".to_string()), | ||
| size: Some("100".to_string()), | ||
| size_name: Some("100 Bytes".to_string()), | ||
| file_name: Some(valid1), | ||
| key: None, | ||
| }, | ||
| // Corrupted attachment | ||
| attachment::Attachment { | ||
| id: Some("corrupted-attachment".to_string()), | ||
| url: Some("https://example.com/corrupted".to_string()), | ||
| size: Some("200".to_string()), | ||
| size_name: Some("200 Bytes".to_string()), | ||
| file_name: Some(corrupted), | ||
| key: None, | ||
| }, | ||
| // Another valid attachment | ||
| attachment::Attachment { | ||
| id: Some("valid-attachment-2".to_string()), | ||
| url: Some("https://example.com/valid2".to_string()), | ||
| size: Some("150".to_string()), | ||
| size_name: Some("150 Bytes".to_string()), | ||
| file_name: Some(valid2), | ||
| key: None, | ||
| }, | ||
| ]), | ||
| fields: None, | ||
| password_history: None, | ||
| creation_date: "2024-01-30T17:55:36.150Z".parse().unwrap(), | ||
| deleted_date: None, | ||
| revision_date: "2024-01-30T17:55:36.150Z".parse().unwrap(), | ||
| archived_date: None, | ||
| data: None, | ||
| }; | ||
|
|
||
| let view: CipherView = key_store.decrypt(&cipher).unwrap(); | ||
|
|
||
| // Should have 2 successful attachments | ||
| assert!(view.attachments.is_some()); | ||
| let successes = view.attachments.as_ref().unwrap(); | ||
| assert_eq!(successes.len(), 2); | ||
| assert_eq!(successes[0].id, Some("valid-attachment".to_string())); | ||
| assert_eq!(successes[1].id, Some("valid-attachment-2".to_string())); | ||
|
|
||
| // Should have 1 failed attachment | ||
| assert!(view.attachment_decryption_failures.is_some()); | ||
| let failures = view.attachment_decryption_failures.as_ref().unwrap(); | ||
| assert_eq!(failures.len(), 1); | ||
| assert_eq!(failures[0].id, Some("corrupted-attachment".to_string())); | ||
| assert_eq!(failures[0].file_name, None); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Vec<AttachmentView>, Vec<AttachmentView>)might be a better choice here (foregoing wrapping them inOption) - an empty Vec conveys the same information and simplifies a lot of the downstream logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like removing the
Optionas it cuts down on the needed code and mental thought. It doesn't match the definitions on theCipherViewso I had to rewrap inSomebut that feels like an okay trade offce23472