Skip to content

Commit 7172185

Browse files
ameysarameysar
committed
MAGETWO-95638: Upload Extension Validation
1 parent 991d785 commit 7172185

File tree

2 files changed

+85
-24
lines changed

2 files changed

+85
-24
lines changed

‎app/code/Magento/CatalogImportExport/Model/Import/Uploader.php

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ class Uploader extends \Magento\MediaStorage\Model\File\Uploader
108108
* @param \Magento\MediaStorage\Model\File\Validator\NotProtectedExtension $validator
109109
* @param \Magento\Framework\Filesystem $filesystem
110110
* @param \Magento\Framework\Filesystem\File\ReadFactory $readFactory
111-
* @param null $filePath
111+
* @param string|null $filePath
112112
* @param \Magento\Framework\App\Filesystem\DirectoryResolver|null $directoryResolver
113113
* @throws \Magento\Framework\Exception\LocalizedException
114114
*/
@@ -157,29 +157,48 @@ public function init()
157157
* @param string $fileName
158158
* @param bool $renameFileOff
159159
* @return array
160+
* @throws \Magento\Framework\Exception\LocalizedException
160161
*/
161162
public function move($fileName, $renameFileOff = false)
162163
{
163164
if ($renameFileOff) {
164165
$this->setAllowRenameFiles(false);
165166
}
167+
168+
if ($this->getTmpDir()) {
169+
$filePath = $this->getTmpDir() . '/';
170+
} else {
171+
$filePath = '';
172+
}
173+
166174
if (preg_match('/\bhttps?:\/\//i', $fileName, $matches)) {
167175
$url = str_replace($matches[0], '', $fileName);
176+
$driver = $matches[0] === $this->httpScheme ? DriverPool::HTTP : DriverPool::HTTPS;
177+
$read = $this->_readFactory->create($url, $driver);
178+
179+
//only use filename (for URI with query parameters)
180+
$parsedUrlPath = parse_url($url, PHP_URL_PATH);
181+
if ($parsedUrlPath) {
182+
$urlPathValues = explode('/', $parsedUrlPath);
183+
if (!empty($urlPathValues)) {
184+
$fileName = end($urlPathValues);
185+
}
186+
}
168187

169-
if ($matches[0] === $this->httpScheme) {
170-
$read = $this->_readFactory->create($url, DriverPool::HTTP);
171-
} else {
172-
$read = $this->_readFactory->create($url, DriverPool::HTTPS);
188+
$fileExtension = pathinfo($fileName, PATHINFO_EXTENSION);
189+
if ($fileExtension && !$this->checkAllowedExtension($fileExtension)) {
190+
throw new \Magento\Framework\Exception\LocalizedException(__('Disallowed file type.'));
173191
}
174192

175193
$fileName = preg_replace('/[^a-z0-9\._-]+/i', '', $fileName);
194+
$filePath = $this->_directory->getRelativePath($filePath . $fileName);
176195
$this->_directory->writeFile(
177-
$this->_directory->getRelativePath($this->getTmpDir() . '/' . $fileName),
196+
$filePath,
178197
$read->readAll()
179198
);
180199
}
181200

182-
$filePath = $this->_directory->getRelativePath($this->getTmpDir() . '/' . $fileName);
201+
$filePath = $this->_directory->getRelativePath($filePath . $fileName);
183202
$this->_setUploadFile($filePath);
184203
$destDir = $this->_directory->getAbsolutePath($this->getDestDir());
185204
$result = $this->save($destDir);

‎app/code/Magento/CatalogImportExport/Test/Unit/Model/Import/UploaderTest.php

Lines changed: 59 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@ class UploaderTest extends \PHPUnit\Framework\TestCase
5656
*/
5757
protected $uploader;
5858

59+
/**
60+
* @inheritdoc
61+
*/
5962
protected function setUp()
6063
{
6164
$this->coreFileStorageDb = $this->getMockBuilder(\Magento\MediaStorage\Helper\File\Storage\Database::class)
@@ -79,7 +82,7 @@ protected function setUp()
7982
->setMethods(['create'])
8083
->getMock();
8184

82-
$this->directoryMock = $this->getMockBuilder(\Magento\Framework\Filesystem\Directory\Writer::class)
85+
$this->directoryMock = $this->getMockBuilder(\Magento\Framework\Filesystem\Directory\Write::class)
8386
->setMethods(['writeFile', 'getRelativePath', 'isWritable', 'isReadable', 'getAbsolutePath'])
8487
->disableOriginalConstructor()
8588
->getMock();
@@ -109,41 +112,53 @@ protected function setUp()
109112
null,
110113
$this->directoryResolver
111114
])
112-
->setMethods(['_setUploadFile', 'save', 'getTmpDir'])
115+
->setMethods(['_setUploadFile', 'save', 'getTmpDir', 'checkAllowedExtension'])
113116
->getMock();
114117
}
115118

