From 50479db4af54fb9c26c5a768fe4a4bb64399c482 Mon Sep 17 00:00:00 2001
From: Olivier Robin <[email protected]>
Date: Mon, 01 Jun 2026 05:31:37 -0700
Subject: [PATCH] Prevent showing RL offline if there is a security interstitial

Interstitial (like safe browsing) should have precedence on showing
offline page, so test their condition first before checking offline page
existence. Also test that there is no interstitial before showing
offline page.

Fixed: 513856644
Change-Id: I5049dd8954eefc23ba47a1bfed309aeae76a2ff6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7859554
Reviewed-by: Mike Dougherty <[email protected]>
Reviewed-by: Rohit Rao <[email protected]>
Commit-Queue: Olivier Robin <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1639318}
---

diff --git a/ios/chrome/browser/reading_list/model/BUILD.gn b/ios/chrome/browser/reading_list/model/BUILD.gn
index ac7fe731..c71be6e5 100644
--- a/ios/chrome/browser/reading_list/model/BUILD.gn
+++ b/ios/chrome/browser/reading_list/model/BUILD.gn
@@ -62,6 +62,7 @@
     "//ios/chrome/browser/signin/model:signin_util",
     "//ios/chrome/browser/sync/model:data_type_store_service_factory",
     "//ios/chrome/common",
+    "//ios/components/security_interstitials",
     "//ios/components/webui:url_constants",
     "//ios/web/common",
     "//ios/web/public",
diff --git a/ios/chrome/browser/reading_list/model/offline_page_tab_helper.mm b/ios/chrome/browser/reading_list/model/offline_page_tab_helper.mm
index 8dac9f2..ef2cf08 100644
--- a/ios/chrome/browser/reading_list/model/offline_page_tab_helper.mm
+++ b/ios/chrome/browser/reading_list/model/offline_page_tab_helper.mm
@@ -23,6 +23,7 @@
 #import "ios/chrome/browser/reading_list/model/reading_list_download_service_factory.h"
 #import "ios/chrome/browser/shared/model/profile/profile_ios.h"
 #import "ios/chrome/browser/shared/model/url/chrome_url_constants.h"
+#import "ios/components/security_interstitials/ios_blocking_page_tab_helper.h"
 #import "ios/web/common/features.h"
 #import "ios/web/public/navigation/navigation_context.h"
 #import "ios/web/public/navigation/navigation_item.h"
@@ -309,6 +310,18 @@
 }
 
 void OfflinePageTabHelper::PresentOfflinePageForOnlineUrl(const GURL& url) {
+  CHECK(web_state_);
+
+  // Do not load offline page if a security interstitial blocking page is
+  // currently committed and showing.
+  security_interstitials::IOSBlockingPageTabHelper* blocking_page_tab_helper =
+      security_interstitials::IOSBlockingPageTabHelper::FromWebState(
+          web_state_);
+  if (blocking_page_tab_helper &&
+      blocking_page_tab_helper->GetCurrentBlockingPage()) {
+    return;
+  }
+
   // As presenting the offline version will replace the content of the committed
   // page, the offline version can only be presented if the navigation currently
   // tracked by the OfflinePageTabHelper is the last committed one.
diff --git a/ios/chrome/browser/reading_list/ui_bundled/BUILD.gn b/ios/chrome/browser/reading_list/ui_bundled/BUILD.gn
index da5212c..cd49aea 100644
--- a/ios/chrome/browser/reading_list/ui_bundled/BUILD.gn
+++ b/ios/chrome/browser/reading_list/ui_bundled/BUILD.gn
@@ -236,6 +236,7 @@
     "//base",
     "//base/test:test_support",
     "//components/signin/public/base",
+    "//components/strings",
     "//components/sync/base",
     "//components/sync/base:features",
     "//ios/chrome/app/strings",
diff --git a/ios/chrome/browser/reading_list/ui_bundled/reading_list_egtest.mm b/ios/chrome/browser/reading_list/ui_bundled/reading_list_egtest.mm
index bb4784a2..6476028 100644
--- a/ios/chrome/browser/reading_list/ui_bundled/reading_list_egtest.mm
+++ b/ios/chrome/browser/reading_list/ui_bundled/reading_list_egtest.mm
@@ -17,6 +17,7 @@
 #import "base/strings/sys_string_conversions.h"
 #import "base/strings/utf_string_conversions.h"
 #import "base/test/ios/wait_util.h"
+#import "components/strings/grit/components_strings.h"
 #import "components/sync/base/user_selectable_type.h"
 #import "ios/chrome/browser/authentication/test/signin_earl_grey.h"
 #import "ios/chrome/browser/authentication/test/signin_earl_grey_ui_test_util.h"
@@ -798,6 +799,48 @@
   AssertIsShowingDistillablePage(false, distillableURL);
 }
 
+// Tests that offline page is not loaded if a security interstitial is visible.
+- (void)testOfflinePageSafeBrowsingPhishingError {
+  GURL distillablePageURL(self.testServer->GetURL(kDistillableURL));
+  GURL nonDistillablePageURL(self.testServer->GetURL(kNonDistillableURL));
+
+  // STAGE 1: Launch normally, load & distill "Tomato" page, and add to list.
+  [ReadingListAppInterface forceConnectionToWifi];
+  [ChromeEarlGrey loadURL:distillablePageURL];
+  [ChromeEarlGrey waitForPageToFinishLoading];
+  AddCurrentPageToReadingList();
+
+  // Navigate away to clear active tab content, wait for distillation.
+  [ChromeEarlGrey loadURL:nonDistillablePageURL];
+  [ChromeEarlGrey waitForPageToFinishLoading];
+  OpenReadingList();
+  AssertEntryVisible(kDistillableTitle);
+  WaitForDistillation();
+  TapToolbarButtonWithID(
+      kReadingListNavigationBarCloseButtonID);  // Close Reading List
+
+  // STAGE 2: Relaunch with phishing flag and safe browsing enabled.
+  AppLaunchConfiguration phishingConfig;
+  phishingConfig.additional_args.push_back(std::string("--mark_as_phishing=") +
+                                           distillablePageURL.spec());
+  phishingConfig.additional_args.push_back(
+      std::string("--enable-features=SafeBrowsingHashPrefixRealTimeLookups"));
+  phishingConfig.relaunch_policy = ForceRelaunchByKilling;
+  [[AppLaunchManager sharedManager]
+      ensureAppLaunchedWithConfiguration:phishingConfig];
+  [ChromeEarlGrey setBoolValue:YES forUserPref:"safebrowsing.enabled"];
+
+  // Open Reading List, tap the "Tomato" entry.
+  OpenReadingList();
+  AssertEntryVisible(kDistillableTitle);
+  TapEntry(kDistillableTitle);
+
+  // Assert Safe Browsing block is shown, and distilled page was NOT loaded.
+  [ChromeEarlGrey waitForWebStateContainingText:l10n_util::GetStringUTF8(
+                                                    IDS_SAFEBROWSING_HEADING)];
+  [ChromeEarlGrey waitForWebStateNotContainingText:kContentToKeep];
+}
+
 // Tests that only the "Select" and "Close" button are showing when not editing.
 - (void)testVisibleButtonsNonEditingMode {
   GREYAssertNil(
diff --git a/ios/chrome/browser/web/model/BUILD.gn b/ios/chrome/browser/web/model/BUILD.gn
index 4071b1f..6178f0b 100644
--- a/ios/chrome/browser/web/model/BUILD.gn
+++ b/ios/chrome/browser/web/model/BUILD.gn
@@ -406,6 +406,7 @@
     "//ios/chrome/browser/infobars/model:public",
     "//ios/chrome/browser/passwords/model",
     "//ios/chrome/browser/reading_list/model",
+    "//ios/chrome/browser/reading_list/model:test_support",
     "//ios/chrome/browser/safe_browsing/model",
     "//ios/chrome/browser/shared/model/profile/test",
     "//ios/chrome/browser/shared/model/url:constants",
diff --git a/ios/chrome/browser/web/model/chrome_web_client.mm b/ios/chrome/browser/web/model/chrome_web_client.mm
index e6589099..bdd9e25 100644
--- a/ios/chrome/browser/web/model/chrome_web_client.mm
+++ b/ios/chrome/browser/web/model/chrome_web_client.mm
@@ -494,23 +494,6 @@
     const std::optional<net::SSLInfo>& ssl_info,
     int64_t navigation_id,
     base::OnceCallback<void(NSString*)> callback) {
-  OfflinePageTabHelper* offline_page_tab_helper =
-      OfflinePageTabHelper::FromWebState(web_state);
-  // WebState that are not attached to a tab may not have an
-  // OfflinePageTabHelper.
-  if (offline_page_tab_helper &&
-      (offline_page_tab_helper->CanHandleErrorLoadingURL(url))) {
-    // An offline version of the page will be displayed to replace this error
-    // page. Loading an error page here can cause a race between the
-    // navigation to load the error page and the navigation to display the
-    // offline version of the page. If the latter navigation interrupts the
-    // former and causes it to fail, this can incorrectly appear to be a
-    // navigation back to the previous committed URL. To avoid this race,
-    // return a nil error page here to avoid an error page load. See
-    // crbug.com/980912.
-    std::move(callback).Run(nil);
-    return;
-  }
   DCHECK(error);
   NSError* final_underlying_error =
       base::ios::GetFinalUnderlyingErrorFromError(error);
@@ -541,6 +524,23 @@
         ssl_info.value(), url, ssl_info.value().is_fatal_cert_error,
         navigation_id, std::move(callback));
   } else {
+    OfflinePageTabHelper* offline_page_tab_helper =
+        OfflinePageTabHelper::FromWebState(web_state);
+    // WebState that are not attached to a tab may not have an
+    // OfflinePageTabHelper.
+    if (offline_page_tab_helper &&
+        (offline_page_tab_helper->CanHandleErrorLoadingURL(url))) {
+      // An offline version of the page will be displayed to replace this error
+      // page. Loading an error page here can cause a race between the
+      // navigation to load the error page and the navigation to display the
+      // offline version of the page. If the latter navigation interrupts the
+      // former and causes it to fail, this can incorrectly appear to be a
+      // navigation back to the previous committed URL. To avoid this race,
+      // return a nil error page here to avoid an error page load. See
+      // crbug.com/980912.
+      std::move(callback).Run(nil);
+      return;
+    }
     std::move(callback).Run(
         GetErrorPage(url, error, is_post, is_off_the_record));
   }
