Open Bug 1338735 Opened 5 years ago Updated 5 years ago

removing all associated container cookies when containers are disabled/removed

Categories

(Core :: DOM: Security, defect, P3)

54 Branch
defect

Tracking

()

ASSIGNED
Tracking Status
firefox54 --- affected
firefox57 --- fix-optional

People

(Reporter: kjozwiak, Assigned: aryx, NeedInfo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][domsecurity-backlog])

Attachments

(1 file)

When a user disables/removes the container feature, we should be removing all associated containers cookies from cookies.sqlite. This prevents situations where a user thinks that disabling containers will remove all the previous sessions that have been established. The user might not log out properly, and just assume that disabling the feature will delete all the cookies that were created with containers.

Someone could potentially re-enable the feature, and use containers to access old sessions that haven't been logged out and are still being saved.

Example/STR:

* install the latest version of nightly
* login into facebook.com using the personal container
* completely disable the container feature via privacy.userContext.enabled under about:config
* restart nightly and the container feature should be completely disabled
* if you inspect cookies.sqlite in your profile folder, you'll notice that the cookies associated with the personal container are still being stored
* re-enable the container feature
* load facebook.com in a personal container and you'll still be logged in
Assignee: nobody → aryx.bugmail
Status: NEW → ASSIGNED
Comment on attachment 8839200 [details]
Bug 1338735 - remove all container cookies when container feature gets disabled.

https://reviewboard.mozilla.org/r/113906/#review115424

This is fairly close, but I'd like to see this once more.

::: browser/components/preferences/cookies.js:48
(Diff revision 1)
>  
>      if (!Services.prefs.getBoolPref("privacy.userContext.enabled")) {
>        document.getElementById("userContextRow").hidden = true;
>      }
> +
> +    Services.obs.notifyObservers(null, "cookiemanager-init", null);

I'd rather just have an onload handler and then wait 1 cycle of the event loop (yield setTimeout(, 0) or something) than add an observer on which add-ons will doubtless start relying etc. etc.

::: browser/components/preferences/in-content/tests/browser.ini:4
(Diff revision 1)
>  [DEFAULT]
>  support-files =
>    head.js
> +  file_set_cookies_for_containers.html

Please add this under the relevant test rather than as being necessary for every single test in this directory.

::: browser/components/preferences/in-content/tests/browser_cookies_removed_if_containers_disabled.js:8
(Diff revision 1)
> +const TEST_HOST = "example.com";
> +const TEST_URL = "http://" + TEST_HOST + "/browser/browser/components/preferences/in-content/tests/";
> +
> +const containersEnabledOriginalValue = Services.prefs.getBoolPref(CONTAINERS_ENABLED_PREF);
> +registerCleanupFunction(function() {
> +  Services.prefs.setBoolPref(CONTAINERS_ENABLED_PREF, containersEnabledOriginalValue);

Don't do this, just use SpecialPowers.pushPrefEnv at the start of your first task.

::: browser/components/preferences/in-content/tests/browser_cookies_removed_if_containers_disabled.js:44
(Diff revision 1)
> +  gBrowser.selectedTab = tab;
> +  tab.ownerGlobal.focus();
> +
> +  let browser = gBrowser.getBrowserForTab(tab);
> +  yield BrowserTestUtils.browserLoaded(browser);
> +  BrowserTestUtils.removeTab(tab);

You should yield for the removeTab.

::: browser/components/preferences/in-content/tests/browser_cookies_removed_if_containers_disabled.js:69
(Diff revision 1)
> +    yield* addCookieForUserContext(TEST_URL + "file_set_cookies_for_containers.html?" + userContextId, userContextId);
> +  }
> +
> +  yield checkCookies(CONTAINERS_COOKIES_ADD.length, "Correct number of cookies shown with containers enabled");
> +
> +  gBrowser.selectedBrowser.contentDocument.getElementById("browserContainersCheckbox").click();

Can you stick this in a temp at the top of the task? Doesn't look like you ever remove the preferences tab anyway.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:226
(Diff revision 1)
>    },
>  
> +  observe() {
> +    if (!Services.prefs.getBoolPref(this._CONTAINERS_ENABLED_PREF)) {
> +      this.getPublicIdentities()
> +          .forEach(({ userContextId } = identity) =>

Isn't the `= identity` bit here superfluous ?
Attachment #8839200 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8839200 [details]
Bug 1338735 - remove all container cookies when container feature gets disabled.

https://reviewboard.mozilla.org/r/113906/#review115426

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm:230
(Diff revision 1)
> +      this.getPublicIdentities()
> +          .forEach(({ userContextId } = identity) =>
> +                   Services.obs.notifyObservers(null,
> +                     "clear-origin-attributes-data",
> +                     JSON.stringify({ userContextId })));
> +    }

In this patch, please remove ContextualIdentityService.disableContainers(). This method has been introduced recently in bug 1340450, but, with this approach is not needed.
Thanks.
Sebastian, I'm triaging through several container bugs and was wondering if you still wanted to fix this? The same issue was fixed within the Containers Test Pilot [1] but I'm not sure if that fix could be applied in the platform implementation.
 
[1] https://github.com/mozilla/testpilot-containers/issues/174
Flags: needinfo?(aryx.bugmail)
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.