Implement 'Not secure' warning for non-secure pages in Incognito mode

chrome/browser/ssl/security_state_tab_helper_browser_tests.cc

Index: chrome/browser/ssl/security_state_tab_helper_browser_tests.cc
diff --git a/chrome/browser/ssl/security_state_tab_helper_browser_tests.cc b/chrome/browser/ssl/security_state_tab_helper_browser_tests.cc
index 8af6de1cc0d9639a45584a7f3df98f5eabad5e0c..5a7bb8be6335fa0e95529006eccceab024026f5d 100644
--- a/chrome/browser/ssl/security_state_tab_helper_browser_tests.cc
+++ b/chrome/browser/ssl/security_state_tab_helper_browser_tests.cc
@@ -9,11 +9,17 @@
 #include "base/macros.h"
 #include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_command_line.h"
 #include "base/threading/sequenced_worker_pool.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/profiles/profile_window.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ssl/cert_verifier_browser_test.h"
 #include "chrome/browser/ssl/ssl_blocking_page.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
@@ -372,6 +378,21 @@ class SecurityStateTabHelperTest : public CertVerifierBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(SecurityStateTabHelperTest);
 };
 
+// Same as SecurityStateTabHelperTest, but with Incognito enabled.
+class SecurityStateTabHelperIncognitoTest : public SecurityStateTabHelperTest {
+ public:
+  SecurityStateTabHelperIncognitoTest() : SecurityStateTabHelperTest() {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    SecurityStateTabHelperTest::SetUpCommandLine(command_line);
+    // Test should run Incognito.
+    command_line->AppendSwitch(switches::kIncognito);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SecurityStateTabHelperIncognitoTest);
+};
+
 class DidChangeVisibleSecurityStateTest : public InProcessBrowserTest {
  public:
   DidChangeVisibleSecurityStateTest()
@@ -933,6 +954,7 @@ class SecurityStateLoadingTest : public SecurityStateTabHelperTest {
                    embedded_test_server()->GetURL("/title1.html").host()));
   }
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(SecurityStateLoadingTest);
 };
 
@@ -983,6 +1005,17 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest,
   ASSERT_TRUE(entry);
   EXPECT_TRUE(entry->GetSSL().content_status &
               content::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP);
+
+  {
+    // Ensure the warning is still present when HTTPBad Phase 2 flag is enabled.
+    base::test::ScopedCommandLine scoped_command_line;
+    scoped_command_line.GetProcessCommandLine()->AppendSwitchASCII(
+        security_state::switches::kMarkHttpAs,
+        security_state::switches::kMarkHttpAsNonSecureWhileIncognito);
+
+    helper->GetSecurityInfo(&security_info);
+    EXPECT_EQ(security_state::HTTP_SHOW_WARNING, security_info.security_level);
+  }
 }
 
 // Tests that when a visible password field is detected on a blob URL, the
@@ -1352,6 +1385,7 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest,
       contents, "document.getElementById('navFrame').src = '/title2.html';"));
   subframe_observer.Wait();
   contents->OnCreditCardInputShownOnHttp();
+  helper->GetSecurityInfo(&security_info);
   EXPECT_EQ(security_state::HTTP_SHOW_WARNING, security_info.security_level);
 
   // Do a main frame navigation and then trigger HTTP_SHOW_WARNING
@@ -1428,6 +1462,7 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest,
   EXPECT_TRUE(content::ExecuteScript(
       contents, "history.pushState({ foo: 'bar' }, 'foo', 'bar');"));
   contents->OnCreditCardInputShownOnHttp();
+  helper->GetSecurityInfo(&security_info);
   EXPECT_EQ(security_state::HTTP_SHOW_WARNING, security_info.security_level);
 
   // Do a main frame navigation and then trigger HTTP_SHOW_WARNING
@@ -1614,6 +1649,272 @@ IN_PROC_BROWSER_TEST_F(DidChangeVisibleSecurityStateTest,
   EXPECT_TRUE(observer.latest_explanations().summary.empty());
 }
 
