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

Files: 
CommentFileSizeAuthor
#9 remove_safemarkup_set-2501819-9.patch787 bytesleslieg
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,676 pass(es). View
#6 remove_safemarkup_set-2501819-6.patch850 bytesleslieg
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,703 pass(es), 7 fail(s), and 1 exception(s). View
#2 search_embedded_form-2501819-2.patch986 bytesleslieg
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,717 pass(es), 6 fail(s), and 2 exception(s). View

Comments

leslieg’s picture

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

leslieg’s picture

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
986 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,717 pass(es), 6 fail(s), and 2 exception(s). View

changed safemarkup::set to safemarkup::format

Status: Needs review » Needs work

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

Cottser’s picture

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

Cottser’s picture

leslieg’s picture

Status: Needs work » Needs review
FileSize
850 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,703 pass(es), 7 fail(s), and 1 exception(s). View

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.

Cottser’s picture

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

Status: Needs work » Needs review
FileSize
787 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,676 pass(es). View

Updated patch with suggestions from @joelpittet

joelpittet’s picture

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

FileSize
1.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,230 pass(es), 6 fail(s), and 0 exception(s). View

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

joelpittet’s picture

@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

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

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.