Problem/Motivation

Drupal\Core\Utility\LinkGenerator calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code. COMPLETED
  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123 - COMPLETED
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it. - New test added and existing tests updated
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps (for XSS and double escaping)

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Create a new menu link item, perhaps with HTML in the link text or double quotes in the URL
  3. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping (and proper single-escaping).

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tetranz’s picture

Assigned: Unassigned » tetranz
tetranz’s picture

tetranz’s picture

Status: Active » Needs review
tetranz’s picture

Patch #2 was calling checkPlain unnecessarily for URL.

Tidied up comments.

dawehner’s picture

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

star-szr’s picture

Status: Needs review » Needs work

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?

star-szr’s picture

@dawehner jinx :)

tetranz’s picture

Fixed the comments.
Changed attributes to a @token for consistency although perhaps not strictly necessary.

tetranz’s picture

Status: Needs work » Needs review
tetranz’s picture

Title: Remove or document SafeMarkup::set in LinkGenerator » Remove SafeMarkup::set in LinkGenerator
star-szr’s picture

Going to review this again.

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
+++ 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.

tetranz’s picture

Thanks. That's a good point about the double quote in the URL. I think it might break it but I will do a test.

It will be a few days before I get back to this.

joelpittet’s picture

Assigned: tetranz » Unassigned

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.

tetranz’s picture

Assigned: Unassigned » tetranz

I have tests which I will upload tomorrow night.

tetranz’s picture

This is my first ever attempt at creating a test.

I'm not sure if I've done the right thing but I also added a test to UnroutedUrlAssemblerTest to show that the url encoding works correctly. I think I needed this to be sure that my mock for the assembler is valid in my new test in LinkGeneratorTest.

tetranz’s picture

I messed up the interdiff. I think #17 is better.

tetranz’s picture

Status: Needs work » Needs review

The last submitted patch, 16: remove_or_document-2501705-16.patch, failed testing.

The last submitted patch, 16: remove_or_document-2501705-16.patch, failed testing.

xjm’s picture

Title: Remove SafeMarkup::set in LinkGenerator » Remove SafeMarkup::set() in LinkGenerator
Status: Needs review » Needs work

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.

pwolanin’s picture

Let me re-roll this with those fixes.

pwolanin’s picture

Assigned: tetranz » Unassigned
Status: Needs work » Needs review
FileSize
4.44 KB
4.39 KB

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.

Status: Needs review » Needs work

The last submitted patch, 23: 2501705-23.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.92 KB
1.42 KB

tweak the code so href attribute is first.

xjm’s picture

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

xjm’s picture

Title: Remove SafeMarkup::set() in LinkGenerator » Remove SafeMarkup::set() in LinkGenerator and document SafeMarkup::set() in LinkGeneratorTest
pwolanin’s picture

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

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

xjm’s picture

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

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

pwolanin’s picture

FileSize
5.44 KB
830 bytes

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

[edit] Oops - that wasy the wrong thing

pwolanin’s picture

FileSize
5.65 KB
2.89 KB

Fix comments in test and elsewhere.

Status: Needs review » Needs work

The last submitted patch, 31: 2501705-31.patch, failed testing.

pwolanin queued 31: 2501705-31.patch for re-testing.

pwolanin’s picture

Status: Needs work » Needs review
xjm’s picture

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

akalata’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • xjm committed c6cc4fa on 8.0.x
    Issue #2501705 by tetranz, pwolanin, Cottser, joelpittet, akalata:...

Status: Fixed » Closed (fixed)

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