Make interstitial links open in a new tab

chrome/browser/ssl/ssl_browser_tests.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 <utility>
 
 #include "base/base64.h"
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 46 matching lines @@
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/test_launcher_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/network_session_configurator/common/network_switches.h"
 #include "components/network_time/network_time_test_utils.h"
 #include "components/network_time/network_time_tracker.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
+#include "components/security_interstitials/content/security_interstitial_controller_client.h"
 #include "components/security_interstitials/core/controller_client.h"
 #include "components/security_interstitials/core/metrics_helper.h"
 #include "components/security_state/core/security_state.h"
 #include "components/security_state/core/switches.h"
 #include "components/ssl_errors/error_classification.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/variations/variations_associated_data.h"
 #include "components/variations/variations_switches.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/browser_context.h"
@@ skipping 51 matching lines @@
 #include "net/cert/nss_cert_database.h"
 #endif  // defined(USE_NSS_CERTS)
 
 using base::ASCIIToUTF16;
 using chrome_browser_interstitials::SecurityInterstitialIDNTest;
 using content::InterstitialPage;
 using content::NavigationController;
 using content::NavigationEntry;
 using content::SSLStatus;
 using content::WebContents;
+using security_interstitials::SecurityInterstitialControllerClient;
 using web_modal::WebContentsModalDialogManager;
 
 const base::FilePath::CharType kDocRoot[] =
     FILE_PATH_LITERAL("chrome/test/data");
 
 namespace {
 
 enum ProceedDecision {
   SSL_INTERSTITIAL_PROCEED,
   SSL_INTERSTITIAL_DO_NOT_PROCEED
@@ skipping 508 matching lines @@
       // Check that the mock reporter received a request to send a report.
       run_loop.Run();
       EXPECT_EQ(https_server_expired_.GetURL("/title1.html").host(),
                 reporter_callback.GetLatestHostnameReported());
     } else {
       base::RunLoop().RunUntilIdle();
       EXPECT_EQ(std::string(), reporter_callback.GetLatestHostnameReported());
     }
   }
 
+  // Helper function for TestInterstitialLinksOpenInNewTab.

[estark, 2017/06/27 00:36:21]

nit: perhaps add a sentence to explain why this is a helper method instead of
just inlined below, for example: "Implemented as a test fixture method because
the whole test fixture class is friended by SSLBlockingPage."

[sperigo, 2017/06/27 17:31:59]

Good idea!

