Translate ONC ProxySettings <-> Shill ProxyConfig

Translate ONC ProxySettings <-> Shill ProxyConfig

This CL adds translation of the ProxySettings property in an ONC dictionary to the ProxyConfig property of a Shill dictionary (which previously was not being handled).

BUG=470445
TBR=abarth@chromium.org for DEPS change (+url)

Committed: https://crrev.com/8336888d95e672e8642889cade066aed7bb1f595
Cr-Commit-Position: refs/heads/master@{#338352}

[stevenjb, 2015-07-07 01:42:02]

stevenjb@chromium.org changed reviewers:
+ jochen@chromium.org, pneubeck@chromium.org

[stevenjb, 2015-07-07 01:42:02]

jochen@chromium.org for components/proxy_config.gypi and
	chrome/chrome_tests_unit.gypi

[stevenjb, 2015-07-07 01:42:09]

The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-07 01:42:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228543002/1

[commit-bot: I haz the power, 2015-07-07 01:50:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-07 01:50:49]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pneubeck (no reviews), 2015-07-07 10:18:48]

the referenced bug number is wrong.

overall lgtm, the most fragile part is the added translation from the
ProxyConfig (stored in Shill) to ONC.
I tried to read it carefully, but I might have missed some subtleties.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:831: << "PAC field is invalid for this
ProxySettings.Type";
i think this error message is wrong.
should instead be:
  Invalid URL in ProxySettings.PAC

(although this part should better be verified in the validator)

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:862: // Create a ProxyConfigDictionary from
the DictionaryValye.
Valye -> Value

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:896: std::vector<std::string> servers;
instead of the following parsing you could instead use
  net::ProxyConfig::ProxyRules::ParseFromString

But as I understand it, you don't have to parse the general grammar described
there
(https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/proxy_co...)
but instead you only have to reverse the above function
ConvertOncProxySettingsToProxyConfig.

You should emphasize that in the header, that this function is not expecting
arbitrary ProxyConfig dictionaries but only the ones created by
ConvertOncProxySettingsToProxyConfig.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:898: if (!servers.empty()) {
according to the ONC spec the Manual subdictionary is required if Type is
Manual.

please check whether it would be more correct to at least insert the empty
Manual dict.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:900: for (std::string server : servers) {
const&

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:901: size_t index = server.find_first_of('=');
optional nit: 'const size_t'

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:905: std::string default_prefix = scheme;
the default prefix for everything but Socks is  net::ProxyServer::SCHEME_HTTP
and not for example FTP for FTP.

IIRC, the meaning of this is:
FTP, HTTP and HTTPS usually are transported over an http connection to the
proxy.
Whereas for socks, the socks protocol itself is used.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:916: if (!onc_scheme.empty()) {
onc_scheme.empty() seems to be a severe error that we at least should log.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:943: for (std::string entry : bypass_list)
const&

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
File chromeos/network/onc/onc_utils_unittest.cc (right):

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils_unittest.cc:194:
test_utils::Equals(onc_proxy_settings, actual_proxy_settings.get()));
you could also just do that in the the previous test, directly after after the
other EXPECT_TRUE(...);

https://codereview.chromium.org/1228543002/diff/1/chromeos/test/data/network/...
File chromeos/test/data/network/shill_wifi_proxy.json (right):

https://codereview.chromium.org/1228543002/diff/1/chromeos/test/data/network/...
chromeos/test/data/network/shill_wifi_proxy.json:3: "ProxyConfig":
"{\"bypass_list\":\"google.com;\\u003Clocal>;\",\"mode\":\"fixed_servers\",\"server\":\"ftp=ftp.example.com:5678;http=http.example.com:1234;socks=socks5://socks.example.com:7890;https=https.example.com:3456\"}",
seems that you got a unicode character \u003C (U+003C = '<') into the ONC by
mistake.

I just wonder where the quotation mark " after google.com went?

[jochen (gone - plz use gerrit), 2015-07-07 11:26:32]

i don't get what this CL is about. please provide a better description

[stevenjb, 2015-07-07 18:30:28]

On 2015/07/07 11:26:32, jochen wrote:
> i don't get what this CL is about. please provide a better description

More information is in the issue, but I went ahead and expanded the summary in
the description a bit. Mostly I just need OWNERS approval for changes to the
proxy_config component DEPS, and for moving some test code from c/b/chromeos to
chromeos/network/onc.

[stevenjb, 2015-07-07 18:30:36]

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:831: << "PAC field is invalid for this
ProxySettings.Type";
On 2015/07/07 10:18:47, pneubeck wrote:
> i think this error message is wrong.
> should instead be:
>   Invalid URL in ProxySettings.PAC
> 
> (although this part should better be verified in the validator)

Done.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:862: // Create a ProxyConfigDictionary from
the DictionaryValye.
On 2015/07/07 10:18:47, pneubeck wrote:
> Valye -> Value

Done.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:896: std::vector<std::string> servers;
On 2015/07/07 10:18:47, pneubeck wrote:
> instead of the following parsing you could instead use
>   net::ProxyConfig::ProxyRules::ParseFromString
> 
> But as I understand it, you don't have to parse the general grammar described
> there
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/proxy_co...)
> but instead you only have to reverse the above function
> ConvertOncProxySettingsToProxyConfig.
> 
> You should emphasize that in the header, that this function is not expecting
> arbitrary ProxyConfig dictionaries but only the ones created by
> ConvertOncProxySettingsToProxyConfig.

I wasn't aware of ProxyRules, I think that is a better way to go in case we ever
set a proxy configuration in Shill through a non ONC path.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:898: if (!servers.empty()) {
On 2015/07/07 10:18:48, pneubeck wrote:
> according to the ONC spec the Manual subdictionary is required if Type is
> Manual.
> 
> please check whether it would be more correct to at least insert the empty
> Manual dict.

Check with who? :P I will always add it.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:900: for (std::string server : servers) {
On 2015/07/07 10:18:47, pneubeck wrote:
> const&

Acknowledged.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:901: size_t index = server.find_first_of('=');
On 2015/07/07 10:18:47, pneubeck wrote:
> optional nit: 'const size_t'

Acknowledged.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:905: std::string default_prefix = scheme;
On 2015/07/07 10:18:47, pneubeck wrote:
> the default prefix for everything but Socks is  net::ProxyServer::SCHEME_HTTP
> and not for example FTP for FTP.
> 
> IIRC, the meaning of this is:
> FTP, HTTP and HTTPS usually are transported over an http connection to the
> proxy.
> Whereas for socks, the socks protocol itself is used.

Acknowledged.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:916: if (!onc_scheme.empty()) {
On 2015/07/07 10:18:48, pneubeck wrote:
> onc_scheme.empty() seems to be a severe error that we at least should log.

Acknowledged.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils.cc:943: for (std::string entry : bypass_list)
On 2015/07/07 10:18:47, pneubeck wrote:
> const&

Using ProxyBypassRules now.

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
File chromeos/network/onc/onc_utils_unittest.cc (right):

https://codereview.chromium.org/1228543002/diff/1/chromeos/network/onc/onc_ut...
chromeos/network/onc/onc_utils_unittest.cc:194:
test_utils::Equals(onc_proxy_settings, actual_proxy_settings.get()));
On 2015/07/07 10:18:48, pneubeck wrote:
> you could also just do that in the the previous test, directly after after the
> other EXPECT_TRUE(...);

Sure, but this makes identifying any failure easier. These are fast.

https://codereview.chromium.org/1228543002/diff/1/chromeos/test/data/network/...
File chromeos/test/data/network/shill_wifi_proxy.json (right):

https://codereview.chromium.org/1228543002/diff/1/chromeos/test/data/network/...
chromeos/test/data/network/shill_wifi_proxy.json:3: "ProxyConfig":
"{\"bypass_list\":\"google.com;\\u003Clocal>;\",\"mode\":\"fixed_servers\",\"server\":\"ftp=ftp.example.com:5678;http=http.example.com:1234;socks=socks5://socks.example.com:7890;https=https.example.com:3456\"}",
On 2015/07/07 10:18:48, pneubeck wrote:
> seems that you got a unicode character \u003C (U+003C = '<') into the ONC by
> mistake.
> 
> I just wonder where the quotation mark " after http://google.com went?

Unfortunately due to a quirk of our parser if I include < in the string, the
"expected" value looks like:

\\u003Clocal>

And the "actual" value looks like:

\u003Clocal>

After beating my head against this for an hour I decided it was easier just to
use the escape code in the .json, rather than figuring out where the extra
escaping is coming from.

https://codereview.chromium.org/1228543002/diff/1/chromeos/test/data/network/...
chromeos/test/data/network/shill_wifi_proxy.json:3: "ProxyConfig":
"{\"bypass_list\":\"google.com;\\u003Clocal>;\",\"mode\":\"fixed_servers\",\"server\":\"ftp=ftp.example.com:5678;http=http.example.com:1234;socks=socks5://socks.example.com:7890;https=https.example.com:3456\"}",
On 2015/07/07 10:18:48, pneubeck wrote:
> seems that you got a unicode character \u003C (U+003C = '<') into the ONC by
> mistake.
> 
> I just wonder where the quotation mark " after http://google.com went?

What quotation mark after what? The ONC exclude domains are
"google.com,<local>".

[pneubeck (no reviews), 2015-07-08 06:52:48]

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:822: proxy_list =
proxy_rules.MapUrlSchemeToProxyList(scheme);
according to the documentation, this should not be called if type ==
TYPE_NO_RULES .
this is ensured by the caller of this function, so probably drop a comment in
front of this function.

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:830: if (server.scheme() ==
net::ProxyServer::SCHEME_SOCKS5)
i think you should handle all non-default schemes here. as the default for
non-socks is HTTP, this could look like (analogous to lines 780, 789 and 760):

// For all proxy types except SOCKS, the default scheme of the proxy host is
HTTP.
net::ProxyServer::Scheme default_scheme = net::ProxyServer::SCHEME_HTTP;
if (onc_scheme == ::onc::proxy::kSocks) {
  default_scheme = net::ProxyServer::SCHEME_SOCKS4;
}
// Only prefix the host with a non-default scheme.
if (server.scheme() != default_scheme)
  host = SchemeToString(server.scheme()) + "://" + host;

[jochen (gone - plz use gerrit), 2015-07-08 13:20:14]

jochen@chromium.org changed reviewers:
+ eroman@chromium.org

[jochen (gone - plz use gerrit), 2015-07-08 13:20:15]

+eroman, do those changes make sense from a net PoV?

https://codereview.chromium.org/1228543002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/ui_proxy_config.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/ui_proxy_config.cc:124: "http", http_proxy.server,
&spec);
please don't use 'http' as a string but the kHttpScheme etc.. constants

https://codereview.chromium.org/1228543002/diff/20001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/1228543002/diff/20001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:573:
'../google_apis/google_apis.gyp:google_apis_test_support',
indent

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:751: const char kSchemeSocks[] = "socks";
please don't redefine but use the proper constants

https://codereview.chromium.org/1228543002/diff/20001/components/proxy_config...
File components/proxy_config/proxy_config_dictionary.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/components/proxy_config...
components/proxy_config/proxy_config_dictionary.cc:150: std::string* spec) {
why not return a string?

[stevenjb, 2015-07-08 16:23:07]

Patchset #3 (id:40001) has been deleted

[stevenjb, 2015-07-08 16:26:15]

https://codereview.chromium.org/1228543002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/ui_proxy_config.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/ui_proxy_config.cc:124: "http", http_proxy.server,
&spec);
On 2015/07/08 13:20:15, jochen wrote:
> please don't use 'http' as a string but the kHttpScheme etc.. constants

I just moved the function call, but I will go ahead and make the requested
changes. Done (note: no scheme is defined for "socks").

https://codereview.chromium.org/1228543002/diff/20001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/1228543002/diff/20001/chromeos/chromeos.gyp#n...
chromeos/chromeos.gyp:573:
'../google_apis/google_apis.gyp:google_apis_test_support',
On 2015/07/08 13:20:15, jochen wrote:
> indent

huh?

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:751: const char kSchemeSocks[] = "socks";
On 2015/07/08 13:20:15, jochen wrote:
> please don't redefine but use the proper constants

Done.

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:822: proxy_list =
proxy_rules.MapUrlSchemeToProxyList(scheme);
On 2015/07/08 06:52:48, pneubeck wrote:
> according to the documentation, this should not be called if type ==
> TYPE_NO_RULES .
> this is ensured by the caller of this function, so probably drop a comment in
> front of this function.

I added an explicit test here.

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:830: if (server.scheme() ==
net::ProxyServer::SCHEME_SOCKS5)
On 2015/07/08 06:52:48, pneubeck wrote:
> i think you should handle all non-default schemes here. as the default for
> non-socks is HTTP, this could look like (analogous to lines 780, 789 and 760):
> 
> // For all proxy types except SOCKS, the default scheme of the proxy host is
> HTTP.
> net::ProxyServer::Scheme default_scheme = net::ProxyServer::SCHEME_HTTP;
> if (onc_scheme == ::onc::proxy::kSocks) {
>   default_scheme = net::ProxyServer::SCHEME_SOCKS4;
> }
> // Only prefix the host with a non-default scheme.
> if (server.scheme() != default_scheme)
>   host = SchemeToString(server.scheme()) + "://" + host;

So, there is no "SchemeToString" function, and effectively if I wrote one here
it would be "if (scheme == net::ProxyServer::SCHEME_SOCKS5) return "socks5",
unless you are suggesting we handle SCHEME_DIRECT and SCEME_QUIC?

I went ahead and updated the comment.

https://codereview.chromium.org/1228543002/diff/20001/components/proxy_config...
File components/proxy_config/proxy_config_dictionary.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/components/proxy_config...
components/proxy_config/proxy_config_dictionary.cc:150: std::string* spec) {
On 2015/07/08 13:20:15, jochen wrote:
> why not return a string?

I just moved this so that it could be used outside of src/chrome; I would just
as soon not re-implement it.

[stevenjb, 2015-07-08 16:34:08]

The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run

[stevenjb, 2015-07-08 16:34:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1228543002/#ps60001
(title: "Feedback")

[stevenjb, 2015-07-08 16:34:24]

PTAL

[commit-bot: I haz the power, 2015-07-08 16:35:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228543002/60001

[stevenjb, 2015-07-08 16:37:39]

The CQ bit was unchecked by stevenjb@chromium.org

[stevenjb, 2015-07-08 16:45:18]

The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run

[stevenjb, 2015-07-08 16:45:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1228543002/#ps80001
(title: "Compile fix")

[commit-bot: I haz the power, 2015-07-08 16:45:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228543002/80001

[commit-bot: I haz the power, 2015-07-08 16:56:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-08 16:56:44]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[eroman, 2015-07-08 21:26:38]

Looks fine from net point of view.

My only concern was around a seeming lack of error checking when parsing from
JSON.

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_translator_shill_to_onc.cc:572:
ReadDictionaryFromJson(proxy_config_str));
This can return NULL.

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_translator_shill_to_onc.cc:574:
ConvertProxyConfigToOncProxySettings(*proxy_config_value);
Note that proxy_config_value could be NULL here.

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:758: net::ProxyServer::FromURI(host,
default_proxy_scheme);
Parsing might fail, in which case proxy_server will be invalid. Where does this
data come from?

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:765:
net::HostPortPair(proxy_server.host_port_pair().host(),
proxy_server may be invalid, in which case calling host_port_pair() is not
valid.

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:766: static_cast<uint16>(port)));
possibility of underflow.
Is the input considered preference considered valid?

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:860:
onc_proxy_settings.GetDictionaryWithoutPathExpansion(::onc::proxy::kManual,
The error checking throughout is pretty weak.
For instance here it is assumed that this will succeed and manual_proxy be
non-null.

[stevenjb, 2015-07-08 22:13:08]

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_translator_shill_to_onc.cc:572:
ReadDictionaryFromJson(proxy_config_str));
On 2015/07/08 21:26:37, eroman wrote:
> This can return NULL.

Good catch, added test.

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_translator_shill_to_onc.cc:574:
ConvertProxyConfigToOncProxySettings(*proxy_config_value);
On 2015/07/08 21:26:37, eroman wrote:
> Note that proxy_config_value could be NULL here.

ConvertProxyConfigToOncProxySettings can also return null, added a test here
too.

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:758: net::ProxyServer::FromURI(host,
default_proxy_scheme);
On 2015/07/08 21:26:38, eroman wrote:
> Parsing might fail, in which case proxy_server will be invalid. Where does
this
> data come from?
Theoretically, ONC validation should ensure that these are valid values, but
even if the are not, if I read the code correctly, this would fail gracefully;
ProxyConfigDictionary::EncodeAndAppendProxyServer would do nothing and |spec| in
AppendProxyServerForScheme would be unchanged (i.e. the proxy would not be
appended).

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:765:
net::HostPortPair(proxy_server.host_port_pair().host(),
On 2015/07/08 21:26:38, eroman wrote:
> proxy_server may be invalid, in which case calling host_port_pair() is not
> valid.

Since the scheme is invalid, the host_port_pair value shouldn't matter here.

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:766: static_cast<uint16>(port)));
On 2015/07/08 21:26:38, eroman wrote:
> possibility of underflow.
> Is the input considered preference considered valid?

The ONC input should be validated already. Otherwise it appears that the port
gets clamped to a valid range elsewhere (I'm not super familiar with the proxy
code myself, this particular code was just re-located in this CL).

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:860:
onc_proxy_settings.GetDictionaryWithoutPathExpansion(::onc::proxy::kManual,
On 2015/07/08 21:26:38, eroman wrote:
> The error checking throughout is pretty weak.
> For instance here it is assumed that this will succeed and manual_proxy be
> non-null.

Here ONC validation will definitely ensure that if type == Manual then a Manual
dictionary exists.

[pneubeck (no reviews), 2015-07-09 07:16:12]

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:830: if (server.scheme() ==
net::ProxyServer::SCHEME_SOCKS5)
On 2015/07/08 16:26:14, stevenjb wrote:
> On 2015/07/08 06:52:48, pneubeck wrote:
> > i think you should handle all non-default schemes here. as the default for
> > non-socks is HTTP, this could look like (analogous to lines 780, 789 and
760):
> > 
> > // For all proxy types except SOCKS, the default scheme of the proxy host is
> > HTTP.
> > net::ProxyServer::Scheme default_scheme = net::ProxyServer::SCHEME_HTTP;
> > if (onc_scheme == ::onc::proxy::kSocks) {
> >   default_scheme = net::ProxyServer::SCHEME_SOCKS4;
> > }
> > // Only prefix the host with a non-default scheme.
> > if (server.scheme() != default_scheme)
> >   host = SchemeToString(server.scheme()) + "://" + host;
> 
> So, there is no "SchemeToString" function, 
sadly... the net/proxy code hides this conversion in the .cc .

> and effectively if I wrote one here
> it would be "if (scheme == net::ProxyServer::SCHEME_SOCKS5) return "socks5",
no, that's not right. you're adding the prefix for every _non_-default scheme.

> unless you are suggesting we handle SCHEME_DIRECT and SCEME_QUIC?
at least SCHEME_DIRECT should not be necessary, i guess.

> I went ahead and updated the comment.

The current code will not be able to handle:

http=socks4://example.com;https=...

which must be translated to

Manual: {
  "HTTP": {
     host: "socks4://example.com",
     port: 123
  },
  "HTTPS":
   ....
}

instead i think you're currently dropping the scheme 'socks4'.


e.g. a similar configuration is mentioned here:
https://www.chromium.org/developers/design-documents/network-stack/socks-proxy

https://codereview.chromium.org/1228543002/diff/140001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/140001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:788: url_scheme = kSocksScheme;
According to
https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/proxy_se...
you could as well set
  url_scheme = url::kSocks4Scheme
ugh... there is no url::*Socks*  o_o

https://codereview.chromium.org/1228543002/diff/140001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils_unittest.cc (right):

https://codereview.chromium.org/1228543002/diff/140001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils_unittest.cc:23: scoped_ptr<base::Value>
ReadTestJson(const std::string& filename) {
optional nit: this could eventually be put into a common place together with
chromeos/network/onc/onc_test_utils.cc:ReadTestDictionary

[pneubeck (no reviews), 2015-07-09 07:20:32]

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:766: static_cast<uint16>(port)));
On 2015/07/08 22:13:08, stevenjb wrote:
> On 2015/07/08 21:26:38, eroman wrote:
> > possibility of underflow.
> > Is the input considered preference considered valid?
> 
> The ONC input should be validated already. Otherwise it appears that the port
> gets clamped to a valid range elsewhere (I'm not super familiar with the proxy
> code myself, this particular code was just re-located in this CL).

The port of the proxy location isn't validated currently by the ONC validator:
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/o...

probably something worth adding.

[stevenjb, 2015-07-09 18:24:29]

Trying the SchemeToString suggestion this time. If this isn't satisfactory (at
least for now), then I am going to need some more explicit help with this.
Thanks.

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:830: if (server.scheme() ==
net::ProxyServer::SCHEME_SOCKS5)
On 2015/07/09 07:16:12, pneubeck wrote:
> On 2015/07/08 16:26:14, stevenjb wrote:
> > On 2015/07/08 06:52:48, pneubeck wrote:
> > > i think you should handle all non-default schemes here. as the default for
> > > non-socks is HTTP, this could look like (analogous to lines 780, 789 and
> 760):
> > > 
> > > // For all proxy types except SOCKS, the default scheme of the proxy host
is
> > > HTTP.
> > > net::ProxyServer::Scheme default_scheme = net::ProxyServer::SCHEME_HTTP;
> > > if (onc_scheme == ::onc::proxy::kSocks) {
> > >   default_scheme = net::ProxyServer::SCHEME_SOCKS4;
> > > }
> > > // Only prefix the host with a non-default scheme.
> > > if (server.scheme() != default_scheme)
> > >   host = SchemeToString(server.scheme()) + "://" + host;
> > 
> > So, there is no "SchemeToString" function, 
> sadly... the net/proxy code hides this conversion in the .cc .
> 
> > and effectively if I wrote one here
> > it would be "if (scheme == net::ProxyServer::SCHEME_SOCKS5) return "socks5",
> no, that's not right. you're adding the prefix for every _non_-default scheme.
> 
> > unless you are suggesting we handle SCHEME_DIRECT and SCEME_QUIC?
> at least SCHEME_DIRECT should not be necessary, i guess.
> 
> > I went ahead and updated the comment.
> 
> The current code will not be able to handle:
> 
> http=socks4://example.com;https=...
> 
> which must be translated to
> 
> Manual: {
>   "HTTP": {
>      host: "socks4://example.com",
>      port: 123
>   },
>   "HTTPS":
>    ....
> }
> 
> instead i think you're currently dropping the scheme 'socks4'.
> 
> 
> e.g. a similar configuration is mentioned here:
> https://www.chromium.org/developers/design-documents/network-stack/socks-proxy

My head is starting to hurt. I am looking at the code in ProxyConfig/ProxyServer
and am now wondering if we should even be using it here. Sigh.

This example seems to contradict your previous suggestion. If we set
default_scheme == SCHEME_SOCKS4 for ::onc::proxy::kSocks, then both socks:// and
socks4:// will skip the scheme since they both translate to SCHEME_SOCKS4 in
ProxyConfig.

I went ahead and added a SchemeToString() function to make this explicit and to
cover QUIC

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:766: static_cast<uint16>(port)));
On 2015/07/09 07:20:32, pneubeck wrote:
> On 2015/07/08 22:13:08, stevenjb wrote:
> > On 2015/07/08 21:26:38, eroman wrote:
> > > possibility of underflow.
> > > Is the input considered preference considered valid?
> > 
> > The ONC input should be validated already. Otherwise it appears that the
port
> > gets clamped to a valid range elsewhere (I'm not super familiar with the
proxy
> > code myself, this particular code was just re-located in this CL).
> 
> The port of the proxy location isn't validated currently by the ONC validator:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/o...
> 
> probably something worth adding.

