fix(identity): reject stale profile updates with ETag/If-Match instead of losing the write - #1366
Open
marcelo-maciel wants to merge 6 commits into
Open
Conversation
…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
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
|
CI note: the Everything else is green: Backend CI, Frontend CI, Unit Tests, Integration Tests, E2E (admin and |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PUT /identity/profileis a full-representation update with no concurrency token, so twooverlapping self-updates silently lose one another's changes (#1359). This adds an optimistic
precondition:
GET /identity/profilereturns a strongETag,PUThonoursIf-Match, and astale token is answered with
412 Precondition Failedinstead of overwriting the newer write.Closes #1359.
The token is already there — no migration
The issue's own suggested fix proposed a new
RowVersion/xmincolumn. That isn't needed:AspNetUsers.ConcurrencyStampis already mappedIsConcurrencyToken()in the snapshot, andASP.NET Identity's
UserStore.UpdateAsyncrotates it on every write. It can serve as thevalidator directly, which keeps this change at zero schema impact and avoids carrying two
concurrency tokens on one entity.
The stamp is exposed as the
ETagand never as a body field ([JsonIgnore]onUserDto.ConcurrencyStamp), so it stays out of the OpenAPI contract and cannot be spoofedthrough the request body.
Behaviour
If-MatchIf-Matchmatching the stored stampIf-Match: *If-Match412, nothing written.W/"...")412—If-Matchmandates the strong comparison function.400, not412: a412would 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.UpdateAsyncanswers a lost race withIdentityResult.Failed(ConcurrencyFailure())rather than throwing, so it used to surface as a generic
500. It now maps to the same412.RefreshSignInAsyncran before the success check, refreshing the sign-in even when the updatehad failed. It now runs only on success.
Ordering matters
The precondition is checked immediately after
FindByIdAsync, ahead of the storage calls andahead of
SetPhoneNumberAsync. Anywhere later and a412would already have orphaned an upload,or — on the
deleteCurrentImagepath — deleted the avatar with no database change to show for it.UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCurrentImageRequestedcovers exactlythat; I confirmed it goes red if the guard is moved down.
Worth naming for reviewers:
SetPhoneNumberAsyncis a second database write and itsIdentityResultis discarded (pre-existing, untouched here). The handler is therefore not atomicacross the two writes. A phone-number race is still reported, because the subsequent
UpdateAsyncalso fails and that failure now maps to
412— but the mechanism is that mapping, not atomicity.Client
clients/dashboardreads theETagfrom the profile fetch, echoes it asIf-Matchon save, andretries once on
412against a fresh read. The retry is deliberate: the stamp also rotates onwrites the user never perceives as a profile edit (a password change, a failed sign-in, a new
avatar), so a single
412should resolve itself rather than surface as a failed save.clients/adminhas noPUT /identity/profilecaller onmain, so nothing to change there. (Theissue 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-Matchcontract is a no-op in the browser unless CORS cooperates, and it did not:ETagis not a CORS-safelisted response header.FSH.Framework.Web.Corsnever calledWithExposedHeaders, so a browser hid the tag from JS on every cross-origin call — which isevery dev run, since both React apps point
apiBaseat the API's own origin. The client thenread
null, stopped sendingIf-Match, and the endpoint degraded straight back to the lostupdate it now prevents. One
WithExposedHeaderscall (plus a four-line comment), placed after thebranch so it covers both policies — neither
AllowAnyHeadernorWithHeadersimplies exposure.if-matchis not a safelisted request header either. It joinsAllowedHeadersin bothshipped
appsettings, otherwiseAllowAll: falsestrips the precondition before the endpointsees it.
I know
src/BuildingBlocksis protected, so this is deliberately the smallest possible change andkept in its own commit (
feat(cors): expose ETag and allow If-Match…) — easy to drop or reworkwithout 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:
CorsPolicyTestsasserts the exposureat the policy level for both branches, and
GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsReadasserts it end to end againsta 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
tenantheader both front-ends send on every request isalso missing from
AllowedHeaders, so anyone enabling the restricted policy today is alreadybroken. Filed as #1367 rather than folded into this one.
Also in this PR
Directory.Packages.propspinsSSH.NETto2026.0.0(cherry-picked byte-identical from #1333):NU1903breaksrestoreonmaintoo, so nothing builds without it. Drop that commit once #1333merges.
Verification
0(~1057 tests)clients/dashboardPlaywright: 152 passed;tsc -bclean andeslint .exit0(12 pre-existingreact-refreshwarnings, none in touched files)moving it below the storage calls turns the avatar test red; dropping the CORS exposure turns
the three new CORS gates red.
Docs + changelog in
fullstackhero/docsfollow in a companion PR.