fix: security + robustness audit (auth bypass, server-list DoS, JSON injection, input validation)

.Tests/XUnit.Tests/GameServerAttributeTest.cs

@@ -1,129 +1,80 @@
-/*
-using System.Text;
-using System.Text.Json;
+using System.Text.Json.Nodes;
 using UT4MasterServer.Models;
 
 namespace XUnit.Tests;
 
 public class GameServerAttributeTest
 {
-	public static TheoryData<object?, object?, bool, bool, bool> TestCases = new()
+	[Fact]
+	public void ToJObject_EmitsEachStoredType()
 	{
-		{ "3", "3", true, false, true },
-		{ "3", 3, false, false, false },
-		{ "3", true, false, false, false },
-		{ "3", false, false, false, false },
-		{ "3", null, false, false, false },
-		{ "2", "3", false, true, true },
-		{ "4", "3", false, false, false },
-
-		{ 3, "3", false, false, false },
-		{ 3, 3, true, false, true },
-		{ 3, true, false, false, false },
-		{ 3, false, false, false, false },
-		{ 3, null, false, false, false },
-
-		{ true, true, true, false, true },
-		{ true, "true", false, false, false },
-		{ true, 1, false, false, false },
-		{ true, null, false, false, false },
-		{ false, false, true, false, true },
-		{ false, null, false, false, false },
-		{ true, false, false, false, false },
-		{ false, true, false, true, true },
-
-		{ " ", "", false, false, false },
-		{ " ", " ", true, false, true },
-		{ "", " ", false, true, true },
-	};
-
-	public static TheoryData<object?, bool, bool, bool> TestCasesNull = new()
+		var attributes = new GameServerAttributes();
+		attributes.Set("UT_SERVERNAME_s", "My Server");
+		attributes.Set("UT_RANKED_i", 1);
+		attributes.Set("UT_PRIVATE_b", true);
+
+		JsonObject obj = attributes.ToJObject();
+
+		Assert.Equal(3, obj.Count);
+		Assert.Equal("My Server", obj["UT_SERVERNAME_s"]!.GetValue<string>());
+		Assert.Equal(1, obj["UT_RANKED_i"]!.GetValue<int>());
+		Assert.True(obj["UT_PRIVATE_b"]!.GetValue<bool>());
+	}
+
+	[Fact]
+	public void ToJObject_KeyWithoutTypeSuffix_DoesNotThrow()
 	{
+		var attributes = new GameServerAttributes();
+		attributes.Set("CUSTOMKEY", "value");
+
+		// used to throw while serializing the server list because the key
+		// does not end in a known type suffix
+		JsonObject obj = attributes.ToJObject();
 
-		{ null, true, false, true },
-		{ true, false, false, false },
-		{ 3, false, false, false },
-		{ "null", false, false, false },
-	};
+		Assert.Equal("value", obj["CUSTOMKEY"]!.GetValue<string>());
+	}
 
-	[Theory]
-	[MemberData(nameof(TestCases))]
-	public void TestAttributesNonNull(object? attrValue, object? compareValue, bool expectedEq, bool expectedLt, bool expectedLte)
+	[Fact]
+	public void ToJObject_KeyWithMismatchedTypeSuffix_EmitsActualStoredType()
 	{
-		JsonElement jsonElem = CreateJsonElement(compareValue);
-		var gsa = new GameServerAttributes();
-		if (attrValue is string attrValueString)
-		{
-			gsa.Set("key", attrValueString);
-		}
-		else if (attrValue is int attrValueInt)
-		{
-			gsa.Set("key", attrValueInt);
-		}
-		else if (attrValue is bool attrValueBool)
-		{
-			gsa.Set("key", attrValueBool);
-		}
-		else
-		{
-			Assert.Fail("undesired test case");
-		}
-
-		Assert.Equal(expectedEq, gsa.Eq("key", jsonElem));
-		Assert.Equal(expectedLt, gsa.Lt("key", jsonElem));
-		Assert.Equal(expectedLte, gsa.Lte("key", jsonElem));
+		var attributes = new GameServerAttributes();
+		attributes.Set("UT_MISMATCH_i", "not an int");
+		attributes.Set("UT_MISMATCH_s", 42);
+		attributes.Set("UT_MISMATCH_b", 7);
+
+		// used to throw an invalid cast because the value type did not
+		// match the key suffix
+		JsonObject obj = attributes.ToJObject();
+
+		Assert.Equal("not an int", obj["UT_MISMATCH_i"]!.GetValue<string>());
+		Assert.Equal(42, obj["UT_MISMATCH_s"]!.GetValue<int>());
+		Assert.Equal(7, obj["UT_MISMATCH_b"]!.GetValue<int>());
 	}
 
-	[Theory]
-	[MemberData(nameof(TestCasesNull))]
-	public void TestAttributesNull(object? compareValue, bool expectedEq, bool expectedLt, bool expectedLte)
+	[Fact]
+	public void ToJObject_MixedAttributes_SerializesToJson()
 	{
-		JsonElement jsonElem = CreateJsonElement(compareValue);
-		var gsa = new GameServerAttributes();
-		gsa.Set("key", null as string);
-		Assert.Equal(expectedEq, gsa.Eq("key", jsonElem));
-		Assert.Equal(expectedLt, gsa.Lt("key", jsonElem));
-		Assert.Equal(expectedLte, gsa.Lte("key", jsonElem));
-
-		gsa.Set("key", null as int?);
-		Assert.Equal(expectedEq, gsa.Eq("key", jsonElem));
-		Assert.Equal(expectedLt, gsa.Lt("key", jsonElem));
-		Assert.Equal(expectedLte, gsa.Lte("key", jsonElem));
-
-		gsa.Set("key", null as bool?);
-		Assert.Equal(expectedEq, gsa.Eq("key", jsonElem));
-		Assert.Equal(expectedLt, gsa.Lt("key", jsonElem));
-		Assert.Equal(expectedLte, gsa.Lte("key", jsonElem));
+		var attributes = new GameServerAttributes();
+		attributes.Set("UT_SERVERNAME_s", "Server");
+		attributes.Set("UT_SERVERTRUSTLEVEL_i", 2);
+		attributes.Set("BADKEY", true);
+		attributes.Set("UT_WEIRD_i", "string stored under int key");
+
+		var json = attributes.ToJObject().ToJsonString();
+
+		Assert.False(string.IsNullOrEmpty(json));
 	}
 
-	private static JsonElement CreateJsonElement(object? obj)
+	[Fact]
+	public void ToJObject_NullValue_RemovesAttribute()
 	{
-		StringBuilder sb = new();
-		if (obj is null)
-		{
-			sb.Append("null");
-		}
-		else if (obj is string objString)
-		{
-			sb.Append($"\"{objString}\"");
-		}
-		else if (obj is int objInt)
-		{
-			sb.Append(objInt.ToString());
-		}
-		else if (obj is bool objBool)
-		{
-			sb.Append(objBool ? "true" : "false");
-		}
-		else
-		{
-			Assert.Fail("undesired test case");
-		}
-
-		var utf8 = Encoding.UTF8.GetBytes(sb.ToString());
-
-		Utf8JsonReader jsonReader = new(utf8);
-		return JsonElement.ParseValue(ref jsonReader);
+		var attributes = new GameServerAttributes();
+		attributes.Set("UT_SERVERNAME_s", "Server");
+		attributes.Set("UT_SERVERNAME_s", (string?)null);
+
+		JsonObject obj = attributes.ToJObject();
+
+		Assert.False(attributes.Contains("UT_SERVERNAME_s"));
+		Assert.Empty(obj);
 	}
 }
-*/

