[ChromeDriver] Add Cookie spec compliance

Make Add Cookie command compliant to W3C spec. Also added a set of
helper functions to help parsing some property types used in W3C spec.
ChromeDriver now passes all WPT WebDriver tests related to cookie.

Bug: chromedriver:2002
Change-Id: I3779a14e010bca865498a9d40710974e38ede829
Reviewed-on: https://chromium-review.googlesource.com/c/1366925
Commit-Queue: John Chen <johnchen@chromium.org>
Reviewed-by: Caleb Rouleau <crouleau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615539}

chrome/test/chromedriver/chrome/web_view_impl.cc

@@ -569,8 +569,8 @@ Status WebViewImpl::AddCookie(const std::string& name,
   params.SetString("path", path);
   params.SetBoolean("secure", secure);
   params.SetBoolean("httpOnly", httpOnly);
-  params.SetDouble("expirationDate", expiry);
-  params.SetDouble("expires", expiry);
+  if (expiry >= 0)
+    params.SetDouble("expires", expiry);
 
   std::unique_ptr<base::DictionaryValue> result;
   Status status =

chrome/test/chromedriver/util.cc

@@ -430,3 +430,110 @@ Status NotifyCommandListenersBeforeCommand(Session* session,
   }
   return Status(kOk);
 }
+
+namespace {
+
+template <typename T>
+bool GetOptionalValue(const base::DictionaryValue* dict,
+                      base::StringPiece path,
+                      T* out_value,
+                      bool* has_value,
+                      bool (base::Value::*getter)(T*) const) {
+  if (has_value != nullptr)
+    *has_value = false;
+  const base::Value* value;
+  if (!dict->Get(path, &value) || value->is_none())
+    return true;
+  if ((value->*getter)(out_value)) {
+    if (has_value != nullptr)
+      *has_value = true;
+    return true;
+  }
+  return false;
+}
+
+}  // namespace
+
+bool GetOptionalBool(const base::DictionaryValue* dict,
+                     base::StringPiece path,
+                     bool* out_value,
+                     bool* has_value) {
+  return GetOptionalValue(dict, path, out_value, has_value,
+                          &base::Value::GetAsBoolean);
+}
+
+bool GetOptionalInt(const base::DictionaryValue* dict,
+                    base::StringPiece path,
+                    int* out_value,
+                    bool* has_value) {
+  if (GetOptionalValue(dict, path, out_value, has_value,
+                       &base::Value::GetAsInteger)) {
+    return true;
+  }
+  // See if we have a double that contains an int value.
+  double d;
+  if (!dict->GetDouble(path, &d))
+    return false;
+  int i = static_cast<int>(d);
+  if (i == d) {
+    *out_value = i;
+    if (has_value != nullptr)
+      *has_value = true;
+    return true;
+  }
+  return false;
+}
+
+bool GetOptionalDouble(const base::DictionaryValue* dict,
+                       base::StringPiece path,
+                       double* out_value,
+                       bool* has_value) {
+  // base::Value::GetAsDouble already converts int to double if needed.
+  return GetOptionalValue(dict, path, out_value, has_value,
+                          &base::Value::GetAsDouble);
+}
+
+bool GetOptionalString(const base::DictionaryValue* dict,
+                       base::StringPiece path,
+                       std::string* out_value,
+                       bool* has_value) {
+  return GetOptionalValue(dict, path, out_value, has_value,
+                          &base::Value::GetAsString);
+}
+
+bool GetOptionalSafeInt(const base::DictionaryValue* dict,
+                        base::StringPiece path,
+                        int64_t* out_value,
+                        bool* has_value) {
+  // Check if we have a normal int, which is always a safe int.
+  int temp_int;
+  bool temp_has_value;
+  if (GetOptionalValue(dict, path, &temp_int, &temp_has_value,
+                       &base::Value::GetAsInteger)) {
+    if (has_value != nullptr)
+      *has_value = temp_has_value;
+    if (temp_has_value)
+      *out_value = temp_int;
+    return true;
+  }
+
+  // Check if we have a double, which may or may not contain a safe int value.
+  double temp_double;
+  if (!dict->GetDouble(path, &temp_double))
+    return false;
+
+  // Verify that the value is an integer.
+  int64_t temp_int64 = static_cast<int64_t>(temp_double);
+  if (temp_int64 != temp_double)
+    return false;
+
+  // Verify that the value is in the range for safe integer.
+  if (temp_int64 >= (1ll << 53) || temp_int64 <= -(1ll << 53))
+    return false;
+
+  // Got a good value.
+  *out_value = temp_int64;
+  if (has_value != nullptr)
+    *has_value = true;
+  return true;
+}

