Closed Bug 1196105 Opened 5 years ago Closed 5 years ago

Heartbeat: Audit/fix possible data leaks for the private window targeting option

Categories

(Firefox :: Tours, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 43
Iteration:
43.2 - Sep 7
Tracking Status
firefox41 --- fixed
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

(Whiteboard: [fxprivacy])

Attachments

(1 file)

For the private window option added in bug 1196104, we should ensure that it doesn't leak whether a user has/had a private window open. We should double-check that the callbacks/URLs don't leak this information and fix cases that do.

Some cases to fix:
* All of the Heartbeat notifications/events leak to Heartbeat whether or not a private window is open. We should probably always suppress these events/notifications when heartbeat is targeting private windows.
* The flowId shouldn't be passed along in URLs as the flowId could be specific to the private browsing recipe
Flags: qe-verify?
Priority: -- → P1
Bug 1196105 - Heartbeat: Don't fire UITour notifications for private window heartbeats. r=bgrins,ehsan
Attachment #8652014 - Flags: review?(bgrinstead)
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 43.1 - Aug 24
Flags: qe-verify? → qe-verify-
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #0)
> * All of the Heartbeat notifications/events leak to Heartbeat whether or not
> a private window is open. We should probably always suppress these
> events/notifications when heartbeat is targeting private windows.

My patch covers this case. See bug 1196104 comment 2 for testing instructions and look at the test's use of UITour.observe.

> * The flowId shouldn't be passed along in URLs as the flowId could be
> specific to the private browsing recipe

I realized that this isn't necessary since heartbeat already controls the rest of the URL. Not sending one parameter achieves nothing.
Iteration: 43.1 - Aug 24 → 43.2 - Sep 7
https://reviewboard.mozilla.org/r/17049/#review15293

::: browser/components/uitour/test/browser_UITour_heartbeat.js:388
(Diff revision 1)
> +    yield new Promise((resolve) => {

I don't understand what this is doing, but maybe I'm missing something.  It looks like 'resolve' is being passed in as the second parameter to the gContentAPI.observe function (but that function seems to be expecting a string 'aTopic'?)
https://reviewboard.mozilla.org/r/17049/#review15293

> I don't understand what this is doing, but maybe I'm missing something.  It looks like 'resolve' is being passed in as the second parameter to the gContentAPI.observe function (but that function seems to be expecting a string 'aTopic'?)

Could you include more context in your review comments? In this case seeing the line where resolve was used would be relevant.

This is ensuring that the observer is setup. The second argument is the callback to tell you this. You must be looking at the wrong function. The definition is at https://mxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour-lib.js?rev=e0f2fd166388#82
Comment on attachment 8652014 [details]
MozReview Request: Bug 1196105 - Heartbeat: Don't fire UITour notifications for private window heartbeats. r=bgrins,ehsan

https://reviewboard.mozilla.org/r/17051/#review15311

::: browser/components/uitour/test/browser_UITour_heartbeat.js:389
(Diff revision 1)
> +      gContentAPI.observe(function (aEventName, aData) {

Nit: extra whitespace between function and (
Attachment #8652014 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/586fdc6ed2c9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment on attachment 8652014 [details]
MozReview Request: Bug 1196105 - Heartbeat: Don't fire UITour notifications for private window heartbeats. r=bgrins,ehsan

Approval Request Comment
[Feature/regressing bug #]: Private Browsing with Tracking Protection measurements via HeartBeat (using UITour)
[User impact if declined]: No way to measure the (change in) user sentiment towards private browsing so we won't be able to address user concerns. Specifically this patch reduces the amount of information Heartbeat gets about heartbeat UI interactions to avoid leaking whether a user has a private window open. We want to get a baseline in 41 to measure 42 sentiment against.
[Describe test coverage new/current, TreeHerder]: Automated tests for the notification/event behaviour.
[Risks and why]: Low risk since this is an isolated change only affecting UITour for the heartbeat API. Worst case is that the heartbeat data collection doesn't work which wouldn't be end of the world as the data can also be passed with URLs params.
[String/UUID change made/needed]: None
Attachment #8652014 - Flags: approval-mozilla-beta?
Attachment #8652014 - Flags: approval-mozilla-aurora?
Comment on attachment 8652014 [details]
MozReview Request: Bug 1196105 - Heartbeat: Don't fire UITour notifications for private window heartbeats. r=bgrins,ehsan

Same question as in https://bugzilla.mozilla.org/show_bug.cgi?id=1196102#c12
Attachment #8652014 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8652014 [details]
MozReview Request: Bug 1196105 - Heartbeat: Don't fire UITour notifications for private window heartbeats. r=bgrins,ehsan

We need this patch in Beta41 in order to get baseline numbers. I would have preferred to stabilize this patch on Aurora channel but the sooner this gets uplifted to Beta, we can get more users to test it and identify regressions. Beta41+

It's good to see an automated test.
Attachment #8652014 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Florin, just fyi, here is the other one.
Flags: needinfo?(florin.mezei)
Removing ni? since Heartbeat in PB mode was tested in bug 1196104, and there was no follow up on that.
Flags: needinfo?(florin.mezei)
You need to log in before you can comment on or make changes to this bug.