Bug 1224579: [webext] Fix the handling of domain cookies. r=evilpie
authorKris Maglione <maglione.k@gmail.com>
Wed, 23 Dec 2015 11:18:38 -0500
changeset 277975 8338cdb566e28bd2dcaf7ef726cdd49529ae431d
parent 277974 2eb35642f31d462c3978b62e369679fd3a45203f
child 277976 6786ccb1dba475cb92f0a69b8daf544d4213c8da
push id16829
push usermaglione.k@gmail.com
push dateFri, 01 Jan 2016 00:13:21 +0000
treeherderfx-team@8338cdb566e2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersevilpie
bugs1224579
milestone46.0a1
Bug 1224579: [webext] Fix the handling of domain cookies. r=evilpie
toolkit/components/extensions/ext-cookies.js
toolkit/components/extensions/test/mochitest/test_ext_cookies_permissions.html
--- a/toolkit/components/extensions/ext-cookies.js
+++ b/toolkit/components/extensions/ext-cookies.js
@@ -66,20 +66,29 @@ function checkSetCookiePermissions(exten
   if (uri.scheme != "http" && uri.scheme != "https") {
     return false;
   }
 
   if (!extension.whiteListedHosts.matchesIgnoringPath(uri)) {
     return false;
   }
 
-  // The cookie service ignores any leading '.' passed in, but adds one if the
-  // proposed domain is not the exact domain of the URL. So start by stripping
-  // it off.
-  cookie.host = cookie.host.replace(/^\./, "");
+  if (!cookie.host) {
+    // If no explicit host is specified, this becomes a host-only cookie.
+    cookie.host = uri.host;
+    return true;
+  }
+
+  // A leading "." is not expected, but is tolerated if it's not the only
+  // character in the host. If there is one, start by stripping it off. We'll
+  // add a new one on success.
+  if (cookie.host.length > 1) {
+    cookie.host = cookie.host.replace(/^\./, "");
+  }
+  cookie.host = cookie.host.toLowerCase();
 
   if (cookie.host != uri.host) {
     // Not an exact match, so check for a valid subdomain.
     let baseDomain;
     try {
       baseDomain = Services.eTLD.getBaseDomain(uri);
     } catch (e) {
       if (e.result == Cr.NS_ERROR_HOST_IS_IP_ADDRESS ||
@@ -99,20 +108,21 @@ function checkSetCookiePermissions(exten
     // unrelated domains.
     if (!isSubdomain(cookie.host, baseDomain) ||
         !isSubdomain(uri.host, cookie.host)) {
       return false;
     }
 
     // RFC2109 suggests that we may only add cookies for sub-domains 1-level
     // below us, but enforcing that would break the web, so we don't.
+  }
 
-    // This is a valid sub-domain cookie, so add (or re-add) a leading dot.
-    cookie.host = "." + cookie.host;
-  }
+  // An explicit domain was passed, so add a leading "." to make this a
+  // domain cookie.
+  cookie.host = "." + cookie.host;
 
   // We don't do any significant checking of path permissions. RFC2109
   // suggests we only allow sites to add cookies for sub-paths, similar to
   // same origin policy enforcement, but no-one implements this.
 
   return true;
 }
 
@@ -247,23 +257,16 @@ extensions.registerSchemaAPI("cookies", 
         let result = Array.from(query(details, allowed, extension), convert);
 
         runSafe(context, callback, result);
       },
 
       set: function(details, callback) {
         let uri = NetUtil.newURI(details.url).QueryInterface(Ci.nsIURL);
 
-        let domain;
-        if (details.domain !== null) {
-          domain = details.domain.toLowerCase();
-        } else {
-          domain = uri.host; // "If omitted, the cookie becomes a host-only cookie."
-        }
-
         let path;
         if (details.path !== null) {
           path = details.path;
         } else {
           // This interface essentially emulates the behavior of the
           // Set-Cookie header. In the case of an omitted path, the cookie
           // service uses the directory path of the requesting URL, ignoring
           // any filename or query parameters.
@@ -273,17 +276,17 @@ extensions.registerSchemaAPI("cookies", 
         let name = details.name !== null ? details.name : "";
         let value = details.value !== null ? details.value : "";
         let secure = details.secure !== null ? details.secure : false;
         let httpOnly = details.httpOnly !== null ? details.httpOnly : false;
         let isSession = details.expirationDate === null;
         let expiry = isSession ? 0 : details.expirationDate;
         // Ignore storeID.
 
-        let cookieAttrs = { host: domain, path: path, isSecure: secure };
+        let cookieAttrs = { host: details.domain, path: path, isSecure: secure };
         if (checkSetCookiePermissions(extension, uri, cookieAttrs)) {
           // TODO: Set |lastError| when false.
           //
           // The permission check may have modified the domain, so use
           // the new value instead.
           Services.cookies.add(cookieAttrs.host, path, name, value,
                                secure, httpOnly, isSession, expiry);
         }
--- a/toolkit/components/extensions/test/mochitest/test_ext_cookies_permissions.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_cookies_permissions.html
@@ -108,30 +108,32 @@ function* testCookies(options) {
     },
 
     background: `(${background})(${JSON.stringify(options)})`,
   });
 
 
   let cookieSvc = SpecialPowers.Services.cookies;
 
+  let domain = options.domain.replace(/^\.?/, ".");
+
   // This will be evicted after we add a fourth cookie.
-  cookieSvc.add(options.domain, "/", "evicted", "bar", options.secure, false, false, options.expiry);
+  cookieSvc.add(domain, "/", "evicted", "bar", options.secure, false, false, options.expiry);
   // This will be modified by the background script.
-  cookieSvc.add(options.domain, "/", "foo", "bar", options.secure, false, false, options.expiry);
+  cookieSvc.add(domain, "/", "foo", "bar", options.secure, false, false, options.expiry);
   // This will be deleted by the background script.
-  cookieSvc.add(options.domain, "/", "deleted", "bar", options.secure, false, false, options.expiry);
+  cookieSvc.add(domain, "/", "deleted", "bar", options.secure, false, false, options.expiry);
 
 
   yield extension.startup();
 
   yield extension.awaitMessage("change-cookies");
-  cookieSvc.add(options.domain, "/", "x", "y", options.secure, false, false, options.expiry);
-  cookieSvc.add(options.domain, "/", "x", "z", options.secure, false, false, options.expiry);
-  cookieSvc.remove(options.domain, "x", "/", false);
+  cookieSvc.add(domain, "/", "x", "y", options.secure, false, false, options.expiry);
+  cookieSvc.add(domain, "/", "x", "z", options.secure, false, false, options.expiry);
+  cookieSvc.remove(domain, "x", "/", false);
   extension.sendMessage("cookies-changed");
 
   yield extension.awaitFinish("cookie-permissions");
   yield extension.unload();
 
 
   function getCookies(host) {
     let cookies = [];
@@ -172,17 +174,17 @@ function* testCookies(options) {
 
     is(cookies[0].name, "deleted", "correct second cookie name");
 
     is(cookies[1].name, "foo", "correct cookie name");
     is(cookies[1].value, "bar", "correct cookie value");
   }
 
   for (let cookie of cookies) {
-    cookieSvc.remove(options.domain, cookie.name, "/", false);
+    cookieSvc.remove(cookie.host, cookie.name, "/", false);
   }
   // Make sure we don't silently poison subsequent tests if something goes wrong.
   is(getCookies(options.domain).length, 0, "cookies cleared");
 }
 
 
 add_task(function* init() {
   // We need to trigger a cookie eviction in order to test our batch delete