X-Chrome-Connected is stripped when it should not be in headers.

chrome/browser/signin/chrome_signin_helper.cc

Index: chrome/browser/signin/chrome_signin_helper.cc
diff --git a/chrome/browser/signin/chrome_signin_helper.cc b/chrome/browser/signin/chrome_signin_helper.cc
index 6787db2f6026f1d4c358474b069de8e7e694bf78..613dfd09cefd227c337acee477690db9316005f3 100644
--- a/chrome/browser/signin/chrome_signin_helper.cc
+++ b/chrome/browser/signin/chrome_signin_helper.cc
@@ -118,7 +118,7 @@ ManageAccountsParams BuildManageAccountsParamsHelper(net::URLRequest* request,
 
 }  // namespace
 
-bool AppendMirrorRequestHeaderHelper(net::URLRequest* request,
+bool FixMirrorRequestHeaderHelper(net::URLRequest* request,

[eroman, 2016/08/23 20:02:36]

style -- fix indentation

[Ramin Halavati, 2016/08/26 17:04:31]

Done.

                                      const GURL& redirect_url,
                                      ProfileIOData* io_data,
                                      int child_id,
@@ -148,9 +148,14 @@ bool AppendMirrorRequestHeaderHelper(net::URLRequest* request,
     profile_mode_mask |= PROFILE_MODE_INCOGNITO_DISABLED;
   }
 
-  return AppendMirrorRequestHeaderIfPossible(
-      request, redirect_url, io_data->google_services_account_id()->GetValue(),
-      io_data->GetCookieSettings(), profile_mode_mask);
+  if (AppendMirrorRequestHeaderIfPossible(
+        request, redirect_url,
+        io_data->google_services_account_id()->GetValue(),
+        io_data->GetCookieSettings(), profile_mode_mask))

[mmenke, 2016/08/23 20:34:16]

AppendMirrorRequestHeaderIfPossible seems problematic.  It doesn't look like it
checks for HTTPS, and relies on HSTS being enabled for Google domains...Which
hopefully is true, but this code should not rely on it.  That also means it
exposes this information to HTTP-only extensions, even if all google domains are
HSTS, which seems weird.

A third party ServiceWorker can also serve requests to http://www.google.com
with responses from http://www.evil.com...  I believe we won't expose headers
for the request in that case, either to the ServiceWorker or over the network
but it still makes me very nervous.

[mmenke, 2016/08/23 20:44:52]

And this also assumes all Google.* TLDs are owned by Google, which also seems
like a problem.  If someone registers google.mil, or just hijacks request to it,
for instance, it looks like they could get this information.

[Ramin Halavati, 2016/08/26 17:04:31]

I have added a test for content::IsOriginSecure and don't generate a header if
the origin is not secure.

[Ramin Halavati, 2016/08/26 17:04:31]

I think this is a bigger issue of google_util::IsGoogleDomainUrl. I think that
multiple services depend on this function working properly. I think we should
consider this out of scope for this CL. How would you suggest to fix this?

[mmenke, 2016/08/26 17:42:20]

My feeling is that we should not rely on it for any security or privacy related
checks.  I don't know what project mirror is, but I don't think we should just
be arbitrarily adding privacy-relevant headers to requests based on requested
URLs.  It seems like a problematic design to me.

I do agree that this seems like a more general problem with IsGoogleDomainUrl
itself, even with an HTTPS check added to it.

+    return true;
+
+  request->RemoveRequestHeaderByName(signin::kChromeConnectedHeader);

[eroman, 2016/08/23 20:02:36]

This doesn't seem right.

This code is called for all requests flowing through resource dispatcher host.
Which means that any other consumer that tries to set their own header named
X-Chrome-Connected will have it stripped at this layer. At best that is
confusing, at worst that is a bug which prevents using this header name on
non-google domains.

[Ramin Halavati, 2016/08/26 17:04:31]

It's updated so that it is removed only when it is redirecting from a domain
which is eligible for having this header, to one that is not eligible.

+  return false;
 }
 
 void ProcessMirrorResponseHeaderIfExists(net::URLRequest* request,