690307 - localhost URLs with spaces are redirected to search when opened from history drop-down

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Pro]

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

Steps to reproduce:

First you need to have in history an URL from localhost containing a space. For example http://localhost/my project. When you start typing 'localhost', this URL pops up in history drop-down, but with browser.urlbar.trimURLs = true (default as of 7) it only has 'localhost/my project' text (without http://).


Actual results:

Clicking the URL in drop-down gets you to default search engine (searching for 'localhost/my project'), as if this text is entered straight into location bar, instead of opening 'http://localhost/my project'.


Expected results:

This behavior is obviously invalid, it should open 'http://localhost/my project' in browser.

Comment 1

[Pro]

It works ok without space or when you add http:// before the aforementioned URL.

Comment 3

[mati8]

FF 8.0 still has this bug.
It is really easy to resolve.
Why it is ignored?
This is pretty ennoying for programmers (they use localhost adress).
FF has become famous beacause programmers and webdesigners was telling their clients that they shoud switch from IE to FF because it is better (programmers and webdesigner has been doing it because they were annoyed by some acting of IE causing that well coded page was not properly rendered by IE).
And now You do not resolve this very annoying bug for programmers, webdesigner.
When programmers and webdesigner will have stopped to like FF they will start to recommend their clients different product and success of FF will be damaged.

Comment 4

[Tim (fmdeveloper)]

Duplicate of Bug 438399?

Comment 6

[Pro]

Actually it's not a duplicate. I'm ok with the behavior described in the bug #438399. But I'm talking about selecting the address from the history. browser.urlbar.trimURLs should not affect the these urls. When address is selected from history drop-down browser should immediately visit it. We can't have keyworded searches in history, only urls go there.

Comment 7

[Edward]

I can confirm this on Firefox 9.0.1/OSX

I can also confirm that this bug is *not* a duplicate of bug 438399 nor is bug 696801 a duplicate of this bug.

Comment 8

[Filipus Klutiero]

Bug 694184 shows a different scenario caused by the same bug:
Resubmitted the current URL by selecting the address bar and pressing the "Enter" key.

Actual results:

Firefox fell back to performing a Google Search instead of loading the URL.

Comment 9

[Jani Patokallio]

Another confirmation here.  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0.2) Gecko/20100101 Firefox/10.0.2

Also, this does not affect just localhost, but any host on the local network without a FQDN.  Sample URL:

http://pluto/peopleandculture/employeeexperience/Facilities/Our%20Company%20cafe/SitePages/Home.aspx

Works fine when bookmarked, but redirects to search when retrieved from browser history.  Incredibly annoying!  The bug goes away if I replace "pluto" with "pluto.domain.net".

And wWhat's with setting this back to "UNCONFIRMED"?  It's trivially replicated, four people have already said they're all getting this, and now I'm the fifth!

Comment 15

[Karl Tomlinson (:karlt)]

The URL should not be trimmed if loading the trimmed version does not load the correct page.

Comment 21

[:Gijs (he/him)]

[Tracking Requested - why for this release]: This got worse with 33 (see below), and we should try to fix it for 34.

Marco, considering what I saw of protocol-less URIs in bug 1048857, is there any chance this bug is caused by us loading the "URI" without the protocol for http URIs, and it therefore going through the URI fixup service, which makes the search happen if the input has spaces? Can we just always prefix http:// here if the URI we're loading from the dropdown has no protocol?

NB: people are now seeing this for other local URLs (ie things that are not "localhost" but nevertheless do not have dots because they get looked up in the local domain) because of our fixes for searching for single words. I think we should fix this for 34 if at all possible.

Comment 22

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #21)
> Marco, considering what I saw of protocol-less URIs in bug 1048857, is there
> any chance this bug is caused by us loading the "URI" without the protocol
> for http URIs, and it therefore going through the URI fixup service, which
> makes the search happen if the input has spaces? Can we just always prefix
> http:// here if the URI we're loading from the dropdown has no protocol?

The protocol is only trimmed for autoFill, that was indeed related to bug 1048857, but here people is selecting an entry from the results dropdown, there's no trimming at all in the backend, autocomplete returns the whole http://domain/etc url. If then the binding does some fancy things with the returned value, I'm not sure.  Ideally it should just load it. If we are trimming before loading the page, it's possible there's a bug.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to :Gijs Kruitbosch from comment #21)
> NB: people are now seeing this for other local URLs (ie things that are not
> "localhost" but nevertheless do not have dots because they get looked up in
> the local domain) because of our fixes for searching for single words. I
> think we should fix this for 34 if at all possible.

