Skip to content

Commit 2a089c9

Browse files
authored
BAP-20848: Unwanted script execution possible in email template preview (#31379)
- added preview sanitizing in EmailRenderer
1 parent d9d0210 commit 2a089c9

File tree

3 files changed

+50
-48
lines changed

3 files changed

+50
-48
lines changed

Diff for: src/Oro/Bundle/EmailBundle/Provider/EmailRenderer.php

+16-1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use Oro\Bundle\EntityBundle\Twig\Sandbox\TemplateRenderer;
88
use Oro\Bundle\EntityBundle\Twig\Sandbox\TemplateRendererConfigProviderInterface;
99
use Oro\Bundle\EntityBundle\Twig\Sandbox\VariableProcessorRegistry;
10+
use Oro\Bundle\UIBundle\Tools\HtmlTagHelper;
1011
use Symfony\Contracts\Translation\TranslatorInterface;
1112
use Twig\Environment;
1213

@@ -20,6 +21,9 @@ class EmailRenderer extends TemplateRenderer
2021
/** @var TranslatorInterface */
2122
private $translator;
2223

24+
/** @var HtmlTagHelper|null */
25+
private $htmlTagHelper;
26+
2327
public function __construct(
2428
Environment $environment,
2529
TemplateRendererConfigProviderInterface $configProvider,
@@ -31,6 +35,11 @@ public function __construct(
3135
$this->translator = $translator;
3236
}
3337

38+
public function setHtmlTagHelper(HtmlTagHelper $htmlTagHelper): void
39+
{
40+
$this->htmlTagHelper = $htmlTagHelper;
41+
}
42+
3443
/**
3544
* Compiles the given email template.
3645
*
@@ -61,8 +70,14 @@ public function compilePreview(EmailTemplateInterface $template): string
6170
{
6271
$this->ensureSandboxConfigured();
6372

73+
if ($this->htmlTagHelper) {
74+
$content = $this->htmlTagHelper->sanitize($template->getContent(), 'default', false);
75+
} else {
76+
$content = $template->getContent();
77+
}
78+
6479
return $this->environment
65-
->createTemplate('{% verbatim %}' . $template->getContent() . '{% endverbatim %}')
80+
->createTemplate('{% verbatim %}' . $content . '{% endverbatim %}')
6681
->render();
6782
}
6883

Diff for: src/Oro/Bundle/EmailBundle/Resources/config/services.yml

+1
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,7 @@ services:
338338
- '@Doctrine\Inflector\Inflector'
339339
calls:
340340
- [addSystemVariableDefaultFilter, ['userSignature', 'oro_html_sanitize']]
341+
- [setHtmlTagHelper, ['@oro_ui.html_tag_helper']]
341342
lazy: true
342343

343344
oro_email.email_renderer_configuration:

Diff for: src/Oro/Bundle/EmailBundle/Tests/Unit/Provider/EmailRendererTest.php

+33-47
Original file line numberDiff line numberDiff line change
@@ -22,22 +22,9 @@
2222

2323
class EmailRendererTest extends \PHPUnit\Framework\TestCase
2424
{
25-
private const ENTITY_VARIABLE_TEMPLATE =
26-
'{% if %val% is defined %}'
27-
. '{{ _entity_var("%name%", %val%, %parent%) }}'
28-
. '{% else %}'
29-
. '{{ "oro.email.variable.not.found" }}'
30-
. '{% endif %}';
31-
3225
/** @var TemplateRendererConfigProviderInterface|\PHPUnit\Framework\MockObject\MockObject */
3326
private $configProvider;
3427

35-
/** @var VariableProcessorRegistry|\PHPUnit\Framework\MockObject\MockObject */
36-
private $variablesProcessorRegistry;
37-
38-
/** @var TranslatorInterface|\PHPUnit\Framework\MockObject\MockObject */
39-
private $translation;
40-
4128
/** @var ContainerInterface|\PHPUnit\Framework\MockObject\MockObject */
4229
private $container;
4330

@@ -46,27 +33,38 @@ class EmailRendererTest extends \PHPUnit\Framework\TestCase
4633

4734
protected function setUp(): void
4835
{
49-
$environment = new Environment(new ArrayLoader(), ['strict_variables' => true]);
5036
$this->configProvider = $this->createMock(TemplateRendererConfigProviderInterface::class);
51-
$this->variablesProcessorRegistry = $this->createMock(VariableProcessorRegistry::class);
52-
$this->translation = $this->createMock(TranslatorInterface::class);
5337
$this->container = $this->createMock(ContainerInterface::class);
5438

55-
$this->translation->expects($this->any())
39+
$variablesProcessorRegistry = $this->createMock(VariableProcessorRegistry::class);
40+
41+
$htmlTagHelper = $this->createMock(HtmlTagHelper::class);
42+
$htmlTagHelper->expects(self::any())
43+
->method('sanitize')
44+
->with(self::isType(\PHPUnit\Framework\Constraint\IsType::TYPE_STRING), 'default', false)
45+
->willReturnCallback(static function (string $content) {
46+
return $content . '(sanitized)';
47+
});
48+
49+
$translation = $this->createMock(TranslatorInterface::class);
50+
$translation->expects(self::any())
5651
->method('trans')
5752
->willReturnArgument(0);
5853

54+
$environment = new Environment(new ArrayLoader(), ['strict_variables' => true]);
5955
$environment->addExtension(new SandboxExtension(new SecurityPolicy()));
6056
$environment->addExtension(new HttpKernelExtension());
6157
$environment->addExtension(new HtmlTagExtension($this->container));
6258

6359
$this->renderer = new EmailRenderer(
6460
$environment,
6561
$this->configProvider,
66-
$this->variablesProcessorRegistry,
67-
$this->translation,
62+
$variablesProcessorRegistry,
63+
$translation,
6864
(new InflectorFactory())->build()
6965
);
66+
67+
$this->renderer->setHtmlTagHelper($htmlTagHelper);
7068
}
7169

7270
private function getEmailTemplate(string $content, string $subject = ''): EmailTemplateInterface
@@ -78,37 +76,25 @@ private function getEmailTemplate(string $content, string $subject = ''): EmailT
7876
return $emailTemplate;
7977
}
8078

81-
private function getEntityVariableTemplate(string $propertyName, string $path, string $parentPath): string
82-
{
83-
return strtr(
84-
self::ENTITY_VARIABLE_TEMPLATE,
85-
[
86-
'%name%' => $propertyName,
87-
'%val%' => $path,
88-
'%parent%' => $parentPath,
89-
]
90-
);
91-
}
92-
9379
private function expectVariables(array $entityVariableProcessors = [], array $systemVariableValues = []): void
9480
{
9581
$entityVariableProcessorsMap = [];
9682
foreach ($entityVariableProcessors as $entityClass => $processors) {
9783
$entityVariableProcessorsMap[] = [$entityClass, $processors];
9884
}
99-
$this->configProvider->expects($this->any())
85+
$this->configProvider->expects(self::any())
10086
->method('getEntityVariableProcessors')
10187
->willReturnMap($entityVariableProcessorsMap);
102-
$this->configProvider->expects($this->any())
88+
$this->configProvider->expects(self::any())
10389
->method('getSystemVariableValues')
10490
->willReturn($systemVariableValues);
10591
}
10692

107-
public function testCompilePreview()
93+
public function testCompilePreview(): void
10894
{
10995
$entity = new TestEntityForVariableProvider();
11096

111-
$this->configProvider->expects($this->any())
97+
$this->configProvider->expects(self::any())
11298
->method('getConfiguration')
11399
->willReturn([
114100
'properties' => [],
@@ -123,10 +109,10 @@ public function testCompilePreview()
123109
$emailTemplate->setContent($template);
124110
$emailTemplate->setSubject('');
125111

126-
$this->assertSame($template, $this->renderer->compilePreview($emailTemplate));
112+
self::assertSame($template.'(sanitized)', $this->renderer->compilePreview($emailTemplate));
127113
}
128114

129-
public function testCompileMessage()
115+
public function testCompileMessage(): void
130116
{
131117
$entity = new TestEntityForVariableProvider();
132118
$entity->setField1('Test');
@@ -144,7 +130,7 @@ public function testCompileMessage()
144130
. ' {{ system.testVar }}'
145131
. ' N/A';
146132

147-
$this->configProvider->expects($this->any())
133+
$this->configProvider->expects(self::any())
148134
->method('getConfiguration')
149135
->willReturn([
150136
'properties' => [],
@@ -155,7 +141,7 @@ public function testCompileMessage()
155141
$this->expectVariables($entityVariableProcessors, $systemVars);
156142

157143
$htmlTagHelper = $this->createMock(HtmlTagHelper::class);
158-
$this->container->expects($this->once())
144+
$this->container->expects(self::once())
159145
->method('get')
160146
->with('oro_ui.html_tag_helper')
161147
->willReturn($htmlTagHelper);
@@ -169,13 +155,13 @@ public function testCompileMessage()
169155
$result = $this->renderer->compileMessage($emailTemplate, $templateParams);
170156
$expectedContent = 'test <a href="http://example.com">test</a> 2 test_system N/A';
171157

172-
$this->assertIsArray($result);
173-
$this->assertCount(2, $result);
174-
$this->assertSame($subject, $result[0]);
175-
$this->assertSame($expectedContent, $result[1]);
158+
self::assertIsArray($result);
159+
self::assertCount(2, $result);
160+
self::assertSame($subject, $result[0]);
161+
self::assertSame($expectedContent, $result[1]);
176162
}
177163

178-
public function testRenderTemplate()
164+
public function testRenderTemplate(): void
179165
{
180166
$template = 'test '
181167
. "\n"
@@ -184,7 +170,7 @@ public function testRenderTemplate()
184170
. '{{ system.currentDate }}';
185171

186172
$entity = new TestEntityForVariableProvider();
187-
$this->configProvider->expects($this->any())
173+
$this->configProvider->expects(self::any())
188174
->method('getConfiguration')
189175
->willReturn([
190176
'properties' => [],
@@ -197,7 +183,7 @@ public function testRenderTemplate()
197183
], ['currentDate' => '10-12-2019']);
198184

199185
$htmlTagHelper = $this->createMock(HtmlTagHelper::class);
200-
$this->container->expects($this->once())
186+
$this->container->expects(self::once())
201187
->method('get')
202188
->with('oro_ui.html_tag_helper')
203189
->willReturn($htmlTagHelper);
@@ -208,6 +194,6 @@ public function testRenderTemplate()
208194
'test '
209195
. "\n"
210196
. '10-12-2019';
211-
$this->assertSame($expectedRenderedResult, $result);
197+
self::assertSame($expectedRenderedResult, $result);
212198
}
213199
}

0 commit comments

Comments
 (0)