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)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 55
People
(Reporter: dqeswn, Assigned: KWierso, Mentored)
References
Details
(Keywords: privacy, Whiteboard: [fxsearch])
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-review-board-request
|
mak
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
|
Details |
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
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Component: Search → Location Bar
Ever confirmed: true
Priority: -- → P3
Whiteboard: [fxsearch]
Comment 1•8 years 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•8 years ago
|
||
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•8 years ago
|
Attachment #8835129 -
Attachment is patch: true
Comment 3•8 years 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•8 years ago
|
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.
Comment 7•8 years 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•8 years ago
|
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).
Updated•8 years ago
|
Priority: P2 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•8 years 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•8 years 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•8 years 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•8 years ago
|
Assignee: mak77 → wkocher
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Attachment #8835129 -
Attachment is obsolete: true
Comment 13•8 years 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•8 years 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•8 years 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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee | ||
Comment 17•8 years 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•8 years 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•8 years ago
|
||
Patch applies cleanly to aurora and beta (and release/esr52, if we want to go that far).
Assignee | ||
Comment 20•8 years 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?
Comment 21•8 years ago
|
||
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 22•8 years ago
|
||
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+
Comment 23•8 years ago
|
||
bugherder uplift |
Comment 24•8 years ago
|
||
bugherder uplift |
Comment 25•8 years ago
|
||
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)
Updated•8 years ago
|
tracking-firefox-esr52:
--- → 53+
Comment 26•8 years ago
|
||
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+
Comment 27•8 years ago
|
||
bugherder uplift |
Comment 28•8 years ago
|
||
53.0b6 appears to fix the problem for me. Thanks, all, for the rapid response.
Updated•8 years ago
|
Flags: needinfo?(andrei.vaida)
Comment 29•8 years ago
|
||
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]
Comment 30•7 years ago
|
||
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.
Description
•