[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_review
 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,                  |
  TorBrowserTeam201811                           |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by arthuredelstein):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:56 gk]:

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

 Thanks for the reviews, everyone! You found the right patch despite my
 typo.
 Here's the new branch: https://github.com/arthuredelstein/tor-
 browser/commit/22343+12

 > 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.

 That was an oversight, and I have corrected 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?

 This was a good catch. I found I needed to patch the `saveURL` function in
 `browser/base/content/utilityOverlay.js`.

 > 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.

 Another good find. I patched that file.

 > 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.

 True -- I removed that unnecessary line.

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

 Fixed.

 > 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?

 Done, thanks.

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