test(user): cover HTML metacharacters in username validation - #41738
Merged
Conversation
The username allow-list in OC\User\Manager::createUser() already rejects any character outside "a-z", "A-Z", "0-9" and "+_.@-'", so HTML and script metacharacters can never reach a stored username. That guard was only covered by three generic invalid-character cases, none resembling an injection payload. Extend the invalid-character data provider with quote, angle-bracket and full script-tag payloads so a future relaxation of the allow-list fails the suite rather than passing unnoticed. Test-only change; no production behaviour is affected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
The entry used "Tests:" as its type, which calens rejects - it validates against a closed set of Bugfix, Change, Enhancement and Security, so the changelog lint failed. Use "Change:", matching how existing test and tooling entries are typed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
phil-davis
approved these changes
Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds regression coverage for HTML/script metacharacters in the username
validation allow-list. Test-only — no production behaviour changes.
OC\User\Manager::createUser()already rejects every character outsidea-z,A-Z,0-9and+_.@-', so markup can never reach a storedusername. That guard was only exercised by three generic invalid-character
cases (
John#Smith,John^Smith,JohnSmith(CEO)), none of which lookedlike an injection payload. This extends the data provider so a future
relaxation of the allow-list fails the suite instead of passing unnoticed.
Added cases:
"><script>alert(document.cookie)</script><img src=x onerror=alert(1)>John<b>SmithJohn"SmithBackground
This came out of triaging an external report claiming stored XSS via the
username field in
/settings/users. The report did not reproduce: theallow-list rejects the payload server-side at
lib/private/User/Manager.php, and the render sink insettings/js/users/users.jsuses jQuery.text()(textContent), so nomarkup is interpretable. Both admin-panel and provisioning-API creation
paths funnel through the same
createUser()method, so there is nobypassing path.
No fix was required. These tests exist so that conclusion stays true.
Testing
Both new payloads confirmed rejected by the existing regex; full
ManagerTestgreen with no regressions.🤖 Generated with Claude Code