From c1e0affe1893e1687c2de91a8a23140fa037ebe6 Mon Sep 17 00:00:00 2001 From: Johannes Salas Schmidt Date: Tue, 21 Jul 2026 14:14:31 +0200 Subject: [PATCH] Bug 2056116 - Don't crash sync on logins with invalid guids A login guid that's invalid for the sync server panicked the uploader ("record's ID is invalid"). sync15 now returns an error instead of asserting, fetch_outgoing skips such records, add_many_with_meta validates (or regenerates, under the fixup_invalid_guids feature) the guid, and a v6 migration repairs existing rows. --- CHANGELOG.md | 6 +++ components/logins/Cargo.toml | 6 +++ components/logins/src/db.rs | 80 +++++++++++++++++++++++++++- components/logins/src/schema.rs | 67 ++++++++++++++++++++++- components/logins/src/sync/engine.rs | 46 ++++++++++++++-- components/sync15/src/bso/content.rs | 20 ++++--- 6 files changed, 210 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e287f4be2e..9ce2726197 100644 --- a/CHANGELOG.md +++ b/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 ✨ diff --git a/components/logins/Cargo.toml b/components/logins/Cargo.toml index dcf2b8024c..2f126f9eba 100644 --- a/components/logins/Cargo.toml +++ b/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" } diff --git a/components/logins/src/db.rs b/components/logins/src/db.rs index a200a55048..5315d92bac 100644 --- a/components/logins/src/db.rs +++ b/components/logins/src/db.rs @@ -614,8 +614,19 @@ impl LoginDb { ) -> Result>> { 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 { + 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 { 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(); diff --git a/components/logins/src/schema.rs b/components/logins/src/schema.rs index bac2c335c8..c7b31ef781 100644 --- a/components/logins/src/schema.rs +++ b/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 = 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::("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 = 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())); + } } diff --git a/components/logins/src/sync/engine.rs b/components/logins/src/sync/engine.rs index 2e807fca86..d53742925a 100644 --- a/components/logins/src/sync/engine.rs +++ b/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> { 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::>() + bsos.filter_map(|r| r.transpose()).collect::>() } 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 = 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 f40d773d1e..9cd0460299 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,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()); } }