Copilot commented on code in PR #3529:
URL: https://github.com/apache/brpc/pull/3529#discussion_r3948646952
##########
test/brpc_builtin_service_unittest.cpp:
##########
@@ -686,6 +697,58 @@ TEST_F(BuiltinServiceTest, flags) {
TestFlags(true);
}
+TEST_F(BuiltinServiceTest, flags_escaping) {
+ // Save all flags and restore them on any exit of this test, since the
+ // /flags service below modifies `reloadable_string_flag_for_ut'.
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FlagsService service;
+ brpc::FlagsRequest req;
+ brpc::FlagsResponse res;
Review Comment:
The same `FlagsRequest req` / `FlagsResponse res` objects are reused across
three `default_method` invocations. If `default_method` ever starts populating
fields in `req`/`res` (or relies on them being default/empty), state could leak
between sub-cases and make the test brittle. Consider scoping `req`/`res`
inside each `{ ... }` block or calling `req.Clear()` / `res.Clear()` before
each call.
##########
test/brpc_builtin_service_unittest.cpp:
##########
@@ -686,6 +697,58 @@ TEST_F(BuiltinServiceTest, flags) {
TestFlags(true);
}
+TEST_F(BuiltinServiceTest, flags_escaping) {
+ // Save all flags and restore them on any exit of this test, since the
+ // /flags service below modifies `reloadable_string_flag_for_ut'.
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FlagsService service;
+ brpc::FlagsRequest req;
+ brpc::FlagsResponse res;
+ const std::string payload = "<svg onload=alert(1)>&\"'";
+ const std::string escaped = brpc::WebEscape(payload);
+
+ // Reflected: the ?setvalue= value is echoed into the html page.
+ {
+ ClosureChecker done;
+ brpc::Controller cntl;
+ SetUpController(&cntl, true);
+ cntl.http_request()._unresolved_path = "reloadable_string_flag_for_ut";
+ cntl.http_request().uri().SetQuery(brpc::SETVALUE_STR, payload);
+ service.default_method(&cntl, &req, &res, &done);
Review Comment:
The same `FlagsRequest req` / `FlagsResponse res` objects are reused across
three `default_method` invocations. If `default_method` ever starts populating
fields in `req`/`res` (or relies on them being default/empty), state could leak
between sub-cases and make the test brittle. Consider scoping `req`/`res`
inside each `{ ... }` block or calling `req.Clear()` / `res.Clear()` before
each call.
##########
test/brpc_builtin_service_unittest.cpp:
##########
@@ -686,6 +697,58 @@ TEST_F(BuiltinServiceTest, flags) {
TestFlags(true);
}
+TEST_F(BuiltinServiceTest, flags_escaping) {
+ // Save all flags and restore them on any exit of this test, since the
+ // /flags service below modifies `reloadable_string_flag_for_ut'.
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FlagsService service;
+ brpc::FlagsRequest req;
+ brpc::FlagsResponse res;
+ const std::string payload = "<svg onload=alert(1)>&\"'";
+ const std::string escaped = brpc::WebEscape(payload);
+
+ // Reflected: the ?setvalue= value is echoed into the html page.
+ {
+ ClosureChecker done;
+ brpc::Controller cntl;
+ SetUpController(&cntl, true);
+ cntl.http_request()._unresolved_path = "reloadable_string_flag_for_ut";
+ cntl.http_request().uri().SetQuery(brpc::SETVALUE_STR, payload);
+ service.default_method(&cntl, &req, &res, &done);
+ EXPECT_FALSE(cntl.Failed());
+ const std::string& body = cntl.response_attachment().to_string();
+ EXPECT_EQ(std::string::npos, body.find(payload))
+ << "unescaped payload in html: " << body;
+ CheckContent(cntl, escaped.c_str());
+ }
+ // Stored: ?setvalue&withform renders the flag value stored above.
+ {
+ ClosureChecker done;
+ brpc::Controller cntl;
+ SetUpController(&cntl, true);
+ cntl.http_request()._unresolved_path = "reloadable_string_flag_for_ut";
+ cntl.http_request().uri().SetQuery(brpc::SETVALUE_STR, "");
+ cntl.http_request().uri().SetQuery("withform", "");
+ service.default_method(&cntl, &req, &res, &done);
Review Comment:
The same `FlagsRequest req` / `FlagsResponse res` objects are reused across
three `default_method` invocations. If `default_method` ever starts populating
fields in `req`/`res` (or relies on them being default/empty), state could leak
between sub-cases and make the test brittle. Consider scoping `req`/`res`
inside each `{ ... }` block or calling `req.Clear()` / `res.Clear()` before
each call.
##########
test/brpc_builtin_service_unittest.cpp:
##########
@@ -686,6 +697,58 @@ TEST_F(BuiltinServiceTest, flags) {
TestFlags(true);
}
+TEST_F(BuiltinServiceTest, flags_escaping) {
+ // Save all flags and restore them on any exit of this test, since the
+ // /flags service below modifies `reloadable_string_flag_for_ut'.
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FlagsService service;
+ brpc::FlagsRequest req;
+ brpc::FlagsResponse res;
+ const std::string payload = "<svg onload=alert(1)>&\"'";
+ const std::string escaped = brpc::WebEscape(payload);
+
+ // Reflected: the ?setvalue= value is echoed into the html page.
+ {
+ ClosureChecker done;
+ brpc::Controller cntl;
+ SetUpController(&cntl, true);
+ cntl.http_request()._unresolved_path = "reloadable_string_flag_for_ut";
+ cntl.http_request().uri().SetQuery(brpc::SETVALUE_STR, payload);
+ service.default_method(&cntl, &req, &res, &done);
+ EXPECT_FALSE(cntl.Failed());
+ const std::string& body = cntl.response_attachment().to_string();
+ EXPECT_EQ(std::string::npos, body.find(payload))
+ << "unescaped payload in html: " << body;
+ CheckContent(cntl, escaped.c_str());
+ }
+ // Stored: ?setvalue&withform renders the flag value stored above.
+ {
+ ClosureChecker done;
+ brpc::Controller cntl;
+ SetUpController(&cntl, true);
+ cntl.http_request()._unresolved_path = "reloadable_string_flag_for_ut";
+ cntl.http_request().uri().SetQuery(brpc::SETVALUE_STR, "");
+ cntl.http_request().uri().SetQuery("withform", "");
+ service.default_method(&cntl, &req, &res, &done);
+ EXPECT_FALSE(cntl.Failed());
+ const std::string& body = cntl.response_attachment().to_string();
+ EXPECT_EQ(std::string::npos, body.find(payload))
+ << "unescaped payload in html: " << body;
+ CheckContent(cntl, escaped.c_str());
+ }
+ // Plain text output is not html-escaped.
+ {
+ ClosureChecker done;
+ brpc::Controller cntl;
+ SetUpController(&cntl, false);
+ cntl.http_request()._unresolved_path = "reloadable_string_flag_for_ut";
+ cntl.http_request().uri().SetQuery(brpc::SETVALUE_STR, payload);
+ service.default_method(&cntl, &req, &res, &done);
Review Comment:
The same `FlagsRequest req` / `FlagsResponse res` objects are reused across
three `default_method` invocations. If `default_method` ever starts populating
fields in `req`/`res` (or relies on them being default/empty), state could leak
between sub-cases and make the test brittle. Consider scoping `req`/`res`
inside each `{ ... }` block or calling `req.Clear()` / `res.Clear()` before
each call.
--
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]
