From 25718a2bb2bcdfeabb72c167efd2fcb133a12173 Mon Sep 17 00:00:00 2001
From: Mike Dougherty <[email protected]>
Date: Thu, 27 Aug 2026 09:59:22 -0700
Subject: [PATCH] [iOS] Prevent address bar spoofing on provisional server redirects

Previously, `CRWWKNavigationHandler` updated the pending
`NavigationItemImpl`'s URL and virtual URL immediately upon receiving a
provisional server redirect in
`didReceiveServerRedirectForProvisionalNavigation:`. Because
browser-initiated pending items are returned by `GetVisibleItem()` while
loading, this allowed the omnibox to prematurely display the redirect
destination URL (and secure padlock) before the navigation ever
committed. If the destination document stalled or failed to commit, the
originating document remained painted while the omnibox displayed the
spoofed destination domain. This CL fixes the issue by:
1. Keeping the initial requested URL on the pending `NavigationItemImpl`
   during provisional redirects, while still tracking the updated URL on
   `NavigationContextImpl`.
2. Deferring the update of the navigation item's URL and virtual URL to
   commit time in `webPageChangedWithContext:webView:`.
3. Ensuring `context->SetHasCommitted(true)` in `didCommitNavigation:`
   correctly handles redirect commits.
Tests:
- Added `CRWKNavigationHandlerIntTest.VisibleItemDuringServerRedirect`,
  which verifies that `NavigationManager::GetVisibleItem()` continues to
  return the original requested URL during `DidRedirectNavigation` and
  only updates to the destination URL after the navigation finishes and
  commits.

Fixed: 546438368
Change-Id: I1a86863a40ad948f913782455c953677ce00843b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8281650
Commit-Queue: Mike Dougherty <[email protected]>
Reviewed-by: Justin Cohen <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1687338}
---
diff --git a/ios/web/navigation/crw_wk_navigation_handler.mm b/ios/web/navigation/crw_wk_navigation_handler.mm
index 1cd434e2..7786680 100644
--- a/ios/web/navigation/crw_wk_navigation_handler.mm
+++ b/ios/web/navigation/crw_wk_navigation_handler.mm
@@ -698,10 +698,6 @@
   // navigation. WKWebView allows multiple provisional navigations, while
   // Navigation Manager has only one pending navigation.
   if (item) {
-    if (!self.pendingNavigationInfo.unsafeRedirect) {
-      item->SetVirtualURL(webViewURL);
-      item->SetURL(webViewURL);
-    }
     // Redirects (3xx response code), must change POST requests to GETs.
     item->SetPostData(nil);
     item->ResetHttpRequestHeaders();
@@ -891,15 +887,16 @@
               ->mime_type());
     }
 
