Security: medium/low defence-in-depth hardening (pass 3)#105
Merged
Conversation
…installer)
Low-severity hardening grouped from the security backlog:
- checklogin: return the generic _US_INCORRECTLOGIN for every credential
failure instead of the auth provider's specific errors (user enumeration).
- common: cast the session user id to int in the last_login UPDATE.
- kernel/comment, notification, image: allowlist Criteria getSort() against
real column names before ORDER BY (mirrors privmessage/configcategory).
- kernel/block: intval the group-id lists and validate the ORDER BY clause
against a column/direction pattern in the deprecated group-block methods.
- xoopseditor/tinymce7: add JSON_HEX_TAG|JSON_HEX_APOS|JSON_HEX_QUOT to the
config json_encode to block </script> breakout.
- smarty3_plugins/xoPageNav: read params explicitly and escape the prev/next
hrefs (only the numbered links were escaped before).
- install/page_dbsettings: restrict DB_NAME to a safe identifier before it
reaches the backtick-quoted CREATE/ALTER DATABASE.
- install/page_tablesfill: read the admin session fields explicitly.
- xoopssecurity: bound bad-IP pattern length and block only on a definite
preg_match hit (a malformed pattern no longer warns or mis-blocks).
- pm/templates/pm_readpmsg: document the getVar('s') pre-escape contract.
XoopsMediaUploader::checkMimeType() now content-sniffs the uploaded bytes with finfo and refuses files whose real type is PHP source or HTML markup, regardless of the client-supplied MIME or the extension — closing the 'PHP disguised as .jpg' vector (M-12). The double-extension sanitiser list gains pht, phar, shtml and php8 so those segments are neutralised in names like shell.phar.jpg. The check runs in upload() before the file is copied to its destination. All core callers are image-only, so legitimate uploads are unaffected.
image.php capped the OUTPUT dimensions in the previous pass, but the source itself was decoded unconditionally. Reject the input before imagecreatefrom*() when the raw blob/file exceeds 25 MiB or the source would decode to more than ~40 MP, on both the DB-blob (imagecreatefromstring) and file (getimagesize) paths. Also reject when getimagesize() fails outright. This stops a decode bomb from exhausting memory even though the thumbnail output is later capped.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #105 +/- ##
============================================
+ Coverage 18.13% 18.59% +0.45%
- Complexity 7854 8062 +208
============================================
Files 666 670 +4
Lines 43208 43892 +684
============================================
+ Hits 7837 8160 +323
- Misses 35371 35732 +361 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several security enhancements and code quality improvements across the codebase, including input validation, SQL injection prevention via sorting allowlists and integer casting, protection against image decode bombs, mitigation of user enumeration, and explicit parameter extraction instead of using extract(). The review feedback recommends expanding the sorting allowlists in image.php and comment.php to include missing valid columns (imgcat_id, com_text, and com_sig), suppressing potential compilation warnings in xoopssecurity.php when matching user-defined regex patterns, and safely accessing the session array in page_tablesfill.php to prevent PHP 8.0+ warnings.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+318
to
+321
| $sort = !in_array($criteria->getSort(), [ | ||
| 'image_id', 'image_name', 'image_nicename', 'image_mimetype', | ||
| 'image_created', 'image_display', 'image_weight', | ||
| ], true) ? 'image_weight' : $criteria->getSort(); |
There was a problem hiding this comment.
The column imgcat_id is a valid field of the image table (representing the image category) and is commonly used for sorting images. It should be included in the sorting allowlist to prevent valid sort requests from being ignored and falling back to image_weight.
$sort = !in_array($criteria->getSort(), [
'image_id', 'image_name', 'image_nicename', 'image_mimetype',
'image_created', 'image_display', 'image_weight', 'imgcat_id',
], true) ? 'image_weight' : $criteria->getSort();
Comment on lines
+481
to
+486
| $sort = !in_array($criteria->getSort(), [ | ||
| 'com_id', 'com_pid', 'com_rootid', 'com_modid', 'com_itemid', 'com_icon', | ||
| 'com_created', 'com_modified', 'com_uid', 'com_user', 'com_email', 'com_url', | ||
| 'com_ip', 'com_title', 'com_status', 'com_exparams', | ||
| 'dohtml', 'dosmiley', 'doxcode', 'doimage', 'dobr', | ||
| ], true) ? 'com_id' : $criteria->getSort(); |
There was a problem hiding this comment.
The columns com_text and com_sig are valid fields of the xoopscomments table but are missing from the sorting allowlist. They should be added to ensure that any legitimate sorting requests on these fields are not ignored.
$sort = !in_array($criteria->getSort(), [
'com_id', 'com_pid', 'com_rootid', 'com_modid', 'com_itemid', 'com_icon',
'com_created', 'com_modified', 'com_uid', 'com_user', 'com_email', 'com_url',
'com_ip', 'com_title', 'com_text', 'com_status', 'com_sig', 'com_exparams',
'dohtml', 'dosmiley', 'doxcode', 'doimage', 'dobr',
], true) ? 'com_id' : $criteria->getSort();| if (!is_string($bi) || '' === $bi || strlen($bi) > 255) { | ||
| continue; | ||
| } | ||
| if (1 === preg_match('/' . $bi . '/', $ip)) { |
There was a problem hiding this comment.
If an administrator enters a pattern containing the / character (such as a CIDR block like 192.168.1.0/24 or other regex delimiters), preg_match will fail with a compilation error and trigger a PHP E_WARNING. To prevent these warnings from cluttering logs or being displayed to users, use the error suppression operator @ on preg_match.
if (1 === @preg_match('/' . $bi . '/', $ip)) {
Comment on lines
+83
to
+85
| $adminname = (string) ($_SESSION['siteconfig']['adminname'] ?? ''); | ||
| $adminmail = (string) ($_SESSION['siteconfig']['adminmail'] ?? ''); | ||
| $adminpass = (string) ($_SESSION['siteconfig']['adminpass'] ?? ''); |
There was a problem hiding this comment.
In PHP 8.0+, attempting to access a nested array offset on a null value (e.g., if $_SESSION['siteconfig'] is not set or has been cleared due to session expiration) will trigger a Warning: Trying to access array offset on value of type null. To prevent this warning, extract $_SESSION['siteconfig'] with a fallback to an empty array first.
$siteconfig = $_SESSION['siteconfig'] ?? [];
$adminname = (string) ($siteconfig['adminname'] ?? '');
$adminmail = (string) ($siteconfig['adminmail'] ?? '');
$adminpass = (string) ($siteconfig['adminpass'] ?? '');![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/include/common.php (1)
256-261:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix undefined DB handle in the exception path.
At Line 260,
$db->error()references an undefined variable. Ifexec()throws, this catch block can crash while building the error message.🔧 Suggested fix
} catch (Exception $e) { throw new \RuntimeException( - \sprintf(_DB_QUERY_ERROR, $sql) . $db->error(), + \sprintf(_DB_QUERY_ERROR, $sql) . $xoopsDB->error(), E_USER_ERROR, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@htdocs/include/common.php` around lines 256 - 261, The catch block after calling $xoopsDB->exec($sql) uses an undefined $db->error(), causing a secondary error; change the exception construction to reference the correct DB handle and include the caught exception message—for example, replace $db->error() with $xoopsDB->error() (and/or append $e->getMessage()) so the throw new \RuntimeException(\sprintf(_DB_QUERY_ERROR, $sql) . ' ' . $xoopsDB->error() . ' ' . $e->getMessage(), E_USER_ERROR) — ensure the catch uses $e only for message text and $xoopsDB for DB error details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/class/smarty3_plugins/function.xoPageNav.php`:
- Around line 45-47: The calls to htmlspecialchars used when building the
paginated links (e.g. creating $prevUrl and the other escaped URLs) must include
an explicit charset to be deterministic; update each htmlspecialchars(...) call
in function xoPageNav (the occurrences that build $prevUrl and the other link
vars around the block shown and the ones at lines referenced in the review) to
pass ENT_QUOTES | ENT_HTML5 as the flags and 'UTF-8' as the encoding argument
(i.e. htmlspecialchars($value, ENT_QUOTES | ENT_HTML5, 'UTF-8')). Ensure every
escaped attribute in that function uses the same explicit 'UTF-8' encoding.
In `@htdocs/class/uploader.php`:
- Line 612: The htmlspecialchars() call used when setting the upload error
message omits the explicit charset; update the call so it passes 'UTF-8' as the
third argument (e.g., change htmlspecialchars($detected, ENT_QUOTES | ENT_HTML5)
to htmlspecialchars($detected, ENT_QUOTES | ENT_HTML5, 'UTF-8')) where the error
is built (the setErrors(...) invocation that wraps htmlspecialchars) to ensure
deterministic UTF-8 escaping.
In `@htdocs/kernel/image.php`:
- Around line 318-322: The ORDER BY validation currently rejects comma-separated
multi-column clauses; update both locations to split the incoming
$criteria->getSort() value by commas, validate each clause individually against
the allowlist (the tokens
'image_id','image_name','image_nicename','image_mimetype','image_created','image_display','image_weight'
for image.php and the corresponding allowed columns in notification.php), allow
an optional ascending/descending suffix (ASC/DESC) per clause, reassemble only
the validated clauses joined by commas, and use that safe string in the existing
$sql .= ' ORDER BY ' . ... expression; for any clause that fails validation drop
it (and if none remain default to the original fallback 'image_weight' or the
notification handler's fallback) so callers that pass safe multi-column sorts
continue to work. For htdocs/kernel/image.php (lines 318-322) implement the
per-clause parsing/validation with the existing $sort variable and
criteria->getSort(), and for htdocs/kernel/notification.php (lines 358-362)
apply the same change using that handler's allowed column set and fallback
behavior.
---
Outside diff comments:
In `@htdocs/include/common.php`:
- Around line 256-261: The catch block after calling $xoopsDB->exec($sql) uses
an undefined $db->error(), causing a secondary error; change the exception
construction to reference the correct DB handle and include the caught exception
message—for example, replace $db->error() with $xoopsDB->error() (and/or append
$e->getMessage()) so the throw new \RuntimeException(\sprintf(_DB_QUERY_ERROR,
$sql) . ' ' . $xoopsDB->error() . ' ' . $e->getMessage(), E_USER_ERROR) — ensure
the catch uses $e only for message text and $xoopsDB for DB error details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7893d67b-a9fc-458d-b15d-7b5b191f4249
📒 Files selected for processing (14)
htdocs/class/smarty3_plugins/function.xoPageNav.phphtdocs/class/uploader.phphtdocs/class/xoopseditor/tinymce7/TinyMCE.phphtdocs/class/xoopssecurity.phphtdocs/image.phphtdocs/include/checklogin.phphtdocs/include/common.phphtdocs/install/page_dbsettings.phphtdocs/install/page_tablesfill.phphtdocs/kernel/block.phphtdocs/kernel/comment.phphtdocs/kernel/image.phphtdocs/kernel/notification.phphtdocs/modules/pm/templates/pm_readpmsg.tpl
- kernel comment/notification/image: replace the exact single-column ORDER BY allowlist with the same comma-separated column/direction pattern used in block.php, so valid multi-column sorts and columns not in the original list (imgcat_id, com_text, com_sig, ...) are no longer dropped while injection is still blocked (CodeRabbit, Gemini). - image.php: for file-backed image records, check is_file()/filesize() BEFORE file_get_contents() so an oversized file is rejected without being read fully into memory (Grok, Codex). - xoopssecurity: scan bad-IP patterns under a temporary error handler so a malformed admin pattern (e.g. a CIDR whose '/' collides with the delimiter) neither warns nor blocks (Grok, Codex, Gemini). - xoPageNav, uploader: pass 'UTF-8' explicitly to the new htmlspecialchars() calls (CodeRabbit). - tests: add UploaderContentSniffTest covering PHP/HTML-as-image rejection and genuine-PNG acceptance (Grok, Codex test-gap).
- xoopssecurity: drop the redundant restore_error_handler() before exit() in checkBadips() — exit() bypasses finally, and finally already restores on the normal path, so the explicit pre-exit restore was just noise (Grok). - image.php: treat filesize() === false as a rejection on the file-backed path (false > N is false, so a stat failure would otherwise slip through to file_get_contents()) (Grok, Codex). - install/page_tablesfill: read siteconfig into a local array first, then pull the admin fields from it (Grok, Gemini). - include/common: fix the pre-existing $db->error() -> $xoopsDB->error() in the last_login UPDATE catch block ($db was undefined there) (Grok).
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/include/common.php (1)
256-262:⚠️ Potential issue | 🟡 MinorCatch
\Throwableinstead ofExceptionin the login DBexec()boundaryIn
htdocs/include/common.php(lines 256-262), theexec()error boundary currently catchesException, butErrorwon’t be handled (PHP 8’sexec()failures can surface asError). Widen the catch to\Throwableto keep the flow consistent with the coding guidelines.Suggested patch
- } catch (Exception $e) { + } catch (\Throwable $e) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@htdocs/include/common.php` around lines 256 - 262, The try/catch around $xoopsDB->exec($sql) currently catches Exception only; change the catch to catch (\Throwable $e) so PHP 8 Errors as well as Exceptions are handled and the existing RuntimeException throw (sprintf(_DB_QUERY_ERROR, $sql) . $xoopsDB->error(), E_USER_ERROR) remains unchanged; update the catch clause from "catch (Exception $e)" to "catch (\Throwable $e)" (keeping the variable $e if you need it) so the exec() boundary handles all throwables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/kernel/comment.php`:
- Around line 481-487: Replace the regex-only ORDER BY validation with an
explicit allowlist rebuild: parse Criteria::getSort() into comma-separated
clauses, for each clause validate that the column (and any allowed table alias)
exactly matches one of the real, permitted sortable columns for the handler,
permit an optional ASC/DESC only if the column is allowed, and then reassemble
and set $sort to the joined, approved clauses (or fall back to a safe default)
so only handler-owned columns reach SQL. For htdocs/kernel/comment.php (lines
481-487) validate against the xoopscomments table’s actual column names and
rebuild the ORDER BY from those approved clauses (instead of using the regex),
and for htdocs/kernel/image.php (lines 318-324) apply the same pattern using the
image handler’s supported sortable columns (including any intentional table
aliases); any clause not matching the allowlist must be dropped and if no
clauses remain use the handler’s safe default sort.
- Line 488: The ORDER BY direction can be injected because Criteria::$order is
public and Criteria::getOrder() may return an unvalidated value; fix the root
cause in criteria.php by making the order property non-public (private or
protected) and updating getOrder() to enforce the whitelist (only return 'ASC'
or 'DESC', default to 'ASC' on invalid input) or otherwise sanitize/cast the
value centrally; then no direct change is needed in the call sites. For the
consolidated sites: htdocs/kernel/comment.php (488-488) and
htdocs/kernel/image.php (325-325) continue to use $criteria->getOrder() for SQL
assembly but rely on the corrected Criteria::getOrder()—no per-site SQL changes
required if the Criteria fix is applied; alternatively, if you cannot change
Criteria, sanitize the order before concatenation at these two sites by
replacing $criteria->getOrder() with a small whitelist check that only allows
'ASC' or 'DESC' and falls back to 'ASC'.
---
Outside diff comments:
In `@htdocs/include/common.php`:
- Around line 256-262: The try/catch around $xoopsDB->exec($sql) currently
catches Exception only; change the catch to catch (\Throwable $e) so PHP 8
Errors as well as Exceptions are handled and the existing RuntimeException throw
(sprintf(_DB_QUERY_ERROR, $sql) . $xoopsDB->error(), E_USER_ERROR) remains
unchanged; update the catch clause from "catch (Exception $e)" to "catch
(\Throwable $e)" (keeping the variable $e if you need it) so the exec() boundary
handles all throwables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a9af830-0e61-421b-bfe4-4aac36dd630e
📒 Files selected for processing (10)
htdocs/class/smarty3_plugins/function.xoPageNav.phphtdocs/class/uploader.phphtdocs/class/xoopssecurity.phphtdocs/image.phphtdocs/include/common.phphtdocs/install/page_tablesfill.phphtdocs/kernel/comment.phphtdocs/kernel/image.phphtdocs/kernel/notification.phptests/unit/htdocs/class/UploaderContentSniffTest.php
- kernel/object: add XoopsObjectHandler::buildOrderBy(), a shared helper that
rebuilds an ORDER BY list from a per-handler column allowlist, preserves table
prefixes, and attaches each clause's own direction (falling back to the criteria
order). This replaces the syntax-only regex in the comment/notification/image
handlers and fixes two issues raised in review:
* sort tokens are now restricted to real columns of each table, not just any
identifier-shaped string (CodeRabbit, Codex);
* the criteria order is no longer appended a second time, so a sort that
already carries a direction can no longer produce 'ORDER BY col DESC ASC'
(Grok, Codex).
com_text/com_sig are included in the comment allowlist (Gemini).
- modules/system/class/block: port the same int casts on group-id / block-id
lists and the ORDER BY validation that kernel/block.php received — this is the
active handler reached via xoops_getModuleHandler('block') in the block admin
UI, including getNonGroupedBlocks() (Grok, Codex).
- include/common: widen the last_login catch to \Throwable for PHP 8.x DB
boundaries (CodeRabbit, Grok).
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 (2)
htdocs/class/smarty3_plugins/function.xoPageNav.php (1)
40-41:⚠️ Potential issue | 🟡 MinorFix pagination clamp bounds in
xoPageNav(function.xoPageNav.php lines 40–41).
$minPage/$maxPageare usingmin/maxthe wrong way around, so the page window can go outside valid bounds (e.g.,currentPage=1can produce$minPage <= 0, and$maxPagecan exceed$lastPage). Likely swap tomax(1, ...)for the minimum andmin($lastPage, ...)for the maximum.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@htdocs/class/smarty3_plugins/function.xoPageNav.php` around lines 40 - 41, In xoPageNav, the pagination clamp is inverted: set $minPage to max(1, ceil($currentPage - $linksCount / 2)) and set $maxPage to min($lastPage, floor($currentPage + $linksCount / 2)) so the page window is clamped inside [1, $lastPage]; update the assignments for $minPage and $maxPage accordingly (ensure the ceil/floor and arithmetic grouping remains as intended).htdocs/modules/system/class/block.php (1)
416-425:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winShort-circuit empty group lists before building SQL.
After the new
array_map('intval', ...), an empty array still falls through to$groupid[0]here, and the siblingIN (...)paths ingetBlockByPerm()/getAllByGroupModule()still generate invalid SQL for[]. Return an empty result as soon as the normalized list is empty instead of building a broken predicate.🛠️ Suggested fix
if (is_array($groupid)) { - $groupid = array_map('intval', $groupid); + $groupid = array_values(array_map('intval', $groupid)); + if ([] === $groupid) { + return []; + } $sql .= ' AND (l.gperm_groupid=' . $groupid[0] . '';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@htdocs/modules/system/class/block.php` around lines 416 - 425, After normalizing $groupid with array_map('intval') in the block permission code, short-circuit when the resulting array is empty to avoid referencing $groupid[0] and generating invalid SQL; in the methods that build this predicate (e.g., getBlockByPerm() and getAllByGroupModule() in class Block / block.php) check if (empty($groupid)) and return an empty result (or appropriate empty dataset) immediately before constructing the SQL, otherwise continue building the IN/OR clauses as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/kernel/notification.php`:
- Around line 361-364: The ORDER BY allowlist in
XoopsNotificationHandler::getObjects() includes an invalid column 'not_class'
which causes XoopsObjectHandler::buildOrderBy() to emit ORDER BY not_class even
though the xoopsnotifications table has no such column; update the allowlist in
that method by replacing 'not_class' with 'not_category' (or otherwise map the
sort token to the actual column name) so buildOrderBy() only produces valid
column names, then run tests/queries that sort by the notification class to
confirm the SQL no longer references not_class.
In `@htdocs/kernel/object.php`:
- Around line 1092-1096: The current buildOrderBy() accepts and re-emits a
qualifier from $m[1] without validation; change it to reject or strip qualifiers
so only allowlisted column names are used: after the preg_match, require that
$m[1] is empty (reject the token) or ignore $m[1] entirely and build the clause
using only $m[2] (the validated column) plus the normalized direction from
$m[3], and then append that to $parts; ensure you still check
in_array(strtolower($m[2]), $allowedColumns, true) and keep direction
normalization (uppercase or default $order). This prevents attacker-controlled
qualifiers like "bogus.com_id" from reaching the generated ORDER BY.
---
Outside diff comments:
In `@htdocs/class/smarty3_plugins/function.xoPageNav.php`:
- Around line 40-41: In xoPageNav, the pagination clamp is inverted: set
$minPage to max(1, ceil($currentPage - $linksCount / 2)) and set $maxPage to
min($lastPage, floor($currentPage + $linksCount / 2)) so the page window is
clamped inside [1, $lastPage]; update the assignments for $minPage and $maxPage
accordingly (ensure the ceil/floor and arithmetic grouping remains as intended).
In `@htdocs/modules/system/class/block.php`:
- Around line 416-425: After normalizing $groupid with array_map('intval') in
the block permission code, short-circuit when the resulting array is empty to
avoid referencing $groupid[0] and generating invalid SQL; in the methods that
build this predicate (e.g., getBlockByPerm() and getAllByGroupModule() in class
Block / block.php) check if (empty($groupid)) and return an empty result (or
appropriate empty dataset) immediately before constructing the SQL, otherwise
continue building the IN/OR clauses as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b088f695-1d16-461e-b9e5-1cc5c2666951
📒 Files selected for processing (12)
htdocs/class/smarty3_plugins/function.xoPageNav.phphtdocs/class/uploader.phphtdocs/class/xoopssecurity.phphtdocs/image.phphtdocs/include/common.phphtdocs/install/page_tablesfill.phphtdocs/kernel/comment.phphtdocs/kernel/image.phphtdocs/kernel/notification.phphtdocs/kernel/object.phphtdocs/modules/system/class/block.phptests/unit/htdocs/class/UploaderContentSniffTest.php
XOOPS#105 round 5) Follow-up to the pass-3 hardening review (Grok/Codex): - kernel/notification.php: drop `not_class` from the ORDER BY allowlist — it is an index name on not_category, not a real column, and would emit invalid SQL. - kernel/object.php: buildOrderBy() takes a canonical $columnPrefix. It drops any caller-supplied qualifier and re-applies the canonical one, so an unknown alias (e.g. "bogus.com_id") cannot reach SQL while a joined query can force its own alias to stay unambiguous. The prefix itself is validated ('' or "alias."). - kernel/image.php: the binary path joins imagebody, where image_id exists in both tables; pass the "i." prefix when $getbinary so the joined ORDER BY is not ambiguous (single-table path stays bare). Adds imgcat_id to the allowlist. - image.php: reject the URL/src path when filesize() returns false, and reject a decode failure (imagecreatefromstring() not a GdImage) before imagesx(). - class/uploader.php: add explicit 'UTF-8' to the mediaRealType error escape. - xoopseditor/tinymce5: add JSON_HEX_TAG|APOS|QUOT to the settings json_encode, matching TinyMCE7 (blocks a </script> breakout; callbacks are emitted separately). - tests: refresh the uploader denylist expectations to the current default and add XoopsObjectHandlerBuildOrderByTest covering the helper (injection, direction, fallback, bogus-prefix drop, canonical "i." join prefix, malformed-prefix guard).
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 (1)
htdocs/modules/system/class/block.php (1)
416-425:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard empty group-ID arrays before building the predicate.
An empty array still breaks all three hardened array branches: this path reads
$groupid[0], while the sibling branches on Line 484 and Line 530 generateIN (). That turns a legitimate “no groups” result into warnings or invalid SQL instead of a clean empty result.Suggested fix
if (is_array($groupid)) { $groupid = array_map('intval', $groupid); + if ([] === $groupid) { + return $ret; + } $sql .= ' AND (l.gperm_groupid=' . $groupid[0] . '';Apply the same empty-array early return in
getBlockByPerm()andgetAllByGroupModule()before emitting theirIN (...)clauses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@htdocs/modules/system/class/block.php` around lines 416 - 425, The code builds SQL predicates from $groupid arrays without guarding against an empty array, which leads to reading $groupid[0] and emitting invalid "IN ()" clauses; fix by adding an early guard in getBlockByPerm() and getAllByGroupModule() (and the block-building code shown) that checks if (is_array($groupid) && count($groupid) === 0) and returns an empty result (e.g., return []; or otherwise short-circuit the query) before constructing any SQL, or alternatively convert empty arrays into a safe predicate (e.g., FALSE condition) so neither $groupid[0] is accessed nor IN () is emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/class/xoopssecurity.php`:
- Around line 268-282: Add an is_array() guard before iterating
$xoopsConfig['bad_ips'] to avoid a TypeError if the config value is not an
array; specifically, wrap the foreach block with a check like if
(!empty($xoopsConfig['bad_ips']) && is_array($xoopsConfig['bad_ips'])) { ... }
so the set_error_handler/restore_error_handler pair still surrounds only the
safe iteration and preg_match('/' . $bi . '/', $ip) call, and skip iteration
when the guard fails.
In `@htdocs/image.php`:
- Around line 347-351: The new source-bytes check uses is_file($imagePath) and
rejects when $imagePath is a URL, breaking remote-image mode; change the logic
so the filesize check only applies for local files: compute a local flag (e.g.,
$isLocal = is_file($imagePath)) and keep $srcBytes = $isLocal ?
filesize($imagePath) : false, then update the combined guard that currently
checks $srcBytes to only enforce the filesize existence/limit when $isLocal (for
example replace the `false === $srcBytes || $srcBytes > $imgSrcMaxBytes`
condition with `($isLocal && (false === $srcBytes || $srcBytes >
$imgSrcMaxBytes))`), leaving the $imagesize pixel check intact; this preserves
remote-image mode while retaining local-file size protections.
In `@htdocs/modules/system/class/block.php`:
- Around line 446-451: The ORDER BY allowlist regex used to validate $orderby in
class Block (variable $orderby) is too permissive (it allows tokens like "b.",
"b..weight", "m.block_id.desc"); tighten the token pattern to require either a
column name or table.column where both table and column start with [A-Za-z_] and
are followed by [A-Za-z0-9_]* (i.e. disallow trailing dots or extra dots), keep
the optional ASC|DESC qualifier and comma-separated list semantics, and replace
the current permissive pattern with this stricter pattern in the three
occurrences (the current check around $orderby in block.php and the same pattern
repeated at the sites noted on Line 575 and Line 650) so malformed inputs fall
back to the safe default.
In `@tests/unit/htdocs/class/UploaderContentSniffTest.php`:
- Around line 45-46: The temp-file setup in the probe helper currently casts
tempnam() to string and ignores file_put_contents() return value; change the
setup in UploaderContentSniffTest so you validate tempnam() didn't return false
and that file_put_contents(...) !== false (or returns the expected byte count)
and fail the test or throw an exception if either check fails; update references
to $this->tmp to remain a string only after successful validation so subsequent
test steps don't silently proceed on a failed temp-dir or write.
---
Outside diff comments:
In `@htdocs/modules/system/class/block.php`:
- Around line 416-425: The code builds SQL predicates from $groupid arrays
without guarding against an empty array, which leads to reading $groupid[0] and
emitting invalid "IN ()" clauses; fix by adding an early guard in
getBlockByPerm() and getAllByGroupModule() (and the block-building code shown)
that checks if (is_array($groupid) && count($groupid) === 0) and returns an
empty result (e.g., return []; or otherwise short-circuit the query) before
constructing any SQL, or alternatively convert empty arrays into a safe
predicate (e.g., FALSE condition) so neither $groupid[0] is accessed nor IN ()
is emitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a11541e7-ba50-4b70-93f7-11ebc6381394
📒 Files selected for processing (15)
htdocs/class/smarty3_plugins/function.xoPageNav.phphtdocs/class/uploader.phphtdocs/class/xoopseditor/tinymce5/tinymce.phphtdocs/class/xoopssecurity.phphtdocs/image.phphtdocs/include/common.phphtdocs/install/page_tablesfill.phphtdocs/kernel/comment.phphtdocs/kernel/image.phphtdocs/kernel/notification.phphtdocs/kernel/object.phphtdocs/modules/system/class/block.phptests/unit/htdocs/class/UploaderContentSniffTest.phptests/unit/htdocs/class/XoopsMediaUploaderTest.phptests/unit/htdocs/kernel/XoopsObjectHandlerBuildOrderByTest.php
…size cap (PR XOOPS#105 round 6) Round-6 review follow-up (Grok/Codex), all CI green on the prior commit: - image.php: scope the source byte cap to local files. The previous guard rejected every non-local source (is_file() false -> filesize() false), which would break remote-image mode if ONLY_LOCAL_IMAGES is ever disabled. The pixel cap from getimagesize() still applies to remote sources. - kernel/block.php + modules/system/class/block.php: tighten the ORDER BY allowlist regex so malformed dotted identifiers (`b.`, `b..weight`, `m.block_id.desc`) are rejected and fall back to the default instead of reaching SQL as invalid identifiers. - kernel/block.php::getNonGroupedBlocks: add the array_map('intval') on the bid IN(...) list and the ORDER BY guard that the active system handler already has. - block group-id handling: return early on an empty group-id array in every method that builds a gperm_groupid clause (getAllBlocksByGroup, getBlockByPerm, getAllByGroupModule, both files) instead of emitting `gperm_groupid=` from an undefined $groupid[0] or a malformed `IN ()`. - xoopssecurity: skip the bad-IP scan unless bad_ips is actually an array, so a corrupted config value cannot raise a TypeError. - UploaderContentSniffTest: assert tempnam()/file_put_contents() succeeded so a failed fixture setup cannot masquerade as a pass.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/class/xoopseditor/tinymce5/tinymce.php`:
- Around line 219-221: The current catch block for json_encode(...,
JSON_THROW_ON_ERROR) only handles \JsonException, so change the error handling
in tinymce.php to also catch all Throwables: either replace the existing catch
(\JsonException $e) with catch (\Throwable $e) or add a second catch (\Throwable
$e) after the \JsonException one; in either case preserve the existing behavior
(call trigger_error('TinyMCE5 settings JSON encode failed: ' . $e->getMessage(),
E_USER_WARNING); and return "\n<!-- TinyMCE: failed to encode editor
configuration -->\n";).
In `@htdocs/install/page_tablesfill.php`:
- Around line 83-86: Normalize $_SESSION['siteconfig'] to an array before
reading offsets: replace the current direct read into $siteconfig with a guard
like $siteconfig = is_array($_SESSION['siteconfig']) ? $_SESSION['siteconfig'] :
[]; then keep the existing assignments to $adminname, $adminmail, and $adminpass
(casting to string as before) to avoid PHP warnings when $_SESSION['siteconfig']
is not an array.
In `@htdocs/kernel/block.php`:
- Around line 856-859: Replace the regex-only validation of the $orderby string
with a rebuild from a per-query allowlist of real column names (same pattern as
the existing buildOrderBy() helper): parse incoming tokens, map allowed
alias.column names to their canonical column identifiers for this query (only
the aliases/columns actually selected by the query), ignore or drop unknown
tokens, preserve ASC/DESC, and if the resulting list is empty fall back to the
safe default 'b.weight,b.bid'; then append the sanitized, rebuilt ORDER BY to
$sql instead of concatenating raw $orderby. Ensure you use the XOOPS DB handlers
(parameterized queries where applicable) and do not directly inject user input
into $sql; apply this change wherever $orderby is validated currently (the
locations using the regex check) so ordering is always derived from the
per-query allowlist.
In `@tests/unit/htdocs/class/XoopsMediaUploaderTest.php`:
- Around line 42-44: The test currently injects a hard-coded list into
createUploader(), then asserts the same list in
testDefaultExtensionsToBeSanitized(), which masks regressions; update the
fixture and test so the assertion checks the uploader's actual default instead
of a supplied value: remove the override of extensionsToBeSanitized from the
shared createUploader() helper (stop passing that hard-coded array into
createUploader()), and change testDefaultExtensionsToBeSanitized() to
instantiate the uploader with no custom extensions and assert against the
uploader instance's default property (extensionsToBeSanitized) or the class's
declared default, ensuring the test verifies the class default rather than a
test-supplied value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15433c34-cd1a-4d98-98be-bdb3759a95cc
📒 Files selected for processing (16)
htdocs/class/smarty3_plugins/function.xoPageNav.phphtdocs/class/uploader.phphtdocs/class/xoopseditor/tinymce5/tinymce.phphtdocs/class/xoopssecurity.phphtdocs/image.phphtdocs/include/common.phphtdocs/install/page_tablesfill.phphtdocs/kernel/block.phphtdocs/kernel/comment.phphtdocs/kernel/image.phphtdocs/kernel/notification.phphtdocs/kernel/object.phphtdocs/modules/system/class/block.phptests/unit/htdocs/class/UploaderContentSniffTest.phptests/unit/htdocs/class/XoopsMediaUploaderTest.phptests/unit/htdocs/kernel/XoopsObjectHandlerBuildOrderByTest.php
Comment on lines
+219
to
+221
| } catch (\JsonException $e) { | ||
| trigger_error('TinyMCE5 settings JSON encode failed: ' . $e->getMessage(), E_USER_WARNING); | ||
| return "\n<!-- TinyMCE: failed to encode editor configuration -->\n"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="htdocs/class/xoopseditor/tinymce5/tinymce.php"
# Print file header context + the exact region around the referenced lines.
nl -ba "$FILE" | sed -n '180,260p'Repository: XOOPS/XoopsCore27
Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="htdocs/class/xoopseditor/tinymce5/tinymce.php"
if [ ! -f "$FILE" ]; then
echo "File not found: $FILE" >&2
exit 1
fi
# Show surrounding lines using cat -n (since nl isn't available)
cat -n "$FILE" | sed -n '200,250p'Repository: XOOPS/XoopsCore27
Length of output: 2351
Catch \Throwable for the non-fatal JSON encode fallback.
htdocs/class/xoopseditor/tinymce5/tinymce.php currently catches only \JsonException for json_encode(... JSON_THROW_ON_ERROR ...), so any other thrown Throwable would bypass the trigger_error(..., E_USER_WARNING) and the safe HTML comment return. Catch \Throwable $e (or add a second catch (\Throwable $e)) while keeping the existing warning + return behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/class/xoopseditor/tinymce5/tinymce.php` around lines 219 - 221, The
current catch block for json_encode(..., JSON_THROW_ON_ERROR) only handles
\JsonException, so change the error handling in tinymce.php to also catch all
Throwables: either replace the existing catch (\JsonException $e) with catch
(\Throwable $e) or add a second catch (\Throwable $e) after the \JsonException
one; in either case preserve the existing behavior (call trigger_error('TinyMCE5
settings JSON encode failed: ' . $e->getMessage(), E_USER_WARNING); and return
"\n<!-- TinyMCE: failed to encode editor configuration -->\n";).
Source: Coding guidelines
Comment on lines
+856
to
+859
| if (!preg_match('/^\s*(?:[A-Za-z_]\w*\.)?[A-Za-z_]\w*(?:\s+(?:ASC|DESC))?(?:\s*,\s*(?:[A-Za-z_]\w*\.)?[A-Za-z_]\w*(?:\s+(?:ASC|DESC))?)*\s*$/i', (string) $orderby)) { | ||
| $orderby = 'b.weight,b.bid'; | ||
| } | ||
| $sql .= ' ORDER BY ' . $orderby; |
There was a problem hiding this comment.
Replace regex-only ORDER BY checks with per-query column allowlists.
Line 856, Line 1071, and Line 1150 currently validate ORDER BY syntax shape only. Inputs like unknown/invalid alias.column tokens still pass and reach SQL execution, causing query failures instead of deterministic safe fallback. Please route these through the same real-column allowlist rebuild strategy used elsewhere (e.g., buildOrderBy() pattern), scoped to each method’s actual aliases/columns.
As per coding guidelines, "Use parameterized queries via XOOPS database handlers — never concatenate user input into SQL"; and based on PR hardening objectives, ORDER BY should be rebuilt from allowlisted real columns with safe fallback.
Also applies to: 1071-1074, 1148-1153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@htdocs/kernel/block.php` around lines 856 - 859, Replace the regex-only
validation of the $orderby string with a rebuild from a per-query allowlist of
real column names (same pattern as the existing buildOrderBy() helper): parse
incoming tokens, map allowed alias.column names to their canonical column
identifiers for this query (only the aliases/columns actually selected by the
query), ignore or drop unknown tokens, preserve ASC/DESC, and if the resulting
list is empty fall back to the safe default 'b.weight,b.bid'; then append the
sanitized, rebuilt ORDER BY to $sql instead of concatenating raw $orderby.
Ensure you use the XOOPS DB handlers (parameterized queries where applicable)
and do not directly inject user input into $sql; apply this change wherever
$orderby is validated currently (the locations using the regex check) so
ordering is always derived from the per-query allowlist.
Source: Coding guidelines
… tests (PR XOOPS#105 round 7) CodeRabbit round-7 follow-up (all CI green; both Grok/Codex report no blocking issues): - install/page_tablesfill: normalise $_SESSION['siteconfig'] to an array before the offset reads. The previous `?? []` only covered null/unset; a corrupted non-array session value would still raise a TypeError on `$siteconfig['admin*']`. - XoopsMediaUploaderTest: read the real class default in the default-value tests via newInstanceWithoutConstructor() instead of the shared fixture, which injected the same list it asserted (a self-fulfilling test that could not catch a regression in the class default).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security hardening — pass 3 (medium/low defence-in-depth)
Third hardening pass over the remaining backlog items. Pass 1 (#92) covered the
Critical + High findings; pass 2 (#102) covered the medium XSS/SSRF/crypto set.
This pass takes the remaining medium/low defence-in-depth items that are safe to
apply now, and explicitly defers the ones that need a test environment or a
caller audit (listed at the bottom).
Included
Defence-in-depth batch (
security(core))_US_INCORRECTLOGINfor every credentialfailure instead of the auth provider's specific errors (user enumeration).
intin thelast_loginUPDATE.Criteria::getSort()against the real column names before
ORDER BY(mirrors the existingprivmessage/configcategorypattern).getOrder()is already constrainedto
ASC/DESC.intvalthe group-id lists and validate theORDER BYclause against a column/direction pattern in the deprecated group-block methods.
JSON_HEX_TAG|JSON_HEX_APOS|JSON_HEX_QUOTtothe config
json_encodeto block</script>breakout (the raw-JS placeholdermechanism is unaffected — placeholders are alphanumeric).
extract()and escape the prev/next hrefs (only the numbered links were escaped before).
DB_NAMEto a safe identifier before itreaches the backtick-quoted
CREATE/ALTER DATABASE(mysqli_real_escape_stringdoes not escape backticks).
of
extract($_SESSION['siteconfig']).preg_matchhit, so a malformed admin pattern no longer warns or mis-blocks.getVar('s')pre-escape contract.Upload content-sniffing (
security(upload))XoopsMediaUploader::checkMimeType()now content-sniffs the uploaded bytes withfinfoand refuses files whose real type is PHP source or HTML, regardless ofthe client-supplied MIME or extension. The double-extension sanitiser gains
pht/phar/shtml/php8. Runs inupload()before the file is copied. Allcore callers are image-only, so legitimate uploads are unaffected.
Image source cap (
security(image))imagecreatefrom*()when the raw blob/file exceeds25 MiB or would decode to more than ~40 MP, on both the DB-blob and file paths
(the previous pass only capped the output). Also rejects when
getimagesize()fails outright.
Verification
Focused PHPUnit run (security/uploader/image/kernel suites): 673 tests, 1213
assertions, 0 failures/errors. PHP-lint clean on every changed file.
Deliberately deferred (need a test env or an audit — not shipped here)
mainfile.phpis written at step 7 of 14 and every later step re-includescommon.inc.php; DB-state signals also flip mid-install. Needs a realfresh+mid+rerun install test before a correct (completion-marker) guard.
getVar('field','e')(already escaped), so renderer-side escaping would visiblybreak admin edit forms. Needs a renderer-contract decision + caller normalisation.
SnoopyTestadded in "security audit phase 2") and its notice marks removal fora future version, so it is being kept and maintained on purpose.
XOOPS_URLbranch (cosmetic; removal risksXOOPS_URLwriting), L-17 / T-19 the repo-root
upgrade/tree (separate git repo —its own follow-up), L-18 request-hash pinning (needs a per-endpoint
request-method audit;
user.php'soplegitimately arrives via both GET and POST).Summary by CodeRabbit
Release Notes
Review round 2 (CodeRabbit / Gemini / Grok / Codex) — addressed in 1b4a275
comment/notification/imagehandlers now use the same comma-separated column/direction pattern as
block.phpinstead of an exact single-column list, so valid multi-column sorts and columns
outside the original list are preserved while injection is still blocked.
is_file()/filesize()beforefile_get_contents(), so an oversized file isrejected without being read into memory.
error handler, so a malformed admin pattern (e.g. a CIDR whose
/collides with thedelimiter) genuinely neither warns nor blocks.
'UTF-8'added to the new calls.UploaderContentSniffTestcovers PHP/HTML-as-imagerejection and genuine-PNG acceptance.
Scope note on upload sniffing:
checkMimeType()blocks executable/markup contentby sniffing (PHP-source and HTML families) rather than enforcing the full detected MIME
against the per-call allowlist — enforcing exact-MIME equality risks false rejects
(
image/jpegvsimage/pjpeg/image/x-png), and declared images are independentlyre-validated by
checkImageType()'sgetimagesize().