AC-9883: Fix Catalog System Config

Author: pawan-adobe-security

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

@@ -3,6 +3,8 @@
  * Copyright © Magento, Inc. All rights reserved.
  * See COPYING.txt for license details.
  */
+declare(strict_types=1);
+
 namespace Magento\Config\Model\Config\Backend;
 
 use Exception;
@@ -94,12 +96,20 @@ public function beforeSave()
         if (!empty($file)) {
             $uploadDir = $this->_getUploadDir();
             try {
+                // sanitize filename
+                $fileName = strtolower(
+                    preg_replace(
+                        ['#[\\s-]+#', '#[^A-Za-z0-9._ -]+#'],
+                        ['-', ''],
+                        $file['name']
+                    )
+                );
                 /** @var Uploader $uploader */
                 $uploader = $this->_uploaderFactory->create(['fileId' => $file]);
                 $uploader->setAllowedExtensions($this->_getAllowedExtensions());
                 $uploader->setAllowRenameFiles(true);
                 $uploader->addValidateCallback('size', $this, 'validateMaxSize');
-                $result = $uploader->save($uploadDir);
+                $result = $uploader->save($uploadDir, $fileName);
             } catch (Exception $e) {
                 throw new LocalizedException(__('%1', $e->getMessage()));
             }
@@ -114,7 +124,7 @@ public function beforeSave()
             if (is_array($value) && !empty($value['delete'])) {
                 $this->setValue('');
             } elseif (is_array($value) && !empty($value['value'])) {
-                $this->setValue($value['value']);
+                $this->setValueAfterValidation($value['value']);
             } else {
                 $this->unsValue();
             }
@@ -266,4 +276,21 @@ protected function _getAllowedExtensions()
     {
         return [];
     }
+
+    /**
+     * Validate if the value is intercepted
+     *
+     * @param string $value
+     * @return void
+     * @throws LocalizedException
+     */
+    private function setValueAfterValidation($value)
+    {
+        // avoid intercepting value
+        if (!preg_match('/^[A-Za-z0-9._\/ -]*$/', $value)) {
+            throw new LocalizedException(__('%1', 'Invalid file name'));
+        }
+
+        $this->setValue($value);
+    }
 }

app/code/Magento/Config/Test/Unit/Model/Config/Backend/FileTest.php

@@ -152,7 +152,7 @@ public function testBeforeSave()
             ->willReturn($this->uploaderMock);
         $this->uploaderMock->expects($this->once())
             ->method('save')
-            ->with($uploadDir . '/' . $scope . '/' . $scopeId, null)
+            ->with($uploadDir . '/' . $scope . '/' . $scopeId, $name)
             ->willReturn($result);
 
         $this->assertEquals($this->model, $this->model->beforeSave());
@@ -197,7 +197,7 @@ public function testBeforeWithoutRequest()
             ->willReturn($this->uploaderMock);
         $this->uploaderMock->expects($this->once())
             ->method('save')
-            ->with($uploadDir, null)
+            ->with($uploadDir, $name)
             ->willReturn($result);
 
         $this->assertEquals($this->model, $this->model->beforeSave());
@@ -300,7 +300,7 @@ public function testBeforeSaveWithException()
             ->willReturn($this->uploaderMock);
         $this->uploaderMock->expects($this->once())
             ->method('save')
-            ->with($uploadDir, null)
+            ->with($uploadDir, $name)
             ->willThrowException(new \Exception($exception));
 
         $this->model->beforeSave();

app/code/Magento/Config/Test/Unit/Model/Config/Backend/Image/LogoTest.php

@@ -56,7 +56,7 @@ protected function setUp(): void
             ->willReturn($this->uploaderMock);
         $this->requestDataMock = $this
             ->getMockBuilder(RequestDataInterface::class)
-            ->setMethods(['getTmpName'])
+            ->setMethods(['getTmpName', 'getName'])
             ->getMockForAbstractClass();
         $mediaDirectoryMock = $this->getMockBuilder(WriteInterface::class)
             ->disableOriginalConstructor()
