Remove ContentBrowserClient::IsSafeRedirectTarget.

The deleted ChromeContentBrowserClient::IsSafeRedirectTarget was mostly
redundant wrt ExtensionNavigationThrottle ("mostly", because the
throttle considers only the navigation initiator - doesn't consider
origins of intermediate hops as initiators;  this aspect of behavior is
not important to preserve).

Bug: 442579
Change-Id: I617510e425c0d944a114c9b4ea749d0d31b51d6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1779310
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701730}

chrome/browser/chrome_content_browser_client.cc

@@ -5083,23 +5083,6 @@ ChromeContentBrowserClient::CreateWindowForPictureInPicture(
   return content::OverlayWindow::Create(controller);
 }
 
-bool ChromeContentBrowserClient::IsSafeRedirectTarget(
-    const GURL& url,
-    content::BrowserContext* context) {
-#if BUILDFLAG(ENABLE_EXTENSIONS)
-  if (url.SchemeIs(extensions::kExtensionScheme)) {
-    const Extension* extension = extensions::ExtensionRegistry::Get(context)
-                                     ->enabled_extensions()
-                                     .GetByID(url.host());
-    if (!extension)
-      return false;
-    return extensions::WebAccessibleResourcesInfo::IsResourceWebAccessible(
-        extension, url.path());
-  }
-#endif  // BUILDFLAG(ENABLE_EXTENSIONS)
-  return true;
-}
-
 void ChromeContentBrowserClient::RegisterRendererPreferenceWatcher(
     content::BrowserContext* browser_context,
     mojo::PendingRemote<blink::mojom::RendererPreferenceWatcher> watcher) {

chrome/browser/chrome_content_browser_client.h

@@ -545,8 +545,6 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
       network::mojom::URLLoaderFactoryPtr* out_factory) override;
   std::unique_ptr<content::OverlayWindow> CreateWindowForPictureInPicture(
       content::PictureInPictureWindowController* controller) override;
-  bool IsSafeRedirectTarget(const GURL& url,
-                            content::BrowserContext* context) override;
   void RegisterRendererPreferenceWatcher(
       content::BrowserContext* browser_context,
       mojo::PendingRemote<blink::mojom::RendererPreferenceWatcher> watcher)

chrome/browser/extensions/extension_navigation_throttle_unittest.cc

@@ -67,8 +67,7 @@ class ExtensionNavigationThrottleUnitTest
 
   // Checks that trying to navigate the given |host| to |extension_url| results
   // in the |expected_will_start_result|, and also that navigating to
-  // |extension_url| via http redirect will cancel the request unless
-  // |expected_will_start_result| is PROCEED.
+  // |extension_url| via http redirect gives the same result.
   void CheckTestCase(
       content::RenderFrameHost* host,
       const GURL& extension_url,
@@ -87,18 +86,11 @@ class ExtensionNavigationThrottleUnitTest
     GURL http_url("https://example.com");
     test_handle.set_url(http_url);
 
-    // TODO(nick): https://crbug.com/695421 It should be possible to support
-    // return values other than PROCEED and CANCEL from
-    // ExtensionNavigationThrottle::WillRedirectRequest.
-    NavigationThrottle::ThrottleAction expected_will_redirect_result =
-        (expected_will_start_result == NavigationThrottle::PROCEED)
-            ? NavigationThrottle::PROCEED
-            : NavigationThrottle::CANCEL;
     EXPECT_EQ(NavigationThrottle::PROCEED,
               throttle->WillStartRequest().action())
         << http_url;
     test_handle.set_url(extension_url);
-    EXPECT_EQ(expected_will_redirect_result,
+    EXPECT_EQ(expected_will_start_result,
               throttle->WillRedirectRequest().action())
         << extension_url;
   }

chrome/browser/extensions/process_manager_browsertest.cc

@@ -1390,8 +1390,8 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
       "/server-redirect?" + inaccessible_extension_resource.spec()));
   content::WebContents* sneaky_popup =
       OpenPopup(main_frame, redirect_to_inaccessible, false);
-  EXPECT_EQ(redirect_to_inaccessible,
-            sneaky_popup->GetLastCommittedURL().spec());
+  EXPECT_EQ(inaccessible_extension_resource,
+            sneaky_popup->GetLastCommittedURL());
   EXPECT_EQ(
       content::PAGE_TYPE_ERROR,
       sneaky_popup->GetController().GetLastCommittedEntry()->GetPageType());
@@ -1401,8 +1401,8 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
   // Adding "noopener" to the navigation shouldn't make it work either.
   content::WebContents* sneaky_noopener_popup =
       OpenPopupNoOpener(main_frame, redirect_to_inaccessible);
-  EXPECT_EQ(redirect_to_inaccessible,
-            sneaky_noopener_popup->GetLastCommittedURL().spec());
+  EXPECT_EQ(inaccessible_extension_resource,
+            sneaky_noopener_popup->GetLastCommittedURL());
   EXPECT_EQ(content::PAGE_TYPE_ERROR, sneaky_noopener_popup->GetController()
                                           .GetLastCommittedEntry()
                                           ->GetPageType());
