-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Avoid triggering the autoloader for user-input values #40506
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param object $object
, so this seems suspicious :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh too quick:
it's explicitly passing anything in there to get the proper exception
but still :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Valid
constraint should only validate objects if they exist as objects. If a user sends a string and that gets assigned to a property,Valid
should 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