diff --git a/app/Helpers/Helper.php b/app/Helpers/Helper.php index ea62380400..2c2818b9ac 100644 --- a/app/Helpers/Helper.php +++ b/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', diff --git a/app/Http/Controllers/Api/ImportController.php b/app/Http/Controllers/Api/ImportController.php index 12df62f557..8b5541d129 100644 --- a/app/Http/Controllers/Api/ImportController.php +++ b/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); diff --git a/app/Http/Requests/UploadFileRequest.php b/app/Http/Requests/UploadFileRequest.php index e647b0307c..6fc40feb18 100644 --- a/app/Http/Requests/UploadFileRequest.php +++ b/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') { diff --git a/app/Rules/AllowedUploadExtension.php b/app/Rules/AllowedUploadExtension.php new file mode 100644 index 0000000000..17535789f0 --- /dev/null +++ b/app/Rules/AllowedUploadExtension.php @@ -0,0 +1,102 @@ + $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 new file mode 100644 index 0000000000..4b0406878f --- /dev/null +++ b/tests/Feature/FileUploads/UploadFileValidationTest.php @@ -0,0 +1,149 @@ +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 new file mode 100644 index 0000000000..6801b314d8 --- /dev/null +++ b/tests/Unit/Rules/AllowedUploadExtensionTest.php @@ -0,0 +1,166 @@ +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', "assertFalse($this->passes( + $this->realUpload('shell.jpg', ""), + )); + } + + #[Test] + public function rejects_php_content_disguised_as_text(): void + { + $this->assertFalse($this->passes( + $this->realUpload('notes.txt', ""), + )); + } + + // 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()); + } +}