From d592f083e77d44162c672637d66334cba1851561 Mon Sep 17 00:00:00 2001
From: Piotr Krzeszewski <[email protected]>
Date: Thu, 06 Aug 2026 08:03:17 -0700
Subject: [PATCH] [Safe Browsing] Disable keyboard override when proceeding is disabled

When SafeBrowsingProceedAnywayDisabled policy is active, typing "thisisunsafe"
prematurely allowlisted the host before policy checks forced navigation away.

This change:
- Sets `disableKeyboardOverride` in SafeBrowsingLoudErrorUI.
- Prevents allowlisting in BaseBlockingPage if proceeding is disabled.
- Adds unit test coverage for policy enforcement.

Tested with local reproduction steps presented in the bug

Bug: 501763003
Change-Id: Idea6720c9a8c07c6bfb261e9d29d4e50734f84cc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8178244
Reviewed-by: thefrog <[email protected]>
Reviewed-by: Michał Kaczmarczyk <[email protected]>
Commit-Queue: Piotr Krzeszewski <[email protected]>
Reviewed-by: Dominique Fauteux-Chapleau <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1674980}
---

diff --git a/components/safe_browsing/content/browser/base_blocking_page.cc b/components/safe_browsing/content/browser/base_blocking_page.cc
index 4b07a2bc..d600077c 100644
--- a/components/safe_browsing/content/browser/base_blocking_page.cc
+++ b/components/safe_browsing/content/browser/base_blocking_page.cc
@@ -118,7 +118,8 @@
       static_cast<security_interstitials::SecurityInterstitialCommand>(command);
 
   if (interstitial_command ==
-      security_interstitials::SecurityInterstitialCommand::CMD_PROCEED) {
+          security_interstitials::SecurityInterstitialCommand::CMD_PROCEED &&
+      !sb_error_ui_->is_proceed_anyway_disabled()) {
     // With committed interstitials, OnProceed() doesn't get called, so handle
     // adding to the allow list here.
     set_proceeded(true);
diff --git a/components/safe_browsing/content/browser/ui_manager_unittest.cc b/components/safe_browsing/content/browser/ui_manager_unittest.cc
index 4ef14a1..7ed70e7 100644
--- a/components/safe_browsing/content/browser/ui_manager_unittest.cc
+++ b/components/safe_browsing/content/browser/ui_manager_unittest.cc
@@ -96,7 +96,8 @@
   TestSafeBrowsingBlockingPage(BaseUIManager* manager,
                                content::WebContents* web_contents,
                                const GURL& main_frame_url,
-                               const UnsafeResourceList& unsafe_resources)
+                               const UnsafeResourceList& unsafe_resources,
+                               bool is_proceed_anyway_disabled = false)
       : SafeBrowsingBlockingPage(
             manager,
             web_contents,
@@ -117,14 +118,14 @@
             BaseSafeBrowsingErrorUI::SBErrorDisplayOptions(
                 BaseBlockingPage::IsMainPageResourceLoadPending(
                     unsafe_resources),
-                false,                 // is_extended_reporting_opt_in_allowed
-                false,                 // is_off_the_record
-                false,                 // is_extended_reporting_enabled
-                false,                 // is_extended_reporting_policy_managed
-                false,                 // is_enhanced_protection_enabled
-                false,                 // is_proceed_anyway_disabled
-                false,                 // should_open_links_in_new_tab
-                true,                  // always_show_back_to_safety
+                false,  // is_extended_reporting_opt_in_allowed
+                false,  // is_off_the_record
+                false,  // is_extended_reporting_enabled
+                false,  // is_extended_reporting_policy_managed
+                false,  // is_enhanced_protection_enabled
+                is_proceed_anyway_disabled,  // is_proceed_anyway_disabled
+                false,                       // should_open_links_in_new_tab
+                true,                        // always_show_back_to_safety
                 false,                 // is_enhanced_protection_message_enabled
                 false,                 // is_safe_browsing_managed
                 "cpn_safe_browsing"),  // help_center_article_link
@@ -132,7 +133,7 @@
             /*navigation_observer_manager=*/nullptr,
             /*metrics_collector=*/nullptr,
             /*trigger_manager=*/nullptr,
-            /*is_proceed_anyway_disabled=*/false,
+            is_proceed_anyway_disabled,
             /*is_safe_browsing_surveys_enabled=*/true,
             /*trust_safety_sentiment_service_trigger=*/base::NullCallback(),
             /*ignore_auto_revocation_notifications_trigger=*/
@@ -824,4 +825,27 @@
       &threat_type));
 }
 
