Skip to content

Commit ef591d2

Browse files
nathanjosiahnathanjosiah
committed
Merge remote-tracking branch 'origin/MC-24196' into borg-2.4.0
2 parents 9c5c8db + ce7fe49 commit ef591d2

File tree

10 files changed

+175
-20
lines changed

10 files changed

+175
-20
lines changed

‎app/code/Magento/Cms/Model/Template/Filter.php

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@
55
*/
66
namespace Magento\Cms\Model\Template;
77

8-
use Magento\Framework\Exception\LocalizedException;
9-
108
/**
119
* Cms Template Filter Model
1210
*/
@@ -41,8 +39,8 @@ public function mediaDirective($construction)
4139
{
4240
// phpcs:ignore Magento2.Functions.DiscouragedFunction
4341
$params = $this->getParameters(html_entity_decode($construction[2], ENT_QUOTES));
44-
if (preg_match('/\.\.(\\\|\/)/', $params['url'])) {
45-
throw new \InvalidArgumentException('Image path must be absolute');
42+
if (preg_match('/(^.*:\/\/.*|\.\.\/.*)/', $params['url'])) {
43+
throw new \InvalidArgumentException('Image path must be absolute and not include URLs');
4644
}
4745

4846
return $this->_storeManager->getStore()->getBaseMediaDir() . '/' . $params['url'];

‎app/code/Magento/Cms/Test/Unit/Model/Template/FilterTest.php

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,4 +105,24 @@ public function testMediaDirectiveRelativePath()
105105
->willReturn($baseMediaDir);
106106
$this->filter->mediaDirective($construction);
107107
}
108+
109+
/**
110+
* Test using media directive with a URL path including schema.
111+
*
112+
* @covers \Magento\Cms\Model\Template\Filter::mediaDirective
113+
* @expectedException \InvalidArgumentException
114+
*/
115+
public function testMediaDirectiveURL()
116+
{
117+
$baseMediaDir = 'pub/media';
118+
$construction = [
119+
'{{media url="http://wysiwyg/images/image.jpg"}}',
120+
'media',
121+
' url="http://wysiwyg/images/../image.jpg"'
122+
];
123+
$this->storeMock->expects($this->any())
124+
->method('getBaseMediaDir')
125+
->willReturn($baseMediaDir);
126+
$this->filter->mediaDirective($construction);
127+
}
108128
}

‎app/code/Magento/Email/Model/Template/Filter.php

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@
66
namespace Magento\Email\Model\Template;
77

