Closed
Bug 1038215
Opened 10 years ago
Closed 10 years ago
Session Restore storing CC info, circumventing autofill restrictions.
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 34
People
(Reporter: mhoye, Assigned: tyler.colgan, Mentored)
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 2 obsolete files)
5.81 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
Despite bug 188285 I can still see credit card numbers getting autofilled in Firefox. Here's a snippet of a sessionstore-backups/recovery.bak file where it's all coming from, but from a user's perspective it appears as regular autofill pulldown. formdata":{"xpath":{ "//xhtml:div[@id='main']/xhtml:form[@name='ActionForm']/xhtml:table/xhtml:tbody/xhtml:tr[3]/xhtml:td[2]/xhtml:select[@name='paymentMethodCategory']":{"selectedIndex":1,"value":"CC"}, "//xhtml:div[@id='divPaymentMethodDetails']/xhtml:table/xhtml:tbody/xhtml:tr/xhtml:td[2]/xhtml:select[@name='paymentMethod.type']":{"selectedIndex":0, "value":""}, "//xhtml:div[@id='divPaymentMethodDetails']/xhtml:table/xhtml:tbody/xhtml:tr[2]/xhtml:td[2]/xhtml:input[@name='paymentMethod.accountNumber']":"################", "//xhtml:div[@id='divPaymentMethodDetails']/xhtml:table/xhtml:tbody/xhtml:tr[4]/xhtml:td[2]/xhtml:select[@name='paymentMethod.creditCardExpiryDat e.month']":{"selectedIndex":0,"value":""}, "//xhtml:div[@id='divPaymentMethodDetails']/xhtml:table/xhtml:tbody/xhtml:tr[4]/xhtml:td[2]/xhtml:select[@name='paymentMethod.creditCardExpiryDate.year']":{"selectedIndex":0, [...]
Comment 1•10 years ago
|
||
Implementing the Luhn checksum algorithm from bug 188285 and applying that only to <input> fields shouldn't be too hard. FormData.collect() in FormData.jsm would need to do that in order to prevent us from saving CC numbers.
Mentor: ttaubert
Whiteboard: [good-first-bug][lang=js]
Assignee | ||
Comment 2•10 years ago
|
||
What would be the best way to detect <input> fields in FormData.collect() and exclude them from being collected if they contain CC numbers? Something like: while (node = formNodes.iterateNext()) { let hasDefaultValue = true; let value; ... // We do not want to collect credit card numbers. See bug 1038215. if (node instanceof Ci.nsIDOMHTMLInputElement && IsValidCCNumber(node.value)) { continue; } ... } (This is assuming that a working version of IsValidCCNumber() would be established.)
Comment 3•10 years ago
|
||
Yeah, that looks great to me. That's exactly how we should do it.
Assignee | ||
Comment 4•10 years ago
|
||
Here's a patch that adds this check along with an implementation of the Luhn algorithm. Would it be a good idea to add some automated test cases for this as well? (cf. test cases added in this diff: https://bugzilla.mozilla.org/attachment.cgi?id=433049&action=diff) If so, could you point me to some information about how to run the pre-existing tests for this area and how to add new test cases?
Attachment #8463135 -
Flags: feedback?(ttaubert)
Comment 5•10 years ago
|
||
Comment on attachment 8463135 [details] [diff] [review] Change FormData.collect() to avoid storing CC data (version 1) Review of attachment 8463135 [details] [diff] [review]: ----------------------------------------------------------------- Tyler, thank you, this looks great! You're right, this should have tests. You can take a look at browser_formdata.js [1] and add another task/test at the bottom there. I hope this is straight forward looking at the existing code but if there are any questions feel free to ask here or on IRC! Thanks! [1] https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_formdata.js ::: toolkit/modules/sessionstore/FormData.jsm @@ +42,5 @@ > > /** > + * Returns whether the given value is a valid credit card number based on > + * the Luhn algorithm. See https://en.wikipedia.org/wiki/Luhn_algorithm and > + * bug 1038215. Nit: I don't think we need a reference to the bug number here, it's all in Git/Hg for us :) @@ +68,5 @@ > + if (i % 2) { > + // Double every other value. > + total += currentDigit * 2; > + // If the doubled value will be >10, add the digits together. > + if(currentDigit > 4) { Tiny nit: if () { @@ +149,5 @@ > if (!node.id && generatedCount > MAX_TRAVERSED_XPATHS) { > continue; > } > > + // We do not want to collect credit card numbers. See bug 1038215. Nit: I don't think we need a reference to the bug number here either.
Attachment #8463135 -
Flags: feedback?(ttaubert) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
Thanks for the info, Tim. Here's another iteration of the patch, which includes your recommendations for FormData.collect(). This version also adds a test to check that valid CC numbers are not being collected, and that non-valid CC numbers are still being collected. The example credit card numbers used in the test are taken verbatim from the test added in the fix for bug 188285.
Attachment #8463135 -
Attachment is obsolete: true
Attachment #8463513 -
Flags: feedback?(ttaubert)
Comment 7•10 years ago
|
||
Comment on attachment 8463513 [details] [diff] [review] Change FormData.collect() to avoid storing CC data (version 2) Review of attachment 8463513 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Tyler, this looks great. I'd like to fix a few nits but otherwise I will very likely r+ your next patch :) Before we land it we should then push it to try (I can do that for you) to check that it doesn't break any of our tests on any of our platforms. ::: browser/components/sessionstore/test/browser_formdata.js @@ +268,5 @@ > + "3350852696538147", "5011802870046957" > + ]; > + > + const URL = "http://mochi.test:8888/browser/browser/components/" + > + "sessionstore/test/browser_formdata_sample.html"; Nit: Please indent by two more spaces. @@ +272,5 @@ > + "sessionstore/test/browser_formdata_sample.html"; > + > + // Creates a tab, loads a page with a form field, sets the value of the > + // field, and then removes the tab to trigger data collection. > + function createAndRemoveTab(formValue) { function* createAndRemoveTab(formValue) { @@ +286,5 @@ > + gBrowser.removeTab(tab); > + } > + > + // Test that valid CC numbers are not collected. > + for (let i = 0; i < validCCNumbers.length; i++) { for (let number of validCCNumbers) { @@ +289,5 @@ > + // Test that valid CC numbers are not collected. > + for (let i = 0; i < validCCNumbers.length; i++) { > + yield createAndRemoveTab(validCCNumbers[i]); > + let [{state: {formdata}}] = JSON.parse(ss.getClosedTabData(window)); > + ok(typeof formdata === 'undefined', "valid CC numbers are not collected"); Does this yell in strict mode? Should we maybe check |"formdata" in state|? @@ +293,5 @@ > + ok(typeof formdata === 'undefined', "valid CC numbers are not collected"); > + } > + > + // Test that non-CC numbers are still collected. > + for (let i = 0; i < invalidCCNumbers.length; i++) { for (let number of invalidCCNumbers) { @@ +297,5 @@ > + for (let i = 0; i < invalidCCNumbers.length; i++) { > + yield createAndRemoveTab(invalidCCNumbers[i]); > + let [{state: {formdata}}] = JSON.parse(ss.getClosedTabData(window)); > + is(formdata.id.txt, invalidCCNumbers[i], > + "numbers that are not valid CC numbers are still collected"); Tiny nit: please indent by one more space. ::: toolkit/modules/sessionstore/FormData.jsm @@ +66,5 @@ > + > + if (i % 2) { > + // Double every other value. > + total += currentDigit * 2; > + // If the doubled value will be >10, add the digits together. Nit: "will be >=10" or "will be >9"
Attachment #8463513 -
Flags: feedback?(ttaubert) → feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Thanks, here's another iteration. I actually didn't get any complaints in strict mode about using formdata that way, but I think checking |"formdata" in state| is probably better, so I updated it to use that.
Attachment #8463513 -
Attachment is obsolete: true
Attachment #8464087 -
Flags: review?(ttaubert)
Comment 9•10 years ago
|
||
Comment on attachment 8464087 [details] [diff] [review] Change FormData.collect() to avoid storing CC data (version 3) Review of attachment 8464087 [details] [diff] [review]: ----------------------------------------------------------------- That looks great, thank you! I'll push it to try in a second.
Attachment #8464087 -
Flags: review?(ttaubert) → review+
Comment 10•10 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2801f6661759
Assignee: nobody → tyler.colgan
Status: NEW → ASSIGNED
Comment 11•10 years ago
|
||
Pushed the patch again including a request for a longer test timeout. It failed (i.e. timed out) on debug builds that run a lot slower than our opt builds. https://tbpl.mozilla.org/?tree=Try&rev=4ce4e84dfdda
Comment 12•10 years ago
|
||
Landed. Thanks Tyler! https://hg.mozilla.org/integration/fx-team/rev/d751028876fa
Reporter | ||
Comment 13•10 years ago
|
||
Thank you, Tyler! Tim, do we have something else Tyler could sink his teeth into, now that this is landed?
Comment 14•10 years ago
|
||
Yes, I thought about that as well! Tyler, if you want to help us fix a few more sessionstore bugs these are good candidates: bug 1030765, bug 1031298, bug 1036341, and bug 952224. If you want to take a look at different parts of the browser then that's fine, we have mentored bugs for almost everything and we can certainly help you find something :)
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d751028876fa
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•10 years ago
|
Whiteboard: [good-first-bug][lang=js] → [good first bug][lang=js]
You need to log in
before you can comment on or make changes to this bug.
Description
•