1318070 - keyword.enabled is half-broken, it's half enabled even when it's set to false

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[avada]

When I add a single "word" of gibberish to the urlbar and press enter the expected thing happens: FF tries to resolve it, then I get an error page.

However when i add multiple words of gibberish and press enter a search with the default search engine is performed. Which shouldn't happen. I should get a "The address isn't valid" error.

It happens with a brand new profile after setting keyword.enabled=false

Comment 1

[Tsudojon Yurohenyo]

It is particularly annoying when using bookmark keywords regularly (because there will be spaces between the keyword and the arguments). If you mistype the keyword, then Firefox will leak the content of your address bar to Google, instead of displaying some "The address wasn't understood" local error page.

It is definitely new, although I'm not sure when it started. I occasionally mistype bookmark keywords, and it didn't send me to Google before. I don't think I changed anything to my configuration which could have changed this. I'm using Firefox 51 currently, and only noticed it yesterday.

I don't think anyone would want to disable "keyword.enabled", but still keep the feature when entering multiple words... Searches can often be a single word, so this would only work half the time... (note that including spaces before or after one word as a trick, does not send you to Google, even with "browser.fixup.alternate.enabled" disabled, because Firefox will trim the spaces, interpret the word as a top-level domain, and try to connect to it with HTTP).


Thus "keyword.enabled" being set to "false" should prevent Firefox from sending the content of the address bar to the default search engine, whether there are multiples words or not in the address bar. A "The address wasn't understood" error page should be displayed if no bookmark keywords are recognized.


The current workaround is to spend two hours and a half to:

1) Read the OpenSearch specs.

2) Create a custom OpenSearch plugin.

3) Set the template to something like "http://local/{searchTerms}" (and add "127.0.0.1 local" to your "/etc/hosts" file if not done before, so no DNS request is sent) because Firefox refuses to use only "{searchTerms}", or an empty string, or start with anything that's not some recognized protocols (can't use "about:blank", for example).

4) Then upload it somewhere because Firefox does not have a button to add OpenSearch plugins manually, nor accept local files, nor accept files in a "searchplugins" directory in your profile because it has been replaced by a binary file grouping all search engines.

5) Then create a webpage to point to the plugin because Firefox cannot load the plugin directly.

6) Then add the search bar to the toolbar because I don't use the search bar.

7) Then muck around some more, because "Firefox could not download the search plugin from: http://home.local/search.xml", without any detail in the browser console, isn't quite intuitive. Don't forget to also load your XML file in another tab, to reload it every time you change it, because caching. And to go to the preferences to remove the search engine every time you need to adjust something while testing, because you cannot see or edit them from the UI, and adding an engine with the same name results in an error, without the possibility to replace the current engine.

8) Then go to the preferences to set the new engine as the default one.

9) Then when you mistype a bookmark keyword, you just have to remove the URL prefix you added, and remove the first '+' which Firefox added to convert the spaces.

10) Then search Mozilla's bugzilla, and write a comment to give more details and a workaround, because the workaround is partial and annoying.

Note you cannot use "keyword.URL", because it has been remove some time ago. You couldn't set it to an empty string anyway, because it would then default to the default search engine.

Comment 2

[Mark Smith [:mcs]]

Attachment: proposed fix

This patch fixes the problem for me. It would be really good to fix this since it can lead to unintentional information leaks (small typos leak info to my default search engine).

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8835129 [details] [diff] [review]
proposed fix

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

This works, but it's not coherent with the rest, in particular, if we skip this last catch-all handler, we won't show any action, we should instead show a visit action.

You should rather modify _matchUnknownUrl to return a broken visituri entry if urifixup throws an NS_ERROR_MALFORMED_URI, something like:

    try {
      fixupInfo = Services.uriFixup.getFixupURIInfo(this._originalSearchString,
                                                    flags);
    } catch (e) {
=>    if (e.result == Cr.NS_ERROR_MALFORMED_URI && !Prefs.keywordEnabled) {
        let value = PlacesUtils.mozActionURI("visiturl", {
          url: this._originalSearchString,
          input: this._originalSearchString,
        });
        this._addMatch({
          value,
          comment: this._originalSearchString,
          style: "action visiturl",
          frecency: 0,
        });

        return true;
      }
      return false;
    }

And then this needs a test, you can probably add one to toolkit/components/places/tests/unifiedcomplete/test_visit_url.js
See for example
http://searchfox.org/mozilla-central/rev/afcf40f3eafd895611a839017730debb58a342a6/toolkit/components/places/tests/unifiedcomplete/test_visit_url.js#132

After we have this, you can check the other tests are still good, through ./mach test toolkit/components/places/tests/unifiedcomplete/

Comment 4

[Charles]

This is affecting me and others as well: https://support.mozilla.org/t5/Firefox/How-do-I-prevent-ALL-searches-from-the-address-bar/m-p/1372606

It would be nice to get the above fix out in the beta channel soon - it's a terrible and unexpected leak of private/confidential information.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Mark, do you plan to complete the work for your patch?

Comment 7

[Mark Smith [:mcs]]

(In reply to Marco Bonardo [::mak] from comment #5)
> Mark, do you plan to complete the work for your patch?

I would like to, but I do not know when I will have time to work on this again. I think it is an important bug though and I really hope someone else finds time to work on it!

Comment 8

[moz-nuc]

As a consequence of this information is also leaked to the "default" search engine when a search keyword is mis-typed or keypress is dropped.

So e.g. if the user has a search keyword 'w' for Wikipedia, and the letter 'w' is mis-typed or keypress doesn't register, the entire search goes to e.g. Google instead.

Thus:

w Something that's none of Google's business -> Wikipedia

Mis-type:
e Something that's none of Google's business -> Google

Lost keypress:
 Something that's none of Google's business -> Google

End result: Google aggregates mis-directed query with everything else it is inexorably collecting about the user. /tinfoil

Compounding this, the UI designers made it impossible to remove every 'standard' search engine (i.e. those in about:preferences#search), even if the user never wants to use search other than via keywords. As per Tsudojon's beautifully illustrated comment.

Also I'm fairly sure the keyword.enabled setting used to work properly (e.g. a year ago).

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Bug 1318070 - Make sure multi-word queries are rejected when keyword.enabled is false

Review commit: https://reviewboard.mozilla.org/r/121724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121724/

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Something like this? The test I added appears to pass, but I'm not sure if it's testing the right thing. 

When I do a two word query with this patch and keyword.enabled = false, it just brings up a "The address isn’t valid" error page, and the two word query stays in the address bar, unchanged.

Comment 11

[Charles]

FWIW, a local "Address isn't valid" error page/message is *exactly* what I want FF to bring up when I paste the wrong thing into the address bar and hit enter by reflex.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8848844 [details]
Bug 1318070 - Make sure multi-word queries are rejected when keyword.enabled is false

https://reviewboard.mozilla.org/r/121724/#review124448

LGTM!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13b38cd29a4a
Make sure multi-word queries are rejected when keyword.enabled is false r=mak

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/141019b411dc
Backed out changeset 13b38cd29a4a for esline failures a=backout
https://hg.mozilla.org/integration/autoland/rev/784c281fcc1e
Make sure multi-word queries are rejected when keyword.enabled is false r=mak

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Autoland's new zero tolerance policy means this got backed out in https://hg.mozilla.org/integration/autoland/rev/141019b411dc for an eslint failure.

Relanded with a fix in https://hg.mozilla.org/integration/autoland/rev/784c281fcc1e

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/784c281fcc1e

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8848844 [details]
Bug 1318070 - Make sure multi-word queries are rejected when keyword.enabled is false

Would probably be good to uplift this as far as we're able to. I'll make sure the patch uplifts cleanly.

Approval Request Comment
[Feature/Bug causing the regression]:

[User impact if declined]:
Potential data leaks to the default search engine if you use bookmark keywords and mistype the keyword.

[Is this code covered by automated tests?]:
Test added in patch.

[Has the fix been verified in Nightly?]:
Not yet.

[Needs manual test from QE? If yes, steps to reproduce]: 
I don't think it does. But the steps to test are:
1. Flip keyword.enabled to false.
2. Type a two+ word query into the location bar and hit enter.
If it results in a search to yahoo (or whatever the default search engine is), that's a failure.
If it results in an internal error page and no search is performed, the patch is working as intended.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Can't really speak to the risk, but it shouldn't be very risky.

[String changes made/needed]:
None.

NOTE TO UPLIFTER: The commit that made it to mozilla-central is the one to be uplifted, otherwise you'll be uplifting an eslint failure.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

As far as I know, all supported versions except esr45 are affected.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Patch applies cleanly to aurora and beta (and release/esr52, if we want to go that far).

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8848844 [details]
Bug 1318070 - Make sure multi-word queries are rejected when keyword.enabled is false

Would be nice to not leave this unfixed on esr52 for a year. I just applied the patch to esr52, built locally and checked that it works.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Could prevent a leak of things typed in the location bar to default search engines.
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 21

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8848844 [details]
Bug 1318070 - Make sure multi-word queries are rejected when keyword.enabled is false

Fix a broken multi-word queries issue when keyword.enabled is false. Aurora54+ & Beta53+.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ca1fbc0c4c17

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2bbf316c5d61

Comment 25

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Wes' assessment on manual testing needs (see Comment 17) and the fact that this fix has automated coverage.

Gerry, we'll make sure to include this scenario when we run our recurring Awesomebar test suite on Beta 53 -- moving the ni? from Brindusa to myself.

Comment 26

[Julien Cristau [:jcristau]]

Comment on attachment 8848844 [details]
Bug 1318070 - Make sure multi-word queries are rejected when keyword.enabled is false

fix handling of keyword.enabled=false, esr52+

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/a68d6d9b87d0

Comment 28

[Charles]

53.0b6 appears to fix the problem for me.  Thanks, all, for the rapid response.

Comment 29

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 53.0a1 (2016-11-16) (64-bit) on Ubuntu 16.04 LTS 64 Bit!

The fix's fix is verified with latest Beta 54.0b12 and latest Nightly 55.0a1 !

Build ID 	: 20170529095024
User Agent 	: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0

Build ID 	: 20170531100318
User Agent 	: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

Comment 30

[Mahfuza Humayra Mohona]

I have reproduced this bug with Nightly 53.0a1 (2016-11-16) on Windows 10 LTS 64 Bit!

The bug's fix is verified with latest Beta 54.0b13 and latest Nightly 55.0a1 !

Build ID 	:20170601133324
User Agent 	:Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0

Build ID 	:20170603030204
User Agent 	:Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
 
[bugday-20170531]