fix: Client IP resolving when proxy reusing TCP connection (#3043)

Proxy may reuse TCP connections to send HTTP requests from different
clients. This PR fixes bug when Clio resolves client IP only once per
connection.

src/web/ProxyIpResolver.cpp

@@ -8,6 +8,7 @@
 
 #include <boost/beast/http/field.hpp>
 
+#include <algorithm>
 #include <optional>
 #include <string>
 #include <string_view>
@@ -49,20 +50,20 @@ ProxyIpResolver::fromConfig(util::config::ClioConfigDefinition const& config)
     return ProxyIpResolver{std::move(ips), std::move(tokens)};
 }
 
-std::string
+std::optional<std::string>
 ProxyIpResolver::resolveClientIp(std::string const& connectionIp, HttpHeaders const& headers) const
 {
     if (proxyIps_.contains(connectionIp)) {
-        return extractClientIp(headers).value_or(connectionIp);
+        return extractClientIp(headers);
     }
 
     if (auto it = headers.find(kPROXY_TOKEN_HEADER); it != headers.end()) {
         auto const tokenHash = util::sha256sum(it->value());
         if (proxyTokens_.contains(tokenHash)) {
-            return extractClientIp(headers).value_or(connectionIp);
+            return extractClientIp(headers);
         }
     }
-    return connectionIp;
+    return std::nullopt;
 }
 
 std::optional<std::string>
@@ -78,14 +79,17 @@ ProxyIpResolver::extractClientIp(HttpHeaders const& headers)
     auto const headerValue = util::toLower(it->value());
 
     static constexpr std::string_view kFOR_PREFIX = "for=";
-    auto const startPos = headerValue.find(kFOR_PREFIX);
+    auto const startPos = headerValue.rfind(kFOR_PREFIX);
     if (startPos == std::string::npos) {
         return std::nullopt;
     }
     auto value = it->value().substr(startPos + kFOR_PREFIX.size());
 
-    static constexpr char kDELIMITER = ';';
+    static constexpr char kSECTION_DELIMITER = ';';
-    auto const endPos = value.find(kDELIMITER);
+    static constexpr char kCHAIN_DELIMITER = ',';
+    auto const sectionEnd = value.find(kSECTION_DELIMITER);
+    auto const chainEnd = value.find(kCHAIN_DELIMITER);
+    auto const endPos = std::min(sectionEnd, chainEnd);
     auto const ip = value.substr(0, endPos);
 
     static constexpr auto kMIN_IP_LENGTH = 7;  // minimum 3 dots + 4 digits

src/web/ProxyIpResolver.hpp

@@ -59,16 +59,16 @@ public:
      *
      * If the connection IP is in the trusted proxy list, or if a valid proxy token is provided in
      * the headers, this method will attempt to extract the client's IP from the `Forwarded` header.
-     * Otherwise, it returns the connection IP.
+     * Otherwise, returns std::nullopt.
      *
      * @param connectionIp The IP address of the direct connection.
      * @param headers The HTTP request headers.
-     * @return The resolved client IP address as a string.
+     * @return The resolved client IP address if the connection is from a trusted proxy, otherwise
+     * std::nullopt.
      */
-    std::string
+    std::optional<std::string>
     resolveClientIp(std::string const& connectionIp, HttpHeaders const& headers) const;
 
