From 1d1a3b1debac26a74bd7e73c906c1749bd859d38 Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 21 Nov 2024 11:15:44 +0200 Subject: [PATCH 1/5] Make sure link text is sanitized --- src/Plugin/Filter/LinkConverter.php | 30 ++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/Plugin/Filter/LinkConverter.php b/src/Plugin/Filter/LinkConverter.php index f59fcb7..6eaa456 100644 --- a/src/Plugin/Filter/LinkConverter.php +++ b/src/Plugin/Filter/LinkConverter.php @@ -4,8 +4,8 @@ namespace Drupal\helfi_api_base\Plugin\Filter; -use Drupal\Component\Render\MarkupInterface; use Drupal\Component\Utility\Html; +use Drupal\Component\Utility\Xss; use Drupal\Core\Language\LanguageManagerInterface; use Drupal\Core\Link; use Drupal\Core\Plugin\ContainerFactoryPluginInterface; @@ -13,24 +13,26 @@ use Drupal\Core\Render\Markup; use Drupal\Core\Render\RenderContext; use Drupal\Core\Render\RendererInterface; +use Drupal\Core\StringTranslation\TranslatableMarkup; +use Drupal\filter\Attribute\Filter; use Drupal\filter\FilterProcessResult; use Drupal\filter\Plugin\FilterBase; +use Drupal\filter\Plugin\FilterInterface; use Drupal\helfi_api_base\Link\UrlHelper; use Psr\Log\LoggerInterface; use Symfony\Component\DependencyInjection\ContainerInterface; /** * Provides a 'Link converter' filter. - * - * @Filter( - * id = "helfi_link_converter", - * title = @Translation("Hel.fi: Link converter"), - * description = @Translation("Runs embedded links through a template. NOTE: This filter must be run after 'Convert URLs into links' filter."), - * type = Drupal\filter\Plugin\FilterInterface::TYPE_TRANSFORM_REVERSIBLE, - * settings = {}, - * weight = -10 - * ) */ +#[Filter( + id: 'helfi_link_converter', + title: new TranslatableMarkup('Hel.fi: Link converter'), + type: FilterInterface::TYPE_TRANSFORM_REVERSIBLE, + description: new TranslatableMarkup("Runs embedded links through a template. NOTE: This filter must be run after 'Convert URLs into links' filter."), + weight: -10, + settings: [], +)] final class LinkConverter extends FilterBase implements ContainerFactoryPluginInterface { /** @@ -155,12 +157,10 @@ private function render(array $build, \DOMNode $node, FilterProcessResult &$resu * @param \DOMElement $node * The node. * - * @todo Review this. - * - * @return \Drupal\Component\Render\MarkupInterface|string + * @return string * The rendered markup or string. */ - private function getLinkText(\DOMElement $node) : MarkupInterface|string { + private function getLinkText(\DOMElement $node) : string { if ($node->childElementCount === 0) { return $node->nodeValue; } @@ -170,7 +170,7 @@ private function getLinkText(\DOMElement $node) : MarkupInterface|string { /** @var \DOMNode $childNode */ $text .= $childNode->C14N(); } - return Markup::create($text); + return Xss::filterAdmin(Markup::create($text)); } /** From b5d263967e9019250b9f7a55e76b5711f2bdb5ab Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 21 Nov 2024 11:34:37 +0200 Subject: [PATCH 2/5] Fixed chain order --- src/Plugin/Filter/LinkConverter.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Plugin/Filter/LinkConverter.php b/src/Plugin/Filter/LinkConverter.php index 6eaa456..0ef7844 100644 --- a/src/Plugin/Filter/LinkConverter.php +++ b/src/Plugin/Filter/LinkConverter.php @@ -4,6 +4,7 @@ namespace Drupal\helfi_api_base\Plugin\Filter; +use Drupal\Component\Render\MarkupInterface; use Drupal\Component\Utility\Html; use Drupal\Component\Utility\Xss; use Drupal\Core\Language\LanguageManagerInterface; @@ -157,10 +158,10 @@ private function render(array $build, \DOMNode $node, FilterProcessResult &$resu * @param \DOMElement $node * The node. * - * @return string + * @return string|\Drupal\Component\Render\MarkupInterface * The rendered markup or string. */ - private function getLinkText(\DOMElement $node) : string { + private function getLinkText(\DOMElement $node) : MarkupInterface|string { if ($node->childElementCount === 0) { return $node->nodeValue; } @@ -170,7 +171,7 @@ private function getLinkText(\DOMElement $node) : string { /** @var \DOMNode $childNode */ $text .= $childNode->C14N(); } - return Xss::filterAdmin(Markup::create($text)); + return Markup::create(Xss::filter($text, ['span'])); } /** From 00d7dd6e6888accace5c84a1cfe24e677f248a01 Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 21 Nov 2024 12:46:02 +0200 Subject: [PATCH 3/5] Simplified link converter logic --- src/Plugin/Filter/LinkConverter.php | 34 ++++--------------- .../Functional/LinkConverterFilterTest.php | 3 +- 2 files changed, 7 insertions(+), 30 deletions(-) diff --git a/src/Plugin/Filter/LinkConverter.php b/src/Plugin/Filter/LinkConverter.php index 0ef7844..8fd1cb6 100644 --- a/src/Plugin/Filter/LinkConverter.php +++ b/src/Plugin/Filter/LinkConverter.php @@ -4,14 +4,10 @@ namespace Drupal\helfi_api_base\Plugin\Filter; -use Drupal\Component\Render\MarkupInterface; use Drupal\Component\Utility\Html; -use Drupal\Component\Utility\Xss; use Drupal\Core\Language\LanguageManagerInterface; -use Drupal\Core\Link; use Drupal\Core\Plugin\ContainerFactoryPluginInterface; use Drupal\Core\Render\BubbleableMetadata; -use Drupal\Core\Render\Markup; use Drupal\Core\Render\RenderContext; use Drupal\Core\Render\RendererInterface; use Drupal\Core\StringTranslation\TranslatableMarkup; @@ -92,8 +88,7 @@ public function process($text, $langcode) : FilterProcessResult { } try { - $build = Link::fromTextAndUrl($this->getLinkText($node), UrlHelper::parse($value)) - ->toRenderable(); + $url = UrlHelper::parse($value); } catch (\InvalidArgumentException) { $this->logger->notice( @@ -101,6 +96,11 @@ public function process($text, $langcode) : FilterProcessResult { ); continue; } + $build = [ + '#type' => 'link', + '#url' => $url, + '#title' => $node->nodeValue, + ]; $build['#attributes'] = $this->getNodeAttributes($node); unset($build['#attributes']['href']); @@ -152,28 +152,6 @@ private function render(array $build, \DOMNode $node, FilterProcessResult &$resu $node->parentNode->removeChild($node); } - /** - * Renders the text. - * - * @param \DOMElement $node - * The node. - * - * @return string|\Drupal\Component\Render\MarkupInterface - * The rendered markup or string. - */ - private function getLinkText(\DOMElement $node) : MarkupInterface|string { - if ($node->childElementCount === 0) { - return $node->nodeValue; - } - // Keep all child elements. - $text = ''; - foreach ($node->childNodes as $childNode) { - /** @var \DOMNode $childNode */ - $text .= $childNode->C14N(); - } - return Markup::create(Xss::filter($text, ['span'])); - } - /** * Renders attributes for given node. * diff --git a/tests/src/Functional/LinkConverterFilterTest.php b/tests/src/Functional/LinkConverterFilterTest.php index d445f5f..245ba13 100644 --- a/tests/src/Functional/LinkConverterFilterTest.php +++ b/tests/src/Functional/LinkConverterFilterTest.php @@ -151,9 +151,8 @@ public function testFilter() : void { $this->assertFalse($children->hasAttribute('data-is-external')); } $element = $this->getSession()->getPage()->find('css', '.nested-dom-link'); - $children = $element->find('css', '.nested'); // Make sure nested tags get run through filter. - $this->assertFalse($children->hasAttribute('onload')); + $this->assertNull($element->find('css', '.nested')); } } From dfd157b8598cd16554e45097ef3bcba951bd4028 Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 21 Nov 2024 12:56:25 +0200 Subject: [PATCH 4/5] Fixed covers annotation --- src/Plugin/Filter/LinkConverter.php | 4 ++-- tests/src/Kernel/Plugin/Filter/LinkConverterTest.php | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Plugin/Filter/LinkConverter.php b/src/Plugin/Filter/LinkConverter.php index 8fd1cb6..7c4d11b 100644 --- a/src/Plugin/Filter/LinkConverter.php +++ b/src/Plugin/Filter/LinkConverter.php @@ -100,10 +100,10 @@ public function process($text, $langcode) : FilterProcessResult { '#type' => 'link', '#url' => $url, '#title' => $node->nodeValue, + '#attributes' => $this->getNodeAttributes($node), ]; - $build['#attributes'] = $this->getNodeAttributes($node); - unset($build['#attributes']['href']); + $this->render($build, $node, $result); } diff --git a/tests/src/Kernel/Plugin/Filter/LinkConverterTest.php b/tests/src/Kernel/Plugin/Filter/LinkConverterTest.php index 302d59a..984c595 100644 --- a/tests/src/Kernel/Plugin/Filter/LinkConverterTest.php +++ b/tests/src/Kernel/Plugin/Filter/LinkConverterTest.php @@ -13,7 +13,7 @@ use Drupal\filter\FilterProcessResult; /** - * Tests custom language negotiator functionality. + * Tests Link converter filter. * * @coversDefaultClass \Drupal\helfi_api_base\Plugin\Filter\LinkConverter * @group helfi_api_base @@ -49,7 +49,6 @@ protected function setUp(): void { * @covers ::create * @covers ::process * @covers ::render - * @covers ::getLinkText * @covers ::getNodeAttributes */ public function testInvalidLink() : void { @@ -61,7 +60,6 @@ public function testInvalidLink() : void { * @covers ::create * @covers ::process * @covers ::render - * @covers ::getLinkText * @covers ::getNodeAttributes * @dataProvider linkProcessingData */ From 9cf87e550c6ceb13b3e7a24d288854eb98980a00 Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 21 Nov 2024 16:08:20 +0200 Subject: [PATCH 5/5] Reverted the change since we actually need to support html tags inside links --- src/Plugin/Filter/LinkConverter.php | 29 ++++++++++++++++++- .../Functional/LinkConverterFilterTest.php | 6 ++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/Plugin/Filter/LinkConverter.php b/src/Plugin/Filter/LinkConverter.php index 7c4d11b..6ac3b73 100644 --- a/src/Plugin/Filter/LinkConverter.php +++ b/src/Plugin/Filter/LinkConverter.php @@ -4,10 +4,13 @@ namespace Drupal\helfi_api_base\Plugin\Filter; +use Drupal\Component\Render\MarkupInterface; use Drupal\Component\Utility\Html; +use Drupal\Component\Utility\Xss; use Drupal\Core\Language\LanguageManagerInterface; use Drupal\Core\Plugin\ContainerFactoryPluginInterface; use Drupal\Core\Render\BubbleableMetadata; +use Drupal\Core\Render\Markup; use Drupal\Core\Render\RenderContext; use Drupal\Core\Render\RendererInterface; use Drupal\Core\StringTranslation\TranslatableMarkup; @@ -99,7 +102,7 @@ public function process($text, $langcode) : FilterProcessResult { $build = [ '#type' => 'link', '#url' => $url, - '#title' => $node->nodeValue, + '#title' => $this->getLinkText($node), '#attributes' => $this->getNodeAttributes($node), ]; unset($build['#attributes']['href']); @@ -152,6 +155,30 @@ private function render(array $build, \DOMNode $node, FilterProcessResult &$resu $node->parentNode->removeChild($node); } + /** + * Renders the text. + * + * @param \DOMElement $node + * The node. + * + * @return \Drupal\Component\Render\MarkupInterface|string + * The rendered markup or string. + */ + private function getLinkText(\DOMElement $node) : MarkupInterface|string { + if ($node->childElementCount === 0) { + return $node->nodeValue; + } + // Keep all child elements. + $text = ''; + foreach ($node->childNodes as $childNode) { + /** @var \DOMNode $childNode */ + $text .= $childNode->C14N(); + } + // Make sure we support HTML inside html tags, such as + // . + return Markup::create(Xss::filterAdmin($text)); + } + /** * Renders attributes for given node. * diff --git a/tests/src/Functional/LinkConverterFilterTest.php b/tests/src/Functional/LinkConverterFilterTest.php index 245ba13..69b8534 100644 --- a/tests/src/Functional/LinkConverterFilterTest.php +++ b/tests/src/Functional/LinkConverterFilterTest.php @@ -151,8 +151,10 @@ public function testFilter() : void { $this->assertFalse($children->hasAttribute('data-is-external')); } $element = $this->getSession()->getPage()->find('css', '.nested-dom-link'); - // Make sure nested tags get run through filter. - $this->assertNull($element->find('css', '.nested')); + $children = $element->find('css', '.nested'); + // Make sure HTML inside links is kept, but sanitized. + $this->assertNotNull($children); + $this->assertFalse($children->hasAttribute('onload')); } }