Probably. Currently we seem to at least be clamping the port somewhere - if I
enter an invalid port in the UI it comes back as a valid value after the
round-trip (ONC -> Shill -> ONC), so it's getting clamped *somewhere*.
Unfortunately this effort is the first time I have really looked at any of the
proxy code, and there seems to be a *lot* of it.

https://codereview.chromium.org/1228543002/diff/140001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/140001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:788: url_scheme = kSocksScheme;
On 2015/07/09 07:16:12, pneubeck wrote:
> According to
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/proxy_se...
> you could as well set
>   url_scheme = url::kSocks4Scheme
> ugh... there is no url::*Socks*  o_o

Right. So... suggestions? I was assuming that socks===socks4.

https://codereview.chromium.org/1228543002/diff/140001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils_unittest.cc (right):

https://codereview.chromium.org/1228543002/diff/140001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils_unittest.cc:23: scoped_ptr<base::Value>
ReadTestJson(const std::string& filename) {
On 2015/07/09 07:16:12, pneubeck wrote:
> optional nit: this could eventually be put into a common place together with
> chromeos/network/onc/onc_test_utils.cc:ReadTestDictionary

Eventually :) We should do that if/when we need it elsewhere.

[pneubeck (no reviews), 2015-07-10 07:20:43]

