Bug 1394139 - [Form Autofill] Don't save obviously bogus phone numbers. r=MattN, a=jcristau
authorLuke Chang <lchang@mozilla.com>
Thu, 31 Aug 2017 19:08:50 +0800
changeset 423995 7f8111c430c503c066c95c3b1ace2966d188ffc7
parent 423994 ab51d952c3e18341b2375aab2858bac748fdaa87
child 423996 a52bc131f20b76cb2a4bb90ceeee44ae8e77ca2b
push id1517
push userjlorenzo@mozilla.com
push dateThu, 14 Sep 2017 16:50:54 +0000
treeherdermozilla-release@3b41fd564418 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN, jcristau
bugs1394139
milestone56.0
Bug 1394139 - [Form Autofill] Don't save obviously bogus phone numbers. r=MattN, a=jcristau 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
@@ -508,25 +508,57 @@ 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;
     }
 
     if (data.creditCard && !data.creditCard.record["cc-number"]) {
       log.debug("No credit card record saving since card number is empty");
       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 = "";
+        }
+      }
+    }
+  },
 };
--- a/browser/extensions/formautofill/FormAutofillUtils.jsm
+++ b/browser/extensions/formautofill/FormAutofillUtils.jsm
@@ -91,16 +91,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];
+      }
+    }
+  },
+
   defineLazyLogGetter(scope, logPrefix) {
     XPCOMUtils.defineLazyGetter(scope, "log", () => {
       let ConsoleAPI = Cu.import("resource://gre/modules/Console.jsm", {}).ConsoleAPI;
       return new ConsoleAPI({
         maxLogLevelPref: "extensions.formautofill.loglevel",
         prefix: logPrefix,
       });
     });
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -1327,36 +1327,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]