Skip to content

refactor(modulesadmin): replace $myts->htmlSpecialChars() with native… #1689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mambax7 merged 5 commits into from
Apr 7, 2026
Merged

refactor(modulesadmin): replace $myts->htmlSpecialChars() with native… #1689

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
043d7b7
refactor(modulesadmin): replace $myts->htmlSpecialChars() with native…
mambax7 Apr 7, 2026
075b5e6
refactor(modulesadmin): escape dirnames at output only, fix PHPStan c…
mambax7 Apr 7, 2026
b17dd2e
fix(modulesadmin): add missing PHPStan stub file, tighten test assert…
mambax7 Apr 7, 2026
63e8ba9
fix(modulesadmin): add XOOPS header to stub, fix constant types, cove…
mambax7 Apr 7, 2026
a07d75d
fix(modulesadmin): escape dirname and image at HTML output point
mambax7 Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions htdocs/modules/system/admin/modulesadmin/main.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,6 @@
exit;
}

$myts = \MyTextSanitizer::getInstance();

// Define main template
$GLOBALS['xoopsOption']['template_main'] = 'system_modules.tpl';
// Call Header
Expand Down Expand Up @@ -267,8 +265,8 @@
$mid = (int) $mid;
$newname[$mid] = trim((string) XoopsFilterInput::clean($newname[$mid], 'STRING'));
$modifs_mods[$i]['mid'] = $mid;
$modifs_mods[$i]['oldname'] = $myts->htmlSpecialChars($oldname[$mid]);
$modifs_mods[$i]['newname'] = $myts->htmlSpecialChars(trim($newname[$mid]));
$modifs_mods[$i]['oldname'] = htmlspecialchars($oldname[$mid], ENT_QUOTES | ENT_HTML5, 'UTF-8');
$modifs_mods[$i]['newname'] = htmlspecialchars(trim($newname[$mid]), ENT_QUOTES | ENT_HTML5, 'UTF-8');
++$i;
}
$xoopsTpl->assign('modifs_mods', $modifs_mods);
Expand Down Expand Up @@ -315,7 +313,7 @@

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 */
Comment on lines 314 to 317

Copilot AI Apr 7, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$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.

Copilot uses AI. Check for mistakes.
$module_handler = xoops_getHandler('module');
Expand Down Expand Up @@ -377,7 +375,7 @@

case 'uninstall':
$module = Request::getString('module', '');
$module = $myts->htmlSpecialChars($module);
$module = htmlspecialchars($module, ENT_QUOTES | ENT_HTML5, 'UTF-8');
$msgs = '';
// Get module handler
/** @var XoopsModuleHandler $module_handler */
Expand Down Expand Up @@ -440,7 +438,7 @@

case 'update':
$module = Request::getString('module', '');
$module = $myts->htmlSpecialChars($module);
$module = htmlspecialchars($module, ENT_QUOTES | ENT_HTML5, 'UTF-8');
// Get module handler
/** @var XoopsModuleHandler $module_handler */
$module_handler = xoops_getHandler('module');
Expand Down
6 changes: 2 additions & 4 deletions htdocs/modules/system/admin/modulesadmin/modulesadmin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -832,9 +832,7 @@ function xoops_module_update($dirname)
$dirname = trim((string) $dirname);
$xoopsDB =& $GLOBALS['xoopsDB'];

$myts = \MyTextSanitizer::getInstance();

$dirname = $myts->htmlSpecialChars(trim($dirname));
$dirname = htmlspecialchars(trim($dirname), ENT_QUOTES | ENT_HTML5, 'UTF-8');

Copilot AI Apr 7, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$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.

Suggested change
$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 '';
}

Copilot uses AI. Check for mistakes.
/** @var XoopsModuleHandler $module_handler */
$module_handler = xoops_getHandler('module');
$module = $module_handler->getByDirname($dirname);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Expand Down Expand Up @@ -876,7 +874,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');

Copilot AI Apr 7, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
}
$msgs[] = '</div><div class="logger">';

Expand Down
19 changes: 8 additions & 11 deletions tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,21 +215,18 @@ public function testSanitizesPostDataUsingFilterInput(): void
}

/**
* Verify that MyTextSanitizer is used for HTML output sanitization.
* Verify that module dirnames and names are escaped for HTML output.
*/
public function testUsesTextSanitizerForOutput(): void
public function testUsesHtmlspecialcharsForOutput(): void
{
// Module dirnames and display names are identifier-like values that
// use native htmlspecialchars() with ENT_QUOTES | ENT_HTML5 instead
// of the MyTextSanitizer wrapper (which preserves & for free-form
// text — not needed for dirnames).
$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'
);
Comment on lines 225 to 241

Copilot AI Apr 7, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
Comment on lines 217 to 242

Copilot AI Apr 7, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

Expand Down
Loading