Copilot commented on code in PR #3525:
URL: https://github.com/apache/brpc/pull/3525#discussion_r3950806654
##########
src/brpc/server.cpp:
##########
@@ -2371,6 +2371,42 @@ bool Server::AcceptRequest(Controller* cntl) const {
return true;
}
+bool Server::RejectBuiltinAccess(Controller* cntl,
+ const MethodProperty* mp) const {
+ if (!cntl->is_security_mode() ||
+ (!mp->is_builtin_service && !mp->params.is_tabbed)) {
+ return false;
+ }
+ cntl->SetFailed(EPERM, "Not allowed to access builtin services, try "
+ "ServerOptions.internal_port=%d instead if you're
in internal network",
+ _options.internal_port);
+ return true;
+}
Review Comment:
The rejection condition includes both builtin **and Tabbed** services
(`mp->params.is_tabbed`), but the message says “builtin services” only. To
avoid misleading users, update the text to mention both builtin and Tabbed
services (or adjust the condition if Tabbed access should be treated
differently).
##########
test/brpc_server_unittest.cpp:
##########
@@ -1719,6 +1721,158 @@ TEST_F(ServerTest,
builtin_services_are_gated_by_internal_port) {
ASSERT_EQ(0, server.Join());
}
+// Call the same ordinary service the way a browser would.
+void CallEchoByHttp(const butil::EndPoint& ep, brpc::Controller* cntl) {
Review Comment:
These helper functions have external linkage in a test translation unit. To
avoid potential symbol collisions at link time as the test suite grows
(especially with generic names like `PickUnusedPort`/`StartWithInternalPort`),
make them `static` or place them in an unnamed namespace.
##########
test/brpc_server_unittest.cpp:
##########
@@ -1719,6 +1721,158 @@ TEST_F(ServerTest,
builtin_services_are_gated_by_internal_port) {
ASSERT_EQ(0, server.Join());
}
+// Call the same ordinary service the way a browser would.
+void CallEchoByHttp(const butil::EndPoint& ep, brpc::Controller* cntl) {
+ brpc::ChannelOptions copt;
+ copt.protocol = brpc::PROTOCOL_HTTP;
+ copt.max_retry = 0;
+ brpc::Channel chan;
+ ASSERT_EQ(0, chan.Init(ep, &copt));
+ test::EchoRequest req;
+ test::EchoResponse res;
+ req.set_message(EXP_REQUEST);
+ cntl->http_request().uri() = "/EchoService/Echo";
+ cntl->http_request().set_method(brpc::HTTP_METHOD_POST);
+ cntl->http_request().set_content_type("application/json");
+ chan.CallMethod(nullptr, cntl, &req, &res, nullptr);
+}
+
+// Returns a port nothing is listening on, or -1. `ServerOptions.internal_port`
+// has to be an explicit number, Server::Start() rejects 0 because it stands
+// for an ephemeral port, so ask the system for a free one rather than hardcode
+// a port that another test may be listening on.
+int PickUnusedPort() {
Review Comment:
These helper functions have external linkage in a test translation unit. To
avoid potential symbol collisions at link time as the test suite grows
(especially with generic names like `PickUnusedPort`/`StartWithInternalPort`),
make them `static` or place them in an unnamed namespace.
##########
test/brpc_redis_unittest.cpp:
##########
@@ -1223,6 +1225,102 @@ TEST_F(RedisTest, server_sanity) {
ASSERT_EQ("", response.reply(3).data());
}
+// Returns a port nothing is listening on, or -1. ServerOptions.internal_port
+// has to be an explicit number, Server::Start() rejects 0 because it stands
+// for an ephemeral port, so ask the system for a free one rather than hardcode
+// a port that another test may be listening on.
+static int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY,
0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+// Starts `server' on an ephemeral port and fills options->internal_port with
+// another one. Both are released before Start() binds them and something else
+// may take one in between, hence the retries. Returns 0 on success.
+static int StartWithInternalPort(brpc::Server* server,
+ brpc::ServerOptions* options) {
Review Comment:
`PickUnusedPort()` / `StartWithInternalPort()` duplicate the same logic
added in `test/brpc_server_unittest.cpp`. Consider centralizing these helpers
in a shared test utility (or a common header) to prevent future divergence and
reduce copy/paste maintenance.
##########
test/brpc_server_unittest.cpp:
##########
@@ -1719,6 +1721,158 @@ TEST_F(ServerTest,
builtin_services_are_gated_by_internal_port) {
ASSERT_EQ(0, server.Join());
}
+// Call the same ordinary service the way a browser would.
+void CallEchoByHttp(const butil::EndPoint& ep, brpc::Controller* cntl) {
+ brpc::ChannelOptions copt;
+ copt.protocol = brpc::PROTOCOL_HTTP;
+ copt.max_retry = 0;
+ brpc::Channel chan;
+ ASSERT_EQ(0, chan.Init(ep, &copt));
+ test::EchoRequest req;
+ test::EchoResponse res;
+ req.set_message(EXP_REQUEST);
+ cntl->http_request().uri() = "/EchoService/Echo";
+ cntl->http_request().set_method(brpc::HTTP_METHOD_POST);
+ cntl->http_request().set_content_type("application/json");
+ chan.CallMethod(nullptr, cntl, &req, &res, nullptr);
+}
+
+// Returns a port nothing is listening on, or -1. `ServerOptions.internal_port`
+// has to be an explicit number, Server::Start() rejects 0 because it stands
+// for an ephemeral port, so ask the system for a free one rather than hardcode
+// a port that another test may be listening on.
+int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY,
0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+// Starts `server` on an ephemeral port and fills `options->internal_port` with
+// another one. Both are released before Start() binds them and something else
+// may take one in between, hence the retries. Returns 0 on success.
+int StartWithInternalPort(brpc::Server* server, brpc::ServerOptions* options) {
Review Comment:
These helper functions have external linkage in a test translation unit. To
avoid potential symbol collisions at link time as the test suite grows
(especially with generic names like `PickUnusedPort`/`StartWithInternalPort`),
make them `static` or place them in an unnamed namespace.
##########
test/brpc_redis_unittest.cpp:
##########
@@ -1223,6 +1225,102 @@ TEST_F(RedisTest, server_sanity) {
ASSERT_EQ("", response.reply(3).data());
}
+// Returns a port nothing is listening on, or -1. ServerOptions.internal_port
+// has to be an explicit number, Server::Start() rejects 0 because it stands
+// for an ephemeral port, so ask the system for a free one rather than hardcode
+// a port that another test may be listening on.
+static int PickUnusedPort() {
Review Comment:
`PickUnusedPort()` / `StartWithInternalPort()` duplicate the same logic
added in `test/brpc_server_unittest.cpp`. Consider centralizing these helpers
in a shared test utility (or a common header) to prevent future divergence and
reduce copy/paste maintenance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
