Bug 1394139 - [Form Autofill] Don't save obviously bogus phone numbers, r=MattN
authorLuke Chang <lchang@mozilla.com>
Thu, 31 Aug 2017 19:08:50 +0800
changeset 379166 9683379b676231fe7b7b8cce364dc10754c91915
parent 379165 b38fde90b1c805d791cad6cb3de755c4155a661b
child 379167 7d9dde6b07c42ec33683f48469ce223b148b8622
push id32451
push userkwierso@gmail.com
push dateWed, 06 Sep 2017 22:51:37 +0000
treeherdermozilla-central@d8e238b811d3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1394139
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1394139 - [Form Autofill] Don't save obviously bogus phone numbers, r=MattN MozReview-Commit-ID: Bc1zJEmKFuh
browser/extensions/formautofill/FormAutofillHandler.jsm
browser/extensions/formautofill/FormAutofillUtils.jsm
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/unit/test_createRecords.js
browser/extensions/formautofill/test/unit/test_onFormSubmitted.js
browser/extensions/formautofill/test/unit/xpcshell.ini
--- a/browser/extensions/formautofill/FormAutofillHandler.jsm
+++ b/browser/extensions/formautofill/FormAutofillHandler.jsm
@@ -554,16 +554,18 @@ FormAutofillHandler.prototype = {
         data[type].record[detail.fieldName] = value;
 
         if (detail.state == "AUTO_FILLED") {
           data[type].untouchedFields.push(detail.fieldName);
         }
       });
     });
 
+    this.normalizeAddress(data.address);
+
     if (data.address &&
         Object.values(data.address.record).filter(v => v).length <
         FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) {
       log.debug("No address record saving since there are only",
                      Object.keys(data.address.record).length,
                      "usable fields");
       delete data.address;
     }
@@ -572,16 +574,46 @@ FormAutofillHandler.prototype = {
         !FormAutofillUtils.isCCNumber(data.creditCard.record["cc-number"]))) {
       log.debug("No credit card record saving since card number is invalid");
       delete data.creditCard;
     }
 
     return data;
   },
 
