From dca14285fd1506c28f32108ea82da6e1525a8e99 Mon Sep 17 00:00:00 2001 From: Mathijs van Veluw Date: Mon, 9 Sep 2024 11:36:37 +0200 Subject: [PATCH] Fix sync with new native clients (#4932) --- src/api/core/accounts.rs | 2 +- src/api/core/ciphers.rs | 4 +- src/api/core/organizations.rs | 2 +- src/db/models/cipher.rs | 73 ++++++++++++++++++++++++++--------- 4 files changed, 58 insertions(+), 23 deletions(-) diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 187b9096..cc9cd6ce 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -490,7 +490,7 @@ async fn post_rotatekey(data: Json, headers: Headers, mut conn: DbConn, // Bitwarden does not process the import if there is one item invalid. // Since we check for the size of the encrypted note length, we need to do that here to pre-validate it. // TODO: See if we can optimize the whole cipher adding/importing and prevent duplicate code and checks. - Cipher::validate_notes(&data.ciphers)?; + Cipher::validate_cipher_data(&data.ciphers)?; let user_uuid = &headers.user.uuid; diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index 6fcde07c..13bf701f 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -233,7 +233,7 @@ pub struct CipherData { favorite: Option, reprompt: Option, - password_history: Option, + pub password_history: Option, // These are used during key rotation // 'Attachments' is unused, contains map of {id: filename} @@ -563,7 +563,7 @@ async fn post_ciphers_import( // Bitwarden does not process the import if there is one item invalid. // Since we check for the size of the encrypted note length, we need to do that here to pre-validate it. // TODO: See if we can optimize the whole cipher adding/importing and prevent duplicate code and checks. - Cipher::validate_notes(&data.ciphers)?; + Cipher::validate_cipher_data(&data.ciphers)?; // Read and create the folders let existing_folders: Vec = diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index e0c3f081..6d9f055a 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -1596,7 +1596,7 @@ async fn post_org_import( // Bitwarden does not process the import if there is one item invalid. // Since we check for the size of the encrypted note length, we need to do that here to pre-validate it. // TODO: See if we can optimize the whole cipher adding/importing and prevent duplicate code and checks. - Cipher::validate_notes(&data.ciphers)?; + Cipher::validate_cipher_data(&data.ciphers)?; let mut collections = Vec::new(); for coll in data.collections { diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index d577971d..1c6e499d 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -79,19 +79,39 @@ impl Cipher { } } - pub fn validate_notes(cipher_data: &[CipherData]) -> EmptyResult { + pub fn validate_cipher_data(cipher_data: &[CipherData]) -> EmptyResult { let mut validation_errors = serde_json::Map::new(); let max_note_size = CONFIG._max_note_size(); let max_note_size_msg = format!("The field Notes exceeds the maximum encrypted value length of {} characters.", &max_note_size); for (index, cipher) in cipher_data.iter().enumerate() { + // Validate the note size and if it is exceeded return a warning if let Some(note) = &cipher.notes { if note.len() > max_note_size { validation_errors .insert(format!("Ciphers[{index}].Notes"), serde_json::to_value([&max_note_size_msg]).unwrap()); } } + + // Validate the password history if it contains `null` values and if so, return a warning + if let Some(Value::Array(password_history)) = &cipher.password_history { + for pwh in password_history { + if let Value::Object(pwo) = pwh { + if pwo.get("password").is_some_and(|p| !p.is_string()) { + validation_errors.insert( + format!("Ciphers[{index}].Notes"), + serde_json::to_value([ + "The password history contains a `null` value. Only strings are allowed.", + ]) + .unwrap(), + ); + break; + } + } + } + } } + if !validation_errors.is_empty() { let err_json = json!({ "message": "The model state is invalid.", @@ -153,27 +173,39 @@ impl Cipher { .as_ref() .and_then(|s| { serde_json::from_str::>>(s) - .inspect_err(|e| warn!("Error parsing fields {:?}", e)) - .ok() - }) - .map(|d| d.into_iter().map(|d| d.data).collect()) - .unwrap_or_default(); - let password_history_json: Vec<_> = self - .password_history - .as_ref() - .and_then(|s| { - serde_json::from_str::>>(s) - .inspect_err(|e| warn!("Error parsing password history {:?}", e)) + .inspect_err(|e| warn!("Error parsing fields {e:?} for {}", self.uuid)) .ok() }) .map(|d| d.into_iter().map(|d| d.data).collect()) .unwrap_or_default(); + let password_history_json: Vec<_> = self + .password_history + .as_ref() + .and_then(|s| { + serde_json::from_str::>>(s) + .inspect_err(|e| warn!("Error parsing password history {e:?} for {}", self.uuid)) + .ok() + }) + .map(|d| { + // Check every password history item if they are valid and return it. + // If a password field has the type `null` skip it, it breaks newer Bitwarden clients + d.into_iter() + .filter_map(|d| match d.data.get("password") { + Some(p) if p.is_string() => Some(d.data), + _ => None, + }) + .collect() + }) + .unwrap_or_default(); + // Get the type_data or a default to an empty json object '{}'. // If not passing an empty object, mobile clients will crash. - let mut type_data_json = serde_json::from_str::>(&self.data) - .map(|d| d.data) - .unwrap_or_else(|_| Value::Object(serde_json::Map::new())); + let mut type_data_json = + serde_json::from_str::>(&self.data).map(|d| d.data).unwrap_or_else(|_| { + warn!("Error parsing data field for {}", self.uuid); + Value::Object(serde_json::Map::new()) + }); // NOTE: This was marked as *Backwards Compatibility Code*, but as of January 2021 this is still being used by upstream // Set the first element of the Uris array as Uri, this is needed several (mobile) clients. @@ -189,10 +221,13 @@ impl Cipher { // Fix secure note issues when data is invalid // This breaks at least the native mobile clients - if self.atype == 2 - && (self.data.is_empty() || self.data.eq("{}") || self.data.to_ascii_lowercase().eq("{\"type\":null}")) - { - type_data_json = json!({"type": 0}); + if self.atype == 2 { + match type_data_json { + Value::Object(ref t) if t.get("type").is_some_and(|t| t.is_number()) => {} + _ => { + type_data_json = json!({"type": 0}); + } + } } // Clone the type_data and add some default value.