fix(identity): reject stale profile updates with ETag/If-Match instead of losing the write by marcelo-maciel · Pull Request #1387 · fullstackhero/dotnet-starter-kit

marcelo-maciel · 2026-09-14T09:55:12Z

Carries two infrastructure fixes that are not this PR's topic. Without them CI cannot even reach this PR's code.

Dependency bump: Microsoft.SourceLink.GitHub to 10.0.401 and Testcontainers to 4.14.0, clearing NU1902/NU1903 so restore succeeds. Same fix as #1375 and #1369.

MinIO (7d6ab655): minio/minio is gone from Docker Hub, so every Testcontainers-backed integration test dies on the image pull. Pulls from quay.io on a pinned tag instead. Same fix as #1388.

Both are byte-identical to the PRs that own them, so they stay mergeable in either order and these copies can simply be dropped once those land.

Reopened from #1366. That 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
7daadf535701d5919975fbb2388e8842072362bd. The earlier review history stays on #1366.

What

PUT /identity/profile is a full-representation update with no concurrency token, so two
overlapping self-updates silently lose one another's changes (#1359). This adds an optimistic
precondition: GET /identity/profile returns a strong ETag, PUT honours If-Match, and a
stale token is answered with 412 Precondition Failed instead of overwriting the newer write.

Closes #1359.

The token is already there — no migration

The issue's own suggested fix proposed a new RowVersion/xmin column. That isn't needed:
AspNetUsers.ConcurrencyStamp is already mapped IsConcurrencyToken() in the snapshot, and
ASP.NET Identity's UserStore.UpdateAsync rotates it on every write. It can serve as the
validator directly, which keeps this change at zero schema impact and avoids carrying two
concurrency tokens on one entity.

The stamp is exposed as the ETag and never as a body field ([JsonIgnore] on
UserDto.ConcurrencyStamp), so it stays out of the OpenAPI contract and cannot be spoofed
through the request body.

Behaviour

Request	Result
No `If-Match`	Accepted, as before. Backward compatible.
`If-Match` matching the stored stamp	Accepted; the stamp rotates.
`If-Match: *`	Accepted (RFC 9110: matches any current representation).
Stale `If-Match`	`412`, nothing written.
Weak validator (`W/"..."`)	`412` — `If-Match` mandates the strong comparison function.
Malformed header	`400`, not `412`: a `412` would send a well-behaved client into a refetch-and-retry loop it can never win, since the malformed header is its own bug.

Two smaller fixes fell out of this:

UserManager.UpdateAsync answers a lost race with IdentityResult.Failed(ConcurrencyFailure())
rather than throwing, so it used to surface as a generic 500. It now maps to the same 412.
RefreshSignInAsync ran before the success check, refreshing the sign-in even when the update
had failed. It now runs only on success.

Ordering matters

The precondition is checked immediately after FindByIdAsync, ahead of the storage calls and
ahead of SetPhoneNumberAsync. Anywhere later and a 412 would already have orphaned an upload,
or — on the deleteCurrentImage path — deleted the avatar with no database change to show for it.
UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCurrentImageRequested covers exactly
that; I confirmed it goes red if the guard is moved down.

Worth naming for reviewers: SetPhoneNumberAsync is a second database write and its
IdentityResult is discarded (pre-existing, untouched here). The handler is therefore not atomic
across the two writes. A phone-number race is still reported, because the subsequent UpdateAsync
also fails and that failure now maps to 412 — but the mechanism is that mapping, not atomicity.

Client

clients/dashboard reads the profile through getMyProfileWithETag, seeds the form from that
read, and sends that same read's tag as If-Match on save. Both halves matter: the tag has to come
from the read the user actually saw, because the lost update this guards against happens between
that moment and the save. A tag fetched inside the save is never stale and so never catches
anything.

A 412 is not retried. An earlier revision of this PR retried once against a fresh read, on the
theory that the stamp also rotates on writes the user never perceives as a profile edit (a password
change, a new avatar). Review removed it: the only body the client holds is the one built against
the values the user saw, so resending it against a freshly fetched tag performs exactly the
overwrite the 412 rejected. The save now keeps the user's edits on screen, adopts the current
version, and asks for a deliberate re-save.

Two latent bugs surfaced once the tests drove that path for real: the client's global
staleTime: 30_000 made the post-412 fetchQuery hand back the cached copy carrying the very tag
the server had just rejected (now staleTime: 0), and a save attempted before the profile read
landed was silently a no-op (the button is disabled until the read succeeds, with the reason on
screen).

clients/admin has no PUT /identity/profile caller on main, so nothing to change there. (The
issue text claims both apps write the profile — that part of my own report was wrong.)

The CORS piece — why this PR touches BuildingBlocks

An ETag/If-Match contract is a no-op in the browser unless CORS cooperates, and it did not:

ETag is not a CORS-safelisted response header. FSH.Framework.Web.Cors never called
WithExposedHeaders, so a browser hid the tag from JS on every cross-origin call — which is
every dev run, since both React apps point apiBase at the API's own origin. The client then
read null, stopped sending If-Match, and the endpoint degraded straight back to the lost
update it now prevents. One WithExposedHeaders call (plus a four-line comment), placed after the
branch so it covers both policies — neither AllowAnyHeader nor WithHeaders implies exposure.
if-match is not a safelisted request header either. It joins AllowedHeaders in both
shipped appsettings, otherwise AllowAll: false strips the precondition before the endpoint
sees it.