I don't think this is a case that is likely to be hit by the majority of the Firefox user base. I'm not going to track but would be happy to have a safe fix proposed (soon) for 34. If not 34, we still have time to get the fix into 35. I'm happy to reconsider tracking if you feel strongly about this but keep in mind that tracking really means that relman will push on the bug to get resolved. If you're going to do that anyway, you don't need us to track it.

Comment 24

[Marti Raudsepp]

(In reply to Lawrence Mandel [:lmandel] from comment #23)
> I don't think this is a case that is likely to be hit by the majority of the
> Firefox user base.

Do take into account that leaking URLs of private hosts to search engines is a privacy issue, not just an annoyance.

Comment 25

[Jan Tošovský]

What annoying regression https://bugzilla.mozilla.org/show_bug.cgi?id=1087205
(I don't usually invest time to minor issues, but in this case I am really disappointed so it is worth searching bug database ;-)

Comment 26

[miketosh]

(In reply to Marti Raudsepp from comment #24)
> Do take into account that leaking URLs of private hosts to search engines is
> a privacy issue, not just an annoyance.

+1, sounds like it needs a keyword between "sec-low" and "sec-moderate" 

  "sec-low" (Minor security vulnerabilities such as leaks or spoofs of 
    non-sensitive information. Missing best practice security controls) 

  "sec-moderate" (Vulnerabilities which can provide an attacker additional 
    information or positioning that could be used in combination with other 
    vulnerabilities)

Comment 27

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #22)
> (In reply to :Gijs Kruitbosch from comment #21)
> > Marco, considering what I saw of protocol-less URIs in bug 1048857, is there
> > any chance this bug is caused by us loading the "URI" without the protocol
> > for http URIs, and it therefore going through the URI fixup service, which
> > makes the search happen if the input has spaces? Can we just always prefix
> > http:// here if the URI we're loading from the dropdown has no protocol?
> 
> The protocol is only trimmed for autoFill, that was indeed related to bug
> 1048857, but here people is selecting an entry from the results dropdown,
> there's no trimming at all in the backend, autocomplete returns the whole
> http://domain/etc url. If then the binding does some fancy things with the
> returned value, I'm not sure.  Ideally it should just load it. If we are
> trimming before loading the page, it's possible there's a bug.

It looks like the urlbar binding trims the URL that gets put in the url attribute of the richlistitem..., and when you click/enter on an autocomplete item, that just executes the "someone hit enter" handler for the autocomplete binding, which then puts the trimmed value in the URL bar and navigates to it.

From a quick look it seems the url value also gets used for display; we should probably disentangle the two.

Comment 28

[:Gijs (he/him)]

It looks like line 717-720 are at fault here:

http://hg.mozilla.org/mozilla-central/annotate/0e631971b841/browser/base/content/urlbarBindings.xml#l707


Marco, why shouldn't we just never trim the value as we get it from autocomplete, assuming .textValue is only set through there? :-)

Comment 29

[:Gijs (he/him)]

Alternatively, of course, we could argue that:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#712

needs to be taught to actually do what the comment above it says...

Comment 30

[:Gijs (he/him)]

Attachment: make trimURL not generate URLs that parse back into search queries,

So this is the last option. The downside is that this leaves http:// in the URL bar. The upside is that this leaves http:// in the URL bar, so that if you modify the URL in place without using the autocomplete popup, you don't get the same stupid behaviour.

Comment 31

[:Gijs (he/him)]

Marco M., can you add this? Thanks!

Comment 32

[Marco Mucci [:MarcoM]]

Added to IT 36.2

Comment 33

[Marco Bonardo [:mak]]

Comment on attachment 8515536 [details] [diff] [review]
make trimURL not generate URLs that parse back into search queries,

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

::: browser/base/content/utilityOverlay.js
@@ +716,5 @@
> +  // remove single trailing slash for http/https/ftp URLs
> +  let rv = aURL.replace(/^((?:http|https|ftp):\/\/[^/]+)\/$/, "$1");
> +
> +  // Strip the leading http:// only if the host has at least one '.' or
> +  // is an ipv6 ip:

s/is/looks like/

@@ +718,5 @@
> +
> +  // Strip the leading http:// only if the host has at least one '.' or
> +  // is an ipv6 ip:
> +  let hostMatch = rv.match(/^http:\/\/([^\/]*)(?:\/|$)/);
> +  if (hostMatch && (hostMatch[1].contains('.') || /\[[\da-f:]*\]/.test(hostMatch[1]))) {

please make a temp var for code readability:

let ipv6Regex = /\[[\da-f:]*(%[\w\d]+)?\]/;

this is a bit different than your regex, since it also supports suffix zone ids (afaik [fe01::5:6%eth0] is a valid ipv6, per https://tools.ietf.org/html/rfc6874).

So please add a couple tests to browser_urlbarTrimURLs.js for
http://[fe01::5:6%eth0] (should trim)
http://[fe01::5:6%eth0]:6000 (should trim)

I still have one doubt about the contains('.') though, what about localhost.localdomain, does createFixupURI handle that correctly even if we trim? (I suspect so, but I figured it would be better asking regardless)

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 8515536 [details] [diff] [review]
make trimURL not generate URLs that parse back into search queries,

>+  let hostMatch = rv.match(/^http:\/\/([^\/]*)(?:\/|$)/);

(?:\/|$) is redundant here, as [^\/]* will match everything until the first slash.

>+  if (hostMatch && (hostMatch[1].contains('.')

double quotes

Comment 35

[:Gijs (he/him)]

Per IRC discussion with Marco, not updating for %eth stuff, but applied all the other nits (including Dão's):

remote:   https://hg.mozilla.org/integration/fx-team/rev/e47f1c46f365

Comment 36

[Edward]

(In reply to :Gijs Kruitbosch from comment #35)

The tests from https://hg.mozilla.org/integration/fx-team/rev/e47f1c46f365 are actually not including the issue reported here. So, please add something like
> "http://localhost/a b c"
and
> "http://localhost.localdomain/a b c"
to the tests too.

Comment 37

[:Gijs (he/him)]

Attachment: add more tests for the localhost + spaces case,

Note that the localdomain case can have its protocol removed; that was never broken. I'm not convinced this adds much, but it's easy enough to do so we might as well make sure this doesn't regress.

Comment 38

[Marco Bonardo [:mak]]

Comment on attachment 8517545 [details] [diff] [review]
add more tests for the localhost + spaces case,

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

doesn't add anything to what we have... but doesn't hurt as well. whatever.

Comment 39

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/04bf819653bb

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e47f1c46f365

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04bf819653bb

Comment 42

[:Gijs (he/him)]

Comment on attachment 8515536 [details] [diff] [review]
make trimURL not generate URLs that parse back into search queries,

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: selecting localhost URLs with spaces, or URLs with no "." in the hostname, from the urlbar dropdown will not work
[Describe test coverage new/current, TBPL]: this patch adds an automated test to the already fairly good coverage of the trimURL function which it modifies
[Risks and why]: very low risk; the worst that can happen is that we show the "http" protocol in cases where we didn't before and still don't need to (the patch addresses the issue by showing the protocol when we do need to; it is of course possible that we are now erring too far on the side of caution, but that's a lot better than getting the decision wrong and not going to the location indicated by the user)
[String/UUID change made/needed]: nope

Comment 43

[Andrei Vaida [:avaida]]

Reproduced the initial issue on older nightlies such as  10.0a1 (2011-09-29) and 36.0a1 (2014-10-29), using Windows 7 64-bit.

This issue is now verified fixed on Nightly 36.0a1 (2014-11-09) using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.5. Keeping an eye on this, for when it lands on Aurora and Beta channels.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/07c0fd08235f
https://hg.mozilla.org/releases/mozilla-aurora/rev/443e9311c046

https://hg.mozilla.org/releases/mozilla-beta/rev/ffb4891a237d
https://hg.mozilla.org/releases/mozilla-beta/rev/9ebc7ee50a9c

Comment 45

[Andrei Vaida [:avaida]]

Verified fixed on Firefox 34.0b8 (build1 / 20141110195804) and Aurora 35.0a2 (2014-11-11) as well, using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.5.

Comment 46

[Tero Turtiainen]

Does anybody know straight away does this fix the issue I am having in Firefox 33.1?

I click location bar, select an intranet site from the history. Fictious example: intranet/Something/Somewhere.html (the http:// is not visible). The page loads ok. Then I edit the URL in the location bar and remove Somewhere.html, the location bar now contains "intranet/Something/". I press return to load the page, but the browser does a google search for "intranet/Something".

Comment 47

[:Gijs (he/him)]

(In reply to Tero Turtiainen from comment #46)
> Does anybody know straight away does this fix the issue I am having in
> Firefox 33.1?
> 
> I click location bar, select an intranet site from the history. Fictious
> example: intranet/Something/Somewhere.html (the http:// is not visible). The
> page loads ok. Then I edit the URL in the location bar and remove
> Somewhere.html, the location bar now contains "intranet/Something/". I press
> return to load the page, but the browser does a google search for
> "intranet/Something".

Yes.

Equally, you could set the pref "browser.fixup.domainwhitelist.intranet" (for whatever 'intranet' is in the example URLs) to true using about:config, and that'd make it work in 33.1.