fix(gameserver): percent-encode ServerDB URI query values (#41)

justfile

@@ -279,7 +279,7 @@ test-auth-unit:
     unset VCPKG_ROOT
     cmake --preset {{ preset }} -DBUILD_TESTING=ON > /dev/null 2>&1 \
         || cmake --preset {{ preset }} -DBUILD_TESTING=ON
-    cmake --build --preset {{ preset }} --target test_xpid_patch --target test_parse_endpoint --target test_behavioral --target test_token_auth --target test_messages --target test_crash_recovery --target test_nevr_config --target test_service_map --target test_social_facade --target test_scenario_early_quit --target test_early_quit_lockout --target test_schannel_cred_guard --target test_hooking --target test_plugin_load_plan --target test_system_module_loader --target test_login_redirect_override --target test_websocket_frame --target test_protobuf_transport --target test_websocket_client_auth --target test_url_diagnostics --target test_winhttp_stub --target test_callback_unregistration --target test_server_context --target test_session_unregister --target test_mic_lifecycle --target test_telemetry_snapshot_store --target test_coop_ai_trace
+    cmake --build --preset {{ preset }} --target test_xpid_patch --target test_parse_endpoint --target test_behavioral --target test_token_auth --target test_messages --target test_crash_recovery --target test_nevr_config --target test_service_map --target test_social_facade --target test_scenario_early_quit --target test_early_quit_lockout --target test_schannel_cred_guard --target test_hooking --target test_plugin_load_plan --target test_system_module_loader --target test_login_redirect_override --target test_websocket_frame --target test_protobuf_transport --target test_websocket_client_auth --target test_url_diagnostics --target test_serverdb_uri --target test_winhttp_stub --target test_callback_unregistration --target test_server_context --target test_session_unregister --target test_mic_lifecycle --target test_telemetry_snapshot_store --target test_coop_ai_trace
     cmake --build --preset {{ preset }} --target test_mic_dsp
     cmake --build --preset {{ preset }} --target test_game_image_guard
     bin="build/{{ preset }}/bin/test_xpid_patch.exe"
@@ -384,7 +384,7 @@ test-auth-unit:
         exit 1
     fi
     wine "$bin"
-    for test_name in test_system_module_loader test_login_redirect_override test_websocket_frame test_protobuf_transport test_websocket_client_auth test_url_diagnostics test_winhttp_stub test_callback_unregistration test_server_context test_session_unregister test_mic_lifecycle test_telemetry_snapshot_store test_coop_ai_trace; do
+    for test_name in test_system_module_loader test_login_redirect_override test_websocket_frame test_protobuf_transport test_websocket_client_auth test_url_diagnostics test_serverdb_uri test_winhttp_stub test_callback_unregistration test_server_context test_session_unregister test_mic_lifecycle test_telemetry_snapshot_store test_coop_ai_trace; do
         bin="build/{{ preset }}/bin/${test_name}.exe"
         if [[ ! -f "$bin" ]]; then
             echo "ERROR: GTest binary not found: $bin" >&2
@@ -1350,6 +1350,27 @@ verify:
         echo "identity must come from the presented credential or config, never the binary." >&2
         exit 1
     fi
+    # #41: config credentials reach Nakama in a URL query from two places — the
+    # ServerDB registration URI (server/gameserver.cpp) and the bridge's config/
+    # login connections (compat/ws_bridge.cpp). Both must go through the
+    # percent-encoder in server/serverdb_uri.cpp. A raw append lets a password
+    # with '&', '#', '%' or '+' rewrite the query, and if only one site encodes,
+    # the two paths send different passwords for the same account.
+    # Falsified 2026-10-05 against 323352b: the pattern hits gameserver.cpp:1433
+    # and ws_bridge.cpp:1002-1003; on the fixed tree it exits 1.
+    I41_RC=0; I41_HITS=$(grep -nE '[?&]password=%s|\+= *cfgPassword|"&password="' \
+        src/runtime/server/gameserver.cpp src/runtime/compat/ws_bridge.cpp) || I41_RC=$?
+    sensor_stage1 "#41 raw URL credential" "server/gameserver.cpp compat/ws_bridge.cpp" "$I41_RC"
+    if [ "$I41_RC" -eq 0 ]; then
+        printf '%s\n' "$I41_HITS" >&2
+        echo "verify: FAIL — #41 a credential is concatenated into a URL unencoded; use ServerDbUri (server/serverdb_uri.h)." >&2
+        exit 1
+    fi
+    if ! grep -q 'ServerDbUri::BuildLegacyUri(' src/runtime/server/gameserver.cpp \
+       || ! grep -q 'ServerDbUri::BuildBridgeCredentialUri(' src/runtime/compat/ws_bridge.cpp; then
+        echo "verify: FAIL — #41 a URL-credential site no longer calls the ServerDbUri encoder." >&2
+        exit 1
+    fi
     # N20 (owner decision, 2026-07-27): the nevr_discord_id config fallback applies
     # in CLIENT mode too, not only server mode. Two assertions, because either one
     # alone is satisfiable by the bug — the first fires if the fallback is deleted,

src/runtime/CMakeLists.txt

@@ -58,6 +58,7 @@ set(PATCHES_SOURCES
     "server/session_success_dispatch.cpp"
     "server/callback_unregistration.cpp"
     "server/session_unregister.cpp"
+    "server/serverdb_uri.cpp"
     "server/telemetry_streamer.cpp"
     "server/upnp.cpp"
 )
@@ -119,6 +120,7 @@ set(PATCHES_HEADERS
     "server/session_success_dispatch.h"
     "server/callback_unregistration.h"
     "server/session_unregister.h"
+    "server/serverdb_uri.h"
     "server/telemetry_streamer.h"
     "server/messages.h"
     "server/upnp.h"
@@ -174,7 +176,7 @@ set_source_files_properties(ext/plugin_load_plan.cpp PROPERTIES SKIP_PRECOMPILE_
 # #60: plugin_manifest.cpp builds the login's nevr_plugins array with nlohmann-json;
 # pure and PCH-free for the same reason as plugin_load_plan.cpp.
 set_source_files_properties(ext/plugin_manifest.cpp PROPERTIES SKIP_PRECOMPILE_HEADERS ON)
-set_source_files_properties(log/url_diagnostics.cpp server/websocket_client.cpp server/websocket_frame.cpp server/protobuf_transport.cpp server/session_success_dispatch.cpp server/callback_unregistration.cpp server/session_unregister.cpp server/telemetry_streamer.cpp server/upnp.cpp server/gameserver.cpp server/messages.cpp PROPERTIES SKIP_PRECOMPILE_HEADERS ON)
+set_source_files_properties(log/url_diagnostics.cpp server/websocket_client.cpp server/websocket_frame.cpp server/protobuf_transport.cpp server/session_success_dispatch.cpp server/callback_unregistration.cpp server/session_unregister.cpp server/serverdb_uri.cpp server/telemetry_streamer.cpp server/upnp.cpp server/gameserver.cpp server/messages.cpp PROPERTIES SKIP_PRECOMPILE_HEADERS ON)
 
 # Scenario-test control endpoint (docs/design/2026-10-01-social-scenario-harness.md). It can inject
 # messages into a live session, so it is OFF by default and only the mingw-scenario preset turns it
@@ -333,6 +335,16 @@ if(BUILD_TESTING)
     target_link_libraries(test_url_diagnostics PRIVATE GTest::gtest GTest::gtest_main CURL::libcurl)
     gtest_discover_tests(test_url_diagnostics DISCOVERY_MODE PRE_TEST)
 
+    # Issue #41: the ServerDB URI builder (percent-encoded query) — pure, curl only.
+    add_executable(test_serverdb_uri
+      tests/test_serverdb_uri.cpp
+      server/serverdb_uri.cpp)
+    set_source_files_properties(tests/test_serverdb_uri.cpp server/serverdb_uri.cpp
+      PROPERTIES SKIP_PRECOMPILE_HEADERS ON)
+    target_include_directories(test_serverdb_uri PRIVATE ${CMAKE_SOURCE_DIR}/src)
+    target_link_libraries(test_serverdb_uri PRIVATE GTest::gtest GTest::gtest_main CURL::libcurl)
+    gtest_discover_tests(test_serverdb_uri DISCOVERY_MODE PRE_TEST)
+
     # GH #27: drives the REAL libcurl-backed IWinHttpRequest stub through its COM
     # vtable and IDispatch::Invoke against a loopback HTTP/1.1 listener, so the
     # StatusText it reports is the reason phrase the server actually sent.
@@ -564,6 +576,8 @@ if(BUILD_TESTING)
       ext/module_loader.cpp
       log/url_diagnostics.cpp
       compat/ws_bridge.cpp
+      # #41: ws_bridge.cpp builds its URL credentials through the ServerDbUri encoder.
+      server/serverdb_uri.cpp
       hook/symbol_corpus.cpp
       # N84: plugin_loader calls HookGuard::VerifyAll after each plugin init.
       # Linked as production source (not a stub) so the test drives the real
@@ -593,6 +607,7 @@ if(BUILD_TESTING)
       ext/module_loader.cpp
       log/url_diagnostics.cpp
       compat/ws_bridge.cpp
+      server/serverdb_uri.cpp
       hook/symbol_corpus.cpp
       patch/broadcaster_hook_stats.cpp
       hook/hook_liveness.cpp

src/runtime/compat/ws_bridge.cpp

@@ -36,6 +36,7 @@
 #include "runtime/lifecycle/service_config.h"  // NevrCfgGetFlat (N133 S4a: config.yaml reads)
 #include "runtime/log/url_diagnostics.h"
 #include "runtime/log/security_diagnostics.h"
+#include "runtime/server/serverdb_uri.h"
 #include "core/logging.h"
 #include <exception>
 #include <stdexcept>
@@ -991,16 +992,23 @@ void InstallWebSocketBridge() {
             // password just means "no URL credentials" — we fall through to the
             // Bearer/JWT path and never put an empty secret on the wire (N115).
             // The password value is never logged.
+            // Issue #41: both values are percent-encoded (ServerDbUri, the same
+            // encoder the ServerDB registration URI uses), so a password with
+            // '&', '=', '#', '%', '+' or whitespace reaches Nakama byte-for-byte.
             {
               const char* cfgDiscordId = NevrCfgGetFlat("nevr_discord_id");
               const char* cfgPassword = NevrCfgGetFlat("nevr_password");
-              if (cfgDiscordId && cfgDiscordId[0] != '\0' && cfgPassword && cfgPassword[0] != '\0') {
-                char sep = (remoteUrl.find('?') != std::string::npos) ? '&' : '?';
-                remoteUrl += sep;
-                remoteUrl += "discordid=";
-                remoteUrl += cfgDiscordId;
-                remoteUrl += "&password=";
-                remoteUrl += cfgPassword;
+              std::optional<std::string> withCredentials = ServerDbUri::BuildBridgeCredentialUri(
+                  remoteUrl, cfgDiscordId ? std::string_view(cfgDiscordId) : std::string_view(),
+                  cfgPassword ? std::string_view(cfgPassword) : std::string_view());
+              if (withCredentials) {
+                remoteUrl = std::move(*withCredentials);
+              } else {
+                // Allocation failure in the encoder: connect without URL credentials
+                // (Bearer path below) rather than put an unencoded secret on the wire.
+                Log(EchoVR::LogLevel::Error,
+                    "[NEVR.WS] conn=%d could not percent-encode URL credentials; connecting without them",
+                    connIdx);
               }
             }
             // conn>=2 (matchmaker): pnsradmatchmaking uses protobuf, not EchoVR

src/runtime/server/gameserver.cpp

@@ -14,6 +14,7 @@
 #include "auth_token_refresh.h"
 #include "runtime/server/constants.h"
 #include "runtime/server/protobuf_transport.h"
+#include "runtime/server/serverdb_uri.h"
 #include "runtime/server/session_success_dispatch.h"
 #include "runtime/server/session_unregister.h"
 #include "runtime/server/callback_unregistration.h"
@@ -1410,8 +1411,11 @@ VOID GameServerLib::RequestRegistration(INT64 serverId, CHAR*, EchoVR::SymbolId
     ServerFatal("Server authentication failed — no valid token for ServerDB connection");
   }
 
-  thread_local static CHAR constructedUri[1024];
+  // Owns the constructed URI for the rest of this call; Connect() copies it
+  // (websocket_client.cpp setUrl(std::string(uri))).
+  std::string constructedUri;
   if (!serverDbUri || serverDbUri[0] == '\0') {
+    const auto orEmpty = [](const char* value) { return value ? std::string_view(value) : std::string_view(); };
     // guilds/regions are list-shaped registration metadata; read CSV so a yaml
     // list `[a, b]` and a scalar CSV both build the same guilds=/regions= param.
     const char* guilds = NevrCfgGetFlatCsv("nevr_guilds");
@@ -1425,17 +1429,17 @@ VOID GameServerLib::RequestRegistration(INT64 serverId, CHAR*, EchoVR::SymbolId
       // Token auth (BAC-2): identity via the Bearer JWT (sent by Connect()); discord_id/
       // password dropped; guilds/regions stay as registration metadata. nevr_serverdb_uri
       // points at the token route that forwards the real Authorization header
-      // (docs/guides/token-auth-migration.md).
-      int written = snprintf(constructedUri, sizeof(constructedUri), "%s", tokenUri);
-      const char* sep = "?";
-      if (guilds && guilds[0] != '\0' && written > 0 && written < (int)sizeof(constructedUri)) {
-        written += snprintf(constructedUri + written, sizeof(constructedUri) - written, "%sguilds=%s", sep, guilds);
-        sep = "&";
+      // (29ce275710ad4c779d118eafc638fef613343e28:docs/guides/token-auth-migration.md).
+      // Issue #41: query values are percent-encoded by ServerDbUri, not snprintf.
+      std::optional<std::string> built =
+          ServerDbUri::BuildTokenRouteUri(tokenUri, orEmpty(guilds), orEmpty(regions));
+      if (!built) {
+        Log(EchoVR::LogLevel::Error, "[NEVR.GAMESERVER] could not percent-encode the token-route serverdb URI");
+        ServerFatal("Could not build the ServerDB URI (token route)");
+        return;
       }
-      if (regions && regions[0] != '\0' && written > 0 && written < (int)sizeof(constructedUri)) {
-        snprintf(constructedUri + written, sizeof(constructedUri) - written, "%sregions=%s", sep, regions);
-      }
-      serverDbUri = constructedUri;
+      constructedUri = std::move(*built);
+      serverDbUri = constructedUri.c_str();
       const std::string diagnostic = LogDiagnostics::FormatRedactedUrlDiagnostic(
           "[NEVR.GAMESERVER] constructed serverdb URI for token auth: ", constructedUri);
       Log(EchoVR::LogLevel::Debug, "%s", diagnostic.c_str());
@@ -1446,24 +1450,23 @@ VOID GameServerLib::RequestRegistration(INT64 serverId, CHAR*, EchoVR::SymbolId
       const char* discordId = NevrCfgGetFlat("nevr_discord_id");
       const char* password = NevrCfgGetFlat("nevr_password");
       if (socketUri && socketUri[0] != '\0' && discordId && discordId[0] != '\0') {
-        int written = 0;
-        if (password && password[0] != '\0') {
-          written = snprintf(constructedUri, sizeof(constructedUri), "%s?discord_id=%s&password=%s", socketUri, discordId, password);
-        } else {
-          written = snprintf(constructedUri, sizeof(constructedUri), "%s?discord_id=%s", socketUri, discordId);
-        }
-        if (guilds && guilds[0] != '\0' && written > 0 && written < (int)sizeof(constructedUri)) {
-          written += snprintf(constructedUri + written, sizeof(constructedUri) - written, "&guilds=%s", guilds);
-        }
-        if (regions && regions[0] != '\0' && written > 0 && written < (int)sizeof(constructedUri)) {
-          snprintf(constructedUri + written, sizeof(constructedUri) - written, "&regions=%s", regions);
+        // Issue #41: every value is percent-encoded, so a password containing
+        // '&', '=', '#', '%', '+' or whitespace can no longer rewrite the query.
+        std::optional<std::string> built = ServerDbUri::BuildLegacyUri(
+            socketUri, discordId, orEmpty(password), orEmpty(guilds), orEmpty(regions));
+        if (!built) {
+          Log(EchoVR::LogLevel::Error, "[NEVR.GAMESERVER] could not percent-encode the legacy serverdb URI");
+          ServerFatal("Could not build the ServerDB URI (legacy url-param auth)");
+          return;
         }
-        serverDbUri = constructedUri;
+        constructedUri = std::move(*built);
+        serverDbUri = constructedUri.c_str();
         // Do NOT log constructedUri here — this branch embeds the operator's
-        // password directly in the query string (see snprintf above).
+        // password in the query string. Presence only, never the value.
         Log(EchoVR::LogLevel::Debug,
-            "[NEVR.GAMESERVER] constructed serverdb URI (legacy url-param auth): discord_id=%s (password redacted)",
-            discordId);
+            "[NEVR.GAMESERVER] constructed serverdb URI (legacy url-param auth, percent-encoded): "
+            "discord_id=%s password=%s",
+            discordId, (password && password[0] != '\0') ? "present (redacted)" : "absent");
       } else {
         serverDbUri = "ws://localhost:777/serverdb";
         const std::string diagnostic = LogDiagnostics::FormatRedactedUrlDiagnostic(

src/runtime/server/serverdb_uri.cpp

@@ -0,0 +1,87 @@
+#include "runtime/server/serverdb_uri.h"
+
+#include <curl/curl.h>
+
+#include <initializer_list>
+#include <limits>
+#include <memory>
+
+namespace ServerDbUri {
+namespace {
+
+struct QueryParam {
+  std::string_view key;  // literal, RFC 3986 unreserved; appended as-is
+  std::string_view value;
+  bool isList;  // comma-separated: encode each element, keep the commas
+};
+
+std::optional<std::string> EncodeList(std::string_view csv) {
+  std::string encoded;
+  size_t start = 0;
+  while (true) {
+    const size_t comma = csv.find(',', start);
+    const std::string_view element =
+        csv.substr(start, comma == std::string_view::npos ? std::string_view::npos : comma - start);
+    const std::optional<std::string> encodedElement = EncodeQueryValue(element);
+    if (!encodedElement) return std::nullopt;
+    encoded += *encodedElement;
+    if (comma == std::string_view::npos) break;
+    encoded += ',';
+    start = comma + 1;
+  }
+  return encoded;
+}
+
+std::optional<std::string> AppendQuery(std::string_view base, std::initializer_list<QueryParam> params) {
+  std::string uri(base);
+  for (const QueryParam& param : params) {
+    if (param.value.empty()) continue;
+    const std::optional<std::string> encoded = param.isList ? EncodeList(param.value) : EncodeQueryValue(param.value);
+    if (!encoded) return std::nullopt;
+    if (uri.find('?') == std::string::npos) {
+      uri += '?';
+    } else if (uri.back() != '?' && uri.back() != '&') {
+      uri += '&';
+    }
+    uri.append(param.key);
+    uri += '=';
+    uri += *encoded;
+  }
+  return uri;
+}
+
+}  // namespace
+
+std::optional<std::string> EncodeQueryValue(std::string_view value) {
+  // curl_easy_escape treats length 0 as "call strlen", which would read past a
+  // non-terminated string_view — so the empty case never reaches it.
+  if (value.empty()) return std::string();
+  if (value.size() > static_cast<size_t>((std::numeric_limits<int>::max)())) return std::nullopt;
+  // A null handle is accepted since libcurl 7.82.0 (vcpkg ships 8.18.0).
+  std::unique_ptr<char, decltype(&curl_free)> escaped(
+      curl_easy_escape(nullptr, value.data(), static_cast<int>(value.size())), &curl_free);
+  if (!escaped) return std::nullopt;
+  return std::string(escaped.get());
+}
+
+std::optional<std::string> BuildLegacyUri(std::string_view socketUri, std::string_view discordId,
+                                          std::string_view password, std::string_view guilds,
+                                          std::string_view regions) {
+  return AppendQuery(socketUri, {{"discord_id", discordId, false},
+                                 {"password", password, false},
+                                 {"guilds", guilds, true},
+                                 {"regions", regions, true}});
+}
+
+std::optional<std::string> BuildTokenRouteUri(std::string_view tokenUri, std::string_view guilds,
+                                              std::string_view regions) {
+  return AppendQuery(tokenUri, {{"guilds", guilds, true}, {"regions", regions, true}});
+}
+
+std::optional<std::string> BuildBridgeCredentialUri(std::string_view remoteUri, std::string_view discordId,
+                                                    std::string_view password) {
+  if (discordId.empty() || password.empty()) return std::string(remoteUri);
+  return AppendQuery(remoteUri, {{"discordid", discordId, false}, {"password", password, false}});
+}
+
+}  // namespace ServerDbUri