From f41d533cf7003cd38a26fd642beed327b1ef6ffe Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Tue, 25 Aug 2026 13:19:57 +0100 Subject: [PATCH] fix: Internal error on warning injection (#3177) --- src/web/LoadWarning.hpp | 54 +++++++++++++ src/web/impl/HttpBase.hpp | 24 +++--- src/web/impl/WsBase.hpp | 26 +++--- src/web/ng/RPCServerHandler.hpp | 25 ++---- tests/unit/CMakeLists.txt | 1 + tests/unit/web/LoadWarningTests.cpp | 87 +++++++++++++++++++++ tests/unit/web/ng/RPCServerHandlerTests.cpp | 46 +++++++++++ 7 files changed, 214 insertions(+), 49 deletions(-) create mode 100644 src/web/LoadWarning.hpp create mode 100644 tests/unit/web/LoadWarningTests.cpp diff --git a/src/web/LoadWarning.hpp b/src/web/LoadWarning.hpp new file mode 100644 index 000000000..189a08244 --- /dev/null +++ b/src/web/LoadWarning.hpp @@ -0,0 +1,54 @@ +#pragma once + +#include "rpc/Errors.hpp" + +#include +#include +#include +#include + +#include +#include + +namespace web { + +/** + * @brief Parse a serialized response body and attach the DOSGuard "load" warning to it. + * + * Sets `warning` to "load" and appends a rpc::WarningCode::WarnRpcRateLimit entry to the `warnings` + * array, creating that array if the body has none - or replacing it if `warnings` is present but is + * not an array. + * + * @note Not every response body is JSON. Over plain HTTP the error paths return text/html bodies + * (e.g. "Null method" or "Unable to parse JSON from the request"), which cannot carry the warning. + * For those this returns std::nullopt and the caller must send the body unchanged - parsing them as + * JSON would throw. + * + * @param message The serialized response body + * @return The response as a JSON object with the warning attached; callers that need a string body + * must serialize it again. std::nullopt if @p message does not parse as a JSON object. + */ +inline std::optional +withLoadWarning(std::string_view message) +{ + boost::system::error_code ec; + auto const parsed = boost::json::parse(message, ec); + if (ec.failed() or not parsed.is_object()) + return std::nullopt; + + auto jsonResponse = parsed.as_object(); + jsonResponse["warning"] = "load"; + + if (jsonResponse.contains("warnings") and jsonResponse["warnings"].is_array()) { + jsonResponse["warnings"].as_array().push_back( + rpc::makeWarning(rpc::WarningCode::WarnRpcRateLimit) + ); + } else { + jsonResponse["warnings"] = + boost::json::array{rpc::makeWarning(rpc::WarningCode::WarnRpcRateLimit)}; + } + + return jsonResponse; +} + +} // namespace web diff --git a/src/web/impl/HttpBase.hpp b/src/web/impl/HttpBase.hpp index 0ed4d3f00..a21af1738 100644 --- a/src/web/impl/HttpBase.hpp +++ b/src/web/impl/HttpBase.hpp @@ -8,6 +8,7 @@ #include "util/log/Logger.hpp" #include "util/prometheus/Http.hpp" #include "web/AdminVerificationStrategy.hpp" +#include "web/LoadWarning.hpp" #include "web/ProxyIpResolver.hpp" #include "web/SubscriptionContextInterface.hpp" #include "web/dosguard/DOSGuardInterface.hpp" @@ -309,27 +310,20 @@ public: } /** - * @brief Send a response to the client - * The message length will be added to the DOSGuard, if the limit is reached, a warning will be - * added to the response + * @copydoc ConnectionBase::send + * + * @note The message length is added to the DOSGuard. If that puts the client over its limit + * and the body parses as a JSON object, a "load" warning is attached to it; bodies that are + * not JSON objects - the text/html error paths - are sent unchanged. */ void send(std::string&& msg, http::status status = http::status::ok) override { if (!dosGuard_.get().add(clientIp_, msg.size())) { - auto jsonResponse = boost::json::parse(msg).as_object(); - jsonResponse["warning"] = "load"; - if (jsonResponse.contains("warnings") && jsonResponse["warnings"].is_array()) { - jsonResponse["warnings"].as_array().push_back( - rpc::makeWarning(rpc::WarningCode::WarnRpcRateLimit) - ); - } else { - jsonResponse["warnings"] = - boost::json::array{rpc::makeWarning(rpc::WarningCode::WarnRpcRateLimit)}; + if (auto const warned = withLoadWarning(msg); warned.has_value()) { + // Reserialize when we need to include this warning + msg = boost::json::serialize(*warned); } - - // Reserialize when we need to include this warning - msg = boost::json::serialize(jsonResponse); } sender_(httpResponse(status, "application/json", std::move(msg))); } diff --git a/src/web/impl/WsBase.hpp b/src/web/impl/WsBase.hpp index 0e61c199c..4022b2a9e 100644 --- a/src/web/impl/WsBase.hpp +++ b/src/web/impl/WsBase.hpp @@ -4,6 +4,7 @@ #include "rpc/common/Types.hpp" #include "util/Taggable.hpp" #include "util/log/Logger.hpp" +#include "web/LoadWarning.hpp" #include "web/SubscriptionContext.hpp" #include "web/SubscriptionContextInterface.hpp" #include "web/dosguard/DOSGuardInterface.hpp" @@ -196,29 +197,20 @@ public: } /** - * @brief Send a message to the client - * @param msg The message to send - * Send this message to the client. The message length will be added to the DOSGuard - * If the DOSGuard is triggered, the message will be modified to include a warning + * @copydoc ConnectionBase::send + * + * @note The message length is added to the DOSGuard. If that puts the client over its limit + * and the message parses as a JSON object, a "load" warning is attached to it; messages that + * are not JSON objects are sent unchanged. */ void send(std::string&& msg, http::status) override { if (!dosGuard_.get().add(clientIp_, msg.size())) { - auto jsonResponse = boost::json::parse(msg).as_object(); - jsonResponse["warning"] = "load"; - - if (jsonResponse.contains("warnings") && jsonResponse["warnings"].is_array()) { - jsonResponse["warnings"].as_array().push_back( - rpc::makeWarning(rpc::WarningCode::WarnRpcRateLimit) - ); - } else { - jsonResponse["warnings"] = - boost::json::array{rpc::makeWarning(rpc::WarningCode::WarnRpcRateLimit)}; + if (auto const warned = withLoadWarning(msg); warned.has_value()) { + // Reserialize when we need to include this warning + msg = boost::json::serialize(*warned); } - - // Reserialize when we need to include this warning - msg = boost::json::serialize(jsonResponse); } auto sharedMsg = std::make_shared(std::move(msg)); send(std::move(sharedMsg)); diff --git a/src/web/ng/RPCServerHandler.hpp b/src/web/ng/RPCServerHandler.hpp index 64db667e5..ac50f46ef 100644 --- a/src/web/ng/RPCServerHandler.hpp +++ b/src/web/ng/RPCServerHandler.hpp @@ -13,6 +13,7 @@ #include "util/Profiler.hpp" #include "util/Taggable.hpp" #include "util/log/Logger.hpp" +#include "web/LoadWarning.hpp" #include "web/SubscriptionContextInterface.hpp" #include "web/dosguard/DOSGuardInterface.hpp" #include "web/ng/Connection.hpp" @@ -182,7 +183,13 @@ public: // NOLINTBEGIN(bugprone-unchecked-optional-access) if (not dosguard_.get().add(connectionMetadata.ip(), response->message().size())) { - response->setMessage(makeLoadWarning(*response)); + if (auto const warned = withLoadWarning(response->message()); warned.has_value()) { + response->setMessage(*warned); + } else { + LOG(log_.debug()) << connectionMetadata.tag() + << "Rate limit reached but the response body is not a JSON " + "object; sending it without a load warning"; + } } return *std::move(response); @@ -357,22 +364,6 @@ private: return web::ng::Response{boost::beast::http::status::service_unavailable, error, request}; } - static boost::json::object - makeLoadWarning(Response const& response) - { - auto jsonResponse = boost::json::parse(response.message()).as_object(); - jsonResponse["warning"] = "load"; - if (jsonResponse.contains("warnings") && jsonResponse["warnings"].is_array()) { - jsonResponse["warnings"].as_array().push_back( - rpc::makeWarning(rpc::WarningCode::WarnRpcRateLimit) - ); - } else { - jsonResponse["warnings"] = - boost::json::array{rpc::makeWarning(rpc::WarningCode::WarnRpcRateLimit)}; - } - return jsonResponse; - } - [[nodiscard]] bool shouldReplaceParams(boost::json::object const& req) const { diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index b95d4aadf..c87a454ee 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -206,6 +206,7 @@ target_sources( web/dosguard/WeightsTests.cpp web/dosguard/WhitelistHandlerTests.cpp web/impl/ErrorHandlingTests.cpp + web/LoadWarningTests.cpp web/ng/ResponseTests.cpp web/ng/RequestTests.cpp web/ng/RPCServerHandlerTests.cpp diff --git a/tests/unit/web/LoadWarningTests.cpp b/tests/unit/web/LoadWarningTests.cpp new file mode 100644 index 000000000..c4adaf34a --- /dev/null +++ b/tests/unit/web/LoadWarningTests.cpp @@ -0,0 +1,87 @@ +#include "rpc/Errors.hpp" +#include "web/LoadWarning.hpp" + +#include + +#include +#include + +using namespace web; + +namespace { + +struct LoadWarningNonJsonTest : testing::TestWithParam {}; + +} // namespace + +INSTANTIATE_TEST_SUITE_P( + NonJsonBodies, + LoadWarningNonJsonTest, + testing::Values( + "Unable to parse JSON from the request", + "Null method", + "method is empty", + "method is not string", + "params unparsable", + "Bad target", + "Too many requests for one connection", + "", + "[1, 2, 3]", // valid JSON, but not an object + "42", + "null", + R"("a string")" + ) +); + +TEST_P(LoadWarningNonJsonTest, ReturnsNulloptInsteadOfThrowing) +{ + EXPECT_NO_THROW({ EXPECT_FALSE(withLoadWarning(GetParam()).has_value()); }); +} + +TEST(LoadWarningTest, AddsWarningToJsonObjectWithoutExistingWarnings) +{ + auto const result = withLoadWarning(R"({"result": {"status": "success"}})"); + + ASSERT_TRUE(result.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) + EXPECT_EQ(result->at("warning").as_string(), "load"); + ASSERT_EQ(result->at("warnings").as_array().size(), 1); + EXPECT_EQ( + result->at("warnings").as_array().at(0).as_object().at("id").as_int64(), + static_cast(rpc::WarningCode::WarnRpcRateLimit) + ); + EXPECT_EQ(result->at("result").as_object().at("status").as_string(), "success"); + // NOLINTEND(bugprone-unchecked-optional-access) +} + +TEST(LoadWarningTest, AppendsToExistingWarningsArray) +{ + auto const result = + withLoadWarning(R"({"warnings": [{"id": 2001, "message": "already here"}]})"); + + ASSERT_TRUE(result.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) + EXPECT_EQ(result->at("warning").as_string(), "load"); + ASSERT_EQ(result->at("warnings").as_array().size(), 2); + EXPECT_EQ(result->at("warnings").as_array().at(0).as_object().at("id").as_int64(), 2001); + EXPECT_EQ( + result->at("warnings").as_array().at(1).as_object().at("id").as_int64(), + static_cast(rpc::WarningCode::WarnRpcRateLimit) + ); + // NOLINTEND(bugprone-unchecked-optional-access) +} + +TEST(LoadWarningTest, ReplacesNonArrayWarningsField) +{ + auto const result = withLoadWarning(R"({"warnings": "not an array"})"); + + ASSERT_TRUE(result.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) + ASSERT_TRUE(result->at("warnings").is_array()); + ASSERT_EQ(result->at("warnings").as_array().size(), 1); + EXPECT_EQ( + result->at("warnings").as_array().at(0).as_object().at("id").as_int64(), + static_cast(rpc::WarningCode::WarnRpcRateLimit) + ); + // NOLINTEND(bugprone-unchecked-optional-access) +} diff --git a/tests/unit/web/ng/RPCServerHandlerTests.cpp b/tests/unit/web/ng/RPCServerHandlerTests.cpp index 3a0f9c79a..2dab021d4 100644 --- a/tests/unit/web/ng/RPCServerHandlerTests.cpp +++ b/tests/unit/web/ng/RPCServerHandlerTests.cpp @@ -197,6 +197,52 @@ TEST_F(NgRpcServerHandlerTest, JsonParseFailed) }); } +TEST_F(NgRpcServerHandlerTest, JsonParseFailedAndDosguardLimitReached) +{ + runSpawn([&](boost::asio::yield_context yield) { + auto const request = makeHttpRequest("not a json"); + + EXPECT_CALL(dosguard_, isOk(ip_)).WillOnce(Return(true)); + EXPECT_CALL(*rpcEngine_, post).WillOnce([&](auto&& fn, auto&&) { + EXPECT_CALL(*rpcEngine_, notifyBadSyntax); + fn(yield); + return true; + }); + EXPECT_CALL(dosguard_, add(ip_, testing::_)).WillOnce(Return(false)); + + auto response = rpcServerHandler_(request, connectionMetadata_, nullptr, yield); + auto const responseHttp = std::move(response).intoHttpResponse(); + + EXPECT_EQ(responseHttp.result(), http::status::bad_request); + EXPECT_EQ(responseHttp.body(), "Unable to parse JSON from the request"); + }); +} + +TEST_F(NgRpcServerHandlerTest, WsJsonParseFailedAndDosguardLimitReached) +{ + runSpawn([&](boost::asio::yield_context yield) { + auto const request = makeWsRequest("not a json"); + + EXPECT_CALL(dosguard_, isOk(ip_)).WillOnce(Return(true)); + EXPECT_CALL(*rpcEngine_, post).WillOnce([&](auto&& fn, auto&&) { + EXPECT_CALL(*rpcEngine_, notifyBadSyntax); + fn(yield); + return true; + }); + EXPECT_CALL(dosguard_, add(ip_, testing::_)).WillOnce(Return(false)); + + auto const response = rpcServerHandler_(request, connectionMetadata_, nullptr, yield); + auto const responseJson = boost::json::parse(response.message()).as_object(); + + EXPECT_EQ(responseJson.at("error").as_string(), "badSyntax"); + EXPECT_EQ(responseJson.at("warning").as_string(), "load"); + EXPECT_EQ( + responseJson.at("warnings").as_array().at(0).as_object().at("id").as_int64(), + static_cast(rpc::WarningCode::WarnRpcRateLimit) + ); + }); +} + TEST_F(NgRpcServerHandlerTest, DosguardRejectedParsedRequest) { runSpawn([&](boost::asio::yield_context yield) {