[ChromeDriver] W3C compliant unknown capability handling

Per W3C spec, unrecognized capabilities should trigger invalid argument
error, instead of being silently ignored.

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

chrome/test/chromedriver/capabilities.cc

@@ -713,7 +713,8 @@ bool Capabilities::IsRemoteBrowser() const {
   return debugger_address.IsValid();
 }
 
-Status Capabilities::Parse(const base::DictionaryValue& desired_caps) {
+Status Capabilities::Parse(const base::DictionaryValue& desired_caps,
+                           bool w3c_compliant) {
   std::map<std::string, Parser> parser_map;
 
   // W3C defined capabilities.
@@ -727,18 +728,21 @@ Status Capabilities::Parse(const base::DictionaryValue& desired_caps) {
   parser_map["pageLoadStrategy"] = base::BindRepeating(&ParsePageLoadStrategy);
   parser_map["proxy"] = base::BindRepeating(&ParseProxy);
   parser_map["timeouts"] = base::BindRepeating(&ParseTimeouts);
-  // TODO(https://crbug.com/chromedriver/2596): "unexpectedAlertBehaviour" is
-  // legacy name of "unhandledPromptBehavior", remove when we stop supporting
-  // legacy mode.
-  parser_map["unexpectedAlertBehaviour"] =
-      base::BindRepeating(&ParseUnhandledPromptBehavior);
+  if (!w3c_compliant) {
+    // TODO(https://crbug.com/chromedriver/2596): "unexpectedAlertBehaviour" is
+    // legacy name of "unhandledPromptBehavior", remove when we stop supporting
+    // legacy mode.
+    parser_map["unexpectedAlertBehaviour"] =
+        base::BindRepeating(&ParseUnhandledPromptBehavior);
+  }
   parser_map["unhandledPromptBehavior"] =
       base::BindRepeating(&ParseUnhandledPromptBehavior);
 
   // ChromeDriver specific capabilities.
   // goog:chromeOptions is the current spec conformance, but chromeOptions is
-  // still supported
-  if (desired_caps.GetDictionary("goog:chromeOptions", nullptr)) {
+  // still supported in legacy mode.
+  if (w3c_compliant ||
+      desired_caps.GetDictionary("goog:chromeOptions", nullptr)) {
     parser_map["goog:chromeOptions"] = base::BindRepeating(&ParseChromeOptions);
   } else {
     parser_map["chromeOptions"] = base::BindRepeating(&ParseChromeOptions);
@@ -752,15 +756,24 @@ Status Capabilities::Parse(const base::DictionaryValue& desired_caps) {
     parser_map["networkConnectionEnabled"] =
         base::BindRepeating(&ParseBoolean, &network_emulation_enabled);
   }
+  // goog:testName is set by some tests to help debugging, and is ignored.
+  parser_map["goog:testName"] = base::BindRepeating(&IgnoreCapability);
 
-  for (auto it = parser_map.begin(); it != parser_map.end(); ++it) {
-    const base::Value* capability = NULL;
-    if (desired_caps.Get(it->first, &capability)) {
-      Status status = it->second.Run(*capability, this);
-      if (status.IsError()) {
-        return Status(kInvalidArgument, "cannot parse capability: " + it->first,
-                      status);
-      }
+  for (base::DictionaryValue::Iterator it(desired_caps); !it.IsAtEnd();
+       it.Advance()) {
+    if (parser_map.find(it.key()) == parser_map.end()) {
+      // The specified capability is unrecognized. W3C spec requires us to
+      // return an error. In legacy mode, for backward compatibility reasons,
+      // we ignore unrecognized capabilities.
+      if (w3c_compliant)
+        return Status(kInvalidArgument, "unrecognized capability: " + it.key());
+      else
+        continue;
+    }
+    Status status = parser_map[it.key()].Run(it.value(), this);
+    if (status.IsError()) {
+      return Status(kInvalidArgument, "cannot parse capability: " + it.key(),
+                    status);
     }
   }
   // Perf log must be enabled if perf log prefs are specified; otherwise, error.

chrome/test/chromedriver/capabilities.h

@@ -99,7 +99,8 @@ struct Capabilities {
 
   // Accepts all W3C defined capabilities (including those not yet supported by
   // ChromeDriver) and all ChromeDriver-specific extensions.
-  Status Parse(const base::DictionaryValue& desired_caps);
+  Status Parse(const base::DictionaryValue& desired_caps,
+               bool w3c_compliant = true);
 
   // Check if all specified capabilities are supported by ChromeDriver.
   // The long term goal is to support all standard capabilities, thus making

chrome/test/chromedriver/capabilities_unittest.cc

@@ -102,6 +102,24 @@ TEST(Switches, Unparsed) {
   ASSERT_EQ("---e=--1=1 --a --b --c=1 --d=1", switches.ToString());
 }
 
+TEST(ParseCapabilities, UnknownCapabilityLegacy) {
+  // In legacy mode, unknown capabilities are ignored.
+  Capabilities capabilities;
+  base::DictionaryValue caps;
+  caps.SetString("foo", "bar");
+  Status status = capabilities.Parse(caps, false);
+  ASSERT_TRUE(status.IsOk());
+}
+
+TEST(ParseCapabilities, UnknownCapabilityW3c) {
+  // In W3C mode, unknown capabilities results in error.
+  Capabilities capabilities;
+  base::DictionaryValue caps;
+  caps.SetString("foo", "bar");
+  Status status = capabilities.Parse(caps);
+  ASSERT_EQ(status.code(), kInvalidArgument);
+}
+
 TEST(ParseCapabilities, WithAndroidPackage) {
   Capabilities capabilities;
   base::DictionaryValue caps;

chrome/test/chromedriver/client/chromedriver.py

@@ -228,7 +228,7 @@ class ChromeDriver(object):
       options['w3c'] = send_w3c_capability
 
     params = {
-        'chromeOptions': options,
+        'goog:chromeOptions': options,
         'loggingPrefs': logging_prefs
     }
 
@@ -238,7 +238,10 @@ class ChromeDriver(object):
 
     if unexpected_alert_behaviour:
       assert type(unexpected_alert_behaviour) is str
-      params['unexpectedAlertBehaviour'] = unexpected_alert_behaviour
+      if send_w3c_request:
+        params['unhandledPromptBehavior'] = unexpected_alert_behaviour
+      else:
+        params['unexpectedAlertBehaviour'] = unexpected_alert_behaviour
 
     if network_connection:
       params['networkConnectionEnabled'] = network_connection

chrome/test/chromedriver/log_replay/client_replay.py

@@ -353,22 +353,22 @@ def _ReplaceBinary(payload, binary):
     trigger ChromeDriver's mechanism for locating the Chrome binary.
   """
   if ("desiredCapabilities" in payload
-      and "chromeOptions" in payload["desiredCapabilities"]):
+      and "goog:chromeOptions" in payload["desiredCapabilities"]):
     if binary:
-      (payload["desiredCapabilities"]["chromeOptions"]
+      (payload["desiredCapabilities"]["goog:chromeOptions"]
        ["binary"]) = binary
-    elif "binary" in payload["desiredCapabilities"]["chromeOptions"]:
-      del payload["desiredCapabilities"]["chromeOptions"]["binary"]
+    elif "binary" in payload["desiredCapabilities"]["goog:chromeOptions"]:
+      del payload["desiredCapabilities"]["goog:chromeOptions"]["binary"]
 
   elif binary:
     if "desiredCapabilities" not in payload:
       payload["desiredCapabilities"] = {
-          "chromeOptions": {
+          "goog:chromeOptions": {
               "binary": binary
           }
       }
-    elif "chromeOptions" not in payload["desiredCapabilities"]:
-      payload["desiredCapabilities"]["chromeOptions"] = {
+    elif "goog:chromeOptions" not in payload["desiredCapabilities"]:
+      payload["desiredCapabilities"]["goog:chromeOptions"] = {
           "binary": binary
       }
 
@@ -399,7 +399,7 @@ class _Payload(object):
     word InitSession:
     [1532467931.153][INFO]: [<session_id>] COMMAND InitSession {
        "desiredCapabilities": {
-          "chromeOptions": {
+          "goog:chromeOptions": {
              "args": [ "no-sandbox", "disable-gpu" ],
              "binary": "<binary_path>"
           }

chrome/test/chromedriver/log_replay/client_replay_unittest.py

@@ -215,7 +215,7 @@ class ChromeDriverClientReplayUnitTest(unittest.TestCase):
   def testReplaceBinary(self):
     payload_dict = {
         "desiredCapabilities": {
-            "chromeOptions": {
+            "goog:chromeOptions": {
                 "binary": "/path/to/logged binary/with spaces/"
             },
             "other_things": ["some", "uninteresting", "strings"]
@@ -223,7 +223,7 @@ class ChromeDriverClientReplayUnitTest(unittest.TestCase):
     }
     payload_replaced = {
         "desiredCapabilities": {
-            "chromeOptions": {
+            "goog:chromeOptions": {
                 "binary": "replacement_binary"
             },
             "other_things": ["some", "uninteresting", "strings"]
@@ -235,7 +235,7 @@ class ChromeDriverClientReplayUnitTest(unittest.TestCase):
   def testReplaceBinary_none(self):
     payload_dict = {
         "desiredCapabilities": {
-            "chromeOptions": {
+            "goog:chromeOptions": {
                 "binary": "/path/to/logged binary/with spaces/"
             },
             "other_things": ["some", "uninteresting", "strings"]
@@ -243,7 +243,7 @@ class ChromeDriverClientReplayUnitTest(unittest.TestCase):
     }
     payload_replaced = {
         "desiredCapabilities": {
-            "chromeOptions": {},
+            "goog:chromeOptions": {},
             "other_things": ["some", "uninteresting", "strings"]
         }
     }
@@ -254,7 +254,7 @@ class ChromeDriverClientReplayUnitTest(unittest.TestCase):
     payload_dict = {"desiredCapabilities": {}}
     payload_replaced = {
         "desiredCapabilities": {
-            "chromeOptions": {
+            "goog:chromeOptions": {
                 "binary": "replacement_binary"
             }
         }

chrome/test/chromedriver/session_commands.cc

@@ -231,7 +231,7 @@ Status InitSessionHelper(const InitSessionParams& bound_params,
   }
 
   Capabilities capabilities;
-  Status status = capabilities.Parse(*desired_caps);
+  Status status = capabilities.Parse(*desired_caps, session->w3c_compliant);
   if (status.IsError())
     return status;
   status = capabilities.CheckSupport();