Bug 1538015 (part 2) - restrict when incoming synced prefs are applied. r=tcsc,pauljt,Gijs
authorMark Hammond <mhammond@skippinet.com.au>
Tue, 25 Jun 2019 04:40:58 +0000
changeset 542841 e36730a6d5898a03b25439a75340e46c8240848c
parent 542840 643214222e38498bbd795d70890f0e6b9ac581b0
child 542842 a4f89580ce4029fcd0b48f9ef4563b9728beb5d1
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc, pauljt, Gijs
bugs1538015
milestone69.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 1538015 (part 2) - restrict when incoming synced prefs are applied. r=tcsc,pauljt,Gijs Specifically, a "control pref" for a pref must already exist locally, or a new preference, `services.sync.prefs.dangerously_allow_arbitrary` must be set to true. This also removes a few preferences from the set we sync by default based due to potential harm which can be caused syncing inappropriate values. Differential Revision: https://phabricator.services.mozilla.com/D29775
browser/app/profile/firefox.js
services/sync/modules/engines/prefs.js
services/sync/tests/unit/test_engine_changes_during_sync.js
services/sync/tests/unit/test_prefs_store.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1201,20 +1201,16 @@ pref("services.sync.prefs.sync.browser.n
 pref("services.sync.prefs.sync.browser.newtabpage.activity-stream.section.highlights.includeVisited", true);
 pref("services.sync.prefs.sync.browser.newtabpage.activity-stream.section.highlights.includeBookmarks", true);
 pref("services.sync.prefs.sync.browser.newtabpage.activity-stream.section.highlights.includeDownloads", true);
 pref("services.sync.prefs.sync.browser.newtabpage.activity-stream.section.highlights.includePocket", true);
 pref("services.sync.prefs.sync.browser.newtabpage.activity-stream.section.highlights.rows", true);
 pref("services.sync.prefs.sync.browser.newtabpage.enabled", true);
 pref("services.sync.prefs.sync.browser.newtabpage.pinned", true);
 pref("services.sync.prefs.sync.browser.offline-apps.notify", true);
-pref("services.sync.prefs.sync.browser.safebrowsing.phishing.enabled", true);
-pref("services.sync.prefs.sync.browser.safebrowsing.malware.enabled", true);
-pref("services.sync.prefs.sync.browser.safebrowsing.downloads.enabled", true);
-pref("services.sync.prefs.sync.browser.safebrowsing.passwords.enabled", true);
 pref("services.sync.prefs.sync.browser.search.update", true);
 pref("services.sync.prefs.sync.browser.search.widget.inNavBar", true);
 pref("services.sync.prefs.sync.browser.startup.homepage", true);
 pref("services.sync.prefs.sync.browser.startup.page", true);
 pref("services.sync.prefs.sync.browser.tabs.loadInBackground", true);
 pref("services.sync.prefs.sync.browser.tabs.warnOnClose", true);
 pref("services.sync.prefs.sync.browser.tabs.warnOnOpen", true);
 pref("services.sync.prefs.sync.browser.taskbar.previews.enable", true);
@@ -1223,28 +1219,25 @@ pref("services.sync.prefs.sync.browser.u
 pref("services.sync.prefs.sync.browser.urlbar.suggest.bookmark", true);
 pref("services.sync.prefs.sync.browser.urlbar.suggest.history", true);
 pref("services.sync.prefs.sync.browser.urlbar.suggest.openpage", true);
 pref("services.sync.prefs.sync.browser.urlbar.suggest.searches", true);
 pref("services.sync.prefs.sync.dom.disable_open_during_load", true);
 pref("services.sync.prefs.sync.dom.disable_window_flip", true);
 pref("services.sync.prefs.sync.dom.disable_window_move_resize", true);
 pref("services.sync.prefs.sync.dom.event.contextmenu.enabled", true);
-pref("services.sync.prefs.sync.extensions.personas.current", true);
 pref("services.sync.prefs.sync.extensions.update.enabled", true);
 pref("services.sync.prefs.sync.intl.accept_languages", true);
 pref("services.sync.prefs.sync.layout.spellcheckDefault", true);
 pref("services.sync.prefs.sync.media.autoplay.default", true);
 pref("services.sync.prefs.sync.media.eme.enabled", true);
 pref("services.sync.prefs.sync.network.cookie.cookieBehavior", true);
 pref("services.sync.prefs.sync.network.cookie.lifetimePolicy", true);
 pref("services.sync.prefs.sync.network.cookie.thirdparty.sessionOnly", true);
 pref("services.sync.prefs.sync.permissions.default.image", true);
-pref("services.sync.prefs.sync.pref.advanced.images.disable_button.view_image", true);
-pref("services.sync.prefs.sync.pref.advanced.javascript.disable_button.advanced", true);
 pref("services.sync.prefs.sync.pref.downloads.disable_button.edit_actions", true);
 pref("services.sync.prefs.sync.pref.privacy.disable_button.cookie_exceptions", true);
 pref("services.sync.prefs.sync.privacy.clearOnShutdown.cache", true);
 pref("services.sync.prefs.sync.privacy.clearOnShutdown.cookies", true);
 pref("services.sync.prefs.sync.privacy.clearOnShutdown.downloads", true);
 pref("services.sync.prefs.sync.privacy.clearOnShutdown.formdata", true);
 pref("services.sync.prefs.sync.privacy.clearOnShutdown.history", true);
 pref("services.sync.prefs.sync.privacy.clearOnShutdown.offlineApps", true);
@@ -1259,25 +1252,26 @@ pref("services.sync.prefs.sync.privacy.t
 pref("services.sync.prefs.sync.privacy.trackingprotection.cryptomining.annotate.enabled", true);
 pref("services.sync.prefs.sync.privacy.trackingprotection.fingerprinting.enabled", true);
 pref("services.sync.prefs.sync.privacy.trackingprotection.fingerprinting.annotate.enabled", true);
 pref("services.sync.prefs.sync.privacy.trackingprotection.pbmode.enabled", true);
 pref("services.sync.prefs.sync.privacy.resistFingerprinting", true);
 pref("services.sync.prefs.sync.privacy.reduceTimerPrecision", true);
 pref("services.sync.prefs.sync.privacy.resistFingerprinting.reduceTimerPrecision.microseconds", true);
 pref("services.sync.prefs.sync.privacy.resistFingerprinting.reduceTimerPrecision.jitter", true);
-pref("services.sync.prefs.sync.security.OCSP.enabled", true);
-pref("services.sync.prefs.sync.security.OCSP.require", true);
 pref("services.sync.prefs.sync.security.default_personal_cert", true);
-pref("services.sync.prefs.sync.security.tls.version.min", true);
-pref("services.sync.prefs.sync.security.tls.version.max", true);
 pref("services.sync.prefs.sync.services.sync.syncedTabs.showRemoteIcons", true);
 pref("services.sync.prefs.sync.signon.rememberSignons", true);
 pref("services.sync.prefs.sync.spellchecker.dictionary", true);
-pref("services.sync.prefs.sync.xpinstall.whitelist.required", true);
+
+// A preference which, if false, means sync will only apply incoming preference
+// changes if there's already a local services.sync.prefs.sync.* control pref.
+// If true, all incoming preferences will be applied and the local "control
+// pref" updated accordingly.
+pref("services.sync.prefs.dangerously_allow_arbitrary", false);
 
 // A preference that controls whether we should show the icon for a remote tab.
 // This pref has no UI but exists because some people may be concerned that
 // fetching these icons to show remote tabs may leak information about that
 // user's tabs and bookmarks. Note this pref is also synced.
 pref("services.sync.syncedTabs.showRemoteIcons", true);
 
 // Whether the character encoding menu is under the main Firefox button. This
--- a/services/sync/modules/engines/prefs.js
+++ b/services/sync/modules/engines/prefs.js
@@ -13,16 +13,55 @@ const {Store, SyncEngine, Tracker} = Chr
 const {CryptoWrapper} = ChromeUtils.import("resource://services-sync/record.js");
 const {Svc, Utils} = ChromeUtils.import("resource://services-sync/util.js");
 const {SCORE_INCREMENT_XLARGE} = ChromeUtils.import("resource://services-sync/constants.js");
 const {CommonUtils} = ChromeUtils.import("resource://services-common/utils.js");
 
 XPCOMUtils.defineLazyGetter(this, "PREFS_GUID",
                             () => CommonUtils.encodeBase64URL(Services.appinfo.ID));
 
+// In bug 1538015, we decided that it isn't always safe to allow all "incoming"
+// preferences to be applied locally. So we have introduced another preference,
+// which if false (the default) will ignore all incoming preferences which don't
+// already have the "control" preference locally set. If this new
+// preference is set to true, then we continue our old behavior of allowing all
+// preferences to be updated, even those which don't already have a local
+// "control" pref.
+const PREF_SYNC_PREFS_ARBITRARY = "services.sync.prefs.dangerously_allow_arbitrary";
+
+XPCOMUtils.defineLazyPreferenceGetter(this, "ALLOW_ARBITRARY", PREF_SYNC_PREFS_ARBITRARY);
+
+// The SUMO supplied URL we log with more information about how custom prefs can
+// continue to be synced. SUMO have told us that this URL will remain "stable".
+const PREFS_DOC_URL_TEMPLATE = "https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/sync-custom-preferences";
+XPCOMUtils.defineLazyGetter(this, "PREFS_DOC_URL",
+  () => Services.urlFormatter.formatURL(PREFS_DOC_URL_TEMPLATE));
+
+
+// Check for a local control pref or PREF_SYNC_PREFS_ARBITRARY
+this.isAllowedPrefName = function(prefName) {
+  if (prefName == PREF_SYNC_PREFS_ARBITRARY) {
+    return false; // never allow this.
+  }
+  if (ALLOW_ARBITRARY) {
+    // user has set the "dangerous" pref, so everything is allowed.
+    return true;
+  }
+  // The pref must already have a control pref set, although it doesn't matter
+  // here whether that value is true or false. We can't use prefHasUserValue
+  // here because we also want to check prefs still with default values.
+  try {
+    Services.prefs.getBoolPref(PREF_SYNC_PREFS_PREFIX + prefName);
+    // pref exists!
+    return true;
+  } catch (_) {
+    return false;
+  }
+};
+
 function PrefRec(collection, id) {
   CryptoWrapper.call(this, collection, id);
 }
 PrefRec.prototype = {
   __proto__: CryptoWrapper.prototype,
   _logName: "Sync.Record.Pref",
 };
 
@@ -94,25 +133,36 @@ PrefStore.prototype = {
       this.__prefs = new Preferences();
     }
     return this.__prefs;
   },
 
   _getSyncPrefs() {
     let syncPrefs = Services.prefs.getBranch(PREF_SYNC_PREFS_PREFIX)
                                   .getChildList("")
-                                  .filter(pref => !isUnsyncableURLPref(pref));
+                                  .filter(pref => isAllowedPrefName(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);
+    if (pref.startsWith(PREF_SYNC_PREFS_PREFIX)) {
+      // this is an incoming control pref, which is ignored if there's not already
+      // a local control pref for the preference.
+      let controlledPref = pref.slice(PREF_SYNC_PREFS_PREFIX.length);
+      return isAllowedPrefName(controlledPref);
+    }
+
+    // This is the pref itself - it must be both allowed, and have a control
+    // pref which is true.
+    if (!this._prefs.get(PREF_SYNC_PREFS_PREFIX + pref, false)) {
+      return false;
+    }
+    return isAllowedPrefName(pref);
   },
 
   _getAllPrefs() {
     let values = {};
     for (let pref of this._getSyncPrefs()) {
       // 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.
@@ -124,21 +174,53 @@ PrefStore.prototype = {
     return values;
   },
 
   _setAllPrefs(values) {
     // Update 'services.sync.prefs.sync.foo.pref' before 'foo.pref', otherwise
     // _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) {
+      let value = values[pref];
       if (!this._isSynced(pref)) {
+        // An extra complication just so we can warn when we decline to sync a
+        // preference due to no local control pref.
+        if (!pref.startsWith(PREF_SYNC_PREFS_PREFIX)) {
+          // this is an incoming pref - if the incoming value is not null and
+          // there's no local control pref, then it means we would have previously
+          // applied a value, but now will decline to.
+          // We need to check this here rather than in _isSynced because the
+          // default list of prefs we sync has changed, so we don't want to report
+          // this message when we wouldn't have actually applied a value.
+          // We should probably remove all of this in ~ Firefox 80.
+          if (value !== null) { // null means "use the default value"
+            let controlPref = PREF_SYNC_PREFS_PREFIX + pref;
+            let controlPrefExists;
+            try {
+              Services.prefs.getBoolPref(controlPref);
+              controlPrefExists = true;
+            } catch (ex) {
+              controlPrefExists = false;
+            }
+            if (!controlPrefExists) {
+              // This is a long message and written to both the sync log and the
+              // console, but note that users who have not customized the control
+              // prefs will never see this.
+              let msg = `Not syncing the preference '${pref}' because it has no local ` +
+                `control preference (${PREF_SYNC_PREFS_PREFIX}${pref}) and ` +
+                `the preference ${PREF_SYNC_PREFS_ARBITRARY} isn't true. ` +
+                `See ${PREFS_DOC_URL} for more information`;
+              console.warn(msg);
+              this._log.warn(msg);
+            }
+          }
+        }
         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;
       }
 
       if (value == null) {
         // Pref has gone missing. The best we can do is reset it.
         this._prefs.reset(pref);
--- a/services/sync/tests/unit/test_engine_changes_during_sync.js
+++ b/services/sync/tests/unit/test_engine_changes_during_sync.js
@@ -140,17 +140,19 @@ add_task(async function test_passwords_c
     engine._uploadOutgoing = uploadOutgoing;
     await cleanup(engine, server);
   }
 });
 
 add_task(async function test_prefs_change_during_sync() {
   _("Ensure that we don't bump the score when applying prefs.");
 
-  const TEST_PREF = "services.sync.prefs.sync.test.duringSync";
+  const TEST_PREF = "test.duringSync";
+  // create a "control pref" for the pref we sync.
+  Services.prefs.setBoolPref("services.sync.prefs.sync.test.duringSync", true);
 
   enableValidationPrefs();
 
   let engine = Service.engineManager.get("prefs");
   let server = await serverForEnginesWithKeys({"foo": "password"}, [engine]);
   await SyncTestingInfrastructure(server);
   let collection = server.user("foo").collection("prefs");
 
--- a/services/sync/tests/unit/test_prefs_store.js
+++ b/services/sync/tests/unit/test_prefs_store.js
@@ -98,34 +98,84 @@ add_task(async function run_test() {
       "services.sync.prefs.sync.testing.deleted-with-incoming-control-pref": true,
       "testing.somepref": "im a new pref from other device",
       "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",
+      // Make sure our "master control pref" is ignored.
+      "services.sync.prefs.dangerously_allow_arbitrary": true,
+      "services.sync.prefs.sync.services.sync.prefs.dangerously_allow_arbitrary": true,
     };
     await store.update(record);
-    // Note that 'prefs' here is looking at the *control* prefs only.
     Assert.strictEqual(prefs.get("testing.int"), 42);
     Assert.strictEqual(prefs.get("testing.string"), "im in ur prefs");
     Assert.strictEqual(prefs.get("testing.bool"), false);
     Assert.strictEqual(prefs.get("testing.deleted-without-control-pref"), "I'm deleted-without-control-pref");
     Assert.strictEqual(prefs.get("testing.deleted-with-local-control-pref"), undefined);
-    Assert.strictEqual(prefs.get("testing.deleted-with-incoming-control-pref"), undefined);
+    Assert.strictEqual(prefs.get("testing.deleted-with-incoming-control-pref"), "I'm deleted-with-incoming-control-pref");
     Assert.strictEqual(prefs.get("testing.dont.change"), "Please don't change me.");
-    Assert.strictEqual(prefs.get("testing.somepref"), "im a new pref from other device");
+    Assert.strictEqual(prefs.get("testing.somepref"), undefined);
     Assert.strictEqual(prefs.get("testing.synced.url"), "https://www.example.com");
     Assert.strictEqual(prefs.get("testing.unsynced.url"), "https://www.example.com/2");
-    Assert.strictEqual(Svc.Prefs.get("prefs.sync.testing.somepref"), true);
+    Assert.strictEqual(Svc.Prefs.get("prefs.sync.testing.somepref"), undefined);
+    Assert.strictEqual(prefs.get("services.sync.prefs.dangerously_allow_arbitrary"), false);
+    Assert.strictEqual(prefs.get("services.sync.prefs.sync.services.sync.prefs.dangerously_allow_arbitrary"), undefined);
 
     _("Only the current app's preferences are applied.");
     record = new PrefRec("prefs", "some-fake-app");
     record.value = {
       "testing.int": 98,
     };
     await store.update(record);
     Assert.equal(prefs.get("testing.int"), 42);
   } finally {
     prefs.resetBranch("");
   }
 });
+
+add_task(async function test_dangerously_allow() {
+  _("services.sync.prefs.dangerously_allow_arbitrary");
+  // read our custom prefs file before doing anything.
+  Services.prefs.readDefaultPrefsFromFile(do_get_file("prefs_test_prefs_store.js"));
+  // configure so that arbitrary prefs are synced.
+  Services.prefs.setBoolPref("services.sync.prefs.dangerously_allow_arbitrary", true);
+
+  let engine = Service.engineManager.get("prefs");
+  let store = engine._store;
+  let prefs = new Preferences();
+  try {
+    _("Update some prefs");
+    // This pref is not going to be reset or deleted as there's no "control pref"
+    // in either the incoming record or locally.
+    prefs.set("testing.deleted-without-control-pref", "I'm deleted-without-control-pref");
+    // Another pref with only a local control pref.
+    prefs.set("testing.deleted-with-local-control-pref", "I'm deleted-with-local-control-pref");
+    prefs.set("services.sync.prefs.sync.testing.deleted-with-local-control-pref", true);
+    // And a pref without a local control pref but one that's incoming.
+    prefs.set("testing.deleted-with-incoming-control-pref", "I'm deleted-with-incoming-control-pref");
+    let record = new PrefRec("prefs", PREFS_GUID);
+    record.value = {
+      "testing.deleted-without-control-pref": null,
+      "testing.deleted-with-local-control-pref": null,
+      "testing.deleted-with-incoming-control-pref": null,
+      "services.sync.prefs.sync.testing.deleted-with-incoming-control-pref": true,
+      "testing.somepref": "im a new pref from other device",
+      "services.sync.prefs.sync.testing.somepref": true,
+      // Make sure our "master control pref" is ignored, even when it's already set.
+      "services.sync.prefs.dangerously_allow_arbitrary": false,
+      "services.sync.prefs.sync.services.sync.prefs.dangerously_allow_arbitrary": true,
+
+    };
+    await store.update(record);
+    Assert.strictEqual(prefs.get("testing.deleted-without-control-pref"), "I'm deleted-without-control-pref");
+    Assert.strictEqual(prefs.get("testing.deleted-with-local-control-pref"), undefined);
+    Assert.strictEqual(prefs.get("testing.deleted-with-incoming-control-pref"), undefined);
+    Assert.strictEqual(prefs.get("testing.somepref"), "im a new pref from other device");
+    Assert.strictEqual(Svc.Prefs.get("prefs.sync.testing.somepref"), true);
+    Assert.strictEqual(prefs.get("services.sync.prefs.dangerously_allow_arbitrary"), true);
+    Assert.strictEqual(prefs.get("services.sync.prefs.sync.services.sync.prefs.dangerously_allow_arbitrary"), undefined);
+  } finally {
+    prefs.resetBranch("");
+  }
+});