Fix stored XSS in Group name (CVE-2024-25891-98)#7675
Conversation
Add input sanitization and output escaping for group names: Input sanitization: - Add strip_tags() when creating/updating groups in people-groups.php API Output escaping with htmlspecialchars(): - GroupView.php: page title, card header, delete confirmation dialog - SundaySchoolReports.php: group select dropdown - CartToGroup.php: group select dropdown Fixes #6848
DawoudIO
requested review from
DAcodedBEAT,
MrClever,
bigtigerku,
grayeul and
respencer
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR addresses CVE-2024-25891-98, a stored cross-site scripting (XSS) vulnerability in group names. The fix implements a defense-in-depth approach with both input sanitization and output escaping.
Key changes:
- Input sanitization: Strips HTML tags from group names when creating/updating groups via the API
- Output escaping: Applies
htmlspecialchars()with ENT_QUOTES and UTF-8 encoding when displaying group names in HTML contexts - Coverage: Fixes XSS vulnerabilities in group selection dropdowns, page titles, card headers, and confirmation dialogs
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/api/routes/people/people-groups.php | Adds strip_tags() sanitization to group name input in POST endpoints for creating and updating groups |
| src/GroupView.php | Escapes group name output in page title, card header, and delete confirmation dialog |
| src/sundayschool/SundaySchoolReports.php | Escapes group name in group selection dropdown options |
| src/CartToGroup.php | Escapes group name in group selection dropdown options |
| } | ||
| $group->setName($groupSettings['groupName']); | ||
| $group->setName(strip_tags($groupSettings['groupName'])); | ||
| $group->setDescription($groupSettings['description'] ?? ''); |
There was a problem hiding this comment.
The description field should also be sanitized with strip_tags() for consistency with the group name field and to prevent stored XSS vulnerabilities.
Apply the same sanitization:
$group->setDescription(strip_tags($groupSettings['description'] ?? ''));| $group->setName($input['groupName']); | ||
| $group->setName(strip_tags($input['groupName'])); | ||
| $group->setType($input['groupType']); | ||
| $group->setDescription($input['description'] ?? ''); |
There was a problem hiding this comment.
The description field should also be sanitized with strip_tags() for consistency with the group name field and to prevent stored XSS vulnerabilities.
Apply the same sanitization:
$group->setDescription(strip_tags($input['description'] ?? ''));There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Add strip_tags() sanitization when creating new calendars to prevent XSS payloads from being stored in the database. This is part of CVE-2023-24690 which covers multiple XSS vulnerabilities: - Calendar Name XSS (fixed here) - Group Name XSS (fixed in PR #7675) - Group Description XSS (fixed in PR #7675) Fixes #6444
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>
Co-authored-by: DawoudIO <554959+DawoudIO@users.noreply.github.com>
What Changed
Add input sanitization and output escaping for group names:
Input sanitization:
Output escaping with htmlspecialchars():
Fixes #6848
Type
Testing
Screenshots
Security Check
Code Quality
Pre-Merge