Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug: Valid Twig syntax (eg: conditional with greater/less than) will throw an exception.
Issue priority Normal: only affect one area of functionality
Prioritized changes Yes: Bug fix, security enhancement, and follow up from a major change (#2342287: Allow Twig in Views token replacement).
Disruption None. Does not change existing API or site builder workflow.

(Issue summary updated as of comment #68)

Problem/Motivation

Now that we can use Twig in Views rewrites, we need to respect Twig syntax while filtering for possible XSS vulnerabilities. User-generated content (tokens) that may be used in Views rewrites is auto-escaped by Twig. Thus we only need to validate the rewrite template entered by the site builder. XSS exposure is mitigated by the fact that "Administer Views" permissions are needed to rewrite Views' output.

Additional information

This change is essential to allow #2342287: Allow Twig in Views token replacement to be used to it's full potential. Otherwise, code such as this (which is exactly what result rewriting is all about!) results in a Twig parser error because the ">" character was converted to ">"

{% if field_example|length < 50 %}
  {{ field_example }}
{% else %}
  This field is too long to display
{% endif %}

Proposed resolution

Add a #post_render function to the inline-template that calls Xss::filterAdmin to remove XSS vulnerabilities after token replacement. This will improve on D7 security -- previously rewritten strings were filtered for XSS vulnerabilities before, not after, token replacement allowing tokens to be combined into potential security holes.

Remaining tasks

  1. Moot (using different approach with #post_render): Settle concerns raised in #8 and #11 regarding code bloat and confusion over which filter to use when.
  2. Moot (using different approach with #post_render): Determine if removing the Xss::filterAdmin() is an option or a security regression.
  3. #21 Add -test-only patch to demonstrate the issue.
  4. Add patch to fix the issue.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#99 2466931-90-99.interdiff.txt2.13 KBmikeker
#99 2466931-99-xss-twig-postrender.patch8.79 KBmikeker
#90 2466931-88-90.interdiff.txt569 bytesmikeker
#90 2466931-90-xss-twig-postrender.patch8.45 KBmikeker
#88 2466931-84-88.interdiff.txt1.97 KBmikeker
#88 2466931-88-xss-twig-postrender.patch8.19 KBmikeker
#85 2466931-76-84.interdiff.txt847 bytesmikeker
#85 2466931-84-xss-twig-postrender.patch8.21 KBmikeker
#76 2466931-74-76.interdiff.txt1.32 KBmikeker
#76 2466931-76-xss-twig-postrender.patch8.22 KBmikeker
#74 2466931-74-xss-twig-postrender.patch7.95 KBmikeker
#74 2466931-60-74.interdiff.txt4.2 KBmikeker
#60 2466931-58-60.interdiff.txt583 bytesmikeker
#60 2466931-60-xss-twig-postrender.patch8.44 KBmikeker
#58 2466931-56-58.interdiff.txt1.2 KBmikeker
#58 2466931-58-xss-twig-postrender.patch8.43 KBmikeker
#56 2466931-53-56.interdiff.txt930 bytesmikeker
#56 2466931-56-xss-twig-postrender.patch7.92 KBmikeker
#53 2466931-53-xss-twig-postrender.patch7.01 KBmikeker
#53 2466931-49-53.interdiff.txt778 bytesmikeker
#49 2466931-47-49.interdiff.txt617 bytesmikeker
#49 2466931-49-xss-twig-postrender.patch6.25 KBmikeker
#47 2466931-38-47.interdiff.txt1.36 KBmikeker
#47 2466931-47-xss-twig-postrender.patch6.14 KBmikeker
#38 2466931-38-xss-twig-postrender.patch6.33 KBmikeker
#38 2466931-38-xss-twig-postrender-tests-only.patch2.82 KBmikeker
#31 2466931-31-31-no-SafeMarkup-set.interdiff.txt617 bytesmikeker
#31 2466931-31-no-SafeMarkup-set.patch6.94 KBmikeker
#31 2466931-29-31.interdiff.txt2.35 KBmikeker
#31 2466931-31-xss-twig-postrender.patch6.33 KBmikeker
#29 2466931-25-29.interdiff.txt799 bytesmikeker
#29 2466931-29-xss-twig-post_render.patch4.25 KBmikeker
#25 2466931-21-25.interdiff.patch757 bytesmikeker
#25 2466931-25-xss-twig-post_render.patch4.33 KBmikeker
#21 2466931-21-xss-twig-filter-post_render.patch3.59 KBmikeker
#21 2466931-9-21.interdiff.txt752 bytesmikeker
#21 2466931-21-xss-twig-filter.patch7.05 KBmikeker
#21 2466931-21-xss-twig-filter-tests-only.patch754 bytesmikeker
#9 2466931-7-9.interdiff.txt863 bytesmikeker
#9 2466931-9_xss-filterAdminTwig.patch5.66 KBmikeker
#7 2466931-7_xss-filterAdminTwig.patch5.84 KBmikeker
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeker’s picture

Issue summary: View changes
mikeker’s picture

Issue summary: View changes

Grrr... table formatting...

mikeker’s picture

Category: Feature request » Task

Based on feedback from @YesCT, I'm re-categorizing this as a task since it is a "minor API changes such as adding new hooks."

Note: adding this to core would let something like #2055597: Allow multi-value field rendering to pass through Views rewrite only once move to contrib (I believe, haven't verified that). But without this in core, there's no chance of #2055597: Allow multi-value field rendering to pass through Views rewrite only once working and #2342287: Allow Twig in Views token replacement has essentially been shot in the foot.

mikeker’s picture

Issue summary: View changes

Updated issue summary.

mikeker’s picture

Issue summary: View changes
mikeker’s picture

Issue summary: View changes

Corrected HTML entities.

mikeker’s picture

Status: Active » Needs review
FileSize
5.84 KB

Attached patch adds a static function Xss::filterAdminTwig and unit tests.

joelpittet’s picture

I'd try not to name it after "Twig" or for the purpose of Twig if possible because all of the filters are to exclude unsafe markup from Twig.

There must a better solution that a new filter mechanism and maybe it's more permissive twig markup in views.

mikeker’s picture

Sorry, previous patch had some commented out code.

@joelpittet: I'll ping you on IRC next week to discuss. Thanks for the feedback.

star-szr’s picture

Issue tags: +Twig
mikeker’s picture

Talked with @joelpittet on IRC. In summary: his concern is about unnecessary bloat and the confusion of having two Xss::filterAdmin type functions. Since editing this field requires "administer views" permissions, he suggested removing the XSS filtering on this field as a possible solution.

I have some concerns about that as it could be seen as a security regression vs D7. I'm going to try and rustle up some more opinions on this...

mikeker’s picture

Issue summary: View changes

Updated issue summary with a simplified example. Added remaining tasks.

Fabianx’s picture

Where would that filter be needed? Can the conversions be shown as well, please?

mikeker’s picture

@Fabianx: Here's the IRC conversation, trimmed of the occasional confusion and unrelated banter:

(8:46:00 PM) mikeker: joelpittet: Got a minute to talk about https://www.drupal.org/node/2466931?
(8:48:18 PM) mikeker: joelpittet: I wanted to see if you had any naming suggestions. You said you didn't like the idea of filterAdminTwig().
(8:48:20 PM) joelpittet: mikeker: I don't understand what problem it's solving
(8:48:50 PM) mikeker: joelpittet: The problem is that filterAdmin() removes standalone > and < characters.
(8:49:02 PM) mikeker: joelpittet: But thtat's valid in a Twig context.
(8:49:29 PM) mikeker: joelpittet: I suppose I should back up and say that this all part of rewriting field output using Twig.
(8:49:30 PM) joelpittet: mikeker: maybe it shouldn't be filtered, do you know how we are currently dealing with the body field?
(8:50:55 PM) mikeker: joelpittet: The body field is rendered using whatever text formatter is set for that display.
(8:52:14 PM) joelpittet: mikeker: kinda hoping views can be treated the same way
(8:53:07 PM) mikeker: joelpittet: I think i'm not explaining it well....
(8:53:34 PM) mikeker: joelpittet: Totally bogus example:
(8:54:46 PM) mikeker: joelpittet: If you rewrite a text field such that it truncates after 25 characters you could include Twig similar to: {% if field_test|length > 25 %} .... do something...
(8:55:17 PM) mikeker: joelpittet: But Xss::filterAdmin() will encode the > to &gt; and the Twig parser will barf.
(8:58:41 PM) joelpittet: mikeker: my suggestion is we should not use Xss::filterAdmin() on those fields at all.
(8:59:33 PM) joelpittet: mikeker: you're suggestion is on the safer side, but I think we should not be filtering the input from the admin interface and autoescape should be on by default.
(8:59:37 PM) mikeker: joelpittet: Isn't that going to be seen as a security regression? Granted, you need "admin views" permission to put text in a field rewrite. 
(8:59:46 PM) joelpittet: mikeker: hope not:D
(8:59:55 PM) joelpittet: mikeker: exactly!
(9:00:48 PM) joelpittet: mikeker: same with the body field, if I let my users have Full HTML they can put XSS attacks in if they want!
(9:01:01 PM) joelpittet: mikeker: and I think they should be able
(9:01:01 PM) mikeker: joelpittet: Just to clarify: if someone types a <script>...</script> in a template that's passed to Twig, it gets escaped?
(9:01:12 PM) joelpittet: mikeker: nope
(9:01:24 PM) joelpittet: mikeker: if that is in a variable then yes
(9:03:05 PM) mikeker: joelpittet: Hmmm.... Currently the Views field tokens are passed as variables (which could contain user-generated content) but the rewrite field is processed as an inline template.
(9:03:45 PM) joelpittet: mikeker: if that is safe content you can |raw it, but not recommended
(9:06:03 PM) mikeker: joelpittet: My worry is that we used to filter out XSS vectors in rewritten fields and now we won't be. Is adding an extra filter to Xss that bad?
(9:06:55 PM) joelpittet: mikeker: to me it's bloat and begs the question why choose one over the other...
(9:07:28 PM) joelpittet: mikeker: so i'm trying at all possible to keep things simple(ish)
(9:07:40 PM) mikeker: joelpittet: But if we add Twig processing in elsewhere (including contrib) we'll need something....
(9:07:52 PM) mikeker: joelpittet: (Normally I'm all for simple!  :)
(9:09:27 PM) joelpittet: mikeker: probably need to do some work with twig sandboxing (haven't played enough with it yet to say for sure how to implement)
(9:09:31 PM) mikeker: joelpittet: I was thinking specifically about using Twig to process Token replacements (though I realize that's not making it in 8.0)
(9:10:04 PM) joelpittet: mikeker: sounds like lots of little twig files in your file system...
(9:10:23 PM) mikeker: joelpittet: it looks like a forest over here.
(9:10:45 PM) joelpittet: mikeker: haha
(9:11:07 PM) mikeker: joelpittet: Actually, they could be handled as inline templates -- despite the bad rap those have gotten with PHP Filter, I'm convinced we can do it right with Twig.
(9:12:21 PM) mikeker: joelpittet: The Views rewrite stuff is using inline templates.
(9:12:41 PM) joelpittet: mikeker: fewf
(9:13:55 PM) mikeker: joelpittet: And before you say "code in the database" I would agrue that it's display logic. :)
(9:14:16 PM) joelpittet: mikeker: I'd like to explore no filters there... but maybe run it by Fabianx or chx?
(9:14:45 PM) joelpittet: mikeker: I wouldn't say that.  XSS won't hurt my DB:)
(9:14:59 PM) mikeker: joelpittet: ha!
(9:15:12 PM) joelpittet: mikeker: all for raw in the DB and filter on the way out!
(9:15:26 PM) mikeker: joelpittet: Will do re: pinging Fabianx and/or chx.
(9:18:33 PM) mikeker: joelpittet: Thanks for the feedback!
(9:18:38 PM) mikeker: joelpittet++
(9:18:46 PM) joelpittet: mikeker: no worries, thanks for thinking about it!
(9:18:49 PM) joelpittet: mikeker++
mikeker’s picture

Where would that filter be needed?

@Fabianx: The long answer is above. The short answer is that we (in D7) ran field rewrites through filter_xss_admin. I assumed we would want to do the same in D8. But D8 now allows Twig syntax in rewrites and Xss::filterAdmin removes things like standalone < and > which is valid in a Twig syntax.

I would like to get confirmation that removing filterAdmin will not be considered a security regression before I try pushing for that.

Thanks!

Fabianx’s picture

So I also have a short and a long answer.

In short:

+1 to using twig templates for tokens
+1 to allowing more complex logic
-1 for the chosen implementation of doing a pre-parse for {{ }} (I will comment a post-commit review on the original issue for follow-up)
-1 to not enabling sandbox mode (no don't want to allow functions by default)
+1 to allow '>' in twig templates
+1 to keep XSS security
-1 to the new filter solution - that feels wrong

Proposed better solution:

Use a #post_render function for the inline template, that filters the XSS of the 'executed' / 'rendered' twig template.

I am not sure that will work, but it feels the most secure. (Lets write some tests for it)

What I would like to see (not all in this issue, but in general) is:

a) A test-only patch using a '<' in the twig template in a view filter field, so that we see where core fails with the code right now
b) A successful patch that makes the test-only patch pass - using the new filter (so it can be evaluated)
c) Another successful patch that uses XSS::filterAdmin() on the "result" of the twig template, to prevent early rendering this should be done via a #post_render callback
d) Some more tests to ensure this works in many cases.

Independently lets open issues for:

a) sandbox mode -- maybe allow url / path / link as functions, but restrict everything else
b) better token parsing (or rather just checking for % and @ as prefix and ignoring everything else)

I hope that helps.

mikeker’s picture

@Fabianx: Thank you for the detailed feedback! Just a couple quick comments.

Use a #post_render function for the inline template, that filters the XSS of the 'executed' / 'rendered' twig template.

I like this idea. One concern is that the inline template marks the result as safe. If there were a "

...malicous code...

" in the template, could it be marked as safe before then getting filtered? Couldn't that allow the same mallicious code through elsewhere in the render pipeline?

better token parsing (or rather just checking for % and @ as prefix and ignoring everything else)

Is #2396607: Allow Views token matching for validation (and remove dead code) what you're talking about? It doesn't actually change the parsing but it does put it all in one place.

Fabianx’s picture

#17:

I like this idea. One concern is that the inline template marks the result as safe. If there were a "...malicous code..." in the template, could it be marked as safe before then getting filtered? Couldn't that allow the same mallicious code through elsewhere in the render pipeline?

Wow, excellent thought. Yes, this would be a rare case, where we would undo a mark-safe if there was a difference detected. In which case we should probably do both your new filter and the #post_render.

--

The better token parsing was more my concern of doing a live strtr() on user input twig template, that feels wrong. It is covered here:

https://www.drupal.org/node/2492839 (my points 1 and 3 are actually invalid after discussion with dawehner) ...

cilefen’s picture

Might we want to validate the twig syntax too?

star-szr’s picture

mikeker’s picture

From #16:

a) A test-only patch using a '<' in the twig template in a view filter field, so that we see where core fails with the code right now
b) A successful patch that makes the test-only patch pass - using the new filter (so it can be evaluated)
c) Another successful patch that uses XSS::filterAdmin() on the "result" of the twig template, to prevent early rendering this should be done via a #post_render callback
d) Some more tests to ensure this works in many cases.

