[tor-bugs] #22343 [Applications/Tor Browser]: Save as... in the context menu results in using the catch-all circuit

[Tor Bug Tracker & Wiki]

#22343: Save as... in the context menu results in using the catch-all circuit
-------------------------------------------------+-------------------------
 Reporter:  gk                                   |          Owner:
                                                 |  arthuredelstein
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  High                                 |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Major                                |     Resolution:
 Keywords:  tbb-linkability, ff52-esr,           |  Actual Points:
  tbb-7.0-must, tbb-7.0-issues, tbb-regression,  |
  tbb-7.0-frequent, TorBrowserTeam201801         |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by gk):

 * keywords:
     tbb-linkability, ff52-esr, tbb-7.0-must, tbb-7.0-issues, tbb-
     regression, tbb-7.0-frequent, TorBrowserTeam201801R
     =>
     tbb-linkability, ff52-esr, tbb-7.0-must, tbb-7.0-issues, tbb-
     regression, tbb-7.0-frequent, TorBrowserTeam201801
 * status:  needs_review => needs_revision


Comment:

 Okay, here come some review notes:

 1) You added a `doc.nodePrincipal` to the `saveURL` call in `browser.js`.
 But it seems to me that principal gets passed as `aIsContentWindowPrivate`
 in `contentAreaUtils.js`. The function definition is
 {{{
 function saveURL(aURL, aFileName, aFilePickerTitleKey, aShouldBypassCache,
                  aSkipPrompt, aReferrer, aSourceDocument,
 aIsContentWindowPrivate,
                  aContentPrincipal)
 }}}

 2) Could you elaborate a bit on why just adding the
 `isContentWindowPrivate` parameter in the `saveURL` call in
 `nsConetextMenu.js` is enough skipping the loading prinicpal one? It seems
 to me that's wrong. Looking at the code I tried to test my hypothesis by
 setting `browser.download.saveLinkAsFilenameTimeout` to `0`. Then I get
 indeed a notice in the Torbutton log that the download seems to happen
 over the catch-all circuit if I try to download a link via `Save Link
 as...`.

 3) There is
 {{{
  * @param aContentPrincipal [optional]
  *        The principal to be used for loading and saving the target URL.
 }}}
 added to the comment regarding `internalSave` in `contentAreaUtils.js`.
 Maybe at it to the comment for `saveImageURL` as well?

 4) I stumbled over
 {{{
 +  if (persistArgs.sourceURI.scheme !== "file") {
 +    persist.loadingPrincipal = persistArgs.loadingPrincipal;
 +  }
 }}}
 wondering why `file://` is special here. What about `view-source:`? I am
 not sure if it is related to just that file scheme check but when I went
 to download the view-source result of torproject.org I get
 {{{
 Security Error: Content at view-source:https://www.torproject.org/ may not
 load or link to resource://gre-resources/viewsource.css.
 }}}
 in my browser console which is not happening without your patch. So, do
 you want to check for a non-content loading principal or is it really all
 principals as long as the scheme is not `file://`. If so a comment might
 help (too)?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22343#comment:38>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online