fix(storage): add extra validation for type in object_metadata_parser

google/cloud/storage/internal/metadata_parser.cc

@@ -121,6 +121,17 @@ StatusOr<std::chrono::system_clock::time_point> ParseTimestampField(
   return google::cloud::internal::InvalidArgumentError(std::move(os).str(),
                                                        GCP_ERROR_INFO());
 }
+StatusOr<std::string> ParseStringField(nlohmann::json const& json,
+                                       char const* field_name) {
+  auto it = json.find(field_name);
+  if (it == json.end() || it->is_null()) return std::string{};
+  if (it->is_string()) return it->get<std::string>();
+
+  std::ostringstream os;
+  os << "Error parsing field <" << field_name << "> as a string, json=" << json;
+  return google::cloud::internal::InvalidArgumentError(std::move(os).str(),
+                                                       GCP_ERROR_INFO());
+}
 
 Status NotJsonObject(nlohmann::json const& j,
                      google::cloud::internal::ErrorInfoBuilder eib) {

google/cloud/storage/internal/metadata_parser.h

@@ -80,6 +80,15 @@ StatusOr<std::uint64_t> ParseUnsignedLongField(nlohmann::json const& json,
 StatusOr<std::chrono::system_clock::time_point> ParseTimestampField(
     nlohmann::json const& json, char const* field_name);
 
+/**
+ * Parses a string field.
+ *
+ * @return the value of @p field_name in @p json, or an empty string if the
+ * field is not present.
+ */
+StatusOr<std::string> ParseStringField(nlohmann::json const& json,
+                                       char const* field_name);
+
 /// Return an error indicating that `j` is not a JSON object (it may be a
 /// string, or other valid JSON)
 Status NotJsonObject(nlohmann::json const& j,

google/cloud/storage/internal/metadata_parser_test.cc

@@ -235,6 +235,52 @@ TEST(MetadataParserTest, ParseIntegralFieldInvalidFieldType) {
   CheckParseInvalidFieldType<std::uint64_t>(&ParseUnsignedLongField);
 }
 
+/// @test Verify that we parse string values in JSON objects.
+TEST(MetadataParserTest, ParseStringField) {
+  std::string text = R"""({
+      "field1": "value1",
+      "field2": ""
+  })""";
+  auto json_object = nlohmann::json::parse(text);
+  EXPECT_EQ("value1", ParseStringField(json_object, "field1").value());
+  EXPECT_EQ("", ParseStringField(json_object, "field2").value());
+}
+
+/// @test Verify that we parse missing string values in JSON objects.
+TEST(MetadataParserTest, ParseMissingStringField) {
+  std::string text = R"""({
+      "some-field": "some-value"
+  })""";
+  auto json_object = nlohmann::json::parse(text);
+  auto actual = ParseStringField(json_object, "missing-field").value();
+  EXPECT_EQ("", actual);
+}
+
+/// @test Verify that we return an error for invalid string field types.
+TEST(MetadataParserTest, ParseInvalidStringFieldType) {
+  std::string text = R"""({
+      "field": 12345
+  })""";
+  auto json_object = nlohmann::json::parse(text);
+
+  auto actual = ParseStringField(json_object, "field");
+  EXPECT_THAT(actual, StatusIs(StatusCode::kInvalidArgument));
+  EXPECT_THAT(actual.status().message(),
+              ::testing::HasSubstr("Error parsing field <field> as a string"));
+}
+
+/// @test Verify that we return an error for complex types where a string is
+/// expected.
+TEST(MetadataParserTest, ParseInvalidStringFieldTypeComplex) {
+  std::string text = R"""({
+      "field": [1, 2, 3]
+  })""";
+  auto json_object = nlohmann::json::parse(text);
+
+  EXPECT_THAT(ParseStringField(json_object, "field"),
+              StatusIs(StatusCode::kInvalidArgument));
+}
+
 TEST(MetadataParserTest, NotJsonObject) {
   EXPECT_THAT(NotJsonObject(nlohmann::json{}, GCP_ERROR_INFO()),
               StatusIs(StatusCode::kInvalidArgument));

google/cloud/storage/internal/object_metadata_parser.cc

@@ -19,6 +19,7 @@
 #include <nlohmann/json.hpp>
 #include <functional>
 #include <map>
+#include <sstream>
 #include <string>
 #include <utility>
 #include <vector>
@@ -109,7 +110,19 @@ Status ParseMetadata(ObjectMetadata& meta, nlohmann::json const& json) {
   if (f == json.end()) return Status{};
   std::map<std::string, std::string> metadata;
   for (auto const& kv : f->items()) {
-    metadata.emplace(kv.key(), kv.value().get<std::string>());
+    if (!kv.value().is_string()) {
+      if (kv.value().is_null()) {
+        metadata.emplace(kv.key(), std::string{});
+      } else {
+        std::ostringstream os;
+        os << "Error parsing field <metadata." << kv.key()
+           << "> as a string, json=" << kv.value();
+        return google::cloud::internal::InvalidArgumentError(
+            std::move(os).str(), GCP_ERROR_INFO());
+      }
+    } else {
+      metadata.emplace(kv.key(), kv.value().get<std::string>());
+    }
   }
   meta.mutable_metadata() = std::move(metadata);
   return Status{};
@@ -204,6 +217,14 @@ Status ParseHardDeleteTime(ObjectMetadata& meta, nlohmann::json const& json) {
   return Status{};
 }
 
+Status SetStringField(ObjectMetadata& meta, nlohmann::json const& json,
+                      char const* key,
+                      ObjectMetadata& (ObjectMetadata::*setter)(std::string)) {
+  auto v = internal::ParseStringField(json, key);
+  if (!v) return std::move(v).status();
+  (meta.*setter)(*std::move(v));
+  return Status{};
+}
 }  // namespace
 
 StatusOr<ObjectMetadata> ObjectMetadataParser::FromJson(
@@ -214,78 +235,74 @@ StatusOr<ObjectMetadata> ObjectMetadataParser::FromJson(
   Parser parsers[] = {
       ParseAcl,
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_bucket(json.value("bucket", ""));
-        return Status{};
+        return SetStringField(meta, json, "bucket",
+                              &ObjectMetadata::set_bucket);
       },
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_cache_control(json.value("cacheControl", ""));
-        return Status{};
+        return SetStringField(meta, json, "cacheControl",
+                              &ObjectMetadata::set_cache_control);
       },
       ParseComponentCount,
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_content_disposition(json.value("contentDisposition", ""));
-        return Status{};
+        return SetStringField(meta, json, "contentDisposition",
+                              &ObjectMetadata::set_content_disposition);
       },
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_content_encoding(json.value("contentEncoding", ""));
-        return Status{};
+        return SetStringField(meta, json, "contentEncoding",
+                              &ObjectMetadata::set_content_encoding);
       },
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_content_language(json.value("contentLanguage", ""));
-        return Status{};
+        return SetStringField(meta, json, "contentLanguage",
+                              &ObjectMetadata::set_content_language);
       },
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_content_type(json.value("contentType", ""));
-        return Status{};
+        return SetStringField(meta, json, "contentType",
+                              &ObjectMetadata::set_content_type);
       },
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_crc32c(json.value("crc32c", ""));
-        return Status{};
+        return SetStringField(meta, json, "crc32c",
+                              &ObjectMetadata::set_crc32c);
       },
       ParseCustomTime,
       ParseCustomerEncryption,
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_etag(json.value("etag", ""));
-        return Status{};
+        return SetStringField(meta, json, "etag", &ObjectMetadata::set_etag);
       },
       ParseEventBasedHold,
       ParseGeneration,
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_id(json.value("id", ""));
-        return Status{};
+        return SetStringField(meta, json, "id", &ObjectMetadata::set_id);
       },
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_kind(json.value("kind", ""));
-        return Status{};
+        return SetStringField(meta, json, "kind", &ObjectMetadata::set_kind);
       },
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_kms_key_name(json.value("kmsKeyName", ""));
-        return Status{};
+        return SetStringField(meta, json, "kmsKeyName",
+                              &ObjectMetadata::set_kms_key_name);
       },
       ParseMetageneration,
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_md5_hash(json.value("md5Hash", ""));
-        return Status{};
+        return SetStringField(meta, json, "md5Hash",
+                              &ObjectMetadata::set_md5_hash);
       },
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_media_link(json.value("mediaLink", ""));
-        return Status{};
+        return SetStringField(meta, json, "mediaLink",
+                              &ObjectMetadata::set_media_link);
       },
       ParseMetadata,
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_name(json.value("name", ""));
-        return Status{};
+        return SetStringField(meta, json, "name", &ObjectMetadata::set_name);
       },
       ParseOwner,
       ParseRetentionExpirationTime,
       ParseRetention,
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_self_link(json.value("selfLink", ""));
-        return Status{};
+        return SetStringField(meta, json, "selfLink",
+                              &ObjectMetadata::set_self_link);
       },
       [](ObjectMetadata& meta, nlohmann::json const& json) {
-        meta.set_storage_class(json.value("storageClass", ""));
-        return Status{};
+        return SetStringField(meta, json, "storageClass",
+                              &ObjectMetadata::set_storage_class);
       },
       ParseSize,
       ParseTemporaryHold,