Problem/Motivation

(name of function/class::method that calls it) 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.
  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

N/A

API changes

N/A

CommentFileSizeAuthor
#11 remove_safemarkup_set-2501819-11.patch1.52 KBedysmp
#9 remove_safemarkup_set-2501819-9.patch787 bytesleslieg
#6 remove_safemarkup_set-2501819-6.patch850 bytesleslieg
#2 search_embedded_form-2501819-2.patch986 bytesleslieg
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

leslieg’s picture

leslieg CreditAttribution: leslieg as a volunteer commented

Working on removing SafeMarkup::set in search_embedded_form_preprocess_search_result() as part of NHDevDays2

leslieg’s picture

leslieg CreditAttribution: leslieg as a volunteer commented
Title: Remove or document SafeMarkup::set in search_embedded_form_preprocess_search_result() » Remove SafeMarkup::set in search_embedded_form_preprocess_search_result()
Status: Active » Needs review
FileSize
search_embedded_form-2501819-2.patch986 bytes

changed safemarkup::set to safemarkup::format

Status: Needs review » Needs work

The last submitted patch, 2: search_embedded_form-2501819-2.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented 
+++ b/core/modules/search/tests/modules/search_embedded_form/search_embedded_form.module
@@ -16,5 +16,6 @@
-  $variables['snippet'] = SafeMarkup::set(SafeMarkup::escape($variables['snippet']) . drupal_render($form));
+//  $variables['snippet'] = SafeMarkup::set(SafeMarkup::escape($variables['snippet']) . drupal_render($form));
+  $variables['snippet'] = SafeMarkup::format('<span@author_attributes>@submitted</span>', ['@author_attributes' => $author_attributes, '@submitted' => $variables['submitted']]);

It looks like this got mixed up with another patch, author_attributes shouldn't be here.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Parent issue: » #2280965: [meta] Remove every SafeMarkup::set() call
leslieg’s picture

leslieg CreditAttribution: leslieg as a volunteer commented
Status: Needs work » Needs review
FileSize
remove_safemarkup_set-2501819-6.patch850 bytes

Fixed issue with inadvertent mix of commits

Status: Needs review » Needs work

The last submitted patch, 6: remove_safemarkup_set-2501819-6.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented 
+++ b/core/modules/search/tests/modules/search_embedded_form/search_embedded_form.module
@@ -16,5 +16,5 @@
-  $variables['snippet'] = SafeMarkup::set(SafeMarkup::escape($variables['snippet']) . drupal_render($form));
+  $variables['snippet'] = SafeMarkup::format('@snippet@rendered_form', ['@snippet' => $variables['snippet'], '@rendered_form' => $drupal_render($form)]);

$drupal_render -> drupal_render (better yet, use the render service directly).

leslieg’s picture

leslieg CreditAttribution: leslieg as a volunteer commented
Status: Needs work » Needs review
FileSize
remove_safemarkup_set-2501819-9.patch787 bytes

Updated patch with suggestions from @joelpittet

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Issue tags: +Needs manual testing

Sweet just need to double check that worked out for real. Nice to see that come back green thanks @leslieg

edysmp’s picture

edysmp
Primary language Spanish
Location Nicaragua
CreditAttribution: edysmp at MTech, LLC commented
FileSize
remove_safemarkup_set-2501819-11.patch1.52 KB

The preprocess is not used, not able to reproduce against HEAD, new patch remove the preprocess attach

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented

@edysmp It's used for a test but this will be interesting to see what testbot says about that...

Status: Needs review » Needs work

The last submitted patch, 11: remove_safemarkup_set-2501819-11.patch, failed testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs manual testing

Now realizing this is just in a test, doesn't need manual testing. RTBC #9

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thanks @joelpittet -- this is indeed only a test module used to support Drupal\search\Tests\SearchEmbedFormTest, which is kind of a weird and oddly specific test to begin with.

Whenever we change test code it's good to look at what the intent of the test is and ensure that any changes are in line with that. In this case, the test dates back to the D7 cycle in #497206: Avoid search conflicts with other forms, use menu API instead of search_get_keys(). Looking at that issue and the assertions in the test, it's clear that the intent is just to make sure the form submission works, so it's not worth the effort to improve the themeable output of the test module to best practices!

Given that, the minimal fix from #9 is fine (I especially like that we got rid of that dangling drupal_render()!). And the fact that the test passes is indeed the confirmation we need in this case rather than manual testing.

This issue only changes test code and it is a required part of a critical issue, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x.

  • xjm committed f8b8f3b on 8.0.x 
    Issue #2501819 by leslieg, edysmp, Cottser, joelpittet: Remove...

Status: Fixed » Closed (fixed)

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