987387 - Make sendBeacon respect private browsing

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 1

[Richard Barnes [:rbarnes]]

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.

Comment 2

[(no longer active)]

(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.)

Comment 3

[(no longer active)]

Attachment: Part 1: Make navigator.sendBeacon() respect private browsing mode; r=jdm

Comment 4

[(no longer active)]

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...)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Opening

Comment 6

[(no longer active)]

I spun off the appId part to bug 988107 to avoid the situation of multiple patches landing in this bug.

Comment 7

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1c527eeafc

Comment 8

[(no longer active)]

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.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We also need tests here.

Comment 10

[(no longer active)]

Yep, I'll write one this weekend.

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a902f2f072ca for non-unified bustage. I presume the https://tbpl.mozilla.org/php/getParsedLog.php?id=36704452&tree=Mozilla-Inbound error message is just as good, but I like the explicitness of https://tbpl.mozilla.org/php/getParsedLog.php?id=36704025&tree=Mozilla-Inbound more.

Comment 12

[(no longer active)]

Oops, relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/47cb8b25fc45

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47cb8b25fc45

Comment 14

[(no longer active)]

I'm getting a daily release tracking email from this bug because I guess we haven't closed it yet.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 16

[(no longer active)]

Attachment: 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

Comment 17

[(no longer active)]

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.

Comment 18

[(no longer active)]

Setting checkin-needed to get the first part landed on Aurora.

Comment 19

[Josh Matthews [:jdm]]

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.

Comment 20

[(no longer active)]

Landed the test here too, please uplift them both to Aurora!

https://hg.mozilla.org/integration/mozilla-inbound/rev/352a2109c6e6

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed the test out in http://hg.mozilla.org/integration/mozilla-inbound/rev/56dffd1a24f8 for m-oth bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=37027922&tree=Mozilla-Inbound

Comment 22

[(no longer active)]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a5d0788594a4
https://hg.mozilla.org/releases/mozilla-aurora/rev/b64ed600b9ed

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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