Problem/Motivation

_batch_page() calls SafeMarkup::setMultiple() 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)

Not necessary, we are only adding documentation.

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
#1 document-2501447-1.patch562 bytesstar-szr

Comments

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new document-2501447-1.patch562 bytes

I think this is all that's needed.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Thanks, good to go.

xjm’s picture

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

Ahh, I remember this one. Yep, that's one of those legit internal uses. And there's that @todo still. :)

This issue only changes documentation and is also a required part of completing a critical, 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. Thanks @joelpittet and @Cottser!

Fixed a comma splice on commit:

diff --git a/core/includes/batch.inc b/core/includes/batch.inc
index 98e4ddd..9281837 100644
--- a/core/includes/batch.inc
+++ b/core/includes/batch.inc
@@ -49,7 +49,7 @@ function _batch_page(Request $request) {
   }
   // Restore safe strings from previous batches.
   // This is safe because we are passing through the known safe values from
-  // SafeMarkup::getAll(), see _batch_shutdown().
+  // SafeMarkup::getAll(). See _batch_shutdown().
   // @todo Ensure we are not storing an excessively large string list in:
   //   https://www.drupal.org/node/2295823
   if (!empty($batch['safe_strings'])) {

  • xjm committed c29f895 on 8.0.x 
    Issue #2501447 by Cottser: Document SafeMarkup::setMultiple in...
star-szr’s picture

star-szr
he/him
Primary language English
commented

Thanks for the comma fix!

Status: Fixed » Closed (fixed)

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