Skip to content

fix(web): correct idempotent replay payload and serialize concurrent duplicates - #1333

Closed
marcelo-maciel wants to merge 16 commits into
fullstackhero:mainfrom
marcelo-maciel:fix/idempotency-correctness
Closed

marcelo-maciel wants to merge 16 commits into
fullstackhero:mainfrom
marcelo-maciel:fix/idempotency-correctness

Conversation

@marcelo-maciel

@marcelo-maciel marcelo-maciel commented Jul 11, 2026 •

Copy link
Copy Markdown
Contributor

Fixes the idempotency filter. It started as three defects (audit findings API-01 and CONC-01, plus a replay path that never engaged) and grew after review: re-reading the whole filter rather than the delta surfaced the rest, listed at the bottom.

API-03 (root): replay never engaged, even in production

The write went through HybridCache.SetAsync while the probe read IDistributedCache by the raw key. HybridCache keys its L2 entries under its own scheme, so the raw-key probe never found the entry and idempotency replay silently never engaged, not just in tests. The repo's ChatSendMessageTests replay test was skipped with this exact symptom attributed to a "test-env cache split"; un-skipping it proves it was the filter.

Fix: write to the same IDistributedCache, key and serializer the probe uses. Idempotency entries are short-lived (TTL) and their HybridCache tag-purge path was unused, so dropping HybridCache here loses nothing.

API-01: replay served the wrong shape and status

The filter cached SerializeToUtf8Bytes(result) where result is the wrapped IResult (Ok<T>/Created<T>), storing {"value":{...},"statusCode":200} instead of the wire DTO, and read Response.StatusCode before the IResult executed, so a 201 replayed as 200. The handler result is now executed into a buffer to capture the real wire body and status.

CONC-01: concurrent duplicates both executed

probe -> execute -> write had no atomic reservation, so two concurrent same-key requests both ran the handler. An atomic in-flight reservation now serializes duplicates: Redis SET NX when an IConnectionMultiplexer is registered (the multi-instance case, where this stack already requires Redis for the shared Data Protection key ring), an in-process set otherwise (single instance). A duplicate in flight gets 409 Conflict. Redis stays optional.

What review added

  • The response is stored before the body reaches the client and on CancellationToken.None, so the timeout-then-retry that idempotency exists to absorb replays instead of re-executing.
  • Replay carries the allow-listed headers Location and ETag, set while the IResult executes and previously dropped.
  • Only 2xx is stored. A failure is not a record of a committed side effect, and caching it locked the caller out of that key for the full TTL.
  • The key is scoped to the resolved tenant, the caller, the HTTP method, the route pattern and the resolved route values. Tenant alone let one key replay another endpoint's response, and a root operator acting on two tenants shared one bucket.
  • The reservation is sound under the races it exists for: re-probe once the lock is held, lock: prefix instead of a suffix on the entry key, compare-and-delete release against the reservation's own token, and the in-process fallback expires on ReservationTtl.
  • 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. The trade-off, that a disconnect no longer aborts an idempotent handler, is called out in the review thread and in the docs.
  • Probe, reservation and store all fail open; a handler that writes to Response itself is passed through; the 409 carries Retry-After: 1; IdempotencyOptions is validated on start.

Tests

IdempotencyEndpointFilterReplayTests (33 cases) covers each branch: replay preserves 201 and the plain DTO body, allow-listed headers only, concurrent duplicates execute once with the loser getting 409, probe and store failures fall through to execution, reservation release under a thrown handler. OptionsDefaultsTests pins the startup validation. ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused is un-skipped and passes against real Redis. Every fix was mutation-tested: 22 mutations, each reverting one fix, each required to turn its pinning test red, 22 of 22 red.

Docs

fullstackhero/docs#240, including the changelog entry and the documented ceilings (buffered response has no size cap, so not for streaming endpoints; no lease renewal; cross-instance dedup depends on the cache being on Valkey).

Unrelated: one dependency pin, so CI can run at all

build(deps): pin SSH.NET to the patched 2026.0.0 is not part of this fix. The Testcontainers packages pull SSH.NET 2025.1.0 transitively, and that version now carries GHSA-q939-rpr3-3284 (CVE-2026-48798, high). Under TreatWarningsAsErrors the advisory surfaces as NU1903 as an error, so dotnet restore src/FSH.Starter.slnx fails on main and on every open PR — Backend CI, CodeQL and the template smoke build all die before a single test runs. Testcontainers 4.11.0 and 4.13.0 both depend on 2025.1.0, so bumping Testcontainers does not clear it; 2026.0.0 is the first patched release. Transitive pinning is already enabled, so the one entry is enough, in the same block and the same shape as the MessagePack, Microsoft.OpenApi and SQLitePCLRaw pins already there. Happy to split it out if you would rather land it on its own.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@iammukeshm iammukeshm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The three fixes are the right idea — same-store/same-key symmetry, buffer-and-capture for the true wire body+status, and an atomic SET NX / TryAdd reservation with re-probe-on-race. Tenant isolation is preserved (reservation key derives from the tenant-scoped cache key). Two things to fix before merge:

🔴 HIGH — reservation TTL is the 24h response TTL; a crash mid-request strands the lock for a day. The in-flight reservation uses options.DefaultTtl (24h). If the process is killed between reserving and the finally release (OOM, pod eviction, SIGKILL), the Redis key survives 24h and every retry of that idempotency key 409s for a full day. Use a short, request-timeout-scale TTL for the in-flight reservation (seconds/minutes), decoupled from the 24h stored-response TTL.

⚠️ MEDIUM — reservation now fails closed on a Redis blip. StringSetAsync is treated as authoritative, so a transient Redis error throws and 500s the request. On main idempotency degraded gracefully (convenience, not correctness). Wrap the reserve/release in try/catch and fail open (proceed) to match the existing stance used for the response write.

nit — ReleaseReservationAsync's KeyDeleteAsync is unguarded; a Redis fault at release throws out of finally and (with the TTL issue) strands the lock. Make it best-effort too.

⚠️ Also touches protected src/BuildingBlocks/Web (Golden Rule #4) — needs explicit maintainer sign-off.

Tests are good (replay shape + concurrent-once/409, integration un-skipped); note the concurrent path only exercises the in-process branch, not the Redis NX branch.

@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Addressed in 08b34dcc.

🔴 HIGH — reservation TTL. Split out IdempotencyOptions.ReservationTtl (default 1m), decoupled from the 24h DefaultTtl. The reservation now keys to the short TTL, so a SIGKILL between reserve and the finally-release frees the lock in ~1m instead of stranding it for a day. Documented the constraint on the option: it must outlast the longest handler runtime or a concurrent duplicate can slip through.

⚠️ MEDIUM — fail closed on Redis blip. TryReserveAsync now wraps StringSetAsync in try/catch (ex is not OperationCanceledException) and fails open (proceeds), logging a warning — same best-effort stance as the response write. A transient Redis error no longer 500s the request; the response cache still dedups later retries.

nit — unguarded release. ReleaseReservationAsync's KeyDeleteAsync is now best-effort under the same filtered catch, so a Redis fault can't throw out of the finally. The short ReservationTtl expires a missed delete anyway.

Redis NX branch coverage. Added two unit tests over a substituted IConnectionMultiplexer: one asserts the reservation TTL is ReservationTtl and not DefaultTtl (guards the HIGH fix from regressing), the other faults both StringSetAsync and KeyDeleteAsync and asserts the handler still runs without throwing. Framework suite green (126/126).

Golden Rule #4 — src/BuildingBlocks/Web. Acknowledged; this needs your maintainer sign-off. The change is scoped to the Idempotency/ filter + options.

@iammukeshm iammukeshm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The three blockers from my last review are properly fixed — ReservationTtl is decoupled and documented, reserve and release both fail open with the reasoning written down, and the release can no longer throw out of finally. The _ = ct discard is a bit odd but harmless.

Re-reading the whole filter rather than just the delta surfaced two things the previous pass missed. One of them defeats the feature's core promise, so requesting changes.

🔴 HIGH — the response cache write is cancelled by client disconnect, so the handler re-executes on retry

await distributedCache.SetAsync(
    cacheKey, payload,
    new DistributedCacheEntryOptions { ... },
    httpContext.RequestAborted).ConfigureAwait(false);   // <-- here

Walk the sequence that idempotency exists to defend against:

  1. Client POSTs with an Idempotency-Key. Handler runs, side effect commits.
  2. Client times out and hangs up. RequestAborted fires.
  3. The SetAsync is cancelled — nothing is cached. OperationCanceledException is deliberately excluded from the best-effort catch, so it propagates.
  4. finally releases the reservation.
  5. Client retries with the same key. Probe misses, reservation is free, handler executes a second time.

Client-timeout-then-retry is not an edge case — it's the single most common way a duplicate request is generated, and it's the exact scenario the feature is for. The stored response is the durable record that the side effect already happened; its write must outlive the request that produced it.

Use CancellationToken.None for that SetAsync. The body write to the client on the line above should keep RequestAborted (no point writing to a dead socket), but the cache write must not.

While you're there: the catch (Exception ex) when (ex is not OperationCanceledException) around it will no longer skip cancellation once the token is None, which is what you want — a genuine cache failure stays a logged warning.

⚠️ MEDIUM — replay drops response headers, so a replayed 201 has no Location

ExecuteAndCaptureAsync captures status code, content type and body — nothing else. But executing an IResult is precisely when headers get set: TypedResults.Created(uri, value) writes Location. First call returns 201 + Location; the replay returns 201 with no Location. A client that follows the header on create works the first time and breaks on retry, which is the worst failure shape — it only shows up under the retry conditions nobody tests.

Capture the response headers into CachedIdempotentResponse and replay them. Filter to a safe set rather than replaying everything (skip Date, Server, Transfer-Encoding, Content-Length, and anything the host will set itself) — Location and ETag are the ones that carry meaning here.

note — non-2xx responses are now cached for the full 24h

This is pre-existing behaviour, not something you introduced, but this PR makes it bite. Previously the cached status was always the default 200 (the bug you're fixing), so a replayed error was already wrong in an obvious way. Now capture is faithful: a handler that returns 409, 429 or a 500-shaped IResult gets that status stored and replayed for DefaultTtl — 24 hours. A caller that hits a transient downstream failure is then locked out of retrying that key for a day.

Not blocking, because it's a change in an existing behaviour rather than a regression, but the right fix is small: only cache statusCode is >= 200 and < 300. If you'd rather do it here, I'll take it; if not, please open an issue so it doesn't get lost.

on the design, which is sound

Same-store/same-key symmetry is the correct diagnosis — a HybridCache write keying L2 under its own scheme while a raw-key IDistributedCache probe reads plain keys is exactly why replay silently never engaged, and why the integration test was skipped rather than failing. Buffer-and-capture is the right way to get the true wire shape, and returning Results.Empty rather than null to stop the framework appending a serialised null is a detail that would have caused a confusing bug.

Tenant isolation holds: the reservation key derives from the already tenant-scoped cache key.

Agreed that the in-process fallback is acceptable given multi-instance deployments in this stack already run Redis — the comment justifying it is the right way to record that assumption. As noted last time, the concurrency test only exercises the in-process branch; that's a reasonable limit for a unit test and I'm not asking for a Redis-backed one.

BuildingBlocks sign-off (Golden Rule #4)

Granted in principle for Web/Idempotency — the change is contained and the feature was measurably broken. Merging once the RequestAborted fix and the header capture land.

…duplicates

Two defects in IdempotencyEndpointFilter:

API-01 — the filter cached JsonSerializer.SerializeToUtf8Bytes(result) where result
is the wrapped IResult (Ok<T>/Created<T>), so it stored {"value":...,"statusCode":200}
instead of the wire DTO, and it read Response.StatusCode before the IResult executed,
so a 201 Created replayed as 200. The handler result is now executed into a buffer to
capture the real wire body + status, which is what gets served and cached.

CONC-01 — probe->execute->write had no atomic reservation, so two concurrent requests
with the same key both missed the probe and both executed the handler. An atomic in-flight
reservation now serializes duplicates: Redis SET NX when an IConnectionMultiplexer is
registered (the multi-instance case — this stack already requires Redis there for the
shared Data Protection key ring), an in-process set otherwise (single instance). A
duplicate that arrives while the original is still running gets 409 Conflict.

Redis stays optional: without it the app falls back to the in-memory reservation, correct
for a single instance where a cross-container race cannot occur.
…ore)

The write went through HybridCache.SetAsync while the probe read IDistributedCache by
the raw key. HybridCache keys its L2 entries under its own scheme, so the probe never
found the entry and replay silently never engaged — even in production. Proven by
un-skipping ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused,
which now passes.

Write to the same IDistributedCache, key and serializer the probe uses. Idempotency
entries are short-lived (TTL) and their HybridCache tag-purge path was unused, so dropping
HybridCache here loses nothing.
Address review on #1333:

- Reservation used the 24h response TTL, so a crash between reserving
  and the finally-release stranded the Redis lock for a day (every retry
  409s). Add IdempotencyOptions.ReservationTtl (default 1m), decoupled
  from DefaultTtl.
- Reserve now fails open on a transient Redis error instead of 500ing
  the request, matching the best-effort stance of the response write.
- Guard the release KeyDeleteAsync so a Redis fault can't throw out of
  the finally.

Tests: reservation uses ReservationTtl not DefaultTtl; a faulting Redis
on reserve/release proceeds without throwing (exercises the Redis NX
branch the prior tests skipped).
Three defects surfaced by re-reading the whole filter rather than the delta.

The response store was tied to the client's connection, so the retry that
idempotency exists to serve re-executed the handler. Two paths caused it, not
one: the body write to the client ran before the store, so a closed socket threw
and skipped it entirely; and the capture itself ran under RequestAborted, where
WriteAsJsonAsync swallows the cancellation and hands back an EMPTY body — which
was then cached and replayed as a 200 for the full 24h TTL. The capture now runs
with the abort token detached (it writes to an in-memory buffer, never the
socket), the store runs before the client write and on CancellationToken.None,
and only then does the body go out.

Replay dropped every response header, so a replayed 201 arrived without
Location: a client that follows the header worked on the first call and broke on
the retry. The captured response now carries an allow-listed set (Location,
ETag) and replays it. Transport and host-owned headers stay out — a stale
Content-Length would corrupt the response.

Non-2xx is no longer stored. Faithful status capture made the pre-existing
behaviour bite: a transient downstream failure locked the caller out of that key
for 24h. A failure is not a record of a committed side effect.

CachedIdempotentResponse is no longer a HybridCache type, so its
[ImmutableObject(true)] contract (and the CachedTypeContractTests entry
asserting it) described a store this filter stopped using. Both dropped. The new
Headers property defaults to empty so entries written before it deserialize.

Tests: replayed 201 carries Location; a first call whose client disconnects
still replays the real DTO body; a non-2xx first response lets the retry run.
All three fail on the previous commit and pass here.
@marcelo-maciel
marcelo-maciel force-pushed the fix/idempotency-correctness branch from 08b34dc to f7c8c32 Compare August 12, 2026 07:57
…t alone

The entry was keyed on tenant + caller key, with nothing identifying the
operation. One key reused against a second idempotent endpoint replayed the
first endpoint's response and the second request silently never ran. Thirty-one
endpoints across eight modules share that namespace, and one of them
(self-registration) is anonymous: it resolves no tenant claim, so every caller
of it lands in the same "global" bucket. This was latent only for as long as
replay never engaged — the fix that makes replay work is what would have put it
on the wire. The key now folds in the HTTP method and the route pattern.

Two smaller things in the same area. The 409 for an in-flight duplicate and the
400 for an over-long key emitted a bare JSON string, where every other error
these endpoints produce is RFC 9457 ProblemDetails; both now match. And an
unreadable cache entry (written by another version, or another writer at the
same key) let JsonException escape as a 500 — that path only became reachable
once replay started engaging at all. It now degrades to a miss and logs.

ReleaseReservationAsync also swallows cancellation now, not just faults: it runs
in a finally after the response body has already gone to the client, so anything
thrown there can only reset the connection on a request that succeeded.

Tests: a key reused across two route patterns runs the second handler; an
unreadable entry runs the handler (with a valid entry seeded at the same key
first, so the assertion can't pass as a plain cache miss); a 204 replays without
a fabricated content type. Each fails on a mutated implementation.
The reservation guarded the handler against concurrent duplicates but four
holes let one through anyway, or locked a caller out of a key:

- The entry was keyed on the caller's `tenant` claim. A root operator scoping
  requests to different tenants shares one "root" bucket, so one key reused
  across two targets replays the first tenant's body to the second. Key off the
  resolved tenant context instead — the one BaseDbContext scopes the side effect
  to — with the claim as the fallback for a JWT-only request (Finbuckle's claim
  strategy runs pre-authentication and resolves nothing for those). The raw
  `tenant` header is deliberately not a fallback: an unresolved header is one
  Finbuckle refused, and an unvalidated value has no business in a shared key.

- The cache was probed once, before the reservation. The original request can
  store its response and release the lock inside that window; the duplicate then
  takes the free lock and executes the handler again. Probe once more with the
  lock held.

- The lock was a `:inflight` suffix on the entry key, so a caller key ending in
  that suffix put its 24h entry exactly where another key's lock goes — every
  later request with that key 409s for the full response TTL. Give the lock its
  own prefix.

- Release was an unconditional delete. A request that failed open on a Redis
  blip, or one whose reservation had already expired, freed a lock another
  request was holding. Release via compare-and-delete against the token the
  reservation was taken with; failing open carries no token and deletes nothing.

The in-process fallback also gains the TTL takeover the Redis branch gets for
free: without it a handler that never returns strands the key until the process
restarts and every retry 409s forever.

Each fix is pinned by a test that was verified to fail when the fix is reverted.
Follow-up from adversarial passes over the whole filter. Each item below is
pinned by a test verified to fail when the fix is reverted.

- The handler ran under the client's abort token. A disconnect after the side
  effect committed cancelled the next await inside the handler (an EF read, an
  outbox write, a Mediator behaviour), so the filter had nothing to store and
  the client's retry re-executed the side effect — the duplicate this filter
  exists to absorb. The handler now runs with the token detached; the trade is
  that a disconnect no longer aborts an idempotent handler.

- The probe was the one link that hard-failed. Reserve and store both degrade to
  a warning when the cache is down, so a `RedisConnectionException` on the probe
  took every idempotent endpoint down for exactly the clients that send a key.
  It now fails open as a miss.

- A handler that writes the response itself had already started it, so the buffer
  swap captured nothing and setting the captured status threw. That case now
  passes through untouched and stores nothing.

- The key covered the route pattern but not its values, so `PUT /tickets/1` and
  `PUT /tickets/2` were one operation: the second replayed the first ticket's
  response and never ran. It now folds in the resolved route values.

- The key was not scoped to the caller, so two users of one tenant reusing a
  low-entropy key on the same endpoint received each other's response bodies
  while their own request was silently suppressed.

- The 409 said "retry shortly" with no `Retry-After`. It now sends 1 second: the
  original is normally about to store its response, and the reservation TTL is
  the worst case, not the hint.

Also: options are validated at startup like every other block here (a zero TTL
failed silently inside the best-effort write, so nothing was ever stored),
`CacheKeys.Tags.Idempotency` no longer claims to be applied, and the cached
headers dictionary documents that its comparer does not survive deserialization.
Ceilings that stay: no size cap on the buffered response (do not put
`.WithIdempotency()` on a streaming endpoint), no lease renewal, and a lock whose
Redis may not be the cache's Redis — all three now carry `ponytail:` notes.
Comment thread src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs Fixed
A test-quality pass over the suite found assertions that survive the mutation
they exist to catch, and branches with no test at all. Each case below now fails
when the behaviour it pins is reverted.

Assertions that could not fail:
- The Lua release script was matched with Arg.Any<string>() while the fake
  hardcoded compare-and-delete, so swapping the script for an unconditional
  `del` kept the suite green — the exact bug the script's comment warns about.
  The script text is asserted now.
- `(result as IStatusCodeHttpResult)?.StatusCode.ShouldBe(409)` skips the whole
  assertion for a result that isn't one, which is precisely the mutation it
  guards. Cast instead.
- The concurrency test relied on the default one-minute ReservationTtl outliving
  the test; a CI freeze past it hands the key over and fails a correct filter.
  It pins the TTL explicitly.

Branches with no coverage: a handler that throws (the release has to stay in the
finally, or one exception strands the key until the TTL), the tenant-claim
fallback (collapsing it to "global" puts every JWT-only caller in one bucket and
replays across tenants), the refused duplicate's re-probe, the restrictive half
of the header allow-list (Set-Cookie must not come back on a replay), the
best-effort store, a faulting release, the no-header pass-through, the
MaxKeyLength rejection, and an entry stored without the Headers member — the
shape a previous version wrote, which has to keep replaying through a rolling
deploy.

Also drops a stale comment claiming body capture is out of reach; this PR is
what made it possible, and the integration suite asserts it end to end.
…mapping

The idempotency filter scopes its cache key by ClaimsPrincipal.GetUserId(), which
reads ClaimTypes.NameIdentifier only. Until now nothing proved that claim type is
present after JwtBearer validates a real issued token: every existing test built the
principal by hand, so caller scoping could have been inert in production (every
caller collapsing into one bucket) with a green suite.

Round-trips a token from TokenService through JsonWebTokenHandler configured with
JwtBearerOptions' own MapInboundClaims default, then asserts GetUserId() resolves.
Verified with the claim removed from the token as well: the short-form `sub` maps to
it, so both shapes IdentityService emits resolve.
…changes

The rule described the reservation work from the previous round but not what
landed after it, so an agent reading it would still believe the probe hard-fails
and the key ignores route values and the caller. Adds the abort-token detachment
together with the constraint it implies (no streaming endpoints), the HasStarted
pass-through, Retry-After and the startup validation.
@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Both blockers landed in 52365f34: the store runs on CancellationToken.None before the body reaches the client, and Location/ETag are captured and replayed off an allow-list. Your non-blocking note is in too, only 2xx is stored.

Then I did what you did, re-read the whole filter instead of the delta, and that turned into three more commits (53ef779b, b18e541c, 08fcadef, plus two test/doc commits). Please re-review from 52365f34, not from the diff you already approved. One of those changes is a semantic change to every endpoint carrying .WithIdempotency(), and I would rather you reject it now than discover it later.

The one that needs your call: the handler no longer sees client disconnect

The SetAsync fix you asked for closes the cancellation at the store. It does not close it inside the handler:

1. Client POSTs with a key. Handler commits the side effect.
2. Client hangs up. RequestAborted fires.
3. The handler's NEXT await, an EF read, an outbox write, a Mediator behaviour, throws OCE.
4. next(context) never returns a result. There is nothing to store, on any token.
5. Retry with the same key: probe misses, handler runs, side effect happens twice.

Same duplicate as before, one link upstream. So the handler now runs with the token detached and the original restored right after:

var originalAborted = httpContext.RequestAborted;
try
{
    httpContext.RequestAborted = CancellationToken.None;
    result = await next(context).ConfigureAwait(false);
}
finally
{
    httpContext.RequestAborted = originalAborted;
}

The cost is real and it applies to all 29 idempotent endpoints: a client disconnect no longer aborts the handler. An abusive client can open requests and hang up, and the work still runs to completion. That is the trade-off I am asking you to accept or veto.

The alternative I rejected: catch OperationCanceledException around next(context) and store anyway. There is nothing to store, the exception arrives instead of the result, so it buys nothing. Third option if the trade-off is unacceptable: leave the token attached, accept that a disconnect mid-handler means the retry re-executes, and document it as a known hole. I would take the current behaviour, but this is a framework other people ship, so it is your call, not mine.

Disclosures, since they are outside what you looked at

  • The key shape changed again. It now folds in the resolved route values and the caller (GetUserId(), "anon" when unauthenticated) on top of tenant + method + route pattern. Two consequences: PUT /tickets/1 and PUT /tickets/2 stopped being one operation, and two users of one tenant reusing a low-entropy key stopped seeing each other's response bodies. It also means entries written before the deploy are unreachable. They expire on their own TTL, and a retry that lands inside the rollout window re-executes. Harmless today because replay never engaged, worth a line in the release notes.
  • I touched src/BuildingBlocks/Caching/CacheKeys.cs, one XML comment, no code. Your sign-off was for Web/Idempotency/, so this is outside it. The doc said the entry is written with Tags.Idempotency for bulk invalidation; this PR is what stopped applying that tag (IDistributedCache has no tags), so the comment became false because of this change. I would rather fix it here than leave the lie in a shared file, but say the word and I will revert the line.
  • Caller scoping is not inert. GetUserId() reads ClaimTypes.NameIdentifier only, and until f3d66c56 nothing proved that claim survives bearer validation of a real token. Every test built the principal by hand, so the scoping could have collapsed every caller into one bucket with a green suite. There is now a round-trip test through JsonWebTokenHandler configured with JwtBearerOptions' own MapInboundClaims default. I verified both shapes IdentityService emits: with the explicit ClaimTypes.NameIdentifier claim removed, the short-form sub maps to it and GetUserId() still resolves.

The rest of what landed

Reservation soundness (53ef779b): the entry is keyed on the resolved tenant rather than the tenant claim, and never on the raw header, which is unvalidated when Finbuckle did not resolve it. The cache is re-probed once the lock is held, because the original request can store and release inside the probe-to-reserve window. The lock lives under a lock: prefix instead of a :inflight suffix on the entry key: a caller key ending in :inflight could otherwise park a 24 h entry exactly where another key's lock goes and 409 that key for a day. Release is a compare-and-delete against the reservation's own token, so a request that failed open, or whose lease expired, cannot free a lock someone else holds. The in-process fallback expires on ReservationTtl like the Redis branch instead of stranding the key until restart.

Remaining holes (b18e541c): the probe was the one link that still hard-failed, so a Redis blip returned 500 on every idempotent endpoint for exactly the clients that send a key. It fails open as a miss now. A handler that writes to Response itself is passed through instead of having an empty capture stored and its status set after the response started. The 409 carries Retry-After: 1. And IdempotencyOptions is validated on start: a zero DefaultTtl used to throw inside the best-effort store, log a warning and carry on, so nothing was ever cached.

08fcadef is coverage only, and it is the commit I trust least on its own merits, so I gated it. Every fix above was mutation-tested: 22 mutations, each reverting one fix, each required to turn its pinning test red. 22 of 22 red, filter file restored byte for byte (sha256 1b167d75...). Two tests came back green under mutation on the first run and were rewritten: one exercised the in-process branch when the collision only exists in Redis, the other matched ScriptEvaluateAsync(Arg.Any<string>(), ...) while the fake hardcoded the Lua semantics. Both "covered" bugs that stayed reintroducible.

Deliberately not fixed, documented as ceilings instead

Three ponytail: comments in the filter rather than code, because each is a design decision I do not want to make unilaterally: no size cap on the buffered response, so do not put .WithIdempotency() on a streaming endpoint; no lease renewal while the handler runs, so a handler slower than ReservationTtl lets a duplicate through; and the lock's multiplexer may not be the cache's Redis, so cross-instance dedup depends on the cache being on Valkey. Also not done, each a separate PR if you want it: no request-body fingerprint, so same key with a different body replays the first response instead of returning 422, and Idempotency-Replayed is not in the CORS exposed-headers list, so browser clients cannot read it.

Docs: fullstackhero/docs#240, including the changelog entry and the ceilings above.

Verification on 5b33004a: 1087 unit tests pass locally across every test project (Integration.Middleware.Tests' 5 need Docker, which is not up on this box; they pass in CI), and CI is green on the same commit including Integration Tests and both scaffold builds.

…lazily

The startup-validation tests resolved IOptions<IdempotencyOptions>.Value, which
validates on first access with or without .ValidateOnStart(). Deleting that call
left all six green while moving the failure from boot to the first request that
carries an Idempotency-Key — a suite that could not see the difference between
"rejected at startup" and "rejected once, in production, per process".

They now go through IStartupValidator, which is what .ValidateOnStart() registers
and what the host runs before serving traffic. Verified: with .ValidateOnStart()
removed, 6 of the 10 tests fail.
@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Two more things from an adversarial pass over the tests and the tenancy/keyspace, both verified at source. One is fixed in e6b0bb7e; the other is a decision I would rather you make.

Fixed: the startup-validation tests could not see whether validation happened at startup

OptionsDefaultsTests resolved IOptions<IdempotencyOptions>.Value, which validates on first access with or without .ValidateOnStart(). So the tests passed either way, and deleting .ValidateOnStart() would have moved the failure from boot to the first request carrying an Idempotency-Key, once per process, in production. They now go through IStartupValidator, which is what .ValidateOnStart() registers and what the host runs before serving traffic. Measured: with .ValidateOnStart() removed, 6 of the 10 tests fail; before the change, 0 did.

That is the second vacuous gate this PR has produced, which is why I mutation-test rather than trust a green suite.

Your call: /self-register is anonymous, so every caller shares the "anon" bucket

b18e541c folded the caller into the key, which closed the same-tenant/two-users collision for authenticated endpoints. It does not close it for anonymous ones, and there is exactly one: SelfRegisterUserEndpoint is .AllowAnonymous().WithIdempotency() with the tenant coming from the tenant header.

Concretely, two different people self-registering on tenant B, each sending a low-entropy key ("1", "retry"), build the identical entry key:

idem:t:<B>:anon:POST:/self-register:<K>

The second one replays the first registrant's cached 201 with the first registrant's RegisterUserResponse(UserId), and their account is silently never created. Same shape lets any anonymous caller pin a key in a tenant's anon bucket for the full 24 h TTL. It stays inside one tenant (the tenant is a hard prefix), so nothing leaks across tenants, and it was unreachable before this PR because replay never engaged. Making replay work is what puts it on the wire.

Three ways to close it, smallest blast radius first, and I do not want to pick for you:

  1. Drop .WithIdempotency() from /self-register (one line, in the Identity module). Registration is already guarded by the unique-email constraint, so a genuine retry surfaces as a 400 rather than a duplicate account. Cheapest, and it takes the only anonymous idempotent endpoint out of the picture.
  2. Fold a hash of the request body into the key. Closes it generally, for authenticated callers too, at the cost of buffering the request body on every idempotent endpoint.
  3. Stripe semantics: same key + different body returns 422. The correct end state, the largest change, and the one I would rather send as its own PR with its own tests.

I would take (1) in this PR and open an issue for (3). Say which and I will land it.

Informational, pre-existing, not introduced here

ResolveTenant's claim fallback keys under the JWT tenant claim, while the side effect keys off the resolved Finbuckle context only. Those diverge for a JWT-only request with no tenant header, because UseMultiTenant() runs before UseAuthentication() and the claim strategy no-ops (the comment at MultitenancyModule.cs:104 says so). Such a request keys the entry by claim while the handler's TenantInfo is null, so the save fails: non-2xx, never cached, no replay and nothing leaked. Flagging it only so the fallback is not read as a hole, and because it is the reason the fallback exists at all rather than defaulting straight to "global".

…sage

Asserting only OptionsValidationException let a clause be deleted with every row
still green: a zero DefaultTtl also trips "ReservationTtl must not exceed
DefaultTtl", so the row aimed at DefaultTtl passed on the wrong clause. Each row
now names the failure it expects.

Per-clause mutation run: removing any one of the five clauses fails exactly the
one row that targets it; removing .ValidateOnStart() fails all six cases, since
IStartupValidator is then unregistered.
@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Addendum on the options tests, because the IStartupValidator fix exposed a second weakness in the same file.

Asserting only OptionsValidationException was not enough: a zero DefaultTtl also trips the ReservationTtl must not exceed DefaultTtl clause, so the row aimed at DefaultTtl passed on the wrong clause, and that clause could be deleted with all five rows still green. Each row now names the failure message it expects (75eb3b9b).

Per-clause mutation run, so the claim is measured rather than asserted:

header-name           RED on exactly 1 row
default-ttl           RED on exactly 1 row
reservation-positive  RED on exactly 1 row
reservation-bound     RED on exactly 1 row
max-key-length        RED on exactly 1 row
validate-on-start     RED on all 6 cases (IStartupValidator is unregistered)
Extensions.cs restored byte for byte

CI is green on 75eb3b9b.

…ymous endpoints

The cache key scopes by caller, and ResolveCaller returns "anon" for every
unauthenticated request, so on an anonymous endpoint all callers share one bucket.
Two people registering on the same tenant with the same low-entropy key ("1",
"retry") built the identical key: the second replayed the first registrant's 201
with the first registrant's UserId, and their own account was silently never
created. /self-register was the only anonymous idempotent endpoint. It was
unreachable until replay started engaging.

A retry there is already safe without the filter — the unique-email constraint
rejects the duplicate — so the endpoint drops .WithIdempotency() rather than
gaining a body fingerprint.

Deleting one call would leave nothing stopping the next one, so WithIdempotency()
now attaches IdempotentEndpointMetadata: an endpoint filter is invisible in
metadata, and the marker makes the wiring inspectable. IdempotencyWiringTests
walks the endpoint map and fails when an AllowAnonymous() endpoint carries it,
plus a second test that fails if the marker stops being attached, so the first
cannot pass over an empty set.
@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Went ahead with option (1) on the anonymous-caller collision, in 430122aa. Revert it if you prefer (2) or (3) instead.

/self-register drops .WithIdempotency(). A retry there is already safe without the filter, since the unique-email constraint rejects the duplicate, and it was the only anonymous idempotent endpoint.

Deleting one call leaves nothing stopping the next one, so the rule is now enforced rather than documented. WithIdempotency() attaches an IdempotentEndpointMetadata marker, because an endpoint filter is invisible in endpoint metadata and nothing could otherwise assert which endpoints are idempotent. IdempotencyWiringTests walks the endpoint map and fails when an AllowAnonymous() endpoint carries the marker, in the same shape as the existing AuthRateLimitWiringTests. A second test fails if the marker stops being attached, so the first cannot pass over an empty set.

Both gates are mutation-verified locally against Testcontainers:

put .WithIdempotency() back on /self-register  -> 1 of 2 red (the anonymous-endpoint test)
stop attaching the marker in WithIdempotency() -> 1 of 2 red (the discoverability test)
both files restored byte for byte

The docs page framed this as a caveat to work around with fresh UUIDs; it is corrected to state the rule, and the changelog entry is in (fullstackhero/docs#240). .agents/rules/security.md says it too, so an agent adding an endpoint reads it there.

Still yours to decide: the RequestAborted detachment from my earlier comment, and whether a request-body fingerprint (option 3, Stripe's 422 on key reuse with a different body) should be its own PR. This change does not close that one, it only removes the case where a low-entropy key from a different caller collides.

Unrelated and pre-existing, flagging because it bit me locally: dotnet build src/FSH.Starter.slnx now fails NU1903 on SSH.NET 2025.1.0 (a transitive dependency, GHSA-q939-rpr3-3284) for Integration.Tests and Integration.Middleware.Tests. It is not from this PR and CI is green on it, so I left it alone rather than bumping a package here.

…ertions

UpdateTheme_Should_NotLeakAcrossTenants_When_RootOperatorTargetsTenantA was seen
returning 401 instead of 204 twice on a loaded machine, then passed four runs in a
row (including two with 14 of 16 cores saturated) and passes in isolation and in
CI. A bare status assertion gives nothing to work with: the reason JwtBearer
rejected the token is in the ProblemDetails body, which the test discarded.

The assertions now report method, URL and body on mismatch. Exercised by
expecting the wrong status on purpose: the failure message carries the body.

This is diagnosis, not a fix. The cause is still unidentified, and this test can
still go red.
The Testcontainers packages pull SSH.NET 2025.1.0 transitively, which carries
GHSA-q939-rpr3-3284 (CVE-2026-48798, high): ScpClient recursive download writes
files outside the target directory. Under TreatWarningsAsErrors that advisory is
NU1903 as an error, so `dotnet restore src/FSH.Starter.slnx` fails for the whole
solution — Backend CI, CodeQL and the template smoke build all die at restore.

Testcontainers 4.11.0 and 4.13.0 both depend on 2025.1.0, so bumping
Testcontainers does not clear it. 2026.0.0 is the first patched release, and
transitive pinning is already enabled, so this entry alone bumps it — same shape
as the MessagePack, Microsoft.OpenApi and SQLitePCLRaw pins next to it.
CodeQL flagged the `Response.HasStarted` pass-through warning (alert 28,
cs/log-forging): it logged `operation`, which folds in `Request.Method`, the
resolved route values and the raw request path, so three caller-controlled
sources reached a log line. Every other log in the filter already passes
`HashKey(...)`, which is why this was the only one.

The warning now logs the route pattern read off the endpoint's `RoutePattern`,
a literal from the route table, which identifies the endpoint just as well.
`operation` is unchanged for the cache key, where the route values have to stay:
`PUT /tickets/1` and `PUT /tickets/2` are different operations.
@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Three commits landed after my last comment and I never wrote them up, which is my fault — the first one matters most, because the alert it fixes still reads as open.

CodeQL alert 28 is fixed (7c2dcbaf)

The bot's cs/log-forging finding was correct. The Response.HasStarted pass-through warning logged operation, and operation folds in Request.Method, the resolved route values and the raw request path — three caller-controlled sources on one log line. It was the only such line in the filter; every other log already goes through HashKey(...), which is why nothing else was flagged.

That warning now logs the route pattern read off the endpoint's RoutePattern, a literal from the route table, which identifies the endpoint just as precisely. operation is unchanged where it feeds the cache key — the route values have to stay there, since PUT /tickets/1 and PUT /tickets/2 are genuinely different operations.

NU1903 on SSH.NET (c2046037) — and this one may want to come back out

I had flagged this as pre-existing and out of scope, then hit it hard enough to fix it here: under TreatWarningsAsErrors the advisory is an error, so dotnet restore src/FSH.Starter.slnx fails for the whole solution and Backend CI, CodeQL and the template smoke build all die at restore. The commit pins SSH.NET to 2026.0.0, the first patched release, in the same shape as the MessagePack / Microsoft.OpenApi / SQLitePCLRaw entries next to it.

The commit message says bumping Testcontainers doesn't clear it. That was true of 4.11.0 and 4.13.0, the versions I checked at the time, and it is no longer true: Testcontainers 4.14.0 pulls SSH.NET 2026.0.0 transitively, so the bump alone is sufficient. Verified on a restore of the full solution:

> Testcontainers.PostgreSql   4.14.0
> SSH.NET                                 2026.0.0
> Testcontainers                          4.14.0

So #1369 solves the same advisory without adding a transitive pin, which is the cleaner fix. If that one merges first, c2046037 here should be dropped during the rebase — otherwise it is a duplicate PackageVersion item, exactly the trap you caught on the System.Security.Cryptography.Xml pin in #1323. Happy to drop it now and let this branch depend on #1369, or keep it and drop it later; your call on which ordering you'd rather manage.

Test diagnosis, not a fix (b2770010)

UpdateTheme_Should_NotLeakAcrossTenants_When_RootOperatorTargetsTenantA was seen returning 401 instead of 204 twice on a loaded machine, then passed four runs in a row — including two with 14 of 16 cores saturated — and passes in isolation and in CI. A bare status assertion gave nothing to work with: JwtBearer's reason for rejecting the token is in the ProblemDetails body, which the test discarded.

The assertions now report method, URL and body on mismatch, exercised by expecting the wrong status on purpose so the failure message is known to carry the body. The cause is still unidentified and this test can still go red — I would rather say that plainly than have it read as closed.

Still with you

Unchanged from my 12 Aug comment: the RequestAborted detachment, and whether a request-body fingerprint (option 3, Stripe's 422 on key reuse with a different body) belongs in its own PR. Neither blocks this branch as it stands.

@marcelo-maciel

Copy link
Copy Markdown
Contributor Author

Reopened as #1378.

This PR was closed automatically on 2026-09-14, when the head fork was deleted.
The branch and the commits are unchanged; the head commit is still 7c2dcbaf8c470657eae1320164c173fc4323c1b4.

marcelo-maciel added a commit to marcelo-maciel/dotnet-starter-kit that referenced this pull request Sep 14, 2026
Restore fails for the whole solution under `TreatWarningsAsErrors`, on this
branch and on `main` alike: the same head commit `124f182e` was green on
2026-08-10 and red today, so this is advisory-database drift, not a regression
from this PR.

- `SSH.NET` 2026.0.0 (NU1903, GHSA-q939-rpr3-3284): transitive through the
  Testcontainers packages, which all depend on the vulnerable 2025.1.0. Carried
  byte for byte from fullstackhero#1333, where the pin is still waiting to merge.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq): 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0, and the
  8.x line has no patched release, so the package itself has to move. 10.0.401
  depends on `Microsoft.Build.Tasks.Git` 10.0.401, past the patched 10.0.303.
  Build-time only (`PrivateAssets="all"`), referenced only where
  `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is excluded
  from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…ine (#1379)

* fix(web): honor X-Forwarded-* so the real client IP reaches the pipeline

UseHeroPlatform never called UseForwardedHeaders, so behind the reverse proxy
(Caddy / cloudflared) Connection.RemoteIpAddress was always the proxy container IP.
That collapsed the rate-limit partitions into a single install-wide bucket (one anonymous
spike throttles every tenant's login) and recorded a useless proxy IP on audit trails and
user sessions.

Register ForwardedHeadersOptions (X-Forwarded-For + X-Forwarded-Proto, known
networks/proxies cleared to trust the immediate upstream) and call UseForwardedHeaders
first in the pipeline, before HTTPS redirect / rate limiting / auth / audit read the client.
Lock the trusted set down via ForwardedHeadersOptions when the ingress topology is fixed.

* fix(web): bind forwarded-headers trust to configured proxies

Address review on #1334. Instead of clearing the known-proxy allow-list
(which trusts X-Forwarded-* from any source and reopens the IP-spoofing
hole this PR is meant to close), trust only the ingress proxies/networks
bound from the new TrustedProxyOptions, and honor a configurable
ForwardLimit for the real multi-hop ingress. With nothing configured the
framework default (loopback only) stands, so a client reaching the app
directly can't forge its IP/scheme.

Add a negative test proving an untrusted source's X-Forwarded-For is
ignored, alongside the trusted-proxy happy path. TestServer has no socket,
so the connection IP is stamped via a test-only startup filter.

* fix(web): name the offending setting when trusted-proxy config is malformed

A typo'd entry in TrustedProxyOptions surfaced as a bare FormatException from
IPAddress.Parse / IPNetwork.Parse, with nothing in the message pointing at the
setting that caused it. For config an operator edits once per deployment, under
time pressure, while wiring up an ingress, that is the wrong failure mode: the
silent version of it leaves the app trusting nobody while looking configured.

Both parses now use TryParse and throw an InvalidOperationException naming the
config path and the offending value.

Also closes two gaps the change exposed:

- TrustedProxyOptionsBindingTests pins the TrustedProxyOptions ->
  ForwardedHeadersOptions binding through AddHeroPlatform: the loopback-only
  default when the section is absent, KnownProxies + ForwardLimit binding, and
  both malformed-entry messages. Before this, renaming the config section broke
  nothing that any test could see. The host builder runs with DisableDefaults so
  an ambient TrustedProxyOptions__* on the machine cannot change what "nothing
  configured" resolves to.

- The untrusted-source integration test asserted only that the connection IP was
  persisted, which stays true when forwarded-header processing is absent
  entirely, so it passed with app.UseForwardedHeaders() removed. It now sends the
  identical header from the trusted proxy as well and asserts that arm is
  honored, so the trust boundary is what the test actually pins.

* build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open

`NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by
Testcontainers, fails `restore` for the whole solution under
`TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix
belongs to #1333, which is still open.

Carried byte-identical to #1333's version of the file, comment included, so both
stay mergeable in either order and this copy can simply be dropped once #1333
lands.

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(web): rebuild the forwarded-headers trust list instead of appending to it

AddHeroPlatform only added to KnownProxies/KnownIPNetworks, which assumes whatever is
already there is the framework's loopback default. Under
ASPNETCORE_FORWARDEDHEADERS_ENABLED=true, ConfigureWebDefaults registers
ForwardedHeadersOptionsSetup, which empties both lists. An empty list is not "trust
nobody" in ForwardedHeadersMiddleware: it only validates the peer when at least one
entry exists, so the app rewrote RemoteIpAddress from an X-Forwarded-For sent by any
caller, forging the rate-limit partition and the audit IP.

Clear both lists unconditionally, then either restate the loopback default or apply the
configured proxies/networks.

The new test builds through WebApplication.CreateBuilder with the flag set, asserts
ForwardedHeadersOptionsSetup is actually registered so it cannot pass vacuously, and
checks the resolved lists equal a fresh ForwardedHeadersOptions.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* test(security): read the session this request created, not the newest one

`GetNewestSessionIpAsync` ordered `UserSessions` by `CreatedAt` and took the
first row from the whole table. It is safe only because the collection runs
serially; any other test in it issuing a token leaves the assertion reading a
row this request did not create. Ordering is not what makes it correct either:
`CreatedAt` comes from a single `TimeProvider.System` read and two issues can
land on the same tick, and `Id` is a random `Guid`, so a tiebreak on it picks
deterministically but not necessarily correctly.

Snapshot the session ids before the request and take the one that was not
there. `ShouldHaveSingleItem` asserts the correlation instead of assuming it.

Also states what the factory's `PostConfigure<ForwardedHeadersOptions>` leaves
these tests covering. It overwrites the flags, the forward limit and both trust
lists wholesale, so the `TrustedProxyOptions` binding is not what runs here -
the middleware and the placement of `UseForwardedHeaders` are. The binding has
its own gate in `Framework.Tests/Web/TrustedProxyOptionsBindingTests`, and the
comment now says so rather than reading as if this pinned production.

Verified: `dotnet test --filter FullyQualifiedName~ForwardedHeadersIpTests`
passes 2/2; inverting the new filter to `before.Contains(s.Id)` fails 2/2.

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…1380)

* fix(mailing): send real HTML with a text alternative, not bare text

Every provider puts MailRequest.Body in the HTML slot — MailKit's
BodyBuilder.HtmlBody, SendGrid's htmlContent — but the password-reset and
welcome mails passed plain text. A bare URL inside an HTML part is not
auto-linked by most clients, so the reset link arrived as dead text and the
user had no way to complete the flow. The welcome mail additionally
interpolated the user-supplied first name straight into that HTML.

MailRequest gains an optional TextBody carrying the text/plain alternative.
SmtpMailService emits both parts as multipart/alternative; SendGridMailService
stops passing Body as plainTextContent, which had been shipping raw markup to
text-only clients. Identity builds its bodies through EmailBodies, which
HTML-encodes every interpolated value, and billing bodies gained their plain
twin so no message goes out HTML-only.

Verified: build -warnaserror 0/0; unit suites green (Identity 317,
Framework 122, Billing 123, and the rest).

* fix(deps): pin System.Security.Cryptography.Xml to 10.0.10

The test hosts pull 10.0.8 transitively, which carries HIGH-severity
advisories (GHSA-23rf-6693-g89p, GHSA-8q5v-6pqq-x66h, GHSA-cvvh-rhrc-wg4q,
GHSA-g8r8-53c2-pm3f) and trips NuGetAudit under TreatWarningsAsErrors,
breaking the build of every test project. 10.0.10 is the patched
servicing release. Mirrors the existing Microsoft.OpenApi transitive pin.

* build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open

`NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by
Testcontainers, fails `restore` for the whole solution under
`TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix
belongs to #1333, which is still open.

Carried byte-identical to #1333's version of the file, comment included, so both
stay mergeable in either order and this copy can simply be dropped once #1333
lands.

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* fix(mailing): keep a text/plain part when only Body is supplied

Moving `plainTextContent` from `Body` to `TextBody` made the text part vanish
rather than become empty: `MailHelper.CreateSingleEmail` only adds it when the
string is non-null and non-empty. Every caller inside this repo was migrated, so
the tree is fine — but this is a template, and a consumer who still writes
`new MailRequest(to, subject, "Your code is 123456")` silently went from a
two-part message to HTML-only, with no compiler error and no warning.

`TextBody ?? Body` restores the old behaviour for them and changes nothing for a
caller that supplies both. Covered by a test that would have caught the drop.

* test(mailing): gate the body mapping on the default provider too

The `text/plain` regression this PR fixes was gated only on SendGrid, which is
the optional provider. SMTP is the default (`UseSendGrid` defaults to false), and
its body builder had no test at all, so putting the markup back into both parts
there would have kept the suite green.

The transport needs a server and is not where the mapping happens, so the MIME
build is what these drive: both parts present as `multipart/alternative`, HTML
alone staying `text/html` with no invented plain part, and an attachment wrapping
the alternative in `multipart/mixed` without dropping the text. `AddAttachmentsAsync`
is `internal` with `InternalsVisibleTo` for the test assembly, the same way
`Storage` already exposes its internals to the integration suite.

Mapping `TextBody` back to `request.Body` turns all three red.

* test(notifications): gate the module's mail bodies with its own test project

The module that produces every templated mail on this branch had no test
project at all, so the text/plain twin this PR is about was only gated at the
provider end. `Notifications.Tests` closes that: the templates are pure string
builders, so the whole thing runs without a DbContext or a host.

`BillingEmailBodiesTests` walks all four templates through one theory and
asserts the pair is a real pair - an HTML body carrying the subject and markup,
and a plain twin that is non-empty, repeats the subject and contains no angle
bracket. Plus the two places the copy can drift on its own: a tenant name is
escaped in the markup and left literal in the text part (entities in
text/plain are read as entities), and the optional due-date line disappears
without leaving a blank paragraph behind. `BillingEmailSenderTests` covers the
best-effort send: both bodies reach the `MailRequest`, an absent address sends
nothing, a throwing provider does not escape the integration-event handler, and
the warning it logs identifies the operation without carrying the recipient.

`IMailService` is ours, so the doubles are real implementations rather than
substitutes, per `.agents/rules/testing.md`. The module exposes its internals to
this assembly the same way `Mailing` already does for `Framework.Tests`.

Verified: `dotnet test src/Tests/Notifications.Tests` passes 13/13; making
`Expired` reuse its HTML as the text body fails the theory arm and the sender's
pass-through case. `dotnet build src/FSH.Starter.slnx` exits 0 with the project
registered, and Architecture.Tests stays 55/55.

* ci: run Notifications.Tests in the unit-test job

The job enumerates test projects by name rather than running the solution, so
adding `Notifications.Tests` to `FSH.Starter.slnx` put it in the build and
nowhere else: the previous run on this branch was green without executing a
single one of its tests. A gate CI never runs is not a gate.

Registering it in the slnx stays - that is what `dotnet test src/FSH.Starter.slnx`
and the IDE read - but this list is the wiring that matters on a PR.

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…duplicates (#1378)

* fix(web): correct idempotent replay payload and serialize concurrent duplicates

Two defects in IdempotencyEndpointFilter:

API-01 — the filter cached JsonSerializer.SerializeToUtf8Bytes(result) where result
is the wrapped IResult (Ok<T>/Created<T>), so it stored {"value":...,"statusCode":200}
instead of the wire DTO, and it read Response.StatusCode before the IResult executed,
so a 201 Created replayed as 200. The handler result is now executed into a buffer to
capture the real wire body + status, which is what gets served and cached.

CONC-01 — probe->execute->write had no atomic reservation, so two concurrent requests
with the same key both missed the probe and both executed the handler. An atomic in-flight
reservation now serializes duplicates: Redis SET NX when an IConnectionMultiplexer is
registered (the multi-instance case — this stack already requires Redis there for the
shared Data Protection key ring), an in-process set otherwise (single instance). A
duplicate that arrives while the original is still running gets 409 Conflict.

Redis stays optional: without it the app falls back to the in-memory reservation, correct
for a single instance where a cross-container race cannot occur.

* fix(web): make idempotency replay actually engage (symmetric cache store)

The write went through HybridCache.SetAsync while the probe read IDistributedCache by
the raw key. HybridCache keys its L2 entries under its own scheme, so the probe never
found the entry and replay silently never engaged — even in production. Proven by
un-skipping ChatSendMessageTests.SendMessage_Should_Replay_Same_Response_When_Idempotency_Key_Reused,
which now passes.

Write to the same IDistributedCache, key and serializer the probe uses. Idempotency
entries are short-lived (TTL) and their HybridCache tag-purge path was unused, so dropping
HybridCache here loses nothing.

* fix(web): short-TTL, fail-open idempotency reservation

Address review on #1333:

- Reservation used the 24h response TTL, so a crash between reserving
  and the finally-release stranded the Redis lock for a day (every retry
  409s). Add IdempotencyOptions.ReservationTtl (default 1m), decoupled
  from DefaultTtl.
- Reserve now fails open on a transient Redis error instead of 500ing
  the request, matching the best-effort stance of the response write.
- Guard the release KeyDeleteAsync so a Redis fault can't throw out of
  the finally.

Tests: reservation uses ReservationTtl not DefaultTtl; a faulting Redis
on reserve/release proceeds without throwing (exercises the Redis NX
branch the prior tests skipped).

* fix(web): make the stored idempotent response outlive the request

Three defects surfaced by re-reading the whole filter rather than the delta.

The response store was tied to the client's connection, so the retry that
idempotency exists to serve re-executed the handler. Two paths caused it, not
one: the body write to the client ran before the store, so a closed socket threw
and skipped it entirely; and the capture itself ran under RequestAborted, where
WriteAsJsonAsync swallows the cancellation and hands back an EMPTY body — which
was then cached and replayed as a 200 for the full 24h TTL. The capture now runs
with the abort token detached (it writes to an in-memory buffer, never the
socket), the store runs before the client write and on CancellationToken.None,
and only then does the body go out.

Replay dropped every response header, so a replayed 201 arrived without
Location: a client that follows the header worked on the first call and broke on
the retry. The captured response now carries an allow-listed set (Location,
ETag) and replays it. Transport and host-owned headers stay out — a stale
Content-Length would corrupt the response.

Non-2xx is no longer stored. Faithful status capture made the pre-existing
behaviour bite: a transient downstream failure locked the caller out of that key
for 24h. A failure is not a record of a committed side effect.

CachedIdempotentResponse is no longer a HybridCache type, so its
[ImmutableObject(true)] contract (and the CachedTypeContractTests entry
asserting it) described a store this filter stopped using. Both dropped. The new
Headers property defaults to empty so entries written before it deserialize.

Tests: replayed 201 carries Location; a first call whose client disconnects
still replays the real DTO body; a non-2xx first response lets the retry run.
All three fail on the previous commit and pass here.

* fix(web): scope the idempotency entry to the operation, not the tenant alone

The entry was keyed on tenant + caller key, with nothing identifying the
operation. One key reused against a second idempotent endpoint replayed the
first endpoint's response and the second request silently never ran. Thirty-one
endpoints across eight modules share that namespace, and one of them
(self-registration) is anonymous: it resolves no tenant claim, so every caller
of it lands in the same "global" bucket. This was latent only for as long as
replay never engaged — the fix that makes replay work is what would have put it
on the wire. The key now folds in the HTTP method and the route pattern.

Two smaller things in the same area. The 409 for an in-flight duplicate and the
400 for an over-long key emitted a bare JSON string, where every other error
these endpoints produce is RFC 9457 ProblemDetails; both now match. And an
unreadable cache entry (written by another version, or another writer at the
same key) let JsonException escape as a 500 — that path only became reachable
once replay started engaging at all. It now degrades to a miss and logs.

ReleaseReservationAsync also swallows cancellation now, not just faults: it runs
in a finally after the response body has already gone to the client, so anything
thrown there can only reset the connection on a request that succeeded.

Tests: a key reused across two route patterns runs the second handler; an
unreadable entry runs the handler (with a valid entry seeded at the same key
first, so the assertion can't pass as a plain cache miss); a 204 replays without
a fabricated content type. Each fails on a mutated implementation.

* fix(web): close the idempotency reservation's races

The reservation guarded the handler against concurrent duplicates but four
holes let one through anyway, or locked a caller out of a key:

- The entry was keyed on the caller's `tenant` claim. A root operator scoping
  requests to different tenants shares one "root" bucket, so one key reused
  across two targets replays the first tenant's body to the second. Key off the
  resolved tenant context instead — the one BaseDbContext scopes the side effect
  to — with the claim as the fallback for a JWT-only request (Finbuckle's claim
  strategy runs pre-authentication and resolves nothing for those). The raw
  `tenant` header is deliberately not a fallback: an unresolved header is one
  Finbuckle refused, and an unvalidated value has no business in a shared key.

- The cache was probed once, before the reservation. The original request can
  store its response and release the lock inside that window; the duplicate then
  takes the free lock and executes the handler again. Probe once more with the
  lock held.

- The lock was a `:inflight` suffix on the entry key, so a caller key ending in
  that suffix put its 24h entry exactly where another key's lock goes — every
  later request with that key 409s for the full response TTL. Give the lock its
  own prefix.

- Release was an unconditional delete. A request that failed open on a Redis
  blip, or one whose reservation had already expired, freed a lock another
  request was holding. Release via compare-and-delete against the token the
  reservation was taken with; failing open carries no token and deletes nothing.

The in-process fallback also gains the TTL takeover the Redis branch gets for
free: without it a handler that never returns strands the key until the process
restarts and every retry 409s forever.

Each fix is pinned by a test that was verified to fail when the fix is reverted.

* fix(web): keep an idempotent handler alive past a client disconnect

Follow-up from adversarial passes over the whole filter. Each item below is
pinned by a test verified to fail when the fix is reverted.

- The handler ran under the client's abort token. A disconnect after the side
  effect committed cancelled the next await inside the handler (an EF read, an
  outbox write, a Mediator behaviour), so the filter had nothing to store and
  the client's retry re-executed the side effect — the duplicate this filter
  exists to absorb. The handler now runs with the token detached; the trade is
  that a disconnect no longer aborts an idempotent handler.

- The probe was the one link that hard-failed. Reserve and store both degrade to
  a warning when the cache is down, so a `RedisConnectionException` on the probe
  took every idempotent endpoint down for exactly the clients that send a key.
  It now fails open as a miss.

- A handler that writes the response itself had already started it, so the buffer
  swap captured nothing and setting the captured status threw. That case now
  passes through untouched and stores nothing.

- The key covered the route pattern but not its values, so `PUT /tickets/1` and
  `PUT /tickets/2` were one operation: the second replayed the first ticket's
  response and never ran. It now folds in the resolved route values.

- The key was not scoped to the caller, so two users of one tenant reusing a
  low-entropy key on the same endpoint received each other's response bodies
  while their own request was silently suppressed.

- The 409 said "retry shortly" with no `Retry-After`. It now sends 1 second: the
  original is normally about to store its response, and the reservation TTL is
  the worst case, not the hint.

Also: options are validated at startup like every other block here (a zero TTL
failed silently inside the best-effort write, so nothing was ever stored),
`CacheKeys.Tags.Idempotency` no longer claims to be applied, and the cached
headers dictionary documents that its comparer does not survive deserialization.
Ceilings that stay: no size cap on the buffered response (do not put
`.WithIdempotency()` on a streaming endpoint), no lease renewal, and a lock whose
Redis may not be the cache's Redis — all three now carry `ponytail:` notes.

* test(web): cover the idempotency branches no test could fail on

A test-quality pass over the suite found assertions that survive the mutation
they exist to catch, and branches with no test at all. Each case below now fails
when the behaviour it pins is reverted.

Assertions that could not fail:
- The Lua release script was matched with Arg.Any<string>() while the fake
  hardcoded compare-and-delete, so swapping the script for an unconditional
  `del` kept the suite green — the exact bug the script's comment warns about.
  The script text is asserted now.
- `(result as IStatusCodeHttpResult)?.StatusCode.ShouldBe(409)` skips the whole
  assertion for a result that isn't one, which is precisely the mutation it
  guards. Cast instead.
- The concurrency test relied on the default one-minute ReservationTtl outliving
  the test; a CI freeze past it hands the key over and fails a correct filter.
  It pins the TTL explicitly.

Branches with no coverage: a handler that throws (the release has to stay in the
finally, or one exception strands the key until the TTL), the tenant-claim
fallback (collapsing it to "global" puts every JWT-only caller in one bucket and
replays across tenants), the refused duplicate's re-probe, the restrictive half
of the header allow-list (Set-Cookie must not come back on a replay), the
best-effort store, a faulting release, the no-header pass-through, the
MaxKeyLength rejection, and an entry stored without the Headers member — the
shape a previous version wrote, which has to keep replaying through a rolling
deploy.

Also drops a stale comment claiming body capture is out of reach; this PR is
what made it possible, and the integration suite asserts it end to end.

* test(identity): pin that the caller id claim survives bearer inbound mapping

The idempotency filter scopes its cache key by ClaimsPrincipal.GetUserId(), which
reads ClaimTypes.NameIdentifier only. Until now nothing proved that claim type is
present after JwtBearer validates a real issued token: every existing test built the
principal by hand, so caller scoping could have been inert in production (every
caller collapsing into one bucket) with a green suite.

Round-trips a token from TokenService through JsonWebTokenHandler configured with
JwtBearerOptions' own MapInboundClaims default, then asserts GetUserId() resolves.
Verified with the claim removed from the token as well: the short-form `sub` maps to
it, so both shapes IdentityService emits resolve.

* docs(agents): idempotency rule covers the abort-token, probe and key changes

The rule described the reservation work from the previous round but not what
landed after it, so an agent reading it would still believe the probe hard-fails
and the key ignores route values and the caller. Adds the abort-token detachment
together with the constraint it implies (no streaming endpoints), the HasStarted
pass-through, Retry-After and the startup validation.

* test(web): assert idempotency options through IStartupValidator, not lazily

The startup-validation tests resolved IOptions<IdempotencyOptions>.Value, which
validates on first access with or without .ValidateOnStart(). Deleting that call
left all six green while moving the failure from boot to the first request that
carries an Idempotency-Key — a suite that could not see the difference between
"rejected at startup" and "rejected once, in production, per process".

They now go through IStartupValidator, which is what .ValidateOnStart() registers
and what the host runs before serving traffic. Verified: with .ValidateOnStart()
removed, 6 of the 10 tests fail.

* test(web): pin each idempotency options clause to its own failure message

Asserting only OptionsValidationException let a clause be deleted with every row
still green: a zero DefaultTtl also trips "ReservationTtl must not exceed
DefaultTtl", so the row aimed at DefaultTtl passed on the wrong clause. Each row
now names the failure it expects.

Per-clause mutation run: removing any one of the five clauses fails exactly the
one row that targets it; removing .ValidateOnStart() fails all six cases, since
IStartupValidator is then unregistered.

* fix(identity): drop idempotency from self-registration, and gate anonymous endpoints

The cache key scopes by caller, and ResolveCaller returns "anon" for every
unauthenticated request, so on an anonymous endpoint all callers share one bucket.
Two people registering on the same tenant with the same low-entropy key ("1",
"retry") built the identical key: the second replayed the first registrant's 201
with the first registrant's UserId, and their own account was silently never
created. /self-register was the only anonymous idempotent endpoint. It was
unreachable until replay started engaging.

A retry there is already safe without the filter — the unique-email constraint
rejects the duplicate — so the endpoint drops .WithIdempotency() rather than
gaining a body fingerprint.

Deleting one call would leave nothing stopping the next one, so WithIdempotency()
now attaches IdempotentEndpointMetadata: an endpoint filter is invisible in
metadata, and the marker makes the wiring inspectable. IdempotencyWiringTests
walks the endpoint map and fails when an AllowAnonymous() endpoint carries it,
plus a second test that fails if the marker stops being attached, so the first
cannot pass over an empty set.

* test(multitenancy): carry the response body into the theme status assertions

UpdateTheme_Should_NotLeakAcrossTenants_When_RootOperatorTargetsTenantA was seen
returning 401 instead of 204 twice on a loaded machine, then passed four runs in a
row (including two with 14 of 16 cores saturated) and passes in isolation and in
CI. A bare status assertion gives nothing to work with: the reason JwtBearer
rejected the token is in the ProblemDetails body, which the test discarded.

The assertions now report method, URL and body on mismatch. Exercised by
expecting the wrong status on purpose: the failure message carries the body.

This is diagnosis, not a fix. The cause is still unidentified, and this test can
still go red.

* build(deps): pin SSH.NET to the patched 2026.0.0

The Testcontainers packages pull SSH.NET 2025.1.0 transitively, which carries
GHSA-q939-rpr3-3284 (CVE-2026-48798, high): ScpClient recursive download writes
files outside the target directory. Under TreatWarningsAsErrors that advisory is
NU1903 as an error, so `dotnet restore src/FSH.Starter.slnx` fails for the whole
solution — Backend CI, CodeQL and the template smoke build all die at restore.

Testcontainers 4.11.0 and 4.13.0 both depend on 2025.1.0, so bumping
Testcontainers does not clear it. 2026.0.0 is the first patched release, and
transitive pinning is already enabled, so this entry alone bumps it — same shape
as the MessagePack, Microsoft.OpenApi and SQLitePCLRaw pins next to it.

* fix(web): keep request-derived text out of the idempotency warning

CodeQL flagged the `Response.HasStarted` pass-through warning (alert 28,
cs/log-forging): it logged `operation`, which folds in `Request.Method`, the
resolved route values and the raw request path, so three caller-controlled
sources reached a log line. Every other log in the filter already passes
`HashKey(...)`, which is why this was the only one.

The warning now logs the route pattern read off the endpoint's `RoutePattern`,
a literal from the route table, which identifies the endpoint just as well.
`operation` is unchanged for the cache key, where the route values have to stay:
`PUT /tickets/1` and `PUT /tickets/2` are different operations.

* fix(web): detach the bound CancellationToken argument, not only RequestAborted

Minimal-API parameter binding resolves a handler's CancellationToken from
HttpContext.RequestAborted before endpoint filters run, so the handler already
holds a copy of the original token. Reassigning the property inside the filter
never reached it: a client disconnect after the side effect committed could
still cancel an await inside the handler, leaving the filter with nothing to
store and letting the retry execute the side effect a second time.

Replace the bound argument as well. RequestAborted keeps being reassigned for
code that reads it directly instead of taking it as a parameter.

The regression test runs a real host over the real filter; a hand-built
EndpointFilterInvocationContext skips binding entirely and cannot see this.

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* test(idempotency): exercise the Redis refusal, not just the in-process one

Every concurrency test ran with `multiplexer: null`, so the branch that actually
refuses a duplicate in a multi-instance deployment — `StringSetAsync(...,
When.NotExists)` coming back false — was never executed. The one test that does
share a keyspace asserts the opposite case (a key that must NOT be blocked).

Verified by mutation: flipping the reservation to `When.Always` turns the new
test red and leaves the rest of the suite green, which is exactly the regression
that used to be invisible.

* fix(idempotency): let an endpoint cap its own replay window

`RequestUploadUrl` returns a presigned URL good for `FilesOptions.UploadUrlTtlMinutes`
(15 by default) and cached the response for `DefaultTtl`, 24 hours. A client
retrying with the same key an hour later got a 200 carrying a URL that had
expired 45 minutes earlier, with no way out except inventing a new key. Dropping
`.WithIdempotency()` from the endpoint was the other option and is worse: the
handler INSERTs a pending `FileAsset` and pre-checks quota, so the common case (a
network retry seconds later) would start duplicating rows to fix the rare one.

`WithIdempotency(TimeSpan)` puts the window on the endpoint that knows it.
`IdempotentEndpointMetadata` carries it, the filter prefers it over the
configured default, and the endpoint reads the value at map time from the same
option the handler mints the URL with, so the two cannot drift. A non-positive
TTL throws rather than expiring every entry on write, which would leave the
endpoint advertising an idempotency it no longer has.

Two more from the same review:

- A handler returning a bare `string` was captured through `WriteAsJsonAsync`,
  which quotes it and sends `application/json`. Minimal APIs write `text/plain`,
  so the first response through this filter differed from the same handler's
  response without it, and the replay then repeated the difference.
- The filter comment and `.agents/rules/security.md` both described anonymous
  endpoints as a live case of the shared bucket while `IdempotencyWiringTests`
  fails the build for exactly that. They now describe the floor the code has,
  not a configuration it forbids.

* test(idempotency): make the client-abort test actually abort

`IdempotentHandler_Should_RunToCompletion_When_ClientAbortIsSignalled` signalled
nothing: the handler slept 20ms on its own token and the assertion passed whether
or not the filter detached anything. Deleting `DetachBoundCancellationTokens` left
it green.

The abort is real now. A middleware ahead of the endpoint publishes a token the
test controls as `RequestAborted` (TestServer cannot hang up a live request from
the client side), and the handler cancels it mid-flight. The first response then
never reaches the client, which is correct and is asserted rather than swallowed:
the store is deliberately sequenced ahead of the body write for this case. What
the test asserts is the point of the detach, that the retry replays instead of
committing the side effect twice.

With the detach reverted, both tests in the class go red.

* docs(agents): condense the idempotency rule into scannable bullets

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
* fix(web): honor X-Forwarded-* so the real client IP reaches the pipeline

UseHeroPlatform never called UseForwardedHeaders, so behind the reverse proxy
(Caddy / cloudflared) Connection.RemoteIpAddress was always the proxy container IP.
That collapsed the rate-limit partitions into a single install-wide bucket (one anonymous
spike throttles every tenant's login) and recorded a useless proxy IP on audit trails and
user sessions.

Register ForwardedHeadersOptions (X-Forwarded-For + X-Forwarded-Proto, known
networks/proxies cleared to trust the immediate upstream) and call UseForwardedHeaders
first in the pipeline, before HTTPS redirect / rate limiting / auth / audit read the client.
Lock the trusted set down via ForwardedHeadersOptions when the ingress topology is fixed.

* fix(web): bind forwarded-headers trust to configured proxies

Address review on #1334. Instead of clearing the known-proxy allow-list
(which trusts X-Forwarded-* from any source and reopens the IP-spoofing
hole this PR is meant to close), trust only the ingress proxies/networks
bound from the new TrustedProxyOptions, and honor a configurable
ForwardLimit for the real multi-hop ingress. With nothing configured the
framework default (loopback only) stands, so a client reaching the app
directly can't forge its IP/scheme.

Add a negative test proving an untrusted source's X-Forwarded-For is
ignored, alongside the trusted-proxy happy path. TestServer has no socket,
so the connection IP is stamped via a test-only startup filter.

* fix(web): name the offending setting when trusted-proxy config is malformed

A typo'd entry in TrustedProxyOptions surfaced as a bare FormatException from
IPAddress.Parse / IPNetwork.Parse, with nothing in the message pointing at the
setting that caused it. For config an operator edits once per deployment, under
time pressure, while wiring up an ingress, that is the wrong failure mode: the
silent version of it leaves the app trusting nobody while looking configured.

Both parses now use TryParse and throw an InvalidOperationException naming the
config path and the offending value.

Also closes two gaps the change exposed:

- TrustedProxyOptionsBindingTests pins the TrustedProxyOptions ->
  ForwardedHeadersOptions binding through AddHeroPlatform: the loopback-only
  default when the section is absent, KnownProxies + ForwardLimit binding, and
  both malformed-entry messages. Before this, renaming the config section broke
  nothing that any test could see. The host builder runs with DisableDefaults so
  an ambient TrustedProxyOptions__* on the machine cannot change what "nothing
  configured" resolves to.

- The untrusted-source integration test asserted only that the connection IP was
  persisted, which stays true when forwarded-header processing is absent
  entirely, so it passed with app.UseForwardedHeaders() removed. It now sends the
  identical header from the trusted proxy as well and asserts that arm is
  honored, so the trust boundary is what the test actually pins.

* build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open

`NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by
Testcontainers, fails `restore` for the whole solution under
`TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix
belongs to #1333, which is still open.

Carried byte-identical to #1333's version of the file, comment included, so both
stay mergeable in either order and this copy can simply be dropped once #1333
lands.

* fix(web): reject a trusted-proxy ForwardLimit below 1 at startup

TrustedProxyOptions.ForwardLimit was passed straight to ForwardedHeadersOptions
with no validation, and neither bad value announces itself. Zero truncates the
unwind loop in ApplyForwarders to zero iterations, so X-Forwarded-* stop being
processed with no error and no log while the config still reads as configured.
A negative value makes the middleware allocate a negative-length buffer, which
throws OverflowException on every request - including requests carrying no
forwarded headers at all - and UseForwardedHeaders sits after UseExceptionHandler,
so that surfaces as a plain 500 rather than a boot failure a smoke test catches.

Reject anything below 1 where the malformed KnownProxies/KnownNetworks entries
are already rejected, naming the setting and the offending value. The throw lands
during startup, so a bad hop count fails the deploy instead of the traffic.

Closes #1358

* docs(web): record why X-Forwarded-Host stays out of the flag list

Review note from #1334, left for the follow-up: the flag list carries only
X-Forwarded-For and X-Forwarded-Proto, and the omission is deliberate. Rewriting
Request.Host from a header is a host-header injection primitive, and the three
Identity endpoints that build a public URL from the request would then mail
confirmation links pointing wherever the header said. The consequence an operator
has to know is that Request.Host keeps the internal host behind a proxy, and those
links carry it.

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(web): rebuild the forwarded-headers trust list instead of appending to it

AddHeroPlatform only added to KnownProxies/KnownIPNetworks, which assumes whatever is
already there is the framework's loopback default. Under
ASPNETCORE_FORWARDEDHEADERS_ENABLED=true, ConfigureWebDefaults registers
ForwardedHeadersOptionsSetup, which empties both lists. An empty list is not "trust
nobody" in ForwardedHeadersMiddleware: it only validates the peer when at least one
entry exists, so the app rewrote RemoteIpAddress from an X-Forwarded-For sent by any
caller, forging the rate-limit partition and the audit IP.

Clear both lists unconditionally, then either restate the loopback default or apply the
configured proxies/networks.

The new test builds through WebApplication.CreateBuilder with the flag set, asserts
ForwardedHeadersOptionsSetup is actually registered so it cannot pass vacuously, and
checks the resolved lists equal a fresh ForwardedHeadersOptions.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* docs(web): say what a ForwardLimit above the real hop count costs

The option documented the failure of setting it too low and the two invalid
values, but not the one an operator is most likely to reach for: rounding it up
"to be safe". The middleware trusts one entry per hop counting from the right and
only ever checks the peer, so a limit of 2 behind a single proxy hands the caller
its own RemoteIpAddress, and every IP-based rate limit and audit entry follows it.

* test(security): read the session this request created, not the newest one

`GetNewestSessionIpAsync` ordered `UserSessions` by `CreatedAt` and took the
first row from the whole table. It is safe only because the collection runs
serially; any other test in it issuing a token leaves the assertion reading a
row this request did not create. Ordering is not what makes it correct either:
`CreatedAt` comes from a single `TimeProvider.System` read and two issues can
land on the same tick, and `Id` is a random `Guid`, so a tiebreak on it picks
deterministically but not necessarily correctly.

Snapshot the session ids before the request and take the one that was not
there. `ShouldHaveSingleItem` asserts the correlation instead of assuming it.

Also states what the factory's `PostConfigure<ForwardedHeadersOptions>` leaves
these tests covering. It overwrites the flags, the forward limit and both trust
lists wholesale, so the `TrustedProxyOptions` binding is not what runs here -
the middleware and the placement of `UseForwardedHeaders` are. The binding has
its own gate in `Framework.Tests/Web/TrustedProxyOptionsBindingTests`, and the
comment now says so rather than reading as if this pinned production.

Verified: `dotnet test --filter FullyQualifiedName~ForwardedHeadersIpTests`
passes 2/2; inverting the new filter to `before.Contains(s.Id)` fails 2/2.

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
iammukeshm added a commit to fullstackhero/docs that referenced this pull request Sep 25, 2026
…#240)

* docs(idempotency): rewrite for the corrected replay behaviour

Tracks fullstackhero/dotnet-starter-kit#1333. The concept page described a
HybridCache-backed store, a key scoped to the tenant only, and a replay of
"status + body" - none of which is what the filter does now.

Rewrites the flow section around the same-store/same-key symmetry, what is
captured (wire body, real status, allow-listed Location/ETag), the 2xx-only
rule, the store happening before the client write on an uncancellable token,
and the in-flight reservation with its 409 and the new ReservationTtl option.
Replaces the "the cache key does NOT include the route" gotcha, which is no
longer true, with the body and anonymous-tenant caveats that are. Adds the
changelog entry and the ReservationTtl mention on the Web building-block page.

* docs(idempotency): reservation races and how the tenant on the key is chosen

The key follows the resolved tenant (a root operator's target included), the
claim is only a fallback and an unresolved `tenant` header is never used. Both
reservation branches expire on `ReservationTtl`, and releasing is a
compare-and-delete so a request that failed open frees nothing.

* docs(idempotency): handler survives a client disconnect, and the key's new scopes

The handler runs with the abort token detached, the probe fails open like the
rest, a handler that writes the response itself is passed through, and the key
covers the caller and the resolved route values. Adds the `Retry-After` on the
409, the startup validation of the options, and the ceilings worth knowing:
buffered responses, and cross-instance dedup depending on the cache being on
Valkey rather than on the reservation.

* docs(changelog): 29 idempotent endpoints, not 31

* docs(idempotency): anonymous endpoints must not be idempotent

The page framed the shared "anon" caller bucket as a caveat to work around with
fresh UUIDs. It is a defect: two self-registrations on one tenant with the same
low-entropy key made the second replay the first registrant's 201, and no account
was created. Documents the fix instead - the endpoint dropped .WithIdempotency(),
and an integration test now fails the build if an anonymous endpoint carries it.

* docs(idempotency): document the per-endpoint replay window

An endpoint whose 2xx body goes stale on its own schedule can now cap the replay
window with WithIdempotency(ttl). RequestUploadUrl is the case that motivated it:
a presigned URL good for fifteen minutes, cached for the twenty-four-hour default,
so a retry an hour later replayed a 200 carrying a URL that was already dead.

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…1385)

* fix(mailing): send real HTML with a text alternative, not bare text

Every provider puts MailRequest.Body in the HTML slot — MailKit's
BodyBuilder.HtmlBody, SendGrid's htmlContent — but the password-reset and
welcome mails passed plain text. A bare URL inside an HTML part is not
auto-linked by most clients, so the reset link arrived as dead text and the
user had no way to complete the flow. The welcome mail additionally
interpolated the user-supplied first name straight into that HTML.

MailRequest gains an optional TextBody carrying the text/plain alternative.
SmtpMailService emits both parts as multipart/alternative; SendGridMailService
stops passing Body as plainTextContent, which had been shipping raw markup to
text-only clients. Identity builds its bodies through EmailBodies, which
HTML-encodes every interpolated value, and billing bodies gained their plain
twin so no message goes out HTML-only.

Verified: build -warnaserror 0/0; unit suites green (Identity 317,
Framework 122, Billing 123, and the rest).

* fix(deps): pin System.Security.Cryptography.Xml to 10.0.10

The test hosts pull 10.0.8 transitively, which carries HIGH-severity
advisories (GHSA-23rf-6693-g89p, GHSA-8q5v-6pqq-x66h, GHSA-cvvh-rhrc-wg4q,
GHSA-g8r8-53c2-pm3f) and trips NuGetAudit under TreatWarningsAsErrors,
breaking the build of every test project. 10.0.10 is the patched
servicing release. Mirrors the existing Microsoft.OpenApi transitive pin.

* build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open

`NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by
Testcontainers, fails `restore` for the whole solution under
`TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix
belongs to #1333, which is still open.

Carried byte-identical to #1333's version of the file, comment included, so both
stay mergeable in either order and this copy can simply be dropped once #1333
lands.

* refactor(mailing): one HTML shell and one encoder for every module

Follow-up to the nit on #1351: `EmailBodies` (Identity) and
`BillingEmailBodies.Wrap` (Notifications) had grown into two independent HTML
shells with two different escapers, and they would have drifted.

- New `FSH.Framework.Mailing.HtmlEmail` holds the document shell (doctype,
  charset, viewport, card) and the encoder. Both modules already referenced the
  Mailing building block, so no new project reference.
- `Encode` is `WebUtility.HtmlEncode` everywhere. The hand-rolled four-`Replace`
  chain in Notifications covered only `&`, `<` and `>` — safe in element content,
  not in an attribute — and is gone.
- `EmailBodies` is deleted; its two callers use `HtmlEmail` directly rather than
  a pass-through.
- Billing mail now renders in the same document as identity mail, so it gains a
  doctype and a `<meta charset>` it did not have.

`Shell` takes trusted markup and does not encode it; the doc comment says so and
a test pins it, because "hardening" that would render every e-mail as visible
tags.

* fix(notifications): encode the invoice amount, currency included, in the HTML part

`amountText` embeds `currency`, which is data rather than a literal, and was the
only value in this file reaching the markup unencoded — `invoiceNumber`,
`tenantName` and `plan` were all escaped already.

Not a vulnerability today, and the description says so: the only writer is
`CreatePlanCommand`, capped at three characters by its validator and gated by
`BillingPermissions.Manage`, while the top-up path passes a hardcoded "USD".
Three characters in element content cannot form a working payload. This is
consistency and defence in depth: the only thing standing between the value and
the markup is a length rule in another module.

No-op for every real currency code: encoding "100.00 USD" returns it unchanged.

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* fix(mailing): keep a text/plain part when only Body is supplied

Moving `plainTextContent` from `Body` to `TextBody` made the text part vanish
rather than become empty: `MailHelper.CreateSingleEmail` only adds it when the
string is non-null and non-empty. Every caller inside this repo was migrated, so
the tree is fine — but this is a template, and a consumer who still writes
`new MailRequest(to, subject, "Your code is 123456")` silently went from a
two-part message to HTML-only, with no compiler error and no warning.

`TextBody ?? Body` restores the old behaviour for them and changes nothing for a
caller that supplies both. Covered by a test that would have caught the drop.

* refactor(mailing): fix a misleading test name and two dead lines

The encoder test was named Encode_Should_EncodeOnce_When_ValueIsAlreadyEscaped
and commented as guarding against double-encoding, while asserting exactly that
double-encoding. The behaviour is right — the input is text, so a literal
ampersand must be escaped even when it spells an entity — but the name invited
someone to 'fix' the encoder into leaving raw ampersands in markup.

Also drops an orphan using (EmailBodies is deleted) and the two blank entries in
the text builder that the Join filtered straight back out.

* docs(mailing): say plainly which half of the claim is true

The type doc said HtmlEmail was the single shell for outbound mail. It is the
single encoder; UserRegistrationService.BuildConfirmationEmailHtml still builds
its own document, and that is the most-seen e-mail in the product. Migrating it
changes layout and copy, so it belongs in its own PR — but the doc should not
claim coverage that does not exist in the meantime.

* refactor(identity): drop EmailBodies, superseded by the shared HtmlEmail

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
iammukeshm added a commit that referenced this pull request Sep 25, 2026
…d of losing the write (#1387)

* build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open

`NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by
Testcontainers, fails `restore` for the whole solution under
`TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix
belongs to #1333, which is still open.

Carried byte-identical to #1333's version of the file, comment included, so both
stay mergeable in either order and this copy can simply be dropped once #1333
lands.

* fix(identity): reject a stale profile update instead of losing the write

`PUT /identity/profile` is a full-representation update: every field is assigned
from the request, so a caller working from a stale read blanks whatever changed
in between. Nothing on the request said which version the caller had edited, so
the server could not tell a deliberate overwrite from a lost update and accepted
both.

`AspNetUsers.ConcurrencyStamp` is already mapped as an EF concurrency token and
Identity's store rotates it on every `UserManager.UpdateAsync`, so the version
marker exists — it just was not on the wire. `GET /identity/profile` now
publishes it as a strong `ETag`, and `PUT /identity/profile` honours `If-Match`:
a token that no longer matches gets `412 Precondition Failed` instead of
silently winning. No migration and no schema change.

The header stays optional — absent means today's behaviour, so existing clients
keep working. A `ponytail:` comment marks the future path where it becomes
required and a missing header answers `428`.

Details worth calling out:

- The precondition is checked immediately after the user is loaded, before the
  storage calls. Any later and a rejected update would already have uploaded an
  orphan blob or, on the `deleteCurrentImage` path, deleted the avatar for a
  request that then fails and changes nothing in the database.
- `IdentityResult`'s `ConcurrencyFailure` is mapped to the same 412. Identity's
  store returns it rather than throwing, so a race lost one layer down used to
  surface as a generic 500.
- `RefreshSignInAsync` now runs after the success guard. It used to refresh the
  sign-in even when the update had failed.
- `*` in `If-Match` asks only that the resource exist. Weak validators can never
  satisfy the strong comparison the header mandates, so they answer 412. A
  malformed header answers 400: 412 would send a client into a refetch-and-retry
  loop it can never win, since the broken header is its own bug.

Tests: integration coverage for the ETag shape, matching/stale/list/`*`/weak/
malformed preconditions, token rotation and the avatar-survives-412 case, plus a
handler unit test that the tokens reach the service.

* test(identity): assert a rejected profile update leaves no partial write

The avatar case only checked the image URL. `SetPhoneNumberAsync` persists on its
own, ahead of the final `UserManager.UpdateAsync`, so a precondition checked too
late would let a field through on a request that then answers 412. Asserting the
name as well pins that down, and the comment now says what the test proves rather
than claiming the storage call itself is observed.

* fix(dashboard): send If-Match when saving the profile, retry once on 412

`updateMyProfile` reads the profile, merges the edited fields and PUTs the whole
representation back. Nothing tied that write to the version it was built from, so a
concurrent change — another tab, a phone, a slow save racing a fast one — was
silently overwritten.

The read now also picks up the profile's `ETag` and the PUT echoes it in `If-Match`,
so the server can answer 412 instead of accepting a stale representation. A 412 is
retried once from a fresh read: the token rotates on writes the user never thinks of
as profile edits (a password change, a failed sign-in, a new avatar), and turning
those into a failed save would be noise. A second 412 propagates.

`apiFetch` grew an `onResponse` hook, because it returns the parsed body and there
was no way to reach a response header from a caller.

Note for anyone running the API on a separate origin (the dev setup does — the page
is on 5174 and the API on 7030): `ETag` is not a CORS-safelisted response header, so
the browser hides it from JS unless the API also sends
`Access-Control-Expose-Headers: ETag`, and `If-Match` has to be an allowed request
header. The framework's CORS policy does neither today, which is a separate change
in protected code. Until it lands this path degrades to the old behaviour — the
client reads no tag and sends no precondition. Same-origin deployments (the shipped
`apiBase: ""` default) are unaffected.

* test(identity): gate the ETag CORS exposure the front-end depends on

The dashboard specs mock `Access-Control-Expose-Headers: ETag`, which the API does
not send: `FSH.Framework.Web.Cors` never calls `WithExposedHeaders`. A browser
therefore hides the tag from JS on any cross-origin call, the client stops sending
`If-Match`, and the endpoint silently falls back to the lost-update behaviour this
branch set out to fix -- with every test still green.

Assert it instead of describing it in a comment. The test is skipped so the suite
stays green until the framework change lands (protected code, needs approval);
the skip reason names exactly what has to change to un-skip it.

Verified: un-skipped it fails on the missing header; with `WithExposedHeaders("ETag")`
added locally to the AllowAll branch it passes. That temporary edit was reverted --
`src/BuildingBlocks` is untouched by this branch.

Refs #1359

* feat(cors): expose ETag and allow If-Match so clients can use preconditions

`ETag` is not a CORS-safelisted response header, so a browser hid it from JS on every
cross-origin call -- which is every dev run, since both React apps point `apiBase` at
the API's own origin. A front-end that cannot read the validator cannot send `If-Match`,
so the optimistic-concurrency precondition on `PUT /identity/profile` degraded straight
back to the lost update it exists to prevent, with the whole suite still green.

Exposed for both policy branches: neither `AllowAnyHeader` nor `WithHeaders` implies
exposure, and the header carries no data of its own, only a validator.

`if-match` joins `AllowedHeaders` in both shipped appsettings for the mirror-image reason:
with `AllowAll: false` the request header is stripped before it reaches the endpoint.

Gates: `CorsPolicyTests` covers both branches at the policy level and
`GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsRead` covers it end to end,
so the front-end mocks can no longer hide a server that stops sending the header. Verified by
mutation -- dropping the argument turns all three red; restored and re-run green.

Refs #1359

* build(deps): bump Testcontainers to 4.14.0 and SourceLink past their advisories

`dotnet restore` fails for the whole solution under `TreatWarningsAsErrors`, on
`main` and on every open PR alike. Advisory-database drift, not a regression from
any change: a commit green on 2026-08-10 is red today with no edits.

- `Testcontainers.PostgreSql` / `.Redis` / `.Minio` 4.11.0 -> 4.14.0 (NU1903,
  GHSA-q939-rpr3-3284). 4.11.0 depends on `SSH.NET` 2025.1.0; 4.14.0 already
  depends on the patched 2026.0.0, so the advisory clears with no transitive pin
  to remember to remove later. Same fix as #1369, so the two do not conflict.
- `Microsoft.SourceLink.GitHub` 8.0.0 -> 10.0.401 (NU1902,
  GHSA-23fw-v26w-5fgq). 8.0.0 drags in `Microsoft.Build.Tasks.Git` 8.0.0 and the
  8.x line has no patched release, so a transitive pin cannot fix it; the package
  itself has to move. 10.0.401 depends on `Microsoft.Build.Tasks.Git` 10.0.401,
  past the patched 10.0.303. Build-time only (`PrivateAssets="all"`), referenced
  only where `IsPackable == true`, which is the CLI alone - and `src/Tools/**` is
  excluded from the template, so the scaffold never sees it.

Verified: `dotnet restore src/FSH.Starter.slnx` exits 0 with no NU19xx, and
`dotnet build src/FSH.Starter.slnx -c Release -warnaserror` reports 0 warnings
and 0 errors.

* fix(infra): pull MinIO from quay.io on a pinned tag, not Docker Hub

MinIO withdrew `minio/minio` from Docker Hub. Docker Hub's API now answers
`object not found` for the repository, and a pull fails with:

    pull access denied for minio/minio, repository does not exist or may
    require 'docker login'

That takes down every Testcontainers-backed integration test (the harness boots
a MinIO container per fixture, so all 724 tests in `Integration.Tests` fail at
container start), the Aspire AppHost, and the Docker Compose deployment. The
image is still published at `quay.io/minio/minio`:

- `Integration.Tests` and `Integration.Middleware.Tests` harnesses
- `AppHost.cs`, via Aspire's `WithImageRegistry` / `WithImageTag`
- `deploy/docker/docker-compose.yml` and the image table in its README

The tag is pinned to `RELEASE.2025-09-07T16-13-09Z` rather than `:latest`. quay
has not moved `:latest` since 2025-09-07, so the two resolve to the same digest
today; pinning only removes the surprise of a silent move later, and keeps the
test harness off a floating tag. Whether to track a newer release, or a different
S3-compatible image, is a separate call.

While in the README's image table: `postgres` and `redis` rows had drifted from
what compose actually ships (`postgres:18-alpine`, `valkey/valkey:9.1.0-alpine`).

Verified: `docker pull minio/minio:latest` fails with the error above;
`docker pull quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` succeeds
(`sha256:14cea493d9a34af32f524e538b8346cf79f3321eff8e708c1e2960462bd8936e`, the
same digest `:latest` resolves to). `dotnet test Integration.Tests -c Release`
passes against the pinned image, and the Aspire manifest renders the container
as `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z`.

* fix(dashboard): take the profile ETag from the read the form was seeded with

The save read the profile again and used that read's ETag as If-Match. A
tag fetched at save time is current by construction, so it matched
whatever a concurrent writer had just stored and the PUT went through:
the endpoint gained 412 handling while the client could never trigger
it. The lost update the PR set out to stop happens between the user
seeing the values and pressing save, and nothing was watching that gap.

The ETag now travels with the profile the form was seeded from, held in
a ref so a background refetch cannot advance it to a version the user
never saw. The 412 retry is gone with it: the only body available is the
one typed against the old values, so resending it against a fresh tag
performs exactly the overwrite the 412 prevented. The page warns, keeps
the typed edits on screen, adopts the current version, and waits for a
deliberate second save.

Two consequences fell out of getting there. The refetch after a conflict
has to pass staleTime 0, or the client's 30s default hands back the
cached copy carrying the tag the server just rejected. And Save is now
disabled until the profile read lands, since a save carries that read's
unedited fields and version — previously the save built its own body, so
it could run without one.

The topbar and the security page share this query key, so they read
through the same ETag-carrying function: one key, one shape.

Gates: the two new specs fail on the previous client (the save sent the
post-change tag; the retry overwrote) and pass after. profile.spec 9/9,
tsc and lint clean. Full dashboard suite 151/153 with 2 failures that
pass on their own run and touch none of this — a pre-existing flake
under 6 workers, reported separately.

* fix(identity): survive a weak ETag and a rotated stamp on the profile form

Three defects an independent review found in the concurrency work, all of them
on paths a normal user walks.

**A compressing edge makes the profile permanently unsavable.** The endpoint only
emits a strong validator, but Cloudflare (and any edge that re-encodes a
response) downgrades the tag it forwards to `W/"..."` by default. The client
stored and echoed that verbatim, the server dropped it under the strong
comparison `If-Match` mandates, and every save answered 412 — on a profile
nobody else was touching, with the UI blaming a concurrent editor. The client
strips the `W/` prefix: a weak tag can only be a transport artefact here.

**Changing the avatar guaranteed a 412 on the next save.** Setting the image is a
second write to the same row, so Identity rotates the concurrency stamp, but the
image mutation only invalidated the query — the form kept the pre-image tag.
It adopts the new version instead, which also refreshes the cached copy the
topbar avatar reads.

**A lost race on the delete-avatar path could destroy the blob.** The `If-Match`
guard is not the last word: another writer can still land between it and
`UpdateAsync`, which then fails with `ConcurrencyFailure` and maps to 412. The
old blob had already been removed by then, leaving `AspNetUsers.ImageUrl`
pointing at something that no longer exists — unrecoverable, and invisible until
the next page load. The delete now runs only after the database write succeeds.

Both client fixes are mutation-checked: reverting either turns its new spec red.
The third has no automated test — reproducing it needs a real race between
`FindByIdAsync` and `UpdateAsync` inside one request — so it is inspection plus
the existing 17 profile integration tests staying green.

* fix(infra): pull minio/mc from quay.io too, not just minio/minio

The MinIO carve-out this branch carries only moved `minio/minio`. `minio/mc` is
gone from Docker Hub as well (`hub.docker.com/v2/repositories/minio/mc/` answers
404), and it is what `minio-init` runs: without it `dotnet run --project
src/Host/FSH.Starter.AppHost` and `docker compose up` both die on the image pull,
and the `fsh` bucket is never created, so the first upload fails with
NoSuchBucket.

Same pinned tag as #1388, which owns the fix, so the copy stays byte-identical
to it and can be dropped once that lands.

* build(deps): drop the dead SSH.NET pin

The pin's own comment says "Testcontainers 4.11.0 and 4.13.0 both depend on
2025.1.0, so bumping Testcontainers does not help", but the branch also bumps
Testcontainers to 4.14.0, whose nuspec declares `SSH.NET >= 2026.0.0`. The two
statements cannot both be true, and the bump is the one that is: with the pin
removed, `dotnet restore src/FSH.Starter.slnx --force` reports zero NU1902/NU1903
and exits 0. It was carrying a transitive pin that no longer pins anything.

The MessagePack pin above it stays: that one is still load-bearing (removing it
brings GHSA-hv8m-jj95-wg3x straight back, verified in the same probe).

* fix(identity): close three gaps review found around the precondition

- `onSuccess` fired `adoptCurrentVersion()` without awaiting it, so `isPending`
  dropped before the new tag was in hand: the button re-enabled over a spent tag
  and a quick second save 412'd against the user's own write, with a toast
  blaming someone else. A failed refetch was also an unhandled rejection that
  left the form stranded on a tag the server had already rejected.
- `if-match` in `CorsOptions:AllowedHeaders` had no gate. `CorsPolicyTests`
  builds its configuration in memory, so removing the header from the shipped
  appsettings kept the suite green while the restricted policy stripped the
  precondition off every PUT — the feature would degrade back to the lost update
  it exists to prevent, silently. The new test loads the shipped files the way
  the host does, for both environments.
- The disabled-save path (profile read failing) was described in the PR body as
  one of the two latent bugs fixed, and had no test. It has one now.

---------

Co-authored-by: iammukeshm <iammukeshm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants