diff --git a/browser/extensions/formautofill/FormAutofillParent.jsm b/browser/extensions/formautofill/FormAutofillParent.jsm index 4b895c4bd512..a04ba6042ca8 100644 --- a/browser/extensions/formautofill/FormAutofillParent.jsm +++ b/browser/extensions/formautofill/FormAutofillParent.jsm @@ -593,9 +593,7 @@ class FormAutofillParent extends JSWindowActorParent { creditCard.record["cc-type"] = ""; } - // We'll show the credit card doorhanger if: - // - User applys autofill and changed - // - User fills form manually and the filling data is not duplicated to storage + // If `guid` is present, the form has been autofilled. if (creditCard.guid) { // Indicate that the user has used Credit Card Autofill to fill in a form. setUsedStatus(3); @@ -643,26 +641,48 @@ class FormAutofillParent extends JSWindowActorParent { timeStartedFillingMS ); } else { - // Indicate that the user neither sees the doorhanger nor uses Autofill - // but somehow has a duplicate record in the storage. Will be reset to 2 - // if the doorhanger actually shows below. - setUsedStatus(1); - // Add the probe to record credit card manual filling. Services.telemetry.scalarAdd( "formautofill.creditCards.fill_type_manual", 1 ); this._recordFormFillingTime("creditCard", "manual", timeStartedFillingMS); - } - // Early return if it's a duplicate data - let dupGuid = await gFormAutofillStorage.creditCards.getDuplicateGuid( - creditCard.record - ); - if (dupGuid) { - gFormAutofillStorage.creditCards.notifyUsed(dupGuid); - return false; + let existingGuid = await gFormAutofillStorage.creditCards.getDuplicateGuid( + creditCard.record + ); + + if (existingGuid) { + creditCard.guid = existingGuid; + + let originalCCData = await gFormAutofillStorage.creditCards.get( + creditCard.guid + ); + + gFormAutofillStorage.creditCards._normalizeRecord(creditCard.record); + + // If the credit card record is a duplicate, check if the fields match the + // record. + let recordUnchanged = true; + for (let field in creditCard.record) { + if (field == "cc-number") { + continue; + } + if (creditCard.record[field] != originalCCData[field]) { + recordUnchanged = false; + break; + } + } + + if (recordUnchanged) { + // Indicate that the user neither sees the doorhanger nor uses Autofill + // but somehow has a duplicate record in the storage. Will be reset to 2 + // if the doorhanger actually shows below. + setUsedStatus(1); + gFormAutofillStorage.creditCards.notifyUsed(creditCard.guid); + return false; + } + } } // Indicate that the user has seen the doorhanger. diff --git a/browser/extensions/formautofill/FormAutofillStorage.jsm b/browser/extensions/formautofill/FormAutofillStorage.jsm index b786d2def73f..75a3eca9aa3a 100644 --- a/browser/extensions/formautofill/FormAutofillStorage.jsm +++ b/browser/extensions/formautofill/FormAutofillStorage.jsm @@ -1996,24 +1996,21 @@ class CreditCards extends AutofillRecords { async getDuplicateGuid(targetCreditCard) { let clonedTargetCreditCard = this._clone(targetCreditCard); this._normalizeRecord(clonedTargetCreditCard); + if (!clonedTargetCreditCard["cc-number"]) { + return null; + } + for (let creditCard of this._data) { - let isDuplicate = await Promise.all( - this.VALID_FIELDS.map(async field => { - if (!clonedTargetCreditCard[field]) { - return !creditCard[field]; - } - if (field == "cc-number" && creditCard[field]) { - // Compare the masked numbers instead when decryption requires a password - // because we don't want to leak the credit card number. - return ( - CreditCard.getLongMaskedNumber(clonedTargetCreditCard[field]) == - creditCard[field] - ); - } - return clonedTargetCreditCard[field] == creditCard[field]; - }) - ).then(fieldResults => fieldResults.every(result => result)); - if (isDuplicate) { + if (creditCard.deleted) { + continue; + } + + let decrypted = await OSKeyStore.decrypt( + creditCard["cc-number-encrypted"], + false + ); + + if (decrypted == clonedTargetCreditCard["cc-number"]) { return creditCard.guid; } } diff --git a/browser/extensions/formautofill/test/browser/creditCard/browser_creditCard_doorhanger.js b/browser/extensions/formautofill/test/browser/creditCard/browser_creditCard_doorhanger.js index faf1326ea134..d2cbf7386b5a 100644 --- a/browser/extensions/formautofill/test/browser/creditCard/browser_creditCard_doorhanger.js +++ b/browser/extensions/formautofill/test/browser/creditCard/browser_creditCard_doorhanger.js @@ -302,11 +302,7 @@ add_task(async function test_submit_changed_subset_creditCard_form() { creditCards = await getCreditCards(); is(creditCards.length, 1, "Still 1 credit card in storage"); - is( - creditCards[0]["cc-name"], - TEST_CREDIT_CARD_1["cc-name"], - "name field still exists" - ); + is(creditCards[0]["cc-name"], undefined, "name field got cleared"); is( SpecialPowers.getIntPref(CREDITCARDS_USED_STATUS_PREF), 2, diff --git a/browser/extensions/formautofill/test/unit/test_creditCardRecords.js b/browser/extensions/formautofill/test/unit/test_creditCardRecords.js index b6530f4e14c3..cf8674940d27 100644 --- a/browser/extensions/formautofill/test/unit/test_creditCardRecords.js +++ b/browser/extensions/formautofill/test/unit/test_creditCardRecords.js @@ -785,15 +785,15 @@ add_task(async function test_getDuplicateGuid() { null ); - // Subset shouldn't be treated as a duplicate. + // Subset with the same number is a duplicate. let record = Object.assign({}, TEST_CREDIT_CARD_3); delete record["cc-exp-month"]; - Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), null); + Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), guid); - // Superset shouldn't be treated as a duplicate. + // Superset with the same number is a duplicate. record = Object.assign({}, TEST_CREDIT_CARD_3); record["cc-name"] = "John Doe"; - Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), null); + Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), guid); // Numbers with the same last 4 digits shouldn't be treated as a duplicate. record = Object.assign({}, TEST_CREDIT_CARD_3); @@ -802,13 +802,44 @@ add_task(async function test_getDuplicateGuid() { // 09 and 90 adjacent digits, which is still a valid credit card number. record["cc-number"] = "358999378390" + last4Digits; - // We treat numbers with the same last 4 digits as a duplicate. + // We don't treat numbers with the same last 4 digits as a duplicate. + Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), null); +}); + +add_task(async function test_getDuplicateGuidMatch() { + let profileStorage = await initProfileStorage( + TEST_STORE_FILE_NAME, + [TEST_CREDIT_CARD_2], + "creditCards" + ); + let guid = profileStorage.creditCards._data[0].guid; + + // Absolutely a duplicate. + Assert.equal( + await profileStorage.creditCards.getDuplicateGuid(TEST_CREDIT_CARD_2), + guid + ); + + // Absolutely not a duplicate. + Assert.equal( + await profileStorage.creditCards.getDuplicateGuid(TEST_CREDIT_CARD_1), + null + ); + + // Numbers with the same last 4 digits shouldn't be treated as a duplicate. + record = Object.assign({}, TEST_CREDIT_CARD_2); + + // We change month from `1` to `2` + record["cc-exp-month"] = 2; Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), guid); - // Even though the last 4 digits are the same, an invalid credit card number - // should never be treated as a duplicate. - record["cc-number"] = "************" + last4Digits; - Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), null); + // We change year from `2000` to `2001` + record["cc-exp-year"] = 2001; + Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), guid); + + // New name, same card + record["cc-name"] = "John Doe"; + Assert.equal(await profileStorage.creditCards.getDuplicateGuid(record), guid); }); add_task(async function test_creditCardFillDisabled() {