Skip to content

Commit

Permalink
[ChromeDriver] W3C compliant parsing proxy setting
Browse files Browse the repository at this point in the history 
Update parsing of proxy settings in session capabilities, for W3C
compliance. Changes include:
* Handle noProxy containing a list of strings.
* Handle socksProxy and socksVersion.
* Return kInvalidArgument for errors.

Bug: chromedriver:2537
Change-Id: I545291b8249e147f75babee5dfe72137bfc4cdca
Reviewed-on: https://chromium-review.googlesource.com/1226096
Reviewed-by: Caleb Rouleau <crouleau@chromium.org>
Commit-Queue: John Chen <johnchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591496}
  • Loading branch information
@JohnChen0
JohnChen0 authored and Commit Bot committed Sep 14, 2018
1 parent 425b550 commit 1b25ac3
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 26 deletions.
54 changes: 41 additions & 13 deletions chrome/test/chromedriver/capabilities.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,10 @@ Status ParseExtensions(const base::Value& option, Capabilities* capabilities) {
Status ParseProxy(const base::Value& option, Capabilities* capabilities) {
const base::DictionaryValue* proxy_dict;
if (!option.GetAsDictionary(&proxy_dict))
return Status(kUnknownError, "must be a dictionary");
return Status(kInvalidArgument, "must be a dictionary");
std::string proxy_type;
if (!proxy_dict->GetString("proxyType", &proxy_type))
return Status(kUnknownError, "'proxyType' must be a string");
return Status(kInvalidArgument, "'proxyType' must be a string");
proxy_type = base::ToLowerASCII(proxy_type);
if (proxy_type == "direct") {
capabilities->switches.SetSwitch("no-proxy-server");
Expand All @@ -259,13 +259,15 @@ Status ParseProxy(const base::Value& option, Capabilities* capabilities) {
} else if (proxy_type == "pac") {
base::CommandLine::StringType proxy_pac_url;
if (!proxy_dict->GetString("proxyAutoconfigUrl", &proxy_pac_url))
return Status(kUnknownError, "'proxyAutoconfigUrl' must be a string");
return Status(kInvalidArgument, "'proxyAutoconfigUrl' must be a string");
capabilities->switches.SetSwitch("proxy-pac-url", proxy_pac_url);
} else if (proxy_type == "autodetect") {
capabilities->switches.SetSwitch("proxy-auto-detect");
} else if (proxy_type == "manual") {
const char* const proxy_servers_options[][2] = {
{"ftpProxy", "ftp"}, {"httpProxy", "http"}, {"sslProxy", "https"}};
{"ftpProxy", "ftp"}, {"httpProxy", "http"}, {"sslProxy", "https"},
{"socksProxy", "socks"}};
const std::string kSocksProxy = "socksProxy";
const base::Value* option_value = NULL;
std::string proxy_servers;
for (size_t i = 0; i < arraysize(proxy_servers_options); ++i) {
Expand All @@ -276,10 +278,22 @@ Status ParseProxy(const base::Value& option, Capabilities* capabilities) {
std::string value;
if (!option_value->GetAsString(&value)) {
return Status(
kUnknownError,
kInvalidArgument,
base::StringPrintf("'%s' must be a string",
proxy_servers_options[i][0]));
}
if (proxy_servers_options[i][0] == kSocksProxy) {
int socksVersion;
if (!proxy_dict->GetInteger("socksVersion", &socksVersion))
return Status(
kInvalidArgument,
"Specifying 'socksProxy' requires an integer for 'socksVersion'");
if (socksVersion < 0 || socksVersion > 255)
return Status(
kInvalidArgument,
"'socksVersion' must be between 0 and 255");
value = base::StringPrintf("socks%d://%s", socksVersion, value.c_str());
}
// Converts into Chrome proxy scheme.
// Example: "http=localhost:9000;ftp=localhost:8000".
if (!proxy_servers.empty())
Expand All @@ -290,23 +304,37 @@ Status ParseProxy(const base::Value& option, Capabilities* capabilities) {

std::string proxy_bypass_list;
if (proxy_dict->Get("noProxy", &option_value) && !option_value->is_none()) {
if (!option_value->GetAsString(&proxy_bypass_list))
return Status(kUnknownError, "'noProxy' must be a string");
// W3C requires noProxy to be a list of strings, while legacy protocol
// requires noProxy to be a string of comma-separated items.
// In practice, library implementations are not always consistent,
// so we accept both formats regardless of the W3C mode setting.
if (option_value->is_list()) {
const base::Value::ListStorage& list = option_value->GetList();
for (const base::Value& item : list) {
if (!item.is_string())
return Status(kInvalidArgument,
"'noProxy' must be a list of strings");
if (!proxy_bypass_list.empty())
proxy_bypass_list += ",";
proxy_bypass_list += item.GetString();
}
} else if (option_value->is_string()) {
proxy_bypass_list = option_value->GetString();
} else {
return Status(kInvalidArgument, "'noProxy' must be a list or a string");
}
}

if (proxy_servers.empty() && proxy_bypass_list.empty()) {
return Status(kUnknownError,
"proxyType is 'manual' but no manual "
"proxy capabilities were found");
}
// W3C doesn't require specifying any proxy servers even when proxyType is
// manual, even though such a setting would be useless.
if (!proxy_servers.empty())
capabilities->switches.SetSwitch("proxy-server", proxy_servers);
if (!proxy_bypass_list.empty()) {
capabilities->switches.SetSwitch("proxy-bypass-list",
proxy_bypass_list);
}
} else {
return Status(kUnknownError, "unrecognized proxy type:" + proxy_type);
return Status(kInvalidArgument, "unrecognized proxy type: " + proxy_type);
}
return Status(kOk);
}
Expand Down
45 changes: 32 additions & 13 deletions chrome/test/chromedriver/capabilities_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,30 +279,26 @@ TEST(ParseCapabilities, ManualProxy) {
proxy.SetString("ftpProxy", "localhost:9001");
proxy.SetString("httpProxy", "localhost:8001");
proxy.SetString("sslProxy", "localhost:10001");
proxy.SetString("noProxy", "google.com, youtube.com");
proxy.SetString("socksProxy", "localhost:12345");
proxy.SetInteger("socksVersion", 5);
std::unique_ptr<base::ListValue> bypass = std::make_unique<base::ListValue>();
bypass->AppendString("google.com");
bypass->AppendString("youtube.com");
proxy.SetList("noProxy", std::move(bypass));
base::DictionaryValue caps;
caps.SetKey("proxy", std::move(proxy));
Status status = capabilities.Parse(caps);
ASSERT_TRUE(status.IsOk());
ASSERT_EQ(2u, capabilities.switches.GetSize());
ASSERT_EQ(
"ftp=localhost:9001;http=localhost:8001;https=localhost:10001",
"ftp=localhost:9001;http=localhost:8001;https=localhost:10001;"
"socks=socks5://localhost:12345",
capabilities.switches.GetSwitchValue("proxy-server"));
ASSERT_EQ(
"google.com, youtube.com",
"google.com,youtube.com",
capabilities.switches.GetSwitchValue("proxy-bypass-list"));
}

TEST(ParseCapabilities, MissingSettingForManualProxy) {
Capabilities capabilities;
base::DictionaryValue proxy;
proxy.SetString("proxyType", "manual");
base::DictionaryValue caps;
caps.SetKey("proxy", std::move(proxy));
Status status = capabilities.Parse(caps);
ASSERT_FALSE(status.IsOk());
}

TEST(ParseCapabilities, IgnoreNullValueForManualProxy) {
Capabilities capabilities;
base::DictionaryValue proxy;
Expand All @@ -321,6 +317,29 @@ TEST(ParseCapabilities, IgnoreNullValueForManualProxy) {
capabilities.switches.GetSwitchValue("proxy-server"));
}

TEST(ParseCapabilities, MissingSocksVersion) {
Capabilities capabilities;
base::DictionaryValue proxy;
proxy.SetString("proxyType", "manual");
proxy.SetString("socksProxy", "localhost:6000");
base::DictionaryValue caps;
caps.SetKey("proxy", std::move(proxy));
Status status = capabilities.Parse(caps);
ASSERT_FALSE(status.IsOk());
}

TEST(ParseCapabilities, BadSocksVersion) {
Capabilities capabilities;
base::DictionaryValue proxy;
proxy.SetString("proxyType", "manual");
proxy.SetString("socksProxy", "localhost:6000");
proxy.SetInteger("socksVersion", 256);
base::DictionaryValue caps;
caps.SetKey("proxy", std::move(proxy));
Status status = capabilities.Parse(caps);
ASSERT_FALSE(status.IsOk());
}

TEST(ParseCapabilities, AcceptInsecureCertsDisabledByDefault) {
Capabilities capabilities;
base::DictionaryValue caps;
Expand Down

0 comments on commit 1b25ac3

Please sign in to comment.