+  normalizeAddress(address) {
+    if (!address) {
+      return;
+    }
+
+    // Normalize Tel
+    FormAutofillUtils.compressTel(address.record);
+    if (address.record.tel) {
+      let allTelComponentsAreUntouched = Object.keys(address.record)
+        .filter(field => FormAutofillUtils.getCategoryFromFieldName(field) == "tel")
+        .every(field => address.untouchedFields.includes(field));
+      if (allTelComponentsAreUntouched) {
+        // No need to verify it if none of related fields are modified after autofilling.
+        if (!address.untouchedFields.includes("tel")) {
+          address.untouchedFields.push("tel");
+        }
+      } else {
+        let strippedNumber = address.record.tel.replace(/[\s\(\)-]/g, "");
+
+        // Remove "tel" if it contains invalid characters or the length of its
+        // number part isn't between 5 and 15.
+        // (The maximum length of a valid number in E.164 format is 15 digits
+        //  according to https://en.wikipedia.org/wiki/E.164 )
+        if (!/^(\+?)[\da-zA-Z]{5,15}$/.test(strippedNumber)) {
+          address.record.tel = "";
+        }
+      }
+    }
+  },
+
   async _decrypt(cipherText, reauth) {
     return new Promise((resolve) => {
       Services.cpmm.addMessageListener("FormAutofill:DecryptedString", function getResult(result) {
         Services.cpmm.removeMessageListener("FormAutofill:DecryptedString", getResult);
         resolve(result.data);
       });
 
       Services.cpmm.sendAsyncMessage("FormAutofill:GetDecryptedString", {cipherText, reauth});
--- a/browser/extensions/formautofill/FormAutofillUtils.jsm
+++ b/browser/extensions/formautofill/FormAutofillUtils.jsm
@@ -113,16 +113,42 @@ this.FormAutofillUtils = {
       return "";
     }
     return array
       .map(s => s ? s.trim() : "")
       .filter(s => s)
       .join(this.getAddressSeparator());
   },
 
+  /**
+   * In-place concatenate tel-related components into a single "tel" field and
+   * delete unnecessary fields.
+   * @param {object} address An address record.
+   */
+  compressTel(address) {
+    let telCountryCode = address["tel-country-code"] || "";
+    let telAreaCode = address["tel-area-code"] || "";
+
+    if (!address.tel) {
+      if (address["tel-national"]) {
+        address.tel = telCountryCode + address["tel-national"];
+      } else if (address["tel-local"]) {
+        address.tel = telCountryCode + telAreaCode + address["tel-local"];
+      } else if (address["tel-local-prefix"] && address["tel-local-suffix"]) {
+        address.tel = telCountryCode + telAreaCode + address["tel-local-prefix"] + address["tel-local-suffix"];
+      }
+    }
+
+    for (let field in address) {
+      if (field != "tel" && this.getCategoryFromFieldName(field) == "tel") {
+        delete address[field];
+      }
+    }
+  },
+
   fmtMaskedCreditCardLabel(maskedCCNum = "") {
     return {
       affix: "****",
       label: maskedCCNum.replace(/^\**/, ""),
     };
   },
 
   defineLazyLogGetter(scope, logPrefix) {
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -1304,36 +1304,24 @@ class Addresses extends AutofillRecords 
     delete address["country-name"];
   }
 
   _normalizeTel(address) {
     if (!address.tel && TEL_COMPONENTS.every(c => !address[c])) {
       return;
     }
 
-    let region = address["tel-country-code"] || address.country || FormAutofillUtils.DEFAULT_COUNTRY_CODE;
-    let number;
+    FormAutofillUtils.compressTel(address);
 
-    if (address.tel) {
-      number = address.tel;
-    } else if (address["tel-national"]) {
-      number = address["tel-national"];
-    } else if (address["tel-local"]) {
-      number = (address["tel-area-code"] || "") + address["tel-local"];
-    } else if (address["tel-local-prefix"] && address["tel-local-suffix"]) {
-      number = (address["tel-area-code"] || "") + address["tel-local-prefix"] + address["tel-local-suffix"];
-    }
+    let possibleRegion = address.country || FormAutofillUtils.DEFAULT_COUNTRY_CODE;
+    let tel = PhoneNumber.Parse(address.tel, possibleRegion);
 
-    let tel = PhoneNumber.Parse(number, region);
     if (tel && tel.internationalNumber) {
       // Force to save numbers in E.164 format if parse success.
       address.tel = tel.internationalNumber;
-    } else if (!address.tel) {
-      // Save the original number anyway if "tel" is omitted.
-      address.tel = number;
     }
 
     TEL_COMPONENTS.forEach(c => delete address[c]);
   }
 
   /**
    * Merge new address into the specified address if mergeable.
    *
new file mode 100644
--- /dev/null
+++ b/browser/extensions/formautofill/test/unit/test_createRecords.js
@@ -0,0 +1,143 @@
+/*
+ * Test for the normalization of records created by FormAutofillHandler.
+ */
+
+"use strict";
+
+Cu.import("resource://formautofill/FormAutofillHandler.jsm");
+
+const TESTCASES = [
+  {
+    description: "Don't create address record if filled data is less than 3",
+    document: `<form>
+                <input id="given-name" autocomplete="given-name">
+                <input id="family-name" autocomplete="family-name">
+                <input id="country" autocomplete="country">
+               </form>`,
+    formValue: {
+      "given-name": "John",
+      "family-name": "Doe",
+    },
+    expectedRecord: {
+      address: undefined,
+    },
+  },
+  {
+    description: "\"tel\" related fields should be concatenated",
+    document: `<form>
+                <input id="given-name" autocomplete="given-name">
+                <input id="family-name" autocomplete="family-name">
+                <input id="tel-country-code" autocomplete="tel-country-code">
+                <input id="tel-national" autocomplete="tel-national">
+               </form>`,
+    formValue: {
+      "given-name": "John",
+      "family-name": "Doe",
+      "tel-country-code": "+1",
+      "tel-national": "1234567890",
+    },
+    expectedRecord: {
+      address: {
+        "given-name": "John",
+        "family-name": "Doe",
+        "tel": "+11234567890",
+      },
+    },
+  },
+  {
+    description: "\"tel\" should be removed if it's too short",
+    document: `<form>
+                <input id="given-name" autocomplete="given-name">
+                <input id="family-name" autocomplete="family-name">
+                <input id="organization" autocomplete="organization">
+                <input id="tel" autocomplete="tel-national">
+               </form>`,
+    formValue: {
+      "given-name": "John",
+      "family-name": "Doe",
+      "organization": "Mozilla",
+      "tel": "1234",
+    },
+    expectedRecord: {
+      address: {
+        "given-name": "John",
+        "family-name": "Doe",
+        "organization": "Mozilla",
+        "tel": "",
+      },
+    },
+  },
+  {
+    description: "\"tel\" should be removed if it's too long",
+    document: `<form>
+                <input id="given-name" autocomplete="given-name">
+                <input id="family-name" autocomplete="family-name">
+                <input id="organization" autocomplete="organization">
+                <input id="tel" autocomplete="tel-national">
+               </form>`,
+    formValue: {
+      "given-name": "John",
+      "family-name": "Doe",
+      "organization": "Mozilla",
+      "tel": "1234567890123456",
+    },
+    expectedRecord: {
+      address: {
+        "given-name": "John",
+        "family-name": "Doe",
+        "organization": "Mozilla",
+        "tel": "",
+      },
+    },
+  },
+  {
+    description: "\"tel\" should be removed if it contains invalid characters",
+    document: `<form>
+                <input id="given-name" autocomplete="given-name">
+                <input id="family-name" autocomplete="family-name">
+                <input id="organization" autocomplete="organization">
+                <input id="tel" autocomplete="tel-national">
+               </form>`,
+    formValue: {
+      "given-name": "John",
+      "family-name": "Doe",
+      "organization": "Mozilla",
+      "tel": "12345###!!!",
+    },
+    expectedRecord: {
+      address: {
+        "given-name": "John",
+        "family-name": "Doe",
+        "organization": "Mozilla",
+        "tel": "",
+      },
+    },
+  },
+];
+
+for (let testcase of TESTCASES) {
+  add_task(async function() {
+    do_print("Starting testcase: " + testcase.description);
+
+    let doc = MockDocument.createTestDocument("http://localhost:8080/test/", testcase.document);
+    let form = doc.querySelector("form");
+    let formLike = FormLikeFactory.createFromForm(form);
+    let handler = new FormAutofillHandler(formLike);
+
+    handler.collectFormFields();
+
+    for (let id in testcase.formValue) {
+      doc.getElementById(id).value = testcase.formValue[id];
+    }
+
+    let record = handler.createRecords();
+
+    for (let type in testcase.expectedRecord) {
+      if (!testcase.expectedRecord[type]) {
+        do_check_eq(record[type], undefined);
+      } else {
+        Assert.deepEqual(record[type].record, testcase.expectedRecord[type]);
+      }
+    }
+  });
+}
--- a/browser/extensions/formautofill/test/unit/test_onFormSubmitted.js
+++ b/browser/extensions/formautofill/test/unit/test_onFormSubmitted.js
@@ -388,17 +388,94 @@ const TESTCASES = [
             "tel": "1-650-903-0800",
             "email": "",
           },
           untouchedFields: [],
         },
       },
     },
   },
