Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PSR-6 cache issue with RecursiveContextualValidator.php #20685

Closed
ahungry opened this issue Nov 29, 2016 · 5 comments
Closed

PSR-6 cache issue with RecursiveContextualValidator.php #20685

ahungry opened this issue Nov 29, 2016 · 5 comments

Comments

@ahungry
Copy link

ahungry commented Nov 29, 2016

In the RecursiveContextualValidator.php file, it makes the class metadata map, with propertyName as a key for the caching.

This has a bad bug when a key (property name) is re-used between nested class instances, such that the value of the key is included in the cache name - however, if multiple entities / nested object instances have the same key/value pairs, this causes a collision and gives an error about the class metadata map expecting an object, but a boolean being returned.

The other portion of this bug is that the value used as part of the cache key is not escaped in anyway, and it is very likely and possible that the value has 'illegal' cache key characters (parenthesis etc.).

This is not extensively tested, but seems to fix for our use case (adding something similar to this after line 563 in Symfony/Component/Validator/Validator/RecursiveContextualValidator.php):

            // Need to escape this and ensure uniqueness.                                                                                                                                              
            $propertyName = base64_encode($propertyPath . '.' . $propertyName);

Edit: This is with Symfony 3.1.5+ that seems to have introduced the issue

@javiereguiluz javiereguiluz changed the title psr6 cache issue with Symfony/Component/Validator/Validator/RecursiveContextualValidator.php PSR-6 cache issue with RecursiveContextualValidator.php Nov 29, 2016
@ahungry
Copy link
Author

ahungry commented Nov 29, 2016

I think this is primarily an issue when @Assert\Valid is used on non-object properties (or mixed case ones that could be a non-object or an object).

This is a minimum testing file to illustrate the exception thrown (the test used here will not pass):

<?php

use Symfony\Component\Validator\Constraints as Assert;
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;

class Parent20685
{
    /**                                                                                                                                                                                                
     * @Assert\Valid                                                                                                                                                                                   
     */
    public $child20685A;

    /**                                                                                                                                                                                                
     * @Assert\Valid                                                                                                                                                                                   
     */
    public $child20685B;
}

class Child20685A
{
    /**                                                                                                                                                                                                
     * @Assert\Valid                                                                                                                                                                                   
     * @Assert\NotBlank                                                                                                                                                                                
     */
    public $name;
}

class Child20685B
{
    /**                                                                                                                                                                                                
     * @Assert\Valid                                                                                                                                                                                   
     * @Assert\NotBlank                                                                                                                                                                                
     */
    public $name;
}

class Validator206858Test extends WebTestCase
{
    public function test206858()
    {
        static::bootKernel();
	$container = static::$kernel->getContainer();
	$validator = $container->get('validator');

        $parent = new Parent20685();
	$childA = new Child20685A();
        $childB = new Child20685B();

        $childA->name = false;
        $childB->name = 'fake';

        $parent->child20685A = [$childA];
        $parent->child20685B = [$childB];

        $validator->validate($parent, null, ['Default']);

        $this->assertEquals(1, 1);
    }
}

With the change I posted in the initial post, this test will pass fine without an error.

The test also passes on sub 3.1.5 versions (3.0.* and below seem to have no problems with using @Assert\Valid on object properties that may or may not be objects themselves).

@ahungry
Copy link
Author

ahungry commented Nov 29, 2016

Also - in actual program usage, with debug set to true in the second AppKernel parameter, this does not result in an exception being thrown, but without (debug set to false) it does, which seems odd.

@nicolas-grekas
Copy link
Member

ping @dunglas I guess

Simperfit pushed a commit to Simperfit/symfony that referenced this issue Dec 3, 2016
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Dec 3, 2016
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Dec 3, 2016
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Dec 4, 2016
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Dec 4, 2016
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Dec 4, 2016
@dunglas
Copy link
Member

dunglas commented Dec 7, 2016

@ahungry can you confirm that #20745 fix your problem please?

@ahungry
Copy link
Author

ahungry commented Dec 8, 2016

Looks good, thanks!

nicolas-grekas added a commit that referenced this issue Dec 8, 2016
This PR was squashed before being merged into the 3.1 branch (closes #20745).

Discussion
----------

[Validator] add class name to the cache key

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |  #20685
| License       | MIT
| Doc PR        |

Adding the class name to the cache key to avoid collision

Commits
-------

1681fc9 [Validator] add class name to the cache key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants