Skip to content

fix(settings): escape group names in the users admin group list - #41758

Merged
DeepDiver1975 merged 1 commit into
masterfrom
fix/escape-group-names-in-group-list
Jul 29, 2026
Merged

fix(settings): escape group names in the users admin group list#41758
DeepDiver1975 merged 1 commit into
masterfrom
fix/escape-group-names-in-group-list

Conversation

@DeepDiver1975

Copy link
Copy Markdown
Member

Description

GroupList.addGroup() in settings/js/users/groups.js built each list item of the
users administration group list by concatenating the group id and the group name
into an HTML string, which jQuery then parses as markup. Both interpolated values
are now passed through the existing escapeHTML() helper (core/js/js.js), so the
values are rendered verbatim.

escapeHTML() covers both contexts the values appear in - the data-gid attribute
and the .groupname text node - and data-gid still round-trips as the literal
string, so getGroupLI() lookups keep matching.

Adds three regression tests to settings/tests/js/users/groupsSpec.js, which is
already wired into the settings karma suite.

Related Issue

n/a - no tracking issue.

Motivation and Context

Unlike usernames, group names are not restricted to an allow-listed character set:
Group\Manager::createGroup() only rejects empty and untrimmed names, and names
can also originate from an external backend such as LDAP. Group and quota values
in the user rows of the same panel (settings/js/users.js) already go through
escapeHTML(), so this makes the output encoding consistent across the users
administration panel.

The PHP template settings/templates/users/part.grouplist.php renders group names
with p() and was already correct - only the JS render path was affected.

How Has This Been Tested?

  • test environment: local checkout, Node 22 + jsdom harness driving the real
    settings/js/users/groups.js with the repo's vendored jQuery and the real
    escapeHTML() extracted from core/js/js.js
  • test case 1: group name containing <script> markup - renders as text, no
    <script> element in the row
  • test case 2: group id containing an attribute breakout - no stray attribute is
    created and data-gid still reads back as the literal string
  • test case 3: group name <img src=x onerror="..."> - no img element inside
    .groupname, name renders as text
  • all three fail on the unpatched code and pass with the change; the pre-existing
    adds new user group spec still passes (4/4)
  • note: karma is not installed in this environment, so the official
    make test-js runner was not executed locally - CI (js-unit.yml) covers it
  • manual check: create a group named <img src=x onerror=alert(1)> in
    Settings -> Users; the name renders as literal text, both on creation and after
    a group-membership toggle (which re-renders via GroupList.update())

Screenshots (if appropriate):

n/a

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised: n/a
  • Changelog item, see TEMPLATE

@DeepDiver1975
DeepDiver1975 requested a review from a team as a code owner July 29, 2026 07:48
GroupList.addGroup() built each list item by concatenating the group id and
group name into an HTML string, which jQuery then parses as markup. Unlike
usernames, group names are not restricted to an allow-listed character set -
Group\Manager::createGroup() only rejects empty and untrimmed names - so those
values were not rendered verbatim.

Both interpolated values now go through escapeHTML(), the same helper already
used for the group and quota values in the user rows, so the encoding is
consistent across the users administration panel. escapeHTML() covers the
attribute and the text context, and data-gid still round-trips as the literal
string, so getGroupLI() lookups keep matching.

Adds regression tests for the escaping to the settings karma suite.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
@DeepDiver1975
DeepDiver1975 force-pushed the fix/escape-group-names-in-group-list branch from 49ab896 to aea1c7d Compare July 29, 2026 07:48
@DeepDiver1975 DeepDiver1975 self-assigned this Jul 29, 2026
@DeepDiver1975 DeepDiver1975 added this to the development milestone Jul 29, 2026
@DeepDiver1975
DeepDiver1975 merged commit e1af7c5 into master Jul 29, 2026
29 checks passed
@DeepDiver1975
DeepDiver1975 deleted the fix/escape-group-names-in-group-list branch July 29, 2026 08:30
@phil-davis

Copy link
Copy Markdown
Contributor

Manually checked this, and it is fixed.
Previously, I could add a group named <b>Bold</b> and the group name was rendered as just the word "Bold" in bold type.
Now the group name renders exactly as the literal characters <b>Bold</b>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants