search

humhub-contrib / popover-vcard

0 stars 4 forks source link

Twig Sandbox/ Security policy #23

Open dantefromhell opened 8 months ago

dantefromhell commented 8 months ago

The twig security policy keeps giving me headaches due to its restrictive settings, looking at #16 #18 #21 I'm not the only user struggling with this.

Todays issue is the twig filter trim which is super helpful to trim any accidental whitespaces in the beginning and end of profile fields not being permitted.

According to VCardUser.php only if, for and escape are allowed, while twig has a lot more functions to offer [1].

I do love the popover module but I find the current situation very irritating, each version updates requires manual fixing of functionality that worked in 1.1.1.

I've tried to understand why the policy was introduced, but I wasn't able to find any hints in the commit history/ messages.

I would love to get to a place where updates of this module don't break working configuration and I wonder what the best path forward is? My current ideas are:

  1. Properly handle the the \Twig\Sandbox\SecurityError so it won't lead to HTTP/500 errors.
  2. Review the Twig language reference [1] and create a list of harmless functions and whitelist them in the SecurityPolicy. E.g. I don't understand why the current policy allows if but not for.
  3. Document the allowed twig expressions accordingly. As of 2024-02-14 the module configuration dialogs has no mention of the security policy being and place and what it restricts. Users can enter any valid twig expression, and learn of the not enabled twig expression by troubleshooting HTTP/500 errors.
  4. As an extension of 2) it would IMO be great if the module configuration dialog will show an error message if any forbidden twig language element is used.

Alternatively (which might require careful consideration) the twig SecurityPolicy could also be removed.

What is the best way forward to resolving this?

[1] https://twig.symfony.com/doc/

luke- commented 8 months ago

We could create a module configuration similar to the CustomPages module. Here, additionally required Twig features could be activated by the using the configuration file if required.

https://github.com/humhub/custom-pages/blob/master/Module.php#L34-L41

It was introduced because server-side scripting was also possible via Twig, which is not desired.

Please create an issue for improved error handling (point 1).

dantefromhell commented 7 months ago

Thanks for the hints @luke- this is indeed helpful.

As I'm drafting a PR I'm wondering though how to best handle the allowedMethods attribute of enableTwiqSandboxExtensionConfig. Needing to whitelist every profile field explicitly feels like much overhead. Any suggestions?

Please create an issue for improved error handling (point 1).

Does #21 not cover the HTTP/500 issue sufficiently?