Skip to content

Commit

Permalink
MAGETWO-98677: Path check for images
Browse files Browse the repository at this point in the history
  • Loading branch information
@omiroshnichenko
omiroshnichenko committed Apr 19, 2019
1 parent 79de888 commit a270b5b
Show file tree
Hide file tree
Showing 7 changed files with 247 additions and 23 deletions.
26 changes: 18 additions & 8 deletions app/code/Magento/Cms/Helper/Wysiwyg/Images.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ class Images extends \Magento\Framework\App\Helper\AbstractHelper
protected $_currentUrl;

/**
* Currenty selected store ID if applicable
* Currently selected store ID if applicable
*
* @var int
*/
protected $_storeId = null;
protected $_storeId;

/**
* @var \Magento\Framework\Filesystem\Directory\Write
Expand Down Expand Up @@ -71,7 +71,7 @@ public function __construct(
$this->_storeManager = $storeManager;

$this->_directory = $filesystem->getDirectoryWrite(DirectoryList::MEDIA);
$this->_directory->create(\Magento\Cms\Model\Wysiwyg\Config::IMAGE_DIRECTORY);
$this->_directory->create($this->getStorageRoot());
}

/**
Expand All @@ -93,7 +93,17 @@ public function setStoreId($store)
*/
public function getStorageRoot()
{
return $this->_directory->getAbsolutePath(\Magento\Cms\Model\Wysiwyg\Config::IMAGE_DIRECTORY);
return $this->_directory->getAbsolutePath($this->getStorageRootSubpath());
}

/**
* Get image storage root subpath. User is unable to traverse outside of this subpath in media gallery
*
* @return string
*/
public function getStorageRootSubpath()
{
return '';
}

