Merge branch '5.1' into 5.2

* 5.1: prevent hash collisions caused by reused object hashes autoconfigure behavior describing tags on decorators [Validator][RecursiveContextualValidator] Prevent validated hash collisions
2020-11-13 10:44:33 +01:00 · 2020-11-13 10:44:33 +01:00 · cc130e1d9c
commit cc130e1d9c
parent 873b0b4f53 355f18d34d
9 changed files with 209 additions and 24 deletions
--- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
+++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@ -533,6 +533,14 @@ class FrameworkExtension extends Extension

        $container->registerForAutoconfiguration(RouteLoaderInterface::class)
            ->addTag('routing.route_loader');
+
+        $container->setParameter('container.behavior_describing_tags', [
+            'container.service_locator',
+            'container.service_subscriber',
+            'kernel.event_subscriber',
+            'kernel.locale_aware',
+            'kernel.reset',
+        ]);
    }

    /**
--- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
+++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
@ -1622,6 +1622,20 @@ abstract class FrameworkExtensionTest extends TestCase
        $this->assertSame('my_response_factory', (string) $argument);
    }

+    public function testRegisterParameterCollectingBehaviorDescribingTags()
+    {
+        $container = $this->createContainerFromFile('default_config');
+
+        $this->assertTrue($container->hasParameter('container.behavior_describing_tags'));
+        $this->assertEquals([
+            'container.service_locator',
+            'container.service_subscriber',
+            'kernel.event_subscriber',
+            'kernel.locale_aware',
+            'kernel.reset',
+        ], $container->getParameter('container.behavior_describing_tags'));
+    }
+
    protected function createContainer(array $data = [])
    {
        return new ContainerBuilder(new EnvPlaceholderParameterBag(array_merge([
--- a/src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php
+++ b/src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php
@ -35,12 +35,22 @@ class ResolveInstanceofConditionalsPass implements CompilerPassInterface
            }
        }

+        $tagsToKeep = [];
+
+        if ($container->hasParameter('container.behavior_describing_tags')) {
+            $tagsToKeep = $container->getParameter('container.behavior_describing_tags');
+        }
+
        foreach ($container->getDefinitions() as $id => $definition) {
-            $container->setDefinition($id, $this->processDefinition($container, $id, $definition));
+            $container->setDefinition($id, $this->processDefinition($container, $id, $definition, $tagsToKeep));
+        }
+
+        if ($container->hasParameter('container.behavior_describing_tags')) {
+            $container->getParameterBag()->remove('container.behavior_describing_tags');
        }
    }

-    private function processDefinition(ContainerBuilder $container, string $id, Definition $definition): Definition
+    private function processDefinition(ContainerBuilder $container, string $id, Definition $definition, array $tagsToKeep): Definition
    {
        $instanceofConditionals = $definition->getInstanceofConditionals();
        $autoconfiguredInstanceof = $definition->isAutoconfigured() ? $container->getAutoconfiguredInstanceof() : [];
@ -114,10 +124,10 @@ class ResolveInstanceofConditionalsPass implements CompilerPassInterface
            }

            // Don't add tags to service decorators
-            if (null === $definition->getDecoratedService()) {
-                $i = \count($instanceofTags);
-                while (0 <= --$i) {
-                    foreach ($instanceofTags[$i] as $k => $v) {
+            $i = \count($instanceofTags);
+            while (0 <= --$i) {
+                foreach ($instanceofTags[$i] as $k => $v) {
+                    if (null === $definition->getDecoratedService() || \in_array($k, $tagsToKeep, true)) {
                        foreach ($v as $v) {
                            if ($definition->hasTag($k) && \in_array($v, $definition->getTag($k))) {
                                continue;
--- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveInstanceofConditionalsPassTest.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveInstanceofConditionalsPassTest.php
@ -12,12 +12,16 @@
 namespace Symfony\Component\DependencyInjection\Tests\Compiler;

 use PHPUnit\Framework\TestCase;
+use Symfony\Component\Config\Resource\ResourceInterface;
+use Symfony\Component\Config\ResourceCheckerInterface;
 use Symfony\Component\DependencyInjection\Argument\BoundArgument;
 use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\Compiler\ResolveChildDefinitionsPass;
 use Symfony\Component\DependencyInjection\Compiler\ResolveInstanceofConditionalsPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Contracts\Service\ResetInterface;
+use Symfony\Contracts\Service\ServiceSubscriberInterface;

 class ResolveInstanceofConditionalsPassTest extends TestCase
 {
@ -325,4 +329,60 @@ class ResolveInstanceofConditionalsPassTest extends TestCase

        $this->assertSame(['manual' => [[]]], $container->getDefinition('decorator')->getTags());
    }
+
+    public function testDecoratorsKeepBehaviorDescribingTags()
+    {
+        $container = new ContainerBuilder();
+
+        $container->setParameter('container.behavior_describing_tags', [
+            'container.service_subscriber',
+            'kernel.reset',
+        ]);
+
+        $container->register('decorator', DecoratorWithBehavior::class)
+            ->setAutoconfigured(true)
+            ->setDecoratedService('decorated')
+        ;
+
+        $container->registerForAutoconfiguration(ResourceCheckerInterface::class)
+            ->addTag('config_cache.resource_checker')
+        ;
+        $container->registerForAutoconfiguration(ServiceSubscriberInterface::class)
+            ->addTag('container.service_subscriber')
+        ;
+        $container->registerForAutoconfiguration(ResetInterface::class)
+            ->addTag('kernel.reset', ['method' => 'reset'])
+        ;
+
+        (new ResolveInstanceofConditionalsPass())->process($container);
+
+        $this->assertEquals([
+            'container.service_subscriber' => [0 => []],
+            'kernel.reset' => [
+                [
+                    'method' => 'reset',
+                ],
+            ],
+        ], $container->getDefinition('decorator')->getTags());
+        $this->assertFalse($container->hasParameter('container.behavior_describing_tags'));
+    }
+}
+
+class DecoratorWithBehavior implements ResetInterface, ResourceCheckerInterface, ServiceSubscriberInterface
+{
+    public function reset()
+    {
+    }
+
+    public function supports(ResourceInterface $metadata)
+    {
+    }
+
+    public function isFresh(ResourceInterface $resource, $timestamp)
+    {
+    }
+
+    public static function getSubscribedServices()
+    {
+    }
 }
--- a/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
+++ b/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
@ -25,7 +25,6 @@ use Symfony\Component\Validator\Exception\UnexpectedTypeException;
 class FormValidator extends ConstraintValidator
 {
    private $resolvedGroups;
-    private $fieldFormConstraints;

    /**
     * {@inheritdoc}
@ -68,7 +67,6 @@ class FormValidator extends ConstraintValidator

            if ($hasChildren && $form->isRoot()) {
                $this->resolvedGroups = new \SplObjectStorage();
-                $this->fieldFormConstraints = [];
            }

            if ($groups instanceof GroupSequence) {
@ -93,7 +91,6 @@ class FormValidator extends ConstraintValidator
                            $this->resolvedGroups[$field] = (array) $group;
                            $fieldFormConstraint = new Form();
                            $fieldFormConstraint->groups = $group;
-                            $this->fieldFormConstraints[] = $fieldFormConstraint;
                            $this->context->setNode($this->context->getValue(), $field, $this->context->getMetadata(), $this->context->getPropertyPath());
                            $validator->atPath(sprintf('children[%s]', $field->getName()))->validate($field, $fieldFormConstraint, $group);
                        }
@ -139,10 +136,8 @@ class FormValidator extends ConstraintValidator
                foreach ($form->all() as $field) {
                    if ($field->isSubmitted()) {
                        $this->resolvedGroups[$field] = $groups;
-                        $fieldFormConstraint = new Form();
-                        $this->fieldFormConstraints[] = $fieldFormConstraint;
                        $this->context->setNode($this->context->getValue(), $field, $this->context->getMetadata(), $this->context->getPropertyPath());
-                        $validator->atPath(sprintf('children[%s]', $field->getName()))->validate($field, $fieldFormConstraint);
+                        $validator->atPath(sprintf('children[%s]', $field->getName()))->validate($field, $formConstraint);
                    }
                }
            }
@ -150,7 +145,6 @@ class FormValidator extends ConstraintValidator
            if ($hasChildren && $form->isRoot()) {
                // destroy storage to avoid memory leaks
                $this->resolvedGroups = new \SplObjectStorage();
-                $this->fieldFormConstraints = [];
            }
        } elseif (!$form->isSynchronized()) {
            $childrenSynchronized = true;
@ -159,11 +153,8 @@ class FormValidator extends ConstraintValidator
            foreach ($form as $child) {
                if (!$child->isSynchronized()) {
                    $childrenSynchronized = false;
-
-                    $fieldFormConstraint = new Form();
-                    $this->fieldFormConstraints[] = $fieldFormConstraint;
                    $this->context->setNode($this->context->getValue(), $child, $this->context->getMetadata(), $this->context->getPropertyPath());
-                    $validator->atPath(sprintf('children[%s]', $child->getName()))->validate($child, $fieldFormConstraint);
+                    $validator->atPath(sprintf('children[%s]', $child->getName()))->validate($child, $formConstraint);
                }
            }

--- a/src/Symfony/Component/Form/composer.json
+++ b/src/Symfony/Component/Form/composer.json
@ -29,7 +29,7 @@
    },
    "require-dev": {
        "doctrine/collections": "~1.0",
-        "symfony/validator": "^4.4.12|^5.1.6",
+        "symfony/validator": "^4.4.17|^5.1.9",
        "symfony/dependency-injection": "^4.4|^5.0",
        "symfony/expression-language": "^4.4|^5.0",
        "symfony/config": "^4.4|^5.0",
--- a/src/Symfony/Component/Validator/Context/ExecutionContext.php
+++ b/src/Symfony/Component/Validator/Context/ExecutionContext.php
@ -128,6 +128,7 @@ class ExecutionContext implements ExecutionContextInterface
     * @var array
     */
    private $initializedObjects;
