[Validator] Avoid triggering the autoloader for user-input values #40506
Conversation
src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
Outdated
Show resolved
Hide resolved
| @@ -305,6 +305,10 @@ protected function normalizeGroups($groups) | |||
| private function validateObject($object, string $propertyPath, array $groups, int $traversalStrategy, ExecutionContextInterface $context) | |||
| { | |||
| try { | |||
| if (!\is_object($object)) { | |||
| throw new NoSuchMetadataException(sprintf('Cannot create metadata for non-objects. Got: "%s".', \gettype($object))); | |||
There was a problem hiding this comment.
TBF, the validation layer should not throw here, but reject the payload with a validation error, but I suppose "one thing at a time"
| @@ -305,6 +305,10 @@ protected function normalizeGroups($groups) | |||
| private function validateObject($object, string $propertyPath, array $groups, int $traversalStrategy, ExecutionContextInterface $context) | |||
| { | |||
| try { | |||
| if (!\is_object($object)) { | |||
There was a problem hiding this comment.
This execution path is missing a test case.
Test case would probably:
- register an autoloader
- verify if said autoloader is triggered with malformed input
- unregister autoloader
There was a problem hiding this comment.
Yeah sorry but that goes beyond the time I'm willing to invest in this :D If someone wants to add a test please be my guest.
There was a problem hiding this comment.
That's understandable, it's not like these kind of issues appeared/regressed because of a lack of testing or such 
EDIT: to be clear, I fully understand/agree that time constraints are what they are. I just wouldn't merge without proper testing, not asking for random contributors to put more effort in it. Somebody else will pick it up, perhaps me, if I get to it.
There was a problem hiding this comment.
@param object $object, so this seems suspicious :)
There was a problem hiding this comment.
ohh too quick:
it's explicitly passing anything i 8000 n there to get the proper exception
but still :)
There was a problem hiding this comment.
Yes and no.. if you do expect an object and you have no type hint, validing that you did indeed get an object and throwing otherwise doesn't seem that unreasonable.
There was a problem hiding this comment.
but as we explicitly use that private method with non-objects to get the error message it generates, I would change the phpdoc to mixed instead (we don't want that exception to become a TypeError due to adding a native object typehint)
4ab0655 to
e45eb23
Compare
There was a problem hiding this comment.
I moved the check outside of the validateObject() method, it doesn't need to be inside of it.
I'm now merging even if a test case could be added.
@Ocramius thanks for the review. We'd be happy to merge a PR of yours to improve this even further!
|
Thank you @Seldaek. |
Following-up to https://twitter.com/seldaek/status/1372450636361502721 - mostly to see if the build passes or if this breaks some undocumented/unclear-to-me assumptions.
Essentially using the
Validconstraint should only validate objects if they exist as objects. If a user sends a string and that gets assigned to a property,Validshould not attempt autoloading that user-given string.As far as I can tell, this is used in two places:
symfony/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
Lines 364 to 365 in acb32dd
symfony/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
Lines 652 to 660 in acb32dd