STR:

1. Load attached extension, e.g. at about:debugging
2. Click on the extension button to trigger a download. Confirm the download if asked.

Expected:

• The extension button should show "OK" (=webRequest event intercepted the request).

Actual:

• The extension button shows "FAIL" (=download completed without firing webRequest event).

This is not a regression and still happens on the latest Nightly (71).
This works in Chrome 76.

Being able to observe download requests through the webRequest API allows extensions to reliably detect the response headers and inspect/modify its response.
Extensions can already observe and modify requests that they created through <a download>, so not being able to do the same with downloads created through downloads.download is a bug.

If anyone is interested in fixing this:

1. Downloads are created here: https://searchfox.org/mozilla-central/rev/8ecd15049c11eb753717a6631213ba5ecc90e123/toolkit/components/extensions/parent/ext-downloads.js#762-773
2. which calls Downloads.createDownload
3. which calls Download.fromSerializable.
4. Since the "saver" property is not set, it defaults to "copy",
5. which is handled by DownloadCopySaver.fromSerializable
6. ... which creates downloads with the system principal: https://searchfox.org/mozilla-central/rev/8ecd15049c11eb753717a6631213ba5ecc90e123/toolkit/components/downloads/DownloadCore.jsm#2049

To make sure that extensions can see their own requests, this should be created with the calling extension's principal instead. This could be done by forwarding context.principal from step 1, all the way through step 6, and forward that principal to NetUtil.newChannel via loadingPrincipal.

This bug has a test case and a description of how to fix this. To make sure that this feature works as intended, a unit test should be added, for example by adding a test after https://searchfox.org/mozilla-central/rev/250f5cc9fb8bdcbb6b23d2a06acfd48addb2f99b/browser/components/extensions/test/browser/browser.ini#275

Documentation on getting started to contributing to the WebExtensions API implementation is available at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

To make sure that extensions can see their own requests, this should be created with the calling extension's principal instead.

webRequest should see ALL downloads, not just downloads initiated by the extension. The use cases are adblockers or antivirus tools that would want to block and/or modify certain downloads.

