Sanitize https:// URLs before sending them to PAC scripts.

net/proxy/proxy_service_unittest.cc

Index: net/proxy/proxy_service_unittest.cc
diff --git a/net/proxy/proxy_service_unittest.cc b/net/proxy/proxy_service_unittest.cc
index a0db685aaba34234fca5ac5a7afcf567a214b7eb..5cd57e79074f8f61b46194dc13c1db593a8589d0 100644
--- a/net/proxy/proxy_service_unittest.cc
+++ b/net/proxy/proxy_service_unittest.cc
@@ -3480,4 +3480,212 @@ TEST_F(ProxyServiceTest, SynchronousWithFixedConfiguration) {
   EXPECT_EQ(0u, factory->pending_requests().size());
 }
 
+// This test checks that URLs are sanitized before being passed on to the proxy
+// resolver (i.e. PAC scripts).
+TEST_F(ProxyServiceTest, SanitizeUrlsBeforeResolving) {

[mmenke, 2016/05/20 17:40:23]

Didn't realize you had this sort of test as well, yesterday - just assumed that
ProxyService::SanitizeUrl's existence as a public method meant you were only
testing at that level.

Could consider making it private / moving it to an anonymous namespace, and make
all tests go through the ConfigService, so you're only testing its public
interface, rather than its implementation details.  Could either add a helper
method, or just put the request2 / request3 in a loop.  Up to you, I just have a
personal preference to avoid adding to the public API just for tests if it can
reasonably be avoided, and think this is little enough boilerplate that it falls
into that category.

[eroman, 2016/05/20 21:23:53]

Done.
I agree that is a better approach.
All of the testing is now going through ProxyService.

+  MockProxyConfigService* config_service =
+      new MockProxyConfigService("http://foopy/proxy.pac");
+  MockAsyncProxyResolver resolver;
+  MockAsyncProxyResolverFactory* factory =
+      new MockAsyncProxyResolverFactory(false);
+
+  ProxyService service(base::WrapUnique(config_service),
+                       base::WrapUnique(factory), nullptr);
+
+  // Request 1 ----------------------------------------------------------------
+
+  // This URL includes a lot of different parts (some of these are expected to
+  // be stripped).
+  GURL url1("http://username:password@www.google.com:8080/path?query#fragment");
+  GURL sanitized_url1("http://www.google.com:8080/path?query");
+
+  ProxyInfo info1;
+  TestCompletionCallback callback1;
+  int rv = service.ResolveProxy(url1, std::string(), LOAD_NORMAL, &info1,
+                                callback1.callback(), nullptr, nullptr,
+                                BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  // First step is to download the PAC script.
+  EXPECT_EQ(GURL("http://foopy/proxy.pac"),
+            factory->pending_requests()[0]->script_data()->url());
+  factory->pending_requests()[0]->CompleteNowWithForwarder(OK, &resolver);
+
+  ASSERT_EQ(1u, resolver.pending_requests().size());
+  // Ensure that the sanitized URL was sent to the proxy resolver (not the full
+  // URL).
+  EXPECT_EQ(sanitized_url1, resolver.pending_requests()[0]->url());
+
+  // Complete the request.
+  resolver.pending_requests()[0]->results()->UsePacString("DIRECT");
+  resolver.pending_requests()[0]->CompleteNow(OK);
+  EXPECT_EQ(OK, callback1.WaitForResult());
+  EXPECT_TRUE(info1.is_direct());
+
+  // Request 2 ----------------------------------------------------------------
+
+  // Start another request. This time the URL is https://, so a different
+  // sanitization strategy should be used.
+  GURL url2(
+      "https://username:password@www.google.com:8080/path?query#fragment");
+  GURL sanitized_url2("https://www.google.com:8080/");
+
+  ProxyInfo info2;
+  TestCompletionCallback callback2;
+  rv = service.ResolveProxy(url2, std::string(), LOAD_NORMAL, &info2,
+                            callback2.callback(), nullptr, nullptr,
+                            BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  ASSERT_EQ(1u, resolver.pending_requests().size());
+  // Ensure that the sanitized URL was sent to the proxy resolver (not the full
+  // URL).
+  EXPECT_EQ(sanitized_url2, resolver.pending_requests()[0]->url());
+
+  // Complete the request.
+  resolver.pending_requests()[0]->results()->UsePacString("DIRECT");
+  resolver.pending_requests()[0]->CompleteNow(OK);
+  EXPECT_EQ(OK, callback2.WaitForResult());
+  EXPECT_TRUE(info2.is_direct());
+
+  // Request 3 ----------------------------------------------------------------
+
+  // Change the sanitization policy.
+  service.set_sanitize_url_policy(ProxyService::SanitizeUrlPolicy::UNSAFE);
+
+  // Start another request. This is the same as request 2, however the expected
+  // sanitized URL is different (includes the path and query).
+  GURL url3 = url2;
+  GURL sanitized_url3("https://www.google.com:8080/path?query");
+
+  ProxyInfo info3;
+  TestCompletionCallback callback3;
+  rv = service.ResolveProxy(url3, std::string(), LOAD_NORMAL, &info3,
+                            callback3.callback(), nullptr, nullptr,
+                            BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  ASSERT_EQ(1u, resolver.pending_requests().size());
+  // Ensure that the sanitized URL was sent to the proxy resolver (not the full
+  // URL).
+  EXPECT_EQ(sanitized_url3, resolver.pending_requests()[0]->url());
+
+  // Complete the request.
+  resolver.pending_requests()[0]->results()->UsePacString("DIRECT");
+  resolver.pending_requests()[0]->CompleteNow(OK);
+  EXPECT_EQ(OK, callback3.WaitForResult());
+  EXPECT_TRUE(info3.is_direct());
+}
+
+// Tests SanitizeUrlForPacScript() using input URLs that have a
+// non-cryptographic scheme (i.e. http://). The sanitized result is consistent
+// regardless of the stripping mode selected.
+TEST_F(ProxyServiceTest, SanitizeUrlForPacScriptNonCryptographic) {
+  const struct {
+    const char* raw_url;
+    const char* sanitized_url;
+  } kTests[] = {
+      // Embedded identity is stripped.
+      {
+          "http://foo:bar@example.com/", "http://example.com/",
+      },
+      {
+          "ftp://foo:bar@example.com/", "ftp://example.com/",

[mmenke, 2016/05/20 17:40:23]

Seems like we should have a test with an FTP URL with a path.

[eroman, 2016/05/20 21:23:53]

Done.

+      },
+      // Reference fragment is stripped.
+      {
+          "http://example.com/blah#hello", "http://example.com/blah",
+      },
+      // Query parameters are NOT stripped.
+      {
+          "http://example.com/foo/bar/baz?hello",
+          "http://example.com/foo/bar/baz?hello",
+      },
+      // Fragment is stripped, but path and query are left intact.
+      {
+          "http://foo:bar@example.com/foo/bar/baz?hello#sigh",
+          "http://example.com/foo/bar/baz?hello",
+      },
+      // Port numbers are not affected.
+      {
+          "http://example.com:88/hi", "http://example.com:88/hi",
+      },
+  };
+
+  for (const auto& test : kTests) {
+    // The result of SanitizeUrlForPacScript() is the same regardless of the
+    // second parameter (sanitization mode), since the input URLs do not use a
+    // cryptographic scheme.
+    GURL raw_url(test.raw_url);
+    ASSERT_TRUE(raw_url.is_valid());
+    EXPECT_FALSE(raw_url.SchemeIsCryptographic());
+
+    EXPECT_EQ(GURL(test.sanitized_url),
+              ProxyService::SanitizeUrl(
+                  raw_url, ProxyService::SanitizeUrlPolicy::UNSAFE));
+
+    EXPECT_EQ(GURL(test.sanitized_url),
+              ProxyService::SanitizeUrl(raw_url,
+                                        ProxyService::SanitizeUrlPolicy::SAFE));
+  }
+}
+
+// Tests SanitizeUrlForPacScript() using input URLs that have a cryptographic
+// schemes (i.e. https://). The sanitized result differs depending
+// on the sanitization mode chosen.
+TEST_F(ProxyServiceTest, SanitizeUrlForPacScriptCryptographic) {
+  const struct {
+    // Input URL.
+    const char* raw_url;
+
+    // Output URL when stripping of cryptographic URLs is disabled.
+    const char* sanitized_url_unstripped;
+
+    // Output URL when stripping of cryptographic URLs is enabled.
+    const char* sanitized_url;
+  } kTests[] = {
+      // Embedded identity is always stripped.
+      {
+          "https://foo:bar@example.com/", "https://example.com/",
+          "https://example.com/",
+      },
+      // Fragments are always stripped, but stripping path is conditional on the
+      // mode.
+      {
+          "https://example.com/blah#hello", "https://example.com/blah",
+          "https://example.com/",
+      },
+      // Stripping the query is conditional on the mode.
+      {
+          "https://example.com/foo/bar/baz?hello",
+          "https://example.com/foo/bar/baz?hello", "https://example.com/",

[mmenke, 2016/05/20 17:40:23]

Suggest no path on this one (Well...a path of "/", to be technical).  Next test
checks path + query.  While GURL no doubt has plenty of testing, feel that no
path here gives us marginally more test coverage here.

[eroman, 2016/05/20 21:23:53]

Done.

+      },
+      // The embedded identity and fragment is always stripped, however path and
+      // query are conditional on the stripping mode.
+      {
+          "https://foo:bar@example.com/foo/bar/baz?hello#sigh",
+          "https://example.com/foo/bar/baz?hello", "https://example.com/",
+      },
+      // The URL's port should not be stripped.
+      {
+          "https://example.com:88/hi", "https://example.com:88/hi",
+          "https://example.com:88/",
+      },

[mmenke, 2016/05/20 17:40:23]

Suggest also having a wss test.  Could add a ws test above as well, though I'm
less concerned about that.

[eroman, 2016/05/20 21:23:53]

Done.

+  };
+
+  for (const auto& test : kTests) {
+    GURL raw_url(test.raw_url);
+    ASSERT_TRUE(raw_url.is_valid());
+    EXPECT_TRUE(raw_url.SchemeIsCryptographic());
+
+    EXPECT_EQ(GURL(test.sanitized_url_unstripped),
+              ProxyService::SanitizeUrl(
+                  raw_url, ProxyService::SanitizeUrlPolicy::UNSAFE));
+
+    EXPECT_EQ(GURL(test.sanitized_url),
+              ProxyService::SanitizeUrl(raw_url,
+                                        ProxyService::SanitizeUrlPolicy::SAFE));
+  }
+}
+
 }  // namespace net