From a6dd40b972c2257cd3f0560702c22330cfb20180 Mon Sep 17 00:00:00 2001 From: Alexandre Brandizzi Date: Fri, 25 Sep 2026 13:12:49 -0300 Subject: [PATCH] fix(auth): only count real guesses, drop codes that cannot be emailed, trace reset email sends --- .../AuthenticationServiceTests.cs | 21 +++++++++++++ .../SentryPipelineContractTests.cs | 1 + .../PasswordResetEmailDelivery.cs | 31 ++++++++++++++++--- .../Helpers/InMemoryPasswordResetThrottle.cs | 11 +++++++ .../Implementation/AuthenticationService.cs | 14 +++++++-- .../Interfaces/IPasswordResetThrottle.cs | 8 ++++- .../PasswordResetAbuseLimitsTests.cs | 28 +++++++++++++++++ .../PasswordResetTestHost.cs | 1 + 8 files changed, 107 insertions(+), 8 deletions(-) diff --git a/Api.SeaHavenIndustries.Tests/AuthenticationServiceTests.cs b/Api.SeaHavenIndustries.Tests/AuthenticationServiceTests.cs index 6b539d2..0ac9cde 100644 --- a/Api.SeaHavenIndustries.Tests/AuthenticationServiceTests.cs +++ b/Api.SeaHavenIndustries.Tests/AuthenticationServiceTests.cs @@ -138,6 +138,7 @@ public class AuthenticationServiceTests var registered = new Mock(); var unregistered = new Mock(); var email = new Mock(); + email.Setup(e => e.TryEnqueue(It.IsAny(), It.IsAny(), It.IsAny())).Returns(true); await NewService(userData, registered, email, out _, out _).ForgetPasswordAsync("alice@example.com", CancellationToken.None); await NewService(userData, unregistered, email, out _, out _).ForgetPasswordAsync("nope@example.com", CancellationToken.None); @@ -149,6 +150,26 @@ public class AuthenticationServiceTests email.Verify(e => e.TryEnqueue(It.IsAny(), It.IsAny(), It.IsAny()), Times.Once); } + [Fact] + public async Task ForgetPassword_EmailThatCannotBeQueued_DropsTheCodeAndDoesNotCountTheRequest() + { + var user = IdentityTestHelpers.User(); + var userData = new Mock(); + userData.Setup(u => u.GetByEmailNormalizedAsync(It.IsAny(), It.IsAny())).ReturnsAsync(user); + var forget = new Mock(); + var email = new Mock(); + email.SetupSequence(e => e.TryEnqueue(user.Email!, It.IsAny(), It.IsAny())) + .Returns(false).Returns(false).Returns(false).Returns(true); + + var service = NewService(userData, forget, email, out _, out _); + for (var request = 0; request < 4; request++) + await service.ForgetPasswordAsync("alice@example.com", CancellationToken.None); + + // Three undelivered codes were removed, and they did not use up the hourly limit of three. + forget.Verify(f => f.RemoveByEmailAsync(user.Email!, It.IsAny()), Times.Exactly(3)); + email.Verify(e => e.TryEnqueue(user.Email!, It.IsAny(), It.IsAny()), Times.Exactly(4)); + } + [Fact] public async Task ForgetPassword_DeletedAccount_IsTreatedAsUnregistered() { diff --git a/Api.SeaHavenIndustries.Tests/SentryPipelineContractTests.cs b/Api.SeaHavenIndustries.Tests/SentryPipelineContractTests.cs index b5ff0d3..90f539b 100644 --- a/Api.SeaHavenIndustries.Tests/SentryPipelineContractTests.cs +++ b/Api.SeaHavenIndustries.Tests/SentryPipelineContractTests.cs @@ -61,6 +61,7 @@ public class SentryPipelineContractTests [InlineData("Api.SeaHavenIndustries/HostedServices/WorkOrderWeekRolledHostedService.cs", "workorders.week-rolled-job")] [InlineData("Api.SeaHavenIndustries/HostedServices/PastDueCacheHostedService.cs", "workorders.past-due-cache-job")] [InlineData("Api.SeaHavenIndustries/HostedServices/UpliftLifecycleHostedService.cs", "uplifts.lifecycle-sweep")] + [InlineData("Api.SeaHavenIndustries/HostedServices/PasswordResetEmailDelivery.cs", "auth.password-reset-email")] public void Workers_UseSharedBackgroundTransactionHelper_WithStableNames(string relativePath, string transactionName) { var source = File.ReadAllText(Path.Combine(RepoRoot(), relativePath)); diff --git a/Api.SeaHavenIndustries/HostedServices/PasswordResetEmailDelivery.cs b/Api.SeaHavenIndustries/HostedServices/PasswordResetEmailDelivery.cs index 348073f..5b2b7af 100644 --- a/Api.SeaHavenIndustries/HostedServices/PasswordResetEmailDelivery.cs +++ b/Api.SeaHavenIndustries/HostedServices/PasswordResetEmailDelivery.cs @@ -1,5 +1,7 @@ using System.Threading.Channels; +using Api.SeaHavenIndustries.Observability; using SeaHaven.Services.Interfaces; +using Sentry; namespace Api.SeaHavenIndustries.HostedServices { @@ -62,32 +64,43 @@ namespace Api.SeaHavenIndustries.HostedServices private readonly PasswordResetEmailChannel _channel; private readonly IServiceScopeFactory _scopeFactory; private readonly ILogger _logger; + private readonly IHub _sentryHub; public PasswordResetEmailSenderHostedService( PasswordResetEmailChannel channel, IServiceScopeFactory scopeFactory, - ILogger logger) + ILogger logger, + IHub sentryHub) { _channel = channel; _scopeFactory = scopeFactory; _logger = logger; + _sentryHub = sentryHub; } protected override async Task ExecuteAsync(CancellationToken stoppingToken) { await foreach (var email in _channel.Reader.ReadAllAsync(stoppingToken)) { + using var transaction = SentryObservability.BeginBackgroundTransaction( + _sentryHub, + "auth.password-reset-email", + $"{nameof(PasswordResetEmailSenderHostedService)}.{nameof(SendAsync)}"); try { - await using var scope = _scopeFactory.CreateAsyncScope(); - var sender = scope.ServiceProvider.GetRequiredService(); - if (!await sender.SendEmailAsync(email.EmailTo, email.Subject, email.Body)) - _logger.LogWarning("Password reset email was not accepted by the mail provider."); + await SendAsync(email); + transaction.FinishOk(); + } + catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) + { + transaction.FinishCancelled(); + throw; } catch (Exception ex) { // The message can echo the recipient or the body, so only the type is logged. _logger.LogError("Password reset email failed with {ExceptionType}.", ex.GetType().FullName); + transaction.FinishError(ex); } finally { @@ -95,5 +108,13 @@ namespace Api.SeaHavenIndustries.HostedServices } } } + + private async Task SendAsync(PasswordResetEmail email) + { + await using var scope = _scopeFactory.CreateAsyncScope(); + var sender = scope.ServiceProvider.GetRequiredService(); + if (!await sender.SendEmailAsync(email.EmailTo, email.Subject, email.Body)) + _logger.LogWarning("Password reset email was not accepted by the mail provider."); + } } } diff --git a/SeaHaven.Services/Helpers/InMemoryPasswordResetThrottle.cs b/SeaHaven.Services/Helpers/InMemoryPasswordResetThrottle.cs index 00ad7aa..0502f0c 100644 --- a/SeaHaven.Services/Helpers/InMemoryPasswordResetThrottle.cs +++ b/SeaHaven.Services/Helpers/InMemoryPasswordResetThrottle.cs @@ -46,6 +46,17 @@ namespace SeaHaven.Services.Helpers } } + public void ReleaseCodeRequest(string email) + { + var now = _timeProvider.GetUtcNow(); + lock (_gate) + { + var account = AccountFor(email, now); + if (account.Requests.Count > 0) + account.Requests.RemoveAt(account.Requests.Count - 1); + } + } + public bool TryReserveCheck(string email) { var now = _timeProvider.GetUtcNow(); diff --git a/SeaHaven.Services/Implementation/AuthenticationService.cs b/SeaHaven.Services/Implementation/AuthenticationService.cs index 94e0833..32cc06c 100644 --- a/SeaHaven.Services/Implementation/AuthenticationService.cs +++ b/SeaHaven.Services/Implementation/AuthenticationService.cs @@ -162,7 +162,12 @@ namespace SeaHaven.Services.Implementation if (active) { var body = $"Your Password Reset Code is: {code}. It expires in {(int)ResetCodeLifetime.TotalMinutes} minutes."; - _resetEmails.TryEnqueue(user!.Email!, "Forget Password Request.", body); + if (!_resetEmails.TryEnqueue(user!.Email!, "Forget Password Request.", body)) + { + // The code will never reach the user: drop it and do not count the request. + await _forgetPasswordDataService.RemoveByEmailAsync(user.Email!, cancellationToken); + _resetThrottle.ReleaseCodeRequest(requested); + } } } @@ -219,17 +224,22 @@ namespace SeaHaven.Services.Implementation // The per-account budget spans every code the account is sent, so asking for // new codes does not buy more guesses. A slot is reserved before comparing and - // given back only when the code matches. + // given back when the code matches or there is no live code to guess at. if (!_resetThrottle.TryReserveCheck(email)) return null; var pending = await _forgetPasswordDataService.GetByEmailAsync(email, cancellationToken); if (pending == null) + { + _resetThrottle.ReleaseCheck(email); return null; + } var nowUtc = _timeProvider.GetUtcNow().UtcDateTime; if (!await _forgetPasswordDataService.TryConsumeAttemptAsync(pending.Id, MaxCodeAttempts, nowUtc, cancellationToken)) { + // Expired or out of attempts: nothing was compared, so no guess is counted. + _resetThrottle.ReleaseCheck(email); await _forgetPasswordDataService.RemoveByEmailAsync(pending.Email, cancellationToken); return null; } diff --git a/SeaHaven.Services/Interfaces/IPasswordResetThrottle.cs b/SeaHaven.Services/Interfaces/IPasswordResetThrottle.cs index 8cdcc2e..5484994 100644 --- a/SeaHaven.Services/Interfaces/IPasswordResetThrottle.cs +++ b/SeaHaven.Services/Interfaces/IPasswordResetThrottle.cs @@ -12,13 +12,19 @@ namespace SeaHaven.Services.Interfaces /// bool TryAcceptCodeRequest(string email); + /// Gives back an accepted code request whose email could not be queued. + void ReleaseCodeRequest(string email); + /// /// Reserves one failed check for the email before a code is compared. Returns /// false once the account has used its failed checks for the window. /// bool TryReserveCheck(string email); - /// Gives back the reservation of a check whose code matched. + /// + /// Gives back a reservation that did not become a failed guess: the code matched, + /// or there was no live code to compare it with. + /// void ReleaseCheck(string email); } } diff --git a/SeaHavenIndustries.Tests/PasswordResetAbuseLimitsTests.cs b/SeaHavenIndustries.Tests/PasswordResetAbuseLimitsTests.cs index 2431cc4..9fd8388 100644 --- a/SeaHavenIndustries.Tests/PasswordResetAbuseLimitsTests.cs +++ b/SeaHavenIndustries.Tests/PasswordResetAbuseLimitsTests.cs @@ -92,6 +92,34 @@ public sealed class PasswordResetAbuseLimitsTests Assert.Equal(HttpStatusCode.OK, (await host.ResetAsync(Alice, host.Sent.LatestCodeFor(Alice), NewPassword, NextClient())).StatusCode); } + [Fact] + public async Task Checks_without_a_live_code_do_not_use_up_the_account_budget() + { + await using var host = await PasswordResetTestHost.StartAsync(); + await host.AddUserAsync(Alice, OldPassword); + + // No code issued yet. + for (var check = 0; check < 10; check++) + { + await host.VerifyAsync(Alice, "123456", NextClient()); + await host.ResetAsync(Alice, "123456", NewPassword, NextClient()); + } + + // A code that has expired. + await host.ForgetPasswordAsync(Alice, NextClient()); + var expired = host.Sent.LatestCodeFor(Alice); + host.Time.Advance(TimeSpan.FromMinutes(16)); + for (var check = 0; check < 5; check++) + await host.VerifyAsync(Alice, expired, NextClient()); + + await host.ForgetPasswordAsync(Alice, NextClient()); + var live = host.Sent.LatestCodeFor(Alice); + + Assert.Equal(HttpStatusCode.OK, (await host.VerifyAsync(Alice, live, NextClient())).StatusCode); + Assert.Equal(HttpStatusCode.OK, (await host.ResetAsync(Alice, live, NewPassword, NextClient())).StatusCode); + Assert.Equal(HttpStatusCode.OK, (await host.LoginAsync(Alice, NewPassword)).StatusCode); + } + [Fact] public void Concurrent_checks_can_never_exceed_the_account_budget() { diff --git a/SeaHavenIndustries.Tests/PasswordResetTestHost.cs b/SeaHavenIndustries.Tests/PasswordResetTestHost.cs index a16774e..1199080 100644 --- a/SeaHavenIndustries.Tests/PasswordResetTestHost.cs +++ b/SeaHavenIndustries.Tests/PasswordResetTestHost.cs @@ -93,6 +93,7 @@ internal sealed class PasswordResetTestHost : IAsyncDisposable manager.FeatureProviders.Add(new OnlyAuthenticationController()); }); builder.Services.AddPasswordResetRateLimiting(); + builder.Services.AddSingleton(Sentry.Extensibility.HubAdapter.Instance); builder.Services.AddPasswordResetEmailDelivery(); var app = builder.Build();