diff --git a/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs b/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs
index d2fee6a07..470741444 100644
--- a/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs
+++ b/Projects/UOContent.Tests/Tests/Network/Packets/AuthIdTests.cs
@@ -123,10 +123,32 @@ 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.
+ ///
[Fact]
- public void IsSpentEvenWhenTheAccountDoesNotMatch()
+ public void SurvivesAnAttemptFromTheWrongAddress()
{
- var account = CreateAccount("authid-spent-user");
+ var account = CreateAccount("authid-not-burned-address-user");
+ var authId = Register(account, AddressX);
+
+ Assert.Equal(
+ IncomingAccountPackets.AuthIdResult.Rejected,
+ IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressY, out _)
+ );
+
+ Assert.Equal(1, IncomingAccountPackets.AuthIdWindowCount);
+ Assert.Equal(
+ IncomingAccountPackets.AuthIdResult.Vouched,
+ IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressX, out _)
+ );
+ }
+
+ [Fact]
+ public void SurvivesAnAttemptForTheWrongAccount()
+ {
+ var account = CreateAccount("authid-not-burned-account-user");
var authId = Register(account, AddressX);
Assert.Equal(
@@ -134,13 +156,52 @@ public class AuthIdTests : IDisposable
IncomingAccountPackets.ConsumeAuthId(authId, "not-the-owner", AddressX, out _)
);
- // Spent regardless, so a guessed id cannot be reused to probe for its owner.
+ Assert.Equal(1, IncomingAccountPackets.AuthIdWindowCount);
Assert.Equal(
- IncomingAccountPackets.AuthIdResult.Rejected,
+ IncomingAccountPackets.AuthIdResult.Vouched,
IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressX, out _)
);
}
+ [Fact]
+ public void ARejectedAttemptYieldsNoEntry()
+ {
+ var account = CreateAccount("authid-no-leak-user");
+ var authId = Register(account, AddressX);
+
+ IncomingAccountPackets.ConsumeAuthId(authId, "not-the-owner", AddressX, out var entry);
+
+ 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()
+ {
+ var account = CreateAccount("authid-expired-spent-user");
+ var authId = Register(account, AddressX);
+
+ var now = Core._now;
+
+ try
+ {
+ Core._now = now + TimeSpan.FromMinutes(30.0);
+
+ Assert.Equal(
+ IncomingAccountPackets.AuthIdResult.Expired,
+ IncomingAccountPackets.ConsumeAuthId(authId, account.Username, AddressX, out _)
+ );
+ Assert.Equal(0, IncomingAccountPackets.AuthIdWindowCount);
+ }
+ finally
+ {
+ Core._now = now;
+ }
+ }
+
///
/// 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
diff --git a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs
index 236deb362..a2217b24f 100644
--- a/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs
+++ b/Projects/UOContent/Network/Packets/IncomingAccountPackets.cs
@@ -398,21 +398,27 @@ public static class IncomingAccountPackets
int authId, string username, IPAddress address, out AuthIDPersistence entry
)
{
- if (!_authIDWindow.Remove(authId, out entry))
+ if (!_authIDWindow.TryGetValue(authId, out entry))
{
return AuthIdResult.Rejected;
}
- // Address before age: a different address is a different client, however fresh the id is.
- if (!Utility.Intern(address).Equals(entry.Address))
+ // 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.
+ if (!Utility.Intern(address).Equals(entry.Address)
+ || entry.Account == null
+ || !username.InsensitiveEquals(entry.Account.Username))
{
+ entry = default;
return AuthIdResult.Rejected;
}
- if (entry.Account == null || !username.InsensitiveEquals(entry.Account.Username))
- {
- 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.
+ _authIDWindow.Remove(authId);
return Core.Now - entry.Age > _authIDLifetime ? AuthIdResult.Expired : AuthIdResult.Vouched;
}