[Extensions Bindings] Add native declarativeContent verification

extensions/renderer/declarative_content_hooks_delegate.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/renderer/declarative_content_hooks_delegate.h"
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "extensions/renderer/api_type_reference_map.h"
+#include "extensions/renderer/argument_spec.h"
+#include "gin/arguments.h"
+#include "gin/converter.h"
+#include "third_party/WebKit/public/platform/WebString.h"
+#include "third_party/WebKit/public/web/WebSelector.h"
+
+namespace extensions {
+
+namespace {
+
+void CallbackHelper(const v8::FunctionCallbackInfo<v8::Value>& info) {
+  CHECK(info.Data()->IsExternal());
+  v8::Local<v8::External> external = info.Data().As<v8::External>();
+  auto* callback =
+      static_cast<DeclarativeContentHooksDelegate::HandlerCallback*>(
+          external->Value());
+  callback->Run(info);
+}
+
+// Copies the properties from src -> dst, similar to Object.assign().

[jbroman, 2017/05/01 20:16:40]

Don't know how much you care, but at a glance I think there's at least three
behaviour differences (which may or may not be desirable):
- Object.assign copies symbol keys
- Object.assign uses Set rather than CreateDataProperty
- Object.assign checks that it is an own property, if that changes during
iteration

[Devlin, 2017/05/01 20:46:48]

I'm not too worried about those.  The goal wasn't so much to implement
Object.assign() as just to copy the parameters over.  I only mentioned
Object.assign() as a "it's basically like this"-type comment, but I realize that
it might be confusing.  I've removed the reference to avoid making it seem like
it's supposed to be a spec-appropriate implementation.

+bool V8Assign(v8::Local<v8::Context> context,
+              v8::Local<v8::Object> src,
+              v8::Local<v8::Object> dst) {
+  v8::Local<v8::Array> own_property_names;
+  if (!src->GetOwnPropertyNames(context).ToLocal(&own_property_names))
+    return false;
+
+  uint32_t length = own_property_names->Length();
+  for (uint32_t i = 0; i < length; ++i) {
+    v8::Local<v8::Value> key;
+    if (!own_property_names->Get(context, i).ToLocal(&key))
+      return false;
+    DCHECK(key->IsString() || key->IsNumber());

[jbroman, 2017/05/01 20:16:40]

Note that this is true (without a property filter, GetOwnPropertyNames does skip
symbols), but Object.assign does not. But you only really need to downcast to
v8::Name, so I'm not sure this DCHECK is that helpful.

[Devlin, 2017/05/01 20:46:48]

I think the DCHECK is useful only because below we assume the value is either a
string or uint; it just makes that assumption more clear.

+
+    v8::Local<v8::Value> prop_value;
+    if (!src->Get(context, key).ToLocal(&prop_value))
+      return false;
+
+    bool success = false;

[jbroman, 2017/05/01 20:16:40]

super-nit: might as well factor out the result checking, too?

v8::Maybe<bool> set_result =
    key->IsString()
        ? dst->CreateDataProperty(context,
                                  key.As<v8::String>(),
                                  prop_value)
        : dst->CreateDataProperty(context,
                                  key.As<v8::Uint32>()->Value(),
                                  prop_value);
if (!set_result.IsJust() || !set_result.FromJust())
  return false;

[Devlin, 2017/05/01 20:46:48]

Sure.  I thought it would be too verbose, but if you're fine with it, works for
me. :)

+    if (key->IsString()) {
+      v8::Maybe<bool> set_result =
+          dst->CreateDataProperty(context, key.As<v8::String>(), prop_value);
+      success = set_result.IsJust() && set_result.FromJust();
+    } else {
+      v8::Maybe<bool> set_result = dst->CreateDataProperty(
+          context, key.As<v8::Uint32>()->Value(), prop_value);
+      success = set_result.IsJust() && set_result.FromJust();
+    }
+    if (!success)
+      return false;
+  }
+
+  return true;
+}
+
+// Canonicalizes any css selectors specified in a page state matcher, returning
+// true on success.
+bool CanonicalizeCssSelectors(v8::Local<v8::Context> context,
+                              v8::Local<v8::Object> object,
+                              std::string* error) {
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::Local<v8::String> key = gin::StringToSymbol(isolate, "css");
+  v8::Maybe<bool> has_css = object->HasOwnProperty(context, key);
+  // Note: don't bother populating |error| if script threw an exception.
+  if (!has_css.IsJust())
+    return false;
+
+  if (!has_css.FromJust())
+    return true;
+
+  v8::Local<v8::Value> css;
+  if (!object->Get(context, key).ToLocal(&css))
+    return false;
+
+  if (css->IsUndefined())
+    return true;
+
+  if (!css->IsArray())
+    return false;
+
+  v8::Local<v8::Array> css_array = css.As<v8::Array>();
+  uint32_t length = css_array->Length();
+  for (uint32_t i = 0; i < length; ++i) {
+    v8::Local<v8::Value> val;
+    if (!css_array->Get(context, i).ToLocal(&val) || !val->IsString())
+      return false;
+    const char* selector = *v8::String::Utf8Value(val.As<v8::String>());

[jbroman, 2017/05/01 20:16:40]

This is unsafe. The buffer returned by Utf8Value::operator* is only valid until
the Utf8Value is destroyed (the V8 internal representation is different). If you
want to use Utf8Value, you'll have to have it on the stack and not just as a
temporary. (Aside: if you're using Utf8Value, it does provide the length for
you, so you don't have to use the implicit strlen that appears here on both
conversion to WebString and std::string.)

[Devlin, 2017/05/01 20:46:48]

Oh, I had no idea.  Fixed.  Thanks for catching it!

+    std::string parsed =
+        blink::CanonicalizeSelector(blink::WebString::FromUTF8(selector),
+                                    blink::kWebSelectorTypeCompound)
+            .Utf8();
+    if (parsed.empty()) {
+      *error = "Invalid css: " + std::string(selector);
+      return false;
+    }
+    v8::Maybe<bool> set_result =
+        css_array->Set(context, i, gin::StringToSymbol(isolate, parsed));
+    if (!set_result.IsJust() || !set_result.FromJust())
+      return false;
+  }
+
+  return true;
+}
+
+// Validates the source object against the expected spec, and copies over values
+// to |this|. Returns true on success.
+bool Validate(const ArgumentSpec* spec,
+              const APITypeReferenceMap& type_refs,
+              v8::Local<v8::Context> context,
+              v8::Local<v8::Object> this_object,
+              v8::Local<v8::Object> source_object,
+              const std::string& type_name,
+              std::string* error) {
+  if (!source_object.IsEmpty() &&
+      !V8Assign(context, source_object, this_object)) {
+    return false;
+  }
+
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::Maybe<bool> set_result = this_object->CreateDataProperty(
+      context, gin::StringToSymbol(isolate, "instanceType"),
+      gin::StringToSymbol(isolate, type_name));
+  if (!set_result.IsJust() || !set_result.FromJust()) {
+    return false;
+  }
+
+  if (!spec->ParseArgument(context, this_object, type_refs, nullptr, error)) {
+    return false;
+  }
+
+  if (type_name == "declarativeContent.PageStateMatcher" &&
+      !CanonicalizeCssSelectors(context, this_object, error)) {
+    return false;
+  }
+  return true;
+}
+
+}  // namespace
+
+DeclarativeContentHooksDelegate::DeclarativeContentHooksDelegate()
+    : weak_factory_(this) {}
+
+DeclarativeContentHooksDelegate::~DeclarativeContentHooksDelegate() {}
+
+void DeclarativeContentHooksDelegate::InitializeTemplate(
+    v8::Isolate* isolate,
+    v8::Local<v8::ObjectTemplate> object_template,
+    const APITypeReferenceMap& type_refs) {
+  // Add constructors for the API types.
+  // TODO(devlin): We'll need to extract out common logic here and share it with
+  // declarativeWebRequest.
+  struct {
+    const char* full_name;
+    const char* exposed_name;
+  } kTypes[] = {
+      {"declarativeContent.PageStateMatcher", "PageStateMatcher"},
+      {"declarativeContent.ShowPageAction", "ShowPageAction"},
+      {"declarativeContent.SetIcon", "SetIcon"},
+      {"declarativeContent.RequestContentScript", "RequestContentScript"},
+  };
+  for (const auto type : kTypes) {
+    const ArgumentSpec* spec = type_refs.GetSpec(type.full_name);
+    DCHECK(spec);
+    callbacks_.push_back(base::MakeUnique<HandlerCallback>(base::Bind(
+        &DeclarativeContentHooksDelegate::HandleCall,

[jbroman, 2017/05/01 20:16:40]

This seems slightly paradoxical. If the DeclarativeContentHooksDelegate always
outlives the function, then the weak ptr is unnecessary. If not, then the vector
will have been destroyed and it will be illegal to access the callback in the
first place, before we even get to the weak ptr.

[Devlin, 2017/05/01 20:46:48]

Fair enough - habit.  Removed.

+        weak_factory_.GetWeakPtr(), spec, &type_refs, type.full_name)));
+    object_template->Set(
+        gin::StringToSymbol(isolate, type.exposed_name),
+        v8::FunctionTemplate::New(
+            isolate, &CallbackHelper,
+            v8::External::New(isolate, callbacks_.back().get())));
+  }
+}
+
+void DeclarativeContentHooksDelegate::HandleCall(
+    const ArgumentSpec* spec,
+    const APITypeReferenceMap* type_refs,
+    const std::string& type_name,
+    const v8::FunctionCallbackInfo<v8::Value>& info) {
+  gin::Arguments arguments(info);
+  v8::Isolate* isolate = arguments.isolate();
+  v8::HandleScope handle_scope(isolate);
+  v8::Local<v8::Context> context = isolate->GetCurrentContext();
+
+  // TODO(devlin): It would be pretty nice to be able to throw an error if
+  // Arguments::IsConstructCall() is false. That would ensure that the caller
+  // used `new declarativeContent.Foo()`, which is a) the documented approach
+  // and b) allows us (more) confidence that the |this| object we receive is
+  // an unmodified instance. But we don't know how many extensions enforcing
+  // that may break, and it's also incompatible with SetIcon().
+
+  v8::Local<v8::Object> this_object = info.This();
+  if (this_object.IsEmpty()) {
+    // Crazy script (e.g. declarativeContent.Foo.apply(null, args);).
+    NOTREACHED();
+    return;
+  }
+
+  // TODO(devlin): Find a way to use APISignature here? It's a little awkward
+  // because of undocumented expected properties like instanceType and not
+  // requiring an argument at all. We may need a better way of expressing these
+  // in the JSON schema.
+  std::vector<v8::Local<v8::Value>> argument_list = arguments.GetAll();
+  if (argument_list.size() > 1) {
+    arguments.ThrowTypeError("Invalid invocation.");

[Devlin, 2017/05/01 19:29:45]

Note: will rebase and use the new API invocation errors here.

+    return;
+  }
+
+  v8::Local<v8::Object> properties;
+  if (!argument_list.empty()) {
+    if (!argument_list[0]->IsObject()) {
+      arguments.ThrowTypeError("Invalid invocation.");
+      return;
+    }
+    properties = argument_list[0].As<v8::Object>();
+  }
+
+  std::string error;
+  bool success = false;
+  {
+    v8::TryCatch try_catch(isolate);
+    success = Validate(spec, *type_refs, context, this_object, properties,
+                       type_name, &error);
+    if (try_catch.HasCaught()) {
+      try_catch.ReThrow();
+      return;
+    }
+  }
+
+  if (!success)
+    arguments.ThrowTypeError("Invalid invocation: " + error);
+}
+
+}  // namespace extensions