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

Files whose sniff yields nothing usable (empty, octet-stream, INI-shaped) now pass.

app/Helpers/Helper.php

@@ -1404,6 +1404,7 @@ class Helper
             '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 @@ class Helper
             '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',

app/Http/Controllers/Api/ImportController.php

@@ -57,14 +57,28 @@ class ImportController extends Controller
             $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);

app/Http/Requests/UploadFileRequest.php

@@ -4,6 +4,7 @@ namespace App\Http\Requests;
 
 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 @@ class UploadFileRequest extends Request
      */
     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 @@ class UploadFileRequest extends Request
     {
 
         $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') {

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);
+        }
+    }
+}

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);
+    }
+}

tests/Unit/Rules/AllowedUploadExtensionTest.php

@@ -0,0 +1,166 @@
+<?php
+
+namespace Tests\Unit\Rules;
+
+use App\Rules\AllowedUploadExtension;
+use Illuminate\Http\UploadedFile;
+use Illuminate\Support\Facades\Validator;
+use PHPUnit\Framework\Attributes\Test;
+use Tests\TestCase;
+
+class AllowedUploadExtensionTest extends TestCase
+{
+    private array $tempFiles = [];
+
+    protected function tearDown(): void
+    {
+        foreach ($this->tempFiles as $path) {
+            @unlink($path);
+        }
+
+        parent::tearDown();
+    }
+
+    // Real UploadedFile pointed at a real temp file. UploadedFile::fake()
+    // bypasses finfo (its getMimeType() reads MimeType::from($name), which
+    // is extension-only), so it can't reproduce the sniff-vs-extension
+    // mismatch this rule exists to tolerate.
+    private function realUpload(string $clientName, string $content): UploadedFile
+    {
+        $path = tempnam(sys_get_temp_dir(), 'snipeit_rule_');
+        file_put_contents($path, $content);
+        $this->tempFiles[] = $path;
+
+        return new UploadedFile($path, $clientName, null, null, true);
+    }
+
+    private function passes(UploadedFile $file, array $extensions = ['txt', 'csv', 'jpg', 'pdf']): bool
+    {
+        return Validator::make(
+            ['file' => $file],
+            ['file' => [new AllowedUploadExtension($extensions)]],
+        )->passes();
+    }
+
+    #[Test]
+    public function accepts_plain_text_file_with_matching_extension(): void
+    {
+        $this->assertTrue($this->passes(
+            $this->realUpload('notes.txt', "hello world\nmore text\n"),
+        ));
+    }
+
+    // Issue #12460: empty text upload. finfo returns application/x-empty,
+    // which does not reverse-map to any extension. Laravel's `mimes:txt`
+    // would reject; this rule accepts the client-supplied extension.
+    #[Test]
+    public function accepts_empty_txt_file(): void
+    {
+        $this->assertTrue($this->passes(
+            $this->realUpload('empty.txt', ''),
+        ));
+    }
+
+    // Issue #12460 (TechWilk repro): plain text whose first byte is `;`
+    // and whose fields are tab-separated matches libmagic's INI heuristic.
+    // finfo returns application/x-wine-extension-ini; guessExtension()
+    // returns null. The rule should still accept it because the client
+    // extension is on the allowlist.
+    #[Test]
+    public function accepts_txt_file_that_libmagic_misidentifies_as_ini(): void
+    {
+        $this->assertTrue($this->passes(
+            $this->realUpload('sample.txt', ";Bob[A]\tSmith[B]\r\n50\t0.8"),
+        ));
+    }
+
+    // Issue #10387: CSVs whose sniffed MIME is unhelpful (Windows/IIS
+    // finfo commonly returns application/octet-stream) or that trip a
+    // non-CSV magic signature. The UploadFileRequest path should still
+    // accept those when the extension is on the allowlist.
+    #[Test]
+    public function accepts_csv_when_content_sniff_yields_octet_stream(): void
+    {
+        $this->assertTrue($this->passes(
+            $this->realUpload('inventory.csv', "\x00\x01\x02random,binary,bytes\n"),
+            ['csv', 'txt'],
+        ));
+    }
+
+    // The extension-check backstop still catches the classic mislabel:
+    // a PNG (well-known magic bytes) renamed to .txt. Client extension
+    // passes, but guessExtension() returns 'png' and 'png' is not in the
+    // allowlist, so the rule rejects.
+    #[Test]
+    public function rejects_binary_file_whose_sniff_disagrees_with_allowlist(): void
+    {
+        $png = base64_decode(
+            'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII='
+        );
+
+        $this->assertFalse($this->passes(
+            $this->realUpload('sneaky.txt', $png),
+            ['txt', 'csv'], // png deliberately absent
+        ));
+    }
+
+    #[Test]
+    public function rejects_extension_not_on_allowlist(): void
+    {
+        $this->assertFalse($this->passes(
+            $this->realUpload('installer.exe', "MZ\x90\x00"),
+        ));
+    }
+
+    #[Test]
+    public function rejects_php_executable_extension_even_if_allowlisted(): void
+    {
+        // Even a maliciously-permissive caller can't punch a PHP file
+        // through. This mirrors Laravel's own shouldBlockPhpUpload guard.
+        $this->assertFalse($this->passes(
+            $this->realUpload('shell.php', "<?php system(\$_GET['c']);"),
+            ['txt', 'php'],
+        ));
+    }
+
+    // The webshell case: PHP source dressed up as an image. finfo sniffs
+    // to text/x-php, which has no reverse map in Symfony's guesser (so the
+    // sniff crosscheck can't catch it), and my "uninformative sniff"
+    // fallback would otherwise let it through. The explicit executable-MIME
+    // belt catches it.
+    #[Test]
+    public function rejects_php_content_disguised_as_an_image(): void
+    {
+        $this->assertFalse($this->passes(
+            $this->realUpload('shell.jpg', "<?php system(\$_GET['c']); ?>"),
+        ));
+    }
+
+    #[Test]
+    public function rejects_php_content_disguised_as_text(): void
+    {
+        $this->assertFalse($this->passes(
+            $this->realUpload('notes.txt', "<?php echo 'hi'; ?>"),
+        ));
+    }
+
+    // Shebang scripts stay allowed. Snipe-IT does not execute uploads and
+    // legitimate script snippets end up in .txt support-ticket attachments
+    // often enough that rejecting them is user-hostile.
+    #[Test]
+    public function accepts_shell_script_content_in_txt(): void
+    {
+        $this->assertTrue($this->passes(
+            $this->realUpload('snippet.txt', "#!/bin/bash\necho hi\n"),
+        ));
+    }
+
+    #[Test]
+    public function rejects_non_file_values(): void
+    {
+        $this->assertFalse(Validator::make(
+            ['file' => 'not-a-file'],
+            ['file' => [new AllowedUploadExtension(['txt'])]],
+        )->passes());
+    }
+}