From b6afa7cd94faafe5233f9b277d2e46150f0f0b53 Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Fri, 14 Oct 2016 10:49:01 -0700 Subject: [PATCH 1/9] test for escape --- test/common/http/access_log/access_log_formatter_test.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/common/http/access_log/access_log_formatter_test.cc b/test/common/http/access_log/access_log_formatter_test.cc index 4f269ef10814..e19f6d9e7a87 100644 --- a/test/common/http/access_log/access_log_formatter_test.cc +++ b/test/common/http/access_log/access_log_formatter_test.cc @@ -114,7 +114,7 @@ TEST(AccessLogFormatterTest, requestInfoFormatter) { TEST(AccessLogFormatterTest, requestHeaderFormatter) { MockRequestInfo requestInfo; - HeaderMapImpl request_header{{":method", "GET"}, {":path", "/"}}; + HeaderMapImpl request_header{{":method", "GET"}, {":path", "/"}, {"x-test", "hello\nworld"}}; HeaderMapImpl response_header{{":method", "PUT"}}; { @@ -136,6 +136,11 @@ TEST(AccessLogFormatterTest, requestHeaderFormatter) { RequestHeaderFormatter formatter("does_not_exist", "", Optional()); EXPECT_EQ("-", formatter.format(request_header, response_header, requestInfo)); } + + { + RequestHeaderFormatter formatter("x-test", "", Optional()); + EXPECT_EQ("hello\\nworld", formatter.format(request_header, response_header, requestInfo)); + } } TEST(AccessLogFormatterTest, responseHeaderFormatter) { From 4e7525f102d1b902f50379d8a69cb4efaf875e28 Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Mon, 31 Oct 2016 15:02:44 -0700 Subject: [PATCH 2/9] plumbing for boolean escape flag --- .../http/access_log/access_log_formatter.cc | 22 ++++++++++--------- .../http/access_log/access_log_formatter.h | 7 +++--- .../access_log/access_log_formatter_test.cc | 18 +++++++-------- 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index 0c57459f3cc4..acb251194a02 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -135,8 +135,8 @@ std::vector AccessLogFormatParser::parse(const std::string& format parseCommand(token, start, main_header, alternative_header, max_length); - formatters.emplace_back( - FormatterPtr(new RequestHeaderFormatter(main_header, alternative_header, max_length))); + formatters.emplace_back(FormatterPtr( + new RequestHeaderFormatter(main_header, alternative_header, max_length, true))); } else if (token.find("RESP(") == 0) { std::string main_header, alternative_header; Optional max_length; @@ -144,8 +144,8 @@ std::vector AccessLogFormatParser::parse(const std::string& format parseCommand(token, start, main_header, alternative_header, max_length); - formatters.emplace_back( - FormatterPtr(new ResponseHeaderFormatter(main_header, alternative_header, max_length))); + formatters.emplace_back(FormatterPtr( + new ResponseHeaderFormatter(main_header, alternative_header, max_length, false))); } else { formatters.emplace_back(FormatterPtr(new RequestInfoFormatter(token))); } @@ -219,8 +219,9 @@ std::string PlainStringFormatter::format(const Http::HeaderMap&, const Http::Hea HeaderFormatter::HeaderFormatter(const std::string& main_header, const std::string& alternative_header, - const Optional& max_length) - : main_header_(main_header), alternative_header_(alternative_header), max_length_(max_length) {} + const Optional& max_length, bool escape_newlines) + : main_header_(main_header), alternative_header_(alternative_header), max_length_(max_length), + escape_newlines_(escape_newlines) {} std::string HeaderFormatter::format(const HeaderMap& headers) const { std::string header_value = headers.get(main_header_); @@ -242,8 +243,8 @@ std::string HeaderFormatter::format(const HeaderMap& headers) const { ResponseHeaderFormatter::ResponseHeaderFormatter(const std::string& main_header, const std::string& alternative_header, - const Optional& max_length) - : HeaderFormatter(main_header, alternative_header, max_length) {} + const Optional& max_length, bool) + : HeaderFormatter(main_header, alternative_header, max_length, false) {} std::string ResponseHeaderFormatter::format(const Http::HeaderMap&, const Http::HeaderMap& response_headers, @@ -253,8 +254,9 @@ std::string ResponseHeaderFormatter::format(const Http::HeaderMap&, RequestHeaderFormatter::RequestHeaderFormatter(const std::string& main_header, const std::string& alternative_header, - const Optional& max_length) - : HeaderFormatter(main_header, alternative_header, max_length) {} + const Optional& max_length, + bool escape_newlines) + : HeaderFormatter(main_header, alternative_header, max_length, escape_newlines) {} std::string RequestHeaderFormatter::format(const Http::HeaderMap& request_headers, const Http::HeaderMap&, const RequestInfo&) const { diff --git a/source/common/http/access_log/access_log_formatter.h b/source/common/http/access_log/access_log_formatter.h index a3b1c92ae890..b9faa84491a1 100644 --- a/source/common/http/access_log/access_log_formatter.h +++ b/source/common/http/access_log/access_log_formatter.h @@ -85,7 +85,7 @@ class PlainStringFormatter : public Formatter { class HeaderFormatter { public: HeaderFormatter(const std::string& main_header, const std::string& alternative_header, - const Optional& max_length); + const Optional& max_length, bool escape_newlines); std::string format(const HeaderMap& headers) const; @@ -93,6 +93,7 @@ class HeaderFormatter { LowerCaseString main_header_; LowerCaseString alternative_header_; Optional max_length_; + bool escape_newlines_; }; /** @@ -101,7 +102,7 @@ class HeaderFormatter { class RequestHeaderFormatter : public Formatter, HeaderFormatter { public: RequestHeaderFormatter(const std::string& main_header, const std::string& alternative_header, - const Optional& max_length); + const Optional& max_length, bool escape_newlines); // Formatter::format std::string format(const HeaderMap& request_headers, const HeaderMap&, @@ -114,7 +115,7 @@ class RequestHeaderFormatter : public Formatter, HeaderFormatter { class ResponseHeaderFormatter : public Formatter, HeaderFormatter { public: ResponseHeaderFormatter(const std::string& main_header, const std::string& alternative_header, - const Optional& max_length); + const Optional& max_length, bool); // Formatter::format std::string format(const HeaderMap&, const HeaderMap& response_headers, diff --git a/test/common/http/access_log/access_log_formatter_test.cc b/test/common/http/access_log/access_log_formatter_test.cc index e19f6d9e7a87..e5371deb83e2 100644 --- a/test/common/http/access_log/access_log_formatter_test.cc +++ b/test/common/http/access_log/access_log_formatter_test.cc @@ -118,27 +118,27 @@ TEST(AccessLogFormatterTest, requestHeaderFormatter) { HeaderMapImpl response_header{{":method", "PUT"}}; { - RequestHeaderFormatter formatter(":Method", "", Optional()); + RequestHeaderFormatter formatter(":Method", "", Optional(), true); EXPECT_EQ("GET", formatter.format(request_header, response_header, requestInfo)); } { - RequestHeaderFormatter formatter(":path", ":method", Optional()); + RequestHeaderFormatter formatter(":path", ":method", Optional(), true); EXPECT_EQ("/", formatter.format(request_header, response_header, requestInfo)); } { - RequestHeaderFormatter formatter(":TEST", ":METHOD", Optional()); + RequestHeaderFormatter formatter(":TEST", ":METHOD", Optional(), true); EXPECT_EQ("GET", formatter.format(request_header, response_header, requestInfo)); } { - RequestHeaderFormatter formatter("does_not_exist", "", Optional()); + RequestHeaderFormatter formatter("does_not_exist", "", Optional(), true); EXPECT_EQ("-", formatter.format(request_header, response_header, requestInfo)); } { - RequestHeaderFormatter formatter("x-test", "", Optional()); + RequestHeaderFormatter formatter("x-test", "", Optional(), true); EXPECT_EQ("hello\\nworld", formatter.format(request_header, response_header, requestInfo)); } } @@ -149,22 +149,22 @@ TEST(AccessLogFormatterTest, responseHeaderFormatter) { HeaderMapImpl response_header{{":method", "PUT"}, {"test", "test"}}; { - ResponseHeaderFormatter formatter(":method", "", Optional()); + ResponseHeaderFormatter formatter(":method", "", Optional(), false); EXPECT_EQ("PUT", formatter.format(request_header, response_header, requestInfo)); } { - ResponseHeaderFormatter formatter("test", ":method", Optional()); + ResponseHeaderFormatter formatter("test", ":method", Optional(), false); EXPECT_EQ("test", formatter.format(request_header, response_header, requestInfo)); } { - ResponseHeaderFormatter formatter(":path", ":method", Optional()); + ResponseHeaderFormatter formatter(":path", ":method", Optional(), false); EXPECT_EQ("PUT", formatter.format(request_header, response_header, requestInfo)); } { - ResponseHeaderFormatter formatter("does_not_exist", "", Optional()); + ResponseHeaderFormatter formatter("does_not_exist", "", Optional(), false); EXPECT_EQ("-", formatter.format(request_header, response_header, requestInfo)); } } From 674061f3236c4433cdf73b935ba06addd73c1f77 Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Mon, 31 Oct 2016 15:51:00 -0700 Subject: [PATCH 3/9] make test pass --- source/common/http/access_log/access_log_formatter.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index acb251194a02..5d04deb828c4 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -238,6 +238,17 @@ std::string HeaderFormatter::format(const HeaderMap& headers) const { return header_value.substr(0, max_length_.value()); } + if (escape_newlines_ == true) { + // TODO: factor out into function? replace multiple characters + size_t pos = 0; + std::string search = "\n"; + std::string replace = "\\n"; + while ((pos = header_value.find(search, pos)) != std::string::npos) { + header_value.replace(pos, search.length(), replace); + pos += replace.length(); + } + } + return header_value; } From 4f5d3dc654ce3bdb215faf208c211d3fce05ba1c Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Mon, 31 Oct 2016 17:59:19 -0700 Subject: [PATCH 4/9] refactor string replacement into function --- source/common/common/utility.cc | 13 +++++++++++++ source/common/common/utility.h | 2 ++ .../common/http/access_log/access_log_formatter.cc | 12 +++--------- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 5137872ee003..1450e0b788fc 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -107,3 +107,16 @@ const std::string& StringUtil::valueOrDefault(const std::string& input, const std::string& default_value) { return input.empty() ? default_value : input; } + +std::string StringUtil::replaceAll(const std::string& input, const std::string& search, const std::string& replacement) { + std::string input_ = input; + + size_t pos = 0; + while ((pos = input_.find(search, pos)) != std::string::npos) { + input_.replace(pos, search.length(), replacement); + pos += replacement.length(); + } + + return input_; + +} diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 280e1b9ec0ef..e6571bda057f 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -98,4 +98,6 @@ class StringUtil { */ static const std::string& valueOrDefault(const std::string& input, const std::string& default_value); + + static std::string replaceAll(const std::string& input, const std::string& search, const std::string& replacement); }; diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index 5d04deb828c4..a6d92d6ba829 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -238,15 +238,9 @@ std::string HeaderFormatter::format(const HeaderMap& headers) const { return header_value.substr(0, max_length_.value()); } - if (escape_newlines_ == true) { - // TODO: factor out into function? replace multiple characters - size_t pos = 0; - std::string search = "\n"; - std::string replace = "\\n"; - while ((pos = header_value.find(search, pos)) != std::string::npos) { - header_value.replace(pos, search.length(), replace); - pos += replace.length(); - } + if (escape_newlines_) { + header_value = StringUtil::replaceAll(header_value, "\r", "\\r"); + header_value = StringUtil::replaceAll(header_value, "\n", "\\n"); } return header_value; From 05e85e2c04b3552b9f780b4072dda88157ee73a2 Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Mon, 31 Oct 2016 18:03:59 -0700 Subject: [PATCH 5/9] fix format --- source/common/common/utility.cc | 8 ++++---- source/common/common/utility.h | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 1450e0b788fc..e2af8becc114 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -108,15 +108,15 @@ const std::string& StringUtil::valueOrDefault(const std::string& input, return input.empty() ? default_value : input; } -std::string StringUtil::replaceAll(const std::string& input, const std::string& search, const std::string& replacement) { +std::string StringUtil::replaceAll(const std::string& input, const std::string& search, + const std::string& replacement) { std::string input_ = input; size_t pos = 0; while ((pos = input_.find(search, pos)) != std::string::npos) { - input_.replace(pos, search.length(), replacement); - pos += replacement.length(); + input_.replace(pos, search.length(), replacement); + pos += replacement.length(); } return input_; - } diff --git a/source/common/common/utility.h b/source/common/common/utility.h index e6571bda057f..39c766ed73b9 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -99,5 +99,6 @@ class StringUtil { static const std::string& valueOrDefault(const std::string& input, const std::string& default_value); - static std::string replaceAll(const std::string& input, const std::string& search, const std::string& replacement); + static std::string replaceAll(const std::string& input, const std::string& search, + const std::string& replacement); }; From d5ebe792b0b483f65eba2f1c7e4c558f6083e8ba Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Tue, 1 Nov 2016 16:48:03 -0700 Subject: [PATCH 6/9] fix variable name to not resemble class field --- source/common/common/utility.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index e2af8becc114..4fb5454aaa71 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -110,13 +110,13 @@ const std::string& StringUtil::valueOrDefault(const std::string& input, std::string StringUtil::replaceAll(const std::string& input, const std::string& search, const std::string& replacement) { - std::string input_ = input; + std::string result = input; size_t pos = 0; - while ((pos = input_.find(search, pos)) != std::string::npos) { - input_.replace(pos, search.length(), replacement); + while ((pos = result.find(search, pos)) != std::string::npos) { + result.replace(pos, search.length(), replacement); pos += replacement.length(); } - return input_; + return result; } From 11dc607636d0f8867e8758302f085d3a20bce117 Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Tue, 1 Nov 2016 16:51:59 -0700 Subject: [PATCH 7/9] docstring for replaceAll --- source/common/common/utility.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 39c766ed73b9..b1828d2c39d3 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -99,6 +99,14 @@ class StringUtil { static const std::string& valueOrDefault(const std::string& input, const std::string& default_value); + /** + * Replace all instances of a substring in an input string with another string. + * + * @param input supplies string to perform replacement on + * @param search supplies substring to look for and replace + * @param replacement supplies string to replace search substring + * @return copy of string with all instances of substring replaced + */ static std::string replaceAll(const std::string& input, const std::string& search, const std::string& replacement); }; From d5c20f1bef92b6521fc631a7e857b01e9043fb43 Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Tue, 1 Nov 2016 17:16:45 -0700 Subject: [PATCH 8/9] remove unused variable --- source/common/http/access_log/access_log_formatter.cc | 4 ++-- source/common/http/access_log/access_log_formatter.h | 2 +- test/common/http/access_log/access_log_formatter_test.cc | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index a6d92d6ba829..29e3b18edf3e 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -145,7 +145,7 @@ std::vector AccessLogFormatParser::parse(const std::string& format parseCommand(token, start, main_header, alternative_header, max_length); formatters.emplace_back(FormatterPtr( - new ResponseHeaderFormatter(main_header, alternative_header, max_length, false))); + new ResponseHeaderFormatter(main_header, alternative_header, max_length))); } else { formatters.emplace_back(FormatterPtr(new RequestInfoFormatter(token))); } @@ -248,7 +248,7 @@ std::string HeaderFormatter::format(const HeaderMap& headers) const { ResponseHeaderFormatter::ResponseHeaderFormatter(const std::string& main_header, const std::string& alternative_header, - const Optional& max_length, bool) + const Optional& max_length) : HeaderFormatter(main_header, alternative_header, max_length, false) {} std::string ResponseHeaderFormatter::format(const Http::HeaderMap&, diff --git a/source/common/http/access_log/access_log_formatter.h b/source/common/http/access_log/access_log_formatter.h index b9faa84491a1..9a214c847670 100644 --- a/source/common/http/access_log/access_log_formatter.h +++ b/source/common/http/access_log/access_log_formatter.h @@ -115,7 +115,7 @@ class RequestHeaderFormatter : public Formatter, HeaderFormatter { class ResponseHeaderFormatter : public Formatter, HeaderFormatter { public: ResponseHeaderFormatter(const std::string& main_header, const std::string& alternative_header, - const Optional& max_length, bool); + const Optional& max_length); // Formatter::format std::string format(const HeaderMap&, const HeaderMap& response_headers, diff --git a/test/common/http/access_log/access_log_formatter_test.cc b/test/common/http/access_log/access_log_formatter_test.cc index e5371deb83e2..2e7ce1a6c0af 100644 --- a/test/common/http/access_log/access_log_formatter_test.cc +++ b/test/common/http/access_log/access_log_formatter_test.cc @@ -149,22 +149,22 @@ TEST(AccessLogFormatterTest, responseHeaderFormatter) { HeaderMapImpl response_header{{":method", "PUT"}, {"test", "test"}}; { - ResponseHeaderFormatter formatter(":method", "", Optional(), false); + ResponseHeaderFormatter formatter(":method", "", Optional()); EXPECT_EQ("PUT", formatter.format(request_header, response_header, requestInfo)); } { - ResponseHeaderFormatter formatter("test", ":method", Optional(), false); + ResponseHeaderFormatter formatter("test", ":method", Optional()); EXPECT_EQ("test", formatter.format(request_header, response_header, requestInfo)); } { - ResponseHeaderFormatter formatter(":path", ":method", Optional(), false); + ResponseHeaderFormatter formatter(":path", ":method", Optional()); EXPECT_EQ("PUT", formatter.format(request_header, response_header, requestInfo)); } { - ResponseHeaderFormatter formatter("does_not_exist", "", Optional(), false); + ResponseHeaderFormatter formatter("does_not_exist", "", Optional()); EXPECT_EQ("-", formatter.format(request_header, response_header, requestInfo)); } } From 5c2a20f9f85d1c49835c87b6cac3e5a2eef4e7cf Mon Sep 17 00:00:00 2001 From: Daniel Hochman Date: Tue, 1 Nov 2016 17:24:28 -0700 Subject: [PATCH 9/9] fix_format --- source/common/http/access_log/access_log_formatter.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/http/access_log/access_log_formatter.cc b/source/common/http/access_log/access_log_formatter.cc index 29e3b18edf3e..0f5f362ad2af 100644 --- a/source/common/http/access_log/access_log_formatter.cc +++ b/source/common/http/access_log/access_log_formatter.cc @@ -144,8 +144,8 @@ std::vector AccessLogFormatParser::parse(const std::string& format parseCommand(token, start, main_header, alternative_header, max_length); - formatters.emplace_back(FormatterPtr( - new ResponseHeaderFormatter(main_header, alternative_header, max_length))); + formatters.emplace_back( + FormatterPtr(new ResponseHeaderFormatter(main_header, alternative_header, max_length))); } else { formatters.emplace_back(FormatterPtr(new RequestInfoFormatter(token))); }