From 7cc254e978dd7699145eeda120873c03e9abf762 Mon Sep 17 00:00:00 2001
From: Nicolás Peña <[email protected]>
Date: Wed, 19 Aug 2026 14:14:47 -0700
Subject: [PATCH] [FedCM] Fix UAF when double clicking user other account

When the user clicks "Use a different account" in a FedCM dialog,
Request::LoginToIdP registers Request as an IdpSigninStatusObserver
and opens the IdP login popup.

If the user refocuses the FedCM dialog and clicks on the "Use a
different account" again while the accounts fetch is in flight, we
previously re-registered Request as an observer. When the pending
response arrived, updating the sign-in status would synchronously
notify observers. Because Request was observing again, it would start a
new accounts fetch, deleting the current AccountsFetcher while on
OnAccountsResponseReceived, causing a UAF.

This CL fixes this by:
1. Checking if the login popup is already open, and avoiding registering
as an observer again.
2. Adding a WeakPtr guard in OnAccountsResponseReceived so it returns
safely if destroyed during synchronous callbacks.

Fixed: 548340637
Change-Id: Ie7f7e5cc43809c6b1143d99d366fb1c52c480189
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8270646
Reviewed-by: Yi Gu <[email protected]>
Commit-Queue: Nicolás Peña <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1682618}
---

diff --git a/content/browser/webid/accounts_fetcher.cc b/content/browser/webid/accounts_fetcher.cc
index dc77cd3b..09b3907 100644
--- a/content/browser/webid/accounts_fetcher.cc
+++ b/content/browser/webid/accounts_fetcher.cc
@@ -377,9 +377,13 @@
   const std::optional<bool> old_idp_signin_status =
       permission_delegate_->GetIdpSigninStatus(
           url::Origin::Create(idp_config_url));
+  base::WeakPtr<AccountsFetcher> weak_this = weak_ptr_factory_.GetWeakPtr();
   UpdateIdpSigninStatusForAccountsEndpointResponse(
       idp_config_url, status, idp_info->has_failing_idp_signin_status,
       permission_delegate_);
+  if (!weak_this) {
+    return;
+  }
 
   if (status.parse_status != ParseStatus::kSuccess) {
     if (IsFedCmNativeIdPsEnabled() && network_manager_) {
diff --git a/content/browser/webid/accounts_fetcher_unittest.cc b/content/browser/webid/accounts_fetcher_unittest.cc
index 26669da7..0e3cdbd 100644
--- a/content/browser/webid/accounts_fetcher_unittest.cc
+++ b/content/browser/webid/accounts_fetcher_unittest.cc
@@ -478,4 +478,92 @@
   SetBrowserClientForTesting(original_client);
 }
 
+// Verifies that if AccountsFetcher is destroyed re-entrantly during
+// SetIdpSigninStatus (e.g. via observer callbacks), OnAccountsResponseReceived
+// returns cleanly without use-after-free.
+TEST_F(AccountsFetcherTest, ReentrantDestructionInAccountsResponse) {
+  const GURL kIdpConfigUrl("https://idp.example/fedcm.json");
+  const GURL kAccountsEndpoint("https://idp.example/accounts.json");
+  const GURL kTokenEndpoint("https://idp.example/token.json");
+
+  auto network_manager =
+      std::make_unique<StrictMock<MockIdpNetworkRequestManager>>();
+
+  IdpNetworkRequestManager::AccountsRequestCallback accounts_callback;
+  EXPECT_CALL(*network_manager, SendAccountsRequest)
+      .WillOnce(
+          WithArg<2>([&accounts_callback](
+                         IdpNetworkRequestManager::AccountsRequestCallback cb) {
+            accounts_callback = std::move(cb);
+            return true;
+          }));
+
+  std::unique_ptr<AccountsFetcher> fetcher = std::make_unique<AccountsFetcher>(
+      *main_rfh(), network_manager.get(), api_permission_delegate_.get(),
+      permission_delegate_.get(),
+      AccountsFetcher::FedCmFetchingParams(
+          blink::mojom::RpMode::kPassive, /*icon_ideal_size=*/0,
+          /*icon_minimum_size=*/0,
+          password_manager::CredentialMediationRequirement::kOptional),
+      base::BindLambdaForTesting(
+          [](base::TimeTicks, std::vector<AccountsFetcher::Result>) {
+            FAIL() << "Callback should not be called after destruction";
+          }));
+
+  auto config = blink::mojom::IdentityProviderConfig::New();
+  config->config_url = kIdpConfigUrl;
+
+  auto options = blink::mojom::IdentityProviderRequestOptions::New(
+      std::move(config), "nonce", /*login_hint=*/"", /*domain_hint=*/"",
+      /*fields=*/std::nullopt, /*params_json=*/std::nullopt,
+      /*format=*/std::nullopt);
+
+  IdpNetworkRequestManager::Endpoints endpoints;
+  endpoints.accounts = kAccountsEndpoint;
+  endpoints.token = kTokenEndpoint;
+
+  auto idp_info = std::make_unique<IdentityProviderInfo>(
+      options, endpoints, IdentityProviderMetadata(),
+      blink::mojom::RpContext::kSignIn, blink::mojom::RpMode::kPassive,
+      /*format=*/std::nullopt);
+
+  std::vector<std::unique_ptr<IdentityProviderInfo>> cached_idp_infos;
+  cached_idp_infos.push_back(std::move(idp_info));
+
+  base::flat_map<GURL, AccountsFetcher::IdentityProviderGetInfo>
+      token_request_get_infos;
+  token_request_get_infos.emplace(
+      kIdpConfigUrl,
+      AccountsFetcher::IdentityProviderGetInfo(
+          options.Clone(), blink::mojom::RpContext::kSignIn,
+          blink::mojom::RpMode::kPassive, /*format=*/std::nullopt));
+
+  fetcher->FetchAccountsForIdps(
+      std::move(cached_idp_infos), token_request_get_infos, metrics_.get(),
+      url::Origin::Create(GURL("https://rp.example")), base::DoNothing());
+
+  EXPECT_CALL(*permission_delegate_, SetIdpSigninStatus)
+      .WillOnce(
+          [&fetcher](const url::Origin&, bool,
+                     base::optional_ref<
+                         const blink::common::webid::LoginStatusOptions>) {
+            fetcher.reset();
+          });
+
+  IdpNetworkRequestManager::AccountsResponse accounts_response;
+  accounts_response.accounts.push_back(
+      base::MakeRefCounted<IdentityRequestAccount>(
+          "123", "[email protected]", "User Name", "[email protected]",
+          "User Name", "User", GURL(), /*phone=*/"", /*username=*/"",
+          /*potentially_approved_site_hashes=*/std::vector<std::string>(),
+          /*login_hints=*/std::vector<std::string>(),
+          /*domain_hints=*/std::vector<std::string>(),
+          /*labels=*/std::vector<std::string>()));
+
+  ASSERT_TRUE(accounts_callback);
+  std::move(accounts_callback)
+      .Run({ParseStatus::kSuccess, net::HTTP_OK}, std::move(accounts_response));
+  EXPECT_FALSE(fetcher);
+}
+
 }  // namespace content::webid
diff --git a/content/browser/webid/request.cc b/content/browser/webid/request.cc
index 8ecd8fe..548a89ad 100644
--- a/content/browser/webid/request.cc
+++ b/content/browser/webid/request.cc
@@ -1498,16 +1498,18 @@
   // the popup window is open. When using the active flow the dialog may
   // still be up in some cases, but we do not expect that browser automation
   // needs to interact with the account chooser in this case.
-  if (dialog_type_ != DialogType::kNone) {
+  if (dialog_type_ != DialogType::kNone && dialog_type_ != dialog_type) {
     // This call ensures that we send a dialogClosed event if an account
     // chooser or mismatch dialog is open.
     devtools_instrumentation::DidCloseFedCmDialog(render_frame_host());
   }
   // TODO(crbug.com/336815315): Should we notify browser automation of this
   // dialog?
+  if (dialog_type_ != dialog_type) {
+    UMA_HISTOGRAM_ENUMERATION("Blink.FedCm.Popup.DialogType", dialog_type);
+  }
   dialog_type_ = dialog_type;
   config_url_ = idp_config_url;
-  UMA_HISTOGRAM_ENUMERATION("Blink.FedCm.Popup.DialogType", dialog_type_);
 
   auto create_registry_async = [](base::WeakPtr<Request> weak_this,
                                   const GURL& idp_config_url,
@@ -2440,6 +2442,12 @@
     // needed.
     MaybeAppendQueryParameters(it->second, &login_url);
   }
+
+  if (dialog_type_ == DialogType::kLoginToIdpPopup) {
+    ShowModalDialog(DialogType::kLoginToIdpPopup, idp_config_url, login_url);
+    return;
+  }
+
   permission_delegate()->AddIdpSigninStatusObserver(this);
 
   account_ids_before_login_.clear();
diff --git a/content/browser/webid/request_unittest.cc b/content/browser/webid/request_unittest.cc
index 775d244..90795857 100644
--- a/content/browser/webid/request_unittest.cc
+++ b/content/browser/webid/request_unittest.cc
@@ -9789,4 +9789,46 @@
   }
 }
 
+// Verifies that calling LoginToIdP when the login popup is already open
+// refocuses the popup without re-registering the IdpSigninStatus observer or
+// resetting state, and that query parameters are preserved.
+TEST_F(RequestTest, LoginToIdPRefocusesWhenPopupAlreadyOpen) {
+  RequestParameters parameters = kDefaultRequestParameters;
+  parameters.identity_providers[0].domain_hint = kDomainHint;
+
+  test_permission_delegate_

diff --git a/content/browser/webid/accounts_fetcher_unittest.cc b/content/browser/webid/accounts_fetcher_unittest.cc
index 26669da7..0e3cdbd 100644
--- a/content/browser/webid/accounts_fetcher_unittest.cc
+++ b/content/browser/webid/accounts_fetcher_unittest.cc
@@ -478,4 +478,92 @@
   SetBrowserClientForTesting(original_client);
 }
 
+// Verifies that if AccountsFetcher is destroyed re-entrantly during
+// SetIdpSigninStatus (e.g. via observer callbacks), OnAccountsResponseReceived
+// returns cleanly without use-after-free.
+TEST_F(AccountsFetcherTest, ReentrantDestructionInAccountsResponse) {
+  const GURL kIdpConfigUrl("https://idp.example/fedcm.json");
+  const GURL kAccountsEndpoint("https://idp.example/accounts.json");
+  const GURL kTokenEndpoint("https://idp.example/token.json");
+
+  auto network_manager =
+      std::make_unique<StrictMock<MockIdpNetworkRequestManager>>();
+
+  IdpNetworkRequestManager::AccountsRequestCallback accounts_callback;
+  EXPECT_CALL(*network_manager, SendAccountsRequest)
+      .WillOnce(
+          WithArg<2>([&accounts_callback](
+                         IdpNetworkRequestManager::AccountsRequestCallback cb) {
+            accounts_callback = std::move(cb);
+            return true;
+          }));
+
+  std::unique_ptr<AccountsFetcher> fetcher = std::make_unique<AccountsFetcher>(
+      *main_rfh(), network_manager.get(), api_permission_delegate_.get(),
+      permission_delegate_.get(),
+      AccountsFetcher::FedCmFetchingParams(
+          blink::mojom::RpMode::kPassive, /*icon_ideal_size=*/0,
+          /*icon_minimum_size=*/0,
+          password_manager::CredentialMediationRequirement::kOptional),
+      base::BindLambdaForTesting(
+          [](base::TimeTicks, std::vector<AccountsFetcher::Result>) {
+            FAIL() << "Callback should not be called after destruction";
+          }));
+
+  auto config = blink::mojom::IdentityProviderConfig::New();
+  config->config_url = kIdpConfigUrl;
+
+  auto options = blink::mojom::IdentityProviderRequestOptions::New(
+      std::move(config), "nonce", /*login_hint=*/"", /*domain_hint=*/"",
+      /*fields=*/std::nullopt, /*params_json=*/std::nullopt,
+      /*format=*/std::nullopt);
+
+  IdpNetworkRequestManager::Endpoints endpoints;
+  endpoints.accounts = kAccountsEndpoint;
+  endpoints.token = kTokenEndpoint;
+
+  auto idp_info = std::make_unique<IdentityProviderInfo>(
+      options, endpoints, IdentityProviderMetadata(),
+      blink::mojom::RpContext::kSignIn, blink::mojom::RpMode::kPassive,
+      /*format=*/std::nullopt);
+
+  std::vector<std::unique_ptr<IdentityProviderInfo>> cached_idp_infos;
+  cached_idp_infos.push_back(std::move(idp_info));
+
+  base::flat_map<GURL, AccountsFetcher::IdentityProviderGetInfo>
+      token_request_get_infos;
+  token_request_get_infos.emplace(
+      kIdpConfigUrl,
+      AccountsFetcher::IdentityProviderGetInfo(
+          options.Clone(), blink::mojom::RpContext::kSignIn,
+          blink::mojom::RpMode::kPassive, /*format=*/std::nullopt));
+
+  fetcher->FetchAccountsForIdps(
+      std::move(cached_idp_infos), token_request_get_infos, metrics_.get(),
+      url::Origin::Create(GURL("https://rp.example")), base::DoNothing());
+
+  EXPECT_CALL(*permission_delegate_, SetIdpSigninStatus)
+      .WillOnce(
+          [&fetcher](const url::Origin&, bool,
+                     base::optional_ref<
+                         const blink::common::webid::LoginStatusOptions>) {
+            fetcher.reset();
+          });
+
+  IdpNetworkRequestManager::AccountsResponse accounts_response;
+  accounts_response.accounts.push_back(
+      base::MakeRefCounted<IdentityRequestAccount>(
+          "123", "[email protected]", "User Name", "[email protected]",
+          "User Name", "User", GURL(), /*phone=*/"", /*username=*/"",
+          /*potentially_approved_site_hashes=*/std::vector<std::string>(),
+          /*login_hints=*/std::vector<std::string>(),
+          /*domain_hints=*/std::vector<std::string>(),
+          /*labels=*/std::vector<std::string>()));
+
+  ASSERT_TRUE(accounts_callback);
+  std::move(accounts_callback)
+      .Run({ParseStatus::kSuccess, net::HTTP_OK}, std::move(accounts_response));
+  EXPECT_FALSE(fetcher);
+}
+
 }  // namespace content::webid
