1029148 - store "current search engine" configuration outside of prefs

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

A common way to hijack searches is to override the browser.search.selecedEngine pref (e.g. by writing to the user's pref file, or dropping in a user.js file in the profile, or shipping an addon with a different value for that pref).

Since this is a high-value configuration option, it makes more sense to store this information outside of prefs (which are designed to be modular and overridden).

The requirements here are:
- remove all uses of browser.search.selectedEngine pref in Firefox. This probably involves adjusting the engineMetadataService to allow setting "global" metadata not tied to a particular engine, and then using that mechanism to store a selectedEngine attribute. 
- on migration to Firefox 33, migrate the value of that pref to the new storage (users should experience no change in selected engine as a result of this change)

Comment 1

[Marco Bonardo [:mak]]

Some questions:
1. whatever storage we use here, it will still be some sort of simple file in the profile folder (let's say for example we store a json in searchplugins folder). How does this mitigate the hijacking issue, since one can just replace/modify this file instead of user.js? This us going to break the already existing methods, but new methods are basically the same with some minor changes.
2. We will have to add specific code for enterprises using ESR to manage their preferred default engine, prefs were giving this for free
3. We will need to add code to Sync to synchronize the choice, it currently just synchronizes prefs (among which browser.search.selectedEngine).

The costs so far look higher than the small benefit of breaking existing methods.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [:mak] from comment #1)
> 1. whatever storage we use here, it will still be some sort of simple file
> in the profile folder (let's say for example we store a json in
> searchplugins folder). How does this mitigate the hijacking issue, since one
> can just replace/modify this file instead of user.js? This us going to break
> the already existing methods, but new methods are basically the same with
> some minor changes.

The goal is to mitigate in the short term, not address hijacking once and for all. We are working on various other efforts to tackle the broader problem, this is just a part of it.

> 2. We will have to add specific code for enterprises using ESR to manage
> their preferred default engine, prefs were giving this for free

We're not removing "defaultenginename", which is used to set the default engine for new profiles, so this shouldn't be a problem.

> 3. We will need to add code to Sync to synchronize the choice, it currently
> just synchronizes prefs (among which browser.search.selectedEngine).

Yes, this will be complicated. I discussed this with Florian, I might be Ok with losing this functionality.

Comment 3

[Florian Quèze [:florian]]

Here is a summary of (what I took away from) the conversations I had with Gavin on this topic:

Goal: we want to make it as difficult as possible to hijack the search engine by dropping a file or editing prefs.js in the profile folder.

Non-goal: attempting to mitigate hijacking done by touching the application folder, or by installing an add-on (and then using the nsIBrowserSearchService API).

Current state:
Since bug 860560 setting a user value to either of the browser.search.selectedEngine or browser.search.defaultenginename preferences changes the engine.

Proposed changes:
- completely stop using browser.search.selectedEngine (except for a one time migration),
- ignore user values of browser.search.defaultenginename (we still need to respect the default value, because that's the way the default search engine is customized for different locales, or for ESR users).
- Store the current search engine in the search-metadata.json file (this means the engineMetadataService object in nsSearchService.js may need some tweaking).
- No longer sync the selected search engine.
- Include in the search-metadata.json file a verification hash that should be unique to the current profile. This is to prevent hijacking through an installer copying the same search-metadata.json file to the profile of all users.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

So basically generate a random key, store it in search-metadata.json and somewhere else (preferences, again?) and check that it is correct? Or use a hash from the current profile name?

Comment 5

[Florian Quèze [:florian]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> So basically generate a random key, store it in search-metadata.json and
> somewhere else (preferences, again?) and check that it is correct? Or use a
> hash from the current profile name?

Indeed, using the profile name was what I had in mind. Saving something else in the preferences wouldn't be a real improvement.

Comment 6

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> > So basically generate a random key, store it in search-metadata.json and
> > somewhere else (preferences, again?) and check that it is correct? Or use a
> > hash from the current profile name?
> 
> Indeed, using the profile name was what I had in mind. Saving something else
> in the preferences wouldn't be a real improvement.

You mean the random directory path? Because the profile name itself is always 'default', by, err, default...

Comment 7

[:Gijs (he/him)]

Also, using that path/ID will mean that this will invalidate the search engine pref/hash when using Firefox Reset, or when you move profile data to a different machine... which might be OK, but is something we should keep in mind.

Comment 8

[Florian Quèze [:florian]]

I mean the name of the folder that contains the profile.

When I move a profile from a machine to another I usually keep the whole folder, so the name would be kept. I guess others could just take the content of the folder, in which case yes, we would lose the search engine tweaks that the user made.

Not sure about the expected behavior when using Firefox Reset. Thanks for pointing it out.

Comment 9

[Robert Kaiser]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> - No longer sync the selected search engine.

Hrm, that would be a sad side-effect, as it's pretty likely that if I set DuckDuckGo as my primary search engine, I'd like to have that on all devices.

Comment 10

[Florian Quèze [:florian]]

Attachment: WIP

This patch implements the changes proposed in comment 3 and seems to work, but I need to look closer at the rest of the code to check that I'm not breaking things, and think about testing.

Comment 11

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Challenging project, good progress.

Would it make sense to attempt to enumerate all the downstream user experiences that currently rely on search's "preffiness", to allow mitigating impact?

Among these:

- about:support, SUMO
- Reset Firefox
- FHR data collection and segmentation
- Test Pilot search related work
- profile migration (automatic and manual)
- Sync (for settings)

All of these rely in exposing *something*.  Some of them rely on having "settable" things.

Comment 12

[Bill Selman]

From a user study I've been working on, there is some inconsistency out in the wild between the browser.search.defaultenginename value and the user's actual default set in the searchbox.

More detail: I've been conducting a user study on search hijacking specifically recruiting users who have been hijacked. The most effective way to screen hijacked users is to view the value set in about:config in browser.search.defaultenginename. However, we have received quite a few false positives where a participant's value for browser.search.defaultenginename will be a hijacking search engine, but their default searchbox is set to google. I've followed up with some participants to ask them if they recall switching back to google as their search engine (or whatever their default) and no one can recall how it happened.

I'm not sure how this setting mismatch happens in practice. Perhaps an anti-virus program?

I will attach a screenshot of an instance of this mismatch.

Comment 13

[Bill Selman]

Attachment: Default_search_engine_config_mismatch.png

Comment 14

[Florian Quèze [:florian]]

(In reply to Bill Selman from comment #12)

If the hijacking happened a long time ago (before Firefox 24), the mismatch could be something that has been fixed by bug 860560.

Comment 15

[Florian Quèze [:florian]]

Attachment: Patch v2

I'm unsure about how to test this. I would need to re-init the search service after writing to the profile folder in the test. The only idea I have for now is to add a wrappedJSObject getter on the service and use it to reset some internal values so that the initialization code can run again. I'm not really happy with this idea though.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> Created attachment 8451756 [details] [diff] [review]
> Patch v2
> 
> I'm unsure about how to test this. I would need to re-init the search
> service after writing to the profile folder in the test.

A slightly cleaner path than wrappedJSObject may be to use the fact ss is an nsIObserver and call .observe() with your "test-do-something" topic, and there you might do whatever internal work you need. This would restrict the internals exposure to specific actions you need.

Comment 17

[Marco Bonardo [:mak]]

hm, and well, if you just put each single test in a separate xpcshell-test there should not even be a problem, just do whatever setup you need before initing ss.

Comment 18

[Kris Maglione [:kmag]]

Our current blocklisting system allows us to revert preference changes when a blocked add-on is detected. That currently allows us to reset the selected search engine for add-ons which change it, but that will no longer be possible after these change.

We need to either add a new mechanism to reset search changes when malicious add-ons are detected, or magically reset it when we receive a request to reset 'browser.search.defaultenginename'. I'm weakly in favor of the latter approach.

Comment 19

[Mike Connor [:mconnor]]

We'll have to address this in bug 1031441

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8451756 [details] [diff] [review]
Patch v2

Review of attachment 8451756 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. It seems like this may makes it harder for us to change the default search engine for profiles which haven't explicitly indicated a preference (but I haven't fully thought through it). I'm not sure if this is how we would do such a migration if needed anyways. (i.e. only change new profiles or also change existing profiles with no user-set selectedEngine).

This definitely needs a test before landing and Marco's suggestion seems reasonable.

Comment 21

[Florian Quèze [:florian]]

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #20)
> Comment on attachment 8451756 [details] [diff] [review]

> this may makes it harder for us to change
> the default search engine for profiles which haven't explicitly indicated a
> preference

I don't think it will. If there's no user-set search engine in the profile, after the patch we still use the value of the browser.search.defaultenginename preference as the default search engine.

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 8451756 [details] [diff] [review]
Patch v2

I like Marco's suggestion in comment 16.

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8451756 [details] [diff] [review]
Patch v2

Review of attachment 8451756 [details] [diff] [review]:
-----------------------------------------------------------------

I can't help but think we have too much logic differing between defaultEngine and currentEngine which should be the same at all times. I'm not sure why the code wasn't simplified more when the two were made to be the same. Are there cases where they aren't the same? i.e. Can't defaultEngine just be an alias for currentEngine?

For posterity, why was the metadata service chosen for this (with the fake global engine) rather than putting this in search.json which already supports global properties and I believe already exists for all users? I realize it's more of a cache but it seems to fit this use. This patch may be adding extra I/O for a large proportion of our users since search-metadata.json currently doesn't exist for users who haven't customized their search engines AFAIK.

r- for lack of tests (which I figured would come before my review).

::: browser/components/nsBrowserGlue.js
@@ +1615,5 @@
>      }
>  
> +    if (currentUIVersion < 23) {
> +      const kSelectedEnginePref = "browser.search.selectedEngine";
> +      if (Services.prefs.prefHasUserValue(kSelectedEnginePref)) {

If I'm not mistaken, this didn't create the hash upon migration if the engine is the default. This means that if I have the original default engine of "Google" as my current engine, I will still be susceptible to hijacking after this patch.

I think we may want to be more defensive for this migration to ensure malware doesn't trigger it again and cause the currentEngine to be overwritten by modifying the migration version in prefs.js to a value lower than 23. I verified that this hijacking attempt is possible with this patch.

::: toolkit/components/search/nsSearchService.js
@@ +3697,5 @@
> +    // Data is an array of bytes.
> +    let data = converter.convertToByteArray(str, {});
> +    let hasher = Cc["@mozilla.org/security/hash;1"]
> +                   .createInstance(Ci.nsICryptoHash);
> +    hasher.init(hasher.SHA256);

Nit: Ci.nsICryptoHash.SHA256 is a little more clear/standard IMO.

@@ +4057,5 @@
>      if (!this._currentEngine) {
> +      let name = engineMetadataService.getGlobalAttr("current");
> +      if (engineMetadataService.getGlobalAttr("hash") == this._getVerificationHash(name)) {
> +        this._currentEngine = this.getEngineByName(name);
> +      }

I think an else block here that logs a message when the hash doesn't match would be useful for debugging tests and with bug reports.

@@ -4169,5 @@
>    _addObservers: function SRCH_SVC_addObservers() {
>      Services.obs.addObserver(this, SEARCH_ENGINE_TOPIC, false);
>      Services.obs.addObserver(this, QUIT_APPLICATION_TOPIC, false);
>      Services.prefs.addObserver(BROWSER_SEARCH_PREF + "defaultenginename", this, false);
> -    Services.prefs.addObserver(BROWSER_SEARCH_PREF + "selectedEngine", this, false);

This will break extensions who set the selectedEngine from their code instead of setting it through the service. If we're fine with that (assuming I'm correct), why don't we also remove the defaultenginename pref observer too to simplify the code?

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #23)
> I can't help but think we have too much logic differing between
> defaultEngine and currentEngine which should be the same at all times. I'm
> not sure why the code wasn't simplified more when the two were made to be
> the same. Are there cases where they aren't the same? i.e. Can't
> defaultEngine just be an alias for currentEngine?

There's history in bug 738818 and bug 860560. I think the short answer is "API compatibility concerns" (both for other apps using the search service and add-ons). Not a great choice in retrospect but not worth revisiting now unfortunately :( 

> For posterity, why was the metadata service chosen for this (with the fake
> global engine) rather than putting this in search.json which already
> supports global properties and I believe already exists for all users? I
> realize it's more of a cache but it seems to fit this use. This patch may be
> adding extra I/O for a large proportion of our users since
> search-metadata.json currently doesn't exist for users who haven't
> customized their search engines AFAIK.

search.json is a cache, and it gets blown away in various cases (e.g. when build ID changes). The selected engine can't get blown away. Don't see how we'd resolve that conflict without larger changes.

> I think an else block here that logs a message when the hash doesn't match
> would be useful for debugging tests and with bug reports.

Telemetry!

Comment 25

[Matthew N. [:MattN]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #24)
> (In reply to Matthew N. [:MattN] (behind on bugmail) from comment #23)
> > I can't help but think we have too much logic differing between
> > defaultEngine and currentEngine which should be the same at all times. I'm
> > not sure why the code wasn't simplified more when the two were made to be
> > the same. Are there cases where they aren't the same? i.e. Can't
> > defaultEngine just be an alias for currentEngine?
> 
> There's history in bug 738818 and bug 860560. I think the short answer is
> "API compatibility concerns" (both for other apps using the search service
> and add-ons). Not a great choice in retrospect but not worth revisiting now
> unfortunately :( 

I skimmed them quickly the other day but didn't see an answer. I guess bug 738818 comment 105 is one.

> > For posterity, why was the metadata service chosen for this (with the fake
> > global engine) rather than putting this in search.json which already
> > supports global properties and I believe already exists for all users? I
> > realize it's more of a cache but it seems to fit this use. This patch may be
> > adding extra I/O for a large proportion of our users since
> > search-metadata.json currently doesn't exist for users who haven't
> > customized their search engines AFAIK.
> 
> search.json is a cache, and it gets blown away in various cases (e.g. when
> build ID changes). The selected engine can't get blown away. Don't see how
> we'd resolve that conflict without larger changes.

I was hoping that in the cases where it gets blown away we had already read the value into memory and could write it out again while recreating search.json.

> > I think an else block here that logs a message when the hash doesn't match
> > would be useful for debugging tests and with bug reports.
> 
> Telemetry!

Good idea.

Comment 26

[Florian Quèze [:florian]]

Attachment: Patch v3

Now with a test, following the approach in comment 16. Bug 1018240 has conveniently added a way to re-init the search service from an observer notification since I wrote attachment 8451756 [details] [diff] [review].

I removed the defaultenginename pref observer, as suggested in comment 23, but it seems (https://tbpl.mozilla.org/?tree=Try&rev=dca69e0f09b6) that breaks browser/base/content/test/general/browser_aboutHome.js so either we should revert this change, or I'll look into fixing that test.

I'm not completely sure what the expected use case of Services.search.defaultEngine is at this point; it looks like we could as well remove it. But that's probably for another bug.

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8461223 [details] [diff] [review]
Patch v3

Review of attachment 8461223 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> I removed the defaultenginename pref observer, as suggested in comment 23,
> but it seems (https://tbpl.mozilla.org/?tree=Try&rev=dca69e0f09b6) that
> breaks browser/base/content/test/general/browser_aboutHome.js so either we
> should revert this change, or I'll look into fixing that test.

I suspect the test should change but without knowing what it's doing it's hard to say.

::: toolkit/components/search/nsSearchService.js
@@ +3199,5 @@
>      this.__sortedEngines = null;
> +    this._currentEngine = null;
> +    this._defaultEngine = null;
> +
> +    // Clear the meta data service.

Nit: "metadata"

::: toolkit/components/search/tests/xpcshell/test_prefSync.js
@@ -83,5 @@
> -
> -  // Test that setting the currentEngine to the original default engine clears
> -  // the selectedEngine pref, rather than setting it. To do this we need to
> -  // set the value of defaultenginename on the default branch.
> -  let defaultBranch = Services.prefs.getDefaultBranch("");

A test like this should be preserved (in test_selectedEngine.js) to make sure we don't regress my concern about changing the default in the future.

::: toolkit/components/search/tests/xpcshell/test_selectedEngine.js
@@ +44,5 @@
> +function asyncReInit() {
> +  let promise = waitForNotification("reinit-complete");
> +
> +  Services.search.QueryInterface(Ci.nsIObserver)
> +          .observe(null, "nsPref:changed", "general.useragent.locale");

I don't think this is used.

@@ +93,5 @@
> +  do_check_eq(Services.search.currentEngine.name, getDefaultEngineName());
> +});
> +
> +// An engine set without a valid hash should be ignored.
> +add_task(function* test_persistAcrossRestarts() {

duplicate/inaccurate function name

@@ +116,5 @@
> +  do_check_eq(Services.search.currentEngine.name, getDefaultEngineName());
> +});
> +
> +
> +function run_test() {

A sanity check that the hash is truthy wouldn't hurt. Perhaps testing the the hash function (verifying it takes the profile path salt into account) would be good.

Comment 28

[Florian Quèze [:florian]]

Comment on attachment 8461223 [details] [diff] [review]
Patch v3

Review of attachment 8461223 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/tests/xpcshell/test_selectedEngine.js
@@ +44,5 @@
> +function asyncReInit() {
> +  let promise = waitForNotification("reinit-complete");
> +
> +  Services.search.QueryInterface(Ci.nsIObserver)
> +          .observe(null, "nsPref:changed", "general.useragent.locale");

It is since http://hg.mozilla.org/mozilla-central/rev/0234ad055a7b (or I don't understand your comment).

Comment 29

[Matthew N. [:MattN]]

(In reply to Florian Quèze [:florian] [:flo] from comment #28)
> Comment on attachment 8461223 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8461223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/search/tests/xpcshell/test_selectedEngine.js
> @@ +44,5 @@
> > +function asyncReInit() {
> > +  let promise = waitForNotification("reinit-complete");
> > +
> > +  Services.search.QueryInterface(Ci.nsIObserver)
> > +          .observe(null, "nsPref:changed", "general.useragent.locale");
> 
> It is since http://hg.mozilla.org/mozilla-central/rev/0234ad055a7b (or I
> don't understand your comment).

Ah, sorry, I misread this as adding a listener. Nevermind this. Note bug 1043627 though.

Comment 30

[Kris Maglione [:kmag]]

(In reply to Bill Selman from comment #12) 
> More detail: I've been conducting a user study on search hijacking
> specifically recruiting users who have been hijacked. The most effective way
> to screen hijacked users is to view the value set in about:config in
> browser.search.defaultenginename. However, we have received quite a few
> false positives where a participant's value for
> browser.search.defaultenginename will be a hijacking search engine, but
> their default searchbox is set to google. I've followed up with some
> participants to ask them if they recall switching back to google as their
> search engine (or whatever their default) and no one can recall how it
> happened.

I think the issue with this screenshot is that the value of the preference is not a valid search engine name, it's a URL. Unlike the old 'keyword.URL' preference, 'browser.search.defaultenginename' only makes sense if it contains the name of an installed search engine. If it's not valid, the search service just defaults to (in this case) Google.

Comment 31

[Kris Maglione [:kmag]]

I think that, since we're relying on a salted hash here, we should require that the salt include a string along the lines of:

"By modifying this file, I agree that I am doing so only within $appName itself, using official, user-driven search engine selection processes, and in a way which does not circumvent user consent. I acknowledge that any attempt to change this file from outside of $appName is a malicious act, and will be responded to accordingly."

I also think that we should try to make sure that we're implementing this in such a way that we're certain it would trigger the anti-circumvention provisions of the DMCA if modified from the outside.

Comment 32

[Florian Quèze [:florian]]

Attachment: Patch v4

(In reply to Matthew N. [:MattN] from comment #27)

> (In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > I removed the defaultenginename pref observer, as suggested in comment 23,
> > but it seems (https://tbpl.mozilla.org/?tree=Try&rev=dca69e0f09b6) that
> > breaks browser/base/content/test/general/browser_aboutHome.js so either we
> > should revert this change, or I'll look into fixing that test.
> 
> I suspect the test should change but without knowing what it's doing it's
> hard to say.

I can't reproduce locally, so we will see what will happen on try (https://tbpl.mozilla.org/?tree=Try&rev=e69066ab8d84) with this new version of the patch, maybe someone fixed it somehow since last week.

> ::: toolkit/components/search/tests/xpcshell/test_prefSync.js
> @@ -83,5 @@
> > -
> > -  // Test that setting the currentEngine to the original default engine clears
> > -  // the selectedEngine pref, rather than setting it. To do this we need to
> > -  // set the value of defaultenginename on the default branch.
> > -  let defaultBranch = Services.prefs.getDefaultBranch("");
> 
> A test like this should be preserved (in test_selectedEngine.js) to make
> sure we don't regress my concern about changing the default in the future.

I'm glad you asked me to test this. That's how I discovered that the current engineMetadataService._commit implementation always saves the same data (the 'store' variable was kept in a closure).

I removed the aStore parameter that didn't seem used anywhere (but maybe try will tell me I was wrong on that).

> A sanity check that the hash is truthy wouldn't hurt. Perhaps testing the
> the hash function (verifying it takes the profile path salt into account)
> would be good.

I added a check for the length of the hash (which should always be the same). I don't see a good way to actually test the hash function without duplicating most of its code in the test; which would end up not testing much.


About bug 1043627, Gavin said there that he was OK with us keeping that reinitialization code pref'ed off for Firefox desktop. After looking at this again, I think removing the ifdef only in the observe method but not around the add/removeObserver calls is even easier and doesn't expose us to more risk than a pref would.

Comment 33

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #32)
> Created attachment 8466378 [details] [diff] [review]
> Patch v4
> 
> (In reply to Matthew N. [:MattN] from comment #27)
> 
> > (In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > > I removed the defaultenginename pref observer, as suggested in comment 23,
> > > but it seems (https://tbpl.mozilla.org/?tree=Try&rev=dca69e0f09b6) that
> > > breaks browser/base/content/test/general/browser_aboutHome.js so either we
> > > should revert this change, or I'll look into fixing that test.
> > 
> > I suspect the test should change but without knowing what it's doing it's
> > hard to say.
> 
> I can't reproduce locally, so we will see what will happen on try
> (https://tbpl.mozilla.org/?tree=Try&rev=e69066ab8d84) with this new version
> of the patch, maybe someone fixed it somehow since last week.

Still not fixed :(.

Comment 34

[Matthew N. [:MattN]]

Comment on attachment 8466378 [details] [diff] [review]
Patch v4

Review of attachment 8466378 [details] [diff] [review]:
-----------------------------------------------------------------

r=me (ignoring the test issue)

Comment 35

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #32)

> > ::: toolkit/components/search/tests/xpcshell/test_prefSync.js
> > @@ -83,5 @@
> > > -
> > > -  // Test that setting the currentEngine to the original default engine clears
> > > -  // the selectedEngine pref, rather than setting it. To do this we need to
> > > -  // set the value of defaultenginename on the default branch.
> > > -  let defaultBranch = Services.prefs.getDefaultBranch("");
> > 
> > A test like this should be preserved (in test_selectedEngine.js) to make
> > sure we don't regress my concern about changing the default in the future.
> 
> I'm glad you asked me to test this. That's how I discovered that the current
> engineMetadataService._commit implementation always saves the same data (the
> 'store' variable was kept in a closure).

Gavin asked me for details about this so I looked into it again. It only became a problem because when reinitializing the search service we call engineMetadataService.init again and that creates a new object in engineMetadataService._store.

Comment 36

[Kris Maglione [:kmag]]

I'm going to ask again, please do not land this without adding a disclaimer about outside modifications to the hash input. As the person who is most often responsible for dealing with violators in these cases, I can tell you that it is much more difficult to do so when it's not crystal clear that they're blatantly and knowingly in the wrong.

Adding a disclaimer to the hash input is a trivial change now and will be much more difficult once it lands.

Comment 37

[Matthew N. [:MattN]]

(In reply to Kris Maglione [:kmag] from comment #31)
> I also think that we should try to make sure that we're implementing this in
> such a way that we're certain it would trigger the anti-circumvention
> provisions of the DMCA if modified from the outside.

Presumably this would require talking with legal and could block this bug for weeks so feel free to start that discussion with legal now. If this actually will help legally then I don't have an objection. If not, it's not going to do anything for malicious authors.

Gavin, what are your thoughts on comment 31 and comment 36?

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Adding that info about intent to the salt is a reasonable request, regardless of its possible legal implications or uses. Florian said he was going to whip up a patch.

Comment 39

[Florian Quèze [:florian]]

Attachment: Add a disclaimer in the salt

Comment 40

[Kris Maglione [:kmag]]

(In reply to Matthew N. [:MattN] from comment #37)
> Presumably this would require talking with legal and could block this bug
> for weeks so feel free to start that discussion with legal now.

I don't think there's any need to block on legal, but I do think it's worth checking in case, e.g., using a cryptographic signature gives us a stronger legal position.

(In reply to Florian Quèze [:florian] [:flo] from comment #39)
> Add a disclaimer in the salt

I have one nit. I'd prefer that the disclaimer comes after the profile path, so that no-one can claim that they used a pre-computed seed rather than the disclaimer string itself.

Comment 41

[Florian Quèze [:florian]]

Attachment: Add a disclaimer in the salt v2

(In reply to Kris Maglione [:kmag] from comment #40)

> I have one nit. I'd prefer that the disclaimer comes after the profile path,
> so that no-one can claim that they used a pre-computed seed rather than the
> disclaimer string itself.

Ok.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8467375 [details] [diff] [review]
Add a disclaimer in the salt v2

You could use fancy new template strings for this (which aren't documented that well yet, but see http://tc39wiki.calculist.org/es6/template-strings/ and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/template_strings), but that's not upliftable.

Comment 43

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/eae149b23424
https://hg.mozilla.org/integration/fx-team/rev/10f1f952ff73

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eae149b23424
https://hg.mozilla.org/mozilla-central/rev/10f1f952ff73

Comment 45

[Kris Maglione [:kmag]]

This is probably not the ideal place to discuss this, but should we also store the homepage setting outside of preferences in the same way? A significant amount of our search traffic comes from about:home, and a lot of search hijackers hijack the homepage at the same time.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Sure - want to file that bug?

Comment 47

[Kris Maglione [:kmag]]

Bug 1051082

Comment 48

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Verified using latest Nightly 2014-08-10 under Win 7 64-bit, Ubuntu 13.04 32-bit and Mac OSX 10.9.4 all the specific points from comment 3.

It seems that now, the selected search engine is not remembered when opening another Firefox version with the same profile as on Nightly (where I changed the default search engine), even if the user is not connected to sync. Is this expected or should I file a new bug for it?

Is there any other verification needed? Thank you!

Comment 49

[Florian Quèze [:florian]]

(In reply to Petruta Rasa [QA] [:petruta] from comment #48)

> It seems that now, the selected search engine is not remembered when opening
> another Firefox version with the same profile as on Nightly (where I changed
> the default search engine), even if the user is not connected to sync. Is
> this expected or should I file a new bug for it?

If you mean that you set the search engine using Nightly, and then use the same profile with an older Firefox and it doesn't see the change you made using Nightly, I think that's expected.

Comment 50

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Thanks, marking as verified based on above comments.

Comment 51

[:Gijs (he/him)]

(In reply to Gregg Lind (User Research - Test Pilot) from comment #11)
> Challenging project, good progress.
> 
> Would it make sense to attempt to enumerate all the downstream user
> experiences that currently rely on search's "preffiness", to allow
> mitigating impact?
> 
> Among these:
> 
> - about:support, SUMO
> - Reset Firefox
> - FHR data collection and segmentation
> - Test Pilot search related work
> - profile migration (automatic and manual)
> - Sync (for settings)
> 
> All of these rely in exposing *something*.  Some of them rely on having
> "settable" things.

Did we file bugs to address this list? I'm thinking sync in particular; we should expect backlash if we did break this, and I thought sync was able to sync other things than prefs?

Comment 52

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch (intermittently here 14-15 August; then away until 19th) from comment #51)
> (In reply to Gregg Lind (User Research - Test Pilot) from comment #11)

> > - about:support, SUMO
> > - Reset Firefox
> > - FHR data collection and segmentation
> > - Test Pilot search related work
> > - profile migration (automatic and manual)
> > - Sync (for settings)
> > 
> > All of these rely in exposing *something*.  Some of them rely on having
> > "settable" things.
> 
> Did we file bugs to address this list? I'm thinking sync in particular; we
> should expect backlash if we did break this, and I thought sync was able to
> sync other things than prefs?

I didn't. Gavin said in comment 2 that he might be OK with breaking sync. I think I did some limited testing and didn't notice anything broken in the other things mentioned in this list. Might be worth having people who actually understand these areas double check that everything is fine though.

Comment 53

[:Gavin Sharp [email: [e-mail]]]

Fixing Sync shouldn't be too hard, can you file a followup?

Comment 54

[Florian Quèze [:florian]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #53)
> Fixing Sync shouldn't be too hard, can you file a followup?

Filed bug 1054960.