331522 - Suppress keyword search (e.g. Google "I feel lucky") on middleclick URL paste

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Firefox's automated keyword search feature makes it possible for users to accidentally disclose confidential text to Google and possibly other third parties, simply by selecting the text and then accidentally middle-clicking in a Firefox window. This also makes a confusing user experience when the browser suddenly navigates to a random page.

A straightforward way to fix this is to suppress the keyword search for loads generated from middle-click URL paste. It isn't quite as straightforward as one might like because keyword search launches in two places ... one from nsDefaultURIFixup at the start of a load, and also from nsWebShell if the hostname fails to resolve. The second is less important (text with spaces generates a bad URL error early and we don't reach the nsWebShell code) but should still be suppressed.

Comment 1

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

Attachment: fix

Boris, I'm sending this review your way, but only because I think this is a simple patch that shouldn't take much time to look at.

Comment 2

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

IMHO the Firefox JS changes are fairly trivial, but if you want me to get separate review on them from mconnor, I will.

Comment 3

[Hixie (not reading bugmail)]

I use this feature ALL the fricking time. Please don't remove it.

Comment 4

[Hixie (not reading bugmail)]

(In particular one of the things I do the most is select an address from my e-mail in Pine, and the middle-click paste it into Firefox, thus causing Firefox to bring up the address centered in a Google Maps window. It's awesome.)

Comment 5

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

Ah, there's one leftover dump() in there that I'll take out before checkin :-)

Comment 6

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

Attachment: the right patch

actually, I merely attached the wrong patch. Here is the right patch with dump()s removed.

Comment 7

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

Ian: do you have any other idea for how to resolve the information leaks, then? If not, I'm tempted to suggest that your behavior belongs in an extension.

But you're right that at least someone in Firefox should have some input...

Comment 8

[Mike Connor [:mconnor]]

So, I still don't get why we haven't killed contentLoadURL by default.  It always seems to confuse the hell out of new Linux users.  And experienced ones, sometimes...  its also really annoying if you have a web form and you're trying to mm paste into a form and miss!

Hixie, I think the general annoyance of this type of behaviour, along with the privacy concerns, make it hard to keep as a handler for content.  Middle-click on the favicon in the URL bar should continue to work, after my review comments.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 216061 [details] [diff] [review]
the right patch

So, its late so I'll just add some general review comments for now:

- This feels bloaty, but I guess its the only way to do this right without just killing contentLoadURL completely.
- I'd like to make fixup the special case, not suppressing fixup.  This is basically replacing suppressFixup with doFixup, and only setting true where we want to fix up user input.  IK is only really useful when input is coming from a user and we're trying to help them.  Outside of the URL bar (including the mmpaste on favicon) I don't think we want the fixup behaviour.  See bug 310826 for the problems this can cause.
- loadURI in tabbrowser needs to match the browser changes, since a lot of code calls the tabbrowser method, which forwards to currentBrowser.loadURI
- use aFoo for args where other args do

Comment 10

[Boris Zbarsky [:bzbarsky]]

The docshell changes seem generally ok; need to think through using a member here a little bit.  darin, are you ok with the webnav change given the freezing we're trying to do there?

I do agree that it seems to me like we should _not_ do fixup unless it's explicitly requested...

Comment 11

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

I think changing the default to not do fixup would be too much of a change for this interface. but, adding new flags seems fine to me even after we freeze it (like for some other interfaces), if we explicitly say that this may happen.

Comment 12

[Darin Fisher]

Comment on attachment 216061 [details] [diff] [review]
the right patch

>Index: docshell/base/nsIWebNavigation.idl
... 
>+  /**
>+   * This flag specifies that keyword fixup should not be applied.
>+   * This flag is useful when the user may have supplied the URI text by
>+   * mistake, in which case the text should not be disclosed to a
>+   * third-party server.
>+   */
>+  const unsigned long LOAD_FLAGS_NO_KEYWORD_FIXUP = 0x2000;