-
+  {
+    description: "Shouldn't save tel whose length is too short",
+    formValue: {
+      "street-addr": "331 E. Evelyn Avenue",
+      "address-level1": "CA",
+      "country": "US",
+      "tel": "1234",
+    },
+    expectedResult: {
+      formSubmission: true,
+      records: {
+        address: {
+          guid: null,
+          record: {
+            "street-address": "331 E. Evelyn Avenue",
+            "address-level1": "CA",
+            "address-level2": "",
+            "country": "US",
+            "tel": "",
+            "email": "",
+          },
+          untouchedFields: [],
+        },
+      },
+    },
+  },
+  {
+    description: "Shouldn't save tel whose length is too long",
+    formValue: {
+      "street-addr": "331 E. Evelyn Avenue",
+      "address-level1": "CA",
+      "country": "US",
+      "tel": "1234567890123456",
+    },
+    expectedResult: {
+      formSubmission: true,
+      records: {
+        address: {
+          guid: null,
+          record: {
+            "street-address": "331 E. Evelyn Avenue",
+            "address-level1": "CA",
+            "address-level2": "",
+            "country": "US",
+            "tel": "",
+            "email": "",
+          },
+          untouchedFields: [],
+        },
+      },
+    },
+  },
+  {
+    description: "Shouldn't save tel which contains invalid characters",
+    formValue: {
+      "street-addr": "331 E. Evelyn Avenue",
+      "address-level1": "CA",
+      "country": "US",
+      "tel": "12345###!!",
+    },
+    expectedResult: {
+      formSubmission: true,
+      records: {
+        address: {
+          guid: null,
+          record: {
+            "street-address": "331 E. Evelyn Avenue",
+            "address-level1": "CA",
+            "address-level2": "",
+            "country": "US",
+            "tel": "",
+            "email": "",
+          },
+          untouchedFields: [],
+        },
+      },
+    },
+  },
 ];
 
 add_task(async function handle_earlyformsubmit_event() {
   do_print("Starting testcase: Test an invalid form element");
   let fakeForm = MOCK_DOC.createElement("form");
   sinon.spy(FormAutofillContent, "_onFormSubmit");
 
   do_check_eq(FormAutofillContent.notify(fakeForm), true);
--- a/browser/extensions/formautofill/test/unit/xpcshell.ini
+++ b/browser/extensions/formautofill/test/unit/xpcshell.ini
@@ -15,16 +15,17 @@ support-files =
 [heuristics/third_party/test_QVC.js]
 [heuristics/third_party/test_Sears.js]
 [heuristics/third_party/test_Staples.js]
 [heuristics/third_party/test_Walmart.js]
 [test_activeStatus.js]
 [test_addressRecords.js]
 [test_autofillFormFields.js]
 [test_collectFormFields.js]
+[test_createRecords.js]
 [test_creditCardRecords.js]
 [test_extractLabelStrings.js]
 [test_findLabelElements.js]
 [test_getAdaptedProfiles.js]
 [test_getCategoriesFromFieldNames.js]
 [test_getFormInputDetails.js]
 [test_getInfo.js]
 [test_getRecords.js]