diff --git a/CHANGELOG.md b/CHANGELOG.md index f876e4d29fa..65724ccf555 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,12 @@ - `SyncManager::sync()` now fails immediately with a new `SyncManagerError::Busy` when a sync is already in progress, instead of blocking until it finishes. +## 🔧 What's Fixed 🔧 + +### Logins + +- Fixed a crash ("record's ID is invalid") when syncing a login whose guid is invalid for the sync server. Serializing such a record now returns an error instead of panicking, and the logins engine skips it so a single login can no longer block the whole sync. ([Bug 2056116](https://bugzilla.mozilla.org/show_bug.cgi?id=2056116)) + # v154.0 (_2026-07-20_) ## ✨ What's Changed ✨ diff --git a/components/logins/src/sync/engine.rs b/components/logins/src/sync/engine.rs index e4e71cf8402..06ca88e5e2b 100644 --- a/components/logins/src/sync/engine.rs +++ b/components/logins/src/sync/engine.rs @@ -258,11 +258,25 @@ impl LoginsSyncEngine { ))?; let bsos = stmt.query_and_then( named_params! { ":fxa_origin": FXA_CREDENTIALS_ORIGIN }, - |row| { + |row| -> Result> { self.scope.err_if_interrupted()?; - Ok(if row.get::<_, bool>("is_deleted")? { + let guid: Guid = row.get::<_, String>("guid")?.into(); + // A guid we consider invalid for the sync server used to panic the + // uploader (bug 2056116). We can't serialize such a record, so skip it + // rather than let a single login block the whole sync. + if !guid.is_valid_for_sync_server() { + // Report the length rather than the guid itself, which is arbitrary + // data we'd rather not send to Sentry. + report_error!( + "logins-invalid-outgoing-guid", + "skipping outgoing login with a guid that is invalid for the sync server (len {})", + guid.len() + ); + return Ok(None); + } + Ok(Some(if row.get::<_, bool>("is_deleted")? { let envelope = OutgoingEnvelope { - id: row.get::<_, String>("guid")?.into(), + id: guid, sortindex: Some(TOMBSTONE_SORTINDEX), ..Default::default() }; @@ -273,10 +287,10 @@ impl LoginsSyncEngine { EncryptedLogin::from_row(row)?.into_bso(db.encdec.as_ref(), unknown)?; bso.envelope.sortindex = Some(DEFAULT_SORTINDEX); bso - }) + })) }, )?; - bsos.collect::>() + bsos.filter_map(|r| r.transpose()).collect::>() } fn do_apply_incoming( @@ -823,6 +837,31 @@ mod tests { assert!(changes["changed"].get("deleted").is_none()); } + #[test] + fn test_fetch_outgoing_skips_invalid_guid() { + ensure_initialized(); + let store = LoginStore::new_in_memory(); + // A local login with a guid we consider invalid for the sync server (contains + // a comma), inserted directly to mimic a record that was stored before guids + // were validated (bug 2056116). + insert_login( + &store.lock_db().unwrap(), + "invalid,guid", + Some("password"), + None, + ); + // A normal local login that should still be uploaded. + insert_login(&store.lock_db().unwrap(), "valid", Some("password"), None); + + // Must not panic, and must upload only the valid record. + let changeset = run_fetch_outgoing(store); + let ids: Vec = changeset + .iter() + .map(|b| b.envelope.id.to_string()) + .collect(); + assert_eq!(ids, vec!["valid".to_string()]); + } + #[test] fn test_fetch_outgoing_excludes_fxa_credentials() { ensure_initialized(); diff --git a/components/sync15/src/bso/content.rs b/components/sync15/src/bso/content.rs index f40d773d1eb..3d44f9e0000 100644 --- a/components/sync15/src/bso/content.rs +++ b/components/sync15/src/bso/content.rs @@ -14,6 +14,7 @@ use crate::Guid; use crate::error::{trace, warn}; use error_support::report_error; use serde::Serialize; +use serde::ser::Error as _; // The only errors we return here are serde errors. type Result = std::result::Result; @@ -208,7 +209,13 @@ where match map.get("id").as_ref().and_then(|v| v.as_str()) { Some(id) => { let id: Guid = id.into(); - assert!(id.is_valid_for_sync_server(), "record's ID is invalid"); + if !id.is_valid_for_sync_server() { + // This is a sanity check on our own IDs, not something the + // server enforces, so a violation is an error for this one + // record rather than a reason to panic the process, which + // took down the whole parent process (bug 2056116). + return Err(serde_json::Error::custom("record's ID is invalid")); + } id } // In practice, this is a "static" error and not influenced by runtime behavior @@ -232,7 +239,10 @@ where if let Some(ref mut map) = payload.as_object_mut() { if let Some(content_id) = map.get("id").as_ref().and_then(|v| v.as_str()) { assert_eq!(content_id, id); - assert!(id.is_valid_for_sync_server(), "record's ID is invalid"); + if !id.is_valid_for_sync_server() { + // See content_with_id_to_json: don't panic on an invalid ID. + return Err(serde_json::Error::custom("record's ID is invalid")); + } } else { map.insert("id".to_string(), serde_json::Value::String(id.to_string())); } @@ -382,24 +392,23 @@ mod tests { } #[test] - #[should_panic] fn test_content_empty_id() { error_support::init_for_tests(); let val = TestStruct { id: Guid::new(""), data: 1, }; - let _ = OutgoingBso::from_content_with_id(val); + // An invalid ID is a recoverable error, not a panic (bug 2056116). + assert!(OutgoingBso::from_content_with_id(val).is_err()); } #[test] - #[should_panic] fn test_content_invalid_id() { error_support::init_for_tests(); let val = TestStruct { id: Guid::new(&"X".repeat(65)), data: 1, }; - let _ = OutgoingBso::from_content_with_id(val); + assert!(OutgoingBso::from_content_with_id(val).is_err()); } }