Bug 1413479 - [Form Autofill] A field with an empty string in the incoming record shouldn't be saved to storage. r=steveck
MozReview-Commit-ID: Iwp05R4GS6X
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -308,52 +308,47 @@ class AutofillRecords {
* @param {boolean} [options.sourceSync = false]
* Did sync generate this addition?
* @returns {string}
* The GUID of the newly added item..
*/
add(record, {sourceSync = false} = {}) {
this.log.debug("add:", record);
+ let recordToSave = this._cloneAndCleanUp(record);
+
if (sourceSync) {
// Remove tombstones for incoming items that were changed on another
// device. Local deletions always lose to avoid data loss.
- let index = this._findIndexByGUID(record.guid, {
+ let index = this._findIndexByGUID(recordToSave.guid, {
includeDeleted: true,
});
if (index > -1) {
let existing = this.data[index];
if (existing.deleted) {
this.data.splice(index, 1);
} else {
- throw new Error(`Record ${record.guid} already exists`);
+ throw new Error(`Record ${recordToSave.guid} already exists`);
}
}
- let recordToSave = this._clone(record);
- return this._saveRecord(recordToSave, {sourceSync});
- }
+ } else if (!recordToSave.deleted) {
+ this._normalizeRecord(recordToSave);
+
+ recordToSave.guid = this._generateGUID();
+ recordToSave.version = this.version;
- if (record.deleted) {
- return this._saveRecord(record);
+ // Metadata
+ let now = Date.now();
+ recordToSave.timeCreated = now;
+ recordToSave.timeLastModified = now;
+ recordToSave.timeLastUsed = 0;
+ recordToSave.timesUsed = 0;
}
- let recordToSave = this._clone(record);
- this._normalizeRecord(recordToSave);
-
- recordToSave.guid = this._generateGUID();
- recordToSave.version = this.version;
-
- // Metadata
- let now = Date.now();
- recordToSave.timeCreated = now;
- recordToSave.timeLastModified = now;
- recordToSave.timeLastUsed = 0;
- recordToSave.timesUsed = 0;
-
- return this._saveRecord(recordToSave);
+ return this._saveRecord(recordToSave, {sourceSync});
}
_saveRecord(record, {sourceSync = false} = {}) {
if (!record.guid) {
throw new Error("Record missing GUID");
}
let recordToSave;
@@ -423,17 +418,17 @@ class AutofillRecords {
let oldValue = recordFound[field];
let newValue = recordToUpdate[field];
// Resume the old field value in the perserve case
if (preserveOldProperties && newValue === undefined) {
newValue = oldValue;
}
- if (!newValue) {
+ if (newValue === undefined || newValue === "") {
delete recordFound[field];
} else {
recordFound[field] = newValue;
}
this._maybeStoreLastSyncedField(recordFound, field, oldValue);
}
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -34,17 +34,17 @@ const TEST_ADDRESS_3 = {
const TEST_ADDRESS_4 = {
"given-name": "Timothy",
"additional-name": "John",
"family-name": "Berners-Lee",
organization: "World Wide Web Consortium",
};
-const TEST_ADDRESS_FOR_UPDATE = {
+const TEST_ADDRESS_WITH_EMPTY_FIELD = {
"name": "Tim Berners",
"street-address": "",
};
const TEST_ADDRESS_WITH_INVALID_FIELD = {
"street-address": "Another Address",
invalidField: "INVALID",
};
@@ -294,16 +294,22 @@ add_task(async function test_add() {
do_check_neq(addresses[0].guid, undefined);
do_check_eq(addresses[0].version, 1);
do_check_neq(addresses[0].timeCreated, undefined);
do_check_eq(addresses[0].timeLastModified, addresses[0].timeCreated);
do_check_eq(addresses[0].timeLastUsed, 0);
do_check_eq(addresses[0].timesUsed, 0);
+ // Empty string should be deleted before saving.
+ profileStorage.addresses.add(TEST_ADDRESS_WITH_EMPTY_FIELD);
+ let address = profileStorage.addresses.data[2];
+ do_check_eq(address.name, TEST_ADDRESS_WITH_EMPTY_FIELD.name);
+ do_check_eq(address["street-address"], undefined);
+
Assert.throws(() => profileStorage.addresses.add(TEST_ADDRESS_WITH_INVALID_FIELD),
/"invalidField" is not a valid field\./);
});
add_task(async function test_update() {
let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
[TEST_ADDRESS_1, TEST_ADDRESS_2]);
@@ -328,31 +334,37 @@ add_task(async function test_update() {
let address = profileStorage.addresses.get(guid, {rawData: true});
do_check_eq(address.country, undefined);
do_check_neq(address.timeLastModified, timeLastModified);
do_check_record_matches(address, TEST_ADDRESS_3);
do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
// Test preserveOldProperties parameter and field with empty string.
- profileStorage.addresses.update(guid, TEST_ADDRESS_FOR_UPDATE, true);
+ profileStorage.addresses.update(guid, TEST_ADDRESS_WITH_EMPTY_FIELD, true);
await onChanged;
await profileStorage._saveImmediately();
profileStorage.addresses.pullSyncChanges(); // force sync metadata, which we check below.
address = profileStorage.addresses.get(guid, {rawData: true});
do_check_eq(address["given-name"], "Tim");
do_check_eq(address["family-name"], "Berners");
do_check_eq(address["street-address"], undefined);
do_check_eq(address["postal-code"], "12345");
do_check_neq(address.timeLastModified, timeLastModified);
do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 2);
+ // Empty string should be deleted while updating.
+ profileStorage.addresses.update(profileStorage.addresses.data[0].guid, TEST_ADDRESS_WITH_EMPTY_FIELD);
+ address = profileStorage.addresses.data[0];
+ do_check_eq(address.name, TEST_ADDRESS_WITH_EMPTY_FIELD.name);
+ do_check_eq(address["street-address"], undefined);
+
Assert.throws(
() => profileStorage.addresses.update("INVALID_GUID", TEST_ADDRESS_3),
/No matching record\./
);
Assert.throws(
() => profileStorage.addresses.update(guid, TEST_ADDRESS_WITH_INVALID_FIELD),
/"invalidField" is not a valid field\./
--- a/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
@@ -23,16 +23,22 @@ const TEST_CREDIT_CARD_2 = {
};
const TEST_CREDIT_CARD_3 = {
"cc-number": "9999888877776666",
"cc-exp-month": 1,
"cc-exp-year": 2000,
};
+const TEST_CREDIT_CARD_WITH_EMPTY_FIELD = {
+ "cc-name": "",
+ "cc-number": "1234123412341234",
+ "cc-exp-month": 1,
+};
+
const TEST_CREDIT_CARD_WITH_2_DIGITS_YEAR = {
"cc-number": "1234123412341234",
"cc-exp-month": 1,
"cc-exp-year": 12,
};
const TEST_CREDIT_CARD_WITH_INVALID_FIELD = {
"cc-name": "John Doe",
@@ -165,16 +171,22 @@ add_task(async function test_add() {
do_check_neq(creditCards[0].guid, undefined);
do_check_eq(creditCards[0].version, 1);
do_check_neq(creditCards[0].timeCreated, undefined);
do_check_eq(creditCards[0].timeLastModified, creditCards[0].timeCreated);
do_check_eq(creditCards[0].timeLastUsed, 0);
do_check_eq(creditCards[0].timesUsed, 0);
+ // Empty string should be deleted before saving.
+ profileStorage.creditCards.add(TEST_CREDIT_CARD_WITH_EMPTY_FIELD);
+ let creditCard = profileStorage.creditCards.data[2];
+ do_check_eq(creditCard["cc-exp-month"], TEST_CREDIT_CARD_WITH_EMPTY_FIELD["cc-exp-month"]);
+ do_check_eq(creditCard["cc-name"], undefined);
+
Assert.throws(() => profileStorage.creditCards.add(TEST_CREDIT_CARD_WITH_INVALID_FIELD),
/"invalidField" is not a valid field\./);
});
add_task(async function test_update() {
let path = getTempFile(TEST_STORE_FILE_NAME).path;
await prepareTestCreditCards(path);
@@ -197,16 +209,22 @@ add_task(async function test_update() {
await profileStorage.initialize();
let creditCard = profileStorage.creditCards.get(guid);
do_check_eq(creditCard["cc-name"], undefined);
do_check_neq(creditCard.timeLastModified, timeLastModified);
do_check_credit_card_matches(creditCard, TEST_CREDIT_CARD_3);
+ // Empty string should be deleted while updating.
+ profileStorage.creditCards.update(profileStorage.creditCards.data[0].guid, TEST_CREDIT_CARD_WITH_EMPTY_FIELD);
+ creditCard = profileStorage.creditCards.data[0];
+ do_check_eq(creditCard["cc-exp-month"], TEST_CREDIT_CARD_WITH_EMPTY_FIELD["cc-exp-month"]);
+ do_check_eq(creditCard["cc-name"], undefined);
+
Assert.throws(
() => profileStorage.creditCards.update("INVALID_GUID", TEST_CREDIT_CARD_3),
/No matching record\./
);
Assert.throws(
() => profileStorage.creditCards.update(guid, TEST_CREDIT_CARD_WITH_INVALID_FIELD),
/"invalidField" is not a valid field\./