Closed Bug 331522 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: fixed1.8.1)

Attachments

(3 files, 2 obsolete files)

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.
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #216060 - Flags: superreview?(bzbarsky)
Attachment #216060 - Flags: review?(bzbarsky)
IMHO the Firefox JS changes are fairly trivial, but if you want me to get separate review on them from mconnor, I will.
I use this feature ALL the fricking time. Please don't remove it.
(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.)
Ah, there's one leftover dump() in there that I'll take out before checkin :-)
Attached patch the right patch (obsolete) — Splinter Review
actually, I merely attached the wrong patch. Here is the right patch with dump()s removed.
Attachment #216060 - Attachment is obsolete: true
Attachment #216061 - Flags: superreview?(bzbarsky)
Attachment #216061 - Flags: review?(bzbarsky)
Attachment #216060 - Flags: superreview?(bzbarsky)
Attachment #216060 - Flags: review?(bzbarsky)
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...
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 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
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...
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 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...
(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?
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.
Attached patch updated patchSplinter Review
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.
Attachment #216061 - Attachment is obsolete: true
Attachment #216376 - Flags: superreview?(darin)
Attachment #216376 - Flags: review?(darin)
Attachment #216061 - Flags: superreview?(bzbarsky)
Attachment #216061 - Flags: review?(bzbarsky)
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.
(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.
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 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
Attachment #216376 - Flags: superreview?(darin)
Attachment #216376 - Flags: superreview+
Attachment #216376 - Flags: review?(darin)
Attachment #216376 - Flags: review+
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.
Attachment #216376 - Flags: review?(mconnor) → review+
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.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 216376 [details] [diff] [review]
updated patch

Should we take this on the 1.8.1 branch?
Attachment #216376 - Flags: approval-branch-1.8.1?(darin)
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.
Attachment #216376 - Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
Blocks: 332668
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
(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.
(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?
Moving SeaMonkey work to #332668
(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 :-)
Blocks: 263213
Flags: blocking1.8.1?
Attached patch additional fixSplinter Review
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).
clearing blocking1.8.1? flag because it's already there
Flags: blocking1.8.1?
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.8? → blocking1.8.0.8-
You need to log in before you can comment on or make changes to this bug.