Problem/Motivation

The SafeMarkup::format() use in the LinkGenerator is a bad example. Let's ditch it.

 $result = SafeMarkup::format('<a@attributes>@text</a>', array('@attributes' => new Attribute($attributes), '@text' => $variables['text']));

This type of markup building where attributes is just being shoved is wrong. Also at the moment hook_link_alter() can change $variables['text'] without being concerned with safeness which is a security bug waiting to happen. The hook was introduced in #2047619: Add a link generator service for route-based links

Proposed resolution

Make GeneratedLink a SafeString. This has many advantages as we no longer have Drupal::l() returning a string sometimes and an object others. It always returns an object.

This issue also unblocks #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list

Remaining tasks

  • Review
  • Commit

User interface changes

None

API changes

LinkGenerator::generate() always returns a GeneratedLink.

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
3.67 KB
dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
@@ -488,16 +488,16 @@ public function testGenerateBubbleableMetadata() {
-    $this->assertSame($expected_link_markup, $this->linkGenerator->generate('Test', $url));
+    $this->assertSame($expected_link_markup, (string) $this->linkGenerator->generate('Test', $url));
     $generated_link = $this->linkGenerator->generate('Test', $url, TRUE);
-    $this->assertSame($expected_link_markup, $generated_link->getGeneratedLink());
+    $this->assertSame($expected_link_markup, (string) $generated_link->getGeneratedLink());
     $this->assertInstanceOf('\Drupal\Core\Render\BubbleableMetadata', $generated_link);
 
     // Test ::generateFromLink().
     $link = new Link('Test', $url);
-    $this->assertSame($expected_link_markup, $this->linkGenerator->generateFromLink($link));
+    $this->assertSame($expected_link_markup, (string) $this->linkGenerator->generateFromLink($link));
     $generated_link = $this->linkGenerator->generateFromLink($link, TRUE);
-    $this->assertSame($expected_link_markup, $generated_link->getGeneratedLink());
+    $this->assertSame($expected_link_markup, (string) $generated_link->getGeneratedLink());

Can we have at least one dedicated assertInstanceOf?

Status: Needs review » Needs work

The last submitted patch, 2: 2568977-2.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
5.6 KB

@dawehner we can check every link asserted against :)

The fail in #2 is fun.

The last submitted patch, 2: 2568977-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2568977-5.patch, failed testing.

Status: Needs work » Needs review

lauriii queued 5: 2568977-5.patch for re-testing.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -161,7 +169,10 @@ public function generate($text, Url $url, $collect_bubbleable_metadata = FALSE)
+    // either rendered or escaped.
+    $result = SafeString::create('<a' . $attributes . '>' . $variables['text'] . '</a>');
 

What I wonder whether we can consider SafeString still as internal then. The link generator is not really part of the rendering system, as we see in the namespace

alexpott’s picture

So how about this then...

dawehner’s picture

It would be nice if this works!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice solution!

dawehner’s picture

dawehner’s picture

This was a bit premature, I think

The last submitted patch, 10: 2568977-10.patch, failed testing.

fgm’s picture

Tiny commenting error, I think.

+++ b/core/lib/Drupal/Core/Utility/LinkGeneratorInterface.php
@@ -64,7 +64,7 @@
    *   An HTML string containing a link to the given route and parameters.

This is now always a GeneratedLink, so comment needs to be fixed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2568977-14.patch, failed testing.

plach queued 14: 2568977-14.patch for re-testing.

The last submitted patch, 14: 2568977-14.patch, failed testing.

lauriii queued 14: 2568977-14.patch for re-testing.

The last submitted patch, 14: 2568977-14.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.66 KB
16.83 KB

Hmmm... yet another untested hook... hook_link_alter(). This patch now closes a security hole in hook_link_alter().

alexpott’s picture

Issue summary: View changes
Wim Leers’s picture

This patch now closes a security hole in hook_link_alter().

:O I've never even heard of that hook.


Alex Pott pinged me:

