From 5ecc573cd0c95e8d5513a14836adb41d7c9f31eb Mon Sep 17 00:00:00 2001
From: Takashi Toyoshima <[email protected]>
Date: Tue, 28 Jul 2026 22:40:25 -0700
Subject: [PATCH] Improve CloudApProviderWin to manage requests in a queue

This refactoring implements a URL-keyed request merging mechanism:
- Grouping concurrent auth requests by their precise target GURL.
- Restricting systemic IDP/COM token fetching operations to run exactly once
  per unique URL.
- Executing an atomic token broadcast that distributes copied auth headers
  strictly and exclusively to matching requests.
- Enforcing a strict browser-wide ceiling (capped at 100 pending requests)
  to protect against systemic memory-exhaustion/DoS attacks.

Bug: 520389619
Change-Id: I76725a71eb08fb84316bb68d311f6851081d3760
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7924786
Commit-Queue: Takashi Toyoshima <[email protected]>
Reviewed-by: Igor Ruvinov <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1670002}
---

diff --git a/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win.cc b/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win.cc
index ebaea83f..4d9b090 100644
--- a/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win.cc
+++ b/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win.cc
@@ -437,16 +437,43 @@
 void CloudApProviderWin::GetData(
     const GURL& url,
     PlatformAuthProviderManager::GetDataCallback callback) {
-  get_data_subscriptions_.push_back(
-      on_get_data_callback_list_.Add(std::move(callback)));
-  if (!base::ThreadPool::CreateCOMSTATaskRunner(
-           {base::TaskPriority::USER_BLOCKING,
-            base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN, base::MayBlock()})
-           ->PostTaskAndReplyWithResult(
-               FROM_HERE, base::BindOnce(&GetAuthData, url),
-               base::BindOnce(&CloudApProviderWin::OnGetDataCallback,
-                              base::Unretained(this)))) {
-    OnGetDataCallback(net::HttpRequestHeaders());
+  if (!base::FeatureList::IsEnabled(
+          enterprise_auth::kCloudApAuthDataQueueing)) {
+    get_data_subscriptions_.push_back(
+        on_get_data_callback_list_.Add(std::move(callback)));
+    if (!base::ThreadPool::CreateCOMSTATaskRunner(
+             {base::TaskPriority::USER_BLOCKING,
+              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN,
+              base::MayBlock()})
+             ->PostTaskAndReplyWithResult(
+                 FROM_HERE, base::BindOnce(&GetAuthData, url),
+                 base::BindOnce(&CloudApProviderWin::OnGetDataCallback,
+                                base::Unretained(this)))) {
+      OnGetDataCallback(net::HttpRequestHeaders());
+    }
+    return;
+  }
+
+  if (total_enqueued_requests_ >= kMaxQueueSize) {
+    VLOG_POLICY(1, EXTENSIBLE_SSO)
+        << "[CloudAPAuthEnabled] Enqueued requests limit (" << kMaxQueueSize
+        << ") exceeded. Failing request.";
+    base::UmaHistogramBoolean(
+        "Enterprise.PlatformAuth.GetAuthData.QueueOverflow", true);
+    // Global queue limit exceeded, fail the request asynchronously.
+    base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
+        FROM_HERE,
+        base::BindOnce(std::move(callback), net::HttpRequestHeaders()));
+    return;
+  }
+
+  total_enqueued_requests_++;
+  auto& callbacks = request_queues_[url];
+  bool is_first_request = callbacks.empty();
+  callbacks.push_back(std::move(callback));
+
+  if (is_first_request) {
+    StartFetch(url);
   }
 }
 
@@ -472,4 +499,38 @@
   ParseCookieInfo(cookie_info, cookie_info_count, auth_headers);
 }
 
