Adjust HTTP-bad console messages

chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc

Index: chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
diff --git a/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc b/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
index f70f3d98f8b6982a82fce54d897bfc16fe50c0bc..e588b5d27c040d887eaadf0dcfe67151b0e201d8 100644
--- a/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
+++ b/chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
@@ -1169,7 +1169,19 @@ void CheckForOneHttpWarningConsoleMessage(
   const std::vector<base::string16>& messages = delegate->console_messages();
   ASSERT_EQ(1u, messages.size());
   EXPECT_NE(base::string16::npos,
-            messages[0].find(base::ASCIIToUTF16("not secure")));
+            messages[0].find(base::ASCIIToUTF16("warning has been added")));
+}
+
+// Checks that |delegate| has observed exactly one console message for
+// NONE that will be HTTP_SHOW_WARNING in future. This does not check
+// for the exact string (for fear of being too brittle) but rather just
+// a keyword ("not secure" and "M56").

[elawrence, 2016/10/20 17:18:29]

This comment is no longer correct. Given the shortness of the test, maybe just
say "To avoid brittleness, we look for keywords in the warning rather than the
exact text" or something?

[estark, 2016/10/20 17:45:03]

Done.

+void CheckForOneFutureHttpWarningConsoleMessage(
+    ConsoleWebContentsDelegate* delegate) {
+  const std::vector<base::string16>& messages = delegate->console_messages();
+  ASSERT_EQ(1u, messages.size());
+  EXPECT_NE(base::string16::npos,
+            messages[0].find(base::ASCIIToUTF16("warning will be added")));
 }
 
 // Tests that console messages are printed upon a call to
@@ -1243,6 +1255,80 @@ IN_PROC_BROWSER_TEST_F(ChromeSecurityStateModelClientTestWithPasswordCcSwitch,
   ASSERT_NO_FATAL_FAILURE(CheckForOneHttpWarningConsoleMessage(delegate));
 }
 
+// Tests that console messages are printed upon a call to
+// GetSecurityInfo() on a NONE page that will be marked
+// HTTP_SHOW_WARNING in future, exactly once per main-frame navigation.
+IN_PROC_BROWSER_TEST_F(ChromeSecurityStateModelClientTest, ConsoleMessage) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  host_resolver()->AddRule("*", embedded_test_server()->GetURL("/").host());
+  ConsoleWebContentsDelegate* delegate = new ConsoleWebContentsDelegate(
+      Browser::CreateParams(browser()->profile()));
+  content::WebContents* original_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::WebContents* contents =
+      content::WebContents::Create(content::WebContents::CreateParams(
+          original_contents->GetBrowserContext()));
+  ASSERT_TRUE(contents);
+  contents->SetDelegate(delegate);
+  delegate->tab_strip_model()->AppendWebContents(contents, true);
+  int index = delegate->tab_strip_model()->GetIndexOfWebContents(contents);
+  delegate->tab_strip_model()->ActivateTabAt(index, true);
+  ASSERT_EQ(contents, delegate->tab_strip_model()->GetActiveWebContents());
+
+  // Navigate to an HTTP page. Use a non-local hostname so that is it
+  // not considered secure.
+  GURL http_url =
+      GetURLWithNonLocalHostname(embedded_test_server(), "/title1.html");
+  ui_test_utils::NavigateToURL(delegate, http_url);
+  content::NavigationEntry* entry = contents->GetController().GetVisibleEntry();
+  ASSERT_TRUE(entry);
+  EXPECT_EQ(http_url, entry->GetURL());
+  EXPECT_TRUE(delegate->console_messages().empty());
+
+  // Trigger the a state that will be marked as HTTP_SHOW_WARNING in future.
+  base::RunLoop first_message;
+  delegate->set_console_message_callback(first_message.QuitClosure());
+  contents->OnPasswordInputShownOnHttp();
+  first_message.Run();
+
+  // Check that the correct state was actually triggered.
+  ChromeSecurityStateModelClient* client =
+      ChromeSecurityStateModelClient::FromWebContents(contents);
+  ASSERT_TRUE(client);
+  security_state::SecurityStateModel::SecurityInfo security_info;
+  client->GetSecurityInfo(&security_info);
+  EXPECT_EQ(security_state::SecurityStateModel::NONE,
+            security_info.security_level);
+  EXPECT_TRUE(security_info.displayed_private_user_data_input_on_http);
+
+  // Check that the expected console message is present.
+  ASSERT_NO_FATAL_FAILURE(CheckForOneFutureHttpWarningConsoleMessage(delegate));
+  delegate->ClearConsoleMessages();
+
+  // Two subsequent triggers of VisibleSSLStateChanged -- one on the
+  // same navigation and one on another navigation -- should only result
+  // in one additional console message.

[elawrence, 2016/10/20 17:18:29]

Can we assert that this second trigger on the same page (without navigation)
does not add any message to the console? E.g. by changing this code to:

  // Fire a second notification
  contents->OnCreditCardInputShownOnHttp();

  // Ensure it didn't add another console warning
 ASSERT_NO_FATAL_FAILURE(CheckForOneFutureHttpWarningConsoleMessage(delegate));

  delegate->ClearConsoleMessages();

That way there's no chance that the warning we're looking for from loading
/title2.html actually came from the second trigger on the first page.

[estark, 2016/10/20 17:45:03]

That would be giving us a false sense of correctness, I think. The notification
about a console message being added is async, so checking for another console
warning immediately after firing the notification isn't useful -- even if the
second sensitive input had triggered a console message, the test
WebContentsDelegate wouldn't have heard about it until the async "console
message added" arrives. And there's no way for the test to wait to check that
that message *never* arrives.

I think to really test this properly, we'd have to add something to
content::MockRenderProcessHost to expose all the console message IPCs that have
been sent, so that we can synchronously test that a console message has or has
not been sent. Which is doable, but not worth blocking this CL on, IMO.

+  contents->OnCreditCardInputShownOnHttp();
+  GURL second_http_url =
+      GetURLWithNonLocalHostname(embedded_test_server(), "/title2.html");
+  ui_test_utils::NavigateToURL(delegate, second_http_url);
+  entry = contents->GetController().GetVisibleEntry();
+  ASSERT_TRUE(entry);
+  EXPECT_EQ(second_http_url, entry->GetURL());
+
+  base::RunLoop second_message;
+  delegate->set_console_message_callback(second_message.QuitClosure());
+  contents->OnPasswordInputShownOnHttp();
+  second_message.Run();
+
+  client->GetSecurityInfo(&security_info);
+  EXPECT_EQ(security_state::SecurityStateModel::NONE,
+            security_info.security_level);
+  EXPECT_TRUE(security_info.displayed_private_user_data_input_on_http);
+
+  ASSERT_NO_FATAL_FAILURE(CheckForOneFutureHttpWarningConsoleMessage(delegate));
+}
+
 // Tests that additional HTTP_SHOW_WARNING console messages are not
 // printed after subframe navigations.
 IN_PROC_BROWSER_TEST_F(ChromeSecurityStateModelClientTestWithPasswordCcSwitch,