Closed Bug 792517 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: File Handling, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18
Tracking Status
firefox18 + fixed

People

(Reporter: jdm, Assigned: jdm)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → josh
I want to write a test for this, but I haven't found good examples of triggering Save As yet.
Attachment #663262 - Attachment is obsolete: true
The test needs PB cookie separation to actually test what it's supposed to test. It probably passes unconditionally without that patch applied.
Attachment #666027 - Flags: review?(ehsan)
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!
Attachment #666027 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/cb64683950d9
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
You need to log in before you can comment on or make changes to this bug.