+void CloudApProviderWin::StartFetch(const GURL& url) {
+  if (!base::ThreadPool::CreateCOMSTATaskRunner(
+           {base::TaskPriority::USER_BLOCKING,
+            base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN, base::MayBlock()})
+           ->PostTaskAndReplyWithResult(
+               FROM_HERE, base::BindOnce(&GetAuthData, url),
+               base::BindOnce(&CloudApProviderWin::OnFetchCompleted,
+                              weak_factory_.GetWeakPtr(), url))) {
+    OnFetchCompleted(url, net::HttpRequestHeaders());
+  }
+}
+
+void CloudApProviderWin::OnFetchCompleted(
+    const GURL& url,
+    net::HttpRequestHeaders auth_headers) {
+  auto it = request_queues_.find(url);
+  CHECK(it != request_queues_.end());
+  CHECK(!it->second.empty());
+
+  std::vector<PlatformAuthProviderManager::GetDataCallback> callbacks =
+      std::move(it->second);
+  request_queues_.erase(it);
+
+  CHECK_GE(total_enqueued_requests_, callbacks.size());
+  total_enqueued_requests_ -= callbacks.size();
+
+  for (size_t i = 0; i < callbacks.size(); ++i) {
+    auto callback = std::move(callbacks[i]);
+    net::HttpRequestHeaders headers =
+        (i + 1 == callbacks.size()) ? std::move(auth_headers) : auth_headers;
+    std::move(callback).Run(std::move(headers));
+  }
+}
+
 }  // namespace enterprise_auth
diff --git a/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win.h b/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win.h
index dff86da..bcc9696f 100644
--- a/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win.h
+++ b/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win.h
@@ -5,11 +5,13 @@
 #ifndef CHROME_BROWSER_ENTERPRISE_PLATFORM_AUTH_CLOUD_AP_PROVIDER_WIN_H_
 #define CHROME_BROWSER_ENTERPRISE_PLATFORM_AUTH_CLOUD_AP_PROVIDER_WIN_H_
 
+#include <map>
 #include <optional>
 #include <vector>
 
 #include "base/callback_list.h"
 #include "base/gtest_prod_util.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/enterprise/platform_auth/platform_auth_provider.h"
 
 class GURL;
@@ -52,9 +54,13 @@
   FRIEND_TEST_ALL_PREFIXES(CloudApProviderWinTest,
                            ParseCookieInfo_HeaderFeatureEnabled);
 
-  // Runs the stored callbacks using the provided auth headers.
+  // Runs the stored callbacks using the provided auth headers (legacy
+  // non-queuing implementation).
   void OnGetDataCallback(net::HttpRequestHeaders auth_headers);
 
+  // Maximum total number of pending data requests allowed across all URLs.
+  static constexpr size_t kMaxQueueSize = 100;
+
   // Overrides support detection with `level` if it has a value, or resets the
   // override if not.
   static void SetSupportLevelForTesting(std::optional<SupportLevel> level);
@@ -64,14 +70,30 @@
                                  const DWORD cookie_info_count,
                                  net::HttpRequestHeaders& auth_headers);
 
-  // List of callbacks to run when auth data is received.
+  // List of callbacks to run when auth data is received (legacy non-queuing
+  // implementation).
   using GetDataCallbackList =
       base::OnceCallbackList<void(net::HttpRequestHeaders)>;
   GetDataCallbackList on_get_data_callback_list_;
 
   // Subscriptions for auth data requests. Guarantees that the corresponding
-  // callbacks are run on destruction.
+  // callbacks are run on destruction (legacy non-queuing implementation).
   std::vector<base::CallbackListSubscription> get_data_subscriptions_;
+
+  // Starts a background task to fetch auth data for `url`.
+  void StartFetch(const GURL& url);
+
+  // Handles completion of a background fetch for `url`.
+  void OnFetchCompleted(const GURL& url, net::HttpRequestHeaders auth_headers);
+
+  std::map<GURL, std::vector<PlatformAuthProviderManager::GetDataCallback>>
+      request_queues_;
+
+  // Total number of pending requests stored across all vectors in
+  // `request_queues_`.
+  size_t total_enqueued_requests_ = 0;
+
+  base::WeakPtrFactory<CloudApProviderWin> weak_factory_{this};
 };
 
 }  // namespace enterprise_auth
diff --git a/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win_unittest.cc b/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win_unittest.cc
index f885dfc..bc3da91 100644
--- a/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win_unittest.cc
+++ b/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win_unittest.cc
@@ -9,9 +9,12 @@
 #include <memory>
 #include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"