+    private $cachedObjectsRefs;

    /**
     * @param mixed $root The root value of the validated object graph
@ -141,6 +142,7 @@ class ExecutionContext implements ExecutionContextInterface
        $this->translator = $translator;
        $this->translationDomain = $translationDomain;
        $this->violations = new ConstraintViolationList();
+        $this->cachedObjectsRefs = new \SplObjectStorage();
    }

    /**
@ -346,4 +348,20 @@ class ExecutionContext implements ExecutionContextInterface
    {
        return isset($this->initializedObjects[$cacheKey]);
    }
+
+    /**
+     * @internal
+     *
+     * @param object $object
+     *
+     * @return string
+     */
+    public function generateCacheKey($object)
+    {
+        if (!isset($this->cachedObjectsRefs[$object])) {
+            $this->cachedObjectsRefs[$object] = spl_object_hash($object);
+        }
+
+        return $this->cachedObjectsRefs[$object];
+    }
 }
--- a/src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php
+++ b/src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php
@ -13,12 +13,15 @@ namespace Symfony\Component\Validator\Tests\Validator;

 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Translation\IdentityTranslator;
+use Symfony\Component\Validator\Constraint;
 use Symfony\Component\Validator\Constraints\All;
 use Symfony\Component\Validator\Constraints\Callback;
 use Symfony\Component\Validator\Constraints\Cascade;
 use Symfony\Component\Validator\Constraints\Collection;
 use Symfony\Component\Validator\Constraints\Expression;
 use Symfony\Component\Validator\Constraints\GroupSequence;
