Disallow non-scalar property values when inserting and updating

This will prevent unintended changes to bound parameter behavior.
devtheorem · Oct 22, 2024 · f5be4ac · f5be4ac
1 parent 399eb98
commit f5be4ac
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 21 deletions.
diff --git a/composer.json b/composer.json
@@ -34,6 +34,7 @@
         "analyze": "psalm",
         "test": "phpunit",
         "test-mssql": "phpunit --exclude-group mysql",
-        "test-mysql": "phpunit --exclude-group mssql"
+        "test-mysql": "phpunit --exclude-group mssql",
+        "test-without-db": "phpunit --exclude-group mssql,mysql"
     }
 }
diff --git a/src/Entities.php b/src/Entities.php
@@ -182,7 +182,8 @@ public function patchByIds(array $ids, array $mergePatch): int
             return 0;
         }
 
-        $colVals = self::propertiesToColumns($this->map, $this->processValues($mergePatch, $ids));
+        $data = $this->processValues($mergePatch, $ids);
+        $colVals = self::propertiesToColumns($this->map, $data, complexValues: false);
         $colVals = $this->processRow($colVals, $ids);
 
         return $this->db->updateRows($this->getTableName(), $colVals, [$this->idColumn => $ids]);
@@ -274,7 +275,7 @@ public function getEntities(array $filter = [], array $fields = [], array $sort
         /** @psalm-suppress MixedArgumentTypeCoercion */
         $select = $this->db->select($this->getBaseSelect($queryOptions))
             ->where(self::propertiesToColumns($selectMap, $processedFilter))
-            ->orderBy(self::propertiesToColumns($selectMap, $sort));
+            ->orderBy(self::propertiesToColumns($selectMap, $sort, complexValues: false));
 
         if ($limit !== 0) {
             $select->offset($offset, $limit);
@@ -305,8 +306,12 @@ public function countEntities(array $filter = []): int
      * Converts nested properties to an array of columns and values using a map.
      * @return array<string, mixed>
      */
-    public static function propertiesToColumns(array $map, array $properties, bool $allowExtraProperties = false): array
-    {
-        return Helpers::propsToColumns($map, $properties, $allowExtraProperties, true);
+    public static function propertiesToColumns(
+        array $map,
+        array $properties,
+        bool $ignoreUnmapped = false,
+        bool $complexValues = true,
+    ): array {
+        return Helpers::propsToColumns($map, $properties, $ignoreUnmapped, $complexValues, true);
     }
 }
diff --git a/src/Helpers.php b/src/Helpers.php
@@ -290,16 +290,24 @@ public static function propMapToAliasMap(array $map): array
      */
     public static function allPropertiesToColumns(array $map, array $properties): array
     {
-        self::propsToColumns($properties, $map, false, false);
-        return self::propsToColumns($map, $properties, true, true);
+        // ensure that $map doesn't contain any properties that aren't in $properties
+        self::propsToColumns($properties, $map, false, false, false);
+        return self::propsToColumns($map, $properties, true, false, true);
     }
 
     /**
      * @param array<string, mixed> $columns
      * @return array<string, mixed>
      */
-    public static function propsToColumns(array $map, array $properties, bool $allowExtraProperties, bool $buildColumns, string $context = '', array &$columns = []): array
-    {
+    public static function propsToColumns(
+        array $map,
+        array $properties,
+        bool $ignoreUnmapped,
+        bool $complexValues,
+        bool $buildColumns,
+        string $context = '',
+        array $columns = [],
+    ): array {
         if ($context !== '') {
             $context .= '.';
         }
@@ -308,7 +316,7 @@ public static function propsToColumns(array $map, array $properties, bool $allow
             $contextProp = $context . $property;
 
             if (!array_key_exists($property, $map)) {
-                if ($allowExtraProperties) {
+                if ($ignoreUnmapped) {
                     continue;
                 }
 
@@ -322,20 +330,31 @@ public static function propsToColumns(array $map, array $properties, bool $allow
             if (is_array($newMap)) {
                 if (!is_array($value)) {
                     if ($buildColumns) {
-                        throw new HttpException("Expected {$contextProp} property to be an object, got " . gettype($value), StatusCode::BAD_REQUEST);
+                        $msg = "Expected $contextProp property to be an object, got " . get_debug_type($value);
+                        throw new HttpException($msg, StatusCode::BAD_REQUEST);
                     } else {
                         continue;
                     }
                 }
 
-                self::propsToColumns($newMap, $value, $allowExtraProperties, $buildColumns, $contextProp, $columns);
+                $columns = self::propsToColumns(
+                    map: $newMap,
+                    properties: $value,
+                    ignoreUnmapped: $ignoreUnmapped,
+                    complexValues: $complexValues,
+                    buildColumns: $buildColumns,
+                    context: $contextProp,
+                    columns: $columns,
+                );
             } elseif ($buildColumns) {
                 if (!is_string($newMap)) {
-                    throw new \Exception('Map values must be arrays or strings, found ' . gettype($newMap) . " for {$contextProp} property");
-                }
-
-                if (array_key_exists($newMap, $columns)) {
-                    throw new \Exception("Column '{$newMap}' is mapped to more than one property ({$contextProp})");
+                    $msg = 'Map values must be arrays or strings, found ' . get_debug_type($newMap) . " for $contextProp property";
+                    throw new \Exception($msg);
+                } elseif (array_key_exists($newMap, $columns)) {
+                    throw new \Exception("Column '$newMap' is mapped to more than one property ($contextProp)");
+                } elseif (!$complexValues && !is_null($value) && !is_scalar($value)) {
+                    $msg = "Expected $contextProp property to have a scalar value, got " . get_debug_type($value);
+                    throw new HttpException($msg, StatusCode::BAD_REQUEST);
                 }
 
                 /** @psalm-suppress MixedAssignment */

diff --git a/test/EntitiesTest.php b/test/EntitiesTest.php
@@ -82,7 +82,15 @@ public function testPropertiesToColumns(): void
             'BinaryColumn' => ['abc123', 1],
         ];
 
-        $this->assertSame($binaryExpectation, Helpers::allPropertiesToColumns($binaryMap, $binaryObj));
+        $this->assertSame($binaryExpectation, Entities::propertiesToColumns($binaryMap, $binaryObj));
+
+        try {
+            Helpers::allPropertiesToColumns($binaryMap, $binaryObj);
+            $this->fail('Failed to throw exception for non-scalar property value');
+        } catch (\Exception $e) {
+            $this->assertSame('Expected binaryProp property to have a scalar value, got array', $e->getMessage());
+            $this->assertSame(400, $e->getCode());
+        }
     }
 
     public function testInvalidMap(): void
@@ -94,7 +102,8 @@ public function testInvalidMap(): void
                 Entities::propertiesToColumns(['prop' => $type], ['prop' => true]);
                 $this->fail('Failed to throw exception for invalid map value');
             } catch (\Exception $e) {
-                $this->assertSame("Map values must be arrays or strings, found " . gettype($type) . " for prop property", $e->getMessage());
+                $msg = "Map values must be arrays or strings, found " . get_debug_type($type) . " for prop property";
+                $this->assertSame($msg, $e->getMessage());
             }
         }
 
@@ -140,7 +149,7 @@ public function testInvalidPropertiesToColumns(): void
             Entities::propertiesToColumns($this->propertyMap, $badData);
             $this->fail('Failed to throw exception for null property');
         } catch (HttpException $e) {
-            $this->assertSame("Expected client property to be an object, got NULL", $e->getMessage());
+            $this->assertSame("Expected client property to be an object, got null", $e->getMessage());
         }
 
         try {