From 0359dea0fc215af080f2071449224c47a5b20603 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Sat, 11 Jul 2026 17:29:42 -0300 Subject: [PATCH 01/16] fix(web): correct idempotent replay payload and serialize concurrent duplicates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in IdempotencyEndpointFilter: API-01 — the filter cached JsonSerializer.SerializeToUtf8Bytes(result) where result is the wrapped IResult (Ok/Created), so it stored {"value":...,"statusCode":200} instead of the wire DTO, and it read Response.StatusCode before the IResult executed, so a 201 Created replayed as 200. The handler result is now executed into a buffer to capture the real wire body + status, which is what gets served and cached. CONC-01 — probe->execute->write had no atomic reservation, so two concurrent requests with the same key both missed the probe and both executed the handler. An atomic in-flight reservation now serializes duplicates: Redis SET NX when an IConnectionMultiplexer is registered (the multi-instance case — this stack already requires Redis there for the shared Data Protection key ring), an in-process set otherwise (single instance). A duplicate that arrives while the original is still running gets 409 Conflict. Redis stays optional: without it the app falls back to the in-memory reservation, correct for a single instance where a cross-container race cannot occur. --- .../Idempotency/IdempotencyEndpointFilter.cs | 193 ++++++++++++---- .../IdempotencyEndpointFilterReplayTests.cs | 215 ++++++++++++++++++ 2 files changed, 369 insertions(+), 39 deletions(-) create mode 100644 src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index fa73b35c7c..002cf4cbdb 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Security.Cryptography; using System.Text; using System.Text.Json; @@ -9,6 +10,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using StackExchange.Redis; namespace FSH.Framework.Web.Idempotency; @@ -21,13 +23,21 @@ namespace FSH.Framework.Web.Idempotency; /// Uses directly for the probe read (bypassing /// 's factory-mandatory API) and /// for the write path so replays benefit from L1 and the regular tag invalidation story. -/// Using HybridCache with DisableUnderlyingData as a "get-only probe" is a -/// known anti-pattern tracked at dotnet/aspnetcore#57191. +/// The handler result is executed into a buffer so the cached payload is the real wire body and +/// status code (an Ok<T>/Created<T> wrapper would otherwise be serialized +/// verbatim, and Response.StatusCode is still the default at filter time — the IResult sets +/// it only when it executes). Concurrent duplicate keys are serialized by an atomic in-flight +/// reservation (Redis SET NX when a multiplexer is registered, an in-process set otherwise). /// public sealed class IdempotencyEndpointFilter : IEndpointFilter { private static readonly JsonSerializerOptions JsonOpts = new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + // In-process reservation used when no Redis multiplexer is registered. Single-instance only — + // a multi-instance host in this stack already runs Redis (shared Data Protection key ring), so + // the Redis branch below covers every deployment where cross-instance duplicates are possible. + private static readonly ConcurrentDictionary InFlight = new(StringComparer.Ordinal); + public async ValueTask InvokeAsync(EndpointFilterInvocationContext context, EndpointFilterDelegate next) { ArgumentNullException.ThrowIfNull(context); @@ -59,60 +69,165 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // Probe-only read via IDistributedCache (real GetAsync, null on miss — unlike HybridCache's // factory). Bypasses L1: replays are rare vs first-calls, so L1 warmth has little value. - var cachedBytes = await distributedCache.GetAsync(cacheKey, httpContext.RequestAborted).ConfigureAwait(false); - if (cachedBytes is not null && cachedBytes.Length > 0) + var cached = await ProbeAsync(distributedCache, cacheKey, httpContext.RequestAborted).ConfigureAwait(false); + if (cached is not null) + { + return await ReplayAsync(httpContext, cached, idempotencyKey, logger).ConfigureAwait(false); + } + + // Atomically reserve the key so concurrent duplicates don't both execute the handler. + var multiplexer = httpContext.RequestServices.GetService(); + var reservationKey = cacheKey + ":inflight"; + if (!await TryReserveAsync(multiplexer, reservationKey, options.DefaultTtl, httpContext.RequestAborted).ConfigureAwait(false)) { - var cached = JsonSerializer.Deserialize(cachedBytes, JsonOpts); - if (cached is not null) + // Another request with this key is in flight. It may have finished between the probe + // and the reservation — re-probe once, otherwise report the in-progress conflict. + var raced = await ProbeAsync(distributedCache, cacheKey, httpContext.RequestAborted).ConfigureAwait(false); + return raced is not null + ? await ReplayAsync(httpContext, raced, idempotencyKey, logger).ConfigureAwait(false) + : TypedResults.Conflict("A request with this Idempotency-Key is already being processed."); + } + + try + { + var result = await next(context).ConfigureAwait(false); + + // Execute the result into a buffer to capture the real wire body + status code, then + // serve that buffer to the client. Returning the IResult unexecuted would leave + // Response.StatusCode at its default and cache the wrapper object, not the wire body. + var (statusCode, contentType, body) = await ExecuteAndCaptureAsync(result, httpContext).ConfigureAwait(false); + + httpContext.Response.StatusCode = statusCode; + if (contentType is not null) { - if (logger.IsEnabled(LogLevel.Debug)) - { - logger.LogDebug("Idempotent replay for key {KeyHash}", HashKey(idempotencyKey)); - } - httpContext.Response.Headers["Idempotency-Replayed"] = "true"; - httpContext.Response.StatusCode = cached.StatusCode; - if (cached.ContentType is not null) - { - httpContext.Response.ContentType = cached.ContentType; - } + httpContext.Response.ContentType = contentType; + } - if (cached.Body.Length > 0) + if (body.Length > 0) + { + await httpContext.Response.Body.WriteAsync(body, httpContext.RequestAborted).ConfigureAwait(false); + } + + // Cache the response through HybridCache so the tag invalidation path works for purges. + try + { + var responseToCache = new CachedIdempotentResponse { - await httpContext.Response.Body.WriteAsync(cached.Body, httpContext.RequestAborted).ConfigureAwait(false); - } + StatusCode = statusCode, + ContentType = contentType ?? "application/json", + Body = body, + }; - return null; // Response already written + var setOptions = new HybridCacheEntryOptions + { + Expiration = options.DefaultTtl, + LocalCacheExpiration = options.DefaultTtl < TimeSpan.FromMinutes(2) ? options.DefaultTtl : TimeSpan.FromMinutes(2), + }; + await hybridCache.SetAsync(cacheKey, responseToCache, setOptions, tags, httpContext.RequestAborted).ConfigureAwait(false); + } + // Best-effort caching: idempotency replay is a convenience, not a correctness requirement + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogWarning(ex, "Failed to cache idempotent response for key {KeyHash}", HashKey(idempotencyKey)); } + + // Response already written to the body directly; return an empty result so the framework + // doesn't serialize a null return and append "null" after the captured payload. + return Results.Empty; + } + finally + { + await ReleaseReservationAsync(multiplexer, reservationKey).ConfigureAwait(false); + } + } + + private static async ValueTask ProbeAsync( + IDistributedCache cache, string cacheKey, CancellationToken ct) + { + var bytes = await cache.GetAsync(cacheKey, ct).ConfigureAwait(false); + return bytes is { Length: > 0 } + ? JsonSerializer.Deserialize(bytes, JsonOpts) + : null; + } + + private static async ValueTask ReplayAsync( + HttpContext httpContext, CachedIdempotentResponse cached, string idempotencyKey, ILogger logger) + { + if (logger.IsEnabled(LogLevel.Debug)) + { + logger.LogDebug("Idempotent replay for key {KeyHash}", HashKey(idempotencyKey)); } - // Execute the handler - var result = await next(context).ConfigureAwait(false); + httpContext.Response.Headers["Idempotency-Replayed"] = "true"; + httpContext.Response.StatusCode = cached.StatusCode; + if (cached.ContentType is not null) + { + httpContext.Response.ContentType = cached.ContentType; + } - // Cache the response through HybridCache so the tag invalidation path works for purges. + if (cached.Body.Length > 0) + { + await httpContext.Response.Body.WriteAsync(cached.Body, httpContext.RequestAborted).ConfigureAwait(false); + } + + // Empty result (not null) so the framework doesn't append a serialized "null". + return Results.Empty; + } + + private static async Task<(int StatusCode, string? ContentType, byte[] Body)> ExecuteAndCaptureAsync( + object? result, HttpContext httpContext) + { + var originalBody = httpContext.Response.Body; + await using var buffer = new MemoryStream(); + httpContext.Response.Body = buffer; try { - var body = result is not null ? JsonSerializer.SerializeToUtf8Bytes(result, JsonOpts) : []; - var responseToCache = new CachedIdempotentResponse + switch (result) { - StatusCode = httpContext.Response.StatusCode is > 0 and < 600 ? httpContext.Response.StatusCode : 200, - ContentType = "application/json", - Body = body - }; + case null: + break; + case IResult endpointResult: + await endpointResult.ExecuteAsync(httpContext).ConfigureAwait(false); + break; + default: + // A non-IResult return is serialized as JSON by the framework — mirror that. + await httpContext.Response.WriteAsJsonAsync(result, result.GetType(), options: null, contentType: null, httpContext.RequestAborted).ConfigureAwait(false); + break; + } - var setOptions = new HybridCacheEntryOptions - { - Expiration = options.DefaultTtl, - LocalCacheExpiration = options.DefaultTtl < TimeSpan.FromMinutes(2) ? options.DefaultTtl : TimeSpan.FromMinutes(2), - }; - await hybridCache.SetAsync(cacheKey, responseToCache, setOptions, tags, httpContext.RequestAborted).ConfigureAwait(false); + var statusCode = httpContext.Response.StatusCode is > 0 and < 600 + ? httpContext.Response.StatusCode + : StatusCodes.Status200OK; + return (statusCode, httpContext.Response.ContentType, buffer.ToArray()); + } + finally + { + httpContext.Response.Body = originalBody; } - // Best-effort caching: idempotency replay is a convenience, not a correctness requirement - catch (Exception ex) when (ex is not OperationCanceledException) + } + + private static async ValueTask TryReserveAsync( + IConnectionMultiplexer? multiplexer, string reservationKey, TimeSpan ttl, CancellationToken ct) + { + if (multiplexer is not null) + { + var db = multiplexer.GetDatabase(); + return await db.StringSetAsync(reservationKey, "1", ttl, When.NotExists).ConfigureAwait(false); + } + + _ = ct; + return InFlight.TryAdd(reservationKey, 0); + } + + private static async ValueTask ReleaseReservationAsync(IConnectionMultiplexer? multiplexer, string reservationKey) + { + if (multiplexer is not null) { - logger.LogWarning(ex, "Failed to cache idempotent response for key {KeyHash}", HashKey(idempotencyKey)); + await multiplexer.GetDatabase().KeyDeleteAsync(reservationKey).ConfigureAwait(false); + return; } - return result; + InFlight.TryRemove(reservationKey, out _); } private static string HashKey(string key) diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs new file mode 100644 index 0000000000..3460d98f62 --- /dev/null +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -0,0 +1,215 @@ +using System.Text.Json; +using FSH.Framework.Web.Idempotency; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Caching.Distributed; +using Microsoft.Extensions.Caching.Hybrid; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; + +namespace Framework.Tests.Web; + +/// +/// Runtime repro for audit findings API-01 (idempotency replay stores the wrong wire shape and +/// status) and CONC-01 (no in-flight reservation, so concurrent duplicate keys execute twice). +/// +/// These exercise the REAL . The one part we substitute is a +/// faithful single-store idempotency backend (write via HybridCache round-trips to the SAME +/// IDistributedCache the probe reads, using the filter's own serialization). This isolates the +/// filter's shape/status logic from the app's test-env cache split — the caveat that permanently +/// skips ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused. +/// +public sealed class IdempotencyEndpointFilterReplayTests +{ + private const string Key = "fixed-idempotency-key"; + + private static readonly JsonSerializerOptions CamelCase = + new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + + // ─── API-01: replayed status ───────────────────────────────────── + + [Fact] + public async Task Replay_Should_PreserveCreatedStatus_When_FirstResponseWas201() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var id = Guid.NewGuid(); + + // First call: handler returns a 201 Created (the framework would execute it AFTER the filter + // returns — so at cache time Response.StatusCode is still the default 200). + var first = NewContext(provider); + await filter.InvokeAsync( + new TestFilterContext(first), + _ => ValueTask.FromResult(TypedResults.Created($"/samples/{id}", new SampleDto(id, "widget")))); + + // Second call, same key: must replay. + var replayBody = new MemoryStream(); + var second = NewContext(provider, replayBody); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => throw new InvalidOperationException("handler must NOT run on an idempotent replay")); + + second.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeTrue( + "sanity: the replay path must actually engage, otherwise this test would be vacuous"); + second.Response.StatusCode.ShouldBe( + StatusCodes.Status201Created, + "a correct replay must reproduce the original 201 Created — the filter captures Response.StatusCode " + + "BEFORE the IResult executes, so it caches (and replays) 200 instead."); + } + + // ─── API-01: replayed body wire shape ──────────────────────────── + + [Fact] + public async Task Replay_Should_ReturnPlainDtoBody_Not_WrappedIResult() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var id = Guid.NewGuid(); + + var first = NewContext(provider); + await filter.InvokeAsync( + new TestFilterContext(first), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(id, "widget")))); + + var replayBody = new MemoryStream(); + var second = NewContext(provider, replayBody); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => throw new InvalidOperationException("handler must NOT run on an idempotent replay")); + + replayBody.Position = 0; + using var doc = JsonDocument.Parse(replayBody.ToArray()); + + doc.RootElement.TryGetProperty("value", out _).ShouldBeFalse( + "a correct replay body is the wire DTO; the filter caches SerializeToUtf8Bytes(result) where " + + "result is the wrapped Ok/Created, leaking the {\"value\":...} envelope onto the wire."); + doc.RootElement.TryGetProperty("id", out _).ShouldBeTrue( + "the plain DTO's own properties should be at the JSON root"); + } + + // ─── CONC-01: no in-flight reservation ─────────────────────────── + + [Fact] + public async Task Filter_Should_ExecuteHandlerOnce_When_TwoConcurrentRequestsShareKey() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + + int executions = 0; + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + // First request enters the handler and holds the in-flight reservation until released. + EndpointFilterDelegate first = async _ => + { + Interlocked.Increment(ref executions); + started.SetResult(); + await release.Task.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + return TypedResults.Ok(new SampleDto(Guid.NewGuid(), "first")); + }; + + // Second request shares the key; its handler must never run while the first is in flight. + EndpointFilterDelegate second = _ => + { + Interlocked.Increment(ref executions); + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "second"))); + }; + + var firstCall = filter.InvokeAsync(new TestFilterContext(NewContext(provider)), first).AsTask(); + await started.Task.WaitAsync(TimeSpan.FromSeconds(5)); // first now holds the reservation + + var secondResult = await filter.InvokeAsync(new TestFilterContext(NewContext(provider)), second); + + release.SetResult(); + await firstCall.WaitAsync(TimeSpan.FromSeconds(10)); + + executions.ShouldBe( + 1, + "an idempotent endpoint must execute the handler exactly once for concurrent duplicate keys; " + + "the second request should be rejected while the first is in flight."); + (secondResult as IStatusCodeHttpResult)?.StatusCode.ShouldBe( + StatusCodes.Status409Conflict, + "a concurrent duplicate that arrives while the original is still running gets 409 Conflict."); + } + + // ─── harness ───────────────────────────────────────────────────── + + private static ServiceProvider BuildProvider() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddDistributedMemoryCache(); + services.AddSingleton>(Options.Create(new IdempotencyOptions())); + services.AddSingleton(sp => + new WriteThroughHybridCache(sp.GetRequiredService(), CamelCase)); + return services.BuildServiceProvider(); + } + + private static DefaultHttpContext NewContext(IServiceProvider provider, Stream? responseBody = null) + { + var context = new DefaultHttpContext { RequestServices = provider }; + context.Request.Method = "POST"; + context.Request.Headers["Idempotency-Key"] = Key; + if (responseBody is not null) + { + context.Response.Body = responseBody; + } + + return context; + } + + private sealed record SampleDto(Guid Id, string Name); + + private sealed class TestFilterContext : EndpointFilterInvocationContext + { + public TestFilterContext(HttpContext httpContext) => HttpContext = httpContext; + + public override HttpContext HttpContext { get; } + + public override IList Arguments { get; } = new List(); + + public override T GetArgument(int index) => (T)Arguments[index]!; + } + + /// + /// A faithful single-store idempotency backend: HybridCache.SetAsync serializes the cached + /// response with the exact options the filter's probe uses and writes it into the same + /// IDistributedCache, so a correctly-wired backend's replay is what surfaces the filter's bug. + /// + private sealed class WriteThroughHybridCache : HybridCache + { + private readonly IDistributedCache _store; + private readonly JsonSerializerOptions _options; + + public WriteThroughHybridCache(IDistributedCache store, JsonSerializerOptions options) + { + _store = store; + _options = options; + } + + public override ValueTask GetOrCreateAsync( + string key, + TState state, + Func> factory, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default) => factory(state, cancellationToken); + + public override async ValueTask SetAsync( + string key, + T value, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default) + { + var bytes = JsonSerializer.SerializeToUtf8Bytes(value, _options); + await _store.SetAsync(key, bytes, new DistributedCacheEntryOptions(), cancellationToken) + .ConfigureAwait(false); + } + + public override ValueTask RemoveAsync(string key, CancellationToken cancellationToken = default) => + ValueTask.CompletedTask; + + public override ValueTask RemoveByTagAsync(string tag, CancellationToken cancellationToken = default) => + ValueTask.CompletedTask; + } +} From e669afc73d0b36e2eb697ad87647865311b538a7 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Sat, 11 Jul 2026 18:20:03 -0300 Subject: [PATCH 02/16] fix(web): make idempotency replay actually engage (symmetric cache store) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write went through HybridCache.SetAsync while the probe read IDistributedCache by the raw key. HybridCache keys its L2 entries under its own scheme, so the probe never found the entry and replay silently never engaged — even in production. Proven by un-skipping ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused, which now passes. Write to the same IDistributedCache, key and serializer the probe uses. Idempotency entries are short-lived (TTL) and their HybridCache tag-purge path was unused, so dropping HybridCache here loses nothing. --- .../Idempotency/IdempotencyEndpointFilter.cs | 26 ++++---- .../IdempotencyEndpointFilterReplayTests.cs | 61 ++----------------- .../Tests/Chat/ChatSendMessageTests.cs | 2 +- 3 files changed, 18 insertions(+), 71 deletions(-) diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index 002cf4cbdb..39869bbebe 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -6,7 +6,6 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Caching.Distributed; -using Microsoft.Extensions.Caching.Hybrid; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -20,9 +19,9 @@ namespace FSH.Framework.Web.Idempotency; /// for subsequent requests with the same key. /// /// -/// Uses directly for the probe read (bypassing -/// 's factory-mandatory API) and -/// for the write path so replays benefit from L1 and the regular tag invalidation story. +/// Uses for both the probe read and the write, on the same raw key +/// and serializer, so the two are symmetric (a HybridCache write keys its L2 entries under its own +/// scheme, which a raw-key probe never finds — replay then silently never engages). /// The handler result is executed into a buffer so the cached payload is the real wire body and /// status code (an Ok<T>/Created<T> wrapper would otherwise be serialized /// verbatim, and Response.StatusCode is still the default at filter time — the IResult sets @@ -59,13 +58,11 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter } var distributedCache = httpContext.RequestServices.GetRequiredService(); - var hybridCache = httpContext.RequestServices.GetRequiredService(); var logger = httpContext.RequestServices.GetRequiredService>(); // Include tenant context in cache key for isolation var tenantId = httpContext.User.FindFirst("tenant")?.Value ?? "global"; var cacheKey = CacheKeys.IdempotencyEntry(tenantId, idempotencyKey); - var tags = new[] { CacheKeys.Tags.Idempotency, CacheKeys.Tags.Tenant(tenantId) }; // Probe-only read via IDistributedCache (real GetAsync, null on miss — unlike HybridCache's // factory). Bypasses L1: replays are rare vs first-calls, so L1 warmth has little value. @@ -108,7 +105,10 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter await httpContext.Response.Body.WriteAsync(body, httpContext.RequestAborted).ConfigureAwait(false); } - // Cache the response through HybridCache so the tag invalidation path works for purges. + // Write to the SAME store + key the probe reads. HybridCache.SetAsync keys its L2 entries + // under its own scheme, so a raw-key IDistributedCache probe never found them and replay + // silently never engaged. Idempotency entries are short-lived (TTL) and their tag-purge + // path was unused, so IDistributedCache alone — symmetric with the probe — is correct. try { var responseToCache = new CachedIdempotentResponse @@ -118,12 +118,12 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter Body = body, }; - var setOptions = new HybridCacheEntryOptions - { - Expiration = options.DefaultTtl, - LocalCacheExpiration = options.DefaultTtl < TimeSpan.FromMinutes(2) ? options.DefaultTtl : TimeSpan.FromMinutes(2), - }; - await hybridCache.SetAsync(cacheKey, responseToCache, setOptions, tags, httpContext.RequestAborted).ConfigureAwait(false); + var payload = JsonSerializer.SerializeToUtf8Bytes(responseToCache, JsonOpts); + await distributedCache.SetAsync( + cacheKey, + payload, + new DistributedCacheEntryOptions { AbsoluteExpirationRelativeToNow = options.DefaultTtl }, + httpContext.RequestAborted).ConfigureAwait(false); } // Best-effort caching: idempotency replay is a convenience, not a correctness requirement catch (Exception ex) when (ex is not OperationCanceledException) diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs index 3460d98f62..154d3d7496 100644 --- a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -2,29 +2,21 @@ using FSH.Framework.Web.Idempotency; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Caching.Distributed; -using Microsoft.Extensions.Caching.Hybrid; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; namespace Framework.Tests.Web; /// -/// Runtime repro for audit findings API-01 (idempotency replay stores the wrong wire shape and -/// status) and CONC-01 (no in-flight reservation, so concurrent duplicate keys execute twice). -/// -/// These exercise the REAL . The one part we substitute is a -/// faithful single-store idempotency backend (write via HybridCache round-trips to the SAME -/// IDistributedCache the probe reads, using the filter's own serialization). This isolates the -/// filter's shape/status logic from the app's test-env cache split — the caveat that permanently -/// skips ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused. +/// Regression for audit findings API-01 (idempotency replay stored the wrong wire shape and status) +/// and CONC-01 (no in-flight reservation, so concurrent duplicate keys executed twice). These +/// exercise the REAL against a real in-memory +/// — the same store the filter now uses for both probe and write. /// public sealed class IdempotencyEndpointFilterReplayTests { private const string Key = "fixed-idempotency-key"; - private static readonly JsonSerializerOptions CamelCase = - new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; - // ─── API-01: replayed status ───────────────────────────────────── [Fact] @@ -139,8 +131,6 @@ private static ServiceProvider BuildProvider() services.AddLogging(); services.AddDistributedMemoryCache(); services.AddSingleton>(Options.Create(new IdempotencyOptions())); - services.AddSingleton(sp => - new WriteThroughHybridCache(sp.GetRequiredService(), CamelCase)); return services.BuildServiceProvider(); } @@ -169,47 +159,4 @@ private sealed class TestFilterContext : EndpointFilterInvocationContext public override T GetArgument(int index) => (T)Arguments[index]!; } - - /// - /// A faithful single-store idempotency backend: HybridCache.SetAsync serializes the cached - /// response with the exact options the filter's probe uses and writes it into the same - /// IDistributedCache, so a correctly-wired backend's replay is what surfaces the filter's bug. - /// - private sealed class WriteThroughHybridCache : HybridCache - { - private readonly IDistributedCache _store; - private readonly JsonSerializerOptions _options; - - public WriteThroughHybridCache(IDistributedCache store, JsonSerializerOptions options) - { - _store = store; - _options = options; - } - - public override ValueTask GetOrCreateAsync( - string key, - TState state, - Func> factory, - HybridCacheEntryOptions? options = null, - IEnumerable? tags = null, - CancellationToken cancellationToken = default) => factory(state, cancellationToken); - - public override async ValueTask SetAsync( - string key, - T value, - HybridCacheEntryOptions? options = null, - IEnumerable? tags = null, - CancellationToken cancellationToken = default) - { - var bytes = JsonSerializer.SerializeToUtf8Bytes(value, _options); - await _store.SetAsync(key, bytes, new DistributedCacheEntryOptions(), cancellationToken) - .ConfigureAwait(false); - } - - public override ValueTask RemoveAsync(string key, CancellationToken cancellationToken = default) => - ValueTask.CompletedTask; - - public override ValueTask RemoveByTagAsync(string tag, CancellationToken cancellationToken = default) => - ValueTask.CompletedTask; - } } diff --git a/src/Tests/Integration.Tests/Tests/Chat/ChatSendMessageTests.cs b/src/Tests/Integration.Tests/Tests/Chat/ChatSendMessageTests.cs index 004a72066a..28bbd65535 100644 --- a/src/Tests/Integration.Tests/Tests/Chat/ChatSendMessageTests.cs +++ b/src/Tests/Integration.Tests/Tests/Chat/ChatSendMessageTests.cs @@ -71,7 +71,7 @@ public async Task SendMessage_Should_Trim_Body_Whitespace() // ─── idempotency ───────────────────────────────────────────────── - [Fact(Skip = "Idempotency replay does not engage in the test environment — IDistributedCache (probe) and HybridCache (write-through) are wired to separate in-process stores, so the second call never sees the cached response. Same caveat as IdempotencyFilterTests.cs: 'full replay-with-matching-body coverage is not yet possible'. Backlog item 2.4b tracks the fix.")] + [Fact] public async Task SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused() { using var client = await _auth.CreateRootAdminClientAsync(); From 8a8761a88ce21d4803bfd014414fb3221a6eccdb Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 13 Jul 2026 15:47:33 -0300 Subject: [PATCH 03/16] fix(web): short-TTL, fail-open idempotency reservation Address review on #1333: - Reservation used the 24h response TTL, so a crash between reserving and the finally-release stranded the Redis lock for a day (every retry 409s). Add IdempotencyOptions.ReservationTtl (default 1m), decoupled from DefaultTtl. - Reserve now fails open on a transient Redis error instead of 500ing the request, matching the best-effort stance of the response write. - Guard the release KeyDeleteAsync so a Redis fault can't throw out of the finally. Tests: reservation uses ReservationTtl not DefaultTtl; a faulting Redis on reserve/release proceeds without throwing (exercises the Redis NX branch the prior tests skipped). --- .../Idempotency/IdempotencyEndpointFilter.cs | 35 +++++++-- .../Web/Idempotency/IdempotencyOptions.cs | 9 +++ .../IdempotencyEndpointFilterReplayTests.cs | 72 ++++++++++++++++++- 3 files changed, 107 insertions(+), 9 deletions(-) diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index 39869bbebe..f2172103a6 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -75,7 +75,7 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // Atomically reserve the key so concurrent duplicates don't both execute the handler. var multiplexer = httpContext.RequestServices.GetService(); var reservationKey = cacheKey + ":inflight"; - if (!await TryReserveAsync(multiplexer, reservationKey, options.DefaultTtl, httpContext.RequestAborted).ConfigureAwait(false)) + if (!await TryReserveAsync(multiplexer, reservationKey, options.ReservationTtl, logger, idempotencyKey, httpContext.RequestAborted).ConfigureAwait(false)) { // Another request with this key is in flight. It may have finished between the probe // and the reservation — re-probe once, otherwise report the in-progress conflict. @@ -137,7 +137,7 @@ await distributedCache.SetAsync( } finally { - await ReleaseReservationAsync(multiplexer, reservationKey).ConfigureAwait(false); + await ReleaseReservationAsync(multiplexer, reservationKey, logger, idempotencyKey).ConfigureAwait(false); } } @@ -207,23 +207,44 @@ await distributedCache.SetAsync( } private static async ValueTask TryReserveAsync( - IConnectionMultiplexer? multiplexer, string reservationKey, TimeSpan ttl, CancellationToken ct) + IConnectionMultiplexer? multiplexer, string reservationKey, TimeSpan ttl, ILogger logger, string idempotencyKey, CancellationToken ct) { if (multiplexer is not null) { - var db = multiplexer.GetDatabase(); - return await db.StringSetAsync(reservationKey, "1", ttl, When.NotExists).ConfigureAwait(false); + try + { + var db = multiplexer.GetDatabase(); + return await db.StringSetAsync(reservationKey, "1", ttl, When.NotExists).ConfigureAwait(false); + } + // Fail open on a Redis blip: the reservation is a concurrency convenience, not a correctness + // requirement (the response cache still dedups later retries). Proceed rather than 500 the + // request, matching the best-effort stance the response write already takes. + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogWarning(ex, "Idempotency reservation failed for key {KeyHash}; proceeding without it", HashKey(idempotencyKey)); + return true; + } } _ = ct; return InFlight.TryAdd(reservationKey, 0); } - private static async ValueTask ReleaseReservationAsync(IConnectionMultiplexer? multiplexer, string reservationKey) + private static async ValueTask ReleaseReservationAsync(IConnectionMultiplexer? multiplexer, string reservationKey, ILogger logger, string idempotencyKey) { if (multiplexer is not null) { - await multiplexer.GetDatabase().KeyDeleteAsync(reservationKey).ConfigureAwait(false); + try + { + await multiplexer.GetDatabase().KeyDeleteAsync(reservationKey).ConfigureAwait(false); + } + // Best-effort release: a Redis fault here must not throw out of the finally. The short + // ReservationTtl expires the key anyway, so a missed delete self-heals in seconds. + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogWarning(ex, "Failed to release idempotency reservation for key {KeyHash}", HashKey(idempotencyKey)); + } + return; } diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyOptions.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyOptions.cs index fd8bdac92b..73abe99891 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyOptions.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyOptions.cs @@ -15,6 +15,15 @@ public sealed class IdempotencyOptions /// public TimeSpan DefaultTtl { get; set; } = TimeSpan.FromHours(24); + /// + /// Time-to-live for the in-flight reservation that serializes concurrent duplicate keys. + /// Decoupled from : it must only outlast the handler's execution, so a + /// crash between reserving and releasing frees the key in seconds instead of stranding it for the + /// full response TTL (every retry would 409 until it expired). Must exceed the longest expected + /// handler runtime — if it lapses mid-request a concurrent duplicate can slip through. Default: 1 minute. + /// + public TimeSpan ReservationTtl { get; set; } = TimeSpan.FromMinutes(1); + /// /// Maximum allowed length for the idempotency key. Default: 128 characters. /// diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs index 154d3d7496..bba14bff0c 100644 --- a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -4,6 +4,8 @@ using Microsoft.Extensions.Caching.Distributed; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; +using NSubstitute; +using StackExchange.Redis; namespace Framework.Tests.Web; @@ -123,17 +125,83 @@ public async Task Filter_Should_ExecuteHandlerOnce_When_TwoConcurrentRequestsSha "a concurrent duplicate that arrives while the original is still running gets 409 Conflict."); } + // ─── HIGH: reservation TTL is the short ReservationTtl, not the 24h response TTL ───── + + [Fact] + public async Task Reservation_Should_UseReservationTtl_Not_ResponseTtl() + { + var options = new IdempotencyOptions + { + ReservationTtl = TimeSpan.FromSeconds(37), // distinct from DefaultTtl to prove which one is used + }; + var db = Substitute.For(); + TimeSpan? capturedTtl = null; + db.StringSetAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(ci => { capturedTtl = ci.ArgAt(2); return Task.FromResult(true); }); + var provider = BuildProvider(options, RedisMultiplexer(db)); + var filter = new IdempotencyEndpointFilter(); + + await filter.InvokeAsync( + new TestFilterContext(NewContext(provider)), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget")))); + + capturedTtl.ShouldBe( + options.ReservationTtl, + "the in-flight reservation must use the short ReservationTtl; keying it to the 24h response TTL " + + "would strand the lock for a day if the process is killed before the finally-release runs."); + capturedTtl.ShouldNotBe(options.DefaultTtl); + } + + // ─── MEDIUM + nit: a Redis fault on reserve/release fails open, never 500s ─────────── + + [Fact] + public async Task Filter_Should_ProceedWithoutThrowing_When_RedisReservationFaults() + { + var db = Substitute.For(); + db.StringSetAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromException(new RedisException("reserve blip"))); + db.KeyDeleteAsync(Arg.Any(), Arg.Any()) + .Returns(Task.FromException(new RedisException("release blip"))); + var provider = BuildProvider(new IdempotencyOptions(), RedisMultiplexer(db)); + var filter = new IdempotencyEndpointFilter(); + + int executions = 0; + var result = await filter.InvokeAsync( + new TestFilterContext(NewContext(provider)), + _ => { executions++; return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget"))); }); + + executions.ShouldBe( + 1, + "a transient Redis error on the reservation must fail open — the handler still runs. On main " + + "idempotency degraded gracefully; treating the reservation as authoritative would 500 the request."); + result.ShouldNotBeNull("the request must complete normally, not throw out of the filter"); + } + // ─── harness ───────────────────────────────────────────────────── - private static ServiceProvider BuildProvider() + private static ServiceProvider BuildProvider() => BuildProvider(new IdempotencyOptions(), multiplexer: null); + + private static ServiceProvider BuildProvider(IdempotencyOptions options, IConnectionMultiplexer? multiplexer) { var services = new ServiceCollection(); services.AddLogging(); services.AddDistributedMemoryCache(); - services.AddSingleton>(Options.Create(new IdempotencyOptions())); + services.AddSingleton>(Options.Create(options)); + if (multiplexer is not null) + { + services.AddSingleton(multiplexer); + } + return services.BuildServiceProvider(); } + private static IConnectionMultiplexer RedisMultiplexer(IDatabase db) + { + var mux = Substitute.For(); + mux.GetDatabase(Arg.Any(), Arg.Any()).Returns(db); + return mux; + } + private static DefaultHttpContext NewContext(IServiceProvider provider, Stream? responseBody = null) { var context = new DefaultHttpContext { RequestServices = provider }; From f7c8c3290268b6c57db07651f3f073950cd947d5 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 04:57:09 -0300 Subject: [PATCH 04/16] fix(web): make the stored idempotent response outlive the request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects surfaced by re-reading the whole filter rather than the delta. The response store was tied to the client's connection, so the retry that idempotency exists to serve re-executed the handler. Two paths caused it, not one: the body write to the client ran before the store, so a closed socket threw and skipped it entirely; and the capture itself ran under RequestAborted, where WriteAsJsonAsync swallows the cancellation and hands back an EMPTY body — which was then cached and replayed as a 200 for the full 24h TTL. The capture now runs with the abort token detached (it writes to an in-memory buffer, never the socket), the store runs before the client write and on CancellationToken.None, and only then does the body go out. Replay dropped every response header, so a replayed 201 arrived without Location: a client that follows the header worked on the first call and broke on the retry. The captured response now carries an allow-listed set (Location, ETag) and replays it. Transport and host-owned headers stay out — a stale Content-Length would corrupt the response. Non-2xx is no longer stored. Faithful status capture made the pre-existing behaviour bite: a transient downstream failure locked the caller out of that key for 24h. A failure is not a record of a committed side effect. CachedIdempotentResponse is no longer a HybridCache type, so its [ImmutableObject(true)] contract (and the CachedTypeContractTests entry asserting it) described a store this filter stopped using. Both dropped. The new Headers property defaults to empty so entries written before it deserialize. Tests: replayed 201 carries Location; a first call whose client disconnects still replays the real DTO body; a non-2xx first response lets the retry run. All three fail on the previous commit and pass here. --- .agents/rules/security.md | 2 +- .../Idempotency/CachedIdempotentResponse.cs | 17 ++- .../Idempotency/IdempotencyEndpointFilter.cs | 129 +++++++++++++----- .../Caching.Tests/CachedTypeContractTests.cs | 2 - .../IdempotencyEndpointFilterReplayTests.cs | 103 ++++++++++++++ 5 files changed, 208 insertions(+), 45 deletions(-) diff --git a/.agents/rules/security.md b/.agents/rules/security.md index b3fb38404b..90c835c3eb 100644 --- a/.agents/rules/security.md +++ b/.agents/rules/security.md @@ -17,7 +17,7 @@ Chained partitioned fixed-window limiter: **tenant → user → IP** (defaults 1 ## Idempotency (`Web/Idempotency/`) -Opt-in per endpoint with **`.WithIdempotency()`**. Reads the `Idempotency-Key` header (max 128 chars, 24h TTL); replays return the cached response with `Idempotency-Replayed: true`. Cache key is tenant-scoped (`CacheKeys.IdempotencyEntry`). Put it on POSTs that must be replay-safe (e.g. CreateTenant). +Opt-in per endpoint with **`.WithIdempotency()`**. Reads the `Idempotency-Key` header (max 128 chars, `DefaultTtl` 24h); replays return the cached status, body and the allow-listed headers (`Location`, `ETag`) plus `Idempotency-Replayed: true`. Cache key is tenant-scoped (`CacheKeys.IdempotencyEntry`) and probe and write share one `IDistributedCache` + key + serializer — asymmetry there makes replay silently never engage. Only **2xx** is stored: a failure isn't a record of a committed side effect, and caching it would lock the key out for the full TTL. The store happens **before** the body reaches the client and on `CancellationToken.None`, so a client that times out and retries replays instead of re-executing. Concurrent duplicates are serialized by an in-flight reservation (Redis `SET NX`, else in-process) on the short `ReservationTtl` (default 1m, must outlast the slowest handler); a duplicate still in flight gets **409**. Reserve and release both fail open. Put it on POSTs that must be replay-safe (e.g. CreateTenant). ## Quota enforcement (`Quota/`) diff --git a/src/BuildingBlocks/Web/Idempotency/CachedIdempotentResponse.cs b/src/BuildingBlocks/Web/Idempotency/CachedIdempotentResponse.cs index ed6d686c3f..aa3ead8370 100644 --- a/src/BuildingBlocks/Web/Idempotency/CachedIdempotentResponse.cs +++ b/src/BuildingBlocks/Web/Idempotency/CachedIdempotentResponse.cs @@ -1,18 +1,21 @@ -using System.ComponentModel; - namespace FSH.Framework.Web.Idempotency; /// /// A cached HTTP response for idempotent replay. /// -/// -/// Marked + sealed so HybridCache can reuse the -/// in-process instance across requests without re-deserializing on every L1 hit. -/// -[ImmutableObject(true)] public sealed record CachedIdempotentResponse { public int StatusCode { get; init; } + public string? ContentType { get; init; } + public byte[] Body { get; init; } = []; + + /// + /// Response headers replayed alongside the body. Only headers that carry meaning for the caller + /// are captured (see the filter's allow-list) — the host sets the transport ones itself, and + /// replaying a stale Content-Length or Transfer-Encoding would corrupt the response. + /// Defaults to empty so entries written before headers were captured still deserialize. + /// + public IReadOnlyDictionary Headers { get; init; } = new Dictionary(StringComparer.OrdinalIgnoreCase); } diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index f2172103a6..ea51648585 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -9,6 +9,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using Microsoft.Extensions.Primitives; using StackExchange.Redis; namespace FSH.Framework.Web.Idempotency; @@ -27,11 +28,21 @@ namespace FSH.Framework.Web.Idempotency; /// verbatim, and Response.StatusCode is still the default at filter time — the IResult sets /// it only when it executes). Concurrent duplicate keys are serialized by an atomic in-flight /// reservation (Redis SET NX when a multiplexer is registered, an in-process set otherwise). +/// The stored response is written before the body reaches the client and with a token that cannot be +/// cancelled: it is the durable record that the side effect already happened, so it has to outlive the +/// request that produced it — a client that times out and retries is the commonest duplicate there is. /// public sealed class IdempotencyEndpointFilter : IEndpointFilter { private static readonly JsonSerializerOptions JsonOpts = new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + // Response headers worth replaying. Allow-list rather than block-list: executing an IResult is + // exactly when these get set (Created(uri, value) writes Location), and a replayed 201 without + // Location breaks any client that follows it — under retry conditions nobody tests. Everything + // else is either transport (Content-Length, Transfer-Encoding) or host-owned (Date, Server), and + // replaying a stale value there corrupts the response. + private static readonly string[] ReplayableHeaders = ["Location", "ETag"]; + // In-process reservation used when no Redis multiplexer is registered. Single-instance only — // a multi-instance host in this stack already runs Redis (shared Data Protection key ring), so // the Redis branch below covers every deployment where cross-instance duplicates are possible. @@ -75,7 +86,7 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // Atomically reserve the key so concurrent duplicates don't both execute the handler. var multiplexer = httpContext.RequestServices.GetService(); var reservationKey = cacheKey + ":inflight"; - if (!await TryReserveAsync(multiplexer, reservationKey, options.ReservationTtl, logger, idempotencyKey, httpContext.RequestAborted).ConfigureAwait(false)) + if (!await TryReserveAsync(multiplexer, reservationKey, options.ReservationTtl, logger, idempotencyKey).ConfigureAwait(false)) { // Another request with this key is in flight. It may have finished between the probe // and the reservation — re-probe once, otherwise report the in-progress conflict. @@ -92,43 +103,28 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // Execute the result into a buffer to capture the real wire body + status code, then // serve that buffer to the client. Returning the IResult unexecuted would leave // Response.StatusCode at its default and cache the wrapper object, not the wire body. - var (statusCode, contentType, body) = await ExecuteAndCaptureAsync(result, httpContext).ConfigureAwait(false); + var captured = await ExecuteAndCaptureAsync(result, httpContext).ConfigureAwait(false); - httpContext.Response.StatusCode = statusCode; - if (contentType is not null) + httpContext.Response.StatusCode = captured.StatusCode; + if (captured.ContentType is not null) { - httpContext.Response.ContentType = contentType; + httpContext.Response.ContentType = captured.ContentType; } - if (body.Length > 0) + // Store BEFORE the body goes to the client, and only on success. The handler's side effect + // has already committed at this point, so the record of it must not depend on the client + // still being there; writing to a socket the client closed throws, and doing that first + // would skip the store and let the retry re-execute the handler. Non-2xx is not a record + // of a committed side effect — caching it would lock the key out for the full TTL after a + // transient downstream failure, so a retry with the same key is allowed to run again. + if (captured.StatusCode is >= 200 and < 300) { - await httpContext.Response.Body.WriteAsync(body, httpContext.RequestAborted).ConfigureAwait(false); + await CacheResponseAsync(distributedCache, cacheKey, captured, options.DefaultTtl, logger, idempotencyKey).ConfigureAwait(false); } - // Write to the SAME store + key the probe reads. HybridCache.SetAsync keys its L2 entries - // under its own scheme, so a raw-key IDistributedCache probe never found them and replay - // silently never engaged. Idempotency entries are short-lived (TTL) and their tag-purge - // path was unused, so IDistributedCache alone — symmetric with the probe — is correct. - try - { - var responseToCache = new CachedIdempotentResponse - { - StatusCode = statusCode, - ContentType = contentType ?? "application/json", - Body = body, - }; - - var payload = JsonSerializer.SerializeToUtf8Bytes(responseToCache, JsonOpts); - await distributedCache.SetAsync( - cacheKey, - payload, - new DistributedCacheEntryOptions { AbsoluteExpirationRelativeToNow = options.DefaultTtl }, - httpContext.RequestAborted).ConfigureAwait(false); - } - // Best-effort caching: idempotency replay is a convenience, not a correctness requirement - catch (Exception ex) when (ex is not OperationCanceledException) + if (captured.Body.Length > 0) { - logger.LogWarning(ex, "Failed to cache idempotent response for key {KeyHash}", HashKey(idempotencyKey)); + await httpContext.Response.Body.WriteAsync(captured.Body, httpContext.RequestAborted).ConfigureAwait(false); } // Response already written to the body directly; return an empty result so the framework @@ -141,6 +137,39 @@ await distributedCache.SetAsync( } } + // Write to the SAME store + key the probe reads. HybridCache.SetAsync keys its L2 entries under + // its own scheme, so a raw-key IDistributedCache probe never found them and replay silently never + // engaged. Idempotency entries are short-lived (TTL) and their tag-purge path was unused, so + // IDistributedCache alone — symmetric with the probe — is correct. + private static async ValueTask CacheResponseAsync( + IDistributedCache distributedCache, + string cacheKey, + CachedIdempotentResponse response, + TimeSpan ttl, + ILogger logger, + string idempotencyKey) + { + try + { + var payload = JsonSerializer.SerializeToUtf8Bytes(response, JsonOpts); + + // CancellationToken.None on purpose: RequestAborted is already signalled whenever this + // matters (client hung up), and cancelling the store is what makes the retry re-execute. + await distributedCache.SetAsync( + cacheKey, + payload, + new DistributedCacheEntryOptions { AbsoluteExpirationRelativeToNow = ttl }, + CancellationToken.None).ConfigureAwait(false); + } + // Best-effort caching: a store that is down degrades idempotency to a convenience rather + // than 500ing a request whose side effect already committed. The token above is None, so + // an OperationCanceledException here is not a client disconnect and is left to propagate. + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogWarning(ex, "Failed to cache idempotent response for key {KeyHash}", HashKey(idempotencyKey)); + } + } + private static async ValueTask ProbeAsync( IDistributedCache cache, string cacheKey, CancellationToken ct) { @@ -165,6 +194,11 @@ await distributedCache.SetAsync( httpContext.Response.ContentType = cached.ContentType; } + foreach (var header in cached.Headers) + { + httpContext.Response.Headers[header.Key] = header.Value; + } + if (cached.Body.Length > 0) { await httpContext.Response.Body.WriteAsync(cached.Body, httpContext.RequestAborted).ConfigureAwait(false); @@ -174,12 +208,20 @@ await distributedCache.SetAsync( return Results.Empty; } - private static async Task<(int StatusCode, string? ContentType, byte[] Body)> ExecuteAndCaptureAsync( + private static async Task ExecuteAndCaptureAsync( object? result, HttpContext httpContext) { var originalBody = httpContext.Response.Body; + var originalAborted = httpContext.RequestAborted; await using var buffer = new MemoryStream(); httpContext.Response.Body = buffer; + + // Detach the client's abort token while capturing. The result is being written to an + // in-memory buffer, never the socket, so a client that hung up must not truncate it — and + // ASP.NET's WriteAsJsonAsync reads RequestAborted itself and swallows the cancellation, so + // the capture would silently come back EMPTY and that empty body would be cached and + // replayed for the full TTL. + httpContext.RequestAborted = CancellationToken.None; try { switch (result) @@ -191,23 +233,41 @@ await distributedCache.SetAsync( break; default: // A non-IResult return is serialized as JSON by the framework — mirror that. - await httpContext.Response.WriteAsJsonAsync(result, result.GetType(), options: null, contentType: null, httpContext.RequestAborted).ConfigureAwait(false); + await httpContext.Response.WriteAsJsonAsync(result, result.GetType(), options: null, contentType: null, CancellationToken.None).ConfigureAwait(false); break; } var statusCode = httpContext.Response.StatusCode is > 0 and < 600 ? httpContext.Response.StatusCode : StatusCodes.Status200OK; - return (statusCode, httpContext.Response.ContentType, buffer.ToArray()); + + var headers = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var name in ReplayableHeaders) + { + var value = httpContext.Response.Headers[name]; + if (!StringValues.IsNullOrEmpty(value)) + { + headers[name] = value.ToString(); + } + } + + return new CachedIdempotentResponse + { + StatusCode = statusCode, + ContentType = httpContext.Response.ContentType ?? "application/json", + Body = buffer.ToArray(), + Headers = headers, + }; } finally { httpContext.Response.Body = originalBody; + httpContext.RequestAborted = originalAborted; } } private static async ValueTask TryReserveAsync( - IConnectionMultiplexer? multiplexer, string reservationKey, TimeSpan ttl, ILogger logger, string idempotencyKey, CancellationToken ct) + IConnectionMultiplexer? multiplexer, string reservationKey, TimeSpan ttl, ILogger logger, string idempotencyKey) { if (multiplexer is not null) { @@ -226,7 +286,6 @@ private static async ValueTask TryReserveAsync( } } - _ = ct; return InFlight.TryAdd(reservationKey, 0); } diff --git a/src/Tests/Caching.Tests/CachedTypeContractTests.cs b/src/Tests/Caching.Tests/CachedTypeContractTests.cs index 46a08129c0..29b766c809 100644 --- a/src/Tests/Caching.Tests/CachedTypeContractTests.cs +++ b/src/Tests/Caching.Tests/CachedTypeContractTests.cs @@ -1,5 +1,4 @@ using System.ComponentModel; -using FSH.Framework.Web.Idempotency; using FSH.Modules.Identity; using FSH.Modules.Multitenancy.Contracts.Dtos; @@ -30,7 +29,6 @@ public static TheoryData CachedTypes typeof(BrandAssetsDto), typeof(TypographyDto), typeof(LayoutDto), - typeof(CachedIdempotentResponse), }; // Reach into the Identity runtime assembly for the internal PermissionSet type. diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs index bba14bff0c..d02f785f62 100644 --- a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -177,6 +177,109 @@ public async Task Filter_Should_ProceedWithoutThrowing_When_RedisReservationFaul result.ShouldNotBeNull("the request must complete normally, not throw out of the filter"); } + // ─── MEDIUM: replay must carry the headers the IResult set (Location on 201) ───────── + + [Fact] + public async Task Replay_Should_PreserveLocationHeader_When_FirstResponseWasCreated() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var id = Guid.NewGuid(); + var location = $"/samples/{id}"; + + var first = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(first), + _ => ValueTask.FromResult(TypedResults.Created(location, new SampleDto(id, "widget")))); + + first.Response.Headers.Location.ToString().ShouldBe( + location, + "sanity: executing Created(uri, value) is what sets Location, so the first call must have it"); + + var second = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => throw new InvalidOperationException("handler must NOT run on an idempotent replay")); + + second.Response.Headers.Location.ToString().ShouldBe( + location, + "a replayed 201 without Location breaks any client that follows the header — and only under " + + "the retry conditions nobody tests. The captured response must carry the meaningful headers."); + } + + // ─── HIGH: the stored response must outlive the request that produced it ───────────── + + [Fact] + public async Task FirstCall_Should_StillCacheResponse_When_ClientDisconnectsAfterHandlerRan() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var id = Guid.NewGuid(); + + using var aborted = new CancellationTokenSource(); + await aborted.CancelAsync(); + var first = NewContext(provider, new MemoryStream()); + first.RequestAborted = aborted.Token; + + try + { + await filter.InvokeAsync( + new TestFilterContext(first), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(id, "widget")))); + } + catch (OperationCanceledException) + { + // Writing to a socket the client already closed is allowed to fail — the handler's side + // effect has committed by then, so the stored response must survive it regardless. + } + + var replayBody = new MemoryStream(); + var second = NewContext(provider, replayBody); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => throw new InvalidOperationException( + "handler must NOT re-run: client-timeout-then-retry is the exact duplicate this feature defends against")); + + second.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeTrue( + "the store must not be tied to the client's connection: if it is, a client that times out and " + + "retries re-executes the handler — the single most common way a duplicate is generated."); + + using var doc = JsonDocument.Parse(replayBody.ToArray()); + doc.RootElement.GetProperty("id").GetGuid().ShouldBe( + id, + "capturing under RequestAborted is worse than not caching: WriteAsJsonAsync swallows the " + + "cancellation, so an EMPTY body gets stored and replayed as a 200 for the full TTL."); + } + + // ─── note: a failure response must not lock the key out for the full 24h TTL ───────── + + [Fact] + public async Task Filter_Should_NotCacheResponse_When_FirstResponseIsNotSuccessful() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + + var first = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(first), + _ => ValueTask.FromResult(TypedResults.Conflict("downstream busy"))); + + int executions = 0; + var second = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget"))); + }); + + executions.ShouldBe( + 1, + "caching a non-2xx locks the caller out of retrying that key for the full response TTL (24h) after " + + "a transient downstream failure. Only a successful response is a record of a committed side effect."); + } + // ─── harness ───────────────────────────────────────────────────── private static ServiceProvider BuildProvider() => BuildProvider(new IdempotencyOptions(), multiplexer: null); From 52365f34a16610b561c5a66fc2b7e64d716d92c9 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 05:20:39 -0300 Subject: [PATCH 05/16] fix(web): scope the idempotency entry to the operation, not the tenant alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The entry was keyed on tenant + caller key, with nothing identifying the operation. One key reused against a second idempotent endpoint replayed the first endpoint's response and the second request silently never ran. Thirty-one endpoints across eight modules share that namespace, and one of them (self-registration) is anonymous: it resolves no tenant claim, so every caller of it lands in the same "global" bucket. This was latent only for as long as replay never engaged — the fix that makes replay work is what would have put it on the wire. The key now folds in the HTTP method and the route pattern. Two smaller things in the same area. The 409 for an in-flight duplicate and the 400 for an over-long key emitted a bare JSON string, where every other error these endpoints produce is RFC 9457 ProblemDetails; both now match. And an unreadable cache entry (written by another version, or another writer at the same key) let JsonException escape as a 500 — that path only became reachable once replay started engaging at all. It now degrades to a miss and logs. ReleaseReservationAsync also swallows cancellation now, not just faults: it runs in a finally after the response body has already gone to the client, so anything thrown there can only reset the connection on a request that succeeded. Tests: a key reused across two route patterns runs the second handler; an unreadable entry runs the handler (with a valid entry seeded at the same key first, so the assertion can't pass as a plain cache miss); a 204 replays without a fabricated content type. Each fails on a mutated implementation. --- .agents/rules/security.md | 2 +- .../Idempotency/IdempotencyEndpointFilter.cs | 71 +++++++++-- .../IdempotencyEndpointFilterReplayTests.cs | 118 ++++++++++++++++++ 3 files changed, 178 insertions(+), 13 deletions(-) diff --git a/.agents/rules/security.md b/.agents/rules/security.md index 90c835c3eb..1fda9d0911 100644 --- a/.agents/rules/security.md +++ b/.agents/rules/security.md @@ -17,7 +17,7 @@ Chained partitioned fixed-window limiter: **tenant → user → IP** (defaults 1 ## Idempotency (`Web/Idempotency/`) -Opt-in per endpoint with **`.WithIdempotency()`**. Reads the `Idempotency-Key` header (max 128 chars, `DefaultTtl` 24h); replays return the cached status, body and the allow-listed headers (`Location`, `ETag`) plus `Idempotency-Replayed: true`. Cache key is tenant-scoped (`CacheKeys.IdempotencyEntry`) and probe and write share one `IDistributedCache` + key + serializer — asymmetry there makes replay silently never engage. Only **2xx** is stored: a failure isn't a record of a committed side effect, and caching it would lock the key out for the full TTL. The store happens **before** the body reaches the client and on `CancellationToken.None`, so a client that times out and retries replays instead of re-executing. Concurrent duplicates are serialized by an in-flight reservation (Redis `SET NX`, else in-process) on the short `ReservationTtl` (default 1m, must outlast the slowest handler); a duplicate still in flight gets **409**. Reserve and release both fail open. Put it on POSTs that must be replay-safe (e.g. CreateTenant). +Opt-in per endpoint with **`.WithIdempotency()`**. Reads the `Idempotency-Key` header (max 128 chars, `DefaultTtl` 24h); replays return the cached status, body and the allow-listed headers (`Location`, `ETag`) plus `Idempotency-Replayed: true`. Cache key is scoped to tenant **and operation** (method + route pattern) via `CacheKeys.IdempotencyEntry` — tenant alone lets one key reused on a second idempotent endpoint replay the first one's response, and anonymous endpoints (self-registration) all share the `"global"` tenant. Probe and write share one `IDistributedCache` + key + serializer — asymmetry there makes replay silently never engage. Only **2xx** is stored: a failure isn't a record of a committed side effect, and caching it would lock the key out for the full TTL. The store happens **before** the body reaches the client and on `CancellationToken.None`, so a client that times out and retries replays instead of re-executing. Concurrent duplicates are serialized by an in-flight reservation (Redis `SET NX`, else in-process) on the short `ReservationTtl` (default 1m, must outlast the slowest handler); a duplicate still in flight gets **409**. Reserve and release both fail open. Put it on POSTs that must be replay-safe (e.g. CreateTenant). ## Quota enforcement (`Quota/`) diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index ea51648585..d3d4e02696 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -5,6 +5,7 @@ using FSH.Framework.Caching; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.Caching.Distributed; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; @@ -65,7 +66,14 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter if (idempotencyKey.Length > options.MaxKeyLength) { - return TypedResults.BadRequest($"Idempotency key exceeds maximum length of {options.MaxKeyLength}."); + // ProblemDetails, not a bare JSON string: every other error these endpoints can produce + // goes out as RFC 9457 through the global handler, and a client parsing that shape chokes + // on a naked string. + return TypedResults.Problem( + detail: $"Idempotency key exceeds maximum length of {options.MaxKeyLength}.", + instance: httpContext.Request.Path, + statusCode: StatusCodes.Status400BadRequest, + title: "Invalid Idempotency-Key"); } var distributedCache = httpContext.RequestServices.GetRequiredService(); @@ -73,11 +81,18 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // Include tenant context in cache key for isolation var tenantId = httpContext.User.FindFirst("tenant")?.Value ?? "global"; - var cacheKey = CacheKeys.IdempotencyEntry(tenantId, idempotencyKey); + + // Scope the entry to the operation as well as the tenant. Keyed on the tenant alone, one key + // reused across two idempotent endpoints replays the first endpoint's response on the second + // — the request silently never runs. That was harmless only while replay never engaged; it + // does now. Anonymous endpoints (self-registration) resolve no tenant claim and share the + // "global" bucket, so scoping by operation is what keeps them apart. + var operation = $"{httpContext.Request.Method}:{RouteIdentity(httpContext)}"; + var cacheKey = CacheKeys.IdempotencyEntry(tenantId, $"{operation}:{idempotencyKey}"); // Probe-only read via IDistributedCache (real GetAsync, null on miss — unlike HybridCache's // factory). Bypasses L1: replays are rare vs first-calls, so L1 warmth has little value. - var cached = await ProbeAsync(distributedCache, cacheKey, httpContext.RequestAborted).ConfigureAwait(false); + var cached = await ProbeAsync(distributedCache, cacheKey, logger, idempotencyKey, httpContext.RequestAborted).ConfigureAwait(false); if (cached is not null) { return await ReplayAsync(httpContext, cached, idempotencyKey, logger).ConfigureAwait(false); @@ -90,10 +105,14 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter { // Another request with this key is in flight. It may have finished between the probe // and the reservation — re-probe once, otherwise report the in-progress conflict. - var raced = await ProbeAsync(distributedCache, cacheKey, httpContext.RequestAborted).ConfigureAwait(false); + var raced = await ProbeAsync(distributedCache, cacheKey, logger, idempotencyKey, httpContext.RequestAborted).ConfigureAwait(false); return raced is not null ? await ReplayAsync(httpContext, raced, idempotencyKey, logger).ConfigureAwait(false) - : TypedResults.Conflict("A request with this Idempotency-Key is already being processed."); + : TypedResults.Problem( + detail: "A request with this Idempotency-Key is already being processed. Retry shortly.", + instance: httpContext.Request.Path, + statusCode: StatusCodes.Status409Conflict, + title: "Idempotent request in progress"); } try @@ -171,12 +190,26 @@ await distributedCache.SetAsync( } private static async ValueTask ProbeAsync( - IDistributedCache cache, string cacheKey, CancellationToken ct) + IDistributedCache cache, string cacheKey, ILogger logger, string idempotencyKey, CancellationToken ct) { var bytes = await cache.GetAsync(cacheKey, ct).ConfigureAwait(false); - return bytes is { Length: > 0 } - ? JsonSerializer.Deserialize(bytes, JsonOpts) - : null; + if (bytes is not { Length: > 0 }) + { + return null; + } + + try + { + return JsonSerializer.Deserialize(bytes, JsonOpts); + } + // An entry that can't be read is a miss, not a 500. This path only became reachable once + // replay started engaging at all, and a shared cache can hold an entry written by another + // version or another writer at the same key — re-running the handler beats failing the request. + catch (JsonException ex) + { + logger.LogWarning(ex, "Discarding unreadable idempotency entry for key {KeyHash}", HashKey(idempotencyKey)); + return null; + } } private static async ValueTask ReplayAsync( @@ -254,7 +287,9 @@ private static async Task ExecuteAndCaptureAsync( return new CachedIdempotentResponse { StatusCode = statusCode, - ContentType = httpContext.Response.ContentType ?? "application/json", + // Left null when the result set none (204, an empty body): fabricating + // "application/json" there would replay a content type for a response with no content. + ContentType = httpContext.Response.ContentType, Body = buffer.ToArray(), Headers = headers, }; @@ -297,8 +332,14 @@ private static async ValueTask ReleaseReservationAsync(IConnectionMultiplexer? m { await multiplexer.GetDatabase().KeyDeleteAsync(reservationKey).ConfigureAwait(false); } - // Best-effort release: a Redis fault here must not throw out of the finally. The short - // ReservationTtl expires the key anyway, so a missed delete self-heals in seconds. + // Cancellation is swallowed too, not just faults: nothing here may throw out of the + // finally, because by this point the response body has already gone to the client and an + // exception can only reset the connection on a request that actually succeeded. The short + // ReservationTtl expires a missed delete on its own. + catch (OperationCanceledException) + { + // Shutdown or a cancelled Redis call — the reservation expires with its TTL. + } catch (Exception ex) when (ex is not OperationCanceledException) { logger.LogWarning(ex, "Failed to release idempotency reservation for key {KeyHash}", HashKey(idempotencyKey)); @@ -310,6 +351,12 @@ private static async ValueTask ReleaseReservationAsync(IConnectionMultiplexer? m InFlight.TryRemove(reservationKey, out _); } + // The route pattern, not the resolved path: two requests to the same endpoint with different + // route values are different operations and their idempotency keys are already distinct, while + // the pattern keeps the entry stable for the same operation. + private static string RouteIdentity(HttpContext httpContext) => + (httpContext.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText ?? httpContext.Request.Path.ToString(); + private static string HashKey(string key) { var hash = SHA256.HashData(Encoding.UTF8.GetBytes(key)); diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs index d02f785f62..e568e8299b 100644 --- a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -1,6 +1,9 @@ using System.Text.Json; +using FSH.Framework.Caching; using FSH.Framework.Web.Idempotency; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; +using Microsoft.AspNetCore.Routing.Patterns; using Microsoft.Extensions.Caching.Distributed; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; @@ -19,6 +22,9 @@ public sealed class IdempotencyEndpointFilterReplayTests { private const string Key = "fixed-idempotency-key"; + // Mirrors the serializer the filter stores entries with. + private static readonly JsonSerializerOptions CacheJsonOpts = new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + // ─── API-01: replayed status ───────────────────────────────────── [Fact] @@ -251,6 +257,32 @@ await filter.InvokeAsync( "cancellation, so an EMPTY body gets stored and replayed as a 200 for the full TTL."); } + // ─── a bodiless success must not gain a content type it never had ─────────────────── + + [Fact] + public async Task Replay_Should_NotInventContentType_When_FirstResponseWasNoContent() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + + var first = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(first), + _ => ValueTask.FromResult(TypedResults.NoContent())); + + first.Response.ContentType.ShouldBeNull("sanity: a 204 carries no content type"); + + var second = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => throw new InvalidOperationException("handler must NOT run on an idempotent replay")); + + second.Response.StatusCode.ShouldBe(StatusCodes.Status204NoContent); + second.Response.ContentType.ShouldBeNull( + "defaulting the captured content type to application/json replays a 204 that advertises a JSON " + + "body it does not have."); + } + // ─── note: a failure response must not lock the key out for the full 24h TTL ───────── [Fact] @@ -280,6 +312,85 @@ await filter.InvokeAsync( "a transient downstream failure. Only a successful response is a record of a committed side effect."); } + // ─── an unreadable entry is a miss, not a 500 ─────────────────────────────────────── + + [Fact] + public async Task Filter_Should_RunHandler_When_CachedEntryIsUnreadable() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var cache = provider.GetRequiredService(); + + // The key the filter reads: tenant + operation + caller key. Proven below by seeding a VALID + // entry at it first — otherwise a wrong key here would make the real assertion pass as a + // plain cache miss and the test would assert nothing. + var storedKey = CacheKeys.IdempotencyEntry("global", $"POST::{Key}"); + await cache.SetAsync( + storedKey, + JsonSerializer.SerializeToUtf8Bytes( + new CachedIdempotentResponse { StatusCode = StatusCodes.Status200OK, Body = "{}"u8.ToArray() }, + CacheJsonOpts), + new DistributedCacheEntryOptions()); + + var seeded = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(seeded), + _ => throw new InvalidOperationException("sanity: a valid entry at this key must replay")); + seeded.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeTrue( + "sanity: this is the key the filter probes"); + + await cache.SetAsync(storedKey, "{ this is not the cached shape"u8.ToArray(), new DistributedCacheEntryOptions()); + + int executions = 0; + var context = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(context), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget"))); + }); + + executions.ShouldBe( + 1, + "an entry written by another version or another writer at the same key must degrade to a cache " + + "miss; letting JsonException escape turns a shared-cache accident into a 500 on every retry."); + } + + // ─── one key reused across two endpoints must not replay the other's response ──────── + + [Fact] + public async Task Filter_Should_NotReplayAcrossEndpoints_When_SameKeyIsReused() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var ticketId = Guid.NewGuid(); + + var onTickets = NewContext(provider, new MemoryStream()); + onTickets.SetEndpoint(RouteEndpointFor("api/v1/tickets")); + await filter.InvokeAsync( + new TestFilterContext(onTickets), + _ => ValueTask.FromResult(TypedResults.Created($"/tickets/{ticketId}", new SampleDto(ticketId, "ticket")))); + + int executions = 0; + var onBrands = NewContext(provider, new MemoryStream()); + onBrands.SetEndpoint(RouteEndpointFor("api/v1/catalog/brands")); + await filter.InvokeAsync( + new TestFilterContext(onBrands), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Created("/brands/1", new SampleDto(Guid.NewGuid(), "brand"))); + }); + + executions.ShouldBe( + 1, + "keyed on tenant + key alone, a key reused against a second idempotent endpoint replays the " + + "first endpoint's response and the second request silently never runs. 31 endpoints in this " + + "repo share that namespace, and one of them is anonymous (self-registration, no tenant claim)."); + onBrands.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeFalse(); + } + // ─── harness ───────────────────────────────────────────────────── private static ServiceProvider BuildProvider() => BuildProvider(new IdempotencyOptions(), multiplexer: null); @@ -305,6 +416,13 @@ private static IConnectionMultiplexer RedisMultiplexer(IDatabase db) return mux; } + private static RouteEndpoint RouteEndpointFor(string pattern) => new( + _ => Task.CompletedTask, + RoutePatternFactory.Parse(pattern), + order: 0, + new EndpointMetadataCollection(), + displayName: pattern); + private static DefaultHttpContext NewContext(IServiceProvider provider, Stream? responseBody = null) { var context = new DefaultHttpContext { RequestServices = provider }; From 53ef779ba31c21ab55043ac1c79c6e4c48aec63f Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:10:29 -0300 Subject: [PATCH 06/16] fix(web): close the idempotency reservation's races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reservation guarded the handler against concurrent duplicates but four holes let one through anyway, or locked a caller out of a key: - The entry was keyed on the caller's `tenant` claim. A root operator scoping requests to different tenants shares one "root" bucket, so one key reused across two targets replays the first tenant's body to the second. Key off the resolved tenant context instead — the one BaseDbContext scopes the side effect to — with the claim as the fallback for a JWT-only request (Finbuckle's claim strategy runs pre-authentication and resolves nothing for those). The raw `tenant` header is deliberately not a fallback: an unresolved header is one Finbuckle refused, and an unvalidated value has no business in a shared key. - The cache was probed once, before the reservation. The original request can store its response and release the lock inside that window; the duplicate then takes the free lock and executes the handler again. Probe once more with the lock held. - The lock was a `:inflight` suffix on the entry key, so a caller key ending in that suffix put its 24h entry exactly where another key's lock goes — every later request with that key 409s for the full response TTL. Give the lock its own prefix. - Release was an unconditional delete. A request that failed open on a Redis blip, or one whose reservation had already expired, freed a lock another request was holding. Release via compare-and-delete against the token the reservation was taken with; failing open carries no token and deletes nothing. The in-process fallback also gains the TTL takeover the Redis branch gets for free: without it a handler that never returns strands the key until the process restarts and every retry 409s forever. Each fix is pinned by a test that was verified to fail when the fix is reverted. --- .agents/rules/security.md | 2 +- .../Idempotency/IdempotencyEndpointFilter.cs | 138 +++++- .../IdempotencyEndpointFilterReplayTests.cs | 461 +++++++++++++++++- 3 files changed, 581 insertions(+), 20 deletions(-) diff --git a/.agents/rules/security.md b/.agents/rules/security.md index 1fda9d0911..4a0ebcadba 100644 --- a/.agents/rules/security.md +++ b/.agents/rules/security.md @@ -17,7 +17,7 @@ Chained partitioned fixed-window limiter: **tenant → user → IP** (defaults 1 ## Idempotency (`Web/Idempotency/`) -Opt-in per endpoint with **`.WithIdempotency()`**. Reads the `Idempotency-Key` header (max 128 chars, `DefaultTtl` 24h); replays return the cached status, body and the allow-listed headers (`Location`, `ETag`) plus `Idempotency-Replayed: true`. Cache key is scoped to tenant **and operation** (method + route pattern) via `CacheKeys.IdempotencyEntry` — tenant alone lets one key reused on a second idempotent endpoint replay the first one's response, and anonymous endpoints (self-registration) all share the `"global"` tenant. Probe and write share one `IDistributedCache` + key + serializer — asymmetry there makes replay silently never engage. Only **2xx** is stored: a failure isn't a record of a committed side effect, and caching it would lock the key out for the full TTL. The store happens **before** the body reaches the client and on `CancellationToken.None`, so a client that times out and retries replays instead of re-executing. Concurrent duplicates are serialized by an in-flight reservation (Redis `SET NX`, else in-process) on the short `ReservationTtl` (default 1m, must outlast the slowest handler); a duplicate still in flight gets **409**. Reserve and release both fail open. Put it on POSTs that must be replay-safe (e.g. CreateTenant). +Opt-in per endpoint with **`.WithIdempotency()`**. Reads the `Idempotency-Key` header (max 128 chars, `DefaultTtl` 24h); replays return the cached status, body and the allow-listed headers (`Location`, `ETag`) plus `Idempotency-Replayed: true`. Cache key is scoped to tenant **and operation** (method + route pattern) via `CacheKeys.IdempotencyEntry` — tenant alone lets one key reused on a second idempotent endpoint replay the first one's response, and anonymous endpoints (self-registration) all share the `"global"` tenant. Tenant comes from the **resolved** `IMultiTenantContextAccessor` (what the side effect is scoped to, incl. a root operator's target tenant), claim as fallback, `"global"` otherwise — never the raw `tenant` header, which is unvalidated when Finbuckle didn't resolve it. Probe and write share one `IDistributedCache` + key + serializer — asymmetry there makes replay silently never engage. Only **2xx** is stored: a failure isn't a record of a committed side effect, and caching it would lock the key out for the full TTL. The store happens **before** the body reaches the client and on `CancellationToken.None`, so a client that times out and retries replays instead of re-executing. Concurrent duplicates are serialized by an in-flight reservation (Redis `SET NX`, else in-process) under a `lock:` prefix — never a suffix on the entry key, which a caller key ending in it would collide with — on the short `ReservationTtl` (default 1m, must outlast the slowest handler; both branches expire on it). The cache is re-probed **after** the lock is taken (the original can settle in the probe→reserve window), release is a compare-and-delete on the reservation's own token, and a duplicate still in flight gets **409**. Reserve and release both fail open; a request that failed open holds no token and releases nothing. Put it on POSTs that must be replay-safe (e.g. CreateTenant). ## Quota enforcement (`Quota/`) diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index d3d4e02696..302c55e40b 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -2,7 +2,10 @@ using System.Security.Cryptography; using System.Text; using System.Text.Json; +using Finbuckle.MultiTenant.Abstractions; using FSH.Framework.Caching; +using FSH.Framework.Shared.Constants; +using FSH.Framework.Shared.Multitenancy; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; @@ -44,10 +47,16 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // replaying a stale value there corrupts the response. private static readonly string[] ReplayableHeaders = ["Location", "ETag"]; + // Compare-and-delete: a reservation is released only by the request that took it. An + // unconditional delete lets a request that failed open, or one whose reservation already expired, + // free a lock another request is still holding — and then a third request runs the handler too. + private const string ReleaseIfOwnedScript = + "if redis.call('get', KEYS[1]) == ARGV[1] then return redis.call('del', KEYS[1]) else return 0 end"; + // In-process reservation used when no Redis multiplexer is registered. Single-instance only — // a multi-instance host in this stack already runs Redis (shared Data Protection key ring), so // the Redis branch below covers every deployment where cross-instance duplicates are possible. - private static readonly ConcurrentDictionary InFlight = new(StringComparer.Ordinal); + private static readonly ConcurrentDictionary InFlight = new(StringComparer.Ordinal); public async ValueTask InvokeAsync(EndpointFilterInvocationContext context, EndpointFilterDelegate next) { @@ -79,8 +88,7 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter var distributedCache = httpContext.RequestServices.GetRequiredService(); var logger = httpContext.RequestServices.GetRequiredService>(); - // Include tenant context in cache key for isolation - var tenantId = httpContext.User.FindFirst("tenant")?.Value ?? "global"; + var tenantId = ResolveTenant(httpContext); // Scope the entry to the operation as well as the tenant. Keyed on the tenant alone, one key // reused across two idempotent endpoints replays the first endpoint's response on the second @@ -98,10 +106,16 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter return await ReplayAsync(httpContext, cached, idempotencyKey, logger).ConfigureAwait(false); } - // Atomically reserve the key so concurrent duplicates don't both execute the handler. + // Atomically reserve the key so concurrent duplicates don't both execute the handler. The + // lock lives under its own prefix rather than a suffix on the entry key: a caller-supplied + // key ending in the suffix would otherwise land the lock exactly on another entry's key. + // ponytail: the reservation is not renewed while the handler runs, so a handler slower than + // ReservationTtl lets a duplicate through (the entry is not stored yet either, so the probe + // can't catch it). Add lease renewal if an idempotent endpoint ever runs longer than that. var multiplexer = httpContext.RequestServices.GetService(); - var reservationKey = cacheKey + ":inflight"; - if (!await TryReserveAsync(multiplexer, reservationKey, options.ReservationTtl, logger, idempotencyKey).ConfigureAwait(false)) + var reservationKey = "lock:" + cacheKey; + var reservation = await TryReserveAsync(multiplexer, reservationKey, options.ReservationTtl, logger, idempotencyKey).ConfigureAwait(false); + if (reservation.Denied) { // Another request with this key is in flight. It may have finished between the probe // and the reservation — re-probe once, otherwise report the in-progress conflict. @@ -117,6 +131,15 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter try { + // Probe again now that the key is held. The first probe and the reservation are two + // steps, and the original request can store its response and release in between — the + // duplicate would then take the freed lock and run the handler a second time. + var settled = await ProbeAsync(distributedCache, cacheKey, logger, idempotencyKey, httpContext.RequestAborted).ConfigureAwait(false); + if (settled is not null) + { + return await ReplayAsync(httpContext, settled, idempotencyKey, logger).ConfigureAwait(false); + } + var result = await next(context).ConfigureAwait(false); // Execute the result into a buffer to capture the real wire body + status code, then @@ -152,7 +175,7 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter } finally { - await ReleaseReservationAsync(multiplexer, reservationKey, logger, idempotencyKey).ConfigureAwait(false); + await ReleaseReservationAsync(multiplexer, reservationKey, reservation, logger, idempotencyKey).ConfigureAwait(false); } } @@ -301,36 +324,80 @@ private static async Task ExecuteAndCaptureAsync( } } - private static async ValueTask TryReserveAsync( + private static async ValueTask TryReserveAsync( IConnectionMultiplexer? multiplexer, string reservationKey, TimeSpan ttl, ILogger logger, string idempotencyKey) { + var token = Guid.NewGuid().ToString("N"); + if (multiplexer is not null) { try { var db = multiplexer.GetDatabase(); - return await db.StringSetAsync(reservationKey, "1", ttl, When.NotExists).ConfigureAwait(false); + return await db.StringSetAsync(reservationKey, token, ttl, When.NotExists).ConfigureAwait(false) + ? Reservation.Held(token) + : Reservation.Refused; } // Fail open on a Redis blip: the reservation is a concurrency convenience, not a correctness // requirement (the response cache still dedups later retries). Proceed rather than 500 the - // request, matching the best-effort stance the response write already takes. + // request, matching the best-effort stance the response write already takes — but proceed + // WITHOUT ownership, so the release can't delete a lock another request is holding. catch (Exception ex) when (ex is not OperationCanceledException) { logger.LogWarning(ex, "Idempotency reservation failed for key {KeyHash}; proceeding without it", HashKey(idempotencyKey)); - return true; + return Reservation.Unowned; } } - return InFlight.TryAdd(reservationKey, 0); + return TryReserveInProcess(reservationKey, token, ttl); + } + + // Mirrors the Redis branch: take the key if free, take it over if the holder's reservation has + // outlived the TTL. Without the takeover a handler that never returns strands the key until the + // process restarts, and every retry of it 409s forever — the Redis branch self-heals on expiry. + private static Reservation TryReserveInProcess(string reservationKey, string token, TimeSpan ttl) + { + var ttlMs = (long)ttl.TotalMilliseconds; + while (true) + { + var entry = new InFlightEntry(token, Environment.TickCount64); + if (InFlight.TryAdd(reservationKey, entry)) + { + return Reservation.Held(token); + } + + if (!InFlight.TryGetValue(reservationKey, out var holder)) + { + continue; + } + + if (Environment.TickCount64 - holder.StartedAtMs < ttlMs) + { + return Reservation.Refused; + } + + if (InFlight.TryUpdate(reservationKey, entry, holder)) + { + return Reservation.Held(token); + } + } } - private static async ValueTask ReleaseReservationAsync(IConnectionMultiplexer? multiplexer, string reservationKey, ILogger logger, string idempotencyKey) + private static async ValueTask ReleaseReservationAsync( + IConnectionMultiplexer? multiplexer, string reservationKey, Reservation reservation, ILogger logger, string idempotencyKey) { + if (reservation.Token is not { } token) + { + return; + } + if (multiplexer is not null) { try { - await multiplexer.GetDatabase().KeyDeleteAsync(reservationKey).ConfigureAwait(false); + await multiplexer.GetDatabase() + .ScriptEvaluateAsync(ReleaseIfOwnedScript, [reservationKey], [token]) + .ConfigureAwait(false); } // Cancellation is swallowed too, not just faults: nothing here may throw out of the // finally, because by this point the response body has already gone to the client and an @@ -348,7 +415,32 @@ private static async ValueTask ReleaseReservationAsync(IConnectionMultiplexer? m return; } - InFlight.TryRemove(reservationKey, out _); + if (InFlight.TryGetValue(reservationKey, out var holder) && holder.Token == token) + { + InFlight.TryRemove(new KeyValuePair(reservationKey, holder)); + } + } + + // Tenant scope for the cache key: the resolved tenant context first, the claim only as a + // fallback. The resolved context is the tenant the handler's side effect actually lands in + // (BaseDbContext scopes its query filters off the same accessor), including the case where a root + // operator scopes one request to another tenant — keyed on the claim alone, every tenant a root + // operator touches would share one "root" bucket and a reused key would replay one tenant's + // response body to another. The claim covers requests that carry a JWT but no tenant header: + // Finbuckle's claim strategy runs before authentication, so it resolves nothing for them. + // The raw header is deliberately NOT a fallback — an unresolved header is one Finbuckle refused + // (no such tenant), and an unvalidated caller-supplied value has no business in a shared key. + private static string ResolveTenant(HttpContext httpContext) + { + var resolved = httpContext.RequestServices + .GetService>()?.MultiTenantContext?.TenantInfo?.Id; + if (!string.IsNullOrWhiteSpace(resolved)) + { + return resolved; + } + + var fromClaim = httpContext.User.FindFirst(ClaimConstants.Tenant)?.Value; + return string.IsNullOrWhiteSpace(fromClaim) ? "global" : fromClaim; } // The route pattern, not the resolved path: two requests to the same endpoint with different @@ -362,6 +454,22 @@ private static string HashKey(string key) var hash = SHA256.HashData(Encoding.UTF8.GetBytes(key)); return Convert.ToHexString(hash.AsSpan(0, 8)); } + + /// + /// Outcome of an in-flight reservation attempt. Token is the proof of ownership: it is + /// null when the reservation was refused (a duplicate is running) and also when the store failed + /// and we proceeded without one, so neither case releases a lock it does not hold. + /// + private readonly record struct Reservation(bool Denied, string? Token) + { + public static Reservation Refused => new(Denied: true, Token: null); + + public static Reservation Unowned => new(Denied: false, Token: null); + + public static Reservation Held(string token) => new(Denied: false, token); + } + + private readonly record struct InFlightEntry(string Token, long StartedAtMs); } public static class IdempotencyEndpointExtensions diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs index e568e8299b..cc0c4d02ac 100644 --- a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -1,10 +1,16 @@ +using System.Security.Claims; using System.Text.Json; +using Finbuckle.MultiTenant; +using Finbuckle.MultiTenant.Abstractions; using FSH.Framework.Caching; +using FSH.Framework.Shared.Constants; +using FSH.Framework.Shared.Multitenancy; using FSH.Framework.Web.Idempotency; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Patterns; using Microsoft.Extensions.Caching.Distributed; +using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using NSubstitute; @@ -20,7 +26,10 @@ namespace Framework.Tests.Web; /// public sealed class IdempotencyEndpointFilterReplayTests { - private const string Key = "fixed-idempotency-key"; + // Per-instance, not a shared const: the filter's in-process reservation set is static and + // process-wide, so a key shared across tests would surface a leak in one as a phantom 409 in + // another — and xUnit runs test classes in parallel. + private readonly string Key = Guid.NewGuid().ToString("N"); // Mirrors the serializer the filter stores entries with. private static readonly JsonSerializerOptions CacheJsonOpts = new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; @@ -218,7 +227,10 @@ await filter.InvokeAsync( [Fact] public async Task FirstCall_Should_StillCacheResponse_When_ClientDisconnectsAfterHandlerRan() { - var provider = BuildProvider(); + // TokenSensitiveCache, not the plain in-memory one: MemoryDistributedCache ignores the token, + // so against it this test passes whether the store uses CancellationToken.None or the + // cancelled RequestAborted — it would assert nothing about the fix it exists to pin. + var provider = BuildProviderWith(new TokenSensitiveCache(NewMemoryCache())); var filter = new IdempotencyEndpointFilter(); var id = Guid.NewGuid(); @@ -391,6 +403,227 @@ await filter.InvokeAsync( onBrands.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeFalse(); } + // ─── the entry is scoped to the tenant the side effect lands in, not the caller's claim ── + + [Fact] + public async Task Filter_Should_ScopeEntryToResolvedTenant_Not_CallerClaim() + { + var filter = new IdempotencyEndpointFilter(); + var cache = NewMemoryCache(); + + // A root operator scoping one request to tenant "acme" via header: the claim stays "root", + // Finbuckle resolves the target, and the handler writes into acme's data. + var onAcme = NewContext(BuildProviderWith(cache, TenantContext("acme")), new MemoryStream()); + onAcme.User = TenantPrincipal("root"); + await filter.InvokeAsync( + new TestFilterContext(onAcme), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "acme-order")))); + + // Same operator, same key, now scoped to a different tenant. + int executions = 0; + var onGlobex = NewContext(BuildProviderWith(cache, TenantContext("globex")), new MemoryStream()); + onGlobex.User = TenantPrincipal("root"); + await filter.InvokeAsync( + new TestFilterContext(onGlobex), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "globex-order"))); + }); + + executions.ShouldBe( + 1, + "keyed on the claim, every tenant a root operator touches shares one \"root\" bucket: the second " + + "request replays acme's response body to globex and never runs. The entry must follow the resolved " + + "tenant, which is the one BaseDbContext scopes the side effect to."); + onGlobex.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeFalse(); + } + + [Fact] + public async Task Filter_Should_IgnoreUnresolvedTenantHeader_When_BuildingTheKey() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + + // No claim and no resolved tenant context: the header names a tenant Finbuckle refused (no + // such tenant), so it is caller-supplied and unvalidated. + var context = NewContext(provider, new MemoryStream()); + context.Request.Headers[MultitenancyConstants.Identifier] = "acme"; + await filter.InvokeAsync( + new TestFilterContext(context), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget")))); + + var cache = provider.GetRequiredService(); + (await cache.GetAsync(CacheKeys.IdempotencyEntry("global", $"POST::{Key}"))).ShouldNotBeNull( + "an unvalidated header must not choose the bucket: taking it would let any caller write into — " + + "and replay out of — a real tenant's idempotency namespace."); + (await cache.GetAsync(CacheKeys.IdempotencyEntry("acme", $"POST::{Key}"))).ShouldBeNull(); + } + + // ─── the reservation must be re-probed: the original can settle between probe and reserve ─── + + [Fact] + public async Task Filter_Should_Replay_When_OriginalSettledBetweenProbeAndReservation() + { + var inner = NewMemoryCache(); + var id = Guid.NewGuid(); + + // Simulates the original request finishing in the window between the first probe (a miss) and + // the reservation: the entry appears, and the lock it held is already released. + var racing = new SeedAfterFirstMissCache( + inner, + async key => await inner.SetAsync( + key, + JsonSerializer.SerializeToUtf8Bytes( + new CachedIdempotentResponse + { + StatusCode = StatusCodes.Status200OK, + ContentType = "application/json", + Body = JsonSerializer.SerializeToUtf8Bytes(new SampleDto(id, "original"), CacheJsonOpts), + }, + CacheJsonOpts), + new DistributedCacheEntryOptions()).ConfigureAwait(false)); + + var filter = new IdempotencyEndpointFilter(); + int executions = 0; + var context = NewContext(BuildProviderWith(racing), new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(context), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "duplicate"))); + }); + + executions.ShouldBe( + 0, + "probing once before the reservation leaves a window: the original stores its response and " + + "releases the lock in it, the duplicate then takes the free lock and executes the handler a " + + "second time. The probe has to be repeated once the key is held."); + context.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeTrue(); + } + + // ─── one caller's stored entry must not land on another key's lock ────────────────── + + [Fact] + public async Task Filter_Should_NotBlockKey_When_AnotherKeysEntryLandsWhereItsLockWouldGo() + { + // Entries and locks share one Redis keyspace in production (IDistributedCache writes the entry + // under its raw key — RedisCacheOptions.InstanceName is empty by default — and the reservation + // is a StringSet on the same connection), so the fake shares one dictionary between the two. + var keyspace = new SharedKeyspace(); + var provider = BuildProviderWith(new KeyspaceCache(keyspace), KeyspaceMultiplexer(keyspace)); + var filter = new IdempotencyEndpointFilter(); + + // A completed request under the caller-supplied key ":inflight" leaves a stored entry + // behind. Under the suffix scheme that entry sits exactly where the lock for "" goes. + await filter.InvokeAsync( + new TestFilterContext(NewContextForKey(provider, $"{Key}:inflight")), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "poisoner")))); + + int executions = 0; + var victim = NewContextForKey(provider, Key); + var result = await filter.InvokeAsync( + new TestFilterContext(victim), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "victim"))); + }); + + executions.ShouldBe( + 1, + "with the lock stored as \":inflight\", one request with a key ending in \":inflight\" " + + "parks a 24h entry on another key's lock: every later request with that key sees the reservation " + + "taken and 409s for the whole response TTL. The lock needs its own prefix."); + (result as IStatusCodeHttpResult)?.StatusCode.ShouldNotBe(StatusCodes.Status409Conflict); + } + + // ─── a reservation is released only by the request that owns it ────────────────────── + + [Fact] + public async Task Release_Should_UseTheOwnershipToken_When_ReservationWasHeld() + { + var db = Substitute.For(); + RedisValue storedToken = RedisValue.Null; + db.StringSetAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(ci => { storedToken = ci.ArgAt(1); return Task.FromResult(true); }); + db.ScriptEvaluateAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(RedisResult.Create(1))); + var provider = BuildProvider(new IdempotencyOptions(), RedisMultiplexer(db)); + var filter = new IdempotencyEndpointFilter(); + + await filter.InvokeAsync( + new TestFilterContext(NewContext(provider, new MemoryStream())), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget")))); + + await db.Received(1).ScriptEvaluateAsync( + Arg.Any(), + Arg.Any(), + Arg.Is(values => values.Length == 1 && values[0] == storedToken), + Arg.Any()); + await db.DidNotReceive().KeyDeleteAsync(Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task Release_Should_DeleteNothing_When_ReservationFailedOpen() + { + var db = Substitute.For(); + db.StringSetAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromException(new RedisException("reserve blip"))); + var provider = BuildProvider(new IdempotencyOptions(), RedisMultiplexer(db)); + var filter = new IdempotencyEndpointFilter(); + + await filter.InvokeAsync( + new TestFilterContext(NewContext(provider, new MemoryStream())), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget")))); + + await db.DidNotReceive().ScriptEvaluateAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + await db.DidNotReceive().KeyDeleteAsync(Arg.Any(), Arg.Any()); + } + + // ─── the in-process reservation self-heals on TTL, like the Redis one ──────────────── + + [Fact] + public async Task Reservation_Should_BeRetaken_When_TheHolderOutlivesTheReservationTtl() + { + var provider = BuildProvider(new IdempotencyOptions { ReservationTtl = TimeSpan.Zero }, multiplexer: null); + var filter = new IdempotencyEndpointFilter(); + + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var holderCall = filter.InvokeAsync( + new TestFilterContext(NewContext(provider, new MemoryStream())), + async _ => + { + started.SetResult(); + await release.Task.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + return TypedResults.Ok(new SampleDto(Guid.NewGuid(), "holder")); + }).AsTask(); + await started.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + int executions = 0; + var result = await filter.InvokeAsync( + new TestFilterContext(NewContext(provider, new MemoryStream())), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "second"))); + }); + + release.SetResult(); + await holderCall.WaitAsync(TimeSpan.FromSeconds(10)); + + executions.ShouldBe( + 1, + "the in-process set has no expiry of its own: without the TTL takeover a handler that never " + + "returns strands the key until the process restarts and every retry 409s forever, while the " + + "Redis branch self-heals when the reservation expires."); + (result as IStatusCodeHttpResult)?.StatusCode.ShouldNotBe(StatusCodes.Status409Conflict); + } + // ─── harness ───────────────────────────────────────────────────── private static ServiceProvider BuildProvider() => BuildProvider(new IdempotencyOptions(), multiplexer: null); @@ -409,6 +642,70 @@ private static ServiceProvider BuildProvider(IdempotencyOptions options, IConnec return services.BuildServiceProvider(); } + private static ServiceProvider BuildProviderWith(IDistributedCache cache) => + BuildProviderWith(cache, tenantAccessor: null, multiplexer: null); + + private static ServiceProvider BuildProviderWith(IDistributedCache cache, IMultiTenantContextAccessor? tenantAccessor) => + BuildProviderWith(cache, tenantAccessor, multiplexer: null); + + private static ServiceProvider BuildProviderWith(IDistributedCache cache, IConnectionMultiplexer multiplexer) => + BuildProviderWith(cache, tenantAccessor: null, multiplexer); + + private static ServiceProvider BuildProviderWith( + IDistributedCache cache, + IMultiTenantContextAccessor? tenantAccessor, + IConnectionMultiplexer? multiplexer) + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddSingleton(cache); + services.AddSingleton>(Options.Create(new IdempotencyOptions())); + if (tenantAccessor is not null) + { + services.AddSingleton(tenantAccessor); + } + + if (multiplexer is not null) + { + services.AddSingleton(multiplexer); + } + + return services.BuildServiceProvider(); + } + + private static MemoryDistributedCache NewMemoryCache() => + new(Options.Create(new MemoryDistributedCacheOptions())); + + // What Finbuckle leaves behind for the endpoint filter: the tenant the request is scoped to, + // which for a root operator using the tenant header is the target, not the caller's own tenant. + private static IMultiTenantContextAccessor TenantContext(string tenantId) + { + var accessor = Substitute.For>(); + accessor.MultiTenantContext.Returns(new MultiTenantContext(new AppTenantInfo(tenantId, tenantId))); + return accessor; + } + + // A Redis stand-in whose reservation commands hit the same dictionary the entries live in, which + // is how the two sit in a real deployment. + private static IConnectionMultiplexer KeyspaceMultiplexer(SharedKeyspace keyspace) + { + var db = Substitute.For(); + db.StringSetAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(ci => Task.FromResult( + ci.ArgAt(3) == When.NotExists + ? keyspace.TryAdd(ci.ArgAt(0).ToString(), (byte[])ci.ArgAt(1)!) + : keyspace.Set(ci.ArgAt(0).ToString(), (byte[])ci.ArgAt(1)!))); + db.ScriptEvaluateAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(ci => + { + keyspace.RemoveIfValueMatches( + ci.ArgAt(1)[0].ToString(), + (byte[])ci.ArgAt(2)[0]!); + return Task.FromResult(RedisResult.Create(1)); + }); + return RedisMultiplexer(db); + } + private static IConnectionMultiplexer RedisMultiplexer(IDatabase db) { var mux = Substitute.For(); @@ -423,11 +720,17 @@ private static IConnectionMultiplexer RedisMultiplexer(IDatabase db) new EndpointMetadataCollection(), displayName: pattern); - private static DefaultHttpContext NewContext(IServiceProvider provider, Stream? responseBody = null) + private DefaultHttpContext NewContext(IServiceProvider provider, Stream? responseBody = null) => + NewContextForKey(provider, Key, responseBody); + + private static DefaultHttpContext NewContextForKey(IServiceProvider provider, string idempotencyKey) => + NewContextForKey(provider, idempotencyKey, new MemoryStream()); + + private static DefaultHttpContext NewContextForKey(IServiceProvider provider, string idempotencyKey, Stream? responseBody) { var context = new DefaultHttpContext { RequestServices = provider }; context.Request.Method = "POST"; - context.Request.Headers["Idempotency-Key"] = Key; + context.Request.Headers["Idempotency-Key"] = idempotencyKey; if (responseBody is not null) { context.Response.Body = responseBody; @@ -436,8 +739,158 @@ private static DefaultHttpContext NewContext(IServiceProvider provider, Stream? return context; } + private static ClaimsPrincipal TenantPrincipal(string tenantId) => + new(new ClaimsIdentity([new Claim(ClaimConstants.Tenant, tenantId)], "test")); + private sealed record SampleDto(Guid Id, string Name); + /// + /// Wraps the in-memory cache so a test can see the the filter + /// stores with. MemoryDistributedCache ignores it entirely, so without this the difference + /// between CancellationToken.None and a cancelled RequestAborted — the whole point + /// of the store-outlives-the-request fix — is invisible to every assertion. + /// + private sealed class TokenSensitiveCache(IDistributedCache inner) : IDistributedCache + { + public byte[]? Get(string key) => inner.Get(key); + + public Task GetAsync(string key, CancellationToken token = default) => inner.GetAsync(key, token); + + public void Refresh(string key) => inner.Refresh(key); + + public Task RefreshAsync(string key, CancellationToken token = default) => inner.RefreshAsync(key, token); + + public void Remove(string key) => inner.Remove(key); + + public Task RemoveAsync(string key, CancellationToken token = default) => inner.RemoveAsync(key, token); + + public void Set(string key, byte[] value, DistributedCacheEntryOptions options) => inner.Set(key, value, options); + + public Task SetAsync(string key, byte[] value, DistributedCacheEntryOptions options, CancellationToken token = default) + { + // What a real IDistributedCache does: RedisCache.SetImplAsync calls ThrowIfCancellationRequested. + token.ThrowIfCancellationRequested(); + return inner.SetAsync(key, value, options, token); + } + } + + /// + /// One keyspace for both the cached entries and the reservations — what a real Redis is. + /// + private sealed class SharedKeyspace + { + private readonly Dictionary _values = new(StringComparer.Ordinal); + + public bool TryAdd(string key, byte[] value) + { + lock (_values) + { + return _values.TryAdd(key, value); + } + } + + public bool Set(string key, byte[] value) + { + lock (_values) + { + _values[key] = value; + return true; + } + } + + public byte[]? Get(string key) + { + lock (_values) + { + return _values.TryGetValue(key, out var value) ? value : null; + } + } + + public void Remove(string key) + { + lock (_values) + { + _values.Remove(key); + } + } + + public void RemoveIfValueMatches(string key, byte[] expected) + { + lock (_values) + { + if (_values.TryGetValue(key, out var value) && value.AsSpan().SequenceEqual(expected)) + { + _values.Remove(key); + } + } + } + } + + private sealed class KeyspaceCache(SharedKeyspace keyspace) : IDistributedCache + { + public byte[]? Get(string key) => keyspace.Get(key); + + public Task GetAsync(string key, CancellationToken token = default) => Task.FromResult(keyspace.Get(key)); + + public void Refresh(string key) + { + } + + public Task RefreshAsync(string key, CancellationToken token = default) => Task.CompletedTask; + + public void Remove(string key) => keyspace.Remove(key); + + public Task RemoveAsync(string key, CancellationToken token = default) + { + keyspace.Remove(key); + return Task.CompletedTask; + } + + public void Set(string key, byte[] value, DistributedCacheEntryOptions options) => keyspace.Set(key, value); + + public Task SetAsync(string key, byte[] value, DistributedCacheEntryOptions options, CancellationToken token = default) + { + keyspace.Set(key, value); + return Task.CompletedTask; + } + } + + /// + /// Answers the first probe as a miss and then seeds the entry, reproducing the window in which + /// the original request stores its response and releases the lock — the window a single + /// probe-before-reserve cannot see. + /// + private sealed class SeedAfterFirstMissCache(IDistributedCache inner, Func seed) : IDistributedCache + { + private int _probes; + + public byte[]? Get(string key) => inner.Get(key); + + public async Task GetAsync(string key, CancellationToken token = default) + { + var bytes = await inner.GetAsync(key, token).ConfigureAwait(false); + if (Interlocked.Increment(ref _probes) == 1) + { + await seed(key).ConfigureAwait(false); + } + + return bytes; + } + + public void Refresh(string key) => inner.Refresh(key); + + public Task RefreshAsync(string key, CancellationToken token = default) => inner.RefreshAsync(key, token); + + public void Remove(string key) => inner.Remove(key); + + public Task RemoveAsync(string key, CancellationToken token = default) => inner.RemoveAsync(key, token); + + public void Set(string key, byte[] value, DistributedCacheEntryOptions options) => inner.Set(key, value, options); + + public Task SetAsync(string key, byte[] value, DistributedCacheEntryOptions options, CancellationToken token = default) => + inner.SetAsync(key, value, options, token); + } + private sealed class TestFilterContext : EndpointFilterInvocationContext { public TestFilterContext(HttpContext httpContext) => HttpContext = httpContext; From b18e541c2ef0ddc4ba3d03696325f8ba09856302 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:43:21 -0300 Subject: [PATCH 07/16] fix(web): keep an idempotent handler alive past a client disconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up from adversarial passes over the whole filter. Each item below is pinned by a test verified to fail when the fix is reverted. - The handler ran under the client's abort token. A disconnect after the side effect committed cancelled the next await inside the handler (an EF read, an outbox write, a Mediator behaviour), so the filter had nothing to store and the client's retry re-executed the side effect — the duplicate this filter exists to absorb. The handler now runs with the token detached; the trade is that a disconnect no longer aborts an idempotent handler. - The probe was the one link that hard-failed. Reserve and store both degrade to a warning when the cache is down, so a `RedisConnectionException` on the probe took every idempotent endpoint down for exactly the clients that send a key. It now fails open as a miss. - A handler that writes the response itself had already started it, so the buffer swap captured nothing and setting the captured status threw. That case now passes through untouched and stores nothing. - The key covered the route pattern but not its values, so `PUT /tickets/1` and `PUT /tickets/2` were one operation: the second replayed the first ticket's response and never ran. It now folds in the resolved route values. - The key was not scoped to the caller, so two users of one tenant reusing a low-entropy key on the same endpoint received each other's response bodies while their own request was silently suppressed. - The 409 said "retry shortly" with no `Retry-After`. It now sends 1 second: the original is normally about to store its response, and the reservation TTL is the worst case, not the hint. Also: options are validated at startup like every other block here (a zero TTL failed silently inside the best-effort write, so nothing was ever stored), `CacheKeys.Tags.Idempotency` no longer claims to be applied, and the cached headers dictionary documents that its comparer does not survive deserialization. Ceilings that stay: no size cap on the buffered response (do not put `.WithIdempotency()` on a streaming endpoint), no lease renewal, and a lock whose Redis may not be the cache's Redis — all three now carry `ponytail:` notes. --- src/BuildingBlocks/Caching/CacheKeys.cs | 6 +- .../Idempotency/CachedIdempotentResponse.cs | 2 + .../Web/Idempotency/Extensions.cs | 14 +- .../Idempotency/IdempotencyEndpointFilter.cs | 135 +++++++-- .../IdempotencyEndpointFilterReplayTests.cs | 282 +++++++++++++++++- .../Web/OptionsDefaultsTests.cs | 50 ++++ 6 files changed, 464 insertions(+), 25 deletions(-) diff --git a/src/BuildingBlocks/Caching/CacheKeys.cs b/src/BuildingBlocks/Caching/CacheKeys.cs index 20e732e4e1..3350ccdc92 100644 --- a/src/BuildingBlocks/Caching/CacheKeys.cs +++ b/src/BuildingBlocks/Caching/CacheKeys.cs @@ -16,7 +16,11 @@ public static class Tags /// Tag applied to every tenant theme entry. public const string Themes = "themes"; - /// Tag applied to every idempotency replay entry. + /// + /// Reserved for idempotency replay entries — not applied to them today. Those entries live in + /// IDistributedCache, which carries no tags, so a tag purge does not reach them; they + /// expire on their own TTL instead. + /// public const string Idempotency = "idempotency"; /// Per-tenant tag — invalidates all entries scoped to a tenant. diff --git a/src/BuildingBlocks/Web/Idempotency/CachedIdempotentResponse.cs b/src/BuildingBlocks/Web/Idempotency/CachedIdempotentResponse.cs index aa3ead8370..2a61ee2944 100644 --- a/src/BuildingBlocks/Web/Idempotency/CachedIdempotentResponse.cs +++ b/src/BuildingBlocks/Web/Idempotency/CachedIdempotentResponse.cs @@ -16,6 +16,8 @@ public sealed record CachedIdempotentResponse /// are captured (see the filter's allow-list) — the host sets the transport ones itself, and /// replaying a stale Content-Length or Transfer-Encoding would corrupt the response. /// Defaults to empty so entries written before headers were captured still deserialize. + /// Iterate it, do not look a header up by name: deserialization replaces this instance with a + /// plain case-SENSITIVE dictionary, so the initializer's comparer only holds on the write path. /// public IReadOnlyDictionary Headers { get; init; } = new Dictionary(StringComparer.OrdinalIgnoreCase); } diff --git a/src/BuildingBlocks/Web/Idempotency/Extensions.cs b/src/BuildingBlocks/Web/Idempotency/Extensions.cs index 0c92f4bbd2..4e30ddb687 100644 --- a/src/BuildingBlocks/Web/Idempotency/Extensions.cs +++ b/src/BuildingBlocks/Web/Idempotency/Extensions.cs @@ -14,8 +14,20 @@ public static IServiceCollection AddHeroIdempotency(this IServiceCollection serv ArgumentNullException.ThrowIfNull(services); ArgumentNullException.ThrowIfNull(configuration); + // Validated at startup, like every other options block here. A misconfigured TTL fails + // SILENTLY otherwise: a zero or negative DefaultTtl throws inside the best-effort cache + // write, which logs a warning and moves on, so no response is ever stored and replay never + // engages — the exact silent failure this filter exists to have stopped having. A + // ReservationTtl of zero expires the in-flight lock the moment it is taken, so concurrent + // duplicates both run the handler. services.AddOptions() - .BindConfiguration(nameof(IdempotencyOptions)); + .BindConfiguration(nameof(IdempotencyOptions)) + .Validate(o => !string.IsNullOrWhiteSpace(o.HeaderName), "IdempotencyOptions.HeaderName is required.") + .Validate(o => o.DefaultTtl > TimeSpan.Zero, "IdempotencyOptions.DefaultTtl must be greater than zero.") + .Validate(o => o.ReservationTtl > TimeSpan.Zero, "IdempotencyOptions.ReservationTtl must be greater than zero.") + .Validate(o => o.ReservationTtl <= o.DefaultTtl, "IdempotencyOptions.ReservationTtl must not exceed DefaultTtl — the reservation only has to outlast the handler.") + .Validate(o => o.MaxKeyLength > 0, "IdempotencyOptions.MaxKeyLength must be greater than zero.") + .ValidateOnStart(); return services; } diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index 302c55e40b..c8fc7334e1 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -5,6 +5,7 @@ using Finbuckle.MultiTenant.Abstractions; using FSH.Framework.Caching; using FSH.Framework.Shared.Constants; +using FSH.Framework.Shared.Identity.Claims; using FSH.Framework.Shared.Multitenancy; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; @@ -35,6 +36,8 @@ namespace FSH.Framework.Web.Idempotency; /// The stored response is written before the body reaches the client and with a token that cannot be /// cancelled: it is the durable record that the side effect already happened, so it has to outlive the /// request that produced it — a client that times out and retries is the commonest duplicate there is. +/// For the same reason the handler itself runs with the client's abort token detached, so a disconnect +/// mid-request cannot leave a committed side effect with no stored response behind it. /// public sealed class IdempotencyEndpointFilter : IEndpointFilter { @@ -56,6 +59,10 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // In-process reservation used when no Redis multiplexer is registered. Single-instance only — // a multi-instance host in this stack already runs Redis (shared Data Protection key ring), so // the Redis branch below covers every deployment where cross-instance duplicates are possible. + // ponytail: the multiplexer and the IDistributedCache are resolved independently, so a host that + // configures Redis for one and not the other (quota Redis without caching Redis) gets a shared + // lock over a per-process entry store. Cross-instance dedup needs the CACHE on Redis; the lock + // alone cannot provide it. private static readonly ConcurrentDictionary InFlight = new(StringComparer.Ordinal); public async ValueTask InvokeAsync(EndpointFilterInvocationContext context, EndpointFilterDelegate next) @@ -90,13 +97,15 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter var tenantId = ResolveTenant(httpContext); - // Scope the entry to the operation as well as the tenant. Keyed on the tenant alone, one key - // reused across two idempotent endpoints replays the first endpoint's response on the second - // — the request silently never runs. That was harmless only while replay never engaged; it - // does now. Anonymous endpoints (self-registration) resolve no tenant claim and share the - // "global" bucket, so scoping by operation is what keeps them apart. + // Scope the entry to the caller and the operation, not the tenant alone. Keyed on the tenant + // alone, one key reused across two idempotent endpoints replays the first endpoint's response + // on the second — the request silently never runs — and two users of the same tenant who pick + // the same low-entropy key ("1", "retry") on the same endpoint get each other's response + // bodies while their own request is suppressed. That was harmless only while replay never + // engaged; it does now. Anonymous endpoints (self-registration) resolve neither a tenant nor a + // caller and share one bucket, so the operation is what keeps them apart from each other. var operation = $"{httpContext.Request.Method}:{RouteIdentity(httpContext)}"; - var cacheKey = CacheKeys.IdempotencyEntry(tenantId, $"{operation}:{idempotencyKey}"); + var cacheKey = CacheKeys.IdempotencyEntry(tenantId, $"{ResolveCaller(httpContext)}:{operation}:{idempotencyKey}"); // Probe-only read via IDistributedCache (real GetAsync, null on miss — unlike HybridCache's // factory). Bypasses L1: replays are rare vs first-calls, so L1 warmth has little value. @@ -120,13 +129,21 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // Another request with this key is in flight. It may have finished between the probe // and the reservation — re-probe once, otherwise report the in-progress conflict. var raced = await ProbeAsync(distributedCache, cacheKey, logger, idempotencyKey, httpContext.RequestAborted).ConfigureAwait(false); - return raced is not null - ? await ReplayAsync(httpContext, raced, idempotencyKey, logger).ConfigureAwait(false) - : TypedResults.Problem( - detail: "A request with this Idempotency-Key is already being processed. Retry shortly.", - instance: httpContext.Request.Path, - statusCode: StatusCodes.Status409Conflict, - title: "Idempotent request in progress"); + if (raced is not null) + { + return await ReplayAsync(httpContext, raced, idempotencyKey, logger).ConfigureAwait(false); + } + + // "Retry shortly" is only actionable with a number on it. One second, not ReservationTtl: + // the original is normally still running and about to store its response, and the TTL is + // the worst case (the holder died) — telling every client to wait it out serializes them + // behind a lock that has probably already been released. + httpContext.Response.Headers.RetryAfter = "1"; + return TypedResults.Problem( + detail: "A request with this Idempotency-Key is already being processed. Retry shortly.", + instance: httpContext.Request.Path, + statusCode: StatusCodes.Status409Conflict, + title: "Idempotent request in progress"); } try @@ -140,7 +157,41 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter return await ReplayAsync(httpContext, settled, idempotencyKey, logger).ConfigureAwait(false); } - var result = await next(context).ConfigureAwait(false); + // The handler runs with the client's abort token detached. It commits a side effect, and + // the record of that side effect is the stored response — so the handler has to reach the + // end even if the client hangs up mid-request. Left attached, a disconnect after the + // commit cancels the next await inside the handler (an EF read, an outbox write, a + // Mediator behaviour), the exception leaves the filter with nothing to store, and the + // client's retry re-executes the side effect: the exact duplicate this filter is for. + // The trade is that a client disconnect no longer aborts an idempotent handler. + var originalAborted = httpContext.RequestAborted; + object? result; + try + { + httpContext.RequestAborted = CancellationToken.None; + result = await next(context).ConfigureAwait(false); + } + finally + { + httpContext.RequestAborted = originalAborted; + } + + // A handler that wrote the response itself (an HttpContext-taking handler returning null) + // has already started it. Capturing is impossible at that point — the buffer swap comes + // too late, so the entry would be an empty body replayed for the full TTL — and setting + // the status below would throw. Hand the handler's own return back to the pipeline and + // leave idempotency out of it. + if (httpContext.Response.HasStarted) + { + logger.LogWarning( + "Idempotent handler for {Operation} started the response itself; nothing captured or stored for key {KeyHash}", + operation, + HashKey(idempotencyKey)); + + // Empty rather than null when the handler returned nothing: a null return makes the + // framework append a serialized "null" to what the handler already wrote. + return result ?? Results.Empty; + } // Execute the result into a buffer to capture the real wire body + status code, then // serve that buffer to the client. Returning the IResult unexecuted would leave @@ -215,7 +266,21 @@ await distributedCache.SetAsync( private static async ValueTask ProbeAsync( IDistributedCache cache, string cacheKey, ILogger logger, string idempotencyKey, CancellationToken ct) { - var bytes = await cache.GetAsync(cacheKey, ct).ConfigureAwait(false); + byte[]? bytes; + try + { + bytes = await cache.GetAsync(cacheKey, ct).ConfigureAwait(false); + } + // Fail open here as well, or the probe is the one link that hard-fails the request: the + // reservation and the store both degrade to a warning when the cache is down, while the probe + // runs on EVERY keyed request — letting a connection error escape takes every idempotent + // endpoint down for the clients that send a key, and leaves it up for the ones that don't. + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogWarning(ex, "Idempotency probe failed for key {KeyHash}; treating it as a miss", HashKey(idempotencyKey)); + return null; + } + if (bytes is not { Length: > 0 }) { return null; @@ -264,6 +329,11 @@ await distributedCache.SetAsync( return Results.Empty; } + // ponytail: the whole response is buffered in memory with no size cap, and with the abort token + // detached a result that never completes on its own never completes here either. Fine for the + // small JSON payloads the idempotent endpoints return; do not put .WithIdempotency() on a + // streaming, SSE or large-file endpoint. Add a size ceiling (skip the store, stream through) + // before one exists. private static async Task ExecuteAndCaptureAsync( object? result, HttpContext httpContext) { @@ -443,11 +513,36 @@ private static string ResolveTenant(HttpContext httpContext) return string.IsNullOrWhiteSpace(fromClaim) ? "global" : fromClaim; } - // The route pattern, not the resolved path: two requests to the same endpoint with different - // route values are different operations and their idempotency keys are already distinct, while - // the pattern keeps the entry stable for the same operation. - private static string RouteIdentity(HttpContext httpContext) => - (httpContext.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText ?? httpContext.Request.Path.ToString(); + // The caller, so one tenant's users don't share an entry. Falls back to the tenant-wide bucket + // for anonymous endpoints, which have no caller to scope by. + private static string ResolveCaller(HttpContext httpContext) + { + var userId = httpContext.User.GetUserId(); + return string.IsNullOrWhiteSpace(userId) ? "anon" : userId; + } + + // The route pattern PLUS its resolved values — the pattern alone makes PUT /tickets/1 and + // PUT /tickets/2 the same operation, so one key reused across two resources replays the first + // one's response and the second update silently never runs. The values are the parsed ones, not + // the raw path, so a retry of the same request matches while a different resource does not. + // The request body is deliberately not part of it: reading it here would buffer every payload, + // so the same key against the same resource with a changed body still replays (documented). + private static string RouteIdentity(HttpContext httpContext) + { + var pattern = (httpContext.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText ?? httpContext.Request.Path.ToString(); + var values = httpContext.Request.RouteValues; + if (values.Count == 0) + { + return pattern; + } + + var resolved = values + .Where(value => value.Value is not null) + .OrderBy(value => value.Key, StringComparer.Ordinal) + .Select(value => $"{value.Key}={value.Value}"); + + return $"{pattern}[{string.Join('&', resolved)}]"; + } private static string HashKey(string key) { diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs index cc0c4d02ac..e1623c75f8 100644 --- a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -7,6 +7,7 @@ using FSH.Framework.Shared.Multitenancy; using FSH.Framework.Web.Idempotency; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Patterns; using Microsoft.Extensions.Caching.Distributed; @@ -336,7 +337,7 @@ public async Task Filter_Should_RunHandler_When_CachedEntryIsUnreadable() // The key the filter reads: tenant + operation + caller key. Proven below by seeding a VALID // entry at it first — otherwise a wrong key here would make the real assertion pass as a // plain cache miss and the test would assert nothing. - var storedKey = CacheKeys.IdempotencyEntry("global", $"POST::{Key}"); + var storedKey = CacheKeys.IdempotencyEntry("global", $"anon:POST::{Key}"); await cache.SetAsync( storedKey, JsonSerializer.SerializeToUtf8Bytes( @@ -454,10 +455,10 @@ await filter.InvokeAsync( _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget")))); var cache = provider.GetRequiredService(); - (await cache.GetAsync(CacheKeys.IdempotencyEntry("global", $"POST::{Key}"))).ShouldNotBeNull( + (await cache.GetAsync(CacheKeys.IdempotencyEntry("global", $"anon:POST::{Key}"))).ShouldNotBeNull( "an unvalidated header must not choose the bucket: taking it would let any caller write into — " + "and replay out of — a real tenant's idempotency namespace."); - (await cache.GetAsync(CacheKeys.IdempotencyEntry("acme", $"POST::{Key}"))).ShouldBeNull(); + (await cache.GetAsync(CacheKeys.IdempotencyEntry("acme", $"anon:POST::{Key}"))).ShouldBeNull(); } // ─── the reservation must be re-probed: the original can settle between probe and reserve ─── @@ -624,6 +625,210 @@ public async Task Reservation_Should_BeRetaken_When_TheHolderOutlivesTheReservat (result as IStatusCodeHttpResult)?.StatusCode.ShouldNotBe(StatusCodes.Status409Conflict); } + // ─── the handler must reach the end even if the client hangs up ───────────────────── + + [Fact] + public async Task Handler_Should_RunToCompletion_When_ClientDisconnectsMidRequest() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var id = Guid.NewGuid(); + + using var aborted = new CancellationTokenSource(); + var first = NewContext(provider, new MemoryStream()); + first.RequestAborted = aborted.Token; + + var committed = false; + try + { + await filter.InvokeAsync( + new TestFilterContext(first), + async invocation => + { + // The side effect commits, then the client gives up — and the handler still has + // work to do (an EF read, an outbox write, a Mediator behaviour), all of which + // observe HttpContext.RequestAborted. + await aborted.CancelAsync().ConfigureAwait(false); + invocation.HttpContext.RequestAborted.ThrowIfCancellationRequested(); + committed = true; + return TypedResults.Ok(new SampleDto(id, "widget")); + }); + } + catch (OperationCanceledException) + { + // Writing the body to a socket the client closed is allowed to fail. + } + + committed.ShouldBeTrue( + "the handler must not be cancelled by the client disconnect: it has already committed, and " + + "the stored response is the only record of that. Cancelled mid-handler, nothing is stored " + + "and the client's retry re-executes the side effect."); + + var replayBody = new MemoryStream(); + var second = NewContext(provider, replayBody); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => throw new InvalidOperationException("handler must NOT re-run after a client disconnect")); + + second.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeTrue(); + } + + // ─── a handler that writes the response itself is left alone ──────────────────────── + + [Fact] + public async Task Filter_Should_PassThrough_When_HandlerStartedTheResponseItself() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + + // DefaultHttpContext's own response feature reports HasStarted = false forever, so the + // scenario needs a feature that says what a real server says once bytes are on the wire. + var body = new MemoryStream(); + var context = NewStartedResponseContext(provider, body); + var result = await filter.InvokeAsync( + new TestFilterContext(context), + async invocation => + { + await invocation.HttpContext.Response.Body.WriteAsync("written-by-the-handler"u8.ToArray()).ConfigureAwait(false); + return null; + }); + + result.ShouldNotBeNull("a null return would make the framework append a serialized \"null\" to the handler's own output"); + + var cache = provider.GetRequiredService(); + (await cache.GetAsync(CacheKeys.IdempotencyEntry("global", $"anon:POST::{Key}"))).ShouldBeNull( + "the buffer swap comes after the handler ran, so a handler that started the response leaves " + + "an EMPTY capture — storing it would replay a blank 200 for the full TTL, and setting the " + + "captured status on an already-started response throws."); + } + + // ─── a cache that is down degrades idempotency, it does not 500 the request ───────── + + [Fact] + public async Task Filter_Should_RunHandler_When_TheProbeItselfFails() + { + var provider = BuildProviderWith(new FaultyCache()); + var filter = new IdempotencyEndpointFilter(); + + int executions = 0; + var result = await filter.InvokeAsync( + new TestFilterContext(NewContext(provider, new MemoryStream())), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget"))); + }); + + executions.ShouldBe( + 1, + "the reservation and the store both fail open, so the probe must too: it runs on every keyed " + + "request, and letting a connection error escape takes every idempotent endpoint down for the " + + "clients that send a key while the ones that don't keep working."); + result.ShouldNotBeNull(); + } + + // ─── same key, different resource: the second request must still run ──────────────── + + [Fact] + public async Task Filter_Should_NotReplayAcrossRouteValues_When_SameKeyTargetsAnotherResource() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + + var onFirstTicket = NewContext(provider, new MemoryStream()); + onFirstTicket.SetEndpoint(RouteEndpointFor("api/v1/tickets/{id}")); + onFirstTicket.Request.RouteValues["id"] = "1"; + await filter.InvokeAsync( + new TestFilterContext(onFirstTicket), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "ticket-1")))); + + int executions = 0; + var onSecondTicket = NewContext(provider, new MemoryStream()); + onSecondTicket.SetEndpoint(RouteEndpointFor("api/v1/tickets/{id}")); + onSecondTicket.Request.RouteValues["id"] = "2"; + await filter.InvokeAsync( + new TestFilterContext(onSecondTicket), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "ticket-2"))); + }); + + executions.ShouldBe( + 1, + "keyed on the route pattern alone, PUT /tickets/1 and PUT /tickets/2 are the same operation: " + + "the second request replays the first ticket's response and its own update never runs."); + onSecondTicket.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeFalse(); + } + + // ─── two users of one tenant must not share an entry ─────────────────────────────── + + [Fact] + public async Task Filter_Should_ScopeEntryToTheCaller_When_TwoUsersShareATenantAndAKey() + { + var cache = NewMemoryCache(); + var filter = new IdempotencyEndpointFilter(); + + var byAlice = NewContext(BuildProviderWith(cache, TenantContext("acme")), new MemoryStream()); + byAlice.User = CallerPrincipal("acme", userId: "alice"); + await filter.InvokeAsync( + new TestFilterContext(byAlice), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "alice-order")))); + + int executions = 0; + var byBob = NewContext(BuildProviderWith(cache, TenantContext("acme")), new MemoryStream()); + byBob.User = CallerPrincipal("acme", userId: "bob"); + await filter.InvokeAsync( + new TestFilterContext(byBob), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "bob-order"))); + }); + + executions.ShouldBe( + 1, + "two users of the same tenant who pick the same low-entropy key on the same endpoint would " + + "otherwise get each other's response body while their own request is silently suppressed."); + byBob.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeFalse(); + } + + // ─── the 409 has to say how long to wait ─────────────────────────────────────────── + + [Fact] + public async Task Conflict_Should_CarryRetryAfter_When_ADuplicateIsStillInFlight() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var holderCall = filter.InvokeAsync( + new TestFilterContext(NewContext(provider, new MemoryStream())), + async _ => + { + started.SetResult(); + await release.Task.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + return TypedResults.Ok(new SampleDto(Guid.NewGuid(), "holder")); + }).AsTask(); + await started.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + var duplicate = NewContext(provider, new MemoryStream()); + var result = await filter.InvokeAsync( + new TestFilterContext(duplicate), + _ => throw new InvalidOperationException("the duplicate's handler must not run")); + + release.SetResult(); + await holderCall.WaitAsync(TimeSpan.FromSeconds(10)); + + (result as IStatusCodeHttpResult)?.StatusCode.ShouldBe(StatusCodes.Status409Conflict); + duplicate.Response.Headers.RetryAfter.ToString().ShouldBe( + "1", + "\"retry shortly\" is only actionable with a number on it, and the original is normally about " + + "to store its response — the reservation TTL is the worst case, not the hint."); + } + // ─── harness ───────────────────────────────────────────────────── private static ServiceProvider BuildProvider() => BuildProvider(new IdempotencyOptions(), multiplexer: null); @@ -739,9 +944,31 @@ private static DefaultHttpContext NewContextForKey(IServiceProvider provider, st return context; } + // A context whose response reports itself as already started, which is what a handler that writes + // to HttpContext.Response leaves behind on a real server. + private DefaultHttpContext NewStartedResponseContext(IServiceProvider provider, Stream responseBody) + { + var features = new FeatureCollection(); + features.Set(new HttpRequestFeature + { + Method = "POST", + Path = "/", + Headers = new HeaderDictionary { ["Idempotency-Key"] = Key }, + }); + features.Set(new StartedResponseFeature()); + features.Set(new StreamResponseBodyFeature(responseBody)); + + return new DefaultHttpContext(features) { RequestServices = provider }; + } + private static ClaimsPrincipal TenantPrincipal(string tenantId) => new(new ClaimsIdentity([new Claim(ClaimConstants.Tenant, tenantId)], "test")); + private static ClaimsPrincipal CallerPrincipal(string tenantId, string userId) => + new(new ClaimsIdentity( + [new Claim(ClaimConstants.Tenant, tenantId), new Claim(ClaimTypes.NameIdentifier, userId)], + "test")); + private sealed record SampleDto(Guid Id, string Name); /// @@ -774,6 +1001,55 @@ public Task SetAsync(string key, byte[] value, DistributedCacheEntryOptions opti } } + private sealed class StartedResponseFeature : IHttpResponseFeature + { + public Stream Body { get; set; } = Stream.Null; + + public bool HasStarted => true; + + public IHeaderDictionary Headers { get; set; } = new HeaderDictionary(); + + public string? ReasonPhrase { get; set; } + + public int StatusCode { get; set; } = StatusCodes.Status200OK; + + public void OnCompleted(Func callback, object state) + { + } + + public void OnStarting(Func callback, object state) + { + } + } + + /// + /// A cache whose connection is down: every operation throws, the way `RedisCache` does when the + /// server is unreachable. + /// + private sealed class FaultyCache : IDistributedCache + { + public byte[]? Get(string key) => throw new InvalidOperationException("cache is down"); + + public Task GetAsync(string key, CancellationToken token = default) => + Task.FromException(new InvalidOperationException("cache is down")); + + public void Refresh(string key) => throw new InvalidOperationException("cache is down"); + + public Task RefreshAsync(string key, CancellationToken token = default) => + Task.FromException(new InvalidOperationException("cache is down")); + + public void Remove(string key) => throw new InvalidOperationException("cache is down"); + + public Task RemoveAsync(string key, CancellationToken token = default) => + Task.FromException(new InvalidOperationException("cache is down")); + + public void Set(string key, byte[] value, DistributedCacheEntryOptions options) => + throw new InvalidOperationException("cache is down"); + + public Task SetAsync(string key, byte[] value, DistributedCacheEntryOptions options, CancellationToken token = default) => + Task.FromException(new InvalidOperationException("cache is down")); + } + /// /// One keyspace for both the cached entries and the reservations — what a real Redis is. /// diff --git a/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs b/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs index 013f334895..749aef42fa 100644 --- a/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs +++ b/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs @@ -1,6 +1,9 @@ using FSH.Framework.Web.Idempotency; using FSH.Framework.Web.RateLimiting; using FSH.Framework.Web.Security; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; namespace Framework.Tests.Web; @@ -67,8 +70,55 @@ public void IdempotencyOptions_Should_HaveDefaults_When_Constructed() // Assert options.HeaderName.ShouldBe("Idempotency-Key"); options.DefaultTtl.ShouldBe(TimeSpan.FromHours(24)); + options.ReservationTtl.ShouldBe(TimeSpan.FromMinutes(1)); options.MaxKeyLength.ShouldBe(128); } + // A bad TTL is invisible at runtime: a zero DefaultTtl throws inside the best-effort cache write, + // which logs a warning and carries on, so nothing is ever stored and replay never engages. It has + // to be rejected at startup instead. + [Theory] + [InlineData("DefaultTtl", "00:00:00")] + [InlineData("ReservationTtl", "00:00:00")] + [InlineData("ReservationTtl", "48:00:00")] + [InlineData("MaxKeyLength", "0")] + [InlineData("HeaderName", "")] + public void AddHeroIdempotency_Should_FailAtStartup_When_OptionsAreInvalid(string key, string value) + { + // Arrange + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection([new KeyValuePair($"IdempotencyOptions:{key}", value)]) + .Build(); + var services = new ServiceCollection(); + services.AddSingleton(configuration); + services.AddHeroIdempotency(configuration); + var provider = services.BuildServiceProvider(); + + // Act + var act = () => provider.GetRequiredService>().Value; + + // Assert + act.ShouldThrow(); + } + + [Fact] + public void AddHeroIdempotency_Should_Bind_When_OptionsAreValid() + { + // Arrange + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection([new KeyValuePair("IdempotencyOptions:ReservationTtl", "00:02:00")]) + .Build(); + var services = new ServiceCollection(); + services.AddSingleton(configuration); + services.AddHeroIdempotency(configuration); + var provider = services.BuildServiceProvider(); + + // Act + var options = provider.GetRequiredService>().Value; + + // Assert — sanity: the validators above reject bad values without rejecting good ones. + options.ReservationTtl.ShouldBe(TimeSpan.FromMinutes(2)); + } + #endregion } From 08fcadeffef44ad1e3bc5b607318c14de2c4e101 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:11:07 -0300 Subject: [PATCH 08/16] test(web): cover the idempotency branches no test could fail on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A test-quality pass over the suite found assertions that survive the mutation they exist to catch, and branches with no test at all. Each case below now fails when the behaviour it pins is reverted. Assertions that could not fail: - The Lua release script was matched with Arg.Any() while the fake hardcoded compare-and-delete, so swapping the script for an unconditional `del` kept the suite green — the exact bug the script's comment warns about. The script text is asserted now. - `(result as IStatusCodeHttpResult)?.StatusCode.ShouldBe(409)` skips the whole assertion for a result that isn't one, which is precisely the mutation it guards. Cast instead. - The concurrency test relied on the default one-minute ReservationTtl outliving the test; a CI freeze past it hands the key over and fails a correct filter. It pins the TTL explicitly. Branches with no coverage: a handler that throws (the release has to stay in the finally, or one exception strands the key until the TTL), the tenant-claim fallback (collapsing it to "global" puts every JWT-only caller in one bucket and replays across tenants), the refused duplicate's re-probe, the restrictive half of the header allow-list (Set-Cookie must not come back on a replay), the best-effort store, a faulting release, the no-header pass-through, the MaxKeyLength rejection, and an entry stored without the Headers member — the shape a previous version wrote, which has to keep replaying through a rolling deploy. Also drops a stale comment claiming body capture is out of reach; this PR is what made it possible, and the integration suite asserts it end to end. --- .../IdempotencyEndpointFilterReplayTests.cs | 371 ++++++++++++++++-- .../Idempotency/IdempotencyFilterTests.cs | 7 +- 2 files changed, 350 insertions(+), 28 deletions(-) diff --git a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs index e1623c75f8..fe9507f07e 100644 --- a/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -1,4 +1,5 @@ using System.Security.Claims; +using System.Text; using System.Text.Json; using Finbuckle.MultiTenant; using Finbuckle.MultiTenant.Abstractions; @@ -8,6 +9,7 @@ using FSH.Framework.Web.Idempotency; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; +using Microsoft.AspNetCore.Http.HttpResults; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Patterns; using Microsoft.Extensions.Caching.Distributed; @@ -101,7 +103,10 @@ await filter.InvokeAsync( [Fact] public async Task Filter_Should_ExecuteHandlerOnce_When_TwoConcurrentRequestsShareKey() { - var provider = BuildProvider(); + // Explicit, generous ReservationTtl rather than the 1-minute default: the assertion is about + // the lock holding, and on the default a CI freeze longer than the TTL would hand the key over + // and turn a real pass into a flake. + var provider = BuildProvider(new IdempotencyOptions { ReservationTtl = TimeSpan.FromMinutes(30) }, multiplexer: null); var filter = new IdempotencyEndpointFilter(); int executions = 0; @@ -136,9 +141,12 @@ public async Task Filter_Should_ExecuteHandlerOnce_When_TwoConcurrentRequestsSha 1, "an idempotent endpoint must execute the handler exactly once for concurrent duplicate keys; " + "the second request should be rejected while the first is in flight."); - (secondResult as IStatusCodeHttpResult)?.StatusCode.ShouldBe( - StatusCodes.Status409Conflict, - "a concurrent duplicate that arrives while the original is still running gets 409 Conflict."); + // Cast, don't null-conditional: `as ... ?.ShouldBe(...)` skips the assertion entirely for a + // result that isn't an IStatusCodeHttpResult — the check evaporates exactly when it's broken. + secondResult.ShouldBeAssignableTo()! + .StatusCode.ShouldBe( + StatusCodes.Status409Conflict, + "a concurrent duplicate that arrives while the original is still running gets 409 Conflict."); } // ─── HIGH: reservation TTL is the short ReservationTtl, not the 24h response TTL ───── @@ -471,19 +479,8 @@ public async Task Filter_Should_Replay_When_OriginalSettledBetweenProbeAndReserv // Simulates the original request finishing in the window between the first probe (a miss) and // the reservation: the entry appears, and the lock it held is already released. - var racing = new SeedAfterFirstMissCache( - inner, - async key => await inner.SetAsync( - key, - JsonSerializer.SerializeToUtf8Bytes( - new CachedIdempotentResponse - { - StatusCode = StatusCodes.Status200OK, - ContentType = "application/json", - Body = JsonSerializer.SerializeToUtf8Bytes(new SampleDto(id, "original"), CacheJsonOpts), - }, - CacheJsonOpts), - new DistributedCacheEntryOptions()).ConfigureAwait(false)); + var racing = new ProbeHookCache(inner); + racing.SeedOnNextProbe(key => SeedEntryAsync(inner, key, id)); var filter = new IdempotencyEndpointFilter(); int executions = 0; @@ -558,8 +555,14 @@ await filter.InvokeAsync( new TestFilterContext(NewContext(provider, new MemoryStream())), _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget")))); + // The script text is asserted, not just the fact that a script ran: an unconditional + // `del` passed through the same call would satisfy a script-agnostic assertion while + // deleting a lock a second request now owns. await db.Received(1).ScriptEvaluateAsync( - Arg.Any(), + Arg.Is(script => + script.Contains("get", StringComparison.Ordinal) && + script.Contains("ARGV[1]", StringComparison.Ordinal) && + script.Contains("del", StringComparison.Ordinal)), Arg.Any(), Arg.Is(values => values.Length == 1 && values[0] == storedToken), Arg.Any()); @@ -822,13 +825,288 @@ public async Task Conflict_Should_CarryRetryAfter_When_ADuplicateIsStillInFlight release.SetResult(); await holderCall.WaitAsync(TimeSpan.FromSeconds(10)); - (result as IStatusCodeHttpResult)?.StatusCode.ShouldBe(StatusCodes.Status409Conflict); + result.ShouldBeAssignableTo()!.StatusCode.ShouldBe(StatusCodes.Status409Conflict); duplicate.Response.Headers.RetryAfter.ToString().ShouldBe( "1", "\"retry shortly\" is only actionable with a number on it, and the original is normally about " + "to store its response — the reservation TTL is the worst case, not the hint."); } + // ─── a handler that throws must not strand the key ───────────────────────────────── + + [Fact] + public async Task Filter_Should_ReleaseTheReservation_When_TheHandlerThrows() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + + var thrown = await Should.ThrowAsync(async () => + await filter.InvokeAsync( + new TestFilterContext(NewContext(provider, new MemoryStream())), + _ => throw new InvalidOperationException("handler blew up"))); + + thrown.Message.ShouldBe( + "handler blew up", + "the filter must not swallow a handler exception — the global handler turns it into ProblemDetails"); + + int executions = 0; + var retry = NewContext(provider, new MemoryStream()); + var result = await filter.InvokeAsync( + new TestFilterContext(retry), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget"))); + }); + + executions.ShouldBe( + 1, + "released only on the success path, a handler that throws (validation, DB down) strands the " + + "reservation and every retry of that key 409s until the TTL expires. The release belongs in " + + "the finally."); + result.ShouldBeOfType(); + } + + // ─── the claim fallback still partitions tenants ─────────────────────────────────── + + [Fact] + public async Task Filter_Should_ScopeEntryToTheTenantClaim_When_NoTenantContextIsResolved() + { + var cache = NewMemoryCache(); + var filter = new IdempotencyEndpointFilter(); + + // A JWT-only request: no tenant header, so Finbuckle's claim strategy (which runs before + // authentication) resolved nothing and the claim is all there is. + var fromAcme = NewContext(BuildProviderWith(cache), new MemoryStream()); + fromAcme.User = CallerPrincipal("acme", userId: "shared-integration"); + await filter.InvokeAsync( + new TestFilterContext(fromAcme), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "acme-order")))); + + int executions = 0; + var fromGlobex = NewContext(BuildProviderWith(cache), new MemoryStream()); + fromGlobex.User = CallerPrincipal("globex", userId: "shared-integration"); + await filter.InvokeAsync( + new TestFilterContext(fromGlobex), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "globex-order"))); + }); + + executions.ShouldBe( + 1, + "collapsing the claim fallback to \"global\" puts every JWT-only caller in one bucket: same " + + "key, same route, and tenant A's response body replays to tenant B."); + fromGlobex.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeFalse(); + } + + // ─── the denied duplicate re-probes before it 409s ───────────────────────────────── + + [Fact] + public async Task DeniedDuplicate_Should_Replay_When_TheOriginalSettledWhileItWasBeingRefused() + { + var inner = NewMemoryCache(); + var cache = new ProbeHookCache(inner); + var provider = BuildProviderWith(cache); + var filter = new IdempotencyEndpointFilter(); + var id = Guid.NewGuid(); + + var started = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + // The original is in flight and holds the reservation, so the duplicate below is refused. + var holderCall = filter.InvokeAsync( + new TestFilterContext(NewContext(provider, new MemoryStream())), + async _ => + { + started.SetResult(); + await release.Task.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + return TypedResults.Ok(new SampleDto(id, "original")); + }).AsTask(); + await started.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + // The original stores its response between the duplicate's first probe and its refusal. + cache.SeedOnNextProbe(key => SeedEntryAsync(inner, key, id)); + + int executions = 0; + var duplicate = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(duplicate), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "duplicate"))); + }); + + release.SetResult(); + await holderCall.WaitAsync(TimeSpan.FromSeconds(10)); + + executions.ShouldBe(0); + duplicate.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeTrue( + "409ing straight from the refusal returns a conflict for a request whose answer is already " + + "stored — the refused duplicate has to re-probe once before reporting the conflict."); + } + + // ─── the header allow-list is a list, not a copy ─────────────────────────────────── + + [Fact] + public async Task Replay_Should_CarryOnlyAllowListedHeaders_When_TheHandlerSetOthers() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + + var first = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(first), + invocation => + { + invocation.HttpContext.Response.Headers.ETag = "\"v1\""; + invocation.HttpContext.Response.Headers.SetCookie = "session=abc; Path=/"; + invocation.HttpContext.Response.Headers["X-Trace"] = "first-call-only"; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget"))); + }); + + first.Response.Headers.SetCookie.ToString().ShouldNotBeEmpty("sanity: the first call sets the header on the real response"); + + var second = NewContext(provider, new MemoryStream()); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => throw new InvalidOperationException("handler must NOT run on an idempotent replay")); + + second.Response.Headers.ETag.ToString().ShouldBe("\"v1\"", "ETag is on the allow-list and carries meaning for the caller"); + second.Response.Headers.SetCookie.ToString().ShouldBeEmpty( + "replaying everything the first response carried resurrects a stale Set-Cookie (and a stale " + + "Content-Length or Date, which corrupts the response) hours after the fact."); + second.Response.Headers.ContainsKey("X-Trace").ShouldBeFalse(); + } + + // ─── a store that fails must not fail the request ────────────────────────────────── + + [Fact] + public async Task Filter_Should_Succeed_When_TheStoreFails() + { + var provider = BuildProviderWith(new WriteFaultyCache(NewMemoryCache())); + var filter = new IdempotencyEndpointFilter(); + + var result = await filter.InvokeAsync( + new TestFilterContext(NewContext(provider, new MemoryStream())), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget")))); + + result.ShouldBeOfType( + "the handler's side effect has already committed when the store runs; 500ing the request " + + "because the cache is down reports a failure for work that succeeded."); + } + + [Fact] + public async Task Filter_Should_NotThrow_When_TheReleaseFaults() + { + var db = Substitute.For(); + db.StringSetAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromResult(true)); + db.ScriptEvaluateAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Task.FromException(new RedisException("release blip"))); + var provider = BuildProvider(new IdempotencyOptions(), RedisMultiplexer(db)); + var filter = new IdempotencyEndpointFilter(); + + var result = await filter.InvokeAsync( + new TestFilterContext(NewContext(provider, new MemoryStream())), + _ => ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget")))); + + result.ShouldBeOfType( + "the release runs in a finally, after the response has gone to the client — an exception out " + + "of it can only reset the connection on a request that already succeeded. The short " + + "ReservationTtl cleans up the missed delete."); + } + + // ─── the opt-in is per request: no header, no idempotency ────────────────────────── + + [Fact] + public async Task Filter_Should_PassThrough_When_NoKeyHeaderIsSent() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var handlerResult = TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget")); + + var first = NewContextForKey(provider, idempotencyKey: string.Empty); + var result = await filter.InvokeAsync( + new TestFilterContext(first), + _ => ValueTask.FromResult(handlerResult)); + + result.ShouldBeSameAs( + handlerResult, + "with no key the filter is a no-op: the handler's own result goes back unexecuted, exactly as " + + "on an endpoint without .WithIdempotency()."); + first.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeFalse(); + + int executions = 0; + var second = NewContextForKey(provider, idempotencyKey: string.Empty); + await filter.InvokeAsync( + new TestFilterContext(second), + _ => + { + executions++; + return ValueTask.FromResult(handlerResult); + }); + + executions.ShouldBe( + 1, + "keyless requests must not share one entry: dropping the empty-key guard puts every one of " + + "them in the same bucket and the second caller replays the first caller's response."); + } + + [Fact] + public async Task Filter_Should_Reject_When_TheKeyIsLongerThanMaxKeyLength() + { + var options = new IdempotencyOptions { MaxKeyLength = 16 }; + var provider = BuildProvider(options, multiplexer: null); + var filter = new IdempotencyEndpointFilter(); + + int executions = 0; + var result = await filter.InvokeAsync( + new TestFilterContext(NewContextForKey(provider, new string('k', options.MaxKeyLength + 1))), + _ => + { + executions++; + return ValueTask.FromResult(TypedResults.Ok(new SampleDto(Guid.NewGuid(), "widget"))); + }); + + executions.ShouldBe(0, "an over-long key is rejected before the handler runs"); + result.ShouldBeAssignableTo()! + .StatusCode.ShouldBe( + StatusCodes.Status400BadRequest, + "an unbounded key is a cache-key injection surface, and the rejection goes out as RFC 9457 " + + "ProblemDetails like every other error on these endpoints."); + } + + // ─── an entry written before headers were captured still replays ─────────────────── + + [Fact] + public async Task Replay_Should_Work_When_TheStoredEntryHasNoHeaders() + { + var provider = BuildProvider(); + var filter = new IdempotencyEndpointFilter(); + var cache = provider.GetRequiredService(); + + // The shape a previous version wrote: status, content type and body, no headers member. + await cache.SetAsync( + CacheKeys.IdempotencyEntry("global", $"anon:POST::{Key}"), + """{"statusCode":200,"contentType":"application/json","body":"eyJvayI6dHJ1ZX0="}"""u8.ToArray(), + new DistributedCacheEntryOptions()); + + var replayBody = new MemoryStream(); + var context = NewContext(provider, replayBody); + await filter.InvokeAsync( + new TestFilterContext(context), + _ => throw new InvalidOperationException("handler must NOT run: the entry is readable")); + + context.Response.StatusCode.ShouldBe(StatusCodes.Status200OK); + context.Response.Headers.ContainsKey("Idempotency-Replayed").ShouldBeTrue( + "a rolling deploy replays entries written by the previous version: a missing headers member " + + "must deserialize to empty, not throw and discard the entry."); + Encoding.UTF8.GetString(replayBody.ToArray()).ShouldBe("""{"ok":true}"""); + } + // ─── harness ───────────────────────────────────────────────────── private static ServiceProvider BuildProvider() => BuildProvider(new IdempotencyOptions(), multiplexer: null); @@ -881,6 +1159,20 @@ private static ServiceProvider BuildProviderWith( private static MemoryDistributedCache NewMemoryCache() => new(Options.Create(new MemoryDistributedCacheOptions())); + // The entry the original request would have stored, written straight into the store. + private static Task SeedEntryAsync(MemoryDistributedCache cache, string cacheKey, Guid id) => + cache.SetAsync( + cacheKey, + JsonSerializer.SerializeToUtf8Bytes( + new CachedIdempotentResponse + { + StatusCode = StatusCodes.Status200OK, + ContentType = "application/json", + Body = JsonSerializer.SerializeToUtf8Bytes(new SampleDto(id, "original"), CacheJsonOpts), + }, + CacheJsonOpts), + new DistributedCacheEntryOptions()); + // What Finbuckle leaves behind for the endpoint filter: the tenant the request is scoped to, // which for a root operator using the tenant header is the target, not the caller's own tenant. private static IMultiTenantContextAccessor TenantContext(string tenantId) @@ -1132,22 +1424,25 @@ public Task SetAsync(string key, byte[] value, DistributedCacheEntryOptions opti } /// - /// Answers the first probe as a miss and then seeds the entry, reproducing the window in which - /// the original request stores its response and releases the lock — the window a single - /// probe-before-reserve cannot see. + /// Lets a test drop an entry into the store at an exact point in the filter's sequence: the hook + /// runs after one probe has already answered, which is the window in which the original request + /// stores its response and releases the lock. /// - private sealed class SeedAfterFirstMissCache(IDistributedCache inner, Func seed) : IDistributedCache + private sealed class ProbeHookCache(IDistributedCache inner) : IDistributedCache { - private int _probes; + private Func? _onNextProbe; + + public void SeedOnNextProbe(Func seed) => Interlocked.Exchange(ref _onNextProbe, seed); public byte[]? Get(string key) => inner.Get(key); public async Task GetAsync(string key, CancellationToken token = default) { var bytes = await inner.GetAsync(key, token).ConfigureAwait(false); - if (Interlocked.Increment(ref _probes) == 1) + var hook = Interlocked.Exchange(ref _onNextProbe, null); + if (hook is not null) { - await seed(key).ConfigureAwait(false); + await hook(key).ConfigureAwait(false); } return bytes; @@ -1167,6 +1462,30 @@ public Task SetAsync(string key, byte[] value, DistributedCacheEntryOptions opti inner.SetAsync(key, value, options, token); } + /// + /// Reads fine, refuses to write — a cache that has gone down between the probe and the store. + /// + private sealed class WriteFaultyCache(IDistributedCache inner) : IDistributedCache + { + public byte[]? Get(string key) => inner.Get(key); + + public Task GetAsync(string key, CancellationToken token = default) => inner.GetAsync(key, token); + + public void Refresh(string key) => inner.Refresh(key); + + public Task RefreshAsync(string key, CancellationToken token = default) => inner.RefreshAsync(key, token); + + public void Remove(string key) => inner.Remove(key); + + public Task RemoveAsync(string key, CancellationToken token = default) => inner.RemoveAsync(key, token); + + public void Set(string key, byte[] value, DistributedCacheEntryOptions options) => + throw new InvalidOperationException("cache write is down"); + + public Task SetAsync(string key, byte[] value, DistributedCacheEntryOptions options, CancellationToken token = default) => + Task.FromException(new InvalidOperationException("cache write is down")); + } + private sealed class TestFilterContext : EndpointFilterInvocationContext { public TestFilterContext(HttpContext httpContext) => HttpContext = httpContext; diff --git a/src/Tests/Integration.Tests/Tests/Idempotency/IdempotencyFilterTests.cs b/src/Tests/Integration.Tests/Tests/Idempotency/IdempotencyFilterTests.cs index 0b17040669..619a9aed92 100644 --- a/src/Tests/Integration.Tests/Tests/Idempotency/IdempotencyFilterTests.cs +++ b/src/Tests/Integration.Tests/Tests/Idempotency/IdempotencyFilterTests.cs @@ -16,8 +16,11 @@ public IdempotencyFilterTests(FshWebApplicationFactory factory) _auth = new AuthHelper(factory); } - // Full replay-with-matching-body coverage isn't possible yet (filter captures the raw IResult, not the body — dotnet/aspnetcore#57191, backlog 2.4b). - // These tests verify only the wiring: Idempotency-Replayed header presence/absence and that a distinct key forces fresh execution. + // These cover the wiring: Idempotency-Replayed presence/absence, and that a distinct key forces fresh + // execution. Replay of the exact body — no longer out of reach, the filter buffers the executed result + // instead of caching the raw IResult — is asserted end-to-end against real Redis in + // ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused, and branch + // by branch in Framework.Tests/Web/IdempotencyEndpointFilterReplayTests. [Fact] public async Task CreateBillingPlan_Should_ExecuteNormally_When_NoIdempotencyKey() From f3d66c56d7edb4abed74562cad504565a5e42cda Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:32:06 -0300 Subject: [PATCH 09/16] test(identity): pin that the caller id claim survives bearer inbound mapping The idempotency filter scopes its cache key by ClaimsPrincipal.GetUserId(), which reads ClaimTypes.NameIdentifier only. Until now nothing proved that claim type is present after JwtBearer validates a real issued token: every existing test built the principal by hand, so caller scoping could have been inert in production (every caller collapsing into one bucket) with a green suite. Round-trips a token from TokenService through JsonWebTokenHandler configured with JwtBearerOptions' own MapInboundClaims default, then asserts GetUserId() resolves. Verified with the claim removed from the token as well: the short-form `sub` maps to it, so both shapes IdentityService emits resolve. --- .../Services/TokenServiceTests.cs | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/Tests/Identity.Tests/Services/TokenServiceTests.cs b/src/Tests/Identity.Tests/Services/TokenServiceTests.cs index 260ca098e9..a685436957 100644 --- a/src/Tests/Identity.Tests/Services/TokenServiceTests.cs +++ b/src/Tests/Identity.Tests/Services/TokenServiceTests.cs @@ -4,9 +4,12 @@ using System.Text; using FSH.Modules.Identity; using FSH.Modules.Identity.Authorization.Jwt; +using FSH.Framework.Shared.Identity.Claims; using FSH.Modules.Identity.Services; +using Microsoft.AspNetCore.Authentication.JwtBearer; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using Microsoft.IdentityModel.JsonWebTokens; using Microsoft.IdentityModel.Tokens; using NSubstitute; @@ -160,6 +163,54 @@ public async Task IssueAsync_Should_ProduceTokenSignedWithConfiguredKey() validationResult.IsValid.ShouldBeTrue(); } + [Fact] + public async Task IssuedToken_Should_ExposeUserIdClaim_AfterBearerInboundMapping() + { + // Arrange - IdentityService emits the user id twice: as the RFC 7519 short-form `sub` and as + // ClaimTypes.NameIdentifier. Everything that scopes per caller reads it through + // ClaimsPrincipal.GetUserId(), which looks at ClaimTypes.NameIdentifier only — the idempotency + // endpoint filter's cache key and per-user rate limiting among them. If inbound claim mapping + // stopped producing that claim type, those would silently collapse every authenticated caller + // into one bucket instead of failing loudly, so pin the round trip through the real handler. + var options = Options.Create(new JwtOptions + { + Issuer = Issuer, + Audience = Audience, + SigningKey = SigningKey, + AccessTokenMinutes = 30, + RefreshTokenDays = 7 + }); + var service = new TokenService(options, _logger, _metrics, TimeProvider.System); + Claim[] claims = + [ + new(System.IdentityModel.Tokens.Jwt.JwtRegisteredClaimNames.Sub, "user-123"), + new(ClaimTypes.NameIdentifier, "user-123"), + new(ClaimTypes.Email, "user@example.com") + ]; + + // Act + var response = await service.IssueAsync("user-123", claims); + + // JwtBearer validates with JsonWebTokenHandler and passes its own MapInboundClaims through; + // read that default off JwtBearerOptions instead of hardcoding it, and ConfigureJwtBearerOptions + // never overrides it, so this mirrors what the pipeline actually does to the token. + var handler = new JsonWebTokenHandler { MapInboundClaims = new JwtBearerOptions().MapInboundClaims }; + var validationResult = await handler.ValidateTokenAsync( + response.AccessToken, + new TokenValidationParameters + { + ValidIssuer = Issuer, + ValidAudience = Audience, + IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(SigningKey)), + RoleClaimType = ClaimTypes.Role, + ClockSkew = TimeSpan.FromMinutes(2) + }); + + // Assert + validationResult.IsValid.ShouldBeTrue(); + new ClaimsPrincipal(validationResult.ClaimsIdentity).GetUserId().ShouldBe("user-123"); + } + #endregion #region IssueAccessOnlyAsync Tests From 5b33004a24a2056e351a1d771f30ef7c10f06832 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:34:26 -0300 Subject: [PATCH 10/16] docs(agents): idempotency rule covers the abort-token, probe and key changes The rule described the reservation work from the previous round but not what landed after it, so an agent reading it would still believe the probe hard-fails and the key ignores route values and the caller. Adds the abort-token detachment together with the constraint it implies (no streaming endpoints), the HasStarted pass-through, Retry-After and the startup validation. --- .agents/rules/security.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.agents/rules/security.md b/.agents/rules/security.md index 4a0ebcadba..51ea7554b9 100644 --- a/.agents/rules/security.md +++ b/.agents/rules/security.md @@ -17,7 +17,7 @@ Chained partitioned fixed-window limiter: **tenant → user → IP** (defaults 1 ## Idempotency (`Web/Idempotency/`) -Opt-in per endpoint with **`.WithIdempotency()`**. Reads the `Idempotency-Key` header (max 128 chars, `DefaultTtl` 24h); replays return the cached status, body and the allow-listed headers (`Location`, `ETag`) plus `Idempotency-Replayed: true`. Cache key is scoped to tenant **and operation** (method + route pattern) via `CacheKeys.IdempotencyEntry` — tenant alone lets one key reused on a second idempotent endpoint replay the first one's response, and anonymous endpoints (self-registration) all share the `"global"` tenant. Tenant comes from the **resolved** `IMultiTenantContextAccessor` (what the side effect is scoped to, incl. a root operator's target tenant), claim as fallback, `"global"` otherwise — never the raw `tenant` header, which is unvalidated when Finbuckle didn't resolve it. Probe and write share one `IDistributedCache` + key + serializer — asymmetry there makes replay silently never engage. Only **2xx** is stored: a failure isn't a record of a committed side effect, and caching it would lock the key out for the full TTL. The store happens **before** the body reaches the client and on `CancellationToken.None`, so a client that times out and retries replays instead of re-executing. Concurrent duplicates are serialized by an in-flight reservation (Redis `SET NX`, else in-process) under a `lock:` prefix — never a suffix on the entry key, which a caller key ending in it would collide with — on the short `ReservationTtl` (default 1m, must outlast the slowest handler; both branches expire on it). The cache is re-probed **after** the lock is taken (the original can settle in the probe→reserve window), release is a compare-and-delete on the reservation's own token, and a duplicate still in flight gets **409**. Reserve and release both fail open; a request that failed open holds no token and releases nothing. Put it on POSTs that must be replay-safe (e.g. CreateTenant). +Opt-in per endpoint with **`.WithIdempotency()`**. Reads the `Idempotency-Key` header (max 128 chars, `DefaultTtl` 24h); replays return the cached status, body and the allow-listed headers (`Location`, `ETag`) plus `Idempotency-Replayed: true`. Cache key is scoped to tenant **and operation** (method + route pattern) via `CacheKeys.IdempotencyEntry` — tenant alone lets one key reused on a second idempotent endpoint replay the first one's response, and anonymous endpoints (self-registration) all share the `"global"` tenant. Tenant comes from the **resolved** `IMultiTenantContextAccessor` (what the side effect is scoped to, incl. a root operator's target tenant), claim as fallback, `"global"` otherwise — never the raw `tenant` header, which is unvalidated when Finbuckle didn't resolve it. Probe and write share one `IDistributedCache` + key + serializer — asymmetry there makes replay silently never engage. Only **2xx** is stored: a failure isn't a record of a committed side effect, and caching it would lock the key out for the full TTL. The store happens **before** the body reaches the client and on `CancellationToken.None`, so a client that times out and retries replays instead of re-executing. Concurrent duplicates are serialized by an in-flight reservation (Redis `SET NX`, else in-process) under a `lock:` prefix — never a suffix on the entry key, which a caller key ending in it would collide with — on the short `ReservationTtl` (default 1m, must outlast the slowest handler; both branches expire on it). The cache is re-probed **after** the lock is taken (the original can settle in the probe→reserve window), release is a compare-and-delete on the reservation's own token, and a duplicate still in flight gets **409**. Reserve and release both fail open; a request that failed open holds no token and releases nothing. The **probe** fails open too — it runs on every request carrying a key, so a Redis blip must not 500 the endpoint for exactly those clients. The key also folds in the resolved **route values** (`PUT /tickets/1` ≠ `/tickets/2`) and the **caller** (`GetUserId()`, `"anon"` otherwise), so two users of one tenant reusing a low-entropy key never see each other's body. The handler runs with `HttpContext.RequestAborted` **detached** — a client hanging up after the side effect committed used to cancel the handler's next await, leaving nothing to store and letting the retry re-execute; the trade-off is that a disconnect no longer aborts an idempotent handler, so keep those handlers short and **never** put `.WithIdempotency()` on a streaming or large-file endpoint (the response is buffered to be captured). A handler that writes to `Response` itself is passed through untouched (`HasStarted`), the 409 carries `Retry-After: 1`, and `IdempotencyOptions` is validated on start. Put it on POSTs that must be replay-safe (e.g. CreateTenant). ## Quota enforcement (`Quota/`) From e6b0bb7ee84d0b377577d60100d1a96b86e6c7c1 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:49:13 -0300 Subject: [PATCH 11/16] test(web): assert idempotency options through IStartupValidator, not lazily MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The startup-validation tests resolved IOptions.Value, which validates on first access with or without .ValidateOnStart(). Deleting that call left all six green while moving the failure from boot to the first request that carries an Idempotency-Key — a suite that could not see the difference between "rejected at startup" and "rejected once, in production, per process". They now go through IStartupValidator, which is what .ValidateOnStart() registers and what the host runs before serving traffic. Verified: with .ValidateOnStart() removed, 6 of the 10 tests fail. --- src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs b/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs index 749aef42fa..cb42af10f7 100644 --- a/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs +++ b/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs @@ -94,8 +94,10 @@ public void AddHeroIdempotency_Should_FailAtStartup_When_OptionsAreInvalid(strin services.AddHeroIdempotency(configuration); var provider = services.BuildServiceProvider(); - // Act - var act = () => provider.GetRequiredService>().Value; + // Act — through IStartupValidator, which is what .ValidateOnStart() registers and what the host + // runs before serving traffic. Resolving IOptions<>.Value instead would validate lazily and pass + // with .ValidateOnStart() deleted, moving the failure from boot to the first keyed request. + var act = () => provider.GetRequiredService().Validate(); // Assert act.ShouldThrow(); @@ -114,9 +116,11 @@ public void AddHeroIdempotency_Should_Bind_When_OptionsAreValid() var provider = services.BuildServiceProvider(); // Act + provider.GetRequiredService().Validate(); var options = provider.GetRequiredService>().Value; - // Assert — sanity: the validators above reject bad values without rejecting good ones. + // Assert — sanity: the validators above reject bad values without rejecting good ones, and the + // startup validation this configuration passes through does not reject a valid one. options.ReservationTtl.ShouldBe(TimeSpan.FromMinutes(2)); } From 75eb3b9ba11ca79133fc159572502f0f22f11bfb Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:57:22 -0300 Subject: [PATCH 12/16] test(web): pin each idempotency options clause to its own failure message Asserting only OptionsValidationException let a clause be deleted with every row still green: a zero DefaultTtl also trips "ReservationTtl must not exceed DefaultTtl", so the row aimed at DefaultTtl passed on the wrong clause. Each row now names the failure it expects. Per-clause mutation run: removing any one of the five clauses fails exactly the one row that targets it; removing .ValidateOnStart() fails all six cases, since IStartupValidator is then unregistered. --- .../Web/OptionsDefaultsTests.cs | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs b/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs index cb42af10f7..8db3077d39 100644 --- a/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs +++ b/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs @@ -77,13 +77,16 @@ public void IdempotencyOptions_Should_HaveDefaults_When_Constructed() // A bad TTL is invisible at runtime: a zero DefaultTtl throws inside the best-effort cache write, // which logs a warning and carries on, so nothing is ever stored and replay never engages. It has // to be rejected at startup instead. + // The expected message is part of the case: a zero DefaultTtl also trips the + // "ReservationTtl must not exceed DefaultTtl" clause, so asserting only the exception type let the + // DefaultTtl clause be deleted with every row still green. [Theory] - [InlineData("DefaultTtl", "00:00:00")] - [InlineData("ReservationTtl", "00:00:00")] - [InlineData("ReservationTtl", "48:00:00")] - [InlineData("MaxKeyLength", "0")] - [InlineData("HeaderName", "")] - public void AddHeroIdempotency_Should_FailAtStartup_When_OptionsAreInvalid(string key, string value) + [InlineData("DefaultTtl", "00:00:00", "DefaultTtl must be greater than zero")] + [InlineData("ReservationTtl", "00:00:00", "ReservationTtl must be greater than zero")] + [InlineData("ReservationTtl", "48:00:00", "ReservationTtl must not exceed DefaultTtl")] + [InlineData("MaxKeyLength", "0", "MaxKeyLength must be greater than zero")] + [InlineData("HeaderName", "", "HeaderName is required")] + public void AddHeroIdempotency_Should_FailAtStartup_When_OptionsAreInvalid(string key, string value, string expectedFailure) { // Arrange var configuration = new ConfigurationBuilder() @@ -99,8 +102,9 @@ public void AddHeroIdempotency_Should_FailAtStartup_When_OptionsAreInvalid(strin // with .ValidateOnStart() deleted, moving the failure from boot to the first keyed request. var act = () => provider.GetRequiredService().Validate(); - // Assert - act.ShouldThrow(); + // Assert — the failure has to be the clause this row targets, not any clause that happens to trip + act.ShouldThrow() + .Failures.ShouldContain(failure => failure.Contains(expectedFailure, StringComparison.Ordinal)); } [Fact] From 430122aa067f0830198b3dd6d752301ec4bb4a08 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:10:59 -0300 Subject: [PATCH 13/16] fix(identity): drop idempotency from self-registration, and gate anonymous endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cache key scopes by caller, and ResolveCaller returns "anon" for every unauthenticated request, so on an anonymous endpoint all callers share one bucket. Two people registering on the same tenant with the same low-entropy key ("1", "retry") built the identical key: the second replayed the first registrant's 201 with the first registrant's UserId, and their own account was silently never created. /self-register was the only anonymous idempotent endpoint. It was unreachable until replay started engaging. A retry there is already safe without the filter — the unique-email constraint rejects the duplicate — so the endpoint drops .WithIdempotency() rather than gaining a body fingerprint. Deleting one call would leave nothing stopping the next one, so WithIdempotency() now attaches IdempotentEndpointMetadata: an endpoint filter is invisible in metadata, and the marker makes the wiring inspectable. IdempotencyWiringTests walks the endpoint map and fails when an AllowAnonymous() endpoint carries it, plus a second test that fails if the marker stops being attached, so the first cannot pass over an empty set. --- .agents/rules/security.md | 2 +- .../Idempotency/IdempotencyEndpointFilter.cs | 8 ++- .../Idempotency/IdempotentEndpointMetadata.cs | 14 +++++ .../SelfRegisterUserEndpoint.cs | 5 +- .../Idempotency/IdempotencyWiringTests.cs | 55 +++++++++++++++++++ 5 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 src/BuildingBlocks/Web/Idempotency/IdempotentEndpointMetadata.cs create mode 100644 src/Tests/Integration.Tests/Tests/Idempotency/IdempotencyWiringTests.cs diff --git a/.agents/rules/security.md b/.agents/rules/security.md index 51ea7554b9..232e8d32ab 100644 --- a/.agents/rules/security.md +++ b/.agents/rules/security.md @@ -17,7 +17,7 @@ Chained partitioned fixed-window limiter: **tenant → user → IP** (defaults 1 ## Idempotency (`Web/Idempotency/`) -Opt-in per endpoint with **`.WithIdempotency()`**. Reads the `Idempotency-Key` header (max 128 chars, `DefaultTtl` 24h); replays return the cached status, body and the allow-listed headers (`Location`, `ETag`) plus `Idempotency-Replayed: true`. Cache key is scoped to tenant **and operation** (method + route pattern) via `CacheKeys.IdempotencyEntry` — tenant alone lets one key reused on a second idempotent endpoint replay the first one's response, and anonymous endpoints (self-registration) all share the `"global"` tenant. Tenant comes from the **resolved** `IMultiTenantContextAccessor` (what the side effect is scoped to, incl. a root operator's target tenant), claim as fallback, `"global"` otherwise — never the raw `tenant` header, which is unvalidated when Finbuckle didn't resolve it. Probe and write share one `IDistributedCache` + key + serializer — asymmetry there makes replay silently never engage. Only **2xx** is stored: a failure isn't a record of a committed side effect, and caching it would lock the key out for the full TTL. The store happens **before** the body reaches the client and on `CancellationToken.None`, so a client that times out and retries replays instead of re-executing. Concurrent duplicates are serialized by an in-flight reservation (Redis `SET NX`, else in-process) under a `lock:` prefix — never a suffix on the entry key, which a caller key ending in it would collide with — on the short `ReservationTtl` (default 1m, must outlast the slowest handler; both branches expire on it). The cache is re-probed **after** the lock is taken (the original can settle in the probe→reserve window), release is a compare-and-delete on the reservation's own token, and a duplicate still in flight gets **409**. Reserve and release both fail open; a request that failed open holds no token and releases nothing. The **probe** fails open too — it runs on every request carrying a key, so a Redis blip must not 500 the endpoint for exactly those clients. The key also folds in the resolved **route values** (`PUT /tickets/1` ≠ `/tickets/2`) and the **caller** (`GetUserId()`, `"anon"` otherwise), so two users of one tenant reusing a low-entropy key never see each other's body. The handler runs with `HttpContext.RequestAborted` **detached** — a client hanging up after the side effect committed used to cancel the handler's next await, leaving nothing to store and letting the retry re-execute; the trade-off is that a disconnect no longer aborts an idempotent handler, so keep those handlers short and **never** put `.WithIdempotency()` on a streaming or large-file endpoint (the response is buffered to be captured). A handler that writes to `Response` itself is passed through untouched (`HasStarted`), the 409 carries `Retry-After: 1`, and `IdempotencyOptions` is validated on start. Put it on POSTs that must be replay-safe (e.g. CreateTenant). +Opt-in per endpoint with **`.WithIdempotency()`**. Reads the `Idempotency-Key` header (max 128 chars, `DefaultTtl` 24h); replays return the cached status, body and the allow-listed headers (`Location`, `ETag`) plus `Idempotency-Replayed: true`. Cache key is scoped to tenant **and operation** (method + route pattern) via `CacheKeys.IdempotencyEntry` — tenant alone lets one key reused on a second idempotent endpoint replay the first one's response, and anonymous endpoints (self-registration) all share the `"global"` tenant. Tenant comes from the **resolved** `IMultiTenantContextAccessor` (what the side effect is scoped to, incl. a root operator's target tenant), claim as fallback, `"global"` otherwise — never the raw `tenant` header, which is unvalidated when Finbuckle didn't resolve it. Probe and write share one `IDistributedCache` + key + serializer — asymmetry there makes replay silently never engage. Only **2xx** is stored: a failure isn't a record of a committed side effect, and caching it would lock the key out for the full TTL. The store happens **before** the body reaches the client and on `CancellationToken.None`, so a client that times out and retries replays instead of re-executing. Concurrent duplicates are serialized by an in-flight reservation (Redis `SET NX`, else in-process) under a `lock:` prefix — never a suffix on the entry key, which a caller key ending in it would collide with — on the short `ReservationTtl` (default 1m, must outlast the slowest handler; both branches expire on it). The cache is re-probed **after** the lock is taken (the original can settle in the probe→reserve window), release is a compare-and-delete on the reservation's own token, and a duplicate still in flight gets **409**. Reserve and release both fail open; a request that failed open holds no token and releases nothing. The **probe** fails open too — it runs on every request carrying a key, so a Redis blip must not 500 the endpoint for exactly those clients. The key also folds in the resolved **route values** (`PUT /tickets/1` ≠ `/tickets/2`) and the **caller** (`GetUserId()`, `"anon"` otherwise), so two users of one tenant reusing a low-entropy key never see each other's body. The handler runs with `HttpContext.RequestAborted` **detached** — a client hanging up after the side effect committed used to cancel the handler's next await, leaving nothing to store and letting the retry re-execute; the trade-off is that a disconnect no longer aborts an idempotent handler, so keep those handlers short and **never** put `.WithIdempotency()` on a streaming or large-file endpoint (the response is buffered to be captured). A handler that writes to `Response` itself is passed through untouched (`HasStarted`), the 409 carries `Retry-After: 1`, and `IdempotencyOptions` is validated on start. **Never on an `AllowAnonymous()` endpoint** — every unauthenticated caller resolves to the same `"anon"` caller, so one caller's low-entropy key replays another's response; `Integration.Tests/.../IdempotencyWiringTests` fails the build if one appears (`WithIdempotency()` attaches `IdempotentEndpointMetadata` so the endpoint map is inspectable). Put it on authenticated POSTs that must be replay-safe (e.g. CreateTenant). ## Quota enforcement (`Quota/`) diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index c8fc7334e1..e70f72bf83 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -576,6 +576,12 @@ public static class IdempotencyEndpointExtensions public static RouteHandlerBuilder WithIdempotency(this RouteHandlerBuilder builder) { ArgumentNullException.ThrowIfNull(builder); - return builder.AddEndpointFilter(); + + // The marker is what makes the wiring inspectable: an endpoint filter leaves no metadata, so + // without it nothing can assert which endpoints are idempotent — including the rule that an + // anonymous endpoint must not be, since every unauthenticated caller shares one cache bucket. + return builder + .WithMetadata(IdempotentEndpointMetadata.Instance) + .AddEndpointFilter(); } } diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotentEndpointMetadata.cs b/src/BuildingBlocks/Web/Idempotency/IdempotentEndpointMetadata.cs new file mode 100644 index 0000000000..1690b9844b --- /dev/null +++ b/src/BuildingBlocks/Web/Idempotency/IdempotentEndpointMetadata.cs @@ -0,0 +1,14 @@ +namespace FSH.Framework.Web.Idempotency; + +/// +/// Marks an endpoint as idempotent. Applied by WithIdempotency() so the wiring can be asserted +/// from the endpoint map: an endpoint filter itself is invisible in metadata. +/// +public sealed class IdempotentEndpointMetadata +{ + public static IdempotentEndpointMetadata Instance { get; } = new(); + + private IdempotentEndpointMetadata() + { + } +} diff --git a/src/Modules/Identity/Modules.Identity/Features/v1/Users/SelfRegistration/SelfRegisterUserEndpoint.cs b/src/Modules/Identity/Modules.Identity/Features/v1/Users/SelfRegistration/SelfRegisterUserEndpoint.cs index 022936da7c..051a0cc96a 100644 --- a/src/Modules/Identity/Modules.Identity/Features/v1/Users/SelfRegistration/SelfRegisterUserEndpoint.cs +++ b/src/Modules/Identity/Modules.Identity/Features/v1/Users/SelfRegistration/SelfRegisterUserEndpoint.cs @@ -27,8 +27,11 @@ internal static RouteHandlerBuilder MapSelfRegisterUserEndpoint(this IEndpointRo .WithName("SelfRegisterUser") .WithSummary("Self register user") .WithDescription("Allow a user to self-register. Anonymous; tenant identified via the tenant header.") + // Deliberately NOT .WithIdempotency(): the entry key scopes by caller, and every unauthenticated + // caller is the same "anon" caller, so two people registering on one tenant with the same + // low-entropy key would collide — the second would replay the first's 201 and never get an + // account. Retries are already safe here, the unique-email constraint rejects the duplicate. .AllowAnonymous() - .WithIdempotency() .Produces(StatusCodes.Status201Created) .Produces(StatusCodes.Status400BadRequest); } diff --git a/src/Tests/Integration.Tests/Tests/Idempotency/IdempotencyWiringTests.cs b/src/Tests/Integration.Tests/Tests/Idempotency/IdempotencyWiringTests.cs new file mode 100644 index 0000000000..19eaef29d2 --- /dev/null +++ b/src/Tests/Integration.Tests/Tests/Idempotency/IdempotencyWiringTests.cs @@ -0,0 +1,55 @@ +using FSH.Framework.Web.Idempotency; +using Integration.Tests.Infrastructure; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; + +namespace Integration.Tests.Tests.Idempotency; + +[Collection(FshCollectionDefinition.Name)] +public sealed class IdempotencyWiringTests +{ + private readonly FshWebApplicationFactory _factory; + + public IdempotencyWiringTests(FshWebApplicationFactory factory) + { + _factory = factory; + } + + // The entry key scopes by caller, and ResolveCaller returns "anon" for every unauthenticated request. + // So on an anonymous endpoint all callers share one bucket: two people sending the same low-entropy + // key ("1", "retry") against one tenant collide, and the second replays the first's response instead + // of being served. This asserts the shape rather than the single endpoint that had the problem, so + // re-adding .WithIdempotency() to an anonymous endpoint fails here instead of in production. + [Fact] + public void AnonymousEndpoints_Should_NotBeIdempotent() + { + _ = _factory.Server; + + var idempotentAnonymous = IdempotentEndpoints() + .Where(endpoint => endpoint.Metadata.GetMetadata() is not null) + .Select(endpoint => endpoint.RoutePattern.RawText ?? endpoint.DisplayName ?? "") + .ToList(); + + idempotentAnonymous.ShouldBeEmpty( + "Anonymous endpoints share the \"anon\" caller bucket, so an idempotency key from one caller " + + "can replay another caller's response:\n - " + string.Join("\n - ", idempotentAnonymous)); + } + + // Guards the test above against passing vacuously: if WithIdempotency() ever stops attaching the + // marker, the query above returns nothing and would report success over an unchecked endpoint map. + [Fact] + public void IdempotentEndpoints_Should_BeDiscoverableFromTheEndpointMap() + { + _ = _factory.Server; + + IdempotentEndpoints().ShouldNotBeEmpty( + "No endpoint carries IdempotentEndpointMetadata, so nothing can assert idempotency wiring."); + } + + private List IdempotentEndpoints() => + _factory.Services.GetRequiredService() + .Endpoints.OfType() + .Where(endpoint => endpoint.Metadata.GetMetadata() is not null) + .ToList(); +} From b2770010c92cb465564e5cc6675196e70628e86d Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:27:18 -0300 Subject: [PATCH 14/16] test(multitenancy): carry the response body into the theme status assertions UpdateTheme_Should_NotLeakAcrossTenants_When_RootOperatorTargetsTenantA was seen returning 401 instead of 204 twice on a loaded machine, then passed four runs in a row (including two with 14 of 16 cores saturated) and passes in isolation and in CI. A bare status assertion gives nothing to work with: the reason JwtBearer rejected the token is in the ProblemDetails body, which the test discarded. The assertions now report method, URL and body on mismatch. Exercised by expecting the wrong status on purpose: the failure message carries the body. This is diagnosis, not a fix. The cause is still unidentified, and this test can still go red. --- .../Tests/Multitenancy/TenantThemeTests.cs | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/Tests/Integration.Tests/Tests/Multitenancy/TenantThemeTests.cs b/src/Tests/Integration.Tests/Tests/Multitenancy/TenantThemeTests.cs index 837b78631a..1f305f7b5e 100644 --- a/src/Tests/Integration.Tests/Tests/Multitenancy/TenantThemeTests.cs +++ b/src/Tests/Integration.Tests/Tests/Multitenancy/TenantThemeTests.cs @@ -241,7 +241,7 @@ public async Task UpdateTheme_Should_NotLeakAcrossTenants_When_RootOperatorTarge clientA.DefaultRequestHeaders.Authorization = new("Bearer", rootToken.AccessToken); clientA.DefaultRequestHeaders.Add("tenant", _tenantA); var update = await clientA.PutAsJsonAsync(ThemePath, ValidTheme(primary: marker)); - update.StatusCode.ShouldBe(HttpStatusCode.NoContent); + await ShouldHaveStatusAsync(update, HttpStatusCode.NoContent); } // Act — root operator now scopes to tenant B and reads its theme. @@ -251,7 +251,7 @@ public async Task UpdateTheme_Should_NotLeakAcrossTenants_When_RootOperatorTarge var responseB = await clientB.GetAsync(ThemePath); // Assert — tenant B must NOT see tenant A's customization. - responseB.StatusCode.ShouldBe(HttpStatusCode.OK); + await ShouldHaveStatusAsync(responseB, HttpStatusCode.OK); var themeB = await responseB.Content.ReadFromJsonAsync(Json); themeB.ShouldNotBeNull(); themeB.LightPalette.Primary.ShouldNotBe(marker); @@ -375,6 +375,23 @@ private static object ValidTheme( }; } + // The root-operator path here has been seen returning 401 intermittently on a loaded machine, and a + // bare status assertion says nothing about why: JwtBearer puts the validation failure reason in the + // ProblemDetails body (OnChallenge, Development only), so carry the body into the failure message. + // Cause still unidentified; this exists so the next occurrence is diagnosable instead of a mystery. + private static async Task ShouldHaveStatusAsync(HttpResponseMessage response, HttpStatusCode expected) + { + if (response.StatusCode == expected) + { + return; + } + + var body = await response.Content.ReadAsStringAsync(); + response.StatusCode.ShouldBe( + expected, + $"{response.RequestMessage?.Method} {response.RequestMessage?.RequestUri} returned body: {body}"); + } + private async Task GetTokenWithRetryAsync(string email, string password, string tenant, int maxRetries = 30) { Exception? last = null; From c204603783093a2539d654973c702285c0b4a304 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Thu, 13 Aug 2026 06:21:39 -0300 Subject: [PATCH 15/16] build(deps): pin SSH.NET to the patched 2026.0.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Testcontainers packages pull SSH.NET 2025.1.0 transitively, which carries GHSA-q939-rpr3-3284 (CVE-2026-48798, high): ScpClient recursive download writes files outside the target directory. Under TreatWarningsAsErrors that advisory is NU1903 as an error, so `dotnet restore src/FSH.Starter.slnx` fails for the whole solution — Backend CI, CodeQL and the template smoke build all die at restore. Testcontainers 4.11.0 and 4.13.0 both depend on 2025.1.0, so bumping Testcontainers does not clear it. 2026.0.0 is the first patched release, and transitive pinning is already enabled, so this entry alone bumps it — same shape as the MessagePack, Microsoft.OpenApi and SQLitePCLRaw pins next to it. --- src/Directory.Packages.props | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 0d38b28190..7674befa8f 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -143,5 +143,12 @@ AccessViolation). Transitive pinning is enabled, so this entry alone bumps it. Remove once the SignalR backplane package depends on a patched version itself. --> + + \ No newline at end of file From 7c2dcbaf8c470657eae1320164c173fc4323c1b4 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Thu, 13 Aug 2026 08:42:48 -0300 Subject: [PATCH 16/16] fix(web): keep request-derived text out of the idempotency warning CodeQL flagged the `Response.HasStarted` pass-through warning (alert 28, cs/log-forging): it logged `operation`, which folds in `Request.Method`, the resolved route values and the raw request path, so three caller-controlled sources reached a log line. Every other log in the filter already passes `HashKey(...)`, which is why this was the only one. The warning now logs the route pattern read off the endpoint's `RoutePattern`, a literal from the route table, which identifies the endpoint just as well. `operation` is unchanged for the cache key, where the route values have to stay: `PUT /tickets/1` and `PUT /tickets/2` are different operations. --- .../Web/Idempotency/IdempotencyEndpointFilter.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index e70f72bf83..02848e5d62 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -183,9 +183,14 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter // leave idempotency out of it. if (httpContext.Response.HasStarted) { + // The route PATTERN, not the operation used for the cache key: the latter folds in the + // request method, the resolved route values and the raw path, so logging it puts + // caller-controlled text in a log line (CodeQL cs/log-forging). The pattern is a literal + // from the route table and identifies the endpoint just as well for this warning. + var routePattern = (httpContext.GetEndpoint() as RouteEndpoint)?.RoutePattern.RawText ?? "unknown"; logger.LogWarning( - "Idempotent handler for {Operation} started the response itself; nothing captured or stored for key {KeyHash}", - operation, + "Idempotent handler for {RoutePattern} started the response itself; nothing captured or stored for key {KeyHash}", + routePattern, HashKey(idempotencyKey)); // Empty rather than null when the handler returned nothing: a null return makes the