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 2 commits
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
9 changes: 2 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,6 @@

case 'install':
$module = Request::getString('module', '');
$module = $myts->htmlSpecialChars($module);
// 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 +374,6 @@

case 'uninstall':
$module = Request::getString('module', '');
$module = $myts->htmlSpecialChars($module);
$msgs = '';
// Get module handler
/** @var XoopsModuleHandler $module_handler */
Expand Down Expand Up @@ -440,7 +436,6 @@

case 'update':
$module = Request::getString('module', '');
$module = $myts->htmlSpecialChars($module);
// Get module handler
/** @var XoopsModuleHandler $module_handler */
$module_handler = xoops_getHandler('module');
Expand Down
6 changes: 1 addition & 5 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 @@ -831,10 +831,6 @@ function xoops_module_update($dirname)
global $xoopsUser, $xoopsConfig, $xoopsTpl;
$dirname = trim((string) $dirname);
$xoopsDB =& $GLOBALS['xoopsDB'];

$myts = \MyTextSanitizer::getInstance();

$dirname = $myts->htmlSpecialChars(trim($dirname));
/** @var XoopsModuleHandler $module_handler */
$module_handler = xoops_getHandler('module');
$module = $module_handler->getByDirname($dirname);
Expand Down Expand Up @@ -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');

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
4 changes: 2 additions & 2 deletions phpstan.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ parameters:
- htdocs/xoops_lib/vendor/
scanFiles:
- htdocs/xoops_lib/vendor/kint-php/kint/src/Kint.php
bootstrapFiles:
- constants.php
stubFiles:
- phpstan-constants.stub.php
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +22 to +23

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.

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

Suggested change
stubFiles:
- phpstan-constants.stub.php

Copilot uses AI. Check for mistakes.
Comment on lines 20 to +23

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

Copilot uses AI. Check for mistakes.
treatPhpDocTypesAsCertain: false
22 changes: 15 additions & 7 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,29 @@ public function testSanitizesPostDataUsingFilterInput(): void
}

/**
* 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'
);
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 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