(In reply to Nils Maier [:nmaier] from comment #2)

To make sure that extensions can see their own requests, this should be created with the calling extension's principal instead.

webRequest should see ALL downloads, not just downloads initiated by the extension. The use cases are adblockers or antivirus tools that would want to block and/or modify certain downloads.

webRequest does already see all downloads by web pages (including <a download> by the extension itself).

The bug here is that webRequest does not see downloads initiated by browser.downloads.download, and the cause of that is that the implementation does not forward the contextual information (i.e. the extension's principal) to allow the downloads implementation to make that decision.

I'm probably being remarkably dense here and falling at the first hurdle, but where is the "attached extension" mentioned in the STR?

(In reply to Neil Bleasdale [:nbleasdale] from comment #4)

I'm probably being remarkably dense here and falling at the first hurdle, but where is the "attached extension" mentioned in the STR?

Here it is. Thanks for pointing this out.

Hey Rob, can i work in this bug ?

Hi Ajitesh, thanks for your interest in this bug!

You're currently working on another good-first-bug. Let me guide you through that bug before assigning a new good-first-bug.

Hey Rob,
Can I work on this bug?

(In reply to sanjananayar2k1 from comment #8)

Hey Rob,
Can I work on this bug?

Yes, you can. comment 0 and comment 1 describe the issue and the necessary steps to resolve the issue. Try to understand the issue and proposed steps, and once you've got an idea on how to write the patch, set up a development environment as explained in https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

If you have questions, feel free to write a comment here or contact me (e.g. by e-mail).

Thanks!
Please give me some time, as I am a beginner.

Can i submit a patch in this bug ?

Sure, go ahead and submit a patch via Phabricator. The process to do so is documented on the wiki page linked from comment 9.

Do i need to omit the loadUsingSystemPrincipal and instead use loadingPrincipal & loadingNode in the NetUtil.newChannel . Or there any other way to trigger the extension's principal. Can you tell me, what is the difference between system's principal and extension's principal?

(In reply to Ajitesh from comment #13)

Do i need to omit the loadUsingSystemPrincipal and instead use loadingPrincipal & loadingNode in the NetUtil.newChannel . Or there any other way to trigger the extension's principal.

Yes. But only when an (extension) principal is set (forwarded from ext-downloads.js).

Can you tell me, what is the difference between system's principal and extension's principal?

The system principal has a higher privilege than extension principals. To prevent extensions from modifying higher-privileged requests, the webRequest API hides such requests from extensions.
Since the downloads API generates network requests on behalf of the extension, this limitation is too strict, and to fix that, the extension principal should be used instead of the system principal.

I guess the url property of the DownloadSourceObject of Downloads.createDownload needs to be modified to forward the extension principal from ext-downloads.js. Can you please tell me the functioning of source.adjustChannel = adjustChannel; ? I think this may be involved in some way with the corresponding download principal.

(In reply to Ajitesh from comment #15)

I guess the url property of the DownloadSourceObject of Downloads.createDownload needs to be modified to forward the extension principal from ext-downloads.js.

The url property shouldn't be modified. You have to assign the extension's principal to the source dictionary, and use that with NetUtil.newChannel.

Can you please tell me the functioning of source.adjustChannel = adjustChannel; ? I think this may be involved in some way with the corresponding download principal.

adjustChannel is used to modify the request after its initialization. You shouldn't use this. The approach that you described in comment 13 looks correct (but use loadingPrincipal, not loadingNode).

https://searchfox.org/mozilla-central/rev/8ecd15049c11eb753717a6631213ba5ecc90e123/toolkit/components/downloads/Downloads.jsm#75-86 specifies the parameters of source. I cant able to find any parameter that can directly pass context.principal from a new Download Object, from ext-downloads.js

That piece of documentation is out of date. The source code is the source of truth:

1. In Searchfox, click on the "fromSerializable" method in the function. That will show a panel with various search links. Click on it (Search for substring fromSerializable)"
2. In the search results, click on Download.fromSerializable (because it matches the function name that you were looking for).
3. Look how the source property of the parameter is handled. You will see that it is handled by DownloadSource.fromSerializable.
4. If you follow the similar steps as in step 1-2, but with the DownloadSource.fromSerializable method from step 3, then you will find how the source is parsed, along with more up-to-date documentation.

Once you understand how the source object is parsed and used, adding the principal to that object should be doable.

I looked at this issue.
See also this.
When you change to loadingPrincipal instead of loadUsingSystemPrincipal, you should look at the parameter values.
Exceptions[0] occur if you simply change the parameters.
So, you should be more considerate about parameter[1](e.g, securityFlags).

[0]
https://searchfox.org/mozilla-central/rev/174f1195ec740e8f17223b48018f7e14e6d4e40e/netwerk/base/NetUtil.jsm#225

[1]
https://searchfox.org/mozilla-central/rev/8ecd15049c11eb753717a6631213ba5ecc90e123/toolkit/components/downloads/DownloadCore.jsm#2049

In the source dictionary, i think adding the referrerInfo would help (in the ext-downloads.js). But now the download is failing. Do i need to pass something else into referrerInfo ?

i tried this:

ext-downloads.js:

const source = {
                url: options.url,
                isPrivate: options.incognito,
                referrerInfo: options.referrerInfo,
              };

and set loadingPrincipal: true, in NetUtil.newChannel but the download keeps failing.

Right now i am busy in some works so new contributors are welcomed to take this bug. Read the above discussions to follow the issue and if stuck ask the mentor @Rob he will help you out.

I think am I going to take this up, I am very close to submitting a patch.

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

That piece of documentation is out of date. The source code is the source of truth:

1. In Searchfox, click on the "fromSerializable" method in the function. That will show a panel with various search links. Click on it (Search for substring fromSerializable)"
2. In the search results, click on Download.fromSerializable (because it matches the function name that you were looking for).
3. Look how the source property of the parameter is handled. You will see that it is handled by DownloadSource.fromSerializable.
4. If you follow the similar steps as in step 1-2, but with the DownloadSource.fromSerializable method from step 3, then you will find how the source is parsed, along with more up-to-date documentation.

Once you understand how the source object is parsed and used, adding the principal to that object should be doable.

I followed the steps you mentioned on both fromSerializable method and the DownloadSource.fromSerializable. I found that the source dictionary can only have properties that are mentioned here.
I am struggling a tad bit with adding the principal to the source.
Also on reading about the NetUtil and loadingPrincipal from here , I think instead of setting loadingPrincipal:true , we need to assign the loadingPrincipal in a similar way as the system principal is assigned here.
Please guide me.

(In reply to Harsh from comment #24)

I followed the steps you mentioned on both fromSerializable method and the DownloadSource.fromSerializable. I found that the source dictionary can only have properties that are mentioned here.
I am struggling a tad bit with adding the principal to the source.

You can add the principal as a new member of the DownloadsSource. fromSerializable and toSerializable must properly (de)serialize the value though, which you can do with the E10SUtils.serializePrincipal and E10SUtils.deserializePrincipal helper methods.

Also on reading about the NetUtil and loadingPrincipal from here , I think instead of setting loadingPrincipal:true , we need to assign the loadingPrincipal in a similar way as the system principal is assigned here.

Your understanding is correct - loadingPrincipal should be assigned a nsIPrincipal, not a boolean.
Looking at NetUtil.jsm helps with understanding how the value is used, but after that you should look into modifying DownloadCore.jsm, to change how NetUtil.newChannel is called. To emphasize: There is no need to modify NetUtil.jsm itself.

Depends on D63104

Hi Rob,

I have submitted a patch for this bug. Although my download still fails, I think I am close.
Please review and let me know where I am going wrong.
Also, I have accidentally submitted the patch before marking a previous open patch abandoned. I have now marked that patch abandoned.

Best
Harsh

(In reply to Harsh from comment #27)

I have submitted a patch for this bug. Although my download still fails, I think I am close.
Please review and let me know where I am going wrong.

I've posted a review on your patch. If you have failures that you cannot resolve, sharing the code, command and error output would allow me to look into your issue and provide better help.

Hey Harsh, how's it going with this bug?

(In reply to Caitlin Neiman [:caitmuenster] from comment #29)

Hey Harsh, how's it going with this bug?

Hi Caitlin
I am currently working on writing unit tests for this bug.
I recently talked to Rob via email and got a few pointers on how to proceed with that.
Once I have something, I will update the revision on phabricator.
Best,
Harsh

Hey Harsh! Just wanted to check in on how the unit tests are coming along for this bug.

We have had a conversation over email where I answered some questions about writing tests.

Hi Rob,
I had left a couple queries on phabricator to your suggestions. Could you look at them and provide a few pointers on how I should proceed?
Best,
Harsh

Answered in the patch, thanks for the reminder.

https://hg.mozilla.org/mozilla-central/rev/02deda62a783