Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Location Bar
P1
normal
RESOLVED FIXED
8 months ago
2 months ago

People

(Reporter: avada, Assigned: KWierso, Mentored)

Tracking

({privacy})

Trunk
Firefox 55
privacy
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 wontfix, firefox53 fixed, firefox54 fixed, firefox55 fixed, firefox-esr45 unaffected, firefox-esr5253+ fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 months ago
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
(Reporter)

Updated

8 months ago
Summary: keyword.enabled is half-broken, it's half enabled even whet it's set to false → keyword.enabled is half-broken, it's half enabled even when it's set to false

Updated

8 months ago
Status: UNCONFIRMED → NEW
Component: Search → Location Bar
Ever confirmed: true
Priority: -- → P3
Whiteboard: [fxsearch]

Comment 1

6 months ago
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

5 months ago
Created attachment 8835129 [details] [diff] [review]
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).
Attachment #8835129 - Flags: review?(mak77)

Updated

5 months ago
Attachment #8835129 - Attachment is patch: true

Comment 3

5 months ago
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/
Attachment #8835129 - Flags: review?(mak77)

Updated

5 months ago
Mentor: mak77@bonardo.net

Comment 4

4 months ago
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

4 months ago
Mark, do you plan to complete the work for your patch?
Flags: needinfo?(mcs)

Updated

4 months ago
Duplicate of this bug: 1346505

Updated

4 months ago
Keywords: privacy
Priority: P3 → P2

Comment 7

4 months ago
(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!
Flags: needinfo?(mcs)

Updated

4 months ago
Assignee: nobody → mak77

Comment 8

4 months ago
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).

Updated

4 months ago
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 10

4 months ago
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

4 months ago
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

4 months ago
mozreview-review
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!
Attachment #8848844 - Flags: review?(mak77) → review+

Updated

4 months ago
Assignee: mak77 → wkocher
Status: NEW → ASSIGNED
(Assignee)

Updated

4 months ago
Attachment #8835129 - Attachment is obsolete: true

Comment 13

4 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/13b38cd29a4a
Make sure multi-word queries are rejected when keyword.enabled is false r=mak

Comment 14

4 months ago
Pushed by kwierso@gmail.com:
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
(Assignee)

Comment 15

4 months ago
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
(Assignee)

Comment 16

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/784c281fcc1e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Comment 17

4 months ago
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.
Attachment #8848844 - Flags: approval-mozilla-beta?
Attachment #8848844 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

4 months ago
As far as I know, all supported versions except esr45 are affected.
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → affected
(Assignee)

Comment 19

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

Comment 20

4 months ago
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.
Attachment #8848844 - Flags: approval-mozilla-esr52?
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
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+.
Attachment #8848844 - Flags: approval-mozilla-beta?
Attachment #8848844 - Flags: approval-mozilla-beta+
Attachment #8848844 - Flags: approval-mozilla-aurora?
Attachment #8848844 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca1fbc0c4c17
status-firefox54: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/2bbf316c5d61
status-firefox53: affected → fixed
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.
Flags: qe-verify-
Flags: needinfo?(brindusa.tot)
Flags: needinfo?(andrei.vaida)
status-firefox52: affected → wontfix
tracking-firefox-esr52: --- → 53+
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+
Attachment #8848844 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
https://hg.mozilla.org/releases/mozilla-esr52/rev/a68d6d9b87d0
status-firefox-esr52: affected → fixed

Comment 28

4 months ago
53.0b6 appears to fix the problem for me.  Thanks, all, for the rapid response.
Flags: needinfo?(andrei.vaida)
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
QA Whiteboard: [bugday-20170531]
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]
You need to log in before you can comment on or make changes to this bug.