User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release) Build ID: 20140724030201 Steps to reproduce: step 1) use Firefox 31 or above step 2) let auto fill and auto suggestions from bookmarks, etc. enabled or use a clean profile step 3) type any address in the URL bar (e.g: mozilla.org) step 4) move your mouse to the drop down menu with suggestions and move the cursor towards one entry of the list that differs at least slightly from mozilla.org, for example: https://support.mozilla.org/en-US/products/firefox step 5) press enter/return key while neither using arrow keys nor using left mouse button and while the cursor is still over a (slightly) different entry from the URL bar Actual results: Enter/return key opened the address in the drop down list, in this example: https://support.mozilla.org/en-US/products/firefox It behaved exactly like the extension "Enter Selects" (https://addons.mozilla.org/en-US/firefox/addon/enter-selects/) Expected results: Enter/return key should have opened the address in the URL bar, in this example: mozilla.org (redirected to https://www.mozilla.org/en-US/ for EN-US localizations) from the official description (https://support.mozilla.org/en-US/kb/awesome-bar-find-your-bookmarks-history-and-tabs): "How to use the autocomplete list Just start typing in the location bar and the autocomplete drop-down will show matching sites from your browsing history, as well as sites you've bookmarked or tagged. Matched terms are highlighted, making the list of results easy to scan. When you see the site you want, just click on it or use the up and down arrows on your keyboard to highlight it and then press EnterReturn. " and "URL autocomplete Firefox will also complete URLs of websites that you've been to before. For example, if you type "aw" Firefox may autocomplete "awesomefoundation.org" if you've visited that site in the past. Pressing EnterReturn in this case would take you directly to that address. " To sum it up: enter/return doesn't open the address in the URL bar when the cursor is placed on a different entry from the drop down list of suggestions.

To complete this report, I will add below the text of the original bug I reported (#1071046), which has been marked as a duplicate of this bug. This is, I think, even more representative of the bug, because when the validation is done, the drop-down list is closed: ``Type some letters in the address bar. The most frequently visited site address appears, as well as a drop-down list. Move your mouse over a site in the list, then click in the address bar. The drop-down list disappears, and the address in the bar hasn't changed. Validate using enter. Guess which site is loaded? Not the one whose address was displayed in the bar, but the one you highlighted using the mouse.''

Does it happen with previous versions older than FF31?

@Loic: no idea, sorry. But what I can say is that I discovered this bug a few weeks ago, and since that it happens to me really often (a little less now that I have identified it). Since I haven't changed my way of using Firefox, I *guess* it wasn't present before. More info: I just tested it on Linux (Fedora 20, x86_64), and the bug is present on this version too.

I agree: this behavior is really weird : after you clicked in the urlbar for typing an url, your mouse cursor will probably stay next to it, and so when the dropdown menu with the suggestions will appear, your cursor will probably be over one of the menu item. And so, if you are used to press enter after typing your url, really often your browser will go to the hovered suggestion, and not to the url you have just type. It happens to me quite often.

@ Loic: no, everything's been working fine up to and including FF 30, hence why I wrote FF31+ (FF31 and the following versions). You can find and test the difference the best way with FF 30: https://ftp.mozilla.org/pub/mozilla.org/firefox/releases/ http://sourceforge.net/projects/portableapps/files/Mozilla%20Firefox,%20Portable%20Ed./ http://www.filehippo.com/download_firefox/57856/ http://www.oldapps.com/firefox.php?system=Windows_7 @ Tadam: and that's exactly why this bug is so annoying. You have to take care to leave your mouse above and to not move it down if you want to open an address by pressing enter/return without disabling the drop down suggestions all along.

Why is this bug not flagged as a regression since FF31?

I didn't find anything resembling the word "regression" in the options above,especially not under (Project/Tracking)Flags, but under "Keywords". Did you mean that? If yes, I'm not sure whether "regression" or "regressionwindow-wanted" would be the right one, but I'll change it to "regression" for now.

Also since it's apparently present on Linux, too, I've changed the hardware and OS to "All", because I don't believe it's hardware and/or OS related, as I've also tested it with clean installs, including complete deletion of the Mozilla folders in %localappdata% and %appdata% (don't worry, I've got backups for the latter), on Windows 7 and 8.1 and each time, this bug appears the moment you use or update to a version > 30.X, even with forked browsers such as Waterfox or Cyberfox.

I can confirm that this is still an issue in 34beta1. This needs to be considered a critical bug due to the security risk involved, with users automatically going to a website while unaware of the true target site.

It's still an issue in latest Nightly 36.0a1 (2014-10-17)...

Gavin, considering the number of dupes and the fact that this is a fairly recent regression, I'd really like to get this into the prioritized backlog if that is at all possible (I need to focus on some other bugs at least until the end of this iteration).

@:Gijs, this is regressed by bug 754265

Confirmed: good=2014-04-18 bad=2014-04-19 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7fe3ee0cf8be&tochange=582b2d81ebe1

This was added for this iteration.

anything new on this?

Marco (M), taking this as well... I poked at this yesterday. As best I can tell, the reason this happens is that with the normal autocomplete binding, you can combine mouse and keyboard input. So if you go to e.g. this testcase: https://bug467442.bugzilla.mozilla.org/attachment.cgi?id=350865 and click in one of the boxes to have the autocomplete list show up, mouse over an entry, and then hit enter, the text of the entry gets stuck in the box... and that's it. I don't think we want to break that functionality in the general autocomplete case. The basic request here is that the URL bar should behave differently. In fact, it could/should *never* need to put the value of the selected item in the field when enter is pressed, because if the selected item in the autocomplete list is manipulated with the keyboard, it is always prefilled in the location bar already, making it unnecessary to further update the value when enter is pressed. Because of the binding inheritance, I don't know how easy this will be to implement in a clean way, but it should be possible, at least... Marco (B), does this sound right to you?

Added to IT 37.1

(In reply to :Gijs Kruitbosch from comment #27) > Marco (B), does this sound right to you? I think you are correct in general, but let's not forget formfill uses the same code as the urlbar, so I'd not be too much surprised if it broke in both places. The first thing to do would be to check that, cause if we also regressed formfill, this is a quite major bug. The only thing to notice is that this regressed due to a code change, for sure something in nsAutoCompleteController here (https://hg.mozilla.org/mozilla-central/rev/669dff46362f) so it's not that we must design new interaction or such, we "just" have to fix a code regression. I suspect the problem is here https://hg.mozilla.org/mozilla-central/rev/669dff46362f#l2.133 the !finalValue.Equals(inputValue) is intended to replace the current value with the "final" value, but it should do only if the selection is "real". For example if you type something and move your mouse over other entries, you see that the input field is not updated, but if you use the arrow keys, you see that it gets updated (due to completeSelectedIndex, that acts only on keyboard navigation). in both cases there is a selection but the behavior differs. The thing the last check is trying to handle there, is that even if completeSelectedIndex is true, the currently inserted value may not be the final value, but the check is very likely not strict enough. So maybe we should also fetch the current popup entry value GetResultValueAt(selectedIndex, false, currentEntryValue); and replace the last check with (currentEnteryValue.Equals(inputValue) && !finalValue.Equals(inputValue)) This is slightly less efficient though (2 string comparisons) Or we should find a more efficient way to tell if the selection is "real".

Comment on attachment 8531362 [details] [diff] [review] fix mouseover vs. enter issue in the urlbar, Review of attachment 8531362 [details] [diff] [review]: ----------------------------------------------------------------- This solution sounds simple and promising, it appears to also fix bug 1108186 (I was expecting that since looks like being the same). I'd like to have a generic toolkit/autocomplete xpcshell test with a mock search provider and using API calls instead of mouse/keyboard interactions, in addition to this browser specific test. ::: browser/base/content/test/general/browser_urlbarEnterAfterMouseOver.js @@ +36,5 @@ > + > + let initiallySelected = gURLBar.popup.richlistbox.selectedIndex; > + > + info("Key Down to select the next item"); > + EventUtils.synthesizeKey("VK_DOWN", {}); Please ensure your test works reliably for both the 2 autocomplete providers we have, the legacy one (nsPlacesAutocomplete + urlinlinecomplete) and the new one (unifiedComplete)... what is in use depends on the browser.urlbar.unifiedcomplete pref, the test may have to behave differently based on that, or it could opt out for legacy code if making it behave correctly in both cases is complex. Since this is a bug in toolkit, it might be better to also create a more general test into toolkit/components/autocomplete/tests, it couldbe an xpcshell-test with a mock popup object returning what we expect to trigger the bug. It will also need a mock search that provides a result with a different finalCompleteValue. You don't actually need to simulate mouse/keyboard there but just call the appropriate autocomplete APIs. See for example: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/tests/unit/test_finalCompleteValue.js http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/tests/unit/test_finalDefaultCompleteValue.js http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/tests/unit/test_popupSelectionVsDefaultCompleteValue.js Globally, I'd be fine having both tests: one xpcshell in toolkit and the browser-chrome in browser. @@ +55,5 @@ > + yield Promise.all([autocompletePopupHidden, openedExpectedPage]); > + > + gBrowser.removeCurrentTab(); > + > + yield promiseWaitForCondition(() => gBrowser.tabs.length == tabCount); Is this really needed? I think the browser-chrome harness is robust against this, indeed it would fail stating a tab was left open if that wouldn't be the case. ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +1257,5 @@ > GetResultValueAt(selectedIndex, true, finalValue); > input->GetTextValue(inputValue); > if (!completeSelection || aIsPopupSelection || > + (completeSelection && mCompletedSelectionIndex == selectedIndex && > + !finalValue.Equals(inputValue))) { at this point completeSelection is surely true, otherwise we'd been handled by the first !completeSelection check...

This has an extra test. Note that I've refined the implementation so that it uses the final complete value of the keyboard-selected item if present, and does nothing otherwise. This distinction is tested in the xpcshell test. At least locally, all the other toolkit autocomplete tests still pass.

remote: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=020e2c223698

Comment on attachment 8534756 [details] [diff] [review] fix mouseover vs. enter issue in the urlbar, Review of attachment 8534756 [details] [diff] [review]: ----------------------------------------------------------------- Crossing fingers! ::: browser/base/content/test/general/browser_urlbarEnterAfterMouseOver.js @@ +55,5 @@ > + "Value in the URL bar should be updated by keyboard selection"); > + > + // Would love to use a synthetic mousemove event here, but that doesn't seem to do anything. > + // EventUtils.synthesizeMouseAtCenter(results[3], {type: "mousemove"}); > + gURLBar.popup.richlistbox.selectedIndex = 3; might we sanity check this index is different from the keyboard selected index? Just to be sure. ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +1256,4 @@ > value = finalValue; > + } else if (mCompletedSelectionIndex != -1) { > + // If completeselectedindex is true, or > + // EnterMatch was called via other means, for instance pressing Enter, I think this should read more like: "If completeselectedindex is true and EnterMatch was not invoked by mouse clicking on a match (for example the user pressed Enter), ..." @@ +1258,5 @@ > + // If completeselectedindex is true, or > + // EnterMatch was called via other means, for instance pressing Enter, > + // don't fill in the value as it will have already been filled in as > + // needed, unless the final complete value of the keyboard-selected > + // item in the list differs . "unless the selected match has a final complete value than differs from the user-facing value." @@ +1267,5 @@ > + value = finalValue; > + } > + // Note that if the user opens the popup, mouses over entries without > + // ever selecting one with the keyboard, and then hits enter, none of > + // the above cases will be hit. I think it's worth clarifying the reason, something like "since mouseover doesn't activate completeselectedindex and thus mCompletedSelectionIndex would not be set." I leave the exact wording to you, since you are far more English-skilled than me ::: toolkit/components/autocomplete/nsAutoCompleteController.h @@ +150,5 @@ > uint32_t mSearchesOngoing; > uint32_t mSearchesFailed; > bool mFirstSearchResult; > uint32_t mImmediateSearchesCount; > + int32_t mCompletedSelectionIndex; I know the documentation is lacking here, but would be nice to document this property (what it tracks and when) It should have been done for any property added in the past ideally, let's at least try to be nicer in future. ::: toolkit/components/autocomplete/tests/unit/test_finalCompleteValueSelectedIndex.js @@ +1,3 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ please remove header, tests are PD by default if no header exists. I think I wrote the test you were inspired by and I'm fine with PD. @@ +7,5 @@ > + this._finalCompleteValues = aResultValues.map(x => x[1]); > +} > +AutoCompleteResult.prototype = Object.create(AutoCompleteResultBase.prototype); > + > +var selectByWasCalled = false; s/var/let/ @@ +12,5 @@ > +function AutoCompleteInput(aSearches) { > + this.searches = aSearches; > + this.popup.selectedIndex = 0; > + this.popup.selectBy = function(reverse, page) { > + do_check_eq(selectByWasCalled, false); nit: Could you please use Assert.jsm mehtods in the test, instead of the old do_ helpers? @@ +81,5 @@ > + registerAutoCompleteSearch(search); > + > + let controller = Cc["@mozilla.org/autocomplete/controller;1"]. > + getService(Ci.nsIAutoCompleteController); > + some trailing spaces in the 2 rows above

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #35) > ::: browser/base/content/test/general/browser_urlbarEnterAfterMouseOver.js > @@ +55,5 @@ > > + "Value in the URL bar should be updated by keyboard selection"); > > + > > + // Would love to use a synthetic mousemove event here, but that doesn't seem to do anything. > > + // EventUtils.synthesizeMouseAtCenter(results[3], {type: "mousemove"}); > > + gURLBar.popup.richlistbox.selectedIndex = 3; > > might we sanity check this index is different from the keyboard selected > index? Just to be sure. The keyboard selected index isn't exposed, so that's not currently possible. Do you think we should be exposing it? That's possible, but I'd have to update the idl and rev it, which is not upliftable beyond aurora. I'm also skeptical that it's of any use beyond just tests, and whether that is worth it... Can you clarify what you would prefer?

hm, I thought we could just store gURLBar.popup.richlistbox.selectedIndex BEFORE setting it, and check we are not setting it to the same value it already has.

w/ nits and extra assertion to check the selectedIndex: remote: https://hg.mozilla.org/integration/fx-team/rev/9b446ed418fe

thx for the fix! Is there a way to get this fix out more quickly? It's just a small bug fix; and if it doesn't depend on special code, then it shouldn't be hard to get this out a few month earlier (FF 35 maybe).

(In reply to felix.bau from comment #40) > thx for the fix! > Is there a way to get this fix out more quickly? > It's just a small bug fix; and if it doesn't depend on special code, then it > shouldn't be hard to get this out a few month earlier (FF 35 maybe). The patch and the code affected here are really not just a "small bugfix". The autocomplete code is used in a lot of different places, and it's pretty fragile. IMO it's too early to say (the fix isn't even in today's nightly yet, from the looks of it!) if/when we can uplift this and to be sure we didn't break anything else. Please be patient.

Thanks for the fix, good job! :)

(In reply to :Gijs Kruitbosch from comment #41) > IMO it's too early to say (the fix isn't even in today's nightly > yet, from the looks of it!) if/when we can uplift this and to be sure we > didn't break anything else. Please be patient. While I certainly understand the caution here, I would also urge some degree of expedience in getting this tested and pushed out. This is a very frustrating bug in what is one of the most common UI interactions (entering a URL in the addressbar). It also has potential security implications -- an attacker could perhaps design a set of history items such that the user could be routed a page other than the one the user intended via this bug. That doesn't mean it should be rushed out without any testing or caution, of course, but I think it should definitely happen sooner rather than later.

I had thought of this as well, but came to the conclusion that there are no additional security implications here. First of all the described case is very unlikely to happen. Second you need js activated to insert entries into the browsers history(and that fairly often to make it show up in the URL bar). So you could simply change the URL of the window instead by using js (document.window...) Third there are other possibilities that don't require js to be activated and are therefore even more successful, like: -adding a hidden frame -adding a meta tag to the html header that tells the browser to switch to a different page (forward)

I had thought of this as well, but came to the conclusion that there are no additional security implications here. First of all the described case is very unlikely to happen. Second you need js activated to insert entries into the browsers history(and that fairly often to make it show up in the URL bar). So you could simply change the URL of the window instead by using js (document.window...) Third there are other possibilities that don't require js to be activated and are therefore even more successful, like: -adding a hidden frame -adding a meta tag to the html header that tells the browser to switch to a different page (forward)

(In reply to felix.bau from comment #46) > First of all the described case is very unlikely to happen. What specifically is the case you're referring to? This bug happens to me almost every time I use the addressbar. > Second you need js activated to insert entries into the browsers history(and that fairly often to make it show up in the URL bar). So you could simply change the URL of the window instead by using js (document.window...) If you were going for an immediate, overt attack: yes. But if you were trying to be more subtle, so as to reduce the user's likelihood of noticing the attack, this is entirely plausible.

the described case ^= the user types an adress in the url bar that matches the attack url (you would have to opened that site many many times already why shouldn't the attacker run the attack on the same side instead?

Like I said, if an attacker was trying to be subtle, there might be reason to manipulate the bug in this manner. It's speculation to be sure, but I don't think "we shouldn't fix this because an attacker wouldn't want to use it" is much of an argument. And again, I'm not suggesting this needs to be rushed out as an urgent fix, but I think it ought to go out sooner rather than later.

I was able to reproduce the issue with Firefox 32.0.3 (20140923175406), using the steps from Comment 0 and Comment 7. This is now verified fixed on Nightly 37.0a1 (2014-12-21) using Windows 7 64-bit, Ubuntu 12.04 LTS 32-bit and Mac OS X 10.9.5. Since this fix might affect a few other things (see Comment 41), further regression testing will be performed on this area to ensure everything works as expected.

Comment on attachment 8534756 [details] [diff] [review] fix mouseover vs. enter issue in the urlbar, Approval Request Comment [Feature/regressing bug #]: bug 1067634 [User impact if declined]: confusing behaviour of the URL bar when using both the mouse and the keyboard to select an item from the autocomplete dropdown [Describe test coverage new/current, TBPL]: has several automated tests [Risks and why]: medium-low; has tests and was thoroughly reviewed/debated in-person between Marco and me; but on the downside, this is autocomplete code. :-( [String/UUID change made/needed]: no

(In reply to :Gijs Kruitbosch from comment #51) > Comment on attachment 8534756 [details] [diff] [review] > fix mouseover vs. enter issue in the urlbar, > > Approval Request Comment > [Feature/regressing bug #]: bug 1067634 copy/paste fail, I meant bug 754265.

Firefox 37 is a long way away for a bug that continues to attract dupes and, frankly, makes Firefox often-infuriating to use. I'm not familiar with the milestone planning but can this make it into 36 if not 35? (Personally I'd like to see this go out with a 34 point release.)

It's fixed in FF36, but it's too late for FF35 which will be released early in January.

(In reply to Craig K from comment #56) > Firefox 37 is a long way away for a bug that continues to attract dupes and, > frankly, makes Firefox often-infuriating to use. I'm not familiar with the > milestone planning but can this make it into 36 if not 35? (Personally I'd > like to see this go out with a 34 point release.) As Loic said, it was uplifted to 36. 35 is not feasible, as the release candidate goes to build today and this change needs more baking than 1 day. Considering that you're using bugzilla, please consider at least using beta. This would mean you will see the fix for this issue in 2 weeks' time. Assuming you file bugs based on what you encounter on beta, that also helps us find (and fix) issues sooner, rather than after a new version has been released. As a rule, we do not create point releases (and certainly not 1 week before a new major release) except for serious security or stability issues, which this isn't.

What about when a user types in starts typing in https://www in the 'awesomebar' and is given results that include the nonsecure "http://"? If people consider this bug as a security flaw because it directs them to where they don't actually intend to go, wouldn't being offered non secure sites when the user obviously wants a secure site be considered a flaw? And yet I am told that it is intended behavior. I don't understand firefox anymore.

(In reply to illumilor.e from comment #59) > What about when a user types in starts typing in https://www in the > 'awesomebar' and is given results that include the nonsecure "http://"? This is a very different bug, please file it separately (and look for dupes, because I'm pretty sure this is on file).

(In reply to Gijs Kruitbosch from comment #60) > (In reply to illumilor.e from comment #59) > > What about when a user types in starts typing in https://www in the > > 'awesomebar' and is given results that include the nonsecure "http://"? > > This is a very different bug, please file it separately (and look for dupes, > because I'm pretty sure this is on file). How is this a different bug? If I had both secure and non-secure candidates in the awesomebar list for the same domain, and accidentally moused-over the nonsecure version, I would be routed to it.

" (and look for dupes, because I'm pretty sure this is on file)." It is, and the devs have said that it is intended behavior. I was asking why it is intended behavior, because I do not understand. It seems like a security risk to me.

Related: bug 1091958

(In reply to illumilor.e from comment #62) > " (and look for dupes, because I'm pretty sure this is on file)." > > It is, and the devs have said that it is intended behavior. I was asking why > it is intended behavior, because I do not understand. It seems like a > security risk to me. Then please do that on bug 1091958. It's confusing to spread the discussion over two different bugs.

I'm sorry for the last spreading of this discussion, just wanted to add my bits and then I'm quiet :) : this is only a security flow if the user is being attacked MITM so that the Server you are connecting to might not be able to forward you too https:// instead since many sites support https:// now although it was only http:// a few years ago, you could clear your history once an the problem will be gone and btw.: A lot of people that I know still type: www.example.com or http://www.example.com or just example.com and so they will land on http anyways if they type the stuff in manually other persons are using google instead of typing the url directly a workaround to get https for the most used sites even if your browser is trying to open http at first is still: HTTPSEverywhere so if you are concerned, then you could use it ;) about another comment above: "Considering that you're using bugzilla, please consider at least using beta. This would mean you will see the fix for this issue in 2 weeks' time. Assuming you file bugs based on what you encounter on beta, that also helps us find (and fix) issues sooner, rather than after a new version has been released." ahmm... you know that this issue is half a year old already, I don't see your "got fixed in 2 weeks" this is no critic, pls don't take this personal, but such a comment might be useless if bugs only get noticed if lots of people notice them (for a longer amount of time), which only happens if the bug is already part of a release I guess this bug would have survived beta even if more people would have commented on it because the devs didn't notice it there where a dozen of reports that got all marked as duplicates -> no indicator that this might be a usability issue that a lot of people were noticing!? you still have the estimated number of unreported cases into account which is likely way higher so yeah... I'm sorry for my bashing I understand that it if there were other things that the devs were prioritizing I just hope this doesn't happen to often Now I'm waiting for FF 36 (thx for uplifting it) And since you asked for it I'm gonna update to the beta and report bugs I'm encountering more often Have a nice day :)

(In reply to felix.bau from comment #65) > ahmm... you know that this issue is half a year old already, I don't see > your "got fixed in 2 weeks" I didn't say bugs get fixed in 2 weeks, I said *this bug* will be fixed on beta in 2 weeks' time - that's when the first beta release after the merge of 36 to beta (which happens 1 week from now) is normally released.

I'm sorry for not reading your post correctly. Maybe I skipped a bit too much, while rereading it ^^

Verified fixed on: * Nightly 37.0a1 (2015-01-12), * Nightly 38.0a1 (2015-01-13), * Aurora 36.0a2 (2015-01-11), * Aurora 37.0a2 (2015-01-13), * Beta 36.0b1 (20150112201205), as well, using Ubuntu 12.04 LTS 32-bit, Mac OS X 10.9.5 and Windows 7 32-bit with the steps from Comment 0 and Comment 7.

Firefox 48 has the same problem as described in the OP again. Is this change intended? If yes, how do I get the old behavior back?

Please, file a new bug report instead of tweaking old fixed bug.

Okay done and sorry for the trouble: https://bugzilla.mozilla.org/show_bug.cgi?id=1292310