From 07d6a9f19e25cb0c5121994ed6a5b9df933407ed Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 19 Aug 2026 22:38:39 +0600 Subject: [PATCH 1/4] fix(acl): align Media menu and file editor permissions (#14468) Remove file_manager from the Media top menu parent so Sources remains reachable without the browser permission. Gate file editor Save on file_update and source policies, and add unpackFile to Media Browser view. --- _build/data/transport.core.menus.php | 2 +- .../Controllers/MediaAccessPolicyTest.php | 32 +++++++++++++ .../System/SystemFileEditControllerTest.php | 45 +++++++++++++++++++ .../modext/widgets/media/modx.browser.js | 27 +++++++++++ .../default/system/file/edit.class.php | 8 +++- .../3.3.0-clear-media-parent-permission.php | 25 +++++++++++ setup/includes/upgrades/mysql/3.3.0-pl.php | 12 +++++ 7 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 _build/test/Tests/Controllers/MediaAccessPolicyTest.php create mode 100644 _build/test/Tests/Controllers/System/SystemFileEditControllerTest.php create mode 100644 setup/includes/upgrades/common/3.3.0-clear-media-parent-permission.php create mode 100644 setup/includes/upgrades/mysql/3.3.0-pl.php diff --git a/_build/data/transport.core.menus.php b/_build/data/transport.core.menus.php index a7ee89b2078..f2b10446666 100644 --- a/_build/data/transport.core.menus.php +++ b/_build/data/transport.core.menus.php @@ -94,7 +94,7 @@ [ 'text' => 'media', 'description' => '', - 'permissions' => 'file_manager', + 'permissions' => '', 'action' => '', 'icon' => '', 'children' => [ diff --git a/_build/test/Tests/Controllers/MediaAccessPolicyTest.php b/_build/test/Tests/Controllers/MediaAccessPolicyTest.php new file mode 100644 index 00000000000..1dab486bbab --- /dev/null +++ b/_build/test/Tests/Controllers/MediaAccessPolicyTest.php @@ -0,0 +1,32 @@ +assertFileExists($menusFile); + + $contents = file_get_contents($menusFile); + $this->assertMatchesRegularExpression( + "/'text' => 'media',\s*\n\s*'description' => '',\s*\n\s*'permissions' => '',/", + $contents, + 'Media parent menu must not require file_manager; child items enforce their own permissions.' + ); + $this->assertMatchesRegularExpression( + "/'text' => 'file_browser',[\s\S]*?'permissions' => 'file_manager',/", + $contents, + 'Media Browser child must still require file_manager.' + ); + } +} diff --git a/_build/test/Tests/Controllers/System/SystemFileEditControllerTest.php b/_build/test/Tests/Controllers/System/SystemFileEditControllerTest.php new file mode 100644 index 00000000000..895793a073b --- /dev/null +++ b/_build/test/Tests/Controllers/System/SystemFileEditControllerTest.php @@ -0,0 +1,45 @@ +assertTrue($this->controller->checkPermissions()); + } + + public function testCanSaveGatesOnFileUpdateAndSourceSavePolicy() + { + $controllerFile = MODX_MANAGER_PATH . 'controllers/default/system/file/edit.class.php'; + $contents = file_get_contents($controllerFile); + $this->assertStringContainsString( + "\$this->canSave = \$this->modx->hasPermission('file_update') && \$source->checkPolicy('save');", + $contents + ); + $this->assertStringContainsString("\$source->checkPolicy('view')", $contents); + $this->assertStringNotContainsString("\$this->canSave = true;", $contents); + } + + public function testMediaBrowserViewDefinesUnpackFileHandler() + { + $browserFile = MODX_MANAGER_PATH . 'assets/modext/widgets/media/modx.browser.js'; + $contents = file_get_contents($browserFile); + $this->assertStringContainsString('unpackFile: function', $contents); + $this->assertStringContainsString('file: data.pathRelative', $contents); + } +} diff --git a/manager/assets/modext/widgets/media/modx.browser.js b/manager/assets/modext/widgets/media/modx.browser.js index 6723d3257f5..003aa90c8df 100644 --- a/manager/assets/modext/widgets/media/modx.browser.js +++ b/manager/assets/modext/widgets/media/modx.browser.js @@ -192,6 +192,33 @@ Ext.extend(MODx.browser.View,MODx.DataView,{ w.show(e.target); } + ,unpackFile: function(item,e) { + var node = this.cm.activeNode; + var data = this.lookup[node.id]; + MODx.msg.confirm({ + text: _('file_download_unzip') + ' ' + data.name + ,url: MODx.config.connector_url + ,params: { + action: 'Browser/File/Unpack' + ,file: data.pathRelative + ,wctx: MODx.ctx || '' + ,source: this.config.source + } + ,listeners: { + 'success': {fn: function() { + if (this.config.tree) { + if (this.config.tree.cm.activeNode && this.config.tree.cm.activeNode.id.match(/.*?\/$/)) { + this.config.tree.refreshParentNode(); + } else { + this.config.tree.refresh(); + } + } + this.run(); + }, scope: this} + } + }); + } + ,downloadFile: function(item,e) { var node = this.cm.activeNode; var data = this.lookup[node.id]; diff --git a/manager/controllers/default/system/file/edit.class.php b/manager/controllers/default/system/file/edit.class.php index 60de7d64891..95f45e2f63d 100644 --- a/manager/controllers/default/system/file/edit.class.php +++ b/manager/controllers/default/system/file/edit.class.php @@ -93,6 +93,12 @@ public function process(array $scriptProperties = []) return false; } + if (!$source->checkPolicy('view')) { + $this->failure($this->modx->lexicon('permission_denied')); + + return false; + } + if ($this->fileRecord = $source->getObjectContents($this->filename)) { $this->fileRecord['source'] = $source->get('id'); } @@ -114,7 +120,7 @@ public function process(array $scriptProperties = []) if (!empty($this->fileRecord['last_modified'])) { $this->fileRecord['last_modified'] = $formatter->formatDateTime($this->fileRecord['last_modified']); } - $this->canSave = true; + $this->canSave = $this->modx->hasPermission('file_update') && $source->checkPolicy('save'); $placeholders['fa'] = $this->fileRecord; $placeholders['OnFileEditFormPrerender'] = $this->fireEvents(); diff --git a/setup/includes/upgrades/common/3.3.0-clear-media-parent-permission.php b/setup/includes/upgrades/common/3.3.0-clear-media-parent-permission.php new file mode 100644 index 00000000000..c10e1317155 --- /dev/null +++ b/setup/includes/upgrades/common/3.3.0-clear-media-parent-permission.php @@ -0,0 +1,25 @@ +getObject(modMenu::class, [ + 'text' => 'media', + 'permissions' => 'file_manager', +]); +if ($mediaMenu instanceof modMenu) { + $mediaMenu->set('permissions', ''); + if ($mediaMenu->save()) { + $mediaMenu->rebuildCache(''); + } +} diff --git a/setup/includes/upgrades/mysql/3.3.0-pl.php b/setup/includes/upgrades/mysql/3.3.0-pl.php new file mode 100644 index 00000000000..efff0ed755b --- /dev/null +++ b/setup/includes/upgrades/mysql/3.3.0-pl.php @@ -0,0 +1,12 @@ + Date: Wed, 19 Aug 2026 22:40:35 +0600 Subject: [PATCH 2/4] style(media): eslint-clean unpackFile handler in modx.browser.js Use const, template literals, and trailing commas in the unpackFile block. --- .../modext/widgets/media/modx.browser.js | 46 ++++++++++--------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/manager/assets/modext/widgets/media/modx.browser.js b/manager/assets/modext/widgets/media/modx.browser.js index 003aa90c8df..6b4241d3572 100644 --- a/manager/assets/modext/widgets/media/modx.browser.js +++ b/manager/assets/modext/widgets/media/modx.browser.js @@ -192,29 +192,33 @@ Ext.extend(MODx.browser.View,MODx.DataView,{ w.show(e.target); } - ,unpackFile: function(item,e) { - var node = this.cm.activeNode; - var data = this.lookup[node.id]; + // eslint-disable-next-line comma-style, comma-spacing + ,unpackFile: function(item, e) { + const node = this.cm.activeNode, + data = this.lookup[node.id]; MODx.msg.confirm({ - text: _('file_download_unzip') + ' ' + data.name - ,url: MODx.config.connector_url - ,params: { - action: 'Browser/File/Unpack' - ,file: data.pathRelative - ,wctx: MODx.ctx || '' - ,source: this.config.source - } - ,listeners: { - 'success': {fn: function() { - if (this.config.tree) { - if (this.config.tree.cm.activeNode && this.config.tree.cm.activeNode.id.match(/.*?\/$/)) { - this.config.tree.refreshParentNode(); - } else { - this.config.tree.refresh(); + text: `${_('file_download_unzip')} ${data.name}`, + url: MODx.config.connector_url, + params: { + action: 'Browser/File/Unpack', + file: data.pathRelative, + wctx: MODx.ctx || '', + source: this.config.source + }, + listeners: { + success: { + fn: function() { + if (this.config.tree) { + if (this.config.tree.cm.activeNode && this.config.tree.cm.activeNode.id.match(/.*?\/$/)) { + this.config.tree.refreshParentNode(); + } else { + this.config.tree.refresh(); + } } - } - this.run(); - }, scope: this} + this.run(); + }, + scope: this + } } }); } From 497f3276403e7fdb8f873b91fd21be095f3ffca1 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 19 Aug 2026 22:55:43 +0600 Subject: [PATCH 3/4] fix(acl): tighten file_view gates and Media permission lexicon (#14468) Use source view policy in Browser/File/Get, hide tree page links without file_view, and clarify en permission descriptions for visibility and upload vs create. --- .../Controllers/MediaAccessPolicyTest.php | 27 +++++++++++++++++++ core/lexicon/en/permissions.inc.php | 10 +++---- .../Processors/Browser/File/Get.php | 14 +--------- .../src/Revolution/Sources/modMediaSource.php | 13 +++++++-- .../Revolution/Sources/modS3MediaSource.php | 6 ++++- 5 files changed, 49 insertions(+), 21 deletions(-) diff --git a/_build/test/Tests/Controllers/MediaAccessPolicyTest.php b/_build/test/Tests/Controllers/MediaAccessPolicyTest.php index 1dab486bbab..9cece80d105 100644 --- a/_build/test/Tests/Controllers/MediaAccessPolicyTest.php +++ b/_build/test/Tests/Controllers/MediaAccessPolicyTest.php @@ -29,4 +29,31 @@ public function testMediaParentMenuHasNoFileManagerPermissionInTransport() 'Media Browser child must still require file_manager.' ); } + + public function testBrowserFileGetUsesViewSourcePolicy() + { + $file = MODX_CORE_PATH . 'src/Revolution/Processors/Browser/File/Get.php'; + $contents = file_get_contents($file); + $this->assertStringContainsString("public \$policy = 'view';", $contents); + $this->assertStringNotContainsString("checkPolicy('delete')", $contents); + } + + public function testFileTreePageLinkRequiresFileViewPermission() + { + $sourceFile = MODX_CORE_PATH . 'src/Revolution/Sources/modMediaSource.php'; + $contents = file_get_contents($sourceFile); + $this->assertStringContainsString( + "&& \$this->hasPermission('file_view')\n && \$canView", + $contents + ); + } + + public function testDirectoryChmodLexiconDescribesVisibility() + { + $lexiconFile = MODX_CORE_PATH . 'lexicon/en/permissions.inc.php'; + $contents = file_get_contents($lexiconFile); + $this->assertStringContainsString('set directory visibility', $contents); + $this->assertStringContainsString('Does not include uploading files', $contents); + $this->assertStringContainsString('Does not include creating empty files', $contents); + } } diff --git a/core/lexicon/en/permissions.inc.php b/core/lexicon/en/permissions.inc.php index 43d2ae252d5..2bb103bf696 100644 --- a/core/lexicon/en/permissions.inc.php +++ b/core/lexicon/en/permissions.inc.php @@ -40,7 +40,7 @@ $_lang['perm.delete_template_desc'] = 'To delete or remove any Templates.'; $_lang['perm.delete_tv_desc'] = 'To delete or remove any TVs.'; $_lang['perm.delete_user_desc'] = 'To delete or remove any Users.'; -$_lang['perm.directory_chmod_desc'] = 'To chmod a physical directory.'; +$_lang['perm.directory_chmod_desc'] = 'To set directory visibility (public/private) in the manager. Also requires the Media Source save policy.'; $_lang['perm.directory_create_desc'] = 'To create a physical directory.'; $_lang['perm.directory_list_desc'] = 'To list subdirectories for a physical directory.'; $_lang['perm.directory_remove_desc'] = 'To delete a physical directory.'; @@ -65,15 +65,15 @@ $_lang['perm.error_log_erase_desc'] = 'To erase the error log.'; $_lang['perm.error_log_view_desc'] = 'To view the error log.'; $_lang['perm.export_static_desc'] = 'To export the site to static HTML.'; -$_lang['perm.file_create_desc'] = 'To create a file.'; +$_lang['perm.file_create_desc'] = 'To create a new empty file. Does not include uploading files.'; $_lang['perm.file_list_desc'] = 'To list files within a given physical directory.'; $_lang['perm.file_manager_desc'] = 'To use the file manager utility.'; $_lang['perm.file_remove_desc'] = 'To delete physical files.'; $_lang['perm.file_tree_desc'] = 'To view the Files Tree on the left nav.'; -$_lang['perm.file_update_desc'] = 'To edit the content of physical files. WARNING: grants ability to execute arbitrary server-side code.'; -$_lang['perm.file_upload_desc'] = 'To upload files to a directory.'; +$_lang['perm.file_update_desc'] = 'To edit the content of physical files. Also requires file_view and the Media Source save policy. WARNING: grants ability to execute arbitrary server-side code.'; +$_lang['perm.file_upload_desc'] = 'To upload files to a directory. Does not include creating empty files.'; $_lang['perm.file_unpack_desc'] = 'To extract zip archives.'; -$_lang['perm.file_view_desc'] = 'To view the contents of a file.'; +$_lang['perm.file_view_desc'] = 'To view the contents of a file. Media Source view policy must also pass.'; $_lang['perm.flush_sessions_desc'] = 'Can flush Sessions across the site.'; $_lang['perm.frames_desc'] = 'To use the MODX Manager UI at all.'; $_lang['perm.help_desc'] = 'To view the Help page.'; diff --git a/core/src/Revolution/Processors/Browser/File/Get.php b/core/src/Revolution/Processors/Browser/File/Get.php index 653ead9a36b..bd185c95dbf 100644 --- a/core/src/Revolution/Processors/Browser/File/Get.php +++ b/core/src/Revolution/Processors/Browser/File/Get.php @@ -23,22 +23,10 @@ class Get extends Browser { public $permission = 'file_view'; + public $policy = 'view'; public $languageTopics = ['file']; - /** - * @return array|bool|string - */ - public function initialize() - { - if (!$this->getSource() || !$this->source->checkPolicy('delete')) { - return $this->failure($this->modx->lexicon('permission_denied')); - } - - return true; - } - - /** * @return array|bool|mixed|string */ diff --git a/core/src/Revolution/Sources/modMediaSource.php b/core/src/Revolution/Sources/modMediaSource.php index 2d43cf04911..43ff2b6a55f 100644 --- a/core/src/Revolution/Sources/modMediaSource.php +++ b/core/src/Revolution/Sources/modMediaSource.php @@ -1869,6 +1869,7 @@ protected function buildFileList($path, $ext, $image_extensions, $bases, $proper $editAction = $this->getEditActionId(); $canSave = $this->checkPolicy('save'); $canRemove = $this->checkPolicy('remove'); + $canView = $this->checkPolicy('view'); $id = rawurlencode(htmlspecialchars_decode($path, ENT_COMPAT)); $cls = []; @@ -1888,7 +1889,11 @@ protected function buildFileList($path, $ext, $image_extensions, $bases, $proper $cls[] = 'pupdate'; } $page = null; - if (!$this->isFileBinary($path)) { + if ( + !$this->isFileBinary($path) + && $this->hasPermission('file_view') + && $canView + ) { $page = !empty($editAction) ? '?a=' . $editAction . '&file=' . $id . '&wctx=' . $this->ctx->get('key') . '&source=' . $this->get('id') : null; @@ -1960,7 +1965,11 @@ protected function buildFileBrowserViewList($path, $ext, $image_extensions, $bas $editAction = $this->getEditActionId(); $page = null; - if (!$this->isFileBinary($path)) { + if ( + !$this->isFileBinary($path) + && $this->hasPermission('file_view') + && $this->checkPolicy('view') + ) { $page = !empty($editAction) ? '?a=' . $editAction . '&file=' . $path . '&wctx=' . $this->ctx->get('key') . '&source=' . $this->get('id') : null; diff --git a/core/src/Revolution/Sources/modS3MediaSource.php b/core/src/Revolution/Sources/modS3MediaSource.php index c6db2ac5d21..22b4a07470e 100644 --- a/core/src/Revolution/Sources/modS3MediaSource.php +++ b/core/src/Revolution/Sources/modS3MediaSource.php @@ -732,7 +732,11 @@ protected function buildFileBrowserViewList($path, $ext, $image_extensions, $bas $editAction = $this->getEditActionId(); $page = null; - if (!$this->isFileBinary($path)) { + if ( + !$this->isFileBinary($path) + && $this->hasPermission('file_view') + && $this->checkPolicy('view') + ) { $page = !empty($editAction) ? '?a=' . $editAction . '&file=' . $path . From 1113f5bad87b14793dc8add8c1ca181cfbfe2055 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Thu, 20 Aug 2026 10:15:22 +0600 Subject: [PATCH 4/4] style: trim trailing whitespace in media source qtip HTML --- core/src/Revolution/Sources/modMediaSource.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/Revolution/Sources/modMediaSource.php b/core/src/Revolution/Sources/modMediaSource.php index 43ff2b6a55f..34b7630e331 100644 --- a/core/src/Revolution/Sources/modMediaSource.php +++ b/core/src/Revolution/Sources/modMediaSource.php @@ -1937,10 +1937,10 @@ protected function buildFileList($path, $ext, $image_extensions, $bases, $proper $preview_image = $this->buildManagerImagePreview($path, $ext, $imageWidth, $imageHeight, $bases, $properties); // Once minimum php requirement is brought up to 7.4+, heredoc closing can be indented $file_list['qtip'] = << QTIP;