/**
Expand Down Expand Up @@ -141,7 +151,7 @@ public function convertIdToPath($id)
return $this->getStorageRoot();
} else {
$path = $this->getStorageRoot() . $this->idDecode($id);
if (strpos($path, DIRECTORY_SEPARATOR . '..' . DIRECTORY_SEPARATOR) !== false) {
if (preg_match('/\.\.(\\\|\/)/', $path)) {
throw new \InvalidArgumentException('Path is invalid');
}

Expand Down Expand Up @@ -208,7 +218,7 @@ public function getImageHtmlDeclaration($filename, $renderAsTag = false)
public function getCurrentPath()
{
if (!$this->_currentPath) {
$currentPath = $this->_directory->getAbsolutePath() . \Magento\Cms\Model\Wysiwyg\Config::IMAGE_DIRECTORY;
$currentPath = $this->getStorageRoot();
$path = $this->_getRequest()->getParam($this->getTreeNodeName());
if ($path) {
$path = $this->convertIdToPath($path);
Expand Down Expand Up @@ -244,7 +254,7 @@ public function getCurrentUrl()
)->getBaseUrl(
\Magento\Framework\UrlInterface::URL_TYPE_MEDIA
);
$this->_currentUrl = $mediaUrl . $this->_directory->getRelativePath($path) . '/';
$this->_currentUrl = rtrim($mediaUrl . $this->_directory->getRelativePath($path), '/') . '/';
}
return $this->_currentUrl;
}
Expand All @@ -261,7 +271,7 @@ public function idEncode($string)
}

/**
* Revert opration to idEncode
* Revert operation to idEncode
*
* @param string $string
* @return string
Expand Down
56 changes: 48 additions & 8 deletions app/code/Magento/Cms/Model/Wysiwyg/Images/Storage.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,10 +243,12 @@ protected function getConditionsForExcludeDirs()
protected function removeItemFromCollection($collection, $conditions)
{
$regExp = $conditions['reg_exp'] ? '~' . implode('|', array_keys($conditions['reg_exp'])) . '~i' : null;
$storageRootLength = strlen($this->_cmsWysiwygImages->getStorageRoot());
$storageRoot = $this->_cmsWysiwygImages->getStorageRoot();
$storageRootLength = strlen($storageRoot);

foreach ($collection as $key => $value) {
$rootChildParts = explode('/', substr($value->getFilename(), $storageRootLength));
$mediaSubPathname = substr($value->getFilename(), $storageRootLength);
$rootChildParts = explode('/', '/' . ltrim($mediaSubPathname, '/'));

if (array_key_exists($rootChildParts[1], $conditions['plain'])
|| ($regExp && preg_match($regExp, $value->getFilename()))) {
Expand Down Expand Up @@ -321,6 +323,8 @@ public function getFilesCollection($path, $type = null)
$item->setName($item->getBasename());
$item->setShortName($this->_cmsWysiwygImages->getShortFilename($item->getBasename()));
$item->setUrl($this->_cmsWysiwygImages->getCurrentUrl() . $item->getBasename());
$item->setSize(filesize($item->getFilename()));
$item->setMimeType(\mime_content_type($item->getFilename()));

if ($this->isImage($item->getBasename())) {
$thumbUrl = $this->getThumbnailUrl($item->getFilename(), true);
Expand Down Expand Up @@ -412,7 +416,7 @@ public function createDirectory($name, $path)
/**
* Recursively delete directory from storage
*
* @param string $path Target dir
* @param string $path Absolute path to target directory
* @return void
* @throws \Magento\Framework\Exception\LocalizedException
*/
Expand All @@ -421,12 +425,19 @@ public function deleteDirectory($path)
if ($this->_coreFileStorageDb->checkDbUsage()) {
$this->_directoryDatabaseFactory->create()->deleteDirectory($path);
}
if (!$this->isPathAllowed($path, $this->getConditionsForExcludeDirs())) {
throw new \Magento\Framework\Exception\LocalizedException(
__('We cannot delete directory %1.', $this->_getRelativePathToRoot($path))
);
}
try {
$this->_deleteByPath($path);
$path = $this->getThumbnailRoot() . $this->_getRelativePathToRoot($path);
$this->_deleteByPath($path);
} catch (\Magento\Framework\Exception\FileSystemException $e) {
throw new \Magento\Framework\Exception\LocalizedException(__('We cannot delete directory %1.', $path));
throw new \Magento\Framework\Exception\LocalizedException(
__('We cannot delete directory %1.', $this->_getRelativePathToRoot($path))
);
}
}

Expand Down Expand Up @@ -473,14 +484,18 @@ public function deleteFile($target)
/**
* Upload and resize new file
*
* @param string $targetPath Target directory
* @param string $targetPath Absolute path to target directory
* @param string $type Type of storage, e.g. image, media etc.
* @return array File info Array
* @throws \Magento\Framework\Exception\LocalizedException
* @throws \Exception
*/
public function uploadFile($targetPath, $type = null)
{
if (!$this->isPathAllowed($targetPath, $this->getConditionsForExcludeDirs())) {
throw new \Magento\Framework\Exception\LocalizedException(
__('We can\'t upload the file to current folder right now. Please try another folder.')
);
}
/** @var \Magento\MediaStorage\Model\File\Uploader $uploader */
$uploader = $this->_uploaderFactory->create(['fileId' => 'image']);
$allowed = $this->getAllowedExtensions($type);
Expand Down Expand Up @@ -748,7 +763,7 @@ protected function _getRelativePathToRoot($path)
}

