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

net/proxy/proxy_service.cc

Index: net/proxy/proxy_service.cc
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index 5373a34a9ce73c52baf3ae719abcebbc94358388..baa81047db4aeec383ef85099879bec08a74842c 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -349,6 +349,23 @@ class UnsetProxyConfigService : public ProxyConfigService {
 
 }  // namespace
 
+GURL SanitizeUrlForPacScript(const GURL& url,
+                             SanitizeUrlForPacScriptPolicy policy) {
+  DCHECK(url.is_valid());
+  GURL::Replacements replacements;
+  replacements.ClearUsername();
+  replacements.ClearPassword();
+  replacements.ClearRef();
+
+  if (policy == SanitizeUrlForPacScriptPolicy::SAFE &&
+      url.SchemeIsCryptographic()) {
+    replacements.ClearPath();
+    replacements.ClearQuery();
+  }

[mmenke, 2016/05/19 22:33:23]

An alternative approach would be to do:

if (policy == SanitizeUrlForPacScriptPolicy::SAFE && 
url.SchemeIsCryptographic()) {
  return url.GetOrigin();
}

GURL::Replacements replacements;
...

Nice thing about that approach is it emphasizes this is indeed just an origin. 
Downside is it de-emphasizes just what the differences are between the two
paths.

[eroman, 2016/05/19 23:26:05]

I added a TODO to explore that, will follow-up.

There are some differences I would like to better understand between the two
approaches -- GetOrigin() short circuits on non-standard URL schemes and
completely clears the URL. I am not sure if that would come into play here, but
would like to play it safe by using the existing pattern of code.

+
+  return url.ReplaceComponents(replacements);
+}
+
 // ProxyService::InitProxyResolver --------------------------------------------
 
 // This glues together two asynchronous steps:
@@ -1050,9 +1067,11 @@ int ProxyService::ResolveProxyHelper(const GURL& raw_url,
   if (current_state_ == STATE_NONE)
     ApplyProxyConfigIfAvailable();
 
-  // Strip away any reference fragments and the username/password, as they
-  // are not relevant to proxy resolution.
-  GURL url = SimplifyUrlForRequest(raw_url);
+  // Sanitize the URL before passing it on to the proxy resolver (i.e. PAC
+  // script). The goal is to remove sensitive data (like embedded user names
+  // and password), and local data (i.e. reference fragment).
+  GURL url =
+      SanitizeUrlForPacScript(raw_url, sanitize_url_for_pac_script_policy_);

[mmenke, 2016/05/19 22:33:23]

Should we only do this when we create the PacRequest?  Seems weird to call it
SanitizeForPac, and then send it to TryToCompleteSynchronously, which doesn't
actually run a PAC script.

[eroman, 2016/05/19 23:26:05]

I hope to merge this CL to M52 so don't want to change the existing code
organization too much.

I have ideas on improving this but will not be tackling them here.

(There are several refactoring opportunities around layering issues in this file
-- too many PAC concepts leak into ProxyService. Also the distinction between
ProxyService, ProxyConfigService, and ProxyResolver is not great.)

 
   // Check if the request can be completed right away. (This is the case when
   // using a direct connection for example).