+  security_interstitials::SecurityInterstitialControllerClient*
+  GetControllerClientFromInterstitialPage(SSLBlockingPage* ssl_interstitial) {
+    return ssl_interstitial->controller();
+  }
+
   net::EmbeddedTestServer https_server_;
   net::EmbeddedTestServer https_server_expired_;
   net::EmbeddedTestServer https_server_mismatched_;
   net::SpawnedTestServer wss_server_expired_;
   net::SpawnedTestServer wss_server_mismatched_;
 
  protected:
   // Navigates to an interstitial and clicks through the certificate
   // error; then navigates to a page at |path| that loads unsafe content.
   void SetUpUnsafeContentsWithUserException(const std::string& path) {
@@ skipping 2243 matching lines @@
 
   content::WindowedNotificationObserver observer(
       content::NOTIFICATION_LOAD_STOP,
       content::Source<NavigationController>(&tab->GetController()));
   InterstitialPage* interstitial_page = tab->GetInterstitialPage();
   ASSERT_EQ(SSLBlockingPage::kTypeForTesting,
             interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
   content::RenderViewHost* interstitial_rvh =
       interstitial_page->GetMainFrame()->GetRenderViewHost();
   int result = -1;
-  std::string javascript =
+  const std::string javascript =
       base::StringPrintf("window.domAutomationController.send(%d);",
                          security_interstitials::CMD_PROCEED);
   ASSERT_TRUE(content::ExecuteScriptAndExtractInt(
               interstitial_rvh, javascript, &result));
   // The above will hang without the fix.
   EXPECT_EQ(1, result);
   observer.Wait();
   CheckAuthenticationBrokenState(
       tab, net::CERT_STATUS_DATE_INVALID, AuthState::NONE);
 }
@@ skipping 14 matching lines @@
 
   content::WindowedNotificationObserver observer(
       content::NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED,
       content::NotificationService::AllSources());
   InterstitialPage* interstitial_page = tab->GetInterstitialPage();
   ASSERT_EQ(SSLBlockingPage::kTypeForTesting,
             interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
   content::RenderViewHost* interstitial_rvh =
       interstitial_page->GetMainFrame()->GetRenderViewHost();
   int result = -1;
-  std::string javascript =
+  const std::string javascript =
       base::StringPrintf("window.domAutomationController.send(%d);",
                          security_interstitials::CMD_DONT_PROCEED);
   ASSERT_TRUE(content::ExecuteScriptAndExtractInt(
       interstitial_rvh, javascript, &result));
   // The above will hang without the fix.
   EXPECT_EQ(0, result);
   observer.Wait();
   EXPECT_EQ("about:blank", tab->GetVisibleURL().spec());
 }
 
+// Verifies that links in the interstitial open in a new tab.
+// https://crbug.com/717616
+IN_PROC_BROWSER_TEST_F(SSLUITest, TestInterstitialLinksOpenInNewTab) {
+  ASSERT_TRUE(https_server_.Start());
+  ASSERT_TRUE(https_server_expired_.Start());
+
+  WebContents* interstitial_tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ui_test_utils::NavigateToURL(
+      browser(), https_server_expired_.GetURL("/ssl/google.html"));
+  content::WaitForInterstitialAttach(
+      browser()->tab_strip_model()->GetActiveWebContents());
+  InterstitialPage* interstitial_page = interstitial_tab->GetInterstitialPage();
+  ASSERT_TRUE(
+      content::WaitForRenderFrameReady(interstitial_page->GetMainFrame()));
+  CheckAuthenticationBrokenState(interstitial_tab,
+                                 net::CERT_STATUS_DATE_INVALID,
+                                 AuthState::SHOWING_INTERSTITIAL);
+  ASSERT_EQ(SSLBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+
+  content::TestNavigationObserver nav_observer(nullptr);
+  nav_observer.StartWatchingNewWebContents();
+
+  SSLBlockingPage* ssl_interstitial =
+      static_cast<SSLBlockingPage*>(interstitial_page->GetDelegateForTesting());
+  security_interstitials::SecurityInterstitialControllerClient* client =
+      GetControllerClientFromInterstitialPage(ssl_interstitial);
+
+  // Mocking out the help center URL so that our test will hit the test server

[estark, 2017/06/27 00:36:21]

teeny-tiny nit: "Mocking" => "Mock"

[sperigo, 2017/06/27 17:31:59]

Done.

+  // instead of a real server.
+  // NOTE: The CMD_OPEN_HELP_CENTER code in
+  // components/security_interstitials/core/ssl_error_ui.cc ends up appending
+  // a path to whatever URL is passed to it. Since that path doesn't exist on
+  // our test server, this results in a 404. This is expected behavior, and
+  // things are still working as expected so long as the test passes!
+  const GURL mock_help_center_url = https_server_.GetURL("/title1.html");

[estark, 2017/06/27 00:36:21]

nit: const GURL&

(otherwise it'll make an unnecessary copy)

[meacer, 2017/06/27 00:41:46]

I don't think you want a reference here, it'll be a ref to a temp variable so
copying is correct. Consider this a "GURL mock_help_center_url" but with a
const.

[estark, 2017/06/27 00:45:46]

Oops, that's right, my bad!

[sperigo, 2017/06/27 17:31:59]

Acknowledged.

[sperigo, 2017/06/27 17:31:59]

Acknowledged.

[sperigo, 2017/06/27 17:31:59]

Acknowledged.

+  client->SetBaseHelpCenterUrlForTesting(mock_help_center_url);
+
+  EXPECT_EQ(1, browser()->tab_strip_model()->count());
+
+  int result = -1;
+  const std::string javascript =
+      base::StringPrintf("window.domAutomationController.send(%d);",
+                         security_interstitials::CMD_OPEN_HELP_CENTER);
+  ASSERT_TRUE(content::ExecuteScriptAndExtractInt(
+      interstitial_page->GetMainFrame(), javascript, &result));
+  EXPECT_EQ(security_interstitials::CMD_OPEN_HELP_CENTER, result);
+
+  nav_observer.Wait();
+
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+  WebContents* new_tab = browser()->tab_strip_model()->GetActiveWebContents();

[estark, 2017/06/27 00:36:21]

nit: ASSERT_TRUE(new_tab) after this line

[sperigo, 2017/06/27 17:31:59]

Done.

+  // This path is the one mentioned above that is appended in ssl_error_ui.cc.
+  EXPECT_EQ(mock_help_center_url.Resolve("answer/6098869"), new_tab->GetURL());

[estark, 2017/06/27 00:36:22]

For a slightly less brittle test, you could do:

EXPECT_EQ(mock_help_center_url.host(), new_tab->GetURL().host());

That'll check that the help center url was to the test server as expected, but
someone can change the path without having to update the test.

[sperigo, 2017/06/27 17:31:59]

Good idea! Thanks.

+}
+
 // Verifies that switching tabs, while showing interstitial page, will not
-// affect the visibility of the interestitial.
+// affect the visibility of the interstitial.
 // https://crbug.com/381439
 IN_PROC_BROWSER_TEST_F(SSLUITest, InterstitialNotAffectedByHideShow) {
   ASSERT_TRUE(https_server_expired_.Start());
   ASSERT_TRUE(https_server_.Start());
 
   WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
   EXPECT_TRUE(tab->GetRenderWidgetHostView()->IsShowing());
   ui_test_utils::NavigateToURL(
       browser(), https_server_expired_.GetURL("/ssl/google.html"));
   CheckAuthenticationBrokenState(
@@ skipping 1959 matching lines @@
 
 // Visit a page over https that contains a frame with a redirect.
 
 // XMLHttpRequest insecure content in synchronous mode.
 
 // XMLHttpRequest insecure content in asynchronous mode.
 
 // XMLHttpRequest over bad ssl in synchronous mode.
 
 // XMLHttpRequest over OK ssl in synchronous mode.