Skip to content

Commit 195a26f

Browse files
cammonrocammonro
authored
Merge pull request #1795 from algolia/fix/MAGE-1379-phpcs-phtml-error
MAGE-1377 MAGE-1379 MAGE-1381 Sanitize untrusted inputs
2 parents 716c9b5 + acaeebc commit 195a26f

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

42 files changed

+571
-500
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
- Added support for PHPUnit 10
4444
- Added support for Magento 2.4.8 on PHP 8.4 - Special shout out to @jajajaime for his help here
4545
- Refactored code to PHP 8.2 baseline standard
46+
- Added string escaping to untrusted inputs
4647
- The InstantSearch "replace categories" feature must now be explicitly enabled on new instances aligning with our documentation: https://www.algolia.com/doc/integration/magento-2/guides/category-pages/#enable-category-pages
4748

4849
### Breaking Changes

‎Helper/ConfigHelper.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1253,7 +1253,8 @@ public function preventBackendRendering($storeId = null): bool
12531253
if (!isset($_SERVER['HTTP_USER_AGENT'])) {
12541254
return false;
12551255
}
1256-
$userAgent = mb_strtolower((string) $_SERVER['HTTP_USER_AGENT'], 'utf-8');
1256+
$userAgent = mb_strtolower((string) filter_input(INPUT_SERVER, 'HTTP_USER_AGENT', FILTER_SANITIZE_SPECIAL_CHARS), 'utf-8');
1257+
12571258
$allowedUserAgents = $this->configInterface->getValue(
12581259
self::BACKEND_RENDERING_ALLOWED_USER_AGENTS,
12591260
ScopeInterface::SCOPE_STORE,

‎Model/Backend/QueueCron.php

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@
77

88
class QueueCron extends Value
99
{
10-
const CRON_REGEX = '/^(\*|[0-9,\-\/\*]+)\s+(\*|[0-9,\-\/\*]+)\s+(\*|[0-9,\-\/\*]+)\s+(\*|[0-9,\-\/\*]+)\s+(\*|[0-9,\-\/\*]+)$/';
10+
const CRON_FORMAT_REGEX = '/^(\*|[0-9,\-\/\*]+)\s+(\*|[0-9,\-\/\*]+)\s+(\*|[0-9,\-\/\*]+)\s+(\*|[0-9,\-\/\*]+)\s+(\*|[0-9,\-\/\*]+)$/';
11+
const CRON_DISALLOW_REGEX = '/[^@a-z0-9\*\-,\/ ]/';
1112

1213
protected array $mappings = [
1314
'@yearly' => '0 0 1 1 *',
@@ -27,8 +28,19 @@ public function beforeSave()
2728
$this->setValue($value);
2829
}
2930

30-
if (!preg_match(self::CRON_REGEX, $value)) {
31-
throw new InvalidCronException("Cron expression \"$value\" is not valid.");
31+
if (!preg_match(self::CRON_FORMAT_REGEX, $value)) {
32+
// This use of preg_replace is safe — static regex without /e modifier.
33+
// phpcs:ignore
34+
$safeValue = preg_replace(self::CRON_DISALLOW_REGEX, '', (string) $value);
35+
$msg = ($safeValue !== $value)
36+
? 'Cron expression is invalid.'
37+
: sprintf(
38+
'Cron expression "%s" is not valid.',
39+
htmlspecialchars($safeValue, ENT_QUOTES, 'UTF-8')
40+
);
41+
// Content is already escaped at this point
42+
// phpcs:ignore
43+
throw new InvalidCronException($msg);
3244
}
3345

3446
return parent::beforeSave();

‎Test/Unit/Model/Backend/QueueCronTest.php

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ protected function setUp(): void
3131
/**
3232
* @dataProvider valuesProvider
3333
*/
34-
public function testInput($value, $isValid): void
34+
public function testInput($value, $isValid, $canReplay = true): void
3535
{
3636
$this->queueCronModel->setValue($value);
3737

@@ -45,8 +45,11 @@ public function testInput($value, $isValid): void
4545
"Cron expression \"$value\" is not valid but it should be."
4646
);
4747

48+
$msg = $canReplay
49+
? "Cron expression \"$value\" is not valid."
50+
: "Cron expression is invalid.";
4851
$this->assertEquals(
49-
"Cron expression \"$value\" is not valid.",
52+
$msg,
5053
$exception->getMessage()
5154
);
5255
}
@@ -94,6 +97,11 @@ public static function valuesProvider(): array
9497
[
9598
'value' => '@foo', // Not working alias
9699
'isValid' => false
100+
],
101+
[
102+
'value' => '"><script>alert(\'XSS\')</script>',
103+
'isValid' => false,
104+
'canReplay' => false
97105
]
98106
];
99107
}

‎view/adminhtml/templates/analytics/form.phtml

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,36 @@
11
<?php
22
/** @var \Magento\Backend\Block\Template $block */
3+
/** @var \Magento\Framework\Escaper $escaper */
34
/** @var \Algolia\AlgoliaSearch\ViewModel\Adminhtml\Analytics\Form $form */
45
$form = $block->getViewModel();
56
?>
67

78
<?php $isFormDisabled = !$form->isAnalyticsApiEnabled() ? ' disabled' : '' ?>
89
<div class="algolia-analytics-form" id="algolia-analytics-daterange">
9-
<form id="algolia-analytics-form" data-mage-init='{"validation":{}}' action="<?php echo $form->getFormAction() ?>" method="post">
10-
<input type="hidden" name="form_key" value="<?php echo $block->getFormKey() ?>" />
10+
<form id="algolia-analytics-form" data-mage-init='{"validation":{}}' action="<?= $escaper->escapeUrl($form->getFormAction()) ?>" method="post">
11+
<input type="hidden" name="form_key" value="<?= $escaper->escapeHtmlAttr($block->getFormKey()) ?>" />
1112
<div class="inline-field">
12-
<select name="type" class="select admin__control-select"<?php echo $isFormDisabled ?>>
13-
<?php foreach ($form->getSections() as $key => $section) : ?>
14-
<option value="<?php echo $key ?>"<?php echo ($key == $form->getFormValue('type')) ? ' selected' : '' ?>>
15-
<?php echo ucwords($key) ?>
13+
<select name="type" class="select admin__control-select"<?= /** phpcs:ignore Magento2.Security.XssTemplate.FoundUnescaped **/ $isFormDisabled ?>>
14+
<?php foreach ($form->getSections() as $key => $section): ?>
15+
<option value="<?= $escaper->escapeHtmlAttr($key) ?>"<?= ($key == $form->getFormValue('type')) ? ' selected' : '' ?>>
16+
<?= $escaper->escapeHtml(ucwords($key)) ?>
1617
</option>
1718
<?php endforeach; ?>
1819
</select>
1920
</div>
2021
<div class="inline-field">
21-
<input type="text" id="analytics_from" name="from" <?php echo $isFormDisabled ?>
22+
<input type="text" id="analytics_from" name="from" <?= /** phpcs:ignore Magento2.Security.XssTemplate.FoundUnescaped **/ $isFormDisabled ?>
2223
class="admin__control-text input-text date-range-analytics-from"
23-
placeholder="<?php echo $block->escapeHtmlAttr(__('Start Date')) ?>" value="<?php echo $form->getFormValue('from') ?>"/>
24+
placeholder="<?= $escaper->escapeHtmlAttr(__('Start Date')) ?>" value="<?= $escaper->escapeHtmlAttr($form->getFormValue('from')) ?>"/>
2425
</div>
2526
<span class="divide">-</span>
2627
<div class="inline-field">
27-
<input type="text" id="analytics_to" name="to" <?php echo $isFormDisabled ?>
28+
<input type="text" id="analytics_to" name="to" <?= /** phpcs:ignore Magento2.Security.XssTemplate.FoundUnescaped **/ $isFormDisabled ?>
2829
class="admin__control-text input-text validate-date-range date-range-analytics-to"
29-
placeholder="<?php echo $block->escapeHtmlAttr(__('End Date')) ?>" value="<?php echo $form->getFormValue('to') ?>"/>
30+
placeholder="<?= $escaper->escapeHtmlAttr(__('End Date')) ?>" value="<?= $escaper->escapeHtmlAttr($form->getFormValue('to')) ?>"/>
3031
</div>
31-
<button type="submit" class="action submit primary algolia-daterange-submit" <?php echo $isFormDisabled ?>>
32-
<?php echo $block->escapeHtml(__('Update')) ?>
32+
<button type="submit" class="action submit primary algolia-daterange-submit" <?= /** phpcs:ignore Magento2.Security.XssTemplate.FoundUnescaped **/ $isFormDisabled ?>>
33+
<?= $escaper->escapeHtml(__('Update')) ?>
3334
</button>
3435
</form>
3536
</div>
@@ -70,4 +71,4 @@ $form = $block->getViewModel();
7071
}
7172
});
7273
});
73-
</script>
74+
</script>

‎view/adminhtml/templates/analytics/graph.phtml

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
<?php
22
/** @var \Magento\Backend\Block\Template $block */
3+
/** @var \Magento\Framework\Escaper $escaper */
34
?>
4-
<?php if (!empty($block->getAnalytics())) : ?>
5+
<?php if (!empty($block->getAnalytics())): ?>
56
<div id="algolia-analyatics-diagram"></div>
67
<script type="text/javascript" src="https://www.gstatic.com/charts/loader.js"></script>
78
<script type="text/javascript">
@@ -11,17 +12,17 @@
1112

1213
var data = new google.visualization.arrayToDataTable([
1314
['Date', 'Searches', {type: 'string', role: 'tooltip', p: {'html': true}}, {role: 'style'}],
14-
<?php foreach ($block->getAnalytics() as $item) : ?>
15-
['<?php echo $item['formatted'] ?>', <?php echo $item['count'] ?>,
16-
"<div style='padding: 1.5rem; line-height: 18px;'><strong style='font-size: 14px;'><?php echo $item['formatted'] ?></strong><br/><br/>" +
17-
"<p>Searches: <strong style='color: #5468ff; padding-left: 3px;'><?php echo $item['count'] ?></strong></p>" +
18-
"<p>Users: <strong style='color: #3a46a1; padding-left: 3px;'><?php echo $item['users'] ?></strong></p>" +
19-
"<p>No Result Rate: <strong style='color: #3ab2bd; padding-left: 3px;'><?php echo round((int) $item['rate'] * 100, 2) . '%' ?></strong></p>" +
20-
<?php if (isset($item['ctr'])) : ?>
15+
<?php foreach ($block->getAnalytics() as $item): ?>
16+
['<?= $escaper->escapeHtml($item['formatted']) ?>', <?= $escaper->escapeHtml($item['count']) ?>,
17+
"<div style='padding: 1.5rem; line-height: 18px;'><strong style='font-size: 14px;'><?= $escaper->escapeHtml($item['formatted']) ?></strong><br/><br/>" +
18+
"<p>Searches: <strong style='color: #5468ff; padding-left: 3px;'><?= $escaper->escapeHtml($item['count']) ?></strong></p>" +
19+
"<p>Users: <strong style='color: #3a46a1; padding-left: 3px;'><?= $escaper->escapeHtml($item['users']) ?></strong></p>" +
20+
"<p>No Result Rate: <strong style='color: #3ab2bd; padding-left: 3px;'><?= $escaper->escapeHtml(round((int) $item['rate'] * 100, 2) . '%') ?></strong></p>" +
21+
<?php if (isset($item['ctr'])): ?>
2122
"<br/>" +
22-
"<p>CTR: <strong style='color: #5468ff; padding-left: 3px;'><?php echo round((int) $item['ctr'] * 100, 2) . '%' ?></strong></p>" +
23-
"<p>Conversion Rate: <strong style='color: #3a46a1; padding-left: 3px;'><?php echo round((int) $item['conversion'] * 100, 2) . '%' ?></strong></p>" +
24-
"<p>Avg Click Position: <strong style='color: #3ab2bd; padding-left: 3px;'><?php echo $item['clickPos'] ? $item['clickPos'] : '-' ?></strong></p>" +
23+
"<p>CTR: <strong style='color: #5468ff; padding-left: 3px;'><?= $escaper->escapeHtml(round((int) $item['ctr'] * 100, 2) . '%') ?></strong></p>" +
24+
"<p>Conversion Rate: <strong style='color: #3a46a1; padding-left: 3px;'><?= $escaper->escapeHtml(round((int) $item['conversion'] * 100, 2) . '%') ?></strong></p>" +
25+
"<p>Avg Click Position: <strong style='color: #3ab2bd; padding-left: 3px;'><?= $escaper->escapeHtml($item['clickPos'] ? $item['clickPos'] : '-') ?></strong></p>" +
2526
<?php endif ?>
2627
"</div>",
2728
'stroke-opacity: 0; fill-opacity: 0.75;'
@@ -57,8 +58,8 @@
5758
chart.draw(data, options);
5859
}
5960
</script>
60-
<?php else : ?>
61+
<?php else: ?>
6162
<div class="dashboard-diagram-nodata">
62-
<span><?php echo $block->escapeHtml(__('No Data Found')) ?></span>
63+
<span><?= $escaper->escapeHtml(__('No Data Found')) ?></span>
6364
</div>
64-
<?php endif ?>
65+
<?php endif ?>

0 commit comments

Comments
 (0)