diff --git a/ios/chrome/browser/web/model/chrome_web_client_unittest.mm b/ios/chrome/browser/web/model/chrome_web_client_unittest.mm

diff --git a/ios/chrome/browser/web/model/chrome_web_client_unittest.mm b/ios/chrome/browser/web/model/chrome_web_client_unittest.mm
index 6def60e4..a918163 100644
--- a/ios/chrome/browser/web/model/chrome_web_client_unittest.mm
+++ b/ios/chrome/browser/web/model/chrome_web_client_unittest.mm
@@ -17,11 +17,16 @@
 #import "components/captive_portal/core/captive_portal_detector.h"
 #import "components/content_settings/core/browser/host_content_settings_map.h"
 #import "components/lookalikes/core/lookalike_url_util.h"
+#import "components/reading_list/core/reading_list_entry.h"
+#import "components/reading_list/core/reading_list_model.h"
 #import "components/safe_browsing/ios/browser/safe_browsing_url_allow_list.h"
 #import "components/security_interstitials/core/unsafe_resource.h"
 #import "components/strings/grit/components_strings.h"
 #import "ios/chrome/browser/content_settings/model/host_content_settings_map_factory.h"
+#import "ios/chrome/browser/reading_list/model/offline_page_tab_helper.h"
 #import "ios/chrome/browser/reading_list/model/offline_url_utils.h"
+#import "ios/chrome/browser/reading_list/model/reading_list_model_factory.h"
+#import "ios/chrome/browser/reading_list/model/reading_list_test_utils.h"
 #import "ios/chrome/browser/safe_browsing/model/safe_browsing_blocking_page.h"
 #import "ios/chrome/browser/shared/model/profile/test/test_profile_ios.h"
 #import "ios/chrome/browser/shared/model/url/chrome_url_constants.h"
@@ -337,6 +342,125 @@
   EXPECT_TRUE([page containsString:error_string]);
 }
 
