Follow-up to #2280965: [meta] Remove every SafeMarkup::set() call

Problem/Motivation

\Drupal\Tests\Core\Form\FormCacheTest::testSetCacheWithSafeStrings() 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. Install the search_extra_type module
  3. Go to /search, click the "Dummy search type" tab, and enter <test>
  4. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  5. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cilefen’s picture

Status: Active » Needs review
pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

just a test code cleanup, looks good.

tim.plunkett’s picture

+++ b/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
@@ -427,7 +427,7 @@ public function testSetCacheAuthUser() {
+    SafeMarkup::checkplain('a_safe_string');

checkPlain

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work
cilefen’s picture

Status: Needs work » Needs review
FileSize
587 bytes

I blame the reviewer ;-).

tim.plunkett’s picture

I actually think this is a valid usage of ::set(), since it's just trying to populate the list of safe strings.
But I haven't been as involved with the other issues like this, can we just document it and leave it?

cilefen’s picture

Yes, it probably fine.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I agree this is valid usage - cause it is what is under test. Committed c4281e4 and pushed to 8.0.x. Thanks!

  • alexpott committed c4281e4 on 8.0.x
    Issue #2539300 by cilefen, tim.plunkett: Remove SafeMarkup::set in \...

Status: Fixed » Closed (fixed)

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