Do not allow extension origins or URLs to commit in web processes.

This CL revives some of the checks from r512959 to ensure that most
extension URLs only commit in the correct extension processes.
There are several exceptions that must be accounted for.

BUG=770239, 840857

Change-Id: Id3dd2a7814041186d4de6f61e2dee440939b57d9
Reviewed-on: https://chromium-review.googlesource.com/1025075
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567799}

chrome/browser/chrome_security_exploit_browsertest.cc

@@ -24,6 +24,7 @@
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
+#include "extensions/common/extension_urls.h"
 #include "mojo/public/cpp/bindings/strong_associated_binding.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -51,13 +52,6 @@ class ChromeSecurityExploitBrowserTest
     extension_ = LoadExtension(test_data_dir_.AppendASCII("simple_with_icon"));
   }
 
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    extensions::ExtensionBrowserTest::SetUpCommandLine(command_line);
-    // Since we assume exploited renderer process, it can bypass the same origin
-    // policy at will. Simulate that by passing the disable-web-security flag.
-    command_line->AppendSwitch(switches::kDisableWebSecurity);
-  }
-
   const extensions::Extension* extension() { return extension_; }
 
   std::unique_ptr<content::BlobHandle> CreateMemoryBackedBlob(
@@ -86,7 +80,24 @@ class ChromeSecurityExploitBrowserTest
   DISALLOW_COPY_AND_ASSIGN(ChromeSecurityExploitBrowserTest);
 };
 
