[access log] handle empty string in HeaderFormatter (#15077)

* [access log] Use a PlainStringFormatter for the empty string case. Fixes crash on this ASSERT with assumption there is always a format provider. Signed-off-by: Asra Ali <[email protected]>
envoyproxy · Feb 22, 2021 · 70fbf11 · 70fbf11
1 parent e29e3c0
commit 70fbf11
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 1 deletion.
diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc
@@ -504,7 +504,9 @@ SubstitutionFormatParser::parse(const std::string& format,
     pos = command_end_position;
   }
 
-  if (!current_token.empty()) {
+  if (!current_token.empty() || format.empty()) {
+    // Create a PlainStringFormatter with the final string literal. If the format string was empty,
+    // this creates a PlainStringFormatter with an empty string.
     formatters.emplace_back(FormatterProviderPtr{new PlainStringFormatter(current_token)});
   }
 

diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc
@@ -1861,6 +1861,32 @@ TEST(SubstitutionFormatterTest, StructFormatterSingleOperatorTest) {
       expected_json_map);
 }
 
+TEST(SubstitutionFormatterTest, EmptyStructFormatterTest) {
+  StreamInfo::MockStreamInfo stream_info;
+  Http::TestRequestHeaderMapImpl request_header;
+  Http::TestResponseHeaderMapImpl response_header;
+  Http::TestResponseTrailerMapImpl response_trailer;
+  std::string body;
+
+  envoy::config::core::v3::Metadata metadata;
+  populateMetadataTestData(metadata);
+  absl::optional<Http::Protocol> protocol = Http::Protocol::Http11;
+  EXPECT_CALL(stream_info, protocol()).WillRepeatedly(Return(protocol));
+
+  absl::node_hash_map<std::string, std::string> expected_json_map = {{"protocol", ""}};
+
+  ProtobufWkt::Struct key_mapping;
+  TestUtility::loadFromYaml(R"EOF(
+    protocol: ''
+  )EOF",
+                            key_mapping);
+  StructFormatter formatter(key_mapping, false, false);
+
+  verifyStructOutput(
+      formatter.format(request_header, response_header, response_trailer, stream_info, body),
+      expected_json_map);
+}
+
 TEST(SubstitutionFormatterTest, StructFormatterNonExistentHeaderTest) {
   StreamInfo::MockStreamInfo stream_info;
   Http::TestRequestHeaderMapImpl request_header{{"some_request_header", "SOME_REQUEST_HEADER"}};
@@ -2731,6 +2757,20 @@ TEST(SubstitutionFormatterTest, ParserSuccesses) {
   }
 }
 
+TEST(SubstitutionFormatterTest, EmptyFormatParse) {
+  Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, {":path", "/"}};
+  Http::TestResponseHeaderMapImpl response_headers;
+  Http::TestResponseTrailerMapImpl response_trailers;
+  StreamInfo::MockStreamInfo stream_info;
+  std::string body;
+
+  auto providers = SubstitutionFormatParser::parse("");
+
+  EXPECT_EQ(providers.size(), 1);
+  EXPECT_EQ("", providers[0]->format(request_headers, response_headers, response_trailers,
+                                     stream_info, body));
+}
+
 TEST(SubstitutionFormatterTest, FormatterExtension) {
   Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, {":path", "/"}};
   Http::TestResponseHeaderMapImpl response_headers;

diff --git a/test/extensions/filters/network/common/fuzz/network_readfilter_corpus/empty_providers b/test/extensions/filters/network/common/fuzz/network_readfilter_corpus/empty_providers
diff --git a/test/integration/local_reply_integration_test.cc b/test/integration/local_reply_integration_test.cc
@@ -85,6 +85,63 @@ TEST_P(LocalReplyIntegrationTest, MapStatusCodeAndFormatToJson) {
   EXPECT_TRUE(TestUtility::jsonStringEqual(response->body(), expected_body));
 }
 
+TEST_P(LocalReplyIntegrationTest, EmptyStructFormatter) {
+  const std::string yaml = R"EOF(
+mappers:
+  - filter:
+      header_filter:
+        header:
+          name: test-header
+          exact_match: exact-match-value
+    status_code: 550
+body_format:
+  json_format:
+    level: TRACE
+    user_agent: ""
+  )EOF";
+  setLocalReplyConfig(yaml);
+  initialize();
+
+  const std::string expected_body = R"({
+      "level": "TRACE",
+      "user_agent": "",
+})";
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  auto encoder_decoder = codec_client_->startRequest(
+      Http::TestRequestHeaderMapImpl{{":method", "POST"},
+                                     {":path", "/test/long/url"},
+                                     {":scheme", "http"},
+                                     {":authority", "host"},
+                                     {"test-header", "exact-match-value"}});
+  auto response = std::move(encoder_decoder.second);
+
+  ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_));
+
+  ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
+  ASSERT_TRUE(upstream_request_->waitForHeadersComplete());
+  ASSERT_TRUE(fake_upstream_connection_->close());
+  ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
+  response->waitForEndStream();
+
+  if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) {
+    ASSERT_TRUE(codec_client_->waitForDisconnect());
+  } else {
+    codec_client_->close();
+  }
+
+  EXPECT_FALSE(upstream_request_->complete());
+  EXPECT_EQ(0U, upstream_request_->bodyLength());
+
+  EXPECT_TRUE(response->complete());
+  EXPECT_EQ("application/json", response->headers().ContentType()->value().getStringView());
+  EXPECT_EQ("34", response->headers().ContentLength()->value().getStringView());
+  EXPECT_EQ("550", response->headers().Status()->value().getStringView());
+  // Check if returned json is same as expected
+  EXPECT_TRUE(TestUtility::jsonStringEqual(response->body(), expected_body));
+}
+
 // For grpc, the error message is in grpc-message header.
 // If it is json, the header value is in json format.
 TEST_P(LocalReplyIntegrationTest, MapStatusCodeAndFormatToJson4Grpc) {