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

chrome/browser/ssl/security_state_tab_helper.cc

Index: chrome/browser/ssl/security_state_tab_helper.cc
diff --git a/chrome/browser/ssl/security_state_tab_helper.cc b/chrome/browser/ssl/security_state_tab_helper.cc
index 590c3fdf83e48707f0fc9003b8911b5d764d3222..c4aa4191e831cd1d5645df30a03060dc1bc17429 100644
--- a/chrome/browser/ssl/security_state_tab_helper.cc
+++ b/chrome/browser/ssl/security_state_tab_helper.cc
@@ -15,6 +15,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/security_state/content/content_utils.h"
 #include "components/ssl_config/ssl_config_prefs.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
@@ -119,6 +120,17 @@ void SecurityStateTabHelper::DidFinishNavigation(
     // and not for same-document navigations like reference fragments and
     // pushState.
     logged_http_warning_on_current_navigation_ = false;
+
+    security_state::SecurityInfo security_info;
+    GetSecurityInfo(&security_info);
+    if (security_info.is_incognito &&
+        security_info.security_level == security_state::HTTP_SHOW_WARNING) {
+      web_contents()->GetMainFrame()->AddMessageToConsole(
+          content::CONSOLE_MESSAGE_LEVEL_WARNING,
+          "This page was loaded non-securely in an incognito mode browser. A "
+          "warning has been added to the URL bar. For more information, see "
+          "https://goo.gl/y8SRRv.");
+    }
   }
 }
 
@@ -199,5 +211,13 @@ SecurityStateTabHelper::GetVisibleSecurityState() const {
   // information is still being initialized, thus no need to check for that.
   state->malicious_content_status = GetMaliciousContentStatus();
 
+  if (!state->certificate &&

[estark, 2017/06/09 05:09:37]

Curious why check the certificate here? You could check
content::IsOriginSecure(web_contents()->GetController().GetVisibleEntry()->GetURL())
to be consistent with Phase 1
(https://cs.chromium.org/chromium/src/components/password_manager/content/brow...)

[elawrence, 2017/06/13 15:31:35]

It was intended as a simple optimization to avoid performing more expensive work
in the common case where the page is HTTPS.

I'll revise this section to make it simpler.

+      security_state::IsHttpWarningForIncognitoEnabled()) {

[estark, 2017/06/09 05:09:38]

IIRC, the reason for checking the field trial here was to avoid checking the
field trial on every call, but it doesn't look like you're caching the result of
the field trial check anywhere. I suggest either:

- Check security_state::IsHttpWarningForIncognitoEnabled() once in the
SecurityStateTabHelper constructor and remember the result as a data member, and
rename VisibleSecurityState::is_incognito to |is_incognito_warning_enabled| or
something to indicate that it might be false even when we are in incognito.

- OR decide that we don't care about the cost of checking the field trial (I
think that would probably be reasonable given that it hasn't been an issue so
far). In that case I'd make VisibileSecurityState::is_incognito do what it says
on the tin (set it to true if and only if we are in incognito mode) and do the
field trial check inside security_state.cc where we do the other field trial
checks. 

[elawrence, 2017/06/13 15:31:35]

The is_incognito flag now does what it says on the tin.

+    content::BrowserContext* context = web_contents()->GetBrowserContext();
+    if (context->IsOffTheRecord() &&
+        !Profile::FromBrowserContext(context)->IsGuestSession()) {

[estark, 2017/06/09 05:09:38]

If it's not too hard, could you add a test that the warning isn't shown in guest
mode?

[elawrence, 2017/06/13 15:31:35]

Added SecurityStateTabHelperTest.SecurityLevelNotDowngradedForHTTPInGuestMode.

+      state->is_incognito = true;
+    }
+  }
   return state;
 }