From 543b185f7ef722f185726a18a467be897b024150 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 22 Feb 2026 23:07:31 -0500 Subject: [PATCH] fix: address code quality issues from review - Make ConnectReceived thread-safe with Volatile.Read/Write (accessed from auth timeout task and command pipeline) - Include authTimeoutTask in Task.WhenAny to propagate exceptions - Clear nonce after authentication with CryptographicOperations.ZeroMemory - Avoid closure allocation on publish permission cache hot path (method group) - Update AuthTimeout default to 2s to match Go server --- src/NATS.Server/Auth/ClientPermissions.cs | 2 +- src/NATS.Server/NatsClient.cs | 17 ++++++++++++++--- src/NATS.Server/NatsOptions.cs | 2 +- tests/NATS.Server.Tests/AuthConfigTests.cs | 2 +- 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/NATS.Server/Auth/ClientPermissions.cs b/src/NATS.Server/Auth/ClientPermissions.cs index 63aa3b1..d773fb1 100644 --- a/src/NATS.Server/Auth/ClientPermissions.cs +++ b/src/NATS.Server/Auth/ClientPermissions.cs @@ -34,7 +34,7 @@ public sealed class ClientPermissions : IDisposable if (_publish == null) return true; - return _pubCache.GetOrAdd(subject, s => _publish.IsAllowed(s)); + return _pubCache.GetOrAdd(subject, _publish.IsAllowed); } public bool IsSubscribeAllowed(string subject, string? queue = null) diff --git a/src/NATS.Server/NatsClient.cs b/src/NATS.Server/NatsClient.cs index e69e3d5..473c52c 100644 --- a/src/NATS.Server/NatsClient.cs +++ b/src/NATS.Server/NatsClient.cs @@ -1,6 +1,7 @@ using System.Buffers; using System.IO.Pipelines; using System.Net.Sockets; +using System.Security.Cryptography; using System.Text; using System.Text.Json; using Microsoft.Extensions.Logging; @@ -40,9 +41,12 @@ public sealed class NatsClient : IDisposable public ulong Id { get; } public ClientOptions? ClientOpts { get; private set; } public IMessageRouter? Router { get; set; } - public bool ConnectReceived { get; private set; } public Account? Account { get; private set; } + // Thread-safe: read from auth timeout task on threadpool, written from command pipeline + private int _connectReceived; + public bool ConnectReceived => Volatile.Read(ref _connectReceived) != 0; + // Stats public long InMsgs; public long OutMsgs; @@ -106,7 +110,10 @@ public sealed class NatsClient : IDisposable var processTask = ProcessCommandsAsync(pipe.Reader, _clientCts.Token); var pingTask = RunPingTimerAsync(_clientCts.Token); - await Task.WhenAny(fillTask, processTask, pingTask); + if (authTimeoutTask != null) + await Task.WhenAny(fillTask, processTask, pingTask, authTimeoutTask); + else + await Task.WhenAny(fillTask, processTask, pingTask); } catch (OperationCanceledException) { @@ -258,6 +265,10 @@ public sealed class NatsClient : IDisposable } _logger.LogDebug("Client {ClientId} authenticated as {Identity}", Id, result.Identity); + + // Clear nonce after use — defense-in-depth against memory dumps + if (_nonce != null) + CryptographicOperations.ZeroMemory(_nonce); } // If no account was assigned by auth, assign global account @@ -267,7 +278,7 @@ public sealed class NatsClient : IDisposable Account.AddClient(Id); } - ConnectReceived = true; + Volatile.Write(ref _connectReceived, 1); _logger.LogDebug("CONNECT received from client {ClientId}, name={ClientName}", Id, ClientOpts?.Name); } diff --git a/src/NATS.Server/NatsOptions.cs b/src/NATS.Server/NatsOptions.cs index d37c7a1..c641fe3 100644 --- a/src/NATS.Server/NatsOptions.cs +++ b/src/NATS.Server/NatsOptions.cs @@ -26,5 +26,5 @@ public sealed class NatsOptions public string? NoAuthUser { get; set; } // Auth timing - public TimeSpan AuthTimeout { get; set; } = TimeSpan.FromSeconds(1); + public TimeSpan AuthTimeout { get; set; } = TimeSpan.FromSeconds(2); } diff --git a/tests/NATS.Server.Tests/AuthConfigTests.cs b/tests/NATS.Server.Tests/AuthConfigTests.cs index b1798cd..f52d253 100644 --- a/tests/NATS.Server.Tests/AuthConfigTests.cs +++ b/tests/NATS.Server.Tests/AuthConfigTests.cs @@ -16,6 +16,6 @@ public class AuthConfigTests opts.Users.ShouldBeNull(); opts.NKeys.ShouldBeNull(); opts.NoAuthUser.ShouldBeNull(); - opts.AuthTimeout.ShouldBe(TimeSpan.FromSeconds(1)); + opts.AuthTimeout.ShouldBe(TimeSpan.FromSeconds(2)); } }