Skip to content

Commit

Permalink
Handle lists with and without spaces in XFF
Browse files Browse the repository at this point in the history
Currently, `Utility::getLastAddressFromXFF()` only handles lists that
use a comma plus a space as the separator.

Per https://tools.ietf.org/html/rfc7239#section-7.1, spaces are allowed
around commas — so we should handle that too.

This fixes envoyproxy#3607.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
  • Loading branch information
Raul Gutierrez Segales committed Jun 12, 2018
1 parent 7beeea0 commit 25b4ee2
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 1 deletion.
6 changes: 5 additions & 1 deletion source/common/http/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ Utility::getLastAddressFromXFF(const Http::HeaderMap& request_headers, uint32_t
}

absl::string_view xff_string(xff_header->value().c_str(), xff_header->value().size());
static const std::string seperator(", ");
static const std::string seperator(",");
// Ignore the last num_to_skip addresses at the end of XFF.
for (uint32_t i = 0; i < num_to_skip; i++) {
std::string::size_type last_comma = xff_string.rfind(seperator);
Expand All @@ -303,6 +303,10 @@ Utility::getLastAddressFromXFF(const Http::HeaderMap& request_headers, uint32_t
xff_string = xff_string.substr(last_comma + seperator.size());
}

// ignore the whitespace, since they are allowed in HTTP lists (see rfc7239)
xff_string = StringUtil::ltrim(xff_string);
xff_string = StringUtil::rtrim(xff_string);

try {
// This technically requires a copy because inet_pton takes a null terminated string. In
// practice, we are working with a view at the end of the owning string, and could pass the
Expand Down
18 changes: 18 additions & 0 deletions test/common/http/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,24 @@ TEST(HttpUtility, getLastAddressFromXFF) {
EXPECT_EQ(nullptr, ret.address_);
EXPECT_FALSE(ret.single_address_);
}
{
const std::string first_address = "192.0.2.10";
const std::string second_address = "192.0.2.1";
const std::string third_address = "10.0.0.1";
TestHeaderMapImpl request_headers{{"x-forwarded-for", "192.0.2.10,192.0.2.1,10.0.0.1"}};
auto ret = Utility::getLastAddressFromXFF(request_headers);
EXPECT_EQ(third_address, ret.address_->ip()->addressAsString());
EXPECT_FALSE(ret.single_address_);
ret = Utility::getLastAddressFromXFF(request_headers, 1);
EXPECT_EQ(second_address, ret.address_->ip()->addressAsString());
EXPECT_FALSE(ret.single_address_);
ret = Utility::getLastAddressFromXFF(request_headers, 2);
EXPECT_EQ(first_address, ret.address_->ip()->addressAsString());
EXPECT_FALSE(ret.single_address_);
ret = Utility::getLastAddressFromXFF(request_headers, 3);
EXPECT_EQ(nullptr, ret.address_);
EXPECT_FALSE(ret.single_address_);
}
{
TestHeaderMapImpl request_headers{{"x-forwarded-for", ""}};
auto ret = Utility::getLastAddressFromXFF(request_headers);
Expand Down

0 comments on commit 25b4ee2

Please sign in to comment.