1419591 - One time clean up of Container cookie and site data in Nightly

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Tanvi Vyas[:tanvi]]

Bug 1347515 caused a regression for Nightly Containers users.  The bug was backed out within 4 days (landed 11/10, backed out 11/14). But during those 4 days, some Nightly users who restarted Nightly experienced odd Container behavior.  Namely, their custom container names, icons, and colors were gone.  However, the cookies and site data associated with them were not gone.  This bug is to do a one time clean up of site data associated with Containers in Nightly, to avoid any potential data leakage.

Here is a detailed example.  Assume a user created a custom container for Task-1 that had a userContextId=5.  After a Nightly restart during the regression period, it appeared to the user as if the custom container for Task-1 was gone.  The color, icon, and name for Task-1 were gone, and the container counter was reset, but the cookies that were created while using Task-1 still existed and were still indexed with userContextId=5.  The user then decided to create a new custom Container for Task-2 that got a userContextId=5 (because the counter was reset).  We do not want cookie data from Task-1 to still exist in the newly created Task-2 container, as that could be a privacy issue.

There is no way to know which users reset Nightly during this regression and were affected by the bug (it was a race condition) and no way of knowing which users had customized their Containers.  Even though this bug existed for a short period of time on Nightly only (which does not guarantee stability), we want to do our due diligence and clean up any potential for data leakage between Containers.  Hence, this bug is to wipe all cookies and site data associated with Containers one time for Nightly users only.  This bug should never make it to Beta.  We should back out of Nightly after a couple weeks (once the cleanup is complete).

This means that Nightly Containers users will get logged out of their services one time and have to log back in.  Users who do not use containers will not be affected.

To prevent such an regression from occurring again, we have filed followup bug 1419589 and plan to add more automated tests.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - migration 2 to 3

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - tests

This test should cover all except corrupted file. This is done in bug 1419589.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - migration 2 to 3

Let's make eslint happy.

Comment 4

[Tanvi Vyas[:tanvi]]

Comment on attachment 8930827 [details] [diff] [review]
part 1 - migration 2 to 3


>@@ -438,12 +454,39 @@ function _ContextualIdentityService(path
>+    // *Only in nightly* we delete data of the all non-default containers.
>+    if (AppConstants.NIGHTLY_BUILD) {
>+      // Let's start from the last 'known' userContextId.
>+      const minUserContextId =
>+        this._defaultIdentities[this._defaultIdentities.length - 1].userContextId;
>+      let maxUserContextId = minUserContextId;
>+      const enumerator = Services.cookies.enumerator;
>+      while (enumerator.hasMoreElements()) {
>+        const cookie = enumerator.getNext().QueryInterface(Ci.nsICookie);
>+        if (cookie.originAttributes.userContextId > maxUserContextId) {
>+          maxUserContextId = cookie.originAttributes.userContextId;
>+        }
>+      }
>+
>+      for (let i = minUserContextId; i <= maxUserContextId; ++i) {
>+        Services.obs.notifyObservers(null, "clear-origin-attributes-data",
>+                                     JSON.stringify({ userContextId: i }));
>+      }
>+    }
>+
>+    return data;
>+  },
> };

minUserContextId should be 1 here, and go to the max.  The max should be the larger of (the max we find from the cookies, the current max userContextId found by the counter).  This way, we ensure we clear out all data at least up to the current max userContextId (ex: what if there is no cookei associated with usercontextId 5, but there is localstorage).  We do also have to look at the cookies, because a user may only have 5 containers now, but previously they may have had 10.  The cookies help us determine this.

Does that make sense?

Comment 5

[Jonathan Kingston [:jkt] he/him]

Can you edit the test to change the minUserContextId to be 1 like Tanvi suggested, everything else looks fine here.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 1 - migration 2 to 3

My patch did what tanvi said except starting for userContextId 5. Let's start from 1.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 2 - tests

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc1d13d74ac
Migration 2 to 3 for ContextualIdentityService deletes all the container cookies, r=jkt
https://hg.mozilla.org/integration/mozilla-inbound/rev/47b38ef21292
Test for migration 2 to 3 for ContextualIdentityService data, r=jkt

Comment 9

[Natalia Csoregi [:nataliaCs]]

Backed out for failing Chrome tests on Linux Debug toolkit/content/tests/chrome/test_bug437844.xul 

Please see Bug 1419589

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8813c46b4f3
Backed out 3 changesets (bug 1419591, bug 1419589) for failing Chrome tests on Linux Debug toolkit/content/tests/chrome/test_bug437844.xul. r=backout on a CLOSED TREE

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fdab4c30448
Migration 2 to 3 for ContextualIdentityService deletes all the container cookies, r=jkt
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc908d7e80ff
Test for migration 2 to 3 for ContextualIdentityService data, r=jkt

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4fdab4c30448
https://hg.mozilla.org/mozilla-central/rev/bc908d7e80ff

Comment 13

[Tanvi Vyas[:tanvi]]

baku, can you file a followup to remove this code in a few weeks?

Comment 14

[Andrea Marchesini [:baku]]

(In reply to Tanvi Vyas[:tanvi] from comment #13)
> baku, can you file a followup to remove this code in a few weeks?

Keeping the NI in order to remember.

Comment 15

[Tanvi Vyas[:tanvi]]

baku, can we remove this code now?

Comment 16

[Andrea Marchesini [:baku]]

No needs to. I updated the test to check that we don't delete data when migrating from version 2 to 3 in beta/release.
We can keep this code because the cleaning of cookies happens only in nightly.