fix(tree): drag-and-drop move runs the cycle and target permission checks

core/src/Controllers/MoveDocument.php

@@ -55,12 +55,11 @@ protected function handle()
 
         $document = $this->getDocument($documentID);
 
-        // the form (a=51) checks the source document; the action must too, or the check is bypassable
-        $this->checkDocumentPermission($document->getKey(), 'access_permission_denied');
-
-        $parents = $this->managerTheme->getCore()->getParentIds($newParentID);
-        if (\in_array($document->getKey(), $parents, true)) {
-            $this->managerTheme->alertAndQuit('error_movedocument2');
+        // the form (a=51) checks the source document; the action must too, or the check is bypassable
+        $this->checkDocumentPermission($document->getKey(), 'access_permission_denied');
+
+        if (MoveDocumentTargetGuard::isInsideItself($document->getKey(), $newParentID)) {
+            $this->managerTheme->alertAndQuit('error_movedocument1');
         }
 
         // check user has permission to move document to chosen location
@@ -87,10 +86,9 @@ protected function handle()
             if (MoveDocumentTargetGuard::blocksParent($parentDocument)) {
                 $this->managerTheme->alertAndQuit('error_parent_deleted');
             };
-            $children = allChildren($document->getKey());
-            if (\in_array($parentDocument->getKey(), $children, true)) {
-                $this->managerTheme->alertAndQuit('You cannot move a document to a child document!', false);
-            }
+            if (MoveDocumentTargetGuard::isInsideItself($document->getKey(), $parentDocument->getKey())) {
+                $this->managerTheme->alertAndQuit('error_movedocument1');
+            }
 
             $parentDocument->isfolder = true;
             $parentDocument->save();

core/src/Support/MoveDocumentTargetGuard.php

@@ -2,6 +2,7 @@
 
 namespace EvolutionCMS\Support;
 
+use EvolutionCMS\Legacy\Permissions;
 use EvolutionCMS\Models\SiteContent;
 
 class MoveDocumentTargetGuard
@@ -10,4 +11,40 @@ public static function blocksParent(?SiteContent $parentDocument): bool
     {
         return $parentDocument === null || (int)$parentDocument->deleted === 1;
     }
+
+    /**
+     * True when the target is the document itself or one of its descendants (a move would create a cycle).
+     * Walks the parent column, not the alias listing or closure table, so a stale cache cannot let a cycle through.
+     * @since 3.5.8
+     */
+    public static function isInsideItself(int $documentId, int $parentId): bool
+    {
+        $seen = [];
+        while ($parentId > 0 && !isset($seen[$parentId])) {
+            if ($parentId === $documentId) {
+                return true;
+            }
+            $seen[$parentId] = true;
+            $parentId = (int)(SiteContent::withTrashed()->where('id', $parentId)->value('parent') ?? 0);
+        }
+
+        return false;
+    }
+
+    /**
+     * True when document permissions (use_udperms) deny the current manager user the target folder.
+     * @since 3.5.8
+     */
+    public static function deniedForUser(int $parentId): bool
+    {
+        if (!evo()->getConfig('use_udperms')) {
+            return false;
+        }
+        $udperms = new Permissions();
+        $udperms->user = evo()->getLoginUserID('mgr');
+        $udperms->document = $parentId;
+        $udperms->role = $_SESSION['mgrRole'] ?? 0;
+
+        return !$udperms->checkPermissions();
+    }
 }

core/tests/Unit/MoveDocumentTargetGuardTest.php

@@ -14,3 +14,77 @@
         ->and(MoveDocumentTargetGuard::blocksParent($deletedParent))->toBeTrue()
         ->and(MoveDocumentTargetGuard::blocksParent($activeParent))->toBeFalse();
 });
