From 70055ad08db85ffe0454b9526c5da43e704bac16 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 17 Sep 2026 04:03:51 +0200 Subject: [PATCH] fix: a data attribute is escaped like everything else the theme renders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight places in four templates emitted data-="" out of a DataGridAction::getData()/getRuntimeData() map with no escaping at all. Nothing leaks today — every current addData() passes an ACL route or an integer id, and FileGrid's item-type must match the administrator's own MIME allow-list exactly — but nothing in the templates or in DataGridActionBase constrains what a caller may pass, and one addData() carrying a name, a login or a note breaks straight out of the attribute. data-onclick was unescaped the same way. ThemeEscapesWhatItRendersTest decides what carries text by the getter's name or by $_getvar(, so a bare $dataValue in a loop matched neither: the rule that holds the rest of the theme to escaping-on-output had a blind spot exactly where a caller-supplied map is rendered. aDataAttributeMapIsEscaped() closes it narrowly — only the loops whose attribute name is itself a variable, which are precisely the ones taking an arbitrary map. Writing the guard found more than the review did: it named three sites, and the scan found five more, in _partials/fixed-header.inc and grid/datagrid-grid.inc. --- .../views/_partials/fixed-header.inc | 4 +- .../views/account/search-rows.inc | 10 +++- .../views/grid/datagrid-grid.inc | 6 +- .../views/grid/datagrid-rows.inc | 8 ++- .../View/ThemeEscapesWhatItRendersTest.php | 56 +++++++++++++++++++ 5 files changed, 75 insertions(+), 9 deletions(-) diff --git a/public/themes/material-blue/views/_partials/fixed-header.inc b/public/themes/material-blue/views/_partials/fixed-header.inc index 4b4bd9a82..b607e2819 100644 --- a/public/themes/material-blue/views/_partials/fixed-header.inc +++ b/public/themes/material-blue/views/_partials/fixed-header.inc @@ -34,7 +34,7 @@ use function SP\__; getData() as $dataName => $dataValue): ?> - + > getIcon()->getIcon(); ?> @@ -102,7 +102,7 @@ use function SP\__; getData() as $dataName => $dataValue): ?> - + > getIcon()->getIcon(); ?> getTitle()); ?> diff --git a/public/themes/material-blue/views/account/search-rows.inc b/public/themes/material-blue/views/account/search-rows.inc index 88334971b..a1fe8710e 100644 --- a/public/themes/material-blue/views/account/search-rows.inc +++ b/public/themes/material-blue/views/account/search-rows.inc @@ -421,8 +421,14 @@ $favoriteRouteOff = $_getvar('favoriteRouteOff'); data-parent-id="getParentId(); ?>" getData() as $dataName => $dataValue): - printf('data-%s="%s"', $dataName, $dataValue); + printf('data-%s="%s"', $dataName, $_e($dataValue)); endforeach; ?>> getIcon()->getIcon(); ?> @@ -459,7 +465,7 @@ $favoriteRouteOff = $_getvar('favoriteRouteOff'); echo $accountSearchData->getParentId(); ?>" getData() as $dataName => $dataValue): - printf('data-%s="%s"', $dataName, $dataValue); + printf('data-%s="%s"', $dataName, $_e($dataValue)); endforeach; ?>> getData() as $dataName => $dataValue - ): echo 'data-', $dataName, '=', '"', $dataValue, '"'; endforeach; ?>> + ): echo 'data-', $dataName, '=', '"', $_e($dataValue), '"'; endforeach; ?>> getIcon()->getIcon(); ?> @@ -90,7 +90,7 @@ $index = $index ?? 0; getData() as $dataName => $dataValue - ): echo 'data-', $dataName, '=', '"', $dataValue, '"'; endforeach; ?>> + ): echo 'data-', $dataName, '=', '"', $_e($dataValue), '"'; endforeach; ?>>
=""` out of + * `DataGridAction::getData()` / `getRuntimeData()`, where the value is a bare `$dataValue` — + * invisible to that rule, and unescaped. What reaches them today is routes and integer ids, so + * nothing was leaking; but nothing in the templates or in `DataGridActionBase` constrains it, + * and a single `addData()` call carrying a name, a login or a note would break straight out of + * the attribute. + * + * This is narrower than the rule above on purpose: it looks only at the loops whose attribute + * *name* is itself a variable, which are exactly the ones taking an arbitrary map from a + * caller. A fixed `data-item-id=""` is an id by construction and is left to + * the rule above. + */ + #[Test] + #[DataProvider('templates')] + public function aDataAttributeMapIsEscaped(string $template): void + { + $source = (string)file_get_contents($template); + + $unescaped = []; + + // printf('data-%s="%s"', $name, $value) and echo 'data-', $name, '=', '"', $value, '"' + // The quote characters are matched with `.` rather than written into the class, which + // keeps this pattern readable inside a single-quoted PHP string. + $patterns = [ + '/printf\(\s*.data-%s="%s".\s*,(?P[^;]*?)\);/s', + '/echo\s+.data-.\s*,(?P[^;]*?);/s', + ]; + + foreach ($patterns as $pattern) { + preg_match_all($pattern, $source, $matches, PREG_OFFSET_CAPTURE); + + foreach ($matches['args'] as [$args, $offset]) { + if (str_contains($args, '$_e(')) { + continue; + } + + $unescaped[] = sprintf( + '%s:%d %s', + basename($template), + substr_count(substr($source, 0, $offset), "\n") + 1, + trim(preg_replace('/\s+/', ' ', $args)) + ); + } + } + + self::assertSame( + [], + $unescaped, + "a data attribute built from a caller-supplied map went out unescaped:\n" . implode("\n", $unescaped) + ); + } + /** * Inside a `