From dc801502fd321954434b98295a1a8f7f78f6fe07 Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Mon, 20 Jul 2026 09:14:07 +0200 Subject: [PATCH 1/3] fix(security): apply existing SSRF + one-time-use guards where they were missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First remediation wave from the code-verified July security review. Every fix here reuses a pattern this repo had already solved once and simply hadn't wired up at the second site — no new security machinery is invented. SSRF guard, now shared The CIMD metadata fetcher's transport guard (DNS resolved and validated before connect, closing the rebinding window; redirects off; tight timeouts) moves out of the CIMD namespace to Modgud.Infrastructure/Http as SsrfIpGuard + SsrfSafeHttpHandlerFactory, with a purpose label so a refusal is diagnosable from the log. The IP range table is unchanged; its tests moved with it. Newly protected, both previously a bare HttpClient: - SAML IdP metadata fetch (SamlSetup) - dynamic OIDC discovery + backchannel (DynamicOidcSchemeManager) Both URLs are realm-admin supplied, and a realm admin is a lower-trust tier than the platform operator, so "an admin configured it" is not a reason to skip the guard. One-time use, actually enforced EmailOtpChallenge and PasskeyCeremony consumed via Delete. Marten does NOT version-check deletes, so two concurrent redemptions of the same code or ceremony_id both passed and each completed a login / minted a token — the single-use guarantee existed only in a comment. Both documents now carry a ConsumedAt marker written through a version-checked Store (the pattern MagicLinkChallenge already used), so exactly one racer wins. The native urn:cocoar:magic grant queried by user + token hash and checked only expiry, never IsConsumed. Because the web flow marks links consumed rather than deleting them, a link already used in the browser stayed redeemable through the native channel. Now gated. Follow-on fix found while writing the tests: since a consumed challenge now survives, the issue-path rate limit would have throttled the next request and locked a user out of email OTP right after a successful login. Consumed challenges are exempt, and the re-issue path mutates the loaded row instead of storing a fresh instance (a fresh instance carries no version and would be rejected now that the document is version-checked). SAML ACS body cap The anonymous ACS endpoint does XML parsing + signature validation per request and inherited Kestrel's 30 MB default. Capped at 512 KB, matching the tightening AssetsEndpoints already applies. Tests Four regression tests: concurrent consume of a passkey ceremony and of an email OTP each let exactly one racer win; a consumed OTP is rejected while re-issue still works; and a magic link consumed in the web flow is rejected by the native grant. That last one was negative-controlled — with the gate removed it mints tokens successfully, so it pins the actual vulnerability rather than asserting current behaviour. The passkey ceremony test helper now asserts the security property (still redeemable?) instead of the storage mechanism (row deleted?). Gated locally: build clean, unit 1395/1395, integration 521/521. Deferred deliberately: the SAML ACS rate limit. Its policy enum is per-realm configurable and would pull in DTOs, manifest export, settings patch and frontend — out of scope for a "wire up the existing guard" change, and P3. --- .../CocoarNativeGrantFlowTests.cs | 35 ++++ .../CocoarPasskeyGrantFlowTests.cs | 23 ++- .../OneTimeCredentialAtomicityTests.cs | 168 ++++++++++++++++++ .../Auth/OAuth/AuthorizationEndpoints.cs | 31 +++- .../ExternalAuth/DynamicOidcSchemeManager.cs | 11 +- .../Api/ExternalAuth/Saml/SamlEndpoints.cs | 13 ++ .../Domain/EmailOtpChallenge.cs | 11 ++ .../Domain/PasskeyCeremony.cs | 11 ++ .../Identity/EmailOtpService.cs | 81 +++++++-- .../Setup/MartenStoreOptionsExtensions.cs | 16 +- .../Modgud.Authentication/Setup/SamlSetup.cs | 12 +- .../CimdIpGuard.cs => Http/SsrfIpGuard.cs} | 18 +- .../SsrfSafeHttpHandlerFactory.cs} | 34 ++-- .../OpenIddict/Cimd/CimdClientResolver.cs | 3 +- .../OpenIddict/OpenIddictExtensions.cs | 3 +- .../Http/SsrfIpGuardTests.cs} | 19 +- 16 files changed, 423 insertions(+), 66 deletions(-) create mode 100644 src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs rename src/dotnet/Modgud.Infrastructure/{OpenIddict/Cimd/CimdIpGuard.cs => Http/SsrfIpGuard.cs} (87%) rename src/dotnet/Modgud.Infrastructure/{OpenIddict/Cimd/CimdHttpMessageHandlerFactory.cs => Http/SsrfSafeHttpHandlerFactory.cs} (58%) rename src/dotnet/Modgud.Tests.Unit/{OAuth/Cimd/CimdIpGuardTests.cs => Infrastructure/Http/SsrfIpGuardTests.cs} (77%) diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/CocoarNativeGrantFlowTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/CocoarNativeGrantFlowTests.cs index a89635fb..d66ff513 100644 --- a/src/dotnet/Modgud.Api.Tests/Authorization/CocoarNativeGrantFlowTests.cs +++ 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(); + var hash = MagicLinkChallenge.HashToken(token); + var challenge = await session.Query() + .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] diff --git a/src/dotnet/Modgud.Api.Tests/Authorization/CocoarPasskeyGrantFlowTests.cs b/src/dotnet/Modgud.Api.Tests/Authorization/CocoarPasskeyGrantFlowTests.cs index 37ac8982..4cef6b30 100644 --- a/src/dotnet/Modgud.Api.Tests/Authorization/CocoarPasskeyGrantFlowTests.cs +++ b/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 CeremonyExistsAsync(Guid ceremonyId) + /// + /// Asserts the security property (the ceremony can still be redeemed) rather + /// than the storage mechanism. Consuming is a version-checked Store of + /// ConsumedAt, 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. + /// + private async Task CeremonyIsRedeemableAsync(Guid ceremonyId) { using var scope = NewSystemTenantScope(); var session = scope.ServiceProvider.GetRequiredService(); var doc = await session.LoadAsync(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) => diff --git a/src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs b/src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs new file mode 100644 index 00000000..58845727 --- /dev/null +++ b/src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs @@ -0,0 +1,168 @@ +using JasperFx; +using Marten; +using Microsoft.Extensions.DependencyInjection; +using Modgud.Api.Tests.Infrastructure; +using Modgud.Authentication.Domain; +using Modgud.Authentication.Identity; + +namespace Modgud.Api.Tests.Security; + +/// +/// Pins the one-time-use guarantee for the credentials that previously only +/// *claimed* it in a comment. Two gaps were closed: +/// +/// +/// Cross-channel replay — the web magic-link flow marks a +/// challenge consumed (it cannot Delete: Marten does not version-check +/// deletes, and a version-checked Store is what wins the race). The native +/// urn:cocoar:magic grant queried by user+hash and never looked at +/// IsConsumed, so a link already used in the browser stayed redeemable +/// through the native channel. +/// Concurrent redemption — EmailOtpChallenge and +/// PasskeyCeremony consumed via Delete, which Marten does NOT +/// version-check, so two simultaneous redemptions of one code/ceremony could +/// both succeed. Both now carry a ConsumedAt marker written through a +/// version-checked Store. +/// +/// +[Collection(IntegrationTestCollection.Name)] +public class OneTimeCredentialAtomicityTests : IntegrationTestBase +{ + public OneTimeCredentialAtomicityTests(SharedPostgresFixture fixture) : base(fixture) { } + + // ── concurrent redemption: exactly one racer may win ───────────────────── + + [Fact] + public async Task PasskeyCeremony_ConcurrentConsume_OnlyOneWins() + { + var ct = TestContext.Current.CancellationToken; + var id = Guid.NewGuid(); + + await using (var seed = GetTenantedDocumentSession()) + { + seed.Store(new PasskeyCeremony + { + Id = id, + OptionsJson = "{}", + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(PasskeyCeremony.ExpirationMinutes), + CreatedAt = DateTimeOffset.UtcNow, + }); + await seed.SaveChangesAsync(ct); + } + + await using var sA = GetTenantedDocumentSession(); + await using var sB = GetTenantedDocumentSession(); + var a = await sA.LoadAsync(id, ct); + var b = await sB.LoadAsync(id, ct); + Assert.NotNull(a); + Assert.NotNull(b); + // Both racers observed a live, unconsumed ceremony — the exact window the + // old Delete-based consume left open. + Assert.False(a!.IsConsumed); + Assert.False(b!.IsConsumed); + + a.ConsumedAt = DateTimeOffset.UtcNow; + sA.Store(a); + await sA.SaveChangesAsync(ct); + + b.ConsumedAt = DateTimeOffset.UtcNow; + sB.Store(b); + await Assert.ThrowsAsync(async () => await sB.SaveChangesAsync(ct)); + } + + [Fact] + public async Task EmailOtpChallenge_ConcurrentConsume_OnlyOneWins() + { + var ct = TestContext.Current.CancellationToken; + var userId = Guid.NewGuid(); + + await using (var seed = GetTenantedDocumentSession()) + { + seed.Store(new EmailOtpChallenge + { + Id = userId, + CodeHash = "hash", + Attempts = 0, + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(EmailOtpChallenge.ExpirationMinutes), + CreatedAt = DateTimeOffset.UtcNow, + Email = "otp-race@test.com", + }); + await seed.SaveChangesAsync(ct); + } + + await using var sA = GetTenantedDocumentSession(); + await using var sB = GetTenantedDocumentSession(); + var a = await sA.LoadAsync(userId, ct); + var b = await sB.LoadAsync(userId, ct); + Assert.NotNull(a); + Assert.NotNull(b); + + a!.ConsumedAt = DateTimeOffset.UtcNow; + sA.Store(a); + await sA.SaveChangesAsync(ct); + + b!.ConsumedAt = DateTimeOffset.UtcNow; + sB.Store(b); + await Assert.ThrowsAsync(async () => await sB.SaveChangesAsync(ct)); + } + + // ── the consumed marker must actually gate redemption ──────────────────── + + [Fact] + public async Task EmailOtp_ConsumedCode_IsRejected_AndReissueStillWorks() + { + var ct = TestContext.Current.CancellationToken; + var user = await Factory.CreateTestUserWithIdentityAsync( + firstname: "Otp", lastname: "Reissue", acronym: "or", + email: "otp-reissue@test.com", password: "TestPass1234"); + + using var scope = Factory.Services.CreateScope(); + var otp = scope.ServiceProvider.GetRequiredService(); + + // Seed an already-consumed challenge with a known code. The hash matches + // the service's own scheme (unsalted SHA-256 hex over the raw code), so the + // rejection below can only come from the IsConsumed gate, not a hash miss. + const string code = "123456"; + var codeHash = Convert.ToHexStringLower( + System.Security.Cryptography.SHA256.HashData(System.Text.Encoding.UTF8.GetBytes(code))); + + await using (var seed = GetTenantedDocumentSession()) + { + // RequestOtpAsync requires the user's explicit email-OTP opt-in. + var u = await seed.LoadAsync(user.Id, ct); + u!.EmailOtpEnabled = true; + seed.Store(u); + seed.Store(new EmailOtpChallenge + { + Id = user.Id, + CodeHash = codeHash, + Attempts = 0, + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(EmailOtpChallenge.ExpirationMinutes), + CreatedAt = DateTimeOffset.UtcNow, + Email = "otp-reissue@test.com", + ConsumedAt = DateTimeOffset.UtcNow, + }); + await seed.SaveChangesAsync(ct); + } + + // A consumed code must not authenticate, even though it is unexpired and + // the hash matches. + var replay = await otp.VerifyOtpAsync(user.Id, code, ct); + Assert.True(replay.IsError); + + // Re-issuing must still work — two regressions are guarded here at once: + // (1) the load-then-mutate rewrite (the document is version-checked now, + // so storing a FRESH instance over the consumed row would be rejected), + // and (2) the rate-limit exemption for consumed challenges (the row now + // survives the consume, and without the exemption it would throttle the + // very next request and lock the user out of email OTP). + var reissue = await otp.RequestOtpAsync(user.Id, ct); + Assert.False(reissue.IsError, reissue.IsError ? reissue.FirstError.Description : null); + + await using var read = GetTenantedDocumentSession(); + var fresh = await read.LoadAsync(user.Id, ct); + Assert.NotNull(fresh); + Assert.False(fresh!.IsConsumed); + Assert.Equal(0, fresh.Attempts); + } +} diff --git a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs index bf41480c..9bf8373d 100644 --- a/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs +++ b/src/dotnet/Modgud.Api/Features/Auth/OAuth/AuthorizationEndpoints.cs @@ -729,9 +729,13 @@ private static async Task ExchangeNativeMagicAsync( return ForbidNativeGrant(Errors.InvalidGrant, "Invalid or expired link."); var hash = MagicLinkChallenge.HashToken(token); + // IsConsumed must be checked HERE too, not just in the web flow: the web + // redemption marks the challenge consumed rather than deleting it (so the + // version-checked Store can win the concurrency race), which left a link + // already used in the browser still redeemable through this native grant. var challenge = await session.Query() .FirstOrDefaultAsync(c => c.UserId == userId && c.TokenHash == hash, ct); - if (challenge is null || challenge.IsExpired) + if (challenge is null || challenge.IsExpired || challenge.IsConsumed) { if (challenge is not null) { session.Delete(challenge); await session.SaveChangesAsync(ct); } return ForbidNativeGrant(Errors.InvalidGrant, "Invalid or expired link."); @@ -809,18 +813,33 @@ private static async Task ExchangeNativePasskeyAsync( return await ForbidFactorFailureAsync("Invalid or expired passkey ceremony."); var ceremony = await session.LoadAsync(ceremonyId, ct); - if (ceremony is null || ceremony.IsExpired) + if (ceremony is null || ceremony.IsExpired || ceremony.IsConsumed) { - if (ceremony is not null) { session.Delete(ceremony); await session.SaveChangesAsync(ct); } + if (ceremony is not null && ceremony.IsExpired) + { + session.Delete(ceremony); + await session.SaveChangesAsync(ct); + } return await ForbidFactorFailureAsync("Invalid or expired passkey ceremony."); } // Single-use: consume ANY presented live ceremony as soon as it resolves — // before the assertion-presence check and the verify — so a captured // ceremony_id can never be replayed, even when paired with a - // missing/garbage assertion. - session.Delete(ceremony); - await session.SaveChangesAsync(ct); + // missing/garbage assertion. This is a VERSION-CHECKED Store of the + // ConsumedAt marker, not a Delete: Marten does not version-check deletes, + // so two concurrent redemptions of one ceremony_id would otherwise both + // proceed and each mint a token. The loser's SaveChangesAsync throws. + ceremony.ConsumedAt = DateTimeOffset.UtcNow; + session.Store(ceremony); + try + { + await session.SaveChangesAsync(ct); + } + catch (JasperFx.ConcurrencyException) + { + return await ForbidFactorFailureAsync("Invalid or expired passkey ceremony."); + } // ADR-0009 per-client RP-ID: a ceremony begun for a specific client may only // be redeemed by that same client. Skipped for a legacy/realm-scoped ceremony diff --git a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/DynamicOidcSchemeManager.cs b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/DynamicOidcSchemeManager.cs index f6638696..d9d79c6d 100644 --- a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/DynamicOidcSchemeManager.cs +++ b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/DynamicOidcSchemeManager.cs @@ -149,7 +149,16 @@ public async Task RegisterAsync(LoginProvider config) { if (!options.Scope.Contains(scope)) options.Scope.Add(scope); } - options.Backchannel ??= new HttpClient(); + // Authority/MetadataAddress are admin-supplied, so discovery + the + // token/userinfo backchannel are SSRF sinks. Use the same transport + // guard as the CIMD and SAML metadata fetchers (DNS resolved and + // validated before connect — closing the rebinding window — redirects + // off, tight timeouts) instead of a bare HttpClient. + options.Backchannel ??= new HttpClient( + Modgud.Infrastructure.Http.SsrfSafeHttpHandlerFactory.Create("OIDC metadata/backchannel fetch")) + { + Timeout = TimeSpan.FromSeconds(15), + }; // Cookie + response-mode interaction, two cases: // diff --git a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlEndpoints.cs index eb03e58e..cf3bcf90 100644 --- a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/Saml/SamlEndpoints.cs @@ -1,5 +1,6 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Routing; using Modgud.Infrastructure.Persistence.Tenancy; @@ -93,9 +94,21 @@ private static async Task AcsAsync( [FromServices] SamlLoginFlow flow, CancellationToken ct) { + // This endpoint is anonymous and does real work per request (XML parse + + // signature validation over the whole document), so cap the body well + // below ASP.NET Core's 30 MB default — a SAMLResponse form post is a few + // KB. Same tightening AssetsEndpoints applies for the same reason. + http.Features.Get() + ?.MaxRequestBodySize = MaxAcsBodyBytes; + if (!manager.TryGetBySlug(TenantContext.Current, slug, out var provider) || provider is null) return Results.NotFound(); return await flow.HandleAcsAsync(provider, http, ct); } + + /// 512 KB — a signed SAMLResponse (base64, possibly with an + /// embedded encrypted assertion and a cert chain) is comfortably under this; + /// anything larger is abuse, not a login. + private const long MaxAcsBodyBytes = 512 * 1024; } diff --git a/src/dotnet/Modgud.Authentication/Domain/EmailOtpChallenge.cs b/src/dotnet/Modgud.Authentication/Domain/EmailOtpChallenge.cs index e82679ef..7af5310b 100644 --- a/src/dotnet/Modgud.Authentication/Domain/EmailOtpChallenge.cs +++ b/src/dotnet/Modgud.Authentication/Domain/EmailOtpChallenge.cs @@ -17,6 +17,17 @@ public class EmailOtpChallenge public const int ExpirationMinutes = 10; public const int RateLimitMinutes = 2; + /// + /// Set when the code was successfully redeemed. Consuming is a + /// version-checked Store of this marker rather than a Delete — + /// Marten does NOT enforce optimistic concurrency on deletes, so two + /// concurrent redemptions of the same code would both delete-and-proceed. + /// A later replay is rejected by the gate. The row + /// is reaped by expiry / the next issue for this user. + /// + public DateTimeOffset? ConsumedAt { get; set; } + public bool IsExpired => DateTimeOffset.UtcNow >= ExpiresAt; + public bool IsConsumed => ConsumedAt is not null; public bool HasExceededAttempts => Attempts >= MaxAttempts; } diff --git a/src/dotnet/Modgud.Authentication/Domain/PasskeyCeremony.cs b/src/dotnet/Modgud.Authentication/Domain/PasskeyCeremony.cs index a235b9e1..1739b59a 100644 --- a/src/dotnet/Modgud.Authentication/Domain/PasskeyCeremony.cs +++ b/src/dotnet/Modgud.Authentication/Domain/PasskeyCeremony.cs @@ -46,7 +46,18 @@ public class PasskeyCeremony public DateTimeOffset ExpiresAt { get; set; } public DateTimeOffset CreatedAt { get; set; } + /// + /// Set when the ceremony was consumed. Consuming is a version-checked + /// Store of this marker rather than a Delete — Marten does NOT + /// enforce optimistic concurrency on deletes, so two concurrent + /// redemptions of the same ceremony_id would both delete-and-proceed + /// and each mint a token. A later replay is rejected by the + /// gate; the row is reaped by expiry. + /// + public DateTimeOffset? ConsumedAt { get; set; } + public const int ExpirationMinutes = 5; public bool IsExpired => DateTimeOffset.UtcNow >= ExpiresAt; + public bool IsConsumed => ConsumedAt is not null; } diff --git a/src/dotnet/Modgud.Authentication/Identity/EmailOtpService.cs b/src/dotnet/Modgud.Authentication/Identity/EmailOtpService.cs index f3c9e42c..d46008d1 100644 --- a/src/dotnet/Modgud.Authentication/Identity/EmailOtpService.cs +++ b/src/dotnet/Modgud.Authentication/Identity/EmailOtpService.cs @@ -74,9 +74,14 @@ public async Task> RequestNativeRegistrationOtpAsync(Guid userId, // (2FA opt-in vs. native confirmed-mailbox) run before this is reached. private async Task> IssueChallengeAsync(ApplicationUser user, CancellationToken ct) { - // Rate limiting: check if a recent challenge exists + // Rate limiting: check if a recent challenge exists. A CONSUMED challenge + // must not throttle the next request — consuming now leaves the row in + // place (a version-checked Store is what makes the one-time use atomic; + // see VerifyOtpAsync), whereas it used to be deleted. Without the + // IsConsumed exemption a user who just logged in with a code would be + // locked out of requesting a new one for the whole rate-limit window. var existing = await session.LoadAsync(user.Id, ct); - if (existing is not null && !existing.IsExpired) + if (existing is not null && !existing.IsExpired && !existing.IsConsumed) { var timeSinceCreation = DateTimeOffset.UtcNow - existing.CreatedAt; if (timeSinceCreation.TotalMinutes < config.RateLimitMinutes) @@ -88,18 +93,46 @@ private async Task> IssueChallengeAsync(ApplicationUser user, Canc var code = GenerateOtpCode(); var codeHash = HashCode(code); - // Store challenge (overwrites existing) - var challenge = new EmailOtpChallenge + // Store the challenge, overwriting any existing one for this user. The + // document is version-checked (see MartenStoreOptionsExtensions), so a + // re-issue MUST mutate the row we just loaded rather than storing a + // fresh instance — a fresh instance carries no version and the update + // would be rejected. Resetting ConsumedAt/Attempts is what makes the + // new code usable after a previous one was consumed. + if (existing is not null) { - Id = user.Id, - CodeHash = codeHash, - Attempts = 0, - ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(config.ExpirationMinutes), - CreatedAt = DateTimeOffset.UtcNow, - Email = user.Email!, - }; - session.Store(challenge); - await session.SaveChangesAsync(ct); + existing.CodeHash = codeHash; + existing.Attempts = 0; + existing.ConsumedAt = null; + existing.ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(config.ExpirationMinutes); + existing.CreatedAt = DateTimeOffset.UtcNow; + existing.Email = user.Email!; + session.Store(existing); + } + else + { + session.Store(new EmailOtpChallenge + { + Id = user.Id, + CodeHash = codeHash, + Attempts = 0, + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(config.ExpirationMinutes), + CreatedAt = DateTimeOffset.UtcNow, + Email = user.Email!, + }); + } + + try + { + await session.SaveChangesAsync(ct); + } + catch (JasperFx.ConcurrencyException) + { + // Two issue requests raced. The other one won and its code is already + // in the user's mailbox — don't send a second, conflicting code. + return Error.Validation("EmailOtp.AlreadySent", + "A verification code was recently sent. Please wait before requesting a new one."); + } // Send email await emailService.SendTemplatedEmailAsync( @@ -121,7 +154,7 @@ public async Task> VerifyOtpAsync(Guid userId, string code, Cancel { var challenge = await session.LoadAsync(userId, ct); - if (challenge is null) + if (challenge is null || challenge.IsConsumed) return Error.Validation("EmailOtp.NoPendingChallenge", "No pending verification code found. Please request a new one."); @@ -157,9 +190,23 @@ public async Task> VerifyOtpAsync(Guid userId, string code, Cancel "The verification code is invalid."); } - // Success — delete challenge - session.Delete(challenge); - await session.SaveChangesAsync(ct); + // Success — consume the challenge with a VERSION-CHECKED Store rather than + // a Delete. Marten does not enforce optimistic concurrency on deletes, so + // two concurrent redemptions of the same correct code would both + // delete-and-proceed and both authenticate. Storing ConsumedAt makes the + // losing racer's SaveChangesAsync throw, and any later replay is rejected + // by the IsConsumed gate above. + challenge.ConsumedAt = DateTimeOffset.UtcNow; + session.Store(challenge); + try + { + await session.SaveChangesAsync(ct); + } + catch (JasperFx.ConcurrencyException) + { + return Error.Validation("EmailOtp.InvalidCode", + "The verification code is invalid."); + } return true; } diff --git a/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs b/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs index c0f9d212..b3f1d965 100644 --- a/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs +++ b/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs @@ -63,8 +63,15 @@ public static StoreOptions UseModgudAuthentication(this StoreOptions options) options.Schema.For() .Identity(x => x.Id); + // Same one-time-use reasoning as MagicLinkChallenge below: the consume is + // a version-checked Store of ConsumedAt (Marten does not version-check + // deletes), so exactly one of two concurrent redemptions of the same code + // wins and the loser gets a ConcurrencyException. Re-issuing a code for a + // user mutates the loaded row rather than storing a fresh instance, so the + // version chain stays intact. options.Schema.For() - .Identity(x => x.Id); + .Identity(x => x.Id) + .UseOptimisticConcurrency(true); // Audit #25 — optimistic concurrency makes the "one-time use" consume // atomic. Login loads the challenge then deletes it; two concurrent @@ -121,9 +128,14 @@ public static StoreOptions UseModgudAuthentication(this StoreOptions options) // ADR-0010 Phase 2 — cookieless WebAuthn assertion ceremony for the // native urn:cocoar:passkey grant. Single-use, short-TTL; keyed by the // server-generated ceremonyId. Indexed by ExpiresAt for an optional sweep. + // Optimistic concurrency + a ConsumedAt marker make the single-use + // guarantee real: the redeem path version-checks its Store, so two + // concurrent redemptions of one ceremony_id cannot both mint a token + // (a Delete would not be version-checked). options.Schema.For() .Identity(x => x.Id) - .Index(x => x.ExpiresAt); + .Index(x => x.ExpiresAt) + .UseOptimisticConcurrency(true); // ADR-0009 — cookieless WebAuthn ATTESTATION ceremony for native per-client // passkey enrollment (Bearer-authenticated; a native client has no session diff --git a/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs b/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs index 91d86904..4bb6f01e 100644 --- a/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs +++ b/src/dotnet/Modgud.Authentication/Setup/SamlSetup.cs @@ -36,7 +36,17 @@ public static IServiceCollection AddModgudSaml(this IServiceCollection services) // so the fetcher itself can stay singleton (matches the singleton- // DynamicSamlSchemeManager that uses it) while still benefiting // from the framework's connection pooling + handler rotation. - services.AddHttpClient(SamlMetadataFetcher.HttpClientName); + // The metadata URL is admin-supplied, so the fetch is an SSRF sink: it + // gets the same transport-level guard as the CIMD fetcher (DNS resolved + // + validated before connect, redirects off, tight timeouts). A realm + // admin is a lower-trust tier than the platform operator, so + // "an admin configured it" is not a reason to skip this. + services.AddHttpClient(SamlMetadataFetcher.HttpClientName, client => + { + client.Timeout = TimeSpan.FromSeconds(5); + }) + .ConfigurePrimaryHttpMessageHandler(() => + Modgud.Infrastructure.Http.SsrfSafeHttpHandlerFactory.Create("SAML metadata fetch")); services.AddSingleton(); services.AddSingleton(); diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdIpGuard.cs b/src/dotnet/Modgud.Infrastructure/Http/SsrfIpGuard.cs similarity index 87% rename from src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdIpGuard.cs rename to src/dotnet/Modgud.Infrastructure/Http/SsrfIpGuard.cs index 0d4feb84..71e437b4 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdIpGuard.cs +++ b/src/dotnet/Modgud.Infrastructure/Http/SsrfIpGuard.cs @@ -1,21 +1,23 @@ using System.Net; using System.Net.Sockets; -namespace Modgud.Infrastructure.OpenIddict.Cimd; +namespace Modgud.Infrastructure.Http; /// -/// SSRF address guard for the CIMD metadata fetcher. Classifies a resolved -/// as routable-public (allowed) or -/// non-public/special-use (blocked). Pure + dependency-free so the full -/// range table is unit-testable. +/// SSRF address guard for every server-side fetch of an operator- or +/// admin-supplied URL (CIMD client metadata, SAML IdP metadata, OIDC discovery +/// + backchannel). Classifies a resolved as +/// routable-public (allowed) or non-public/special-use (blocked). Pure + +/// dependency-free so the full range table is unit-testable. /// -/// The fetcher resolves DNS itself and connects to a validated IP via -/// SocketsHttpHandler.ConnectCallback — checking the resolved +/// Callers resolve DNS themselves and connect to a validated IP via +/// SocketsHttpHandler.ConnectCallback (see +/// ) — checking the resolved /// address (not the hostname) and connecting to exactly that address closes /// the DNS-rebinding window where a name resolves "public" at validation /// time and "private" at connect time. /// -public static class CimdIpGuard +public static class SsrfIpGuard { /// /// True if must NOT be connected to (loopback, diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdHttpMessageHandlerFactory.cs b/src/dotnet/Modgud.Infrastructure/Http/SsrfSafeHttpHandlerFactory.cs similarity index 58% rename from src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdHttpMessageHandlerFactory.cs rename to src/dotnet/Modgud.Infrastructure/Http/SsrfSafeHttpHandlerFactory.cs index f72b09c5..7cea5344 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdHttpMessageHandlerFactory.cs +++ b/src/dotnet/Modgud.Infrastructure/Http/SsrfSafeHttpHandlerFactory.cs @@ -1,36 +1,46 @@ using System.Net.Sockets; -namespace Modgud.Infrastructure.OpenIddict.Cimd; +namespace Modgud.Infrastructure.Http; /// -/// Builds the primary for the CIMD metadata -/// fetcher with the SSRF defences baked into the transport itself: +/// Builds the primary for any server-side +/// fetch of an operator- or admin-supplied URL, with the SSRF defences baked +/// into the transport itself: /// /// redirects disabled — a 30x to an internal host can't be followed; /// a that resolves /// DNS, refuses any non-public resolved address -/// (), and connects to exactly that validated IP — +/// (), and connects to exactly that validated IP — /// closing the DNS-rebinding gap a name-then-connect check would leave; /// tight connect/response timeouts. /// /// TLS still validates against the request hostname (SNI + cert), so pinning /// the socket to the pre-validated IP doesn't weaken certificate checking. +/// +/// Used by every admin-supplied-URL fetcher: CIMD client metadata, SAML +/// IdP metadata, and the dynamic OIDC discovery/backchannel. A realm admin is a +/// lower-trust tier than the platform operator, so "an admin configured it" is +/// NOT a reason to skip this guard. /// -internal static class CimdHttpMessageHandlerFactory +public static class SsrfSafeHttpHandlerFactory { - public static SocketsHttpHandler Create() => new() + /// Short human-readable label for the fetch (e.g. + /// "SAML metadata fetch"), used in the refusal message so a blocked + /// connection is diagnosable from the log alone. + public static SocketsHttpHandler Create(string purpose) => new() { AllowAutoRedirect = false, AutomaticDecompression = System.Net.DecompressionMethods.None, ConnectTimeout = TimeSpan.FromSeconds(5), - // Don't reuse pooled connections across hosts longer than needed; a - // CIMD fetch is a one-shot lookup, not a chatty API. + // Don't reuse pooled connections across hosts longer than needed; these + // are one-shot metadata lookups, not chatty APIs. PooledConnectionLifetime = TimeSpan.FromMinutes(2), - ConnectCallback = ConnectAsync, + ConnectCallback = (context, cancellationToken) => + ConnectAsync(purpose, context, cancellationToken), }; private static async ValueTask ConnectAsync( - SocketsHttpConnectionContext context, CancellationToken cancellationToken) + string purpose, SocketsHttpConnectionContext context, CancellationToken cancellationToken) { var host = context.DnsEndPoint.Host; var port = context.DnsEndPoint.Port; @@ -40,7 +50,7 @@ private static async ValueTask ConnectAsync( foreach (var address in addresses) { - if (CimdIpGuard.IsBlocked(address)) continue; + if (SsrfIpGuard.IsBlocked(address)) continue; var socket = new Socket(address.AddressFamily, SocketType.Stream, ProtocolType.Tcp) { @@ -61,6 +71,6 @@ private static async ValueTask ConnectAsync( // Every resolved address was non-public (or the host resolved to // nothing). Refuse — this is the SSRF block path. throw new IOException( - $"CIMD metadata fetch refused: '{host}' did not resolve to a routable public address."); + $"{purpose} refused: '{host}' did not resolve to a routable public address."); } } diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs b/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs index f8f07adb..8fbf5662 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs +++ b/src/dotnet/Modgud.Infrastructure/OpenIddict/Cimd/CimdClientResolver.cs @@ -38,7 +38,8 @@ namespace Modgud.Infrastructure.OpenIddict.Cimd; public sealed class CimdClientResolver { /// Named client whose primary - /// handler carries the SSRF guard (). + /// handler carries the SSRF guard + /// (). public const string HttpClientName = "Modgud.Cimd.MetadataFetcher"; private const int MaxBodyBytes = 5 * 1024; diff --git a/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs b/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs index a3005c59..0ac5f1d8 100644 --- a/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs +++ b/src/dotnet/Modgud.Infrastructure/OpenIddict/OpenIddictExtensions.cs @@ -102,7 +102,8 @@ public static IServiceCollection AddOpenIddictWithMarten( { client.Timeout = TimeSpan.FromSeconds(5); }) - .ConfigurePrimaryHttpMessageHandler(() => Cimd.CimdHttpMessageHandlerFactory.Create()); + .ConfigurePrimaryHttpMessageHandler(() => + Modgud.Infrastructure.Http.SsrfSafeHttpHandlerFactory.Create("CIMD metadata fetch")); // Custom Marten stores (OpenIddict 7 pattern: register the store directly, // not the entity → store mapping) diff --git a/src/dotnet/Modgud.Tests.Unit/OAuth/Cimd/CimdIpGuardTests.cs b/src/dotnet/Modgud.Tests.Unit/Infrastructure/Http/SsrfIpGuardTests.cs similarity index 77% rename from src/dotnet/Modgud.Tests.Unit/OAuth/Cimd/CimdIpGuardTests.cs rename to src/dotnet/Modgud.Tests.Unit/Infrastructure/Http/SsrfIpGuardTests.cs index 30c1b65a..94545a86 100644 --- a/src/dotnet/Modgud.Tests.Unit/OAuth/Cimd/CimdIpGuardTests.cs +++ b/src/dotnet/Modgud.Tests.Unit/Infrastructure/Http/SsrfIpGuardTests.cs @@ -1,15 +1,16 @@ using System.Net; -using Modgud.Infrastructure.OpenIddict.Cimd; +using Modgud.Infrastructure.Http; -namespace Modgud.Tests.Unit.OAuth.Cimd; +namespace Modgud.Tests.Unit.Infrastructure.Http; /// -/// Pins the SSRF address classifier. The CIMD fetcher resolves -/// DNS itself and refuses any non-public resolved address before connecting; -/// a gap here would re-open the server-side-request-forgery surface the whole -/// CIMD design is gated behind. +/// Pins the SSRF address classifier shared by every admin-supplied-URL fetch +/// (CIMD client metadata, SAML IdP metadata, OIDC discovery/backchannel). Each +/// resolves DNS itself and refuses any non-public resolved address before +/// connecting; a gap here would re-open the server-side-request-forgery surface +/// all three are gated behind. /// -public class CimdIpGuardTests +public class SsrfIpGuardTests { [Theory] // IPv4 special-use / non-routable @@ -44,7 +45,7 @@ public class CimdIpGuardTests [InlineData("::ffff:169.254.169.254")] public void Blocks_non_public_addresses(string address) { - Assert.True(CimdIpGuard.IsBlocked(IPAddress.Parse(address)), + Assert.True(SsrfIpGuard.IsBlocked(IPAddress.Parse(address)), $"{address} is non-public and must be blocked."); } @@ -57,7 +58,7 @@ public void Blocks_non_public_addresses(string address) [InlineData("2606:2800:220:1:248:1893:25c8:1946")] // example.com v6 public void Allows_routable_public_addresses(string address) { - Assert.False(CimdIpGuard.IsBlocked(IPAddress.Parse(address)), + Assert.False(SsrfIpGuard.IsBlocked(IPAddress.Parse(address)), $"{address} is a routable public address and must be allowed."); } } From a4149422bbedc542ca00da27142148718110785d Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Mon, 20 Jul 2026 10:03:18 +0200 Subject: [PATCH 2/3] fix(security): move the web passkey ceremony server-side (P0-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The web login ceremony shipped the full AssertionOptions to the browser as plain Base64 in the Modgud.Passkey.Challenge cookie — no signature, no encryption — and the login endpoint parsed it back as trusted input. A client could therefore rewrite the ceremony options or re-present an old challenge together with an old assertion, and the cookie was never actually cleared because Delete was called without the Path the cookie was set with. The native /connect/passkey/begin flow already solved this with a server-side PasskeyCeremony record; the code comment even noted that the two flows differed only in "challenge transport". They no longer differ: - login-options persists a PasskeyCeremony (realm-scoped, so ClientId is null) and the cookie carries nothing but its opaque id. Tampering with the id merely fails to resolve. - login loads the ceremony, rejects it when expired or already consumed, and consumes it BEFORE verifying via a version-checked Store of ConsumedAt — the same single-use mechanism wave 1 gave this document, so a captured ceremony cannot be replayed and two concurrent logins cannot both redeem one challenge. - The RP ID is pinned at begin-time and reused at redeem, so an admin editing PrimaryDomain mid-ceremony cannot cause a begin/redeem drift (matching the native flow's rationale). - Cookie name and path are constants now, shared by Append and Delete, so the delete actually matches. - Expired ceremonies are cleaned opportunistically on the same traffic that creates them, as the native begin endpoint does. Test: the cookie must parse as a bare Guid, the authoritative options must live in the server-side record, and a cookie in the OLD client-held format (Base64 of attacker-chosen options) must not authenticate anything. Gated locally: build clean, unit 1395/1395, integration 522/522. One run showed the known load-dependent UserView catch-up flake in an unrelated class's fixture setup (documented in the Critter-Stack blocker notes); it passed in isolation and the suite was green on a re-run. Not included: the account-lockout concurrency fix (P0-4). An atomic increment alone does nothing there because UpdateAsync writes AccessFailedCount back from the in-memory user after every AccessFailedAsync, and the reset-detection that emits a security event keys off that same write-back. Untangling the counter from that round-trip is a focused change to the Identity store contract and needs its own pass with genuinely concurrent tests. --- .../OneTimeCredentialAtomicityTests.cs | 52 ++++++++++ .../Api/Account/PasskeyEndpoints.cs | 98 +++++++++++++++---- 2 files changed, 131 insertions(+), 19 deletions(-) diff --git a/src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs b/src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs index 58845727..4218d982 100644 --- a/src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Security/OneTimeCredentialAtomicityTests.cs @@ -1,3 +1,5 @@ +using System.Net; +using System.Net.Http.Json; using JasperFx; using Marten; using Microsoft.Extensions.DependencyInjection; @@ -106,6 +108,56 @@ public async Task EmailOtpChallenge_ConcurrentConsume_OnlyOneWins() await Assert.ThrowsAsync(async () => await sB.SaveChangesAsync(ct)); } + // ── web passkey ceremony state must not be client-held ─────────────────── + + [Fact] + public async Task WebPasskeyLoginOptions_KeepsCeremonyServerSide_AndIgnoresForgedCookieState() + { + // The web login ceremony used to ship the FULL AssertionOptions to the + // browser as plain Base64 (no signature, no encryption) and parse it back + // as trusted input — so a client could rewrite the ceremony or replay an + // old challenge. The cookie must now carry nothing but an opaque id, with + // the authoritative state server-side, exactly like the native flow. + var ct = TestContext.Current.CancellationToken; + var client = Factory.CreateClient(); + + var resp = await client.PostAsJsonAsync("/api/account/passkey/login-options", new { }, ct); + Assert.True(resp.IsSuccessStatusCode, + $"login-options failed ({(int)resp.StatusCode}): {await resp.Content.ReadAsStringAsync(ct)}"); + + var setCookie = Assert.Single( + resp.Headers.GetValues("Set-Cookie").Where(c => c.StartsWith(CookieName, StringComparison.Ordinal))); + var cookieValue = setCookie[(CookieName.Length + 1)..].Split(';')[0]; + + // Opaque id, not ceremony state. + Assert.True(Guid.TryParse(cookieValue, out var ceremonyId), + $"cookie must carry only a ceremony id, got: {cookieValue[..Math.Min(40, cookieValue.Length)]}"); + + // The real options live server-side and the ceremony is still redeemable. + await using (var read = GetTenantedDocumentSession()) + { + var ceremony = await read.LoadAsync(ceremonyId, ct); + Assert.NotNull(ceremony); + Assert.False(ceremony!.IsConsumed); + Assert.Contains("challenge", ceremony.OptionsJson, StringComparison.OrdinalIgnoreCase); + } + + // A cookie in the OLD client-held format (Base64 of attacker-chosen + // options) must not authenticate anything — it isn't a ceremony id, so it + // never resolves to server state. + var forged = Convert.ToBase64String( + System.Text.Encoding.UTF8.GetBytes("""{"challenge":"AAAA","rpId":"localhost"}""")); + using var req = new HttpRequestMessage(HttpMethod.Post, "/api/account/passkey/login") + { + Content = JsonContent.Create(new { id = "x", response = new { } }), + }; + req.Headers.Add("Cookie", $"{CookieName}={forged}"); + var forgedResp = await client.SendAsync(req, ct); + Assert.Equal(HttpStatusCode.Unauthorized, forgedResp.StatusCode); + } + + private const string CookieName = "Modgud.Passkey.Challenge"; + // ── the consumed marker must actually gate redemption ──────────────────── [Fact] diff --git a/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs index e370f492..a542f9e8 100644 --- a/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs @@ -25,6 +25,14 @@ public record PasskeyDisplayDto(string Id, string DisplayName, DateTimeOffset Cr private const string RegistrationCacheKey = "fido2.attestationOptions"; + /// Carries only the server-side ceremony id — never the ceremony + /// state itself. + private const string PasskeyChallengeCookie = "Modgud.Passkey.Challenge"; + + /// The cookie is scoped to the passkey endpoints; every Append AND + /// Delete must use this exact path or the delete silently does nothing. + private const string PasskeyCookiePath = "/api/account/passkey"; + public static WebApplication MapPasskeyEndpoints(this WebApplication application, string path) { var group = application.MapGroup($"{path}/account/passkey") @@ -296,23 +304,51 @@ public static WebApplication MapPasskeyEndpoints(this WebApplication application UserVerification = UserVerificationRequirement.Required, }); - // Store challenge in a secure cookie (anonymous users don't have sessions). + // The ceremony state lives SERVER-SIDE, exactly like the native + // /connect/passkey/begin flow — the cookie only carries an opaque + // ceremony id. It used to carry the whole AssertionOptions as plain + // Base64 (no signature, no encryption) and the login endpoint parsed it + // back as trusted input, so a client could rewrite the ceremony options + // and replay an old challenge. Anonymous users have no session, but they + // don't need one: the id is a lookup key, and tampering with it merely + // fails to resolve. + // + // Opportunistic cleanup on the same traffic that creates the docs, as + // the native begin endpoint does — bounds orphaned-ceremony growth + // without a scheduled job (backed by the ExpiresAt index). + session.DeleteWhere(c => c.ExpiresAt < DateTimeOffset.UtcNow); + + var optionsJson = options.ToJson(); + var ceremony = new PasskeyCeremony + { + Id = Guid.NewGuid(), + OptionsJson = optionsJson, + // Realm-scoped web login: no per-client binding, and the RP ID is + // pinned so an admin editing the setting mid-ceremony can't cause a + // begin/redeem drift (same rationale as the native flow). + ClientId = null, + RpId = await rpIdResolver.GetPrimaryDomainAsync(ct), + ExpiresAt = DateTimeOffset.UtcNow.AddMinutes(PasskeyCeremony.ExpirationMinutes), + CreatedAt = DateTimeOffset.UtcNow, + }; + session.Store(ceremony); + await session.SaveChangesAsync(ct); + // Secure=Request.IsHttps mirrors CookieSecurePolicy.SameAsRequest: // the cookie carries Secure on HTTPS requests and not on plain HTTP // (dev only). With ForwardedHeaders middleware behind the reverse // proxy, IsHttps reflects the public scheme, so production deploys // always get Secure even when Kestrel itself listens on HTTP behind // the proxy. - var optionsJson = options.ToJson(); - context.Response.Cookies.Append("Modgud.Passkey.Challenge", - Convert.ToBase64String(Encoding.UTF8.GetBytes(optionsJson)), + context.Response.Cookies.Append(PasskeyChallengeCookie, + ceremony.Id.ToString(), new CookieOptions { HttpOnly = true, Secure = context.Request.IsHttps, SameSite = SameSiteMode.Strict, - MaxAge = TimeSpan.FromMinutes(5), - Path = "/api/account/passkey", + MaxAge = TimeSpan.FromMinutes(PasskeyCeremony.ExpirationMinutes), + Path = PasskeyCookiePath, }); return Results.Content(optionsJson, "application/json"); @@ -334,33 +370,57 @@ public static WebApplication MapPasskeyEndpoints(this WebApplication application { var fido2 = await fido2Factory.CreateAsync(ct); - // Retrieve challenge from cookie - var challengeCookie = context.Request.Cookies["Modgud.Passkey.Challenge"]; - if (string.IsNullOrEmpty(challengeCookie)) + // The cookie carries only the ceremony id; the authoritative options + // come from the server-side record. Delete MUST repeat the Path the + // cookie was set with — a pathless Delete does not match it and the + // stale cookie survives in the browser. + var challengeCookie = context.Request.Cookies[PasskeyChallengeCookie]; + context.Response.Cookies.Delete(PasskeyChallengeCookie, + new CookieOptions { Path = PasskeyCookiePath }); + + if (!Guid.TryParse(challengeCookie, out var ceremonyId)) return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); + var ceremony = await session.LoadAsync(ceremonyId, ct); + if (ceremony is null || ceremony.IsExpired || ceremony.IsConsumed) + return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); + + // Single-use: consume before verifying, via a version-checked Store of + // ConsumedAt (Marten does not version-check deletes), so a captured + // ceremony can never be replayed and two concurrent logins cannot both + // redeem one challenge. + ceremony.ConsumedAt = DateTimeOffset.UtcNow; + session.Store(ceremony); + try + { + await session.SaveChangesAsync(ct); + } + catch (JasperFx.ConcurrencyException) + { + return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); + } + AssertionOptions options; try { - var optionsJson = Encoding.UTF8.GetString(Convert.FromBase64String(challengeCookie)); - options = AssertionOptions.FromJson(optionsJson); + options = AssertionOptions.FromJson(ceremony.OptionsJson); } catch { return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); } - context.Response.Cookies.Delete("Modgud.Passkey.Challenge"); - // Verify the assertion via the shared verifier (the SAME FIDO2 verify - // the native urn:cocoar:passkey grant uses — no fork). Only the - // challenge transport differs: web reads AssertionOptions from the - // cookie above, native from the server-side ceremony doc. The web flow - // is realm-scoped: RP ID = PrimaryDomain (the fido2 above was built with - // it), so legacy null-RpId credentials resolve to it and still verify. + // the native urn:cocoar:passkey grant uses — no fork). Both flows now + // also share the challenge transport: a server-side ceremony doc. The + // web flow is realm-scoped: RP ID = PrimaryDomain (the fido2 above was + // built with it), so legacy null-RpId credentials resolve to it and + // still verify. Prefer the RP ID pinned at begin-time so an admin + // changing PrimaryDomain mid-ceremony can't cause a begin/redeem drift. var primaryDomain = await rpIdResolver.GetPrimaryDomainAsync(ct); + var ceremonyRpId = ceremony.RpId ?? primaryDomain; var storedCredential = await PasskeyAssertionVerifier.VerifyAsync( - fido2, options, body.GetRawText(), session, primaryDomain, primaryDomain, ct); + fido2, options, body.GetRawText(), session, ceremonyRpId, ceremonyRpId, ct); if (storedCredential is null) return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); From f855f8f4ab4f38918703d340816b4c22228b21d5 Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Mon, 20 Jul 2026 11:52:48 +0200 Subject: [PATCH 3/3] fix(security): make the account-lockout counter concurrency-safe (P0-4) The five-attempt lockout was bypassable by sending the guesses in parallel instead of sequentially. IncrementAccessFailedCountAsync only did an in-memory `user.AccessFailedCount++`, and EventSourcedUserStore.UpdateAsync -- which ASP.NET Identity calls after EVERY AccessFailedAsync -- wrote the whole UserSecurityData document back from a load-time snapshot. Concurrent failed logins therefore all read N and all wrote N+1, so a burst registered as roughly one failure and the threshold never tripped. Fixing the increment alone would not have helped: the whole-document Store was the second half of the race. So the lockout fields are now owned end to end by the IUserLockoutStore methods: - IncrementAccessFailedCountAsync does a server-side jsonb increment (the existing "Audit #24" pattern from the email-OTP attempt counter) and reads the authoritative value back, because UserManager compares that return value against MaxFailedAccessAttempts. - ResetAccessFailedCountAsync and SetLockoutEndDateAsync write through atomic patches. - GetAccessFailedCountAsync and GetLockoutEndDateAsync re-fetch authoritatively, the same idiom as GetSecurityStampAsync. This also closes a smaller gap: IsLockedOutAsync read a mirror that could not see a lockout set by a concurrent request. - UpdateAsync patches only the fields it actually owns and no longer touches AccessFailedCount or LockoutEnd. That incidentally protects the grace-period fields from the same clobbering. The lock/unlock and resolved-failure-streak audit events were stale-vs-fresh diffs inside UpdateAsync that only worked because of the whole-document Store. They move to the paths that actually change those values, unchanged in shape. Tests: three integration tests driving the real login endpoint, so each attempt gets its own DI scope and Marten session. Verified as real coverage by reverting the store and confirming both concurrency tests fail without the fix. Unit 1395/1395, integration 525/525. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Audit/LoginFailureStreakEmissionTests.cs | 4 +- .../Security/LockoutConcurrencyTests.cs | 125 ++++++++++++ .../Identity/EventSourcedUserStore.cs | 192 ++++++++++++------ 3 files changed, 261 insertions(+), 60 deletions(-) create mode 100644 src/dotnet/Modgud.Api.Tests/Security/LockoutConcurrencyTests.cs diff --git a/src/dotnet/Modgud.Api.Tests/Audit/LoginFailureStreakEmissionTests.cs b/src/dotnet/Modgud.Api.Tests/Audit/LoginFailureStreakEmissionTests.cs index cb6d41c5..69c3dc4b 100644 --- a/src/dotnet/Modgud.Api.Tests/Audit/LoginFailureStreakEmissionTests.cs +++ b/src/dotnet/Modgud.Api.Tests/Audit/LoginFailureStreakEmissionTests.cs @@ -8,7 +8,9 @@ namespace Modgud.Api.Tests.Audit; /// -/// Proves the Phase 1 emission logic in EventSourcedUserStore.AppendSecurityChangeEvents: +/// Proves the Phase 1 emission logic in EventSourcedUserStore.ResetAccessFailedCountAsync +/// (it moved there from the former AppendSecurityChangeEvents diff when the +/// lockout counter was made concurrency-safe — see P0-4 / LockoutConcurrencyTests): /// a known-user failure streak is recorded as exactly ONE aggregated /// when the access-failed counter /// resolves (>0 → 0), not one event per attempt (Decision (b)). diff --git a/src/dotnet/Modgud.Api.Tests/Security/LockoutConcurrencyTests.cs b/src/dotnet/Modgud.Api.Tests/Security/LockoutConcurrencyTests.cs new file mode 100644 index 00000000..60830a8b --- /dev/null +++ b/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; + +/// +/// Pins the account-lockout counter as concurrency-safe (P0-4). +/// +/// +/// The counter used to be incremented in memory (user.AccessFailedCount++) +/// and written back by EventSourcedUserStore.UpdateAsync as part of a +/// whole-document Store — which ASP.NET Identity triggers after EVERY +/// AccessFailedAsync. 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. +/// +/// +/// +/// The counter is now a server-side jsonb increment and the lockout fields are +/// written exclusively through the IUserLockoutStore 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. +/// +/// +[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(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(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(user.Id, ct); + Assert.NotNull(securityData); + Assert.Equal(0, securityData!.AccessFailedCount); + } +} diff --git a/src/dotnet/Modgud.Authentication/Identity/EventSourcedUserStore.cs b/src/dotnet/Modgud.Authentication/Identity/EventSourcedUserStore.cs index 5065c51f..1448f749 100644 --- a/src/dotnet/Modgud.Authentication/Identity/EventSourcedUserStore.cs +++ b/src/dotnet/Modgud.Authentication/Identity/EventSourcedUserStore.cs @@ -1,4 +1,5 @@ using Marten; +using Marten.Patching; using Microsoft.AspNetCore.Identity; using Modgud.Authentication.Domain; using Modgud.Authentication.Events; @@ -112,20 +113,29 @@ public async Task UpdateAsync(ApplicationUser user, Cancellation var securityData = await session.LoadAsync(user.Id, cancellationToken); if (securityData is not null) { - // Detect and append security-related events by comparing - // the user's transient properties against the persisted UserSecurityData - AppendSecurityChangeEvents(user, securityData); - - // Sync security data - securityData.PasswordHash = user.PasswordHash; - securityData.SecurityStamp = user.SecurityStamp ?? securityData.SecurityStamp; - securityData.AccessFailedCount = user.AccessFailedCount; - securityData.LockoutEnd = user.LockoutEnd; - securityData.TwoFactorEnabled = user.TwoFactorEnabled; - securityData.AuthenticatorKey = user.AuthenticatorKey; - securityData.UpdateConcurrencyStamp(); - user.ConcurrencyStamp = securityData.ConcurrencyStamp; - session.Store(securityData); + // P0-4 — patch ONLY the fields this round-trip actually owns instead of + // storing the whole document. ASP.NET Identity calls UpdateAsync after + // EVERY AccessFailedAsync, so a full Store here rewrote the lockout + // fields from a load-time snapshot and silently undid whatever a + // concurrent request had just incremented: the five-attempt lockout was + // bypassable by firing attempts in parallel. AccessFailedCount and + // LockoutEnd are now written exclusively by the IUserLockoutStore + // methods below, via atomic jsonb patches, and are deliberately absent + // from this write set. The same reasoning protects the grace-period + // fields (SecureSetupDueAt, GracePeriodDaysOverride, TwoFactorExempt), + // which are owned by their own admin/2FA paths. + var newConcurrencyStamp = Guid.NewGuid().ToString(); + session.Patch(user.Id) + .Set(x => x.PasswordHash, user.PasswordHash); + session.Patch(user.Id) + .Set(x => x.SecurityStamp, user.SecurityStamp ?? securityData.SecurityStamp); + session.Patch(user.Id) + .Set(x => x.TwoFactorEnabled, user.TwoFactorEnabled); + session.Patch(user.Id) + .Set(x => x.AuthenticatorKey, user.AuthenticatorKey); + session.Patch(user.Id) + .Set(x => x.ConcurrencyStamp, newConcurrencyStamp); + user.ConcurrencyStamp = newConcurrencyStamp; } else { @@ -296,32 +306,124 @@ public Task SetNormalizedEmailAsync(ApplicationUser user, string? normalizedEmai // IUserLockoutStore - public Task GetLockoutEndDateAsync(ApplicationUser user, CancellationToken cancellationToken) + // P0-4 — the lockout counter and the lockout window are DB-authoritative. + // Both are read straight from UserSecurityData (the same "authoritative + // re-fetch" idiom as GetSecurityStampAsync / GetTwoFactorEnabledAsync) and + // written only through atomic jsonb patches, never as part of a whole-document + // Store. The transient mirrors on ApplicationUser are kept in sync so callers + // that inspect the user object still see the truth, but nothing depends on + // them for the lockout decision. + + public async Task GetLockoutEndDateAsync(ApplicationUser user, CancellationToken cancellationToken) { - return Task.FromResult(user.LockoutEnd); + cancellationToken.ThrowIfCancellationRequested(); + + // This is the predicate behind UserManager.IsLockedOutAsync. Reading the + // in-memory mirror meant a lockout set by a CONCURRENT request after this + // user was loaded went unseen for the rest of the request. + var securityData = await session.LoadAsync(user.Id, cancellationToken); + if (securityData is null) + return user.LockoutEnd; + + user.LockoutEnd = securityData.LockoutEnd; + return securityData.LockoutEnd; } - public Task SetLockoutEndDateAsync(ApplicationUser user, DateTimeOffset? lockoutEnd, CancellationToken cancellationToken) + public async Task SetLockoutEndDateAsync(ApplicationUser user, DateTimeOffset? lockoutEnd, CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); + + var securityData = await session.LoadAsync(user.Id, cancellationToken); user.LockoutEnd = lockoutEnd; - return Task.CompletedTask; + + if (securityData is null) + return; // No document yet — UpdateAsync's create branch persists the mirror. + + // Lock/unlock audit events used to be derived inside UpdateAsync by diffing + // the in-memory user against the loaded document. That diff is gone with the + // whole-document Store, so the transition is detected here, at the only place + // that actually changes the value. + if (securityData.LockoutEnd != lockoutEnd) + { + if (lockoutEnd.HasValue && lockoutEnd > DateTimeOffset.UtcNow) + session.Events.Append(user.Id, new UserLockedOutEvent(user.Id, lockoutEnd.Value)); + else if (securityData.LockoutEnd.HasValue && !lockoutEnd.HasValue) + session.Events.Append(user.Id, new UserUnlockedEvent(user.Id)); + } + + session.Patch(user.Id).Set(x => x.LockoutEnd, lockoutEnd); } - public Task GetAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) + public async Task GetAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) { - return Task.FromResult(user.AccessFailedCount); + cancellationToken.ThrowIfCancellationRequested(); + + var securityData = await session.LoadAsync(user.Id, cancellationToken); + if (securityData is null) + return user.AccessFailedCount; + + user.AccessFailedCount = securityData.AccessFailedCount; + return securityData.AccessFailedCount; } - public Task IncrementAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) + public async Task IncrementAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) { - user.AccessFailedCount++; - return Task.FromResult(user.AccessFailedCount); + cancellationToken.ThrowIfCancellationRequested(); + + // Server-side jsonb increment — the "Audit #24" pattern already used for the + // email-OTP attempt counter. A read-then-write increment lets N concurrent + // failed logins all read the same value and write value+1, so only ONE + // attempt in the burst is ever recorded and MaxFailedAccessAttempts never + // trips. The patch lands every attempt regardless of concurrency. + // + // The flush is required, not incidental: UserManager compares this return + // value against MaxFailedAccessAttempts to decide whether to lock, so we + // must read back a value that includes our own increment. + session.Patch(user.Id).Increment(x => x.AccessFailedCount, 1); + await session.SaveChangesAsync(cancellationToken); + + var securityData = await session.LoadAsync(user.Id, cancellationToken); + if (securityData is null) + { + // Migration-created user with no security document yet — fall back to the + // pre-existing in-memory behaviour; UpdateAsync's create branch persists it. + user.AccessFailedCount++; + return user.AccessFailedCount; + } + + // A racer may have incremented further between our commit and this read, so + // this can be HIGHER than our own increment. That direction is fail-closed + // (the burst locks out at least as early as it should), which is exactly the + // bias we want on a brute-force counter. + user.AccessFailedCount = securityData.AccessFailedCount; + return securityData.AccessFailedCount; } - public Task ResetAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) + public async Task ResetAccessFailedCountAsync(ApplicationUser user, CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); + + var securityData = await session.LoadAsync(user.Id, cancellationToken); user.AccessFailedCount = 0; - return Task.CompletedTask; + + if (securityData is null) + return; // No document yet — UpdateAsync's create branch persists the mirror. + + // A failure streak just resolved — the counter went from >0 back to 0 (a + // successful sign-in or an unlock). Record it as ONE aggregated audit event + // (Decision (b)), not one per attempt: no stream spam, and an attacker + // spraying a victim can't inflate that victim's stream. No IP (the aggregate + // has no single source); erasable with the user's stream. This detector used + // to live in UpdateAsync as a stale-vs-fresh diff; it now sits on the only + // path that actually resets the counter, and reads the authoritative value + // rather than the possibly-stale mirror. + if (securityData.AccessFailedCount > 0) + { + session.Events.Append(user.Id, new UserLoginFailuresObservedEvent( + user.Id, securityData.AccessFailedCount, DateTimeOffset.UtcNow)); + } + + session.Patch(user.Id).Set(x => x.AccessFailedCount, 0); } public Task GetLockoutEnabledAsync(ApplicationUser user, CancellationToken cancellationToken) @@ -386,40 +488,12 @@ public Task GetPhoneNumberConfirmedAsync(ApplicationUser user, Cancellatio public Task SetPhoneNumberConfirmedAsync(ApplicationUser user, bool confirmed, CancellationToken cancellationToken) => Task.CompletedTask; - private void AppendSecurityChangeEvents(ApplicationUser user, UserSecurityData securityData) - { - var events = new List(); - - // Lockout changed — detected by comparing user's transient properties - // against the persisted UserSecurityData document - if (user.LockoutEnd != securityData.LockoutEnd) - { - if (user.LockoutEnd.HasValue && user.LockoutEnd > DateTimeOffset.UtcNow) - { - events.Add(new UserLockedOutEvent(user.Id, user.LockoutEnd.Value)); - } - else if (securityData.LockoutEnd.HasValue && !user.LockoutEnd.HasValue) - { - events.Add(new UserUnlockedEvent(user.Id)); - } - } - - // A failure streak just resolved — the access-failed counter went from >0 - // back to 0 (a successful sign-in or an unlock reset it). Record it as ONE - // aggregated audit event (Decision (b)), not one per attempt: no stream spam, - // and an attacker spraying a victim can't inflate that victim's stream. No IP - // (the aggregate has no single source); erasable with the user's stream. - if (securityData.AccessFailedCount > 0 && user.AccessFailedCount == 0) - { - events.Add(new UserLoginFailuresObservedEvent( - user.Id, securityData.AccessFailedCount, DateTimeOffset.UtcNow)); - } - - if (events.Count > 0) - { - session.Events.Append(user.Id, events.ToArray()); - } - } + // The former AppendSecurityChangeEvents lived here. Both of its detectors + // (lockout transition, resolved failure streak) were stale-vs-fresh diffs that + // only worked because UpdateAsync wrote the whole security document from the + // in-memory user — the very pattern that made the lockout counter racy (P0-4). + // They now sit on SetLockoutEndDateAsync / ResetAccessFailedCountAsync, the + // only paths that actually change those values. private async Task PopulateSecurityDataAsync(ApplicationUser user, CancellationToken cancellationToken) {