Bug 1395122 - [Form Autofill] Part 3: Ensure the credit card number will be encrypted/decrypted upon being accessed by sync. r=markh,kitcambridge,seanlee draft
authorLuke Chang <lchang@mozilla.com>
Tue, 05 Sep 2017 14:11:55 +0800
changeset 660547 51e2df8fd667b9d2e8fab44fff3e0e0b51d0412e
parent 660546 a607019239ae68f0097449d9328ff75d7f1314b0
child 660548 39ae9bcd3cd74af65e942f38e3c328ad452713fb
push id78427
push userbmo:lchang@mozilla.com
push dateThu, 07 Sep 2017 03:59:04 +0000
reviewersmarkh, kitcambridge, seanlee
bugs1395122
milestone57.0a1
Bug 1395122 - [Form Autofill] Part 3: Ensure the credit card number will be encrypted/decrypted upon being accessed by sync. r=markh,kitcambridge,seanlee MozReview-Commit-ID: JyqqrXOPOvz
browser/extensions/formautofill/FormAutofillSync.jsm
browser/extensions/formautofill/ProfileStorage.jsm
--- a/browser/extensions/formautofill/FormAutofillSync.jsm
+++ b/browser/extensions/formautofill/FormAutofillSync.jsm
@@ -142,16 +142,19 @@ FormAutofillStore.prototype = {
   },
 
   async createRecord(id, collection) {
     this._log.trace("Create record", id);
     let record = new AutofillRecord(collection, id);
     let entry = this.storage.get(id, {
       rawData: true,
     });
+    if (this.storage.decryptCCNumberFields) {
+      await this.storage.decryptCCNumberFields(entry);
+    }
     if (entry) {
       record.fromEntry(entry);
     } else {
       // We should consider getting a more authortative indication it's actually deleted.
       this._log.debug(`Failed to get autofill record with id "${id}", assuming deleted`);
       record.deleted = true;
     }
     return record;
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -503,17 +503,17 @@ class AutofillRecords {
     this.log.debug("get:", guid, rawData);
 
     let recordFound = this._findByGUID(guid);
     if (!recordFound) {
       return null;
     }
 
     // The record is cloned to avoid accidental modifications from outside.
-    let clonedRecord = this._clone(recordFound);
+    let clonedRecord = this._cloneAndCleanUp(recordFound);
     if (rawData) {
       this._stripComputedFields(clonedRecord);
     } else {
       this._recordReadProcessor(clonedRecord);
     }
     return clonedRecord;
   }
 
@@ -527,17 +527,17 @@ class AutofillRecords {
    * @returns {Array.<Object>}
    *          An array containing clones of all records.
    */
   getAll({rawData = false, includeDeleted = false} = {}) {
     this.log.debug("getAll", rawData, includeDeleted);
 
     let records = this._store.data[this._collectionName].filter(r => !r.deleted || includeDeleted);
     // Records are cloned to avoid accidental modifications from outside.
-    let clonedRecords = records.map(r => this._clone(r));
+    let clonedRecords = records.map(r => this._cloneAndCleanUp(r));
     clonedRecords.forEach(record => {
       if (rawData) {
         this._stripComputedFields(record);
       } else {
         this._recordReadProcessor(record);
       }
     });
     return clonedRecords;
@@ -591,16 +591,18 @@ class AutofillRecords {
     }
   }
 
   /**
    * Attempts a three-way merge between a changed local record, an incoming
    * remote record, and the shared parent that we synthesize from the last
    * synced fields - see _maybeStoreLastSyncedField.
    *
+   * NOTE: localRecord and remoteRecord are both decrypted in creditCard case.
+   *
    * @param   {Object} localRecord
    *          The changed local record, currently in storage.
    * @param   {Object} remoteRecord
    *          The remote record.
    * @returns {Object|null}
    *          The merged record, or `null` if there are conflicts and the
    *          records can't be merged.
    */
@@ -612,25 +614,42 @@ class AutofillRecords {
     let mergedRecord = {};
     for (let field of INTERNAL_FIELDS) {
       if (remoteRecord[field] != null) {
         mergedRecord[field] = remoteRecord[field];
       }
     }
 
     for (let field of this.VALID_FIELDS) {
+      // It's unnecessary to handle encrypted fields because the incoming
+      // records should already be decrypted.
+      if (field.endsWith("-encrypted")) {
+        continue;
+      }
+
       let isLocalSame = false;
       let isRemoteSame = false;
       if (field in sync.lastSyncedFields) {
         // If the field has changed since the last sync, compare hashes to
         // determine if the local and remote values are different. Hashing is
         // expensive, but we don't expect this to happen frequently.
         let lastSyncedValue = sync.lastSyncedFields[field];
-        isLocalSame = lastSyncedValue == sha512(localRecord[field]);
-        isRemoteSame = lastSyncedValue == sha512(remoteRecord[field]);
+        let localValue = localRecord[field];
+        let remoteValue = remoteRecord[field];
+
+        // "cc-number" is stored in a masked format and "lastSyncedValue" is
+        // calculated based on it so we should get masked numbers before
+        // comparing them.
+        if (field == "cc-number") {
+          localValue = this._getMaskedCCNumber(localValue);
+          remoteValue = this._getMaskedCCNumber(remoteValue);
+        }
+
+        isLocalSame = lastSyncedValue == sha512(localValue);
+        isRemoteSame = lastSyncedValue == sha512(remoteValue);
       } else {
         // Otherwise, if the field hasn't changed since the last sync, we know
         // it's the same locally.
         isLocalSame = true;
         isRemoteSame = localRecord[field] == remoteRecord[field];
       }
 
       let value;
@@ -671,21 +690,25 @@ class AutofillRecords {
    * @param   {boolean} [options.keepSyncMetadata = false]
    *          Should we copy Sync metadata? This is true if `remoteRecord` is a
    *          merged record with local changes that we need to upload. Passing
    *          `keepSyncMetadata` retains the record's change counter and
    *          last synced fields, so that we don't clobber the local change if
    *          the sync is interrupted after the record is merged, but before
    *          it's uploaded.
    */
-  _replaceRecordAt(index, remoteRecord, {keepSyncMetadata = false} = {}) {
+  async _replaceRecordAt(index, remoteRecord, {keepSyncMetadata = false} = {}) {
     let localRecord = this._store.data[this._collectionName][index];
     let newRecord = this._clone(remoteRecord);
 
+    if (this.encryptCCNumberFields) {
+      await this.encryptCCNumberFields(newRecord);
+    }
     this._stripComputedFields(newRecord);
+    this._normalizeFields(newRecord);
 
     this._store.data[this._collectionName][index] = newRecord;
 
     if (keepSyncMetadata) {
       // It's safe to move the Sync metadata from the old record to the new
       // record, since we always clone records when we return them, and we
       // never hand out references to the metadata object via public methods.
       newRecord._sync = localRecord._sync;
@@ -722,17 +745,17 @@ class AutofillRecords {
    * original record remains unchanged in storage.
    *
    * @param   {Object} localRecord
    *          The local record.
    * @returns {string}
    *          A clone of the local record with a new GUID.
    */
   _forkLocalRecord(localRecord) {
-    let forkedLocalRecord = this._clone(localRecord);
+    let forkedLocalRecord = this._cloneAndCleanUp(localRecord);
 
     this._stripComputedFields(forkedLocalRecord);
 
     forkedLocalRecord.guid = this._generateGUID();
     this._store.data[this._collectionName].push(forkedLocalRecord);
 
     // Give the record fresh Sync metadata and bump its change counter as a
     // side effect. This also excludes the forked record from de-duping on the
@@ -766,40 +789,44 @@ class AutofillRecords {
       throw new Error(`Can't reconcile tombstone ${remoteRecord.guid}`);
     }
 
     let localIndex = this._findIndexByGUID(remoteRecord.guid);
     if (localIndex < 0) {
       throw new Error(`Record ${remoteRecord.guid} not found`);
     }
 
-    let localRecord = this._store.data[this._collectionName][localIndex];
+    let localRecord = this._clone(this._store.data[this._collectionName][localIndex]);
     let sync = this._getSyncMetaData(localRecord, true);
 
     let forkedGUID = null;
 
     if (sync.changeCounter === 0) {
       // Local not modified. Replace local with remote.
-      this._replaceRecordAt(localIndex, remoteRecord, {
+      await this._replaceRecordAt(localIndex, remoteRecord, {
         keepSyncMetadata: false,
       });
     } else {
+      if (this.decryptCCNumberFields) {
+        await this.decryptCCNumberFields(localRecord);
+      }
+
       let mergedRecord = this._mergeSyncedRecords(localRecord, remoteRecord);
       if (mergedRecord) {
         // Local and remote modified, but we were able to merge. Replace the
         // local record with the merged record.
-        this._replaceRecordAt(localIndex, mergedRecord, {
+        await this._replaceRecordAt(localIndex, mergedRecord, {
           keepSyncMetadata: true,
         });
       } else {
         // Merge conflict. Fork the local record, then replace the original
         // with the merged record.
         let forkedLocalRecord = this._forkLocalRecord(localRecord);
         forkedGUID = forkedLocalRecord.guid;
-        this._replaceRecordAt(localIndex, remoteRecord, {
+        await this._replaceRecordAt(localIndex, remoteRecord, {
           keepSyncMetadata: false,
         });
       }
     }
 
     this._store.saveSoon();
     Services.obs.notifyObservers({wrappedJSObject: {
       sourceSync: true,
@@ -1010,16 +1037,22 @@ class AutofillRecords {
       if (profile.guid == record.guid) {
         throw new Error(`Record ${record.guid} already exists`);
       }
       if (this._getSyncMetaData(profile)) {
         // This record has already been uploaded, so it can't be a dupe of
         // another incoming item.
         continue;
       }
+
+      profile = this._clone(profile);
+      if (this.decryptCCNumberFields) {
+        await this.decryptCCNumberFields(profile);
+      }
+
       let keys = new Set(Object.keys(record));
       for (let key of Object.keys(profile)) {
         keys.add(key);
       }
       // Ignore internal and computed fields when matching records. Internal
       // fields are synced, but almost certainly have different values than the
       // local record, and we'll update them in `reconcile`. Computed fields
       // aren't synced at all.
@@ -1052,16 +1085,20 @@ class AutofillRecords {
     return null;
   }
 
   /**
    * Internal helper functions.
    */
 
   _clone(record) {
+    return Object.assign({}, record);
+  }
+
+  _cloneAndCleanUp(record) {
     let result = {};
     for (let key in record) {
       // Do not expose hidden fields and fields with empty value (mainly used
       // as placeholders of the computed fields).
       if (!key.startsWith("_") && record[key] !== "") {
         result[key] = record[key];
       }
     }