fix(acl): Media access policy menu, file editor, and browser unpack

_build/data/transport.core.menus.php

@@ -94,7 +94,7 @@
             [
                 'text' => 'media',
                 'description' => '',
-                'permissions' => 'file_manager',
+                'permissions' => '',
                 'action' => '',
                 'icon' => '<i class="icon-file-image-o icon"></i>',
                 'children' => [

_build/test/Tests/Controllers/MediaAccessPolicyTest.php

@@ -0,0 +1,59 @@
+<?php
+
+namespace MODX\Revolution\Tests\Controllers;
+
+use MODX\Revolution\MODxTestCase;
+
+/**
+ * Tests for Media access policy fixes (#14468)
+ *
+ * @package modx-test
+ * @group Controllers
+ */
+class MediaAccessPolicyTest extends MODxTestCase
+{
+    public function testMediaParentMenuHasNoFileManagerPermissionInTransport()
+    {
+        $menusFile = dirname(__DIR__, 3) . '/data/transport.core.menus.php';
+        $this->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.'
+        );
+    }
+
+    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);
+    }
+}

_build/test/Tests/Controllers/System/SystemFileEditControllerTest.php

@@ -0,0 +1,45 @@
+<?php
+
+namespace MODX\Revolution\Tests\Controllers\System;
+
+use MODX\Revolution\MODxControllerTestCase;
+
+/**
+ * Tests for system/file/edit controller (#14468 file_update gate)
+ *
+ * @package modx-test
+ * @group Controllers
+ */
+class SystemFileEditControllerTest extends MODxControllerTestCase
+{
+    /** @var \SystemFileEditManagerController $controller */
+    public $controller;
+
+    public $controllerName = 'SystemFileEditManagerController';
+    public $controllerPath = 'system/file/edit';
+
+    public function testCheckPermissionsRequiresFileView()
+    {
+        $this->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);
+    }
+}

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. <strong>WARNING: grants ability to execute arbitrary server-side code.</strong>';
-$_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. <strong>WARNING: grants ability to execute arbitrary server-side code.</strong>';
+$_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.';

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
      */

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;
@@ -1932,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
-                    <img 
-                        src="{$preview_image['src']}" 
-                        width="{$preview_image['width']}" 
-                        height="{$preview_image['height']}" 
+                    <img
+                        src="{$preview_image['src']}"
+                        width="{$preview_image['width']}"
+                        height="{$preview_image['height']}"
                         alt="{$path}"
                     >
 QTIP;
@@ -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;

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 .

manager/assets/modext/widgets/media/modx.browser.js

@@ -192,6 +192,37 @@ Ext.extend(MODx.browser.View,MODx.DataView,{
         w.show(e.target);
     }
 
+    // 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();
+                            }
+                        }
+                        this.run();
+                    },
+                    scope: this
+                }
+            }
+        });
+    }
+
     ,downloadFile: function(item,e) {
         var node = this.cm.activeNode;
         var data = this.lookup[node.id];

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();

setup/includes/upgrades/common/3.3.0-clear-media-parent-permission.php

@@ -0,0 +1,25 @@
+<?php
+
+/**
+ * Clear file_manager permission from the Media top menu parent.
+ *
+ * The parent should remain visible when the user can access any child
+ * (Media Browser or Media Sources). file_manager stays on media/browser only.
+ *
+ * @var modX $modx
+ * @package setup
+ */
+
+use MODX\Revolution\modMenu;
+
+/** @var modMenu $mediaMenu */
+$mediaMenu = $modx->getObject(modMenu::class, [
+    'text' => 'media',
+    'permissions' => 'file_manager',
+]);
+if ($mediaMenu instanceof modMenu) {
+    $mediaMenu->set('permissions', '');
+    if ($mediaMenu->save()) {
+        $mediaMenu->rebuildCache('');
+    }
+}

setup/includes/upgrades/mysql/3.3.0-pl.php

@@ -0,0 +1,12 @@
+<?php
+
+/**
+ * Specific upgrades for Revolution 3.3.0-pl
+ *
+ * @var modX $modx
+ * @var modInstallVersion $this
+ * @package setup
+ * @subpackage upgrades
+ */
+
+include dirname(__DIR__) . '/common/3.3.0-clear-media-parent-permission.php';