Fix web accessible resource checks in ShouldAllowOpenURL

chrome/browser/extensions/window_open_apitest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/result_codes.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/process_manager.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/test/extension_test_message_listener.h"
@@ skipping 249 matching lines @@
                      last_loaded_extension_id() + "/newtab.html"),
                  false,
                  &newtab));
 
   // Extension API should succeed.
   bool result = false;
   ASSERT_TRUE(content::ExecuteScriptAndExtractBool(newtab, "testExtensionApi()",
                                                    &result));
   EXPECT_TRUE(result);
 }
+
+// Tests that calling window.open for an extension URL from a non-HTTP or HTTPS
+// URL on a new tab cannot access non-web-accessible resources.
+IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest,
+                       WindowOpenInaccessibleResourceFromDataURL) {

[alexmos, 2016/10/28 00:29:41]

This is checking the case that would've previously been incorrectly allowed by
ShouldAllowOpenURL due to the SchemeIsHTTPOrHTTPS() restriction.

+  ASSERT_TRUE(LoadExtension(
+      test_data_dir_.AppendASCII("uitest").AppendASCII("window_open")));
+
+  ui_test_utils::NavigateToURL(browser(), GURL("data:text/html,foo"));
+
+  // test.html is not web-accessible and should not be loaded.
+  GURL extension_url(std::string(extensions::kExtensionScheme) +
+                     url::kStandardSchemeSeparator +
+                     last_loaded_extension_id() + "/test.html");
+  content::WindowedNotificationObserver windowed_observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::NotificationService::AllSources());
+  ASSERT_TRUE(content::ExecuteScript(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      "window.open('" + extension_url.spec() + "');"));
+  windowed_observer.Wait();
+  content::NavigationController* controller =
+      content::Source<content::NavigationController>(windowed_observer.source())
+          .ptr();
+  content::WebContents* newtab = controller->GetWebContents();
+  ASSERT_TRUE(newtab);
+
+  EXPECT_NE(extension_url, newtab->GetMainFrame()->GetLastCommittedURL());
+  EXPECT_NE(std::string(extensions::kExtensionScheme),
+            newtab->GetMainFrame()->GetSiteInstance()->GetSiteURL().scheme());

[ncarter (slow), 2016/10/28 21:45:03]

If you add UMA stats, you could use a histogram_tester here to validate that we
blocked it for the reason you expect.

[alexmos, 2016/10/31 23:34:48]

Done.  Never used histogram_tester before, so wasn't sure whether it was worth
exposing the ShouldAllowOpenURLFailureReason enum values to tests or just using
hardcoded values.  I did the latter for now, as the enum covers such obscure
cases that I'm not sure it's worth exposing more widely, but also happy to do
the former (is there a good place for the enum?).

[ncarter (slow), 2016/10/31 23:42:09]

Hardcoded values are the way to go. It's arguably a layering violation, but I
actually kind of like this kind of deliberate brittleness for "tests in chrome/
that are really tests of logic in content/".

+}