Closed Bug 402788 Opened 13 years ago Closed 13 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: dmose, Assigned: florian)

Details

(Whiteboard: [proto])

Attachments

(1 file, 2 obsolete files)

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.
Flags: blocking1.9?
Whiteboard: [proto]
Priority: -- → P3
Flags: blocking1.9? → blocking1.9+
Priority: P3 → P2
Whiteboard: [proto] → [proto][needs patch]
Assignee: dmose → florian
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.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #290894 - Flags: review?(gavin.sharp)
Attached patch patch v2 (same with tests) (obsolete) — Splinter Review
Attachment #290894 - Attachment is obsolete: true
Attachment #291802 - Flags: review?(gavin.sharp)
Attachment #290894 - Flags: review?(gavin.sharp)
Whiteboard: [proto][needs patch] → [proto][needs review gavin]
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.
Attachment #291802 - Flags: review?(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
Attachment #291802 - Flags: review?(dmose) → review+
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.
Attachment #291802 - Flags: review?(gavin.sharp) → review+
Whiteboard: [proto][needs review gavin] → [proto]
Whiteboard: [proto] → [proto][needs spinoff bug, checkin]
Target Milestone: --- → mozilla1.9beta4
Nit fixed, and unbitrotted patch.
Attachment #291802 - Attachment is obsolete: true
(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.
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [proto][needs spinoff bug, checkin] → [proto]
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.
I removed the "dump(e)" from test_registerHandler.html.  That should fix the Tinderbox issue.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.