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

VERIFIED FIXED in Firefox 34

Status

()

Firefox
Location Bar
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Pro, Assigned: Gijs)

Tracking

({regression})

7 Branch
Firefox 36
regression
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox33 wontfix, firefox34 verified, firefox35 verified, firefox36 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Component: General → Location Bar
(Reporter)

Comment 1

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

Updated

6 years ago
Duplicate of this bug: 696801

Comment 3

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

6 years ago
Duplicate of Bug 438399?

Updated

6 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 438399
(Reporter)

Comment 6

6 years ago
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.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---

Comment 7

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

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

6 years ago
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!

Updated

5 years ago
Version: 7 Branch → 10 Branch
Duplicate of this bug: 797960
Duplicate of this bug: 693530
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 779485
Duplicate of this bug: 768564
Duplicate of this bug: 798871
The URL should not be trimmed if loading the trimmed version does not load the correct page.
Blocks: 665580
Keywords: regression
Version: 10 Branch → 7 Branch
Duplicate of this bug: 839686
Duplicate of this bug: 838544

Updated

3 years ago
Duplicate of this bug: 1030003
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1014969
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1087205
(Assignee)

Comment 21

3 years ago
[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.
tracking-firefox34: --- → ?
Flags: qe-verify+
Flags: needinfo?(mak77)
Flags: in-testsuite?
Flags: firefox-backlog+
(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.
Flags: needinfo?(mak77)
(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.
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
tracking-firefox34: ? → ---

Comment 24

3 years ago
(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

3 years ago
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

3 years ago
(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)
(Assignee)

Comment 27

3 years ago
(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.
(Assignee)

Comment 28

3 years ago
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? :-)
Flags: needinfo?(mak77)
(Assignee)

Comment 29

3 years ago
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...
(Assignee)

Comment 30

3 years ago
Created attachment 8515536 [details] [diff] [review]
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.
Attachment #8515536 - Flags: review?(mak77)
(Assignee)

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 31

3 years ago
Marco M., can you add this? Thanks!
Points: --- → 5
Flags: needinfo?(mak77) → needinfo?(mmucci)
(Assignee)

Updated

3 years ago
status-firefox33: affected → wontfix
OS: Linux → All
Hardware: x86_64 → All
Added to IT 36.2
Iteration: --- → 36.2
Flags: needinfo?(mmucci)
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)
Attachment #8515536 - Flags: review?(mak77) → review+
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
(Assignee)

Updated

3 years ago
Depends on: 1094179
(Assignee)

Comment 35

3 years ago
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
Whiteboard: [fixed-in-fx-team]

Comment 36

3 years ago
(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.
(Assignee)

Comment 37

3 years ago
Created attachment 8517545 [details] [diff] [review]
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.
Attachment #8517545 - Flags: review?(mak77)
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.
Attachment #8517545 - Flags: review?(mak77) → review+
(Assignee)

Comment 39

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/04bf819653bb
https://hg.mozilla.org/mozilla-central/rev/e47f1c46f365
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago3 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
QA Contact: bogdan.maris
QA Contact: bogdan.maris → andrei.vaida
https://hg.mozilla.org/mozilla-central/rev/04bf819653bb
(Assignee)

Updated

3 years ago
status-firefox36: affected → fixed
(Assignee)

Comment 42

3 years ago
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
Attachment #8515536 - Flags: approval-mozilla-beta?
Attachment #8515536 - Flags: approval-mozilla-aurora?
Attachment #8515536 - Flags: approval-mozilla-beta?
Attachment #8515536 - Flags: approval-mozilla-beta+
Attachment #8515536 - Flags: approval-mozilla-aurora?
Attachment #8515536 - Flags: approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
status-firefox36: fixed → verified
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
status-firefox34: affected → fixed
status-firefox35: affected → fixed
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.
status-firefox34: fixed → verified
status-firefox35: fixed → verified

Comment 46

3 years ago
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".
(Assignee)

Comment 47

3 years ago
(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.
You need to log in before you can comment on or make changes to this bug.