[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #22343 [Applications/Tor Browser]: Save as... in the context menu results in using the catch-all circuit
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs