fix(moduleadmin): render safe-subset HTML in About changelog#119
Conversation
The 2.7.1 hardening pass wrapped every changelog line in htmlspecialchars, so the intentional <h5>/<strong>/<hr> markup shipped in module changelog.txt files rendered as literal source text on the module About page. Changelog files ship inside the module and are authored by the module developer, so full escaping broke legitimate formatting for no real gain. Render an allowlist of presentational tags instead, and strip all attributes from the surviving tags, so headings/emphasis/rules display again while a poisoned changelog still cannot inject <script>, event handlers, or href=javascript: vectors. Module metadata fields keep their existing htmlspecialchars escaping.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRender module changelog.txt content on the About page using an allowlisted subset of presentational HTML tags with all attributes stripped, instead of fully escaping lines, so trusted module formatting is preserved without enabling script or event-handler injection. Sequence diagram for renderAbout changelog sanitization and renderingsequenceDiagram
participant ModuleAdmin
participant Filesystem
participant PHP_file as file
participant PHP_array_map as array_map
participant PHP_changelogLine as changelogLine
ModuleAdmin->>Filesystem: is_file(changelog.txt)
alt module_language_file_exists
ModuleAdmin->>PHP_file: file(changelog_language_path)
else fallback_to_english_or_docs
ModuleAdmin->>Filesystem: is_file(english_or_docs_changelog.txt)
ModuleAdmin->>PHP_file: file(english_or_docs_changelog_path)
end
ModuleAdmin->>PHP_array_map: array_map(changelogLine, file_lines)
PHP_array_map->>PHP_changelogLine: changelogLine(line)
PHP_changelogLine->>PHP_changelogLine: strip_tags(line, allowedTags)
PHP_changelogLine->>PHP_changelogLine: preg_replace('<tag_with_attributes>', '<tag_without_attributes>')
PHP_array_map-->>ModuleAdmin: sanitized_lines
ModuleAdmin->>ModuleAdmin: implode('<br>', sanitized_lines)
ModuleAdmin-->>ModuleAdmin: append_to_ret
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider promoting the
$allowedTagslist and$changelogLinesanitizer closure into a shared helper or constant so the sanitization rules are reusable and centrally maintained across module HTML rendering. - The attribute-stripping
preg_replacecurrently rewrites all matching tags; consider tightening the regex to better preserve tag structure (e.g., self-closing or void tags) while still removing attributes for malformed or unusual HTML syntax.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider promoting the `$allowedTags` list and `$changelogLine` sanitizer closure into a shared helper or constant so the sanitization rules are reusable and centrally maintained across module HTML rendering.
- The attribute-stripping `preg_replace` currently rewrites all matching tags; consider tightening the regex to better preserve tag structure (e.g., self-closing or void tags) while still removing attributes for malformed or unusual HTML syntax.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #119 +/- ##
============================================
+ Coverage 18.13% 18.69% +0.55%
- Complexity 7854 8046 +192
============================================
Files 666 670 +4
Lines 43208 43794 +586
============================================
+ Hits 7837 8188 +351
- Misses 35371 35606 +235 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request updates the ModuleAdmin “About” page changelog rendering so module-provided changelog.txt content can display intended formatting again by allowing a restricted set of presentational HTML tags, while stripping attributes from surviving tags to reduce injection risk.
Changes:
- Introduce an allowlist-based changelog line sanitizer using
strip_tags()plus attribute stripping. - Switch changelog rendering (language and docs fallback) from full
htmlspecialcharsescaping to the new sanitizer.
| $changelogLine = static function ($line) use ($allowedTags) { | ||
| $line = strip_tags((string) $line, $allowedTags); | ||
| // Strip any attributes (onclick=, style=, href=, …) from the surviving tags. | ||
| return preg_replace('/<\s*(\/?)\s*([a-zA-Z][a-zA-Z0-9]*)\b[^>]*>/', '<$1$2>', $line); | ||
| }; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/Frameworks/moduleclasses/moduleadmin/moduleadmin.php`:
- Around line 611-619: Normalize the new changelog lookup block in
moduleadmin.php to PSR-12 spacing by removing unnecessary spaces inside control
structures and function calls. Update the nested if statements and the is_file,
is_readable, implode, array_map, and file calls in this block to match the
project’s standard style, using the surrounding moduleadmin::changelog-related
logic as the reference point.
- Around line 604-609: Add regression tests for the changelog sanitization in
renderAbout() by covering the $changelogLine closure and its handling of allowed
tags, attribute stripping, and disallowed tag removal. Update ModuleAdminTest so
it asserts that safe tags like those in the allowedTags list survive, attributes
such as onclick/style/href are removed, and unsafe tags are stripped, ensuring
the sanitizer behavior in moduleadmin.php does not regress.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a29003d7-76d1-428d-b17f-51315bb5159a
📒 Files selected for processing (1)
htdocs/Frameworks/moduleclasses/moduleadmin/moduleadmin.php
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Address PR review feedback on the About-page changelog rendering. Replace the strip_tags()-then-attribute-strip closure with an escape-first approach: HTML-escape the whole line, then re-enable only an explicit allowlist of attribute-free presentational tags. This preserves literal angle-bracket text (e.g. "<table>", "requires PHP <= 8.0") and "&" as visible text instead of dropping it, and enforces the tag allowlist in the pattern itself rather than relying solely on strip_tags(). Extract the logic into ModuleAdmin::sanitizeChangelogLine() and add regression tests covering allowed tags, attribute stripping, disallowed tag handling, and literal-text preservation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/Frameworks/moduleclasses/moduleadmin/moduleadmin.php`:
- Around line 526-537: The sanitization logic in sanitizeChangelogLine is fine,
but this new public method should be typed for safety. Add an explicit parameter
type for the $line argument in sanitizeChangelogLine, and handle the fact that
preg_replace_callback() can return null by normalizing the return value before
exposing it. Keep the allowlist regex and htmlspecialchars flow unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68befc11-0521-4627-a5a2-975fb6209d1b
📒 Files selected for processing (2)
htdocs/Frameworks/moduleclasses/moduleadmin/moduleadmin.phptests/unit/htdocs/Frameworks/moduleclasses/ModuleAdminTest.php
| public static function sanitizeChangelogLine($line) | ||
| { | ||
| static $allowed = 'h1|h2|h3|h4|h5|h6|p|br|hr|ul|ol|li|strong|b|em|i|u|code|pre|blockquote|span'; | ||
|
|
||
| $escaped = htmlspecialchars((string) $line, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); | ||
|
|
||
| return preg_replace_callback( | ||
| '#<(/?)\s*(' . $allowed . ')\s*(/?)>#i', | ||
| static fn($m) => '<' . $m[1] . strtolower($m[2]) . $m[3] . '>', | ||
| $escaped | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Sanitizer logic is sound; consider typing this new public method.
The escape-then-re-enable-allowlist approach is correct and safe: attribute-bearing and non-allowlisted tags stay inert escaped text on PHP 8.2–8.5, and the allowlist is enforced by the pattern itself rather than delegated to strip_tags().
As a small type-safety hardening on this new public security-relevant surface, consider declaring the parameter type. Note preg_replace_callback() returns ?string, so guard the return rather than declaring a bare : string.
♻️ Typed signature with null-safe return
- public static function sanitizeChangelogLine($line)
+ public static function sanitizeChangelogLine(string $line): string
{
static $allowed = 'h1|h2|h3|h4|h5|h6|p|br|hr|ul|ol|li|strong|b|em|i|u|code|pre|blockquote|span';
- $escaped = htmlspecialchars((string) $line, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
+ $escaped = htmlspecialchars($line, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
- return preg_replace_callback(
+ return preg_replace_callback(
'#<(/?)\s*(' . $allowed . ')\s*(/?)>`#i`',
static fn($m) => '<' . $m[1] . strtolower($m[2]) . $m[3] . '>',
$escaped
- );
+ ) ?? $escaped;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static function sanitizeChangelogLine($line) | |
| { | |
| static $allowed = 'h1|h2|h3|h4|h5|h6|p|br|hr|ul|ol|li|strong|b|em|i|u|code|pre|blockquote|span'; | |
| $escaped = htmlspecialchars((string) $line, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); | |
| return preg_replace_callback( | |
| '#<(/?)\s*(' . $allowed . ')\s*(/?)>#i', | |
| static fn($m) => '<' . $m[1] . strtolower($m[2]) . $m[3] . '>', | |
| $escaped | |
| ); | |
| } | |
| public static function sanitizeChangelogLine(string $line): string | |
| { | |
| static $allowed = 'h1|h2|h3|h4|h5|h6|p|br|hr|ul|ol|li|strong|b|em|i|u|code|pre|blockquote|span'; | |
| $escaped = htmlspecialchars($line, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); | |
| return preg_replace_callback( | |
| '#<(/?)\s*(' . $allowed . ')\s*(/?)>`#i`', | |
| static fn($m) => '<' . $m[1] . strtolower($m[2]) . $m[3] . '>', | |
| $escaped | |
| ) ?? $escaped; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 534-534: Avoid variables with short names like $m. Configured minimum length is 3. (undefined)
(ShortVariable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/Frameworks/moduleclasses/moduleadmin/moduleadmin.php` around lines 526
- 537, The sanitization logic in sanitizeChangelogLine is fine, but this new
public method should be typed for safety. Add an explicit parameter type for the
$line argument in sanitizeChangelogLine, and handle the fact that
preg_replace_callback() can return null by normalizing the return value before
exposing it. Keep the allowlist regex and htmlspecialchars flow unchanged.
Code Review ✅ Approved 2 resolved / 2 findingsRestores changelog formatting by implementing an HTML allowlist and attribute stripping, effectively replacing overly broad escaping. This addresses previous issues with literal angle-bracket rendering and provides a more robust sanitization approach. ✅ 2 resolved✅ Edge Case: Literal angle-bracket text in changelog is silently dropped
✅ Security: Sanitizer relies solely on strip_tags for the tag allowlist
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
The 2.7.1 hardening pass wrapped every changelog line in htmlspecialchars, so the intentional
//- Introduce a sanitization helper for changelog lines that strips all attributes while allowing a whitelist of presentational HTML tags for formatting.
- Update changelog rendering to use the new sanitization helper for both language-specific and docs-based changelog files.
-
- Changelog entries are now displayed more safely while preserving a small set of basic formatting like bold or italics.
- Unsupported or potentially harmful HTML is shown as plain text instead of being rendered.
-
- Added coverage for changelog sanitization to verify safe output across multiple formatting cases.
markup shipped in module changelog.txt files rendered as literal source text on the module About page.
Changelog files ship inside the module and are authored by the module developer, so full escaping broke legitimate formatting for no real gain. Render an allowlist of presentational tags instead, and strip all attributes from the surviving tags, so headings/emphasis/rules display again while a poisoned changelog still cannot inject <script>, event handlers, or href=javascript: vectors.
Module metadata fields keep their existing htmlspecialchars escaping.
Summary by Sourcery
Render module changelog content on the About page using a restricted set of safe HTML tags instead of fully escaping all markup.
Enhancements:
Summary by CodeRabbit
Bug Fixes
Tests