- Issue created by @gillesbailleux
- ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
Problem is related only to twig because the very same problem occurs on another website when updating v3.14.0 => v3.14.1
otheruser@otherserver:~/public_html$ composer update -W Loading composer repositories with package information Updating dependencies Lock file operations: 0 installs, 1 update, 0 removals - Upgrading twig/twig (v3.14.0 => v3.14.1) Writing lock file Installing dependencies from lock file (including require-dev) Package operations: 0 installs, 1 update, 0 removals - Downloading twig/twig (v3.14.1) - Upgrading twig/twig (v3.14.0 => v3.14.1): Extracting archive Generating autoload files 47 packages you are using are looking for funding. Use the `composer fund` command to find out more!
- ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
Removing the drupal/twig_tweak (3.4.0) module does not solve the problem.
- ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
Same blank page appears when updating a block.
- ๐ซ๐ทFrance raphaelbertrand Lauris
same problem, related error seem to be:
PHP Fatal error: Allowed memory size of *** bytes exhausted (tried to allocate 262144 bytes) in ***/vendor/twig/twig/src/Extension/SandboxExtension.php on line 130 - ๐ซ๐ทFrance raphaelbertrand Lauris
Reverting change in src/Extension/SandboxExtension.php in this commit of twig/tiwg have same effect than downgrade to 3.14.0
Fix sandbox handling for __toString()
https://github.com/twigphp/Twig/commit/2bb8c2460a2c519c498df9b643d527711...if (\is_array($obj)) { foreach ($obj as $v) { $this->ensureToStringAllowed($v, $lineno, $source); } return $obj; }
- ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
@raphaelbertrand: any advice to increase PHP memory? If yes, where?
- ๐ซ๐ทFrance raphaelbertrand Lauris
i tried with differents settings (on differents servers), it doesn't seem to solve the problem.
- ๐บ๐ธUnited States mradcliffe USA
I also ran into this updating my local ddev development environment from 10.3 to 11.0, but prior to updating to 11.0 I did run composer update -W per upgrade instructions.
I added the empty stracktrace from #8 that I also got into the issue summary.
This seems like a bug report.
- ๐บ๐ธUnited States mradcliffe USA
# 3.14.1 (2024-11-06)
* [BC BREAK] Fix a security issue in the sandbox mode allowing an attacker to call attributes on Array-like objects
They are now checked via the property policy
* Fix a security issue in the sandbox mode allowing an attacker to be able to call `toString()`
under some circumstances on an object even if the `__toString()` method is not allowed by the security policyWe may be doing something that's triggering this based on the BC BREAK note here.
- ๐ซ๐ทFrance raphaelbertrand Lauris
same problem on taxonomy and many other edit form.
In text format config, it occur on text format with ckeditor5 enabled or when trying to enable it.
Maybe it is related to ckeditor5 ? - ๐ซ๐ทFrance raphaelbertrand Lauris
@mradcliffe
it seem to be related to this looking at the commit of twig/twig causing the error:
* Fix a security issue in the sandbox mode allowing an attacker to be able to call `toString()`
under some circumstances on an object even if the `__toString()` method is not allowed by the security policy - ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
@raphaelbertrand: when trying to edit a block which does not use ckeditor5, the same blank page appears.
- ๐บ๐ธUnited States mradcliffe USA
Following Issue Priority > Critical > Bug โ , I think this qualifies as a Critical bug report as the site becomes unusable when applying the release for content editors. Either render system or theme system.
- ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
Increasing the PHP APCu memory from 32 to 350 MB does not solve the problem.
- ๐บ๐ธUnited States mradcliffe USA
Enabling xdebug provides a better stack trace.
- ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
@mradcliffe: the infinite loop you spotted can only by fixed by the twig maintainer, right?
- ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
The Symfony founder and project lead posted this:
https://github.com/twigphp/Twig/security/advisories/GHSA-6377-hfv9-hqf6 - ๐บ๐ธUnited States mradcliffe USA
I don't know if this should be addressed upstream or not, @gillesbailleux. Reporting an issue upstream might help to get someone more knowledgeable about the right approach though.
- ๐ซ๐ทFrance raphaelbertrand Lauris
maybe reporting this to twig maintainers can help them to know that the sanbox can cause an infinite loop in certain case an write a new exception case to prevent this and throw debuging info to dev?
- ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
@mradcliffe: nothing can be done from the Drupal community so I guess the Symfony community will fix this.
- ๐บ๐ธUnited States mradcliffe USA
I think I spoke too soon about it being infinite in our case, but it does hit the recursion limit. Sorry.
Checking the is_object($v) helps to reduce that.
- ๐บ๐ธUnited States mradcliffe USA
I filed an issue upstream - https://github.com/twigphp/Twig/issues/4439
- ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
Thank you very much @mradcliffe
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
Is this because our objects reference themselves and we don't have recursion protection?
- ๐บ๐ธUnited States mradcliffe USA
Adding some tags, updated issue summary with sections from template.
I don't feel like only checking \is_object($v) would be acceptable upstream since the point of the method ensureToStringAllowed() method checks arrays as well, but as soon as we recurse via arrays too, we run into the recursion limit.
A potential Drupal-only solution would be to create a custom DrupalSandboxExtension that copies the code in SandboxExtension, and maybe checks $policy::$allowed_methods somehow to ensure that nobody has overridden core's TwigSecurityPolicy to remove __toString as an allowed method. If it is still allowed, then we can bypass the call to that method entirely and return $obj, and if somehow someone is doing something funky in custom land, then it would try its best with the original code.
We cannot extend SandboxExtension because it is final so this would require maintaining the code separately.
And then maybe we can work with twig/twig:4.x to allow for a way to override ensureToStringAllowed on the policy level rather on the extension level so we can get rid of it.
- ๐ฉ๐ชGermany jan kellermann
We had this problem, too. The update of twig/twig was triggered by roave/security-advisories:dev-latest.
We hat to downgrade and fix twig/twig to 3.14.0 in composer.json
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
- ๐ฉ๐ชGermany jan kellermann
Thank you larowlan, But the white page persists.
We changes serialize() with json_encode() and removed the first unset() befor the return statement to prevent recursion in different trees of array.
And for Drupal we had to remove the patches for test.
- ๐ฆ๐บAustralia elc
Forced downgrading to 3.14.0 has fixed this in D10.3.6 production sites.
Is also causing testing failures in contrib with useful error indication.
- ๐ฉ๐ชGermany olkoeller Hessen
How can I pin twig/twig to 3.14.0?
I am using Drupal 10 via Docker container with Dockerfile:
...
FROM drupal:10.2.3-php8.3-apache-bullseye
... - ๐ฉ๐ชGermany olkoeller Hessen
so I added
"conflict": {
"drupal/drupal": "*",
"twig/twig": "3.14.1"
},
to my composer.json.This seems to work, I do not get the error, when trying to add new content.
Thanks
- ๐ฆ๐บAustralia elc
twig 3.14.2 has been released:
- ๐ธ๐ฐSlovakia kaszarobert
In the meantime v3.14.2 was released. Does this problem affect that version also?
- ๐ฉ๐ชGermany jan kellermann
A new twig version is released:
https://github.com/twigphp/Twig/releases/tag/v3.14.2Big thanks to larowlan !
- ๐บ๐ธUnited States mradcliffe USA
twig/twig:3.14.2 and twig/twig:3.11.3 are released now by Fabien.
This needs to be in 11.x-dev since it needs to go into 11.0.x, 11.1.x, 10.3.x and 10.4.x.
- ๐ฆ๐บAustralia elc
Confirm that 3.14.2 fixes the issue for us here.
Does there need to be a change in Drupal (core-recommends)? I've just run composer update for twig everywhere and we're good to go.
The core-recommends is
twig/twig": "~v3.14.0"
so most everyone will now bypass 3.14.1 to .2 and never know this happened. Would it be safer to change that totwig/twig": "~v3.14.2"
? Or specifically add a conflict for a known bad version? - Merge request !10085Issue #3485956: Requires twig/twig:3.14.2 to ensure 3.14.1 is bypassed โ (Closed) created by mradcliffe
- ๐บ๐ธUnited States mradcliffe USA
Updating twig/twig caused some Functional failures so setting this to Needs work.
- ๐ฌ๐งUnited Kingdom jonathan1055
Pipelines for Contrib at 10.3.6 and 11.0.5 at the start of the week both had
Locking twig/twig (v3.14.0)
and ran fine.Earlier today both jobs had
Locking twig/twig (v3.14.1)
which caused many test failures.Re-running now gives
Locking twig/twig (v3.14.2)
and the tests pass, so in principle this has solved it for some contrib at least. - ๐บ๐ธUnited States mradcliffe USA
I ran the pipeline again on the merge request, and it passed.
- heddn Nicaragua
This is now resolved with the 3.14.2 release. See https://github.com/twigphp/Twig/pull/4441
- ๐ณ๐ฑNetherlands Remco Hoeneveld
Great stuf! can confirm the site i worked on was also fixed by adding `twig/twig": "~v3.14.2"`
- ๐ซ๐ทFrance XTaz
Just running
composer update
and it's ok for me, twig was updated to 3.14.2 - ๐บ๐ธUnited States mradcliffe USA
I'm going to re-open this as Normal since there is a workaround, but we should still update the dependency in core.
I'm removing the performance and security tags since that is no longer relevant. And maybe it's probably a task now.
- ๐ง๐ชBelgium gillesbailleux La Roche-en-Ardenne
The composer update -W command has installed the latest Twig version: v3.14.2.
I notice that nodes and blocks are now editable.
Many thanks to @mradcliffe for the tremendous support on this issue.
- ๐ฌ๐งUnited Kingdom longwave UK
Thanks, the MR looks good - because of the composer lock hash changes unfortunately I think we need MRs for all active branches (11.x, 11.1.x, 11.0.x, 10.5.x, 10.4.x, 10.3.x)
- Merge request !10090Issue #3485956: Updates twig/twig:3.14.2 in 10.3.x โ (Closed) created by mradcliffe
- ๐บ๐ธUnited States mradcliffe USA
10.3.x, 10.4.x and 10.5.x composer.lock files match previous release of composer so I cherry-picked the 10.3.x commit into branches for 10.4.x and 10.5.x.
- Merge request !10093Issue #3485956: Requires twig/twig:3.14.2 to ensure 3.14.1 is bypassed โ (Closed) created by mradcliffe
- Merge request !10094Issue #3485956: Requires twig/twig:3.14.2 to ensure 3.14.1 is bypassed โ (Closed) created by mradcliffe
- Merge request !10095Issue #3485956: Requires twig/twig:3.14.2 to ensure 3.14.1 is bypassed โ (Closed) created by mradcliffe
- Merge request !10096Issue #3485956: Requires twig/twig:3.14.2 to ensure 3.14.1 is bypassed โ (Closed) created by mradcliffe
- ๐บ๐ธUnited States mradcliffe USA
I cleaned up the issue summary a little bit and added the branches to the issue summary.
Changes look good on all 6 MRs (11.x, 11.1.x, 11.0.x, 10.5.x, 10.4.x, 10.3.x).
-
longwave โ
committed 8afbe179 on 11.x
Issue #3485956 by mradcliffe, jan kellermann, gillesbailleux,...
-
longwave โ
committed 8afbe179 on 11.x
-
longwave โ
committed 4f7d08fd on 11.1.x
Issue #3485956 by mradcliffe, jan kellermann, gillesbailleux,...
-
longwave โ
committed 4f7d08fd on 11.1.x
-
longwave โ
committed e7631fe8 on 11.0.x
Issue #3485956 by mradcliffe, jan kellermann, gillesbailleux,...
-
longwave โ
committed e7631fe8 on 11.0.x
-
longwave โ
committed 704eccb4 on 10.5.x
Issue #3485956 by mradcliffe, jan kellermann, gillesbailleux,...
-
longwave โ
committed 704eccb4 on 10.5.x
-
longwave โ
committed 65655b02 on 10.4.x
Issue #3485956 by mradcliffe, jan kellermann, gillesbailleux,...
-
longwave โ
committed 65655b02 on 10.4.x
-
longwave โ
committed d6c8acf0 on 10.3.x
Issue #3485956 by mradcliffe, jan kellermann, gillesbailleux,...
-
longwave โ
committed d6c8acf0 on 10.3.x
- ๐ฌ๐งUnited Kingdom longwave UK
Thank you for creating all the MRs and to everyone who helped out here with debugging and testing.
Committed and pushed the new minimum version to all six active branches.
- ๐บ๐ธUnited States themodularlab
I know this probably requires a new issue to be raised, but after updating to twig 3.14.2, I'm seeing extremely slow load times that did not exist prior to 3.14.1 or 3.14.2. Just curious if anyone else is also encountering performance issues.
- ๐ฌ๐งUnited Kingdom longwave UK
It's certainly possible that the security fix has a performance hit; please do open a new issue to discuss as others with the same problem are not necessarily going to find it here.
- ๐ฏ๐ตJapan acvellio
@themodularlab
On my site, performance deteriorated dramatically after updating from twig 3.14.0 to twig 3.14.2.
With twig 3.14.0, the time required to open the Node edit screen was a few seconds. However, after updating to twig 3.14.2, it now takes more than 30 seconds.
- ๐ฎ๐นItaly senzaesclusiva
I encountered the same problem in a from scratch D 10.3.7 installation. The #52 suggestion โcomposer update -Wโ solved the problem.
For the moment I do not notice any performance slowdown with the Twig update to 3.14.2, but it should be considered that I have few test nodes and with few fields per node. Maybe as I continue to develop the site the problem may arise.
One possible issue to keep monitored
My 2 cents - ๐ฎ๐นItaly senzaesclusiva
I wanted to add an aspect that puzzles me with respect to this issue.
I am not a programmer and am unfamiliar with the code, but I am quite skeptical that the problem that occurred is caused by Twig.
Why?
I am managing the development of 3 sites installed starting with D 10.3.6 and later upgraded to D 10.3.7, in none of them did I run 'composer update -W' , which maintain Twig version 3.14.0.
So I am wondering why, if D10.3.7 installs|updates|requires Twig to version 3.14.1 , composer does not warn of the need to update it.
I am a little confused, about this - ๐ฌ๐งUnited Kingdom longwave UK
@senzaesclusiva This only affects people who download the zip/tar.gz version of Drupal and do not use Composer; those files contain the latest compatible versions of all dependencies, which happened to be Twig 3.14.1 for Drupal 10.3.7.
- ๐ฎ๐นItaly senzaesclusiva
Ahhh...that's the mystery I had missed :-)
Thank you very much Automatically closed - issue fixed for 2 weeks with no activity.