Harden reorder, save, and update against crafted payloads

  - Reject duplicate item IDs in reorder tree payload (prevents cycles)
  - Add visited-set guard in menus_item_can_be_enabled() parent walk
  - On edit, derive category from DB not POST (prevents cross-category moves)
  - Protected items cannot be re-parented via tampered form
  - Wrap update helpers in try/catch so failures propagate to module errors
  - Include items_protected in seed item lookup (prevents overwriting custom items)

Author: mambax7

htdocs/modules/system/admin/menus/main.php

@@ -159,9 +159,14 @@ function menus_item_can_be_enabled(XoopsMenusItemsHandler $itemHandler, XoopsMen
         return false;
     }
 
-    // Walk the full parent chain
+    // Walk the full parent chain (with visited-set guard against cycles)
     $parentId = (int) $item->getVar('items_pid');
+    $visited  = [];
     while ($parentId > 0) {
+        if (isset($visited[$parentId])) {
+            break; // Cycle detected — stop walking
+        }
+        $visited[$parentId] = true;
         $parent = $itemHandler->get($parentId);
         if (!is_object($parent)) {
             break;
@@ -481,20 +486,25 @@ function menus_assign_template_defaults(\XoopsTpl $tpl, string $op): void
         menus_require_token(false);
         $itemHandler = menus_item_handler();
         $itemId      = Request::getInt('items_id', 0, 'POST');
-        $catId       = Request::getInt('items_cid', 0, 'POST');
-        $xoopsTpl->assign('category_id', $catId);
 
         if ($itemId > 0) {
             $item = $itemHandler->get($itemId);
             if (!is_object($item) || $item->isNew()) {
                 redirect_header(MENUS_ADMIN_URL, 3, _AM_SYSTEM_MENUS_ERROR_ITEMNOTFOUND);
             }
+            // Existing item: derive category from DB, not POST (prevents cross-category moves via tampered form)
+            $catId    = (int) $item->getVar('items_cid');
+            $parentId = (int) $item->getVar('items_protected')
+                ? (int) $item->getVar('items_pid')   // Protected items keep their parent
+                : Request::getInt('items_pid', 0, 'POST');
         } else {
-            $item = $itemHandler->create();
+            $item     = $itemHandler->create();
+            $catId    = Request::getInt('items_cid', 0, 'POST');
+            $parentId = Request::getInt('items_pid', 0, 'POST');
         }
+        $xoopsTpl->assign('category_id', $catId);
 
         $isProtected = (bool) $item->getVar('items_protected');
-        $parentId    = Request::getInt('items_pid', 0, 'POST');
 
         // Validate parent
         if ($parentId > 0) {
@@ -675,6 +685,28 @@ function menus_assign_template_defaults(\XoopsTpl $tpl, string $op): void
             menus_send_json(false, ['message' => _AM_SYSTEM_MENUS_ERROR_ITEMDEPTH]);
         }
 
+        // Collect all IDs from the tree and reject duplicates
+        $seenIds = [];
+        $collectIds = function (array $nodes) use (&$collectIds, &$seenIds): bool {
+            foreach ($nodes as $node) {
+                $id = (int) ($node['id'] ?? 0);
+                if ($id <= 0) {
+                    continue;
+                }
+                if (isset($seenIds[$id])) {
+                    return false; // Duplicate — reject entire payload
+                }
+                $seenIds[$id] = true;
+                if (!empty($node['children']) && !$collectIds($node['children'])) {
+                    return false;
+                }
+            }
+            return true;
+        };
+        if (!$collectIds($tree)) {
+            menus_send_json(false, ['message' => 'Duplicate item IDs in tree']);
+        }
+
         // Walk the tree and update positions
         $itemHandler = menus_item_handler();
         $position    = 0;

htdocs/modules/system/include/update.php

@@ -149,10 +149,14 @@ function system_menu_update(XoopsModule $module): void
     $db = \XoopsDatabaseFactory::getDatabaseConnection();
     $mid = (int) $module->getVar('mid');
 
-    system_menu_create_tables($db);
-    system_menu_normalize_schema($db);
-    system_menu_migrate_unsafe_urls($db);
-    system_menu_seed_defaults($db, $mid);
+    try {
+        system_menu_create_tables($db);
+        system_menu_normalize_schema($db);
+        system_menu_migrate_unsafe_urls($db);
+        system_menu_seed_defaults($db, $mid);
+    } catch (\Throwable $e) {
+        $module->setErrors('Menu migration failed: ' . $e->getMessage());
+    }
 }
 
 /**
@@ -336,11 +340,13 @@ function system_menu_ensure_item(XoopsMySQLDatabase $db, int $categoryId, array
 {
     $table = $db->prefix('menusitems');
     $result = $db->query(sprintf(
-        "SELECT `items_id`, `items_active` FROM `%s` WHERE `items_cid` = %d AND `items_title` = %s"
+        "SELECT `items_id`, `items_active` FROM `%s`"
+        . " WHERE `items_cid` = %d AND `items_title` = %s AND `items_protected` = %d"
         . " ORDER BY `items_id` ASC",
         $table,
         $categoryId,
-        $db->quote($definition['title'])
+        $db->quote($definition['title']),
+        (int) $definition['protected']
     ));
     if ($db->isResultSet($result) && ($result instanceof \mysqli_result) && ($row = $db->fetchArray($result))) {
         $active = (int) ($row['items_active'] ?? $definition['active']);