I know src/BuildingBlocks is protected, so this is deliberately the smallest possible change and
kept in its own commit (feat(cors): expose ETag and allow If-Match…) — easy to drop or rework
without touching the rest. Happy to split it into its own PR if you'd rather review it separately.

Both directions are gated rather than described in a comment: CorsPolicyTests asserts the exposure
at the policy level for both branches, and
GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsRead asserts it end to end against
a cross-origin request. The front-end specs mock the header, so without a server-side gate nothing
would notice the policy dropping it. Verified by mutation: removing the argument turns all three
red.

Separately and not fixed here: the tenant header both front-ends send on every request is
also missing from AllowedHeaders, so anyone enabling the restricted policy today is already
broken. Filed as #1367 rather than folded into this one.

Also in this PR

Directory.Packages.props pins SSH.NET to 2026.0.0 (cherry-picked byte-identical from #1333):
NU1903 breaks restore on main too, so nothing builds without it. Drop that commit once #1333
merges.

Verification

Integration suite (Testcontainers): 756 passed / 1 skipped (pre-existing) / 0 failed
All 13 other test projects green, aggregate exit 0 (~1057 tests)
clients/dashboard Playwright: 152 passed, 1 failed — tests/chat/chat.spec.ts:107, a
pre-existing flake under worker contention that passes 5/5 in isolation and touches none of the
changed files. tsc -b clean and eslint . exit 0 (12 pre-existing react-refresh warnings,
none in touched files)
Mutation-checked at three points: reverting the precondition guard to a no-op turns 4 tests red;
moving it below the storage calls turns the avatar test red; dropping the CORS exposure turns
the three new CORS gates red.

Docs + changelog: fullstackhero/docs#246. It used
to advise clients to refetch and retry once on 412, which is the overwrite this PR's client
deliberately refuses to perform. Corrected in fullstackhero/docs@632e8d87, which also documents that
the tag must come from the read that populated the form.

…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 fullstackhero#1333, which is still open. Carried byte-identical to fullstackhero#1333's version of the file, comment included, so both stay mergeable in either order and this copy can simply be dropped once fullstackhero#1333 lands.

`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.

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.

`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.

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 fullstackhero#1359

…itions `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 fullstackhero#1359

…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 fullstackhero#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.

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`.

…ed 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.

marcelo-maciel · 2026-09-17T19:59:26Z

Follow-ups this PR does not carry:

Merge order against #1384. This PR changes the updateMyProfile signature and removes getMyProfile; #1384's language switcher calls both from clients/dashboard/src/components/layout/topbar.tsx. Whichever merges second fails tsc -b in Frontend CI. Agreed order: this PR first, #1384 adapts its switcher to getMyProfileWithETag and passes profile, expectedETag and locale.

Docs (AGENTS.md rule 10): present, and the wrong line is now fixed. fullstackhero/docs#246 already documents the precondition, so the rule is satisfied. It does, however, tell clients to "refetch and retry once on 412", and that guidance recreates the bug the status code exists to prevent: the only body the client holds is the one built against the values the user saw, so resending it against a freshly fetched tag performs exactly the overwrite the 412 rejected. The dashboard client in this PR deliberately does not retry. It adopts the current version, keeps the user's edits on screen, and asks for a deliberate re-save. Corrected in fullstackhero/docs@632e8d87: the page now says not to auto-retry, adds that the tag must come from the read that populated the form, and the changelog entry matches what the dashboard actually does. Site build green.

Test-suite note. Full dashboard Playwright run on this branch: 152 passed, 1 failed (tests/chat/chat.spec.ts:107). That spec passes in isolation (5/5), touches none of the changed files, and fails only under worker contention. Pre-existing flake, flagged rather than papered over.

The page told clients to refetch and retry once on 412, and the changelog said the tenant dashboard does exactly that. Both recreate the bug the status code prevents: the only body a client holds is the one built from the values the user saw, so resending it against a freshly fetched tag performs the overwrite the 412 rejected. Replaced with what fullstackhero/dotnet-starter-kit#1387 actually ships: take the tag from the read that populated the form, never from a read inside the save, and on 412 keep the user's edits, adopt the current version, and ask for a deliberate re-save.

marcelo-maciel added 6 commits August 17, 2026 06:44

marcelo-maciel mentioned this pull request Sep 14, 2026

fix(identity): reject stale profile updates with ETag/If-Match instead of losing the write #1366

Closed

marcelo-maciel added 3 commits September 16, 2026 16:39

marcelo-maciel mentioned this pull request Sep 17, 2026

feat(dashboard): localize the tenant app with react-i18next (4/4) #1384

Open

Navigation Menu

fix(identity): reject stale profile updates with ETag/If-Match instead of losing the write - #1387

fix(identity): reject stale profile updates with ETag/If-Match instead of losing the write#1387
marcelo-maciel wants to merge 9 commits into
fullstackhero:mainfrom
marcelo-maciel:fix/identity-profile-concurrency

marcelo-maciel commented Sep 14, 2026 •

edited

Loading

Uh oh!

marcelo-maciel commented Sep 17, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

marcelo-maciel commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

The token is already there — no migration

Behaviour

Ordering matters

Client

The CORS piece — why this PR touches BuildingBlocks

Also in this PR

Verification

Uh oh!

marcelo-maciel commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

marcelo-maciel commented Sep 14, 2026 •

edited

Loading

marcelo-maciel commented Sep 17, 2026 •

edited

Loading