#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@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs