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

Status: RESOLVED FIXED

(Firefox :: Tours, enhancement, P1)

Description

[Matthew N. [:MattN]]

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

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1196105 - Heartbeat: Don't fire UITour notifications for private window heartbeats. r=bgrins,ehsan

Bug 1196105 - Heartbeat: Don't fire UITour notifications for private window heartbeats. r=bgrins,ehsan

Comment 2

[Matthew N. [:MattN]]

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

Comment 3

[Brian Grinstead [:bgrins]]

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'?)

Comment 4

[Matthew N. [:MattN]]

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 5

[Brian Grinstead [:bgrins]]

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 (

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e2ac20d24cb2

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent OSX browser_UITour_heartbeat.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=4392763&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/f581f987f024

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/586fdc6ed2c9

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/586fdc6ed2c9

Comment 10

[Matthew N. [:MattN]]

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

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1140b1a07842

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Florin, just fyi, here is the other one.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d3f403db226

Comment 16

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing ni? since Heartbeat in PB mode was tested in bug 1196104, and there was no follow up on that.