protobuf: report field numbers for unknown fields. (#7978)

Since binary proto won't have field names, report at least the field numbers, as per https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.unknown_field_set#UnknownField. Also fix minor typo encountered while doing this work. Risk level: Low Testing: Unit tests added/updated. Fixes #7937 Signed-off-by: Harvey Tuch <htuch@google.com>
envoyproxy · Aug 21, 2019 · 6ab225d · 6ab225d
1 parent 09466b5
commit 6ab225d
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 10 deletions.
diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc
@@ -88,6 +88,21 @@ ProtoValidationException::ProtoValidationException(const std::string& validation
   ENVOY_LOG_MISC(debug, "Proto validation error; throwing {}", what());
 }
 
+void MessageUtil::checkUnknownFields(const Protobuf::Message& message,
+                                     ProtobufMessage::ValidationVisitor& validation_visitor) {
+  const auto& unknown_fields = message.GetReflection()->GetUnknownFields(message);
+  // If there are no unknown fields, we're done here.
+  if (unknown_fields.empty()) {
+    return;
+  }
+  std::string error_msg;
+  for (int n = 0; n < unknown_fields.field_count(); ++n) {
+    error_msg += absl::StrCat(n > 0 ? ", " : "", unknown_fields.field(n).number());
+  }
+  validation_visitor.onUnknownField("type " + message.GetTypeName() + " with unknown field set {" +
+                                    error_msg + "}");
+}
+
 void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message,
                                ProtobufMessage::ValidationVisitor& validation_visitor) {
   Protobuf::util::JsonParseOptions options;

diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h
@@ -207,11 +207,7 @@ class MessageUtil {
   }
 
   static void checkUnknownFields(const Protobuf::Message& message,
-                                 ProtobufMessage::ValidationVisitor& validation_visitor) {
-    if (!message.GetReflection()->GetUnknownFields(message).empty()) {
-      validation_visitor.onUnknownField("type " + message.GetTypeName());
-    }
-  }
+                                 ProtobufMessage::ValidationVisitor& validation_visitor);
 
   static void loadFromJson(const std::string& json, Protobuf::Message& message,
                            ProtobufMessage::ValidationVisitor& validation_visitor);

diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc
@@ -147,15 +147,31 @@ TEST_F(ProtobufUtilityTest, LoadBinaryProtoFromFile) {
   EXPECT_TRUE(TestUtility::protoEqual(bootstrap, proto_from_file));
 }
 
+// An unknown field (or with wrong type) in a message is rejected.
 TEST_F(ProtobufUtilityTest, LoadBinaryProtoUnknownFieldFromFile) {
   ProtobufWkt::Duration source_duration;
   source_duration.set_seconds(42);
   const std::string filename =
       TestEnvironment::writeStringToFileForTest("proto.pb", source_duration.SerializeAsString());
   envoy::config::bootstrap::v2::Bootstrap proto_from_file;
-  EXPECT_THROW_WITH_MESSAGE(
-      TestUtility::loadFromFile(filename, proto_from_file, *api_), EnvoyException,
-      "Protobuf message (type envoy.config.bootstrap.v2.Bootstrap) has unknown fields");
+  EXPECT_THROW_WITH_MESSAGE(TestUtility::loadFromFile(filename, proto_from_file, *api_),
+                            EnvoyException,
+                            "Protobuf message (type envoy.config.bootstrap.v2.Bootstrap with "
+                            "unknown field set {1}) has unknown fields");
+}
+
+// Multiple unknown fields (or with wrong type) in a message are rejected.
+TEST_F(ProtobufUtilityTest, LoadBinaryProtoUnknownMultipleFieldsFromFile) {
+  ProtobufWkt::Duration source_duration;
+  source_duration.set_seconds(42);
+  source_duration.set_nanos(42);
+  const std::string filename =
+      TestEnvironment::writeStringToFileForTest("proto.pb", source_duration.SerializeAsString());
+  envoy::config::bootstrap::v2::Bootstrap proto_from_file;
+  EXPECT_THROW_WITH_MESSAGE(TestUtility::loadFromFile(filename, proto_from_file, *api_),
+                            EnvoyException,
+                            "Protobuf message (type envoy.config.bootstrap.v2.Bootstrap with "
+                            "unknown field set {1, 2}) has unknown fields");
 }
 
 TEST_F(ProtobufUtilityTest, LoadTextProtoFromFile) {
@@ -333,7 +349,8 @@ TEST_F(ProtobufUtilityTest, AnyConvertWrongFields) {
   source_any.set_type_url("type.google.com/google.protobuf.Timestamp");
   EXPECT_THROW_WITH_MESSAGE(TestUtility::anyConvert<ProtobufWkt::Timestamp>(source_any),
                             EnvoyException,
-                            "Protobuf message (type google.protobuf.Timestamp) has unknown fields");
+                            "Protobuf message (type google.protobuf.Timestamp with unknown "
+                            "field set {1}) has unknown fields");
 }
 
 TEST_F(ProtobufUtilityTest, JsonConvertSuccess) {

diff --git a/test/server/server_test.cc b/test/server/server_test.cc
@@ -696,7 +696,7 @@ TEST_P(ServerInstanceImplTest, EmptyBootstrap) {
 }
 
 // Custom header bootstrap succeeds.
-TEST_P(ServerInstanceImplTest, CusomHeaderBoostrap) {
+TEST_P(ServerInstanceImplTest, CustomHeaderBootstrap) {
   options_.config_path_ = TestEnvironment::writeStringToFileForTest(
       "custom.yaml", "header_prefix: \"x-envoy\"\nstatic_resources:\n");
   options_.service_cluster_name_ = "some_cluster_name";