Addition of flags is fine, in general.  I wonder, though... what
about a flag to control other URI fixup?  Do we need to explicitly
mention the "keyword" system in this interface?  Would it be enough
to disable all URI fixup?

Based on the summary of this bug, I can see why you might want to
only exclude keyword fixup.  Still, it feels a bit unfortunate to
encode such things in the API.  Is there perhaps another way of
wording this flag that does not mention the keyword fixup system?

Or, maybe it's not such a big deal.  I can't think of any better
wording, and nsIURIFixup mentions the keyword system too.  Hmm...

Comment 13

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

(In reply to comment #12)
> Addition of flags is fine, in general.  I wonder, though... what
> about a flag to control other URI fixup?  Do we need to explicitly
> mention the "keyword" system in this interface?  Would it be enough
> to disable all URI fixup?

It'd be nice to still be able to paste "www.smurfs.com" to go there.

> Based on the summary of this bug, I can see why you might want to
> only exclude keyword fixup.  Still, it feels a bit unfortunate to
> encode such things in the API.  Is there perhaps another way of
> wording this flag that does not mention the keyword fixup system?

How about NO_THIRD_PARTY_LOOKUP?

Comment 14

[Darin Fisher]

Yeah, I like the term "third party" too.  Although, I'd probably stick with "FIXUP" instead of "LOOKUP" since clearly domain name servers will be consulted for "lookup."  The comments for this flag can mention the possibility of a third-party service being invoked to fixup the URI if the URI could not be resolved.  I would imagine that if we ever supported a plug-in API for URI fixup that we'd want to skip that API if this flag were specified.

Comment 15

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

Attachment: updated patch

Updated to comments.

The favicon currently isn't a drop target. However, "New Window" and "New Tab" buttons are, if you add them to your toolbar. So I just made those latter allow third-party lookups on drop.

Comment 16

[Darin Fisher]

Why did you switch it from NO_THIRD_PARTY_FIXUP to ALLOW_THIRD_PARTY_FIXUP?  That changes the behavior of the interface in the default case, which might cause compatibility problems (e.g., an extension previously using this API expected keyword fixup, but now will get none).  That's bad from an API stability point of view.  That said, if we didn't care about API stability, then I'd definitely want this to be an opt-in flag as well.  Hmm.. /me torn.

Comment 17

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

(In reply to comment #16)
> Why did you switch it from NO_THIRD_PARTY_FIXUP to ALLOW_THIRD_PARTY_FIXUP? 
> That changes the behavior of the interface in the default case, which might
> cause compatibility problems (e.g., an extension previously using this API
> expected keyword fixup, but now will get none).  That's bad from an API
> stability point of view.  That said, if we didn't care about API stability,
> then I'd definitely want this to be an opt-in flag as well.  Hmm.. /me torn.

Boris and Mike wanted it that way (comment #9 and comment #10), and I prefer it that way too.

Fixing 310826 (which this does) is guaranteed to break compatibility in some way. Furthermore code broken by this change would also be broken if keywords were disabled or if the keyword.URL was changed.

On the other hand I don't really care as long as I can fix this bug. So you choose.

Comment 18

[Darin Fisher]

OK.  I also greatly prefer the opt-in solution from an API point-of-view.  Let's go with it.  Please post to m.d.platform to let people know about this change.  we can always reconsider this change before it goes out with any shipping product.

Comment 19

[Darin Fisher]

Comment on attachment 216376 [details] [diff] [review]
updated patch

>Index: docshell/base/nsDocShell.cpp

>+    mAllowKeywordSearchOnLoadFailure(PR_FALSE),

mAllowKeywordFixup would be fine too.  it's a bit shorter.


>+    rv = LoadURI(uri, loadInfo, aLoadFlags & LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP, PR_TRUE);

nit: wrap long lines at 80 chars


I did not review the browser/ changes.


>Index: toolkit/content/contentAreaUtils.js

> /**
>  * openNewTabWith: opens a new tab with the given URL.
>  *
>  * @param href The URL to open (as a string).
>  * @param sourceURL The URL of the document from which the URL came, or null.
>  *          This is used to set the referrer header and to do a security check of whether
>  *          the document as allowed to reference the URL.
>  *          If null, there will be no referrer header and no security check.
>  * @param postData Form POST data, or null.
>  * @param event The triggering event (for the purpose of determining whether to open in the background), or null
>  */ 
>+function openNewTabWith(href, sourceURL, postData, event, aAllowThirdPartyFixup)

I think you should document the aAllowThirdPartyFixup parameter.
Also, style convention (for this function at least) seems to be
that you should not begin parameter names with "a"

Please document the fact that this parameter can be undefined.
That's important for backwards compat.


r+sr=darin

Comment 20

[Mike Connor [:mconnor]]

Comment on attachment 216376 [details] [diff] [review]
updated patch

Yeah, I really like this approach, with one change.  If we're doing fixup on DnD to the new tab and new window buttons, why are we not for the Go button?  Change that and we're good to go.

Comment 21

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

Attachment: patch as checked in

I just checked this in.

I tweaked the Go button drop code to allow it to perform keyword searches, but it doesn't work because it passes the URI through the URI parser and then back to a spec. I'm not sure whether we should change this. I'm happy to deal with it in a followup bug.

Comment 22

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

Comment on attachment 216376 [details] [diff] [review]
updated patch

Should we take this on the 1.8.1 branch?

Comment 23

[Darin Fisher]

Comment on attachment 216376 [details] [diff] [review]
updated patch

Yes, I think this should be OK for 1.8.1.  I worry about the API impact, but extension authors should be able to deal.

Comment 24

[Brian Kennelly]

Keywords no longer work at all, even from the URL bar.  From the description and commentary, it looks like you were trying to preserve it for user input.

Is there a preference to restore the old behaviour?

Gecko/20060408 SeaMonkey/1.5a

Comment 25

[[e-mail]]

(In reply to comment #24)
>Keywords no longer work at all, even from the URL bar.  From the description
>and commentary, it looks like you were trying to preserve it for user input.
They were turned off in the back end for all applications. They were also turned on in the front end for Firefox. Feel free to volunteer for the work that will be needed to turn them on in the front end for SeaMonkey.

Comment 26

[Brian Kennelly]

(In reply to comment #25)
> They were turned off in the back end for all applications. They were also
> turned on in the front end for Firefox. Feel free to volunteer for the work
> that will be needed to turn them on in the front end for SeaMonkey.
> 

Thanks, I may do that.  If I understand correctly, I need to add a new paramater to calls to nsDocShell::LoadURI, indicating 'true' for user entry (URL bar, Open Location), and 'false' for all others.  (I understand that this will require changes to other functions between the user input and the call.)

I noticed that this bug is marked 'Resolved Fixed'.  Should I open a new ticket for SeaMonkey?

Comment 27

[Brian Kennelly]

Moving SeaMonkey work to #332668

Comment 28

[[e-mail]]

(In reply to comment #26)
>If I understand correctly, I need to add a new paramater to calls to
>nsDocShell::LoadURI, indicating 'true' for user entry (URL bar, Open
>Location), and 'false' for all others.  (I understand that this will
>require changes to other functions between the user input and the call.)
It doesn't quite work like that. If you read nsIWebNavigation.idl you will see that the second parameter to loadURI are load flags. Thus in that case it is only necessary to pass LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP to that function call. Additionally the flag values are passed via browser.xml and tabbrowser.xml's loadURIWithFlags methods so only their callers (starting with tabbrowser.xml's addTab and navigator.js's loadURI) will need to have the new parameter.

>Should I open a new ticket for SeaMonkey?
I see you noticed that I already have :-)

Comment 29

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

Fixed on branch.

Comment 30

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

Attachment: additional fix

The checked in patch from this bug regressed bug 331222. I just checked this in branch and trunk to fix it (after fixing bug 334776).

Comment 31

[Wolfgang Rosenauer [:wolfiR]]

clearing blocking1.8.1? flag because it's already there