Closed Bug 1669566 Opened 4 years ago Closed 3 years ago

browser.downloads.download() method only passes default container cookies

Categories

(WebExtensions :: Untriaged, enhancement, P3)

Product:
WebExtensions
Component:
Untriaged
Version:
Firefox 83
Type:
enhancement

Tracking

(firefox92 fixed)

Status:
RESOLVED FIXED
Milestone:
92 Branch
Tracking Flags:
Tracking Status
firefox92 --- fixed

People

(Reporter: karim, Assigned: karim, Mentored)

References

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(4 files)

Bug 1669566 - Add cookieStoreId option to browser.downloads APIs. r=robwu
47 bytes, text/x-phabricator-request
Details | Review
web_initiated test
1.50 KB, text/plain
Details
web_initiated test
1.50 KB, text/plain
Details
Bug 1669566 - Add cookieStoreId functionality to browser.download.download, browser.download.search, and browser.download.erase; add unit tests. r=robwu
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:81.0) Gecko/20100101 Firefox/81.0

Steps to reproduce:

In container x, set a cookie for host y. Then, call browser.downloads.download({url: y}) in a browser extension.

Actual results:

browser.downloads.download will use the cookies for host y in the default container, and not in container x. (As stated in the docs, "[i]f the specified url uses the HTTP or HTTPS protocol, then the request will include all cookies currently set for its hostname.").

Expected results:

I believe browser.downloads.download() should have an option to specify the container or 'cookieStoreId' which has the desired cookies.

Product: Firefox → WebExtensions

I want to mark this as a good-second-bug, but since that option doesn't exist I'll mark it as good-first-bug instead.
This bug is especially relevant for the Outreachy project because the ability to resolve this bug is a good predictor for the ability to succeed in the Outreachy project. Below are some hints towards fixing this bug. To get started, read the relevant extension documentation on MDN and see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp .

We should support cookieStoreId in the browser.downloads.download, browser.downloads.search and browser.downloads.erase APIs, with the following semantics:

The implementation of the downloads API is at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/parent/ext-downloads.js . To make it easier to find the code that you need to change, search for incognito and isPrivate.
Another part of the implementation is in the DownloadCore - https://searchfox.org/mozilla-central/source/toolkit/components/downloads/DownloadCore.jsm . Internally, you should be looking for the userContextId name.

Mentor: tomica
Severity: -- → N/A
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
Priority: -- → P3
Mentor: tomica → rob

