Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| { | ||
| try { | ||
| if (!\is_object($object)) { | ||
| thrownewNoSuchMetadataException(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"
| privatefunctionvalidateObject($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 tomixed instead (we don't want that exception to become a TypeError due to adding a nativeobject typehint)
nicolas-grekas left a comment
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 thevalidateObject() 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!
nicolas-grekas commentedMar 23, 2021
Thank you@Seldaek. |
Following-up tohttps://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 inacb32dd
symfony/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
Lines 652 to 660 inacb32dd