neos
class(.*\\SpecialController) leads to ProxyBuilding of TYPO3Fluid\\Fluid\\Core\\Variables\\StandardVariableProvider
Description
If you use a matcher in your Policy.yaml like that:
privilegeTargets:
Neos\Flow\Security\Authorization\Privilege\Method\MethodPrivilege:
MyPrivilegeName:
matcher: "class(.*\\StartController)"
Flow will build a proxy for TYPO3Fluid\\Fluid\\Core\\Variables\\StandardVariableProvider which then does not match TYPO3Fluid\\Fluid\\Core\\Variables\\VariableProviderInterface because of the constructor definition, resulting in a PHP Fatal error.
Expected behavior
While we probably cannot change the "awkward" decision of TYPO3Fluid we probably should not all of sudden build proxy for every class, if we stumble over such an expression.
I'm still unsure, where the problem exactly arises but believe it comes from here:
\Neos\Flow\Aop\Pointcut\PointcutClassNameFilter::reduceTargetClassNames but I cannot get the big picture together.
Flow: 6.3
The proxy building shouldn't be affected by your policy, i.e. proxies are always created unless a class has a Proxy(false) annotation or isn't a class. => Flow won't create a proxy for VariableProviderInterface.
resulting in a PHP Fatal error.
Could you share that?
The error?
[Tue Aug 31 19:21:55.347686 2021] [proxy_fcgi:error] [pid 30257:tid 140693842413120] [client 127.0.0.1:44376] AH01071: Got error 'PHP message: PHP Fatal error: Declaration of TYPO3Fluid\\Fluid\\Core\\Variables\\StandardVariableProvider::__construct() must be compatible with TYPO3Fluid\\Fluid\\Core\\Variables\\VariableProviderInterface::__construct(array $variables = Array) in /projectpath/Data/Temporary/Development/Cache/Code/Flow_Object_Classes/TYPO3Fluid_Fluid_Core_Variables_StandardVariableProvider.php on line 410'
Coming when renderin any Fluid template.
And I'm absolutely with you. It should not, but it does.
Ah, the problem is that the interface declares a constructor (which is considered bad style btw) in this case.
That and the fact that apparently Flow changes the constructor for the StandardVariableProvider somehow.
Is it possible that your privilege matches that class somehow?
I'm not sure if class(.*\\StartController) does what it is expected to do. Btw what is it expected to do? Match all classes that happen to contain the string "\StartController" in their fully qualified name?
The privilege matches, even if it definitely should not. And the matches function returns false for this specific class name, too.
But if this matcher is set, the modified constructor is built. If it is not, it is not.
And yes, the "intention" is, to match anything ending with "\StartContoller" so \Acme\Package\StartController but not \Acme\Package\SpecialStartController
It is quite "easy" to fix the problem by adjusting the matcher. But I guess the framework should not trigger this unexpected behavior nevertheless.
It happens with
privilegeTargets:
Neos\Flow\Security\Authorization\Privilege\Method\MethodPrivilege:
MyPrivilegeName:
matcher: "class(.*StartController)"
too.
That is why I suspecting the reduceTargetClassNames as guilty. Because it searches for expressions beginning with . (and some other chars) and returns the unmodified ClassNameIndex in such a case.
(thinking of it it comes to my mind, that it maybe should be possible to copy the original constructor signatur with all arguments being optional and not changing anything else, allowing to advice classes EVEN if they implements "bad" interfaces. But that does not mean, that I consider this behavior here as at least "unexpected")
OK, I can reproduce this:
With the privilege as described above an invalid proxy class for the StandardVariableProvider class is generated.
And, interestingly, proxies for 90 additional classes (in a 5.3 Neos app) - even though I don't have a single StartController.
The (simplified) proxy code that is generated: https://3v4l.org/qk9DT
So I think the bug is two fold:
- The matcher seems to be misinterpreted and matches too many classes
- We don't support proxying of classes that implement an interface with a constructor declaration
it maybe should be possible to copy the original constructor signatur with all arguments being optional and not changing anything else
Yes, that seems like a good idea
Related thread on discuss -> https://discuss.neos.io/t/flow-generates-faulty-proxy-classes/5592/11
@bwaidelich thanks for pointing me to this issue. Just to link it here, this issue is related to this one: https://github.com/neos/neos-development-collection/issues/3418 - though I just realize I obviuosly posted that one in the wrong repository...
https://github.com/neos/flow-development-collection/issues/2554 I moved the issue to the flow-development-collection and I agree with Bastian's assessment
PS: I would suggest to focus this issue on the "The matcher seems to be misinterpreted and matches too many classes" and the other one regarding proxying of constructors with arguments/interfaces
I tried to address the proxy building issue with #2555
Shall we convert this report to one covering the other issue (a pointcut expression of class(.*\\StartController) should not target the StandardVariableProvider class)?
Unfortunately the issue still remains, whenever using a pointcut expression like class(.*\\....) a proxy class is built for every class in the system (!)
I was able to track this down to PointcutClassNameFilter:: reduceTargetClassNames() This piece of code is more than 10 years old(!) and it was introduced in order to speed up proxy class building by skipping all classes that don't match the specified prefix.
I wonder why we can't filter the class names against the whole regex, but I'm sure there is a reason for that
@bwaidelich could it be that it'S as simple as \Neos\Flow\Aop\AspectContainer::reduceTargetClassNames to me it looks like it unions all classes that are affected by one of the methods in the aspect, so if just one doesn't restrict the classnames in the pointcut, that would mean all classes are affected, and the AOP ProxyBuilder then unions all class names of all AspectContainers, so I have the feeling if just one aspect has an unrestricted pointcut everything will be affected, BUT shouldn't Fluid as a third party package be excluded from proxy building (yes it shoudl but the fluid adaptor includes them in settings because I guess that was necessary at some point)
Right
a proxy class is built for every class in the system
was not precise. Only for classes for which object management is enabled (and yes, it seems for Fluid this is the case)
I have the feeling if just one aspect has an unrestricted pointcut everything will be affected
The thing is: It doesn't have to be class(.*) as in: "match all classes". But even if it is class(.*\\SomeClass), a proxy is built because the regex doesn't match => the class is not filtered
Unfortunately the issue still remains, whenever using a pointcut expression like
class(.*\\....)a proxy class is built for every class in the system (!)I was able to track this down to PointcutClassNameFilter:: reduceTargetClassNames() This piece of code is more than 10 years old(!) and it was introduced in order to speed up proxy class building by skipping all classes that don't match the specified prefix.
if (!preg_match('/^([^\.\(\)\{\}\[\]\?\+\$\!\|]+)/', $this->originalExpressionString, $matches)) {
😨 this regex is a little bit hellish and even after reading it twice I'm not sure what it's supposed to do. Maybe because it's a NOT matches something starting with NOT these, mostly unnecessarily escaped, characters. Am I right in that this check will just make any expression that doesn't start with a character, hence a Namespace/Classname, not filter anything out? Still, AFAIK this is only an optimizing pre-filtering done to reduce the amount of class names that the actual matches() method has to be run against. So even if that just passes along all classes, the matches() method should finally disregard all classes that do not match the regex .*\\Acme, so why isn't it?
https://3v4l.org/ARMDP#v7.4.30
As this bug just hit me too, I also found an older related bug report which I assume could be closed: #1011
In case someone googles the error, I received this one:
Cannot build proxy for class "TYPO3Fluid\Fluid\Core\Variables\ChainedVariableProvider" because it implements interface "TYPO3Fluid\Fluid\Core\Variables\VariableProviderInterface" that defines a constructor with arguments.
I took some time today trying to reproduce this behavior through tests, but was not successful yet. My current take is that the bug must lie in the optimization code (reduceTargetClassNames) as you already pointed out.
Whoever tries to fix this next: try to create a unit test containing the mentioned filter and target class and see it fail.
