402788 - ensure web-based protocol handlers can't override gecko-internal stuff

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Web-based protocol handlers should not be able to register for schemes that gecko will always want to handle internally: javascript, chrome, http, etc are examples.  This should already work by default, but we need some tests written to be sure.  Further, we'll probably need a whitelist of internally-handled things that _should_ be overridable, such as feed.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

feeds is going to need to be special-cased, and there's already a bug for that.  

As far as which protocols are allowed to be registered, we should probably base this off the existing network.protocol-handler.external / network.protocol-handler.external-default blacklist/default list.

I'm not sure how the .expose prefs play into this; they may be irrelevant here, at least until we support web-based protocol handlers in non-browser apps.

Comment 2

[Florian Quèze [:florian]]

Attachment: patch v1

Comment 3

[Florian Quèze [:florian]]

Attachment: patch v2 (same with tests)

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 291802 [details] [diff] [review]
patch v2 (same with tests)

Gavin requested that someone who knows the exthandler prefs review this too. Requesting an additionnal review from dmose.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 291802 [details] [diff] [review]
patch v2 (same with tests)

r=dmose for the pref bits, on the condition of spinning off a bug: we should really have one instance of this code rather than multiple (there are already others); I think it probably wants to be called nsIExternalProtocolService.isAllowedProtocol

Comment 6

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

Comment on attachment 291802 [details] [diff] [review]
patch v2 (same with tests)

>Index: browser/components/feeds/src/WebContentConverter.js

>+    // check if it is in the black list
>+    var pb = Cc["@mozilla.org/preferences-service;1"].
>+             getService(Ci.nsIPrefService).getBranch(null);

nit: Could just getService(Ci.nsIPrefBranch) and omit the getBranch call.

Comment 7

[Florian Quèze [:florian]]

Attachment: patch v3 (ready for checkin)

Nit fixed, and unbitrotted patch.

Comment 8

[Florian Quèze [:florian]]

(In reply to comment #5)
> (From update of attachment 291802 [details] [diff] [review])
> r=dmose for the pref bits, on the condition of spinning off a bug: we should
> really have one instance of this code rather than multiple (there are already
> others); I think it probably wants to be called
> nsIExternalProtocolService.isAllowedProtocol
> 

Filed bug 416456 to track this issue.


Checking in browser/components/feeds/src/WebContentConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/WebContentConverter.js,v  <--  WebContentConverter.js
new revision: 1.27; previous revision: 1.26
done
Checking in browser/components/feeds/test/Makefile.in;
/cvsroot/mozilla/browser/components/feeds/test/Makefile.in,v  <--  Makefile.in
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/browser/components/feeds/test/test_registerHandler.html,v
done
Checking in browser/components/feeds/test/test_registerHandler.html;
/cvsroot/mozilla/browser/components/feeds/test/test_registerHandler.html,v  <--  test_registerHandler.html
initial revision: 1.1
done

Fixed.

Comment 9

[Phil Ringnalda (:philor)]

You'll probably want to rewrite your test so it doesn't put false positives in Tinderbox logs - look at any successful test run's build log, and you'll now see four "errors" from this test, and although I got the blame this time, next time I won't, and every time people will get distracted by very long error-looking lines that shove the fact that the test actually passed off several screens to the right.

Comment 10

[Jesse Ruderman]

I removed the "dump(e)" from test_registerHandler.html.  That should fix the Tinderbox issue.