232720 - Internet Keywords / nsDefaultURIFixup::CreateFixupURI need to be redone

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[Jerry Talkington]

User-Agent:       
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7a) Gecko/20040130 Firebird/0.8.0+

The way keywords are implemented is kind of shaky.  Some of the problems I see
with it are:
A) keywords are never triggered if |.| or a |:| appear anywhere in the URLBar,
even if it's clearly not a scheme or host delimiter.
B) a |?| at the beginning of something typed in the URLBar will trigger
keywords, but not if the input matches 1), or if keywords are disabled (however,
the |keyword:| psuedo-scheme still works regardless of the state of
|keywords.enabled|)
C) keywords are invoked for https URLs that fail, which introduces security concerns
D) any input that starts with |ftp| is transformed to an ftp URL, which keywords
are not invoked for.  So if one was to try to enter the "keyword" FTP_PROXY, it
will fail
E) if a transformed URI fails to load due to lookup errors, only the host
portion is sent, and it is only tried if the host doesn't have a |.| in it
There are probably a couple of other things that I forgot to mention.

Since I've spent quite a bit of time looking at the code while tracking down bug
178123, bug 184433, bug 229668, etc., I'm willing to do it.

I propose that we:
1) add a GUI to disable keywords in Firebird, perhaps with an input for the URL,
and stick it under Advanced/Browser
2) treat |?| at the beginning as the equivalent of |keyword:|, and make it work
regardless of whether or not keywords are enabled
3) do better checking for |.| and |:|, allowing them to occur after a space that
is in the |path| component
4) Either 
   a) start doing keyword lookups for ftp URLs
   b) (IMHO better) don't guess that "hosts" that start with ftp... are ftp
protocol, and try to load them as HTTP (since we are a web browser first, and
many ftp servers have httpd running on them as well, and we could always scan
for ftp... starting hosts and try to load them as ftp after the HTTP load fails)
5) Be more aggressive with keyword lookups if host name lookup fails, and
include "hosts" with a |.| in them and the path as part of the keywords, or
exclude any URIs with path information (i.e. if I type "homo/sapien" in now, the
keyword redirects me to "http://homo.net/")

And probably some other stuff that I can't quite remember now.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Boris Zbarsky [:bzbarsky]]

Jerry, should I just assign the bug to you?

Comment 2

[Jerry Talkington]

(In reply to comment #1)
> Jerry, should I just assign the bug to you?

Hmm, I didn't realize that I can change the owner on bugs that report, I'll do
that now.

Oops, it won't let me change the owner, although it gives me the option.  I get
an error (filed bug 232723), but in the meantime could you please assign it to
me please?

Comment 3

[alanjstr]

see also bug 150025

Comment 4

[Jerry Talkington]

(In reply to comment #3)
> see also bug 150025

Yes, I'm going to make it so that a host of "localhost" is ignored for any
after-the-fact fixup.

Comment 5

[Jerry Talkington]

Attachment: Patch to clean up the keywords backend.

Here's a patch for the backend stuff.  It doesn't touch the GUI.

This patch:
1) Treats "?" at the beginning of a typed URL as a synonym of "keyword:"
2) Cleans up the keyword check so that more strings that are invalid URLs are
picked up.
3) Adds a readonly member to nsIURIFixup, to indicate if a scheme was added.
4) Adds a new load type to nsIDocShellLoadInfo and nsIWebNavigation, so we
differentiate between normal loads (i.e. those that were valid when typed into
the location bar,) and those that we added a scheme to.

For unresolveable hosts:
5) Only does a keyword check on unknown hosts that we added the scheme to.
6) Sends the entire string that was typed into the location bar, not just the
host name.

I'll work on the GUI stuff next.  I won't be able to do anything with
autocomplete JavaScript, but I already took a stab at the XUL stuff for
preferences, and it looks doable.

Comment 6

[benc]

So, is are the keyword prefs sort of disconnected from each other?

I thought keyword.enable enabled URL bars usage of the kewyord: protocol.

and keyword.URL was the URL prefix that the kewyord:<STUFF> was mapped to for
the IK lookup.

If you are going to really do a lot of re-writing, I suggest you consider a mode
that is purely server based as described in bug 127872.

Comment 7

[Jerry Talkington]

