[Extensions Bindings] Throw access errors on restricted sendRequest

We throw errors when certain extensions (unpacked with an event page)
try to use the deprecated sendRequest-related properties of the
chrome.extension API. Implement this for native bindings. In order to
do this, add an InitializeInstance() method on native binding hooks
that allows hooks to modify the newly-created instance of an API for a
specific context.

Add unittests for both InitializeInstance and the restriction of
sendRequest properties.

Bug: 653596

Change-Id: I22be1f0f9d4f299e127b21084649000bab63ddfa
Reviewed-on: https://chromium-review.googlesource.com/775516
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519957}

chrome/renderer/extensions/extension_hooks_delegate.cc

@@ -73,6 +73,20 @@ void GetAliasedFeature(v8::Local<v8::Name> property_name,
       runtime_obj->Get(context, property_name).ToLocalChecked());
 }
 
+// A helper method to throw a deprecation error on access.
+void ThrowDeprecatedAccessError(
+    v8::Local<v8::Name> name,
+    const v8::PropertyCallbackInfo<v8::Value>& info) {
+  static constexpr char kError[] =
+      "extension.sendRequest, extension.onRequest, and "
+      "extension.onRequestExternal are deprecated. Please use "
+      "runtime.sendMessage, runtime.onMessage, and runtime.onMessageExternal "
+      "instead.";
+  v8::Isolate* isolate = info.GetIsolate();
+  isolate->ThrowException(
+      v8::Exception::Error(gin::StringToV8(isolate, kError)));
+}
+
 }  // namespace
 
 ExtensionHooksDelegate::ExtensionHooksDelegate(
@@ -134,7 +148,7 @@ void ExtensionHooksDelegate::InitializeTemplate(
     v8::Isolate* isolate,
     v8::Local<v8::ObjectTemplate> object_template,
     const APITypeReferenceMap& type_refs) {
-  static const char* const kAliases[] = {
+  static constexpr const char* kAliases[] = {
       "connect",   "connectNative",     "sendMessage", "sendNativeMessage",
       "onConnect", "onConnectExternal", "onMessage",   "onMessageExternal",
   };
@@ -145,10 +159,34 @@ void ExtensionHooksDelegate::InitializeTemplate(
   }
 }
 
+void ExtensionHooksDelegate::InitializeInstance(
+    v8::Local<v8::Context> context,
+    v8::Local<v8::Object> instance) {
+  // Throw access errors for deprecated sendRequest-related properties. This
+  // isn't terribly efficient, but is only done for certain unpacked extensions
+  // and only if they access the chrome.extension module.
+  if (messaging_util::IsSendRequestDisabled(
+          ScriptContextSet::GetContextByV8Context(context))) {
+    static constexpr const char* kDeprecatedSendRequestProperties[] = {
+        "sendRequest", "onRequest", "onRequestExternal"};
+    for (const char* property : kDeprecatedSendRequestProperties) {
+      v8::Maybe<bool> success = instance->SetAccessor(
+          context, gin::StringToV8(context->GetIsolate(), property),
+          &ThrowDeprecatedAccessError);
+      DCHECK(success.IsJust());
+      DCHECK(success.FromJust());
+    }
+  }
+}
+
 RequestResult ExtensionHooksDelegate::HandleSendRequest(
     ScriptContext* script_context,
     const std::vector<v8::Local<v8::Value>>& arguments) {
   DCHECK_EQ(3u, arguments.size());
+  // This DCHECK() is correct because no context with sendRequest-related
+  // APIs disabled should have scriptable access to a context with them
+  // enabled.
+  DCHECK(!messaging_util::IsSendRequestDisabled(script_context));
 
   std::string target_id;
   std::string error;

chrome/renderer/extensions/extension_hooks_delegate.h

@@ -32,6 +32,8 @@ class ExtensionHooksDelegate : public APIBindingHooksDelegate {
   void InitializeTemplate(v8::Isolate* isolate,
                           v8::Local<v8::ObjectTemplate> object_template,
                           const APITypeReferenceMap& type_refs) override;
+  void InitializeInstance(v8::Local<v8::Context> context,
+                          v8::Local<v8::Object> instance) override;
 
  private:
   // Request handlers for the corresponding API methods.

chrome/renderer/extensions/extension_hooks_delegate_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/strings/stringprintf.h"
 #include "extensions/common/extension_builder.h"
+#include "extensions/common/value_builder.h"
 #include "extensions/renderer/bindings/api_binding_test_util.h"
 #include "extensions/renderer/message_target.h"
 #include "extensions/renderer/messaging_util.h"
@@ -107,4 +108,57 @@ TEST_F(ExtensionHooksDelegateTest, MessagingSanityChecks) {
                          SendMessageTester::OPEN);
 }
 
+TEST_F(ExtensionHooksDelegateTest, SendRequestDisabled) {
+  // Construct an extension for which sendRequest is disabled (unpacked
+  // extension with an event page).
+  // TODO(devlin): Add a SetBackgroundPage() to ExtensionBuilder?
+  scoped_refptr<Extension> extension =
+      ExtensionBuilder("foo")
+          .MergeManifest(
+              DictionaryBuilder()
+                  .Set("background", DictionaryBuilder()
+                                         .SetBoolean("persistent", false)
+                                         .Set("page", "page.html")
+                                         .Build())
+                  .Build())
+          .SetLocation(Manifest::UNPACKED)
+          .Build();
+  RegisterExtension(extension);
+
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = AddContext();
+  ScriptContext* script_context = CreateScriptContext(
+      context, extension.get(), Feature::BLESSED_EXTENSION_CONTEXT);
+  script_context->set_url(extension->url());
+  bindings_system()->UpdateBindingsForContext(script_context);
+  ASSERT_TRUE(messaging_util::IsSendRequestDisabled(script_context));
+
+  enum AccessBehavior {
+    THROWS,
+    DOESNT_THROW,
+  };
+
+  auto check_access = [context](const char* object, AccessBehavior behavior) {
+    SCOPED_TRACE(object);
+    constexpr char kExpectedError[] =
+        "Uncaught Error: extension.sendRequest, extension.onRequest, and "
+        "extension.onRequestExternal are deprecated. Please use "
+        "runtime.sendMessage, runtime.onMessage, and runtime.onMessageExternal "
+        "instead.";
+    v8::Local<v8::Function> function = FunctionFromString(
+        context, base::StringPrintf("(function() {%s;})", object));
+    if (behavior == THROWS)
+      RunFunctionAndExpectError(function, context, 0, nullptr, kExpectedError);
+    else
+      RunFunction(function, context, 0, nullptr);
+  };
+
+  check_access("chrome.extension.sendRequest", THROWS);
+  check_access("chrome.extension.onRequest", THROWS);
+  check_access("chrome.extension.onRequestExternal", THROWS);
+  check_access("chrome.extension.sendMessage", DOESNT_THROW);
+  check_access("chrome.extension.onMessage", DOESNT_THROW);
+  check_access("chrome.extension.onMessageExternal", DOESNT_THROW);
+}
+
 }  // namespace extensions

extensions/renderer/bindings/api_binding.cc

@@ -367,6 +367,8 @@ v8::Local<v8::Object> APIBinding::CreateInstance(
     }
   }
 
+  binding_hooks_->InitializeInstance(context, object);
+
   return object;
 }
 