Thanks for addressing the scheme prefixing.  lgtm

You see now why I originally only translated from ONC -> ProxyConfig and not
back.
If we would store the user's settings as ONC, we would never need the
translation direction ProxyConfig -> ONC .

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:830: if (server.scheme() ==
net::ProxyServer::SCHEME_SOCKS5)
On 2015/07/09 18:24:28, stevenjb wrote:
> On 2015/07/09 07:16:12, pneubeck wrote:
> > On 2015/07/08 16:26:14, stevenjb wrote:
> > > On 2015/07/08 06:52:48, pneubeck wrote:
> > > > i think you should handle all non-default schemes here. as the default
for
> > > > non-socks is HTTP, this could look like (analogous to lines 780, 789 and
> > 760):
> > > > 
> > > > // For all proxy types except SOCKS, the default scheme of the proxy
host
> is
> > > > HTTP.
> > > > net::ProxyServer::Scheme default_scheme = net::ProxyServer::SCHEME_HTTP;
> > > > if (onc_scheme == ::onc::proxy::kSocks) {
> > > >   default_scheme = net::ProxyServer::SCHEME_SOCKS4;
> > > > }
> > > > // Only prefix the host with a non-default scheme.
> > > > if (server.scheme() != default_scheme)
> > > >   host = SchemeToString(server.scheme()) + "://" + host;
> > > 
> > > So, there is no "SchemeToString" function, 
> > sadly... the net/proxy code hides this conversion in the .cc .
> > 
> > > and effectively if I wrote one here
> > > it would be "if (scheme == net::ProxyServer::SCHEME_SOCKS5) return
"socks5",
> > no, that's not right. you're adding the prefix for every _non_-default
scheme.
> > 
> > > unless you are suggesting we handle SCHEME_DIRECT and SCEME_QUIC?
> > at least SCHEME_DIRECT should not be necessary, i guess.
> > 
> > > I went ahead and updated the comment.
> > 
> > The current code will not be able to handle:
> > 
> > http=socks4://example.com;https=...
> > 
> > which must be translated to
> > 
> > Manual: {
> >   "HTTP": {
> >      host: "socks4://example.com",
> >      port: 123
> >   },
> >   "HTTPS":
> >    ....
> > }
> > 
> > instead i think you're currently dropping the scheme 'socks4'.
> > 
> > 
> > e.g. a similar configuration is mentioned here:
> >
https://www.chromium.org/developers/design-documents/network-stack/socks-proxy
> 
> My head is starting to hurt. I am looking at the code in
ProxyConfig/ProxyServer
> and am now wondering if we should even be using it here. Sigh.
> 
> This example seems to contradict your previous suggestion.
I'm not sure where you see a contradiction.

