[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:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Major                                |     Resolution:
 Keywords:  tbb-linkability, tbb-usability,      |  Actual Points:
  ff52-esr, tbb-7.0-must, tbb-7.0-issues, tbb-   |
  regression, tbb-7.0-frequent,                  |
  TorBrowserTeam201810                           |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by gk):

 * cc: igt0, sysrqb (added)
 * keywords:
     tbb-linkability, tbb-usability, ff52-esr, tbb-7.0-must,
     tbb-7.0-issues, tbb-regression, tbb-7.0-frequent,
     TorBrowserTeam201810R
     =>
     tbb-linkability, tbb-usability, ff52-esr, tbb-7.0-must,
     tbb-7.0-issues, tbb-regression, tbb-7.0-frequent, TorBrowserTeam201810
 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:54 arthuredelstein]:
 > Here's my patch from comment:42 rebased on top of tor-
 browser-60.3.0esr-8.5-1:
 > https://github.com/arthuredelstein/tor-browser/commit/22343+10
 (4833bfbd82ac4fb6b8836b326a84a11af8971044)

 Alright, I think that's `22343+11`? At least that's the one I used. :)
 (the commit I looked at is the same).

 I think we are close, nice work! Some questions and nits below:

 1) We have

 {{{
 +        const principal =
 Services.scriptSecurityManager.createCodebasePrincipal(
 +          makeURI(blobURL), thisPrincipal.originAttributes);
          saveImageURL(blobURL, "canvas.png", "SaveImageTitle",
                       true, false, referrerURI, null, null, null,
 -                     isPrivate);
 +                     isPrivate, principal);
        }, Cu.reportError);
      } else if (this.onImage) {
 -      urlSecurityCheck(this.mediaURL, this.principal);
 +      const principal =
 Services.scriptSecurityManager.createCodebasePrincipal(
 +        makeURI(this.mediaURL), thisPrincipal.originAttributes);
 +      urlSecurityCheck(this.mediaURL, principal);
        saveImageURL(this.mediaURL, null, "SaveImageTitle", false,
                     false, referrerURI, null,
 gContextMenuContentData.contentType,
 -                   gContextMenuContentData.contentDisposition,
 isPrivate);
 +                   gContextMenuContentData.contentDisposition,
 isContentWindowPrivate,
 +                   principal);
 }}}

 in the patch. Why are we switching in the `onImage` case from `isPrivate`
 to `isContentWindowPrivate` but not in the former case (the latter
 `onVideo` or `orAudio` case already had that before)? If that's correct
 and not just an oversight, it might be worth commenting it.

 2) From looking at he code in `ContentClick.jsm` It seems we might be able
 to trigger `window.openLinkIn(json.href, where, params);` which could lead
 to false FPI in the `save` case (see the: `// Todo(903022): code for where
 == save`) or is that just a leftover comment and we are actually good?

 3) What about
 {{{
 ContentAreaUtils.saveImageURL(aTarget.currentRequestFinalURI.spec, null,
 "SaveImageTitle",
                                           false, true,
 aTarget.ownerDocument.documentURIObject,
                                           aTarget.ownerDocument);
 }}}

 in `mobile/android/chrome/content/browser.js`? *If* we can hit it it
 should get amended as well for FPI reasons I think (maybe even without
 that just to be on the safe side). Maybe igt0/sysrqb have some insight
 here. I did not look closer during the review.

 4) Where is the new `Ci` needed in `contentAreaUtils.js` (see:
 {{{
 +function saveDocument(aDocument, aSkipPrompt, aContentPrincipal) {
 +  const Ci = Components.interfaces;
 +
 }}}
 It seems to me it is some leftover as `Ci` is already available in that
 .js file.

 5) `loadingPrincipal  : aContentPrincipal,`: the two whitespaces before
 the `:` should go.

 6) Maybe add some comment to
 {{{
 +  attribute nsIPrincipal loadingPrincipal;
 }}}
 in the .idl file given that everything else in that file does get a
 comment explaining what those items are about?

 I guess mcs/brade could be good second reviewers given that they already
 looked at previous patch iterations. Let's hear as well what they think

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