Enforce user gesture requirement on browser side for WebUI.

Also, don't trust the renderer process to send the correct URL.

Bug: 823864
Change-Id: I1f5c1c7908d8157240b5a7d5fc2df944267cf537
Reviewed-on: https://chromium-review.googlesource.com/979399
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547114}

chrome/browser/ui/webui/webui_browsertest.cc

@@ -2,7 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/command_line.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/weak_ptr.h"
+#include "base/run_loop.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -14,16 +22,48 @@
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui_message_handler.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
+#include "third_party/WebKit/public/platform/WebMouseEvent.h"
 
 namespace {
 
 class TestWebUIMessageHandler : public content::WebUIMessageHandler {
  public:
-  void RegisterMessages() override {}
+  void RegisterMessages() override {
+    web_ui()->RegisterMessageCallback(
+        "messageRequiringGesture",
+        base::BindRepeating(&TestWebUIMessageHandler::OnMessageRequiringGesture,
+                            base::Unretained(this)));
+    web_ui()->RegisterMessageCallback(
+        "notifyFinish",
+        base::BindRepeating(&TestWebUIMessageHandler::OnNotifyFinish,
+                            base::Unretained(this)));
+  }
+
+  void set_finish_closure(base::RepeatingClosure closure) {
+    finish_closure_ = std::move(closure);
+  }
+
+  int message_requiring_gesture_count() const {
+    return message_requiring_gesture_count_;
+  }
+
+ private:
+  void OnMessageRequiringGesture(const base::ListValue* args) {
+    ++message_requiring_gesture_count_;
+  }
+
+  void OnNotifyFinish(const base::ListValue* args) {
+    if (finish_closure_)
+      finish_closure_.Run();
+  }
+
+  int message_requiring_gesture_count_ = 0;
+  base::RepeatingClosure finish_closure_;
 };
 
 }  // namespace
@@ -93,3 +133,51 @@ IN_PROC_BROWSER_TEST_F(WebUIImplBrowserTest, SameDocumentNavigationsAndReload) {
   // Verify that after a reload, the test handler has been disallowed.
   EXPECT_FALSE(test_handler->IsJavascriptAllowed());
 }
+
+IN_PROC_BROWSER_TEST_F(WebUIImplBrowserTest, MessagesRequiringGesture) {
+  ui_test_utils::NavigateToURL(browser(), GURL(chrome::kChromeUIFlagsURL));
+
+  auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
+  auto* test_handler = new TestWebUIMessageHandler();
+  web_contents->GetWebUI()->AddMessageHandler(base::WrapUnique(test_handler));
+
+  auto* main_rfh = web_contents->GetMainFrame();
+
+  // First, make sure that a message that should require a user gesture is
+  // ignored if there is no recent user interaction.
+  main_rfh->ExecuteJavaScriptForTests(
+      base::UTF8ToUTF16("chrome.send('messageRequiringGesture');"
+                        "chrome.send('notifyFinish');"));
+  {
+    base::RunLoop run_loop;
+    test_handler->set_finish_closure(run_loop.QuitClosure());
+    run_loop.Run();
+  }
+  EXPECT_EQ(0, test_handler->message_requiring_gesture_count());
+
+  // Next, test that even with a fake user interaction in Blink, the message is
+  // still ignored.
+  main_rfh->ExecuteJavaScriptWithUserGestureForTests(
+      base::UTF8ToUTF16("chrome.send('messageRequiringGesture');"
+                        "chrome.send('notifyFinish');"));
+  {
+    base::RunLoop run_loop;
+    test_handler->set_finish_closure(run_loop.QuitClosure());
+    run_loop.Run();
+  }
+  EXPECT_EQ(0, test_handler->message_requiring_gesture_count());
+
+  // Finally, test that as long as there's a recent user interaction, the
+  // message is processed.
+  content::SimulateMouseClick(web_contents, 0,
+                              blink::WebMouseEvent::Button::kLeft);
+  main_rfh->ExecuteJavaScriptForTests(
+      base::UTF8ToUTF16("chrome.send('messageRequiringGesture');"
+                        "chrome.send('notifyFinish');"));
+  {
+    base::RunLoop run_loop;
+    test_handler->set_finish_closure(run_loop.QuitClosure());
+    run_loop.Run();
+  }
+  EXPECT_EQ(1, test_handler->message_requiring_gesture_count());
+}

