Problem/Motivation

There is no render #thingie the behaves like twig.

  • #markup admin filters if not safe
  • #plain_text always escapes regardless of safety
  • Twig will escape if not safe

This means that issues like #2567159: Fix improper usage of t() in ViewsBlock are resorting to inline templates to get this behaviour. Funnily enough we use to be able to flip #markup between filtering and escaping but that got changed in #2555931: Add #plain_text to escape text in render arrays. I think the rationale behind that change was good since

$render_array = ['#plain_text' => SafeString::create('<em>I win</em>')];

looks awful.

But

$render_array = ['#plain_text' => t('@thing: @subthing', $placeholders)];

the fact that this will double escape is also awful.

Proposed resolution

Not sure. We could make #plain_text use the inbuilt double escaping prevention in htmlspecialchars() or maybe once t() and SafeMarkup::format return objects we could glean information as to how they are made safe.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#28 2568045-9.x-27.patch9.1 KBalexpott
#28 2568045-9.x-27-test-only.patch1.16 KBalexpott
#27 2568045-9.x-27.patch9.1 KBalexpott
#15 2568045-review.txt2.01 KBdawehner
#15 interdiff.txt1.14 KBdawehner
#15 2568045-15.patch29.7 KBdawehner
#3 2568045.3.patch1.73 KBalexpott

Issue fork drupal-2568045

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2568045.3.patch1.73 KB

Here is the simplest fix.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

So one problem with this approach is that it should be allowed for translators to add html elements like bdi and dir see http://www.w3.org/International/questions/qa-html-dir for more.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 3: 2568045.3.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -790,7 +790,9 @@ protected function ensureMarkupIsSafe(array $elements) {
-      $elements['#markup'] = SafeString::create(Html::escape($elements['#plain_text']));
+      // Prevent double escaping by calling htmlspecialchars() directly with
+      // $double_encode set to FALSE.
+      $elements['#markup'] = SafeString::create(htmlspecialchars($elements['#plain_text'], ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8', FALSE));

Should we make that an option on Html::escape?

The last submitted patch, 3: 2568045.3.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Should we make that an option on Html::escape?

I think an explicit method (on top of that) would be better DX, something like HTML::escapeUnsafe().

dawehner’s picture

dawehner
Primary language German
commented

I think an explicit method (on top of that) would be better DX, something like HTML::escapeUnsafe().

Agreed!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

So the filter admin page relies on double escaping :)

effulgentsia’s picture

effulgentsia commented

it should be allowed for translators to add html elements like bdi and dir

Exactly. I think we should postpone this on #2509218: Ensure that SafeString objects can be used in non-HTML contexts and then do:

'#plain_text' => t('@thing: @subthing', $placeholders)->getTranslationForNonHtml();

in places where we want translated plain-text strings.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

t('@thing: @subthing', $placeholders)->getTranslationForNonHtml();

Offtopic: Yes yes double yes. not that method name but yes:) CLI or email can be smart about rendered context. Value objects FTW and PHP shortcomings be-damned!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Hitting this in #2513534: Remove the 'disabled' region from Block UI, this patch fixes my issue.

My workaround is to use #markup with t():

diff --git a/core/modules/block/src/BlockListBuilder.php b/core/modules/block/src/BlockListBuilder.php
index 95868db..4607e33 100644
--- a/core/modules/block/src/BlockListBuilder.php
+++ b/core/modules/block/src/BlockListBuilder.php
@@ -265,12 +265,14 @@ protected function buildBlocksForm() {
             $form[$entity_id]['#attributes']['class'][] = 'color-success';
             $form[$entity_id]['#attributes']['class'][] = 'js-block-placed';
           }
