Follow-up to #2501705: Remove SafeMarkup::set() in LinkGenerator and document SafeMarkup::set() in LinkGeneratorTest

Problem/Motivation

LinkGeneratorTest calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code.
  • 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
  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.
  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. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  3. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
use_checkAdminXss.patch830 byteslokapujya
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya at Babson College commented
Status: Active » Needs review
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer and for Drupal Association commented
Status: Needs review » Reviewed & tested by the community

It's in a test and em is a safe admin XSS tag so this seems very appropriate.

Thank you @lokapujya

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Closed (duplicate)
Related issues: +#2501705: Remove SafeMarkup::set() in LinkGenerator and document SafeMarkup::set() in LinkGeneratorTest

So, since this is a test, I looked at what the intent of the test is.

It was added in #2047619: Add a link generator service for route-based links to test the good old 'html' => TRUE option, which, sensibly enough, got changed when we removed that in #2273923: Remove html => TRUE option from l() and link generator.. In that issue, both this unit test and the code it tests in LinkGenerator::generate() we updated to use SafeMarkup::set(). As @alexpott pointed out to me this morning, since this is a unit test, the correct thing to do with HEAD would actually be to keep the call. So, we would document this call instead based on that. Unit tests should test exactly the thing they're testing, not filterXss() which is not called by the tested method and therefore not something we should add to the unit test either.

However, there's probably another issue for that SafeMarkup::set() call in the LinkGenerator. Let's merge them, and either add a comment for this line of the test in that issue, or update it as appropriate.

So, marking as a duplicate of #2501705: Remove SafeMarkup::set() in LinkGenerator and document SafeMarkup::set() in LinkGeneratorTest.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: Remove SafeMarkup::set in testGenerateWithHtml » Document SafeMarkup::set() in testGenerateWithHtml()

And, titling correctly for posterity. :)