From 97896f27ad35f913c7f9c944c84165ecc273a505 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Sun, 1 Nov 2026 11:12:34 -0500 Subject: [PATCH 1/7] fix(security): require tokens for AJAX admin toggles, account self-delete and profile deactivation; fix blocks admin PHP 8 parse error --- .../modules/avatars/admin/avatar_custom.php | 8 ++++--- .../modules/avatars/admin/avatar_system.php | 8 ++++--- .../templates/admin/avatars_admin_custom.tpl | 2 +- .../templates/admin/avatars_admin_system.tpl | 2 +- htdocs/modules/images/admin/header.php | 7 ++---- htdocs/modules/page/admin/content.php | 3 +++ htdocs/modules/page/admin/permissions.php | 7 +----- htdocs/modules/page/admin/related.php | 2 ++ .../templates/admin/page_admin_content.tpl | 4 ++-- .../templates/admin/page_admin_related.tpl | 2 +- htdocs/modules/profile/admin/deactivate.php | 16 +++++++++----- htdocs/modules/profile/admin/step.php | 2 ++ .../profile/templates/admin/steplist.tpl | 2 +- .../profile/templates/profile_userinfo.tpl | 1 + htdocs/modules/profile/user.php | 3 +++ htdocs/modules/profile/userinfo.php | 1 + htdocs/modules/smilies/admin/smilies.php | 2 ++ .../templates/admin/smilies_smilies.tpl | 2 +- htdocs/modules/system/admin.php | 2 ++ .../modules/system/admin/blocksadmin/main.php | 9 ++++++-- .../modules/system/admin/extensions/main.php | 1 + .../system/admin/modulesadmin/main.php | 4 ++++ htdocs/modules/system/blocks/main.php | 1 + htdocs/modules/system/include/functions.php | 17 ++++++++++++++ htdocs/modules/system/js/blocks.js | 12 +++++----- htdocs/modules/system/js/module.js | 6 ++--- .../templates/admin/system_blocks_item.tpl | 2 +- .../system/templates/admin/system_index.tpl | 2 +- htdocs/modules/userrank/admin/userrank.php | 2 ++ .../userrank/templates/admin/userrank.tpl | 2 +- htdocs/user.php | 3 +++ .../unit/xoopsLib/Xoops/Core/SecurityTest.php | 16 ++++++++++++++ xoops_lib/Xoops/Core/Security.php | 22 +++++++++++++++++++ 33 files changed, 132 insertions(+), 43 deletions(-) diff --git a/htdocs/modules/avatars/admin/avatar_custom.php b/htdocs/modules/avatars/admin/avatar_custom.php index ae4db9f50..153c82f00 100644 --- a/htdocs/modules/avatars/admin/avatar_custom.php +++ b/htdocs/modules/avatars/admin/avatar_custom.php @@ -89,6 +89,7 @@ } } $xoops->tpl()->assign('avatars_list', $avatar_list); + $xoops->tpl()->assign('token', $xoops->security()->createToken(3600)); // Display Page Navigation if ($avatar_count > $nb_avatars) { $nav = new XoopsPageNav($avatar_count, $nb_avatars, $start, 'start', 'op=list'); @@ -212,9 +213,10 @@ break; case "update_display": - $avatar_id = Request::getInt('avatar_id', 0); - if ($avatar_id > 0) { - $obj = $avatar_Handler->get($avatar_id); + $xoops->security()->requireAjaxToken(); + $avatar_id = Request::getInt('avatar_id', 0, 'POST'); + $obj = $avatar_id > 0 ? $avatar_Handler->get($avatar_id) : null; + if (is_object($obj)) { $old = $obj->getVar('avatar_display'); $obj->setVar('avatar_display', !$old); if ($avatar_Handler->insert($obj)) { diff --git a/htdocs/modules/avatars/admin/avatar_system.php b/htdocs/modules/avatars/admin/avatar_system.php index 43a019710..c272f2499 100644 --- a/htdocs/modules/avatars/admin/avatar_system.php +++ b/htdocs/modules/avatars/admin/avatar_system.php @@ -84,6 +84,7 @@ $avatar_list[$i]['count'] = count($avatar_Handler->getUser($avatars_arr[$i])); } $xoops->tpl()->assign('avatars_list', $avatar_list); + $xoops->tpl()->assign('token', $xoops->security()->createToken(3600)); // Display Page Navigation if ($avatar_count > $nb_avatars) { $nav = new XoopsPageNav($avatar_count, $nb_avatars, $start, 'start', 'op=list'); @@ -219,9 +220,10 @@ break; case "update_display": - $avatar_id = Request::getInt('avatar_id', 0); - if ($avatar_id > 0) { - $obj = $avatar_Handler->get($avatar_id); + $xoops->security()->requireAjaxToken(); + $avatar_id = Request::getInt('avatar_id', 0, 'POST'); + $obj = $avatar_id > 0 ? $avatar_Handler->get($avatar_id) : null; + if (is_object($obj)) { $old = $obj->getVar('avatar_display'); $obj->setVar('avatar_display', !$old); if ($avatar_Handler->insert($obj)) { diff --git a/htdocs/modules/avatars/templates/admin/avatars_admin_custom.tpl b/htdocs/modules/avatars/templates/admin/avatars_admin_custom.tpl index 7ccfcd2a3..496159f4f 100644 --- a/htdocs/modules/avatars/templates/admin/avatars_admin_custom.tpl +++ b/htdocs/modules/avatars/templates/admin/avatars_admin_custom.tpl @@ -27,7 +27,7 @@
<{$avatar.avatar_name}>
- <{if $avatar.avatar_display}><{translate key='DISPLAY_IN_FORM'}><{else}><{translate key='DO_NOT_DISPLAY_IN_FORM'}><{/if}> diff --git a/htdocs/modules/avatars/templates/admin/avatars_admin_system.tpl b/htdocs/modules/avatars/templates/admin/avatars_admin_system.tpl index 7cc7c5890..3cd6e15a6 100644 --- a/htdocs/modules/avatars/templates/admin/avatars_admin_system.tpl +++ b/htdocs/modules/avatars/templates/admin/avatars_admin_system.tpl @@ -27,7 +27,7 @@
<{$avatar.avatar_name}>
- <{if $avatar.avatar_display}><{translate key='DISPLAY_IN_FORM'}><{else}><{translate key='DO_NOT_DISPLAY_IN_FORM'}><{/if}> <{$avatar.count}> <{translate key='USERS' dirname='avatars'}> diff --git a/htdocs/modules/images/admin/header.php b/htdocs/modules/images/admin/header.php index 661a41899..5300977aa 100644 --- a/htdocs/modules/images/admin/header.php +++ b/htdocs/modules/images/admin/header.php @@ -43,11 +43,8 @@ // check WRITE right by category before continue // (op names must match the controllers: categories.php and images.php) -// the AJAX display toggle keeps its token valid for the next click on the same page -if ($op === 'display' && ('POST' !== Request::getMethod() || !$xoops->security()->check(false))) { - // non-2xx so Xoops.changeStatus() does not flip the icon - http_response_code(403); - exit(); +if ($op === 'display') { + $xoops->security()->requireAjaxToken(); } $write_cats = array(); if ($redirect === 'categories.php' && $imgcat_id > 0 && in_array($op, array('edit', 'save', 'del', 'display'), true)) { diff --git a/htdocs/modules/page/admin/content.php b/htdocs/modules/page/admin/content.php index 31d4c32db..ceda7aa08 100644 --- a/htdocs/modules/page/admin/content.php +++ b/htdocs/modules/page/admin/content.php @@ -33,6 +33,7 @@ case 'list': default: + $xoops->tpl()->assign('token', $xoops->security()->createToken(3600)); $admin_page->addTips(PageLocale::CONTENT_TIPS); $admin_page->addItemButton(PageLocale::A_ADD_CONTENT, 'content.php?op=new', 'add'); $admin_page->renderTips(); @@ -206,6 +207,7 @@ break; case 'update_status': + $xoops->security()->requireAjaxToken(); $content_id = Request::getInt('content_id', 0); if ($content_id > 0) { $obj = $content_Handler->get($content_id); @@ -219,6 +221,7 @@ break; case 'update_display': + $xoops->security()->requireAjaxToken(); $content_id = Request::getInt('content_id', 0); if ($content_id > 0) { $obj = $content_Handler->get($content_id); diff --git a/htdocs/modules/page/admin/permissions.php b/htdocs/modules/page/admin/permissions.php index 71882248b..7565e75cd 100644 --- a/htdocs/modules/page/admin/permissions.php +++ b/htdocs/modules/page/admin/permissions.php @@ -96,12 +96,7 @@ break; case 'update_view': - // keep the token valid for the next toggle on the same page - if ('POST' !== Request::getMethod() || !$xoops->security()->check(false)) { - // non-2xx so Xoops.changeStatus() does not flip the icon - http_response_code(403); - exit(); - } + $xoops->security()->requireAjaxToken(); $content_id = Request::getInt('content_id', 0, 'POST'); $group = Request::getInt('group', 0, 'POST'); $status = Request::getString('status', '', 'POST'); diff --git a/htdocs/modules/page/admin/related.php b/htdocs/modules/page/admin/related.php index 278d60778..0f14414ae 100644 --- a/htdocs/modules/page/admin/related.php +++ b/htdocs/modules/page/admin/related.php @@ -33,6 +33,7 @@ case 'list': default: + $xoops->tpl()->assign('token', $xoops->security()->createToken(3600)); $admin_page->addTips(PageLocale::RELATED_TIPS); $admin_page->addItemButton(PageLocale::A_ADD_RELATED, 'related.php?op=new', 'add'); $admin_page->renderTips(); @@ -178,6 +179,7 @@ break; case 'update_status': + $xoops->security()->requireAjaxToken(); $related_id = Request::getInt('related_id', 0); if ($related_id > 0) { $obj = $related_Handler->get($related_id); diff --git a/htdocs/modules/page/templates/admin/page_admin_content.tpl b/htdocs/modules/page/templates/admin/page_admin_content.tpl index 72577d44e..e9976c5f0 100644 --- a/htdocs/modules/page/templates/admin/page_admin_content.tpl +++ b/htdocs/modules/page/templates/admin/page_admin_content.tpl @@ -32,11 +32,11 @@ <{$pagecontent.content_title}> " /> - <{if $pagecontent.content_maindisplay}><{translate key=<{else}><{translate key="A_ENABLE"}><{/if}>" title="<{if $pagecontent.content_maindisplay}><{translate key="A_DISABLE"}><{else}><{translate key="A_ENABLE"}><{/if}>" /> + <{if $pagecontent.content_maindisplay}><{translate key=<{else}><{translate key="A_ENABLE"}><{/if}>" title="<{if $pagecontent.content_maindisplay}><{translate key="A_DISABLE"}><{else}><{translate key="A_ENABLE"}><{/if}>" /> " /> - <{if $pagecontent.content_status}><{translate key=<{else}><{translate key="A_ENABLE"}><{/if}>" title="<{if $pagecontent.content_status}><{translate key="A_DISABLE"}><{else}><{translate key="A_ENABLE"}><{/if}>" /> + <{if $pagecontent.content_status}><{translate key=<{else}><{translate key="A_ENABLE"}><{/if}>" title="<{if $pagecontent.content_status}><{translate key="A_DISABLE"}><{else}><{translate key="A_ENABLE"}><{/if}>" /> <{$pagecontent.content_weight}> <{$pagecontent.content_hits}> diff --git a/htdocs/modules/page/templates/admin/page_admin_related.tpl b/htdocs/modules/page/templates/admin/page_admin_related.tpl index a2efe9c80..b7c0cf82e 100644 --- a/htdocs/modules/page/templates/admin/page_admin_related.tpl +++ b/htdocs/modules/page/templates/admin/page_admin_related.tpl @@ -34,7 +34,7 @@ <{$item_r.navigation}> " /> - <{if $item_r.related_domenu}><{translate key=<{else}><{translate key="A_ENABLE"}><{/if}>" title="<{if $item_r.related_domenu}><{translate key="A_DISABLE"}><{else}><{translate key="A_ENABLE"}><{/if}>" /> + <{if $item_r.related_domenu}><{translate key=<{else}><{translate key="A_ENABLE"}><{/if}>" title="<{if $item_r.related_domenu}><{translate key="A_DISABLE"}><{else}><{translate key="A_ENABLE"}><{/if}>" /> "> diff --git a/htdocs/modules/profile/admin/deactivate.php b/htdocs/modules/profile/admin/deactivate.php index 3c2aa800e..affbac652 100644 --- a/htdocs/modules/profile/admin/deactivate.php +++ b/htdocs/modules/profile/admin/deactivate.php @@ -10,6 +10,7 @@ */ use Xoops\Core\FixedGroups; +use Xoops\Core\Request; /** * Extended User Profile @@ -25,11 +26,16 @@ $xoops = Xoops::getInstance(); $xoops->header(); -if (!isset($_REQUEST['uid'])) { +if ('POST' !== Request::getMethod() || !$xoops->security()->check()) { + $xoops->redirect('index.php', 3, implode('
', $xoops->security()->getErrors())); +} +$uid = Request::getInt('uid', 0, 'POST'); +$level = Request::getInt('level', 0, 'POST') === 1 ? 1 : 0; +if (0 === $uid) { $xoops->redirect("index.php", 2, _PROFILE_AM_NOSELECTION); } $member_handler = $xoops->getHandlerMember(); -$user = $member_handler->getUser($_REQUEST['uid']); +$user = $member_handler->getUser($uid); if (!$user || $user->isNew()) { $xoops->redirect("index.php", 2, _PROFILE_AM_USERDONEXIT); } @@ -37,15 +43,15 @@ if (in_array(FixedGroups::ADMIN, $user->getGroups())) { $xoops->redirect("index.php", 2, _PROFILE_AM_CANNOTDEACTIVATEWEBMASTERS); } -$user->setVar('level', $_REQUEST['level']); +$user->setVar('level', $level); if ($member_handler->insertUser($user)) { - if ($_REQUEST['level'] == 1) { + if (1 === $level) { $message = _PROFILE_AM_USER_ACTIVATED; } else { $message = _PROFILE_AM_USER_DEACTIVATED; } } else { - if ($_REQUEST['level'] == 1) { + if (1 === $level) { $message = _PROFILE_AM_USER_NOT_ACTIVATED; } else { $message = _PROFILE_AM_USER_NOT_DEACTIVATED; diff --git a/htdocs/modules/profile/admin/step.php b/htdocs/modules/profile/admin/step.php index e64fedfb2..f22842fcc 100644 --- a/htdocs/modules/profile/admin/step.php +++ b/htdocs/modules/profile/admin/step.php @@ -39,6 +39,7 @@ switch ($op) { case "list": default: + $xoops->tpl()->assign('token', $xoops->security()->createToken(3600)); // Add Scripts $xoops->theme()->addScript('media/xoops/xoops.js'); $admin_page->addItemButton(XoopsLocale::A_ADD . ' ' . _PROFILE_AM_STEP, 'step.php?op=new', 'add'); @@ -123,6 +124,7 @@ break; case "step_update": + $xoops->security()->requireAjaxToken(); $id = Request::getInt('id', 0, 'post'); if ($id > 0) { $obj = $regstep_Handler->get($id); diff --git a/htdocs/modules/profile/templates/admin/steplist.tpl b/htdocs/modules/profile/templates/admin/steplist.tpl index 95649e99b..02441db4e 100644 --- a/htdocs/modules/profile/templates/admin/steplist.tpl +++ b/htdocs/modules/profile/templates/admin/steplist.tpl @@ -26,7 +26,7 @@ {if $step.step_save}{translate key='NO'}{else}{translate key='YES'}{/if} diff --git a/htdocs/modules/profile/templates/profile_userinfo.tpl b/htdocs/modules/profile/templates/profile_userinfo.tpl index 8663bb654..39f2a9da2 100644 --- a/htdocs/modules/profile/templates/profile_userinfo.tpl +++ b/htdocs/modules/profile/templates/profile_userinfo.tpl @@ -48,6 +48,7 @@
+ {$token} {if $userlevel == 1} diff --git a/htdocs/modules/profile/user.php b/htdocs/modules/profile/user.php index ca99433a2..b2fa2cb47 100644 --- a/htdocs/modules/profile/user.php +++ b/htdocs/modules/profile/user.php @@ -105,6 +105,9 @@ echo $xoops->confirm(array('op' => 'delete', 'ok' => 1), 'user.php', XoopsLocale::Q_ARE_YOU_SURE_TO_DELETE_ACCOUNT . '
' . XoopsLocale::THIS_WILL_REMOVE_ALL_YOUR_INFO); include __DIR__ . '/footer.php'; } else { + if (!$xoops->security()->check()) { + $xoops->redirect('user.php', 3, implode('
', $xoops->security()->getErrors())); + } $del_uid = $xoops->user->getVar("uid"); if (false != $xoops->getHandlerMember()->deleteUser($xoops->user)) { $xoops->getHandlerOnline()->destroy($del_uid); diff --git a/htdocs/modules/profile/userinfo.php b/htdocs/modules/profile/userinfo.php index 9b0582f10..42cfd43b2 100644 --- a/htdocs/modules/profile/userinfo.php +++ b/htdocs/modules/profile/userinfo.php @@ -117,6 +117,7 @@ $xoops->tpl()->assign('lang_editprofile', XoopsLocale::EDIT_PROFILE); $xoops->tpl()->assign('lang_deleteaccount', XoopsLocale::DELETE_ACCOUNT); $xoops->tpl()->assign('userlevel', $thisUser->isActive()); + $xoops->tpl()->assign('token', $xoops->security()->getTokenHTML()); } // Let extensions add navigation button diff --git a/htdocs/modules/smilies/admin/smilies.php b/htdocs/modules/smilies/admin/smilies.php index 5b43916ca..978e7f23d 100644 --- a/htdocs/modules/smilies/admin/smilies.php +++ b/htdocs/modules/smilies/admin/smilies.php @@ -45,6 +45,7 @@ switch ($op) { case 'list': default: + $xoops->tpl()->assign('token', $xoops->security()->createToken(3600)); $admin_page->addTips(_AM_SMILIES_TIPS); $admin_page->addItemButton(_AM_SMILIES_ADD, 'smilies.php?op=add', 'add'); $admin_page->renderTips(); @@ -162,6 +163,7 @@ case 'smilies_update_display': $xoops->logger()->quiet(); + $xoops->security()->requireAjaxToken(); $smiley_id = Request::getInt('smiley_id', 0); if ($smiley_id > 0) { $obj = $helper->getHandlerSmilies()->get($smiley_id); diff --git a/htdocs/modules/smilies/templates/admin/smilies_smilies.tpl b/htdocs/modules/smilies/templates/admin/smilies_smilies.tpl index ce1b93991..4b837ce31 100644 --- a/htdocs/modules/smilies/templates/admin/smilies_smilies.tpl +++ b/htdocs/modules/smilies/templates/admin/smilies_smilies.tpl @@ -32,7 +32,7 @@ {$smiley.smiley_emotion} - {if $smiley.smiley_display}{$smarty.const._AM_SMILIES_OFF}{else}{$smarty.const._AM_SMILIES_ON}{/if} + {if $smiley.smiley_display}{$smarty.const._AM_SMILIES_OFF}{else}{$smarty.const._AM_SMILIES_ON}{/if} diff --git a/htdocs/modules/system/admin.php b/htdocs/modules/system/admin.php index 295ff18ca..d5b551ead 100644 --- a/htdocs/modules/system/admin.php +++ b/htdocs/modules/system/admin.php @@ -81,6 +81,7 @@ $op = Request::getString('op', ''); if ($op === 'system_activate') { \Xoops::getInstance()->logger()->quiet(); + $xoops->security()->requireAjaxToken(); $part = Request::getString('type', ''); $config_handler = $xoops->getHandlerConfig(); @@ -102,6 +103,7 @@ // Define scripts $xoops->theme()->addBaseScriptAssets('@jquery'); $xoops->theme()->addBaseScriptAssets('modules/system/js/admin.js'); + system_addAjaxToken(); // Define Breadcrumb and tips $admin_page = new \Xoops\Module\Admin(); $admin_page->addBreadcrumbLink(SystemLocale::CONTROL_PANEL, \XoopsBaseConfig::get('url') . '/admin.php', true); diff --git a/htdocs/modules/system/admin/blocksadmin/main.php b/htdocs/modules/system/admin/blocksadmin/main.php index e28867715..bf581f6e4 100644 --- a/htdocs/modules/system/admin/blocksadmin/main.php +++ b/htdocs/modules/system/admin/blocksadmin/main.php @@ -54,8 +54,8 @@ 'selvis' => -1 ); foreach ($sel as $key => $value) { - $_{$key} = isset($_COOKIE[$key]) ? (int)($_COOKIE[$key]) : $value; - ${$key} = Request::getInt($key, $_{$key}, $method); + $default = isset($_COOKIE[$key]) ? (int)($_COOKIE[$key]) : $value; + ${$key} = Request::getInt($key, $default, $method); setcookie($key, ${$key}); } @@ -81,6 +81,7 @@ // Define scripts $xoops->theme()->addScript('modules/system/js/admin.js'); $xoops->theme()->addScript('modules/system/js/blocks.js'); + system_addAjaxToken(); // Define Breadcrumb and tips $admin_page = new \Xoops\Module\Admin(); $admin_page->addBreadcrumbLink(SystemLocale::CONTROL_PANEL, \XoopsBaseConfig::get('url') . '/admin.php', true); @@ -203,6 +204,7 @@ $xoops->theme()->addScript('media/jquery/plugins/jquery.form.js'); $xoops->theme()->addScript('modules/system/js/admin.js'); $xoops->theme()->addScript('modules/system/js/blocks.js'); + system_addAjaxToken(); // Define Breadcrumb and tips $admin_page = new \Xoops\Module\Admin(); $admin_page->addBreadcrumbLink(SystemLocale::CONTROL_PANEL, \XoopsBaseConfig::get('url') . '/admin.php', true); @@ -223,6 +225,7 @@ break; case 'display': + $xoops->security()->requireAjaxToken(); // Initialize blocks handler $block_handler = $xoops->getHandlerBlock(); // Get variable @@ -238,6 +241,7 @@ break; case 'drag': + $xoops->security()->requireAjaxToken(); // Initialize blocks handler $block_handler = $xoops->getHandlerBlock(); // Get variable @@ -253,6 +257,7 @@ break; case 'order': + $xoops->security()->requireAjaxToken(); // Initialize blocks handler $block_handler = $xoops->getHandlerBlock(); if (isset($_POST['blk'])) { diff --git a/htdocs/modules/system/admin/extensions/main.php b/htdocs/modules/system/admin/extensions/main.php index 85a2ea36c..2fb81cda8 100644 --- a/htdocs/modules/system/admin/extensions/main.php +++ b/htdocs/modules/system/admin/extensions/main.php @@ -58,6 +58,7 @@ $xoops->theme()->addScript('media/jquery/plugins/jquery.jeditable.js'); $xoops->theme()->addScript('modules/system/js/admin.js'); $xoops->theme()->addScript('modules/system/js/module.js'); + system_addAjaxToken(); // Define Breadcrumb and tips $admin_page = new \Xoops\Module\Admin(); $admin_page->addBreadcrumbLink(SystemLocale::CONTROL_PANEL, \XoopsBaseConfig::get('url') . '/admin.php', true); diff --git a/htdocs/modules/system/admin/modulesadmin/main.php b/htdocs/modules/system/admin/modulesadmin/main.php index 36e12ed85..8856ccd03 100644 --- a/htdocs/modules/system/admin/modulesadmin/main.php +++ b/htdocs/modules/system/admin/modulesadmin/main.php @@ -58,6 +58,7 @@ $xoops->theme()->addScript('media/jquery/plugins/jquery.jeditable.js'); $xoops->theme()->addScript('modules/system/js/admin.js'); $xoops->theme()->addScript('modules/system/js/module.js'); + system_addAjaxToken(); // Define Breadcrumb and tips $admin_page = new \Xoops\Module\Admin(); $admin_page->addBreadcrumbLink(SystemLocale::CONTROL_PANEL, \XoopsBaseConfig::get('url') . '/admin.php', true); @@ -108,6 +109,7 @@ break; case 'order': + $xoops->security()->requireAjaxToken(); // Get Module Handler $module_handler = $xoops->getHandlerModule(); if (isset($_POST['mod'])) { @@ -131,6 +133,7 @@ case 'active': $xoops->logger()->quiet(); + $xoops->security()->requireAjaxToken(); //$xoops->disableErrorReporting(); // Get module handler $module_handler = $xoops->getHandlerModule(); @@ -159,6 +162,7 @@ case 'display_in_menu': $xoops->logger()->quiet(); + $xoops->security()->requireAjaxToken(); //$xoops->disableErrorReporting(); // Get module handler $module_handler = $xoops->getHandlerModule(); diff --git a/htdocs/modules/system/blocks/main.php b/htdocs/modules/system/blocks/main.php index 1a1ce1ae5..9ef275fb5 100644 --- a/htdocs/modules/system/blocks/main.php +++ b/htdocs/modules/system/blocks/main.php @@ -80,6 +80,7 @@ function b_system_main_edit($options) // Define scripts $xoops->theme()->addScript('media/jquery/plugins/jquery.jeditable.js'); $xoops->theme()->addScript('modules/system/js/module.js'); + system_addAjaxToken(); $admin_page->addTips(SystemLocale::MENU_TIPS); $admin_page->renderTips(); diff --git a/htdocs/modules/system/include/functions.php b/htdocs/modules/system/include/functions.php index 92afb5690..2785ec08b 100644 --- a/htdocs/modules/system/include/functions.php +++ b/htdocs/modules/system/include/functions.php @@ -123,6 +123,23 @@ function system_AdminIcons($img) return $url; } +/** + * Give the page's AJAX scripts (admin.js, blocks.js, module.js) the token + * their state changes send, as the JS global XOOPS_TOKEN + * + * @return void + */ +function system_addAjaxToken() +{ + static $added = false; + if (!$added) { + $xoops = Xoops::getInstance(); + $token = json_encode($xoops->security()->createToken(3600)); + $xoops->theme()->addScript('', array('type' => 'text/javascript'), 'var XOOPS_TOKEN = ' . $token . ';'); + $added = true; + } +} + /** * @param string $name * @return void diff --git a/htdocs/modules/system/js/blocks.js b/htdocs/modules/system/js/blocks.js index 8e88c5a3a..c4400698e 100644 --- a/htdocs/modules/system/js/blocks.js +++ b/htdocs/modules/system/js/blocks.js @@ -8,16 +8,16 @@ $(document).ready( connectWith: '.xo-blocksection', update: function(event, ui) { var list = $(this).sortable( 'serialize'); - $.post( 'admin.php?fct=blocksadmin&op=order', list ); + $.post( 'admin.php?fct=blocksadmin&op=order', list + '&XOOPS_TOKEN_REQUEST=' + XOOPS_TOKEN ); }, receive: function(event, ui) { var side = $(this).attr('side'); var bid = $(ui.item).attr('bid'); var list = $(this).sortable( 'serialize'); - $.post( 'admin.php', { fct: 'blocksadmin', op: 'drag', bid: bid, side: side } ); + $.post( 'admin.php', { fct: 'blocksadmin', op: 'drag', bid: bid, side: side, XOOPS_TOKEN_REQUEST: XOOPS_TOKEN } ); - $.post( 'admin.php?fct=blocksadmin&op=order', list ); + $.post( 'admin.php?fct=blocksadmin&op=order', list + '&XOOPS_TOKEN_REQUEST=' + XOOPS_TOKEN ); } } @@ -31,16 +31,16 @@ $(document).ready( connectWith: '.xo-blocksection'/*, update: function(event, ui) { var list = $(this).sortable( 'serialize'); - $.post( 'admin.php?fct=blocksadmin&op=order', list ); + $.post( 'admin.php?fct=blocksadmin&op=order', list + '&XOOPS_TOKEN_REQUEST=' + XOOPS_TOKEN ); }, receive: function(event, ui) { var side = $(this).attr('side'); var bid = $(ui.item).attr('bid'); var list = $(this).sortable( 'serialize'); - $.post( 'admin.php', { fct: 'blocksadmin', op: 'drag', bid: bid, side: side } ); + $.post( 'admin.php', { fct: 'blocksadmin', op: 'drag', bid: bid, side: side, XOOPS_TOKEN_REQUEST: XOOPS_TOKEN } ); - $.post( 'admin.php?fct=blocksadmin&op=order', list ); + $.post( 'admin.php?fct=blocksadmin&op=order', list + '&XOOPS_TOKEN_REQUEST=' + XOOPS_TOKEN ); }*/ } diff --git a/htdocs/modules/system/js/module.js b/htdocs/modules/system/js/module.js index cceb2da6e..27b948e2f 100644 --- a/htdocs/modules/system/js/module.js +++ b/htdocs/modules/system/js/module.js @@ -64,7 +64,7 @@ $(function() { placeholder: 'ui-state-highlight', update: function(event, ui) { var list = $(this).sortable( 'serialize'); - $.post( 'admin.php?fct=modulesadmin&op=order', list ); + $.post( 'admin.php?fct=modulesadmin&op=order', list + '&XOOPS_TOKEN_REQUEST=' + XOOPS_TOKEN ); }} ); $( "#sortable" ).disableSelection(); @@ -94,7 +94,7 @@ function module_Detail(id){ } function module_Disable(id, enable, disable){ - $.post( 'admin.php', { fct: 'modulesadmin', op: 'active', mid: id } , + $.post( 'admin.php', { fct: 'modulesadmin', op: 'active', mid: id, XOOPS_TOKEN_REQUEST: XOOPS_TOKEN } , function(reponse, textStatus) { if (textStatus=='success') { if(reponse){ @@ -110,7 +110,7 @@ function module_Disable(id, enable, disable){ } function module_Hide(id){ - $.post( 'admin.php', { fct: 'modulesadmin', op: 'display_in_menu', mid: id } , + $.post( 'admin.php', { fct: 'modulesadmin', op: 'display_in_menu', mid: id, XOOPS_TOKEN_REQUEST: XOOPS_TOKEN } , function(reponse, textStatus) { if (textStatus=='success') { if(reponse != 0){ diff --git a/htdocs/modules/system/templates/admin/system_blocks_item.tpl b/htdocs/modules/system/templates/admin/system_blocks_item.tpl index 2bc0c2d0f..02ecb78d6 100644 --- a/htdocs/modules/system/templates/admin/system_blocks_item.tpl +++ b/htdocs/modules/system/templates/admin/system_blocks_item.tpl @@ -9,7 +9,7 @@
- <{if $item.visible}><{translate key='HIDE_BLOCK' dirname='system'}><{else}><{translate key='DISPLAY_BLOCK' dirname='system'}><{/if}> <{$item.name}> <{translate key='A_EDIT'}> diff --git a/htdocs/modules/system/templates/admin/system_index.tpl b/htdocs/modules/system/templates/admin/system_index.tpl index 6b322d991..24574668f 100644 --- a/htdocs/modules/system/templates/admin/system_index.tpl +++ b/htdocs/modules/system/templates/admin/system_index.tpl @@ -29,7 +29,7 @@ <{if $menuitem.used}> - <{translate key='CHANGE_STATUS'}> + <{translate key='CHANGE_STATUS'}> <{/if}> diff --git a/htdocs/modules/userrank/admin/userrank.php b/htdocs/modules/userrank/admin/userrank.php index 5f709b518..f8183ed68 100644 --- a/htdocs/modules/userrank/admin/userrank.php +++ b/htdocs/modules/userrank/admin/userrank.php @@ -43,6 +43,7 @@ case 'list': default: + $xoops->tpl()->assign('token', $xoops->security()->createToken(3600)); // Add Scripts $xoops->theme()->addScript('media/xoops/xoops.js'); @@ -178,6 +179,7 @@ // Update userrank status case 'userrank_update_special': + $xoops->security()->requireAjaxToken(); // Get rank id $rank_id = Request::getInt('rank_id', 0); if ($rank_id > 0) { diff --git a/htdocs/modules/userrank/templates/admin/userrank.tpl b/htdocs/modules/userrank/templates/admin/userrank.tpl index 5a5f70bae..c2ba434b1 100644 --- a/htdocs/modules/userrank/templates/admin/userrank.tpl +++ b/htdocs/modules/userrank/templates/admin/userrank.tpl @@ -29,7 +29,7 @@ <{$urank.rank_max}> - <{if $urank.rank_special}><{$smarty.const._AM_USERRANK_OFF}><{else}><{$smarty.const._AM_USERRANK_ON}><{/if}> + <{if $urank.rank_special}><{$smarty.const._AM_USERRANK_OFF}><{else}><{$smarty.const._AM_USERRANK_ON}><{/if}> diff --git a/htdocs/user.php b/htdocs/user.php index 456b7cd91..a2f678e84 100644 --- a/htdocs/user.php +++ b/htdocs/user.php @@ -121,6 +121,9 @@ ); $xoops->footer(); } else { + if (!$xoops->security()->check()) { + $xoops->redirect('user.php', 3, implode('
', $xoops->security()->getErrors())); + } $del_uid = $xoops->user->getVar("uid"); $member_handler = $xoops->getHandlerMember(); if (false != $member_handler->deleteUser($xoops->user)) { diff --git a/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php b/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php index 49930fd0f..7a5c1afa5 100644 --- a/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php +++ b/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php @@ -12,6 +12,7 @@ class SecurityTest extends \PHPUnit\Framework\TestCase protected $SERVER_save; protected $SESSION_save; + protected $POST_save; protected $moduleConfig_save; /** @@ -23,6 +24,7 @@ protected function setUp(): void $this->object = new Security(); $this->SERVER_save = $_SERVER; $this->SESSION_save = $_SESSION; + $this->POST_save = $_POST; $this->moduleConfig_save = \Xoops::getInstance()->moduleConfig; } @@ -34,6 +36,7 @@ protected function tearDown(): void { $_SERVER = $this->SERVER_save; $_SESSION = $this->SESSION_save; + $_POST = $this->POST_save; \Xoops::getInstance()->moduleConfig = $this->moduleConfig_save; } @@ -76,6 +79,19 @@ public function test_check() $this->assertTrue(empty($_SESSION['XOOPS_TOKEN_SESSION'])); } + public function test_requireAjaxToken_keepsTokenForNextToggle() + { + unset($_SESSION['XOOPS_TOKEN_SESSION']); + $token = $this->object->createToken(3600); + $_SERVER['REQUEST_METHOD'] = 'POST'; + $_POST['XOOPS_TOKEN_REQUEST'] = $token; + + // returns (does not exit) for a valid POST, twice with one token + $this->object->requireAjaxToken(); + $this->object->requireAjaxToken(); + $this->assertSame($token, $_SESSION['XOOPS_TOKEN_SESSION'][0]['id']); + } + public function test_createToken() { $instance = $this->object; diff --git a/xoops_lib/Xoops/Core/Security.php b/xoops_lib/Xoops/Core/Security.php index 245e353e1..8ef303358 100644 --- a/xoops_lib/Xoops/Core/Security.php +++ b/xoops_lib/Xoops/Core/Security.php @@ -46,6 +46,28 @@ public function check($clearIfValid = true, $token = false, $name = 'XOOPS_TOKEN return $this->validateToken($token, $clearIfValid, $name); } + /** + * Stop an AJAX state change unless it is a POST carrying a valid token. + * + * The token is kept, so every toggle on a page can reuse the one token + * the page minted (use createToken() with a long timeout for it). A + * rejected request ends with HTTP 403, which keeps Xoops.changeStatus() + * from flipping its icon. + * + * @param string $name name of session variable + * + * @return void + */ + public function requireAjaxToken($name = 'XOOPS_TOKEN') + { + // the token must come in the body, a query string leaks through logs and referers + $token = Request::getString($name . '_REQUEST', '', 'POST'); + if ('POST' !== Request::getMethod() || !$this->check(false, $token, $name)) { + http_response_code(403); + exit(); + } + } + /** * Create a token in the user's session * From 4f8d415e46895171a677b3e18c77a2f78816556b Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 2 Nov 2026 09:09:25 -0500 Subject: [PATCH 2/7] fix(security): drop unguarded related view toggle, validate deactivate level, reload on expired AJAX token, test token rejections --- htdocs/media/xoops/xoops.js | 11 +++++ .../modules/avatars/admin/avatar_custom.php | 12 +---- .../modules/avatars/admin/avatar_system.php | 12 +---- htdocs/modules/avatars/admin/header.php | 21 +++++++++ htdocs/modules/page/admin/related.php | 13 ------ htdocs/modules/profile/admin/deactivate.php | 4 +- htdocs/modules/system/include/functions.php | 4 +- .../unit/xoopsLib/Xoops/Core/SecurityTest.php | 44 +++++++++++++++++++ xoops_lib/Xoops/Core/Security.php | 22 ++++++++-- 9 files changed, 101 insertions(+), 42 deletions(-) diff --git a/htdocs/media/xoops/xoops.js b/htdocs/media/xoops/xoops.js index 08c3c16e4..8f584c95b 100644 --- a/htdocs/media/xoops/xoops.js +++ b/htdocs/media/xoops/xoops.js @@ -101,3 +101,14 @@ var Xoops = { } }; + +// An admin toggle was refused by Security::requireAjaxToken(): its page token has +// expired, so nothing was saved. Reload to show the real state and get a fresh +// token, instead of leaving a changed icon or sorted list on screen. +if (window.jQuery) { + jQuery(document).ajaxError(function (event, xhr) { + if (xhr.status === 403 && xhr.getResponseHeader('X-Xoops-Token') === 'rejected') { + window.location.reload(); + } + }); +} diff --git a/htdocs/modules/avatars/admin/avatar_custom.php b/htdocs/modules/avatars/admin/avatar_custom.php index 153c82f00..45e39c5ca 100644 --- a/htdocs/modules/avatars/admin/avatar_custom.php +++ b/htdocs/modules/avatars/admin/avatar_custom.php @@ -213,17 +213,7 @@ break; case "update_display": - $xoops->security()->requireAjaxToken(); - $avatar_id = Request::getInt('avatar_id', 0, 'POST'); - $obj = $avatar_id > 0 ? $avatar_Handler->get($avatar_id) : null; - if (is_object($obj)) { - $old = $obj->getVar('avatar_display'); - $obj->setVar('avatar_display', !$old); - if ($avatar_Handler->insert($obj)) { - exit; - } - echo $obj->getHtmlErrors(); - } + avatars_updateDisplay($avatar_Handler); break; } $xoops->footer(); diff --git a/htdocs/modules/avatars/admin/avatar_system.php b/htdocs/modules/avatars/admin/avatar_system.php index c272f2499..84434bd9d 100644 --- a/htdocs/modules/avatars/admin/avatar_system.php +++ b/htdocs/modules/avatars/admin/avatar_system.php @@ -220,17 +220,7 @@ break; case "update_display": - $xoops->security()->requireAjaxToken(); - $avatar_id = Request::getInt('avatar_id', 0, 'POST'); - $obj = $avatar_id > 0 ? $avatar_Handler->get($avatar_id) : null; - if (is_object($obj)) { - $old = $obj->getVar('avatar_display'); - $obj->setVar('avatar_display', !$old); - if ($avatar_Handler->insert($obj)) { - exit; - } - echo $obj->getHtmlErrors(); - } + avatars_updateDisplay($avatar_Handler); break; } $xoops->footer(); diff --git a/htdocs/modules/avatars/admin/header.php b/htdocs/modules/avatars/admin/header.php index 669349be3..afaf8d7df 100644 --- a/htdocs/modules/avatars/admin/header.php +++ b/htdocs/modules/avatars/admin/header.php @@ -20,3 +20,24 @@ * @version $Id$ */ require_once dirname(dirname(dirname(__DIR__))) . '/include/cp_header.php'; + +/** + * AJAX toggle of an avatar's display flag, shared by the system and custom avatar pages + * + * @param \Xoops\Core\Kernel\XoopsPersistableObjectHandler $avatarHandler avatar handler + * + * @return void + */ +function avatars_updateDisplay(\Xoops\Core\Kernel\XoopsPersistableObjectHandler $avatarHandler): void +{ + \Xoops::getInstance()->security()->requireAjaxToken(); + $avatar_id = \Xmf\Request::getInt('avatar_id', 0, 'POST'); + $obj = $avatar_id > 0 ? $avatarHandler->get($avatar_id) : null; + if (is_object($obj)) { + $obj->setVar('avatar_display', !$obj->getVar('avatar_display')); + if ($avatarHandler->insert($obj)) { + exit; + } + echo $obj->getHtmlErrors(); + } +} diff --git a/htdocs/modules/page/admin/related.php b/htdocs/modules/page/admin/related.php index 0f14414ae..37121db57 100644 --- a/htdocs/modules/page/admin/related.php +++ b/htdocs/modules/page/admin/related.php @@ -191,18 +191,5 @@ echo $obj->getHtmlErrors(); } break; - - case 'view': - $related_id = Request::getInt('related_id', 0); - if ($related_id > 0) { - $obj = $related_Handler->get($related_id); - $old = $obj->getVar('related_domenu'); - $obj->setVar('related_domenu', !$old); - if ($related_Handler->insert($obj)) { - exit; - } - echo $obj->getHtmlErrors(); - } - break; } $xoops->footer(); diff --git a/htdocs/modules/profile/admin/deactivate.php b/htdocs/modules/profile/admin/deactivate.php index affbac652..9dbf91ff5 100644 --- a/htdocs/modules/profile/admin/deactivate.php +++ b/htdocs/modules/profile/admin/deactivate.php @@ -30,8 +30,8 @@ $xoops->redirect('index.php', 3, implode('
', $xoops->security()->getErrors())); } $uid = Request::getInt('uid', 0, 'POST'); -$level = Request::getInt('level', 0, 'POST') === 1 ? 1 : 0; -if (0 === $uid) { +$level = Request::getInt('level', -1, 'POST'); +if (0 === $uid || !in_array($level, [0, 1], true)) { $xoops->redirect("index.php", 2, _PROFILE_AM_NOSELECTION); } $member_handler = $xoops->getHandlerMember(); diff --git a/htdocs/modules/system/include/functions.php b/htdocs/modules/system/include/functions.php index 2785ec08b..a463d340a 100644 --- a/htdocs/modules/system/include/functions.php +++ b/htdocs/modules/system/include/functions.php @@ -129,13 +129,15 @@ function system_AdminIcons($img) * * @return void */ -function system_addAjaxToken() +function system_addAjaxToken(): void { static $added = false; if (!$added) { $xoops = Xoops::getInstance(); $token = json_encode($xoops->security()->createToken(3600)); $xoops->theme()->addScript('', array('type' => 'text/javascript'), 'var XOOPS_TOKEN = ' . $token . ';'); + // reloads the page when the token has expired (HTTP 403) + $xoops->theme()->addScript('media/xoops/xoops.js'); $added = true; } } diff --git a/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php b/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php index 7a5c1afa5..f96233f09 100644 --- a/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php +++ b/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php @@ -79,6 +79,8 @@ public function test_check() $this->assertTrue(empty($_SESSION['XOOPS_TOKEN_SESSION'])); } + // a wrongful exit() here would end the whole runner with status 0 + #[\PHPUnit\Framework\Attributes\RunInSeparateProcess] public function test_requireAjaxToken_keepsTokenForNextToggle() { unset($_SESSION['XOOPS_TOKEN_SESSION']); @@ -92,6 +94,48 @@ public function test_requireAjaxToken_keepsTokenForNextToggle() $this->assertSame($token, $_SESSION['XOOPS_TOKEN_SESSION'][0]['id']); } + public static function rejectedAjaxRequests(): array + { + return [ + 'GET with body token' => ['GET', 'valid', false], + 'POST without token' => ['POST', null, false], + 'POST invalid token' => ['POST', 'nope', false], + 'POST expired token' => ['POST', 'valid', true], + 'POST token in query' => ['POST', 'query', false], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('rejectedAjaxRequests')] + public function test_checkAjaxToken_rejects(string $method, ?string $token, bool $expired) + { + unset($_SESSION['XOOPS_TOKEN_SESSION']); + $valid = $this->object->createToken(3600); + if ($expired) { + $_SESSION['XOOPS_TOKEN_SESSION'][0]['expire'] = time() - 1; + } + $_SERVER['REQUEST_METHOD'] = $method; + $GET_save = $_GET; + if ('query' === $token) { + $_GET['XOOPS_TOKEN_REQUEST'] = $valid; + } elseif (null !== $token) { + $_POST['XOOPS_TOKEN_REQUEST'] = 'valid' === $token ? $valid : $token; + } + + try { + $this->assertFalse($this->object->checkAjaxToken()); + } finally { + $_GET = $GET_save; + } + } + + public function test_checkAjaxToken_acceptsValidPost() + { + unset($_SESSION['XOOPS_TOKEN_SESSION']); + $_SERVER['REQUEST_METHOD'] = 'POST'; + $_POST['XOOPS_TOKEN_REQUEST'] = $this->object->createToken(3600); + $this->assertTrue($this->object->checkAjaxToken()); + } + public function test_createToken() { $instance = $this->object; diff --git a/xoops_lib/Xoops/Core/Security.php b/xoops_lib/Xoops/Core/Security.php index 8ef303358..e15370a6f 100644 --- a/xoops_lib/Xoops/Core/Security.php +++ b/xoops_lib/Xoops/Core/Security.php @@ -58,16 +58,30 @@ public function check($clearIfValid = true, $token = false, $name = 'XOOPS_TOKEN * * @return void */ - public function requireAjaxToken($name = 'XOOPS_TOKEN') + public function requireAjaxToken($name = 'XOOPS_TOKEN'): void { - // the token must come in the body, a query string leaks through logs and referers - $token = Request::getString($name . '_REQUEST', '', 'POST'); - if ('POST' !== Request::getMethod() || !$this->check(false, $token, $name)) { + if (!$this->checkAjaxToken($name)) { http_response_code(403); + // media/xoops/xoops.js reloads the page on this marker to get a fresh token + header('X-Xoops-Token: rejected'); exit(); } } + /** + * Is this a POST carrying a valid token in its body? The token is kept. + * + * @param string $name name of session variable + * + * @return bool + */ + public function checkAjaxToken($name = 'XOOPS_TOKEN'): bool + { + // the token must come in the body, a query string leaks through logs and referers + $token = Request::getString($name . '_REQUEST', '', 'POST'); + return 'POST' === Request::getMethod() && '' !== $token && $this->check(false, $token, $name); + } + /** * Create a token in the user's session * From 699a33280c193fbe7f4c1382312f57a27802fca3 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 2 Nov 2026 09:46:11 -0500 Subject: [PATCH 3/7] fix(security): accept only exact 0/1 deactivate level; rename test save fields --- htdocs/modules/profile/admin/deactivate.php | 6 ++-- .../unit/xoopsLib/Xoops/Core/SecurityTest.php | 28 +++++++++---------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/htdocs/modules/profile/admin/deactivate.php b/htdocs/modules/profile/admin/deactivate.php index 9dbf91ff5..0751e07fd 100644 --- a/htdocs/modules/profile/admin/deactivate.php +++ b/htdocs/modules/profile/admin/deactivate.php @@ -30,10 +30,12 @@ $xoops->redirect('index.php', 3, implode('
', $xoops->security()->getErrors())); } $uid = Request::getInt('uid', 0, 'POST'); -$level = Request::getInt('level', -1, 'POST'); -if (0 === $uid || !in_array($level, [0, 1], true)) { +// exact "0" or "1" only: getInt() would turn "0.5" or "abc" into a valid level +$level = Request::getString('level', '', 'POST'); +if (0 === $uid || !in_array($level, ['0', '1'], true)) { $xoops->redirect("index.php", 2, _PROFILE_AM_NOSELECTION); } +$level = (int) $level; $member_handler = $xoops->getHandlerMember(); $user = $member_handler->getUser($uid); if (!$user || $user->isNew()) { diff --git a/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php b/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php index f96233f09..48181568f 100644 --- a/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php +++ b/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php @@ -10,10 +10,10 @@ class SecurityTest extends \PHPUnit\Framework\TestCase */ protected $object; - protected $SERVER_save; - protected $SESSION_save; - protected $POST_save; - protected $moduleConfig_save; + protected $savedServer; + protected $savedSession; + protected $savedPost; + protected $savedModuleConfig; /** * Sets up the fixture, for example, opens a network connection. @@ -22,10 +22,10 @@ class SecurityTest extends \PHPUnit\Framework\TestCase protected function setUp(): void { $this->object = new Security(); - $this->SERVER_save = $_SERVER; - $this->SESSION_save = $_SESSION; - $this->POST_save = $_POST; - $this->moduleConfig_save = \Xoops::getInstance()->moduleConfig; + $this->savedServer = $_SERVER; + $this->savedSession = $_SESSION; + $this->savedPost = $_POST; + $this->savedModuleConfig = \Xoops::getInstance()->moduleConfig; } /** @@ -34,10 +34,10 @@ protected function setUp(): void */ protected function tearDown(): void { - $_SERVER = $this->SERVER_save; - $_SESSION = $this->SESSION_save; - $_POST = $this->POST_save; - \Xoops::getInstance()->moduleConfig = $this->moduleConfig_save; + $_SERVER = $this->savedServer; + $_SESSION = $this->savedSession; + $_POST = $this->savedPost; + \Xoops::getInstance()->moduleConfig = $this->savedModuleConfig; } public function test___construct() @@ -114,7 +114,7 @@ public function test_checkAjaxToken_rejects(string $method, ?string $token, bool $_SESSION['XOOPS_TOKEN_SESSION'][0]['expire'] = time() - 1; } $_SERVER['REQUEST_METHOD'] = $method; - $GET_save = $_GET; + $savedGet = $_GET; if ('query' === $token) { $_GET['XOOPS_TOKEN_REQUEST'] = $valid; } elseif (null !== $token) { @@ -124,7 +124,7 @@ public function test_checkAjaxToken_rejects(string $method, ?string $token, bool try { $this->assertFalse($this->object->checkAjaxToken()); } finally { - $_GET = $GET_save; + $_GET = $savedGet; } } From 5e3dc16645a8b6e0ad356614fab251a61d568a91 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 2 Nov 2026 10:36:13 -0500 Subject: [PATCH 4/7] fix(security): require token for module rename, escape and fail rename properly; isolate AJAX token rejection test --- .../system/admin/modulesadmin/main.php | 23 +++++++++++-------- htdocs/modules/system/js/module.js | 3 ++- .../unit/xoopsLib/Xoops/Core/SecurityTest.php | 6 +++++ 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/htdocs/modules/system/admin/modulesadmin/main.php b/htdocs/modules/system/admin/modulesadmin/main.php index 8856ccd03..b8272cf67 100644 --- a/htdocs/modules/system/admin/modulesadmin/main.php +++ b/htdocs/modules/system/admin/modulesadmin/main.php @@ -93,19 +93,24 @@ case 'rename': $xoops->logger()->quiet(); - //$xoops->disableErrorReporting(); + $xoops->security()->requireAjaxToken(); $mid = Request::getInt('id', 0, 'post'); $value = Request::getString('value', '', 'post'); - if ($mid != 0) { - $module_handler = $xoops->getHandlerModule(); - $module = $module_handler->getById($mid); - $module->setVar('name', $value); - if ($module_handler->insertModule($module)) { - echo $value; - - } + $module_handler = $xoops->getHandlerModule(); + $module = $mid > 0 ? $module_handler->getById($mid) : null; + if (!is_object($module)) { + http_response_code(404); + break; + } + $module->setVar('name', $value); + if (!$module_handler->insertModule($module)) { + // an error status makes jEditable restore the old name + http_response_code(500); + break; } + // jEditable inserts the response as HTML + echo htmlspecialchars($value, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'); break; case 'order': diff --git a/htdocs/modules/system/js/module.js b/htdocs/modules/system/js/module.js index 27b948e2f..a120f47e4 100644 --- a/htdocs/modules/system/js/module.js +++ b/htdocs/modules/system/js/module.js @@ -47,7 +47,8 @@ $(function() { $(".rename").editable("admin.php?fct=modulesadmin&op=rename", { indicator : "", - cssclass : 'span2' + cssclass : 'span2', + submitdata : function() { return { XOOPS_TOKEN_REQUEST: XOOPS_TOKEN }; } }); $(".modal-backdrop").click(function(){ diff --git a/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php b/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php index 48181568f..a8c97dc31 100644 --- a/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php +++ b/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php @@ -115,8 +115,12 @@ public function test_checkAjaxToken_rejects(string $method, ?string $token, bool } $_SERVER['REQUEST_METHOD'] = $method; $savedGet = $_GET; + $savedPost = $_POST; + $savedRequest = $_REQUEST; + unset($_POST['XOOPS_TOKEN_REQUEST'], $_REQUEST['XOOPS_TOKEN_REQUEST']); if ('query' === $token) { $_GET['XOOPS_TOKEN_REQUEST'] = $valid; + $_REQUEST['XOOPS_TOKEN_REQUEST'] = $valid; } elseif (null !== $token) { $_POST['XOOPS_TOKEN_REQUEST'] = 'valid' === $token ? $valid : $token; } @@ -125,6 +129,8 @@ public function test_checkAjaxToken_rejects(string $method, ?string $token, bool $this->assertFalse($this->object->checkAjaxToken()); } finally { $_GET = $savedGet; + $_POST = $savedPost; + $_REQUEST = $savedRequest; } } From 65fc7102810542c30bd8b04a5cdb383f600b7438 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 2 Nov 2026 13:12:33 -0500 Subject: [PATCH 5/7] refactor(avatars): import classes in admin header instead of fully qualified names --- htdocs/modules/avatars/admin/header.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/htdocs/modules/avatars/admin/header.php b/htdocs/modules/avatars/admin/header.php index afaf8d7df..061f3acb5 100644 --- a/htdocs/modules/avatars/admin/header.php +++ b/htdocs/modules/avatars/admin/header.php @@ -19,19 +19,23 @@ * @author Mage Grégory (AKA Mage) * @version $Id$ */ + +use Xmf\Request; +use Xoops\Core\Kernel\XoopsPersistableObjectHandler; + require_once dirname(dirname(dirname(__DIR__))) . '/include/cp_header.php'; /** * AJAX toggle of an avatar's display flag, shared by the system and custom avatar pages * - * @param \Xoops\Core\Kernel\XoopsPersistableObjectHandler $avatarHandler avatar handler + * @param XoopsPersistableObjectHandler $avatarHandler avatar handler * * @return void */ -function avatars_updateDisplay(\Xoops\Core\Kernel\XoopsPersistableObjectHandler $avatarHandler): void +function avatars_updateDisplay(XoopsPersistableObjectHandler $avatarHandler): void { - \Xoops::getInstance()->security()->requireAjaxToken(); - $avatar_id = \Xmf\Request::getInt('avatar_id', 0, 'POST'); + Xoops::getInstance()->security()->requireAjaxToken(); + $avatar_id = Request::getInt('avatar_id', 0, 'POST'); $obj = $avatar_id > 0 ? $avatarHandler->get($avatar_id) : null; if (is_object($obj)) { $obj->setVar('avatar_display', !$obj->getVar('avatar_display')); From 10260045eef7e6980b4dd50764f89a8d0dff1087 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 2 Nov 2026 14:05:46 -0500 Subject: [PATCH 6/7] fix(admin): return HTTP 500 when an AJAX toggle fails to save so the status icon is not flipped --- htdocs/modules/avatars/admin/header.php | 1 + htdocs/modules/page/admin/content.php | 2 ++ htdocs/modules/page/admin/related.php | 1 + htdocs/modules/profile/admin/step.php | 1 + htdocs/modules/smilies/admin/smilies.php | 1 + htdocs/modules/userrank/admin/userrank.php | 1 + 6 files changed, 7 insertions(+) diff --git a/htdocs/modules/avatars/admin/header.php b/htdocs/modules/avatars/admin/header.php index 061f3acb5..1ad817830 100644 --- a/htdocs/modules/avatars/admin/header.php +++ b/htdocs/modules/avatars/admin/header.php @@ -42,6 +42,7 @@ function avatars_updateDisplay(XoopsPersistableObjectHandler $avatarHandler): vo if ($avatarHandler->insert($obj)) { exit; } + http_response_code(500); echo $obj->getHtmlErrors(); } } diff --git a/htdocs/modules/page/admin/content.php b/htdocs/modules/page/admin/content.php index ceda7aa08..3fb831659 100644 --- a/htdocs/modules/page/admin/content.php +++ b/htdocs/modules/page/admin/content.php @@ -216,6 +216,7 @@ if ($content_Handler->insert($obj)) { exit; } + http_response_code(500); echo $obj->getHtmlErrors(); } break; @@ -230,6 +231,7 @@ if ($content_Handler->insert($obj)) { exit; } + http_response_code(500); echo $obj->getHtmlErrors(); } break; diff --git a/htdocs/modules/page/admin/related.php b/htdocs/modules/page/admin/related.php index 37121db57..437d7ef0e 100644 --- a/htdocs/modules/page/admin/related.php +++ b/htdocs/modules/page/admin/related.php @@ -188,6 +188,7 @@ if ($related_Handler->insert($obj)) { exit; } + http_response_code(500); echo $obj->getHtmlErrors(); } break; diff --git a/htdocs/modules/profile/admin/step.php b/htdocs/modules/profile/admin/step.php index f22842fcc..3e80ff6d5 100644 --- a/htdocs/modules/profile/admin/step.php +++ b/htdocs/modules/profile/admin/step.php @@ -133,6 +133,7 @@ if ($regstep_Handler->insert($obj)) { exit; } + http_response_code(500); echo $obj->getHtmlErrors(); } break; diff --git a/htdocs/modules/smilies/admin/smilies.php b/htdocs/modules/smilies/admin/smilies.php index 978e7f23d..23244a7e7 100644 --- a/htdocs/modules/smilies/admin/smilies.php +++ b/htdocs/modules/smilies/admin/smilies.php @@ -172,6 +172,7 @@ if ($helper->getHandlerSmilies()->insert($obj)) { exit; } + http_response_code(500); echo $obj->getHtmlErrors(); } break; diff --git a/htdocs/modules/userrank/admin/userrank.php b/htdocs/modules/userrank/admin/userrank.php index f8183ed68..1375dcf43 100644 --- a/htdocs/modules/userrank/admin/userrank.php +++ b/htdocs/modules/userrank/admin/userrank.php @@ -189,6 +189,7 @@ if ($userrank_Handler->insert($obj)) { exit; } + http_response_code(500); echo $obj->getHtmlErrors(); } break; From d5d8b7ad8238f4ee0dce153d9cb673bc3ea0caec Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 2 Nov 2026 15:30:10 -0500 Subject: [PATCH 7/7] fix(security): accept account-action tokens only from a POST body; return HTTP 500 when a system toggle fails to save --- htdocs/modules/profile/admin/deactivate.php | 2 +- htdocs/modules/profile/user.php | 2 +- htdocs/modules/system/admin.php | 5 ++++- .../modules/system/admin/blocksadmin/main.php | 7 ++++--- .../system/admin/modulesadmin/main.php | 8 +++++--- htdocs/user.php | 2 +- .../unit/xoopsLib/Xoops/Core/SecurityTest.php | 18 +++++++++++++---- xoops_lib/Xoops/Core/Security.php | 20 ++++++++++++------- 8 files changed, 43 insertions(+), 21 deletions(-) diff --git a/htdocs/modules/profile/admin/deactivate.php b/htdocs/modules/profile/admin/deactivate.php index 0751e07fd..6b9e13d0b 100644 --- a/htdocs/modules/profile/admin/deactivate.php +++ b/htdocs/modules/profile/admin/deactivate.php @@ -26,7 +26,7 @@ $xoops = Xoops::getInstance(); $xoops->header(); -if ('POST' !== Request::getMethod() || !$xoops->security()->check()) { +if (!$xoops->security()->checkPost()) { $xoops->redirect('index.php', 3, implode('
', $xoops->security()->getErrors())); } $uid = Request::getInt('uid', 0, 'POST'); diff --git a/htdocs/modules/profile/user.php b/htdocs/modules/profile/user.php index b2fa2cb47..8f395ecaf 100644 --- a/htdocs/modules/profile/user.php +++ b/htdocs/modules/profile/user.php @@ -105,7 +105,7 @@ echo $xoops->confirm(array('op' => 'delete', 'ok' => 1), 'user.php', XoopsLocale::Q_ARE_YOU_SURE_TO_DELETE_ACCOUNT . '
' . XoopsLocale::THIS_WILL_REMOVE_ALL_YOUR_INFO); include __DIR__ . '/footer.php'; } else { - if (!$xoops->security()->check()) { + if (!$xoops->security()->checkPost()) { $xoops->redirect('user.php', 3, implode('
', $xoops->security()->getErrors())); } $del_uid = $xoops->user->getVar("uid"); diff --git a/htdocs/modules/system/admin.php b/htdocs/modules/system/admin.php index d5b551ead..a444365f0 100644 --- a/htdocs/modules/system/admin.php +++ b/htdocs/modules/system/admin.php @@ -91,7 +91,10 @@ /* @var $conf XoopsConfigItem */ if ($conf->getVar('conf_name') == 'active_' . $part) { $conf->setVar('conf_value', !$conf->getVar('conf_value')); - $config_handler->insertConfig($conf); + if (!$config_handler->insertConfig($conf)) { + // a non-2xx status keeps system_setStatus() from flipping the icon + http_response_code(500); + } } } exit; diff --git a/htdocs/modules/system/admin/blocksadmin/main.php b/htdocs/modules/system/admin/blocksadmin/main.php index bf581f6e4..09f5125bc 100644 --- a/htdocs/modules/system/admin/blocksadmin/main.php +++ b/htdocs/modules/system/admin/blocksadmin/main.php @@ -235,7 +235,8 @@ $block = $block_handler->get($block_id); $block->setVar('visible', $visible); if (!$block_handler->insertBlock($block)) { - $error = true; + // a non-2xx status keeps system_setStatus() from flipping the icon + http_response_code(500); } } break; @@ -251,7 +252,7 @@ $block = $block_handler->get($block_id); $block->setVar('side', $side); if (!$block_handler->insertBlock($block)) { - $error = true; + http_response_code(500); } } break; @@ -267,7 +268,7 @@ $block = $block_handler->get($order); $block->setVar('weight', $i); if (!$block_handler->insertBlock($block)) { - $error = true; + http_response_code(500); } ++$i; } diff --git a/htdocs/modules/system/admin/modulesadmin/main.php b/htdocs/modules/system/admin/modulesadmin/main.php index b8272cf67..c1aee6602 100644 --- a/htdocs/modules/system/admin/modulesadmin/main.php +++ b/htdocs/modules/system/admin/modulesadmin/main.php @@ -126,7 +126,7 @@ if ($module->getVar('weight') != 0) { $module->setVar('weight', $i); if (!$module_handler->insertModule($module)) { - $error = true; + http_response_code(500); } ++$i; } @@ -150,7 +150,9 @@ // Set value $module->setVar('isactive', !$old); if (!$module_handler->insertModule($module)) { - $error = true; + // a non-2xx status keeps system_setStatus() from flipping the icon + http_response_code(500); + break; } $blocks = $block_handler->getByModule($module_id); /* @var $block XoopsBlock */ @@ -178,7 +180,7 @@ // Set value $module->setVar('weight', !$old); if (!$module_handler->insertModule($module)) { - $error = true; + http_response_code(500); } else { echo !$old; } diff --git a/htdocs/user.php b/htdocs/user.php index a2f678e84..afba2674c 100644 --- a/htdocs/user.php +++ b/htdocs/user.php @@ -121,7 +121,7 @@ ); $xoops->footer(); } else { - if (!$xoops->security()->check()) { + if (!$xoops->security()->checkPost()) { $xoops->redirect('user.php', 3, implode('
', $xoops->security()->getErrors())); } $del_uid = $xoops->user->getVar("uid"); diff --git a/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php b/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php index a8c97dc31..2f5c89686 100644 --- a/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php +++ b/tests/unit/xoopsLib/Xoops/Core/SecurityTest.php @@ -106,7 +106,7 @@ public static function rejectedAjaxRequests(): array } #[\PHPUnit\Framework\Attributes\DataProvider('rejectedAjaxRequests')] - public function test_checkAjaxToken_rejects(string $method, ?string $token, bool $expired) + public function test_checkPost_rejects(string $method, ?string $token, bool $expired) { unset($_SESSION['XOOPS_TOKEN_SESSION']); $valid = $this->object->createToken(3600); @@ -126,7 +126,7 @@ public function test_checkAjaxToken_rejects(string $method, ?string $token, bool } try { - $this->assertFalse($this->object->checkAjaxToken()); + $this->assertFalse($this->object->checkPost(false)); } finally { $_GET = $savedGet; $_POST = $savedPost; @@ -134,12 +134,22 @@ public function test_checkAjaxToken_rejects(string $method, ?string $token, bool } } - public function test_checkAjaxToken_acceptsValidPost() + public function test_checkPost_acceptsValidPostAndKeepsToken() { unset($_SESSION['XOOPS_TOKEN_SESSION']); $_SERVER['REQUEST_METHOD'] = 'POST'; $_POST['XOOPS_TOKEN_REQUEST'] = $this->object->createToken(3600); - $this->assertTrue($this->object->checkAjaxToken()); + $this->assertTrue($this->object->checkPost(false)); + $this->assertTrue($this->object->checkPost(false)); + } + + public function test_checkPost_consumesTokenByDefault() + { + unset($_SESSION['XOOPS_TOKEN_SESSION']); + $_SERVER['REQUEST_METHOD'] = 'POST'; + $_POST['XOOPS_TOKEN_REQUEST'] = $this->object->createToken(); + $this->assertTrue($this->object->checkPost()); + $this->assertFalse($this->object->checkPost()); } public function test_createToken() diff --git a/xoops_lib/Xoops/Core/Security.php b/xoops_lib/Xoops/Core/Security.php index e15370a6f..3a95a7b85 100644 --- a/xoops_lib/Xoops/Core/Security.php +++ b/xoops_lib/Xoops/Core/Security.php @@ -60,7 +60,7 @@ public function check($clearIfValid = true, $token = false, $name = 'XOOPS_TOKEN */ public function requireAjaxToken($name = 'XOOPS_TOKEN'): void { - if (!$this->checkAjaxToken($name)) { + if (!$this->checkPost(false, $name)) { http_response_code(403); // media/xoops/xoops.js reloads the page on this marker to get a fresh token header('X-Xoops-Token: rejected'); @@ -69,17 +69,23 @@ public function requireAjaxToken($name = 'XOOPS_TOKEN'): void } /** - * Is this a POST carrying a valid token in its body? The token is kept. + * Is this a POST carrying a valid token in its body? * - * @param string $name name of session variable + * Use it instead of check() for state changes: check() also accepts a + * token from the query string, which leaks through logs and referers. + * + * @param bool $clearIfValid whether to clear the token after validation + * @param string $name name of session variable * * @return bool */ - public function checkAjaxToken($name = 'XOOPS_TOKEN'): bool + public function checkPost($clearIfValid = true, $name = 'XOOPS_TOKEN'): bool { - // the token must come in the body, a query string leaks through logs and referers - $token = Request::getString($name . '_REQUEST', '', 'POST'); - return 'POST' === Request::getMethod() && '' !== $token && $this->check(false, $token, $name); + if ('POST' !== Request::getMethod()) { + $this->setErrors('Request must be sent by POST'); + return false; + } + return $this->check($clearIfValid, Request::getString($name . '_REQUEST', '', 'POST'), $name); } /**