Bug 1352365 - Remove duplicate PrivacyLevel checks in SessionCookies.jsm r=mikedeboer
authorTim Taubert <ttaubert@mozilla.com>
Fri, 31 Mar 2017 11:54:18 +0200
changeset 398854 ecc68a5077b73d53631017407b09e39fbd713ffb
parent 398853 15e815413234d71fbc93bb0dc78f3d589b6e39f4
child 398855 7cb01f9b0aedb6b509128cd325d8a5b026e55825
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmikedeboer
bugs1352365
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1352365 - Remove duplicate PrivacyLevel checks in SessionCookies.jsm r=mikedeboer PrivacyLevel checks currently allow to disable storing secure cookies and any cookies belonging to an HTTPS host, or completely disable storing cookies. We call PrivacyLevel.canSave() for every host found in the shistory of a given window's tabs. We then call it again for every cookie when retrieving all cookies stored for a given host. The two different privacy checks exist because in the past an HTTP site could send a secure cookie too. Since Firefox 52 this isn’t possible anymore, only HTTPS sites can send secure cookies. So as soon as nsICookie.isSecure=true we know the site was loaded over TLS. That means there are the following scenarios: [PRIVACY_LEVEL=NONE] (default) We store all cookies. [PRIVACY_LEVEL=FULL] We store no cookies at all. [PRIVACY_LEVEL=ENCRYPTED] HTTP site sends cookie: Store. HTTP site sends secure cookie: Can't happen since Fx52 HTTPS site sends cookie: Store. The site is HTTPS but we should store the cookie anyway because the "Secure" directive is missing. That means the site wants us to send it for HTTP requests too. HTTPS site sends secure cookie: Don't store. This allows us to simplify the code and remove the per-host PrivacyLevel checks. Checking nsICookie.isSecure is enough to tell whether we want to keep a cookie or not.
browser/components/sessionstore/SessionCookies.jsm
browser/components/sessionstore/test/browser_cookies.js
browser/components/sessionstore/test/browser_cookies.sjs
--- a/browser/components/sessionstore/SessionCookies.jsm
+++ b/browser/components/sessionstore/SessionCookies.jsm
@@ -23,18 +23,18 @@ const MAX_EXPIRY = Math.pow(2, 62);
 /**
  * The external API implemented by the SessionCookies module.
  */
 this.SessionCookies = Object.freeze({
   update(windows) {
     SessionCookiesInternal.update(windows);
   },
 
-  getHostsForWindow(window, checkPrivacy = false) {
-    return SessionCookiesInternal.getHostsForWindow(window, checkPrivacy);
+  getHostsForWindow(window) {
+    return SessionCookiesInternal.getHostsForWindow(window);
   },
 
   restore(cookies) {
     SessionCookiesInternal.restore(cookies);
   }
 });
 
 /**
@@ -53,26 +53,30 @@ var SessionCookiesInternal = {
    *
    * @param windows
    *        Array of window state objects.
    *        [{ tabs: [...], cookies: [...] }, ...]
    */
   update(windows) {
     this._ensureInitialized();
 
+    // Check whether we're allowed to store cookies.
+    let storeAnyCookies = PrivacyLevel.canSave(false);
+    let storeSecureCookies = PrivacyLevel.canSave(true);
+
     for (let window of windows) {
       let cookies = [];
 
-      // Collect all cookies for the current window.
-      for (let host of this.getHostsForWindow(window, true)) {
-        for (let cookie of CookieStore.getCookiesForHost(host)) {
-          // getCookiesForHost() will only return hosts with the right privacy
-          // rules. Check again here to exclude HTTPS-only cookies if needed.
-          if (PrivacyLevel.canSave(cookie.secure)) {
-            cookies.push(cookie);
+      if (storeAnyCookies) {
+        // Collect all cookies for the current window.
+        for (let host of this.getHostsForWindow(window)) {
+          for (let cookie of CookieStore.getCookiesForHost(host)) {
+            if (!cookie.secure || storeSecureCookies) {
+              cookies.push(cookie);
+            }
           }
         }
       }
 
       // Don't include/keep empty cookie sections.
       if (cookies.length) {
         window.cookies = cookies;
       } else if ("cookies" in window) {
@@ -82,26 +86,24 @@ var SessionCookiesInternal = {
   },
 
   /**
    * Returns a map of all hosts for a given window that we might want to
    * collect cookies for.
    *
    * @param window
    *        A window state object containing tabs with history entries.
-   * @param checkPrivacy (bool)
-   *        Whether to check the privacy level for each host.
    * @return {set} A set of hosts for a given window state object.
    */
-  getHostsForWindow(window, checkPrivacy = false) {
+  getHostsForWindow(window) {
     let hosts = new Set();
 
     for (let tab of window.tabs) {
       for (let entry of tab.entries) {
-        this._extractHostsFromEntry(entry, hosts, checkPrivacy);
+        this._extractHostsFromEntry(entry, hosts);
       }
     }
 
     return hosts;
   },
 
   /**
    * Restores a given list of session cookies.
@@ -166,36 +168,29 @@ var SessionCookiesInternal = {
   /**
    * Fill a given map with hosts found in the given entry's session history and
    * any child entries.
    *
    * @param entry
    *        the history entry, serialized
    * @param hosts
    *        the set that will be used to store hosts
-   * @param checkPrivacy
-   *        should we check the privacy level for https
    */
-  _extractHostsFromEntry(entry, hosts, checkPrivacy) {
+  _extractHostsFromEntry(entry, hosts) {
     try {
       // It's alright if this throws for about: URIs.
       let {host, scheme} = Utils.makeURI(entry.url);
-
-      if (scheme == "file") {
+      if (/^(file|https?)$/.test(scheme)) {
         hosts.add(host);
-      } else if (/https?/.test(scheme)) {
-        if (!checkPrivacy || PrivacyLevel.canSave(scheme == "https")) {
-          hosts.add(host);
-        }
       }
     } catch (ex) { }
 
     if (entry.children) {
       for (let child of entry.children) {
-        this._extractHostsFromEntry(child, hosts, checkPrivacy);
+        this._extractHostsFromEntry(child, hosts);
       }
     }
   },
 
   /**
    * Updates or adds a given cookie to the store.
    */
   _updateCookie(cookie) {
--- a/browser/components/sessionstore/test/browser_cookies.js
+++ b/browser/components/sessionstore/test/browser_cookies.js
@@ -1,17 +1,17 @@
 "use strict";
 
 const PATH = "/browser/browser/components/sessionstore/test/";
 
 /**
  * Remove all cookies to start off a clean slate.
  */
 add_task(function* test_setup() {
-  requestLongerTimeout(2);
+  requestLongerTimeout(3);
   Services.cookies.removeAll();
 });
 
 /**
  * Test multiple scenarios with different Set-Cookie header domain= params.
  */
 add_task(function* test_run() {
   // Set-Cookie: foobar=random()
@@ -75,32 +75,134 @@ add_task(function* test_run() {
     domain: ".www.example.com",
     cookieHost: ".www.example.com",
     cookieURIs: ["http://www.example.com/" + PATH],
     noCookieURIs: ["http://example.com"]
   });
 });
 
 /**
+ * Test multiple scenarios with different privacy levels.
+ */
+add_task(function* test_run_privacy_level() {
+  registerCleanupFunction(() => {
+    Services.prefs.clearUserPref("browser.sessionstore.privacy_level");
+  });
+
+  // With the default privacy level we collect all cookies.
+  yield testCookieCollection({
+    host: "http://example.com",
+    domain: ".example.com",
+    cookieHost: ".example.com",
+    cookieURIs: ["https://example.com/" + PATH, "http://example.com/" + PATH],
+    noCookieURIs: ["about:robots"]
+  });
+
+  // With the default privacy level we collect all cookies.
+  yield testCookieCollection({
+    host: "https://example.com",
+    domain: ".example.com",
+    cookieHost: ".example.com",
+    cookieURIs: ["https://example.com/" + PATH, "http://example.com/" + PATH],
+    noCookieURIs: ["about:robots"]
+  });
+
+  // With the default privacy level we collect all cookies.
+  yield testCookieCollection({
+    isSecure: true,
+    host: "https://example.com",
+    domain: ".example.com",
+    cookieHost: ".example.com",
+    cookieURIs: ["https://example.com/" + PATH, "http://example.com/" + PATH],
+    noCookieURIs: ["about:robots"]
+  });
+
+  // Set level=encrypted, don't store any secure cookies.
+  Services.prefs.setIntPref("browser.sessionstore.privacy_level", 1);
+
+  // With level=encrypted, non-secure cookies will be stored.
+  yield testCookieCollection({
+    host: "http://example.com",
+    domain: ".example.com",
+    cookieHost: ".example.com",
+    cookieURIs: ["https://example.com/" + PATH, "http://example.com/" + PATH],
+    noCookieURIs: ["about:robots"]
+  });
+
+  // With level=encrypted, non-secure cookies will be stored,
+  // even if sent by an HTTPS site.
+  yield testCookieCollection({
+    host: "https://example.com",
+    domain: ".example.com",
+    cookieHost: ".example.com",
+    cookieURIs: ["https://example.com/" + PATH, "http://example.com/" + PATH],
+    noCookieURIs: ["about:robots"]
+  });
+
+  // With level=encrypted, secure cookies will NOT be stored.
+  yield testCookieCollection({
+    isSecure: true,
+    host: "https://example.com",
+    domain: ".example.com",
+    cookieHost: ".example.com",
+    noCookieURIs: ["https://example.com/" + PATH, "http://example.com/" + PATH]
+  });
+
+  // Set level=full, don't store any cookies.
+  Services.prefs.setIntPref("browser.sessionstore.privacy_level", 2);
+
+  // With level=full we must not store any cookies.
+  yield testCookieCollection({
+    host: "http://example.com",
+    domain: ".example.com",
+    cookieHost: ".example.com",
+    noCookieURIs: ["https://example.com/" + PATH, "http://example.com/" + PATH]
+  });
+
+  // With level=full we must not store any cookies.
+  yield testCookieCollection({
+    host: "https://example.com",
+    domain: ".example.com",
+    cookieHost: ".example.com",
+    noCookieURIs: ["https://example.com/" + PATH, "http://example.com/" + PATH]
+  });
+
+  // With level=full we must not store any cookies.
+  yield testCookieCollection({
+    isSecure: true,
+    host: "https://example.com",
+    domain: ".example.com",
+    cookieHost: ".example.com",
+    noCookieURIs: ["https://example.com/" + PATH, "http://example.com/" + PATH]
+  });
+
+  Services.prefs.clearUserPref("browser.sessionstore.privacy_level");
+});
+
+/**
  * Generic test function to check sessionstore's cookie collection module with
  * different cookie domains given in the Set-Cookie header. See above for some
  * usage examples.
  */
 var testCookieCollection = async function(params) {
   let tab = gBrowser.addTab("about:blank");
   let browser = tab.linkedBrowser;
 
   let urlParams = new URLSearchParams();
   let value = Math.random();
   urlParams.append("value", value);
 
   if (params.domain) {
     urlParams.append("domain", params.domain);
   }
 
+  if (params.isSecure) {
+    urlParams.append("secure", "1");
+  }
+
   // Construct request URI.
   let requestUri = `${params.host}${PATH}browser_cookies.sjs?${urlParams}`;
 
   // Wait for the browser to load and the cookie to be set.
   // These two events can probably happen in no particular order,
   // so let's wait for them in parallel.
   await Promise.all([
     waitForNewCookie(),
--- a/browser/components/sessionstore/test/browser_cookies.sjs
+++ b/browser/components/sessionstore/test/browser_cookies.sjs
@@ -7,15 +7,20 @@ Components.utils.importGlobalProperties(
 
 function handleRequest(req, resp) {
   resp.setStatusLine(req.httpVersion, 200);
 
   let params = new URLSearchParams(req.queryString);
   let value = params.get("value");
 
   let domain = "";
-  if  (params.has("domain")) {
+  if (params.has("domain")) {
     domain = `; Domain=${params.get("domain")}`;
   }
 
-  resp.setHeader("Set-Cookie", `foobar=${value}${domain}`);
+  let secure = "";
+  if (params.has("secure")) {
+    secure = "; Secure";
+  }
+
+  resp.setHeader("Set-Cookie", `foobar=${value}${domain}${secure}`);
   resp.write("<meta charset=utf-8>hi");
 }