diff --git a/src/brpc/builtin/flags_service.cpp b/src/brpc/builtin/flags_service.cpp index 18152baa86..569da31394 100644 --- a/src/brpc/builtin/flags_service.cpp +++ b/src/brpc/builtin/flags_service.cpp @@ -117,11 +117,11 @@ void FlagsService::set_value_page(Controller* cntl, const bool is_string = (info.type == "string"); os << "" "
" - " Set `" << name << "' from "; + " Set `" << WebEscape(name) << "' from "; if (is_string) { os << '"'; } - os << info.current_value; + os << WebEscape(info.current_value); if (is_string) { os << '"'; } @@ -177,9 +177,12 @@ void FlagsService::default_method(::google::protobuf::RpcController* cntl_base, return; } butil::IOBufBuilder os; - os << "Set `" << constraint << "' to " << *value_str; if (use_html) { + os << "Set `" << WebEscape(constraint) << "' to " + << WebEscape(*value_str); os << "
[back to flags]"; + } else { + os << "Set `" << constraint << "' to " << *value_str; } os.move_to(cntl->response_attachment()); return; diff --git a/test/brpc_builtin_service_unittest.cpp b/test/brpc_builtin_service_unittest.cpp index e496c05f57..f13c195525 100644 --- a/test/brpc_builtin_service_unittest.cpp +++ b/test/brpc_builtin_service_unittest.cpp @@ -60,6 +60,17 @@ DEFINE_bool(foo, false, "Flags for UT"); BRPC_VALIDATE_GFLAG(foo, brpc::PassValidate); +// A reloadable string gflag so that FlagsService is able to modify its +// value via ?setvalue=. String flags must register the validator manually, +// see comments in butil/reloadable_flags.h. +DEFINE_string(reloadable_string_flag_for_ut, "", "Flags for UT"); +static bool PassValidateStringFlag(const char*, const std::string&) { + return true; +} +const bool ALLOW_UNUSED dummy_validate_reloadable_string_flag_for_ut = + GFLAGS_NAMESPACE::RegisterFlagValidator( + &FLAGS_reloadable_string_flag_for_ut, PassValidateStringFlag); + namespace brpc { DECLARE_bool(enable_rpcz); DECLARE_bool(rpcz_hex_log_id); @@ -686,6 +697,62 @@ TEST_F(BuiltinServiceTest, flags) { TestFlags(true); } +TEST_F(BuiltinServiceTest, flags_escaping) { + brpc::FlagsService service; + brpc::FlagsRequest req; + brpc::FlagsResponse res; + const std::string payload = "&\"'"; + const std::string escaped = brpc::WebEscape(payload); + std::string saved_value; + ASSERT_TRUE(GFLAGS_NAMESPACE::GetCommandLineOption( + "reloadable_string_flag_for_ut", &saved_value)); + + // 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); + EXPECT_FALSE(cntl.Failed()); + CheckContent(cntl, payload.c_str()); + } + + // Restore the flag value not to affect other tests. + GFLAGS_NAMESPACE::SetCommandLineOption( + "reloadable_string_flag_for_ut", saved_value.c_str()); +} + TEST_F(BuiltinServiceTest, bad_method) { TestBadMethod(false); TestBadMethod(true);