diff --git a/CHANGELOG.md b/CHANGELOG.md index 5170a68ad7..2676e0f1f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## ✨ What's Changed ✨ +### Autofill + +- Added credit card equivalents of the address bulk-import API, for applications migrating a credit card collection into this store: `add_credit_card_with_meta()`, `add_many_credit_cards_with_meta()`, `update_credit_card_with_meta()`, `add_many_credit_card_tombstones()` and `delete_all_credit_cards()`. These take the guid, timestamps and sync change counter from the caller, so a migrated record keeps the identity it already had. `cc_number_enc` is stored exactly as supplied and is not checked against the store's key, matching `add_credit_card()`. ([bug 2068982](https://bugzilla.mozilla.org/show_bug.cgi?id=2068982)) +- Timestamps outside the range a JS `Date` can represent are now reported as 0 wherever they enter the store: on the metadata an application supplies to the bulk-import APIs, on every read out of the local database, and on incoming sync payloads. This matches the treatment logins received in [bug 2066257](https://bugzilla.mozilla.org/show_bug.cgi?id=2066257) and covers addresses, credit cards and passports. ([bug 2068982](https://bugzilla.mozilla.org/show_bug.cgi?id=2068982)) + ### Logins - Timestamps outside the range a JS `Date` can represent are now reported as 0 wherever they enter or leave the store: on the metadata an application supplies to `add_with_meta()`, on every read out of the local database, and on incoming sync payloads, similar to what Desktop does. ([bug 2066257](https://bugzilla.mozilla.org/show_bug.cgi?id=2066257)) diff --git a/Cargo.lock b/Cargo.lock index 625905a7e7..0d2fe2f441 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2549,6 +2549,7 @@ dependencies = [ "sync15", "tempfile", "thiserror 2.0.3", + "types", "uniffi", "url", ] diff --git a/components/autofill/src/autofill.udl b/components/autofill/src/autofill.udl index 7fc40bace0..bcec6cc38d 100644 --- a/components/autofill/src/autofill.udl +++ b/components/autofill/src/autofill.udl @@ -107,6 +107,48 @@ dictionary Passport { i64 times_used; }; +/// Metadata fields managed internally by the library: the guid, timestamps and +/// local sync state. These are automatically set on `add_credit_card` and +/// updated on operations like `touch` and `update_credit_card`. Not included in +/// `UpdatableCreditCardFields`; use `add_credit_card_with_meta` when importing +/// records that already have metadata. +dictionary CreditCardMeta { + string guid; + i64 time_created; + i64? time_last_used; + i64 time_last_modified; + i64 times_used; + i64 sync_change_counter; +}; + +/// A credit card together with its metadata, passed to `add_credit_card_with_meta` +/// and `update_credit_card_with_meta` when importing a record from another store. +dictionary UpdatableCreditCardFieldsWithMeta { + UpdatableCreditCardFields fields; + CreditCardMeta meta; +}; + +/// A bulk insert result entry, returned per input record by `add_many_credit_cards_with_meta` +[Enum] +interface CreditCardBulkResultEntry { + Success(CreditCard credit_card); + Error(string message); +}; + +/// A tombstone for a record deleted locally but not yet uploaded, supplied to +/// `add_many_credit_card_tombstones` when migrating from another store. +dictionary CreditCardTombstone { + string guid; + i64 time_deleted; +}; + +/// Per-record result of `add_many_credit_card_tombstones`. +[Enum] +interface CreditCardBulkTombstoneResultEntry { + Success(string guid); + Error(string message); +}; + /// Metadata fields managed internally by the library: the guid, timestamps and /// local sync state. These are automatically set on `add_address` and updated on /// operations like `touch` and `update_address`. Not included in @@ -189,6 +231,27 @@ interface Store { [Throws=AutofillApiError] void touch_credit_card(string guid); + [Throws=AutofillApiError] + CreditCard add_credit_card_with_meta(UpdatableCreditCardFieldsWithMeta entry_with_meta); + + [Throws=AutofillApiError] + sequence add_many_credit_cards_with_meta(sequence entries_with_meta); + + [Throws=AutofillApiError] + sequence add_many_credit_card_tombstones(sequence tombstones); + + /// Removes every credit card and every credit card tombstone. + /// + /// A migration primitive: it leaves the sync mirror intact and produces no + /// tombstones, so the deletions are never uploaded and a synced profile gets + /// the records back on the next sync. Use `delete_credit_card` to delete on the + /// user's behalf. + [Throws=AutofillApiError] + void delete_all_credit_cards(); + + [Throws=AutofillApiError] + void update_credit_card_with_meta(UpdatableCreditCardFieldsWithMeta entry_with_meta); + [Throws=AutofillApiError] Address add_address(UpdatableAddressFields a); @@ -202,6 +265,11 @@ interface Store { sequence add_many_address_tombstones(sequence tombstones); /// Removes every address and every address tombstone. + /// + /// A migration primitive: it leaves the sync mirror intact and produces no + /// tombstones, so the deletions are never uploaded and a synced profile gets + /// the records back on the next sync. Use `delete_address` to delete on the + /// user's behalf. [Throws=AutofillApiError] void delete_all_addresses(); diff --git a/components/autofill/src/db/addresses.rs b/components/autofill/src/db/addresses.rs index f789abc236..6a613132c9 100644 --- a/components/autofill/src/db/addresses.rs +++ b/components/autofill/src/db/addresses.rs @@ -12,6 +12,7 @@ use crate::db::{ }, schema::{ADDRESS_COMMON_COLS, ADDRESS_COMMON_VALS}, }; +use crate::db::{timestamp_from_millis, with_savepoint, CounterUpdate}; use crate::error::*; use rusqlite::{Connection, Transaction}; @@ -86,33 +87,6 @@ pub(crate) fn add_many_addresses_with_meta( Ok(results) } -/// Runs `op` in a savepoint, rolling back to it if `op` fails, so that a record -/// reported as an error by the bulk functions leaves nothing behind. The shared -/// triggers reject a guid that exists in the counterpart table with -/// `RAISE(FAIL)`, which aborts the statement but keeps the row it already -/// inserted - so without this the offending row would be committed along with -/// the rest of the batch, putting the guid in both `addresses_data` and -/// `addresses_tombstones`. -/// -/// The outer `Result` is a savepoint failure and aborts the batch; the inner one -/// is the record's own failure. -fn with_savepoint( - tx: &Transaction<'_>, - op: impl FnOnce() -> Result, -) -> Result> { - tx.execute_batch("SAVEPOINT bulk_record")?; - match op() { - Ok(value) => { - tx.execute_batch("RELEASE bulk_record")?; - Ok(Ok(value)) - } - Err(e) => { - tx.execute_batch("ROLLBACK TO bulk_record; RELEASE bulk_record")?; - Ok(Err(e)) - } - } -} - /// Removes every address and every address tombstone, in one transaction. /// /// Deleting the rows alone is not enough. A delete leaves a tombstone behind for @@ -160,14 +134,6 @@ pub(crate) fn add_many_address_tombstones( Ok(results) } -/// `Timestamp` is a `u64`, so a negative millisecond value would wrap to a huge -/// one and then win every "latest wins" comparison in `Metadata::merge`. Clamp to -/// 0, which already means "unset" for these fields. The tuple constructor is used -/// rather than `Timestamp::from`, which asserts non-zero. -fn timestamp_from_millis(millis: i64) -> Timestamp { - Timestamp(millis.max(0) as u64) -} - fn internal_address_from_meta( fields: UpdatableAddressFields, meta: &AddressMeta, @@ -345,31 +311,6 @@ pub(crate) fn update_address( Ok(()) } -/// How `update_internal_address` should treat the change counter. -pub(crate) enum CounterUpdate { - /// Record a local change awaiting upload. - Increment, - /// Leave the counter alone, for a change that must not be uploaded - eg one - /// applied by Sync, which is already what the server has. - Leave, - /// Replace the counter, for a record whose counter is owned by the caller. - Set(i64), -} - -impl CounterUpdate { - /// The SQL assigned to `sync_change_counter`, and the value bound to - /// `:counter` within it. `Leave` adds 0 rather than dropping `:counter` from - /// the SQL, because rusqlite rejects a named parameter the statement doesn't - /// use. - fn as_sql(&self) -> (&'static str, i64) { - match self { - Self::Increment => ("sync_change_counter + :counter", 1), - Self::Leave => ("sync_change_counter + :counter", 0), - Self::Set(counter) => (":counter", *counter), - } - } -} - /// Updates all fields including metadata - although the change counter gets /// slightly special treatment, see `CounterUpdate`. pub(crate) fn update_internal_address( @@ -950,20 +891,56 @@ mod tests { } #[test] - fn test_address_add_with_meta_clamps_negative_timestamps() -> Result<()> { + fn test_address_add_with_meta_sanitizes_out_of_range_timestamps() -> Result<()> { let db = new_mem_db(); - let meta = AddressMeta { - guid: "abc".to_string(), - time_created: -1, - time_last_used: Some(-1), - time_last_modified: -1, - times_used: 0, - sync_change_counter: 0, - }; - add_address_with_meta(&db, test_fields("123 Main Street"), meta)?; + // Negative, and the value from bug 2066257 - a negative microsecond + // timestamp that a JS consumer already reinterpreted as a u64 and + // divided by 1000, so it reaches us as a huge positive number. Both are + // "we don't know when", and a `.max(0)` would only catch the first. + for (guid, out_of_range) in [("abc", -1), ("def", 18446744071857664)] { + let meta = AddressMeta { + guid: guid.to_string(), + time_created: out_of_range, + time_last_used: Some(out_of_range), + time_last_modified: out_of_range, + times_used: 0, + sync_change_counter: 0, + }; + add_address_with_meta(&db, test_fields("123 Main Street"), meta)?; + + let retrieved = get_address(&db, &Guid::new(guid))?; + assert_eq!( + retrieved.metadata.time_created.as_millis(), + 0, + "{out_of_range} survived" + ); + assert_eq!(retrieved.metadata.time_last_used.as_millis(), 0); + assert_eq!(retrieved.metadata.time_last_modified.as_millis(), 0); + } - let retrieved = get_address(&db, &Guid::new("abc"))?; + Ok(()) + } + + /// Surface 2: a value already on disk, put there before the import path + /// sanitized anything. Reading it must repair rather than propagate it. + #[test] + fn test_address_from_row_sanitizes_corrupt_timestamps() -> Result<()> { + let db = new_mem_db(); + + let address = add_address(&db, test_fields("123 Main Street"))?; + db.execute( + // Three shapes that are not representable dates: the u64-reinterpreted + // value from bug 2066257, a raw negative, and MAX_DATE_MS + 1. + "UPDATE addresses_data + SET time_created = 18446744071857664, + time_last_used = -1, + time_last_modified = 8640000000000001 + WHERE guid = :guid", + rusqlite::named_params! { ":guid": address.guid }, + )?; + + let retrieved = get_address(&db, &address.guid)?; assert_eq!(retrieved.metadata.time_created.as_millis(), 0); assert_eq!(retrieved.metadata.time_last_used.as_millis(), 0); assert_eq!(retrieved.metadata.time_last_modified.as_millis(), 0); diff --git a/components/autofill/src/db/credit_cards.rs b/components/autofill/src/db/credit_cards.rs index 3499b64662..662794e4a3 100644 --- a/components/autofill/src/db/credit_cards.rs +++ b/components/autofill/src/db/credit_cards.rs @@ -5,10 +5,14 @@ use crate::db::{ models::{ - credit_card::{InternalCreditCard, UpdatableCreditCardFields}, + credit_card::{ + CreditCardMeta, InternalCreditCard, UpdatableCreditCardFields, + UpdatableCreditCardFieldsWithMeta, + }, Metadata, }, schema::{CREDIT_CARD_COMMON_COLS, CREDIT_CARD_COMMON_VALS}, + timestamp_from_millis, with_savepoint, CounterUpdate, }; use crate::error::*; @@ -52,6 +56,148 @@ pub(crate) fn add_credit_card( Ok(credit_card) } +/// Adds a credit card **including metadata**, taking the guid, timestamps and +/// sync change counter from the caller rather than generating them. Normally you +/// will use `add_credit_card` instead; this is for importing records from +/// another store that already have metadata. +/// +/// `cc_number_enc` is stored exactly as given and is not checked against the +/// store's key, matching `add_credit_card`. An importing application owns the +/// ciphertext it supplies. +pub(crate) fn add_credit_card_with_meta( + conn: &Connection, + fields: UpdatableCreditCardFields, + meta: CreditCardMeta, +) -> Result { + let tx = conn.unchecked_transaction()?; + let card = internal_credit_card_from_meta(fields, &meta); + add_internal_credit_card(&tx, &card)?; + tx.commit()?; + Ok(card) +} + +/// Adds multiple credit cards **including metadata** within a single +/// transaction. Each record gets its own result, so a record that fails to +/// insert is reported as `Err(message)` without aborting the rest of the batch. +pub(crate) fn add_many_credit_cards_with_meta( + conn: &Connection, + entries: Vec, +) -> Result>> { + let tx = conn.unchecked_transaction()?; + let mut results = Vec::with_capacity(entries.len()); + for entry in entries { + let card = internal_credit_card_from_meta(entry.fields, &entry.meta); + match with_savepoint(&tx, || add_internal_credit_card(&tx, &card))? { + Ok(()) => results.push(Ok(card)), + Err(e) => results.push(Err(e.to_string())), + } + } + tx.commit()?; + Ok(results) +} + +/// Removes every credit card and every credit card tombstone, in one +/// transaction. +/// +/// Deleting the rows alone is not enough. A delete leaves a tombstone behind for +/// any guid the sync mirror knows, and the insert trigger then rejects re-adding +/// that guid, so a wipe that kept them could not be followed by a re-import of +/// the same records. Clearing both tables is what makes the wipe repeatable. +pub(crate) fn delete_all_credit_cards(conn: &Connection) -> Result<()> { + let tx = conn.unchecked_transaction()?; + tx.execute("DELETE FROM credit_cards_data", [])?; + // After the data, so the tombstones the delete trigger just created go too. + tx.execute("DELETE FROM credit_cards_tombstones", [])?; + tx.commit()?; + Ok(()) +} + +/// Adds tombstones for records that were deleted locally but not yet uploaded, +/// within a single transaction and with a result per record. `time_deleted` comes +/// from the caller rather than being stamped as now, so that a deletion imported +/// from another store keeps its original time. Without the tombstone the next +/// sync has nothing to say the record was deleted and takes the server copy. +pub(crate) fn add_many_credit_card_tombstones( + conn: &Connection, + tombstones: Vec<(String, i64)>, +) -> Result>> { + let tx = conn.unchecked_transaction()?; + let mut results = Vec::with_capacity(tombstones.len()); + for (guid, time_deleted) in tombstones { + let inserted = with_savepoint(&tx, || { + tx.execute( + "INSERT INTO credit_cards_tombstones (guid, time_deleted) + VALUES (:guid, :time_deleted)", + rusqlite::named_params! { + ":guid": &guid, + ":time_deleted": timestamp_from_millis(time_deleted), + }, + )?; + Ok(()) + })?; + match inserted { + Ok(()) => results.push(Ok(guid)), + Err(e) => results.push(Err(e.to_string())), + } + } + tx.commit()?; + Ok(results) +} + +fn internal_credit_card_from_meta( + fields: UpdatableCreditCardFields, + meta: &CreditCardMeta, +) -> InternalCreditCard { + InternalCreditCard { + guid: Guid::new(&meta.guid), + cc_name: fields.cc_name, + cc_number_enc: fields.cc_number_enc, + cc_number_last_4: fields.cc_number_last_4, + cc_exp_month: fields.cc_exp_month, + cc_exp_year: fields.cc_exp_year, + cc_type: fields.cc_type, + metadata: Metadata { + time_created: timestamp_from_millis(meta.time_created), + time_last_used: timestamp_from_millis(meta.time_last_used.unwrap_or(0)), + time_last_modified: timestamp_from_millis(meta.time_last_modified), + times_used: meta.times_used, + sync_change_counter: meta.sync_change_counter, + }, + } +} + +/// Updates a credit card **including metadata**, setting both its fields and its +/// timestamps and `times_used` to the supplied values. Normally you will use +/// `update_credit_card` instead, which owns the metadata itself; this is for +/// keeping a record identical to one held in another store. Errors with +/// `NoSuchRecord` if the guid is absent. +pub(crate) fn update_credit_card_with_meta( + conn: &Connection, + fields: UpdatableCreditCardFields, + meta: CreditCardMeta, +) -> Result<()> { + let tx = conn.unchecked_transaction()?; + + let card = internal_credit_card_from_meta(fields, &meta); + // Checked up front because `update_internal_credit_card` does not report + // how many rows it changed. + let exists: bool = tx.query_row( + "SELECT EXISTS(SELECT 1 FROM credit_cards_data WHERE guid = :guid)", + rusqlite::named_params! { ":guid": card.guid }, + |row| row.get(0), + )?; + if !exists { + return Err(Error::NoSuchRecord(card.guid.to_string())); + } + update_internal_credit_card( + &tx, + &card, + CounterUpdate::Set(card.metadata.sync_change_counter), + )?; + tx.commit()?; + Ok(()) +} + pub(crate) fn add_internal_credit_card( tx: &Transaction<'_>, card: &InternalCreditCard, @@ -162,16 +308,16 @@ pub fn update_credit_card( } /// Updates all fields including metadata - although the change counter gets -/// slightly special treatment (eg, when called by Sync we don't want the -/// change counter incremented). +/// slightly special treatment, see `CounterUpdate`. pub(crate) fn update_internal_credit_card( tx: &Transaction<'_>, card: &InternalCreditCard, - flag_as_changed: bool, + counter: CounterUpdate, ) -> Result<()> { - let change_counter_increment = flag_as_changed as u32; // will be 1 or 0 + let (counter_sql, counter_value) = counter.as_sql(); tx.execute( - "UPDATE credit_cards_data + &format!( + "UPDATE credit_cards_data SET cc_name = :cc_name, cc_number_enc = :cc_number_enc, cc_number_last_4 = :cc_number_last_4, @@ -182,8 +328,9 @@ pub(crate) fn update_internal_credit_card( time_last_used = :time_last_used, time_last_modified = :time_last_modified, times_used = :times_used, - sync_change_counter = sync_change_counter + :change_incr - WHERE guid = :guid", + sync_change_counter = {counter_sql} + WHERE guid = :guid" + ), rusqlite::named_params! { ":cc_name": card.cc_name, ":cc_number_enc": card.cc_number_enc, @@ -195,7 +342,7 @@ pub(crate) fn update_internal_credit_card( ":time_last_used": card.metadata.time_last_used, ":time_last_modified": card.metadata.time_last_modified, ":times_used": card.metadata.times_used, - ":change_incr": change_counter_increment, + ":counter": counter_value, ":guid": card.guid, }, )?; @@ -294,6 +441,323 @@ pub(crate) mod tests { use nss_as::ensure_initialized; use sync15::bso::IncomingBso; + fn meta_test_fields(cc_name: &str) -> UpdatableCreditCardFields { + UpdatableCreditCardFields { + cc_name: cc_name.to_string(), + // The `credit_cards_data` CHECK constraint requires either an empty + // string or more than 20 characters, real ciphertext being long. + cc_number_enc: "0123456789012345678901234567890".to_string(), + cc_number_last_4: "1234".to_string(), + cc_exp_month: 4, + cc_exp_year: 2030, + cc_type: "visa".to_string(), + } + } + + fn meta_test_meta(guid: &str, sync_change_counter: i64) -> CreditCardMeta { + CreditCardMeta { + guid: guid.to_string(), + time_created: 1000, + time_last_used: Some(2000), + time_last_modified: 3000, + times_used: 4, + sync_change_counter, + } + } + + fn count_cc_tombstones(conn: &Connection, guid: &str) -> Result { + Ok(conn.query_row( + "SELECT COUNT(*) FROM credit_cards_tombstones WHERE guid = :guid", + rusqlite::named_params! { ":guid": guid }, + |row| row.get(0), + )?) + } + + #[test] + fn test_credit_card_add_with_meta() -> Result<()> { + let db = new_mem_db(); + + let saved = + add_credit_card_with_meta(&db, meta_test_fields("Jane Doe"), meta_test_meta("abc", 2))?; + + // the supplied guid is used rather than a fresh one being generated. + assert_eq!(saved.guid.as_str(), "abc"); + + let retrieved = get_credit_card(&db, &Guid::new("abc"))?; + assert_eq!(retrieved.cc_name, "Jane Doe"); + assert_eq!(retrieved.metadata.time_created.as_millis(), 1000); + assert_eq!(retrieved.metadata.time_last_used.as_millis(), 2000); + assert_eq!(retrieved.metadata.time_last_modified.as_millis(), 3000); + assert_eq!(retrieved.metadata.times_used, 4); + assert_eq!(retrieved.metadata.sync_change_counter, 2); + + Ok(()) + } + + #[test] + fn test_credit_card_add_with_meta_sanitizes_out_of_range_timestamps() -> Result<()> { + let db = new_mem_db(); + + // Negative, and the value from bug 2066257 - a negative microsecond + // timestamp that a JS consumer already reinterpreted as a u64 and + // divided by 1000, so it reaches us as a huge positive number. Both are + // "we don't know when", and a `.max(0)` would only catch the first. + for (guid, out_of_range) in [("abc", -1), ("def", 18446744071857664)] { + let meta = CreditCardMeta { + guid: guid.to_string(), + time_created: out_of_range, + time_last_used: Some(out_of_range), + time_last_modified: out_of_range, + times_used: 0, + sync_change_counter: 0, + }; + add_credit_card_with_meta(&db, meta_test_fields("Jane Doe"), meta)?; + + let retrieved = get_credit_card(&db, &Guid::new(guid))?; + assert_eq!( + retrieved.metadata.time_created.as_millis(), + 0, + "{out_of_range} survived" + ); + assert_eq!(retrieved.metadata.time_last_used.as_millis(), 0); + assert_eq!(retrieved.metadata.time_last_modified.as_millis(), 0); + } + + Ok(()) + } + + /// Surface 2: a value already on disk, put there before the import path + /// sanitized anything. Reading it must repair rather than propagate it. + #[test] + fn test_credit_card_from_row_sanitizes_corrupt_timestamps() -> Result<()> { + let db = new_mem_db(); + + let card = add_credit_card(&db, meta_test_fields("Jane Doe"))?; + db.execute( + // Three shapes that are not representable dates: the u64-reinterpreted + // value from bug 2066257, a raw negative, and MAX_DATE_MS + 1. + "UPDATE credit_cards_data + SET time_created = 18446744071857664, + time_last_used = -1, + time_last_modified = 8640000000000001 + WHERE guid = :guid", + rusqlite::named_params! { ":guid": card.guid }, + )?; + + let retrieved = get_credit_card(&db, &card.guid)?; + assert_eq!(retrieved.metadata.time_created.as_millis(), 0); + assert_eq!(retrieved.metadata.time_last_used.as_millis(), 0); + assert_eq!(retrieved.metadata.time_last_modified.as_millis(), 0); + + Ok(()) + } + + #[test] + fn test_credit_card_update_with_meta_keeps_supplied_counter() -> Result<()> { + let db = new_mem_db(); + + add_credit_card_with_meta(&db, meta_test_fields("Jane Doe"), meta_test_meta("abc", 0))?; + + // the supplied counter must be applied, not the one already in the row. + update_credit_card_with_meta( + &db, + meta_test_fields("Jane Q. Doe"), + meta_test_meta("abc", 1), + )?; + + let retrieved = get_credit_card(&db, &Guid::new("abc"))?; + assert_eq!(retrieved.cc_name, "Jane Q. Doe"); + assert_eq!(retrieved.metadata.sync_change_counter, 1); + + // and back down again. + update_credit_card_with_meta( + &db, + meta_test_fields("Jane Q. Doe"), + meta_test_meta("abc", 0), + )?; + assert_eq!( + get_credit_card(&db, &Guid::new("abc"))? + .metadata + .sync_change_counter, + 0 + ); + + Ok(()) + } + + #[test] + fn test_credit_card_update_with_meta_errors_when_missing() -> Result<()> { + let db = new_mem_db(); + + let result = update_credit_card_with_meta( + &db, + meta_test_fields("Jane Doe"), + meta_test_meta("abc", 3), + ); + assert!(matches!(result, Err(Error::NoSuchRecord(guid)) if guid == "abc")); + assert!(get_credit_card(&db, &Guid::new("abc")).is_err()); + + Ok(()) + } + + #[test] + fn test_credit_card_add_many_with_meta_isolates_failures() -> Result<()> { + let db = new_mem_db(); + + // the second entry has an empty guid, which the `credit_cards_data` + // CHECK constraint rejects. The others must still be inserted. + let results = add_many_credit_cards_with_meta( + &db, + vec![ + UpdatableCreditCardFieldsWithMeta { + fields: meta_test_fields("One"), + meta: meta_test_meta("aaa", 1), + }, + UpdatableCreditCardFieldsWithMeta { + fields: meta_test_fields("Two"), + meta: meta_test_meta("", 1), + }, + UpdatableCreditCardFieldsWithMeta { + fields: meta_test_fields("Three"), + meta: meta_test_meta("ccc", 1), + }, + ], + )?; + + assert_eq!(results.len(), 3); + assert!(results[0].is_ok()); + assert!(results[1].is_err()); + assert!(results[2].is_ok()); + assert_eq!(get_all_credit_cards(&db)?.len(), 2); + + Ok(()) + } + + #[test] + fn test_delete_all_credit_cards_allows_a_reimport() -> Result<()> { + let db = new_mem_db(); + + // A tombstone left by an earlier import, and a record sharing no guid + // with it. + add_many_credit_card_tombstones(&db, vec![("gone".to_string(), 1234)])?; + let card = add_credit_card(&db, meta_test_fields("Jane Doe"))?; + + delete_all_credit_cards(&db)?; + assert_eq!(get_all_credit_cards(&db)?.len(), 0); + let tombstones: i64 = + db.query_row("SELECT COUNT(*) FROM credit_cards_tombstones", [], |row| { + row.get(0) + })?; + assert_eq!(tombstones, 0, "tombstones are cleared with the records"); + + // The point of clearing them: re-importing the same guids succeeds, + // where the insert trigger would reject a guid still tombstoned. + let results = add_many_credit_cards_with_meta( + &db, + vec![ + UpdatableCreditCardFieldsWithMeta { + fields: meta_test_fields("Jane Doe"), + meta: CreditCardMeta { + guid: card.guid.to_string(), + ..Default::default() + }, + }, + UpdatableCreditCardFieldsWithMeta { + fields: meta_test_fields("Gone"), + meta: CreditCardMeta { + guid: "gone".to_string(), + ..Default::default() + }, + }, + ], + )?; + assert!( + results.iter().all(|r| r.is_ok()), + "a previously tombstoned guid can be re-imported: {results:?}" + ); + + Ok(()) + } + + #[test] + fn test_credit_card_add_many_tombstones() -> Result<()> { + let db = new_mem_db(); + + let results = add_many_credit_card_tombstones(&db, vec![("aaa".to_string(), 1234)])?; + assert_eq!(results.len(), 1); + assert!(results[0].is_ok()); + + // the supplied deletion time is used rather than being stamped as now. + let time_deleted: i64 = db.query_row( + "SELECT time_deleted FROM credit_cards_tombstones WHERE guid = 'aaa'", + [], + |row| row.get(0), + )?; + assert_eq!(time_deleted, 1234); + + Ok(()) + } + + #[test] + fn test_credit_card_add_many_tombstones_rejects_live_guid() -> Result<()> { + let db = new_mem_db(); + + add_credit_card_with_meta(&db, meta_test_fields("Jane Doe"), meta_test_meta("abc", 0))?; + + // a guid cannot be in both `credit_cards_data` and + // `credit_cards_tombstones`; the trigger enforcing that must not take + // the rest of the batch down. + let results = add_many_credit_card_tombstones( + &db, + vec![("abc".to_string(), 1234), ("ddd".to_string(), 5678)], + )?; + + assert_eq!(results.len(), 2); + assert!(results[0].is_err()); + assert!(results[1].is_ok()); + + // the rejected tombstone must not have been committed anyway - see + // `with_savepoint`. + assert_eq!(count_cc_tombstones(&db, "abc")?, 0); + assert!(get_credit_card(&db, &Guid::new("abc")).is_ok()); + assert_eq!(count_cc_tombstones(&db, "ddd")?, 1); + + Ok(()) + } + + #[test] + fn test_credit_card_add_many_with_meta_rejects_deleted_guid() -> Result<()> { + let db = new_mem_db(); + + add_many_credit_card_tombstones(&db, vec![("aaa".to_string(), 1234)])?; + + // the other side of the same invariant: a guid in + // `credit_cards_tombstones` cannot be inserted into + // `credit_cards_data`. + let results = add_many_credit_cards_with_meta( + &db, + vec![ + UpdatableCreditCardFieldsWithMeta { + fields: meta_test_fields("One"), + meta: meta_test_meta("aaa", 1), + }, + UpdatableCreditCardFieldsWithMeta { + fields: meta_test_fields("Two"), + meta: meta_test_meta("bbb", 1), + }, + ], + )?; + + assert_eq!(results.len(), 2); + assert!(results[0].is_err()); + assert!(results[1].is_ok()); + + assert!(get_credit_card(&db, &Guid::new("aaa")).is_err()); + assert_eq!(get_all_credit_cards(&db)?.len(), 1); + + Ok(()) + } + pub fn get_all( conn: &Connection, table_name: String, @@ -565,7 +1029,7 @@ pub(crate) mod tests { cc_type: "mastercard".to_string(), ..Default::default() }, - false, + CounterUpdate::Leave, )?; let record_exists: bool = tx.query_row( diff --git a/components/autofill/src/db/mod.rs b/components/autofill/src/db/mod.rs index cfb5e35500..6b8fe5ea9d 100644 --- a/components/autofill/src/db/mod.rs +++ b/components/autofill/src/db/mod.rs @@ -85,6 +85,71 @@ impl DerefMut for AutofillDb { } } +/// Runs `op` in a savepoint, rolling back to it if `op` fails, so that a record +/// reported as an error by a bulk function leaves nothing behind. The shared +/// triggers reject a guid that exists in the counterpart table with +/// `RAISE(FAIL)`, which aborts the statement but keeps the row it already +/// inserted - so without this the offending row would be committed along with +/// the rest of the batch, putting the guid in both the data and tombstone +/// tables. +/// +/// The outer `Result` is a savepoint failure and aborts the batch; the inner one +/// is the record's own failure. +pub(crate) fn with_savepoint( + tx: &rusqlite::Transaction<'_>, + op: impl FnOnce() -> Result, +) -> Result> { + tx.execute_batch("SAVEPOINT bulk_record")?; + match op() { + Ok(value) => { + tx.execute_batch("RELEASE bulk_record")?; + Ok(Ok(value)) + } + Err(e) => { + tx.execute_batch("ROLLBACK TO bulk_record; RELEASE bulk_record")?; + Ok(Err(e)) + } + } +} + +/// Builds a `Timestamp` from millis an application supplied. +/// +/// Anything that is not a representable date becomes 0, which already means +/// "unset" for these fields - see `sanitize_timestamp`. A bare `.max(0)` would +/// not be enough: the corrupt values actually seen in the wild arrive *already* +/// huge, because the negative-to-`u64` reinterpretation happened before the +/// value reached us, and one of those would win every "latest wins" comparison +/// in `Metadata::merge`. The tuple constructor is used rather than +/// `Timestamp::from`, which asserts non-zero. +pub(crate) fn timestamp_from_millis(millis: i64) -> types::Timestamp { + types::Timestamp(types::sanitize_timestamp(millis) as u64) +} + +/// How an `update_internal_*` should treat the record's change counter. +pub(crate) enum CounterUpdate { + /// Record a local change awaiting upload. + Increment, + /// Leave the counter alone, for a change that must not be uploaded - eg one + /// applied by Sync, which is already what the server has. + Leave, + /// Replace the counter, for a record whose counter is owned by the caller. + Set(i64), +} + +impl CounterUpdate { + /// The SQL assigned to `sync_change_counter`, and the value bound to + /// `:counter` within it. `Leave` adds 0 rather than dropping `:counter` from + /// the SQL, because rusqlite rejects a named parameter the statement doesn't + /// use. + pub(crate) fn as_sql(&self) -> (&'static str, i64) { + match self { + Self::Increment => ("sync_change_counter + :counter", 1), + Self::Leave => ("sync_change_counter + :counter", 0), + Self::Set(counter) => (":counter", *counter), + } + } +} + fn unurl_path(p: impl AsRef) -> PathBuf { p.as_ref() .to_str() diff --git a/components/autofill/src/db/models/address.rs b/components/autofill/src/db/models/address.rs index fa226739c0..d126ef4430 100644 --- a/components/autofill/src/db/models/address.rs +++ b/components/autofill/src/db/models/address.rs @@ -6,6 +6,7 @@ use super::Metadata; use rusqlite::Row; use sync_guid::Guid; +use types::Timestamp; // UpdatableAddressFields contains the fields we support for creating a new // address or updating an existing one. It's missing the guid, our "internal" @@ -162,9 +163,9 @@ impl InternalAddress { tel: row.get("tel")?, email: row.get("email")?, metadata: Metadata { - time_created: row.get("time_created")?, - time_last_used: row.get("time_last_used")?, - time_last_modified: row.get("time_last_modified")?, + time_created: row.get::<_, Timestamp>("time_created")?.sanitized(), + time_last_used: row.get::<_, Timestamp>("time_last_used")?.sanitized(), + time_last_modified: row.get::<_, Timestamp>("time_last_modified")?.sanitized(), times_used: row.get("times_used")?, sync_change_counter: row.get("sync_change_counter")?, }, diff --git a/components/autofill/src/db/models/credit_card.rs b/components/autofill/src/db/models/credit_card.rs index a4149c6e52..0be0397142 100644 --- a/components/autofill/src/db/models/credit_card.rs +++ b/components/autofill/src/db/models/credit_card.rs @@ -6,6 +6,7 @@ use super::Metadata; use rusqlite::Row; use sync_guid::Guid; +use types::Timestamp; #[derive(Debug, Clone, Default)] pub struct UpdatableCreditCardFields { @@ -19,6 +20,58 @@ pub struct UpdatableCreditCardFields { pub cc_type: String, } +/// Metadata fields managed internally by the library: the guid, timestamps and +/// local sync state. These are automatically set on `add_credit_card` and +/// updated on operations like `touch` and `update_credit_card`. Not included in +/// `UpdatableCreditCardFields`; use `add_credit_card_with_meta` when importing +/// records that already have metadata. +#[derive(Debug, Clone, Default)] +pub struct CreditCardMeta { + pub guid: String, + pub time_created: i64, + pub time_last_used: Option, + pub time_last_modified: i64, + pub times_used: i64, + /// Local changes not yet uploaded; 0 means it matches what was last synced. + pub sync_change_counter: i64, +} + +/// A tombstone for a record deleted locally but not yet uploaded, supplied to +/// `add_many_credit_card_tombstones` when migrating from another store. +#[derive(Debug, Clone, Default)] +pub struct CreditCardTombstone { + pub guid: String, + pub time_deleted: i64, +} + +/// Per-record result of `add_many_credit_card_tombstones`. +#[derive(Debug)] +pub enum CreditCardBulkTombstoneResultEntry { + Success { guid: String }, + Error { message: String }, +} + +/// A credit card together with its metadata, passed to +/// `add_credit_card_with_meta` and `update_credit_card_with_meta` when importing +/// a record from another store. +#[derive(Debug, Clone, Default)] +pub struct UpdatableCreditCardFieldsWithMeta { + pub fields: UpdatableCreditCardFields, + pub meta: CreditCardMeta, +} + +/// A bulk insert result entry, returned per input record by +/// `add_many_credit_cards_with_meta` so that one record failing does not abort +/// the batch. Note that although the success case is much larger than the error +/// case, this is negligible in real life, as we expect a very small +/// success/error ratio. +#[allow(clippy::large_enum_variant)] +#[derive(Debug)] +pub enum CreditCardBulkResultEntry { + Success { credit_card: CreditCard }, + Error { message: String }, +} + #[derive(Debug, Clone, Default)] pub struct CreditCard { pub guid: String, @@ -91,9 +144,9 @@ impl InternalCreditCard { cc_exp_year: row.get("cc_exp_year")?, cc_type: row.get("cc_type")?, metadata: Metadata { - time_created: row.get("time_created")?, - time_last_used: row.get("time_last_used")?, - time_last_modified: row.get("time_last_modified")?, + time_created: row.get::<_, Timestamp>("time_created")?.sanitized(), + time_last_used: row.get::<_, Timestamp>("time_last_used")?.sanitized(), + time_last_modified: row.get::<_, Timestamp>("time_last_modified")?.sanitized(), times_used: row.get("times_used")?, sync_change_counter: row.get("sync_change_counter")?, }, diff --git a/components/autofill/src/db/models/passport.rs b/components/autofill/src/db/models/passport.rs index 87f4d27983..bb7d7ed1b1 100644 --- a/components/autofill/src/db/models/passport.rs +++ b/components/autofill/src/db/models/passport.rs @@ -6,6 +6,7 @@ use super::Metadata; use rusqlite::Row; use sync_guid::Guid; +use types::Timestamp; // What you pass to create or update a passport. #[derive(Debug, Clone, Default)] @@ -100,9 +101,9 @@ impl InternalPassport { expiry_date_day: row.get("expiry_date_day")?, expiry_date_year: row.get("expiry_date_year")?, metadata: Metadata { - time_created: row.get("time_created")?, - time_last_used: row.get("time_last_used")?, - time_last_modified: row.get("time_last_modified")?, + time_created: row.get::<_, Timestamp>("time_created")?.sanitized(), + time_last_used: row.get::<_, Timestamp>("time_last_used")?.sanitized(), + time_last_modified: row.get::<_, Timestamp>("time_last_modified")?.sanitized(), times_used: row.get("times_used")?, sync_change_counter: row.get("sync_change_counter")?, }, diff --git a/components/autofill/src/db/passports.rs b/components/autofill/src/db/passports.rs index c8aa1100e5..3472c8eb55 100644 --- a/components/autofill/src/db/passports.rs +++ b/components/autofill/src/db/passports.rs @@ -205,6 +205,33 @@ mod tests { } } + /// Passports have no with-meta import and no sync engine, so the read path + /// is their only untrusted timestamp surface - but a value already on disk + /// still has to be repaired rather than propagated. + #[test] + fn test_passport_from_row_sanitizes_corrupt_timestamps() -> Result<()> { + let db = new_mem_db(); + + let saved = add_passport(&db, sample_fields("Jane Doe", "X1234567"))?; + db.execute( + // Three shapes that are not representable dates: the u64-reinterpreted + // value from bug 2066257, a raw negative, and MAX_DATE_MS + 1. + "UPDATE passports_data + SET time_created = 18446744071857664, + time_last_used = -1, + time_last_modified = 8640000000000001 + WHERE guid = :guid", + rusqlite::named_params! { ":guid": saved.guid }, + )?; + + let retrieved = get_passport(&db, &saved.guid)?; + assert_eq!(retrieved.metadata.time_created.as_millis(), 0); + assert_eq!(retrieved.metadata.time_last_used.as_millis(), 0); + assert_eq!(retrieved.metadata.time_last_modified.as_millis(), 0); + + Ok(()) + } + #[test] fn test_passport_create_and_read() -> Result<()> { let db = new_mem_db(); diff --git a/components/autofill/src/db/store.rs b/components/autofill/src/db/store.rs index 4bb7d1cd25..cf98154743 100644 --- a/components/autofill/src/db/store.rs +++ b/components/autofill/src/db/store.rs @@ -6,7 +6,10 @@ use crate::db::models::address::{ Address, AddressBulkResultEntry, AddressBulkTombstoneResultEntry, AddressTombstone, UpdatableAddressFields, UpdatableAddressFieldsWithMeta, }; -use crate::db::models::credit_card::{CreditCard, UpdatableCreditCardFields}; +use crate::db::models::credit_card::{ + CreditCard, CreditCardBulkResultEntry, CreditCardBulkTombstoneResultEntry, CreditCardTombstone, + UpdatableCreditCardFields, UpdatableCreditCardFieldsWithMeta, +}; use crate::db::models::passport::{Passport, UpdatablePassportFields}; use crate::db::{ addresses, credit_cards, credit_cards::CreditCardsDeletionMetrics, passports, AutofillDb, @@ -90,6 +93,97 @@ impl Store { Ok(credit_card.into()) } + /// Adds a credit card **including metadata**. Normally the metadata (guid, + /// timestamps, sync change counter) is owned by this store and + /// `add_credit_card` will take care of it. This is for the case where a + /// record is being migrated from another store that already holds it. + #[handle_error(Error)] + pub fn add_credit_card_with_meta( + &self, + entry_with_meta: UpdatableCreditCardFieldsWithMeta, + ) -> ApiResult { + Ok(credit_cards::add_credit_card_with_meta( + &self.lock_db()?.writer, + entry_with_meta.fields, + entry_with_meta.meta, + )? + .into()) + } + + /// Adds multiple credit cards **including metadata**, with a result per + /// record. + #[handle_error(Error)] + pub fn add_many_credit_cards_with_meta( + &self, + entries_with_meta: Vec, + ) -> ApiResult> { + let results = credit_cards::add_many_credit_cards_with_meta( + &self.lock_db()?.writer, + entries_with_meta, + )?; + Ok(results + .into_iter() + .map(|result| match result { + Ok(credit_card) => CreditCardBulkResultEntry::Success { + credit_card: credit_card.into(), + }, + Err(message) => CreditCardBulkResultEntry::Error { message }, + }) + .collect()) + } + + /// Adds tombstones for credit cards whose deletion has not yet been + /// uploaded, with a result per record. + #[handle_error(Error)] + pub fn add_many_credit_card_tombstones( + &self, + tombstones: Vec, + ) -> ApiResult> { + let results = credit_cards::add_many_credit_card_tombstones( + &self.lock_db()?.writer, + tombstones + .into_iter() + .map(|t| (t.guid, t.time_deleted)) + .collect(), + )?; + Ok(results + .into_iter() + .map(|result| match result { + Ok(guid) => CreditCardBulkTombstoneResultEntry::Success { guid }, + Err(message) => CreditCardBulkTombstoneResultEntry::Error { message }, + }) + .collect()) + } + + /// Removes every credit card and every credit card tombstone. + /// + /// A migration primitive: it leaves the sync mirror intact and produces no + /// tombstones, so the deletions are never uploaded and a synced profile gets + /// the records back on the next sync. Use `delete_credit_card` to delete on the + /// user's behalf. + #[handle_error(Error)] + pub fn delete_all_credit_cards(&self) -> ApiResult<()> { + credit_cards::delete_all_credit_cards(&self.lock_db()?.writer)?; + Ok(()) + } + + /// Updates a credit card **including metadata**, setting both its fields + /// and its timestamps and `times_used` to the supplied values. Normally you + /// will use `update_credit_card` instead, which leaves `time_last_modified` + /// to this store; this is for keeping a record identical to one held + /// elsewhere. Errors with `NoSuchRecord` if the guid is absent. + #[handle_error(Error)] + pub fn update_credit_card_with_meta( + &self, + entry_with_meta: UpdatableCreditCardFieldsWithMeta, + ) -> ApiResult<()> { + credit_cards::update_credit_card_with_meta( + &self.lock_db()?.writer, + entry_with_meta.fields, + entry_with_meta.meta, + ) + } + #[handle_error(Error)] pub fn get_credit_card(&self, guid: String) -> ApiResult { let credit_card = @@ -197,6 +291,11 @@ impl Store { } /// Removes every address and every address tombstone. + /// + /// A migration primitive: it leaves the sync mirror intact and produces no + /// tombstones, so the deletions are never uploaded and a synced profile gets + /// the records back on the next sync. Use `delete_address` to delete on the + /// user's behalf. #[handle_error(Error)] pub fn delete_all_addresses(&self) -> ApiResult<()> { addresses::delete_all_addresses(&self.lock_db()?.writer)?; diff --git a/components/autofill/src/sync/address/incoming.rs b/components/autofill/src/sync/address/incoming.rs index c58c4b08dc..a656443899 100644 --- a/components/autofill/src/sync/address/incoming.rs +++ b/components/autofill/src/sync/address/incoming.rs @@ -4,9 +4,10 @@ */ use super::AddressPayload; -use crate::db::addresses::{add_internal_address, update_internal_address, CounterUpdate}; +use crate::db::addresses::{add_internal_address, update_internal_address}; use crate::db::models::address::InternalAddress; use crate::db::schema::ADDRESS_COMMON_COLS; +use crate::db::CounterUpdate; use crate::error::*; use crate::sync::address::name_utils::{join_name_parts, split_name, NameParts}; use crate::sync::common::*; diff --git a/components/autofill/src/sync/address/mod.rs b/components/autofill/src/sync/address/mod.rs index 61ef0e6d70..5af981f345 100644 --- a/components/autofill/src/sync/address/mod.rs +++ b/components/autofill/src/sync/address/mod.rs @@ -132,9 +132,9 @@ impl InternalAddress { tel: p.entry.tel, email: p.entry.email, metadata: Metadata { - time_created: p.entry.time_created, - time_last_used: p.entry.time_last_used, - time_last_modified: p.entry.time_last_modified, + time_created: p.entry.time_created.sanitized(), + time_last_used: p.entry.time_last_used.sanitized(), + time_last_modified: p.entry.time_last_modified.sanitized(), times_used: p.entry.times_used, sync_change_counter: 0, }, @@ -244,3 +244,27 @@ fn get_forked_record(local_record: InternalAddress) -> InternalAddress { local_record_data } + +/// Surface 3: the sync server is a source we cannot refuse. A record whose +/// timestamps are not representable dates must be repaired on the way in, not +/// stored and then allowed to win every "latest wins" merge. +#[test] +fn test_from_payload_sanitizes_out_of_range_timestamps() { + let mut payload = InternalAddress { + street_address: "123 Main Street".to_string(), + ..Default::default() + } + .into_payload() + .unwrap(); + + // the value from bug 2066257, a u64-reinterpreted negative, and one just + // past the largest date a JS `Date` can hold. + payload.entry.time_created = Timestamp(18446744071857664); + payload.entry.time_last_used = Timestamp(u64::MAX); + payload.entry.time_last_modified = Timestamp(types::MAX_DATE_MS as u64 + 1); + + let address = InternalAddress::from_payload(payload).unwrap(); + assert_eq!(address.metadata.time_created.as_millis(), 0); + assert_eq!(address.metadata.time_last_used.as_millis(), 0); + assert_eq!(address.metadata.time_last_modified.as_millis(), 0); +} diff --git a/components/autofill/src/sync/credit_card/incoming.rs b/components/autofill/src/sync/credit_card/incoming.rs index f73feb3298..c8b40bf426 100644 --- a/components/autofill/src/sync/credit_card/incoming.rs +++ b/components/autofill/src/sync/credit_card/incoming.rs @@ -7,6 +7,7 @@ use super::CreditCardPayload; use crate::db::credit_cards::{add_internal_credit_card, update_internal_credit_card}; use crate::db::models::credit_card::InternalCreditCard; use crate::db::schema::CREDIT_CARD_COMMON_COLS; +use crate::db::CounterUpdate; use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::sync::common::*; @@ -223,7 +224,15 @@ impl ProcessIncomingRecordImpl for IncomingCreditCardsImpl { new_record: Self::Record, flag_as_changed: bool, ) -> Result<()> { - update_internal_credit_card(tx, &new_record, flag_as_changed)?; + update_internal_credit_card( + tx, + &new_record, + if flag_as_changed { + CounterUpdate::Increment + } else { + CounterUpdate::Leave + }, + )?; Ok(()) } diff --git a/components/autofill/src/sync/credit_card/mod.rs b/components/autofill/src/sync/credit_card/mod.rs index 213f8bf158..056a6c86c2 100644 --- a/components/autofill/src/sync/credit_card/mod.rs +++ b/components/autofill/src/sync/credit_card/mod.rs @@ -135,9 +135,9 @@ impl InternalCreditCard { cc_exp_year: p.entry.cc_exp_year, cc_type: p.entry.cc_type, metadata: Metadata { - time_created: p.entry.time_created, - time_last_used: p.entry.time_last_used, - time_last_modified: p.entry.time_last_modified, + time_created: p.entry.time_created.sanitized(), + time_last_used: p.entry.time_last_used.sanitized(), + time_last_modified: p.entry.time_last_modified.sanitized(), times_used: p.entry.times_used, sync_change_counter: 0, }, @@ -297,3 +297,30 @@ fn test_to_from_payload() { // But the encrypted value should not. assert_ne!(cc2.cc_number_enc, cc.cc_number_enc); } + +/// Surface 3: the sync server is a source we cannot refuse. A record whose +/// timestamps are not representable dates must be repaired on the way in, not +/// stored and then allowed to win every "latest wins" merge. +#[test] +fn test_from_payload_sanitizes_out_of_range_timestamps() { + nss_as::ensure_initialized(); + let key = crate::encryption::create_autofill_key().unwrap(); + let encdec = EncryptorDecryptor::new(&key).unwrap(); + let cc = InternalCreditCard { + cc_number_enc: crate::encryption::encrypt_string(key, "1234567812345678".to_string()) + .unwrap(), + ..Default::default() + }; + + let mut payload = cc.into_payload(&encdec).unwrap(); + // the value from bug 2066257, a u64-reinterpreted negative, and one just + // past the largest date a JS `Date` can hold. + payload.entry.time_created = Timestamp(18446744071857664); + payload.entry.time_last_used = Timestamp(u64::MAX); + payload.entry.time_last_modified = Timestamp(types::MAX_DATE_MS as u64 + 1); + + let cc = InternalCreditCard::from_payload(payload, &encdec).unwrap(); + assert_eq!(cc.metadata.time_created.as_millis(), 0); + assert_eq!(cc.metadata.time_last_used.as_millis(), 0); + assert_eq!(cc.metadata.time_last_modified.as_millis(), 0); +} diff --git a/components/logins/Cargo.toml b/components/logins/Cargo.toml index 2f126f9eba..39fc144d3e 100644 --- a/components/logins/Cargo.toml +++ b/components/logins/Cargo.toml @@ -34,6 +34,7 @@ perform_additional_origin_fixups = [] fixup_invalid_guids = [] [dependencies] +types = { path = "../support/types" } sync15 = { path = "../sync15" } serde = "1" serde_derive = "1" diff --git a/components/logins/src/util.rs b/components/logins/src/util.rs index 78dc0c2ad3..02c978cb3e 100644 --- a/components/logins/src/util.rs +++ b/components/logins/src/util.rs @@ -23,24 +23,9 @@ pub fn system_time_millis_from_row(row: &Row<'_>, col_name: &str) -> Result