12:48:07 <alexpott> WimLeers: one thought that crosses my mind is what is the point of the $collect_bubbleable_metadata now?
12:48:50 <WimLeers> alexpott: looking…
12:49:29 <WimLeers> alexpott: oh wow
12:49:43 <WimLeers> alexpott: this is basically what I wanted to do in the first place, but the concern was "too much BC breakage"
12:49:52 <alexpott> WimLeers: like is it possible to fix Url::toString not returning a string because that is nonsense
12:50:04 <alexpott> WimLeers: not much bc break here :)
12:50:08 <WimLeers> Very interesting
12:50:21 <alexpott> WimLeers: SafeStrings are quite cool
12:50:28 <WimLeers> :P
12:50:59 <alexpott> WimLeers: and once t() returns them everything has to deal with them... so this is actually a very very small change
12:51:36 <alexpott> WimLeers: basically we did all the prep for this whilst doing the prep for the t() (storing out assertions etc)
12:52:27 <WimLeers> alexpott: The only reason left to still have $collect_bubbleable_metadata, is as a minor perf optimization, to not do that work in cases where we know we won't need cacheability metadata (think e-mails).
12:52:45 <alexpott> WimLeers: also why would you do this... RedirectResponse($batch_url->setAbsolute()->toString(TRUE)->getGeneratedUrl());
12:52:47 <WimLeers> alexpott: but that's such a minority case that I'd argue it's better to KISS, and just get rid of that param
12:53:19 <alexpott> WimLeers: I would love to solve Url::toString() too... because that is wtf inducing
12:53:26 <WimLeers> alexpott: +1

So, +1 to removing $collect_bubbleable_metadata, and just always doing it. It simplifies the Link generation API.

alexpott’s picture

alexpott’s picture

So should we do the $collect_bubbleable_metadata removal in this issue? One argument for is that it keeps it consistent with HEAD - if you get a generated link from the generator it has the metadata there. One argument to not do that is scope. Thoughts?

Status: Needs review » Needs work

The last submitted patch, 22: 2568977-22.patch, failed testing.

The last submitted patch, 22: 2568977-22.patch, failed testing.

Wim Leers’s picture

