Skip to content

Commit

Permalink
Improve file system error feedback (#16479)
Browse files Browse the repository at this point in the history 
### What does it do?
Creates/updates error messaging to be more accurate and descriptive when
a file/folder can not be moved or otherwise changed. Also, in second
commit, aligns the code formatting to our current rule set.

### Why is it needed?
Current error feedback is for the most part unactionable because it's
too ambiguous.

### How to test

1. Create at least one file and one folder that have read-only
permission.
2. Verify the new messaging appears and its language is clear by
attempting to: a) edit (file), rename*, or move a read-only
file/directory; or b) create new files/folders in your new read-only
directory.

* Note that, interestingly, in many cases renaming or moving a read-only
file is possible when you have write permissions on the parent/target
directory. This is just how unix works and MODX currently doesn't
attempt to apply the fine-grained measures that'd be necessary to lock
down files to that degree.

### Related issue(s)/PR(s)
Resolves #16074
  • Loading branch information
@smg6511
smg6511 committed Mar 25, 2024
1 parent 70cca64 commit c5a4116
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 38 deletions.
18 changes: 16 additions & 2 deletions core/lexicon/en/file.inc.php
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
<?php

/**
* File English lexicon topic
*
* @language en
* @package modx
* @subpackage lexicon
*/

$_lang['directory'] = 'Directory';
$_lang['file_create'] = 'Create File';
$_lang['file_download'] = 'Download File';
Expand All @@ -14,17 +16,22 @@
$_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: ';
$_lang['file_err_rename'] = 'MODX failed to rename the file. Please make sure your permissions are set correctly.';
$_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';
Expand All @@ -40,9 +47,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.';
Expand Down Expand Up @@ -115,3 +125,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'];
93 changes: 57 additions & 36 deletions core/src/Revolution/Sources/modMediaSource.php
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?php

namespace MODX\Revolution\Sources;

use Exception;
Expand Down Expand Up @@ -361,7 +362,6 @@ public function getContainerList($path)
$directories = $dirNames = $files = $fileNames = [];

if (!empty($path)) {

// Ensure the provided path can be read.
try {
$mimeType = $this->filesystem->mimeType($path);
Expand All @@ -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()) {
Expand Down Expand Up @@ -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']);
Expand All @@ -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());
Expand Down Expand Up @@ -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.
Expand All @@ -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')) {
Expand Down Expand Up @@ -726,7 +723,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;
}
Expand Down Expand Up @@ -812,7 +813,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;
}
Expand Down Expand Up @@ -960,7 +967,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;
}
Expand Down Expand Up @@ -999,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) {
Expand Down Expand Up @@ -1052,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;
}
Expand All @@ -1071,7 +1079,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;
}
Expand Down Expand Up @@ -1135,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
Expand Down Expand Up @@ -1176,7 +1188,7 @@ public function uploadObjectsToContainer($container, array $objects = [])
$this->xpdo->log(modX::LOG_LEVEL_ERROR, $e->getMessage());
}
}

$objects[$key] = $file;
}

Expand Down Expand Up @@ -1228,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());
}
Expand Down Expand Up @@ -1528,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;
Expand Down Expand Up @@ -1567,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 '';
}

Expand Down Expand Up @@ -1630,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])) {
Expand Down Expand Up @@ -1729,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)) {
Expand Down Expand Up @@ -1833,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 = [];

Expand Down Expand Up @@ -1894,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'] = '<img src="' . $preview_image['src'] . '" width="' . $preview_image['width'] . '" height="' . $preview_image['height'] . '" alt="' . $path . '" />';
// 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']}"
alt="{$path}"
>
QTIP;
}
}

Expand Down Expand Up @@ -2343,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)) {
Expand Down

0 comments on commit c5a4116

Please sign in to comment.