Harden auth validation against null request fields
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
namespace GameList.Contracts;
|
||||
|
||||
public record RegisterRequest(string Username, string Password, string? DisplayName, string? AdminKey);
|
||||
public record RegisterRequest(string? Username, string? Password, string? DisplayName, string? AdminKey);
|
||||
|
||||
public record LoginRequest(string Username, string Password);
|
||||
public record LoginRequest(string? Username, string? Password);
|
||||
|
||||
@@ -23,7 +23,7 @@ public static class AuthEndpoints
|
||||
{
|
||||
if (!AuthValidator.TryValidateRegistration(request, out var validated, out var registrationError))
|
||||
{
|
||||
authAttemptMonitor.RecordFailure(ctx, "auth-register", request.Username.Trim(), "validation-failed");
|
||||
authAttemptMonitor.RecordFailure(ctx, "auth-register", NormalizeActor(request.Username), "validation-failed");
|
||||
return EndpointHelpers.BadRequestError(registrationError);
|
||||
}
|
||||
|
||||
@@ -31,7 +31,7 @@ public static class AuthEndpoints
|
||||
if (exists)
|
||||
return EndpointHelpers.ConflictError("Username already taken.");
|
||||
|
||||
var (hash, salt) = PasswordHasher.HashPassword(request.Password);
|
||||
var (hash, salt) = PasswordHasher.HashPassword(validated.Password);
|
||||
var expectedAdminKey = config["ADMIN_PASSWORD"];
|
||||
var wantsAdmin = !string.IsNullOrWhiteSpace(validated.AdminKey);
|
||||
if (wantsAdmin)
|
||||
@@ -99,12 +99,12 @@ public static class AuthEndpoints
|
||||
{
|
||||
if (!AuthValidator.TryValidateLogin(request, out _, out var normalizedUsername, out var loginError))
|
||||
{
|
||||
authAttemptMonitor.RecordFailure(ctx, "auth-login", request.Username.Trim(), "validation-failed");
|
||||
authAttemptMonitor.RecordFailure(ctx, "auth-login", NormalizeActor(request.Username), "validation-failed");
|
||||
return EndpointHelpers.BadRequestError(loginError);
|
||||
}
|
||||
|
||||
var player = await db.Players.FirstOrDefaultAsync(p => p.NormalizedUsername == normalizedUsername);
|
||||
if (player == null || !PasswordHasher.Verify(request.Password, player.PasswordHash, player.PasswordSalt))
|
||||
if (player == null || !PasswordHasher.Verify(request.Password ?? string.Empty, player.PasswordHash, player.PasswordSalt))
|
||||
{
|
||||
authAttemptMonitor.RecordFailure(ctx, "auth-login", normalizedUsername, "invalid-credentials");
|
||||
return EndpointHelpers.UnauthorizedError("Invalid username or password.");
|
||||
@@ -135,4 +135,6 @@ public static class AuthEndpoints
|
||||
return Results.NoContent();
|
||||
});
|
||||
}
|
||||
|
||||
private static string NormalizeActor(string? username) => string.IsNullOrWhiteSpace(username) ? "(missing)" : username.Trim();
|
||||
}
|
||||
|
||||
@@ -12,7 +12,7 @@ internal static class AuthValidator
|
||||
|
||||
public static bool TryValidateRegistration(RegisterRequest request, out ValidatedRegistration validated, out string error)
|
||||
{
|
||||
var username = (request.Username).Trim();
|
||||
var username = (request.Username ?? string.Empty).Trim();
|
||||
if (string.IsNullOrWhiteSpace(username) || username.Length > MaxUsernameLength)
|
||||
{
|
||||
validated = default;
|
||||
@@ -61,14 +61,14 @@ internal static class AuthValidator
|
||||
}
|
||||
|
||||
var adminKey = EndpointHelpers.TrimTo(request.AdminKey, MaxAdminKeyLength);
|
||||
validated = new ValidatedRegistration(username, username.ToLowerInvariant(), displayName, adminKey);
|
||||
validated = new ValidatedRegistration(username, username.ToLowerInvariant(), password, displayName, adminKey);
|
||||
error = string.Empty;
|
||||
return true;
|
||||
}
|
||||
|
||||
public static bool TryValidateLogin(LoginRequest request, out string username, out string normalizedUsername, out string error)
|
||||
{
|
||||
username = (request.Username).Trim();
|
||||
username = (request.Username ?? string.Empty).Trim();
|
||||
normalizedUsername = string.Empty;
|
||||
|
||||
if (string.IsNullOrWhiteSpace(username) || string.IsNullOrWhiteSpace(request.Password))
|
||||
@@ -94,5 +94,5 @@ internal static class AuthValidator
|
||||
return true;
|
||||
}
|
||||
|
||||
public readonly record struct ValidatedRegistration(string Username, string NormalizedUsername, string DisplayName, string? AdminKey);
|
||||
public readonly record struct ValidatedRegistration(string Username, string NormalizedUsername, string Password, string DisplayName, string? AdminKey);
|
||||
}
|
||||
|
||||
@@ -212,6 +212,29 @@ public class AuthTests
|
||||
Assert.Equal(HttpStatusCode.BadRequest, badKey.StatusCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Register_and_login_with_null_fields_return_bad_request()
|
||||
{
|
||||
await using var factory = new TestWebApplicationFactory();
|
||||
var client = factory.CreateClientWithCookies();
|
||||
|
||||
var register = await client.PostAsJsonAsync("/api/auth/register", new
|
||||
{
|
||||
Username = (string?)null,
|
||||
Password = (string?)null,
|
||||
DisplayName = (string?)null,
|
||||
AdminKey = (string?)null
|
||||
});
|
||||
Assert.Equal(HttpStatusCode.BadRequest, register.StatusCode);
|
||||
|
||||
var login = await client.PostAsJsonAsync("/api/auth/login", new
|
||||
{
|
||||
Username = (string?)null,
|
||||
Password = (string?)null
|
||||
});
|
||||
Assert.Equal(HttpStatusCode.BadRequest, login.StatusCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Non_admin_cannot_access_admin_routes()
|
||||
{
|
||||
|
||||
1
TESTS.md
1
TESTS.md
@@ -33,6 +33,7 @@ stateDiagram-v2
|
||||
### 1) Authentication & Identity
|
||||
- Register success (player, admin key path) issues cookie, trims fields, stores normalized username, hashes password.
|
||||
- Register rejects missing/long username, weak password policy violations, missing display name, duplicate username, bad admin key, >24 chars username, >16 display name.
|
||||
- Register/login null payload fields fail closed with `400` (no `500` on malformed JSON bodies).
|
||||
- Bootstrap-admin key path only works until the owner account exists; bootstrap admin is marked as owner.
|
||||
- Database uniqueness guard enforces single owner row (`IsOwner=true`) even if writes bypass endpoint-level checks.
|
||||
- `/api/auth/options` reports owner presence for registration UI behavior.
|
||||
|
||||
Reference in New Issue
Block a user