+// Tests PrepareErrorPage with a Reading List entry, which would normally
+// trigger the offline page bypass, but with a Safe Browsing error (which is a
+// security error), so the Safe Browsing interstitial should NOT be bypassed and
+// must be displayed instead.
+TEST_F(ChromeWebClientTest,
+       PrepareErrorPageWithOfflineDistilledPageAndSafeBrowsingError) {
+  // Store a distilled Reading List entry for the test URL.
+  std::vector<scoped_refptr<ReadingListEntry>> initial_entries;
+  initial_entries.push_back(base::MakeRefCounted<ReadingListEntry>(
+      GURL(kTestUrl), "Test Title", base::Time::Now()));
+
+  TestProfileIOS::Builder builder;
+  builder.AddTestingFactory(ReadingListModelFactory::GetInstance(),
+                            ReadingListModelTestingFactoryWithFakeStorage(
+                                std::move(initial_entries)));
+  std::unique_ptr<TestProfileIOS> reading_list_profile =
+      std::move(builder).Build();
+
+  ReadingListModel* reading_list_model =
+      ReadingListModelFactory::GetForProfile(reading_list_profile.get());
+  reading_list_model->SetEntryDistilledInfoIfExists(
+      GURL(kTestUrl), base::FilePath("distilled.html"),
+      GURL("http://foo.bar/distilled"), 50, base::Time::FromTimeT(100));
+
+  web::FakeWebState web_state;
+  web_state.SetBrowserState(reading_list_profile.get());
+  OfflinePageTabHelper::CreateForWebState(&web_state, reading_list_model);
+  SafeBrowsingUrlAllowList::CreateForWebState(&web_state);
+  SafeBrowsingUnsafeResourceContainer::CreateForWebState(&web_state);
+  security_interstitials::IOSBlockingPageTabHelper::CreateForWebState(
+      &web_state);
+
+  security_interstitials::UnsafeResource resource;
+  resource.threat_type =
+      safe_browsing::SBThreatType::SB_THREAT_TYPE_URL_PHISHING;
+  resource.url = GURL(kTestUrl);
+  resource.weak_web_state = web_state.GetWeakPtr();
+  resource.threat_source = safe_browsing::ThreatSource::LOCAL_PVER4;
+  SafeBrowsingUrlAllowList::FromWebState(&web_state)
+      ->AddPendingUnsafeNavigationDecision(resource.url, resource.threat_type);
+  SafeBrowsingUnsafeResourceContainer::FromWebState(&web_state)
+      ->StoreMainFrameUnsafeResource(resource);
+
+  NSError* error =
+      [NSError errorWithDomain:kSafeBrowsingErrorDomain
+                          code:base::checked_cast<NSInteger>(
+                                   SafeBrowsingErrorCode::kUnsafeResource)
+                      userInfo:nil];
+  __block bool callback_called = false;
+  __block NSString* page = nil;
+  base::OnceCallback<void(NSString*)> callback =
+      base::BindOnce(^(NSString* error_html) {
+        callback_called = true;
+        page = error_html;
+      });
+
+  ChromeWebClient web_client;
+  web_client.PrepareErrorPage(&web_state, GURL(kTestUrl), error,
+                              /*is_post=*/false,
+                              /*is_off_the_record=*/false,
+                              /*info=*/std::optional<net::SSLInfo>(),
+                              /*navigation_id=*/0, std::move(callback));
+
+  EXPECT_TRUE(callback_called);
+  EXPECT_NSNE(nil, page);
+  NSString* error_string = l10n_util::GetNSString(IDS_SAFEBROWSING_HEADING);
+  EXPECT_TRUE([page containsString:error_string]);
+}
+
+// Tests PrepareErrorPage with a Reading List entry and a regular non-security
+// error, verifying that it returns nil error page to hand over control to the
+// offline page presentation.
+TEST_F(ChromeWebClientTest,
+       PrepareErrorPageWithOfflineDistilledPageAndNonSecurityError) {
+  // Store a distilled Reading List entry for the test URL.
+  std::vector<scoped_refptr<ReadingListEntry>> initial_entries;
+  initial_entries.push_back(base::MakeRefCounted<ReadingListEntry>(
+      GURL(kTestUrl), "Test Title", base::Time::Now()));
+
+  TestProfileIOS::Builder builder;
+  builder.AddTestingFactory(ReadingListModelFactory::GetInstance(),
+                            ReadingListModelTestingFactoryWithFakeStorage(
+                                std::move(initial_entries)));
+  std::unique_ptr<TestProfileIOS> reading_list_profile =
+      std::move(builder).Build();
+
+  ReadingListModel* reading_list_model =
+      ReadingListModelFactory::GetForProfile(reading_list_profile.get());
+  reading_list_model->SetEntryDistilledInfoIfExists(
+      GURL(kTestUrl), base::FilePath("distilled.html"),
+      GURL("http://foo.bar/distilled"), 50, base::Time::FromTimeT(100));
+
+  web::FakeWebState web_state;
+  web_state.SetBrowserState(reading_list_profile.get());
+  OfflinePageTabHelper::CreateForWebState(&web_state, reading_list_model);
+
+  // A typical non-security network error (e.g., connection timed out).
+  NSError* error = [NSError errorWithDomain:NSURLErrorDomain
+                                       code:NSURLErrorTimedOut
+                                   userInfo:nil];
+  __block bool callback_called = false;
+  __block NSString* page = @"dummy_initial_value";
+  base::OnceCallback<void(NSString*)> callback =
+      base::BindOnce(^(NSString* error_html) {
+        callback_called = true;
+        page = error_html;
+      });
+
+  ChromeWebClient web_client;
+  web_client.PrepareErrorPage(&web_state, GURL(kTestUrl), error,
+                              /*is_post=*/false,
+                              /*is_off_the_record=*/false,
+                              /*info=*/std::optional<net::SSLInfo>(),
+                              /*navigation_id=*/0, std::move(callback));
+
+  EXPECT_TRUE(callback_called);
+  EXPECT_NSEQ(nil, page);
+}
+
 // Tests PrepareErrorPage for a safe browsing enterprise block error, which
 // results in a committed enterprise interstitial.
 TEST_F(ChromeWebClientTest,