[Extensions Bindings] DCHECK on invalid responses

Add native validation for bindings when passing the response back to
the caller that the response fits the values specified in the schema.
Do this by adding the expected callback signature to the type
reference map, and then matching the returned values against that
signature.

Given the performance cost, we don't want to check these values by
default. Instead, only do so if response validation is enabled. Outside
of testing, this is currently only true for debug builds; this matches
current behavior for JS bindings as well. However, this is easily
tweakable by a boolean, which could be linked to a finch feature if we
so desired.

Additionally, tweak matching against expected signatures for responses
to be stricter than signature matching for callers. This is because we
want to ensure that extensions are invoked with the appropriate
arguments at the appropriate indices, rather than needing them to do
our own complicated signature matching. This is a behavior change (and
and improvement) over current JS response validation, since it will
require that every response is truly valid, rather than being able to
be mutated into something valid.

Add unittests for all affected systems, including response validation,
signature matching, and callback signature parsing.

Bug: 829913

Change-Id: Ib630b3bee935f54e252f66bba7bccf14724adfa9
Reviewed-on: https://chromium-review.googlesource.com/1011407
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@{#553859}

extensions/renderer/BUILD.gn

@@ -52,6 +52,8 @@ jumbo_source_set("renderer") {
     "bindings/api_last_error.h",
     "bindings/api_request_handler.cc",
     "bindings/api_request_handler.h",
+    "bindings/api_response_validator.cc",
+    "bindings/api_response_validator.h",
     "bindings/api_signature.cc",
     "bindings/api_signature.h",
     "bindings/api_type_reference_map.cc",
@@ -407,6 +409,7 @@ source_set("unit_tests") {
     "bindings/api_invocation_errors_unittest.cc",
     "bindings/api_last_error_unittest.cc",
     "bindings/api_request_handler_unittest.cc",
+    "bindings/api_response_validator_unittest.cc",
     "bindings/api_signature_unittest.cc",
     "bindings/argument_spec_builder.cc",
     "bindings/argument_spec_builder.h",

extensions/renderer/bindings/api_binding.cc

@@ -61,6 +61,34 @@ std::string GetJSEnumEntryName(const std::string& original) {
   return result;
 }
 
+struct SignaturePair {
+  std::unique_ptr<APISignature> method_signature;
+  std::unique_ptr<APISignature> callback_signature;
+};
+
+SignaturePair GetAPISignatureFromDictionary(const base::DictionaryValue* dict) {
+  const base::ListValue* params = nullptr;
+  CHECK(dict->GetList("parameters", &params));
+
+  SignaturePair result;
+  result.method_signature = std::make_unique<APISignature>(*params);
+  // If response validation is enabled, parse the callback signature. Otherwise,
+  // there's no reason to, so don't bother.
+  if (result.method_signature->has_callback() &&
+      binding::IsResponseValidationEnabled()) {
+    const base::Value* callback_params = params->GetList().back().FindKeyOfType(
+        "parameters", base::Value::Type::LIST);
+    if (callback_params) {
+      const base::ListValue* params_as_list = nullptr;
+      callback_params->GetAsList(&params_as_list);
+      result.callback_signature =
+          std::make_unique<APISignature>(*params_as_list);
+    }
+  }
+
+  return result;
+}
+
 void RunAPIBindingHandlerCallback(
     const v8::FunctionCallbackInfo<v8::Value>& info) {
   gin::Arguments args(info);
@@ -205,20 +233,23 @@ APIBinding::APIBinding(const std::string& api_name,
       std::string name;
       CHECK(func_dict->GetString("name", &name));
 
-      const base::ListValue* params = nullptr;
-      CHECK(func_dict->GetList("parameters", &params));
+      SignaturePair signatures = GetAPISignatureFromDictionary(func_dict);
 
       bool for_io_thread = false;
       func_dict->GetBoolean("forIOThread", &for_io_thread);
 
-      auto signature = std::make_unique<APISignature>(*params);
       std::string full_name =
           base::StringPrintf("%s.%s", api_name_.c_str(), name.c_str());
       methods_[name] = std::make_unique<MethodData>(
-          full_name, signature.get(),
+          full_name, signatures.method_signature.get(),
           for_io_thread ? binding::RequestThread::IO
                         : binding::RequestThread::UI);
-      type_refs->AddAPIMethodSignature(full_name, std::move(signature));
+      type_refs->AddAPIMethodSignature(full_name,
+                                       std::move(signatures.method_signature));
+      if (signatures.callback_signature) {
+        type_refs->AddCallbackSignature(
+            full_name, std::move(signatures.callback_signature));
+      }
     }
   }
 
@@ -254,11 +285,16 @@ APIBinding::APIBinding(const std::string& api_name,
           std::string function_name;
           CHECK(func_dict->GetString("name", &function_name));
 
-          const base::ListValue* params = nullptr;
-          CHECK(func_dict->GetList("parameters", &params));
+          SignaturePair signatures = GetAPISignatureFromDictionary(func_dict);
+
+          std::string full_name =
+              base::StringPrintf("%s.%s", id.c_str(), function_name.c_str());
           type_refs->AddTypeMethodSignature(
-              base::StringPrintf("%s.%s", id.c_str(), function_name.c_str()),
-              std::make_unique<APISignature>(*params));
+              full_name, std::move(signatures.method_signature));
+          if (signatures.callback_signature) {
+            type_refs->AddCallbackSignature(
+                full_name, std::move(signatures.callback_signature));
+          }
         }
       }
     }

extensions/renderer/bindings/api_binding_unittest.cc

@@ -17,6 +17,7 @@
 #include "extensions/renderer/bindings/api_event_handler.h"
 #include "extensions/renderer/bindings/api_invocation_errors.h"
 #include "extensions/renderer/bindings/api_request_handler.h"
+#include "extensions/renderer/bindings/api_signature.h"
 #include "extensions/renderer/bindings/api_type_reference_map.h"
 #include "extensions/renderer/bindings/binding_access_checker.h"
 #include "extensions/renderer/bindings/test_js_runner.h"
@@ -74,6 +75,32 @@ const char kFunctions[] =
     "  }]"
     "}]";
 
+constexpr char kFunctionsWithCallbackSignatures[] = R"(
+    [{
+       "name": "noCallback",
+       "parameters": [{
+         "name": "int",
+         "type": "integer"
+       }]
+     }, {
+       "name": "intCallback",
+       "parameters": [{
+         "name": "callback",
+         "type": "function",
+         "parameters": [{
+           "name": "int",
+           "type": "integer"
+         }]
+       }]
+     }, {
+       "name": "noParamCallback",
+       "parameters": [{
+         "name": "callback",
+         "type": "function",
+         "parameters": []
+       }]
+     }])";
+
 bool AllowAllFeatures(v8::Local<v8::Context> context, const std::string& name) {
   return true;
 }
@@ -1582,4 +1609,37 @@ TEST_F(APIBindingUnittest, AccessAPIMethodsAndEventsAfterInvalidation) {
                             "Uncaught Error: Extension context invalidated.");
 }
 
+TEST_F(APIBindingUnittest, CallbackSignaturesAreAdded) {
+  std::unique_ptr<base::AutoReset<bool>> response_validation_override =
+      binding::SetResponseValidationEnabledForTesting(true);
+
+  SetFunctions(kFunctionsWithCallbackSignatures);
+  InitializeBinding();
+
+  EXPECT_FALSE(type_refs().GetCallbackSignature("test.noCallback"));
+
+  const APISignature* int_signature =
+      type_refs().GetCallbackSignature("test.intCallback");
+  ASSERT_TRUE(int_signature);
+  EXPECT_EQ("integer int", int_signature->GetExpectedSignature());
+
+  const APISignature* no_param_signature =
+      type_refs().GetCallbackSignature("test.noParamCallback");
+  ASSERT_TRUE(no_param_signature);
+  EXPECT_EQ("", no_param_signature->GetExpectedSignature());
+}
+
+TEST_F(APIBindingUnittest,
+       CallbackSignaturesAreNotAddedWhenValidationDisabled) {
+  std::unique_ptr<base::AutoReset<bool>> response_validation_override =
+      binding::SetResponseValidationEnabledForTesting(false);
+
+  SetFunctions(kFunctionsWithCallbackSignatures);
+  InitializeBinding();
+
+  EXPECT_FALSE(type_refs().GetCallbackSignature("test.noCallback"));
+  EXPECT_FALSE(type_refs().GetCallbackSignature("test.intCallback"));
+  EXPECT_FALSE(type_refs().GetCallbackSignature("test.noParamCallback"));
+}
+
 }  // namespace extensions

extensions/renderer/bindings/api_binding_util.cc

@@ -16,6 +16,17 @@
 namespace extensions {
 namespace binding {
 
+namespace {
+
+bool g_response_validation_enabled =
+#if DCHECK_IS_ON()
+    true;
+#else
+    false;
+#endif
+
+}  // namespace
+
 class ContextInvalidationData : public base::SupportsUserData::Data {
  public:
   ContextInvalidationData();
@@ -150,5 +161,15 @@ void ContextInvalidationListener::OnInvalidated() {
   std::move(on_invalidated_).Run();
 }
 
+bool IsResponseValidationEnabled() {
+  return g_response_validation_enabled;
+}
+
+std::unique_ptr<base::AutoReset<bool>> SetResponseValidationEnabledForTesting(
+    bool is_enabled) {
+  return std::make_unique<base::AutoReset<bool>>(&g_response_validation_enabled,
+                                                 is_enabled);
+}
+
 }  // namespace binding
 }  // namespace extensions

extensions/renderer/bindings/api_binding_util.h

@@ -5,8 +5,10 @@
 #ifndef EXTENSIONS_RENDERER_BINDING_API_BINDING_UTIL_H_
 #define EXTENSIONS_RENDERER_BINDING_API_BINDING_UTIL_H_
 
+#include <memory>
 #include <string>
 
+#include "base/auto_reset.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "v8/include/v8.h"
@@ -54,6 +56,16 @@ class ContextInvalidationListener {
 // "linux", "win", or "mac".
 std::string GetPlatformString();
 
+// Returns true if response validation is enabled, and the bindings system
+// should check the values returned by the browser against the expected results
+// defined in the schemas. By default, this is corresponds to whether DCHECK is
+// enabled.
+bool IsResponseValidationEnabled();
+
+// Override response validation for testing purposes.
+std::unique_ptr<base::AutoReset<bool>> SetResponseValidationEnabledForTesting(
+    bool is_enabled);
+
 }  // namespace binding
 }  // namespace extensions
 

extensions/renderer/bindings/api_bindings_system.cc

@@ -8,6 +8,7 @@
 #include "base/values.h"
 #include "extensions/renderer/bindings/api_binding_hooks.h"
 #include "extensions/renderer/bindings/api_binding_util.h"
+#include "extensions/renderer/bindings/api_response_validator.h"
 
 namespace extensions {
 
@@ -28,7 +29,12 @@ APIBindingsSystem::APIBindingsSystem(
       event_handler_(event_listeners_changed, &exception_handler_),
       access_checker_(is_available),
       get_api_schema_(get_api_schema),
-      on_silent_request_(on_silent_request) {}
+      on_silent_request_(on_silent_request) {
+  if (binding::IsResponseValidationEnabled()) {
+    request_handler_.SetResponseValidator(
+        std::make_unique<APIResponseValidator>(&type_reference_map_));
+  }
+}
 
 APIBindingsSystem::~APIBindingsSystem() {}
 

extensions/renderer/bindings/api_request_handler.cc

@@ -8,6 +8,7 @@
 #include "base/guid.h"
 #include "base/values.h"
 #include "content/public/renderer/v8_value_converter.h"
+#include "extensions/renderer/bindings/api_response_validator.h"
 #include "extensions/renderer/bindings/exception_handler.h"
 #include "extensions/renderer/bindings/js_runner.h"
 #include "gin/converter.h"
@@ -24,10 +25,12 @@ APIRequestHandler::PendingRequest::PendingRequest(
     v8::Isolate* isolate,
     v8::Local<v8::Function> callback,
     v8::Local<v8::Context> context,
-    const std::vector<v8::Local<v8::Value>>& local_callback_args)
+    const std::vector<v8::Local<v8::Value>>& local_callback_args,
+    const std::string& method_name)
     : isolate(isolate),
       context(isolate, context),
       callback(isolate, callback),
+      method_name(method_name),
       user_gesture_token(
           blink::WebUserGestureIndicator::CurrentUserGestureToken()) {
   if (!local_callback_args.empty()) {
@@ -98,7 +101,8 @@ int APIRequestHandler::StartRequest(v8::Local<v8::Context> context,
 
     request->has_callback = true;
     pending_requests_.insert(std::make_pair(
-        request_id, PendingRequest(isolate, callback, context, callback_args)));
+        request_id,
+        PendingRequest(isolate, callback, context, callback_args, method)));
   }
 
   request->has_user_gesture =
@@ -168,6 +172,15 @@ void APIRequestHandler::CompleteRequest(
   if (!error.empty())
     last_error_.SetError(context, error);
 
+  if (response_validator_) {
+    bool has_custom_callback = !pending_request.callback_arguments.empty();
+    response_validator_->ValidateResponse(
+        context, pending_request.method_name, args, error,
+        has_custom_callback
+            ? APIResponseValidator::CallbackType::kAPIProvided
+            : APIResponseValidator::CallbackType::kCallerProvided);
+  }
+
   v8::TryCatch try_catch(isolate);
   // args.size() is converted to int, but args is controlled by chrome and is
   // never close to std::numeric_limits<int>::max.
@@ -190,8 +203,9 @@ int APIRequestHandler::AddPendingRequest(v8::Local<v8::Context> context,
                                          v8::Local<v8::Function> callback) {
   int request_id = next_request_id_++;
   pending_requests_.emplace(
-      request_id, PendingRequest(context->GetIsolate(), callback, context,
-                                 std::vector<v8::Local<v8::Value>>()));
+      request_id,
+      PendingRequest(context->GetIsolate(), callback, context,
+                     std::vector<v8::Local<v8::Value>>(), std::string()));
   return request_id;
 }
 
@@ -205,6 +219,12 @@ void APIRequestHandler::InvalidateContext(v8::Local<v8::Context> context) {
   }
 }
 
+void APIRequestHandler::SetResponseValidator(
+    std::unique_ptr<APIResponseValidator> response_validator) {
+  DCHECK(!response_validator_);
+  response_validator_ = std::move(response_validator);
+}
+
 std::set<int> APIRequestHandler::GetPendingRequestIdsForTesting() const {
   std::set<int> result;
   for (const auto& pair : pending_requests_)

extensions/renderer/bindings/api_request_handler.h

@@ -21,6 +21,7 @@ class ListValue;
 }
 
 namespace extensions {
+class APIResponseValidator;
 class ExceptionHandler;
 
 // A wrapper around a map for extension API calls. Contains all pending requests
@@ -85,8 +86,13 @@ class APIRequestHandler {
   // Invalidates any requests that are associated with |context|.
   void InvalidateContext(v8::Local<v8::Context> context);
 
+  void SetResponseValidator(std::unique_ptr<APIResponseValidator> validator);
+
   APILastError* last_error() { return &last_error_; }
   int last_sent_request_id() const { return last_sent_request_id_; }
+  bool has_response_validator_for_testing() const {
+    return response_validator_.get() != nullptr;
+  }
 
   std::set<int> GetPendingRequestIdsForTesting() const;
 
@@ -95,7 +101,8 @@ class APIRequestHandler {
     PendingRequest(v8::Isolate* isolate,
                    v8::Local<v8::Function> callback,
                    v8::Local<v8::Context> context,
-                   const std::vector<v8::Local<v8::Value>>& callback_args);
+                   const std::vector<v8::Local<v8::Value>>& callback_args,
+                   const std::string& method_name);
     ~PendingRequest();
     PendingRequest(PendingRequest&&);
     PendingRequest& operator=(PendingRequest&&);
@@ -104,6 +111,7 @@ class APIRequestHandler {
     v8::Global<v8::Context> context;
     v8::Global<v8::Function> callback;
     std::vector<v8::Global<v8::Value>> callback_arguments;
+    std::string method_name;
     blink::WebUserGestureToken user_gesture_token;
   };
 
@@ -126,6 +134,10 @@ class APIRequestHandler {
   // during this object's lifetime.
   ExceptionHandler* const exception_handler_;
 
+  // The response validator used to check the responses for resolved requests.
+  // Null if response validation is disabled.
+  std::unique_ptr<APIResponseValidator> response_validator_;
+
   DISALLOW_COPY_AND_ASSIGN(APIRequestHandler);
 };
 

extensions/renderer/bindings/api_response_validator.cc

+// Copyright 2018 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/bindings/api_response_validator.h"
+
+#include "extensions/renderer/bindings/api_binding_util.h"
+#include "extensions/renderer/bindings/api_signature.h"
+#include "extensions/renderer/bindings/api_type_reference_map.h"
+
+namespace extensions {
+
+namespace {
+
+APIResponseValidator::TestHandler::HandlerMethod*
+    g_failure_handler_for_testing = nullptr;
+
+}  // namespace
+
+APIResponseValidator::TestHandler::TestHandler(HandlerMethod method)
+    : method_(method) {
+  DCHECK(!g_failure_handler_for_testing)
+      << "Only one TestHandler is allowed at a time.";
+  g_failure_handler_for_testing = &method_;
+}
+
+APIResponseValidator::TestHandler::~TestHandler() {
+  DCHECK_EQ(&method_, g_failure_handler_for_testing);
+  g_failure_handler_for_testing = nullptr;
+}
+
+APIResponseValidator::APIResponseValidator(const APITypeReferenceMap* type_refs)
+    : type_refs_(type_refs) {}
+
+APIResponseValidator::~APIResponseValidator() = default;
+
+void APIResponseValidator::ValidateResponse(
+    v8::Local<v8::Context> context,
+    const std::string& method_name,
+    const std::vector<v8::Local<v8::Value>> response_arguments,
+    const std::string& api_error,
+    CallbackType callback_type) {
+  DCHECK(binding::IsResponseValidationEnabled());
+
+  // If the callback is API-provided, the response can't be validated against
+  // the expected schema because the callback may modify the arguments.
+  if (callback_type == CallbackType::kAPIProvided)
+    return;
+
+  // If the call failed, there are no expected arguments.
+  if (!api_error.empty()) {
+    // TODO(devlin): It would be really nice to validate that
+    // |response_arguments| is empty here, but some functions both set an error
+    // and supply arguments.
+    return;
+  }
+
+  const APISignature* signature = type_refs_->GetCallbackSignature(method_name);
+  // If there's no corresponding signature, don't validate. This can
+  // legitimately happen with APIs that create custom requests.
+  if (!signature)
+    return;
+
+  std::string error;
+  if (signature->ValidateResponse(context, response_arguments, *type_refs_,
+                                  &error)) {
+    // Response was valid.
+    return;
+  }
+
+  // The response did not match the expected schema.
+  if (g_failure_handler_for_testing) {
+    g_failure_handler_for_testing->Run(method_name, error);
+  } else {
+    NOTREACHED() << "Error validating response to `" << method_name
+                 << "`: " << error;
+  }
+}
+
+}  // namespace extensions

extensions/renderer/bindings/api_response_validator.h

+// Copyright 2018 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.
+
+#ifndef EXTENSIONS_RENDERER_BINDINGS_API_RESPONSE_VALIDATOR_H_
+#define EXTENSIONS_RENDERER_BINDINGS_API_RESPONSE_VALIDATOR_H_
+
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "v8/include/v8.h"
+
+namespace extensions {
+class APITypeReferenceMap;
+
+// A class to validate the responses to API calls sent by the browser. This
+// helps ensure that the browser returns values that match the expected schema
+// (which corresponds to the public documentation).
+class APIResponseValidator {
+ public:
+  // Allow overriding the default failure behavior.
+  class TestHandler {
+   public:
+    using HandlerMethod =
+        base::RepeatingCallback<void(const std::string&, const std::string&)>;
+
+    explicit TestHandler(HandlerMethod method);
+    ~TestHandler();
+
+   private:
+    HandlerMethod method_;
+
+    DISALLOW_COPY_AND_ASSIGN(TestHandler);
+  };
+
+  // The origin of the callback passed to the response.
+  enum class CallbackType {
+    // NOTE(devlin): There's deliberately not a kNoCallback value here, since
+    // ValidateResponse() is only invoked if there was some callback provided.
+    // This is important, since some API implementations can adjust behavior
+    // based on whether a callback is provided.
+
+    // The callback was directly provided by the author script.
+    kCallerProvided,
+    // The callback was provided by the API, such as through custom bindings.
+    kAPIProvided,
+  };
+
+  explicit APIResponseValidator(const APITypeReferenceMap* type_refs);
+  ~APIResponseValidator();
+
+  // Validates a response against the expected schema. By default, this will
+  // NOTREACHED() in cases of validation failure.
+  void ValidateResponse(
+      v8::Local<v8::Context> context,
+      const std::string& method_name,
+      const std::vector<v8::Local<v8::Value>> response_arguments,
+      const std::string& api_error,
+      CallbackType callback_type);
+
+ private:
+  // The type reference map; guaranteed to outlive this object.
+  const APITypeReferenceMap* type_refs_;
+
+  DISALLOW_COPY_AND_ASSIGN(APIResponseValidator);
+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_RENDERER_BINDINGS_API_RESPONSE_VALIDATOR_H_

extensions/renderer/bindings/api_response_validator_unittest.cc

+// Copyright 2018 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/bindings/api_response_validator.h"
+
+#include <vector>
+
+#include "base/auto_reset.h"
+#include "base/bind.h"
+#include "base/optional.h"
+#include "extensions/renderer/bindings/api_binding_test.h"
+#include "extensions/renderer/bindings/api_binding_test_util.h"
+#include "extensions/renderer/bindings/api_binding_util.h"
+#include "extensions/renderer/bindings/api_invocation_errors.h"
+#include "extensions/renderer/bindings/api_signature.h"
+#include "extensions/renderer/bindings/api_type_reference_map.h"
+#include "extensions/renderer/bindings/argument_spec.h"
+#include "extensions/renderer/bindings/argument_spec_builder.h"
+#include "gin/converter.h"
+#include "v8/include/v8.h"
+
+namespace extensions {
+namespace {
+
+constexpr char kMethodName[] = "oneString";
+
+std::unique_ptr<APISignature> OneStringSignature() {
+  std::vector<std::unique_ptr<ArgumentSpec>> specs;
+  specs.push_back(ArgumentSpecBuilder(ArgumentType::STRING, "str").Build());
+  return std::make_unique<APISignature>(std::move(specs));
+}
+
+std::vector<v8::Local<v8::Value>> StringToV8Vector(
+    v8::Local<v8::Context> context,
+    const char* args) {
+  v8::Local<v8::Value> v8_args = V8ValueFromScriptSource(context, args);
+  if (v8_args.IsEmpty()) {
+    ADD_FAILURE() << "Could not convert args: " << args;
+    return std::vector<v8::Local<v8::Value>>();
+  }
+  EXPECT_TRUE(v8_args->IsArray());
+  std::vector<v8::Local<v8::Value>> vector_args;
+  EXPECT_TRUE(gin::ConvertFromV8(context->GetIsolate(), v8_args, &vector_args));
+  return vector_args;
+}
+
+}  // namespace
+
+class APIResponseValidatorTest : public APIBindingTest {
+ public:
+  APIResponseValidatorTest()
+      : type_refs_(APITypeReferenceMap::InitializeTypeCallback()),
+        test_handler_(
+            base::BindRepeating(&APIResponseValidatorTest::OnValidationFailure,
+                                base::Unretained(this))),
+        validator_(&type_refs_) {}
+  ~APIResponseValidatorTest() override = default;
+
+  void SetUp() override {
+    APIBindingTest::SetUp();
+    response_validation_override_ =
+        binding::SetResponseValidationEnabledForTesting(true);
+    type_refs_.AddCallbackSignature(kMethodName, OneStringSignature());
+  }
+
+  void TearDown() override {
+    response_validation_override_.reset();
+    APIBindingTest::TearDown();
+  }
+
+  APIResponseValidator* validator() { return &validator_; }
+  const base::Optional<std::string>& failure_method() const {
+    return failure_method_;
+  }
+  const base::Optional<std::string>& failure_error() const {
+    return failure_error_;
+  }
+
+  void reset() {
+    failure_method_ = base::nullopt;
+    failure_error_ = base::nullopt;
+  }
+
+ private:
+  void OnValidationFailure(const std::string& method,
+                           const std::string& error) {
+    failure_method_ = method;
+    failure_error_ = error;
+  }
+
+  std::unique_ptr<base::AutoReset<bool>> response_validation_override_;
+
+  APITypeReferenceMap type_refs_;
+  APIResponseValidator::TestHandler test_handler_;
+  APIResponseValidator validator_;
+
+  base::Optional<std::string> failure_method_;
+  base::Optional<std::string> failure_error_;
+
+  DISALLOW_COPY_AND_ASSIGN(APIResponseValidatorTest);
+};
+
+TEST_F(APIResponseValidatorTest, TestValidation) {
+  using namespace api_errors;
+
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  validator()->ValidateResponse(
+      context, kMethodName, StringToV8Vector(context, "['hi']"), std::string(),
+      APIResponseValidator::CallbackType::kCallerProvided);
+  EXPECT_FALSE(failure_method());
+  EXPECT_FALSE(failure_error());
+
+  validator()->ValidateResponse(
+      context, kMethodName, StringToV8Vector(context, "[1]"), std::string(),
+      APIResponseValidator::CallbackType::kCallerProvided);
+  EXPECT_EQ(kMethodName, failure_method().value_or("no value"));
+  EXPECT_EQ(ArgumentError("str", InvalidType(kTypeString, kTypeInteger)),
+            failure_error().value_or("no value"));
+}
+
+TEST_F(APIResponseValidatorTest, TestDoesNotValidateWithAPIProvidedCallback) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  validator()->ValidateResponse(
+      context, kMethodName, StringToV8Vector(context, "['hi']"), std::string(),
+      APIResponseValidator::CallbackType::kCallerProvided);
+  EXPECT_FALSE(failure_method());
+  EXPECT_FALSE(failure_error());
+
+  validator()->ValidateResponse(
+      context, kMethodName, StringToV8Vector(context, "[1]"), std::string(),
+      APIResponseValidator::CallbackType::kAPIProvided);
+  EXPECT_FALSE(failure_method());
+  EXPECT_FALSE(failure_error());
+}
+
+TEST_F(APIResponseValidatorTest, TestDoesNotValidateWhenAPIErrorPresent) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  validator()->ValidateResponse(
+      context, kMethodName, {}, "Some API Error",
+      APIResponseValidator::CallbackType::kCallerProvided);
+  EXPECT_FALSE(failure_method());
+  EXPECT_FALSE(failure_error());
+}
+
+TEST_F(APIResponseValidatorTest, TestDoesNotValidateWithUnknownSignature) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  validator()->ValidateResponse(
+      context, "differentMethodName", StringToV8Vector(context, "[true]"),
+      std::string(), APIResponseValidator::CallbackType::kCallerProvided);
+  EXPECT_FALSE(failure_method());
+  EXPECT_FALSE(failure_error());
+}
+
+}  // namespace extensions

extensions/renderer/bindings/api_signature.cc

@@ -44,7 +44,7 @@ class ArgumentParser {
         error_(error) {}
 
   // Tries to parse the arguments against the expected signature.
-  bool ParseArguments();
+  bool ParseArguments(bool signature_has_callback);
 
  protected:
   v8::Isolate* GetIsolate() { return context_->GetIsolate(); }
@@ -176,7 +176,7 @@ class BaseValueArgumentParser : public ArgumentParser {
   DISALLOW_COPY_AND_ASSIGN(BaseValueArgumentParser);
 };
 
-bool ArgumentParser::ParseArguments() {
+bool ArgumentParser::ParseArguments(bool signature_has_callback) {
   if (provided_arguments_.size() > signature_.size()) {
     *error_ = api_errors::NoMatchingSignature();
     return false;
@@ -190,7 +190,6 @@ bool ArgumentParser::ParseArguments() {
   }
   DCHECK_EQ(resolved_arguments.size(), signature_.size());
 
-  bool signature_has_callback = HasCallback(signature_);
   size_t end_size =
       signature_has_callback ? signature_.size() - 1 : signature_.size();
   for (size_t i = 0; i < end_size; ++i) {
@@ -325,10 +324,13 @@ APISignature::APISignature(const base::ListValue& specification) {
     CHECK(value.GetAsDictionary(&param));
     signature_.push_back(std::make_unique<ArgumentSpec>(*param));
   }
+
+  has_callback_ = HasCallback(signature_);
 }
 
 APISignature::APISignature(std::vector<std::unique_ptr<ArgumentSpec>> signature)
-    : signature_(std::move(signature)) {}
+    : signature_(std::move(signature)),
+      has_callback_(HasCallback(signature_)) {}
 
 APISignature::~APISignature() {}
 
@@ -342,7 +344,7 @@ bool APISignature::ParseArgumentsToV8(
   std::vector<v8::Local<v8::Value>> v8_values;
   V8ArgumentParser parser(
       context, signature_, arguments, type_refs, error, &v8_values);
-  if (!parser.ParseArguments())
+  if (!parser.ParseArguments(has_callback_))
     return false;
   *v8_out = std::move(v8_values);
   return true;
@@ -360,7 +362,7 @@ bool APISignature::ParseArgumentsToJSON(
   std::unique_ptr<base::ListValue> json = std::make_unique<base::ListValue>();
   BaseValueArgumentParser parser(
       context, signature_, arguments, type_refs, error, json.get());
-  if (!parser.ParseArguments())
+  if (!parser.ParseArguments(has_callback_))
     return false;
   *json_out = std::move(json);
   *callback_out = parser.callback();
@@ -415,6 +417,56 @@ bool APISignature::ConvertArgumentsIgnoringSchema(
   return true;
 }
 
+bool APISignature::ValidateResponse(
+    v8::Local<v8::Context> context,
+    const std::vector<v8::Local<v8::Value>>& arguments,
+    const APITypeReferenceMap& type_refs,
+    std::string* error) const {
+  size_t expected_size = signature_.size();
+  size_t actual_size = arguments.size();
+  if (actual_size > expected_size) {
+    *error = api_errors::TooManyArguments();
+    return false;
+  }
+
+  // Easy validation: arguments go in order, and must match the expected schema.
+  // Anything less is failure.
+  std::string parse_error;
+  for (size_t i = 0; i < actual_size; ++i) {
+    DCHECK(!arguments[i].IsEmpty());
+    const ArgumentSpec& spec = *signature_[i];
+    if (arguments[i]->IsNullOrUndefined()) {
+      if (!spec.optional()) {
+        *error = api_errors::MissingRequiredArgument(spec.name().c_str());
+        return false;
+      }
+      continue;
+    }
+
+    if (!spec.ParseArgument(context, arguments[i], type_refs, nullptr, nullptr,
+                            &parse_error)) {
+      *error = api_errors::ArgumentError(spec.name(), parse_error);
+      return false;
+    }
+  }
+
+  // Responses may omit trailing optional parameters (which would then be
+  // undefined for the caller).
+  // NOTE(devlin): It might be nice to see if we could require all arguments to
+  // be present, no matter what. For one, it avoids this loop, and it would also
+  // unify what a "not found" value was (some APIs use undefined, some use
+  // null).
+  for (size_t i = actual_size; i < expected_size; ++i) {
+    if (!signature_[i]->optional()) {
+      *error =
+          api_errors::MissingRequiredArgument(signature_[i]->name().c_str());
+      return false;
+    }
+  }
+
+  return true;
+}
+
 std::string APISignature::GetExpectedSignature() const {
   if (!expected_signature_.empty() || signature_.empty())
     return expected_signature_;

extensions/renderer/bindings/api_signature.h

@@ -60,15 +60,27 @@ class APISignature {
       std::unique_ptr<base::ListValue>* json_out,
       v8::Local<v8::Function>* callback_out) const;
 
+  // Validates the provided |arguments| as if they were returned as a response
+  // to an API call. This validation is much stricter than the versions above,
+  // since response arguments are not allowed to have optional inner parameters.
+  bool ValidateResponse(v8::Local<v8::Context> context,
+                        const std::vector<v8::Local<v8::Value>>& arguments,
+                        const APITypeReferenceMap& type_refs,
+                        std::string* error) const;
+
   // Returns a developer-readable string of the expected signature. For
   // instance, if this signature expects a string 'someStr' and an optional int
   // 'someInt', this would return "string someStr, optional integer someInt".
   std::string GetExpectedSignature() const;
 
+  bool has_callback() const { return has_callback_; }
+
  private:
   // The list of expected arguments.
   std::vector<std::unique_ptr<ArgumentSpec>> signature_;
 
+  bool has_callback_ = false;
+
   // A developer-readable signature string, lazily set.
   mutable std::string expected_signature_;
 

extensions/renderer/bindings/api_signature_unittest.cc

@@ -206,6 +206,17 @@ class APISignatureTest : public APIBindingTest {
             expected_error);
   }
 
+  void ExpectResponsePass(const APISignature& signature,
+                          base::StringPiece arg_values) {
+    RunResponseTest(signature, arg_values, base::nullopt);
+  }
+
+  void ExpectResponseFailure(const APISignature& signature,
+                             base::StringPiece arg_values,
+                             const std::string& expected_error) {
+    RunResponseTest(signature, arg_values, expected_error);
+  }
+
   const APITypeReferenceMap& type_refs() const { return type_refs_; }
 
  private:
@@ -239,6 +250,27 @@ class APISignatureTest : public APIBindingTest {
     }
   }
 
+  void RunResponseTest(const APISignature& signature,
+                       base::StringPiece arg_values,
+                       base::Optional<std::string> expected_error) {
+    SCOPED_TRACE(arg_values);
+    v8::Local<v8::Context> context = MainContext();
+    v8::Local<v8::Value> v8_args = V8ValueFromScriptSource(context, arg_values);
+    ASSERT_FALSE(v8_args.IsEmpty());
+    ASSERT_TRUE(v8_args->IsArray());
+    std::vector<v8::Local<v8::Value>> vector_args;
+    ASSERT_TRUE(gin::ConvertFromV8(isolate(), v8_args, &vector_args));
+
+    std::string error;
+    bool should_succeed = !expected_error;
+    bool success =
+        signature.ValidateResponse(context, vector_args, type_refs_, &error);
+    EXPECT_EQ(should_succeed, success) << error;
+    ASSERT_EQ(should_succeed, error.empty());
+    if (!should_succeed)
+      EXPECT_EQ(*expected_error, error);
+  }
+
   APITypeReferenceMap type_refs_;
 
   DISALLOW_COPY_AND_ASSIGN(APISignatureTest);
@@ -523,4 +555,46 @@ TEST_F(APISignatureTest, ParseArgumentsToV8) {
   EXPECT_EQ("baz", prop2);
 }
 
+// Tests response validation, which is stricter than typical validation.
+TEST_F(APISignatureTest, ValidateResponse) {
+  using namespace api_errors;
+
+  v8::HandleScope handle_scope(isolate());
+
+  {
+    auto signature = StringAndInt();
+    ExpectResponsePass(*signature, "['hello', 42]");
+    ExpectResponseFailure(
+        *signature, "['hello', 'goodbye']",
+        ArgumentError("int", InvalidType(kTypeInteger, kTypeString)));
+  }
+
+  {
+    auto signature = StringOptionalIntAndBool();
+    ExpectResponsePass(*signature, "['hello', 42, true]");
+    ExpectResponsePass(*signature, "['hello', null, true]");
+    // Responses are not allowed to omit optional inner parameters.
+    ExpectResponseFailure(
+        *signature, "['hello', true]",
+        ArgumentError("int", InvalidType(kTypeInteger, kTypeBoolean)));
+  }
+
+  {
+    SpecVector specs;
+    specs.push_back(
+        ArgumentSpecBuilder(ArgumentType::STRING, "string").Build());
+    specs.push_back(ArgumentSpecBuilder(ArgumentType::INTEGER, "int")
+                        .MakeOptional()
+                        .Build());
+    auto signature = std::make_unique<APISignature>(std::move(specs));
+    // Responses *are* allowed to omit optional trailing parameters (which will
+    // then be `undefined` to the caller).
+    ExpectResponsePass(*signature, "['hello']");
+
+    ExpectResponseFailure(
+        *signature, "['hello', true]",
+        ArgumentError("int", InvalidType(kTypeInteger, kTypeBoolean)));
+  }
+}
+
 }  // namespace extensions

extensions/renderer/bindings/api_type_reference_map.cc

@@ -76,6 +76,20 @@ bool APITypeReferenceMap::HasTypeMethodSignature(
   return type_methods_.find(name) != type_methods_.end();
 }
 
+void APITypeReferenceMap::AddCallbackSignature(
+    const std::string& name,
+    std::unique_ptr<APISignature> signature) {
+  DCHECK(callback_signatures_.find(name) == callback_signatures_.end())
+      << "Cannot re-register signature for: " << name;
+  callback_signatures_[name] = std::move(signature);
+}
+
+const APISignature* APITypeReferenceMap::GetCallbackSignature(
+    const std::string& name) const {
+  auto iter = callback_signatures_.find(name);
+  return iter == callback_signatures_.end() ? nullptr : iter->second.get();
+}
+
 void APITypeReferenceMap::AddCustomSignature(
     const std::string& name,
     std::unique_ptr<APISignature> signature) {

extensions/renderer/bindings/api_type_reference_map.h

@@ -64,6 +64,12 @@ class APITypeReferenceMap {
   // Looks up a custom signature that was previously added.
   const APISignature* GetCustomSignature(const std::string& name) const;
 
+  // Adds an expected signature for an API callback.
+  void AddCallbackSignature(const std::string& name,
+                            std::unique_ptr<APISignature> signature);
+
+  const APISignature* GetCallbackSignature(const std::string& name) const;
+
   bool empty() const { return type_refs_.empty(); }
   size_t size() const { return type_refs_.size(); }
 
@@ -74,6 +80,7 @@ class APITypeReferenceMap {
   std::map<std::string, std::unique_ptr<APISignature>> api_methods_;
   std::map<std::string, std::unique_ptr<APISignature>> type_methods_;
   std::map<std::string, std::unique_ptr<APISignature>> custom_signatures_;
+  std::map<std::string, std::unique_ptr<APISignature>> callback_signatures_;
 
   DISALLOW_COPY_AND_ASSIGN(APITypeReferenceMap);
 };

extensions/renderer/native_extension_bindings_system_unittest.cc

@@ -5,6 +5,7 @@
 #include "extensions/renderer/native_extension_bindings_system_test_base.h"
 
 #include "base/strings/stringprintf.h"
+#include "base/test/bind_test_util.h"
 #include "components/crx_file/id_util.h"
 #include "extensions/common/extension_api.h"
 #include "extensions/common/extension_builder.h"
@@ -14,6 +15,7 @@
 #include "extensions/common/value_builder.h"
 #include "extensions/renderer/bindings/api_binding_test_util.h"
 #include "extensions/renderer/bindings/api_invocation_errors.h"
+#include "extensions/renderer/bindings/api_response_validator.h"
 #include "extensions/renderer/bindings/test_js_runner.h"
 #include "extensions/renderer/message_target.h"
 #include "extensions/renderer/native_extension_bindings_system.h"
@@ -1115,4 +1117,109 @@ TEST_F(NativeExtensionBindingsSystemUnittest, APIIsInitializedByOwningContext) {
   EXPECT_EQ(context, api_bridge.As<v8::Object>()->CreationContext());
 }
 
+class ResponseValidationNativeExtensionBindingsSystemUnittest
+    : public NativeExtensionBindingsSystemUnittest,
+      public testing::WithParamInterface<bool> {
+ public:
+  ResponseValidationNativeExtensionBindingsSystemUnittest() = default;
+  ~ResponseValidationNativeExtensionBindingsSystemUnittest() override = default;
+
+  void SetUp() override {
+    response_validation_override_ =
+        binding::SetResponseValidationEnabledForTesting(GetParam());
+    NativeExtensionBindingsSystemUnittest::SetUp();
+  }
+
+  void TearDown() override {
+    NativeExtensionBindingsSystemUnittest::TearDown();
+    response_validation_override_.reset();
+  }
+
+ private:
+  std::unique_ptr<base::AutoReset<bool>> response_validation_override_;
+
+  DISALLOW_COPY_AND_ASSIGN(
+      ResponseValidationNativeExtensionBindingsSystemUnittest);
+};
+
+TEST_P(ResponseValidationNativeExtensionBindingsSystemUnittest,
+       ResponseValidation) {
+  // The APIResponseValidator should only be used if response validation is
+  // enabled. Otherwise, it should be null.
+  EXPECT_EQ(GetParam(), bindings_system()
+                            ->api_system()
+                            ->request_handler()
+                            ->has_response_validator_for_testing());
+
+  base::Optional<std::string> validation_failure_method_name;
+  base::Optional<std::string> validation_failure_error;
+
+  auto on_validation_failure =
+      [&validation_failure_method_name, &validation_failure_error](
+          const std::string& method_name, const std::string& error) {
+        validation_failure_method_name = method_name;
+        validation_failure_error = error;
+      };
+  APIResponseValidator::TestHandler test_validation_failure_handler(
+      base::BindLambdaForTesting(on_validation_failure));
+
+  scoped_refptr<Extension> extension =
+      ExtensionBuilder("foo")
+          .AddPermissions({"idle", "power", "webRequest"})
+          .Build();
+  RegisterExtension(extension);
+
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  ScriptContext* script_context = CreateScriptContext(
+      context, extension.get(), Feature::BLESSED_EXTENSION_CONTEXT);
+  script_context->set_url(extension->url());
+
+  bindings_system()->UpdateBindingsForContext(script_context);
+
+  const char kCallIdleQueryState[] =
+      "(function() { chrome.idle.queryState(30, function() {}); })";
+
+  v8::Local<v8::Function> call_idle_query_state =
+      FunctionFromString(context, kCallIdleQueryState);
+  RunFunctionOnGlobal(call_idle_query_state, context, 0, nullptr);
+
+  EXPECT_FALSE(validation_failure_method_name);
+  EXPECT_FALSE(validation_failure_error);
+
+  // Respond with a valid value. Validation should not fail.
+  ASSERT_TRUE(has_last_params());
+  bindings_system()->HandleResponse(last_params().request_id, true,
+                                    *ListValueFromString("['active']"),
+                                    std::string());
+
+  EXPECT_FALSE(validation_failure_method_name);
+  EXPECT_FALSE(validation_failure_error);
+
+  // Run the function again, and response with an invalid value.
+  RunFunctionOnGlobal(call_idle_query_state, context, 0, nullptr);
+  ASSERT_TRUE(has_last_params());
+  bindings_system()->HandleResponse(last_params().request_id, true,
+                                    *ListValueFromString("['bad enum']"),
+                                    std::string());
+
+  // Validation should fail iff response validation is enabled.
+  if (GetParam()) {
+    EXPECT_EQ("idle.queryState",
+              validation_failure_method_name.value_or("no value"));
+    EXPECT_EQ(api_errors::ArgumentError(
+                  "newState",
+                  api_errors::InvalidEnumValue({"active", "idle", "locked"})),
+              validation_failure_error.value_or("no value"));
+  } else {
+    EXPECT_FALSE(validation_failure_method_name);
+    EXPECT_FALSE(validation_failure_error);
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(,
+                        ResponseValidationNativeExtensionBindingsSystemUnittest,
+                        testing::Bool());
+
 }  // namespace extensions