Bug 1408962 - Make add cookie command webdriver spec comformant r=ato,whimboo
authorPeter Major <majorpetya@gmail.com>
Sat, 28 Oct 2017 11:37:40 +0100
changeset 444031 229d540c474e8022ec98823623d390b8ed27ed69
parent 444030 0c8659b2921a5cd3c423301b246bafdcbf4a202f
child 444032 9d06816dccb88d20fa114de445c02576980b4536
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato, whimboo
bugs1408962
milestone58.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 1408962 - Make add cookie command webdriver spec comformant r=ato,whimboo * Changed positiveInteger to validate upper bounds * Removed processing for the no longer existent 'session' field * Added default values for absent fields MozReview-Commit-ID: E4x6TyMwZQs
testing/marionette/assert.js
testing/marionette/cookie.js
testing/marionette/test_cookie.js
testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py
--- a/testing/marionette/assert.js
+++ b/testing/marionette/assert.js
@@ -228,17 +228,17 @@ assert.callable = function(obj, msg = ""
  * @return {number}
  *     <var>obj</var> is returned unaltered.
  *
  * @throws {InvalidArgumentError}
  *     If <var>obj</var> is not an integer.
  */
 assert.integer = function(obj, msg = "") {
   msg = msg || pprint`Expected ${obj} to be an integer`;
-  return assert.that(Number.isInteger, msg)(obj);
+  return assert.that(Number.isSafeInteger, msg)(obj);
 };
 
 /**
  * Asserts that <var>obj</var> is a positive integer.
  *
  * @param {?} obj
  *     Value to test.
  * @param {string=} msg
--- a/testing/marionette/cookie.js
+++ b/testing/marionette/cookie.js
@@ -4,17 +4,20 @@
 
 "use strict";
 
 const {interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 Cu.import("resource://gre/modules/Services.jsm");
 
 Cu.import("chrome://marionette/content/assert.js");
-const {InvalidCookieDomainError} = Cu.import("chrome://marionette/content/error.js", {});
+const {
+  InvalidCookieDomainError,
+  UnableToSetCookieError,
+} = Cu.import("chrome://marionette/content/error.js", {});
 const {pprint} = Cu.import("chrome://marionette/content/format.js", {});
 
 this.EXPORTED_SYMBOLS = ["cookie"];
 
 const IPV4_PORT_EXPR = /:\d+$/;
 
 /** @namespace */
 this.cookie = {
@@ -29,54 +32,50 @@ this.cookie = {
 
 /**
  * Unmarshal a JSON Object to a cookie representation.
  *
  * Effectively this will run validation checks on |json|, which will
  * produce the errors expected by WebDriver if the input is not valid.
  *
  * @param {Object.<string, (number|boolean|string)>} json
- *     Cookie to be deserialised.  <var>name</var> and <var>value</var>
- *     are required fields which must be strings.  The <var>path</var> and
+ *     Cookie to be deserialised. <var>name</var> and <var>value</var>
+ *     are required fields which must be strings. The <var>path</var> and
  *     <var>domain</var> fields are optional, but must be a string if
  *     provided.
- *     The <var>secure</var>, <var>httpOnly</var>, and
- *     <var>session</var>fields are similarly optional, but must be
- *     booleans.  Likewise, the <var>expiry</var> field is optional but
- *     must be unsigned integer.
+ *     The <var>secure</var>, and <var>httpOnly</var> are similarly
+ *     optional, but must be booleans. Likewise, the <var>expiry</var> field
+ *     is optional but must be unsigned integer.
  *
  * @return {Cookie}
  *     Valid cookie object.
  *
  * @throws {InvalidArgumentError}
  *     If any of the properties are invalid.
  */
 cookie.fromJSON = function(json) {
   let newCookie = {};
 
   assert.object(json, pprint`Expected cookie object, got ${json}`);
 
   newCookie.name = assert.string(json.name, "Cookie name must be string");
   newCookie.value = assert.string(json.value, "Cookie value must be string");
 
+  if (typeof json.path != "undefined") {
+    newCookie.path = assert.string(json.path, "Cookie path must be string");
+  }
   if (typeof json.domain != "undefined") {
     newCookie.domain = assert.string(json.domain, "Cookie domain must be string");
   }
-  if (typeof json.path != "undefined") {
-    newCookie.path = assert.string(json.path, "Cookie path must be string");
-  }
   if (typeof json.secure != "undefined") {
     newCookie.secure = assert.boolean(json.secure, "Cookie secure flag must be boolean");
   }
   if (typeof json.httpOnly != "undefined") {
     newCookie.httpOnly = assert.boolean(json.httpOnly, "Cookie httpOnly flag must be boolean");
   }
-  if (typeof json.session != "undefined") {
-    newCookie.session = assert.boolean(json.session, "Cookie session flag must be boolean");
-  }
   if (typeof json.expiry != "undefined") {
     newCookie.expiry = assert.positiveInteger(json.expiry, "Cookie expiry must be a positive integer");
   }
 
   return newCookie;
 };
 
 /**
@@ -88,38 +87,46 @@ cookie.fromJSON = function(json) {
  *     Perform test that <var>newCookie</var>'s domain matches this.
  *
  * @throws {TypeError}
  *     If <var>name</var>, <var>value</var>, or <var>domain</var> are
  *     not present and of the correct type.
  * @throws {InvalidCookieDomainError}
  *     If <var>restrictToHost</var> is set and <var>newCookie</var>'s
  *     domain does not match.
+ * @throws {UnableToSetCookieError}
+ *     If an error occurred while trying to save the cookie.
  */
 cookie.add = function(newCookie, {restrictToHost = null} = {}) {
   assert.string(newCookie.name, "Cookie name must be string");
   assert.string(newCookie.value, "Cookie value must be string");
 
+  if (typeof newCookie.path == "undefined") {
+    newCookie.path = "/";
+  }
+
   let hostOnly = false;
   if (typeof newCookie.domain == "undefined") {
     hostOnly = true;
     newCookie.domain = restrictToHost;
   }
   assert.string(newCookie.domain, "Cookie domain must be string");
 
-  if (typeof newCookie.path == "undefined") {
-    newCookie.path = "/";
+  if (typeof newCookie.secure == "undefined") {
+    newCookie.secure = false;
   }
-
+  if (typeof newCookie.httpOnly == "undefined") {
+    newCookie.httpOnly = false;
+  }
   if (typeof newCookie.expiry == "undefined") {
-    // twenty years into the future
-    let date = new Date();
-    let now = new Date(Date.now());
-    date.setYear(now.getFullYear() + 20);
-    newCookie.expiry = date.getTime() / 1000;
+    // The XPCOM interface requires the expiry field even for session cookies.
+    newCookie.expiry = Number.MAX_SAFE_INTEGER;
+    newCookie.session = true;
+  } else {
+    newCookie.session = false;
   }
 
   let isIpAddress = false;
   try {
     Services.eTLD.getPublicSuffixFromHost(newCookie.domain);
   } catch (e) {
     switch (e.result) {
       case Cr.NS_ERROR_HOST_IS_IP_ADDRESS:
@@ -145,26 +152,30 @@ cookie.add = function(newCookie, {restri
     }
   }
 
   // remove port from domain, if present.
   // unfortunately this catches IPv6 addresses by mistake
   // TODO: Bug 814416
   newCookie.domain = newCookie.domain.replace(IPV4_PORT_EXPR, "");
 
-  cookie.manager.add(
-      newCookie.domain,
-      newCookie.path,
-      newCookie.name,
-      newCookie.value,
-      newCookie.secure,
-      newCookie.httpOnly,
-      newCookie.session,
-      newCookie.expiry,
-      {} /* origin attributes */);
+  try {
+    cookie.manager.add(
+        newCookie.domain,
+        newCookie.path,
+        newCookie.name,
+        newCookie.value,
+        newCookie.secure,
+        newCookie.httpOnly,
+        newCookie.session,
+        newCookie.expiry,
+        {} /* origin attributes */);
+  } catch (e) {
+    throw new UnableToSetCookieError(e);
+  }
 };
 
 /**
  * Remove cookie from the cookie store.
  *
  * @param {Cookie} toDelete
  *     Cookie to remove.
  */
--- a/testing/marionette/test_cookie.js
+++ b/testing/marionette/test_cookie.js
@@ -5,16 +5,19 @@
 const {utils: Cu} = Components;
 
 Cu.import("chrome://marionette/content/cookie.js");
 
 cookie.manager = {
   cookies: [],
 
   add: function (domain, path, name, value, secure, httpOnly, session, expiry, originAttributes) {
+    if (name === "fail") {
+      throw new Error("An error occurred while adding cookie");
+    }
     let newCookie = {
       host: domain,
       path: path,
       name: name,
       value: value,
       isSecure: secure,
       isHttpOnly: httpOnly,
       isSession: session,
@@ -111,28 +114,18 @@ add_test(function test_fromJSON() {
     let test = {
       name: "foo",
       value: "bar",
       httpOnly: invalidType,
     };
     Assert.throws(() => cookie.fromJSON(test), /Cookie httpOnly flag must be boolean/);
   }
 
-  // session
-  for (let invalidType of ["foo", 42, [], {}, null]) {
-    let test = {
-      name: "foo",
-      value: "bar",
-      session: invalidType,
-    };
-    Assert.throws(() => cookie.fromJSON(test), /Cookie session flag must be boolean/);
-  }
-
   // expiry
-  for (let invalidType of ["foo", true, [], {}, null]) {
+  for (let invalidType of [-1, Number.MAX_SAFE_INTEGER + 1, "foo", true, [], {}, null]) {
     let test = {
       name: "foo",
       value: "bar",
       expiry: invalidType,
     };
     Assert.throws(() => cookie.fromJSON(test), /Cookie expiry must be a positive integer/);
   }
 
@@ -147,26 +140,24 @@ add_test(function test_fromJSON() {
   // everything
   let full = cookie.fromJSON({
     name: "name",
     value: "value",
     domain: ".domain",
     path: "path",
     secure: true,
     httpOnly: true,
-    session: true,
     expiry: 42,
   });
   equal("name", full.name);
   equal("value", full.value);
   equal(".domain", full.domain);
   equal("path", full.path);
   equal(true, full.secure);
   equal(true, full.httpOnly);
-  equal(true, full.session);
   equal(42, full.expiry);
 
   run_next_test();
 });
 
 add_test(function test_add() {
   cookie.manager.cookies = [];
 
@@ -216,16 +207,20 @@ add_test(function test_add() {
   cookie.add({
     name: "name5",
     value: "value5",
     domain: "domain5",
     path: "/foo/bar",
   });
   equal("/foo/bar", cookie.manager.cookies[3].path);
 
+  Assert.throws(() => {
+    cookie.add({name: "fail", value: "value6", domain: "domain6"})
+  }, /UnableToSetCookieError/);
+
   run_next_test();
 });
 
 add_test(function test_remove() {
   cookie.manager.cookies = [];
 
   let crumble = {
     name: "test_remove",
@@ -242,41 +237,38 @@ add_test(function test_remove() {
   equal(0, cookie.manager.cookies.length);
   equal(undefined, cookie.manager.cookies[0]);
 
   run_next_test();
 });
 
 add_test(function test_iter() {
   cookie.manager.cookies = [];
+  let tomorrow = new Date();
+  tomorrow.setHours(tomorrow.getHours() + 24);
 
   cookie.add({
-    session: false,
+    expiry: tomorrow,
     name: "0",
     value: "",
     domain: "foo.example.com",
   });
   cookie.add({
-    session: false,
+    expiry: tomorrow,
     name: "1",
     value: "",
     domain: "bar.example.com",
   });
 
   let fooCookies = [...cookie.iter("foo.example.com")];
   equal(1, fooCookies.length);
   equal(".foo.example.com", fooCookies[0].domain);
   equal(true, fooCookies[0].hasOwnProperty("expiry"));
 
-  // here we're explicitly setting session to true as a workaround until
-  // bug 1408962 has been fixed. when that bug has been fixed the cookie
-  // will be created as session cookie simply by leaving out the 'expiry'
-  // property.
   cookie.add({
-    session: true,
     name: "aSessionCookie",
     value: "",
     domain: "session.com",
   });
 
   let sessionCookies = [...cookie.iter("session.com")];
   equal(1, sessionCookies.length);
   equal("aSessionCookie", sessionCookies[0].name);
--- a/testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py
+++ b/testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py
@@ -1,10 +1,11 @@
 from tests.support.fixtures import clear_all_cookies
 from tests.support.fixtures import server_config
+from datetime import datetime, timedelta
 
 def test_add_domain_cookie(session, url):
     session.url = url("/common/blank.html")
     clear_all_cookies(session)
     create_cookie_request = {
         "cookie": {
             "name": "hello",
             "value": "world",
@@ -69,8 +70,75 @@ def test_add_cookie_for_ip(session, url,
     assert "value" in cookie
     assert isinstance(cookie["value"], basestring)
     assert "domain" in cookie
     assert isinstance(cookie["domain"], basestring)
 
     assert cookie["name"] == "hello"
     assert cookie["value"] == "world"
     assert cookie["domain"] == "127.0.0.1"
+
+def test_add_non_session_cookie(session, url):
+    session.url = url("/common/blank.html")
+    clear_all_cookies(session)
+    a_year_from_now = int((datetime.utcnow() + timedelta(days=365)).strftime("%s"))
+    create_cookie_request = {
+        "cookie": {
+            "name": "hello",
+            "value": "world",
+            "expiry": a_year_from_now
+        }
+    }
+    result = session.transport.send("POST", "session/%s/cookie" % session.session_id, create_cookie_request)
+    assert result.status == 200
+    assert "value" in result.body
+    assert isinstance(result.body["value"], dict)
+
+    result = session.transport.send("GET", "session/%s/cookie" % session.session_id)
+    assert result.status == 200
+    assert "value" in result.body
+    assert isinstance(result.body["value"], list)
+    assert len(result.body["value"]) == 1
+    assert isinstance(result.body["value"][0], dict)
+
+    cookie = result.body["value"][0]
+    assert "name" in cookie
+    assert isinstance(cookie["name"], basestring)
+    assert "value" in cookie
+    assert isinstance(cookie["value"], basestring)
+    assert "expiry" in cookie
+    assert isinstance(cookie["expiry"], int)
+
+    assert cookie["name"] == "hello"
+    assert cookie["value"] == "world"
+    assert cookie["expiry"] == a_year_from_now
+
+def test_add_session_cookie(session, url):
+    session.url = url("/common/blank.html")
+    clear_all_cookies(session)
+    create_cookie_request = {
+        "cookie": {
+            "name": "hello",
+            "value": "world"
+        }
+    }
+    result = session.transport.send("POST", "session/%s/cookie" % session.session_id, create_cookie_request)
+    assert result.status == 200
+    assert "value" in result.body
+    assert isinstance(result.body["value"], dict)
+
+    result = session.transport.send("GET", "session/%s/cookie" % session.session_id)
+    assert result.status == 200
+    assert "value" in result.body
+    assert isinstance(result.body["value"], list)
+    assert len(result.body["value"]) == 1
+    assert isinstance(result.body["value"][0], dict)
+
+    cookie = result.body["value"][0]
+    assert "name" in cookie
+    assert isinstance(cookie["name"], basestring)
+    assert "value" in cookie
+    assert isinstance(cookie["value"], basestring)
+    assert "expiry" in cookie
+    assert cookie.get("expiry") is None
+
+    assert cookie["name"] == "hello"
+    assert cookie["value"] == "world"