-    if ((pendingURL == webViewURL) || (context->IsLoadingHtmlString())) {
+    if (context->IsLoadingHtmlString() ||
+        (pendingItem &&
+         ((pendingURL == webViewURL) || (context->GetUrl() == webViewURL)))) {
       // Commit navigation if at least one of these is true:
-      //  - Navigation has pending item (this should always be true, but
-      //    pending item may not exist due to crbug.com/925304).
-      //  - Navigation is loadHTMLString:baseURL: navigation, which does not
-      //    create a pending item, but modifies committed item instead.
-      //  - Transition type is reload with Legacy Navigation Manager (Legacy
-      //    Navigation Manager does not create pending item for reload due to
-      //    crbug.com/676129)
+      //  - Navigation is a `loadHTMLString:baseURL:` navigation, which does not
+      //    create a pending item, but modifies the committed item instead.
+      //  - Navigation has a pending item (this should always be true, but
+      //    pending item may not exist due to crbug.com/925304) whose initial
+      //    URL (`pendingURL`) or redirected URL (`context->GetUrl()`) matches
+      //    `webViewURL`.
       context->SetHasCommitted(true);
     }
     self.webStateImpl->SetContentsMimeType(
@@ -2565,7 +2562,14 @@
   [self resetDocumentSpecificState];
 
   [self.delegate navigationHandlerDidStartLoading:self];
-  self.navigationManagerImpl->CommitPendingItem(context->ReleaseItem());
+  std::unique_ptr<web::NavigationItemImpl> item = context->ReleaseItem();
+  if (item && item->GetURL() != context->GetUrl()) {
+    if (item->GetVirtualURL() == item->GetURL()) {
+      item->SetVirtualURL(context->GetUrl());
+    }
+    item->SetURL(context->GetUrl());
+  }
+  self.navigationManagerImpl->CommitPendingItem(std::move(item));
   if (context->IsLoadingHtmlString()) {
     self.navigationManagerImpl->GetLastCommittedItem()->SetURL(
         context->GetUrl());
diff --git a/ios/web/navigation/crw_wk_navigation_handler_inttest.mm b/ios/web/navigation/crw_wk_navigation_handler_inttest.mm
index f5092cd..9e0a439 100644
--- a/ios/web/navigation/crw_wk_navigation_handler_inttest.mm
+++ b/ios/web/navigation/crw_wk_navigation_handler_inttest.mm
@@ -100,6 +100,40 @@
   ErrorType error_type_ = ErrorType::kNone;
 };
 
+// A WebStateObserver that tracks the url of the visible item during
+// DidRedirectNavigation.
+class RedirectVisibleItemObserver : public web::WebStateObserver {
+ public:
+  explicit RedirectVisibleItemObserver(web::WebState* web_state) {
+    scoped_observation_.Observe(web_state);
+  }
+
+  void DidRedirectNavigation(
+      web::WebState* web_state,
+      web::NavigationContext* navigation_context) override {
+    web::NavigationItem* visible_item =
+        web_state->GetNavigationManager()->GetVisibleItem();
+    ASSERT_TRUE(visible_item);
+    visible_url_during_redirect_ = visible_item->GetURL();
+    did_redirect_ = true;
+  }
+
+  void WebStateDestroyed(web::WebState* web_state) override {
+    scoped_observation_.Reset();
+  }
+
+  bool did_redirect() const { return did_redirect_; }
+  const GURL& visible_url_during_redirect() const {
+    return visible_url_during_redirect_;
+  }
+
+ private:
+  base::ScopedObservation<web::WebState, web::WebStateObserver>
+      scoped_observation_{this};
+  bool did_redirect_ = false;
+  GURL visible_url_during_redirect_;
+};
+
 }  // namespace
 
 namespace web {
@@ -301,4 +335,33 @@
   EXPECT_FALSE(test::IsWebViewContainingText(web_state(), kBlockedDataContent));
 }
 
+// Tests that GetVisibleItem() does not expose uncommitted redirect destinations
+// during provisional redirects, preventing address bar spoofing.
+TEST_F(CRWKNavigationHandlerIntTest, VisibleItemDuringServerRedirect) {
+  ASSERT_TRUE(server_.Start());
+  GURL destination_url(server_.GetURL("/echoall"));
+  GURL redirect_url(
+      server_.GetURL("/server-redirect?" + destination_url.spec()));
+
+  RedirectVisibleItemObserver observer(web_state());
+  ASSERT_TRUE(LoadUrl(redirect_url));
+
+  EXPECT_TRUE(observer.did_redirect());
+  // The visible item during provisional redirect must remain the initial
+  // requested URL to prevent address bar spoofing.
+  EXPECT_EQ(redirect_url, observer.visible_url_during_redirect());
+
+  // Once committed, the visible and last committed items reflect the
+  // destination.
+  NavigationItem* visible_item =
+      web_state()->GetNavigationManager()->GetVisibleItem();
+  ASSERT_TRUE(visible_item);
+  EXPECT_EQ(destination_url, visible_item->GetURL());
+
+  NavigationItem* committed_item =
+      web_state()->GetNavigationManager()->GetLastCommittedItem();
+  ASSERT_TRUE(committed_item);
+  EXPECT_EQ(destination_url, committed_item->GetURL());
+}
+
 }  // namespace web
