feat(Server/Client): Add password authentication flow#10
feat(Server/Client): Add password authentication flow#10galadril wants to merge 8 commits intoifBars:masterfrom
Conversation
Co-authored-by: galadril <14561640+galadril@users.noreply.github.com>
Co-authored-by: galadril <14561640+galadril@users.noreply.github.com>
Co-authored-by: galadril <14561640+galadril@users.noreply.github.com>
Co-authored-by: galadril <14561640+galadril@users.noreply.github.com>
- Extract password dialog UI to dedicated PasswordDialog class - Reduce ClientUIManager from 1488 to 1155 lines (333 lines saved) - Remove duplicate GetUIManager() and GetPlayerManager() methods - Use static properties directly (UIManager, Players) instead of getter methods - Improves separation of concerns and maintainability Co-authored-by: galadril <14561640+galadril@users.noreply.github.com>
Enhanced disconnect logic to fully reset connection and loading state. Menu scene now forces disconnect and UI cleanup, including cursor and time scale resets. Password dialog always re-enables submit button and clears fields for reuse. Improved error handling ensures UI elements are reset even on exceptions. Added launch profile for running batch file.
📝 WalkthroughWalkthroughThis pull request introduces password-based authentication for dedicated servers. Changes span client-side password prompt UI, server-side validation logic, authentication message types, network routing, password hashing utilities, and build/launch scripts to support the complete authentication workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Game)
participant UI as PasswordDialog UI
participant Network as Message Router
participant Server as Server
participant Auth as PlayerAuth
Client->>Network: Connects to Server
Server->>Network: Player Connected
Network->>Server: SendAuthenticationChallenge
Server->>Network: auth_challenge Message
Network->>Client: auth_challenge Received
Client->>UI: ShowPasswordPrompt(serverName)
UI->>UI: Pause Game (timeScale=0)
UI->>UI: Show Password Input
rect rgba(100, 150, 200, 0.5)
Note over Client,UI: User enters password
UI->>UI: OnSubmit (Enter pressed)
end
UI->>Network: SendAuthenticationResponse(passwordHash)
Network->>Server: auth_response Message
Server->>Auth: AuthenticatePlayer(playerInfo, passwordHash)
alt Password Valid
Auth->>Auth: Validation Success
Server->>Network: SendAuthenticationResult(success=true)
Network->>Client: auth_result (success)
Client->>UI: OnAuthenticationSuccess()
UI->>Client: Resume Game (timeScale=1)
UI->>UI: HidePasswordPrompt()
else Password Invalid
Auth->>Auth: Validation Fails
Server->>Network: SendAuthenticationResult(success=false, error)
Network->>Client: auth_result (failure)
Client->>UI: ShowAuthenticationError(message)
UI->>UI: Show Error, Increment Retry
rect rgba(200, 100, 100, 0.5)
Note over UI: Delayed Disconnect (1.5s)
end
UI->>Client: DelayedDisconnect()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Client/Managers/ClientUIManager.cs (1)
46-72:⚠️ Potential issue | 🟡 MinorPrivate field should use
_prefix (_passwordDialog).
Repo style requires_-prefixed private fields; please rename and update usages.
As per coding guidelines: Use camelCase with_prefix for private/internal fields (e.g.,_logger,_config).🔧 Suggested rename (apply across file)
- private PasswordDialog passwordDialog; + private PasswordDialog _passwordDialog; ... - passwordDialog = new PasswordDialog(logger, connectionManager); + _passwordDialog = new PasswordDialog(logger, connectionManager); ... - passwordDialog?.HidePasswordPrompt(); + _passwordDialog?.HidePasswordPrompt(); ... - passwordDialog?.Update(); + _passwordDialog?.Update(); ... - if (passwordDialog != null && passwordDialog.IsVisible) + if (_passwordDialog != null && _passwordDialog.IsVisible) ... - passwordDialog.SetCapturedFont(capturedTmpFont); - passwordDialog.ShowPasswordPrompt(serverName); + _passwordDialog.SetCapturedFont(capturedTmpFont); + _passwordDialog.ShowPasswordPrompt(serverName);
🤖 Fix all issues with AI agents
In `@Client/Managers/ClientUIManager.cs`:
- Around line 1159-1202: Add full XML documentation to the new public UI
methods: ShowPasswordPrompt(string serverName), HidePasswordPrompt(),
IsPasswordDialogVisible(), ShowAuthenticationError(string errorMessage), and
OnAuthenticationSuccess(). For each method include <summary> (existing), <param>
entries for parameters (serverName, errorMessage), a <returns> tag for
IsPasswordDialogVisible describing the boolean result, <exception> tags if these
methods can throw (e.g., null reference if passwordDialog is null), a <remarks>
note about interaction with passwordDialog and capturedTmpFont, and a short
<example> showing typical usage (calling ShowPasswordPrompt then handling
OnAuthenticationSuccess/ShowAuthenticationError). Ensure tags follow the repo
standard and reference the passwordDialog field where appropriate.
In `@Client/Managers/PasswordDialog.cs`:
- Around line 474-479: The asteriskChar assignment for passwordInput currently
uses a malformed character ('�') likely due to encoding; update the
PasswordDialog.cs code that sets passwordInput.asteriskChar to use the Unicode
escape for a bullet (e.g. '\u25CF') so the intended bullet (●) displays reliably
across encodings, leaving the onSubmit listener (OnPasswordSubmit) unchanged.
- Around line 49-53: The PasswordDialog constructor currently doesn't validate
its public parameters; add null checks for the logger and connectionManager
parameters in the PasswordDialog(MelonLogger.Instance logger,
ClientConnectionManager connectionManager) constructor and throw
ArgumentNullException (with the parameter name) for any that are null so callers
get immediate, clear failures instead of later NREs.
- Around line 1-10: Wrap all client-only code in this file with a preprocessor
guard so it is excluded from server builds: add `#if` CLIENT at the top (before
the using directives that reference MelonLoader, TMPro, UnityEngine.UI,
UnityEngine, etc.) and a matching `#endif` at the end of the file (after the
namespace DedicatedServerMod.Client.Managers and the PasswordDialog class
declaration), ensuring all client-specific usings and the entire PasswordDialog
implementation are enclosed.
In `@Client/Patches/MessagingPatches.cs`:
- Around line 80-115: The coroutine SendDelayedClientMessages currently uses
WaitForSeconds and breaks/aborts on timeout which can hang or silently drop the
SendClientReadyMessage when Time.timeScale==0; change the loop to use
WaitForSecondsRealtime(0.1f) to use unscaled time (like PasswordDialog), remove
the break/early-exit behavior so the routine continues retrying until
DailySummary.Instance is spawned and IsSpawned is true, and instead emit a
single warning log if the timeout threshold is crossed (track a bool like
warnedOnce) while still attempting
SendClientReadyMessage()/RequestServerDataIfConnected() only when DailySummary
is confirmed spawned; keep references to DailySummary.Instance,
SendClientReadyMessage, and RequestServerDataIfConnected to locate the fix.
In `@run.bat`:
- Line 5: The batch call uses an invalid variable expansion "%run.ps1"; replace
the quoted token with the batch expansion for the script's directory plus the
script name (use %~dp0 followed by run.ps1) and keep the whole path quoted so
PowerShell is invoked with the correct full path; update the line that invokes
powershell.exe (the one containing -File "%run.ps1") to use "%~dp0run.ps1"
instead.
- Around line 1-5: The run.bat file currently uses Unix (LF) line endings;
convert the file to Windows CRLF line endings so the batch commands (e.g., lines
containing "@echo off" and "powershell.exe -ExecutionPolicy Bypass -File
\"%run.ps1\"") are parsed correctly by the Windows shell—update the file
encoding/line endings in your editor or run a tool (e.g., dos2unix/unix2dos or
Git autocrlf) to replace LF with CRLF and re-commit the updated run.bat.
In `@Server/Player/PlayerAuthentication.cs`:
- Around line 39-43: Add complete XML documentation for the public methods
AuthenticatePlayer and RequiresPassword: expand the existing <summary> to
remain, then add <param> entries for each parameter (e.g., playerInfo,
passwordHash, steamTicket for AuthenticatePlayer; the input parameter(s) for
RequiresPassword), a <returns> describing the AuthenticationResult or bool
return, <exception> tags for any exceptions thrown (specify exception types and
when they occur), a <remarks> with any important behavioral details (e.g., side
effects, thread-safety, whether null is allowed), and an <example> showing a
short usage snippet calling AuthenticatePlayer and handling the result; ensure
the XML tag names exactly match the coding guideline and place these comments
immediately above the AuthenticatePlayer and RequiresPassword method
declarations.
- Around line 39-43: AuthenticatePlayer currently dereferences the parameter
playerInfo without validation; add a guard at the start of the public method
AuthenticatePlayer to validate playerInfo and throw new
ArgumentNullException(nameof(playerInfo)) when it's null. Locate the
AuthenticatePlayer(ConnectedPlayerInfo playerInfo, string passwordHash = null,
string steamTicket = null) method and insert the null-check before any use of
playerInfo to prevent NREs and satisfy the coding guidelines for public API
parameter validation.
- Around line 84-94: The code currently logs a TODO and continues even when
ServerConfig.Instance.RequireAuthentication is true; change AuthenticatePlayer
(the method handling steamTicket and playerInfo) to immediately reject when
RequireAuthentication is true and the steamTicket is null/empty/whitespace:
check the steamTicket variable after reading it and if it's missing return a
failed authentication result (or throw the existing auth failure path) instead
of logging and proceeding; keep the logger.Msg for diagnostic context (include
playerInfo.DisplayName) but ensure the method returns the failure path when
steamTicket is not provided while RequireAuthentication is true so
authentication cannot be bypassed.
In `@Shared/Networking/MessageRouter.cs`:
- Around line 369-462: Add full XML documentation to the two public methods
SendAuthenticationChallenge and SendAuthenticationResponse: keep the existing
<summary> and add <param> entries for each parameter (e.g., conn,
requiresPassword, serverName for SendAuthenticationChallenge and passwordHash
for SendAuthenticationResponse), add a <returns> noting void, add an <exception>
tag documenting that exceptions may be thrown (e.g., System.Exception) and under
what conditions, add a <remarks> explaining the `#if` SERVER/#if CLIENT
conditional behavior and side effects (sends messages via CustomMessaging and
logs), and add a short <example> showing typical usage for each method (e.g.,
calling SendAuthenticationChallenge on server or SendAuthenticationResponse on
client). Ensure tags are present on both methods and reflect the actual
parameters and behavior.
- Around line 282-343: Handlers HandleAuthenticationResponse, HandleClientReady
and HandleServerDataRequest currently dereference NetworkConnection (e.g.,
conn.ClientId) or pass conn into other APIs without null checks; add a
null-guard at the top of each handler that logs a warning (include which
handler) and returns early if conn is null to avoid NullReferenceExceptions and
prevent calling methods like CustomMessaging.SendToClient or
MelonCoroutines.Start(DelayedDisconnect) with a null connection; retain existing
behavior for non-null connections and keep references to
PlayerManager/FindPlayerByConnection and DelayedDisconnect unchanged.
🧹 Nitpick comments (9)
run.ps1 (1)
79-93: Server start lacks startup verification.The server is started (Lines 82-90) and immediately marked as successful (Line 92), but there's no verification that the server actually started successfully. If the server process crashes immediately, the script would still report success and attempt to start the client.
💡 Consider adding a process check
if (Test-Path $startServerBat) { Write-Host " Using start_server.bat" -ForegroundColor Gray Start-Process -FilePath "cmd.exe" -ArgumentList "/c start_server.bat" -WorkingDirectory $serverPath } else { Write-Host " Starting: $serverExe" -ForegroundColor Gray Start-Process -FilePath $serverExe -WorkingDirectory $serverPath } +# Brief delay to catch immediate crashes +Start-Sleep -Seconds 2 +$serverProc = Get-Process -Name "Schedule I" -ErrorAction SilentlyContinue +if (-not $serverProc) { + Write-Host " ✗ Server may have failed to start!" -ForegroundColor Red +} + Write-Host " ✓ Server started" -ForegroundColor GreenClient/Managers/ClientPlayerSetup.cs (1)
89-89: Consider caching the logger instance.Creating a new
MelonLogger.InstanceinPlayerLoadedPrefix_HandleDedicatedServer(and other static methods throughout this file) allocates on each call. While this patch doesn't run frequently, caching a static logger would be more efficient and consistent with typical patterns.💡 Example refactor
public class ClientPlayerSetup { private readonly MelonLogger.Instance logger; private HarmonyLib.Harmony harmony; + private static MelonLogger.Instance _staticLogger; + private static MelonLogger.Instance StaticLogger => + _staticLogger ??= new MelonLogger.Instance("ClientPlayerSetup"); // Then in static methods: - var logger = new MelonLogger.Instance("ClientPlayerSetup"); + var logger = StaticLogger;Utils/PasswordHasher.cs (1)
57-66: Consider constant-time comparison for hash verification.
string.Equalsis not constant-time, which could theoretically leak information via timing attacks. While the remarks correctly note this is for server password validation (not user passwords) and network latency makes exploitation difficult, using a constant-time comparison would be a defense-in-depth improvement.🔒 Proposed fix using fixed-time comparison
public static bool VerifyPassword(string password, string hash) { if (string.IsNullOrEmpty(password) || string.IsNullOrEmpty(hash)) { return false; } string computedHash = HashPassword(password); - return string.Equals(computedHash, hash, StringComparison.OrdinalIgnoreCase); + return FixedTimeEquals(computedHash, hash); +} + +/// <summary> +/// Performs a constant-time comparison to prevent timing attacks. +/// </summary> +private static bool FixedTimeEquals(string a, string b) +{ + if (a.Length != b.Length) + return false; + + int result = 0; + for (int i = 0; i < a.Length; i++) + { + result |= char.ToLowerInvariant(a[i]) ^ char.ToLowerInvariant(b[i]); + } + return result == 0; }Alternatively, if targeting .NET Core 2.1+, you can use
CryptographicOperations.FixedTimeEqualson the byte arrays.Shared/Networking/AuthenticationMessages.cs (1)
10-98: Seal authentication message DTOs.
These message types look like DTOs and aren’t intended for inheritance; sealing them matches repo standards and avoids unintended subclassing.
As per coding guidelines: Mark classes assealedif they are not intended for inheritance.♻️ Proposed change
- public class AuthenticationChallengeMessage + public sealed class AuthenticationChallengeMessage { ... } - public class AuthenticationResponseMessage + public sealed class AuthenticationResponseMessage { ... } - public class AuthenticationResultMessage + public sealed class AuthenticationResultMessage { ... } - public class ServerInfoMessage + public sealed class ServerInfoMessage { ... }Client/Managers/PasswordDialog.cs (5)
14-36: Class should besealedand private fields should use_prefix.Per coding guidelines, classes not intended for inheritance should be marked
sealed. Additionally, private fields should use camelCase with_prefix (e.g.,_logger,_connectionManager,_passwordPromptOverlay).♻️ Proposed fix for naming conventions
- public class PasswordDialog + public sealed class PasswordDialog { - private readonly MelonLogger.Instance logger; - private readonly ClientConnectionManager connectionManager; + private readonly MelonLogger.Instance _logger; + private readonly ClientConnectionManager _connectionManager; // UI Components - private GameObject passwordPromptOverlay; - private GameObject passwordPromptPanel; - private TMP_InputField passwordInput; - private TMP_Text passwordPromptTitle; - private TMP_Text passwordErrorText; - private Button passwordSubmitButton; - private Button passwordCancelButton; + private GameObject _passwordPromptOverlay; + private GameObject _passwordPromptPanel; + private TMP_InputField _passwordInput; + private TMP_Text _passwordPromptTitle; + private TMP_Text _passwordErrorText; + private Button _passwordSubmitButton; + private Button _passwordCancelButton; // Captured fonts for UI consistency - private TMP_FontAsset capturedTmpFont; + private TMP_FontAsset _capturedTmpFont; // Cursor lock enforcement - private bool isPasswordDialogActive = false; + private bool _isPasswordDialogActive = false; // Authentication retry tracking - private int authRetryCount = 0; + private int _authRetryCount = 0;As per coding guidelines: "Use camelCase with
_prefix for private/internal fields" and "Mark classes assealedif they are not intended for inheritance".
145-176: Dead code:TriggerGamePauseMenu()is never called.This method is defined but never invoked anywhere in the class. If it's intended for future use, consider removing it until needed (YAGNI principle). If it was meant to be called, it should be integrated into the dialog show flow.
#!/bin/bash # Verify TriggerGamePauseMenu is not called elsewhere in the codebase rg -n "TriggerGamePauseMenu" --type cs
259-276: Redundant conditional: both branches execute identical code.The if/else branches for scene check both unlock the cursor with the same operations. The conditional can be simplified.
♻️ Proposed simplification
// Only restore cursor lock if we're still in-game (not in menu) var currentScene = UnityEngine.SceneManagement.SceneManager.GetActiveScene().name; logger.Msg($"Current scene: {currentScene}"); - - if (currentScene != "Menu") - { - // In-game: lock cursor - UnityEngine.Cursor.lockState = CursorLockMode.None; // Keep unlocked temporarily - UnityEngine.Cursor.visible = true; - logger.Msg("Cursor unlocked (will be locked by game when appropriate)"); - } - else - { - // In menu: keep cursor unlocked - UnityEngine.Cursor.lockState = CursorLockMode.None; - UnityEngine.Cursor.visible = true; - logger.Msg("Cursor unlocked for menu"); - } + + // Keep cursor unlocked - game will re-lock when appropriate + UnityEngine.Cursor.lockState = CursorLockMode.None; + UnityEngine.Cursor.visible = true; + logger.Msg(currentScene != "Menu" + ? "Cursor unlocked (will be locked by game when appropriate)" + : "Cursor unlocked for menu");
197-214: Consider caching theWaitForSecondsRealtimeinstance.
new WaitForSecondsRealtime(0.1f)allocates a new object on each iteration. While the impact is minimal during the brief dialog display, caching the wait object aligns with allocation-conscious practices.♻️ Optional: Cache the wait object
Add a static field:
private static readonly WaitForSecondsRealtime CursorEnforceWait = new WaitForSecondsRealtime(0.1f);Then use it in the coroutine:
- yield return new UnityEngine.WaitForSecondsRealtime(0.1f); + yield return CursorEnforceWait;
641-648: Consider usingconnectionManager.IsConnectedToDedicatedServerfor clearer intent.The
DailySummary.Instancecheck works because CustomMessaging validates this internally, but it's indirect—checking whether a UI element exists to verify network availability creates a fragile coupling. SinceconnectionManager.IsConnectedToDedicatedServeris already available to the PasswordDialog, it would express the intent more clearly and avoid relying on the UI layer's state as a proxy for connection status.
| /// <summary> | ||
| /// Shows the password prompt dialog. | ||
| /// </summary> | ||
| /// <param name="serverName">The name of the server requesting authentication</param> | ||
| public void ShowPasswordPrompt(string serverName) | ||
| { | ||
| // Set the captured font before showing the dialog | ||
| passwordDialog.SetCapturedFont(capturedTmpFont); | ||
| passwordDialog.ShowPasswordPrompt(serverName); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Hides the password prompt dialog. | ||
| /// </summary> | ||
| public void HidePasswordPrompt() | ||
| { | ||
| passwordDialog.HidePasswordPrompt(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks if the password dialog is currently visible. | ||
| /// </summary> | ||
| /// <returns>True if the password dialog is visible, false otherwise</returns> | ||
| public bool IsPasswordDialogVisible() | ||
| { | ||
| return passwordDialog?.IsVisible ?? false; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Shows an authentication error message. | ||
| /// </summary> | ||
| /// <param name="errorMessage">The error message to display</param> | ||
| public void ShowAuthenticationError(string errorMessage) | ||
| { | ||
| passwordDialog.ShowAuthenticationError(errorMessage); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Called when authentication succeeds. | ||
| /// </summary> | ||
| public void OnAuthenticationSuccess() | ||
| { | ||
| passwordDialog.OnAuthenticationSuccess(); | ||
| } |
There was a problem hiding this comment.
Add full XML doc tags for the new public UI methods.
These public APIs currently only have summaries; please include <param>, <returns>, <exception>, <remarks>, and <example> tags per repo standards.
As per coding guidelines: XML documentation is required for all public/protected APIs; include <summary>, <param>, <returns>, <exception>, <remarks>, and <example> tags.
🤖 Prompt for AI Agents
In `@Client/Managers/ClientUIManager.cs` around lines 1159 - 1202, Add full XML
documentation to the new public UI methods: ShowPasswordPrompt(string
serverName), HidePasswordPrompt(), IsPasswordDialogVisible(),
ShowAuthenticationError(string errorMessage), and OnAuthenticationSuccess(). For
each method include <summary> (existing), <param> entries for parameters
(serverName, errorMessage), a <returns> tag for IsPasswordDialogVisible
describing the boolean result, <exception> tags if these methods can throw
(e.g., null reference if passwordDialog is null), a <remarks> note about
interaction with passwordDialog and capturedTmpFont, and a short <example>
showing typical usage (calling ShowPasswordPrompt then handling
OnAuthenticationSuccess/ShowAuthenticationError). Ensure tags follow the repo
standard and reference the passwordDialog field where appropriate.
| using MelonLoader; | ||
| using System; | ||
| using TMPro; | ||
| using UnityEngine; | ||
| using UnityEngine.UI; | ||
| using UnityEngine.Events; | ||
| using ScheduleOne.PlayerScripts; | ||
|
|
||
| namespace DedicatedServerMod.Client.Managers | ||
| { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how many files in Client/ folder use `#if` CLIENT
echo "=== Files with `#if` CLIENT in Client/ folder ==="
rg "#if CLIENT" Client/ --count-matches | head -20
echo ""
echo "=== Sample of Client files that use `#if` CLIENT ==="
rg -l "#if CLIENT" Client/ | head -5 | xargs -I {} bash -c 'echo "File: {}" && head -15 {}'Repository: ifBars/S1DedicatedServers
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Check if Client/ has project-level configuration
echo "=== Looking for .csproj files ==="
fd -e csproj . | head -10
echo ""
echo "=== Check for Assembly-level directives in Client folder ==="
fd -e cs . Client/ | xargs grep -l "AssemblyInfo\|#if CLIENT\|#if SERVER" | head -5Repository: ifBars/S1DedicatedServers
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Check the actual file content to see what it imports and does
echo "=== Content of PasswordDialog.cs (first 30 lines) ==="
head -30 "Client/Managers/PasswordDialog.cs"Repository: ifBars/S1DedicatedServers
Length of output: 1020
Add #if CLIENT preprocessor guard to protect client-specific code.
This file contains client-specific dependencies (TMPro, UnityEngine.UI, MelonLoader) that will cause compilation errors in server builds. Per coding guidelines and build requirements, all client-specific code must be wrapped with #if CLIENT to support building successfully in all four configurations (Mono_Server, Mono_Client, Il2cpp_Server, Il2cpp_Client).
🤖 Prompt for AI Agents
In `@Client/Managers/PasswordDialog.cs` around lines 1 - 10, Wrap all client-only
code in this file with a preprocessor guard so it is excluded from server
builds: add `#if` CLIENT at the top (before the using directives that reference
MelonLoader, TMPro, UnityEngine.UI, UnityEngine, etc.) and a matching `#endif` at
the end of the file (after the namespace DedicatedServerMod.Client.Managers and
the PasswordDialog class declaration), ensuring all client-specific usings and
the entire PasswordDialog implementation are enclosed.
| public PasswordDialog(MelonLogger.Instance logger, ClientConnectionManager connectionManager) | ||
| { | ||
| this.logger = logger; | ||
| this.connectionManager = connectionManager; | ||
| } |
There was a problem hiding this comment.
Constructor should validate parameters.
The constructor accepts logger and connectionManager but doesn't validate them for null. Per coding guidelines, public method parameters should be validated.
🛡️ Proposed fix to add parameter validation
public PasswordDialog(MelonLogger.Instance logger, ClientConnectionManager connectionManager)
{
- this.logger = logger;
- this.connectionManager = connectionManager;
+ this._logger = logger ?? throw new ArgumentNullException(nameof(logger));
+ this._connectionManager = connectionManager ?? throw new ArgumentNullException(nameof(connectionManager));
}As per coding guidelines: "Validate public method parameters and throw ArgumentException or ArgumentNullException for invalid inputs".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public PasswordDialog(MelonLogger.Instance logger, ClientConnectionManager connectionManager) | |
| { | |
| this.logger = logger; | |
| this.connectionManager = connectionManager; | |
| } | |
| public PasswordDialog(MelonLogger.Instance logger, ClientConnectionManager connectionManager) | |
| { | |
| this._logger = logger ?? throw new ArgumentNullException(nameof(logger)); | |
| this._connectionManager = connectionManager ?? throw new ArgumentNullException(nameof(connectionManager)); | |
| } |
🤖 Prompt for AI Agents
In `@Client/Managers/PasswordDialog.cs` around lines 49 - 53, The PasswordDialog
constructor currently doesn't validate its public parameters; add null checks
for the logger and connectionManager parameters in the
PasswordDialog(MelonLogger.Instance logger, ClientConnectionManager
connectionManager) constructor and throw ArgumentNullException (with the
parameter name) for any that are null so callers get immediate, clear failures
instead of later NREs.
| passwordInput = inputObj.AddComponent<TMP_InputField>(); | ||
| passwordInput.inputType = TMP_InputField.InputType.Password; | ||
| passwordInput.asteriskChar = '�'; | ||
|
|
||
| // Enable Enter key to submit | ||
| passwordInput.onSubmit.AddListener((text) => OnPasswordSubmit()); |
There was a problem hiding this comment.
Potential encoding issue with asterisk character.
Line 476 shows '�' which suggests a character encoding problem. If the intended character is a bullet (●), use the Unicode escape '\u25CF' to ensure consistent behavior across different file encodings.
🔧 Proposed fix
passwordInput = inputObj.AddComponent<TMP_InputField>();
passwordInput.inputType = TMP_InputField.InputType.Password;
- passwordInput.asteriskChar = '�';
+ passwordInput.asteriskChar = '\u25CF'; // Bullet character ●📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| passwordInput = inputObj.AddComponent<TMP_InputField>(); | |
| passwordInput.inputType = TMP_InputField.InputType.Password; | |
| passwordInput.asteriskChar = '�'; | |
| // Enable Enter key to submit | |
| passwordInput.onSubmit.AddListener((text) => OnPasswordSubmit()); | |
| passwordInput = inputObj.AddComponent<TMP_InputField>(); | |
| passwordInput.inputType = TMP_InputField.InputType.Password; | |
| passwordInput.asteriskChar = '\u25CF'; // Bullet character ● | |
| // Enable Enter key to submit | |
| passwordInput.onSubmit.AddListener((text) => OnPasswordSubmit()); |
🤖 Prompt for AI Agents
In `@Client/Managers/PasswordDialog.cs` around lines 474 - 479, The asteriskChar
assignment for passwordInput currently uses a malformed character ('�') likely
due to encoding; update the PasswordDialog.cs code that sets
passwordInput.asteriskChar to use the Unicode escape for a bullet (e.g.
'\u25CF') so the intended bullet (●) displays reliably across encodings, leaving
the onSubmit listener (OnPasswordSubmit) unchanged.
| private static System.Collections.IEnumerator SendDelayedClientMessages() | ||
| { | ||
| // Wait for DailySummary to be spawned (up to 5 seconds) | ||
| float timeout = 5f; | ||
| float elapsed = 0f; | ||
|
|
||
| while (elapsed < timeout) | ||
| { | ||
| var ds = DailySummary.Instance; | ||
| if (ds != null) | ||
| { | ||
| var nb = ds as NetworkBehaviour; | ||
| if (nb != null && nb.IsSpawned) | ||
| { | ||
| _logger?.Msg("DailySummary is spawned, sending client messages"); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| yield return new WaitForSeconds(0.1f); | ||
| elapsed += 0.1f; | ||
| } | ||
|
|
||
| if (elapsed >= timeout) | ||
| { | ||
| _logger?.Warning("Timeout waiting for DailySummary to spawn"); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| // Send "client ready" message to server so it knows we can receive authentication challenges | ||
| SendClientReadyMessage(); | ||
|
|
||
| // Now that RPCs are registered, request server data if connected | ||
| RequestServerDataIfConnected(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the MessagingPatches.cs file
find . -name "MessagingPatches.cs" -type fRepository: ifBars/S1DedicatedServers
Length of output: 105
🏁 Script executed:
# Get the full context of the file to understand SendClientReadyMessage and related code
wc -l "Client/Patches/MessagingPatches.cs"Repository: ifBars/S1DedicatedServers
Length of output: 107
🏁 Script executed:
# Read the file to see the full implementation
cat -n "Client/Patches/MessagingPatches.cs"Repository: ifBars/S1DedicatedServers
Length of output: 8818
🏁 Script executed:
# Find the CustomMessaging implementation
find . -path "*/Shared/Networking/*" -name "*CustomMessaging*" -o -path "*/Networking/*" -name "*CustomMessaging*" | head -20Repository: ifBars/S1DedicatedServers
Length of output: 51
🏁 Script executed:
# Also search more broadly
find . -name "*CustomMessaging*" -type fRepository: ifBars/S1DedicatedServers
Length of output: 96
🏁 Script executed:
# If found, get file size and examine it
wc -l "Shared/Networking/CustomMessaging.cs" 2>/dev/null || echo "File not found, searching for it..."Repository: ifBars/S1DedicatedServers
Length of output: 104
🏁 Script executed:
# Broader search for CustomMessaging class
rg -l "CustomMessaging" --type cs | grep -v node_modules | head -5Repository: ifBars/S1DedicatedServers
Length of output: 236
🏁 Script executed:
# Read the CustomMessaging.cs file
wc -l "Shared/CustomMessaging.cs"Repository: ifBars/S1DedicatedServers
Length of output: 98
🏁 Script executed:
# Examine the file
cat -n "Shared/CustomMessaging.cs"Repository: ifBars/S1DedicatedServers
Length of output: 13383
🏁 Script executed:
# Check for timeScale usage that might affect this timing
rg "timeScale" --type cs -B 2 -A 2 | head -50Repository: ifBars/S1DedicatedServers
Length of output: 3174
🏁 Script executed:
# Also verify the WaitForSecondsRealtime suggestion is valid
rg "WaitForSecondsRealtime" --type csRepository: ifBars/S1DedicatedServers
Length of output: 351
Use unscaled waits and continue retrying after timeout to prevent client_ready message loss during startup with paused game state.
The coroutine uses WaitForSeconds, which respects Time.timeScale. If the game is paused (timeScale = 0) before DailySummary spawns—such as during password authentication in PasswordDialog—the elapsed time freeze prevents the timeout from firing, indefinitely blocking the coroutine. Even if the timeout does fire, the coroutine continues to call SendClientReadyMessage(), which calls SendToServer(). However, if DailySummary is still not spawned, SendToServer() silently returns without sending the message (lines 134-138 in CustomMessaging.cs), leaving authentication stuck without warning.
Replace WaitForSeconds with WaitForSecondsRealtime (already used elsewhere in PasswordDialog.cs) and keep retrying after the timeout instead of exiting, logging once:
🔧 Suggested fix
private static System.Collections.IEnumerator SendDelayedClientMessages()
{
// Wait for DailySummary to be spawned (up to 5 seconds)
float timeout = 5f;
float elapsed = 0f;
+ bool warned = false;
- while (elapsed < timeout)
+ while (true)
{
var ds = DailySummary.Instance;
if (ds != null)
{
var nb = ds as NetworkBehaviour;
if (nb != null && nb.IsSpawned)
{
_logger?.Msg("DailySummary is spawned, sending client messages");
break;
}
}
- yield return new WaitForSeconds(0.1f);
- elapsed += 0.1f;
+ if (!warned && elapsed >= timeout)
+ {
+ _logger?.Warning("Timeout waiting for DailySummary to spawn");
+ warned = true;
+ }
+
+ yield return new WaitForSecondsRealtime(0.1f);
+ elapsed += 0.1f;
}
-
- if (elapsed >= timeout)
- {
- _logger?.Warning("Timeout waiting for DailySummary to spawn");
- }🤖 Prompt for AI Agents
In `@Client/Patches/MessagingPatches.cs` around lines 80 - 115, The coroutine
SendDelayedClientMessages currently uses WaitForSeconds and breaks/aborts on
timeout which can hang or silently drop the SendClientReadyMessage when
Time.timeScale==0; change the loop to use WaitForSecondsRealtime(0.1f) to use
unscaled time (like PasswordDialog), remove the break/early-exit behavior so the
routine continues retrying until DailySummary.Instance is spawned and IsSpawned
is true, and instead emit a single warning log if the timeout threshold is
crossed (track a bool like warnedOnce) while still attempting
SendClientReadyMessage()/RequestServerDataIfConnected() only when DailySummary
is confirmed spawned; keep references to DailySummary.Instance,
SendClientReadyMessage, and RequestServerDataIfConnected to locate the fix.
| REM Full debug launcher - builds and starts both server and client | ||
| REM Double-click this file for complete testing workflow | ||
|
|
||
| powershell.exe -ExecutionPolicy Bypass -File "%run.ps1" |
There was a problem hiding this comment.
Invalid variable expansion syntax will cause script failure.
The %run.ps1 syntax is incorrect. The % character starts variable expansion in batch files, but there's no closing %. This will not correctly resolve the path to run.ps1.
🐛 Proposed fix
-powershell.exe -ExecutionPolicy Bypass -File "%run.ps1"
+powershell.exe -ExecutionPolicy Bypass -File "%~dp0run.ps1"%~dp0 expands to the drive and path of the batch file, ensuring the PowerShell script is found regardless of the current working directory.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| powershell.exe -ExecutionPolicy Bypass -File "%run.ps1" | |
| powershell.exe -ExecutionPolicy Bypass -File "%~dp0run.ps1" |
🧰 Tools
🪛 Blinter (1.0.112)
[error] 5-5: Invalid variable expansion syntax. Explanation: Variable references must use proper %VAR% or !VAR! syntax with matching delimiters. Recommendation: Ensure variables use matching % or ! delimiters: %VAR% for standard, !VAR! for delayed expansion. Context: Variable reference may have mismatched % delimiters
(E011)
[error] 5-5: PowerShell execution policy bypass. Explanation: Bypassing PowerShell execution policy can allow malicious scripts to run. Recommendation: Avoid using -ExecutionPolicy Bypass unless absolutely necessary. Context: PowerShell execution policy bypass detected
(SEC009)
🤖 Prompt for AI Agents
In `@run.bat` at line 5, The batch call uses an invalid variable expansion
"%run.ps1"; replace the quoted token with the batch expansion for the script's
directory plus the script name (use %~dp0 followed by run.ps1) and keep the
whole path quoted so PowerShell is invoked with the correct full path; update
the line that invokes powershell.exe (the one containing -File "%run.ps1") to
use "%~dp0run.ps1" instead.
| /// <summary> | ||
| /// Authenticate a player using their Steam ticket | ||
| /// Authenticate a player using their password hash and Steam ticket | ||
| /// </summary> | ||
| public AuthenticationResult AuthenticatePlayer(ConnectedPlayerInfo playerInfo, string steamTicket = null) | ||
| public AuthenticationResult AuthenticatePlayer(ConnectedPlayerInfo playerInfo, string passwordHash = null, string steamTicket = null) | ||
| { |
There was a problem hiding this comment.
Public API docs need the full XML tag set.
AuthenticatePlayer and RequiresPassword are public but only have summaries; please add <param>, <returns>, <exception>, <remarks>, and <example> tags.
As per coding guidelines: XML documentation is required for all public/protected APIs; include <summary>, <param>, <returns>, <exception>, <remarks>, and <example> tags.
Also applies to: 115-122
🤖 Prompt for AI Agents
In `@Server/Player/PlayerAuthentication.cs` around lines 39 - 43, Add complete XML
documentation for the public methods AuthenticatePlayer and RequiresPassword:
expand the existing <summary> to remain, then add <param> entries for each
parameter (e.g., playerInfo, passwordHash, steamTicket for AuthenticatePlayer;
the input parameter(s) for RequiresPassword), a <returns> describing the
AuthenticationResult or bool return, <exception> tags for any exceptions thrown
(specify exception types and when they occur), a <remarks> with any important
behavioral details (e.g., side effects, thread-safety, whether null is allowed),
and an <example> showing a short usage snippet calling AuthenticatePlayer and
handling the result; ensure the XML tag names exactly match the coding guideline
and place these comments immediately above the AuthenticatePlayer and
RequiresPassword method declarations.
Guard AuthenticatePlayer against null playerInfo.
This public API dereferences playerInfo immediately; add an ArgumentNullException to avoid NREs.
As per coding guidelines: Validate public method parameters and throw ArgumentException or ArgumentNullException for invalid inputs.
🛡️ Proposed fix
public AuthenticationResult AuthenticatePlayer(ConnectedPlayerInfo playerInfo, string passwordHash = null, string steamTicket = null)
{
+ if (playerInfo == null)
+ throw new ArgumentNullException(nameof(playerInfo));
try
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// Authenticate a player using their Steam ticket | |
| /// Authenticate a player using their password hash and Steam ticket | |
| /// </summary> | |
| public AuthenticationResult AuthenticatePlayer(ConnectedPlayerInfo playerInfo, string steamTicket = null) | |
| public AuthenticationResult AuthenticatePlayer(ConnectedPlayerInfo playerInfo, string passwordHash = null, string steamTicket = null) | |
| { | |
| /// <summary> | |
| /// Authenticate a player using their password hash and Steam ticket | |
| /// </summary> | |
| public AuthenticationResult AuthenticatePlayer(ConnectedPlayerInfo playerInfo, string passwordHash = null, string steamTicket = null) | |
| { | |
| if (playerInfo == null) | |
| throw new ArgumentNullException(nameof(playerInfo)); | |
| try | |
| { |
🤖 Prompt for AI Agents
In `@Server/Player/PlayerAuthentication.cs` around lines 39 - 43,
AuthenticatePlayer currently dereferences the parameter playerInfo without
validation; add a guard at the start of the public method AuthenticatePlayer to
validate playerInfo and throw new ArgumentNullException(nameof(playerInfo)) when
it's null. Locate the AuthenticatePlayer(ConnectedPlayerInfo playerInfo, string
passwordHash = null, string steamTicket = null) method and insert the null-check
before any use of playerInfo to prevent NREs and satisfy the coding guidelines
for public API parameter validation.
| // If Steam authentication is required, validate ticket | ||
| if (ServerConfig.Instance.RequireAuthentication) | ||
| { | ||
| // TODO: Implement actual Steam ticket validation | ||
| // This would involve: | ||
| // 1. Validating the ticket with Steam's servers | ||
| // 2. Extracting the SteamID from the validated ticket | ||
| // 3. Checking if the SteamID matches expected values | ||
|
|
||
| logger.Msg($"TODO: Validate Steam ticket for {playerInfo.DisplayName}"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the PlayerAuthentication.cs file
find . -name "PlayerAuthentication.cs" -type fRepository: ifBars/S1DedicatedServers
Length of output: 108
🏁 Script executed:
# Read the PlayerAuthentication.cs file to understand the full method context
cat -n ./Server/Player/PlayerAuthentication.csRepository: ifBars/S1DedicatedServers
Length of output: 9975
RequireAuthentication bypasses authentication when no Steam ticket is provided.
When RequireAuthentication is true, the method only logs a TODO and proceeds to return success (line 97) regardless of whether steamTicket is null, empty, or invalid. This completely undermines the authentication requirement. The method should fail immediately if no Steam ticket is provided when authentication is required.
Proposed fix
if (ServerConfig.Instance.RequireAuthentication)
{
+ if (string.IsNullOrWhiteSpace(steamTicket))
+ {
+ return new AuthenticationResult
+ {
+ IsSuccessful = false,
+ Message = "Steam authentication required",
+ ShouldDisconnect = true
+ };
+ }
// TODO: Implement actual Steam ticket validation
// This would involve:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If Steam authentication is required, validate ticket | |
| if (ServerConfig.Instance.RequireAuthentication) | |
| { | |
| // TODO: Implement actual Steam ticket validation | |
| // This would involve: | |
| // 1. Validating the ticket with Steam's servers | |
| // 2. Extracting the SteamID from the validated ticket | |
| // 3. Checking if the SteamID matches expected values | |
| logger.Msg($"TODO: Validate Steam ticket for {playerInfo.DisplayName}"); | |
| } | |
| // If Steam authentication is required, validate ticket | |
| if (ServerConfig.Instance.RequireAuthentication) | |
| { | |
| if (string.IsNullOrWhiteSpace(steamTicket)) | |
| { | |
| return new AuthenticationResult | |
| { | |
| IsSuccessful = false, | |
| Message = "Steam authentication required", | |
| ShouldDisconnect = true | |
| }; | |
| } | |
| // TODO: Implement actual Steam ticket validation | |
| // This would involve: | |
| // 1. Validating the ticket with Steam's servers | |
| // 2. Extracting the SteamID from the validated ticket | |
| // 3. Checking if the SteamID matches expected values | |
| logger.Msg($"TODO: Validate Steam ticket for {playerInfo.DisplayName}"); | |
| } |
🤖 Prompt for AI Agents
In `@Server/Player/PlayerAuthentication.cs` around lines 84 - 94, The code
currently logs a TODO and continues even when
ServerConfig.Instance.RequireAuthentication is true; change AuthenticatePlayer
(the method handling steamTicket and playerInfo) to immediately reject when
RequireAuthentication is true and the steamTicket is null/empty/whitespace:
check the steamTicket variable after reading it and if it's missing return a
failed authentication result (or throw the existing auth failure path) instead
of logging and proceeding; keep the logger.Msg for diagnostic context (include
playerInfo.DisplayName) but ensure the method returns the failure path when
steamTicket is not provided while RequireAuthentication is true so
authentication cannot be bypassed.
| private static void HandleAuthenticationResponse(NetworkConnection conn, string data) | ||
| { | ||
| #if SERVER | ||
| try | ||
| { | ||
| var response = JsonConvert.DeserializeObject<AuthenticationResponseMessage>(data); | ||
| if (response == null) | ||
| { | ||
| _logger.Warning("HandleAuthenticationResponse: Failed to deserialize response"); | ||
| return; | ||
| } | ||
|
|
||
| _logger.Msg($"Received auth response from ClientId {conn.ClientId}"); | ||
|
|
||
| // Get player manager instance | ||
| var playerManager = DedicatedServerMod.Server.Core.ServerBootstrap.Players; | ||
| if (playerManager == null) | ||
| { | ||
| _logger.Error("HandleAuthenticationResponse: PlayerManager not found"); | ||
| SendAuthenticationResult(conn, false, "Server error"); | ||
| conn.Disconnect(true); | ||
| return; | ||
| } | ||
|
|
||
| var playerInfo = playerManager.GetPlayer(conn); | ||
| if (playerInfo == null) | ||
| { | ||
| _logger.Warning($"HandleAuthenticationResponse: No player info for ClientId {conn.ClientId}"); | ||
| SendAuthenticationResult(conn, false, "Player not found"); | ||
| conn.Disconnect(true); | ||
| return; | ||
| } | ||
|
|
||
| // Authenticate player with password hash | ||
| var authResult = playerManager.Authentication.AuthenticatePlayer(playerInfo, response.PasswordHash); | ||
|
|
||
| if (authResult.IsSuccessful) | ||
| { | ||
| playerInfo.IsAuthenticated = true; | ||
| _logger.Msg($"Player authenticated successfully: ClientId {conn.ClientId}"); | ||
| SendAuthenticationResult(conn, true, "Authentication successful"); | ||
| } | ||
| else | ||
| { | ||
| _logger.Warning($"Authentication failed for ClientId {conn.ClientId}: {authResult.Message}"); | ||
| SendAuthenticationResult(conn, false, authResult.Message); | ||
|
|
||
| if (authResult.ShouldDisconnect) | ||
| { | ||
| // Delay disconnect slightly to ensure message is sent | ||
| MelonCoroutines.Start(DelayedDisconnect(conn, 0.5f)); | ||
| } | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.Error($"HandleAuthenticationResponse: Error: {ex}"); | ||
| SendAuthenticationResult(conn, false, "Authentication error"); | ||
| conn.Disconnect(true); | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root && find . -name "MessageRouter.cs" -type fRepository: ifBars/S1DedicatedServers
Length of output: 124
🏁 Script executed:
# Find and read the MessageRouter.cs file
head -350 ./Shared/Networking/MessageRouter.cs | tail -100Repository: ifBars/S1DedicatedServers
Length of output: 4292
🏁 Script executed:
# Get the full RouteServerMessage method and context around lines 282-343
sed -n '250,370p' ./Shared/Networking/MessageRouter.cs | cat -nRepository: ifBars/S1DedicatedServers
Length of output: 5783
🏁 Script executed:
# Also check HandleClientReady around lines 531-556
sed -n '500,570p' ./Shared/Networking/MessageRouter.cs | cat -nRepository: ifBars/S1DedicatedServers
Length of output: 3345
🏁 Script executed:
# Search for RouteServerMessage definition to understand null handling
rg -n "RouteServerMessage" ./Shared/Networking/MessageRouter.cs -A 30Repository: ifBars/S1DedicatedServers
Length of output: 1312
🏁 Script executed:
# Check if there are null checks on NetworkConnection elsewhere in the codebase
rg -n "conn == null|conn != null" ./Shared/Networking/ -B 2 -A 2Repository: ifBars/S1DedicatedServers
Length of output: 848
🏁 Script executed:
# Search for FishNet NetworkConnection usage patterns in the codebase
rg -n "NetworkConnection" ./Shared/Networking/MessageRouter.cs -B 2 -A 2 | head -100Repository: ifBars/S1DedicatedServers
Length of output: 2435
🏁 Script executed:
# Check HandleAdminConsoleCommand for null dereference
sed -n '122,200p' ./Shared/Networking/MessageRouter.cs | cat -nRepository: ifBars/S1DedicatedServers
Length of output: 3702
🏁 Script executed:
# Check HandleServerDataRequest for null dereference
sed -n '631,700p' ./Shared/Networking/MessageRouter.cs | cat -nRepository: ifBars/S1DedicatedServers
Length of output: 4221
Add null guards to message handlers that dereference NetworkConnection.
HandleAuthenticationResponse, HandleClientReady, and HandleServerDataRequest all dereference conn.ClientId or pass conn to methods without null checks. Since FindPlayerByConnection and DelayedDisconnect both defend against null connections, null values are possible. Add guards at the start of each handler:
- HandleAuthenticationResponse (line 282): dereferences
conn.ClientIdat line 45 - HandleClientReady (line 531): dereferences
conn.ClientIdat line 37 - HandleServerDataRequest (line 631): passes
conntoCustomMessaging.SendToClient()at line 15
Suggested guards
private static void HandleAuthenticationResponse(NetworkConnection conn, string data)
{
`#if` SERVER
try
{
+ if (conn == null)
+ {
+ _logger.Warning("HandleAuthenticationResponse: connection is null");
+ return;
+ }
var response = JsonConvert.DeserializeObject<AuthenticationResponseMessage>(data); private static void HandleClientReady(NetworkConnection conn)
{
`#if` SERVER
try
{
+ if (conn == null)
+ {
+ _logger.Warning("HandleClientReady: connection is null");
+ return;
+ }
_logger.Msg($"Client ready message received from ClientId {conn.ClientId}"); private static void HandleServerDataRequest(NetworkConnection conn)
{
try
{
+ if (conn == null)
+ {
+ _logger.Warning("HandleServerDataRequest: connection is null");
+ return;
+ }
var config = Shared.Configuration.ServerConfig.Instance;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static void HandleAuthenticationResponse(NetworkConnection conn, string data) | |
| { | |
| #if SERVER | |
| try | |
| { | |
| var response = JsonConvert.DeserializeObject<AuthenticationResponseMessage>(data); | |
| if (response == null) | |
| { | |
| _logger.Warning("HandleAuthenticationResponse: Failed to deserialize response"); | |
| return; | |
| } | |
| _logger.Msg($"Received auth response from ClientId {conn.ClientId}"); | |
| // Get player manager instance | |
| var playerManager = DedicatedServerMod.Server.Core.ServerBootstrap.Players; | |
| if (playerManager == null) | |
| { | |
| _logger.Error("HandleAuthenticationResponse: PlayerManager not found"); | |
| SendAuthenticationResult(conn, false, "Server error"); | |
| conn.Disconnect(true); | |
| return; | |
| } | |
| var playerInfo = playerManager.GetPlayer(conn); | |
| if (playerInfo == null) | |
| { | |
| _logger.Warning($"HandleAuthenticationResponse: No player info for ClientId {conn.ClientId}"); | |
| SendAuthenticationResult(conn, false, "Player not found"); | |
| conn.Disconnect(true); | |
| return; | |
| } | |
| // Authenticate player with password hash | |
| var authResult = playerManager.Authentication.AuthenticatePlayer(playerInfo, response.PasswordHash); | |
| if (authResult.IsSuccessful) | |
| { | |
| playerInfo.IsAuthenticated = true; | |
| _logger.Msg($"Player authenticated successfully: ClientId {conn.ClientId}"); | |
| SendAuthenticationResult(conn, true, "Authentication successful"); | |
| } | |
| else | |
| { | |
| _logger.Warning($"Authentication failed for ClientId {conn.ClientId}: {authResult.Message}"); | |
| SendAuthenticationResult(conn, false, authResult.Message); | |
| if (authResult.ShouldDisconnect) | |
| { | |
| // Delay disconnect slightly to ensure message is sent | |
| MelonCoroutines.Start(DelayedDisconnect(conn, 0.5f)); | |
| } | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.Error($"HandleAuthenticationResponse: Error: {ex}"); | |
| SendAuthenticationResult(conn, false, "Authentication error"); | |
| conn.Disconnect(true); | |
| } | |
| #endif | |
| } | |
| private static void HandleAuthenticationResponse(NetworkConnection conn, string data) | |
| { | |
| `#if` SERVER | |
| try | |
| { | |
| if (conn == null) | |
| { | |
| _logger.Warning("HandleAuthenticationResponse: connection is null"); | |
| return; | |
| } | |
| var response = JsonConvert.DeserializeObject<AuthenticationResponseMessage>(data); | |
| if (response == null) | |
| { | |
| _logger.Warning("HandleAuthenticationResponse: Failed to deserialize response"); | |
| return; | |
| } | |
| _logger.Msg($"Received auth response from ClientId {conn.ClientId}"); | |
| // Get player manager instance | |
| var playerManager = DedicatedServerMod.Server.Core.ServerBootstrap.Players; | |
| if (playerManager == null) | |
| { | |
| _logger.Error("HandleAuthenticationResponse: PlayerManager not found"); | |
| SendAuthenticationResult(conn, false, "Server error"); | |
| conn.Disconnect(true); | |
| return; | |
| } | |
| var playerInfo = playerManager.GetPlayer(conn); | |
| if (playerInfo == null) | |
| { | |
| _logger.Warning($"HandleAuthenticationResponse: No player info for ClientId {conn.ClientId}"); | |
| SendAuthenticationResult(conn, false, "Player not found"); | |
| conn.Disconnect(true); | |
| return; | |
| } | |
| // Authenticate player with password hash | |
| var authResult = playerManager.Authentication.AuthenticatePlayer(playerInfo, response.PasswordHash); | |
| if (authResult.IsSuccessful) | |
| { | |
| playerInfo.IsAuthenticated = true; | |
| _logger.Msg($"Player authenticated successfully: ClientId {conn.ClientId}"); | |
| SendAuthenticationResult(conn, true, "Authentication successful"); | |
| } | |
| else | |
| { | |
| _logger.Warning($"Authentication failed for ClientId {conn.ClientId}: {authResult.Message}"); | |
| SendAuthenticationResult(conn, false, authResult.Message); | |
| if (authResult.ShouldDisconnect) | |
| { | |
| // Delay disconnect slightly to ensure message is sent | |
| MelonCoroutines.Start(DelayedDisconnect(conn, 0.5f)); | |
| } | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.Error($"HandleAuthenticationResponse: Error: {ex}"); | |
| SendAuthenticationResult(conn, false, "Authentication error"); | |
| conn.Disconnect(true); | |
| } | |
| `#endif` | |
| } |
🤖 Prompt for AI Agents
In `@Shared/Networking/MessageRouter.cs` around lines 282 - 343, Handlers
HandleAuthenticationResponse, HandleClientReady and HandleServerDataRequest
currently dereference NetworkConnection (e.g., conn.ClientId) or pass conn into
other APIs without null checks; add a null-guard at the top of each handler that
logs a warning (include which handler) and returns early if conn is null to
avoid NullReferenceExceptions and prevent calling methods like
CustomMessaging.SendToClient or MelonCoroutines.Start(DelayedDisconnect) with a
null connection; retain existing behavior for non-null connections and keep
references to PlayerManager/FindPlayerByConnection and DelayedDisconnect
unchanged.
| /// <summary> | ||
| /// Sends an authentication challenge to a client (server-side). | ||
| /// </summary> | ||
| public static void SendAuthenticationChallenge(NetworkConnection conn, bool requiresPassword, string serverName) | ||
| { | ||
| #if SERVER | ||
| try | ||
| { | ||
| var challenge = new AuthenticationChallengeMessage | ||
| { | ||
| RequiresPassword = requiresPassword, | ||
| ServerName = serverName | ||
| }; | ||
|
|
||
| var json = JsonConvert.SerializeObject(challenge); | ||
| CustomMessaging.SendToClient(conn, "auth_challenge", json); | ||
| _logger.Msg($"Sent auth challenge to ClientId {conn.ClientId} (password required: {requiresPassword})"); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.Error($"SendAuthenticationChallenge: Error: {ex}"); | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Handles an authentication challenge from the server (client-side). | ||
| /// </summary> | ||
| private static void HandleAuthenticationChallenge(string data) | ||
| { | ||
| #if CLIENT | ||
| try | ||
| { | ||
| var challenge = JsonConvert.DeserializeObject<AuthenticationChallengeMessage>(data); | ||
| if (challenge == null) | ||
| { | ||
| _logger.Warning("HandleAuthenticationChallenge: Failed to deserialize challenge"); | ||
| return; | ||
| } | ||
|
|
||
| _logger.Msg($"Received auth challenge from server: {challenge.ServerName} (password required: {challenge.RequiresPassword})"); | ||
|
|
||
| if (challenge.RequiresPassword) | ||
| { | ||
| // Trigger password prompt via ClientUIManager | ||
| var uiManager = DedicatedServerMod.Client.Core.ClientBootstrap.Instance?.UIManager; | ||
| if (uiManager != null) | ||
| { | ||
| uiManager.ShowPasswordPrompt(challenge.ServerName); | ||
| } | ||
| else | ||
| { | ||
| _logger.Error("HandleAuthenticationChallenge: UIManager not found"); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // No password required, send empty response to continue | ||
| SendAuthenticationResponse(null); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.Error($"HandleAuthenticationChallenge: Error: {ex}"); | ||
| } | ||
| #else | ||
| _logger.Warning("HandleAuthenticationChallenge called but CLIENT directive not defined"); | ||
| #endif | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Sends an authentication response to the server (client-side). | ||
| /// </summary> | ||
| public static void SendAuthenticationResponse(string passwordHash) | ||
| { | ||
| #if CLIENT | ||
| try | ||
| { | ||
| var response = new AuthenticationResponseMessage | ||
| { | ||
| PasswordHash = passwordHash ?? string.Empty, | ||
| ClientVersion = DedicatedServerMod.Utils.Constants.MOD_VERSION | ||
| }; | ||
|
|
||
| var json = JsonConvert.SerializeObject(response); | ||
| CustomMessaging.SendToServer("auth_response", json); | ||
| _logger.Msg("Sent auth response to server"); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.Error($"SendAuthenticationResponse: Error: {ex}"); | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Public router APIs need full XML doc tags.
SendAuthenticationChallenge and SendAuthenticationResponse are public but only have summaries; please add <param>, <returns>, <exception>, <remarks>, and <example> tags.
As per coding guidelines: XML documentation is required for all public/protected APIs; include <summary>, <param>, <returns>, <exception>, <remarks>, and <example> tags.
🤖 Prompt for AI Agents
In `@Shared/Networking/MessageRouter.cs` around lines 369 - 462, Add full XML
documentation to the two public methods SendAuthenticationChallenge and
SendAuthenticationResponse: keep the existing <summary> and add <param> entries
for each parameter (e.g., conn, requiresPassword, serverName for
SendAuthenticationChallenge and passwordHash for SendAuthenticationResponse),
add a <returns> noting void, add an <exception> tag documenting that exceptions
may be thrown (e.g., System.Exception) and under what conditions, add a
<remarks> explaining the `#if` SERVER/#if CLIENT conditional behavior and side
effects (sends messages via CustomMessaging and logs), and add a short <example>
showing typical usage for each method (e.g., calling SendAuthenticationChallenge
on server or SendAuthenticationResponse on client). Ensure tags are present on
both methods and reflect the actual parameters and behavior.
|
This implementation uses PBKDF2, which is a solid and widely accepted approach Increasing the iteration count (e.g. 100,000+) would improve resistance against Alternatively, algorithms like bcrypt or Argon2 are commonly used for password Another thing to consider is pre-authenticated players count towards max users, this means players can indefinitely occupy slots as soon as they connect. Overall the structure here is good, but it's just something to consider. |
First version of a challenge-response password authentication. Server serverPassword config field now enforced via SHA256 hash validation during connection handshake.
Flow: Client connects → Server sends auth_challenge → Client prompts password → Hashes with SHA256 → Sends auth_response → Server validates → Accept or reject with reason.
Screenshots
Release Notes
Password-Protected Server Connections: Implemented challenge-response password authentication using SHA256 hashing. Server enforces password validation during connection handshake before allowing client access.
Password Authentication UI: Added new PasswordDialog component with user-friendly password prompt interface, error messaging, and automatic game pause/cursor management during authentication.
Authentication Flow Refactoring: Moved client authentication to occur after client-ready message instead of during initial connection, allowing time for proper initialization before password prompt appears.
Server-Side Password Validation: Enhanced
PlayerAuthenticationto validate password hashes before Steam ticket validation, with configurable password protection via serverPassword config field.Client State Cleanup: Improved connection and UI state management on disconnect, including proper cleanup of password dialog state, time scale restoration, and connection flags.
Enhanced Logging: Updated message routing and connection handling with improved debug logging for authentication flow and client readiness status.
Development Tooling: Added launch configuration (launchSettings.json) and PowerShell/batch scripts (run.ps1, run.bat) for streamlined debug server/client startup workflow.
Contribution Summary by Author