Skip to content

Close two paths that reach services past their intended gate #3512

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

Open
chenBright wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Close two paths that reach services past their intended gate #3512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions src/brpc/policy/http2_rpc_protocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,6 +1273,14 @@ int H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
case 'p':
if (strcmp(name + 2, /*p*/"ath") == 0) {
matched = true;
// RFC 9113 8.3.1: :path MUST NOT be empty and, apart from
// the asterisk-form that OPTIONS may use, MUST begin with
// '/'.
if (pair.value != "*" &&

@wasphin wasphin Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any chance here to check the OPTIONS requests? Not a must though.

(pair.value.empty() || pair.value[0] != '/')) {
LOG(ERROR) << "Invalid path=" << pair.value;
return -1;
}
// Including path/query/fragment
h.uri().SetH2Path(pair.value);
}
Expand Down
18 changes: 18 additions & 0 deletions src/brpc/policy/http_rpc_protocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ DEFINE_string(request_id_header, "x-request-id", "The http header to mark a sess
DEFINE_bool(use_http_error_code, false, "Whether set the x-bd-error-code header "
"of http response to brpc error code");

DEFINE_bool(http_allow_empty_path_segments, false,
"Dispatch http paths containing empty segments (consecutive "
"slashes) as if the empty segments were absent. RFC 3986 treats "
"//foo and /foo as distinct paths, so accepting both lets a "
"request slip past a front proxy that only matches the collapsed "
"form. Turn this on to restore the old lenient behavior.");

// Read user address from the header specified by -http_header_of_user_ip
static bool GetUserAddressFromHeaderImpl(const HttpHeader& headers,
butil::EndPoint* user_addr) {
Expand Down Expand Up @@ -1159,6 +1166,17 @@ FindMethodPropertyByURIImpl(const std::string& uri_path, const Server* server,
const Server::MethodProperty*
FindMethodPropertyByURI(const std::string& uri_path, const Server* server,
std::string* unresolved_path) {
// FindMethodPropertyByURIImpl() splits `uri_path` with a StringSplitter
// that skips empty fields, so //foo, /foo// and /foo//bar all resolve like
// their collapsed forms. A front proxy enforcing an ACL on the collapsed
// form does not match the padded ones and lets them through, which is how
// //flags?setvalue= reaches a builtin service that /flags cannot.
// Collapsing the path here would not help: the proxy has already passed
// the padded literal. Only rejecting it removes the differential.
if (!FLAGS_http_allow_empty_path_segments &&
uri_path.find("//") != std::string::npos) {
return nullptr;
}
const Server::MethodProperty* mp =
FindMethodPropertyByURIImpl(uri_path, server, unresolved_path);
if (mp != nullptr) {
Expand Down
47 changes: 45 additions & 2 deletions test/brpc_http_message_unittest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ int main(int argc, char* argv[]) {
}

namespace policy {
DECLARE_bool(http_allow_empty_path_segments);
Server::MethodProperty*
FindMethodPropertyByURI(const std::string& uri_path, const Server* server,
std::string* unknown_method_str);
Expand Down Expand Up @@ -442,8 +443,7 @@ TEST(HttpMessageTest, find_method_property_by_uri) {
ASSERT_EQ("index", mp->method->service()->name());

mp = FindMethodPropertyByURI("//", &server, nullptr);
ASSERT_TRUE(mp);
ASSERT_EQ("index", mp->method->service()->name());
ASSERT_FALSE(mp);

mp = FindMethodPropertyByURI("flags", &server, &unknown_method);
ASSERT_TRUE(mp);
Expand Down Expand Up @@ -484,6 +484,49 @@ TEST(HttpMessageTest, find_method_property_by_uri) {
ASSERT_FALSE(mp);
}

// A path with empty segments is a different path per RFC 3986, but the
// splitter used to resolve it skips them, so //flags used to reach the same
// builtin service as /flags. That difference is what lets a request slip past
// a front proxy whose ACL only matches the collapsed form, so such paths are
// rejected rather than collapsed.
TEST(HttpMessageTest, reject_empty_path_segments) {
brpc::Server server;
ASSERT_EQ(0, server.AddService(new test::EchoService(),
brpc::SERVER_OWNS_SERVICE));
ASSERT_EQ(0, server.Start("127.0.0.1:0", nullptr));
std::string unknown_method;

const char* const kRejected[] = {
"//",
"//flags",
"///flags",
"/flags//port",
"//EchoService/Echo",
"/EchoService//Echo",
"/EchoService/Echo//",
};
for (const char* path : kRejected) {
ASSERT_FALSE(FindMethodPropertyByURI(path, &server, &unknown_method))
<< "path=" << path;
}

// The collapsed forms keep working.
ASSERT_TRUE(FindMethodPropertyByURI("/", &server, nullptr));
ASSERT_TRUE(FindMethodPropertyByURI("/flags/port", &server,
&unknown_method));
ASSERT_TRUE(FindMethodPropertyByURI("/EchoService/Echo", &server,
&unknown_method));

// -http_allow_empty_path_segments restores the old lenient behavior for
// deployments that depend on it.
brpc::policy::FLAGS_http_allow_empty_path_segments = true;
for (const char* path : kRejected) {
ASSERT_TRUE(FindMethodPropertyByURI(path, &server, &unknown_method))
<< "path=" << path;
}
brpc::policy::FLAGS_http_allow_empty_path_segments = false;
}
Comment thread
chenBright marked this conversation as resolved.

TEST(HttpMessageTest, http_header) {
brpc::HttpHeader header;

Expand Down
48 changes: 48 additions & 0 deletions test/brpc_http_rpc_protocol_unittest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2129,6 +2129,54 @@ TEST_F(HttpTest, http2_handle_goaway_streams) {
}
}

// RFC 9113 8.3.1: :path MUST NOT be empty and, apart from the asterisk-form
// that OPTIONS may use, MUST begin with '/'.
TEST_F(HttpTest, http2_reject_path_not_starting_with_slash) {
brpc::policy::H2Context* h2_ctx =
new brpc::policy::H2Context(_socket.get(), &_server);
ASSERT_EQ(0, h2_ctx->Init());
_socket->initialize_parsing_context(&h2_ctx);

// Encoding and decoding go through the same HPacker here, which is fine:
// it keeps the encoding and decoding tables apart and the header below is
// indexed into neither.
brpc::HPackOptions options;
options.index_policy = brpc::HPACK_NOT_INDEX_HEADER;

struct PathCase {
const char* path;
bool accepted;
};
const PathCase kCases[] = {
{ "/flags", true },
{ "/", true },
{ "/flags?setvalue=1", true },
{ "*", true }, // asterisk-form, used by OPTIONS
{ "flags", false },
{ "", false },
{ "flags/port", false },
{ "*/flags", false },
{ "http://somewhere/flags", false }, // absolute-form
};
int stream_id = 1;
for (const PathCase& c : kCases) {
butil::IOBufAppender appender;
brpc::HPacker::Header header(":path", c.path);
h2_ctx->hpacker().Encode(&appender, header, options);
butil::IOBuf buf;
appender.move_to(buf);
butil::IOBufBytesIterator it(buf);

brpc::policy::H2StreamContext* h2_msg =
new brpc::policy::H2StreamContext(false);
h2_msg->Init(h2_ctx, stream_id);
stream_id += 2;
ASSERT_EQ(c.accepted ? 0 : -1, h2_msg->ConsumeHeaders(it))
<< "path=`" << c.path << '\'';
h2_msg->Destroy();
}
}

TEST_F(HttpTest, spring_protobuf_content_type) {
const int port = 8923;
brpc::Server server;
Expand Down
101 changes: 88 additions & 13 deletions test/brpc_server_unittest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ DECLARE_bool(enable_dir_service);

namespace policy {
DECLARE_bool(use_http_error_code);
DECLARE_bool(http_allow_empty_path_segments);

extern bool SerializeRpcMessage(const google::protobuf::Message& serializer,
Controller& cntl, ContentType content_type,
Expand All @@ -96,6 +97,22 @@ void* RunClosure(void* arg) {
bool g_verify_success = true;
const std::string g_unauthorized_error_text = "unauthorized";

// Paths with empty segments (consecutive slashes) are rejected by default,
// see -http_allow_empty_path_segments. Turns the leniency back on for the
// duration of the scope.
class AllowEmptyPathSegmentsScope {
public:
AllowEmptyPathSegmentsScope()
: _saved(brpc::policy::FLAGS_http_allow_empty_path_segments) {
brpc::policy::FLAGS_http_allow_empty_path_segments = true;
}
~AllowEmptyPathSegmentsScope() {
brpc::policy::FLAGS_http_allow_empty_path_segments = _saved;
}
private:
const bool _saved;
};

class MyAuthenticator : public brpc::Authenticator {
public:
MyAuthenticator() = default;
Expand Down Expand Up @@ -527,8 +544,21 @@ TEST_F(ServerTest, various_forms_of_uri_paths) {
cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
cntl.request_attachment().append("{\"message\":\"foo\"}");
http_channel.CallMethod(nullptr, &cntl, nullptr, nullptr, nullptr);
ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText() << cntl.response_attachment();
ASSERT_EQ(2, service_v1.ncalled.load());
ASSERT_TRUE(cntl.Failed());
ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode());
LOG(INFO) << "Expected error: " << cntl.ErrorText();
ASSERT_EQ(1, service_v1.ncalled.load());

{
AllowEmptyPathSegmentsScope allow_empty_path_segments;
cntl.Reset();
cntl.http_request().uri() = "/EchoService///Echo//";
cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
cntl.request_attachment().append("{\"message\":\"foo\"}");
http_channel.CallMethod(nullptr, &cntl, nullptr, nullptr, nullptr);
ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText() << cntl.response_attachment();
ASSERT_EQ(2, service_v1.ncalled.load());
}

cntl.Reset();
cntl.http_request().uri() = "/EchoService /Echo/";
Expand Down Expand Up @@ -783,15 +813,31 @@ TEST_F(ServerTest, restful_mapping) {
ASSERT_EQ(2, service_v1.ncalled.load());
ASSERT_EQ("{\"message\":\"bar_v1\"}", cntl.response_attachment());

// Adding extra slashes (and heading/trailing spaces) is OK.
// Heading/trailing spaces are OK, extra slashes are not: //v1/echo and
// /v1/echo are different paths per RFC 3986, and dispatching both to the
// same method lets a request slip past a front proxy whose ACL only
// matches the collapsed form.
cntl.Reset();
cntl.http_request().uri() = " //v1///echo//// ";
cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
cntl.request_attachment().append("{\"message\":\"hello\"}");
http_channel.CallMethod(nullptr, &cntl, nullptr, nullptr, nullptr);
ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText();
ASSERT_EQ(3, service_v1.ncalled.load());
ASSERT_EQ("{\"message\":\"hello_v1\"}", cntl.response_attachment());
ASSERT_TRUE(cntl.Failed());
ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode());
LOG(INFO) << "Expected error: " << cntl.ErrorText();
ASSERT_EQ(2, service_v1.ncalled.load());

{
AllowEmptyPathSegmentsScope allow_empty_path_segments;
cntl.Reset();
cntl.http_request().uri() = " //v1///echo//// ";
cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
cntl.request_attachment().append("{\"message\":\"hello\"}");
http_channel.CallMethod(nullptr, &cntl, nullptr, nullptr, nullptr);
ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText();
ASSERT_EQ(3, service_v1.ncalled.load());
ASSERT_EQ("{\"message\":\"hello_v1\"}", cntl.response_attachment());
}

// /v3/echo must be exactly matched.
cntl.Reset();
Expand Down Expand Up @@ -896,24 +942,53 @@ TEST_F(ServerTest, restful_mapping) {
ASSERT_EQ("{\"message\":\"1.flv_v1_Echo4\"}", cntl.response_attachment());
ASSERT_EQ(1, service_v1.ncalled_echo4.load());

// A path with empty segments is rejected by default, even when a restful
// mapping would match the collapsed form: //v6/d.flv and /v6/d.flv are
// different paths per RFC 3986, and dispatching both to the same method
// lets a request slip past a front proxy whose ACL only matches the
// collapsed form.
cntl.Reset();
cntl.http_request().uri() = "//v6//d.flv//";
cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
cntl.request_attachment().append("{\"message\":\"d.flv\"}");
http_channel.CallMethod(nullptr, &cntl, nullptr, nullptr, nullptr);
ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText();
ASSERT_EQ("{\"message\":\"d.flv_v1_Echo5\"}", cntl.response_attachment());
ASSERT_EQ(1, service_v1.ncalled_echo5.load());
ASSERT_TRUE(cntl.Failed());
ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode());
LOG(INFO) << "Expected error: " << cntl.ErrorText();
ASSERT_EQ(0, service_v1.ncalled_echo5.load());

// matched the global restful map.
// Ditto for the global restful map.
cntl.Reset();
cntl.http_request().uri() = "//d.flv//";
cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
cntl.request_attachment().append("{\"message\":\"d.flv\"}");
http_channel.CallMethod(nullptr, &cntl, nullptr, nullptr, nullptr);
ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText();
ASSERT_EQ("{\"message\":\"d.flv_v1\"}", cntl.response_attachment());
ASSERT_EQ(9, service_v1.ncalled.load());
ASSERT_TRUE(cntl.Failed());
ASSERT_EQ(brpc::EHTTP, cntl.ErrorCode());
LOG(INFO) << "Expected error: " << cntl.ErrorText();
ASSERT_EQ(8, service_v1.ncalled.load());

{
AllowEmptyPathSegmentsScope allow_empty_path_segments;
cntl.Reset();
cntl.http_request().uri() = "//v6//d.flv//";
cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
cntl.request_attachment().append("{\"message\":\"d.flv\"}");
http_channel.CallMethod(nullptr, &cntl, nullptr, nullptr, nullptr);
ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText();
ASSERT_EQ("{\"message\":\"d.flv_v1_Echo5\"}", cntl.response_attachment());
ASSERT_EQ(1, service_v1.ncalled_echo5.load());

// matched the global restful map.
cntl.Reset();
cntl.http_request().uri() = "//d.flv//";
cntl.http_request().set_method(brpc::HTTP_METHOD_POST);
cntl.request_attachment().append("{\"message\":\"d.flv\"}");
http_channel.CallMethod(nullptr, &cntl, nullptr, nullptr, nullptr);
ASSERT_FALSE(cntl.Failed()) << cntl.ErrorText();
ASSERT_EQ("{\"message\":\"d.flv_v1\"}", cntl.response_attachment());
ASSERT_EQ(9, service_v1.ncalled.load());
}

cntl.Reset();
cntl.http_request().uri() = "/v7/e.flv";
Expand Down
Loading