Follow-up to #2280965: [meta] Remove every SafeMarkup::set() call

Field has a funky trait that uses SafeMarkup::set() this was documented in #2501441: Document SafeMarkup::set in AllowedTagsXssTrait::fieldFilterXss but we have new tools because of #2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString and even with the SafeMarkup::set() it is not going to work because text transformations are applied to the result.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
19.49 KB

The solution could do with the new render array capabilities added by #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter

Also gonna to add test coverage for the transformations and the current broken-ness in HEAD.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
    @@ -0,0 +1,113 @@
    + * Defines an object that passes safe strings through the Filter system.
    

    s/Filter/Field/

  2. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
    @@ -0,0 +1,113 @@
    +   * Returns a list of tags allowed by AllowedTagsXssTrait::fieldFilterXss().
    

    C/P remnant.

  3. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
    @@ -0,0 +1,113 @@
    +    return array('a', 'b', 'big', 'code', 'del', 'em', 'i', 'ins',  'pre', 'q', 'small', 'span', 'strong', 'sub', 'sup', 'tt', 'ol', 'ul', 'li', 'p', 'br', 'img');
    

    Nit: [].

  4. +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
    @@ -48,7 +49,8 @@ public function viewElements(FieldItemListInterface $items) {
    +        // @todo convert to markup using the tag list.
    
    +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsKeyFormatter.php
    @@ -35,7 +35,8 @@ public function viewElements(FieldItemListInterface $items) {
    +      // @todo convert to markup using the tag list.
    

    These are the two only places that will use #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter. The IS made it sound like many places would use what's introduced there.

  5. +++ b/core/modules/options/src/Plugin/views/argument/StringListField.php
    @@ -80,12 +81,14 @@ public function summaryName($data) {
    +      // Filtered an escaped string will not change it.
    
    +++ b/core/modules/views/src/Plugin/views/argument/ListString.php
    @@ -72,12 +73,14 @@ public function summaryName($data) {
    +      // Filtered an escaped string will not change it.
    

    Parse error.

Status: Needs review » Needs work

The last submitted patch, 2: 2551511.2.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
11.33 KB
19.51 KB

@Wim Leers is awesome.

Status: Needs review » Needs work

The last submitted patch, 5: 2551511.5.patch, failed testing.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
19.75 KB
904 bytes

Fixed fatal error in Drupal\options\Tests\OptionsFieldUITest.

Status: Needs review » Needs work

The last submitted patch, 7: remove-2551511-7.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
21.09 KB

Just getting this green again for the moment.

edit:

+++ b/core/modules/file/file.field.inc
@@ -7,8 +7,10 @@
 use Drupal\Component\Utility\Html;

and this is unused now as well

stefan.r’s picture

And some nits:

  1. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
    @@ -0,0 +1,113 @@
    +    return ['a', 'b', 'big', 'code', 'del', 'em', 'i', 'ins',  'pre', 'q', 'small', 'span', 'strong', 'sub', 'sup', 'tt', 'ol', 'ul', 'li', 'p', 'br', 'img'];
    

    Double space after ins

  2. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
    @@ -0,0 +1,113 @@
    +   * @param $option
    +   * @return $this|static
    +   *   If the case transformation does not affect the string the same object is
    

    Missing documentation, $this is not a type

  3. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
    @@ -0,0 +1,113 @@
    +        break;
    ...
    +        break;
    ...
    +        break;
    

    Missing newlines

Wim Leers’s picture

Status: Needs review » Needs work
RavindraSingh’s picture

Addressed the points by @stefan.r in #9 #10.

+++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
@@ -0,0 +1,113 @@
+   * @param $option
+   * @return $this|static
+   *   If the case transformation does not affect the string the same object is

Missing documentation, $this is not a type but its getting returned as a safe String.

RavindraSingh’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Title: Remove SafeMarkup::set() from AllowedTagsXssTrait » [PP-1] Remove SafeMarkup::set() from AllowedTagsXssTrait
Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
    @@ -0,0 +1,113 @@
    +  /**
    +   * @param $option
    +   * @return $this|static
    +   *   If the case transformation does not affect the string the same object is
    +   *   returned. If it does, then a new object is returned, preserving
    +   *   immutability.
    +   */
    

    Docs need to be improved, as #12 also said.

  2. +++ b/core/modules/file/file.field.inc
    @@ -7,8 +7,10 @@
    +
     /**
    

    Unnecessary whitespace addition.

  3. +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
    @@ -48,7 +49,8 @@ public function viewElements(FieldItemListInterface $items) {
    +        // @todo convert to markup using the tag list.
    +        $elements[$delta] = array('#markup' => FieldFilteredString::create($output));
    
    +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsKeyFormatter.php
    @@ -35,7 +36,8 @@ public function viewElements(FieldItemListInterface $items) {
    +      // @todo convert to markup using the tag list.
    +      $elements[$delta] = array('#markup' => FieldFilteredString::create($item->value));
    

    I guess these make this issue blocked on #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter? This can still continue in the mean time though.

  4. Finally, tests are needed for the caseTransform() stuff, per #2.
xjm’s picture

Title: [PP-1] Remove SafeMarkup::set() from AllowedTagsXssTrait » Remove SafeMarkup::set() from AllowedTagsXssTrait

Status: Needs work » Needs review

stefan.r queued 12: remove-2551511-9.patch for re-testing.

Mariano’s picture

Assigned: Unassigned » Mariano

Looking at this as part of the Acquia Build Week Hackathon

Mariano’s picture

Addressed comments on #14.

xjm’s picture

Assigned: Mariano » Unassigned
FileSize
4.16 KB

Thanks @Mariano!

Note that it's helpful to provide interdiffs when updating existing patches in the d.o patch-based workflow to make it easier to review the changes. I've attached one here. :)

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
  1. +++ b/core/tests/Drupal/Tests/Core/Field/FieldFilteredStringTest.php
    @@ -0,0 +1,101 @@
    +  public function testCreate($string, $expected, $instance_of_check = FALSE) {
    ...
    +    $data[] = [
    +      '', '',
    +    ];
    

    The gain of making the third parameter optional is very small. Let's just be explicit and require FALSE to be specified?

  2. +++ b/core/tests/Drupal/Tests/Core/Field/FieldFilteredStringTest.php
    @@ -0,0 +1,101 @@
    +    $data[] = [
    +      'teststring', 'teststring', TRUE
    +    ];
    

    This looks a bit strange. Let's do either:

    $data[] = [$x, $y];
    

    or

    $data[] = [
      $x,
      $y,
    ];
    

    But not this in-between variant. Either of the two would be consistent with the rest of core.

  3. +++ b/core/tests/Drupal/Tests/Core/Field/FieldFilteredStringTest.php
    @@ -0,0 +1,101 @@
    +
    +  /**
    +   * @covers: ::allowedTags
    +   */
    +  public function testAllowedTags() {
    +    $expected = ['a', 'b', 'big', 'code', 'del', 'em', 'i', 'ins',  'pre', 'q', 'small', 'span', 'strong', 'sub', 'sup', 'tt', 'ol', 'ul', 'li', 'p', 'br', 'img'];
    +
    +    $this->assertSame($expected, FieldFilteredString::allowedTags());
    +  }
    +
    +  /**
    +   * @covers: ::displayAllowedTags
    +   */
    +  public function testdisplayAllowedTags() {
    +    $expected = '<a> <b> <big> <code> <del> <em> <i> <ins> <pre> <q> <small> <span> <strong> <sub> <sup> <tt> <ol> <ul> <li> <p> <br> <img>';
    +
    +    $this->assertSame($expected, FieldFilteredString::displayAllowedTags());
    +  }
    

    This is just asserting the same exact hardcoded list. That's not worth testing.

  4. +++ b/core/tests/Drupal/Tests/Core/Field/FieldFilteredStringTest.php
    @@ -0,0 +1,101 @@
    +  public function testCaseTransform($option, $test_string, $expected) {
    ...
    +  public function providertestCaseTransform() {
    

    Perfect :)

Wim Leers’s picture

Issue tags: +Needs reroll
Anonymous’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.31 KB
24.21 KB

The patch from #18 still applied.

Fixed #20, except for 3. The first tests an array, the other one a concatenated string (which uses that array). That is not the exact same thing. However, if you feel strongly about that, I'll remove it.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Fair point. I don't feel very strongly about it. Let's let a committer decide then :)

Dear committer, see #20.3 — I think those are useless tests, especially testAllowedTags(). If you think this is useful, keep it, if you think it's not, remove them.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.95 KB
21.89 KB

I realised that FieldFilteredString::caseTransform is completely unnecessary... less code! We can just do the case transforming before the filtering.

alexpott’s picture

Improved the FieldFilteredStringTest to actually test the filtering and normalising FieldFilteredString::create() does.

And I agree with @Wim Leers - testAllowedTags() is pretty pointless.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Excellent! Just one thing:

+++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
@@ -48,7 +49,8 @@ public function viewElements(FieldItemListInterface $items) {
+        // @todo convert to markup using the tag list.

+++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsKeyFormatter.php
@@ -35,7 +36,8 @@ public function viewElements(FieldItemListInterface $items) {
+      // @todo convert to markup using the tag list.

This @todo is still left and can now actually be done.

(I forgot about that when RTBC'ing in #23 — sorry.)

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.79 KB
20.35 KB

Ah yes... in fact here we don't need to even create the object - we can just use #allowed_tags for late filtering.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Yep, exactly :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: 2551511.27.patch, failed testing.

Wim Leers’s picture

Failing on:

    $this->assertRaw('<strong>Test with an upload field.</strong>');

This should actually be escaped AFAICT?

As should the next assertion (which is not failing because the test already failed on the above:

    $this->assertRaw('<em>Test with a non upload field.</em>');
alexpott’s picture

I worked off an old branch... alexpott--

Here's the correct patch.

Wim Leers’s picture

Status: Needs work » Needs review

Such noob :P

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

back to RTBC

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Field/FieldFilteredString.php
@@ -0,0 +1,80 @@
+ *
+ * @internal
+ *   This object is marked as internal because it should only be used in the
+ *   Filter module on strings that have already been been filtered and sanitized
+ *   in \Drupal\filter\Plugin\FilterInterface.
+ *
+ * @see \Drupal\Core\Render\SafeString
+ */
+final class FieldFilteredString implements SafeStringInterface, \Countable {
+  use SafeStringTrait;
+

These docs are copy and pasted from the filter tips patch I think?

Needs updating, and also needs to apply to the usages across field modules and views, which makes this a bit less @internal.

Wim Leers’s picture

which makes this a bit less @internal.

Hrm good point. It's still "relatively" @internal to the Field module, but given that it's now used by the options, image and views module, that's indeed less the case. It's still all places where "Field-specific stuff" is happening.

Wondering what Alex Pott thinks about this.

alexpott’s picture

So perhaps we need to update the SafeStringInterface documentation since if you filter on create (like this implementation) it is way less dangerous and less deserve of the internal-ness.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.55 KB
22.86 KB

So I've tried to completely remove the need for FieldFilteredString but we got three problems:

  • template_preprocess_file_upload_help / file-upload-help.html.twig is tricky and ugly
  • The use of FieldFilteredString to display allowed values in both the formatters and views summary values.
  • The super interesting code in NumericFormatterBase
          // Account for prefix and suffix.
          if ($this->getSetting('prefix_suffix')) {
            $prefixes = isset($settings['prefix']) ? array_map(array('Drupal\Core\Field\FieldFilteredString', 'create'), explode('|', $settings['prefix'])) : array('');
            $suffixes = isset($settings['suffix']) ? array_map(array('Drupal\Core\Field\FieldFilteredString', 'create'), explode('|', $settings['suffix'])) : array('');
            $prefix = (count($prefixes) > 1) ? $this->formatPlural($item->value, $prefixes[0], $prefixes[1]) : $prefixes[0];
            $suffix = (count($suffixes) > 1) ? $this->formatPlural($item->value, $suffixes[0], $suffixes[1]) : $suffixes[0];
            $output = $prefix . $output . $suffix;
          }
    

This SafeStringInterface is way safer than the other SafeStringInterface objects because it filters on create.

Given the above I think we should proceed here with better documentation. Especially since the mixing of filtered and escaped strings in SafeMarkup is definitely asking for security issues at the worst and unexpected behaviour at the very least.

Wim Leers’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/NumericFormatterBase.php
@@ -75,8 +75,8 @@ public function viewElements(FieldItemListInterface $items) {
-        $prefixes = isset($settings['prefix']) ? array_map(array($this, 'fieldFilterXss'), explode('|', $settings['prefix'])) : array('');
-        $suffixes = isset($settings['suffix']) ? array_map(array($this, 'fieldFilterXss'), explode('|', $settings['suffix'])) : array('');
+        $prefixes = isset($settings['prefix']) ? array_map(array('Drupal\Core\Field\FieldFilteredString', 'create'), explode('|', $settings['prefix'])) : array('');
+        $suffixes = isset($settings['suffix']) ? array_map(array('Drupal\Core\Field\FieldFilteredString', 'create'), explode('|', $settings['suffix'])) : array('');

Wow, good catch. Why was this not causing test failures before?

Wim Leers’s picture

stefan_r answered #38 in IRC:

12:59:39 <stefan_r> as to the AllowedXssTrait one, why would the previous code trigger a test failure? it's essentially the same, except instead of calling the wrapper we call the method directly
13:01:26 <stefan_r> ie AllowedXssTrait::fieldFilterXss() just wraps FieldFilteredString::create()

Of course. We're basically just keeping AllowedXssTrait::fieldFilterXss() for BC. Should we deprecate it?

EDIT: deprecate for 9, of course.

stefan.r’s picture

The whole trait is deprecated in the patch, we could copy that @deprecated notice to all its methods if needed?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#40: oops, missed that. My bad.

Back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/tests/Drupal/Tests/Core/Field/FieldFilteredStringTest.php
@@ -0,0 +1,63 @@
+    // HTML will be normalised.

normalize. Fixed on commit, for some definitions of fixed.

  • catch committed 97ea6b5 on 8.0.x
    Issue #2551511 by alexpott, stefan.r, RavindraSingh, pjonckiere, JeroenT...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.