From 97909222f2bb5ed0fe5a4571851e182e05ecadf5 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 01:43:15 -0300 Subject: [PATCH 01/13] build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- src/Directory.Packages.props | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 0d38b28190..7674befa8f 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -143,5 +143,12 @@ AccessViolation). Transitive pinning is enabled, so this entry alone bumps it. Remove once the SignalR backplane package depends on a patched version itself. --> + + \ No newline at end of file From a2815d2f2f654267e8c9ecfe36424e3a860c6229 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 06:59:04 -0300 Subject: [PATCH 02/13] fix(identity): reject a stale profile update instead of losing the write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../DTOs/UserDto.cs | 12 +- .../Services/IUserProfileService.cs | 6 +- .../Services/IUserService.cs | 2 +- .../v1/Users/UpdateUser/UpdateUserCommand.cs | 13 ++ .../GetUserProfile/GetUserProfileEndpoint.cs | 18 +- .../UpdateUser/UpdateUserCommandHandler.cs | 1 + .../v1/Users/UpdateUser/UpdateUserEndpoint.cs | 46 +++- .../Services/UserProfileService.cs | 46 +++- .../Modules.Identity/Services/UserService.cs | 4 +- .../Handlers/UpdateUserCommandHandlerTests.cs | 31 ++- .../Tests/Users/UserProfileTests.cs | 219 ++++++++++++++++++ 11 files changed, 381 insertions(+), 17 deletions(-) diff --git a/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs b/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs index 0ccd71384e..24c8094ab5 100644 --- a/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs +++ b/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs @@ -1,4 +1,6 @@ -namespace FSH.Modules.Identity.Contracts.DTOs; +using System.Text.Json.Serialization; + +namespace FSH.Modules.Identity.Contracts.DTOs; public class UserDto { @@ -22,4 +24,12 @@ public class UserDto /// Whether the user has enrolled in TOTP-based two-factor authentication. public bool TwoFactorEnabled { get; set; } + + /// + /// The stored optimistic-concurrency token for this user, populated only by the self-profile + /// read. It never reaches the response body — GET /identity/profile turns it into the + /// response's ETag, and that header is the token clients echo back in If-Match. + /// + [JsonIgnore] + public string? ConcurrencyStamp { get; set; } } \ No newline at end of file diff --git a/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs b/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs index f305b4a782..4b6ce7c26e 100644 --- a/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs +++ b/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs @@ -24,9 +24,11 @@ public interface IUserProfileService Task GetCountAsync(CancellationToken cancellationToken); /// - /// Updates a user's profile. + /// Updates a user's profile. When is non-null the + /// update is rejected with unless the + /// stored concurrency token matches one of the entries — the caller edited a stale copy. /// - Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, CancellationToken cancellationToken = default); + Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, IReadOnlyList? expectedConcurrencyStamps, CancellationToken cancellationToken = default); /// /// Sets the profile image URL directly (no upload). Used by the presigned-upload flow: diff --git a/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserService.cs b/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserService.cs index 91ab3467fa..b365a46e89 100644 --- a/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserService.cs +++ b/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserService.cs @@ -15,7 +15,7 @@ public interface IUserService Task ToggleStatusAsync(bool activateUser, string userId, CancellationToken cancellationToken); Task GetOrCreateFromPrincipalAsync(ClaimsPrincipal principal, CancellationToken cancellationToken = default); Task RegisterAsync(string firstName, string lastName, string email, string userName, string password, string confirmPassword, string phoneNumber, string origin, CancellationToken cancellationToken); - Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, CancellationToken cancellationToken = default); + Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, IReadOnlyList? expectedConcurrencyStamps, CancellationToken cancellationToken = default); Task DeleteAsync(string userId, CancellationToken cancellationToken = default); Task ConfirmEmailAsync(string userId, string code, string tenant, CancellationToken cancellationToken); Task AdminConfirmEmailAsync(string userId, CancellationToken cancellationToken = default); diff --git a/src/Modules/Identity/Modules.Identity.Contracts/v1/Users/UpdateUser/UpdateUserCommand.cs b/src/Modules/Identity/Modules.Identity.Contracts/v1/Users/UpdateUser/UpdateUserCommand.cs index 09292a46bc..1299b88100 100644 --- a/src/Modules/Identity/Modules.Identity.Contracts/v1/Users/UpdateUser/UpdateUserCommand.cs +++ b/src/Modules/Identity/Modules.Identity.Contracts/v1/Users/UpdateUser/UpdateUserCommand.cs @@ -1,5 +1,6 @@ using FSH.Framework.Shared.Storage; using Mediator; +using System.Text.Json.Serialization; namespace FSH.Modules.Identity.Contracts.v1.Users.UpdateUser; @@ -12,4 +13,16 @@ public class UpdateUserCommand : ICommand public string? Email { get; set; } public FileUploadRequest? Image { get; set; } public bool DeleteCurrentImage { get; set; } + + /// + /// Concurrency tokens the caller is willing to overwrite, taken from the request's + /// If-Match header by the endpoint. means the caller sent no + /// precondition and accepts whatever version is stored; a non-null list means the update + /// only proceeds when the stored token matches one of the entries. + /// + /// + /// Header-derived, never read from the request body — the endpoint always overwrites it. + /// + [JsonIgnore] + public IReadOnlyList? ExpectedConcurrencyStamps { get; set; } } \ No newline at end of file diff --git a/src/Modules/Identity/Modules.Identity/Features/v1/Users/GetUserProfile/GetUserProfileEndpoint.cs b/src/Modules/Identity/Modules.Identity/Features/v1/Users/GetUserProfile/GetUserProfileEndpoint.cs index c6038cdbb9..4fe544d89b 100644 --- a/src/Modules/Identity/Modules.Identity/Features/v1/Users/GetUserProfile/GetUserProfileEndpoint.cs +++ b/src/Modules/Identity/Modules.Identity/Features/v1/Users/GetUserProfile/GetUserProfileEndpoint.cs @@ -6,6 +6,7 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; +using Microsoft.Net.Http.Headers; using System.Security.Claims; namespace FSH.Modules.Identity.Features.v1.Users.GetUserProfile; @@ -14,18 +15,29 @@ public static class GetUserProfileEndpoint { internal static RouteHandlerBuilder MapGetMeEndpoint(this IEndpointRouteBuilder endpoints) { - return endpoints.MapGet("/profile", async (ClaimsPrincipal user, IMediator mediator, CancellationToken cancellationToken) => + return endpoints.MapGet("/profile", async (ClaimsPrincipal user, HttpResponse response, IMediator mediator, CancellationToken cancellationToken) => { if (user.GetUserId() is not { } userId || string.IsNullOrEmpty(userId)) { throw new UnauthorizedException(); } - return TypedResults.Ok(await mediator.Send(new GetCurrentUserProfileQuery(userId), cancellationToken)); + var profile = await mediator.Send(new GetCurrentUserProfileQuery(userId), cancellationToken); + + // The profile is a full-representation resource: PUT /profile rewrites every field, so + // a caller editing a stale copy would blank whatever changed meanwhile. Publishing the + // stored concurrency token as a strong ETag lets that caller echo it back in If-Match + // and have the server reject the stale write. + if (!string.IsNullOrEmpty(profile.ConcurrencyStamp)) + { + response.Headers.ETag = new EntityTagHeaderValue($"\"{profile.ConcurrencyStamp}\"", isWeak: false).ToString(); + } + + return TypedResults.Ok(profile); }) .WithName("GetCurrentUserProfile") .WithSummary("Get current user profile") - .WithDescription("Retrieve the authenticated user's profile from the access token.") + .WithDescription("Retrieve the authenticated user's profile from the access token. The response carries a strong ETag — echo it in If-Match on PUT /identity/profile to reject a lost update.") .RequireAuthorization() .Produces(StatusCodes.Status200OK) .Produces(StatusCodes.Status401Unauthorized); diff --git a/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserCommandHandler.cs b/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserCommandHandler.cs index 9b6608e03a..61bbf01cf0 100644 --- a/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserCommandHandler.cs +++ b/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserCommandHandler.cs @@ -24,6 +24,7 @@ await _userService.UpdateAsync( command.PhoneNumber ?? string.Empty, command.Image!, command.DeleteCurrentImage, + command.ExpectedConcurrencyStamps, cancellationToken).ConfigureAwait(false); return Unit.Value; diff --git a/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserEndpoint.cs b/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserEndpoint.cs index 68751c8654..7ebeba09f8 100644 --- a/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserEndpoint.cs +++ b/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserEndpoint.cs @@ -6,6 +6,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Routing; +using Microsoft.Net.Http.Headers; using System.Security.Claims; namespace FSH.Modules.Identity.Features.v1.Users.UpdateUser; @@ -14,7 +15,7 @@ public static class UpdateUserEndpoint { internal static RouteHandlerBuilder MapUpdateUserEndpoint(this IEndpointRouteBuilder endpoints) { - return endpoints.MapPut("/profile", async ([FromBody] UpdateUserCommand request, ClaimsPrincipal user, IMediator mediator, CancellationToken cancellationToken) => + return endpoints.MapPut("/profile", async ([FromBody] UpdateUserCommand request, ClaimsPrincipal user, HttpRequest httpRequest, IMediator mediator, CancellationToken cancellationToken) => { if (user.GetUserId() is not { } userId || string.IsNullOrEmpty(userId)) { @@ -25,15 +26,54 @@ internal static RouteHandlerBuilder MapUpdateUserEndpoint(this IEndpointRouteBui // only, regardless of any id the caller supplied in the body. request.Id = userId; + // Header-derived, so it overwrites whatever the body carried. + request.ExpectedConcurrencyStamps = ReadExpectedConcurrencyStamps(httpRequest); + await mediator.Send(request, cancellationToken); return TypedResults.Ok(); }) .WithName("UpdateUserProfile") .WithSummary("Update user profile") .RequireAuthorization() - .WithDescription("Update profile details for the authenticated user. Any signed-in user may edit their own profile; no admin permission required.") + .WithDescription("Update profile details for the authenticated user. Any signed-in user may edit their own profile; no admin permission required. Echo the ETag from GET /identity/profile in If-Match and a stale full-representation update is rejected with 412 instead of silently overwriting a concurrent change.") .Produces(StatusCodes.Status200OK) .Produces(StatusCodes.Status401Unauthorized) - .Produces(StatusCodes.Status400BadRequest); + .Produces(StatusCodes.Status400BadRequest) + .Produces(StatusCodes.Status412PreconditionFailed); + } + + /// + /// Turns the request's If-Match header into the set of concurrency tokens the caller is + /// willing to overwrite. Returns when there is no precondition to + /// enforce: either the header is absent, or it is *, which asks only that the resource + /// exist — and it does, or the update answers 404 on its own. + /// + private static List? ReadExpectedConcurrencyStamps(HttpRequest request) + { + var ifMatch = request.Headers.IfMatch; + if (ifMatch.Count == 0) + { + return null; + } + + if (!EntityTagHeaderValue.TryParseStrictList(ifMatch, out var entityTags)) + { + // Answering 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. 400 names the bug instead. + throw new BadHttpRequestException("The If-Match header is not a valid entity-tag list."); + } + + if (entityTags.Contains(EntityTagHeaderValue.Any)) + { + return null; + } + + // If-Match mandates the strong comparison function, so a weak validator can never match. + // Dropping the weak entries leaves a list no stored token matches, which is exactly the + // 412 the RFC asks for. + return entityTags + .Where(entityTag => !entityTag.IsWeak) + .Select(entityTag => entityTag.Tag.ToString().Trim('"')) + .ToList(); } } \ No newline at end of file diff --git a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs index c96c90384b..b984fbd571 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs @@ -12,6 +12,7 @@ using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Options; +using System.Net; namespace FSH.Modules.Identity.Services; @@ -21,6 +22,7 @@ internal sealed class UserProfileService( IStorageService storageService, IMultiTenantContextAccessor multiTenantContextAccessor, IOptions originOptions, + IdentityErrorDescriber errorDescriber, IHttpContextAccessor httpContextAccessor) : IUserProfileService { private readonly Uri? _originUrl = originOptions.Value.OriginUrl; @@ -48,6 +50,7 @@ public async Task GetAsync(string userId, CancellationToken cancellatio EmailConfirmed = user.EmailConfirmed, PhoneNumber = user.PhoneNumber, TwoFactorEnabled = user.TwoFactorEnabled, + ConcurrencyStamp = user.ConcurrencyStamp, }; } @@ -75,12 +78,18 @@ public async Task> GetListAsync(CancellationToken cancellationToke return result; } - public async Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, CancellationToken cancellationToken = default) + public async Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, IReadOnlyList? expectedConcurrencyStamps, CancellationToken cancellationToken = default) { var user = await userManager.FindByIdAsync(userId); _ = user ?? throw new NotFoundException("user not found"); + // This is a full-representation update, so a caller working from a stale read would + // silently blank whatever changed since. The precondition is checked here, before the + // storage calls below: a rejected update must not leave an orphan upload behind, and on + // the deleteCurrentImage path it must not remove the avatar with no database change. + EnsureConcurrencyStampMatches(user, expectedConcurrencyStamps); + Uri imageUri = user.ImageUrl ?? null!; // image is optional: text-only edits forward a null FileUploadRequest, so guard before // dereferencing Data or the common no-image update path NREs. @@ -108,14 +117,47 @@ public async Task UpdateAsync(string userId, string firstName, string lastName, } var result = await userManager.UpdateAsync(user); - await signInManager.RefreshSignInAsync(user); if (!result.Succeeded) { + // Identity's store answers a lost race with ConcurrencyFailure instead of throwing, + // so it would otherwise surface as a generic 500. It is the same condition the + // If-Match check above reports, just detected one layer down: another writer landed + // between our read and our save. + if (result.Errors.Any(error => string.Equals(error.Code, errorDescriber.ConcurrencyFailure().Code, StringComparison.Ordinal))) + { + throw StaleProfileException(); + } + throw new CustomException("Update profile failed"); } + + await signInManager.RefreshSignInAsync(user); + } + + private static void EnsureConcurrencyStampMatches(FshUser user, IReadOnlyList? expectedConcurrencyStamps) + { + // A null list means the caller sent no If-Match and accepts the stored version as-is. + // ponytail: keep the precondition optional for backward compatibility; a future major can + // require it and answer 428 Precondition Required when the header is missing. + if (expectedConcurrencyStamps is null) + { + return; + } + + var storedStamp = user.ConcurrencyStamp; + if (storedStamp is null || !expectedConcurrencyStamps.Contains(storedStamp, StringComparer.Ordinal)) + { + throw StaleProfileException(); + } } + private static CustomException StaleProfileException() => + new( + "The profile changed since you loaded it. Reload it and apply your changes again.", + errors: null, + HttpStatusCode.PreconditionFailed); + public async Task SetImageUrlAsync(string userId, string? imageUrl, CancellationToken cancellationToken) { EnsureValidTenant(); diff --git a/src/Modules/Identity/Modules.Identity/Services/UserService.cs b/src/Modules/Identity/Modules.Identity/Services/UserService.cs index e11963512f..d2797a1cd0 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserService.cs @@ -55,8 +55,8 @@ public Task> GetListAsync(CancellationToken cancellationToken) public Task GetCountAsync(CancellationToken cancellationToken) => profileService.GetCountAsync(cancellationToken); - public Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, CancellationToken cancellationToken = default) - => profileService.UpdateAsync(userId, firstName, lastName, phoneNumber, image, deleteCurrentImage, cancellationToken); + public Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, IReadOnlyList? expectedConcurrencyStamps, CancellationToken cancellationToken = default) + => profileService.UpdateAsync(userId, firstName, lastName, phoneNumber, image, deleteCurrentImage, expectedConcurrencyStamps, cancellationToken); public Task ExistsWithEmailAsync(string email, string? exceptId = null, CancellationToken cancellationToken = default) => profileService.ExistsWithEmailAsync(email, exceptId, cancellationToken); diff --git a/src/Tests/Identity.Tests/Handlers/UpdateUserCommandHandlerTests.cs b/src/Tests/Identity.Tests/Handlers/UpdateUserCommandHandlerTests.cs index f89478916a..b7e6980f85 100644 --- a/src/Tests/Identity.Tests/Handlers/UpdateUserCommandHandlerTests.cs +++ b/src/Tests/Identity.Tests/Handlers/UpdateUserCommandHandlerTests.cs @@ -39,7 +39,31 @@ await _userService.Received(1).UpdateAsync( command.LastName ?? string.Empty, command.PhoneNumber ?? string.Empty, command.Image!, - command.DeleteCurrentImage); + command.DeleteCurrentImage, + command.ExpectedConcurrencyStamps); + } + + [Fact] + public async Task Handle_Should_ForwardExpectedConcurrencyStamps_When_CallerSentIfMatch() + { + // Arrange — the endpoint fills ExpectedConcurrencyStamps from the If-Match header; the + // handler has to carry it through or the precondition is silently dropped. + var command = _fixture.Create(); + var stamps = new List { "stamp-a", "stamp-b" }; + command.ExpectedConcurrencyStamps = stamps; + + // Act + await _sut.Handle(command, CancellationToken.None); + + // Assert + await _userService.Received(1).UpdateAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Is?>(actual => actual != null && actual.SequenceEqual(stamps))); } [Fact] @@ -66,7 +90,8 @@ await _userService.Received(1).UpdateAsync( string.Empty, string.Empty, null!, - true); + true, + null); } [Fact] @@ -83,7 +108,7 @@ public async Task Handle_Should_ThrowException_When_UserServiceThrows() // Arrange var command = _fixture.Create(); var expectedExceptionMessage = "Update failed"; - _userService.UpdateAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + _userService.UpdateAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>()) .Returns(x => throw new InvalidOperationException(expectedExceptionMessage)); // Act & Assert diff --git a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs index f999e85300..272f0fe7dd 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -113,6 +113,225 @@ public async Task UpdateProfile_Should_Return400_When_PhoneNumberExceedsMaxLengt #endregion + #region Optimistic concurrency (ETag / If-Match) + + [Fact] + public async Task GetProfile_Should_ReturnStrongETag_When_ProfileIsRead() + { + // Arrange + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-read"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + // Act + var response = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + + // Assert — If-Match mandates strong comparison, so the tag must not be weak. + response.StatusCode.ShouldBe(HttpStatusCode.OK); + response.Headers.ETag.ShouldNotBeNull(); + response.Headers.ETag!.IsWeak.ShouldBeFalse(); + response.Headers.ETag.Tag.ShouldStartWith("\""); + response.Headers.ETag.Tag.ShouldEndWith("\""); + } + + [Fact] + public async Task UpdateProfile_Should_PersistAndRotateETag_When_IfMatchMatches() + { + // Arrange + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-match"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + var etag = await ReadProfileETagAsync(userClient); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "Matched" }, etag); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.OK); + + var reread = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await reread.DeserializeAsync(); + dto.FirstName.ShouldBe("Matched"); + + // The token has to move, or a second save built from the same snapshot would be accepted. + reread.Headers.ETag!.ToString().ShouldNotBe(etag); + var replay = await PutProfileAsync(userClient, new { firstName = "Replayed" }, etag); + replay.StatusCode.ShouldBe(HttpStatusCode.PreconditionFailed); + } + + [Fact] + public async Task UpdateProfile_Should_Return412AndKeepConcurrentChange_When_IfMatchIsStale() + { + // Arrange — the lost update itself: a caller reads, someone else writes, and the caller's + // full-representation PUT would otherwise echo every old value back over that write. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-stale"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + var staleETag = await ReadProfileETagAsync(userClient); + + // A concurrent writer lands between that read and the write below. + var concurrent = await PutProfileAsync( + userClient, + new { firstName = "Concurrent", lastName = "Winner", phoneNumber = "5550001111" }, + ifMatch: null); + concurrent.StatusCode.ShouldBe(HttpStatusCode.OK); + + // Act — the first caller saves the snapshot it loaded before that write. + var response = await PutProfileAsync( + userClient, + new { firstName = "Stale", lastName = "Loser", phoneNumber = "5559998888" }, + staleETag); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.PreconditionFailed); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.FirstName.ShouldBe("Concurrent"); + dto.LastName.ShouldBe("Winner"); + dto.PhoneNumber.ShouldBe("5550001111"); + } + + [Fact] + public async Task UpdateProfile_Should_Succeed_When_IfMatchIsAny() + { + // Arrange — `*` asks only that the resource exist, so it must not block the update. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-any"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "Wildcard" }, "*"); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.OK); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.FirstName.ShouldBe("Wildcard"); + } + + [Fact] + public async Task UpdateProfile_Should_Return412_When_IfMatchIsWeak() + { + // Arrange — a weak validator can never satisfy the strong comparison If-Match requires, + // even when the tag it carries is the current one. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-weak"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + var etag = await ReadProfileETagAsync(userClient); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "Weak" }, $"W/{etag}"); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.PreconditionFailed); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.FirstName.ShouldNotBe("Weak"); + } + + [Theory] + [InlineData("not-an-entity-tag")] + [InlineData("\"unterminated")] + public async Task UpdateProfile_Should_Return400_When_IfMatchIsMalformed(string ifMatch) + { + // Arrange — a malformed header is the client's own bug. 412 would send it into a + // refetch-and-retry loop it can never win, so the request is rejected as a bad request. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-bad"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "Malformed" }, ifMatch); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.BadRequest); + } + + [Fact] + public async Task UpdateProfile_Should_Succeed_When_IfMatchListContainsCurrentETag() + { + // Arrange — If-Match takes a list; matching any entry is enough. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-list"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + var etag = await ReadProfileETagAsync(userClient); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "Listed" }, $"\"someone-elses-tag\", {etag}"); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.OK); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.FirstName.ShouldBe("Listed"); + } + + [Fact] + public async Task UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCurrentImageRequested() + { + // Arrange — the precondition is checked before the storage calls run. Checking it any later + // would delete the avatar (and orphan uploads) on a request that then answers 412 and + // changes nothing in the database. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-image"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + const string imageUrl = "https://cdn.example.com/avatars/keep-me.png"; + var setImage = await userClient.PutAsJsonAsync( + $"{TestConstants.IdentityBasePath}/profile/image", new { imageUrl }); + setImage.StatusCode.ShouldBe(HttpStatusCode.NoContent); + + var staleETag = await ReadProfileETagAsync(userClient); + var concurrent = await PutProfileAsync(userClient, new { firstName = "Concurrent" }, ifMatch: null); + concurrent.StatusCode.ShouldBe(HttpStatusCode.OK); + + // Act + var response = await PutProfileAsync( + userClient, + new { firstName = "Stale", deleteCurrentImage = true }, + staleETag); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.PreconditionFailed); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.ImageUrl.ShouldBe(imageUrl); + } + + private static async Task ReadProfileETagAsync(HttpClient client) + { + var response = await client.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + response.StatusCode.ShouldBe(HttpStatusCode.OK); + response.Headers.ETag.ShouldNotBeNull(); + return response.Headers.ETag!.ToString(); + } + + private static async Task PutProfileAsync(HttpClient client, object body, string? ifMatch) + { + using var request = new HttpRequestMessage( + HttpMethod.Put, + $"{TestConstants.IdentityBasePath}/profile") + { + Content = JsonContent.Create(body) + }; + + if (ifMatch is not null) + { + // Unvalidated on purpose: the malformed-header cases have to reach the server. + request.Headers.TryAddWithoutValidation("If-Match", ifMatch); + } + + return await client.SendAsync(request); + } + + #endregion + #region SetProfileImage (PUT /profile/image) [Fact] From c35d13d8b21e5bb81be7df3b96bbb2da6753f75d Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 07:35:13 -0300 Subject: [PATCH 03/13] 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. --- .../Integration.Tests/Tests/Users/UserProfileTests.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs index 272f0fe7dd..86a5123e02 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -274,9 +274,10 @@ public async Task UpdateProfile_Should_Succeed_When_IfMatchListContainsCurrentET [Fact] public async Task UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCurrentImageRequested() { - // Arrange — the precondition is checked before the storage calls run. Checking it any later - // would delete the avatar (and orphan uploads) on a request that then answers 412 and - // changes nothing in the database. + // Arrange — a rejected delete-my-avatar request must leave the profile exactly as it was. + // The precondition runs as the first statement after the user is loaded, ahead of the + // storage calls and of SetPhoneNumberAsync (which persists on its own), so a 412 cannot + // leave a half-applied update behind. using var adminClient = await _auth.CreateRootAdminClientAsync(); var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-image"); using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); @@ -302,6 +303,7 @@ public async Task UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCu var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); var dto = await profile.DeserializeAsync(); dto.ImageUrl.ShouldBe(imageUrl); + dto.FirstName.ShouldBe("Concurrent"); } private static async Task ReadProfileETagAsync(HttpClient client) From e4650e4649f4b25d730fe8a2c6eb3589e8ca3c7b Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 07:42:20 -0300 Subject: [PATCH 04/13] fix(dashboard): send If-Match when saving the profile, retry once on 412 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- clients/dashboard/src/api/identity.ts | 40 +++++- clients/dashboard/src/lib/api-client.ts | 11 +- .../dashboard/tests/settings/profile.spec.ts | 123 ++++++++++++++++-- 3 files changed, 160 insertions(+), 14 deletions(-) diff --git a/clients/dashboard/src/api/identity.ts b/clients/dashboard/src/api/identity.ts index 3297c8c18a..d2ee87eb31 100644 --- a/clients/dashboard/src/api/identity.ts +++ b/clients/dashboard/src/api/identity.ts @@ -1,4 +1,4 @@ -import { apiFetch } from "@/lib/api-client"; +import { apiFetch, ApiRequestError } from "@/lib/api-client"; import type { PagedResponse } from "@/api/catalog"; // ----------------------------- @@ -396,17 +396,53 @@ export type UpdateProfileInput = { phoneNumber?: string | null; }; +/** + * Reads the profile along with the ETag the server publishes for it. The tag is the + * profile's version marker: echoing it back in `If-Match` on the PUT below is what lets + * the server reject a save built from a snapshot someone else has since changed. + */ +async function readProfileWithETag(): Promise<{ profile: UserDto; etag: string | null }> { + let etag: string | null = null; + const profile = await apiFetch("/api/v1/identity/profile", { + onResponse: (response) => { + etag = response.headers.get("ETag"); + }, + }); + return { profile, etag }; +} + /** * Updates the authenticated user's profile. Maps to UpdateUserCommand * server-side. Image and email changes go through their own dedicated * endpoints — this is for the editable profile fields surfaced in * settings/profile. Reads the current profile first so unset optional * fields keep their existing values instead of being nulled. + * + * That read-modify-write is why the PUT carries `If-Match`: the server answers 412 when + * the profile moved in between, instead of accepting a full representation built from a + * stale copy and blanking the concurrent change. A 412 is retried once against a fresh + * read, because the token also rotates on writes the user never sees as profile edits (a + * password change, a failed sign-in, a new avatar) and surfacing those as a failed save + * would be noise. A second 412 means the profile is changing faster than this client can + * follow, and the error propagates. */ export async function updateMyProfile(input: UpdateProfileInput): Promise { - const profile = await getMyProfile(); + try { + await putProfileFromFreshRead(input); + } catch (error) { + if (error instanceof ApiRequestError && error.status === 412) { + await putProfileFromFreshRead(input); + return; + } + throw error; + } +} + +async function putProfileFromFreshRead(input: UpdateProfileInput): Promise { + const { profile, etag } = await readProfileWithETag(); await apiFetch(`/api/v1/identity/profile`, { method: "PUT", + headers: etag ? { "If-Match": etag } : undefined, body: JSON.stringify({ id: profile.id, firstName: input.firstName ?? profile.firstName ?? null, diff --git a/clients/dashboard/src/lib/api-client.ts b/clients/dashboard/src/lib/api-client.ts index 417eab0ca8..d83991b80f 100644 --- a/clients/dashboard/src/lib/api-client.ts +++ b/clients/dashboard/src/lib/api-client.ts @@ -73,6 +73,13 @@ type RequestInitEx = RequestInit & { * uploads) should override this explicitly. */ timeoutMs?: number; + /** + * Called with the final response before its body is read, so a caller can pick up a + * response header `apiFetch` does not model — the `ETag` on `GET /identity/profile`, + * which a later `PUT` echoes back in `If-Match`. Runs for error responses too, and + * must not throw. + */ + onResponse?: (response: Response) => void; }; const DEFAULT_TIMEOUT_MS = 30_000; @@ -156,7 +163,7 @@ export async function apiFetch( path: string, init: RequestInitEx = {}, ): Promise { - const { skipAuth, headers, timeoutMs = DEFAULT_TIMEOUT_MS, signal, ...rest } = init; + const { skipAuth, headers, timeoutMs = DEFAULT_TIMEOUT_MS, signal, onResponse, ...rest } = init; const mergedHeaders = new Headers(headers); if (!mergedHeaders.has("Content-Type") && rest.body && typeof rest.body === "string") { @@ -218,6 +225,8 @@ export async function apiFetch( } } + onResponse?.(response); + if (!response.ok) { const problem = await parseError(response); throw new ApiRequestError( diff --git a/clients/dashboard/tests/settings/profile.spec.ts b/clients/dashboard/tests/settings/profile.spec.ts index 91b97df567..01ef2fff3a 100644 --- a/clients/dashboard/tests/settings/profile.spec.ts +++ b/clients/dashboard/tests/settings/profile.spec.ts @@ -2,20 +2,22 @@ import { expect, test } from "@playwright/test"; import { mockJsonResponse, mockProblemDetails } from "../helpers/api-mocks"; import { seedAuthedSession, TEST_USER } from "../helpers/auth-seed"; +const PROFILE = { + id: TEST_USER.sub, + userName: "alice", + email: TEST_USER.email, + firstName: TEST_USER.firstName, + lastName: TEST_USER.lastName, + phoneNumber: "", + isActive: true, + emailConfirmed: true, + twoFactorEnabled: false, +}; + // All settings tests need an authed session and a mocked profile fetch. test.beforeEach(async ({ page }) => { await seedAuthedSession(page, TEST_USER); - await mockJsonResponse(page, "**/api/v1/identity/profile", { - id: TEST_USER.sub, - userName: "alice", - email: TEST_USER.email, - firstName: TEST_USER.firstName, - lastName: TEST_USER.lastName, - phoneNumber: "", - isActive: true, - emailConfirmed: true, - twoFactorEnabled: false, - }); + await mockJsonResponse(page, "**/api/v1/identity/profile", PROFILE); }); test.describe("settings/profile — wired to PUT /identity/profile", () => { @@ -107,6 +109,105 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { await expect(page.getByText(/first name cannot be empty/i)).toBeVisible(); }); + // The dashboard talks to the API cross-origin in dev, and `ETag` is not a CORS-safelisted + // response header — the browser hides it from JS unless the server also sends + // `Access-Control-Expose-Headers: ETag`. These mocks send it for the same reason the API + // has to: without it the client reads `null` and silently stops sending `If-Match`. + const ETAG_CORS_HEADERS = { + "Content-Type": "application/json", + "Access-Control-Expose-Headers": "ETag", + } as const; + + test("echoes the profile ETag back as If-Match on save", async ({ page }) => { + const etag = '"stamp-1"'; + await page.route("**/api/v1/identity/profile", async (route) => { + if (route.request().method() === "PUT") { + await route.fulfill({ + status: 200, + headers: { "Content-Type": "application/json" }, + body: '""', + }); + return; + } + await route.fulfill({ + status: 200, + headers: { ...ETAG_CORS_HEADERS, ETag: etag }, + body: JSON.stringify(PROFILE), + }); + }); + + await page.goto("/settings/profile"); + await expect(page.getByLabel("First name")).toHaveValue("Alice"); + + await page.getByLabel("First name").fill("Alicia"); + + const putReqPromise = page.waitForRequest( + (req) => + req.url().includes("/api/v1/identity/profile") && + req.method() === "PUT" && + !req.url().includes("/image"), + { timeout: 5_000 }, + ); + await page.getByRole("button", { name: /save changes/i }).click(); + const putReq = await putReqPromise; + + // Without this the server cannot tell a deliberate overwrite from a lost update. + expect(putReq.headers()["if-match"]).toBe(etag); + }); + + test("refetches and retries once when the save is rejected with 412", async ({ page }) => { + // The token also rotates on writes the user never sees as profile edits (a password + // change, a failed sign-in, a new avatar), so a single 412 has to resolve itself + // against a fresh read instead of surfacing as a failed save. + const sentIfMatch: string[] = []; + let getCount = 0; + + await page.route("**/api/v1/identity/profile", async (route) => { + const request = route.request(); + if (request.method() === "PUT") { + sentIfMatch.push(request.headers()["if-match"] ?? ""); + if (sentIfMatch.length === 1) { + await route.fulfill({ + status: 412, + headers: { "Content-Type": "application/problem+json" }, + body: JSON.stringify({ + status: 412, + title: "CustomException", + detail: "The profile changed since you loaded it.", + }), + }); + return; + } + await route.fulfill({ + status: 200, + headers: { "Content-Type": "application/json" }, + body: '""', + }); + return; + } + + // Every read hands out a fresh token, so the retry provably carries a re-read one. + getCount += 1; + await route.fulfill({ + status: 200, + headers: { ...ETAG_CORS_HEADERS, ETag: `"stamp-${getCount}"` }, + body: JSON.stringify(PROFILE), + }); + }); + + await page.goto("/settings/profile"); + await expect(page.getByLabel("First name")).toHaveValue("Alice"); + + await page.getByLabel("First name").fill("Alicia"); + await page.getByRole("button", { name: /save changes/i }).click(); + + await expect(page.getByText(/profile saved/i)).toBeVisible(); + await expect(page.getByText(/save failed/i)).toBeHidden(); + expect(sentIfMatch).toHaveLength(2); + expect(sentIfMatch[0]).not.toBe(""); + expect(sentIfMatch[1]).not.toBe(sentIfMatch[0]); + }); + test("Reset button reverts edits to the original profile values", async ({ page }) => { await page.goto("/settings/profile"); await expect(page.getByLabel("First name")).toHaveValue("Alice"); From acd3d5d521423c0a2ae8ae74dd07e2ef68b6a08b Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 07:56:55 -0300 Subject: [PATCH 05/13] 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 --- .../Tests/Users/UserProfileTests.cs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs index 86a5123e02..b9354e6c10 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -306,6 +306,33 @@ public async Task UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCu dto.FirstName.ShouldBe("Concurrent"); } + [Fact(Skip = "Blocked on CORS: FSH.Framework.Web.Cors never calls WithExposedHeaders, so a browser hides the ETag from JS on a cross-origin call and the precondition silently degrades to the old lost-update behaviour. Drop the Skip once ETag is exposed.")] + public async Task GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsRead() + { + // Arrange — ETag is not a CORS-safelisted response header, so the contract only reaches a + // front-end if the server also lists it in Access-Control-Expose-Headers. Asserted here + // rather than left as a comment: the front-end specs mock the header, so nothing else in + // the suite notices when the server stops sending it. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-cors"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + using var request = new HttpRequestMessage(HttpMethod.Get, $"{TestConstants.IdentityBasePath}/profile"); + request.Headers.TryAddWithoutValidation("Origin", "http://localhost:5174"); + + // Act + var response = await userClient.SendAsync(request); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.OK); + response.Headers.ETag.ShouldNotBeNull(); + response.Headers.TryGetValues("Access-Control-Expose-Headers", out var exposedHeaders).ShouldBeTrue(); + exposedHeaders! + .SelectMany(value => value.Split(',')) + .Select(value => value.Trim()) + .ShouldContain(value => string.Equals(value, "ETag", StringComparison.OrdinalIgnoreCase)); + } + private static async Task ReadProfileETagAsync(HttpClient client) { var response = await client.GetAsync($"{TestConstants.IdentityBasePath}/profile"); From 7daadf535701d5919975fbb2388e8842072362bd Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:02:30 -0300 Subject: [PATCH 06/13] 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 --- .../dashboard/tests/settings/profile.spec.ts | 6 ++- src/BuildingBlocks/Web/Cors/Extensions.cs | 7 +++ .../appsettings.Production.json | 2 +- src/Host/FSH.Starter.Api/appsettings.json | 2 +- .../Framework.Tests/Web/CorsPolicyTests.cs | 46 +++++++++++++++++++ .../Tests/Users/UserProfileTests.cs | 2 +- 6 files changed, 60 insertions(+), 5 deletions(-) create mode 100644 src/Tests/Framework.Tests/Web/CorsPolicyTests.cs diff --git a/clients/dashboard/tests/settings/profile.spec.ts b/clients/dashboard/tests/settings/profile.spec.ts index 01ef2fff3a..c48b25439f 100644 --- a/clients/dashboard/tests/settings/profile.spec.ts +++ b/clients/dashboard/tests/settings/profile.spec.ts @@ -111,8 +111,10 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { // The dashboard talks to the API cross-origin in dev, and `ETag` is not a CORS-safelisted // response header — the browser hides it from JS unless the server also sends - // `Access-Control-Expose-Headers: ETag`. These mocks send it for the same reason the API - // has to: without it the client reads `null` and silently stops sending `If-Match`. + // `Access-Control-Expose-Headers: ETag`. These mocks mirror what the CORS policy now sends; + // without it the client reads `null` and silently stops sending `If-Match`. The server side of + // that contract is asserted by `GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsRead`, + // since a mock alone would keep passing if the policy stopped exposing the header. const ETAG_CORS_HEADERS = { "Content-Type": "application/json", "Access-Control-Expose-Headers": "ETag", diff --git a/src/BuildingBlocks/Web/Cors/Extensions.cs b/src/BuildingBlocks/Web/Cors/Extensions.cs index 6475dc844e..49e1e30177 100644 --- a/src/BuildingBlocks/Web/Cors/Extensions.cs +++ b/src/BuildingBlocks/Web/Cors/Extensions.cs @@ -2,6 +2,7 @@ using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; +using Microsoft.Net.Http.Headers; using System; using AspNetCorsOptions = Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions; @@ -53,6 +54,12 @@ public static IServiceCollection AddHeroCors( .WithMethods(settings.AllowedMethods) .AllowCredentials(); } + + // `ETag` is not a CORS-safelisted response header, so a browser hides it from JS on any + // cross-origin call — and a front-end that cannot read the validator cannot send + // `If-Match`, which degrades an optimistic-concurrency endpoint back to a lost update. + // Exposed for both policies: the header carries no data of its own, only a validator. + builder.WithExposedHeaders(HeaderNames.ETag); }); }); }); diff --git a/src/Host/FSH.Starter.Api/appsettings.Production.json b/src/Host/FSH.Starter.Api/appsettings.Production.json index 332724534b..869b1c5ffc 100644 --- a/src/Host/FSH.Starter.Api/appsettings.Production.json +++ b/src/Host/FSH.Starter.Api/appsettings.Production.json @@ -59,7 +59,7 @@ "CorsOptions": { "AllowAll": false, "AllowedOrigins": [], - "AllowedHeaders": [ "content-type", "authorization" ], + "AllowedHeaders": [ "content-type", "authorization", "if-match" ], "AllowedMethods": [ "GET", "POST", "PUT", "DELETE" ] }, "JwtOptions": { diff --git a/src/Host/FSH.Starter.Api/appsettings.json b/src/Host/FSH.Starter.Api/appsettings.json index 293fdfebb6..527ed10c93 100644 --- a/src/Host/FSH.Starter.Api/appsettings.json +++ b/src/Host/FSH.Starter.Api/appsettings.json @@ -100,7 +100,7 @@ "http://localhost:5173", "http://localhost:5174" ], - "AllowedHeaders": [ "content-type", "authorization" ], + "AllowedHeaders": [ "content-type", "authorization", "if-match" ], "AllowedMethods": [ "GET", "POST", "PUT", "DELETE" ] }, "JwtOptions": { diff --git a/src/Tests/Framework.Tests/Web/CorsPolicyTests.cs b/src/Tests/Framework.Tests/Web/CorsPolicyTests.cs new file mode 100644 index 0000000000..f17ece3e0f --- /dev/null +++ b/src/Tests/Framework.Tests/Web/CorsPolicyTests.cs @@ -0,0 +1,46 @@ +using FSH.Framework.Web.Cors; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using AspNetCorsOptions = Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions; + +namespace Framework.Tests.Web; + +public sealed class CorsPolicyTests +{ + private const string PolicyName = "FSHCorsPolicy"; + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void Policy_Should_ExposeETag_When_Built(bool allowAll) + { + // Arrange — ETag is not a CORS-safelisted response header, so a front-end can only read the + // concurrency validator (and answer with If-Match) if the policy exposes it explicitly. + // Both branches are covered: the restricted one builds from configured lists, and neither + // AllowAnyHeader nor WithHeaders implies exposure. + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["CorsOptions:AllowAll"] = allowAll ? "true" : "false", + ["CorsOptions:AllowedOrigins:0"] = "https://app.example.com", + ["CorsOptions:AllowedHeaders:0"] = "content-type", + ["CorsOptions:AllowedMethods:0"] = "GET" + }) + .Build(); + + var services = new ServiceCollection(); + services.AddHeroCors(configuration); + + // Act + var policy = services + .BuildServiceProvider() + .GetRequiredService>() + .Value + .GetPolicy(PolicyName); + + // Assert + policy.ShouldNotBeNull(); + policy!.ExposedHeaders.ShouldContain("ETag"); + } +} diff --git a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs index b9354e6c10..937ff727e2 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -306,7 +306,7 @@ public async Task UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCu dto.FirstName.ShouldBe("Concurrent"); } - [Fact(Skip = "Blocked on CORS: FSH.Framework.Web.Cors never calls WithExposedHeaders, so a browser hides the ETag from JS on a cross-origin call and the precondition silently degrades to the old lost-update behaviour. Drop the Skip once ETag is exposed.")] + [Fact] public async Task GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsRead() { // Arrange — ETag is not a CORS-safelisted response header, so the contract only reaches a From 04979928a8bc1949275b4c25c5d4f6185364765e Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 14 Sep 2026 14:44:42 -0300 Subject: [PATCH 07/13] 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. --- src/Directory.Packages.props | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 7674befa8f..89162470ad 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -9,7 +9,8 @@ - + + @@ -122,9 +123,10 @@ - - - + + + + From 7d6ab655bfa7f7707adce6868886c25f78af2a08 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 14 Sep 2026 14:45:29 -0300 Subject: [PATCH 08/13] 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`. --- deploy/docker/README.md | 6 +++--- deploy/docker/docker-compose.yml | 3 ++- src/Host/FSH.Starter.AppHost/AppHost.cs | 3 +++ .../Infrastructure/MiddlewareWebApplicationFactory.cs | 3 ++- .../Infrastructure/FshWebApplicationFactory.cs | 3 ++- 5 files changed, 12 insertions(+), 6 deletions(-) diff --git a/deploy/docker/README.md b/deploy/docker/README.md index bcb1304593..0219164b7c 100644 --- a/deploy/docker/README.md +++ b/deploy/docker/README.md @@ -8,9 +8,9 @@ This brings up the full stack on a single host: | `admin` | `fsh/admin:local` | `FSH_ADMIN_PORT` (default 8081) | Operator console (nginx + React) | | `dashboard` | `fsh/dashboard:local` | `FSH_DASHBOARD_PORT` (default 8082) | Tenant dashboard (nginx + React) | | `migrator` | `fsh/dbmigrator:local` | — | One-shot: applies EF migrations + seeds the root tenant + creates the default admin user | -| `postgres` | `postgres:17-alpine` | (internal) | Identity, tenant catalog, module schemas | -| `redis` | `redis:7-alpine` | (internal) | HybridCache L2, Data Protection keys, idempotency store | -| `minio` | `minio/minio:latest` | (internal) | S3-compatible blob store for the Files module | +| `postgres` | `postgres:18-alpine` | (internal) | Identity, tenant catalog, module schemas | +| `redis` | `valkey/valkey:9.1.0-alpine` | (internal) | HybridCache L2, Data Protection keys, idempotency store | +| `minio` | `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` | (internal) | S3-compatible blob store for the Files module | The compose file does **not** include a reverse proxy or TLS terminator. You bring your own edge — Cloudflare Tunnel, AWS ALB, Tailscale Funnel, your existing nginx, anything that can route a TLS subdomain to a host:port on this machine. diff --git a/deploy/docker/docker-compose.yml b/deploy/docker/docker-compose.yml index d43c744f5b..9457a61232 100644 --- a/deploy/docker/docker-compose.yml +++ b/deploy/docker/docker-compose.yml @@ -54,7 +54,8 @@ services: # - "6379:6379" minio: - image: minio/minio:latest + # quay.io: minio/minio is gone from Docker Hub. Tag pinned; quay stopped moving :latest. + image: quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z container_name: fsh-minio restart: unless-stopped command: ["server", "/data", "--console-address", ":9001"] diff --git a/src/Host/FSH.Starter.AppHost/AppHost.cs b/src/Host/FSH.Starter.AppHost/AppHost.cs index e7a70abd05..fb3506ab42 100644 --- a/src/Host/FSH.Starter.AppHost/AppHost.cs +++ b/src/Host/FSH.Starter.AppHost/AppHost.cs @@ -52,7 +52,10 @@ var minioUser = builder.AddParameter("minio-user", "minioadmin"); var minioPassword = builder.AddParameter("minio-password", "minioadmin", secret: true); +// quay.io: minio/minio is gone from Docker Hub. Tag pinned; quay stopped moving :latest. var minio = builder.AddContainer("minio", "minio/minio") + .WithImageRegistry("quay.io") + .WithImageTag("RELEASE.2025-09-07T16-13-09Z") .WithArgs("server", "/data", "--console-address", ":9001") .WithHttpEndpoint(port: 9000, targetPort: 9000, name: "api") .WithHttpEndpoint(port: 9001, targetPort: 9001, name: "console") diff --git a/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs b/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs index 4c2939c454..e8b7898023 100644 --- a/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs +++ b/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs @@ -55,7 +55,8 @@ public sealed class MiddlewareWebApplicationFactory : WebApplicationFactory, I .WithCleanUp(true) .Build(); - private readonly MinioContainer _minio = new MinioBuilder("minio/minio:latest") + // quay.io: minio/minio is gone from Docker Hub. Tag pinned; quay stopped moving :latest. + private readonly MinioContainer _minio = new MinioBuilder("quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z") .WithUsername(MinioAccessKey) .WithPassword(MinioSecretKey) .WithAutoRemove(true) From 957cf0464c043cc156af6d753c6a7b4d27eb5fd9 Mon Sep 17 00:00:00 2001 From: "Marcelo M. M." <4993482+marcelo-maciel@users.noreply.github.com> Date: Thu, 17 Sep 2026 15:22:29 -0300 Subject: [PATCH 09/13] fix(dashboard): take the profile ETag from the read the form was seeded with MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- clients/dashboard/src/api/identity.ts | 63 ++++++------ .../src/components/layout/topbar.tsx | 10 +- .../dashboard/src/pages/settings/profile.tsx | 89 +++++++++++++---- .../dashboard/src/pages/settings/security.tsx | 7 +- .../dashboard/tests/settings/profile.spec.ts | 99 ++++++++++++++++--- 5 files changed, 189 insertions(+), 79 deletions(-) diff --git a/clients/dashboard/src/api/identity.ts b/clients/dashboard/src/api/identity.ts index d2ee87eb31..cbd4db8b05 100644 --- a/clients/dashboard/src/api/identity.ts +++ b/clients/dashboard/src/api/identity.ts @@ -1,4 +1,4 @@ -import { apiFetch, ApiRequestError } from "@/lib/api-client"; +import { apiFetch } from "@/lib/api-client"; import type { PagedResponse } from "@/api/catalog"; // ----------------------------- @@ -146,10 +146,6 @@ export async function getMyPermissions(): Promise { return (await apiFetch(`/api/v1/identity/permissions`)) ?? []; } -export async function getMyProfile(): Promise { - return apiFetch("/api/v1/identity/profile"); -} - export async function registerUser(input: RegisterUserInput): Promise { return apiFetch(`/api/v1/identity/register`, { method: "POST", @@ -391,17 +387,28 @@ export async function endImpersonation(): Promise { // ----------------------------- export type UpdateProfileInput = { - firstName?: string | null; - lastName?: string | null; - phoneNumber?: string | null; + /** + * The profile the form was seeded from, and the ETag that read carried. Both come from the + * caller rather than from a read inside the save: the lost update this guards against happens + * between the moment the user saw the values and the moment they press save, so a tag fetched + * inside the save has no chance of being stale and no chance of catching anything. + */ + profile: UserDto; + expectedETag: string | null; + firstName: string | null; + lastName: string | null; + phoneNumber: string | null; }; /** * Reads the profile along with the ETag the server publishes for it. The tag is the * profile's version marker: echoing it back in `If-Match` on the PUT below is what lets * the server reject a save built from a snapshot someone else has since changed. + * + * Use this as the read that populates an edit form, and hand the tag it returns back to + * {@link updateMyProfile}. A tag read at save time cannot detect anything. */ -async function readProfileWithETag(): Promise<{ profile: UserDto; etag: string | null }> { +export async function getMyProfileWithETag(): Promise<{ profile: UserDto; etag: string | null }> { let etag: string | null = null; const profile = await apiFetch("/api/v1/identity/profile", { onResponse: (response) => { @@ -412,37 +419,23 @@ async function readProfileWithETag(): Promise<{ profile: UserDto; etag: string | } /** - * Updates the authenticated user's profile. Maps to UpdateUserCommand - * server-side. Image and email changes go through their own dedicated - * endpoints — this is for the editable profile fields surfaced in - * settings/profile. Reads the current profile first so unset optional - * fields keep their existing values instead of being nulled. + * Updates the authenticated user's profile. Maps to UpdateUserCommand server-side. Image and + * email changes go through their own dedicated endpoints — this is for the editable profile + * fields surfaced in settings/profile. The unedited fields come off `input.profile`, the copy + * the form was seeded from, so they keep their values instead of being nulled. * - * That read-modify-write is why the PUT carries `If-Match`: the server answers 412 when - * the profile moved in between, instead of accepting a full representation built from a - * stale copy and blanking the concurrent change. A 412 is retried once against a fresh - * read, because the token also rotates on writes the user never sees as profile edits (a - * password change, a failed sign-in, a new avatar) and surfacing those as a failed save - * would be noise. A second 412 means the profile is changing faster than this client can - * follow, and the error propagates. + * The PUT carries `If-Match` with that same copy's ETag, so the server answers 412 when the + * profile moved after the user last saw it, instead of accepting a full representation built + * from a stale snapshot and blanking the concurrent change. A 412 is NOT retried here: the only + * body this function has is the one the user typed against the old values, and resending it + * against a fresh tag performs exactly the overwrite the 412 exists to prevent. The caller + * decides — normally by telling the user the profile changed and asking for a deliberate re-save. */ export async function updateMyProfile(input: UpdateProfileInput): Promise { - try { - await putProfileFromFreshRead(input); - } catch (error) { - if (error instanceof ApiRequestError && error.status === 412) { - await putProfileFromFreshRead(input); - return; - } - throw error; - } -} - -async function putProfileFromFreshRead(input: UpdateProfileInput): Promise { - const { profile, etag } = await readProfileWithETag(); + const { profile, expectedETag } = input; await apiFetch(`/api/v1/identity/profile`, { method: "PUT", - headers: etag ? { "If-Match": etag } : undefined, + headers: expectedETag ? { "If-Match": expectedETag } : undefined, body: JSON.stringify({ id: profile.id, firstName: input.firstName ?? profile.firstName ?? null, diff --git a/clients/dashboard/src/components/layout/topbar.tsx b/clients/dashboard/src/components/layout/topbar.tsx index 0383d28e30..d78dfdc19a 100644 --- a/clients/dashboard/src/components/layout/topbar.tsx +++ b/clients/dashboard/src/components/layout/topbar.tsx @@ -36,7 +36,7 @@ import { DropdownMenuTrigger, } from "@/components/ui/dropdown-menu"; import { Avatar } from "@/components/ui/avatar"; -import { getMyProfile } from "@/api/identity"; +import { getMyProfileWithETag } from "@/api/identity"; import { useAuth } from "@/auth/use-auth"; import { useSseStatus } from "@/sse/sse-context"; import { useTheme } from "@/components/theme/theme-provider"; @@ -149,13 +149,15 @@ function SimpleMenuItem({ export function Topbar() { const { user, logout } = useAuth(); // Shared with the Profile settings page (same query key), so changing the - // photo there invalidates this and the topbar avatar updates live. + // photo there invalidates this and the topbar avatar updates live. That sharing is also why + // this reads through the ETag-carrying variant: one query key must hold one shape, and the + // settings page needs the tag to save against. const { data: profile } = useQuery({ queryKey: ["identity", "me"], - queryFn: getMyProfile, + queryFn: getMyProfileWithETag, staleTime: 5 * 60 * 1000, }); - const avatarUrl = profile?.imageUrl ?? null; + const avatarUrl = profile?.profile.imageUrl ?? null; const { status: sseStatus, eventCount } = useSseStatus(); const { mode, setMode } = useTheme(); const { setOpen: setPaletteOpen } = useCommandPalette(); diff --git a/clients/dashboard/src/pages/settings/profile.tsx b/clients/dashboard/src/pages/settings/profile.tsx index 373b3212a0..bd8e8d5831 100644 --- a/clients/dashboard/src/pages/settings/profile.tsx +++ b/clients/dashboard/src/pages/settings/profile.tsx @@ -3,7 +3,8 @@ import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { Camera, Fingerprint, UserCircle2 } from "lucide-react"; import { toast } from "sonner"; import { useAuth } from "@/auth/use-auth"; -import { getMyProfile, setProfileImage, updateMyProfile } from "@/api/identity"; +import { getMyProfileWithETag, setProfileImage, updateMyProfile } from "@/api/identity"; +import type { UserDto } from "@/api/identity"; import { ApiRequestError } from "@/lib/api-client"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; @@ -19,11 +20,18 @@ export function ProfileSettings() { const profileQuery = useQuery({ queryKey: PROFILE_KEY, - queryFn: getMyProfile, + queryFn: getMyProfileWithETag, }); - const profile = profileQuery.data; + const profile = profileQuery.data?.profile; const loading = profileQuery.isLoading; + + // The version this form is editing against, captured when the form is seeded. Deliberately a + // ref and not `profileQuery.data`: a background refetch would otherwise move it to a version + // the user never saw, and the save would carry an If-Match that matches whatever someone else + // just wrote — silently overwriting it, which is exactly what the ETag exists to prevent. It + // moves only on a deliberate step: a successful save, or the user being told about a conflict. + const editingVersionRef = useRef<{ profile: UserDto; etag: string | null } | null>(null); const [firstName, setFirstName] = useState(""); const [lastName, setLastName] = useState(""); const [phone, setPhone] = useState(""); @@ -35,29 +43,55 @@ export function ProfileSettings() { const seededRef = useRef(false); useEffect(() => { if (seededRef.current) return; - if (profile) { - setFirstName(profile.firstName ?? ""); - setLastName(profile.lastName ?? ""); - setPhone(profile.phoneNumber ?? ""); + if (profileQuery.data) { + const { profile: seeded } = profileQuery.data; + setFirstName(seeded.firstName ?? ""); + setLastName(seeded.lastName ?? ""); + setPhone(seeded.phoneNumber ?? ""); + editingVersionRef.current = profileQuery.data; seededRef.current = true; } else if (user && loading) { setFirstName(user.name?.split(" ")[0] ?? ""); setLastName(user.name?.split(" ").slice(1).join(" ") ?? ""); } - }, [profile, user, loading]); + }, [profileQuery.data, user, loading]); + + // Re-reads the profile and adopts it as the version the form edits against, so the next save + // carries a tag the server will accept. + const adoptCurrentVersion = async () => { + const fresh = await queryClient.fetchQuery({ + queryKey: PROFILE_KEY, + queryFn: getMyProfileWithETag, + // Must reach the network. The client's default staleTime would hand back the cached copy, + // and the cached copy carries the very tag the server just rejected. + staleTime: 0, + }); + editingVersionRef.current = fresh; + return fresh; + }; const saveMutation = useMutation({ - mutationFn: () => - updateMyProfile({ - firstName: firstName.trim() || null, - lastName: lastName.trim() || null, - phoneNumber: phone.trim() || null, - }), + mutationFn: updateMyProfile, onSuccess: () => { toast.success("Profile saved"); - queryClient.invalidateQueries({ queryKey: PROFILE_KEY }); + // The save moved the profile on, so the tag the form holds is spent: adopt the new one or + // a second save in the same sitting would 412 against the user's own write. + void adoptCurrentVersion(); }, - onError: (err: unknown) => { + onError: async (err: unknown) => { + // 412 means someone else wrote the profile after this form was seeded. Do NOT resend: the + // only body available is the one typed against the old values, and pushing it through + // against a fresh tag performs the overwrite the 412 just prevented. Keep the user's + // edits on screen, adopt the current version, and let them decide whether to save again. + if (err instanceof ApiRequestError && err.status === 412) { + await adoptCurrentVersion(); + toast.warning("Profile changed elsewhere", { + description: + "Someone updated this profile while you were editing. Review your changes and save again to apply them.", + }); + return; + } + const message = err instanceof ApiRequestError ? err.problem?.detail ?? err.problem?.title ?? err.message @@ -68,7 +102,17 @@ export function ProfileSettings() { const onSubmit = (e: FormEvent) => { e.preventDefault(); - saveMutation.mutate(); + const editing = editingVersionRef.current; + if (!editing) return; + // Everything the save needs travels through mutate(), never through state the callbacks + // close over: the values that go out must be the ones on screen when the button was pressed. + saveMutation.mutate({ + profile: editing.profile, + expectedETag: editing.etag, + firstName: firstName.trim() || null, + lastName: lastName.trim() || null, + phoneNumber: phone.trim() || null, + }); }; const onReset = () => { @@ -84,6 +128,9 @@ export function ProfileSettings() { (profile?.firstName ?? "") !== firstName || (profile?.lastName ?? "") !== lastName || (profile?.phoneNumber ?? "") !== phone; + // A save carries the unedited fields and the version tag off the profile read, so until that + // read lands there is nothing to save against. Disabled rather than silently doing nothing. + const canSave = profileQuery.isSuccess; const imageMutation = useMutation({ mutationFn: (url: string | null) => setProfileImage(url), @@ -108,8 +155,8 @@ export function ProfileSettings() { className="flex items-start gap-2 rounded-lg border border-[oklch(from_var(--color-destructive)_l_c_h_/_0.30)] bg-[oklch(from_var(--color-destructive)_l_c_h_/_0.06)] px-3 py-2 text-[13px] text-[var(--color-destructive)]" > - Couldn't load your profile. Showing details from your session; - saved changes may not reflect the latest server state. + Couldn't load your profile. Showing details from your session; saving is disabled + until the profile loads, because a save has to carry the version it was read at. )} @@ -137,12 +184,12 @@ export function ProfileSettings() { type="button" variant="ghost" onClick={onReset} - disabled={saving || !dirty} + disabled={saving || !dirty || !canSave} size="sm" > Reset - diff --git a/clients/dashboard/src/pages/settings/security.tsx b/clients/dashboard/src/pages/settings/security.tsx index b9dc17ddc6..63ecfef218 100644 --- a/clients/dashboard/src/pages/settings/security.tsx +++ b/clients/dashboard/src/pages/settings/security.tsx @@ -41,7 +41,7 @@ import { changePassword, disableTwoFactor, enrollTwoFactor, - getMyProfile, + getMyProfileWithETag, verifyEnrollTwoFactor, type TwoFactorEnrollmentResponse, } from "@/api/identity"; @@ -95,8 +95,9 @@ function apiErrorMessage(err: unknown, fallback: string): string { export function SecuritySettings() { const queryClient = useQueryClient(); - const profileQuery = useQuery({ queryKey: PROFILE_KEY, queryFn: getMyProfile }); - const twoFactorEnabled = profileQuery.data?.twoFactorEnabled ?? false; + // Same key as the topbar and the profile page, so same shape: the read carries the ETag. + const profileQuery = useQuery({ queryKey: PROFILE_KEY, queryFn: getMyProfileWithETag }); + const twoFactorEnabled = profileQuery.data?.profile.twoFactorEnabled ?? false; const sessionsQuery = useQuery({ queryKey: ["identity", "sessions", "me"], diff --git a/clients/dashboard/tests/settings/profile.spec.ts b/clients/dashboard/tests/settings/profile.spec.ts index c48b25439f..b190026ab6 100644 --- a/clients/dashboard/tests/settings/profile.spec.ts +++ b/clients/dashboard/tests/settings/profile.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from "@playwright/test"; -import { mockJsonResponse, mockProblemDetails } from "../helpers/api-mocks"; +import { mockJsonResponse } from "../helpers/api-mocks"; import { seedAuthedSession, TEST_USER } from "../helpers/auth-seed"; const PROFILE = { @@ -94,9 +94,23 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { }); test("surfaces a destructive toast on server error", async ({ page }) => { - await mockProblemDetails(page, "**/api/v1/identity/profile", 400, { - title: "Validation failed", - detail: "First name cannot be empty.", + // The 400 belongs to the PUT. The GET has to keep working: the save is built from the + // profile that read returned, so failing it would test "cannot save yet", not "save failed". + await page.route("**/api/v1/identity/profile", async (route) => { + if (route.request().method() !== "PUT") { + await route.fallback(); + return; + } + await route.fulfill({ + status: 400, + headers: { "Content-Type": "application/problem+json" }, + body: JSON.stringify({ + type: "https://httpstatuses.io/400", + title: "Validation failed", + status: 400, + detail: "First name cannot be empty.", + }), + }); }); await page.goto("/settings/profile"); @@ -157,18 +171,65 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { expect(putReq.headers()["if-match"]).toBe(etag); }); - test("refetches and retries once when the save is rejected with 412", async ({ page }) => { - // The token also rotates on writes the user never sees as profile edits (a password - // change, a failed sign-in, a new avatar), so a single 412 has to resolve itself - // against a fresh read instead of surfacing as a failed save. + test("the If-Match comes from the read that seeded the form, not from a read at save time", async ({ + page, + }) => { + // The lost update happens between the user seeing the values and pressing save. A tag read + // inside the save is always current by construction, so it matches whatever the other writer + // just stored and the overwrite goes through. Here the server moves on after the form is + // seeded: the save must still carry the seeded tag, which is what lets the server say 412. const sentIfMatch: string[] = []; - let getCount = 0; + let currentStamp = 1; await page.route("**/api/v1/identity/profile", async (route) => { const request = route.request(); if (request.method() === "PUT") { sentIfMatch.push(request.headers()["if-match"] ?? ""); - if (sentIfMatch.length === 1) { + await route.fulfill({ + status: 412, + headers: { "Content-Type": "application/problem+json" }, + body: JSON.stringify({ + status: 412, + title: "CustomException", + detail: "The profile changed since you loaded it.", + }), + }); + return; + } + + await route.fulfill({ + status: 200, + headers: { ...ETAG_CORS_HEADERS, ETag: `"stamp-${currentStamp}"` }, + body: JSON.stringify(PROFILE), + }); + }); + + await page.goto("/settings/profile"); + await expect(page.getByLabel("First name")).toHaveValue("Alice"); + + // Someone else writes the profile while the user is typing. + currentStamp = 2; + + await page.getByLabel("First name").fill("Alicia"); + await page.getByRole("button", { name: /save changes/i }).click(); + + await expect(page.getByText(/profile changed elsewhere/i)).toBeVisible(); + expect(sentIfMatch[0]).toBe('"stamp-1"'); + }); + + test("a 412 warns and keeps the edits instead of resending the stale body", async ({ page }) => { + // Retrying the same body against a fresh tag performs exactly the overwrite the 412 just + // prevented. The save stops, the user's typing stays on screen, and a deliberate second save + // goes out against the version they were just told about. + const sentIfMatch: string[] = []; + let currentStamp = 1; + + await page.route("**/api/v1/identity/profile", async (route) => { + const request = route.request(); + if (request.method() === "PUT") { + const ifMatch = request.headers()["if-match"] ?? ""; + sentIfMatch.push(ifMatch); + if (ifMatch !== `"stamp-${currentStamp}"`) { await route.fulfill({ status: 412, headers: { "Content-Type": "application/problem+json" }, @@ -188,11 +249,9 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { return; } - // Every read hands out a fresh token, so the retry provably carries a re-read one. - getCount += 1; await route.fulfill({ status: 200, - headers: { ...ETAG_CORS_HEADERS, ETag: `"stamp-${getCount}"` }, + headers: { ...ETAG_CORS_HEADERS, ETag: `"stamp-${currentStamp}"` }, body: JSON.stringify(PROFILE), }); }); @@ -200,14 +259,22 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { await page.goto("/settings/profile"); await expect(page.getByLabel("First name")).toHaveValue("Alice"); + currentStamp = 2; await page.getByLabel("First name").fill("Alicia"); await page.getByRole("button", { name: /save changes/i }).click(); + await expect(page.getByText(/profile changed elsewhere/i)).toBeVisible(); + await expect(page.getByText(/profile saved/i)).toBeHidden(); + // One PUT only: no silent retry behind the user's back. + expect(sentIfMatch).toHaveLength(1); + // The typing survived the rejection — nothing to retype. + await expect(page.getByLabel("First name")).toHaveValue("Alicia"); + + // Saving again now carries the version the warning told the user about. + await page.getByRole("button", { name: /save changes/i }).click(); await expect(page.getByText(/profile saved/i)).toBeVisible(); - await expect(page.getByText(/save failed/i)).toBeHidden(); expect(sentIfMatch).toHaveLength(2); - expect(sentIfMatch[0]).not.toBe(""); - expect(sentIfMatch[1]).not.toBe(sentIfMatch[0]); + expect(sentIfMatch[1]).toBe('"stamp-2"'); }); test("Reset button reverts edits to the original profile values", async ({ page }) => { From b1a77cd1ada6da93e171233ef56b564121e66ae5 Mon Sep 17 00:00:00 2001 From: "Marcelo M. M." <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:25:20 -0300 Subject: [PATCH 10/13] fix(identity): survive a weak ETag and a rotated stamp on the profile form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- clients/dashboard/src/api/identity.ts | 8 +- .../dashboard/src/pages/settings/profile.tsx | 8 +- .../dashboard/tests/settings/profile.spec.ts | 75 +++++++++++++++++++ .../Services/UserProfileService.cs | 14 +++- 4 files changed, 100 insertions(+), 5 deletions(-) diff --git a/clients/dashboard/src/api/identity.ts b/clients/dashboard/src/api/identity.ts index cbd4db8b05..baa860057f 100644 --- a/clients/dashboard/src/api/identity.ts +++ b/clients/dashboard/src/api/identity.ts @@ -412,7 +412,13 @@ export async function getMyProfileWithETag(): Promise<{ profile: UserDto; etag: let etag: string | null = null; const profile = await apiFetch("/api/v1/identity/profile", { onResponse: (response) => { - etag = response.headers.get("ETag"); + // Strip a `W/` prefix rather than pass it through. The endpoint only ever emits a strong + // validator, so a weak one is an artefact of the transport: a compressing edge (Cloudflare + // does this by default once it re-encodes a response) downgrades the tag it forwards. Sending + // it back as-is means the server drops it under the strong comparison `If-Match` mandates and + // answers 412 forever, which is a profile the user can never save. + const header = response.headers.get("ETag"); + etag = header ? header.replace(/^W\//, "") : null; }, }); return { profile, etag }; diff --git a/clients/dashboard/src/pages/settings/profile.tsx b/clients/dashboard/src/pages/settings/profile.tsx index bd8e8d5831..524340ab85 100644 --- a/clients/dashboard/src/pages/settings/profile.tsx +++ b/clients/dashboard/src/pages/settings/profile.tsx @@ -134,9 +134,13 @@ export function ProfileSettings() { const imageMutation = useMutation({ mutationFn: (url: string | null) => setProfileImage(url), - onSuccess: () => { + onSuccess: async () => { toast.success("Profile image updated"); - queryClient.invalidateQueries({ queryKey: PROFILE_KEY }); + // Setting the image is a second write to the same row, so ASP.NET Identity rotates the + // concurrency stamp and the tag this form is holding is spent. Adopting the new version (which + // also refreshes the cache, so the topbar avatar still updates) keeps the next save from + // answering 412 and telling the user someone else edited their profile. + await adoptCurrentVersion(); }, onError: (e: unknown) => { const message = diff --git a/clients/dashboard/tests/settings/profile.spec.ts b/clients/dashboard/tests/settings/profile.spec.ts index b190026ab6..decdd9fc8b 100644 --- a/clients/dashboard/tests/settings/profile.spec.ts +++ b/clients/dashboard/tests/settings/profile.spec.ts @@ -289,4 +289,79 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { await expect(page.getByLabel("First name")).toHaveValue("Alice"); await expect(page.getByLabel("Phone")).toHaveValue(""); }); + + // A compressing edge re-encodes the response and downgrades the validator it forwards: + // Cloudflare does exactly this by default once Brotli/gzip is on. The endpoint only ever emits a + // strong tag, so a weak one reaching the client is a transport artefact — and echoing it back + // unchanged means the server drops it under the strong comparison If-Match mandates and answers + // 412 to every save, forever, on a profile nobody else is touching. + test("sends a strong If-Match even when the edge downgraded the ETag to a weak one", async ({ + page, + }) => { + await page.route("**/api/v1/identity/profile", async (route) => { + if (route.request().method() === "PUT") { + await route.fulfill({ status: 200, headers: { "Content-Type": "application/json" }, body: '""' }); + return; + } + await route.fulfill({ + status: 200, + headers: { ...ETAG_CORS_HEADERS, ETag: 'W/"stamp-1"' }, + body: JSON.stringify(PROFILE), + }); + }); + + await page.goto("/settings/profile"); + await expect(page.getByLabel("First name")).toHaveValue("Alice"); + await page.getByLabel("First name").fill("Alicia"); + + const putReqPromise = page.waitForRequest( + (req) => + req.url().includes("/api/v1/identity/profile") && + req.method() === "PUT" && + !req.url().includes("/image"), + { timeout: 5_000 }, + ); + await page.getByRole("button", { name: /save changes/i }).click(); + const putReq = await putReqPromise; + + expect(putReq.headers()["if-match"]).toBe('"stamp-1"'); + }); + + // Setting the image is a second write to the same row, so Identity rotates the concurrency stamp. + // Without adopting the new version the next save carries the pre-image tag, gets a 412, and the + // user is told someone else edited their profile — on a profile only they touched. + test("a save after changing the avatar carries the tag the image write produced", async ({ page }) => { + let currentStamp = 1; + const sentIfMatch: string[] = []; + + await page.route("**/api/v1/identity/profile/image", async (route) => { + currentStamp += 1; + await route.fulfill({ status: 200, headers: { "Content-Type": "application/json" }, body: '""' }); + }); + + await page.route("**/api/v1/identity/profile", async (route) => { + const request = route.request(); + if (request.method() === "PUT") { + sentIfMatch.push(request.headers()["if-match"] ?? ""); + await route.fulfill({ status: 200, headers: { "Content-Type": "application/json" }, body: '""' }); + return; + } + await route.fulfill({ + status: 200, + headers: { ...ETAG_CORS_HEADERS, ETag: `"stamp-${currentStamp}"` }, + body: JSON.stringify({ ...PROFILE, imageUrl: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==" }), + }); + }); + + await page.goto("/settings/profile"); + await expect(page.getByLabel("First name")).toHaveValue("Alice"); + + await page.getByRole("button", { name: /^remove$/i }).click(); + await expect(page.getByText(/profile image updated/i)).toBeVisible(); + + await page.getByLabel("First name").fill("Alicia"); + await page.getByRole("button", { name: /save changes/i }).click(); + + await expect.poll(() => sentIfMatch).toEqual(['"stamp-2"']); + }); }); diff --git a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs index b984fbd571..7bd605fa90 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs @@ -90,6 +90,11 @@ public async Task UpdateAsync(string userId, string firstName, string lastName, // the deleteCurrentImage path it must not remove the avatar with no database change. EnsureConcurrencyStampMatches(user, expectedConcurrencyStamps); + // The old blob is only deleted once the database write has gone through. UpdateAsync can + // still lose a race here — the If-Match check above is not the last word, because another + // writer can land between it and the save — and deleting first would leave AspNetUsers + // pointing at a blob that no longer exists, which no retry can repair. + string? replacedBlob = null; Uri imageUri = user.ImageUrl ?? null!; // image is optional: text-only edits forward a null FileUploadRequest, so guard before // dereferencing Data or the common no-image update path NREs. @@ -99,12 +104,12 @@ public async Task UpdateAsync(string userId, string firstName, string lastName, user.ImageUrl = new Uri(imageString, UriKind.RelativeOrAbsolute); if (deleteCurrentImage && imageUri != null) { - await storageService.RemoveAsync(imageUri.ToString(), cancellationToken); + replacedBlob = imageUri.ToString(); } } else if (deleteCurrentImage && imageUri != null) { - await storageService.RemoveAsync(imageUri.ToString(), cancellationToken); + replacedBlob = imageUri.ToString(); user.ImageUrl = null; } @@ -132,6 +137,11 @@ public async Task UpdateAsync(string userId, string firstName, string lastName, throw new CustomException("Update profile failed"); } + if (replacedBlob is not null) + { + await storageService.RemoveAsync(replacedBlob, cancellationToken); + } + await signInManager.RefreshSignInAsync(user); } From 063aee0a811839ec16fd951d694296176292f0db Mon Sep 17 00:00:00 2001 From: "Marcelo M. M." <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:28:42 -0300 Subject: [PATCH 11/13] 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. --- deploy/docker/docker-compose.yml | 3 ++- src/Host/FSH.Starter.AppHost/AppHost.cs | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/deploy/docker/docker-compose.yml b/deploy/docker/docker-compose.yml index 9457a61232..dea9a817ff 100644 --- a/deploy/docker/docker-compose.yml +++ b/deploy/docker/docker-compose.yml @@ -80,7 +80,8 @@ services: # policy is set — objects are served via the API / presigned URLs, not a # public bucket. minio-init: - image: minio/mc:latest + # quay.io: minio/mc is gone from Docker Hub too. Tag pinned; quay stopped moving :latest. + image: quay.io/minio/mc:RELEASE.2025-08-13T08-35-41Z container_name: fsh-minio-init restart: "no" depends_on: diff --git a/src/Host/FSH.Starter.AppHost/AppHost.cs b/src/Host/FSH.Starter.AppHost/AppHost.cs index fb3506ab42..4fc689599d 100644 --- a/src/Host/FSH.Starter.AppHost/AppHost.cs +++ b/src/Host/FSH.Starter.AppHost/AppHost.cs @@ -76,6 +76,8 @@ """).ReplaceLineEndings("\n"); var minioInit = builder.AddContainer("minio-init", "minio/mc") + .WithImageRegistry("quay.io") + .WithImageTag("RELEASE.2025-08-13T08-35-41Z") .WithEntrypoint("/bin/sh") .WithArgs("-c", minioInitScript) .WithEnvironment("MC_USER", minioUser) From aa33639a8e169481eb63768e5dad9bacd7b2c665 Mon Sep 17 00:00:00 2001 From: "Marcelo M. M." <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:34:59 -0300 Subject: [PATCH 12/13] 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). --- src/Directory.Packages.props | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 89162470ad..854deb9530 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -145,12 +145,5 @@ AccessViolation). Transitive pinning is enabled, so this entry alone bumps it. Remove once the SignalR backplane package depends on a patched version itself. --> - - \ No newline at end of file From a5db0525294058688bcb84488fb7b5c0080f727d Mon Sep 17 00:00:00 2001 From: "Marcelo M. M." <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 18 Sep 2026 05:12:20 -0300 Subject: [PATCH 13/13] fix(identity): close three gaps review found around the precondition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `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. --- .../dashboard/src/pages/settings/profile.tsx | 10 ++-- .../dashboard/tests/settings/profile.spec.ts | 22 +++++++++ .../Web/CorsHeaderConfigurationTests.cs | 49 +++++++++++++++++++ 3 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 src/Tests/Framework.Tests/Web/CorsHeaderConfigurationTests.cs diff --git a/clients/dashboard/src/pages/settings/profile.tsx b/clients/dashboard/src/pages/settings/profile.tsx index 524340ab85..02776556d8 100644 --- a/clients/dashboard/src/pages/settings/profile.tsx +++ b/clients/dashboard/src/pages/settings/profile.tsx @@ -72,11 +72,15 @@ export function ProfileSettings() { const saveMutation = useMutation({ mutationFn: updateMyProfile, - onSuccess: () => { + onSuccess: async () => { toast.success("Profile saved"); // The save moved the profile on, so the tag the form holds is spent: adopt the new one or - // a second save in the same sitting would 412 against the user's own write. - void adoptCurrentVersion(); + // a second save in the same sitting would 412 against the user's own write. Awaited rather + // than fire-and-forget: isPending has to stay true until the new tag is in hand, or the + // button re-enables over a spent one and a quick second click 412s against the user's own + // save. It also keeps a failed refetch from becoming an unhandled rejection that would + // strand the form on a tag the server has already rejected. + await adoptCurrentVersion(); }, onError: async (err: unknown) => { // 412 means someone else wrote the profile after this form was seeded. Do NOT resend: the diff --git a/clients/dashboard/tests/settings/profile.spec.ts b/clients/dashboard/tests/settings/profile.spec.ts index decdd9fc8b..348de47f82 100644 --- a/clients/dashboard/tests/settings/profile.spec.ts +++ b/clients/dashboard/tests/settings/profile.spec.ts @@ -364,4 +364,26 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { await expect.poll(() => sentIfMatch).toEqual(['"stamp-2"']); }); + + // Without the profile read there is no ETag and no unedited-field values, so a save would either + // be a silent no-op or blank the fields it cannot see. The button is disabled and says why — + // which nothing exercised, so re-enabling it would not have failed anything. + test("saving is disabled while the profile read is failing", async ({ page }) => { + await page.route("**/api/v1/identity/profile", async (route) => { + if (route.request().method() === "GET") { + await route.fulfill({ + status: 500, + headers: { "Content-Type": "application/problem+json" }, + body: JSON.stringify({ status: 500, title: "Server Error" }), + }); + return; + } + throw new Error("no write may be attempted while the read is failing"); + }); + + await page.goto("/settings/profile"); + + await expect(page.getByText(/saving is disabled/i)).toBeVisible(); + await expect(page.getByRole("button", { name: /save changes/i })).toBeDisabled(); + }); }); diff --git a/src/Tests/Framework.Tests/Web/CorsHeaderConfigurationTests.cs b/src/Tests/Framework.Tests/Web/CorsHeaderConfigurationTests.cs new file mode 100644 index 0000000000..4293386d62 --- /dev/null +++ b/src/Tests/Framework.Tests/Web/CorsHeaderConfigurationTests.cs @@ -0,0 +1,49 @@ +using Microsoft.Extensions.Configuration; + +namespace Framework.Tests.Web; + +/// +/// The `If-Match` contract only reaches the endpoint if CORS lets the header through: it is not +/// safelisted, so with CorsOptions:AllowAll = false the browser's preflight decides whether +/// the precondition ever arrives. CorsPolicyTests builds its configuration in memory, so it +/// cannot notice the shipped files dropping the header — and dropping it degrades the feature back +/// to the lost update this PR exists to prevent, silently, with every test still green. +/// +public sealed class CorsHeaderConfigurationTests +{ + private static string HostDirectory() + { + var directory = new DirectoryInfo(AppContext.BaseDirectory); + while (directory is not null) + { + var candidate = Path.Combine(directory.FullName, "src", "Host", "FSH.Starter.Api"); + if (File.Exists(Path.Combine(candidate, "appsettings.json"))) + { + return candidate; + } + + directory = directory.Parent; + } + + throw new InvalidOperationException("Could not locate src/Host/FSH.Starter.Api from the test output directory."); + } + + [Theory] + [InlineData("Development")] + [InlineData("Production")] + public void AllowedHeaders_Should_CarryIfMatch_When_TheShippedFilesAreLoadedInOrder(string environment) + { + var host = HostDirectory(); + var configuration = new ConfigurationBuilder() + .SetBasePath(host) + .AddJsonFile("appsettings.json", optional: false) + .AddJsonFile($"appsettings.{environment}.json", optional: false) + .Build(); + + var headers = configuration.GetSection("CorsOptions:AllowedHeaders").Get() ?? []; + + headers.ShouldContain( + "if-match", + "the restricted CORS policy strips any header not on this list, so the PUT would never see the precondition"); + } +}