Twig icon indicating copy to clipboard operation
Twig copied to clipboard

[Messenger] Unable to register extension "Twig\Extension\SandboxExtension" as extensions have already been initialized

Open PhilETaylor opened this issue 3 years ago • 4 comments

Back in 2013 https://github.com/twigphp/Twig/issues/962 - the solution proposed was to " recommend you to fix your code to avoid any issues in the future"

Well here we are in 2022... and using the Symfony Messenger component to run message:consume workers...

Now given that fetching services (auto injection) gets the SAME service every time

And given that a single message:consume process can run many messages, thus reusing the same container

And given that each message is going to render a Twig template, with a DIFFERENT set of sandboxed security policies in my case, I have today stumbled upon this error message on the SECOND message consumed by Message Handlers:

Unable to register extension "Twig\Extension\SandboxExtension" as extensions have already been initialized

My exact extract of code is not important, but here it is, basically the Security policy will change depending on the user that owns the template

The FIRST message consumed works perfect. The SECOND message errors with the above error.

        $policy     = new SecurityPolicy($tags, $filters, $methods, $properties, $functions);
        $sandbox    = new SandboxExtension($policy, true);
        $this->twig->setExtensions([$sandbox]);

Now reading https://symfony.com/doc/current/service_container/shared.html it says that if we dont share the service in services.php that it will create a new instance each time - this doesnt seem to be the case.

in services.php I tried the following with no luck.

    $services->set(Environment::class)
        ->args([service('twig.loader.native_filesystem')])
        ->share(false);

Maybe related? https://github.com/symfony/symfony/issues/40642

I dont think this is a support request, but rather reporting a limitation now of twig environment, which only allows a single set of extensions to be loaded, and thus in long running processes like messenger:consume (or other runtimes like RoadRunner) this can be a problem

Or maybe I have missed something and Im being an idiot again?

thoughts?

PhilETaylor avatar Feb 01 '22 18:02 PhilETaylor

Can't you mutate the sandbox via a call like the following?

$this->twig->getExtension(SandboxExtension::class)->setSecurityPolicy($policy);

fabpot avatar Feb 05 '22 08:02 fabpot

In my case I could change the policy for my user generated twig template on each run - you are correct.

HOWEVER. given that a single message:consume process can run many messages, thus reusing the same container, and given that fetching services (auto injection) gets the SAME service every time... when a different message handler for, say, the rendering of an email message, or use of twig WITHOUT a sandbox, then the twig service is now polluted with a Sandbox extension and we have to make a direct and specific check for that and disable it on every use of twig - which I admit we can do easily with the following code, but should we really be forced to do this?

if ($this->twig->hasExtension(SandboxExtension::class)) {
   $this->twig->getExtension(SandboxExtension::class)->disableSandbox();
}

Im thinking that, for long running processes like messenger:consume - a reset method for Twig would be a good thing, or at least honour the non-shared services mode as documented at https://symfony.com/doc/current/service_container/shared.html which, when dropped into services.php, doesnt seem to give a fresh service on each retrieval, but continues to give the same service. If this worked like the documentation says then we would have no pollution issue and we could simply set ->share(false)with

 $services->set(Environment::class)
        ->args([service('twig.loader.native_filesystem')])
        ->share(false);

and like the documentation says

Now, whenever you request the App\SomeNonSharedService from the container, you will be passed a new instance.

but that's not happening.

I also investigated $messenger->resetOnMessage(true); which is documented here: https://symfony.com/doc/current/messenger.html#stateless-worker ( I even fixed a bug in the documentation for you ;-) https://github.com/symfony/symfony-docs/pull/16465) but again that DOESNT seem to reset the state of the Twig service between messenger runs.

So I think this is at least 2 bugs...

  1. That shared services disabled is not passing a new instance
  2. that resetOnMessage(true) is also not passing a new instance.

PhilETaylor avatar Feb 05 '22 12:02 PhilETaylor

It seems like a broader issue related to using Twig in kept-alive processes.

For example, EasyAdmin extension suffers from the same problem https://github.com/EasyCorp/EasyAdminBundle/issues/3715

t-richard avatar Feb 17 '22 10:02 t-richard

It seems like a broader issue related to using Twig in kept-alive processes.

That's correct. At least in my opinion it is.

PhilETaylor avatar Feb 17 '22 10:02 PhilETaylor

How about add new method \Twig\Environment::setGlobal(string $name, mixed $vlue): void It will replace the global variable if not exists.

tourze avatar Mar 09 '23 10:03 tourze

Sorry, so much time has elapsed since I opened this, or worked on this area in my code.

I have just checked, and my workaround was not to use sandboxing when rendering the twig template in the messenger fed long-running process, but to use sandboxing while the users were creating their custom twig layouts (Which are email bodies for reports and reporting) and to validate the user input when they designed their twig layouts before I saved their custom twig layouts to the db.

This then means when I pull the custom twig layouts from the users account, when im running in messenger fed long running processes, they are already "clean" and only contain the tags/methods/functions/filters that the sandbox allowed them, so its safe to run without a sandbox in the long running process Twig service environment, when rendering the actual template (for sending by email)

Hope that helps someone else.

I still think there is a valid issue to be fixed somewhere here, but as Im the only one with this specific issue, and I have worked around it a long time ago, I'll close this issue today as not to be just old noise.

PhilETaylor avatar Mar 09 '23 15:03 PhilETaylor

If anything I confirm I came across the easy admin issue today which seems to boil down to Environment class in twig does not implement kernel.reset would it be possible on my application code to decorate this service so that I implement it on my side ?

as I'm new to decorating service and kernel.reset , my understanding is that I can not just nuke all private variables and I need my "reset" method to reset the service to its internal state : * right after it was initialized for the first time by the dependency injection system * just before the application code started to mess with its internal state

so for example if an extension is loaded through a services.yaml tag , i need to keep it (because that code will not be called at the next request in the same kernel) , but if an extension is directly added by doing addExtensions I need to discard it ?

@fabpot is my understanding correct ? is there a silver bullet pattern for this ? (as the way I see it, it's quite generic to all symfony services )

Edit: I'm thinking listening on some kind of "ServiceInitializedEvent" and/or overriding the kernel/container class to be able to plug myself there and then "cloning" the state at that moment using reflection

allan-simon avatar Oct 26 '23 01:10 allan-simon