@@ -1467,17 +1467,18 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
   const GURL sneaky_extension2_manifest(embedded_test_server()->GetURL(
       "/server-redirect?" + extension2_manifest.spec()));
   {
-    content::RenderFrameDeletedObserver frame_deleted_observer(
-        ChildFrameAt(main_frame, 1));
+    content::TestNavigationObserver nav_observer(tab, 1);
     EXPECT_TRUE(ExecuteScript(
         tab, base::StringPrintf("frames[1].location.href = '%s';",
                                 sneaky_extension2_manifest.spec().c_str())));
-    WaitForLoadStop(tab);
-    frame_deleted_observer.WaitUntilDeleted();
-    EXPECT_EQ(sneaky_extension2_manifest,
-              ChildFrameAt(main_frame, 1)->GetLastCommittedURL())
+    nav_observer.Wait();
+    EXPECT_FALSE(nav_observer.last_navigation_succeeded())
         << "The initial navigation should be allowed, but not the server "
            "redirect to extension2's manifest";
+    EXPECT_EQ(net::ERR_BLOCKED_BY_CLIENT, nav_observer.last_net_error_code());
+    EXPECT_EQ(extension2_manifest, nav_observer.last_navigation_url());
+    EXPECT_EQ(extension2_manifest,
+              ChildFrameAt(main_frame, 1)->GetLastCommittedURL());
     EXPECT_EQ(1u, pm->GetAllFrames().size());
     EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension1->id()).size());
     EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());

content/browser/loader/navigation_url_loader_impl.cc

@@ -4,7 +4,9 @@
 
 #include "content/browser/loader/navigation_url_loader_impl.h"
 
+#include <map>
 #include <memory>
+#include <set>
 #include <utility>
 
 #include "base/bind.h"
@@ -276,15 +278,6 @@ void UnknownSchemeCallback(
       handled_externally ? net::ERR_ABORTED : net::ERR_UNKNOWN_URL_SCHEME));
 }
 
-// Determines whether it is safe to redirect from |from_url| to |to_url|.
-bool IsRedirectSafe(const GURL& from_url,
-                    const GURL& to_url,
-                    BrowserContext* browser_context) {
-  return IsSafeRedirectTarget(from_url, to_url) &&
-         GetContentClient()->browser()->IsSafeRedirectTarget(to_url,
-                                                             browser_context);
-}
-
 }  // namespace
 
 // Kept around during the lifetime of the navigation request, and is
@@ -919,7 +912,7 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
   void OnReceiveRedirect(const net::RedirectInfo& redirect_info,
                          network::mojom::URLResponseHeadPtr head) override {
     if (!bypass_redirect_checks_ &&
-        !IsRedirectSafe(url_, redirect_info.new_url, browser_context_)) {
+        !IsSafeRedirectTarget(url_, redirect_info.new_url)) {
       // Call CancelWithError instead of OnComplete so that if there is an
       // intercepting URLLoaderFactory (created through the embedder's
       // ContentBrowserClient::WillCreateURLLoaderFactory) it gets notified.

content/public/browser/content_browser_client.cc

@@ -902,11 +902,6 @@ ContentBrowserClient::CreateWindowForPictureInPicture(
   return nullptr;
 }
 
-bool ContentBrowserClient::IsSafeRedirectTarget(const GURL& url,
-                                                BrowserContext* context) {
-  return true;
-}
-
 void ContentBrowserClient::RegisterRendererPreferenceWatcher(
     BrowserContext* browser_context,
     mojo::PendingRemote<blink::mojom::RendererPreferenceWatcher> watcher) {

content/public/browser/content_browser_client.h

@@ -1545,10 +1545,6 @@ class CONTENT_EXPORT ContentBrowserClient {
   virtual std::unique_ptr<OverlayWindow> CreateWindowForPictureInPicture(
       PictureInPictureWindowController* controller);
 
-  // Returns true if it is safe to redirect to |url|, otherwise returns false.
-  // This is called on the UI thread.
-  virtual bool IsSafeRedirectTarget(const GURL& url, BrowserContext* context);
-
   // Registers the watcher to observe updates in RendererPreferences.
   virtual void RegisterRendererPreferenceWatcher(
       BrowserContext* browser_context,

extensions/browser/extension_navigation_throttle.cc

@@ -4,6 +4,8 @@
 
 #include "extensions/browser/extension_navigation_throttle.h"
 
+#include <string>
+
 #include "components/guest_view/browser/guest_view_base.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
@@ -185,13 +187,7 @@ ExtensionNavigationThrottle::WillStartRequest() {
 
 content::NavigationThrottle::ThrottleCheckResult
 ExtensionNavigationThrottle::WillRedirectRequest() {
-  ThrottleCheckResult result = WillStartOrRedirectRequest();
-  if (result.action() == BLOCK_REQUEST) {
-    // TODO(nick): https://crbug.com/695421 means that BLOCK_REQUEST does not
-    // work here. Just return |result|.
-    return CANCEL;
-  }
-  return result;
+  return WillStartOrRedirectRequest();
 }
 
 const char* ExtensionNavigationThrottle::GetNameForLogging() {