a) Done: 2466931-21-xss-twig-filter-tests-only.patch
b) Done: 2466931-21-xss-twig-filter.patch (includes the test from a, see interdiff)
c) Done: 2466931-21-xss-twig-filter-post_render.patch
d) To do...

I love the #post_render option suggested by @Fabianx! Much cleaner and it fixes a long-standing (D7 and probably D6) potential XSS hole with field rewrites where malicious code is split in half and then recombined from two tokens (think first and last name fields rewritten into a full name).

My concern raised in #17 is not an issue. SafeMarkup::set is called after the #post_render functions are called. See Renderer::doRender.

Unless there are objections to the #post_render approach, I'll start writing some tests for this.

The last submitted patch, 21: 2466931-21-xss-twig-filter-tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: 2466931-21-xss-twig-filter-post_render.patch, failed testing.

Fabianx’s picture

#21: Great patches! Much clearer now what is the scope for the usage.

Unfortunately the concerns in #17 are still warranted as the output of twig_render_template itself is marked as safe currently, which is probably not needed as its only used ever within render arrays or we could ask you to mark things safe yourself ...

Should probably re-visit that ::set call after #2273925: Ensure #markup is XSS escaped in Renderer::doRender() is in / edit: could do directly, no need to wait as we deal only with #theme, not #markup here.

Could you open an issue for using the sandbox mode for those inline templates, please? Thanks!

mikeker’s picture

This patch fixes the test that shows as broken in the post_render version of #21. I'll look at items raised in #24 shortly.

Fabianx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: 2466931-21-25.interdiff.patch, failed testing.

Fabianx’s picture

#25: Could you explain why this fixes it? /me curious ...

mikeker’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
799 bytes

Actually, this one is better.

re: #28

Could you explain why this fixes it?

This is a unit test so we hvae to mock the input and output of render(). In this case, we added a parameter to the $build param in the code and thus need to reflect that change in the mocked object.... At least that's what my admittedly limited understanding of PHPUnit leads me to believe.

Fabianx’s picture

I think the #post_render approach looks pretty good - test wise.

Could we add a test explicitly checking for '<script>' not being present in the template after #post_render?

Could we try removing SafeMarkup::set() from twig_render_template() and see what breaks?

As far as I can see that call does nothing except burn memory, because in case there is a prefix or suffix we trust what comes from theme anyway, so not needed to mark safe.

mikeker’s picture

Could we add a test explicitly checking for '<script>' not being present in the template after #post_render?

Yes. See 2466931-31-xss-twig-postrender.patch and regular interdiff.

Could we try removing SafeMarkup::set() from twig_render_template() and see what breaks?

