1042519 - Characters followed by a single dot as a location bar input point to a server error page

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Andrei Vaida [:avaida]]

Note: this is a follow-up for Bug 693808.

Reproducible on Nightly 34.0a1 (2014-07-22), Build ID: 20140722030201, using:
 * Windows 7 64-bit,
 * Ubuntu 14.04 LTS 32-bit,
 * Mac OS X 10.9.4.

Steps to reproduce:
1. Launch Nightly 34.
2. Bring the Location Bar into focus and type in 'test.' without the apostrophe.
3. Press the <enter> key.

Expected result: 
A google search is performed for the 'test.' keyword (w/o apostrophe).

Actual result: 
The user is redirected to a "Server not found" error page, with www.test. as URL.

Comment 1

[:Gijs (he/him)]

Attachment: test. should result in a keyword lookup instead of an error page,

I removed a bunch of logic that the test mirrored from the implementation by just adding the info to the test - the test was getting hard to maintain. I also noticed I missed a case where the info object was not correctly updated with information about whether or not a dot was present.

Comment 2

[:Gijs (he/him)]

Jenn, can you add this one, too?

Comment 3

[:Gijs (he/him)]

Note that inputHostHasDot is becoming even more fuzzy now, because it's false for the "test." case. Which I am somewhat uneasy about, but I can't think of a better name. :-(

Comment 4

[Marco Mucci [:MarcoM]]

Added to Iteration 34.1

Comment 5

[:Gijs (he/him)]

Comment on attachment 8462094 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

Actually, no, I should hold off on asking for review here.

I think I'm suffering from "let's design an API without any actual consumers" issues. It's not clear to me what consumers generally would want to know about the input here. It approximates "is it even possible for this input to end up approximately being a URI" and/or "did we only need a protocol to fix this up, or did we need to do more, or did we go the whole hog and just keyword-search it?" - but currently only the "fixupUsedKeyword" bool is in use by the browser.js consumer, and that's written from the "what did we do to get this output" perspective, not "what was the input like".

I'm half-tempted to just remove the inputHostHasDot bool (and probably the inputHasProtocol one as well) from the info object altogether, or to rewrite them as "what did we to to get this output (in other words, add "fixupAddedProtocol" and "fixupCreatedAlternativeURI" flags instead).

Which kind of sucks because we just landed this and it's now on Aurora... although I suppose that also means "better sooner than later"...

Thoughts?

Comment 6

[Boris Zbarsky [:bzbarsky]]

You could do that and uplift to Aurora?

Comment 7

[:Gijs (he/him)]

Attachment: test. should result in a keyword lookup instead of an error page,

OK. I'm much happier with this than the previous patch. :-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8462489 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

>+    uint32_t dotCount = aURIString.CountChar('.');

Might be faster for long URIs to RFindChar and then see whether it's found at dotLoc?  Unclear.  I'm probably OK with it either way.

>+            if (dotLoc == aURIString.Length()) {

How could that happen?  Missing a -1?

r=me with the above dealt with.

I didn't read the test changes very carefully.  Let me know if I should.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8462489 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

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

::: docshell/base/nsDefaultURIFixup.cpp
@@ +967,5 @@
>              nsAutoCString pref("browser.fixup.domainwhitelist.");
> +            if (dotLoc == aURIString.Length()) {
> +              pref.Append(Substring(asciiHost, 0, asciiHost.Length() - 1));
> +            } else if (dotLoc == 0) {
> +              pref.Append(StringTail(asciiHost, asciiHost.Length() - 1));

Adding a test for ".localhost" in browser code showed this doesn't actually work: if not doing a search lookup, ".localhost" is not equivalent to "localhost", and the URI fixup code changes "http://.localhost/" to "http://www..localhost/" which is a different host still. So I've removed this "else if" block again so ".localhost" is controlled by a different whitelist pref than "localhost".

However, I've kept the code ~12 lines up so as to do a search query for ".localhost" by default (which it didn't do before). This is what IE does. Chrome corrects to "localhost" and loads that. If someone feels strongly that we should put work into making that syntax work, I think that should be a separate followup bug.

In the meantime, I don't think this change warrants re-requesting review on the core bits... but we need a browser part, so I'll request review for a new patch from a browser peer in a bit.

Comment 10

[:Gijs (he/him)]

Attachment: test. should result in a keyword lookup instead of an error page,

I've adjusted the things bz found in his review, but when testing the length - 1 pref issue, I noticed that the frontend code also needs to be adapted slightly to get the whitelisting right. r?jaws on the browser bits (I added a test for this case).

Comment 11

[:Gijs (he/him)]

Comment on attachment 8466979 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

Marco, seems Jared might not be here... do you have time to look at the browser/ part here? It's quite small... :-)

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8466979 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

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

::: browser/base/content/browser.js
@@ +795,5 @@
>    let hostName = alternativeURI.host;
>    // and the ascii-only host for the pref:
>    let asciiHost = alternativeURI.asciiHost;
> +  // Normalize out the single trailing dot:
> +  if (asciiHost.indexOf('.') == asciiHost.length - 1) {

asciiHost.endsWith('.') (fwiw here you should have used lastIndexOf)

@@ +796,5 @@
>    // and the ascii-only host for the pref:
>    let asciiHost = alternativeURI.asciiHost;
> +  // Normalize out the single trailing dot:
> +  if (asciiHost.indexOf('.') == asciiHost.length - 1) {
> +    asciiHost = asciiHost.substring(0, asciiHost.length - 1);

asciiHost = asciiHost.slice(0, -1);

::: browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.js
@@ +49,5 @@
>  });
>  
> +function get_test_function_for_localhost_with_hostname(hostName) {
> +  return function* test_navigate_single_host() {
> +    let pref = "browser.fixup.domainwhitelist.localhost";

nit: const

Comment 13

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #12)
> Comment on attachment 8466979 [details] [diff] [review]
> test. should result in a keyword lookup instead of an error page,
> 
> Review of attachment 8466979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +795,5 @@
> >    let hostName = alternativeURI.host;
> >    // and the ascii-only host for the pref:
> >    let asciiHost = alternativeURI.asciiHost;
> > +  // Normalize out the single trailing dot:
> > +  if (asciiHost.indexOf('.') == asciiHost.length - 1) {
> 
> asciiHost.endsWith('.') (fwiw here you should have used lastIndexOf)

Commented on why that wasn't an option.

> @@ +796,5 @@
> >    // and the ascii-only host for the pref:
> >    let asciiHost = alternativeURI.asciiHost;
> > +  // Normalize out the single trailing dot:
> > +  if (asciiHost.indexOf('.') == asciiHost.length - 1) {
> > +    asciiHost = asciiHost.substring(0, asciiHost.length - 1);
> 
> asciiHost = asciiHost.slice(0, -1);
> 
> :::
> browser/base/content/test/general/browser_urlbarSearchSingleWordNotification.
> js
> @@ +49,5 @@
> >  });
> >  
> > +function get_test_function_for_localhost_with_hostname(hostName) {
> > +  return function* test_navigate_single_host() {
> > +    let pref = "browser.fixup.domainwhitelist.localhost";
> 
> nit: const

Addressed these.

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

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/782db4943ae5

Comment 15

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 34.0a1 (2014-08-06) using Windows 7 64-bit, Ubuntu 13.04 32-bit and Mac OS X 10.9.4. Leaving the [qa+] keyword in place until this fix reaches Aurora.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8466979 [details] [diff] [review]
test. should result in a keyword lookup instead of an error page,

Approval Request Comment
[Feature/regressing bug #]: 693808
[User impact if declined]: different APIs on Aurora/nightly for urifixup which might confuse add-ons, some user input might not correctly trigger searches
[Describe test coverage new/current, TBPL]: has automated tests, has been verified by QA
[Risks and why]: low-to-medium risk - low because the change shouldn't really affect things and the code is gaining code coverage to the point where I would say it's pretty good now, but medium-ish because this affects a really oft-used feature in the browser (the location bar!) and so... yeah.
[String/UUID change made/needed]: 1 uuid change, should be good for Aurora AIUI.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c111b7cf29ff

Comment 18

[Andrei Vaida [:avaida]]

Verified fixed on Aurora 33.0a2 (2014-08-08) as well, using Windows 7 64-bit, Ubuntu 13.04 32-bit and Mac OS X 10.9.4.