Bug 2056116 - Don't crash sync on logins with invalid guids

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 [Full Changelog](In progress)
 
+## 🔧 What's Fixed 🔧
+
+### Logins
+
+- Fixed a parent-process crash ("record's ID is invalid") when syncing a login whose guid is invalid for the sync server. The sync engine now skips such records instead of panicking, `add_with_meta()` / `add_many_with_meta()` reject invalid guids on import (or regenerate them with the new `fixup_invalid_guids` feature), and a schema migration to version 6 repairs any already-stored invalid guids. ([Bug 2056116](https://bugzilla.mozilla.org/show_bug.cgi?id=2056116))
+
 # v154.0 (_2026-07-20_)
 
 ## ✨ What's Changed ✨

components/logins/Cargo.toml

@@ -26,6 +26,12 @@ ignore_form_action_origin_validation_errors = []
 # behavior. Used on Desktop during migration to salvage legacy/addon-generated
 # entries that would otherwise be lost.
 perform_additional_origin_fixups = []
+# Regenerates guids that are invalid for the sync server (empty, > 64 chars, or
+# containing a comma / non-printable byte) when importing via `add_with_meta` /
+# `add_many_with_meta`, instead of rejecting them. Used on Desktop during
+# migration to salvage entries whose guids would otherwise crash the sync
+# uploader (bug 2056116).
+fixup_invalid_guids = []
 
 [dependencies]
 sync15 = { path = "../sync15" }

components/logins/src/db.rs

@@ -614,8 +614,19 @@ impl LoginDb {
     ) -> Result<Vec<Result<EncryptedLogin>>> {
         let tx = self.unchecked_transaction()?;
         let mut results = vec![];
-        for entry_with_meta in entries_with_meta {
-            let guid = Guid::from_string(entry_with_meta.meta.id.clone());
+        for mut entry_with_meta in entries_with_meta {
+            let guid = match Self::validate_or_fixup_guid(Guid::from_string(
+                entry_with_meta.meta.id.clone(),
+            )) {
+                Ok(guid) => guid,
+                Err(err) => {
+                    results.push(Err(err));
+                    continue;
+                }
+            };
+            // Keep `meta.id` in sync with the (possibly regenerated) guid; it is used
+            // as the stored/envelope id and when encrypting `sec_fields` below.
+            entry_with_meta.meta.id = guid.to_string();
             match self.fixup_and_check_for_dupes(&guid, entry_with_meta.entry) {
                 Ok(new_entry) => {
                     let sec_fields = SecureLoginFields {
@@ -649,6 +660,33 @@ impl LoginDb {
         Ok(results)
     }
 
+    /// Validates a caller-supplied guid from the "with meta" import path against the
+    /// sync server's rules (see `Guid::is_valid_for_sync_server`). A guid that is
+    /// invalid for the sync server can never have existed on the server, so
+    /// regenerating it loses no sync identity.
+    ///
+    /// With the `fixup_invalid_guids` feature (enabled on Desktop during migration),
+    /// an invalid guid is silently replaced with a fresh random one. Without it, an
+    /// invalid guid is rejected so the problem surfaces at write time instead of being
+    /// persisted and later crashing the sync uploader (bug 2056116).
+    fn validate_or_fixup_guid(guid: Guid) -> Result<Guid> {
+        if guid.is_valid_for_sync_server() {
+            return Ok(guid);
+        }
+        #[cfg(feature = "fixup_invalid_guids")]
+        {
+            warn!("regenerating a login guid that is invalid for the sync server");
+            Ok(Guid::random())
+        }
+        #[cfg(not(feature = "fixup_invalid_guids"))]
+        {
+            Err(InvalidLogin::IllegalFieldValue {
+                field_info: "guid is not valid for the sync server".into(),
+            }
+            .into())
+        }
+    }
+
     pub fn add(&self, entry: LoginEntry) -> Result<EncryptedLogin> {
         let guid = Guid::random();
         let now_ms = util::system_time_ms_i64(SystemTime::now());
@@ -1574,6 +1612,44 @@ mod tests {
         assert_eq!(fetched.meta, meta);
     }
 
+    #[test]
+    fn test_add_with_meta_invalid_guid() {
+        ensure_initialized();
+
+        let now_ms = util::system_time_ms_i64(SystemTime::now());
+        // A guid containing a comma is invalid for the sync server.
+        let meta = LoginMeta {
+            id: "invalid,guid".to_string(),
+            time_created: now_ms,
+            time_password_changed: now_ms,
+            time_last_used: now_ms,
+            times_used: 1,
+            time_last_breach_alert_dismissed: None,
+        };
+        let db = LoginDb::open_in_memory();
+        let result = db.add_with_meta(LoginEntryWithMeta {
+            entry: LoginEntry {
+                origin: "https://www.example.com".into(),
+                http_realm: Some("https://www.example.com".into()),
+                username: "test".into(),
+                password: "sekret".into(),
+                ..LoginEntry::default()
+            },
+            meta,
+        });
+
+        // Without the fixup feature the invalid guid is rejected; with it, the guid
+        // is regenerated to one that is valid for the sync server.
+        #[cfg(not(feature = "fixup_invalid_guids"))]
+        assert!(result.is_err());
+
+        #[cfg(feature = "fixup_invalid_guids")]
+        {
+            let login = result.expect("invalid guid should be repaired");
+            assert!(Guid::new(&login.meta.id).is_valid_for_sync_server());
+        }
+    }
+
     #[test]
     fn test_add_with_meta_duplicate_id() {
         ensure_initialized();

components/logins/src/schema.rs

@@ -84,15 +84,17 @@
 
 use crate::error::*;
 use lazy_static::lazy_static;
-use rusqlite::Connection;
+use rusqlite::{named_params, Connection};
 use sql_support::ConnExt;
+use sync_guid::Guid;
 
 /// Version 1: SQLCipher -> plaintext migration.
 /// Version 2: addition of `loginsM.enc_unknown_fields`.
 /// Version 3: addition of `timeOfLastBreach` and `timeLastBreachAlertDismissed`.
 /// Version 4: addition of `breachesL` table
 /// Version 5: removal of `timeOfLastBreach`.
-pub(super) const VERSION: i64 = 5;
+/// Version 6: repair of local guids that are invalid for the sync server (bug 2056116).
+pub(super) const VERSION: i64 = 6;
 
 /// Every column shared by both tables except for `id`
 ///
@@ -272,11 +274,44 @@ fn upgrade_from(db: &Connection, from: i64) -> Result<()> {
         ALTER TABLE loginsM DROP COLUMN timeOfLastBreach;",
         )?),
 
+        5 => repair_invalid_local_guids(db),
+
         // next migration, add here
         _ => Err(Error::IncompatibleVersion(from)),
     }
 }
 
+/// Regenerates the guid of any local login whose guid is invalid for the sync
+/// server (empty, longer than 64 chars, or containing a comma / non-printable
+/// byte). Such logins could be persisted by the "with meta" import path before
+/// the guid was validated, and would panic the sync uploader (bug 2056116).
+///
+/// Only `loginsL` is touched: `loginsM` mirrors server records whose guids are
+/// always valid, and rewriting them would break the mirror's tie to the server.
+/// A login with an invalid guid can never have reached the server, so giving it a
+/// fresh guid loses no sync identity, and the guid is not used as part of the
+/// `secFields` encryption, so the credentials remain decryptable.
+fn repair_invalid_local_guids(db: &Connection) -> Result<()> {
+    let invalid_guids: Vec<String> = db
+        .query_rows_and_then("SELECT guid FROM loginsL", [], |row| {
+            row.get::<_, String>(0)
+        })?
+        .into_iter()
+        .filter(|guid| !Guid::new(guid).is_valid_for_sync_server())
+        .collect();
+    for old_guid in invalid_guids {
+        let new_guid = Guid::random();
+        db.execute(
+            "UPDATE loginsL SET guid = :new_guid WHERE guid = :old_guid",
+            named_params! {
+                ":new_guid": new_guid.as_str(),
+                ":old_guid": old_guid,
+            },
+        )?;
+    }
+    Ok(())
+}
+
 pub(crate) fn create(db: &Connection) -> Result<()> {
     debug!("Creating schema");
     db.execute_all(&[
@@ -315,6 +350,7 @@ CREATE TABLE IF NOT EXISTS loginsL (
     timeLastUsed        INTEGER,
     timePasswordChanged INTEGER NOT NULL,
     secFields           TEXT,
+    guid                TEXT NOT NULL UNIQUE,
 
     local_modified INTEGER,
 
@@ -369,4 +405,31 @@ PRAGMA user_version=1;
         let version = db.conn_ext_query_one::<i64>("PRAGMA user_version").unwrap();
         assert_eq!(version, VERSION);
     }
+
+    #[test]
+    fn test_repair_invalid_local_guids() {
+        use crate::db::test_utils::insert_login;
+
+        ensure_initialized();
+        let db = LoginDb::open_in_memory();
+        // Insert (bypassing validation) a local login with a guid the sync server
+        // would reject, alongside a normal one.
+        insert_login(&db, "invalid,guid", Some("password"), None);
+        insert_login(&db, "valid", Some("password"), None);
+
+        repair_invalid_local_guids(&db).unwrap();
+
+        let guids: Vec<String> = db
+            .query_rows_and_then("SELECT guid FROM loginsL", [], |row| {
+                row.get::<_, String>(0)
+            })
+            .unwrap();
+        assert_eq!(guids.len(), 2);
+        // The valid guid is untouched; the invalid one was regenerated to a valid guid.
+        assert!(guids.contains(&"valid".to_string()));
+        assert!(!guids.contains(&"invalid,guid".to_string()));
+        assert!(guids
+            .iter()
+            .all(|g| Guid::new(g).is_valid_for_sync_server()));
+    }
 }

components/logins/src/sync/engine.rs

@@ -258,11 +258,22 @@ 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 that is invalid for the sync server would panic the uploader
+                // (bug 2056116). Such a record can never have reached the server, so
+                // skip it instead of blocking the whole sync; the schema migration
+                // repairs these locally so they can sync on a later run.
+                if !guid.is_valid_for_sync_server() {
+                    warn!(
+                        "skipping outgoing login with a guid that is invalid for the sync server"
+                    );
+                    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 +284,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(
@@ -819,6 +830,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 whose guid the sync server would reject (contains a comma),
+        // inserted directly to mimic a record imported 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,12 @@ 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() {
+                        // A record with an invalid ID can never be accepted by the
+                        // sync server. Return an error rather than panicking the
+                        // 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 +238,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 +391,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());
     }
 }