From 900d034003875d16aa7b5a293be82a9fd932c681 Mon Sep 17 00:00:00 2001 From: Mathias Arlaud Date: Wed, 29 Nov 2023 16:51:19 +0100 Subject: [PATCH] [Serializer] Fix unexpected allowed attributes --- .../Resources/config/serializer.php | 2 + .../Normalizer/AbstractObjectNormalizer.php | 4 + .../Normalizer/GetSetMethodNormalizer.php | 86 ++++++++++++++++--- .../Normalizer/ObjectNormalizer.php | 45 +++++++++- .../Normalizer/GetSetMethodNormalizerTest.php | 36 ++++++++ .../Tests/Normalizer/ObjectNormalizerTest.php | 36 ++++++++ 6 files changed, 194 insertions(+), 15 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php b/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php index ca0cf9b53612..63964f34f559 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php +++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php @@ -137,6 +137,8 @@ service('property_info')->ignoreOnInvalid(), service('serializer.mapping.class_discriminator_resolver')->ignoreOnInvalid(), null, + [], + service('property_info')->ignoreOnInvalid(), ]) ->alias(PropertyNormalizer::class, 'serializer.normalizer.property') diff --git a/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php b/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php index 4b03fa9ddb11..d5d34e8a7525 100644 --- a/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php +++ b/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php @@ -147,6 +147,8 @@ public function supportsNormalization($data, ?string $format = null) */ public function normalize($object, ?string $format = null, array $context = []) { + $context['_read_attributes'] = true; + if (!isset($context['cache_key'])) { $context['cache_key'] = $this->getCacheKey($format, $context); } @@ -359,6 +361,8 @@ public function supportsDenormalization($data, string $type, ?string $format = n */ public function denormalize($data, string $type, ?string $format = null, array $context = []) { + $context['_read_attributes'] = false; + if (!isset($context['cache_key'])) { $context['cache_key'] = $this->getCacheKey($format, $context); } diff --git a/src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php b/src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php index 8d749b4e1d48..9aaac706f213 100644 --- a/src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php +++ b/src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php @@ -36,6 +36,7 @@ */ class GetSetMethodNormalizer extends AbstractObjectNormalizer { + private static $reflectionCache = []; private static $setterAccessibleCache = []; /** @@ -43,7 +44,7 @@ class GetSetMethodNormalizer extends AbstractObjectNormalizer */ public function supportsNormalization($data, ?string $format = null) { - return parent::supportsNormalization($data, $format) && $this->supports(\get_class($data)); + return parent::supportsNormalization($data, $format) && $this->supports(\get_class($data), true); } /** @@ -51,7 +52,7 @@ public function supportsNormalization($data, ?string $format = null) */ public function supportsDenormalization($data, string $type, ?string $format = null) { - return parent::supportsDenormalization($data, $type, $format) && $this->supports($type); + return parent::supportsDenormalization($data, $type, $format) && $this->supports($type, false); } /** @@ -63,18 +64,22 @@ public function hasCacheableSupportsMethod(): bool } /** - * Checks if the given class has any getter method. + * Checks if the given class has any getter or setter method. */ - private function supports(string $class): bool + private function supports(string $class, bool $readAttributes): bool { if (null !== $this->classDiscriminatorResolver && $this->classDiscriminatorResolver->getMappingForClass($class)) { return true; } - $class = new \ReflectionClass($class); - $methods = $class->getMethods(\ReflectionMethod::IS_PUBLIC); - foreach ($methods as $method) { - if ($this->isGetMethod($method)) { + if (!isset(self::$reflectionCache[$class])) { + self::$reflectionCache[$class] = new \ReflectionClass($class); + } + + $reflection = self::$reflectionCache[$class]; + + foreach ($reflection->getMethods(\ReflectionMethod::IS_PUBLIC) as $reflectionMethod) { + if ($readAttributes ? $this->isGetMethod($reflectionMethod) : $this->isSetMethod($reflectionMethod)) { return true; } } @@ -95,6 +100,17 @@ private function isGetMethod(\ReflectionMethod $method): bool ); } + /** + * Checks if a method's name matches /^set.+$/ and can be called non-statically with one parameter. + */ + private function isSetMethod(\ReflectionMethod $method): bool + { + return !$method->isStatic() + && (\PHP_VERSION_ID < 80000 || !$method->getAttributes(Ignore::class)) + && 1 === $method->getNumberOfRequiredParameters() + && str_starts_with($method->name, 'set'); + } + /** * {@inheritdoc} */ @@ -124,19 +140,17 @@ protected function extractAttributes(object $object, ?string $format = null, arr */ protected function getAttributeValue(object $object, string $attribute, ?string $format = null, array $context = []) { - $ucfirsted = ucfirst($attribute); - - $getter = 'get'.$ucfirsted; + $getter = 'get'.$attribute; if (method_exists($object, $getter) && \is_callable([$object, $getter])) { return $object->$getter(); } - $isser = 'is'.$ucfirsted; + $isser = 'is'.$attribute; if (method_exists($object, $isser) && \is_callable([$object, $isser])) { return $object->$isser(); } - $haser = 'has'.$ucfirsted; + $haser = 'has'.$attribute; if (method_exists($object, $haser) && \is_callable([$object, $haser])) { return $object->$haser(); } @@ -149,7 +163,7 @@ protected function getAttributeValue(object $object, string $attribute, ?string */ protected function setAttributeValue(object $object, string $attribute, $value, ?string $format = null, array $context = []) { - $setter = 'set'.ucfirst($attribute); + $setter = 'set'.$attribute; $key = \get_class($object).':'.$setter; if (!isset(self::$setterAccessibleCache[$key])) { @@ -160,4 +174,48 @@ protected function setAttributeValue(object $object, string $attribute, $value, $object->$setter($value); } } + + protected function isAllowedAttribute($classOrObject, string $attribute, ?string $format = null, array $context = []) + { + if (!parent::isAllowedAttribute($classOrObject, $attribute, $format, $context)) { + return false; + } + + $class = \is_object($classOrObject) ? \get_class($classOrObject) : $classOrObject; + + if (!isset(self::$reflectionCache[$class])) { + self::$reflectionCache[$class] = new \ReflectionClass($class); + } + + $reflection = self::$reflectionCache[$class]; + + if ($context['_read_attributes'] ?? true) { + foreach (['get', 'is', 'has'] as $getterPrefix) { + $getter = $getterPrefix.$attribute; + $reflectionMethod = $reflection->hasMethod($getter) ? $reflection->getMethod($getter) : null; + if ($reflectionMethod && $this->isGetMethod($reflectionMethod)) { + return true; + } + } + + return false; + } + + $setter = 'set'.$attribute; + if ($reflection->hasMethod($setter) && $this->isSetMethod($reflection->getMethod($setter))) { + return true; + } + + $constructor = $reflection->getConstructor(); + + if ($constructor && $constructor->isPublic()) { + foreach ($constructor->getParameters() as $parameter) { + if ($parameter->getName() === $attribute) { + return true; + } + } + } + + return false; + } } diff --git a/src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php b/src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php index 0dafaa7b7bd5..434b53a87f1d 100644 --- a/src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php +++ b/src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php @@ -14,7 +14,11 @@ use Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException; use Symfony\Component\PropertyAccess\PropertyAccess; use Symfony\Component\PropertyAccess\PropertyAccessorInterface; +use Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor; +use Symfony\Component\PropertyInfo\PropertyInfoExtractorInterface; use Symfony\Component\PropertyInfo\PropertyTypeExtractorInterface; +use Symfony\Component\PropertyInfo\PropertyWriteInfo; +use Symfony\Component\Serializer\Annotation\Ignore; use Symfony\Component\Serializer\Exception\LogicException; use Symfony\Component\Serializer\Mapping\AttributeMetadata; use Symfony\Component\Serializer\Mapping\ClassDiscriminatorResolverInterface; @@ -28,11 +32,14 @@ */ class ObjectNormalizer extends AbstractObjectNormalizer { + private static $reflectionCache = []; + protected $propertyAccessor; + protected $propertyInfoExtractor; private $objectClassResolver; - public function __construct(?ClassMetadataFactoryInterface $classMetadataFactory = null, ?NameConverterInterface $nameConverter = null, ?PropertyAccessorInterface $propertyAccessor = null, ?PropertyTypeExtractorInterface $propertyTypeExtractor = null, ?ClassDiscriminatorResolverInterface $classDiscriminatorResolver = null, ?callable $objectClassResolver = null, array $defaultContext = []) + public function __construct(?ClassMetadataFactoryInterface $classMetadataFactory = null, ?NameConverterInterface $nameConverter = null, ?PropertyAccessorInterface $propertyAccessor = null, ?PropertyTypeExtractorInterface $propertyTypeExtractor = null, ?ClassDiscriminatorResolverInterface $classDiscriminatorResolver = null, ?callable $objectClassResolver = null, array $defaultContext = [], ?PropertyInfoExtractorInterface $propertyInfoExtractor = null) { if (!class_exists(PropertyAccess::class)) { throw new LogicException('The ObjectNormalizer class requires the "PropertyAccess" component. Install "symfony/property-access" to use it.'); @@ -45,6 +52,8 @@ public function __construct(?ClassMetadataFactoryInterface $classMetadataFactory $this->objectClassResolver = $objectClassResolver ?? function ($class) { return \is_object($class) ? \get_class($class) : $class; }; + + $this->propertyInfoExtractor = $propertyInfoExtractor ?: new ReflectionExtractor(); } /** @@ -182,4 +191,38 @@ protected function getAllowedAttributes($classOrObject, array $context, bool $at return $allowedAttributes; } + + protected function isAllowedAttribute($classOrObject, string $attribute, ?string $format = null, array $context = []) + { + if (!parent::isAllowedAttribute($classOrObject, $attribute, $format, $context)) { + return false; + } + $class = \is_object($classOrObject) ? \get_class($classOrObject) : $classOrObject; + + if ($context['_read_attributes'] ?? true) { + return $this->propertyInfoExtractor->isReadable($class, $attribute) || $this->hasAttributeAccessorMethod($class, $attribute); + } + + return $this->propertyInfoExtractor->isWritable($class, $attribute) + || ($writeInfo = $this->propertyInfoExtractor->getWriteInfo($class, $attribute)) && PropertyWriteInfo::TYPE_NONE !== $writeInfo->getType(); + } + + private function hasAttributeAccessorMethod(string $class, string $attribute): bool + { + if (!isset(self::$reflectionCache[$class])) { + self::$reflectionCache[$class] = new \ReflectionClass($class); + } + + $reflection = self::$reflectionCache[$class]; + + if (!$reflection->hasMethod($attribute)) { + return false; + } + + $method = $reflection->getMethod($attribute); + + return !$method->isStatic() + && (\PHP_VERSION_ID < 80000 || !$method->getAttributes(Ignore::class)) + && !$method->getNumberOfRequiredParameters(); + } } diff --git a/src/Symfony/Component/Serializer/Tests/Normalizer/GetSetMethodNormalizerTest.php b/src/Symfony/Component/Serializer/Tests/Normalizer/GetSetMethodNormalizerTest.php index f6b2c3e69ed9..e7c23cf58a57 100644 --- a/src/Symfony/Component/Serializer/Tests/Normalizer/GetSetMethodNormalizerTest.php +++ b/src/Symfony/Component/Serializer/Tests/Normalizer/GetSetMethodNormalizerTest.php @@ -521,6 +521,23 @@ public function testDenormalizeWithDiscriminator() $this->assertEquals($denormalized, $normalizer->denormalize(['type' => 'two', 'url' => 'url'], GetSetMethodDummyInterface::class)); } + + public function testSupportsAndNormalizeWithOnlyParentGetter() + { + $obj = new GetSetDummyChild(); + $obj->setFoo('foo'); + + $this->assertTrue($this->normalizer->supportsNormalization($obj)); + $this->assertSame(['foo' => 'foo'], $this->normalizer->normalize($obj)); + } + + public function testSupportsAndDenormalizeWithOnlyParentSetter() + { + $this->assertTrue($this->normalizer->supportsDenormalization(['foo' => 'foo'], GetSetDummyChild::class)); + + $obj = $this->normalizer->denormalize(['foo' => 'foo'], GetSetDummyChild::class); + $this->assertSame('foo', $obj->getFoo()); + } } class GetSetDummy @@ -825,3 +842,22 @@ public function setUrl(string $url): void $this->url = $url; } } + +class GetSetDummyChild extends GetSetDummyParent +{ +} + +class GetSetDummyParent +{ + private $foo; + + public function getFoo() + { + return $this->foo; + } + + public function setFoo($foo) + { + $this->foo = $foo; + } +} diff --git a/src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php b/src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php index 6bc99f913285..eff523b367f9 100644 --- a/src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php +++ b/src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php @@ -18,6 +18,7 @@ use Symfony\Component\PropertyInfo\Extractor\PhpStanExtractor; use Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor; use Symfony\Component\PropertyInfo\PropertyInfoExtractor; +use Symfony\Component\Serializer\Annotation\Ignore; use Symfony\Component\Serializer\Exception\LogicException; use Symfony\Component\Serializer\Exception\RuntimeException; use Symfony\Component\Serializer\Exception\UnexpectedValueException; @@ -919,6 +920,31 @@ public function testSamePropertyAsMethodWithMethodSerializedName() $this->assertSame($expected, $this->normalizer->normalize($object)); } + + public function testNormalizeWithIgnoreAnnotationAndPrivateProperties() + { + $classMetadataFactory = new ClassMetadataFactory(new AnnotationLoader(new AnnotationReader())); + $normalizer = new ObjectNormalizer($classMetadataFactory); + + $this->assertSame(['foo' => 'foo'], $normalizer->normalize(new ObjectDummyWithIgnoreAnnotationAndPrivateProperty())); + } + + public function testDenormalizeWithIgnoreAnnotationAndPrivateProperties() + { + $classMetadataFactory = new ClassMetadataFactory(new AnnotationLoader(new AnnotationReader())); + $normalizer = new ObjectNormalizer($classMetadataFactory); + + $obj = $normalizer->denormalize([ + 'foo' => 'set', + 'ignore' => 'set', + 'private' => 'set', + ], ObjectDummyWithIgnoreAnnotationAndPrivateProperty::class); + + $expected = new ObjectDummyWithIgnoreAnnotationAndPrivateProperty(); + $expected->foo = 'set'; + + $this->assertEquals($expected, $obj); + } } class ProxyObjectDummy extends ObjectDummy @@ -1207,3 +1233,13 @@ public function getInner() return $this->inner; } } + +class ObjectDummyWithIgnoreAnnotationAndPrivateProperty +{ + public $foo = 'foo'; + + /** @Ignore */ + public $ignored = 'ignored'; + + private $private = 'private'; +}