Bug 1407675 - Fix cookie creation via Marionette using IP address r?ato, whimboo draft
authorPeter Major <majorpetya@gmail.com>
Thu, 12 Oct 2017 16:06:26 +0100
changeset 679320 35c23416bbfb929b20561a1a330497a228df2974
parent 679173 bc7a5be76b723cf6aac1a919156e74997c5f4902
child 735576 522c668b009b803da9e63b01f9f6cf806c8bc714
push id84194
push userbmo:majorpetya@gmail.com
push dateThu, 12 Oct 2017 15:09:28 +0000
reviewersato, whimboo
bugs1407675
milestone58.0a1
Bug 1407675 - Fix cookie creation via Marionette using IP address r?ato, whimboo The issue here was that the cookie domain was always prepended with '.' character. To resolve this edge-case Marionette now first checks whether the cookie domain is in fact an IP address. MozReview-Commit-ID: 4xBd4rscXxx
testing/marionette/cookie.js
testing/marionette/driver.js
testing/marionette/test_cookie.js
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py
testing/web-platform/tests/webdriver/tests/cookies/get_named_cookie.py
--- a/testing/marionette/cookie.js
+++ b/testing/marionette/cookie.js
@@ -1,15 +1,15 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
-const {interfaces: Ci, utils: Cu} = Components;
+const {interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 Cu.import("resource://gre/modules/Services.jsm");
 
 Cu.import("chrome://marionette/content/assert.js");
 const {
   InvalidCookieDomainError,
   pprint,
 } = Cu.import("chrome://marionette/content/error.js", {});
@@ -55,22 +55,17 @@ 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.domain != "undefined") {
-    let domain = assert.string(json.domain, "Cookie domain must be string");
-    if (domain.substring(0, 1) !== ".") {
-      // make sure that this is stored as a domain cookie
-      domain = "." + domain;
-    }
-    newCookie.domain = domain;
+    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") {
@@ -99,35 +94,60 @@ cookie.fromJSON = function(json) {
  *     not present and of the correct type.
  * @throws {InvalidCookieDomainError}
  *     If <var>restrictToHost</var> is set and <var>newCookie</var>'s
  *     domain does not match.
  */
 cookie.add = function(newCookie, {restrictToHost = null} = {}) {
   assert.string(newCookie.name, "Cookie name must be string");
   assert.string(newCookie.value, "Cookie value must be string");
+
+  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.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;
   }
 
+  let isIpAddress = false;
+  try {
+    Services.eTLD.getPublicSuffixFromHost(newCookie.domain);
+  } catch (e) {
+    switch (e.result) {
+      case Cr.NS_ERROR_HOST_IS_IP_ADDRESS:
+        isIpAddress = true;
+        break;
+      default:
+        throw new InvalidCookieDomainError(newCookie.domain);
+    }
+  }
+
+  if (!hostOnly && !isIpAddress) {
+    // only store this as a domain cookie if the domain was specified in the
+    // request and it wasn't an IP address.
+    newCookie.domain = "." + newCookie.domain;
+  }
+
   if (restrictToHost) {
     if (!restrictToHost.endsWith(newCookie.domain) &&
-        ("." + restrictToHost) !== newCookie.domain) {
-      throw new InvalidCookieDomainError(
-          `Cookies may only be set ` +
+        ("." + restrictToHost) !== newCookie.domain &&
+        restrictToHost !== newCookie.domain) {
+      throw new InvalidCookieDomainError(`Cookies may only be set ` +
           `for the current domain (${restrictToHost})`);
     }
   }
 
   // remove port from domain, if present.
   // unfortunately this catches IPv6 addresses by mistake
   // TODO: Bug 814416
   newCookie.domain = newCookie.domain.replace(IPV4_PORT_EXPR, "");
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -2626,19 +2626,16 @@ GeckoDriver.prototype.addCookie = functi
   let {protocol, hostname} = this.currentURL;
 
   const networkSchemes = ["ftp:", "http:", "https:"];
   if (!networkSchemes.includes(protocol)) {
     throw new InvalidCookieDomainError("Document is cookie-averse");
   }
 
   let newCookie = cookie.fromJSON(cmd.parameters.cookie);
-  if (typeof newCookie.domain == "undefined") {
-    newCookie.domain = hostname;
-  }
 
   cookie.add(newCookie, {restrictToHost: hostname});
 };
 
 /**
  * Get all the cookies for the current domain.
  *
  * This is the equivalent of calling <code>document.cookie</code> and
--- a/testing/marionette/test_cookie.js
+++ b/testing/marionette/test_cookie.js
@@ -32,17 +32,18 @@ cookie.manager = {
           candidate.path === path) {
         return this.cookies.splice(i, 1);
       }
     }
     return false;
   },
 
   getCookiesFromHost(host, originAttributes = {}) {
-    let hostCookies = this.cookies.filter(cookie => cookie.host === host);
+    let hostCookies = this.cookies.filter(cookie => cookie.host === host ||
+       cookie.host === "." + host);
     let nextIndex = 0;
 
     return {
       hasMoreElements () {
         return nextIndex < hostCookies.length;
       },
 
       getNext () {
@@ -78,17 +79,17 @@ add_test(function test_fromJSON() {
     Assert.throws(() => cookie.fromJSON(test), /Cookie domain must be string/);
   }
   let test = {
     name: "foo",
     value: "bar",
     domain: "domain"
   };
   let parsedCookie = cookie.fromJSON(test);
-  equal(parsedCookie.domain, ".domain");
+  equal(parsedCookie.domain, "domain");
 
   // path
   for (let invalidType of [42, true, [], {}, null]) {
     let test = {
       name: "foo",
       value: "bar",
       path: invalidType,
     };
@@ -184,17 +185,17 @@ add_test(function test_add() {
   cookie.add({
     name: "name",
     value: "value",
     domain: "domain",
   });
   equal(1, cookie.manager.cookies.length);
   equal("name", cookie.manager.cookies[0].name);
   equal("value", cookie.manager.cookies[0].value);
-  equal("domain", cookie.manager.cookies[0].host);
+  equal(".domain", cookie.manager.cookies[0].host);
   equal("/", cookie.manager.cookies[0].path);
   ok(cookie.manager.cookies[0].expiry > new Date(Date.now()).getTime() / 1000);
 
   cookie.add({
     name: "name2",
     value: "value2",
     domain: "domain2",
   });
@@ -205,17 +206,17 @@ add_test(function test_add() {
     cookie.add(biscuit, {restrictToHost: "other domain"});
   }, /Cookies may only be set for the current domain/);
 
   cookie.add({
     name: "name4",
     value: "value4",
     domain: "my.domain:1234",
   });
-  equal("my.domain", cookie.manager.cookies[2].host);
+  equal(".my.domain", cookie.manager.cookies[2].host);
 
   cookie.add({
     name: "name5",
     value: "value5",
     domain: "domain5",
     path: "/foo/bar",
   });
   equal("/foo/bar", cookie.manager.cookies[3].path);
@@ -242,18 +243,16 @@ add_test(function test_remove() {
   equal(undefined, cookie.manager.cookies[0]);
 
   run_next_test();
 });
 
 add_test(function test_iter() {
   cookie.manager.cookies = [];
 
-  cookie.add({name: "0", value: "", domain: "example.com"});
-  cookie.add({name: "1", value: "", domain: "foo.example.com"});
-  cookie.add({name: "2", value: "", domain: "bar.example.com"});
-
+  cookie.add({name: "0", value: "", domain: "foo.example.com"});
+  cookie.add({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(".foo.example.com", fooCookies[0].domain);
 
   run_next_test();
 });
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -409311,16 +409311,22 @@
     ]
    ],
    "webdriver/tests/contexts/resizing_and_positioning.py": [
     [
      "/webdriver/tests/contexts/resizing_and_positioning.py",
      {}
     ]
    ],
+   "webdriver/tests/cookies/add_cookie.py": [
+    [
+     "/webdriver/tests/cookies/add_cookie.py",
+     {}
+    ]
+   ],
    "webdriver/tests/cookies/delete_cookie.py": [
     [
      "/webdriver/tests/cookies/delete_cookie.py",
      {}
     ]
    ],
    "webdriver/tests/cookies/get_named_cookie.py": [
     [
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py
@@ -0,0 +1,76 @@
+from tests.support.fixtures import clear_all_cookies
+from tests.support.fixtures import server_config
+
+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",
+            "domain": "web-platform.test",
+            "path": "/",
+            "httpOnly": False,
+            "secure": False
+        }
+    }
+    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 "domain" in cookie
+    assert isinstance(cookie["domain"], basestring)
+
+    assert cookie["name"] == "hello"
+    assert cookie["value"] == "world"
+    assert cookie["domain"] == ".web-platform.test"
+
+def test_add_cookie_for_ip(session, url, server_config):
+    session.url = "http://127.0.0.1:%s/404" % (server_config["ports"]["http"][0])
+    clear_all_cookies(session)
+    create_cookie_request = {
+        "cookie": {
+            "name": "hello",
+            "value": "world",
+            "domain": "127.0.0.1",
+            "path": "/",
+            "httpOnly": False,
+            "secure": False
+        }
+    }
+    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 "domain" in cookie
+    assert isinstance(cookie["domain"], basestring)
+
+    assert cookie["name"] == "hello"
+    assert cookie["value"] == "world"
+    assert cookie["domain"] == "127.0.0.1"
--- a/testing/web-platform/tests/webdriver/tests/cookies/get_named_cookie.py
+++ b/testing/web-platform/tests/webdriver/tests/cookies/get_named_cookie.py
@@ -60,45 +60,8 @@ def test_duplicated_cookie(session, url)
     assert "name" in cookie
     assert isinstance(cookie["name"], basestring)
     assert "value" in cookie
     assert isinstance(cookie["value"], basestring)
 
     assert cookie["name"] == "hello"
     assert cookie["value"] == "newworld"
 
-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",
-            "domain": "web-platform.test",
-            "path": "/",
-            "httpOnly": False,
-            "secure": False
-        }
-    }
-    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 "domain" in cookie
-    assert isinstance(cookie["domain"], basestring)
-
-    assert cookie["name"] == "hello"
-    assert cookie["value"] == "world"
-    assert cookie["domain"] == ".web-platform.test"
-