Closed Bug 987387 Opened 6 years ago Closed 6 years ago

Make sendBeacon respect private browsing

Categories

(Core :: DOM: Core & HTML, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox28 --- unaffected
firefox29 --- unaffected
firefox30 + fixed
firefox31 + fixed

People

(Reporter: sicking, Assigned: ehsan)

References

Details

Attachments

(2 files)

I'm slightly worried that sendBeacon doesn't work with private browsing. IIRC we get the "use private browsing for this request" flag from the nsILoadGroup. However sendBeacon clears the loadgroup in order to make the request survive the user leaving the page.

We should check both that outgoing sendBeacon requests from private-browsing tabs send private-browsing cookies rather than the "real" cookies, and we should check that any set-cookie response headers in the response from the sendBeacon requests only affect private-browsing cookies and does not affect the "real" cookies.

Likewise we should check that sendBeacon from within FirefoxOS apps uses the cookie-jar for the app rather than some other cookie jar. Both for sent cookies and for incoming set-cookie headers.

Richard Barnes did some testing and it looked like there was at least some problems here.

This might be a blocker for shipping sendBeacon.
My ad-hoc testing for private browsing mode indicates that:

1. Cookies do not leak normal->private: If a cookie is set by a site in a normal, non-private window, and then the site is loaded in private browsing mode, then the cookie is not sent.

2. Cookies do leak private->normal: If a cookie is set by a site in a private window, and then the site is loaded in a normal window, then the cookie is sent.

This obviously leaks information about the user's private browsing activities.
(In reply to Richard Barnes [:rbarnes] from comment #1)
> My ad-hoc testing for private browsing mode indicates that:
> 
> 1. Cookies do not leak normal->private: If a cookie is set by a site in a
> normal, non-private window, and then the site is loaded in private browsing
> mode, then the cookie is not sent.
> 
> 2. Cookies do leak private->normal: If a cookie is set by a site in a
> private window, and then the site is loaded in a normal window, then the
> cookie is sent.
> 
> This obviously leaks information about the user's private browsing
> activities.

Yes, what you said above is true (BTW I would be happy to clarify how different components handle PB mode if needed, in case you don't feel like testing things manually. :)

But it's actually much worse than that.  Non-private requests can also set cookies, or write cache entries to disk, both of which mean that we'll be leaking out information about the private browsing visited websites, which basically breaks the contract of private browsing.

We should *definitely* fix this before shipping this feature.

(BTW, Jonas, why did you mark this as a security sensitive bug?  We don't usually do that for privacy leakage bugs.)
Assignee: nobody → ehsan
Comment on attachment 8396116 [details] [diff] [review]
Part 1: Make navigator.sendBeacon() respect private browsing mode; r=jdm

(I'm not sure how to deliver the correct appId to this channel...)
Attachment #8396116 - Flags: review?(josh)
Opening
Group: dom-core-security
I spun off the appId part to bug 988107 to avoid the situation of multiple patches landing in this bug.
Summary: Verify that sendBeacon works with Private Browsing and FirefoxOS apps → Make sendBeacon respect private browsing
Attachment #8396116 - Flags: review?(josh) → review+
Comment on attachment 8396116 [details] [diff] [review]
Part 1: Make navigator.sendBeacon() respect private browsing mode; r=jdm

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 936340
User impact if declined: see comment 2.
Testing completed (on m-c, etc.): Locally.
Risk to taking this patch (and alternatives if risky): minimal, this is a new feature which is not heavily used yet.
String or IDL/UUID changes made by this patch: none.
Attachment #8396116 - Flags: approval-mozilla-aurora?
We also need tests here.
Yep, I'll write one this weekend.
Keywords: leave-open
Flags: needinfo?(ehsan)
Oops, relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/47cb8b25fc45
Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)
I'm getting a daily release tracking email from this bug because I guess we haven't closed it yet.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Keywords: leave-open
Resolution: --- → FIXED
Comment on attachment 8396116 [details] [diff] [review]
Part 1: Make navigator.sendBeacon() respect private browsing mode; r=jdm

approving for uplift, landing and status update should stop the daily reminders.
Attachment #8396116 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8398976 [details] [diff] [review]
Part 2: Add a test to ensure that sendBeacon respects private browsing by checking to see if a private cookie is set when calling sendBeacon in private browsing mode; r=jdm

I moved the cookie test to the mochitest-chrome suite so that I can open up browser windows, and extended it to test both setting the cookies in private and normal mode.
Attachment #8398976 - Flags: review?(josh)
Flags: needinfo?(ehsan)
Setting checkin-needed to get the first part landed on Aurora.
Keywords: checkin-needed
Comment on attachment 8398976 [details] [diff] [review]
Part 2: Add a test to ensure that sendBeacon respects private browsing by checking to see if a private cookie is set when calling sendBeacon in private browsing mode; r=jdm

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

::: dom/tests/mochitest/beacon/chrome.ini
@@ +2,5 @@
> +skip-if = buildapp == 'b2g' || e10s
> +
> +[test_beaconCookies.html]
> +support-files = beacon-set-cookie.sjs
> +                file_beaconCookies.html

I've never seen support-files per test before.
Attachment #8398976 - Flags: review?(josh) → review+
No longer blocks: 936340
Blocks: 990220
Landed the test here too, please uplift them both to Aurora!

https://hg.mozilla.org/integration/mozilla-inbound/rev/352a2109c6e6
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla31
Fixed the test and relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/030829581465

The problem was that I was not removing an observer in the private window case and that fired the next time "cookie-changed" was fired in an unrelated test.
Flags: needinfo?(ehsan)
Depends on: 1266588
No longer depends on: 1266588
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.