content/browser/web_contents/web_contents_impl.cc

@@ -5536,10 +5536,13 @@ void WebContentsImpl::OnUserInteraction(
     observer.DidGetUserInteraction(type);
 
   ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();
-  // Exclude scroll events as user gestures for resource load dispatches.
-  // rdh is NULL in unittests.
-  if (rdh && type != blink::WebInputEvent::kMouseWheel)
-    rdh->OnUserGesture();
+  if (type != blink::WebInputEvent::kMouseWheel) {
+    // Exclude scroll events as user gestures for resource load dispatches.
+    // rdh is NULL in unittests.
+    if (rdh)
+      rdh->OnUserGesture();
+    last_user_interaction_ = base::TimeTicks::Now();
+  }
 }
 
 void WebContentsImpl::FocusOwningWebContents(

content/browser/web_contents/web_contents_impl.h

@@ -20,6 +20,7 @@
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/process/process.h"
+#include "base/time/time.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "components/download/public/common/download_url_parameters.h"
@@ -285,6 +286,17 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
     return manifest_manager_host_.get();
   }
 
+  // TODO(https://crbug.com/826293): This is a simple mitigation to validate
+  // that an action that requires a user gesture actually has one in the
+  // trustworthy browser process, rather than relying on the untrustworthy
+  // renderer. This should be eventually merged into and accounted for in the
+  // user activation work.
+  bool HasRecentUserInteraction() const {
+    static constexpr base::TimeDelta kMaxInterval =
+        base::TimeDelta::FromSeconds(1);
+    return base::TimeTicks::Now() - last_user_interaction_ <= kMaxInterval;
+  }
+
 #if defined(OS_ANDROID)
   std::set<RenderWidgetHostImpl*> GetAllRenderWidgetHosts();
   void SetImportance(ChildProcessImportance importance);
@@ -1517,6 +1529,9 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
   // the WebContents creation time.
   base::TimeTicks last_active_time_;
 
+  // The time that the user last interacted with this WebContents.
+  base::TimeTicks last_user_interaction_;
+
   // See description above setter.
   bool closed_by_user_gesture_;
 

content/browser/webui/web_ui_impl.cc

@@ -11,7 +11,9 @@
 
 #include "base/debug/dump_without_crashing.h"
 #include "base/json/json_writer.h"
+#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
 #include "base/values.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/frame_host/frame_tree.h"
@@ -79,7 +81,7 @@ base::string16 WebUI::GetJavascriptCall(
   return result;
 }
 
