From fc048b91a0d6aee79fec6661780bb7530033cb21 Mon Sep 17 00:00:00 2001 From: chenBright Date: Mon, 31 Aug 2026 22:34:42 +0800 Subject: [PATCH] Close two paths that reach services past their intended gate --- src/brpc/policy/http2_rpc_protocol.cpp | 8 ++ src/brpc/policy/http_rpc_protocol.cpp | 18 ++++ test/brpc_http_message_unittest.cpp | 47 ++++++++++- test/brpc_http_rpc_protocol_unittest.cpp | 48 +++++++++++ test/brpc_server_unittest.cpp | 101 ++++++++++++++++++++--- 5 files changed, 207 insertions(+), 15 deletions(-) diff --git a/src/brpc/policy/http2_rpc_protocol.cpp b/src/brpc/policy/http2_rpc_protocol.cpp index af9ed0b151..e80b601145 100644 --- a/src/brpc/policy/http2_rpc_protocol.cpp +++ b/src/brpc/policy/http2_rpc_protocol.cpp @@ -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 != "*" && + (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 9d63de23e9..d31e9efd02 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 87f3b3736f..951c910876 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 af2e17bca0..efbd955222 100644 --- a/test/brpc_http_rpc_protocol_unittest.cpp +++ b/test/brpc_http_rpc_protocol_unittest.cpp @@ -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; diff --git a/test/brpc_server_unittest.cpp b/test/brpc_server_unittest.cpp index ed0268e853..60575731ba 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";