[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_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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs