From 6ab225d3c9e24ac22e93d5d12c44b9d4336d0ab4 Mon Sep 17 00:00:00 2001 From: htuch Date: Wed, 21 Aug 2019 17:08:36 -0400 Subject: [PATCH] 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 --- source/common/protobuf/utility.cc | 15 +++++++++++++++ source/common/protobuf/utility.h | 6 +----- test/common/protobuf/utility_test.cc | 25 +++++++++++++++++++++---- test/server/server_test.cc | 2 +- 4 files changed, 38 insertions(+), 10 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 50a95b419974..33185a00fc5e 100644 --- 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 index a05587338e26..2f2aff2a3f6e 100644 --- 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 index 04fb200fc075..9d437982a0e0 100644 --- 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(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 index ca2a755d26ac..1b9b0deeee19 100644 --- 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";