Yes as well: see 2466931-31-no-SafeMarkup-set.patch with the interdiff being between the two patches. But I think it's going to break a lot of things. At least, it should -- in my five second evaluation, anything output by links.html.twig is double-escaped. Perhaps other template files as well? I haven't looked into why.

Finally, I don't think twig_render_template is a factor here as it's not called (as far as I can see) when rendering an inline template. Please let me know if I'm missing it! Though if it is just burning memory, we should still investigate! :)

(Edit: properly escaped HTML...)

Status: Needs review » Needs work

The last submitted patch, 31: 2466931-31-no-SafeMarkup-set.patch, failed testing.

The last submitted patch, 31: 2466931-31-xss-twig-postrender.patch, failed testing.

Fabianx’s picture

#31: Oh, you are totally right, I forgot that inline templates use a different way.

We should be good then :).

Disregard the SafeMarkup::set removal then ...

Thanks for the patch for the #post_render.

We should be ready here soon. (need: final --tests patch and final normal patch for reviews)

Architecture is sane now! :)

The last submitted patch, 31: 2466931-31-no-SafeMarkup-set.patch, failed testing.

mikeker’s picture

Title: Xss::filterAdmin removes too much; Xss::filterAdminTwig is needed » Valid Twig syntax is incorrectly escaped in Views rewrites
Issue summary: View changes
Issue tags: -Needs issue summary update