diff --git a/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win_unittest.cc b/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win_unittest.cc
index f885dfc..bc3da91 100644
--- a/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win_unittest.cc
+++ b/chrome/browser/enterprise/platform_auth/cloud_ap_provider_win_unittest.cc
@@ -9,9 +9,12 @@
 #include <memory>
 #include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
@@ -42,6 +45,21 @@
 
 class CloudApProviderWinTest : public ::testing::Test {
  protected:
+  static constexpr size_t kMaxQueueSize = CloudApProviderWin::kMaxQueueSize;
+
+  static size_t GetTotalFetches(const base::HistogramTester& tester) {
+    size_t total = 0;
+    for (const auto& bucket : tester.GetAllSamples(
+             "Enterprise.PlatformAuth.GetAuthData.SuccessTime")) {
+      total += bucket.count;
+    }
+    for (const auto& bucket : tester.GetAllSamples(
+             "Enterprise.PlatformAuth.GetAuthData.FailureTime")) {
+      total += bucket.count;
+    }
+    return total;
+  }
+
   ~CloudApProviderWinTest() override {
     // Clear an override of the join type made by any test.
     CloudApProviderWin::SetSupportLevelForTesting(std::nullopt);
@@ -198,4 +216,155 @@
             base::WideToASCII(L"data"));
 }
 
+TEST_F(CloudApProviderWinTest, GetDataQueueAndOverflow) {
+  CloudApProviderWin provider;
+  GURL url("https://test.com");
+  base::HistogramTester histogram_tester;
+
+  int success_count = 0;
+  int overflow_count = 0;
+  base::RunLoop run_loop;
+  base::RepeatingClosure barrier_closure =
+      base::BarrierClosure(kMaxQueueSize + 1, run_loop.QuitClosure());
+
+  // Queue up to the max size (100 items).
+  for (size_t i = 0; i < kMaxQueueSize; ++i) {
+    provider.GetData(url,
+                     base::BindOnce(
+                         [](int* success_count, base::RepeatingClosure barrier,
+                            net::HttpRequestHeaders headers) {
+                           (*success_count)++;
+                           barrier.Run();
+                         },
+                         &success_count, barrier_closure));
+  }
+
+  // The 101st request should overflow.
+  provider.GetData(url,
+                   base::BindOnce(
+                       [](int* overflow_count, base::RepeatingClosure barrier,
+                          net::HttpRequestHeaders headers) {
+                         // Overflow requests are completed with empty headers.
+                         EXPECT_TRUE(headers.IsEmpty());
+                         (*overflow_count)++;
+                         barrier.Run();
+                       },
+                       &overflow_count, barrier_closure));
+
+  run_loop.Run();
+
+  EXPECT_EQ(success_count, static_cast<int>(kMaxQueueSize));
+  EXPECT_EQ(overflow_count, 1);
+
+  // Check that the queue overflow histogram recorded 1 sample.
+  histogram_tester.ExpectBucketCount(
+      "Enterprise.PlatformAuth.GetAuthData.QueueOverflow", true, 1);
+
+  // Exactly 1 background fetch should have occurred for the 100 merged
+  // requests.
+  EXPECT_EQ(GetTotalFetches(histogram_tester), 1u);
+}
+
+TEST_F(CloudApProviderWinTest, GetDataSameUrl) {
+  CloudApProviderWin provider;
+  GURL url("https://test.com");
+  base::HistogramTester histogram_tester;
+
+  int count = 0;
+  base::RunLoop run_loop;
+  base::RepeatingClosure barrier_closure =
+      base::BarrierClosure(5, run_loop.QuitClosure());
+
+  for (int i = 0; i < 5; ++i) {
+    provider.GetData(url, base::BindOnce(
+                              [](int* count, base::RepeatingClosure barrier,
+                                 net::HttpRequestHeaders) {
+                                (*count)++;
+                                barrier.Run();
+                              },
+                              &count, barrier_closure));
+  }
+
+  run_loop.Run();
+
+  EXPECT_EQ(count, 5);
+  // Only 1 background fetch should have occurred for the 5 merged same-URL
+  // requests.
+  EXPECT_EQ(GetTotalFetches(histogram_tester), 1u);
+}
+
+TEST_F(CloudApProviderWinTest, GetDataMultipleUrls) {
+  CloudApProviderWin provider;
+  GURL url1("https://test1.com");
+  GURL url2("https://test2.com");
+
+  int count = 0;
+  base::RunLoop run_loop;
+  base::RepeatingClosure barrier_closure =
+      base::BarrierClosure(2, run_loop.QuitClosure());
+
+  auto cb = [](int* count, base::RepeatingClosure barrier,
+               net::HttpRequestHeaders) {
+    (*count)++;
+    barrier.Run();
+  };
+
+  provider.GetData(url1, base::BindOnce(cb, &count, barrier_closure));
+  provider.GetData(url2, base::BindOnce(cb, &count, barrier_closure));
+
+  run_loop.Run();
+
+  EXPECT_EQ(count, 2);
+}
+
+TEST_F(CloudApProviderWinTest, GetDataGlobalOverflow) {
+  CloudApProviderWin provider;
+  base::HistogramTester histogram_tester;
+
+  int success_count = 0;
+  int overflow_count = 0;
+  base::RunLoop run_loop;
+  base::RepeatingClosure barrier_closure =
+      base::BarrierClosure(kMaxQueueSize + 1, run_loop.QuitClosure());
+
+  // Queue up to the global max size across distinct URLs.
+  for (size_t i = 0; i < kMaxQueueSize; ++i) {
+    GURL url(base::JoinString({"https://test", base::NumberToString(i), ".com"},
+                              ""));
+    provider.GetData(url,
+                     base::BindOnce(
+                         [](int* success_count, base::RepeatingClosure barrier,
+                            net::HttpRequestHeaders headers) {
+                           (*success_count)++;
+                           barrier.Run();
+                         },
+                         &success_count, barrier_closure));
+  }
+
+  // The next request (even to a new URL) should overflow due to the global
+  // limit.
+  GURL overflow_url("https://overflow.com");
+  provider.GetData(overflow_url,
+                   base::BindOnce(
+                       [](int* overflow_count, base::RepeatingClosure barrier,
+                          net::HttpRequestHeaders headers) {
+                         EXPECT_TRUE(headers.IsEmpty());
+                         (*overflow_count)++;
+                         barrier.Run();
+                       },
+                       &overflow_count, barrier_closure));
+
+  run_loop.Run();
+
+  EXPECT_EQ(success_count, static_cast<int>(kMaxQueueSize));
+  EXPECT_EQ(overflow_count, 1);
+
+  // Check that the queue overflow histogram recorded 1 sample.
+  histogram_tester.ExpectBucketCount(
+      "Enterprise.PlatformAuth.GetAuthData.QueueOverflow", true, 1);
+
+  // 100 distinct URLs should trigger 100 distinct background fetches.
+  EXPECT_EQ(GetTotalFetches(histogram_tester), kMaxQueueSize);
+}
+
 }  // namespace enterprise_auth
diff --git a/chrome/browser/enterprise/platform_auth/platform_auth_provider_manager_browsertest.cc b/chrome/browser/enterprise/platform_auth/platform_auth_provider_manager_browsertest.cc
index 7ecd5b09..f927164d 100644
--- a/chrome/browser/enterprise/platform_auth/platform_auth_provider_manager_browsertest.cc
+++ b/chrome/browser/enterprise/platform_auth/platform_auth_provider_manager_browsertest.cc
@@ -4,8 +4,15 @@
 
 #include "chrome/browser/enterprise/platform_auth/platform_auth_provider_manager.h"
 
+#include "base/synchronization/lock.h"
+#include "base/test/bind.h"
+#include "build/build_config.h"
 #include "chrome/browser/enterprise/platform_auth/mock_platform_auth_provider.h"
 #include "chrome/browser/enterprise/platform_auth/scoped_set_provider_for_testing.h"
+#if !BUILDFLAG(IS_ANDROID)
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
+#endif
 #include "chrome/test/base/chrome_test_utils.h"
 #include "chrome/test/base/platform_browser_test.h"
 #include "content/public/test/browser_test.h"
