263213 - Don't use I'm Feeling Lucky search when protocol (such as http:// or https://) specified

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Tuukka Tolvanen (sp3000)]

(linux Firefox aviary cvs 20040926)

While the "browse by first search match" feature is useful, it doesn't make
sense to search for strings that obviously are meant as URLs, such as
http://that-box-under-your-desk/pr0n/ or https://that-box-under-your-desk/pr0n/
-- curiously enough, ftp://that-box-under-your-desk/pr0n/ works as expected.

Not doing the I'm Feeling Lucky search for non-resolving urls, where the user
has properly specified the protocol, would fix many of the issues marked as dup
of bug 231720, without introducing typo-guessing heuristics.

To reproduce:
    Type http://that-box-under-your-desk/pr0n/ or
    http://that-box-under-your-desk/pr0n/ into location bar and hit enter
    (assuming that-box-under-your-desk doesn't resolve).
For comparison:
    Type ftp://that-box-under-your-desk/pr0n/ into location bar and hit enter
    (assuming that-box-under-your-desk doesn't resolve).
Actual results:
    Loads first google search result for that-box-under-your-desk
Expected results:
    Same results for http(s) as for ftp: Alert: "that-box-under-your-desk could
    not be found. Please check the name and try again."

Comment 1

[Tuukka Tolvanen (sp3000)]

...and in case the code is all the same as seamonkey IKs, this would be more or
less bug 95390.

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Or dupe to bug 231720.

Comment 3

[Tuukka Tolvanen (sp3000)]

This is expressly not specifically about malformed urls. This is specifically
about recognizing that nobody types http://this-is-a-search-term or
https://this-is-a-search-term meaning "this-is-a-search-term" as a search term. 

( And if they did intend a search, you shouldn't be stripping the "http://" or
"https://" from the search terms anyhow :P )

Comment 4

[CoJaBo]

(In reply to comment #0)
Also reported as bug 272112.

Comment 5

[timeless]

*** Bug 272112 has been marked as a duplicate of this bug. ***

Comment 6

[Ian Grigg]

If any domain is mistyped with https the redirect goes to Paypal.  This breaches
the security model of HTTPS; if a secure HTTPS url is indicated and certificates
are being expected to be checked, then the browser should not make any
adjustments arbitrarily to the URL typed in to URL bar, and/or make changes seen
obvious.

As it was discovered by payments people (Gordon Katz of KatzGlobal.com), and as
everyone in that world is panicing about phishing, I think this could be major.
 It currently it appears mostly embarrassing rather than exploitable.  I can't
quite see how to exploit it but phishers are more persistent than I.

(Comments copied from #289793)

Comment 7

[Robin Monks]

*** Bug 245330 has been marked as a duplicate of this bug. ***

Comment 8

[Robin Monks]

*** Bug 302974 has been marked as a duplicate of this bug. ***

Comment 9

[Daniel Veditz [:dveditz]]

*** Bug 231720 has been marked as a duplicate of this bug. ***

Comment 10

[Daniel Veditz [:dveditz]]

*** Bug 235786 has been marked as a duplicate of this bug. ***

Comment 11

[James Ross]

-> All/All

Comment 12

[James Ross]

I blame http://lxr.mozilla.org/seamonkey/source/docshell/base/nsWebShell.cpp#701

That seems to be the "oops, host doesn't resolve, let's screw with the user's input" code. It also appears to do it only for http/https schemes!

The easy fix is to just change it to disable keywords if there is a scheme at all, but I'm not sure why it allows http://word in the first place...

Comment 13

[Nicholas Tung]

it seems like people are panicking too much but it works by simply clicking on a url which a company could have mistyped, however I don't understand how paypal gets this; isn't google the only one who could ever get the information? When the "i'm feeling lucky" search occurs, don't any security parameters in the URL get wiped after google redirects it?

the only thing that could be bad is already fixed - localhost really goes to 127.0.0.1 as it should. then again "google" should always go to google and not have to pass through google's "i'm feeling lucky" search just to save milliseconds.

Comment 14

[Adam Guthrie]

*** Bug 317405 has been marked as a duplicate of this bug. ***

Comment 15

[Phil Ringnalda (:philor)]

*** Bug 319517 has been marked as a duplicate of this bug. ***

Comment 16

[Dagfinn I. Mannsåker]

(In reply to comment #12)
> I'm not sure why it allows http://word in the first place...

Because it's quite common to have a default search domain specified in the resolver (i.e. /etc/resolv.conf on Linux), enabling you to omit the domain name for local hosts. Removing this ability would reduce usability drastically.

Comment 17

[James Ross]

Oh FFS. Will you PLEASE READ the damn bug!

We're talking about the auto-searching code, and it allows "http://foo" to trigger an automatic search for "foo" if the host does not resolve.

Comment 18

[Justin Kerk]

I've also seen this be a problem on web forums when someone posts an obviously invalid URL as a joke (like http://rofl/) but due to this "feature" the link works and goes to whichever website is #1 on Google at the moment, causing confusion all around.

Comment 19

[Jesse Ruderman]

Justin, that's bug 310826, although a fix for this bug might fix that too.

Comment 20

[James Ross]

Attachment: Don't allow keyword-ing of ANY scheme

This removes the explicit exception for the http and https schemes for doing keyword searches when the host lookup fails.

Please note that this exception was put in deliberately, due to the way other stuff happened. See <https://bugzilla.mozilla.org/show_bug.cgi?id=143080#c17>. I could not reproduce the mentioned behaviour, but it could make this really hard to fix.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fwiw, that patch just disables keywords completely.  That's fine by me (not that I'm the one to make that decision), but if that's what we're doing we should remove the code, not just leave dead code in the tree.

Comment 22

[James Ross]

Hm, odd. I couldn't test is because stuff was failing to compile elsewhere.

Comment 23

[Simon Bünzli]

*** Bug 330962 has been marked as a duplicate of this bug. ***

Comment 24

[James Ross]

The patch doesn't actually disable it here, it just stops it doing it for single words and other host-like things. Anyway, this is being screwed about with in an excessively invasive way over in bug 331522, so I'm not going to try and do anything here until that's done.

Comment 25

[Jesse Ruderman]

*** Bug 332962 has been marked as a duplicate of this bug. ***

Comment 26

[Boris Zbarsky [:bzbarsky]]

So where do we actually create a URI from this string?  Is that happening in docshell, or out in the chrome?

Comment 27

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #26)
> So where do we actually create a URI from this string?  Is that happening in
> docshell, or out in the chrome?

in docshell, from what I can see:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp&rev=1.785&mark=2799,2806#2778

handleURLBarCommand calls BrowserLoadURL which calls loadURI, in the standard case.

Comment 28

[Boris Zbarsky [:bzbarsky]]

OK.  So would it make sense to disable keyword lookup at that point?  That is, if the "allow keyword" flag is set, before calling into fixup try to ExtractScheme or whatever on the string.  If that succeeds, toggle off the flag.  roc, darin, biesi, what do you think?

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

sounds like a good idea.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree.

Comment 31

[Darin Fisher]

You could simply disable keyword search if the string can be parsed into a nsIURI.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Would it make sense to modify the URIFixup API to indicate that information via an out parameter, perhaps?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so-ish?

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 217936 [details] [diff] [review]
Like so-ish?

is there a point in using URI Fixup when we already have a valid URI?

Comment 35

[Boris Zbarsky [:bzbarsky]]

> is there a point in using URI Fixup when we already have a valid URI?

Yes.  For example, consider what URI fixup does with:

  view-source:google.com

I could add a comment to that effect if desired.

Comment 36

[Darin Fisher]

Comment on attachment 217936 [details] [diff] [review]
Like so-ish?

>Index: docshell/base/nsDocShell.cpp

>+    nsAutoString uriString(aURI);
>+    // Cleanup the empty spaces that might be on each end.
>+    uriString.Trim(" ");
>+    // Eliminate embedded newlines, which single-line text fields now allow:
>+    uriString.StripChars("\r\n");
>+    NS_ENSURE_TRUE(!uriString.IsEmpty(), NS_ERROR_FAILURE);
>+
>+    rv = NS_NewURI(getter_AddRefs(uri), uriString);
>+    if (uri) {
>+        aLoadFlags = aLoadFlags & ~LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
>+    }
>+    
>+    if (sURIFixup) {
>+        // Call the fixup object.  This will clobber the rv from
>+        // NS_NewURI above, but that's fine with us.
>         PRUint32 fixupFlags = 0;
>         if (aLoadFlags & LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP) {
>           fixupFlags |= nsIURIFixup::FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP;
>         }
>         rv = sURIFixup->CreateFixupURI(NS_ConvertUTF16toUTF8(aURI), fixupFlags,
>                                        getter_AddRefs(uri));

Notice that this is doing a conversion from UTF16 to UTF8 twice
for the URI string.  NS_NewURI takes a nsAString, so you might
as well convert up-front, and save some effort.

Is there any reason not to pass the trim'd and strip'd URI string
to CreateFixupURI?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Will do the UTF8 thing up front (and pass the trimmed version to the fixup service).  Do you want an updated patch with those changes?

Comment 38

[Darin Fisher]

Comment on attachment 217936 [details] [diff] [review]
Like so-ish?

no need :)

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 217936 [details] [diff] [review]
Like so-ish?

hm, ok... please do add the comment. it sort of uglifies this code because it converts string->uri twice, oh well

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 217936 [details] [diff] [review]
Like so-ish?

damn product changes losing review flags!

Comment 42

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 43

[Darin Fisher]

Comment on attachment 218097 [details] [diff] [review]
Updated to review comments

a=darin for 1.8.1

Comment 44

[Boris Zbarsky [:bzbarsky]]

I'll land this on branch once bug 331522 lands there.

Comment 45

[Justin Kerk]

Hmm, now the I'm Feeling Lucky search doesn't happen, but it sends you to http://www.foobar.com/ if you type in http://foobar/ - is that really desirable? Seems like if you give a full URL you don't want it trying random similar hostnames - this could be a security issue if you go to http://localnetwork/script?importantpassword without thinking when you're away from the network, so it sends that query to localnetwork.com... (which it will, assuming localnetwork.com exists)

Comment 46

[Jorge Villalobos [:jorgev] (he/him)]

That looks like a separate bug to me. "Do not autocomplete URLs when GET parameters are used".
Or something.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Yeah, that's a totally separate issue.  I'm not touching that one with a 10-foot pole, personally.  ;)

Comment 48

[Simon Bünzli]

*** Bug 337836 has been marked as a duplicate of this bug. ***

Comment 49

[Darin Fisher]

Please land this patch on the MOZILLA_1_8_BRANCH.  Thanks!

Comment 50

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.

Comment 51

[Ria Klaassen (not reading all bugmail)]

*** Bug 344826 has been marked as a duplicate of this bug. ***

Comment 52

[Ria Klaassen (not reading all bugmail)]

*** Bug 356307 has been marked as a duplicate of this bug. ***

Comment 53

[timeless]

verified w/ ff4.0b1 :)