Harden TwigSandbox methods

> There actual is _no_ return type for this method

Twig 3 declares the return type as void so any return statement would error, but we are still on Twig 2 :(

+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -55,15 +56,32 @@ public function __construct() {
+        @trigger_error('Not specifying a fully-qualified method name is deprecated in drupal:9.3.0 and will throw an error in drupal:10.0.0.', E_USER_DEPRECATED);

This message should probably explain that it's part of twig_sandbox_allowed_methods. Also needs a test and change record.

+ if (strpos($method, $prefix) === 0) {
As Drupal core requires "symfony/polyfill-php80" it should be safe to use str_starts_with.

Very sensible. Implementation looks complete, tests look good.

Twig\Sandbox\SecurityError: Calling "id" method on a "Drupal\Core\Layout\LayoutDefinition" object is not allowed. in Drupal\Core\Template\TwigSandboxPolicy->checkMethodAllowed() (line 10 of core/themes/stable/templates/layout/layout.html.twig). 

I added LayoutDefinition::id because its a plugin not an entity. Maybe we add PluginDefinitionInterface? Its probably safe but I just added LayoutDefinition in this pass to see how things go. Thoughts?

I'm concerned about contrib here. It's easy for sites to add to twig_sandbox_allowed_methods. But how do contrib modules and themes do that if they need to?

The only clue I could find was https://stackoverflow.com/questions/16070602/how-to-properly-enable-the-...

Even if there's no supported way for contrib to currently add methods, it could be frustrating if we removed global support for id etc and there was no easy fix for contrib to restore it.

I wonder it's best to keep id and label global for now, and discuss making them more specific in a follow-up. This issue would add good infrastructure for more precise and harder policies, even if it didn't do much hardening itself.

The best hardening I have thought of would be to create a DrupalObjectInterface or TwigSandboxableInterface (without any methods on it itself), and have EntityInterface, PluginInterface, Url, etc extend from it. By extending from it these child interfaces would be contracting that they shared the generic Drupal understanding of id, label, get, toString as safe operations.

If we wanted to be really radical, we could give DrupalObjectInterface an isSafeMethodCall($method) {} and therefore delegate responsibility for twig sandboxing to the classes themselves. After all, they are the ones who know what their own methods mean.

1. +++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
   @@ -55,15 +57,32 @@ public function __construct() {
   +      if (isset($this->allowed_methods[$name])) {
   +        $this->allowed_methods[$name][] = $class;
   

   Worth commenting this condition is for BC and can be removed in 10.0?

2. +++ b/core/tests/Drupal/Tests/Core/Template/TwigSandboxTest.php
   @@ -107,54 +116,190 @@ public function testEntitySafePrefixes() {
   +      public function bundle() {
   +        return 'id';
   

   Nit: should return "bundle" for consistency?

If tightening some of these globals is holding the issue back from getting committed we could make them global again. Since this is about that hardening, it seems worth going as far as possible

It's not only about hardening. It's also about making it easier to add other much needed exceptions like entity::toUrl and URL:: setAbsolute.

I'm happy to RTBC as is one we have a CR, but I suspect there will be more discussion of the BC.

adding extension for contrib being the method for adding their own security exceptions. That's actually exactly how core's is doing it are a couple contrib modules adding extensions with tweaks and features so this doesn't seem a big deal.

I tried a grep at http://grep.xnddx.ru and could not find anything altering the sandbox policy. We should have a working example of this for the CR if we are going to break BC for id and label. I'm happy to write the CR if you can provide the snippet.

Ah, of course. Preprocess is the right solution for the CR.

How about we deprecate calling id & label from twig outside of EntityInterface? i.e. trigger a deprecation error if they are used in the sandbox on an object that doesn't extend EntityInterface.

That would progress the hardening but respect our usual BC approach.

The CR looks good, I've tweaked slightly.

Stepping back to look at this problem space more broadly I come to think that:

1. If being class-specific is good for twig_sandbox_allowed_methods then probably it's good for twig_sandbox_allowed_prefixes too. The present patch only does methods.
2. It would be good to have this sandbox logic handled by a service so that sites or contrib could decorate it.
3. It's unfortunate that methods added in settings.php replace the defaults rather than add to them; this means that if you wish to add a method, you have to take maintenance responsibility for the whole list of methods.
4. I dislike the way that this central policy, which lacks intimate knowledge of the objects, has the job of making decisions about which are safe. I like the idea of creating a TwigSafeMethodInterface with an isTwigSafeMethod($function) method, that can be used on classes like EntityBase to allow decentralising the logic for this.

@neclimundul I'd appreciate your opinion on which of these are relevant to this issue, which are worth a follow-up, and which I should forget about.

I'm not going to drag the issue down with unwanted scope-creep, we'll get this to RTBC soon. But the _prefixes question above deserves at least a reason to be given here.

+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -88,20 +107,27 @@ public function checkPropertyAllowed($obj, $property) {}
+        if (str_starts_with($method, $prefix)) {

str_starts_with requires PHP 8, but we're still supporting php 7.4.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

What are the odds this could cause a breaking change for existing sites?

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

This looks good to me.
The main issue here is the disruption to existing sites who don't have an existing entry in settings.php who will now see their default change.
I think the only way we can handle that is with a release note, so tagging for needing a release note in the issue summary.
The change notice also looks good, but needs some version number updates. I think this is most likely 11.2 territory given we're in RC for 11.1, so I think it would be best to use that as the version number.
I'd be keen to see this go into 11.x (and therefore 11.2) early in the minor cycle.

Tagging as NW for the CR/Release note updates

I pushed a commit to fix the version number in the deprecation message, and also updated the CR. Leaving NW for the release note.

I'm not clear if we should be adding a default version of this to default.settings.php.

Added release note snippet to IS

Removing the release note tag thanks to #41.

However, I opened a new MR thread that should be addressed before merging (either renaming the function, or at least fixing the docs, or possibly refactoring this code again).

Via Slack, @alexpott confirmed:
“ I think adding another setting is a good idea - twig_sandbox_allowed_class_methods seems best”
“ It is a bit least worst possible option - but sometimes that’s what you need.”

I’ll work on a version of this later today.

Ugh, sorry. Got busy with too many other things. If anyone else wants to run with this, please do. I'll reassign if I pick it back up.

Not directly related to the topic of this issue, but related:

Why is ::uuid not allowed as well for the types where ::id is allowed? At least this would make sense in relation to #1726734: Replace serial entity IDs with UUIDs in URLs, or even globally?, #2353611: Make it possible to link to an entity by UUID and related issues.

@mvonfrie, re #45: Feel free to open another postponed child issue about that, much like #2907810: [PP-1] Add $entity->toUrl() and $entity->toLink() methods to allowed methods list in Twig sandbox policy. Once this issue is merged and fixed, it'll be much easier and safer to expand the allowed methods and target specific interfaces where we want to allow other methods.

Thanks,
-Derek

Created #3506428: [PP-1] Add $entity->uuid() methods to allowed methods list in Twig sandbox policy.

It's probably too late for this idea, but nevertheless - perhaps for a future iteration: I think it'd make much more sense to use PHP attributes for this and specifically mark the methods that are really intended for consumption in Twig with #[TwigAllow]. Currently it's IMHO also a problem that contrib modules can't easily extend this.

But we now have this?

https://www.drupal.org/node/3551699

Shouldnt this be used instead? Just wondering... Or perhaps I'm confusing something (sorry in advance if that's the case)?