+++ b/core/lib/Drupal.php
@@ -546,10 +546,10 @@ public static function linkGenerator() {
-   * @return string|\Drupal\Core\GeneratedLink
+   * @return \Drupal\Core\GeneratedLink
    *   An HTML string containing a link to the given route and parameters.
-   *   When $collect_bubbleable_metadata is TRUE, a GeneratedLink object is
-   *   returned, containing the generated link plus bubbleable metadata.
+   *   When $collect_bubbleable_metadata is TRUE, the GeneratedLink object has
+   *   bubbleable metadata.

+++ b/core/lib/Drupal/Core/Link.php
@@ -144,10 +144,8 @@ public function setUrl(Url $url) {
-   * @return string|\Drupal\Core\GeneratedLink
+   * @return \Drupal\Core\GeneratedLink
    *   The link HTML markup.
-   *   When $collect_bubbleable_metadata is TRUE, a GeneratedLink object is
-   *   returned, containing the generated link plus bubbleable metadata.

+++ b/core/lib/Drupal/Core/Routing/LinkGeneratorTrait.php
@@ -35,7 +35,7 @@
-   * @return string
+   * @return \Drupal\Core\GeneratedLink

+++ b/core/lib/Drupal/Core/Utility/LinkGeneratorInterface.php
@@ -64,10 +64,10 @@
-   * @return string|\Drupal\Core\GeneratedLink
+   * @return \Drupal\Core\GeneratedLink
    *   An HTML string containing a link to the given route and parameters.
-   *   When $collect_bubbleable_metadata is TRUE, a GeneratedLink object is
-   *   returned, containing the generated link plus bubbleable metadata.
+   *   When $collect_bubbleable_metadata is TRUE, the GeneratedLink object has
+   *   bubbleable metadata.

@@ -88,10 +88,10 @@ public function generate($text, Url $url, $collect_bubbleable_metadata = FALSE);
-   * @return string|\Drupal\Core\GeneratedLink
+   * @return \Drupal\Core\GeneratedLink
    *   An HTML string containing a link to the given route and parameters.
-   *   When $collect_bubbleable_metadata is TRUE, a GeneratedLink object is
-   *   returned, containing the generated link plus bubbleable metadata.
+   *   When $collect_bubbleable_metadata is TRUE, the GeneratedLink object has
+   *   bubbleable metadata.

This is what makes it IMHO within scope: the reason this $collect_bubbleable_metadata parameter existed, was to get different return values.

If that's no longer the case, there's very, very little remaining value to keeping it a separate parameter, and IMHO is mainly just unnecessarily confusing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
31.13 KB
19.62 KB

So yet again this current crop of SafeMarkup::format() find more bugs. This time in FieldPluginBase::renderAsLink(). That is concatenating markup together without a care in the world for safeness! The correct fix here is to change that to a render array.

This patch also removes $collect_bubbleable_metadata from the LinkGenerator.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Link.php
    @@ -150,8 +150,8 @@ public function setUrl(Url $url) {
    -  public function toString($collect_bubbleable_metadata = FALSE) {
    -    return $this->getLinkGenerator()->generateFromLink($this, $collect_bubbleable_metadata);
    +  public function toString() {
    

    Docblock not updated.

  2. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
    @@ -155,18 +155,18 @@ public function generate($text, Url $url, $collect_bubbleable_metadata = FALSE)
    +    // External urls can not have cacheable metadata.
    

    Nit: s/urls/URLs/

  3. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -1533,19 +1535,20 @@ protected function renderAsLink($alter, $text, $tokens) {
    -    $value = '';
    +    $render = [
    +      '#type' => 'link',
    +      '#title' => $text,
    +      '#url' => $final_url,
    +    ];
     
         if (!empty($alter['prefix'])) {
    -      $value .= Xss::filterAdmin($this->viewsTokenReplace($alter['prefix'], $tokens));
    +      $render['#prefix'] = $this->viewsTokenReplace($alter['prefix'], $tokens);
         }
    -
    -    $value .= $this->linkGenerator()->generate($text, $final_url);
    -
         if (!empty($alter['suffix'])) {
    -      $value .= Xss::filterAdmin($this->viewsTokenReplace($alter['suffix'], $tokens));
    +      $render['#suffix'] = $this->viewsTokenReplace($alter['suffix'], $tokens);
    

    So much better! :)

  4. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -1533,19 +1535,20 @@ protected function renderAsLink($alter, $text, $tokens) {
    +    return $this->getRenderer()->render($render);
    

    Aha, this is how you got around the fact that the Views render pipeline is string-based, i.e. this is why you are able to use a render array above.

    Makes sense. And is consistent with prior behavior wrt bubbling: the bubbling is now done by the renderer, previously it was done by the link generator.

  5. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
    @@ -112,8 +112,10 @@ public function testGenerateHrefs($route_name, array $parameters, $absolute, $ex
    +      ->willReturnCallback(function() use ($expected_url) {
    +        $url = new GeneratedUrl();
    +        return $url->setGeneratedUrl($expected_url);
    +      });
    
    @@ -134,7 +136,10 @@ public function testGenerate() {
    +      ->willReturnCallback(function() {
    +        $url = new GeneratedUrl();
    +        return $url->setGeneratedUrl('/test-route-1#the-fragment');
    +      });
    
    @@ -198,7 +203,7 @@ public function testGenerateUrlWithQuotes() {
    +      ->willReturn((new GeneratedUrl())->setGeneratedUrl('/example?foo=%22bar%22&zoo=baz'));
    

    Could be simplified to:

    ->willReturn((new GeneratedUrl())->setGeneratedUrl($expected_url));
    

    because in other places (last quoted bit), that is done too.

    Updated.

Addressed all my nits, which is… all my feedback. This looks great.

dawehner’s picture

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1533,19 +1535,20 @@ protected function renderAsLink($alter, $text, $tokens) {
     if (!empty($alter['prefix'])) {
-      $value .= Xss::filterAdmin($this->viewsTokenReplace($alter['prefix'], $tokens));
+      $render['#prefix'] = $this->viewsTokenReplace($alter['prefix'], $tokens);
     }
...
     if (!empty($alter['suffix'])) {
-      $value .= Xss::filterAdmin($this->viewsTokenReplace($alter['suffix'], $tokens));
+      $render['#suffix'] = $this->viewsTokenReplace($alter['suffix'], $tokens);
     }
+    return $this->getRenderer()->render($render);

Could we add a follow up to not have to deal the render() call in viewsTokenReplace()? Now that we have a render array we would not longer need that additional render() call

pwolanin’s picture

Assigned: Unassigned » pwolanin

Taking a look at this

pwolanin’s picture

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
30.69 KB
1.2 KB

These changes are very welcome in terms of removing a WTF from the api:

-  public static function l($text, Url $url, $collect_bubbleable_metadata = FALSE) {
-    return static::getContainer()->get('link_generator')->generate($text, $url, $collect_bubbleable_metadata);
+  public static function l($text, Url $url) {
+    return static::getContainer()->get('link_generator')->generate($text, $url);

This patch just removes a unused "use" and fixes the doc on one return value.

Otherwise looks good.

alexpott’s picture

pwolanin’s picture

Assigned: pwolanin » Unassigned
Issue summary: View changes

Added a draft change record

znerol’s picture

Found only nits:

  1. +++ b/core/lib/Drupal.php
    @@ -546,16 +546,15 @@ public static function linkGenerator() {
    -  public static function l($text, Url $url, $collect_bubbleable_metadata = FALSE) {
    -    return static::getContainer()->get('link_generator')->generate($text, $url, $collect_bubbleable_metadata);
    +  public static function l($text, Url $url) {
    +    return static::getContainer()->get('link_generator')->generate($text, $url);
    

    Great! Should also remove @param docs for the removed parameter.

  2. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
    @@ -440,7 +440,11 @@ function hook_system_breadcrumb_alter(\Drupal\Core\Breadcrumb\Breadcrumb &$bread
    + *     changes this text is needs to preserve the safeness of the original text.
    

    s/ is / it /

  3. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -1305,8 +1305,10 @@ public function renderText($alter) {
         // Preserve whether or not the string is safe. Since $suffix comes from
    -    // \Drupal::l(), it is safe to append.
    -    if ($value_is_safe) {
    +    // \Drupal::l(), it is safe to append. Use SafeMarkup::isSafe() here because
    +    // $value_is_safe is safe has been used above and renderAsLink() can return
    +    // both safe and unsafe values.
    

    This is a bit difficult to understand especially ... because $value_is_safe is safe has been used ....

alexpott’s picture

@znerol good points. Addressed them.

  • effulgentsia committed a7254db on 8.0.x
    Issue #2568977 by alexpott, dawehner, pwolanin, Wim Leers: Replace...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Nice improvements. Pushed to 8.0.x. Some questions for either answering here, or if needing a patch or discussion, can be follow-ups:

  1. +++ b/core/lib/Drupal/Core/GeneratedLink.php
    @@ -46,4 +47,18 @@ public function setGeneratedLink($generated_link) {
    +  public function __toString() {
    +    return (string) $this->generatedLink;
    +  }
    

    Why are we casting $this->generatedLink to a string? When is it not that already? Also, should we now deprecate the getGeneratedLink() method and instruct people to cast the GeneratedLink object to a string instead?

  2. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
    @@ -149,21 +154,26 @@ public function generate($text, Url $url, $collect_bubbleable_metadata = FALSE)
    +    // This is safe because Attribute does escaping and $variables['text'] is
    +    // either rendered or escaped.
    +    return $generated_link->setGeneratedLink('<a' . $attributes . '>' . $variables['text'] . '</a>');
    

    Whoa. Scary that GeneratedLink implements SafeStringInterface and also has a public setter. Should we add an @internal to setGeneratedLink() and some other docs explaining that the caller is responsible for security?

Also, the CR looks like a good start, but I didn't publish it yet, because it doesn't mention that GeneratedLink now implements a __toString(), so most calling code should be unaffected, but invocations of PHP functions that treat scalar strings and stringifiable objects differently would be affected. I think that would be good info to add so people don't think the change is bigger than it really is.

effulgentsia’s picture

effulgentsia’s picture

Once #42 is figured out, the source of that might also be some good info to include in the CR per #41.

effulgentsia’s picture

Title: Replace SafeMarkup::format() in the link generator - it's a bad example to everyone » [needs change record updates] Replace SafeMarkup::format() in the link generator - it's a bad example to everyone
Status: Fixed » Needs work
Issue tags: +Needs change record updates

Per last paragraph of #41 and #43.

alexpott’s picture

Title: [needs change record updates] Replace SafeMarkup::format() in the link generator - it's a bad example to everyone » Replace SafeMarkup::format() in the link generator - it's a bad example to everyone
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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