diff --git a/content/browser/webid/request_unittest.cc b/content/browser/webid/request_unittest.cc
index 775d244..90795857 100644
--- a/content/browser/webid/request_unittest.cc
+++ b/content/browser/webid/request_unittest.cc
@@ -9789,4 +9789,46 @@
   }
 }
 
+// Verifies that calling LoginToIdP when the login popup is already open
+// refocuses the popup without re-registering the IdpSigninStatus observer or
+// resetting state, and that query parameters are preserved.
+TEST_F(RequestTest, LoginToIdPRefocusesWhenPopupAlreadyOpen) {
+  RequestParameters parameters = kDefaultRequestParameters;
+  parameters.identity_providers[0].domain_hint = kDomainHint;
+
+  test_permission_delegate_
+      ->idp_signin_statuses_[OriginFromString(kProviderUrlFull)] = true;
+
+  auto dialog_controller =
+      std::make_unique<TestDialogController>(kConfigurationValid);
+  base::WeakPtr<TestDialogController> weak_dialog_controller =
+      dialog_controller->AsWeakPtr();
+  SetDialogController(std::move(dialog_controller));
+
+  std::string expected_url =
+      std::string(kIdpLoginUrl) + "?domain_hint=domain%40corp.com";
+  std::unique_ptr<WebContents> modal(CreateTestWebContents());
+  EXPECT_CALL(*weak_dialog_controller,
+              ShowModalDialog(GURL(expected_url), _, _, _, _))
+      .Times(2)
+      .WillRepeatedly(::testing::Return(modal.get()));
+
+  // AddIdpSigninStatusObserver should only be called once when opening the
+  // popup the first time, and NOT again when LoginToIdP is called while the
+  // popup is already open.
+  EXPECT_CALL(*test_permission_delegate_, AddIdpSigninStatusObserver).Times(1);
+
+  RunDontWaitForCallback(parameters, kConfigurationValid);
+  EXPECT_TRUE(did_show_idp_signin_status_mismatch_dialog());
+
+  // First LoginToIdP call: opens the popup, adds observer, sets dialog_type_.
+  SimulateLoginToIdP();
+  EXPECT_EQ(request_->GetDialogType(), Request::DialogType::kLoginToIdpPopup);
+
+  // Second LoginToIdP call while popup is already open: refocuses the popup
+  // without re-registering observer or resetting state.
+  SimulateLoginToIdP();
+  EXPECT_EQ(request_->GetDialogType(), Request::DialogType::kLoginToIdpPopup);
+}
+
 }  // namespace content::webid