[tab-under] Revert r530976 and bring back in-background constraint

This caused a spike in metrics tracking users allowing the redirect
via native UI. Addionally, it seems plausible that it breaks legitimate
auth cases (though I have no direct repro).

It is unfortunate that it allows the popup-redirect case, but that is
not an abused vector (yet), and we are trying to be conservative to
increase the chances of launch.

Bug: 733736
Change-Id: I3559fa6d9e050e476da4c22346ea3ca81526bf06
Reviewed-on: https://chromium-review.googlesource.com/990996
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547610}

chrome/browser/ui/blocked_content/popup_opener_tab_helper_unittest.cc

@@ -201,14 +201,14 @@ TEST_F(PopupOpenerTabHelperTest, FirstNavigation_NoLogging) {
   histogram_tester()->ExpectTotalCount(kTabUnderVisibleTime, 0);
 }
 
-TEST_F(PopupOpenerTabHelperTest, VisibleNavigation_LogsMetrics) {
+TEST_F(PopupOpenerTabHelperTest, VisibleNavigation_NoLogging) {
   NavigateAndCommitWithoutGesture(GURL("https://first.test/"));
   SimulatePopup();
   web_contents()->WasShown();
   NavigateAndCommitWithoutGesture(GURL("https://example.test/"));
   raw_clock()->Advance(base::TimeDelta::FromMinutes(1));
   DeleteContents();
-  histogram_tester()->ExpectTotalCount(kTabUnderVisibleTime, 1);
+  histogram_tester()->ExpectTotalCount(kTabUnderVisibleTime, 0);
 }
 
 // This is counter intuitive, but we want to log metrics in the dry-run state if
@@ -749,3 +749,32 @@ TEST_F(BlockTabUnderTest, ControlledByPrefs) {
   EXPECT_FALSE(NavigateAndCommitWithoutGesture(GURL("https://example.test2/")));
   ExpectUIShown(true);
 }
+
+// Ensure that even though the *redirect* occurred in the background, if the
+// navigation started in the foreground there is no blocking.
+TEST_F(BlockTabUnderTest,
+       TabUnderCrossOriginRedirectFromForeground_IsNotBlocked) {
+  EXPECT_TRUE(NavigateAndCommitWithoutGesture(GURL("https://first.test/")));
+  SimulatePopup();
+
+  web_contents()->WasShown();
+
+  // Navigate to a same-origin URL that redirects cross origin.
+  const GURL same_origin("https://first.test/path");
+  const GURL cross_origin("https://example.test/");
+  std::unique_ptr<content::NavigationSimulator> simulator =
+      content::NavigationSimulator::CreateRendererInitiated(same_origin,
+                                                            main_rfh());
+  simulator->SetHasUserGesture(false);
+  simulator->Start();
+  EXPECT_EQ(content::NavigationThrottle::PROCEED,
+            simulator->GetLastThrottleCheckResult());
+
+  web_contents()->WasHidden();
+
+  simulator->Redirect(cross_origin);
+  EXPECT_EQ(content::NavigationThrottle::PROCEED,
+            simulator->GetLastThrottleCheckResult());
+  simulator->Commit();
+  ExpectUIShown(false);
+}

chrome/browser/ui/blocked_content/tab_under_blocker_browsertest.cc

@@ -163,11 +163,8 @@ IN_PROC_BROWSER_TEST_F(TabUnderBlockerBrowserTest,
   EXPECT_FALSE(IsUiShownForUrl(opener, cross_origin_url));
 }
 
-// Test for crbug.com/733736, where a spoof shift-click does not trigger
-// tab-under because the subsequent navigation is not considered to be in the
-// background.
 IN_PROC_BROWSER_TEST_F(TabUnderBlockerBrowserTest,
-                       SpoofShiftClickTabUnder_IsBlocked) {
+                       SpoofCtrlClickTabUnder_IsBlocked) {
   content::WebContents* opener =
       browser()->tab_strip_model()->GetActiveWebContents();
   ui_test_utils::NavigateToURL(browser(),
@@ -175,14 +172,17 @@ IN_PROC_BROWSER_TEST_F(TabUnderBlockerBrowserTest,
   const std::string cross_origin_url =
       embedded_test_server()->GetURL("a.com", "/title1.html").spec();
 
-  const std::string script = R"(
-    var evt = new MouseEvent("click", {
-      view : window,
-      shiftKey : true
-    });
-    document.getElementById("title1").dispatchEvent(evt);
-    window.location = "%s";
-  )";
+  const std::string script =
+      "var evt = new MouseEvent('click', {"
+      "  view : window,"
+#if defined(OS_MACOSX)
+      "  metaKey : true"
+#else
+      "  ctrlKey : true"
+#endif
+      "});"
+      "document.getElementById('title1').dispatchEvent(evt);"
+      "window.location = '%s';";
 
   content::TestNavigationObserver navigation_observer(nullptr, 1);
   content::TestNavigationObserver tab_under_observer(opener, 1);

chrome/browser/ui/blocked_content/tab_under_navigation_throttle.cc

@@ -30,6 +30,7 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/visibility.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/common/console_message_level.h"
@@ -143,13 +144,15 @@ TabUnderNavigationThrottle::TabUnderNavigationThrottle(
                  handle->GetWebContents()->GetBrowserContext())
                  ->GetBoolean(prefs::kTabUnderProtection)),
       has_opened_popup_since_last_user_gesture_at_start_(
-          HasOpenedPopupSinceLastUserGesture()) {}
+          HasOpenedPopupSinceLastUserGesture()),
+      started_in_foreground_(handle->GetWebContents()->GetVisibility() ==
+                             content::Visibility::VISIBLE) {}
 
 bool TabUnderNavigationThrottle::IsSuspiciousClientRedirect() const {
   // Some browser initiated navigations have HasUserGesture set to false. This
   // should eventually be fixed in crbug.com/617904. In the meantime, just dont
   // block browser initiated ones.
-  if (!navigation_handle()->IsInMainFrame() ||
+  if (started_in_foreground_ || !navigation_handle()->IsInMainFrame() ||
       navigation_handle()->HasUserGesture() ||
       !navigation_handle()->IsRendererInitiated()) {
     return false;

chrome/browser/ui/blocked_content/tab_under_navigation_throttle.h

@@ -34,8 +34,18 @@ constexpr char kBlockTabUnderFormatMessage[] =
 //    c. It is cross site to the last committed URL in the tab.
 //    d. The target site has a Site Engagement score below some threshold (by
 //       default, a score of 0).
+//    e. The navigation started in the background.
 // 2. The tab has opened a popup and hasn't received a user gesture since then.
 //    This information is tracked by the PopupOpenerTabHelper.
+//
+//  TODO(csharrison): Unfortunately, the provision that a navigation must start
+//  in the background to be considered a tab-under restricts the scope of the
+//  intervention. For instance, popups that do not completely hide the original
+//  page may cause subsequent tab-under navigations to occur while visible (not
+//  compeltely backgrounded). See https://crbug.com/733736.
+//
+//  For now, we allow these tab-unders because this pattern seems to be
+//  legitimate for some cases (like auth).
 class TabUnderNavigationThrottle : public content::NavigationThrottle {
  public:
   static const base::Feature kBlockTabUnders;
@@ -108,6 +118,10 @@ class TabUnderNavigationThrottle : public content::NavigationThrottle {
   // gesture, at the time this navigation is starting.
   const bool has_opened_popup_since_last_user_gesture_at_start_ = false;
 
+  // Whether this object was created when the hosting WebContents had visibility
+  // content::Visibility::VISIBLE.
+  const bool started_in_foreground_ = false;
+
   // True if the throttle has seen a tab under.
   bool seen_tab_under_ = false;