-WebUIImpl::WebUIImpl(WebContents* contents)
+WebUIImpl::WebUIImpl(WebContentsImpl* contents)
     : bindings_(BINDINGS_POLICY_WEB_UI),
       web_contents_(contents),
       web_contents_observer_(new MainFrameNavigationObserver(this, contents)) {
@@ -105,9 +107,9 @@ bool WebUIImpl::OnMessageReceived(const IPC::Message& message,
 }
 
 void WebUIImpl::OnWebUISend(RenderFrameHost* sender,
-                            const GURL& source_url,
                             const std::string& message,
                             const base::ListValue& args) {
+  const GURL& source_url = sender->GetLastCommittedURL();
   if (!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
           sender->GetProcess()->GetID()) ||
       !WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
@@ -122,7 +124,14 @@ void WebUIImpl::OnWebUISend(RenderFrameHost* sender,
   if (!sender->IsCurrent())
     return;
 
-  ProcessWebUIMessage(source_url, message, args);
+  if (base::EndsWith(message, "RequiringGesture",
+                     base::CompareCase::SENSITIVE) &&
+      !web_contents_->HasRecentUserInteraction()) {
+    LOG(ERROR) << message << " received without recent user interaction";
+    return;
+  }
+
+  ProcessWebUIMessage(sender->GetLastCommittedURL(), message, args);
 }
 
 void WebUIImpl::RenderFrameCreated(RenderFrameHost* render_frame_host) {

content/browser/webui/web_ui_impl.h

@@ -22,11 +22,12 @@ class Message;
 
 namespace content {
 class RenderFrameHost;
+class WebContentsImpl;
 
 class CONTENT_EXPORT WebUIImpl : public WebUI,
                                  public base::SupportsWeakPtr<WebUIImpl> {
  public:
-  WebUIImpl(WebContents* contents);
+  explicit WebUIImpl(WebContentsImpl* contents);
   ~WebUIImpl() override;
 
   // Called when a RenderFrame is created for a WebUI (reload after a renderer
@@ -85,7 +86,6 @@ class CONTENT_EXPORT WebUIImpl : public WebUI,
 
   // IPC message handling.
   void OnWebUISend(RenderFrameHost* sender,
-                   const GURL& source_url,
                    const std::string& message,
                    const base::ListValue& args);
 
@@ -107,8 +107,8 @@ class CONTENT_EXPORT WebUIImpl : public WebUI,
   // The WebUIMessageHandlers we own.
   std::vector<std::unique_ptr<WebUIMessageHandler>> handlers_;
 
-  // Non-owning pointer to the WebContents this WebUI is associated with.
-  WebContents* web_contents_;
+  // Non-owning pointer to the WebContentsImpl this WebUI is associated with.
+  WebContentsImpl* web_contents_;
 
   // Notifies this WebUI about notifications in the main frame.
   std::unique_ptr<MainFrameNavigationObserver> web_contents_observer_;

content/common/frame_messages.h

@@ -1705,8 +1705,7 @@ IPC_MESSAGE_ROUTED1(FrameHostMsg_UpdateFaviconURL,
 
 // A message from HTML-based UI.  When (trusted) Javascript calls
 // send(message, args), this message is sent to the browser.
-IPC_MESSAGE_ROUTED3(FrameHostMsg_WebUISend,
-                    GURL /* source_url */,
+IPC_MESSAGE_ROUTED2(FrameHostMsg_WebUISend,
                     std::string /* message */,
                     base::ListValue /* args */)
 

content/renderer/web_ui_extension.cc

@@ -7,7 +7,6 @@
 #include <memory>
 #include <utility>
 
-#include "base/strings/string_util.h"
 #include "base/values.h"
 #include "content/common/frame_messages.h"
 #include "content/public/common/bindings_policy.h"
@@ -23,7 +22,6 @@
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebKit.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
-#include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
 #include "third_party/WebKit/public/web/WebView.h"
 #include "url/gurl.h"
 #include "v8/include/v8.h"
@@ -98,13 +96,6 @@ void WebUIExtension::Send(gin::Arguments* args) {
     return;
   }
 
-  if (base::EndsWith(message, "RequiringGesture",
-                     base::CompareCase::SENSITIVE) &&
-      !blink::WebUserGestureIndicator::IsProcessingUserGesture(frame)) {
-    NOTREACHED();
-    return;
-  }
-
   // If they've provided an optional message parameter, convert that into a
   // Value to send to the browser process.
   std::unique_ptr<base::ListValue> content;
@@ -131,7 +122,6 @@ void WebUIExtension::Send(gin::Arguments* args) {
 
   // Send the message up to the browser.
   render_frame->Send(new FrameHostMsg_WebUISend(render_frame->GetRoutingID(),
-                                                frame->GetDocument().Url(),
                                                 message, *content));
 }