Skip to content

Commit f1f1d85

Browse files
omiroshnichenkoomiroshnichenko
committed
MAGETWO-98676: Path check for images
1 parent b771a6b commit f1f1d85

File tree

10 files changed

+361
-42
lines changed

10 files changed

+361
-42
lines changed

‎app/code/Magento/Cms/Helper/Wysiwyg/Images.php

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,11 @@ class Images extends \Magento\Framework\App\Helper\AbstractHelper
2727
protected $_currentUrl;
2828

2929
/**
30-
* Currenty selected store ID if applicable
30+
* Currently selected store ID if applicable
3131
*
3232
* @var int
3333
*/
34-
protected $_storeId = null;
34+
protected $_storeId;
3535

3636
/**
3737
* @var \Magento\Framework\Filesystem\Directory\Write
@@ -71,7 +71,7 @@ public function __construct(
7171
$this->_storeManager = $storeManager;
7272

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

7777
/**
@@ -93,7 +93,17 @@ public function setStoreId($store)
9393
*/
9494
public function getStorageRoot()
9595
{
96-
return $this->_directory->getAbsolutePath(\Magento\Cms\Model\Wysiwyg\Config::IMAGE_DIRECTORY);
96+
return $this->_directory->getAbsolutePath($this->getStorageRootSubpath());
97+
}
98+
99+
/**
100+
* Get image storage root subpath. User is unable to traverse outside of this subpath in media gallery
101+
*
102+
* @return string
103+
*/
104+
public function getStorageRootSubpath()
105+
{
106+
return '';
97107
}
98108

99109
/**
@@ -141,7 +151,7 @@ public function convertIdToPath($id)
141151
return $this->getStorageRoot();
142152
} else {
143153
$path = $this->getStorageRoot() . $this->idDecode($id);
144-
if (strpos($path, DIRECTORY_SEPARATOR . '..' . DIRECTORY_SEPARATOR) !== false) {
154+
if (preg_match('/\.\.(\\\|\/)/', $path)) {
145155
throw new \InvalidArgumentException('Path is invalid');
146156
}
147157

@@ -208,7 +218,7 @@ public function getImageHtmlDeclaration($filename, $renderAsTag = false)
208218
public function getCurrentPath()
209219
{
210220
if (!$this->_currentPath) {
211-
$currentPath = $this->_directory->getAbsolutePath() . \Magento\Cms\Model\Wysiwyg\Config::IMAGE_DIRECTORY;
221+
$currentPath = $this->getStorageRoot();
212222
$path = $this->_getRequest()->getParam($this->getTreeNodeName());
213223
if ($path) {
214224
$path = $this->convertIdToPath($path);
@@ -244,7 +254,7 @@ public function getCurrentUrl()
244254
)->getBaseUrl(
245255
\Magento\Framework\UrlInterface::URL_TYPE_MEDIA
246256
);
247-
$this->_currentUrl = $mediaUrl . $this->_directory->getRelativePath($path) . '/';
257+
$this->_currentUrl = rtrim($mediaUrl . $this->_directory->getRelativePath($path), '/') . '/';
248258
}
249259
return $this->_currentUrl;
250260
}

‎app/code/Magento/Cms/Model/Wysiwyg/Images/Storage.php

Lines changed: 47 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -238,10 +238,12 @@ protected function getConditionsForExcludeDirs()
238238
protected function removeItemFromCollection($collection, $conditions)
239239
{
240240
$regExp = $conditions['reg_exp'] ? '~' . implode('|', array_keys($conditions['reg_exp'])) . '~i' : null;
241-
$storageRootLength = strlen($this->_cmsWysiwygImages->getStorageRoot());
241+
$storageRoot = $this->_cmsWysiwygImages->getStorageRoot();
242+
$storageRootLength = strlen($storageRoot);
242243

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

246248
if (array_key_exists($rootChildParts[1], $conditions['plain'])
247249
|| ($regExp && preg_match($regExp, $value->getFilename()))) {
@@ -316,6 +318,8 @@ public function getFilesCollection($path, $type = null)
316318
$item->setName($item->getBasename());
317319
$item->setShortName($this->_cmsWysiwygImages->getShortFilename($item->getBasename()));
318320
$item->setUrl($this->_cmsWysiwygImages->getCurrentUrl() . $item->getBasename());
321+
$item->setSize(filesize($item->getFilename()));
322+
$item->setMimeType(\mime_content_type($item->getFilename()));
319323

320324
if ($this->isImage($item->getBasename())) {
321325
$thumbUrl = $this->getThumbnailUrl($item->getFilename(), true);
@@ -407,7 +411,7 @@ public function createDirectory($name, $path)
407411
/**
408412
* Recursively delete directory from storage
409413
*
410-
* @param string $path Target dir
414+
* @param string $path Absolute path to target directory
411415
* @return void
412416
* @throws \Magento\Framework\Exception\LocalizedException
413417
*/
@@ -416,12 +420,19 @@ public function deleteDirectory($path)
416420
if ($this->_coreFileStorageDb->checkDbUsage()) {
417421
$this->_directoryDatabaseFactory->create()->deleteDirectory($path);
418422
}
423+
if (!$this->isPathAllowed($path, $this->getConditionsForExcludeDirs())) {
424+
throw new \Magento\Framework\Exception\LocalizedException(
425+
__('We cannot delete directory %1.', $this->_getRelativePathToRoot($path))
426+
);
427+
}
419428
try {
420429
$this->_deleteByPath($path);
421430
$path = $this->getThumbnailRoot() . $this->_getRelativePathToRoot($path);
422431
$this->_deleteByPath($path);
423432
} catch (\Magento\Framework\Exception\FileSystemException $e) {
424-
throw new \Magento\Framework\Exception\LocalizedException(__('We cannot delete directory %1.', $path));
433+
throw new \Magento\Framework\Exception\LocalizedException(
434+
__('We cannot delete directory %1.', $this->_getRelativePathToRoot($path))
435+
);
425436
}
426437
}
427438

@@ -468,14 +479,18 @@ public function deleteFile($target)
468479
/**
469480
* Upload and resize new file.
470481
*
471-
* @param string $targetPath Target directory
482+
* @param string $targetPath Absolute path to target directory
472483
* @param string $type Type of storage, e.g. image, media etc.
473484
* @return array File info Array
474485
* @throws \Magento\Framework\Exception\LocalizedException
475-
* @throws \Exception
476486
*/
477487
public function uploadFile($targetPath, $type = null)
478488
{
489+
if (!$this->isPathAllowed($targetPath, $this->getConditionsForExcludeDirs())) {
490+
throw new \Magento\Framework\Exception\LocalizedException(
491+
__('We can\'t upload the file to current folder right now. Please try another folder.')
492+
);
493+
}
479494
/** @var \Magento\MediaStorage\Model\File\Uploader $uploader */
480495
$uploader = $this->_uploaderFactory->create(['fileId' => 'image']);
481496
$allowed = $this->getAllowedExtensions($type);
@@ -725,7 +740,7 @@ protected function _validatePath($path)
725740
*/
726741
protected function _sanitizePath($path)
727742
{
728-
return rtrim(preg_replace('~[/\\\]+~', '/', $this->_directory->getDriver()->getRealPath($path)), '/');
743+
return rtrim(preg_replace('~[/\\\]+~', '/', $this->_directory->getDriver()->getRealPathSafety($path)), '/');
729744
}
730745

731746
/**
@@ -771,4 +786,29 @@ private function getExtensionsList($type = null)
771786

772787
return $allowed;
773788
}
789+
790+
/**
791+
* Check if path is not in excluded dirs.
792+
*
793+
* @param string $path Absolute path
794+
* @param array $conditions Exclude conditions
795+
* @return bool
796+
*/
797+
private function isPathAllowed($path, $conditions)
798+
{
799+
$isAllowed = true;
800+
$regExp = $conditions['reg_exp'] ? '~' . implode('|', array_keys($conditions['reg_exp'])) . '~i' : null;
801+
$storageRoot = $this->_cmsWysiwygImages->getStorageRoot();
802+
$storageRootLength = strlen($storageRoot);
803+
804+
$mediaSubPathname = substr($path, $storageRootLength);
805+
$rootChildParts = explode('/', '/' . ltrim($mediaSubPathname, '/'));
806+
807+
if (array_key_exists($rootChildParts[1], $conditions['plain'])
808+
|| ($regExp && preg_match($regExp, $path))) {
809+
$isAllowed = false;
810+
}
811+
812+
return $isAllowed;
813+
}
774814
}

‎app/code/Magento/Cms/Test/Unit/Helper/Wysiwyg/ImagesTest.php

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ class ImagesTest extends \PHPUnit_Framework_TestCase
7474

7575
protected function setUp()
7676
{
77-
$this->path = 'PATH/';
77+
$this->path = 'PATH';
7878
$this->objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this);
7979

