Fixes #10387, #12460, and #19389 - better validation for text files

snipe · snipe · commit 5478c671ab24 · 2026-07-28T21:16:39.000+01:00
Files whose sniff yields nothing usable (empty, octet-stream, INI-shaped) now pass.
diff --git a/app/Helpers/Helper.php b/app/Helpers/Helper.php
@@ -1404,6 +1404,7 @@ public static function filetype_icon($filename)
             'png' => 'far fa-image',
             'webp' => 'far fa-image',
             'avif' => 'far fa-image',
+            'ico' => 'far fa-image',
             'svg' => 'fas fa-vector-square',
 
             // word
@@ -1428,14 +1429,17 @@ public static function filetype_icon($filename)
             'txt' => 'far fa-file-alt',
             'rtf' => 'far fa-file-alt',
             'xml' => 'fas fa-code',
+            'json' => 'fas fa-code',
 
             // Misc
             'pdf' => 'far fa-file-pdf',
             'lic' => 'far fa-save',
+            'key' => 'fas fa-key',
 
             // video
             'mov' => 'fa-solid fa-video',
             'mp4' => 'fa-solid fa-video',
+            'webm' => 'fa-solid fa-video',
 
             // audio
             'ogg' => 'fa-solid fa-file-audio',
diff --git a/app/Http/Controllers/Api/ImportController.php b/app/Http/Controllers/Api/ImportController.php
@@ -57,14 +57,28 @@ public function store(): JsonResponse
             $detector = new EncodingDetector;
 
             foreach ($files as $file) {
-                if (! in_array($file->getMimeType(), [
+                $allowedMimes = [
                     'application/vnd.ms-excel',
                     'text/csv',
                     'application/csv',
                     'text/x-Algol68', // because wtf CSV files?
                     'text/plain',
                     'text/comma-separated-values',
-                    'text/tsv', ])) {
+                    'text/tsv',
+                ];
+                $allowedExtensions = ['csv', 'tsv', 'txt'];
+                $clientExtension = strtolower(trim($file->getClientOriginalExtension()));
+
+                // The MIME allowlist is the primary check. When it fails,
+                // fall back to the client extension because finfo returns
+                // `application/octet-stream` for CSVs on Windows/IIS and
+                // for various perfectly-valid CSVs whose first row happens
+                // to match another magic signature. Callers reach this
+                // endpoint only with the `import` permission, and the CSV
+                // reader below will reject anything that isn't actually
+                // parseable with a more precise error than a MIME veto.
+                // See issue #10387.
+                if (! in_array($file->getMimeType(), $allowedMimes) && ! in_array($clientExtension, $allowedExtensions, true)) {
                     $results['error'] = 'File type must be CSV. Uploaded file is '.$file->getMimeType();
 
                     return response()->json(Helper::formatStandardApiResponse('error', null, $results['error']), 422);
diff --git a/app/Http/Requests/UploadFileRequest.php b/app/Http/Requests/UploadFileRequest.php
@@ -4,6 +4,7 @@
 
 use App\Helpers\Helper;
 use App\Http\Traits\ConvertsBase64ToFiles;
+use App\Rules\AllowedUploadExtension;
 use enshrined\svgSanitize\Sanitizer;
 use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Facades\Storage;
@@ -29,10 +30,21 @@ public function authorize()
      */
     public function rules()
     {
-        $max_file_size = Helper::file_upload_max_size();
-
+        // AllowedUploadExtension replaces Laravel's `mimes:` rule because
+        // `mimes:` content-sniffs, reverse-maps the detected MIME to a
+        // single extension, and rejects anything the guesser can't map,
+        // even when the client extension is on the allowlist. That was
+        // rejecting legitimate uploads (empty .txt, INI-shaped text,
+        // Windows-sniffed .csv reporting octet-stream) with a generic
+        // "check the form below" error. See issues #12460 and #10387.
         return [
-            'file.*' => 'required|mimes:'.config('filesystems.allowed_upload_extensions_for_validator').'|max:'.$max_file_size,
+            'file.*' => [
+                'bail',
+                'required',
+                'file',
+                new AllowedUploadExtension(config('filesystems.allowed_upload_extensions_array')),
+                'max:'.Helper::file_upload_max_size(),
+            ],
         ];
     }
 
@@ -44,7 +56,13 @@ public function handleFile(string $dirname, string $name_prefix, $file): string
     {
 
         $extension = $file->getClientOriginalExtension();
-        $file_name = $name_prefix.'-'.str_random(8).'-'.str_slug(basename($file->getClientOriginalName(), '.'.$extension)).'.'.$file->guessExtension();
+        // Prefer the content-sniffed extension for the stored name so a
+        // rename can't hide the real content type from the filesystem.
+        // Fall back to the client extension when finfo returns nothing,
+        // otherwise the stored filename ends in a bare "." and the
+        // eventual download has no extension.
+        $stored_extension = $file->guessExtension() ?: strtolower($extension);
+        $file_name = $name_prefix.'-'.str_random(8).'-'.str_slug(basename($file->getClientOriginalName(), '.'.$extension)).'.'.$stored_extension;
 
         // Check for SVG and sanitize it
         if ($file->getMimeType() === 'image/svg+xml') {
diff --git a/app/Rules/AllowedUploadExtension.php b/app/Rules/AllowedUploadExtension.php
@@ -0,0 +1,102 @@
+<?php
+
+namespace App\Rules;
+
+use Closure;
+use Illuminate\Contracts\Validation\ValidationRule;
+use Illuminate\Http\UploadedFile;
+
+class AllowedUploadExtension implements ValidationRule
+{
+    /** @param  array<int, string>  $extensions  */
+    public function __construct(private readonly array $extensions) {}
+
+    public function validate(string $attribute, mixed $value, Closure $fail): void
+    {
+        if (! $value instanceof UploadedFile || ! $value->isValid()) {
+            $fail(trans('validation.uploaded', ['attribute' => $attribute]));
+
+            return;
+        }
+
+        // Never let a PHP-executable extension through, even if a caller's
+        // allowlist accidentally names one. Mirrors the guard baked into
+        // Laravel's own `mimes:` rule via shouldBlockPhpUpload, so this rule
+        // stays a safe drop-in replacement.
+        $phpExecutableExtensions = [
+            'php', 'php3', 'php4', 'php5', 'php7', 'php8', 'phtml', 'phar',
+        ];
+
+        $clientExtension = strtolower(trim($value->getClientOriginalExtension()));
+        $allowed = array_map('strtolower', $this->extensions);
+
+        $rejected = trans('validation.mimes', [
+            'attribute' => $attribute,
+            'values' => implode(', ', $allowed),
+        ]);
+
+        if (in_array($clientExtension, $phpExecutableExtensions, true)) {
+            $fail($rejected);
+
+            return;
+        }
+
+        if (! in_array($clientExtension, $allowed, true)) {
+            $fail($rejected);
+
+            return;
+        }
+
+        // Belt against content that finfo confidently identifies as
+        // server-runnable, even when it wouldn't reverse-map to an
+        // extension on the allowlist. Catches the classic webshell
+        // upload (PHP bytes named `shell.jpg`) which otherwise slips
+        // through because Symfony's guesser returns null for text/x-php
+        // and the uninformative-sniff branch below would let it pass.
+        // Native executable formats (PE, ELF, Mach-O) live here as
+        // defense-in-depth. Shebang scripts (shell, python, perl) are
+        // deliberately absent because Snipe-IT does not execute uploads
+        // and script snippets in .txt attachments are legitimate.
+        $executableContentMimes = [
+            'text/x-php',
+            'application/x-httpd-php',
+            'application/x-httpd-php-source',
+            'application/x-executable',
+            'application/x-mach-binary',
+            'application/x-elf',
+            'application/x-sharedlib',
+        ];
+
+        $sniffedMime = strtolower((string) $value->getMimeType());
+
+        if (in_array($sniffedMime, $executableContentMimes, true)) {
+            $fail($rejected);
+
+            return;
+        }
+
+        // Symfony's guessExtension() sniffs the content with finfo and
+        // reverse-maps the detected MIME to an extension. It returns null
+        // when libmagic matches nothing that reverse-maps cleanly (e.g.
+        // INI-shaped plain text). Empty files and unknown binary blobs
+        // sniff to application/x-empty and application/octet-stream,
+        // which reverse-map to 'bin' but carry no real signal. Windows
+        // and other thin magic databases also default to octet-stream
+        // for many everyday files (see issue #10387). Treat all three
+        // as "no evidence against the client extension" and defer to
+        // what the client sent. When the sniff does yield a meaningful
+        // extension it must also be on the allowlist, which still
+        // catches obvious mislabels like an .exe renamed to .txt.
+        $uninformativeMimes = ['application/octet-stream', 'application/x-empty', ''];
+
+        if (in_array($sniffedMime, $uninformativeMimes, true)) {
+            return;
+        }
+
+        $guessed = strtolower(trim((string) $value->guessExtension()));
+
+        if ($guessed !== '' && ! in_array($guessed, $allowed, true)) {
+            $fail($rejected);
+        }
+    }
+}
diff --git a/tests/Feature/FileUploads/UploadFileValidationTest.php b/tests/Feature/FileUploads/UploadFileValidationTest.php
@@ -0,0 +1,149 @@
+<?php
+
+namespace Tests\Feature\FileUploads;
+
+use App\Models\Actionlog;
+use App\Models\Asset;
+use App\Models\License;
+use App\Models\User;
+use Illuminate\Http\UploadedFile;
+use Illuminate\Support\Facades\Storage;
+use PHPUnit\Framework\Attributes\Test;
+use Tests\TestCase;
+
+// Regression coverage for issues #12460 and #10387: legitimate uploads
+// that were rejected because Laravel's built-in `mimes:` rule (and the
+// hand-rolled MIME allowlist in the CSV importer) rely on finfo content
+// sniffing that misidentifies ordinary files. Fake UploadedFiles bypass
+// finfo entirely (their getMimeType() reads MimeType::from($name)), so
+// these tests use real temp files.
+class UploadFileValidationTest extends TestCase
+{
+    private array $tempFiles = [];
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        Storage::fake();
+    }
+
+    protected function tearDown(): void
+    {
+        foreach ($this->tempFiles as $path) {
+            @unlink($path);
+        }
+
+        parent::tearDown();
+    }
+
+    private function realUpload(string $clientName, string $content): UploadedFile
+    {
+        $path = tempnam(sys_get_temp_dir(), 'snipeit_upload_');
+        file_put_contents($path, $content);
+        $this->tempFiles[] = $path;
+
+        return new UploadedFile($path, $clientName, null, null, true);
+    }
+
+    // Issue #12460 TechWilk reproduction: plain-text .txt whose bytes
+    // trigger libmagic's INI heuristic (leading `;`, tab-separated
+    // values). Before the fix, UploadFileRequest's `mimes:txt,...` rule
+    // rejected this because finfo returned application/x-wine-extension-ini
+    // and Symfony's guesser had no reverse mapping to `txt`.
+    #[Test]
+    public function accepts_txt_file_that_libmagic_misidentifies_as_ini(): void
+    {
+        $license = License::factory()->create();
+
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->post(
+                route('api.files.store', ['object_type' => 'licenses', 'id' => $license->id]),
+                ['file' => [$this->realUpload('sample.txt', ";Bob[A]\tSmith[B]\r\n50\t0.8")]]
+            )
+            ->assertOk();
+
+        $log = Actionlog::where('item_id', $license->id)
+            ->where('item_type', License::class)
+            ->where('action_type', 'uploaded')
+            ->latest('id')
+            ->firstOrFail();
+
+        // Stored filename must still carry a .txt extension. Before the
+        // handleFile fallback, guessExtension() returned null on this
+        // input and the stored name ended in a bare "." with no
+        // extension, breaking the eventual download.
+        $this->assertStringEndsWith('.txt', $log->filename);
+    }
+
+    // Issue #12460 primary: empty .txt file. finfo returns
+    // application/x-empty for zero-byte files.
+    #[Test]
+    public function accepts_empty_txt_file(): void
+    {
+        $asset = Asset::factory()->create();
+
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->post(
+                route('api.files.store', ['object_type' => 'assets', 'id' => $asset->id]),
+                ['file' => [$this->realUpload('empty.txt', '')]]
+            )
+            ->assertOk();
+    }
+
+    // The extension allowlist is still authoritative: an .exe rename
+    // must not slip past just because we deferred to the client
+    // extension. Sniff returns application/x-dosexec which reverse-maps
+    // to `exe`, and `exe` is not on the extensions allowlist.
+    #[Test]
+    public function still_rejects_files_whose_extension_is_not_on_the_allowlist(): void
+    {
+        $asset = Asset::factory()->create();
+
+        $peHeader = "MZ\x90\x00\x03\x00\x00\x00\x04\x00\x00\x00\xff\xff\x00\x00";
+
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->post(
+                route('api.files.store', ['object_type' => 'assets', 'id' => $asset->id]),
+                ['file' => [$this->realUpload('installer.exe', $peHeader)]]
+            )
+            ->assertSessionHasErrors('file.0');
+    }
+
+    // Issue #10387: CSV importer used to reject anything whose sniffed
+    // MIME wasn't on a small hand-rolled list. Windows/IIS commonly
+    // sniffs .csv as application/octet-stream because the platform's
+    // magic database is thinner than Linux's. Real CSV content of the
+    // shape below also sniffs as octet-stream on the current test
+    // environment, which is exactly the scenario the reporter hit.
+    #[Test]
+    public function csv_importer_accepts_csv_that_content_sniffs_as_octet_stream(): void
+    {
+        // Leading NULs guarantee finfo returns application/octet-stream,
+        // reproducing the Windows-sniff behavior deterministically.
+        $csv = "\x00\x01\x02header1,header2\nvalue1,value2\n";
+
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->post(
+                route('api.imports.store'),
+                ['files' => [$this->realUpload('inventory.csv', $csv)]]
+            )
+            ->assertOk();
+    }
+
+    // Backstop: a genuine non-CSV file (a PNG here) whose extension is
+    // also not csv/tsv/txt must still be rejected by the importer.
+    #[Test]
+    public function csv_importer_still_rejects_non_csv_extensions(): void
+    {
+        $png = base64_decode(
+            'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII='
+        );
+
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->post(
+                route('api.imports.store'),
+                ['files' => [$this->realUpload('picture.png', $png)]]
+            )
+            ->assertStatus(422);
+    }
+}
diff --git a/tests/Unit/Rules/AllowedUploadExtensionTest.php b/tests/Unit/Rules/AllowedUploadExtensionTest.php