+
+class MoveGuardCoreStub
+{
+    public array $config = [];
+    public function getConfig($name = '', $default = null) { return $this->config[$name] ?? $default; }
+    public function getLoginUserID($context = '') { return 1; }
+}
+
+function moveGuardTree(): void
+{
+    $capsule = new Illuminate\Database\Capsule\Manager();
+    $capsule->addConnection(['driver' => 'sqlite', 'database' => ':memory:', 'prefix' => '']);
+    $capsule->setAsGlobal();
+    $capsule->bootEloquent();
+    Illuminate\Database\Eloquent\Model::setConnectionResolver($capsule->getDatabaseManager());
+    $capsule->getConnection()->getSchemaBuilder()->create('site_content', function (Illuminate\Database\Schema\Blueprint $t) {
+        $t->increments('id');
+        $t->unsignedInteger('parent')->default(0);
+        $t->unsignedInteger('deleted')->default(0);
+        $t->boolean('privatemgr')->default(0);
+    });
+    // 1 > 2 > 3 > 4, 5 at root, 6 > 7 forms a corrupt cycle
+    $capsule->table('site_content')->insert([
+        ['id' => 1, 'parent' => 0], ['id' => 2, 'parent' => 1], ['id' => 3, 'parent' => 2], ['id' => 4, 'parent' => 3],
+        ['id' => 5, 'parent' => 0], ['id' => 6, 'parent' => 7], ['id' => 7, 'parent' => 6],
+    ]);
+}
+
+test('a document cannot be moved onto itself or into its own subtree', function () {
+    moveGuardTree();
+
+    expect(MoveDocumentTargetGuard::isInsideItself(2, 2))->toBeTrue()
+        ->and(MoveDocumentTargetGuard::isInsideItself(2, 3))->toBeTrue()
+        ->and(MoveDocumentTargetGuard::isInsideItself(2, 4))->toBeTrue()
+        ->and(MoveDocumentTargetGuard::isInsideItself(2, 1))->toBeFalse()
+        ->and(MoveDocumentTargetGuard::isInsideItself(2, 5))->toBeFalse()
+        ->and(MoveDocumentTargetGuard::isInsideItself(2, 0))->toBeFalse()
+        ->and(MoveDocumentTargetGuard::isInsideItself(3, 999))->toBeFalse()
+        ->and(MoveDocumentTargetGuard::isInsideItself(5, 6))->toBeFalse();
+});
+
+test('target permission check is skipped without udperms and passes for administrators', function () {
+    moveGuardTree();
+    defined('IN_MANAGER_MODE') || define('IN_MANAGER_MODE', false);
+    defined('IN_INSTALL_MODE') || define('IN_INSTALL_MODE', false);
+    defined('EVO_API_MODE') || define('EVO_API_MODE', true);
+    defined('EVO_CLASS') || define('EVO_CLASS', MoveGuardCoreStub::class);
+    require_once dirname(__DIR__, 2) . '/functions/preload.php';
+    global $evo;
+    $evo = new MoveGuardCoreStub();
+    $_SESSION['mgrRole'] = 2;
+
+    expect(MoveDocumentTargetGuard::deniedForUser(1))->toBeFalse();
+
+    $evo->config['use_udperms'] = 1;
+    $_SESSION['mgrRole'] = 1;
+    expect(MoveDocumentTargetGuard::deniedForUser(1))->toBeFalse();
+
+    $evo = null;
+    unset($_SESSION['mgrRole']);
+});
+
+test('tree drag-and-drop and the move action both run the cycle and target permission guards', function () {
+    $root = dirname(__DIR__, 3);
+    $ajax = file_get_contents("$root/manager/media/style/default/ajax.php");
+    $move = substr($ajax, strpos($ajax, "case 'movedocument'"), strpos($ajax, "case 'getLockedElements'") - strpos($ajax, "case 'movedocument'"));
+    $controller = file_get_contents("$root/core/src/Controllers/MoveDocument.php");
+
+    expect($move)->toContain('MoveDocumentTargetGuard::isInsideItself($id, $parent)')->toContain('MoveDocumentTargetGuard::deniedForUser($parent)')
+        ->and(strpos($move, 'isInsideItself'))->toBeGreaterThan(strpos($move, '$parent = $eventParent;'))
+        ->and(strpos($move, 'isInsideItself'))->toBeLessThan(strpos($move, '$document->parent = $parent;'))
+        ->and(substr_count($controller, 'MoveDocumentTargetGuard::isInsideItself('))->toBe(2)
+        ->and($controller)->not->toContain('allChildren(')->not->toContain('getParentIds(');
+});

manager/media/style/default/ajax.php

@@ -628,6 +628,10 @@
                     $parentDeleted = $parent > 0 && MoveDocumentTargetGuard::blocksParent($parentDocument);
                     if ($parentDeleted) {
                         $json['errors'] = $_lang['error_parent_deleted'];
+                    } elseif (MoveDocumentTargetGuard::isInsideItself($id, $parent)) {
+                        $json['errors'] = $_lang['error_movedocument1'];
+                    } elseif ($parent != $parentOld && MoveDocumentTargetGuard::deniedForUser($parent)) {
+                        $json['errors'] = $_lang['access_permission_parent_denied'];
                     } elseif (empty($json['errors'])) {
                         // check privileges user for move docs
                         if (!empty(evo()->config['tree_show_protected']) && $role != 1) {