Leverage attributes in Twig Sandbox Policy

I agree that having custom modules specific configuration information
in settings.php is undesirable. The workaround I use is
to define a 'marker interface' (named TwigAccess) and
to allow that class in the settings.php. A 'marker interface'
is an PHP interface that does not specify any methods. Classes I intend for
twig access implement TwigAccess and be allowed access with
no further ado.

Two issues come to mind.
1. My app has a base module where I can define the shared interface
once and for all. Just the one class is added to settings.php when
the base module is installed.
Many apps won't have such a base module. Perhaps it would make sense to
add the marker interface to core?

2. Some people dislike marker interfaces on principle.
It does feel a little dodgey; but better than relying on settings.php.

Anyway, the TwigAccess interface approach solves my problem, might be of use to others,
and might point towards a resolution to this issue.

I'd be glad to post the little bits of code if this is of interest and unclear.

There has only been two comments here in 10 years. Is there anything to do here?

I am setting the status to Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Almost 10 years ago since this ticket was created. I'd like to bringt it back to life, because even after all this time, I think the request or problem stated is still very valid: Currently you can only extend the allowed methods and classes of Twig via $settings. A use can do this, but a module author can't do this. They could only add it to the documentation of their module: "Please, if you install this module you need to add this line to your settings and if you installed this module you can remove this line again". This is very inconvenient and unflexible. Modules should be able to provide objects like from the built in "Attribute" class to pass to twig that can be used freely in Twig and where you can call any method on. Actually, I believe you can currently inherit your class from Attribute and it will work, but that's of course a very ugly hack.

At the moment, there are some method prefixes that will always work, but considering the discussion in #3239105 this will get hardened in the future.

Now, there are many different options to solve this:

1) Provide an interface in core (TwigSafeInterface) that is included in core and has no methods but is by default considered safe. Any classes could implement this interface and be safe. Disadvantage here is that you can't just mark individual methods as safe.

2) Turn the policy into a service so it can be decorated: At least better than the current situation, and that's what's been requested for here in this issue, but of course a bit complicated for module authors with a significant amount of confusing boilerplate code.

3) IMHO the most modern method that is now available to us with modern PHP: Extend the default security policy so that it looks at PHP attributes like #[TwigSafe] on classes and methods. This would be the easiest and most flexible method to solve this IMHO.

Modernizing the IS.

Implementation is dead simple. NW for tests though.

Test is equally trivial.

Added change record draft.

Looks great. Wonderful to see this finally moving forward after all this time. Thanks for the implementation! Just a few more thoughts on this: Do we also need to allow this on whole classes? That could eventually replace the somewhat arbitrary special case for HTMLAttribute... Or, do we force developers to use it on any individual method - could be safer, I don't know. And, looking at the issue summary, we still don't protect properties, right? I guess this would be only about writing to them. Is this actually possible in twig? set obj.test="xxx" isn't possible as far as I know. Might be wrong though.

@rgpublic:

> Thanks for the implementation!
Nice you like it. Maybe you want to review and RTBC.

> And, looking at the issue summary, we still don't protect properties, right?
> I guess this would be only about writing to them. Is this actually possible in twig?
You are right (yet!), so that point is minor and i removed it from the IS.

> Do we also need to allow this on whole classes? That could eventually replace the somewhat arbitrary special case for HTMLAttribute...
> Or, do we force developers to use it on any individual method - could be safer, I don't know.
I suppose the latter.

That said, i decided to focus on the first step with a huge quick-win, and postpone ALL other changes and bikeshedding discussions about deprecation paths to a followup.

Well, I don't know whether I'm entitled to do this, but if I take the "Reviewed and tested by the community" verbatim, I am part of the community :-) so....

The patch is really simple and looks good to me. I wanted to take my job a bit more seriously though, so I applied the patch to an actual project where I really had that problem this patch is meant to solve. Until now, I've subclassed my class from Attribute as a hack. I removed that and of course the sandbox exception appeared. Now, I've patched Drupal and added the attribute. Works! Removed the attribute again: Sandbox error. So this seems to all works as intended. Great! I can now confirm that this patch works in real life. I couldn't test the test (wow recursion, lol) because it didn't apply to my older Drupal version but it looks good to me as well.

One final thought, before I would set this to RTBC: I've noticed that the new folder (and namespace) is called "Attribute". Could it be a bit confusing that we already have a class that is called "Attribute"? Perhaps we should call the folder "Attributes"? Or perhaps even omit the subfolder altogether for just that single attribute so far? I don't know...

Setting RTBC per #31

#31.2:
Yes i wondered too whether the php attribute should live in a "PhpAttribute" dir, as we have a html attribute class called Attribute. I decided to go with the quasi-standard dir name. If maintainer or committer has a different view on this, it's a trivial change to make.

I really like this change - in fact I think it is fantastic and think we should consider using it in core as a follow-up.

However I think the documentation on the attribute needs work as we should provide context about when it should be used.

> I really like this change - in fact I think it is fantastic and think we should consider using it in core as a follow-up.

Nice you like it!

Self-review: This adds TwigAllowed method attribute. Legacy pattern-based whitelisting is added to that. Maybe add a TwigAllowedByAttributeOnly class attribute, to opt out of legacy behavior. This is needed to cleanly deprecate legacy.

> However I think the documentation on the attribute needs work as we should provide context about when it should be used.

Yes right. If this is the future, it must be well documented and good UX.
Added verbose docs, and updated the CR.
Also thought a bit about possible deprecation paths, and added an all-or-nothing opt-in class attribute, plus tests.

Tests are green (spellchecking has cleanup hiccups).
Test-only tests fail like they should. So RTBC again.

Added followups, postponed for now.

Ups, of cource technically the changes need review.

I'm unsure whether the "TwigAllowedByAttributeOnly" is really such a great idea. We'd need to deprecate this attribute again in the future. Anyone using the new TwigAllowed attribute would/should be aware of the current whitelist. Currently whitelisted are e.g. methods starting with get / is etc. I think it would probably be just fine if the new TwigAllowed attribute is additive to the whitelist - just as proposed - and then one day we do hand out deprecation notices for every method call that is just allowed due to the whitelist but missing that attribute. In a further step, the whitelist would be set to empty and then subsequently removed. I think that deprecation path would be sufficient and wouldn't need that additional auxiliary attribute. Just my thoughts on this...

Hmm, i see your point!

So we have:
- Verion 1 - only TwigAllowed
- Bad: In the first step, whitelisting for a class can be a mixture of (legacy) Magic and explicit Attributes
- Good: Only one attribute, only one deprecation step
- Verion 2 - TwigAllowed + Class attribute
- Good: The class attribute makes it crystal clear if legacy Magic OR explicit Attributes rule
- Bad: One more deprecation step

I don't have strong feelings.
@alexpott, do you have?

Echoing @rgpublic in #40. Thanks so much for picking this up again @geek-merlin and @rgpublic! Let’s go with version 1 on a single #[TwigAllowed] attribute and skip TwigAllowedByAttributeOnly (looks like you did already if I read the latest code correctly). Seems like a simpler approach with less clean-up later.

I merged 11.x in because the branch was very out-of-date and that was causing pipeline fails for spell-checking.

I think adding TwigAllowedByAttributeOnly is unnecessary too - let's remove that complexity - it is still in the MR.

Thank you all for your feedback, and for saving me from barking up the wrong tree ;-). Partial revert is coming.

Here we go: Initial 1-attribute approach, verbose phpdoc. CR updated too.
Tests are green, test-only red the way expected.

Please someone get this to RTBC - I'd love to land this in 11.3.0

Bump!

@alexpott: I can't self-RTBC, but code-wise this this is back to #37 which was RTBC.

I tried the new version from #45 again on my project. It works. The code is also pretty simple, so I think I can safely set this to RTBC again. I'm also really looking forward to this. It's really a brilliant step forward for a saner, more modern and better extensible Drupal/Twig ecosystem IMHO. It just makes so much more sense. The current situation is also not really safe. If there was e.g. some method "function isleOfManDestroy()" the beautiful British isle could be destroyed from Twig just because it starts with "is" ;-) That's not really a solid approach and it will make the whole system much safer if we actually declare what's actually intended to be used from Twig in the future.

Committed and pushed c2e40edf741 to 11.x and 3acf8ea004c to 11.3.x. Thanks!

Thanks everyone!