refactor(modulesadmin): replace $myts->htmlSpecialChars() with native…#1689
Conversation
… htmlspecialchars() Replace 7 occurrences of $myts->htmlSpecialChars() with native htmlspecialchars($x, ENT_QUOTES | ENT_HTML5, 'UTF-8') in the module admin pages where the input is structurally constrained to identifier- like values (directory names, module display names, author strings) that cannot contain pre-existing HTML entities. The MyTextSanitizer wrapper has a post-processing step that un-encodes & back to & — designed for free-form text that may already contain HTML entities from the database. These 7 call sites handle module dirnames and metadata where that & pass-through is unnecessary and the standard PHP function provides correct, simpler escaping. Also removes the now-unused $myts = \MyTextSanitizer::getInstance() initialization from both files, and updates the source-asserting test to match the new pattern. Closes XOOPS#869 (partial — only the identifier-constrained call sites; the remaining ~78 occurrences depend on the & pass-through contract and cannot be safely replaced without a broader data-model migration). Files changed: - htdocs/modules/system/admin/modulesadmin/main.php (5 replacements) - htdocs/modules/system/admin/modulesadmin/modulesadmin.php (2 replacements) - tests/unit/.../MainControllerTest.php (test updated) Full suite: 6040 tests, 10527 assertions, 0 failures.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplaced MyTextSanitizer htmlSpecialChars usage with explicit htmlspecialchars(..., ENT_QUOTES | ENT_HTML5, 'UTF-8') for display output, stopped pre-escaping module dirnames before DB/filesystem lookups, updated unit tests to assert these behaviors, and added a PHPStan constants stub with config change. Changes
Sequence Diagram(s)(Skipped — changes are localized sanitization and test/config updates without a multi-component sequential flow needing visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.46)Invalid entry in excludePaths: If the excluded path can sometimes exist, append (?) parameters: 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1689 +/- ##
=============================================
+ Coverage 0 19.22% +19.22%
- Complexity 0 7584 +7584
=============================================
Files 0 621 +621
Lines 0 40085 +40085
=============================================
+ Hits 0 7707 +7707
- Misses 0 32378 +32378 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors parts of the System “modulesadmin” admin flow to replace MyTextSanitizer::htmlSpecialChars() with native htmlspecialchars(..., ENT_QUOTES | ENT_HTML5, 'UTF-8'), and updates a unit test to reflect the new escaping approach.
Changes:
- Replace several
$myts->htmlSpecialChars()calls withhtmlspecialchars(..., ENT_QUOTES | ENT_HTML5, 'UTF-8')in modules admin pages. - Remove now-unused
MyTextSanitizer::getInstance()initialization. - Update
MainControllerTestto assert the new escaping approach is used.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
htdocs/modules/system/admin/modulesadmin/main.php |
Swaps sanitizer calls to htmlspecialchars() for module names/dirnames used during confirm/install/uninstall/update flows. |
htdocs/modules/system/admin/modulesadmin/modulesadmin.php |
Replaces escaping for $dirname and module author output with htmlspecialchars(), and removes sanitizer initialization. |
tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php |
Renames/adjusts a test to look for htmlspecialchars() usage instead of MyTextSanitizer usage. |
| $myts = \MyTextSanitizer::getInstance(); | ||
|
|
||
| $dirname = $myts->htmlSpecialChars(trim($dirname)); | ||
| $dirname = htmlspecialchars(trim($dirname), ENT_QUOTES | ENT_HTML5, 'UTF-8'); |
There was a problem hiding this comment.
$dirname is a module identifier used for DB lookup, cache clearing, filesystem paths, and include_once later in this function. HTML-escaping it here can change the identifier (notably converting & to &) and break module updates/paths. Keep $dirname as a validated/raw dirname for internal operations (e.g., basename() + a whitelist regex), and only apply htmlspecialchars() at the point of HTML output.
| $dirname = htmlspecialchars(trim($dirname), ENT_QUOTES | ENT_HTML5, 'UTF-8'); | |
| $dirname = basename($dirname); | |
| if ('' === $dirname || !preg_match('/\A[a-zA-Z0-9_\-]+\z/', $dirname)) { | |
| trigger_error('Invalid module dirname: ' . basename($dirname), E_USER_WARNING); | |
| return ''; | |
| } |
| case 'install': | ||
| $module = Request::getString('module', ''); | ||
| $module = $myts->htmlSpecialChars($module); | ||
| $module = htmlspecialchars($module, ENT_QUOTES | ENT_HTML5, 'UTF-8'); | ||
| // Get module handler | ||
| /** @var XoopsModuleHandler $module_handler */ |
There was a problem hiding this comment.
$module is used as a module dirname for loadInfoAsVar() / getByDirname() and is also passed to xoops_confirm() (which already escapes hidden values). HTML-escaping it here can change the identifier (e.g., & -> &) and can also lead to double-escaped hidden fields. Prefer validating/normalizing the dirname (e.g., basename() + allowed-char regex or membership in XoopsLists::getModulesList() like the *_ok branches) and only escape when interpolating into HTML output.
| $this->assertStringContainsString( | ||
| '$myts = \MyTextSanitizer::getInstance();', | ||
| 'htmlspecialchars(', | ||
| $this->sourceCode, | ||
| 'Should initialize MyTextSanitizer' | ||
| ); | ||
|
|
||
| // Should use htmlSpecialChars for output | ||
| $this->assertStringContainsString( | ||
| '$myts->htmlSpecialChars', | ||
| $this->sourceCode, | ||
| 'Should use htmlSpecialChars for output sanitization' | ||
| 'Should use htmlspecialchars for output sanitization' | ||
| ); |
There was a problem hiding this comment.
This assertion is very broad ('htmlspecialchars(' anywhere in the source) and can pass even if the specific output-escaping behavior regresses or the required flags/encoding change. Consider asserting a more specific pattern (e.g., presence of ENT_QUOTES | ENT_HTML5 and 'UTF-8' at the relevant call sites) or checking that the old $myts->htmlSpecialChars usage is absent, to keep the test meaningful.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
htdocs/modules/system/admin/modulesadmin/main.php (3)
376-383:⚠️ Potential issue | 🟠 MajorSame issue — HTML-escaping before database lookup.
Line 378 escapes
$modulebefore it's passed togetByDirname()on line 383. Per the relevant code snippet fromhtdocs/kernel/module.php:815-834,getByDirname()performs a SQL lookup using the dirname value. An HTML-escaped dirname likefoo&barwon't match the raw valuefoo&barstored in the database.🐛 Proposed fix
case 'uninstall': $module = Request::getString('module', ''); - $module = htmlspecialchars($module, ENT_QUOTES | ENT_HTML5, 'UTF-8'); $msgs = ''; // Get module handler /** `@var` XoopsModuleHandler $module_handler */ $module_handler = xoops_getHandler('module'); $mod = $module_handler->getByDirname($module);Escape only when outputting to HTML context (in
$msgsconstruction andxoops_confirm()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 376 - 383, The code HTML-escapes $module before calling $module_handler->getByDirname($module), which breaks the DB lookup; revert so getByDirname() receives the raw dirname from Request::getString('module', '') (remove or move the htmlspecialchars call), then apply htmlspecialchars only when building output ($msgs and in xoops_confirm() calls) so user-facing HTML is escaped but database lookups use the original $module value.
439-445:⚠️ Potential issue | 🟠 MajorSame issue — update case also escapes before
getByDirname().Consistent with the install and uninstall cases, the dirname is HTML-escaped before the database lookup. This will fail for any module dirname containing special characters.
🐛 Proposed fix
case 'update': $module = Request::getString('module', ''); - $module = htmlspecialchars($module, ENT_QUOTES | ENT_HTML5, 'UTF-8'); // Get module handler /** `@var` XoopsModuleHandler $module_handler */ $module_handler = xoops_getHandler('module'); $mod = $module_handler->getByDirname($module);Apply escaping only at HTML output points.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 439 - 445, The update case is escaping the module dirname before the DB lookup (variable $module passed through htmlspecialchars) which will break getByDirname(); revert to using the raw dirname for the lookup (use $module as received from Request::getString) when calling $module_handler->getByDirname($module) and only apply htmlspecialchars at the HTML output point (not before getByDirname), mirroring the install/uninstall fixes; update references around the case 'update' block (variables $module, htmlspecialchars, getByDirname, $mod) accordingly.
314-321:⚠️ Potential issue | 🟠 MajorRemove htmlspecialchars() before filesystem operations — escaping breaks module path lookup.
The dirname is HTML-escaped on line 316 before
loadInfoAsVar($module)on line 321. Sincebasename()does not decode HTML entities, an escaped dirname likefoo&barwill be used to construct the filesystem pathmodules/foo&bar/language/...instead of matching the actual directoryfoo&bar. This violates the principle of context-appropriate escaping: escape only for output rendering, not for data operations.Remove the htmlspecialchars() call before the filesystem operation and apply escaping only when rendering the module name in HTML output:
♻️ Proposed fix
case 'install': $module = Request::getString('module', ''); - $module = htmlspecialchars($module, ENT_QUOTES | ENT_HTML5, 'UTF-8'); // Get module handler /** `@var` XoopsModuleHandler $module_handler */ $module_handler = xoops_getHandler('module'); $mod = $module_handler->create(); $mod->loadInfoAsVar($module);Then escape
$moduleonly when passing it toxoops_confirm()for HTML rendering, or escape at the point of final output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 314 - 321, The code currently HTML-escapes $module before performing filesystem operations which breaks module path lookup; remove the htmlspecialchars() call so that the raw $module value is passed into $module_handler->create() and $mod->loadInfoAsVar($module) (i.e., stop replacing $module with an escaped string before loadInfoAsVar), and instead perform escaping only when rendering to HTML (for example, escape $module when passing it to xoops_confirm() or any template/output functions). Ensure all references to $module used for filesystem or handler operations use the unescaped value and only apply htmlspecialchars() at the output layer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php`:
- Around line 835-838: The code currently HTML-escapes $dirname with
htmlspecialchars before using it for module lookup, which breaks module
resolution; revert the change so $module_handler->getByDirname() receives the
raw, unescaped $dirname, and only use a new $dirname_escaped (or similar) for
HTML output (logs, image URLs, templates). Locate the htmlspecialchars call and
the call to $module_handler->getByDirname($dirname) and ensure the escape
applies only where rendering occurs, leaving $dirname intact for internal
operations like getByDirname() and subsequent module logic.
In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php`:
- Around line 218-230: The test testUsesHtmlspecialcharsForOutput currently just
checks for the presence of "htmlspecialchars(" but the implementation escapes
dirnames too early (before internal calls like loadInfoAsVar() and
getByDirname()), so fix the implementation to perform htmlspecialchars(...) only
at output boundaries (e.g., where values are assigned into templates or
concatenated into HTML) and not before calling internal handlers; then tighten
the test to assert that htmlspecialchars(...) appears in the template-assignment
or HTML-construction code paths (and does NOT appear immediately surrounding
calls to loadInfoAsVar() or getByDirname()) so the assertion verifies escaping
happens at output, not prior to internal logic.
---
Outside diff comments:
In `@htdocs/modules/system/admin/modulesadmin/main.php`:
- Around line 376-383: The code HTML-escapes $module before calling
$module_handler->getByDirname($module), which breaks the DB lookup; revert so
getByDirname() receives the raw dirname from Request::getString('module', '')
(remove or move the htmlspecialchars call), then apply htmlspecialchars only
when building output ($msgs and in xoops_confirm() calls) so user-facing HTML is
escaped but database lookups use the original $module value.
- Around line 439-445: The update case is escaping the module dirname before the
DB lookup (variable $module passed through htmlspecialchars) which will break
getByDirname(); revert to using the raw dirname for the lookup (use $module as
received from Request::getString) when calling
$module_handler->getByDirname($module) and only apply htmlspecialchars at the
HTML output point (not before getByDirname), mirroring the install/uninstall
fixes; update references around the case 'update' block (variables $module,
htmlspecialchars, getByDirname, $mod) accordingly.
- Around line 314-321: The code currently HTML-escapes $module before performing
filesystem operations which breaks module path lookup; remove the
htmlspecialchars() call so that the raw $module value is passed into
$module_handler->create() and $mod->loadInfoAsVar($module) (i.e., stop replacing
$module with an escaped string before loadInfoAsVar), and instead perform
escaping only when rendering to HTML (for example, escape $module when passing
it to xoops_confirm() or any template/output functions). Ensure all references
to $module used for filesystem or handler operations use the unescaped value and
only apply htmlspecialchars() at the output layer.
🪄 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: 26ec9a6f-e9d4-4967-91d2-de43655a61ba
📒 Files selected for processing (3)
htdocs/modules/system/admin/modulesadmin/main.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phptests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
Outdated
Show resolved
Hide resolved
…onfig for CodeRabbit Address CodeRabbit and Copilot review findings on PR XOOPS#1689, plus fix the PHPStan configuration that was blocking CodeRabbit's static analysis. Escaping fix: The original commit replaced $myts->htmlSpecialChars() with native htmlspecialchars() on module dirnames. Both Copilot and CodeRabbit flagged that the escaping was applied BEFORE the dirname was used for database lookups (getByDirname), filesystem operations (loadInfoAsVar), and cache clearing — which would break module resolution if the dirname ever contained escapable characters. - modulesadmin.php: remove the early htmlspecialchars() on $dirname (line 835). The dirname is used raw for DB lookup, cache clear, and filesystem include. The only HTML output point ($module->getInfo('author') at line 877) was already correctly escaped at the output site. - main.php: remove the early htmlspecialchars() on $module in the install, uninstall, and update cases (lines 316, 379, 442). These values feed directly into loadInfoAsVar() and getByDirname(). The xoops_confirm() call handles its own escaping. - main.php: keep htmlspecialchars() on $oldname/$newname in the list case (lines 268-269) — these are display values going to Smarty templates and are correctly escaped at the output point. PHPStan configuration: - Replace bootstrapFiles (which CodeRabbit disallows for security — arbitrary PHP execution) with stubFiles pointing to a new phpstan-constants.stub.php that declares XOOPS runtime constants as type stubs. The original bootstrapFiles entry referenced a non-existent constants.php at the repo root. Test update: - Strengthen the test to verify both that htmlspecialchars() is present (for display escaping) AND that MyTextSanitizer is absent (confirming the migration). Previous test was too broad.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
htdocs/modules/system/admin/modulesadmin/main.php (2)
217-217: 🧹 Nitpick | 🔵 TrivialSame pre-existing inconsistency at line 217.
This
htmlspecialchars()call also lacks the explicit'UTF-8'charset. The changed lines in this PR (268-269) set a good example—consider aligning these in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/main.php` at line 217, The htmlspecialchars call setting $toinstall_mods[$i]['name'] uses ENT_QUOTES|ENT_HTML5 but omits the explicit charset; update the call that wraps $module->getInfo('name') to include 'UTF-8' as the fourth parameter (matching the changes at the other lines), i.e. locate the assignment to $toinstall_mods[$i]['name'] and modify the htmlspecialchars invocation to pass ENT_QUOTES | ENT_HTML5 and 'UTF-8' so encoding is consistent.
138-138: 🧹 Nitpick | 🔵 TrivialPre-existing inconsistency: missing charset parameter.
While outside this PR's direct changes, line 138 omits
'UTF-8'whereas the new code at lines 268-269 correctly includes it. Consider a follow-up to normalize these for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/main.php` at line 138, The call that assigns $listed_mods[$i]['name'] uses htmlspecialchars((string) $module->getVar('name'), ENT_QUOTES | ENT_HTML5) without the charset argument; update this to include 'UTF-8' to match other usages (e.g., the code at lines setting module names later) so replace the htmlspecialchars invocation used in the $listed_mods assignment to include 'UTF-8' as the third parameter.htdocs/modules/system/admin/modulesadmin/modulesadmin.php (3)
86-86: 🧹 Nitpick | 🔵 TrivialMissing
'UTF-8'charset parameter for consistency.Line 86 omits the charset parameter while line 875 (the changed line) includes
'UTF-8'. For defensive encoding and consistency with the coding guidelines, include the charset explicitly.- $msgs[] = '<strong>' . _AUTHOR . ':</strong> ' . htmlspecialchars(trim($module->getInfo('author')), ENT_QUOTES | ENT_HTML5); + $msgs[] = '<strong>' . _AUTHOR . ':</strong> ' . htmlspecialchars(trim($module->getInfo('author')), ENT_QUOTES | ENT_HTML5, 'UTF-8');As per coding guidelines: "Escape all output with htmlspecialchars($value, ENT_QUOTES, 'UTF-8')".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php` at line 86, Update the htmlspecialchars call that builds $msgs[] for the module author to include the explicit charset parameter; specifically change the call wrapping trim($module->getInfo('author')) in the $msgs[] line so it passes 'UTF-8' as the third argument (preserving the existing flags ENT_QUOTES | ENT_HTML5) to comply with the coding guideline.
1500-1500: 🧹 Nitpick | 🔵 TrivialMissing
'UTF-8'charset parameter inxoops_module_log_header().- $msgs[] = '<strong>' . _AUTHOR . ':</strong> ' . htmlspecialchars(trim((string) $module->getInfo('author')), ENT_QUOTES | ENT_HTML5); + $msgs[] = '<strong>' . _AUTHOR . ':</strong> ' . htmlspecialchars(trim((string) $module->getInfo('author')), ENT_QUOTES | ENT_HTML5, 'UTF-8');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php` at line 1500, The xoops_module_log_header() call is missing the explicit 'UTF-8' charset, so update the invocation to pass 'UTF-8' (matching how you encode module info displayed here) and ensure the output building line using htmlspecialchars(trim((string) $module->getInfo('author'))) uses that same charset context; specifically modify the xoops_module_log_header(...) call to include 'UTF-8' and keep the htmlspecialchars usage on $module->getInfo('author') intact so the author string is encoded consistently.
661-661: 🧹 Nitpick | 🔵 TrivialMissing
'UTF-8'charset parameter—same inconsistency as line 86.- $msgs[] = '<strong>' . _AUTHOR . ':</strong> ' . htmlspecialchars(trim($module->getInfo('author')), ENT_QUOTES | ENT_HTML5); + $msgs[] = '<strong>' . _AUTHOR . ':</strong> ' . htmlspecialchars(trim($module->getInfo('author')), ENT_QUOTES | ENT_HTML5, 'UTF-8');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php` at line 661, The call to htmlspecialchars in the modules list is missing the explicit 'UTF-8' encoding parameter (in the expression building $msgs[] with trim($module->getInfo('author')) and ENT_QUOTES | ENT_HTML5); update that htmlspecialchars invocation to pass 'UTF-8' as the third argument to match the encoding used elsewhere (same fix as applied at line 86) so character encoding is explicit and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpstan.neon`:
- Around line 22-23: The phpstan.neon references a non-existent stub file under
the stubFiles key: phpstan-constants.stub.php; either create that stub file with
the required constant/type definitions or remove the entry from phpstan.neon
(edit the stubFiles list) so PHPStan will not try to load a missing file. Ensure
the change targets the phpstan.neon configuration and the
phpstan-constants.stub.php identifier so future static analysis runs succeed.
---
Outside diff comments:
In `@htdocs/modules/system/admin/modulesadmin/main.php`:
- Line 217: The htmlspecialchars call setting $toinstall_mods[$i]['name'] uses
ENT_QUOTES|ENT_HTML5 but omits the explicit charset; update the call that wraps
$module->getInfo('name') to include 'UTF-8' as the fourth parameter (matching
the changes at the other lines), i.e. locate the assignment to
$toinstall_mods[$i]['name'] and modify the htmlspecialchars invocation to pass
ENT_QUOTES | ENT_HTML5 and 'UTF-8' so encoding is consistent.
- Line 138: The call that assigns $listed_mods[$i]['name'] uses
htmlspecialchars((string) $module->getVar('name'), ENT_QUOTES | ENT_HTML5)
without the charset argument; update this to include 'UTF-8' to match other
usages (e.g., the code at lines setting module names later) so replace the
htmlspecialchars invocation used in the $listed_mods assignment to include
'UTF-8' as the third parameter.
In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php`:
- Line 86: Update the htmlspecialchars call that builds $msgs[] for the module
author to include the explicit charset parameter; specifically change the call
wrapping trim($module->getInfo('author')) in the $msgs[] line so it passes
'UTF-8' as the third argument (preserving the existing flags ENT_QUOTES |
ENT_HTML5) to comply with the coding guideline.
- Line 1500: The xoops_module_log_header() call is missing the explicit 'UTF-8'
charset, so update the invocation to pass 'UTF-8' (matching how you encode
module info displayed here) and ensure the output building line using
htmlspecialchars(trim((string) $module->getInfo('author'))) uses that same
charset context; specifically modify the xoops_module_log_header(...) call to
include 'UTF-8' and keep the htmlspecialchars usage on
$module->getInfo('author') intact so the author string is encoded consistently.
- Line 661: The call to htmlspecialchars in the modules list is missing the
explicit 'UTF-8' encoding parameter (in the expression building $msgs[] with
trim($module->getInfo('author')) and ENT_QUOTES | ENT_HTML5); update that
htmlspecialchars invocation to pass 'UTF-8' as the third argument to match the
encoding used elsewhere (same fix as applied at line 86) so character encoding
is explicit and consistent.
🪄 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: 86549213-9e09-4260-aaed-39d9070241b3
📒 Files selected for processing (4)
htdocs/modules/system/admin/modulesadmin/main.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phpphpstan.neontests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
htdocs/modules/system/admin/modulesadmin/main.php:321
- In the
installflow,$modulecomes fromRequest::getString('module', '')and is passed directly to$mod->loadInfoAsVar($module)without validating that the module directory exists / thatloadInfoAsVar()succeeded. If an invalidmodulevalue is provided,loadInfo()can fail and subsequent$mod->getInfo()/getVar()calls may error. Consider validating$moduleagainstXoopsLists::getModulesList()(or checking theloadInfoAsVar()/loadInfo()return) and redirecting on failure.
case 'install':
$module = Request::getString('module', '');
// Get module handler
/** @var XoopsModuleHandler $module_handler */
$module_handler = xoops_getHandler('module');
$mod = $module_handler->create();
$mod->loadInfoAsVar($module);
// Construct message
htdocs/modules/system/admin/modulesadmin/main.php:382
- In the
uninstallflow,$mod = $module_handler->getByDirname($module)can returnfalse(per handler contract), but the code immediately calls$mod->getInfo()/$mod->getVar(). Add aninstanceof XoopsModule(oris_object) check and handle the error (e.g., redirect) to avoid fatal errors on invalidmoduleinput.
case 'uninstall':
$module = Request::getString('module', '');
$msgs = '';
// Get module handler
/** @var XoopsModuleHandler $module_handler */
$module_handler = xoops_getHandler('module');
$mod = $module_handler->getByDirname($module);
// Construct message
htdocs/modules/system/admin/modulesadmin/main.php:445
- In the
updateflow,$mod = $module_handler->getByDirname($module)can returnfalse, but the code assumes it is an object and calls$mod->getInfo()/getVar(). Add a guard (e.g.,if (!$mod instanceof XoopsModule) redirect_header(...)) to prevent a fatal error when an invalidmoduleis provided.
case 'update':
$module = Request::getString('module', '');
// Get module handler
/** @var XoopsModuleHandler $module_handler */
$module_handler = xoops_getHandler('module');
$mod = $module_handler->getByDirname($module);
// Construct message
if ($mod->getInfo('image') !== false && trim($mod->getInfo('image')) != '') {
$msgs = '<img src="' . XOOPS_URL . '/modules/' . $mod->getVar('dirname', 'n') . '/' . trim($mod->getInfo('image')) . '" alt="" />';
| stubFiles: | ||
| - phpstan-constants.stub.php |
There was a problem hiding this comment.
phpstan.neon now references phpstan-constants.stub.php under stubFiles, but that file does not exist in the repository (and constants.php is also not present at repo root). This will break PHPStan runs unless the stub file is added (or the path updated to where it actually lives).
| stubFiles: | |
| - phpstan-constants.stub.php |
| scanFiles: | ||
| - htdocs/xoops_lib/vendor/kint-php/kint/src/Kint.php | ||
| bootstrapFiles: | ||
| - constants.php | ||
| stubFiles: | ||
| - phpstan-constants.stub.php |
There was a problem hiding this comment.
The PR description’s explicit file list does not mention phpstan.neon, but this PR modifies it. Either update the PR description to include this change or move the PHPStan config update into a separate PR to keep the scope clear.
| /** | ||
| * Verify that MyTextSanitizer is used for HTML output sanitization. | ||
| * Verify that htmlspecialchars is used for display output, but NOT for | ||
| * dirnames that feed into DB lookups and filesystem operations. | ||
| */ | ||
| public function testUsesTextSanitizerForOutput(): void | ||
| public function testUsesHtmlspecialcharsForDisplayOnly(): void | ||
| { | ||
| // Display names (list operation) are escaped for HTML output | ||
| $this->assertStringContainsString( | ||
| '$myts = \MyTextSanitizer::getInstance();', | ||
| 'htmlspecialchars(', | ||
| $this->sourceCode, | ||
| 'Should initialize MyTextSanitizer' | ||
| 'Should use htmlspecialchars for display output' | ||
| ); | ||
|
|
||
| // Should use htmlSpecialChars for output | ||
| $this->assertStringContainsString( | ||
| // MyTextSanitizer should no longer be used — dirnames and display | ||
| // names now use native htmlspecialchars where appropriate | ||
| $this->assertStringNotContainsString( | ||
| '$myts->htmlSpecialChars', | ||
| $this->sourceCode, | ||
| 'Should use htmlSpecialChars for output sanitization' | ||
| 'Should not use MyTextSanitizer htmlSpecialChars' | ||
| ); | ||
| $this->assertStringNotContainsString( | ||
| 'MyTextSanitizer::getInstance()', | ||
| $this->sourceCode, | ||
| 'Should not instantiate MyTextSanitizer' | ||
| ); | ||
| } |
There was a problem hiding this comment.
The docblock and method name claim this test verifies that htmlspecialchars is used for display output but not for dirnames used in DB/filesystem operations, but the assertions only check that htmlspecialchars( appears somewhere and that MyTextSanitizer usage is absent. Either tighten the assertions to specifically cover the dirname/non-dirname behavior described, or adjust the comment/name so they match what the test actually verifies.
…ions
Address CodeRabbit and Copilot review findings on the second push:
- Add phpstan-constants.stub.php that was referenced by phpstan.neon
but never committed. Both CodeRabbit and Copilot flagged this as
critical — PHPStan runs would fail with a missing file error.
- Tighten test assertions per Copilot feedback:
- testUsesHtmlspecialcharsForDisplayOnly now asserts the specific
ENT_QUOTES | ENT_HTML5 flags are present, not just the function name.
- New testDirnamesNotEscapedBeforeDbLookup verifies that the install,
uninstall, and update operations do NOT escape $module before passing
it to loadInfoAsVar() / getByDirname(). This directly tests the
escape-at-output-not-input fix from the previous commit.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpstan-constants.stub.php`:
- Around line 1-11: Add the standard XOOPS copyright header block at the top of
phpstan-constants.stub.php so the file begins with the required XOOPS header
before the existing docblock; specifically, prepend the standard multi-line
XOOPS copyright header immediately above the current "/** PHPStan constant stubs
for XOOPS core." docblock (i.e., before the existing PHP file-level comment) so
the file starts with the mandated header.
In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php`:
- Around line 246-273: The test testDirnamesNotEscapedBeforeDbLookup currently
only asserts dirname non-escaping for 'install', 'uninstall', and 'update'
sections; extend it to also cover the '_ok' branches so regressions in the
success-handling paths are caught. Use the same pattern calling
extractOperationSection('install_ok'), extractOperationSection('uninstall_ok'),
and extractOperationSection('update_ok'), assert each is not empty with
assertNotEmpty, and run the same
assertStringNotContainsString('htmlspecialchars($module', ...) checks (same
message text) for each *_ok section to mirror the existing checks.
- Around line 221-227: The test testUsesHtmlspecialcharsForDisplayOnly currently
only checks for the flag substring in $this->sourceCode; change the assertion to
verify the full call shape including flags and charset by asserting that
htmlspecialchars is invoked as htmlspecialchars(..., ENT_QUOTES | ENT_HTML5,
'UTF-8') (or use an appropriate regex/assertMatchesRegularExpression on
$this->sourceCode) so the test confirms both the flag combination and the
'UTF-8' charset are present.
🪄 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: f3d21d68-9976-40ba-a9ef-fee3e89bab8e
📒 Files selected for processing (2)
phpstan-constants.stub.phptests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
Outdated
Show resolved
Hide resolved
tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
Show resolved
Hide resolved
phpstan-constants.stub.php
Outdated
| * | ||
| * @phpstan-type non-empty-string |
There was a problem hiding this comment.
The PHPDoc tag @phpstan-type non-empty-string is syntactically invalid because @phpstan-type requires an alias (e.g. @phpstan-type NonEmptyString non-empty-string) and here it has none. This can cause PHPStan to error while parsing stubs; either remove this line or rewrite it with a proper alias and then actually use that alias in annotations.
| * | |
| * @phpstan-type non-empty-string |
phpstan-constants.stub.php
Outdated
| define('XOOPS_GROUP_ADMIN', 1); | ||
| define('XOOPS_GROUP_USERS', 2); | ||
| define('XOOPS_GROUP_ANONYMOUS', 3); |
There was a problem hiding this comment.
These stubbed group constants don’t match the types in htdocs/mainfile.dist.php (there they are defined as strings like '1', '2', '3'). Using ints here can change PHPStan’s inferred types and make analysis results diverge from runtime; align the stub values/types with the real definitions.
| define('XOOPS_GROUP_ADMIN', 1); | |
| define('XOOPS_GROUP_USERS', 2); | |
| define('XOOPS_GROUP_ANONYMOUS', 3); | |
| define('XOOPS_GROUP_ADMIN', '1'); | |
| define('XOOPS_GROUP_USERS', '2'); | |
| define('XOOPS_GROUP_ANONYMOUS', '3'); |
…r _ok branches
Address all CodeRabbit and Copilot findings from the latest review round
in one comprehensive pass:
- phpstan-constants.stub.php: add required XOOPS copyright header block,
remove invalid @phpstan-type annotation (missing alias), change group
constants from int to string ('1','2','3') matching mainfile.dist.php.
- MainControllerTest: strengthen testUsesHtmlspecialcharsForDisplayOnly
to assert the full call shape including 'UTF-8' charset, not just the
flag constants. Add assertions for install_ok, uninstall_ok, and
update_ok operation branches in testDirnamesNotEscapedBeforeDbLookup
to cover the actual execution paths alongside the confirmation dialogs.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php (1)
1218-1220: 🧹 Nitpick | 🔵 TrivialMinor dead code:
$endPos === falsecheck is unreachable.At line 1218,
$endPosis always assigned a value by lines 1209-1215. If$nextCaseisfalse, theelsebranch ensures$endPosgets either$lastBreak + 6orstrlen($this->sourceCode). The condition can never be true.♻️ Optional cleanup
- if ($endPos === false) { - return ''; - } - return substr($this->sourceCode, $startPos, $endPos - $startPos);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php` around lines 1218 - 1220, The check `if ($endPos === false) return '';` is unreachable because `$endPos` is always set by the preceding `if ($nextCase !== false) { ... } else { $endPos = $lastBreak + 6; /* or */ strlen($this->sourceCode); }` logic; remove this dead branch. Edit the test in MainControllerTest to delete the unreachable `$endPos === false` check (or replace it with an assertion/comment if you prefer) and ensure references to `$endPos`, `$nextCase`, `$lastBreak`, and `$this->sourceCode` remain unchanged so the remaining flow still uses the computed `$endPos`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php`:
- Around line 1218-1220: The check `if ($endPos === false) return '';` is
unreachable because `$endPos` is always set by the preceding `if ($nextCase !==
false) { ... } else { $endPos = $lastBreak + 6; /* or */
strlen($this->sourceCode); }` logic; remove this dead branch. Edit the test in
MainControllerTest to delete the unreachable `$endPos === false` check (or
replace it with an assertion/comment if you prefer) and ensure references to
`$endPos`, `$nextCase`, `$lastBreak`, and `$this->sourceCode` remain unchanged
so the remaining flow still uses the computed `$endPos`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed86a198-9a2f-4cf2-b271-c6ebb5f88e1e
📒 Files selected for processing (2)
phpstan-constants.stub.phptests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
| @@ -876,7 +872,7 @@ function xoops_module_update($dirname) | |||
| } | |||
| $msgs[] = '<strong>' . _VERSION . ':</strong> ' . $module->getInfo('version'); | |||
| if ($module->getInfo('author') !== false && trim($module->getInfo('author')) != '') { | |||
| $msgs[] = '<strong>' . _AUTHOR . ':</strong> ' . $myts->htmlSpecialChars(trim($module->getInfo('author'))); | |||
| $msgs[] = '<strong>' . _AUTHOR . ':</strong> ' . htmlspecialchars(trim($module->getInfo('author')), ENT_QUOTES | ENT_HTML5, 'UTF-8'); | |||
There was a problem hiding this comment.
In xoops_module_update(), $dirname is no longer HTML-escaped (the prior MyTextSanitizer call was removed), but it’s still interpolated into an HTML attribute for the module image URL. If a module dirname contains characters like quotes or <, this becomes an XSS risk in the admin UI. Keep $dirname unmodified for handler/path operations, but introduce a separately escaped/URL-encoded value when embedding it into HTML/URLs (and use that for the img src path segment).
| // Display names in the list/confirm operation must use the full | ||
| // htmlspecialchars(..., ENT_QUOTES | ENT_HTML5, 'UTF-8') call shape |
There was a problem hiding this comment.
The inline test comment says the list/confirm display output “must use” the full htmlspecialchars(..., ENT_QUOTES | ENT_HTML5, 'UTF-8') call shape, but the source under test still has list/installlist escaping calls without the explicit charset. Consider tightening the wording to match what the test actually enforces (presence of at least one call using the explicit UTF-8 form) to avoid misleading future maintainers.
| // Display names in the list/confirm operation must use the full | |
| // htmlspecialchars(..., ENT_QUOTES | ENT_HTML5, 'UTF-8') call shape | |
| // Verify the source contains at least one htmlspecialchars call using | |
| // ENT_QUOTES | ENT_HTML5 with an explicit 'UTF-8' charset. |
The earlier commit removed early htmlspecialchars() on $dirname to fix
the DB lookup bug, but left the img src output point unescaped. If a
module dirname or image filename contained quotes or angle brackets,
the raw interpolation into the src attribute would be an XSS vector.
Escape both $dirname and $module->getInfo('image') at the two HTML
output sites (lines 657 and 871 in xoops_module_install and
xoops_module_update) using htmlspecialchars() with ENT_QUOTES |
ENT_HTML5. The dirname stays raw for all internal operations (DB
lookup, filesystem, cache clearing).
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
htdocs/modules/system/admin/modulesadmin/modulesadmin.php:661
- In this log header,
$dirname/imageare escaped withhtmlspecialchars(..., ENT_QUOTES | ENT_HTML5, 'UTF-8'), but the author line useshtmlspecialchars(..., ENT_QUOTES | ENT_HTML5)without an explicit charset. For consistent output encoding (and to avoid relying ondefault_charset), pass the same'UTF-8'(or_CHARSET) third argument here as well.
$msgs[] = '<img src="' . XOOPS_URL . '/modules/' . htmlspecialchars($dirname, ENT_QUOTES | ENT_HTML5, 'UTF-8') . '/' . htmlspecialchars(trim($module->getInfo('image')), ENT_QUOTES | ENT_HTML5, 'UTF-8') . '" alt="" />';
}
$msgs[] = '<strong>' . _VERSION . ':</strong> ' . $module->getInfo('version');
if ($module->getInfo('author') !== false && trim($module->getInfo('author')) != '') {
$msgs[] = '<strong>' . _AUTHOR . ':</strong> ' . htmlspecialchars(trim($module->getInfo('author')), ENT_QUOTES | ENT_HTML5);
| // used for loadInfoAsVar() and getByDirname(). It must NOT be escaped | ||
| // before those calls — escaping would change the identifier. | ||
| $installSection = $this->extractOperationSection('install'); | ||
| $this->assertNotEmpty($installSection, 'Install operation should exist'); | ||
| $this->assertStringNotContainsString( | ||
| 'htmlspecialchars($module', | ||
| $installSection, | ||
| 'Install operation must not escape $module dirname before DB lookup' | ||
| ); | ||
|
|
||
| $uninstallSection = $this->extractOperationSection('uninstall'); | ||
| $this->assertNotEmpty($uninstallSection, 'Uninstall operation should exist'); | ||
| $this->assertStringNotContainsString( | ||
| 'htmlspecialchars($module', | ||
| $uninstallSection, | ||
| 'Uninstall operation must not escape $module dirname before DB lookup' | ||
| ); | ||
|
|
||
| $updateSection = $this->extractOperationSection('update'); | ||
| $this->assertNotEmpty($updateSection, 'Update operation should exist'); | ||
| $this->assertStringNotContainsString( | ||
| 'htmlspecialchars($module', | ||
| $updateSection, | ||
| 'Update operation must not escape $module dirname before DB lookup' | ||
| ); | ||
|
|
||
| // The _ok branches perform the actual module install/uninstall/update | ||
| // and also use $module as a dirname for handler lookups. | ||
| $installOkSection = $this->extractOperationSection('install_ok'); | ||
| $this->assertNotEmpty($installOkSection, 'install_ok operation should exist'); | ||
| $this->assertStringNotContainsString( | ||
| 'htmlspecialchars($module', | ||
| $installOkSection, | ||
| 'install_ok operation must not escape $module dirname' | ||
| ); | ||
|
|
||
| $uninstallOkSection = $this->extractOperationSection('uninstall_ok'); | ||
| $this->assertNotEmpty($uninstallOkSection, 'uninstall_ok operation should exist'); | ||
| $this->assertStringNotContainsString( | ||
| 'htmlspecialchars($module', | ||
| $uninstallOkSection, | ||
| 'uninstall_ok operation must not escape $module dirname' | ||
| ); | ||
|
|
||
| $updateOkSection = $this->extractOperationSection('update_ok'); | ||
| $this->assertNotEmpty($updateOkSection, 'update_ok operation should exist'); | ||
| $this->assertStringNotContainsString( | ||
| 'htmlspecialchars($module', | ||
| $updateOkSection, | ||
| 'update_ok operation must not escape $module dirname' |
There was a problem hiding this comment.
These assertions check that the operation sections do not contain the substring htmlspecialchars($module anywhere, but the docstring says “must NOT be escaped before DB lookup”. As written, the test would also fail if $module were escaped later for display (which would be valid) and doesn’t actually assert what argument is passed to loadInfoAsVar() / getByDirname(). Consider asserting directly that the lookup calls use raw $module (e.g., match loadInfoAsVar($module) / getByDirname($module) and ensure those call expressions do not wrap $module), instead of banning the substring in the whole section.
| // used for loadInfoAsVar() and getByDirname(). It must NOT be escaped | |
| // before those calls — escaping would change the identifier. | |
| $installSection = $this->extractOperationSection('install'); | |
| $this->assertNotEmpty($installSection, 'Install operation should exist'); | |
| $this->assertStringNotContainsString( | |
| 'htmlspecialchars($module', | |
| $installSection, | |
| 'Install operation must not escape $module dirname before DB lookup' | |
| ); | |
| $uninstallSection = $this->extractOperationSection('uninstall'); | |
| $this->assertNotEmpty($uninstallSection, 'Uninstall operation should exist'); | |
| $this->assertStringNotContainsString( | |
| 'htmlspecialchars($module', | |
| $uninstallSection, | |
| 'Uninstall operation must not escape $module dirname before DB lookup' | |
| ); | |
| $updateSection = $this->extractOperationSection('update'); | |
| $this->assertNotEmpty($updateSection, 'Update operation should exist'); | |
| $this->assertStringNotContainsString( | |
| 'htmlspecialchars($module', | |
| $updateSection, | |
| 'Update operation must not escape $module dirname before DB lookup' | |
| ); | |
| // The _ok branches perform the actual module install/uninstall/update | |
| // and also use $module as a dirname for handler lookups. | |
| $installOkSection = $this->extractOperationSection('install_ok'); | |
| $this->assertNotEmpty($installOkSection, 'install_ok operation should exist'); | |
| $this->assertStringNotContainsString( | |
| 'htmlspecialchars($module', | |
| $installOkSection, | |
| 'install_ok operation must not escape $module dirname' | |
| ); | |
| $uninstallOkSection = $this->extractOperationSection('uninstall_ok'); | |
| $this->assertNotEmpty($uninstallOkSection, 'uninstall_ok operation should exist'); | |
| $this->assertStringNotContainsString( | |
| 'htmlspecialchars($module', | |
| $uninstallOkSection, | |
| 'uninstall_ok operation must not escape $module dirname' | |
| ); | |
| $updateOkSection = $this->extractOperationSection('update_ok'); | |
| $this->assertNotEmpty($updateOkSection, 'update_ok operation should exist'); | |
| $this->assertStringNotContainsString( | |
| 'htmlspecialchars($module', | |
| $updateOkSection, | |
| 'update_ok operation must not escape $module dirname' | |
| // used for loadInfoAsVar() and getByDirname(). It must be passed raw | |
| // to those lookup calls — escaping would change the identifier. | |
| $installSection = $this->extractOperationSection('install'); | |
| $this->assertNotEmpty($installSection, 'Install operation should exist'); | |
| $this->assertMatchesRegularExpression( | |
| '/loadInfoAsVar\(\s*\$module\s*\)/', | |
| $installSection, | |
| 'Install operation should pass raw $module to loadInfoAsVar()' | |
| ); | |
| $uninstallSection = $this->extractOperationSection('uninstall'); | |
| $this->assertNotEmpty($uninstallSection, 'Uninstall operation should exist'); | |
| $this->assertMatchesRegularExpression( | |
| '/getByDirname\(\s*\$module\s*\)/', | |
| $uninstallSection, | |
| 'Uninstall operation should pass raw $module to getByDirname()' | |
| ); | |
| $updateSection = $this->extractOperationSection('update'); | |
| $this->assertNotEmpty($updateSection, 'Update operation should exist'); | |
| $this->assertMatchesRegularExpression( | |
| '/loadInfoAsVar\(\s*\$module\s*\)/', | |
| $updateSection, | |
| 'Update operation should pass raw $module to loadInfoAsVar()' | |
| ); | |
| // The _ok branches perform the actual module install/uninstall/update | |
| // and also use $module as a dirname for handler lookups. | |
| $installOkSection = $this->extractOperationSection('install_ok'); | |
| $this->assertNotEmpty($installOkSection, 'install_ok operation should exist'); | |
| $this->assertMatchesRegularExpression( | |
| '/getByDirname\(\s*\$module\s*\)/', | |
| $installOkSection, | |
| 'install_ok operation should pass raw $module to getByDirname()' | |
| ); | |
| $uninstallOkSection = $this->extractOperationSection('uninstall_ok'); | |
| $this->assertNotEmpty($uninstallOkSection, 'uninstall_ok operation should exist'); | |
| $this->assertMatchesRegularExpression( | |
| '/getByDirname\(\s*\$module\s*\)/', | |
| $uninstallOkSection, | |
| 'uninstall_ok operation should pass raw $module to getByDirname()' | |
| ); | |
| $updateOkSection = $this->extractOperationSection('update_ok'); | |
| $this->assertNotEmpty($updateOkSection, 'update_ok operation should exist'); | |
| $this->assertMatchesRegularExpression( | |
| '/getByDirname\(\s*\$module\s*\)/', | |
| $updateOkSection, | |
| 'update_ok operation should pass raw $module to getByDirname()' |
… htmlspecialchars()
Replace 7 occurrences of $myts->htmlSpecialChars() with native
htmlspecialchars($x, ENT_QUOTES | ENT_HTML5, 'UTF-8') in the module
admin pages where the input is structurally constrained to identifier-
like values (directory names, module display names, author strings)
that cannot contain pre-existing HTML entities.
The MyTextSanitizer wrapper has a post-processing step that un-encodes
& back to & — designed for free-form text that may already contain
HTML entities from the database. These 7 call sites handle module
dirnames and metadata where that & pass-through is unnecessary and
the standard PHP function provides correct, simpler escaping.
Also removes the now-unused $myts = \MyTextSanitizer::getInstance()
initialization from both files, and updates the source-asserting test
to match the new pattern.
Closes #869 (partial — only the identifier-constrained call sites;
the remaining ~78 occurrences depend on the & pass-through contract
and cannot be safely replaced without a broader data-model migration).
Files changed:
Full suite: 6040 tests, 10527 assertions, 0 failures.
Summary by CodeRabbit
Refactor
Tests
Chores