Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UHF-10967: Patch core due to #3487031 #847

Merged
merged 3 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@
"[#UHF-9388] Process configuration translation files for custom modules (https://www.drupal.org/i/2845437)": "https://raw.githubusercontent.com/City-of-Helsinki/drupal-helfi-platform-config/fd68277191b8f8ec290e53b5fbbae699b2260384/patches/drupal-2845437-process-custom-module-translation-config-10.3.x.patch",
"[#UHF-9690] Allow updating lists when switching from allowed values to allowed values function (https://www.drupal.org/i/2873353)": "https://www.drupal.org/files/issues/2021-05-18/allow-allowed-values-function-update-D9-2873353_1.patch",
"[#UHF-9952, #UHF-9980] Duplicate <br /> tags (https://www.drupal.org/i/3083786)": "https://www.drupal.org/files/issues/2024-08-08/3083786--mr-8066--10-3-backport.patch",
"[#UHF-10716] Ensure consistent ordering when calculating library asset order (https://www.drupal.org/i/3467860)": "https://raw.githubusercontent.com/City-of-Helsinki/drupal-helfi-platform-config/955e2fc9493c6574ab070187b8a5a8634da7daab/patches/drupal-3467860-optimized-js-assets-mismatch.patch"
"[#UHF-10716] Ensure consistent ordering when calculating library asset order (https://www.drupal.org/i/3467860)": "https://raw.githubusercontent.com/City-of-Helsinki/drupal-helfi-platform-config/955e2fc9493c6574ab070187b8a5a8634da7daab/patches/drupal-3467860-optimized-js-assets-mismatch.patch",
"[#UHF-10967] Performance Degraded after update to twig 3.14.2 (https://www.drupal.org/project/drupal/issues/3487031)": "https://raw.githubusercontent.com/City-of-Helsinki/drupal-helfi-platform-config/90b44ce5a778d05bbe89f7eaca6412b7bd34efa0/patches/10177.patch"
},
"drupal/default_content": {
"https://www.drupal.org/project/default_content/issues/2640734#comment-14638943": "https://raw.githubusercontent.com/City-of-Helsinki/drupal-helfi-platform-config/main/patches/default_content_2.0.0-alpha2-2640734_manual_imports-e164a354.patch"
Expand Down
253 changes: 253 additions & 0 deletions patches/10177.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,253 @@
From 69b3409ec7356d8583b27ffcd54313084e07008c Mon Sep 17 00:00:00 2001
From: Lee Rowlands <[email protected]>
Date: Thu, 14 Nov 2024 04:54:37 +1000
Subject: [PATCH 1/3] Possible fix

---
.../RemoveCheckToStringNodeVisitor.php | 61 +++++++++++++++++++
.../Drupal/Core/Template/TwigExtension.php | 11 ++++
.../Template/TwigSimpleCheckToStringNode.php | 33 ++++++++++
3 files changed, 105 insertions(+)
create mode 100644 core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
create mode 100644 core/lib/Drupal/Core/Template/TwigSimpleCheckToStringNode.php

diff --git a/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php b/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
new file mode 100644
index 000000000000..7752eb6d6481
--- /dev/null
+++ b/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
@@ -0,0 +1,61 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Core\Template;
+
+use Twig\Environment;
+use Twig\Node\CheckToStringNode;
+use Twig\Node\Node;
+use Twig\NodeVisitor\NodeVisitorInterface;
+
+/**
+ * Defines a TwigNodeVisitor that replaces CheckToStringNodes.
+ *
+ * When Drupal's default SandboxPolicy is active, we set __toString as an
+ * allowed method. In which case, Twig's SandboxExtension need not check if
+ * __toString is allowed on every object.
+ */
+final class RemoveCheckToStringNodeVisitor implements NodeVisitorInterface {
+
+ public function __construct(protected bool $active) {
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function enterNode(Node $node, Environment $env): Node {
+ if (!$this->active) {
+ // __toString isn't an allowed method so we need to leave the existing
+ // CheckToStringNode in place.
+ return $node;
+ }
+ if ($node instanceof CheckToStringNode) {
+ // Replace this with the faster equivalent, __toString is an allowed
+ // methoed so any checking of __toString on a per object basis is
+ // performance overhead.
+ $new = new TwigSimpleCheckToStringNode($node->getNode('exprt'));
+ if ($node->hasAttribute('spread')) {
+ $new->setAttribute('spread', $node->getAttribute('spread'));
+ }
+ return $new;
+ }
+ return $node;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function leaveNode(Node $node, Environment $env): ?Node {
+ return $node;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getPriority() {
+ // Runs after sandbox visitor.
+ return 1;
+ }
+
+}
diff --git a/core/lib/Drupal/Core/Template/TwigExtension.php b/core/lib/Drupal/Core/Template/TwigExtension.php
index cd34aec44973..3a792a82854b 100644
--- a/core/lib/Drupal/Core/Template/TwigExtension.php
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -13,6 +13,7 @@
use Drupal\Core\Render\RenderableInterface;
use Drupal\Core\Render\RendererInterface;
use Drupal\Core\Routing\UrlGeneratorInterface;
+use Drupal\Core\Site\Settings;
use Drupal\Core\Theme\ThemeManagerInterface;
use Drupal\Core\Url;
use Twig\Environment;
@@ -158,9 +159,19 @@ public function getFilters() {
public function getNodeVisitors() {
// The node visitor is needed to wrap all variables with
// render_var -> TwigExtension->renderVar() function.
+ $methods = Settings::get('twig_sandbox_allowed_methods', [
+ // Only allow idempotent methods.
+ 'id',
+ 'label',
+ 'bundle',
+ 'get',
+ '__toString',
+ 'toString',
+ ]);
return [
new TwigNodeVisitor(),
new TwigNodeVisitorCheckDeprecations(),
+ new RemoveCheckToStringNodeVisitor(\in_array('__toString', $methods, TRUE)),
];
}

diff --git a/core/lib/Drupal/Core/Template/TwigSimpleCheckToStringNode.php b/core/lib/Drupal/Core/Template/TwigSimpleCheckToStringNode.php
new file mode 100644
index 000000000000..42f32a5d4694
--- /dev/null
+++ b/core/lib/Drupal/Core/Template/TwigSimpleCheckToStringNode.php
@@ -0,0 +1,33 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Core\Template;
+
+use Twig\Compiler;
+use Twig\Node\CheckToStringNode;
+
+/**
+ * Defines a twig node for simplifying CheckToStringNode.
+ *
+ * Drupal's sandbox policy is very permissive with checking whether an object
+ * can be converted to a string. We allow any object with a __toString method.
+ * This means that the array traversal in the default SandboxExtension
+ * implementation added by the parent class is a performance overhead we don't
+ * need.
+ *
+ * @see \Drupal\Core\Template\TwigSandboxPolicy
+ * @see \Drupal\Core\Template\RemoveCheckToStringNodeVisitor
+ */
+final class TwigSimpleCheckToStringNode extends CheckToStringNode {
+
+ /**
+ * {@inheritdoc}
+ */
+ public function compile(Compiler $compiler): void {
+ $expr = $this->getNode('expr');
+ $compiler
+ ->subcompile($expr);
+ }
+
+}
--
GitLab


From 34f891288e46226330d853dd358f04e3f5fe0852 Mon Sep 17 00:00:00 2001
From: Lee Rowlands <[email protected]>
Date: Thu, 14 Nov 2024 05:16:04 +1000
Subject: [PATCH 2/3] Lint

---
.../Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php b/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
index 7752eb6d6481..762a992fa136 100644
--- a/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
+++ b/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
@@ -26,15 +26,15 @@ public function __construct(protected bool $active) {
*/
public function enterNode(Node $node, Environment $env): Node {
if (!$this->active) {
- // __toString isn't an allowed method so we need to leave the existing
+ // __toString isn't an allowed method, so we need to leave the existing
// CheckToStringNode in place.
return $node;
}
if ($node instanceof CheckToStringNode) {
// Replace this with the faster equivalent, __toString is an allowed
- // methoed so any checking of __toString on a per object basis is
+ // method so any checking of __toString on a per-object basis is
// performance overhead.
- $new = new TwigSimpleCheckToStringNode($node->getNode('exprt'));
+ $new = new TwigSimpleCheckToStringNode($node->getNode('expr'));
if ($node->hasAttribute('spread')) {
$new->setAttribute('spread', $node->getAttribute('spread'));
}
--
GitLab


From 67bbb31276e5f50411f5b09979d8055791d1fc32 Mon Sep 17 00:00:00 2001
From: Lee Rowlands <[email protected]>
Date: Thu, 14 Nov 2024 05:36:35 +1000
Subject: [PATCH 3/3] Cleaner approach

---
.../Core/Template/RemoveCheckToStringNodeVisitor.php | 7 +------
core/lib/Drupal/Core/Template/TwigExtension.php | 11 +++++++++--
2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php b/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
index 762a992fa136..65076b2f2b0e 100644
--- a/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
+++ b/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
@@ -18,18 +18,13 @@
*/
final class RemoveCheckToStringNodeVisitor implements NodeVisitorInterface {

- public function __construct(protected bool $active) {
+ public function __construct() {
}

/**
* {@inheritdoc}
*/
public function enterNode(Node $node, Environment $env): Node {
- if (!$this->active) {
- // __toString isn't an allowed method, so we need to leave the existing
- // CheckToStringNode in place.
- return $node;
- }
if ($node instanceof CheckToStringNode) {
// Replace this with the faster equivalent, __toString is an allowed
// method so any checking of __toString on a per-object basis is
diff --git a/core/lib/Drupal/Core/Template/TwigExtension.php b/core/lib/Drupal/Core/Template/TwigExtension.php
index 3a792a82854b..255da96c4e78 100644
--- a/core/lib/Drupal/Core/Template/TwigExtension.php
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -168,11 +168,18 @@ public function getNodeVisitors() {
'__toString',
'toString',
]);
- return [
+ $visitors = [
new TwigNodeVisitor(),
new TwigNodeVisitorCheckDeprecations(),
- new RemoveCheckToStringNodeVisitor(\in_array('__toString', $methods, TRUE)),
];
+ if (\in_array('__toString', $methods, TRUE)) {
+ // When __toString is an allowed method, there is no point in running
+ // \Twig\Extension\SandboxExtension::ensureToStringAllowed, so we add a
+ // node visitor to remove any CheckToStringNode nodes added by the
+ // sandbox extension.
+ $visitors[] = new RemoveCheckToStringNodeVisitor();
+ }
+ return $visitors;
}

/**
--
GitLab