fix(modulesadmin): add CSRF token to module admin AJAX requests#1685
Conversation
Module toggle and drag-and-drop reorder AJAX calls did not send CSRF tokens, causing failures when token validation is enforced. Send token with each request and refresh from response.
WalkthroughMoved module order/display/display_in_menu handling into an early CSRF-checked block in PHP that returns refreshed token HTML and exits; JS now appends Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (JS)
participant Server as Server (PHP)
participant Security as CSRF Validator
Client->>Client: locate `input[name="XOOPS_TOKEN_REQUEST"]` in DOM
Client->>Client: add token name/value to POST payload
Client->>Server: POST request (payload + token)
Server->>Security: $GLOBALS['xoopsSecurity']->check()
alt token valid
Security-->>Server: valid
Server->>Server: perform order/display/display_in_menu update
Server->>Server: generate refreshed token HTML (getTokenHTML)
Server-->>Client: 200 OK + response HTML
Client->>Client: parse response HTML for new token
Client->>Client: update token input value in DOM
else token invalid
Security-->>Server: invalid
Server->>Server: generate refreshed token HTML (getTokenHTML)
Server-->>Client: 403 + token HTML
Client->>Client: parse response (or jqXHR.responseText) for token
Client->>Client: update token input value in DOM
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.46)PHPStan was skipped because the config uses disallowed 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 #1685 +/- ##
=============================================
+ Coverage 0 19.07% +19.07%
- Complexity 0 7473 +7473
=============================================
Files 0 620 +620
Lines 0 39323 +39323
=============================================
+ Hits 0 7500 +7500
- Misses 0 31823 +31823 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/js/admin.js`:
- Around line 104-106: Replace the substring existence check using indexOf with
the ES6 includes method: in the token refresh block where the variable reponse
is tested (the if that currently reads reponse && reponse.indexOf('<input') !==
-1), change it to use reponse.includes('<input') while preserving the
null/undefined guard (reponse && ...), so the condition becomes reponse &&
reponse.includes('<input'); keep the body that updates
$('#modules-token').html(reponse) unchanged.
- Around line 96-99: Replace the function-scoped declaration of $tokenInput with
a block-scoped constant: change the var $tokenInput =
$("input[name='XOOPS_TOKEN_REQUEST']").first(); to use const since $tokenInput
is never reassigned; ensure the rest of the CSRF injection logic that references
$tokenInput, data, and the input name "XOOPS_TOKEN_REQUEST" remains unchanged.
In `@htdocs/modules/system/js/module.js`:
- Around line 26-29: Replace the mutable declaration with an immutable one:
change "var $tokenInput = $("input[name='XOOPS_TOKEN_REQUEST']").first();" to
use const so $tokenInput is declared as a constant (const $tokenInput = ...);
keep the existing token serialization logic using encodeURIComponent and input
name 'XOOPS_TOKEN_REQUEST' unchanged.
- Around line 30-34: The PHP handler for the 'order' operation in
modulesadmin/main.php must echo the refreshed CSRF token HTML (use
$GLOBALS['xoopsSecurity']->getTokenHTML()) before it exits so the client
receives a new token; ensure this is added to the end of the 'order' case where
module weights are saved. On the client side, update the AJAX success check in
module.js: locate the $.post call to 'admin.php?fct=modulesadmin&op=order' and
change the response check that uses response.indexOf('<input') to
response.includes('<input'), and continue to update
$('#modules-token').html(response) when the includes() test passes so the page
token is replaced.
🪄 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: 97cd43d2-852c-42f4-9f25-b7b5c5897bba
📒 Files selected for processing (2)
htdocs/modules/system/js/admin.jshtdocs/modules/system/js/module.js
There was a problem hiding this comment.
Pull request overview
This PR aims to make System “Modules Admin” AJAX actions compatible with XOOPS CSRF validation by sending XOOPS_TOKEN_REQUEST with module reorder (drag-and-drop) and module status toggle requests, and attempting to refresh the token after each request.
Changes:
- Append
XOOPS_TOKEN_REQUESTto the sortable reorder POST inmodule.js. - Inject
XOOPS_TOKEN_REQUESTinto thesystem_setStatus()POST payload inadmin.js. - Attempt to refresh the CSRF token from the AJAX response after success.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| htdocs/modules/system/js/module.js | Adds CSRF token to module reorder AJAX POST and tries to refresh token from response. |
| htdocs/modules/system/js/admin.js | Adds CSRF token to system_setStatus() POSTs and tries to refresh token from response. |
| function system_setStatus(data, img, file) { | ||
| var $tokenInput = $("input[name='XOOPS_TOKEN_REQUEST']").first(); | ||
| if ($tokenInput.length) { | ||
| data[$tokenInput.attr('name')] = $tokenInput.val(); | ||
| } | ||
| // Post request | ||
| $.post(file, data, |
There was a problem hiding this comment.
system_setStatus() now pulls the first input[name="XOOPS_TOKEN_REQUEST"] in the document and mutates the outgoing data. On pages with multiple forms/tokens this can send an unrelated or already-consumed token (XOOPS tokens are cleared on successful validation). Consider scoping the selector to the form that triggered the action (or passing in a token container/selector), so the correct token is always sent.
| function system_setStatus(data, img, file) { | |
| var $tokenInput = $("input[name='XOOPS_TOKEN_REQUEST']").first(); | |
| if ($tokenInput.length) { | |
| data[$tokenInput.attr('name')] = $tokenInput.val(); | |
| } | |
| // Post request | |
| $.post(file, data, | |
| function system_setStatus(data, img, file, tokenScope) { | |
| var requestData = $.extend({}, data); | |
| var $tokenContainer = $(); | |
| var $tokenInput; | |
| if (tokenScope) { | |
| $tokenContainer = $(tokenScope).first(); | |
| } else { | |
| $tokenContainer = $('img#' + img).closest('form'); | |
| } | |
| $tokenInput = $tokenContainer.find("input[name='XOOPS_TOKEN_REQUEST']").first(); | |
| if (!$tokenInput.length) { | |
| $tokenInput = $("#modules-token").find("input[name='XOOPS_TOKEN_REQUEST']").first(); | |
| } | |
| if (!$tokenInput.length) { | |
| $tokenInput = $("input[name='XOOPS_TOKEN_REQUEST']").first(); | |
| } | |
| if ($tokenInput.length) { | |
| requestData[$tokenInput.attr('name')] = $tokenInput.val(); | |
| } | |
| // Post request | |
| $.post(file, requestData, |
…lookup Order AJAX handler now returns refreshed token HTML. Both JS files scope token lookup to form[name='moduleadmin'] instead of grabbing the first token on the page. Token refresh updates the existing hidden input value directly instead of targeting nonexistent #modules-token container. Use const and .includes() per SonarQube.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/js/admin.js`:
- Around line 96-109: The token lookup in system_setStatus (using const $form =
$('form[name="moduleadmin"]') and $tokenInput =
$form.find("input[name='XOOPS_TOKEN_REQUEST']").first()) is too narrow and skips
pages without that form; change the lookup to prefer the moduleadmin form but
fall back to a global selector such as
$('input[name="XOOPS_TOKEN_REQUEST"]').first() when $tokenInput.length is zero
so the AJAX payload always includes the CSRF token; update the logic that sets
data[$tokenInput.attr('name')] to only run when a token input is found and
ensure the token-update block that reads the returned response still uses the
same $tokenInput reference.
🪄 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: 3b026b9c-2ff3-4195-8294-eddaea63987e
📒 Files selected for processing (3)
htdocs/modules/system/admin/modulesadmin/main.phphtdocs/modules/system/js/admin.jshtdocs/modules/system/js/module.js
| function system_setStatus(data, img, file) { | ||
| const $form = $('form[name="moduleadmin"]'); | ||
| const $tokenInput = $form.find("input[name='XOOPS_TOKEN_REQUEST']").first(); | ||
| if ($tokenInput.length) { | ||
| data[$tokenInput.attr('name')] = $tokenInput.val(); | ||
| } | ||
| // Post request | ||
| $.post(file, data, | ||
| function (reponse, textStatus) { | ||
| if (textStatus == 'success') { | ||
| if (textStatus === 'success') { | ||
| if (reponse && reponse.includes('<input')) { | ||
| const $newToken = $(reponse).filter('input[name="XOOPS_TOKEN_REQUEST"]'); | ||
| if ($newToken.length && $tokenInput.length) { | ||
| $tokenInput.val($newToken.val()); | ||
| } | ||
| } |
There was a problem hiding this comment.
system_setStatus() now appends a CSRF token and tries to refresh it from the AJAX response, but the modulesadmin display / display_in_menu endpoints currently return an empty body (no new token). Since XOOPS tokens are single-use by default, any server-side xoopsSecurity->check() will consume the token and subsequent toggle requests will fail unless those endpoints also return a refreshed token (or a JSON payload containing it).
| } | ||
| // Return refreshed CSRF token for the next AJAX request | ||
| echo $GLOBALS['xoopsSecurity']->getTokenHTML(); | ||
| exit; | ||
| break; |
There was a problem hiding this comment.
The reorder handler returns a refreshed token, but it does not validate the incoming CSRF token before applying weight changes. If the goal is to enforce CSRF protection for this write operation, add a $xoopsSecurity->check() (and return an error/HTTP status on failure) before processing the POSTed mod[] data; otherwise sending/refreshing the token on the client doesn’t actually protect this endpoint.
Add server-side CSRF validation to the order handler before mutating weights. Return refreshed token HTML from display and display_in_menu handlers so repeated AJAX toggles don't fail. Fall back to global token input when moduleadmin form is not present on the page. Remove dead break after exit
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
htdocs/modules/system/admin/modulesadmin/main.php (2)
249-274:⚠️ Potential issue | 🔴 CriticalMissing CSRF validation before state mutation in the
displayhandler.The
displaycase togglesisactiveon modules and their blocks—a state-changing operation—but unlike theordercase, it does not call$GLOBALS['xoopsSecurity']->check()before performing the mutation. The client sends a token (per the JS changes), but the server never validates it.Either add
displayto the validation list at lines 36-40, or add an explicit check at the top of this case:🔒 Proposed fix
case 'display': + if (!$GLOBALS['xoopsSecurity']->check()) { + echo ''; + exit; + } // Get module handler /** `@var` XoopsModuleHandler $module_handler */ $module_handler = xoops_getHandler('module');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 249 - 274, Add CSRF validation to the 'display' case before mutating module state: call $GLOBALS['xoopsSecurity']->check() (or include 'display' in the existing validation list) at the start of the case and abort/return an error (and echo a fresh token) if the check fails, then proceed to use xoops_getHandler('module'), toggle isactive on the XoopsModule and its XoopsBlock entries and call xoops_setActiveModules() only after the token is valid; keep returning $GLOBALS['xoopsSecurity']->getTokenHTML() at the end for the next AJAX call.
276-290:⚠️ Potential issue | 🔴 CriticalMissing CSRF validation before state mutation in the
display_in_menuhandler.Same issue as
display: this handler modifiesshow_in_menuwithout validating the CSRF token first.🔒 Proposed fix
case 'display_in_menu': + if (!$GLOBALS['xoopsSecurity']->check()) { + echo ''; + exit; + } /** `@var` XoopsModuleHandler $module_handler */ $module_handler = xoops_getHandler('module');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 276 - 290, The display_in_menu case mutates module state without validating CSRF — before calling Request::getInt('mid', 0) or performing $module_handler->get/$module->setVar/$module_handler->insert, call the XOOPS security token validation ($GLOBALS['xoopsSecurity']->check(...) or the appropriate token check) and if the check fails set $error = true (or return an appropriate AJAX error) and echo $GLOBALS['xoopsSecurity']->getTokenHTML() without performing the toggle; keep the existing echo of getTokenHTML() for successful paths as well so each AJAX response returns a fresh token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@htdocs/modules/system/admin/modulesadmin/main.php`:
- Around line 249-274: Add CSRF validation to the 'display' case before mutating
module state: call $GLOBALS['xoopsSecurity']->check() (or include 'display' in
the existing validation list) at the start of the case and abort/return an error
(and echo a fresh token) if the check fails, then proceed to use
xoops_getHandler('module'), toggle isactive on the XoopsModule and its
XoopsBlock entries and call xoops_setActiveModules() only after the token is
valid; keep returning $GLOBALS['xoopsSecurity']->getTokenHTML() at the end for
the next AJAX call.
- Around line 276-290: The display_in_menu case mutates module state without
validating CSRF — before calling Request::getInt('mid', 0) or performing
$module_handler->get/$module->setVar/$module_handler->insert, call the XOOPS
security token validation ($GLOBALS['xoopsSecurity']->check(...) or the
appropriate token check) and if the check fails set $error = true (or return an
appropriate AJAX error) and echo $GLOBALS['xoopsSecurity']->getTokenHTML()
without performing the toggle; keep the existing echo of getTokenHTML() for
successful paths as well so each AJAX response returns a fresh token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0a197a7-0bf9-41f7-ad4b-4b553881ae15
📒 Files selected for processing (2)
htdocs/modules/system/admin/modulesadmin/main.phphtdocs/modules/system/js/admin.js
| case 'order': | ||
| // Get Module Handler | ||
| if (!$GLOBALS['xoopsSecurity']->check()) { | ||
| echo ''; |
There was a problem hiding this comment.
For invalid CSRF tokens this returns an empty 200 response, so the client can’t refresh the token and subsequent AJAX requests may keep failing until a full page reload. Consider returning a refreshed token (and ideally a non-200 status / error flag) even on failure so the UI can recover from stale/consumed tokens.
| echo ''; | |
| if (!headers_sent()) { | |
| http_response_code(403); | |
| } | |
| // Return refreshed CSRF token so the next AJAX request can recover | |
| echo $GLOBALS['xoopsSecurity']->getTokenHTML(); |
| } | ||
| // Return refreshed CSRF token for the next AJAX toggle | ||
| echo $GLOBALS['xoopsSecurity']->getTokenHTML(); | ||
| break; |
There was a problem hiding this comment.
This endpoint toggles module activation but does not validate the CSRF token. Since the client now sends XOOPS_TOKEN_REQUEST and you’re issuing new tokens in the response, the handler should also call $GLOBALS['xoopsSecurity']->check() (and stop processing on failure) to actually enforce CSRF protection and avoid accumulating unused tokens in the session.
| @@ -280,6 +285,8 @@ | |||
| $error = true; | |||
| } | |||
| } | |||
| // Return refreshed CSRF token for the next AJAX toggle | |||
| echo $GLOBALS['xoopsSecurity']->getTokenHTML(); | |||
| break; | |||
There was a problem hiding this comment.
This endpoint updates the module’s show_in_menu flag but does not validate the CSRF token. Add $GLOBALS['xoopsSecurity']->check() (and fail fast on mismatch) so the state change is CSRF-protected and the token refresh you echo is tied to a validated request.
| if (textStatus === 'success') { | ||
| if (reponse?.includes('<input')) { | ||
| const $newToken = $(reponse).filter('input[name="XOOPS_TOKEN_REQUEST"]'); | ||
| if ($newToken.length && $tokenInput.length) { | ||
| $tokenInput.val($newToken.val()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Token extraction uses $(reponse).filter('input[name="XOOPS_TOKEN_REQUEST"]'), which only matches top-level nodes. If the response includes any surrounding markup, the token input won’t be found and the next POST will re-use a stale token. Use a selector that searches descendants (e.g., find) or parse into a container before querying.
Add CSRF validation to display and display_in_menu handlers. Return 403 with fresh token on CSRF failure so client can recover. Parse token response into container div with .find() instead of .filter() to handle wrapped markup reliably
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/js/module.js`:
- Around line 31-38: The AJAX post call that currently only handles success
should also handle error responses (notably HTTP 403) so the CSRF recovery token
in the response body isn't discarded; update the $.post call around the modules
order request to use an error handler (either chain .fail() on the returned
jqXHR or switch to $.ajax with an error callback) that checks for jqXHR.status
=== 403, parses jqXHR.responseText the same way the success branch does (create
a temporary element, find input[name="XOOPS_TOKEN_REQUEST"] and read its value)
and then update $tokenInput.val(...) (same variables used in the existing
success path) so the fresh token is preserved on CSRF failures.
- Around line 31-38: Rapid sequential drag-drop can reuse a consumed token
causing 403s; before sending the POST to 'admin.php?fct=modulesadmin&op=order'
disable the sortable UI (the element initialized with .sortable) to prevent new
reorder events, then re-enable it when the AJAX completes (in
success/error/always) and update $tokenInput from the response as currently done
in the $.post callback; use the sortable instance's disable/enable methods
around the $.post call to ensure only one reorder request is in flight.
🪄 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: 9d46f2be-d759-4e08-b5f3-1f9981b0e30e
📒 Files selected for processing (3)
htdocs/modules/system/admin/modulesadmin/main.phphtdocs/modules/system/js/admin.jshtdocs/modules/system/js/module.js
| $.post( 'admin.php?fct=modulesadmin&op=order', list, function (response) { | ||
| if (response?.includes('<input')) { | ||
| const $newToken = $('<div>').html(response).find('input[name="XOOPS_TOKEN_REQUEST"]').first(); | ||
| if ($newToken.length && $tokenInput.length) { | ||
| $tokenInput.val($newToken.val()); |
There was a problem hiding this comment.
$.post() success callback will not run for 403 responses (jQuery treats 4xx/5xx as errors). Since the server returns a refreshed token on CSRF failure with HTTP 403, the token-refresh logic here won’t execute when the token is missing/expired. Add an error/fail handler that parses xhr.responseText for the new XOOPS_TOKEN_REQUEST and (for reorder) consider cancelling/reverting the sort when the request fails so the UI doesn’t show an unsaved order.
| if (!headers_sent()) { | ||
| http_response_code(403); | ||
| } |
There was a problem hiding this comment.
These CSRF failure branches set HTTP 403 but return the refreshed token as HTML. Because the JS currently uses $.post(..., successCallback), the success callback won’t run on 403, so the client won’t actually consume this refreshed token unless it also handles the error path. Either ensure the client updates the token on AJAX errors, or consider returning 200 with an explicit failure indicator while still providing the refreshed token.
| if (!headers_sent()) { | |
| http_response_code(403); | |
| } | |
| // Return a refreshed CSRF token using a success response so the | |
| // existing AJAX success callback can consume it for the next request. |
extractOperationSection stops at the first break which is now the CSRF failure path. Assert CSRF check in the extracted section and verify toggle logic against the full source code
| const $form = $('form[name="moduleadmin"]'); | ||
| const $tokenInput = $form.length | ||
| ? $form.find("input[name='XOOPS_TOKEN_REQUEST']").first() | ||
| : $("input[name='XOOPS_TOKEN_REQUEST']").first(); | ||
| if ($tokenInput.length) { | ||
| data[$tokenInput.attr('name')] = $tokenInput.val(); | ||
| } | ||
| // Post request | ||
| $.post(file, data, | ||
| function (reponse, textStatus) { | ||
| if (textStatus == 'success') { | ||
| if (textStatus === 'success') { | ||
| if (reponse?.includes('<input')) { | ||
| const $newToken = $('<div>').html(reponse).find('input[name="XOOPS_TOKEN_REQUEST"]').first(); | ||
| if ($newToken.length && $tokenInput.length) { | ||
| $tokenInput.val($newToken.val()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Token refresh is currently only attempted in the
| function (reponse, textStatus) { | ||
| if (textStatus == 'success') { | ||
| if (textStatus === 'success') { | ||
| if (reponse?.includes('<input')) { |
There was a problem hiding this comment.
This introduces optional chaining and String.prototype.includes (e.g., response?.includes(...)). These constructs aren’t used elsewhere in modules/system/js and will cause a hard syntax/runtime failure in older JS engines. If admin pages need to keep broad browser compatibility, prefer a more compatible guard (e.g., typeof response === 'string' and indexOf) or ensure the required polyfills/transpilation are in place.
| if (reponse?.includes('<input')) { | |
| if (typeof reponse === 'string' && reponse.indexOf('<input') !== -1) { |
| $.post( 'admin.php?fct=modulesadmin&op=order', list, function (response) { | ||
| if (response?.includes('<input')) { | ||
| const $newToken = $('<div>').html(response).find('input[name="XOOPS_TOKEN_REQUEST"]').first(); | ||
| if ($newToken.length && $tokenInput.length) { | ||
| $tokenInput.val($newToken.val()); | ||
| } |
There was a problem hiding this comment.
This uses optional chaining and String.prototype.includes (response?.includes(...)). modules/system/js doesn’t otherwise use these newer constructs, and they will break parsing/runtime in older browsers. Consider using a more compatible check (e.g., typeof response === 'string' and indexOf) or ensure the admin JS pipeline/polyfills guarantee support.
Extract _refreshToken helper and call it from both .done() and .fail() callbacks so 403 CSRF responses still update the token. Disable sortable during the order request and re-enable in .always() to prevent token race from rapid successive drags.
| if (!$GLOBALS['xoopsSecurity']->check()) { | ||
| if (!headers_sent()) { | ||
| http_response_code(403); | ||
| } | ||
| echo $GLOBALS['xoopsSecurity']->getTokenHTML(); |
There was a problem hiding this comment.
http_response_code(403) is guarded by headers_sent(), but this script calls xoops_cp_header() before the switch, so headers are very likely already sent for these AJAX ops. That makes the 403 ineffective (client will see 200 even on CSRF failure). Consider handling order/display/display_in_menu before xoops_cp_header() (early-return for AJAX endpoints) so status codes and minimal responses work reliably, or otherwise avoid relying on http_response_code() here.
| } | ||
|
|
||
| function _refreshToken(html, $tokenInput) { | ||
| if (html?.includes('<input') && $tokenInput.length) { |
There was a problem hiding this comment.
html?.includes(...) uses optional chaining, which is not used elsewhere in the system JS and will break on older browsers if the admin UI still targets them. Prefer an explicit check like typeof html === 'string' (or html && ...) to keep JS syntax consistent with the rest of this file.
| if (html?.includes('<input') && $tokenInput.length) { | |
| if (typeof html === 'string' && html.includes('<input') && $tokenInput.length) { |
| // Toggle logic follows the CSRF block in the full source | ||
| $this->assertMatchesRegularExpression( | ||
| '/\$old\s+=\s+\$module->getVar\(\'isactive\'\);/', | ||
| $displaySection, | ||
| $this->sourceCode, | ||
| 'Should get current isactive state' | ||
| ); | ||
|
|
||
| $this->assertStringContainsString( | ||
| "setVar('isactive', !\$old)", | ||
| $displaySection, | ||
| $this->sourceCode, | ||
| 'Should toggle isactive state' |
There was a problem hiding this comment.
These assertions were changed to search the entire controller source ($this->sourceCode) instead of the extracted display case. That weakens the test because it could pass if the same snippets appear in another operation. A more robust fix would be to improve extractOperationSection() to capture the full case body (e.g., stop at the next case or the last break; before the next case) so the toggle logic is validated within the display operation.
| // Toggle logic follows the CSRF block in the full source | ||
| $this->assertMatchesRegularExpression( | ||
| '/\$old\s+=\s+\(int\)\s*\$module->getVar\(\'show_in_menu\'\);/', | ||
| $displayInMenuSection, | ||
| $this->sourceCode, | ||
| 'Should get current show_in_menu' | ||
| ); | ||
|
|
||
| $this->assertStringContainsString( | ||
| "setVar('show_in_menu',", | ||
| $displayInMenuSection, | ||
| $this->sourceCode, | ||
| 'Should toggle show_in_menu' | ||
| ); |
There was a problem hiding this comment.
Same issue as display: searching $this->sourceCode instead of the extracted display_in_menu section reduces test specificity and may hide regressions. Consider adjusting extractOperationSection() to include the full case body even when an early break; exists inside an if block.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
htdocs/modules/system/js/admin.js (1)
104-131:⚠️ Potential issue | 🟠 Major
system_setStatus()still allows same-token double submits.Unlike the sortable path, this helper leaves the control active until the response comes back. Two fast clicks can therefore send the same pre-refresh token twice, so one request falls into the 403 path and silently no-ops. Please add an in-flight guard or disable the image before
$.post(...), then clear it in.always().♻️ Proposed fix
function system_setStatus(data, img, file) { const $form = $('form[name="moduleadmin"]'); const $tokenInput = $form.length ? $form.find("input[name='XOOPS_TOKEN_REQUEST']").first() : $("input[name='XOOPS_TOKEN_REQUEST']").first(); + const $img = $('img#' + img); + if ($img.data('requestInFlight')) { + return; + } if ($tokenInput.length) { data[$tokenInput.attr('name')] = $tokenInput.val(); } + $img.data('requestInFlight', true); // Post request $.post(file, data) .done(function (response) { _refreshToken(response, $tokenInput); - $('img#' + img).hide(); + $img.hide(); $('#loading_' + img).show(); setTimeout(function () { $('#loading_' + img).hide(); - $('img#' + img).fadeIn('fast'); + $img.fadeIn('fast'); }, 500); // Change image src - if ($('img#' + img).attr("src") == IMG_ON) { - $('img#' + img).attr("src", IMG_OFF); + if ($img.attr("src") == IMG_ON) { + $img.attr("src", IMG_OFF); } else { - $('img#' + img).attr("src", IMG_ON); + $img.attr("src", IMG_ON); } }) .fail(function (jqXHR) { _refreshToken(jqXHR.responseText, $tokenInput); + }) + .always(function () { + $img.removeData('requestInFlight'); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/js/admin.js` around lines 104 - 131, The system_setStatus function allows duplicate submissions—add an in-flight guard or disable the target control before calling $.post to prevent double-clicks: inside system_setStatus (referencing the img id parameter and IMG_ON/IMG_OFF logic) set a flag on the image element (or disable it via jQuery .data()/.prop()) or add a module-level map keyed by img to track in-flight requests, then skip sending if already in-flight; clear that flag (or re-enable the image) in the promise chain .always() after the $.post completes and continue to call _refreshToken in done/fail as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php`:
- Around line 738-755: The toggle assertions are using $this->sourceCode
(file-wide) instead of the scoped section; update extractOperationSection() so
it stops at the next "case" label rather than the first nested "break;" (so
display/display_in_menu sections don't run past their switch branch), then
change the two assertions that currently inspect $this->sourceCode to assert
against $displaySection and $displayInMenuSection respectively (e.g., the checks
for \$old = \$module->getVar('isactive') and setVar('isactive', !\$old) should
target the per-operation section returned by extractOperationSection()).
---
Duplicate comments:
In `@htdocs/modules/system/js/admin.js`:
- Around line 104-131: The system_setStatus function allows duplicate
submissions—add an in-flight guard or disable the target control before calling
$.post to prevent double-clicks: inside system_setStatus (referencing the img id
parameter and IMG_ON/IMG_OFF logic) set a flag on the image element (or disable
it via jQuery .data()/.prop()) or add a module-level map keyed by img to track
in-flight requests, then skip sending if already in-flight; clear that flag (or
re-enable the image) in the promise chain .always() after the $.post completes
and continue to call _refreshToken in done/fail 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: 93129688-8a7d-4b3a-956f-a67fee798675
📒 Files selected for processing (3)
htdocs/modules/system/js/admin.jshtdocs/modules/system/js/module.jstests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
extractOperationSection now stops at the next case label instead of the first break, so CSRF early-exit blocks don't truncate the section. Removed http_response_code(403) from AJAX handlers since headers are already sent by xoops_cp_header before the switch statement.
| echo $GLOBALS['xoopsSecurity']->getTokenHTML(); | ||
| break; |
There was a problem hiding this comment.
On CSRF validation failure this echoes a refreshed token and breaks, which still results in a 200 OK response. Since system_setStatus() toggles the status icon in the .done(...) branch, a CSRF-rejected toggle can incorrectly update the UI. Consider sending a non-2xx status (e.g., 403) and then exiting after emitting the new token so the client’s .fail(...) path runs and the UI stays unchanged.
| echo $GLOBALS['xoopsSecurity']->getTokenHTML(); | |
| break; | |
| header('HTTP/1.1 403 Forbidden'); | |
| echo $GLOBALS['xoopsSecurity']->getTokenHTML(); | |
| exit(); |
| if (!$GLOBALS['xoopsSecurity']->check()) { | ||
| echo $GLOBALS['xoopsSecurity']->getTokenHTML(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Same issue as the other AJAX endpoints: CSRF failure echoes a new token but returns 200 OK. That will cause the caller (system_setStatus) to treat the request as success and flip the menu icon even though the DB update didn’t run. Consider returning a 403 (or another non-2xx) and exiting after outputting the refreshed token so the .fail(...) handler runs.
extractOperationSection fell back to the first closing brace when no next case label exists, truncating the update_ok section. Use strrpos to find the last break; for the final case in the switch.
| case 'display_in_menu': | ||
| // Get module handler | ||
|
|
||
| if (!$GLOBALS['xoopsSecurity']->check()) { |
There was a problem hiding this comment.
Same issue as display: on CSRF failure this returns token HTML with HTTP 200, which will be treated as success by jQuery and can result in the menu visibility icon toggling client-side even though no change was saved. Set an error status (e.g. 403) when $xoopsSecurity->check() fails so the client remains in .fail() and does not apply UI changes.
| if (!$GLOBALS['xoopsSecurity']->check()) { | |
| if (!$GLOBALS['xoopsSecurity']->check()) { | |
| http_response_code(403); |
Move order, display, and display_in_menu handlers before xoops_cp_header() so HTTP 403 status works on CSRF failure. Remove duplicate cases from switch. Update tests to search full source since AJAX operations are no longer inside the switch.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php (1)
735-756:⚠️ Potential issue | 🟠 MajorRestore branch-scoped assertions for the early AJAX cases.
Because
extractOperationSection()still looks for"\n case ", it cannot terminate the earlier AJAX switch cases. That forced these checks back to$this->sourceCodescans, so they now pass even iforder/display*logic drifts out of the matching branch or outside the CSRF guard.♻️ Suggested direction
- $nextCase = strpos($this->sourceCode, "\n case ", $startPos); + $remainder = substr($this->sourceCode, $startPos); + $nextCase = false; + if (preg_match('/^\s*case\s+[\'"][^\'"]+[\'"]:/m', $remainder, $match, PREG_OFFSET_CAPTURE)) { + $nextCase = $startPos + $match[0][1]; + } if ($nextCase !== false) { $endPos = $nextCase; } else {After that, switch the
display,display_in_menu, andorderassertions back to extracted case sections instead of$this->sourceCode.As per coding guidelines,
tests/**/*.php: Review test code for proper assertions, test isolation, and edge case coverage.Also applies to: 766-781, 795-829, 1139-1148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php` around lines 735 - 756, Restore the branch-scoped assertions by calling extractOperationSection() for each AJAX case and running the assertions against the returned section instead of $this->sourceCode: locate usages of extractOperationSection() and use it to get the 'display', 'display_in_menu' and 'order' case sections, then assert that each section contains "case 'display':" (or the appropriate case label), the "xoopsSecurity']->check()" CSRF check, the regex /\$old\s+=\s+\$module->getVar\('isactive'\);/ and the "setVar('isactive', !\$old)" line within that extracted section so the checks fail if the logic drifts out of the matching switch branch or outside the CSRF guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php`:
- Around line 735-756: Restore the branch-scoped assertions by calling
extractOperationSection() for each AJAX case and running the assertions against
the returned section instead of $this->sourceCode: locate usages of
extractOperationSection() and use it to get the 'display', 'display_in_menu' and
'order' case sections, then assert that each section contains "case 'display':"
(or the appropriate case label), the "xoopsSecurity']->check()" CSRF check, the
regex /\$old\s+=\s+\$module->getVar\('isactive'\);/ and the "setVar('isactive',
!\$old)" line within that extracted section so the checks fail if the logic
drifts out of the matching switch branch or outside the CSRF guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5c068ca-84a8-448e-be07-dde9d0e97346
📒 Files selected for processing (4)
htdocs/modules/system/admin/modulesadmin/main.phphtdocs/modules/system/js/admin.jshtdocs/modules/system/js/module.jstests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
| function system_setStatus(data, img, file) { | ||
| const $form = $('form[name="moduleadmin"]'); | ||
| const $tokenInput = $form.length | ||
| ? $form.find("input[name='XOOPS_TOKEN_REQUEST']").first() | ||
| : $("input[name='XOOPS_TOKEN_REQUEST']").first(); | ||
| if ($tokenInput.length) { | ||
| data[$tokenInput.attr('name')] = $tokenInput.val(); | ||
| } | ||
| // Post request | ||
| $.post(file, data, | ||
| function (reponse, textStatus) { | ||
| if (textStatus == 'success') { | ||
| $('img#' + img).hide(); | ||
| $('#loading_' + img).show(); | ||
| setTimeout(function () { | ||
| $('#loading_' + img).hide(); | ||
| $('img#' + img).fadeIn('fast'); | ||
| }, 500); | ||
| // Change image src | ||
| if ($('img#' + img).attr("src") == IMG_ON) { | ||
| $('img#' + img).attr("src", IMG_OFF); | ||
| } else { | ||
| $('img#' + img).attr("src", IMG_ON); | ||
| } | ||
| $.post(file, data) | ||
| .done(function (response) { | ||
| _refreshToken(response, $tokenInput); | ||
| $('img#' + img).hide(); | ||
| $('#loading_' + img).show(); | ||
| setTimeout(function () { | ||
| $('#loading_' + img).hide(); | ||
| $('img#' + img).fadeIn('fast'); | ||
| }, 500); | ||
| // Change image src | ||
| if ($('img#' + img).attr("src") == IMG_ON) { | ||
| $('img#' + img).attr("src", IMG_OFF); | ||
| } else { | ||
| $('img#' + img).attr("src", IMG_ON); | ||
| } | ||
| }) | ||
| .fail(function (jqXHR) { | ||
| _refreshToken(jqXHR.responseText, $tokenInput); | ||
| }); |
There was a problem hiding this comment.
Serialize status-toggle requests too.
system_setStatus() still allows overlapping POSTs, and the token is only refreshed after the response comes back. A second rapid click on any status icon can therefore reuse the still-stale token and 403 before _refreshToken() has a chance to update it. Since this helper is shared by multiple admin toggle UIs, it needs the same in-flight protection that module.js now has for reorders.
♻️ Minimal guard
+let statusRequestInFlight = false;
+
function system_setStatus(data, img, file) {
+ if (statusRequestInFlight) {
+ return;
+ }
+ statusRequestInFlight = true;
const $form = $('form[name="moduleadmin"]');
const $tokenInput = $form.length
? $form.find("input[name='XOOPS_TOKEN_REQUEST']").first()
: $("input[name='XOOPS_TOKEN_REQUEST']").first();
if ($tokenInput.length) {
data[$tokenInput.attr('name')] = $tokenInput.val();
}
// Post request
$.post(file, data)
.done(function (response) {
_refreshToken(response, $tokenInput);
...
})
.fail(function (jqXHR) {
_refreshToken(jqXHR.responseText, $tokenInput);
+ })
+ .always(function () {
+ statusRequestInFlight = false;
});
}| $this->assertStringContainsString( | ||
| "xoopsSecurity']->check()", | ||
| $this->sourceCode, | ||
| 'Should validate CSRF token' |
There was a problem hiding this comment.
The CSRF assertion here is too broad: searching for "xoopsSecurity']->check()" in the full source will pass even if the AJAX block doesn’t validate tokens, because main.php also calls $GLOBALS['xoopsSecurity']->check() for non-AJAX ops (confirm/submit/etc). Consider asserting the check occurs within the AJAX-only block (e.g., regex that matches the in_array($op, ['order','display','display_in_menu'], true) block and a check() call nearby).
| $this->assertStringContainsString( | |
| "xoopsSecurity']->check()", | |
| $this->sourceCode, | |
| 'Should validate CSRF token' | |
| $this->assertMatchesRegularExpression( | |
| '/in_array\(\s*\$op\s*,\s*\[\s*\'order\'\s*,\s*\'display\'\s*,\s*\'display_in_menu\'\s*\]\s*,\s*true\s*\)\s*\)\s*\{[\s\S]*?\$GLOBALS\[\'xoopsSecurity\'\]->check\(\)/', | |
| $this->sourceCode, | |
| 'Should validate CSRF token in the AJAX operations block' |
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/admin/modulesadmin/main.php`:
- Around line 80-88: The 'display_in_menu' case currently assumes
$module_handler->get($module_id) returns an object and calls $module->getVar(),
which can throw a TypeError if null; update the case 'display_in_menu' block to
mirror other cases by checking that $module = $module_handler->get($module_id)
is not null before using $module->getVar(), $module->setVar('show_in_menu',
...), and $module_handler->insert($module), and only perform the toggle/insert
when $module is truthy.
- Around line 65-79: The code toggling a module's isactive state should guard
against a null module and handle exceptions from XoopsBlock::getByModule():
first check the result of $module_handler->get($module_id) and bail (return an
AJAX error response with a fresh token) if null; when calling
XoopsBlock::getByModule($module_id) wrap it in a try/catch(\Throwable $t) to
catch DB/runtime errors, return a graceful AJAX error (with new token) on
failure, and change the toggle to explicit integer assignment ($old ? 0 : 1)
when calling $module->setVar('isactive', ...) and $block->setVar('isactive',
...); ensure xoops_setActiveModules() is only called after successful updates.
- Around line 52-64: The code calls $module_handler->get($order) and then
unconditionally calls $module->setVar(...) and $module_handler->insert(...),
which will throw a TypeError when get() returns null/false for a non-existent
module; update the 'order' case to check the result of
$module_handler->get($order) (e.g., if $module is truthy) before calling setVar
and insert, skip or log invalid IDs from Request::getArray('mod', [], 'POST'),
and ensure the loop continues safely without operating on null.
🪄 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: 62cfe20f-d450-4181-8054-c8baa87302c7
📒 Files selected for processing (1)
htdocs/modules/system/admin/modulesadmin/main.php
| case 'order': | ||
| if (Request::hasVar('mod', 'POST')) { | ||
| $i = 1; | ||
| foreach (Request::getArray('mod', [], 'POST') as $order) { | ||
| if ($order > 0) { | ||
| $module = $module_handler->get($order); | ||
| $module->setVar('weight', $i); | ||
| $module_handler->insert($module); | ||
| ++$i; | ||
| } | ||
| } | ||
| } | ||
| break; |
There was a problem hiding this comment.
Missing null check after $module_handler->get() will cause TypeError.
If $order contains an ID for a non-existent module, get() returns null (or false), and the subsequent setVar() call on line 58 will throw a TypeError in PHP 8+. This applies to untrusted AJAX input.
🛡️ Proposed fix with null guard
case 'order':
if (Request::hasVar('mod', 'POST')) {
$i = 1;
foreach (Request::getArray('mod', [], 'POST') as $order) {
- if ($order > 0) {
- $module = $module_handler->get($order);
+ $order = (int) $order;
+ if ($order > 0) {
+ $module = $module_handler->get($order);
+ if (null === $module) {
+ continue;
+ }
$module->setVar('weight', $i);
$module_handler->insert($module);
++$i;
}
}
}
break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 52 - 64, The
code calls $module_handler->get($order) and then unconditionally calls
$module->setVar(...) and $module_handler->insert(...), which will throw a
TypeError when get() returns null/false for a non-existent module; update the
'order' case to check the result of $module_handler->get($order) (e.g., if
$module is truthy) before calling setVar and insert, skip or log invalid IDs
from Request::getArray('mod', [], 'POST'), and ensure the loop continues safely
without operating on null.
| case 'display': | ||
| $module_id = Request::getInt('mid', 0); | ||
| if ($module_id > 0) { | ||
| $module = $module_handler->get($module_id); | ||
| $old = $module->getVar('isactive'); | ||
| $module->setVar('isactive', !$old); | ||
| $module_handler->insert($module); | ||
| $blocks = XoopsBlock::getByModule($module_id); | ||
| foreach ($blocks as $block) { | ||
| $block->setVar('isactive', !$old); | ||
| $block->store(); | ||
| } | ||
| xoops_setActiveModules(); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Same null check issue; also consider exception handling for XoopsBlock::getByModule().
-
Line 68:
$module_handler->get($module_id)may returnnullfor an invalid ID, causing a fatal error on line 69. -
Line 72: Per the
XoopsBlock::getByModule()implementation (context snippet 4), a database query failure throwsRuntimeException. An uncaught exception here would return a 500 error instead of a graceful AJAX response with a fresh token.
🛡️ Proposed fix with guards
case 'display':
$module_id = Request::getInt('mid', 0);
if ($module_id > 0) {
$module = $module_handler->get($module_id);
+ if (null === $module) {
+ break;
+ }
$old = $module->getVar('isactive');
- $module->setVar('isactive', !$old);
+ $module->setVar('isactive', $old ? 0 : 1);
$module_handler->insert($module);
- $blocks = XoopsBlock::getByModule($module_id);
- foreach ($blocks as $block) {
- $block->setVar('isactive', !$old);
- $block->store();
+ try {
+ $blocks = XoopsBlock::getByModule($module_id);
+ foreach ($blocks as $block) {
+ $block->setVar('isactive', $old ? 0 : 1);
+ $block->store();
+ }
+ } catch (\Throwable $e) {
+ trigger_error('Block update failed: ' . $e->getMessage(), E_USER_WARNING);
}
xoops_setActiveModules();
}
break;Note: Changed !$old to explicit $old ? 0 : 1 for clearer integer toggle semantics, since isactive is stored as an integer field. As per coding guidelines, prefer \Throwable in catch blocks over \Exception to cover both exceptions and errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 65 - 79, The
code toggling a module's isactive state should guard against a null module and
handle exceptions from XoopsBlock::getByModule(): first check the result of
$module_handler->get($module_id) and bail (return an AJAX error response with a
fresh token) if null; when calling XoopsBlock::getByModule($module_id) wrap it
in a try/catch(\Throwable $t) to catch DB/runtime errors, return a graceful AJAX
error (with new token) on failure, and change the toggle to explicit integer
assignment ($old ? 0 : 1) when calling $module->setVar('isactive', ...) and
$block->setVar('isactive', ...); ensure xoops_setActiveModules() is only called
after successful updates.
| case 'display_in_menu': | ||
| $module_id = Request::getInt('mid', 0); | ||
| if ($module_id > 0) { | ||
| $module = $module_handler->get($module_id); | ||
| $old = (int) $module->getVar('show_in_menu'); | ||
| $module->setVar('show_in_menu', $old ? 0 : 1); | ||
| $module_handler->insert($module); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Add null guard for consistency with other cases.
Line 83: Same pattern—$module_handler->get($module_id) may return null for an invalid module ID, causing a fatal TypeError when calling getVar() on line 84.
🛡️ Proposed fix
case 'display_in_menu':
$module_id = Request::getInt('mid', 0);
if ($module_id > 0) {
$module = $module_handler->get($module_id);
+ if (null === $module) {
+ break;
+ }
$old = (int) $module->getVar('show_in_menu');
$module->setVar('show_in_menu', $old ? 0 : 1);
$module_handler->insert($module);
}
break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 80 - 88, The
'display_in_menu' case currently assumes $module_handler->get($module_id)
returns an object and calls $module->getVar(), which can throw a TypeError if
null; update the case 'display_in_menu' block to mirror other cases by checking
that $module = $module_handler->get($module_id) is not null before using
$module->getVar(), $module->setVar('show_in_menu', ...), and
$module_handler->insert($module), and only perform the toggle/insert when
$module is truthy.
| $i = 1; | ||
| foreach (Request::getArray('mod', [], 'POST') as $order) { | ||
| if ($order > 0) { | ||
| $module = $module_handler->get($order); |
There was a problem hiding this comment.
$module_handler->get($order) can return false (module not found). The subsequent $module->setVar(...) will then fatal. Add an is_object($module) guard (and consider returning an error status) before using the module object for AJAX reorder input.
| $module = $module_handler->get($order); | |
| $module = $module_handler->get($order); | |
| if (!is_object($module)) { | |
| trigger_error( | |
| basename(__FILE__) . ': Unable to reorder module. Module not found for id ' . (int)$order, | |
| E_USER_WARNING | |
| ); | |
| http_response_code(404); | |
| echo $GLOBALS['xoopsSecurity']->getTokenHTML(); | |
| exit; | |
| } |
| $module_id = Request::getInt('mid', 0); | ||
| if ($module_id > 0) { | ||
| $module = $module_handler->get($module_id); | ||
| $old = $module->getVar('isactive'); | ||
| $module->setVar('isactive', !$old); | ||
| $module_handler->insert($module); |
There was a problem hiding this comment.
$module_handler->get($module_id) can return false. This path then calls $module->getVar() / setVar() and will fatal on invalid/unknown mid. Add an is_object($module) check (and ideally return a non-200 status) before toggling module/block state.
| $old = (int) $module->getVar('show_in_menu'); | ||
| $module->setVar('show_in_menu', $old ? 0 : 1); | ||
| $module_handler->insert($module); |
There was a problem hiding this comment.
$module_handler->get($module_id) can return false. Calling $module->getVar() / setVar() without checking will fatal if an invalid/unknown mid is posted. Add an is_object($module) guard before toggling show_in_menu.
| $old = (int) $module->getVar('show_in_menu'); | |
| $module->setVar('show_in_menu', $old ? 0 : 1); | |
| $module_handler->insert($module); | |
| if (is_object($module)) { | |
| $old = (int) $module->getVar('show_in_menu'); | |
| $module->setVar('show_in_menu', $old ? 0 : 1); | |
| $module_handler->insert($module); | |
| } else { | |
| trigger_error('Invalid module id in ' . basename(__FILE__) . ': ' . $module_id, E_USER_WARNING); | |
| } |
| $this->assertStringContainsString( | ||
| "xoopsSecurity']->check()", | ||
| $this->sourceCode, | ||
| 'Should validate CSRF token' | ||
| ); |
There was a problem hiding this comment.
This CSRF assertion is not scoped to the AJAX operations: xoopsSecurity->check() already exists elsewhere in the controller, so this test would pass even if the display AJAX path didn't validate the token. Consider extracting the AJAX block (or the display case body) and asserting check() occurs within that specific block.
| public function testOrderOperationExitsAfterProcessing(): void | ||
| { | ||
| $orderSection = $this->extractOperationSection('order'); | ||
|
|
||
| $this->assertStringContainsString( | ||
| 'exit;', | ||
| $orderSection, | ||
| $this->sourceCode, | ||
| 'Should exit after processing order (AJAX endpoint)' | ||
| ); |
There was a problem hiding this comment.
Asserting 'exit;' against the entire source is too broad (there are multiple exit; statements unrelated to the order AJAX endpoint). Scope this assertion to the order AJAX block/case so the test fails if the endpoint stops exiting early.
Module toggle and drag-and-drop reorder AJAX calls did not send
CSRF tokens, causing failures when token validation is enforced.
Send token with each request and refresh from response.
Summary by CodeRabbit
Bug Fixes
Tests