Remove SafeMarkup::set() in LinkGenerator and document SafeMarkup::set() in LinkGeneratorTest

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -131,20 +131,19 @@ public function generate($text, Url $url, $collect_cacheability_metadata = FALSE
+    // it here in an HTML argument context, we encode it below with SafeFormat::format

80 chars exceeded ... :(

Thanks @tetranz!

1. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -131,20 +131,19 @@ public function generate($text, Url $url, $collect_cacheability_metadata = FALSE
   +    // it here in an HTML argument context, we encode it below with SafeFormat::format
   

   SafeMarkup::format rather than SafeFormat::format.

   Also this is now longer than 80 character so should be wrapped per https://www.drupal.org/node/1354#drupal.

2. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -131,20 +131,19 @@ public function generate($text, Url $url, $collect_cacheability_metadata = FALSE
   +    // $attributes are safe because all subclasses of AttributeValueBase call checkPlain in their __toString()
   +    $result = SafeMarkup::format('<a href="@url"' . $attributes . '>@text</a>', ['@url' => $url, '@text' => $variables['text']]);
   

   Maybe you tried this but I don't think we can concatenate in the attributes like this. Can it be passed in as a @token as well?

@dawehner jinx :)

Going to review this again.

+++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -130,21 +130,20 @@ public function generate($text, Url $url, $collect_cacheability_metadata = FALSE
-      $url = SafeMarkup::checkPlain($url->toString($collect_cacheability_metadata));
+      $url = $url->toString($collect_cacheability_metadata);
...
-      $url = SafeMarkup::checkPlain($generated_url->getGeneratedUrl());
+      $url = $generated_url->getGeneratedUrl();
...
+    $result = SafeMarkup::format('<a href="@url"@attributes>@text</a>', ['@url' => $url, '@attributes' => $attributes, '@text' => $variables['text']]);

I think we can do these checkPlain removals. It will likely still escape the value correctly under most conditions and the URL is already Url encoded.

Example test:

<?php
use Drupal\Component\Utility\SafeMarkup;
use Drupal\Core\Url;

$example = '/giraffe?unicorn="awesome"';
SafeMarkup::set($example);
$url = Url::fromUserInput($example);
print Drupal::l('kittens', $url);

Something like this likely needs to be added to a test in LinkGeneratorTest. To ensure we don't have " breaking the href attribute.

Thanks for letting us know. For now I'll unassign you until a few days and please grab it again if it's not been completed by then.

I marked #2502035: Document SafeMarkup::set() in testGenerateWithHtml() as a duplicate of this issue, since I'm fairly certain that's test coverage for the lines we're changing here. Either we add a comment to the test to document why that call was appropriate, or we change the test so that it's testing the exact right thing for this issue.

Based on what the patch here is doing, I think that the intent of the test is still to ensure that the $safe_text doesn't get escaped a second time when it's already in the safe list, so I think the SafeMarkup::set() call in the test should still be retained and documented as an intentional unit test for the interaction between SafeMarkup and the LinkGenerator. See my comment on #2502035: Document SafeMarkup::set() in testGenerateWithHtml() for more background info.

Meanwhile, couple of nitpicks I noticed while scanning this patch to decide what to do with the test:

1. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -130,21 +130,20 @@ public function generate($text, Url $url, $collect_cacheability_metadata = FALSE
   +    // We encode it below with SafeMarkup::format because we are using it here
   

   Minor: there should be parens on method names in comments.

2. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
   @@ -181,6 +182,40 @@ public function testGenerateExternal() {
   +   * Tests the generate() method with a url containing double quotes.
   +   * This tests that quotes are url encoded by the urlAssembler and therefore
   +   * the final url string is not broken by being marked as safe.
   

   Minor: We should have a blank line between the one-line summary and the trst of the docs.

Let me re-roll this with those fixes.

I'm really not seeing why the added test case adds value. Certainly setting the url-encoded value as safe won't have an effect.

Also, href is just an attribute, so I think we can just more consistently manage it through the Attribute class.

tweak the code so href attribute is first.

Still need the added inline comment on the existing SafeMarkup::set() call in LinkGeneratorTest.

So, I don't see why the patch was calling SafeMarkup::set() in LinkGeneratorTest. My last patch doesn't have it.

@pwolanin was a bit confused at my comments; we sorted it out in IRC. The added scope is to do with the existing SafeMarkup::set() in HEAD in the test.

adding the test change from: #2502035: Document SafeMarkup::set() in testGenerateWithHtml()

[edit] Oops - that wasy the wrong thing

Fix comments in test and elsewhere.

+++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
@@ -364,16 +364,19 @@ public function testGenerateWithHtml() {
+    // Test that safe HTML is output inside the anchor tag unescaped. The
+    // SafeMarkup::set() call is an intentional unit test for the interaction
+    // between SafeMarkup and the LinkGenerator.

Yay, that! Thanks. +1.

This patch removes one instance SafeMarkup::set, replacing it with SafeMarkup::format as part of a refactoring of how link item attributes (including the href) are generated.

There are also two new tests (one for external URLs and one for local) to ensure that the URL generator correctly escapes characters, and an update to an existing test to ensure that previously-sanitized output (for example, the text within the ) is not affected by the change. The use of SafeMarkup::set in this test case is properly documented.

I've updated the issue summary, and manually tested with both a menu link with HTML in the link text and double quotes in the URL, noting no change between HEAD and this patch.

It's great that we are able to get rid of two checkPlain() and an escape() in addition to the set(). Given how frequently the LinkGenerator is used, this patch might actually make a slight dent in the callstack and memory overhead in addition to being security hardening.

Thanks @akalata also for the excellent review.

I discussed this patch in IRC with @alexpott as well. I am somewhat cautious about the use of SafeMarkup::format() to assemble HTML tags since used incorrectly it can give the illusion of sanitization while actually generating markup that is unsafe, but in this case it is safe, and it is internal enough to the core API that I think there is a low risk of contrib misusing the pattern seen here. The tag can only be <a>, the attributes are safely assembled with the Attribute class, and the link content is also sanitized because it is used with an @ placeholder.

As a required part of a critical issue, this patch can be committed any time during the beta phase. Committed and pushed to 8.0.x. Awesome work!