8000 Merge branch '5.1' into 5.2 · symfony/symfony@cc130e1 · GitHub
[go: up one dir, main page]
Skip to content

Commit cc130e1

Browse files
xabbuhxabbuh
committed
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
2 parents 873b0b4 + 355f18d commit cc130e1

File tree

9 files changed

+209
-24
lines changed

9 files changed

+209
-24
lines changed

‎src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,14 @@ public function load(array $configs, ContainerBuilder $container)
533533

534534
$container->registerForAutoconfiguration(RouteLoaderInterface::class)
535535
->addTag('routing.route_loader');
536+
537+
$container->setParameter('container.behavior_describing_tags', [
538+
'container.service_locator',
539+
'container.service_subscriber',
540+
'kernel.event_subscriber',
541+
'kernel.locale_aware',
542+
'kernel.reset',
543+
]);
536544
}
537545

538546
/**

‎src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1622,6 +1622,20 @@ public function testHttpClientMockResponseFactory()
16221622
$this->assertSame('my_response_factory', (string) $argument);
16231623
}
16241624

1625+
public function testRegisterParameterCollectingBehaviorDescribingTags()
1626+
{
1627+
$container = $this->createContainerFromFile('default_config');
1628+
1629+
$this->assertTrue($container->hasParameter('container.behavior_describing_tags'));
1630+
$this->assertEquals([
1631+
'container.service_locator',
1632+
'container.service_subscriber',
1633+
'kernel.event_subscriber',
1634+
'kernel.locale_aware',
1635+
'kernel.reset',
1636+
], $container->getParameter('container.behavior_describing_tags'));
1637+
}
1638+
16251639
protected function createContainer(array $data = [])
16261640
{
16271641
return new ContainerBuilder(new EnvPlaceholderParameterBag(array_merge([

‎src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,22 @@ public function process(ContainerBuilder $container)
3535
}
3636
}
3737

38+
$tagsToKeep = [];
39+
40+
if ($container->hasParameter('container.behavior_describing_tags')) {
41+
$tagsToKeep = $container->getParameter('container.behavior_describing_tags');
42+
}
43+
3844
foreach ($container->getDefinitions() as $id => $definition) {
39-
$container->setDefinition($id, $this->processDefinition($container, $id, $definition));
45+
$container->setDefinition($id, $this->processDefinition($container, $id, $definition, $tagsToKeep));
46+
}
47+
48+
if ($container->hasParameter('container.behavior_describing_tags')) {
49+
$container->getParameterBag()->remove('container.behavior_describing_tags');
4050
}
4151
}
4252

43-
private function processDefinition(ContainerBuilder $container, string $id, Definition $definition): Definition
53+
private function processDefinition(ContainerBuilder $container, string $id, Definition $definition, array $tagsToKeep): Definition
4454
{
4555
$instanceofConditionals = $definition->getInstanceofConditionals();
4656
$autoconfiguredInstanceof = $definition->isAutoconfigured() ? $container->getAutoconfiguredInstanceof() : [];
@@ -114,10 +124,10 @@ private function processDefinition(ContainerBuilder $container, string $id, Defi
114124
}
115125

116126
// Don't add tags to service decorators
117-
if (null === $definition->getDecoratedService()) {
118-
$i = \count($instanceofTags);
119-
while (0 <= --$i) {
120-
foreach ($instanceofTags[$i] as $k => $v) {
127+
$i = \count($instanceofTags);
128+
while (0 <= --$i) {
129+
foreach ($instanceofTags[$i] as $k => $v) {
130+
if (null === $definition->getDecoratedService() || \in_array($k, $tagsToKeep, true)) {
121131
foreach ($v as $v) {
122132
if ($definition->hasTag($k) && \in_array($v, $definition->getTag($k))) {
123133
continue;

‎src/Symfony/Component/DependencyInjection/Tests/Compiler/ResolveInstanceofConditionalsPassTest.php

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,16 @@
1212
namespace Symfony\Component\DependencyInjection\Tests\Compiler;
1313

1414
use PHPUnit\Framework\TestCase;
15+
use Symfony\Component\Config\Resource\ResourceInterface;
16+
use Symfony\Component\Config\ResourceCheckerInterface;
1517
use Symfony\Component\DependencyInjection\Argument\BoundArgument;
1618
use Symfony\Component\DependencyInjection\ChildDefinition;
1719
use Symfony\Component\DependencyInjection\Compiler\ResolveChildDefinitionsPass;
1820
use Symfony\Component\DependencyInjection\Compiler\ResolveInstanceofConditionalsPass;
1921
use Symfony\Component\DependencyInjection\ContainerBuilder;
2022
use Symfony\Component\DependencyInjection\Reference;
23+
use Symfony\Contracts\Service\ResetInterface;
24+
use Symfony\Contracts\Service\ServiceSubscriberInterface;
2125

2226
class ResolveInstanceofConditionalsPassTest extends TestCase
2327
{
@@ -325,4 +329,60 @@ public function testDecoratorsAreNotAutomaticallyTagged()
325329

326330
$this->assertSame(['manual' => [[]]], $container->getDefinition('decorator')->getTags());
327331
}
332+
333+
public function testDecoratorsKeepBehaviorDescribingTags()
334+
{
335+
$container = new ContainerBuilder();
336+
337+
$container->setParameter('container.behavior_describing_tags', [
338+
'container.service_subscriber',
339+
'kernel.reset',
340+
]);
341+
342+
$container->register('decorator', DecoratorWithBehavior::class)
343+
->setAutoconfigured(true)
344+
->setDecoratedService('decorated')
345+
;
346+
347+
$container->registerForAutoconfiguration(ResourceCheckerInterface::class)
348+
->addTag('config_cache.resource_checker')
349+
;
350+
$container->registerForAutoconfiguration(ServiceSubscriberInterface::class)
351+
->addTag('container.service_subscriber')
352+
;
353+
$container->registerForAutoconfiguration(ResetInterface::class)
354+
->addTag('kernel.reset', ['method' => 'reset'])
355+
;
356+
357+
(new ResolveInstanceofConditionalsPass())->process($container);
358+
359+
$this->assertEquals([
360+
'container.service_subscriber' => [0 => []],
361+
'kernel.reset' => [
362+
[
363+
'method' => 'reset',
364+
],
365+
],
366+
], $container->getDefinition('decorator')->getTags());
367+
$this->assertFalse($container->hasParameter('container.behavior_describing_tags'));
368+
}
369+
}
370+
371+
class DecoratorWithBehavior implements ResetInterface, ResourceCheckerInterface, ServiceSubscriberInterface
372+
{
373+
public function reset()
374+
{
375+
}
376+
377+
public function supports(ResourceInterface $metadata)
378+
{
379+
}
380+
381+
public function isFresh(ResourceInterface $resource, $timestamp)
382+
{
383+
}
384+
385+
public static function getSubscribedServices()
386+
{
387+
}
328388
}

‎src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
class FormValidator extends ConstraintValidator
2626
{
2727
private $resolvedGroups;
28-
private $fieldFormConstraints;
2928

3029
/**
3130
* {@inheritdoc}
@@ -68,7 +67,6 @@ public function validate($form, Constraint $formConstraint)
6867

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

7472
if ($groups instanceof GroupSequence) {
@@ -93,7 +91,6 @@ public function validate($form, Constraint $formConstraint)
9391
$this->resolvedGroups[$field] = (array) $group;
9492
$fieldFormConstraint = new Form();
9593
$fieldFormConstraint->groups = $group;
96-
$this->fieldFormConstraints[] = $fieldFormConstraint;
9794
$this->context->setNode($this->context->getValue(), $field, $this->context->getMetadata(), $this->context->getPropertyPath());
9895
$validator->atPath(sprintf('children[%s]', $field->getName()))->validate($field, $fieldFormConstraint, $group);
9996
}
@@ -139,18 +136,15 @@ public function validate($form, Constraint $formConstraint)
139136
foreach ($form->all() as $field) {
140137
if ($field->isSubmitted()) {
141138
$this->resolvedGroups[$field] = $groups;
142-
$fieldFormConstraint = new Form();
143-
$this->fieldFormConstraints[] = $fieldFormConstraint;
144139
$this->context->setNode($this->context->getValue(), $field, $this->context->getMetadata(), $this->context->getPropertyPath());
145-
$validator->atPath(sprintf('children[%s]', $field->getName()))->validate($field, $fieldFormConstraint);
140+
$validator->atPath(sprintf('children[%s]', $field->getName()))->validate($field, $formConstraint);
146141
}
147142
}
148143
}
149144

150145
if ($hasChildren && $form->isRoot()) {
151146
// destroy storage to avoid memory leaks
152147
$this->resolvedGroups = new \SplObjectStorage();
153-
$this->fieldFormConstraints = [];
154148
}
155149
} elseif (!$form->isSynchronized()) {
156150
$childrenSynchronized = true;
@@ -159,11 +153,8 @@ public function validate($form, Constraint $formConstraint)
159153
foreach ($form as $child) {
160154
if (!$child->isSynchronized()) {
161155
$childrenSynchronized = false;
162-
163-
$fieldFormConstraint = new Form();
164-
$this->fieldFormConstraints[] = $fieldFormConstraint;
165156
$this->context->setNode($this->context->getValue(), $child, $this->context->getMetadata(), $this->context->getPropertyPath());
166-
$validator->atPath(sprintf('children[%s]', $child->getName()))->validate($child, $fieldFormConstraint);
157+ $validator->atPath(sprintf('children[%s]', $child->getName()))->validate($child, $formConstraint);
167158
}
168159
}
169160

‎src/Symfony/Component/Form/composer.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
},
3030
"require-dev": {
3131
"doctrine/collections": "~1.0",
32-
"symfony/validator": "^4.4.12|^5.1.6",
32+
"symfony/validator": "^4.4.17|^5.1.9",
3333
"symfony/dependency-injection": "^4.4|^5.0",
3434
"symfony/expression-language": "^4.4|^5.0",
3535
"symfony/config": "^4.4|^5.0",

‎src/Symfony/Component/Validator/Context/ExecutionContext.php

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ class ExecutionContext implements ExecutionContextInterface
128128
* @var array
129129
*/
130130
private $initializedObjects;
131+
private $cachedObjectsRefs;
131132

132133
/**
133134
* @param mixed $root The root value of the validated object graph
@@ -141,6 +142,7 @@ public function __construct(ValidatorInterface $validator, $root, TranslatorInte
141142
$this->translator = $translator;
142143
$this->translationDomain = $translationDomain;
143144
$this->violations = new ConstraintViolationList();
145+
$this->cachedObjectsRefs = new \SplObjectStorage();
144146
}
145147

146148
/**
@@ -346,4 +348,20 @@ public function isObjectInitialized(string $cacheKey): bool
346348
{
347349
return isset($this->initializedObjects[$cacheKey]);
348350
}
351+
352+
/**
353+
* @internal
354+
*
355+
* @param object $object
356+
*
357+
* @return string
358+
*/
359+
public function generateCacheKey($object)
360+
{
361+
if (!isset($this->cachedObjectsRefs[$object])) {
362+
$this->cachedObjectsRefs[$object] = spl_object_hash($object);
363+
}
364+
365+
return $this->cachedObjectsRefs[$object];
366+
}
349367
}

‎src/Symfony/Component/Validator/Tests/Validator/RecursiveValidatorTest.php

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,15 @@
1313

1414
use PHPUnit\Framework\TestCase;
1515
use Symfony\Component\Translation\IdentityTranslator;
16+
use Symfony\Component\Validator\Constraint;
1617
use Symfony\Component\Validator\Constraints\All;
1718
use Symfony\Component\Validator\Constraints\Callback;
1819
use Symfony\Component\Validator\Constraints\Cascade;
1920
use Symfony\Component\Validator\Constraints\Collection;
2021
use Symfony\Component\Validator\Constraints\Expression;
2122
use Symfony\Component\Validator\Constraints\GroupSequence;
23+
use Symfony\Component\Validator\Constraints\IsFalse;
24+
use Symfony\Component\Validator\Constraints\IsNull;
2225
use Symfony\Component\Validator\Constraints\IsTrue;
2326
use Symfony\Component\Validator\Constraints\Length;
2427
use Symfony\Component\Validator\Constraints\NotBlank;
@@ -28,6 +31,7 @@
2831
use Symfony\Component\Validator\Constraints\Traverse;
2932
use Symfony\Component\Validator\Constraints\Type;
3033
use Symfony\Component\Validator\Constraints\Valid;
34+
use Symfony\Component\Validator\ConstraintValidator;
3135
use Symfony\Component\Validator\ConstraintValidatorFactory;
3236
use Symfony\Component\Validator\ConstraintViolationInterface;
3337
use Symfony\Component\Validator\Context\ExecutionContextFactory;
@@ -2330,4 +2334,66 @@ public function testValidateWithExplicitCascade()
23302334

23312335
CascadingEntity::$staticChild = null;
23322336
}
2337+
2338+
public function testValidatedConstraintsHashesDoNotCollide()
2339+
{
2340+
$metadata = new ClassMetadata(Entity::class);
2341+
$metadata->addPropertyConstraint('initialized', new NotNull(['groups' => 'should_pass']));
2342+
$metadata->addPropertyConstraint('initialized', new IsNull(['groups' => 'should_fail']));
2343+
2344+
$this->metadataFactory->addMetadata($metadata);
2345+
2346+
$entity = new Entity();
2347+
$entity->data = new \stdClass();
2348+
2349+
$this->assertCount(2, $this->validator->validate($entity, new TestConstraintHashesDoNotCollide()));
2350+
}
2351+
2352+
public function testValidatedConstraintsHashesDoNotCollideWithSameConstraintValidatingDifferentProperties()
2353+
{
2354+
$value = new \stdClass();
2355+
2356+
$entity = new Entity();
2357+
$entity->firstName = $value;
2358+
$entity->setLastName($value);
2359+
2360+
$validator = $this->validator->startContext($entity);
2361+
2362+
$constraint = new IsNull();
2363+
$validator->atPath('firstName')
2364+
->validate($entity->firstName, $constraint);
2365+
$validator->atPath('lastName')
2366+
->validate($entity->getLastName(), $constraint);
2367+
2368+
$this->assertCount(2, $validator->getViolations());
2369+
}
2370+
}
2371+
2372+
final class TestConstraintHashesDoNotCollide extends Constraint
2373+
{
2374+
}
2375+
2376+
final class TestConstraintHashesDoNotCollideValidator extends ConstraintValidator
2377+
{
2378+
/**
2379+
* {@inheritdoc}
2380+
*/
2381+
public function validate($value, Constraint $constraint)
2382+
{
2383+
if (!$value instanceof Entity) {
2384+
throw new \LogicException();
2385+
}
2386+
2387+
$this->context->getValidator()
2388+
->inContext($this->context)
2389+
->atPath('data')
2390+
->validate($value, new NotNull())
2391+
->validate($value, new NotNull())
2392+
->validate($value, new IsFalse());
2393+
2394+
$this->context->getValidator()
2395+
->inContext($this->context)
2396+
->validate($value, null, new GroupSequence(['should_pass']))
2397+
->validate($value, null, new GroupSequence(['should_fail']));
2398+
}
23332399
}

0 commit comments

Comments
 (0)
0