Make EntityMerger an allowlist; fix Ignore & falsy-merge bugs (#523)

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

phpstan-baseline.neon

@@ -70,16 +70,6 @@ parameters:
 			count: 1
 			path: src/Air/StationCache/StationCache.php
 
-		-
-			message: "#^Instanceof between ReflectionAttribute\\<Symfony\\\\Component\\\\Serializer\\\\Attribute\\\\Ignore\\> 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

src/Air/Util/EntityMerger/Attribute/Mergeable.php

@@ -0,0 +1,17 @@
+<?php declare(strict_types=1);
+
+namespace App\Air\Util\EntityMerger\Attribute;
+
+/**
+ * Marks an entity property as safe to overwrite through EntityMerger.
+ *
+ * EntityMerger uses an explicit allowlist: only properties carrying this
+ * attribute are copied from the deserialized request body onto the managed
+ * entity. Identity keys (e.g. Station::$stationCode, City::$slug) and system
+ * fields must NOT be annotated, so they can never be changed via the update
+ * endpoints.
+ */
+#[\Attribute(\Attribute::TARGET_PROPERTY)]
+final class Mergeable
+{
+}

src/Air/Util/EntityMerger/EntityMerger.php

@@ -2,7 +2,7 @@
 
 namespace App\Air\Util\EntityMerger;
 
-use Symfony\Component\Serializer\Attribute\Ignore;
+use App\Air\Util\EntityMerger\Attribute\Mergeable;
 
 class EntityMerger implements EntityMergerInterface
 {
@@ -11,42 +11,45 @@ public function merge(object $source, object $destination): object
     {
         $reflectionClass = new \ReflectionClass($source);
 
-        /** @var \ReflectionClass $reflectionClass */
         foreach ($reflectionClass->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

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')]

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

tests/Air/Util/EntityMerger/EntityMergerTest.php

@@ -0,0 +1,133 @@
+<?php declare(strict_types=1);
+
+namespace App\Tests\Air\Util\EntityMerger;
+
+use App\Air\Util\EntityMerger\Attribute\Mergeable;
+use App\Air\Util\EntityMerger\EntityMerger;
+use PHPUnit\Framework\TestCase;
+
+class EntityMergerTest extends TestCase
+{
+    public function testMergeableFieldIsCopied(): void
+    {
+        $source = (new EntityMergerFixture())->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;
+    }
+}