-private:
     /**
      * @brief Extracts the client IP from the `Forwarded` HTTP header.
      *

src/web/impl/HttpBase.hpp

@@ -122,6 +122,7 @@ class HttpBase : public ConnectionBase {
     SendLambda sender_;
     std::shared_ptr<AdminVerificationStrategy> adminVerification_;
     std::shared_ptr<ProxyIpResolver> proxyIpResolver_;
+    bool isProxyConnection_ = false;
 
 protected:
     boost::beast::flat_buffer buffer_;
@@ -222,14 +223,26 @@ public:
         if (ec)
             return httpFail(ec, "read");
 
-        if (auto resolvedIp = proxyIpResolver_->resolveClientIp(clientIp_, req_);
+        auto const updateClientIp = [&](std::string newIp) {
-            resolvedIp != clientIp_) {
+            if (newIp == clientIp_)
+                return;
             LOG(log_.info()) << tag()
-                             << "Detected a forwarded request from proxy. Proxy ip: " << clientIp_
+                             << "Detected a forwarded request from proxy. Resolved client ip: "
-                             << ". Resolved client ip: " << resolvedIp;
+                             << newIp;
             dosGuard_.get().decrement(clientIp_);
-            clientIp_ = std::move(resolvedIp);
+            clientIp_ = std::move(newIp);
             dosGuard_.get().increment(clientIp_);
+        };
+
+        if (isProxyConnection_) {
+            if (auto resolvedIp = ProxyIpResolver::extractClientIp(req_); resolvedIp.has_value())
+                updateClientIp(std::move(*resolvedIp));
+        } else if (
+            auto resolvedIp = proxyIpResolver_->resolveClientIp(clientIp_, req_);
+            resolvedIp.has_value()
+        ) {
+            updateClientIp(std::move(*resolvedIp));
+            isProxyConnection_ = true;
         }
 
         if (req_.method() == http::verb::get and req_.target() == "/health")

src/web/ng/Connection.hpp

@@ -26,6 +26,7 @@ class ConnectionMetadata : public util::Taggable {
 protected:
     std::string ip_;  // client ip
     std::optional<bool> isAdmin_;
+    bool isProxyConnection_ = false;
 
 public:
     /**
@@ -63,6 +64,26 @@ public:
         ip_ = std::move(newIp);
     }
 
+    /**
+     * @brief Mark this connection as coming through a trusted proxy.
+     */
+    void
+    markAsProxyConnection()
+    {
+        isProxyConnection_ = true;
+    }
+
+    /**
+     * @brief Whether this connection was identified as coming through a trusted proxy.
+     *
+     * @return true if the connection is a proxy connection.
+     */
+    [[nodiscard]] bool
+    isProxyConnection() const
+    {
+        return isProxyConnection_;
+    }
+
     /**
      * @brief Get whether the client is an admin.
      *

src/web/ng/impl/ConnectionHandler.cpp

@@ -402,14 +402,26 @@ ConnectionHandler::handleRequest(
 void
 ConnectionHandler::resolveClientIp(Connection& connection, Request const& request) const
 {
-    if (auto resolvedClientIp =
+    auto const updateIp = [&](std::string newIp) {
-            proxyIpResolver_.resolveClientIp(connection.ip(), request.httpHeaders());
+        if (newIp == connection.ip())
-        resolvedClientIp != connection.ip()) {
+            return;
         LOG(log_.info()) << connection.tag()
-                         << "Detected a forwarded request from proxy. Proxy ip: " << connection.ip()
+                         << "Detected a forwarded request from proxy. Resolved client ip: "
-                         << ". Resolved client ip: " << resolvedClientIp;
+                         << newIp;
-        onIpChangeHook_(connection.ip(), resolvedClientIp);
+        onIpChangeHook_(connection.ip(), newIp);
-        connection.setIp(std::move(resolvedClientIp));
+        connection.setIp(std::move(newIp));
+    };
+
+    if (connection.isProxyConnection()) {
+        if (auto resolvedIp = ProxyIpResolver::extractClientIp(request.httpHeaders());
+            resolvedIp.has_value())
+            updateIp(std::move(*resolvedIp));
+    } else if (
+        auto resolvedIp = proxyIpResolver_.resolveClientIp(connection.ip(), request.httpHeaders());
+        resolvedIp.has_value()
+    ) {
+        updateIp(std::move(*resolvedIp));
+        connection.markAsProxyConnection();
     }
 }
 

tests/unit/web/ProxyIpResolverTests.cpp

@@ -11,6 +11,7 @@
 #include <fmt/format.h>
 #include <gtest/gtest.h>
 
+#include <optional>
 #include <string>
 #include <unordered_set>
 #include <utility>
@@ -26,7 +27,7 @@ struct ProxyIpResolverTestParams {
     std::unordered_set<std::string> proxyTokens;
     std::vector<std::pair<std::string, std::string>> headers;
     std::string connectionIp;
-    std::string expectedIp;
+    std::optional<std::string> expectedIp;
 };
 
 class ProxyIpResolverTest : public ::testing::TestWithParam<ProxyIpResolverTestParams> {};
@@ -61,11 +62,11 @@ TEST_F(ProxyIpResolverTest, FromConfig)
     auto const proxyIpResolver = ProxyIpResolver::fromConfig(config);
     ProxyIpResolver::HttpHeaders headers;
 
-    EXPECT_EQ(proxyIpResolver.resolveClientIp(clientIp, headers), clientIp);
+    EXPECT_EQ(proxyIpResolver.resolveClientIp(clientIp, headers), std::nullopt);
-    EXPECT_EQ(proxyIpResolver.resolveClientIp(proxyIp, headers), proxyIp);
+    EXPECT_EQ(proxyIpResolver.resolveClientIp(proxyIp, headers), std::nullopt);
 
     headers.set(boost::beast::http::field::forwarded, fmt::format("for={}", clientIp));
-    EXPECT_EQ(proxyIpResolver.resolveClientIp(clientIp, headers), clientIp);
+    EXPECT_EQ(proxyIpResolver.resolveClientIp(clientIp, headers), std::nullopt);
     EXPECT_EQ(proxyIpResolver.resolveClientIp(proxyIp, headers), clientIp);
 
     headers.set(ProxyIpResolver::kPROXY_TOKEN_HEADER, proxyToken);
@@ -95,7 +96,7 @@ INSTANTIATE_TEST_SUITE_P(
             .proxyTokens = {},
             .headers = {},
             .connectionIp = "1.2.3.4",
-            .expectedIp = "1.2.3.4"
+            .expectedIp = std::nullopt
         },
         ProxyIpResolverTestParams{
             .testName = "TrustedProxyIpWithForwardedHeader",
@@ -111,7 +112,7 @@ INSTANTIATE_TEST_SUITE_P(
             .proxyTokens = {},
             .headers = {},
             .connectionIp = "5.6.7.8",
-            .expectedIp = "5.6.7.8"
+            .expectedIp = std::nullopt
         },
         ProxyIpResolverTestParams{
             .testName = "UntrustedProxyIpWithForwardedHeader",
@@ -119,7 +120,7 @@ INSTANTIATE_TEST_SUITE_P(
             .proxyTokens = {},
             .headers = {{std::string(http::to_string(http::field::forwarded)), "for=1.2.3.4"}},
             .connectionIp = "5.6.7.8",
-            .expectedIp = "5.6.7.8"
+            .expectedIp = std::nullopt
         },
         ProxyIpResolverTestParams{
             .testName = "TrustedProxyTokenWithForwardedHeader",
@@ -137,7 +138,7 @@ INSTANTIATE_TEST_SUITE_P(
             .proxyTokens = {"test_token"},
             .headers = {{std::string(ProxyIpResolver::kPROXY_TOKEN_HEADER), "test_token"}},
             .connectionIp = "5.6.7.8",
-            .expectedIp = "5.6.7.8"
+            .expectedIp = std::nullopt
         },
         ProxyIpResolverTestParams{
             .testName = "UntrustedProxyTokenWithForwardedHeader",
@@ -147,7 +148,7 @@ INSTANTIATE_TEST_SUITE_P(
                 {{std::string(ProxyIpResolver::kPROXY_TOKEN_HEADER), "test_token"},
                  {std::string(http::to_string(http::field::forwarded)), "for=1.2.3.4"}},
             .connectionIp = "5.6.7.8",
-            .expectedIp = "5.6.7.8"
+            .expectedIp = std::nullopt
         },
         ProxyIpResolverTestParams{
             .testName = "ForwardedHeaderWithAdditionalFields",
@@ -173,7 +174,7 @@ INSTANTIATE_TEST_SUITE_P(
             .proxyTokens = {},
             .headers = {{std::string(http::to_string(http::field::forwarded)), "by=1.2.3.4"}},
             .connectionIp = "5.6.7.8",
-            .expectedIp = "5.6.7.8"
+            .expectedIp = std::nullopt
         },
         ProxyIpResolverTestParams{
             .testName = "ForwardedHeaderWithIpInQuotes",
@@ -190,7 +191,27 @@ INSTANTIATE_TEST_SUITE_P(
             .headers =
                 {{std::string(http::to_string(http::field::forwarded)), "for=\";some_other_text"}},
             .connectionIp = "5.6.7.8",
-            .expectedIp = "5.6.7.8"
+            .expectedIp = std::nullopt
+        },
+        ProxyIpResolverTestParams{
+            .testName = "ForwardedHeaderWithMultipleForValues",
+            .proxyIps = {"5.6.7.8"},
+            .proxyTokens = {},
+            .headers =
+                {{std::string(http::to_string(http::field::forwarded)),
+                  "for=1.2.3.4, for=9.10.11.12"}},
+            .connectionIp = "5.6.7.8",
+            .expectedIp = "9.10.11.12"
+        },
+        ProxyIpResolverTestParams{
+            .testName = "ForwardedHeaderWithMultipleForValuesAndSectionDelimiters",
+            .proxyIps = {"5.6.7.8"},
+            .proxyTokens = {},
+            .headers =
+                {{std::string(http::to_string(http::field::forwarded)),
+                  "for=1.2.3.4; proto=http, for=9.10.11.12; proto=https"}},
+            .connectionIp = "5.6.7.8",
+            .expectedIp = "9.10.11.12"
         }
     ),
     tests::util::kNAME_GENERATOR

tests/unit/web/ng/impl/ConnectionHandlerTests.cpp

@@ -593,6 +593,110 @@ TEST_F(ConnectionHandlerSequentialProcessingTest, OnIpChangeHookCalledWhenSentFr
              ) mutable { connectionHandler.processConnection(std::move(c), yield); });
 }
 
+TEST_F(ConnectionHandlerSequentialProcessingTest, ProxyConnection_SameClientReuses_HookCalledOnce)
+{
+    std::string const target = "/some/target";
+    testing::StrictMock<testing::MockFunction<Response(
+        Request const&,
+        ConnectionMetadata const&,
+        web::SubscriptionContextPtr,
+        boost::asio::yield_context
+    )>>
+        getHandlerMock;
+    connectionHandler.onGet(target, getHandlerMock.AsStdFunction());
+
+    StrictMockHttpConnectionPtr mockProxyConnection = std::make_unique<StrictMockHttpConnection>(
+        proxyIp, boost::beast::flat_buffer{}, tagDecoratorFactory
+    );
+
+    auto request = http::request<http::string_body>{http::verb::get, target, 11, ""};
+    request.set(http::field::forwarded, fmt::format("for={}", clientIp));
+
+    EXPECT_CALL(*mockProxyConnection, wasUpgraded).WillOnce(Return(false));
+    EXPECT_CALL(*mockProxyConnection, receive)
+        .WillOnce(Return(makeRequest(request)))
+        .WillOnce(Return(makeRequest(request)))
+        .WillOnce(Return(makeError(http::error::end_of_stream)));
+
+    EXPECT_CALL(onIpChangeMock, Call(proxyIp, clientIp));
+
+    EXPECT_CALL(getHandlerMock, Call)
+        .Times(2)
+        .WillRepeatedly([](Request const& req, auto&&, auto&&, auto&&) {
+            return Response(http::status::ok, "ok", req);
+        });
+
+    EXPECT_CALL(*mockProxyConnection, send)
+        .Times(2)
+        .WillRepeatedly(Return(std::expected<void, web::ng::Error>{}));
+
+    EXPECT_CALL(onDisconnectMock, Call)
+        .WillOnce([this, ptr = mockProxyConnection.get()](Connection const& c) {
+            EXPECT_EQ(&c, ptr);
+            EXPECT_EQ(c.ip(), clientIp);
+        });
+
+    runSpawn([this, c = std::move(mockProxyConnection)](boost::asio::yield_context yield) mutable {
+        connectionHandler.processConnection(std::move(c), yield);
+    });
+}
+
+TEST_F(
+    ConnectionHandlerSequentialProcessingTest,
+    ProxyConnection_DifferentClientReuses_HookCalledForEachIpChange
+)
+{
+    std::string const target = "/some/target";
+    std::string const anotherClientIp = "9.10.11.12";
+    testing::StrictMock<testing::MockFunction<Response(
+        Request const&,
+        ConnectionMetadata const&,
+        web::SubscriptionContextPtr,
+        boost::asio::yield_context
+    )>>
+        getHandlerMock;
+    connectionHandler.onGet(target, getHandlerMock.AsStdFunction());
+
+    StrictMockHttpConnectionPtr mockProxyConnection = std::make_unique<StrictMockHttpConnection>(
+        proxyIp, boost::beast::flat_buffer{}, tagDecoratorFactory
+    );
+
+    auto request1 = http::request<http::string_body>{http::verb::get, target, 11, ""};
+    request1.set(http::field::forwarded, fmt::format("for={}", clientIp));
+
+    auto request2 = http::request<http::string_body>{http::verb::get, target, 11, ""};
+    request2.set(http::field::forwarded, fmt::format("for={}", anotherClientIp));
+
+    EXPECT_CALL(*mockProxyConnection, wasUpgraded).WillOnce(Return(false));
+    EXPECT_CALL(*mockProxyConnection, receive)
+        .WillOnce(Return(makeRequest(request1)))
+        .WillOnce(Return(makeRequest(request2)))
+        .WillOnce(Return(makeError(http::error::end_of_stream)));
+
+    EXPECT_CALL(onIpChangeMock, Call(proxyIp, clientIp));
+    EXPECT_CALL(onIpChangeMock, Call(clientIp, anotherClientIp));
+
+    EXPECT_CALL(getHandlerMock, Call)
+        .Times(2)
+        .WillRepeatedly([](Request const& req, auto&&, auto&&, auto&&) {
+            return Response(http::status::ok, "ok", req);
+        });
+
+    EXPECT_CALL(*mockProxyConnection, send)
+        .Times(2)
+        .WillRepeatedly(Return(std::expected<void, web::ng::Error>{}));
+
+    EXPECT_CALL(onDisconnectMock, Call)
+        .WillOnce([anotherClientIp, ptr = mockProxyConnection.get()](Connection const& c) {
+            EXPECT_EQ(&c, ptr);
+            EXPECT_EQ(c.ip(), anotherClientIp);
+        });
+
+    runSpawn([this, c = std::move(mockProxyConnection)](boost::asio::yield_context yield) mutable {
+        connectionHandler.processConnection(std::move(c), yield);
+    });
+}
+
 TEST_F(ConnectionHandlerSequentialProcessingTest, Stop)
 {
     testing::StrictMock<testing::MockFunction<Response(
@@ -840,6 +944,85 @@ TEST_F(ConnectionHandlerParallelProcessingTest, OnIpChangeHookCalledWhenSentFrom
     });
 }
 
+TEST_F(ConnectionHandlerParallelProcessingTest, ProxyConnection_SameClientReuses_HookCalledOnce)
+{
+    connectionHandler.onWs([](Request const& req, auto&&, auto&&, auto&&) {
+        return Response(http::status::ok, "ok", req);
+    });
+
+    StrictMockWsConnectionPtr mockProxyConnection = std::make_unique<StrictMockWsConnection>(
+        proxyIp, boost::beast::flat_buffer{}, tagDecoratorFactory
+    );
+
+    headers.set(http::field::forwarded, fmt::format("for={}", clientIp));
+
+    EXPECT_CALL(*mockProxyConnection, wasUpgraded).WillOnce(Return(true));
+    EXPECT_CALL(*mockProxyConnection, receive)
+        .WillOnce(Return(makeRequest("msg", headers)))
+        .WillOnce(Return(makeRequest("msg", headers)))
+        .WillOnce(Return(makeError(websocket::error::closed)));
+
+    EXPECT_CALL(onIpChangeMock, Call(proxyIp, clientIp));
+
+    EXPECT_CALL(*mockProxyConnection, send)
+        .Times(2)
+        .WillRepeatedly(Return(std::expected<void, web::ng::Error>{}));
+
+    EXPECT_CALL(onDisconnectMock, Call)
+        .WillOnce([this, ptr = mockProxyConnection.get()](Connection const& c) {
+            EXPECT_EQ(&c, ptr);
+            EXPECT_EQ(c.ip(), clientIp);
+        });
+
+    runSpawn([this, c = std::move(mockProxyConnection)](boost::asio::yield_context yield) mutable {
+        connectionHandler.processConnection(std::move(c), yield);
+    });
+}
+
+TEST_F(
+    ConnectionHandlerParallelProcessingTest,
+    ProxyConnection_DifferentClientReuses_HookCalledForEachIpChange
+)
+{
+    std::string const anotherClientIp = "9.10.11.12";
+    connectionHandler.onWs([](Request const& req, auto&&, auto&&, auto&&) {
+        return Response(http::status::ok, "ok", req);
+    });
+
+    StrictMockWsConnectionPtr mockProxyConnection = std::make_unique<StrictMockWsConnection>(
+        proxyIp, boost::beast::flat_buffer{}, tagDecoratorFactory
+    );
+
+    Request::HttpHeaders headers1;
+    headers1.set(http::field::forwarded, fmt::format("for={}", clientIp));
+
+    Request::HttpHeaders headers2;
+    headers2.set(http::field::forwarded, fmt::format("for={}", anotherClientIp));
+
+    EXPECT_CALL(*mockProxyConnection, wasUpgraded).WillOnce(Return(true));
+    EXPECT_CALL(*mockProxyConnection, receive)
+        .WillOnce(Return(makeRequest("msg", headers1)))
+        .WillOnce(Return(makeRequest("msg", headers2)))
+        .WillOnce(Return(makeError(websocket::error::closed)));
+
+    EXPECT_CALL(onIpChangeMock, Call(proxyIp, clientIp));
+    EXPECT_CALL(onIpChangeMock, Call(clientIp, anotherClientIp));
+
+    EXPECT_CALL(*mockProxyConnection, send)
+        .Times(2)
+        .WillRepeatedly(Return(std::expected<void, web::ng::Error>{}));
+
+    EXPECT_CALL(onDisconnectMock, Call)
+        .WillOnce([anotherClientIp, ptr = mockProxyConnection.get()](Connection const& c) {
+            EXPECT_EQ(&c, ptr);
+            EXPECT_EQ(c.ip(), anotherClientIp);
+        });
+
+    runSpawn([this, c = std::move(mockProxyConnection)](boost::asio::yield_context yield) mutable {
+        connectionHandler.processConnection(std::move(c), yield);
+    });
+}
+
 TEST_F(ConnectionHandlerParallelProcessingTest, Receive_Handle_Send_Loop)
 {
     testing::StrictMock<testing::MockFunction<Response(