Closed
Bug 1429732
Opened 6 years ago
Closed 6 years ago
Consider making registerProtocolHandler require SecureContext
Categories
(Core :: DOM: Security, enhancement, P3)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: jkt, Assigned: jkt)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, sec-want, site-compat, Whiteboard: [domsecurity-backlog1][adv-main60-])
Attachments
(1 file)
Registering a protocol handler is a powerful action in that all types of a certain protocol would be sent to a third party web page the user has approved. The problem here is that over an insecure connection this will allow a MiTM to detect the URLs that are being sent to the protocol handler. The protocols permitted are: bitcoin, geo, im, irc, ircs, magnet, mailto, mms, news, nntp, sip, sms, smsto, ssh, tel, urn, webcal, wtai, xmpp And web+... As far as I'm aware we don't have telemetry on this feature so we would likely have to get some on secure/not secure. I found stats on it's overall usage before of 0.2% however I can't find the source for that stat.
Comment 1•6 years ago
|
||
Seems like a good idea. Are you going to land telemetry in this bug or in a separate bug this depends on? I recommend a separate bug so we don't land patches and then leave a bug open.
Assignee | ||
Comment 2•6 years ago
|
||
My current plan is to land in this bug: - A pref to disable - A default for nightly to disable - A warning deprecation for 62 removal This will give us beta and stable deprecation statistics if we need to extend the period of deprecation. I'm taking this rapid approach due to the design of this API and the expected low usage 0.002836% of pageloads according to Chrome. Perhaps a follow up task would be to clean up prior handlers to ensure they are HTTPS too.
Flags: needinfo?(jkt)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jkt
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Keywords: dev-doc-needed
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8948241 [details] Bug 1429732 - Use a pref to disable registerProtocolHandler in insecure contexts. https://reviewboard.mozilla.org/r/217758/#review223596 ::: dom/base/nsGlobalWindowInner.h:851 (Diff revision 2) > mozilla::dom::CallerType aCallerType, > mozilla::ErrorResult& aError); > void SetOuterHeight(JSContext* aCx, JS::Handle<JS::Value> aValue, > mozilla::dom::CallerType aCallerType, > mozilla::ErrorResult& aError); > + static bool RegisterProtocolHandlerAllowedForContext(JSContext* /* unused */, JSObject* aObj); Move this near the other IsFoobarEnabled() static methods. ::: dom/locales/en-US/chrome/dom/dom.properties:356 (Diff revision 2) > # LOCALIZATION NOTE: %1$S is the invalid property value and %2$S is the property name. > InvalidKeyframePropertyValue=Keyframe property value “%1$S” is invalid according to the syntax for “%2$S”. > # LOCALIZATION NOTE: Do not translate "ReadableStream". > ReadableStreamReadingFailed=Failed to read data from the ReadableStream: “%S”. > +# LOCALIZATION NOTE: Do not translate "registerProtocolHandler". > +RegisterProtocolHandlerInsecureWarning=Use of the registerProtocolHandler for insecure connections will be removed in version 62. Don't be too precise :) Just say that it will be removed soon. ::: dom/webidl/Navigator.webidl:80 (Diff revision 2) > }; > > [NoInterfaceObject] > interface NavigatorContentUtils { > // content handler registration > - [Throws] > + [Throws,Func="nsGlobalWindowInner::RegisterProtocolHandlerAllowedForContext"] Space after ','
Attachment #8948241 -
Flags: review?(amarchesini) → review+
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8948241 [details] Bug 1429732 - Use a pref to disable registerProtocolHandler in insecure contexts. https://reviewboard.mozilla.org/r/217758/#review223882
Attachment #8948241 -
Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8948241 [details] Bug 1429732 - Use a pref to disable registerProtocolHandler in insecure contexts. https://reviewboard.mozilla.org/r/217758/#review223596 > Don't be too precise :) Just say that it will be removed soon. We agreed over IRC this was consistent with other newer notices.
Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dcb60325f0cf Use a pref to disable registerProtocolHandler in insecure contexts. r=baku,dao
Comment 10•6 years ago
|
||
Backed out for breaking uriloader/exthandler/tests/mochitest/mochitest.ini: runByManifest mode must be enabled to set the `prefs` key (fails at least on Android): https://hg.mozilla.org/integration/autoland/rev/7f1a4d21cf30b8edce8816f7f752f2345f7e5af9 Push with failures (one 24 has the same failure summary but a different reason in the raw log): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=dcb60325f0cf4e375a0e2bb36d96e7632a16bdba&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=161116295&repo=autoland&lineNumber=1571 ERROR parsing uriloader/exthandler/tests/mochitest/mochitest.ini: runByManifest mode must be enabled to set the `prefs` key
Flags: needinfo?(jkt)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3f96ca5e9d2d Use a pref to disable registerProtocolHandler in insecure contexts. r=baku,dao
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3f96ca5e9d2d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 16•6 years ago
|
||
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/support-for-registerprotocolhandler-on-insecure-sites-has-been-deprecated/
Keywords: site-compat
Comment 17•6 years ago
|
||
I've added an entry for this on our experimental features page: https://developer.mozilla.org/en-US/Firefox/Experimental_features#Security Let me know if this reads OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1][adv-main60-]
You need to log in
before you can comment on or make changes to this bug.
Description
•