desktop-pwas: show location bar whenever the site is not considered s…

…ecure

Changes the logic to show the location bar any time a site is not secure
e.g. if it displays mixed content, has a mixed content form, etc.

Before, we would only show the location bar for dangerous sites e.g.
invalid certificate, or if the site was not using https.

Based on https://crrev.com/c/923226 by mgiuca.

Bug: 811158
Change-Id: I72b59eaab031f20924057d1c7ec614f221c52c61
Reviewed-on: https://chromium-review.googlesource.com/1001075
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550098}

chrome/browser/ssl/ssl_browsertest.cc

@@ -1300,23 +1300,6 @@ IN_PROC_BROWSER_TEST_P(SSLUITest, TestBrokenHTTPSWithActiveInsecureContent) {
 
 namespace {
 
-// A WebContentsObserver that allows the user to wait for a
-// DidChangeVisibleSecurityState event.
-class SecurityStateWebContentsObserver : public content::WebContentsObserver {
- public:
-  explicit SecurityStateWebContentsObserver(content::WebContents* web_contents)
-      : content::WebContentsObserver(web_contents) {}
-  ~SecurityStateWebContentsObserver() override {}
-
-  void WaitForDidChangeVisibleSecurityState() { run_loop_.Run(); }
-
-  // WebContentsObserver:
-  void DidChangeVisibleSecurityState() override { run_loop_.Quit(); }
-
- private:
-  base::RunLoop run_loop_;
-};
-
 // A WebContentsObserver that allows the user to wait for a same-document
 // navigation. Tests using this observer will fail if a non-same-document
 // navigation completes after calling WaitForSameDocumentNavigation.

chrome/browser/ssl/ssl_browsertest_util.cc

@@ -95,4 +95,18 @@ void CheckSecurityState(content::WebContents* tab,
   AuthState::Check(*entry, expected_authentication_state);
 }
 
+SecurityStateWebContentsObserver::SecurityStateWebContentsObserver(
+    content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents) {}
+
+SecurityStateWebContentsObserver::~SecurityStateWebContentsObserver() {}
+
+void SecurityStateWebContentsObserver::WaitForDidChangeVisibleSecurityState() {
+  run_loop_.Run();
+}
+
+void SecurityStateWebContentsObserver::DidChangeVisibleSecurityState() {
+  run_loop_.Quit();
+}
+
 }  // namespace ssl_test_util

chrome/browser/ssl/ssl_browsertest_util.h

@@ -5,7 +5,9 @@
 #ifndef CHROME_BROWSER_SSL_SSL_BROWSERTEST_UTIL_H_
 #define CHROME_BROWSER_SSL_SSL_BROWSERTEST_UTIL_H_
 
+#include "base/run_loop.h"
 #include "components/security_state/core/security_state.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "net/cert/cert_status_flags.h"
 
 namespace content {
@@ -48,6 +50,22 @@ void CheckSecurityState(content::WebContents* tab,
                         security_state::SecurityLevel expected_security_level,
                         int expected_authentication_state);
 
+// A WebContentsObserver that allows the user to wait for a
+// DidChangeVisibleSecurityState event.
+class SecurityStateWebContentsObserver : public content::WebContentsObserver {
+ public:
+  explicit SecurityStateWebContentsObserver(content::WebContents* web_contents);
+  ~SecurityStateWebContentsObserver() override;
+
+  void WaitForDidChangeVisibleSecurityState();
+
+  // WebContentsObserver:
+  void DidChangeVisibleSecurityState() override;
+
+ private:
+  base::RunLoop run_loop_;
+};
+
 }  // namespace ssl_test_util
 
 #endif  // CHROME_BROWSER_SSL_SSL_BROWSERTEST_UTIL_H_

chrome/browser/ui/browser.cc

@@ -1501,8 +1501,12 @@ void Browser::VisibleSecurityStateChanged(WebContents* source) {
   // When the current tab's security state changes, we need to update the URL
   // bar to reflect the new state.
   DCHECK(source);
-  if (tab_strip_model_->GetActiveWebContents() == source)
+  if (tab_strip_model_->GetActiveWebContents() == source) {
     UpdateToolbar(false);
+
+    if (hosted_app_controller_)
+      hosted_app_controller_->UpdateLocationBarVisibility(true);
+  }
 }
 
 void Browser::AddNewContents(WebContents* source,

chrome/browser/ui/extensions/hosted_app_browser_controller.cc

@@ -51,15 +51,39 @@ base::StringPiece ExpectedSchemeForApp(base::StringPiece scheme) {
   return url::kHttpsScheme;
 }
 
+bool IsSiteSecure(const content::WebContents* web_contents) {
+  const SecurityStateTabHelper* helper =
+      SecurityStateTabHelper::FromWebContents(web_contents);
+  if (helper) {
+    security_state::SecurityInfo security_info;
+    helper->GetSecurityInfo(&security_info);
+    switch (security_info.security_level) {
+      case security_state::SECURITY_LEVEL_COUNT:
+        NOTREACHED();
+        return false;
+      case security_state::EV_SECURE:
+      case security_state::SECURE:
+      case security_state::SECURE_WITH_POLICY_INSTALLED_CERT:
+        return true;
+      case security_state::NONE:
+      case security_state::HTTP_SHOW_WARNING:
+      case security_state::DANGEROUS:
+        return false;
+    }
+  }
+  return false;
+}
+
 // Returns true if |page_url| is both secure (not http) and on the same origin
 // as |app_url|. Note that even if |app_url| is http, this still returns true as
-// long as |page_url| is https.
+// long as |page_url| is https. To avoid breaking Hosted Apps and Bookmark Apps
+// that might redirect to sites in the same domain but with "www.", this returns
+// true if |page_url| is secure and in the same origin as |app_url| with "www.".
 bool IsSameOriginAndSecure(const GURL& app_url, const GURL& page_url) {
-  const std::string www("www.");
   return ExpectedSchemeForApp(app_url.scheme_piece()) ==
              page_url.scheme_piece() &&
          (app_url.host_piece() == page_url.host_piece() ||
-          www + app_url.host() == page_url.host_piece()) &&
+          std::string("www.") + app_url.host() == page_url.host_piece()) &&
          app_url.port() == page_url.port();
 }
 
@@ -169,19 +193,24 @@ bool HostedAppBrowserController::ShouldShowLocationBar() const {
   if (web_contents->GetLastCommittedURL().is_empty())
     return false;
 
-  const SecurityStateTabHelper* helper =
-      SecurityStateTabHelper::FromWebContents(web_contents);
-  if (helper) {
-    security_state::SecurityInfo security_info;
-    helper->GetSecurityInfo(&security_info);
-    if (security_info.security_level == security_state::DANGEROUS)
-      return true;
+  GURL launch_url = AppLaunchInfo::GetLaunchWebURL(extension);
+  if (!IsSameOriginAndSecure(launch_url, web_contents->GetLastCommittedURL()))
+    return true;
+
+  // Check the visible URL, because we would like to indicate to the user that
+  // they are navigating to a different origin than that of the app as soon as
+  // the navigation starts, even if the navigation hasn't committed yet.
+  if (!IsSameOriginAndSecure(launch_url, web_contents->GetVisibleURL()))
+    return true;
+
+  // We consider URLs with kExtensionScheme secure.
+  if (!(IsSiteSecure(web_contents) ||
+        web_contents->GetLastCommittedURL().scheme_piece() ==
+            kExtensionScheme)) {
+    return true;
   }
 
-  GURL launch_url = AppLaunchInfo::GetLaunchWebURL(extension);
-  return !IsSameOriginAndSecure(launch_url, web_contents->GetVisibleURL()) ||
-         !IsSameOriginAndSecure(launch_url,
-                                web_contents->GetLastCommittedURL());
+  return false;
 }
 
 void HostedAppBrowserController::UpdateLocationBarVisibility(

chrome/browser/ui/extensions/hosted_app_browsertest.cc

@@ -310,6 +310,8 @@ class HostedAppTest
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     ExtensionBrowserTest::SetUpCommandLine(command_line);
+    // Browser will both run and display insecure content.
+    command_line->AppendSwitch(switches::kAllowRunningInsecureContent);
     command_line->AppendSwitch(switches::kUseMockCertVerifierForTesting);
   }
 
@@ -523,13 +525,42 @@ IN_PROC_BROWSER_TEST_P(HostedAppTest, ShouldShowLocationBarMixedContent) {
   SetupAppWithURL(app_url);
 
   // Navigate to another page on the same origin, but with mixed content; the
-  // location bar should be hidden.
-  // TODO(ortuno): Make the location bar visible.
+  // location bar should be shown.
   NavigateAndCheckForLocationBar(
       app_browser_,
       https_server()->GetURL("app.com",
                              "/ssl/page_displays_insecure_content.html"),
-      false);
+      true);
+}
+
+IN_PROC_BROWSER_TEST_P(HostedAppTest,
+                       ShouldShowLocationBarDynamicMixedContent) {
+  ASSERT_TRUE(https_server()->Start());
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  const GURL app_url = https_server()->GetURL("app.com", "/simple.html");
+
+  SetupAppWithURL(app_url);
+
+  // Navigate to a page on the same origin. Since mixed content hasn't been
+  // loaded yet, the location bar shouldn't be shown.
+  NavigateAndCheckForLocationBar(app_browser_, app_url, false);
+
+  // Load mixed content; now the location bar should be shown.
+  content::WebContents* web_contents =
+      app_browser_->tab_strip_model()->GetActiveWebContents();
+  ssl_test_util::SecurityStateWebContentsObserver observer(web_contents);
+  ASSERT_TRUE(content::ExecuteScript(
+      web_contents,
+      base::StringPrintf("let i = document.createElement('img');"
+                         "i.src = '%s';"
+                         "document.body.appendChild(i);",
+                         embedded_test_server()
+                             ->GetURL("foo.com", "/ssl/google_files/logo.gif")
+                             .spec()
+                             .c_str())));
+  observer.WaitForDidChangeVisibleSecurityState();
+  EXPECT_TRUE(app_browser_->hosted_app_controller()->ShouldShowLocationBar());
 }
 
 IN_PROC_BROWSER_TEST_P(HostedAppTest,