Closed Bug 1419591 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Security, defect, P1)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: tanvi, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
See Also: → 1419589
Attached patch part 1 - migration 2 to 3 (obsolete) — Splinter Review
Attachment #8930825 - Flags: review?(jkt)
Attached patch part 2 - tests (obsolete) — Splinter Review
This test should cover all except corrupted file. This is done in bug 1419589.
Attachment #8930826 - Flags: review?(jkt)
Attached patch part 1 - migration 2 to 3 (obsolete) — Splinter Review
Let's make eslint happy.
Attachment #8930825 - Attachment is obsolete: true
Attachment #8930825 - Flags: review?(jkt)
Attachment #8930827 - Flags: review?(jkt)
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?
Can you edit the test to change the minUserContextId to be 1 like Tanvi suggested, everything else looks fine here.
Flags: needinfo?(amarchesini)
My patch did what tanvi said except starting for userContextId 5. Let's start from 1.
Attachment #8930827 - Attachment is obsolete: true
Attachment #8930827 - Flags: review?(jkt)
Flags: needinfo?(amarchesini)
Attachment #8932264 - Flags: review?(jkt)
Attached patch part 2 - testsSplinter Review
Attachment #8930826 - Attachment is obsolete: true
Attachment #8930826 - Flags: review?(jkt)
Attachment #8932265 - Flags: review?(jkt)
Attachment #8932264 - Flags: review?(jkt) → review+
Attachment #8932265 - Flags: review?(jkt) → review+
Pushed by amarchesini@mozilla.com:
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
Backed out for failing Chrome tests on Linux Debug toolkit/content/tests/chrome/test_bug437844.xul 

Please see Bug 1419589
Flags: needinfo?(amarchesini)
Backout by ncsoregi@mozilla.com:
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
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/4fdab4c30448
https://hg.mozilla.org/mozilla-central/rev/bc908d7e80ff
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
baku, can you file a followup to remove this code in a few weeks?
Flags: needinfo?(amarchesini)
(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.
Depends on: 1425979
baku, can we remove this code now?
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.
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.