From fe0bf736fb7f2fbc6f2125bc0deb197ef21ca2e1 Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Mon, 27 Apr 2026 11:32:07 +0100 Subject: [PATCH] refactor: Use error code in `make_address()` calls (#3044) Function `ip::make_address()` throws an exception on an invalid IP. Refactor to a better error handling without exceptions. --- docs/config-description.md | 4 +- src/app/ClioApplication.cpp | 18 +++-- src/util/config/ConfigDefinition.cpp | 6 +- src/web/Server.hpp | 21 +++--- src/web/dosguard/WhitelistHandler.cpp | 40 ++++++++--- src/web/dosguard/WhitelistHandler.hpp | 40 +++++++---- tests/unit/rpc/RPCEngineTests.cpp | 4 +- tests/unit/web/ServerTests.cpp | 43 ++++++++---- .../web/dosguard/WhitelistHandlerTests.cpp | 70 ++++++++++++++++++- 9 files changed, 191 insertions(+), 55 deletions(-) diff --git a/docs/config-description.md b/docs/config-description.md index 312eac6b8..6a5a15f88 100644 --- a/docs/config-description.md +++ b/docs/config-description.md @@ -230,7 +230,7 @@ This document provides a list of all available Clio configuration properties in - **Required**: False - **Type**: string - **Default value**: None -- **Constraints**: None +- **Constraints**: The value must be a valid IP address. - **Description**: The list of IP addresses to whitelist for DOS protection. ### dos_guard.max_fetches @@ -342,7 +342,7 @@ This document provides a list of all available Clio configuration properties in - **Required**: True - **Type**: string - **Default value**: None -- **Constraints**: None +- **Constraints**: The value must be a valid IP address. - **Description**: List of proxy ip addresses. When Clio receives a request from proxy it will use `Forwarded` value (if any) as client ip. When this option is used together with `server.proxy.tokens` Clio will identify proxy by ip or by token. ### server.proxy.tokens.[] diff --git a/src/app/ClioApplication.cpp b/src/app/ClioApplication.cpp index 7755db4e1..d1975952f 100644 --- a/src/app/ClioApplication.cpp +++ b/src/app/ClioApplication.cpp @@ -92,9 +92,14 @@ ClioApplication::run(bool const useNgWebServer) boost::asio::io_context ioc{threads}; // Rate limiter, to prevent abuse - auto whitelistHandler = web::dosguard::WhitelistHandler{config_}; + auto whitelistHandler = web::dosguard::WhitelistHandler::create(config_); + if (not whitelistHandler.has_value()) { + LOG(util::LogService::fatal()) << whitelistHandler.error(); + return EXIT_FAILURE; + } + auto const dosguardWeights = web::dosguard::Weights::make(config_); - auto dosGuard = web::dosguard::DOSGuard{config_, whitelistHandler, dosguardWeights}; + auto dosGuard = web::dosguard::DOSGuard{config_, *whitelistHandler, dosguardWeights}; auto sweepHandler = web::dosguard::IntervalSweepHandler{config_, ioc, dosGuard}; auto cache = data::LedgerCache{}; @@ -222,10 +227,15 @@ ClioApplication::run(bool const useNgWebServer) config_, backend, rpcEngine, etl, dosGuard ); - auto const httpServer = web::makeHttpServer(config_, ioc, dosGuard, handler, cache); + auto const expectedHttpServer = web::makeHttpServer(config_, ioc, dosGuard, handler, cache); + if (not expectedHttpServer.has_value()) { + LOG(util::LogService::fatal()) << expectedHttpServer.error(); + return EXIT_FAILURE; + } + appStopper_.setOnStop( Stopper::makeOnStopCallback( - *httpServer, + **expectedHttpServer, *balancer, *etl, *subscriptions, diff --git a/src/util/config/ConfigDefinition.cpp b/src/util/config/ConfigDefinition.cpp index a29c616e5..c5db1287e 100644 --- a/src/util/config/ConfigDefinition.cpp +++ b/src/util/config/ConfigDefinition.cpp @@ -312,7 +312,8 @@ getClioConfig() {"num_markers", ConfigValue{ConfigType::Integer}.optional().withConstraint(gValidateNumMarkers)}, - {"dos_guard.whitelist.[]", Array{ConfigValue{ConfigType::String}.optional()}}, + {"dos_guard.whitelist.[]", + Array{ConfigValue{ConfigType::String}.optional().withConstraint(gValidateIp)}}, {"dos_guard.max_fetches", ConfigValue{ConfigType::Integer}.defaultValue(1000'000u).withConstraint(gValidateUint32)}, {"dos_guard.max_connections", @@ -361,7 +362,8 @@ getClioConfig() {"server.ws_max_sending_queue_size", ConfigValue{ConfigType::Integer}.defaultValue(1500).withConstraint(gValidateUint32)}, {"server.__ng_web_server", ConfigValue{ConfigType::Boolean}.defaultValue(false)}, - {"server.proxy.ips.[]", Array{ConfigValue{ConfigType::String}}}, + {"server.proxy.ips.[]", + Array{ConfigValue{ConfigType::String}.withConstraint(gValidateIp)}}, {"server.proxy.tokens.[]", Array{ConfigValue{ConfigType::String}}}, {"prometheus.enabled", ConfigValue{ConfigType::Boolean}.defaultValue(true)}, diff --git a/src/web/Server.hpp b/src/web/Server.hpp index e1c9873ea..a79299996 100644 --- a/src/web/Server.hpp +++ b/src/web/Server.hpp @@ -375,7 +375,7 @@ using HttpServer = Server; * @return The server instance */ template -static std::shared_ptr> +static std::expected>, std::string> makeHttpServer( util::config::ClioConfigDefinition const& config, boost::asio::io_context& ioc, @@ -388,19 +388,24 @@ makeHttpServer( auto expectedSslContext = ng::impl::makeServerSslContext(config); if (not expectedSslContext) { - LOG(log.error()) << "Failed to create SSL context: " << expectedSslContext.error(); - return nullptr; + return std::unexpected( + fmt::format("Failed to create SSL context: {}", expectedSslContext.error()) + ); } auto const serverConfig = config.getObject("server"); - auto const address = boost::asio::ip::make_address(serverConfig.get("ip")); + + auto const ipFromConfig = serverConfig.get("ip"); + boost::system::error_code ec; + auto const address = boost::asio::ip::make_address(ipFromConfig, ec); + if (ec.failed()) + return std::unexpected(fmt::format("Invalid 'server.ip' config value: {}", ipFromConfig)); + auto const port = serverConfig.get("port"); auto expectedAdminVerification = makeAdminVerificationStrategy(config); - if (not expectedAdminVerification.has_value()) { - LOG(log.error()) << expectedAdminVerification.error(); - throw std::logic_error{expectedAdminVerification.error()}; - } + if (not expectedAdminVerification.has_value()) + return std::unexpected(expectedAdminVerification.error()); // If the transactions number is 200 per ledger, A client which subscribes everything will send // 400+ feeds for each ledger. we allow user delay 3 ledgers by default diff --git a/src/web/dosguard/WhitelistHandler.cpp b/src/web/dosguard/WhitelistHandler.cpp index b7eb98f7a..819ad8824 100644 --- a/src/web/dosguard/WhitelistHandler.cpp +++ b/src/web/dosguard/WhitelistHandler.cpp @@ -8,32 +8,48 @@ #include #include #include -#include #include #include -#include #include +#include #include namespace web::dosguard { -void +WhitelistHandler::WhitelistHandler(Whitelist whitelist) : whitelist_(std::move(whitelist)) +{ +} + +std::expected Whitelist::add(std::string_view net) { using namespace boost::asio; if (not isMask(net)) { - ips_.push_back(ip::make_address(net)); - return; + boost::system::error_code ec; + auto const ip = ip::make_address(net, ec); + if (ec.failed()) + return std::unexpected{fmt::format("Malformed whitelist ip address: {}. ", net)}; + ips_.push_back(ip); + return {}; } if (isV4(net)) { - subnetsV4_.push_back(ip::make_network_v4(net)); + boost::system::error_code ec; + auto const net4 = ip::make_network_v4(net, ec); + if (ec.failed()) + return std::unexpected{fmt::format("Malformed network: {}. ", net)}; + subnetsV4_.push_back(net4); } else if (isV6(net)) { - subnetsV6_.push_back(ip::make_network_v6(net)); + boost::system::error_code ec; + auto const net6 = ip::make_network_v6(net, ec); + if (ec.failed()) + return std::unexpected{fmt::format("Malformed network: {}. ", net)}; + subnetsV6_.push_back(net6); } else { - throw std::runtime_error(fmt::format("malformed network: {}", net.data())); + return std::unexpected{fmt::format("Malformed network: {}. ", net)}; } + return {}; } bool @@ -41,7 +57,11 @@ Whitelist::isWhiteListed(std::string_view ip) const { using namespace boost::asio; - auto const addr = ip::make_address(ip); + boost::system::error_code ec; + auto const addr = ip::make_address(ip, ec); + if (ec.failed()) + return false; + if (std::ranges::find(ips_, addr) != std::end(ips_)) return true; @@ -95,7 +115,7 @@ Whitelist::isV6(std::string_view net) bool Whitelist::isMask(std::string_view net) { - return net.find('/') != std::string_view::npos; + return net.contains('/'); } } // namespace web::dosguard diff --git a/src/web/dosguard/WhitelistHandler.hpp b/src/web/dosguard/WhitelistHandler.hpp index 6a8e9613c..83b7b342f 100644 --- a/src/web/dosguard/WhitelistHandler.hpp +++ b/src/web/dosguard/WhitelistHandler.hpp @@ -14,11 +14,11 @@ #include #include -#include +#include #include #include -#include #include +#include #include namespace web::dosguard { @@ -36,16 +36,15 @@ public: * @brief Add network address to whitelist. * * @param net Network part of the ip address - * @throws std::runtime::error when the network address is not valid + * @return void on success, or an error string if the address is not valid */ - void + std::expected add(std::string_view net); /** * @brief Checks to see if ip address is whitelisted. * * @param ip IP address - * @throws std::runtime::error when the network address is not valid * @return true if the given IP is whitelisted; false otherwise */ bool @@ -76,21 +75,38 @@ class WhitelistHandler : public WhitelistHandlerInterface { public: /** - * @brief Adds all whitelisted IPs and masks from the given config. + * @brief Constructs a WhitelistHandler from an already-built Whitelist. + * + * @param whitelist The whitelist to use + */ + explicit WhitelistHandler(Whitelist whitelist); + + /** + * @brief Creates a WhitelistHandler by loading all whitelisted IPs and masks from config. * * @param config The Clio config to use * @param resolver The resolver to use for hostname resolution + * @return The WhitelistHandler on success, or an error string if any whitelist entry is invalid */ template - WhitelistHandler( - util::config::ClioConfigDefinition const& config, - HostnameResolverType&& resolver = {} - ) + static std::expected + create(util::config::ClioConfigDefinition const& config, HostnameResolverType&& resolver = {}) { std::unordered_set const arr = getWhitelist(config, std::forward(resolver)); - for (auto const& net : arr) - whitelist_.add(net); + Whitelist whitelist; + std::optional errors; + for (auto const& net : arr) { + if (auto result = whitelist.add(net); !result.has_value()) { + if (!errors.has_value()) + errors.emplace(); + errors->append(std::move(result).error()); + } + } + if (errors.has_value()) { + return std::unexpected{std::move(errors).value()}; + } + return WhitelistHandler(std::move(whitelist)); } /** diff --git a/tests/unit/rpc/RPCEngineTests.cpp b/tests/unit/rpc/RPCEngineTests.cpp index 874ee1a28..538226294 100644 --- a/tests/unit/rpc/RPCEngineTests.cpp +++ b/tests/unit/rpc/RPCEngineTests.cpp @@ -81,7 +81,9 @@ struct RPCEngineTest : util::prometheus::WithPrometheus, util::TagDecoratorFactory tagFactory{cfg}; WorkQueue queue = WorkQueue::makeWorkQueue(cfg); - web::dosguard::WhitelistHandler whitelistHandler{cfg}; + web::dosguard::WhitelistHandler whitelistHandler{ + web::dosguard::WhitelistHandler::create(cfg).value() + }; web::dosguard::Weights weights{1, {}}; web::dosguard::DOSGuard dosGuard{cfg, whitelistHandler, weights}; std::shared_ptr handlerProvider = std::make_shared(); diff --git a/tests/unit/web/ServerTests.cpp b/tests/unit/web/ServerTests.cpp index 04fcb09ce..6879e85a2 100644 --- a/tests/unit/web/ServerTests.cpp +++ b/tests/unit/web/ServerTests.cpp @@ -150,13 +150,15 @@ struct WebServerTest : public virtual ::testing::Test { boost::asio::io_context ctxSync; std::string const port = std::to_string(tests::util::generateFreePort()); ClioConfigDefinition cfg{getParseServerConfig(generateJSONWithDynamicPort(port))}; - dosguard::WhitelistHandler whitelistHandler{cfg}; + dosguard::WhitelistHandler whitelistHandler{dosguard::WhitelistHandler::create(cfg).value()}; dosguard::Weights dosguardWeights{1, {}}; dosguard::DOSGuard dosGuard{cfg, whitelistHandler, dosguardWeights}; dosguard::IntervalSweepHandler sweepHandler{cfg, ctxSync, dosGuard}; ClioConfigDefinition cfgOverload{getParseServerConfig(generateJSONDataOverload(port))}; - dosguard::WhitelistHandler whitelistHandlerOverload{cfgOverload}; + dosguard::WhitelistHandler whitelistHandlerOverload{ + dosguard::WhitelistHandler::create(cfgOverload).value() + }; dosguard::DOSGuard dosGuardOverload{cfgOverload, whitelistHandlerOverload, dosguardWeights}; dosguard::IntervalSweepHandler sweepHandlerOverload{cfgOverload, ctxSync, dosGuardOverload}; // this ctx is for http server @@ -216,11 +218,26 @@ makeServerSync( std::reference_wrapper cache ) { - return web::makeHttpServer(config, ioc, dosGuard, handler, cache); + auto result = web::makeHttpServer(config, ioc, dosGuard, handler, cache); + [&]() { ASSERT_TRUE(result.has_value()); }(); + return std::move(result).value(); } } // namespace +TEST_F(WebServerTest, InvalidIpAddress) +{ + auto jsonConfig = generateJSONWithDynamicPort(port); + jsonConfig.as_object()["server"].as_object()["ip"] = "not-an-ip"; + + auto cache = MockLedgerCache(); + auto const e = std::make_shared(); + auto const result = + web::makeHttpServer(getParseServerConfig(jsonConfig), ctx, dosGuard, e, cache); + ASSERT_FALSE(result.has_value()); + EXPECT_THAT(result.error(), testing::HasSubstr("Invalid 'server.ip' config value")); +} + TEST_F(WebServerTest, Http) { auto cache = MockLedgerCache(); @@ -294,8 +311,9 @@ TEST_F(WebServerTest, IncompleteSslConfig) jsonConfig.as_object()["ssl_key_file"] = sslKeyFile.path; auto cache = MockLedgerCache(); - auto const server = makeServerSync(getParseServerConfig(jsonConfig), ctx, dosGuard, e, cache); - EXPECT_EQ(server, nullptr); + auto const result = + web::makeHttpServer(getParseServerConfig(jsonConfig), ctx, dosGuard, e, cache); + EXPECT_FALSE(result.has_value()); } TEST_F(WebServerTest, WrongSslConfig) @@ -307,8 +325,9 @@ TEST_F(WebServerTest, WrongSslConfig) jsonConfig.as_object()["ssl_cert_file"] = "wrong_path"; auto cache = MockLedgerCache(); - auto const server = makeServerSync(getParseServerConfig(jsonConfig), ctx, dosGuard, e, cache); - EXPECT_EQ(server, nullptr); + auto const result = + web::makeHttpServer(getParseServerConfig(jsonConfig), ctx, dosGuard, e, cache); + EXPECT_FALSE(result.has_value()); } TEST_F(WebServerTest, Https) @@ -694,9 +713,8 @@ TEST_F(WebServerTest, AdminErrorCfgTestBothAdminPasswordAndLocalAdminSet) )}; MockLedgerCache cache; - EXPECT_THROW( - web::makeHttpServer(serverConfig, ctx, dosGuardOverload, e, cache), std::logic_error - ); + auto const result = web::makeHttpServer(serverConfig, ctx, dosGuardOverload, e, cache); + EXPECT_FALSE(result.has_value()); } TEST_F(WebServerTest, AdminErrorCfgTestBothAdminPasswordAndLocalAdminFalse) @@ -719,9 +737,8 @@ TEST_F(WebServerTest, AdminErrorCfgTestBothAdminPasswordAndLocalAdminFalse) )}; MockLedgerCache cache; - EXPECT_THROW( - web::makeHttpServer(serverConfig, ctx, dosGuardOverload, e, cache), std::logic_error - ); + auto const result = web::makeHttpServer(serverConfig, ctx, dosGuardOverload, e, cache); + EXPECT_FALSE(result.has_value()); } struct WebServerPrometheusTest : util::prometheus::WithPrometheus, WebServerTest {}; diff --git a/tests/unit/web/dosguard/WhitelistHandlerTests.cpp b/tests/unit/web/dosguard/WhitelistHandlerTests.cpp index bdc94cc1a..928cb64e0 100644 --- a/tests/unit/web/dosguard/WhitelistHandlerTests.cpp +++ b/tests/unit/web/dosguard/WhitelistHandlerTests.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -62,7 +63,9 @@ TEST_F(WhitelistHandlerTest, TestWhiteListIPV4) ClioConfigDefinition const cfg{ getParseWhitelistHandlerConfig(boost::json::parse(kJSON_DATA_IP_V4)) }; - WhitelistHandler const whitelistHandler{cfg, mockResolver}; + auto const result = WhitelistHandler::create(cfg, mockResolver); + ASSERT_TRUE(result.has_value()); + auto const& whitelistHandler = *result; EXPECT_TRUE(whitelistHandler.isWhiteListed("192.168.1.10")); EXPECT_FALSE(whitelistHandler.isWhiteListed("193.168.0.123")); @@ -86,7 +89,9 @@ TEST_F(WhitelistHandlerTest, TestWhiteListResolvesHostname) ClioConfigDefinition const cfg{ getParseWhitelistHandlerConfig(boost::json::parse(kJSON_DATA_IP_V4)) }; - WhitelistHandler const whitelistHandler{cfg}; + auto const result = WhitelistHandler::create(cfg); + ASSERT_TRUE(result.has_value()); + auto const& whitelistHandler = *result; EXPECT_TRUE(whitelistHandler.isWhiteListed("127.0.0.1")); EXPECT_FALSE(whitelistHandler.isWhiteListed("193.168.0.123")); @@ -110,10 +115,69 @@ TEST_F(WhitelistHandlerTest, TestWhiteListIPV6) ClioConfigDefinition const cfg{ getParseWhitelistHandlerConfig(boost::json::parse(kJSON_DATA_IP_V6)) }; - WhitelistHandler const whitelistHandler{cfg}; + auto const result = WhitelistHandler::create(cfg); + ASSERT_TRUE(result.has_value()); + auto const& whitelistHandler = *result; EXPECT_TRUE(whitelistHandler.isWhiteListed("2002:1dd8:85a7:0000:0000:8a6e:0000:1111")); EXPECT_FALSE(whitelistHandler.isWhiteListed("2002:1dd8:85a7:1101:0000:8a6e:0000:1111")); EXPECT_TRUE(whitelistHandler.isWhiteListed("2001:0db8:85a3:0000:0000:8a2e:0000:0000")); EXPECT_TRUE(whitelistHandler.isWhiteListed("2001:0db8:85a3:0000:1111:8a2e:0370:7334")); } + +struct WhitelistTest : public virtual ::testing::Test {}; + +TEST_F(WhitelistTest, AddValidIPV4) +{ + Whitelist whitelist; + EXPECT_TRUE(whitelist.add("1.2.3.4").has_value()); +} + +TEST_F(WhitelistTest, AddInvalidIP) +{ + Whitelist whitelist; + auto const result = whitelist.add("not-an-ip"); + ASSERT_FALSE(result.has_value()); + EXPECT_THAT(result.error(), testing::HasSubstr("not-an-ip")); +} + +TEST_F(WhitelistTest, AddInvalidNetwork) +{ + Whitelist whitelist; + auto const result = whitelist.add("not-a-net/24"); + ASSERT_FALSE(result.has_value()); + EXPECT_THAT(result.error(), testing::HasSubstr("not-a-net/24")); +} + +TEST_F(WhitelistTest, IsWhiteListedWithInvalidIP) +{ + Whitelist whitelist; + EXPECT_FALSE(whitelist.isWhiteListed("not-an-ip")); +} + +TEST_F(WhitelistHandlerTest, CreateWithInvalidIPFails) +{ + struct MockResolver { + MOCK_METHOD(std::vector, resolve, (std::string_view, std::string_view)); + MOCK_METHOD(std::vector, resolve, (std::string_view)); + }; + + static constexpr auto kJSON = R"JSON( + { + "dos_guard": { + "whitelist": ["not-an-ip"] + } + } + )JSON"; + + testing::StrictMock mockResolver; + EXPECT_CALL(mockResolver, resolve(testing::_)) + .WillOnce([](auto hostname) -> std::vector { + return {std::string{hostname}}; + }); + + ClioConfigDefinition const cfg{getParseWhitelistHandlerConfig(boost::json::parse(kJSON))}; + auto const result = WhitelistHandler::create(cfg, mockResolver); + ASSERT_FALSE(result.has_value()); + EXPECT_THAT(result.error(), testing::HasSubstr("not-an-ip")); +}