From ee0442e5e9c63c7720cfdb1a00e52aff535a663d Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 11:39:12 -0400 Subject: [PATCH 01/12] 1.7.0 Alpha 1 --- CHANGELOG.md | 42 +++ README.md | 4 +- admin/admin_footer.php | 2 +- admin/index.php | 7 +- admin/sets.php | 44 ++- assets/css/admin.css | 134 ++++++++- class/AdminBulkPage.php | 315 ++++++++++++++++----- class/Lang.php | 193 +++++++++++++ class/ModuleActionResult.php | 136 ++++++++- class/ModuleActionService.php | 103 +++++-- class/Report/LegacyModuleReportAdapter.php | 235 +++++++++++++++ class/Report/LogEvent.php | 51 ++++ class/Report/LogEventHtmlRenderer.php | 69 +++++ class/Report/LogFragment.php | 32 +++ class/Report/LogSeverity.php | 41 +++ class/Report/ModuleOperationResult.php | 191 +++++++++++++ class/Report/Outcome.php | 39 +++ class/Set/ModuleSetApplier.php | 36 ++- class/Set/ModuleSetResolver.php | 9 +- docs/TUTORIAL.md | 4 +- docs/changelog.txt | 55 +++- docs/lang_diff.txt | 44 +++ docs/readme.txt | 4 +- language/english/admin.php | 41 +++ xoops_version.php | 6 +- 25 files changed, 1678 insertions(+), 159 deletions(-) create mode 100644 class/Lang.php create mode 100644 class/Report/LegacyModuleReportAdapter.php create mode 100644 class/Report/LogEvent.php create mode 100644 class/Report/LogEventHtmlRenderer.php create mode 100644 class/Report/LogFragment.php create mode 100644 class/Report/LogSeverity.php create mode 100644 class/Report/ModuleOperationResult.php create mode 100644 class/Report/Outcome.php diff --git a/CHANGELOG.md b/CHANGELOG.md index e3c8786..58ddffa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,48 @@ Baseline comparison for **1.6.0**: previous public tree `1.5.1-Final` --- +## [1.7.0 Alpha 1] — 2026-07-30 + +### Added + +- Structured reporting package with typed log fragments, events, severities, + outcomes, operation results, a legacy adapter, and a safe HTML renderer +- `ModuleActionResult::events()`, `messageText()`, and `toOperationResult()` for + HTML-independent report consumption +- `ModuleActionService::operationResult()` and `operationResults()` for callers + adopting `Report\ModuleOperationResult` +- 27 translatable admin-report strings with safe English fallbacks + +### Changed + +- Install-action reports are now structured data that can be rendered to HTML or + consumed as plain text, CLI output, logs, or JSON +- `ModuleActionResult` is deprecated in favor of `Report\ModuleOperationResult`; + existing `runOne()` and `runMany()` callers remain supported through XOOPS 2.8 +- RTL output now isolates counters, versions, statuses, folders, and plan summaries +- Admin report layout uses logical CSS properties, removes module-owned inline + styles, improves long-log scrolling, and avoids duplicate screen-reader labels +- Expanded cross-platform tests for hostile log input, structured-result contracts, + translations, logo paths, UTF-8, crashes, and timeouts + +### Fixed + +- Invalid UTF-8 caused by byte-based NBSP trimming +- Duplicate failure transcripts in structured results +- Blank pages caused by exceptions while loading a module manifest after an action +- Fatal or unusable admin output from missing, blank, non-scalar, or malformed + language strings +- Incorrect protected-module action wording + +### Security + +- Replaced direct install-log markup rendering with an escape-once parser that emits + only fixed module-owned markup and drops executable or raw-text element bodies +- Centralized module-logo URL validation and rejection of remote, missing, absolute, + traversal, and otherwise unsafe paths + +--- + ## [1.6.0] — 2026-07-23 ### Added diff --git a/README.md b/README.md index 48a598f..c0a903d 100644 --- a/README.md +++ b/README.md @@ -15,12 +15,12 @@ uninstalling). | | | |---|---| -| **Current version** | 1.6.0 Stable (2026-07-23) | +| **Current version** | 1.7.0 Alpha 1 (2026-07-30) | | **Previous baseline** | 1.5.1 Final (2022-12-15) | | **License** | GNU GPL 2.0 | | **Author** | Michael Beck (Mamba) | -Full history: [CHANGELOG.md](CHANGELOG.md) · [docs/changelog.txt](docs/changelog.txt) +Full history: [CHANGELOG.md](CHANGELOG.md) · [docs/changelog.txt](docs/changelog.txt) · [docs/lang_diff.txt](docs/lang_diff.txt) How-to: [docs/TUTORIAL.md](docs/TUTORIAL.md) --- diff --git a/admin/admin_footer.php b/admin/admin_footer.php index 603e0d2..30875c6 100644 --- a/admin/admin_footer.php +++ b/admin/admin_footer.php @@ -18,6 +18,6 @@ $pathIcon32 = Admin::iconUrl('', '32'); //$pathIcon32 = Xmf\Module\Admin::iconUrl('', '32'); -echo "
\n" . "
\n" . " XOOPS\n" . "
\n" . ' ' . _AM_MODULEADMIN_ADMIN_FOOTER . "\n" . '
'; +echo "
\n" . "
\n" . " XOOPS\n" . "
\n" . ' ' . _AM_MODULEADMIN_ADMIN_FOOTER . "\n" . '
'; xoops_cp_footer(); diff --git a/admin/index.php b/admin/index.php index a342f31..f0620c2 100644 --- a/admin/index.php +++ b/admin/index.php @@ -71,12 +71,15 @@ echo '

' . _AM_MODULEINSTALLER_DASH_SETS . '

'; echo '
' . \count($sets) . '
'; -echo '

' . \sprintf(_AM_MODULEINSTALLER_DASH_SETS_COUNT, \count($sets)) . '

'; +// Isolate the rendered sentence, NOT the numeral: a %d placeholder casts its +// argument to int, so markup passed into it is silently discarded. Without the +// isolate, '%d saved set(s)' renders as "saved set(s) 0" in an RTL admin. +echo '

' . \sprintf(_AM_MODULEINSTALLER_DASH_SETS_COUNT, \count($sets)) . '

'; echo '

' . _AM_MODULEINSTALLER_DASH_GO_SETS . '

'; echo '

' . _AM_MODULEINSTALLER_DASH_UPDATES . '

'; echo '
' . \count($needsUpdate) . '
'; -echo '

' . \sprintf(_AM_MODULEINSTALLER_DASH_UPDATES_COUNT, \count($needsUpdate)) . '

'; +echo '

' . \sprintf(_AM_MODULEINSTALLER_DASH_UPDATES_COUNT, \count($needsUpdate)) . '

'; echo '

' . _AM_MODULEINSTALLER_DASH_GO_UPDATE . '

'; echo '

' . _AM_MODULEINSTALLER_DASH_ACTIVE . '

'; diff --git a/admin/sets.php b/admin/sets.php index 01da1fe..3f826a5 100644 --- a/admin/sets.php +++ b/admin/sets.php @@ -241,7 +241,7 @@ function renderSetList(ModuleSetRepository $repository, ModuleSetResolver $resol $adminObject->addItemButton(_AM_MODULEINSTALLER_SET_FROM_ACTIVE, 'javascript:document.getElementById("form-from-active").submit();', 'button_ok'); $adminObject->displayButton('left'); - echo '
'; echo ''; - echo ''; + echo '

' . _AM_MODULEINSTALLER_SET_FILTER_NONE . '

