From d06dc4817bab6d2d2516cafe2afa8257f3ca7b4c Mon Sep 17 00:00:00 2001
From: Jochen Eisinger <jochen@chromium.org>
Date: Wed, 24 Jun 2026 12:12:23 -0700
Subject: [PATCH] [SPC] Reject updateWith() once the dialog has been shown

The Secure Payment Confirmation dialog reads the payment details once
when it is set up and does not observe later spec changes, so
updateWith() must not be allowed to mutate the details after that point.
The only legitimate updateWith() for SPC is the one that resolves the
promise passed into show(), which arrives while the spec is still in the
INITIAL_PAYMENT_DETAILS state and before the dialog has read anything.
Reject any later updateWith() as a bad message and tear down the
request.

Also extend PaymentRequestValidationTest with the bits needed to
exercise Init()/Show() end to end (TestPersonalDataManager,
IsBrowserWindowActive() mock, credential_ids in the SPC method data) and
add coverage for both the rejected and the show-promise paths.

TAG=agy

Fixed: 498079379
Change-Id: If2677fa62d24ed82f3de41cfbf6724f1a7432e6b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7984525
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Slobodan Pejic <slobodan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1651879}
---

diff --git a/components/payments/content/payment_request.cc b/components/payments/content/payment_request.cc
index 847c687..f1c3e5c 100644
--- a/components/payments/content/payment_request.cc
+++ b/components/payments/content/payment_request.cc
@@ -543,6 +543,18 @@
     return;
   }
 
+  // The "secure-payment-confirmation" dialog displays a snapshot of the
+  // payment details at the time it is shown and does not refresh, so the only
+  // permitted update is the resolution of the promise passed into show()
+  // before the dialog has displayed anything.
+  if (spec_->IsSecurePaymentConfirmationRequested() && spec_->IsInitialized()) {
+    log_.Error(errors::kSecurePaymentConfirmationUpdateWithNotAllowed);
+    mojo::ReportBadMessage(
+        errors::kSecurePaymentConfirmationUpdateWithNotAllowed);
+    ResetAndDeleteThis();
+    return;
+  }
+
   // ID cannot be updated. Updating the total is optional.
   if (!details || details->id) {
     log_.Error(errors::kInvalidPaymentDetails);
diff --git a/components/payments/content/payment_request_unittest.cc b/components/payments/content/payment_request_unittest.cc
index ab8298b..4465d72 100644
--- a/components/payments/content/payment_request_unittest.cc
+++ b/components/payments/content/payment_request_unittest.cc
@@ -10,6 +10,7 @@
 
 #include "base/test/bind.h"
 #include "base/test/scoped_feature_list.h"
+#include "components/autofill/core/browser/data_manager/test_personal_data_manager.h"
 #include "components/payments/content/mock_content_payment_request_delegate.h"
 #include "components/payments/content/payment_request_display_manager.h"
 #include "content/public/browser/render_frame_host.h"
@@ -39,6 +40,8 @@
       scoped_features_.InitAndDisableFeature(
           ::features::kSecurePaymentConfirmation);
     }
+    personal_data_manager_.test_address_data_manager()
+        .SetAutofillProfileEnabled(false);
     NavigateAndCommit(page_url_);
   }
 
@@ -56,6 +59,10 @@
         .WillByDefault(testing::ReturnRefOfCopy(std::string("en-US")));
     ON_CALL(*delegate, GetLastCommittedURL())
         .WillByDefault(testing::ReturnRef(page_url_));
+    ON_CALL(*delegate, GetPersonalDataManager())
+        .WillByDefault(testing::Return(&personal_data_manager_));
+    ON_CALL(*delegate, IsBrowserWindowActive())
+        .WillByDefault(testing::Return(true));
     return delegate;
   }
 
@@ -66,6 +73,8 @@
     if (IsSecurePaymentConfirmationEnabled()) {
       spc_method->secure_payment_confirmation =
           mojom::SecurePaymentConfirmationRequest::New();
+      spc_method->secure_payment_confirmation->credential_ids.push_back(
+          {1, 2, 3, 4});
       spc_method->secure_payment_confirmation->challenge = {1, 2, 3, 4};
       spc_method->secure_payment_confirmation->rp_id = "rp.id";
       spc_method->secure_payment_confirmation->instrument =
@@ -92,6 +101,7 @@
   }
 
   const GURL page_url_{"https://a.com"};
+  autofill::TestPersonalDataManager personal_data_manager_;
   PaymentRequestDisplayManager display_manager_;
   base::test::ScopedFeatureList scoped_features_;
 };
@@ -188,6 +198,75 @@
   }
 }
 
+// Tests that the payment details cannot be updated for a
+// "secure-payment-confirmation" request after it has been shown, because the
+// dialog displays a snapshot of the details and would not reflect changes made
+// while it was visible.
+TEST_P(PaymentRequestValidationTest,
+       SecurePaymentConfirmationRejectsUpdateWith) {
+  mojo::test::BadMessageObserver bad_message_observer;
+
+  mojo::Remote<mojom::PaymentRequest> payment_request;
+  new PaymentRequest(CreateMockDelegate(),
+                     payment_request.BindNewPipeAndPassReceiver());
+
+  mojo::PendingRemote<mojom::PaymentRequestClient> client_remote;
+  auto dummy_receiver = client_remote.InitWithNewPipeAndPassReceiver();
+
+  std::vector<mojom::PaymentMethodDataPtr> method_data;
+  method_data.push_back(CreateSpcMethodData());
+
+  payment_request->Init(std::move(client_remote), std::move(method_data),
+                        CreateDummyDetails(), mojom::PaymentOptions::New());
+  payment_request->Show(/*wait_for_updated_details=*/false,
+                        /*had_user_activation=*/true);
+
+  auto updated_details = mojom::PaymentDetails::New();
+  updated_details->total = mojom::PaymentItem::New();
+  updated_details->total->label = "Total";
+  updated_details->total->amount = mojom::PaymentCurrencyAmount::New();
+  updated_details->total->amount->currency = "USD";
+  updated_details->total->amount->value = "5000.00";
+  payment_request->UpdateWith(std::move(updated_details));
+
+  payment_request.FlushForTesting();
+  EXPECT_TRUE(bad_message_observer.got_bad_message());
+}
+
+// Tests that updateWith() is allowed when resolving the promise passed into
+// show() for "secure-payment-confirmation", because that update happens before
+// the dialog displays the details.
+TEST_P(PaymentRequestValidationTest,
+       SecurePaymentConfirmationAllowsUpdateWithForShowPromise) {
+  mojo::test::BadMessageObserver bad_message_observer;
+
+  mojo::Remote<mojom::PaymentRequest> payment_request;
+  new PaymentRequest(CreateMockDelegate(),
+                     payment_request.BindNewPipeAndPassReceiver());
+
+  mojo::PendingRemote<mojom::PaymentRequestClient> client_remote;
+  auto dummy_receiver = client_remote.InitWithNewPipeAndPassReceiver();
+
+  std::vector<mojom::PaymentMethodDataPtr> method_data;
+  method_data.push_back(CreateSpcMethodData());
+
+  payment_request->Init(std::move(client_remote), std::move(method_data),
+                        CreateDummyDetails(), mojom::PaymentOptions::New());
+  payment_request->Show(/*wait_for_updated_details=*/true,
+                        /*had_user_activation=*/true);
+
+  auto updated_details = mojom::PaymentDetails::New();
+  updated_details->total = mojom::PaymentItem::New();
+  updated_details->total->label = "Total";
+  updated_details->total->amount = mojom::PaymentCurrencyAmount::New();
+  updated_details->total->amount->currency = "USD";
+  updated_details->total->amount->value = "2.00";
+  payment_request->UpdateWith(std::move(updated_details));
+
+  payment_request.FlushForTesting();
+  EXPECT_FALSE(bad_message_observer.got_bad_message());
+}
+
 INSTANTIATE_TEST_SUITE_P(All, PaymentRequestValidationTest, testing::Bool());
 
 }  // namespace
diff --git a/components/payments/core/error_strings.cc b/components/payments/core/error_strings.cc
index 1ff7e00..6c518bc 100644
--- a/components/payments/core/error_strings.cc
+++ b/components/payments/core/error_strings.cc
@@ -34,6 +34,7 @@
 const char kPayerPhoneEmpty[] = "Payment app returned invalid response. Missing field \"payerPhone\".";
 const char kProhibitedOrigin[] = "Only localhost, file://, and cryptographic scheme origins allowed.";
 const char kProhibitedOriginOrInvalidSslExplanation[] = "No UI will be shown. CanMakePayment and hasEnrolledInstrument will always return false. Show will be rejected with NotSupportedError.";
+const char kSecurePaymentConfirmationUpdateWithNotAllowed[] = "Payment details cannot be updated for the \"secure-payment-confirmation\" method while it is showing.";
 const char kShippingAddressInvalid[] = "Payment app returned invalid shipping address in response.";
 const char kShippingOptionEmpty[] = "Payment app returned invalid response. Missing field \"shipping option\".";
 const char kShippingOptionIdRequired[] = "Shipping option identifier required.";
diff --git a/components/payments/core/error_strings.h b/components/payments/core/error_strings.h
index c9c91eb..647bb835 100644
--- a/components/payments/core/error_strings.h
+++ b/components/payments/core/error_strings.h
@@ -77,6 +77,10 @@
 // or kInvalidSslCertificate error.
 extern const char kProhibitedOriginOrInvalidSslExplanation[];
 
+// Mojo call PaymentRequest::UpdateWith() is not allowed for the
+// "secure-payment-confirmation" method once its dialog has been shown.
+extern const char kSecurePaymentConfirmationUpdateWithNotAllowed[];

diff --git a/components/payments/content/payment_request_unittest.cc b/components/payments/content/payment_request_unittest.cc
index ab8298b..4465d72 100644
--- a/components/payments/content/payment_request_unittest.cc
+++ b/components/payments/content/payment_request_unittest.cc
@@ -10,6 +10,7 @@
 
 #include "base/test/bind.h"
 #include "base/test/scoped_feature_list.h"
+#include "components/autofill/core/browser/data_manager/test_personal_data_manager.h"
 #include "components/payments/content/mock_content_payment_request_delegate.h"
 #include "components/payments/content/payment_request_display_manager.h"
 #include "content/public/browser/render_frame_host.h"
@@ -39,6 +40,8 @@
       scoped_features_.InitAndDisableFeature(
           ::features::kSecurePaymentConfirmation);
     }
+    personal_data_manager_.test_address_data_manager()
+        .SetAutofillProfileEnabled(false);
     NavigateAndCommit(page_url_);
   }
 
@@ -56,6 +59,10 @@
         .WillByDefault(testing::ReturnRefOfCopy(std::string("en-US")));
     ON_CALL(*delegate, GetLastCommittedURL())
         .WillByDefault(testing::ReturnRef(page_url_));
+    ON_CALL(*delegate, GetPersonalDataManager())
+        .WillByDefault(testing::Return(&personal_data_manager_));
+    ON_CALL(*delegate, IsBrowserWindowActive())
+        .WillByDefault(testing::Return(true));
     return delegate;
   }
 
@@ -66,6 +73,8 @@
     if (IsSecurePaymentConfirmationEnabled()) {
       spc_method->secure_payment_confirmation =
           mojom::SecurePaymentConfirmationRequest::New();
+      spc_method->secure_payment_confirmation->credential_ids.push_back(
+          {1, 2, 3, 4});
       spc_method->secure_payment_confirmation->challenge = {1, 2, 3, 4};
       spc_method->secure_payment_confirmation->rp_id = "rp.id";
       spc_method->secure_payment_confirmation->instrument =
@@ -92,6 +101,7 @@
   }
 
   const GURL page_url_{"https://a.com"};
+  autofill::TestPersonalDataManager personal_data_manager_;
   PaymentRequestDisplayManager display_manager_;
   base::test::ScopedFeatureList scoped_features_;
 };
@@ -188,6 +198,75 @@
   }
 }
 
+// Tests that the payment details cannot be updated for a
+// "secure-payment-confirmation" request after it has been shown, because the
+// dialog displays a snapshot of the details and would not reflect changes made
+// while it was visible.
+TEST_P(PaymentRequestValidationTest,
+       SecurePaymentConfirmationRejectsUpdateWith) {
+  mojo::test::BadMessageObserver bad_message_observer;
+
+  mojo::Remote<mojom::PaymentRequest> payment_request;
+  new PaymentRequest(CreateMockDelegate(),
+                     payment_request.BindNewPipeAndPassReceiver());
+
+  mojo::PendingRemote<mojom::PaymentRequestClient> client_remote;
+  auto dummy_receiver = client_remote.InitWithNewPipeAndPassReceiver();
+
+  std::vector<mojom::PaymentMethodDataPtr> method_data;
+  method_data.push_back(CreateSpcMethodData());
+
+  payment_request->Init(std::move(client_remote), std::move(method_data),
+                        CreateDummyDetails(), mojom::PaymentOptions::New());
+  payment_request->Show(/*wait_for_updated_details=*/false,
+                        /*had_user_activation=*/true);
+
+  auto updated_details = mojom::PaymentDetails::New();
+  updated_details->total = mojom::PaymentItem::New();
+  updated_details->total->label = "Total";
+  updated_details->total->amount = mojom::PaymentCurrencyAmount::New();
+  updated_details->total->amount->currency = "USD";
+  updated_details->total->amount->value = "5000.00";
+  payment_request->UpdateWith(std::move(updated_details));
+
+  payment_request.FlushForTesting();
+  EXPECT_TRUE(bad_message_observer.got_bad_message());
+}
+
+// Tests that updateWith() is allowed when resolving the promise passed into
+// show() for "secure-payment-confirmation", because that update happens before
+// the dialog displays the details.
+TEST_P(PaymentRequestValidationTest,
+       SecurePaymentConfirmationAllowsUpdateWithForShowPromise) {
+  mojo::test::BadMessageObserver bad_message_observer;
+
+  mojo::Remote<mojom::PaymentRequest> payment_request;
+  new PaymentRequest(CreateMockDelegate(),
+                     payment_request.BindNewPipeAndPassReceiver());
+
+  mojo::PendingRemote<mojom::PaymentRequestClient> client_remote;
+  auto dummy_receiver = client_remote.InitWithNewPipeAndPassReceiver();
+
+  std::vector<mojom::PaymentMethodDataPtr> method_data;
+  method_data.push_back(CreateSpcMethodData());
+
+  payment_request->Init(std::move(client_remote), std::move(method_data),
+                        CreateDummyDetails(), mojom::PaymentOptions::New());
+  payment_request->Show(/*wait_for_updated_details=*/true,
+                        /*had_user_activation=*/true);
+
+  auto updated_details = mojom::PaymentDetails::New();
+  updated_details->total = mojom::PaymentItem::New();
+  updated_details->total->label = "Total";
+  updated_details->total->amount = mojom::PaymentCurrencyAmount::New();
+  updated_details->total->amount->currency = "USD";
+  updated_details->total->amount->value = "2.00";
+  payment_request->UpdateWith(std::move(updated_details));
+
+  payment_request.FlushForTesting();
+  EXPECT_FALSE(bad_message_observer.got_bad_message());
+}
+
 INSTANTIATE_TEST_SUITE_P(All, PaymentRequestValidationTest, testing::Bool());
 
 }  // namespace