Superglobal Migration to Xmf\Request#1633
Conversation
- htdocs root: ajaxfineupload.php, image.php - include/: common.php, notification_update.php, + all comment/findusers/site-closed files - modules/profile/: search.php, edituser.php, activate.php, register.php, changemail.php, changepass.php, + all admin files - modules/system/: admin/tplsets/, admin/comments/, admin/groups/, admin/users/, themes (transition, default, dark) - modules/pm/: readpmsg.php, viewpmsg.php, admin/prune.php, pmlite.php - class/: xoopseditor/sampleform.inc.php, all TinyMCE plugins (tinymce/tinymce5/tinymce7) - install/: All 9 page files + installwizard.php - xoops_lib/modules/protector/: admin/center.php, admin/prefix_manager.php, admin.php, module_icon.php, postcommon_register_insert_js_check.php, index.php, notification_update.php - Frameworks/: art/functions.ini.php
WalkthroughLarge, cross-cutting migration replacing direct PHP superglobals with the Xmf\Request API across htdocs, modules, and xoops_lib; includes upload normalization, safer file I/O, a reference-return change in the theme factory, and a new PHPUnit suite validating Request semantics. 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 |
mambax7
requested review from
ForXoops,
alain01,
geekwright,
ggoffy,
ihackcode,
kevinpetit,
liomj,
montuy337513 and
tad0616
There was a problem hiding this comment.
Pull request overview
This PR migrates direct superglobal access ($_GET, $_POST, $_REQUEST, $_COOKIE) to the Xmf\Request API across the XOOPS 2.5.x codebase. It eliminates dozens of raw superglobal reads in favour of type-safe, sanitised accessors and adds a companion regression test suite.
Changes:
- Core files (
htdocs/include/,htdocs/class/,htdocs/install/): Replace all direct superglobal reads withXmf\Requestcalls, using explicit hash parameters (GET,POST,COOKIE). - Module files (
modules/system/,modules/profile/,modules/pm/,xoops_lib/modules/protector/, editor plugins): Same migration, often also refactoring intermediate variables to avoid re-reading the request superglobal. - New test class
tests/unit/htdocs/migration/SuperglobalMigrationTest.php: Regression tests that verify sanitisation behaviour of theXmf\Requestmethods used in the migrated code.
Reviewed changes
Copilot reviewed 92 out of 92 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/htdocs/migration/SuperglobalMigrationTest.php |
New regression tests for Xmf\Request sanitisation behaviour |
htdocs/search.php |
Migrates superglobal reads; mids now reads only from POST |
htdocs/include/notification_update.php |
Migrates not_redirect, not_submit, not_list to Request |
htdocs/include/comment_*.php |
Replace $_GET/$_POST checks with Request::hasVar |
htdocs/modules/profile/search.php |
Migrates all search params to POST-only reads |
htdocs/modules/profile/admin/category.php |
Migrates CRUD operations; delete id now reads POST-only |
htdocs/modules/profile/admin/field.php |
Migrates CRUD operations using Request |
htdocs/modules/profile/admin/step.php |
Migrates CRUD operations using Request |
htdocs/modules/profile/admin/user.php |
Migrates user edit/delete to Request |
htdocs/xoops_lib/modules/protector/filters_disabled/postcommon_register_insert_js_check.php |
Changes anti-spam trigger from !empty($_POST) to specific field check |
htdocs/xoops_lib/modules/protector/admin/prefix_manager.php |
Migrates prefix CRUD to Request::getCmd |
htdocs/modules/system/admin/* |
Migrates admin module superglobal reads |
htdocs/class/xoopskernel.php, htdocs/class/theme.php |
Migrates theme-select logic |
htdocs/install/class/installwizard.php + page files |
Migrates install wizard cookie/POST reads |
htdocs/xoops_lib/modules/protector/filters_disabled/postcommon_register_insert_js_check.php
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 42
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (26)
htdocs/class/xoopseditor/tinymce7/js/tinymce/plugins/xoopsemoticons/xoopsemoticons.php (2)
13-14: 🧹 Nitpick | 🔵 TrivialDocblock version mismatch.
The docblock states "Xoopsemoticons plugin for tinymce v5" but this file resides in the
tinymce7directory. Should be updated to reflect v7./** - * Xoopsemoticons plugin for tinymce v5 + * Xoopsemoticons plugin for tinymce v7 *🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopseditor/tinymce7/js/tinymce/plugins/xoopsemoticons/xoopsemoticons.php` around lines 13 - 14, Update the top-of-file docblock which currently reads "Xoopsemoticons plugin for tinymce v5" to correctly state v7; locate the docblock comment at the beginning of the file (the block containing "Xoopsemoticons plugin for tinymce v5") and change the version text to "tinymce v7" so the header matches the tinymce7 directory.
75-83: 🧹 Nitpick | 🔵 TrivialInconsistent comparison operators.
Lines 75 and 83 use loose comparison
$op == ''while the newly added line 48 uses strict comparison$op === ''. For consistency and type safety, prefer strict comparisons throughout.♻️ Suggested fix
- if ($op == '') { + if ($op === '') {And at line 83:
- if ($op == '') { + if ($op === '') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopseditor/tinymce7/js/tinymce/plugins/xoopsemoticons/xoopsemoticons.php` around lines 75 - 83, The file uses loose comparison for $op in two if statements (the blocks around the xoopsemoticons output where you see if ($op == '')) while elsewhere the code uses strict comparison ($op === ''); update both occurrences of if ($op == '') to use strict comparison if ($op === '') to keep consistency and type-safety with the other check of $op; ensure you change every matching if ($op == '') in the xoopsemoticons output logic so behavior remains identical but comparisons are consistent.htdocs/modules/profile/changemail.php (1)
44-44:⚠️ Potential issue | 🟡 MinorDead code:
$mytsis declared but never used.The
MyTextSanitizerinstance is assigned but not referenced anywhere in this file. Consider removing this line.🧹 Suggested cleanup
} else { - $myts = \MyTextSanitizer::getInstance(); $pass = trim(Request::getString('passwd', '', 'POST'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/changemail.php` at line 44, The line creating an unused MyTextSanitizer instance ($myts = \MyTextSanitizer::getInstance();) is dead code; remove that statement from changemail.php so no unused variable remains, and run a quick grep for $myts in this file to confirm there are no remaining references before committing.htdocs/include/findusers.php (2)
486-488:⚠️ Potential issue | 🟡 MinorDead code:
$queryis assigned but never used.The variable
$queryis set to an empty string but is never referenced elsewhere in this file. Based on the summary, the old code cleared$_POST['query']—now it just creates an unused variable. This entire block can be removed.Proposed fix
- if (Request::hasVar('query', 'POST')) { - $query = ''; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/include/findusers.php` around lines 486 - 488, Remove the dead if-block that checks Request::hasVar('query','POST') and assigns $query = '' since $query is never used; delete the entire block and run a quick grep for $query in this scope (and any nearby logic using Request::get/hasVar('query')) to ensure no other code depended on that assignment.
639-650:⚠️ Potential issue | 🟠 MajorDirect
$_POSTaccess not migrated—inconsistent with PR purpose.This PR is specifically about migrating superglobals to
Xmf\Request, yet$_POSTis accessed directly here (and again at line 724). This violates the coding guidelines requiring all user input be filtered viaXmf\Request.Consider enumerating the expected POST keys and retrieving them through
Request::getVar()orRequest::getArray(), then iterating over that filtered collection instead.Proposed approach
- foreach ($_POST as $k => $v) { + // Define expected POST keys to avoid direct superglobal access + $postKeys = ['uname', 'email', 'user_sort', 'user_order', 'limit', 'start', 'mode', 'target', 'multiple', 'user_submit', 'XOOPS_TOKEN_REQUEST', /* add other expected keys */]; + $postData = []; + foreach ($postKeys as $k) { + if (Request::hasVar($k, 'POST')) { + $postData[$k] = Request::getVar($k, '', 'POST'); + } + } + foreach ($postData as $k => $v) {As per coding guidelines: "All user input must be filtered using Xmf\Request::getVar() or Xmf\FilterInput::clean() — never access $_GET, $_POST, or $_REQUEST directly"
htdocs/modules/system/admin/modulesadmin/main.php (1)
81-82:⚠️ Potential issue | 🟡 MinorDead code:
$moduleis immediately overwritten by the foreach loop.Line 81 retrieves
'module'from the request, but line 82'sforeach ($installed_mods as $module)immediately reassigns$moduleon every iteration. TheRequest::getArray()result is never used.This appears to be an erroneous addition during the superglobal migration—remove it.
🧹 Proposed fix
- $module = Request::getArray('module', []); foreach ($installed_mods as $module) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 81 - 82, The line calling Request::getArray('module', []) is dead because its result is immediately shadowed by the loop variable in foreach ($installed_mods as $module); remove the unused Request::getArray('module', []) statement to avoid the overwritten $module and keep only the foreach ($installed_mods as $module) loop (search for Request::getArray and the foreach to locate the two statements).htdocs/xoops_lib/modules/protector/filters_disabled/postcommon_register_insert_js_check.php (1)
15-19:⚠️ Potential issue | 🟠 MajorDo not skip anti-spam validation when the hidden field is missing.
Line 15 changes the guard from “this is a form POST” to “the POST already contains
antispam_md5”. That creates a bypass: a bot can omit the field and avoidcheckValidate()entirely.🔧 Proposed fix
- if (\Xmf\Request::hasVar('antispam_md5', 'POST')) { - if (!$this->checkValidate()) { - die(_MD_PROTECTOR_TURNJAVASCRIPTON); - } - } + if ('POST' === ($_SERVER['REQUEST_METHOD'] ?? '')) { + if (!\Xmf\Request::hasVar('antispam_md5', 'POST') || !$this->checkValidate()) { + die(_MD_PROTECTOR_TURNJAVASCRIPTON); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/xoops_lib/modules/protector/filters_disabled/postcommon_register_insert_js_check.php` around lines 15 - 19, The current guard only calls checkValidate() when the POST contains antispam_md5, allowing bots to bypass validation by omitting that field; change the condition so checkValidate() runs for any form POST (e.g., check $_SERVER['REQUEST_METHOD'] === 'POST' or other framework POST-detection) instead of testing only for 'antispam_md5', ensuring the checkValidate() call in postcommon_register_insert_js_check.php always executes on POST submissions.htdocs/modules/system/admin/mailusers/main.php (2)
244-251:⚠️ Potential issue | 🟡 MinorType safety: Cast mail_to_user elements to int.
Consistent with the pattern at line 234 where
array_map('intval', $mail_to_group)is applied, the$mail_to_userarray elements should also be cast to integers before being used to constructXoopsUserobjects and checked against$added_id.🛡️ Proposed fix
$mail_to_user = Request::getArray('mail_to_user', [], 'POST'); if (!empty($mail_to_user)) { + $mail_to_user = array_map('intval', $mail_to_user); foreach ($mail_to_user as $to_user) { - if (!in_array($to_user, $added_id)) { + if ($to_user > 0 && !in_array($to_user, $added_id, true)) { $added[] = new XoopsUser($to_user); $added_id[] = $to_user; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/mailusers/main.php` around lines 244 - 251, The mail_to_user elements are not cast to integers before use; mirror the pattern used for mail_to_group by converting Request::getArray('mail_to_user', [], 'POST') into integers (e.g., via array_map('intval', $mail_to_user')) before iterating, then use the integer values when checking against $added_id and when constructing XoopsUser($to_user) to ensure type safety for the XoopsUser instantiation and the in_array($to_user, $added_id) check.
319-326: 🧹 Nitpick | 🔵 TrivialMinor: Redundant backslash on
\is_array().The leading backslash in
\is_array()is unnecessary here since there's no namespace conflict with the global function. While not incorrect, it's inconsistent with other function calls in the file.♻️ Consistency fix
- if (\is_array($mail_send_to) && !empty($mail_send_to)) { + if (is_array($mail_send_to) && !empty($mail_send_to)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/mailusers/main.php` around lines 319 - 326, Remove the redundant leading backslash on the global function call in the mail recipient check: replace "\is_array($mail_send_to)" with "is_array($mail_send_to)" in the conditional around the XoopsFormHidden additions (the block that adds mail_send_to[] or the single mail_send_to hidden). This keeps calls consistent with other global functions in the file and avoids the unnecessary fully-qualified call.htdocs/class/xoopseditor/tinymce5/js/tinymce/plugins/xoopsimagemanager/xoopsimagemanager.php (2)
312-312:⚠️ Potential issue | 🟡 MinorBug: Undefined variable
$error— should be$errors.The variable is defined as
$errorson line 298, but this line references$error. This will cause an undefined variable warning on PHP 8.x and display incorrect output.🐛 Proposed fix
- redirect_header($current_file . '?target=' . $target, 3, xoops_error(implode('<br>', $error))); + redirect_header($current_file . '?target=' . $target, 3, xoops_error(implode('<br>', $errors)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopseditor/tinymce5/js/tinymce/plugins/xoopsimagemanager/xoopsimagemanager.php` at line 312, The redirect call uses an undefined variable $error causing a PHP warning; change the variable to the correctly defined $errors in the redirect_header invocation so it passes the intended error string (keep the rest of the call using $current_file, $target and implode('<br>', $errors)) — update the line that calls redirect_header(...) to reference $errors instead of $error.
38-42:⚠️ Potential issue | 🔴 CriticalCritical: Variable variable injection from
$_POSTcompletely undermines this migration.This loop creates PHP variables directly from unsanitized POST keys and values. An attacker can overwrite critical variables like
$op,$imgcat_id,$image_id,$target, or even$isadminby crafting malicious POST data.This pattern violates the coding guideline requiring all user input to be filtered via
Xmf\Request. The entire purpose of this PR's migration is negated by these lines.🔒 Proposed fix: Remove variable injection and use explicit Request calls
-if (isset($_POST)) { - foreach ($_POST as $k => $v) { - ${$k} = $v; - } -} +// Retrieve specific POST variables explicitly with proper filtering +$image_nicename = Request::getString('image_nicename', '', 'POST'); +$image_display = Request::getInt('image_display', 0, 'POST'); +$image_weight = Request::getInt('image_weight', 0, 'POST'); +$imgcat_name = Request::getString('imgcat_name', '', 'POST'); +$imgcat_maxsize = Request::getInt('imgcat_maxsize', 50000, 'POST'); +$imgcat_maxwidth = Request::getInt('imgcat_maxwidth', 120, 'POST'); +$imgcat_maxheight = Request::getInt('imgcat_maxheight', 120, 'POST'); +$imgcat_display = Request::getInt('imgcat_display', 1, 'POST'); +$imgcat_weight = Request::getInt('imgcat_weight', 0, 'POST'); +$imgcat_storetype = Request::getString('imgcat_storetype', 'file', 'POST'); +$readgroup = Request::getArray('readgroup', [], 'POST'); +$writegroup = Request::getArray('writegroup', [], 'POST');As per coding guidelines: "All user input must be filtered using Xmf\Request::getVar() or Xmf\FilterInput::clean() — never access $_GET, $_POST, or $_REQUEST directly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopseditor/tinymce5/js/tinymce/plugins/xoopsimagemanager/xoopsimagemanager.php` around lines 38 - 42, The code uses a variable-variable loop over $_POST which allows arbitrary POST keys to create/override local variables (e.g., $op, $imgcat_id, $image_id, $target, $isadmin); remove this loop and replace it with explicit, typed calls to Xmf\Request::getVar()/getInt()/getBool() (or Xmf\FilterInput::clean()) for each expected parameter (identify occurrences where $op, $imgcat_id, $image_id, $target, $isadmin are later used) providing safe defaults and proper types; ensure no direct access to $_POST remains and validate/escape values where needed before use.htdocs/edituser.php (1)
45-106:⚠️ Potential issue | 🟠 MajorScope
saveuserinputs toPOSTexplicitly.This branch mutates account data, but the migrated reads default to
Request::get*()with$hash = 'default', which resolves to$_REQUEST(including GET and COOKIE data). On a state-changing endpoint, query-string data can override form POST data, allowing attackers to inject passwords and other sensitive fields via URL manipulation.🔧 Suggested change
- $uid = Request::getInt('uid', 0); + $uid = Request::getInt('uid', 0, 'POST'); ... - $email = Request::getEmail('email', ''); + $email = Request::getEmail('email', '', 'POST'); ... - $password = Request::getString('password', ''); + $password = Request::getString('password', '', 'POST'); ... - $vpass = Request::getString('vpass', ''); + $vpass = Request::getString('vpass', '', 'POST'); ... - $edituser->setVar('name', Request::getString('name', '')); + $edituser->setVar('name', Request::getString('name', '', 'POST'));Apply the same
, 'POST'source argument to the remaining setters in thissaveuserblock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/edituser.php` around lines 45 - 106, The saveuser branch is reading inputs from the default request source (which includes GET/COOKIE) — change every Request::get* / Request::getEmail / Request::getUrl call in this saveuser block to explicitly use the 'POST' source so only POST body values are accepted; specifically update calls like Request::getString('name', ''), Request::getEmail('email', ''), Request::getString('password',''), Request::getString('vpass',''), Request::getUrl('url',''), Request::getString('user_icq',''), Request::getString('user_from',''), Request::getString('user_sig',''), Request::getBool('user_viewemail',0), Request::getString('user_aim',''), Request::getString('user_yim',''), Request::getString('user_msnm',''), Request::getBool('attachsig',0), Request::getFloat('timezone_offset',0.0), Request::getInt('uorder',0), Request::getString('umode','flat'), Request::getInt('notify_method',1), Request::getInt('notify_mode',1), Request::getString('bio',''), Request::getString('user_occ',''), Request::getString('user_intrest',''), Request::getBool('user_mailok',0) to include the 'POST' source argument so only POST data is used when setting $edituser via setVar.htdocs/modules/system/themes/dark/dark.php (1)
39-53: 🧹 Nitpick | 🔵 TrivialRemove or re-enable this constructor block.
This migration is inside a block comment, so Line 43 has no runtime effect and makes the file harder to reason about. If the transition redirect is still required, bring the constructor back as live code and test it; otherwise delete the whole block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/themes/dark/dark.php` around lines 39 - 53, The commented-out __construct() in class dark contains a transition redirect using the 'transition_theme' cookie (getInt('transition_theme', ...)), setcookie(...) and header('location: ...') which currently has no effect; either remove this entire block to clean up dead code or restore it as live code by uncommenting the __construct() method (preserving the existing logic: check \Xmf\Request::getInt('transition_theme',0,'COOKIE'), setcookie('transition_theme',1, time()+86400*365, '/', null, null, true) and perform header('Location: ' . XOOPS_URL . '/admin.php?show=info') followed by exit/die) and then test the transition flow to ensure cookie scope and redirect behave as intended. Ensure you update any namespace/imports if needed and keep header capitalization as 'Location' and call exit after redirect.htdocs/modules/system/admin/groups/main.php (1)
341-355:⚠️ Potential issue | 🟡 MinorDo not report success when no membership update ran.
If
edit_groupis set butselgroups <= 0or the command is unexpected, Line 355 still redirects with_AM_SYSTEM_GROUPS_DBUPDATEDeven though nothing changed. Keep the success redirect inside the executed add/delete branches, or use$errorto return a failure message for invalid submissions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/groups/main.php` around lines 341 - 355, The redirect with the success message is executed unconditionally when $edit_group is set even if no change occurred; modify the logic in the block handling $edit_group (using variables $edit_group, $selgroups, $memberslist_id and calls member_handler->addUserToGroup / member_handler->removeUsersFromGroup) so that redirect_header(..., _AM_SYSTEM_GROUPS_DBUPDATED) is only called after a successful add/delete (i.e., move the redirect into each branch after setting $error = false), or alternatively initialize and set $error appropriately and perform the redirect only when $error === false; ensure unexpected commands or selgroups <= 0 do not trigger the success redirect.htdocs/class/xoopseditor/tinymce7/js/tinymce/plugins/xoopsimagemanager/xoopsimagemanager.php (1)
283-315:⚠️ Potential issue | 🟠 MajorUse the collected
$errorsarray in the delete failure path.Line 315 implodes
$error, but this branch accumulates messages in$errors. Any delete failure will raise an undefined-variable notice and drop the real error list.Proposed fix
- redirect_header($current_file . '?target=' . $target, 3, xoops_error(implode('<br>', $error))); + redirect_header($current_file . '?target=' . $target, 3, xoops_error(implode('<br>', $errors)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopseditor/tinymce7/js/tinymce/plugins/xoopsimagemanager/xoopsimagemanager.php` around lines 283 - 315, The redirect on delete failure uses an undefined variable $error instead of the collected $errors array; update the final error branch so the redirect_header/xoops_error call implodes $errors (not $error) so the accumulated messages from $errors (populated during image deletion and $imgcat_handler->delete failure) are shown; locate the block around the foreach and the final if (count($errors) > 0) and replace implode('<br>', $error) with implode('<br>', $errors).htdocs/modules/system/themes/transition/transition.php (1)
37-51: 🧹 Nitpick | 🔵 TrivialDelete or revive the commented constructor instead of editing it.
Line 41 is inside a block-commented constructor, so this migration does not touch a live request path. Leaving rewritten dead code here makes it harder to audit which superglobal reads are still active.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/themes/transition/transition.php` around lines 37 - 51, The commented-out constructor block containing public function __construct(), the \Xmf\Request::getInt('transition_theme', 0, 'COOKIE') call, setcookie(...) and header('location: ...') should not be left as edited dead code: either remove the entire block-commented constructor entirely, or fully revive it (uncomment) and implement it correctly—when reviving, keep the logic in __construct, use the Request call as shown, set the cookie with proper params, and perform a framework-safe redirect (replace raw header(...) and die() with the appropriate redirect helper) so the code is either gone or becomes a live, auditable path.htdocs/modules/system/admin/maintenance/main.php (1)
211-223:⚠️ Potential issue | 🟠 Major
$dump_modules/$dump_tablesarearray|false, so the exclusivity check is broken.After Lines 211-212, Line 215 can never match
=== true. If both POST arrays are present, the error path is skipped anddump_tables()wins silently. Derive explicit booleans before validating the mutually exclusive inputs.💡 Proposed fix
- $dump = new SystemMaintenance(); - $dump_modules = Request::getArray('dump_modules', [], 'POST') ?: false; - $dump_tables = Request::getArray('dump_tables', [], 'POST') ?: false; + $dump = new SystemMaintenance(); + $dump_modules = Request::getArray('dump_modules', [], 'POST'); + $dump_tables = Request::getArray('dump_tables', [], 'POST'); + $has_dump_modules = $dump_modules !== []; + $has_dump_tables = $dump_tables !== []; $drop = Request::getInt('drop', 1); - if (($dump_tables === true && $dump_modules === true) || ($dump_tables === false && $dump_modules === false)) { + if ($has_dump_tables === $has_dump_modules) { redirect_header('admin.php?fct=maintenance', 2, _AM_SYSTEM_MAINTENANCE_DUMP_ERROR_TABLES_OR_MODULES); } - if ($dump_tables !== false) { + if ($has_dump_tables) { $result = $dump->dump_tables($dump_tables, $drop); - } elseif ($dump_modules !== false) { + } else { $result = $dump->dump_modules($dump_modules, $drop); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/maintenance/main.php` around lines 211 - 223, The exclusivity check uses strict comparisons to true/false on $dump_modules/$dump_tables which are actually array|false, so the condition never triggers; fix by deriving explicit booleans (e.g. $hasDumpModules = $dump_modules !== false; $hasDumpTables = $dump_tables !== false) right after the Request::getArray calls and then use those booleans in the mutual-exclusion check before calling dump_tables() or dump_modules(); keep the redirect_header(_AM_SYSTEM_MAINTENANCE_DUMP_ERROR_TABLES_OR_MODULES) path when both or neither booleans are true and only call $dump->dump_tables($dump_tables, $drop) or $dump->dump_modules($dump_modules, $drop) when the respective boolean is true.htdocs/modules/pm/admin/prune.php (1)
56-67: 🧹 Nitpick | 🔵 TrivialConsider caching the
notifyusersvalue to avoid duplicate request parsing.
Request::getInt('notifyusers', 0, 'POST')is called twice (lines 56 and 67). While functionally correct, caching the value improves readability and efficiency.♻️ Proposed optimization
+ $notifyUsers = Request::getInt('notifyusers', 0, 'POST') === 1; - if (Request::getInt('notifyusers', 0, 'POST') === 1) { + if ($notifyUsers) { $notifycriteria = $criteria; $notifycriteria->add(new Criteria('to_delete', 0)); $notifycriteria->setGroupBy('to_userid'); // Get array of uid => number of deleted messages $uids = $pm_handler->getCount($notifycriteria); } $deletedrows = $pm_handler->deleteAll($criteria); if ($deletedrows === false) { redirect_header('prune.php', 2, _PM_AM_ERRORWHILEPRUNING); } - if (Request::getInt('notifyusers', 0, 'POST') === 1) { + if ($notifyUsers) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/pm/admin/prune.php` around lines 56 - 67, Cache the POST flag returned by Request::getInt('notifyusers', 0, 'POST') into a local variable (e.g. $notifyUsers) and use that variable in both places where Request::getInt(...) is currently called; update the two conditional checks (the one that sets $notifycriteria and $uids and the subsequent check after deleteAll) to use $notifyUsers so you avoid duplicate request parsing and make the logic clearer while leaving $notifycriteria, $uids and $deletedrows handling intact.htdocs/xoops_lib/modules/protector/admin/prefix_manager.php (1)
192-194:⚠️ Potential issue | 🔴 CriticalCritical:
$thisused outside class context—fatal error.Line 192 uses
$this->db->isResultSet($result), but this file is procedural PHP, not a class method. This will cause a fatal error at runtime.🐛 Proposed fix
- if ($this->db->isResultSet($result)) { + if ($db->isResultSet($result)) { $db->freeRecordSet($result); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/xoops_lib/modules/protector/admin/prefix_manager.php` around lines 192 - 194, The code incorrectly references $this->db->isResultSet($result) in procedural scope causing a fatal error; change the check to call the procedural DB instance instead (use $db->isResultSet($result)) so it matches the subsequent $db->freeRecordSet($result) call and ensure both calls use the same $db variable rather than $this.htdocs/modules/pm/viewpmsg.php (1)
58-62:⚠️ Potential issue | 🟡 MinorGuard against json_decode failure.
json_decode()returnsnullon invalid JSON. Passingnulltocount()in PHP 8.x triggers aTypeError. The!empty()check on line 59 helps, butarray_map()on line 60 would still fail if$clean_msg_idisnullbefore that check.🛡️ Proposed defensive fix
- $clean_msg_id = json_decode(Request::getString('msg_ids', '', 'POST'), true, 2); - if (!empty($clean_msg_id)) { + $clean_msg_id = json_decode(Request::getString('msg_ids', '', 'POST'), true, 2) ?? []; + if (!empty($clean_msg_id)) { $clean_msg_id = array_map('intval', $clean_msg_id); } + $size = count($clean_msg_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/pm/viewpmsg.php` around lines 58 - 62, The code calls json_decode into $clean_msg_id and then uses array_map and count without ensuring it's an array; if json_decode returns null this can cause a TypeError. After calling json_decode(Request::getString(...)) ensure $clean_msg_id is an array (e.g. check is_array($clean_msg_id) or use a fallback to an empty array) before calling array_map('intval', ...) and count(), and/or verify json_last_error() to handle invalid JSON; update the logic around $clean_msg_id, array_map and $size to operate on a guaranteed array (referencing the $clean_msg_id variable and the json_decode(Request::getString(...)) call).htdocs/modules/profile/admin/step.php (1)
114-119:⚠️ Potential issue | 🟡 MinorRedundant parameter fetch—use the passed argument instead.
The function receives
$step_das a parameter (line 114), but line 118 re-fetches the value fromRequest::getInt('step_id', 0, 'GET'). This is redundant and could cause inconsistency if the values differ.🐛 Proposed fix
function profile_stepsave_toggle($step_d, $step_save) { $step_save = ($step_save == 1) ? 0 : 1; $handler = xoops_getModuleHandler('regstep'); - $obj = $handler->get(Request::getInt('step_id', 0, 'GET')); + $obj = $handler->get($step_d); $obj->setVar('step_save', $step_save);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/step.php` around lines 114 - 119, The function profile_stepsave_toggle currently ignores its $step_d parameter and re-fetches the step id via Request::getInt('step_id', 0, 'GET'), which can cause inconsistencies; change the code in profile_stepsave_toggle to use the passed-in $step_d when calling $handler->get(...) (i.e., replace Request::getInt(...) with $step_d) and keep the existing toggle logic for $step_save and the $obj->setVar('step_save', $step_save) call.htdocs/modules/pm/readpmsg.php (1)
82-82:⚠️ Potential issue | 🟡 MinorPotential undefined variable access.
$res1and$res2are only set inside their respectiveifblocks (lines 66-72 and 74-81). If neither condition is true, line 82 accesses undefined variables, causing a warning/notice in PHP 8.x.🛡️ Proposed fix—initialize before the conditionals
case 'save': + $res1 = false; + $res2 = false; if ($pm->getVar('to_userid') == $GLOBALS['xoopsUser']->getVar('uid')) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/pm/readpmsg.php` at line 82, Initialize $res1 and $res2 before the conditional blocks to avoid undefined variable notices: add initial assignments (e.g., set both to true) before the if statements that set them, so the later expression $res = $res1 && $res2; always reads defined values; update the variables near where they are currently assigned to ensure existing logic is preserved.htdocs/install/page_pathsettings.php (1)
115-149:⚠️ Potential issue | 🔴 CriticalNever execute an autoloader from a user-supplied path in
checkpath.After only an
is_dir()check, this GET branch echoes the absolute autoloader path and theninclude_onces$newPath/vendor/autoload.php. That turns an installer-time validator into arbitrary local code execution and leaks server paths. Resolve the path withrealpath(), enforce an allowed boundary, validate existence, and stop at reporting here—do not execute the autoloader in this request.🔒 Safer direction for this branch
- $composerAutoloader = $xoopsTrustPath . '/vendor/autoload.php'; - echo "$composerAutoloader"; - if (!file_exists($composerAutoloader)) { - echo "Error: Could not find the Composer autoloader in the specified path."; + $realTrustPath = realpath($newPath); + if ($realTrustPath === false || !is_dir($realTrustPath)) { + echo 'Error: Invalid library directory.'; exit(); } - - // Include the autoloader only once - include_once $composerAutoloader; + $composerAutoloader = $realTrustPath . '/vendor/autoload.php'; + if (!is_file($composerAutoloader)) { + echo 'Error: Could not find ' . basename($composerAutoloader) . '.'; + exit(); + }As per coding guidelines, "Validate file paths with realpath() and boundary checks to prevent directory traversal" and "use basename() in error messages to avoid exposing server paths".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/install/page_pathsettings.php` around lines 115 - 149, The GET 'checkpath' branch currently echoes and include_once's a user-supplied autoloader path (variables: $newPath, $composerAutoloader, $pathKey) which leaks server paths and allows code execution; fix by resolving $newPath with realpath(), ensure it is inside an allowed boundary (e.g. compare realpath($newPath) startsWith a known installer root or XOOPS_ROOT_PATH) and reject if realpath() is false or outside the boundary, verify file_exists($composerAutoloader) but do NOT include or execute it, avoid echoing absolute paths (use basename($composerAutoloader) or a generic message), and only update session/ call $pathController->updateXoopsTrustPath() after the path is fully validated; remove the include_once line and replace direct path echoes with safe messages before calling genPathCheckHtml($pathKey, $pathController->checkPath($pathKey)).htdocs/modules/profile/search.php (1)
186-233:⚠️ Potential issue | 🔴 CriticalReplace this ad-hoc SQL block with
$criteria->add(new Criteria(...)).The code attempts to destructure
Criteria::render()as an array ([$clause, $params]), but it returns a string. More critically, it calls$xoopsDB->prepare(),bindValue(), andfetchAll()— methods that do not exist in the XOOPSXoopsDatabaseabstraction. This will fatal error at runtime.Instead, use
$criteria->add(new Criteria('uname', $uname, 'LIKE'))to stay consistent with the CriteriaCompo pattern, the rest of the function, and the Handler API used throughout XOOPS:Corrected approach
- $criteria = new Criteria('uname', $uname, 'LIKE'); - [$clause, $params] = $criteria->render(); - - // Prepare and execute the SQL query - $sql = "SELECT * FROM " . $xoopsDB->prefix('users') . " WHERE " . $clause; - $stmt = $xoopsDB->prepare($sql); - - foreach ($params as $placeholder => $value) { - $stmt->bindValue($placeholder, $value); - } - - $stmt->execute(); - $results = $stmt->fetchAll(); - - // Process results - $search_url = []; - $searchvars = []; - - if ($results) { - foreach ($results as $row) { - // Populate search URL and search variables based on the results - $search_url[] = 'uname=' . urlencode($row['uname']); - $search_url[] = 'uname_match=' . urlencode((string) $uname_match); - $searchvars[] = 'uname'; - } - } + $criteria->add(new Criteria('uname', $uname, 'LIKE')); + $searchvars[] = 'uname'; + $search_url[] = 'uname=' . rawurlencode($uname); + $search_url[] = 'uname_match=' . rawurlencode((string) $uname_match);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/search.php` around lines 186 - 233, The code incorrectly treats Criteria::render() as returning an array and uses low-level PDO-style calls ($xoopsDB->prepare(), bindValue(), fetchAll()) which don't exist on the XOOPS DB abstraction; replace the ad-hoc SQL and PDO logic by adding a Criteria to the existing Criteria/CriteriaCompo and using the Handler API (e.g., $criteria->add(new Criteria('uname', $uname, 'LIKE')) and then fetch users via the member handler like $memberHandler->getUsers($criteria) or equivalent), removing the $xoopsDB->prepare/bindValue/execute/fetchAll block and building $search_url/$searchvars from the returned user objects/arrays.htdocs/install/page_langselect.php (1)
33-39: 🧹 Nitpick | 🔵 TrivialMigration looks correct; consider validating language input.
The migration to
\Xmf\Request::hasVar()andgetString()is correct, and addingexit()after the redirect is good practice for terminating script execution.However, the
$langvalue from POST is set directly into a cookie without validation against the available languages list. While this is likely existing behavior, it could be hardened:🛡️ Optional: Validate language before setting cookie
if ($_SERVER['REQUEST_METHOD'] === 'POST' && \Xmf\Request::hasVar('lang', 'POST')) { $lang = \Xmf\Request::getString('lang', '', 'POST'); + $availableLanguages = getDirList(__DIR__ . '/../language/'); + if (!in_array($lang, $availableLanguages, true)) { + $lang = 'english'; + } xoops_setcookie('xo_install_lang', $lang, 0, '', '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/install/page_langselect.php` around lines 33 - 39, Validate the POSTed language before writing the cookie: after retrieving $lang via \Xmf\Request::getString('lang', '', 'POST'), check it against the available languages array (the same source used by the installer UI) and only call xoops_setcookie('xo_install_lang', $lang, 0, '', '') when the value is present in that list; if invalid, fall back to a default language or skip setting the cookie and still perform $wizard->redirectToPage('+1') (use the existing functions or array that enumerate languages to perform the check).htdocs/include/comment_delete.php (1)
34-44: 🧹 Nitpick | 🔵 TrivialIncomplete migration:
filter_input_arrayand$_POSTaccess remain.Lines 34-44 still use
filter_input_array(INPUT_POST, ...)andfilter_input_array(INPUT_GET, ...)with a direct$_POSTcheck. For consistency with this PR's migration goals and coding guidelines, these should also be migrated toXmf\Requestmethods.♻️ Suggested migration
-if (!empty($_POST)) { - $result = filter_input_array(INPUT_POST, $filters); -} else { - $result = filter_input_array(INPUT_GET, $filters); -} -$com_mode = $result['com_mode'] ?: 'flat'; -$com_order = $result['com_order'] ?: XOOPS_COMMENT_OLD1ST; -$com_id = $result['com_id'] ?: 0; -if ($result['op']) { - $op = $result['op']; -} +$com_mode = \Xmf\Request::getString('com_mode', 'flat', 'REQUEST'); +$com_order = \Xmf\Request::getInt('com_order', XOOPS_COMMENT_OLD1ST, 'REQUEST'); +$com_id = \Xmf\Request::getInt('com_id', 0, 'REQUEST'); +$op = \Xmf\Request::getCmd('op', 'delete', 'REQUEST');As per coding guidelines: "All user input must be filtered using Xmf\Request::getVar() or Xmf\FilterInput::clean() — never access $_GET, $_POST, or $_REQUEST directly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/include/comment_delete.php` around lines 34 - 44, The code still uses filter_input_array and a raw $_POST check; replace the filter_input_array(INPUT_POST/INPUT_GET, $filters) flow with Xmf\Request helpers: fetch the POST data via Xmf\Request::getArray('POST') or read each expected field using Xmf\Request::getString/getInt/getArray for the keys used (so $result is not populated from filter_input_array), then set $com_mode, $com_order, $com_id and $op using Xmf\Request::getString/getInt (or default values) instead of relying on $result; update any logic that checks $_POST (the initial if (!empty($_POST)) block) to use Xmf\Request methods to detect POST vs GET input and ensure $result/$com_* variables are populated from Xmf\Request calls rather than filter_input_array or $_POST.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 77300eda-0882-4a31-958a-43e8c43495af
📒 Files selected for processing (92)
htdocs/Frameworks/art/functions.ini.phphtdocs/admin.phphtdocs/ajaxfineupload.phphtdocs/banners.phphtdocs/class/smarty3_plugins/function.xoops_link.phphtdocs/class/theme.phphtdocs/class/xoopseditor/sampleform.inc.phphtdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsemotions/xoopsemotions.phphtdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsimagemanager/xoopsimagebrowser.phphtdocs/class/xoopseditor/tinymce5/js/tinymce/plugins/xoopsemoticons/xoopsemoticons.phphtdocs/class/xoopseditor/tinymce5/js/tinymce/plugins/xoopsimagemanager/xoopsimagemanager.phphtdocs/class/xoopseditor/tinymce7/js/tinymce/plugins/xoopsemoticons/xoopsemoticons.phphtdocs/class/xoopseditor/tinymce7/js/tinymce/plugins/xoopsimagemanager/xoopsimagemanager.phphtdocs/class/xoopskernel.phphtdocs/class/xoopssecurity.phphtdocs/edituser.phphtdocs/image.phphtdocs/imagemanager.phphtdocs/include/comment_delete.phphtdocs/include/comment_edit.phphtdocs/include/comment_form.phphtdocs/include/comment_new.phphtdocs/include/comment_post.phphtdocs/include/comment_reply.phphtdocs/include/comment_view.phphtdocs/include/common.phphtdocs/include/findusers.phphtdocs/include/formdhtmltextarea_preview.phphtdocs/include/notification_functions.phphtdocs/include/notification_update.phphtdocs/include/site-closed.phphtdocs/install/class/installwizard.phphtdocs/install/page_configsave.phphtdocs/install/page_configsite.phphtdocs/install/page_dbconnection.phphtdocs/install/page_dbsettings.phphtdocs/install/page_langselect.phphtdocs/install/page_moduleinstaller.phphtdocs/install/page_pathsettings.phphtdocs/install/page_siteinit.phphtdocs/install/page_theme.phphtdocs/misc.phphtdocs/modules/pm/admin/prune.phphtdocs/modules/pm/pmlite.phphtdocs/modules/pm/readpmsg.phphtdocs/modules/pm/viewpmsg.phphtdocs/modules/profile/activate.phphtdocs/modules/profile/admin/category.phphtdocs/modules/profile/admin/field.phphtdocs/modules/profile/admin/step.phphtdocs/modules/profile/admin/user.phphtdocs/modules/profile/admin/visibility.phphtdocs/modules/profile/changemail.phphtdocs/modules/profile/changepass.phphtdocs/modules/profile/class/field.phphtdocs/modules/profile/edituser.phphtdocs/modules/profile/preloads/core.phphtdocs/modules/profile/register.phphtdocs/modules/profile/search.phphtdocs/modules/protector/index.phphtdocs/modules/protector/notification_update.phphtdocs/modules/system/admin/avatars/main.phphtdocs/modules/system/admin/banners/main.phphtdocs/modules/system/admin/blocksadmin/main.phphtdocs/modules/system/admin/comments/main.phphtdocs/modules/system/admin/groupperm.phphtdocs/modules/system/admin/groups/main.phphtdocs/modules/system/admin/images/main.phphtdocs/modules/system/admin/mailusers/main.phphtdocs/modules/system/admin/maintenance/main.phphtdocs/modules/system/admin/modulesadmin/main.phphtdocs/modules/system/admin/preferences/main.phphtdocs/modules/system/admin/smilies/main.phphtdocs/modules/system/admin/tplsets/jquery.phphtdocs/modules/system/admin/tplsets/main.phphtdocs/modules/system/admin/userrank/main.phphtdocs/modules/system/admin/users/main.phphtdocs/modules/system/themes/dark/dark.phphtdocs/modules/system/themes/default/default.phphtdocs/modules/system/themes/transition/transition.phphtdocs/notifications.phphtdocs/pmlite.phphtdocs/readpmsg.phphtdocs/register.phphtdocs/search.phphtdocs/viewpmsg.phphtdocs/xoops_lib/modules/protector/admin.phphtdocs/xoops_lib/modules/protector/admin/center.phphtdocs/xoops_lib/modules/protector/admin/prefix_manager.phphtdocs/xoops_lib/modules/protector/filters_disabled/postcommon_register_insert_js_check.phphtdocs/xoops_lib/modules/protector/module_icon.phptests/unit/htdocs/migration/SuperglobalMigrationTest.php
💤 Files with no reviewable changes (1)
- htdocs/modules/system/admin/users/main.php
.../class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsemotions/xoopsemotions.php
Outdated
Show resolved
Hide resolved
.../class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsemotions/xoopsemotions.php
Outdated
Show resolved
Hide resolved
| if (\Xmf\Request::hasVar('op', 'GET')) { | ||
| $op = \Xmf\Request::getString('op', '', 'GET'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='htdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsimagemanager/xoopsimagebrowser.php'
echo 'Raw POST hydration still present:'
rg -n -C2 '\$_POST|\$\{\$k\}' "$file"
echo
echo 'Touched branches that still depend on that state:'
rg -n -C1 'Request::hasVar\('\''op'\'', '\''(GET|POST)'\''\)|count\(\$image_id\)' "$file"Repository: XOOPS/XoopsCore25
Length of output: 1596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='htdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsimagemanager/xoopsimagebrowser.php'
echo '=== POST hydration and early context (lines 25-50) ==='
sed -n '25,50p' "$file"
echo
echo '=== count($image_id) context (lines 100-120) ==='
sed -n '100,120p' "$file"
echo
echo '=== Variable initializations before POST hydration ==='
sed -n '15,35p' "$file"Repository: XOOPS/XoopsCore25
Length of output: 2614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='htdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsimagemanager/xoopsimagebrowser.php'
# Verify which variables are consumed in the 'save' operation
echo '=== Variables used in save branch (lines 100-130) ==='
sed -n '100,130p' "$file" | grep -E '\$image_id|\$image_display|\$image_weight|\$image_nicename|\$imgcat_id'
echo
echo '=== Check for any filtering/escaping before DB insert ==='
sed -n '100,130p' "$file" | grep -E 'FilterInput|htmlspecialchars|setVar|insert'Repository: XOOPS/XoopsCore25
Length of output: 1069
Complete the POST migration: retrieve all POST fields explicitly via Request instead of mass-assigning from raw $_POST.
The Request::hasVar() guards check for the presence of op in POST, but the actual value—along with $image_id, $image_display, $image_weight, $image_nicename, $imgcat_id, etc.—still flows from the foreach ($_POST as $k => $v) { ${$k} = $v; } block at lines 32–35. These unfiltered variables are passed directly to setVar() and insert() without validation or escaping.
Additionally, PHP 8.2+ enforces strict count() behavior. If $image_id is missing or scalar from POST, line 105's count($image_id) raises a TypeError. Retrieve POST fields explicitly with Request::getArray() or Request::get*() methods, validate their types, and remove the variable-variable hydration.
Applies to all POST operation branches: lines 101–130, 133–191, 194–247, 249–309, 322–360, and similar patterns throughout the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@htdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsimagemanager/xoopsimagebrowser.php`
around lines 77 - 79, Replace the unsafe mass-assignment from $_POST (the
foreach with ${$k} = $v) by explicitly retrieving each expected POST field using
Request helpers (e.g. \Xmf\Request::getInt('image_id', 0, 'POST'),
::getString('image_nicename', '', 'POST'), ::getArray('image_id', [], 'POST') or
::getArray('image_weight', [], 'POST') as appropriate), validate types/lengths
(ensure arrays where count() is used), and remove the variable-variable
hydration; then use those validated variables when calling setVar() and insert()
in the POST handling branches that inspect $op and work with $image_id,
$image_display, $image_weight, $image_nicename, $imgcat_id, etc., so count() is
only called on arrays and all inputs are sanitized before setVar()/insert().
| $op = \Xmf\Request::getCmd('op', '', 'GET'); | ||
| if ($op === '') { | ||
| $op = \Xmf\Request::getCmd('op', '', 'POST'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Correct migration to Xmf\Request – LGTM with optional simplification.
The change properly replaces direct superglobal access with \Xmf\Request::getCmd(), which aligns with the coding guidelines. The explicit GET-then-POST fallback maintains clear priority semantics.
For a slightly more concise approach, you could use the 'REQUEST' hash which combines both:
$op = \Xmf\Request::getCmd('op', '', 'REQUEST');However, the current implementation is perfectly valid if explicit GET-over-POST priority is intentional.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@htdocs/class/xoopseditor/tinymce5/js/tinymce/plugins/xoopsemoticons/xoopsemoticons.php`
around lines 47 - 50, The current code sets $op by first calling
\Xmf\Request::getCmd('op', '', 'GET') and falling back to
\Xmf\Request::getCmd('op', '', 'POST'); if you want a concise equivalent that
preserves the same overall behavior (but accepts any request source with lower
priority to PHP's order), replace the two-step logic with a single call to
\Xmf\Request::getCmd('op', '', 'REQUEST'); otherwise leave the explicit
GET-then-POST sequence as-is to maintain strict GET-over-POST priority.
| if (Request::hasVar('action', 'GET')) { | ||
| $action = trim(strip_tags(Request::getString('action', '', 'GET'))); | ||
| } elseif (!empty($_POST['action'])) { | ||
| } elseif (Request::hasVar('action', 'POST')) { | ||
| $action = trim(strip_tags(Request::getString('action', '', 'POST'))); | ||
| } |
There was a problem hiding this comment.
Preserve the default action when the parameter is empty.
Request::hasVar() is true for ?action=, so Lines 42/44 can replace the safe default 'search' with an empty string. That skips the search-form branch and falls through without a matching switch case, yielding a broken page instead of the default form. Keep 'search' unless the submitted action is non-empty and allowed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/search.php` around lines 41 - 45, Request::hasVar('action', ...)
treats an empty parameter as present, so the current logic can overwrite the
intended default 'search' with an empty string and break the switch handling;
update the assignment of $action in the branches using
Request::getString('action', ...) to only override the default when the returned
string is non-empty and allowed (e.g. trim(strip_tags(...)) !== ''), otherwise
leave $action as the default 'search' (ensure this check is applied for both GET
and POST branches and use the same sanitization used currently).
| if (Request::hasVar('delete_messages', 'POST') && (Request::hasVar('msg_id', 'POST') || Request::hasVar('msg_ids', 'POST'))) { | ||
| if (!$GLOBALS['xoopsSecurity']->check()) { | ||
| echo implode('<br>', $GLOBALS['xoopsSecurity']->getErrors()); | ||
| exit(); | ||
| } elseif (empty($_REQUEST['ok'])) { | ||
| } elseif (Request::getInt('ok', 0) === 0) { | ||
| include $GLOBALS['xoops']->path('header.php'); | ||
| xoops_confirm( | ||
| [ | ||
| 'ok' => 1, | ||
| 'delete_messages' => 1, | ||
| 'msg_ids' => json_encode(array_map('intval', $_POST['msg_id'])), | ||
| 'msg_ids' => json_encode(array_map('intval', Request::getArray('msg_id', [], 'POST'))), | ||
| ], | ||
| $_SERVER['REQUEST_URI'], | ||
| _PM_SURE_TO_DELETE, | ||
| ); | ||
| include $GLOBALS['xoops']->path('footer.php'); | ||
| exit(); | ||
| } | ||
| $clean_msg_id = json_decode($_POST['msg_ids'], true, 2); | ||
| $clean_msg_id = json_decode(Request::getString('msg_ids', '', 'POST'), true, 2); |
There was a problem hiding this comment.
Normalize one message-ID source before confirmation and reuse it.
The branch now accepts either msg_id[] or msg_ids, but the confirmation form always serializes msg_id[] on Line 41 and the delete step only reads msg_ids on Line 49. That breaks callers posting only msg_ids, and msg_id[]-only submissions with ok=1 can still drive null into count() on Line 53. Build one normalized ID array up front, use it for the confirm payload, and reuse the same array after confirmation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/viewpmsg.php` around lines 31 - 49, Normalize the message-ID source
into a single array before the confirmation branch and reuse it for both the
xoops_confirm payload and the deletion step: detect either
Request::getArray('msg_id', ...) or Request::getString('msg_ids', ...) once and
convert to an integer array (e.g. $msgIdsNormalized), use that array when
building the xoops_confirm 'msg_ids' value instead of always serializing
msg_id[], and on confirmation decode/reuse the same normalized array rather than
calling json_decode(Request::getString('msg_ids', ...)) separately; update
references to $clean_msg_id (or replace with $msgIdsNormalized) so count() and
deletion logic operate on a guaranteed array.
| if (Request::hasVar('copy', 'POST') && Request::hasVar('old_prefix', 'POST')) { | ||
| $new_prefix = Request::getCmd('new_prefix', '', 'POST'); | ||
| $old_prefix = Request::getCmd('old_prefix', '', 'POST'); | ||
| if (preg_match('/[^0-9A-Za-z_-]/', $new_prefix)) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the duplicated prefix validation regex to a constant.
The regex /[^0-9A-Za-z_-]/ is repeated three times. Defining a constant improves maintainability and ensures consistency if the pattern needs to change.
♻️ Proposed refactor
Add at the top of the file after the use statement:
const PREFIX_INVALID_CHAR_PATTERN = '/[^0-9A-Za-z_-]/';Then replace each occurrence:
- if (preg_match('/[^0-9A-Za-z_-]/', $new_prefix)) {
+ if (preg_match(PREFIX_INVALID_CHAR_PATTERN, $new_prefix)) {Also applies to: 85-85, 207-207
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 14-14: Define a constant instead of duplicating this literal "/[^0-9A-Za-z_-]/" 3 times.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/xoops_lib/modules/protector/admin/prefix_manager.php` at line 14,
Extract the duplicate regex into a single constant and use it everywhere: define
a constant named PREFIX_INVALID_CHAR_PATTERN (e.g., immediately after the use
statements) with the pattern '/[^0-9A-Za-z_-]/', then replace all literal uses
of that regex (the three occurrences that validate $new_prefix and other prefix
checks in this file) with the constant PREFIX_INVALID_CHAR_PATTERN so the
validation logic in prefix_manager.php references the constant instead of
repeating the literal pattern.
| if (!empty($_GET['file'])) { | ||
| $file_base = preg_replace('/[^0-9a-z_]/', '', Request::getString('file', '', 'GET')); | ||
| } | ||
| $file_base = preg_replace('/[^0-9a-z_]/', '', Request::getString('file', 'module_icon', 'GET')); |
There was a problem hiding this comment.
Restore the default after sanitization.
An input like ?file=!!! sanitizes to '', so the code falls through to .png instead of module_icon.png. Reapply the default when the sanitized basename is empty.
Suggested fix
$file_base = preg_replace('/[^0-9a-z_]/', '', Request::getString('file', 'module_icon', 'GET'));
+if ($file_base === '') {
+ $file_base = 'module_icon';
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $file_base = preg_replace('/[^0-9a-z_]/', '', Request::getString('file', 'module_icon', 'GET')); | |
| $file_base = preg_replace('/[^0-9a-z_]/', '', Request::getString('file', 'module_icon', 'GET')); | |
| if ($file_base === '') { | |
| $file_base = 'module_icon'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/xoops_lib/modules/protector/module_icon.php` at line 27, The sanitized
basename stored in file_base can become empty (e.g. ?file=!!!) causing the
fallback to use ".png" instead of the intended default "module_icon"; after
calling preg_replace on Request::getString in module_icon.php, check if
file_base is an empty string and if so reassign it to the default "module_icon"
so subsequent code builds "module_icon.png" as expected.
| $user->setVar('uname', Request::getString('uname', '', 'POST')); | ||
| $user->setVar('email', Request::getString('email', '', 'POST')); | ||
| $newLevel = Request::getInt('level', -1, 'POST'); | ||
| if ($newLevel !== -1 && $user->getVar('level') != $newLevel) { | ||
| $user->setVar('level', $newLevel); | ||
| } | ||
| $password = $vpass = null; | ||
| if (!empty($_POST['password'])) { | ||
| if (Request::getString('password', '', 'POST') !== '') { | ||
| $password = trim(Request::getString('password', '', 'POST')); | ||
| $vpass = trim(Request::getString('vpass', '', 'POST')); | ||
| $user->setVar('pass', password_hash($password, PASSWORD_DEFAULT)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Password handling is secure, but consider caching the request value.
The password is fetched twice from Request::getString(). While functionally correct, you could store it once:
♻️ Minor optimization
$password = $vpass = null;
-if (Request::getString('password', '', 'POST') !== '') {
- $password = trim(Request::getString('password', '', 'POST'));
+$password = trim(Request::getString('password', '', 'POST'));
+if ($password !== '') {
$vpass = trim(Request::getString('vpass', '', 'POST'));
$user->setVar('pass', password_hash($password, PASSWORD_DEFAULT));
} elseif ($user->isNew()) {
$password = $vpass = '';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/admin/user.php` around lines 117 - 127, The code calls
Request::getString('password', '', 'POST') twice—cache the POST value into a
local variable once and reuse it for $password (and for computing $vpass) before
trimming and hashing; update the block around Request::getString('password', '',
'POST'), the $password/$vpass assignments and the user->setVar('pass',
password_hash(...)) usage to use the cached variable instead of calling
Request::getString twice.
htdocs/modules/profile/edituser.php
Outdated
| $postedAvatar = Request::getString('user_avatar', '', 'POST'); | ||
| if (!empty($postedAvatar)) { | ||
| $user_avatar = $xoopsDB->escape($postedAvatar); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if Criteria class handles escaping internally
ast-grep --pattern $'class Criteria {
$$$
render($$$) {
$$$
}
$$$
}'Repository: XOOPS/XoopsCore25
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find the Criteria class definition
fd -t f "Criteria\.php" --type f | head -20Repository: XOOPS/XoopsCore25
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Also search for Criteria class definition in PHP files
rg "class Criteria" -t php --type-list php | head -20Repository: XOOPS/XoopsCore25
Length of output: 450
🏁 Script executed:
#!/bin/bash
# Search for class Criteria definition
rg "class Criteria[^a-zA-Z]" -t phpRepository: XOOPS/XoopsCore25
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Check the edituser.php file for use statements to understand where Criteria comes from
head -50 htdocs/modules/profile/edituser.php | grep -E "^use |^require|^include"Repository: XOOPS/XoopsCore25
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Read the Criteria class to understand how it handles escaping
cat -n htdocs/class/criteria.phpRepository: XOOPS/XoopsCore25
Length of output: 22868
Remove redundant database escaping—let Criteria handle it.
Criteria::render() already applies $db->quote() to all values (line 583 for standard operators, line 459 for IN, line 557 for LIKE). The explicit $xoopsDB->escape() call creates double escaping and is unnecessary.
♻️ Simplify to
$postedAvatar = Request::getString('user_avatar', '', 'POST');
if (!empty($postedAvatar)) {
- $user_avatar = $xoopsDB->escape($postedAvatar);
+ $user_avatar = $postedAvatar;
$criteria_avatar = new CriteriaCompo(new Criteria('avatar_file', $user_avatar));The Criteria class handles all escaping via $db->quote() when render() builds the SQL fragment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/edituser.php` around lines 213 - 215, The code
redundantly escapes the posted avatar string before passing it to the Criteria
builder; remove the explicit $xoopsDB->escape() call and assign the raw
Request::getString('user_avatar', '', 'POST') value to $user_avatar so
Criteria::render() (in the Criteria class) can perform proper quoting/escaping
itself; update the block that currently uses $postedAvatar and $xoopsDB->escape
to simply set $user_avatar from the Request value and rely on Criteria::render()
for escaping.
|
|
||
| case 'delete': | ||
| if ($_REQUEST['id'] == $GLOBALS['xoopsUser']->getVar('uid')) { | ||
| $deleteId = Request::getInt('id', 0); |
There was a problem hiding this comment.
In htdocs/modules/profile/admin/user.php, the delete case at line 199 reads $deleteId = Request::getInt('id', 0) without specifying a hash. The initial delete request is a GET (from a link), but the POST confirmation also needs the id. Both phases are covered by the default REQUEST hash, which is correct per the admin delete flow pattern. However, this is inconsistent with the explicit hash style used elsewhere in this PR. Consider using 'GET' for the initial retrieval and 'POST' for the ok-confirmation path. In the current code, the same $deleteId variable is used for both, so the default REQUEST hash is the simplest correct choice.
htdocs/modules/profile/search.php
Outdated
|
|
||
| $limit_default = 20; | ||
| $op = $_REQUEST['op'] ?? 'search'; | ||
| $op = Request::getCmd('op', 'search', 'POST'); |
There was a problem hiding this comment.
The op variable in htdocs/modules/profile/search.php is read exclusively from POST (Request::getCmd('op', 'search', 'POST')), but the original code used $_REQUEST['op'] which reads from both GET and POST. The search form in the search case uses method POST and submits op=results, which is fine. However, if any link or redirect passes op via GET (e.g., pagination links that reconstruct the URL), the results case will no longer be triggered. Verify that the results case is always triggered exclusively by a POST form submission and is never linked to via GET.
Additionally, uname, uname_match, email, email_match, sortby, selgroups, order, limit, and start are all now read from POST only, whereas the original code used $_REQUEST. If any external link or URL bookmark included these as GET parameters, they will no longer be processed.
| $sslSessionId = \Xmf\Request::getString($xoopsConfig['sslpost_name'], '', 'POST'); | ||
| if ($xoopsConfig['use_ssl'] && $sslSessionId !== '' && preg_match('/^[a-zA-Z0-9,-]{22,256}$/', $sslSessionId)) { | ||
| session_id($sslSessionId); | ||
| } elseif ($xoopsConfig['use_mysession'] && $xoopsConfig['session_name'] != '' && $xoopsConfig['session_expire'] > 0) { |
There was a problem hiding this comment.
The code reads $sslSessionId directly from a POST parameter named by $xoopsConfig['sslpost_name'] and passes it to session_id() whenever use_ssl is enabled. This allows an attacker to fix a victim’s session ID by sending a crafted POST request with a chosen ID before the victim authenticates, then reuse that same session after login to hijack the account. To mitigate, do not accept session IDs from user input at all, or at minimum bind them to a server-generated, one-time token and ignore arbitrary values that do not match a server-side expectation.
| $sslSessionId = \Xmf\Request::getString($xoopsConfig['sslpost_name'], '', 'POST'); | |
| if ($xoopsConfig['use_ssl'] && $sslSessionId !== '' && preg_match('/^[a-zA-Z0-9,-]{22,256}$/', $sslSessionId)) { | |
| session_id($sslSessionId); | |
| } elseif ($xoopsConfig['use_mysession'] && $xoopsConfig['session_name'] != '' && $xoopsConfig['session_expire'] > 0) { | |
| if ($xoopsConfig['use_mysession'] && $xoopsConfig['session_name'] != '' && $xoopsConfig['session_expire'] > 0) { |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
htdocs/modules/system/admin/banners/main.php (3)
190-190:⚠️ Potential issue | 🔴 CriticalBug: Trailing space in field name 'passwd ' will break password saving.
Both the database field name and the request parameter contain a trailing space (
'passwd 'instead of'passwd'). This will:
- Fail to retrieve the password from the POST data (form field is likely named
passwd)- Attempt to set a non-existent database column
🐛 Proposed fix
- $obj->setVar('passwd ', Request::getString('passwd ', '')); + $obj->setVar('passwd', Request::getString('passwd', ''));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/banners/main.php` at line 190, The bug is caused by a trailing space in the field name; change the usage of 'passwd ' to 'passwd' in both places so the request parameter and DB field match. Locate the call to $obj->setVar('passwd ', Request::getString('passwd ', '')) and update the first argument to 'passwd' and the Request::getString key to 'passwd' so $obj->setVar and Request::getString use the correct field name. Ensure there are no other occurrences of 'passwd ' elsewhere in the file/class and run the save flow to verify password persists.
311-314:⚠️ Potential issue | 🟡 MinorUndefined variable risk:
$namemay not be set when used on line 354.The variable
$nameis only assigned inside theif (is_object($name_client))block, but it's used unconditionally on line 354 ($banner['name'] = $name). If the client object doesn't exist,$namewill be undefined, triggering anE_WARNINGin PHP 8.x.🛡️ Proposed fix
$name_client = $banner_client_Handler->get($banner_arr[$i]->getVar('cid')); + $name = ''; if (is_object($name_client)) { $name = $name_client->getVar('name'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/banners/main.php` around lines 311 - 314, Initialize a default value for $name before the is_object($name_client) check so $banner['name'] won't be undefined; ensure the block that retrieves $name from $name_client->getVar('name') only overrides that default, referencing the same $name_client (result of get($banner_arr[$i]->getVar('cid'))) and the later use $banner['name'] to avoid E_WARNING when the client object is missing.
397-398:⚠️ Potential issue | 🟠 MajorPotential TypeError: Missing null check before accessing
$name_client.The
$banner_client_Handler->get()method can returnfalseornullif the client doesn't exist (e.g., orphaned banner record referencing a deleted client). Calling->getVar('name')on a non-object will throw aTypeErrorin PHP 8.x.This is inconsistent with lines 311-314 where
is_object($name_client)is properly checked.🛡️ Proposed fix
$banner_finish['datestart'] = formatTimestamp($banner_finish_arr[$i]->getVar('datestart'), 'm'); $banner_finish['dateend'] = formatTimestamp($banner_finish_arr[$i]->getVar('dateend'), 'm'); $name_client = $banner_client_Handler->get($banner_finish_arr[$i]->getVar('cid')); - $banner_finish['name'] = $name_client->getVar('name'); + $banner_finish['name'] = is_object($name_client) ? $name_client->getVar('name') : _AM_SYSTEM_BANNERS_UNKNOWN_CLIENT;Alternatively, using PHP 8.0+ nullsafe operator:
- $banner_finish['name'] = $name_client->getVar('name'); + $banner_finish['name'] = $name_client?->getVar('name') ?? '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/banners/main.php` around lines 397 - 398, The code calls $banner_client_Handler->get(...) and then immediately $name_client->getVar('name') without checking that $name_client is an object, which can cause a TypeError; update the block around where $banner_finish_arr and $banner_client_Handler are used (the lines creating $name_client and setting $banner_finish['name']) to guard against null/false by checking is_object($name_client) (or use the PHP nullsafe operator on $name_client) before calling getVar('name'), and set a sensible fallback (e.g., empty string or 'unknown') when the client is missing.htdocs/modules/system/admin/groupperm.php (2)
51-51: 🧹 Nitpick | 🔵 TrivialConsider strict comparison for type safety.
Line 51 uses loose equality
$selected == 1, which would also match'1',true, or1.0. If the expected value is strictly integer1, prefer=== 1. If it may be a string from form data,=== '1'or explicit casting would be more explicit.Same applies to line 57 with
$pid != 0.♻️ Strict comparison examples
- if ($selected == 1) { + if ((int) $selected === 1) {- if ($pid != 0 && !array_key_exists($pid, $item_ids)) { + if ((int) $pid !== 0 && !array_key_exists($pid, $item_ids)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/groupperm.php` at line 51, The loose comparisons should be made strict: change the conditional using $selected == 1 to use a strict check (either === 1 if $selected is an int or === '1' / (int)$selected === 1 if it comes from form input) and likewise change $pid != 0 to a strict comparison ($pid !== 0 or $pid !== '0' or (int)$pid !== 0) so type mismatches don’t pass; update the conditionals in groupperm.php where $selected and $pid are used (replace the == and != operators with the appropriate === / !== or explicit casts) to ensure correct type-safe behavior.
87-91:⚠️ Potential issue | 🟠 MajorValidate
redirect_urlto prevent path traversal.The user-supplied
redirect_urlis concatenated directly into the URL path without validation. An attacker could submit values like../../admin.phporindex.php?x=to manipulate the redirect destination within the site structure.Consider sanitizing by removing traversal sequences and ensuring it's a simple relative path (no
.., no protocol, no leading slash).🛡️ Proposed fix with path validation
- $adminindex = Request::getString('redirect_url', '', 'POST') ?: (string) $module->getInfo('adminindex'); + $redirectUrl = Request::getString('redirect_url', '', 'POST'); + // Sanitize: only allow safe relative paths (no traversal, no protocol, no leading slash) + if ($redirectUrl !== '' && !preg_match('#(?:^/|\.\.|\://|[\x00-\x1f])#', $redirectUrl)) { + $adminindex = $redirectUrl; + } else { + $adminindex = (string) $module->getInfo('adminindex'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/groupperm.php` around lines 87 - 91, Validate and sanitize the user-provided redirect URL returned from Request::getString('redirect_url', ...) before using it to build $backlink: check the value in $adminindex for any path traversal or unsafe characters (disallow sequences like '..', leading slashes, URL schemes such as "http:"/"://", query strings, or other path separators), only accept a simple relative filename (e.g., alphanumerics, dots, dashes, underscores, optionally a .php extension) and if the value fails validation fall back to $module->getInfo('adminindex'); update the logic around adminindex and $backlink (and keep using $module->getVar('dirname')) so untrusted input can never inject traversal or protocol content into the constructed URL.htdocs/modules/system/admin/comments/main.php (2)
184-193: 🧹 Nitpick | 🔵 TrivialRedundant count check after
!empty().Line 187's
if ($commentslist_count > 0)is always true at that point because line 185 already ensures!empty($commentslist_id). This adds unnecessary nesting.♻️ Simplified iteration
$commentslist_id = Request::getArray('commentslist_id', [], 'POST'); if (!empty($commentslist_id)) { - $commentslist_count = count($commentslist_id); - if ($commentslist_count > 0) { - for ($i = 0; $i < $commentslist_count; ++$i) { - $criteria->add(new Criteria('com_id', $commentslist_id[$i]), 'OR'); - } + foreach ($commentslist_id as $comId) { + $criteria->add(new Criteria('com_id', (int)$comId), 'OR'); } $verif = true; }Using
foreachis more idiomatic and eliminates the redundant count check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/comments/main.php` around lines 184 - 193, The block unnecessarily checks count after !empty($commentslist_id); simplify by removing the redundant if ($commentslist_count > 0) branch and iterate directly over $commentslist_id (e.g., using foreach) to add Criteria('com_id', $id) entries to $criteria; update the loop that currently uses $commentslist_count and index-based access to use each $id from $commentslist_id and call $criteria->add(new Criteria('com_id', $id), 'OR') so the logic remains identical but cleaner.
168-183: 🧹 Nitpick | 🔵 TrivialMisleading variable names in group iteration.
The variable names
$deland$u_nameare confusing:
$delis just a numeric array index (not a "delete" indicator)$u_nameholds a group ID (not a username)This makes the code harder to understand and maintain.
♻️ Clearer variable naming
$comments_groupe = Request::getArray('comments_groupe', [], 'POST'); if (!empty($comments_groupe)) { - foreach ($comments_groupe as $del => $u_name) { + foreach ($comments_groupe as $groupId) { /** `@var` XoopsMemberHandler $member_handler */ $member_handler = xoops_getHandler('member'); - $members = $member_handler->getUsersByGroup($u_name, true); + $members = $member_handler->getUsersByGroup((int)$groupId, true); $mcount = count($members); if ($mcount > 4000) { redirect_header('admin.php?fct=comments', 2, _MP_DELETECOUNT);Since the array index
$delis never used, a simplerforeach ($comments_groupe as $groupId)improves readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/comments/main.php` around lines 168 - 183, The foreach over $comments_groupe uses misleading variable names $del and $u_name; change the loop to use a clear name like $groupId (e.g., foreach ($comments_groupe as $groupId)) and update all uses inside the loop (the call to $member_handler->getUsersByGroup($u_name, true) and the Criteria creation that references $members[$i]->getVar('uid')) to use $groupId instead; keep $member_handler, $members, getUsersByGroup, and the Criteria construction unchanged otherwise.htdocs/modules/profile/admin/step.php (1)
114-119: 🧹 Nitpick | 🔵 TrivialFunction parameter
$step_dis ignored; use it instead of re-reading from request.The function receives
$step_d(step ID) as its first parameter but discards it, callingRequest::getInt('step_id', 0, 'GET')again at line 118. This is redundant and creates a subtle inconsistency with the reference implementation infield.php(Context snippet 1), which correctly uses its passed$field_idparameter.♻️ Use the passed parameter
function profile_stepsave_toggle($step_d, $step_save) { $step_save = ($step_save == 1) ? 0 : 1; $handler = xoops_getModuleHandler('regstep'); - $obj = $handler->get(Request::getInt('step_id', 0, 'GET')); + $obj = $handler->get($step_d); $obj->setVar('step_save', $step_save);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/step.php` around lines 114 - 119, The function profile_stepsave_toggle currently ignores its first parameter $step_d and re-reads the step id from Request::getInt; change it to use the passed-in $step_d when fetching the object: replace the Request::getInt('step_id', ...) call in the xoops_getModuleHandler('regstep')->get(...) invocation with the $step_d parameter so profile_stepsave_toggle($step_d, $step_save) consistently uses the provided step id before calling $obj->setVar('step_save', $step_save).htdocs/modules/pm/viewpmsg.php (1)
58-62:⚠️ Potential issue | 🟠 MajorGuard the decoded
msg_idspayload before using it as an array.
xoops_confirm()sendsmsg_idsback as a hidden POST field, so this value is still client-controlled on the second submit. If it is tampered into invalid or scalar JSON, the array logic below will fail before the delete loop runs.♻️ Proposed fix
- $clean_msg_id = json_decode(Request::getString('msg_ids', '', 'POST'), true, 2); - if (!empty($clean_msg_id)) { - $clean_msg_id = array_map('intval', $clean_msg_id); - } + $clean_msg_id = json_decode(Request::getString('msg_ids', '', 'POST'), true, 2); + if (!is_array($clean_msg_id)) { + $clean_msg_id = []; + } else { + $clean_msg_id = array_map('intval', $clean_msg_id); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/pm/viewpmsg.php` around lines 58 - 62, Validate the decoded msg_ids payload from Request::getString before treating it as an array: after $clean_msg_id = json_decode(Request::getString('msg_ids', '', 'POST'), true, 2) ensure $clean_msg_id is an array (e.g. is_array($clean_msg_id)) and if not, set it to an empty array; only call array_map('intval', $clean_msg_id) and count($clean_msg_id) when $clean_msg_id is an array so downstream loops that use $clean_msg_id and $size won’t fail on tampered or scalar JSON payloads.
♻️ Duplicate comments (5)
htdocs/install/page_theme.php (1)
38-42:⚠️ Potential issue | 🟠 MajorReject incomplete POST payloads before updating themes.
This still accepts a malformed POST: if the config-specific field is missing,
getString(..., '', 'POST')collapses it to'', and Line 50 then propagates that empty theme to all users. Whenconf_idsis absent or empty,$new_valueis also never initialized before Line 50 reuses it. Guard the per-config field withRequest::hasVar()and only callupdateUsersByField()for a present, non-empty value.🐛 Proposed fix
if ($_SERVER['REQUEST_METHOD'] === 'POST') { /** `@var` XoopsConfigHandler $config_handler */ $config_handler = xoops_getHandler('config'); + $newValue = null; if (\Xmf\Request::hasVar('conf_ids', 'POST')) { $confIds = \Xmf\Request::getArray('conf_ids', [], 'POST'); - foreach ($confIds as $key => $conf_id) { + foreach ($confIds as $conf_id) { $config = $config_handler->getConfig((int) $conf_id); - $new_value = \Xmf\Request::getString($config->getVar('conf_name'), '', 'POST'); - $config->setConfValueForInput($new_value); + $confName = $config->getVar('conf_name'); + if (!\Xmf\Request::hasVar($confName, 'POST')) { + continue; + } + $newValue = \Xmf\Request::getString($confName, '', 'POST'); + $config->setConfValueForInput($newValue); $config_handler->insertConfig($config); } } @@ - $member_handler->updateUsersByField('theme', $new_value); + if (null !== $newValue && '' !== $newValue) { + $member_handler->updateUsersByField('theme', $newValue); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/install/page_theme.php` around lines 38 - 42, The loop that reads per-config POST fields using \Xmf\Request::getString(...) can accept missing or empty fields and then propagate an empty theme via updateUsersByField; also $new_value may be uninitialized if conf_ids is absent/empty. Fix by guarding each config-specific field with \Xmf\Request::hasVar($config->getVar('conf_name'), 'POST') and only call \Xmf\Request::getString(...) and config_handler->updateUsersByField(...) when that hasVar check passes and the retrieved value is not an empty string; also ensure you skip the entire loop if $confIds is empty so $new_value is not used uninitialized. Use the identifiers $confIds, $new_value, $config_handler->getConfig(...), $config->getVar('conf_name'), \Xmf\Request::hasVar, \Xmf\Request::getString and updateUsersByField in your changes.htdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsemotions/xoopsemotions.php (1)
66-74: 🧹 Nitpick | 🔵 TrivialInconsistent namespace usage: FQN vs imported alias.
The file imports
use Xmf\Request;at line 15, yet lines 66 and 74 use the fully-qualified\Xmf\Request::while lines 46–47 and 72–73 use the importedRequest::alias. For consistency (and brevity), prefer the alias throughout since theusestatement is already present.♻️ Proposed fix for consistency
- $uploadFiles = \Xmf\Request::getArray('xoops_upload_file', [], 'POST'); + $uploadFiles = Request::getArray('xoops_upload_file', [], 'POST');- $smile_display = \Xmf\Request::getInt('smile_display', 0, 'POST') > 0 ? 1 : 0; + $smile_display = Request::getInt('smile_display', 0, 'POST') > 0 ? 1 : 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsemotions/xoopsemotions.php` around lines 66 - 74, The code mixes fully-qualified \Xmf\Request:: calls with the imported Request alias; update the uses of \Xmf\Request::getArray(...) and \Xmf\Request::getInt(...) in the xoopsemotions plugin block to use the imported Request::getArray(...) and Request::getInt(...) respectively so the file consistently uses the Request alias declared by use Xmf\Request; (leave existing Request::getString(...) usages unchanged).htdocs/search.php (1)
41-45:⚠️ Potential issue | 🟡 MinorEmpty action parameter still overwrites safe default.
Request::hasVar('action', 'GET')returnstruefor?action=(empty value), causing$actionto become an empty string instead of preserving the default'search'. This breaks theswitchat line 130 since no case matches, resulting in a page that renders with incomplete content.The fix should ensure a non-empty, valid action before overwriting the default:
🐛 Proposed fix
$action = 'search'; -if (Request::hasVar('action', 'GET')) { - $action = trim(strip_tags(Request::getString('action', '', 'GET'))); -} elseif (Request::hasVar('action', 'POST')) { - $action = trim(strip_tags(Request::getString('action', '', 'POST'))); -} +$actionInput = ''; +if (Request::hasVar('action', 'GET')) { + $actionInput = trim(strip_tags(Request::getString('action', '', 'GET'))); +} elseif (Request::hasVar('action', 'POST')) { + $actionInput = trim(strip_tags(Request::getString('action', '', 'POST'))); +} +if ($actionInput !== '') { + $action = $actionInput; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/search.php` around lines 41 - 45, The current logic assigns $action to an empty string when Request::hasVar('action', ...) is true for cases like ?action=; change the assignment so that you only overwrite the default $action ('search') when the retrieved value from Request::getString('action', ...) is non-empty after trimming/strip_tags; i.e., call Request::getString, trim/strip_tags it, then check that the result is not an empty string before setting $action (referencing Request::hasVar, Request::getString and the $action variable used by the switch).htdocs/include/notification_update.php (1)
43-46:⚠️ Potential issue | 🟠 MajorNormalize relative return paths and validate origin more strictly.
notification_select.phpsubmitsxoops_getenv('PHP_SELF'), so validnot_redirectvalues can be relative paths. This check sends those users back toXOOPS_URL . '/'instead of the originating page, and a plain prefix match is still too weak for same-origin validation. Accept leading-/paths, normalize them toXOOPS_URL, and compare parsed scheme/host/port for absolute URLs.🔐 Proposed fix
-$not_redirect = Request::getUrl('not_redirect', XOOPS_URL . '/', 'POST'); -if (strpos($not_redirect, XOOPS_URL) !== 0) { - $not_redirect = XOOPS_URL . '/'; -} +$not_redirect = Request::getString('not_redirect', '/', 'POST'); +if (str_starts_with($not_redirect, '/')) { + $not_redirect = XOOPS_URL . $not_redirect; +} else { + $redirectParts = parse_url($not_redirect); + $baseParts = parse_url(XOOPS_URL); + $sameOrigin = is_array($redirectParts) + && is_array($baseParts) + && ($redirectParts['scheme'] ?? null) === ($baseParts['scheme'] ?? null) + && ($redirectParts['host'] ?? null) === ($baseParts['host'] ?? null) + && ($redirectParts['port'] ?? null) === ($baseParts['port'] ?? null); + + if (!$sameOrigin) { + $not_redirect = XOOPS_URL . '/'; + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/include/notification_update.php` around lines 43 - 46, The current $not_redirect validation rejects valid relative paths and uses a weak prefix check; update the logic around Request::getUrl('not_redirect', XOOPS_URL . '/', 'POST') to accept leading-absolute-paths (values starting with '/') by normalizing them to XOOPS_URL + path (so "/foo" becomes XOOPS_URL . "/foo") and to more strictly validate absolute URLs by parsing both the provided URL and XOOPS_URL and comparing scheme, host, and port; if the value is neither a normalized leading-"/" path nor an absolute URL with matching scheme/host/port, fall back to XOOPS_URL . '/'. Reference $not_redirect, Request::getUrl, XOOPS_URL and notification_select.php/xoops_getenv('PHP_SELF') when locating where to change the validation.htdocs/modules/profile/register.php (1)
170-173:⚠️ Potential issue | 🟠 MajorUse
Request::getVar()for required dynamic fields too.Line 172 still validates dynamic profile fields with
Request::getString(). That coercescheckbox,select_multi, andgroup_multisubmissions away from their original array shape, so required-field validation can still misbehave even though the merge/save path now usesgetVar(). Based on learnings: "Applies to **/*.php : All user input must be filtered using Xmf\Request::getVar() or Xmf\FilterInput::clean() — never access $_GET, $_POST, or $_REQUEST directly"💡 Suggested fix
if (Request::hasVar('step', 'POST') && isset($_SESSION['profile_required'])) { foreach ($_SESSION['profile_required'] as $name => $title) { - if (!Request::hasVar($name, 'POST') || Request::getString($name, '', 'POST') === '') { + $value = Request::getVar($name, null, 'POST'); + $isEmpty = $value === null + || $value === '' + || (is_array($value) && [] === array_filter($value, static fn ($item) => $item !== '' && $item !== null)); + if ($isEmpty) { $stop .= sprintf(_FORM_ENTER, $title) . '<br>'; } } }#!/bin/bash set -e # Verify Request getter semantics and the profile field save behavior. rg -n -A12 -B3 'public static function getVar|public static function getString' htdocs/xoops_lib/vendor/xoops/xmf/src/Request.php sed -n '374,401p' htdocs/modules/profile/class/field.php sed -n '167,175p' htdocs/modules/profile/register.phpExpected result:
Request::getString()is string-oriented,ProfileField::getValueForSave()preserves arrays for multi-value field types, and the current validation branch still callsRequest::getString().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/register.php` around lines 170 - 173, The required-field validation is still using Request::getString() for dynamic fields in the block iterating $_SESSION['profile_required']; change the check to use Request::getVar($name, null, 'POST') (or Request::getVar with the same signature used elsewhere) so multi-value types (checkbox, select_multi, group_multi) keep their array shape; update the condition in the foreach that references Request::getString($name, '', 'POST') to use Request::getVar and adjust the empty check accordingly to handle arrays and strings consistently (see the loop over $_SESSION['profile_required'] and ProfileField::getValueForSave references).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/banners.php`:
- Around line 389-391: The password input is being HTML-filtered by
Request::getString causing stripped characters; change the retrieval in the
banners handling logic where Request::hasVar('pass','POST') and the assignment
to $clean_pass to use Request::getVar (or Request::getString) with
Request::MASK_ALLOW_RAW so the raw password bytes are preserved for
authentication; locate the use of Request::getString('pass', '', 'POST') and
replace it with a call that passes MASK_ALLOW_RAW to avoid HTML filtering while
still using the Request API.
In `@htdocs/include/notification_update.php`:
- Line 60: Normalize and validate the $update_list returned by
Request::getArray('not_list', [], 'POST') before any loops that split or
destructure entry['params']: ensure each item is an array, has a 'params' key
whose value is a string, split it by ',' and only keep entries where the split
yields exactly three non-empty parts; build a sanitized $update_list of these
normalized entries (or skip/record invalid ones) so the subsequent code that
explodes/destructures params cannot fail.
In `@htdocs/modules/pm/viewpmsg.php`:
- Around line 80-84: When handling the posted message IDs for the save path
(when $op === 'save'), normalize and validate each $msg element before passing
it to $pm_handler->get(): ensure values returned by Request::getArray('msg_id',
[], 'POST') are scalar and cast/clean them to integers (or use
Xmf\Request/Xmf\FilterInput clean APIs) to prevent nested or non-scalar inputs
reaching $pm_handler->get; iterate over $msg, convert each entry to a safe int
ID, skip invalid entries, then call $pm_handler->get($id) with the sanitized ID.
In `@htdocs/modules/profile/admin/category.php`:
- Around line 63-70: The id retrieval is missing the explicit POST hash for
consistency: update the Request::getInt call used when fetching the id (paired
with Request::hasVar('id', 'POST')) to include the 'POST' hash (i.e., change
Request::getInt('id', 0) to Request::getInt('id', 0, 'POST')) so it matches the
surrounding Request::getString/getInt calls and is explicit about input source.
In `@htdocs/modules/profile/admin/user.php`:
- Around line 141-150: Calls to getValueForSave in the user admin flow are
passing an extra second argument (e.g. $fields[$i]->getValueForSave($fieldValue,
$user->getVar($fieldname, 'n')) and similarly with $profile) even though
getValueForSave($value) accepts only one parameter; remove the unused second
argument so these lines call $fields[$i]->getValueForSave($fieldValue) before
assigning into $user->setVar(...) or $profile->setVar(...), keeping the
surrounding Request::getString($fieldname, '', 'POST') and editable_fields
checks intact.
In `@htdocs/modules/profile/preloads/core.php`:
- Around line 45-47: Replace direct accesses of $_SERVER['QUERY_STRING' ] with
Xmf\Request::getString to keep request handling consistent: in the header
redirect and the other three spots (the header('location:
./modules/profile/user.php' ... ) block and the other occurrences that currently
use $_SERVER['QUERY_STRING']), call Request::getString('QUERY_STRING', '',
'SERVER') into a local $queryString variable and use empty($queryString) ? '' :
'?' . $queryString when building the URL; update occurrences around the header
call and any functions/methods referencing $_SERVER['QUERY_STRING'] so they use
$queryString instead.
In `@htdocs/modules/profile/register.php`:
- Around line 30-31: Replace the raw $_SERVER['QUERY_STRING'] use in the
redirect with the XMF request helper: obtain a sanitized query string via
Request::getString (instead of direct $_SERVER access) and use that when
building the Location header in the same redirect branch where
Request::getString('op',..., 'GET') is used; update the header('location: ...')
call to append the sanitized query string variable rather than
$_SERVER['QUERY_STRING'] so all input flows through XMF's Request handling.
In `@htdocs/modules/system/admin/comments/main.php`:
- Around line 140-141: The two calls to Request::getString for $com_after and
$com_before use time() (an int) as the default and omit the 'POST' source,
creating a type mismatch and inconsistent source compared to the earlier hasVar
guard; change Request::getString('comments_after', '') and
Request::getString('comments_before', '') and include the 'POST' source argument
so they read from POST like the preceding hasVar check, then keep the existing
strtotime(...) and the if ($com_after)/if ($com_before) checks to handle
empty/false values.
- Around line 161-167: The loop over $comments_userid uses raw values from
Request::getArray and passes each $del into new Criteria('com_uid', $del); to
harden it, validate/cast each element inside the foreach: ensure $del is numeric
(e.g., is_numeric) and then cast to (int) before creating the Criteria or skip
non-numeric entries; update the foreach over $comments_userid to use the
validated/casted $del when calling Criteria('com_uid', $del) to enforce type
safety.
In `@htdocs/modules/system/admin/groupperm.php`:
- Around line 43-47: Remove the redundant is_array() guard since
Request::getArray('perms', [], 'POST') always returns an array; update the
conditional that currently reads if (is_array($perms) && !empty($perms)) to only
check !empty($perms) so the foreach over $perms (and subsequent use of
xoops_getHandler('groupperm') / $gperm_handler) runs only when there are
entries.
---
Outside diff comments:
In `@htdocs/modules/pm/viewpmsg.php`:
- Around line 58-62: Validate the decoded msg_ids payload from
Request::getString before treating it as an array: after $clean_msg_id =
json_decode(Request::getString('msg_ids', '', 'POST'), true, 2) ensure
$clean_msg_id is an array (e.g. is_array($clean_msg_id)) and if not, set it to
an empty array; only call array_map('intval', $clean_msg_id) and
count($clean_msg_id) when $clean_msg_id is an array so downstream loops that use
$clean_msg_id and $size won’t fail on tampered or scalar JSON payloads.
In `@htdocs/modules/profile/admin/step.php`:
- Around line 114-119: The function profile_stepsave_toggle currently ignores
its first parameter $step_d and re-reads the step id from Request::getInt;
change it to use the passed-in $step_d when fetching the object: replace the
Request::getInt('step_id', ...) call in the
xoops_getModuleHandler('regstep')->get(...) invocation with the $step_d
parameter so profile_stepsave_toggle($step_d, $step_save) consistently uses the
provided step id before calling $obj->setVar('step_save', $step_save).
In `@htdocs/modules/system/admin/banners/main.php`:
- Line 190: The bug is caused by a trailing space in the field name; change the
usage of 'passwd ' to 'passwd' in both places so the request parameter and DB
field match. Locate the call to $obj->setVar('passwd ',
Request::getString('passwd ', '')) and update the first argument to 'passwd' and
the Request::getString key to 'passwd' so $obj->setVar and Request::getString
use the correct field name. Ensure there are no other occurrences of 'passwd '
elsewhere in the file/class and run the save flow to verify password persists.
- Around line 311-314: Initialize a default value for $name before the
is_object($name_client) check so $banner['name'] won't be undefined; ensure the
block that retrieves $name from $name_client->getVar('name') only overrides that
default, referencing the same $name_client (result of
get($banner_arr[$i]->getVar('cid'))) and the later use $banner['name'] to avoid
E_WARNING when the client object is missing.
- Around line 397-398: The code calls $banner_client_Handler->get(...) and then
immediately $name_client->getVar('name') without checking that $name_client is
an object, which can cause a TypeError; update the block around where
$banner_finish_arr and $banner_client_Handler are used (the lines creating
$name_client and setting $banner_finish['name']) to guard against null/false by
checking is_object($name_client) (or use the PHP nullsafe operator on
$name_client) before calling getVar('name'), and set a sensible fallback (e.g.,
empty string or 'unknown') when the client is missing.
In `@htdocs/modules/system/admin/comments/main.php`:
- Around line 184-193: The block unnecessarily checks count after
!empty($commentslist_id); simplify by removing the redundant if
($commentslist_count > 0) branch and iterate directly over $commentslist_id
(e.g., using foreach) to add Criteria('com_id', $id) entries to $criteria;
update the loop that currently uses $commentslist_count and index-based access
to use each $id from $commentslist_id and call $criteria->add(new
Criteria('com_id', $id), 'OR') so the logic remains identical but cleaner.
- Around line 168-183: The foreach over $comments_groupe uses misleading
variable names $del and $u_name; change the loop to use a clear name like
$groupId (e.g., foreach ($comments_groupe as $groupId)) and update all uses
inside the loop (the call to $member_handler->getUsersByGroup($u_name, true) and
the Criteria creation that references $members[$i]->getVar('uid')) to use
$groupId instead; keep $member_handler, $members, getUsersByGroup, and the
Criteria construction unchanged otherwise.
In `@htdocs/modules/system/admin/groupperm.php`:
- Line 51: The loose comparisons should be made strict: change the conditional
using $selected == 1 to use a strict check (either === 1 if $selected is an int
or === '1' / (int)$selected === 1 if it comes from form input) and likewise
change $pid != 0 to a strict comparison ($pid !== 0 or $pid !== '0' or (int)$pid
!== 0) so type mismatches don’t pass; update the conditionals in groupperm.php
where $selected and $pid are used (replace the == and != operators with the
appropriate === / !== or explicit casts) to ensure correct type-safe behavior.
- Around line 87-91: Validate and sanitize the user-provided redirect URL
returned from Request::getString('redirect_url', ...) before using it to build
$backlink: check the value in $adminindex for any path traversal or unsafe
characters (disallow sequences like '..', leading slashes, URL schemes such as
"http:"/"://", query strings, or other path separators), only accept a simple
relative filename (e.g., alphanumerics, dots, dashes, underscores, optionally a
.php extension) and if the value fails validation fall back to
$module->getInfo('adminindex'); update the logic around adminindex and $backlink
(and keep using $module->getVar('dirname')) so untrusted input can never inject
traversal or protocol content into the constructed URL.
---
Duplicate comments:
In
`@htdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsemotions/xoopsemotions.php`:
- Around line 66-74: The code mixes fully-qualified \Xmf\Request:: calls with
the imported Request alias; update the uses of \Xmf\Request::getArray(...) and
\Xmf\Request::getInt(...) in the xoopsemotions plugin block to use the imported
Request::getArray(...) and Request::getInt(...) respectively so the file
consistently uses the Request alias declared by use Xmf\Request; (leave existing
Request::getString(...) usages unchanged).
In `@htdocs/include/notification_update.php`:
- Around line 43-46: The current $not_redirect validation rejects valid relative
paths and uses a weak prefix check; update the logic around
Request::getUrl('not_redirect', XOOPS_URL . '/', 'POST') to accept
leading-absolute-paths (values starting with '/') by normalizing them to
XOOPS_URL + path (so "/foo" becomes XOOPS_URL . "/foo") and to more strictly
validate absolute URLs by parsing both the provided URL and XOOPS_URL and
comparing scheme, host, and port; if the value is neither a normalized
leading-"/" path nor an absolute URL with matching scheme/host/port, fall back
to XOOPS_URL . '/'. Reference $not_redirect, Request::getUrl, XOOPS_URL and
notification_select.php/xoops_getenv('PHP_SELF') when locating where to change
the validation.
In `@htdocs/install/page_theme.php`:
- Around line 38-42: The loop that reads per-config POST fields using
\Xmf\Request::getString(...) can accept missing or empty fields and then
propagate an empty theme via updateUsersByField; also $new_value may be
uninitialized if conf_ids is absent/empty. Fix by guarding each config-specific
field with \Xmf\Request::hasVar($config->getVar('conf_name'), 'POST') and only
call \Xmf\Request::getString(...) and config_handler->updateUsersByField(...)
when that hasVar check passes and the retrieved value is not an empty string;
also ensure you skip the entire loop if $confIds is empty so $new_value is not
used uninitialized. Use the identifiers $confIds, $new_value,
$config_handler->getConfig(...), $config->getVar('conf_name'),
\Xmf\Request::hasVar, \Xmf\Request::getString and updateUsersByField in your
changes.
In `@htdocs/modules/profile/register.php`:
- Around line 170-173: The required-field validation is still using
Request::getString() for dynamic fields in the block iterating
$_SESSION['profile_required']; change the check to use Request::getVar($name,
null, 'POST') (or Request::getVar with the same signature used elsewhere) so
multi-value types (checkbox, select_multi, group_multi) keep their array shape;
update the condition in the foreach that references Request::getString($name,
'', 'POST') to use Request::getVar and adjust the empty check accordingly to
handle arrays and strings consistently (see the loop over
$_SESSION['profile_required'] and ProfileField::getValueForSave references).
In `@htdocs/search.php`:
- Around line 41-45: The current logic assigns $action to an empty string when
Request::hasVar('action', ...) is true for cases like ?action=; change the
assignment so that you only overwrite the default $action ('search') when the
retrieved value from Request::getString('action', ...) is non-empty after
trimming/strip_tags; i.e., call Request::getString, trim/strip_tags it, then
check that the result is not an empty string before setting $action (referencing
Request::hasVar, Request::getString and the $action variable used by the
switch).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4f2b0c2-0eb6-4429-9f86-1ab01448474d
📒 Files selected for processing (19)
htdocs/banners.phphtdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsemotions/xoopsemotions.phphtdocs/include/common.phphtdocs/include/notification_update.phphtdocs/install/page_configsite.phphtdocs/install/page_theme.phphtdocs/modules/pm/viewpmsg.phphtdocs/modules/profile/admin/category.phphtdocs/modules/profile/admin/field.phphtdocs/modules/profile/admin/step.phphtdocs/modules/profile/admin/user.phphtdocs/modules/profile/preloads/core.phphtdocs/modules/profile/register.phphtdocs/modules/system/admin/avatars/main.phphtdocs/modules/system/admin/banners/main.phphtdocs/modules/system/admin/comments/main.phphtdocs/modules/system/admin/groupperm.phphtdocs/modules/system/admin/maintenance/main.phphtdocs/search.php
| if (Request::hasVar('pass', 'POST')) { | ||
| $clean_pass = Request::getString('pass', '', 'POST'); | ||
| } |
There was a problem hiding this comment.
Password input is corrupted by HTML filtering.
Request::getString() applies FilterInput::clean() which strips characters like <, >, &, and " from the input. Passwords can legitimately contain these characters, so filtering them causes authentication failures for users with such passwords.
Use Request::MASK_ALLOW_RAW to bypass HTML filtering for password fields:
🔒 Proposed fix to preserve password integrity
if (Request::hasVar('pass', 'POST')) {
- $clean_pass = Request::getString('pass', '', 'POST');
+ $clean_pass = Request::getString('pass', '', 'POST', Request::MASK_ALLOW_RAW);
}As per coding guidelines: "All user input must be filtered using Xmf\Request::getVar() or Xmf\FilterInput::clean()". However, passwords are a special case—they must be compared byte-for-byte against stored hashes, so HTML sanitization is inappropriate here. The raw flag preserves the original input while still using the Request abstraction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/banners.php` around lines 389 - 391, The password input is being
HTML-filtered by Request::getString causing stripped characters; change the
retrieval in the banners handling logic where Request::hasVar('pass','POST') and
the assignment to $clean_pass to use Request::getVar (or Request::getString)
with Request::MASK_ALLOW_RAW so the raw password bytes are preserved for
authentication; locate the use of Request::getString('pass', '', 'POST') and
replace it with a call that passes MASK_ALLOW_RAW to avoid HTML filtering while
still using the Request API.
| if (in_array($fields[$i]->getVar('field_id'), $editable_fields) && Request::hasVar($fieldname, 'POST')) { | ||
| $fieldValue = Request::getString($fieldname, '', 'POST'); | ||
| if (in_array($fieldname, $userfields)) { | ||
| $value = $fields[$i]->getValueForSave($_REQUEST[$fieldname], $user->getVar($fieldname, 'n')); | ||
| $value = $fields[$i]->getValueForSave($fieldValue, $user->getVar($fieldname, 'n')); | ||
| $user->setVar($fieldname, $value); | ||
| } else { | ||
| $value = $fields[$i]->getValueForSave(($_REQUEST[$fieldname] ?? ''), $profile->getVar($fieldname, 'n')); | ||
| $value = $fields[$i]->getValueForSave($fieldValue, $profile->getVar($fieldname, 'n')); | ||
| $profile->setVar($fieldname, $value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify getValueForSave method signature across the codebase
echo "== Checking getValueForSave method signatures =="
ast-grep --pattern $'function getValueForSave($$$) {
$$$
}'
echo ""
echo "== Checking all call sites for getValueForSave =="
rg -n -C2 'getValueForSave\(' --type=phpRepository: XOOPS/XoopsCore25
Length of output: 8152
getValueForSave() method signature accepts only one parameter; remove unused second argument.
The method defined at htdocs/modules/profile/class/field.php:374 has the signature public function getValueForSave($value). Lines 144 and 147 pass a second argument ($user->getVar($fieldname, 'n') / $profile->getVar($fieldname, 'n')), which PHP silently ignores. All other call sites in the codebase—edituser.php:73, admin/field.php:232, register.php:159, and the test suite—correctly pass a single argument.
Remove unused second argument
if (in_array($fields[$i]->getVar('field_id'), $editable_fields) && Request::hasVar($fieldname, 'POST')) {
$fieldValue = Request::getString($fieldname, '', 'POST');
if (in_array($fieldname, $userfields)) {
- $value = $fields[$i]->getValueForSave($fieldValue, $user->getVar($fieldname, 'n'));
+ $value = $fields[$i]->getValueForSave($fieldValue);
$user->setVar($fieldname, $value);
} else {
- $value = $fields[$i]->getValueForSave($fieldValue, $profile->getVar($fieldname, 'n'));
+ $value = $fields[$i]->getValueForSave($fieldValue);
$profile->setVar($fieldname, $value);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (in_array($fields[$i]->getVar('field_id'), $editable_fields) && Request::hasVar($fieldname, 'POST')) { | |
| $fieldValue = Request::getString($fieldname, '', 'POST'); | |
| if (in_array($fieldname, $userfields)) { | |
| $value = $fields[$i]->getValueForSave($_REQUEST[$fieldname], $user->getVar($fieldname, 'n')); | |
| $value = $fields[$i]->getValueForSave($fieldValue, $user->getVar($fieldname, 'n')); | |
| $user->setVar($fieldname, $value); | |
| } else { | |
| $value = $fields[$i]->getValueForSave(($_REQUEST[$fieldname] ?? ''), $profile->getVar($fieldname, 'n')); | |
| $value = $fields[$i]->getValueForSave($fieldValue, $profile->getVar($fieldname, 'n')); | |
| $profile->setVar($fieldname, $value); | |
| } | |
| } | |
| if (in_array($fields[$i]->getVar('field_id'), $editable_fields) && Request::hasVar($fieldname, 'POST')) { | |
| $fieldValue = Request::getString($fieldname, '', 'POST'); | |
| if (in_array($fieldname, $userfields)) { | |
| $value = $fields[$i]->getValueForSave($fieldValue); | |
| $user->setVar($fieldname, $value); | |
| } else { | |
| $value = $fields[$i]->getValueForSave($fieldValue); | |
| $profile->setVar($fieldname, $value); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/admin/user.php` around lines 141 - 150, Calls to
getValueForSave in the user admin flow are passing an extra second argument
(e.g. $fields[$i]->getValueForSave($fieldValue, $user->getVar($fieldname, 'n'))
and similarly with $profile) even though getValueForSave($value) accepts only
one parameter; remove the unused second argument so these lines call
$fields[$i]->getValueForSave($fieldValue) before assigning into
$user->setVar(...) or $profile->setVar(...), keeping the surrounding
Request::getString($fieldname, '', 'POST') and editable_fields checks intact.
| $from = Request::getString('from', '', 'GET'); | ||
| if ($op !== 'login' && $from !== 'profile') { | ||
| header('location: ./modules/profile/user.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect Xmf\\Request accessors:"
fd 'Request\.php$' -X rg -n -C2 'class Request|function\s+(getVar|getString|hasVar)|SERVER'
echo
echo "Current raw QUERY_STRING usage in this file:"
rg -n -C1 "\\$_SERVER\\['QUERY_STRING'\\]" htdocs/modules/profile/preloads/core.phpRepository: XOOPS/XoopsCore25
Length of output: 7688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== QUERY_STRING usage in target file ==="
rg -n 'QUERY_STRING' htdocs/modules/profile/preloads/core.php
echo
echo "=== Request::getString() signature and SERVER support ==="
sed -n '225,250p' htdocs/xoops_lib/vendor/xoops/xmf/src/Request.php
echo
echo "=== getVar() with SERVER hash example ==="
sed -n '80,120p' htdocs/xoops_lib/vendor/xoops/xmf/src/Request.phpRepository: XOOPS/XoopsCore25
Length of output: 3096
Migrate all $_SERVER['QUERY_STRING'] accesses through Xmf\Request for consistency.
Four instances of raw $_SERVER['QUERY_STRING'] access remain in this file (lines 47, 57, 78, 87). Replace them with Request::getString('QUERY_STRING', '', 'SERVER') to complete the migration to the XMF request layer:
$queryString = Request::getString('QUERY_STRING', '', 'SERVER');
header('location: ./modules/profile/user.php' . (empty($queryString) ? '' : '?' . $queryString));This maintains consistency with the earlier migration of $op and $from to use Xmf\Request, aligning with the project standards to access request data through the XMF utilities layer.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 45-45: Avoid using static access to class '\Xmf\Request' in method 'eventCoreUserStart'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/preloads/core.php` around lines 45 - 47, Replace
direct accesses of $_SERVER['QUERY_STRING' ] with Xmf\Request::getString to keep
request handling consistent: in the header redirect and the other three spots
(the header('location: ./modules/profile/user.php' ... ) block and the other
occurrences that currently use $_SERVER['QUERY_STRING']), call
Request::getString('QUERY_STRING', '', 'SERVER') into a local $queryString
variable and use empty($queryString) ? '' : '?' . $queryString when building the
URL; update occurrences around the header call and any functions/methods
referencing $_SERVER['QUERY_STRING'] so they use $queryString instead.
htdocs/modules/profile/register.php
Outdated
| if (Request::getString('op', '', 'GET') !== '' && in_array(Request::getString('op', '', 'GET'), ['actv', 'activate'])) { | ||
| header('location: ./activate.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); |
There was a problem hiding this comment.
Finish the migration on the redirect query string.
Line 31 still appends raw $_SERVER['QUERY_STRING'] to the redirect, so this path bypasses the XMF input-handling migration applied elsewhere in the file. Route this through an XMF helper before reusing it in the Location header. Based on learnings: "Applies to **/*.php : Use Xmf\Request, Xmf\Module\Helper, and other XMF utilities from htdocs/class/libraries/ instead of direct superglobal or legacy function access"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/register.php` around lines 30 - 31, Replace the raw
$_SERVER['QUERY_STRING'] use in the redirect with the XMF request helper: obtain
a sanitized query string via Request::getString (instead of direct $_SERVER
access) and use that when building the Location header in the same redirect
branch where Request::getString('op',..., 'GET') is used; update the
header('location: ...') call to append the sanitized query string variable
rather than $_SERVER['QUERY_STRING'] so all input flows through XMF's Request
handling.
htdocs/banners.php
Outdated
| $op = Request::getString('op', '', 'POST'); | ||
|
|
||
| if (isset($_POST['login'])) { | ||
| if (Request::hasVar('login', 'POST')) { | ||
| $clean_login = Request::getString('login', '', 'POST'); | ||
| } | ||
|
|
||
| if (isset($_POST['pass'])) { | ||
| $clean_pass = Request::getString('pass', '', 'POST'); | ||
| if (Request::hasVar('pass', 'POST')) { | ||
| $clean_pass = Request::getText('pass', '', 'POST'); | ||
| } | ||
|
|
||
| if (isset($_POST['url'])) { | ||
| if (Request::hasVar('url', 'POST')) { | ||
| $clean_url = Request::getUrl('url', '', 'POST'); | ||
| } | ||
|
|
||
| if (isset($_POST['bid'])) { | ||
| if (Request::hasVar('bid', 'POST')) { | ||
| $clean_bid = Request::getInt('bid', 0, 'POST'); | ||
| } | ||
|
|
||
| if (isset($_POST['cid'])) { | ||
| if (Request::hasVar('cid', 'POST')) { | ||
| $clean_cid = Request::getInt('cid', 0, 'POST'); | ||
| } | ||
| } elseif (!empty($_GET['op'])) { | ||
| } elseif (Request::hasVar('op', 'GET')) { | ||
| // from $_GET we use keys: op, bid, cid | ||
| $op = Request::getCmd('op', '', 'GET'); | ||
| $op = Request::getString('op', '', 'GET'); |
There was a problem hiding this comment.
In banners.php, the op parameter was previously read with Request::getCmd('op', '', 'POST') (and 'GET'), but has been changed to Request::getString('op', '', 'POST') (and 'GET'). The getCmd method restricts values to [A-Za-z0-9._-], which is the appropriate type-safe filter for an operation/command parameter. Using getString for an op parameter is a security downgrade and inconsistent with how every other op parameter is handled throughout the codebase (e.g., edituser.php:36, profile/search.php:25, pm/readpmsg.php:28). This should be reverted to use Request::getCmd.
htdocs/modules/profile/register.php
Outdated
| if (Request::hasVar('step', 'POST') && isset($_SESSION['profile_required'])) { | ||
| foreach ($_SESSION['profile_required'] as $name => $title) { | ||
| if (!isset($_POST[$name]) || empty($_POST[$name])) { | ||
| if (!Request::hasVar($name, 'POST') || Request::getString($name, '', 'POST') === '') { |
There was a problem hiding this comment.
In htdocs/modules/profile/register.php line 172, Request::getString($name, '', 'POST') === '' is used to check if a required field is empty. However, the original code used empty($_POST[$name]), which also catches zero-like values (e.g., '0', 0). For text fields this difference is usually fine, but for numeric required fields (e.g., a required integer field stored as '0'), getString will return '0' which is not equal to '', so the field will incorrectly pass validation. The more accurate replacement would be empty(Request::getString($name, '', 'POST')) to preserve the original empty check semantics.
| #[Test] | ||
| public function getUrlFiltersInvalidRedirectUrl(): void | ||
| { | ||
| $_POST['not_redirect'] = 'https://evil.com/phish'; | ||
| $value = Request::getUrl('not_redirect', '', 'POST'); | ||
| // getUrl uses FILTER_SANITIZE_URL — it preserves valid URLs. | ||
| // The APPLICATION must still validate that the domain is trusted. | ||
| // But at least XSS payloads are stripped: | ||
| $this->assertIsString($value); | ||
| } |
There was a problem hiding this comment.
The test getUrlFiltersInvalidRedirectUrl (line 223–231) tests that Request::getUrl() accepts 'https://evil.com/phish' as a valid URL and only asserts assertIsString($value). The test makes no assertion that would catch an open redirect vulnerability — it explicitly notes that domain validation is the application's responsibility. However, the actual notification_update.php migration now correctly adds a domain-validation guard (strpos($not_redirect, XOOPS_URL) !== 0). A test that validates this guard behavior (i.e., that an off-domain URL is rejected/normalized) would be more meaningful and would actually verify the security improvement. The current test only confirms getUrl returns a string, which is trivially true and provides no regression coverage for the open-redirect fix.
| redirect_header('index.php', 2, _PROFILE_AM_FIELDNOTCONFIGURABLE); | ||
| } | ||
| if (isset($_REQUEST['ok']) && $_REQUEST['ok'] == 1) { | ||
| if (Request::getInt('ok', 0) === 1) { |
There was a problem hiding this comment.
In htdocs/modules/profile/admin/field.php line 333, the delete operation reads id using Request::getInt('id', 0) (no hash parameter, defaults to REQUEST). However, the ok check on the same line reads from POST only (Request::getInt('ok', 0)). The delete flow starts with a GET request (clicking a delete link), and the confirmation form then POST-submits. The id must be available on both the initial GET and the POST confirmation. This is correctly using REQUEST for id, which is consistent with the codebase convention for delete flows (as established in category.php:82 and step.php:75). However, for ok check on line 333, using Request::getInt('ok', 0) without 'POST' also reads from GET — which means a crafted GET request with ok=1 in the URL would skip the confirmation dialog and delete the field directly. This should specify 'POST' for the ok parameter.
| $field_id = Request::getInt('field_id', 0); | ||
| if ($field_id > 0) { | ||
| $field_required = Request::getInt('field_required', 0); |
There was a problem hiding this comment.
In htdocs/modules/profile/admin/field.php lines 356–358, the toggle case reads field_id and field_required using Request::getInt('field_id', 0) and Request::getInt('field_required', 0) without specifying a hash. This defaults to REQUEST (GET+POST+COOKIE). Looking at the URL patterns for toggle operations (typically field.php?op=toggle&field_id=X&field_required=Y), these are GET parameters. Using REQUEST instead of 'GET' is inconsistent with the codebase convention established in this same PR (for example, step.php:97 uses 'GET' for the analogous step_id). These should specify 'GET' explicitly to be consistent and correctly restrict the source.
htdocs/modules/profile/search.php
Outdated
| @@ -224,7 +228,7 @@ | |||
| foreach ($results as $row) { | |||
| // Populate search URL and search variables based on the results | |||
| $search_url[] = 'uname=' . urlencode($row['uname']); | |||
| $search_url[] = 'uname_match=' . urlencode($_REQUEST['uname_match']); | |||
| $search_url[] = 'uname_match=' . urlencode((string) $uname_match); | |||
| $searchvars[] = 'uname'; | |||
| } | |||
| } | |||
| @@ -233,9 +237,11 @@ | |||
| // You might render a page or redirect the user based on these results | |||
| } | |||
|
|
|||
| if (isset($_REQUEST['email']) && $_REQUEST['email'] !== '') { | |||
| $string = $xoopsDB->escape(trim($_REQUEST['email'])); | |||
| switch ($_REQUEST['email_match']) { | |||
| $email = Request::getString('email', '', 'POST'); | |||
| $email_match = Request::getInt('email_match', 0, 'POST'); | |||
| if ($email !== '') { | |||
| $string = $xoopsDB->escape(trim($email)); | |||
| switch ($email_match) { | |||
| case XOOPS_MATCH_START: | |||
| $string .= '%'; | |||
| break; | |||
| @@ -249,8 +255,8 @@ | |||
| break; | |||
| } | |||
| $searchvars[] = 'email'; | |||
| $search_url[] = 'email=' . $_REQUEST['email']; | |||
| $search_url[] = 'email_match=' . $_REQUEST['email_match']; | |||
| $search_url[] = 'email=' . $email; | |||
| $search_url[] = 'email_match=' . $email_match; | |||
| $criteria->add(new Criteria('email', $string, 'LIKE')); | |||
| $criteria->add(new Criteria('user_viewemail', 1)); | |||
| } | |||
| @@ -262,52 +268,55 @@ | |||
| continue; | |||
| } | |||
| $fieldname = $fields[$i]->getVar('field_name'); | |||
| $fieldValues = Request::getArray($fieldname, [], 'POST'); | |||
| if (in_array($fields[$i]->getVar('field_type'), ['select', 'radio', 'timezone'])) { | |||
| if (empty($_REQUEST[$fieldname])) { | |||
| if (empty($fieldValues)) { | |||
| continue; | |||
| } | |||
|
|
|||
| //If field value is sent through request and is not an empty value | |||
| switch ($fields[$i]->getVar('field_valuetype')) { | |||
| case XOBJ_DTYPE_OTHER: | |||
| case XOBJ_DTYPE_INT: | |||
| $value = array_map('intval', $_REQUEST[$fieldname]); | |||
| $value = array_map('intval', $fieldValues); | |||
| $searchvars[] = $fieldname; | |||
| $criteria->add(new Criteria($fieldname, '(' . implode(',', $value) . ')', 'IN')); | |||
| break; | |||
|
|
|||
| case XOBJ_DTYPE_URL: | |||
| case XOBJ_DTYPE_TXTBOX: | |||
| case XOBJ_DTYPE_TXTAREA: | |||
| $value = array_map([$GLOBALS['xoopsDB'], 'quoteString'], $_REQUEST[$fieldname]); | |||
| $value = array_map([$GLOBALS['xoopsDB'], 'quoteString'], $fieldValues); | |||
| $searchvars[] = $fieldname; | |||
| $criteria->add(new Criteria($fieldname, '(' . implode(',', $value) . ')', 'IN')); | |||
| break; | |||
| } | |||
| foreach ($_REQUEST[$fieldname] as $value) { | |||
| foreach ($fieldValues as $value) { | |||
| $search_url[] = $fieldname . '[]=' . $value; | |||
| } | |||
| } else { | |||
| //Other fields (not radio, not select) | |||
| switch ($fields[$i]->getVar('field_valuetype')) { | |||
| case XOBJ_DTYPE_OTHER: | |||
| case XOBJ_DTYPE_INT: | |||
| $largerVal = Request::getString($fieldname . '_larger', '', 'POST'); | |||
| $smallerVal = Request::getString($fieldname . '_smaller', '', 'POST'); | |||
| switch ($fields[$i]->getVar('field_type')) { | |||
| case 'date': | |||
| case 'datetime': | |||
| $value = $_REQUEST[$fieldname . '_larger']; | |||
| if (!($value = strtotime($_REQUEST[$fieldname . '_larger']))) { | |||
| $value = (int) $_REQUEST[$fieldname . '_larger']; | |||
| $value = strtotime($largerVal); | |||
| if (!$value) { | |||
| $value = (int) $largerVal; | |||
| } | |||
| if ($value > 0) { | |||
| $search_url[] = $fieldname . '_larger=' . $value; | |||
| $searchvars[] = $fieldname; | |||
| $criteria->add(new Criteria($fieldname, $value, '>=')); | |||
| } | |||
|
|
|||
| $value = $_REQUEST[$fieldname . '_smaller']; | |||
| if (!($value = strtotime($_REQUEST[$fieldname . '_smaller']))) { | |||
| $value = (int) $_REQUEST[$fieldname . '_smaller']; | |||
| $value = strtotime($smallerVal); | |||
| if (!$value) { | |||
| $value = (int) $smallerVal; | |||
| } | |||
| if ($value > 0) { | |||
| $search_url[] = $fieldname . '_smaller=' . $value; | |||
| @@ -317,29 +326,29 @@ | |||
| break; | |||
|
|
|||
| default: | |||
| if (isset($_REQUEST[$fieldname . '_larger']) && (int) $_REQUEST[$fieldname . '_larger'] !== 0) { | |||
| $value = (int) $_REQUEST[$fieldname . '_larger']; | |||
| $search_url[] = $fieldname . '_larger=' . $value; | |||
| $intLarger = Request::getInt($fieldname . '_larger', 0, 'POST'); | |||
| $intSmaller = Request::getInt($fieldname . '_smaller', 0, 'POST'); | |||
| if ($intLarger !== 0) { | |||
| $search_url[] = $fieldname . '_larger=' . $intLarger; | |||
| $searchvars[] = $fieldname; | |||
| $criteria->add(new Criteria($fieldname, $value, '>=')); | |||
| $criteria->add(new Criteria($fieldname, $intLarger, '>=')); | |||
| } | |||
|
|
|||
| if (isset($_REQUEST[$fieldname . '_smaller']) && (int) $_REQUEST[$fieldname . '_smaller'] !== 0) { | |||
| $value = (int) $_REQUEST[$fieldname . '_smaller']; | |||
| $search_url[] = $fieldname . '_smaller=' . $value; | |||
| if ($intSmaller !== 0) { | |||
| $search_url[] = $fieldname . '_smaller=' . $intSmaller; | |||
| $searchvars[] = $fieldname; | |||
| $criteria->add(new Criteria($fieldname, $value, '<=')); | |||
| $criteria->add(new Criteria($fieldname, $intSmaller, '<=')); | |||
| } | |||
| break; | |||
| } | |||
|
|
|||
| if (isset($_REQUEST[$fieldname]) && !isset($_REQUEST[$fieldname . '_smaller']) && !isset($_REQUEST[$fieldname . '_larger'])) { | |||
| if (!is_array($_REQUEST[$fieldname])) { | |||
| $value = (int) $_REQUEST[$fieldname]; | |||
| if (!empty($fieldValues) && $largerVal === '' && $smallerVal === '') { | |||
| if (!is_array($fieldValues)) { | |||
| $value = (int) $fieldValues; | |||
| $search_url[] = $fieldname . '=' . $value; | |||
| $criteria->add(new Criteria($fieldname, $value, '=')); | |||
| } else { | |||
| $value = array_map('intval', $_REQUEST[$fieldname]); | |||
| $value = array_map('intval', $fieldValues); | |||
| foreach ($value as $thisvalue) { | |||
| $search_url[] = $fieldname . '[]=' . $thisvalue; | |||
| } | |||
| @@ -353,9 +362,11 @@ | |||
| case XOBJ_DTYPE_URL: | |||
| case XOBJ_DTYPE_TXTBOX: | |||
| case XOBJ_DTYPE_TXTAREA: | |||
| if (isset($_REQUEST[$fieldname]) && $_REQUEST[$fieldname] !== '') { | |||
| $value = $xoopsDB->escape(trim($_REQUEST[$fieldname])); | |||
| switch ($_REQUEST[$fieldname . '_match']) { | |||
| $textFieldVal = Request::getString($fieldname, '', 'POST'); | |||
| $textFieldMatch = Request::getInt($fieldname . '_match', 0, 'POST'); | |||
| if ($textFieldVal !== '') { | |||
| $value = $xoopsDB->escape(trim($textFieldVal)); | |||
| switch ($textFieldMatch) { | |||
| case XOOPS_MATCH_START: | |||
| $value .= '%'; | |||
| break; | |||
| @@ -368,8 +379,8 @@ | |||
| $value = '%' . $value . '%'; | |||
| break; | |||
| } | |||
| $search_url[] = $fieldname . '=' . $_REQUEST[$fieldname]; | |||
| $search_url[] = $fieldname . '_match=' . $_REQUEST[$fieldname . '_match']; | |||
| $search_url[] = $fieldname . '=' . $textFieldVal; | |||
| $search_url[] = $fieldname . '_match=' . $textFieldMatch; | |||
| $operator = 'LIKE'; | |||
| $criteria->add(new Criteria($fieldname, $value, $operator)); | |||
| $searchvars[] = $fieldname; | |||
| @@ -391,16 +402,17 @@ | |||
|
|
|||
| // change by zyspec: | |||
| $sortby = 'uname'; | |||
| if (!empty($_REQUEST['sortby'])) { | |||
| switch ($_REQUEST['sortby']) { | |||
| $sortbyInput = Request::getCmd('sortby', '', 'POST'); | |||
| if ($sortbyInput !== '') { | |||
| switch ($sortbyInput) { | |||
| case 'name': | |||
| case 'email': | |||
| case 'uname': | |||
| $sortby = $_REQUEST['sortby']; | |||
| $sortby = $sortbyInput; | |||
| break; | |||
| default: | |||
| if (isset($fields[$_REQUEST['sortby']])) { | |||
| $sortby = $fields[$_REQUEST['sortby']]->getVar('field_name'); | |||
| if (isset($fields[$sortbyInput])) { | |||
| $sortby = $fields[$sortbyInput]->getVar('field_name'); | |||
| } | |||
| break; | |||
| } | |||
| @@ -410,19 +422,20 @@ | |||
| // add search groups , only for Webmasters | |||
| $searchgroups = []; | |||
| if ($GLOBALS['xoopsUser'] && $GLOBALS['xoopsUser']->isAdmin()) { | |||
| $searchgroups = empty($_REQUEST['selgroups']) ? [] : array_map('intval', $_REQUEST['selgroups']); | |||
| $selgroups = Request::getArray('selgroups', [], 'POST'); | |||
| $searchgroups = empty($selgroups) ? [] : array_map('intval', $selgroups); | |||
| foreach ($searchgroups as $group) { | |||
| $search_url[] = 'selgroups[]=' . $group; | |||
| } | |||
| } | |||
|
|
|||
| $order = $_REQUEST['order'] == 0 ? 'ASC' : 'DESC'; | |||
| $order = Request::getInt('order', 0, 'POST') === 0 ? 'ASC' : 'DESC'; | |||
| $criteria->setOrder($order); | |||
|
|
|||
| $limit = empty($_REQUEST['limit']) ? $limit_default : (int) $_REQUEST['limit']; | |||
| $limit = Request::getInt('limit', $limit_default, 'POST'); | |||
| $criteria->setLimit($limit); | |||
|
|
|||
| $start = isset($_REQUEST['start']) ? (int) $_REQUEST['start'] : 0; | |||
| $start = Request::getInt('start', 0, 'POST'); | |||
There was a problem hiding this comment.
In htdocs/modules/profile/search.php (the results case), all search parameters (uname, email, limit, start, order, sortby, selgroups, and all dynamic field values) are now read exclusively from POST. The search results pagination links (built at line 483 via XoopsPageNav) produce GET links appending start=N to the URL. When a user clicks these pagination links, the subsequent GET request will not carry any POST data, so all filters (uname, email, etc.) will be empty and the search criteria will be lost. Either the pagination must use a form with POST, or the parameters must also be read from GET. This is a regression in pagination behavior.
| $_postData = filter_input_array(INPUT_POST, FILTER_DEFAULT) ?? []; | ||
| foreach ($_postData as $k => $v) { | ||
| ${$k} = $v; | ||
| } | ||
| unset($_postData); |
There was a problem hiding this comment.
In htdocs/modules/system/admin/preferences/main.php, the dynamic POST variable extraction pattern foreach ($_postData as $k => $v) { ${$k} = $v; } (lines 29–31) still imports all POST keys as local variables using variable variables. Although this pattern was pre-existing (moved from iterating $_REQUEST to filter_input_array(INPUT_POST)), it still introduces a risk where attacker-controlled POST keys can overwrite local PHP variables, including critical ones like $conf_ids, $redirect, or $count. This is better than iterating $_REQUEST but still an unsafe pattern. The individual variables should instead be retrieved with explicit Request::getArray/Request::getString calls as needed.
| @@ -100,5 +107,5 @@ | |||
| } | |||
| } | |||
|
|
|||
| redirect_header($_POST['not_redirect'] . $argstring, 3, _NOT_UPDATEOK); | |||
| redirect_header($not_redirect . $argstring, 3, _NOT_UPDATEOK); | |||
There was a problem hiding this comment.
$not_redirect is validated only with a strpos($not_redirect, XOOPS_URL) === 0 prefix check before being passed to redirect_header(), which still allows external URLs like https://example.com.attacker.com/... to be used as redirect targets. An attacker can POST a crafted not_redirect value starting with XOOPS_URL (e.g., from a malicious form) and force users to be redirected to a phishing site under an attacker-controlled domain. To fix this, parse XOOPS_URL and $not_redirect with parse_url() and enforce exact scheme/host (and optionally path) equality, or restrict redirects to relative paths only.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/modules/pm/viewpmsg.php (1)
58-62:⚠️ Potential issue | 🔴 CriticalTypeError risk:
json_decodemay returnnullor a non-array.When
json_decode()fails (malformed JSON) or decodes a scalar,$clean_msg_idwon't be an array. In PHP 8.0+,count(null)throws aTypeError. The!empty()guard on line 59 only protects thearray_mapcall, not the subsequentcount().🐛 Proposed fix to ensure $clean_msg_id is always an array
- $clean_msg_id = json_decode(Request::getString('msg_ids', '', 'POST'), true, 2); - if (!empty($clean_msg_id)) { + $decoded = json_decode(Request::getString('msg_ids', '', 'POST'), true, 2); + $clean_msg_id = is_array($decoded) ? array_map('intval', $decoded) : []; + if (!empty($clean_msg_id)) { + // Already sanitized above - $clean_msg_id = array_map('intval', $clean_msg_id); } $size = count($clean_msg_id);Or more concisely:
- $clean_msg_id = json_decode(Request::getString('msg_ids', '', 'POST'), true, 2); - if (!empty($clean_msg_id)) { - $clean_msg_id = array_map('intval', $clean_msg_id); - } + $decoded = json_decode(Request::getString('msg_ids', '', 'POST'), true, 2); + $clean_msg_id = is_array($decoded) ? array_map('intval', $decoded) : []; $size = count($clean_msg_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/pm/viewpmsg.php` around lines 58 - 62, The code uses json_decode(Request::getString(...)) into $clean_msg_id but then calls count($clean_msg_id), which can throw a TypeError if json_decode returns null or a non-array; ensure $clean_msg_id is an array before using array_map or count by checking is_array($clean_msg_id) (or casting the decoded value to an array) and defaulting to an empty array when it's not, so update the handling around the json_decode call and the subsequent array_map/count logic for $clean_msg_id.
♻️ Duplicate comments (4)
htdocs/banners.php (1)
389-391:⚠️ Potential issue | 🔴 CriticalPassword input may still be filtered by
getText().The switch from
getString()togetText()doesn't fully resolve the filtering concern.Request::getText()still appliesFilterInput::clean(), which can modify special characters that are valid in passwords. For byte-exact hash comparison, raw input is required.The previous review suggested using
Request::MASK_ALLOW_RAW:🔒 Proposed fix to preserve password integrity
if (Request::hasVar('pass', 'POST')) { - $clean_pass = Request::getText('pass', '', 'POST'); + $clean_pass = Request::getString('pass', '', 'POST', Request::MASK_ALLOW_RAW); }Xmf Request MASK_ALLOW_RAW password filtering🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` around lines 389 - 391, The password is still being filtered by Request::getText (which calls FilterInput::clean); replace the call to Request::getText('pass', '', 'POST') with a raw retrieval that uses Request::MASK_ALLOW_RAW (e.g., use Request::getVar or Request::getString with the Request::MASK_ALLOW_RAW flag) so the password bytes are returned unchanged for exact hash comparison; keep the Request::hasVar check and only change the retrieval call.htdocs/modules/system/admin/comments/main.php (1)
161-165:⚠️ Potential issue | 🟡 MinorNormalize posted ID arrays before using them in purge queries.
Request::getArray()preserves raw POST values. Here those values are used as user IDs, group IDs, and comment IDs without per-element validation, so malformed strings can still driveCriteria(...)andgetUsersByGroup(...). Cast/filter each element tointfirst, and only set$verifonce at least one valid ID was added.🛡️ Proposed hardening
$comments_userid = Request::getArray('comments_userid', [], 'POST'); if (!empty($comments_userid)) { foreach ($comments_userid as $del) { - $criteria->add(new Criteria('com_uid', $del), 'OR'); + if (!is_numeric($del)) { + continue; + } + $criteria->add(new Criteria('com_uid', (int)$del), 'OR'); + $verif = true; } - $verif = true; } $comments_groupe = Request::getArray('comments_groupe', [], 'POST'); if (!empty($comments_groupe)) { - foreach ($comments_groupe as $del => $u_name) { - /** `@var` XoopsMemberHandler $member_handler */ - $member_handler = xoops_getHandler('member'); - $members = $member_handler->getUsersByGroup($u_name, true); + /** `@var` XoopsMemberHandler $member_handler */ + $member_handler = xoops_getHandler('member'); + foreach ($comments_groupe as $groupId) { + if (!is_numeric($groupId)) { + continue; + } + $members = $member_handler->getUsersByGroup((int)$groupId, true); $mcount = count($members); if ($mcount > 4000) { redirect_header('admin.php?fct=comments', 2, _MP_DELETECOUNT); @@ - $verif = true; } $commentslist_id = Request::getArray('commentslist_id', [], 'POST'); if (!empty($commentslist_id)) { - $commentslist_count = count($commentslist_id); - if ($commentslist_count > 0) { - for ($i = 0; $i < $commentslist_count; ++$i) { - $criteria->add(new Criteria('com_id', $commentslist_id[$i]), 'OR'); - } + foreach ($commentslist_id as $commentId) { + if (!is_numeric($commentId)) { + continue; + } + $criteria->add(new Criteria('com_id', (int)$commentId), 'OR'); + $verif = true; } - $verif = true; }As per coding guidelines, "All user input must be filtered using Xmf\Request::getVar() or Xmf\FilterInput::clean() — never access $_GET, $_POST, or $_REQUEST directly".
Also applies to: 168-173, 184-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/comments/main.php` around lines 161 - 165, Normalize and validate the posted ID arrays returned by Request::getArray() (e.g., $comments_userid, other arrays used at lines referenced) before using them in queries: iterate each element, cast/filter each value to int (or use Xmf\FilterInput::clean/getVar) and skip non-positive/non-numeric results, then only add Criteria('com_uid', $id) (or call getUsersByGroup with the cleaned id) for validated ids and set $verif only if at least one valid id was added; apply the same per-element normalization to the arrays used around the other affected blocks (lines 168-173, 184-189) to avoid injecting malformed strings into Criteria/getUsersByGroup.htdocs/modules/profile/admin/field.php (1)
355-359:⚠️ Potential issue | 🔴 CriticalRequire POST + XOOPS token validation for the toggle action.
This still changes persisted field metadata from request parameters without
$GLOBALS['xoopsSecurity']->check(), so an authenticated admin can be CSRFed into flippingfield_required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/field.php` around lines 355 - 359, The 'toggle' case reads Request::getInt('field_id'/'field_required') and calls profile_visible_toggle() without checking request method or XOOPS token; require the request to be POST and call $GLOBALS['xoopsSecurity']->check() before performing the toggle. Specifically, in the 'toggle' branch verify $_SERVER['REQUEST_METHOD'] === 'POST' (or Request::getMethod()), then call $GLOBALS['xoopsSecurity']->check() and bail with an error/redirect if it fails, and only then retrieve field_id/field_required and invoke profile_visible_toggle(); this ensures CSRF protection for the toggle action.htdocs/modules/profile/admin/user.php (1)
144-147:⚠️ Potential issue | 🟡 MinorDrop the dead second argument to
getValueForSave().
ProfileField::getValueForSave()only declares one parameter, so the extra$user->getVar(...)/$profile->getVar(...)value is never consumed. Keeping it here makes the save path look like it is doing a merge with the old value when it is not.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/user.php` around lines 144 - 147, The calls to ProfileField::getValueForSave are passing an unused second argument which misleads readers into thinking the old value is merged; update the two call sites ($fields[$i]->getValueForSave(...)) to pass only the single declared parameter ($fieldValue) and remove the extra $user->getVar(...) and $profile->getVar(...) arguments so getValueForSave is invoked with its correct signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/profile/admin/category.php`:
- Around line 63-67: The code only checks Request::hasVar('id','POST') so edit
submissions that keep id in the query string fall back to handler->create()
instead of loading the existing record; change the check to include GET/REQUEST
(e.g. Request::hasVar('id','REQUEST') or check both POST and GET) and use
Request::getInt('id', 0, 'REQUEST') when calling handler->get(...) so
handler->get(...) loads the existing ProfileCategory instead of creating a new
one; update the block around handler->get(...) / handler->create() accordingly
(references: Request::hasVar, Request::getInt, handler->get, handler->create,
ProfileCategory::getForm).
In `@htdocs/modules/profile/admin/step.php`:
- Around line 96-99: The toggle action must be made POST-only and stop
re-reading step_id from GET: change request checks to use POST (e.g.
Request::hasVar('step_save','POST') and Request::getInt('step_id', 0, 'POST'))
and pass the validated $step_id into profile_stepsave_toggle($step_id) without
letting that function re-fetch from Request; then update
profile_stepsave_toggle() to accept and use its $step_id parameter (remove or
guard any internal Request::getInt('step_id', ...) call) so the handler relies
only on the validated POSTed ID and is not CSRFable.
- Around line 57-65: The save branch handling Request::hasVar('id','POST') needs
XOOPS security validation: call $GLOBALS['xoopsSecurity']->check() early in the
POST handling path (before using $handler->get/$handler->create and before
persisting via the object with setVar calls) and abort/redirect with an error
message if check() returns false so forged POSTs cannot create/update
registration steps; ensure the check is performed for both create and update
flows surrounding the use of Request::getInt/getString and the subsequent save
logic.
- Around line 75-76: The delete flow currently proceeds as soon as
Request::getInt('ok', 0, 'POST') === 1 without validating the XOOPS security
token; add a token check (use $GLOBALS['xoopsSecurity']->check() or the
appropriate XOOPS token validation helper) immediately after detecting ok===1
and before calling the handler delete logic (the code that operates on $obj or
$handler->delete($obj)); if the token check fails, abort/redirect with an error
rather than performing the delete. Ensure the check validates the POST token
generated by xoops_confirm() so CSRF is prevented.
In `@htdocs/modules/profile/admin/user.php`:
- Around line 61-62: The code calls $handler->getUser(...) and immediately
dereferences its return (e.g., ->getGroups(), ->setVar()) which can be null;
update each usage of $handler->getUser (the instance stored in $obj) to check
that $obj is a valid object before calling methods on it, and if not found
return/redirect with an error (fail closed) instead of proceeding; apply this
guard in the checks around the admin-group test (where getGroups() is used), the
edit/save/delete flows (where setVar() and other user methods are invoked), and
the other locations noted (the blocks around the second and third occurrences),
ensuring any downstream logic uses the validated $obj.
In `@htdocs/modules/system/admin/comments/main.php`:
- Around line 61-64: The dropdowns lose their selected state because the code
reads Request::getInt('status', 0) and Request::getInt('module', 0) but the
form/pager use 'comments_status' and 'comments_module'; update the
Request::getInt calls that set $statusVal and $module to use 'comments_status'
and 'comments_module' respectively so they match the form/pager keys (keep the
existing validation against $status_array and existing defaults) and ensure any
other references in this file use those same request keys to preserve selection
state.
In `@htdocs/modules/system/admin/groupperm.php`:
- Around line 43-47: The code currently reads $perms via
Request::getArray('perms', [], 'POST') and immediately calls
$gperm_handler->deleteByModule(...) which removes permissions before validating
each $perm_data; this can lead to TypeError and data loss if a malformed perms
entry (e.g. perms[foo]=1) makes $perm_data a string. Before calling
deleteByModule, iterate $perms and validate that for each $perm_name the
$perm_data is an array and that $perm_data['groups'], $perm_data['parents'], and
$perm_data['itemname'] exist and are arrays (use is_array checks); only proceed
to call $gperm_handler->deleteByModule($modid, $perm_name) for perm_names that
pass validation (skip/ignore invalid entries and optionally log or collect
errors), and then perform the subsequent add/grant logic on those validated
entries to avoid deleting permissions when payload structure is invalid.
---
Outside diff comments:
In `@htdocs/modules/pm/viewpmsg.php`:
- Around line 58-62: The code uses json_decode(Request::getString(...)) into
$clean_msg_id but then calls count($clean_msg_id), which can throw a TypeError
if json_decode returns null or a non-array; ensure $clean_msg_id is an array
before using array_map or count by checking is_array($clean_msg_id) (or casting
the decoded value to an array) and defaulting to an empty array when it's not,
so update the handling around the json_decode call and the subsequent
array_map/count logic for $clean_msg_id.
---
Duplicate comments:
In `@htdocs/banners.php`:
- Around line 389-391: The password is still being filtered by Request::getText
(which calls FilterInput::clean); replace the call to Request::getText('pass',
'', 'POST') with a raw retrieval that uses Request::MASK_ALLOW_RAW (e.g., use
Request::getVar or Request::getString with the Request::MASK_ALLOW_RAW flag) so
the password bytes are returned unchanged for exact hash comparison; keep the
Request::hasVar check and only change the retrieval call.
In `@htdocs/modules/profile/admin/field.php`:
- Around line 355-359: The 'toggle' case reads
Request::getInt('field_id'/'field_required') and calls profile_visible_toggle()
without checking request method or XOOPS token; require the request to be POST
and call $GLOBALS['xoopsSecurity']->check() before performing the toggle.
Specifically, in the 'toggle' branch verify $_SERVER['REQUEST_METHOD'] ===
'POST' (or Request::getMethod()), then call $GLOBALS['xoopsSecurity']->check()
and bail with an error/redirect if it fails, and only then retrieve
field_id/field_required and invoke profile_visible_toggle(); this ensures CSRF
protection for the toggle action.
In `@htdocs/modules/profile/admin/user.php`:
- Around line 144-147: The calls to ProfileField::getValueForSave are passing an
unused second argument which misleads readers into thinking the old value is
merged; update the two call sites ($fields[$i]->getValueForSave(...)) to pass
only the single declared parameter ($fieldValue) and remove the extra
$user->getVar(...) and $profile->getVar(...) arguments so getValueForSave is
invoked with its correct signature.
In `@htdocs/modules/system/admin/comments/main.php`:
- Around line 161-165: Normalize and validate the posted ID arrays returned by
Request::getArray() (e.g., $comments_userid, other arrays used at lines
referenced) before using them in queries: iterate each element, cast/filter each
value to int (or use Xmf\FilterInput::clean/getVar) and skip
non-positive/non-numeric results, then only add Criteria('com_uid', $id) (or
call getUsersByGroup with the cleaned id) for validated ids and set $verif only
if at least one valid id was added; apply the same per-element normalization to
the arrays used around the other affected blocks (lines 168-173, 184-189) to
avoid injecting malformed strings into Criteria/getUsersByGroup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30808dfa-93b1-4056-810b-2d47a5b2d360
📒 Files selected for processing (9)
htdocs/banners.phphtdocs/class/xoopseditor/tinymce/tinymce/jscripts/tiny_mce/plugins/xoopsemotions/xoopsemotions.phphtdocs/modules/pm/viewpmsg.phphtdocs/modules/profile/admin/category.phphtdocs/modules/profile/admin/field.phphtdocs/modules/profile/admin/step.phphtdocs/modules/profile/admin/user.phphtdocs/modules/system/admin/comments/main.phphtdocs/modules/system/admin/groupperm.php
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 92 out of 92 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
htdocs/xoops_lib/modules/protector/filters_disabled/postcommon_register_insert_js_check.php:19
- The behavior of
execute()inpostcommon_register_insert_js_check.phphas changed in a security-relevant way. The old check!empty($_POST)fired on any POST request, meaning every form submission was required to carry a validantispam_md5value. The new checkhasVar('antispam_md5', 'POST')only triggers the validation when that field is already present in the request. This means a bot can bypass the anti-spam check entirely by simply omitting theantispam_md5field from its POST request. The intent was to protect all registration-targeted POST requests, so the condition should instead check whether the current request targets the registration form (e.g., check if the request URI matchesregister.php) or at minimum retain the original behavior of requiring the field to be present and valid whenever any POST data is sent.
htdocs/viewpmsg.php
Outdated
| echo implode('<br>', $GLOBALS['xoopsSecurity']->getErrors()); | ||
| exit(); | ||
| } elseif (empty($_REQUEST['ok'])) { | ||
| } elseif (Request::getInt('ok', 0) === 0) { |
There was a problem hiding this comment.
In htdocs/viewpmsg.php and htdocs/readpmsg.php, the ok confirmation parameter is read via Request::getInt('ok', 0) without specifying a hash (defaults to $_REQUEST, which includes GET+POST+COOKIE). Per the established codebase convention for admin delete flows (see htdocs/modules/profile/admin/category.php, step.php, field.php, user.php), the ok parameter must be read exclusively from the 'POST' hash to prevent CSRF-like GET-triggered confirmation bypass. It should be Request::getInt('ok', 0, 'POST').
| $sample_form->setExtra('enctype="multipart/form-data"'); | ||
| // Not required but for user-friendly concern | ||
| $editor = !empty($_REQUEST['editor']) ? $_REQUEST['editor'] : ''; | ||
| $editor = \Xmf\Request::getString('editor', '', 'POST'); |
There was a problem hiding this comment.
In sampleform.inc.php, $editor was previously read from $_REQUEST['editor'] (GET or POST), but the new code only reads from 'POST'. This is a behavioral change: if the editor selector value was historically passed via a GET parameter (e.g., in a link or bookmark), it will now be silently ignored and the cookie-stored value will be used instead. If the field is only ever submitted via a form POST, this is fine; but if it can also arrive via GET, the hash should be broadened or use both Request::getString('editor', '', 'GET') ?: Request::getString('editor', '', 'POST').
| $editor = \Xmf\Request::getString('editor', '', 'POST'); | |
| $editor = \Xmf\Request::getString('editor', '', 'GET') ?: \Xmf\Request::getString('editor', '', 'POST'); |
| public function getStringTrimsSessionId(): void | ||
| { | ||
| $_POST['ssl_session'] = ' abc123def456 '; | ||
| $value = Request::getString('ssl_session', '', 'POST'); | ||
| $this->assertSame('abc123def456', $value); | ||
| } |
There was a problem hiding this comment.
The test at line 246-251 asserts that Request::getString('ssl_session', '', 'POST') trims whitespace from the session ID value ' abc123def456 ' and returns 'abc123def456'. However, Request::getString() (from the XMF library) uses FILTER_SANITIZE_STRING/FILTER_DEFAULT for sanitization but does not inherently trim leading/trailing whitespace. This assertion may fail unless XMF's implementation does trim. Please verify the actual behavior of Request::getString() regarding whitespace trimming; if it does not trim, the test will produce a false positive (passing today but breaking on an XMF update) or it actually fails now.
| public function getCmdAllowsValidDbPrefix(): void | ||
| { | ||
| $_POST['new_prefix'] = 'xoops_2024'; | ||
| // getCmd allows [A-Za-z0-9._-], lowercases | ||
| $value = Request::getCmd('new_prefix', '', 'POST'); | ||
| $this->assertSame('xoops_2024', $value); | ||
| } |
There was a problem hiding this comment.
The getCmdAllowsValidDbPrefix test at line 292-298 asserts that Request::getCmd('new_prefix', '', 'POST') returns 'xoops_2024' for input 'xoops_2024'. However, getCmd allows [A-Za-z0-9._-] and an underscore _ is not in that allowed set for getCmd (it allows . and - but not _). This means 'xoops_2024' would have the underscore stripped and return 'xoops2024', causing the assertion to fail. Please verify the exact character set accepted by Request::getCmd() in the version of XMF used.
htdocs/readpmsg.php
Outdated
| echo implode('<br>', $GLOBALS['xoopsSecurity']->getErrors()); | ||
| exit(); | ||
| } elseif (empty($_REQUEST['ok'])) { | ||
| } elseif (Request::getInt('ok', 0) === 0) { |
There was a problem hiding this comment.
Same issue as in htdocs/viewpmsg.php: Request::getInt('ok', 0) is called without a hash argument, so it reads from $_REQUEST (GET+POST+COOKIE). The ok confirmation parameter should be restricted to 'POST' only: Request::getInt('ok', 0, 'POST'), consistent with the established codebase convention.
| @@ -83,7 +83,7 @@ | |||
| xoops_confirm( | |||
| [ | |||
| 'ok' => 1, | |||
| 'id' => $_REQUEST['id'], | |||
| 'id' => Request::getInt('id', 0), | |||
There was a problem hiding this comment.
In the 'delete' case, Request::getInt('id', 0) is called without specifying a hash (line 75), which reads from $_REQUEST (GET+POST). This is technically correct for the delete flow (the id arrives via GET on the first request and via POST in the confirmation), and is consistent with how the same pattern is used in category.php and field.php. However, in the xoops_confirm() call on line 86, Request::getInt('id', 0) is used again without a hash to populate the hidden form field. Since the confirmation POST will include id in POST, and the initial GET also has it, both should work. The pattern is intentional here, but for clarity and consistency it would be better to use Request::getInt('id', 0, 'REQUEST') explicitly or document why no hash is specified.
htdocs/edituser.php
Outdated
| @@ -42,23 +42,23 @@ | |||
| if (!$GLOBALS['xoopsSecurity']->check()) { | |||
| redirect_header('index.php', 3, _US_NOEDITRIGHT . '<br>' . implode('<br>', $GLOBALS['xoopsSecurity']->getErrors())); | |||
| } | |||
| $uid = XoopsRequest::getInt('uid', 0); | |||
| $uid = Request::getInt('uid', 0); | |||
There was a problem hiding this comment.
Several Request::getCmd, Request::getInt, Request::getString, etc. calls on lines 36, 45, 51, 56, 61, 81 do not pass an explicit hash ('POST'/'GET'). Omitting the hash causes Xmf\Request to read from $_REQUEST (GET + POST + COOKIE combined), which is the exact antipattern this migration is meant to fix. Each of these should specify 'POST' since this is a form-submit handler. For example: Request::getCmd('op', 'editprofile', 'POST'), Request::getInt('uid', 0, 'POST'), etc.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
htdocs/modules/profile/admin/step.php (1)
128-146:⚠️ Potential issue | 🟡 MinorRename
$step_dparameter to camelCase for PSR-12 compliance.PHPMD flags
$step_das not following camelCase naming. The parameter should be$stepId(or similar) to comply with PSR-12 standards. Also consider adding type hints for PHP 8.2+ compatibility.♻️ Proposed refactor
/** - * `@param` $step_d - * `@param` $step_save + * `@param` int $stepId + * `@param` int $stepSave */ -function profile_stepsave_toggle($step_d, $step_save) +function profile_stepsave_toggle(int $stepId, int $stepSave): void { - $step_save = ($step_save == 1) ? 0 : 1; + $stepSave = ($stepSave === 1) ? 0 : 1; $handler = xoops_getModuleHandler('regstep'); - $obj = $handler->get((int) $step_d); + $obj = $handler->get($stepId); if (!is_object($obj)) { redirect_header('step.php', 1, _PROFILE_AM_SAVESTEP_TOGGLE_FAILED); } - $obj->setVar('step_save', $step_save); + $obj->setVar('step_save', $stepSave); if ($handler->insert($obj, true)) { redirect_header('step.php', 1, _PROFILE_AM_SAVESTEP_TOGGLE_SUCCESS); } else { redirect_header('step.php', 1, _PROFILE_AM_SAVESTEP_TOGGLE_FAILED); } }Note: With typed parameters, the
(int)cast at line 136 becomes unnecessary. Also, using strict comparison (===) at line 134 is preferable for type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/step.php` around lines 128 - 146, Rename the function parameter $step_d to camelCase like $stepId in profile_stepsave_toggle and update all internal references (including the call to $handler->get((int) $step_d)) to use $stepId; also rename $step_save to $stepSave for consistency and update the toggle logic to use strict comparison (===) instead of ==, remove the redundant (int) cast if you add typed signatures, and add PHP 8.2+ type hints (e.g., int $stepId, int $stepSave) to the profile_stepsave_toggle signature to improve type safety.htdocs/modules/system/admin/groups/main.php (1)
302-317:⚠️ Potential issue | 🔴 CriticalGuard
$objbefore entering the delete flow.
$groups_Handler->get($groups_id)can return a non-object for a stale or already-deleted id. This branch still reaches$obj->getVar('name')in the confirm view, and the confirmed-delete path can also proceed with an invalid group reference.🛡️ Proposed fix
if ($groups_id > 0) { $obj = $groups_Handler->get($groups_id); + if (!is_object($obj)) { + redirect_header('admin.php?fct=groups', 1, _AM_SYSTEM_DBERROR); + break; + } if (Request::getInt('ok', 0, 'POST') === 1) { if (!$GLOBALS['xoopsSecurity']->check()) { redirect_header('admin.php?fct=groups', 3, implode(',', $GLOBALS['xoopsSecurity']->getErrors()));Also applies to: 329-336
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/groups/main.php` around lines 302 - 317, The code calls $groups_Handler->get($groups_id) and assigns it to $obj but never verifies $obj is a valid object before using it in the confirm view or in the deletion flow; guard both the confirm branch (where $obj->getVar('name') is used) and the confirmed-delete path by checking that $obj is an object (and non-null) before proceeding—if $obj is not valid, redirect back or show an error; update the logic around $obj, $groups_id, the delete-confirmation rendering and the block that executes delete (uses $member_handler->getGroup, $member_handler->deleteGroup, and $gperm_handler->deleteByGroup) to early-return or redirect when $obj is invalid.
♻️ Duplicate comments (3)
htdocs/modules/profile/admin/step.php (2)
83-108: 🧹 Nitpick | 🔵 TrivialDelete case: security and validation look good, but
$_SERVER['REQUEST_URI']remains unfiltered.The CSRF protection and object validation are correctly implemented. However, line 104 still accesses
$_SERVER['REQUEST_URI']directly. Per coding guidelines, avoid direct superglobal access.Consider using
Request::getUrl('REQUEST_URI', 'SERVER')or sanitizing the value:♻️ Proposed fix
xoops_confirm( [ 'ok' => 1, 'id' => $obj->getVar('step_id'), 'op' => 'delete', ], - $_SERVER['REQUEST_URI'], + Request::getString('REQUEST_URI', '', 'SERVER'), sprintf(_PROFILE_AM_RUSUREDEL, $obj->getVar('step_name')), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/step.php` around lines 83 - 108, Replace the direct use of $_SERVER['REQUEST_URI'] in the xoops_confirm call with the Request wrapper to avoid unfiltered superglobal access; specifically update the third argument of xoops_confirm to use Request::getUrl('REQUEST_URI', 'SERVER') (or Request::getString('REQUEST_URI', '', 'SERVER') if your framework requires a string getter) so the return URL is retrieved via the Request API and properly sanitized when invoking xoops_confirm in the delete case handling.
30-30: 🧹 Nitpick | 🔵 TrivialNested ternary still present — extract for readability.
SonarCloud flags this nested ternary, and a previous review suggested the same refactor. Consider using explicit conditionals or a
matchexpression:♻️ Proposed refactor
-$op = Request::hasVar('op', 'POST') ? Request::getCmd('op', 'list', 'POST') : Request::getCmd('op', (Request::hasVar('id', 'GET') || Request::hasVar('id', 'POST')) ? 'edit' : 'list', 'GET'); +if (Request::hasVar('op', 'POST')) { + $op = Request::getCmd('op', 'list', 'POST'); +} elseif (Request::hasVar('id', 'GET') || Request::hasVar('id', 'POST')) { + $op = Request::getCmd('op', 'edit', 'GET'); +} else { + $op = Request::getCmd('op', 'list', 'GET'); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/step.php` at line 30, The nested ternary assigning $op using Request::hasVar and Request::getCmd is hard to read; replace it with an explicit conditional block (if/else) or a match expression: first check Request::hasVar('op','POST') to branch to Request::getCmd('op','list','POST'), otherwise check (Request::hasVar('id','GET') || Request::hasVar('id','POST')) to choose Request::getCmd('op','edit','GET') or default to Request::getCmd('op','list','GET'); update the code around the $op assignment (where Request::hasVar and Request::getCmd are used) so the logic is clear and un-nested.htdocs/modules/system/admin/groups/main.php (1)
227-232:⚠️ Potential issue | 🔴 CriticalBroaden the
getGroup()miss check beyondnull.This new guard is likely too narrow. If
XoopsMemberHandler::getGroup()forwards the underlying handler’sfalsesentinel on a miss, a staleg_idstill falls through tosetVar()and fatals on PHP 8.x.🛡️ Proposed fix
$group = $member_handler->getGroup($gid); - if ($group === null) { + if (!is_object($group)) { redirect_header('admin.php?fct=groups', 1, _AM_SYSTEM_DBERROR); + break; } $group->setVar('name', Request::getString('name', '', 'POST'));Run this to confirm the
getGroup()return contract:#!/bin/bash set -eu echo "Locate XoopsMemberHandler::getGroup():" rg -n -C3 --type=php 'function\s+&?getGroup\s*\(' htdocs echo echo "Inspect the underlying group handler return contract:" sed -n '141,165p' htdocs/kernel/group.phpExpected result:
getGroup()delegates togHandler->get(...), andhtdocs/kernel/group.phpshowsfalseis returned for a missing group. If so, this branch should match thegroups_editguard and use!is_object($group).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/groups/main.php` around lines 227 - 232, The guard after calling XoopsMemberHandler::getGroup($gid) is too narrow (checks === null); change it to test for a non-object (e.g., !is_object($group)) so false return values from the underlying handler are caught before calling $group->setVar(), matching the other groups_edit guard; keep the redirect_header('admin.php?fct=groups', 1, _AM_SYSTEM_DBERROR) behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@htdocs/modules/profile/admin/step.php`:
- Around line 128-146: Rename the function parameter $step_d to camelCase like
$stepId in profile_stepsave_toggle and update all internal references (including
the call to $handler->get((int) $step_d)) to use $stepId; also rename $step_save
to $stepSave for consistency and update the toggle logic to use strict
comparison (===) instead of ==, remove the redundant (int) cast if you add typed
signatures, and add PHP 8.2+ type hints (e.g., int $stepId, int $stepSave) to
the profile_stepsave_toggle signature to improve type safety.
In `@htdocs/modules/system/admin/groups/main.php`:
- Around line 302-317: The code calls $groups_Handler->get($groups_id) and
assigns it to $obj but never verifies $obj is a valid object before using it in
the confirm view or in the deletion flow; guard both the confirm branch (where
$obj->getVar('name') is used) and the confirmed-delete path by checking that
$obj is an object (and non-null) before proceeding—if $obj is not valid,
redirect back or show an error; update the logic around $obj, $groups_id, the
delete-confirmation rendering and the block that executes delete (uses
$member_handler->getGroup, $member_handler->deleteGroup, and
$gperm_handler->deleteByGroup) to early-return or redirect when $obj is invalid.
---
Duplicate comments:
In `@htdocs/modules/profile/admin/step.php`:
- Around line 83-108: Replace the direct use of $_SERVER['REQUEST_URI'] in the
xoops_confirm call with the Request wrapper to avoid unfiltered superglobal
access; specifically update the third argument of xoops_confirm to use
Request::getUrl('REQUEST_URI', 'SERVER') (or Request::getString('REQUEST_URI',
'', 'SERVER') if your framework requires a string getter) so the return URL is
retrieved via the Request API and properly sanitized when invoking xoops_confirm
in the delete case handling.
- Line 30: The nested ternary assigning $op using Request::hasVar and
Request::getCmd is hard to read; replace it with an explicit conditional block
(if/else) or a match expression: first check Request::hasVar('op','POST') to
branch to Request::getCmd('op','list','POST'), otherwise check
(Request::hasVar('id','GET') || Request::hasVar('id','POST')) to choose
Request::getCmd('op','edit','GET') or default to
Request::getCmd('op','list','GET'); update the code around the $op assignment
(where Request::hasVar and Request::getCmd are used) so the logic is clear and
un-nested.
In `@htdocs/modules/system/admin/groups/main.php`:
- Around line 227-232: The guard after calling
XoopsMemberHandler::getGroup($gid) is too narrow (checks === null); change it to
test for a non-object (e.g., !is_object($group)) so false return values from the
underlying handler are caught before calling $group->setVar(), matching the
other groups_edit guard; keep the redirect_header('admin.php?fct=groups', 1,
_AM_SYSTEM_DBERROR) behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c4c6d29-a236-481b-8731-2349ed0fc73e
📒 Files selected for processing (2)
htdocs/modules/profile/admin/step.phphtdocs/modules/system/admin/groups/main.php
- Pin all Request calls in edituser.php saveuser handler to 'POST' - Widen getGroup() null check to !is_object() to catch false returns - Add null guard in groups delete case after get() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Rename $step_d/$step_save to $stepId/$stepSave, add int type hints and void return type, use strict comparison (===). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
| #[Test] | ||
| public function getCmdAllowsValidDbPrefix(): void | ||
| { | ||
| $_POST['new_prefix'] = 'xoops_2024'; | ||
| // getCmd allows [A-Za-z0-9._-] and underscores, lowercases | ||
| $value = Request::getCmd('new_prefix', '', 'POST'); | ||
| $this->assertSame('xoops_2024', $value); | ||
| } |
There was a problem hiding this comment.
In the test getCmdAllowsValidDbPrefix at line 313, getCmd() lowercases its output. The test expects 'xoops_2024' to equal Request::getCmd('new_prefix', '', 'POST') when the input is 'xoops_2024'. Since 'xoops_2024' is already lowercase, this passes. However, the corresponding production code in prefix_manager.php uses Request::getString to read new_prefix (line 14), not getCmd. The test is therefore testing the wrong method against this field — getCmd would silently lowercases uppercase prefixes and strips characters not in [A-Za-z0-9._-], whereas getString preserves case and characters. There is a mismatch between what the test exercises and what the code actually does. The test for the prefix field should use Request::getString to match the actual production call.
| #[Test] | ||
| public function getCmdSanitisesPathTraversalInDir(): void | ||
| { | ||
| // getCmd allows [A-Za-z0-9._-] and lowercases | ||
| $_GET['dir'] = '../../etc/passwd'; | ||
| $value = Request::getCmd('dir', '', 'GET'); | ||
| // Slashes are stripped by getCmd | ||
| $this->assertStringNotContainsString('/', $value); | ||
| $this->assertStringNotContainsString('\\', $value); | ||
| } |
There was a problem hiding this comment.
The test getCmdSanitisesPathTraversalInDir at line 68-76 calls Request::getCmd('dir', '', 'GET') without setting any explicit hash in the global, and then checks that slashes are stripped. However, the actual production code in jquery.php uses Request::getString('dir', '') (without a hash, i.e., reads $_REQUEST). This test exercises a different method (getCmd) than what the production code calls (getString). It does not actually verify the behavior of the migrated code and may give false confidence about path traversal protection.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/modules/system/admin/groups/main.php (1)
349-364:⚠️ Potential issue | 🟡 MinorDo not report
_AM_SYSTEM_GROUPS_DBUPDATEDwhen the bulk action was a no-op.If
edit_groupis unexpected,selgroupsis0, or the add path receives an emptymemberslist_id,$errorstaystruebut the branch still redirects with a success message.Possible fix
if ($edit_group !== '') { if ($edit_group === 'add_group' && $selgroups > 0) { foreach ($memberslist_id as $uid) { $member_handler->addUserToGroup($selgroups, (int) $uid); $error = false; } } elseif ($edit_group === 'delete_group' && $selgroups > 0) { $member_handler->removeUsersFromGroup($selgroups, $memberslist_id); $error = false; } - //if ($error === true) - redirect_header('admin.php?fct=users', 1, _AM_SYSTEM_GROUPS_DBUPDATED); + redirect_header( + 'admin.php?fct=users', + 1, + $error ? _AM_SYSTEM_DBERROR : _AM_SYSTEM_GROUPS_DBUPDATED + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/groups/main.php` around lines 349 - 364, The code currently always calls redirect_header(..., _AM_SYSTEM_GROUPS_DBUPDATED) even when no bulk action occurred; update the logic so the success redirect happens only when an action actually modified groups (i.e., when $error is set to false). Constrain the redirect_header call to run only if $error === false after handling edit_group (check the variables edit_group, selgroups, memberslist_id and calls to $member_handler->addUserToGroup and $member_handler->removeUsersFromGroup), otherwise show an appropriate error/notice or avoid the success message.
♻️ Duplicate comments (1)
htdocs/modules/system/admin/groups/main.php (1)
154-157: 🛠️ Refactor suggestion | 🟠 MajorNormalize permission ID arrays before persisting them.
Request::getArray()does not type individual items.system_catids,admin_mids,read_mids, andread_bidsare later copied into integer permission fields, so they should be normalized to ints once at input time (or cast inside each loop), just like the bulk-add path now does formemberslist_id.Verify that
Xmf\Request::getArray()is returning raw item values here and that these four paths still lack per-item integer normalization beforegperm_itemidis assigned. Expected result: the method returns an array without item casting, and the permission loops do not cast$s_cid,$a_mid,$r_mid, or$r_bid.#!/bin/bash set -euo pipefail request_file="$(fd 'Request.php$' htdocs/class/libraries | head -n1)" printf 'Inspecting %s\n\n' "$request_file" sed -n '/function getArray/,/^[[:space:]]*}/p' "$request_file" | sed -n '1,80p' printf '\nCall sites in htdocs/modules/system/admin/groups/main.php:\n' rg -n -C2 "Request::getArray\('(system_catids|admin_mids|read_mids|read_bids)'" htdocs/modules/system/admin/groups/main.php || true printf '\nAssignments to gperm_itemid around the affected loops:\n' rg -n -C2 "gperm_itemid'.*(s_cid|a_mid|r_mid|r_bid)" htdocs/modules/system/admin/groups/main.php || true printf '\nPer-item integer casts for those loop variables:\n' rg -n -C1 "\(int\)\s*\$(s_cid|a_mid|r_mid|r_bid)" htdocs/modules/system/admin/groups/main.php || trueAs per coding guidelines, "All user input must be filtered using Xmf\Request::getVar() or Xmf\FilterInput::clean() — never access
$_GET,$_POST, or$_REQUESTdirectly."Also applies to: 219-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/groups/main.php` around lines 154 - 157, Request::getArray returns raw values so normalize the permission ID arrays immediately: after calling Request::getArray for system_catids, admin_mids, read_mids, and read_bids, map each item to an int (or alternatively cast each loop variable) so that $s_cid, $a_mid, $r_mid and $r_bid are integers before being assigned to gperm_itemid; update the code around the bulk-add path pattern used for memberslist_id to ensure consistent per-item integer casting and remove any reliance on raw input values from Request::getArray.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/edituser.php`:
- Around line 56-61: The password fields are currently fetched with
Request::getString() which applies XSS cleaning and can alter characters before
hashing; replace those calls with Request::getText() when retrieving $password
and $vpass in edituser.php so the raw password text is returned (still cast to
string) before you validate length and compare for hashing/validation; update
the occurrences where Request::getString('password'...) and
Request::getString('vpass'...) are used to Request::getText(...) to fix
authentication breaks caused by character escaping.
---
Outside diff comments:
In `@htdocs/modules/system/admin/groups/main.php`:
- Around line 349-364: The code currently always calls redirect_header(...,
_AM_SYSTEM_GROUPS_DBUPDATED) even when no bulk action occurred; update the logic
so the success redirect happens only when an action actually modified groups
(i.e., when $error is set to false). Constrain the redirect_header call to run
only if $error === false after handling edit_group (check the variables
edit_group, selgroups, memberslist_id and calls to
$member_handler->addUserToGroup and $member_handler->removeUsersFromGroup),
otherwise show an appropriate error/notice or avoid the success message.
---
Duplicate comments:
In `@htdocs/modules/system/admin/groups/main.php`:
- Around line 154-157: Request::getArray returns raw values so normalize the
permission ID arrays immediately: after calling Request::getArray for
system_catids, admin_mids, read_mids, and read_bids, map each item to an int (or
alternatively cast each loop variable) so that $s_cid, $a_mid, $r_mid and $r_bid
are integers before being assigned to gperm_itemid; update the code around the
bulk-add path pattern used for memberslist_id to ensure consistent per-item
integer casting and remove any reliance on raw input values from
Request::getArray.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f413687f-ce88-4f7b-ab4d-8f30911de134
📒 Files selected for processing (2)
htdocs/edituser.phphtdocs/modules/system/admin/groups/main.php
Copyable Unit Test EditsCopyable Editstests/unit/htdocs/class/smarty3_plugins/FunctionXoopsLinkTest.phpThis is a new file. tests/unit/htdocs/class/ThemeTest.phpThis is a new file. tests/unit/htdocs/class/XoopsKernelTest.phpThis is a new file. |
- Change getString() to getText() for all password/pass/vpass fields across 7 files to prevent XSS filtering from altering special characters before hashing - Add PHPDoc docstrings to all test methods in SuperglobalMigrationTest Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| $copy_file = $path_file . '.back'; | ||
| copy($path_file, $copy_file); | ||
| // Save modif | ||
| if (isset($_REQUEST['templates'])) { | ||
| $open = fopen('' . $path_file . '', 'w+'); | ||
| $temp = stripslashes((string) $_REQUEST['templates']); | ||
| if (Request::hasVar('templates', 'POST')) { | ||
| $open = fopen($path_file, 'w+'); | ||
| if ($open === false) { | ||
| redirect_header('admin.php?fct=tplsets', 2, _AM_SYSTEM_TEMPLATES_ERROR); | ||
| } | ||
| $temp = Request::getText('templates', '', 'POST'); | ||
| if (!fwrite($open, xoops_utf8_encode($temp))) { | ||
| fclose($open); | ||
| redirect_header('admin.php?fct=tplsets', 2, _AM_SYSTEM_TEMPLATES_ERROR); | ||
| } | ||
| fclose($open); |
There was a problem hiding this comment.
In the tpls_save handler, XoopsLoad::load('XoopsRequest') and XoopsRequest::getString('path_file', '') are still used (lines 372–373) even though the rest of the file has been migrated to Xmf\Request. This is an inconsistency in the migration: path_file should be read as Request::getString('path_file', '', 'POST') using the already-imported Xmf\Request alias, and the explicit XoopsLoad call should be removed.
| if (Request::getInt('refresh', 0, 'GET') === 0 && 'submit' !== $op) { | ||
| $jump = 'pmlite.php?refresh=' . time() . ''; |
There was a problem hiding this comment.
In htdocs/modules/pm/pmlite.php (the htdocs/ root version, not the modules/pm/ one), the check if (Request::getInt('refresh', 0, 'GET') === 0 && 'submit' !== $op) replaces if (empty($_GET['refresh']) && $op !== 'submit'). The original used empty() which treats the string '0' as falsy — consistent with the previous behavior. The new strict === 0 check is also correct because getInt returns an integer. This change is fine, but note the comment on line 36 still has a trailing empty string concatenation: $jump = 'pmlite.php?refresh=' . time() . ''; — this is a pre-existing nit that wasn't cleaned up.
| if ($open === false) { | ||
| redirect_header('admin.php?fct=tplsets', 2, _AM_SYSTEM_TEMPLATES_ERROR); | ||
| } |
There was a problem hiding this comment.
In the tpls_save case, redirect_header() is called when fopen() returns false (line 389), but there is no exit or return after it. Because redirect_header() does not halt PHP execution, the code will continue executing into fwrite() using $open = false, causing a PHP fatal error ("fwrite(): supplied argument is not a valid stream resource"). An exit; must be added immediately after the redirect_header() call on line 389.
| * Verify getCmd returns a clean op value. | ||
| */ | ||
| #[Test] | ||
| public function getCmdReturnsCleanOp(): void | ||
| { | ||
| $_REQUEST['op'] = 'results'; | ||
| $this->assertSame('results', Request::getCmd('op', 'search')); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getCmd defaults to 'search' when op is missing. | ||
| */ | ||
| #[Test] | ||
| public function getCmdDefaultsToSearchForMissingOp(): void | ||
| { | ||
| unset($_REQUEST['op']); | ||
| $this->assertSame('search', Request::getCmd('op', 'search')); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getCmd strips SQL injection from op parameter. | ||
| */ | ||
| #[Test] | ||
| public function getCmdStripsInjectionFromOp(): void | ||
| { | ||
| $_REQUEST['op'] = "results'; DROP TABLE users; --"; | ||
| $value = Request::getCmd('op', 'search'); | ||
| // getCmd strips everything except [A-Za-z0-9._-] | ||
| $this->assertStringNotContainsString("'", $value); | ||
| $this->assertStringNotContainsString(';', $value); | ||
| $this->assertStringNotContainsString(' ', $value); | ||
| } |
There was a problem hiding this comment.
In the test file, several tests (e.g. getCmdReturnsCleanOp, getCmdDefaultsToSearchForMissingOp, getIntReturnsIntegerForLimit, etc.) call Request::getCmd(), Request::getInt(), etc. without specifying an explicit hash parameter (e.g. no 'GET', 'POST', or 'COOKIE' argument). This means these tests read from $_REQUEST (the combined superglobal), not from a specific hash. This makes these tests less effective as regression tests for the hash-separation that is the core goal of this migration, since they don't verify that the specific hash separation is working. The tests at section 11 (postHashDoesNotReadGetValues, getHashDoesNotReadPostValues) correctly use explicit hashes, but the earlier tests (sections 3–8, 12–15) test $_REQUEST fallback behavior rather than pinned-hash behavior.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
htdocs/modules/system/admin/groups/main.php (1)
312-320:⚠️ Potential issue | 🟡 MinorAdd null check after
getGroup()beforedeleteGroup().At line 315,
$member_handler->getGroup($groups_id)can returnnullif the group was already deleted (race condition). Line 316 then passes$grouptodeleteGroup(), which will fail or produce undefined behavior in PHP 8.x if$groupis null.🛡️ Proposed fix
/** `@var` XoopsMemberHandler $member_handler */ $member_handler = xoops_getHandler('member'); $group = $member_handler->getGroup($groups_id); + if (!is_object($group)) { + redirect_header('admin.php?fct=groups', 1, _AM_SYSTEM_DBERROR); + } $member_handler->deleteGroup($group);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/groups/main.php` around lines 312 - 320, getGroup($groups_id) can return null; add a null check after calling $member_handler->getGroup($groups_id) and only call $member_handler->deleteGroup($group) when $group is not null to avoid passing null into deleteGroup; still call $gperm_handler->deleteByGroup($groups_id) to clean up permissions and then proceed with the existing redirect_header('admin.php?fct=groups', 1, _AM_SYSTEM_GROUPS_DBUPDATED).htdocs/register.php (1)
63-69:⚠️ Potential issue | 🟠 MajorRead the posted timezone from
timezone_offset.Line 69 still fetches
cid, but this form poststimezone_offsetand the confirmation form preserves that same key at Lines 121-122. As written, every registration falls back to$xoopsConfig['default_TZ']and ignores the user's selection.Suggested fix
- $timezone_offset = Request::getFloat('cid', $xoopsConfig['default_TZ'], 'POST'); + $timezone_offset = Request::getFloat('timezone_offset', $xoopsConfig['default_TZ'], 'POST');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/register.php` around lines 63 - 69, The timezone is being read from the wrong POST key: change the Request::getFloat call that assigns $timezone_offset to read 'timezone_offset' instead of 'cid' so the user's posted timezone selection is used (keep $xoopsConfig['default_TZ'] as the default value). Update the Request::getFloat invocation that sets $timezone_offset to use the 'timezone_offset' key.
♻️ Duplicate comments (4)
tests/unit/htdocs/migration/SuperglobalMigrationTest.php (4)
22-26: 🛠️ Refactor suggestion | 🟠 MajorAdd the required XOOPS metadata tags to the class docblock.
The file header is present now, but
SuperglobalMigrationTeststill omits the mandated@category,@package,@author,@copyright,@license, and@linktags.As per coding guidelines, "Class docblocks must include `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link` tags".♻️ Suggested fix
/** * Each test sets up a superglobal, calls Request::get*(), and asserts * the value matches what the OLD raw-superglobal code would have produced. * If these pass after migration, the behaviour is preserved. + * + * `@category` Test + * `@package` Tests + * `@author` XOOPS Development Team + * `@copyright` (c) 2000-2026 XOOPS Project (https://xoops.org) + * `@license` GNU GPL 2.0 or later (https://www.gnu.org/licenses/gpl-2.0.html) + * `@link` https://xoops.org */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/migration/SuperglobalMigrationTest.php` around lines 22 - 26, The class docblock for SuperglobalMigrationTest is missing required XOOPS metadata tags; update the docblock above the class SuperglobalMigrationTest to include `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link` tags with appropriate values (match project conventions for category/package and fill author/copyright/license/link metadata).
17-19:⚠️ Potential issue | 🟠 MajorUse PHPUnit 9.6-compatible discovery for this class.
#[Test]and#[CoversNothing]make this file depend on PHPUnit 10+ discovery, and none of the methods starts withtest, so the suite disappears on PHPUnit 9.6. Remove the attribute-based discovery, movecoversNothingto a DocBlock annotation, and rename the methods totest....Does PHPUnit 9.6 support PHPUnit\Framework\Attributes\Test and PHPUnit\Framework\Attributes\CoversNothing, or are those attributes only available in PHPUnit 10+?As per coding guidelines, "Tests must work across PHPUnit 9.6, 10.5, and 11.x" and "Name test methods test{MethodName} or test{MethodName}{Scenario}".
Also applies to: 27-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/migration/SuperglobalMigrationTest.php` around lines 17 - 19, The test class SuperglobalMigrationTest currently uses PHPUnit 10+ attribute discovery (#[Test] and #[CoversNothing]) which breaks PHPUnit 9.6; remove those attributes from the class and methods, add a DocBlock on the class with `@coversNothing`, and rename each test method to start with "test" (e.g., change method names that rely on #[Test] to test{OriginalName} and update any references), ensuring you also update the two other methods mentioned (the ones at the other attribute lines) to follow the same renaming and removal of #[Test]/#[CoversNothing].
294-307:⚠️ Potential issue | 🟠 MajorStrengthen this redirect regression with a behavior assertion.
assertIsString()plusassertNotSame('', $value)still passes ifRequest::getUrl()returns the wrong non-empty value, so this test will not catch a migration regression. Assert the expected sanitized URL instead.As per coding guidelines, "tests/**/*.php: Review test code for proper assertions, test isolation, and edge case coverage."♻️ Suggested fix
$_POST['not_redirect'] = 'https://evil.com/phish'; $value = Request::getUrl('not_redirect', '', 'POST'); - // getUrl preserves valid URLs — domain validation is the app's responsibility - // (see notification_update.php parse_url() check). This test verifies getUrl - // returns a non-empty string for well-formed URLs. - $this->assertIsString($value); - $this->assertNotSame('', $value); + // getUrl preserves well-formed URLs; domain validation is handled by the caller. + $this->assertSame('https://evil.com/phish', $value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/migration/SuperglobalMigrationTest.php` around lines 294 - 307, Replace the loose assertions in getUrlReturnsValidUrlFormat to assert the exact expected sanitized URL so the test fails on regressions: after setting $_POST['not_redirect'] and calling Request::getUrl('not_redirect', '', 'POST'), assert that the returned value equals the expected URL (e.g. "https://evil.com/phish") instead of using assertIsString/assertNotSame; update the assertion in the getUrlReturnsValidUrlFormat test accordingly to use assertSame against Request::getUrl's result.
42-48:⚠️ Potential issue | 🟠 MajorReset the request globals after backing them up.
These tests include missing-input cases, but
setUp()currently carries forward whatever request state the runner bootstrapped. Start each test from empty hashes so the negatives stay deterministic.Based on learnings, "Tests must be fully isolated with no XOOPS installation required for unit tests".♻️ Suggested fix
protected function setUp(): void { $this->backupRequest = $_REQUEST; $this->backupPost = $_POST; $this->backupGet = $_GET; $this->backupCookie = $_COOKIE; + + $_REQUEST = []; + $_POST = []; + $_GET = []; + $_COOKIE = []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/migration/SuperglobalMigrationTest.php` around lines 42 - 48, The setUp() in SuperglobalMigrationTest captures current superglobals but doesn't clear them, causing non-deterministic negative tests; after backing up $this->backupRequest/$this->backupPost/$this->backupGet/$this->backupCookie in setUp(), explicitly reset the runtime superglobals ($_REQUEST, $_POST, $_GET, $_COOKIE) to empty arrays so each test starts from a clean state, and ensure tearDown() (or add one) restores the originals from those backup properties to avoid leaking state between tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/install/page_siteinit.php`:
- Around line 42-43: The code is trimming admin passwords and thus can alter
user-entered secrets; remove the trim() calls around the password retrieval so
the raw POST values are preserved: replace the trimmed assignments for
$vars['adminpass'] and $vars['adminpass2'] (which call
trim(\Xmf\Request::getText(...))) with direct assignments from
\Xmf\Request::getText('adminpass', '', 'POST') and
\Xmf\Request::getText('adminpass2', '', 'POST') respectively so
comparisons/storage use the exact user input.
In `@htdocs/modules/profile/admin/step.php`:
- Around line 110-118: The toggle branch currently requires a POST token but the
UI still sends GET links, so update the handler in the 'toggle' case to support
the existing GET contract: only run $GLOBALS['xoopsSecurity']->check() for POST
submissions, and if Request::hasVar('step_id','GET') read step_id and step_save
using Request::getInt(...,'GET') and call profile_stepsave_toggle($step_id,
$step_save) for GET requests; alternatively (if you prefer the POST-only
approach) migrate profile_admin_steplist.tpl to submit a POST form with step_id
and step_save and keep the token check—pick one and implement the corresponding
change to either the handler (add GET branch) or the template (convert to POST).
In `@htdocs/modules/profile/changemail.php`:
- Around line 44-46: The password variable $pass is being trimmed before
authentication which breaks verification for passwords that include
leading/trailing spaces; change the assignment for $pass to use the raw POST
value from Request::getText('passwd', '', 'POST') without wrapping it in trim(),
leaving the email assignment trimmed as-is, so password_verify() (or any
subsequent check in changemail.php) receives the exact original submitted
password.
In `@htdocs/modules/system/admin/groups/main.php`:
- Around line 219-222: In the groups_save_update branch, the arrays
$system_catids, $admin_mids, $read_mids, and $read_bids are taken from
Request::getArray but their elements are not cast to integers before being
assigned to gperm_itemid; update each foreach that sets gperm_itemid (the loops
that iterate over $system_catids, $admin_mids, $read_mids, and $read_bids around
the existing gperm_itemid assignments) to cast the item value to (int) when
assigning to gperm_itemid so only integers are stored.
- Around line 154-162: The permission item IDs from
Request::getArray('system_catids'...), Request::getArray('admin_mids'...),
Request::getArray('read_mids'...), and Request::getArray('read_bids'...) are
strings; when creating gperm entries you must cast each iterated value to int to
ensure DB type safety—update the foreach loops that iterate $system_catids (use
(int)$catid when setting gperm_itemid), $admin_mids (cast $a_mid), $read_mids
(cast $r_mid), and $read_bids (cast $r_bid) so the values passed to
gperm->setVar('gperm_itemid', ...) are integers.
In `@htdocs/modules/system/admin/users/main.php`:
- Around line 185-186: The code currently uses Request::getText('pass2') as the
gate for writing/validating the new password, which lets a non-empty
Request::getText('password') be ignored if pass2 is blank; change all such
checks to gate on Request::getText('password') instead (i.e., if password is
non-empty then require that password == pass2 and set/pass it to
insertUser()/updateUser()), update the validation branches that reference
Request::getText('pass2') (including where insertUser() is called) so the
password is persisted whenever password is provided, and keep the confirmation
check (password == pass2) only when a password is supplied. Ensure
Request::getText('password') is the source of truth for deciding to
update/persist the password across the create and edit flows.
- Around line 185-186: Several password checks and assignments are using
Request::getText(), which trims whitespace; replace those calls with
Request::getVar(..., 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM)
so password input is not silently trimmed. Update every occurrence where
passwords are compared or stored (the expressions using
Request::getText('pass2'), Request::getText('password'), and any other password
field retrievals referenced in the review) to use Request::getVar with the
combined MASK_ALLOW_RAW | MASK_NO_TRIM flags so comparisons and hashing use the
raw user input.
---
Outside diff comments:
In `@htdocs/modules/system/admin/groups/main.php`:
- Around line 312-320: getGroup($groups_id) can return null; add a null check
after calling $member_handler->getGroup($groups_id) and only call
$member_handler->deleteGroup($group) when $group is not null to avoid passing
null into deleteGroup; still call $gperm_handler->deleteByGroup($groups_id) to
clean up permissions and then proceed with the existing
redirect_header('admin.php?fct=groups', 1, _AM_SYSTEM_GROUPS_DBUPDATED).
In `@htdocs/register.php`:
- Around line 63-69: The timezone is being read from the wrong POST key: change
the Request::getFloat call that assigns $timezone_offset to read
'timezone_offset' instead of 'cid' so the user's posted timezone selection is
used (keep $xoopsConfig['default_TZ'] as the default value). Update the
Request::getFloat invocation that sets $timezone_offset to use the
'timezone_offset' key.
---
Duplicate comments:
In `@tests/unit/htdocs/migration/SuperglobalMigrationTest.php`:
- Around line 22-26: The class docblock for SuperglobalMigrationTest is missing
required XOOPS metadata tags; update the docblock above the class
SuperglobalMigrationTest to include `@category`, `@package`, `@author`, `@copyright`,
`@license`, and `@link` tags with appropriate values (match project conventions for
category/package and fill author/copyright/license/link metadata).
- Around line 17-19: The test class SuperglobalMigrationTest currently uses
PHPUnit 10+ attribute discovery (#[Test] and #[CoversNothing]) which breaks
PHPUnit 9.6; remove those attributes from the class and methods, add a DocBlock
on the class with `@coversNothing`, and rename each test method to start with
"test" (e.g., change method names that rely on #[Test] to test{OriginalName} and
update any references), ensuring you also update the two other methods mentioned
(the ones at the other attribute lines) to follow the same renaming and removal
of #[Test]/#[CoversNothing].
- Around line 294-307: Replace the loose assertions in
getUrlReturnsValidUrlFormat to assert the exact expected sanitized URL so the
test fails on regressions: after setting $_POST['not_redirect'] and calling
Request::getUrl('not_redirect', '', 'POST'), assert that the returned value
equals the expected URL (e.g. "https://evil.com/phish") instead of using
assertIsString/assertNotSame; update the assertion in the
getUrlReturnsValidUrlFormat test accordingly to use assertSame against
Request::getUrl's result.
- Around line 42-48: The setUp() in SuperglobalMigrationTest captures current
superglobals but doesn't clear them, causing non-deterministic negative tests;
after backing up
$this->backupRequest/$this->backupPost/$this->backupGet/$this->backupCookie in
setUp(), explicitly reset the runtime superglobals ($_REQUEST, $_POST, $_GET,
$_COOKIE) to empty arrays so each test starts from a clean state, and ensure
tearDown() (or add one) restores the originals from those backup properties to
avoid leaking state between tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9969a17e-e47c-43e3-8efb-540b022c21c6
📒 Files selected for processing (10)
htdocs/edituser.phphtdocs/install/page_siteinit.phphtdocs/modules/profile/admin/step.phphtdocs/modules/profile/changemail.phphtdocs/modules/profile/changepass.phphtdocs/modules/profile/register.phphtdocs/modules/system/admin/groups/main.phphtdocs/modules/system/admin/users/main.phphtdocs/register.phptests/unit/htdocs/migration/SuperglobalMigrationTest.php
| $system_catids = Request::getArray('system_catids', [], 'POST'); | ||
| $admin_mids = Request::getArray('admin_mids', [], 'POST'); | ||
| $read_mids = Request::getArray('read_mids', [], 'POST'); | ||
| $read_bids = Request::getArray('read_bids', [], 'POST'); | ||
| /** @var XoopsMemberHandler $member_handler */ | ||
| $member_handler = xoops_getHandler('member'); | ||
| $group = $member_handler->createGroup(); | ||
| $group->setVar('name', $_POST['name']); | ||
| $group->setVar('description', $_POST['desc']); | ||
| $group->setVar('name', Request::getString('name', '', 'POST')); | ||
| $group->setVar('description', Request::getString('desc', '', 'POST')); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cast array elements to int for type safety on permission item IDs.
Request::getArray() returns string values (per FilterInput::process() behavior). When iterating $system_catids, $admin_mids, $read_mids, and $read_bids to set gperm_itemid, these remain as strings. For database consistency and explicit type safety, cast to int in the foreach loops.
♻️ Proposed fix for permission ID casting
foreach ($system_catids as $s_cid) {
$sysperm = $gperm_handler->create();
$sysperm->setVar('gperm_groupid', $groupid);
- $sysperm->setVar('gperm_itemid', $s_cid);
+ $sysperm->setVar('gperm_itemid', (int) $s_cid);
$sysperm->setVar('gperm_name', 'system_admin');Apply the same (int) cast pattern for $a_mid, $r_mid, and $r_bid in their respective loops.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/groups/main.php` around lines 154 - 162, The
permission item IDs from Request::getArray('system_catids'...),
Request::getArray('admin_mids'...), Request::getArray('read_mids'...), and
Request::getArray('read_bids'...) are strings; when creating gperm entries you
must cast each iterated value to int to ensure DB type safety—update the foreach
loops that iterate $system_catids (use (int)$catid when setting gperm_itemid),
$admin_mids (cast $a_mid), $read_mids (cast $r_mid), and $read_bids (cast
$r_bid) so the values passed to gperm->setVar('gperm_itemid', ...) are integers.
| $system_catids = Request::getArray('system_catids', [], 'POST'); | ||
| $admin_mids = Request::getArray('admin_mids', [], 'POST'); | ||
| $read_mids = Request::getArray('read_mids', [], 'POST'); | ||
| $read_bids = Request::getArray('read_bids', [], 'POST'); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same type casting concern applies here.
The groups_save_update case uses identical array parameters. Apply the same (int) cast to array elements when setting gperm_itemid in the foreach loops at lines 259, 268, 277, and 285.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/groups/main.php` around lines 219 - 222, In the
groups_save_update branch, the arrays $system_catids, $admin_mids, $read_mids,
and $read_bids are taken from Request::getArray but their elements are not cast
to integers before being assigned to gperm_itemid; update each foreach that sets
gperm_itemid (the loops that iterate over $system_catids, $admin_mids,
$read_mids, and $read_bids around the existing gperm_itemid assignments) to cast
the item value to (int) when assigning to gperm_itemid so only integers are
stored.
Fixed files:
1. htdocs/install/page_siteinit.php — Removed trim(), switched getText() → getVar() with MASK_ALLOW_RAW | MASK_NO_TRIM for adminpass/adminpass2
2. htdocs/modules/profile/changemail.php — Removed trim(), switched to getVar() with raw+no-trim for passwd
3. htdocs/modules/profile/changepass.php — Removed trim(), switched to getVar() with raw+no-trim for oldpass, newpass, vpass
4. htdocs/modules/system/admin/tplsets/main.php — Replaced XoopsRequest::getString with Request::getString (Xmf), removed
XoopsLoad::load('XoopsRequest') line
5. htdocs/modules/profile/admin/step.php — Reverted toggle handler back to GET (matches template's GET links), removed CSRF check (not needed for
idempotent toggle)
6. htdocs/modules/system/admin/groups/main.php — Added !is_object($group) null guard after getGroup() in delete flow
7. htdocs/modules/system/admin/users/main.php — All getText('password') and getText('pass2') → getVar() with raw+no-trim and explicit 'POST' hash
8. htdocs/edituser.php — getText('password')/getText('vpass') → getVar() with raw+no-trim
9. htdocs/register.php — getText('pass')/getText('vpass') → getVar() with raw+no-trim
10. htdocs/modules/profile/register.php — getText('pass')/getText('vpass') → getVar() with raw+no-trim
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 93 out of 93 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
htdocs/modules/system/admin/users/main.php:216
- At line 193,
Request::getString('uname')is called without an explicit hash parameter. Per the codebase convention (established inhtdocs/edituser.php:36-106), allXmf\Requestcalls must include an explicit hash ('GET','POST', or'COOKIE'). Omitting the hash defaults to reading from$_REQUEST(GET+POST+COOKIE combined), which is the security antipattern being eliminated by this PR. The same issue exists at lines 173, 174, 175, 179, 180, 181, 200-216 where severalRequest::getString(),Request::getEmail(),Request::getUrl(), etc. calls are missing the hash argument.
| $_REQUEST['dir'] = '/mytheme/<script>alert(1)</script>/'; | ||
| $value = Request::getString('dir', ''); | ||
| $this->assertStringNotContainsString('<script>', $value); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getCmd sanitises path traversal sequences in dir. | ||
| */ | ||
| #[Test] | ||
| public function getCmdSanitisesPathTraversalInDir(): void | ||
| { | ||
| // getCmd allows [A-Za-z0-9._-] and lowercases | ||
| $_GET['dir'] = '../../etc/passwd'; | ||
| $value = Request::getCmd('dir', '', 'GET'); | ||
| // Slashes are stripped by getCmd | ||
| $this->assertStringNotContainsString('/', $value); | ||
| $this->assertStringNotContainsString('\\', $value); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getString preserves a valid theme path. | ||
| */ | ||
| #[Test] | ||
| public function getStringPreservesValidThemePath(): void | ||
| { | ||
| $_REQUEST['dir'] = '/starter_theme/templates/'; | ||
| $value = Request::getString('dir', ''); | ||
| $this->assertSame('/starter_theme/templates/', $value); |
There was a problem hiding this comment.
In the test for getStringStripsScriptTagsFromDir (line 72) and getStringPreservesValidThemePath (line 97), Request::getString('dir', '') is called without an explicit hash. The tests set $_REQUEST['dir'] directly, and without a hash, getString reads from $_REQUEST. This means the tests are not testing the POST/GET-pinned behavior that the actual migrated code uses, and they would still pass even if the production code used a wrong hash. The tests should use an explicit hash (e.g. 'GET' or 'POST') and set the corresponding superglobal ($_GET['dir'] or $_POST['dir']), consistent with how other tests in this file work (e.g. lines 84-87).
| /** | ||
| * Verify getCmd returns a clean op value. | ||
| */ | ||
| #[Test] | ||
| public function getCmdReturnsCleanOp(): void | ||
| { | ||
| $_REQUEST['op'] = 'results'; | ||
| $this->assertSame('results', Request::getCmd('op', 'search')); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getCmd defaults to 'search' when op is missing. | ||
| */ | ||
| #[Test] | ||
| public function getCmdDefaultsToSearchForMissingOp(): void | ||
| { | ||
| unset($_REQUEST['op']); | ||
| $this->assertSame('search', Request::getCmd('op', 'search')); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getCmd strips SQL injection from op parameter. | ||
| */ | ||
| #[Test] | ||
| public function getCmdStripsInjectionFromOp(): void | ||
| { | ||
| $_REQUEST['op'] = "results'; DROP TABLE users; --"; | ||
| $value = Request::getCmd('op', 'search'); | ||
| // getCmd strips everything except [A-Za-z0-9._-] | ||
| $this->assertStringNotContainsString("'", $value); | ||
| $this->assertStringNotContainsString(';', $value); | ||
| $this->assertStringNotContainsString(' ', $value); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // 4. profile/search.php — $_REQUEST['uname'] | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * Verify getString preserves a valid username. | ||
| */ | ||
| #[Test] | ||
| public function getStringPreservesValidUname(): void | ||
| { | ||
| $_REQUEST['uname'] = 'john_doe'; | ||
| $this->assertSame('john_doe', Request::getString('uname', '')); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getString strips XSS from username. | ||
| */ | ||
| #[Test] | ||
| public function getStringStripsXssFromUname(): void | ||
| { | ||
| $_REQUEST['uname'] = '<script>alert("xss")</script>'; | ||
| $value = Request::getString('uname', ''); | ||
| $this->assertStringNotContainsString('<script>', $value); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // 5. profile/search.php — $_REQUEST['limit'] and $_REQUEST['start'] | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * Verify getInt returns integer for limit parameter. | ||
| */ | ||
| #[Test] | ||
| public function getIntReturnsIntegerForLimit(): void | ||
| { | ||
| $_REQUEST['limit'] = '25'; | ||
| $this->assertSame(25, Request::getInt('limit', 20)); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getInt strips non-numeric characters from limit. | ||
| */ | ||
| #[Test] | ||
| public function getIntStripsNonNumericFromLimit(): void | ||
| { | ||
| $_REQUEST['limit'] = '25; DROP TABLE'; | ||
| $this->assertSame(25, Request::getInt('limit', 20)); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getInt returns default for missing start parameter. | ||
| */ | ||
| #[Test] | ||
| public function getIntDefaultsForMissingStart(): void | ||
| { | ||
| unset($_REQUEST['start']); | ||
| $this->assertSame(0, Request::getInt('start', 0)); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // 6. profile/search.php — $_REQUEST['order'] | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * Verify getInt converts order string to integer. | ||
| */ | ||
| #[Test] | ||
| public function getIntConvertsOrderToInteger(): void | ||
| { | ||
| $_REQUEST['order'] = '1'; | ||
| $this->assertSame(1, Request::getInt('order', 0)); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getInt returns default for non-numeric order. | ||
| */ | ||
| #[Test] | ||
| public function getIntClampsNonNumericOrder(): void | ||
| { | ||
| $_REQUEST['order'] = 'DESC'; | ||
| $this->assertSame(0, Request::getInt('order', 0)); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // 7. profile/search.php — $_REQUEST['sortby'] | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * Verify getCmd preserves a valid sortby value. | ||
| */ | ||
| #[Test] | ||
| public function getCmdPreservesValidSortby(): void | ||
| { | ||
| $_REQUEST['sortby'] = 'uname'; | ||
| $this->assertSame('uname', Request::getCmd('sortby', '')); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getCmd strips SQL injection from sortby. | ||
| */ | ||
| #[Test] | ||
| public function getCmdStripsInjectionFromSortby(): void | ||
| { | ||
| $_REQUEST['sortby'] = 'uname; DROP TABLE users'; | ||
| $value = Request::getCmd('sortby', ''); | ||
| $this->assertStringNotContainsString(';', $value); | ||
| $this->assertStringNotContainsString(' ', $value); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------- | ||
| // 8. profile/search.php — $_REQUEST['selgroups'] (array) | ||
| // --------------------------------------------------------------- | ||
|
|
||
| /** | ||
| * Verify getArray returns array for selgroups parameter. | ||
| */ | ||
| #[Test] | ||
| public function getArrayReturnsArrayForSelgroups(): void | ||
| { | ||
| $_REQUEST['selgroups'] = ['1', '2', '3']; | ||
| $value = Request::getArray('selgroups', []); | ||
| $this->assertIsArray($value); | ||
| $this->assertCount(3, $value); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getArray returns empty default for missing selgroups. | ||
| */ | ||
| #[Test] | ||
| public function getArrayReturnsEmptyDefaultForMissing(): void | ||
| { | ||
| unset($_REQUEST['selgroups']); | ||
| $value = Request::getArray('selgroups', []); | ||
| $this->assertSame([], $value); | ||
| } |
There was a problem hiding this comment.
Multiple tests (e.g. lines 126–151, 163–176, 186–211, 219–235, 244–261, 270–288, 414–432, 440–457, 465–483) call Request::getCmd()/getString()/getInt()/getArray() without an explicit hash, then populate $_REQUEST[...]. These tests validate that Request sanitizes values, but they do not validate hash-pinning — the core security property this PR introduces. Without testing that POST-parameterized calls don't read from GET and vice versa, the tests give false confidence about the migration's correctness for hash-specific calls.
| $op = Request::getWord('op', '', 'POST'); | ||
|
|
||
| if (isset($_POST['login'])) { | ||
| if (Request::hasVar('login', 'POST')) { | ||
| $clean_login = Request::getString('login', '', 'POST'); | ||
| } | ||
|
|
||
| if (isset($_POST['pass'])) { | ||
| $clean_pass = Request::getString('pass', '', 'POST'); | ||
| if (Request::hasVar('pass', 'POST')) { | ||
| $clean_pass = Request::getText('pass', '', 'POST'); |
There was a problem hiding this comment.
At line 390, Request::getText() is used (with no hash argument) to read 'pass' for validation in the old banners.php. However, on line 383, Request::getWord() is used instead of Request::getCmd() to read op. getWord allows word characters (\w), which includes underscores, but is less restrictive than getCmd and may not match the intent. More critically, both getWord() and the getText() on line 390 are called without an explicit hash argument, falling back to $_REQUEST.
| if (!is_object($obj)) { | ||
| redirect_header('step.php', 1, _PROFILE_AM_SAVESTEP_TOGGLE_FAILED); | ||
| } |
There was a problem hiding this comment.
In profile_stepsave_toggle() at line 135, redirect_header() is called when $obj is not an object, but there is no return or exit after it. PHP execution will continue to line 137 ($obj->setVar(...)) despite $obj being non-object, causing a fatal error. redirect_header() does not halt execution.
| if (Request::getText('password', '', 'POST') !== '') { | ||
| $password = trim(Request::getText('password', '', 'POST')); | ||
| $vpass = trim(Request::getText('vpass', '', 'POST')); |
There was a problem hiding this comment.
In modules/profile/admin/user.php on lines 130-132, password fields are read using Request::getText() (which allows all characters including HTML/script tags) rather than Request::getVar(..., 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM). The rest of the PR consistently uses Request::getVar with MASK_ALLOW_RAW | MASK_NO_TRIM for password fields (see register.php, changepass.php, edituser.php). Using getText() for passwords may strip or alter special characters in passwords.
| mb_strtolower(Request::getString('password'), 'UTF-8') === mb_strtolower(Request::getString('uname'), 'UTF-8')) { | ||
| if ('' !== Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) && | ||
| '' !== Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) && | ||
| mb_strtolower(Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM), 'UTF-8') === mb_strtolower(Request::getString('uname'), 'UTF-8')) { |
There was a problem hiding this comment.
In htdocs/modules/system/admin/users/main.php on line 193, Request::getString('uname') is called without an explicit hash (no third argument). Per the codebase convention established in this PR and repository memories, all Xmf\Request calls must include an explicit hash ('GET', 'POST', or 'COOKIE'). Omitting the hash defaults to $_REQUEST (GET+POST+COOKIE combined), which is the antipattern being fixed across the whole codebase in this PR.
| $this->assertSame('results', Request::getCmd('op', 'search')); | ||
| } | ||
|
|
||
| /** | ||
| * Verify getCmd defaults to 'search' when op is missing. | ||
| */ | ||
| #[Test] | ||
| public function getCmdDefaultsToSearchForMissingOp(): void | ||
| { | ||
| unset($_REQUEST['op']); | ||
| $this->assertSame('search', Request::getCmd('op', 'search')); | ||
| } |
There was a problem hiding this comment.
In the test at line 127, Request::getCmd('op', 'search') is called without an explicit hash (no third argument 'GET', 'POST', or 'COOKIE'). The test at line 137 has the same issue. These tests call getCmd against $_REQUEST, which means they test the combined superglobal rather than a specific hash, defeating the purpose of verifying that GET/POST separation works. Also, line 71 (Request::getString('dir', '')) similarly omits the hash. These tests do not match the migration patterns validated in the later tests (sections 11+), which correctly use explicit hashes.
| if ($open === false) { | ||
| redirect_header('admin.php?fct=tplsets', 2, _AM_SYSTEM_TEMPLATES_ERROR); | ||
| } |
There was a problem hiding this comment.
In the tpls_restore case, after redirect_header() is called on line 388 when $open === false, there is no return or exit. PHP continues to the fwrite() call on an invalid (false) file handle, causing a warning. redirect_header() does not halt PHP execution.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| 'id' => Request::getInt('id', 0, 'GET'), | ||
| 'op' => 'delete', |
There was a problem hiding this comment.
In field.php, the delete case's xoops_confirm() call uses Request::getInt('id', 0, 'GET') for the 'id' field in the confirmation data. When the delete case is reached via POST (after the confirm form is submitted), $_GET['id'] will be absent and this will always produce id=0 in the confirm form, breaking the delete workflow for any second-confirmation flow. It should use $obj->getVar('field_id') to reliably carry the correct ID into the confirm form, consistent with the fix applied in step.php (line 101) and user.php.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
htdocs/modules/system/admin/groups/main.php (1)
30-30: 🧹 Nitpick | 🔵 TrivialConsider specifying request source for
opparameter.Currently
Request::getString('op', 'list')checks both GET and POST (via$_REQUEST). Ifopshould only be accepted from specific sources for security hardening, consider adding an explicit scope:$op = Request::getString('op', 'list', 'GET');However, if accepting from both sources is intentional (e.g., supporting both navigation links and form actions), the current implementation is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/groups/main.php` at line 30, The request handling for the operation parameter currently uses Request::getString('op', 'list') which reads from both GET and POST; if you want to restrict input to a specific source for hardening, change the call to explicitly pass the source (e.g., use Request::getString('op', 'list', 'GET')) so only GET is accepted, otherwise leave as-is if accepting both GET and POST is intentional; update the invocation that assigns $op and ensure any related form/link behavior still works with the chosen source.htdocs/modules/system/admin/tplsets/main.php (1)
372-377:⚠️ Potential issue | 🟠 MajorReject out-of-root
path_filevalues before copying or writing.Line 374 concatenates the path without a forward slash separator and calls
realpath()but never validates that the resolved path remains withinXOOPS_ROOT_PATH . '/themes'. A crafted relative path like/../../../etc/passwd.cssbypasses the boundary and gets copied/written on lines 383 and 386 as long as the extension matches. Additionally,realpath()can returnfalseon invalid paths, andpathinfo()on line 376 is called without guarding against this.🛡️ Proposed fix
$clean_path_file = Request::getString('path_file', '', 'POST'); if (!empty($clean_path_file)) { - $path_file = realpath(XOOPS_ROOT_PATH.'/themes'.trim($clean_path_file)); - $path_file = str_replace('\\','/',$path_file); - $pathInfo = pathinfo($path_file); - if (!in_array($pathInfo['extension'], ['css', 'html', 'tpl'])) { + $themes_root = realpath(XOOPS_ROOT_PATH . '/themes'); + $path_file = false === $themes_root ? false : realpath($themes_root . '/' . ltrim(trim($clean_path_file), '/\\')); + if (false === $path_file) { + redirect_header('admin.php?fct=tplsets', 2, _AM_SYSTEM_TEMPLATES_ERROR); + exit; + } + $themes_root = str_replace('\\', '/', $themes_root); + $path_file = str_replace('\\', '/', $path_file); + if (0 !== strncmp($path_file, $themes_root . '/', strlen($themes_root) + 1)) { + redirect_header('admin.php?fct=tplsets', 2, _AM_SYSTEM_TEMPLATES_ERROR); + exit; + } + $pathInfo = pathinfo($path_file); + if (!isset($pathInfo['extension']) || !in_array($pathInfo['extension'], ['css', 'html', 'tpl'], true)) { redirect_header('admin.php?fct=tplsets', 2, _AM_SYSTEM_TEMPLATES_ERROR); exit; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/tplsets/main.php` around lines 372 - 377, The code reads Request::getString('path_file') into $clean_path_file and builds $path_file with realpath(XOOPS_ROOT_PATH.'/themes'.trim($clean_path_file)) but does not validate realpath result or ensure the resolved path is inside XOOPS_ROOT_PATH.'/themes', nor guard pathinfo against false; update the logic in the block using Request::getString, realpath and pathinfo to (1) ensure you prepend a directory separator between XOOPS_ROOT_PATH.'/themes' and the user input, (2) call realpath and reject if it returns false, (3) verify the resolved $path_file begins with the canonical XOOPS_ROOT_PATH.'/themes' prefix (use the same realpath on that prefix) to prevent directory traversal, (4) only then call pathinfo and the existing in_array extension check before performing the downstream copy/write operations that currently use $path_file.
♻️ Duplicate comments (10)
htdocs/modules/system/admin/users/main.php (4)
306-307:⚠️ Potential issue | 🔴 CriticalUser can be created without a password hash due to
pass2gating.Line 263 validates that
passwordis required, but Lines 306-307 only set the password hash whenpass2is non-empty. This creates a gap: submittingpassword="test"withpass2=""passes validation but results in a user being inserted without a password hash.🐛 Proposed fix
- if ('' !== Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM)) { - $newuser->setVar('pass', password_hash(Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM), PASSWORD_DEFAULT)); - } + // Password is required for new users (validated at line 263), always set it + $password = Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); + $newuser->setVar('pass', password_hash($password, PASSWORD_DEFAULT));Additionally, the mismatch validation at Lines 278-279 should run unconditionally when creating a user to ensure both fields are provided and match.
🤖 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 306 - 307, The create-user flow currently only sets the password hash when Request::getVar('pass2', ...) is non-empty, allowing a user to be created with an empty password hash; change the logic so that password hashing via $newuser->setVar('pass', password_hash(..., PASSWORD_DEFAULT)) always runs once validation succeeds (use the password value from Request::getVar('password', ...)), and move/ensure the password mismatch check that compares Request::getVar('password', ...) and Request::getVar('pass2', ...) runs unconditionally during user creation so both fields are required and must match before inserting the user; update code around the Request::getVar(...) checks and $newuser->setVar('pass', ...) to reflect this.
185-196: 🧹 Nitpick | 🔵 TrivialCache password values to avoid repeated Request::getVar calls.
The same
Request::getVar('password', ...)andRequest::getVar('pass2', ...)calls are invoked multiple times with identical parameters. This is both inefficient and creates maintenance risk if parameters need updating.♻️ Proposed refactor
+ $password = Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); + $pass2 = Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); + - if ('' !== Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) && - Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) != Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM)) { + if ($pass2 !== '' && $password !== $pass2) { xoops_error(_AM_SYSTEM_USERS_STNPDNM); break; } - if ('' !== Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) && - '' !== Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) && - mb_strtolower(Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM), 'UTF-8') === mb_strtolower(Request::getString('uname'), 'UTF-8')) { + if ($pass2 !== '' && $password !== '' && + mb_strtolower($password, 'UTF-8') === mb_strtolower(Request::getString('uname'), 'UTF-8')) { xoops_error(_AM_SYSTEM_USERS_PWDEQUALSUNAME); break; }This also enables stricter
!==comparison and improves readability.🤖 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 185 - 196, Cache Request::getVar('password', ...) and Request::getVar('pass2', ...) into local variables at the start of the validation block and use those variables in the comparisons instead of calling Request::getVar repeatedly; update the two conditional checks that reference Request::getVar('password', ...) and Request::getVar('pass2', ...) to use the cached $password and $pass2 variables, keep the strict !== comparisons, and for the username-equals-password check use mb_strtolower($password, 'UTF-8') compared to mb_strtolower(Request::getString('uname'), 'UTF-8') to preserve the existing behavior.
263-263:⚠️ Potential issue | 🟡 MinorValidation includes
passwordbut notpass2for new user.The required-field validation checks for
passwordbeing non-empty, but doesn't requirepass2. This inconsistency allows submission without password confirmation.🛡️ Proposed fix
- if (!Request::getString('uname') || !Request::getString('email') || !Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM)) { + $password = Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); + $pass2 = Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); + if (!Request::getString('uname') || !Request::getString('email') || $password === '' || $pass2 === '') { xoops_error(_AM_SYSTEM_USERS_YMCACF); break; }🤖 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 263, The form validation currently requires Request::getVar('password', ...) but omits the confirmation field 'pass2', allowing submissions without a password confirmation; update the required-field check (the if that calls Request::getString('uname'), Request::getString('email'), Request::getVar('password', ...)) to also require Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) and then ensure later code compares the two fields (password vs pass2) and returns the same validation error path if they differ.
224-226: 🧹 Nitpick | 🔵 TrivialConsider gating password update on
passwordfield, notpass2.Currently, the password is only updated when
pass2is non-empty. If a user fills inpasswordbut leavespass2blank, the password update is silently skipped. For edit flows this may be acceptable (empty fields = no change), but the gate should logically be on the primarypasswordfield.♻️ Proposed refactor (assuming $password variable from earlier)
- if ('' !== Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM)) { - $edituser->setVar('pass', password_hash(Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM), PASSWORD_DEFAULT)); + if ($password !== '') { + $edituser->setVar('pass', password_hash($password, PASSWORD_DEFAULT)); }🤖 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 224 - 226, The password update is gated on the secondary field 'pass2' so if 'password' is provided and 'pass2' is blank the change is skipped; change the condition to check the primary 'password' Request::getVar value (or the previously extracted $password variable if present) instead of 'pass2', and only then call $edituser->setVar('pass', password_hash(...)) using Request::getVar('password', ...) so edits occur when the primary password field is filled; keep the same Request masks and PASSWORD_DEFAULT hashing.htdocs/modules/system/admin/tplsets/main.php (1)
147-147:⚠️ Potential issue | 🟡 MinorDon't use
?:to mergeactive_templateshere.Line 147 still treats an explicit GET value of
0as “not set”, so it falls through to POST and can take the wrong branch when both sources are present. Pick the source withRequest::hasVar()first, or just read POST since this flow submits via POST.🔧 Proposed fix
- if ((Request::getInt('active_templates', 0, 'GET') ?: Request::getInt('active_templates', 0, 'POST')) === 0) { + $active_templates = Request::hasVar('active_templates', 'GET') + ? Request::getInt('active_templates', 0, 'GET') + : Request::getInt('active_templates', 0, 'POST'); + if ($active_templates === 0) {🔍 Verification script
Verify whether this admin flow actually posts
active_templates, which would make the GET fallback unnecessary.#!/bin/bash rg -n -C2 "active_templates|XoopsThemeForm" htdocs/modules/system/admin/tplsets/main.phpExpected: the forms around Lines 65-91 and 345-362 submit via
postand carryactive_templatesin hidden fields, while Line 147 still merges GET/POST with?:.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/tplsets/main.php` at line 147, Replace the `?:` merge that uses Request::getInt('active_templates', 0, 'GET') ?: Request::getInt('active_templates', 0, 'POST') so an explicit GET value of 0 isn't treated as "unset"; instead, prefer POST or explicitly check existence with Request::hasVar('active_templates', 'POST') first and use Request::getInt('active_templates', 0, 'POST') when present, otherwise fall back to Request::getInt('active_templates', 0, 'GET') (or simply always read POST if the form submits via POST).htdocs/modules/profile/register.php (1)
30-32:⚠️ Potential issue | 🟡 MinorFinish the migration in the activation redirect.
This branch still appends raw
$_SERVER['QUERY_STRING']to theLocationheader, so request data bypasses the XMF wrapper here.Suggested fix
$regOp = Request::getString('op', '', 'GET'); -if ($regOp !== '' && in_array($regOp, ['actv', 'activate'])) { - header('location: ./activate.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); +if ($regOp !== '' && in_array($regOp, ['actv', 'activate'], true)) { + $queryString = Request::getString('QUERY_STRING', '', 'SERVER'); + header('location: ./activate.php' . ($queryString === '' ? '' : '?' . $queryString)); exit(); }Run this to confirm the branch still reads directly from
$_SERVERand thatRequestsupports theSERVERhash.#!/bin/bash set -euo pipefail sed -n '30,33p' htdocs/modules/profile/register.php printf '\n---\n' request_file="$(fd '^Request\.php$' htdocs | grep '/vendor/xoops/xmf/src/Request.php$' | head -n1)" sed -n '80,127p' "$request_file"Based on learnings: "Use Xmf\Request, Xmf\Module\Helper, and other XMF utilities from htdocs/class/libraries/ instead of direct superglobal or legacy function access".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/register.php` around lines 30 - 32, The redirect uses raw $_SERVER['QUERY_STRING'] which bypasses XMF's Request wrapper; replace that direct superglobal access by retrieving the query string via Request (e.g. Request::getString('QUERY_STRING', '', 'SERVER')) and append it to the location only when non-empty so the activation branch uses XMF's sanitized SERVER value instead of $_SERVER['QUERY_STRING']; update the code around $regOp handling and the header('location: ...') call to use that Request-based value.htdocs/modules/profile/admin/user.php (2)
147-153: 🧹 Nitpick | 🔵 TrivialRemove the dead second argument from
getValueForSave().
htdocs/modules/profile/class/field.phpdefinesgetValueForSave($value), so the existing-value argument in both branches no longer participates in the save path. Keeping it here just makes this admin flow diverge from the single-argument call pattern already used inhtdocs/modules/profile/edituser.php.♻️ Minimal cleanup
- $value = $fields[$i]->getValueForSave($fieldValue, $user->getVar($fieldname, 'n')); + $value = $fields[$i]->getValueForSave($fieldValue); $user->setVar($fieldname, $value); } else { - $value = $fields[$i]->getValueForSave($fieldValue, $profile->getVar($fieldname, 'n')); + $value = $fields[$i]->getValueForSave($fieldValue); $profile->setVar($fieldname, $value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/user.php` around lines 147 - 153, The call sites in the admin save flow pass a now-unused second argument to Field::getValueForSave; update both branches so $fields[$i]->getValueForSave(...) is called with a single argument (remove the existing-value parameter), keeping the rest of the logic intact: locate the two calls on the branches that currently pass $user->getVar($fieldname, 'n') and $profile->getVar($fieldname, 'n') and remove those second parameters so the method signature matches the single-argument definition used elsewhere (e.g., edituser.php) while preserving Request::getVar($fieldname, null, 'POST'), in_array checks, and the subsequent $user->setVar($fieldname, $value) behavior.
240-240:⚠️ Potential issue | 🟡 MinorFinish the Request migration for the confirmation action.
Line 240 still passes
$_SERVER['REQUEST_URI']straight intoxoops_confirm(), so this flow keeps a raw superglobal dependency inside a Request-migration PR. Pull the current URI through the XMF request layer before passing it on. Based on learnings: "Use Xmf\Request, Xmf\Module\Helper, and other XMF utilities from htdocs/class/libraries/ instead of direct superglobal or legacy function access."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/user.php` at line 240, Replace the direct superglobal use of $_SERVER['REQUEST_URI'] passed into xoops_confirm() by retrieving the current URI via the XMF request helper: call Xmf\Request to get the REQUEST_URI (e.g. Request::getString('REQUEST_URI', '') or the appropriate Request:: method) and pass that variable into xoops_confirm() instead of $_SERVER['REQUEST_URI']; update the xoops_confirm call site in the confirmation action (the code invoking xoops_confirm) to use the new $currentUri value.htdocs/modules/profile/admin/step.php (2)
104-104:⚠️ Potential issue | 🟡 MinorFinish the Request migration for the delete confirmation action.
Line 104 still feeds
$_SERVER['REQUEST_URI']intoxoops_confirm(), which leaves a raw superglobal read in the middle of this migration. Pull the current URI through the XMF request layer first. Based on learnings: "Use Xmf\Request, Xmf\Module\Helper, and other XMF utilities from htdocs/class/libraries/ instead of direct superglobal or legacy function access."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/step.php` at line 104, Replace the direct superglobal usage ($_SERVER['REQUEST_URI']) passed into xoops_confirm() with the XMF request helper: obtain the current URI via Xmf\Request (e.g. Request::getString('REQUEST_URI', '', 'SERVER') or \Xmf\Request::getString(...)) and pass that value into xoops_confirm(); ensure you import or fully qualify Xmf\Request so no raw $_SERVER access remains.
110-115:⚠️ Potential issue | 🔴 CriticalKeep the toggle write path off GET.
This branch still mutates persisted state from query parameters, and the UI template
htdocs/modules/profile/templates/profile_admin_steplist.tplinvokes it with a plain link. That reopens the toggle to CSRF and crawler/prefetch side effects; this needs a tokenized POST flow before callingprofile_stepsave_toggle().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/step.php` around lines 110 - 115, The 'toggle' case currently mutates state via GET parameters (Request::hasVar/Request::getInt) and calls profile_stepsave_toggle(step_id, step_save) directly; change this to require a tokenized POST flow: only accept the toggle when Request::getMethod() is POST (or Request::hasVar('step_save', 'POST')), validate the CSRF token used by your app (e.g., checkToken/validateToken function) and then read step_id/step_save from POST before calling profile_stepsave_toggle; also update the template profile_admin_steplist.tpl to render a form/button that submits POST with the token instead of a plain link.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/edituser.php`:
- Around line 56-61: The password inputs retrieved with
Request::getVar('password', ...) and Request::getVar('vpass', ...) can be arrays
due to MASK_ALLOW_RAW and must be normalized before using strlen() or
password_hash(); modify the edituser.php logic around the $password and $vpass
handling to explicitly validate and coerce them to strings (or reject
non-string/array inputs) — e.g. check is_array($password) and is_array($vpass)
and handle that case (return an error) or cast/normalize to a string, and ensure
the same normalization occurs before calling strlen($password) and before any
password_hash() usage so TypeError is avoided.
In `@htdocs/modules/profile/admin/step.php`:
- Around line 129-137: The function profile_stepsave_toggle currently flips the
client-provided $stepSave instead of the stored value; change its signature to
profile_stepsave_toggle(int $stepId): void (remove the redundant $stepSave
param), load the regstep object via xoops_getModuleHandler('regstep') and derive
the new value from $obj->getVar('step_save') (e.g. toggle 1↔0), call
$obj->setVar('step_save', $newValue') and persist the change, and update all
call sites to pass only the stepId.
In `@htdocs/modules/profile/changepass.php`:
- Line 33: Replace the direct use of the superglobal $_SERVER['REQUEST_URI']
when constructing the XoopsThemeForm action with a filtered value: retrieve the
raw request URI and pass it through Xmf\Request::getString(...) or
Xmf\FilterInput::clean(...) (or equivalent project-safe sanitizer) before
supplying it to the XoopsThemeForm constructor in changepass.php; update the
line that calls new XoopsThemeForm(_PROFILE_MA_CHANGEPASSWORD, 'form',
$_SERVER['REQUEST_URI'], 'post', true) to use the sanitized variable so the form
action cannot contain unescaped user-controlled input.
In `@htdocs/modules/profile/register.php`:
- Around line 184-185: The raw POST fetches for $pass and $vpass use
Request::MASK_ALLOW_RAW and can yield arrays, which breaks password_hash() and
the mailer template; after obtaining $pass and $vpass from Request::getVar,
ensure each is a string by replacing non-strings with an empty string (e.g., if
(!is_string($pass)) $pass = ''; if (!is_string($vpass)) $vpass = '';) and apply
the same is_string() guard immediately before each use of those variables in
password_hash(...) and before passing them into the mailer/template rendering so
only scalar strings are ever supplied.
- Around line 171-174: The current required-field check uses
empty(Request::getVar(...)) which treats the valid string '0' as empty; update
the validation for fields in the loop that references Request::hasVar and
Request::getVar on $_SESSION['profile_required'] so it uses an explicit
empty-string check (e.g. Request::getVar($name, '', 'POST') === '' or
strlen(Request::getVar($name, '', 'POST')) === 0) instead of empty(), thereby
allowing the scalar '0' value accepted by ProfileField::getValueForSave() while
still rejecting truly empty submissions.
In `@htdocs/modules/system/admin/users/main.php`:
- Around line 278-289: Validation is bypassed when pass2 is empty; ensure both
Request::getVar('password', ...) and Request::getVar('pass2', ...) are required
for new-user flows by adding an explicit non-empty check before the existing
equality and username-equals-password checks: if either password or pass2 is
empty, call xoops_error with an appropriate message and stop processing,
otherwise proceed to compare password vs pass2 (use the existing
_AM_SYSTEM_USERS_STNPDNM error) and check password vs uname (use
_AM_SYSTEM_USERS_PWDEQUALSUNAME); update the logic around the
Request::getVar('pass2', ...), Request::getVar('password', ...), xoops_error and
related break statements so the equality/name checks always run only when both
fields are present.
In `@htdocs/register.php`:
- Around line 67-68: After reading $pass and $vpass via Request::getVar (with
MASK_ALLOW_RAW) ensure they are scalar strings before any string operations:
check is_scalar($pass) && is_scalar($vpass) (or is_string) and if either is not
scalar, treat it as invalid — e.g. set to empty string and/or return a
validation error to the user — so downstream code (validation, hashing,
escaping) never receives an array/object; do this immediately after the
Request::getVar calls that populate $pass and $vpass.
---
Outside diff comments:
In `@htdocs/modules/system/admin/groups/main.php`:
- Line 30: The request handling for the operation parameter currently uses
Request::getString('op', 'list') which reads from both GET and POST; if you want
to restrict input to a specific source for hardening, change the call to
explicitly pass the source (e.g., use Request::getString('op', 'list', 'GET'))
so only GET is accepted, otherwise leave as-is if accepting both GET and POST is
intentional; update the invocation that assigns $op and ensure any related
form/link behavior still works with the chosen source.
In `@htdocs/modules/system/admin/tplsets/main.php`:
- Around line 372-377: The code reads Request::getString('path_file') into
$clean_path_file and builds $path_file with
realpath(XOOPS_ROOT_PATH.'/themes'.trim($clean_path_file)) but does not validate
realpath result or ensure the resolved path is inside XOOPS_ROOT_PATH.'/themes',
nor guard pathinfo against false; update the logic in the block using
Request::getString, realpath and pathinfo to (1) ensure you prepend a directory
separator between XOOPS_ROOT_PATH.'/themes' and the user input, (2) call
realpath and reject if it returns false, (3) verify the resolved $path_file
begins with the canonical XOOPS_ROOT_PATH.'/themes' prefix (use the same
realpath on that prefix) to prevent directory traversal, (4) only then call
pathinfo and the existing in_array extension check before performing the
downstream copy/write operations that currently use $path_file.
---
Duplicate comments:
In `@htdocs/modules/profile/admin/step.php`:
- Line 104: Replace the direct superglobal usage ($_SERVER['REQUEST_URI'])
passed into xoops_confirm() with the XMF request helper: obtain the current URI
via Xmf\Request (e.g. Request::getString('REQUEST_URI', '', 'SERVER') or
\Xmf\Request::getString(...)) and pass that value into xoops_confirm(); ensure
you import or fully qualify Xmf\Request so no raw $_SERVER access remains.
- Around line 110-115: The 'toggle' case currently mutates state via GET
parameters (Request::hasVar/Request::getInt) and calls
profile_stepsave_toggle(step_id, step_save) directly; change this to require a
tokenized POST flow: only accept the toggle when Request::getMethod() is POST
(or Request::hasVar('step_save', 'POST')), validate the CSRF token used by your
app (e.g., checkToken/validateToken function) and then read step_id/step_save
from POST before calling profile_stepsave_toggle; also update the template
profile_admin_steplist.tpl to render a form/button that submits POST with the
token instead of a plain link.
In `@htdocs/modules/profile/admin/user.php`:
- Around line 147-153: The call sites in the admin save flow pass a now-unused
second argument to Field::getValueForSave; update both branches so
$fields[$i]->getValueForSave(...) is called with a single argument (remove the
existing-value parameter), keeping the rest of the logic intact: locate the two
calls on the branches that currently pass $user->getVar($fieldname, 'n') and
$profile->getVar($fieldname, 'n') and remove those second parameters so the
method signature matches the single-argument definition used elsewhere (e.g.,
edituser.php) while preserving Request::getVar($fieldname, null, 'POST'),
in_array checks, and the subsequent $user->setVar($fieldname, $value) behavior.
- Line 240: Replace the direct superglobal use of $_SERVER['REQUEST_URI'] passed
into xoops_confirm() by retrieving the current URI via the XMF request helper:
call Xmf\Request to get the REQUEST_URI (e.g. Request::getString('REQUEST_URI',
'') or the appropriate Request:: method) and pass that variable into
xoops_confirm() instead of $_SERVER['REQUEST_URI']; update the xoops_confirm
call site in the confirmation action (the code invoking xoops_confirm) to use
the new $currentUri value.
In `@htdocs/modules/profile/register.php`:
- Around line 30-32: The redirect uses raw $_SERVER['QUERY_STRING'] which
bypasses XMF's Request wrapper; replace that direct superglobal access by
retrieving the query string via Request (e.g. Request::getString('QUERY_STRING',
'', 'SERVER')) and append it to the location only when non-empty so the
activation branch uses XMF's sanitized SERVER value instead of
$_SERVER['QUERY_STRING']; update the code around $regOp handling and the
header('location: ...') call to use that Request-based value.
In `@htdocs/modules/system/admin/tplsets/main.php`:
- Line 147: Replace the `?:` merge that uses Request::getInt('active_templates',
0, 'GET') ?: Request::getInt('active_templates', 0, 'POST') so an explicit GET
value of 0 isn't treated as "unset"; instead, prefer POST or explicitly check
existence with Request::hasVar('active_templates', 'POST') first and use
Request::getInt('active_templates', 0, 'POST') when present, otherwise fall back
to Request::getInt('active_templates', 0, 'GET') (or simply always read POST if
the form submits via POST).
In `@htdocs/modules/system/admin/users/main.php`:
- Around line 306-307: The create-user flow currently only sets the password
hash when Request::getVar('pass2', ...) is non-empty, allowing a user to be
created with an empty password hash; change the logic so that password hashing
via $newuser->setVar('pass', password_hash(..., PASSWORD_DEFAULT)) always runs
once validation succeeds (use the password value from
Request::getVar('password', ...)), and move/ensure the password mismatch check
that compares Request::getVar('password', ...) and Request::getVar('pass2', ...)
runs unconditionally during user creation so both fields are required and must
match before inserting the user; update code around the Request::getVar(...)
checks and $newuser->setVar('pass', ...) to reflect this.
- Around line 185-196: Cache Request::getVar('password', ...) and
Request::getVar('pass2', ...) into local variables at the start of the
validation block and use those variables in the comparisons instead of calling
Request::getVar repeatedly; update the two conditional checks that reference
Request::getVar('password', ...) and Request::getVar('pass2', ...) to use the
cached $password and $pass2 variables, keep the strict !== comparisons, and for
the username-equals-password check use mb_strtolower($password, 'UTF-8')
compared to mb_strtolower(Request::getString('uname'), 'UTF-8') to preserve the
existing behavior.
- Line 263: The form validation currently requires Request::getVar('password',
...) but omits the confirmation field 'pass2', allowing submissions without a
password confirmation; update the required-field check (the if that calls
Request::getString('uname'), Request::getString('email'),
Request::getVar('password', ...)) to also require Request::getVar('pass2', '',
'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) and then
ensure later code compares the two fields (password vs pass2) and returns the
same validation error path if they differ.
- Around line 224-226: The password update is gated on the secondary field
'pass2' so if 'password' is provided and 'pass2' is blank the change is skipped;
change the condition to check the primary 'password' Request::getVar value (or
the previously extracted $password variable if present) instead of 'pass2', and
only then call $edituser->setVar('pass', password_hash(...)) using
Request::getVar('password', ...) so edits occur when the primary password field
is filled; keep the same Request masks and PASSWORD_DEFAULT hashing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4074e32-8566-4561-9039-719f81d0e391
📒 Files selected for processing (11)
htdocs/edituser.phphtdocs/install/page_siteinit.phphtdocs/modules/profile/admin/step.phphtdocs/modules/profile/admin/user.phphtdocs/modules/profile/changemail.phphtdocs/modules/profile/changepass.phphtdocs/modules/profile/register.phphtdocs/modules/system/admin/groups/main.phphtdocs/modules/system/admin/tplsets/main.phphtdocs/modules/system/admin/users/main.phphtdocs/register.php
| $password = Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); | ||
| if (!empty($password)) { | ||
| if (strlen($password) < $xoopsConfigUser['minpass']) { | ||
| $errors[] = sprintf(_US_PWDTOOSHORT, $xoopsConfigUser['minpass']); | ||
| } else { | ||
| $vpass = XoopsRequest::getString('vpass', ''); | ||
| $vpass = Request::getVar('vpass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
request_file="$(fd '^Request\.php$' htdocs | grep '/vendor/xoops/xmf/src/Request.php$' | head -n1)"
echo "Inspecting: ${request_file}"
sed -n '80,127p' "$request_file"
printf '\n---\n'
sed -n '537,572p' "$request_file"Repository: XOOPS/XoopsCore25
Length of output: 2925
🏁 Script executed:
cat -n htdocs/edituser.php | sed -n '50,70p'Repository: XOOPS/XoopsCore25
Length of output: 1198
🏁 Script executed:
# Also check if there's any type-checking or normalization after these lines
cat -n htdocs/edituser.php | sed -n '50,120p'Repository: XOOPS/XoopsCore25
Length of output: 4346
Normalize raw password inputs before calling strlen() or hashing.
With MASK_ALLOW_RAW, Request::getVar() bypasses type coercion entirely. An attacker can send password[]=value to inject an array into $password, which will then pass the !empty() check and cause strlen() on line 58 and password_hash() on line 86 to throw a TypeError in PHP 8.2+.
Suggested fix
$password = Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM);
+ $password = is_string($password) ? $password : '';
if (!empty($password)) {
if (strlen($password) < $xoopsConfigUser['minpass']) {
$errors[] = sprintf(_US_PWDTOOSHORT, $xoopsConfigUser['minpass']);
} else {
$vpass = Request::getVar('vpass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM);
+ $vpass = is_string($vpass) ? $vpass : '';
if ($password != $vpass) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $password = Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); | |
| if (!empty($password)) { | |
| if (strlen($password) < $xoopsConfigUser['minpass']) { | |
| $errors[] = sprintf(_US_PWDTOOSHORT, $xoopsConfigUser['minpass']); | |
| } else { | |
| $vpass = XoopsRequest::getString('vpass', ''); | |
| $vpass = Request::getVar('vpass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); | |
| $password = Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); | |
| $password = is_string($password) ? $password : ''; | |
| if (!empty($password)) { | |
| if (strlen($password) < $xoopsConfigUser['minpass']) { | |
| $errors[] = sprintf(_US_PWDTOOSHORT, $xoopsConfigUser['minpass']); | |
| } else { | |
| $vpass = Request::getVar('vpass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); | |
| $vpass = is_string($vpass) ? $vpass : ''; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/edituser.php` around lines 56 - 61, The password inputs retrieved with
Request::getVar('password', ...) and Request::getVar('vpass', ...) can be arrays
due to MASK_ALLOW_RAW and must be normalized before using strlen() or
password_hash(); modify the edituser.php logic around the $password and $vpass
handling to explicitly validate and coerce them to strings (or reject
non-string/array inputs) — e.g. check is_array($password) and is_array($vpass)
and handle that case (return an error) or cast/normalize to a string, and ensure
the same normalization occurs before calling strlen($password) and before any
password_hash() usage so TypeError is avoided.
| if (!Request::hasVar('submit', 'POST')) { | ||
| //show change password form | ||
| include_once $GLOBALS['xoops']->path('class/xoopsformloader.php'); | ||
| $form = new XoopsThemeForm(_PROFILE_MA_CHANGEPASSWORD, 'form', $_SERVER['REQUEST_URI'], 'post', true); |
There was a problem hiding this comment.
Direct $_SERVER['REQUEST_URI'] access remains.
While the PR migrates $_POST access to Xmf\Request, line 33 still directly accesses $_SERVER['REQUEST_URI']. This superglobal is user-controllable and can contain malicious payloads if not escaped in the form's action attribute.
For consistency with the migration goals and to mitigate potential XSS vectors in the form action, consider:
- $form = new XoopsThemeForm(_PROFILE_MA_CHANGEPASSWORD, 'form', $_SERVER['REQUEST_URI'], 'post', true);
+ $form = new XoopsThemeForm(_PROFILE_MA_CHANGEPASSWORD, 'form', Request::getString('REQUEST_URI', '', 'SERVER'), 'post', true);Alternatively, if XoopsThemeForm internally escapes the action URL, this may be acceptable—but explicit filtering is more defensive.
As per coding guidelines: "All user input must be filtered using Xmf\Request::getVar() or Xmf\FilterInput::clean() — never access $_GET, $_POST, or $_REQUEST directly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/changepass.php` at line 33, Replace the direct use of
the superglobal $_SERVER['REQUEST_URI'] when constructing the XoopsThemeForm
action with a filtered value: retrieve the raw request URI and pass it through
Xmf\Request::getString(...) or Xmf\FilterInput::clean(...) (or equivalent
project-safe sanitizer) before supplying it to the XoopsThemeForm constructor in
changepass.php; update the line that calls new
XoopsThemeForm(_PROFILE_MA_CHANGEPASSWORD, 'form', $_SERVER['REQUEST_URI'],
'post', true) to use the sanitized variable so the form action cannot contain
unescaped user-controlled input.
| if (Request::hasVar('step', 'POST') && isset($_SESSION['profile_required'])) { | ||
| foreach ($_SESSION['profile_required'] as $name => $title) { | ||
| if (!isset($_POST[$name]) || empty($_POST[$name])) { | ||
| if (!Request::hasVar($name, 'POST') || empty(Request::getVar($name, '', 'POST'))) { | ||
| $stop .= sprintf(_FORM_ENTER, $title) . '<br>'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Official PHP manual: does empty('0') return true, and what is the recommended way to validate required form fields when 0 is a legitimate value?
💡 Result:
Yes. In PHP, empty('0') returns true (the string "0" is considered “empty”/falsey by empty()), and empty() is essentially equivalent to !isset($var) || $var == false. [1]
Recommended “required field” validation when 0 is a valid value
Don’t use empty() for required checks. Instead, check presence and then check for an empty string (optionally after trimming):
if (!array_key_exists('qty', $_POST) || trim((string)$_POST['qty']) === '') {
$errors['qty'] = 'Required';
}array_key_exists()(orisset()if you don’t need to acceptnull) ensures the field was submitted.- Comparing to
''(aftertrim) rejects truly blank input but accepts"0".
If the field must be an integer, validate after the required check:
$qty = filter_input(INPUT_POST, 'qty', FILTER_VALIDATE_INT);
if ($qty === null || $qty === false) {
$errors['qty'] = 'Must be an integer';
}Notes:
filter_input()returnsfalseif the variable is not set (andfalseon failure unlessFILTER_NULL_ON_FAILUREis used). [2]FILTER_VALIDATE_INTis the built-in integer validator. [3]
Sources: PHP manual for empty() [1], filter_input() [2], and filter constants/FILTER_VALIDATE_INT [3].
[1] https://www.php.net/manual/en/function.empty.php
[2] https://www.php.net/manual/en/function.filter-input.php
[3] https://www.php.net/manual/en/filter.constants.php
🏁 Script executed:
# First, check if the file exists and view the relevant lines
head -n 180 htdocs/modules/profile/register.php | tail -n 20Repository: XOOPS/XoopsCore25
Length of output: 619
🏁 Script executed:
# Also get more context around those lines to understand the validation flow
sed -n '165,180p' htdocs/modules/profile/register.phpRepository: XOOPS/XoopsCore25
Length of output: 481
🏁 Script executed:
# Search for ProfileField class to understand getValueForSave() behavior
fd -t f ProfileField.phpRepository: XOOPS/XoopsCore25
Length of output: 43
🏁 Script executed:
# Search for ProfileField class definition
rg -t php "class ProfileField" --max-count 5Repository: XOOPS/XoopsCore25
Length of output: 336
🏁 Script executed:
# Search for getValueForSave method to understand how zero values are processed
rg -t php "getValueForSave" --max-count 10 -A 3Repository: XOOPS/XoopsCore25
Length of output: 5666
🏁 Script executed:
# Check how the profile_required fields are populated and what types they are
rg -t php "profile_required" --max-count 10 -B 2 -A 2Repository: XOOPS/XoopsCore25
Length of output: 1552
🏁 Script executed:
# Get the full implementation of getValueForSave method
sed -n '/public function getValueForSave/,/^ public\|^}/p' htdocs/modules/profile/class/field.php | head -100Repository: XOOPS/XoopsCore25
Length of output: 1174
🏁 Script executed:
# Check how zero values are handled in radio/select/yesno field types
rg -t php "field_type.*radio|field_type.*select|field_type.*yesno" -A 10 -B 2Repository: XOOPS/XoopsCore25
Length of output: 4424
🏁 Script executed:
# Look at the test cases to understand expected zero-value behavior
sed -n '/getValueForSaveTextboxReturnsValue/,/^ public/p' tests/unit/htdocs/modules/profile/ProfileFieldTest.php | head -50Repository: XOOPS/XoopsCore25
Length of output: 369
🏁 Script executed:
# Check if checkbox fields can be in profile_required
rg -t php "field_required.*checkbox|checkbox.*field_required" -B 3 -A 3Repository: XOOPS/XoopsCore25
Length of output: 43
🏁 Script executed:
# Look at how edituser.php handles this - it might show the intended approach
sed -n '230,260p' htdocs/modules/profile/edituser.php | cat -nRepository: XOOPS/XoopsCore25
Length of output: 1920
🏁 Script executed:
# Check if there are any test cases validating zero values for yesno/radio/select
rg -t php "getValueForSave.*0|getValueForSave.*false" -B 2 -A 2Repository: XOOPS/XoopsCore25
Length of output: 1546
Use explicit presence checks instead of empty() for required form fields with zero-valued options.
The validation at lines 171-174 fails when a user legitimately selects '0' from a yesno, radio, or select field. Since empty('0') returns true in PHP, the check incorrectly rejects valid submissions. ProfileField::getValueForSave() accepts and preserves these scalar zero values unchanged for these field types, so the validation logic must handle them explicitly.
Suggested fix
if (Request::hasVar('step', 'POST') && isset($_SESSION['profile_required'])) {
foreach ($_SESSION['profile_required'] as $name => $title) {
- if (!Request::hasVar($name, 'POST') || empty(Request::getVar($name, '', 'POST'))) {
+ $value = Request::getVar($name, null, 'POST');
+ if (!Request::hasVar($name, 'POST') || $value === null || $value === '' || $value === []) {
$stop .= sprintf(_FORM_ENTER, $title) . '<br>';
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (Request::hasVar('step', 'POST') && isset($_SESSION['profile_required'])) { | |
| foreach ($_SESSION['profile_required'] as $name => $title) { | |
| if (!isset($_POST[$name]) || empty($_POST[$name])) { | |
| if (!Request::hasVar($name, 'POST') || empty(Request::getVar($name, '', 'POST'))) { | |
| $stop .= sprintf(_FORM_ENTER, $title) . '<br>'; | |
| if (Request::hasVar('step', 'POST') && isset($_SESSION['profile_required'])) { | |
| foreach ($_SESSION['profile_required'] as $name => $title) { | |
| $value = Request::getVar($name, null, 'POST'); | |
| if (!Request::hasVar($name, 'POST') || $value === null || $value === '' || $value === []) { | |
| $stop .= sprintf(_FORM_ENTER, $title) . '<br>'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/register.php` around lines 171 - 174, The current
required-field check uses empty(Request::getVar(...)) which treats the valid
string '0' as empty; update the validation for fields in the loop that
references Request::hasVar and Request::getVar on $_SESSION['profile_required']
so it uses an explicit empty-string check (e.g. Request::getVar($name, '',
'POST') === '' or strlen(Request::getVar($name, '', 'POST')) === 0) instead of
empty(), thereby allowing the scalar '0' value accepted by
ProfileField::getValueForSave() while still rejecting truly empty submissions.
| $pass = Request::getVar('pass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); | ||
| $vpass = Request::getVar('vpass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
request_file="$(fd '^Request\.php$' htdocs | grep '/vendor/xoops/xmf/src/Request.php$' | head -n1)"
echo "Inspecting: ${request_file}"
sed -n '80,127p' "$request_file"
printf '\n---\n'
sed -n '537,572p' "$request_file"Repository: XOOPS/XoopsCore25
Length of output: 2925
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the actual register.php file at the specified lines
echo "=== Lines 184-185 ==="
sed -n '180,190p' htdocs/modules/profile/register.php
echo -e "\n=== Lines 225 ==="
sed -n '220,230p' htdocs/modules/profile/register.php
echo -e "\n=== Lines 281 ==="
sed -n '276,286p' htdocs/modules/profile/register.php
# Also check how $pass and $vpass are used after retrieval
echo -e "\n=== Search for password_hash usage with \$pass ==="
rg -n 'password_hash.*\$pass' htdocs/modules/profile/register.php -A 2
echo -e "\n=== Search for mailer/template usage with \$vpass ==="
rg -n '\$vpass' htdocs/modules/profile/register.php -B 2 -A 2Repository: XOOPS/XoopsCore25
Length of output: 3576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Test password_hash behavior with array input
echo "=== Testing password_hash() with array input in PHP 8.2+ ==="
php <<'EOF'
try {
$result = password_hash([], PASSWORD_DEFAULT);
echo "Result: " . var_export($result, true) . "\n";
} catch (TypeError $e) {
echo "TypeError caught: " . $e->getMessage() . "\n";
}
EOF
# Find and inspect XoopsUserUtility::validate
echo -e "\n=== Searching for XoopsUserUtility class ==="
fd 'XoopsUserUtility' htdocs --type f | head -5
echo -e "\n=== Inspecting XoopsUserUtility::validate method ==="
grep -n 'function validate' htdocs/xoops_lib/Xmf/User/Utility.php -A 30Repository: XOOPS/XoopsCore25
Length of output: 506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find XoopsUserUtility class
echo "=== Searching for XoopsUserUtility ==="
fd 'Utility' htdocs --type f | grep -i user | head -10
echo -e "\n=== Broader search in Xmf ==="
find htdocs/xoops_lib -name "*.php" -type f | xargs grep -l "class XoopsUserUtility" 2>/dev/null || echo "Not found in xoops_lib"
echo -e "\n=== Search in profile module ==="
find htdocs/modules/profile -name "*.php" -type f | xargs grep -l "XoopsUserUtility" 2>/dev/null | head -5
echo -e "\n=== Locate validate function definition ==="
rg 'function validate.*\$pass.*\$vpass' htdocs --type php -A 5
echo -e "\n=== Alternative: search for XoopsUserUtility anywhere ==="
fd --type f '\.php$' htdocs | xargs grep -l "class.*XoopsUserUtility" 2>/dev/null | head -5Repository: XOOPS/XoopsCore25
Length of output: 99
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Test 1: password_hash() with array input
echo "=== Test: password_hash() with array input ==="
php -d display_errors=1 <<'PHP'
<?php
// Simulate what happens if $pass is an array
$pass = ['value' => 'test123'];
try {
$result = password_hash($pass, PASSWORD_DEFAULT);
echo "Success (unexpected): " . var_export($result, true) . "\n";
} catch (TypeError $e) {
echo "TypeError (expected): " . $e->getMessage() . "\n";
} catch (Exception $e) {
echo "Exception: " . get_class($e) . ": " . $e->getMessage() . "\n";
}
// Test 2: What happens with conditional check
echo "\n=== Test: Conditional before password_hash() ==="
$pass = ['value' => 'test123'];
echo "is_array(\$pass): " . (is_array($pass) ? "true" : "false") . "\n";
echo "Condition (\$pass ? ...): " . ($pass ? "true" : "false") . "\n";
if ($pass) {
echo "Would call password_hash() with array!\n";
}
?>
PHP
# Test 3: Verify Request behavior with array input
echo -e "\n=== Test: Mock Request::getVar with array input ==="
php -d display_errors=1 <<'PHP'
<?php
// Simulate $_POST with array input
$_POST = ['pass' => ['value' => 'test123']];
// Simulate MASK_ALLOW_RAW behavior
define('MASK_ALLOW_RAW', 1);
define('MASK_NO_TRIM', 2);
// When MASK_ALLOW_RAW is set, cleanVar() returns the variable unchanged
$mask = MASK_ALLOW_RAW | MASK_NO_TRIM;
if ($mask & MASK_ALLOW_RAW) {
// With MASK_ALLOW_RAW, the variable is returned unchanged
$pass = $_POST['pass']; // This is still an array!
echo "With MASK_ALLOW_RAW: ";
var_dump($pass);
}
?>
PHPRepository: XOOPS/XoopsCore25
Length of output: 354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Create a test PHP file instead
cat > /tmp/test_array.php <<'PHP'
<?php
// Test what happens with array input
$pass = ['value' => 'test123'];
// Test 1: Conditional check with array
echo "Test 1: Conditional behavior\n";
echo "is_array(\$pass): " . (is_array($pass) ? "true" : "false") . "\n";
echo "\$pass ? ... evaluates to: " . ($pass ? "true" : "false") . "\n";
echo "So if (\$pass) { password_hash(...) } WILL execute with array!\n";
// Test 2: What password_hash() does with array
echo "\nTest 2: password_hash() with array\n";
try {
$result = password_hash($pass, PASSWORD_DEFAULT);
echo "Result: $result\n";
} catch (Throwable $e) {
echo "Error: " . get_class($e) . ": " . $e->getMessage() . "\n";
}
// Test 3: Template assignment with array
echo "\nTest 3: Smarty template assignment\n";
echo "Assigning array to template variable would cause rendering issues\n";
echo "Array value: " . print_r($pass, true) . "\n";
?>
PHP
php /tmp/test_array.php
# Also verify the Request code flow
echo -e "\n=== Verify Request behavior with MASK_ALLOW_RAW ==="
grep -A 5 "MASK_ALLOW_RAW" htdocs/xoops_lib/vendor/xoops/xmf/src/Request.php | head -15Repository: XOOPS/XoopsCore25
Length of output: 963
Guard these raw password fetches against array input.
Request::getVar(..., 'string', ..., Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) bypasses type enforcement, allowing array payloads through. A malformed pass[]=... or vpass[]=... POST input will be returned unchanged as an array. When this array reaches password_hash() at lines 197 and 228, it triggers a TypeError: password_hash(): Argument #1 ($password) must be of type string, array given. Similarly, passing an array to the mailer template at line 281 causes incorrect rendering.
Apply is_string() guards to all three locations:
Suggested fix
$pass = Request::getVar('pass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM);
$vpass = Request::getVar('vpass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM);
+ $pass = is_string($pass) ? $pass : '';
+ $vpass = is_string($vpass) ? $vpass : ''; $pass = Request::getVar('pass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM);
+ $pass = is_string($pass) ? $pass : '';- $xoopsMailer->assign('X_UPASS', Request::getVar('vpass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM));
+ $mailPassword = Request::getVar('vpass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM);
+ $xoopsMailer->assign('X_UPASS', is_string($mailPassword) ? $mailPassword : '');Also applies to: 225, 281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/register.php` around lines 184 - 185, The raw POST
fetches for $pass and $vpass use Request::MASK_ALLOW_RAW and can yield arrays,
which breaks password_hash() and the mailer template; after obtaining $pass and
$vpass from Request::getVar, ensure each is a string by replacing non-strings
with an empty string (e.g., if (!is_string($pass)) $pass = ''; if
(!is_string($vpass)) $vpass = '';) and apply the same is_string() guard
immediately before each use of those variables in password_hash(...) and before
passing them into the mailer/template rendering so only scalar strings are ever
supplied.
| if ('' !== Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) && | ||
| Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) != Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM)) { | ||
| xoops_error(_AM_SYSTEM_USERS_STNPDNM); | ||
| break; | ||
| } | ||
|
|
||
| if ('' !== Request::getString('pass2') && | ||
| '' !== Request::getString('password') && | ||
| mb_strtolower(Request::getString('password'), 'UTF-8') === mb_strtolower(Request::getString('uname'), 'UTF-8')) { | ||
| if ('' !== Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) && | ||
| '' !== Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) && | ||
| mb_strtolower(Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM), 'UTF-8') === mb_strtolower(Request::getString('uname'), 'UTF-8')) { | ||
| xoops_error(_AM_SYSTEM_USERS_PWDEQUALSUNAME); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Validation checks are bypassed when pass2 is empty.
For new user creation, both password and pass2 should be required. The current checks at Lines 278-279 and 284-286 are skipped entirely when pass2 is empty, allowing the flow to proceed without proper password confirmation.
🐛 Proposed fix
+ $password = Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM);
+ $pass2 = Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM);
+
// Password match (if confirm provided)
- if ('' !== Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) &&
- Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) != Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM)) {
+ if ($password !== $pass2) {
xoops_error(_AM_SYSTEM_USERS_STNPDNM);
break;
}
- if ('' !== Request::getVar('pass2', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) &&
- '' !== Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM) &&
- mb_strtolower(Request::getVar('password', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM), 'UTF-8') === mb_strtolower(Request::getString('uname'), 'UTF-8')) {
+ if (mb_strtolower($password, 'UTF-8') === mb_strtolower(Request::getString('uname'), 'UTF-8')) {
xoops_error(_AM_SYSTEM_USERS_PWDEQUALSUNAME);
break;
}🤖 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 278 - 289,
Validation is bypassed when pass2 is empty; ensure both
Request::getVar('password', ...) and Request::getVar('pass2', ...) are required
for new-user flows by adding an explicit non-empty check before the existing
equality and username-equals-password checks: if either password or pass2 is
empty, call xoops_error with an appropriate message and stop processing,
otherwise proceed to compare password vs pass2 (use the existing
_AM_SYSTEM_USERS_STNPDNM error) and check password vs uname (use
_AM_SYSTEM_USERS_PWDEQUALSUNAME); update the logic around the
Request::getVar('pass2', ...), Request::getVar('password', ...), xoops_error and
related break statements so the equality/name checks always run only when both
fields are present.
| $pass = Request::getVar('pass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); | ||
| $vpass = Request::getVar('vpass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
request_file="$(fd '^Request\.php$' htdocs | grep '/vendor/xoops/xmf/src/Request.php$' | head -n1)"
echo "Inspecting: ${request_file}"
sed -n '80,127p' "$request_file"
printf '\n---\n'
sed -n '537,572p' "$request_file"Repository: XOOPS/XoopsCore25
Length of output: 2925
Reject non-scalar password inputs to prevent type errors in downstream string operations.
When Request::getVar() is called with MASK_ALLOW_RAW, it skips all type coercion. A malformed POST request like pass[]=x leaves $pass and $vpass as arrays, which will cause failures in downstream code expecting strings (validation, hashing, escaping). The type='string' parameter does not convert arrays to scalars once MASK_ALLOW_RAW is set.
Fix
$pass = Request::getVar('pass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM);
$vpass = Request::getVar('vpass', '', 'POST', 'string', Request::MASK_ALLOW_RAW | Request::MASK_NO_TRIM);
+ $pass = is_string($pass) ? $pass : '';
+ $vpass = is_string($vpass) ? $vpass : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/register.php` around lines 67 - 68, After reading $pass and $vpass via
Request::getVar (with MASK_ALLOW_RAW) ensure they are scalar strings before any
string operations: check is_scalar($pass) && is_scalar($vpass) (or is_string)
and if either is not scalar, treat it as invalid — e.g. set to empty string
and/or return a validation error to the user — so downstream code (validation,
hashing, escaping) never receives an array/object; do this immediately after the
Request::getVar calls that populate $pass and $vpass.
- step.php: Read step_save from loaded object instead of client request
- field.php: Use $obj->getVar('field_id') in delete confirmation form
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
| $obj = $groups_Handler->get($groups_id); | ||
| if (isset($_POST['ok']) && $_POST['ok'] == 1) { | ||
| if (!is_object($obj)) { | ||
| redirect_header('admin.php?fct=groups', 1, _AM_SYSTEM_DBERROR); | ||
| } |
There was a problem hiding this comment.
In the groups_delete case (around line 305-306), redirect_header() is used as a null-guard after $groups_Handler->get(), but redirect_header() does not halt PHP execution. After that redirect call, execution continues to $member_handler->deleteGroup($group) (line 319) with a non-object, causing a fatal error. The redirect_header() must be followed by return or exit.
| $restorePath = Request::getText('path_file', '', 'POST'); | ||
| $themesRoot = realpath(XOOPS_ROOT_PATH . '/themes'); | ||
| $resolved = realpath($restorePath); | ||
|
|
||
| // Strict prefix check to prevent directory traversal | ||
| $valid_dir = ($resolved !== false && $themesRoot !== false && strpos($resolved, $themesRoot) === 0); | ||
|
|
||
| $old_file = $_REQUEST['path_file'] . '.back'; | ||
| $new_file = $_REQUEST['path_file']; | ||
| $old_file = $resolved . '.back'; | ||
| $new_file = $resolved; |
There was a problem hiding this comment.
The tpls_restore case reads $restorePath from Request::getText('path_file', '', 'POST') and then calls realpath($restorePath). If $restorePath is an empty string (no POST parameter), realpath('') returns the current working directory (not false), so $resolved would be a valid path. The subsequent check $resolved !== false would pass even though no path was provided. A guard for empty $restorePath before calling realpath() should be added.
| $obj = $groups_Handler->get($groups_id); | ||
| if (!is_object($obj)) { | ||
| redirect_header('admin.php?fct=groups', 1, _AM_SYSTEM_DBERROR); | ||
| } | ||
| $form = $obj->getForm(); |
There was a problem hiding this comment.
In htdocs/modules/system/admin/groups/main.php at the groups_edit handler (around line 138-141), redirect_header() is used as a null-guard after $groups_Handler->get($groups_id), but it is not followed by return or exit. Execution will continue to $form = $obj->getForm() on line 141, causing a fatal error if $obj is not an object.
| $group = $member_handler->getGroup($groups_id); | ||
| if (!is_object($group)) { | ||
| redirect_header('admin.php?fct=groups', 1, _AM_SYSTEM_DBERROR); | ||
| } | ||
| $member_handler->deleteGroup($group); |
There was a problem hiding this comment.
In htdocs/modules/system/admin/groups/main.php at the groups_delete handler (around line 316-319), the redirect_header() null-guard for $group = $member_handler->getGroup($groups_id) is not followed by return or exit. Execution continues to $member_handler->deleteGroup($group) with a potentially non-object value, causing a fatal error.
| /** | ||
| * Verify getUrl returns a valid URL format from POST. | ||
| */ | ||
| #[Test] | ||
| public function getUrlReturnsValidUrlFormat(): void | ||
| { | ||
| $_POST['not_redirect'] = 'https://evil.com/phish'; | ||
| $value = Request::getUrl('not_redirect', '', 'POST'); | ||
| // getUrl preserves valid URLs — domain validation is the app's responsibility | ||
| // (see notification_update.php parse_url() check). This test verifies getUrl | ||
| // returns a non-empty string for well-formed URLs. | ||
| $this->assertIsString($value); | ||
| $this->assertNotSame('', $value); | ||
| } |
There was a problem hiding this comment.
In the test getUrlReturnsValidUrlFormat (line 300-307), the test validates that Request::getUrl() returns a non-empty string for https://evil.com/phish — a URL from an external domain. However, the test comment states "domain validation is the app's responsibility." This means the test does not actually verify the open-redirect protection that notification_update.php implements. The test is misleading because it implies getUrl() validates domain, when the real protection is in application code. The test should be renamed or updated to clearly describe what is being tested (that Request::getUrl() does NOT perform domain filtering) to avoid false confidence.
| $obj = $handler->get(Request::getInt('id', 0, 'GET')); | ||
| if (!$obj) { | ||
| redirect_header('category.php', 3, _TAKINGBACK); | ||
| } |
There was a problem hiding this comment.
In htdocs/modules/profile/admin/category.php, the redirect_header() null-guard calls (e.g., lines 55-57 and 72-74) are not followed by return or exit. Since redirect_header() does not halt PHP execution, code will continue executing after the redirect, potentially causing fatal errors when trying to call methods on non-objects (e.g., $form = $obj->getForm()).
| if (!is_object($obj)) { | ||
| redirect_header('step.php', 1, _PROFILE_AM_SAVESTEP_TOGGLE_FAILED); | ||
| } |
There was a problem hiding this comment.
In the profile_stepsave_toggle() function, redirect_header() on line 130 does not halt PHP execution. After the redirect call, execution will continue to line 132 ($obj->setVar(...)), which will cause a fatal error calling a method on a non-object. The redirect_header() call must be followed by return or exit.
| @@ -143,8 +146,11 @@ | |||
| $xoBreadCrumb->addTips(sprintf(_AM_SYSTEM_SMILIES_NAV_TIPS_FORM1, implode(', ', $mimetypes)) . sprintf(_AM_SYSTEM_SMILIES_NAV_TIPS_FORM2, $upload_size / 1000)); | |||
| $xoBreadCrumb->render(); | |||
|
|
|||
| if (isset($_POST['smilies_id'])) { | |||
| $obj = $smilies_Handler->get(Request::getInt('smilies_id', 0)); | |||
| if (Request::hasVar('smilies_id', 'POST')) { | |||
| $obj = $smilies_Handler->get(Request::getInt('smilies_id', 0, 'POST')); | |||
| if (!is_object($obj)) { | |||
| redirect_header('admin.php?fct=smilies', 2, _AM_SYSTEM_DBERROR); | |||
| } | |||
There was a problem hiding this comment.
In the smilies_edit save handler (line 151-153 in smilies/main.php), redirect_header() is called as a null-guard if $obj is not an object, but since redirect_header() does not halt execution, the code will continue to call methods on a non-object. The same issue exists at line 206-208 in the smilies_delete case. Each redirect_header() null-guard must be followed by return or exit.
| if (!is_object($group)) { | ||
| redirect_header('admin.php?fct=groups', 1, _AM_SYSTEM_DBERROR); | ||
| } | ||
| $group->setVar('name', Request::getString('name', '', 'POST')); | ||
| $group->setVar('description', Request::getString('desc', '', 'POST')); |
There was a problem hiding this comment.
In htdocs/modules/system/admin/groups/main.php at the groups_create handler (around line 228-232), redirect_header() is used as a null-guard after $member_handler->getGroup($gid), but it is not followed by return or exit. Execution will continue to $group->setVar(...) on the next line, causing a fatal error if $group is not an object.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
htdocs/modules/profile/admin/step.php (1)
110-113:⚠️ Potential issue | 🔴 CriticalDon't mutate
step_saveover GET.This reopens the original CSRF issue: Line 111 accepts a state-changing GET, and the current UI in
htdocs/modules/profile/templates/profile_admin_steplist.tpl:11still triggers it with a plain<a href="step.php?op=toggle...">. Any page an authenticated admin visits can therefore flipstep_save. Please move the toggle back to a tokenized POST form and restore$GLOBALS['xoopsSecurity']->check()here.🔒 Required fix
case 'list': $criteria = new CriteriaCompo(); $criteria->setSort('step_order'); $criteria->setOrder('ASC'); $GLOBALS['xoopsTpl']->assign('steps', $handler->getObjects($criteria, true, false)); + $GLOBALS['xoopsTpl']->assign('token', $GLOBALS['xoopsSecurity']->getTokenHTML()); $template_main = 'profile_admin_steplist.tpl'; break; @@ case 'toggle': - if (Request::hasVar('step_id', 'GET')) { - $step_id = Request::getInt('step_id', 0, 'GET'); - profile_stepsave_toggle($step_id); + if (!$GLOBALS['xoopsSecurity']->check()) { + redirect_header('step.php', 3, implode(',', $GLOBALS['xoopsSecurity']->getErrors())); + } + if (Request::hasVar('step_id', 'POST')) { + $step_id = Request::getInt('step_id', 0, 'POST'); + profile_stepsave_toggle($step_id); } break;Update
htdocs/modules/profile/templates/profile_admin_steplist.tplto replace the toggle link with a small POST form that submitsstep_idplus the XOOPS security token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/admin/step.php` around lines 110 - 113, The toggle operation currently accepts state-changing GETs (Request::hasVar/Request::getInt) and calls profile_stepsave_toggle($step_id); change this to require a tokenized POST: in the handler re-check the request method and call $GLOBALS['xoopsSecurity']->check() before calling profile_stepsave_toggle, rejecting non-POST or invalid-token requests; also update the UI template to replace the plain <a> toggle link with a small POST form that submits step_id plus the XOOPS security token so the server-side POST+check flow is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/profile/admin/field.php`:
- Around line 374-377: The toggle currently uses the POSTed field_required value
which can be stale or tampered; change the logic so
profile_visible_toggle($field_id, ...) computes the new value from the stored
object instead of the request. Remove or stop using
Request::getInt('field_required', ...) and inside the call path
(profile_visible_toggle and the code that loads the field object) read
$obj->getVar('field_required') and invert that to derive the new state, then
pass that computed value to profile_visible_toggle; apply the same change to the
other similar block that uses field_required (the block around the other
profile_visible_toggle call).
- Around line 146-151: Validate and sanitize the incoming field id list before
using it to index the POST maps: ensure each id in $field_ids is cast/normalized
(e.g., to int/string) and exists as a key in the corresponding arrays
($oldweight, $weight, $oldcat, $category) and in the result of getObjects();
filter $field_ids to the intersection of those valid keys so you won't access
undefined-array-key entries, and when calling getObjects() on the filtered ids,
check the returned $fields contains each requested id and skip/log any missing
ids instead of assuming all were returned (update the loop that uses $field_ids
and the code relying on $fields to handle missing entries safely).
---
Duplicate comments:
In `@htdocs/modules/profile/admin/step.php`:
- Around line 110-113: The toggle operation currently accepts state-changing
GETs (Request::hasVar/Request::getInt) and calls
profile_stepsave_toggle($step_id); change this to require a tokenized POST: in
the handler re-check the request method and call
$GLOBALS['xoopsSecurity']->check() before calling profile_stepsave_toggle,
rejecting non-POST or invalid-token requests; also update the UI template to
replace the plain <a> toggle link with a small POST form that submits step_id
plus the XOOPS security token so the server-side POST+check flow is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54accc77-05be-4aec-be94-08926fbb05de
📒 Files selected for processing (2)
htdocs/modules/profile/admin/field.phphtdocs/modules/profile/admin/step.php
| $field_ids = Request::getArray('field_ids', [], 'POST'); | ||
| if (count($field_ids) > 0) { | ||
| $oldweight = Request::getArray('oldweight', [], 'POST'); | ||
| $oldcat = Request::getArray('oldcat', [], 'POST'); | ||
| $category = Request::getArray('category', [], 'POST'); | ||
| $weight = Request::getArray('weight', [], 'POST'); |
There was a problem hiding this comment.
Validate reordered ids before indexing the POST maps.
This loop assumes every submitted field_id exists in oldweight, weight, oldcat, category, and later in $fields. A stale or tampered submission will raise undefined-array-key warnings at Line 154 and can fatal at Lines 166-168 when getObjects() does not return one of the requested ids.
🛡️ Suggested hardening
$field_ids = Request::getArray('field_ids', [], 'POST');
if (count($field_ids) > 0) {
$oldweight = Request::getArray('oldweight', [], 'POST');
$oldcat = Request::getArray('oldcat', [], 'POST');
$category = Request::getArray('category', [], 'POST');
$weight = Request::getArray('weight', [], 'POST');
$ids = [];
foreach ($field_ids as $field_id) {
+ $field_id = (int) $field_id;
+ if (!isset($oldweight[$field_id], $weight[$field_id], $oldcat[$field_id], $category[$field_id])) {
+ continue;
+ }
if ($oldweight[$field_id] != $weight[$field_id] || $oldcat[$field_id] != $category[$field_id]) {
//if field has changed
- $ids[] = (int) $field_id;
+ $ids[] = $field_id;
}
}
if (count($ids) > 0) {
$errors = [];
//if there are changed fields, fetch the fieldcategory objects
@@
$fields = $field_handler->getObjects(new Criteria('field_id', '(' . implode(',', $ids) . ')', 'IN'), true);
foreach ($ids as $i) {
+ if (!isset($fields[$i])) {
+ $errors[] = _PROFILE_AM_FIELDNOTCONFIGURABLE;
+ continue;
+ }
$fields[$i]->setVar('field_weight', (int) $weight[$i]);
$fields[$i]->setVar('cat_id', (int) $category[$i]);Also applies to: 153-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/admin/field.php` around lines 146 - 151, Validate and
sanitize the incoming field id list before using it to index the POST maps:
ensure each id in $field_ids is cast/normalized (e.g., to int/string) and exists
as a key in the corresponding arrays ($oldweight, $weight, $oldcat, $category)
and in the result of getObjects(); filter $field_ids to the intersection of
those valid keys so you won't access undefined-array-key entries, and when
calling getObjects() on the filtered ids, check the returned $fields contains
each requested id and skip/log any missing ids instead of assuming all were
returned (update the loop that uses $field_ids and the code relying on $fields
to handle missing entries safely).
| $field_id = Request::getInt('field_id', 0, 'POST'); | ||
| if ($field_id > 0) { | ||
| $field_required = Request::getInt('field_required', 0, 'POST'); | ||
| profile_visible_toggle($field_id, $field_required); |
There was a problem hiding this comment.
Toggle field_required from the stored row, not the request.
profile_visible_toggle() already loads the field object, but it computes the next value from the client-supplied field_required. A stale admin page or a tampered POST can therefore leave the flag unchanged or flip to the wrong state. Derive the new value from $obj->getVar('field_required') and drop the redundant request parameter.
🔁 Suggested fix
$field_id = Request::getInt('field_id', 0, 'POST');
if ($field_id > 0) {
- $field_required = Request::getInt('field_required', 0, 'POST');
- profile_visible_toggle($field_id, $field_required);
+ profile_visible_toggle($field_id);
}
break;
@@
-function profile_visible_toggle($field_id, $field_required)
+function profile_visible_toggle($field_id)
{
- $field_required = ($field_required == 1) ? 0 : 1;
$this_handler = xoops_getModuleHandler('field', 'profile');
$obj = $this_handler->get($field_id);
if (!is_object($obj)) {
redirect_header('field.php', 2, _PROFILE_AM_FIELDNOTCONFIGURABLE);
}
+ $field_required = ((int) $obj->getVar('field_required') === 1) ? 0 : 1;
$obj->setVar('field_required', $field_required);Also applies to: 390-398
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/admin/field.php` around lines 374 - 377, The toggle
currently uses the POSTed field_required value which can be stale or tampered;
change the logic so profile_visible_toggle($field_id, ...) computes the new
value from the stored object instead of the request. Remove or stop using
Request::getInt('field_required', ...) and inside the call path
(profile_visible_toggle and the code that loads the field object) read
$obj->getVar('field_required') and invert that to derive the new state, then
pass that computed value to profile_visible_toggle; apply the same change to the
other similar block that uses field_required (the block around the other
profile_visible_toggle call).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1633 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Refactor
Bug Fixes / Security
Tests
UX