1325982 - Reimplement keyword.URL as keyword.engine [SeaMonkey]

Status: ASSIGNED

(SeaMonkey :: Search, enhancement)

Description

[Philip Chee]

keyword.URL was removed in Bug 738818 in part due to search hijacking by malicious parties.

See Bug 738818 part 2: remove keyword.URL
https://hg.mozilla.org/mozilla-central/rev/ea44f501b0c5

In this bug I plan to re-implement the functionality as a string pref: "keyword.engine".

To mitigate against hijacking, keyword.engine contains a case sensitive string which is the name of one of the installed search engines (default plus user installed) e.g. "Google". If there is no match for an installed search engine or if it is empty, then the urlbar search will use the users default search engine as it does now.

This patch is against mozilla-central but will not land in m-c. Instead I plan to land this on a SeaMonkey specific release branch on mozilla-release and/or mozilla-esr52. This way I don't have to deal with Mozilla bureaucracy.

Comment 1

[Philip Chee]

Attachment: Patch 1.0 implement keyword.engine

> This patch is against mozilla-central but will not land in m-c. Instead I plan 
> to land this on a SeaMonkey specific release branch on mozilla-release and/or 
> mozilla-esr52. This way I don't have to deal with Mozilla bureaucracy.

Comment 2

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8822081 [details] [diff] [review]
Patch 1.0 implement keyword.engine

Technically all is well. Works fine. f+ for this.

But I am not sure it warrants a separate release branch. Could also probably be guarded by a def for SeaMonkey only or a call to appconstants or whatever to see that it is only used in SeaMonkey.

IanN should decide so f neutral :)

If its put in I would suggest a followup patch and adding some visual indicator or making it accessible via preferences. Now its just a hidden second search engine which might or might not be different from the current engine. And just another of these countless undocumented preferences no one knows about.

FRG

Comment 3

[Philip Chee]

> If its put in I would suggest a followup patch and adding some visual
> indicator or making it accessible via preferences. Now its just a hidden
> second search engine which might or might not be different from the current
> engine. And just another of these countless undocumented preferences no one
> knows about.

The original keyword.URL didn't have any indicator and the preferences were hidden either. I do intend to create some UI for this in a followup if this lands.

Comment 4

[Ian Neal]

Comment on attachment 8822081 [details] [diff] [review]
Patch 1.0 implement keyword.engine

Does a default need setting for "keyword.engine" like it was for "keyword.URL"?
It would be nicer if it could be part of the main code though.

Comment 5

[Mike Kaply [:mkaply]]

Comment on attachment 8822081 [details] [diff] [review]
Patch 1.0 implement keyword.engine

This looks fine as a SeaMonkey only patch. The reason keyword.URL was removed was due to hijacking, and this would bring back that problem, so I don't see this landing in the mainline.

Comment 6

[Philip Chee]

(In reply to Mike Kaply [:mkaply] from comment #5)
> Comment on attachment 8822081 [details] [diff] [review]
> Patch 1.0 implement keyword.engine
> 
> This looks fine as a SeaMonkey only patch. The reason keyword.URL was
> removed was due to hijacking, and this would bring back that problem, so I
> don't see this landing in the mainline.
The keyword engine has to be an installed engine. If a malicious player can insert a bad search engine, it's already game over.

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 8822081 [details] [diff] [review]
Patch 1.0 implement keyword.engine

Florian:

What are your thoughts on this as a Firefox patch? Once WebExtensions happens, extensions like my Keyword Search aren't working, and it would be really nice to have some clean way to specify the keyword engine separate from the search engine in the upper right.

Comment 8

[Philip Chee]

Attachment: Patch 1.1 implement keyword.engine with added #IFDEF MOZ_SUITE

This patch:
1. wraps the added code in a #ifdef MOZ_SUITE
2. Checks for a localized preference first then falls back to a CString.

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8822081 [details] [diff] [review]
Patch 1.0 implement keyword.engine

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

I don't think we would want such a pref in Firefox if it doesn't have UI to revert it easily. The f- flag is only about having this in Firefox. For a SeaMonkey only patch, I don't see a problem as I don't expect SeaMonkey to be hijacked as badly as Firefox is.

(In reply to Philip Chee from comment #6)
> (In reply to Mike Kaply [:mkaply] from comment #5)
> > Comment on attachment 8822081 [details] [diff] [review]
> > Patch 1.0 implement keyword.engine
> > 
> > This looks fine as a SeaMonkey only patch. The reason keyword.URL was
> > removed was due to hijacking, and this would bring back that problem, so I
> > don't see this landing in the mainline.
> The keyword engine has to be an installed engine. If a malicious player can
> insert a bad search engine, it's already game over.

No, we haven't blocked sideloading of search engines. The restrictions against hijacking only affect the engine set as the default.

::: docshell/base/nsDefaultURIFixup.cpp
@@ +460,5 @@
>    nsCOMPtr<nsIBrowserSearchService> searchSvc =
>      do_GetService("@mozilla.org/browser/search-service;1");
>    if (searchSvc) {
> +    nsCOMPtr<nsISearchEngine> searchEngine;
> +    nsAdoptingCString engineName = Preferences::GetCString("keyword.engine");

[this comment assumes an intent to make the patch Firefox-ready]

You need to verify that the engine we are getting from this is either a default engine (ie. an engine shipped with the browser), or that it has a valid verification hash (see http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/toolkit/components/search/nsSearchService.js#2413 for an example of check).

It would be difficult to implement here, so you would want to add a method for it in the search service, eg searchSvc->GetKeywordEngine.