From 46cdbac830b69584bdc3bb61998c401c5eb28d43 Mon Sep 17 00:00:00 2001 From: Zach H Date: Sun, 18 Oct 2026 16:50:04 -0400 Subject: [PATCH] [Servatrice] Match trusted_sources by address, not substring (#7582) trusted_sources was checked with QString::contains. A dual-stack listener reports a local proxy as ::ffff:127.0.0.1, which never matches the default "127.0.0.1,::1". Behind nginx the forwarded client IP header was then ignored, every websocket user shared the proxy's address, and logins failed with "too many concurrent connections" once max_users_per_address was reached. Add isTrustedSource(), which parses each entry as a QHostAddress and compares exact addresses with IPv4-mapped addresses normalized, and use it at all four call sites. The ban command also read server/trusted_sources instead of the documented security/trusted_sources; it now uses the same getter. Entries that are not IP addresses are logged at startup since they no longer match anything. Co-authored-by: Claude Opus 5.5 --- servatrice/CMakeLists.txt | 1 + servatrice/servatrice.ini.example | 3 +- servatrice/src/servatrice.cpp | 17 ++++++- servatrice/src/servatrice.h | 2 + servatrice/src/serversocketinterface.cpp | 10 ++-- servatrice/src/trusted_sources.cpp | 23 ++++++++++ servatrice/src/trusted_sources.h | 20 ++++++++ tests/CMakeLists.txt | 11 +++++ tests/trusted_sources_test.cpp | 58 ++++++++++++++++++++++++ 9 files changed, 136 insertions(+), 9 deletions(-) create mode 100644 servatrice/src/trusted_sources.cpp create mode 100644 servatrice/src/trusted_sources.h create mode 100644 tests/trusted_sources_test.cpp diff --git a/servatrice/CMakeLists.txt b/servatrice/CMakeLists.txt index f6d28d6c1..4db1683d0 100644 --- a/servatrice/CMakeLists.txt +++ b/servatrice/CMakeLists.txt @@ -18,6 +18,7 @@ set(servatrice_SOURCES src/settingscache.cpp src/isl_interface.cpp src/signalhandler.cpp + src/trusted_sources.cpp ${VERSION_STRING_CPP} src/smtpclient.cpp src/smtp/qxthmac.cpp diff --git a/servatrice/servatrice.ini.example b/servatrice/servatrice.ini.example index 68478810b..63f8864ba 100644 --- a/servatrice/servatrice.ini.example +++ b/servatrice/servatrice.ini.example @@ -465,7 +465,8 @@ legacy_backup_ttl_days=180 ; You may want to allow an unlimited number of users from a trusted source. This setting can contain a ; comma-separed list of IP addresses which will allow an unlimited number of connections from each of the -; IP addresses listed (ignoring the max_users_per_address). Default is "127.0.0.1,::1"; example: "192.73.233.244,81.4.100.74" +; IP addresses listed (ignoring the max_users_per_address). Entries must be exact IP addresses; hostnames, prefixes +; and subnets are ignored. Default is "127.0.0.1,::1"; example: "192.73.233.244,81.4.100.74" trusted_sources="127.0.0.1,::1" ; Servatrice can avoid users from flooding rooms with large number of messages in an interval of time. diff --git a/servatrice/src/servatrice.cpp b/servatrice/src/servatrice.cpp index a306de523..77ddff1fc 100644 --- a/servatrice/src/servatrice.cpp +++ b/servatrice/src/servatrice.cpp @@ -33,6 +33,7 @@ #include "serversocketinterface.h" #include "settingscache.h" #include "smtpclient.h" +#include "trusted_sources.h" #include #include @@ -324,6 +325,14 @@ bool Servatrice::initServer() qDebug() << "Maximum websocket user limit:" << getMaxWebSocketUserLimit(); } + // trusted_sources entries are matched as exact addresses; older configs may hold hostnames, + // prefixes or subnets that substring matching used to accept and that now never match. + for (const QString &entry : getTrustedSources().split(',', Qt::SkipEmptyParts)) { + if (QHostAddress(entry.trimmed()).isNull()) { + qWarning() << "security/trusted_sources entry is not an IP address and will be ignored:" << entry.trimmed(); + } + } + qDebug() << "Accept registered users only:" << getRegOnlyServerEnabled(); qDebug() << "Registration enabled:" << getRegistrationEnabled(); if (getRegistrationEnabled()) { @@ -1324,6 +1333,11 @@ int Servatrice::getMaxUsersPerAddress() const return settingsCache->value("security/max_users_per_address", 4).toInt(); } +QString Servatrice::getTrustedSources() const +{ + return settingsCache->value("security/trusted_sources", "127.0.0.1,::1").toString(); +} + int Servatrice::getMessageCountingInterval() const { return settingsCache->value("security/message_counting_interval", 10).toInt(); @@ -1538,8 +1552,7 @@ 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. - const QString trustedSources = settingsCache->value("security/trusted_sources", "127.0.0.1,::1").toString(); - return trustedSources.contains(ipAddress, Qt::CaseInsensitive); + return isTrustedSource(getTrustedSources(), ipAddress); } void Servatrice::clearFailedLogins(const QString &ipAddress, const QString &userName) diff --git a/servatrice/src/servatrice.h b/servatrice/src/servatrice.h index 119adc8b2..3dba23234 100644 --- a/servatrice/src/servatrice.h +++ b/servatrice/src/servatrice.h @@ -321,6 +321,8 @@ public: int getDeckShareMaxDecksPerShare() const; int getDeckShareMaxSharesPerDay() const; int getMaxUsersPerAddress() const; + /// Raw security/trusted_sources list; match addresses against it with isTrustedSource(). + QString getTrustedSources() const; int getMessageCountingInterval() const override; int getMaxMessageCountPerInterval() const override; int getMaxMessageSizePerInterval() const override; diff --git a/servatrice/src/serversocketinterface.cpp b/servatrice/src/serversocketinterface.cpp index b24126d93..f8b84b6c8 100644 --- a/servatrice/src/serversocketinterface.cpp +++ b/servatrice/src/serversocketinterface.cpp @@ -42,6 +42,7 @@ #include "server_logger.h" #include "server_protocolhandler.h" #include "settingscache.h" +#include "trusted_sources.h" #include "version_string.h" #include @@ -220,8 +221,7 @@ bool AbstractServerSocketInterface::initSession() delete identSe; // allow unlimited number of connections from the trusted sources - QString trustedSources = settingsCache->value("security/trusted_sources", "127.0.0.1,::1").toString(); - if (trustedSources.contains(getAddress(), Qt::CaseInsensitive)) { + if (isTrustedSource(servatrice->getTrustedSources(), getPeerAddress())) { return true; } @@ -1908,9 +1908,8 @@ Response::ResponseCode AbstractServerSocketInterface::cmdBanFromServer(const Com if (amountRemove != 0) { removeSaidMessages(userName, amountRemove); } - QString trustedSources = settingsCache->value("server/trusted_sources", "127.0.0.1,::1").toString(); int minutes = cmd.minutes(); - if (trustedSources.contains(address, Qt::CaseInsensitive)) { + if (isTrustedSource(servatrice->getTrustedSources(), address)) { address = ""; } @@ -4560,8 +4559,7 @@ void WebsocketServerSocketInterface::initConnection(void *_socket) if (websocketIPHeader.length() > 0 && socket->request().hasRawHeader(websocketIPHeader)) { // 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. - QString trustedSources = settingsCache->value("security/trusted_sources", "127.0.0.1,::1").toString(); - if (trustedSources.contains(socket->peerAddress().toString(), Qt::CaseInsensitive)) { + if (isTrustedSource(servatrice->getTrustedSources(), socket->peerAddress())) { QString header(socket->request().rawHeader(websocketIPHeader)); QHostAddress parsed(header); if (!parsed.isNull()) { diff --git a/servatrice/src/trusted_sources.cpp b/servatrice/src/trusted_sources.cpp new file mode 100644 index 000000000..327491936 --- /dev/null +++ b/servatrice/src/trusted_sources.cpp @@ -0,0 +1,23 @@ +#include "trusted_sources.h" + +#include + +bool isTrustedSource(const QString &trustedSources, const QHostAddress &address) +{ + if (address.isNull()) { + return false; + } + const QStringList entries = trustedSources.split(',', Qt::SkipEmptyParts); + for (const QString &entry : entries) { + const QHostAddress trusted(entry.trimmed()); + if (!trusted.isNull() && trusted.isEqual(address, QHostAddress::ConvertV4MappedToIPv4)) { + return true; + } + } + return false; +} + +bool isTrustedSource(const QString &trustedSources, const QString &address) +{ + return isTrustedSource(trustedSources, QHostAddress(address.trimmed())); +} diff --git a/servatrice/src/trusted_sources.h b/servatrice/src/trusted_sources.h new file mode 100644 index 000000000..3aea55e41 --- /dev/null +++ b/servatrice/src/trusted_sources.h @@ -0,0 +1,20 @@ +#ifndef TRUSTED_SOURCES_H +#define TRUSTED_SOURCES_H + +#include +#include + +/** + * @brief True if @p address is listed in the comma-separated @p trustedSources (security/trusted_sources). + * + * Entries are parsed as addresses and compared exactly, never as substrings, so + * 27.0.0.1 does not match 127.0.0.1. IPv4-mapped IPv6 addresses compare equal to + * their IPv4 form: a dual-stack listener reports a local proxy as ::ffff:127.0.0.1, + * which matches a 127.0.0.1 entry. Malformed entries and null addresses never match. + */ +bool isTrustedSource(const QString &trustedSources, const QHostAddress &address); + +/** @brief Overload for an address in string form; an unparsable string never matches. */ +bool isTrustedSource(const QString &trustedSources, const QString &address); + +#endif diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b58942f3f..dfc2cb3f3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -208,6 +208,17 @@ cockatrice_add_test( LABELS server ) +cockatrice_add_test( + NAME + trusted_sources_test + SOURCES + ${CMAKE_SOURCE_DIR}/servatrice/src/trusted_sources.cpp + trusted_sources_test.cpp + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/servatrice/src + LABELS + server +) cockatrice_add_test( NAME server_card_counter_test diff --git a/tests/trusted_sources_test.cpp b/tests/trusted_sources_test.cpp new file mode 100644 index 000000000..a16b21f2f --- /dev/null +++ b/tests/trusted_sources_test.cpp @@ -0,0 +1,58 @@ +#include "trusted_sources.h" + +#include "gtest/gtest.h" +#include + +namespace +{ + +const QString kDefault = QStringLiteral("127.0.0.1,::1"); + +TEST(TrustedSourcesTest, MatchesListedAddresses) +{ + ASSERT_TRUE(isTrustedSource(kDefault, QHostAddress("127.0.0.1"))); + ASSERT_TRUE(isTrustedSource(kDefault, QHostAddress("::1"))); + ASSERT_FALSE(isTrustedSource(kDefault, QHostAddress("10.0.0.1"))); +} + +TEST(TrustedSourcesTest, MatchesIpv4MappedPeerAgainstPlainIpv4Entry) +{ + // A dual-stack listener reports a local IPv4 proxy as ::ffff:127.0.0.1; + // it must still match the default 127.0.0.1 entry. + ASSERT_TRUE(isTrustedSource(kDefault, QHostAddress("::ffff:127.0.0.1"))); +} + +TEST(TrustedSourcesTest, MatchesPlainIpv4PeerAgainstMappedEntry) +{ + ASSERT_TRUE(isTrustedSource("::ffff:192.0.2.7", QHostAddress("192.0.2.7"))); +} + +TEST(TrustedSourcesTest, DoesNotMatchSubstrings) +{ + // Substring matching would let 27.0.0.1 or 0.0.1 pass as 127.0.0.1. + ASSERT_FALSE(isTrustedSource(kDefault, QHostAddress("27.0.0.1"))); + ASSERT_FALSE(isTrustedSource("192.0.2.10", QHostAddress("192.0.2.1"))); + ASSERT_FALSE(isTrustedSource(kDefault, QHostAddress(":"))); +} + +TEST(TrustedSourcesTest, ToleratesWhitespaceAndCase) +{ + ASSERT_TRUE(isTrustedSource(" 192.0.2.7 , 2001:DB8::1 ", QHostAddress("2001:db8::1"))); + ASSERT_TRUE(isTrustedSource(" 192.0.2.7 , 2001:DB8::1 ", QHostAddress("192.0.2.7"))); +} + +TEST(TrustedSourcesTest, IgnoresMalformedEntriesAndNullAddresses) +{ + ASSERT_FALSE(isTrustedSource("not-an-ip,,", QHostAddress("127.0.0.1"))); + ASSERT_FALSE(isTrustedSource("", QHostAddress("127.0.0.1"))); + ASSERT_FALSE(isTrustedSource(kDefault, QHostAddress())); +} + +TEST(TrustedSourcesTest, StringOverloadParsesAddress) +{ + ASSERT_TRUE(isTrustedSource(kDefault, QStringLiteral("::ffff:127.0.0.1"))); + ASSERT_FALSE(isTrustedSource(kDefault, QStringLiteral("27.0.0.1"))); + ASSERT_FALSE(isTrustedSource(kDefault, QString())); +} + +} // namespace