From b3e7977b55492a968289dd9dcc5f207b1e25d9a8 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 28 Sep 2023 10:59:55 -0400 Subject: [PATCH 1/2] Improve file system error feedback Provides more action-specific messaging when a file/folder action can not be taken --- core/lexicon/en/file.inc.php | 16 ++++++++++-- .../src/Revolution/Sources/modMediaSource.php | 26 ++++++++++++++++--- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/core/lexicon/en/file.inc.php b/core/lexicon/en/file.inc.php index ac8ad0a7648..5efce137955 100644 --- a/core/lexicon/en/file.inc.php +++ b/core/lexicon/en/file.inc.php @@ -14,10 +14,13 @@ $_lang['file_edit'] = 'Edit File'; $_lang['file_open'] = 'Open File Url'; $_lang['file_err_ae'] = 'File %s already exists'; -$_lang['file_err_create'] = 'An unknown error occurred while trying to create the file.'; +$_lang['file_err_create_general_exception'] = 'An unknown error occurred while trying to create the file. Please check the MODX and/or server error logs for more information.'; +$_lang['file_err_create_write_exception'] = 'The file could not be created. Please verify you have write permissions for its target directory and try again.'; $_lang['file_err_ext_not_allowed'] = 'File extension `[[+ext]]` is not permitted.'; $_lang['file_err_filter'] = 'No files match the specified filter.'; $_lang['file_err_invalid'] = 'The file is not a regular file and cannot be deleted.'; +$_lang['file_err_move_general_exception'] = 'An unknown error occurred while trying to move the file. Please check the MODX and/or server error logs for more information.'; +$_lang['file_err_move_write_exception'] = 'The file could not be moved. Please verify you have write permissions for both the file and its target directory and try again.'; $_lang['file_err_nf'] = 'File does not exist!'; $_lang['file_err_ns'] = 'Please specify a valid file.'; $_lang['file_err_open'] = 'Cannot open file: '; @@ -25,6 +28,8 @@ $_lang['file_err_remove'] = 'MODX failed to delete the file. Please make sure your permissions are set correctly.'; $_lang['file_err_too_large'] = 'Uploaded file is too large at [[+size]] bytes. Please ensure your files are less than [[+allowed]] bytes.'; $_lang['file_err_unzip'] = 'Unzip Failed!'; +$_lang['file_err_update_general_exception'] = 'An unknown system error occurred while trying to update this file. Please check the MODX and/or server error logs for more information.'; +$_lang['file_err_update_write_exception'] = 'The file could not be updated. Please verify you have write permissions for it and try again.'; $_lang['file_err_upload'] = 'An error occurred while trying to upload the files.'; $_lang['file_extensions'] = 'File Extensions'; $_lang['file_folder_path'] = 'Path'; @@ -40,9 +45,12 @@ $_lang['file_folder_err_ae'] = 'A directory already exists with that name in that location.'; $_lang['file_folder_err_create'] = 'An unknown error occurred while trying to create the directory.'; $_lang['file_folder_err_invalid'] = 'The specified directory is not a directory.'; +$_lang['file_folder_err_move_general_exception'] = 'An unknown error occurred while trying to move the directory. Please check the MODX and/or server error logs for more information.'; +$_lang['file_folder_err_move_write_exception'] = 'The directory could not be moved. Please verify you have write permissions for both this directory and its target directory and try again.'; $_lang['file_folder_err_ns'] = 'Please specify a valid directory.'; $_lang['file_folder_err_ns_name'] = 'Please specify a valid name for the directory.'; -$_lang['file_folder_err_rename'] = 'An unknown error occurred while trying to rename the directory.'; +$_lang['file_folder_err_rename_general_exception'] = 'An unknown error occurred while trying to rename the directory. Please check the MODX and/or server error logs for more information.'; +$_lang['file_folder_err_rename_write_exception'] = 'The directory could not be renamed. Please verify you have write permissions for it and try again.'; $_lang['file_folder_err_rename_protected'] = 'Renaming the protected system directory is not permitted.'; $_lang['file_folder_err_remove'] = 'An error occurred while trying to delete the directory.'; $_lang['file_folder_err_remove_protected'] = 'Deleting the protected system directory is not permitted.'; @@ -115,3 +123,7 @@ $_lang['upload.clear_list.notpermitted'] = 'Delete not permitted only'; $_lang['upload.msg.title.error'] = 'Error'; $_lang['upload.upload.success'] = 'Upload successful'; + +/** Deprecated keys */ +$_lang['file_err_create'] = $_lang['file_err_create_general_exception']; +$_lang['file_folder_err_rename'] = $_lang['file_folder_err_rename_general_exception']; diff --git a/core/src/Revolution/Sources/modMediaSource.php b/core/src/Revolution/Sources/modMediaSource.php index e44f8bcee15..53fb8601606 100644 --- a/core/src/Revolution/Sources/modMediaSource.php +++ b/core/src/Revolution/Sources/modMediaSource.php @@ -726,7 +726,11 @@ public function createObject($path, $name, $content) try { $this->filesystem->write($path, $content, $config); } catch (FilesystemException | UnableToWriteFile $e) { - $this->addError('name', $this->xpdo->lexicon('file_err_create')); + $messageKey = $e instanceof UnableToWriteFile + ? 'file_err_create_write_exception' + : 'file_err_create_general_exception' + ; + $this->addError('name', $this->xpdo->lexicon($messageKey)); $this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage()); return false; } @@ -812,7 +816,13 @@ public function moveObject($from, $to, $point = 'append', $to_source = 0) try { $this->filesystem->move($path, $newPath); } catch (FilesystemException | UnableToMoveFile $e) { - $this->addError('from', $this->xpdo->lexicon('file_err_rename')); + // $this->addError('from', $this->xpdo->lexicon('file_err_rename')); + $prefix = $mimeType === 'directory' ? 'file_folder_' : 'file_' ; + $messageKey = $e instanceof UnableToMoveFile + ? $prefix . 'err_move_write_exception' + : $prefix . 'err_move_write_general' + ; + $this->addError('name', $this->xpdo->lexicon($messageKey)); $this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage()); return false; } @@ -960,7 +970,11 @@ public function renameContainer($oldPath, $newName) try { $this->filesystem->move($oldPath, $newPath); } catch (FilesystemException | UnableToMoveFile $e) { - $this->addError('name', $this->xpdo->lexicon('file_folder_err_rename')); + $messageKey = $e instanceof UnableToMoveFile + ? 'file_folder_err_rename_write_exception' + : 'file_folder_err_rename_general_exception' + ; + $this->addError('name', $this->xpdo->lexicon($messageKey)); $this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage()); return false; } @@ -1071,7 +1085,11 @@ public function updateObject($path, $content) try { $this->filesystem->write($path, $content, $config); } catch (FilesystemException | UnableToWriteFile $e) { - $this->addError('name', $this->xpdo->lexicon('file_folder_err_update')); + $messageKey = $e instanceof UnableToWriteFile + ? 'file_err_update_write_exception' + : 'file_err_update_write_general' + ; + $this->addError('name', $this->xpdo->lexicon($messageKey)); $this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage()); return false; } From c7241f234008849dda597eadcd4c64615af6e6d6 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 28 Sep 2023 11:23:53 -0400 Subject: [PATCH 2/2] Code formatting fixes Note that one substantive change was made in modMediaSource ~L1849: The default flag for htmlspecialchars_decode was changed in php 8.1, thus applying explicitly what was the default flag at the time this was originally written. --- core/lexicon/en/file.inc.php | 2 + .../src/Revolution/Sources/modMediaSource.php | 67 ++++++++++--------- 2 files changed, 37 insertions(+), 32 deletions(-) diff --git a/core/lexicon/en/file.inc.php b/core/lexicon/en/file.inc.php index 5efce137955..0af3c2e2fcf 100644 --- a/core/lexicon/en/file.inc.php +++ b/core/lexicon/en/file.inc.php @@ -1,4 +1,5 @@ filesystem->mimeType($path); @@ -375,16 +375,15 @@ public function getContainerList($path) $this->addError('path', $this->xpdo->lexicon('file_folder_err_invalid')); return []; } - } try { $re = '#^(.*?/|)(' . implode('|', array_map('preg_quote', $skipFiles)) . ')/?$#'; $contents = $this->filesystem->listContents($path) - ->filter(function(StorageAttributes $attributes) use ($re) { + ->filter(function (StorageAttributes $attributes) use ($re) { return !preg_match($re, $attributes->path()); }) - ->filter(function(StorageAttributes $attributes) use ($properties) { + ->filter(function (StorageAttributes $attributes) use ($properties) { if ($attributes->isDir()) { return $this->hasPermission('directory_list'); } elseif ($attributes->isFile()) { @@ -420,9 +419,7 @@ public function getContainerList($path) $directories[$file_name]['menu'] = [ 'items' => $this->getListDirContextMenu(), ]; - - } - elseif ($object instanceof FileAttributes) { + } elseif ($object instanceof FileAttributes) { // @TODO review/refactor extension and mime_type would be better for filesystems that // may not always have an extension on it. For example would be S3 and you have an HTML file // but the name is just myPage - $this->filesystem->getMimetype($object['path']); @@ -449,9 +446,7 @@ public function getContainerList($path) foreach ($files as $file) { $ls[] = $file; } - return $ls; - } catch (FilesystemException $e) { $this->addError('path', $e->getMessage()); $this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage()); @@ -486,14 +481,12 @@ public function getObjectsInContainer($path) $files = $fileNames = []; if (!empty($path) && $path != DIRECTORY_SEPARATOR) { - try { $mimeType = $this->filesystem->mimeType($path); } catch (FilesystemException | UnableToRetrieveMetadata $e) { $this->addError('path', $e->getMessage()); $this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage()); return []; - } // Ensure this is a directory. @@ -511,7 +504,11 @@ public function getObjectsInContainer($path) return []; } foreach ($contents as $object) { - if (in_array($object['path'], $skipFiles) || in_array(trim($object['path'], DIRECTORY_SEPARATOR), $skipFiles) || (in_array($fullPath . $object['path'], $skipFiles))) { + if ( + in_array($object['path'], $skipFiles) || + in_array(trim($object['path'], DIRECTORY_SEPARATOR), $skipFiles) || + in_array($fullPath . $object['path'], $skipFiles) + ) { continue; } if ($object instanceof DirectoryAttributes && !$this->hasPermission('directory_list')) { @@ -1013,12 +1010,10 @@ public function renameObject($oldPath, $newName) if (!$this->filesystem->fileExists($oldPath)) { $this->addError('name', $this->xpdo->lexicon('file_err_invalid')); return false; - } - elseif ($this->checkFileExists() && $this->filesystem->fileExists($newPath)) { + } elseif ($this->checkFileExists() && $this->filesystem->fileExists($newPath)) { $this->addError('name', sprintf($this->xpdo->lexicon('file_err_ae'), $newName)); return false; - } - elseif (!$this->checkFileType($newName)) { + } elseif (!$this->checkFileType($newName)) { return false; } } catch (FilesystemException | UnableToRetrieveMetadata $e) { @@ -1066,8 +1061,7 @@ public function updateObject($path, $content) try { if (!$this->checkFileType($path)) { return false; - } - elseif (!$this->filesystem->fileExists($path)) { + } elseif (!$this->filesystem->fileExists($path)) { $this->addError('file', $this->xpdo->lexicon('file_err_nf') . ': ' . $path); return false; } @@ -1153,7 +1147,7 @@ public function uploadObjectsToContainer($container, array $objects = []) continue; } - if ((boolean)$this->xpdo->getOption('upload_translit')) { + if ((bool)$this->xpdo->getOption('upload_translit')) { $newName = $this->xpdo->filterPathSegment($file['name']); // If the file name is different after filtering, call OnFileManagerFileRename @@ -1194,7 +1188,7 @@ public function uploadObjectsToContainer($container, array $objects = []) $this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage()); } } - + $objects[$key] = $file; } @@ -1246,7 +1240,6 @@ public function setVisibility($path, $visibility) $this->filesystem->setVisibility($path, $visibility); return true; } - } catch (FilesystemException | UnableToSetVisibility $e) { $this->addError('path', $this->xpdo->lexicon('file_err_nf') . ' ' . $e->getMessage()); } @@ -1546,13 +1539,15 @@ public function setProperties($properties, $merge = false) if (!empty($propertyArray['options'])) { foreach ($propertyArray['options'] as $optionKey => &$option) { - if (empty($option['text']) && !empty($option['name'])) $option['text'] = $option['name']; + if (empty($option['text']) && !empty($option['name'])) { + $option['text'] = $option['name']; + } unset($option['menu'], $option['name']); } } if ($propertyArray['type'] == 'combo-boolean' && is_numeric($propertyArray['value'])) { - $propertyArray['value'] = (boolean)$propertyArray['value']; + $propertyArray['value'] = (bool)$propertyArray['value']; } $propertiesArray[$key] = $propertyArray; @@ -1585,7 +1580,7 @@ public function prepareSrcForThumb($src) return ''; } } catch (FilesystemException | UnableToRetrieveMetadata $e) { - $this->xpdo->log(modX::LOG_LEVEL_ERROR,$e->getMessage()); + $this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage()); return ''; } @@ -1648,9 +1643,9 @@ public function findPolicy($context = '') $enabled = true; $context = 'mgr'; if ($context === $this->xpdo->context->get('key')) { - $enabled = (boolean)$this->xpdo->getOption('access_media_source_enabled', null, true); + $enabled = (bool)$this->xpdo->getOption('access_media_source_enabled', null, true); } elseif ($obj = $this->xpdo->getContext($context)) { - $enabled = (boolean)$obj->getOption('access_media_source_enabled', true); + $enabled = (bool)$obj->getOption('access_media_source_enabled', true); } if ($enabled) { if (empty($this->_policies) || !isset($this->_policies[$context])) { @@ -1747,9 +1742,9 @@ public function clearCache(array $options = []) $options[xPDO::OPT_CACHE_KEY] = $this->getOption('cache_media_sources_key', $options, 'media_sources'); $options[xPDO::OPT_CACHE_HANDLER] = $this->getOption('cache_media_sources_handler', $options, $this->getOption(xPDO::OPT_CACHE_HANDLER, $options)); - $options[xPDO::OPT_CACHE_FORMAT] = (integer)$this->getOption('cache_media_sources_format', $options, $this->getOption(xPDO::OPT_CACHE_FORMAT, $options, xPDOCacheManager::CACHE_PHP)); - $options[xPDO::OPT_CACHE_ATTEMPTS] = (integer)$this->getOption('cache_media_sources_attempts', $options, $this->getOption(xPDO::OPT_CACHE_ATTEMPTS, $options, 10)); - $options[xPDO::OPT_CACHE_ATTEMPT_DELAY] = (integer)$this->getOption('cache_media_sources_attempt_delay', $options, $this->getOption(xPDO::OPT_CACHE_ATTEMPT_DELAY, $options, 1000)); + $options[xPDO::OPT_CACHE_FORMAT] = (int)$this->getOption('cache_media_sources_format', $options, $this->getOption(xPDO::OPT_CACHE_FORMAT, $options, xPDOCacheManager::CACHE_PHP)); + $options[xPDO::OPT_CACHE_ATTEMPTS] = (int)$this->getOption('cache_media_sources_attempts', $options, $this->getOption(xPDO::OPT_CACHE_ATTEMPTS, $options, 10)); + $options[xPDO::OPT_CACHE_ATTEMPT_DELAY] = (int)$this->getOption('cache_media_sources_attempt_delay', $options, $this->getOption(xPDO::OPT_CACHE_ATTEMPT_DELAY, $options, 1000)); if ($c->prepare() && $c->stmt->execute()) { while ($row = $c->stmt->fetch(PDO::FETCH_ASSOC)) { @@ -1851,7 +1846,7 @@ protected function buildFileList($path, $ext, $image_extensions, $bases, $proper $editAction = $this->getEditActionId(); $canSave = $this->checkPolicy('save'); $canRemove = $this->checkPolicy('remove'); - $id = rawurlencode(htmlspecialchars_decode($path)); + $id = rawurlencode(htmlspecialchars_decode($path, ENT_COMPAT)); $cls = []; @@ -1912,7 +1907,15 @@ protected function buildFileList($path, $ext, $image_extensions, $bases, $proper $imageWidth = $this->ctx->getOption('filemanager_image_width', 400); $imageHeight = $this->ctx->getOption('filemanager_image_height', 300); $preview_image = $this->buildManagerImagePreview($path, $ext, $imageWidth, $imageHeight, $bases, $properties); - $file_list['qtip'] = '' . $path . ''; + // Once minimum php requirement is brought up to 7.4+, heredoc closing can be indented + $file_list['qtip'] = << +QTIP; } } @@ -2361,7 +2364,7 @@ protected function getImageDimensions($path, $ext) try { $file = $this->filesystem->read($path); } catch (FilesystemException $e) { - $this->addError('file',$e->getMessage()); + $this->addError('file', $e->getMessage()); } $size = @getimagesize($this->getBasePath() . $path); if (is_array($size)) {