Harden TwigSandbox methods

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.

Reroll. The conflict was with core converting to starts_with I believe. No useful interdiff, the patch is the same.

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

Looking at this, it was targeted originally at deprecation in 9 and removal in 10. The current patch is the removal, not the deprecation so... likely to break things.

This needs to be re-rolled to deprecate this in 10 like the earlier patches did for 9.

Tiny change but I've been I've been running in circles on some other things... This should be the non-breaking version. It just throws that deprecation message if your site doesn't match the hardened format.

The last submitted patch, 28: 3239105-28.patch, failed testing. View results →

Quick reroll on MR

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.)

broken by 📌 Remove misc usage of whitelist in tests and comments Fixed . reroll soon.

Also conflicted with 🐛 Performance Degraded after update to twig 3.14.2 Active

merge request made and passing so back to NR.

The performance regression changes where a bit disruptive. They make this change kinda BC breaking because now the settings are directly exposed in the less secure expression format. Which is really frustrating and I don't really know how to resolve it.

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.

I was confused why we hadn't changed default.settings.php and the answer is that its not currently there. This is one of those power user settings we don't document and only set if you're running into an edge case.

I suspect that will continue and changing that would be its own issue but I understand how its tied up in this. I think we can mitigate this by linking to the documentation in the changelog and maybe release notes so in the unlikely case this does push someone into the edge case they know how to resolve it.

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? → , ✨ Make it possible to link to an entity by UUID Active and related issues.

@mvonfrie, re #45: Feel free to open another postponed child issue about that, much like ✨ Add $entity->toUrl() and $entity->toLink() methods to allowed methods list in Twig sandbox policy Needs work . 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 ✨ [PP-1] Add $entity->uuid() methods to allowed methods list in Twig sandbox policy Postponed .