SafeBrowsing Delayed Warnings: Allow modifier keys

Delayed Warnings feature delays certain interstitials until the user
presses a key. It presently ignores ESC key as it can be used to exit
from fullscreen mode.

The user can also use browser shortcuts such as Control+T to open a
new tab. This CL allows modifier keys (Control, Alt etc) so that browser
shortcuts don't show the interstitial.

There are two exceptions made by the CL:
- Shift pressed with other character keys still show the interstitial as
the user may be typing uppercase letters.
- Ctrl+C and Ctrl+V still show the interstitial as they are shortcuts
for copy & paste.

Bug: 1131149
Change-Id: I5d3532b9296beef783e01e18d14e0b7a53309fe2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425206Reviewed-by: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Commit-Queue: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809848}

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

@@ -2206,6 +2206,73 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageDelayedWarningBrowserTest,
                                DelayedWarningEvent::kWarningNotShown, 1);
 }
 
+IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageDelayedWarningBrowserTest,
+                       KeyPress_ModifierKey_WarningNotShown) {
+  base::HistogramTester histograms;
+  NavigateAndAssertNoInterstitial();
+
+  // Press CTRL+A key. The interstitial should not be shown because we ignore
+  // the CTRL modifier unless it's CTRL+C or CTRL+V.
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::NativeWebKeyboardEvent event(
+      blink::WebKeyboardEvent::Type::kRawKeyDown,
+      blink::WebInputEvent::kControlKey,
+      blink::WebInputEvent::GetStaticTimeStampForTests());
+  event.windows_key_code = ui::VKEY_A;
+  // Browser expects a non-synthesized event to have an os_event. Make the
+  // browser ignore this event instead.
+  event.skip_in_browser = true;
+  contents->GetRenderViewHost()->GetWidget()->ForwardKeyboardEvent(event);
+  AssertNoInterstitial(browser(), false);
+
+  // Navigate to about:blank twice to "flush" metrics, if any. The delayed
+  // warning user interaction observer may not have been deleted after the first
+  // navigation.
+  ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
+
+  histograms.ExpectTotalCount(kDelayedWarningsHistogram, 2);
+  histograms.ExpectBucketCount(kDelayedWarningsHistogram,
+                               DelayedWarningEvent::kPageLoaded, 1);
+  histograms.ExpectBucketCount(kDelayedWarningsHistogram,
+                               DelayedWarningEvent::kWarningNotShown, 1);
+}
+
+IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageDelayedWarningBrowserTest,
+                       KeyPress_CtrlC_WarningShown) {
+  base::HistogramTester histograms;
+  NavigateAndAssertNoInterstitial();
+
+  // Press CTRL+C. The interstitial should be shown.
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::TestNavigationObserver observer(contents);
+
+  content::NativeWebKeyboardEvent event(
+      blink::WebKeyboardEvent::Type::kRawKeyDown,
+      blink::WebInputEvent::kControlKey,
+      blink::WebInputEvent::GetStaticTimeStampForTests());
+  event.windows_key_code = ui::VKEY_C;
+  event.native_key_code = ui::VKEY_C;
+  // We don't set event.skip_in_browser = true here because the event will be
+  // consumed by UserInteractionObserver and not passed to the browser.
+  contents->GetRenderViewHost()->GetWidget()->ForwardKeyboardEvent(event);
+
+  observer.WaitForNavigationFinished();
+  EXPECT_TRUE(WaitForReady(browser()));
+
+  EXPECT_TRUE(ClickAndWaitForDetach(browser(), "primary-button"));
+  AssertNoInterstitial(browser(), false);  // Assert the interstitial is gone
+  EXPECT_EQ(GURL(url::kAboutBlankURL),     // Back to "about:blank"
+            browser()->tab_strip_model()->GetActiveWebContents()->GetURL());
+
+  histograms.ExpectTotalCount(kDelayedWarningsHistogram, 2);
+  histograms.ExpectBucketCount(kDelayedWarningsHistogram,
+                               DelayedWarningEvent::kPageLoaded, 1);
+  histograms.ExpectBucketCount(kDelayedWarningsHistogram,
+                               DelayedWarningEvent::kWarningShownOnKeypress, 1);
+}
+
 // Similar to KeyPress_ESC_WarningNotShown, but a character key is pressed after
 // ESC. The warning should be shown.
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageDelayedWarningBrowserTest,

chrome/browser/safe_browsing/user_interaction_observer.cc

@@ -16,11 +16,14 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "third_party/blink/public/common/input/web_mouse_event.h"
+#include "ui/events/keycodes/keyboard_codes.h"
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 #include "extensions/browser/extension_registry.h"
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 
+using blink::WebInputEvent;
+
 namespace {
 // Id for extension that enables users to report sites to Safe Browsing.
 const char kPreventElisionExtensionId[] = "jknemblkbdhdcpllfgbfekkdciegfboi";
@@ -303,11 +306,29 @@ void SafeBrowsingUserInteractionObserver::RecordUMA(DelayedWarningEvent event) {
   }
 }
 
+bool IsAllowedModifier(const content::NativeWebKeyboardEvent& event) {
+  const int key_modifiers =
+      event.GetModifiers() & blink::WebInputEvent::kKeyModifiers;
+  // If the only modifier is shift, the user may be typing uppercase
+  // letters.
+  if (key_modifiers == WebInputEvent::kShiftKey) {
+    return event.windows_key_code == ui::VKEY_SHIFT;
+  }
+  // Disallow CTRL+C and CTRL+V.
+  if (key_modifiers == WebInputEvent::kControlKey &&
+      (event.windows_key_code == ui::VKEY_C ||
+       event.windows_key_code == ui::VKEY_V)) {
+    return false;
+  }
+  return key_modifiers != 0;
+}
+
 bool SafeBrowsingUserInteractionObserver::HandleKeyPress(
     const content::NativeWebKeyboardEvent& event) {
   // Allow non-character keys such as ESC. These can be used to exit fullscreen,
   // for example.
-  if (!event.IsCharacterKey()) {
+  if (!event.IsCharacterKey() || event.is_browser_shortcut ||
+      IsAllowedModifier(event)) {
     return false;
   }
   ShowInterstitial(DelayedWarningEvent::kWarningShownOnKeypress);