Bug 704320 adds site-specified referrer policies (changes in what is sent and when for HTTP Referer), and we need to support that in Downloads.jsm. Before we can call that feature complete, we should make sure jsdownloads does the right thing. Specifically: 1. Resumed downloads must send the proper referrer (policy stored with download record) 2. "Open Download Page" continues to work (whole referrer is stored with the download)

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #0) > (policy stored with download record) More probably what we want to store are two strings, the "referrer to send" and the "download source page", rather than the policy value. We should use the existing "referrer" entry for one of the two, depending on whether we want to send the wrong referrer or go to the wrong download page when a download created on a newer version is restarted on an earlier version. I'm leaning towards storing the "referrer to send" in the "referrer" property, also in case add-ons use the value to create a channel.

(In reply to :Paolo Amadini from comment #1) > I'm leaning towards storing the "referrer to send" in the "referrer" > property, also in case add-ons use the value to create a channel. Currently, the referrer passed around in various gecko locations is the full referrer. The policy gets enforced at channel creation time (which is why we pass around pairs of referrer+policy). If we want to store the computed referrer+policy URI with the downloads, then we'll need to expose a "compute referrer given policy" function since right now all the logic is inside HttpBaseChannel::SetReferrerWithPolicy(). I'm not opposed to lifting it out and into something available through xpcom/JS, but first I'd like to understand a bit more about the needs for Downloads.jsm. Paolo: can you elaborate on the case where add-ons will use the referrer value to create a channel? If we store the policy along side the referrer, can't add-on developers just use SetReferrerWithPolicy when they create the new channel instead of SetReferrer?

Until now, the referrer policy handling has been transient, something handled only for the duration of a session. If we persist the information, however, there are a few more considerations to make. I think the decision should be based on a general analysis rather than which methods are currently available for getting the information. One consideration is backwards compatibility. Storing the "referrer to send" would allow previous versions to do the right thing when restarting downloads. When weighting this, however, it's worth considering that using one profile with multiple installations is uncommon, and old installations would send full referrers around anyways. Another is forwards compatibility. Storing the "referrer to send" would allow us to change how the policy is handled exactly in memory, in case we need to add more transformation logic than a simple integer. If the new logic requires more or different data (for example an existing load context), we may not be able to compute the final value anymore, or we would need to keep a compatibility/migration layer when reading the policy value for old downloads. This is the main reason for which I think the referrer to send is a better persistent piece of data than an enumeration value. The last consideration, add-ons, is a secondary one. My concern was about the consequence of inaction on existing add-on code, but this should probably affect add-ons using the referrer in memory as well, regardless of downloads. From a quick search in the Add-ons MXR, it does not seem to me that many add-ons consuming the Downloads.jsm API are currently extracting the referrer, although it used to be the case with the old API, since it did not have a method to restart a past download. There are a few exceptions, for example it looks like FlashGot has some code that extends Downloads.jsm and uses the referrer, but that code would need to be updated anyways.

(In reply to :Paolo Amadini from comment #3) > Another is forwards compatibility. Storing the "referrer to send" would > allow us to change how the policy is handled exactly in memory, in case we > need to add more transformation logic than a simple integer. If the new > logic requires more or different data (for example an existing load > context), we may not be able to compute the final value anymore, or we would > need to keep a compatibility/migration layer when reading the policy value > for old downloads. This is the main reason for which I think the referrer to > send is a better persistent piece of data than an enumeration value. This argument clinches it for me... I'll start working on lifting out the "apply policy" logic and try to figure out how we can get at it from JS. This will take a while longer than I initially expected (the policy-applying logic was not originally intended to be exposed to JS).

This patch adds "applyReferrerPolicy" to NetUtil.jsm so that we can see the effect of applying a Referrer Policy to a RequestURI/ReferrerURI pair by creating a channel, setting a referrer, and returning whatever gets set. We need this to calculate and store the "effective" referrer for Downloads.jsm so we can store it. Right now, all the code to calculate "effective" referrers is in C++ and since we won't be calling this function regularly, I think it's the best option that doesn't require hacking up an idl or duplicating logic in JS. mcmanus: I wasn't sure who to flag for these changes to NetUtil.jsm, but maybe you have an idea (or can review)?

Paolo: going through toolkit/components/jsdownloads, I'm having trouble figuring out how the referrer gets set on the downloads (specifically how DownloadSource gets configured) in the first place. Can you provide guidance of some other files/components I'll need to hack up? Especially, what code in toolkit/components/downloads is still used with the new Downloads.jsm implementation. Trolling through mxr looking for createDownload() and other related calls is taking a while and if you have pointers to help focus my search I'd appreciate it.

A representative case can be single-click downloads. These are normal page loads that are passed to nsIExternalHelperAppService (uriloader/exthandler) as soon as the Content-Disposition header is detected. The code implementing nsIExternalHelperAppService creates an nsITransfer, implemented in turn by the DownloadLegacy.js component. The old code in components/downloads is not involved. The code in uriloader/exthandler handles the data from the channel, and forward some state notifications through nsITransfer. DownloadLegacy.js forwards the notifications to a newly created Download object whose saver type is a DownloadLegacySaver. The DownloadLegacySaver reads the referrer from the existing channel lazily, when it's available: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#2338 Now, if the download is stopped and restarted, the saving method changes completely. The uriloader/exthandler code is not involved anymore, and the DownloadLegacySaver forwards everything to the DownloadCopySaver. The DownloadCopySaver is the one normally used by used by Downloads.createDownload(), where everything is expected to be provided by the caller before the download starts: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Downloads.jsm#createDownload%28%29 In this case, the DownloadCopySaver would use the referrer property to initialize its own channel: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#1914 Hope this helps!

NetUtil.jsm moved, so updating this patch. Carrying over r=mcmanus.

Thanks for the info, Paolo. It was indeed helpful, though I wasn't sure I found the right spots, but your description makes me think I did. Here's a first whack at a patch. No tests yet, but seems to work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff2980304129 I will work on making some unit tests.

Comment on attachment 8552572 [details] [diff] [review] bug1073187-downloadsjsm Review of attachment 8552572 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay! Comments below, also looking forward to seeing the tests. The tests about computing the right referrer based on policy are specific to DownloadLegacy, they can go in test_DownloadLegacy.jsm. The tests about setting the referrer properties with createDownload can go in test_DownloadCore.jsm. The existing "test_referrer" is currently in common_test_Download.js, but doesn't actually belong there because it doesn't have different behavior based on the gUseLegacySaver variable (we're executing the identical test twice). ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +1258,1 @@ > * referrer should be sent or the download source is not HTTP. nit: early line break, wrapping at 80 characters is preferred to keeping the last line unaltered @@ +1262,5 @@ > /** > + * String containing the full URI of the download source. Can be null if the > + * download source is not HTTP. > + */ > + downloadSource: null, The term "download source" denotes the file being downloaded. This would read as "myDownload.source.downloadSource" in calling code. I don't have any brilliant idea for the term we could use here. Maybe referringPage? This is where we should go when the user selects "Open Download Page" from the context menu (but of course any use of the term "download" is redundant here). @@ +1310,5 @@ > * Can be omitted or null if no referrer should be sent or > * the download source is not HTTP. > + * downloadSource: String containing the full URI of the download > + * source. Can be omitted null if the download source is not > + * HTTP. nit: alignment @@ +2357,5 @@ > if (aRequest instanceof Ci.nsIHttpChannel && aRequest.referrer) { > + // We store the "computed" referrer (what shows up in the HTTP Referer > + // header) in referrer, and the whole referrer regardless of a referrer > + // policy applied into downloadSource. > + var ref = NetUtil.applyReferrerPolicy(aRequest.uri, Please use "let" so that this is locally scoped. nit: let referrer = @@ +2360,5 @@ > + // policy applied into downloadSource. > + var ref = NetUtil.applyReferrerPolicy(aRequest.uri, > + aRequest.referrer, > + aRequest.referrerPolicy); > + this.download.source.referrer = ref; Per documentation and Downloads API convention, you should coerce empty string to null here ("referrer || null"). ::: toolkit/components/jsdownloads/src/DownloadImport.jsm @@ +144,5 @@ > let downloadOptions = { > source: { > url: source, > + referrer: referrer, > + downloadSource: referrer nit: feel free to add a comma on the last line of multi-line initializers in modified code (like in the initializer just below it). ::: toolkit/components/jsdownloads/src/Downloads.jsm @@ +79,5 @@ > * referrer: String containing the referrer URI of the download > * source. Can be omitted or null if no referrer should > * be sent or the download source is not HTTP. > + * This is the referrer in its final form: after any > + * policy is applied (see bug 704320). You may just reuse the wording from DownloadCore.jsm. The bug number isn't going to be particularly useful to people looking for the information, maybe consider adding a "(see nsIHTTPChannel.referrerPolicy)" in both places instead. @@ +82,5 @@ > + * This is the referrer in its final form: after any > + * policy is applied (see bug 704320). > + * downloadSource: String containing the full URI of the download > + * source. Can be omitted null if the download source > + * is not HTTP. whitespace at end of line

Ugh. I typed this up last week and forgot to hit submit before switching tabs. This may end up not doing exactly what we want because of the case Alex mentions in bug 1113431. https://bugzilla.mozilla.org/show_bug.cgi?id=1113431#c7 When we resume the download, if we don't have the referrer policy stored a cross-origin referrer may get dropped on connections from https to http (since the default is to drop the referrer, but some referrerPolicy values allow origin to be transmitted). Paolo: to make this correct, we probably have to store the referrer policy with the download and then re-apply it when we construct the new channel so we don't get too aggressive and strip the "already computed" referrer. What do you think? Another option is to hack up nsIHttpChannel and add SetPreciseReferrer() function or something that ignores all the rules about dropping referrer (I hate to do this, because it could potentially be buggy given user-set prefs).

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #11) > This may end up not doing exactly what we want because of the case Alex > mentions in bug 1113431. > > https://bugzilla.mozilla.org/show_bug.cgi?id=1113431#c7 Not sure I'm following, this comment seems to be talking about a resolved bug. > When we resume the download, if we don't have the referrer policy stored a > cross-origin referrer may get dropped on connections from https to http > (since the default is to drop the referrer, but some referrerPolicy values > allow origin to be transmitted). > > Paolo: to make this correct, we probably have to store the referrer policy > with the download and then re-apply it when we construct the new channel so > we don't get too aggressive and strip the "already computed" referrer. What > do you think? If you set the stored referrer using setReferrerWithPolicy with REFERRER_POLICY_UNSAFE_URL, instead of setting the "referrer" property, does this do what you need already? http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1043 We should make sure to have a test covering this case.

(In reply to :Paolo Amadini from comment #12) > > https://bugzilla.mozilla.org/show_bug.cgi?id=1113431#c7 > > Not sure I'm following, this comment seems to be talking about a resolved > bug. I should probably have linked to comment 5: https://bugzilla.mozilla.org/show_bug.cgi?id=1113431#c5 Policies like REFERRER_POLICY_UNSAFE_URL override the default "don't send referrer from https to http" behavior. If we store only the referrer as "sent" and not the policy, when we re-apply the referrer HttpBaseChannel will not do the overriding and omit the referrer entirely. Consider this case: https://source.com/foo triggers download at http://cdn.com/bar with referrer policy ORIGIN_WHEN_CROSSORIGIN. Normally, the effective referrer would be "https://source.com/", so we'd compute that and store it with the download. Later, when we resume the download, we call SetReferrer without a policy, but with the effective/computed referrer "https://source.com". HttpBaseChannel decides, since this is https->http, to omit the referrer. Our new effective referrer on download resume is empty (which is wrong). To get the override, I think we have to store the policy with the referrer, whether or not we compute it. Or we need HttpBaseChannel to have an UnsafeSetReferrerDirectly that ignores things like cross-origin referrer stripping. I'd much rather store the policy than make HttpBaseChannel more dangerous. Does this help clarify my concern?

(In reply to :Paolo Amadini from comment #12) > If you set the stored referrer using setReferrerWithPolicy with > REFERRER_POLICY_UNSAFE_URL, instead of setting the "referrer" property, does > this do what you need already? Sorry, forgot to add... this would work, but it seems a bit hacky to apply this policy to get what we want. If you like, I can implement this, but it feels weird and I'll probably put a few very detailed comments around this code to make it clear what's going on and why we're using that policy.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #13) > I should probably have linked to comment 5: > https://bugzilla.mozilla.org/show_bug.cgi?id=1113431#c5 This says: 1. UI code doesn't pass referrer policy to docshell 2. Links opened via the context menu get no referrer at all Is this a different issue you would like to discuss?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #13) > Or we need HttpBaseChannel to have an > UnsafeSetReferrerDirectly that ignores things like cross-origin referrer > stripping. I'd much rather store the policy than make HttpBaseChannel more > dangerous. > > Sorry, forgot to add... this would work, but it seems a bit hacky to apply > this policy to get what we want. If you like, I can implement this, but it > feels weird and I'll probably put a few very detailed comments around this > code to make it clear what's going on and why we're using that policy. Thanks for clarifying, and apologies if I don't get the relationship with the comments on the other bug yet. I still think we should store the referrer to send, and it's quite easy to go for the REFERRER_POLICY_UNSAFE_URL option with adequate comments, or you can go for the separate API. I have no particular preference over which one is used. In all cases the key point is having a test for the HTTPS to HTTP case that ensures the referrer is preserved across restarts. How we get there is an implementation detail that may even change in the future.

It should probably compute and store "referrer to send" at the time of the user's click. That's because origin-when-crossorigin also needs the triggering principal, which could well change if anything non-trivial happens during the download. One could, of course, persist that too... but it would likely be saner to store just the referrer itself, not all of the {referrerURI, referrerPolicy, triggeringPrincipal}.

MozReview-Commit-ID: 4Ttks5GjYQH

Comment on attachment 8925441 [details] [diff] [review] Add referrer policy support to Downloads.jsm Paolo, may I get your idea about the patch before going through the test? If I understand correctly, the coverage of this bug is described in comment 0, and we still keep the current behavior of downloads the same (Save link as for example, though I see we ignore the referrer in some cases), or we will do that in a follow up. We ignore the referrer like: https://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/toolkit/content/contentAreaUtils.js#803

Comment on attachment 8925441 [details] [diff] [review] Add referrer policy support to Downloads.jsm Hi Thomas, thanks for working on this! There has been more discussion after comment 0, and we devised a different approach that should also make the code easier to follow than the approach followed in this patch. Take a read, and let me know if you have any questions!

Thanks Paolo for the prompt reply. Sorry I missed the last comment 16 and 17, so basically, we decided to compute the referrer at the time of creating a download and only store referrer. I agree with comment 5. I will follow that but we have to add a triggeringPricipal from referrerURI. That is something looks like let triggeringPrincipal = Services.scriptSecurityManager.createCodebasePrincipal(referrerURI, {}); var chan = NetUtil.newChannel({ uri: uri, loadingPrincipal: Services.scriptSecurityManager.getSystemPrincipal(), triggeringPrincipal: triggeringPrincipal, contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER }); Otherwise the computed referrer will be wrong because we will consider all the cases as cross-origin.

MozReview-Commit-ID: JabcQXR3Fxx

Comment on attachment 8925842 [details] [diff] [review] Add a compute request referrer helper to NetUtil.jsm Patrick, The last patch which computes the sending referrer seems incorrect (ignore triggering principal). Could you please take a look at the new patch?

Comment on attachment 8926271 [details] Bug 1073187 - Add a compute request referrer helper to NetUtil.jsm https://reviewboard.mozilla.org/r/197540/#review202872

Paolo, do you mind taking a look at the patch? That is basically propagating referrer policy and computing the referrer request when we are trying to save something (as approach in comment 16 and 17). I added some tests for the patch. But it's a bit tricky that I have to use add_task instead of waitForExplicitFinish for the case saving alt key ( I have no idea the following line does not work with waitForExplicitFinish) BrowserTestUtils.synthesizeMouseAtCenter(id, {altKey: true}, browser);

Comment on attachment 8926272 [details] Bug 1073187 - Keep the full referrer provided to downloads instead of applying the default referrer policy. https://reviewboard.mozilla.org/r/197542/#review206368 The approach looks good, I'll do the code review later this week. One thing to consider is that we may be breaking the "Go To Download Page" functionality, so we may need to store the initiating URL separately from the referrer. Depending on how common restrictive referrer policies are, we may leave this for a follow-up and implement it later. Do we send HTTPS referrers to HTTP requests by default? ::: toolkit/content/contentAreaUtils.js:1012 (Diff revision 3) > dummy); > } catch (e) { > } > } > if (fileName) > - return validateFileName(fileName); > + return fileName; These look like leftovers from a merge.

(In reply to :Paolo Amadini from comment #31) > Comment on attachment 8926272 [details] > Bug 1073187 - Compute request referrer before passing to Download.jsm > > https://reviewboard.mozilla.org/r/197542/#review206368 > > The approach looks good, I'll do the code review later this week. > > One thing to consider is that we may be breaking the "Go To Download Page" > functionality, so we may need to store the initiating URL separately from > the referrer. Depending on how common restrictive referrer policies are, we > may leave this for a follow-up and implement it later. Do we send HTTPS > referrers to HTTP requests by default? Thanks Paolo. No, the referrer header is omitted by default https->http. That was added in the test cases > ::: toolkit/content/contentAreaUtils.js:1012 > (Diff revision 3) > > dummy); > > } catch (e) { > > } > > } > > if (fileName) > > - return validateFileName(fileName); > > + return fileName; > > These look like leftovers from a merge. Thanks for the catching. I will update that after your review

Comment on attachment 8926272 [details] Bug 1073187 - Keep the full referrer provided to downloads instead of applying the default referrer policy. https://reviewboard.mozilla.org/r/197542/#review207820 Sorry for the delay, this ended up being a pretty big patch. It will definitely help if each logical change is separated into its own changeset, starting with the Downloads API additions and ending with the smaller front-end modifications. In particular, all the places where we used to start downloads from privileged code without providing a referrer at all, that are now updated to pass the correct referrer, should be handled each in its own in separate changeset. For the low-level API changes, we'll need to start with the change in DownloadCore.jsm that preserves the unmodified "referrer" in the channel after resuming a legacy download, or when the property is provided explicitly to createDownload. This will need its own unit test in the "jsdownloads" folder. Note that for downloads using nsITransfer, we can presume that the caller has already initialized the channel correctly, so I don't think we need to add the referrer and referrer policy to the nsITransfer interface for this purpose, just to read and keep the correct referrer from the channel later. For downloads started with createDownload, at some point we might want to accept an additional initiatingUri property, as in "source: { url, referrer, initiatingUri }", and serialize it only when it's different from the referrer. However, you can just file a bug to implement this later, because the "Go To Download Page" command is already broken for the most common HTTPS -> HTTP case. We may also have to handle this for history downloads, storing the initiating URI separately.

Comment on attachment 8926272 [details] Bug 1073187 - Keep the full referrer provided to downloads instead of applying the default referrer policy. https://reviewboard.mozilla.org/r/197542/#review207820 Thanks for looking at the patch. AFAIK, DownloadLegacy and nsExternalHelperAppService are using the same nsITransfer interface (I guess you are mentioning about nsExternalHelperAppService and suggest that we may not need to modify it). You are right, in nsExternalHelperAppService case, we don't have to add referrer and policy, just use the referrer header channel already set. But it seems incorrect in the case DownloadLegacy, I think we have to pass referrer and referrer policy from the init() then initiate the download (otherwise default value will be used) https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/toolkit/content/contentAreaUtils.js#511 That's reason why I have to change the interface and both DownloadLegacy and nsExternalHelperAppService. The browser tests I added may cover both cases, browser_referrer_download_link.js is using nsExternalHelperAppService, browser_referrer_download_alt.js and browser_referrer_download_image.js are using DownloadLegacy. Please correct me if I am wrong or I am missing anything. I will update the patch based on other suggestions.

Comment on attachment 8926272 [details] Bug 1073187 - Keep the full referrer provided to downloads instead of applying the default referrer policy. https://reviewboard.mozilla.org/r/197542/#review207820 Ah, I remember that I did not add the unit test because I have no idea to test the scheme https->http in xpcshell test. The reason why httpd server does not support https at the moment. Presumely http2 with some provided certs could help us, but I have no idea about that.

(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #34) > it seems incorrect in the case DownloadLegacy, I think we have to pass > referrer and referrer policy from the init() then initiate the download > (otherwise default value will be used) Also in this case, the referrer is read from the channel provided by saveDocument or savePrivacyAwareURI: https://dxr.mozilla.org/mozilla-central/rev/0bb0f14672fdda31c19aea1ed829e050d693b9af/toolkit/components/jsdownloads/src/DownloadCore.jsm#2364-2367 So this should work as long as the channel provided to the callback has the correct referrer. If it doesn't, it may be an issue with the nsIWebBrowserPersist implementation or interface, which we may need to update, otherwise the referrer seen by the front-end may be different from the one sent on the network. By keeping each logical change separate, we can find the best fix to achieve the effect for each of the download paths we need to change. > The browser tests I added may cover both cases, > browser_referrer_download_link.js is using nsExternalHelperAppService, > browser_referrer_download_alt.js and browser_referrer_download_image.js are > using DownloadLegacy. Please correct me if I am wrong or I am missing > anything. I haven't looked at the implementation of the tests yet, but that sounds right. Ideally, each of these tests should be added separately, in the same changeset that modifies the production code that is being tested.

(In reply to :Paolo Amadini from comment #36) > (In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #34) > > it seems incorrect in the case DownloadLegacy, I think we have to pass > > referrer and referrer policy from the init() then initiate the download > > (otherwise default value will be used) > > Also in this case, the referrer is read from the channel provided by > saveDocument or savePrivacyAwareURI: > > https://dxr.mozilla.org/mozilla-central/rev/ > 0bb0f14672fdda31c19aea1ed829e050d693b9af/toolkit/components/jsdownloads/src/ > DownloadCore.jsm#2364-2367 > > So this should work as long as the channel provided to the callback has the > correct referrer. If it doesn't, it may be an issue with the > nsIWebBrowserPersist implementation or interface, which we may need to > update, otherwise the referrer seen by the front-end may be different from > the one sent on the network. By keeping each logical change separate, we can > find the best fix to achieve the effect for each of the download paths we > need to change. Ah, I see, thanks for your clarification. I will also add some unit test for the download api, but for http scheme only

(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #35) > Ah, I remember that I did not add the unit test because I have no idea to > test the scheme https->http in xpcshell test. > The reason why httpd server does not support https at the moment. Presumely > http2 with some provided certs could help us, but I have no idea about that. Ah, I haven't been faced with this problem before. I guess that mochitests may be using a proxy. If it's difficult to set this up in xpcshell, for example if it's done in the infrastructure before the browser is started, just file a bug to have this added as a browser-chrome test in the jsdownloads folder. This is not trivial and may involve moving shared code to a DownloadTestUtils.jsm module, hence we don't need to do this now. Note that the issue fixed here is specifically about restarting a download, not the first time it is started, so it's not covered by the other browser-chrome tests you added.

Thanks Thomas for the quick chat! As you suggested, we can definitely implement an end-to-end test under "browser/base/content/test" for the case of restarting downloads, even though this doesn't replace a unit test for the API, for which we still need to file a bug. This end-to-end test should be added in the same changeset as the Downloads API change that fixes its behavior. Since the test is not strictly needed, if you want you may also do just the API change first and amend the commit to add the test later, but I understand from our chat that you'd like to do both now, which is indeed better. The other referrer fixes can be separate changesets on top of this initial one. Thanks again, and feel free to add more notes if I forgot anything new we discussed today!

Comment on attachment 8926271 [details] Bug 1073187 - Add a compute request referrer helper to NetUtil.jsm https://reviewboard.mozilla.org/r/197540/#review208414 ::: netwerk/base/NetUtil.jsm:525 (Diff revision 2) > + let referreingURI = (aReferringURI instanceof Ci.nsIURI) ? > + aReferringURI : Services.io.newURI(aReferringURI); > + let triggeringPrincipal = Services.scriptSecurityManager > + .createCodebasePrincipal(referreingURI, {}); > + let chan; > + nit, move let chan inside the try-block. Something like let chan = NetUtil.... ::: netwerk/base/NetUtil.jsm:529 (Diff revision 2) > + let chan; > + > + try { > + chan = NetUtil.newChannel({ > + uri: aURI, > + loadingPrincipal: Services.scriptSecurityManager.getSystemPrincipal(), As far as I can tell there is no need for a loadingPrincipal in that case, please remove the explicit loadingPrincipal. Alternatively you could use a NullPrincipal instead of SystemPrincipal and please add a comment that we just create a tmp channel which is never openend - thanks.

Comment on attachment 8926271 [details] Bug 1073187 - Add a compute request referrer helper to NetUtil.jsm https://reviewboard.mozilla.org/r/197540/#review208414 > As far as I can tell there is no need for a loadingPrincipal in that case, please remove the explicit loadingPrincipal. Alternatively you could use a NullPrincipal instead of SystemPrincipal and please add a comment that we just create a tmp channel which is never openend - thanks. I see, that's a good catch. Thanks Chris

Paolo, I changed the patch based on the addressed comments (except testing of restarting download in browser-test, I got an exception, let's do it in xpcshell). Do you mind taking a look? There're some parts I used the default policy (for example pageInfo). I guess it should be done in another follow up bug.

Comment on attachment 8926272 [details] Bug 1073187 - Keep the full referrer provided to downloads instead of applying the default referrer policy. https://reviewboard.mozilla.org/r/197542/#review210954 Sorry for the delay, I had to test the patch locally to understand it better, because even if I reverted the production code, the tests still passed. I was a bit confused in comment 38 as to what we were supposed to test. It's correct that we can use xpcshell here and bug 1420843 could be closed, since we only need HTTPS for the referrer and not on the wire. The issue, however, was that this patch still tested an HTTP referrer. Separating this patch from all the other changes was very useful to narrow that down. Thanks for doing that! I think this part can land the even before the NetUtil change, so I suggest filing a new bug that blocks this one, moving the patch there after updating it, and asking for review again there. In fact, I took a look at the callers of createDownload: https://searchfox.org/mozilla-central/search?q=createDownload\b&regexp=true None of the external callers pass the "referrer" property, so we can change the API already to bypass the default policy. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm:1275 (Diff revision 5) > * referrer: String containing the referrer URI of the download source. > * Can be omitted or null if no referrer should be sent or > * the download source is not HTTP. Edit the API description: * referrer: String containing the referrer URI of the download source. * This is the value that will be sent on the network, * meaning that any referrer policy should be computed in * advance. Can be omitted or null if no referrer should be * sent or the download source is not HTTP. An identical change should be done in "Downloads.jsm". ::: toolkit/components/jsdownloads/src/DownloadCore.jsm:1303 (Diff revision 5) > // Convert String objects to primitive strings at this point. > source.url = aSerializable.url.toString(); > if ("isPrivate" in aSerializable) { > source.isPrivate = aSerializable.isPrivate; > } > - if ("referrer" in aSerializable) { > + if ("referrer" in aSerializable && aSerializable.referrer) { "&& aSerializable.referrer" is not needed. Any falsy value will be discarded later when serializing. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm:1919 (Diff revision 5) > - channel.referrer = NetUtil.newURI(download.source.referrer); > + // Sending Referrer header is computed at the time we initialize a download ( > + // eg. user clicks on "Save Link As"). We use REFERRER_POLICY_UNSAFE_URL > + // to keep the referrer header the same here nit: wrap at 80 characters, dot at the end of the last sentence. ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js:430 (Diff revision 5) > + download = await promiseStartLegacyDownload( > + sourceUrl, { referrer: NetUtil.newURI(TEST_REFERRER_URL) }); > + > + await promiseDownloadStopped(download); > + do_check_eq(download.source.referrer, TEST_REFERRER_URL); nit: "compress" the test cases by removing the blank line in the middle, for better visual separation. ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js:436 (Diff revision 5) > + download = await promiseStartLegacyDownload( > + sourceUrl, { referrer: NetUtil.newURI(TEST_REFERRER_URL), > + targetFile }); > + > + await promiseDownloadStopped(download); > + do_check_eq(download.source.referrer, TEST_REFERRER_URL); I don't think that specifying a target file changes what we're testing. You can remove this test case, and move the "let targetPath" inside the other block. The "let targetFile" is not needed. On the other hand, you can test the property value for the non-HTTP channel, like the other code path does. You can add "let dataSourceUrl" outside the block, or add a constant to the head file. You can test that "download.source.referrer" will be null in this case since the referrer is read directly from the channel. ::: toolkit/components/jsdownloads/test/unit/head.js:69 (Diff revision 5) > "nsIMIMEService"); > > const TEST_TARGET_FILE_NAME = "test-download.txt"; > const TEST_STORE_FILE_NAME = "test-downloads.json"; > > const TEST_REFERRER_URL = "http://www.example.com/referrer.html"; This should become HTTPS, with a comment: // We are testing an HTTPS referrer with HTTP downloads in order to verify that // the default policy will not prevent the referrer from being passed around. const TEST_REFERRER_URL = "https://www.example.com/referrer.html"; ::: toolkit/components/jsdownloads/test/unit/head.js:228 (Diff revision 5) > * An optional object used to control the behavior of this function. > * You may pass an object with a subset of the following fields: > * { > * isPrivate: Boolean indicating whether the download originated from a > * private window. > + * referrer: nsURI referrer for the download source. Let's use a string here and call newURI inside the function, for consistency. ::: toolkit/components/jsdownloads/test/unit/head.js:319 (Diff revision 5) > - persist.savePrivacyAwareURI(sourceURI, null, null, 0, null, null, targetFile, > - isPrivate); > + persist.savePrivacyAwareURI(sourceURI, null, referrer, 0, null, null, > + targetFile, isPrivate); You should pass REFERRER_POLICY_UNSAFE_URL in order to test the HTTPS to HTTP case. Indent like this: persist.savePrivacyAwareURI( sourceURI, null, referrer, Ci.nsIHttpChannel.REFERRER_POLICY_UNSAFE_URL, null, null, targetFile, isPrivate ); ::: toolkit/components/jsdownloads/test/unit/test_DownloadLegacy.js:79 (Diff revision 5) > + mustInterruptResponses(); > + download = await promiseStartLegacyDownload( > + sourceUrl, { referrer: NetUtil.newURI(TEST_REFERRER_URL), > + targetFile }); > + > + await restart_and_check_referrer(download); Also here, this test case looks redundant. nit: remove the blank lines before the restart_and_check_referrer calls.

Comment on attachment 8932410 [details] Bug 1073187 - Pass default referrer/referrer policy when saving content https://reviewboard.mozilla.org/r/203460/#review210962 All the changes to the signature of functions should be made in the same changeset as the callers. To proceed in steps, what you can do here is to use a separate changeset like this one to update all the functions that we already know will require an additional parameter for the referrer policy. The parameter will be passed around, but the first-level callers will all specify Ci.nsIHttpChannel.REFERRER_POLICY_UNSET. I can review this in advance and we can land it, just checking that the existing tests pass. We can then have one or more bugs where we fix the callers, and we add the related test cases. If there is any code path for which we don't have a test case yet, we can file a bug to have the test case added later. ::: browser/base/content/pageinfo/pageInfo.js:713 (Diff revision 2) > if (item instanceof HTMLVideoElement) > titleKey = "SaveVideoTitle"; > else if (item instanceof HTMLAudioElement) > titleKey = "SaveAudioTitle"; > > - saveURL(url, null, titleKey, false, false, makeURI(item.baseURI), > + // TODO bug 1420847, should use a document/element referrer policy You can annotate the other callers with the bug numbers like you did in this case. Ensure that all the bugs block this one, including bug 1420847, and that they are in the right product and component. ::: browser/base/content/pageinfo/pageInfo.js:714 (Diff revision 2) > titleKey = "SaveVideoTitle"; > else if (item instanceof HTMLAudioElement) > titleKey = "SaveAudioTitle"; > > - saveURL(url, null, titleKey, false, false, makeURI(item.baseURI), > + // TODO bug 1420847, should use a document/element referrer policy > + saveURL(url, null, titleKey, false, false, makeURI(item.baseURI), 0, Use the constant rather than the value "0" whenever possible.

Comment on attachment 8932408 [details] Bug 1073187 - Compute request referrer before passing to Download.jsm https://reviewboard.mozilla.org/r/203456/#review210968 A good chunk of this changeset will be moved to the one about changing the function signatures, and the remaining functional parts can be applied together with the related test cases.

Comment on attachment 8932409 [details] Bug 1073187 - Test correct referrer saving content from context menu https://reviewboard.mozilla.org/r/203458/#review210970 Depending on how many different bugs you think it is appropriate to file for each of the callers, these tests may be split in different changesets, and they can land together with each production change being tested. For each bug individually, a good smoketest is to revert the production change, and see if the individual test associated with it now fails.

Comment on attachment 8926272 [details] Bug 1073187 - Keep the full referrer provided to downloads instead of applying the default referrer policy. https://reviewboard.mozilla.org/r/197542/#review210954 Many thanks for sharing your time :). That's definitely right that I have to change to HTTPS referrer in the test, otherwise even we reverted to production code but the tests still pass :). Thanks for finding that. Agree with you, landing this patch first seems more appropriate with the original purpose of this bug. I will file other bugs to fix callers of Download API after this change

Comment on attachment 8926272 [details] Bug 1073187 - Keep the full referrer provided to downloads instead of applying the default referrer policy. https://reviewboard.mozilla.org/r/197542/#review211342 ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js:403 (Diff revision 6) > + let targetFile = getTempFile(TEST_TARGET_FILE_NAME); > + let targetPath = targetFile.path; let targetPath = getTempFile(TEST_TARGET_FILE_NAME).path; ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js:442 (Diff revision 6) > + sourceUrl, { referrer: TEST_REFERRER_URL, > + isPrivate: true}); > + await promiseDownloadStopped(download); > + do_check_eq(download.source.referrer, TEST_REFERRER_URL); > + > + let dataSourceUrl = "data:text/html,<html><body></body></html>"; Move the "let" to the top of the function, and reuse in the other code block. ::: toolkit/components/jsdownloads/test/unit/head.js:69 (Diff revision 6) > "nsIMIMEService"); > > const TEST_TARGET_FILE_NAME = "test-download.txt"; > const TEST_STORE_FILE_NAME = "test-downloads.json"; > > -const TEST_REFERRER_URL = "http://www.example.com/referrer.html"; > +const TEST_REFERRER_URL = "https://www.example.com/referrer.html"; Add the comment I provided in the previous review. ::: toolkit/components/jsdownloads/test/unit/test_DownloadLegacy.js:19 (Diff revision 6) > +/** > + * Checks the referrer for restart downloads. > + * If the legacy download is stopped and restarted, the saving method > + * is changeed from DownloadLegacySaver to the DownloadCopySaver. > + * The referrer header should be passed correctly > + */ typo: changed nit: dot at the end of the sentence

Comment on attachment 8926272 [details] Bug 1073187 - Keep the full referrer provided to downloads instead of applying the default referrer policy. https://reviewboard.mozilla.org/r/197542/#review211350 ::: commit-message-6bea3:1 (Diff revision 6) > +Bug 1073187 - Passing full referrer to download.source > + > +When we resume a download, the request referrer will be computed again > +with default referrer policy (which is wrong in the case cross-origin) > +We should compute the request referrer before calling download API, > +store it in download object and not try to compute again (using > +REFERRER_POLICY_UNSAFE_URL) Edit the changeset summary: Bug 1073187 - Keep the full referrer provided to downloads instead of applying the default referrer policy. nit: case cross-origin => cross-origin case nit: dot at the end of the sentence, after the closing parenthesis

That is a really nice review, Paolo, thanks for that :)

Comment on attachment 8926272 [details] Bug 1073187 - Keep the full referrer provided to downloads instead of applying the default referrer policy. https://reviewboard.mozilla.org/r/197542/#review211370 Thanks for this good patch! ::: commit-message-6bea3:1 (Diff revisions 6 - 7) > -Bug 1073187 - Passing full referrer to download.source > +Bug 1073187 - Keep the full referrer provided to downloads instead of > +applying the default referrer policy. The bug summary should be on a single line so that the tooling handles it correctly.

Pushed by tnguyen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/331935f830b6 Keep the full referrer provided to downloads instead of applying the default referrer policy. r=Paolo