-          $form[$entity_id]['info'] = array(
-            '#plain_text' => $info['status'] ? $info['label'] : $this->t('@label (disabled)', ['@label' => $info['label']]),
-            '#wrapper_attributes' => array(
-              'class' => array('block'),
-            ),
-          );
+          if ($info['status']) {
+            $form[$entity_id]['info']['#plain_text'] = $info['label'];
+          }
+          else {
+            $form[$entity_id]['info']['#markup'] = $this->t('@label (disabled)', ['@label' => $info['label']]);
+          }
+          $form[$entity_id]['info']['#wrapper_attributes']['class'][] = 'block';
+
           $form[$entity_id]['type'] = array(
             '#markup' => $info['category'],
           );
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2568045-15.patch29.7 KB
new interdiff.txt1.14 KB
new 2568045-review.txt2.01 KB

This is a patch on top of #2575615: Introduce HtmlEscapedText and remove SafeMarkup::setMultiple() and SafeMarkup::getAll() and remove the static safeStrings list

Status: Needs review » Needs work

The last submitted patch, 15: 2568045-15.patch, failed testing.

The last submitted patch, 15: 2568045-15.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mjpa’s picture

mjpa commented

Following, although probably pointlessly, as I've come across this too and the work around in #14 just looks ugly...

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.9.x-dev » 9.2.x-dev
Category: Task » Bug report
Priority: Normal » Major
Status: Needs work » Needs review
StatusFileSize
new 2568045-9.x-27.patch9.1 KB

New approach that passes FilterAdminTest, doesn't change behaviour in an unexpected way - ie. RendererTest passes with no changes and also fixes #3199730: Views block description is double-escaped if display name is set.

No interdiff because it has been a while and re-classifying as a major bug because it's very very hard to not double escape when you do something like:

          $form[$entity_id]['info'] = [
            '#plain_text' => $info['status'] ? $info['label'] : $this->t('@label (disabled)', ['@label' => $info['label']]),
            '#wrapper_attributes' => [
              'class' => ['block'],
            ],
          ];

Any & in $info['label'] will be double escaped if a block is disabled.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2568045-9.x-27-test-only.patch1.16 KB
new 2568045-9.x-27.patch9.1 KB

With only the addition to RendererTest applied the test shows exactly what is happening on #3199730: Views block description is double-escaped if display name is set

1) Drupal\Tests\Core\Render\RendererTest::testRenderBasic with data set #12 (array(Drupal\Component\Render\FormattableMarkup Object (...)), '&lt;em&gt;foo &amp; bar&lt;/em&gt;')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'&lt;em&gt;foo &amp; bar&lt;/em&gt;'
+'&lt;em&gt;foo &amp;amp; bar&lt;/em&gt;'

The last submitted patch, 28: 2568045-9.x-27-test-only.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#275308: Make check_plain() reentrant

@catch pointed me to #275308: Make check_plain() reentrant which is pretty similar - but I don't think it is quite the same because in D8/9 we have MarkupInterface so we know when we're dealing with one of these objects.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -735,7 +735,12 @@ protected function xssFilterAdminIfUnsafe($string) {
+    if (isset($elements['#plain_text']) && $elements['#plain_text'] instanceof MarkupInterface) {
+      // Prevent double escaping due to escaping in
+      // \Drupal\Component\Render\MarkupInterface::__toString().
+      $elements['#markup'] = Markup::create(Html::escape(Html::decodeEntities($elements['#plain_text'])));

Is it safe to push the check inside Html::escape(), so any caller that might pass in a MarkupInterface benefits from this fix?

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

knurg’s picture

knurg commented

Any progress on this? Whenever I do rewriting manually on views via "a href" and tokens I end up having this issue... it is pretty severe for me and worked pretty well before Drupal 9 :/

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Issue tags: +Needs Review Queue Initiative

@alexpott should a change be made for #31?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

We could try #31 but I worry about things like the filter admin page which need things double escaped. I think that #plain_text indicates an intention beyond other places where Html::escape() is called so the patch in #28 is correct but moving this to Html::escape() could have unintended impacts.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Wonder if the issue summary could be updated though.

The proposed solution, correct me if I'm wrong, doesn't seem to match up with the solution in the patch.

Solution wise though this looks good.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley’s picture

acbramley commented
Issue tags: +Needs reroll, +Bug Smash Initiative

This was triaged as part of BSI. At the very least needs a reroll onto an MR.

prem suthar made their first commit to this issue’s fork.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.