Closed Bug 1394139 Opened 2 years ago Closed 2 years ago

Don't save obviously bogus phone numbers

Categories

(Toolkit :: Form Manager, defect, P1)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: bj, Assigned: lchang)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 file)

Looking through my saved addresses I find the following phone numbers saved:

* 978 nnn nnnn978-mmm-mmmm978-ppp-pppp
 (Three different valid phone numbers mashed together.)
* 978
 (Just my area code. There are several like this.)
* 617
 (Nearby area code.)
* K1
 (Not sure which site. I thing I was entering emergency contact information for a flight.)

Each of these (and other obviously bogus "phone numbers") shouldn't be saved.

For area codes it would be nice to check for the rest of the phone number in other fields, but don't just save a partial number.
Whiteboard: [form autofill] → [form autofill:MVP]
Assignee: nobody → lchang
Hi Matt,

I designed a rough rule to delete obviously invalid numbers but not sure if it's reasonable. Could you take a look? Thanks.
Comment on attachment 8903112 [details]
Bug 1394139 - [Form Autofill] Don't save obviously bogus phone numbers,

https://reviewboard.mozilla.org/r/174910/#review180294

::: browser/extensions/formautofill/ProfileStorage.jsm:1325
(Diff revision 1)
>        // 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.
> +    } else {
> +      // Save the number anyway if it contains only valid characters and the
> +      // length of its number part is between 5 and 15.
> +      // (The maxinum length of a valid number in E.164 format is 15 digits

Typo: maximum

::: browser/extensions/formautofill/ProfileStorage.jsm:1327
(Diff revision 1)
> +      number = number.replace(/[\s\(\)-]/g, "");
> +      if (number.match(/^(\+?)[\da-zA-Z]{5,15}$/)) {

I was thinking we would leave storage more generic and do this only when saving data from a form submission.

It kinda think we should preserve whitespace for saving but ignore formatting characters for comparision. Ideally our form submission helper would know to only care about numbers while comparing profiles.

::: browser/extensions/formautofill/ProfileStorage.jsm:1328
(Diff revision 1)
> -      // Save the original number anyway if "tel" is omitted.
> +      // Save the number anyway if it contains only valid characters and the
> +      // length of its number part is between 5 and 15.
> +      // (The maxinum length of a valid number in E.164 format is 15 digits
> +      //  according to https://en.wikipedia.org/wiki/E.164 )
> +      number = number.replace(/[\s\(\)-]/g, "");
> +      if (number.match(/^(\+?)[\da-zA-Z]{5,15}$/)) {

RegExp.prototype.test is more performant if you just need a boolean of whether it matches:
`/^(\+?)[\da-zA-Z]{5,15}$/.test(number)`
Attachment #8903112 - Flags: review?(MattN+bmo)
Comment on attachment 8903112 [details]
Bug 1394139 - [Form Autofill] Don't save obviously bogus phone numbers,

https://reviewboard.mozilla.org/r/174910/#review180294

> I was thinking we would leave storage more generic and do this only when saving data from a form submission.
> 
> It kinda think we should preserve whitespace for saving but ignore formatting characters for comparision. Ideally our form submission helper would know to only care about numbers while comparing profiles.

I was also thinking of that. Will update my patch. Thanks.
Comment on attachment 8903112 [details]
Bug 1394139 - [Form Autofill] Don't save obviously bogus phone numbers,

https://reviewboard.mozilla.org/r/174910/#review181156

::: browser/extensions/formautofill/FormAutofillHandler.jsm:595
(Diff revision 2)
> +    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 is modified after autofilling.

"if none of the related fields are"
Attachment #8903112 - Flags: review?(MattN+bmo) → review+
Thanks.
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9683379b6762
[Form Autofill] Don't save obviously bogus phone numbers, r=MattN
https://hg.mozilla.org/mozilla-central/rev/9683379b6762
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8903112 [details]
Bug 1394139 - [Form Autofill] Don't save obviously bogus phone numbers,

Approval Request Comment
[Feature/Bug causing the regression]:
Feature.

[User impact if declined]:
Some invalid phone numbers might be saved in the storage.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It only affects Form Autofill system add-on.

[String changes made/needed]:
N/A
Attachment #8903112 - Flags: approval-mozilla-beta?
Comment on attachment 8903112 [details]
Bug 1394139 - [Form Autofill] Don't save obviously bogus phone numbers,

validate phone numbers in form autofill, beta56+

should be in 56.0b11
Attachment #8903112 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.