/**
* Prepare mime types config settings
* Prepare mime types config settings.
*
* @param string|null $type Type of storage, e.g. image, media etc.
* @return array Array of allowed file extensions
Expand All @@ -761,7 +776,7 @@ private function getAllowedMimeTypes($type = null): array
}

/**
* Get list of allowed file extensions with mime type in values
* Get list of allowed file extensions with mime type in values.
*
* @param string|null $type
* @return array
Expand All @@ -775,4 +790,29 @@ private function getExtensionsList($type = null): array
}
return $allowed;
}

/**
* Check if path is not in excluded dirs.
*
* @param string $path Absolute path
* @param array $conditions Exclude conditions
* @return bool
*/
private function isPathAllowed($path, array $conditions): bool
{
$isAllowed = true;
$regExp = $conditions['reg_exp'] ? '~' . implode('|', array_keys($conditions['reg_exp'])) . '~i' : null;
$storageRoot = $this->_cmsWysiwygImages->getStorageRoot();
$storageRootLength = strlen($storageRoot);

$mediaSubPathname = substr($path, $storageRootLength);
$rootChildParts = explode('/', '/' . ltrim($mediaSubPathname, '/'));

if (array_key_exists($rootChildParts[1], $conditions['plain'])
|| ($regExp && preg_match($regExp, $path))) {
$isAllowed = false;
}

return $isAllowed;
}
}
7 changes: 4 additions & 3 deletions app/code/Magento/Cms/Test/Unit/Model/Wysiwyg/Images/StorageTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class StorageTest extends \PHPUnit\Framework\TestCase
/**
* Directory paths samples
*/
const STORAGE_ROOT_DIR = '/storage/root/dir';
const STORAGE_ROOT_DIR = '/storage/root/dir/';

const INVALID_DIRECTORY_OVER_ROOT = '/storage/some/another/dir';

