Allow CustomProxyConfig to override existing config
A CustomProxyConfig provider like Opera VPN will now be able to specify
that the custom config must take priority over all other sources of
proxy configuration. The default behavior is still the one required by
the Data Reduction Proxy: the DRP config is only effective if nothing
else provided a proxy config.
Change-Id: I4cc0a61375b668ae1806bb6978b3e0893ed941ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1491252
Commit-Queue: Wojciech Dzierżanowski <wdzierzanowski@opera.com>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#640707}diff --git a/chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc b/chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc
index 6577f10..b18953b 100644
--- a/chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc
+++ b/chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc
@@ -27,6 +27,8 @@
#include "components/data_reduction_proxy/core/common/uma_util.h"
#include "components/data_reduction_proxy/proto/client_config.pb.h"
#include "components/prefs/pref_service.h"
+#include "components/proxy_config/proxy_config_dictionary.h"
+#include "components/proxy_config/proxy_config_pref_names.h"
#include "content/public/browser/network_service_instance.h"
#include "content/public/common/network_service_util.h"
#include "content/public/common/service_manager_connection.h"
@@ -381,6 +383,29 @@
EXPECT_FALSE(tunnel_attempted);
}
+IN_PROC_BROWSER_TEST_F(DataReductionProxyBrowsertest,
+ DoesNotOverrideExistingProxyConfig) {
+ // When there's a proxy configuration provided to the browser already (system
+ // proxy, command line, etc.), the DRP proxy must not override it.
+ net::EmbeddedTestServer existing_proxy_server;
+ existing_proxy_server.RegisterRequestHandler(
+ base::BindRepeating(&BasicResponse, kDummyBody));
+ ASSERT_TRUE(existing_proxy_server.Start());
+
+ browser()->profile()->GetPrefs()->Set(
+ proxy_config::prefs::kProxy,
+ ProxyConfigDictionary::CreateFixedServers(
+ existing_proxy_server.host_port_pair().ToString(), ""));
+
+ EnableDataSaver(true);
+
+ // Proxy will be used, so it shouldn't matter if the host cannot be resolved.
+ ui_test_utils::NavigateToURL(browser(),
+ GURL("http://does.not.resolve.com/echo"));
+
+ EXPECT_EQ(GetBody(), kDummyBody);
+}
+
IN_PROC_BROWSER_TEST_F(DataReductionProxyBrowsertest, UMAMetricsRecorded) {
base::HistogramTester histogram_tester;
diff --git a/services/network/network_service_proxy_delegate.cc b/services/network/network_service_proxy_delegate.cc
index d2ba9ff..9bb15f7 100644
--- a/services/network/network_service_proxy_delegate.cc
+++ b/services/network/network_service_proxy_delegate.cc
@@ -299,9 +299,17 @@
bool NetworkServiceProxyDelegate::EligibleForProxy(
const net::ProxyInfo& proxy_info,
const std::string& method) const {
- return proxy_info.is_direct() && proxy_info.proxy_list().size() == 1 &&
- (proxy_config_->allow_non_idempotent_methods ||
- net::HttpUtil::IsMethodIdempotent(method));
+ bool has_existing_config =
+ !proxy_info.is_direct() || proxy_info.proxy_list().size() > 1u;
+ if (!proxy_config_->should_override_existing_config && has_existing_config)
+ return false;
+
+ if (!proxy_config_->allow_non_idempotent_methods &&
+ !net::HttpUtil::IsMethodIdempotent(method)) {
+ return false;
+ }
+
+ return true;
}
net::ProxyConfig::ProxyRules NetworkServiceProxyDelegate::GetProxyRulesForURL(
diff --git a/services/network/network_service_proxy_delegate_unittest.cc b/services/network/network_service_proxy_delegate_unittest.cc
index 57e1421..404028f0 100644
--- a/services/network/network_service_proxy_delegate_unittest.cc
+++ b/services/network/network_service_proxy_delegate_unittest.cc
@@ -475,6 +475,7 @@
TEST_F(NetworkServiceProxyDelegateTest, OnResolveProxyDoesNotOverrideExisting) {
auto config = mojom::CustomProxyConfig::New();
config->rules.ParseFromString("http=foo");
+ config->should_override_existing_config = false;
auto delegate = CreateDelegate(std::move(config));
net::ProxyInfo result;
@@ -489,6 +490,24 @@
EXPECT_FALSE(result.alternative_proxy().is_valid());
}
+TEST_F(NetworkServiceProxyDelegateTest, OnResolveProxyOverridesExisting) {
+ auto config = mojom::CustomProxyConfig::New();
+ config->rules.ParseFromString("http=foo");
+ config->should_override_existing_config = true;
+ auto delegate = CreateDelegate(std::move(config));
+
+ net::ProxyInfo result;
+ result.UsePacString("PROXY bar");
+ delegate->OnResolveProxy(GURL(kHttpUrl), "GET", net::ProxyRetryInfoMap(),
+ &result);
+
+ net::ProxyList expected_proxy_list;
+ expected_proxy_list.AddProxyServer(
+ net::ProxyServer::FromPacString("PROXY foo"));
+ EXPECT_TRUE(result.proxy_list().Equals(expected_proxy_list));
+ EXPECT_FALSE(result.alternative_proxy().is_valid());
+}
+
TEST_F(NetworkServiceProxyDelegateTest, OnResolveProxyDeprioritizesBadProxies) {
auto config = mojom::CustomProxyConfig::New();
config->rules.ParseFromString("http=foo,bar");
diff --git a/services/network/public/mojom/network_context.mojom b/services/network/public/mojom/network_context.mojom
index b4db742..8ad776b 100644
--- a/services/network/public/mojom/network_context.mojom
+++ b/services/network/public/mojom/network_context.mojom
@@ -42,10 +42,12 @@
[EnableIf=is_trial_comparison_cert_verifier_supported]
import "services/network/public/mojom/trial_comparison_cert_verifier.mojom";
-// Config for setting a custom proxy config that will be used if a request
-// matches the proxy rules and would otherwise be direct. This config allows
-// headers to be set on requests to the proxies from the config before and/or
-// after the caching layer.
+// The browser can provide a CustomProxyConfig to a CustomProxyConfigClient
+// running in the network service. The network service will use the given proxy
+// configuration if a request matches the proxy rules and all the other
+// criteria contained within it. This configuration allows the browser to
+// direct the network service to set headers on requests to the given proxies
+// before and/or after the caching layer.
struct CustomProxyConfig {
// The custom proxy rules to use. Note that ftp:// requests are not
// supported.
@@ -55,6 +57,11 @@
// ResourceRequest::custom_proxy_use_alternate_proxy_list is set.
ProxyRules alternate_rules;
+ // Whether the custom proxy config should override other sources of proxy
+ // configuration. If false, the custom config is ignored if a proxy is set in
+ // the operating system, for example.
+ bool should_override_existing_config = false;
+
// Whether the custom proxy config should apply to requests using
// non-idempotent methods. Can be true if the proxy is known to handle this
// case properly.
