[Extensions Bindings] Simplify message option parsing

Now that we create a new V8 object when parsing an argument to V8 (see
crrev.com/37b1fa8a), we don't need to
do all the double-checking for validity and script throwing errors in
messaging_util::ParseMessageOptions. Instead, we can just know that the
value is safe to use.

Update messaging_util::ParseMessageOptions and all its callsites to be
simpler.

Additionally, set the prototype of the parsed object in ArgumentSpec to
null to avoid tricky getters/setters on the Object.prototype, which
could be hit if the argument was missing an optional property.

Bug: 653596

Change-Id: I3f53a3973d9c5a833799248b50832b55bac2792c
Reviewed-on: https://chromium-review.googlesource.com/794451
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521782}

chrome/renderer/extensions/tabs_hooks_delegate.cc

@@ -115,22 +115,9 @@ RequestResult TabsHooksDelegate::HandleSendMessage(
   int tab_id = messaging_util::ExtractIntegerId(arguments[0]);
   messaging_util::MessageOptions options;
   if (!arguments[2]->IsNull()) {
-    std::string error;
-    messaging_util::ParseOptionsResult parse_result =
-        messaging_util::ParseMessageOptions(
-            script_context->v8_context(), arguments[2].As<v8::Object>(),
-            messaging_util::PARSE_FRAME_ID, &options, &error);
-    switch (parse_result) {
-      case messaging_util::TYPE_ERROR: {
-        RequestResult result(RequestResult::INVALID_INVOCATION);
-        result.error = std::move(error);
-        return result;
-      }
-      case messaging_util::THROWN:
-        return RequestResult(RequestResult::THROWN);
-      case messaging_util::SUCCESS:
-        break;
-    }
+    options = messaging_util::ParseMessageOptions(
+        script_context->v8_context(), arguments[2].As<v8::Object>(),
+        messaging_util::PARSE_FRAME_ID);
   }
 
   v8::Local<v8::Value> v8_message = arguments[1];
@@ -164,23 +151,9 @@ RequestResult TabsHooksDelegate::HandleConnect(
 
   messaging_util::MessageOptions options;
   if (!arguments[1]->IsNull()) {
-    std::string error;
-    messaging_util::ParseOptionsResult parse_result =
-        messaging_util::ParseMessageOptions(
-            script_context->v8_context(), arguments[1].As<v8::Object>(),
-            messaging_util::PARSE_FRAME_ID | messaging_util::PARSE_CHANNEL_NAME,
-            &options, &error);
-    switch (parse_result) {
-      case messaging_util::TYPE_ERROR: {
-        RequestResult result(RequestResult::INVALID_INVOCATION);
-        result.error = std::move(error);
-        return result;
-      }
-      case messaging_util::THROWN:
-        return RequestResult(RequestResult::THROWN);
-      case messaging_util::SUCCESS:
-        break;
-    }
+    options = messaging_util::ParseMessageOptions(
+        script_context->v8_context(), arguments[1].As<v8::Object>(),
+        messaging_util::PARSE_FRAME_ID | messaging_util::PARSE_CHANNEL_NAME);
   }
 
   gin::Handle<GinPort> port = messaging_service_->Connect(

extensions/renderer/bindings/argument_spec.cc

@@ -643,8 +643,19 @@ bool ArgumentSpec::ParseArgumentToObject(
 
   if (out_value)
     *out_value = std::move(result);
-  if (v8_out_value)
-    *v8_out_value = convert_to_v8 ? v8_result.Build() : object;
+
+  if (v8_out_value) {
+    if (convert_to_v8) {
+      v8::Local<v8::Object> converted = v8_result.Build();
+      // We set the object's prototype to Null() so that handlers avoid
+      // triggering any tricky getters or setters on Object.prototype.
+      CHECK(converted->SetPrototype(context, v8::Null(context->GetIsolate()))
+                .ToChecked());
+      *v8_out_value = converted;
+    } else {
+      *v8_out_value = object;
+    }
+  }
 
   return true;
 }

extensions/renderer/bindings/argument_spec_unittest.cc

@@ -904,6 +904,44 @@ TEST_F(ArgumentSpecUnitTest, V8Conversion) {
                     EXPECT_EQ("a string", result);
                   }));
   }
+
+  {
+    std::unique_ptr<ArgumentSpec> prop =
+        ArgumentSpecBuilder(ArgumentType::STRING, "str").MakeOptional().Build();
+    std::unique_ptr<ArgumentSpec> spec =
+        ArgumentSpecBuilder(ArgumentType::OBJECT, "obj")
+            .AddProperty("str", std::move(prop))
+            .Build();
+    // The conversion to a v8 value should also protect set an undefined value
+    // on the result value for any absent optional properties. This protects
+    // against cases where an Object.prototype getter would be invoked when a
+    // handler tried to check the value of an argument.
+    constexpr const char kMessWithObjectPrototype[] =
+        R"((function() {
+             Object.defineProperty(
+                 Object.prototype, 'str',
+                 { get() { throw new Error('tricked!'); } });
+           }))";
+    v8::HandleScope handle_scope(instance_->isolate());
+    v8::Local<v8::Context> context =
+        v8::Local<v8::Context>::New(instance_->isolate(), context_);
+    v8::Local<v8::Function> mess_with_proto =
+        FunctionFromString(context, kMessWithObjectPrototype);
+    RunFunction(mess_with_proto, context, 0, nullptr);
+    ExpectSuccess(*spec, "({})", base::BindOnce([](v8::Local<v8::Value> value) {
+      ASSERT_TRUE(value->IsObject());
+      v8::Local<v8::Object> object = value.As<v8::Object>();
+      v8::Local<v8::Context> context = object->CreationContext();
+      // We expect a null prototype to ensure we avoid tricky getters/setters on
+      // the Object prototype.
+      EXPECT_TRUE(object->GetPrototype()->IsNull());
+      gin::Dictionary dict(context->GetIsolate(), object);
+      v8::Local<v8::Value> result;
+      ASSERT_TRUE(dict.Get("str", &result));
+      EXPECT_TRUE(result->IsUndefined());
+    }));
+  }
+
   {
     std::unique_ptr<ArgumentSpec> prop =
         ArgumentSpecBuilder(ArgumentType::STRING, "prop")

extensions/renderer/messaging_util.cc

@@ -137,11 +137,9 @@ int ExtractIntegerId(v8::Local<v8::Value> value) {
   return value->Int32Value();
 }
 
-ParseOptionsResult ParseMessageOptions(v8::Local<v8::Context> context,
-                                       v8::Local<v8::Object> v8_options,
-                                       int flags,
-                                       MessageOptions* options_out,
-                                       std::string* error_out) {
+MessageOptions ParseMessageOptions(v8::Local<v8::Context> context,
+                                   v8::Local<v8::Object> v8_options,
+                                   int flags) {
   DCHECK(!v8_options.IsEmpty());
   DCHECK(!v8_options->IsNull());
 
@@ -149,36 +147,26 @@ ParseOptionsResult ParseMessageOptions(v8::Local<v8::Context> context,
 
   MessageOptions options;
 
-  // Theoretically, our argument matching code already checked the types of
-  // the properties on v8_connect_options. However, since we don't make an
-  // independent copy, it's possible that author script has super sneaky
-  // getters/setters that change the result each time the property is
-  // queried. Make no assumptions.
   gin::Dictionary options_dict(isolate, v8_options);
   if ((flags & PARSE_CHANNEL_NAME) != 0) {
     v8::Local<v8::Value> v8_channel_name;
-    if (!options_dict.Get("name", &v8_channel_name))
-      return THROWN;
+    bool success = options_dict.Get("name", &v8_channel_name);
+    DCHECK(success);
 
     if (!v8_channel_name->IsUndefined()) {
-      if (!v8_channel_name->IsString()) {
-        *error_out = "connectInfo.name must be a string.";
-        return TYPE_ERROR;
-      }
+      DCHECK(v8_channel_name->IsString());
       options.channel_name = gin::V8ToString(v8_channel_name);
     }
   }
 
   if ((flags & PARSE_INCLUDE_TLS_CHANNEL_ID) != 0) {
     v8::Local<v8::Value> v8_include_tls_channel_id;
-    if (!options_dict.Get("includeTlsChannelId", &v8_include_tls_channel_id))
-      return THROWN;
+    bool success =
+        options_dict.Get("includeTlsChannelId", &v8_include_tls_channel_id);
+    DCHECK(success);
 
     if (!v8_include_tls_channel_id->IsUndefined()) {
-      if (!v8_include_tls_channel_id->IsBoolean()) {
-        *error_out = "connectInfo.includeTlsChannelId must be a boolean.";
-        return TYPE_ERROR;
-      }
+      DCHECK(v8_include_tls_channel_id->IsBoolean());
       options.include_tls_channel_id =
           v8_include_tls_channel_id->BooleanValue();
     }
@@ -186,22 +174,16 @@ ParseOptionsResult ParseMessageOptions(v8::Local<v8::Context> context,
 
   if ((flags & PARSE_FRAME_ID) != 0) {
     v8::Local<v8::Value> v8_frame_id;
-    if (!options_dict.Get("frameId", &v8_frame_id))
-      return THROWN;
+    bool success = options_dict.Get("frameId", &v8_frame_id);
+    DCHECK(success);
 
     if (!v8_frame_id->IsUndefined()) {
-      if (!v8_frame_id->IsInt32() &&
-          (!v8_frame_id->IsNumber() ||
-           v8_frame_id.As<v8::Number>()->Value() != 0.0)) {
-        *error_out = "connectInfo.frameId must be an integer.";
-        return TYPE_ERROR;
-      }
+      DCHECK(v8_frame_id->IsInt32());
       options.frame_id = v8_frame_id->Int32Value();
     }
   }
 
-  *options_out = std::move(options);
-  return SUCCESS;
+  return options;
 }
 
 bool GetTargetExtensionId(ScriptContext* script_context,

extensions/renderer/messaging_util.h

@@ -49,13 +49,6 @@ v8::Local<v8::Value> MessageToV8(v8::Local<v8::Context> context,
 // |value| is either an int32 or -0.
 int ExtractIntegerId(v8::Local<v8::Value> value);
 
-// The result of the call to ParseMessageOptions().
-enum ParseOptionsResult {
-  TYPE_ERROR,  // The arguments were invalid.
-  THROWN,      // The script threw an error during parsing.
-  SUCCESS,     // Parsing succeeded.
-};
-
 // Flags for ParseMessageOptions().
 enum ParseOptionsFlags {
   NO_FLAGS = 0,
@@ -70,16 +63,11 @@ struct MessageOptions {
   bool include_tls_channel_id = false;
 };
 
-// Parses the parameters sent to sendMessage or connect, returning the result of
-// the attempted parse. If |check_for_channel_name| is true, also checks for a
-// provided channel name (this is only true for connect() calls). Populates the
-// result in |params_out| or |error_out| (depending on the success of the
-// parse).
-ParseOptionsResult ParseMessageOptions(v8::Local<v8::Context> context,
-                                       v8::Local<v8::Object> v8_options,
-                                       int flags,
-                                       MessageOptions* options_out,
-                                       std::string* error_out);
+// Parses and returns the options parameter for sendMessage or connect.
+// |flags| specifies additional properties to look for on the object.
+MessageOptions ParseMessageOptions(v8::Local<v8::Context> context,
+                                   v8::Local<v8::Object> v8_options,
+                                   int flags);
 
 // Parses the target from |v8_target_id|, or uses the extension associated with
 // the |script_context| as a default. Returns true on success, and false on

extensions/renderer/runtime_hooks_delegate.cc

@@ -165,21 +165,9 @@ RequestResult RuntimeHooksDelegate::HandleSendMessage(
   v8::Local<v8::Context> v8_context = script_context->v8_context();
   messaging_util::MessageOptions options;
   if (!arguments[2]->IsNull()) {
-    messaging_util::ParseOptionsResult parse_result =
-        messaging_util::ParseMessageOptions(
-            v8_context, arguments[2].As<v8::Object>(),
-            messaging_util::PARSE_INCLUDE_TLS_CHANNEL_ID, &options, &error);
-    switch (parse_result) {
-      case messaging_util::TYPE_ERROR: {
-        RequestResult result(RequestResult::INVALID_INVOCATION);
-        result.error = std::move(error);
-        return result;
-      }
-      case messaging_util::THROWN:
-        return RequestResult(RequestResult::THROWN);
-      case messaging_util::SUCCESS:
-        break;
-    }
+    options = messaging_util::ParseMessageOptions(
+        v8_context, arguments[2].As<v8::Object>(),
+        messaging_util::PARSE_INCLUDE_TLS_CHANNEL_ID);
   }
 
   v8::Local<v8::Value> v8_message = arguments[1];
@@ -249,24 +237,10 @@ RequestResult RuntimeHooksDelegate::HandleConnect(
 
   messaging_util::MessageOptions options;
   if (!arguments[1]->IsNull()) {
-    std::string error;
-    messaging_util::ParseOptionsResult parse_result =
-        messaging_util::ParseMessageOptions(
-            script_context->v8_context(), arguments[1].As<v8::Object>(),
-            messaging_util::PARSE_INCLUDE_TLS_CHANNEL_ID |
-                messaging_util::PARSE_CHANNEL_NAME,
-            &options, &error);
-    switch (parse_result) {
-      case messaging_util::TYPE_ERROR: {
-        RequestResult result(RequestResult::INVALID_INVOCATION);
-        result.error = std::move(error);
-        return result;
-      }
-      case messaging_util::THROWN:
-        return RequestResult(RequestResult::THROWN);
-      case messaging_util::SUCCESS:
-        break;
-    }
+    options = messaging_util::ParseMessageOptions(
+        script_context->v8_context(), arguments[1].As<v8::Object>(),
+        messaging_util::PARSE_INCLUDE_TLS_CHANNEL_ID |
+            messaging_util::PARSE_CHANNEL_NAME);
   }
 
   gin::Handle<GinPort> port = messaging_service_->Connect(

extensions/renderer/runtime_hooks_delegate_unittest.cc

@@ -290,6 +290,52 @@ TEST_F(RuntimeHooksDelegateTest, SendMessageErrors) {
   send_message("'some id', 'some message', {}, {}");
 }
 
+TEST_F(RuntimeHooksDelegateTest, SendMessageWithTrickyOptions) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  SendMessageTester tester(ipc_message_sender(), script_context(), 0,
+                           "runtime");
+
+  MessageTarget self_target = MessageTarget::ForExtension(extension()->id());
+  constexpr char kStandardMessage[] = R"({"data":"hello"})";
+  {
+    // Even though we parse the message options separately, we do a conversion
+    // of the object passed into the API. This means that something subtle like
+    // this, which throws on the second access of a property, shouldn't trip us
+    // up.
+    constexpr char kTrickyConnectOptions[] =
+        R"({data: 'hello'},
+           {
+             get includeTlsChannelId() {
+               if (this.checkedOnce)
+                 throw new Error('tricked!');
+               this.checkedOnce = true;
+               return true;
+             }
+           })";
+    tester.TestSendMessage(kTrickyConnectOptions, kStandardMessage, self_target,
+                           true, SendMessageTester::CLOSED);
+  }
+  {
+    // A different form of trickiness: the options object doesn't have the
+    // includeTlsChannelId key (which is acceptable, since its optional), but
+    // any attempt to access the key on an object without a value for it results
+    // in an error. Our argument parsing code should protect us from this.
+    constexpr const char kMessWithObjectPrototype[] =
+        R"((function() {
+             Object.defineProperty(
+                 Object.prototype, 'includeTlsChannelId',
+                 { get() { throw new Error('tricked!'); } });
+           }))";
+    v8::Local<v8::Function> mess_with_proto =
+        FunctionFromString(context, kMessWithObjectPrototype);
+    RunFunction(mess_with_proto, context, 0, nullptr);
+    tester.TestSendMessage("{data: 'hello'}, {}", kStandardMessage, self_target,
+                           false, SendMessageTester::CLOSED);
+  }
+}
+
 class RuntimeHooksDelegateNativeMessagingTest
     : public RuntimeHooksDelegateTest {
  public: