diff --git a/.agents/rules/security.md b/.agents/rules/security.md index b3fb38404b..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, 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 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/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 ed6d686c3f..2a61ee2944 100644 --- a/src/BuildingBlocks/Web/Idempotency/CachedIdempotentResponse.cs +++ b/src/BuildingBlocks/Web/Idempotency/CachedIdempotentResponse.cs @@ -1,18 +1,23 @@ -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. + /// 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 fa73b35c7c..02848e5d62 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -1,14 +1,21 @@ +using System.Collections.Concurrent; 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.Identity.Claims; +using FSH.Framework.Shared.Multitenancy; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.Caching.Distributed; -using Microsoft.Extensions.Caching.Hybrid; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; +using Microsoft.Extensions.Primitives; +using StackExchange.Redis; namespace FSH.Framework.Web.Idempotency; @@ -18,16 +25,46 @@ 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. -/// Using HybridCache with DisableUnderlyingData as a "get-only probe" is a -/// known anti-pattern tracked at dotnet/aspnetcore#57191. +/// 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 +/// 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. +/// 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 { 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"]; + + // 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. + // 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) { ArgumentNullException.ThrowIfNull(context); @@ -45,74 +82,471 @@ 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(); - 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) }; + var tenantId = ResolveTenant(httpContext); + + // 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, $"{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. - var cachedBytes = await distributedCache.GetAsync(cacheKey, httpContext.RequestAborted).ConfigureAwait(false); - if (cachedBytes is not null && cachedBytes.Length > 0) + var cached = await ProbeAsync(distributedCache, cacheKey, logger, idempotencyKey, httpContext.RequestAborted).ConfigureAwait(false); + if (cached is not null) { - var cached = JsonSerializer.Deserialize(cachedBytes, JsonOpts); - 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. 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 = "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. + var raced = await ProbeAsync(distributedCache, cacheKey, logger, idempotencyKey, httpContext.RequestAborted).ConfigureAwait(false); + if (raced 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; - } + return await ReplayAsync(httpContext, raced, idempotencyKey, logger).ConfigureAwait(false); + } - if (cached.Body.Length > 0) - { - await httpContext.Response.Body.WriteAsync(cached.Body, httpContext.RequestAborted).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 + { + // 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); + } + + // 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) + { + // 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 {RoutePattern} started the response itself; nothing captured or stored for key {KeyHash}", + routePattern, + HashKey(idempotencyKey)); - return null; // Response already written + // 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 + // Response.StatusCode at its default and cache the wrapper object, not the wire body. + var captured = await ExecuteAndCaptureAsync(result, httpContext).ConfigureAwait(false); + + httpContext.Response.StatusCode = captured.StatusCode; + if (captured.ContentType is not null) + { + httpContext.Response.ContentType = captured.ContentType; + } + + // 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 CacheResponseAsync(distributedCache, cacheKey, captured, options.DefaultTtl, logger, idempotencyKey).ConfigureAwait(false); + } + + if (captured.Body.Length > 0) + { + 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 + // doesn't serialize a null return and append "null" after the captured payload. + return Results.Empty; + } + finally + { + await ReleaseReservationAsync(multiplexer, reservationKey, reservation, logger, idempotencyKey).ConfigureAwait(false); } + } - // Execute the handler - var result = await next(context).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. + private static async ValueTask CacheResponseAsync( + IDistributedCache distributedCache, + string cacheKey, + CachedIdempotentResponse response, + TimeSpan ttl, + ILogger logger, + string idempotencyKey) + { + try + { + var payload = JsonSerializer.SerializeToUtf8Bytes(response, JsonOpts); - // Cache the response through HybridCache so the tag invalidation path works for purges. + // 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, ILogger logger, string idempotencyKey, CancellationToken ct) + { + byte[]? bytes; try { - var body = result is not null ? JsonSerializer.SerializeToUtf8Bytes(result, JsonOpts) : []; - var responseToCache = new CachedIdempotentResponse + 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; + } + + 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( + HttpContext httpContext, CachedIdempotentResponse cached, string idempotencyKey, ILogger logger) + { + 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; + } + + 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); + } + + // Empty result (not null) so the framework doesn't append a serialized "null". + 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) + { + 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) { - 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, CancellationToken.None).ConfigureAwait(false); + break; + } + + var statusCode = httpContext.Response.StatusCode is > 0 and < 600 + ? httpContext.Response.StatusCode + : StatusCodes.Status200OK; + + 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(); + } + } - var setOptions = new HybridCacheEntryOptions + return new CachedIdempotentResponse { - Expiration = options.DefaultTtl, - LocalCacheExpiration = options.DefaultTtl < TimeSpan.FromMinutes(2) ? options.DefaultTtl : TimeSpan.FromMinutes(2), + StatusCode = statusCode, + // 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, }; - 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) + finally { - logger.LogWarning(ex, "Failed to cache idempotent response for key {KeyHash}", HashKey(idempotencyKey)); + httpContext.Response.Body = originalBody; + httpContext.RequestAborted = originalAborted; + } + } + + 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, 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 — 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 Reservation.Unowned; + } + } + + 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, Reservation reservation, ILogger logger, string idempotencyKey) + { + if (reservation.Token is not { } token) + { + return; } - return result; + if (multiplexer is not null) + { + try + { + 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 + // 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)); + } + + return; + } + + 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 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) @@ -120,6 +554,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 @@ -131,6 +581,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/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/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/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 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/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 new file mode 100644 index 0000000000..fe9507f07e --- /dev/null +++ b/src/Tests/Framework.Tests/Web/IdempotencyEndpointFilterReplayTests.cs @@ -0,0 +1,1499 @@ +using System.Security.Claims; +using System.Text; +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.Http.Features; +using Microsoft.AspNetCore.Http.HttpResults; +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; +using StackExchange.Redis; + +namespace Framework.Tests.Web; + +/// +/// 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 +{ + // 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 }; + + // ─── 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() + { + // 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; + 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."); + // 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 ───── + + [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"); + } + + // ─── 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() + { + // 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(); + + 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."); + } + + // ─── 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] + 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."); + } + + // ─── 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", $"anon: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(); + } + + // ─── 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", $"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", $"anon: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 ProbeHookCache(inner); + racing.SeedOnNextProbe(key => SeedEntryAsync(inner, key, id)); + + 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")))); + + // 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.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()); + 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); + } + + // ─── 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.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); + + private static ServiceProvider BuildProvider(IdempotencyOptions options, IConnectionMultiplexer? multiplexer) + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddDistributedMemoryCache(); + services.AddSingleton>(Options.Create(options)); + if (multiplexer is not null) + { + services.AddSingleton(multiplexer); + } + + 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())); + + // 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) + { + 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(); + mux.GetDatabase(Arg.Any(), Arg.Any()).Returns(db); + return mux; + } + + private static RouteEndpoint RouteEndpointFor(string pattern) => new( + _ => Task.CompletedTask, + RoutePatternFactory.Parse(pattern), + order: 0, + new EndpointMetadataCollection(), + displayName: pattern); + + 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"] = idempotencyKey; + if (responseBody is not null) + { + context.Response.Body = responseBody; + } + + 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); + + /// + /// 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); + } + } + + 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. + /// + 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; + } + } + + /// + /// 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 ProbeHookCache(IDistributedCache inner) : IDistributedCache + { + 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); + var hook = Interlocked.Exchange(ref _onNextProbe, null); + if (hook is not null) + { + await hook(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); + } + + /// + /// 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; + + public override HttpContext HttpContext { get; } + + public override IList Arguments { get; } = new List(); + + public override T GetArgument(int index) => (T)Arguments[index]!; + } +} diff --git a/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs b/src/Tests/Framework.Tests/Web/OptionsDefaultsTests.cs index 013f334895..8db3077d39 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,63 @@ 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. + // 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", "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() + .AddInMemoryCollection([new KeyValuePair($"IdempotencyOptions:{key}", value)]) + .Build(); + var services = new ServiceCollection(); + services.AddSingleton(configuration); + services.AddHeroIdempotency(configuration); + var provider = services.BuildServiceProvider(); + + // 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 — 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] + 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 + provider.GetRequiredService().Validate(); + var options = provider.GetRequiredService>().Value; + + // 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)); + } + #endregion } 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 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(); 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() 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(); +} 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;