We need to use tokens for correct links in ie #2017471: Multilingual tour for language section

Attached one liner fixes this.

CommentFileSizeAuthor
#14 tour-token-2019469.fail_.patch1.57 KBlarowlan
#14 tour-token-2019469.pass_.patch5.48 KBlarowlan
#11 core-tour-use-token-2019469-11.txt1.74 KBclemens.tolboom
#5 core-tour-use-token-2019469-5.patch2.38 KBclemens.tolboom
core-tour-use-token.patch925 bytesclemens.tolboom

Comments

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Status: Active » Needs review
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: +Needs tests

Woot! Needs tests :)

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

What test do we need? We just call \Drupal::token()->replace() which is tested through core/modules/system/lib/Drupal/system/Tests/System/TokenReplaceTest.php

I'm not sure what other tests we could create?

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Have a tour body that has a token that is replaced?

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
StatusFileSize
new core-tour-use-token-2019469-5.patch2.38 KB

Attached patch has a Token replacement test added.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: -Needs tests

Test looks good to me :) This feature will be vital to add links to tips for example where the website URL could need any path prefix.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Unless bot disagrees
Great work

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

+1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed e89fbf3 and pushed to 8.x. Thanks!

pwieck’s picture

pwieck commented
Priority: Normal » Major
Status: Fixed » Needs work

I have just pulled the latest release and using tokens in tours is not working correctly.

Example:

Remember: To make use of this page you must first <a href="[site:url]/admin/config/regional/language">add a language</a>

The link turns into: http://drual.dev/admin/config/regional/url]/admin/config/regional/language
Copy link location makes this: http://drupal.dev/admin/config/regional/url%5D/admin/config/regional/language

Does the small thing if you use [site:name] as in the test. You get 'name]' as the output.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
StatusFileSize
new core-tour-use-token-2019469-11.txt1.74 KB

@pwieck thanks for reporting.

I guess you found a bug in token replacement.

Changing the test for our tour into

body: Is [site:name] always the best dressed? Check <a href="[site:url]/admin/config/regional/language">add a language</a>

outputs indeed wrong replacement with leftover "url]"

<p id="tour-tip-tour-test-1-contents" class="tour-tip-body">Is Drupal always the best dressed? Check <a href="url]/admin/config/regional/language">add a language</a></p>

Adding this (see attachment) to core/modules/system/lib/Drupal/system/Tests/System/TokenReplaceTest.php does not result into the mentioned error (containing a leftover ]) :(

Test result snippet while running System/Token replacement:

<td>Valid tokens replaced while invalid tokens cleared out.&lt;blink&gt;Blinking Text&lt;/blink&gt;v8prvVbr0 secadmin06/17/2013 - 08:26<a href="http://drupal.d8//admin/config/regional/language">add a language</a></td>

I'm puzzled :/

pwieck’s picture

pwieck commented

Any progress on this so we can start tours that need links?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

I have fix, will sort in next 20 min

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review
StatusFileSize
new tour-token-2019469.pass_.patch5.48 KB
new tour-token-2019469.fail_.patch1.57 KB

First a fail to demonstrate that the url token doesn't work.
Then a pass.
Whilst I was at it I removed the call to \Drupal from inside the plugin and injected the token service (which is the right way to do it, calling out to \Drupal is frowned upon from OO code) and re-ordered the use statements into alpha.

Basic difference was filter_xss_admin had to go after token replacement, not before.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Status: Needs review » Needs work

@larowlan do you know why it failed? I still suspect a bug in token somehow.

Patch from #14 looks ok to me. It's very instructive too :)

+++ b/core/modules/tour/lib/Drupal/tour/Plugin/tour/tip/TipPluginText.php
@@ -7,15 +7,18 @@
+use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
+use Drupal\Core\Utility\Token;
 use Drupal\tour\Annotation\Tip;
 use Drupal\tour\TipPluginBase;
+use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**

I guess this change should be done to TipPluginImage too which resides in the tests.

+++ b/core/modules/tour/lib/Drupal/tour/Plugin/tour/tip/TipPluginText.php
@@ -83,7 +117,7 @@ public function getAttributes() {
-    $output .= '<p class="tour-tip-body" id="tour-tip-' . $this->getAriaId() . '-contents">' . \Drupal::token()->replace(filter_xss_admin($this->getBody())) . '</p>';
+    $output .= '<p class="tour-tip-body" id="tour-tip-' . $this->getAriaId() . '-contents">' . filter_xss_admin($this->token->replace($this->getBody())) . '</p>';

I'm not sure this swapping is ok for tours. Are tokens XSS save or not? The new line will ie filter out images which tour could use.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review

It failed because filter_xss_admin strips stuff before token.

$foo = '<a href="[site:url]/foo">Foo me</a>';
echo filter_xss_admin($foo));

outputs

<a href="url]/foo">Foo me</a>

Doing it the other way is fine, filter_xss_admin only strips XSS attacks so if you're doing nothing wrong, nothing to fear.

$bar = '<img src="[site:url]image.jpg" alt="monkies" />';
$tokened = Drupal::token()->replace($bar);
echo $tokened;
echo filter_xss_admin($tokened);

outputs

<img src="http://d82.taco/image.jpg" alt="monkies" />
<img src="http://d82.taco/image.jpg" alt="monkies" />

(my dev site is d82.taco)

That fail count is wrong, random failures - should only be 1 - I will re-test.
We don't need to make the changes to the image tip, it doesn't need injected services from the container.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

#14: tour-token-2019469.fail_.patch queued for re-testing.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

@larowlan thanks for explaining :)

I'm still puzzled as

- [site:name] passes the test in #5 while[site:url] fails

- Reading https://api.drupal.org/api/drupal/core!includes!common.inc/function/filt... the img tag is not allowed.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

We use filter_xss_admin, not filter_xss :).

The difference with [site:url] is its inside a href attribute, filter_xss_admin sanitizes the href against malicious urls.

echo filter_xss_admin('[site:url]');

outputs [site:url]

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Looks like a great fix! Good find on the bug :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@larowlan considering you are making the effort to make the tour plugin manager use the ContainerFactory how about converting the manager to extend Drupal\Core\Plugin\DefaultPluginManager?

Leaving at rtbc in case you consider this out-of-scope...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Out discussed this with @larowlan in IRC decided that converting the manager to DefaultPluginManager is out of scope...

Committed 7979dcd and pushed to 8.x. Thanks!
.

Status: Fixed » Closed (fixed)

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