diff --git a/ios/web/web_state/web_state_observer_inttest.mm b/ios/web/web_state/web_state_observer_inttest.mm
index 7c0b167..3e60236e 100644
--- a/ios/web/web_state/web_state_observer_inttest.mm
+++ b/ios/web/web_state/web_state_observer_inttest.mm
@@ -94,6 +94,12 @@
   EXPECT_EQ(expected_title, base::UTF16ToASCII(web_state->GetTitle()));
 }
 
+// Verifies correctness of WebState's visible URL.
+ACTION_P(VerifyVisibleURL, expected_url) {
+  WebState* web_state = arg0;
+  EXPECT_EQ(expected_url, web_state->GetVisibleURL());
+}
+
 // Verifies correctness of `NavigationContext` (`arg1`) for new page navigation
 // passed to `DidStartNavigation`. Stores `NavigationContext` in `context`
 // pointer.
@@ -2074,31 +2080,36 @@
                   _, RequestInfoMatch(expected_redirect_request_info), _))
       .WillOnce(
           RunOnceCallback<2>(WebStatePolicyDecider::PolicyDecision::Allow()));
-  EXPECT_CALL(observer_, DidRedirectNavigation(web_state(), _));
+  EXPECT_CALL(observer_, DidRedirectNavigation(web_state(), _))
+      .WillOnce(VerifyVisibleURL(url));
   EXPECT_CALL(*decider_,
               MockShouldAllowRequest(
                   _, RequestInfoMatch(expected_redirect_request_info), _))
       .WillOnce(
           RunOnceCallback<2>(WebStatePolicyDecider::PolicyDecision::Allow()));
-  EXPECT_CALL(observer_, DidRedirectNavigation(web_state(), _));
+  EXPECT_CALL(observer_, DidRedirectNavigation(web_state(), _))
+      .WillOnce(VerifyVisibleURL(url));
   EXPECT_CALL(*decider_,
               MockShouldAllowRequest(
                   _, RequestInfoMatch(expected_redirect_request_info), _))
       .WillOnce(
           RunOnceCallback<2>(WebStatePolicyDecider::PolicyDecision::Allow()));
-  EXPECT_CALL(observer_, DidRedirectNavigation(web_state(), _));
+  EXPECT_CALL(observer_, DidRedirectNavigation(web_state(), _))
+      .WillOnce(VerifyVisibleURL(url));
   EXPECT_CALL(*decider_,
               MockShouldAllowRequest(
                   _, RequestInfoMatch(expected_redirect_request_info), _))
       .WillOnce(
           RunOnceCallback<2>(WebStatePolicyDecider::PolicyDecision::Allow()));
-  EXPECT_CALL(observer_, DidRedirectNavigation(web_state(), _));
+  EXPECT_CALL(observer_, DidRedirectNavigation(web_state(), _))
+      .WillOnce(VerifyVisibleURL(url));
   EXPECT_CALL(*decider_,
               MockShouldAllowRequest(
                   _, RequestInfoMatch(expected_redirect_request_info), _))
       .WillOnce(
           RunOnceCallback<2>(WebStatePolicyDecider::PolicyDecision::Allow()));
-  EXPECT_CALL(observer_, DidRedirectNavigation(web_state(), _));
+  EXPECT_CALL(observer_, DidRedirectNavigation(web_state(), _))
+      .WillOnce(VerifyVisibleURL(url));
 
   EXPECT_CALL(*decider_, ShouldAllowResponse(
                              _, ResponseInfoMatch(expected_response_info), _))
@@ -2112,6 +2123,7 @@
   EXPECT_CALL(observer_,
               PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
   ASSERT_TRUE(LoadUrl(url));
+  EXPECT_EQ(redirect_url, web_state()->GetVisibleURL());
 }
 
 // Tests download navigation.