From 751b2f52d7af78adc2dea87e5723829746e01566 Mon Sep 17 00:00:00 2001 From: Zyra Date: Thu, 3 Sep 2026 09:32:42 +0000 Subject: [PATCH] Reproductie van issue #121 (ginkelsoft-development/buildora): FileField mist een verplichte extensie/MIME-whitelist en deny-list voor executables. FileField::accept()/maxSize()/imageDimensions() zijn puur decoratief (help-tekst + HTML accept-attribuut + client-side Alpine.js-check) en worden nooit omgezet naar server-side validatieregels. BuildoraController::store()/update() valideert uploads alleen als een resource-auteur zelf expliciet ->validation(['veld' => 'mimes:...']) toevoegt; standaard is er geen enkele beperking op bestandstype of -grootte. Nieuwe tests leggen dit huidige gedrag vast: - FileFieldTest: accept()/maxSize()/imageDimensions() leveren geen validatieregels op. - FileUploadSecurityTest: de controller slaat een .php/.phtml/.phar-bestand gewoon op ondanks ->accept('image/*'), en een bestand van 5MB ondanks ->maxSize(200). Ook vastgelegd: padtraversal via de bestandsnaam werkt niet, omdat Laravel's UploadedFile::store() standaard een willekeurige hash-naam gebruikt (mitigatie van het framework, niet van Buildora). Geen productiecode gewijzigd; alleen onderzoek + reproductietests voor de vervolgtaak die de fix bouwt. Co-Authored-By: Claude Sonnet 5 --- tests/Feature/FileUploadSecurityTest.php | 160 +++++++++++++++++++++++ tests/Fixtures/DocumentBuildora.php | 33 +++++ tests/Fixtures/DocumentModel.php | 21 +++ tests/Unit/Fields/FileFieldTest.php | 73 +++++++++++ 4 files changed, 287 insertions(+) create mode 100644 tests/Feature/FileUploadSecurityTest.php create mode 100644 tests/Fixtures/DocumentBuildora.php create mode 100644 tests/Fixtures/DocumentModel.php create mode 100644 tests/Unit/Fields/FileFieldTest.php diff --git a/tests/Feature/FileUploadSecurityTest.php b/tests/Feature/FileUploadSecurityTest.php new file mode 100644 index 0000000..0c39098 --- /dev/null +++ b/tests/Feature/FileUploadSecurityTest.php @@ -0,0 +1,160 @@ +store($field->getPath() ?? 'uploads', $disk); + * + * Er wordt nergens een `mimes:`/`mimetypes:`/`max:`-regel afgeleid van + * FileField::accept()/maxSize()/imageDimensions(). Die methodes vullen + * alleen help-tekst en het HTML `accept`-attribuut + een client-side + * Alpine.js-check (resources/views/components/input/file.blade.php) — beide + * triviaal te omzeilen door de request direct te versturen (curl/Postman/ + * aangepaste HTML). + * + * Deze tests wijzigen geen productiecode; ze tonen alleen aan wat er nu + * gebeurt, zodat een vervolgtaak de fix (server-side whitelist + deny-list + * + finfo-detectie + UUID-bestandsnamen) kan bouwen en tegen deze tests kan + * verifiëren (rood -> groen). + */ +class FileUploadSecurityTest extends TestCase +{ + protected function setUp(): void + { + parent::setUp(); + + require_once __DIR__ . '/../Fixtures/DocumentModel.php'; + require_once __DIR__ . '/../Fixtures/DocumentBuildora.php'; + + Schema::create('documents', function (Blueprint $table) { + $table->id(); + $table->string('attachment')->nullable(); + $table->timestamps(); + }); + + Storage::fake('public'); + } + + /** + * @test + * + * Kern van issue #121: een FileField met ->accept('image/*') slaat + * gewoon een .php-bestand op, omdat 'accept' nooit tot een server-side + * validatieregel wordt vertaald. + */ + public function it_stores_a_php_file_even_though_the_field_only_accepts_images(): void + { + $malicious = UploadedFile::fake()->createWithContent( + 'shell.php', + "" + ); + + $response = (new BuildoraController())->store( + $this->requestWithFile($malicious), + 'document' + ); + + $this->assertSame(302, $response->getStatusCode()); + + $document = Document::first(); + $this->assertNotNull($document, 'Record is aangemaakt ondanks kwaadaardige upload.'); + $this->assertNotNull($document->attachment, 'Bestand is opgeslagen zonder server-side typecontrole.'); + + Storage::disk('public')->assertExists($document->attachment); + } + + /** + * @test + * + * Zelfs zonder enige ->accept() te zetten, is er geen ingebouwde + * deny-list voor uitvoerbare extensies (.php, .phtml, .phar, .cgi, ...). + */ + public function it_has_no_builtin_deny_list_for_executable_extensions(): void + { + foreach (['shell.phtml', 'shell.phar', 'shell.cgi'] as $filename) { + Document::query()->delete(); + + $malicious = UploadedFile::fake()->createWithContent( + $filename, + "" + ); + + (new BuildoraController())->store($this->requestWithFile($malicious), 'document'); + + $document = Document::first(); + $this->assertNotNull( + $document->attachment ?? null, + "Bestand [$filename] had geweigerd moeten worden door een deny-list, maar is opgeslagen." + ); + } + } + + /** + * @test + * + * ->maxSize(200) (200 KB) op het FileField wordt nergens afgedwongen: + * een bestand van 5 MB wordt gewoon geaccepteerd en opgeslagen. + */ + public function it_stores_a_file_that_exceeds_the_configured_max_size(): void + { + $oversized = UploadedFile::fake()->create('big.jpg', 5000); // 5000 KB, veld staat max. 200 KB toe + + (new BuildoraController())->store($this->requestWithFile($oversized), 'document'); + + $document = Document::first(); + $this->assertNotNull($document); + $this->assertNotNull($document->attachment, 'Te groot bestand is toch opgeslagen: maxSize() wordt niet gecontroleerd.'); + } + + /** + * @test + * + * Documenteert het huidige, WEL veilige gedrag rond padtraversal: Laravel's + * UploadedFile::store() gebruikt standaard een willekeurige hash-naam in + * plaats van de door de client opgegeven bestandsnaam, dus een naam als + * "../../../../etc/passwd" leidt niet tot het escapen van de upload-map. + * Dit is gedrag van het Laravel-framework, niet een bewuste + * beveiligingsmaatregel van Buildora/FileField — vastgelegd zodat een + * toekomstige "mooiere bestandsnaam behouden"-feature dit niet stilzwijgend + * doorbreekt. + */ + public function path_traversal_in_the_original_filename_does_not_escape_the_upload_directory(): void + { + $traversal = UploadedFile::fake()->createWithContent( + '../../../../etc/passwd', + 'root:x:0:0:root:/root:/bin/bash' + ); + + (new BuildoraController())->store($this->requestWithFile($traversal), 'document'); + + $document = Document::first(); + $this->assertNotNull($document->attachment); + $this->assertStringStartsWith('uploads/', $document->attachment); + $this->assertStringNotContainsString('..', $document->attachment); + + Storage::disk('public')->assertExists($document->attachment); + } + + private function requestWithFile(UploadedFile $file): Request + { + $request = Request::create('/buildora/document', 'POST'); + $request->files->set('attachment', $file); + + return $request; + } +} diff --git a/tests/Fixtures/DocumentBuildora.php b/tests/Fixtures/DocumentBuildora.php new file mode 100644 index 0000000..2957462 --- /dev/null +++ b/tests/Fixtures/DocumentBuildora.php @@ -0,0 +1,33 @@ +accept() en ->maxSize() instelt. Deze fixture +// bewijst dat die instellingen puur decoratief zijn: er is geen server-side +// afdwinging van extensie/MIME-type of bestandsgrootte. + +namespace App\Buildora\Resources; + +use App\Models\Document; +use Ginkelsoft\Buildora\Fields\Types\FileField; +use Ginkelsoft\Buildora\Resources\BuildoraResource; + +if (!class_exists(DocumentBuildora::class)) { + class DocumentBuildora extends BuildoraResource + { + public static function modelClass(): string + { + return Document::class; + } + + public function defineFields(): array + { + return [ + FileField::make('attachment') + ->accept('image/*') + ->maxSize(200) // 200 KB — zie FileUploadSecurityTest: wordt niet afgedwongen. + ->disk('public') + ->path('uploads'), + ]; + } + } +} diff --git a/tests/Fixtures/DocumentModel.php b/tests/Fixtures/DocumentModel.php new file mode 100644 index 0000000..e9e0ddc --- /dev/null +++ b/tests/Fixtures/DocumentModel.php @@ -0,0 +1,21 @@ +validate() aan te roepen). Zonder dat een resource-auteur zelf + * expliciet ->validation(['veld' => 'mimes:...|max:...']) toevoegt, is er + * dus geen server-side extensie/MIME-whitelist en geen deny-list voor + * executables (.php, .phtml, .phar, ...). + */ +class FileFieldTest extends TestCase +{ + /** @test */ + public function accept_does_not_translate_into_a_server_side_validation_rule(): void + { + $field = FileField::make('attachment')->accept('image/*'); + + $this->assertSame('image/*', $field->getAccept()); + $this->assertSame( + [], + $field->getValidationRules(), + 'accept() zou geen effect mogen hebben tenzij ->validation() expliciet is gezet — ' . + 'dit bevestigt dat er standaard GEEN server-side mime/extensie-whitelist is.' + ); + } + + /** @test */ + public function max_size_does_not_translate_into_a_server_side_validation_rule(): void + { + $field = FileField::make('attachment')->maxSize(200); + + $this->assertSame(200, $field->getMaxSizeKb()); + $this->assertSame( + [], + $field->getValidationRules(), + 'maxSize() wordt nergens omgezet naar een "max:"-validatieregel.' + ); + } + + /** @test */ + public function image_dimensions_does_not_translate_into_a_server_side_validation_rule(): void + { + $field = FileField::make('attachment')->imageDimensions(800, 600); + + $this->assertSame(800, $field->getMaxWidth()); + $this->assertSame(600, $field->getMaxHeight()); + $this->assertSame( + [], + $field->getValidationRules(), + 'imageDimensions() wordt nergens omgezet naar een "dimensions:"-validatieregel.' + ); + } + + /** @test */ + public function a_file_field_without_any_configuration_has_no_validation_rules_at_all(): void + { + // Zelfs een "kale" FileField::make() levert geen enkele impliciete + // server-side beperking op: geen verplichte extensie-whitelist en + // geen deny-list voor uitvoerbare bestandstypen. + $field = FileField::make('attachment'); + + $this->assertSame([], $field->getValidationRules()); + } +}