Bug 629463: delete bad server-side clients records. r=philiKON a=beltzner

services/sync/modules/engines.js

@@ -717,7 +717,7 @@ SyncEngine.prototype = {
         try {
           item.decrypt();
         } catch (ex if (Utils.isHMACMismatch(ex) &&
-                        this.handleHMACMismatch())) {
+                        this.handleHMACMismatch(item))) {
           // Let's try handling it.
           // If the callback returns true, try decrypting again, because
           // we've got new keys.
@@ -1083,7 +1083,7 @@ SyncEngine.prototype = {
     this._resetClient();
   },
   
-  handleHMACMismatch: function handleHMACMismatch() {
+  handleHMACMismatch: function handleHMACMismatch(item) {
     return Weave.Service.handleHMACEvent();
   }
 };

services/sync/modules/engines/clients.js

@@ -192,6 +192,20 @@ ClientEngine.prototype = {
   _wipeClient: function _wipeClient() {
     SyncEngine.prototype._resetClient.call(this);
     this._store.wipe();
+  },
+  
+  // Override the default behavior to delete bad records from the server.
+  handleHMACMismatch: function handleHMACMismatch(item) {
+    this._log.debug("Handling HMAC mismatch for " + item.id);
+    if (SyncEngine.prototype.handleHMACMismatch.call(this, item))
+      return true;
+
+    // It's a bad client record. Save it to be deleted at the end of the sync.
+    this._log.debug("Bad client record detected. Scheduling for deletion.");
+    this._deleteId(item.id);
+
+    // Don't try again.
+    return false;
   }
 };
 

services/sync/tests/unit/head_http_server.js

@@ -139,6 +139,17 @@ ServerCollection.prototype = {
            && (!options.newer || (wbo.modified > options.newer));
   },
 
+  count: function(options) {
+    options = options || {};
+    let c = 0;
+    for (let [id, wbo] in Iterator(this.wbos)) {
+      if (wbo.modified && this._inResultSet(wbo, options)) {
+        c++;
+      }
+    }
+    return c;
+  },
+
   get: function(options) {
     let result;
     if (options.full) {

services/sync/tests/unit/test_clients_engine.js

@@ -3,10 +3,86 @@ Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/identity.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/engines/clients.js");
+Cu.import("resource://services-sync/service.js");
 
 const MORE_THAN_CLIENTS_TTL_REFRESH = 691200; // 8 days
 const LESS_THAN_CLIENTS_TTL_REFRESH = 86400; // 1 day
 
+function test_bad_hmac() {
+  _("Ensure that Clients engine deletes corrupt records.");
+  let global = new ServerWBO('global',
+                             {engines: {clients: {version: Clients.version,
+                                                  syncID: Clients.syncID}}});
+  let clientsColl = new ServerCollection({}, true);
+  let keysWBO = new ServerWBO("keys");
+
+  let collectionsHelper = track_collections_helper();
+  let upd = collectionsHelper.with_updated_collection;
+  let collections = collectionsHelper.collections;
+
+  // Watch for deletions in the given collection.
+  let deleted = false;
+  function trackDeletedHandler(coll, handler) {
+    let u = upd(coll, handler);
+    return function(request, response) {
+      if (request.method == "DELETE")
+        deleted = true;
+
+      return u(request, response);
+    };
+  }
+
+  let handlers = {
+    "/1.0/foo/info/collections": collectionsHelper.handler,
+    "/1.0/foo/storage/meta/global": upd("meta", global.handler()),
+    "/1.0/foo/storage/crypto/keys": upd("crypto", keysWBO.handler()),
+    "/1.0/foo/storage/clients": trackDeletedHandler("crypto", clientsColl.handler())
+  };
+
+  let server = httpd_setup(handlers);
+  do_test_pending();
+
+  try {
+    let passphrase = "abcdeabcdeabcdeabcdeabcdea";
+    Service.serverURL = "http://localhost:8080/";
+    Service.clusterURL = "http://localhost:8080/";
+    Service.login("foo", "ilovejane", passphrase);
+
+    CollectionKeys.generateNewKeys();
+
+    _("First sync, client record is uploaded");
+    do_check_eq(0, clientsColl.count());
+    do_check_eq(Clients.lastRecordUpload, 0);
+    Clients.sync();
+    do_check_eq(1, clientsColl.count());
+    do_check_true(Clients.lastRecordUpload > 0);
+    deleted = false;    // Initial setup can wipe the server, so clean up.
+
+    _("Records now: " + clientsColl.get({}));
+    _("Change our keys and our client ID, reupload keys.");
+    Clients.localID = Utils.makeGUID();
+    Clients.resetClient();
+    CollectionKeys.generateNewKeys();
+    let serverKeys = CollectionKeys.asWBO("crypto", "keys");
+    serverKeys.encrypt(Weave.Service.syncKeyBundle);
+    do_check_true(serverKeys.upload(Weave.Service.cryptoKeysURL).success);
+
+    _("Sync.");
+    do_check_true(!deleted);
+    Clients.sync();
+
+    _("Old record was deleted, new one uploaded.");
+    do_check_true(deleted);
+    do_check_eq(1, clientsColl.count());
+    _("Records now: " + clientsColl.get({}));
+
+  } finally {
+    server.stop(do_test_finished);
+    Svc.Prefs.resetBranch("");
+    Records.clearCache();
+  }
+}
+
 function test_properties() {
   try {
     _("Test lastRecordUpload property");
@@ -74,6 +150,9 @@ function test_sync() {
 
 
 function run_test() {
+  initTestLogging("Trace");
+  Log4Moz.repository.getLogger("Engine.Clients").level = Log4Moz.Level.Trace;
+  test_bad_hmac();      // Needs to run first: doesn't use fake service!
   test_properties();
   test_sync();
 }