Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Naming/OrignameNamer.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
*/
final class OrignameNamer implements NamerInterface, ConfigurableInterface
{
use Polyfill\FileExtensionTrait;

private bool $transliterate = false;

public function __construct(private readonly Transliterator $transliterator)
Expand All @@ -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;
}
}
5 changes: 0 additions & 5 deletions src/Naming/Polyfill/FileExtensionTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 11 additions & 3 deletions src/Naming/SlugNamer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
}
Expand All @@ -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;
Expand All @@ -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)
;
}
}
}
8 changes: 6 additions & 2 deletions src/Naming/SmartUniqueNamer.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
*/
final class SmartUniqueNamer implements NamerInterface
{
use Polyfill\FileExtensionTrait;

public function __construct(private readonly Transliterator $transliterator)
{
}
Expand All @@ -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)
Expand Down
3 changes: 0 additions & 3 deletions tests/Naming/Base64NamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
3 changes: 0 additions & 3 deletions tests/Naming/HashNamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
13 changes: 9 additions & 4 deletions tests/Naming/PropertyNamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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],
];
}

Expand All @@ -36,6 +36,7 @@ public static function fileDataProvider(): array
public function testNameReturnsTheRightName(
string $originalFileName,
string $expectedFileName,
?string $guessedExtension,
object $entity,
string $propertyName,
bool $transliterate
Expand All @@ -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')
Expand Down
15 changes: 11 additions & 4 deletions tests/Naming/SlugNamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand Down
33 changes: 20 additions & 13 deletions tests/Naming/SmartUniqidNamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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();
Expand Down
12 changes: 6 additions & 6 deletions tests/Naming/UniqidNamerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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/'],
];
}

Expand Down
Loading