Hey Rob.
I'm new here and I've had a small patch landed. I wanted to work on this and I thought it would be a good-second-bug, as you put it, for me.
I looked through the code and I think I get the general idea of what we're trying to do.
If cookieStoreId is passed, then we should use that to obtain a userContextId and then use that context instead of the default context. (default context is the object passed in getApi)

 getAPI(context) {
    let { extension } = context;
    return {
      downloads: {
        ...

Am I on the right track?

PS. I'm not an Outreachy intern, can I still work on this?

Flags: needinfo?(rob)

Thanks for your interest in this bug. I'd like to reserve this bug for Outreachy applicants for now. It is available to everyone if the bug is still open next month.

Flags: needinfo?(rob)

Hi Rob, I am an Outreachy applicant, I will like to work on this bug.

Hello, it's been a month since my original report. I guess this has opened for anybody to work on?

Hi Karim,

Feel free to take this bug.

(In reply to Kanishk from comment #2)

If cookieStoreId is passed, then we should use that to obtain a userContextId and then use that context instead of the default context. (default context is the object passed in getApi)

Yes, see comment 1 for details and links. Note that some of the cookieStoreId-related implementation is going to be moved around by https://phabricator.services.mozilla.com/D96151 , and maybe also bug 1675391.

PS. I'm not an Outreachy intern, can I still work on this?

Yes, this bug is now available to everyone.

Hi Rob,
So I was reading up on the docs, and here is what I've gathered.

  1. context == container == ContextualIdentity.
  2. cookieStoreId uniquely identifies a container.
  3. The 2 attributes that we're using from context in download, search, erase are: privateBrowsingAllowed and principal. We should get both these attributes from the container specified by cookieStoreId, if that's passed.

For search and erase, can we directly use contextualIdentities.get() which takes a cookieStoreId and skip obtaining userContextId?

Flags: needinfo?(rob)

(In reply to Kanishk from comment #8)

Hi Rob,
So I was reading up on the docs, and here is what I've gathered.

  1. context == container == ContextualIdentity.

"context" in "user context" is indeed a container, and management of those is exposed through the contextualIdentities extension API.

context that you see in the code is an instance of a subclass of BaseContext (list of subclasses). This "context" holds information about where the (extension) script is executing.

  1. cookieStoreId uniquely identifies a container.

Yes.

  1. The 2 attributes that we're using from context in download, search, erase are: privateBrowsingAllowed and principal. We should get both these attributes from the container specified by cookieStoreId, if that's passed.

This getUserContextIdForCookieStoreId utility (available as a global function in the ext-downloads.js file because this ext-tabs-base.js file is executed in the same scope) can be used to enforce some common checks: https://searchfox.org/mozilla-central/rev/919f7400e2e12bc963ee2ae8400ef961972c3059/toolkit/components/extensions/parent/ext-tabs-base.js#2260-2307
Search on Searchfox for other examples where it is used.

(ext-tabs-base.js is probably not the ideal location for this logic, but that's a concern for another bug)

For search and erase, can we directly use contextualIdentities.get() which takes a cookieStoreId and skip obtaining userContextId?

No, you cannot call the implementation of extension APIs from other implementations of the extension API.
Use the helper method that I mentioned before, or if needed use the helper methods from https://searchfox.org/mozilla-central/rev/919f7400e2e12bc963ee2ae8400ef961972c3059/toolkit/components/extensions/parent/ext-toolkit.js#35-100

Flags: needinfo?(rob)
Assignee: nobody → kanishk509
Status: NEW → ASSIGNED

We should support cookieStoreId in the browser.downloads.download

Had a doubt about the desired behaviour when we are in a container (say firefox-container-2).

We are adding support for passing a cookieStoreId explicitly to downloads.download, so should the download item be created with cookieStoreId = firefox-container-2 even if it's not explicitly passed?

Flags: needinfo?(rob)

If we are supposed to infer the cookie store to be used from the container we are in, how do we check the container (userContextId) from inside the downloads API (in ext-downloads.js) so that we can add it source?

(In reply to Kanishk from comment #11)

We should support cookieStoreId in the browser.downloads.download

Had a doubt about the desired behaviour when we are in a container (say firefox-container-2).

We are adding support for passing a cookieStoreId explicitly to downloads.download, so should the download item be created with cookieStoreId = firefox-container-2 even if it's not explicitly passed?

For simplicity, let's just use the default container. Ideally we should default to the current container, but that can be done in a follow-up bug/patch.
We are currently not even setting the isPrivate flag when the download is initiated in private browsing mode - bug 1653636.

Flags: needinfo?(rob)
See Also: → 1653636

Hey :robwu,

I think the patch is ready for review now. Can you take a look at it please?

Flags: needinfo?(rob)

(I've already posted a review)

Flags: needinfo?(rob)

Hey :robwu,

I was working on the changes and you were right, I should have tested if the download was actually picking up the correct cookies, because it is not.

As you can see in the partially-updated patch, I'm able to set the cookies for different containers. The download is picking up the cookies from firefox-default and firefox-private correctly based on if it is incognito.

However, when I'm passing the cookieStoreId of a container I created, the download is picking up the cookies from firefox-default instead.
It seems to me that the userContextId is being correctly set in ext-downloads.js and at DownloadCore.jsm (#1457) (since get cookieStoreId() is working properly) but the download is still picking up the cookies from the default container.

I tried it with the pre-defined containers like firefox-container-2 (work) etc. with the same results, it still picks up the cookies from the default container.

I couldn't find any other snippet of code in DownloadCore.jsm or Downloads.jsm that relates to handling cookie stores.

I've verified that the cookies and the new ContextualIdentity are being created correctly.

[{"name":"","value":"c_new_container","domain":"example.net","hostOnly":true,"path":"/","secure":false,"httpOnly":false,"sameSite":"no_restriction","session":true,"firstPartyDomain":"","storeId":"firefox-container-6"}]

Can you point me to where I should look to make further changes to the downloads implementation? Or if there's a problem with my test setup?

Flags: needinfo?(rob)

To start with, I suggest to trigger a download using the web APIs from a container tab.

The web API to trigger downloads is through an anchor element with the download attribute.

Then check if the browser.download.onChanged event is fired with the right cookieStoreId.

This should also be a unit test - web-initiated downloads should be associated with the right container.

Once you got that to work, it should be easier to compare the difference between the two.

Flags: needinfo?(rob)
Attached file web_initiated test — 
As you suggested, I've been trying to set up web-initiated downloads using the anchor element.

```javascript
Attached file web_initiated test — 
As you suggested, I've been trying to set up web-initiated downloads using the anchor element.

As you suggested, I've been trying to set up web-initiated downloads using the anchor element.

add_task(async function web_initiated() {
  async function containerScript() {
    window.onload = function() {
      browser.downloads.onCreated.addListener(download => {
        browser.test.log(`Download results: ${JSON.stringify(download)}`);
        browser.test.sendMessage("result", {
          status: "success",
          cookies: decodeURIComponent(download.mime.replace("dummy/", "")),
          cookieStoreId: download.cookieStoreId,
        });
      });

      document.getElementById("download_link").click();

      browser.test.sendMessage("test done");
    };
  }

  let extension = ExtensionTestUtils.loadExtension({
    useAddonManager: "temporary",
    manifest: {
      permissions: [
        "downloads",
        "*://example.net/*",
        "cookies",
        "contextualIdentities",
      ],
    },
    incognitoOverride: "spanning",
    files: {
      "container.html": `
        <!DOCTYPE html><meta charset="utf-8">
        <script src="container_script.js"></script>
        <a id="download_link" href="http://example.net/download" download>link</a>
      `,
      "container_script.js": containerScript,
    },
  });

  await extension.startup();

  let containerPage = await ExtensionTestUtils.loadContentPage(
    `moz-extension://${extension.uuid}/container.html`,
    { extension: extension, userContextId: null }
  );

  let result = await extension.awaitMessage("result");
  // check result

  await extension.awaitMessage("test done");
  await containerPage.close();
  await extension.unload();
});

(https://phabricator.services.mozilla.com/D96599)

But trying to open that download link this way is giving me the following error:
[JavaScript Error: "NetworkError when attempting to fetch resource." ...]

Using downloads.download() works fine in this container btw.

I've been stuck on this part for a while and I'm trying to debug it.
Any pointers?

(In reply to Kanishk from comment #20)

As you suggested, I've been trying to set up web-initiated downloads using
the anchor element.

Ignore comment #19 and #20.
There was some issue when I was trying to paste the code as patch.

Flags: needinfo?(rob)

I was away from work last week. Thanks for your patience.

In order to successfully download from a test, you need a server to download from. To ensure that tests are reliable and independent, the server should be part of the test. The "mochitest" test type includes a default test server (because the tests are served from a local web server), but in the "xpcshell" test type that you are using, you need to start the server yourself. The createHttpServer helper can be used, in combination with registerDirectory (if you want to serve local files), or registerPathHandler (recommended to make the test self-contained).

Although not the cause for the test failure, for download tests, you should also override the downloads directory so that the location is predictable. To do so you can copy this part: https://searchfox.org/mozilla-central/rev/66547980e8e8ca583473c74f207cae5bac1ed541/toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js#24-54

Another thing worth of note is that with the right preference set, browser.download.useDownloadDir=true, any triggered download is expected to happen immediately. If this pref is set incorrectly, a download dialog would be triggered. This pref is explicitly set to true at https://searchfox.org/mozilla-central/rev/66547980e8e8ca583473c74f207cae5bac1ed541/toolkit/components/extensions/test/xpcshell/xpcshell-common.ini#2-6

Flags: needinfo?(rob)

Hey rob,

I already have the downloads directory prefs set up exactly as you pointed out and I have also been using registerPathHandler as you suggested. (https://phabricator.services.mozilla.com/D96599)

server.registerPathHandler("/download", (request, response) => {
  response.setStatusLine(request.httpVersion, 200, "OK");

  let cookies = request.hasHeader("Cookie") ? request.getHeader("Cookie") : "";
  // Assign the result through the MIME-type, to make it easier to read the
  // result via the downloads API.
  response.setHeader("Content-Type", `dummy/${encodeURIComponent(cookies)}`);
  // response.setHeader("Content-Type", "text/html; charset=utf-8");

  // Response of length 7.
  response.write("1234567");
});

I have been able to narrow down the error a bit:
Not setting the proper content-type in the header in registerPathHandler gives a related error.
So I'm sure that using <a id="download_link" href="http://example.net/download" download>link</a> and doing document.getElementById("download_link").click() is at least triggering the path handler, but still running into the error "NetworkError when attempting to fetch resource.", whereas the download proceeds error-free when using downloads.download().

I have also been a bit busy with some college work, so haven't been able to work on this much recently. I'll tinker with it some more.

Flags: needinfo?(rob)

Could you share a version of your patch that triggers the error? NetworkError when attempting to fetch resource. does not sound like it should have been triggered by <a download>, as the error usually comes from scripting API, whereas <a download> is supposed to be handled through the browser.

If setting Content-Type causes issues, then can you start with trying to get the test to work at least to the point that you can detect that the download has happened at all? The detection of cookieStoreId can be done later.

(In reply to Kanishk from comment #24)

I have also been a bit busy with some college work, so haven't been able to work on this much recently. I'll tinker with it some more.

That's fine. Take your time :)

Flags: needinfo?(rob)

Could you share a version of your patch that triggers the error?

File test_ext_downloads_cookieStoreId.js in https://phabricator.services.mozilla.com/D96599. I've commented out the other test cases.

NetworkError when attempting to fetch resource. does not sound like it should have been triggered by <a download>

I think so too. I've confirmed that the error is independent of the download attribute in the anchor tag and also of browser.downloads.onCreated.addListener. It occurs in the absence of them too.
The error [JavaScript Error: "NetworkError when attempting to fetch resource." {file: "/home/kanishk509/src/mozilla-unified/testing/xpcshell/head.js" line: 239}] just points me to https://searchfox.org/mozilla-central/source/testing/xpcshell/head.js#239.

If setting Content-Type causes issues, then can you start with trying to get the test to work at least to the point that you can detect that the download has happened at all?

I suspect that the download may have been created, because the container page is receiving the data back from the handler and only throwing the encoding error (in absence of proper content-type header) when I try to navigate to that file in the container (by not using the download attribute).
But the test gets stuck on that error before I can perform any checks in the downloads directory.

Maybe I can navigate to the temp downloads directory in my file browser and check if the download file is created.

Flags: needinfo?(rob)

The test is likely not stuck on that error, the error is just the last thing that appears in the console.

One very suspicious bit in your code is the immediate attempt to trigger .click() after registering the downloads.onCreated event. The registration of extension APIs is asynchronous, so to ensure that the listener has been registered, you should try to make one roundtrip through the parent process, for example by a dummy call to another method of the downloads API.

I'll comment in the patch.

Flags: needinfo?(rob)

The test is likely not stuck on that error, the error is just the last thing that appears in the console.

You're right, it was just waiting for the message from inside the listener. It didn't cross my mind that the error might be non-blocking.

I experimented a bit and found that <a download> just isn't downloading the file and neither is it being picked up by the listener.

I tried doing it with <a download> vs downloads.download while keeping everything else exactly the same:

    // doesn't work
    document.getElementById("download_link").click();

    // works
    await browser.downloads.download({
      url: "http://example.net/download",
    });

    setTimeout(async () => {
      let items = await browser.downloads.search({});
      browser.test.log("No. of downloads found: " + items.length);
      for (let item of items) {
        browser.test.log(toString(item));
      }
    }, 5000);

    // Don't send the message and let this container page run without exiting
    // browser.test.sendMessage("test done");

downloads.download fires off the listener and creates the download file in the temp directory (under obj-x86_64-pc-linux-gnu/temp/xpc-profile-... which I manually checked) whereas <a download> does neither.

I wasn't sure how to wait upon the .click() initiated download so I just waited for 5 secs as a hack for now and also prevented the container page from closing. The download just isn't starting (but without the download attribute, it does try to display the file in the browser, based on the encoding errors as I mentioned in a previous comment).

As you mentioned earlier, browser.download.useDownloadDir is already true by default, so the download should start without showing a location picker. Can you think of anything else that might be preventing the download from starting?

(P.S. I have exams coming up so I might be inactive for a week. I'll pick this back up soon after that.)

Flags: needinfo?(rob)

One doubt I had was that the downloads API we are dealing with here is part of Extensions API right? And a web-initiated download, that we are trying to simulate, is a firefox feature independent of extensions. So will that download interact with downloads api as we expect? For example listening to onCreated etc.

<a download> and downloads.download should both be triggering downloads.

Perhaps it's easier to debug (or it would even solve the issue) if you use a mochitest instead of a xpcshell test. With a mochitest, the test runs in a browser and you can just use the browser's developer tools to debug the issue (sprinkle some long delays in the test to prevent the test from exiting/cleaning up).

To rewrite your xpcshell test to a mochitest:

Flags: needinfo?(rob)

Hey Rob,

I was in the process of converting the tests to mochitest, but unfortunately I haven't been able to give it much time. Due to some deadlines and projects coming up, I think I won't be able to work on this issue right now.
I'm not sure when I can start picking up Mozilla stuff again, so it would be good if you unassign me so someone else can take this up if they want to.
To summarize my progress/findings for anyone who picks this up:

  • The userContextId for the DownloadSorce object is being set correctly ext-downloads.js and passed to DownloadCore.jsm in the linked patch, and we are able to read that later to calculate the getter cookieStoreId().
  • But the download is still not being created with the correct cookies, as far as my xpcshell tests can tell (in the patch).
  • The filtering(using cookieStoreId) mechanism in the patch for search and query seems to be working fine.

Thank you for mentoring me through this, learnt a lot about the codebase, especially the testing frameworks. Sucks that I couldn't bring it to a close.

I hope to come back and pick up this(if it hasn't been picked by then)/other bugs again on Mozilla as soon as possible.

Flags: needinfo?(rob)
Status: ASSIGNED → NEW

Thanks for getting back and for the lot of progress in your patch. The code looked very reasonable, but didn't work for some reason.

I hope to see you back at some point, but if not - thank you for your contributions so far :)

Assignee: kanishk509 → nobody
Flags: needinfo?(rob)
See Also: → 1698863

Hi Karim, congratz on your first patch at bug 1690613!

Are you interested in working on this bug too?

(In reply to Rob Wu [:robwu] from comment #33)

Hi Karim, congratz on your first patch at bug 1690613!

Are you interested in working on this bug too?

Hey Rob,

Many thanks for reviewing!

I was just looking at this bug: I am interested in patching it.

I need some guidance: DownloadSource.fromSerializable does parse a userContextId property, but implementations of DownloadSaver.execute do nothing with it. How am I supposed to bind the userContextId with the nsiChannels? This seems to be the issue that prevented Kanishk's patch from setting the correct cookies.

Flags: needinfo?(rob)

(In reply to karim from comment #35)

I need some guidance: DownloadSource.fromSerializable does parse a userContextId property,

I assume that you're referring to https://searchfox.org/mozilla-central/rev/aa67c5a592026cf42e15b02371259d808d086eb3/toolkit/components/downloads/DownloadCore.jsm#1500
If I use the blame feature of Searchfox (hover over the colored bar at the left), then I can see that the change was introduced in https://searchfox.org/mozilla-central/commit/3ab25176e5a3d3846c2321a212b29413400e1692 . From the diff at https://hg.mozilla.org/mozilla-central/rev/5d6bba3a , it is apparent that the userContextId is currently only used to support the ability to view downloads inline in a new tab in the browser (instead of e.g. saving to disk).

but implementations of DownloadSaver.execute do nothing with it. How am I supposed to bind the userContextId with the nsiChannels? This seems to be the issue that prevented Kanishk's patch from setting the correct cookies.

userContextId is one of the properties of the OriginAttributes dictionary/struct that is used to keep track of the context.

The channel is created by calling NetUtil.newChannel, at https://searchfox.org/mozilla-central/rev/aa67c5a592026cf42e15b02371259d808d086eb3/toolkit/components/downloads/DownloadCore.jsm#2328-2343
If I click through several calls, I end up at nsIOService::NewChannelFromURIWithProxyFlagsInternal, which constructs a LoadInfo instance for the given parameters.
LoadInfo reads the OriginAttributes from mLoadingPrincipal, at https://searchfox.org/mozilla-central/rev/aa67c5a592026cf42e15b02371259d808d086eb3/netwerk/base/LoadInfo.cpp#270

Notes:

You can either try to create a new loadingPrincipal with the desired OriginAttributes, or update channel.loadInfo.originAttributes after the channel has been created (channel.loadInfo.originAttributes is a dictionary; you would have to get the dictionary (and clone the contents?), set the userContextId property and then assign the value again).

Give it a try and let me know if it works as desired.

Flags: needinfo?(rob)
Assignee: nobody → karim
Status: NEW → ASSIGNED

Once the patch lands, then documentation needs to be updated for each affected API at https://github.com/mdn/content/tree/main/files/en-us/mozilla/add-ons/webextensions/api/downloads

and a new entry should be added to the compatibility table at https://github.com/mdn/browser-compat-data/blob/ff97bfe061015ff92e6e3d5322af50d1e0801b3b/webextensions/api/downloads.json

Keywords: dev-doc-needed
See Also: → 1721460
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/5d26d768e142
Add cookieStoreId functionality to browser.download.download, browser.download.search, and browser.download.erase; add unit tests. r=robwu,geckoview-reviewers,agi

https://hg.mozilla.org/mozilla-central/rev/5d26d768e142

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox92: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch
Regressions: 1722255

Documentation changes are ready for review:

Flags: needinfo?(rob)
Flags: needinfo?(rob)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: