Views replacement token bc layer allows for Twig template injection via arguments

We know that a token is either %1 or @1, so replace those directly.

Can you elaborate on that sentence? I don't get that, %1 and @1 are at least not the only possible token names in views. There is stuff like {{ body }} and what not.

Awesome! The first example of a Twig Injection Exploit (now know as a TIE... maybe TIX is more cool and hip?)

Anyhow, we should probably hold off on this until #2466931: Valid Twig syntax is incorrectly escaped in Views rewrites is sorted. Also, the CR from #2342287: Allow Twig in Views token replacement will need to updated.

#3:

Dawehner suggested to break BC and allow e.g. {{ arguments[0] }} and {{ raw_arguments[0] }} instead of %1 and @1.

Just to clarify, %1 is the raw argument and !1 is the raw argument that's been run through strip_tags(Html::decodeEntities($arg)). There is no @1 token. At least that's how I read it (ViewExecutable.php@1036). @dawehner, can you verify?

So the proposal is to replace !n and %n with Twig arrays {{ raw_arguments }} and {{ arguments }}, respectively. Previously, the argument tokens were 1-based while arrays are zero-based, so %1 would be replaced by {{ argument[0] }}. I'm fine with that, but wanted to make sure no one else objects...

Another option would be to use the argument ID as the key to the Twig arrays. Eg: {{ arguments['uid'] }}. This avoids the zero- vs. one-based indexing but we would need to expose the argument IDs somewhere -- probably similar to the "Replacement patterns" details element when rewriting fields.

I've found something else to confuse things: core/modules/views/src/Plugin/views/field/FieldPluginBase.php@871 says:

$options[t('Arguments')]['%' . ++$count] = $this->t('@argument title', array('@argument' => $handler->adminLabel()));
$options[t('Arguments')]['!' . $count] = $this->t('@argument input', array('@argument' => $handler->adminLabel()));

which implies that the % argument is not a raw value so much as the argument title. But that doesn't seem to be how it operates. If I rewrite a field as "%1 and !1" it results in "foo and foo" not "uid and foo" or "Author ID and foo".

Just a small note, {{ arguments['uid'] }} can be also written as {{ arguments.uid }} in Twig.

EDIT: posted here https://www.drupal.org/node/2543796. Leaving this for reference.

=========================
This may not be the correct place to post this issue, so I can repost elsewhere.

Using D8-beta13 I attempted to use a token in a view to rewrite the CSS output:

1. Under Style Settings in the field configuration, select 'create a css class'
2. Insert: icomoon-{{ field_icomoon }} heading where '{{ field_icomoon }}' is the replacement token grabbed from the 'rewrite results' section.
3. Save