+use Symfony\Component\Validator\Constraints\IsFalse;
+use Symfony\Component\Validator\Constraints\IsNull;
 use Symfony\Component\Validator\Constraints\IsTrue;
 use Symfony\Component\Validator\Constraints\Length;
 use Symfony\Component\Validator\Constraints\NotBlank;
@ -28,6 +31,7 @@ use Symfony\Component\Validator\Constraints\Required;
 use Symfony\Component\Validator\Constraints\Traverse;
 use Symfony\Component\Validator\Constraints\Type;
 use Symfony\Component\Validator\Constraints\Valid;
+use Symfony\Component\Validator\ConstraintValidator;
 use Symfony\Component\Validator\ConstraintValidatorFactory;
 use Symfony\Component\Validator\ConstraintViolationInterface;
 use Symfony\Component\Validator\Context\ExecutionContextFactory;
@ -2330,4 +2334,66 @@ class RecursiveValidatorTest extends TestCase

        CascadingEntity::$staticChild = null;
    }
+
+    public function testValidatedConstraintsHashesDoNotCollide()
+    {
+        $metadata = new ClassMetadata(Entity::class);
+        $metadata->addPropertyConstraint('initialized', new NotNull(['groups' => 'should_pass']));
+        $metadata->addPropertyConstraint('initialized', new IsNull(['groups' => 'should_fail']));
+
+        $this->metadataFactory->addMetadata($metadata);
+
+        $entity = new Entity();
+        $entity->data = new \stdClass();
+
+        $this->assertCount(2, $this->validator->validate($entity, new TestConstraintHashesDoNotCollide()));
+    }
+
+    public function testValidatedConstraintsHashesDoNotCollideWithSameConstraintValidatingDifferentProperties()
+    {
+        $value = new \stdClass();
+
+        $entity = new Entity();
+        $entity->firstName = $value;
+        $entity->setLastName($value);
+
+        $validator = $this->validator->startContext($entity);
+
+        $constraint = new IsNull();
+        $validator->atPath('firstName')
+            ->validate($entity->firstName, $constraint);
+        $validator->atPath('lastName')
+            ->validate($entity->getLastName(), $constraint);
+
+        $this->assertCount(2, $validator->getViolations());
+    }
+}
+
+final class TestConstraintHashesDoNotCollide extends Constraint
+{
+}
+
+final class TestConstraintHashesDoNotCollideValidator extends ConstraintValidator
+{
+    /**
+     * {@inheritdoc}
+     */
+    public function validate($value, Constraint $constraint)
+    {
+        if (!$value instanceof Entity) {
+            throw new \LogicException();
+        }
+
+        $this->context->getValidator()
+            ->inContext($this->context)
+            ->atPath('data')
+            ->validate($value, new NotNull())
+            ->validate($value, new NotNull())
+            ->validate($value, new IsFalse());
+
+        $this->context->getValidator()
+            ->inContext($this->context)
+            ->validate($value, null, new GroupSequence(['should_pass']))
+            ->validate($value, null, new GroupSequence(['should_fail']));
+    }
 }
