Closed
Bug 1394139
Opened 7 years ago
Closed 7 years ago
Don't save obviously bogus phone numbers
Categories
(Toolkit :: Form Manager, defect, P1)
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)
59 bytes,
text/x-review-board-request
|
MattN
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
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.
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Whiteboard: [form autofill] → [form autofill:MVP]
Updated•7 years ago
|
Priority: -- → P1
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → lchang
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
Thanks.
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9683379b6762 [Form Autofill] Don't save obviously bogus phone numbers, r=MattN
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9683379b6762
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 12•7 years ago
|
||
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 13•7 years ago
|
||
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+
Comment 14•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/7f8111c430c5
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•