Bug 2056116 - Don't crash sync on an invalid login guid

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 ✨

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<Option<OutgoingBso>> {
                 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::<Result<_>>()
+        bsos.filter_map(|r| r.transpose()).collect::<Result<_>>()
     }
 
     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<String> = 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();

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<T> = std::result::Result<T, serde_json::Error>;
@@ -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());
     }
 }