Lost update on PUT /identity/profile: no concurrency token on a full-representation update

[marcelo-maciel]

Summary

PUT /identity/profile is a full-representation update with no concurrency token, so two overlapping self-updates silently lose one another's changes. The last write wins on every field, not just the one the caller meant to change.

Why it happens

UpdateUserCommandHandler assigns every field unconditionally — there is no "leave this alone" sentinel:

// src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserCommandHandler.cs
await _userService.UpdateAsync(
    ...,
    command.FirstName ?? string.Empty,
    command.LastName ?? string.Empty,
    command.PhoneNumber ?? string.Empty,
    ...,
    command.Locale,
    ...);

Because a partial body would blank the omitted fields, both front-ends compensate client-side with a read-modify-write: getMyProfile() first, merge, then PUT the merged representation (clients/admin/src/api/users.ts:98, clients/dashboard/src/api/identity.ts:411). That is correct as far as it goes, but the read and the write are two round trips with no server-side check that nothing moved in between. Nothing on the request identifies which version of the resource the caller edited, so the server cannot detect the overlap and has to accept the stale copy.

Reproduction

1. Open Settings › Profile and let the form load (this issues the read).
2. Before saving, change the UI language from the topbar language switcher. That is its own read-modify-write and it persists the new locale.
3. Save the profile form.

The profile save was built from a snapshot taken before step 2, so it echoes the old locale back and wins. The same shape applies to any pair of concurrent self-updates: two browser tabs, a phone and a desktop, or a slow save racing a fast one. locale is simply the field that made it visible, because the UI reacts to it.

Impact

Low severity, but it is real data loss on a user-facing endpoint, and it gets worse as more fields land on the profile. Today the observable symptom is a language preference that does not survive a reload.

Suggested fix

Add optimistic concurrency to the resource:

• A RowVersion / xmin concurrency token on the user aggregate, surfaced as an ETag on GET /identity/profile.
• PUT /identity/profile honours If-Match and answers 412 Precondition Failed when the token does not match, 428 Precondition Required if the header is absent (or accepts the absence for one release, to stay backward compatible).
• Both front-ends pass the ETag they read through to the PUT and refetch-and-retry on 412.

This is a contract change to an existing endpoint, which is why it is filed separately rather than folded into #1344.

Context

Found during the adversarial audit of #1344 (i18n). That PR mitigates the visible symptom but does not fix the cause: the topbar stops hydrating the language from the persisted profile once the user has picked a language in-session, so a stale echo no longer changes the UI mid-session. The ponytail: comments in clients/admin/src/components/layout/topbar.tsx and clients/dashboard/src/components/layout/topbar.tsx point here.

[marcelo-maciel]

Working on this. Three corrections and one finding that changes the shape of the fix.

The concurrency token already exists — no migration needed

The suggested fix asked for a new RowVersion / xmin column. That is not necessary: AspNetUsers.ConcurrencyStamp is already mapped as an EF concurrency token (IdentityDbContextModelSnapshot.cs:106, .IsConcurrencyToken()), and UserStore.UpdateAsync rotates it on every UserManager.UpdateAsync (aspnetcore v10.0.8, src/Identity/EntityFrameworkCore/src/UserStore.cs:212-230), answering a lost race with IdentityResult.Failed(ConcurrencyFailure()) rather than throwing. So the version marker is already maintained per write — it simply was not on the wire. Publishing it as the ETag costs no schema change, and adding xmin alongside it would put two concurrency tokens on the same entity for no gain.

Worth knowing about that token: it rotates on any UserManager.UpdateAsync, not only on profile edits. In this repo that means a failed sign-in (AccessFailedAsync), a successful sign-in that had prior failures (ResetAccessFailedCountAsync, which returns early when the counter is already 0), a password change (PasswordExpiryService.UpdateLastPasswordChangeDateAsync), registration, and an avatar change. A client must therefore treat a single 412 as "refetch and retry once", not as a failed save, or an unrelated background write turns into a spurious error in the user's face.

The admin citation in the issue is wrong

The report cites clients/admin/src/api/users.ts:98 as a read-modify-write caller. On main that line is getMyProfile, and clients/admin has no caller of PUT /identity/profile at all — the operator app only reads the profile and sets the avatar through its own endpoint. The admin-side read-modify-write exists on the i18n branches (where the language switcher persists locale), not on main. Only clients/dashboard (src/api/identity.ts, updateMyProfile) writes the profile today, so that is the only front-end the fix touches.

CORS blocks the header flow whenever the API is on another origin

This is the part that is not obvious from the issue text. ETag is not a CORS-safelisted response header, so response.headers.get("ETag") returns null for a cross-origin response unless the server sends Access-Control-Expose-Headers: ETag. Symmetrically, If-Match is not a safelisted request header, so it needs to appear in Access-Control-Allow-Headers.

Neither is configured today. FSH.Framework.Web.Cors never calls WithExposedHeaders, and the shipped restricted policy allows only [ "content-type", "authorization" ] (appsettings.json:103, appsettings.Production.json:62).

Measured, not assumed: with the dashboard's dev setup (page on localhost:5174, apiBase pointed at https://localhost:7030 by clients/dashboard/vite.config.ts:9), a response carrying ETag: "stamp-1" reached the browser and the client still read null. The same request served same-origin exposes the tag fine — which is also why the shipped production default (apiBase: "") would work without any CORS change.

So an ETag-based contract needs the framework to expose the header. That is a change inside src/BuildingBlocks, which is protected in this repo, so it is being raised for a maintainer decision rather than pushed silently.

While looking at that config: the tenant header, which both front-ends send on every request, is also missing from those AllowedHeaders lists. Anyone who turns the restricted policy on today is already blocked by that, independently of this issue. Happy to file it separately.