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 278005 8338cdb566e28bd2dcaf7ef726cdd49529ae431d
parent 278004 2eb35642f31d462c3978b62e369679fd3a45203f
child 278006 6786ccb1dba475cb92f0a69b8daf544d4213c8da
push id29840
push userryanvm@gmail.com
push dateFri, 01 Jan 2016 23:13:05 +0000
treeherdermozilla-central@5933251c8c98 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersevilpie
bugs1224579
milestone46.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 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