@@ -83,6 +83,9 @@ public function testBeforeSave()
         $this->requestDataMock->expects($this->once())
             ->method('getTmpName')
             ->willReturn('/tmp/val');
+        $this->requestDataMock->expects($this->once())
+            ->method('getName')
+            ->willReturn('filename');
         $this->uploaderMock->expects($this->once())
             ->method('setAllowedExtensions')
             ->with(['jpg', 'jpeg', 'gif', 'png']);

app/code/Magento/Config/i18n/en_US.csv

@@ -122,3 +122,4 @@ Dashboard,Dashboard
 "Web Section","Web Section"
 "Store Email Addresses Section","Store Email Addresses Section"
 "Email to a Friend","Email to a Friend"
+"Invalid file name", "Invalid file name"

lib/internal/Magento/Framework/Data/Form/Element/Image.php

@@ -9,6 +9,7 @@
 
 use Magento\Framework\App\ObjectManager;
 use Magento\Framework\Escaper;
+use Magento\Framework\Exception\LocalizedException;
 use Magento\Framework\Math\Random;
 use Magento\Framework\UrlInterface;
 use Magento\Framework\View\Helper\SecureHtmlRenderer;
@@ -66,35 +67,33 @@ public function __construct(
      * Return element html code
      *
      * @return string
+     * @throws LocalizedException
      */
     public function getElementHtml()
     {
         $html = '';
 
-        if ((string)$this->getValue()) {
+        if ((string)$this->getEscapedValue()) {
             $url = $this->_getUrl();
 
             if (!preg_match("/^http\:\/\/|https\:\/\//", $url)) {
                 $url = $this->_urlBuilder->getBaseUrl(['_type' => UrlInterface::URL_TYPE_MEDIA]) . $url;
             }
 
             $linkId = 'linkId' .$this->random->getRandomString(8);
-            $html = '<a previewlinkid="' .$linkId .'" href="' .
-                $url .
-                '" ' .
+            $html = '<a previewlinkid="' .$linkId  .'" href="' .
+                $url . '" ' .
                 $this->_getUiId(
                     'link'
                 ) .
                 '>' .
-                '<img src="' .
-                $url .
-                '" id="' .
+                '<img src="' . $url . '" id="' .
                 $this->getHtmlId() .
                 '_image" title="' .
-                $this->getValue() .
+                $this->getEscapedValue() .
                 '"' .
                 ' alt="' .
-                $this->getValue() .
+                $this->getEscapedValue() .
                 '" height="22" width="22" class="small-image-preview v-middle"  ' .
                 $this->_getUiId() .
                 ' />' .
@@ -120,7 +119,7 @@ public function getElementHtml()
     protected function _getDeleteCheckbox()
     {
         $html = '';
-        if ($this->getValue()) {
+        if ($this->getEscapedValue()) {
             $label = (string)new \Magento\Framework\Phrase('Delete Image');
             $html .= '<span class="delete-image">';
             $html .= '<input type="checkbox"' .
@@ -153,7 +152,8 @@ protected function _getDeleteCheckbox()
      */
     protected function _getHiddenInput()
     {
-        return '<input type="hidden" name="' . parent::getName() . '[value]" value="' . $this->getValue() . '" />';
+        return '<input type="hidden" name="' . parent::getName() .
+            '[value]" value="' . $this->getEscapedValue() . '" />';
     }
 
     /**
@@ -163,7 +163,7 @@ protected function _getHiddenInput()
      */
     protected function _getUrl()
     {
-        return $this->getValue();
+        return $this->getEscapedValue();
     }
 
     /**

lib/internal/Magento/Framework/Data/Test/Unit/Form/Element/ImageTest.php

@@ -15,12 +15,12 @@
 use Magento\Framework\Data\Form\Element\Image;
 use Magento\Framework\DataObject;
 use Magento\Framework\Escaper;
+use Magento\Framework\Math\Random;
+use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
 use Magento\Framework\Url;
-use Magento\Framework\UrlInterface;
+use Magento\Framework\View\Helper\SecureHtmlRenderer;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
-use Magento\Framework\Math\Random;
-use Magento\Framework\View\Helper\SecureHtmlRenderer;
 
 /**
  * Test for the widget.
@@ -44,11 +44,16 @@ class ImageTest extends TestCase
      */
     protected $_image;
 
+    /**
+     * @var array
+     */
+    protected $testData;
+
     protected function setUp(): void
     {
+        $objectManager = new ObjectManager($this);
         $factoryMock = $this->createMock(Factory::class);
         $collectionFactoryMock = $this->createMock(CollectionFactory::class);
-        $escaperMock = $this->createMock(Escaper::class);
         $this->urlBuilder = $this->createMock(Url::class);
         $randomMock = $this->createMock(Random::class);
         $randomMock->method('getRandomString')->willReturn('some-rando-string');
@@ -67,18 +72,28 @@ function (string $tag, array $attrs, ?string $content): string {
                     return "<$tag {$attrs->serialize()}>$content</$tag>";
                 }
             );
-        $this->_image = new Image(
-            $factoryMock,
-            $collectionFactoryMock,
-            $escaperMock,
-            $this->urlBuilder,
-            [],
-            $secureRendererMock,
-            $randomMock
+        $this->_image = $objectManager->getObject(
+            Image::class,
+            [
+                'factoryMock'=>$factoryMock,
+                'collectionFactoryMock'=>$collectionFactoryMock,
+                'urlBuilder' => $this->urlBuilder,
+                '_escaper' => $objectManager->getObject(Escaper::class),
+                'random' => $randomMock,
+                'secureRenderer' => $secureRendererMock,
+            ]
         );
+        $this->testData = [
+            'html_id_prefix' => 'test_id_prefix_',
+            'html_id' => 'test_id',
+            'html_id_suffix' => '_test_id_suffix',
+            'path' => 'catalog/product/placeholder',
+            'value' => 'test_value',
+        ];
+
         $formMock = new DataObject();
-        $formMock->getHtmlIdPrefix('id_prefix');
-        $formMock->getHtmlIdPrefix('id_suffix');
+        $formMock->getHtmlIdPrefix($this->testData['html_id_prefix']);
+        $formMock->getHtmlIdPrefix($this->testData['html_id_suffix']);
         $this->_image->setForm($formMock);
     }
 
@@ -117,21 +132,32 @@ public function testGetElementHtmlWithoutValue()
      */
     public function testGetElementHtmlWithValue()
     {
-        $this->_image->setValue('test_value');
-        $this->urlBuilder->expects($this->once())
-            ->method('getBaseUrl')
-            ->with(['_type' => UrlInterface::URL_TYPE_MEDIA])
-            ->willReturn('http://localhost/media/');
+        $url = 'http://test.example.com/media/';
+
+        $this->_image->setValue($this->testData['value']);
+        $this->_image->setHtmlId($this->testData['html_id']);
+
+        $this->urlBuilder->expects($this->once())->method('getBaseUrl')
+            ->with(['_type' => UrlInterface::URL_TYPE_MEDIA])->willReturn($url);
+
+        $expectedHtmlId = $this->testData['html_id'];
+
         $html = $this->_image->getElementHtml();
+
         $this->assertStringContainsString('class="input-file"', $html);
         $this->assertStringContainsString('<input', $html);
         $this->assertStringContainsString('type="file"', $html);
         $this->assertStringContainsString('value="test_value"', $html);
+
         $this->assertStringContainsString(
-            '<a previewlinkid="linkIdsome-rando-string" href="http://localhost/media/test_value"',
+            '<a previewlinkid="linkIdsome-rando-string" href="'
+            . $url
+            . $this->testData['value']
+            . '"',
             $html
         );
-        $this->assertStringContainsString("imagePreview('_image');\nreturn false;", $html);
+
+        $this->assertStringContainsString("imagePreview('{$expectedHtmlId}_image');\nreturn false;", $html);
         $this->assertStringContainsString('<input type="checkbox"', $html);
     }
 }