Replace deprecated queryF() with query()/exec(), fix runtime errors …#1639
Conversation
…and @ suppression Deprecated API removal — queryF() (deprecated since 2.5.12) Replace all queryF() calls in XOOPS-owned code with the correct modern method: - query() for read statements (SELECT, SHOW, DESCRIBE, EXPLAIN) - exec() for write/DDL statements (INSERT, UPDATE, DELETE, ALTER, CREATE, DROP, SET) Files changed: - class/database/mysqldatabase.php — exec() for SET SQL_BIG_SELECTS; Proxy class delegates to parent::query() instead of queryF() - kernel/online.php, kernel/session.php — SELECT queries → query() - modules/pm/include/install.php, modules/pm/include/update.php — SHOW queries → query() - modules/system/class/maintenance.php — SHOW/SELECT queries → query() - modules/system/include/update.php — SHOW INDEX → query() - xoops_lib/modules/protector/admin/prefix_manager.php — SHOW/SELECT queries → query() - xoops_lib/modules/protector/class/protector.php — INSERT → exec() - 9 upgrade scripts (cnt-2.2.x, upd-2.0.13 through upd-2.5.11) — queryF() → query() or exec() as appropriate; internal query() wrappers updated from $db->queryF() to $db->exec() Misuse of query() for non-SELECT statements → exec() - modules/system/admin/modulesadmin/modulesadmin.php — 8× SET FOREIGN_KEY_CHECKS → exec() - modules/system/admin/avatars/main.php — 2× UPDATE → exec() - xoops_lib/modules/protector/admin/center.php — 3× DELETE → exec() - install/class/dbmanager.php — CREATE DATABASE → exec() - upgrade/upd-2.4.x-to-2.5.0/dbmanager.php — CREATE DATABASE, CREATE TABLE, DROP TABLE → exec() Runtime error fixes from log analysis - class/utility/xoopsutility.php — Fix Fatal TypeError in recursive(): array_map() arguments were swapped ($handler passed as data array) - upgrade/preflight.php — Fix malformed path: __DIR__ . "../language/" → __DIR__ . "/../language/" (missing slash caused file_exists to always fail) - 4× form renderers (Bootstrap3, Bootstrap4, Bootstrap5, Legacy) — Replace @[$encode, $js] = $extension->encode() with null-coalescing to avoid Undefined array key 0/1 when extensions return empty array; removes @ error suppression - modules/system/themes/dark/xotpl/xo_globalnav.tpl — Add |default:'' to $option.link and $option.title - modules/system/themes/transition/xotpl/xo_globalnav.tpl — Same fix as dark theme
WalkthroughNormalized DB API usage (queryF/query → query/exec) across many paths, widened MySQL proxy read-only regex, safer unpacking in form renderers, changed recursive array_map semantics, defensive template defaults, guarded avatar file deletion, protector random_bytes fallback, and multiple upgrade/maintenance hardenings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
GregMage,
NicoAndricq,
alain01,
geekwright,
ggoffy,
ihackcode,
kevinpetit,
liomj,
montuy337513 and
tad0616
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (12)
upgrade/upd-2.0.18-to-2.3.0/settings_db.php (4)
134-134:⚠️ Potential issue | 🟠 MajorXSS vulnerability:
$_SERVER['PHP_SELF']unescaped in form action.
PHP_SELFcan be manipulated viaPATH_INFOto inject malicious scripts. Output must be escaped.🛡️ Proposed fix
-<form action="<?php echo $_SERVER['PHP_SELF']; ?>" method='post'> +<form action="<?php echo htmlspecialchars($_SERVER['PHP_SELF'], ENT_QUOTES, 'UTF-8'); ?>" method='post'>As per coding guidelines: "Escape all output with htmlspecialchars($value, ENT_QUOTES, 'UTF-8') or use Smarty auto-escaping"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php` at line 134, Replace the unescaped usage of $_SERVER['PHP_SELF'] in the form action in settings_db.php with a properly escaped output; specifically, update the form action to echo htmlspecialchars($_SERVER['PHP_SELF'], ENT_QUOTES, 'UTF-8') (or otherwise supply an explicit safe path) instead of raw $_SERVER['PHP_SELF'] to prevent XSS.
99-99:⚠️ Potential issue | 🟡 MinorSyntax error: extra double-quote produces malformed HTML.
There's a stray
\"before the closing>that will break the select element rendering.🐛 Proposed fix
- $field .= "<select name='$name' id='$name'\">"; + $field .= "<select name='$name' id='$name'>";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php` at line 99, The generated select tag in settings_db.php is malformed due to an extra double-quote in the string concatenation for $field; locate the line that appends the select (the one using $field and $name) and remove the stray double-quote before the closing > so the tag is rendered as a proper <select name='$name' id='$name'> element (ensure quotes around $name remain correct for the chosen PHP string delimiter).
95-98:⚠️ Potential issue | 🟡 Minor
$helpparameter output without escaping.While
$labeland$nameare sanitized viahtmlSpecialChars(), the$helpparameter is rendered directly into HTML, creating a potential XSS vector if caller-controlled.🛡️ Proposed fix
$field = "<label for='$name'>$label</label>\n"; if ($help) { - $field .= '<div class="xoform-help">' . $help . "</div>\n"; + $field .= '<div class="xoform-help">' . $myts->htmlSpecialChars($help, ENT_QUOTES, _UPGRADE_CHARSET, false) . "</div>\n"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php` around lines 95 - 98, Escape the untrusted $help output before appending it to $field to prevent XSS: replace the raw $help usage in the block that builds $field (the lines that currently append '<div class="xoform-help">' . $help . "</div>\n") with a properly escaped version (use the same sanitizer used for $label/$name, e.g. htmlSpecialChars or htmlspecialchars with ENT_QUOTES and UTF-8) so the div contains escaped help text rather than raw HTML.
116-120:⚠️ Potential issue | 🟠 MajorDirect
$_POSTaccess violates input filtering guidelines.Using
@$_POST['task']bypasses input validation and applies error suppression. Per coding guidelines, all user input must be filtered usingXmf\Request::getVar()orXmf\FilterInput::clean().🛡️ Proposed fix using Xmf\Request
-if ($_SERVER['REQUEST_METHOD'] === 'POST' && @$_POST['task'] === 'db') { +if ($_SERVER['REQUEST_METHOD'] === 'POST' && \Xmf\Request::getString('task', '', 'POST') === 'db') { $params = ['DB_COLLATION']; foreach ($params as $name) { - $vars[$name] = $_POST[$name] ?? ''; + $vars[$name] = \Xmf\Request::getString($name, '', '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 `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php` around lines 116 - 120, Replace direct and suppressed access to $_POST in the POST handling branch with Xmf\Request input filtering: stop using @$_POST['task'] and $_POST[$name] and instead read the action via Xmf\Request::getString('task', '', 'POST') (or getVar/getInt as appropriate) and read DB_COLLATION via Xmf\Request::getString('DB_COLLATION', '', 'POST') inside the same if ($_SERVER['REQUEST_METHOD'] === 'POST') block; update the loop that builds $vars to use the filtered value for each $name and remove the error suppression operator so all user input is validated via Xmf\Request before use.upgrade/upd-2.4.x-to-2.5.0/dbmanager.php (1)
266-277:⚠️ Potential issue | 🔴 CriticalCritical: Undefined variable
$ctcauses runtime error.Line 272 references
$ct, but the loop declares$keyand$val. In PHP 8.0+, this throws anErrorfor undefined variable access, breaking the upgrade script when deleting tables.🐛 Proposed fix
public function deleteTables($tables) { $deleted = []; $this->db->connect(); foreach ($tables as $key => $val) { if (!$this->db->exec('DROP TABLE ' . $this->db->prefix($key))) { - $deleted[] = $ct; + $deleted[] = $key; } } return $deleted; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.4.x-to-2.5.0/dbmanager.php` around lines 266 - 277, The deleteTables function references an undefined $ct inside the loop causing a runtime error; replace the $ct reference with the correct loop variable ($key) so the function uses the declared variables ($key, $val) and appends the proper table identifier to $deleted when a DROP TABLE fails; update the code in deleteTables to use $key (or $val if your table identifier is stored there) instead of $ct and keep the existing exec/prefix('...') logic intact.upgrade/upd_2.5.10-to-2.5.11/index.php (1)
805-810:⚠️ Potential issue | 🟠 MajorTreat any existing row as already migrated.
Using
1 == $counthere will insert anotherdefault_notificationrecord if the table already contains duplicates. This upgrade should short-circuit on any count greater than zero.Proposed fix
- if (1 == $count) { + if ((int) $count > 0) { $notification_method = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd_2.5.10-to-2.5.11/index.php` around lines 805 - 810, The conditional currently treats only exactly one existing row as migrated; change the logic to treat any count greater than zero as already migrated by updating the check that sets $notification_method (use $count > 0 or (int)$count !== 0) after fetching the row from $GLOBALS['xoopsDB']->fetchRow($result) for the $sql query for `default_notification` so the upgrade short-circuits when any existing rows exist and avoids inserting duplicates.upgrade/upd-2.5.4-to-2.5.5/index.php (1)
87-93:⚠️ Potential issue | 🟠 MajorReturn an explicit boolean from
check_imptotal().This method is documented as
@return bool, but the failure path returnsnull. That makes the upgrade decision ambiguous exactly when the metadata query fails; emit a warning and return a definite boolean instead.As per coding guidelines, "Use trigger_error() with E_USER_WARNING for non-fatal failures 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 `@upgrade/upd-2.5.4-to-2.5.5/index.php` around lines 87 - 93, The function check_imptotal currently returns null on DB query failure; change the failure branch so it triggers a non-fatal warning and returns a definite boolean: call trigger_error() with E_USER_WARNING (use basename(__FILE__) in the message for context) when $GLOBALS['xoopsDB']->query($sql) fails, and then return a boolean (e.g., false) instead of null; keep the existing return ($length != 8) behavior when the query succeeds.htdocs/class/database/mysqldatabase.php (1)
638-646:⚠️ Potential issue | 🟠 MajorThe read-only proxy still permits mutating SQL on POST requests.
XoopsMySQLDatabaseProxyis documented as read-only, but this guard only blocks non-SELECT statements when$this->allowWebChangesis false. On POST,parent::query()will still execute UPDATE/DELETE/DDL and only warn in strict mode, which breaks the proxy’s advertised safety boundary.As per coding guidelines, `htdocs/**/*.php`: Check for proper use of XOOPS database API.🔒 Proposed fix
public function query(string $sql, ?int $limit = null, ?int $start = null) { $sql = ltrim($sql); - if (!$this->allowWebChanges && stripos($sql, 'select') !== 0) { - trigger_error('Database updates are not allowed during processing of a GET request', E_USER_WARNING); + if (!preg_match('/^(SELECT|WITH|SHOW|DESCRIBE|EXPLAIN)\b/i', $sql)) { + trigger_error('XoopsMySQLDatabaseProxy only allows read-only statements', E_USER_WARNING); return false; } return parent::query($sql, $limit, $start); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/database/mysqldatabase.php` around lines 638 - 646, The query method in class XoopsMySQLDatabaseProxy currently only blocks non-SELECT SQL when $this->allowWebChanges is false, letting UPDATE/DELETE/DDL run on POST; change the guard in query(string $sql, ?int $limit = null, ?int $start = null) so that any non-SELECT statement is rejected by the proxy (trigger_error and return false) regardless of $this->allowWebChanges, thereby enforcing the proxy’s read-only contract before calling parent::query($sql, $limit, $start).upgrade/upd-2.0.18-to-2.3.0/index.php (4)
327-334:⚠️ Potential issue | 🔴 Critical
DESCRIBEis a read statement — usequery()instead ofexec().
DESCRIBEreturns a result set with column metadata. Usingexec()discards this result set and returns only a boolean, makingisResultSet()fail and causingfetchArray()to malfunction.Proposed fix
$sql = "DESCRIBE $table"; - $result = $GLOBALS['xoopsDB']->exec($sql); + $result = $GLOBALS['xoopsDB']->query($sql); if (!$GLOBALS['xoopsDB']->isResultSet($result)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.0.18-to-2.3.0/index.php` around lines 327 - 334, The DESCRIBE query is a read operation so replace the call to $GLOBALS['xoopsDB']->exec($sql) with $GLOBALS['xoopsDB']->query($sql) so the result set is preserved for isResultSet() and subsequent fetchArray() calls; keep the existing error check using $GLOBALS['xoopsDB']->isResultSet($result) and throw the same RuntimeException with \sprintf(_DB_QUERY_ERROR, $sql) . $GLOBALS['xoopsDB']->error() if query() fails.
453-454:⚠️ Potential issue | 🟠 MajorDirect
$_POSTaccess violates coding guidelines.Use
Xmf\Request::getVar()orXmf\Request::getString()instead of directly accessing$_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."Proposed fix
+ use Xmf\Request; + // ... in set_configs method: - if (empty($_POST['task']) || $_POST['task'] != $task) { + $postedTask = Request::getString('task', '', 'POST'); + if (empty($postedTask) || $postedTask !== $task) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.0.18-to-2.3.0/index.php` around lines 453 - 454, Replace direct access to $_POST['task'] with Xmf\Request helper; retrieve the POSTed task using Xmf\Request::getString('task', '') (or Xmf\Request::getVar('task', '', 'POST') depending on style) and then compare that sanitized value to $task instead of using $_POST directly; ensure you provide a safe default and use the string-returning method so the conditional if (empty(... ) || ... != $task) operates on the filtered value.
283-287:⚠️ Potential issue | 🔴 Critical
exec()returnsbool, not a result set —isResultSet()check is always false.The
exec()method returnstrueon success orfalseon failure. CallingisResultSet()on this boolean will always returnfalse, causing this function to incorrectly returnfalseeven when theALTER DATABASEsucceeds.Proposed fix
$sql = 'ALTER DATABASE `' . XOOPS_DB_NAME . '` DEFAULT CHARACTER SET ' . $GLOBALS['xoopsDB']->quote($charset) . ' COLLATE ' . $GLOBALS['xoopsDB']->quote($collation); $result = $GLOBALS['xoopsDB']->exec($sql); - if (!$GLOBALS['xoopsDB']->isResultSet($result)) { + if ($result === false) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.0.18-to-2.3.0/index.php` around lines 283 - 287, The current check treats the return of $GLOBALS['xoopsDB']->exec($sql) as a result set by calling isResultSet(), which is wrong because exec() returns bool; change the validation to test the boolean directly (i.e., check if $result === false and return false on failure) for the ALTER DATABASE statement built in $sql, or alternatively use $GLOBALS['xoopsDB']->query($sql) if you intend to receive a result set; update the code that references $result, $GLOBALS['xoopsDB']->exec, and $GLOBALS['xoopsDB']->isResultSet accordingly.
431-431:⚠️ Potential issue | 🟠 MajorMissing quotes around constant name in
defined()call.
defined(PHP_EOL)evaluates the constantPHP_EOL(a newline string) and checks if that string is defined as a constant name. This should bedefined('PHP_EOL')with quotes.Proposed fix
- $newline = defined(PHP_EOL) ? PHP_EOL : (strpos(php_uname(), 'Windows') ? "\r\n" : "\n"); + $newline = defined('PHP_EOL') ? PHP_EOL : (strpos(php_uname(), 'Windows') ? "\r\n" : "\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.0.18-to-2.3.0/index.php` at line 431, The defined() call incorrectly passes the constant value instead of its name; update the check in the $newline assignment to use defined('PHP_EOL') (add quotes around PHP_EOL) so defined() tests for the constant name, leaving the rest of the ternary that uses php_uname() and the $newline variable intact.
🤖 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/system/admin/avatars/main.php`:
- Around line 333-338: In delfileok, initialize and sanitize $user_id before the
existing isset($user_id) check by reading it from the request using Xmf\Request
(or Xmf\FilterInput) so the UID-specific branch can be taken; e.g. use
Xmf\Request::getInt('user_id', 0, 'GET') (or equivalent clean method) to
populate $user_id, then keep the existing avatar->getVar('avatar_type') check
and subsequent $xoopsDB->exec() updates—this ensures $user_id is defined,
sanitized, and used when updating user_avatar by uid instead of always falling
back to filename-based updates.
In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php`:
- Around line 162-166: The cleanup block that calls $db->exec('SET
FOREIGN_KEY_CHECKS = 0'), iterates $created_tables and runs DROP TABLE, then
$db->exec('SET FOREIGN_KEY_CHECKS = 1') must check and log failures and
guarantee re-enabling foreign key checks even if a DROP fails; modify the code
around the $db->exec calls and the foreach over $created_tables so each exec's
return/exception is handled (or wrapped in try/catch), log any errors (using the
existing logger or trigger_error) when a DROP TABLE or SET FOREIGN_KEY_CHECKS
call fails, and ensure the final SET FOREIGN_KEY_CHECKS = 1 is executed in a
finally-like path so foreign key checks are not left disabled for the session.
In `@htdocs/xoops_lib/modules/protector/admin/center.php`:
- Line 164: The DELETE executes even when $ids is empty causing invalid SQL;
update the logic around the $db->exec("DELETE FROM $log_table WHERE lid IN (" .
implode(',', $ids) . ')') call in center.php to first check that $ids is
non-empty (e.g. if (!empty($ids)) or count($ids) > 0) before building and
executing the query, and only perform the exec when the check passes (optionally
sanitize/ensure integers in $ids before implode to be safe).
In `@upgrade/upd_2.5.10-to-2.5.11/index.php`:
- Around line 789-795: The check_notificationmethod() function currently returns
true as long as the SELECT runs, which hides missing rows; update it to fetch
the COUNT result (e.g. using $GLOBALS['xoopsDB']->fetchRow($result)), cast it to
an int, and return true only if the count is greater than zero; keep the
existing error path that returns false when the query fails and ensure this
change prevents skipping apply_notificationmethod() when the row is absent.
In `@upgrade/upd-2.5.4-to-2.5.5/index.php`:
- Around line 27-31: When $GLOBALS['xoopsDB']->query(...) for the SHOW KEYS
probe fails inside the foreach over $tables, do not silently continue and let
the upgrader return true; instead call trigger_error(...) with E_USER_WARNING
and include basename(__FILE__) plus a concise message (and optional DB error
info) and then return false to abort the upgrade path; apply the same change to
the similar check around lines 56-60 so both uses of
$GLOBALS['xoopsDB']->query(...) and $GLOBALS['xoopsDB']->isResultSet(...) are
handled the same way.
- Around line 103-105: The if-statement assigns the exec() return to an unused
local ($result) then tests it; remove the dead assignment and test the exec()
call directly: replace the guarded check that uses "$result =
$GLOBALS['xoopsDB']->exec($sql)" with a direct call
"!$GLOBALS['xoopsDB']->exec($sql)" so the code no longer creates the unused
$result variable (the SQL variable $sql and the XOOPS DB exec() call are the
relevant symbols to change).
In `@upgrade/upd-2.5.5-to-2.5.6/index.php`:
- Around line 33-35: The check_* methods currently treat a failed SHOW COLUMNS
query as "already applied" by returning true when
!$GLOBALS['xoopsDB']->isResultSet($result); update each check_com_*() (the three
check_com_* methods shown) to return false on query failure instead of true so
the upgrader queues the ALTER task; specifically, in the branches after $result
= $GLOBALS['xoopsDB']->query($sql) where isResultSet($result) is checked, change
the early return to return false (or otherwise signal failure) so check_com_*()
reports the patch as not applied when the SHOW COLUMNS query fails.
---
Outside diff comments:
In `@htdocs/class/database/mysqldatabase.php`:
- Around line 638-646: The query method in class XoopsMySQLDatabaseProxy
currently only blocks non-SELECT SQL when $this->allowWebChanges is false,
letting UPDATE/DELETE/DDL run on POST; change the guard in query(string $sql,
?int $limit = null, ?int $start = null) so that any non-SELECT statement is
rejected by the proxy (trigger_error and return false) regardless of
$this->allowWebChanges, thereby enforcing the proxy’s read-only contract before
calling parent::query($sql, $limit, $start).
In `@upgrade/upd_2.5.10-to-2.5.11/index.php`:
- Around line 805-810: The conditional currently treats only exactly one
existing row as migrated; change the logic to treat any count greater than zero
as already migrated by updating the check that sets $notification_method (use
$count > 0 or (int)$count !== 0) after fetching the row from
$GLOBALS['xoopsDB']->fetchRow($result) for the $sql query for
`default_notification` so the upgrade short-circuits when any existing rows
exist and avoids inserting duplicates.
In `@upgrade/upd-2.0.18-to-2.3.0/index.php`:
- Around line 327-334: The DESCRIBE query is a read operation so replace the
call to $GLOBALS['xoopsDB']->exec($sql) with $GLOBALS['xoopsDB']->query($sql) so
the result set is preserved for isResultSet() and subsequent fetchArray() calls;
keep the existing error check using $GLOBALS['xoopsDB']->isResultSet($result)
and throw the same RuntimeException with \sprintf(_DB_QUERY_ERROR, $sql) .
$GLOBALS['xoopsDB']->error() if query() fails.
- Around line 453-454: Replace direct access to $_POST['task'] with Xmf\Request
helper; retrieve the POSTed task using Xmf\Request::getString('task', '') (or
Xmf\Request::getVar('task', '', 'POST') depending on style) and then compare
that sanitized value to $task instead of using $_POST directly; ensure you
provide a safe default and use the string-returning method so the conditional if
(empty(... ) || ... != $task) operates on the filtered value.
- Around line 283-287: The current check treats the return of
$GLOBALS['xoopsDB']->exec($sql) as a result set by calling isResultSet(), which
is wrong because exec() returns bool; change the validation to test the boolean
directly (i.e., check if $result === false and return false on failure) for the
ALTER DATABASE statement built in $sql, or alternatively use
$GLOBALS['xoopsDB']->query($sql) if you intend to receive a result set; update
the code that references $result, $GLOBALS['xoopsDB']->exec, and
$GLOBALS['xoopsDB']->isResultSet accordingly.
- Line 431: The defined() call incorrectly passes the constant value instead of
its name; update the check in the $newline assignment to use defined('PHP_EOL')
(add quotes around PHP_EOL) so defined() tests for the constant name, leaving
the rest of the ternary that uses php_uname() and the $newline variable intact.
In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php`:
- Line 134: Replace the unescaped usage of $_SERVER['PHP_SELF'] in the form
action in settings_db.php with a properly escaped output; specifically, update
the form action to echo htmlspecialchars($_SERVER['PHP_SELF'], ENT_QUOTES,
'UTF-8') (or otherwise supply an explicit safe path) instead of raw
$_SERVER['PHP_SELF'] to prevent XSS.
- Line 99: The generated select tag in settings_db.php is malformed due to an
extra double-quote in the string concatenation for $field; locate the line that
appends the select (the one using $field and $name) and remove the stray
double-quote before the closing > so the tag is rendered as a proper <select
name='$name' id='$name'> element (ensure quotes around $name remain correct for
the chosen PHP string delimiter).
- Around line 95-98: Escape the untrusted $help output before appending it to
$field to prevent XSS: replace the raw $help usage in the block that builds
$field (the lines that currently append '<div class="xoform-help">' . $help .
"</div>\n") with a properly escaped version (use the same sanitizer used for
$label/$name, e.g. htmlSpecialChars or htmlspecialchars with ENT_QUOTES and
UTF-8) so the div contains escaped help text rather than raw HTML.
- Around line 116-120: Replace direct and suppressed access to $_POST in the
POST handling branch with Xmf\Request input filtering: stop using
@$_POST['task'] and $_POST[$name] and instead read the action via
Xmf\Request::getString('task', '', 'POST') (or getVar/getInt as appropriate) and
read DB_COLLATION via Xmf\Request::getString('DB_COLLATION', '', 'POST') inside
the same if ($_SERVER['REQUEST_METHOD'] === 'POST') block; update the loop that
builds $vars to use the filtered value for each $name and remove the error
suppression operator so all user input is validated via Xmf\Request before use.
In `@upgrade/upd-2.4.x-to-2.5.0/dbmanager.php`:
- Around line 266-277: The deleteTables function references an undefined $ct
inside the loop causing a runtime error; replace the $ct reference with the
correct loop variable ($key) so the function uses the declared variables ($key,
$val) and appends the proper table identifier to $deleted when a DROP TABLE
fails; update the code in deleteTables to use $key (or $val if your table
identifier is stored there) instead of $ct and keep the existing
exec/prefix('...') logic intact.
In `@upgrade/upd-2.5.4-to-2.5.5/index.php`:
- Around line 87-93: The function check_imptotal currently returns null on DB
query failure; change the failure branch so it triggers a non-fatal warning and
returns a definite boolean: call trigger_error() with E_USER_WARNING (use
basename(__FILE__) in the message for context) when
$GLOBALS['xoopsDB']->query($sql) fails, and then return a boolean (e.g., false)
instead of null; keep the existing return ($length != 8) behavior when the query
succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3002f49d-6336-4242-9656-4ab847e50b18
📒 Files selected for processing (34)
htdocs/class/database/mysqldatabase.phphtdocs/class/utility/xoopsutility.phphtdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap3.phphtdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap4.phphtdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap5.phphtdocs/class/xoopsform/renderer/XoopsFormRendererLegacy.phphtdocs/install/class/dbmanager.phphtdocs/kernel/online.phphtdocs/kernel/session.phphtdocs/modules/pm/include/install.phphtdocs/modules/pm/include/update.phphtdocs/modules/system/admin/avatars/main.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phphtdocs/modules/system/class/maintenance.phphtdocs/modules/system/include/update.phphtdocs/modules/system/themes/dark/xotpl/xo_globalnav.tplhtdocs/modules/system/themes/transition/xotpl/xo_globalnav.tplhtdocs/xoops_lib/modules/protector/admin/center.phphtdocs/xoops_lib/modules/protector/admin/prefix_manager.phphtdocs/xoops_lib/modules/protector/class/protector.phpupgrade/cnt-2.2.x-to-2.3.0/index.phpupgrade/upd-2.0.13-to-2.0.14/index.phpupgrade/upd-2.0.15-to-2.0.16/index.phpupgrade/upd-2.0.16-to-2.0.17/index.phpupgrade/upd-2.0.17-to-2.0.18/index.phpupgrade/upd-2.0.18-to-2.3.0/index.phpupgrade/upd-2.0.18-to-2.3.0/settings_db.phpupgrade/upd-2.3.0-to-2.3.1/index.phpupgrade/upd-2.3.3-to-2.4.0/index.phpupgrade/upd-2.4.x-to-2.5.0/dbmanager.phpupgrade/upd-2.4.x-to-2.5.0/index.phpupgrade/upd-2.5.4-to-2.5.5/index.phpupgrade/upd-2.5.5-to-2.5.6/index.phpupgrade/upd_2.5.10-to-2.5.11/index.php
| foreach ($tables as $table => $keys) { | ||
| $sql = 'SHOW KEYS FROM `' . $GLOBALS['xoopsDB']->prefix($table) . '`'; | ||
| $result = $GLOBALS['xoopsDB']->queryF($sql); | ||
| $result = $GLOBALS['xoopsDB']->query($sql); | ||
| if (!$GLOBALS['xoopsDB']->isResultSet($result)) { | ||
| continue; |
There was a problem hiding this comment.
Don’t treat a failed SHOW KEYS probe as success.
If query() fails here, both methods continue, and this upgrader then falls through to return true. That can silently skip the missing-index migration instead of surfacing the schema inspection failure.
Suggested fix
- if (!$GLOBALS['xoopsDB']->isResultSet($result)) {
- continue;
- }
+ if (!$GLOBALS['xoopsDB']->isResultSet($result)) {
+ trigger_error(
+ basename(__FILE__) . ": failed to inspect keys for table {$table}",
+ E_USER_WARNING
+ );
+ return false;
+ }As per coding guidelines, "Use trigger_error() with E_USER_WARNING for non-fatal failures and use basename() in error messages to avoid exposing server paths".
Also applies to: 56-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upgrade/upd-2.5.4-to-2.5.5/index.php` around lines 27 - 31, When
$GLOBALS['xoopsDB']->query(...) for the SHOW KEYS probe fails inside the foreach
over $tables, do not silently continue and let the upgrader return true; instead
call trigger_error(...) with E_USER_WARNING and include basename(__FILE__) plus
a concise message (and optional DB error info) and then return false to abort
the upgrade path; apply the same change to the similar check around lines 56-60
so both uses of $GLOBALS['xoopsDB']->query(...) and
$GLOBALS['xoopsDB']->isResultSet(...) are handled the same way.
There was a problem hiding this comment.
Pull request overview
Removes usage of the deprecated queryF() API across XOOPS core/owned code by switching to query() for read statements and exec() for write/DDL, and includes a few targeted runtime-error fixes found via log analysis.
Changes:
- Replace legacy
queryF()calls withquery()/exec()according to statement type (core, modules, installer, upgrade scripts). - Correct several write statements previously executed via
query()to useexec(). - Fix a fatal error in
XoopsUtility::recursive(), remove@suppression in form renderers, and harden a couple of Smarty template accesses.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| upgrade/upd_2.5.10-to-2.5.11/index.php | Use query() for SELECT-based upgrade checks. |
| upgrade/upd-2.5.5-to-2.5.6/index.php | Use query() for SHOW checks; keep upgrade-step semantics (“true = already applied”). |
| upgrade/upd-2.5.4-to-2.5.5/index.php | Use query() for SHOW/SELECT checks; use exec() for ALTER. |
| upgrade/upd-2.4.x-to-2.5.0/index.php | Use query() for SELECT-based upgrade checks. |
| upgrade/upd-2.4.x-to-2.5.0/dbmanager.php | Use exec() for CREATE/DROP; adjust DB manager execution semantics. |
| upgrade/upd-2.3.3-to-2.4.0/index.php | Use query() for SHOW-based key checks. |
| upgrade/upd-2.3.0-to-2.3.1/index.php | Use query() for SHOW COLUMNS checks. |
| upgrade/upd-2.0.18-to-2.3.0/settings_db.php | Use query() for SHOW CHARSET/COLLATION. |
| upgrade/upd-2.0.18-to-2.3.0/index.php | Use query() for read checks; use query() for SHOW INDEX reads. |
| upgrade/upd-2.0.17-to-2.0.18/index.php | Use query() for SHOW COLUMNS read; use exec() in internal mutating query() wrapper. |
| upgrade/upd-2.0.16-to-2.0.17/index.php | Use exec() in internal mutating query() wrapper. |
| upgrade/upd-2.0.15-to-2.0.16/index.php | Use exec() in internal mutating query() wrapper. |
| upgrade/upd-2.0.13-to-2.0.14/index.php | Use exec() in internal mutating query() wrapper. |
| upgrade/cnt-2.2.x-to-2.3.0/index.php | Use query() for SHOW-based checks. |
| htdocs/xoops_lib/modules/protector/class/protector.php | Use exec() for INSERT logging. |
| htdocs/xoops_lib/modules/protector/admin/prefix_manager.php | Use query() for SHOW/SELECT reads. |
| htdocs/xoops_lib/modules/protector/admin/center.php | Use exec() for DELETE statements. |
| htdocs/modules/system/themes/transition/xotpl/xo_globalnav.tpl | Add defaults for optional navigation fields to avoid notices. |
| htdocs/modules/system/themes/dark/xotpl/xo_globalnav.tpl | Add defaults for optional navigation fields to avoid notices. |
| htdocs/modules/system/include/update.php | Use query() for SHOW INDEX reads. |
| htdocs/modules/system/class/maintenance.php | Use query() for SHOW/SELECT reads used by maintenance tasks. |
| htdocs/modules/system/admin/modulesadmin/modulesadmin.php | Use exec() for SET FOREIGN_KEY_CHECKS writes. |
| htdocs/modules/system/admin/avatars/main.php | Use exec() for UPDATE writes. |
| htdocs/modules/pm/include/update.php | Use query() for SHOW COLUMNS reads. |
| htdocs/modules/pm/include/install.php | Use query() for SHOW COLUMNS reads. |
| htdocs/kernel/session.php | Use query() for session SELECT reads (incl. LIMIT/OFFSET). |
| htdocs/kernel/online.php | Use query() for SELECT read in online tracking. |
| htdocs/install/class/dbmanager.php | Use exec() for CREATE DATABASE. |
| htdocs/class/xoopsform/renderer/XoopsFormRendererLegacy.php | Remove @ suppression by safely handling empty encode results. |
| htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap5.php | Remove @ suppression by safely handling empty encode results. |
| htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap4.php | Remove @ suppression by safely handling empty encode results. |
| htdocs/class/xoopsform/renderer/XoopsFormRendererBootstrap3.php | Remove @ suppression by safely handling empty encode results. |
| htdocs/class/utility/xoopsutility.php | Fix recursive array handling to avoid TypeError in array_map(). |
| htdocs/class/database/mysqldatabase.php | Use exec() for SET; proxy now delegates to parent::query() for reads. |
| return $this->queryF($sql, (int)$limit, (int)$start); | ||
| } | ||
| return $this->queryF($sql); | ||
| return parent::query($sql, $limit, $start); |
There was a problem hiding this comment.
With XOOPS_DB_PROXY enabled, XoopsMySQLDatabaseProxy::query() currently blocks any statement that doesn't start with "SELECT" during GET requests. This PR replaces several legacy queryF() calls for SHOW/DESCRIBE/EXPLAIN with query(), so those read-only statements would now be rejected under proxy mode. Consider loosening the proxy guard to allow other read-only verbs (SELECT|WITH|SHOW|DESCRIBE|EXPLAIN) rather than only SELECT, so query() can be used consistently for all read statements.
| foreach ($tables as $key => $val) { | ||
| if (!$this->db->query('DROP TABLE ' . $this->db->prefix($key))) { | ||
| if (!$this->db->exec('DROP TABLE ' . $this->db->prefix($key))) { | ||
| $deleted[] = $ct; |
There was a problem hiding this comment.
In Db_manager::deleteTables(), the failure path appends $ct to $deleted, but $ct is undefined in this scope. This will raise a notice and the returned list will be incorrect. It looks like this should record the current table key/name (e.g., $key or $this->db->prefix($key)) instead.
| $deleted[] = $ct; | |
| $deleted[] = $this->db->prefix($key); |
… PR review Proxy mode: - mysqldatabase.php: widen Proxy::query() allowlist to permit SHOW, DESCRIBE, EXPLAIN (not just SELECT), preventing breakage under XOOPS_DB_PROXY on GET Upgrader correctness: - upd-2.5.5-to-2.5.6: check_com_user/email/url() returned true on query failure, silently suppressing ALTER migrations; now returns false - upd_2.5.10-to-2.5.11: check_notificationmethod() returned true without inspecting the row count; now fetches and checks count > 0 - upd-2.5.4-to-2.5.5: check_keys() continued on SHOW KEYS failure, falling through to return true; now returns false; drop unused $result assignment - upd-2.0.18-to-2.3.0: exec() used for DESCRIBE (needs result set), switched to query(); exec()+isResultSet() mismatch on ALTER DATABASE, use bool check Bug fixes: - avatars/main.php: $user_id was never loaded in delfileok, making the UID-specific UPDATE branch dead code; now loads via Request::getInt(); replace @Unlink with is_file/is_writable guard; escape $file with quote() - protector/center.php: compactlog emitted invalid DELETE...IN() when no duplicates found; guard with !empty($ids) - upd-2.4.x-to-2.5.0/dbmanager.php: deleteTables() referenced undefined $ct; replaced with $this->db->prefix($key) Security (settings_db.php): - Fix malformed <select"> tag (stray quote) - Escape raw $help output to prevent XSS - Replace @$_POST with null coalescing - Replace raw $_SERVER['PHP_SELF'] with escaped SCRIPT_NAME
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
upgrade/upd-2.0.18-to-2.3.0/settings_db.php (1)
116-120:⚠️ Potential issue | 🟠 MajorStop reading request data directly from superglobals.
This still pulls
task,DB_COLLATION, andSCRIPT_NAMEstraight from$_POST/$_SERVER. Please route those values throughXmf\Request::getVar()orXmf\FilterInput::clean()before branching or rendering the form.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: 134-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php` around lines 116 - 120, The code reads request data directly from superglobals ($_SERVER['REQUEST_METHOD'], $_POST['task'], $_POST[$name], and SCRIPT_NAME) which violates input filtering rules; replace these direct accesses by retrieving and cleaning inputs via Xmf\Request::getVar() or Xmf\FilterInput::clean() before branching or assigning to $vars (e.g., use Xmf\Request::getVar('task', '', 'POST') and Xmf\Request::getVar($name, '', 'POST') or the FilterInput equivalent) and use Xmf\Request::getVar('SCRIPT_NAME', '', 'SERVER') instead of $_SERVER—apply the same change for the occurrence around line 134 so all request-derived values are filtered prior to use.
♻️ Duplicate comments (1)
upgrade/upd-2.5.4-to-2.5.5/index.php (1)
29-31:⚠️ Potential issue | 🟠 MajorDon't silently continue after a failed
SHOW KEYS.
check_keys()still hides the probe failure, andapply_keys()still turns a failed schema inspection into a successful upgrade bycontinue-ing past the table. Both branches should warn andreturn falseinstead of treating the failure as normal control flow.Proposed fix
$result = $GLOBALS['xoopsDB']->query($sql); if (!$GLOBALS['xoopsDB']->isResultSet($result)) { - return false; // query failed, surface the issue + trigger_error( + basename(__FILE__) . ": failed to inspect keys for table {$table}: " . $GLOBALS['xoopsDB']->error(), + E_USER_WARNING + ); + return false; }$result = $GLOBALS['xoopsDB']->query($sql); if (!$GLOBALS['xoopsDB']->isResultSet($result)) { - continue; + trigger_error( + basename(__FILE__) . ": failed to inspect keys for table {$table}: " . $GLOBALS['xoopsDB']->error(), + E_USER_WARNING + ); + return false; }As per coding guidelines, "Use trigger_error() with E_USER_WARNING for non-fatal failures and use basename() in error messages to avoid exposing server paths".
Also applies to: 58-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.5.4-to-2.5.5/index.php` around lines 29 - 31, The SHOW KEYS failure in check_keys() and apply_keys() is being swallowed; instead of continuing, change both places where the code currently treats a failed $GLOBALS['xoopsDB']->query($sql) or !$GLOBALS['xoopsDB']->isResultSet($result) as a non-error to (1) call trigger_error("Failed to run SHOW KEYS on " . basename($table) . ": " . $errorMessage, E_USER_WARNING) (capture the DB error text into $errorMessage from the DB API) and (2) return false immediately so the upgrade aborts; update both the check_keys() probe branch and the apply_keys() branch that currently uses continue to use the trigger_error + return false pattern.
🤖 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/system/admin/avatars/main.php`:
- Around line 337-342: The code currently calls $xoopsDB->exec(...) for
user-avatar updates and always treats it as success; modify the branches that
call $xoopsDB->exec (the block that checks $avatar->getVar('avatar_type') and
the else branch using $xoopsDB->quote($file)) to capture and check the exec()
return value, and only proceed to redirect with _AM_SYSTEM_DBUPDATED when the
update succeeded; on failure call trigger_error() with E_USER_WARNING and
include basename($file) (not the full path) in the error message to avoid
exposing server paths, and ensure the deleted avatar cleanup does not leave
users pointing to a deleted file when the UPDATE fails.
- Around line 331-334: The code unconditionally concatenates avatar_file into
$avatarPath and calls unlink($avatarPath), enabling directory traversal; before
unlinking, resolve the target with realpath() and verify it is inside the upload
root (XOOPS_UPLOAD_PATH) or the expected avatars directory: compute $resolved =
realpath($avatarPath) (or realpath(XOOPS_UPLOAD_PATH.'/'.$file_parent) as
needed), ensure $resolved is non-false and starts with
realpath(XOOPS_UPLOAD_PATH) (or the avatars subdirectory path), and only then
call unlink($resolved); if validation fails, skip deletion and log an error.
In `@htdocs/xoops_lib/modules/protector/admin/center.php`:
- Around line 114-115: The three delete paths call $db->exec("DELETE FROM
$log_table WHERE lid='$lid'") (and similar deletes at the other locations) but
ignore its boolean return, so a DB failure still triggers the success redirect
using _AM_MSG_REMOVED; update each delete branch to check the return value of
$db->exec and if it returns false, abort the success redirect and surface the DB
error (e.g., via $db->errorInfo() or $db->errorCode()) or display/log an
appropriate error message instead of redirecting with _AM_MSG_REMOVED; ensure
you modify the branches around the $db->exec calls (the ones referencing
$log_table, $lid and the other two delete call sites) to only redirect on true
and handle false cases by reporting the DB error.
In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php`:
- Line 97: The call to htmlspecialchars uses the default encoding; update the
two occurrences (the one building $field with '<div class="xoform-help">' and
the other at the noted second occurrence) to pass 'UTF-8' explicitly as the
third argument (i.e., htmlspecialchars($help, ENT_QUOTES | ENT_HTML5, 'UTF-8')),
ensuring both calls include the explicit encoding.
In `@upgrade/upd-2.5.5-to-2.5.6/index.php`:
- Around line 33-35: When a SHOW COLUMNS probe fails (the block that calls
$GLOBALS['xoopsDB']->query(...) and checks
!$GLOBALS['xoopsDB']->isResultSet($result) before returning false), emit a
non-fatal warning with trigger_error including basename(__FILE__) and the DB
error string (obtain the error from the DB object, e.g.
$GLOBALS['xoopsDB']->error()) before returning false; apply the same change to
the other identical probe blocks that use $GLOBALS['xoopsDB']->query and
isResultSet so they also call trigger_error(..., E_USER_WARNING) with
basename(__FILE__) and the database error prior to returning.
---
Outside diff comments:
In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php`:
- Around line 116-120: The code reads request data directly from superglobals
($_SERVER['REQUEST_METHOD'], $_POST['task'], $_POST[$name], and SCRIPT_NAME)
which violates input filtering rules; replace these direct accesses by
retrieving and cleaning inputs via Xmf\Request::getVar() or
Xmf\FilterInput::clean() before branching or assigning to $vars (e.g., use
Xmf\Request::getVar('task', '', 'POST') and Xmf\Request::getVar($name, '',
'POST') or the FilterInput equivalent) and use
Xmf\Request::getVar('SCRIPT_NAME', '', 'SERVER') instead of $_SERVER—apply the
same change for the occurrence around line 134 so all request-derived values are
filtered prior to use.
---
Duplicate comments:
In `@upgrade/upd-2.5.4-to-2.5.5/index.php`:
- Around line 29-31: The SHOW KEYS failure in check_keys() and apply_keys() is
being swallowed; instead of continuing, change both places where the code
currently treats a failed $GLOBALS['xoopsDB']->query($sql) or
!$GLOBALS['xoopsDB']->isResultSet($result) as a non-error to (1) call
trigger_error("Failed to run SHOW KEYS on " . basename($table) . ": " .
$errorMessage, E_USER_WARNING) (capture the DB error text into $errorMessage
from the DB API) and (2) return false immediately so the upgrade aborts; update
both the check_keys() probe branch and the apply_keys() branch that currently
uses continue to use the trigger_error + return false pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0413a0be-d1a6-4a49-a565-8f6116b814df
📒 Files selected for processing (9)
htdocs/class/database/mysqldatabase.phphtdocs/modules/system/admin/avatars/main.phphtdocs/xoops_lib/modules/protector/admin/center.phpupgrade/upd-2.0.18-to-2.3.0/index.phpupgrade/upd-2.0.18-to-2.3.0/settings_db.phpupgrade/upd-2.4.x-to-2.5.0/dbmanager.phpupgrade/upd-2.5.4-to-2.5.5/index.phpupgrade/upd-2.5.5-to-2.5.6/index.phpupgrade/upd_2.5.10-to-2.5.11/index.php
| if ($user_id > 0 && $avatar->getVar('avatar_type') === 'C') { | ||
| $xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') | ||
| . " SET user_avatar='blank.gif' WHERE uid=" . $user_id); | ||
| } else { | ||
| $xoopsDB->query('UPDATE ' . $xoopsDB->prefix('users') | ||
| . " SET user_avatar='blank.gif' WHERE user_avatar='" . $file . "'"); | ||
| $xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') | ||
| . " SET user_avatar='blank.gif' WHERE user_avatar=" . $xoopsDB->quote($file)); |
There was a problem hiding this comment.
Check the UPDATE result before reporting success.
Both exec() calls are fire-and-forget. If the user-row update fails, this path still redirects with _AM_SYSTEM_DBUPDATED and leaves users pointing at a deleted avatar.
As per coding guidelines, "Use trigger_error() with E_USER_WARNING for non-fatal failures 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/modules/system/admin/avatars/main.php` around lines 337 - 342, The
code currently calls $xoopsDB->exec(...) for user-avatar updates and always
treats it as success; modify the branches that call $xoopsDB->exec (the block
that checks $avatar->getVar('avatar_type') and the else branch using
$xoopsDB->quote($file)) to capture and check the exec() return value, and only
proceed to redirect with _AM_SYSTEM_DBUPDATED when the update succeeded; on
failure call trigger_error() with E_USER_WARNING and include basename($file)
(not the full path) in the error message to avoid exposing server paths, and
ensure the deleted avatar cleanup does not leave users pointing to a deleted
file when the UPDATE fails.
| $db->exec("DELETE FROM $log_table WHERE lid='$lid'"); | ||
| } |
There was a problem hiding this comment.
Don't redirect with _AM_MSG_REMOVED when exec() fails.
All three delete paths ignore the boolean return from exec(), so a DB failure still shows a success message. Abort the redirect or surface the DB error before reporting removal.
Also applies to: 141-142, 164-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/xoops_lib/modules/protector/admin/center.php` around lines 114 - 115,
The three delete paths call $db->exec("DELETE FROM $log_table WHERE lid='$lid'")
(and similar deletes at the other locations) but ignore its boolean return, so a
DB failure still triggers the success redirect using _AM_MSG_REMOVED; update
each delete branch to check the return value of $db->exec and if it returns
false, abort the success redirect and surface the DB error (e.g., via
$db->errorInfo() or $db->errorCode()) or display/log an appropriate error
message instead of redirecting with _AM_MSG_REMOVED; ensure you modify the
branches around the $db->exec calls (the ones referencing $log_table, $lid and
the other two delete call sites) to only redirect on true and handle false cases
by reporting the DB error.
| $field = "<label for='$name'>$label</label>\n"; | ||
| if ($help) { | ||
| $field .= '<div class="xoform-help">' . $help . "</div>\n"; | ||
| $field .= '<div class="xoform-help">' . htmlspecialchars($help, ENT_QUOTES | ENT_HTML5) . "</div>\n"; |
There was a problem hiding this comment.
Pass an explicit encoding to htmlspecialchars().
Both calls rely on the process default charset. That makes upgrade output dependent on default_charset instead of the repository’s required encoding contract. Pass 'UTF-8' explicitly in both places.
Suggested patch
- $field .= '<div class="xoform-help">' . htmlspecialchars($help, ENT_QUOTES | ENT_HTML5) . "</div>\n";
+ $field .= '<div class="xoform-help">' . htmlspecialchars($help, ENT_QUOTES | ENT_HTML5, 'UTF-8') . "</div>\n";-<form action="<?php echo htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES | ENT_HTML5); ?>" method='post'>
+<form action="<?php echo htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES | ENT_HTML5, 'UTF-8'); ?>" method='post'>As per coding guidelines, "Escape all output with htmlspecialchars($value, ENT_QUOTES, 'UTF-8') or use Smarty auto-escaping".
Also applies to: 134-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php` at line 97, The call to
htmlspecialchars uses the default encoding; update the two occurrences (the one
building $field with '<div class="xoform-help">' and the other at the noted
second occurrence) to pass 'UTF-8' explicitly as the third argument (i.e.,
htmlspecialchars($help, ENT_QUOTES | ENT_HTML5, 'UTF-8')), ensuring both calls
include the explicit encoding.
| $result = $GLOBALS['xoopsDB']->query($sql); | ||
| if (!$GLOBALS['xoopsDB']->isResultSet($result)) { | ||
| throw new \RuntimeException( | ||
| \sprintf(_DB_QUERY_ERROR, $sql) . $GLOBALS['xoopsDB']->error(), | ||
| E_USER_ERROR, | ||
| ); | ||
| return false; // query failed, surface the issue |
There was a problem hiding this comment.
Emit a warning before queueing the ALTER on probe failure.
Returning false here only makes the upgrader treat the column as missing; it does not actually surface why the SHOW COLUMNS probe failed. Call trigger_error(..., E_USER_WARNING) with basename(__FILE__) and the DB error before returning.
As per coding guidelines, "Use trigger_error() with E_USER_WARNING for non-fatal failures and use basename() in error messages to avoid exposing server paths".
Also applies to: 47-49, 61-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upgrade/upd-2.5.5-to-2.5.6/index.php` around lines 33 - 35, When a SHOW
COLUMNS probe fails (the block that calls $GLOBALS['xoopsDB']->query(...) and
checks !$GLOBALS['xoopsDB']->isResultSet($result) before returning false), emit
a non-fatal warning with trigger_error including basename(__FILE__) and the DB
error string (obtain the error from the DB object, e.g.
$GLOBALS['xoopsDB']->error()) before returning false; apply the same change to
the other identical probe blocks that use $GLOBALS['xoopsDB']->query and
isResultSet so they also call trigger_error(..., E_USER_WARNING) with
basename(__FILE__) and the database error prior to returning.
| $sql = 'SHOW COLUMNS FROM ' . $GLOBALS['xoopsDB']->prefix('xoopscomments') . " LIKE 'com_url'"; | ||
| $result = $GLOBALS['xoopsDB']->queryF($sql); | ||
| $result = $GLOBALS['xoopsDB']->query($sql); | ||
| if (!$GLOBALS['xoopsDB']->isResultSet($result)) { | ||
| throw new \RuntimeException( | ||
| \sprintf(_DB_QUERY_ERROR, $sql) . $GLOBALS['xoopsDB']->error(), | ||
| E_USER_ERROR, | ||
| ); | ||
| return false; // query failed, surface the issue | ||
| } |
There was a problem hiding this comment.
Same issue as the other check_* methods in this file: return false on query failure causes the upgrade to attempt an ALTER TABLE rather than surfacing the query error. Consider preserving/logging the DB error or skipping when the schema check can’t be performed.
| $field = "<label for='$name'>$label</label>\n"; | ||
| if ($help) { | ||
| $field .= '<div class="xoform-help">' . $help . "</div>\n"; | ||
| $field .= '<div class="xoform-help">' . htmlspecialchars($help, ENT_QUOTES | ENT_HTML5) . "</div>\n"; | ||
| } | ||
| $field .= "<select name='$name' id='$name'\">"; | ||
| $field .= "<select name='$name' id='$name'>"; |
There was a problem hiding this comment.
htmlspecialchars($help, ENT_QUOTES | ENT_HTML5) relies on default_charset, but this upgrade UI already uses _UPGRADE_CHARSET for other escaping in the same function. To avoid charset mismatches, pass _UPGRADE_CHARSET as the third parameter (or use the same $myts->htmlSpecialChars(..., _UPGRADE_CHARSET, ...) approach).
| } ?> | ||
|
|
||
| <form action="<?php echo $_SERVER['PHP_SELF']; ?>" method='post'> | ||
| <form action="<?php echo htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES | ENT_HTML5); ?>" method='post'> |
There was a problem hiding this comment.
The form action escaping here also omits an explicit encoding and will use default_charset. For consistency with the rest of the upgrade UI (which uses _UPGRADE_CHARSET), pass _UPGRADE_CHARSET as the third argument to htmlspecialchars().
| <form action="<?php echo htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES | ENT_HTML5); ?>" method='post'> | |
| <form action="<?php echo htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES | ENT_HTML5, _UPGRADE_CHARSET); ?>" method='post'> |
| if (is_file($avatarPath) && is_writable($avatarPath)) { | ||
| unlink($avatarPath); |
There was a problem hiding this comment.
The is_writable($avatarPath) guard can prevent deletion even when unlink() would succeed (on Unix, delete permission is determined by the parent directory, not the file mode). Consider attempting unlink() when is_file() is true and handling/logging failures, or checking writability of the parent directory instead.
| if (is_file($avatarPath) && is_writable($avatarPath)) { | |
| unlink($avatarPath); | |
| if (is_file($avatarPath)) { | |
| if (!@unlink($avatarPath)) { | |
| trigger_error( | |
| 'Failed to delete avatar file "' . basename($avatarPath) . '"', | |
| E_USER_WARNING | |
| ); | |
| } |
| $sql = 'SHOW KEYS FROM `' . $GLOBALS['xoopsDB']->prefix($table) . '`'; | ||
| $result = $GLOBALS['xoopsDB']->queryF($sql); | ||
| $result = $GLOBALS['xoopsDB']->query($sql); | ||
| if (!$GLOBALS['xoopsDB']->isResultSet($result)) { | ||
| continue; | ||
| return false; // query failed, surface the issue | ||
| } |
There was a problem hiding this comment.
In check_keys(), returning false when the SHOW KEYS query fails will mark the patch as “needed” and trigger apply_keys(), even though a query failure doesn’t indicate missing keys. Consider restoring the previous behavior (e.g., continue/skip on missing tables) or throwing/logging a DB error explicitly; per XoopsUpgrade::isApplied() semantics, false means “patch needs to be performed”.
| $sql = 'SHOW COLUMNS FROM ' . $GLOBALS['xoopsDB']->prefix('xoopscomments') . " LIKE 'com_user'"; | ||
| $result = $GLOBALS['xoopsDB']->queryF($sql); | ||
| $result = $GLOBALS['xoopsDB']->query($sql); | ||
| if (!$GLOBALS['xoopsDB']->isResultSet($result)) { | ||
| throw new \RuntimeException( | ||
| \sprintf(_DB_QUERY_ERROR, $sql) . $GLOBALS['xoopsDB']->error(), | ||
| E_USER_ERROR, | ||
| ); | ||
| return false; // query failed, surface the issue | ||
| } |
There was a problem hiding this comment.
If the SHOW COLUMNS query fails here, return false will mark this task as “needs to be performed” and then apply_com_user() will attempt an ALTER TABLE that will also fail, but without the DB error context that was previously thrown. Consider either throwing/logging the DB error (so the failure is actionable) or returning true to skip when the table/column can’t be inspected.
| $sql = 'SHOW COLUMNS FROM ' . $GLOBALS['xoopsDB']->prefix('xoopscomments') . " LIKE 'com_email'"; | ||
| $result = $GLOBALS['xoopsDB']->queryF($sql); | ||
| $result = $GLOBALS['xoopsDB']->query($sql); | ||
| if (!$GLOBALS['xoopsDB']->isResultSet($result)) { | ||
| throw new \RuntimeException( | ||
| \sprintf(_DB_QUERY_ERROR, $sql) . $GLOBALS['xoopsDB']->error(), | ||
| E_USER_ERROR, | ||
| ); | ||
| return false; // query failed, surface the issue | ||
| } |
There was a problem hiding this comment.
Same issue as check_com_user(): on query failure, return false will schedule the patch to run and then fail later without preserving the original DB error. Prefer throwing/logging the DB error or skipping when the schema can’t be checked.
- apply_keys() now fails loudly on SHOW KEYS probe failure instead of silently succeeding (upd-2.5.4-to-2.5.5) - check_com_* methods log DB error on failure for actionable diagnostics (upd-2.5.5-to-2.5.6) - Avatar file deletion relaxed from is_writable guard to unlink attempt with trigger_error on failure (avatars/main.php) - FOREIGN_KEY_CHECKS restore calls now log failures (modulesadmin.php) - Replace insecure md5(uniqid(mt_rand())) with bin2hex(random_bytes(4)) for activation key generation (protector.php) - Upgrader settings_db.php: charset-consistent htmlspecialchars, raw $_POST replaced with Xmf\Request - banners.php: XoopsRequest session access, instanceof guards, XSS escaping on image/video URLs - Reverted common.php, pm/profile preloads to master (redundant/harmful)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
upgrade/upd-2.5.5-to-2.5.6/index.php (1)
26-28: 🧹 Nitpick | 🔵 TrivialMissing
@return booltag in docblock.The
check_com_user()method lacks the@return booltag that the other twocheck_com_*methods have. Minor inconsistency in documentation./** * Check if Fast Comment fields already exist * + * `@return` bool */ public function check_com_user()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.5.5-to-2.5.6/index.php` around lines 26 - 28, The docblock for the method check_com_user() is missing the `@return` bool tag; update the PHPDoc above check_com_user() to include "@return bool" so it matches the other check_com_* methods and clearly documents that the function returns a boolean.upgrade/upd-2.5.4-to-2.5.5/index.php (3)
86-96:⚠️ Potential issue | 🟡 MinorReturn type inconsistency: docblock says
boolbutnullis returned.Line 95 returns
nullwhen the query fails, contradicting the@return booldocblock. This creates a type-safety issue:
- If a
boolreturn type hint is added in the future, this will throw aTypeError- Semantically,
null("unknown") differs fromfalse("check passed")PatchStatustreats falsy returns as "needs migration", so a query failure silently schedules theapply_imptotal()task without explicit indication that the check itself failed🛡️ Suggested fix to handle query failure explicitly
public function check_imptotal() { $sql = 'SELECT `imptotal` FROM `' . $GLOBALS['xoopsDB']->prefix('banner') . '` WHERE `bid` = 1'; if ($result = $GLOBALS['xoopsDB']->query($sql)) { $fieldInfo = mysqli_fetch_field_direct($result, 0); + if ($fieldInfo === false) { + $this->logs[] = 'check_imptotal: failed to get field info for imptotal column'; + return false; + } $length = $fieldInfo->length; return ($length != 8); } - return null; + $this->logs[] = 'check_imptotal: query failed for banner table'; + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.5.4-to-2.5.5/index.php` around lines 86 - 96, The method check_imptotal returns null on query failure which contradicts its `@return` bool and can mislead PatchStatus; update check_imptotal() so it always returns a boolean: on successful query keep existing logic using mysqli_fetch_field_direct and return true/false as before, and on query failure return false (or explicitly throw/propagate an exception if you prefer explicit failure handling), ensuring callers like PatchStatus or apply_imptotal() receive a consistent bool result; reference the check_imptotal() function and adjust caller logic if you choose to signal errors differently.
68-75: 🧹 Nitpick | 🔵 TrivialUnused
$resultassignment creates dead local state.Line 71 uses the same pattern (
if (!$result = ...exec($sql))) that was correctly simplified at line 106. For consistency and to eliminate dead code, this assignment should also be removed.♻️ Suggested cleanup
foreach ($keys as $key) { if (!in_array($key, $existing_keys)) { $sql = 'ALTER TABLE `' . $GLOBALS['xoopsDB']->prefix($table) . "` ADD INDEX `{$key}` (`{$key}`)"; - if (!$result = $GLOBALS['xoopsDB']->exec($sql)) { + if (!$GLOBALS['xoopsDB']->exec($sql)) { return false; } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.5.4-to-2.5.5/index.php` around lines 68 - 75, The foreach loop over $keys creates a dead local variable by assigning the exec() return to $result in the conditional (if (!$result = $GLOBALS['xoopsDB']->exec($sql))). Change that conditional to test the exec() call directly (if (!$GLOBALS['xoopsDB']->exec($sql))) and remove the unused $result variable so the code matches the simplified pattern used elsewhere; keep the same $sql construction and early return false behavior on failure.
16-17: 🧹 Nitpick | 🔵 TrivialClass docblock missing per coding guidelines.
The class
Upgrade_255lacks a dedicated docblock with@category,@package,@author,@copyright,@license, and@linktags. While the file-level docblock covers this metadata, the guidelines specify class-level documentation.📝 Suggested class docblock
+/** + * Upgrader from XOOPS 2.5.4 to 2.5.5 + * + * `@category` XoopsUpgrade + * `@package` upgrader + * `@author` Taiwen Jiang <phppp@users.sourceforge.net> + * `@author` trabis <lusopoemas@gmail.com> + * `@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 + */ class Upgrade_255 extends XoopsUpgradeAs per coding guidelines: "Class docblocks must include
@category,@package,@author,@copyright,@license, and@linktags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.5.4-to-2.5.5/index.php` around lines 16 - 17, Add a class-level docblock immediately above the class Upgrade_255 declaration that includes the required tags: `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`; use the same values used in the file-level header (or appropriate project values) and ensure the docblock follows the project's docblock style and formatting conventions so the class Upgrade_255 has its own descriptive metadata.
♻️ Duplicate comments (3)
htdocs/modules/system/admin/avatars/main.php (2)
337-342:⚠️ Potential issue | 🟠 MajorCheck
exec()with=== falsebefore reporting success.Both update branches are still fire-and-forget. If either
exec()fails, this path redirects with_AM_SYSTEM_DBUPDATEDafter the avatar has already been removed. Capture the result, treat onlyfalseas failure, and warn withbasename($file).Proposed fix
- if ($user_id > 0 && $avatar->getVar('avatar_type') === 'C') { - $xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') - . " SET user_avatar='blank.gif' WHERE uid=" . $user_id); - } else { - $xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') - . " SET user_avatar='blank.gif' WHERE user_avatar=" . $xoopsDB->quote($file)); - } + if ($user_id > 0 && $avatar->getVar('avatar_type') === 'C') { + $result = $xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') + . " SET user_avatar='blank.gif' WHERE uid=" . $user_id); + } else { + $result = $xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') + . " SET user_avatar='blank.gif' WHERE user_avatar=" . $xoopsDB->quote($file)); + } + + if ($result === false) { + trigger_error('Failed to reset user avatars for deleted file: ' . basename($file), E_USER_WARNING); + redirect_header('admin.php?fct=avatars', 1, _AM_SYSTEM_DBERROR); + }As per coding guidelines, use
trigger_error()withE_USER_WARNINGfor non-fatal failures and usebasename()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/modules/system/admin/avatars/main.php` around lines 337 - 342, The DB UPDATE calls using $xoopsDB->exec() in the avatar removal branches (when avatar->getVar('avatar_type') === 'C' and the else branch referencing $file) must check the return value for strict === false, assign it to a variable (e.g., $result) and if === false call trigger_error(sprintf("Failed to update user_avatar for %s", basename($file)), E_USER_WARNING) and avoid treating the operation as successful; apply this pattern to both exec() calls so failures are warned with basename($file) and the redirect with _AM_SYSTEM_DBUPDATED is only used when exec() did not return false.
331-334:⚠️ Potential issue | 🟠 MajorValidate the resolved avatar path before unlinking it.
This still deletes a path built from stored data without a
realpath()+ upload-root boundary check, and the warning leaks the full server path. Restrict deletion to the avatars/upload root and only logbasename(...).Proposed fix
- $avatarPath = XOOPS_UPLOAD_PATH . '/' . $file; - if (is_file($avatarPath) && !unlink($avatarPath)) { - trigger_error('Failed to delete avatar file: ' . $avatarPath, E_USER_WARNING); - } + $uploadRoot = realpath(XOOPS_UPLOAD_PATH); + $avatarPath = realpath(XOOPS_UPLOAD_PATH . '/' . $file); + if (false !== $uploadRoot + && false !== $avatarPath + && str_starts_with($avatarPath, $uploadRoot . DIRECTORY_SEPARATOR) + && is_file($avatarPath) + && !unlink($avatarPath) + ) { + trigger_error('Failed to delete avatar file: ' . basename($avatarPath), E_USER_WARNING); + }Based on learnings, validate file paths with
realpath()and boundary checks to prevent directory traversal, and usebasename()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/modules/system/admin/avatars/main.php` around lines 331 - 334, Resolve the computed avatar path with realpath() and verify the resolved path is inside the upload root (resolve XOOPS_UPLOAD_PATH with realpath() too) before calling unlink; only perform unlink($avatarPath) when the realpath begins with the resolved upload-root path to prevent directory traversal, and if deletion fails use trigger_error with basename($file) (not the full server path) to avoid leaking filesystem paths; update the block that sets $avatarPath and calls unlink()/trigger_error accordingly.upgrade/upd-2.0.18-to-2.3.0/settings_db.php (1)
134-134:⚠️ Potential issue | 🟡 MinorPass
'UTF-8'explicitly tohtmlspecialchars().This call is escaped, but it still depends on
default_charset, so the upgrader output can vary by environment.Suggested patch
-<form action="<?php echo htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES | ENT_HTML5); ?>" method='post'> +<form action="<?php echo htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES | ENT_HTML5, 'UTF-8'); ?>" method='post'>As per coding guidelines, "Escape all output with htmlspecialchars($value, ENT_QUOTES, 'UTF-8') or use Smarty auto-escaping".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php` at line 134, The call to htmlspecialchars for $_SERVER['SCRIPT_NAME'] omits an explicit charset; update the call in the form tag to pass 'UTF-8' as the third argument so it reads htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES | ENT_HTML5, 'UTF-8'), ensuring consistent UTF-8 escaping across environments.
🤖 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 200-207: The htmlspecialchars calls for $imageurl in the
banners.php video and image branches omit the explicit charset; update both
calls to include the 'UTF-8' parameter so they read htmlspecialchars($imageurl,
ENT_QUOTES | ENT_HTML5, 'UTF-8') (ensure you update the call inside the video
branch before echoing the <video> source and the subsequent image branch call),
keeping ENT_HTML5 and ENT_QUOTES intact.
In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php`:
- Line 123: Check the result of disabling foreign key checks before proceeding:
replace the bare $db->exec('SET FOREIGN_KEY_CHECKS = 0') call in
modulesadmin.php with a guarded call that either catches exceptions or checks
for a false return and, on failure, logs a warning/error using the same logger
used later around lines 159-161 (use the same logging method/variable) with a
clear message like "Failed to disable foreign key checks" plus the DB
error/exception details, then continue (or handle as appropriate).
In `@htdocs/xoops_lib/modules/protector/class/protector.php`:
- Around line 269-271: Wrap the call to bin2hex(random_bytes(4)) used to set
$actkey in protector.php in a try-catch like the generateSecureToken() pattern
in kernel/member.php: try to generate $actkey with random_bytes(4) and on
Exception catch generate a safe fallback (e.g.,
bin2hex(openssl_random_pseudo_bytes(4)) or a secure fallback using
random_int/uniqid) ensuring the result is 8 hex chars, then call
$xoopsUser->setVar('actkey', $actkey') and userHandler->insert($xoopsUser) as
before.
In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php`:
- Line 116: Replace direct superglobal access by using XMF request helpers:
change the if-condition that checks request method from using
$_SERVER['REQUEST_METHOD'] to Xmf\Request::getMethod() while keeping
Xmf\Request::getString('task', '', 'POST') for the task check, and change the
form action to use Xmf\Request::getVar('SCRIPT_NAME', '', 'server') inside
htmlspecialchars (preserving ENT_QUOTES | ENT_HTML5). Update the occurrences
where the request method and script name are read so all access uses
Xmf\Request::getMethod() and Xmf\Request::getVar('SCRIPT_NAME', '', 'server')
respectively.
---
Outside diff comments:
In `@upgrade/upd-2.5.4-to-2.5.5/index.php`:
- Around line 86-96: The method check_imptotal returns null on query failure
which contradicts its `@return` bool and can mislead PatchStatus; update
check_imptotal() so it always returns a boolean: on successful query keep
existing logic using mysqli_fetch_field_direct and return true/false as before,
and on query failure return false (or explicitly throw/propagate an exception if
you prefer explicit failure handling), ensuring callers like PatchStatus or
apply_imptotal() receive a consistent bool result; reference the
check_imptotal() function and adjust caller logic if you choose to signal errors
differently.
- Around line 68-75: The foreach loop over $keys creates a dead local variable
by assigning the exec() return to $result in the conditional (if (!$result =
$GLOBALS['xoopsDB']->exec($sql))). Change that conditional to test the exec()
call directly (if (!$GLOBALS['xoopsDB']->exec($sql))) and remove the unused
$result variable so the code matches the simplified pattern used elsewhere; keep
the same $sql construction and early return false behavior on failure.
- Around line 16-17: Add a class-level docblock immediately above the class
Upgrade_255 declaration that includes the required tags: `@category`, `@package`,
`@author`, `@copyright`, `@license`, and `@link`; use the same values used in the
file-level header (or appropriate project values) and ensure the docblock
follows the project's docblock style and formatting conventions so the class
Upgrade_255 has its own descriptive metadata.
In `@upgrade/upd-2.5.5-to-2.5.6/index.php`:
- Around line 26-28: The docblock for the method check_com_user() is missing the
`@return` bool tag; update the PHPDoc above check_com_user() to include "@return
bool" so it matches the other check_com_* methods and clearly documents that the
function returns a boolean.
---
Duplicate comments:
In `@htdocs/modules/system/admin/avatars/main.php`:
- Around line 337-342: The DB UPDATE calls using $xoopsDB->exec() in the avatar
removal branches (when avatar->getVar('avatar_type') === 'C' and the else branch
referencing $file) must check the return value for strict === false, assign it
to a variable (e.g., $result) and if === false call
trigger_error(sprintf("Failed to update user_avatar for %s", basename($file)),
E_USER_WARNING) and avoid treating the operation as successful; apply this
pattern to both exec() calls so failures are warned with basename($file) and the
redirect with _AM_SYSTEM_DBUPDATED is only used when exec() did not return
false.
- Around line 331-334: Resolve the computed avatar path with realpath() and
verify the resolved path is inside the upload root (resolve XOOPS_UPLOAD_PATH
with realpath() too) before calling unlink; only perform unlink($avatarPath)
when the realpath begins with the resolved upload-root path to prevent directory
traversal, and if deletion fails use trigger_error with basename($file) (not the
full server path) to avoid leaking filesystem paths; update the block that sets
$avatarPath and calls unlink()/trigger_error accordingly.
In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php`:
- Line 134: The call to htmlspecialchars for $_SERVER['SCRIPT_NAME'] omits an
explicit charset; update the call in the form tag to pass 'UTF-8' as the third
argument so it reads htmlspecialchars($_SERVER['SCRIPT_NAME'], ENT_QUOTES |
ENT_HTML5, 'UTF-8'), ensuring consistent UTF-8 escaping across environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: edc51133-b8ff-4fcb-aefc-21f02b7015df
📒 Files selected for processing (7)
htdocs/banners.phphtdocs/modules/system/admin/avatars/main.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phphtdocs/xoops_lib/modules/protector/class/protector.phpupgrade/upd-2.0.18-to-2.3.0/settings_db.phpupgrade/upd-2.5.4-to-2.5.5/index.phpupgrade/upd-2.5.5-to-2.5.6/index.php
| } | ||
|
|
||
| if ($_SERVER['REQUEST_METHOD'] === 'POST' && @$_POST['task'] === 'db') { | ||
| if ($_SERVER['REQUEST_METHOD'] === 'POST' && Xmf\Request::getString('task', '', 'POST') === 'db') { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect Xmf Request helpers =="
fd '^Request\.php$' -t f | while read -r f; do
echo "-- $f --"
rg -n -C2 "class Request|getString\s*\(|getVar\s*\(|SERVER|_SERVER|REQUEST_METHOD|SCRIPT_NAME" "$f"
done
echo
echo "== Current direct \$_SERVER usages in the reviewed file =="
rg -n -C2 '\$_SERVER\[(.)*(REQUEST_METHOD|SCRIPT_NAME)(.)*\]' upgrade/upd-2.0.18-to-2.3.0/settings_db.phpRepository: XOOPS/XoopsCore25
Length of output: 7397
Replace direct $_SERVER access with XMF request helpers.
Lines 116 and 134 bypass the XMF abstraction layer. Use Xmf\Request::getMethod() for REQUEST_METHOD and Xmf\Request::getVar('SCRIPT_NAME', '', 'server') for SCRIPT_NAME to maintain consistent abstraction and comply with the coding guideline: "Use Xmf\Request, Xmf\Module\Helper, and other XMF utilities instead of direct superglobal access."
Examples
Line 116: if (Xmf\Request::getMethod() === 'POST' && Xmf\Request::getString('task', '', 'POST') === 'db')
Line 134: <form action="<?php echo htmlspecialchars(Xmf\Request::getVar('SCRIPT_NAME', '', 'server'), ENT_QUOTES | ENT_HTML5); ?>" method='post'>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upgrade/upd-2.0.18-to-2.3.0/settings_db.php` at line 116, Replace direct
superglobal access by using XMF request helpers: change the if-condition that
checks request method from using $_SERVER['REQUEST_METHOD'] to
Xmf\Request::getMethod() while keeping Xmf\Request::getString('task', '',
'POST') for the task check, and change the form action to use
Xmf\Request::getVar('SCRIPT_NAME', '', 'server') inside htmlspecialchars
(preserving ENT_QUOTES | ENT_HTML5). Update the occurrences where the request
method and script name are read so all access uses Xmf\Request::getMethod() and
Xmf\Request::getVar('SCRIPT_NAME', '', 'server') respectively.
| */ | ||
|
|
||
| use Xmf\Request; | ||
| use XoopsRequest; |
There was a problem hiding this comment.
The added "use XoopsRequest;" is a non-compound use statement for a global class and will emit "The use statement with non-compound name ... has no effect" in newer PHP versions when warnings are enabled. Since the file is in the global namespace, either remove this use statement or switch to explicitly loading/using the class without importing it.
| use XoopsRequest; |
| $banner_login = XoopsRequest::getString('banner_login', '', 'SESSION'); | ||
| $banner_pass = XoopsRequest::getString('banner_pass', '', 'SESSION'); | ||
| if ($banner_login == '' || $banner_pass == '') { |
There was a problem hiding this comment.
XoopsRequest is used to read session values, but this file never calls XoopsLoad::load('XoopsRequest'). Since core autoloading via XoopsLoad isn't registered globally, this can lead to a fatal "Class 'XoopsRequest' not found" depending on what has already been loaded. Add an explicit XoopsLoad::load('XoopsRequest') (typically right after mainfile.php/common.php is included) or avoid XoopsRequest here.
| { | ||
| $sql = ltrim($sql); | ||
| if (!$this->allowWebChanges && stripos($sql, 'select') !== 0) { | ||
| if (!$this->allowWebChanges && !preg_match('/^\s*(SELECT|WITH|SHOW|DESCRIBE|EXPLAIN)\b/i', $sql)) { |
There was a problem hiding this comment.
The read-only proxy now allows queries starting with "WITH". In MySQL 8+, CTEs can prefix mutating statements (e.g., "WITH ... UPDATE/DELETE"), which would bypass the GET-request write guard and allow database changes through query(). To keep the proxy safely read-only, either remove "WITH" from the allowlist or implement stricter detection that only permits CTEs when the final statement is a SELECT/SHOW/DESCRIBE/EXPLAIN.
| if (!$this->allowWebChanges && !preg_match('/^\s*(SELECT|WITH|SHOW|DESCRIBE|EXPLAIN)\b/i', $sql)) { | |
| if (!$this->allowWebChanges && !preg_match('/^\s*(SELECT|SHOW|DESCRIBE|EXPLAIN)\b/i', $sql)) { |
| if (!$db->exec('SET FOREIGN_KEY_CHECKS = 1')) { | ||
| $errs[] = 'Failed to restore FOREIGN_KEY_CHECKS after table creation'; | ||
| } |
There was a problem hiding this comment.
These new error strings are hard-coded English and will be shown in the module installation/uninstallation output, unlike the surrounding messages which use translated AM_SYSTEM* constants. To keep admin output localizable and consistent, add language constants (and use them here) or reuse an existing translated message pattern.
| $result = $GLOBALS['xoopsDB']->query($sql); | ||
| if (!$GLOBALS['xoopsDB']->isResultSet($result)) { | ||
| throw new \RuntimeException( | ||
| \sprintf(_DB_QUERY_ERROR, $sql) . $GLOBALS['xoopsDB']->error(), | ||
| E_USER_ERROR, | ||
| ); | ||
| $this->logs[] = 'check_com_user: ' . $GLOBALS['xoopsDB']->error(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
In these check_* methods, a database error (isResultSet() false) now returns false, which signals "patch needs to be applied" and will enqueue apply_* to run. This conflates "column missing" with "unable to check" and can lead to apply attempts after a DB failure, producing less clear failures. Consider restoring the previous behavior (throwing an exception) or returning true to explicitly skip the task when the table/column can't be inspected, while still logging the error.
| . " SET user_avatar='blank.gif' WHERE uid=" . (int) $user_id); | ||
| $avatarPath = XOOPS_UPLOAD_PATH . '/' . $file; | ||
| if (is_file($avatarPath) && !unlink($avatarPath)) { | ||
| trigger_error('Failed to delete avatar file: ' . $avatarPath, E_USER_WARNING); |
There was a problem hiding this comment.
trigger_error() message includes the full filesystem path ($avatarPath), which can leak server path details in logs/UI when warnings are displayed. Prefer logging only basename($file) (or a sanitized relative path) in the warning message.
| trigger_error('Failed to delete avatar file: ' . $avatarPath, E_USER_WARNING); | |
| trigger_error('Failed to delete avatar file: ' . basename($avatarPath), E_USER_WARNING); |
| } | ||
| [$cid, $name, $passwd] = $xoopsDB->fetchRow($result); | ||
| if ($_SESSION['banner_pass'] == $passwd) { | ||
| if ($banner_pass == $passwd) { |
There was a problem hiding this comment.
Passwords for banner clients are compared directly in bannerstats() ($banner_pass == $passwd), which means the bannerclient.passwd value is effectively stored in plaintext or another reversible form. If the database or backups are leaked, attackers immediately gain valid client credentials they can reuse on this site or elsewhere. Refactor this flow to store banner client passwords using a strong one-way hashing algorithm (for example via PHP’s password_hash/password_verify) and compare against the hash instead of the raw password.
| if ($banner_pass == $passwd) { | |
| $isValidPassword = false; | |
| if (!empty($passwd) && \password_verify($banner_pass, $passwd)) { | |
| $isValidPassword = true; | |
| // Update hash if current hash needs rehashing | |
| if (\password_needs_rehash($passwd, \PASSWORD_DEFAULT)) { | |
| $newHash = \password_hash($banner_pass, \PASSWORD_DEFAULT); | |
| $updateSql = \sprintf( | |
| 'UPDATE %s SET passwd=%s WHERE cid=%u', | |
| $xoopsDB->prefix('bannerclient'), | |
| $xoopsDB->quote($newHash), | |
| (int) $cid | |
| ); | |
| $xoopsDB->queryF($updateSql); | |
| } | |
| } elseif ($passwd !== '' && \hash_equals((string) $passwd, (string) $banner_pass)) { | |
| // Legacy plaintext password match; migrate to a secure hash | |
| $isValidPassword = true; | |
| $newHash = \password_hash($banner_pass, \PASSWORD_DEFAULT); | |
| $updateSql = \sprintf( | |
| 'UPDATE %s SET passwd=%s WHERE cid=%u', | |
| $xoopsDB->prefix('bannerclient'), | |
| $xoopsDB->quote($newHash), | |
| (int) $cid | |
| ); | |
| $xoopsDB->queryF($updateSql); | |
| } | |
| if ($isValidPassword) { |
| if (($xoopsDB->isResultSet($result2) && $result2 instanceof \mysqli_result)) { | ||
| [$name, $email, $passwd] = $xoopsDB->fetchRow($result2); | ||
| if ($_SESSION['banner_pass'] == $passwd) { | ||
| if ($banner_pass == $passwd) { |
There was a problem hiding this comment.
The same plaintext password pattern appears in emailStats() ($banner_pass == $passwd), again implying that bannerclient.passwd contains unhashed passwords. A database compromise would expose every client’s actual password and allow attackers to access banner stats and any other functionality protected by these credentials. Use a strong password hashing scheme (e.g., password_hash/password_verify) here as well, so only hashes are stored and compared.
| if ($banner_pass == $passwd) { | |
| if ( | |
| (is_string($passwd) && password_verify($banner_pass, $passwd)) | |
| || (is_string($passwd) && hash_equals($passwd, $banner_pass)) | |
| ) { |
| if (($xoopsDB->isResultSet($result) && $result instanceof \mysqli_result)) { | ||
| [$passwd] = $xoopsDB->fetchRow($result); | ||
| if ($_SESSION['banner_pass'] == $passwd) { | ||
| if ($banner_pass == $passwd) { |
There was a problem hiding this comment.
In change_banner_url_by_client() the client password from the session is again compared directly to the database value ($banner_pass == $passwd), confirming that banner client passwords are not hashed. Anyone who obtains a copy of the database gains immediate access to all client accounts and can alter banner destinations or abuse stats. Switch this code to verify the supplied password against a stored hash (using password_verify or similar) and ensure that new or updated passwords are hashed before being saved.
| if ($banner_pass == $passwd) { | |
| $storedPass = (string)$passwd; | |
| $isValid = false; | |
| // First try verifying as a modern password hash | |
| if ($storedPass !== '' && password_verify($banner_pass, $storedPass)) { | |
| $isValid = true; | |
| // Rehash if the current hash needs upgrading | |
| if (password_needs_rehash($storedPass, PASSWORD_DEFAULT)) { | |
| $newHash = password_hash($banner_pass, PASSWORD_DEFAULT); | |
| $updateSql = sprintf( | |
| 'UPDATE %s SET passwd=%s WHERE cid=%u AND login=%s', | |
| $xoopsDB->prefix('bannerclient'), | |
| $xoopsDB->quote($newHash), | |
| $cid, | |
| $xoopsDB->quote($banner_login) | |
| ); | |
| $xoopsDB->exec($updateSql); | |
| } | |
| } else { | |
| // Fallback: legacy plaintext password stored in DB, migrate to hash on successful match | |
| $looksHashed = str_starts_with($storedPass, '$2y$') || str_starts_with($storedPass, '$argon2'); | |
| if (!$looksHashed && $storedPass !== '' && hash_equals($storedPass, $banner_pass)) { | |
| $isValid = true; | |
| $newHash = password_hash($banner_pass, PASSWORD_DEFAULT); | |
| $updateSql = sprintf( | |
| 'UPDATE %s SET passwd=%s WHERE cid=%u AND login=%s', | |
| $xoopsDB->prefix('bannerclient'), | |
| $xoopsDB->quote($newHash), | |
| $cid, | |
| $xoopsDB->quote($banner_login) | |
| ); | |
| $xoopsDB->exec($updateSql); | |
| } | |
| } | |
| if ($isValid) { |
- banners.php: explicit UTF-8 charset in htmlspecialchars for image URLs - settings_db.php: REQUEST_METHOD via Xmf\Request::getMethod(), charset- consistent htmlspecialchars on SCRIPT_NAME - avatars/main.php: use basename() in error message to avoid exposing full server path - modulesadmin.php: log failure of initial SET FOREIGN_KEY_CHECKS = 0 - protector.php: try/catch around random_bytes() with uniqid fallback
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
htdocs/modules/system/admin/avatars/main.php (1)
338-349:⚠️ Potential issue | 🟠 MajorDon’t delete the avatar before the user-row reset succeeds.
The warning is better than a silent failure, but by the time this block runs the avatar row and file are already gone. If either
exec()fails here, affected users still reference a deleted avatar and Line 351 still reports_AM_SYSTEM_DBUPDATED. Move the user reset ahead ofdelete()/unlink(), or abort the delete path when the reset fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/avatars/main.php` around lines 338 - 349, The avatar file/row is being deleted before confirming the DB UPDATE that resets users' user_avatar, so failures leave users pointing at a removed avatar; modify the flow in the avatar deletion handler to perform the user reset (the $xoopsDB->exec UPDATE that sets user_avatar='blank.gif' using Request::getInt('user_id') or matching file via $xoopsDB->quote($file)) before calling $avatar->delete() and unlink(), and if that exec() returns false abort the delete path (log/trigger_error and skip $avatar->delete() and unlink()), ensuring any failure prevents removal of the avatar row/file.
🤖 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/system/admin/avatars/main.php`:
- Around line 331-333: The current traversal guard uses $uploadRoot =
realpath(XOOPS_UPLOAD_PATH) . DIRECTORY_SEPARATOR which becomes just
DIRECTORY_SEPARATOR if realpath fails, collapsing the check; update the logic
around $avatarPath and $uploadRoot so you first compute $uploadRoot =
realpath(XOOPS_UPLOAD_PATH) and explicitly reject the delete (return/skip/error)
if $uploadRoot === false, then append DIRECTORY_SEPARATOR and perform the
existing check comparing $avatarPath and $uploadRoot to ensure a valid upload
root before allowing deletion (references: variables $avatarPath, $uploadRoot
and realpath(XOOPS_UPLOAD_PATH)).
---
Duplicate comments:
In `@htdocs/modules/system/admin/avatars/main.php`:
- Around line 338-349: The avatar file/row is being deleted before confirming
the DB UPDATE that resets users' user_avatar, so failures leave users pointing
at a removed avatar; modify the flow in the avatar deletion handler to perform
the user reset (the $xoopsDB->exec UPDATE that sets user_avatar='blank.gif'
using Request::getInt('user_id') or matching file via $xoopsDB->quote($file))
before calling $avatar->delete() and unlink(), and if that exec() returns false
abort the delete path (log/trigger_error and skip $avatar->delete() and
unlink()), ensuring any failure prevents removal of the avatar row/file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: df307f86-bb58-412f-93b5-637398337dc1
📒 Files selected for processing (4)
htdocs/install/class/dbmanager.phphtdocs/modules/system/admin/avatars/main.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phpupgrade/upd-2.4.x-to-2.5.0/dbmanager.php
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
upgrade/upd-2.5.4-to-2.5.5/index.php:96
- check_imptotal() returns null when the SELECT fails. Per XoopsUpgrade::isApplied()/PatchStatus, any non-true return value is treated as “patch needed”, so null will trigger apply_imptotal() even on query errors (e.g., missing banner table) and can cause upgrade failure. Return a boolean consistently (e.g., log the DB error and return true to skip, or return false only when you positively detect the schema needs changing).
- Check realpath(XOOPS_UPLOAD_PATH) !== false before using it in the boundary comparison, so a failed resolve skips the delete safely - Add explicit parentheses around instanceof checks in banners.php to silence static analysis warnings about operator precedence
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 (2)
htdocs/banners.php (2)
216-216:⚠️ Potential issue | 🟡 MinorMissing explicit charset in
htmlspecialchars()call for$clickurl.This
htmlspecialchars()call escapes$clickurlbut omits the'UTF-8'charset parameter.🔧 Suggested fix
- $clickurl = htmlspecialchars($clickurl, ENT_QUOTES | ENT_HTML5); + $clickurl = htmlspecialchars($clickurl, ENT_QUOTES | ENT_HTML5, 'UTF-8');As per coding guidelines: "Escape all output with htmlspecialchars($value, ENT_QUOTES, 'UTF-8')".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` at line 216, The htmlspecialchars call escaping $clickurl is missing an explicit charset; update the call that references $clickurl (the htmlspecialchars invocation) to include the 'UTF-8' third argument while preserving the current flags (ENT_QUOTES | ENT_HTML5) so it becomes htmlspecialchars(..., ENT_QUOTES | ENT_HTML5, 'UTF-8') to comply with the escaping guideline.
178-178:⚠️ Potential issue | 🟡 MinorMissing explicit charset in
htmlspecialchars()call.Line 178 escapes
$xoopsConfig['sitename']without the'UTF-8'charset parameter. Per coding guidelines, allhtmlspecialchars()calls should include the charset explicitly.🔧 Suggested fix
- echo "</table> - <br><br> - <h4 class='content_title'>" . _BANNERS_FOW_IN . htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5) . '</h4><hr />'; + echo "</table> + <br><br> + <h4 class='content_title'>" . _BANNERS_FOW_IN . htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5, 'UTF-8') . '</h4><hr />';As per coding guidelines: "Escape all output with htmlspecialchars($value, ENT_QUOTES, 'UTF-8') or use Smarty auto-escaping".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` at line 178, Update the htmlspecialchars call that escapes $xoopsConfig['sitename'] to pass an explicit charset; replace the current call htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5) with htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5, 'UTF-8') so the charset is explicitly provided while preserving existing flags in banners.php around the content_title heading.
🤖 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/system/admin/avatars/main.php`:
- Around line 333-336: Merge the nested checks by combining the
file-existence/unlink logic into the outer condition and replace
strpos($avatarPath, $uploadRoot . DIRECTORY_SEPARATOR) === 0 with
str_starts_with($avatarPath, $uploadRoot . DIRECTORY_SEPARATOR) for PHP 8+, so
the single if checks ($uploadRoot !== false && $avatarPath !== false &&
str_starts_with($avatarPath, $uploadRoot . DIRECTORY_SEPARATOR) &&
is_file($avatarPath)) and then attempts unlink($avatarPath) and calls
trigger_error('Failed to delete avatar file: ' . basename($avatarPath),
E_USER_WARNING) if unlink fails; update the block around $uploadRoot,
$avatarPath, is_file and unlink accordingly.
---
Outside diff comments:
In `@htdocs/banners.php`:
- Line 216: The htmlspecialchars call escaping $clickurl is missing an explicit
charset; update the call that references $clickurl (the htmlspecialchars
invocation) to include the 'UTF-8' third argument while preserving the current
flags (ENT_QUOTES | ENT_HTML5) so it becomes htmlspecialchars(..., ENT_QUOTES |
ENT_HTML5, 'UTF-8') to comply with the escaping guideline.
- Line 178: Update the htmlspecialchars call that escapes
$xoopsConfig['sitename'] to pass an explicit charset; replace the current call
htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5) with
htmlspecialchars($xoopsConfig['sitename'], ENT_QUOTES | ENT_HTML5, 'UTF-8') so
the charset is explicitly provided while preserving existing flags in
banners.php around the content_title heading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad2e7cfd-616b-4f64-820e-cc4cc1d3032f
📒 Files selected for processing (2)
htdocs/banners.phphtdocs/modules/system/admin/avatars/main.php
| $sql = ltrim($sql); | ||
| if (!$this->allowWebChanges && stripos($sql, 'select') !== 0) { | ||
| if (!$this->allowWebChanges && !preg_match('/^\s*(SELECT|SHOW|DESCRIBE|EXPLAIN)\b/i', $sql)) { | ||
| trigger_error('Database updates are not allowed during processing of a GET request', E_USER_WARNING); |
There was a problem hiding this comment.
The XoopsMySQLDatabaseProxy::query() docblock says it "allows only SELECT queries", but the new guard permits SHOW/DESCRIBE/EXPLAIN as well. Please update the docblock (and/or comment text) to match the actual accepted read-only statements so the behavior is clear for callers.
- Reset user avatars before deleting avatar record and file, so users are not left pointing at a deleted avatar if the reset fails - Use str_starts_with() and merge nested ifs in avatar path validation - Update proxy class and method docblocks to list all allowed read-only statements (SELECT, SHOW, DESCRIBE, EXPLAIN) - Add explicit UTF-8 charset to remaining htmlspecialchars in banners.php
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/class/database/mysqldatabase.php (1)
613-623:⚠️ Potential issue | 🟡 MinorComplete the proxy class docblock tags.
This edited class docblock still omits required tags such as
@category,@license, and@link, so it does not meet the repository’s PHP docblock standard.As per coding guidelines, "Class docblocks must include
@category,@package,@author,@copyright,@license, and@linktags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/database/mysqldatabase.php` around lines 613 - 623, The class docblock in mysqldatabase.php is missing required tags; update the class header docblock for the MySQL database proxy class to include `@category`, `@license`, and `@link` (in addition to the existing tags). Add a suitable `@category` (e.g., "Xoops" or "Xoops Core"), a `@license` matching the project (e.g., "GNU GPL 2.0 or later" or the repo's canonical license identifier), and a `@link` pointing to the project or package homepage (e.g., https://xoops.org or the module's docs), ensuring the docblock above the MySQL read-only class (the class in mysqldatabase.php) contains `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`.
♻️ Duplicate comments (4)
htdocs/banners.php (4)
108-109:⚠️ Potential issue | 🟠 MajorReplace these interpolated SQL strings with prepared statements.
$xoopsDB->quote()plussprintf()is still string-built SQL. Please switch these reads and writes to the handler's prepared-statement API so this file matches the repository rule consistently.#!/bin/bash set -eu # Verify the current call sites are still assembling SQL as strings. sed -n '103,110p' htdocs/banners.php sed -n '281,360p' htdocs/banners.php # Verify the repository has handler-level prepared statement usage to mirror. rg -n -C2 '\->prepare\s*\(|bindValue\s*\(' htdocs --type phpAs per coding guidelines: "Use parameterized queries via XOOPS database handlers — never concatenate user input into SQL".
Also applies to: 283-284, 350-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` around lines 108 - 109, Replace string-built SQL that uses sprintf and $xoopsDB->quote (e.g., the SELECT in banners.php that sets $sql and calls $xoopsDB->query) with XOOPS handler prepared statements: call $xoopsDB->prepare() with a parameterized query, bind the user input (e.g., $banner_login) via bindValue/bindParam, execute the statement and fetch results from the statement instead of using $xoopsDB->query(); apply the same change pattern to the other occurrences around the regions noted (uses at 283-284 and 350-360) so all SELECT/INSERT/UPDATE/DELETE use prepare + bind + execute rather than sprintf/quote + query.
302-302:⚠️ Potential issue | 🟡 MinorRemove the unused
$dateslot from destructuring.It is never read after Line 302, so this is just dead assignment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` at line 302, Remove the unused $date slot from the array destructuring of $row in banners.php: update the destructuring that currently includes $date (the line assigning [$bid, $imptotal, $impmade, $clicks, $imageurl, $clickurl, $date] = $row) so it no longer binds $date (leave only the variables actually used: $bid, $imptotal, $impmade, $clicks, $imageurl, $clickurl); ensure the remaining order matches $row's columns so no data shifts occur.
103-105:⚠️ Potential issue | 🟠 MajorStop relying on a raw password persisted in session.
These reads still make the banner flow depend on
$_SESSION['banner_pass']holding the reusable client credential across requests. A stolen session becomes a password leak. Persist only an authenticated client identifier/flag and reload the client record server-side.Also applies to: 278-280, 345-347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` around lines 103 - 105, Stop storing or reading raw credentials from session: remove usages of $_SESSION['banner_pass'] and the local $banner_pass/$banner_login checks and instead persist only an authenticated banner client identifier or boolean flag (e.g., $_SESSION['banner_client_id'] or $_SESSION['banner_authenticated']). On request, load the banner client record server‑side via a lookup (e.g., "load banner client by id" function) and validate the session against that record rather than using the raw password; update all code paths that reference $banner_pass/$banner_login to use the new identifier/flag and server-side lookup/validation, and ensure session creation code saves only the identifier/flag (never the plaintext password).
285-285: 🧹 Nitpick | 🔵 TrivialDrop the outer parentheses around these
ifconditions.They are redundant and keep Sonar noisy.
Also applies to: 297-297, 352-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` at line 285, Remove the redundant outer parentheses around the if conditions that wrap the whole expression; specifically update the if that currently reads with the outer parentheses around "$xoopsDB->isResultSet($result2) && $result2 instanceof \mysqli_result" so it simply tests $xoopsDB->isResultSet($result2) && $result2 instanceof \mysqli_result (leave the inner parentheses for function calls/ instanceof as-is). Repeat the same change for the other two occurrences noted (the ifs at the same pattern on the other lines), ensuring you only drop the unnecessary outer parentheses and do not alter the calls to $xoopsDB->isResultSet(...) or the instanceof checks.
🤖 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`:
- Line 121: Replace plaintext password storage and comparisons with PHP's
password hashing API: when saving banner client passwords (the code path that
currently uses Request::getString() to write $passwd), call
password_hash($passwd, PASSWORD_DEFAULT) and persist the hash instead of raw
text; when authenticating (the locations currently using hash_equals((string)
$passwd, $banner_pass) and similar comparisons), replace the equality checks
with password_verify($passwd, $banner_pass) (remove hash_equals there). Also
update any admin form rendering that outputs the stored password as unescaped
text to never render the hash as a plaintext credential (show a blank field or a
placeholder and only update the hash if a new password is submitted). Ensure you
keep variable names like $passwd and $banner_pass so changes are local to those
functions.
In `@htdocs/modules/system/admin/avatars/main.php`:
- Around line 341-346: The long combined if condition in the avatar deletion
block makes the code hard to read; refactor the condition around the realpath
and deletion checks into a multi-line, PSR-12-compliant form by assigning
intermediate boolean expressions or breaking the if() expression across lines.
Specifically, keep the same logic using the existing symbols ($avatarPath,
$uploadRoot, realpath(), str_starts_with(), is_file(), unlink()), for example
compute $isInUpload = $uploadRoot !== false && $avatarPath !== false &&
str_starts_with($avatarPath, $uploadRoot . DIRECTORY_SEPARATOR) and then use if
($isInUpload && is_file($avatarPath) && !unlink($avatarPath)) {
trigger_error(...); } to improve readability while preserving behavior and the
use of basename() in the error.
---
Outside diff comments:
In `@htdocs/class/database/mysqldatabase.php`:
- Around line 613-623: The class docblock in mysqldatabase.php is missing
required tags; update the class header docblock for the MySQL database proxy
class to include `@category`, `@license`, and `@link` (in addition to the existing
tags). Add a suitable `@category` (e.g., "Xoops" or "Xoops Core"), a `@license`
matching the project (e.g., "GNU GPL 2.0 or later" or the repo's canonical
license identifier), and a `@link` pointing to the project or package homepage
(e.g., https://xoops.org or the module's docs), ensuring the docblock above the
MySQL read-only class (the class in mysqldatabase.php) contains `@category`,
`@package`, `@author`, `@copyright`, `@license`, and `@link`.
---
Duplicate comments:
In `@htdocs/banners.php`:
- Around line 108-109: Replace string-built SQL that uses sprintf and
$xoopsDB->quote (e.g., the SELECT in banners.php that sets $sql and calls
$xoopsDB->query) with XOOPS handler prepared statements: call
$xoopsDB->prepare() with a parameterized query, bind the user input (e.g.,
$banner_login) via bindValue/bindParam, execute the statement and fetch results
from the statement instead of using $xoopsDB->query(); apply the same change
pattern to the other occurrences around the regions noted (uses at 283-284 and
350-360) so all SELECT/INSERT/UPDATE/DELETE use prepare + bind + execute rather
than sprintf/quote + query.
- Line 302: Remove the unused $date slot from the array destructuring of $row in
banners.php: update the destructuring that currently includes $date (the line
assigning [$bid, $imptotal, $impmade, $clicks, $imageurl, $clickurl, $date] =
$row) so it no longer binds $date (leave only the variables actually used: $bid,
$imptotal, $impmade, $clicks, $imageurl, $clickurl); ensure the remaining order
matches $row's columns so no data shifts occur.
- Around line 103-105: Stop storing or reading raw credentials from session:
remove usages of $_SESSION['banner_pass'] and the local
$banner_pass/$banner_login checks and instead persist only an authenticated
banner client identifier or boolean flag (e.g., $_SESSION['banner_client_id'] or
$_SESSION['banner_authenticated']). On request, load the banner client record
server‑side via a lookup (e.g., "load banner client by id" function) and
validate the session against that record rather than using the raw password;
update all code paths that reference $banner_pass/$banner_login to use the new
identifier/flag and server-side lookup/validation, and ensure session creation
code saves only the identifier/flag (never the plaintext password).
- Line 285: Remove the redundant outer parentheses around the if conditions that
wrap the whole expression; specifically update the if that currently reads with
the outer parentheses around "$xoopsDB->isResultSet($result2) && $result2
instanceof \mysqli_result" so it simply tests $xoopsDB->isResultSet($result2) &&
$result2 instanceof \mysqli_result (leave the inner parentheses for function
calls/ instanceof as-is). Repeat the same change for the other two occurrences
noted (the ifs at the same pattern on the other lines), ensuring you only drop
the unnecessary outer parentheses and do not alter the calls to
$xoopsDB->isResultSet(...) or the instanceof checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e23fe9b-0a33-44a2-b40c-77d94f06c1a1
📒 Files selected for processing (3)
htdocs/banners.phphtdocs/class/database/mysqldatabase.phphtdocs/modules/system/admin/avatars/main.php
| redirect_header('banners.php', 2, _BANNERS_NO_LOGIN_DATA); | ||
| } | ||
| [$cid, $name, $passwd] = $row; | ||
| if (hash_equals((string) $passwd, $banner_pass)) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
# Verify banner client passwords are still written and rendered as raw strings.
sed -n '188,194p' htdocs/modules/system/admin/banners/main.php
sed -n '76,79p' htdocs/modules/system/class/bannerclient.php
# Verify this file still compares the raw passwd field directly.
rg -n -C1 'hash_equals|passwd' \
htdocs/banners.php \
htdocs/modules/system/admin/banners/main.php \
htdocs/modules/system/class/bannerclient.phpRepository: XOOPS/XoopsCore25
Length of output: 3818
Migrate banner client passwords from plaintext storage to hashed passwords.
Banner client passwords are stored and compared as plaintext across three locations (lines 121, 291, 358). The database stores passwords as XOBJ_DTYPE_TXTBOX, writes them directly via Request::getString() in the admin form, and renders them back as unescaped text. A database breach immediately exposes all banner credentials.
Replace plaintext comparisons with password_hash() on write and password_verify() on authentication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/banners.php` at line 121, Replace plaintext password storage and
comparisons with PHP's password hashing API: when saving banner client passwords
(the code path that currently uses Request::getString() to write $passwd), call
password_hash($passwd, PASSWORD_DEFAULT) and persist the hash instead of raw
text; when authenticating (the locations currently using hash_equals((string)
$passwd, $banner_pass) and similar comparisons), replace the equality checks
with password_verify($passwd, $banner_pass) (remove hash_equals there). Also
update any admin form rendering that outputs the stored password as unescaped
text to never render the hash as a plaintext credential (show a blank field or a
placeholder and only update the hash if a new password is submitted). Ensure you
keep variable names like $passwd and $banner_pass so changes are local to those
functions.
| $file = $avatar->getVar('avatar_file'); | ||
| // Reset user profiles first — before deleting the avatar record or file | ||
| $user_id = Request::getInt('user_id', 0, 'POST'); | ||
| if ($user_id > 0 && $avatar->getVar('avatar_type') === 'C') { | ||
| if (!$xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') |
There was a problem hiding this comment.
$file is retrieved via $avatar->getVar('avatar_file') without a format, which defaults to the 'show' format and HTML-escapes TXTBOX values. That can break the subsequent SQL update (user_avatar comparison) and filesystem lookup/unlink. Fetch the raw value (e.g., format 'n' / none) for DB and path operations, and reserve the escaped 's' format for HTML output only.
- Use getVar('avatar_file', 'n') for raw value to avoid HTML-escaped
filenames breaking SQL comparisons and filesystem operations
- Abort avatar deletion when user profile reset fails instead of
continuing with a warning, preventing orphaned avatar references
- Clarify proxy class docblock: GET-request write protection via
query(), not truly read-only since exec() bypasses the guard
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/class/database/mysqldatabase.php (1)
613-624:⚠️ Potential issue | 🟡 MinorComplete the proxy class docblock metadata.
This class docblock still omits
@category,@license, and@link, so it does not meet the repository PHPDoc contract for classes.Suggested docblock fix
/** * Proxy connection to a MySQL database with GET-request write protection. * * During GET requests (when allowWebChanges is false), this class restricts * {`@link` query()} to read-only statements (SELECT, SHOW, DESCRIBE, EXPLAIN). * Write operations should use {`@link` exec()} which is not subject to this guard. * + * `@category` Database * `@author` Kazumi Ono <onokazu@xoops.org> * `@copyright` (c) 2000-2026 XOOPS Project (https://xoops.org) + * `@license` GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html) + * `@link` https://xoops.org * `@package` class * `@subpackage` database */As per coding guidelines, "Class docblocks must include
@category,@package,@author,@copyright,@license, and@linktags".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/database/mysqldatabase.php` around lines 613 - 624, The class docblock above the MySQL proxy connection (the proxy connection to a MySQL database class) is missing required PHPDoc tags; update that docblock to include `@category` (e.g., "Core" or "Database"), `@license` (with the SPDX identifier or full license text, e.g., "GNU GPL v2 or later" or appropriate license), and `@link` (project URL, e.g., "https://xoops.org"), keeping the existing `@package`, `@subpackage`, `@author`, and `@copyright` tags intact; ensure the new tags appear in the same docblock that documents the proxy connection class so it satisfies the repository PHPDoc contract.
♻️ Duplicate comments (2)
htdocs/modules/system/admin/avatars/main.php (1)
345-350: 🧹 Nitpick | 🔵 TrivialPath validation is robust, but consider PSR-12 line length.
The traversal guard is well-implemented:
realpath()resolves symlinks and normalizes paths$uploadRoot !== falsecheck prevents the collapsed-guard vulnerabilitystr_starts_with()is clean PHP 8.0+ syntaxbasename()in the error message avoids exposing server pathsHowever, line 348 (~165 characters) exceeds PSR-12 recommended line length. Consider splitting for readability:
♻️ Optional PSR-12 line-length fix
- if ($uploadRoot !== false && $avatarPath !== false && str_starts_with($avatarPath, $uploadRoot . DIRECTORY_SEPARATOR) && is_file($avatarPath) && !unlink($avatarPath)) { + if ($uploadRoot !== false + && $avatarPath !== false + && str_starts_with($avatarPath, $uploadRoot . DIRECTORY_SEPARATOR) + && is_file($avatarPath) + && !unlink($avatarPath) + ) { trigger_error('Failed to delete avatar file: ' . basename($avatarPath), E_USER_WARNING); }As per coding guidelines,
**/*.php: Follow PSR-12 coding standard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/avatars/main.php` around lines 345 - 350, The long conditional that validates and deletes the avatar file (using realpath, $avatarPath, $uploadRoot, str_starts_with, is_file, unlink and trigger_error with basename) exceeds PSR-12 line length; refactor by breaking the compound if into multiple shorter statements or named boolean variables (e.g., $isInUploadRoot, $isFile, $deleted = unlink(...)) and then test them in a shorter if, preserving the same checks and the trigger_error call when unlink fails.htdocs/banners.php (1)
285-285: 🧹 Nitpick | 🔵 TrivialRedundant outer parentheses still present.
SonarCloud correctly identifies the extra parentheses wrapping the
ifcondition. The previous review noted this as addressed, but the parentheses remain.- if (($xoopsDB->isResultSet($result2) && $result2 instanceof \mysqli_result)) { + if ($xoopsDB->isResultSet($result2) && $result2 instanceof \mysqli_result) {Same applies to lines 297 and 352.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` at line 285, Remove the redundant outer parentheses around the if conditions in htdocs/banners.php: change constructs like if (($xoopsDB->isResultSet($result2) && $result2 instanceof \mysqli_result)) to if ($xoopsDB->isResultSet($result2) && $result2 instanceof \mysqli_result); apply the same removal for the analogous conditions at the occurrences referenced (lines with $result2 and the other at line 352) so the if statements no longer have an extra set of surrounding parentheses.
🤖 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/class/database/mysqldatabase.php`:
- Line 630: Update the docblock for the query() method to accurately describe
its guard: state that read-only enforcement (SELECT, SHOW, DESCRIBE, EXPLAIN)
only applies when allowWebChanges is false (i.e., for web requests where changes
are disallowed), and that when allowWebChanges is true or for non-GET/allowed
requests query() will defer to parent::query() and may execute non-read
statements; reference the query() method and the allowWebChanges check and
parent::query() behavior in the comment.
---
Outside diff comments:
In `@htdocs/class/database/mysqldatabase.php`:
- Around line 613-624: The class docblock above the MySQL proxy connection (the
proxy connection to a MySQL database class) is missing required PHPDoc tags;
update that docblock to include `@category` (e.g., "Core" or "Database"), `@license`
(with the SPDX identifier or full license text, e.g., "GNU GPL v2 or later" or
appropriate license), and `@link` (project URL, e.g., "https://xoops.org"),
keeping the existing `@package`, `@subpackage`, `@author`, and `@copyright` tags intact;
ensure the new tags appear in the same docblock that documents the proxy
connection class so it satisfies the repository PHPDoc contract.
---
Duplicate comments:
In `@htdocs/banners.php`:
- Line 285: Remove the redundant outer parentheses around the if conditions in
htdocs/banners.php: change constructs like if (($xoopsDB->isResultSet($result2)
&& $result2 instanceof \mysqli_result)) to if ($xoopsDB->isResultSet($result2)
&& $result2 instanceof \mysqli_result); apply the same removal for the analogous
conditions at the occurrences referenced (lines with $result2 and the other at
line 352) so the if statements no longer have an extra set of surrounding
parentheses.
In `@htdocs/modules/system/admin/avatars/main.php`:
- Around line 345-350: The long conditional that validates and deletes the
avatar file (using realpath, $avatarPath, $uploadRoot, str_starts_with, is_file,
unlink and trigger_error with basename) exceeds PSR-12 line length; refactor by
breaking the compound if into multiple shorter statements or named boolean
variables (e.g., $isInUploadRoot, $isFile, $deleted = unlink(...)) and then test
them in a shorter if, preserving the same checks and the trigger_error call when
unlink fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5800ea78-8dfc-4191-a319-d45f611a8bdf
📒 Files selected for processing (3)
htdocs/banners.phphtdocs/class/database/mysqldatabase.phphtdocs/modules/system/admin/avatars/main.php
| $sql = ltrim($sql); | ||
| if (!$this->allowWebChanges && stripos($sql, 'select') !== 0) { | ||
| if (!$this->allowWebChanges && !preg_match('/^\s*(SELECT|SHOW|DESCRIBE|EXPLAIN)\b/i', $sql)) { | ||
| trigger_error('Database updates are not allowed during processing of a GET request', E_USER_WARNING); | ||
|
|
||
| return false; | ||
| } | ||
| // Execute via queryF() to preserve legacy path (and LIMIT semantics) | ||
| if ($limit !== null) { | ||
| $start = max(0, $start ?? 0); | ||
| return $this->queryF($sql, (int)$limit, (int)$start); | ||
| } | ||
| return $this->queryF($sql); | ||
| return parent::query($sql, $limit ?: null, $start ?: null); |
There was a problem hiding this comment.
XoopsMySQLDatabaseProxy::query() blocks read-only CTE queries because the allowlist regex doesn't include WITH, while the base XoopsMySQLDatabase::query() explicitly treats WITH as read-like. This can cause valid read-only queries (e.g. WITH ... SELECT ...) to fail during GET/proxy mode. Consider aligning the proxy regex with the base query() guard (include WITH).
| @@ -626,7 +627,7 @@ class XoopsMySQLDatabaseProxy extends XoopsMySQLDatabase | |||
| /** | |||
| * perform a query on the database | |||
| * | |||
| * this method allows only SELECT queries for safety. | |||
| * This method allows only read-only statements (SELECT, SHOW, DESCRIBE, EXPLAIN) for safety. | |||
| * | |||
| * @param string $sql a valid MySQL query | |||
| * @param int $limit number of records to return | |||
| @@ -638,17 +639,12 @@ class XoopsMySQLDatabaseProxy extends XoopsMySQLDatabase | |||
| public function query(string $sql, ?int $limit = null, ?int $start = null) | |||
| { | |||
| $sql = ltrim($sql); | |||
| if (!$this->allowWebChanges && stripos($sql, 'select') !== 0) { | |||
| if (!$this->allowWebChanges && !preg_match('/^\s*(SELECT|SHOW|DESCRIBE|EXPLAIN)\b/i', $sql)) { | |||
| trigger_error('Database updates are not allowed during processing of a GET request', E_USER_WARNING); | |||
|
|
|||
| return false; | |||
| } | |||
| // Execute via queryF() to preserve legacy path (and LIMIT semantics) | |||
| if ($limit !== null) { | |||
| $start = max(0, $start ?? 0); | |||
| return $this->queryF($sql, (int)$limit, (int)$start); | |||
| } | |||
| return $this->queryF($sql); | |||
| return parent::query($sql, $limit ?: null, $start ?: null); | |||
| } | |||
There was a problem hiding this comment.
The database proxy is enabled broadly in include/common.php for all non-POST requests and for failed referer checks, but the proxy only guards query(). With this PR migrating writes from queryF() to exec(), mutating statements are no longer constrained by the proxy mechanism at all, which undermines the intended web-change protection model. Please either (a) extend the proxy to enforce the same allowWebChanges policy for exec() (with an explicit escape hatch for essential GET-time writes like sessions), or (b) adjust the proxy design/docs so it no longer claims GET-request write protection.
- Update proxy query() docblock to accurately describe conditional GET-request guard rather than implying unconditional restriction - Remove redundant outer parentheses from isResultSet checks - Remove unused $date from array destructurings
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
htdocs/banners.php (2)
102-102: 🧹 Nitpick | 🔵 TrivialUnused global variable
$xoopsLoggerstill declared.The
$xoopsLoggeris declared in the global statement but never used withinbannerstats(). Consider removing it to eliminate dead code.,
♻️ Optional cleanup
- global $xoopsDB, $xoopsConfig, $xoopsLogger, $myts; + global $xoopsDB, $xoopsConfig, $myts;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` at line 102, In bannerstats(), remove the unused variable from the global declaration: drop $xoopsLogger from the global list (currently "global $xoopsDB, $xoopsConfig, $xoopsLogger, $myts;") so it becomes "global $xoopsDB, $xoopsConfig, $myts;" and run a quick search for other references to $xoopsLogger in bannerstats() to ensure nothing else depends on it.
297-297:⚠️ Potential issue | 🔴 CriticalCritical PHP syntax error: Missing parentheses around
ifcondition.Same issue as line 285 — this will cause a parse error.
🐛 Proposed fix
- if $xoopsDB->isResultSet($result) && $result instanceof \mysqli_result { + if ($xoopsDB->isResultSet($result) && $result instanceof \mysqli_result) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` at line 297, The if statement is missing parentheses and causes a PHP parse error; update the conditional around the call to $xoopsDB->isResultSet($result) combined with the instanceof check so the entire expression is wrapped in parentheses (i.e., the condition for the if using $xoopsDB->isResultSet($result) && $result instanceof \mysqli_result should be enclosed) and keep the existing block body intact.
🤖 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`:
- Line 352: The if statement is missing parentheses and causes a PHP parse
error; fix the condition to use proper parentheses like: wrap the entire boolean
expression so it reads if ($xoopsDB->isResultSet($result) && $result instanceof
\mysqli_result) { ... } (also review the similar occurrences around the checks
using $xoopsDB->isResultSet($result) and $result instanceof \mysqli_result at
the other locations).
- Line 285: The if statement at the top of the shown diff is missing parentheses
and causes a PHP parse error; update the condition to use proper PHP if syntax
by wrapping the entire condition in parentheses (i.e., change the statement that
references $xoopsDB->isResultSet($result2) && $result2 instanceof \mysqli_result
so the if is written as if ( $xoopsDB->isResultSet($result2) && $result2
instanceof \mysqli_result ) { ... } ), ensuring you keep the existing condition
logic and the corresponding block braces unchanged.
In `@htdocs/modules/system/admin/avatars/main.php`:
- Around line 324-344: The current flow updates users' user_avatar and then
calls $avt_handler->delete($avatar), which can fail and leave inconsistent
state; fix this by wrapping the user update and $avt_handler->delete($avatar)
call in a single DB transaction: begin a transaction before running the
xoopsDB->exec(...) update(s) and calling $avt_handler->delete($avatar), check
the return of both the update(s) and $avt_handler->delete($avatar), and if any
fail perform a rollback and handle the error (redirect_header/_AM_SYSTEM_DBERROR
or xoops_error) otherwise commit the transaction; alternatively, if transactions
aren’t available capture the affected users’ current user_avatar values before
the update and restore them if $avt_handler->delete($avatar) fails.
---
Duplicate comments:
In `@htdocs/banners.php`:
- Line 102: In bannerstats(), remove the unused variable from the global
declaration: drop $xoopsLogger from the global list (currently "global $xoopsDB,
$xoopsConfig, $xoopsLogger, $myts;") so it becomes "global $xoopsDB,
$xoopsConfig, $myts;" and run a quick search for other references to
$xoopsLogger in bannerstats() to ensure nothing else depends on it.
- Line 297: The if statement is missing parentheses and causes a PHP parse
error; update the conditional around the call to $xoopsDB->isResultSet($result)
combined with the instanceof check so the entire expression is wrapped in
parentheses (i.e., the condition for the if using $xoopsDB->isResultSet($result)
&& $result instanceof \mysqli_result should be enclosed) and keep the existing
block body intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a88d0fee-b420-40b5-8b26-511a9c20d2d1
📒 Files selected for processing (3)
htdocs/banners.phphtdocs/class/database/mysqldatabase.phphtdocs/modules/system/admin/avatars/main.php
| if ($user_id > 0 && $avatar->getVar('avatar_type', 'n') === 'C') { | ||
| if (!$xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') | ||
| . " SET user_avatar='blank.gif' WHERE uid=" . $user_id)) { | ||
| $resetOk = false; | ||
| } | ||
| } else { | ||
| if (!$xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') | ||
| . " SET user_avatar='blank.gif' WHERE user_avatar=" . $xoopsDB->quote($file))) { | ||
| $resetOk = false; | ||
| } | ||
| } | ||
| if (!$resetOk) { | ||
| redirect_header('admin.php?fct=avatars', 2, _AM_SYSTEM_DBERROR); | ||
| } | ||
| // Delete the avatar record | ||
| if (!$avt_handler->delete($avatar)) { | ||
| // Call Header | ||
| xoops_cp_header(); | ||
| // Display errors | ||
| xoops_error(sprintf(_AM_SYSTEM_AVATAR_FAILDEL, $avatar->getVar('avatar_id'))); | ||
| // Call Footer | ||
| xoops_cp_footer(); | ||
| exit(); | ||
| } |
There was a problem hiding this comment.
Make the avatar reset and delete atomic.
The users table is updated before $avt_handler->delete($avatar), so a delete failure leaves users already reset to blank.gif even though the avatar record still exists. XoopsAvatarHandler::delete() has its own failure points, so this path can still commit a partial state. Wrap both steps in one transaction, or capture the affected users and restore their previous user_avatar values if the delete fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/avatars/main.php` around lines 324 - 344, The
current flow updates users' user_avatar and then calls
$avt_handler->delete($avatar), which can fail and leave inconsistent state; fix
this by wrapping the user update and $avt_handler->delete($avatar) call in a
single DB transaction: begin a transaction before running the xoopsDB->exec(...)
update(s) and calling $avt_handler->delete($avatar), check the return of both
the update(s) and $avt_handler->delete($avatar), and if any fail perform a
rollback and handle the error (redirect_header/_AM_SYSTEM_DBERROR or
xoops_error) otherwise commit the transaction; alternatively, if transactions
aren’t available capture the affected users’ current user_avatar values before
the update and restore them if $avt_handler->delete($avatar) fails.
| if $xoopsDB->isResultSet($result2) && $result2 instanceof \mysqli_result { | ||
| $row = $xoopsDB->fetchRow($result2); |
There was a problem hiding this comment.
This if statement is missing parentheses around the condition and will cause a PHP parse error. Wrap the condition in () (and keep the opening {) so the file can be parsed/executed.
| if $xoopsDB->isResultSet($result) && $result instanceof \mysqli_result { | ||
| $row = $xoopsDB->fetchRow($result); |
There was a problem hiding this comment.
This if statement is missing parentheses around the condition and will cause a PHP parse error. Wrap the condition in () so the block is valid PHP syntax.
| if ($xoopsDB->isResultSet($result)) { | ||
| [$passwd] = $xoopsDB->fetchRow($result); | ||
| if ($_SESSION['banner_pass'] == $passwd) { | ||
| if $xoopsDB->isResultSet($result) && $result instanceof \mysqli_result { |
There was a problem hiding this comment.
This if statement is missing parentheses around the condition and will cause a PHP parse error. Wrap the condition in () so the block is valid PHP syntax.
| if $xoopsDB->isResultSet($result) && $result instanceof \mysqli_result { | |
| if ($xoopsDB->isResultSet($result) && $result instanceof \mysqli_result) { |
Restore required parentheses around if conditions at lines 285, 297, and 352 that were accidentally removed in the redundant-parens cleanup. Also remove unused $xoopsLogger global declaration.
|
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 (1)
htdocs/banners.php (1)
21-21: 🧹 Nitpick | 🔵 TrivialOptional: Import
RuntimeExceptionto avoid leading backslash.Per PSR-12 style preferences, add a use statement for
RuntimeExceptionto avoid the fully-qualified\RuntimeExceptionthroughout the file.use Xmf\Request; +use RuntimeException;Then update the throw statements:
- throw new \RuntimeException( + throw new RuntimeException(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/banners.php` at line 21, Add a top-level import for RuntimeException (e.g., "use RuntimeException;") and replace any fully-qualified throw sites that currently use "\RuntimeException" with the unqualified "RuntimeException" in this file (look for throw statements referencing \RuntimeException and update them to throw RuntimeException to follow PSR-12 style).
🤖 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/banners.php`:
- Line 21: Add a top-level import for RuntimeException (e.g., "use
RuntimeException;") and replace any fully-qualified throw sites that currently
use "\RuntimeException" with the unqualified "RuntimeException" in this file
(look for throw statements referencing \RuntimeException and update them to
throw RuntimeException to follow PSR-12 style).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f98164ea-53ab-4088-9dbd-d61d9abe294b
📒 Files selected for processing (1)
htdocs/banners.php
| * Proxy connection to a MySQL database with GET-request write protection. | ||
| * | ||
| * This class allows only SELECT queries to be performed through its | ||
| * {@link query()} method for security reasons. | ||
| * During GET requests (when allowWebChanges is false), this class restricts | ||
| * {@link query()} to read-only statements (SELECT, SHOW, DESCRIBE, EXPLAIN). | ||
| * Write operations should use {@link exec()} which is not subject to this guard. |
There was a problem hiding this comment.
The proxy DB class is described as providing GET-request write protection, but it explicitly states that write operations should use exec() (which is not guarded by allowWebChanges). In proxy mode (set on GET / failed referer in include/common.php), this means mutating SQL can still run via exec(), undermining the stated protection. Consider overriding exec() in XoopsMySQLDatabaseProxy to apply the same allowWebChanges guard (or update the class contract/docs so callers don’t assume GET writes are blocked).
|
|
||
| // delayed insert | ||
| $xoopsDB->queryF($sql4insertlog); | ||
| $xoopsDB->exec($sql4insertlog); |
There was a problem hiding this comment.
Protector’s dblayertrap alternative DB (ProtectorMySQLDatabase) implements SQL inspection in query(), but does not override exec(). Switching this INSERT to exec() means the statement bypasses dblayertrap checks when XOOPS_DB_ALTERNATIVE is active. Consider ensuring exec() statements go through the same inspection (e.g., implement exec() in ProtectorMySQLDatabase, or route through a checked path here).
| $xoopsDB->exec($sql4insertlog); | |
| $xoopsDB->query($sql4insertlog); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1639 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…and @ suppression
Deprecated API removal — queryF() (deprecated since 2.5.12)
Replace all queryF() calls in XOOPS-owned code with the correct modern method:
Files changed:
Misuse of query() for non-SELECT statements → exec()
Runtime error fixes from log analysis
Summary by CodeRabbit
Refactor
Bug Fixes
Security
Upgrade