.Tests/XUnit.Tests/RatingsControllerTest.cs

@@ -0,0 +1,25 @@
+using UT4MasterServer.Controllers.UT;
+
+namespace XUnit.Tests;
+
+public class RatingsControllerTest
+{
+	[Theory]
+	[InlineData(0, 10, 0, 10)]      // in range, unchanged
+	[InlineData(50, 100, 50, 100)]  // upper bounds, unchanged
+	[InlineData(-5, 10, 0, 10)]     // negative skip clamps to 0
+	[InlineData(-1, 10, 0, 10)]
+	[InlineData(0, 0, 0, 100)]      // limit below range falls back to default
+	[InlineData(0, -20, 0, 100)]
+	[InlineData(0, 101, 0, 100)]    // limit above range falls back to default
+	[InlineData(0, 1000, 0, 100)]
+	[InlineData(-3, 5000, 0, 100)]  // both out of range
+	[InlineData(0, 1, 0, 1)]        // lower limit bound, unchanged
+	public void ClampPaging_KeepsValuesWithinSaneBounds(int skip, int limit, int expectedSkip, int expectedLimit)
+	{
+		var (clampedSkip, clampedLimit) = RatingsController.ClampPaging(skip, limit);
+
+		Assert.Equal(expectedSkip, clampedSkip);
+		Assert.Equal(expectedLimit, clampedLimit);
+	}
+}