--- a/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
+++ b/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
@ -108,7 +108,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface
            $this->validateGenericNode(
                $value,
                $previousObject,
-                \is_object($value) ? spl_object_hash($value) : null,
+                \is_object($value) ? $this->generateCacheKey($value) : null,
                $metadata,
                $this->defaultPropertyPath,
                $groups,
@ -176,7 +176,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface

        $propertyMetadatas = $classMetadata->getPropertyMetadata($propertyName);
        $groups = $groups ? $this->normalizeGroups($groups) : $this->defaultGroups;
-        $cacheKey = spl_object_hash($object);
+        $cacheKey = $this->generateCacheKey($object);
        $propertyPath = PropertyPath::append($this->defaultPropertyPath, $propertyName);

        $previousValue = $this->context->getValue();
@ -224,7 +224,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface
        if (\is_object($objectOrClass)) {
            $object = $objectOrClass;
            $class = \get_class($object);
-            $cacheKey = spl_object_hash($objectOrClass);
+            $cacheKey = $this->generateCacheKey($objectOrClass);
            $propertyPath = PropertyPath::append($this->defaultPropertyPath, $propertyName);
        } else {
            // $objectOrClass contains a class name
@ -311,7 +311,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface

            $this->validateClassNode(
                $object,
-                spl_object_hash($object),
+                $this->generateCacheKey($object),
                $classMetadata,
                $propertyPath,
                $groups,
@ -425,7 +425,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface
            $defaultOverridden = false;

            // Use the object hash for group sequences
-            $groupHash = \is_object($group) ? spl_object_hash($group) : $group;
+            $groupHash = \is_object($group) ? $this->generateCacheKey($group, true) : $group;

            if ($context->isGroupValidated($cacheKey, $groupHash)) {
                // Skip this group when validating the properties and when
@ -730,7 +730,7 @@ class RecursiveContextualValidator implements ContextualValidatorInterface
            // Prevent duplicate validation of constraints, in the case
            // that constraints belong to multiple validated groups
            if (null !== $cacheKey) {
-                $constraintHash = spl_object_hash($constraint);
+                $constraintHash = $this->generateCacheKey($constraint, true);
                // instanceof Valid: In case of using a Valid constraint with many groups
                // it makes a reference object get validated by each group
                if ($constraint instanceof Composite || $constraint instanceof Valid) {
@ -762,4 +762,22 @@ class RecursiveContextualValidator implements ContextualValidatorInterface
            }
        }
    }
+
+    /**
+     * @param object $object
+     */
+    private function generateCacheKey($object, bool $dependsOnPropertyPath = false): string
+    {
+        if ($this->context instanceof ExecutionContext) {
+            $cacheKey = $this->context->generateCacheKey($object);
+        } else {
+            $cacheKey = spl_object_hash($object);
+        }
+
+        if ($dependsOnPropertyPath) {
+            $cacheKey .= $this->context->getPropertyPath();
+        }
+
+        return $cacheKey;
+    }
 }