Expand Down Expand Up @@ -434,10 +434,11 @@ protected function generalTestGetDirsCollection($path, $collectionArray = [], $e

public function testUploadFile()
{
$targetPath = '/target/path';
$path = 'target/path';
$targetPath = self::STORAGE_ROOT_DIR . $path;
$fileName = 'image.gif';
$realPath = $targetPath . '/' . $fileName;
$thumbnailTargetPath = self::STORAGE_ROOT_DIR . '/.thumbs';
$thumbnailTargetPath = self::STORAGE_ROOT_DIR . '/.thumbs' . $path;
$thumbnailDestination = $thumbnailTargetPath . '/' . $fileName;
$type = 'image';
$result = [
Expand Down
37 changes: 35 additions & 2 deletions app/code/Magento/Cms/etc/di.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,41 @@
</item>
</argument>
<argument name="dirs" xsi:type="array">
<item name="exclude" xsi:type="string"/>
<item name="include" xsi:type="string"/>
<item name="exclude" xsi:type="array">
<item name="captcha" xsi:type="array">
<item name="regexp" xsi:type="boolean">true</item>
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+captcha[/\\]*$</item>
</item>
<item name="catalog/product" xsi:type="array">
<item name="regexp" xsi:type="boolean">true</item>
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+catalog[/\\]+product[/\\]*$</item>
</item>
<item name="customer" xsi:type="array">
<item name="regexp" xsi:type="boolean">true</item>
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+customer[/\\]*$</item>
</item>
<item name="downloadable" xsi:type="array">
<item name="regexp" xsi:type="boolean">true</item>
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+downloadable[/\\]*$</item>
</item>
<item name="import" xsi:type="array">
<item name="regexp" xsi:type="boolean">true</item>
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+import[/\\]*$</item>
</item>
<item name="theme" xsi:type="array">
<item name="regexp" xsi:type="boolean">true</item>
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+theme[/\\]*$</item>
</item>
<item name="theme_customization" xsi:type="array">
<item name="regexp" xsi:type="boolean">true</item>
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+theme_customization[/\\]*$</item>
</item>
<item name="tmp" xsi:type="array">
<item name="regexp" xsi:type="boolean">true</item>
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+tmp[/\\]*$</item>
</item>
</item>
<item name="include" xsi:type="array"/>
</argument>
</arguments>
</type>
Expand Down
51 changes: 49 additions & 2 deletions ...ntegration/testsuite/Magento/Cms/Controller/Adminhtml/Wysiwyg/Images/DeleteFolderTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
namespace Magento\Cms\Controller\Adminhtml\Wysiwyg\Images;

use Magento\Framework\App\Filesystem\DirectoryList;
use Magento\Framework\App\Response\HttpFactory as ResponseFactory;

/**
* Test for \Magento\Cms\Controller\Adminhtml\Wysiwyg\Images\DeleteFolder class.
Expand All @@ -28,11 +29,21 @@ class DeleteFolderTest extends \PHPUnit\Framework\TestCase
*/
private $mediaDirectory;

/**
* @var string
*/
private $fullDirectoryPath;

/**
* @var \Magento\Framework\Filesystem
*/
private $filesystem;

/**
* @var ResponseFactory
*/
private $responseFactory;

/**
* @inheritdoc
*/
Expand All @@ -41,14 +52,18 @@ protected function setUp()
$objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager();
$this->filesystem = $objectManager->get(\Magento\Framework\Filesystem::class);
$this->mediaDirectory = $this->filesystem->getDirectoryWrite(DirectoryList::MEDIA);
/** @var \Magento\Cms\Helper\Wysiwyg\Images $imagesHelper */
$this->imagesHelper = $objectManager->get(\Magento\Cms\Helper\Wysiwyg\Images::class);
$this->fullDirectoryPath = $this->imagesHelper->getStorageRoot();
$this->responseFactory = $objectManager->get(ResponseFactory::class);
/** @var \Magento\Cms\Helper\Wysiwyg\Images $imagesHelper */
$this->model = $objectManager->get(\Magento\Cms\Controller\Adminhtml\Wysiwyg\Images\DeleteFolder::class);
}

/**
* Execute method with correct directory path to check that directories under WYSIWYG media directory
* can be removed.
*
* @magentoAppIsolation enabled
*/
public function testExecute()
{
Expand All @@ -74,6 +89,7 @@ public function testExecute()
* can be removed.
*
* @magentoDataFixture Magento/Cms/_files/linked_media.php
* @magentoAppIsolation enabled
*/
public function testExecuteWithLinkedMedia()
{
Expand All @@ -85,11 +101,39 @@ public function testExecuteWithLinkedMedia()
$linkedDirectory->create(
$linkedDirectory->getRelativePath($linkedDirectoryPath . DIRECTORY_SEPARATOR . $directoryName)
);
$this->model->getRequest()->setParams(['node' => $this->imagesHelper->idEncode($directoryName)]);
$this->model->getRequest()->setParams(
['node' => $this->imagesHelper->idEncode('wysiwyg' . DIRECTORY_SEPARATOR . $directoryName)]
);
$this->model->getRequest()->setMethod('POST');
$this->model->execute();
$this->assertFalse(is_dir($linkedDirectoryPath . DIRECTORY_SEPARATOR . $directoryName));
}

/**
* Execute method to check that there is no ability to remove folder which is in excluded directories list.
*
* @return void
* @magentoAppIsolation enabled
*/
public function testExecuteWithExcludedDirectoryName()
{
$directoryName = 'downloadable';
$expectedResponseMessage = 'We cannot delete directory /downloadable.';
$mediaDirectory = $this->filesystem->getDirectoryWrite(DirectoryList::MEDIA);
$mediaDirectory->create($directoryName);
$this->assertFileExists($this->fullDirectoryPath . DIRECTORY_SEPARATOR . $directoryName);

$this->model->getRequest()->setParams(['node' => $this->imagesHelper->idEncode($directoryName)]);
$this->model->getRequest()->setMethod('POST');
$jsonResponse = $this->model->execute();
$jsonResponse->renderResult($response = $this->responseFactory->create());
$data = json_decode($response->getBody(), true);

$this->assertTrue($data['error']);
$this->assertEquals($expectedResponseMessage, $data['message']);
$this->assertFileExists($this->fullDirectoryPath . $directoryName);
}

/**
* @inheritdoc
*/
Expand All @@ -102,5 +146,8 @@ public static function tearDownAfterClass()
if ($directory->isExist('wysiwyg')) {
$directory->delete('wysiwyg');
}
if ($directory->isExist('downloadable')) {
$directory->delete('downloadable');
}
}
}
Loading

0 comments on commit a270b5b

Please sign in to comment.