extensions/renderer/bindings/api_binding_hooks.cc

@@ -348,6 +348,12 @@ void APIBindingHooks::InitializeTemplate(
     delegate_->InitializeTemplate(isolate, object_template, type_refs);
 }
 
+void APIBindingHooks::InitializeInstance(v8::Local<v8::Context> context,
+                                         v8::Local<v8::Object> instance) {
+  if (delegate_)
+    delegate_->InitializeInstance(context, instance);
+}
+
 void APIBindingHooks::SetDelegate(
     std::unique_ptr<APIBindingHooksDelegate> delegate) {
   delegate_ = std::move(delegate);

extensions/renderer/bindings/api_binding_hooks.h

@@ -76,6 +76,10 @@ class APIBindingHooks {
                           v8::Local<v8::ObjectTemplate> object_template,
                           const APITypeReferenceMap& type_refs);
 
+  // Performs any extra initialization on an instance of the API.
+  void InitializeInstance(v8::Local<v8::Context> context,
+                          v8::Local<v8::Object> instance);
+
   void SetDelegate(std::unique_ptr<APIBindingHooksDelegate> delegate);
 
  private:

extensions/renderer/bindings/api_binding_hooks_delegate.h

@@ -36,6 +36,11 @@ class APIBindingHooksDelegate {
   virtual void InitializeTemplate(v8::Isolate* isolate,
                                   v8::Local<v8::ObjectTemplate> object_template,
                                   const APITypeReferenceMap& type_refs) {}
+
+  // Allows custom implementations to mutate an instance of the API for a
+  // specific context.
+  virtual void InitializeInstance(v8::Local<v8::Context> context,
+                                  v8::Local<v8::Object> instance) {}
 };
 
 }  // namespace extensions

