Remove technical debt and complication from when doTrustedCallback() could either trigger errors or throw exceptions

Blast, didn't realize this was still outstanding.

Yup, fell off all of our collective radar. Whoops. Now what? Can we still do this?

Hrm, looking more closely at core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php:

 /**
   * ...
   * @param string $error_type
   *   (optional) The type of error to trigger. One of:
   *   - TrustedCallbackInterface::THROW_EXCEPTION
   *   - TrustedCallbackInterface::TRIGGER_DEPRECATION
   *   - TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION
   *   Defaults to TrustedCallbackInterface::THROW_EXCEPTION.
   *...
   */ 
  public function doTrustedCallback(callable $callback, array $args, $message, $error_type = TrustedCallbackInterface::THROW_EXCEPTION, $extra_trusted_interface = NULL) {

Seems a bit more can-o-wormsy than I first suspected. Do we:

A) Completely remove the $error_type argument? That's gonna be trouble since $extra_trusted_interface still needs to exist, so callers will be confused if we remove that argument out from under them.

B) Ignore the requested $error_type and always behave like THROW_EXCEPTION was passed?

The good news is that grep appears to find this as the only live caller in the 9.0.x branch:

core/lib/Drupal/Core/Render/Renderer.php

  return $this->doTrustedCallback($callback, $args, $message, TrustedCallbackInterface::THROW_EXCEPTION, RenderCallbackInterface::class);

The only other hits are in core/tests/Drupal/Tests/Core/Security/DoTrustedCallbackTraitTest.php

So it looks like D9 is already setup to be throwing this exception. But changing the trait seems fraught with peril and BC implications.

Maybe option C) for "What now?" list should be "Close this as 'by design'" since D9 is already throwing exceptions, not trigger_error(), but ripping out all the plumbing that allowed for trigger_error is too much API change to be worth the trouble.

Not sure if/how to proceed, or if this is really still major.

Please advise.

Thanks!
-Derek

Since it's a security hardening, I might still consider it during beta (depending on the specific patch). Probably not during RC though... =/

I really thought we'd already fixed this for D9 in #2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE. I'll try pinging Sam for clarification...

Indeed, as I tried to explain in #7, we're already throwing an exception, not triggering an error, since the caller in core/lib/Drupal/Core/Render/Renderer.php is invoking this method with the TrustedCallbackInterface::THROW_EXCEPTION argument in the 9.0.x branch. The 8.9.x branch's copy of Renderer.php uses TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION, instead.

That's why I don't think this is major. I think a more accurate issue title might be:

"Remove technical debt and bloat from when doTrustedCallback() could either trigger errors or throw exceptions"

I don't think that's a panic-inducing blocker or anything. Yes, it'd be nice to clean up. Maybe there's still time before RC. But I think D9 is already security-hardened on this point.

Probably won't fly, but here's a proof-of-concept for what I think the correct scope of this issue really is. D9 is already throwing exceptions. That happened at #3104307: Remove BC layers in various Drupal\Core components (woah, weird scope management there).

(crosspost with above)

> Indeed, as I tried to explain in #7, we're already throwing an exception, not triggering an error, since the caller in core/lib/Drupal/Core/Render/Renderer.php is invoking this method with the TrustedCallbackInterface::THROW_EXCEPTION argument in the 9.0.x branch. The 8.9.x branch's copy of Renderer.php uses TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION, instead.

Yes, I did change that in the respective issue to remove deprecated code, so agreed, this would be more about changing the default. I'm not sure it's that important or even required. Might still be useful to keep a way to do a deprecation? Maybe we missed a certain type of callback or want to use it for something else as well, so this could be useful again in the future.

I don't think this is major and can probably be 9.1+?

Whoops, added a stray space in a comment in #10. This is better...

whoops! More x-post fun. ;)

Agreed this isn't major or a beta target or all that stuff. Could happen later. I'm not sure it makes sense to keep all the deprecation warning plumbing in place for this.

But perhaps it is, especially since we might use it for #2966711: Limit what can be called by a callback in form arrays. If we're going to scramble to work on anything, that probably makes more sense than this.

Yeah I don't think we should be doing this until we've worked on #2966711: Limit what can be called by a callback in form arrays and possibly never. As potentially contrib might add a render element with it's own special callback and then decide to implement trusted callbacks and want to use the deprecation path.

I'd be in favour of postponing this issue at this point.

Also the issue summary needed an update as 9.x was changed to throw exceptions by #3104307: Remove BC layers in various Drupal\Core components.

Whatever the status this certainly doesn't need to be done as part of 9.0.x.

Also in order to actually implement this in 9.1.x we'd need to deprecate calling this with the TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION and TrustedCallbackInterface::TRIGGER_DEPRECATION :D - deprecate inception!

The above puts me in the won't fix camp for this issue.

I did consider whether we needed to remove this path when we were made the following change in #3104307: Remove BC layers in various Drupal\Core components

-    return $this->doTrustedCallback($callback, $args, $message, TrustedCallbackInterface::TRIGGER_SILENCED_DEPRECATION, RenderCallbackInterface::class);
+    return $this->doTrustedCallback($callback, $args, $message, TrustedCallbackInterface::THROW_EXCEPTION, RenderCallbackInterface::class);

I thought along the same lines as #11 and #14 - that this might be useful in future deprecations of other callback types; we can use the silenced deprecation feature to start with and switch to the exception in the next major release, as we did with render callbacks. There is no security issue here at the moment because nothing in core uses TRIGGER_SILENCED_DEPRECATION except a test for the functionality itself.

Great. I agree all around. More accurate version and status. If something is pointing here in a 9.0.0-panic-list somewhere, we should remove it.

Thanks,
-Derek

p.s. I still think it's weird and poor form to have made the change described in the original summary here as a side effect of #3104307: Remove BC layers in various Drupal\Core components. That was seriously confusing scope management. The security hardening aspects of the diff in #17 have nothing to do with the title of #3104307. This wasn't a BC layer at all. It was an escalation of security hardening behavior. What's done is done, but as someone who's been regularly chastised for bad scope judgement, I feel compelled to complain about it. ;)

It seems to me that this also relates to what is proposed in #2982950: [meta] Standardize the approach for capturing and invoking callables across various subsystems

Rebased this as an MR - we can unpostpone this now we're nearing D12

Getting late but would be good to get this in

Looking at https://search.tresbien.tech/search?q=doTrustedCallback%20-f%3Acore&num=... I think we need to deprecate the argument. I've done this.

For example https://git.drupalcode.org/project/twig_field_value/-/blob/0371644251620... will break if we just remove the code.

Thanks @alexpott

I came to review with eye to RTBC but instead fixed the test failures - just a variable collision - $args already existed

Added some nits and wrote some words in the change record.

Thanks for the review @longwave

All looks good to me, let's do it.

Committed/pushed to main and 12.0.x, thanks!

This will need a backport MR for 11.x

I created the 11.x MR. The conflict was in the test due to #3562361: Add type hints to core/tests code via Rector - round 2

Patches are identical other than a change to a test method signature.

Committed/pushed the backport MR to 11.x, thanks!