792517 - nsContextMenu.js save as handler creates a channel without regard for privacy status

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1008

There's a notification callback we could hijack to return the privacy status of the original document, perhaps. Since the request isn't guaranteed to bypass the cache unless a flag is passed, it looks like this could get stored on disk and that would be bad.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Make the channel created by Save As respect the privacy status of the originating document.

I want to write a test for this, but I haven't found good examples of triggering Save As yet.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Make the channel created by Save As respect the privacy status of the originating document.

Comment 3

[Josh Matthews [:jdm]]

The test needs PB cookie separation to actually test what it's supposed to test. It probably passes unconditionally without that patch applied.

Comment 4

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=42ebb6ed5276

Comment 5

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=183c629b0102

https://hg.mozilla.org/integration/mozilla-inbound/rev/71a2a9d7618e

and then I realized that this hasn't been reviewed yet, so backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5a4d66a6da20.

Comment 6

[(no longer active)]

Comment on attachment 666027 [details] [diff] [review]
Make the channel created by Save As respect the privacy status of the originating document.

Review of attachment 666027 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the stuff below addressed.

::: browser/base/content/nsContextMenu.js
@@ +1008,5 @@
>      var ioService = Cc["@mozilla.org/network/io-service;1"].
>                      getService(Ci.nsIIOService);
>      var channel = ioService.newChannelFromURI(makeURI(linkURL));
> +    if (channel instanceof Ci.nsIPrivateBrowsingChannel) {
> +      var docIsPrivate = PrivateBrowsingUtils.isWindowPrivate(doc.defaultView);

Nit: let

::: browser/base/content/test/browser_save_link.js
@@ +13,5 @@
> +// the first by checking the cookies that are sent.
> +
> +function triggerSave(aCallback) {
> +  var fileName;
> +  gBrowser.loadURI("http://mochi.test:8888/browser/browser/base/content/test/bug792517-2.html");

I'd rather you open a new tab, load the page inside it and close it when you're done to make sure that this test does not mess with the sessionhistory of the tab which might be relied on in future tests.

@@ +20,5 @@
> +      return;
> +    gBrowser.removeEventListener("pageshow", pageShown);
> +
> +    executeSoon(function () {
> +      document.addEventListener("popupshown", contextMenuOpened);

Nit: ditto.

@@ +27,5 @@
> +      EventUtils.synthesizeMouseAtCenter(link,
> +                                         { type: "contextmenu", button: 2 },
> +                                         gBrowser.contentWindow);
> +    });
> +  });

Nit: third argument to addEventListener.

::: browser/base/content/test/bug792517.sjs
@@ +8,5 @@
> +   aResponse.write("cookie-present");
> + } else {
> +   aResponse.setHeader("Set-Cookie", "foopy=1");
> +   aResponse.write("cookie-not-present");
> + }

I think you're supposed to do aResponse.finish() at some point.  I'm surprised that this works!

Comment 7

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb64683950d9

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb64683950d9