> If we set
> default_scheme == SCHEME_SOCKS4 for ::onc::proxy::kSocks, then both socks://
and
> socks4:// will skip the scheme since they both translate to SCHEME_SOCKS4 in
> ProxyConfig.

As socks:// == socks4://, skipping these two in a host url seems the intended
behavior for socks proxies.

Thinking more about it, the whole skipping the default scheme is only an
optimization and might not be worth it. Neither skipping the default nor always
adding the scheme will make the round trip (ONC -> Shill -> ONC) an identity
operation. There are always cases where we get back a different result than what
was originally set.
e.g. set ONC to
   HTTPProxy: {
     Host: "http://1.2.3.4"
     Port: 1234
   }
 => "http://" will be dropped if we skip the default scheme

 or set ONC to
   HTTPProxy: {
     Host: "1.2.3.4"
     Port: 1234
   }
 =>  Scheme prefix "http://" will be added if we always add the prefix


> I went ahead and added a SchemeToString() function to make this explicit and
to cover QUIC

https://codereview.chromium.org/1228543002/diff/140001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/140001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:788: url_scheme = kSocksScheme;
On 2015/07/09 18:24:28, stevenjb wrote:
> On 2015/07/09 07:16:12, pneubeck wrote:
> > According to
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/proxy_se...
> > you could as well set
> >   url_scheme = url::kSocks4Scheme
> > ugh... there is no url::*Socks*  o_o
> 
> Right. So... suggestions? I was assuming that socks===socks4.

socks4 seems to be the new name for socks. so this should be fine.
I was just surprised by the unfortunate mix of newly defined constants and
existing constants.

https://codereview.chromium.org/1228543002/diff/160001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/160001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:860: if (server.scheme() != default_scheme)
as explained in the other comment, it should also be ok to not handle default
vs. non-default scheme here and just execute line 861 (prefixing the scheme)
unconditionally.

up to you.

[jochen (gone - plz use gerrit), 2015-07-10 09:08:47]

lgtm

[stevenjb, 2015-07-10 16:16:59]

https://codereview.chromium.org/1228543002/diff/160001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/160001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:860: if (server.scheme() != default_scheme)
On 2015/07/10 07:20:42, pneubeck wrote:
> as explained in the other comment, it should also be ok to not handle default
> vs. non-default scheme here and just execute line 861 (prefixing the scheme)
> unconditionally.
> 
> up to you.

I am going to leave it as is for now because it would require changing a bunch
of test expectations, but I agree that it would be simpler if we just always
used an explicit prefix and only added missing ones.  There is definitely room
for a cleanup pass here (hopefully by someone more familiar with proxies in
general :) )

[stevenjb, 2015-07-10 16:17:21]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-07-10 16:17:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228543002/160001

[commit-bot: I haz the power, 2015-07-10 16:27:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 16:27:20]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[stevenjb, 2015-07-10 20:22:10]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-07-10 20:22:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228543002/160001

