From bd4342d837387076488147ae20937ffed4449b5f Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Mon, 21 Sep 2026 02:55:52 +0200 Subject: [PATCH 1/2] fix(tree): drag-and-drop move runs the cycle and target permission checks The tree's movedocument AJAX handler accepted a folder dropped onto its own descendant (creating a cycle) and never checked document permissions (use_udperms) on the target, unlike the move action (a=51). MoveDocumentTargetGuard gains isInsideItself(), which walks the parent column rather than the alias listing or closure table so a stale cache cannot let a cycle through, and deniedForUser() wrapping the udperms check; the AJAX handler and the controller now share both. The controller's getParentIds()/allChildren() checks are replaced by the guard, run again after onBeforeMoveDocument may have changed the target. Co-Authored-By: Claude Opus 5 --- core/src/Controllers/MoveDocument.php | 12 ++- core/src/Support/MoveDocumentTargetGuard.php | 37 ++++++++++ .../Unit/MoveDocumentTargetGuardTest.php | 74 +++++++++++++++++++ manager/media/style/default/ajax.php | 4 + 4 files changed, 120 insertions(+), 7 deletions(-) diff --git a/core/src/Controllers/MoveDocument.php b/core/src/Controllers/MoveDocument.php index 7cd97535c3..749a316fd8 100644 --- a/core/src/Controllers/MoveDocument.php +++ b/core/src/Controllers/MoveDocument.php @@ -55,9 +55,8 @@ protected function handle() $document = $this->getDocument($documentID); - $parents = $this->managerTheme->getCore()->getParentIds($newParentID); - if (\in_array($document->getKey(), $parents, true)) { - $this->managerTheme->alertAndQuit('error_movedocument2'); + if (MoveDocumentTargetGuard::isInsideItself($document->getKey(), $newParentID)) { + $this->managerTheme->alertAndQuit('error_movedocument1'); } // check user has permission to move document to chosen location @@ -84,10 +83,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(); diff --git a/core/src/Support/MoveDocumentTargetGuard.php b/core/src/Support/MoveDocumentTargetGuard.php index c3fc792eb3..5767bea161 100644 --- a/core/src/Support/MoveDocumentTargetGuard.php +++ b/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(); + } } diff --git a/core/tests/Unit/MoveDocumentTargetGuardTest.php b/core/tests/Unit/MoveDocumentTargetGuardTest.php index 1b18ed2e7e..60882e036a 100644 --- a/core/tests/Unit/MoveDocumentTargetGuardTest.php +++ b/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, "'parent' => \$parent,")) + ->and(substr_count($controller, 'MoveDocumentTargetGuard::isInsideItself('))->toBe(2) + ->and($controller)->not->toContain('allChildren(')->not->toContain('getParentIds('); +}); diff --git a/manager/media/style/default/ajax.php b/manager/media/style/default/ajax.php index 2fd250deae..69deff3490 100755 --- a/manager/media/style/default/ajax.php +++ b/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) { From edb7f20f0d468c0d8a58af74399624b59b288437 Mon Sep 17 00:00:00 2001 From: Artur Kyryliuk Date: Mon, 21 Sep 2026 11:51:27 +0200 Subject: [PATCH 2/2] upd(tests): anchor move guard test on the model save (#2467) Co-Authored-By: Claude Opus 5 --- core/tests/Unit/MoveDocumentTargetGuardTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/tests/Unit/MoveDocumentTargetGuardTest.php b/core/tests/Unit/MoveDocumentTargetGuardTest.php index 60882e036a..32866fe151 100644 --- a/core/tests/Unit/MoveDocumentTargetGuardTest.php +++ b/core/tests/Unit/MoveDocumentTargetGuardTest.php @@ -84,7 +84,7 @@ function moveGuardTree(): void 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, "'parent' => \$parent,")) + ->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('); });