8080
$this->eventManagerMock = $this->getMock(\Magento\Framework\Event\ManagerInterface::class, [], [], '', false);
@@ -108,7 +108,8 @@ protected function setUp()
108108
->willReturnMap(
109109
[
110110
[WysiwygConfig::IMAGE_DIRECTORY, null, $this->getAbsolutePath(WysiwygConfig::IMAGE_DIRECTORY)],
111-
[null, null, $this->getAbsolutePath(null)]
111+
[null, null, $this->getAbsolutePath(null)],
112+
['', null, $this->getAbsolutePath('')],
112113
]
113114
);
114115

@@ -173,7 +174,7 @@ public function testSetStoreId()
173174
public function testGetStorageRoot()
174175
{
175176
$this->assertEquals(
176-
$this->getAbsolutePath(WysiwygConfig::IMAGE_DIRECTORY),
177+
$this->getAbsolutePath(''),
177178
$this->imagesHelper->getStorageRoot()
178179
);
179180
}
@@ -197,7 +198,7 @@ public function testGetTreeNodeName()
197198
public function testConvertPathToId()
198199
{
199200
$pathOne = '/test_path';
200-
$pathTwo = $this->getAbsolutePath(WysiwygConfig::IMAGE_DIRECTORY) . '/test_path';
201+
$pathTwo = $this->getAbsolutePath('') . '/test_path';
201202
$this->assertEquals(
202203
$this->imagesHelper->convertPathToId($pathOne),
203204
$this->imagesHelper->convertPathToId($pathTwo)
@@ -334,26 +335,30 @@ public function providerIsUsingStaticUrlsAllowed()
334335
*/
335336
public function testGetCurrentPath($pathId, $expectedPath, $isExist)
336337
{
337-
$this->requestMock->expects($this->once())
338+
$this->requestMock->expects($this->any())
338339
->method('getParam')
339-
->willReturn($pathId);
340+
->willReturnMap(
341+
[
342+
['node', null, $pathId],
343+
]
344+
);
340345

341346
$this->directoryWriteMock->expects($this->any())
342347
->method('isDirectory')
343348
->willReturnMap(
344349
[
345-
['/../wysiwyg/test_path', true],
346-
['/../wysiwyg/my.jpg', false],
347-
['/../wysiwyg', true]
350+
['/../test_path', true],
351+
['/../my.jpg', false],
352+
['.', true],
348353
]
349354
);
350355
$this->directoryWriteMock->expects($this->any())
351356
->method('getRelativePath')
352357
->willReturnMap(
353358
[
354-
['PATH/wysiwyg/test_path', '/../wysiwyg/test_path'],
355-
['PATH/wysiwyg/my.jpg', '/../wysiwyg/my.jpg'],
356-
['PATH/wysiwyg', '/../wysiwyg'],
359+
['PATH/test_path', '/../test_path'],
360+
['PATH/my.jpg', '/../my.jpg'],
361+
['PATH', '.'],
357362
]
358363
);
359364
$this->directoryWriteMock->expects($this->once())
@@ -370,7 +375,7 @@ public function testGetCurrentPathThrowException()
370375
{
371376
$this->setExpectedException(
372377
\Magento\Framework\Exception\LocalizedException::class,
373-
'The directory PATH/wysiwyg is not writable by server.'
378+
'The directory PATH is not writable by server.'
374379
);
375380

376381
$this->directoryWriteMock->expects($this->once())
@@ -393,12 +398,12 @@ public function testGetCurrentPathThrowException()
393398
public function providerGetCurrentPath()
394399
{
395400
return [
396-
['L3Rlc3RfcGF0aA--', 'PATH/wysiwyg/test_path', true],
397-
['L215LmpwZw--', 'PATH/wysiwyg', true],
398-
[null, 'PATH/wysiwyg', true],
399-
['L3Rlc3RfcGF0aA--', 'PATH/wysiwyg/test_path', false],
400-
['L215LmpwZw--', 'PATH/wysiwyg', false],
401-
[null, 'PATH/wysiwyg', false]
401+
['L3Rlc3RfcGF0aA--', 'PATH/test_path', true],
402+
['L215LmpwZw--', 'PATH', true],
403+
[null, 'PATH', true],
404+
['L3Rlc3RfcGF0aA--', 'PATH/test_path', false],
405+
['L215LmpwZw--', 'PATH', false],
406+
[null, 'PATH', false],
402407
];
403408
}
404409

‎app/code/Magento/Cms/Test/Unit/Model/Wysiwyg/Images/StorageTest.php

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,9 @@ protected function setUp()
133133
true,
134134
['getRealPath']
135135
);
136-
$this->driverMock->expects($this->any())->method('getRealPath')->willReturnArgument(0);
136+
$this->driverMock = $this->getMockBuilder(\Magento\Framework\Filesystem\DriverInterface::class)
137+
->setMethods(['getRealPathSafety'])
138+
->getMockForAbstractClass();
137139

138140
$this->directoryMock = $this->getMock(
139141
\Magento\Framework\Filesystem\Directory\Write::class,
@@ -283,6 +285,7 @@ public function testDeleteDirectoryOverRoot()
283285
\Magento\Framework\Exception\LocalizedException::class,
284286
sprintf('Directory %s is not under storage root path.', self::INVALID_DIRECTORY_OVER_ROOT)
285287
);
288+
$this->driverMock->expects($this->atLeastOnce())->method('getRealPathSafety')->will($this->returnArgument(0));
286289
$this->imagesStorage->deleteDirectory(self::INVALID_DIRECTORY_OVER_ROOT);
287290
}
288291

@@ -295,6 +298,7 @@ public function testDeleteRootDirectory()
295298
\Magento\Framework\Exception\LocalizedException::class,
296299
sprintf('We can\'t delete root directory %s right now.', self::STORAGE_ROOT_DIR)
297300
);
301+
$this->driverMock->expects($this->atLeastOnce())->method('getRealPathSafety')->will($this->returnArgument(0));
298302
$this->imagesStorage->deleteDirectory(self::STORAGE_ROOT_DIR);
299303
}
300304