@@ -127,4 +134,101 @@
   EXPECT_CALL(*unsafe_mock_provider, Die());
 }
 
+#if !BUILDFLAG(IS_ANDROID)
+IN_PROC_BROWSER_TEST_F(PlatformAuthManagerBrowserTest, ConcurrentNavigations) {
+  base::Lock lock;
+  std::map<GURL, std::string> received_cookies;
+
+  embedded_test_server()->RegisterRequestHandler(base::BindLambdaForTesting(
+      [&](const net::test_server::HttpRequest& request)
+          -> std::unique_ptr<net::test_server::HttpResponse> {
+        // Filter out extraneous requests (e.g., /favicon.ico) automatically
+        // triggered by the browser after page load. Handling them here would
+        // cause lock contention and deadlock (DCHECK failure in base::Lock)
+        // against the main thread during the verification phase.
+        if (request.GetURL().path() != "/title1.html" &&
+            request.GetURL().path() != "/title2.html") {
+          return nullptr;
+        }
+        base::AutoLock auto_lock(lock);
+        if (request.headers.find("cookie") != request.headers.end()) {
+          received_cookies[request.GetURL()] = request.headers.at("cookie");
+        }
+        return nullptr;
+      }));
+
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  auto mock_provider =
+      std::make_unique<::testing::StrictMock<MockPlatformAuthProvider>>();
+  EXPECT_CALL(*mock_provider, SupportsOriginFiltering())
+      .WillRepeatedly(::testing::Return(false));
+
+  base::RunLoop get_data_run_loop;
+  std::map<GURL, PlatformAuthProviderManager::GetDataCallback>
+      pending_callbacks;
+  EXPECT_CALL(*mock_provider, GetData(_, _))
+      .WillRepeatedly([&](const GURL& url,
+                          PlatformAuthProviderManager::GetDataCallback cb) {
+        pending_callbacks[url] = std::move(cb);
+        if (pending_callbacks.size() == 2) {
+          get_data_run_loop.Quit();
+        }
+      });
+
+  MockPlatformAuthProvider* unsafe_mock_provider = mock_provider.get();
+  ScopedSetProviderForTesting set_provider(std::move(mock_provider));
+
+  PlatformAuthProviderManager::GetInstance().SetEnabled(true,
+                                                        base::OnceClosure());
+
+  GURL url1 = embedded_test_server()->GetURL("/title1.html");
+  GURL url2 = embedded_test_server()->GetURL("/title2.html");
+
+  // Start navigation in tab 1.
+  content::WebContents* tab1 = chrome_test_utils::GetActiveWebContents(this);
+  tab1->GetController().LoadURL(url1, content::Referrer(),
+                                ui::PAGE_TRANSITION_TYPED, std::string());
+
+  // Open a second tab and start navigation.
+  chrome::AddTabAt(browser(), GURL("about:blank"), -1, true);
+  content::WebContents* tab2 = chrome_test_utils::GetActiveWebContents(this);
+  ASSERT_NE(tab1, tab2);
+  tab2->GetController().LoadURL(url2, content::Referrer(),
+                                ui::PAGE_TRANSITION_TYPED, std::string());
+
+  // Wait until both navigations have reached the provider's GetData().
+  get_data_run_loop.Run();
+
+  ASSERT_EQ(pending_callbacks.size(), 2u);
+  ASSERT_TRUE(pending_callbacks.find(url1) != pending_callbacks.end());
+  ASSERT_TRUE(pending_callbacks.find(url2) != pending_callbacks.end());
+
+  // Resolve request 1 with token 1.
+  net::HttpRequestHeaders headers1;
+  headers1.SetHeader(net::HttpRequestHeaders::kCookie, "token=Token1");
+  std::move(pending_callbacks[url1]).Run(std::move(headers1));
+
+  // Resolve request 2 with token 2.
+  net::HttpRequestHeaders headers2;
+  headers2.SetHeader(net::HttpRequestHeaders::kCookie, "token=Token2");
+  std::move(pending_callbacks[url2]).Run(std::move(headers2));
+
+  // Wait for both tabs to finish loading.
... (truncated)