(In reply to comment #6)
> So, is are the keyword prefs sort of disconnected from each other?

Yes.

> I thought keyword.enable enabled URL bars usage of the kewyord: protocol.

Nope, it only decides if automatic keyword lookup is enabled.  The keyword:
pseudo-scheme is always enabled. 
 
> and keyword.URL was the URL prefix that the kewyord:<STUFF> was mapped to for
> the IK lookup.

Yes, that is the mapping.  You can also disable the keyword: pseudo-scheme by
making keyword.URL blank.

> If you are going to really do a lot of re-writing, I suggest you consider a mode
> that is purely server based as described in bug 127872.

This patch provides sort of a middle ground between the arguments in bug 127872.
 I would very much be against going to the keyword server first for things typed
into Location bar.  Instead, we try to lookup and connect to the host, and if
that fails, it goes to the keyowrd URL.

Of course, this doesn't work if a proxy is connected.  We could, however, bypass
the proxy (or DNS lookup) and go straight to the keyword server, but I wouldn't
want to do that unless we made another preference option.

Comment 8

[Patrick McCormick]

See also bug 202196, where a user wanted the "ftp" magic to also apply to
"gopher".  Adam refused to apply the patch since it's not very useful to the
vast majority of users, and also due to embedding concerns.  however he did say:

> I wouldn't be against changing
> the fixup API to allow protocols (or anyone else for that matter) 
> to register their own supplemental fixup objects.

so that's an idea to consider.

Comment 9

[Jerry Talkington]

Comment on attachment 140652 [details] [diff] [review]
Patch to clean up the keywords backend.

One thing that I didn't take into account here was that if a username has a
space in it, and a scheme was not present, a keyword lookup would occur (i.e.
typing "Jerry Talkington:mypass@somehost/members" would trigger a lookup.  I'll
submit a new patch shortly.

Comment 10

[Jerry Talkington]

(In reply to comment #8)
> See also bug 202196, where a user wanted the "ftp" magic to also apply to
> "gopher".  Adam refused to apply the patch since it's not very useful to the
> vast majority of users, and also due to embedding concerns.  however he did say:
> 
> > I wouldn't be against changing
> > the fixup API to allow protocols (or anyone else for that matter) 
> > to register their own supplemental fixup objects.
> 
> so that's an idea to consider.
> 

Filed bug 233428 on that, will look at it separately.

Comment 11

[Patrick McCormick]

> I would very much be against going to the keyword server 
> first for things typed into Location bar.

I second this.  I read over bug 127872, and it sounds like the total-keyword
solution is only viable for people who either run their own IK server or have a
fast connection to the IK server.

One idea might not have been an option back in 2002 when bug 127872 was opened;
you could accomplish bug 127872 as a Firebird extension.  Just override
canonizeUrl() (or one of the other URL-rewrite funcs in browser.js) to prepend
"keyword:" to the URL.  This occurs long before any of the functions we are
discussing here are hit.

If I get something like this working, I will update bug 127872.

Comment 12

[Jerry Talkington]

(In reply to comment #11)
> > I would very much be against going to the keyword server 
> > first for things typed into Location bar.
> 
> I second this.  I read over bug 127872, and it sounds like the total-keyword
> solution is only viable for people who either run their own IK server or have a
> fast connection to the IK server.

I don't think that there currently a way to go keywords only mode.
 
> One idea might not have been an option back in 2002 when bug 127872 was opened;
> you could accomplish bug 127872 as a Firebird extension.  Just override
> canonizeUrl() (or one of the other URL-rewrite funcs in browser.js) to prepend
> "keyword:" to the URL.  This occurs long before any of the functions we are
> discussing here are hit.

I think that an extension would be the best solution.  A second, less desireable
, option would be to add another pref (keyword.try_first or somesuch) to allow
the user to chose, which would be disabled by default.

BTW, re comment #10, bug number that I filed is bug 233425.

Comment 13

[Jerry Talkington]

Attachment: Updated patch that accounts for usernames with spaces in them

Comment 14

[Tuukka Tolvanen (sp3000)]

Jerry, in general you need to target a review request for it to get attention...
http://www.mozilla.org/owners.html should be helpful there

Comment 15

[Samuel Sidler (old account; do not CC)]

biesi, have you had a chance to look at this?

(Cleaning up old review requests.)

Comment 16

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

Comment on attachment 140985 [details] [diff] [review]
Updated patch that accounts for usernames with spaces in them

Cleaning up my review queue. Should this in fact be still relevant, please re-request.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.