+TEST_F(SafeBrowsingUIManagerTest,
+       ProceedAnywayDisabled_CommandReceivedDoesNotAllowlist) {
+  security_interstitials::UnsafeResource resource =
+      MakeUnsafeResourceAndStartNavigation(kBadURL);
+  resource.threat_source = ThreatSource::LOCAL_PVER4;
+  std::vector<security_interstitials::UnsafeResource> resources = {resource};
+  TestSafeBrowsingBlockingPage blocking_page(
+      ui_manager(), web_contents(), GURL(kBadURL), resources,
+      /*is_proceed_anyway_disabled=*/true);
+
+  // 1. Verify frontend string population: disableKeyboardOverride must be true.
+  base::DictValue load_time_data;
+  blocking_page.sb_error_ui()->PopulateStringsForHtml(load_time_data);
+  std::optional<bool> disable_keyboard =
+      load_time_data.FindBool("disableKeyboardOverride");
+  ASSERT_TRUE(disable_keyboard.has_value());
+  EXPECT_TRUE(disable_keyboard.value());
+
+  // 2. Verify backend command handling: CMD_PROCEED (1) must not allowlist URL.
+  blocking_page.CommandReceived("1");
+  EXPECT_FALSE(IsAllowlisted(resource));
+}
+
 }  // namespace safe_browsing
diff --git a/components/security_interstitials/core/safe_browsing_loud_error_ui.cc b/components/security_interstitials/core/safe_browsing_loud_error_ui.cc
index e12e759..a154c82 100644
--- a/components/security_interstitials/core/safe_browsing_loud_error_ui.cc
+++ b/components/security_interstitials/core/safe_browsing_loud_error_ui.cc
@@ -86,6 +86,7 @@
       "primaryButtonText",
       l10n_util::GetStringUTF16(IDS_SAFEBROWSING_OVERRIDABLE_SAFETY_BUTTON));
   load_time_data.Set("overridable", !is_proceed_anyway_disabled());
+  load_time_data.Set("disableKeyboardOverride", is_proceed_anyway_disabled());
   load_time_data.Set(
       security_interstitials::kOptInLink,
       l10n_util::GetStringUTF16(IDS_SAFE_BROWSING_SCOUT_REPORTING_AGREE));
