Skip to content

Commit 4b3b814

Browse files
author
OlgaVasyltsun
committed
MC-18100: Changes in Downloadable product upload controller
1 parent 6023fc9 commit 4b3b814

File tree

4 files changed

+248
-12
lines changed

4 files changed

+248
-12
lines changed

‎app/code/Magento/Downloadable/Controller/Adminhtml/Downloadable/File/Upload.php

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
use Magento\MediaStorage\Helper\File\Storage\Database;
1818
use Magento\Framework\App\RequestInterface;
1919
use Magento\Framework\App\State;
20+
use Magento\Framework\Exception\FileSystemException;
21+
use Magento\Framework\Exception\LocalizedException;
2022

2123
/**
2224
* Upload controller
@@ -109,23 +111,27 @@ public function dispatch(RequestInterface $request)
109111
*/
110112
public function execute()
111113
{
112-
$type = $this->getRequest()->getParam('type');
113-
$tmpPath = '';
114-
if ($type == 'samples') {
115-
$tmpPath = $this->_sample->getBaseTmpPath();
116-
} elseif ($type == 'links') {
117-
$tmpPath = $this->_link->getBaseTmpPath();
118-
} elseif ($type == 'link_samples') {
119-
$tmpPath = $this->_link->getBaseSampleTmpPath();
120-
}
121-
122114
try {
115+
$type = $this->getRequest()->getParam('type');
116+
$tmpPath = '';
117+
if ($type === 'samples') {
118+
$tmpPath = $this->_sample->getBaseTmpPath();
119+
} elseif ($type === 'links') {
120+
$tmpPath = $this->_link->getBaseTmpPath();
121+
} elseif ($type === 'link_samples') {
122+
$tmpPath = $this->_link->getBaseSampleTmpPath();
123+
} else {
124+
throw new LocalizedException(__('Upload type can not be determined.'));
125+
}
126+
123127
$uploader = $this->uploaderFactory->create(['fileId' => $type]);
124128

125129
$result = $this->_fileHelper->uploadFromTmp($tmpPath, $uploader);
126130

127131
if (!$result) {
128-
throw new \Exception('File can not be moved from temporary folder to the destination folder.');
132+
throw new FileSystemException(
133+
__('File can not be moved from temporary folder to the destination folder.')
134+
);
129135
}
130136

131137
unset($result['tmp_name'], $result['path']);
@@ -137,6 +143,7 @@ public function execute()
137143
} catch (\Exception $e) {
138144
$result = ['error' => $e->getMessage(), 'errorcode' => $e->getCode()];
139145
}
146+
140147
return $this->resultFactory->create(ResultFactory::TYPE_JSON)->setData($result);
141148
}
142149
}

‎dev/tests/integration/testsuite/Magento/Downloadable/Controller/Adminhtml/Downloadable/File/UploadTest.php

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
7+
namespace Magento\Downloadable\Controller\Adminhtml\Downloadable\File;
8+
9+
use Magento\Framework\Serialize\Serializer\Json;
10+
11+
/**
12+
* Test for Magento\Downloadable\Controller\Adminhtml\Downloadable\File\Upload
13+
*
14+
* @magentoAppArea adminhtml
15+
*/
16+
class UploadTest extends \Magento\TestFramework\TestCase\AbstractBackendController
17+
{
18+
/**
19+
* @var Json
20+
*/
21+
private $jsonSerializer;
22+
23+
/**
24+
* @inheritdoc
25+
*/
26+
protected function setUp()
27+
{
28+
parent::setUp();
29+
30+
$this->jsonSerializer = $this->_objectManager->get(Json::class);
31+
}
32+
33+
/**
34+
* @dataProvider uploadWrongUploadTypeDataProvider
35+
* @return void
36+
*/
37+
public function testUploadWrongUploadType($postData): void
38+
{
39+
$this->getRequest()->setPostValue($postData);
40+
$this->getRequest()->setMethod('POST');
41+
42+
$this->dispatch('backend/admin/downloadable_file/upload');
43+
44+
$body = $this->getResponse()->getBody();
45+
$result = $this->jsonSerializer->unserialize($body);
46+
$this->assertEquals('Upload type can not be determined.', $result['error']);
47+
$this->assertEquals(0, $result['errorcode']);
48+
}
49+
50+
public function uploadWrongUploadTypeDataProvider(): array
51+
{
52+
return [
53+
[
54+
['type' => 'test'],
55+
],
56+
[
57+
[
58+
'type' => [
59+
'type1' => 'test',
60+
],
61+
],
62+
],
63+
];
64+
}
65+
}

‎dev/tests/integration/testsuite/Magento/Framework/File/UploaderTest.php

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
declare(strict_types=1);
7+
8+
namespace Magento\Framework\File;
9+
10+
use Magento\Framework\App\Filesystem\DirectoryList;
11+
12+
/**
13+
* Test for \Magento\Framework\File\Uploader
14+
*/
15+
class UploaderTest extends \PHPUnit\Framework\TestCase
16+
{
17+
/**
18+
* @var \Magento\MediaStorage\Model\File\UploaderFactory
19+
*/
20+
private $uploaderFactory;
21+
22+
/**
23+
* @var \Magento\Framework\Filesystem
24+
*/
25+
private $filesystem;
26+
27+
/**
28+
* @inheritdoc
29+
*/
30+
protected function setUp()
31+
{
32+
$this->uploaderFactory = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()
33+
->get(\Magento\MediaStorage\Model\File\UploaderFactory::class);
34+
35+
$this->filesystem = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()
36+
->get(\Magento\Framework\Filesystem::class);
37+
}
38+
39+
/**
40+
* @return void
41+
*/
42+
public function testUploadFileFromAllowedFolder(): void
43+
{
44+
$mediaDirectory = $this->filesystem->getDirectoryWrite(DirectoryList::MEDIA);
45+
$tmpDirectory = $this->filesystem->getDirectoryWrite(DirectoryList::SYS_TMP);
46+
47+
$fileName = 'text.txt';
48+
$tmpDir = 'tmp';
49+
$filePath = $tmpDirectory->getAbsolutePath($fileName);
50+
51+
$tmpDirectory->writeFile($fileName, 'just a text');
52+
53+
$type = [
54+
'tmp_name' => $filePath,
55+
'name' => $fileName,
56+
];
57+
58+
$uploader = $this->uploaderFactory->create(['fileId' => $type]);
59+
$uploader->save($mediaDirectory->getAbsolutePath($tmpDir));
60+
61+
$this->assertTrue($mediaDirectory->isFile($tmpDir . DIRECTORY_SEPARATOR . $fileName));
62+
}
63+
64+
/**
65+
* @expectedException \InvalidArgumentException
66+
* @expectedExceptionMessage Invalid parameter given. A valid $fileId[tmp_name] is expected.
67+
*
68+
* @return void
69+
*/
70+
public function testUploadFileFromNotAllowedFolder(): void
71+
{
72+
$fileName = 'text.txt';
73+
$tmpDir = 'tmp';
74+
$tmpDirectory = $this->filesystem->getDirectoryWrite(DirectoryList::LOG);
75+
$filePath = $tmpDirectory->getAbsolutePath() . $tmpDir . DIRECTORY_SEPARATOR . $fileName;
76+
77+
$tmpDirectory->writeFile($tmpDir . DIRECTORY_SEPARATOR . $fileName, 'just a text');
78+
79+
$type = [
80+
'tmp_name' => $filePath,
81+
'name' => $fileName,
82+
];
83+
84+
$this->uploaderFactory->create(['fileId' => $type]);
85+
}
86+
87+
/**
88+
* @inheritdoc
89+
*/
90+
protected function tearDown()
91+
{
92+
parent::tearDown();
93+
94+
$tmpDir = 'tmp';
95+
$mediaDirectory = $this->filesystem->getDirectoryWrite(DirectoryList::MEDIA);
96+
$mediaDirectory->delete($tmpDir);
97+
98+
$logDirectory = $this->filesystem->getDirectoryWrite(DirectoryList::LOG);
99+
$logDirectory->delete($tmpDir);
100+
}
101+
}