+// Tests that the security level of a HTTP page in Incognito mode is downgraded
+// to HTTP_SHOW_WARNING when MarkHttpAsNonSecureWhileIncognito is enabled.
+IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperIncognitoTest,
+                       SecurityLevelDowngradedForHTTPInIncognito) {
+  // Set the mode using the command line flag rather than the field trial to
+  // ensure that fieldtrial_testing_config.json does not interfere.
+  base::test::ScopedCommandLine scoped_command_line;
+  scoped_command_line.GetProcessCommandLine()->AppendSwitchASCII(
+      security_state::switches::kMarkHttpAs,
+      security_state::switches::kMarkHttpAsNonSecureWhileIncognito);
+
+  ConsoleWebContentsDelegate* delegate = new ConsoleWebContentsDelegate(
+      Browser::CreateParams(browser()->profile(), true));
+  content::WebContents* original_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::WebContents* contents =
+      content::WebContents::Create(content::WebContents::CreateParams(
+          original_contents->GetBrowserContext()));
+  ASSERT_TRUE(contents);
+  ASSERT_TRUE(contents->GetBrowserContext()->IsOffTheRecord());
+  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());
+
+  SecurityStyleTestObserver observer(contents);
+
+  SecurityStateTabHelper* helper =
+      SecurityStateTabHelper::FromWebContents(contents);
+  ASSERT_TRUE(helper);
+
+  // 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());
+
+  security_state::SecurityInfo security_info;
+  helper->GetSecurityInfo(&security_info);
+  EXPECT_TRUE(security_info.is_incognito);
+  EXPECT_EQ(security_state::HTTP_SHOW_WARNING, security_info.security_level);
+  EXPECT_EQ(1u, observer.latest_explanations().neutral_explanations.size());
+  EXPECT_EQ(blink::kWebSecurityStyleNeutral, observer.latest_security_style());
+
+  // Check that the expected console message is present.
+  ASSERT_NO_FATAL_FAILURE(CheckForOneHttpWarningConsoleMessage(delegate));
+
+  // Ensure that same-page pushstate does not add another notice.
+  EXPECT_TRUE(content::ExecuteScript(
+      contents, "history.pushState({ foo: 'bar' }, 'foo', 'bar');"));
+  EXPECT_EQ(1u, observer.latest_explanations().neutral_explanations.size());
+  EXPECT_EQ(blink::kWebSecurityStyleNeutral, observer.latest_security_style());
+  // Check that the expected console message is present.

[estark, 2017/06/13 22:47:11]

nit: "Check that no additional console message is present."

[elawrence, 2017/06/14 17:01:39]

Done.

+  ASSERT_NO_FATAL_FAILURE(CheckForOneHttpWarningConsoleMessage(delegate));
+}
+
+// Tests that additional HTTP_SHOW_WARNING console messages are not
+// printed after aborted navigations.
+IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperIncognitoTest,
+                       ConsoleMessageNotPrintedForAbortedNavigation) {
+  // Set the mode using the command line flag rather than the field trial to
+  // ensure that fieldtrial_testing_config.json does not interfere.
+  base::test::ScopedCommandLine scoped_command_line;
+  scoped_command_line.GetProcessCommandLine()->AppendSwitchASCII(
+      security_state::switches::kMarkHttpAs,
+      security_state::switches::kMarkHttpAsNonSecureWhileIncognito);
+
+  ConsoleWebContentsDelegate* delegate = new ConsoleWebContentsDelegate(
+      Browser::CreateParams(browser()->profile(), true));
+  content::WebContents* original_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::WebContents* contents =
+      content::WebContents::Create(content::WebContents::CreateParams(
+          original_contents->GetBrowserContext()));
+  ASSERT_TRUE(contents);
+  ASSERT_TRUE(contents->GetBrowserContext()->IsOffTheRecord());
+  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());
+
+  SecurityStyleTestObserver observer(contents);
+
+  SecurityStateTabHelper* helper =
+      SecurityStateTabHelper::FromWebContents(contents);
+  ASSERT_TRUE(helper);
+
+  // 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);
+
+  security_state::SecurityInfo security_info;
+  helper->GetSecurityInfo(&security_info);
+  EXPECT_TRUE(security_info.is_incognito);
+  EXPECT_EQ(security_state::HTTP_SHOW_WARNING, security_info.security_level);
+  EXPECT_EQ(blink::kWebSecurityStyleNeutral, observer.latest_security_style());
+  EXPECT_EQ(1u, observer.latest_explanations().neutral_explanations.size());
+
+  // Check that the expected console message is present.
+  ASSERT_NO_FATAL_FAILURE(CheckForOneHttpWarningConsoleMessage(delegate));
+  delegate->ClearConsoleMessages();
+
+  // Perform a navigation that does not commit.
+  // The embedded test server returns a HTTP/204 only for local URLs, so
+  // we cannot use GetURLWithNonLocalHostname() here.
+  GURL http204_url = embedded_test_server()->GetURL("/nocontent");
+  ui_test_utils::NavigateToURL(delegate, http204_url);
+
+  // No change is expected in the security state.
+  EXPECT_TRUE(security_info.is_incognito);
+  EXPECT_EQ(security_state::HTTP_SHOW_WARNING, security_info.security_level);
+  EXPECT_EQ(blink::kWebSecurityStyleNeutral, observer.latest_security_style());
+  EXPECT_EQ(1u, observer.latest_explanations().neutral_explanations.size());
+
+  // No additional console logging should occur.
+  EXPECT_TRUE(delegate->console_messages().empty());
+}
+
+// Tests that the security level of a HTTP page in Guest mode is not downgraded
+// to HTTP_SHOW_WARNING when MarkHttpAsNonSecureWhileIncognito is enabled.
+#if defined(OS_CHROMEOS)
+// Guest mode cannot be readily browser-tested on ChromeOS.

