This is an automated email from the ASF dual-hosted git repository.
chenBright pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 5b61020a Close two paths that reach services past their intended gate
(#3512)
5b61020a is described below
commit 5b61020a9533887bde2d8cc03f9306b6b7104906
Author: Bright Chen <[email protected]>
AuthorDate: Wed Sep 2 22:23:42 2026 +0800
Close two paths that reach services past their intended gate (#3512)
---
src/brpc/policy/http2_rpc_protocol.cpp | 10 +++
src/brpc/policy/http_rpc_protocol.cpp | 18 ++++++
test/brpc_http_message_unittest.cpp | 47 +++++++++++++-
test/brpc_http_rpc_protocol_unittest.cpp | 50 +++++++++++++++
test/brpc_server_unittest.cpp | 101 +++++++++++++++++++++++++++----
5 files changed, 211 insertions(+), 15 deletions(-)
diff --git a/src/brpc/policy/http2_rpc_protocol.cpp
b/src/brpc/policy/http2_rpc_protocol.cpp
index 2f4b5990..b20a0572 100644
--- a/src/brpc/policy/http2_rpc_protocol.cpp
+++ b/src/brpc/policy/http2_rpc_protocol.cpp
@@ -1373,6 +1373,16 @@ 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 MUST begin
+ // with '/', the only exception being the asterisk-form
+ // that OPTIONS uses. '*' is accepted for any method here,
+ // as http_parser does for HTTP/1: pinning it to OPTIONS
+ // would take the whole header block, since HPACK does not
+ // order pseudo-headers and :method may not have arrived.
+ if (pair.value != "*" && (pair.value.empty() ||
pair.value[0] != '/')) {
+ LOG(ERROR) << "Invalid path=" << pair.value;
+ return -1;
+ }
// Including path/query/fragment
h.uri().SetH2Path(pair.value);
}
diff --git a/src/brpc/policy/http_rpc_protocol.cpp
b/src/brpc/policy/http_rpc_protocol.cpp
index f09f2c83..4d1c8824 100644
--- a/src/brpc/policy/http_rpc_protocol.cpp
+++ b/src/brpc/policy/http_rpc_protocol.cpp
@@ -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) {
@@ -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) {
diff --git a/test/brpc_http_message_unittest.cpp
b/test/brpc_http_message_unittest.cpp
index 87f3b373..951c9108 100644
--- a/test/brpc_http_message_unittest.cpp
+++ b/test/brpc_http_message_unittest.cpp
@@ -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);
@@ -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);
@@ -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;
+}
+
TEST(HttpMessageTest, http_header) {
brpc::HttpHeader header;
diff --git a/test/brpc_http_rpc_protocol_unittest.cpp
b/test/brpc_http_rpc_protocol_unittest.cpp
index 493418e6..e0c7b741 100644
--- a/test/brpc_http_rpc_protocol_unittest.cpp
+++ b/test/brpc_http_rpc_protocol_unittest.cpp
@@ -2356,6 +2356,56 @@ TEST_F(HttpTest, http2_handle_goaway_streams) {
}
}
+// RFC 9113 8.3.1: :path MUST NOT be empty and MUST begin with '/', the only
+// exception being the asterisk-form that OPTIONS uses.
+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 },
+ // Asterisk-form. Only OPTIONS may use it, but it is taken from any
+ // method here, see the comment on the check.
+ { "*", true },
+ { "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;
diff --git a/test/brpc_server_unittest.cpp b/test/brpc_server_unittest.cpp
index 8e386b2a..4e64a851 100644
--- a/test/brpc_server_unittest.cpp
+++ b/test/brpc_server_unittest.cpp
@@ -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,
@@ -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;
@@ -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/";
@@ -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();
@@ -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";
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
