1298215 - Provide an API to show the downloads manager

Status: REOPENED

(WebExtensions :: Frontend, defect, P3)

Description

[Shane Caraveo (:mixedpuppy)]

tab.create fails opening about:downloads.  I guess this is similar to bug 1261289.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Use case would be a newtab override (bug 1234150) that wants to provide links to open various browser controls such as an about:history (doesn't exist yet) or about:downloads page.

Comment 2

[Kris Maglione [:kmag]]

This is intentional. Extensions can open unprivileged about: pages that can be opened by ordinary web pages, but there are serious security and compatibility implications with allowing them to open privileged pages.

Comment 3

[Andy McKay]

Could you outline those concerns?

Comment 4

[Kris Maglione [:kmag]]

(In reply to Andy McKay [:andym] from comment #3)
> Could you outline those concerns?

Not as well as some people. There are various security bugs related to the restrictions preventing web content from loading those pages, and some that I don't have access to about the more stringent restrictions that we've added recently.

The biggest security issues tend to come from the side-effects that those pages often have, especially when passed query parameters, and other issues related to click-jacking.

Even with security issues aside, there are compatibility issues. The UI and URLs of those pages often change. about:downloads exists now, and currently opens in a tab, but that hasn't always been the case, and probably won't always be the case. So if we want to give extensions the ability to open those kinds of pages, we should do it via some kind of appropriate UI method rather than giving them the ability to arbitrarily load the URLs.

Comment 5

[Andy McKay]

Feels like bug 1227462 is relevant here.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Given that we don't need to be 100% compat, I think an alternate UI api to do these things is a fine approach.

If we do eventually want to do away with old school addons, we will need some of these things for our partner distributions.

Comment 7

[Daniel Veditz [:dveditz]]

I'm not entirely sure what Andy was asking me. People install extensions to do things that mere web pages cannot; limiting what they can do to what a web page can do is going to leave people disappointed. Maybe we need to add a permission that allows a Web Extension to enumerate the non-web-accessible URLs it wants to open. We may also decide there are some URLs that are "safe enough" even though we don't let web pages use them. I hope view-source: falls into that category (bug 1261289), and "safe" about: urls (formerly openable by web pages) might. The download manager runs at a higher privilege level so I'd want to have a permission for that one.

An API to "show the download manager" is different than the ability to open/use an arbitrary URL. It's more limited, which has its plusses and minuses. The plusses are on the security side--less for me to worry about--but might not be as flexible for extension authors.

Comment 8

[Andy McKay]

Sorry, in comment 4, Kris addresses security concerns that he doesn't have access to each which leaves us kind of guessing what to do. If you'd rather not talk about that in this bug (or make it security sensitive, that's cool). 

Agree with your comments, an API to show the downloads manager sounds cool but what access to the window or tab do I get after that? Do we need more than just showing it? It feels like if we limit to certain actions like, show a window, we'll get into a rabbit hole of more and more requests.

A permission feels like a cop out though, we've said all these things might be a problem and just stuffing them behind a permission and pushing that risk to the user. 

So my first guess is let's make an explicit permission, then poke holes in that for certain URLs that are safe enough and see if we can take baby steps towards something we can use, but again those vague security worries concern me.

Comment 9

[Kris Maglione [:kmag]]

(In reply to Andy McKay [:andym] from comment #8)
> Agree with your comments, an API to show the downloads manager sounds cool
> but what access to the window or tab do I get after that? Do we need more
> than just showing it? It feels like if we limit to certain actions like,
> show a window, we'll get into a rabbit hole of more and more requests.

Giving any direct access to about:downloads, aside from the ability to open it
in a tab, is completely out of the question. It's fully chrome-privileged, so
any direct access would be a huge privilege escalation.

But I'm not a fan of even allowing them to load the URL directly. That ties
our hands the next time we want to change the implementation. It means that we
can't remove about:downloads, we have to continue allowing it to open in a new
tab, we can't prevent extensions from opening multiple new copies rather than
just focusing an existing one, we can't change the default behavior to open it
in, say, a sidebar or a panel.

For the case of opening the download manager, or other things like
about:addons or about:preferences, an API is the obvious choice. There may be
other cases where we might want to allow direct access to open the URLs, but I
can't think of any immediately obvious examples.

Comment 10

[Shane Caraveo (:mixedpuppy)]

I'm not clear on why tab.create would result in elevated privileges (and of course we're not looking to do that).  I see there is executeScript, which seems blockable for privileged pages.  There is message passing, which seems like something that shouldn't need to be worried about (ie. if about:downloads supported message passing, why wouldn't we allow an addon talk to it?).

Compatibility over concern about special handling of tabs/urls for privileged content pages seems worth thinking about.  eg. We could prevent multiple tabs and ignore options (sidebar/panel/etc) of certain pages if it makes sense.  

Regarding removal of content pages; In some of these cases it's hard to imagine the functionality not existing, but you never know.  I've always had the impression that we're moving towards content pages rather than away.  In that case, we shouldn't be a afraid of making a page that says "oops, addon xyz tried opening a page that does not exist in Firefox."  That page could even be an opportunity to direct users to getting an addon that may substitute the features we removed.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> I'm not clear on why tab.create would result in elevated privileges (and of
> course we're not looking to do that).  I see there is executeScript, which
> seems blockable for privileged pages.  There is message passing, which seems
> like something that shouldn't need to be worried about (ie. if
> about:downloads supported message passing, why wouldn't we allow an addon
> talk to it?).

I'm not especially worried that just opening the tab will cause security
issues, unless we also allow query arguments or the like. Things like
`executeScript`, or loading into a subframe, are more of a concern. I wouldn't
be surprised if other features of the tabs API caused issues, but they're not
really my primary concern.

> Compatibility over concern about special handling of tabs/urls for
> privileged content pages seems worth thinking about.  eg. We could prevent
> multiple tabs and ignore options (sidebar/panel/etc) of certain pages if it
> makes sense.  

We could, but if we're going to give it special handling anyway, I think it
makes much more sense to just provide an API to open it, potentially with
additional features, such as highlighting a specific download.

> Regarding removal of content pages; In some of these cases it's hard to
> imagine the functionality not existing, but you never know.  I've always had
> the impression that we're moving towards content pages rather than away.

We are now, but there's no telling how long that will be the case, or how long
before we decide to integrate the downloads manager into some other content
page, or UI feature, or remove support for the in-content version in favor of
just the panel or a sidebar.

We could try to work around these things by letting add-ons just open a tab
with about:downloads and then implementing some kind of special behavior, but
I can't think of a good reason that we'd want to do that rather than just
provide an API that Does the Right Thing regardless of our current
implementation.

Comment 12

[Andy McKay]

(In reply to Kris Maglione [:kmag] from comment #11)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> > I'm not clear on why tab.create would result in elevated privileges (and of
> > course we're not looking to do that).  I see there is executeScript, which
> > seems blockable for privileged pages.  There is message passing, which seems
> > like something that shouldn't need to be worried about (ie. if
> > about:downloads supported message passing, why wouldn't we allow an addon
> > talk to it?).
> 
> I'm not especially worried that just opening the tab will cause security
> issues, unless we also allow query arguments or the like. Things like
> `executeScript`, or loading into a subframe, are more of a concern. I
> wouldn't
> be surprised if other features of the tabs API caused issues, but they're not
> really my primary concern.

We could probably get a list of those APIs that would cause problems. Of course things like content scripts would be way out. Its worth noting that you can already get things like the about:addons tab and interact using our APIs, so perhaps we could harden that as well.

> We could, but if we're going to give it special handling anyway, I think it
> makes much more sense to just provide an API to open it, potentially with
> additional features, such as highlighting a specific download.

Additional features for downloads would be great, maybe that would be a seperate bug though.

> We could try to work around these things by letting add-ons just open a tab
> with about:downloads and then implementing some kind of special behavior, but
> I can't think of a good reason that we'd want to do that rather than just
> provide an API that Does the Right Thing regardless of our current
> implementation.

My main concern there is that you end up duplicating a whole API off into another API. In the end if there's a tabs.createDownloads or tabs.createAddonManager, I would have no problem with that, since we aren't compatible with Chrome anyway.

Comment 13

[Kris Maglione [:kmag]]

(In reply to Andy McKay [:andym] from comment #12)
> > We could try to work around these things by letting add-ons just open a tab
> > with about:downloads and then implementing some kind of special behavior, but
> > I can't think of a good reason that we'd want to do that rather than just
> > provide an API that Does the Right Thing regardless of our current
> > implementation.
> 
> My main concern there is that you end up duplicating a whole API off into
> another API. In the end if there's a tabs.createDownloads or
> tabs.createAddonManager, I would have no problem with that, since we aren't
> compatible with Chrome anyway.

It think it would be more like `downloads.showDownloads({...})` or
`management.openOptionsPage({extensionId})` (ala `runtime.openOptionsPage`),
or `bookmarks.openBookmarkManager({folderId})`, where the methods would be
part of the appropriate API, and follow the same conventions as the rest of
that API.

Comment 14

[Shane Caraveo (:mixedpuppy)]

I think I like the use of the appropriate api especially as that provides the ability to open non-tab based ui without being weird.  However, that creates a need for an api call for each item we might want to expose, rather than a generic ability to expose pages via a single api call.  I'm on the fence basically, I have a need to support certain pages for a partner (some which may not have content pages yet) and don't mind specific APIs, but I prefer a more open system to avoid additional effort (like this bug) down the road.

Comment 15

[Andy McKay]

I'm happy with the suggestion of the API in comment 13. We'll probably need to file bugs for all the other pages too.

Comment 16

[Thomas Wisniewski]

Attachment: 1298215-add-browser.downloads.showDownloads-web-extension-api.diff

Not meaning to step on your toes, Shane (since you're assigned to this bug), but I took a stab at making a patch here.

I'm not sure if it's anywhere near good enough, but if it's reasonably close then I don't mind helping to get it across the finish line (as well as similar bugs that we've yet to file). Thoughts?

Comment 17

[Andy McKay]

Paul, Thomas is attaching a patch that fixes bug 1371793. That bug has some more details on it, but I think we need some security help to walk through the questions around potential sandbox escaping.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Hi Thomas.  My toes are delicate, if you were stepping on them I'd know.  Happy to have you take this, lets see how comment 17 resolves before doing more.

Comment 19

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(clearing need info since I commented on bug 137193 instead).  FWIW I think this patch looks good to me though.

Comment 20

[Shane Caraveo (:mixedpuppy)]

This dropped off our radars.  Thomas, did you want to pick it up again?

Comment 21

[Thomas Wisniewski]

Attachment: Bug 1298215 - Add an extension API browser.downloads.showDownloads() to show about:downloads in a new tab;

Review commit: https://reviewboard.mozilla.org/r/241588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241588/

Comment 22

[Thomas Wisniewski]

Alright. I've rebased this, added Fennec support, and added a mochitest.

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e70e7a5d7c37041f1799e9bf57a6b9ae4a95d3a

Comment 23

[Code Review Bot [:reviewbot]]

Comment on attachment 8973046 [details]
Bug 1298215 - Add an extension API browser.downloads.showDownloads() to show about:downloads in a new tab;

https://reviewboard.mozilla.org/r/241588/#review247398


Code analysis found 8 defects in this patch:
 - 8 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-downloads.js:839
(Diff revision 1)
>          }).api(),
>  
>          onDeterminingFilename: ignoreEvent(context, "downloads.onDeterminingFilename"),
> +
> +        showDownloads(options) {
> +          Tab.openActiveInternalUrl(extension, "about:downloads", (options || {}).windowId, "downloads");

Error: 'tab' is not defined. [eslint: no-undef]

::: toolkit/components/extensions/parent/ext-tabs-base.js:756
(Diff revision 1)
>     */
>    removeCSS(context, details) {
>      return this._execute(context, details, "css", "removeCSS").then(() => {});
>    }
> +
> +  /**

Error: Missing jsdoc @returns for function. [eslint: valid-jsdoc]

::: toolkit/components/extensions/parent/ext-tabs-base.js:756
(Diff revision 1)
>     */
>    removeCSS(context, details) {
>      return this._execute(context, details, "css", "removeCSS").then(() => {});
>    }
> +
> +  /**

Error: Missing jsdoc for parameter 'requiredpermission'. [eslint: valid-jsdoc]

::: toolkit/components/extensions/parent/ext-tabs-base.js:759
(Diff revision 1)
>    }
> +
> +  /**
> +   * Opens an active tab to an internal link, like about:downloads.
> +   *
> +   * @param {string} url

Error: Expected jsdoc for 'extension' but found 'url'. [eslint: valid-jsdoc]

::: toolkit/components/extensions/parent/ext-tabs-base.js:761
(Diff revision 1)
> +  /**
> +   * Opens an active tab to an internal link, like about:downloads.
> +   *
> +   * @param {string} url
> +   *        The internal url to open.
> +   * @param {integer} [windowId = null]

Error: Expected jsdoc for 'url' but found 'windowid'. [eslint: valid-jsdoc]

::: toolkit/components/extensions/parent/ext-tabs-base.js:773
(Diff revision 1)
> +      if (requiredPermission && !extension.hasPermission(requiredPermission)) {
> +        return Promise.reject({message: `No permission for ${requiredPermission}`});
> +      }
> +
> +      let window = windowId !== null ?
> +        windowTracker.getWindow(windowId, context) :

Error: 'windowtracker' is not defined. [eslint: no-undef]

::: toolkit/components/extensions/parent/ext-tabs-base.js:773
(Diff revision 1)
> +      if (requiredPermission && !extension.hasPermission(requiredPermission)) {
> +        return Promise.reject({message: `No permission for ${requiredPermission}`});
> +      }
> +
> +      let window = windowId !== null ?
> +        windowTracker.getWindow(windowId, context) :

Error: 'context' is not defined. [eslint: no-undef]

::: toolkit/components/extensions/parent/ext-tabs-base.js:774
(Diff revision 1)
> +        return Promise.reject({message: `No permission for ${requiredPermission}`});
> +      }
> +
> +      let window = windowId !== null ?
> +        windowTracker.getWindow(windowId, context) :
> +        windowTracker.topWindow;

Error: 'windowtracker' is not defined. [eslint: no-undef]

Comment 24

[Thomas Wisniewski]

Comment on attachment 8973046 [details]
Bug 1298215 - Add an extension API browser.downloads.showDownloads() to show about:downloads in a new tab;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241588/diff/1-2/

Comment 25

[Thomas Wisniewski]

Whoops, I botched that commit (forgot to add the test).

Here's a fresh patch which also addresses the eslint defects.

New try-run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c4b57870fb75ea2f0272ff5806b321d8b35d313

Comment 26

[Thomas Wisniewski]

It looks like the try-run only had unrelated failures.

Comment 27

[Shane Caraveo (:mixedpuppy)]

The current ui in Firefox opens the library window to the downloads.  I can't find any ui that opens about:downloads.  It seems like we should follow what primary ui in firefox is doing unless we know for certain that window will be removed in favor of about:* pages.

Comment 28

[Thomas Wisniewski]

Fair enough, at least on desktop (Fennec opens a tab with about:downloads). I'm a bit swamped, but I'll try to get around to making a fresh patch that opens the window instead on Desktop, unless someone with more insight chimes in to let us know that this is the way things will work on desktop soon anyhow.

Comment 29

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8973046 [details]
Bug 1298215 - Add an extension API browser.downloads.showDownloads() to show about:downloads in a new tab;

https://reviewboard.mozilla.org/r/241588/#review247968

::: toolkit/components/extensions/parent/ext-downloads.js:839
(Diff revision 2)
>          }).api(),
>  
>          onDeterminingFilename: ignoreEvent(context, "downloads.onDeterminingFilename"),
> +
> +        showDownloads(options) {
> +          Tab.openActiveInternalUrl(context, "about:downloads", (options || {}).windowId, "downloads");

We should use BrowserDownloadsUI here.  That should always do the right thing on desktop where about:downloads is used in private browsing, but the downloads window is otherwise used.  On android, it looks like we should use BrowserApp.selectOrAddTab.

::: toolkit/components/extensions/parent/ext-tabs-base.js:771
(Diff revision 2)
> +   *        Optional. The permission the extension must have to access the URL.
> +   *
> +   * @returns {Promise}
> +   * @static
> +   */
> +  static openActiveInternalUrl(context, url, windowId = null, requiredPermission = null) {

Nice but given the above comments, maybe not for this bug.

Comment 30

[Andrew Swan [:aswan]]

Sorry for the drive-by, but I would suggest limiting this API to only working when called from a user input handler.
That's a one-line in the API schema, eg:
https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/browser/components/extensions/schemas/browser_action.json#439-446

Comment 31

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973046 [details]
Bug 1298215 - Add an extension API browser.downloads.showDownloads() to show about:downloads in a new tab;

https://reviewboard.mozilla.org/r/241588/#review249932

::: toolkit/components/extensions/parent/ext-downloads.js:840
(Diff revision 2)
>  
>          onDeterminingFilename: ignoreEvent(context, "downloads.onDeterminingFilename"),
> +
> +        showDownloads(options) {
> +          Tab.openActiveInternalUrl(context, "about:downloads", (options || {}).windowId, "downloads");
> +        },

I agree with Shane that this API method should match the behavior of the native "Show All Downloads" button on Firefox Desktop and the "Downloads" menu button on Firefox for Android, possibly without re-implementing the behavior from scratch (which would risk to go out-of-sync without being noticed).

The behavior on Firefox Desktop and Android looks pretty similar, but the one on Android doesn't check the if the current window is private (because there is no concept of multiple windows in Firefox for Android), but it does check if the currently selected tab is private or not (and set the previosly selected tab, if any, as the parent of the new opened tab).

It looks that on Firefox Desktop the best way to match the behavior of the native button is to call the `BrowserDownloadsUI()` function from the currently selected chrome window global (or the specified one if there was a windowId explicitly specified in the API call), but on Firefox for Android the native button is implemented on the Java side:

- https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#4109-4112

- https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/mobile/android/base/java/org/mozilla/gecko/Tabs.java#1132-1156

One option to delegate to the Java side the "open the about:downloads" API method is to use the `EventDispatcher` (a messaging channel provided in Firefox for Android to be able to exchange messages between the privileged js code and the java components).

The browserAction are currently being registered/unregistered/updated using this strategy, and you can take a look at the following code fragments to see how to send a message from the JS privileged code to the Java components using `EventDispatcher.instance.sendRequest`:

- a piece of JS privileged code that sends a message to the Java listeners: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/mobile/android/modules/BrowserActions.jsm#68-73

- the Java code which registers/unregisters the listener and handles the received messages:
  - https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#892
  - https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1725
  - https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#2010

::: toolkit/components/extensions/schemas/downloads.json:695
(Diff revision 2)
>            }
>          ]
> +      },
> +      {
> +        "name": "showDownloads",
> +        "type": "function",

This should include `"requireUserInput": true` as suggested by aswan in Comment 30 (and it may also be reasonable to mark it as `"async": true`, e.g. like we did for `openPopup` in the browserAction/pageAction APIs).

Adding the "requireUserInput" in the schema will require also some changes to the test case, e.g. one more test case to check that the API rejects if it has not been called from a user input handler, and then to trigger it successfully from a use input handler), taking a look to the existent test case from the `openPopup` API method may be useful: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/browser/components/extensions/test/browser/browser_ext_openPanel.js#5

::: toolkit/components/extensions/schemas/downloads.json:703
(Diff revision 2)
> +          {
> +            "name": "options",
> +            "optional": true,
> +            "properties": {
> +              "windowId": {
> +                "description": "The window to open a new tab in.",

It would be nice to also mention in the description which is the default behavior if the optional windowId is not being specified.

Also, the windowId is not going to be useful on Firefox for Android, which doesn't have the window API at all, and so we should probably also mention that here.

I'm a bit on the fence about the behavior that an explicit windowId should have on Firefox for Android, e.g. 
should we just ignore the windowId parameter (and maybe log a warning)? or should we reject the API call?

Shane, what would be your opinion about this?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_showDownloads.html:38
(Diff revision 2)
> +      }
> +    });
> +  },
> +};
> +
> +add_task(async function test_webRequest_main_frame() {

The test description should be changed into something like `test_downloads_showDownloads`.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_showDownloads.html:42
(Diff revision 2)
> +  extension.sendMessage("showDownloads");
> +  await extension.awaitMessage("shown");
> +  ok(true, "about:downloads shown");
> +  extension.sendMessage("close");
> +  await extension.awaitMessage("closed");

I think that we should test the behavior also in the following scenarios:

- showDownloads called while the current active window is a private window
- showDownloads called with an explicit windowId which points to a private window, while a non-private window is currently selected
- showDownloads called with an explicit windowId which points to a non-private window, while a private window   is currently selected