Bug 1418885 - [Form Autofill] Ensure all computed fields are removed after normalizing and strip trailing newlines in "street-address". r=steveck
MozReview-Commit-ID: Llhi9AZ3T8H
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -308,17 +308,17 @@ 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);
+ let recordToSave = this._clone(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(recordToSave.guid, {
includeDeleted: true,
});
if (index > -1) {
@@ -1318,51 +1318,46 @@ class Addresses extends AutofillRecords
_normalizeFields(address) {
this._normalizeName(address);
this._normalizeAddress(address);
this._normalizeCountry(address);
this._normalizeTel(address);
}
_normalizeName(address) {
- if (!address.name) {
- return;
- }
-
- let nameParts = FormAutofillNameUtils.splitName(address.name);
- if (!address["given-name"] && nameParts.given) {
- address["given-name"] = nameParts.given;
- }
- if (!address["additional-name"] && nameParts.middle) {
- address["additional-name"] = nameParts.middle;
- }
- if (!address["family-name"] && nameParts.family) {
- address["family-name"] = nameParts.family;
+ if (address.name) {
+ let nameParts = FormAutofillNameUtils.splitName(address.name);
+ if (!address["given-name"] && nameParts.given) {
+ address["given-name"] = nameParts.given;
+ }
+ if (!address["additional-name"] && nameParts.middle) {
+ address["additional-name"] = nameParts.middle;
+ }
+ if (!address["family-name"] && nameParts.family) {
+ address["family-name"] = nameParts.family;
+ }
}
delete address.name;
}
_normalizeAddress(address) {
- if (STREET_ADDRESS_COMPONENTS.every(c => !address[c])) {
- return;
- }
+ if (STREET_ADDRESS_COMPONENTS.some(c => !!address[c])) {
+ // Treat "street-address" as "address-line1" if it contains only one line
+ // and "address-line1" is omitted.
+ if (!address["address-line1"] && address["street-address"] &&
+ !address["street-address"].includes("\n")) {
+ address["address-line1"] = address["street-address"];
+ delete address["street-address"];
+ }
- // Treat "street-address" as "address-line1" if it contains only one line
- // and "address-line1" is omitted.
- if (!address["address-line1"] && address["street-address"] &&
- !address["street-address"].includes("\n")) {
- address["address-line1"] = address["street-address"];
- delete address["street-address"];
+ // Concatenate "address-line*" if "street-address" is omitted.
+ if (!address["street-address"]) {
+ address["street-address"] = STREET_ADDRESS_COMPONENTS.map(c => address[c]).join("\n").replace(/\n+$/, "");
+ }
}
-
- // Concatenate "address-line*" if "street-address" is omitted.
- if (!address["street-address"]) {
- address["street-address"] = STREET_ADDRESS_COMPONENTS.map(c => address[c]).join("\n");
- }
-
STREET_ADDRESS_COMPONENTS.forEach(c => delete address[c]);
}
_normalizeCountry(address) {
let country;
if (address.country) {
country = address.country.toUpperCase();
@@ -1376,30 +1371,27 @@ class Addresses extends AutofillRecords
} else {
delete address.country;
}
delete address["country-name"];
}
_normalizeTel(address) {
- if (!address.tel && TEL_COMPONENTS.every(c => !address[c])) {
- return;
- }
+ if (address.tel || TEL_COMPONENTS.some(c => !!address[c])) {
+ FormAutofillUtils.compressTel(address);
- FormAutofillUtils.compressTel(address);
+ let possibleRegion = address.country || FormAutofillUtils.DEFAULT_COUNTRY_CODE;
+ let tel = PhoneNumber.Parse(address.tel, possibleRegion);
- let possibleRegion = address.country || FormAutofillUtils.DEFAULT_COUNTRY_CODE;
- let tel = PhoneNumber.Parse(address.tel, possibleRegion);
-
- if (tel && tel.internationalNumber) {
- // Force to save numbers in E.164 format if parse success.
- address.tel = tel.internationalNumber;
+ if (tel && tel.internationalNumber) {
+ // Force to save numbers in E.164 format if parse success.
+ address.tel = tel.internationalNumber;
+ }
}
-
TEL_COMPONENTS.forEach(c => delete address[c]);
}
/**
* Merge new address into the specified address if mergeable.
*
* @param {string} guid
* Indicates which address to merge.
@@ -1552,21 +1544,20 @@ class CreditCards extends AutofillRecord
if (creditCard["cc-given-name"] || creditCard["cc-additional-name"] || creditCard["cc-family-name"]) {
if (!creditCard["cc-name"]) {
creditCard["cc-name"] = FormAutofillNameUtils.joinNameParts({
given: creditCard["cc-given-name"],
middle: creditCard["cc-additional-name"],
family: creditCard["cc-family-name"],
});
}
-
- delete creditCard["cc-given-name"];
- delete creditCard["cc-additional-name"];
- delete creditCard["cc-family-name"];
}
+ delete creditCard["cc-given-name"];
+ delete creditCard["cc-additional-name"];
+ delete creditCard["cc-family-name"];
}
_normalizeCCNumber(creditCard) {
if (creditCard["cc-number"]) {
creditCard["cc-number"] = FormAutofillUtils.normalizeCCNumber(creditCard["cc-number"]);
if (!creditCard["cc-number"]) {
delete creditCard["cc-number"];
}
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -35,20 +35,35 @@ 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_WITH_EMPTY_FIELD = {
- "name": "Tim Berners",
+ name: "Tim Berners",
"street-address": "",
};
+const TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD = {
+ name: "",
+ "address-line1": "",
+ "address-line2": "",
+ "address-line3": "",
+ "country-name": "",
+ "tel-country-code": "",
+ "tel-national": "",
+ "tel-area-code": "",
+ "tel-local": "",
+ "tel-local-prefix": "",
+ "tel-local-suffix": "",
+ email: "timbl@w3.org",
+};
+
const TEST_ADDRESS_WITH_INVALID_FIELD = {
"street-address": "Another Address",
invalidField: "INVALID",
};
const TEST_ADDRESS_EMPTY_AFTER_NORMALIZE = {
country: "XXXXXX",
};
@@ -332,16 +347,21 @@ add_task(async function test_add() {
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);
+ // Empty computed fields shouldn't cause any problem.
+ profileStorage.addresses.add(TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD);
+ address = profileStorage.addresses._data[3];
+ do_check_eq(address.email, TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD.email);
+
Assert.throws(() => profileStorage.addresses.add(TEST_ADDRESS_WITH_INVALID_FIELD),
/"invalidField" is not a valid field\./);
Assert.throws(() => profileStorage.addresses.add({}),
/Record contains no valid field\./);
Assert.throws(() => profileStorage.addresses.add(TEST_ADDRESS_EMPTY_AFTER_NORMALIZE),
/Record contains no valid field\./);
@@ -393,16 +413,24 @@ add_task(async function test_update() {
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);
+ // Empty computed fields shouldn't cause any problem.
+ profileStorage.addresses.update(profileStorage.addresses._data[0].guid, TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD, false);
+ address = profileStorage.addresses._data[0];
+ do_check_eq(address.email, TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD.email);
+ profileStorage.addresses.update(profileStorage.addresses._data[1].guid, TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD, true);
+ address = profileStorage.addresses._data[1];
+ do_check_eq(address.email, TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD.email);
+
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\./
@@ -571,19 +599,25 @@ add_task(async function test_mergeToStor
[TEST_ADDRESS_1, TEST_ADDRESS_2]);
// Merge an address to storage
let anotherAddress = profileStorage.addresses._clone(TEST_ADDRESS_2);
profileStorage.addresses.add(anotherAddress);
anotherAddress.email = "timbl@w3.org";
do_check_eq(profileStorage.addresses.mergeToStorage(anotherAddress).length, 2);
do_check_eq(profileStorage.addresses.getAll()[1].email, anotherAddress.email);
do_check_eq(profileStorage.addresses.getAll()[2].email, anotherAddress.email);
+
+ // Empty computed fields shouldn't cause any problem.
+ do_check_eq(profileStorage.addresses.mergeToStorage(TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD).length, 3);
});
add_task(async function test_mergeToStorage_strict() {
let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME,
[TEST_ADDRESS_1, TEST_ADDRESS_2]);
// Try to merge a subset with empty string
let anotherAddress = profileStorage.addresses._clone(TEST_ADDRESS_1);
anotherAddress.email = "";
do_check_eq(profileStorage.addresses.mergeToStorage(anotherAddress, true).length, 0);
do_check_eq(profileStorage.addresses.getAll()[0].email, TEST_ADDRESS_1.email);
+
+ // Empty computed fields shouldn't cause any problem.
+ do_check_eq(profileStorage.addresses.mergeToStorage(TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD, true).length, 1);
});
--- a/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
@@ -34,16 +34,24 @@ const TEST_CREDIT_CARD_4 = {
};
const TEST_CREDIT_CARD_WITH_EMPTY_FIELD = {
"cc-name": "",
"cc-number": "1234123412341234",
"cc-exp-month": 1,
};
+const TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD = {
+ "cc-given-name": "",
+ "cc-additional-name": "",
+ "cc-family-name": "",
+ "cc-exp": "",
+ "cc-number": "1928374619283746",
+};
+
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",
@@ -255,16 +263,22 @@ add_task(async function test_add() {
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);
+ // Empty computed fields shouldn't cause any problem.
+ profileStorage.creditCards.add(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD);
+ creditCard = profileStorage.creditCards._data[3];
+ do_check_eq(creditCard["cc-number"],
+ profileStorage.creditCards._getMaskedCCNumber(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD["cc-number"]));
+
Assert.throws(() => profileStorage.creditCards.add(TEST_CREDIT_CARD_WITH_INVALID_FIELD),
/"invalidField" is not a valid field\./);
Assert.throws(() => profileStorage.creditCards.add({}),
/Record contains no valid field\./);
Assert.throws(() => profileStorage.creditCards.add(TEST_CREDIT_CARD_EMPTY_AFTER_NORMALIZE),
/Record contains no valid field\./);
@@ -299,16 +313,26 @@ add_task(async function test_update() {
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);
+ // Empty computed fields shouldn't cause any problem.
+ profileStorage.creditCards.update(profileStorage.creditCards._data[0].guid, TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD, false);
+ creditCard = profileStorage.creditCards._data[0];
+ do_check_eq(creditCard["cc-number"],
+ profileStorage.creditCards._getMaskedCCNumber(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD["cc-number"]));
+ profileStorage.creditCards.update(profileStorage.creditCards._data[1].guid, TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD, true);
+ creditCard = profileStorage.creditCards._data[1];
+ do_check_eq(creditCard["cc-number"],
+ profileStorage.creditCards._getMaskedCCNumber(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD["cc-number"]));
+
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\./
@@ -487,9 +511,12 @@ add_task(async function test_mergeToStor
// Merge a creditCard to storage
let anotherCreditCard = profileStorage.creditCards._clone(TEST_CREDIT_CARD_3);
anotherCreditCard["cc-name"] = "Foo Bar";
do_check_eq(profileStorage.creditCards.mergeToStorage(anotherCreditCard).length, 2);
do_check_eq(profileStorage.creditCards.getAll()[0]["cc-name"], "Foo Bar");
do_check_eq(profileStorage.creditCards.getAll()[0]["cc-exp"], "2000-01");
do_check_eq(profileStorage.creditCards.getAll()[1]["cc-name"], "Foo Bar");
do_check_eq(profileStorage.creditCards.getAll()[1]["cc-exp"], "2000-01");
+
+ // Empty computed fields shouldn't cause any problem.
+ do_check_eq(profileStorage.creditCards.mergeToStorage(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD).length, 0);
});
--- a/browser/extensions/formautofill/test/unit/test_transformFields.js
+++ b/browser/extensions/formautofill/test/unit/test_transformFields.js
@@ -268,16 +268,54 @@ const ADDRESS_NORMALIZE_TESTCASES = [
"street-address": "street address\nstreet address line 2",
"address-line2": "line2",
"address-line3": "line3",
},
expectedResult: {
"street-address": "street address\nstreet address line 2",
},
},
+ {
+ description: "Has only \"address-line1~2\"",
+ address: {
+ "address-line1": "line1",
+ "address-line2": "line2",
+ },
+ expectedResult: {
+ "street-address": "line1\nline2",
+ },
+ },
+ {
+ description: "Has only \"address-line1\"",
+ address: {
+ "address-line1": "line1",
+ },
+ expectedResult: {
+ "street-address": "line1",
+ },
+ },
+ {
+ description: "Has only \"address-line2~3\"",
+ address: {
+ "address-line2": "line2",
+ "address-line3": "line3",
+ },
+ expectedResult: {
+ "street-address": "\nline2\nline3",
+ },
+ },
+ {
+ description: "Has only \"address-line2\"",
+ address: {
+ "address-line2": "line2",
+ },
+ expectedResult: {
+ "street-address": "\nline2",
+ },
+ },
// Country
{
description: "Has \"country\" in lowercase",
address: {
"country": "us",
},
expectedResult: {