[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}diff --git a/chrome/test/chromedriver/chrome/web_view_impl.cc b/chrome/test/chromedriver/chrome/web_view_impl.cc
index d82a06c0..44c8189 100644
--- a/chrome/test/chromedriver/chrome/web_view_impl.cc
+++ b/chrome/test/chromedriver/chrome/web_view_impl.cc
@@ -569,8 +569,8 @@
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 =
diff --git a/chrome/test/chromedriver/util.cc b/chrome/test/chromedriver/util.cc
index 21294b4..182ea30f 100644
--- a/chrome/test/chromedriver/util.cc
+++ b/chrome/test/chromedriver/util.cc
@@ -430,3 +430,110 @@
}
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;
+}
diff --git a/chrome/test/chromedriver/util.h b/chrome/test/chromedriver/util.h
index 111ced48..65477c1 100644
--- a/chrome/test/chromedriver/util.h
+++ b/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 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_
diff --git a/chrome/test/chromedriver/util_unittest.cc b/chrome/test/chromedriver/util_unittest.cc
index 95cdc9b5..b7c9dbb7b 100644
--- a/chrome/test/chromedriver/util_unittest.cc
+++ b/chrome/test/chromedriver/util_unittest.cc
@@ -48,3 +48,225 @@
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);
+}
diff --git a/chrome/test/chromedriver/window_commands.cc b/chrome/test/chromedriver/window_commands.cc
index 86f4d808..ba7b15d 100644
--- a/chrome/test/chromedriver/window_commands.cc
+++ b/chrome/test/chromedriver/window_commands.cc
@@ -47,6 +47,7 @@
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 @@
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);
}
diff --git a/docs/chromedriver_status.md b/docs/chromedriver_status.md
index 70952ae..38534a1a 100644
--- a/docs/chromedriver_status.md
+++ b/docs/chromedriver_status.md
@@ -47,7 +47,7 @@
| 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)
