Follow-up to #2342287: Allow Twig in Views token replacement

Problem/Motivation

#2342287: Allow Twig in Views token replacement introduced the ability to use twig tokens instead of custom views tokens, which means I write {{ nid }} instead of [nid]. This makes a lot of things possible.

However given that %1 and @1 still are replaced dynamically this not only means a lot of different templates are possible, but also that it might be possible to do a 'Twig-Template-Injection' attack.

Let's assume you have a view listening to giraffe/%arg
and use use %1 in one of the template.

An attacker could access a URL including {{ { '#pre_render': ['drupal_flush_all_caches']} }}.
Drupal does a simple $text = str_replace('%1', $value_from_url, $text); and boom, now you basically have a drupal_flush_all_caches()call everytime the view is rendered.

Proposed resolution

Use {{ arguments.name }} instead of @1 and {{ raw_arguments.name }} instead of %1.

Remaining tasks

- Review the patch
- RTBC the patch
- Commit the patch
- Be happy

User interface changes

- Need to use {{ arguments.name }} instead of @1 in views inline templates
- We show available replacement pattern in the common places already

API changes

- Replace @1 and %1 in views with {{ arguments.name }} and {{ raw_arguments.name }}
- Existing views will be automatically be updated.

Original report

One concern security wise for that issue:

  1. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -320,6 +320,57 @@ public function globalTokenReplace($string = '', array $options = array()) {
    +    // 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);
    

    Overall the straight string replacement while good for keeping BC in a way is a security nightmare.

    Because if I do:

    %1{%2

    e.g. I might suddenly - given enough input possibilities - be able to dynamically change the twig template.

    And that is "Twig-Injection-Exploit"?

    I know it might not be possible, but if framework manager would allow breaking BC, the best would be to just use:

    {{ tokens['%1'] }}
    {{ tokens['@1'] }}

    instead or find some shorter variable names like:

    {{ at[1] }} -- @1
    {{ p[1] }} -- %1

Suggested commit credit

git commit -m 'Issue #2492839 by mikeker, joelpittet, dawehner, lauriii, Fabianx, olli: Views replacement token bc layer allows for Twig template injection via arguments'

Files: 
CommentFileSizeAuthor
#154 interdiff.txt2.1 KBjoelpittet
#154 views_replacement_token-2492839-154.patch63.68 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,863 pass(es). View
#154 Fullscreen_2015-09-13__2_10_PM.png485.71 KBjoelpittet
#152 views_replacement_token-2492839-152.patch63.68 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/display/DisplayPluginBase.php. View
#152 interdiff.txt2.1 KBjoelpittet
#151 views_replacement_token-2492839-151.patch63.5 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,852 pass(es), 0 fail(s), and 70 exception(s). View
#145 2492839-145.patch63.45 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,799 pass(es). View
#145 interdiff.txt1.69 KBjoelpittet
#145 views_view_test_field_argument_tokens_yml_-_html_-____Sites_d8_html_.png169.22 KBjoelpittet
#143 views_replacement_token-2492839-143.patch63.27 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,790 pass(es). View
#143 interdiff.txt1.65 KBjoelpittet
#139 interdiff.txt783 bytesdawehner
#139 2492839-139.patch62.85 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,794 pass(es), 1 fail(s), and 0 exception(s). View
#134 interdiff.txt672 bytesplach
#134 2492839-134.patch62.84 KBplach
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,765 pass(es). View
#127 interdiff.txt11.17 KBdawehner
#127 2492839-127.patch62.84 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,764 pass(es). View
#118 interdiff.txt4.1 KBjoelpittet
#118 views_replacement_token-2492839-115.patch61.34 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,763 pass(es). View
#114 views_replacement_token-2492839-114.patch62.04 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,764 pass(es). View
#114 interdiff.txt887 bytesjoelpittet
#113 interdiff.txt6.08 KBjoelpittet
#113 views_replacement_token-2492839-113.patch62.06 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,753 pass(es). View
#109 views_replacement_token-2492839-105.patch60.42 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,049 pass(es). View
#109 interdiff.txt2.06 KBjoelpittet
#101 Test__Content____Site-Install.png50.28 KBjoelpittet
#101 views_replacement_token-2492839-101.patch58.5 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,056 pass(es), 6 fail(s), and 6 exception(s). View
#101 interdiff.txt2.98 KBjoelpittet
#100 views_replacement_token-2492839-99.patch56.9 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,067 pass(es). View
#100 interdiff.txt728 bytesjoelpittet
#100 Test__Content____Site-Install.png109.89 KBjoelpittet
#98 views_replacement_token-2492839-98.patch56.93 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,075 pass(es). View
#98 interdiff.txt2.52 KBjoelpittet
#96 views_replacement_token-2492839-96.patch54.55 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,075 pass(es). View
#96 interdiff.txt1.97 KBjoelpittet
#93 2492839-90-93.interdiff.txt637 bytesmikeker
#93 2492839-93-twig-tokens-views-arguments.patch54.7 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,067 pass(es). View
#90 2492839-88-90.interdiff.txt1.04 KBmikeker
#90 2492839-90-twig-tokens-views-arguments.patch54.49 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,069 pass(es). View
#88 2492839-83-88.interdiff.txt474 bytesmikeker
#88 2492839-88-twig-tokens-views-arguments.patch54.51 KBmikeker
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,050 pass(es). View
#83 interdiff.txt7.51 KBdawehner
#83 2492839-83.patch54.46 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,050 pass(es). View
#80 interdiff.txt6.83 KBdawehner
#80 2492839-80.patch53.58 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,937 pass(es). View
#77 views_replacement_token-2492839-77.patch49.4 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,608 pass(es). View
#77 interdiff.txt835 bytesjoelpittet
#72 2492839-68-72.interdiff.txt10.38 KBmikeker
#72 2492839-72-twig-tokens-views-arguments.patch48.92 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,595 pass(es), 9 fail(s), and 0 exception(s). View
#68 interdiff.txt16.77 KBdawehner
#68 2492839-68.patch49.08 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,587 pass(es), 21 fail(s), and 12 exception(s). View
#61 interdiff.txt8.6 KBdawehner
#61 2492839-61.patch42.26 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,836 pass(es), 4 fail(s), and 3 exception(s). View
#56 interdiff.txt1.8 KBlauriii
#56 views_replacement_token-2492839-56.patch33.46 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,840 pass(es), 1 fail(s), and 3 exception(s). View
#56 interdiff.txt1.8 KBlauriii
#56 views_replacement_token-2492839-56.patch33.46 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,654 pass(es), 1 fail(s), and 2 exception(s). View
#48 interdiff.txt5.86 KBdawehner
#48 2492839-48.patch32.8 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,445 pass(es), 2 fail(s), and 6,604 exception(s). View
#46 2492839-46.patch3.81 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,597 pass(es), 1 fail(s), and 0 exception(s). View
#37 2492839-32-37.interdiff.txt3.67 KBmikeker
#37 2492839-37-twig-tokens-views-arguments.patch23.12 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,525 pass(es), 8 fail(s), and 3 exception(s). View
#32 2492839-32-reroll.interdiff.txt3.18 KBmikeker
#32 2492839-32-twig-tokens-views-arguments.patch23.13 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,449 pass(es), 26 fail(s), and 15 exception(s). View
#28 2492839-26-28.interdiff.txt1.6 KBmikeker
#28 2492839-28-twig-tokens-views-arguments.patch22.61 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,430 pass(es), 0 fail(s), and 2 exception(s). View
#26 2492839-23-26.interdiff.txt5.48 KBmikeker
#26 2492839-26-twig-tokens-views-arguments.patch21.51 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,429 pass(es), 3 fail(s), and 2 exception(s). View
#23 2492839-21-23.interdiff.txt9.48 KBmikeker
#23 2492839-23-twig-tokens-views-arguments.patch20.73 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,884 pass(es), 13 fail(s), and 2 exception(s). View
#21 2492839-18-21.interdiff.txt2.71 KBmikeker
#21 2492839-21-twig-tokens-views-arguments.patch17.88 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,679 pass(es), 25 fail(s), and 0 exception(s). View
#18 2492839-18-twig-tokens-views-arguments.patch15.17 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,671 pass(es), 25 fail(s), and 0 exception(s). View

Comments

Fabianx’s picture

Status: Closed (fixed) » Active
dawehner’s picture

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.

Fabianx’s picture

Dawehner said that 3. is for the UI purposes, which I forgot.

1. is moot then too. I thought it checked which tokens are available in the text, but it does just remove the curly brackets that are only there for the UI.

Means we just keep point 2:

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

He'll comment in here shortly, too.

Fabianx’s picture

Need to update the IS ...

jibran’s picture

Issue tags: +VDC
Fabianx’s picture

Issue summary: View changes
Fabianx’s picture

mikeker’s picture

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.

Fabianx’s picture

I think this issue is orthogonal to the other one, tagging also revisit before release candidate, because while not critical it makes me uncomfortable overall still ...

mikeker’s picture

mikeker’s picture

#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.

mikeker’s picture

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".

Cottser’s picture

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

mariagwyn’s picture

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.

mikeker’s picture

Title: Follow-up: Allow Twig in Views token replacement » Follow-up: Convert Views' %n and !n replacement tokens to Twig syntax

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

Fabianx’s picture

#15: Will you be able to work on this in the near future?

We are getting ever closer to release candidate and I fear I will need to make that issue critical then (at RC 1 checklist time) (for security reasons), which would probably mean that the original issue is rolled back for simplicity.

I still think this is totally doable with less effort, so not doing so, yet.

mikeker’s picture

@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...

mikeker’s picture

Status: Active » Needs review
FileSize
15.17 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,671 pass(es), 25 fail(s), and 0 exception(s). View

Work in progress...

mikeker’s picture

+++ 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.

Status: Needs review » Needs work

The last submitted patch, 18: 2492839-18-twig-tokens-views-arguments.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
17.88 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,679 pass(es), 25 fail(s), and 0 exception(s). View
2.71 KB

This should fix a few tests...

Status: Needs review » Needs work

The last submitted patch, 21: 2492839-21-twig-tokens-views-arguments.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
20.73 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,884 pass(es), 13 fail(s), and 2 exception(s). View
9.48 KB

Updates some docs and description fields and fixes some tests.

mikeker’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 23: 2492839-23-twig-tokens-views-arguments.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
21.51 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,429 pass(es), 3 fail(s), and 2 exception(s). View
5.48 KB

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.

Status: Needs review » Needs work

The last submitted patch, 26: 2492839-26-twig-tokens-views-arguments.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
22.61 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,430 pass(es), 0 fail(s), and 2 exception(s). View
1.6 KB

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.

Status: Needs review » Needs work

The last submitted patch, 28: 2492839-28-twig-tokens-views-arguments.patch, failed testing.

joelpittet’s picture

Issue tags: +Twig

Tagging for twig visibility

jonathanshaw’s picture

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.

mikeker’s picture

Status: Needs work » Needs review
FileSize
23.13 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,449 pass(es), 26 fail(s), and 15 exception(s). View
3.18 KB

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).

mikeker’s picture

+++ 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.

Status: Needs review » Needs work

The last submitted patch, 32: 2492839-32-twig-tokens-views-arguments.patch, failed testing.

olli’s picture

  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.

catch’s picture

mikeker’s picture

Status: Needs work » Needs review
FileSize
23.12 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,525 pass(es), 8 fail(s), and 3 exception(s). View
3.67 KB

#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...

Status: Needs review » Needs work

The last submitted patch, 37: 2492839-37-twig-tokens-views-arguments.patch, failed testing.

Fabianx’s picture

I was able to easily reproduce this with a view by adding a Node: ID argument and using exclude rather than include, but then I was not able to actually break out of twig land to do something ...

So putting in argument: {{ nid }} or title worked well, but difficult to do something else as there is no object available in that scope.

I have not yet tested what I can do when I create a render element on the fly however.

Fabianx’s picture

Priority: Major » Critical

Okay, I found a way not to delete an entity (#lazy builder and #pre_render are amazingly well protected against anything not using scalar values), but to at least DDOS the site:

Given a view that has a Node: ID argument that is set to exclude rather than filter and a rewritten views field like title that uses that argument either %1 or !1 you can use:

views/[name]/{{ { '#pre_render': ['drupal_flush_all_caches']} }}

OR

views/[name]/{{ { '#lazy_builder': ['drupal_flush_all_caches', []]} }}

OR

views/[name]/{{ { '#post_render': ['drupal_flush_all_caches']} }}

Given that and because catch asked me to make this critical before todays Critical Triage Call, I am marking this critical.

dawehner’s picture

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.

Fabianx’s picture

Given this now has an 'exploit' of sorts, this is now a security issue (D7 not affected obviously) and hence adding 'security' tag.

fgm’s picture

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 ?

Fabianx’s picture

#43: No, any view that exposes an argument and where the trusted user has added a Node ID argument for exclusion but used that within the view somewhere in a rewrite:

e.g.

mycontent-page/1

shows:

Here is all content except for Node '%1' and it excludes the Node ID that was added in the URL.

That scenario is artificially constructed, but I know that I used 'exclude' in real views before in D7 and I also know I used to show arguments in rewrites as well (or used them in decision making), so it is not totally artificial ...

The problem is that the way the strtr() works is inviting the user that puts the argument into the url to change the twig template before it is displayed.

e,g,

Consider the following way:

- Using an argument for exclude with the following template (which would indeed be a misuse of arguments, but ...):

{% set nid_to_exclude = %1 %}
{% if (nid_to_exclude == 2) %}
  {{ foo_field }}
{% else %}
  {{ bar_field }}
{% endif %}

If a user now enters a 'template' into the URL as argument %1 as suggested they can change this template into:

{% set nid_to_exclude = {{ { '#post_render': ['drupal_flush_all_caches'], '#prefix': '2'} }}|render %}
{% if (nid_to_exclude == 2) %}
  {{ foo_field }}
{% else %}
  {{ bar_field }}
{% endif %}

Basically where the %1 was the user of the site has full control of the twig template. I had hoped it was not possible to exploit this (major bug), but as I was able to, so it is now critical.

catch’s picture

Title: Follow-up: Convert Views' %n and !n replacement tokens to Twig syntax » Views replacement token bc layer allows for Twig template injection via arguments

Changing the title to reflect the bug a bit better.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.81 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,597 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 46: 2492839-46.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
32.8 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,445 pass(es), 2 fail(s), and 6,604 exception(s). View
5.86 KB

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.

Status: Needs review » Needs work

The last submitted patch, 48: 2492839-48.patch, failed testing.

pwolanin’s picture

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

dawehner’s picture

mikeker’s picture

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.)

xjm’s picture

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

xjm’s picture

Issue tags: -rc target

Ah, as is "rc target".

Fabianx’s picture

#50:

The criticalness is not the naming, but the fact that user input is executed as a twig template.

So even if we had:

- %really_long_name it would still be problematic.

And there is no technical difference - except for the user / UX - between {{ arguments.1 }} and {{ arguments.some_nice_name }}.

Both would solve the issue as then whatever you put in as argument would in Twig land then just be a string, which you then can print, but which is auto-escaped, etc.

So this would all be fine.

lauriii’s picture

Status: Needs work » Needs review
FileSize
33.46 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,654 pass(es), 1 fail(s), and 2 exception(s). View
1.8 KB
33.46 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,840 pass(es), 1 fail(s), and 3 exception(s). View
1.8 KB

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

The last submitted patch, 56: views_replacement_token-2492839-56.patch, failed testing.

The last submitted patch, 56: views_replacement_token-2492839-56.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 56: views_replacement_token-2492839-56.patch, failed testing.

The last submitted patch, 56: views_replacement_token-2492839-56.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
42.26 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,836 pass(es), 4 fail(s), and 3 exception(s). View
8.6 KB

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

Status: Needs review » Needs work

The last submitted patch, 61: 2492839-61.patch, failed testing.

The last submitted patch, 56: views_replacement_token-2492839-56.patch, failed testing.

The last submitted patch, 61: 2492839-61.patch, failed testing.

joelpittet’s picture

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.

  2. 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.

  3. +++ 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()

  4. +++ 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?

  5. +++ 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

  6. +++ 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.

  7. +++ 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.

  8. +++ 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.

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

    nit: remove extra line break.

dawehner’s picture

Assigned: Unassigned » dawehner

Working on this for a while.

Thank you for the review @joelpittet

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
FileSize
49.08 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,587 pass(es), 21 fail(s), and 12 exception(s). View
16.77 KB

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

Status: Needs review » Needs work

The last submitted patch, 68: 2492839-68.patch, failed testing.

joelpittet’s picture

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.

mikeker’s picture

Assigned: Unassigned » mikeker

Working on fixing the tests...

mikeker’s picture

Assigned: mikeker » Unassigned
Status: Needs work » Needs review
FileSize
48.92 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,595 pass(es), 9 fail(s), and 0 exception(s). View
10.38 KB

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

The last submitted patch, 68: 2492839-68.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 72: 2492839-72-twig-tokens-views-arguments.patch, failed testing.

The last submitted patch, 72: 2492839-72-twig-tokens-views-arguments.patch, failed testing.

joelpittet’s picture

Assigned: Unassigned » joelpittet

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

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs work » Needs review
FileSize
835 bytes
49.4 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,608 pass(es). View

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

joelpittet’s picture

Status: Needs review » Needs work
  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.

mikeker’s picture

#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!

dawehner’s picture

Status: Needs work » Needs review
FileSize
53.58 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,937 pass(es). View
6.83 KB
+++ 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.

dawehner’s picture

Issue summary: View changes
Issue tags: -Needs change record updates

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

lauriii’s picture

  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

dawehner’s picture

FileSize
54.46 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,050 pass(es). View
7.51 KB

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!

Status: Needs review » Needs work

The last submitted patch, 83: 2492839-83.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

Seemed to be a random test failure.

dawehner’s picture

Issue summary: View changes

Cleaned up the remaining tasks

Status: Needs review » Needs work

The last submitted patch, 83: 2492839-83.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
54.51 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,050 pass(es). View
474 bytes

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.

Fabianx’s picture

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

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

    Overall the tokens should be just strings, but just saying (and not sure that is good or bad), this limits e.g. the capability to attach a library (don't do that!) or render something more complex in the future.

  2. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
    @@ -211,7 +211,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    -      '#description' => $this->t('Override the view and other argument titles. Use "%1" for the first argument, "%2" for the second, etc.'),
    +      '#description' => $this->t('Override the view and other argument titles. Use may use Twig syntax in this field. Use "argument.1" for the first argument, "argument.2" for the second, etc.'),
    

    This change is wrong now that we use the machine names.

  3. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
    @@ -348,6 +365,46 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +  protected function getTokenHelp() {
    ...
    +        '#markup' => '<p>' . $this->t("The following replacement tokens are available for this argument.") . '</p>',
    

    Nice!

  4. +++ b/core/modules/views/src/Tests/Plugin/PluginBaseTest.php
    @@ -43,6 +43,16 @@ public function testViewsTokenReplace() {
    +    $result = $this->testPluginBase->viewsTokenReplace($text, $tokens);
    

    I believe this is what fails if that other thing is ->render() instead of renderPlain().

    Should be wrapped in executeInRenderContext(new RenderContext(), function() {

    });

    then it works.

Besides that, looks great!

mikeker’s picture

FileSize
54.49 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,069 pass(es). View
1.04 KB

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!

dawehner’s picture

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++

Fabianx’s picture

#90: Given this is critical and there are no assets currently, this is fine , if:

- we have a major follow-up bug / limitation issue
- and add a @todo pointing to that new issue

Thanks!

mikeker’s picture

dawehner’s picture

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.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

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.

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
1.97 KB
54.55 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,075 pass(es). View

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.

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs work » Needs review

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

joelpittet’s picture

FileSize
2.52 KB
56.93 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,075 pass(es). View

Ok that was easier than I thought:)

mikeker’s picture

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

joelpittet’s picture

Issue summary: View changes
FileSize
109.89 KB
728 bytes
56.9 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,067 pass(es). View

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):

joelpittet’s picture

Issue summary: View changes
FileSize
2.98 KB
58.5 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,056 pass(es), 6 fail(s), and 6 exception(s). View
50.28 KB

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().

joelpittet’s picture

Assigned: joelpittet » Unassigned

Ok feel free to RTBC away @lauriii;)

Status: Needs review » Needs work

The last submitted patch, 101: views_replacement_token-2492839-101.patch, failed testing.

The last submitted patch, 101: views_replacement_token-2492839-101.patch, failed testing.

mikeker’s picture

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.

mikeker’s picture

Status: Needs work » Needs review
lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Thanks joelpittet and mikeker for the additional fixes!

lauriii’s picture

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

joelpittet’s picture

FileSize
2.06 KB
60.42 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,049 pass(es). View

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

joelpittet’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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] == &amp;lt;marquee&amp;gt;test&amp;lt;/marquee&amp;gt;', 'Token label is properly escaped.');
    -    $this->assertRaw('[title_1] == &amp;lt;script&amp;gt;alert(&amp;quot;XSS&amp;quot;)&amp;lt;/script&amp;gt;', 'Token label is properly escaped.');
    +    $this->assertRaw('{{ title }} == &amp;lt;marquee&amp;gt;test&amp;lt;/marquee&amp;gt;', 'Token label is properly escaped.');
    +    $this->assertRaw('{{ title_1 }} == &amp;lt;script&amp;gt;alert(&amp;quot;XSS&amp;quot;)&amp;lt;/script&amp;gt;', '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 :)

joelpittet’s picture

Assigned: Unassigned » joelpittet

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

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs work » Needs review
FileSize
62.06 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,753 pass(es). View
6.08 KB

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 ' == '

joelpittet’s picture

FileSize
887 bytes
62.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,764 pass(es). View

Self-reivew.

alexpott’s picture

Status: Needs review » Needs work

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 }} == &amp;lt;marquee&amp;gt;test&amp;lt;/marquee&amp;gt;', 'Token label is properly escaped.');
-    $this->assertRaw('{{ title_1 }} == &amp;lt;script&amp;gt;alert(&amp;quot;XSS&amp;quot;)&amp;lt;/script&amp;gt;', 'Token label is properly escaped.');
+    $this->assertRaw('{{ title }} == &lt;marquee&gt;test&lt;/marquee&gt;', 'Token label is properly escaped.');
+    $this->assertRaw('{{ title_1 }} == &lt;script&gt;alert(&quot;XSS&quot;)&lt;/script&gt;', 'Token label is properly escaped.');
+    $this->assertNoRaw('&amp;lt;', 'The page does not have double escaped HTML tags.');

Let's revert this.

dawehner’s picture

Issue tags: +D8 Accelerate

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 }} == &lt;marquee&gt;test&lt;/marquee&gt;', 'Token label is properly escaped.');
+    $this->assertRaw('{{ title_1 }} == &lt;script&gt;alert(&quot;XSS&quot;)&lt;/script&gt;', 'Token label is properly escaped.');
+    $this->assertNoRaw('&amp;lt;', '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

alexpott’s picture

joelpittet’s picture

Status: Needs work » Needs review
FileSize
61.34 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,763 pass(es). View
4.1 KB

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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

@joelpittet
Feel free to fix my other points.

joelpittet’s picture

@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
dawehner’s picture

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

mikeker’s picture

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! :)

mikeker’s picture

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.

joelpittet’s picture

Issue summary: View changes

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'

davidhernandez’s picture

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.

olli’s picture

Status: Reviewed & tested by the community » Needs work
  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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
62.84 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,764 pass(es). View
11.17 KB

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

Status: Needs review » Needs work

The last submitted patch, 127: 2492839-127.patch, failed testing.

olli’s picture

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.

joelpittet queued 127: 2492839-127.patch for re-testing.

dawehner’s picture

Status: Needs work » Needs review

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 ...

olli’s picture

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

dawehner’s picture

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

plach’s picture

FileSize
62.84 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,765 pass(es). View
672 bytes

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

Status: Needs review » Needs work

The last submitted patch, 134: 2492839-134.patch, failed testing.

plach queued 134: 2492839-134.patch for re-testing.

plach’s picture

Status: Needs work » Needs review

The failure seems unrelated

plach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

dawehner’s picture

FileSize
62.85 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,794 pass(es), 1 fail(s), and 0 exception(s). View
783 bytes

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 139: 2492839-139.patch, failed testing.

The last submitted patch, 139: 2492839-139.patch, failed testing.

joelpittet’s picture

Assigned: Unassigned » joelpittet

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;)

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs work » Needs review
FileSize
1.65 KB
63.27 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,790 pass(es). View

@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.

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs review » Needs work

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.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
169.22 KB
1.69 KB
63.45 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,799 pass(es). View

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.

mikeker’s picture

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?

dawehner’s picture

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('{{ { &quot;#pre_render&quot;: [&quot;views_test_data_test_pre_render_function&quot;]} }} {{ { &quot;#pre_render&quot;: [&quot;views_test_data_test_pre_render_function&quot;]} }}', (string) $output, 'Ensure that new style placeholders are replaced');

Mh Interesting, but its kinda what we want, indeed

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 145: 2492839-145.patch, failed testing.

joelpittet’s picture

Assigned: Unassigned » joelpittet

Re-rolling.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
63.5 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,852 pass(es), 0 fail(s), and 70 exception(s). View

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.

joelpittet’s picture

FileSize
2.1 KB
63.68 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/views/src/Plugin/views/display/DisplayPluginBase.php. View

Whoops my merge missed the $arg => somehow.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 152: views_replacement_token-2492839-152.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
485.71 KB
63.68 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,863 pass(es). View
2.1 KB

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

Ok enough mistakes to let someone else RTBC:P

The last submitted patch, 151: views_replacement_token-2492839-151.patch, failed testing.

The last submitted patch, 151: views_replacement_token-2492839-151.patch, failed testing.

The last submitted patch, 152: views_replacement_token-2492839-152.patch, failed testing.

dawehner’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Its green, the merge was successful.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 9aec827 on 8.0.x
    Issue #2492839 by joelpittet, mikeker, dawehner, lauriii, plach, Fabianx...
neclimdul’s picture

Status: Fixed » Closed (fixed)

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

amateescu’s picture

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

joelpittet’s picture

Thank you @amateescu