From ea6af0048cbd6a8f07c1d68c271d3d0ebec593cd Mon Sep 17 00:00:00 2001 From: Zach H Date: Sun, 18 Oct 2026 16:53:55 -0400 Subject: [PATCH] [Server] Parse the rightmost address from forwarded header lists (#7586) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Forwarded-For carries a comma-separated list, and constructing a QHostAddress from the raw header value returns a null address for it, so a configured X-Forwarded-For or a multi-hop header never resolved and websocket clients stayed on the proxy's address. - Add parseForwardedAddress, returning the rightmost usable entry of a forwarded header list - Use it for the websocket handshake instead of parsing the raw value - Exempt loopback from per-address auth rate limits so a same-host proxy with a customized trusted_sources list cannot lock every user out - Add forwarded_address_test Co-authored-by: Lukas Brübach --- servatrice/CMakeLists.txt | 1 + servatrice/servatrice.ini.example | 8 +++-- servatrice/src/forwarded_address.cpp | 21 ++++++++++++++ servatrice/src/forwarded_address.h | 18 ++++++++++++ servatrice/src/servatrice.cpp | 7 +++-- servatrice/src/serversocketinterface.cpp | 4 +-- tests/CMakeLists.txt | 11 +++++++ tests/forwarded_address_test.cpp | 37 ++++++++++++++++++++++++ 8 files changed, 100 insertions(+), 7 deletions(-) create mode 100644 servatrice/src/forwarded_address.cpp create mode 100644 servatrice/src/forwarded_address.h create mode 100644 tests/forwarded_address_test.cpp diff --git a/servatrice/CMakeLists.txt b/servatrice/CMakeLists.txt index 4db1683d0..255469934 100644 --- a/servatrice/CMakeLists.txt +++ b/servatrice/CMakeLists.txt @@ -7,6 +7,7 @@ project(Servatrice VERSION "${PROJECT_VERSION_MAJOR}.${PROJECT_VERSION_MINOR}.${ set(servatrice_SOURCES src/email_parser.cpp src/event_loop_watchdog.cpp + src/forwarded_address.cpp src/main.cpp src/metrics_registry.cpp src/ratelimiter.cpp diff --git a/servatrice/servatrice.ini.example b/servatrice/servatrice.ini.example index 63f8864ba..021f5157f 100644 --- a/servatrice/servatrice.ini.example +++ b/servatrice/servatrice.ini.example @@ -42,9 +42,11 @@ websocket_host=any websocket_port=4748 ; If websockets are served through a reverse proxy (e.g. nginx), the real client address can be -; read from an HTTP header such as X-Forwarded-For. The header is ONLY trusted when the websocket -; connection itself originates from an address listed in security/trusted_sources (default 127.0.0.1,::1), -; so that clients cannot spoof their address to bypass bans or rate limits. +; read from an HTTP header such as X-Real-IP or X-Forwarded-For. A comma-separated X-Forwarded-For +; list resolves to its rightmost entry, which is the one the proxy appended. The header is ONLY +; trusted when the websocket connection itself originates from an address listed in +; security/trusted_sources (default 127.0.0.1,::1), so that clients cannot spoof their address to +; bypass bans or rate limits. web_socket_ip_header= ; When database is enabled, servatrice writes the server status in the "update" database table; this diff --git a/servatrice/src/forwarded_address.cpp b/servatrice/src/forwarded_address.cpp new file mode 100644 index 000000000..8d5d3f4c1 --- /dev/null +++ b/servatrice/src/forwarded_address.cpp @@ -0,0 +1,21 @@ +#include "forwarded_address.h" + +#include + +QHostAddress parseForwardedAddress(const QByteArray &headerValue) +{ + const QList entries = headerValue.split(','); + for (auto it = entries.crbegin(); it != entries.crend(); ++it) { + const QByteArray entry = it->trimmed(); + if (entry.isEmpty()) { + continue; + } + + const QHostAddress parsed(QString::fromLatin1(entry)); + if (!parsed.isNull()) { + return parsed; + } + } + + return {}; +} diff --git a/servatrice/src/forwarded_address.h b/servatrice/src/forwarded_address.h new file mode 100644 index 000000000..d77e041bd --- /dev/null +++ b/servatrice/src/forwarded_address.h @@ -0,0 +1,18 @@ +#ifndef FORWARDED_ADDRESS_H +#define FORWARDED_ADDRESS_H + +#include +#include + +/** + * @brief Parses the real client address out of a forwarded header value (X-Real-IP, X-Forwarded-For). + * + * A comma-separated list resolves to its rightmost usable entry. That entry was appended by the + * proxy closest to the server and is the only one a direct client cannot choose, since the earlier + * entries are passed through verbatim from whatever the client sent. + * + * @return the parsed address, or a null QHostAddress when the value holds none. + */ +QHostAddress parseForwardedAddress(const QByteArray &headerValue); + +#endif diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index 77ddff1fc..b6eaf11f4 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -1551,8 +1551,11 @@ bool Servatrice::isRateLimitExempt(const QString &ipAddress) const { // Mirrors the max_users_per_address bypass in initSession: trusted sources // (e.g. 127.0.0.1 for Webatrice behind local nginx, where every websocket - // client shares one address) are not subject to per-address limits. - return isTrustedSource(getTrustedSources(), ipAddress); + // client shares one address) are not subject to per-address limits. Loopback + // is always exempt, so a same-host proxy whose forwarded address could not be + // resolved cannot lock every user out together when trusted_sources is + // customized. + return QHostAddress(ipAddress).isLoopback() || isTrustedSource(getTrustedSources(), ipAddress); } void Servatrice::clearFailedLogins(const QString &ipAddress, const QString &userName) diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index f8b84b6c8..890a93fbc 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -22,6 +22,7 @@ #include "deck_tag_serialization.h" #include "email_parser.h" +#include "forwarded_address.h" #include "libcockatrice/protocol/pb/admin_commands.pb.h" #include "libcockatrice/protocol/pb/developer_commands.pb.h" #include "libcockatrice/protocol/pb/moderator_commands.pb.h" @@ -4560,8 +4561,7 @@ void WebsocketServerSocketInterface::initConnection(void *_socket) // Only trust the forwarded header if the connection itself comes from a trusted proxy, // otherwise a client could spoof its address to bypass bans and rate limits. if (isTrustedSource(servatrice->getTrustedSources(), socket->peerAddress())) { - QString header(socket->request().rawHeader(websocketIPHeader)); - QHostAddress parsed(header); + const QHostAddress parsed = parseForwardedAddress(socket->request().rawHeader(websocketIPHeader)); if (!parsed.isNull()) { address = parsed; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index dfc2cb3f3..8811b8623 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -208,6 +208,17 @@ cockatrice_add_test( LABELS server ) +cockatrice_add_test( + NAME + forwarded_address_test + SOURCES + ${CMAKE_SOURCE_DIR}/servatrice/src/forwarded_address.cpp + forwarded_address_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/servatrice/src + LABELS + server +) cockatrice_add_test( NAME trusted_sources_test diff --git a/tests/forwarded_address_test.cpp b/tests/forwarded_address_test.cpp new file mode 100644 index 000000000..3ae2bc471 --- /dev/null +++ b/tests/forwarded_address_test.cpp @@ -0,0 +1,37 @@ +#include "forwarded_address.h" + +#include "gtest/gtest.h" + +namespace +{ + +TEST(ForwardedAddressTest, ParsesSingleAddress) +{ + EXPECT_EQ(QHostAddress("203.0.113.7"), parseForwardedAddress("203.0.113.7")); + EXPECT_EQ(QHostAddress("::1"), parseForwardedAddress("::1")); + EXPECT_EQ(QHostAddress("203.0.113.7"), parseForwardedAddress(" 203.0.113.7 ")); +} + +TEST(ForwardedAddressTest, UsesRightmostEntryOfAList) +{ + // nginx's $proxy_add_x_forwarded_for appends its own view of the client to + // whatever the client sent, so only the trailing entry is trustworthy. + EXPECT_EQ(QHostAddress("203.0.113.7"), parseForwardedAddress("198.51.100.9, 203.0.113.7")); + EXPECT_EQ(QHostAddress("203.0.113.7"), parseForwardedAddress("spoofed.invalid, 203.0.113.7")); + EXPECT_EQ(QHostAddress("203.0.113.7"), parseForwardedAddress("203.0.113.7, ")); +} + +TEST(ForwardedAddressTest, KeepsIpv4MappedEntries) +{ + EXPECT_EQ(QHostAddress("::ffff:203.0.113.7"), parseForwardedAddress("::ffff:203.0.113.7")); +} + +TEST(ForwardedAddressTest, RejectsValuesWithoutAnAddress) +{ + EXPECT_TRUE(parseForwardedAddress("").isNull()); + EXPECT_TRUE(parseForwardedAddress(",").isNull()); + EXPECT_TRUE(parseForwardedAddress("unknown").isNull()); + EXPECT_TRUE(parseForwardedAddress("garbage, more garbage").isNull()); +} + +} // namespace