Bug 1254221 - Error handling for WebExtension cookies API draft
authorJesper Kristensen <mail@jesperkristensen.dk>
Sun, 08 May 2016 23:24:35 +0200
changeset 367103 cbcd9f401d8f8bce930b78e8b4a45a8063e0c056
parent 367102 c98b533aaa06272493f373a2be63a6a7a1fefee9
child 520915 8ca9af70b6b9e03564c28fe6248c6e674575203d
push id18139
push usermail@jesperkristensen.dk
push dateSat, 14 May 2016 11:16:02 +0000
bugs1254221
milestone49.0a1
Bug 1254221 - Error handling for WebExtension cookies API MozReview-Commit-ID: EXANQbcJyeA
toolkit/components/extensions/ext-cookies.js
toolkit/components/extensions/test/mochitest/test_ext_cookies.html
--- a/toolkit/components/extensions/ext-cookies.js
+++ b/toolkit/components/extensions/ext-cookies.js
@@ -125,156 +125,167 @@ function checkSetCookiePermissions(exten
 
   // 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;
 }
 
-function* query(detailsIn, props, extension, isPrivate) {
-  // Different callers want to filter on different properties. |props|
-  // tells us which ones they're interested in.
-  let details = {};
-  props.forEach(property => {
-    if (detailsIn[property] !== null) {
-      details[property] = detailsIn[property];
+extensions.registerSchemaAPI("cookies", "cookies", (extension, context) => {
+  function* query(detailsIn, props) {
+    // Different callers want to filter on different properties. |props|
+    // tells us which ones they're interested in.
+    let details = {};
+    props.forEach(property => {
+      if (detailsIn[property] !== null) {
+        details[property] = detailsIn[property];
+      }
+    });
+
+    if ("domain" in details) {
+      details.domain = details.domain.toLowerCase().replace(/^\./, "");
     }
-  });
-
-  if ("domain" in details) {
-    details.domain = details.domain.toLowerCase().replace(/^\./, "");
-  }
 
-  if(details.storeId == DEFAULT_STORE) {
-    isPrivate = false;
-  } else if (details.storeId == PRIVATE_STORE) {
-    isPrivate = true;
-  } else if ("storeId" in details) {
-    return;
-  }
+    let isPrivate = context.incognito;
+    if(details.storeId == DEFAULT_STORE) {
+      isPrivate = false;
+    } else if (details.storeId == PRIVATE_STORE) {
+      isPrivate = true;
+    } else if ("storeId" in details) {
+      throw new context.cloneScope.Error("Unknown storeId");
+    }
 
-  // We can use getCookiesFromHost for faster searching.
-  let enumerator;
-  let uri;
-  if ("url" in details) {
-    try {
-      uri = NetUtil.newURI(details.url).QueryInterface(Ci.nsIURL);
+    // We can use getCookiesFromHost for faster searching.
+    let enumerator;
+    let uri;
+    if ("url" in details) {
+      try {
+        uri = NetUtil.newURI(details.url).QueryInterface(Ci.nsIURL);
+      } catch (ex) {
+        // This often happens for about: URLs
+      }
+      if (!uri || uri.scheme != "http" && uri.scheme != "https" ||
+          !extension.whiteListedHosts.matchesIgnoringPath(uri)) {
+        if (props.includes("domain")) {
+          return; // Scilently ignore for getAll
+        }
+        throw new context.cloneScope.Error(`No host permissions for cookies at url: "${details.url}".`);
+      }
       Services.cookies.usePrivateMode(isPrivate, () => {
         enumerator = Services.cookies.getCookiesFromHost(uri.host);
       });
-    } catch (ex) {
-      // This often happens for about: URLs
-      return;
-    }
-  } else if ("domain" in details) {
-    Services.cookies.usePrivateMode(isPrivate, () => {
-      enumerator = Services.cookies.getCookiesFromHost(details.domain);
-    });
-  } else {
-    Services.cookies.usePrivateMode(isPrivate, () => {
-      enumerator = Services.cookies.enumerator;
-    });
-  }
-
-  // Based on nsCookieService::GetCookieStringInternal
-  function matches(cookie) {
-    function domainMatches(host) {
-      return cookie.rawHost == host || (cookie.isDomain && host.endsWith(cookie.host));
+    } else if ("domain" in details) {
+      Services.cookies.usePrivateMode(isPrivate, () => {
+        enumerator = Services.cookies.getCookiesFromHost(details.domain);
+      });
+    } else {
+      Services.cookies.usePrivateMode(isPrivate, () => {
+        enumerator = Services.cookies.enumerator;
+      });
     }
 
-    function pathMatches(path) {
-      let cookiePath = cookie.path.replace(/\/$/, "");
+    // Based on nsCookieService::GetCookieStringInternal
+    function matches(cookie) {
+      function domainMatches(host) {
+        return cookie.rawHost == host || (cookie.isDomain && host.endsWith(cookie.host));
+      }
+
+      function pathMatches(path) {
+        let cookiePath = cookie.path.replace(/\/$/, "");
+
+        if (!path.startsWith(cookiePath)) {
+          return false;
+        }
+
+        // path == cookiePath, but without the redundant string compare.
+        if (path.length == cookiePath.length) {
+          return true;
+        }
 
-      if (!path.startsWith(cookiePath)) {
+        // URL path is a substring of the cookie path, so it matches if, and
+        // only if, the next character is a path delimiter.
+        let pathDelimiters = ["/", "?", "#", ";"];
+        return pathDelimiters.includes(path[cookiePath.length]);
+      }
+
+      // "Restricts the retrieved cookies to those that would match the given URL."
+      if (uri) {
+        if (!domainMatches(uri.host)) {
+          return false;
+        }
+
+        if (cookie.isSecure && uri.scheme != "https") {
+          return false;
+        }
+
+        if (!pathMatches(uri.path)) {
+          return false;
+        }
+      }
+
+      if ("name" in details && details.name != cookie.name) {
         return false;
       }
 
-      // path == cookiePath, but without the redundant string compare.
-      if (path.length == cookiePath.length) {
-        return true;
+      // "Restricts the retrieved cookies to those whose domains match or are subdomains of this one."
+      if ("domain" in details && !isSubdomain(cookie.rawHost, details.domain)) {
+        return false;
       }
 
-      // URL path is a substring of the cookie path, so it matches if, and
-      // only if, the next character is a path delimiter.
-      let pathDelimiters = ["/", "?", "#", ";"];
-      return pathDelimiters.includes(path[cookiePath.length]);
-    }
-
-    // "Restricts the retrieved cookies to those that would match the given URL."
-    if (uri) {
-      if (!domainMatches(uri.host)) {
+      // "Restricts the retrieved cookies to those whose path exactly matches this string.""
+      if ("path" in details && details.path != cookie.path) {
         return false;
       }
 
-      if (cookie.isSecure && uri.scheme != "https") {
+      if ("secure" in details && details.secure != cookie.isSecure) {
+        return false;
+      }
+
+      if ("session" in details && details.session != cookie.isSession) {
+        return false;
+      }
+
+      // Check that the extension has permissions for this host.
+      if (!extension.whiteListedHosts.matchesCookie(cookie)) {
         return false;
       }
 
-      if (!pathMatches(uri.path)) {
-        return false;
-      }
-    }
-
-    if ("name" in details && details.name != cookie.name) {
-      return false;
-    }
-
-    // "Restricts the retrieved cookies to those whose domains match or are subdomains of this one."
-    if ("domain" in details && !isSubdomain(cookie.rawHost, details.domain)) {
-      return false;
-    }
-
-    // "Restricts the retrieved cookies to those whose path exactly matches this string.""
-    if ("path" in details && details.path != cookie.path) {
-      return false;
+      return true;
     }
 
-    if ("secure" in details && details.secure != cookie.isSecure) {
-      return false;
-    }
-
-    if ("session" in details && details.session != cookie.isSession) {
-      return false;
+    while (enumerator.hasMoreElements()) {
+      let cookie = enumerator.getNext().QueryInterface(Ci.nsICookie2);
+      if (matches(cookie)) {
+      yield {cookie, isPrivate};
+      }
     }
-
-    // Check that the extension has permissions for this host.
-    if (!extension.whiteListedHosts.matchesCookie(cookie)) {
-      return false;
-    }
-
-    return true;
   }
 
-  while (enumerator.hasMoreElements()) {
-    let cookie = enumerator.getNext().QueryInterface(Ci.nsICookie2);
-    if (matches(cookie)) {
-      yield {cookie, isPrivate};
-    }
-  }
-}
-
-extensions.registerSchemaAPI("cookies", "cookies", (extension, context) => {
   let self = {
     cookies: {
       get: function(details) {
-        // FIXME: We don't sort by length of path and creation time.
-        for (let cookie of query(details, ["url", "name", "storeId"], extension, context.incognito)) {
-          return Promise.resolve(convert(cookie));
-        }
+        return Promise.resolve().then(() => {
+          // FIXME: We don't sort by length of path and creation time.
+          for (let cookie of query(details, ["url", "name", "storeId"])) {
+            return convert(cookie);
+          }
 
-        // Found no match.
-        return Promise.resolve(null);
+          // Found no match.
+          return null;
+        });
       },
 
       getAll: function(details) {
-        let allowed = ["url", "name", "domain", "path", "secure", "session", "storeId"];
-        let result = Array.from(query(details, allowed, extension, context.incognito), convert);
+        return Promise.resolve().then(() => {
+          let allowed = ["url", "name", "domain", "path", "secure", "session", "storeId"];
+          let result = Array.from(query(details, allowed), convert);
 
-        return Promise.resolve(result);
+          return result;
+        });
       },
 
       set: function(details) {
         let uri = NetUtil.newURI(details.url).QueryInterface(Ci.nsIURL);
 
         let path;
         if (details.path !== null) {
           path = details.path;
@@ -293,48 +304,50 @@ extensions.registerSchemaAPI("cookies", 
         let isSession = details.expirationDate === null;
         let expiry = isSession ? Number.MAX_SAFE_INTEGER : details.expirationDate;
         let isPrivate = context.incognito;
         if(details.storeId == DEFAULT_STORE) {
           isPrivate = false;
         } else if (details.storeId == PRIVATE_STORE) {
           isPrivate = true;
         } else if (details.storeId !== null) {
-          return Promise.reject({message: "Unknown storeId"});
+          return Promise.reject(new context.cloneScope.Error("Unknown storeId"));
         }
 
         let cookieAttrs = {host: details.domain, path: path, isSecure: secure};
         if (!checkSetCookiePermissions(extension, uri, cookieAttrs)) {
-          return Promise.reject({message: `Permission denied to set cookie ${JSON.stringify(details)}`});
+          return Promise.reject(new context.cloneScope.Error(`Permission denied to set cookie ${JSON.stringify(details)}`));
         }
 
         // The permission check may have modified the domain, so use
         // the new value instead.
         Services.cookies.usePrivateMode(isPrivate, () => {
           Services.cookies.add(cookieAttrs.host, path, name, value,
                                secure, httpOnly, isSession, expiry);
         });
 
         return self.cookies.get(details);
       },
 
       remove: function(details) {
-        for (let {cookie, isPrivate} of query(details, ["url", "name", "storeId"], extension, context.incognito)) {
-          Services.cookies.usePrivateMode(isPrivate, () => {
-            Services.cookies.remove(cookie.host, cookie.name, cookie.path, false, cookie.originAttributes);
-          });
-          // Todo: could there be multiple per subdomain?
-          return Promise.resolve({
-            url: details.url,
-            name: details.name,
-            storeId: isPrivate ? PRIVATE_STORE : DEFAULT_STORE,
-          });
-        }
+        return Promise.resolve().then(() => {
+          for (let {cookie, isPrivate} of query(details, ["url", "name", "storeId"])) {
+            Services.cookies.usePrivateMode(isPrivate, () => {
+              Services.cookies.remove(cookie.host, cookie.name, cookie.path, false, cookie.originAttributes);
+            });
+            // Todo: could there be multiple per subdomain?
+            return {
+              url: details.url,
+              name: details.name,
+              storeId: isPrivate ? PRIVATE_STORE : DEFAULT_STORE,
+            };
+          }
 
-        return Promise.resolve(null);
+          return null;
+        });
       },
 
       getAllCookieStores: function() {
         let defaultTabs = [];
         let privateTabs = [];
         for (let window of WindowListManager.browserWindows()) {
           let tabs = TabManager.for(extension).getTabs(window);
           for (let tab of tabs) {
--- a/toolkit/components/extensions/test/mochitest/test_ext_cookies.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_cookies.html
@@ -67,19 +67,28 @@ add_task(function* test_cookies() {
       assertExpected(expected, cookies[0]);
       return browser.cookies.getAll({secure: true});
     }).then(cookies => {
       browser.test.assertEq(cookies.length, 0, "no secure cookies found");
       return browser.cookies.getAll({storeId: STORE_ID});
     }).then(cookies => {
       browser.test.assertEq(cookies.length, 1, "one cookie found for valid storeId");
       assertExpected(expected, cookies[0]);
-      return browser.cookies.getAll({storeId: "invalid_id"});
+      return browser.cookies.getAll({storeId: "invalid_id"}).then(cookie => {
+        browser.test.fail("using invalid storeId should reject");
+      }, error => {
+        browser.test.assertEq(error.message, "Unknown storeId");
+      });
     }).then(cookies => {
-      browser.test.assertEq(cookies.length, 0, "no cookies found for invalid storeId");
+      return browser.cookies.get({url: "http://no-permission", name: "name1"}).then(cookie => {
+        browser.test.fail("using url without permission should reject");
+      }, error => {
+        browser.test.assertEq(error.message, 'No host permissions for cookies at url: "http://no-permission".');
+      });
+    }).then(cookies => {
       return browser.cookies.remove({url: TEST_URL, name: "name1"});
     }).then(details => {
       assertExpected({url: TEST_URL, name: "name1", storeId: STORE_ID}, details);
       return browser.cookies.get({url: TEST_URL, name: "name1"});
     }).then(cookie => {
       browser.test.assertEq(null, cookie, "removed cookie not found");
       return browser.cookies.getAllCookieStores();
     }).then(stores => {
@@ -183,32 +192,40 @@ add_task(function* test_cookies() {
         return browser.cookies.get({url: TEST_URL, name: "store", storeId: PRIVATE_STORE_ID});
       }).then(cookie => {
         browser.test.assertEq("private", cookie.value, "get the private cookie");
         browser.test.assertEq(PRIVATE_STORE_ID, cookie.storeId, "get the private cookie storeId");
         return browser.cookies.get({url: TEST_URL, name: "store", storeId: STORE_ID});
       }).then(cookie => {
         browser.test.assertEq("default", cookie.value, "get the default cookie");
         browser.test.assertEq(STORE_ID, cookie.storeId, "get the default cookie storeId");
+        return browser.cookies.set({url: TEST_URL, name: "store", value: "invalid", expirationDate: THE_FUTURE, storeId: "invalid"}).then(cookie => {
+          browser.test.fail("using invalid storeId should reject");
+        }, error => {
+          browser.test.assertEq(error.message, "Unknown storeId");
+        });
+      }).then(cookie => {
         return browser.cookies.remove({url: TEST_URL, name: "store", storeId: STORE_ID});
       }).then(details => {
         assertExpected({url: TEST_URL, name: "store", storeId: STORE_ID}, details);
         return browser.cookies.get({url: TEST_URL, name: "store", storeId: STORE_ID});
       }).then(cookie => {
         browser.test.assertEq(null, cookie, "deleted the default cookie");
         return browser.cookies.remove({url: TEST_URL, name: "store", storeId: PRIVATE_STORE_ID});
       }).then(details => {
         assertExpected({url: TEST_URL, name: "store", storeId: PRIVATE_STORE_ID}, details);
         return browser.cookies.get({url: TEST_URL, name: "store", storeId: PRIVATE_STORE_ID});
       }).then(cookie => {
         browser.test.assertEq(null, cookie, "deleted the private cookie");
         return browser.windows.remove(privateWindow.id);
       });
     }).then(() => {
       browser.test.notifyPass("cookies");
+    }, error => {
+      browser.test.notifyFail("cookies: " + error);
     });
   }
 
   let extension = ExtensionTestUtils.loadExtension({
     background: `(${backgroundScript})()`,
     manifest: {
       permissions: ["cookies", "*://example.org/"],
     },