8000 [Validator] Avoid triggering the autoloader for user-input values by Seldaek · Pull Request #40506 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Merged
merged 1 commit into from
Mar 23, 2021

Conversation

Seldaek
Copy link
Member
@Seldaek Seldaek commented Mar 18, 2021
Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

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:

  • if (\is_object($value)) {
    $this->validateObject(
    where non-objects are anyway ignored, so this change is harmless there.
  • // If the value is a scalar, pass it anyway, because we want
    // a NoSuchMetadataException to be thrown in that case
    $this->validateObject(
    $value,
    $propertyPath,
    $cascadedGroups,
    $traversalStrategy,
    $context
    );
    where it's explicitly passing anything in there to get the proper exception, so my change makes sure that exception is thrown before autoloading attempts. I am just not 100% sure if there are cases where validateGenericNode will receive a class name as a string to validate in $value. I can't imagine why it would but that doesn't mean it's true.

@nicolas-grekas nicolas-grekas changed the title Avoid triggering the autoloader for user-input values [Validator] Avoid triggering the autoloader for user-input values Mar 18, 2021
@@ -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)));
Copy link
Contributor

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)) {
Copy link
Contributor

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:

  1. register an autoloader
  2. verify if said autoloader is triggered with malformed input
  3. unregister autoloader

Copy link
Member Author

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.

Copy link
Contributor
@Ocramius Ocramius Mar 18, 2021

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 :trollface:

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.

Copy link
Contributor

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 :)

Copy link
Contributor

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 :)

Copy link
Member Author

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.

Copy link
Member

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)

@carsonbot carsonbot changed the title [Validator] Avoid triggering the autoloader for user-input values Avoid triggering the autoloader for user-input values Mar 18, 2021
@carsonbot carsonbot changed the title Avoid triggering the autoloader for user-input values [Validator] Avoid triggering the autoloader for user-input values Mar 18, 2021
Copy link
Member
@nicolas-grekas nicolas-grekas left a 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!

@nicolas-grekas
Copy link
Member

Thank you @Seldaek.

@nicolas-grekas nicolas-grekas merged commit 689056e into symfony:4.4 Mar 23, 2021
This was referenced Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
0