Postponed on #2544156: Deprecate drupal_render_children() according to #11

Problem/Motivation

drupal_render_children() 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)

Not necessary, we are only adding documentation.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#20 2501455.18.patch1.05 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,913 pass(es). View
#4 document-2501455-4.patch569 bytesakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,929 pass(es). View
#1 document-2501455-1.patch538 bytesCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,620 pass(es). View

Comments

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Active » Needs review
FileSize
538 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,620 pass(es). View

I'm not really sure how true this is but it's pretty internal. This is a start, anyway.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

This documentation seems correct.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @Cottser. So looking at this comment, I think we should be a little more explicit about the purpose of the call that render() is not magically making everything sanitized -- saying it is "safe" is a little unspecific. Similarly to #2273925-269: Ensure #markup is XSS escaped in Renderer::doRender() and... whatever other issue it was I bumped back yesterday for the same thing. #2501403: Document SafeMarkup::set in Xss::filter. Kind of blurring together. :) We might want a longish comment on what the render system has done up until that point and how it interacts with the... thingy. Twig autoescaping.

Here's the comment from the doRender() issue to save that page load:

+    // \Drupal\filter\Plugin\FilterInterface. $text is not guaranteed to be
+    // safe, but it has been passed through the filter system and checked with
+    // a text format, so it must be printed as is. (See the note about security
+    // in the method documentation above.)
akalata’s picture

Status: Needs work » Needs review
FileSize
569 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,929 pass(es). View

Here's a try for updated documentation, though I'm not sure it's exactly what you're looking for. I'm thinking we don't need to provide details on the render system since we've documented the specific class that should be handling the checking of markup (maybe?).

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Also good with that/can't think of anything better to describe it. Thanks @akalata

xjm’s picture

Title: Document SafeMarkup::set in drupal_render_children() » Document or remove SafeMarkup::set in drupal_render_children()
Assigned: Unassigned » Wim Leers
Status: Reviewed & tested by the community » Needs review

So, I started to post a slightly tweaked text for this. Then, I was going to suggest it be merged with the patch for docs in Renderer::doRender() so that we could explain it consistently.

Then I realized that this function (unlike doRender()) is not just marking XSS-filtered #markup as safe -- it's marking the entire render array output as safe and returning it, which means (as far as I know) that any unsafe strings in the render array will never get sanitized by Twig either.

What's more, it actually doesn't appear to be a part of the main render API any more. It's only called in four places and it sort of circumvents the rest of the render system actually. I think this function should be replaced entirely in the code that calls it, and deprecated.

I pinged @Wim Leers for feedback on the status of drupal_render_children().

Wim Leers’s picture

Assigned: Wim Leers » joelpittet

Killing drupal_render_children() would be splendid. It is a very, very confusing function anyway.

I'd like input from somebody with very deep Theme system knowledge to make sure that that is actually okay though. Assigning to Joël.

xjm’s picture

Title: Document or remove SafeMarkup::set in drupal_render_children() » Document or remove SafeMarkup::set in drupal_render_children(), or remove drupal_render_children()

Thanks Wim!

joelpittet’s picture

Assigned: joelpittet » Unassigned

I wish assignments notified me somehow:) I just happened to be following this one.

I think we can kill it's use in core almost entirely if not totally. But not sure we can do that in contrib, that I'd definitely need a check on grepping all contrib.

It's not as confusing as drupal_render() which needs some factoring love ;-)
It loops through the element children, renders each child element and concatenates them. Which is actually also done in drupal_render/Renderer::doRender:
/lib/Drupal/Core/Render/Renderer.php:391

    if ((!$theme_is_implemented || isset($elements['#render_children'])) && empty($elements['#children'])) {
      foreach ($children as $key) {
        $elements['#children'] .= $this->doRender($elements[$key]);
      }
      $elements['#children'] = SafeMarkup::set($elements['#children']);
    }

The only difference is that the one in doRender is sorted, and the one in drupal_render_children() is not sorted and can take arbitrary keys.

Currently it's used in the following places:

  1. core/modules/system/system.admin.inc:355
  2. core/modules/toolbar/src/Element/Toolbar.php:101
  3. core/modules/views/views.module:695
  4. core/modules/views_ui/views_ui.theme.inc:175

Maybe we need a compliment to |without() to be |with() to get this feature in twig?
Currently haven't had any reason to use this feature in core.

system.admin.inc and core/modules/views_ui/views_ui.theme.inc are on the theme function chopping block.
#1938930: Convert theme_system_modules_details() to #type table
#1963978: Convert theme_views_ui_build_group_filter_form() to Twig

The views.module one may not even need that extra call at all, but worth testing that theory out. Same with preRenderToolbar. Those two are the wildcards.

So +1 to create a follow-up to deprecate that function and find all it's use in contrib.

Cottser’s picture

@Fabianx would be good to ping here too. I think generally it is used to print remaindered/extra "stuff" contained in a render array, but maybe we can remove it now that we have |without. Not sure at the moment.

xjm’s picture

Great, so here's what I'd recommend:

  1. A major meta a.k.a. plan to deprecate drupal_render_children() and remove its core usages. Document in the beta eval that it's a blocker for part of the SafeMarkup critical.
  2. Make the two issues @Cottser linked part of the meta. (Though the table one sounds scary; if that takes too long we might want to look at an interim fix.)
  3. Child issues to remove the other two core uses and replace them with whatever.
  4. A child issue to mark drupal_render_children() as deprecated in 8.0.x and to be removed before 9.0.0, with what to do instead and strong warnings about the downsides of using it.
  5. Postpone this issue on the above, and change the comment to also include dire warnings about the deprecation.
Cottser’s picture

lauriii’s picture

Status: Needs review » Needs work
lauriii’s picture

Status: Needs work » Postponed

Created the follow-up and will work on that: #2544156: Deprecate drupal_render_children(). drupal_render_children() has the only SafeMarkup::set() call in common.inc so postponing this on the child issue.

jrearick’s picture

Issue summary: View changes

Clarifying what this is postponed on in the issue summary.

alexpott’s picture

Let's swap the SafeMarkup::set() for a SafeString::create() - it'll work fine.

alexpott’s picture

Status: Postponed » Needs work
alexpott’s picture

In discussion with @xjm, @joelpittet, @effulgentsia, @cilefen, @lauriii, @davidhernandez, @yesct and @pwolanin it was concluded that regardless of the outcome of #2544156: Deprecate drupal_render_children() we should do the replacement of SafeMarkup::set() with SafeString::create() as this is consistent with Renderer::render().

xjm’s picture

Title: Document or remove SafeMarkup::set in drupal_render_children(), or remove drupal_render_children() » Remove SafeMarkup::set() in drupal_render_children() and replace it with SafeString
alexpott’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,913 pass(es). View
lauriii’s picture

Status: Needs review » Reviewed & tested by the community

RTBC if this passes.

Tested the few places where render_children is used manually and it seemed to work.

joelpittet’s picture

RTBC++

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed af0ebfe on 8.0.x
    Issue #2501455 by Cottser, alexpott, akalata: Remove SafeMarkup::set()...

Status: Fixed » Closed (fixed)

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