Repository navigation
Users: Validate avatar uploads against configured image file types - #23415
Conversation
|
Claude finished @iOvergaard's task in 6m 49s —— View job PR Review Complete
Review posted. Summary below. Verdict: Approved with Suggestions for improvement The PR correctly fixes three real bugs in the old validation code (substring match on comma-joined string, unchecked index-1 slice on extension-less filenames, hardcoded list ignoring configured Important finding: Accepting SVG for avatar uploads is a new user-writable XSS surface — SVG files served as Suggestions: Test method names don't follow the |
There was a problem hiding this comment.
Pull request overview
This PR aims to make avatar uploads consistent between the backoffice UI and the backend by using the configured image file types (plus an SVG carve-out) instead of a hardcoded allow-list.
Changes:
- Updated
UserService.SetAvatarAsyncto validate avatar extensions againstContentSettings.Imaging.ImageFileTypeswith a special-case forsvgand case-insensitive matching. - Updated backoffice avatar upload components to use
displayableImageFileTypes()when building the file inputacceptlist. - Added unit tests covering default/configured file types, disallowed extensions overriding, and case-insensitive matching.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Umbraco.Core/Services/UserService.cs |
Switches avatar validation to configured image types and adds SVG carve-out/disallowed override logic. |
src/Umbraco.Web.UI.Client/src/packages/user/user/workspace/user/components/user-workspace-avatar/user-workspace-avatar.element.ts |
Aligns avatar upload accept list with “displayable” image types from temporary file config. |
src/Umbraco.Web.UI.Client/src/packages/user/current-user/workspace/current-user-workspace-avatar.element.ts |
Same alignment for current-user avatar upload accept list. |
tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/UserServiceSetAvatarTests.cs |
Adds unit test coverage for the new backend avatar extension validation behavior. |
PR ReviewTarget: Fixes a genuine bug in avatar upload validation — the old code used substring matching on a comma-joined string (so
Important
Suggestions
Approved with Suggestions for improvementGood to go — the bug fix and test coverage are solid. Please carefully consider the SVG XSS note above (even if the decision is to accept the risk given the existing codebase precedent, a brief comment in the code documenting that conclusion would help future reviewers). |
d659e4e to
35efd51
Compare
SetAvatarAsync ignored the configured Umbraco:CMS:Content:Imaging:ImageFileTypes list and used a hardcoded set, and checked membership with a substring match on the comma-joined string, so extensions like "if" passed by matching inside "tiff". The extension-less case also threw from Path.GetExtension(...)[1..]. Validate against ContentSettings.Imaging.ImageFileTypes using the existing GetFileExtension helper and case-insensitive set membership, guard empty extensions, and apply the same invariant check to DisallowedUploadedFileExtensions. The backoffice avatar upload continues to build its accept list from the same configured imageFileTypes, keeping client and server in sync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019hpAn9TYikmsqKt3bFwtoo
35efd51 to
3ddbbf2
Compare
|
Is this intentionally targeted at |
|
@AndyButland I thought it might not be stable enough to be changing this behaviour outright on v17, but the rebase itself went smoothly. We can see how it pans out on v17. |
|
I had already added the 17.7.0 label some hours ago anyway (muscle memory, haha!). |
…racting the extension test logic to an extension method, coupled with a small number of integration tests to verify the behaviour end to end.
AndyButland
left a comment
There was a problem hiding this comment.
Looks good @iOvergaard.
I made a couple of changes relating to testing, as what you had originally, whilst covering well, introduced a unit test on UserService that needed a lot of mocking. I extracted the core logic of this PR into an extension method where it could be more easily fully unit tested, and extended the existing integration test to had a couple of higher level tests.
Manually verified via the backoffice UI to confirm that image settings are respected when selecting images for an avatar.
I did find an unexpected issue, in that configuring images seems to only be additive. E.g.:
"Imaging": {
"ImageFileTypes": [
"jpg",
"png",
"svg"
]
},This doesn't remove the default image types to leave only the three listed. So removal has to be done with code (e.g. in Program.cs):
builder.Services.PostConfigure<ContentSettings>(settings =>
{
settings.Imaging.ImageFileTypes.Remove("svg");
settings.Imaging.ImageFileTypes.Remove("webp");
});With this done I can no longer select .webp or .svg images for the avatar.
|
That doesn't sound right, @AndyButland. While that has probably very little to do with this pull request, I am surprised if that is how appsettings work. Are we sure about that? |



Prerequisites
Description
This PR fixes avatar upload validation in
UserService.SetAvatarAsyncso it validates the uploaded file extension against the configured image file types with correct set semantics.Bugs fixed
Umbraco:CMS:Content:Imaging:ImageFileTypes. It now validates againstContentSettings.Imaging.ImageFileTypes.ifmatched insidetiff). Replaced with proper case-insensitive set membership (InvariantContains).Path.GetExtension(...)[1..]threw when the file had no extension. Empty extensions are now guarded and rejected.DisallowedUploadedFileExtensionscheck is now case-insensitive.Extension extraction uses the existing
GetFileExtension()helper, and the validation is extracted into a smallIsAllowedAvatarFileExtensionhelper for readability.Tests
Added unit tests in
UserServiceSetAvatarTestscovering:if/tiffsubstring case and extension-less filenamesImageFileTypeslist (accepting configured types, rejecting others)Test Plan
Run the unit test suite:
All 12 test cases pass.
https://claude.ai/code/session_019hpAn9TYikmsqKt3bFwtoo