diff --git a/CLAUDE.md b/CLAUDE.md index 255a3ca7..66e84b64 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -96,5 +96,5 @@ The following items are known and should be addressed when hardening for product - **No security headers**: CSP, HSTS, X-Frame-Options are not configured - **No rate limiting** on API endpoints - **Docker services** (Redis, Elasticsearch) run without authentication and with exposed ports -- **EntityMerger** uses reflection to merge all non-`@Ignore` properties — ensure sensitive fields are properly annotated +- **EntityMerger** uses a reflection allowlist: only properties annotated `#[App\Air\Util\EntityMerger\Attribute\Mergeable]` are merged from request bodies. Identity keys (`Station::$stationCode`, `Station::$provider`, `City::$slug`) and system fields are excluded by default — do not annotate them. - **Twig `|raw` usage**: `unitHtml`, `shortNameHtml`, and `exceedanceJson` use `|raw` — these values must never contain user-controlled input diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index eea77dac..4d616749 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -70,16 +70,6 @@ parameters: count: 1 path: src/Air/StationCache/StationCache.php - - - message: "#^Instanceof between ReflectionAttribute\\ and Symfony\\\\Component\\\\Serializer\\\\Attribute\\\\Ignore will always evaluate to false\\.$#" - count: 1 - path: src/Air/Util/EntityMerger/EntityMerger.php - - - - message: "#^Variable \\$reflectionClass in PHPDoc tag @var does not match any variable in the foreach loop\\: \\$reflectionProperty$#" - count: 1 - path: src/Air/Util/EntityMerger/EntityMerger.php - - message: "#^Deprecated in PHP 8\\.4\\: Parameter \\#2 \\$station \\(App\\\\Entity\\\\Station\\) is implicitly nullable via default value null\\.$#" count: 1 diff --git a/src/Air/Util/EntityMerger/Attribute/Mergeable.php b/src/Air/Util/EntityMerger/Attribute/Mergeable.php new file mode 100644 index 00000000..e2897e06 --- /dev/null +++ b/src/Air/Util/EntityMerger/Attribute/Mergeable.php @@ -0,0 +1,17 @@ +getProperties() as $reflectionProperty) { - if ($this->isPropertyExposed($reflectionProperty)) { - $setMethodName = $this->generateSetMethodName($reflectionProperty); - $getMethodName = $this->generateGetMethodName($reflectionProperty, $reflectionClass); - - try { - $newValue = $source->$getMethodName(); - - if ($newValue) { - $destination->$setMethodName($newValue); - } - } catch (\TypeError) { - // deserialized entities passed to this entity merger may not be fully stuffed with properties as - // the serializer does not call the entity's constructor as described here: - // https://stackoverflow.com/questions/31948118/jms-serializer-why-are-new-objects-not-being-instantiated-through-constructor - // - // to avoid these problems, we just skipped empty or null properties and act like we just don't care. - } + if (!$this->isPropertyMergeable($reflectionProperty)) { + continue; } - } - return $destination; - } + // Deserialized entities may have uninitialized typed properties, because the serializer + // does not call the entity constructor. Reading such a property would throw an Error, + // so we skip anything that is not initialized on the source. See + // https://stackoverflow.com/questions/31948118/jms-serializer-why-are-new-objects-not-being-instantiated-through-constructor + if (!$reflectionProperty->isInitialized($source)) { + continue; + } - protected function isPropertyExposed(\ReflectionProperty $reflectionProperty): bool - { - $propertyAttributes = $reflectionProperty->getAttributes(Ignore::class); + $getMethodName = $this->generateGetMethodName($reflectionProperty, $reflectionClass); + $setMethodName = $this->generateSetMethodName($reflectionProperty); + + if (null === $getMethodName || !$reflectionClass->hasMethod($setMethodName)) { + continue; + } + + $newValue = $source->$getMethodName(); - foreach ($propertyAttributes as $propertyAttribute) { - if ($propertyAttribute instanceof Ignore) { - return false; + // Only null is treated as "not provided". Falsy-but-valid values such as 0, 0.0, + // '' or false are intentionally allowed so a field can be corrected to them. + if (null !== $newValue) { + $destination->$setMethodName($newValue); } } - return true; + return $destination; + } + + /** + * Allowlist: a property is only merged when it explicitly carries the #[Mergeable] attribute. + * Identity keys and system fields are therefore protected by default. + */ + protected function isPropertyMergeable(\ReflectionProperty $reflectionProperty): bool + { + return count($reflectionProperty->getAttributes(Mergeable::class)) > 0; } protected function generateSetMethodName(\ReflectionProperty $reflectionProperty): string diff --git a/src/Entity/City.php b/src/Entity/City.php index e4e52503..40405a3b 100644 --- a/src/Entity/City.php +++ b/src/Entity/City.php @@ -2,6 +2,7 @@ namespace App\Entity; +use App\Air\Util\EntityMerger\Attribute\Mergeable; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; use Doctrine\ORM\Mapping as ORM; @@ -22,12 +23,14 @@ class City implements \Stringable protected ?\DateTime $createdAt = null; #[ORM\Column(type: 'string', nullable: false)] + #[Mergeable] protected ?string $name = null; #[ORM\Column(type: 'string', nullable: false)] protected ?string $slug = null; #[ORM\Column(type: 'string', nullable: true)] + #[Mergeable] protected ?string $description = null; #[ORM\OneToMany(targetEntity: 'Station', mappedBy: 'city')] diff --git a/src/Entity/Station.php b/src/Entity/Station.php index 4798f4e0..232543c7 100644 --- a/src/Entity/Station.php +++ b/src/Entity/Station.php @@ -2,6 +2,7 @@ namespace App\Entity; +use App\Air\Util\EntityMerger\Attribute\Mergeable; use App\DBAL\Types\AreaType; use App\DBAL\Types\StationType; use App\Geo\Coordinate\Coordinate; @@ -29,15 +30,19 @@ class Station extends Coordinate protected ?string $stationCode = null; #[ORM\Column(type: 'integer', nullable: true)] + #[Mergeable] protected ?int $ubaStationId = null; #[ORM\Column(type: 'string', nullable: true)] + #[Mergeable] protected ?string $title = null; #[ORM\Column(type: 'float', nullable: false)] + #[Mergeable] protected ?float $latitude = null; #[ORM\Column(type: 'float', nullable: false)] + #[Mergeable] protected ?float $longitude = null; #[ORM\Column( @@ -53,12 +58,15 @@ class Station extends Coordinate protected ?City $city = null; #[ORM\Column(type: 'date', nullable: true)] + #[Mergeable] protected ?\DateTime $fromDate = null; #[ORM\Column(type: 'date', nullable: true)] + #[Mergeable] protected ?\DateTime $untilDate = null; #[ORM\Column(type: 'integer', nullable: true)] + #[Mergeable] protected ?int $altitude = null; #[DoctrineAssert\EnumType(entity: StationType::class)] diff --git a/tests/Air/Util/EntityMerger/EntityMergerTest.php b/tests/Air/Util/EntityMerger/EntityMergerTest.php new file mode 100644 index 00000000..1b3e5e5d --- /dev/null +++ b/tests/Air/Util/EntityMerger/EntityMergerTest.php @@ -0,0 +1,133 @@ +setTitle('new title'); + $destination = (new EntityMergerFixture())->setTitle('old title'); + + (new EntityMerger())->merge($source, $destination); + + $this->assertSame('new title', $destination->getTitle()); + } + + public function testNonMergeableIdentityFieldIsProtected(): void + { + $source = (new EntityMergerFixture())->setIdentity('attacker-value'); + $destination = (new EntityMergerFixture())->setIdentity('original-value'); + + (new EntityMerger())->merge($source, $destination); + + $this->assertSame('original-value', $destination->getIdentity()); + } + + public function testFalsyZeroValueIsMerged(): void + { + // Regression: the old `if ($newValue)` guard discarded 0 / 0.0 / '' / false. + $source = (new EntityMergerFixture())->setCount(0); + $destination = (new EntityMergerFixture())->setCount(42); + + (new EntityMerger())->merge($source, $destination); + + $this->assertSame(0, $destination->getCount()); + } + + public function testNullValueIsNotMerged(): void + { + $source = new EntityMergerFixture(); // title stays null + $destination = (new EntityMergerFixture())->setTitle('keep me'); + + (new EntityMerger())->merge($source, $destination); + + $this->assertSame('keep me', $destination->getTitle()); + } + + public function testUninitializedTypedPropertyIsSkipped(): void + { + // Serializer-hydrated sources can leave typed properties uninitialized; reading the + // getter throws a TypeError which must be swallowed instead of aborting the merge. + $source = (new EntityMergerFixture())->setTitle('from source'); // $number left uninitialized + $destination = (new EntityMergerFixture())->setTitle('dest')->setNumber(7); + + (new EntityMerger())->merge($source, $destination); + + $this->assertSame('from source', $destination->getTitle()); + $this->assertSame(7, $destination->getNumber()); + } + + public function testMergeReturnsDestination(): void + { + $destination = new EntityMergerFixture(); + + $this->assertSame($destination, (new EntityMerger())->merge(new EntityMergerFixture(), $destination)); + } +} + +class EntityMergerFixture +{ + #[Mergeable] + private ?string $title = null; + + private ?string $identity = null; + + #[Mergeable] + private ?int $count = null; + + #[Mergeable] + private int $number; + + public function getTitle(): ?string + { + return $this->title; + } + + public function setTitle(?string $title): self + { + $this->title = $title; + + return $this; + } + + public function getIdentity(): ?string + { + return $this->identity; + } + + public function setIdentity(?string $identity): self + { + $this->identity = $identity; + + return $this; + } + + public function getCount(): ?int + { + return $this->count; + } + + public function setCount(?int $count): self + { + $this->count = $count; + + return $this; + } + + public function getNumber(): int + { + return $this->number; + } + + public function setNumber(int $number): self + { + $this->number = $number; + + return $this; + } +}