fix: Internal error on warning injection (#3177)

src/web/LoadWarning.hpp

@@ -0,0 +1,54 @@
+#pragma once
+
+#include "rpc/Errors.hpp"
+
+#include <boost/json/array.hpp>
+#include <boost/json/object.hpp>
+#include <boost/json/parse.hpp>
+#include <boost/system/error_code.hpp>
+
+#include <optional>
+#include <string_view>
+
+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<boost::json::object>
+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

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)));
     }

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::string>(std::move(msg));
         send(std::move(sharedMsg));

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
     {