Only attach GAIA ID to Mirror header/cookie for whitelisted domain.

components/signin/core/browser/signin_header_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 "components/signin/core/browser/signin_header_helper.h"
 
 #include <stddef.h>
 
 #include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "components/content_settings/core/browser/cookie_settings.h"
 #include "components/google/core/browser/google_util.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/base/escape.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
@@ skipping 16 matching lines @@
 
 bool IsDriveOrigin(const GURL& url) {
   if (!url.SchemeIsCryptographic())
     return false;
 
   const GURL kGoogleDriveURL("https://drive.google.com");
   const GURL kGoogleDocsURL("https://docs.google.com");
   return url == kGoogleDriveURL || url == kGoogleDocsURL;
 }
 
+bool IsUrlEligibleToIncludeGaiaId(const GURL& url, bool is_header_request) {
+  if (is_header_request) {
+    // GAIA Id is only necessary for Drive. Don't set it otherwise.
+    return IsDriveOrigin(url);

[battre, 2016/09/20 09:01:55]

Just to be sure: I think you are passing the url here, not the origin. Are you
sure that all of this works as expected?

[bzanotti, 2016/09/20 14:49:28]

That's a good point, fixed in the new CL.

+  }
+
+  // Cookie requests don't have the granularity to only include the GAIA Id for
+  // Drive origin. Set it on all google.com instead.
+  if (!url.SchemeIsCryptographic())
+    return false;
+
+  const GURL kGoogleDotComURL("https://google.com");
+  return url == kGoogleDotComURL;

[battre, 2016/09/20 09:01:55]

I think this would be false for https://www.google.com, right? Is that intended?

[bzanotti, 2016/09/20 14:49:28]

I cheated a little bit here, because I know that the only code using this will
actually use "https://google.com". I have fixed it to actually extract the
domain and check that it is google.com.

+}
+
 // Determines the service type that has been passed from GAIA in the header.
 signin::GAIAServiceType GetGAIAServiceTypeFromHeader(
     const std::string& header_value) {
   if (header_value == "SIGNOUT")
     return signin::GAIA_SERVICE_TYPE_SIGNOUT;
   else if (header_value == "INCOGNITO")
     return signin::GAIA_SERVICE_TYPE_INCOGNITO;
   else if (header_value == "ADDSESSION")
     return signin::GAIA_SERVICE_TYPE_ADDSESSION;
   else if (header_value == "REAUTH")
@@ skipping 21 matching lines @@
     }
     dictionary[field.substr(0, delim).as_string()] = net::UnescapeURLComponent(
         field.substr(delim + 1).as_string(),
         net::UnescapeRule::PATH_SEPARATORS |
             net::UnescapeRule::URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS);
   }
   return dictionary;
 }
 
 std::string BuildMirrorRequestIfPossible(
-    const char* pattern,
+    bool is_header_request,
     const GURL& url,
     const std::string& account_id,
     const content_settings::CookieSettings* cookie_settings,
     int profile_mode_mask) {
   if (account_id.empty())
     return std::string();
 
   // If signin cookies are not allowed, don't add the header.
   if (!signin::SettingsAllowSigninCookies(cookie_settings)) {
     return std::string();
   }
 
   // Check if url is elligible for the header.
   if (!signin::IsUrlEligibleForXChromeConnectedHeader(url))
     return std::string();
 
-  return base::StringPrintf(
-      pattern, kGaiaIdAttrName, account_id.c_str(), kProfileModeAttrName,
-      base::IntToString(profile_mode_mask).c_str(),
-      kEnableAccountConsistencyAttrName,
-      switches::IsEnableAccountConsistency() ? "true" : "false");
+  std::vector<std::string> parts;
+  if (IsUrlEligibleToIncludeGaiaId(url, is_header_request)) {
+    // Only google.com requires the GAIA ID, don't send it to other domains.

[battre, 2016/09/20 09:01:55]

I think this is inconsistent with sending it to drive.google.com and
docs.google.com as well, right?

[bzanotti, 2016/09/20 14:49:28]

Correct, I forgot to update the comment.

+    parts.push_back(
+        base::StringPrintf("%s=%s", kGaiaIdAttrName, account_id.c_str()));
+  }
+  parts.push_back(
+      base::StringPrintf("%s=%s", kProfileModeAttrName,
+                         base::IntToString(profile_mode_mask).c_str()));
+  parts.push_back(base::StringPrintf(
+      "%s=%s", kEnableAccountConsistencyAttrName,
+      switches::IsEnableAccountConsistency() ? "true" : "false"));
+
+  return base::JoinString(parts, is_header_request ? "," : ":");
 }
 
 }  // namespace
 
 namespace signin {
 
 extern const char kChromeConnectedHeader[] = "X-Chrome-Connected";
 
 ManageAccountsParams::ManageAccountsParams()
     : service_type(GAIA_SERVICE_TYPE_NONE),
@@ skipping 17 matching lines @@
   return cookie_settings &&
          cookie_settings->IsSettingCookieAllowed(gaia_url, gaia_url) &&
          cookie_settings->IsSettingCookieAllowed(google_url, google_url);
 }
 
 std::string BuildMirrorRequestCookieIfPossible(
     const GURL& url,
     const std::string& account_id,
     const content_settings::CookieSettings* cookie_settings,
     int profile_mode_mask) {
-  return BuildMirrorRequestIfPossible("%s=%s:%s=%s:%s=%s", url, account_id,
-                                      cookie_settings, profile_mode_mask);
+  return BuildMirrorRequestIfPossible(false /* is_header_request */, url,
+                                      account_id, cookie_settings,
+                                      profile_mode_mask);
+}
+
+bool AppendOrRemoveMirrorRequestHeaderIfPossible(
+    net::URLRequest* request,
+    const GURL& redirect_url,
+    const std::string& account_id,
+    const content_settings::CookieSettings* cookie_settings,
+    int profile_mode_mask) {
+  const GURL& url = redirect_url.is_empty() ? request->url() : redirect_url;
+  std::string header_value = BuildMirrorRequestIfPossible(
+      true /* is_header_request */, url, account_id, cookie_settings,
+      profile_mode_mask);
+  if (header_value.empty()) {
+    // If the request is being redirected, and it has the x-chrome-connected
+    // header, and current url is a Google URL, and the redirected one is not,
+    // remove the header.
+    if (!redirect_url.is_empty() &&
+        request->extra_request_headers().HasHeader(
+            signin::kChromeConnectedHeader) &&
+        signin::IsUrlEligibleForXChromeConnectedHeader(request->url()) &&
+        !signin::IsUrlEligibleForXChromeConnectedHeader(redirect_url)) {
+      request->RemoveRequestHeaderByName(signin::kChromeConnectedHeader);
+    }
+    return false;
   }
-
-  bool AppendOrRemoveMirrorRequestHeaderIfPossible(
-      net::URLRequest* request,
-      const GURL& redirect_url,
-      const std::string& account_id,
-      const content_settings::CookieSettings* cookie_settings,
-      int profile_mode_mask) {
-    const GURL& url = redirect_url.is_empty() ? request->url() : redirect_url;
-    std::string header_value =
-        BuildMirrorRequestIfPossible("%s=%s,%s=%s,%s=%s", url, account_id,
-                                     cookie_settings, profile_mode_mask);
-    if (header_value.empty()) {
-      // If the request is being redirected, and it has the x-chrome-connected
-      // header, and current url is a Google URL, and the redirected one is not,
-      // remove the header.
-      if (!redirect_url.is_empty() &&
-          request->extra_request_headers().HasHeader(
-              signin::kChromeConnectedHeader) &&
-          signin::IsUrlEligibleForXChromeConnectedHeader(request->url()) &&
-          !signin::IsUrlEligibleForXChromeConnectedHeader(redirect_url)) {
-        request->RemoveRequestHeaderByName(signin::kChromeConnectedHeader);
-      }
-      return false;
-    }
-    request->SetExtraRequestHeaderByName(kChromeConnectedHeader, header_value,
-                                         false);
-    return true;
+  request->SetExtraRequestHeaderByName(kChromeConnectedHeader, header_value,
+                                       false);
+  return true;
 }
 
 ManageAccountsParams BuildManageAccountsParams(
     const std::string& header_value) {
   signin::ManageAccountsParams params;
   MirrorResponseHeaderDictionary header_dictionary =
       ParseMirrorResponseHeader(header_value);
   MirrorResponseHeaderDictionary::const_iterator it = header_dictionary.begin();
   for (; it != header_dictionary.end(); ++it) {
     const std::string key_name(it->first);
@@ skipping 56 matching lines @@
                             url, google_util::ALLOW_SUBDOMAIN,
                             google_util::DISALLOW_NON_STANDARD_PORTS) ||
                         google_util::IsYoutubeDomainUrl(
                             url, google_util::ALLOW_SUBDOMAIN,
                             google_util::DISALLOW_NON_STANDARD_PORTS));
   return is_google_url || IsDriveOrigin(origin) ||
          gaia::IsGaiaSignonRealm(origin);
 }
 
 }  // namespace signin