1461477 - Duplicate implementations of the Luhn algorithm exist in the tree

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

[1] https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/toolkit/components/satchel/formSubmitListener.js#36

[2] https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/toolkit/modules/sessionstore/FormData.jsm#42

Both implementations are fine, they both cover all bases and don't miss any checks. The FormData.jsm one makes an unnecessary call to charAt() since the string has already passed a regex that checks for only digit characters.

One of the two should be removed, I'm indifferent as to which one to keep. The implementation that remains should be moved to a central location though since it will need to be referenced by Password Manager (satchel) and Session Store.

Comment 1

[Matthew N. [:MattN]]

Credit card autofill should also be using this to avoid storing non-CC numbers (there are some bugs filed).

The other day I was thinking it would be useful to have a shared credit card JSM to handle this algorithm (including an API… is this a CC number: [1]) along with things like masking numbers[2], checking expiration dates aren't in the past, normalizing the number[3], displaying CC to users[4], etc.

[1] https://dxr.mozilla.org/mozilla-central/rev/45ec8fd380dd2c308e79dbb396ca87f2ce9b3f9c/browser/extensions/formautofill/FormAutofillUtils.jsm#241-243
[2] https://dxr.mozilla.org/mozilla-central/rev/45ec8fd380dd2c308e79dbb396ca87f2ce9b3f9c/browser/extensions/formautofill/FormAutofillUtils.jsm#382-387
[3] https://dxr.mozilla.org/mozilla-central/rev/45ec8fd380dd2c308e79dbb396ca87f2ce9b3f9c/browser/extensions/formautofill/FormAutofillUtils.jsm#232-239
[4] https://dxr.mozilla.org/mozilla-central/rev/45ec8fd380dd2c308e79dbb396ca87f2ce9b3f9c/browser/extensions/formautofill/FormAutofillUtils.jsm#266-300

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

Review commit: https://reviewboard.mozilla.org/r/246586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246586/

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

https://reviewboard.mozilla.org/r/246586/#review252712