[commit-bot: I haz the power, 2015-07-10 20:34:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-10 20:34:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[stevenjb, 2015-07-10 20:43:41]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-07-10 20:45:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1228543002/160001

[commit-bot: I haz the power, 2015-07-10 20:51:55]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-07-10 20:52:54]

Patchset 8 (id:??) landed as
https://crrev.com/8336888d95e672e8642889cade066aed7bb1f595
Cr-Commit-Position: refs/heads/master@{#338352}

[eroman, 2015-07-13 20:08:38]

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:830: if (server.scheme() ==
net::ProxyServer::SCHEME_SOCKS5)
On 2015/07/10 07:20:42, pneubeck wrote:
> On 2015/07/09 18:24:28, stevenjb wrote:
> > On 2015/07/09 07:16:12, pneubeck wrote:
> > > On 2015/07/08 16:26:14, stevenjb wrote:
> > > > On 2015/07/08 06:52:48, pneubeck wrote:
> > > > > i think you should handle all non-default schemes here. as the default
> for
> > > > > non-socks is HTTP, this could look like (analogous to lines 780, 789
and
> > > 760):
> > > > > 
> > > > > // For all proxy types except SOCKS, the default scheme of the proxy
> host
> > is
> > > > > HTTP.
> > > > > net::ProxyServer::Scheme default_scheme =
net::ProxyServer::SCHEME_HTTP;
> > > > > if (onc_scheme == ::onc::proxy::kSocks) {
> > > > >   default_scheme = net::ProxyServer::SCHEME_SOCKS4;
> > > > > }
> > > > > // Only prefix the host with a non-default scheme.
> > > > > if (server.scheme() != default_scheme)
> > > > >   host = SchemeToString(server.scheme()) + "://" + host;
> > > > 
> > > > So, there is no "SchemeToString" function, 
> > > sadly... the net/proxy code hides this conversion in the .cc .
> > > 
> > > > and effectively if I wrote one here
> > > > it would be "if (scheme == net::ProxyServer::SCHEME_SOCKS5) return
> "socks5",
> > > no, that's not right. you're adding the prefix for every _non_-default
> scheme.
> > > 
> > > > unless you are suggesting we handle SCHEME_DIRECT and SCEME_QUIC?
> > > at least SCHEME_DIRECT should not be necessary, i guess.
> > > 
> > > > I went ahead and updated the comment.
> > > 
> > > The current code will not be able to handle:
> > > 
> > > http=socks4://example.com;https=...
> > > 
> > > which must be translated to
> > > 
> > > Manual: {
> > >   "HTTP": {
> > >      host: "socks4://example.com",
> > >      port: 123
> > >   },
> > >   "HTTPS":
> > >    ....
> > > }
> > > 
> > > instead i think you're currently dropping the scheme 'socks4'.
> > > 
> > > 
> > > e.g. a similar configuration is mentioned here:
> > >
> https://www.chromium.org/developers/design-documents/network-stack/socks-proxy
> > 
> > My head is starting to hurt. I am looking at the code in
> ProxyConfig/ProxyServer
> > and am now wondering if we should even be using it here. Sigh.
> > 
> > This example seems to contradict your previous suggestion.
> I'm not sure where you see a contradiction.
> 
> > If we set
> > default_scheme == SCHEME_SOCKS4 for ::onc::proxy::kSocks, then both socks://
> and
> > socks4:// will skip the scheme since they both translate to SCHEME_SOCKS4 in
> > ProxyConfig.
> 
> As socks:// == socks4://, skipping these two in a host url seems the intended
> behavior for socks proxies.
> 
> Thinking more about it, the whole skipping the default scheme is only an
> optimization and might not be worth it. Neither skipping the default nor
always
> adding the scheme will make the round trip (ONC -> Shill -> ONC) an identity
> operation. There are always cases where we get back a different result than
what
> was originally set.
> e.g. set ONC to
>    HTTPProxy: {
>      Host: "http://1.2.3.4"
>      Port: 1234
>    }
>  => "http://" will be dropped if we skip the default scheme
> 
>  or set ONC to
>    HTTPProxy: {
>      Host: "1.2.3.4"
>      Port: 1234
>    }
>  =>  Scheme prefix "http://" will be added if we always add the prefix
> 
> 
> > I went ahead and added a SchemeToString() function to make this explicit and
> to cover QUIC
> 

There are a few factors at play here with regards to the SOCKS default
protocol...

(1) In PAC scripts, one indicates use of a SOCKS (v4) proxy by using the the
string format "SOCKS proxy:port" rather than "PROXY proxy:port". When SOCKS v5
was added, browsers required specifying it instead as "SOCKS5 proxy:port" to
disambiguate. So in this domain, "SOCKS" refers to v4.

(2) The serialization format you are using by calling
ProxyRules::ParseFromString() is actually the Windows WinInet serialization
format (with more allowances). I guess you decided to use it because there was
already code for it, but it has its own idiosyncrasies. In this format,
"socks=host:port" indicates the use of a socks v4 proxy, because that is how
Windows apps interpreted it. In fact, strictly speaking there is no support for
SOCKS v5 in this windows format, however Chrome hacks it in by interpret leading
protocol designators, such as "socks=socks5://host:port"

(3) Command line flags. This is where it gets interesting. In Chrome you can
specify proxy servers using
--proxy-server="<proxy-protocol>:://<proxy-host>:<proxy-port>". This 3 tuple
expressing the proxy protocol, host, port is represented by ProxyServer class.
Unlike the other representations this is unambiguous about he role of the proxy.
This representation looks like a URL, but isn't, it is just a convenient
serialization. It allows expressing things like "HTTPS" proxies which were a
Chrome invention (not to be confused with an HTTP proxy for https://* URLs,
which is what you are configuring with https=...)

OK, so in this format, "socks://" actually means "socks5://". We invented this
format, so for consistency we could have made socks:// mean socks4://. And in
fact is originally what we did. But because so many people were filing bugs due
to incorrectly specifying a socks 4 proxy when they mean to specify socks 5, we
switched the default here. Socks v4 is practically never what you want to be
using in Chrome. Socks v5 dates back to the 1996 and is widely supported

So to sum it up, if you want to be using ProxyRules::ParseFromString() using the
Windows semantics, then:

   socks=foobar     --> means socks v4 proxy
   socks=socks://foobar --> means socks v5 proxy
   socks=socks4://foobar --> means socks v4 proxy
   socks=socks5://foobar --> means socks v5 proxy

Where the addition of a scheme in front of the host:port is a Chrome invention
and not something that Windows would actually interpret (If you loaded these in
Internet Explorer would fail).

Hope that helps clarify the confusion.

Unfortunately the "standard" way of expressing proxy settings as a series of
host:port per URL scheme, and then SOCKS, is really quite terrible. SOCKS is not
a URL scheme like the others, but rather a proxy protocol (with implicit version
depending on who sets it). And this socks proxy is generally used as a fallback
for something not matched by the URL specific ones (even though you could
interpret it as a proxy for the transport layer in addition to the other proxy
usages).

Also include plenty of tests on how your serialized settings get interpreted,
since if ProxyRules::ParseFromString() were ever changed to match some Windows
specific behavior (which it occasionally does) you wouldn't want your
serializations to break.

</rant>

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/1228543002/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_utils.cc:765:
net::HostPortPair(proxy_server.host_port_pair().host(),
On 2015/07/08 22:13:08, stevenjb wrote:
> On 2015/07/08 21:26:38, eroman wrote:
> > proxy_server may be invalid, in which case calling host_port_pair() is not
> > valid.
> 
> Since the scheme is invalid, the host_port_pair value shouldn't matter here.

It will hit a DCHECK in ProxyServer::host_port_pair()