Make limitations for input.ime.sendKeyEvents API.

chrome/browser/ui/input_method/input_method_engine.cc

Index: chrome/browser/ui/input_method/input_method_engine.cc
diff --git a/chrome/browser/ui/input_method/input_method_engine.cc b/chrome/browser/ui/input_method/input_method_engine.cc
index 81a8f4cac492fad5ae3dff8a4eca70e5d67a154e..7dd2293ffe8bdaab33b3fd975bbf3a9b39026bad 100644
--- a/chrome/browser/ui/input_method/input_method_engine.cc
+++ b/chrome/browser/ui/input_method/input_method_engine.cc
@@ -4,6 +4,11 @@
 
 #include "chrome/browser/ui/input_method/input_method_engine.h"
 
+#include <vector>
+
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_frame_host.h"
 #include "ui/base/ime/composition_text.h"
 #include "ui/base/ime/ime_bridge.h"
@@ -185,16 +190,56 @@ ui::ImeWindow* InputMethodEngine::FindWindowById(int window_id) const {
 bool InputMethodEngine::SendKeyEvent(ui::KeyEvent* event,
                                      const std::string& code) {
   DCHECK(event);
+
+  // input.ime.sendKeyEvents API is only allowed to work on text fields.
+  if (current_input_type_ == ui::TEXT_INPUT_TYPE_NONE)
+    return false;
+
   if (event->key_code() == ui::VKEY_UNKNOWN)
     event->set_key_code(ui::DomCodeToUsLayoutKeyboardCode(event->code()));
 
   ui::IMEInputContextHandlerInterface* input_context =
       ui::IMEBridge::Get()->GetInputContextHandler();
-  if (!input_context)
+  Browser* browser = chrome::FindLastActive();

[Devlin, 2016/06/17 16:22:43]

I'm a little dubious of this - might it be possible that the last active browser
(or active web contents) changed since the input was sent?

[Azure Wei, 2016/06/21 10:04:58]

The last active browser/web content could be different from the current input
method, for example AppWindow (Please also see last comment). 
If so, the last committed url is unreliable. I just white list this case for
now. 

+  if (!input_context || !browser)
     return false;
+
+  content::WebContents* web_contents =
+      browser->tab_strip_model()->GetActiveWebContents();
+  if (web_contents) {
+    GURL url = web_contents->GetController().GetActiveEntry()->GetURL();

[Devlin, 2016/06/17 16:22:43]

GetActiveEntry() is deprecated, and can also return null.  I think this would
probably be better as web_contents->GetLastCommittedURL().

[Azure Wei, 2016/06/21 10:04:58]

Updated with GetLastCommittedURL(). Thanks.

+    if (!url.is_empty() && !IsValidKeyForSpecialPage(url.spec(), event))
+      return false;
+  }
+
   input_context->SendKeyEvent(event);
 
   return true;
 }
 
+bool InputMethodEngine::IsValidKeyForSpecialPage(const std::string& url,

[Devlin, 2016/06/17 16:22:43]

why not pass as a gurl?

[Azure Wei, 2016/06/21 10:04:58]

Done.

+                                                 ui::KeyEvent* ui_event) {
+  // input.ime.sendKeyEvents API is not allowed to send ENTER et al. keys on
+  // some special pages.
+  // Make it a vector, so we can add other pages in the future.
+  std::vector<std::string> pages;
+  pages.push_back(std::string("chrome://"));

[Devlin, 2016/06/17 16:22:43]

What about e.g. chrome-devtools?

[Devlin, 2016/06/17 16:22:43]

use the scheme constant.

[meacer, 2016/06/21 01:06:37]

Drive-by: I'd rather use a whitelist here than a blacklist. Adding http and
https to the allowed schemes list is a good start.

[Azure Wei, 2016/06/21 10:04:58]

Done.

+
+  std::vector<ui::KeyboardCode> keycodes;
+  keycodes.push_back(ui::VKEY_ESCAPE);
+  keycodes.push_back(ui::VKEY_TAB);
+  keycodes.push_back(ui::VKEY_RETURN);

[meacer, 2016/06/21 01:13:10]

I suggested these keys in the thread but I'm not sure if they are the only keys
that we should restrict. I'm a bit concerned about the scenario where the
extension triggers a shortcut that opens a page or somehow clicks on a
confirmation dialog.

Can we be more restrictive here and disable more keys? We can loosen up the
requirements later on, but it's safer to start with a smaller set.

Or even better, can we whitelist allowed keys? Is there any reason the IME API
should send a, say, F2 key?

+
+  for (auto page : pages) {
+    if (url.substr(0, page.size()) == page) {

[Devlin, 2016/06/17 16:22:43]

prefer GURL::SchemeIs()

[Azure Wei, 2016/06/21 10:04:58]

Done.

+      for (auto key : keycodes) {
+        if (key == ui_event->key_code()) {
+          return false;
+        }
+      }
+    }
+  }
+  return true;
+}
+
 }  // namespace input_method