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

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: avaida, Assigned: Gijs)

Tracking

34 Branch
mozilla34
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox33 verified, firefox34 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Flags: firefox-backlog+
(Assignee)

Comment 1

5 years ago
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.
Attachment #8462094 - Flags: review?(bzbarsky)
(Assignee)

Updated

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

Comment 2

5 years ago
Jenn, can you add this one, too?
Iteration: --- → 34.1
Points: --- → 2
QA Whiteboard: [qa+]
Flags: needinfo?(jchaulk)
(Assignee)

Comment 3

5 years ago
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. :-(
Added to Iteration 34.1
Flags: needinfo?(jchaulk)
(Assignee)

Comment 5

5 years ago
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?
Attachment #8462094 - Flags: review?(bzbarsky)
Flags: needinfo?(bzbarsky)
You could do that and uplift to Aurora?
Flags: needinfo?(bzbarsky)
(Reporter)

Updated

5 years ago
QA Contact: andrei.vaida
(Assignee)

Comment 7

5 years ago
OK. I'm much happier with this than the previous patch. :-)
Attachment #8462489 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #8462094 - Attachment is obsolete: true
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.
Attachment #8462489 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 9

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

Comment 10

5 years ago
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).
Attachment #8466979 - Flags: review?(jaws)
(Assignee)

Updated

5 years ago
Attachment #8462489 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
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... :-)
Attachment #8466979 - Flags: review?(mak77)
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
Attachment #8466979 - Flags: review?(mak77)
Attachment #8466979 - Flags: review?(jaws)
Attachment #8466979 - Flags: review+
Iteration: 34.1 → 34.2
(Assignee)

Comment 13

5 years ago
(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
Iteration: 34.2 → 34.1
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

5 years ago
Iteration: 34.1 → 34.2
https://hg.mozilla.org/mozilla-central/rev/782db4943ae5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla34
(Reporter)

Comment 15

5 years ago
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.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 16

5 years ago
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.
Attachment #8466979 - Flags: review+
Attachment #8466979 - Flags: approval-mozilla-aurora?
Depends on: 1049545
Attachment #8466979 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 18

5 years ago
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.
QA Whiteboard: [qa+] → [qa!]
Keywords: qawanted, verifyme
You need to log in before you can comment on or make changes to this bug.