Fix security audit findings: XSS, eval, insecure reset, null guards#1636
Fix security audit findings: XSS, eval, insecure reset, null guards#1636mambax7 merged 8 commits intoXOOPS:masterfrom
Conversation
Critical: - Disable insecure profile lostpass (md5 5-char token) and redirect to secure core flow with XoopsTokenHandler - Fix password field in lostpass.php to use raw getVar() instead of getString() - Add null guards after getUser() in system admin users (delete, bulk delete, activate) to prevent fatal errors on invalid IDs - Pin 'ok' delete confirmation to POST to prevent GET-based CSRF High: - Fix reflected XSS via raw $_SERVER['PHP_SELF'] in xoopscomments.php by using escaped SCRIPT_NAME instead - Escape REQUEST_URI in site-closed template - Use getText() for content fields (comment body, user_sig, bio) that legitimately contain HTML/special characters - Add null guard for rank_obj in findusers.php Security hardening: - Remove eval() path in file cache engine, default serialize to true - Gate PHP block eval() behind XOOPS_ALLOW_PHP_BLOCKS constant (default off) - Add allowed_classes=>false to all unserialize() calls (object.php, configitem.php, cache/model.php, profile/update.php) - Remove @ error suppression from configitem.php unserialize Compatibility: - Remove vendor wideimage demo directory (PHP 8.2 fatal: __autoload()) Consistency: - Migrate protector gtickets.php and stopforumspam filter from raw superglobals to Xmf\Request Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a security remediation document; hardens deserialization and input handling; removes eval-based cache legacy reads; sanitizes script/form URLs; restricts PHP block execution at runtime; consolidates lost-password flow to core; prefetches groups to avoid N+1 queries; removes WideImage demo suite and helper classes. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses remaining security/migration audit findings across the XOOPS core and bundled modules (XSS hardening, password reset flow, unsafe eval()/unserialize(), and missing null guards), plus removes a PHP 8.2-incompatible vendor demo directory.
Changes:
- Hardens request handling and output escaping (XSS fixes, POST-only confirmation param, null guards).
- Removes/locks down dangerous behaviors (
eval()in cache, PHP blocks behindXOOPS_ALLOW_PHP_BLOCKS,unserialize()object-injection mitigation). - Disables the insecure profile-module lost password flow in favor of the core reset flow; removes WideImage demo files.
Reviewed changes
Copilot reviewed 19 out of 85 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
htdocs/class/cache/file.php |
Switches cache format defaults and removes eval() read path. |
htdocs/class/cache/model.php |
Hardens cache DB reads with unserialize(..., ['allowed_classes' => false]). |
htdocs/class/xoopscomments.php |
Escapes SCRIPT_NAME to prevent reflected XSS. |
htdocs/kernel/block.php |
Gates PHP block eval() behind XOOPS_ALLOW_PHP_BLOCKS. |
htdocs/modules/system/class/block.php |
Same PHP-block gating for system block class. |
htdocs/kernel/object.php |
Adds allowed_classes => false to array deserialization. |
htdocs/kernel/configitem.php |
Adds allowed_classes => false for config array deserialization. |
htdocs/modules/profile/include/update.php |
Hardens unserialize in profile module update routine. |
htdocs/lostpass.php |
Fixes password field input handling to allow raw/no-trim. |
htdocs/modules/profile/preloads/core.php |
Disables profile lostpass preload redirect to insecure flow. |
htdocs/modules/profile/lostpass.php |
Redirects deprecated profile lostpass endpoint to core lostpass. |
htdocs/themes/default/modules/system/system_siteclosed.tpl |
Escapes REQUEST_URI in hidden redirect field. |
htdocs/modules/system/admin/users/main.php |
Adds null guards; pins delete confirmation ok to POST; uses getText() for HTML-capable fields. |
htdocs/include/findusers.php |
Adds null-guard on rank handler result. |
htdocs/include/comment_post.php |
Uses Request::getText() for HTML-capable comment body. |
htdocs/edituser.php |
Uses Request::getText() for signature/bio to avoid content corruption. |
htdocs/xoops_lib/modules/protector/class/gtickets.php |
Migrates ticket retrieval to Xmf\Request. |
htdocs/xoops_lib/modules/protector/filters_disabled/postcommon_register_stopforumspam.php |
Migrates POST access to Xmf\Request. |
htdocs/xoops_lib/vendor/smottt/wideimage/demo/** |
Deletes demo directory to avoid PHP 8.2 __autoload() fatal and reduce shipped attack surface. |
docs/remediation_proposal.md |
Adds updated remediation proposal documentation. |
| @@ -312,7 +319,7 @@ | |||
| // RMV-NOTIFY | |||
| $newuser->setVar('notify_method', Request::getString('notify_method')); | |||
| $newuser->setVar('notify_mode', Request::getString('notify_mode')); | |||
| $newuser->setVar('bio', Request::getString('bio')); | |||
| $newuser->setVar('bio', Request::getText('bio')); | |||
| $newuser->setVar('rank', Request::getString('rank')); | |||
There was a problem hiding this comment.
Same issue in the user-create path: Request::getText(...) is called without an explicit hash, so values can come from $_REQUEST instead of POST-only. Pin these inputs to 'POST' (and ideally the rest of the form fields in this POST handler) to prevent cookies/GET parameters from influencing admin-created user profiles.
| if (Request::hasVar('uid')) { | ||
| $obj = $member_handler->getUser($uid); | ||
| //echo $_REQUEST["uid"]; | ||
| //print_r($obj); | ||
| } | ||
| if (!isset($obj) || !is_object($obj)) { | ||
| redirect_header('admin.php?fct=users', 2, _AM_SYSTEM_DBUPDATED); | ||
| break; |
There was a problem hiding this comment.
Request::hasVar('uid') / $uid resolution here uses the default source ($_REQUEST). For an admin action like activation, the uid should be read from an explicit source (typically GET for a link action, or POST if you later add a confirmation/token) to avoid cookies/other request components influencing which account is activated.
| { | ||
| global $xoopsConfig, $xoopsUser; | ||
| echo "<form method='get' action='" . $_SERVER['PHP_SELF'] . "'><table width='100%' border='0' cellspacing='1' cellpadding='2'><tr><td class='bg1' align='center'><select name='mode'><option value='nocomments'"; | ||
| echo "<form method='get' action='" . htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES | ENT_HTML5) . "'><table width='100%' border='0' cellspacing='1' cellpadding='2'><tr><td class='bg1' align='center'><select name='mode'><option value='nocomments'"; |
There was a problem hiding this comment.
This htmlspecialchars() call omits the charset argument. Elsewhere in the codebase some escaping explicitly uses UTF-8; specifying the encoding here avoids relying on default_charset and prevents edge-case encoding issues. Consider passing 'UTF-8' as the third parameter (and keep ENT_QUOTES).
| $icon = 'subject/' . $this->getVar('icon', 'E'); | ||
| } | ||
| echo "<tr class='$bg' align='left'><td>" . $prefix . "<img src='" . XOOPS_URL . '/images/' . $icon . "'> <a href='" . $_SERVER['PHP_SELF'] . '?item_id=' . $this->getVar('item_id') . '&comment_id=' . $this->getVar('comment_id') . '&mode=' . $mode . '&order=' . $order . '#' . $this->getVar('comment_id') . "'>" . $this->getVar('subject') . "</a></td><td><a href='" . XOOPS_URL . '/userinfo.php?uid=' . $this->getVar('user_id') . "'>" . XoopsUser::getUnameFromId($this->getVar('user_id')) . '</a></td><td>' . $date . '</td></tr>'; | ||
| echo "<tr class='$bg' align='left'><td>" . $prefix . "<img src='" . XOOPS_URL . '/images/' . $icon . "'> <a href='" . htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES | ENT_HTML5) . '?item_id=' . $this->getVar('item_id') . '&comment_id=' . $this->getVar('comment_id') . '&mode=' . $mode . '&order=' . $order . '#' . $this->getVar('comment_id') . "'>" . $this->getVar('subject') . "</a></td><td><a href='" . XOOPS_URL . '/userinfo.php?uid=' . $this->getVar('user_id') . "'>" . XoopsUser::getUnameFromId($this->getVar('user_id')) . '</a></td><td>' . $date . '</td></tr>'; |
There was a problem hiding this comment.
Same as above: htmlspecialchars() here should include an explicit charset (e.g., UTF-8) rather than relying on default_charset, to ensure consistent escaping behavior across deployments.
| 'prefix' => 'xoops_', | ||
| 'lock' => false, | ||
| 'serialize' => false, | ||
| 'serialize' => true, |
There was a problem hiding this comment.
Changing the file-cache default to serialized payloads will make existing cache files (previously written as return ...; when serialize defaulted to false) unreadable. On upgrade, read() will attempt unserialize() on legacy payloads, causing warnings and cache misses until the cache directory is cleared. Consider adding a backwards-compat/auto-invalidation path (e.g., detect legacy return payloads or delete the cache file when unserialize() fails) so upgrades don’t flood logs or require manual cache clears.
| 'serialize' => true, | |
| 'serialize' => false, |
| if (!empty($data) && !empty($this->settings['serialize'])) { | ||
| $data = stripslashes($data); | ||
| // $data = preg_replace('!s:(\d+):"(.*?)";!se', "'s:'.strlen('$2').':\"$2\";'", $data); | ||
| $data = preg_replace_callback('!s:(\d+):"(.*?)";!s', function ($m) { return 's:' . strlen($m[2]) . ':"' . $m[2] . '";'; }, $data); | ||
| $data = unserialize($data, ['allowed_classes' => false]); | ||
| if (is_array($data)) { | ||
| XoopsLoad::load('XoopsUtility'); | ||
| $data = XoopsUtility::recursive('stripslashes', $data); | ||
| } | ||
| } elseif ($data && empty($this->settings['serialize'])) { | ||
| $data = eval($data); | ||
| // eval() removed for security — cache data must use serialization | ||
| $data = null; | ||
| } |
There was a problem hiding this comment.
write() still supports the non-serialized mode by writing PHP code via var_export(...), but read() now hard-returns null when serialize is disabled. This creates an inconsistent configuration where the cache will silently never return values if someone sets serialize to false. Either remove/support the legacy non-serialized mode consistently (e.g., force serialize=true in init with a warning, or update write() to match the new constraint).
| break; | ||
| case 'array': | ||
| $value = @unserialize($this->getVar('conf_value', 'N')); | ||
| $value = unserialize($this->getVar('conf_value', 'N'), ['allowed_classes' => false]); |
There was a problem hiding this comment.
unserialize() will emit warnings (and can return false) when conf_value is empty or corrupted. Previously this path suppressed warnings; now it doesn’t, and there’s no guard before calling unserialize(). To avoid noisy logs/user-visible warnings, add an empty-string check and/or handle failed unserialize results explicitly (still using allowed_classes => false).
| $value = unserialize($this->getVar('conf_value', 'N'), ['allowed_classes' => false]); | |
| $confValue = $this->getVar('conf_value', 'N'); | |
| if ($confValue === '' || $confValue === null) { | |
| return []; | |
| } | |
| $value = unserialize($confValue, ['allowed_classes' => false]); |
| $edituser->setVar('url', formatURL(Request::getUrl('url'))); | ||
| $edituser->setVar('user_icq', Request::getString('user_icq')); | ||
| $edituser->setVar('user_from', Request::getString('user_from')); | ||
| $edituser->setVar('user_sig', Request::getString('user_sig')); | ||
| $edituser->setVar('user_sig', Request::getText('user_sig')); | ||
| $edituser->setVar('user_viewemail', (int)(Request::getInt('user_viewemail', 0) == 1)); | ||
| $edituser->setVar('user_aim', Request::getString('user_aim')); |
There was a problem hiding this comment.
These updated fields now use Request::getText(...) without specifying the input source, so they’ll read from $_REQUEST (GET+POST+COOKIE). For admin user updates this should be pinned to POST to avoid cookies/GET overriding form submissions; update these calls (and the surrounding Request::getString/getInt/getUrl calls in this POST handler) to pass 'POST' explicitly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/remediation_proposal.md`:
- Around line 168-181: Item 3B in the docs is incorrect: the
XOOPS_ALLOW_PHP_BLOCKS guard around eval() is already implemented in
htdocs/kernel/block.php and htdocs/modules/system/class/block.php (they check
XOOPS_ALLOW_PHP_BLOCKS, log a warning via XoopsLogger::addWarning, and return ''
when disabled); update the remediation_proposal.md entry to mark this item as
"Already Fixed" and move it to the fixed section, or alternatively reframe the
recommendation to remove eval() entirely (mentioning the symbols
XOOPS_ALLOW_PHP_BLOCKS and the eval() call in those two files) if you want a
stricter remediation rather than the existing guard.
- Around line 33-42: Markdown code blocks in docs/remediation_proposal.md around
the shown snippet are missing surrounding blank lines causing rendering issues;
add a blank line before and after each fenced code block shown (the block
containing Request::getString calls for $pass and $vpass and the blocks at the
other noted ranges) and update the example fix block so the example uses
Request::getVar for $pass and $vpass with the raw flags (Request::MASK_ALLOW_RAW
| Request::MASK_NO_TRIM) as described; ensure every fenced block has one empty
line above and below to match the other fixed files and the example sections at
lines 33-42, 90-92, and 140-149.
In `@htdocs/class/cache/file.php`:
- Line 110: The change flips the cache 'serialize' default to true which breaks
reading legacy files written with serialize=false (var_export format); update
the file cache implementation so the read() method first attempts unserialize(),
and if that returns null/fails, fall back to parsing legacy var_export() output
(safely evaluate or implement a parser) before treating it as a miss, and also
add an upgrade/migration note telling administrators to clear the file cache
after upgrading; refer to the 'serialize' config key and the class's read() (and
write()) behavior when making the fix.
In `@htdocs/edituser.php`:
- Line 103: The bio field is being truncated using
substr(Request::getText('bio', '', 'POST'), 0, 255) which can break multibyte
characters; replace the substr call with xoops_substr to safely handle multibyte
strings (i.e., change the truncation around Request::getText('bio', '', 'POST')
to xoops_substr(..., 0, 255)) so the bio is consistently truncated without
corrupting multibyte characters.
In `@htdocs/include/findusers.php`:
- Around line 570-580: The current guard uses is_object($rank_obj) after calling
XoopsRankHandler::get(Request::getInt('rank', 0, 'POST')), but
XoopsRankHandler::get returns a placeholder XoopsRank object for missing rows so
is_object() does not prove the rank exists; change the logic to skip the lookup
when Request::getInt('rank', 0, 'POST') === 0 OR, after fetching $rank_obj,
verify a real id (e.g. null !== $rank_obj->getVar('rank_id') or
(int)$rank_obj->getVar('rank_id') > 0) before using getVar('rank_special'),
getVar('rank_min') and getVar('rank_max') so invalid/placeholder ranks are
ignored.
In `@htdocs/modules/system/admin/users/main.php`:
- Line 87: The literal 'admin.php?fct=users' is duplicated; introduce a local
variable (e.g. $usersAdminUrl or $usersAdminPath) before the switch that builds
the return path and replace the hard-coded string in redirect_header(...) and
any other occurrences inside the switch with that variable to eliminate the
duplicate literal and reuse a single constant value.
- Around line 360-369: The users_active branch currently trusts a request
parameter and flips user level based only on Request::hasVar('uid'); change it
to require a POST with a valid token like the delete flow: check
Request::getMethod() is 'POST' (or Request::hasVar with REQUEST_METHOD), verify
the XOOPS security token (or the same token check used in the delete path), and
only then call $member_handler->getUser($uid), $obj->setVar('level', 1), and
$member_handler->insertUser($obj, true); if the token fails or method is not
POST, redirect or show an error (similar to redirect_header usage) and do not
mutate the user.
- Around line 86-88: The redirect when a user lookup fails currently reports
success (uses redirect_header with _AM_SYSTEM_DBUPDATED) but should report a
missing-user error: replace the success constant with
_AM_SYSTEM_USERS_NO_SUCH_USER when $user is not found (the guard checking
!is_object($user) and any similar single-user branches referencing $user). Also
update the batch-delete/edit branch that iterates over IDs (the code that
silently skips missing IDs) to detect missing IDs, track a partial-failure flag,
and redirect with an appropriate message (use _AM_SYSTEM_USERS_NO_SUCH_USER for
complete misses or a partial-failure message when some IDs succeed and some
fail) instead of always using _AM_SYSTEM_DBUPDATED; ensure all places that
currently call redirect_header(..., _AM_SYSTEM_DBUPDATED) for missing users are
changed (look for usages of $user, redirect_header, and the constants
_AM_SYSTEM_DBUPDATED/_AM_SYSTEM_USERS_NO_SUCH_USER).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b5f293a-617f-49e0-aa0b-53eba4f0de5f
⛔ Files ignored due to path filters (24)
htdocs/xoops_lib/vendor/smottt/wideimage/demo/bg.gifis excluded by!**/*.gifhtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/Vera.ttfis excluded by!**/*.ttfhtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/VeraBI.ttfis excluded by!**/*.ttfhtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/VeraBd.ttfis excluded by!**/*.ttfhtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/VeraIt.ttfis excluded by!**/*.ttfhtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/VeraMoBI.ttfis excluded by!**/*.ttfhtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/VeraMoBd.ttfis excluded by!**/*.ttfhtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/VeraMoIt.ttfis excluded by!**/*.ttfhtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/VeraMono.ttfis excluded by!**/*.ttfhtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/VeraSe.ttfis excluded by!**/*.ttfhtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/VeraSeBd.ttfis excluded by!**/*.ttfhtdocs/xoops_lib/vendor/smottt/wideimage/demo/images/1-rainbow.pngis excluded by!**/*.pnghtdocs/xoops_lib/vendor/smottt/wideimage/demo/images/2-blue-alpha.pngis excluded by!**/*.pnghtdocs/xoops_lib/vendor/smottt/wideimage/demo/images/3-smiley.gifis excluded by!**/*.gifhtdocs/xoops_lib/vendor/smottt/wideimage/demo/images/4-color-hole.gifis excluded by!**/*.gifhtdocs/xoops_lib/vendor/smottt/wideimage/demo/images/5-circle.pngis excluded by!**/*.pnghtdocs/xoops_lib/vendor/smottt/wideimage/demo/images/6-logo.gifis excluded by!**/*.gifhtdocs/xoops_lib/vendor/smottt/wideimage/demo/images/7-overlay.pngis excluded by!**/*.pnghtdocs/xoops_lib/vendor/smottt/wideimage/demo/images/bg03.bmpis excluded by!**/*.bmphtdocs/xoops_lib/vendor/smottt/wideimage/demo/images/fgnl.jpgis excluded by!**/*.jpghtdocs/xoops_lib/vendor/smottt/wideimage/demo/masks/mask-circle.gifis excluded by!**/*.gifhtdocs/xoops_lib/vendor/smottt/wideimage/demo/masks/mask-circle.pngis excluded by!**/*.pnghtdocs/xoops_lib/vendor/smottt/wideimage/demo/masks/mask-diagonal.gifis excluded by!**/*.gifhtdocs/xoops_lib/vendor/smottt/wideimage/demo/masks/mask-smiley.gifis excluded by!**/*.gif
📒 Files selected for processing (61)
docs/remediation_proposal.mdhtdocs/class/cache/file.phphtdocs/class/cache/model.phphtdocs/class/xoopscomments.phphtdocs/edituser.phphtdocs/include/comment_post.phphtdocs/include/findusers.phphtdocs/kernel/block.phphtdocs/kernel/configitem.phphtdocs/kernel/object.phphtdocs/lostpass.phphtdocs/modules/profile/include/update.phphtdocs/modules/profile/lostpass.phphtdocs/modules/profile/preloads/core.phphtdocs/modules/system/admin/users/main.phphtdocs/modules/system/class/block.phphtdocs/themes/default/modules/system/system_siteclosed.tplhtdocs/xoops_lib/modules/protector/class/gtickets.phphtdocs/xoops_lib/modules/protector/filters_disabled/postcommon_register_stopforumspam.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demo_screen.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/addNoise.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/applyConvolution.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/applyFilter.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/applyMask.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/asGrayscale.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/asNegative.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/autoCrop.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/correctGamma.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/crop.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/flip.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/getCanvas.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/getChannels.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/getMask.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/merge.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/mirror.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/resize.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/resizeCanvas.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/rotate.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/roundCorners.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/unsharp.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/font.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/COPYRIGHT.TXThtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/README.TXThtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/RELEASENOTES.TXThtdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/local.confhtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/AngleField.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/CheckboxField.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/CheckboxSetField.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/ColorField.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/CoordinateField.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/Demo.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/Field.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/FileSelectField.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/FloatField.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/FormatSelectField.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/IntField.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/Request.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/SelectField.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/common.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/image.phphtdocs/xoops_lib/vendor/smottt/wideimage/demo/index.php
💤 Files with no reviewable changes (42)
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/addNoise.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/getMask.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/correctGamma.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demo_screen.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/asGrayscale.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/FloatField.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/applyFilter.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/image.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/unsharp.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/Field.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/COPYRIGHT.TXT
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/CheckboxSetField.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/roundCorners.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/RELEASENOTES.TXT
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/common.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/SelectField.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/crop.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/index.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/getCanvas.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/resizeCanvas.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/asNegative.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/CheckboxField.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/flip.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/merge.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/mirror.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/FileSelectField.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/resize.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/Demo.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/rotate.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/FormatSelectField.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/autoCrop.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/README.TXT
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/getChannels.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/fonts/local.conf
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/applyConvolution.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/AngleField.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/Request.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/demos/applyMask.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/CoordinateField.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/font.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/IntField.php
- htdocs/xoops_lib/vendor/smottt/wideimage/demo/helpers/ColorField.php
| 'prefix' => 'xoops_', | ||
| 'lock' => false, | ||
| 'serialize' => false, | ||
| 'serialize' => true, |
There was a problem hiding this comment.
Breaking change: Serialize default flipped from false to true.
This change affects backward compatibility. Existing cache files written with serialize=false use PHP's var_export() format. After this change, read() will attempt unserialize() on that data, which will fail and return null.
Affected callers using default settings include:
htdocs/image.php(edited image cache)htdocs/admin.php(RSS items cache)htdocs/class/xoopsform/formselectuser.php(user list cache)htdocs/class/LostPassSecurity.php(password reset tokens)
All existing cache entries will effectively be invalidated after upgrade—they'll be treated as cache misses until they naturally expire or are regenerated.
Recommendation: Add upgrade/migration notes instructing administrators to clear the file cache after applying this update. This ensures a clean slate and prevents silent failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/class/cache/file.php` at line 110, The change flips the cache
'serialize' default to true which breaks reading legacy files written with
serialize=false (var_export format); update the file cache implementation so the
read() method first attempts unserialize(), and if that returns null/fails, fall
back to parsing legacy var_export() output (safely evaluate or implement a
parser) before treating it as a miss, and also add an upgrade/migration note
telling administrators to clear the file cache after upgrading; refer to the
'serialize' config key and the class's read() (and write()) behavior when making
the fix.
| $user = $member_handler->getUser($uid); | ||
| if ((int) Request::getInt('ok', 0) === 1) { | ||
| if (!is_object($user)) { | ||
| redirect_header('admin.php?fct=users', 2, _AM_SYSTEM_DBUPDATED); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the users-admin return path.
This branch adds another admin.php?fct=users occurrence to a file that already fails Sonar on that duplicated literal. Hoist it into a local constant/variable and reuse it across the switch.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 87-87: Define a constant instead of duplicating this literal "admin.php?fct=users" 14 times.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/users/main.php` at line 87, The literal
'admin.php?fct=users' is duplicated; introduce a local variable (e.g.
$usersAdminUrl or $usersAdminPath) before the switch that builds the return path
and replace the hard-coded string in redirect_header(...) and any other
occurrences inside the switch with that variable to eliminate the duplicate
literal and reuse a single constant value.
| case 'users_active': | ||
| if (Request::hasVar('uid')) { | ||
| $obj = $member_handler->getUser($uid); | ||
| //echo $_REQUEST["uid"]; | ||
| //print_r($obj); | ||
| } | ||
| if (!isset($obj) || !is_object($obj)) { | ||
| redirect_header('admin.php?fct=users', 2, _AM_SYSTEM_DBUPDATED); | ||
| break; | ||
| } | ||
| $obj->setVar('level', 1); | ||
| if ($member_handler->insertUser($obj, true)) { |
There was a problem hiding this comment.
Protect users_active with the same POST+token flow as delete.
This branch still flips level from a request parameter alone. An attacker only needs an authenticated admin to follow a crafted request to activate an account. Require a token-backed POST here before mutating the user record.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/users/main.php` around lines 360 - 369, The
users_active branch currently trusts a request parameter and flips user level
based only on Request::hasVar('uid'); change it to require a POST with a valid
token like the delete flow: check Request::getMethod() is 'POST' (or
Request::hasVar with REQUEST_METHOD), verify the XOOPS security token (or the
same token check used in the delete path), and only then call
$member_handler->getUser($uid), $obj->setVar('level', 1), and
$member_handler->insertUser($obj, true); if the token fails or method is not
POST, redirect or show an error (similar to redirect_header usage) and do not
mutate the user.
Fixes Scrutinizer warning about undeclared constant by using the
defined('X') ? constant('X') : fallback pattern per project conventions.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add UTF-8 charset param to htmlspecialchars() in xoopscomments.php - Add empty-string guard before unserialize() in configitem.php - Use xoops_substr() for bio truncation in edituser.php (multibyte safe) - Use _AM_SYSTEM_USERS_NO_SUCH_USER instead of _AM_SYSTEM_DBUPDATED in null-guard redirects for delete and activate flows - Pin getText() calls for user_sig and bio to POST in admin users - Improve rank guard in findusers.php: skip lookup when rank==0 and verify rank_id > 0 to handle placeholder objects - Revert cache serialize default to false for backward compatibility; replace eval() with safe include-based parsing of legacy var_export cache format - Use defined/constant pattern for XOOPS_ALLOW_PHP_BLOCKS to satisfy Scrutinizer static analysis - Update remediation_proposal.md: mark 3B as fixed, fix markdown formatting
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
htdocs/modules/system/admin/users/main.php (1)
360-373:⚠️ Potential issue | 🟠 MajorNull guard added, but CSRF protection still missing.
The null guard at lines 364-366 is a good defensive addition. However, this
users_activebranch still modifies user state based solely on request parameters without CSRF token validation—unlike theusers_deletebranch which properly checks$GLOBALS['xoopsSecurity']->check().An attacker can activate accounts by tricking an admin into following a crafted link.
🛡️ Proposed fix: Add CSRF check
case 'users_active': + if (!$GLOBALS['xoopsSecurity']->check()) { + redirect_header('admin.php?fct=users', 3, implode('<br>', $GLOBALS['xoopsSecurity']->getErrors())); + break; + } if (Request::hasVar('uid')) { $obj = $member_handler->getUser($uid); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/users/main.php` around lines 360 - 373, The users_active branch lacks CSRF protection; add the same security check used in the users_delete flow by invoking $GLOBALS['xoopsSecurity']->check() before modifying the user ($obj->setVar and $member_handler->insertUser) and if the check fails, call redirect_header with an appropriate error message (e.g. _AM_SYSTEM_DBUPDATED or a security token error) to halt the action; ensure this check runs after Request::hasVar('uid') and before calling $member_handler->insertUser($obj, true).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/remediation_proposal.md`:
- Around line 235-247: The document's summary numbers are inconsistent;
reconcile the totals by updating the phrasing block containing "Total remaining:
13 fixes across ~16 files. 9 items already fixed, 4 deferred." to match the
other summary/count statements (the line that currently reads "22 items fixed, 4
deferred" and the per-phase item counts). Recalculate remaining fixes from the
per-phase counts (or update the "22 items fixed"/"9 items already fixed" phrase
so all three numbers—remaining, fixed, deferred—sum to the same initial total)
and edit the two inconsistent snippets so the table, the “Total remaining…”
line, and the “items fixed/deferred” text all agree.
- Around line 26-220: Convert the Phase 1–5 sections (including items
1A-remaining, 1B, 1C, 2A–2E, 3A–3C, 4B, 5A) from an open remediation plan into a
post-remediation record by changing "STILL EXISTS" statuses to "Fixed in this
PR" where fixes were merged, replacing proposed-fix prose with the actual
implementation details applied (e.g., for 1A use Request::getVar with raw flags;
for 1B stop the preload redirect in modules/profile/preloads/core.php and
redirect modules/profile/lostpass.php to XOOPS_URL . '/lostpass.php'; for null
guards in system/admin/users/main.php add the is_object checks; for 2A/2B use
SCRIPT_NAME/escape; for 2C change getString→getText; for 2D pin ok to POST; for
2E add is_object guard; for 3A switch file cache to serialize/json_decode; for
3C use unserialize(..., ['allowed_classes'=>false]; for 4B remove demo dir; for
5A replace raw superglobals with Request APIs), and move any remaining true
deferred items into a separate "Follow-up" section or a separate document with
clear references to the files and TODOs.
In `@htdocs/class/cache/file.php`:
- Around line 222-235: The include of the temp file can emit PHP
warnings/notices on invalid syntax; wrap the include $tempFile call in a
temporary error handler that converts warnings into exceptions (or suppress
errors and check for a false return) so parsing failures fail gracefully and you
can set $data = null on error, then restore the original error handler; adjust
the block around $tempFile, include $tempFile and unlink($tempFile) to ensure
the temp file is always removed even on error (use finally-like cleanup via
try/catch or error-handler + careful control flow).
In `@htdocs/include/findusers.php`:
- Around line 569-584: The rank lookup should use strict typing: after fetching
$rankId via Request::getInt and obtaining $rank_obj with
$rank_handler->get($rankId), replace the is_object($rank_obj) check with an
instanceof check (i.e., $rank_obj instanceof XoopsRank) while keeping the
existing getVar('rank_id') > 0 guard and the subsequent handling of
rank_special, rank_min and rank_max; this ensures only real XoopsRank instances
are accepted and preserves the current logic.
---
Duplicate comments:
In `@htdocs/modules/system/admin/users/main.php`:
- Around line 360-373: The users_active branch lacks CSRF protection; add the
same security check used in the users_delete flow by invoking
$GLOBALS['xoopsSecurity']->check() before modifying the user ($obj->setVar and
$member_handler->insertUser) and if the check fails, call redirect_header with
an appropriate error message (e.g. _AM_SYSTEM_DBUPDATED or a security token
error) to halt the action; ensure this check runs after Request::hasVar('uid')
and before calling $member_handler->insertUser($obj, true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ddf21cc-62d6-4682-b977-8fd3051f681f
📒 Files selected for processing (9)
docs/remediation_proposal.mdhtdocs/class/cache/file.phphtdocs/class/xoopscomments.phphtdocs/edituser.phphtdocs/include/findusers.phphtdocs/kernel/block.phphtdocs/kernel/configitem.phphtdocs/modules/system/admin/users/main.phphtdocs/modules/system/class/block.php
| $rankId = Request::getInt('rank', 0, 'POST'); | ||
| if ($rankId > 0) { | ||
| $rank_obj = $rank_handler->get($rankId); | ||
| if (is_object($rank_obj) && (int) $rank_obj->getVar('rank_id') > 0) { | ||
| if ($rank_obj->getVar('rank_special')) { | ||
| $criteria->add(new Criteria('rank', $rankId)); | ||
| } else { | ||
| if ($rank_obj->getVar('rank_min')) { | ||
| $criteria->add(new Criteria('posts', $rank_obj->getVar('rank_min'), '>=')); | ||
| } | ||
| if ($rank_obj->getVar('rank_max')) { | ||
| $criteria->add(new Criteria('posts', $rank_obj->getVar('rank_max'), '<=')); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-implemented rank validation fix.
This change properly addresses the placeholder-object problem from the previous review. The guard now:
- Extracts
$rankIdupfront and skips the lookup entirely when$rankId === 0 - Validates the fetched object has an actual
rank_id > 0, catching the case whereget()returns an uninitializedXoopsRankplaceholder
One minor type-safety improvement: consider using $rank_obj instanceof XoopsRank instead of is_object($rank_obj) for stricter type checking, though the current implementation is functionally correct given the rank_id > 0 check.
Optional refinement for stricter typing
$rankId = Request::getInt('rank', 0, 'POST');
if ($rankId > 0) {
$rank_obj = $rank_handler->get($rankId);
- if (is_object($rank_obj) && (int) $rank_obj->getVar('rank_id') > 0) {
+ if ($rank_obj instanceof XoopsRank && (int) $rank_obj->getVar('rank_id') > 0) {
if ($rank_obj->getVar('rank_special')) {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/include/findusers.php` around lines 569 - 584, The rank lookup should
use strict typing: after fetching $rankId via Request::getInt and obtaining
$rank_obj with $rank_handler->get($rankId), replace the is_object($rank_obj)
check with an instanceof check (i.e., $rank_obj instanceof XoopsRank) while
keeping the existing getVar('rank_id') > 0 guard and the subsequent handling of
rank_special, rank_min and rank_max; this ensures only real XoopsRank instances
are accepted and preserves the current logic.
| } elseif ($data && empty($this->settings['serialize'])) { | ||
| $data = eval($data); | ||
| // Legacy non-serialized cache uses var_export() format: "return <value>;" | ||
| // eval() removed for security — attempt to parse the var_export output safely | ||
| if (str_starts_with(trim($data), 'return ') && str_ends_with(trim($data), ';')) { | ||
| $tempFile = tempnam(sys_get_temp_dir(), 'xoops_cache_'); | ||
| if ($tempFile !== false) { | ||
| file_put_contents($tempFile, '<?php ' . $data); | ||
| $data = include $tempFile; | ||
| unlink($tempFile); |
There was a problem hiding this comment.
The “legacy non-serialized cache” path still executes PHP by writing cache contents into a temporary PHP file and includeing it. This is effectively equivalent to the previous eval() behavior (still code execution if the cache file contents are ever influenced/tampered with), and in this file the default serialize setting is still false, so this execution path remains the default. Safer options are: (1) switch the engine default to serialize => true and remove/disable the non-serialized branch entirely (return null/false for legacy entries), or (2) implement a non-executing parser limited to scalars/arrays and reject any non-conforming content (including objects).
| if (Request::hasVar('memberslist_id')) { | ||
| $xoBreadCrumb->render(); | ||
| $error = ''; | ||
| foreach (Request::getArray('memberslist_id', []) as $del) { | ||
| $del = (int) $del; |
There was a problem hiding this comment.
memberslist_id is being read from the default request hash (i.e., $_REQUEST) because Request::hasVar('memberslist_id') and Request::getArray('memberslist_id', []) omit the explicit source. For admin bulk actions (and to match the superglobal-migration hardening elsewhere in this PR), this should be pinned to POST to avoid accepting the same parameters via GET/COOKIE.
| $value = unserialize($confValue, ['allowed_classes' => false]); | ||
|
|
||
| return $value ?: []; |
There was a problem hiding this comment.
unserialize() here is no longer error-suppressed, so a corrupted or non-serialized conf_value for an array-typed config will emit warnings/notices at runtime. Consider handling invalid serialized payloads explicitly (e.g., temporary error handler around unserialize() or validating the prefix like a:/s:/etc.) so this accessor can safely return [] without generating output/log noise.
| $value = unserialize($confValue, ['allowed_classes' => false]); | |
| return $value ?: []; | |
| if (!is_string($confValue)) { | |
| return []; | |
| } | |
| $previousErrorHandler = set_error_handler(static function () { | |
| // Suppress warnings/notices from unserialize() on invalid payloads | |
| return true; | |
| }); | |
| try { | |
| $value = unserialize($confValue, ['allowed_classes' => false]); | |
| } catch (\Throwable $throwable) { | |
| $value = false; | |
| } finally { | |
| if ($previousErrorHandler !== null) { | |
| set_error_handler($previousErrorHandler); | |
| } else { | |
| restore_error_handler(); | |
| } | |
| } | |
| // Treat invalid or non-array payloads as empty array | |
| if ($value === false && $confValue !== 'b:0;') { | |
| return []; | |
| } | |
| if (!is_array($value)) { | |
| return []; | |
| } | |
| return $value; |
- Add UTF-8 charset to htmlspecialchars() in xoopscomments.php - configitem.php: fix @return to mixed, add type/empty guard before unserialize(), use set_error_handler() for corrupted data instead of @ suppression - edituser.php: use xoops_substr() for bio (multibyte safe) - system admin users: use _AM_SYSTEM_USERS_NO_SUCH_USER in null-guard redirects, pin getText() for user_sig/bio to POST - findusers.php: skip rank lookup when rankId==0, verify rank_id > 0 to handle placeholder objects from handler->get() - cache/file.php: revert to serialize=>true default and remove legacy eval/include path entirely — treat non-serialized entries as cache miss (regenerated on next write) - block.php: use defined()/constant() pattern for XOOPS_ALLOW_PHP_BLOCKS to satisfy Scrutinizer - Rewrite remediation_proposal.md as post-remediation record with upgrade notes
|
|
||
| // Deprecated: this endpoint used a weak md5-based reset token. | ||
| // All password resets now go through the secure core flow. | ||
| include __DIR__ . '/header.php'; |
There was a problem hiding this comment.
modules/profile/lostpass.php now immediately redirects to the core lostpass flow, but still includes the profile module header first. Since redirect_header() exits, the include is unnecessary overhead and may trigger avoidable side effects (template/theme init, globals) on every hit. Consider redirecting immediately without including the module header.
| include __DIR__ . '/header.php'; | |
| require_once __DIR__ . '/../../mainfile.php'; |
| 'path' => XOOPS_VAR_PATH . '/caches/xoops_cache', | ||
| 'extension' => '.php', | ||
| 'prefix' => 'xoops_', | ||
| 'lock' => false, | ||
| 'serialize' => false, | ||
| 'serialize' => true, |
There was a problem hiding this comment.
XoopsCacheFile now defaults 'serialize' to true, but the settings documentation in this class still indicates the default is false. Please update the inline docs to reflect the new default so future configuration/maintenance isn’t misled.
| if ($c_type === 'H') { | ||
| return str_replace('{X_SITEURL}', XOOPS_URL . '/', $this->getVar('content', 'n')); | ||
| } elseif ($c_type === 'P') { | ||
| if (!(defined('XOOPS_ALLOW_PHP_BLOCKS') ? constant('XOOPS_ALLOW_PHP_BLOCKS') : false)) { |
There was a problem hiding this comment.
The XOOPS_ALLOW_PHP_BLOCKS gate treats any non-empty string value (e.g. 'false') as truthy, which can unintentionally enable PHP block execution via misconfiguration. Consider requiring an explicit boolean true (e.g. check that the constant is defined and strictly === true) before allowing the eval() path.
| if (!(defined('XOOPS_ALLOW_PHP_BLOCKS') ? constant('XOOPS_ALLOW_PHP_BLOCKS') : false)) { | |
| if (!(defined('XOOPS_ALLOW_PHP_BLOCKS') && constant('XOOPS_ALLOW_PHP_BLOCKS') === true)) { |
| if ($c_type === 'H') { | ||
| return str_replace('{X_SITEURL}', XOOPS_URL . '/', (string) $this->getVar('content', 'n')); | ||
| } elseif ($c_type === 'P') { | ||
| if (!(defined('XOOPS_ALLOW_PHP_BLOCKS') ? constant('XOOPS_ALLOW_PHP_BLOCKS') : false)) { |
There was a problem hiding this comment.
The XOOPS_ALLOW_PHP_BLOCKS gate treats any non-empty string value (e.g. 'false') as truthy, which can unintentionally enable PHP block execution via misconfiguration. Consider requiring an explicit boolean true (e.g. check that the constant is defined and strictly === true) before allowing the eval() path.
| if (!(defined('XOOPS_ALLOW_PHP_BLOCKS') ? constant('XOOPS_ALLOW_PHP_BLOCKS') : false)) { | |
| $allowPhpBlocks = defined('XOOPS_ALLOW_PHP_BLOCKS') && constant('XOOPS_ALLOW_PHP_BLOCKS') === true; | |
| if (!$allowPhpBlocks) { |
- profile/lostpass.php: use mainfile.php instead of module header for redirect-only endpoint (avoids unnecessary template/theme init) - cache/file.php: update inline doc to reflect serialize default is now true - block.php + system/block.php: require XOOPS_ALLOW_PHP_BLOCKS === true (strict boolean) to prevent string 'false' from being truthy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/remediation_proposal.md`:
- Around line 84-87: Add a brief upgrade caveat explaining that password reset
tokens generated before the upgrade are incompatible with the new core flow and
any pending password reset emails will no longer work; instruct admins/users to
request a new password reset if they received a reset link prior to the upgrade,
and reference the related changes (the cache engine default change and
XOOPS_ALLOW_PHP_BLOCKS note) so readers see this as an additional compatibility
item to consider when upgrading.
In `@htdocs/modules/profile/lostpass.php`:
- Around line 20-23: The current deprecated lostpass endpoint immediately
redirects and drops legacy reset query params (id, act, actkey), breaking
in-flight resets; update htdocs/modules/profile/lostpass.php to detect legacy
parameters and either map them into the core flow's expected uid and token
parameters (translate id->uid and construct a temporary token param or forward
act/actkey into uid/token) or intercept and render a clear "please request a new
password reset" page before calling redirect_header; reference the file's
redirect_header call and the core lostpass.php expectation of uid and token when
implementing the parameter translation or user-facing message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 32f3e168-9557-4041-8824-40ac8522c518
📒 Files selected for processing (6)
docs/remediation_proposal.mdhtdocs/class/cache/file.phphtdocs/kernel/block.phphtdocs/kernel/configitem.phphtdocs/modules/profile/lostpass.phphtdocs/modules/system/class/block.php
| // Deprecated: this endpoint used a weak md5-based reset token. | ||
| // All password resets now go through the secure core flow. | ||
| require_once __DIR__ . '/../../mainfile.php'; | ||
| redirect_header(XOOPS_URL . '/lostpass.php', 0, '', false); |
There was a problem hiding this comment.
In-flight password resets will fail after upgrade.
The core lostpass.php expects uid and token parameters (per context snippet from htdocs/lostpass.php:52-59), but old reset emails contain id, act, and actkey parameters. Users who received reset emails before upgrade but click the link after upgrade will see a generic redirect rather than the reset form.
Consider documenting this in the upgrade notes as expected behavior, or optionally add parameter translation logic in the core flow to recognize legacy parameters and display a "please request a new reset" message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/lostpass.php` around lines 20 - 23, The current
deprecated lostpass endpoint immediately redirects and drops legacy reset query
params (id, act, actkey), breaking in-flight resets; update
htdocs/modules/profile/lostpass.php to detect legacy parameters and either map
them into the core flow's expected uid and token parameters (translate id->uid
and construct a temporary token param or forward act/actkey into uid/token) or
intercept and render a clear "please request a new password reset" page before
calling redirect_header; reference the file's redirect_header call and the core
lostpass.php expectation of uid and token when implementing the parameter
translation or user-facing message.
| $report['ip'] = $_SERVER['REMOTE_ADDR']; | ||
| $report['uname'] = $_POST['uname'] ?? null; | ||
| $report['uname'] = Request::getString('uname', '', 'POST'); | ||
| $result = $this->protector->stopForumSpamLookup($report['email'], $report['ip'], $report['uname']); |
There was a problem hiding this comment.
stopForumSpamLookup() builds the query string by concatenating &email= / &username= without URL-encoding the values. Even after switching to Request::getString(), inputs containing &, = or other special characters can break the request or inject extra query parameters. Encode the email/username (and IP if needed) before calling stopForumSpamLookup(), or update stopForumSpamLookup() to handle encoding internally.
| $result = $this->protector->stopForumSpamLookup($report['email'], $report['ip'], $report['uname']); | |
| $encodedEmail = rawurlencode($report['email']); | |
| $encodedIp = rawurlencode($report['ip']); | |
| $encodedUname = rawurlencode($report['uname']); | |
| $result = $this->protector->stopForumSpamLookup($encodedEmail, $encodedIp, $encodedUname); |
| if (!(defined('XOOPS_ALLOW_PHP_BLOCKS') && constant('XOOPS_ALLOW_PHP_BLOCKS') === true)) { | ||
| $logger = XoopsLogger::getInstance(); | ||
| $logger->addWarning('PHP block execution is disabled. Set XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php to enable.'); | ||
| return ''; |
There was a problem hiding this comment.
When XOOPS_ALLOW_PHP_BLOCKS is not enabled, this logs a warning on every render of a PHP block. On sites with multiple PHP blocks (or repeated renders), this can spam logs/debug output and add overhead. Consider logging once per request (e.g., via a static guard) or downgrading to a quieter notice mechanism.
| $logger = XoopsLogger::getInstance(); | ||
| $logger->addWarning('PHP block execution is disabled. Set XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php to enable.'); |
There was a problem hiding this comment.
When XOOPS_ALLOW_PHP_BLOCKS is not enabled, this logs a warning on every render of a PHP block. On pages with multiple PHP blocks or frequent renders, this can generate noisy logs/debug output and unnecessary overhead. Consider logging once per request (static guard) or otherwise throttling this warning.
| $logger = XoopsLogger::getInstance(); | |
| $logger->addWarning('PHP block execution is disabled. Set XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php to enable.'); | |
| static $phpBlockWarningLogged = false; | |
| if (!$phpBlockWarningLogged) { | |
| $logger = XoopsLogger::getInstance(); | |
| $logger->addWarning('PHP block execution is disabled. Set XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php to enable.'); | |
| $phpBlockWarningLogged = true; | |
| } |
| if (Request::hasVar('memberslist_id')) { | ||
| $xoBreadCrumb->render(); | ||
| $error = ''; | ||
| foreach (Request::getArray('memberslist_id', []) as $del) { | ||
| $del = (int) $del; | ||
| $user = $member_handler->getUser($del); |
There was a problem hiding this comment.
Bulk delete reads memberslist_id via Request::getArray('memberslist_id', []) without a source hash, so it defaults to $_REQUEST. Since this action is meant to be driven by a POSTed form submission, consider pinning it to 'POST' to prevent GET/COOKIE values from influencing which accounts are processed.
| } elseif ($data && empty($this->settings['serialize'])) { | ||
| $data = eval($data); | ||
| // Legacy non-serialized cache used eval() — removed for security. | ||
| // Treat as cache miss; entry will be regenerated with serialization on next write. | ||
| $data = null; | ||
| } |
There was a problem hiding this comment.
XoopsCacheFile::read() no longer evaluates non-serialized cache entries, but write() still supports serialize = false and writes a return ...; payload. With the current change, any consumer configured with serialize = false will write entries that can never be read back (and the legacy serialize=false branch returns null, which doesn’t match the method’s documented false on cache-miss). Consider either removing/ignoring the serialize=false mode (force serialize on), or changing the non-serialized format to something readable without eval() (e.g., a real PHP file you can safely include) and ensure cache-miss returns false consistently.
Pending reset emails sent before upgrade will not work with the new secure token flow. Users must request a new reset after upgrade. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| $logger = XoopsLogger::getInstance(); | ||
| $logger->addWarning('PHP block execution is disabled. Set XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php to enable.'); |
There was a problem hiding this comment.
XoopsLogger does not define an addWarning() method, so this will trigger a fatal error when PHP blocks are disabled. Use an existing mechanism (e.g. trigger_error(..., E_USER_WARNING) or another XoopsLogger method that exists) and then return an empty string.
| $logger = XoopsLogger::getInstance(); | |
| $logger->addWarning('PHP block execution is disabled. Set XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php to enable.'); | |
| trigger_error( | |
| 'PHP block execution is disabled. Set XOOPS_ALLOW_PHP_BLOCKS to true in mainfile.php to enable.', | |
| E_USER_WARNING | |
| ); |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/remediation_proposal.md`:
- Around line 11-55: The summary lists 22 fixes but the table shows 15 grouped
rows, so add a crosswalk that maps each of the original 22 audit finding IDs to
the grouped rows (or annotate each grouped row with a per-row count and the list
of original IDs); update the Phase tables (rows like 1A, 1B, 1C, 2A–2E, 3A–3C,
4A–4C, 5A) to include either "(n findings: ID1, ID2, ...)" or append the exact
original audit IDs that were collapsed into that row (e.g., list the six file
IDs under 1A for getString() -> Request::getVar(), the three system admin
instances under 1C for getUser() null guards, the five unserialize() instances
under 3C, etc.) so an auditor can reconstruct the total of 22 from the document.
- Line 86: The docs currently hardcode the cache path
"xoops_data/caches/xoops_cache/" in the upgrade note about the cache default
change (serialize => true); update this sentence to avoid a literal path and
instead instruct admins to clear their configured cache directory (referencing
the installation variable/constant used by XOOPS, e.g. the configured
var/trust/cache path or the project constant such as XOOPS_VAR_PATH or
XOOPS_CACHE_PATH), and give a short example phrased generically (like "clear
your configured cache directory or the cache path defined by your XOOPS
installation/constant") so readers are directed to the correct location.
- Around line 74-79: Update the "Files Changed" table to remove ellipses and use
the exact repository paths: replace `themes/default/.../system_siteclosed.tpl`
with `themes/default/modules/system/system_siteclosed.tpl` and replace
`xoops_lib/.../postcommon_register_stopforumspam.php` with
`xoops_lib/modules/protector/filters_disabled/postcommon_register_stopforumspam.php`;
ensure these exact filenames are used wherever the table lists file paths (e.g.,
in the rows for "XSS fixes" and "Superglobals") so all entries are fully
qualified and grep-able.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17a194ac-392d-4246-9948-10c342f51c0b
📒 Files selected for processing (1)
docs/remediation_proposal.md
| Two independent audits identified **26 distinct issues** across security, compatibility, and correctness categories. **22 have been fixed** across two rounds of commits. 4 items are deferred as intentional design or already correct. | ||
|
|
||
| --- | ||
|
|
||
| ## Fixed Items | ||
|
|
||
| ### Phase 1: Critical | ||
|
|
||
| | Item | What was fixed | How | | ||
| |------|----------------|-----| | ||
| | 1A | Password fields using `getString()` (all 6 files) | Changed to `Request::getVar()` with `MASK_ALLOW_RAW \| MASK_NO_TRIM` | | ||
| | 1B | Insecure profile lostpass (md5 5-char token) | Disabled preload redirect in `core.php`; `profile/lostpass.php` now redirects to secure core flow | | ||
| | 1C | Missing null guards after `getUser()` in system admin (3 instances) | Added `!is_object()` guards with `_AM_SYSTEM_USERS_NO_SUCH_USER` redirect | | ||
|
|
||
| ### Phase 2: High | ||
|
|
||
| | Item | What was fixed | How | | ||
| |------|----------------|-----| | ||
| | 2A | Reflected XSS via `$_SERVER['PHP_SELF']` in `xoopscomments.php` | Replaced with `htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES \| ENT_HTML5, 'UTF-8')` | | ||
| | 2B | Raw `REQUEST_URI` in `system_siteclosed.tpl` | Added `\|escape` Smarty modifier | | ||
| | 2C | Content fields (`com_text`, `user_sig`, `bio`) using `getString()` | Changed to `getText()` pinned to `'POST'` | | ||
| | 2D | CSRF: `ok` parameter not pinned to POST | Changed to `Request::getInt('ok', 0, 'POST')` | | ||
| | 2E | Missing null guards in `findusers.php` and profile admin files | Added `is_object()` + `rank_id > 0` check; profile admin files already fixed | | ||
|
|
||
| ### Phase 3: Security Hardening | ||
|
|
||
| | Item | What was fixed | How | | ||
| |------|----------------|-----| | ||
| | 3A | `eval()` in file cache engine | Removed `eval()` path; changed default `serialize` to `true`; legacy non-serialized entries treated as cache miss | | ||
| | 3B | `eval()` for custom PHP blocks | Gated behind `XOOPS_ALLOW_PHP_BLOCKS` constant (default `false`) using `defined()/constant()` pattern | | ||
| | 3C | Unsafe `unserialize()` (5 instances) | Added `['allowed_classes' => false]` to all calls; `configitem.php` uses `set_error_handler()` for corrupted data | | ||
|
|
||
| ### Phase 4: Compatibility | ||
|
|
||
| | Item | What was fixed | How | | ||
| |------|----------------|-----| | ||
| | 4A | Smarty `{php}` tags in imagemanager templates | Removed `{php}` blocks (fixed in prior commits) | | ||
| | 4B | PHP 8.2 fatal in vendor demo (`__autoload()`) | Deleted `wideimage/demo/` directory | | ||
| | 4C | Type safety for `$groups_failed` in SQL | Already cast to `(int)` in loop (confirmed correct) | | ||
|
|
||
| ### Phase 5: Consistency | ||
|
|
||
| | Item | What was fixed | How | | ||
| |------|----------------|-----| | ||
| | 5A | Raw superglobals in `gtickets.php` and `stopforumspam.php` | Migrated to `Xmf\Request::getString()` / `hasVar()` | |
There was a problem hiding this comment.
Add a crosswalk from the 22 audit findings to these 15 grouped rows.
The summary says 22 findings were fixed, but this section only exposes 15 line items. Because several rows collapse multiple instances (1A, 1C, 3C, etc.), an auditor cannot reconstruct the closure count from this record alone. Please add the original audit IDs or a per-row finding count so the totals are traceable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/remediation_proposal.md` around lines 11 - 55, The summary lists 22
fixes but the table shows 15 grouped rows, so add a crosswalk that maps each of
the original 22 audit finding IDs to the grouped rows (or annotate each grouped
row with a per-row count and the list of original IDs); update the Phase tables
(rows like 1A, 1B, 1C, 2A–2E, 3A–3C, 4A–4C, 5A) to include either "(n findings:
ID1, ID2, ...)" or append the exact original audit IDs that were collapsed into
that row (e.g., list the six file IDs under 1A for getString() ->
Request::getVar(), the three system admin instances under 1C for getUser() null
guards, the five unserialize() instances under 3C, etc.) so an auditor can
reconstruct the total of 22 from the document.
|
|
||
| $report = []; | ||
| $report['email'] = $_POST['email'] ?? null; | ||
| $report['email'] = Request::getString('email', '', 'POST'); |
There was a problem hiding this comment.
Since $report['email'] is expected to be an email address (and is later concatenated into an HTTP query string), it should be retrieved/validated as an email rather than a generic string. Consider using Request::getEmail('email', '', 'POST') here (and possibly urlencode() when building the stopforumspam URL in stopForumSpamLookup()) to avoid malformed input affecting the outbound request.
| $report['email'] = Request::getString('email', '', 'POST'); | |
| $report['email'] = Request::getEmail('email', '', 'POST'); |
…e path - Add per-item instance counts and file lists so auditors can reconstruct the total of 22 from the grouped table rows - Use full repository paths instead of ellipses in Files Changed table - Reference XOOPS_VAR_PATH instead of hardcoded cache path in upgrade notes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/remediation_proposal.md`:
- Around line 17-56: Phase headers are inconsistent: Phase 4 header ("Phase 4:
Compatibility") omits an "instances" count and Phase 5 uses "1 item, 2 files"
instead of "instances"; update the Phase 4 and Phase 5 headers to match the "X
items, Y instances" format used in Phases 1–3 (e.g., change Phase 4 to "3 items,
3 instances" or to the correct instance count, and change Phase 5 to "1 item, 2
instances"), and add a short parenthetical note under Phase 4 if you
intentionally count files/directories instead of instances to document the
methodology; modify the markdown headers and the summary table entries
(referencing "Phase 4: Compatibility" and "Phase 5: Consistency") so all
phase-level counts follow the same standardized phrasing.
- Around line 1-14: The remediation record references missing source audit files
(security-critical-audit-2026-03-08.md and migration_audit_findings.md) so
auditors cannot verify the claimed "22 actionable items fixed" or the mapping to
grouped items (1A–5A); fix by either adding the original audit documents to the
repo, or adding a crosswalk table in docs/remediation_proposal.md that maps each
original finding ID to the corresponding grouped remediation item, or by
annotating each grouped row (1A–5A) with the specific original finding IDs it
addresses; ensure the chosen change clearly lists each original finding ID and
the remediation commit/PR references so traceability is restored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4cf0d0bc-f90a-4abe-9e01-f47c59bb7708
📒 Files selected for processing (1)
docs/remediation_proposal.md
| # XOOPS Core 2.5 — Security Audit Remediation Record | ||
|
|
||
| **Date:** 2026-03-08 (final) | ||
| **Based on:** `security-critical-audit-2026-03-08.md` + `migration_audit_findings.md` | ||
| **Status:** All 22 actionable items fixed. 4 items deferred as acceptable. | ||
|
|
||
| --- | ||
|
|
||
| ## Executive Summary | ||
|
|
||
| Two independent audits identified **26 distinct issues** across security, compatibility, and correctness categories. **22 have been fixed** across two rounds of commits. 4 items are deferred as intentional design or already correct. | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the original audit documents and extract finding identifiers
# Search for the audit documents referenced in line 4
fd -t f 'security-critical-audit-2026-03-08\.md|migration_audit_findings\.md'
# If found, extract finding IDs/numbers from those documents
for file in $(fd -t f 'security.*audit.*\.md|migration.*audit.*\.md'); do
echo "=== $file ==="
# Look for common finding ID patterns (e.g., SEC-001, MIG-042, Finding 1.1, etc.)
rg -n '^\s*(SEC-|MIG-|Finding\s+\d+|##\s*\d+\.)' "$file" | head -30
doneRepository: XOOPS/XoopsCore25
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find the remediation_proposal.md file
echo "=== Searching for remediation_proposal.md ==="
fd -t f 'remediation_proposal\.md'
# Also search for any audit-related documents
echo ""
echo "=== Searching for all audit-related documents ==="
fd -t f -e md | rg -i 'audit|security|finding|remediation'
# Look for doc directories
echo ""
echo "=== Checking docs directory structure ==="
ls -la docs/ 2>/dev/null || echo "No docs directory"Repository: XOOPS/XoopsCore25
Length of output: 1622
🏁 Script executed:
#!/bin/bash
# Read the remediation_proposal.md file to check its actual content
cat -n docs/remediation_proposal.md
# Search for the referenced audit documents with broader patterns
echo ""
echo "=== Searching for referenced source documents ==="
fd -t f | rg -i 'security.*2026.*03.*08|migration.*audit|audit.*findings'Repository: XOOPS/XoopsCore25
Length of output: 6930
Source audit documents are missing from the repository; cannot verify the claimed 22 findings or their mapping to the 15 grouped remediation items.
The document references security-critical-audit-2026-03-08.md and migration_audit_findings.md (line 4) but neither file exists in the repository. Without these source documents, auditors cannot independently verify that the claimed 22 actionable items fixed correspond to actual audit findings, nor trace how those 22 findings map to the 15 grouped remediation items (1A–5A) presented below.
For a security audit remediation record, this traceability is essential. Add one of:
- The source audit documents to the repository, or
- A crosswalk table mapping the 22 original audit finding IDs to the grouped items below, or
- Inline annotations on each grouped row listing which original finding IDs it addresses
Without this, readers cannot confirm audit closure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/remediation_proposal.md` around lines 1 - 14, The remediation record
references missing source audit files (security-critical-audit-2026-03-08.md and
migration_audit_findings.md) so auditors cannot verify the claimed "22
actionable items fixed" or the mapping to grouped items (1A–5A); fix by either
adding the original audit documents to the repo, or adding a crosswalk table in
docs/remediation_proposal.md that maps each original finding ID to the
corresponding grouped remediation item, or by annotating each grouped row
(1A–5A) with the specific original finding IDs it addresses; ensure the chosen
change clearly lists each original finding ID and the remediation commit/PR
references so traceability is restored.
| ### Phase 1: Critical (3 items, 10 instances) | ||
|
|
||
| | Item | What was fixed | How | | ||
| |------|----------------|-----| | ||
| | 1A (6 files: `lostpass.php`, `edituser.php`, `register.php`, `profile/changepass.php`, `profile/register.php`, `system/admin/users/main.php`) | Password fields using `getString()` | Changed to `Request::getVar()` with `MASK_ALLOW_RAW \| MASK_NO_TRIM` | | ||
| | 1B (2 files: `profile/preloads/core.php`, `profile/lostpass.php`) | Insecure profile lostpass (md5 5-char token) | Disabled preload redirect; `profile/lostpass.php` now redirects to secure core flow | | ||
| | 1C (1 file, 3 instances: `system/admin/users/main.php` lines ~85, ~132, ~355) | Missing null guards after `getUser()` in system admin | Added `!is_object()` guards with `_AM_SYSTEM_USERS_NO_SUCH_USER` redirect | | ||
|
|
||
| ### Phase 2: High (5 items, 11 instances) | ||
|
|
||
| | Item | What was fixed | How | | ||
| |------|----------------|-----| | ||
| | 2A (1 file, 2 instances: `class/xoopscomments.php` lines 268, 444) | Reflected XSS via `$_SERVER['PHP_SELF']` | Replaced with `htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES \| ENT_HTML5, 'UTF-8')` | | ||
| | 2B (1 file: `themes/default/modules/system/system_siteclosed.tpl`) | Raw `REQUEST_URI` in site-closed template | Added `\|escape` Smarty modifier | | ||
| | 2C (3 files, 6 instances: `include/comment_post.php`, `edituser.php`, `system/admin/users/main.php`) | Content fields (`com_text`, `user_sig`, `bio`) using `getString()` | Changed to `getText()` pinned to `'POST'` | | ||
| | 2D (1 file: `system/admin/users/main.php`) | CSRF: `ok` parameter not pinned to POST | Changed to `Request::getInt('ok', 0, 'POST')` | | ||
| | 2E (4 files: `include/findusers.php`, `profile/admin/step.php`, `profile/admin/category.php`, `profile/admin/field.php`) | Missing null guards after `get()` | Added `is_object()` + `rank_id > 0` check in findusers; profile admin files fixed in prior commits | | ||
|
|
||
| ### Phase 3: Security Hardening (3 items, 8 instances) | ||
|
|
||
| | Item | What was fixed | How | | ||
| |------|----------------|-----| | ||
| | 3A (1 file: `class/cache/file.php`) | `eval()` in file cache engine | Removed `eval()` path; changed default `serialize` to `true`; legacy entries treated as cache miss | | ||
| | 3B (2 files: `kernel/block.php`, `modules/system/class/block.php`) | `eval()` for custom PHP blocks | Gated behind `XOOPS_ALLOW_PHP_BLOCKS === true` constant check | | ||
| | 3C (5 instances: `kernel/object.php` x2, `kernel/configitem.php`, `class/cache/model.php`, `modules/profile/include/update.php`) | Unsafe `unserialize()` without `allowed_classes` | Added `['allowed_classes' => false]`; `configitem.php` uses `set_error_handler()` for corrupted data | | ||
|
|
||
| ### Phase 4: Compatibility (3 items) | ||
|
|
||
| | Item | What was fixed | How | | ||
| |------|----------------|-----| | ||
| | 4A (2 files: `system/templates/system_imagemanager.tpl`, `system_imagemanager2.tpl`) | Smarty `{php}` tags | Removed `{php}` blocks (fixed in prior commits) | | ||
| | 4B (1 directory: `xoops_lib/vendor/smottt/wideimage/demo/`) | PHP 8.2 fatal (`__autoload()`) | Deleted demo directory | | ||
| | 4C (1 file: `system/admin/users/main.php`) | Type safety for `$groups_failed` in SQL | Already cast to `(int)` in loop (confirmed correct) | | ||
|
|
||
| ### Phase 5: Consistency (1 item, 2 files) | ||
|
|
||
| | Item | What was fixed | How | | ||
| |------|----------------|-----| | ||
| | 5A (2 files: `xoops_lib/modules/protector/class/gtickets.php`, `xoops_lib/modules/protector/filters_disabled/postcommon_register_stopforumspam.php`) | Raw superglobals | Migrated to `Xmf\Request::getString()` / `hasVar()` | | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Standardize instance counts across all phase headers for consistency.
Phases 1-3 use the format "X items, Y instances" (e.g., "3 items, 10 instances"), but Phase 4 omits the instance count entirely ("3 items") and Phase 5 switches to "1 item, 2 files". This inconsistency makes it difficult to trace how the phase-level counts roll up to the "22 actionable items fixed" total.
For auditability and consistency, please standardize all phase headers to use the same format.
📝 Suggested standardization
-### Phase 4: Compatibility (3 items)
+### Phase 4: Compatibility (3 items, 4 instances)
-### Phase 5: Consistency (1 item, 2 files)
+### Phase 5: Consistency (1 item, 2 instances)If you prefer to count files/directories instead of instances in Phase 4, that's also acceptable—just make the counting methodology explicit (e.g., add a note explaining that Phase 4 counts by file/directory rather than by distinct code fix).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/remediation_proposal.md` around lines 17 - 56, Phase headers are
inconsistent: Phase 4 header ("Phase 4: Compatibility") omits an "instances"
count and Phase 5 uses "1 item, 2 files" instead of "instances"; update the
Phase 4 and Phase 5 headers to match the "X items, Y instances" format used in
Phases 1–3 (e.g., change Phase 4 to "3 items, 3 instances" or to the correct
instance count, and change Phase 5 to "1 item, 2 instances"), and add a short
parenthetical note under Phase 4 if you intentionally count files/directories
instead of instances to document the methodology; modify the markdown headers
and the summary table entries (referencing "Phase 4: Compatibility" and "Phase
5: Consistency") so all phase-level counts follow the same standardized
phrasing.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1636 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
Fixes all 13 remaining issues identified in the security/critical audit and migration audit:
XoopsTokenHandler(random, one-time, expiring tokens)$_SERVER['PHP_SELF']inxoopscomments.php(useSCRIPT_NAMEinstead) andREQUEST_URIin site-closed templateeval()path in file cache engine (defaultserializetotrue); gate PHP blockeval()behindXOOPS_ALLOW_PHP_BLOCKSconstant (defaultfalse)allowed_classes => falseto all 5 bareunserialize()calls acrossobject.php,configitem.php,cache/model.php,profile/update.phpgetUser()/get()in system admin users (delete, bulk delete, activate) andfindusers.phpokdelete confirmation parameter to POST onlygetText()foruser_sig,bio, andcom_textfields that legitimately contain HTMLgetString()usage for password fields inlostpass.phpdemo/directory containing fatal__autoload()gtickets.phpand stopforumspam filter toXmf\RequestFiles changed (18 modified + 67 deleted)
modules/profile/preloads/core.php,modules/profile/lostpass.php,lostpass.phpclass/xoopscomments.php,themes/default/.../system_siteclosed.tplclass/cache/file.php,kernel/block.php,modules/system/class/block.php,kernel/object.php,kernel/configitem.php,class/cache/model.php,modules/profile/include/update.phpmodules/system/admin/users/main.php,include/findusers.phpedituser.php,include/comment_post.php,modules/system/admin/users/main.phpxoops_lib/modules/protector/class/gtickets.php,xoops_lib/.../postcommon_register_stopforumspam.phpxoops_lib/vendor/smottt/wideimage/demo/(deleted)Test plan
lostpass.php(email submission, token link, password change)modules/profile/lostpass.phpredirects to core lostpassXOOPS_ALLOW_PHP_BLOCKSis not definedXOOPS_ALLOW_PHP_BLOCKSis set totrue🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Security
Chores