Closed
Bug 402788
Opened 14 years ago
Closed 13 years ago
ensure web-based protocol handlers can't override gecko-internal stuff
Categories
(Core Graveyard :: File Handling, defect, P2)
Core Graveyard
File Handling
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9beta4
People
(Reporter: dmose, Assigned: florian)
Details
(Whiteboard: [proto])
Attachments
(1 file, 2 obsolete files)
9.18 KB,
patch
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•14 years ago
|
Whiteboard: [proto]
Reporter | ||
Updated•14 years ago
|
Priority: -- → P3
Flags: blocking1.9? → blocking1.9+
Priority: P3 → P2
Reporter | ||
Updated•14 years ago
|
Whiteboard: [proto] → [proto][needs patch]
Reporter | ||
Updated•14 years ago
|
Assignee: dmose → florian
Reporter | ||
Comment 1•14 years ago
|
||
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.
Assignee | ||
Comment 2•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #290894 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #290894 -
Attachment is obsolete: true
Attachment #291802 -
Flags: review?(gavin.sharp)
Attachment #290894 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [proto][needs patch] → [proto][needs review gavin]
Assignee | ||
Comment 4•13 years ago
|
||
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)
Reporter | ||
Comment 5•13 years ago
|
||
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 6•13 years ago
|
||
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+
Updated•13 years ago
|
Whiteboard: [proto][needs review gavin] → [proto]
Reporter | ||
Updated•13 years ago
|
Whiteboard: [proto] → [proto][needs spinoff bug, checkin]
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → mozilla1.9beta4
Assignee | ||
Comment 7•13 years ago
|
||
Nit fixed, and unbitrotted patch.
Attachment #291802 -
Attachment is obsolete: true
Assignee | ||
Comment 8•13 years ago
|
||
(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]
Comment 9•13 years ago
|
||
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•13 years ago
|
||
I removed the "dump(e)" from test_registerHandler.html. That should fix the Tinderbox issue.
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•