@@ -131,8 +132,8 @@
   switch (command) {
     case CMD_PROCEED: {
       // User pressed on the button to proceed.
-      user_made_decision_ = true;
       if (!is_proceed_anyway_disabled()) {
+        user_made_decision_ = true;
         controller()->metrics_helper()->RecordUserDecision(
             MetricsHelper::PROCEED);
         controller()->Proceed();
@@ -143,7 +144,11 @@
     }
     case CMD_DONT_PROCEED: {
       // User pressed on the button to return to safety.
-      user_made_decision_ = true;
+      // Only record a user decision if the command was actually
+      // CMD_DONT_PROCEED (not a fallthrough from a policy-blocked CMD_PROCEED).
+      if (command == CMD_DONT_PROCEED) {
+        user_made_decision_ = true;
+      }
       // Don't record the user action here because there are other ways of
       // triggering DontProceed, like clicking the back button.
       if (is_main_frame_load_pending()) {
diff --git a/components/security_interstitials/core/safe_browsing_quiet_error_ui.cc b/components/security_interstitials/core/safe_browsing_quiet_error_ui.cc
index 75baec1..7482022 100644
--- a/components/security_interstitials/core/safe_browsing_quiet_error_ui.cc
+++ b/components/security_interstitials/core/safe_browsing_quiet_error_ui.cc
@@ -83,8 +83,8 @@
   switch (command) {
     case CMD_PROCEED: {
       // User pressed on the button to proceed.
-      user_made_decision_ = true;
       if (!is_proceed_anyway_disabled()) {
+        user_made_decision_ = true;
         controller()->metrics_helper()->RecordUserDecision(
             MetricsHelper::PROCEED);
         controller()->Proceed();

diff --git a/components/safe_browsing/content/browser/ui_manager_unittest.cc b/components/safe_browsing/content/browser/ui_manager_unittest.cc
index 4ef14a1..7ed70e7 100644
--- a/components/safe_browsing/content/browser/ui_manager_unittest.cc
+++ b/components/safe_browsing/content/browser/ui_manager_unittest.cc
@@ -96,7 +96,8 @@
   TestSafeBrowsingBlockingPage(BaseUIManager* manager,
                                content::WebContents* web_contents,
                                const GURL& main_frame_url,
-                               const UnsafeResourceList& unsafe_resources)
+                               const UnsafeResourceList& unsafe_resources,
+                               bool is_proceed_anyway_disabled = false)
       : SafeBrowsingBlockingPage(
             manager,
             web_contents,
@@ -117,14 +118,14 @@
             BaseSafeBrowsingErrorUI::SBErrorDisplayOptions(
                 BaseBlockingPage::IsMainPageResourceLoadPending(
                     unsafe_resources),
-                false,                 // is_extended_reporting_opt_in_allowed
-                false,                 // is_off_the_record
-                false,                 // is_extended_reporting_enabled
-                false,                 // is_extended_reporting_policy_managed
-                false,                 // is_enhanced_protection_enabled
-                false,                 // is_proceed_anyway_disabled
-                false,                 // should_open_links_in_new_tab
-                true,                  // always_show_back_to_safety
+                false,  // is_extended_reporting_opt_in_allowed
+                false,  // is_off_the_record
+                false,  // is_extended_reporting_enabled
+                false,  // is_extended_reporting_policy_managed
+                false,  // is_enhanced_protection_enabled
+                is_proceed_anyway_disabled,  // is_proceed_anyway_disabled
+                false,                       // should_open_links_in_new_tab
+                true,                        // always_show_back_to_safety
                 false,                 // is_enhanced_protection_message_enabled
                 false,                 // is_safe_browsing_managed
                 "cpn_safe_browsing"),  // help_center_article_link
@@ -132,7 +133,7 @@
             /*navigation_observer_manager=*/nullptr,
             /*metrics_collector=*/nullptr,
             /*trigger_manager=*/nullptr,
-            /*is_proceed_anyway_disabled=*/false,
+            is_proceed_anyway_disabled,
             /*is_safe_browsing_surveys_enabled=*/true,
             /*trust_safety_sentiment_service_trigger=*/base::NullCallback(),
             /*ignore_auto_revocation_notifications_trigger=*/
@@ -824,4 +825,27 @@
       &threat_type));
 }
 
+TEST_F(SafeBrowsingUIManagerTest,
+       ProceedAnywayDisabled_CommandReceivedDoesNotAllowlist) {
+  security_interstitials::UnsafeResource resource =
+      MakeUnsafeResourceAndStartNavigation(kBadURL);
+  resource.threat_source = ThreatSource::LOCAL_PVER4;
+  std::vector<security_interstitials::UnsafeResource> resources = {resource};
+  TestSafeBrowsingBlockingPage blocking_page(
+      ui_manager(), web_contents(), GURL(kBadURL), resources,
+      /*is_proceed_anyway_disabled=*/true);
+
+  // 1. Verify frontend string population: disableKeyboardOverride must be true.
+  base::DictValue load_time_data;
+  blocking_page.sb_error_ui()->PopulateStringsForHtml(load_time_data);
+  std::optional<bool> disable_keyboard =
+      load_time_data.FindBool("disableKeyboardOverride");
+  ASSERT_TRUE(disable_keyboard.has_value());
+  EXPECT_TRUE(disable_keyboard.value());
+
+  // 2. Verify backend command handling: CMD_PROCEED (1) must not allowlist URL.
+  blocking_page.CommandReceived("1");
+  EXPECT_FALSE(IsAllowlisted(resource));
+}
+
 }  // namespace safe_browsing