Updated issue summary and title based on the new (#post_render) approach.

mikeker’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
6.33 KB

Alrighty.... I think this should give us what we need to evaluate this. Attached is a -tests-only patch that demonstrates the problem (and adds some additional verification to ensure script tags are being removed correctly) and a patch with the fix.

The last submitted patch, 38: 2466931-38-xss-twig-postrender-tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 38: 2466931-38-xss-twig-postrender.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 38: 2466931-38-xss-twig-postrender.patch, failed testing.

Status: Needs work » Needs review
mikeker’s picture

Not sure what's happening, but this test is passing on my localhost...

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -341,15 +342,18 @@ public function globalTokenReplace($string = '', array $options = array()) {
    +    $filtered_text = Xss::filterAdmin($text);
    +    if (empty($tokens) || empty($filtered_text)) {
    +      return $filtered_text;
         }
    

    Too bad we need to call Xss:filterAdmin twice here on average, but I don't see how we can change that as we need to call it to see if its empty - I think ...

  2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -371,10 +375,18 @@ protected function viewsTokenReplace($text, $tokens) {
    +        '#post_render' => [
    +          function ($children, $elements) {
    +            return Xss::filterAdmin($children);
    +          }
    

    Nice!

--

Overall looks great to me, not sure we can solve #1, but besides that RTBC.

So setting to that state, to get committer feedback.

mikeker’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -341,15 +342,18 @@ public function globalTokenReplace($string = '', array $options = array()) {
+    // The "Administer views" permission is required to enter what ends up as
+    // $text so we can use the more permissive XSS filter.
+    $filtered_text = Xss::filterAdmin($text);
+    if (empty($tokens) || empty($filtered_text)) {
+      return $filtered_text;
     }
 

The current code only checks if $token is empty. I added the $filtered_text variable assuming I would need it later. That turned out not to be the case. In hindsight, especially considering this can be called several time for each row in a given view result set, I don't think we should do this...

Another patch coming shortly.

mikeker’s picture

Status: Needs work » Needs review
FileSize
6.14 KB
1.36 KB
Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -349,11 +349,8 @@ public function globalTokenReplace($string = '', array $options = array()) {
+      return $text;
     }

Nope, still need to filter here in the return case ...

mikeker’s picture

Status: Needs work » Needs review
FileSize
6.25 KB
617 bytes

Yes, you're right.

Status: Needs review » Needs work

The last submitted patch, 49: 2466931-49-xss-twig-postrender.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review

mikeker’s picture

That was interesting... randomName() returns, well, random characters which sometimes include those that would be escaped by Xss::filterAdmin(). Which causes the test to fail... sometimes.

My apologies, testbot, for cursing you when you were just doing what you were told! :)

Status: Needs review » Needs work

The last submitted patch, 53: 2466931-53-xss-twig-postrender.patch, failed testing.

mikeker’s picture

I was close in #53... The underlying problem is that if the string does not have any Views token delimiters ({{, !, or %) it is not run through Xss::filterAdmin. But it is if it does. That's not good.

/me: starts writing more tests...

mikeker’s picture

Status: Needs work » Needs review
FileSize
7.92 KB
930 bytes

It seems the output from tokenizeValue is filtered later in the render pipeline, so this is only an issue for tests. I've updated StylePluginBase::tokenizeValue to return Xss::filterAdmin'ed text regardless of whether there are Views token delimiters or not.

joelpittet’s picture

Issue tags: +VDC

This may be good to have @dawehner or @timplunkett have a peak at too.

Couple of questions:

  1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -341,15 +342,15 @@ public function globalTokenReplace($string = '', array $options = array()) {
    +      return Xss::filterAdmin($text);
    
    @@ -370,11 +371,19 @@ protected function viewsTokenReplace($text, $tokens) {
    +            return Xss::filterAdmin($children);
    

    Do we need to do this twice?

  2. +++ b/core/modules/views/src/Tests/Handler/FieldUnitTest.php
    @@ -175,7 +175,7 @@ public function testFieldTokens() {
    -    $name_field_2->options['alter']['text'] = '{{ name_2 }} {{ name_1 }}';
    +    $name_field_2->options['alter']['text'] = '{% if name_2|length > 3 %}{{ name_2 }} {{ name_1 }}{% endif %}';
    

    Maybe worth keeping the simple test case in there too?

I'm general +1 on the new plan though:)

mikeker’s picture

Thanks for the review @joelpittet. Agreed about pinging @dawehner -- hadn't thought about @timplunkett, thanks for the suggestion.

Do we need to do this twice?

Yes. It is needed to ensure the output viewsTokenReplace is always sanitized. Previously, if there are no $tokens, I was returning $text unfiltered. See #48.

I suppose splitting the conditional into two parts (one for $text and one for $token) might be more performant... (Xss::filterAdmin doesn't short circuit on an empty string). Updated patch attached along with an update to the function's docblock.

Maybe worth keeping the simple test case in there too?

The names in the test are the names of the Beatles so this sorta keeps the simple test while adding Twig syntax. :)

Status: Needs review » Needs work

The last submitted patch, 58: 2466931-58-xss-twig-postrender.patch, failed testing.

mikeker’s picture

Grrr... Figured there was some test somewhere that would get tripped up by empty. Using strlen instead.

mikeker’s picture

Status: Needs work » Needs review
Fabianx’s picture

Assigned: Unassigned » dawehner
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs subsystem maintainer review

RTBC, from my side, lets get views maintainer review on it.

dawehner’s picture

  1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -333,23 +334,27 @@ public function globalTokenReplace($string = '', array $options = array()) {
    -   *   String with possible tokens.
    +   *   Unsanitized string with possible tokens.
    

    +1 to document more explicit what is going on

  2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -333,23 +334,27 @@ public function globalTokenReplace($string = '', array $options = array()) {
    +    if (!strlen($text)) {
    +      // No need to run Xss::filterAdmin on an empty string.
    +      return '';
    +    }
    

    Seems to be a little bit of an overoptimization, is there a reason for that? Do we often have an empty text there? At least in the places I have checked, there is always some sort of if() wrapping this before. On the other hand the if() here is basically for free

+++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
@@ -453,6 +453,7 @@ public function testRenderAsLinkWithPathAndTokens($path, $tokens, $link_html) {
+      '#post_render' => [function() {}],

Do we really need an empty #post_render entry here?

mikeker’s picture

re: #63:

Item 2:

Seems to be a little bit of an overoptimization, is there a reason for that? Do we often have an empty text there?

Yeah, it may be. In my experience you only see this if you opt to rewrite the field to an empty string. But I wasn't sure about other situations.

The change was in response to #57 and my concern was that Xss::filterAdmin runs through it's full complement of string replacements on an empty string. I figured better safe than sorry.

Item 3:

Do we really need an empty #post_render entry here?

Yes. Since we're mocking the Renderer object, we need to match the incoming parameter list which now includes a #post_render key in the build array.

dawehner’s picture

Yes. Since we're mocking the Renderer object, we need to match the incoming parameter list which now includes a #post_render key in the build array.

Got it, thank you!

tim.plunkett’s picture

Component: base system » views.module
dawehner’s picture

Assigned: dawehner » Unassigned

So its still RTBC from my POV

Fabianx’s picture

Category: Task » Bug report
Issue tags: -Needs subsystem maintainer review +Needs beta evaluation

That is a bug and we need a beta evaluation, thanks to the views maintainers for chiming in.

mikeker’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation

@Fabianx, beta eval already exists in the issue summary. I've updated it to treat this as a bug instead of a task as suggested by #68. Also updated the issue summary to show the current status.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. Why is twig auto escape not protecting us?
  2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -333,23 +334,27 @@ public function globalTokenReplace($string = '', array $options = array()) {
    +      return Xss::filterAdmin($text);
    
    @@ -370,11 +375,19 @@ protected function viewsTokenReplace($text, $tokens) {
    +            return Xss::filterAdmin($children);
    
    +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
    @@ -239,6 +240,11 @@ public function tokenizeValue($value, $row_index) {
    +      $value = Xss::filterAdmin($value);
    

    Can we call SafeMarkup::checkAdminXss instead here?

mikeker’s picture

Status: Needs work » Needs review

Thanks for the review @alexpott. From #70:

Why is twig auto escape not protecting us?

It is, but autoescape only works on the variables passed into a Twig template, not the template itself. In this case, the template (the field rewrite) is the potentially unsafe code (mitigated by the need for the "admin Views" permission). Also, since an XSS hack could be split up into multiple parts and reassembled via a rewrite, it's best to filter for XSS hacks as late as possible.

Can we call SafeMarkup::checkAdminXss instead here?

We could but I don't see the advantage. Renderer::doRender will mark the string as safe following the #post_render callback. Is the concern about potentially double-escaping the string?

alexpott’s picture

Status: Needs review » Needs work

@mikeker good answers. The reason I was asking for SafeMarkup::checkAdminXss() is that atm Xss::filterAdmin() also marks stuff as safe (because Filter::Xss() does) but it won't always do this - see #2506195: Remove SafeMarkup::set() from Xss::filter() but we need to here. Sorry I should have included more detail on why I was asking for that.

mikeker’s picture

Assigned: Unassigned » mikeker

@alexpott, thanks for the clarification. I'll reroll shortly...

mikeker’s picture

Assigned: mikeker » Unassigned
Status: Needs work » Needs review
FileSize
4.2 KB
7.95 KB
Fabianx’s picture

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -377,15 +376,15 @@ protected function viewsTokenReplace($text, $tokens) {
-            return Xss::filterAdmin($children);
+            return SafeMarkup::checkAdminXss($children);

We also in the future won't need to mark this safe as that will happen after #post_render automatically.

So we can leave this one.

I first thought it was a bug that the patch did not fail, but apparently its not marked as safe by the inline entity markup #pre_render'ing.

mikeker’s picture

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 2466931-76-xss-twig-postrender.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review

Seems like a random test failure -- it passes on my localhost.

Retesting....

mikeker’s picture

Status: Needs review » Reviewed & tested by the community

As per #77.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
    @@ -239,6 +239,11 @@ public function tokenizeValue($value, $row_index) {
    +    else {
    +      // ::viewsTokenReplace() will run SafeMarkup::checkAdminXss on the
    +      // resulting string. We do the same here for consistency.
    +      $value = SafeMarkup::checkAdminXss($value);
    +    }
    

    Is this actually necessary?

  2. +++ b/core/modules/views/src/Tests/ViewExecutableTest.php
    @@ -326,7 +327,7 @@ public function testPropertyMethods() {
    +    $this->assertEqual($view->getTitle(), SafeMarkup::checkAdminXss($title));
    

    Use Xss::filterAdmin() no need to use the SafeMarkup version.

Fabianx’s picture

#82.1: Yes, it is, because it was present before. Else no admin filtering is done at all.

mikeker’s picture

Use Xss::filterAdmin() no need to use the SafeMarkup version.

Done.

mikeker’s picture

Status: Needs work » Needs review
FileSize
8.21 KB
847 bytes

Not sure what happened to my uploads. Here they are again.

mikeker’s picture

To expand on what @Fabianx says in #83, "no admin filtering is done at all if there are no token replacements in the rewrite string." Granted it requires admin views permissions to edit a Views field rewrite. Also if tokenizeValue always returns safe markup, it makes it easier to determine if there is safe markup elsewhere in the Views render pipeline.

Fabianx’s picture

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -333,23 +334,27 @@ public function globalTokenReplace($string = '', array $options = array()) {
-      return $text;
+      return SafeMarkup::checkAdminXss($text);

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1284,9 +1284,7 @@ public function renderText($alter) {
-    // Filter this right away as our substitutions are already sanitized.
-    $template = Xss::filterAdmin($alter['text']);

+++ b/core/modules/views/src/Plugin/views/style/StylePluginBase.php
@@ -239,6 +239,11 @@ public function tokenizeValue($value, $row_index) {
+      // ::viewsTokenReplace() will run SafeMarkup::checkAdminXss on the
+      // resulting string. We do the same here for consistency.
+      $value = SafeMarkup::checkAdminXss($value);

While we replace Xss::filterAdmin with checkAdminXss, this is okay, because before the $value was marked safe by the renderer implicitly.

However given the code in HEAD did not fail with the Remove SafeMarkup::set form Renderer::doRender(), it would likely be okay to use SafeString here as well.

Or even don't do anything.

One would need to check if there is new double escaping, if this returned just plain.

So maybe we should use:

Xss::filterAdmin here everywhere and see if it breaks?

mikeker’s picture

As requested, this patch is essentially #84 but with s/SafeMarkup::checkAdminXss/Xss::filterAdmin/g. Let's see what the testbot says.

Status: Needs review » Needs work

The last submitted patch, 88: 2466931-88-xss-twig-postrender.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
8.45 KB
569 bytes

Ugh... That's what I get when I try to do things right before bed! :)

Status: Needs review » Needs work

The last submitted patch, 90: 2466931-90-xss-twig-postrender.patch, failed testing.

The last submitted patch, 90: 2466931-90-xss-twig-postrender.patch, failed testing.

mikeker’s picture

Not sure why this test is failing for the testbot. It passes on Simplytest.me and on my localhost... Retrying one more time.

The last submitted patch, 90: 2466931-90-xss-twig-postrender.patch, failed testing.

The last submitted patch, 90: 2466931-90-xss-twig-postrender.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
8.79 KB
2.13 KB

New tests were failing because of the new render context. But I still don't know why they didn't fail using the Simpletest UI... As mentioned in IRC, running tests using run-test.sh vs Batch API is "just pure WTF."

Anyhow, this should fix things.

@Fabianx: This should have everything going through Xss::filterAdmin() instead of SafeMarkup::checkAdminXss(). I haven't found any double-escaping in my (limited) manual testing and there doesn't seem to be anything showing up in the automated tests.

Are we good here?

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

Sorry for the back and forth and thanks for following up!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This is nice work and nice tests. Committed cc17945 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

contemplate in core ftw!

  • alexpott committed cc17945 on 8.0.x
    Issue #2466931 by mikeker, Fabianx, dawehner, joelpittet: Valid Twig...

Status: Fixed » Closed (fixed)

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

FreeAndEasy’s picture

FreeAndEasy’s picture

Status: Closed (fixed) » Active

(reopening, see here: https://www.drupal.org/node/2542764)

HTML in the rewritten output is escaped when it shouldn't ("The text to display for this field. You may include HTML or Twig. You may enter data from this view as per the "Replacement patterns" below.").

cilefen’s picture

Status: Active » Closed (fixed)

@FreeAndEasy I think "Closed (fixed)" issues are not to be reopened. It is better to go with your follow-up issue.