‎lib/internal/Magento/Framework/File/Uploader.php

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66
namespace Magento\Framework\File;
77

8+
use Magento\Framework\App\Filesystem\DirectoryList;
89
use Magento\Framework\Exception\FileSystemException;
910
use Magento\Framework\Validation\ValidationException;
1011

@@ -14,6 +15,8 @@
1415
* ATTENTION! This class must be used like abstract class and must added
1516
* validation by protected file extension list to extended class
1617
*
18+
* @SuppressWarnings(PHPMD.TooManyFields)
19+
*
1720
* @api
1821
*/
1922
class Uploader
@@ -160,17 +163,27 @@ class Uploader
160163
*/
161164
protected $_result;
162165

166+
/**
167+
* @var DirectoryList
168+
*/
169+
private $directoryList;
170+
163171
/**
164172
* Init upload
165173
*
166174
* @param string|array $fileId
167175
* @param \Magento\Framework\File\Mime|null $fileMime
176+
* @param DirectoryList|null $directoryList
168177
* @throws \DomainException
169178
*/
170179
public function __construct(
171180
$fileId,
172-
Mime $fileMime = null
181+
Mime $fileMime = null,
182+
DirectoryList $directoryList = null
173183
) {
184+
$this->directoryList= $directoryList ?: \Magento\Framework\App\ObjectManager::getInstance()
185+
->get(DirectoryList::class);
186+
174187
$this->_setUploadFileId($fileId);
175188
if (!file_exists($this->_file['tmp_name'])) {
176189
$code = empty($this->_file['tmp_name']) ? self::TMP_NAME_EMPTY : 0;
@@ -552,6 +565,7 @@ private function _getMimeType()
552565
private function _setUploadFileId($fileId)
553566
{
554567
if (is_array($fileId)) {
568+
$this->validateFileId($fileId);
555569
$this->_uploadType = self::MULTIPLE_STYLE;
556570
$this->_file = $fileId;
557571
} else {
@@ -585,6 +599,55 @@ private function _setUploadFileId($fileId)
585599
}
586600
}
587601

602+
/**
603+
* Validates explicitly given uploaded file data.
604+
*
605+
* @param array $fileId
606+
* @return void
607+
* @throws \InvalidArgumentException
608+
*/
609+
private function validateFileId(array $fileId): void
610+
{
611+
$isValid = false;
612+
if (isset($fileId['tmp_name'])) {
613+
$tmpName = trim($fileId['tmp_name']);
614+
615+
if (preg_match('/\.\.(\\\|\/)/', $tmpName) !== 1) {
616+
$allowedFolders = [
617+
sys_get_temp_dir(),
618+
$this->directoryList->getPath(DirectoryList::MEDIA),
619+
$this->directoryList->getPath(DirectoryList::VAR_DIR),
620+
$this->directoryList->getPath(DirectoryList::TMP),
621+
$this->directoryList->getPath(DirectoryList::UPLOAD),
622+
];
623+
624+
$disallowedFolders = [
625+
$this->directoryList->getPath(DirectoryList::LOG),
626+
];
627+
628+
foreach ($allowedFolders as $allowedFolder) {
629+
if (stripos($tmpName, $allowedFolder) === 0) {
630+
$isValid = true;
631+
break;
632+
}
633+
}
634+
635+
foreach ($disallowedFolders as $disallowedFolder) {
636+
if (stripos($tmpName, $disallowedFolder) === 0) {
637+
$isValid = false;
638+
break;
639+
}
640+
}
641+
}
642+
}
643+
644+
if (!$isValid) {
645+
throw new \InvalidArgumentException(
646+
__('Invalid parameter given. A valid $fileId[tmp_name] is expected.')
647+
);
648+
}
649+
}
650+
588651
/**
589652
* Create destination folder
590653
*

0 commit comments

Comments
 (0)