Closed Bug 402788 Opened 14 years ago Closed 13 years ago
ensure web-based protocol handlers can't override gecko-internal stuff
Flags: blocking1.9? → blocking1.9+
Priority: P3 → P2
Whiteboard: [proto] → [proto][needs patch]
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.
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]
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
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.
You need to log in before you can comment on or make changes to this bug.