fix(security): P0 remediation wave — SSRF guards, one-time-use, server-side passkey ceremony, lockout concurrency

src/dotnet/Modgud.Api.Tests/Audit/LoginFailureStreakEmissionTests.cs

@@ -8,7 +8,9 @@
 namespace Modgud.Api.Tests.Audit;
 
 /// <summary>
-/// Proves the Phase 1 emission logic in <c>EventSourcedUserStore.AppendSecurityChangeEvents</c>:
+/// Proves the Phase 1 emission logic in <c>EventSourcedUserStore.ResetAccessFailedCountAsync</c>
+/// (it moved there from the former <c>AppendSecurityChangeEvents</c> diff when the
+/// lockout counter was made concurrency-safe — see P0-4 / LockoutConcurrencyTests):
 /// a known-user failure streak is recorded as exactly ONE aggregated
 /// <see cref="UserLoginFailuresObservedEvent"/> when the access-failed counter
 /// resolves (>0 → 0), not one event per attempt (Decision (b)).

src/dotnet/Modgud.Api.Tests/Authorization/CocoarNativeGrantFlowTests.cs

@@ -259,6 +259,41 @@ public async Task Magic_Grant_TokenReuse_Rejected()
         Assert.Contains("invalid_grant", body);
     }
 
+    [Fact]
+    public async Task Magic_Grant_LinkAlreadyConsumedInWebFlow_Rejected()
+    {
+        // Cross-channel replay guard. The WEB redemption marks the challenge
+        // consumed (ConsumedAt) rather than deleting it — it has to, because
+        // Marten does not version-check deletes and only a version-checked Store
+        // makes the one-time use win a concurrency race. This native grant used to
+        // query by user+token-hash and check nothing but expiry, so a link already
+        // used in the browser stayed fully redeemable here.
+        await EnableNativeGrantsAsync();
+        await SeedNativeClientAsync("native-magic-app");
+
+        var (userId, token) = await SeedMagicLinkAsync(DefaultUser!.Id);
+
+        // Consume it exactly as the web flow does, without touching this grant.
+        var ct = TestContext.Current.CancellationToken;
+        using (var scope = NewSystemTenantScope())
+        {
+            var session = scope.ServiceProvider.GetRequiredService<IDocumentSession>();
+            var hash = MagicLinkChallenge.HashToken(token);
+            var challenge = await session.Query<MagicLinkChallenge>()
+                .FirstOrDefaultAsync(c => c.TokenHash == hash, ct);
+            Assert.NotNull(challenge);
+            challenge!.ConsumedAt = DateTimeOffset.UtcNow;
+            session.Store(challenge);
+            await session.SaveChangesAsync(ct);
+        }
+
+        var response = await PostMagicAsync(userId, token);
+
+        Assert.False(response.IsSuccessStatusCode);
+        var body = await response.Content.ReadAsStringAsync(ct);
+        Assert.Contains("invalid_grant", body);
+    }
+
     // ─────────────────────── Settings lifetime validation ─────────────────────
 
     [Fact]

src/dotnet/Modgud.Api.Tests/Authorization/CocoarPasskeyGrantFlowTests.cs

@@ -55,7 +55,7 @@ public async Task Begin_FlagOn_ReturnsCeremonyAndChallenge()
         Assert.False(string.IsNullOrEmpty(options.GetProperty("challenge").GetString()));
 
         // A single-use ceremony doc was persisted for the realm.
-        Assert.True(await CeremonyExistsAsync(ceremonyId));
+        Assert.True(await CeremonyIsRedeemableAsync(ceremonyId));
     }
 
     [Fact]
@@ -145,7 +145,7 @@ public async Task PasskeyGrant_ExpiredCeremony_InvalidGrant_AndConsumed()
         var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken);
         Assert.Contains("invalid_grant", body);
         // An expired ceremony is consumed (deleted) on the rejected path.
-        Assert.False(await CeremonyExistsAsync(ceremonyId));
+        Assert.False(await CeremonyIsRedeemableAsync(ceremonyId));
     }
 
     [Fact]
@@ -162,7 +162,7 @@ public async Task PasskeyGrant_BogusAssertion_InvalidGrant_AndConsumesCeremony()
         Assert.False(response.IsSuccessStatusCode);
         var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken);
         Assert.Contains("invalid_grant", body);
-        Assert.False(await CeremonyExistsAsync(ceremonyId));
+        Assert.False(await CeremonyIsRedeemableAsync(ceremonyId));
     }
 
     [Fact]
@@ -189,7 +189,7 @@ public async Task PasskeyGrant_MalformedAssertionMatchingCredential_InvalidGrant
         var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken);
         Assert.Contains("invalid_grant", body);
         // The ceremony is single-use — consumed even on this rejected path.
-        Assert.False(await CeremonyExistsAsync(ceremonyId));
+        Assert.False(await CeremonyIsRedeemableAsync(ceremonyId));
     }
 
     // ─────────────────── crypto success path (software authenticator) ─────────
@@ -221,8 +221,8 @@ public async Task PasskeyGrant_ValidAssertion_NativeOrigin_MintsTokens()
         Assert.True(json.RootElement.TryGetProperty("refresh_token", out var rt) && !string.IsNullOrEmpty(rt.GetString()));
         Assert.False(response.Headers.Contains("Set-Cookie"));
 
-        // The ceremony is single-use — gone even on the success path.
-        Assert.False(await CeremonyExistsAsync(Guid.Parse(ceremonyId)));
+        // The ceremony is single-use — no longer redeemable, even on success.
+        Assert.False(await CeremonyIsRedeemableAsync(Guid.Parse(ceremonyId)));
     }
 
     [Fact]
@@ -466,12 +466,19 @@ private async Task SeedCredentialAsync(byte[] credentialId, byte[]? publicKey =
         return all.FirstOrDefault(c => c.CredentialId.SequenceEqual(credentialId));
     }
 
-    private async Task<bool> CeremonyExistsAsync(Guid ceremonyId)
+    /// <summary>
+    /// Asserts the security property (the ceremony can still be redeemed) rather
+    /// than the storage mechanism. Consuming is a version-checked Store of
+    /// <c>ConsumedAt</c>, not a Delete — Marten does not version-check deletes, so
+    /// a Delete would let two concurrent redemptions of one ceremony_id both mint
+    /// a token. A consumed row therefore still exists but must never be usable.
+    /// </summary>
+    private async Task<bool> CeremonyIsRedeemableAsync(Guid ceremonyId)
     {
         using var scope = NewSystemTenantScope();
         var session = scope.ServiceProvider.GetRequiredService<IDocumentSession>();
         var doc = await session.LoadAsync<PasskeyCeremony>(ceremonyId, TestContext.Current.CancellationToken);
-        return doc is not null;
+        return doc is not null && !doc.IsConsumed && !doc.IsExpired;
     }
 
     private Task SeedPasskeyClientAsync(string clientId, string? rpId = null) =>

src/dotnet/Modgud.Api.Tests/Security/LockoutConcurrencyTests.cs

@@ -0,0 +1,125 @@
+using System.Net;
+using System.Net.Http.Json;
+using Marten;
+using Modgud.Api.Tests.Infrastructure;
+using Modgud.Authentication.Domain;
+
+namespace Modgud.Api.Tests.Security;
+
+/// <summary>
+/// Pins the account-lockout counter as concurrency-safe (P0-4).
+///
+/// <para>
+/// The counter used to be incremented in memory (<c>user.AccessFailedCount++</c>)
+/// and written back by <c>EventSourcedUserStore.UpdateAsync</c> as part of a
+/// whole-document Store — which ASP.NET Identity triggers after EVERY
+/// <c>AccessFailedAsync</c>. Concurrent failed logins therefore all read the same
+/// value N and all wrote N+1: a burst of parallel attempts registered as roughly
+/// ONE failure, so the five-attempt threshold never tripped and the lockout was
+/// bypassable simply by not sending the guesses sequentially.
+/// </para>
+///
+/// <para>
+/// The counter is now a server-side jsonb increment and the lockout fields are
+/// written exclusively through the <c>IUserLockoutStore</c> methods, never as part
+/// of a full-document Store. These tests drive the real HTTP endpoint so each
+/// attempt gets its own DI scope and therefore its own Marten session — the only
+/// arrangement in which the original race can actually occur.
+/// </para>
+/// </summary>
+[Collection(IntegrationTestCollection.Name)]
+public class LockoutConcurrencyTests : IntegrationTestBase
+{
+    public LockoutConcurrencyTests(SharedPostgresFixture fixture) : base(fixture) { }
+
+    private const string Password = "TestPass1234";
+
+    [Fact]
+    public async Task Parallel_failed_logins_all_count_toward_the_lockout_threshold()
+    {
+        var ct = TestContext.Current.CancellationToken;
+        var user = await Factory.CreateTestUserWithIdentityAsync(
+            firstname: "Race", lastname: "Lockout", acronym: "rl",
+            email: "rl@test.com", password: Password);
+
+        // Well above the 5-attempt threshold, fired simultaneously. Under the old
+        // read-then-write increment these collapsed into a handful of recorded
+        // failures; every one of them must now land.
+        const int parallelAttempts = 12;
+        var anon = Factory.CreateClient();
+
+        await Task.WhenAll(Enumerable.Range(0, parallelAttempts).Select(_ =>
+            anon.PostAsJsonAsync("/api/account/login",
+                new { UserName = "rl", Password = "Wrong123!@#", RememberMe = false }, ct)));
+
+        // The decisive assertion: the CORRECT password is now refused, because the
+        // account is locked. This is the property an attacker defeats by going
+        // parallel — without the fix the burst leaves the counter below the
+        // threshold and this request succeeds.
+        var withCorrectPassword = await anon.PostAsJsonAsync("/api/account/login",
+            new { UserName = "rl", Password = Password, RememberMe = false }, ct);
+
+        Assert.Equal(HttpStatusCode.Unauthorized, withCorrectPassword.StatusCode);
+
+        // And the lockout is actually persisted, not just inferred from the response.
+        await using var qs = GetTenantedSession();
+        var securityData = await qs.LoadAsync<UserSecurityData>(user.Id, ct);
+        Assert.NotNull(securityData);
+        Assert.NotNull(securityData!.LockoutEnd);
+        Assert.True(securityData.LockoutEnd > DateTimeOffset.UtcNow,
+            "the lockout window must still be open right after the burst");
+    }
+
+    [Fact]
+    public async Task Parallel_failed_logins_are_each_recorded_in_the_counter()
+    {
+        var ct = TestContext.Current.CancellationToken;
+        var user = await Factory.CreateTestUserWithIdentityAsync(
+            firstname: "Count", lastname: "Race", acronym: "cr",
+            email: "cr@test.com", password: Password);
+
+        // Deliberately BELOW the threshold so the counter is observable: crossing it
+        // makes Identity reset the count to 0 and stamp LockoutEnd instead.
+        const int parallelAttempts = 4;
+        var anon = Factory.CreateClient();
+
+        await Task.WhenAll(Enumerable.Range(0, parallelAttempts).Select(_ =>
+            anon.PostAsJsonAsync("/api/account/login",
+                new { UserName = "cr", Password = "Wrong123!@#", RememberMe = false }, ct)));
+
+        await using var qs = GetTenantedSession();
+        var securityData = await qs.LoadAsync<UserSecurityData>(user.Id, ct);
+        Assert.NotNull(securityData);
+
+        // Every attempt landed. The old in-memory increment typically left this at 1.
+        Assert.Equal(parallelAttempts, securityData!.AccessFailedCount);
+        Assert.Null(securityData.LockoutEnd);
+    }
+
+    [Fact]
+    public async Task A_successful_login_still_clears_the_counter()
+    {
+        var ct = TestContext.Current.CancellationToken;
+        var user = await Factory.CreateTestUserWithIdentityAsync(
+            firstname: "Clear", lastname: "Counter", acronym: "cc",
+            email: "cc@test.com", password: Password);
+
+        var anon = Factory.CreateClient();
+        for (var i = 0; i < 3; i++)
+        {
+            await anon.PostAsJsonAsync("/api/account/login",
+                new { UserName = "cc", Password = "Wrong123!@#", RememberMe = false }, ct);
+        }
+
+        var ok = await anon.PostAsJsonAsync("/api/account/login",
+            new { UserName = "cc", Password = Password, RememberMe = false }, ct);
+        Assert.Equal(HttpStatusCode.OK, ok.StatusCode);
+
+        // Now that the counter is DB-authoritative rather than a mirror written back
+        // by UpdateAsync, the reset must still travel all the way to the document.
+        await using var qs = GetTenantedSession();
+        var securityData = await qs.LoadAsync<UserSecurityData>(user.Id, ct);
+        Assert.NotNull(securityData);
+        Assert.Equal(0, securityData!.AccessFailedCount);
+    }
+}