-IN_PROC_BROWSER_TEST_F(ChromeSecurityExploitBrowserTest,
+// Subclass of ChromeSecurityExploitBrowserTest that uses --disable-web-security
+// to simulate an exploited renderer.  Note that this also disables some browser
+// process checks, so it's not ideal for all exploit tests.
+class ChromeWebSecurityDisabledBrowserTest
+    : public ChromeSecurityExploitBrowserTest {
+ public:
+  ChromeWebSecurityDisabledBrowserTest() {}
+  ~ChromeWebSecurityDisabledBrowserTest() override {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ChromeSecurityExploitBrowserTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitch(switches::kDisableWebSecurity);
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeWebSecurityDisabledBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(ChromeWebSecurityDisabledBrowserTest,
                        ChromeExtensionResources) {
   // Load a page that requests a chrome-extension:// image through XHR. We
   // expect this load to fail, as it is an illegal request.
@@ -103,6 +114,131 @@ IN_PROC_BROWSER_TEST_F(ChromeSecurityExploitBrowserTest,
   EXPECT_STREQ(status.c_str(), expected_status.c_str());
 }
 
+// Tests that a normal web process cannot send a commit for a Chrome Web Store
+// URL.  See https://crbug.com/172119.
+IN_PROC_BROWSER_TEST_F(ChromeSecurityExploitBrowserTest,
+                       CommitWebStoreURLInWebProcess) {
+  GURL foo = embedded_test_server()->GetURL("foo.com", "/title1.html");
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::RenderFrameHost* rfh = web_contents->GetMainFrame();
+
+  // This IPC should result in a kill because the Chrome Web Store is not
+  // allowed to commit in |rfh->GetProcess()|.
+  base::HistogramTester histograms;
+  content::RenderProcessHostWatcher crash_observer(
+      rfh->GetProcess(),
+      content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+
+  // Modify an IPC for a commit of a blank URL, which would otherwise be allowed
+  // to commit in any process.
+  GURL blank_url = GURL(url::kAboutBlankURL);
+  GURL webstore_url = extension_urls::GetWebstoreLaunchURL();
+  content::PwnCommitIPC(web_contents, blank_url, webstore_url,
+                        url::Origin::Create(GURL(webstore_url)));
+  web_contents->GetController().LoadURL(
+      blank_url, content::Referrer(), ui::PAGE_TRANSITION_LINK, std::string());
+
+  // If the process is killed in CanCommitURL, this test passes.
+  crash_observer.Wait();
+  histograms.ExpectUniqueSample("Stability.BadMessageTerminated.Content", 1, 1);
+}
+
+// Tests that a non-extension process cannot send a commit of a blank URL with
+// an extension origin.
+IN_PROC_BROWSER_TEST_F(ChromeSecurityExploitBrowserTest,
+                       CommitExtensionOriginInWebProcess) {
+  GURL foo = embedded_test_server()->GetURL("foo.com", "/title1.html");
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::RenderFrameHost* rfh = web_contents->GetMainFrame();
+
+  // This IPC should result in a kill because |ext_origin| is not allowed to
+  // commit in |rfh->GetProcess()|.
+  base::HistogramTester histograms;
+  content::RenderProcessHostWatcher crash_observer(
+      rfh->GetProcess(),
+      content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+
+  // Modify an IPC for a commit of a blank URL, which would otherwise be allowed
+  // to commit in any process.
+  GURL blank_url = GURL(url::kAboutBlankURL);
+  std::string ext_origin = "chrome-extension://" + extension()->id();
+  content::PwnCommitIPC(web_contents, blank_url, blank_url,
+                        url::Origin::Create(GURL(ext_origin)));
+  web_contents->GetController().LoadURL(
+      blank_url, content::Referrer(), ui::PAGE_TRANSITION_LINK, std::string());
+
+  // If the process is killed in CanCommitOrigin, this test passes.
+  crash_observer.Wait();
+  histograms.ExpectUniqueSample("Stability.BadMessageTerminated.Content", 114,
+                                1);
+}
+
+// Tests that a non-extension process cannot send a commit of an extension URL.
+IN_PROC_BROWSER_TEST_F(ChromeSecurityExploitBrowserTest,
+                       CommitExtensionURLInWebProcess) {
+  GURL foo = embedded_test_server()->GetURL("foo.com", "/title1.html");
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::RenderFrameHost* rfh = web_contents->GetMainFrame();
+
+  // This IPC should result in a kill because extension URLs are not allowed to
+  // commit in |rfh->GetProcess()|.
+  base::HistogramTester histograms;
+  content::RenderProcessHostWatcher crash_observer(
+      rfh->GetProcess(),
+      content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+
+  // Modify an IPC for a commit of a blank URL, which would otherwise be allowed
+  // to commit in any process.
+  GURL blank_url = GURL(url::kAboutBlankURL);
+  std::string ext_origin = "chrome-extension://" + extension()->id();
+  content::PwnCommitIPC(web_contents, blank_url, GURL(ext_origin),
+                        url::Origin::Create(GURL(ext_origin)));
+  web_contents->GetController().LoadURL(
+      blank_url, content::Referrer(), ui::PAGE_TRANSITION_LINK, std::string());
+
+  // If the process is killed in CanCommitURL, this test passes.
+  crash_observer.Wait();
+  histograms.ExpectUniqueSample("Stability.BadMessageTerminated.Content", 1, 1);
+}
+
+// Tests that a non-extension process cannot send a commit of an extension
+// filesystem URL.
+IN_PROC_BROWSER_TEST_F(ChromeSecurityExploitBrowserTest,
+                       CommitExtensionFilesystemURLInWebProcess) {
+  GURL foo = embedded_test_server()->GetURL("foo.com", "/title1.html");
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::RenderFrameHost* rfh = web_contents->GetMainFrame();
+
+  // This IPC should result in a kill because extension filesystem URLs are not
+  // allowed to commit in |rfh->GetProcess()|.
+  base::HistogramTester histograms;
+  content::RenderProcessHostWatcher crash_observer(
+      rfh->GetProcess(),
+      content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+
+  // Modify an IPC for a commit of a blank URL, which would otherwise be allowed
+  // to commit in any process.
+  GURL blank_url = GURL(url::kAboutBlankURL);
+  std::string ext_origin = "chrome-extension://" + extension()->id();
+  content::PwnCommitIPC(web_contents, blank_url,
+                        GURL("filesystem:" + ext_origin + "/foo"),
+                        url::Origin::Create(GURL(ext_origin)));
+  web_contents->GetController().LoadURL(
+      blank_url, content::Referrer(), ui::PAGE_TRANSITION_LINK, std::string());
+
+  // If the process is killed in CanCommitURL, this test passes.
+  crash_observer.Wait();
+  histograms.ExpectUniqueSample("Stability.BadMessageTerminated.Content", 1, 1);
+}
+
 // Extension isolation prevents a normal renderer process from being able to
 // create a "blob:chrome-extension://" resource.
 IN_PROC_BROWSER_TEST_F(ChromeSecurityExploitBrowserTest,

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

@@ -13,6 +13,7 @@
 #include "base/debug/alias.h"
 #include "base/debug/dump_without_crashing.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/string_piece.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_web_ui.h"
@@ -54,6 +55,7 @@
 #include "extensions/browser/guest_view/web_view/web_view_renderer_state.h"
 #include "extensions/browser/info_map.h"
 #include "extensions/browser/io_thread_extension_message_filter.h"
+#include "extensions/browser/url_request_util.h"
 #include "extensions/browser/view_type_utils.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extensions_client.h"
@@ -418,25 +420,71 @@ bool ChromeContentBrowserClientExtensionsPart::CanCommitURL(
     content::RenderProcessHost* process_host, const GURL& url) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  // We need to let most extension URLs commit in any process, since this can
-  // be allowed due to web_accessible_resources.  Most hosted app URLs may also
-  // load in any process (e.g., in an iframe).  However, the Chrome Web Store
-  // cannot be loaded in iframes and should never be requested outside its
-  // process.
+  // Enforce that extension URLs commit in the correct extension process where
+  // possible, accounting for many exceptions to the rule.
+
+  // Don't bother if there is no registry.
+  // TODO(rdevlin.cronin): Can this be turned into a DCHECK?  Seems like there
+  // should always be a registry.
   ExtensionRegistry* registry =
       ExtensionRegistry::Get(process_host->GetBrowserContext());
   if (!registry)
     return true;
 
-  const Extension* new_extension =
+  // Only perform the checks below if the URL being committed has an extension
+  // associated with it.
+  const Extension* extension =
       registry->enabled_extensions().GetExtensionOrAppByURL(url);
-  if (new_extension && new_extension->is_hosted_app() &&
-      new_extension->id() == kWebStoreAppId &&
-      !ProcessMap::Get(process_host->GetBrowserContext())
-           ->Contains(new_extension->id(), process_host->GetID())) {
-    return false;
+  if (!extension)
+    return true;
+
+  // If the process is the dedicated process for this extension, then it's safe
+  // to commit.
+  if (ProcessMap::Get(process_host->GetBrowserContext())
+          ->Contains(extension->id(), process_host->GetID())) {
+    return true;
   }
-  return true;
+
+  // Most hosted apps (except for the Chrome Web Store) can commit anywhere.
+  // The Chrome Web Store should never commit outside its process, regardless of
+  // the other exceptions below.
+  if (extension->is_hosted_app())
+    return extension->id() != kWebStoreAppId;
+
+  // Some special case extension URLs must be allowed to load in any guest. Note
+  // that CanCommitURL may be called for validating origins as well, so do not
+  // enforce a path comparison in the special cases unless there is a real path
+  // (more than just "/").
+  // TODO(creis): Remove this call when bugs 688565 and 778021 are resolved.
+  base::StringPiece url_path = url.path_piece();
+  bool is_guest =
+      WebViewRendererState::GetInstance()->IsGuest(process_host->GetID());
+  if (is_guest &&
+      url_request_util::AllowSpecialCaseExtensionURLInGuest(
+          extension, url_path.length() > 1
+                         ? base::make_optional<base::StringPiece>(url_path)
+                         : base::nullopt)) {
+    return true;
+  }
+
+  // Platform app URLs may commit in their own guest processes, when they have
+  // the webview permission.  (Some extensions are allowlisted for webviews as
+  // well, but their pages load in their own extension process and are allowed
+  // through above.)
+  if (is_guest) {
+    std::string owner_extension_id;
+    int owner_process_id = -1;
+    bool found_owner = WebViewRendererState::GetInstance()->GetOwnerInfo(
+        process_host->GetID(), &owner_process_id, &owner_extension_id);
+    DCHECK(found_owner);
+    return extension->is_platform_app() &&
+           extension->permissions_data()->HasAPIPermission(
+               extensions::APIPermission::kWebView) &&
+           extension->id() == owner_extension_id;
+  }
+
+  // Otherwise, the process is wrong for this extension URL.
+  return false;
 }
 
 // static

chrome/browser/extensions/process_management_browsertest.cc

@@ -547,6 +547,13 @@ IN_PROC_BROWSER_TEST_F(ProcessManagementTest,
   EXPECT_TRUE(new_site_instance->HasProcess());
   EXPECT_EQ(new_site_instance->GetProcess(),
             web_contents->GetSiteInstance()->GetProcess());
+
+  // Ensure that reloading a blocked error page completes.
+  content::TestNavigationObserver reload_observer(new_web_contents);
+  new_web_contents->GetController().Reload(content::ReloadType::NORMAL, false);
+  reload_observer.Wait();
+  EXPECT_EQ(reload_observer.last_navigation_url(), blocked_url);
+  EXPECT_FALSE(reload_observer.last_navigation_succeeded());
 }
 
 // Check that whether we can access the window object of a window.open()'d url

content/browser/frame_host/navigation_request.cc

@@ -1144,10 +1144,6 @@ void NavigationRequest::OnRequestFailedInternal(
 
   DCHECK(render_frame_host);
 
-  // Don't ask the renderer to commit an URL if the browser will kill it when
-  // it does.
-  DCHECK(render_frame_host->CanCommitURL(common_params_.url));
-
   NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL(render_frame_host,
                                                            common_params_.url);
 

content/browser/frame_host/render_frame_host_impl.cc

@@ -4966,10 +4966,35 @@ bool RenderFrameHostImpl::ValidateDidCommitParams(
     FrameHostMsg_DidCommitProvisionalLoad_Params* validated_params) {
   RenderProcessHost* process = GetProcess();
 
+  // Error pages may sometimes commit a URL in the wrong process, which requires
+  // an exception for the CanCommitURL checks.  This is ok as long as the origin
+  // is unique.
+  // TODO(creis): Kill the renderer if it claims an error page has a non-unique
+  // origin.
+  bool is_permitted_error_page = false;
+  if (validated_params->origin.unique()) {
+    if (SiteIsolationPolicy::IsErrorPageIsolationEnabled(
+            frame_tree_node_->IsMainFrame())) {
+      // With error page isolation, any URL can commit in an error page process.
+      if (GetSiteInstance()->GetSiteURL() ==
+          GURL(content::kUnreachableWebDataURL)) {
+        is_permitted_error_page = true;
+      }
+    } else {
+      // Without error page isolation, a blocked navigation is expected to
+      // commit in the old renderer process.  This may be true for subframe
+      // navigations even when error page isolation is enabled for main frames.
+      if (GetNavigationHandle() && GetNavigationHandle()->GetNetErrorCode() ==
+                                       net::ERR_BLOCKED_BY_CLIENT) {
+        is_permitted_error_page = true;
+      }
+    }
+  }
+
   // Attempts to commit certain off-limits URL should be caught more strictly
   // than our FilterURL checks.  If a renderer violates this policy, it
   // should be killed.
-  if (!CanCommitURL(validated_params->url)) {
+  if (!is_permitted_error_page && !CanCommitURL(validated_params->url)) {
     VLOG(1) << "Blocked URL " << validated_params->url.spec();
     // Kills the process.
     bad_message::ReceivedBadMessage(process,

content/browser/security_exploit_browsertest.cc

@@ -525,9 +525,11 @@ IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, MismatchedOriginOnCommit) {
                             ->GetFrameTree()
                             ->root();
 
-  // Setup an URL which will never commit, allowing this test to send its own,
-  // malformed, commit message.
-  GURL url(embedded_test_server()->GetURL("/hung"));
+  // Navigate to a new URL, with an interceptor that replaces the origin with
+  // one that does not match params.url.
+  GURL url(embedded_test_server()->GetURL("/title2.html"));
+  PwnCommitIPC(shell()->web_contents(), url, url,
+               url::Origin::Create(GURL("http://bar.com/")));
 
   // Use LoadURL, as the test shouldn't wait for navigation commit.
   NavigationController& controller = shell()->web_contents()->GetController();
@@ -538,25 +540,6 @@ IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, MismatchedOriginOnCommit) {
   RenderProcessHostKillWaiter kill_waiter(
       root->current_frame_host()->GetProcess());
 
-  // Create commit params with different origins in params.url and
-  // params.origin.
-  std::unique_ptr<FrameHostMsg_DidCommitProvisionalLoad_Params> params =
-      std::make_unique<FrameHostMsg_DidCommitProvisionalLoad_Params>();
-  params->nav_entry_id = 0;
-  params->did_create_new_entry = false;
-  params->url = url;
-  params->transition = ui::PAGE_TRANSITION_LINK;
-  params->should_update_history = false;
-  params->gesture = NavigationGestureAuto;
-  params->method = "GET";
-  params->page_state = PageState::CreateFromURL(url);
-  params->origin = url::Origin::Create(GURL("http://bar.com/"));
-
-  service_manager::mojom::InterfaceProviderPtr interface_provider;
-  static_cast<mojom::FrameHost*>(root->current_frame_host())
-      ->DidCommitProvisionalLoad(std::move(params),
-                                 mojo::MakeRequest(&interface_provider));
-
   // When the IPC message is received and validation fails, the process is
   // terminated. However, the notification for that should be processed in a
   // separate task of the message loop, so ensure that the process is still

content/public/test/browser_test_utils.cc

@@ -82,6 +82,7 @@
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/test/accessibility_browser_test_utils.h"
+#include "content/test/did_commit_provisional_load_interceptor.h"
 #include "ipc/ipc_security_test_util.h"
 #include "net/base/filename_util.h"
 #include "net/base/io_buffer.h"
@@ -565,6 +566,43 @@ const char kHasVideoInputDeviceOnSystem[] = R"(
 
 const char kHasVideoInputDevice[] = "has-video-input-device";
 
+// Interceptor that replaces params.url with |new_url| and params.origin with
+// |new_origin| for any commits to |target_url|.
+class CommitOriginInterceptor : public DidCommitProvisionalLoadInterceptor {
+ public:
+  CommitOriginInterceptor(WebContents* web_contents,
+                          const GURL& target_url,
+                          const GURL& new_url,
+                          const url::Origin& new_origin)
+      : DidCommitProvisionalLoadInterceptor(web_contents),
+        target_url_(target_url),
+        new_url_(new_url),
+        new_origin_(new_origin) {}
+  ~CommitOriginInterceptor() override = default;
+
+  // WebContentsObserver:
+  void WebContentsDestroyed() override { delete this; }
+
+ protected:
+  void WillDispatchDidCommitProvisionalLoad(
+      RenderFrameHost* render_frame_host,
+      ::FrameHostMsg_DidCommitProvisionalLoad_Params* params,
+      service_manager::mojom::InterfaceProviderRequest*
+          interface_provider_request) override {
+    if (params->url == target_url_) {
+      params->url = new_url_;
+      params->origin = new_origin_;
+    }
+  }
+
+ private:
+  GURL target_url_;
+  GURL new_url_;
+  url::Origin new_origin_;
+
+  DISALLOW_COPY_AND_ASSIGN(CommitOriginInterceptor);
+};
+
 }  // namespace
 
 bool NavigateIframeToURL(WebContents* web_contents,
@@ -648,6 +686,14 @@ void CrashTab(WebContents* web_contents) {
   watcher.Wait();
 }
 
+void PwnCommitIPC(WebContents* web_contents,
+                  const GURL& target_url,
+                  const GURL& new_url,
+                  const url::Origin& new_origin) {
+  // This will be cleaned up when |web_contents| is destroyed.
+  new CommitOriginInterceptor(web_contents, target_url, new_url, new_origin);
+}
+
 void SimulateUnresponsiveRenderer(WebContents* web_contents,
                                   RenderWidgetHost* widget) {
   static_cast<WebContentsImpl*>(web_contents)

content/public/test/browser_test_utils.h

@@ -131,6 +131,14 @@ void OverrideLastCommittedOrigin(RenderFrameHost* render_frame_host,
 // Causes the specified web_contents to crash. Blocks until it is crashed.
 void CrashTab(WebContents* web_contents);
 
+// Sets up a commit interceptor to alter commits for |target_url| to change
+// their commit URL to |new_url| and origin to |new_origin|. This will happen
+// for all commits in |web_contents|.
+void PwnCommitIPC(WebContents* web_contents,
+                  const GURL& target_url,
+                  const GURL& new_url,
+                  const url::Origin& new_origin);
+
 // Causes the specified web_contents to issue an OnUnresponsiveRenderer event
 // to its observers.
 void SimulateUnresponsiveRenderer(WebContents* web_contents,

extensions/browser/url_request_util.cc

@@ -30,7 +30,7 @@ bool AllowCrossRendererResourceLoad(const GURL& url,
                                     const ExtensionSet& extensions,
                                     const ProcessMap& process_map,
                                     bool* allowed) {
-  std::string resource_path = url.path();
+  base::StringPiece resource_path = url.path_piece();
 
   // This logic is performed for main frame requests in
   // ExtensionNavigationThrottle::WillStartRequest.
@@ -94,8 +94,8 @@ bool AllowCrossRendererResourceLoad(const GURL& url,
     }
 
     // Also allow if the file is explicitly listed as a web_accessible_resource.
-    if (WebAccessibleResourcesInfo::IsResourceWebAccessible(extension,
-                                                            resource_path)) {
+    if (WebAccessibleResourcesInfo::IsResourceWebAccessible(
+            extension, resource_path.as_string())) {
       *allowed = true;
       return true;
     }
@@ -123,40 +123,11 @@ bool AllowCrossRendererResourceLoadHelper(bool is_guest,
                                           const Extension* extension,
                                           const Extension* owner_extension,
                                           const std::string& partition_id,
-                                          const std::string& resource_path,
+                                          base::StringPiece resource_path,
                                           ui::PageTransition page_transition,
                                           bool* allowed) {
   if (is_guest) {
-    // Exceptionally, the resource at path "/success.html" that belongs to the
-    // sign-in extension (loaded by chrome://chrome-signin) is accessible to
-    // WebViews that are not owned by that extension.
-    // This exception is required as in order to mark the end of the the sign-in
-    // flow, Gaia redirects to the following continue URL:
-    // "chrome-extension://mfffpogegjflfpflabcdkioaeobkgjik/success.html".
-    //
-    // TODO(http://crbug.com/688565) Remove this check once the sign-in
-    // extension is deprecated and removed.
-    bool is_signin_extension =
-        extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik";
-    if (is_signin_extension && resource_path == "/success.html") {
-      *allowed = true;
-      return true;
-    }
-
-    // Allow mobile setup web UI (chrome://mobilesetup) to embed resources from
-    // the component mobile activation extension in a webview. This is needed
-    // because the activation web UI relies on the activation extension to
-    // provide parts of its UI, and to redirect POST requests to the network
-    // payment URL during mobile device initialization.
-    //
-    // TODO(http://crbug.com/778021): Fix mobile activation UI not to require
-    // this workaround.
-    bool is_mobile_activation_extension =
-        extension && extension->id() == "iadeocfgjdjdmpenejdbfeaocpbikmab";
-    if (is_mobile_activation_extension &&
-        (resource_path == "/activation.html" ||
-         resource_path == "/portal_offline.html" ||
-         resource_path == "/invalid_device_info.html")) {
+    if (AllowSpecialCaseExtensionURLInGuest(extension, resource_path)) {
       *allowed = true;
       return true;
     }
@@ -168,13 +139,56 @@ bool AllowCrossRendererResourceLoadHelper(bool is_guest,
       return true;
     }
 
-    *allowed = WebviewInfo::IsResourceWebviewAccessible(extension, partition_id,
-                                                        resource_path);
+    *allowed = WebviewInfo::IsResourceWebviewAccessible(
+        extension, partition_id, resource_path.as_string());
     return true;
   }
 
   return false;
 }
 
+bool AllowSpecialCaseExtensionURLInGuest(
+    const Extension* extension,
+    base::Optional<base::StringPiece> resource_path) {
+  // Exceptionally, the resource at path "/success.html" that belongs to the
+  // sign-in extension (loaded by chrome://chrome-signin) is accessible to
+  // WebViews that are not owned by that extension.
+  // This exception is required as in order to mark the end of the the sign-in
+  // flow, Gaia redirects to the following continue URL:
+  // "chrome-extension://mfffpogegjflfpflabcdkioaeobkgjik/success.html".
+  //
+  // TODO(http://crbug.com/688565) Remove this check once the sign-in
+  // extension is deprecated and removed.
+  bool is_signin_extension =
+      extension && extension->id() == "mfffpogegjflfpflabcdkioaeobkgjik";
+  if (is_signin_extension && (!resource_path.has_value() ||
+                              resource_path.value() == "/success.html")) {
+    return true;
+  }
+
+  // Allow mobile setup web UI (chrome://mobilesetup) to embed resources from
+  // the component mobile activation extension in a webview. This is needed
+  // because the activation web UI relies on the activation extension to
+  // provide parts of its UI, and to redirect POST requests to the network
+  // payment URL during mobile device initialization.
+  //
+  // TODO(http://crbug.com/778021): Fix mobile activation UI not to require
+  // this workaround.
+  bool is_mobile_activation_extension =
+      extension && extension->id() == "iadeocfgjdjdmpenejdbfeaocpbikmab";
+  if (is_mobile_activation_extension) {
+    if (!resource_path.has_value())
+      return true;
+    if (resource_path.value() == "/activation.html" ||
+        resource_path.value() == "/portal_offline.html" ||
+        resource_path.value() == "/invalid_device_info.html") {
+      return true;
+    }
+  }
+
+  // Otherwise this isn't a special case, and the normal logic should apply.
+  return false;
+}
+
 }  // namespace url_request_util
 }  // namespace extensions

extensions/browser/url_request_util.h

@@ -7,6 +7,8 @@
 
 #include <string>
 
+#include "base/optional.h"
+#include "base/strings/string_piece.h"
 #include "content/public/common/resource_type.h"
 #include "ui/base/page_transition_types.h"
 
@@ -47,10 +49,22 @@ bool AllowCrossRendererResourceLoadHelper(bool is_guest,
                                           const Extension* extension,
                                           const Extension* owner_extension,
                                           const std::string& partition_id,
-                                          const std::string& resource_path,
+                                          base::StringPiece resource_path,
                                           ui::PageTransition page_transition,
                                           bool* allowed);
 
+// Checks whether the given |extension| and |resource_path| are part of a
+// special case where an extension URL is permitted to load in any guest
+// process, rather than only in guests of a given platform app. If
+// |resource_path| is base::nullopt, then the check is based solely on which
+// extension is passed in, allowing this to be used for origin checks as well as
+// URL checks.
+// TODO(creis): Remove this method when the special cases (listed by bug number
+// in the definition of this method) are gone.
+bool AllowSpecialCaseExtensionURLInGuest(
+    const Extension* extension,
+    base::Optional<base::StringPiece> resource_path);
+
 }  // namespace url_request_util
 }  // namespace extensions