116119
/**
117120
* @dataProvider moveFileUrlDataProvider
121+
* @param string $fileUrl
122+
* @param string $expectedHost
123+
* @param string $expectedFileName
124+
* @param int $checkAllowedExtension
125+
* @return void
118126
*/
119-
public function testMoveFileUrl($fileUrl, $expectedHost, $expectedFileName)
120-
{
127+
public function testMoveFileUrl(
128+
string $fileUrl,
129+
string $expectedHost,
130+
string $expectedFileName,
131+
int $checkAllowedExtension
132+
) {
121133
$destDir = 'var/dest/dir';
122-
$expectedRelativeFilePath = $this->uploader->getTmpDir() . '/' . $expectedFileName;
134+
$expectedRelativeFilePath = $expectedFileName;
123135
$this->directoryMock->expects($this->once())->method('isWritable')->with($destDir)->willReturn(true);
124136
$this->directoryMock->expects($this->any())->method('getRelativePath')->with($expectedRelativeFilePath);
125137
$this->directoryMock->expects($this->once())->method('getAbsolutePath')->with($destDir)
126138
->willReturn($destDir . '/' . $expectedFileName);
127139
// Check writeFile() method invoking.
128-
$this->directoryMock->expects($this->any())->method('writeFile')->will($this->returnValue($expectedFileName));
140+
$this->directoryMock->expects($this->any())->method('writeFile')->willReturn($expectedFileName);
129141

130142
// Create adjusted reader which does not validate path.
131143
$readMock = $this->getMockBuilder(\Magento\Framework\Filesystem\File\Read::class)
132144
->disableOriginalConstructor()
133145
->setMethods(['readAll'])
134146
->getMock();
135147
// Check readAll() method invoking.
136-
$readMock->expects($this->once())->method('readAll')->will($this->returnValue(null));
148+
$readMock->expects($this->once())->method('readAll')->willReturn(null);
137149

138150
// Check create() method invoking with expected argument.
139151
$this->readFactory->expects($this->once())
140152
->method('create')
141153
->will($this->returnValue($readMock))->with($expectedHost);
142154
//Check invoking of getTmpDir(), _setUploadFile(), save() methods.
143-
$this->uploader->expects($this->any())->method('getTmpDir')->will($this->returnValue(''));
144-
$this->uploader->expects($this->once())->method('_setUploadFile')->will($this->returnSelf());
155+
$this->uploader->expects($this->any())->method('getTmpDir')->willReturn('');
156+
$this->uploader->expects($this->once())->method('_setUploadFile')->willReturnSelf();
145157
$this->uploader->expects($this->once())->method('save')->with($destDir . '/' . $expectedFileName)
146158
->willReturn(['name' => $expectedFileName, 'path' => 'absPath']);
159+
$this->uploader->expects($this->exactly($checkAllowedExtension))
160+
->method('checkAllowedExtension')
161+
->willReturn(true);
147162

148163
$this->uploader->setDestDir($destDir);
149164
$result = $this->uploader->move($fileUrl);
@@ -155,14 +170,14 @@ public function testMoveFileName()
155170
{
156171
$destDir = 'var/dest/dir';
157172
$fileName = 'test_uploader_file';
158-
$expectedRelativeFilePath = $this->uploader->getTmpDir() . '/' . $fileName;
173+
$expectedRelativeFilePath = $fileName;
159174
$this->directoryMock->expects($this->once())->method('isWritable')->with($destDir)->willReturn(true);
160175
$this->directoryMock->expects($this->any())->method('getRelativePath')->with($expectedRelativeFilePath);
161176
$this->directoryMock->expects($this->once())->method('getAbsolutePath')->with($destDir)
162177
->willReturn($destDir . '/' . $fileName);
163178
//Check invoking of getTmpDir(), _setUploadFile(), save() methods.
164-
$this->uploader->expects($this->once())->method('getTmpDir')->will($this->returnValue(''));
165-
$this->uploader->expects($this->once())->method('_setUploadFile')->will($this->returnSelf());
179+
$this->uploader->expects($this->once())->method('getTmpDir')->willReturn('');
180+
$this->uploader->expects($this->once())->method('_setUploadFile')->willReturnSelf();
166181
$this->uploader->expects($this->once())->method('save')->with($destDir . '/' . $fileName)
167182
->willReturn(['name' => $fileName]);
168183

@@ -239,12 +254,38 @@ public function moveFileUrlDataProvider()
239254
[
240255
'$fileUrl' => 'http://test_uploader_file',
241256
'$expectedHost' => 'test_uploader_file',
242-
'$expectedFileName' => 'httptest_uploader_file',
257+
'$expectedFileName' => 'test_uploader_file',
258+
'$checkAllowedExtension' => 0,
243259
],
244260
[
245261
'$fileUrl' => 'https://!:^&`;file',
246262
'$expectedHost' => '!:^&`;file',
247-
'$expectedFileName' => 'httpsfile',
263+
'$expectedFileName' => 'file',
264+
'$checkAllowedExtension' => 0,
265+
],
266+
[
267+
'$fileUrl' => 'https://www.google.com/image.jpg',
268+
'$expectedHost' => 'www.google.com/image.jpg',
269+
'$expectedFileName' => 'image.jpg',
270+
'$checkAllowedExtension' => 1,
271+
],
272+
[
273+
'$fileUrl' => 'https://www.google.com/image.jpg?param=1',
274+
'$expectedHost' => 'www.google.com/image.jpg?param=1',
275+
'$expectedFileName' => 'image.jpg',
276+
'$checkAllowedExtension' => 1,
277+
],
278+
[
279+
'$fileUrl' => 'https://www.google.com/image.jpg?param=1&param=2',
280+
'$expectedHost' => 'www.google.com/image.jpg?param=1&param=2',
281+
'$expectedFileName' => 'image.jpg',
282+
'$checkAllowedExtension' => 1,
283+
],
284+
[
285+
'$fileUrl' => 'http://www.google.com/image.jpg?param=1&param=2',
286+
'$expectedHost' => 'www.google.com/image.jpg?param=1&param=2',
287+
'$expectedFileName' => 'image.jpg',
288+
'$checkAllowedExtension' => 1,
248289
],
249290
];
250291
}
@@ -253,8 +294,9 @@ public function moveFileUrlDataProvider()
253294
* @dataProvider validatePathDataProvider
254295
*
255296
* @param bool $pathIsValid
297+
* @return void
256298
*/
257-
public function testSetTmpDir($pathIsValid)
299+
public function testSetTmpDir(bool $pathIsValid)
258300
{
259301
$path = 'path';
260302
$absolutePath = 'absolute_path';
@@ -272,11 +314,11 @@ public function testSetTmpDir($pathIsValid)
272314
*
273315
* @return array
274316
*/
275-
public function validatePathDataProvider()
317+
public function validatePathDataProvider(): array
276318
{
277319
return [
278320
[true],
279-
[false]
321+
[false],
280322
];
281323
}
282324
}

0 commit comments

Comments
 (0)