Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

escape newlines in request headers #184

Closed
wants to merge 10 commits into from
Closed

escape newlines in request headers #184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b6afa7c
test for escape
Oct 14, 2016
4e7525f
plumbing for boolean escape flag
Oct 31, 2016
674061f
make test pass
Oct 31, 2016
4f5d3dc
refactor string replacement into function
Nov 1, 2016
05e85e2
fix format
Nov 1, 2016
e047734
Merge github.com:lyft/envoy into escape-req-headers
Nov 1, 2016
d5ebe79
fix variable name to not resemble class field
Nov 1, 2016
11dc607
docstring for replaceAll
Nov 1, 2016
d5c20f1
remove unused variable
Nov 2, 2016
5c2a20f
fix_format
Nov 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions source/common/common/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 result = input;

size_t pos = 0;
while ((pos = result.find(search, pos)) != std::string::npos) {
result.replace(pos, search.length(), replacement);
pos += replacement.length();
}

return result;
}
11 changes: 11 additions & 0 deletions source/common/common/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,15 @@ 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment on method

const std::string& replacement);
};
21 changes: 14 additions & 7 deletions source/common/http/access_log/access_log_formatter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ std::vector<FormatterPtr> 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<size_t> max_length;
Expand Down Expand Up @@ -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<size_t>& max_length)
: main_header_(main_header), alternative_header_(alternative_header), max_length_(max_length) {}
const Optional<size_t>& 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_);
Expand All @@ -237,13 +238,18 @@ std::string HeaderFormatter::format(const HeaderMap& headers) const {
return header_value.substr(0, max_length_.value());
}

if (escape_newlines_) {
header_value = StringUtil::replaceAll(header_value, "\r", "\\r");
header_value = StringUtil::replaceAll(header_value, "\n", "\\n");
}

return header_value;
}

ResponseHeaderFormatter::ResponseHeaderFormatter(const std::string& main_header,
const std::string& alternative_header,
const Optional<size_t>& max_length)
: HeaderFormatter(main_header, alternative_header, max_length) {}
: HeaderFormatter(main_header, alternative_header, max_length, false) {}

std::string ResponseHeaderFormatter::format(const Http::HeaderMap&,
const Http::HeaderMap& response_headers,
Expand All @@ -253,8 +259,9 @@ std::string ResponseHeaderFormatter::format(const Http::HeaderMap&,

RequestHeaderFormatter::RequestHeaderFormatter(const std::string& main_header,
const std::string& alternative_header,
const Optional<size_t>& max_length)
: HeaderFormatter(main_header, alternative_header, max_length) {}
const Optional<size_t>& 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 {
Expand Down
5 changes: 3 additions & 2 deletions source/common/http/access_log/access_log_formatter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,15 @@ class PlainStringFormatter : public Formatter {
class HeaderFormatter {
public:
HeaderFormatter(const std::string& main_header, const std::string& alternative_header,
const Optional<size_t>& max_length);
const Optional<size_t>& max_length, bool escape_newlines);

std::string format(const HeaderMap& headers) const;

private:
LowerCaseString main_header_;
LowerCaseString alternative_header_;
Optional<size_t> max_length_;
bool escape_newlines_;
};

/**
Expand All @@ -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<size_t>& max_length);
const Optional<size_t>& max_length, bool escape_newlines);

// Formatter::format
std::string format(const HeaderMap& request_headers, const HeaderMap&,
Expand Down
15 changes: 10 additions & 5 deletions test/common/http/access_log/access_log_formatter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,28 +114,33 @@ 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"}};

{
RequestHeaderFormatter formatter(":Method", "", Optional<size_t>());
RequestHeaderFormatter formatter(":Method", "", Optional<size_t>(), true);
EXPECT_EQ("GET", formatter.format(request_header, response_header, requestInfo));
}

{
RequestHeaderFormatter formatter(":path", ":method", Optional<size_t>());
RequestHeaderFormatter formatter(":path", ":method", Optional<size_t>(), true);
EXPECT_EQ("/", formatter.format(request_header, response_header, requestInfo));
}

{
RequestHeaderFormatter formatter(":TEST", ":METHOD", Optional<size_t>());
RequestHeaderFormatter formatter(":TEST", ":METHOD", Optional<size_t>(), true);
EXPECT_EQ("GET", formatter.format(request_header, response_header, requestInfo));
}

{
RequestHeaderFormatter formatter("does_not_exist", "", Optional<size_t>());
RequestHeaderFormatter formatter("does_not_exist", "", Optional<size_t>(), true);
EXPECT_EQ("-", formatter.format(request_header, response_header, requestInfo));
}

{
RequestHeaderFormatter formatter("x-test", "", Optional<size_t>(), true);
EXPECT_EQ("hello\\nworld", formatter.format(request_header, response_header, requestInfo));
}
}

TEST(AccessLogFormatterTest, responseHeaderFormatter) {
Expand Down