@@ -464,10 +468,11 @@ protected function generalTestGetDirsCollection($path, array $collectionArray =
464468
*/
465469
public function testUploadFile()
466470
{
467-
$targetPath = '/target/path';
471+
$path = 'target/path';
472+
$targetPath = self::STORAGE_ROOT_DIR . $path;
468473
$fileName = 'image.gif';
469474
$realPath = $targetPath . '/' . $fileName;
470-
$thumbnailTargetPath = self::STORAGE_ROOT_DIR . '/.thumbs';
475+
$thumbnailTargetPath = self::STORAGE_ROOT_DIR . '/.thumbs' . $path;
471476
$thumbnailDestination = $thumbnailTargetPath . '/' . $fileName;
472477
$type = 'image';
473478
$result = [

‎app/code/Magento/Cms/etc/di.xml

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,41 @@
5050
</item>
5151
</argument>
5252
<argument name="dirs" xsi:type="array">
53-
<item name="exclude" xsi:type="string"/>
54-
<item name="include" xsi:type="string"/>
53+
<item name="exclude" xsi:type="array">
54+
<item name="captcha" xsi:type="array">
55+
<item name="regexp" xsi:type="boolean">true</item>
56+
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+captcha[/\\]*$</item>
57+
</item>
58+
<item name="catalog/product" xsi:type="array">
59+
<item name="regexp" xsi:type="boolean">true</item>
60+
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+catalog[/\\]+product[/\\]*$</item>
61+
</item>
62+
<item name="customer" xsi:type="array">
63+
<item name="regexp" xsi:type="boolean">true</item>
64+
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+customer[/\\]*$</item>
65+
</item>
66+
<item name="downloadable" xsi:type="array">
67+
<item name="regexp" xsi:type="boolean">true</item>
68+
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+downloadable[/\\]*$</item>
69+
</item>
70+
<item name="import" xsi:type="array">
71+
<item name="regexp" xsi:type="boolean">true</item>
72+
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+import[/\\]*$</item>
73+
</item>
74+
<item name="theme" xsi:type="array">
75+
<item name="regexp" xsi:type="boolean">true</item>
76+
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+theme[/\\]*$</item>
77+
</item>
78+
<item name="theme_customization" xsi:type="array">
79+
<item name="regexp" xsi:type="boolean">true</item>
80+
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+theme_customization[/\\]*$</item>
81+
</item>
82+
<item name="tmp" xsi:type="array">
83+
<item name="regexp" xsi:type="boolean">true</item>
84+
<item name="name" xsi:type="string">pub[/\\]+media[/\\]+tmp[/\\]*$</item>
85+
</item>
86+
</item>
87+
<item name="include" xsi:type="array"/>
5588
</argument>
5689
</arguments>
5790
</type>

0 commit comments

Comments
 (0)