Bug 1486954 - Part IV, Recover from decryption errors in some places r=MattN
authorTimothy Guan-tin Chien <timdream@gmail.com>
Wed, 17 Oct 2018 02:34:22 +0000
changeset 500036 c895888bdddc97e36675e49844e7c115e0d6cc12
parent 500035 27e9286503e8bfbb61ed2edfc374257b728fec6b
child 500037 37d35eb8428123975a589ae2068d733f35d48cd0
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1486954
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1486954 - Part IV, Recover from decryption errors in some places r=MattN In case of loss of OS key store key, we should still allow users to go into the manage credit cards dialog and fill the numbers back in. This is not the migration strategy, see Part III. Differential Revision: https://phabricator.services.mozilla.com/D5083
browser/components/payments/content/paymentDialogWrapper.js
browser/extensions/formautofill/FormAutofillStorage.jsm
browser/extensions/formautofill/content/manageDialog.js
browser/extensions/formautofill/test/unit/test_creditCardRecords.js
toolkit/modules/CreditCard.jsm
--- a/browser/components/payments/content/paymentDialogWrapper.js
+++ b/browser/components/payments/content/paymentDialogWrapper.js
@@ -144,17 +144,17 @@ var paymentDialogWrapper = {
     });
 
     return address;
   },
 
   /**
    * @param {string} guid The GUID of the basic card record from storage.
    * @param {string} cardSecurityCode The associated card security code (CVV/CCV/etc.)
-   * @throws if the user cancels entering their master password or an error decrypting
+   * @throws If there is an error decrypting
    * @returns {nsIBasicCardResponseData?} returns response data or null (if the
    *                                      master password dialog was cancelled);
    */
   async _convertProfileBasicCardToPaymentMethodData(guid, cardSecurityCode) {
     let cardData = this.temporaryStore.creditCards.get(guid) ||
                    await formAutofillStorage.creditCards.get(guid);
     if (!cardData) {
       throw new Error(`Basic card not found in storage: ${guid}`);
@@ -498,18 +498,26 @@ var paymentDialogWrapper = {
     window.close();
   },
 
   async onPay({
     selectedPayerAddressGUID: payerGUID,
     selectedPaymentCardGUID: paymentCardGUID,
     selectedPaymentCardSecurityCode: cardSecurityCode,
   }) {
-    let methodData = await this._convertProfileBasicCardToPaymentMethodData(paymentCardGUID,
-                                                                            cardSecurityCode);
+    let methodData;
+    try {
+      methodData = await this._convertProfileBasicCardToPaymentMethodData(paymentCardGUID,
+                                                                          cardSecurityCode);
+    } catch (ex) {
+      // TODO (Bug 1498403): Some kind of "credit card storage error" here, perhaps asking user
+      // to re-enter credit card # from management UI.
+      Cu.reportError(ex);
+      return;
+    }
 
     if (!methodData) {
       // TODO (Bug 1429265/Bug 1429205): Handle when a user hits cancel on the
       // Master Password dialog.
       Cu.reportError("Bug 1429265/Bug 1429205: User canceled master password entry");
       return;
     }
 
--- a/browser/extensions/formautofill/FormAutofillStorage.jsm
+++ b/browser/extensions/formautofill/FormAutofillStorage.jsm
@@ -1696,17 +1696,26 @@ class CreditCards extends AutofillRecord
           throw new Error("Unknown credit card version to migrate: " + creditCard.version);
       }
     }
     return super._computeMigratedRecord(creditCard);
   }
 
   async _stripComputedFields(creditCard) {
     if (creditCard["cc-number-encrypted"]) {
-      creditCard["cc-number"] = await OSKeyStore.decrypt(creditCard["cc-number-encrypted"]);
+      try {
+        creditCard["cc-number"] = await OSKeyStore.decrypt(creditCard["cc-number-encrypted"]);
+      } catch (ex) {
+        if (ex.result == Cr.NS_ERROR_ABORT) {
+          throw ex;
+        }
+        // Quietly recover from encryption error,
+        // so existing credit card entry with undecryptable number
+        // can be updated.
+      }
     }
     await super._stripComputedFields(creditCard);
   }
 
   _normalizeFields(creditCard) {
     this._normalizeCCName(creditCard);
     this._normalizeCCNumber(creditCard);
     this._normalizeCCExpirationDate(creditCard);
--- a/browser/extensions/formautofill/content/manageDialog.js
+++ b/browser/extensions/formautofill/content/manageDialog.js
@@ -321,17 +321,30 @@ class ManageCreditCards extends ManageRe
    *
    * @param  {object} creditCard [optional]
    */
   async openEditDialog(creditCard) {
     // Ask for reauth if user is trying to edit an existing credit card.
     if (!creditCard || await OSKeyStore.ensureLoggedIn(true)) {
       let decryptedCCNumObj = {};
       if (creditCard) {
-        decryptedCCNumObj["cc-number"] = await OSKeyStore.decrypt(creditCard["cc-number-encrypted"]);
+        try {
+          decryptedCCNumObj["cc-number"] = await OSKeyStore.decrypt(creditCard["cc-number-encrypted"]);
+        } catch (ex) {
+          if (ex.result == Cr.NS_ERROR_ABORT) {
+            // User shouldn't be ask to reauth here, but it could happen.
+            // Return here and skip opening the dialog.
+            return;
+          }
+          // We've got ourselves a real error.
+          // Recover from encryption error so the user gets a chance to re-enter
+          // unencrypted credit card number.
+          decryptedCCNumObj["cc-number"] = "";
+          Cu.reportError(ex);
+        }
       }
       let decryptedCreditCard = Object.assign({}, creditCard, decryptedCCNumObj);
       this.prefWin.gSubDialog.open(EDIT_CREDIT_CARD_URL, "resizable=no", {
         record: decryptedCreditCard,
       });
     }
   }
 
--- a/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_creditCardRecords.js
@@ -417,16 +417,24 @@ add_task(async function test_update() {
   creditCard = profileStorage.creditCards._data[0];
   Assert.equal(creditCard["cc-number"],
     CreditCard.getLongMaskedNumber(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD["cc-number"]));
   await profileStorage.creditCards.update(profileStorage.creditCards._data[1].guid, TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD, true);
   creditCard = profileStorage.creditCards._data[1];
   Assert.equal(creditCard["cc-number"],
     CreditCard.getLongMaskedNumber(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD["cc-number"]));
 
+  // Decryption failure of existing record should not prevent it from being updated.
+  creditCard = profileStorage.creditCards._data[0];
+  creditCard["cc-number-encrypted"] = "INVALID";
+  await profileStorage.creditCards.update(profileStorage.creditCards._data[0].guid, TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD, false);
+  creditCard = profileStorage.creditCards._data[0];
+  Assert.equal(creditCard["cc-number"],
+    CreditCard.getLongMaskedNumber(TEST_CREDIT_CARD_WITH_EMPTY_COMPUTED_FIELD["cc-number"]));
+
   await Assert.rejects(profileStorage.creditCards.update("INVALID_GUID", TEST_CREDIT_CARD_3),
     /No matching record\./
   );
 
   await Assert.rejects(profileStorage.creditCards.update(guid, TEST_CREDIT_CARD_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./
   );
 
--- a/toolkit/modules/CreditCard.jsm
+++ b/toolkit/modules/CreditCard.jsm
@@ -203,17 +203,23 @@ class CreditCard {
    * true, decrypted credit card numbers are shown instead.
    */
   async getLabel({showNumbers} = {}) {
     let parts = [];
     let label;
 
     if (showNumbers) {
       if (this._encryptedNumber) {
-        label = await OSKeyStore.decrypt(this._encryptedNumber);
+        try {
+          label = await OSKeyStore.decrypt(this._encryptedNumber);
+        } catch (ex) {
+          // Quietly recover from decryption error.
+          label = this._number;
+          Cu.reportError(ex);
+        }
       } else {
         label = this._number;
       }
     }
     if (this._unmodifiedNumber && !label) {
       if (this.isValidNumber()) {
         label = this.maskedNumber;
       } else {