Merge pull request #1469 from jphetphoumy/dev

CWE-434: Unrestricted Upload of File with Dangerous Type. Polyglot fi…

src/Naming/OrignameNamer.php

@@ -14,6 +14,8 @@
  */
 final class OrignameNamer implements NamerInterface, ConfigurableInterface
 {
+    use Polyfill\FileExtensionTrait;
+
     private bool $transliterate = false;
 
     public function __construct(private readonly Transliterator $transliterator)
@@ -39,6 +41,12 @@ public function name(object $object, PropertyMapping $mapping): string
             $name = $this->transliterator->transliterate($name);
         }
 
+        $extension = $this->getExtension($file);
+
+        if (\is_string($extension) && '' !== $extension) {
+            $name = "$name.$extension";
+        }
+
         return \uniqid().'_'.$name;
     }
 }

src/Naming/Polyfill/FileExtensionTrait.php

@@ -16,11 +16,6 @@ private function getExtension(File $file): ?string
         if (!$file instanceof UploadedFile && !$file instanceof ReplacingFile) {
             throw new \InvalidArgumentException('Unexpected type for $file: '.$file::class);
         }
-        $originalName = $file->getClientOriginalName();
-
-        if ('' !== ($extension = \pathinfo($originalName, \PATHINFO_EXTENSION))) {
-            return $extension;
-        }
 
         if ('' !== ($extension = $file->guessExtension())) {
             return $extension;

src/Naming/SlugNamer.php

@@ -12,6 +12,8 @@
  */
 final class SlugNamer implements NamerInterface
 {
+    use Polyfill\FileExtensionTrait;
+
     public function __construct(private readonly Transliterator $transliterator, private readonly object $service, private readonly string $method)
     {
     }
@@ -20,10 +22,12 @@ public function name(object $object, PropertyMapping $mapping): string
     {
         $file = $mapping->getFile($object);
         $originalName = $file->getClientOriginalName();
-        $extension = \strtolower(\pathinfo($originalName, \PATHINFO_EXTENSION));
+        $extension = $this->getExtension($file);
         $basename = \substr(\pathinfo($originalName, \PATHINFO_FILENAME), 0, 240);
         $basename = \strtolower($this->transliterator->transliterate($basename));
-        $slug = \sprintf('%s.%s', $basename, $extension);
+        $slug = is_string($extension) && '' !== $extension
+            ? \sprintf('%s.%s', $basename, $extension)
+            : $basename;
 
         // check if there another object with same slug
         $num = 0;
@@ -32,7 +36,11 @@ public function name(object $object, PropertyMapping $mapping): string
             if (null === $otherObject) {
                 return $slug;
             }
-            $slug = \sprintf('%s-%d.%s', $basename, ++$num, $extension);
+
+            $slug = \is_string($extension) && '' !== $extension
+                ? \sprintf('%s-%d.%s', $basename, ++$num, $extension)
+                : \sprintf('%s-%d', $basename, ++$num)
+            ;
         }
     }
 }

src/Naming/SmartUniqueNamer.php

@@ -13,6 +13,8 @@
  */
 final class SmartUniqueNamer implements NamerInterface
 {
+    use Polyfill\FileExtensionTrait;
+
     public function __construct(private readonly Transliterator $transliterator)
     {
     }
@@ -22,10 +24,12 @@ public function name(object $object, PropertyMapping $mapping): string
         $file = $mapping->getFile($object);
         $originalName = $file->getClientOriginalName();
         $originalName = $this->transliterator->transliterate($originalName);
-        $originalExtension = \strtolower(\pathinfo($originalName, \PATHINFO_EXTENSION));
+        $originalExtension = $this->getExtension($file);
         $originalBasename = \pathinfo($originalName, \PATHINFO_FILENAME);
         $uniqId = \str_replace('.', '', \uniqid('-', true));
-        $uniqExtension = \sprintf('%s.%s', $uniqId, $originalExtension);
+        $uniqExtension = \is_string($originalExtension) && '' !== $originalExtension
+            ? \sprintf('%s.%s', $uniqId, $originalExtension)
+            : $uniqId;
         $smartName = \sprintf('%s%s', $originalBasename, $uniqExtension);
 
         // Check if smartName is an acceptable size (some filesystems accept a max of 255)

tests/Naming/Base64NamerTest.php

@@ -34,9 +34,6 @@ public static function fileDataProvider(): array
     public function testNameReturnsTheRightName(string $expectedFileName, string $extension, ?int $length): void
     {
         $file = $this->getUploadedFileMock();
-        $file->expects(self::once())
-            ->method('getClientOriginalName')
-            ->willReturn('foo');
 
         $file->expects(self::once())
             ->method('guessExtension')

tests/Naming/HashNamerTest.php

@@ -35,9 +35,6 @@ public static function fileDataProvider(): array
     public function testNameReturnsTheRightName(string $expectedFileName, string $extension, string $algorithm, ?int $length): void
     {
         $file = $this->getUploadedFileMock();
-        $file->expects(self::once())
-            ->method('getClientOriginalName')
-            ->willReturn('foo');
 
         $file->expects(self::once())
             ->method('guessExtension')

tests/Naming/PropertyNamerTest.php

@@ -23,10 +23,10 @@ public static function fileDataProvider(): array
         $weirdEntity->someProperty = 'Yéô';
 
         return [
-            'with ext' => ['some-file-name.jpeg',    'foo.jpeg',                 $entity,      'someProperty',     false],
-            'without ext' => ['some-file-name',      'foo',                      $entity,      'someProperty',     false],
-            'method call' => ['some-file-name.jpeg', 'generated-file-name.jpeg', $entity,      'generateFileName', false],
-            'translit.' => ['some-file-name.jpeg',   'yeo.jpeg',                 $weirdEntity, 'someProperty',     true],
+            'with ext' => ['some-file-name.jpeg',    'foo.jpg',                 'jpg', $entity,      'someProperty',     false],
+            'without ext' => ['some-file-name',      'foo',                      null, $entity,      'someProperty',     false],
+            'method call' => ['some-file-name.jpeg', 'generated-file-name.jpg', 'jpg', $entity,      'generateFileName', false],
+            'translit.' => ['some-file-name.jpeg',   'yeo.jpg',                 'jpg', $weirdEntity, 'someProperty',     true],
         ];
     }
 
@@ -36,6 +36,7 @@ public static function fileDataProvider(): array
     public function testNameReturnsTheRightName(
         string $originalFileName,
         string $expectedFileName,
+        ?string $guessedExtension,
         object $entity,
         string $propertyName,
         bool $transliterate
@@ -45,6 +46,10 @@ public function testNameReturnsTheRightName(
             ->method('getClientOriginalName')
             ->willReturn($originalFileName);
 
+        $file
+            ->method('guessExtension')
+            ->willReturn($guessedExtension);
+
         $mapping = $this->getPropertyMappingMock();
         $mapping->expects(self::once())
             ->method('getFile')

tests/Naming/SlugNamerTest.php

@@ -11,16 +11,17 @@ final class SlugNamerTest extends TestCase
     public static function fileDataProvider(): array
     {
         return [
-            // case -> original name, result pattern
-            'non existing' => ['lala.jpeg', '/lala.jpeg/'],
-            'existing' => ['làlà.mp3', '/lala-1.mp3/'],
+            // case -> original name, guessedExtension, result pattern
+            'non existing' => ['lala.jpeg', 'jpg', '/lala.jpg/'],
+            'guess extension null' => ['lala.jpeg', null, '/lala$/'],
+            'existing' => ['làlà.mp3', 'mp3', '/lala-1.mp3/'],
         ];
     }
 
     /**
      * @dataProvider fileDataProvider
      */
-    public function testNameReturnsAnUniqueName(string $originalName, string $pattern): void
+    public function testNameReturnsAnUniqueName(string $originalName, ?string $guessedExtension, string $pattern): void
     {
         $file = $this->getUploadedFileMock();
         $file
@@ -29,6 +30,12 @@ public function testNameReturnsAnUniqueName(string $originalName, string $patter
             ->willReturn($originalName)
         ;
 
+        $file
+            ->expects(self::once())
+            ->method('guessExtension')
+            ->willReturn($guessedExtension)
+        ;
+
         $entity = new \stdClass();
 
         $mapping = $this->getPropertyMappingMock();

tests/Naming/SmartUniqidNamerTest.php

@@ -10,26 +10,27 @@ final class SmartUniqidNamerTest extends TestCase
     public static function fileDataProvider(): array
     {
         return [
-            // case -> original name, result pattern
-            'typical' => ['lala.jpeg', '/lala-[[:xdigit:]]{22}\.jpeg/'],
-            'accented' => ['làlà.mp3', '/lala-[[:xdigit:]]{22}\.mp3/'],
-            'spaced' => ['a Foo Bar.txt', '/a-foo-bar-[[:xdigit:]]{22}\.txt/'],
-            'special char' => ['yezz!.png', '/yezz-[[:xdigit:]]{22}\.png/'],
-            'long basename' => [\str_repeat('a', 256).'.txt', '/a{228}-[[:xdigit:]]{22}\.txt/'],
-            'long extension' => ['a.'.\str_repeat('a', 256), '/a-[[:xdigit:]]{22}\.a{230}/'],
+            // case -> original name, guessed extension, result pattern
+            'typical' => ['lala.jpeg', 'jpg', '/lala-[[:xdigit:]]{22}\.jpg/'],
+            'accented' => ['làlà.mp3', 'mp3', '/lala-[[:xdigit:]]{22}\.mp3/'],
+            'spaced' => ['a Foo Bar.txt', 'txt', '/a-foo-bar-[[:xdigit:]]{22}\.txt/'],
+            'special char' => ['yezz!.png', 'png', '/yezz-[[:xdigit:]]{22}\.png/'],
+            'long basename' => [\str_repeat('a', 256).'.txt', 'txt', '/a{228}-[[:xdigit:]]{22}\.txt/'],
+            'long extension' => ['a.'.\str_repeat('a', 256), null, '/a-[[:xdigit:]]{22}$/'],
             'long basename and extension' => [\str_repeat('a', 256).'.txt'.\str_repeat('a', 256),
-                                              '/a{228}-[[:xdigit:]]{22}\.txt/', ],
-            'double extension' => ['lala.png.jpg', '/lala-png-[[:xdigit:]]{22}\.jpg/'],
-            'uppercase extension' => ['lala.JPEG', '/lala-[[:xdigit:]]{22}\.jpeg/'],
-            'double uppercase extension' => ['lala.JPEG.JPEG', '/lala-jpeg-[[:xdigit:]]{22}\.jpeg/'],
-            'dot in filename' => ['filename has . spaces (2).jpg', '/filename-has-spaces-2-[[:xdigit:]]{22}\.jpg/'],
+                                              'txt', '/a{228}-[[:xdigit:]]{22}\.txt$/', ],
+            'double extension' => ['lala.png.jpg', 'jpg', '/lala-png-[[:xdigit:]]{22}\.jpg/'],
+            'uppercase extension' => ['lala.JPEG', 'jpg', '/lala-[[:xdigit:]]{22}\.jpg/'],
+            'double uppercase extension' => ['lala.JPEG.JPEG', 'jpg', '/lala-jpeg-[[:xdigit:]]{22}\.jpg/'],
+            'dot in filename' => ['filename has . spaces (2).jpg', 'jpg', '/filename-has-spaces-2-[[:xdigit:]]{22}\.jpg/'],
+            'file with no extension with null mimetype' => ['lala', null, '/lala-[[:xdigit:]]{22}$/'],
         ];
     }
 
     /**
      * @dataProvider fileDataProvider
      */
-    public function testNameReturnsAnUniqueName(string $originalName, string $pattern): void
+    public function testNameReturnsAnUniqueName(string $originalName, ?string $guessExtension, string $pattern): void
     {
         $file = $this->getUploadedFileMock();
         $file
@@ -38,6 +39,12 @@ public function testNameReturnsAnUniqueName(string $originalName, string $patter
             ->willReturn($originalName)
         ;
 
+        $file
+            ->expects(self::once())
+            ->method('guessExtension')
+            ->willReturn($guessExtension)
+        ;
+
         $entity = new \stdClass();
 
         $mapping = $this->getPropertyMappingMock();

tests/Naming/UniqidNamerTest.php

@@ -16,13 +16,13 @@ public static function fileDataProvider(): array
     {
         return [
             // original_name, guessed_extension, pattern
-            ['lala.jpeg',     null,              '/[a-z0-9]{13}.jpeg/'],
-            ['lala.mp3',      'mpga',            '/[a-z0-9]{13}.mp3/'],
+            ['lala.jpeg',     null,              '/[a-z0-9]{13}/'],
+            ['lala.mp3',      'mp3',            '/[a-z0-9]{13}.mp3/'],
             ['lala',          'mpga',            '/[a-z0-9]{13}.mpga/'],
-            ['lala',          null,              '/[a-z0-9]{13}/'],
-            ['lala.0',        null,              '/[a-z0-9]{13}\\.0/'],
-            ['lala.data.0',   null,              '/[a-z0-9]{13}\\.0/'],
-            ['lala.data.0',   'gzip',            '/[a-z0-9]{13}\\.0/'],
+            ['lala',          null,              '/[a-z0-9]{13}$/'],
+            ['lala.0',        null,              '/[a-z0-9]{13}$/'],
+            ['lala.data.0',   null,              '/[a-z0-9]{13}$/'],
+            ['lala.data.0',   'gzip',            '/[a-z0-9]{13}.gzip/'],
         ];
     }