Bug 1412083 - Avoid syncing preferences that refer to moz-extension or blob urls. r?eoger draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 09 Jan 2018 17:46:24 -0500
changeset 718232 11bfd64dc3b66a002ac2e2982ae6ddf89f36f191
parent 717183 ca379fcca95b1f4a3744242ea8647004b99b3507
child 745413 d6ef440bf1f8b0243551541cd89f46e31a3419bd
push id94846
push userbmo:tchiovoloni@mozilla.com
push dateTue, 09 Jan 2018 22:46:47 +0000
reviewerseoger
bugs1412083
milestone59.0a1
Bug 1412083 - Avoid syncing preferences that refer to moz-extension or blob urls. r?eoger MozReview-Commit-ID: 9Ipq5z1dykr
services/sync/modules/engines/prefs.js
services/sync/services-sync.js
services/sync/tests/unit/prefs_test_prefs_store.js
services/sync/tests/unit/test_prefs_store.js
--- a/services/sync/modules/engines/prefs.js
+++ b/services/sync/modules/engines/prefs.js
@@ -67,16 +67,28 @@ PrefsEngine.prototype = {
     if (this.justWiped) {
       this.justWiped = false;
       return true;
     }
     return SyncEngine.prototype._reconcile.call(this, item);
   }
 };
 
+// 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);
   Svc.Obs.add("profile-before-change", function() {
     this.__prefs = null;
   }, this);
 }
 PrefStore.prototype = {
@@ -87,31 +99,35 @@ PrefStore.prototype = {
     if (!this.__prefs) {
       this.__prefs = new Preferences();
     }
     return this.__prefs;
   },
 
   _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);
   },
 
   _isSynced(pref) {
     return pref.startsWith(PREF_SYNC_PREFS_PREFIX) ||
            this._prefs.get(PREF_SYNC_PREFS_PREFIX + pref, false);
   },
 
   _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;
       }
     }
     return values;
   },
 
   _updateLightWeightTheme(themeID) {
@@ -131,16 +147,20 @@ PrefStore.prototype = {
     // _isSynced returns false when 'foo.pref' doesn't exist (e.g., on a new device).
     let prefs = Object.keys(values).sort(a => -a.indexOf(PREF_SYNC_PREFS_PREFIX));
     for (let pref of prefs) {
       if (!this._isSynced(pref)) {
         continue;
       }
 
       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.
         case selectedThemeIDPref:
           selectedThemeIDAfter = value;
           break;
 
         // default is to just set the pref
--- a/services/sync/services-sync.js
+++ b/services/sync/services-sync.js
@@ -25,17 +25,17 @@ pref("services.sync.errorhandler.network
 pref("services.sync.engine.addons", true);
 pref("services.sync.engine.addresses", false);
 pref("services.sync.engine.bookmarks", true);
 pref("services.sync.engine.creditcards", false);
 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);
 pref("services.sync.engine.creditcards.available", false);
 
 // If true, add-on sync ignores changes to the user-enabled flag. This
 // allows people to have the same set of add-ons installed across all
 // profiles while maintaining different enabled states.
--- a/services/sync/tests/unit/prefs_test_prefs_store.js
+++ b/services/sync/tests/unit/prefs_test_prefs_store.js
@@ -7,21 +7,30 @@
 pref("services.sync.prefs.sync.testing.int", true);
 pref("services.sync.prefs.sync.testing.string", true);
 pref("services.sync.prefs.sync.testing.bool", true);
 pref("services.sync.prefs.sync.testing.dont.change", true);
 // this one is a user pref, so it *will* sync.
 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.
 user_pref("testing.int", 123);
 user_pref("testing.string", "ohai");
 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
--- a/services/sync/tests/unit/test_prefs_store.js
+++ b/services/sync/tests/unit/test_prefs_store.js
@@ -52,45 +52,60 @@ add_task(async function run_test() {
     Assert.equal(record.value["testing.string"], "ohai");
     Assert.equal(record.value["testing.bool"], true);
     // non-existing prefs get null as the value
     Assert.equal(record.value["testing.nonexistent"], null);
     // as do prefs that have a default value.
     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!");
     record = new PrefRec("prefs", PREFS_GUID);
     record.value = {
       "testing.int": 42,
       "testing.string": "im in ur prefs",
       "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);
     Assert.equal(prefs.get("testing.string"), "im in ur prefs");
     Assert.equal(prefs.get("testing.bool"), false);
     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");
     // Ensure we don't go to the network to fetch personas and end up leaking
     // stuff.
     Services.io.offline = true;
     Assert.ok(!prefs.get("lightweightThemes.selectedThemeID"));
     Assert.equal(LightweightThemeManager.currentTheme, null);