chrome/test/chromedriver/util.h

@@ -7,6 +7,8 @@
 
 #include <string>
 
+#include "base/values.h"
+
 namespace base {
 class FilePath;
 class ListValue;
@@ -45,4 +47,45 @@ Status UnzipSoleFile(const base::FilePath& unzip_dir,
 Status NotifyCommandListenersBeforeCommand(Session* session,
                                            const std::string& command_name);
 
+// Functions to get an optional value of the given type from a dictionary.
+// Each function has three different outcomes:
+// * Value exists and is of right type:
+//   returns true, *has_value = true, *out_value gets the actual value.
+// * Value does not exist, or exists with value of undefined:
+//   returns true, *has_value = false, *out_value is unchanged.
+// * Value exists but is of wrong type (error condition):
+//   returns false, *has_value undefined, *out_value is unchanged.
+// In addition to provide a convenient way to fetch optional values that are
+// common in W3C WebDriver spec, some of these functions also handles the
+// differences in the definition of an integer:
+// * base::Value uses a definition similar to C++, thus 2.0 is not an integer.
+//   Also, integers are limited to 32-bit.
+// * WebDriver spec (https://www.w3.org/TR/webdriver/#dfn-integer) defines
+//   integer to be a number that is unchanged under the ToInteger operation,
+//   thus 2.0 is an integer. Also, the spec sometimes use "safe integer"
+//   (https://www.w3.org/TR/webdriver/#dfn-maximum-safe-integer), whose
+//   absolute value can occupy up to 53 bits.
+bool GetOptionalBool(const base::DictionaryValue* dict,
+                     base::StringPiece path,
+                     bool* out_value,
+                     bool* has_value = nullptr);
+bool GetOptionalInt(const base::DictionaryValue* dict,
+                    base::StringPiece path,
+                    int* out_value,
+                    bool* has_value = nullptr);
+bool GetOptionalDouble(const base::DictionaryValue* dict,
+                       base::StringPiece path,
+                       double* out_value,
+                       bool* has_value = nullptr);
+bool GetOptionalString(const base::DictionaryValue* dict,
+                       base::StringPiece path,
+                       std::string* out_value,
+                       bool* has_value = nullptr);
+// Handles "safe integer" mentioned in W3C spec,
+// https://www.w3.org/TR/webdriver/#dfn-maximum-safe-integer.
+bool GetOptionalSafeInt(const base::DictionaryValue* dict,
+                        base::StringPiece path,
+                        int64_t* out_value,
+                        bool* has_value = nullptr);
+
 #endif  // CHROME_TEST_CHROMEDRIVER_UTIL_H_

chrome/test/chromedriver/util_unittest.cc

@@ -48,3 +48,225 @@ TEST(UnzipSoleFile, Archive) {
   ASSERT_TRUE(base::ReadFileToString(file, &contents));
   ASSERT_STREQ("COW\n", contents.c_str());
 }
+
+namespace {
+
+const base::StringPiece key = "key";
+const int64_t max_safe_int = (1ll << 53) - 1;
+
+void DictNoInit(base::DictionaryValue* dict) {}
+
+void DictInitNull(base::DictionaryValue* dict) {
+  dict->Set(key, std::make_unique<base::Value>());
+}
+
+class DictInitBool {
+  bool init_value;
+
+ public:
+  explicit DictInitBool(bool v) : init_value(v) {}
+  void operator()(base::DictionaryValue* dict) {
+    dict->SetBoolean(key, init_value);
+  }
+};
+
+class DictInitInt {
+  int init_value;
+
+ public:
+  explicit DictInitInt(int v) : init_value(v) {}
+  void operator()(base::DictionaryValue* dict) {
+    dict->SetInteger(key, init_value);
+  }
+};
+
+class DictInitDouble {
+  double init_value;
+
+ public:
+  explicit DictInitDouble(double v) : init_value(v) {}
+  void operator()(base::DictionaryValue* dict) {
+    dict->SetDouble(key, init_value);
+  }
+};
+
+class DictInitString {
+  std::string init_value;
+
+ public:
+  explicit DictInitString(const std::string& v) : init_value(v) {}
+  void operator()(base::DictionaryValue* dict) {
+    dict->SetString(key, init_value);
+  }
+};
+
+template <typename ResultType, typename DictInitFunc>
+void TestGetOptionalValue(bool (*func_to_test)(const base::DictionaryValue*,
+                                               base::StringPiece,
+                                               ResultType*,
+                                               bool*),
+                          DictInitFunc dict_init_func,
+                          const ResultType& init_result_value,
+                          const ResultType& expected_result_value,
+                          bool expected_return_value,
+                          bool expected_has_value) {
+  base::DictionaryValue dict;
+  dict_init_func(&dict);
+
+  ResultType result_value = init_result_value;
+  bool has_value;
+  bool return_value = func_to_test(&dict, key, &result_value, &has_value);
+  ASSERT_EQ(return_value, expected_return_value);
+  ASSERT_EQ(result_value, expected_result_value);
+  if (return_value)
+    ASSERT_EQ(has_value, expected_has_value);
+
+  result_value = init_result_value;
+  return_value = func_to_test(&dict, key, &result_value, nullptr);
+  ASSERT_EQ(return_value, expected_return_value);
+  ASSERT_EQ(result_value, expected_result_value);
+}
+
+}  // namespace
+
+TEST(GetOptionalValue, BoolNone) {
+  TestGetOptionalValue<bool>(GetOptionalBool, DictNoInit, true, true, true,
+                             false);
+}
+
+TEST(GetOptionalValue, IntNone) {
+  TestGetOptionalValue<int>(GetOptionalInt, DictNoInit, 12345, 12345, true,
+                            false);
+}
+
+TEST(GetOptionalValue, DoubleNone) {
+  TestGetOptionalValue<double>(GetOptionalDouble, DictNoInit, 123.45, 123.45,
+                               true, false);
+}
+
+TEST(GetOptionalValue, StringNone) {
+  TestGetOptionalValue<std::string>(GetOptionalString, DictNoInit, "abcde",
+                                    "abcde", true, false);
+}
+
+TEST(GetOptionalValue, SafeIntNone) {
+  TestGetOptionalValue<int64_t>(GetOptionalSafeInt, DictNoInit, 12345, 12345,
+                                true, false);
+}
+
+TEST(GetOptionalValue, BoolNull) {
+  TestGetOptionalValue<bool>(GetOptionalBool, DictInitNull, true, true, true,
+                             false);
+}
+
+TEST(GetOptionalValue, IntNull) {
+  TestGetOptionalValue<int>(GetOptionalInt, DictInitNull, 12345, 12345, true,
+                            false);
+}
+
+TEST(GetOptionalValue, DoubleNull) {
+  TestGetOptionalValue<double>(GetOptionalDouble, DictInitNull, 123.45, 123.45,
+                               true, false);
+}
+
+TEST(GetOptionalValue, StringNull) {
+  TestGetOptionalValue<std::string>(GetOptionalString, DictInitNull, "abcde",
+                                    "abcde", true, false);
+}
+
+TEST(GetOptionalValue, SafeIntNull) {
+  TestGetOptionalValue<int64_t>(GetOptionalSafeInt, DictInitNull, 12345, 12345,
+                                true, false);
+}
+
+TEST(GetOptionalValue, BoolWrongType) {
+  TestGetOptionalValue<bool>(GetOptionalBool, DictInitString("test"), true,
+                             true, false, false);
+}
+
+TEST(GetOptionalValue, IntWrongType) {
+  TestGetOptionalValue<int>(GetOptionalInt, DictInitString("test"), 12345,
+                            12345, false, false);
+}
+
+TEST(GetOptionalValue, DoubleWrongType) {
+  TestGetOptionalValue<double>(GetOptionalDouble, DictInitString("test"),
+                               123.45, 123.45, false, false);
+}
+
+TEST(GetOptionalValue, StringWrongType) {
+  TestGetOptionalValue<std::string>(GetOptionalString, DictInitBool(false),
+                                    "abcde", "abcde", false, false);
+}
+
+TEST(GetOptionalValue, SafeIntWrongType) {
+  TestGetOptionalValue<int64_t>(GetOptionalSafeInt, DictInitString("test"),
+                                12345, 12345, false, false);
+}
+
+TEST(GetOptionalValue, BoolNoConversion) {
+  TestGetOptionalValue<bool>(GetOptionalBool, DictInitBool(false), true, false,
+                             true, true);
+}
+
+TEST(GetOptionalValue, IntNoConversion) {
+  TestGetOptionalValue<int>(GetOptionalInt, DictInitInt(54321), 12345, 54321,
+                            true, true);
+}
+
+TEST(GetOptionalValue, DoubleNoConversion) {
+  TestGetOptionalValue<double>(GetOptionalDouble, DictInitDouble(54.321),
+                               123.45, 54.321, true, true);
+}
+
+TEST(GetOptionalValue, StringNoConversion) {
+  TestGetOptionalValue<std::string>(GetOptionalString, DictInitString("xyz"),
+                                    "abcde", "xyz", true, true);
+}
+
+TEST(GetOptionalValue, SafeIntNoConversion) {
+  TestGetOptionalValue<int64_t>(GetOptionalSafeInt, DictInitInt(54321), 12345,
+                                54321, true, true);
+}
+
+TEST(GetOptionalValue, DoubleToInt) {
+  TestGetOptionalValue<int>(GetOptionalInt, DictInitDouble(54321.0), 12345,
+                            54321, true, true);
+}
+
+TEST(GetOptionalValue, DoubleToSafeInt) {
+  TestGetOptionalValue<int64_t>(GetOptionalSafeInt, DictInitDouble(54321.0),
+                                12345, 54321, true, true);
+}
+
+TEST(GetOptionalValue, DoubleToIntError) {
+  TestGetOptionalValue<int>(GetOptionalInt, DictInitDouble(5432.1), 12345,
+                            12345, false, false);
+}
+
+TEST(GetOptionalValue, DoubleToSafeIntError) {
+  TestGetOptionalValue<int64_t>(GetOptionalSafeInt, DictInitDouble(5432.1),
+                                12345, 12345, false, false);
+}
+
+TEST(GetOptionalValue, IntToDouble) {
+  TestGetOptionalValue<double>(GetOptionalDouble, DictInitInt(54), 123.45, 54.0,
+                               true, true);
+}
+
+TEST(GetOptionalValue, SafeIntMax) {
+  TestGetOptionalValue<int64_t>(GetOptionalSafeInt,
+                                DictInitDouble(max_safe_int), 12345,
+                                max_safe_int, true, true);
+}
+
+TEST(GetOptionalValue, SafeIntMaxToIntError) {
+  TestGetOptionalValue<int>(GetOptionalInt, DictInitDouble(max_safe_int), 12345,
+                            12345, false, false);
+}
+
+TEST(GetOptionalValue, SafeIntTooLarge) {
+  TestGetOptionalValue<int64_t>(GetOptionalSafeInt,
+                                DictInitDouble(max_safe_int + 1), 12345, 12345,
+                                false, false);
+}

chrome/test/chromedriver/window_commands.cc

@@ -47,6 +47,7 @@ namespace {
 static const char kUnreachableWebDataURL[] = "chrome-error://chromewebdata/";
 const char kDeprecatedUnreachableWebDataURL[] = "data:text/html,chromewebdata";
 
+// TODO(johnchen@chromium.org): Remove when we stop supporting legacy protocol.
 // Defaults to 20 years into the future when adding a cookie.
 const double kDefaultCookieExpiryTime = 20*365*24*60*60;
 
@@ -1560,17 +1561,37 @@ Status ExecuteAddCookie(Session* session,
   if (status.IsError())
     return status;
   std::string domain;
-  cookie->GetString("domain", &domain);
+  if (!GetOptionalString(cookie, "domain", &domain))
+    return Status(kInvalidArgument, "invalid 'domain'");
   std::string path("/");
-  cookie->GetString("path", &path);
+  if (!GetOptionalString(cookie, "path", &path))
+    return Status(kInvalidArgument, "invalid 'path'");
   bool secure = false;
-  cookie->GetBoolean("secure", &secure);
+  if (!GetOptionalBool(cookie, "secure", &secure))
+    return Status(kInvalidArgument, "invalid 'secure'");
   bool httpOnly = false;
-  cookie->GetBoolean("httpOnly", &httpOnly);
+  if (!GetOptionalBool(cookie, "httpOnly", &httpOnly))
+    return Status(kInvalidArgument, "invalid 'httpOnly'");
   double expiry;
-  if (!cookie->GetDouble("expiry", &expiry))
-    expiry = (base::Time::Now() - base::Time::UnixEpoch()).InSeconds() +
-              kDefaultCookieExpiryTime;
+  bool has_value;
+  if (session->w3c_compliant) {
+    // W3C spec says expiry is a safe integer.
+    int64_t expiry_int64;
+    if (!GetOptionalSafeInt(cookie, "expiry", &expiry_int64, &has_value) ||
+        (has_value && expiry_int64 < 0))
+      return Status(kInvalidArgument, "invalid 'expiry'");
+    // Use negative value to indicate expiry not specified.
+    expiry = has_value ? static_cast<double>(expiry_int64) : -1.0;
+  } else {
+    // JSON wire protocol didn't specify the type of expiry, but ChromeDriver
+    // has always accepted double, so we keep that in legacy mode.
+    if (!GetOptionalDouble(cookie, "expiry", &expiry, &has_value) ||
+        (has_value && expiry < 0))
+      return Status(kInvalidArgument, "invalid 'expiry'");
+    if (!has_value)
+      expiry = (base::Time::Now() - base::Time::UnixEpoch()).InSeconds() +
+               kDefaultCookieExpiryTime;
+  }
   return web_view->AddCookie(name, url, cookie_value, domain, path,
       secure, httpOnly, expiry);
 }

docs/chromedriver_status.md

@@ -47,7 +47,7 @@ Below is a list of all WebDriver commands and their current support in ChromeDri
 | POST   | /session/{session id}/execute/async                            | Execute Async Script       | Partially Complete | [2398](https://bugs.chromium.org/p/chromedriver/issues/detail?id=2398) [2556](https://bugs.chromium.org/p/chromedriver/issues/detail?id=2556)
 | GET    | /session/{session id}/cookie                                   | Get All Cookies            | Complete           |
 | GET    | /session/{session id}/cookie/{name}                            | Get Named Cookie           | Complete           |
-| POST   | /session/{session id}/cookie                                   | Add Cookie                 | Partially Complete | [2002](https://bugs.chromium.org/p/chromedriver/issues/detail?id=2002)
+| POST   | /session/{session id}/cookie                                   | Add Cookie                 | Complete           |
 | DELETE | /session/{session id}/cookie/{name}                            | Delete Cookie              | Complete           |
 | DELETE | /session/{session id)/cookie                                   | Delete All Cookies         | Complete           |
 | POST   | /session/{session id}/actions                                  | Perform Actions            | Incomplete         | [1897](https://bugs.chromium.org/p/chromedriver/issues/detail?id=1897)