extensions/renderer/bindings/api_binding_hooks_test_delegate.cc

@@ -35,6 +35,11 @@ void APIBindingHooksTestDelegate::SetTemplateInitializer(
   template_initializer_ = initializer;
 }
 
+void APIBindingHooksTestDelegate::SetInstanceInitializer(
+    const InstanceInitializer& initializer) {
+  instance_initializer_ = initializer;
+}
+
 APIBindingHooks::RequestResult APIBindingHooksTestDelegate::HandleRequest(
     const std::string& method_name,
     const APISignature* signature,
@@ -57,4 +62,11 @@ void APIBindingHooksTestDelegate::InitializeTemplate(
     template_initializer_.Run(isolate, object_template, type_refs);
 }
 
+void APIBindingHooksTestDelegate::InitializeInstance(
+    v8::Local<v8::Context> context,
+    v8::Local<v8::Object> instance) {
+  if (instance_initializer_)
+    instance_initializer_.Run(context, instance);
+}
+
 }  // namespace extensions

extensions/renderer/bindings/api_binding_hooks_test_delegate.h

@@ -36,6 +36,9 @@ class APIBindingHooksTestDelegate : public APIBindingHooksDelegate {
                                                   v8::Local<v8::ObjectTemplate>,
                                                   const APITypeReferenceMap&)>;
 
+  using InstanceInitializer =
+      base::Callback<void(v8::Local<v8::Context>, v8::Local<v8::Object>)>;
+
   // Adds a custom |handler| for the method with the given |name|.
   void AddHandler(base::StringPiece name, const RequestHandler& handler);
 
@@ -44,6 +47,8 @@ class APIBindingHooksTestDelegate : public APIBindingHooksDelegate {
 
   void SetTemplateInitializer(const TemplateInitializer& initializer);
 
+  void SetInstanceInitializer(const InstanceInitializer& initializer);
+
   // APIBindingHooksDelegate:
   bool CreateCustomEvent(v8::Local<v8::Context> context,
                          const std::string& event_name,
@@ -57,11 +62,14 @@ class APIBindingHooksTestDelegate : public APIBindingHooksDelegate {
   void InitializeTemplate(v8::Isolate* isolate,
                           v8::Local<v8::ObjectTemplate> object_template,
                           const APITypeReferenceMap& type_refs) override;
+  void InitializeInstance(v8::Local<v8::Context> context,
+                          v8::Local<v8::Object> instance) override;
 
  private:
   std::map<std::string, RequestHandler> request_handlers_;
   CustomEventFactory custom_event_;
   TemplateInitializer template_initializer_;
+  InstanceInitializer instance_initializer_;
 
   DISALLOW_COPY_AND_ASSIGN(APIBindingHooksTestDelegate);
 };

extensions/renderer/bindings/api_binding_unittest.cc

@@ -1296,6 +1296,55 @@ TEST_F(APIBindingUnittest, HooksTemplateInitializer) {
             GetStringPropertyFromObject(binding_object, context, "oneString"));
 }
 
+TEST_F(APIBindingUnittest, HooksInstanceInitializer) {
+  SetFunctions(kFunctions);
+  static constexpr char kHookedProperty[] = "hookedProperty";
+
+  // Register a hook for the test.oneString method.
+  auto hooks = std::make_unique<APIBindingHooksTestDelegate>();
+  int count = 0;
+  auto hook = [](int* count, v8::Local<v8::Context> context,
+                 v8::Local<v8::Object> object) {
+    v8::Isolate* isolate = context->GetIsolate();
+    // Add a new property only for the first instance.
+    if ((*count)++ == 0) {
+      object
+          ->Set(context, gin::StringToSymbol(isolate, kHookedProperty),
+                gin::ConvertToV8(isolate, 42))
+          .ToChecked();
+    }
+  };
+
+  hooks->SetInstanceInitializer(base::Bind(hook, &count));
+  SetHooksDelegate(std::move(hooks));
+
+  InitializeBinding();
+
+  v8::HandleScope handle_scope(isolate());
+  // Create two instances.
+  v8::Local<v8::Context> context1 = MainContext();
+  v8::Local<v8::Object> binding_object1 = binding()->CreateInstance(context1);
+
+  v8::Local<v8::Context> context2 = AddContext();
+  v8::Local<v8::Object> binding_object2 = binding()->CreateInstance(context2);
+
+  // We should have run the hooks twice (once per instance).
+  EXPECT_EQ(2, count);
+
+  // The extra property should be present on the first binding object, but not
+  // the second.
+  EXPECT_EQ("42", GetStringPropertyFromObject(binding_object1, context1,
+                                              kHookedProperty));
+  EXPECT_EQ("undefined", GetStringPropertyFromObject(binding_object2, context2,
+                                                     kHookedProperty));
+
+  // Sanity check: other values should still be there.
+  EXPECT_EQ("function", GetStringPropertyFromObject(binding_object1, context1,
+                                                    "oneString"));
+  EXPECT_EQ("function", GetStringPropertyFromObject(binding_object2, context1,
+                                                    "oneString"));
+}
+
 // Test that running hooks returning different results correctly sends requests
 // or notifies of silent requests.
 TEST_F(APIBindingUnittest, TestSendingRequestsAndSilentRequestsWithHooks) {

extensions/renderer/dispatcher.cc

@@ -74,6 +74,7 @@
 #include "extensions/renderer/js_extension_bindings_system.h"
 #include "extensions/renderer/logging_native_handler.h"
 #include "extensions/renderer/messaging_bindings.h"
+#include "extensions/renderer/messaging_util.h"
 #include "extensions/renderer/module_system.h"
 #include "extensions/renderer/native_extension_bindings_system.h"
 #include "extensions/renderer/process_info_native_handler.h"
@@ -1343,9 +1344,7 @@ void Dispatcher::RegisterNativeHandlers(
   int manifest_version = extension ? extension->manifest_version() : 1;
   bool is_component_extension =
       extension && Manifest::IsComponentLocation(extension->location());
-  bool send_request_disabled =
-      (extension && Manifest::IsUnpackedLocation(extension->location()) &&
-       BackgroundInfo::HasLazyBackgroundPage(extension));
+  bool send_request_disabled = messaging_util::IsSendRequestDisabled(context);
   module_system->RegisterNativeHandler(
       "process",
       std::unique_ptr<NativeHandler>(new ProcessInfoNativeHandler(

extensions/renderer/messaging_util.cc

@@ -11,6 +11,8 @@
 #include "base/strings/stringprintf.h"
 #include "extensions/common/api/messaging/message.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/manifest.h"
+#include "extensions/common/manifest_handlers/background_info.h"
 #include "extensions/renderer/script_context.h"
 #include "gin/converter.h"
 #include "gin/dictionary.h"
@@ -292,5 +294,11 @@ void MassageSendMessageArguments(
     *arguments_out = {target_id, message, response_callback};
 }
 
+bool IsSendRequestDisabled(ScriptContext* script_context) {
+  const Extension* extension = script_context->extension();
+  return extension && Manifest::IsUnpackedLocation(extension->location()) &&
+         BackgroundInfo::HasLazyBackgroundPage(extension);
+}
+
 }  // namespace messaging_util
 }  // namespace extensions

extensions/renderer/messaging_util.h

@@ -102,6 +102,10 @@ void MassageSendMessageArguments(
     bool allow_options_argument,
     std::vector<v8::Local<v8::Value>>* arguments_out);
 
+// Returns true if the sendRequest-related properties are disabled for the given
+// |script_context|.
+bool IsSendRequestDisabled(ScriptContext* script_context);
+
 }  // namespace messaging_util
 }  // namespace extensions