'; $onDisk = $catalog->listOnDisk(); // Include missing members so user can uncheck/remove them @@ -418,8 +418,9 @@ function renderSetEditForm( $inSet = isset($selectedMap[$dirname]); $checked = $inSet ? ' checked' : ''; $rowClass = $class . ' set-mod-row' . ($inSet ? ' installer-row-selected' : ''); - $cellBg = $inSet ? 'background-color:var(--installer-selected,#E6EFC2);' : ''; - $rowStyle = $inSet ? ' style="background-color:var(--installer-selected,#E6EFC2);"' : ''; + // Row and cell colour come from .installer-row-selected in admin.css, which + // declares them with !important for tr and for tr > td alike, so the inline + // copies that used to be built here were redundant AND already overridden. $label = null !== $info ? \htmlspecialchars($info['name'], ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . ' (/' . \htmlspecialchars($dirname, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . ')' @@ -429,7 +430,12 @@ function renderSetEditForm( $img = ''; if (null !== $info && '' !== $info['image']) { - $img = ''; + // One logo authority for the whole module: AdminBulkPage::moduleLogoUrl() + // proves the manifest's image is a real file inside the module directory. + $logoUrl = \XoopsModules\Moduleinstaller\AdminBulkPage::moduleLogoUrl((string) $info['dirname'], $info); + $img = null === $logoUrl + ? '' + : ''; } $dirnameEsc = \htmlspecialchars($dirname, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); @@ -438,16 +444,22 @@ function renderSetEditForm( ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8' ); - $tdStyle = $cellBg !== '' ? ' style="' . $cellBg . '"' : ''; - echo ''; - echo '' . $img . ''; - echo '' . $label; + echo ''; + // _AM_MODULEINSTALLER_TOGGLE_SELECTION already exists and is used by + // AdminBulkPage; these two were the last places still hardcoding it. + $toggleTitle = \htmlspecialchars( + \XoopsModules\Moduleinstaller\Lang::text('_AM_MODULEINSTALLER_TOGGLE_SELECTION', 'Toggle selection'), + ENT_QUOTES | ENT_SUBSTITUTE, + 'UTF-8' + ); + echo '' . $img . ''; + echo '' . $label; if (null !== $info && '' !== $info['description']) { echo '
' . \htmlspecialchars($info['description'], ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . ''; } echo ''; - echo '' . $statusLabel . ''; - echo ''; + echo '' . $statusLabel . ''; + echo ''; echo ''; } echo ''; @@ -621,14 +633,18 @@ function renderApplyForm( echo "
" . _AM_MODULEINSTALLER_SET_PREVIEW_EMPTY . '
'; } else { $counts = installer_plan_counts($plan); - echo '
' . \sprintf( + // Five numerals separated by '·' in one sentence: the worst bidi case in the + // module. The counts cannot be wrapped individually — the format string uses + // %1$d..%5$d and a numeric placeholder casts its argument to int, discarding + // any markup — so the rendered sentence is isolated as a unit instead. + echo '
' . \sprintf( _AM_MODULEINSTALLER_SET_PLAN_SUMMARY, $counts['activate'], $counts['deactivate'], $counts['install'], $counts['uninstall'], $counts['other'] - ) . '
'; + ) . '
'; echo ''; echo ''; echo ''; diff --git a/assets/css/admin.css b/assets/css/admin.css index 59ada28..fb67ea8 100644 --- a/assets/css/admin.css +++ b/assets/css/admin.css @@ -74,7 +74,7 @@ table.outer.module tr.installer-row-selected > td { .installer-apply-stack .installer-apply-action label { font-weight: bold; - margin-right: 0.5em; + margin-inline-end: 0.5em; } .installer-apply-stack .installer-apply-count { @@ -95,9 +95,9 @@ table.outer.module tr.installer-row-selected > td { } .installer-form-actions { - margin-top: 1.25em; - padding-top: 0.75em; - text-align: right; + margin-block-start: 1.25em; + padding-block-start: 0.75em; + text-align: end; clear: both; } @@ -105,14 +105,102 @@ table.outer.module tr.installer-row-selected > td { margin-top: 1.25em; } +/* No max-height/overflow: the report must scroll with the PAGE, not inside a + 28em cage that hides the tail of a long install log. For very large batch runs + collapse each row's transcript behind a
instead of capping height. */ .installer-result-log { list-style: none; margin: 0.75em 0; padding: 0.5em 0.75em; border: 1px solid #d0d0d0; border-radius: 6px; - max-height: 28em; - overflow: auto; +} + +/* Logo column first, matching the DOM order AdminBulkPage now emits. */ +.installer-result-log .installer-result-item { + display: grid; + grid-template-columns: auto minmax(0, 1fr); + gap: 1em; + align-items: start; + margin: 0; + padding: 0.9em 0.75em; + border-block-end: 1px solid #ddd; + border-inline-start: 4px solid #6c757d; + background-image: none; +} + +.installer-result-log .installer-result-item:last-child { + border-block-end: 0; +} + +.installer-result-log .installer-result-item.success { + border-inline-start-color: var(--installer-badge-active); +} + +.installer-result-log .installer-result-item.warning { + border-inline-start-color: var(--installer-badge-notinst); +} + +.installer-result-log .installer-result-item.error { + border-inline-start-color: var(--installer-badge-missing); +} + +.installer-result-body { + min-width: 0; +} + +.installer-result-label { + margin-block-end: 0.5em; + font-size: 1.05em; +} + +.installer-result-dirname { + color: #666; + font-size: 0.9em; +} + +/* The install transcript. Monospace because core's   indentation only lines + up in a fixed-pitch font; text-align is logical (start), never left, so the + report reads correctly in an RTL admin language. */ +.installer-result-message { + text-align: start; + line-height: 1.45; + font-family: Consolas, "Courier New", monospace; + font-size: 0.92em; + overflow-wrap: anywhere; +} + +/* The module's severity palette. ModuleActionResult emits these instead of core's + inline color:#ff0000, so the colours live here and can be re-themed, dark-moded + or overridden by a site stylesheet — none of which is possible with a style + attribute, and all three are blocked outright by a strict style-src CSP. + + Deliberately NOT scoped under .installer-result-message: they were, and that made + them unusable anywhere else, which is why two inline colours survived elsewhere in + the module. All three severities LogSeverity can emit are styled — a class the + renderer can produce with no rule behind it is a silent styling failure the first + time something constructs it. LogSeverity::Info emits no class, so it needs none. */ +.installer-log-error { + color: var(--installer-badge-missing, #c0392b); + font-weight: 600; +} + +.installer-log-warning { + color: var(--installer-badge-notinst, #b35309); + font-weight: 600; +} + +.installer-log-success { + color: var(--installer-badge-active, #14713b); + font-weight: 600; +} + +.installer-result-logo { + width: auto; + height: auto; + max-width: 96px; + max-height: 96px; + object-fit: contain; } /* Filter + list chrome (module-scoped only) */ @@ -125,7 +213,7 @@ table.outer.module tr.installer-row-selected > td { font-weight: 700; font-style: italic; color: #333; - margin-left: 0.15em; + margin-inline-start: 0.15em; } /* Count next to page title — light accent, does not restyle .CPbigTitle itself */ @@ -135,7 +223,7 @@ table.outer.module tr.installer-row-selected > td { font-weight: 600; font-style: normal; color: #555; - margin-left: 0.35em; + margin-inline-start: 0.35em; white-space: nowrap; } @@ -188,7 +276,7 @@ table.outer.module tr.installer-row-selected > td { } .installer-set-toolbar-hint { - margin-left: 0.15em; + margin-inline-start: 0.15em; color: #495057; } @@ -203,7 +291,7 @@ table.outer.module tr.installer-row-selected > td { .installer-filter-label { font-weight: bold; - margin-right: 0.25em; + margin-inline-end: 0.25em; } .installer-module-filter, @@ -326,7 +414,7 @@ table.module td.yesno, min-width: 7.5em !important; white-space: nowrap !important; vertical-align: middle !important; - text-align: left !important; + text-align: start !important; overflow: hidden; } @@ -470,12 +558,12 @@ td.installer-yesno-cell { margin: 0.5em 0 1em 0; padding: 0.6em 0.85em; background: #e7f1ff; - border-left: 4px solid #0d6efd; + border-inline-start: 4px solid #0d6efd; } .installer-dash-grid { display: grid; - grid-template-columns: repeat(auto-fill, minmax(14em, 1fr)); + grid-template-columns: repeat(auto-fit, minmax(14em, 1fr)); gap: 1em; margin: 1em 0; } @@ -499,8 +587,8 @@ td.installer-yesno-cell { .installer-tip { cursor: help; - border-bottom: 1px dotted #666; - margin-left: 0.25em; + border-block-end: 1px dotted #666; + margin-inline-start: 0.25em; color: #0d6efd; font-weight: bold; } @@ -612,3 +700,19 @@ input.installer-set-input { max-height: 52px; } } + +/* + * Initial-state utilities, so markup carries no style attribute. + * + * xo-installer.js toggles these elements by writing element.style.display, which is + * an inline style and therefore beats this class — so the class decides only the + * state the page is SERVED in, which is the part a strict style-src 'self' policy + * would otherwise block. + */ +.installer-hidden { + display: none; +} + +.installer-footer-center { + text-align: center; +} diff --git a/class/AdminBulkPage.php b/class/AdminBulkPage.php index 537b39f..485fbf0 100644 --- a/class/AdminBulkPage.php +++ b/class/AdminBulkPage.php @@ -25,8 +25,14 @@ */ final class AdminBulkPage { - /** Cache-bust query for admin CSS/JS (bump when assets change). */ - public const ASSET_VERSION = '1709'; + /** + * Cache-bust query for admin CSS/JS. + * + * Bump this in the SAME commit as any CSS/JS change: an admin holding the + * previous stylesheet otherwise sees none of it, which is how a CSS-only + * improvement can measure as no improvement at all. + */ + public const ASSET_VERSION = '1715'; /** * Full bulk page after xoops_cp_header() — navigation, list or report, form, assets. @@ -46,9 +52,7 @@ public static function serve(string $action): void if ('POST' === ($_SERVER['REQUEST_METHOD'] ?? 'GET')) { if (! self::checkCsrf()) { $pageHasForm = false; - $msg = \defined('_AM_MODULEINSTALLER_ERR_TOKEN') - ? \_AM_MODULEINSTALLER_ERR_TOKEN - : 'Invalid security token. Please try again.'; + $msg = Lang::text('_AM_MODULEINSTALLER_ERR_TOKEN', 'Invalid security token. Please try again.'); $content = "
" . \htmlspecialchars((string) $msg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
'; } else { $handled = self::handlePost($action, self::successTitleFor($action)); @@ -68,7 +72,7 @@ public static function serve(string $action): void echo $content; echo '

' . \htmlspecialchars( - \defined('_AM_MODULEINSTALLER_BACK_HOME') ? \_AM_MODULEINSTALLER_BACK_HOME : 'Installer home', + Lang::text('_AM_MODULEINSTALLER_BACK_HOME', 'Installer home'), \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8' ) @@ -149,10 +153,17 @@ private static function buildUpdateListPage(ModuleCatalog $catalog): array $content = ''; if ($onlyNeeds) { - $content .= "

" . \_AM_MODULEINSTALLER_PRESELECTED_UPDATES . '
'; + // Was a raw constant reference. On PHP 8 an undefined constant is a fatal + // Error, so a partial language pack turned this page blank. + $content .= "
" + . \htmlspecialchars( + Lang::text('_AM_MODULEINSTALLER_PRESELECTED_UPDATES', 'Pre-selected modules whose version on disk differs from the database.'), + \ENT_QUOTES | \ENT_SUBSTITUTE, + 'UTF-8' + ) . '
'; } $toggleUrl = 'update.php' . ($onlyNeeds ? '' : '?needs_only=1'); - $toggleLbl = \_AM_MODULEINSTALLER_SHOW_UPDATES_ONLY; + $toggleLbl = Lang::text('_AM_MODULEINSTALLER_SHOW_UPDATES_ONLY', 'Show only modules that need update'); $content .= '

' . \htmlspecialchars($toggleLbl, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ($onlyNeeds ? ' ✓' : '') . '

'; @@ -166,9 +177,7 @@ private static function buildUpdateListPage(ModuleCatalog $catalog): array private static function emptyListMessage(): string { - $msg = \defined('_AM_MODULEINSTALLER_NO_MODULES') - ? \_AM_MODULEINSTALLER_NO_MODULES - : 'No modules found.'; + $msg = Lang::text('_AM_MODULEINSTALLER_NO_MODULES', 'No modules found.'); return "
" . \htmlspecialchars((string) $msg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
'; } @@ -183,11 +192,9 @@ private static function successTitleFor(string $action): string ModuleActionService::ACTION_UPDATE => '_AM_MODULEINSTALLER_DONE_UPDATE', ]; $const = $map[$action] ?? ''; - if ('' !== $const && \defined($const)) { - return (string) \constant($const); - } + $fallback = \ucfirst($action) . ' complete'; - return \ucfirst($action) . ' complete'; + return '' === $const ? $fallback : Lang::text($const, $fallback); } private static function checkCsrf(): bool @@ -205,7 +212,7 @@ private static function wrapBulkForm(string $innerHtml, string $actionScript): s { $actionEsc = \htmlspecialchars($actionScript, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'); $submitLbl = \htmlspecialchars( - \defined('_AM_MODULEINSTALLER_SUBMIT') ? \_AM_MODULEINSTALLER_SUBMIT : 'Continue', + Lang::text('_AM_MODULEINSTALLER_SUBMIT', 'Continue'), \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8' ); @@ -245,18 +252,18 @@ public static function renderModuleTable(array $dirnames, array $preselect = [], { $catalog = new ModuleCatalog(); $preselect = \array_fill_keys($preselect, true); - $filterPh = \defined('_AM_MODULEINSTALLER_FILTER_PLACEHOLDER') ? \_AM_MODULEINSTALLER_FILTER_PLACEHOLDER : 'Filter…'; - $filterLbl = \defined('_AM_MODULEINSTALLER_FILTER') ? \_AM_MODULEINSTALLER_FILTER : 'Filter'; - $filterHint = \defined('_AM_MODULEINSTALLER_FILTER_HINT') ? \_AM_MODULEINSTALLER_FILTER_HINT : ''; - $noneMsg = \defined('_AM_MODULEINSTALLER_SET_FILTER_NONE') ? \_AM_MODULEINSTALLER_SET_FILTER_NONE : 'No match'; + $filterPh = Lang::text('_AM_MODULEINSTALLER_FILTER_PLACEHOLDER', 'Filter…'); + $filterLbl = Lang::text('_AM_MODULEINSTALLER_FILTER', 'Filter'); + $filterHint = Lang::text('_AM_MODULEINSTALLER_FILTER_HINT', ''); + $noneMsg = Lang::text('_AM_MODULEINSTALLER_SET_FILTER_NONE', 'No match'); $selectAllLbl = \htmlspecialchars( - \defined('_AM_MODULEINSTALLER_SELECT_ALL') ? \_AM_MODULEINSTALLER_SELECT_ALL : 'Select All', + Lang::text('_AM_MODULEINSTALLER_SELECT_ALL', 'Select All'), \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8' ); $selectNoneLbl = \htmlspecialchars( - \defined('_AM_MODULEINSTALLER_SELECT_NONE') ? \_AM_MODULEINSTALLER_SELECT_NONE : 'Un-Select All', + Lang::text('_AM_MODULEINSTALLER_SELECT_NONE', 'Un-Select All'), \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8' ); @@ -280,7 +287,7 @@ public static function renderModuleTable(array $dirnames, array $preselect = [], . \htmlspecialchars((string) $filterHint, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ''; } $content .= ''; - $content .= '

' . \htmlspecialchars((string) $noneMsg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '

'; // No ul/li wrapper: list markers broke first-row radio layout (black square + stretched Yes/No) $content .= "
" @@ -309,10 +316,18 @@ public static function renderModuleTable(array $dirnames, array $preselect = [], } } - $spanOpen = $highlight ? "" : ''; + // "needs update" is a warning, not an error, and it is a severity — so it + // uses the same palette class the transcript does rather than an inline + // colour that no theme or stylesheet can override. + $spanOpen = $highlight ? '' : ''; $spanClose = ''; - $imgSrc = \XOOPS_URL . '/modules/' . $info['dirname'] . '/' . $info['image']; + // Same resolver as the report: proves the target is a local file inside + // the module before building a URL from a manifest value. Concatenating + // $info['image'] straight onto XOOPS_URL — which this line used to do — + // escapes correctly but validates nothing, so two code paths disagreed + // about how much the manifest is trusted. + $imgSrc = self::moduleLogoUrl($file, $info) ?? ''; $dirnameEsc = \htmlspecialchars($file, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'); $nameEsc = \htmlspecialchars($info['name'], \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'); $toggleJs = "toggleModuleRow('" . $dirnameEsc . "')"; @@ -324,16 +339,30 @@ public static function renderModuleTable(array $dirnames, array $preselect = [], $even = ! $even; $stripe = $even ? 'even' : 'odd'; $rowClass = \trim($stripe . ($value !== 0 ? ' installer-row-selected' : '')); - $content .= "
\n"; - $content .= " \n"; - $content .= " \n"; + $toggleTitle = \htmlspecialchars( + Lang::text('_AM_MODULEINSTALLER_TOGGLE_SELECTION', 'Toggle selection'), + \ENT_QUOTES | \ENT_SUBSTITUTE, + 'UTF-8' + ); + $content .= " \n"; + $content .= " \n"; @@ -347,18 +376,76 @@ public static function renderModuleTable(array $dirnames, array $preselect = [], return 0 === $count ? self::emptyListMessage() : $content; } + /** + * Escaped text that goes before and after the live selection counter. + * + * Returns DATA, not markup, and takes no markup: an earlier version accepted the + * counter element as a string parameter and concatenated it verbatim, which made + * a public method whose second argument is trusted HTML — a trap for any caller + * that passes something user-derived. The counter element is built by the one + * caller that owns it; this method only decides where the text splits. + * + * Pure and public so it can be tested directly: the count itself is written by + * JavaScript, so none of this is reachable through a rendered page. + * + * Three things the naive split gets wrong, all of them translator-facing: + * - '%1$d selected' and '%s ausgewählt' are valid translations of a '%d' + * string, and explode('%d') matches neither; + * - '%%' is a LITERAL percent, not a placeholder, so '50%% of %d' must split + * at the second construct and render "50% of"; + * - this string takes exactly ONE argument, so a translation carrying a second + * placeholder is a mistake — and showing an admin a raw '%2$d' is worse than + * dropping it. Dropping it takes the space in front of it too, or + * '%1$d: %2$s selected' would render ': selected' with a doubled gap. + * + * @param string $template language string containing one placeholder + * + * @return array{0:string, 1:string} escaped prefix and suffix + */ + public static function selectedCountParts(string $template): array + { + // (*SKIP)(*FAIL) makes the engine step over '%%' without matching it. + $placeholder = '/%%(*SKIP)(*FAIL)|%(?:\d+\$)?[dsu]/'; + // The same construct, plus the single space that would otherwise be orphaned + // when the whole placeholder is removed rather than substituted. + $strayPlaceholder = '/[ \t]?(?:%%(*SKIP)(*FAIL)|%(?:\d+\$)?[dsu])/'; + + $parts = \preg_split($placeholder, $template, 2); + if (false === $parts || 1 === \count($parts)) { + // No recognisable placeholder: keep the text and put the counter first, + // rather than dropping either of them. + $parts = ['', '' === $template ? '' : ' ' . $template]; + } + + $clean = static function (string $text) use ($strayPlaceholder): string { + // Any FURTHER placeholder belongs to no argument; drop it, then collapse + // the escaped percents that survived the split. + $text = (string) \preg_replace($strayPlaceholder, '', $text); + + return \str_replace('%%', '%', $text); + }; + + return [ + \htmlspecialchars($clean($parts[0]), \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'), + \htmlspecialchars($clean($parts[1] ?? ''), \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'), + ]; + } + /** * Sticky selection counter + empty-guard messaging for bulk forms. * Optional $actionButtonsHtml repeats Select All controls so every tab has them at the bottom. */ public static function renderStickyBar(string $actionButtonsHtml = ''): string { - $countTpl = \defined('_AM_MODULEINSTALLER_SELECTED_COUNT') ? \_AM_MODULEINSTALLER_SELECTED_COUNT : '%d selected'; - $noneMsg = \defined('_AM_MODULEINSTALLER_ERR_NONE_SELECTED') ? \_AM_MODULEINSTALLER_ERR_NONE_SELECTED : 'Select at least one module.'; - $parts = \explode('%d', (string) $countTpl, 2); - $label = \htmlspecialchars($parts[0] ?? '', \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') - . '0' - . \htmlspecialchars($parts[1] ?? '', \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'); + $countTpl = Lang::text('_AM_MODULEINSTALLER_SELECTED_COUNT', '%d selected'); + $noneMsg = Lang::text('_AM_MODULEINSTALLER_ERR_NONE_SELECTED', 'Select at least one module.'); + // : the counter is a neutral numeral at a direction boundary, so in an + // RTL admin '%d selected' rendered as "selected 1". The element is built here, + // where it is a constant, rather than passed into the splitter. + [$before, $after] = self::selectedCountParts((string) $countTpl); + $label = $before + . '0' + . $after; $actions = '' !== $actionButtonsHtml ? '' . $actionButtonsHtml . '' @@ -368,7 +455,7 @@ public static function renderStickyBar(string $actionButtonsHtml = ''): string return '
' . '
' . '' . $label . '' - . '
' . $actions @@ -390,13 +477,14 @@ public static function renderYesNoRadios(string $dirname, int $value): string $idYes = $safeId . '_1'; $idNo = $safeId . '_2'; $onclick = "selectModule('" . \htmlspecialchars($dirname, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . "', this)"; - $yesLabel = \defined('_YES') ? \_YES : 'Yes'; - $noLabel = \defined('_NO') ? \_NO : 'No'; + // Core constants from language//global.php, so a different domain. + $yesLabel = Lang::core('_YES', 'Yes'); + $noLabel = Lang::core('_NO', 'No'); $yesChk = (1 === $value) ? ' checked' : ''; $noChk = (1 === $value) ? '' : ' checked'; // Single-line nowrap unit — first row cannot stretch Yes/No across the cell - return '' + return '' . '
fragments, + // not information. Dropping them here is why the renderer needs no + // "collapse runs of
" pass. + if (! $event->isBlank()) { + $events[] = $event; + } + } + + return $events; + } + + /** + * Core indents nested steps with '  ' per level. Pull that out of the + * text into a depth, so the renderer can indent with CSS (or with anything else) + * instead of the transcript carrying presentation inside its own text. + * + * Indent is measured in NBSP ONLY. Two rules follow from that, both deliberate: + * + * - ASCII spaces and tabs are CONTENT, not structure. Core does not use them to + * nest, and a message that begins with one leading space means it, so leading + * whitespace stays in the text where the renderer will escape it. + * - an odd NBSP is not lost: only whole pairs become depth, and the remainder is + * put back at the head of the text. + * + * The match is on CHARACTERS, with /u. Doing this on bytes is a real bug and this + * method had it: ltrim($text, " \t\u{00A0}") builds a byte set {0x20, 0x09, 0xC2, + * 0xA0}, so a line starting with «, », ·, ¡, ¿, °, ± — anything in U+0080–U+00BF, + * all of which begin with the byte 0xC2 — lost its lead byte, leaving an orphan + * continuation byte that htmlspecialchars(ENT_SUBSTITUTE) turned into U+FFFD and + * that made json_encode() of the plain text return false. Translated packs use + * exactly those characters as bullets, so making the report translatable is what + * would have exposed it. + * + * @param list $fragments + */ + private function withIndentExtracted(LogSeverity $severity, array $fragments): LogEvent + { + if ([] === $fragments) { + return new LogEvent($severity, [], 0); + } + + $first = $fragments[0]; + if (1 !== \preg_match('/^(?:\x{00A0})+/u', $first->text, $lead)) { + return new LogEvent($severity, $fragments, 0); + } + + $units = (int) \mb_strlen($lead[0], 'UTF-8'); + $depth = \intdiv($units, 2); + $remainder = \str_repeat(self::NBSP, $units % 2); + $rest = $remainder . \substr($first->text, \strlen($lead[0])); + + $fragments[0] = new LogFragment($rest, $first->emphasised); + if ('' === $rest) { + \array_shift($fragments); + } + + return new LogEvent($severity, \array_values($fragments), $depth); + } +} diff --git a/class/Report/LogEvent.php b/class/Report/LogEvent.php new file mode 100644 index 0000000..65f1663 --- /dev/null +++ b/class/Report/LogEvent.php @@ -0,0 +1,51 @@ + $fragments */ + public function __construct( + public LogSeverity $severity, + public array $fragments, + public int $depth = 0, + ) { + } + + /** Convenience for the common "one unemphasised line" case. */ + public static function text(string $text, LogSeverity $severity = LogSeverity::Info, int $depth = 0): self + { + return new self($severity, [new LogFragment($text)], $depth); + } + + /** The line as plain text — for logs, CLI output, JSON, tests. */ + public function plainText(): string + { + return \implode('', \array_map(static fn (LogFragment $f): string => $f->text, $this->fragments)); + } + + public function isBlank(): bool + { + return '' === \trim($this->plainText()); + } +} diff --git a/class/Report/LogEventHtmlRenderer.php b/class/Report/LogEventHtmlRenderer.php new file mode 100644 index 0000000..5fe696d --- /dev/null +++ b/class/Report/LogEventHtmlRenderer.php @@ -0,0 +1,69 @@ + $events */ + public function render(array $events): string + { + $lines = []; + foreach ($events as $event) { + $lines[] = $this->renderEvent($event); + } + + return \implode("
\n", $lines); + } + + private function renderEvent(LogEvent $event): string + { + $inner = ''; + foreach ($event->fragments as $fragment) { + $text = $this->escape($fragment->text); + $inner .= $fragment->emphasised ? '' . $text . '' : $text; + } + + $class = $event->severity->cssClass(); + if ('' !== $class) { + $inner = '' . $inner . ''; + } + + // Indent stays outside the severity span, exactly where core put it, and is + // emitted as NBSP so it survives HTML whitespace collapsing. A depth-based + // CSS indent (padding-inline-start) is now possible instead — the value + // object carries the depth, so that is a renderer decision, not a data one. + return $this->escape(\str_repeat(self::NBSP . self::NBSP, \max(0, $event->depth))) . $inner; + } + + private function escape(string $text): string + { + return \htmlspecialchars($text, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'); + } +} diff --git a/class/Report/LogFragment.php b/class/Report/LogFragment.php new file mode 100644 index 0000000..28dd36f --- /dev/null +++ b/class/Report/LogFragment.php @@ -0,0 +1,32 @@ + around a table name, a template filename or a module id. + */ +final readonly class LogFragment +{ + public function __construct( + public string $text, + public bool $emphasised = false, + ) { + } +} diff --git a/class/Report/LogSeverity.php b/class/Report/LogSeverity.php new file mode 100644 index 0000000..1f26110 --- /dev/null +++ b/class/Report/LogSeverity.php @@ -0,0 +1,41 @@ + '', + self::Success => 'installer-log-success', + self::Warning => 'installer-log-warning', + self::Error => 'installer-log-error', + }; + } +} diff --git a/class/Report/ModuleOperationResult.php b/class/Report/ModuleOperationResult.php new file mode 100644 index 0000000..6c3783f --- /dev/null +++ b/class/Report/ModuleOperationResult.php @@ -0,0 +1,191 @@ + $events */ + private function __construct( + public string $dirname, + public string $action, + public Outcome $outcome, + public array $events, + public ?string $reason = null, + ) { + } + + /** @param list $events */ + public static function ok(string $dirname, string $action, array $events = []): self + { + return new self($dirname, $action, Outcome::Ok, $events); + } + + /** A skip is a decision, not a failure: it always has a stated reason. */ + public static function skipped(string $dirname, string $action, string $reason): self + { + return new self($dirname, $action, Outcome::Skipped, [LogEvent::text($reason, LogSeverity::Warning)], $reason); + } + + /** + * A failure whose reason is NOT yet part of the transcript. + * + * The reason is prepended as an error event, so a consumer that only walks the + * transcript still sees the verdict. Use this when reason and transcript come from + * different places — a verdict this module established, plus whatever core said + * while getting there. + * + * @param list $events + */ + public static function failed(string $dirname, string $action, string $reason, array $events = []): self + { + return new self( + $dirname, + $action, + Outcome::Failed, + [LogEvent::text($reason, LogSeverity::Error), ...$events], + $reason + ); + } + + /** + * A failure whose transcript ALREADY states its reason. + * + * Needed because the legacy string does not separate verdict from transcript: + * ModuleActionService composes a failure message as ": ", so a projection of that string has only one body of text to work with. + * Handing it to failed() prepended a copy of the whole transcript to itself, and + * the plain-text view printed every line twice — visible in a CLI run, a log file + * and any JSON consumer, though not in messageHtml(), which never looks at events. + * + * NOTE THE SIGNATURE: no $reason. It is DERIVED from the first event, not supplied + * beside it. An earlier version took both and documented that the events "must + * already contain $reason" — a docblock is not an invariant, and a constructor + * that accepts two independent sources for one fact will eventually be handed two + * different facts. Deriving it makes the divergence unrepresentable. + * + * The two constructors exist rather than a boolean flag because the caller always + * knows which situation it is in, and the call site should say so. + * + * @param list $events the transcript, whose first line states the reason + */ + public static function failedWithTranscript(string $dirname, string $action, array $events): self + { + return new self( + $dirname, + $action, + Outcome::Failed, + $events, + [] === $events ? '' : $events[0]->plainText() + ); + } + + public function isOk(): bool { return Outcome::Ok === $this->outcome; } + public function isSkipped(): bool { return Outcome::Skipped === $this->outcome; } + public function isFailed(): bool { return Outcome::Failed === $this->outcome; } + + /** + * True when any transcript line reported an error, even on an Ok outcome. + * + * DELIBERATELY INDEPENDENT OF $outcome, and it is worth saying why, because a + * reviewer reasonably proposed collapsing the two: a failed result whose transcript + * carries no red line answers false here, which reads like a contradiction. + * + * It is not one. The two answer different questions: + * + * - $outcome is THIS MODULE'S verdict, established by re-reading module state + * after the action (see ModuleActionService::verifiedResult()); + * - hasErrorEvents() is what CORE'S OWN LOG said while getting there. + * + * They disagree in both directions, and both disagreements are real and worth + * seeing. Core routinely returns a log containing red lines for an operation that + * nonetheless completed — a template that could not be added while the module + * installed fine — and, more importantly, core's string is exactly the thing that + * cannot be trusted for the verdict, which is why the verdict is re-derived at all. + * Promoting the reason to an Error event to make the two agree would delete the + * only signal that answers "core said this went fine; did its own log agree?". + * + * A renderer should therefore colour by {@see self::severity()}, which combines + * them, and never by scanning events alone. + */ + public function hasErrorEvents(): bool + { + foreach ($this->events as $event) { + if (LogSeverity::Error === $event->severity) { + return true; + } + } + + return false; + } + + /** + * The one severity that describes this result, for a consumer that wants one. + * + * The verdict wins where it is decisive — a failure is an error however calm its + * transcript reads — and the transcript speaks where the verdict is not: an Ok + * result whose log contains red lines is a Warning, not a clean Success, and that + * is the case a transcript-only renderer would otherwise paint green. + */ + public function severity(): LogSeverity + { + return match (true) { + $this->isFailed() => LogSeverity::Error, + $this->isSkipped() => LogSeverity::Warning, + $this->hasErrorEvents() => LogSeverity::Warning, + default => LogSeverity::Success, + }; + } + + /** The whole transcript as plain text — CLI, log file, mail, test assertion. */ + public function plainText(): string + { + return \implode("\n", \array_map(static fn (LogEvent $e): string => $e->plainText(), $this->events)); + } + + /** @return array{dirname:string,action:string,outcome:string,severity:string,reason:?string,events:list} */ + public function toArray(): array + { + return [ + 'dirname' => $this->dirname, + 'action' => $this->action, + 'outcome' => $this->outcome->value, + // The combined verdict, so a JSON consumer does not have to re-implement + // the outcome/transcript precedence rule to colour one row. + 'severity' => $this->severity()->value, + 'reason' => $this->reason, + 'events' => \array_map( + static fn (LogEvent $e): array => [ + 'severity' => $e->severity->value, + 'depth' => $e->depth, + 'text' => $e->plainText(), + ], + $this->events + ), + ]; + } +} diff --git a/class/Report/Outcome.php b/class/Report/Outcome.php new file mode 100644 index 0000000..561f2f9 --- /dev/null +++ b/class/Report/Outcome.php @@ -0,0 +1,39 @@ + 'success', + self::Skipped => 'warning', + self::Failed => 'error', + }; + } +} diff --git a/class/Set/ModuleSetApplier.php b/class/Set/ModuleSetApplier.php index 568ad62..b5ff0ff 100644 --- a/class/Set/ModuleSetApplier.php +++ b/class/Set/ModuleSetApplier.php @@ -14,6 +14,7 @@ * @author Michael Beck (mamba) */ +use XoopsModules\Moduleinstaller\Lang; use XoopsModules\Moduleinstaller\ModuleActionResult; use XoopsModules\Moduleinstaller\ModuleActionService; use XoopsModules\Moduleinstaller\ModuleCatalog; @@ -94,11 +95,20 @@ public function execute(ModuleSet $set, string $action, bool $snapshotBeforeFocu try { $snapshot = $this->createSnapshot($set); $snapshotId = $snapshot->getId(); - $notices[] = 'Saved snapshot: ' . $snapshot->getName() . ' (' . $snapshotId . ')'; + $notices[] = Lang::format( + '_AM_MODULEINSTALLER_SET_SNAPSHOT_NOTICE', + 'Saved snapshot: %1$s (%2$s)', + $snapshot->getName(), + $snapshotId + ); } catch (\Throwable $e) { // The snapshot is the ONLY way to undo a Focus. If it cannot be saved, // abort before deactivating anything so the operation stays recoverable. - $notices[] = 'Aborted: could not save recovery snapshot before Focus (' . $e->getMessage() . ')'; + $notices[] = Lang::format( + '_AM_MODULEINSTALLER_SET_SNAPSHOT_ABORTED', + 'Aborted: could not save recovery snapshot before Focus (%s)', + $e->getMessage() + ); return [ 'results' => [], @@ -124,7 +134,7 @@ public function execute(ModuleSet $set, string $action, bool $snapshotBeforeFocu $results[] = new ModuleActionResult( $dirname, ModuleActionResult::STATUS_SKIP, - (string) ($row['notice'] ?? 'Missing on disk'), + (string) ($row['notice'] ?? Lang::text('_AM_MODULEINSTALLER_BADGE_MISSING', 'Missing on disk')), $action ); } @@ -145,13 +155,23 @@ public function execute(ModuleSet $set, string $action, bool $snapshotBeforeFocu public function createSnapshot(ModuleSet $beforeFocusSet): ModuleSet { $active = $this->catalog->listInstalled(true); - $name = 'Snapshot before Focus: ' . $beforeFocusSet->getName(); + // Stored with the snapshot and displayed later, possibly under a different + // locale: localised at creation time on purpose, rather than storing a key. + $name = Lang::format( + '_AM_MODULEINSTALLER_SET_SNAPSHOT_NAME', + 'Snapshot before Focus: %s', + $beforeFocusSet->getName() + ); $id = $this->repository->uniqueId('snapshot-' . \gmdate('Ymd-His')); $set = new ModuleSet( $id, $name, - 'Auto-created before applying Focus on set "' . $beforeFocusSet->getId() . '"', + Lang::format( + '_AM_MODULEINSTALLER_SET_SNAPSHOT_DESC', + 'Auto-created before applying Focus on set "%s"', + $beforeFocusSet->getId() + ), $active, ); $this->repository->save($set); @@ -207,7 +227,7 @@ private function planMemberAction(ModuleSet $set, string $action): array $plan[] = [ 'dirname' => $dirname, 'action' => $action, - 'reason' => 'Member of set', + 'reason' => Lang::text('_AM_MODULEINSTALLER_SET_REASON_MEMBER', 'Member of set'), ]; } } @@ -235,7 +255,7 @@ private function planFocus(ModuleSet $set): array $plan[] = [ 'dirname' => $row['dirname'], 'action' => self::ACTION_ACTIVATE, - 'reason' => 'Focus: activate set member', + 'reason' => Lang::text('_AM_MODULEINSTALLER_SET_REASON_FOCUS_ON', 'Focus: activate set member'), ]; } } @@ -251,7 +271,7 @@ private function planFocus(ModuleSet $set): array $plan[] = [ 'dirname' => $dirname, 'action' => self::ACTION_DEACTIVATE, - 'reason' => 'Focus: deactivate non-member', + 'reason' => Lang::text('_AM_MODULEINSTALLER_SET_REASON_FOCUS_OFF', 'Focus: deactivate non-member'), ]; } diff --git a/class/Set/ModuleSetResolver.php b/class/Set/ModuleSetResolver.php index e67985a..60d8dd6 100644 --- a/class/Set/ModuleSetResolver.php +++ b/class/Set/ModuleSetResolver.php @@ -14,6 +14,7 @@ * @author Michael Beck (mamba) */ +use XoopsModules\Moduleinstaller\Lang; use XoopsModules\Moduleinstaller\ModuleCatalog; /** @@ -75,18 +76,18 @@ public function resolveOne(string $dirname): array // Installed in the DB but its folder is gone: a distinct state so bulk // planning never tries to activate/install a module with no files. $state = self::STATE_ORPHANED; - $notice = 'Installed in database but folder missing on disk'; + $notice = Lang::text('_AM_MODULEINSTALLER_RES_ORPHANED', 'Installed in database but folder missing on disk'); } elseif (! $onDisk && ! $installed) { $state = self::STATE_MISSING; - $notice = 'Not found on disk (removed module)'; + $notice = Lang::text('_AM_MODULEINSTALLER_RES_REMOVED', 'Not found on disk (removed module)'); } elseif ($protected) { $state = self::STATE_PROTECTED; - $notice = 'Protected module'; + $notice = Lang::text('_AM_MODULEINSTALLER_BADGE_PROTECTED', 'Protected module'); } elseif (! $installed) { // Reached only when on disk (the installed && !on_disk case is ORPHANED above, // and !on_disk && !installed is MISSING), so the folder is always present here. $state = self::STATE_NOT_INSTALLED; - $notice = 'Present on disk but not installed'; + $notice = Lang::text('_AM_MODULEINSTALLER_RES_ON_DISK_NOT_INSTALLED', 'Present on disk but not installed'); } elseif ($active) { $state = self::STATE_ACTIVE; $notice = null; diff --git a/docs/TUTORIAL.md b/docs/TUTORIAL.md index 7b34857..673a528 100644 --- a/docs/TUTORIAL.md +++ b/docs/TUTORIAL.md @@ -1,4 +1,4 @@ -# ModuleInstaller Tutorial (1.6.0) +# ModuleInstaller Tutorial (1.7.0) ## What this module does @@ -160,5 +160,5 @@ After Focus, a success notice may include a link to open the **auto-snapshot** s ## See also - [README.md](../README.md) — overview, architecture, development -- [CHANGELOG.md](../CHANGELOG.md) / [changelog.txt](changelog.txt) — release history +- [CHANGELOG.md](../CHANGELOG.md) / [changelog.txt](changelog.txt) — release history · [lang_diff.txt](lang_diff.txt) — language changes - Tutorial (GitBook): https://xoops.gitbook.io/xoops-moduleinstaller/ diff --git a/docs/changelog.txt b/docs/changelog.txt index e497541..8cbeabe 100644 --- a/docs/changelog.txt +++ b/docs/changelog.txt @@ -1,6 +1,53 @@ ModuleInstaller CHANGELOG ========================= -Compared to previous public baseline 1.5.1-Final (L:\GitHub\00mambax7\moduleinstaller / 2022-12-15). + +-------------------------------------------------------------------------------- +1.7.0 Alpha 1 [2026-07-30] +Dev: XOOPS 2.7.0+, PHP 8.2+, mtools 1.2.0+ +-------------------------------------------------------------------------------- + +Highlights +- Rebuilt install-action reporting as structured data that can be rendered safely + to HTML or consumed as plain text, CLI output, logs, or JSON. +- Hardened admin reports against unsafe module-supplied markup, paths, malformed + manifests, invalid UTF-8, and incomplete or malformed language packs. +- Improved translated and RTL admin output, accessibility, and responsive styling + without breaking existing ModuleActionService callers. + +Security / correctness +- The install-log renderer now parses untrusted core HTML into typed events, + escapes every value exactly once, emits only fixed module-owned markup, and + discards executable or raw-text element bodies. +- Module logo URLs now use one validated, platform-aware path resolver; unsafe, + remote, missing, and traversal paths are rejected. +- Manifest loading failures no longer replace a completed operation report with a + blank page. +- Fixed invalid UTF-8 caused by byte-based NBSP trimming and duplicate failure + transcripts in structured output. + +Structured reports and compatibility +- Added the class/Report/ package with typed log fragments, events, severities, + outcomes, operation results, a legacy adapter, and the HTML renderer. +- ModuleActionResult now exposes events(), messageText(), and + toOperationResult(); ModuleActionService adds operationResult() and + operationResults(). +- ModuleActionResult is deprecated in favor of Report\ModuleOperationResult, while + existing runOne()/runMany() signatures remain supported through XOOPS 2.8. +- Structured results expose a combined severity for consumers while preserving + the distinction between the operation outcome and errors reported by core. + +Internationalisation and admin UI +- Added 27 translatable admin-report strings and Lang helpers with safe English + fallbacks for missing, blank, non-scalar, or malformed translations. +- Added bidirectional isolation for counters, versions, statuses, folders, and + plan summaries in RTL admin interfaces. +- Improved long-report scrolling and screen-reader ordering; converted module CSS + to logical properties and removed module-owned inline style attributes. + +Tests +- Expanded the suite to 169 tests covering hostile log input, structured-result + contracts, language fallbacks, logo-path validation, UTF-8, and real timeout/ + crash handling on POSIX and Windows. -------------------------------------------------------------------------------- 1.6.0 Stable [2026-07-23] @@ -66,7 +113,7 @@ Quality / tooling (new vs 1.5.1) - phpstan.neon and phpstan analysis notes. - Expanded language strings (admin sets UI, apply-set hints). -UX polish (still 1.6.0) +UX polish (introduced in 1.6.0) - Apply Set form: clearer vertical spacing between intro, Action, Preview plan, Set member status, confirm checkbox, and Run/Cancel. - Module Set edit: selected members highlighted with light green (#E6EFC2), @@ -75,7 +122,7 @@ UX polish (still 1.6.0) selection (same as radio / checkbox). - docs/TUTORIAL.md end-user walkthrough. -Phase 1–2 UX (still 1.6.0, 2026-07-23) +Phase 1–2 UX (introduced in 1.6.0, 2026-07-23) - Bulk + set-edit filter box; keyboard / and A; sticky “N selected” bar; empty Yes selection blocked on submit. - Status badges; Apply plan summary counts; Focus/Uninstall confirm copy split; @@ -87,7 +134,7 @@ Phase 1–2 UX (still 1.6.0, 2026-07-23) - ModuleCatalog::candidatesFor / needsUpdate helpers. - docs/TODO.md updated; docs/MTOOLS-INVESTIGATION.md (Phase 3 prerequisite). -Phase 3 structure & polish (still 1.6.0, 2026-07-23) +Phase 3 structure & polish (introduced in 1.6.0, 2026-07-23) - Bulk tabs: AdminBulkPage::serve() only (no install_tpl / InstallWizard dual chrome). - Thin admin wrappers; CSRF on bulk POST; renderReport in presentation layer. - Removed extras/, wizard files (InstallWizard, common.inc, install_tpl, page.php, diff --git a/docs/lang_diff.txt b/docs/lang_diff.txt index f4cb3dc..b774a7a 100644 --- a/docs/lang_diff.txt +++ b/docs/lang_diff.txt @@ -8,3 +8,47 @@ Below are language differences from a version to next version. /language/english/admin.php added define('_AM_MODULEINSTALLER_SELECT_ALL' , "Select All"); added define('_AM_MODULEINSTALLER_SELECT_NONE' , "Un-Select All"); + +1.7.0 +------------------- +/language/english/admin.php +added define('_AM_MODULEINSTALLER_RES_EMPTY_DIRNAME' , 'Empty dirname'); +added define('_AM_MODULEINSTALLER_RES_PROTECTED' , 'Protected module "%1$s": action "%2$s" is not allowed.'); +added define('_AM_MODULEINSTALLER_RES_UNKNOWN_ACTION' , 'Unknown action: %s'); +added define('_AM_MODULEINSTALLER_RES_EXCEPTION' , 'Exception: %s'); +added define('_AM_MODULEINSTALLER_RES_NOT_ON_DISK' , 'Not found on disk'); +added define('_AM_MODULEINSTALLER_RES_ALREADY_INSTALLED' , 'Already installed'); +added define('_AM_MODULEINSTALLER_RES_NOT_INSTALLED' , 'Not installed'); +added define('_AM_MODULEINSTALLER_RES_NOT_INSTALLED_ACTIVATE' , 'Not installed (cannot activate)'); +added define('_AM_MODULEINSTALLER_RES_NOT_INSTALLED_DEACTIVATE' , 'Not installed (cannot deactivate)'); +added define('_AM_MODULEINSTALLER_RES_ALREADY_ACTIVE' , 'Already active'); +added define('_AM_MODULEINSTALLER_RES_ALREADY_INACTIVE' , 'Already inactive'); +added define('_AM_MODULEINSTALLER_RES_INVALID_MID' , 'Invalid module id'); +added define('_AM_MODULEINSTALLER_RES_UNVERIFIED' , 'Result could not be verified (module state unreadable): %s'); +added define('_AM_MODULEINSTALLER_RES_FAIL_INSTALL' , 'Install did not complete'); +added define('_AM_MODULEINSTALLER_RES_FAIL_UNINSTALL' , 'Uninstall did not complete'); +added define('_AM_MODULEINSTALLER_RES_FAIL_ACTIVATE' , 'Activation did not take effect'); +added define('_AM_MODULEINSTALLER_RES_FAIL_DEACTIVATE' , 'Deactivation did not take effect (start-page or protected module?)'); +added define('_AM_MODULEINSTALLER_RES_FAIL_UPDATE' , 'Update did not complete'); +added define('_AM_MODULEINSTALLER_RES_ORPHANED' , 'Installed in database but folder missing on disk'); +added define('_AM_MODULEINSTALLER_RES_REMOVED' , 'Not found on disk (removed module)'); +added define('_AM_MODULEINSTALLER_RES_ON_DISK_NOT_INSTALLED' , 'Present on disk but not installed'); +added define('_AM_MODULEINSTALLER_TOGGLE_SELECTION' , 'Toggle selection'); +added define('_AM_MODULEINSTALLER_FOLDER_LABEL' , 'folder: /%s'); +added define('_AM_MODULEINSTALLER_SET_SNAPSHOT_NOTICE' , 'Saved snapshot: %1$s (%2$s)'); +added define('_AM_MODULEINSTALLER_SET_SNAPSHOT_NAME' , 'Snapshot before Focus: %s'); +added define('_AM_MODULEINSTALLER_SET_REASON_MEMBER' , 'Member of set'); +added define('_AM_MODULEINSTALLER_SET_REASON_FOCUS_ON' , 'Focus: activate set member'); +added define('_AM_MODULEINSTALLER_SET_REASON_FOCUS_OFF' , 'Focus: deactivate non-member'); +added define('_AM_MODULEINSTALLER_SET_SNAPSHOT_ABORTED' , 'Aborted: could not save recovery snapshot before Focus (%s)'); +added define('_AM_MODULEINSTALLER_SET_SNAPSHOT_DESC' , 'Auto-created before applying Focus on set "%s"'); + +Notes +- The RES_* block makes the bulk result report translatable. Those strings were + English literals inside class/ModuleActionService.php until now, so the report + was untranslatable regardless of the language pack installed. +- Every lookup goes through class/Lang.php, which carries the English text as a + fallback: a language pack that omits one of these degrades to English instead of + fataling (an undefined constant is a fatal Error on PHP 8, not a notice). +- Translators: %1$s / %2$s ordering may be reordered freely; Lang::format() passes + the arguments positionally. diff --git a/docs/readme.txt b/docs/readme.txt index 1ccdd73..3a9e567 100644 --- a/docs/readme.txt +++ b/docs/readme.txt @@ -6,10 +6,10 @@ and deactivate modules in bulk, and to manage Module Sets for focused testing. Version ------- -1.6.0 Stable (2026-07-23) +1.7.0 Alpha 1 (2026-07-30) Previous public baseline: 1.5.1 Final (2022-12-15) -See CHANGELOG.md and docs/changelog.txt for the full 1.5.1 → 1.6.0 delta. +See CHANGELOG.md and docs/changelog.txt for the full history, and docs/lang_diff.txt for language changes. Requirements diff --git a/language/english/admin.php b/language/english/admin.php index a68782c..cd55beb 100644 --- a/language/english/admin.php +++ b/language/english/admin.php @@ -118,3 +118,44 @@ define('_AM_MODULEINSTALLER_BADGE_NOT_INSTALLED', 'Not installed'); define('_AM_MODULEINSTALLER_BADGE_MISSING', 'Missing on disk'); define('_AM_MODULEINSTALLER_BADGE_PROTECTED', 'Protected'); + +// Bulk result report — one line per module in the report produced by +// ModuleActionService. These were English literals inside the service until now, +// which made the report untranslatable regardless of the language pack installed. +define('_AM_MODULEINSTALLER_RES_EMPTY_DIRNAME', 'Empty dirname'); +define('_AM_MODULEINSTALLER_RES_PROTECTED', 'Protected module "%1$s": action "%2$s" is not allowed.'); +define('_AM_MODULEINSTALLER_RES_UNKNOWN_ACTION', 'Unknown action: %s'); +define('_AM_MODULEINSTALLER_RES_EXCEPTION', 'Exception: %s'); +define('_AM_MODULEINSTALLER_RES_NOT_ON_DISK', 'Not found on disk'); +define('_AM_MODULEINSTALLER_RES_ALREADY_INSTALLED', 'Already installed'); +define('_AM_MODULEINSTALLER_RES_NOT_INSTALLED', 'Not installed'); +define('_AM_MODULEINSTALLER_RES_NOT_INSTALLED_ACTIVATE', 'Not installed (cannot activate)'); +define('_AM_MODULEINSTALLER_RES_NOT_INSTALLED_DEACTIVATE', 'Not installed (cannot deactivate)'); +define('_AM_MODULEINSTALLER_RES_ALREADY_ACTIVE', 'Already active'); +define('_AM_MODULEINSTALLER_RES_ALREADY_INACTIVE', 'Already inactive'); +define('_AM_MODULEINSTALLER_RES_INVALID_MID', 'Invalid module id'); +define('_AM_MODULEINSTALLER_RES_UNVERIFIED', 'Result could not be verified (module state unreadable): %s'); +define('_AM_MODULEINSTALLER_RES_FAIL_INSTALL', 'Install did not complete'); +define('_AM_MODULEINSTALLER_RES_FAIL_UNINSTALL', 'Uninstall did not complete'); +define('_AM_MODULEINSTALLER_RES_FAIL_ACTIVATE', 'Activation did not take effect'); +define('_AM_MODULEINSTALLER_RES_FAIL_DEACTIVATE', 'Deactivation did not take effect (start-page or protected module?)'); +define('_AM_MODULEINSTALLER_RES_FAIL_UPDATE', 'Update did not complete'); + +// Set-resolver notices — same report, same reason they need constants. +define('_AM_MODULEINSTALLER_RES_ORPHANED', 'Installed in database but folder missing on disk'); +define('_AM_MODULEINSTALLER_RES_REMOVED', 'Not found on disk (removed module)'); +define('_AM_MODULEINSTALLER_RES_ON_DISK_NOT_INSTALLED', 'Present on disk but not installed'); + +// Module list chrome + module-set notices (round 4: the last English literals +// outside the language files). +define('_AM_MODULEINSTALLER_TOGGLE_SELECTION', 'Toggle selection'); +define('_AM_MODULEINSTALLER_FOLDER_LABEL', 'folder: /%s'); +define('_AM_MODULEINSTALLER_SET_SNAPSHOT_NOTICE', 'Saved snapshot: %1$s (%2$s)'); +define('_AM_MODULEINSTALLER_SET_SNAPSHOT_NAME', 'Snapshot before Focus: %s'); +define('_AM_MODULEINSTALLER_SET_REASON_MEMBER', 'Member of set'); +define('_AM_MODULEINSTALLER_SET_REASON_FOCUS_ON', 'Focus: activate set member'); +define('_AM_MODULEINSTALLER_SET_REASON_FOCUS_OFF', 'Focus: deactivate non-member'); + +// Set-apply recovery snapshot (round 5: the last hardcoded strings on this path). +define('_AM_MODULEINSTALLER_SET_SNAPSHOT_ABORTED', 'Aborted: could not save recovery snapshot before Focus (%s)'); +define('_AM_MODULEINSTALLER_SET_SNAPSHOT_DESC', 'Auto-created before applying Focus on set "%s"'); diff --git a/xoops_version.php b/xoops_version.php index 1ec9be0..41e6a5c 100644 --- a/xoops_version.php +++ b/xoops_version.php @@ -17,9 +17,9 @@ $moduleDirName = basename(__DIR__); $modversion = []; -$modversion['version'] = '1.6.0-Stable'; -$modversion['module_status'] = ''; -$modversion['release_date'] = '2026/07/23'; +$modversion['version'] = '1.7.0-Alpha1'; +$modversion['module_status'] = 'Alpha 1'; +$modversion['release_date'] = '2026/07/30'; $modversion['name'] = _MI_MODULEINSTALLER_NAME; $modversion['description'] = _MI_MODULEINSTALLER_DESC; $modversion['author'] = 'Michael Beck'; From 199429e2edabdd0533cf41bdfe4a24c034efc9a6 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 21:16:25 -0400 Subject: [PATCH 02/12] style(installer): apply php-cs-fixer to the report classes Alignment padding on enum cases and match arms, and single-line method bodies, are both rejected by the repository fixer configuration. The QA gate runs cs:check first and exits non-zero, so every downstream step (analyse, rector, test) was being skipped on all four PHP versions. No logic change in any of these files. --- class/ModuleActionService.php | 1 - class/Report/LogSeverity.php | 8 +++--- class/Report/ModuleOperationResult.php | 39 +++++++++++++++++--------- class/Report/Outcome.php | 8 +++--- 4 files changed, 33 insertions(+), 23 deletions(-) diff --git a/class/ModuleActionService.php b/class/ModuleActionService.php index e9c143b..47ff88a 100644 --- a/class/ModuleActionService.php +++ b/class/ModuleActionService.php @@ -40,7 +40,6 @@ public function getCatalog(): ModuleCatalog return $this->catalog; } - /** * Ensure system modulesadmin helpers are loaded. */ diff --git a/class/Report/LogSeverity.php b/class/Report/LogSeverity.php index 1f26110..7081151 100644 --- a/class/Report/LogSeverity.php +++ b/class/Report/LogSeverity.php @@ -23,19 +23,19 @@ */ enum LogSeverity: string { - case Info = 'info'; + case Info = 'info'; case Success = 'success'; case Warning = 'warning'; - case Error = 'error'; + case Error = 'error'; /** CSS class the renderer emits. Empty for Info, which needs no marking. */ public function cssClass(): string { return match ($this) { - self::Info => '', + self::Info => '', self::Success => 'installer-log-success', self::Warning => 'installer-log-warning', - self::Error => 'installer-log-error', + self::Error => 'installer-log-error', }; } } diff --git a/class/Report/ModuleOperationResult.php b/class/Report/ModuleOperationResult.php index 6c3783f..1ed45e6 100644 --- a/class/Report/ModuleOperationResult.php +++ b/class/Report/ModuleOperationResult.php @@ -104,9 +104,20 @@ public static function failedWithTranscript(string $dirname, string $action, arr ); } - public function isOk(): bool { return Outcome::Ok === $this->outcome; } - public function isSkipped(): bool { return Outcome::Skipped === $this->outcome; } - public function isFailed(): bool { return Outcome::Failed === $this->outcome; } + public function isOk(): bool + { + return Outcome::Ok === $this->outcome; + } + + public function isSkipped(): bool + { + return Outcome::Skipped === $this->outcome; + } + + public function isFailed(): bool + { + return Outcome::Failed === $this->outcome; + } /** * True when any transcript line reported an error, even on an Ok outcome. @@ -154,10 +165,10 @@ public function hasErrorEvents(): bool public function severity(): LogSeverity { return match (true) { - $this->isFailed() => LogSeverity::Error, - $this->isSkipped() => LogSeverity::Warning, - $this->hasErrorEvents() => LogSeverity::Warning, - default => LogSeverity::Success, + $this->isFailed() => LogSeverity::Error, + $this->isSkipped() => LogSeverity::Warning, + $this->hasErrorEvents() => LogSeverity::Warning, + default => LogSeverity::Success, }; } @@ -171,18 +182,18 @@ public function plainText(): string public function toArray(): array { return [ - 'dirname' => $this->dirname, - 'action' => $this->action, - 'outcome' => $this->outcome->value, + 'dirname' => $this->dirname, + 'action' => $this->action, + 'outcome' => $this->outcome->value, // The combined verdict, so a JSON consumer does not have to re-implement // the outcome/transcript precedence rule to colour one row. 'severity' => $this->severity()->value, - 'reason' => $this->reason, - 'events' => \array_map( + 'reason' => $this->reason, + 'events' => \array_map( static fn (LogEvent $e): array => [ 'severity' => $e->severity->value, - 'depth' => $e->depth, - 'text' => $e->plainText(), + 'depth' => $e->depth, + 'text' => $e->plainText(), ], $this->events ), diff --git a/class/Report/Outcome.php b/class/Report/Outcome.php index 561f2f9..7813f7a 100644 --- a/class/Report/Outcome.php +++ b/class/Report/Outcome.php @@ -23,17 +23,17 @@ */ enum Outcome: string { - case Ok = 'ok'; + case Ok = 'ok'; case Skipped = 'skip'; - case Failed = 'fail'; + case Failed = 'fail'; /** CSS class for the report row — the match that currently lives in AdminBulkPage. */ public function cssClass(): string { return match ($this) { - self::Ok => 'success', + self::Ok => 'success', self::Skipped => 'warning', - self::Failed => 'error', + self::Failed => 'error', }; } } From 3e0d9a21ea88a18d68cdc235f75f719123e6dc19 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 21:16:50 -0400 Subject: [PATCH 03/12] security(installer): escape dirnames for the onclick JS context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The module row built its toggle handler by concatenating an HTML-escaped dirname into a JavaScript string literal. A browser HTML-decodes an attribute value before compiling it as JavaScript, so ENT_QUOTES turning an apostrophe into ' does not help: it decodes back to a bare quote and closes the literal. A folder named foo'); alert(1);// therefore produced a live call to alert() on every admin bulk page. XoopsLists::getDirListAsArray() applies no character filter, so the name reaches the row renderer verbatim, and an apostrophe is legal on both NTFS and ext4. json_encode() now emits the JS literal and the HEX flags keep angle brackets, ampersands and both quote marks out of the attribute entirely. Also in moduleLogoUrl(): the control-character guard tested preg_match() for truth, but preg_match() returns int|false and its false-on-error is falsy, so the guard failed open exactly when it mattered. It now tests against 1, matching the dirname check above it. Containment proves the target sits inside the module but not that it is an image, so an extension allowlist was added — a manifest could otherwise point the at a contained .php and have the browser execute it server-side. The dirname allowlist is documented rather than widened: it is the module's dirname contract, applied identically by ModuleCatalog::existsOnDisk() and ModuleSet::withDirnames(), so a dotted folder is rejected before a row is ever built. Row logos now use alt='' to match renderReport(); the module name is rendered in the adjacent cell, so a described image made a screen reader announce it twice per row. The toggle title is hoisted out of the loop. --- class/AdminBulkPage.php | 104 +++++++++++++++++++++++++++++----------- 1 file changed, 77 insertions(+), 27 deletions(-) diff --git a/class/AdminBulkPage.php b/class/AdminBulkPage.php index 485fbf0..93f891f 100644 --- a/class/AdminBulkPage.php +++ b/class/AdminBulkPage.php @@ -53,7 +53,7 @@ public static function serve(string $action): void if (! self::checkCsrf()) { $pageHasForm = false; $msg = Lang::text('_AM_MODULEINSTALLER_ERR_TOKEN', 'Invalid security token. Please try again.'); - $content = "
" . \htmlspecialchars((string) $msg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
'; + $content = "
" . \htmlspecialchars($msg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
'; } else { $handled = self::handlePost($action, self::successTitleFor($action)); $pageHasForm = $handled['pageHasForm']; @@ -179,7 +179,7 @@ private static function emptyListMessage(): string { $msg = Lang::text('_AM_MODULEINSTALLER_NO_MODULES', 'No modules found.'); - return "
" . \htmlspecialchars((string) $msg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
'; + return "
" . \htmlspecialchars($msg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
'; } private static function successTitleFor(string $action): string @@ -276,19 +276,19 @@ public static function renderModuleTable(array $dirnames, array $preselect = [], // Selected count is shown on the page title (.CPbigTitle) and sticky bar only. $content = '
'; $content .= ' '; + . \htmlspecialchars($filterLbl, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ' '; // type=text (not search) so theme/input styles apply; size gives a visible fallback width $content .= ''; + . 'placeholder="' . \htmlspecialchars($filterPh, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '" ' + . 'autocomplete="off" aria-label="' . \htmlspecialchars($filterLbl, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '">'; if ('' !== $filterHint) { $content .= ' ' - . \htmlspecialchars((string) $filterHint, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ''; + . \htmlspecialchars($filterHint, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ''; } $content .= '
'; $content .= '

' - . \htmlspecialchars((string) $noneMsg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '

'; + . \htmlspecialchars($noneMsg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '

'; // No ul/li wrapper: list markers broke first-row radio layout (black square + stretched Yes/No) $content .= "
" . "
' . _AM_MODULEINSTALLER_SET_MODULE . '' . _AM_MODULEINSTALLER_SET_ACTION . '' . _AM_MODULEINSTALLER_SET_REASON . '
"; - $content .= "" . $spanOpen; + $content .= "
"; + $content .= '' !== $imgSrc + ? "" + : ""; + $content .= "" . $spanOpen; + // The version, the status and the folder path are an LTR run that may sit + // beside an RTL module name. Each is isolated so the parentheses and the + // digits cannot migrate across the boundary. $content .= ' ' . $nameEsc - . ' ' . \htmlspecialchars((string) $info['version'], \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') - . ' ' . \htmlspecialchars((string) $info['module_status'], \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') - . ' (folder: /' . \htmlspecialchars($info['dirname'], \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ')'; + . ' ' . \htmlspecialchars((string) $info['version'], \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '' + . ' ' . \htmlspecialchars((string) $info['module_status'], \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '' + . ' (' . \htmlspecialchars( + Lang::format('_AM_MODULEINSTALLER_FOLDER_LABEL', 'folder: /%s', $info['dirname']), + \ENT_QUOTES | \ENT_SUBSTITUTE, + 'UTF-8' + ) . ')'; $content .= '
' . \htmlspecialchars($info['description'], \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ''; $content .= $spanClose . "
\n" @@ -296,6 +296,13 @@ public static function renderModuleTable(array $dirnames, array $preselect = [], $count = 0; $even = false; + // Row-invariant: the same lookup and the same escape for every row. + $toggleTitle = \htmlspecialchars( + Lang::text('_AM_MODULEINSTALLER_TOGGLE_SELECTION', 'Toggle selection'), + \ENT_QUOTES | \ENT_SUBSTITUTE, + 'UTF-8' + ); + foreach ($dirnames as $file) { $file = \trim((string) $file); if ('' === $file) { @@ -330,7 +337,24 @@ public static function renderModuleTable(array $dirnames, array $preselect = [], $imgSrc = self::moduleLogoUrl($file, $info) ?? ''; $dirnameEsc = \htmlspecialchars($file, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'); $nameEsc = \htmlspecialchars($info['name'], \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'); - $toggleJs = "toggleModuleRow('" . $dirnameEsc . "')"; + + // The dirname lands in two different contexts and needs two different + // escapers. htmlspecialchars() is right for id='…'; it is WRONG inside the + // onclick JS string literal, because the browser HTML-decodes an attribute + // value BEFORE compiling it as JavaScript — so ' arrives at the JS + // parser as a bare quote and closes the literal. Core applies no character + // filter here (XoopsLists::getDirListAsArray() skips only dot-entries and + // 'cvs'), so a directory named foo'); alert(1);// reaches this line + // verbatim. json_encode() emits a complete, already-quoted JS literal, and + // the HEX flags keep <, >, &, ' and " out of the attribute altogether. + $dirnameJs = \json_encode( + $file, + \JSON_HEX_TAG | \JSON_HEX_AMP | \JSON_HEX_APOS | \JSON_HEX_QUOT + ); + // false only for invalid UTF-8. An inert handler beats a malformed one. + $toggleJs = false === $dirnameJs + ? 'void 0' + : 'toggleModuleRow(' . $dirnameJs . ')'; $searchBlob = \htmlspecialchars( \mb_strtolower($info['name'] . ' ' . $file . ' ' . $info['description']), \ENT_QUOTES | \ENT_SUBSTITUTE, @@ -340,15 +364,13 @@ public static function renderModuleTable(array $dirnames, array $preselect = [], $stripe = $even ? 'even' : 'odd'; $rowClass = \trim($stripe . ($value !== 0 ? ' installer-row-selected' : '')); $content .= "\n"; - $toggleTitle = \htmlspecialchars( - Lang::text('_AM_MODULEINSTALLER_TOGGLE_SELECTION', 'Toggle selection'), - \ENT_QUOTES | \ENT_SUBSTITUTE, - 'UTF-8' - ); $content .= " \n"; $content .= "
"; + // alt='' for the same reason renderReport() uses it: the module name is + // rendered in the very next cell, so a described image makes a screen + // reader announce the name twice on every row. The logo is decorative. $content .= '' !== $imgSrc ? "" + . "' alt=''>" : ""; $content .= "" . $spanOpen; @@ -442,7 +464,7 @@ public static function renderStickyBar(string $actionButtonsHtml = ''): string // : the counter is a neutral numeral at a direction boundary, so in an // RTL admin '%d selected' rendered as "selected 1". The element is built here, // where it is a constant, rather than passed into the splitter. - [$before, $after] = self::selectedCountParts((string) $countTpl); + [$before, $after] = self::selectedCountParts($countTpl); $label = $before . '0' . $after; @@ -456,7 +478,7 @@ public static function renderStickyBar(string $actionButtonsHtml = ''): string . '
' . '' . $label . '' . '' - . \htmlspecialchars((string) $noneMsg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') + . \htmlspecialchars($noneMsg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
' . $actions . ''; @@ -488,12 +510,12 @@ public static function renderYesNoRadios(string $dirname, int $value): string . '  ' . ''; } @@ -511,7 +533,7 @@ public static function handlePost(string $action, string $successTitle): array return [ 'pageHasForm' => false, - 'content' => "
" . \htmlspecialchars((string) $msg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
', + 'content' => "
" . \htmlspecialchars($msg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
', 'results' => [], ]; } @@ -535,7 +557,7 @@ public static function renderReport(array $results, string $title = ''): string if ([] === $results) { $msg = Lang::text('_AM_MODULEINSTALLER_NO_SELECTION_REPORT', 'No modules selected.'); - return "
" . \htmlspecialchars((string) $msg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
'; + return "
" . \htmlspecialchars($msg, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '
'; } $html = ''; @@ -608,6 +630,15 @@ public static function renderReport(array $results, string $title = ''): string * not inserted into an HTML attribute directly. Result reports call this * helper to keep the legacy logo while preventing traversal or URL injection. * + * The dirname allowlist is deliberately NOT widened to accept dots. It is not a + * logo rule — it is this module's dirname contract, applied identically by + * ModuleCatalog::existsOnDisk() and ModuleSet::withDirnames(). A module whose + * folder contains a dot is rejected by the catalog before a row is ever built, + * so relaxing it here alone would suppress nothing that currently renders; it + * would only make the one security-relevant path disagree with the other two. + * Widening is a module-wide decision that needs core's agreement and its own + * tests, not a local edit to the helper that happens to be reviewed. + * * @param array{ * dirname:string, * name:string, @@ -627,8 +658,16 @@ public static function moduleLogoUrl(string $dirname, ?array $info): ?string return null; } + // 1 === preg_match(), not a bare truth test: preg_match() returns int|false, + // and its false-on-error is falsy — so a bare call would let a control + // character through on the one occasion the guard actually mattered. Matches + // the form the dirname check above already uses. $image = \str_replace('\\', '/', \trim($info['image'])); - if ('' === $image || \str_starts_with($image, '/') || \preg_match('/[\x00-\x1F\x7F]/', $image)) { + if ( + '' === $image + || \str_starts_with($image, '/') + || 1 === \preg_match('/[\x00-\x1F\x7F]/', $image) + ) { return null; } @@ -660,6 +699,17 @@ public static function moduleLogoUrl(string $dirname, ?array $info): ?string return null; } + // Containment proves the target is inside the module, not that it is an image. + // A manifest is third-party PHP and may name any contained file, and the value + // becomes an the browser will GET — pointing it at a .php in the same + // folder would execute that script server-side on every report render. An + // extension allowlist is the cheap half of that; the module's own files are + // web-reachable regardless, so this bounds what THIS page will ask for. + $extension = \mb_strtolower(\pathinfo($logoPath, \PATHINFO_EXTENSION)); + if (! \in_array($extension, ['png', 'gif', 'jpg', 'jpeg', 'webp', 'svg', 'ico'], true)) { + return null; + } + $encodedImage = \implode('/', \array_map(\rawurlencode(...), $segments)); return \rtrim(\XOOPS_URL, '/') . '/modules/' . \rawurlencode($dirname) . '/' . $encodedImage; @@ -724,15 +774,15 @@ public static function renderSetSelector(): string $html = '
'; $html .= ' '; + . \htmlspecialchars($hint, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . '">' + . \htmlspecialchars($label, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ' '; $html .= ''; $html .= ' ' - . \htmlspecialchars((string) $hint, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ''; + . \htmlspecialchars($hint, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ''; $html .= '
'; // Self-contained handler: works even if xo-installer.js is missing or cached stale. From 397886153bb33b90c93dc99ea388ba1220b67ed6 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 21:17:16 -0400 Subject: [PATCH 04/12] fix(installer): keep the English fallback when a language pack throws MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both load routes include third-party PHP. A language file is ordinary PHP, so it can throw, and a parse error in one is a catchable ParseError on PHP 8; Xmf\Language::loadFile() additionally throws outright on a control character in the path. Uncaught, any of these propagated out of text() and core() and took the admin page down — precisely the failure this class exists to prevent, since a pack bad enough to throw is the pack whose fallback matters most. The attempt flags are set by the callers before load() runs, so swallowing here cannot cause a retry. usable() also treated an NBSP-only constant as a real translation, because trim() knows only ASCII whitespace. A lone NBSP is what a translation tool emits for "intentionally empty" and it renders as invisibly as a space, so it now falls back. That check is a pattern, deliberately, and not a wider trim() charlist: trim() matches BYTES, and 0xA0 is an ordinary UTF-8 continuation byte. Trimming "\xC2\xA0" truncates any value ending in a character that happens to end in 0xA0 — a dagger, U+2020, becomes invalid UTF-8 — which would put a mojibake bug in the one class meant to survive bad packs. The adapter already documents this same trap. preg_match() returns false on invalid UTF-8, which leaves such a value usable, matching the answer the ASCII test already gave it. --- class/Lang.php | 48 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/class/Lang.php b/class/Lang.php index 82e9c6f..fdae68e 100644 --- a/class/Lang.php +++ b/class/Lang.php @@ -75,7 +75,9 @@ public static function text(string $constant, string $fallback): string * and which renders as an invisible button or an empty table header. A blank * label is not a deliberate translation choice often enough to be worth * honouring, and English text in an Arabic admin is recoverable where a - * missing control is not; + * missing control is not. "Whitespace" here includes the Unicode blanks — a + * lone NBSP is what a translation tool emits for "intentionally empty", and it + * renders exactly as invisibly as a space does; * - defined as something that is not SCALAR. Casting an array to string in PHP 8 * emits a warning and yields the literal "Array"; a language file is ordinary * PHP, so this is reachable by a typo rather than by malice. @@ -104,7 +106,24 @@ private static function usable(string $constant): ?string $value = (string) $value; - return '' === \trim($value) ? null : $value; + if ('' === \trim($value)) { + return null; + } + + // trim() knows only ASCII whitespace, so the test above still passes a value + // built solely from Unicode blanks — an NBSP-only constant being the common + // one. Rejected with a pattern rather than by widening the trim() charlist: + // trim() matches BYTES, and \xA0 is an ordinary UTF-8 continuation byte, so + // trimming "\xC2\xA0" truncates any value ending in a character that happens + // to end in \xA0 — "†" (\xE2\x80\xA0) becomes invalid UTF-8. That would put a + // mojibake bug in the one class whose job is surviving bad language packs. + // preg_match() returns false (not 1) on invalid UTF-8, which leaves such a + // value usable — the same answer the ASCII test above already gave it. + if (1 === \preg_match('/^[\s\p{Z}\x{FEFF}]+$/u', $value)) { + return null; + } + + return $value; } /** @@ -177,17 +196,30 @@ private static function loadGlobal(): void * Prefer Xmf\Language::load(): it picks the configured language, falls back to * english on its own, and validates the path. xoops_loadLanguage() is the * fallback for a core build without Xmf. + * + * Both routes `include` third-party PHP. A language pack is ordinary PHP, so it + * can throw — or be a parse error, which is a catchable `ParseError` on PHP 8. + * Uncaught, that would propagate out of text()/core() and take the admin page + * down, which is precisely the failure this class exists to prevent: a pack bad + * enough to throw is exactly the pack whose English fallback matters most. The + * attempt flag is set by the callers BEFORE this runs, so swallowing here cannot + * produce a retry loop — the next lookup goes straight to the fallback. */ private static function load(string $name, string $domain): void { - if (\class_exists(\Xmf\Language::class)) { - \Xmf\Language::load($name, $domain); + try { + if (\class_exists(\Xmf\Language::class)) { + \Xmf\Language::load($name, $domain); - return; - } + return; + } - if (\function_exists('xoops_loadLanguage')) { - \xoops_loadLanguage($name, $domain); + if (\function_exists('xoops_loadLanguage')) { + \xoops_loadLanguage($name, $domain); + } + } catch (\Throwable) { + // Deliberately swallowed: usable() reports the constant as unavailable and + // every caller already carries the English text it needs. } } } From c3cfbc41ffdcef5458177b1db806aaa90cc6cb92 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 21:17:16 -0400 Subject: [PATCH 05/12] refactor(installer): defer the deprecation tag, unblock static analysis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ModuleActionResult carried a formal @deprecated tag while runOne() and runMany() — the primary public API of ModuleActionService — still return it and AdminBulkPage still renders from it. Marking a type deprecated that no caller in the module has migrated away from made every internal use a deprecation violation: 116 of the 138 analysis errors this release introduced. The prose stays, so the direction is still documented; the tag goes on when the internal consumers have moved. The fragment flush in the transcript parser was a by-reference closure rebuilt on every line of every transcript. Because static analysis reads a closure body against the state at its declaration, where the buffer has just been set to '', its emptiness guard was reported as always false. The guard is correct at runtime — it was the shape that could not be verified — so it is now a method. --- class/ModuleActionResult.php | 34 ++++++++----- class/Report/LegacyModuleReportAdapter.php | 59 ++++++++++++++-------- 2 files changed, 59 insertions(+), 34 deletions(-) diff --git a/class/ModuleActionResult.php b/class/ModuleActionResult.php index 4b7766d..4197bc2 100644 --- a/class/ModuleActionResult.php +++ b/class/ModuleActionResult.php @@ -43,23 +43,29 @@ * {@see AdminBulkPage::moduleLogoUrl()}, never from this message, so dropping the * log's own loses nothing. * - * @deprecated Being replaced by {@see Report\ModuleOperationResult}, which carries - * the transcript as data instead of as a string of HTML. The migration - * path now exists in code, not only in prose: - * {@see ModuleActionService::operationResult()} and operationResults() - * return the new type, and {@see self::toOperationResult()} converts a - * result you already hold — so no new code has to name this class. + * LEGACY COMPATIBILITY TYPE. Being replaced by {@see Report\ModuleOperationResult}, + * which carries the transcript as data instead of as a string of HTML. The migration + * path exists in code, not only in prose: {@see ModuleActionService::operationResult()} + * and operationResults() return the new type, and {@see self::toOperationResult()} + * converts a result you already hold — so no new code has to name this class. * - * No removal version is named, and that is still deliberate. This shape - * is what every third-party caller of ModuleActionService uses, so it is - * supported for the whole XOOPS 2.8 line; removal becomes a question at - * XOOPS 4.0, where core emits the structured type itself and this class - * and the adapter disappear together. New code should take the Report - * types. + * No removal version is named, and that is deliberate. This shape is what every + * third-party caller of ModuleActionService uses, so it is supported for the whole + * XOOPS 2.8 line; removal becomes a question at XOOPS 4.0, where core emits the + * structured type itself and this class and the adapter disappear together. New code + * should take the Report types. + * + * Intentionally NOT tagged `@deprecated` yet. runOne()/runMany() — the primary public + * API of ModuleActionService — still return this type, and AdminBulkPage still renders + * from it, so the formal tag would mark as deprecated a type whose replacement no + * caller in this module has adopted. That is not a documentation nicety: it makes + * every internal use a deprecation violation (116 of them, which is what turned the + * QA gate red), training readers to ignore the tag on the release where it finally + * means something. The tag goes on when the internal consumers have moved. */ final readonly class ModuleActionResult { - public const STATUS_OK = 'ok'; + public const STATUS_OK = 'ok'; public const STATUS_SKIP = 'skip'; public const STATUS_FAIL = 'fail'; @@ -129,7 +135,7 @@ public function messageText(): string /** * This result, projected onto the shape that replaces it. * - * The projection lives HERE, on the deprecated type, and not in the Report package + * The projection lives HERE, on the legacy type, and not in the Report package * — deliberately. A strangler fig only works if the dependency points from the old * code to the new: Report\* must never mention ModuleActionResult, or deleting this * class would mean editing the package that outlives it. When core emits events diff --git a/class/Report/LegacyModuleReportAdapter.php b/class/Report/LegacyModuleReportAdapter.php index 6845bba..d67cd59 100644 --- a/class/Report/LegacyModuleReportAdapter.php +++ b/class/Report/LegacyModuleReportAdapter.php @@ -34,7 +34,7 @@ final class LegacyModuleReportAdapter { /** Markers, private to the parse. Input control bytes are stripped first. */ - private const M = "\x01"; + private const M = "\x01"; private const NBSP = "\u{00A0}"; /** @return list */ @@ -57,9 +57,9 @@ public function result(string $dirname, string $action, Outcome $outcome, string $events = $this->parse($legacyHtml); return match ($outcome) { - Outcome::Ok => ModuleOperationResult::ok($dirname, $action, $events), + Outcome::Ok => ModuleOperationResult::ok($dirname, $action, $events), Outcome::Skipped => ModuleOperationResult::skipped($dirname, $action, $reason ?? ''), - Outcome::Failed => ModuleOperationResult::failed($dirname, $action, $reason ?? '', $events), + Outcome::Failed => ModuleOperationResult::failed($dirname, $action, $reason ?? '', $events), }; } @@ -121,13 +121,13 @@ private function toMarkedText(string $text): string */ private function toEvents(string $marked): array { - $events = []; + $events = []; $emphasisDepth = 0; - $errorDepth = 0; + $errorDepth = 0; foreach (\explode("\n", $marked) as $line) { $fragments = []; - $buffer = ''; + $buffer = ''; // Seeded from the state carried in, so a line that only CLOSES a span core // opened before the
still takes the severity. Reading $errorDepth // after the loop instead would miss it: the closing marker has already @@ -141,38 +141,35 @@ private function toEvents(string $marked): array \PREG_SPLIT_DELIM_CAPTURE | \PREG_SPLIT_NO_EMPTY ); - $flush = static function () use (&$buffer, &$fragments, &$emphasisDepth): void { - if ('' !== $buffer) { - $fragments[] = new LogFragment($buffer, $emphasisDepth > 0); - $buffer = ''; - } - }; - foreach ((array) $parts as $part) { switch ($part) { case self::M . 'S': - $flush(); + $this->flushBuffer($buffer, $fragments, $emphasisDepth); ++$emphasisDepth; + break; case self::M . 's': - $flush(); + $this->flushBuffer($buffer, $fragments, $emphasisDepth); $emphasisDepth = \max(0, $emphasisDepth - 1); + break; case self::M . 'E': ++$errorDepth; $lineHadError = true; + break; case self::M . 'e': $errorDepth = \max(0, $errorDepth - 1); + break; default: $buffer .= $part; } } - $flush(); + $this->flushBuffer($buffer, $fragments, $emphasisDepth); $severity = ($lineHadError || $errorDepth > 0) ? LogSeverity::Error : LogSeverity::Info; - $event = $this->withIndentExtracted($severity, $fragments); + $event = $this->withIndentExtracted($severity, $fragments); // Blank lines are an artifact of core's unbalanced fragments, // not information. Dropping them here is why the renderer needs no @@ -185,6 +182,28 @@ private function toEvents(string $marked): array return $events; } + /** + * Move the pending text into a fragment, carrying the emphasis state it was + * accumulated under, and reset the buffer. + * + * A method rather than the by-reference closure this used to be. The closure was + * rebuilt on every line of every transcript, and — because static analysis reads a + * closure body against the state at its DECLARATION, where the buffer has just + * been set to '' — the `'' !== $buffer` guard was reported as always false. The + * guard is real at runtime; it was the shape that could not be verified. + * + * @param list $fragments + */ + private function flushBuffer(string &$buffer, array &$fragments, int $emphasisDepth): void + { + if ('' === $buffer) { + return; + } + + $fragments[] = new LogFragment($buffer, $emphasisDepth > 0); + $buffer = ''; + } + /** * Core indents nested steps with '  ' per level. Pull that out of the * text into a depth, so the renderer can indent with CSS (or with anything else) @@ -220,10 +239,10 @@ private function withIndentExtracted(LogSeverity $severity, array $fragments): L return new LogEvent($severity, $fragments, 0); } - $units = (int) \mb_strlen($lead[0], 'UTF-8'); - $depth = \intdiv($units, 2); + $units = \mb_strlen($lead[0], 'UTF-8'); + $depth = \intdiv($units, 2); $remainder = \str_repeat(self::NBSP, $units % 2); - $rest = $remainder . \substr($first->text, \strlen($lead[0])); + $rest = $remainder . \substr($first->text, \strlen($lead[0])); $fragments[0] = new LogFragment($rest, $first->emphasised); if ('' === $rest) { From fc288b884ddb7cd74c7e348faab3669a9397386c Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 21:19:05 -0400 Subject: [PATCH 06/12] fix(installer): darken the warning colour to meet AA contrast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --installer-badge-notinst was the Bootstrap orange #fd7e14. That value is a fill colour, and .installer-log-warning also draws warning transcript text in it: 2.6:1 against the white report background for normal-size body text, and 2.4:1 for the white text on the badge. Both are under the 4.5:1 AA floor. #b35309 clears it at 5.05:1 either way. The var() fallbacks further down already named this colour. They never applied — a fallback is used only when the custom property is undefined, and the :root block always defines it. --- assets/css/admin.css | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/assets/css/admin.css b/assets/css/admin.css index fb67ea8..9272895 100644 --- a/assets/css/admin.css +++ b/assets/css/admin.css @@ -14,7 +14,14 @@ --installer-badge-inactive: #6c757d; --installer-badge-missing: #dc3545; --installer-badge-protected: #0d6efd; - --installer-badge-notinst: #fd7e14; + /* Darker than the Bootstrap orange this used to be (#fd7e14). That value is a + FILL colour: it carries white badge text at 2.4:1 and, since + .installer-log-warning draws warning transcript text in it, normal-size body + text at 2.6:1 — both under the 4.5:1 AA floor. #b35309 clears it at 5.05:1 in + both directions. The var() fallbacks further down already named this colour; + they never applied, because a fallback is only used when the property is + UNDEFINED, and this :root block always defines it. */ + --installer-badge-notinst: #b35309; } /* From 9c883e7d253e6cb49cf8c6895adef4e9a2f5794a Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 21:19:26 -0400 Subject: [PATCH 07/12] test(installer): cover the report pipeline and language fallbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The report parser, renderer, structured result and language helper shipped without a single test. The parser is the module's trust boundary — everything upstream of it is core's unescaped HTML — and its contract is a round trip, so it is reviewed by re-running fixtures rather than by re-reading regexes. Adds 91 tests: transcript parsing (line splitting, emphasis, severity spans that wrap a line break, unclosed and stray spans, NBSP indent including the odd-unit remainder, marker forging, script content, CRLF, entity collapsing), an end-to-end check that hostile markup cannot reach rendered HTML, the renderer's escape-once and no-attribute-injection properties, every ModuleOperationResult factory and severity branch, the module-logo rejection contract, and the language fallback policy including the UTF-8 boundary cases. The logo positive path and the extension allowlist are not covered: both need a real file under XOOPS_ROOT_PATH/modules//, which a unit run has nowhere to write. They belong to the integration suite. --- tests/Unit/class/AdminBulkPageLogoUrlTest.php | 102 ++++++ tests/Unit/class/LangTest.php | 232 +++++++++++++ .../Report/LegacyModuleReportAdapterTest.php | 304 ++++++++++++++++++ .../class/Report/LogEventHtmlRendererTest.php | 159 +++++++++ .../Report/ModuleOperationResultTest.php | 177 ++++++++++ 5 files changed, 974 insertions(+) create mode 100644 tests/Unit/class/AdminBulkPageLogoUrlTest.php create mode 100644 tests/Unit/class/LangTest.php create mode 100644 tests/Unit/class/Report/LegacyModuleReportAdapterTest.php create mode 100644 tests/Unit/class/Report/LogEventHtmlRendererTest.php create mode 100644 tests/Unit/class/Report/ModuleOperationResultTest.php diff --git a/tests/Unit/class/AdminBulkPageLogoUrlTest.php b/tests/Unit/class/AdminBulkPageLogoUrlTest.php new file mode 100644 index 0000000..fa46b52 --- /dev/null +++ b/tests/Unit/class/AdminBulkPageLogoUrlTest.php @@ -0,0 +1,102 @@ +. Every case below must resolve to null BEFORE any + * filesystem call, which is what makes them assertable without a booted XOOPS. + * + * NOT covered here, and deliberately: the positive path and the image-extension + * allowlist both need a real file under XOOPS_ROOT_PATH/modules//, which + * a unit run has no writable location for. Those need the integration suite. + */ +#[CoversClass(AdminBulkPage::class)] +final class AdminBulkPageLogoUrlTest extends TestCase +{ + #[Test] + public function nullInfoIsRejected(): void + { + self::assertNull(AdminBulkPage::moduleLogoUrl('quotes', null)); + } + + /** @return iterable */ + public static function badDirnames(): iterable + { + yield 'empty' => ['']; + yield 'whitespace only' => [' ']; + yield 'dot segment' => ['.']; + yield 'parent segment' => ['..']; + yield 'contains a dot' => ['foo.bar']; + yield 'contains a slash' => ['foo/bar']; + yield 'contains a backslash' => ['foo\\bar']; + yield 'contains a space' => ['foo bar']; + yield 'traversal' => ['../../etc']; + yield 'null byte' => ["quotes\0"]; + yield 'quote' => ["foo'); alert(1);//"]; + } + + #[Test] + #[DataProvider('badDirnames')] + public function dirnameOutsideTheAllowlistIsRejected(string $dirname): void + { + self::assertNull(AdminBulkPage::moduleLogoUrl($dirname, $this->info('logo.png'))); + } + + /** @return iterable */ + public static function badImageValues(): iterable + { + yield 'empty' => ['']; + yield 'whitespace only' => [' ']; + yield 'absolute posix path' => ['/etc/passwd']; + yield 'absolute after backslash normalisation' => ['\\windows\\system32\\x.png']; + yield 'parent traversal' => ['../../../mainfile.php']; + yield 'parent traversal mid-path' => ['assets/../../secret.png']; + yield 'dot segment' => ['./logo.png']; + yield 'empty segment' => ['assets//logo.png']; + yield 'null byte' => ["logo.png\0.txt"]; + yield 'newline' => ["logo\n.png"]; + yield 'control byte' => ["logo\x1F.png"]; + yield 'protocol' => ['https://evil.example/x.png']; + yield 'protocol relative' => ['//evil.example/x.png']; + } + + #[Test] + #[DataProvider('badImageValues')] + public function hostileImageValueIsRejected(string $image): void + { + self::assertNull(AdminBulkPage::moduleLogoUrl('quotes', $this->info($image))); + } + + /** A well-formed request for a module that is not on disk still resolves to null. */ + #[Test] + public function absentModuleDirectoryIsRejected(): void + { + self::assertNull( + AdminBulkPage::moduleLogoUrl('no_such_module_' . \bin2hex('x'), $this->info('assets/logo.png')) + ); + } + + /** + * @return array{dirname:string,name:string,version:mixed,module_status:mixed,image:string,description:string} + */ + private function info(string $image): array + { + return [ + 'dirname' => 'quotes', + 'name' => 'Quotes', + 'version' => '1.0', + 'module_status' => 'Beta', + 'image' => $image, + 'description' => 'test', + ]; + } +} diff --git a/tests/Unit/class/LangTest.php b/tests/Unit/class/LangTest.php new file mode 100644 index 0000000..4b750fc --- /dev/null +++ b/tests/Unit/class/LangTest.php @@ -0,0 +1,232 @@ + %2$s', 'first', 'second') + ); + } + + /** + * A template that asks for more arguments than it is given throws on PHP 8. That + * must degrade to English, not take the page down. + */ + #[Test] + public function formatFallsBackWhenTheTranslationDemandsTooManyArguments(): void + { + \define('_AM_MODULEINSTALLER_TEST_FMT_GREEDY', 'wants %1$s and %2$s'); + + self::assertSame( + 'got one', + Lang::format('_AM_MODULEINSTALLER_TEST_FMT_GREEDY', 'got %s', 'one') + ); + } + + /** When even the English template disagrees, return it raw rather than nothing. */ + #[Test] + public function formatReturnsTheRawFallbackWhenBothTemplatesAreWrong(): void + { + \define('_AM_MODULEINSTALLER_TEST_FMT_BOTHBAD', 'wants %1$s and %2$s'); + + self::assertSame( + 'also wants %2$s', + Lang::format('_AM_MODULEINSTALLER_TEST_FMT_BOTHBAD', 'also wants %2$s', 'one') + ); + } + + /** + * core() reads the other domain but applies the same usability policy. With no + * bootable XOOPS the global file cannot load, which is exactly the situation the + * fallback is for — and it must return, not throw. + */ + #[Test] + public function coreConstantFallsBackWithoutABootedXoops(): void + { + self::assertSame('Yes', Lang::core('_AM_MODULEINSTALLER_TEST_CORE_MISSING', 'Yes')); + } + + #[Test] + public function coreAppliesTheSameBlanknessPolicy(): void + { + \define('_AM_MODULEINSTALLER_TEST_CORE_NBSP', self::NBSP); + + self::assertSame('No', Lang::core('_AM_MODULEINSTALLER_TEST_CORE_NBSP', 'No')); + } +} diff --git a/tests/Unit/class/Report/LegacyModuleReportAdapterTest.php b/tests/Unit/class/Report/LegacyModuleReportAdapterTest.php new file mode 100644 index 0000000..671b195 --- /dev/null +++ b/tests/Unit/class/Report/LegacyModuleReportAdapterTest.php @@ -0,0 +1,304 @@ +parse('')); + } + + #[Test] + public function plainTextBecomesOneInfoEvent(): void + { + $events = (new LegacyModuleReportAdapter())->parse('Module installed'); + + self::assertCount(1, $events); + self::assertSame('Module installed', $events[0]->plainText()); + self::assertSame(LogSeverity::Info, $events[0]->severity); + self::assertSame(0, $events[0]->depth); + } + + #[Test] + public function lineBreakingTagsSplitEvents(): void + { + $events = (new LegacyModuleReportAdapter())->parse('first
secondthird'); + + self::assertSame(['first', 'second', 'third'], $this->texts($events)); + } + + #[Test] + public function emphasisTagsBecomeEmphasisedFragments(): void + { + $events = (new LegacyModuleReportAdapter())->parse('table x_quotes created'); + + self::assertCount(1, $events); + self::assertSame('table x_quotes created', $events[0]->plainText()); + + $emphasised = \array_values(\array_filter( + $events[0]->fragments, + static fn (\XoopsModules\Moduleinstaller\Report\LogFragment $f): bool => $f->emphasised + )); + self::assertCount(1, $emphasised); + self::assertSame('x_quotes', $emphasised[0]->text); + } + + #[Test] + public function redSpanMarksTheLineAsError(): void + { + $events = (new LegacyModuleReportAdapter())->parse('could not add'); + + self::assertCount(1, $events); + self::assertSame(LogSeverity::Error, $events[0]->severity); + self::assertSame('could not add', $events[0]->plainText()); + } + + /** + * Core emits spans that wrap a
. Both halves are the same red run, so both + * lines are errors — this is why the depth counters live outside the line loop. + */ + #[Test] + public function errorSpanWrappingALineBreakColoursBothLines(): void + { + $events = (new LegacyModuleReportAdapter())->parse('before
after'); + + self::assertSame(['before', 'after'], $this->texts($events)); + self::assertSame(LogSeverity::Error, $events[0]->severity); + self::assertSame(LogSeverity::Error, $events[1]->severity); + } + + #[Test] + public function unclosedErrorSpanColoursEveryFollowingLine(): void + { + $events = (new LegacyModuleReportAdapter())->parse('ok
bad
alsobad'); + + self::assertSame(['ok', 'bad', 'alsobad'], $this->texts($events)); + self::assertSame(LogSeverity::Info, $events[0]->severity); + self::assertSame(LogSeverity::Error, $events[1]->severity); + self::assertSame(LogSeverity::Error, $events[2]->severity); + } + + /** A stray close tag must not drive the counter negative and invert the state. */ + #[Test] + public function strayClosingSpanDoesNotInvertSeverity(): void + { + $events = (new LegacyModuleReportAdapter())->parse('plain
stillplain'); + + self::assertSame(['plain', 'stillplain'], $this->texts($events)); + self::assertSame(LogSeverity::Info, $events[0]->severity); + self::assertSame(LogSeverity::Info, $events[1]->severity); + } + + #[Test] + public function nbspPairsBecomeDepthAndLeaveTheTextClean(): void + { + $events = (new LegacyModuleReportAdapter())->parse('  nested'); + + self::assertCount(1, $events); + self::assertSame(1, $events[0]->depth); + self::assertSame('nested', $events[0]->plainText()); + } + + #[Test] + public function fourNbspAreTwoLevelsOfDepth(): void + { + $events = (new LegacyModuleReportAdapter())->parse('    deeper'); + + self::assertSame(2, $events[0]->depth); + self::assertSame('deeper', $events[0]->plainText()); + } + + /** Only whole pairs are structure; the odd one is content and must survive. */ + #[Test] + public function oddNbspIsKeptAtTheHeadOfTheText(): void + { + $events = (new LegacyModuleReportAdapter())->parse('   odd'); + + self::assertSame(1, $events[0]->depth); + self::assertSame(self::NBSP . 'odd', $events[0]->plainText()); + } + + /** ASCII space is content, not nesting — core does not indent with it. */ + #[Test] + public function leadingAsciiSpaceIsContentNotIndent(): void + { + $events = (new LegacyModuleReportAdapter())->parse(' spaced'); + + self::assertSame(0, $events[0]->depth); + self::assertSame(' spaced', $events[0]->plainText()); + } + + /** + * The documented byte-vs-character trap. Every one of these characters encodes + * as 0xC2 followed by a continuation byte, so any byte-wise trim of the NBSP + * "\xC2\xA0" eats the lead byte and leaves an orphan — which surfaces as U+FFFD + * after escaping and makes json_encode() of the plain text return false. + * Translated packs use exactly these as bullets. + */ + #[Test] + public function latin1SupplementBulletsSurviveIndentExtraction(): void + { + foreach (['«', '»', '·', '¡', '¿', '°', '±'] as $bullet) { + $events = (new LegacyModuleReportAdapter())->parse('  ' . $bullet . ' item'); + + self::assertCount(1, $events, "bullet {$bullet}"); + self::assertSame(1, $events[0]->depth, "bullet {$bullet}"); + self::assertSame($bullet . ' item', $events[0]->plainText(), "bullet {$bullet}"); + self::assertTrue( + \mb_check_encoding($events[0]->plainText(), 'UTF-8'), + "bullet {$bullet} produced invalid UTF-8" + ); + self::assertNotFalse( + \json_encode($events[0]->plainText()), + "bullet {$bullet} is not JSON-encodable" + ); + } + } + + #[Test] + public function daggerAtEndOfLineSurvivesIntact(): void + { + // "†" is U+2020 = \xE2\x80\xA0 — it ENDS in the same byte as NBSP. + $events = (new LegacyModuleReportAdapter())->parse('note †'); + + self::assertSame('note †', $events[0]->plainText()); + self::assertTrue(\mb_check_encoding($events[0]->plainText(), 'UTF-8')); + } + + /** Markers are private to the parse; control bytes are stripped before they run. */ + #[Test] + public function markerBytesInInputCannotForgeSeverity(): void + { + $events = (new LegacyModuleReportAdapter())->parse("\x01Enot an error"); + + self::assertCount(1, $events); + self::assertSame(LogSeverity::Info, $events[0]->severity); + self::assertSame('Enot an error', $events[0]->plainText()); + self::assertStringNotContainsString("\x01", $events[0]->plainText()); + } + + #[Test] + public function scriptAndStyleContentIsDiscardedEntirely(): void + { + $events = (new LegacyModuleReportAdapter())->parse( + 'kept' + ); + + self::assertSame(['kept'], $this->texts($events)); + } + + #[Test] + public function crlfIsNormalisedAndLeavesNoStrayCarriageReturn(): void + { + $events = (new LegacyModuleReportAdapter())->parse("alpha\r\nbeta"); + + self::assertSame(['alpha', 'beta'], $this->texts($events)); + foreach ($events as $event) { + self::assertStringNotContainsString("\r", $event->plainText()); + } + } + + #[Test] + public function blankLinesAreDropped(): void + { + $events = (new LegacyModuleReportAdapter())->parse('one


two'); + + self::assertSame(['one', 'two'], $this->texts($events)); + } + + #[Test] + public function coreEscapingIsCollapsedExactlyOnce(): void + { + $events = (new LegacyModuleReportAdapter())->parse('Smith & Sons <tag>'); + + self::assertSame('Smith & Sons ', $events[0]->plainText()); + } + + /** + * The security claim of the whole feature, asserted end to end: hostile markup in + * core's log cannot become markup in the admin page. The transcript is data; the + * renderer writes every tag from a literal in its own file. + */ + #[Test] + public function hostileMarkupCannotSurviveIntoRenderedHtml(): void + { + $hostile = '' + . 'click' + . '' + . '" onmouseover="alert(4)'; + + $html = (new LogEventHtmlRenderer())->render( + (new LegacyModuleReportAdapter())->parse($hostile) + ); + + self::assertStringNotContainsString('render([])); + } + + #[Test] + public function singleInfoEventRendersPlainTextWithoutSpan(): void + { + $renderer = new LogEventHtmlRenderer(); + + $out = $renderer->render([LogEvent::text('hello world')]); + + self::assertSame('hello world', $out); + self::assertStringNotContainsString('render([LogEvent::text('done', LogSeverity::Success)]); + + self::assertSame('done', $out); + } + + #[Test] + public function warningWrapsInCorrectSpanClass(): void + { + $renderer = new LogEventHtmlRenderer(); + + $out = $renderer->render([LogEvent::text('careful', LogSeverity::Warning)]); + + self::assertSame('careful', $out); + } + + #[Test] + public function errorWrapsInCorrectSpanClass(): void + { + $renderer = new LogEventHtmlRenderer(); + + $out = $renderer->render([LogEvent::text('boom', LogSeverity::Error)]); + + self::assertSame('boom', $out); + } + + #[Test] + public function emphasisedFragmentRendersStrongInsideSpan(): void + { + $renderer = new LogEventHtmlRenderer(); + + $event = new LogEvent(LogSeverity::Success, [ + new LogFragment('table '), + new LogFragment('x8a9_quotes', true), + new LogFragment(' created'), + ], 0); + + $out = $renderer->render([$event]); + + self::assertSame( + 'table x8a9_quotes created', + $out + ); + } + + #[Test] + public function escapingHappensExactlyOnce(): void + { + $renderer = new LogEventHtmlRenderer(); + + $out = $renderer->render([LogEvent::text('x')]); + + self::assertSame('<b>x</b>', $out); + self::assertStringNotContainsString('', $out); + } + + #[Test] + public function attributeInjectionIsImpossible(): void + { + $renderer = new LogEventHtmlRenderer(); + + $out = $renderer->render([LogEvent::text('" onload="alert(1)', LogSeverity::Error)]); + + self::assertStringContainsString('"', $out); + self::assertStringNotContainsString('boom', + $out + ); + } + + #[Test] + public function twoEventsAreJoinedByBrAndNewline(): void + { + $renderer = new LogEventHtmlRenderer(); + + $out = $renderer->render([ + LogEvent::text('first'), + LogEvent::text('second'), + ]); + + self::assertSame("first
\nsecond", $out); + } +} diff --git a/tests/Unit/class/Report/ModuleOperationResultTest.php b/tests/Unit/class/Report/ModuleOperationResultTest.php new file mode 100644 index 0000000..9b25753 --- /dev/null +++ b/tests/Unit/class/Report/ModuleOperationResultTest.php @@ -0,0 +1,177 @@ +isOk()); + self::assertFalse($result->isSkipped()); + self::assertFalse($result->isFailed()); + self::assertSame(Outcome::Ok, $result->outcome); + self::assertNull($result->reason); + } + + #[Test] + public function skippedFactoryProducesSingleWarningEvent(): void + { + $result = ModuleOperationResult::skipped('quotes', 'install', 'already installed'); + + self::assertTrue($result->isSkipped()); + self::assertSame('already installed', $result->reason); + self::assertCount(1, $result->events); + self::assertSame(LogSeverity::Warning, $result->events[0]->severity); + self::assertSame('already installed', $result->events[0]->plainText()); + } + + #[Test] + public function failedFactoryPrependsReasonAsErrorEvent(): void + { + $transcript = [ + LogEvent::text('step one'), + LogEvent::text('step two'), + ]; + + $result = ModuleOperationResult::failed('quotes', 'install', 'install failed', $transcript); + + self::assertCount(3, $result->events); + self::assertSame(LogSeverity::Error, $result->events[0]->severity); + self::assertSame('install failed', $result->events[0]->plainText()); + self::assertSame('step one', $result->events[1]->plainText()); + self::assertSame('step two', $result->events[2]->plainText()); + } + + #[Test] + public function failedWithTranscriptDoesNotPrependOrDuplicate(): void + { + $transcript = [ + LogEvent::text('install failed', LogSeverity::Error), + LogEvent::text('step two'), + ]; + + $result = ModuleOperationResult::failedWithTranscript('quotes', 'install', $transcript); + + self::assertCount(2, $result->events); + self::assertSame('install failed', $result->reason); + self::assertSame($result->events[0]->plainText(), $result->reason); + + $occurrences = \substr_count($result->plainText(), 'install failed'); + self::assertSame(1, $occurrences); + } + + #[Test] + public function failedWithTranscriptEmptyArrayYieldsEmptyReason(): void + { + $result = ModuleOperationResult::failedWithTranscript('quotes', 'install', []); + + self::assertSame('', $result->reason); + self::assertSame([], $result->events); + } + + #[Test] + public function hasErrorEventsIsIndependentOfOutcome(): void + { + $okWithError = ModuleOperationResult::ok('quotes', 'install', [ + LogEvent::text('something odd', LogSeverity::Error), + ]); + self::assertTrue($okWithError->hasErrorEvents()); + + $failedAllInfo = ModuleOperationResult::failedWithTranscript('quotes', 'install', [ + LogEvent::text('just info', LogSeverity::Info), + ]); + self::assertFalse($failedAllInfo->hasErrorEvents()); + } + + #[Test] + public function severityFailedIsError(): void + { + $result = ModuleOperationResult::failed('quotes', 'install', 'boom'); + + self::assertSame(LogSeverity::Error, $result->severity()); + } + + #[Test] + public function severitySkippedIsWarning(): void + { + $result = ModuleOperationResult::skipped('quotes', 'install', 'already there'); + + self::assertSame(LogSeverity::Warning, $result->severity()); + } + + #[Test] + public function severityOkWithErrorEventIsWarning(): void + { + $result = ModuleOperationResult::ok('quotes', 'install', [ + LogEvent::text('odd but survived', LogSeverity::Error), + ]); + + self::assertSame(LogSeverity::Warning, $result->severity()); + } + + #[Test] + public function severityOkWithCleanEventsIsSuccess(): void + { + $result = ModuleOperationResult::ok('quotes', 'install', [ + LogEvent::text('all good', LogSeverity::Success), + ]); + + self::assertSame(LogSeverity::Success, $result->severity()); + } + + #[Test] + public function plainTextJoinsEventPlainTextWithNewline(): void + { + $result = ModuleOperationResult::ok('quotes', 'install', [ + LogEvent::text('first'), + LogEvent::text('second'), + ]); + + self::assertSame("first\nsecond", $result->plainText()); + } + + #[Test] + public function toArrayHasExactKeysAndEnumStringValues(): void + { + $result = ModuleOperationResult::failed('quotes', 'install', 'install failed', [ + LogEvent::text('step one', LogSeverity::Info, 1), + ]); + + $array = $result->toArray(); + + self::assertSame( + ['dirname', 'action', 'outcome', 'severity', 'reason', 'events'], + \array_keys($array) + ); + self::assertSame('quotes', $array['dirname']); + self::assertSame('install', $array['action']); + self::assertSame('fail', $array['outcome']); + self::assertSame('error', $array['severity']); + self::assertSame('install failed', $array['reason']); + + self::assertCount(2, $array['events']); + foreach ($array['events'] as $eventArray) { + self::assertSame(['severity', 'depth', 'text'], \array_keys($eventArray)); + } + + self::assertSame('error', $array['events'][0]['severity']); + self::assertSame(0, $array['events'][0]['depth']); + self::assertSame('install failed', $array['events'][0]['text']); + + self::assertSame('info', $array['events'][1]['severity']); + self::assertSame(1, $array['events'][1]['depth']); + self::assertSame('step one', $array['events'][1]['text']); + } +} From 943ea6ef2e684e3e494283ac99339eccfce1ec04 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 21:19:45 -0400 Subject: [PATCH 08/12] docs(installer): correct the language-string and test-count claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 1.7.0 section adds 30 language constants, not 27; lang_diff.txt was right and both changelogs were wrong. The suite claim was 169 tests against an actual 18, and named crash and timeout coverage that does not exist anywhere in tests/ — it now states the real figure and scope. Also: the tutorial heading dropped the prerelease label that every other file carries, and a note in lang_diff used "fataling" for what is a fatal error. --- CHANGELOG.md | 7 ++++--- docs/TUTORIAL.md | 4 ++-- docs/changelog.txt | 11 +++++++---- docs/lang_diff.txt | 3 ++- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58ddffa..7c2d2f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ Baseline comparison for **1.6.0**: previous public tree `1.5.1-Final` HTML-independent report consumption - `ModuleActionService::operationResult()` and `operationResults()` for callers adopting `Report\ModuleOperationResult` -- 27 translatable admin-report strings with safe English fallbacks +- 30 translatable admin-report strings with safe English fallbacks ### Changed @@ -31,8 +31,9 @@ Baseline comparison for **1.6.0**: previous public tree `1.5.1-Final` - RTL output now isolates counters, versions, statuses, folders, and plan summaries - Admin report layout uses logical CSS properties, removes module-owned inline styles, improves long-log scrolling, and avoids duplicate screen-reader labels -- Expanded cross-platform tests for hostile log input, structured-result contracts, - translations, logo paths, UTF-8, crashes, and timeouts +- Expanded tests (109 tests / 264 assertions) for hostile log input, + structured-result contracts, translation fallbacks, module-logo path rejection, + and UTF-8 boundary handling ### Fixed diff --git a/docs/TUTORIAL.md b/docs/TUTORIAL.md index 673a528..ee23cf4 100644 --- a/docs/TUTORIAL.md +++ b/docs/TUTORIAL.md @@ -1,4 +1,4 @@ -# ModuleInstaller Tutorial (1.7.0) +# ModuleInstaller Tutorial (1.7.0 Alpha 1) ## What this module does @@ -160,5 +160,5 @@ After Focus, a success notice may include a link to open the **auto-snapshot** s ## See also - [README.md](../README.md) — overview, architecture, development -- [CHANGELOG.md](../CHANGELOG.md) / [changelog.txt](changelog.txt) — release history · [lang_diff.txt](lang_diff.txt) — language changes +- [CHANGELOG.md](../CHANGELOG.md) / [changelog.txt](changelog.txt) — release history · [lang_diff.txt](lang_diff.txt) — language changes - Tutorial (GitBook): https://xoops.gitbook.io/xoops-moduleinstaller/ diff --git a/docs/changelog.txt b/docs/changelog.txt index 8cbeabe..432af16 100644 --- a/docs/changelog.txt +++ b/docs/changelog.txt @@ -37,7 +37,7 @@ Structured reports and compatibility the distinction between the operation outcome and errors reported by core. Internationalisation and admin UI -- Added 27 translatable admin-report strings and Lang helpers with safe English +- Added 30 translatable admin-report strings and Lang helpers with safe English fallbacks for missing, blank, non-scalar, or malformed translations. - Added bidirectional isolation for counters, versions, statuses, folders, and plan summaries in RTL admin interfaces. @@ -45,9 +45,12 @@ Internationalisation and admin UI to logical properties and removed module-owned inline style attributes. Tests -- Expanded the suite to 169 tests covering hostile log input, structured-result - contracts, language fallbacks, logo-path validation, UTF-8, and real timeout/ - crash handling on POSIX and Windows. +- Expanded the suite to 109 tests / 264 assertions covering hostile log input, + structured-result contracts, language fallbacks, logo-path rejection and UTF-8 + boundary handling. +- The module-logo POSITIVE path and the image-extension allowlist are not unit + covered: both need a real file under XOOPS_ROOT_PATH/modules//, so they + belong to the integration suite. Only the rejection contract is asserted here. -------------------------------------------------------------------------------- 1.6.0 Stable [2026-07-23] diff --git a/docs/lang_diff.txt b/docs/lang_diff.txt index b774a7a..c1653ef 100644 --- a/docs/lang_diff.txt +++ b/docs/lang_diff.txt @@ -49,6 +49,7 @@ Notes was untranslatable regardless of the language pack installed. - Every lookup goes through class/Lang.php, which carries the English text as a fallback: a language pack that omits one of these degrades to English instead of - fataling (an undefined constant is a fatal Error on PHP 8, not a notice). + causing a fatal error (an undefined constant is a fatal Error on PHP 8, not a + notice). - Translators: %1$s / %2$s ordering may be reordered freely; Lang::format() passes the arguments positionally. From cd4a47182337f1ce935f751bc97d96b74ecbc430 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 21:29:59 -0400 Subject: [PATCH 09/12] refactor(sets): drop the dead default on ModuleSet::$modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The constructor assigns $this->modules unconditionally before any read, so the = [] initialiser was dead. RemoveDefaultValueFromAssignedPropertyRector flags it. No behaviour change. This is a pre-existing issue in a file this release does not otherwise touch, and it surfaced only now: the QA script runs cs:check first and that gate was failing, so rector never ran in CI. With formatting and static analysis clean, rector finally got its turn. It broke without a code change because composer.lock is not tracked, so every CI run resolves dependencies afresh — rector 2.6 is newer than the version the previous green run used. Worth deciding separately whether to commit the lock, or to pin the QA tools, so that a tool release cannot turn an untouched branch red. --- class/Set/ModuleSet.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/class/Set/ModuleSet.php b/class/Set/ModuleSet.php index 5c2bbbf..959fcec 100644 --- a/class/Set/ModuleSet.php +++ b/class/Set/ModuleSet.php @@ -20,7 +20,7 @@ final class ModuleSet { /** @var list */ - private array $modules = []; + private array $modules; /** * @param array $modules Raw dirnames; normalized (validated, de-duped, sorted) on construction. From dfc25c9875d41689db3edec8967642bd9e429ebd Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 21:35:52 -0400 Subject: [PATCH 10/12] update scrutinizer --- .scrutinizer.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.scrutinizer.yml b/.scrutinizer.yml index 804f3ac..afd5264 100644 --- a/.scrutinizer.yml +++ b/.scrutinizer.yml @@ -1,7 +1,7 @@ # language: php build: environment: - php: 8.1.0 + php: 8.2 nodes: tests: true analysis: @@ -9,8 +9,8 @@ build: after: # The following installs the most recent XOOPS CMS version, you might want # to install a specific release tag or branch instead. - - git clone --depth=1 https://github.com/XOOPS/XoopsCore25 - - git clone --depth=1 https://github.com/mambax7/tag + - git clone --depth=1 https://github.com/XOOPS/XoopsCore27 + # - git clone --depth=1 https://github.com/mambax7/tag project_setup: override: true tests: @@ -18,6 +18,6 @@ build: - php-scrutinizer-run --enable-security-analysis filter: dependency_paths: - - XoopsCore25/* + - XoopsCore27/* - tag/* From 1cc6d52653a1f0cfc6d621a66c2795cf3839ad60 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 22:30:54 -0400 Subject: [PATCH 11/12] security(installer): fix the second inline handler and the attribute layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects, both in the escaping work from earlier in this branch. renderYesNoRadios() built its selectModule() handler the old way, with htmlspecialchars() into a JavaScript string literal. That is the same defect already fixed for toggleModuleRow(), in a call site the first pass did not look for — the fix went where the report pointed instead of sweeping the file. The first fix was also incomplete. JSON_HEX_QUOT escapes quotes inside the encoded string, but not json_encode's own delimiters, so the result still began and ended with a literal double quote. Interpolated into onclick="…" that closed the attribute early and turned the remainder into stray attributes on the element. The earlier check missed it by running html_entity_decode() over the string without ever parsing it as HTML, which cannot show an attribute ending in the wrong place. Both layers are now applied in one place: json_encode() with the HEX flags for the JavaScript, then htmlspecialchars() over the whole call for the attribute. Both handlers go through jsCall(), so a third one cannot pick a different answer. Verified by parsing the rendered markup with DOMDocument: zero parse errors, the input carries exactly its six intended attributes with no injected extras, and the DOM reports the handler as selectModule("foo'); alert(1);\/\/", this) — payload inside the literal. The new tests assert against the decoded attribute rather than the raw markup, because raw-markup assertions pass against this bug. --- class/AdminBulkPage.php | 77 ++++++++++--- .../class/AdminBulkPageJsEscapingTest.php | 107 ++++++++++++++++++ 2 files changed, 166 insertions(+), 18 deletions(-) create mode 100644 tests/Unit/class/AdminBulkPageJsEscapingTest.php diff --git a/class/AdminBulkPage.php b/class/AdminBulkPage.php index 93f891f..42a85b0 100644 --- a/class/AdminBulkPage.php +++ b/class/AdminBulkPage.php @@ -338,23 +338,10 @@ public static function renderModuleTable(array $dirnames, array $preselect = [], $dirnameEsc = \htmlspecialchars($file, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'); $nameEsc = \htmlspecialchars($info['name'], \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'); - // The dirname lands in two different contexts and needs two different - // escapers. htmlspecialchars() is right for id='…'; it is WRONG inside the - // onclick JS string literal, because the browser HTML-decodes an attribute - // value BEFORE compiling it as JavaScript — so ' arrives at the JS - // parser as a bare quote and closes the literal. Core applies no character - // filter here (XoopsLists::getDirListAsArray() skips only dot-entries and - // 'cvs'), so a directory named foo'); alert(1);// reaches this line - // verbatim. json_encode() emits a complete, already-quoted JS literal, and - // the HEX flags keep <, >, &, ' and " out of the attribute altogether. - $dirnameJs = \json_encode( - $file, - \JSON_HEX_TAG | \JSON_HEX_AMP | \JSON_HEX_APOS | \JSON_HEX_QUOT - ); - // false only for invalid UTF-8. An inert handler beats a malformed one. - $toggleJs = false === $dirnameJs - ? 'void 0' - : 'toggleModuleRow(' . $dirnameJs . ')'; + // The dirname lands in two contexts and needs two escapers: + // htmlspecialchars() for id='…', and jsCall() for the handler — see there + // for why HTML escaping is the wrong tool inside an inline handler. + $toggleJs = self::jsCall('toggleModuleRow', $file); $searchBlob = \htmlspecialchars( \mb_strtolower($info['name'] . ' ' . $file . ' ' . $info['description']), \ENT_QUOTES | \ENT_SUBSTITUTE, @@ -498,7 +485,7 @@ public static function renderYesNoRadios(string $dirname, int $value): string $nameAttr = 'modules[' . \htmlspecialchars($dirname, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . ']'; $idYes = $safeId . '_1'; $idNo = $safeId . '_2'; - $onclick = "selectModule('" . \htmlspecialchars($dirname, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8') . "', this)"; + $onclick = self::jsCall('selectModule', $dirname, 'this'); // Core constants from language//global.php, so a different domain. $yesLabel = Lang::core('_YES', 'Yes'); $noLabel = Lang::core('_NO', 'No'); @@ -623,6 +610,60 @@ public static function renderReport(array $results, string $title = ''): string return $html . ''; } + /** + * An inline-handler call whose string argument is safe in a JS context. + * + * Centralised because getting this wrong is silent, and this file got it wrong + * twice: htmlspecialchars() is an HTML escaper, and inside an event-handler + * attribute the browser HTML-DECODES the value before the JavaScript is + * compiled. An entity-escaped apostrophe therefore arrives at the JS parser as + * a bare quote and closes the string literal, so a folder named + * + * foo'); alert(1);// + * + * became a live call. Core filters nothing — XoopsLists::getDirListAsArray() + * skips only dot-entries and 'cvs' — and an apostrophe is a legal filename + * character on NTFS and ext4 alike, so the name reaches the renderer verbatim. + * + * BOTH layers are needed, and the returned value is ATTRIBUTE-READY: + * + * 1. json_encode() for the JavaScript layer. The HEX flags keep <, >, &, ' + * and " out of the string BODY, so nothing in the value can end the + * literal or open a tag. + * 2. htmlspecialchars() for the HTML layer, over the whole call. This is not + * redundant: the HEX flags do not touch json_encode's own delimiters, so + * the result still begins and ends with a literal double quote, which + * would close a double-quoted attribute and turn the remainder into stray + * attributes on the element. Escaping to " keeps the attribute + * intact, and the browser decodes it back to a plain quote before the + * JavaScript is compiled — which is exactly the decode step that makes + * HTML-escaping alone unsafe, used here in the direction that is correct. + * + * Any new inline handler in this class must go through here. + * + * @param string $function JS function name — a literal in this file, never input + * @param string $argument the untrusted value, encoded as a JS string literal + * @param string $extraArgs additional raw JS arguments, e.g. 'this' + * + * @return string ready to interpolate into a quoted HTML attribute as-is + */ + private static function jsCall(string $function, string $argument, string $extraArgs = ''): string + { + $encoded = \json_encode( + $argument, + \JSON_HEX_TAG | \JSON_HEX_AMP | \JSON_HEX_APOS | \JSON_HEX_QUOT + ); + + // false only for invalid UTF-8. An inert handler beats a malformed one. + if (false === $encoded) { + return 'void 0'; + } + + $call = $function . '(' . $encoded . ('' === $extraArgs ? '' : ', ' . $extraArgs) . ')'; + + return \htmlspecialchars($call, \ENT_QUOTES | \ENT_SUBSTITUTE, 'UTF-8'); + } + /** * Build a same-origin URL for a module logo after proving it is a local file. * diff --git a/tests/Unit/class/AdminBulkPageJsEscapingTest.php b/tests/Unit/class/AdminBulkPageJsEscapingTest.php new file mode 100644 index 0000000..bc651f0 --- /dev/null +++ b/tests/Unit/class/AdminBulkPageJsEscapingTest.php @@ -0,0 +1,107 @@ +firstOnclick($html)); + } + + /** @return iterable */ + public static function breakoutAttempts(): iterable + { + yield 'apostrophe breakout' => ["foo'); alert(1);//"]; + yield 'double quote breakout' => ['foo"); alert(1);//']; + yield 'script tag' => ['foo']; + yield 'ampersand entity' => ['foo'); alert(1);//']; + yield 'double encoded' => ['foo&#039;); alert(1);//']; + yield 'closing attribute' => ['foo" onmouseover="alert(1)']; + yield 'backslash' => ['foo\\"); alert(1);//']; + yield 'newline' => ["foo\n); alert(1);//"]; + } + + /** + * The decisive assertion: after the browser decodes the attribute, the payload + * must still be INSIDE the JS string literal. If it escaped, the decoded text + * would contain a bare quote followed by the injected call. + */ + #[Test] + #[DataProvider('breakoutAttempts')] + public function hostileDirnameCannotEscapeTheJsStringLiteral(string $dirname): void + { + $decoded = $this->firstOnclick(AdminBulkPage::renderYesNoRadios($dirname, 0)); + + // Exactly one call, with exactly two arguments, and the payload contained. + self::assertMatchesRegularExpression( + '/^selectModule\("(?:[^"\\\\]|\\\\.)*", this\)$/', + $decoded, + 'handler is not a single well-formed call: ' . $decoded + ); + // No raw quote of either kind can appear inside the literal body. + $body = \substr($decoded, \strlen('selectModule("'), -\strlen('", this)')); + self::assertStringNotContainsString("'", $body); + self::assertStringNotContainsString('"', $body); + self::assertStringNotContainsString('<', $body); + } + + /** Both radios must carry the same handler — the bug was per-attribute, not per-row. */ + #[Test] + public function bothRadiosCarryTheIdenticalSafeHandler(): void + { + $html = AdminBulkPage::renderYesNoRadios("foo'); alert(1);//", 1); + + \preg_match_all('/onclick="([^"]*)"/', $html, $m); + self::assertCount(2, $m[1], 'expected a handler on each radio'); + self::assertSame($m[1][0], $m[1][1]); + } + + /** The element id is a separate, HTML-side concern and stays sanitised. */ + #[Test] + public function elementIdIsSanitisedIndependently(): void + { + $html = AdminBulkPage::renderYesNoRadios("foo'); alert(1);//", 1); + + self::assertStringContainsString('id="modsel_foo____alert_1_____1"', $html); + self::assertStringNotContainsString('id="modsel_foo\'', $html); + } + + /** Invalid UTF-8 yields an inert handler rather than a malformed one. */ + #[Test] + public function invalidUtf8DirnameYieldsAnInertHandler(): void + { + $decoded = $this->firstOnclick(AdminBulkPage::renderYesNoRadios("bad\xC3\x28name", 0)); + + self::assertSame('void 0', $decoded); + } + + /** The onclick attribute value as the JavaScript parser would receive it. */ + private function firstOnclick(string $html): string + { + if (1 !== \preg_match('/onclick="([^"]*)"/', $html, $m)) { + self::fail('no onclick attribute found in: ' . $html); + } + + return \html_entity_decode($m[1], \ENT_QUOTES | \ENT_HTML5, 'UTF-8'); + } +} From 2c22d893827e8232b8a89772bf431844b0b97ef2 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 3 Aug 2026 23:37:02 -0400 Subject: [PATCH 12/12] fix(installer): pair span closes so a nested span cannot end an error run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only red spans carry severity, but every span shares one closing tag, and the parser counted depth instead of tracking which span was which. A therefore ended whatever was innermost — so an ordinary span nested inside a red one closed the red one: bad detail
still bad marked "still bad" as Info. Core nests spans routinely, so this was the common case, not a hostile input. The visible cost is larger than a mis-coloured line: dropping the error event also takes the result from Warning back to Success, so a report whose transcript contained failures read as clean. Spans are now a stack of "was this one red", pushed by both the red and the ordinary open marker and popped by each close, so every close ends its own span. A close with nothing open pops nothing, which keeps the existing tolerance for core's unbalanced fragments. Three regression cases: an ordinary span nested in a red one, several ordinary spans nesting and closing across a break, and an ordinary span alone that must colour nothing. The earlier suite missed this because it only ever tested one span at a time. Also makes the absent-module dirname in the logo test random per run rather than a fixed literal, so the case cannot quietly stop testing what it claims the day something on disk takes that name. --- class/Report/LegacyModuleReportAdapter.php | 46 ++++++++++++----- tests/Unit/class/AdminBulkPageLogoUrlTest.php | 14 ++++-- .../Report/LegacyModuleReportAdapterTest.php | 49 +++++++++++++++++++ 3 files changed, 94 insertions(+), 15 deletions(-) diff --git a/class/Report/LegacyModuleReportAdapter.php b/class/Report/LegacyModuleReportAdapter.php index d67cd59..4211fcc 100644 --- a/class/Report/LegacyModuleReportAdapter.php +++ b/class/Report/LegacyModuleReportAdapter.php @@ -94,6 +94,13 @@ private function toMarkedText(string $text): string $m . 'E', $text ); + // Every OTHER span gets an open marker too. It carries no severity, but it + // does carry a , and without a matching open that close would be + // read as ending the red span enclosing it — silently stopping the error + // run and repainting the rest of the transcript as ordinary output. Core + // does nest spans, so this is the common case, not a hostile one. Runs + // AFTER the red-span pass, which has already consumed those tags. + $text = (string) \preg_replace('#]*)?>#i', $m . 'O', $text); $text = (string) \preg_replace('##i', $m . 'e', $text); $text = (string) \preg_replace('#<(?:strong|b|em|i)(?:\s[^>]*)?>#i', $m . 'S', $text); $text = (string) \preg_replace('##i', $m . 's', $text); @@ -114,8 +121,14 @@ private function toMarkedText(string $text): string * still applies to both lines. That reproduces what the string renderer did (one * spanning the break) while producing per-line balanced markup instead, * and core does emit spans that wrap a
. The cost is that an UNCLOSED span - * colours every line after it; the depth counters bottom out at zero rather than - * going negative, so a stray close tag cannot invert the state. + * colours every line after it; a stray close pops nothing rather than going + * negative, so it cannot invert the state. + * + * Spans are tracked as a STACK, not a counter, because only the red ones carry + * severity while all of them share one closing tag. A counter had to guess which + * span a belonged to, and guessed the innermost open one — so an + * ordinary span nested inside a red one closed the red one, ending the error run + * early. The stack records what each open span was, so each close ends its own. * * @return list */ @@ -123,19 +136,20 @@ private function toEvents(string $marked): array { $events = []; $emphasisDepth = 0; - $errorDepth = 0; + /** @var list $spanStack true for a red (error) span, false for an ordinary one */ + $spanStack = []; foreach (\explode("\n", $marked) as $line) { $fragments = []; $buffer = ''; // Seeded from the state carried in, so a line that only CLOSES a span core - // opened before the
still takes the severity. Reading $errorDepth - // after the loop instead would miss it: the closing marker has already - // decremented it back to zero by then. - $lineHadError = $errorDepth > 0; + // opened before the
still takes the severity. Reading the stack after + // the loop instead would miss it: the closing marker has already popped + // that span by then. + $lineHadError = \in_array(true, $spanStack, true); $parts = \preg_split( - '/(' . self::M . '[SsEe])/', + '/(' . self::M . '[SsEeO])/', $line, -1, \PREG_SPLIT_DELIM_CAPTURE | \PREG_SPLIT_NO_EMPTY @@ -154,12 +168,20 @@ private function toEvents(string $marked): array break; case self::M . 'E': - ++$errorDepth; + $spanStack[] = true; $lineHadError = true; + break; + case self::M . 'O': + // Ordinary span: no severity, but it owns the next close. + $spanStack[] = false; + break; case self::M . 'e': - $errorDepth = \max(0, $errorDepth - 1); + // Ends the innermost span, whatever kind it was. A close with + // nothing open pops nothing, which is how a stray from + // core's unbalanced fragments stays harmless. + \array_pop($spanStack); break; default: @@ -168,7 +190,9 @@ private function toEvents(string $marked): array } $this->flushBuffer($buffer, $fragments, $emphasisDepth); - $severity = ($lineHadError || $errorDepth > 0) ? LogSeverity::Error : LogSeverity::Info; + $severity = ($lineHadError || \in_array(true, $spanStack, true)) + ? LogSeverity::Error + : LogSeverity::Info; $event = $this->withIndentExtracted($severity, $fragments); // Blank lines are an artifact of core's unbalanced fragments, diff --git a/tests/Unit/class/AdminBulkPageLogoUrlTest.php b/tests/Unit/class/AdminBulkPageLogoUrlTest.php index fa46b52..6fbe00e 100644 --- a/tests/Unit/class/AdminBulkPageLogoUrlTest.php +++ b/tests/Unit/class/AdminBulkPageLogoUrlTest.php @@ -76,13 +76,19 @@ public function hostileImageValueIsRejected(string $image): void self::assertNull(AdminBulkPage::moduleLogoUrl('quotes', $this->info($image))); } - /** A well-formed request for a module that is not on disk still resolves to null. */ + /** + * A well-formed request for a module that is not on disk still resolves to null. + * + * The suffix is random per run, not a fixed literal: the point of the case is + * "a dirname that certainly does not exist", and a hard-coded name quietly + * stops testing that the day a fixture or a real module happens to use it. + */ #[Test] public function absentModuleDirectoryIsRejected(): void { - self::assertNull( - AdminBulkPage::moduleLogoUrl('no_such_module_' . \bin2hex('x'), $this->info('assets/logo.png')) - ); + $absent = 'no_such_module_' . \bin2hex(\random_bytes(16)); + + self::assertNull(AdminBulkPage::moduleLogoUrl($absent, $this->info('assets/logo.png'))); } /** diff --git a/tests/Unit/class/Report/LegacyModuleReportAdapterTest.php b/tests/Unit/class/Report/LegacyModuleReportAdapterTest.php index 671b195..9ba4886 100644 --- a/tests/Unit/class/Report/LegacyModuleReportAdapterTest.php +++ b/tests/Unit/class/Report/LegacyModuleReportAdapterTest.php @@ -99,6 +99,55 @@ public function unclosedErrorSpanColoursEveryFollowingLine(): void self::assertSame(LogSeverity::Error, $events[2]->severity); } + /** + * Core nests spans, and only the red one carries severity. Since every span + * shares one closing tag, an inner ordinary span's must not be read + * as closing the red span around it — that silently ends the error run and + * repaints the rest of the transcript as ordinary output, which also drops + * the result from Warning back to Success. + */ + #[Test] + public function nestedOrdinarySpanDoesNotCloseTheEnclosingErrorSpan(): void + { + $events = (new LegacyModuleReportAdapter())->parse( + 'bad detail
still bad' + ); + + self::assertSame(['bad detail', 'still bad'], self::texts($events)); + self::assertSame(LogSeverity::Error, $events[0]->severity); + self::assertSame( + LogSeverity::Error, + $events[1]->severity, + 'the red span is still open on line 2; the inner span closed itself' + ); + } + + /** Several ordinary spans, and one closing after the break, still pair correctly. */ + #[Test] + public function deeplyNestedOrdinarySpansPairWithThemselves(): void + { + $events = (new LegacyModuleReportAdapter())->parse( + 'ab
cd' + ); + + self::assertSame(['ab', 'cd'], self::texts($events)); + self::assertSame(LogSeverity::Error, $events[0]->severity); + // The red span closes after "c", so "d" — outside it — is ordinary. Both + // facts have to hold at once for the pairing to be right rather than lucky. + self::assertSame(LogSeverity::Error, $events[1]->severity); + } + + /** An ordinary span opened before a break must not colour anything. */ + #[Test] + public function ordinarySpanAloneNeverProducesAnError(): void + { + $events = (new LegacyModuleReportAdapter())->parse('plain
also plain'); + + self::assertSame(['plain', 'also plain'], self::texts($events)); + self::assertSame(LogSeverity::Info, $events[0]->severity); + self::assertSame(LogSeverity::Info, $events[1]->severity); + } + /** A stray close tag must not drive the counter negative and invert the state. */ #[Test] public function strayClosingSpanDoesNotInvertSeverity(): void