From ee1531f272f73feee3efce4d1d8b5e5f582396f9 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Sat, 8 Aug 2026 09:20:59 -0700 Subject: [PATCH] docs: trim the auth id comments to what is not in the code Several comments narrated the reasoning that produced the code rather than what a reader needs. Two also outlived their subject: ConsumeAuthId's summary and its call site both still claimed the id is spent whether or not it vouches, which stopped being true when ownership checks moved ahead of the removal. --- .../Fixtures/TestServerInitializer.cs | 3 +- .../Tests/Accounting/AccountPasswordTests.cs | 6 +- .../Security/PasswordProtectionTest.cs | 7 +- .../Tests/Network/Packets/AuthIdTests.cs | 35 ++------- .../UOContent/Accounting/AccountHandler.cs | 5 +- .../Security/Argon2PasswordProtection.cs | 5 +- Projects/UOContent/Network/GameServer.cs | 4 +- .../Network/Packets/IncomingAccountPackets.cs | 75 ++++++++----------- 8 files changed, 52 insertions(+), 88 deletions(-) diff --git a/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs b/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs index 1325700d5..a38af9c3d 100644 --- a/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs +++ b/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs @@ -100,8 +100,7 @@ internal static class TestServerInitializer } World.Configure(); - // Registers the Accounts entity persistence, without which Accounts.NewAccount cannot - // resolve and no test can construct an Account. + // Registers the Accounts entity persistence; without it no test can construct an Account. Server.Accounting.Accounts.Configure(); RaceDefinitions.Configure(); MovementImpl.Configure(); diff --git a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs index 14e6089cd..29a1b818f 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs +++ b/Projects/UOContent.Tests/Tests/Accounting/AccountPasswordTests.cs @@ -30,9 +30,9 @@ public class AccountPasswordTests : IDisposable Assert.False(account.CheckPassword("wrong-password")); } - // SetPassword assigns PasswordAlgorithm before deriving the phrase from it. Reversing those two - // lines salts the hash by the outgoing algorithm's rule and stores it under the incoming one, - // which verifies once and then never again. + // SetPassword assigns PasswordAlgorithm before deriving the phrase from it. Reversed, the hash + // is salted by the outgoing algorithm's rule but stored under the incoming one, which verifies + // once and then never again. [Theory] [InlineData(PasswordProtectionAlgorithm.SHA1)] [InlineData(PasswordProtectionAlgorithm.SHA2)] diff --git a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs index 333c35692..b86df418d 100644 --- a/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs +++ b/Projects/UOContent.Tests/Tests/Accounting/Security/PasswordProtectionTest.cs @@ -75,7 +75,7 @@ public class PasswordProtectionTest Assert.False(passwordProtection.ValidatePassword(encryptedPassword, "Not the same password")); } - // The shipping default before this change. A literal, so it cannot drift with the configured + // The shipping default before this change, as a literal so it cannot drift with the configured // defaults. Password: "hunter2". private const string LegacyArgon2iHash = "$argon2i$v=19$m=8192,t=3,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4gWnB4jDef3x6WhLdVE2LFw"; @@ -105,9 +105,8 @@ public class PasswordProtectionTest Assert.Equal(expected, Argon2PasswordProtection.Instance.NeedsRehash(hash)); } - // Digest and salt lengths are the decoded sizes of the base64 segments, not parameter-list - // entries, so they need their own literals. Current type and cost throughout; only a length - // differs from the defaults. The theory above is the negative control at default lengths. + // Digest and salt lengths are decoded base64 sizes rather than parameter-list entries, so they + // need their own literals. Current type and cost throughout; only a length differs. [Theory] // 16-byte digest: 22 base64 chars instead of the 43 a 32-byte digest encodes to. [InlineData("$argon2id$v=19$m=16384,t=1,p=1$LD1XJz7P3wQmIJ+Tu6ScgA$NO5hBABsHQ172C5nDO2X4g")] diff --git a/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs b/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs index 470741444..456eb174a 100644 --- a/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs +++ b/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs @@ -123,10 +123,8 @@ public class AuthIdTests : IDisposable ); } - /// - /// A rejected attempt must not consume the id. Anyone landing on a live id could otherwise burn - /// it, and its owner would arrive to "unable to find auth id" and have to log in again. - /// + // A rejected attempt must not consume the id, or anyone landing on a live one could burn it and + // force its owner to log in again. [Fact] public void SurvivesAnAttemptFromTheWrongAddress() { @@ -174,10 +172,6 @@ public class AuthIdTests : IDisposable Assert.Null(entry.Account); } - /// - /// An expired id is still spent by its owner: they fall back to the password verify, and the id - /// has done everything it is ever going to do. - /// [Fact] public void AnExpiredIdIsSpentByItsOwner() { @@ -202,11 +196,8 @@ public class AuthIdTests : IDisposable } } - /// - /// An expired id is not a lockout. A player can sit on the server list, and before any of this - /// existed the game login always verified the password anyway -- so falling back to that verify - /// is the behaviour we started from, not a regression. - /// + // Expiry is not a lockout. The game login always verified the password before any of this + // existed, so falling back to that verify is the behaviour we started from. [Fact] public void ExpiresIntoAPasswordVerifyRatherThanARejection() { @@ -275,11 +266,8 @@ public class AuthIdTests : IDisposable Assert.Equal(1, IncomingAccountPackets.AuthIdWindowCount); } - /// - /// A connection gets exactly one id, no matter how many times it re-selects. Handing the same - /// one back rather than minting another is what makes an orphaned id impossible, instead of - /// something to clean up afterwards. - /// + // Handing the same id back rather than minting another is what makes an orphan impossible, + // instead of something to clean up afterwards. [Fact] public void ReSelectingReturnsTheSameIdAndAddsNothingToTheWindow() { @@ -298,10 +286,6 @@ public class AuthIdTests : IDisposable ); } - /// - /// An id is only left behind when a client picks a server and never arrives. Issuing sweeps - /// those, so they do not accumulate. - /// [Fact] public void AbandonedIdsAreSweptWhenNewOnesAreIssued() { @@ -335,11 +319,8 @@ public class AuthIdTests : IDisposable } } - /// - /// A login rush is not a backlog. Every one of these ids belongs to a client on its way to - /// redeem it, so none may be discarded to keep the window at some arbitrary size -- doing so - /// hands a real player "unable to find auth id" and a disconnect. - /// + // A login rush is not a backlog. Every id belongs to a client on its way to redeem it, so none + // may be discarded to hold the window at some arbitrary size. [Fact] public void ALoginRushDoesNotEvictAnyonesAuthId() { diff --git a/Projects/UOContent/Accounting/AccountHandler.cs b/Projects/UOContent/Accounting/AccountHandler.cs index 1896883cf..542e16a40 100644 --- a/Projects/UOContent/Accounting/AccountHandler.cs +++ b/Projects/UOContent/Accounting/AccountHandler.cs @@ -343,9 +343,8 @@ public static class AccountHandler logger.Information("Login: {NetState} Access denied for '{Username}'", e.State, un); e.Accepted = false; } - // The auth id already vouched for this account from this address, and it was only issued - // after the account login packet verified the password. Re-deriving the hash here costs - // another full Argon2 verify to answer a question already answered. + // The auth id was only issued after the account login packet verified this password, so + // re-deriving the hash costs a second Argon2 verify to answer the same question. else if (!e.PreAuthenticated && !acct.CheckPassword(pw)) { logger.Information("Login: {NetState} Invalid password for '{Username}'", e.State, un); diff --git a/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs b/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs index 3b509e099..0a952117d 100644 --- a/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs +++ b/Projects/UOContent/Accounting/Security/Argon2PasswordProtection.cs @@ -37,9 +37,8 @@ public class Argon2PasswordProtection : IPasswordProtection public bool ValidatePassword(string encryptedPassword, string plainPassword) => _passwordHasher.Verify(encryptedPassword, plainPassword); - // The PHC string carries the parameters it was hashed with, so verification uses those rather - // than the configured ones. Comparing them is what lets a parameter change reach existing - // accounts. + // Verification uses the parameters embedded in the PHC string, not the configured ones, so + // comparing them is what lets a parameter change reach existing accounts. public bool NeedsRehash(string encryptedPassword) { // Unparseable but verified: a format this build does not understand, so rewrite it. diff --git a/Projects/UOContent/Network/GameServer.cs b/Projects/UOContent/Network/GameServer.cs index 72f886164..b9de09515 100644 --- a/Projects/UOContent/Network/GameServer.cs +++ b/Projects/UOContent/Network/GameServer.cs @@ -16,8 +16,8 @@ public static partial class GameServer /// /// The auth id presented on this game login was issued to this account, from this address, - /// after the account login packet already verified the password. Read-only: a subscriber - /// must not be able to grant itself the skip. + /// after the account login packet verified the password. Read-only so a subscriber cannot + /// grant itself the skip. /// public bool PreAuthenticated { get; } diff --git a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs index a2217b24f..5ffe8f216 100644 --- a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs +++ b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs @@ -28,14 +28,14 @@ namespace Server.Network; public static class IncomingAccountPackets { - // Initial capacity and the point at which issuing sweeps for expired ids. Not a cap: a login - // rush must not be turned into "unable to find auth id" for whoever lands past the limit. + // Initial capacity and the point at which issuing sweeps expired ids. Not a cap; the window + // grows rather than evicting a live id. private const int _authIDWindowSize = 128; private static int _authIdPurgeThreshold = _authIDWindowSize; - // The gap between PlayServerAck and the client's game login is seconds. Two minutes is generous, - // and bounds how long a stolen id stays usable. + // The gap between PlayServerAck and the game login is seconds. Bounds how long a stolen id + // stays usable. private static readonly TimeSpan _authIDLifetime = TimeSpan.FromMinutes(2.0); private static readonly Dictionary _authIDWindow = @@ -46,8 +46,8 @@ public static class IncomingAccountPackets public DateTime Age; public readonly ClientVersion Version; - // The account and address that earned this id on the account login packet. GameLogin skips - // its own password verify when both match, so the id is a bearer token and must be bound. + // GameLogin skips its password verify when both match, so the id is a bearer token and has + // to be bound to whatever earned it. public readonly IAccount Account; public readonly IPAddress Address; @@ -62,13 +62,11 @@ public static class IncomingAccountPackets internal enum AuthIdResult { - // No such id, or it was issued for a different account or address. A client that presents - // one of those is not one of ours; nothing here is worth a password check. + // No such id, or it was issued for a different account or address. Rejected, - // Right account, right address, but too old to stand in for the verify. A player can idle - // on the server list, so this is a normal thing to do -- fall back to checking the password - // rather than turning it into a lockout. + // Right account and address, too old to stand in for the verify. Idling on the server list + // is normal, so this falls back to the password check rather than becoming a lockout. Expired, // Issued to this account, from this address, recently. Stands in for the password verify. @@ -350,10 +348,9 @@ public static class IncomingAccountPackets EnsureAuthId(state.AuthId, state.Account, state.Address, state.Version); /// - /// One id per connection, by construction. Choosing a server queues a disconnect, but the queue - /// is drained on the next slice, so a client that pipelines another seed/login/select into the - /// same buffer arrives here again. Handing back the id it already holds -- rather than minting a - /// second and orphaning the first -- means no amount of re-selecting can leave anything behind. + /// One id per connection, by construction. Choosing a server queues a disconnect that is not + /// drained until the next slice, so a client pipelining another select into the same buffer + /// arrives here again; handing back the id it already holds cannot orphan one. /// internal static int EnsureAuthId( int existingAuthId, IAccount account, IPAddress address, ClientVersion version @@ -361,12 +358,10 @@ public static class IncomingAccountPackets internal static int RegisterAuthId(IAccount account, IPAddress address, ClientVersion version) { - // An id is issued when a server is picked and consumed by the game login seconds later, so - // the only ones left behind belong to clients that picked a server and never arrived. Sweep - // those, but never evict a live id to make room: the client holding it is on its way to - // redeem it, and taking it away hands a real player a failed login. If everything is still - // live the window simply grows -- that is a login rush, not a backlog. Creating an entry - // costs a successful password verify, so the size is self-limiting. + // Sweep the ids left behind by clients that picked a server and never arrived, but never + // evict a live one to make room -- the client holding it is on its way to redeem it. If all + // are live the window grows, which is a login rush, not a backlog. Each entry costs a + // successful password verify, so the size is self-limiting. if (_authIDWindow.Count >= _authIdPurgeThreshold) { PurgeExpiredAuthIds(); @@ -375,9 +370,8 @@ public static class IncomingAccountPackets int authID; - // A cryptographic draw across the whole int range: the id stands in for a password verify, - // so it has to be unguessable. Zero is reserved -- GameLogin reads state.AuthId == 0 as - // "no auth id was issued". + // The id stands in for a password verify, so it has to be unguessable. Zero is reserved: + // GameLogin reads state.AuthId == 0 as "no auth id was issued". do { authID = RandomNumberGenerator.GetInt32(int.MinValue, int.MaxValue); @@ -389,10 +383,9 @@ public static class IncomingAccountPackets } /// - /// Looks up and spends an auth id. The id is removed whether or not it vouches, so a guessed id - /// cannot be reused to enumerate usernames. An address mismatch is - /// rather than a fallback: network switching mid-login is - /// not supported. + /// Spends an auth id, but only for the account and address it was issued to. An address + /// mismatch is rather than a fallback: network switching + /// mid-login is not supported. /// internal static AuthIdResult ConsumeAuthId( int authId, string username, IPAddress address, out AuthIDPersistence entry @@ -403,11 +396,9 @@ public static class IncomingAccountPackets return AuthIdResult.Rejected; } - // Look, then take. Removing before the presenter has shown the id is theirs would let anyone - // who lands on a live id burn it, and its owner would arrive to "unable to find auth id" and - // have to log in again. Address first: a different address is a different client however - // fresh the id is, and checking it before the username means a remote guesser never learns - // whether a username matched. + // Look, then take: removing before ownership is proven would let anyone landing on a live id + // burn it, leaving its owner to log in again. Address before username, so a remote guesser + // never learns whether a username matched. if (!Utility.Intern(address).Equals(entry.Address) || entry.Account == null || !username.InsensitiveEquals(entry.Account.Username)) @@ -416,8 +407,7 @@ public static class IncomingAccountPackets return AuthIdResult.Rejected; } - // Theirs, so spend it. Expired still counts as spent -- they fall back to the password - // verify and the id has done all it is ever going to do. + // Theirs, so spend it. Expired counts as spent; it has done all it is ever going to do. _authIDWindow.Remove(authId); return Core.Now - entry.Age > _authIDLifetime ? AuthIdResult.Expired : AuthIdResult.Vouched; @@ -427,8 +417,7 @@ public static class IncomingAccountPackets { var now = Core.Now; - // Removing during enumeration is supported on Dictionary since .NET Core 3.0, so this - // reclaims in one pass with no scratch list. + // Dictionary supports removal during enumeration, so this reclaims in one pass. foreach (var (key, entry) in _authIDWindow) { if (now - entry.Age > _authIDLifetime) @@ -468,7 +457,6 @@ public static class IncomingAccountPackets var username = reader.ReadLatin1Safe(30); var password = reader.ReadLatin1Safe(30); - // Spends the id either way, so a guessed one cannot be reused to probe usernames. var authResult = ConsumeAuthId(authId, username, state.Address, out var ap); if (authResult == AuthIdResult.Rejected) @@ -481,8 +469,7 @@ public static class IncomingAccountPackets state.Version = ap.Version; state.Seeded = true; - // Expired still carries a usable entry, and the client just pays the password verify it - // would have paid before any of this existed. + // Expired carries a usable entry; only the password verify skip is withheld. var e = new GameServer.GameLoginEventArgs( state, username, @@ -510,8 +497,8 @@ public static class IncomingAccountPackets public static void PlayServer(NetState state, SpanReader reader) { - // A server is picked once per connection. Picking again would hand back an id this - // connection may already have spent on a game login, so the client could never redeem it. + // A server is picked once per connection. Picking again hands back an id this connection may + // already have spent on a game login, which the client could never redeem. if (state.AuthId != 0) { state.Disconnect("Duplicate play server packet sent."); @@ -539,8 +526,8 @@ public static class IncomingAccountPackets public static void LoginServerSeed(NetState state, SpanReader reader) { - // Seeding happens once per connection. A second one means the client is restarting a - // handshake it already completed, which no real client does. + // Seeding happens once per connection. A second one restarts a handshake this connection + // already completed, which no real client does. if (state.Seeded) { state.Disconnect("Duplicate login server seed packet sent.");