Bug 1412083 - Avoid syncing preferences that refer to moz-extension or blob urls. r=eoger

MozReview-Commit-ID: 9Ipq5z1dykr

--HG--
extra : rebase_source : a8616485879268ce31ef74f4b3fdbc3f4dc5a585

services/sync/modules/engines/prefs.js

@@ -72,6 +72,18 @@ PrefsEngine.prototype = {
   }
 };
 
+// We don't use services.sync.engine.tabs.filteredUrls since it includes
+// about: pages and the like, which we want to be syncable in preferences.
+// Blob and moz-extension uris are never safe to sync, so we limit our check
+// to those.
+const UNSYNCABLE_URL_REGEXP = /^(moz-extension|blob):/i;
+function isUnsyncableURLPref(prefName) {
+  if (Services.prefs.getPrefType(prefName) != Ci.nsIPrefBranch.PREF_STRING) {
+    return false;
+  }
+  const prefValue = Services.prefs.getStringPref(prefName, "");
+  return UNSYNCABLE_URL_REGEXP.test(prefValue);
+}
 
 function PrefStore(name, engine) {
   Store.call(this, name, engine);
@@ -92,7 +104,8 @@ PrefStore.prototype = {
 
   _getSyncPrefs() {
     let syncPrefs = Services.prefs.getBranch(PREF_SYNC_PREFS_PREFIX)
-                                  .getChildList("", {});
+                                  .getChildList("", {})
+                                  .filter(pref => !isUnsyncableURLPref(pref));
     // Also sync preferences that determine which prefs get synced.
     let controlPrefs = syncPrefs.map(pref => PREF_SYNC_PREFS_PREFIX + pref);
     return controlPrefs.concat(syncPrefs);
@@ -106,7 +119,10 @@ PrefStore.prototype = {
   _getAllPrefs() {
     let values = {};
     for (let pref of this._getSyncPrefs()) {
-      if (this._isSynced(pref)) {
+      // Note: _isSynced doesn't call isUnsyncableURLPref since it would cause
+      // us not to apply (syncable) changes to preferences that are set locally
+      // which have unsyncable urls.
+      if (this._isSynced(pref) && !isUnsyncableURLPref(pref)) {
         // Missing and default prefs get the null value.
         values[pref] = this._prefs.isSet(pref) ? this._prefs.get(pref, null) : null;
       }
@@ -136,6 +152,10 @@ PrefStore.prototype = {
       }
 
       let value = values[pref];
+      if (typeof value == "string" && UNSYNCABLE_URL_REGEXP.test(value)) {
+        this._log.trace(`Skipping incoming unsyncable url for pref: ${pref}`);
+        continue;
+      }
 
       switch (pref) {
         // Some special prefs we don't want to set directly.

services/sync/services-sync.js

@@ -30,7 +30,7 @@ pref("services.sync.engine.history", true);
 pref("services.sync.engine.passwords", true);
 pref("services.sync.engine.prefs", true);
 pref("services.sync.engine.tabs", true);
-pref("services.sync.engine.tabs.filteredUrls", "^(about:.*|resource:.*|chrome:.*|wyciwyg:.*|file:.*|blob:.*)$");
+pref("services.sync.engine.tabs.filteredUrls", "^(about:.*|resource:.*|chrome:.*|wyciwyg:.*|file:.*|blob:.*|moz-extension:.*)$");
 
 // The addresses and CC engines might not actually be available at all.
 pref("services.sync.engine.addresses.available", false);

services/sync/tests/unit/prefs_test_prefs_store.js

@@ -12,6 +12,10 @@ pref("services.sync.prefs.sync.testing.dont.change", true);
 user_pref("services.sync.prefs.sync.testing.turned.off", false);
 pref("services.sync.prefs.sync.testing.nonexistent", true);
 pref("services.sync.prefs.sync.testing.default", true);
+pref("services.sync.prefs.sync.testing.synced.url", true);
+// We shouldn't sync the URL, or the flag that says we should sync the pref
+// (otherwise some other client might overwrite our local value).
+user_pref("services.sync.prefs.sync.testing.unsynced.url", true);
 
 // The preference values - these are all user_prefs, otherwise their value
 // will not be synced.
@@ -21,7 +25,12 @@ user_pref("testing.bool", true);
 user_pref("testing.dont.change", "Please don't change me.");
 user_pref("testing.turned.off", "I won't get synced.");
 user_pref("testing.not.turned.on", "I won't get synced either!");
+// Some url we don't want to sync
+user_pref("testing.unsynced.url", "moz-extension://d5d31b00-b944-4afb-bd3d-d0326551a0ae");
+user_pref("testing.synced.url", "https://www.example.com");
 
 // A pref that exists but still has the default value - will be synced with
 // null as the value.
 pref("testing.default", "I'm the default value");
+
+// A pref that shouldn't be synced

services/sync/tests/unit/test_prefs_store.js

@@ -57,16 +57,24 @@ add_task(async function run_test() {
     Assert.equal(record.value["testing.default"], null);
     Assert.equal(false, "testing.turned.off" in record.value);
     Assert.equal(false, "testing.not.turned.on" in record.value);
+    // Prefs we consider unsyncable (since they are URLs that won't be stable on
+    // another firefox) shouldn't be included
+    Assert.equal(record.value["testing.unsynced.url"], null);
+    // Other URLs with user prefs should be synced, though.
+    Assert.equal(record.value["testing.synced.url"], "https://www.example.com");
 
     _("Prefs record contains non-default pref sync prefs too.");
     Assert.equal(record.value["services.sync.prefs.sync.testing.int"], null);
     Assert.equal(record.value["services.sync.prefs.sync.testing.string"], null);
     Assert.equal(record.value["services.sync.prefs.sync.testing.bool"], null);
     Assert.equal(record.value["services.sync.prefs.sync.testing.dont.change"], null);
+    Assert.equal(record.value["services.sync.prefs.sync.testing.synced.url"], null);
     // but this one is a user_pref so *will* be synced.
     Assert.equal(record.value["services.sync.prefs.sync.testing.turned.off"], false);
     Assert.equal(record.value["services.sync.prefs.sync.testing.nonexistent"], null);
     Assert.equal(record.value["services.sync.prefs.sync.testing.default"], null);
+    // This is a user pref, but shouldn't be synced since it refers to an invalid url
+    Assert.equal(record.value["services.sync.prefs.sync.testing.unsynced.url"], null);
 
     _("Update some prefs, including one that's to be reset/deleted.");
     Svc.Prefs.set("testing.deleteme", "I'm going to be deleted!");
@@ -77,7 +85,12 @@ add_task(async function run_test() {
       "testing.bool": false,
       "testing.deleteme": null,
       "testing.somepref": "im a new pref from other device",
-      "services.sync.prefs.sync.testing.somepref": true
+      "services.sync.prefs.sync.testing.somepref": true,
+      // Pretend some a stale remote client is overwriting it with a value
+      // we consider unsyncable.
+      "testing.synced.url": "blob:ebeb707a-502e-40c6-97a5-dd4bda901463",
+      // Make sure we can replace the unsynced URL with a valid URL.
+      "testing.unsynced.url": "https://www.example.com/2",
     };
     await store.update(record);
     Assert.equal(prefs.get("testing.int"), 42);
@@ -86,6 +99,8 @@ add_task(async function run_test() {
     Assert.equal(prefs.get("testing.deleteme"), undefined);
     Assert.equal(prefs.get("testing.dont.change"), "Please don't change me.");
     Assert.equal(prefs.get("testing.somepref"), "im a new pref from other device");
+    Assert.equal(prefs.get("testing.synced.url"), "https://www.example.com");
+    Assert.equal(prefs.get("testing.unsynced.url"), "https://www.example.com/2");
     Assert.equal(Svc.Prefs.get("prefs.sync.testing.somepref"), true);
 
     _("Enable persona");