::: browser/extensions/formautofill/FormAutofillStorage.jsm:1567
(Diff revision 1)
>      delete creditCard["cc-family-name"];
>    }
>  
>    _normalizeCCNumber(creditCard) {
>      if (creditCard["cc-number"]) {
> -      creditCard["cc-number"] = FormAutofillUtils.normalizeCCNumber(creditCard["cc-number"]);
> +      let card = new CreditCard({number: creditCard["cc-number"]});

It's a bit confusing to have a `creditCard` variable and a `card` variable but I didn't want the scope of this bug to start growing unbounded.

Maybe a better name here would remove some of the confusion but I can't think of one. We could do something like `validatedCreditCard` or `creditCardObj`? Or rename the other variable to `creditCardRecord`?

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

https://reviewboard.mozilla.org/r/246586/#review252758

Just two comments for now… I'll try to do a second pass tomorrow.

::: toolkit/components/utils/moz.build:17
(Diff revision 1)
>      'utils.manifest',
>  ]
>  
>  EXTRA_JS_MODULES['components-utils'] = [
>      'ClientEnvironment.jsm',
> +    'CreditCard.jsm',

Hmm… never heard of this directory… I don't understand the difference from toolkit/modules/ tbh.

Can't say I'm a fan of resource://gre/modules/components-utils/

::: toolkit/components/utils/test/browser/browser_CreditCard.js:1
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +ChromeUtils.import("resource://gre/modules/components-utils/CreditCard.jsm", this);

This really should be an xpcshell test (at least for anything not touching master password).

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246586/diff/1-2/

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246586/diff/2-3/

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

https://reviewboard.mozilla.org/r/246584/#review253818

This looks great. Just a couple of questions/issues that I see. The main one I'm not able to easily answer is if the call to MasterPassword.decrypt in getLabel represents a potentially breaking change? I've not had time to really trace all that through so it could be a non-issue, it just jumped out at me.

::: browser/extensions/formautofill/FormAutofillStorage.jsm:1499
(Diff revision 3)
>  class CreditCards extends AutofillRecords {
>    constructor(store) {
>      super(store, "creditCards", VALID_CREDIT_CARD_FIELDS, VALID_CREDIT_CARD_COMPUTED_FIELDS, CREDIT_CARD_SCHEMA_VERSION);
>    }
>  
> -  _getMaskedCCNumber(ccNumber) {
> +  _getMaskedCCNumber(number) {

Do we still need this helper here? It doesnt use any state, there are only a few uses and ISTM they could all be changed to use a static CreditCard.jsm method

::: browser/extensions/formautofill/content/manageDialog.js:347
(Diff revision 3)
> -      option.text = await this.getLabel(option.record, isDecrypted);
> +      let creditCard = new CreditCard({
> +        encryptedNumber: option.record["cc-number-encrypted"],
> +        number: option.record["cc-number"],
> +        name: option.record["cc-name"],
> +      });
> +      option.text = await creditCard.getLabel(isDecrypted);

The new signature for getLabel is `getLabel({showNumbers} = {})`, so I assume this should be `await creditCard.getLabel({showNumbers: isDecrypted})`

::: toolkit/modules/CreditCard.jsm:89
(Diff revision 3)
> +
> +    // Remove dashes and whitespace
> +    let number = this._number.replace(/[\-\s]/g, "");
> +
> +    let len = number.length;
> +    if (len != 9 && len != 15 && len != 16) {

Do we have a bug filed to support the new 19 digit CC numbers. I'm reading that JCB, Discover and Maestro could have 16-19 digits, and Visa could be any one of 13, 16, 19. That is at https://www.freeformatter.com/credit-card-number-generator-validator.html I'm not sure how authoritative that is.

::: toolkit/modules/CreditCard.jsm:116
(Diff revision 3)
> +
> +  isValid() {
> +    let currentDate = new Date();
> +    let currentMonth = currentDate.getMonth() + 1;
> +    let currentYear = currentDate.getFullYear();
> +    if (!this.isValidNumber()) {

Nit: why not move this above the date/month/year stuff return early if not a valid number?

::: toolkit/modules/CreditCard.jsm:145
(Diff revision 3)
> +  async getLabel({showNumbers} = {}) {
> +    let parts = [];
> +    let label;
> +
> +    if (showNumbers) {
> +      if (this._encryptedNumber) {

Doesnt this change when the decryption happens in some cases? The `_getRecords` method of FormAutofillParent does decryption of each, leaving the result in cc-number-decrypted. Is it possible we could end up decrypting twice with this change?

::: toolkit/modules/sessionstore/FormData.jsm:167
(Diff revision 3)
>          continue;
>        }
>  
>        // We do not want to collect credit card numbers.
> -      if (ChromeUtils.getClassName(node) === "HTMLInputElement" &&
> -          isValidCCNumber(node.value)) {
> +      if (ChromeUtils.getClassName(node) === "HTMLInputElement") {
> +        let creditCard = new CreditCard({number: node.value});

This seems like a common pattern. Should we consider a CreditCard.isValidNumber() static method?

::: toolkit/modules/tests/xpcshell/test_CreditCard.js:45
(Diff revision 3)
> +  testValid("4026313395502336", false);
> +  testValid("6387060366272980", false);
> +  testValid("344060747836804", false);
> +  testValid("30190729470496", false);
> +  testValid("36333851788255", false);
> +  testValid("0000", false);

Nit: Could add todo() tests for 19 digit numbers like 3532596776688495393

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246586/diff/3-4/

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

https://reviewboard.mozilla.org/r/246586/#review254240

::: browser/extensions/formautofill/FormAutofillStorage.jsm
(Diff revision 4)
>  
> -  _getMaskedCCNumber(ccNumber) {
> -    if (ccNumber.length <= 4) {
> -      throw new Error(`Invalid credit card number`);
> -    }
> -    return "*".repeat(ccNumber.length - 4) + ccNumber.substr(-4);

Is this kind of masked CC number option still available with the new module? We use it for the placeholder to preview what will autofill and I think we should keep it… personally I think we should always mask like this instead of the "**** 1234" format.

::: browser/extensions/formautofill/FormAutofillStorage.jsm:1536
(Diff revision 4)
>  
>      // Encrypt credit card number
>      if (!("cc-number-encrypted" in creditCard)) {
>        if ("cc-number" in creditCard) {
>          let ccNumber = creditCard["cc-number"];
> -        creditCard["cc-number"] = this._getMaskedCCNumber(ccNumber);
> +        creditCard["cc-number"] = CreditCard.getMaskedNumber(ccNumber);

Also note that we need to keep the old masking if you don't bump the version and I really don't want to implement a version migration so we need the cc-number result in storage to be identical to before. If you change the data of a property without a version bump it will cause sync conflicts.

::: toolkit/modules/tests/browser/browser_CreditCard.js:6
(Diff revision 4)
> +ChromeUtils.import("resource://gre/modules/CreditCard.jsm", this);
> +ChromeUtils.import("resource://formautofill/MasterPassword.jsm", this);

Nit: why are you including the 2nd argument to .import in the two test files?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246586/diff/4-5/

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

https://reviewboard.mozilla.org/r/246586/#review254540

::: toolkit/modules/tests/browser/browser_CreditCard.js:24
(Diff revisions 4 - 5)
> +    // CreditCard.jsm and MasterPassword.jsm are imported into the global scope
> +    // -- the window -- above. If they're not deleted, they outlive the test and
> +    // are reported as a leak.
> +    delete window.MasterPassword;
> +    delete window.CreditCard;

Hmm… is this still needed without the `this` on import?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

https://reviewboard.mozilla.org/r/246586/#review254540

> Hmm… is this still needed without the `this` on import?

This test job shows the failure that caused me to add these lines (patch does not use `this` on import):
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=181044831
https://hg.mozilla.org/try/rev/8ce59f90703d95d96378898783a2215594b3214c#l15.10

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246586/diff/5-6/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b26de736798
Create a CreditCard.jsm to consolidate various credit card handling and validation. r=MattN

Comment 15

[Natalia Csoregi [:nataliaCs]]

Backed out for multiple CreditCards failures e.g. browser_manageCreditCardsDialog.js 

backout: https://hg.mozilla.org/integration/autoland/rev/354491fabd5fa3b82d4275c25e52be418e3baaa4

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9b26de736798720c1b30eeddb6c85941dc32579c&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=181379746&repo=autoland&lineNumber=1835

[task 2018-06-01T17:36:47.154Z] 17:36:47     INFO -  TEST-START | browser/extensions/formautofill/test/unit/test_creditCardRecords.js
[task 2018-06-01T17:36:48.230Z] 17:36:48  WARNING -  TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/unit/test_creditCardRecords.js | xpcshell return code: 0
[task 2018-06-01T17:36:48.245Z] 17:36:48     INFO -  TEST-INFO took 1077ms
[task 2018-06-01T17:36:48.245Z] 17:36:48     INFO -  >>>>>>>
[task 2018-06-01T17:36:48.246Z] 17:36:48     INFO -  PID 9061 | JavaScript strict warning: /builds/worker/workspace/build/tests/xpcshell/tests/browser/extensions/formautofill/test/unit/head.js -> resource://testing-common/sinon-2.3.2.js, line 8941: ReferenceError: reference to undefined property "iso-8859-8-i"
[task 2018-06-01T17:36:48.246Z] 17:36:48     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-06-01T17:36:48.246Z] 17:36:48     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-06-01T17:36:48.246Z] 17:36:48     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-06-01T17:36:48.246Z] 17:36:48     INFO -  running event loop
[task 2018-06-01T17:36:48.246Z] 17:36:48     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "iso-8859-8-i"" {file: "/builds/worker/workspace/build/tests/xpcshell/tests/browser/extensions/formautofill/test/unit/head.js -> resource://testing-common/sinon-2.3.2.js" line: 8941}]"
[task 2018-06-01T17:36:48.246Z] 17:36:48     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "Bootstrapped manifest not allowed to use 'resource' directive." {file: "jar:file:///builds/worker/workspace/build/application/firefox/browser/features/[e-mail]!/chrome.manifest" line: 7}]"
[task 2018-06-01T17:36:48.246Z] 17:36:48     INFO -  browser/extensions/formautofill/test/unit/test_creditCardRecords.js | Starting head_initialize
[task 2018-06-01T17:36:48.246Z] 17:36:48     INFO -  (xpcshell/head.js) | test head_initialize pending (2)
[task 2018-06-01T17:36:48.246Z] 17:36:48     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2018-06-01T17:36:48.247Z] 17:36:48     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2018-06-01T17:36:48.247Z] 17:36:48     INFO -  (xpcshell/head.js) | test head_initialize finished (2)
[task 2018-06-01T17:36:48.248Z] 17:36:48     INFO -  browser/extensions/formautofill/test/unit/test_creditCardRecords.js | Starting test_initialize
[task 2018-06-01T17:36:48.249Z] 17:36:48     INFO -  (xpcshell/head.js) | test test_initialize pending (2)
[task 2018-06-01T17:36:48.249Z] 17:36:48     INFO -  TEST-PASS | browser/extensions/formautofill/test/unit/test_creditCardRecords.js | test_initialize - [test_initialize : 52] Sanity check the temporary file doesn't exist. - true == true

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8980415 [details]
Bug 1461477 - Create a CreditCard.jsm to consolidate various credit card handling and validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246586/diff/6-7/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/223e5900fbbb
Create a CreditCard.jsm to consolidate various credit card handling and validation. r=MattN

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/223e5900fbbb