88
use Magento\Framework\App\ObjectManager;
9+
use Magento\Framework\Exception\MailException;
10+
use Magento\Framework\Exception\NoSuchEntityException;
911
use Magento\Framework\Filesystem;
10-
use Magento\Framework\Filesystem\Directory\ReadInterface;
1112
use Magento\Framework\Filter\VariableResolverInterface;
1213
use Magento\Framework\View\Asset\ContentProcessorException;
1314
use Magento\Framework\View\Asset\ContentProcessorInterface;
@@ -734,27 +735,32 @@ public function modifierEscape($value, $type = 'html')
734735
* {{protocol store="1"}} - Optional parameter which gets protocol from provide store based on store ID or code
735736
*
736737
* @param string[] $construction
737-
* @throws \Magento\Framework\Exception\MailException
738738
* @return string
739+
* @throws MailException
740+
* @throws NoSuchEntityException
739741
*/
740742
public function protocolDirective($construction)
741743
{
742744
$params = $this->getParameters($construction[2]);
745+
743746
$store = null;
744747
if (isset($params['store'])) {
745748
try {
746749
$store = $this->_storeManager->getStore($params['store']);
747750
} catch (\Exception $e) {
748-
throw new \Magento\Framework\Exception\MailException(
751+
throw new MailException(
749752
__('Requested invalid store "%1"', $params['store'])
750753
);
751754
}
752755
}
756+
753757
$isSecure = $this->_storeManager->getStore($store)->isCurrentlySecure();
754758
$protocol = $isSecure ? 'https' : 'http';
755759
if (isset($params['url'])) {
756760
return $protocol . '://' . $params['url'];
757761
} elseif (isset($params['http']) && isset($params['https'])) {
762+
$this->validateProtocolDirectiveHttpScheme($params);
763+
758764
if ($isSecure) {
759765
return $params['https'];
760766
}
@@ -764,6 +770,37 @@ public function protocolDirective($construction)
764770
return $protocol;
765771
}
766772

773+
/**
774+
* Validate protocol directive HTTP parameters.
775+
*
776+
* @param string[] $params
777+
* @return void
778+
* @throws MailException
779+
*/
780+
private function validateProtocolDirectiveHttpScheme(array $params) : void
781+
{
782+
$parsed_http = parse_url($params['http']);
783+
$parsed_https = parse_url($params['https']);
784+
785+
if (empty($parsed_http)) {
786+
throw new MailException(
787+
__('Contents of %1 could not be loaded or is empty', $params['http'])
788+
);
789+
} elseif (empty($parsed_https)) {
790+
throw new MailException(
791+
__('Contents of %1 could not be loaded or is empty', $params['https'])
792+
);
793+
} elseif ($parsed_http['scheme'] !== 'http') {
794+
throw new MailException(
795+
__('Contents of %1 could not be loaded or is empty', $params['http'])
796+
);
797+
} elseif ($parsed_https['scheme'] !== 'https') {
798+
throw new MailException(
799+
__('Contents of %1 could not be loaded or is empty', $params['https'])
800+
);
801+
}
802+
}
803+
767804
/**
768805
* Store config directive
769806
*

‎app/code/Magento/Email/Test/Unit/Model/Template/FilterTest.php

Lines changed: 44 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@
1212
use Magento\Email\Model\Template\Filter;
1313
use Magento\Framework\App\Area;
1414
use Magento\Framework\App\Filesystem\DirectoryList;
15-
use Magento\Framework\Filesystem\Directory\ReadInterface;
15+
use Magento\Framework\Exception\MailException;
16+
use Magento\Framework\Exception\NoSuchEntityException;
1617
use Magento\Framework\View\Asset\File\FallbackContext;
1718

1819
/**
@@ -194,20 +195,16 @@ protected function setUp()
194195
->getMock();
195196

196197
$this->directiveProcessors = [
197-
'depend' =>
198-
$this->getMockBuilder(\Magento\Framework\Filter\DirectiveProcessor\DependDirective::class)
198+
'depend' => $this->getMockBuilder(\Magento\Framework\Filter\DirectiveProcessor\DependDirective::class)
199199
->disableOriginalConstructor()
200200
->getMock(),
201-
'if' =>
202-
$this->getMockBuilder(\Magento\Framework\Filter\DirectiveProcessor\IfDirective::class)
201+
'if' => $this->getMockBuilder(\Magento\Framework\Filter\DirectiveProcessor\IfDirective::class)
203202
->disableOriginalConstructor()
204203
->getMock(),
205-
'template' =>
206-
$this->getMockBuilder(\Magento\Framework\Filter\DirectiveProcessor\TemplateDirective::class)
204+
'template' => $this->getMockBuilder(\Magento\Framework\Filter\DirectiveProcessor\TemplateDirective::class)
207205
->disableOriginalConstructor()
208206
->getMock(),
209-
'legacy' =>
210-
$this->getMockBuilder(\Magento\Framework\Filter\DirectiveProcessor\LegacyDirective::class)
207+
'legacy' => $this->getMockBuilder(\Magento\Framework\Filter\DirectiveProcessor\LegacyDirective::class)
211208
->disableOriginalConstructor()
212209
->getMock(),
213210
];
@@ -432,4 +429,42 @@ public function testConfigDirectiveUnavailable()
432429

433430
$this->assertEquals($scopeConfigValue, $this->getModel()->configDirective($construction));
434431
}
432+
433+
/**
434+
* @throws MailException
435+
* @throws NoSuchEntityException
436+
*/
437+
public function testProtocolDirectiveWithValidSchema()
438+
{
439+
$model = $this->getModel();
440+
$storeMock = $this->createMock(\Magento\Store\Model\Store::class);
441+
$storeMock->expects($this->once())->method('isCurrentlySecure')->willReturn(true);
442+
$this->storeManager->expects($this->once())->method('getStore')->willReturn($storeMock);
443+
444+
$data = [
445+
"{{protocol http=\"http://url\" https=\"https://url\"}}",
446+
"protocol",
447+
" http=\"http://url\" https=\"https://url\""
448+
];
449+
$this->assertEquals('https://url', $model->protocolDirective($data));
450+
}
451+
452+
/**
453+
* @expectedException \Magento\Framework\Exception\MailException
454+
* @throws NoSuchEntityException
455+
*/
456+
public function testProtocolDirectiveWithInvalidSchema()
457+
{
458+
$model = $this->getModel();
459+
$storeMock = $this->createMock(\Magento\Store\Model\Store::class);
460+
$storeMock->expects($this->once())->method('isCurrentlySecure')->willReturn(true);
461+
$this->storeManager->expects($this->once())->method('getStore')->willReturn($storeMock);
462+
463+
$data = [
464+
"{{protocol http=\"https://url\" https=\"http://url\"}}",
465+
"protocol",
466+
" http=\"https://url\" https=\"http://url\""
467+
];
468+
$model->protocolDirective($data);
469+
}
435470
}

‎app/code/Magento/Ui/Controller/Index/Render.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,8 @@ public function execute()
105105

106106
$contentType = $this->contentTypeResolver->resolve($component->getContext());
107107
$this->getResponse()->setHeader('Content-Type', $contentType, true);
108+
109+
return;
108110
} else {
109111
/** @var \Magento\Framework\Controller\Result\Json $resultJson */
110112
$resultJson = $this->resultJsonFactory->create();

‎dev/tests/static/testsuite/Magento/Test/Php/_files/phpstan/blacklist/common.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
# dev/tests/static/framework/bootstrap.php
66
lib/internal/Magento/Framework/Interception/Test/Unit/Config/ConfigTest.php
77
lib/internal/Magento/Framework/Cache/Backend/Eaccelerator.php
8+
lib/internal/Magento/Framework/Image/Adapter/ImageMagick.php
89
dev/tests/integration/framework/deployTestModules.php
910
dev/tests/integration/testsuite/Magento/Framework/Session/ConfigTest.php
1011
dev/tests/integration/testsuite/Magento/Framework/Session/SessionManagerTest.php

‎lib/internal/Magento/Framework/Image/Adapter/Gd2.php

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ protected function _reset()
6060
*/
6161
public function open($filename)
6262
{
63-
if (!$filename || filesize($filename) === 0) {
63+
if (!$filename || filesize($filename) === 0 || !$this->validateURLScheme($filename)) {
6464
throw new \InvalidArgumentException('Wrong file');
6565
}
6666
$this->_fileName = $filename;
@@ -87,6 +87,23 @@ public function open($filename)
8787
}
8888
}
8989

90+
/**
91+
* Checks for invalid URL schema if it exists
92+
*
93+
* @param string $filename
94+
* @return bool
95+
*/
96+
private function validateURLScheme(string $filename) : bool
97+
{
98+
$allowed_schemes = ['ftp', 'ftps', 'http', 'https'];
99+
$url = parse_url($filename);
100+
if ($url && isset($url['scheme']) && !in_array($url['scheme'], $allowed_schemes)) {
101+
return false;
102+
}
103+
104+
return true;
105+
}
106+
90107
/**
91108
* Checks whether memory limit is reached.
92109
*

‎lib/internal/Magento/Framework/Image/Adapter/ImageMagick.php

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
*/
66
namespace Magento\Framework\Image\Adapter;
77

8+
/**
9+
* ImageMagick adapter.
10+
*/
811
class ImageMagick extends \Magento\Framework\Image\Adapter\AbstractAdapter
912
{
1013
/**
@@ -70,13 +73,18 @@ public function backgroundColor($color = null)
7073
*/
7174
public function open($filename)
7275
{
76+
if (!empty($filename) && !$this->validateURLScheme($filename)) {
77+
throw new \InvalidArgumentException('Wrong file');
78+
}
79+
7380
$this->_fileName = $filename;
7481
$this->_checkCanProcess();
7582
$this->_getFileAttributes();
7683

7784
try {
7885
$this->_imageHandler = new \Imagick($this->_fileName);
7986
} catch (\ImagickException $e) {
87+
//phpcs:ignore Magento2.Exceptions.DirectThrow
8088
throw new \Exception(sprintf('Unsupported image format. File: %s', $this->_fileName), $e->getCode(), $e);
8189
}
8290

@@ -85,8 +93,24 @@ public function open($filename)
8593
}
8694

8795
/**
88-
* Save image to specific path.
89-
* If some folders of path does not exist they will be created
96+
* Checks for invalid URL schema if it exists
97+
*
98+
* @param string $filename
99+
* @return bool
100+
*/
101+
private function validateURLScheme(string $filename) : bool
102+
{
103+
$allowed_schemes = ['ftp', 'ftps', 'http', 'https'];
104+
$url = parse_url($filename);
105+
if ($url && isset($url['scheme']) && !in_array($url['scheme'], $allowed_schemes)) {
106+
return false;
107+
}
108+
109+
return true;
110+
}
111+
112+
/**
113+
* Save image to specific path. If some folders of path does not exist they will be created
90114
*
91115
* @param null|string $destination
92116
* @param null|string $newName
@@ -124,6 +148,8 @@ protected function _applyOptions()
124148
}
125149

126150
/**
151+
* Render image and return its binary contents
152+
*
127153
* @see \Magento\Framework\Image\Adapter\AbstractAdapter::getImage
128154
* @return string
129155
*/
@@ -134,11 +160,12 @@ public function getImage()
134160
}
135161

136162
/**
137-
* Change the image size
163+
* Change the image size.
138164
*
139165
* @param null|int $frameWidth
140166
* @param null|int $frameHeight
141167
* @return void
168+
* @throws \ImagickException
142169
*/
143170
public function resize($frameWidth = null, $frameHeight = null)
144171
{
@@ -333,6 +360,7 @@ public function watermark($imagePath, $positionX = 0, $positionY = 0, $opacity =
333360
);
334361
}
335362
} catch (\ImagickException $e) {
363+
//phpcs:ignore Magento2.Exceptions.DirectThrow
336364
throw new \Exception('Unable to create watermark.', $e->getCode(), $e);
337365
}
338366

@@ -351,6 +379,7 @@ public function watermark($imagePath, $positionX = 0, $positionY = 0, $opacity =
351379
public function checkDependencies()
352380
{
353381
if (!class_exists('\Imagick', false)) {
382+
//phpcs:ignore Magento2.Exceptions.DirectThrow
354383
throw new \Exception("Required PHP extension 'Imagick' was not loaded.");
355384
}
356385
}

‎lib/internal/Magento/Framework/Image/Test/Unit/Adapter/Gd2Test.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,4 +151,12 @@ public function testOpenDifferentTypes()
151151

152152
$this->assertNotEquals($type1, $type2);
153153
}
154+
155+
/**
156+
* @expectedException \InvalidArgumentException
157+
*/
158+
public function testOpenInvalidURL()
159+
{
160+
$this->adapter->open('bar://foo.bar');
161+
}
154162
}

‎lib/internal/Magento/Framework/Image/Test/Unit/Adapter/ImageMagickTest.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,4 +91,12 @@ public function testSaveWithException()
9191
$this->loggerMock->expects($this->once())->method('critical')->with($exception);
9292
$this->imageMagic->save('product/cache', 'sample.jpg');
9393
}
94+
95+
/**
96+
* @expectedException \InvalidArgumentException
97+
*/
98+
public function testOpenInvalidUrl()
99+
{
100+
$this->imageMagic->open('bar://foo.bar');
101+
}
94102
}

0 commit comments

Comments
 (0)