AC-10044 AC-9889 improve file path validation

Author: tranthienbinh1989

app/code/Magento/Config/Model/Config/Backend/File.php

@@ -278,8 +278,10 @@ protected function _getAllowedExtensions()
      */
     private function setValueAfterValidation(string $value): void
     {
-        // avoid intercepting value
-        if (preg_match('/[^a-z0-9_\/\\-\\.]+/i', $value)) {
+        if (preg_match('/[^a-z0-9_\/\\-\\.]+/i', $value)
+            // phpcs:ignore Magento2.Functions.DiscouragedFunction
+            || !$this->_mediaDirectory->isFile($this->_getUploadDir() . DIRECTORY_SEPARATOR . basename($value))
+        ) {
             throw new LocalizedException(__('Invalid file name'));
         }
 

app/code/Magento/ImportExport/Controller/Adminhtml/Export/File/Download.php

@@ -16,6 +16,8 @@
 use Magento\Framework\Filesystem;
 use Magento\ImportExport\Model\LocalizedFileName;
 use Throwable;
+use Magento\Framework\Controller\Result\Redirect;
+use Magento\Framework\App\ResponseInterface;
 
 /**
  * Controller that download file by name.
@@ -25,7 +27,7 @@ class Download extends ExportController implements HttpGetActionInterface
     /**
      * Url to this controller
      */
-    const URL = 'adminhtml/export_file/download/';
+    public const URL = 'adminhtml/export_file/download/';
 
     /**
      * @var FileFactory
@@ -64,13 +66,24 @@ public function __construct(
     /**
      * Controller basic method implementation.
      *
-     * @return \Magento\Framework\Controller\Result\Redirect | \Magento\Framework\App\ResponseInterface
+     * @return Redirect|ResponseInterface
      */
     public function execute()
     {
         $resultRedirect = $this->resultRedirectFactory->create();
         $resultRedirect->setPath('adminhtml/export/index');
+
         $fileName = $this->getRequest()->getParam('filename');
+
+        if (empty($fileName)) {
+            $this->messageManager->addErrorMessage(__('Please provide valid export file name'));
+
+            return $resultRedirect;
+        }
+
+        // phpcs:ignore Magento2.Functions.DiscouragedFunction
+        $fileName = basename($fileName);
+
         $exportDirectory = $this->filesystem->getDirectoryRead(DirectoryList::VAR_IMPORT_EXPORT);
 
         try {

app/code/Magento/ImportExport/Controller/Adminhtml/History/Download.php

@@ -3,11 +3,18 @@
  * Copyright © Magento, Inc. All rights reserved.
  * See COPYING.txt for license details.
  */
+
+declare(strict_types=1);
+
 namespace Magento\ImportExport\Controller\Adminhtml\History;
 
 use Magento\Framework\App\Action\HttpGetActionInterface;
 use Magento\Framework\App\Filesystem\DirectoryList;
+use Magento\Framework\Controller\ResultInterface;
+use Magento\ImportExport\Helper\Report;
 use Magento\ImportExport\Model\Import;
+use Magento\Framework\Controller\Result\Redirect;
+use Magento\Framework\App\ResponseInterface;
 
 /**
  * Download history controller
@@ -44,20 +51,27 @@ public function __construct(
     /**
      * Download backup action
      *
-     * @return void|\Magento\Backend\App\Action
+     * @return ResponseInterface|Redirect|ResultInterface
+     * @throws \Exception
      */
     public function execute()
     {
+        $resultRedirect = $this->resultRedirectFactory->create();
+        $resultRedirect->setPath('*/history');
+
+        $fileName = $this->getRequest()->getParam('filename');
+
+        if (empty($fileName)) {
+            return $resultRedirect;
+        }
+
         // phpcs:ignore Magento2.Functions.DiscouragedFunction
-        $fileName = basename($this->getRequest()->getParam('filename'));
+        $fileName = basename($fileName);
 
-        /** @var \Magento\ImportExport\Helper\Report $reportHelper */
-        $reportHelper = $this->_objectManager->get(\Magento\ImportExport\Helper\Report::class);
+        /** @var Report $reportHelper */
+        $reportHelper = $this->_objectManager->get(Report::class);
 
         if (!$reportHelper->importFileExists($fileName)) {
-            /** @var \Magento\Backend\Model\View\Result\Redirect $resultRedirect */
-            $resultRedirect = $this->resultRedirectFactory->create();
-            $resultRedirect->setPath('*/history');
             return $resultRedirect;
         }
 

app/code/Magento/ImportExport/Test/Unit/Controller/Adminhtml/History/DownloadTest.php

@@ -8,13 +8,13 @@
 namespace Magento\ImportExport\Test\Unit\Controller\Adminhtml\History;
 
 use Magento\Backend\App\Action\Context;
-use Magento\Backend\Model\View\Result\Redirect;
 use Magento\Framework\App\Filesystem\DirectoryList;
 use Magento\Framework\App\Request\Http;
 use Magento\Framework\App\Response\Http\FileFactory;
 use Magento\Framework\App\ResponseInterface;
 use Magento\Framework\Controller\Result\Raw;
 use Magento\Framework\Controller\Result\RawFactory;
+use Magento\Framework\Controller\Result\Redirect;
 use Magento\Framework\Controller\Result\RedirectFactory;
 use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper;
 use Magento\ImportExport\Controller\Adminhtml\History\Download;
@@ -181,8 +181,7 @@ public function executeDataProvider()
     {
         return [
             'Normal file name' => ['filename.csv', 'filename.csv'],
-            'Relative file name' => ['../../../../../../../../etc/passwd', 'passwd'],
-            'Empty file name' => ['', ''],
+            'Relative file name' => ['../../../../../../../../etc/passwd', 'passwd']
         ];
     }
 
@@ -196,4 +195,27 @@ public function testExecuteFileNotFound()
         $this->resultRaw->expects($this->never())->method('setContents');
         $this->downloadController->execute();
     }
+
+    /**
+     * Test execute() with return Redirect
+     * @param string|null $requestFilename
+     * @dataProvider executeWithRedirectDataProvider
+     */
+    public function testExecuteWithRedirect(?string $requestFilename): void
+    {
+        $this->request->method('getParam')->with('filename')->willReturn($requestFilename);
+        $this->resultRaw->expects($this->never())->method('setContents');
+        $this->assertSame($this->redirect, $this->downloadController->execute());
+    }
+
+    /**
+     * @return array
+     */
+    public function executeWithRedirectDataProvider(): array
+    {
+        return [
+            'null file name' => [null],
+            'empty file name' => [''],
+        ];
+    }
 }