.Tests/XUnit.Tests/StatisticBaseInputFormatterTest.cs

@@ -0,0 +1,65 @@
+using UT4MasterServer.Formatters;
+using UT4MasterServer.Models.Database;
+
+namespace XUnit.Tests;
+
+public class StatisticBaseInputFormatterTest
+{
+	[Theory]
+	[InlineData("")]
+	[InlineData("\0")]
+	[InlineData("\0\0\0")]
+	[InlineData("   ")]
+	[InlineData(" \t\r\n")]
+	[InlineData("   \0\0")]
+	[InlineData("null")]
+	[InlineData("null\0")]
+	[InlineData("{ not valid json")]
+	[InlineData("[]")]
+	[InlineData("\"just a string\"")]
+	public void TryParse_InvalidBody_ReturnsFalse(string rawValue)
+	{
+		var ok = StatisticBaseInputFormatter.TryParse(rawValue, out StatisticBase? result);
+
+		Assert.False(ok);
+		Assert.Null(result);
+	}
+
+	[Fact]
+	public void TryParse_ValidBodyWithTrailingNul_ReturnsParsedStatistic()
+	{
+		// the game terminates the json body with a trailing NUL character
+		const string rawValue = "{\"MatchesPlayed\":3,\"Kills\":15,\"Deaths\":7}\0";
+
+		var ok = StatisticBaseInputFormatter.TryParse(rawValue, out StatisticBase? result);
+
+		Assert.True(ok);
+		Assert.NotNull(result);
+		Assert.Equal(3, result!.MatchesPlayed);
+		Assert.Equal(15, result.Kills);
+		Assert.Equal(7, result.Deaths);
+	}
+
+	[Fact]
+	public void TryParse_ValidBodyWithoutTrailingNul_ReturnsParsedStatistic()
+	{
+		const string rawValue = "{\"Wins\":2,\"Losses\":1}";
+
+		var ok = StatisticBaseInputFormatter.TryParse(rawValue, out StatisticBase? result);
+
+		Assert.True(ok);
+		Assert.NotNull(result);
+		Assert.Equal(2, result!.Wins);
+		Assert.Equal(1, result.Losses);
+	}
+
+	[Fact]
+	public void TryParse_EmptyJsonObject_ReturnsEmptyStatistic()
+	{
+		var ok = StatisticBaseInputFormatter.TryParse("{}", out StatisticBase? result);
+
+		Assert.True(ok);
+		Assert.NotNull(result);
+		Assert.Null(result!.MatchesPlayed);
+	}
+}

UT4MasterServer.Models/GameServerAttributes.cs

