From 32f5a14f29f5330cb2202d0805338b93f1d245e1 Mon Sep 17 00:00:00 2001 From: Bart Date: Tue, 14 Jul 2026 10:59:22 +0200 Subject: [PATCH 1/4] SPIN-5582: Strip html from database values. Keep html in specific renderer output. --- Util/Datatable.php | 31 +++++++++++++++++++++++++++++++ Util/Formatter/Renderer.php | 4 +--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/Util/Datatable.php b/Util/Datatable.php index d30096b..afb8e4a 100644 --- a/Util/Datatable.php +++ b/Util/Datatable.php @@ -16,6 +16,7 @@ use Ali\DatatableBundle\Util\Formatter\Renderer; use Ali\DatatableBundle\Util\Factory\Prototype\PrototypeBuilder; use Twig\Environment; +use Twig\Markup; class Datatable { @@ -174,7 +175,9 @@ public function execute() } if (!is_null($this->_renderer)) { + $raw_data = $data; array_walk($data, $this->_renderer); + $this->_markRendererOutputAsSafe($data, $raw_data); } if (!is_null($this->_renderer_obj)) { @@ -196,6 +199,34 @@ public function execute() return new JsonResponse($output); } + /** + * Column values untouched by the controller's ->setRenderer() closure are raw entity/DB + * data and must stay plain strings, so Twig (and any renderer view relying on Twig's + * escaping, e.g. _default.html.twig) auto-escapes them and prevents HTML/script injection. + * Values the closure *did* rewrite are trusted, deliberately-built HTML (links, badges, ...), + * so we wrap them in Twig\Markup - the same "already safe" marker the |raw filter produces - + * to stop that same auto-escaping from mangling them. + * + * @param array $data data after the renderer closure ran (modified in place) + * @param array $raw_data data as it was before the renderer closure ran + * + * @return void + */ + private function _markRendererOutputAsSafe(array &$data, array $raw_data) + { + foreach ($data as $row_index => &$row) + { + foreach ($row as $column_index => &$value) + { + $original = $raw_data[$row_index][$column_index] ?? null; + if (is_string($value) && $value !== $original) + { + $value = new Markup($value, 'UTF-8'); + } + } + } + } + /** * get datatable instance by id * return current instance if null diff --git a/Util/Formatter/Renderer.php b/Util/Formatter/Renderer.php index 219b29e..b6400f4 100644 --- a/Util/Formatter/Renderer.php +++ b/Util/Formatter/Renderer.php @@ -34,9 +34,7 @@ public function __construct(Environment $twig, array $renderers, array $fields) */ public function applyView($view_path, array $params) { - $out = $this->twig - ->render($view_path, $params); - return html_entity_decode($out); + return $this->twig->render($view_path, $params); } /** From 0134b756ab585387f4b1353e5bd41ca8c54a52f4 Mon Sep 17 00:00:00 2001 From: Bart Date: Tue, 28 Jul 2026 10:45:46 +0200 Subject: [PATCH 2/4] SPIN-5582: Now also escape HTML from database values when setRenderes isn't configured. * Now also works when only setRenderer is used * Now also works when no renderer is used --- Util/Datatable.php | 43 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/Util/Datatable.php b/Util/Datatable.php index afb8e4a..34f8f56 100644 --- a/Util/Datatable.php +++ b/Util/Datatable.php @@ -181,8 +181,18 @@ public function execute() } if (!is_null($this->_renderer_obj)) { + // ->setRenderers() always renders every column through Twig (falling back to + // _default.html.twig's {{ dt_item }}), so auto-escaping is guaranteed here. $this->_renderer_obj->applyTo($data, $objects); } + else + { + // No Twig pass is guaranteed to run (no ->setRenderers()), so nothing will + // auto-escape leftover raw entity/DB values (or a whole untouched row when + // ->setRenderer() isn't used either). Escape them now instead of relying on + // a pass that may never happen. + $this->_escapeUnsafeValues($data); + } if (!empty($this->_multiple)) { array_walk($data, function($val, $key) use(&$data, $ids) { @@ -201,11 +211,11 @@ public function execute() /** * Column values untouched by the controller's ->setRenderer() closure are raw entity/DB - * data and must stay plain strings, so Twig (and any renderer view relying on Twig's - * escaping, e.g. _default.html.twig) auto-escapes them and prevents HTML/script injection. + * data. They are left as plain strings here and escaped later, either by Twig + * (if ->setRenderers() is also configured, see execute()) or by _escapeUnsafeValues(). * Values the closure *did* rewrite are trusted, deliberately-built HTML (links, badges, ...), * so we wrap them in Twig\Markup - the same "already safe" marker the |raw filter produces - - * to stop that same auto-escaping from mangling them. + * so neither a later Twig pass nor _escapeUnsafeValues() mangles them. * * @param array $data data after the renderer closure ran (modified in place) * @param array $raw_data data as it was before the renderer closure ran @@ -227,6 +237,33 @@ private function _markRendererOutputAsSafe(array &$data, array $raw_data) } } + /** + * Escapes any plain string cell value so raw entity/DB data can never be interpreted as + * HTML/script by the client-side DataTables grid, which inserts aaData directly into the + * DOM. Used whenever no ->setRenderers() Twig pass is configured to guarantee escaping - + * i.e. when the table has no renderer at all, or only a ->setRenderer() closure that may + * leave some columns (or all rows, for columns it doesn't touch) unrendered. + * Values already wrapped in Twig\Markup by _markRendererOutputAsSafe() are deliberately + * built HTML and are left untouched. + * + * @param array $data data after any ->setRenderer() closure ran (modified in place) + * + * @return void + */ + private function _escapeUnsafeValues(array &$data) + { + foreach ($data as &$row) + { + foreach ($row as &$value) + { + if (is_string($value)) + { + $value = htmlspecialchars($value, ENT_QUOTES, 'UTF-8'); + } + } + } + } + /** * get datatable instance by id * return current instance if null From 58210bbb4e3940dacb18dc530c7ed394a61dd2df Mon Sep 17 00:00:00 2001 From: Bart Date: Wed, 29 Jul 2026 10:00:41 +0200 Subject: [PATCH 3/4] SPIN-5582: Now escapes the (possible) html set in the identifier in Datatable.php --- Util/Datatable.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Util/Datatable.php b/Util/Datatable.php index 34f8f56..f990ff3 100644 --- a/Util/Datatable.php +++ b/Util/Datatable.php @@ -196,7 +196,8 @@ public function execute() if (!empty($this->_multiple)) { array_walk($data, function($val, $key) use(&$data, $ids) { - array_unshift($val, ""); + $safe_id = htmlspecialchars((string) $ids[$key], ENT_QUOTES, 'UTF-8'); + array_unshift($val, ""); $data[$key] = $val; }); } From 35b42d3543c5836583d883dcd6718a3ee201985c Mon Sep 17 00:00:00 2001 From: Bart Date: Wed, 2 Sep 2026 16:44:15 +0200 Subject: [PATCH 4/4] SPIN-5582: Reduced double nested foreach loop. --- Util/Datatable.php | 58 ++++++++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/Util/Datatable.php b/Util/Datatable.php index f990ff3..7c0fab5 100644 --- a/Util/Datatable.php +++ b/Util/Datatable.php @@ -173,25 +173,30 @@ public function execute() array_unshift($data, $item); } } + $raw_data = $data; if (!is_null($this->_renderer)) { - $raw_data = $data; array_walk($data, $this->_renderer); - $this->_markRendererOutputAsSafe($data, $raw_data); } if (!is_null($this->_renderer_obj)) { // ->setRenderers() always renders every column through Twig (falling back to // _default.html.twig's {{ dt_item }}), so auto-escaping is guaranteed here. + // The ->setRenderer() closure's output (if any) still needs marking as safe, + // since Twig would otherwise re-escape the HTML it already built. + if (!is_null($this->_renderer)) + { + $this->_markRendererOutputAsSafe($data, $raw_data); + } $this->_renderer_obj->applyTo($data, $objects); } else { // No Twig pass is guaranteed to run (no ->setRenderers()), so nothing will // auto-escape leftover raw entity/DB values (or a whole untouched row when - // ->setRenderer() isn't used either). Escape them now instead of relying on - // a pass that may never happen. - $this->_escapeUnsafeValues($data); + // ->setRenderer() isn't used either). In a single pass: mark values the + // ->setRenderer() closure built as safe HTML, and escape everything else. + $this->_finalizeUnrenderedValues($data, $raw_data); } if (!empty($this->_multiple)) { @@ -212,11 +217,11 @@ public function execute() /** * Column values untouched by the controller's ->setRenderer() closure are raw entity/DB - * data. They are left as plain strings here and escaped later, either by Twig - * (if ->setRenderers() is also configured, see execute()) or by _escapeUnsafeValues(). - * Values the closure *did* rewrite are trusted, deliberately-built HTML (links, badges, ...), - * so we wrap them in Twig\Markup - the same "already safe" marker the |raw filter produces - - * so neither a later Twig pass nor _escapeUnsafeValues() mangles them. + * data, left as plain strings here and escaped later by Twig (->setRenderers() is + * configured whenever this method is called, see execute()). Values the closure *did* + * rewrite are trusted, deliberately-built HTML (links, badges, ...), so we wrap them in + * Twig\Markup - the same "already safe" marker the |raw filter produces - so the later + * Twig pass doesn't mangle them. * * @param array $data data after the renderer closure ran (modified in place) * @param array $raw_data data as it was before the renderer closure ran @@ -239,28 +244,35 @@ private function _markRendererOutputAsSafe(array &$data, array $raw_data) } /** - * Escapes any plain string cell value so raw entity/DB data can never be interpreted as - * HTML/script by the client-side DataTables grid, which inserts aaData directly into the - * DOM. Used whenever no ->setRenderers() Twig pass is configured to guarantee escaping - - * i.e. when the table has no renderer at all, or only a ->setRenderer() closure that may - * leave some columns (or all rows, for columns it doesn't touch) unrendered. - * Values already wrapped in Twig\Markup by _markRendererOutputAsSafe() are deliberately - * built HTML and are left untouched. + * Combines _markRendererOutputAsSafe() and the old _escapeUnsafeValues() into a single + * row/column pass, since every cell needs exactly one of those two treatments and doing + * them as two separate nested loops over the same data was pure overhead. For each string + * cell: if the ->setRenderer() closure changed it, it's deliberately built HTML, so wrap it + * in Twig\Markup; otherwise it's untouched entity/DB data and must be escaped so the + * client-side DataTables grid (which inserts aaData directly into the DOM) can't interpret + * it as HTML/script. Used whenever no ->setRenderers() Twig pass is configured to guarantee + * escaping - i.e. when the table has no renderer at all, or only a ->setRenderer() closure + * that may leave some columns (or all rows, for columns it doesn't touch) unrendered. * - * @param array $data data after any ->setRenderer() closure ran (modified in place) + * @param array $data data after any ->setRenderer() closure ran (modified in place) + * @param array $raw_data data as it was before the ->setRenderer() closure ran * * @return void */ - private function _escapeUnsafeValues(array &$data) + private function _finalizeUnrenderedValues(array &$data, array $raw_data) { - foreach ($data as &$row) + foreach ($data as $row_index => &$row) { - foreach ($row as &$value) + foreach ($row as $column_index => &$value) { - if (is_string($value)) + if (!is_string($value)) { - $value = htmlspecialchars($value, ENT_QUOTES, 'UTF-8'); + continue; } + $original = $raw_data[$row_index][$column_index] ?? null; + $value = $value !== $original + ? new Markup($value, 'UTF-8') + : htmlspecialchars($value, ENT_QUOTES, 'UTF-8'); } } }