Follow-up to #2506133: Replace SafeMarkup::set() in \Drupal\Core\Template\Attribute

Problem/Motivation

#2506133: Replace SafeMarkup::set() in \Drupal\Core\Template\Attribute has overlooked a fairly common use case, which now gets double escaped when translated titles are used as attributes.

Translated attribute values.

Example in core: feed_icon

$variables['attributes']['title'] = t('Message: @escaped', ['@escaped' => '<>']);

Will escape it once in t, if not marked safe, once in attributes, then once more in twig because the string changed in attributes.

If we mark it safe it still gets escaped at least twice...

Proposed resolution

Avoid htmlspecialchars() escaping in attribute if it's marked safe?

Remaining tasks

Review & commit

User interface changes

None

API changes

N/A

CommentFileSizeAuthor
#5 attribute_class_to-2531824-5.patch5.46 KBjoelpittet
#5 interdiff.txt3.72 KBjoelpittet
#5 attribute_class_to-2531824-5-tests-only.patch2.95 KBjoelpittet
#3 attribute_class_to-2531824-3.patch2.42 KBjoelpittet

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Title: Replace SafeMarkup::set() in \Drupal\Core\Template\Attribute » Attribute class to check safe strings before escaping
Status: Needs work » Active
Related issues: +#2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: +Needs tests

AttributeValueBase also needs to be implementing SafeStringInterface because it's values can be printed directly. This should get a test to ensure this.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
StatusFileSize
new attribute_class_to-2531824-3.patch2.42 KB

Here's the fix, now needs some tests.

Status: Needs review » Needs work

The last submitted patch, 3: attribute_class_to-2531824-3.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: -Needs tests
StatusFileSize
new attribute_class_to-2531824-5-tests-only.patch2.95 KB
new interdiff.txt3.72 KB
new attribute_class_to-2531824-5.patch5.46 KB

I expect this will still fail but now it has tests to help aspire to (may need love).

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: attribute_class_to-2531824-5.patch, failed testing.

The last submitted patch, 5: attribute_class_to-2531824-5-tests-only.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Title: Attribute class to check safe strings before escaping » Attribute class to check safe strings before escaping (has tests)

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.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/lib/Drupal/Core/Template/AttributeString.php
@@ -28,7 +30,13 @@ class AttributeString extends AttributeValueBase {
+    if (SafeMarkup::isSafe($this->value)) {

This is deprecated - just need to check if it is MarkupInterface

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.

nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
commented
Assigned: Unassigned » nikunjkotecha
Issue tags: +#dcdelhi
nikunjkotecha’s picture

nikunjkotecha
he/him
Primary language Gujarati
Location India, Gujarat, Rajkot
commented
Assigned: nikunjkotecha » Unassigned
Issue tags: -#dcdelhi

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.

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.

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should 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.

smustgrave’s picture

smustgrave commented

Is this still relevant? I see htmlspecialchars was already replaced by something like Html::escape((string) $this->value);

smustgrave’s picture

smustgrave commented
Status: Needs work » Postponed (maintainer needs more info)

Based on comment #25

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

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should 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.

quietone’s picture

quietone commented
Status: Postponed (maintainer needs more info) » Closed (outdated)
Issue tags: +Bug Smash Initiative

The patch is added tests for escaping Attributes. Since this was create the two test \Drupal\Tests\Core\Template\AttributeTest::testAttributeValues and \Drupal\Tests\Core\Template\TwigExtensionTest::testSafeStringEscaping have been added which are doing the same testing.

Based on that and there has been no response for more information, I am closing this issue.

#2506133: Replace SafeMarkup::set() in \Drupal\Core\Template\Attribute
#2571673: Convert Views t() usage where it is used as an attribute value