When a page with the block is loaded, the "website encountered an unexpected error". Console output shows:
[Thu Jul 30 16:52:36.495200 2015] [fcgid:warn] [pid 17226:tid 3015823360] [client 127.0.0.1:52474] mod_fcgid: stderr: Uncaught PHP Exception Twig_Error_Syntax: "Unexpected token "end of template" of value "" in "{# inline_template_start #}icomoon-{{" at line 1" at /core/vendor/twig/twig/lib/Twig/ExpressionParser.php line 190, referer: http://local.site/admin/structure/views/view/services

Note:
1. same replacement token works without error in the "Rewrite Results" section.
2. CSS replacement works if it does not contain the token.

@mariagwyn: #14 would be better off in a separate issue. I updated the title of this issue to be more specific.

@Fabianx: I can take a stab at it, but I'm on vacation right now and my dev time is limited to the hour-or-so I get while my wife does yoga! :P

I would really, really hate to see #2342287: Allow Twig in Views token replacement rolled back because of this...

Work in progress...

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -357,24 +353,31 @@ protected function viewsTokenReplace($text, $tokens) {
+      // Check for arrays in Twig tokens. Internally these are passed a
+      // dot-delimited strings, but need to be turned into associative arrays
+      // for parsing.
+      if (strpos($token, '.') !== FALSE) {
+        $parts = explode('.', $token);
+        $top = array_shift($parts);
+        $token_array = array(array_pop($parts) => $replacement);
+        foreach(array_reverse($parts) as $key) {
+          $token_array = array($key => $token_array);
+        }
+        $twig_tokens[$top] = $token_array;

Is there any way to hand Twig an array in Twig format (eg: foo.bar.baz) instead of as proper PHP array? I'm worried that changing Views' tokens from a string to an array will be too disruptive to get in this late in the game...

Or we can keep this kludge and add a @todo and a followup issue.

This should fix a few tests...

Updates some docs and description fields and fixes some tests.

In core/modules/views/src/Tests/Handler/AreaEntityTest.php:130 the test uses Views' preview function and sets the result as the raw content:

    /** @var \Drupal\Core\Render\RendererInterface $renderer */
    $renderer = $this->container->get('renderer');
    $view = Views::getView('test_entity_area');
    $preview = $view->preview('default', [$entities[1]->id()]);
    $this->setRawContent(\Drupal::service('renderer')->renderRoot($preview));

Can anyone explain why we use this method rather than rendering the View for real? At first glance, there may be a bug in Views' preview that prevents the token from rendering correctly, but it works as expected when the view is rendered normally.

Can anyone explain why we use this method rather than rendering the View for real?

To answer my own question, it's because you can pass arguments to preview(), but not render() as those are pulled from the request object.

This patch should fix up more tests and remove more instances of ! and % tokens.

This should take care of a couple of the failures. I would appreciated any help on the remaining exceptions as I won't have much time in the next few days to work on this.

LogicException: Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead.

Basically, it throws the exception in Views' preview mode or when running tests, but doesn't when rendering the view regularly.

Tagging for twig visibility

RC1 is imminent, this issue is now at top of the "issues to revisit before RC" list.

@Fabianx warns in #16 that #2342287: Allow Twig in Views token replacement is likely to be rolled back if this issue is not fixed.

Rerolled. PluginBase.php was a fair bit of change so I included an interdiff. Waiting to see what the testbot says before moving on.

@jonathanjfshaw: Remaining items on this issue is pretty clearly laid out in #28 and #19. (The code in #19 is made a bit more wonky by the latest reroll).

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1662,6 +1662,8 @@ protected function getTokenValuesRecursive(array $array, array $parent_keys = ar
+        // @TODO: mikeker: https://www.drupal.org/node/2492839

Oops. Fix coming soon.

1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -357,30 +353,38 @@ protected function viewsTokenReplace($text, $tokens) {
            assert('preg_match(\'/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/\', $token) === 1', 'Tokens need to be valid Twig variables.');
   
   +++ b/core/modules/views/tests/modules/views_test_data/src/Plugin/views/field/FieldTest.php
   @@ -46,7 +46,7 @@ public function getTestValue() {
   -    $tokens['[test__token]'] = $this->getTestValue();
   +    $tokens['{{ test-token }}'] = $this->getTestValue();
   

   That "-" in test-token is not valid?

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1556,8 +1555,8 @@ public function getRenderTokens($item) {
   +    foreach   ($this->displayHandler->getHandlers('argument') as $arg => $handler) {
   

   Some extra space.

3. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -357,30 +353,38 @@ protected function viewsTokenReplace($text, $tokens) {
   -    // Non-Twig tokens are a straight string replacement, Twig tokens get run
   -    // through an inline template for rendering and replacement.
   -    $text = strtr($text, $other_tokens);
   
   @@ -399,7 +403,7 @@ function ($children, $elements) {
        else {
   -      return $text;
   +      Xss::filterAdmin($text);
        }
   

   That else{} looks unreachable now and can be removed? Just to avoid reverting the other issue, would it help to run that strtr() after rendering the inline template?

4. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1556,8 +1555,8 @@ public function getRenderTokens($item) {
   +      $token = "{{ arguments.$arg }}";
          if (!isset($tokens[$token])) {
            $tokens[$token] = '';
          }
   
   +++ b/core/modules/views/src/ViewExecutable.php
   @@ -1028,8 +1028,8 @@ protected function _buildArguments() {
   +        $substitutions["arguments.$id"] = $arg_title;
   +        $substitutions["raw_arguments.$id"] = strip_tags(Html::decodeEntities($arg));
   

   $substitutions is missing "{{" and "}}".

5. +++ b/core/modules/views/tests/src/Unit/Plugin/area/EntityTest.php
   @@ -130,10 +130,10 @@ protected function setupEntityManager() {
   -      ['[test:global_token]', 8],
   +      ['{{ test:global_token }}', 8],
   

   This looks like a global token. Do we need to replace those too?

#28: Filed #2564475: LogicException: Render context is empty in views ui preview.

#33: Fixed.

#35: Thanks for the review @olli!

That "-" in test-token is not valid?

Nope. Fixed.tests

2. Some extra space.

Fixed.

3. That else{} looks unreachable now and can be removed?

I think that is left over from an earlier approach to this issue and can be removed. At the moment it's not doing anything since viewsTokenReplace doesn't take a reference to a string. I've removed it in this patch.

Just to avoid reverting the other issue, would it help to run that strtr() after rendering the inline template?

I don't understand how that would help... Can you clarify? Thanks.

4. $substitutions is missing "{{" and "}}".

Fixed. I keep on missing those...

5. This looks like a global token. Do we need to replace those too?

Not sure. That was needed to make the tests work, but I need to look at it again to see what's happening...

Given that this is a security problem, we should add some test to ensure it actually works and does not allow you to execute code.
On top of that we need an update path + update path testing, happy to do that.

Security-wise, is it correct to say the impact is mitigated by the fact that this is only exposed to authenticated users with the "Administer views" permission, which is marked as having administrative impact ?

Changing the title to reflect the bug a bit better.

Alright, this is not 100% the case fabian had, but still, it shows that things are problematic.

Started with some rudimentary update path. I'm pretty convinced that these aren't all the possible configurations yet.

Regarding arguments.1 vs. arguments[1] vs. arguments.machine_name, I would think at that point we should go with the number, just because people are used to that.

Can you clarify in the issue summary why having those longer variable names is more secure?

Sure, but its written down pretty good in #2492839-40: Views replacement token bc layer allows for Twig template injection via arguments

Regarding arguments.1 vs. arguments[1] vs. arguments.machine_name, I would think at that point we should go with the number, just because people are used to that.

One advantage of going with the number is it makes the upgrade path code a lot easier! But I've always hated having the token names tied to the order of the contextual filters not to mention the ever-present confusion over one- vs. zero-indexing them...

(Also, just for clarification, arguments.1 and arguments[1] are identical to Twig in this context.)

This is now critical so "revisit before release candidate" is now redundant. :)

Ah, as is "rc target".

I was playing around with this and fixed some of the tests on the way.

Exported a test view which should have everything you can imagine for the update path.

Diving in here and reviewing the code straight away:

1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -398,9 +403,6 @@ function ($children, $elements) {
   -    else {
   -      return $text;
   -    }
   

   Don't we still need to return the text if there are no twig tokens? I don't see any early exiting added.

FYI I'm also a proponent to using named arguments if possible here. The order of the arguments make reading views export quite tricky, this would be a nice DX improvement if we can pull it off. +1.

2. +++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
   @@ -53,13 +53,12 @@ public function tokenForm(&$form, FormStateInterface $form_state) {
   +      $options[t('Arguments')]["{{ arguments.$arg }}"] = $this->t('@argument title', array('@argument' => $handler->adminLabel()));
   

   Note to self: find out if the t() can be removed from the keys here for our other issue returning TranslationWrappers from t()

3. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
   @@ -249,7 +249,8 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +      '#description' => $this->t('Override the view and other argument titles. You may use Twig syntax in this field.'),
   +      // @todo: Need to show available tokens.
   

   Do we need a token list here or just examples like before?

4. +++ b/core/modules/views/tests/modules/views_test_data/views_test_data.module
   @@ -110,3 +110,8 @@ function template_preprocess_views_view_mapping_test(&$variables) {
   +function views_test_data_test_pre_render_function($element) {
   +  $element['#markup'] = 'Muh';
   

   Needs a docblock and maybe better values to test? Meh

5. +++ b/core/modules/views/tests/src/Unit/Plugin/area/EntityTest.php
   @@ -130,10 +130,10 @@ protected function setupEntityManager() {
   -      ['!1', 5],
   -      ['%2', 6],
   +      ['{{ arguments.test1 }}', 5],
   +      ['{{ raw_arguments.test2 }}', 6],
   

   This looks backwards. % is arguments and 6 and test1 should be 5.

6. +++ b/core/modules/views/tests/src/Unit/Plugin/area/EntityTest.php
   @@ -130,10 +130,10 @@ protected function setupEntityManager() {
   -      ['[test:global_token]', 8],
   +      ['{{ test:global_token }}', 8],
   

   Surprised this works.

7. +++ b/core/modules/views/views.install
   @@ -117,5 +117,136 @@ function views_update_8001(&$sandbox) {
   +          $token_values = ['path', 'alt', 'link_class', 'rel', 'target', 'query', 'fragment', 'prefix', 'suffix', 'more_link_text', 'more_link_path', 'link_attributes'];
   

   Though long this may be better multi line.

8. +++ b/core/modules/views/views.install
   @@ -117,5 +117,136 @@ function views_update_8001(&$sandbox) {
   +    }
   +
   +
   +    if ($changed) {
   

   nit: remove extra line break.

Working on this for a while.

Thank you for the review @joelpittet

Fixed parts of the review, expanded the test coverage, sighing off for tonight

Less of a review more of a praise:)

1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -401,7 +403,7 @@ function ($children, $elements) {
   -      return (string) $this->getRenderer()->render($build);
   +      return (string) $this->getRenderer()->renderPlain($build);
   

   This is a good change as it's unlikely a token will attach assets:)

2. +++ b/core/modules/views/src/Tests/Update/ArgumentPlaceholderUpdatePathTest.php
   @@ -0,0 +1,49 @@
   +      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.bare.standard.php.gz',
   

   Whoa cool way to test upgrade path, didn't know that was possible.

Working on fixing the tests...

Quick summary of changes:

1. "argument" => "arguments"
2. "displays" => "display"
3. Corrected style options "path" in display_options array
4. Added !1 and raw_arguments to the test
5. Fixed ['fields']['title']['alter']['text'] conversion
6. Moved $token_values outside of the loop

Going to try to tackle the remaining failures, nice cleanup and fixes @mikeker and @dawehner!

Since we are using renderPlain the mock parameter count needed to change and method it expected.

1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -396,10 +403,7 @@ function ($children, $elements) {
   -      return (string) $this->getRenderer()->render($build);
   -    }
   -    else {
   -      return $text;
   +      return (string) $this->getRenderer()->renderPlain($build);
   

   This is my biggest concern: This change looks wrong. I'm quite sure it needs to return $text still. Can't someone explain this?

2. +++ b/core/modules/views/views.install
   @@ -132,22 +132,23 @@ function views_update_8002() {
   -            'link_attributes'
   -          ];
   ...
   +          'link_attributes',
   +          'text',
   +        ];
   

   re #72Thanks for adding the comma @mikeker. Was the 'text' token name needed? It wasn't mentioned in your summary of changes.

#78:

This change looks wrong. I'm quite sure it needs to return $text still. Can't someone explain this?

I think that's a oddity in how the diff looks... That line (core/modules/views/src/Plugin/views/PluginBase.php:406) comes right after the inline template is built and passed the Twig tokens. Returning $text would negate the purpose of the function.

I'm planning to be on the Twig conf call tomorrow morning -- we can chat about this before/after to make sure we're talking about the same thing...

Was the 'text' token name needed? It wasn't mentioned in your summary of changes.

Sorry, missed that one. Yes, it's needed for $data['display']['default']['display_options']['fields']['title']['alter']['text'] in core/modules/views/src/Tests/Update/ArgumentPlaceholderUpdatePathTest.php:37. Crazy array! :)

@joelpittet, Thanks for getting this patch green!

+++ b/core/modules/views/src/Tests/Update/ArgumentPlaceholderUpdatePathTest.php
@@ -0,0 +1,49 @@
+    $this->assertEqual('{{ arguments.nid }}-more-text-{{ raw_arguments.nid }}', $data['display']['default']['display_options']['use_more_text']);
+    $this->assertEqual('{{ arguments.nid }}-custom-url-{{ raw_arguments.nid }}', $data['display']['default']['display_options']['link_url']);

Nice! Thank you

Expanded the test coverage, show the tokens on arguments.

Wrote a CR, updated the issue summary with the proposed solution.

1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -396,10 +402,7 @@ function ($children, $elements) {
   +      return (string) $this->getRenderer()->renderPlain($build);
        }
      }
   

   If there is no tokens to be replaced we should still return the $text because it might be just static text.

2. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
   @@ -348,6 +365,46 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +      $options[t('Arguments')]["{{ arguments.$arg }}"] = $this->t('@argument title', array('@argument' => $handler->adminLabel()));
   +      $options[t('Arguments')]["{{ raw_arguments.$arg }}"] = $this->t('@argument input', array('@argument' => $handler->adminLabel()));
   

   This is going to have problems after #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list , see also #2561259: Cast (optgroup) array keys that may hold translated strings to string

3. +++ b/core/modules/views/src/Tests/Handler/FieldKernelTest.php
   @@ -174,6 +174,44 @@ public function testRewrite() {
   +    $this->assertFalse(strpos((string) $output, 'views_test_data_test_pre_render_function executed') !== FALSE);
   ...
   +    $this->assertFalse(strpos((string) $output, 'views_test_data_test_pre_render_function executed') !== FALSE);
   

   These could definitely use a message

4. +++ b/core/modules/views/src/Tests/Handler/FieldKernelTest.php
   @@ -174,6 +174,44 @@ public function testRewrite() {
   +    $this->assertEqual(' ', (string) $output, "Ensure that old style placeholders aren't replaced");
   

   I think the message is not correct anymore

Thank you for your review @lauriii!

If there is no tokens to be replaced we should still return the $text because it might be just static text.

I do agree, its preventing a potential future bug indeed. I'm pretty sure we have an if everywhere before calling this function, but better be safe than sorry.

This is going to have problems after #2557113: Make t() return a TranslationWrapper object , see also #2561259: Cast array keys that may hold translated strings to string

Let's cast it to a string already. In general though this is existing code and not something this patch really introduces.

These could definitely use a message

Good point

I think the message is not correct anymore

Ha you are right!

Seemed to be a random test failure.

Cleaned up the remaining tasks

Thanks @dawehner!

And apologies to @joelpittet who talked about the need for returning $text but I was misunderstanding what he meant. I'm not convinced you can reach that else clause but, as @dawehner says, better safe than sorry. Along those lines, we should Xss::filterAdmin the value to be consistent with other return values from this function.

Thanks, @Fabianx, for the review! From #89:

1. I would like to understand why this change is needed. And what fails if we don't do that.

Views rendered in preview mode were throwing exceptions (see #2564475: LogicException: Render context is empty in views ui preview). Some tests use preview to render views as it's easier to pass arguments (see test results in #28).

Agreed, it limits our ability to attach assets in the future, but I think that's ok. Maybe?

2: Fixed. Ideally we would link to a d.o docs page that better describes the values in arguments and raw_arguments.

4: I tried that at one point but wasn't able to get it working (see this gist). But I don't really understand render contexts so I may have been doing it completely wrong!

Agreed, it limits our ability to attach assets in the future, but I think that's ok. Maybe?

What about adding a todo to investigate whether we want to create a render context in the future? When I tried to fix the failure I was just too lazy to create the additional code to create a render context :p, but at least for tokens its really not needed.

mikeker++

Added followup and @todo: #2566621: [Follow-up] PluginBase::viewsTokenReplace uses renderPlain which may limit the use of attached assets.

Thank you @Fabianx and @mikeker
I agree we should maybe rethink whether we should have those execution wrappers at a higher level to be honest.

Tested this manually by using the arguments in view title and field override. I was unable to execute Twig and tokens where replaced as supposed to. Also the patch looks good for me.

I think it should be fine with Renderer::renderPlain() because we really shouldn't be doing asset additions in tokens. And if that is needed, it can be a feature request or follow-up as we have it.

Fixed a couple nitpicks.

Trying to figure out how to get the tokens to work with global null views.null for the remaining !1 token for AreaEntityUITest.

Working on the latter part of #96 unless I'm wrong, feel free to stop me:)

Ok that was easier than I thought:)

Both the changes in #96 and #98 look good to me. Thanks for correcting my grammar! :)

This patch fixes this little UI thing(kinda makes it worse looking but correct markup and that's CSS for item list in views fault):

Missed a couple more of those and tried to make the #description that was being built up on there be consistent with the exact same build up.

Found by searching for this:
foreach ($this->view->display_handler->getHandlers('argument') as $arg => $handler) {

Here's a screenshot after to show I didn't break things by moving it to a render array instead of drupal_render().

Ok feel free to RTBC away @lauriii;)

I feel we need to lock down this issue. We're starting to add nice-to-haves and I'm afraid that will destabilize the fix we have consensus on.

@joelpittet is absolutely correct that '#list_type' => $type is wrong and results in bogus HTML. But it turns out there were tests that relied on that bogus HTML (who knew...).

Let's not let perfect get in the way of pretty-good!

Along those lines, #66

find out if the t() can be removed from the keys here for our other issue returning TranslationWrappers from t()

should be opened as a followup.

Thanks joelpittet and mikeker for the additional fixes!

Reposting latest patch because it is not listed in the files list and seems like PIFR cannot find it.

Oh and of course the tests are checking for <fields> in xpath. Here's a fix for that.

Dang cross posted with both of you! I should have just went to bed:P

1. core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_validator_term.yml still has title: '%1' - it was added relatively recently - #1739846: Tests taxonomy argument validator plugin

2. +++ b/core/modules/views/views.install
   @@ -117,5 +117,178 @@ function views_update_8001(&$sandbox) {
   +function _views_update_8002_token_update($text, array $argument_map) {
   

   Missing documentation for $argument_map

3. +++ b/core/modules/views/views.install
   @@ -117,5 +117,178 @@ function views_update_8001(&$sandbox) {
   + * @param array $displays
   ...
   +function _views_update_argument_map($displays) {
   

   Missing documentation for $displays

4. +++ b/core/modules/views/views.install
   @@ -117,5 +117,178 @@ function views_update_8001(&$sandbox) {
   +
   

   Unnecessary new line

5. +++ b/core/modules/views_ui/src/Tests/XssTest.php
   @@ -29,8 +29,8 @@ public function testViewsUi() {
   -    $this->assertRaw('[title] == <marquee>test</marquee>', 'Token label is properly escaped.');
   -    $this->assertRaw('[title_1] == <script>alert("XSS")</script>', 'Token label is properly escaped.');
   +    $this->assertRaw('{{ title }} == <marquee>test</marquee>', 'Token label is properly escaped.');
   +    $this->assertRaw('{{ title_1 }} == <script>alert("XSS")</script>', 'Token label is properly escaped.');
   

   Nothing wrong with this patch because this is fixed elsewhere (by the checkPlain() work) but this test is nicely absurd. It is testing that we have a double escaping bug :)

LOL nice catch on the double escaping test, I thought I read that but I guess not thorough enough.

Fixed the items in #111. And added an extra double escape test to ensure we don't have any more. The double escaping was on the concatenation of a previously escaped admin_label passed in an @ then concatenated to ' == '

Self-reivew.

I'm really sorry - I was just pointing out the double escape test cause it is funny. Fixing this is not in scope for this issue. I should have been clearer.

+++ b/core/modules/views/src/Plugin/views/area/TokenizeAreaPluginBase.php
@@ -80,7 +80,7 @@ public function tokenForm(&$form, FormStateInterface $form_state) {
-            $items[] = $key . ' == ' . $value;
+            $items[] = ['#markup' => $key . ' == ' . $value];

+++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
@@ -389,7 +389,7 @@ protected function getTokenHelp() {
-            $items[] = $key . ' == ' . $value;
+            $items[] = ['#markup' => $key . ' == ' . $value];

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -1746,7 +1746,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-                $items[] = $key . ' == ' . $value;
+                $items[] = ['#markup' => $key . ' == ' . $value];

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -890,7 +890,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-              $items[] = $key . ' == ' . $value;
+              $items[] = ['#markup' => $key . ' == ' . $value];

+++ b/core/modules/views_ui/src/Tests/XssTest.php
@@ -29,8 +29,9 @@ public function testViewsUi() {
-    $this->assertRaw('{{ title }} == <marquee>test</marquee>', 'Token label is properly escaped.');
-    $this->assertRaw('{{ title_1 }} == <script>alert("XSS")</script>', 'Token label is properly escaped.');
+    $this->assertRaw('{{ title }} == <marquee>test</marquee>', 'Token label is properly escaped.');
+    $this->assertRaw('{{ title_1 }} == <script>alert("XSS")</script>', 'Token label is properly escaped.');
+    $this->assertNoRaw('<', 'The page does not have double escaped HTML tags.');

Let's revert this.

Thank you alex!
Here are some additional nitpicks, we can fix them later if we want though

+++ b/core/modules/views_ui/src/Tests/XssTest.php
@@ -29,8 +29,9 @@ public function testViewsUi() {
+    $this->assertRaw('{{ title }} == <marquee>test</marquee>', 'Token label is properly escaped.');
+    $this->assertRaw('{{ title_1 }} == <script>alert("XSS")</script>', 'Token label is properly escaped.');
+    $this->assertNoRaw('<', 'The page does not have double escaped HTML tags.');

Could we use some of assertEscaped/assertNotEscaped for those?

1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -357,34 +353,44 @@ protected function viewsTokenReplace($text, $tokens) {
   +      if (strpos($token, '.') === FALSE) {
   

   I'm curious whether given that this method could be called quite often as well, we could use strpbrk

2. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -357,34 +353,44 @@ protected function viewsTokenReplace($text, $tokens) {
            assert('preg_match(\'/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/\', $token) === 1', 'Tokens need to be valid Twig variables.');
   ...
   +        assert('preg_match(\'/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/\', $top) === 1', 'Tokens need to be valid Twig variables.');
   ...
   +          assert('preg_match(\'/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/\', $key) === 1', 'Tokens need to be valid Twig variables.');
   

   Do you mind adding a follow up maybe even in twig itself to have a constant to validate those properly

3. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
   @@ -396,10 +402,14 @@ function ($children, $elements) {
        else {
   -      return $text;
   +      return Xss::filterAdmin($text);
        }
   

   Can we add some explicit test coverage for that? I guess its not possible because you never actually reach this point?

4. +++ b/core/modules/views/views.install
   @@ -117,5 +117,180 @@ function views_update_8001(&$sandbox) {
   + *   A map of argument machine names keyed by their previous index.
   + * @return string The updated value.
   

   Nitpick: new line needed

The issue that covers the double escaping bug is #2564283: Remove use of SafeMarkup::checkPlain() from adminSummary() and adminLabel() in views plugins

I miss-interpreted the review in #111.5 That crappy test was already there we just swapped out the guts. I'm reverting that.

He was referring to #2564283: Remove use of SafeMarkup::checkPlain() from adminSummary() and adminLabel() in views plugins which deals with that test and more.

@joelpittet
Feel free to fix my other points.

@dawehner re #116

0) That will move to the other issue.

1. Sounds like a bit of optimization and never heard of that function, may be good for a follow-up?
2. @mikeker maybe you can create a follow-up for that?
3. Don't see a big point at the moment it's just balancing what was there. Maybe could use a follow-up.
4. Done in other quick fix patch:) #114

Sounds like a bit of optimization and never heard of that function, may be good for a follow-up?

We used it in #2492967: SQL layer: $match_operator is vulnerable to injection attack as well

Done in other quick fix patch:) #114

Ah I see its used in #114, I think I reviewed something before that

re: #116

1: Learn something new every day in this issue queue! :)

I would like to profile strpbrk vs. strpos vs. strcspn. Should be a follow-up, IMO.

2: Added #2567269: Create a Twig regex constant or function that validates a Twig variable.

3: As you say, I think it will be hard to add a test for that as I can't think of a way to reach that else-block. It was added as a better-safe-than-sorry.

4: Dude, @joelpittet is on it! :)

Also, not sure what the protocol is for this, but I believe @Fabianx and @olli should be added to the Dreditor-supplied commit message for their code reviews and that @Fabianx found the vulnerability that made this a critical. Thanks for considering it.

I'm not totally sure the protocol myself but I believe you just add it to the issue summary.
git commit -m 'Issue #2492839 by mikeker, joelpittet, dawehner, lauriii, Fabianx, olli: Views replacement token bc layer allows for Twig template injection via arguments'

Ideally, we'd want anyone who should get credit to comment on the issue so they appear in the credit area at the bottom of the page. That info is stored in drupal.org.

1. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
   @@ -146,7 +146,7 @@ public function submitOptionsForm(&$form, FormStateInterface $form_state) {
   -    if (strpos($options['target'], '{{') === FALSE && strpos($options['target'], '!') === FALSE && strpos($options['target'], '%') === FALSE && strpos($options['target'], '[') === FALSE) {
   +    if (strpos($options['target'], '{{') === FALSE) {
   
   @@ -161,7 +161,7 @@ public function render($empty = FALSE) {
   -      if (strpos($this->options['target'], '{{') !== FALSE || strpos($this->options['target'], '!') !== FALSE || strpos($this->options['target'], '%') !== FALSE || strpos($this->options['target'], '[') !== FALSE) {
   +      if (strpos($this->options['target'], '{{') !== FALSE) {
   
   +++ b/core/modules/views/tests/src/Unit/Plugin/area/EntityTest.php
   @@ -130,10 +130,10 @@ protected function setupEntityManager() {
   -      ['[test:global_token]', 8],
   +      ['{{ test:global_token }}', 8],
   

   Does this drop support for global tokens in entity area plugin, are they still listed in the ui as available global tokens? : is not allowed?

   Did anyone check other places that use global tokens for possible injection?

2. +++ b/core/modules/views/src/Tests/Handler/FieldKernelTest.php
   @@ -174,6 +174,44 @@ public function testRewrite() {
   +    $name_field_0->options['alter']['text'] = '{{ arguments.name }} {{ raw_arguments.name }}';
   
   +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_field_argument_tokens.yml
   @@ -0,0 +1,59 @@
   +      arguments:
   +        null:
   +          id: null
   +          table: views
   +          field: null
   +          plugin_id: ull
   

   arguments.name -> arguments.null

   null,ull -> 'null'

3. +++ b/core/modules/views/tests/fixtures/update/argument-placeholder.php
   @@ -0,0 +1,19 @@
   +  ->fields(array(
   +    'collection',
   +    'name',
   +    'data',
   +  ))
   +  ->values(array(
   +    'collection' => '',
   +    'name' => 'views.view.test_token_view',
   +    'data' => serialize(\Drupal\Component\Serialization\Yaml::decode(file_get_contents('core/modules/views/tests/modules/views_test_config/test_views/views.view.test_token_view.yml'))),
   +  ))
   +  ->fields([
   +    'collection' => '',
   +  ])
   

   Is this same as

   ->fields([
     'collection' => '',
     'name' => 'views.view.test_token_view',
     'data' => serialize(...),
   ])
   

   ?

4. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_field_argument_tokens.yml
   @@ -0,0 +1,59 @@
   +base_table: views_test_data
   +base_field: nid
   

   base_field: id?

5. +++ b/core/modules/views/views.install
   @@ -117,5 +117,180 @@ function views_update_8001(&$sandbox) {
   +            switch ($area['plugin_id']) {
   +              case 'title':
   ...
   +              case 'result':
   ...
   +              case 'text':
   ...
   +              case 'text_custom':
   

   'entity' is missing

6. +++ b/core/modules/views/views.install
   @@ -117,5 +117,180 @@ function views_update_8001(&$sandbox) {
   +          }
   +          $changed = TRUE;
   +        }
   ...
   +      if (!empty($display['display_options']['arguments'])) {
   +        foreach ($display['display_options']['arguments'] as &$argument) {
   ...
   +        }
   +      }
   ...
   +    foreach ($displays as $display_name => &$display) {
   ...
   +      }
   +    }
   ...
   +    foreach ($displays as $display_name => &$display) {
   +      if (!empty($display['display_options']['style'])) {
   ...
   +      }
   +    }
   

   $changed = TRUE; missing

7. +++ b/core/modules/views/views.install
   @@ -117,5 +117,180 @@ function views_update_8001(&$sandbox) {
   +    // Update RSS description field.
   

   not implemented.

Thank you @olli, really good review!

Did anyone check other places that use global tokens for possible injection?

We thought about that previously, this is the reason why it isn't:

    if ($this->options['tokenize']) {
      $value = $this->view->getStyle()->tokenizeValue($value, 0);
    }
    // As we add the globalTokenForm() we also should replace the token here.
    return $this->globalTokenReplace($value);

So the global tokens come afterwards.

base_field: nid?

It indeed seems to be the case that this field is not used at all, well I copied this test view from somewhere.

'entity' is missing

Really good catch!

$changed = TRUE; missing

As I realized, this $changed = TRUE just makes sense when _views_update_8002_token_update() is actually called.

+ // Update RSS description field.

Also very good point!

Is this same as

It indeed is

Ok thanks.

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_field_argument_tokens.yml
@@ -48,7 +48,7 @@ display:
           id: null
           table: views
           field: null
-          plugin_id: ull
+          plugin_id: null

Sorry I meant that isn't it 'null' in quotes, a string.

Sorry I meant that isn't it 'null' in quotes, a string.

Why would yaml care about that? Strings in yml don't need quoets. Maybe I misunderstand you ...

I believe it cares if you need a string "null", "true" or "false".

Oh, you are too clever. Feel free to just patch it, I'm afk

And here it is, please don't credit me.

The failure seems unrelated

Back to RTBC

Indeed this is how an export looks like in real life:

   arguments:
        'null':
          id: 'null'
          table: views
          field: 'null'
          relationship: none
          group_type: group
          admin_label: ''
          default_action: ignore
          exception:
            value: all
            title_enable: false
            title: All
          title_enable: false
          title: ''
          default_argument_type: fixed
          default_argument_options:
            argument: ''
          default_argument_skip_url: false
          summary_options:
            base_path: ''
            count: true
            items_per_page: 25
            override: false
          summary:
            sort_order: asc
            number_of_records: 0
            format: default_summary
          specify_validation: false
          validate:
            type: none
            fail: 'not found'
          validate_options: {  }
          must_not_be: false
          plugin_id: 'null'

So we need some two more adaptions.

I'm working on this fail, but if you know the solution by just looking at it I'd not be mad if you just post a patch;)

@dawehner gave me a hint, the key itself doesn't need to have strings. I ran through tests to ensure that was correct with a breakpoint after the change.

Nope that should have worked actually. digging deeper into the test to see what's up. It looks like the placeholders not there but there is an escaped pre_render still in there. So technically it won't get executed(good), but will try to render that to the screen I believe.

Ok resolved this. YAML treats null: keys as NULL so that gives you {{ arguments. }} which is wrong. @dawehner's export is correct in quoting that value.

The problem was that the arguments passed in to test if #pre_render doesn't execute were being escaped and that would be the output. So I think the correct solution is to change the test to what we expect the output to be based on the input.

YAML treats null: keys as NULL so that gives you {{ arguments. }} which is wrong. @dawehner's export is correct in quoting that value.

Could we use a nid argument and void the whole NULL vs. "null" bit?

YAML treats null: keys as NULL so that gives you {{ arguments. }} which is wrong. @dawehner's export is correct in quoting that value.
Could we use a nid argument and void the whole NULL vs. "null" bit?

We could for sure, but then we would have to set exclude: true in the argument configuration.

+++ b/core/modules/views/src/Tests/Handler/FieldKernelTest.php
@@ -208,7 +208,7 @@ public function testArgumentTokens() {
-    $this->assertEqual(' ', (string) $output, 'Ensure that new style placeholders are replaced');
+    $this->assertEqual('{{ { "#pre_render": ["views_test_data_test_pre_render_function"]} }} {{ { "#pre_render": ["views_test_data_test_pre_render_function"]} }}', (string) $output, 'Ensure that new style placeholders are replaced');

Mh Interesting, but its kinda what we want, indeed

Could we use a nid argument and void the whole NULL vs. "null" bit?

I think we could expand the test coverage in a follow up, don't we?
For now let's use the NULL argument, which works fine, when we use the quotes, as the export does.

Re-rolling.

Just a conflict with someone who did some of the casting to string stuff that we didn't want in this patch was committed already.

Setting back to RTBC.

Whoops my merge missed the $arg => somehow.

Probably not helpful but this is the re-roll interdiff to show what changed since.

Ok enough mistakes to let someone else RTBC:P

You know, things happen, let's see whether its green again ...

Its green, the merge was successful.

Committed 9aec827 and pushed to 8.0.x. Thanks!

quick follow up. #2568415-2: 2492839 added invalid yaml

The change record for this issue (https://www.drupal.org/node/2566251) was still a draft, I published it now :)

Thank you @amateescu