[estark, 2017/06/13 22:47:11]

Optional nit: Is there a bug or some other kind of reference you can link to for
this?

[elawrence, 2017/06/14 17:01:38]

I'll try to find something better than
https://www.chromium.org/chromium-os/chromiumos-design-docs/user-accounts-and....
Notably, the existing Guest test I found also ifdefs out running on ChromeOS.

Naively trying to run the test on ChromeOS fails on a CHECK:

https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_impl.cc?...

[ERROR:logging_chrome.cc(179)] Unable to create symlink
/b/s/w/itigPkkL/.org.chromium.Chromium.XVWZC0/d2SbUfe/test-user/chrome_debug.log
pointing at
/b/s/w/itigPkkL/.org.chromium.Chromium.XVWZC0/d2SbUfe/test-user/chrome_debug_20170613-085134.log:
No such file or directory

[FATAL:profile_impl.cc(441)] Check failed: user.
#0 base::debug::StackTrace::StackTrace()
#1 logging::LogMessage::~LogMessage()
#2 ProfileImpl::ProfileImpl()
#3 Profile::CreateProfile()
#4 ProfileManager::CreateProfileAsync()
#5 profiles::SwitchToGuestProfile()

+#define MAYBE_SecurityLevelNotDowngradedForHTTPInGuestMode \
+  DISABLED_SecurityLevelNotDowngradedForHTTPInGuestMode
+#else
+#define MAYBE_SecurityLevelNotDowngradedForHTTPInGuestMode \
+  SecurityLevelNotDowngradedForHTTPInGuestMode
+#endif
+IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest,
+                       MAYBE_SecurityLevelNotDowngradedForHTTPInGuestMode) {
+  base::test::ScopedCommandLine scoped_command_line;
+  scoped_command_line.GetProcessCommandLine()->AppendSwitchASCII(
+      security_state::switches::kMarkHttpAs,
+      security_state::switches::kMarkHttpAsNonSecureWhileIncognito);
+
+  // Create a new browser in Guest Mode.
+  EXPECT_EQ(1U, BrowserList::GetInstance()->size());
+  content::WindowedNotificationObserver browser_creation_observer(
+      chrome::NOTIFICATION_BROWSER_WINDOW_READY,
+      content::NotificationService::AllSources());
+  profiles::SwitchToGuestProfile(ProfileManager::CreateCallback());
+  base::RunLoop().RunUntilIdle();

[estark, 2017/06/13 22:47:11]

Is this necessary? ProfileWindowBrowserTest::OpenGuestBrowser doesn't seem to do
it.

[elawrence, 2017/06/14 17:01:39]

Removed. It doesn't appear to be necessary, although it was present in
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_command_contro....

+  browser_creation_observer.Wait();
+  EXPECT_EQ(2U, BrowserList::GetInstance()->size());
+  Profile* guest = g_browser_process->profile_manager()->GetProfileByPath(
+      ProfileManager::GetGuestProfilePath());
+  Browser* guest_browser = chrome::FindAnyBrowser(guest, true);
+  EXPECT_TRUE(guest_browser);

[estark, 2017/06/13 22:47:11]

nit: ASSERT_TRUE to abort the test if false (otherwise the test will crash on
the next line)

[elawrence, 2017/06/14 17:01:38]

Gah, fixed. That'll teach me to copy/paste out of random Chrome tests. :)

+
+  ConsoleWebContentsDelegate* delegate = new ConsoleWebContentsDelegate(
+      Browser::CreateParams(guest_browser->profile(), true));
+  content::WebContents* original_contents =
+      guest_browser->tab_strip_model()->GetActiveWebContents();
+  content::WebContents* contents =
+      content::WebContents::Create(content::WebContents::CreateParams(
+          original_contents->GetBrowserContext()));
+  ASSERT_TRUE(contents);
+  ASSERT_TRUE(contents->GetBrowserContext()->IsOffTheRecord());
+  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());
+
+  SecurityStyleTestObserver observer(contents);
+
+  SecurityStateTabHelper* helper =
+      SecurityStateTabHelper::FromWebContents(contents);
+  ASSERT_TRUE(helper);
+
+  // Navigate to an HTTP page. Use a non-local hostname so that is it

[estark, 2017/06/13 22:47:11]

nit: is it => it is

[elawrence, 2017/06/14 17:01:39]

Fxied.

+  // not considered secure.
+  GURL http_url =
+      GetURLWithNonLocalHostname(embedded_test_server(), "/title1.html");
+  ui_test_utils::NavigateToURL(delegate, http_url);
+
+  security_state::SecurityInfo security_info;
+  helper->GetSecurityInfo(&security_info);
+  EXPECT_FALSE(security_info.is_incognito);
+  EXPECT_EQ(security_state::NONE, security_info.security_level);
+  EXPECT_EQ(0u, observer.latest_explanations().neutral_explanations.size());
+  EXPECT_EQ(blink::kWebSecurityStyleNeutral, observer.latest_security_style());
+
+  // Ensure that same-page pushstate does not add a notice.
+  EXPECT_TRUE(content::ExecuteScript(

[estark, 2017/06/13 22:47:11]

I think you could probably cut out this part of the test (lines 1842-1850)

[elawrence, 2017/06/14 17:01:38]

Done.

+      contents, "history.pushState({ foo: 'bar' }, 'foo', 'bar');"));
+  helper->GetSecurityInfo(&security_info);
+  EXPECT_FALSE(security_info.is_incognito);
+  EXPECT_EQ(security_state::NONE, security_info.security_level);
+  EXPECT_EQ(0u, observer.latest_explanations().neutral_explanations.size());
+  EXPECT_EQ(blink::kWebSecurityStyleNeutral, observer.latest_security_style());
+
+  // No console notification should occur.
+  EXPECT_TRUE(delegate->console_messages().empty());
+}
+
+// Tests that the security level of a HTTP page is NEUTRAL when MarkHttpAs is
+// not set.
+IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperIncognitoTest,
+                       SecurityLevelNeutralByDefaultForHTTP) {
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+
+  ASSERT_TRUE(contents->GetBrowserContext()->IsOffTheRecord());
+
+  SecurityStyleTestObserver observer(contents);
+
+  SecurityStateTabHelper* helper =
+      SecurityStateTabHelper::FromWebContents(contents);
+  ASSERT_TRUE(helper);
+
+  // 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(browser(), http_url);
+
+  security_state::SecurityInfo security_info;
+  helper->GetSecurityInfo(&security_info);
+  EXPECT_TRUE(security_info.is_incognito);
+  EXPECT_EQ(security_state::NONE, security_info.security_level);
+  EXPECT_EQ(0u, observer.latest_explanations().neutral_explanations.size());
+  EXPECT_EQ(blink::kWebSecurityStyleNeutral, observer.latest_security_style());
+}
+
+// Tests that the security level of a HTTP page is downgraded to DANGEROUS when
+// MarkHttpAsDangerous is enabled.
+IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperIncognitoTest,
+                       SecurityLevelDangerousWhenMarkHttpAsDangerous) {
+  base::test::ScopedCommandLine scoped_command_line;
+  scoped_command_line.GetProcessCommandLine()->AppendSwitchASCII(
+      security_state::switches::kMarkHttpAs,
+      security_state::switches::kMarkHttpAsDangerous);
+
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+  ASSERT_TRUE(contents->GetBrowserContext()->IsOffTheRecord());
+
+  SecurityStyleTestObserver observer(contents);
+
+  SecurityStateTabHelper* helper =
+      SecurityStateTabHelper::FromWebContents(contents);
+  ASSERT_TRUE(helper);
+
+  // 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(browser(), http_url);
+
+  security_state::SecurityInfo security_info;
+  helper->GetSecurityInfo(&security_info);
+  EXPECT_TRUE(security_info.is_incognito);
+  EXPECT_EQ(security_state::DANGEROUS, security_info.security_level);
+  EXPECT_EQ(blink::kWebSecurityStyleInsecure, observer.latest_security_style());
+}
+
 // Visit a valid HTTPS page, then a broken HTTPS page, and then go back,
 // and test that the observed security style matches.
 #if defined(OS_CHROMEOS)