@@ -73,24 +73,26 @@ public string[] GetKeys()
 
 	public JsonObject ToJObject()
 	{
-		var attrs = new KeyValuePair<string, JsonNode?>[serverConfigs.Count];
+		var attrs = new List<KeyValuePair<string, JsonNode?>>(serverConfigs.Count);
 
-		var i = 0;
 		foreach (KeyValuePair<string, object> kvp in serverConfigs)
 		{
-			if (kvp.Key.EndsWith("_b"))
+			// emit based on the actual stored type. keys without a known type
+			// suffix or with a value that mismatches their suffix used to leave
+			// a null-key entry or throw an invalid cast, crashing serialization
+			// of the entire server list.
+			if (kvp.Value is bool valueBool)
 			{
-				attrs[i] = new(kvp.Key, (bool)kvp.Value);
+				attrs.Add(new(kvp.Key, valueBool));
 			}
-			else if (kvp.Key.EndsWith("_i"))
+			else if (kvp.Value is int valueInt)
 			{
-				attrs[i] = new(kvp.Key, (int)kvp.Value);
+				attrs.Add(new(kvp.Key, valueInt));
 			}
-			else if (kvp.Key.EndsWith("_s"))
+			else if (kvp.Value is string valueString)
 			{
-				attrs[i] = new(kvp.Key, (string)kvp.Value);
+				attrs.Add(new(kvp.Key, valueString));
 			}
-			i++;
 		}
 
 		return new JsonObject(attrs);

UT4MasterServer.Services/Hosted/ApplicationBackgroundService.cs

@@ -49,32 +49,45 @@ private void DoWork(object? state)
 	{
 		Task.Run(async () =>
 		{
-			using IServiceScope? scope = services.CreateScope();
-
-			SessionService? sessionService = scope.ServiceProvider.GetRequiredService<SessionService>();
-			var deleteCount = await sessionService.RemoveAllExpiredSessionsAsync();
-			if (deleteCount > 0)
+			try
 			{
-				logger.LogInformation("Background task deleted {DeleteCount} expired sessions.", deleteCount);
+				await DoWorkAsync();
 			}
-
-			CodeService? codeService = scope.ServiceProvider.GetRequiredService<CodeService>();
-			deleteCount = await codeService.RemoveAllExpiredCodesAsync();
-			if (deleteCount > 0)
+			catch (Exception ex)
 			{
-				logger.LogInformation("Background task deleted {DeleteCount} expired codes.", deleteCount);
+				// without this, a failing cleanup pass faults the task and is silently discarded
+				logger.LogError(ex, "Background cleanup task failed.");
 			}
+		});
+	}
 
-			MatchmakingService? matchmakingService = scope.ServiceProvider.GetRequiredService<MatchmakingService>();
-			deleteCount = await matchmakingService.RemoveAllStaleAsync();
-			if (deleteCount > 0)
-			{
-				logger.LogInformation("Background task deleted {DeleteCount} stale game servers.", deleteCount);
-			}
+	private async Task DoWorkAsync()
+	{
+		using IServiceScope? scope = services.CreateScope();
 
-			await DeleteOldStatisticsAsync(scope);
-			await MergeOldStatisticsAsync(scope);
-		});
+		SessionService? sessionService = scope.ServiceProvider.GetRequiredService<SessionService>();
+		var deleteCount = await sessionService.RemoveAllExpiredSessionsAsync();
+		if (deleteCount > 0)
+		{
+			logger.LogInformation("Background task deleted {DeleteCount} expired sessions.", deleteCount);
+		}
+
+		CodeService? codeService = scope.ServiceProvider.GetRequiredService<CodeService>();
+		deleteCount = await codeService.RemoveAllExpiredCodesAsync();
+		if (deleteCount > 0)
+		{
+			logger.LogInformation("Background task deleted {DeleteCount} expired codes.", deleteCount);
+		}
+
+		MatchmakingService? matchmakingService = scope.ServiceProvider.GetRequiredService<MatchmakingService>();
+		deleteCount = await matchmakingService.RemoveAllStaleAsync();
+		if (deleteCount > 0)
+		{
+			logger.LogInformation("Background task deleted {DeleteCount} stale game servers.", deleteCount);
+		}
+
+		await DeleteOldStatisticsAsync(scope);
+		await MergeOldStatisticsAsync(scope);
 	}
 
 	public void Dispose()

UT4MasterServer.Services/Scoped/RatingsService.cs

@@ -144,9 +144,9 @@ public async Task<RatingResponse> GetAverageTeamRatingAsync(string ratingType, R
 		return new RankingsResponse()
 		{
 			Rank = rank,
-			AccountID = account.ID,
-			Player = account.Username,
-			CountryFlag = account.CountryFlag,
+			AccountID = accountID,
+			Player = account?.Username ?? UnknownUser,
+			CountryFlag = account?.CountryFlag ?? DefaultCountryFlag,
 			Rating = selectedRating.RatingValue / Rating.Precision,
 			GamesPlayed = selectedRating.GamesPlayed
 		};