Image annotation service: use dedicated JSON parsing service for safety.

This CL switches out our usage of base::JSONReader for the data decoder service
(in another process on desktop Chrome), which is safer for parsing JSON
returned from an external server.

Bug: 916420
Change-Id: I3cb9b20af19064cfa877d88f06baeb417322f0a0
Reviewed-on: https://chromium-review.googlesource.com/c/1491068Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Andrew Moylan <amoylan@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Michael Martis <martis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636608}

services/image_annotation/BUILD.gn

@@ -15,8 +15,10 @@ source_set("lib") {
     "//components/google/core/common",
     "//mojo/public/cpp/bindings",
     "//net",
+    "//services/data_decoder/public/mojom",
     "//services/image_annotation/public/mojom",
     "//services/network/public/cpp",
+    "//services/service_manager/public/cpp",
     "//url",
   ]
 }
@@ -51,10 +53,14 @@ source_set("tests") {
     "//base/test:test_support",
     "//mojo/public/cpp/bindings",
     "//net",
+    "//services/data_decoder/public/cpp:test_support",
+    "//services/data_decoder/public/mojom",
     "//services/image_annotation/public/cpp",
     "//services/image_annotation/public/mojom",
     "//services/network:test_support",
     "//services/network/public/cpp",
+    "//services/service_manager/public/cpp",
+    "//services/service_manager/public/cpp/test:test_support",
     "//testing/gmock",
     "//testing/gtest",
     "//url",

services/image_annotation/DEPS

 include_rules = [
   "+components/google",
   "+net",
+  "+services/data_decoder",
   "+services/network",
   "+third_party/skia",
   "+ui/gfx",

services/image_annotation/annotator.cc

@@ -11,13 +11,14 @@
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/feature_list.h"
-#include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/location.h"
 #include "base/stl_util.h"
 #include "components/google/core/common/google_util.h"
 #include "net/base/load_flags.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
+#include "services/data_decoder/public/mojom/constants.mojom.h"
+#include "services/service_manager/public/cpp/connector.h"
 #include "url/gurl.h"
 
 namespace image_annotation {
@@ -153,18 +154,13 @@ std::tuple<bool, std::vector<mojom::AnnotationPtr>> ParseJsonDescAnnotations(
 
 // Attempts to extract annotation results from the server response, returning a
 // map from each source ID to its annotations (if successfully extracted).
-std::map<std::string, mojom::AnnotateImageResultPtr> ParseJsonResponse(
-    const std::string* const json_response,
+std::map<std::string, mojom::AnnotateImageResultPtr> UnpackJsonResponse(
+    const base::Value& json_data,
     const double min_ocr_confidence) {
-  if (!json_response)
+  if (!json_data.is_dict())
     return {};
 
-  const base::Optional<base::Value> response =
-      base::JSONReader::Read(*json_response);
-  if (!response.has_value() || !response->is_dict())
-    return {};
-
-  const base::Value* const results = response->FindKey("results");
+  const base::Value* const results = json_data.FindKey("results");
   if (!results || !results->is_list())
     return {};
 
@@ -240,8 +236,10 @@ Annotator::Annotator(
     const base::TimeDelta throttle,
     const int batch_size,
     const double min_ocr_confidence,
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+    service_manager::Connector* const connector)
     : url_loader_factory_(std::move(url_loader_factory)),
+      connector_(connector),
       http_request_timer_(
           FROM_HERE,
           throttle,
@@ -250,7 +248,9 @@ Annotator::Annotator(
       server_url_(std::move(server_url)),
       api_key_(std::move(api_key)),
       batch_size_(batch_size),
-      min_ocr_confidence_(min_ocr_confidence) {}
+      min_ocr_confidence_(min_ocr_confidence) {
+  DCHECK(connector_);
+}
 
 Annotator::~Annotator() {}
 
@@ -465,10 +465,37 @@ void Annotator::OnServerResponseReceived(
     const std::unique_ptr<std::string> json_response) {
   http_requests_.erase(http_request_it);
 
+  if (!json_response) {
+    DVLOG(1) << "HTTP request to image annotation server failed.";
+    ProcessResults(source_ids, {});
+    return;
+  }
+
+  // Send JSON string to a dedicated service for safe parsing.
+  GetJsonParser().Parse(*json_response,
+                        base::BindOnce(&Annotator::OnResponseJsonParsed,
+                                       base::Unretained(this), source_ids));
+}
+
+void Annotator::OnResponseJsonParsed(
+    const std::set<std::string>& source_ids,
+    const base::Optional<base::Value> json_data,
+    const base::Optional<std::string>& error) {
   // Extract annotation results for each source ID with valid results.
-  const std::map<std::string, mojom::AnnotateImageResultPtr> results =
-      ParseJsonResponse(json_response.get(), min_ocr_confidence_);
+  std::map<std::string, mojom::AnnotateImageResultPtr> results;
+  if (!json_data.has_value() || error.has_value()) {
+    DVLOG(1) << "Parsing server response JSON failed with error: "
+             << error.value_or("No reason reported.");
+    ProcessResults(source_ids, {});
+  } else {
+    ProcessResults(source_ids,
+                   UnpackJsonResponse(*json_data, min_ocr_confidence_));
+  }
+}
 
+void Annotator::ProcessResults(
+    const std::set<std::string>& source_ids,
+    const std::map<std::string, mojom::AnnotateImageResultPtr>& results) {
   // Process each source ID for which we expect to have results.
   for (const std::string& source_id : source_ids) {
     pending_source_ids_.erase(source_id);
@@ -502,6 +529,18 @@ void Annotator::OnServerResponseReceived(
   }
 }
 
+data_decoder::mojom::JsonParser& Annotator::GetJsonParser() {
+  if (!json_parser_) {
+    connector_->BindInterface(data_decoder::mojom::kServiceName,
+                              mojo::MakeRequest(&json_parser_));
+    json_parser_.set_connection_error_handler(base::BindOnce(
+        [](Annotator* const annotator) { annotator->json_parser_.reset(); },
+        base::Unretained(this)));
+  }
+
+  return *json_parser_;
+}
+
 void Annotator::RemoveRequestInfo(
     const std::string& source_id,
     const RequestInfoList::iterator request_info_it,

services/image_annotation/annotator.h

@@ -18,11 +18,16 @@
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
+#include "services/data_decoder/public/mojom/json_parser.mojom.h"
 #include "services/image_annotation/public/mojom/image_annotation.mojom.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/simple_url_loader.h"
 #include "url/gurl.h"
 
+namespace service_manager {
+class Connector;
+}  // namespace service_manager
+
 namespace image_annotation {
 
 // The annotator communicates with the external image annotation server to
@@ -61,7 +66,8 @@ class Annotator : public mojom::Annotator {
             base::TimeDelta throttle,
             int batch_size,
             double min_ocr_confidence,
-            scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
+            scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+            service_manager::Connector* connector);
   ~Annotator() override;
 
   // Start providing behavior for the given Mojo request.
@@ -101,6 +107,10 @@ class Annotator : public mojom::Annotator {
       HttpRequestQueue::iterator begin_it,
       HttpRequestQueue::iterator end_it);
 
+  // Create or reuse a connection to the data decoder service for safe JSON
+  // parsing.
+  data_decoder::mojom::JsonParser& GetJsonParser();
+
   // Removes the given request, reassigning local processing if its associated
   // image processor had some ongoing.
   void RemoveRequestInfo(const std::string& source_id,
@@ -123,6 +133,17 @@ class Annotator : public mojom::Annotator {
                                 UrlLoaderList::iterator http_request_it,
                                 std::unique_ptr<std::string> json_response);
 
+  // Called when the data decoder service provides parsed JSON data for a server
+  // response.
+  void OnResponseJsonParsed(const std::set<std::string>& source_ids,
+                            base::Optional<base::Value> json_data,
+                            const base::Optional<std::string>& error);
+
+  // Adds the given results to the cache (if successful) and notifies clients.
+  void ProcessResults(
+      const std::set<std::string>& source_ids,
+      const std::map<std::string, mojom::AnnotateImageResultPtr>& results);
+
   // Maps from source ID to previously-obtained annotation results.
   // TODO(crbug.com/916420): periodically clear entries from this cache.
   std::map<std::string, mojom::AnnotateImageResultPtr> cached_results_;
@@ -150,8 +171,13 @@ class Annotator : public mojom::Annotator {
 
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
+  service_manager::Connector* const connector_;
+
   mojo::BindingSet<mojom::Annotator> bindings_;
 
+  // Should not be used directly; GetJsonParser() should be called instead.
+  data_decoder::mojom::JsonParserPtr json_parser_;
+
   // A timer used to throttle HTTP request frequency.
   base::RepeatingTimer http_request_timer_;
 

services/image_annotation/annotator_unittest.cc

@@ -14,10 +14,15 @@
 #include "base/time/time.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "net/http/http_status_code.h"
+#include "services/data_decoder/public/cpp/test_data_decoder_service.h"
+#include "services/data_decoder/public/mojom/constants.mojom.h"
+#include "services/data_decoder/public/mojom/json_parser.mojom.h"
 #include "services/image_annotation/public/mojom/image_annotation.mojom.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "services/network/public/mojom/url_loader.mojom-shared.h"
 #include "services/network/test/test_url_loader_factory.h"
+#include "services/service_manager/public/cpp/connector.h"
+#include "services/service_manager/public/mojom/service.mojom.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -191,6 +196,8 @@ constexpr base::TimeDelta kThrottle = base::TimeDelta::FromSeconds(1);
 // An image processor that holds and exposes the callbacks it is passed.
 class TestImageProcessor : public mojom::ImageProcessor {
  public:
+  TestImageProcessor() = default;
+
   mojom::ImageProcessorPtr GetPtr() {
     mojom::ImageProcessorPtr ptr;
     bindings_.AddBinding(this, mojo::MakeRequest(&ptr));
@@ -212,6 +219,8 @@ class TestImageProcessor : public mojom::ImageProcessor {
   std::vector<GetJpgImageDataCallback> callbacks_;
 
   mojo::BindingSet<mojom::ImageProcessor> bindings_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestImageProcessor);
 };
 
 // A class that supports test URL loading for the "server" use case: where
@@ -298,6 +307,8 @@ class TestServerURLLoaderFactory {
   const std::string server_url_prefix_;
   network::TestURLLoaderFactory loader_factory_;
   scoped_refptr<network::SharedURLLoaderFactory> shared_loader_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestServerURLLoaderFactory);
 };
 
 // Returns a "canonically" formatted version of a JSON string by parsing and
@@ -335,11 +346,12 @@ TEST(AnnotatorTest, SuccessAndCache) {
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
   TestServerURLLoaderFactory test_url_factory(
       "https://ia-pa.googleapis.com/v1/");
+  data_decoder::TestDataDecoderService test_dd_service;
 
-  Annotator annotator(GURL(kTestServerUrl), std::string() /* api_key */,
-                      kThrottle, 1 /* batch_size */,
-                      1.0 /* min_ocr_confidence */,
-                      test_url_factory.AsSharedURLLoaderFactory());
+  Annotator annotator(
+      GURL(kTestServerUrl), std::string() /* api_key */, kThrottle,
+      1 /* batch_size */, 1.0 /* min_ocr_confidence */,
+      test_url_factory.AsSharedURLLoaderFactory(), test_dd_service.connector());
   TestImageProcessor processor;
 
   // First call performs original image annotation.
@@ -401,11 +413,12 @@ TEST(AnnotatorTest, HttpError) {
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
   TestServerURLLoaderFactory test_url_factory(
       "https://ia-pa.googleapis.com/v1/");
+  data_decoder::TestDataDecoderService test_dd_service;
 
-  Annotator annotator(GURL(kTestServerUrl), std::string() /* api_key */,
-                      kThrottle, 1 /* batch_size */,
-                      1.0 /* min_ocr_confidence */,
-                      test_url_factory.AsSharedURLLoaderFactory());
+  Annotator annotator(
+      GURL(kTestServerUrl), std::string() /* api_key */, kThrottle,
+      1 /* batch_size */, 1.0 /* min_ocr_confidence */,
+      test_url_factory.AsSharedURLLoaderFactory(), test_dd_service.connector());
 
   TestImageProcessor processor;
   base::Optional<mojom::AnnotateImageError> error;
@@ -446,11 +459,12 @@ TEST(AnnotatorTest, BackendError) {
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
   TestServerURLLoaderFactory test_url_factory(
       "https://ia-pa.googleapis.com/v1/");
+  data_decoder::TestDataDecoderService test_dd_service;
 
-  Annotator annotator(GURL(kTestServerUrl), std::string() /* api_key */,
-                      kThrottle, 1 /* batch_size */,
-                      1.0 /* min_ocr_confidence */,
-                      test_url_factory.AsSharedURLLoaderFactory());
+  Annotator annotator(
+      GURL(kTestServerUrl), std::string() /* api_key */, kThrottle,
+      1 /* batch_size */, 1.0 /* min_ocr_confidence */,
+      test_url_factory.AsSharedURLLoaderFactory(), test_dd_service.connector());
 
   TestImageProcessor processor;
   base::Optional<mojom::AnnotateImageError> error;
@@ -492,11 +506,12 @@ TEST(AnnotatorTest, ServerError) {
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
   TestServerURLLoaderFactory test_url_factory(
       "https://ia-pa.googleapis.com/v1/");
+  data_decoder::TestDataDecoderService test_dd_service;
 
-  Annotator annotator(GURL(kTestServerUrl), std::string() /* api_key */,
-                      kThrottle, 1 /* batch_size */,
-                      1.0 /* min_ocr_confidence */,
-                      test_url_factory.AsSharedURLLoaderFactory());
+  Annotator annotator(
+      GURL(kTestServerUrl), std::string() /* api_key */, kThrottle,
+      1 /* batch_size */, 1.0 /* min_ocr_confidence */,
+      test_url_factory.AsSharedURLLoaderFactory(), test_dd_service.connector());
 
   TestImageProcessor processor;
   base::Optional<mojom::AnnotateImageError> error;
@@ -538,11 +553,12 @@ TEST(AnnotatorTest, ProcessorFails) {
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
   TestServerURLLoaderFactory test_url_factory(
       "https://ia-pa.googleapis.com/v1/");
+  data_decoder::TestDataDecoderService test_dd_service;
 
-  Annotator annotator(GURL(kTestServerUrl), std::string() /* api_key */,
-                      kThrottle, 1 /* batch_size */,
-                      1.0 /* min_ocr_confidence */,
-                      test_url_factory.AsSharedURLLoaderFactory());
+  Annotator annotator(
+      GURL(kTestServerUrl), std::string() /* api_key */, kThrottle,
+      1 /* batch_size */, 1.0 /* min_ocr_confidence */,
+      test_url_factory.AsSharedURLLoaderFactory(), test_dd_service.connector());
 
   TestImageProcessor processor[3];
   base::Optional<mojom::AnnotateImageError> error[3];
@@ -601,11 +617,12 @@ TEST(AnnotatorTest, ProcessorDies) {
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
   TestServerURLLoaderFactory test_url_factory(
       "https://ia-pa.googleapis.com/v1/");
+  data_decoder::TestDataDecoderService test_dd_service;
 
-  Annotator annotator(GURL(kTestServerUrl), std::string() /* api_key */,
-                      kThrottle, 1 /* batch_size */,
-                      1.0 /* min_ocr_confidence */,
-                      test_url_factory.AsSharedURLLoaderFactory());
+  Annotator annotator(
+      GURL(kTestServerUrl), std::string() /* api_key */, kThrottle,
+      1 /* batch_size */, 1.0 /* min_ocr_confidence */,
+      test_url_factory.AsSharedURLLoaderFactory(), test_dd_service.connector());
 
   TestImageProcessor processor[3];
   base::Optional<mojom::AnnotateImageError> error[3];
@@ -663,11 +680,12 @@ TEST(AnnotatorTest, ConcurrentSameBatch) {
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
   TestServerURLLoaderFactory test_url_factory(
       "https://ia-pa.googleapis.com/v1/");
+  data_decoder::TestDataDecoderService test_dd_service;
 
-  Annotator annotator(GURL(kTestServerUrl), std::string() /* api_key */,
-                      kThrottle, 3 /* batch_size */,
-                      1.0 /* min_ocr_confidence */,
-                      test_url_factory.AsSharedURLLoaderFactory());
+  Annotator annotator(
+      GURL(kTestServerUrl), std::string() /* api_key */, kThrottle,
+      3 /* batch_size */, 1.0 /* min_ocr_confidence */,
+      test_url_factory.AsSharedURLLoaderFactory(), test_dd_service.connector());
 
   TestImageProcessor processor[3];
   base::Optional<mojom::AnnotateImageError> error[3];
@@ -724,11 +742,12 @@ TEST(AnnotatorTest, ConcurrentSeparateBatches) {
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
   TestServerURLLoaderFactory test_url_factory(
       "https://ia-pa.googleapis.com/v1/");
+  data_decoder::TestDataDecoderService test_dd_service;
 
-  Annotator annotator(GURL(kTestServerUrl), std::string() /* api_key */,
-                      kThrottle, 3 /* batch_size */,
-                      1.0 /* min_ocr_confidence */,
-                      test_url_factory.AsSharedURLLoaderFactory());
+  Annotator annotator(
+      GURL(kTestServerUrl), std::string() /* api_key */, kThrottle,
+      3 /* batch_size */, 1.0 /* min_ocr_confidence */,
+      test_url_factory.AsSharedURLLoaderFactory(), test_dd_service.connector());
 
   TestImageProcessor processor[2];
   base::Optional<mojom::AnnotateImageError> error[2];
@@ -829,11 +848,12 @@ TEST(AnnotatorTest, DuplicateWork) {
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
   TestServerURLLoaderFactory test_url_factory(
       "https://ia-pa.googleapis.com/v1/");
+  data_decoder::TestDataDecoderService test_dd_service;
 
-  Annotator annotator(GURL(kTestServerUrl), std::string() /* api_key */,
-                      kThrottle, 1 /* batch_size */,
-                      1.0 /* min_ocr_confidence */,
-                      test_url_factory.AsSharedURLLoaderFactory());
+  Annotator annotator(
+      GURL(kTestServerUrl), std::string() /* api_key */, kThrottle,
+      1 /* batch_size */, 1.0 /* min_ocr_confidence */,
+      test_url_factory.AsSharedURLLoaderFactory(), test_dd_service.connector());
 
   TestImageProcessor processor[4];
   base::Optional<mojom::AnnotateImageError> error[4];
@@ -920,6 +940,7 @@ TEST(AnnotatorTest, DuplicateWork) {
 TEST(AnnotatorTest, ApiKey) {
   base::test::ScopedTaskEnvironment test_task_env(
       base::test::ScopedTaskEnvironment::MainThreadType::MOCK_TIME);
+  data_decoder::TestDataDecoderService test_dd_service;
 
   // A call to a secure Google-owner server URL should include the specified API
   // key.
@@ -929,7 +950,8 @@ TEST(AnnotatorTest, ApiKey) {
 
     Annotator annotator(GURL(kTestServerUrl), "my_api_key", kThrottle,
                         1 /* batch_size */, 1.0 /* min_ocr_confidence */,
-                        test_url_factory.AsSharedURLLoaderFactory());
+                        test_url_factory.AsSharedURLLoaderFactory(),
+                        test_dd_service.connector());
     TestImageProcessor processor;
 
     annotator.AnnotateImage(kImage1Url, processor.GetPtr(), base::DoNothing());
@@ -961,7 +983,8 @@ TEST(AnnotatorTest, ApiKey) {
     Annotator annotator(GURL("http://ia-pa.googleapis.com/v1/annotation"),
                         "my_api_key", kThrottle, 1 /* batch_size */,
                         1.0 /* min_ocr_confidence */,
-                        test_url_factory.AsSharedURLLoaderFactory());
+                        test_url_factory.AsSharedURLLoaderFactory(),
+                        test_dd_service.connector());
     TestImageProcessor processor;
 
     annotator.AnnotateImage(kImage1Url, processor.GetPtr(), base::DoNothing());
@@ -990,7 +1013,8 @@ TEST(AnnotatorTest, ApiKey) {
     Annotator annotator(GURL("https://datascraper.com/annotation"),
                         "my_api_key", kThrottle, 1 /* batch_size */,
                         1.0 /* min_ocr_confidence */,
-                        test_url_factory.AsSharedURLLoaderFactory());
+                        test_url_factory.AsSharedURLLoaderFactory(),
+                        test_dd_service.connector());
     TestImageProcessor processor;
 
     annotator.AnnotateImage(kImage1Url, processor.GetPtr(), base::DoNothing());

services/image_annotation/image_annotation_service.cc

@@ -29,7 +29,8 @@ ImageAnnotationService::ImageAnnotationService(
                  base::TimeDelta::FromMilliseconds(kThrottleMs.Get()),
                  kBatchSize.Get(),
                  kMinOcrConfidence.Get(),
-                 shared_url_loader_factory) {}
+                 shared_url_loader_factory,
+                 service_binding_.GetConnector()) {}
 
 ImageAnnotationService::~ImageAnnotationService() = default;
 

services/image_annotation/public/cpp/BUILD.gn

@@ -24,6 +24,7 @@ source_set("manifest") {
 
   public_deps = [
     "//base",
+    "//services/data_decoder/public/mojom:constants",
     "//services/image_annotation/public/mojom",
     "//services/service_manager/public/cpp",
   ]

services/image_annotation/public/cpp/manifest.cc

@@ -5,6 +5,7 @@
 #include "services/image_annotation/public/cpp/manifest.h"
 
 #include "base/no_destructor.h"
+#include "services/data_decoder/public/mojom/constants.mojom.h"
 #include "services/image_annotation/public/mojom/constants.mojom.h"
 #include "services/image_annotation/public/mojom/image_annotation.mojom.h"
 #include "services/service_manager/public/cpp/manifest_builder.h"
@@ -19,6 +20,7 @@ const service_manager::Manifest& GetManifest() {
           .ExposeCapability(
               mojom::kAnnotationCapability,
               service_manager::Manifest::InterfaceList<mojom::Annotator>())
+          .RequireCapability(data_decoder::mojom::kServiceName, "json_parser")
           .Build()};
   return *manifest;
 }