The default bug view has changed. See this FAQ.

protocol handlers (for mailto etc)

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
6 months ago
a month ago

People

(Reporter: noitidart, Assigned: mixedpuppy)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

50 Branch
mozilla54
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
Hi all, I know there have been requests to the Firefox addon devs on what you need to port to webext.

I have a featured addon - https://addons.mozilla.org/en-US/firefox/addon/mailtowebmails/

It lists all the handlers for a protocol (mailto:) and allows installing more and changing the active handler.

I plan to use native messaging to get the system software like Outlook, Apple mail, and offer to the users to install that.

Right now with webext, if i want to install a mailto handler say for example to "hotmail.com". I have to first open hotmail.com tab and inject content script, as registering webmail protocol handler needs same domain. From webext I need to get around that (as in bootstrap) - https://developer.mozilla.org/en-US/docs/Web/API/Navigator/registerProtocolHandler
(Reporter)

Updated

6 months ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit

Updated

5 months ago
Whiteboard: [design-decision-needed] triaged

Comment 1

4 months ago
I think this is a dupe of bug 1271553.
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1271553
(Reporter)

Comment 2

4 months ago
Yep looks like it, thanks!
(Assignee)

Comment 3

2 months ago
Since bug 1271533 has grown into a large conversation around handling protocols, I'm reopening this bug to deal specifically with the simple case of handling protocols, such as what nav.registerProtocolHandler [RPH] provides.

However, rather than supporting the navigator api this will implement the internal hooks used by RPH.  This allows us to ensure that the protocol handler is unregistered if the addon is disabled or uninstalled.  The user experience will be the same.

- permission dialog may/will have the protocol handler listed, rather than a prompt during registration
- user will have to choose that protocol handler in the standard dialog same as with RPH (nothing we have to do in this patch to get this)
- user has the option to make it default and avoid being asked again (nothing we have to do in this patch to get this)

This will allow an addon to register a page that handles a protocol.  That page may do additional processing then redirect if that is desired.

I will move the patch I've attached to bug 1271533 to this bug.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: WebExtension Expermeint feature request - protocol handlers (for mailto etc) → protocol handlers (for mailto etc)
(Assignee)

Comment 4

2 months ago
Argh.  Above comment is supposed to say bug 1271553
Blocks: 1271553
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Assignee: nobody → mixedpuppy
(Assignee)

Updated

2 months ago
webextensions: --- → +

Comment 6

a month ago
mozreview-review
Comment on attachment 8833708 [details]
Bug 1310427 support protocol handlers,

https://reviewboard.mozilla.org/r/109888/#review114282

::: toolkit/components/extensions/ext-handlers.js:9
(Diff revision 1)
> +  if (!extension.permissions.has("protocol_handler")) {
> +    return;
> +  }

We don't normally have both a manifest property and a permission. The manifest property serves as the permission.

::: toolkit/components/extensions/ext-handlers.js:20
(Diff revision 1)
> +    let eps = Cc["@mozilla.org/uriloader/external-protocol-service;1"]
> +                .getService(Ci.nsIExternalProtocolService);

Please use `defineLazyServiceGetter` for this.

::: toolkit/components/extensions/ext-handlers.js:25
(Diff revision 1)
> +    let hs = Cc["@mozilla.org/uriloader/handler-service;1"]
> +               .getService(Ci.nsIHandlerService);

And this.

::: toolkit/components/extensions/ext-handlers.js:36
(Diff revision 1)
> +
> +extensions.on("shutdown", (type, extension) => {
> +  if (!handlers.has(extension)) {
> +    return;
> +  }
> +  for (let handlerConfig of handlers.get(extension)) {

I'm pretty sure we don't want to do any of this at app shutdown.

::: toolkit/components/extensions/ext-handlers.js:43
(Diff revision 1)
> +                .getService(Ci.nsIExternalProtocolService);
> +    let protoInfo = eps.getProtocolHandlerInfo(handlerConfig.protocol);
> +    let appHandlers = protoInfo.possibleApplicationHandlers;
> +    for (let i = 0; i < appHandlers.length; i++) {
> +      try {
> +        let handler = appHandlers.queryElementAt(i, Ci.nsIWebHandlerApp);

let handler = appHandlers.queryElementAt(i, Ci.nsISupports);
    if (handler instanceof Ci.nsIWebHandlerApp) {
      ...

::: toolkit/components/extensions/ext-handlers.js:44
(Diff revision 1)
> +    let protoInfo = eps.getProtocolHandlerInfo(handlerConfig.protocol);
> +    let appHandlers = protoInfo.possibleApplicationHandlers;
> +    for (let i = 0; i < appHandlers.length; i++) {
> +      try {
> +        let handler = appHandlers.queryElementAt(i, Ci.nsIWebHandlerApp);
> +        if (handler && handler.uriTemplate == handlerConfig.uriTemplate) {

s/==/===/

::: toolkit/components/extensions/ext-handlers.js:46
(Diff revision 1)
> +    for (let i = 0; i < appHandlers.length; i++) {
> +      try {
> +        let handler = appHandlers.queryElementAt(i, Ci.nsIWebHandlerApp);
> +        if (handler && handler.uriTemplate == handlerConfig.uriTemplate) {
> +          appHandlers.removeElementAt(i);
> +          if (protoInfo.preferredApplicationHandler == handler) {

s/==/===/

::: toolkit/components/extensions/ext-handlers.js:52
(Diff revision 1)
> +            protoInfo.preferredApplicationHandler = null;
> +            protoInfo.alwaysAskBeforeHandling = true;
> +          }
> +          break;
> +        }
> +      } catch (e) { continue; }

Please don't eat arbitrary errors.

::: toolkit/components/extensions/ext-handlers.js:56
(Diff revision 1)
> +        }
> +      } catch (e) { continue; }
> +    }
> +    let hs = Cc["@mozilla.org/uriloader/handler-service;1"]
> +               .getService(Ci.nsIHandlerService);
> +    hs.store(protoInfo);

We should only be doing this when we remove an element.

::: toolkit/components/extensions/schemas/extension_handler.json:21
(Diff revision 1)
> +        }]
> +      },
> +      {
> +        "$extend": "WebExtensionManifest",
> +        "properties": {
> +          "handlers": {

"handlers" is a weird name for this. Handlers for what?

::: toolkit/components/extensions/schemas/extension_handler.json:27
(Diff revision 1)
> +                "name": {"type": "string"},
> +                "protocol": {"type": "string"},
> +                "uriTemplate": {"type": "string"}

Please add descriptions for these properties, and the manifest property itself.

::: toolkit/components/extensions/test/mochitest/mochitest-common.ini:69
(Diff revision 1)
>  [test_ext_exclude_include_globs.html]
>  [test_ext_i18n_css.html]
>  [test_ext_generate.html]
>  [test_ext_notifications.html]
>  [test_ext_permission_xhr.html]
> +[test_ext_rph.html]

Please give this a less cryptic name.

::: toolkit/components/extensions/test/mochitest/test_ext_rph.html:63
(Diff revision 1)
> +    const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +    const protoSvc = Cc["@mozilla.org/uriloader/external-protocol-service;1"]
> +                       .getService(Ci.nsIExternalProtocolService);
> +    let protoInfo = protoSvc.getProtocolHandlerInfo("foo");
> +    sendAsyncMessage("preferredAction", protoInfo.preferredAction == protoInfo.useHelperApp);

You can use `assert.equal(...)` (and so forth) here.

::: toolkit/components/extensions/test/mochitest/test_ext_rph.html:78
(Diff revision 1)
> +  ok(yield chromeScript.promiseOneMessage("preferredAction"), "using a helper application is the preferred action");
> +  ok(yield chromeScript.promiseOneMessage("preferredApplicationHandler"), "no preferred handler is set");
> +  is(yield chromeScript.promiseOneMessage("handlers"), 1, "one handler is set");
> +  ok(yield chromeScript.promiseOneMessage("isWebHandler"), "the handler is a web handler");
> +  is(yield chromeScript.promiseOneMessage("uriTemplate"), `${handlerUrl}?val=%s`, "correct url template");

Can we just send an object with all of these properties rather than a bunch of separate messages?

Incidentally, doing it this way is pretty racy. The chrome script code doesn't buffer messages, so if you don't call `promiseOneMessage` before the message arrives, it gets lost.

::: toolkit/components/extensions/test/mochitest/test_ext_rph.html:106
(Diff revision 1)
> +  });
> +
> +  ok(yield chromeScript.promiseOneMessage("preferredApplicationHandler"), "no preferred handler is set");
> +  is(yield chromeScript.promiseOneMessage("handlers"), 0, "no handler is set");
> +  chromeScript.destroy();
> +});

We should try to test that we don't unset the default handler when the app restarts.
Attachment #8833708 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a month ago
mozreview-review-reply
Comment on attachment 8833708 [details]
Bug 1310427 support protocol handlers,

https://reviewboard.mozilla.org/r/109888/#review114282

> We should try to test that we don't unset the default handler when the app restarts.

It only gets unset if the addon is disabled/uninstalled and it's handler was default.  At that point the user will have to pick a new default via the dialog if they use protocol again.
Comment hidden (mozreview-request)
Drive-by comment: this seems like something that warrants a permission prompt.  Since it uses a manifest property instead of a permission, you need to modify `ExtensionData.userPermissions()` and choose a name that won't conflict with any "real" permission names, then add an appropriate string to browser.properties.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a month ago
FYI, the FoxyProxy legacy addon installs its own proxy: protocol handler, so its port to WebExtensions will require this support. Clicking a hyperlink on a page with the proxy: URL enables users to auto-configure proxies with a single-click (after confirming the change in a gBrowser.getNotificationBox())
(Assignee)

Updated

a month ago
Whiteboard: [design-decision-needed] triaged → triaged

Comment 14

a month ago
mozreview-review
Comment on attachment 8833708 [details]
Bug 1310427 support protocol handlers,

https://reviewboard.mozilla.org/r/109888/#review115678

::: toolkit/components/extensions/Extension.jsm:389
(Diff revision 5)
> +    if (Array.isArray(this.manifest.protocol_handlers)) {
> +      result.permissions.push("protocolHandlers");
> +    }

Hrm. So, normally we'd call call this permission "manifest:protocol_handlers", which is how the `hasPermission` method handles it. But property files treat ":" the same way they treat "=", so that won't work.

But if we're going to go this route, I'd rather add all manifest keys that act as permissions to the permissions list rather than doing the check in `hasPermission`, and then have the permissions UI code do whatever mangling is necessary to get the right string bundle key.

::: toolkit/components/extensions/ext-protocolHandlers.js:18
(Diff revision 5)
> +const whitelist = ["bitcoin", "geo", "im", "irc", "ircs", "magnet", "mailto",
> +                   "mms", "news", "nntp", "sip", "sms", "smsto", "ssh", "tel",
> +                   "urn", "webcal", "wtai", "xmpp"];

Please use a `Set` for this.

::: toolkit/components/extensions/ext-protocolHandlers.js:38
(Diff revision 5)
> +    if (!whitelist.includes(handlerConfig.protocol) &&
> +        !handlerConfig.protocol.startsWith("web+") &&
> +        !handlerConfig.protocol.startsWith("ext+")) {
> +      Cu.reportError("Unable to register non-whitelisted protocol scheme.");

We should handle this in the schema instead, so the reported error is easier to identify.

::: toolkit/components/extensions/schemas/extension_protocol_handlers.json:1
(Diff revision 5)
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

Please remove.
Attachment #8833708 - Flags: review?(kmaglione+bmo)
Bah, mozreview. Close this time, just a couple of concerns about validation and permission handling.
(Assignee)

Comment 16

a month ago
(In reply to Kris Maglione [:kmag] from comment #14)
> ::: toolkit/components/extensions/Extension.jsm:389
> (Diff revision 5)
> > +    if (Array.isArray(this.manifest.protocol_handlers)) {
> > +      result.permissions.push("protocolHandlers");
> > +    }
> 
> Hrm. So, normally we'd call call this permission
> "manifest:protocol_handlers", which is how the `hasPermission` method
> handles it. But property files treat ":" the same way they treat "=", so
> that won't work.
> 
> But if we're going to go this route, I'd rather add all manifest keys that
> act as permissions to the permissions list rather than doing the check in
> `hasPermission`, and then have the permissions UI code do whatever mangling
> is necessary to get the right string bundle key.

This feels like a separate bug, can we do a followup on the permissions system for that?
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request)
(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> This feels like a separate bug, can we do a followup on the permissions
> system for that?

I'd rather not land this with it adding an artificial "protocolHandlers" permission, especially if we're also adding a locale string for it at the same time.
Flags: needinfo?(kmaglione+bmo)

Comment 19

a month ago
mozreview-review
Comment on attachment 8833708 [details]
Bug 1310427 support protocol handlers,

https://reviewboard.mozilla.org/r/109888/#review116154

::: toolkit/components/extensions/ext-protocolHandlers.js:33
(Diff revision 6)
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("manifest_protocol_handlers", (type, directive, extension, manifest) => {
> +  for (let handlerConfig of manifest.protocol_handlers) {
> +    handlerConfig.uriTemplate = extension.baseURI.resolve(handlerConfig.uriTemplate);
> +    let url = new URL(handlerConfig.uriTemplate);
> +    if (!["http:", "https:", "moz-extension:"].includes(url.protocol)) {

This should also be validated in the schema. You might need to add a new format checker for it. Possibly `choices: [ExtensionUrl, httpUrl]` or something like that.

::: toolkit/components/extensions/schemas/extension_protocol_handlers.json:25
(Diff revision 6)
> +                "mms", "news", "nntp", "sip", "sms", "smsto", "ssh", "tel",
> +                "urn", "webcal", "wtai", "xmpp"
> +              ]
> +            }, {
> +              "type": "string",
> +              "pattern": "^(ext|web)\\+.*"

s/\.\$/[a-z0-9.+-]+$/
Attachment #8833708 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 20

a month ago
(In reply to Kris Maglione [:kmag] from comment #18)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> > This feels like a separate bug, can we do a followup on the permissions
> > system for that?
> 
> I'd rather not land this with it adding an artificial "protocolHandlers"
> permission, especially if we're also adding a locale string for it at the
> same time.

I've had second thoughts on having the permission at all.  The user will have to a) install an addon, b) click on a link with the protocol, c) select the addon in a dialog to handle the protocol, and possibly c2) select it to be default.

Because of step c, it feels unnecessary to deal with the permission hurdle right now.  If we add a way to make a default protocol, then we will definitely need to deal with the permission, in which case an explicit permission (as was originally done) may be better.
Comment hidden (mozreview-request)
(Assignee)

Comment 22

a month ago
mozreview-review-reply
Comment on attachment 8833708 [details]
Bug 1310427 support protocol handlers,

https://reviewboard.mozilla.org/r/109888/#review115678

> Hrm. So, normally we'd call call this permission "manifest:protocol_handlers", which is how the `hasPermission` method handles it. But property files treat ":" the same way they treat "=", so that won't work.
> 
> But if we're going to go this route, I'd rather add all manifest keys that act as permissions to the permissions list rather than doing the check in `hasPermission`, and then have the permissions UI code do whatever mangling is necessary to get the right string bundle key.

Skipping on a permission per comment in bug.
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
See Also: → bug 1342133

Comment 24

a month ago
mozreview-review
Comment on attachment 8833708 [details]
Bug 1310427 support protocol handlers,

https://reviewboard.mozilla.org/r/109888/#review116626

r=me with issues fixed.

::: toolkit/components/extensions/schemas/extension_protocol_handlers.json:30
(Diff revision 8)
> +              "pattern": "^(ext|web)\\+[a-z0-9.+-]+$"
> +            }]
> +          },
> +          "uriTemplate": {
> +            "description": "The URL of the handler, as a string. This string should include \"%s\" as a placeholder which will be replaced with the escaped URL of the document to be handled. This URL might be a true URL, or it could be a phone number, email address, or so forth.",
> +            "type": "string",

"type" and "choices" are mutually exclusive.

::: toolkit/components/extensions/schemas/manifest.json:230
(Diff revision 8)
>        },
>        {
> +        "id": "HttpURL",
> +        "type": "string",
> +        "format": "url",
> +        "pattern": "^(https?)://.*$",

No need for parens.

::: toolkit/components/extensions/schemas/manifest.json:231
(Diff revision 8)
>        {
> +        "id": "HttpURL",
> +        "type": "string",
> +        "format": "url",
> +        "pattern": "^(https?)://.*$",
> +        "preprocess": "localize"

This should be on the property itself, not the base type.

::: toolkit/components/extensions/test/mochitest/test_ext_protocolHandlers.html:26
(Diff revision 8)
> +    manifest: {
> +      "protocol_handlers": [
> +        {
> +          "protocol": "ext+foo",
> +          "name": "a foo protocol handler",
> +          "uriTemplate": "foo.html?val=%s",

Need to test that this works with http: and https: URLs too.

::: toolkit/components/extensions/test/mochitest/test_ext_protocolHandlers.html:137
(Diff revision 8)
> +  let loaded = yield extension.startup().then(() => {
> +    extension.unload();
> +    return true;
> +  }).catch(() => {
> +    return false;
> +  });

`yield Assert.rejects(extension.startup(), ...)`

::: toolkit/components/extensions/test/mochitest/test_ext_protocolHandlers.html:151
(Diff revision 8)
> +          "protocol": "http",
> +          "name": "take over the http protocol",

This should be a valid protocol so we're actually testing the `uriTemplate`. Would be good to also test the error message.

::: toolkit/components/extensions/test/mochitest/test_ext_protocolHandlers.html:160
(Diff revision 8)
> +  let loaded = yield extension.startup().then(() => {
> +    extension.unload();
> +    return true;
> +  }).catch(() => {
> +    return false;
> +  });
> +  ok(!loaded, "unable to register restricted handler uriTemplate");

Same here.
Attachment #8833708 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)

Comment 26

a month ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/704db7ae2d85
support protocol handlers, r=kmag
(Assignee)

Updated

a month ago
Depends on: 1342577

Comment 27

a month ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/975fd16eef79
support protocol handlers: disable test on Android (bug 1342577). r=test-disabling-to-fix-orange-as-requested-by-developer

Comment 28

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/704db7ae2d85
https://hg.mozilla.org/mozilla-central/rev/975fd16eef79
Status: REOPENED → RESOLVED
Last Resolved: 4 months agoa month ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

a month ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.