From 19d27bb6a60399bd142a00add46052bf1737231b Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 20 Aug 2019 15:56:52 -0400 Subject: [PATCH 1/3] protobuf: report field numbers for unknown fields. 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..6d98e638d5e9 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) { + 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(s) " + + 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..99d6ba693fa1 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(s) 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(s) 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(s) 1) has unknown fields"); } TEST_F(ProtobufUtilityTest, JsonConvertSuccess) { diff --git a/test/server/server_test.cc b/test/server/server_test.cc index ca2a755d26ac..ff4f8efa10c7 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, CusomHeaderBootstrap) { options_.config_path_ = TestEnvironment::writeStringToFileForTest( "custom.yaml", "header_prefix: \"x-envoy\"\nstatic_resources:\n"); options_.service_cluster_name_ = "some_cluster_name"; From a6fda7b22965d2ef2088cb6bc7a41db8931e572a Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 20 Aug 2019 17:56:03 -0400 Subject: [PATCH 2/3] More typo. Signed-off-by: Harvey Tuch --- test/server/server_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/server/server_test.cc b/test/server/server_test.cc index ff4f8efa10c7..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, CusomHeaderBootstrap) { +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"; From 7a660d7dd54875975ebeaf1c061e7f10067d19a9 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 21 Aug 2019 13:56:50 -0400 Subject: [PATCH 3/3] Review feedback. Signed-off-by: Harvey Tuch --- source/common/protobuf/utility.cc | 6 +++--- test/common/protobuf/utility_test.cc | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 6d98e638d5e9..33185a00fc5e 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -90,7 +90,7 @@ ProtoValidationException::ProtoValidationException(const std::string& validation void MessageUtil::checkUnknownFields(const Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor) { - auto& unknown_fields = message.GetReflection()->GetUnknownFields(message); + const auto& unknown_fields = message.GetReflection()->GetUnknownFields(message); // If there are no unknown fields, we're done here. if (unknown_fields.empty()) { return; @@ -99,8 +99,8 @@ void MessageUtil::checkUnknownFields(const Protobuf::Message& message, 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(s) " + - error_msg); + validation_visitor.onUnknownField("type " + message.GetTypeName() + " with unknown field set {" + + error_msg + "}"); } void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message, diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 99d6ba693fa1..9d437982a0e0 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -157,7 +157,7 @@ TEST_F(ProtobufUtilityTest, LoadBinaryProtoUnknownFieldFromFile) { EXPECT_THROW_WITH_MESSAGE(TestUtility::loadFromFile(filename, proto_from_file, *api_), EnvoyException, "Protobuf message (type envoy.config.bootstrap.v2.Bootstrap with " - "unknown field(s) 1) has unknown fields"); + "unknown field set {1}) has unknown fields"); } // Multiple unknown fields (or with wrong type) in a message are rejected. @@ -171,7 +171,7 @@ TEST_F(ProtobufUtilityTest, LoadBinaryProtoUnknownMultipleFieldsFromFile) { EXPECT_THROW_WITH_MESSAGE(TestUtility::loadFromFile(filename, proto_from_file, *api_), EnvoyException, "Protobuf message (type envoy.config.bootstrap.v2.Bootstrap with " - "unknown field(s) 1, 2) has unknown fields"); + "unknown field set {1, 2}) has unknown fields"); } TEST_F(ProtobufUtilityTest, LoadTextProtoFromFile) { @@ -350,7 +350,7 @@ TEST_F(ProtobufUtilityTest, AnyConvertWrongFields) { EXPECT_THROW_WITH_MESSAGE(TestUtility::anyConvert(source_any), EnvoyException, "Protobuf message (type google.protobuf.Timestamp with unknown " - "field(s) 1) has unknown fields"); + "field set {1}) has unknown fields"); } TEST_F(ProtobufUtilityTest, JsonConvertSuccess) {