Limit what can be called by a callback in render arrays to reduce the risk of RCE

Nice approach, looking good - going to be disruptive though - but security trumps BC right?

1. +++ b/core/includes/common.inc
   @@ -764,96 +764,12 @@ function drupal_pre_render_link($element) {
    function drupal_pre_render_links($element) {
   
   +++ b/core/modules/color/color.module
   @@ -112,26 +112,17 @@ function color_library_info_alter(&$libraries, $extension) {
    function color_block_view_pre_render(array $build) {
   
   +++ b/core/modules/filter/filter.module
   @@ -305,17 +306,13 @@ function check_markup($text, $format_id = NULL, $langcode = '', $filter_types_to
    function filter_form_access_denied($element) {
   
   +++ b/core/modules/history/history.module
   @@ -186,9 +187,10 @@ function history_user_delete($account) {
    function history_attach_timestamp($node_id) {
   
   +++ b/core/modules/toolbar/toolbar.module
   @@ -217,25 +215,14 @@ function toolbar_toolbar() {
    function toolbar_prerender_toolbar_administration_tray(array $element) {
   
   @@ -312,49 +299,13 @@ function toolbar_get_rendered_subtrees() {
    function _toolbar_do_get_rendered_subtrees(array $data) {
   
   

   should we have trigger_error inside these too?

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -760,4 +747,61 @@ protected function ensureMarkupIsSafe(array $elements) {
   +   * @param $callback_name
   +   * @param $interface
   +   * @param $callback
   +   * @param array $args
   

   missing some docs here

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -760,4 +747,61 @@ protected function ensureMarkupIsSafe(array $elements) {
   +      $object_or_classname = $callback[0];
   +      $method_name = $callback[1];
   

   nit: could use list() here

4. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -760,4 +747,61 @@ protected function ensureMarkupIsSafe(array $elements) {
   +    elseif ($callback instanceof \Closure) {
   

   still needed given serialization comment?

5. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -760,4 +747,61 @@ protected function ensureMarkupIsSafe(array $elements) {
   +      @trigger_error(sprintf('Render %s callbacks must implement %s or be a anonymous function. The callback is %s. Deprecated in 8.6.x and will be enforced in Drupal 8.7.0', $callback_name, $interface, $object_or_classname), E_USER_DEPRECATED);
   

   The callback 'is' or the callback 'was'?

6. +++ b/core/modules/color/color.module
   @@ -112,26 +112,17 @@ function color_library_info_alter(&$libraries, $extension) {
   +  // @todo Convert to an object implementing RenderCallbackInterface
   

   seems to be done?

I prefer the new interface that specifies which methods can be used, just having the empty interface would still allow any method on that class which seemed still a bit leaky.

If we're going to do a hard API break in a minor release for this, should we consider a three stage break?

8.6 -> E_USER_DEPRECATED
8.7 -> E_USER_WARNING
8.8 -> exception

Hm, I understand that we have to do something, but this is going to break dozens of contrib modules and countless custom modules/sites as well.

Agreed with @catch that we should not completely break this without having actually visible errors/warnings that site admins/users will see in their logs and have time to react to it. A lot of it might be hidden in rarely used features that not everyone will be testing when they update to a new minor version, so filling their logs with warnings gives them another 6 month to update their modules/code. If we don't want to wait for a full year then I can also see that we already switch to warnings in the next minor. Better than just going straight from deprecated (or would it be a non-suppressed deprecation message?) to not working.

Either way, it's pretty much guaranteed that this will result in lots of people complaining once we introduce that break.

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -821,4 +822,11 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
+   * {@inheritdoc}
+   */
+  public static function renderCallbacks() {
+    return ['removeSubmittedInfo'];
+  }
+

Why make the interface/method render-specific? Maybe use something like SafeCallbackInterface or TrustedCallbackInterface or something.

Even if we only use it for render-related callbacks now, we might want to extend it/re-use it for something similar later?

Looking good. I think we need to add deprecation tests for the 7 functional callbacks we deprecated (views, block, color, links, history, etc)

1. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -1530,7 +1530,8 @@ function hook_entity_view_alter(array &$build, Drupal\Core\Entity\EntityInterfac
   +    // The object implements \Drupal\Core\Security\TrustedCallbackInterface.
   

   should we say 'must implement' instead of 'implements'?

   Should we add a @see to the docblock pointing to the interface for easier cross-linking?

2. +++ b/core/lib/Drupal/Core/Render/Element/Table.php
   @@ -64,7 +64,7 @@ class Table extends FormElement {
   -    $class = get_class($this);
   +    $class = get_class();
   

   unrelated?

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -756,4 +746,33 @@ protected function ensureMarkupIsSafe(array $elements) {
   +    $message = sprintf('Render %s callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was %s. Support for this callback is deprecated in 8.7.0 and will be enforced in Drupal 9.0.0. See https://www.drupal.org/node/2966725', $callback_type, '%s');
   

   is enforced the right word here? unsupported perhaps?

4. +++ b/core/lib/Drupal/Core/Security/TrustedCallbackInterface.php
   @@ -0,0 +1,35 @@
   +  const THROW_EXCEPTION = 'exception';
   

   is this used?

5. +++ b/core/lib/Drupal/Core/Security/TrustedCallbackTrait.php
   @@ -0,0 +1,89 @@
   +/**
   + * @todo
   + */
   +trait TrustedCallbackTrait {
   

   :)

6. +++ b/core/modules/block/src/BlockViewBuilder.php
   @@ -135,6 +136,13 @@ protected static function buildPreRenderableBlock($entity, ModuleHandlerInterfac
   +  public static function trustedCallbacks() {
   +    return ['preRender', 'lazyBuilder'];
   

   should we call the parent here too?

7. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -169,7 +169,7 @@ public function buildComponents(array &$build, array $entities, array $displays,
   -          '#lazy_builder' => ['history_attach_timestamp', [$commented_entity->id()]],
   +          '#lazy_builder' => ['\Drupal\history\HistoryRenderCallback::lazyBuilder', [$commented_entity->id()]],
   

   should we use the array syntax here so we can use the ::class constant?

8. +++ b/core/modules/views/tests/modules/views_test_data/views_test_data.views_execution.inc
   @@ -54,21 +54,14 @@ function views_test_data_views_pre_render(ViewExecutable $view) {
   +    // @todo convert to an object that implements TrustedCallbackInterface.
   

   is that already done?

9. +++ b/core/tests/Drupal/Tests/Core/Render/RendererCallbackTest.php
   @@ -0,0 +1,73 @@
   +      'Procedural function pre render' => [array_merge($render_array, ['#pre_render' => ['\Drupal\Tests\Core\Render\callback']]), 'Render #pre_render callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was \Drupal\Tests\Core\Render\callback. Support for this callback is deprecated in 8.7.0 and will be enforced in Drupal 9.0.0. See https://www.drupal.org/node/2966725'],
   +      'Static object method post render' => [array_merge($render_array, ['#post_render' => ['\Drupal\Tests\Core\Render\RendererCallbackTest::renderCallback']]), 'Render #post_render callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was \Drupal\Tests\Core\Render\RendererCallbackTest::renderCallback. Support for this callback is deprecated in 8.7.0 and will be enforced in Drupal 9.0.0. See https://www.drupal.org/node/2966725'],
   +      'Object method access callback' => [array_merge($render_array, ['#access_callback' => [$this, 'renderCallback']]), 'Render #access_callback callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was Drupal\Tests\Core\Render\RendererCallbackTest::renderCallback. Support for this callback is deprecated in 8.7.0 and will be enforced in Drupal 9.0.0. See https://www.drupal.org/node/2966725'],
   +      'Procedural function lazy builder' => [['#lazy_builder' => ['\Drupal\Tests\Core\Render\callback', []]], 'Render #lazy_builder callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was \Drupal\Tests\Core\Render\callback. Support for this callback is deprecated in 8.7.0 and will be enforced in Drupal 9.0.0. See https://www.drupal.org/node/2966725'],
   

   can we add an explicit case for objects with an __invoke method?

10. +++ b/core/tests/Drupal/Tests/Core/Security/TrustedCallbackTraitTest.php
    @@ -0,0 +1,134 @@
    +    $tests['TrustedCallbackInterface_object'] = [[new TrustedMethods(), 'unTrustedCallback'], TrustedInterface::class];
    +    $tests['TrustedCallbackInterface_static_string'] = ['\Drupal\Tests\Core\Security\TrustedMethods::unTrustedCallback', TrustedInterface::class];
    +    $tests['TrustedCallbackInterface_static_array'] = [[TrustedMethods::class, 'unTrustedCallback'], TrustedInterface::class];
    +    $tests['untrusted_object'] = [[new UntrustedObject(), 'callback'], TrustedInterface::class];
    +    $tests['untrusted_object_static_string'] = ['\Drupal\Tests\Core\Security\UntrustedObject::callback', TrustedInterface::class];
    +    $tests['untrusted_object_static_array'] = [[UntrustedObject::class, 'callback'], TrustedInterface::class];
    

    same here for __invoke?

1. +++ b/core/includes/common.inc
   @@ -211,7 +210,7 @@ function valid_email_address($mail) {
   + * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
   

   8.0.x-dev is a lie. ;)

2. +++ b/core/includes/common.inc
   @@ -225,7 +224,6 @@ function valid_email_address($mail) {
   -  @trigger_error(__FUNCTION__ . '() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use UrlHelper::stripDangerousProtocols() or UrlHelper::filterBadProtocol() instead. See https://www.drupal.org/node/2560027', E_USER_DEPRECATED);
   

   Do we mean to un-deprecate this? I don't see any mention of it in the comments.

3. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
   @@ -65,6 +66,13 @@ public function getInfo() {
   diff --git a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   
   diff --git a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   index 0040ae076a..d1713f6065 100644
   
   index 0040ae076a..d1713f6065 100644
   --- a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   
   --- a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -5,7 +5,6 @@
   

   Everything in here seems unrelated. Is this a rebase #fail?

4. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -110,6 +111,13 @@ public function view(EntityInterface $_entity, $view_mode = 'full') {
   diff --git a/core/lib/Drupal/Core/Entity/EntityManager.php b/core/lib/Drupal/Core/Entity/EntityManager.php
   
   diff --git a/core/lib/Drupal/Core/Entity/EntityManager.php b/core/lib/Drupal/Core/Entity/EntityManager.php
   index 691b282276..045ca0f377 100644
   
   index 691b282276..045ca0f377 100644
   --- a/core/lib/Drupal/Core/Entity/EntityManager.php
   
   --- a/core/lib/Drupal/Core/Entity/EntityManager.php
   +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   
   +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -306,42 +306,39 @@ public function clearCachedFieldDefinitions() {
   
   @@ -306,42 +306,39 @@ public function clearCachedFieldDefinitions() {
      /**
   

   And here.

5. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -142,6 +143,13 @@ public function view(EntityInterface $entity, $view_mode = 'full', $langcode = N
   diff --git a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   
   diff --git a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   index 019570bda7..f5187c118f 100644
   
   index 019570bda7..f5187c118f 100644
   --- a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   
   --- a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   
   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -12,11 +12,8 @@
   

   Eeek... and here.

Okay, aborting deep dive. I think there's major rebase badness in #48. And now that I see it, the patch size grew by over 100K, but the interdiff claims to be only 13k. ;) So, maybe only point 1 is actually valid... ;)

This looks good to me, +1 for an early 8.8 commit

We need a release notes snippet.

Support for TrustedCallbackInterface would be a great feature to add to #2982949: Introduce CallableResolver to help standardise the DX and error handling for callbacks across various subsystems. Shameless self promotion on only a tangentially related issue :)

Trying again. Wow, this is a big + confusing patch. Mostly I found nits, some DX concerns.

1. +++ b/core/lib/Drupal/Core/Render/Element/Link.php
   @@ -99,4 +100,105 @@ public static function preRenderLink($element) {
   +   * $build['links'] = array(
   +   *   '#theme' => 'links__node',
   +   *   '#pre_render' => array(Link::class, 'preRenderLinks'),
   +   *   'comment' => array(
   +   *     '#theme' => 'links__node__comment',
   +   *     '#links' => array(
   +   *       // An array of links associated with node comments, suitable for
   +   *       // passing in to links.html.twig.
   +   *     ),
   +   *   ),
   +   *   'statistics' => array(
   +   *     '#theme' => 'links__node__statistics',
   +   *     '#links' => array(
   +   *       // An array of links associated with node statistics, suitable for
   +   *       // passing in to links.html.twig.
   +   *     ),
   +   *   ),
   +   *   'translation' => array(
   +   *     '#theme' => 'links__node__translation',
   +   *     '#links' => array(
   +   *       // An array of links associated with node translation, suitable for
   +   *       // passing in to links.html.twig.
   +   *     ),
   +   *   ),
   +   * );
   

   Is it out of scope to fix these to be short array syntax as we move them?

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -756,4 +746,33 @@ protected function ensureMarkupIsSafe(array $elements) {
   +   * @param array|string|object $callback
   

   Can $callback actually be of type "object"? I don't see that usage anywhere. I'm probably missing something.

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -756,4 +746,33 @@ protected function ensureMarkupIsSafe(array $elements) {
   +   *   The callbacks return value.
   

   Nit: "The callback's return value.

4. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -756,4 +746,33 @@ protected function ensureMarkupIsSafe(array $elements) {
   +    $message = sprintf('Render %s callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was %s. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725', $callback_type, '%s');
   

   DX: This message isn't really true. ;) The render callback doesn't implement TrustedCallbackInterface. The callback must be a public method of a class that implements that interface. Not exactly sure the most concise way to say this. Perhaps something like "Render %s callbacks must be methods of a class that implements \DrupalCore\Security\TrustedCallbackInterface or be an anonymous function...." ?

   Meanwhile, I think I'm really missing something, but I don't see how the "The callback was %s" part is working at all. We're passing "'%s'" as the 3rd arg to sprintf(). I don't see how that identifies the callback, neither here nor in doTrustedCallback().

5. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -756,4 +746,33 @@ protected function ensureMarkupIsSafe(array $elements) {
   +    return $this->doTrustedCallback($callback, $args, $message, TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION, ElementInterface::class);
   

   I don't understand why we pass ElementInterface::class as the "extra trusted interface" here. Maybe a comment to help mere mortals understand what's happening?

   Meanwhile, I don't understand why "doTrustedCallback()" is called "doTrustedCallback()" if it's handling untrusted callbacks. ;) The relationship between doCallback() and doTrustedCallback() doesn't make much sense to me.

6. +++ b/core/lib/Drupal/Core/Security/TrustedCallbackInterface.php
   @@ -0,0 +1,35 @@
   +  /**
   +   * Untrusted callbacks trigger E_USER_WARNING errors.
   +   */
   +  const TRIGGER_WARNING = 'warning';
   +
   +  /**
   +   * Untrusted callbacks trigger silenced E_USER_DEPRECATION errors.
   +   */
   +  const TRIGGER_SILENCED_DEPRECATION = 'silenced_deprecation';
   

   Why do we support these options at all? If the point of this is security hardening, why do we ever invoke untrusted callbacks anymore?

7. +++ b/core/lib/Drupal/Core/Security/TrustedCallbackInterface.php
   @@ -0,0 +1,35 @@
   +   *   List of trusted callbacks.
   

   DX: Would be helpful to say we mean the name(s) of the public method(s) provided by this class that we're trusting. I.e. we expect ['fooBar'] not ['WhateverIAm::fooBar()'] or worse ['SomeOtherClass::iTrustYou()'], etc.

8. +++ b/core/lib/Drupal/Core/Security/TrustedCallbackTrait.php
   @@ -0,0 +1,89 @@
   +   * Performs a trusted callback.
   

   Uhh, sort of. ;) More like "determines if a callback is trusted, if so, perform it, otherwise, maybe invoke it (!) and probably generate errors." I know that's not what we want to say here, but from my outsider perspective the DX of this is really confusing and weird.

9. +++ b/core/lib/Drupal/Core/Security/TrustedCallbackTrait.php
   @@ -0,0 +1,89 @@
   +   * @param $message
   +   *   The error message if the callback is not trusted.
   

   Seems core actually only calls this once, from Renderer::doCallback(). Seems weird to pass this error message in like this.

10. +++ b/core/lib/Drupal/Core/Security/TrustedCallbackTrait.php
    @@ -0,0 +1,89 @@
    +   * @param string $extra_trusted_interface
    +   *   (optional) An additional interface that if implemented by the callback
    +   *   object means any public methods on that object are trusted.
    

    This seems weird and dangerous. Why do we support this?

11. +++ b/core/lib/Drupal/Core/Security/TrustedCallbackTrait.php
    @@ -0,0 +1,89 @@
    +    elseif ($callback instanceof \Closure) {
    +      $safe_callback = TRUE;
    +    }
    

    Why do we automatically trust \Closure? Perhaps we could comment this?

12. +++ b/core/lib/Drupal/Core/Security/TrustedCallbackTrait.php
    @@ -0,0 +1,89 @@
    +      $message = sprintf($message, $description);
    

    Oh, there it is! ;) Double sprintf() to the "rescue". :/ This is some obscure magic for the formatting of $message. If it has to work like this, can we please document it somewhere?

13. +++ b/core/modules/block/tests/modules/block_test/block_test.module
    @@ -25,7 +25,10 @@ function block_test_block_view_test_cache_alter(array &$build, BlockPluginInterf
    -    $build['#pre_render'][] = 'block_test_pre_render_alter_content';
    +    $callback = function ($build) {
    +      return block_test_pre_render_alter_content($build);
    +    };
    +    $build['#pre_render'][] = $callback;
    

    Side note: I don't see how this makes the callback inherently more trust-worthy. ;)

14. +++ b/core/modules/color/src/ColorBlock.php
    @@ -0,0 +1,39 @@
    +/**
    + * Render callback object.
    + */
    +class ColorBlock implements TrustedCallbackInterface {
    

    DX: Not clear why this is called "ColorBlock". Nor what "Render callback object" means. Help?

15. +++ b/core/modules/filter/src/Element/TextFormat.php
    @@ -247,6 +247,24 @@ public static function processFormat(&$element, FormStateInterface $form_state,
    +   * @param array $element
    +   *
    +   * @return array
    

    Do these need/want descriptions?

16. +++ b/core/modules/node/src/NodeViewBuilder.php
    @@ -146,4 +148,11 @@ protected static function buildLinks(NodeInterface $entity, $view_mode) {
    +  public static function trustedCallbacks() {
    +    return array_merge(parent::trustedCallbacks(), ['renderLinks']);
    +  }
    

    Eeek, good point! I didn't even pay attention to all the other implementations of trustedCallbacks to see if others should be doing this, too. :/

17. +++ b/core/modules/system/tests/src/Kernel/Theme/FunctionsTest.php
    @@ -417,9 +418,112 @@ public function testIndexedKeyedLinks() {
    +    // Define the base array to be rendered, containing a variety of different
    +    // kinds of links.
    +    $base_array = [
    

    ...

    Kinda sad to completely copy/paste this huge block of code to wrap the second one in the legacy/deprecations stuff. :( Wonder if these can be refactored to better share code between the two test methods?

18. +++ b/core/modules/toolbar/src/Controller/ToolbarController.php
    @@ -52,4 +55,90 @@ public function checkSubTreeAccess($hash) {
    +    // Load the administrative menu. The first level is the "Administration" link.
    

    Nit: 80 chars.

19. +++ b/core/modules/toolbar/src/Controller/ToolbarController.php
    @@ -52,4 +55,90 @@ public function checkSubTreeAccess($hash) {
    +  /**
    +   * #pre_render callback for toolbar_get_rendered_subtrees().
    +   *
    +   * @internal
    +   */
    +  public static function preRenderGetRenderedSubtrees(array $data) {
    

    Why is this @internal when nearly everything else added by this patch is not? (Oh, I see later, because it's replacing a _ prefixed function).

20. +++ b/core/modules/toolbar/src/Controller/ToolbarController.php
    @@ -52,4 +55,90 @@ public function checkSubTreeAccess($hash) {
    +    // Load the administration menu. The first level is the "Administration" link.
    

    Nit: 80 chars.

21. +++ b/core/modules/toolbar/src/Controller/ToolbarController.php
    @@ -52,4 +55,90 @@ public function checkSubTreeAccess($hash) {
    +      // Many routes have dots as route name, while some special ones like <front>
    

    Nit: 80 chars.

22. +++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
    @@ -149,6 +149,15 @@ protected function defineOptions() {
    +  public static function trustedCallbacks() {
    +    $callbacks = parent::trustedCallbacks();
    +    $callbacks[] = 'preRenderMoveArgumentOptions';
    +    return $callbacks;
    +  }
    
    +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2126,6 +2126,15 @@ public function render() {
    +  public static function trustedCallbacks() {
    +    $callbacks = parent::trustedCallbacks();
    +    $callbacks[] = 'elementPreRender';
    +    return $callbacks;
    +  }
    

    Why not the array_merge() approach from above? Consistency++, right?

23. +++ b/core/modules/views/tests/modules/views_test_data/src/Controller/ViewsTestDataController.php
    @@ -24,4 +26,33 @@ public function errorFormPage() {
    +   *   A render array
    

    Nit: missing period.

24. +++ b/core/tests/Drupal/Tests/Core/Render/RendererCallbackTest.php
    @@ -0,0 +1,81 @@
    +    return [
    +      'Procedural function pre render' => [array_merge($render_array, ['#pre_render' => ['\Drupal\Tests\Core\Render\callback']]), 'Render #pre_render callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was \Drupal\Tests\Core\Render\callback. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725'],
    +      'Static object method post render' => [array_merge($render_array, ['#post_render' => ['\Drupal\Tests\Core\Render\RendererCallbackTest::renderCallback']]), 'Render #post_render callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was \Drupal\Tests\Core\Render\RendererCallbackTest::renderCallback. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725'],
    +      'Object method access callback' => [array_merge($render_array, ['#access_callback' => [$this, 'renderCallback']]), 'Render #access_callback callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was Drupal\Tests\Core\Render\RendererCallbackTest::renderCallback. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725'],
    +      'Procedural function lazy builder' => [['#lazy_builder' => ['\Drupal\Tests\Core\Render\callback', []]], 'Render #lazy_builder callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was \Drupal\Tests\Core\Render\callback. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725'],
    +      'Invokable object access callback' => [array_merge($render_array, ['#access_callback' => $this]), 'Render #access_callback callbacks must implement \Drupal\Core\Security\TrustedCallbackInterface or be a anonymous function. The callback was Drupal\Tests\Core\Render\RendererCallbackTest. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725'],
    +    ];
    

    Wonder if all this would be more readable if:
    a) This was multi-line.
    b) The only thing we're saving with "array_merge($render_array" is "#type => 'container'" -- maybe the lesser evil is to just have a fresh render array each time? ;)

25. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
    --- /dev/null
    +++ b/core/tests/Drupal/Tests/Core/Security/TrustedCallbackTraitTest.php
    

    Ran out of steam and time. My eyes glazed over at this. ;) So close to the end of this monster patch, but I have to stop here. Sorry for the incomplete review!

From a quick look at this, it seems like it's only giving a deprecation notice and there is no way to turn on the blocking behavior in 8.8? I would want to add this via a setting or similar and think about turning it on by default for new installs?

I think this patch looks great!

Here's some minor, nit-picky "review" that is totally ignorable:

1. +++ b/core/modules/system/tests/src/Kernel/Theme/FunctionsTest.php
   @@ -417,9 +418,112 @@ public function testIndexedKeyedLinks() {
   +   * Test the use of Links::preRenderLinks() on a nested array of links.
   

   Minor type-o:

   Links::preRenderLinks() should be Link::preRenderLinks() - an -s on Links:: vs Link::

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -756,4 +746,36 @@ protected function ensureMarkupIsSafe(array $elements) {
   +    $message = sprintf('Render %s callbacks must be methods of a class that implements \DrupalCore\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725', $callback_type, '%s');
   +    // Add \Drupal\Core\Render\Element\ElementInterface as an extra trusted
   +    // interface so that all public methods on Render elements are considered
   +    // trusted.
   +    return $this->doTrustedCallback($callback, $args, $message, TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION, ElementInterface::class);
   

   I think this is the code that would need to change if we wanted to implement something like @pwolanin suggests in #59 and make it possible to configure a site to throw an exception rather than call an untrusted callback. I think having this option is a great idea!

3. +++ b/core/modules/block/tests/modules/block_test/block_test.module
   @@ -25,7 +25,10 @@ function block_test_block_view_test_cache_alter(array &$build, BlockPluginInterf
   -    $build['#pre_render'][] = 'block_test_pre_render_alter_content';
   +    $callback = function ($build) {
   +      return block_test_pre_render_alter_content($build);
   +    };
   +    $build['#pre_render'][] = $callback;
   

   This appears to be wrapping in an anonymous function rather than converting this callback to a class and using TrustedCallbackInterface which seems like a less than ideal pattern. This is just a test so that's fine, but I guess I'd maybe worry that developers might see this pattern and use it in real code?

4. +++ b/core/modules/filter/filter.module
   @@ -305,17 +306,14 @@ function check_markup($text, $format_id = NULL, $langcode = '', $filter_types_to
   + * @see https://www.drupal.org/node/2966725
   
   +++ b/core/modules/filter/src/Element/TextFormat.php
   @@ -226,7 +226,7 @@ public static function processFormat(&$element, FormStateInterface $form_state,
   -      array_unshift($element['value']['#pre_render'], 'filter_form_access_denied');
   +      array_unshift($element['value']['#pre_render'], [get_called_class(), 'accessDeniedCallback']);
   

   A question: why get_called_class() and not static::class? I'm not sure the latter is really better, but get_called_class() is a function call which seems like it'd be slower, but PHP might optimize these to be the same thing anyway.

5. +++ b/core/modules/views/src/Form/ViewsFormMainForm.php
   @@ -21,7 +74,7 @@ public function buildForm(array $form, FormStateInterface $form_state, ViewExecu
   -    $form['#pre_render'][] = 'views_pre_render_views_form_views_form';
   +    $form['#pre_render'][] = [get_called_class(), 'preRenderViewsForm'];
   

   Same question here.

Re: #59: Indeed. That's basically what #56.6 is about. See @alexpott's reply in #57: "Because it we don't we'll break the Drupal 8 world."

I agree that a setting or config knob to enable "break the Drupal 8 world so I can fix anything potentially insecure" mode would be a great approach, not just generate the deprecation warnings. That seems like some lovely opt-in additional hardening we could provide now, not just after D9. So yeah, +1 to all that. ;)

However, I'm not sure about enable-by-default for new installs -- it has more to do with what contribs are doing things that might be broken. It's not simply an existing vs. new install question. It's about how "bleeding edge" your contrib footprint is. So yes to an option, but no to enabling it by default (until more of contrib can catch up ... probably in the post D9 world).

Thanks,
-Derek

p.s. I missed #60. Yup to a lot of that. #60.3 is exactly #56.13. ;)

@alexpott Thanks for continuing to push this forward!

I've put a comment on #3048484: Allows sites to configure a harden security mode to throw exception in when non-trusted callbacks are made but just wanted to add here that the security value of this issue is greatly decreased without a way to make this throw an exception in Drupal 8. Without that, we're basically just accepting that there could be a path to escalate lesser vulnerabilities to RCE until Drupal 9.

It would be nice if this could make use of #2982950: [meta] Standardize the approach for capturing and invoking callables across various subsystems..

@joachim I wasn't aware of that issue, but after a quick skim of it: Would using that simply be a matter of replacing the use of 'controller_resolver' here for 'callback_resolver'? If so, that's a relatively simple change, which I think could happen easily after this issue is committed - I don't see any reason to postpone this one on that one. Although, if that one does get committed first, yeah, we should certainly update this issue to use it. :-)

Without the ability to turn on blocking we are not even partially solving this issue. I would go the other direction and put the protection / exception on by default and provide an opt-out as we did for the image cache query string protection.

We can then remove the opt-out in Drupal 9.

@alexpott - ok, I understand this needs to be done in steps in some way, but I don't think it should go into core without at least the option to turn on the enforcing mode via a setting (roughly a 1 line change to this patch).

Looking at subclasses of Drupal\Core\Render\Element\ElementInterface, it includes some with methods like \Drupal\file\Element\ManagedFile::valueCallback() which can call other arbitrary callbacks in the array '#file_value_callbacks'

That method is actually part of \Drupal\Core\Render\Element\FormElementInterface. I haven't tried to see if I can exploit that example, but wondering if we can/should restrict further the methods allowed to be called on the extra_trusted_interface? Or add a blacklist?

Not yet finding an exploit path there - I thought I could build an element and use array_filter to apply an arbitrary callback (e..g system()), but running into this check:
A #lazy_builder callback's context may only contain scalar values or NULL.

Still, any static method on any RenderElement in core or contrib (including a form element) could be targeted (obviously), and #file_value_callbacks still seems risky when we start considering value callbacks for form elements in the next issue.

I still feel pretty strongly that we should have a way to turn on blocking for this right away. However, any kind of progress forward is better than no progress, so I'm going to mark this as RTBC, and perhaps we can get a follow-up in to have a "blocking mode" shortly after this one.

@alexpott addressed all my review concerns. I haven't had (and won't have) time to thoroughly review the last of the patch, but it was only in the final few pages of the test coverage that I ran out of steam. I also haven't reviewed the interdiffs since #62 but I'm sure it's all fine. +1 to RTBC so we can land this, un-postpone #3048484: Allows sites to configure a harden security mode to throw exception in when non-trusted callbacks are made and get on with the D8 hardening solution there (which will indeed be a very small patch once this is committed). $progress++ ;)

Thanks,
-Derek

Couple of smaller issues but one larger question (at the end)

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -756,4 +746,36 @@ protected function ensureMarkupIsSafe(array $elements) {
   +    $message = sprintf('Render %s callbacks must be methods of a class that implements \DrupalCore\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725', $callback_type, '%s');
   

   Nit: Drupal\Core, not DrupalCore

2. +++ b/core/lib/Drupal/Core/Security/TrustedCallbackInterface.php
   @@ -0,0 +1,41 @@
   +   *   List of the trusted callbacks provided by the implementing class.
   

   Does this need an example or more documentation to indicate it's literally the method name and not the fully qualified classname + method name?

3. +++ b/core/modules/history/src/HistoryRenderCallback.php
   @@ -0,0 +1,34 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function trustedCallbacks() {
   +    return ['lazyBuilder'];
   +  }
   +
   

   Seems slightly overkill to have to implement an interface with one method, that then points to the only other method on the class.

   I'm wondering a bit if we couldn't have two interfaces using the $other_interface feature - one for classes like this, and one where it's a larger class and the methods actually need specifying individually.

#2: docs are better. I think your example code is probably the minimum amount of code we'd have to provide to have an example, which is a fair amount.

#3 while I can't think of a good name, why not just an empty interface you can implement if you're providing a single use class that's only purpose is to provide a render callback? Like RenderCallbackInterface or something like that?

Interdiff in #81 looks good. One nit:

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -775,7 +775,7 @@ protected function doCallback($callback_type, $callback, array $args) {
     // Add \Drupal\Core\Render\Element\ElementInterface as an extra trusted
     // interface so that all public methods on Render elements are considered
     // trusted.
-    return $this->doTrustedCallback($callback, $args, $message, TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION, ElementInterface::class);
+    return $this->doTrustedCallback($callback, $args, $message, TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION, RenderCallbackInterface::class);

The comment no longer matches the code...

Otherwise, yeah, this seems cleaner and easier.

The 3 points from @catch in #78 are all addressed:
#78.1 via #79
#78.3 via #83
#78.2 -- class docs are now this, which IMHO is clear enough:

 /**
   * Lists the trusted callbacks provided by the implementing class.
   *
   * Trusted callbacks are public methods on the implementing class and can be
   * invoked via
   * \Drupal\Core\Security\DoTrustedCallbackTrait::doTrustedCallback().
   *
   * @return string[]
   *   List of method names implemented by the class that can be used as trusted
   *   callbacks.
   *
   * @see \Drupal\Core\Security\DoTrustedCallbackTrait::doTrustedCallback()
   */
  public static function trustedCallbacks();

Numerous thorough reviews on this. It's blocking #3048484: Allows sites to configure a harden security mode to throw exception in when non-trusted callbacks are made which would be a big security hardening win. Bot is happy with #83. Back to RTBC.

Thanks!
-Derek

p.s. This isn't a bug. This seems like a critical security hardening task, but not an actual bug or vulnerability.

#81 is what I was thinking and looks good.

Looks like some cruft crept in though:

+++ b/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php
@@ -0,0 +1,103 @@
+/**
+ * Ensures that TrustedCallbackInterface can be enforced for callback methods.
+ *
+ * @see \Drupal\Core\Security\TrustedCallbackInterface
+ */
+trait DoTrustedCallbackTrait {
+

+++ b/core/lib/Drupal/Core/Security/TrustedCallbackTrait.php
@@ -0,0 +1,99 @@
+
+/**
+ * @todo
+ */
+trait TrustedCallbackTrait {
+
+  /**

We only need one of these.

Simply removed that extra trait which catch spotted. Hopefully I've understood correctly and that is all that's needed.

Committed 5a42b47 and pushed to 8.8.x. Thanks!

FYI - the change record says:

Introduced in branch: 8.7.x
Introduced in version: 8.7.0

I'm assuming that should be 8.8.x / 8.8.0 respectively?

It's also unclear from the change record whether it is a deprecation or a hard removal - from the issue summary I'm assuming a deprecation at this stage?

Edit: Didn't want to edit the CR in case is actually correct...

Ohhh, no it's 8.8.x and a deprecation, will fix now!

I was just trying to update my code for Drupal 8.8.0 but also keep it compatible for drupal 8.7.x.

Since none of the interfaces exist my only option is to convert the callbacks to anonymous functions. Am I right?

> I was just trying to update my code for Drupal 8.8.0 but also keep it compatible for drupal 8.7.x.

That's one of the reasons why several people here were against already adding a mode that enforces this.

Short answer is you can't do that. If you rely on a feature added in 8.8, no matter what, you are no longer compatible with Drupal 8.7, that's why it is too early to make this change in contrib modules, you need to wait at least until 8.8 is released (And update your .info.yml accordingly).

Why not just add the interfaces to the Drupal 8.7.x branch?

Drupal\Core\Security\TrustedCallbackInterface
and
Drupal\Core\Render\Element\RenderCallbackInterface

I improved the release note a little. We'll also want to mention this again in the 9.0.0 release notes. Do we have a critical followup filed against 9.0.x yet to switch the default?

If I understand correctly, the only way to make a contrib module fully drupal 9 compatible is to drop 8.7 compatibility even though that version is supported til june 2020.

Cant there be something like #95 committed to 8.7 to solve that issue for contribs?

This workaround can work, but its not pretty that now basically most modules will have to add this dummy interface in their code.

https://git.drupalcode.org/project/address/commit/63a75fe

Looks like we missed a spot #3213572: #date_time_callbacks and #date_date_callbacks bypass the TrustedCallbackInterface protections

Another spot we missed #3218381: #file_value_callbacks bypass the TrustedCallbackInterface protections