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

chrome/browser/signin/chrome_signin_helper.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/signin/chrome_signin_helper.h"
 
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/profiles/profile_io_data.h"
@@ skipping 100 matching lines @@
     ManageAccountsParams empty_params;
     empty_params.service_type = GAIA_SERVICE_TYPE_NONE;
     return empty_params;
   }
 
   return BuildManageAccountsParamsIfExists(request, io_data->IsOffTheRecord());
 }
 
 }  // namespace
 
-bool AppendMirrorRequestHeaderHelper(net::URLRequest* request,
-                                     const GURL& redirect_url,
-                                     ProfileIOData* io_data,
-                                     int child_id,
-                                     int route_id) {
+bool FixMirrorRequestHeaderHelper(net::URLRequest* request,
+                                  const GURL& redirect_url,
+                                  ProfileIOData* io_data,
+                                  int child_id,
+                                  int route_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
   if (io_data->IsOffTheRecord())
     return false;
 
 #if !defined(OS_ANDROID)
   extensions::WebViewRendererState::WebViewInfo webview_info;
   bool is_guest = extensions::WebViewRendererState::GetInstance()->GetInfo(
       child_id, route_id, &webview_info);
   // Do not set the x-chrome-connected header on requests from a native signin
   // webview, as identified by an empty extension id which means the webview is
   // embedded in a webui page, otherwise user may end up with a blank page as
   // gaia uses the header to decide whether it returns 204 for certain end
   // points.
   if (is_guest && webview_info.owner_host.empty())
     return false;
 #endif  // !defined(OS_ANDROID)
 
   int profile_mode_mask = PROFILE_MODE_DEFAULT;
   if (io_data->incognito_availibility()->GetValue() ==
           IncognitoModePrefs::DISABLED ||
       IncognitoModePrefs::ArePlatformParentalControlsEnabled()) {
     profile_mode_mask |= PROFILE_MODE_INCOGNITO_DISABLED;
   }
 

[mmenke, 2016/08/30 19:12:26]

Think everything below this point may belong in a single method in
signin_header_helper.h - seems like the code that handles
AppendMirrorRequestHeaderIfPossible belongs in the same file that handles
removing the header on redirect as well.

[Ramin Halavati, 2016/09/01 10:41:44]

Done.

The removed part is now moved to signin_header_helper.cc and the function
AppendMirrorRequestHeaderIfPossible is renamed to
AppendOrRemoveMirrorRequestHeaderIfPossible. Do you suggest that this one would
also become void?

-  return AppendMirrorRequestHeaderIfPossible(
-      request, redirect_url, io_data->google_services_account_id()->GetValue(),
-      io_data->GetCookieSettings(), profile_mode_mask);
+  // If new url is eligible to have the header, return true, otherwise if
+  // redirecting to another site and x-chrome-header exists, and the redirected

[mmenke, 2016/08/30 19:12:26]

x-chrome-header -> x-chrome-connected header (Or x-chrome header, I suppose)

[Ramin Halavati, 2016/09/01 10:41:44]

Done.

+  // site is not illigible and current site was illigible, remove it.

[mmenke, 2016/08/30 19:12:27]

illigible -> eligible (x2)

[Ramin Halavati, 2016/09/01 10:41:44]

Done.

+  if (AppendMirrorRequestHeaderIfPossible(
+          request, redirect_url,
+          io_data->google_services_account_id()->GetValue(),
+          io_data->GetCookieSettings(), profile_mode_mask)) {
+    return true;

[mmenke, 2016/08/30 19:12:27]

The return value isn't used, and it isn't clear what it means.  Let's just make
this method void.

[Ramin Halavati, 2016/09/01 10:41:44]

Done.

+  } else {

[mmenke, 2016/08/30 19:12:27]

Since you have the early return, this else isn't needed.

[Ramin Halavati, 2016/09/01 10:41:44]

Done.

+    if (!redirect_url.is_empty() &&  // redirecting
+        request->extra_request_headers().HasHeader(
+            signin::kChromeConnectedHeader) &&  // x-chrome-header exists

[mmenke, 2016/08/30 19:12:27]

Rather than inline comments, suggest writing out description in full sentences,
just before the if.

i.e. "If the request is being redirected from Google URL to a non-Google URL,
remove the x-chrome-connected header, if present."

[Ramin Halavati, 2016/09/01 10:41:44]

Done.

+        signin::IsUrlElligibleForXChromeConnectedHeader(request->url()) &&
+        !signin::IsUrlElligibleForXChromeConnectedHeader(redirect_url))
+      request->RemoveRequestHeaderByName(signin::kChromeConnectedHeader);

[mmenke, 2016/08/30 19:12:27]

Use braces when an if condition takes multiple lines.

[Ramin Halavati, 2016/09/01 10:41:44]

Done.

+  }
+  return false;
 }
 
 void ProcessMirrorResponseHeaderIfExists(net::URLRequest* request,
                                          ProfileIOData* io_data,
                                          int child_id,
                                          int route_id) {
   ManageAccountsParams params =
       BuildManageAccountsParamsHelper(request, io_data);
   if (params.service_type == GAIA_SERVICE_TYPE_NONE)
     return;
 
   params.child_id = child_id;
   params.route_id = route_id;
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(ProcessMirrorHeaderUIThread, child_id, route_id, params));
 }
 
 }  // namespace signin