Closed Bug 1318070 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: dqeswn, Assigned: KWierso, Mentored)

References

Details

(Keywords: privacy, Whiteboard: [fxsearch])

Attachments

(1 file, 1 obsolete file)

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
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
Status: UNCONFIRMED → NEW
Component: Search → Location Bar
Ever confirmed: true
Priority: -- → P3
Whiteboard: [fxsearch]
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.
Attached patch proposed fix (obsolete) (deleted) — Splinter Review
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)
Attachment #8835129 - Attachment is patch: true
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)
Mentor: mak77
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.
Mark, do you plan to complete the work for your patch?
Flags: needinfo?(mcs)
Keywords: privacy
Priority: P3 → P2
(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)
Assignee: nobody → mak77
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).
Priority: P2 → P1
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.
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 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+
Assignee: mak77 → wkocher
Status: NEW → ASSIGNED
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
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
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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?
As far as I know, all supported versions except esr45 are affected.
Patch applies cleanly to aurora and beta (and release/esr52, if we want to go that far).
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+
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)
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+
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.

Attachment

General

Created:
Updated:
Size: