Views inline field separator renders HTML as plain text

The bug is still present on the latest Drupal 8 version (8.4.4)

Patch for views-fields output. As this fix uses the twig raw filter, it needs to be reviewed for potentially unsafe HTML input. It appears though that HTML tags and attributes like <script> and style are stripped on input, so using raw is safe.

Not sure if we should do this. Tagging with needs security review.

I'm checking in on this one to see if it'll get security review in the near future? I don't have the liberty to apply the patch unless it's gotten approval. (I'd made a recent duplicate of this issue, for D8)

I am a bit confused. In the current configuration, this should use Xss::filterAdmin, which covers a lot of usecase already. If you need more you could adapt the template as well.

Does someone mind updating the issue summary with an actual example where it breaks?

Dawehner,

If I'm understanding the question, this is all I need to do to see the error. This was done in 8.5.3. I'm currently upgraded to 8.5.4. Haven't tried recreating from beginning, but the page I'd made still renders the string of html as text when viewed

1. Make a view (in my case, it's a block, not page)
2. Add several fields of a content type.
3. Click Settings for Fields format.
4. Scroll to bottom of pop-up window, and where the “Separator” field is, add a break tag. < / br >
(Just below this field, you’ll see the note leading people to believe they can simply add html)
5. Save your view, add the block to a page and go to that page.

I don't see this…

Field one
Field two

I see this: Field one< br / >field two

Duplicate of above.. I tried editing the original message, but it kicked my message down here.

Thanks @turpentine, that example is good for reproducing this issue.

In terms of Xss filtering, sanitization is occurring when the view is saved. Unsafe code is not able to get into the rendered separator using the view builder, even if the output template uses the |raw filter.

We could also try turning off autoescaping for the {{ field.separator }} value, which should produce the same result as |raw:

...
{% for field in fields -%}
  {% autoescape false %}
    {{ field.separator }}
  {% endautoescape %}

...

@garryh thanks for reporting, agree this is a definite concern, the use of BR as line separator between inline label:value pairs is a long-standing trick that no longer works.

I can't believe this has not been solved in almost 4 years! I'm using Drupal 8.7.7 and my server should be up-to-date

On the other hand, I'm a beginning Drupal user and there may be a better way to achieve what I need. I experimented a bit, did not get what I wanted, and eventually landed on this bug report

I want an easy way (in Views) to display some labels and their matching fields on the same line, but not everything on the same line, which seems like a legitimate need. Unfortunately, as this has been detailed above, I'm getting the following after specifying a simple <br/> line break as a separator

Location: Vienna, Austria<br />Date(s): Sunday 3 May 2020 to Friday 8 May 2020

When I check the source code of the page, I see that the page code is using < br / > instead of the actual HTML tag specified in the Separator field.

Well, if you want to prevent code insertion, could someone at least add an option where users could request a line break as a separator (or specify how many line breaks they want), instead of just the existing Separator text field. I'm sure this would cover the basic needs of many users

I know there has already been a lot of work done on this but just curious why we can't just supply some markup around the separator instead of allowing the developer to add markup? If there was at least a span or div then we could simply target with CSS. Maybe I'm misunderstanding the problem.

Using raw looks like the wrong fix. The separator was already Xss filtered, but passed along as a string, so twig had no way of knowing it was safe. So if we pass it along as a markup render array, we should be fine.

This uses that path, so no interdiff since this is a new direction. This also adds test coverage for this. Seems like the Fields row plugin had no specific test coverage so far....

Also updated the IS with the steps to reproduce from #8, thanks for those, very helpful.

@Lendude pinged me for a review. Using a #markup render array looks like a clean / simple / safe solution for this. Test coverage looks great.

Tiniest of nits: we're missing a comma at the end of the #markup. Oh look, bot agrees: https://www.drupal.org/pift-ci-job/1480927

Pointless to have someone else upload the fix so I can RTBC. I'll just do both. ;)

TEST-ONLY would be identical, so I'm just leaving #17 visible in the summary. interdiff proves all I'm doing is the 1-char CS nitpick fix.

Thanks!
-Derek

In theory RTBC can not happen at the same time as a patch upload, so RTBC++ that this issue doesn't have to go back.

Can we also test HTML attributes? Let's change the separator to <span class="field-separator">|</span>.

--- a/core/modules/views/tests/src/Kernel/Plugin/StyleFieldsTest.php
+++ b/core/modules/views/tests/src/Kernel/Plugin/StyleFieldsTest.php
@@ -35,14 +35,14 @@ public function testInlineFields() {
         'id' => 'id',
         'name' => 'name',
       ],
-      'separator' => '<br />',
+      'separator' => '<span class="field-separator">|</span>',
     ];
     $view->display_handler->setOption('row', $row);
     $view->initDisplay();
     $view->initStyle();
     $output = $view->preview();
     $output = $renderer->renderRoot($output);
-    $this->assertContains('<div class="views-row"><span class="views-field views-field-age"><span class="field-content">25</span></span><br /><span class="views-field views-field-id"><span class="field-content">1</span></span><br /><span class="views-field views-field-name"><span class="field-content">John</span></span></div>', (string) $output);
+    $this->assertContains('<div class="views-row"><span class="views-field views-field-age"><span class="field-content">25</span></span><span class="field-separator">|</span><span class="views-field views-field-id"><span class="field-content">1</span></span><span class="field-separator">|</span><span class="views-field views-field-name"><span class="field-content">John</span></span></div>', (string) $output);
     $view->destroy();

@Krzysztof Domański sure we can, but would we expect that to behave differently than a simpeler tag? If that would fail then that would indicate a change/problem in Xss::filter, not a change/problem in Views, so I'm not sure if we need to cover this here.

@Lendude I agree, that change (#21) is not necessary here. Tested manually. RTBC+1.

Re: #20 thanks for RTBC++'ing. Yeah, I know we're not technically supposed to do what I did in #19, but there are times when formal adherence to policy is just silly. ;) Adding a single (non-impactful) comma to satisfy coding standards seemed safe enough to do while RTBCing.

Re: #21, #22 and #23: Yeah, that seems unnecessary. Glad we agree in #23 that #19 is still RTBC. ;)

Cheers,
-Derek

#25 looks like a totally unrelated random fail. Re-queued #19 for testing...

And lo, #19 is green again. Re-RTBC.

+++ b/core/modules/views/views.theme.inc
@@ -139,7 +139,9 @@ function template_preprocess_views_view_fields(&$variables) {
+        $object->separator = [
+          '#markup' => Xss::filterAdmin($variables['options']['separator']),
+        ];

This isn't quite right... this change makes us filter twice.

Here's the documentation for #markup...

 * - #markup: Specifies that the array provides HTML markup directly. Unless
 *   the markup is very simple, such as an explanation in a paragraph tag, it
 *   is normally preferable to use #theme or #type instead, so that the theme
 *   can customize the markup. Note that the value is passed through
 *   \Drupal\Component\Utility\Xss::filterAdmin(), which strips known XSS
 *   vectors while allowing a permissive list of HTML tags that are not XSS
 *   vectors. (For example, <script> and <style> are not allowed.) See
 *   \Drupal\Component\Utility\Xss::$adminTags for the list of allowed tags. If
 *   your markup needs any of the tags not in this whitelist, then you can
 *   implement a theme hook and/or an asset library. Alternatively, you can use
 *   the key #allowed_tags to alter which tags are filtered.

So we can remove the Xss::filterAdmin() here because it is automatically filtered.

Here's a patch that does that and makes the test that the separator is XSS filtered a little more explicit. Leaving a tRTBC because these changes a very minor.

Crediting @garryh for filing the issue and @turpentyne for providing steps to reproduce.

My changes in #28 don't affect the functionality of the patch so I'm committing this to 9.0.x and 8.9.x.

Committed and pushed 28050a2901 to 9.0.x and fb704aeac7 to 8.9.x. Thanks!

Let's leave this as fixed in 8.9.x. Whilst I think this is eligible for 8.8.x release I think committers are very busy atm and I don't want to add this to release manager's workload.