[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: 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, TorBrowserTeam201804 |
Parent ID: | Points:
Reviewer: | Sponsor:
-------------------------------------------------+-------------------------
Changes (by arthuredelstein):
* status: needs_revision => needs_review
Comment:
Replying to [comment:38 gk]:
> Okay, here come some review notes:
Thanks for the review.
> 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)
> }}}
Good catch -- fixed.
> 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...`.
Yes, this was also wrong. Fixed.
> 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?
Added.
> 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)?
Now, instead of disallowing file:// URLs, instead I whitelist http, https
and ftp URLs. Other protocols will use the default principal.
New version:
https://github.com/arthuredelstein/tor-browser/commit/22343+10
(516cbf20b2b722b3f9522b4b9e5f5b6c25c34322)
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22343#comment:42>
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