File uploads: Fixed FD-56333 - tighten mime-types for inline display

app/Helpers/StorageHelper.php

@@ -14,9 +14,18 @@ class StorageHelper
         if ($disk == 'default') {
             $disk = config('filesystems.default');
         }
+
+        // Neutralize the response so a browser can't be tricked into treating
+        // an uploaded file as active content: force a generic content type,
+        // stop MIME sniffing, and keep the attachment disposition.
+        $safeHeaders = [
+            'Content-Type' => 'application/octet-stream',
+            'X-Content-Type-Options' => 'nosniff',
+        ];
+
         switch (config("filesystems.disks.$disk.driver")) {
             case 'local':
-                return response()->download(Storage::disk($disk)->path($filename)); // works for PRIVATE or public?!
+                return response()->download(Storage::disk($disk)->path($filename), null, $safeHeaders);
 
             case 's3':
                 Storage::disk($disk)->temporaryUrl(
@@ -29,7 +38,7 @@ class StorageHelper
                 );
 
             default:
-                return Storage::disk($disk)->download($filename);
+                return Storage::disk($disk)->download($filename, null, $safeHeaders);
         }
     }
 
@@ -87,31 +96,29 @@ class StorageHelper
      */
     public static function allowSafeInline($file_with_path)
     {
+        // Extension is the coarse gate; the server-detected MIME must also
+        // land in the extension's allowed set (config/filesystems.php →
+        // allowed_inline_display), so a .png that is actually XML/HTML/XSLT
+        // can't ride the extension check into an inline response.
+        $allowed_inline = config('filesystems.allowed_inline_display', []);
 
-        $allowed_inline = [
-            'avif',
-            'gif',
-            'gif',
-            'jpg',
-            'mov',
-            'mp3',
-            'mp4',
-            'ogg',
-            'pdf',
-            'png',
-            'svg',
-            'wav',
-            'webm',
-            'webp',
-        ];
-
-        // The file exists and is allowed to be displayed inline
-        if (Storage::exists($file_with_path) && (in_array(pathinfo($file_with_path, PATHINFO_EXTENSION), $allowed_inline))) {
-            return true;
+        if (! Storage::exists($file_with_path)) {
+            return false;
         }
 
-        return false;
+        $extension = strtolower(pathinfo($file_with_path, PATHINFO_EXTENSION));
 
+        if (! isset($allowed_inline[$extension])) {
+            return false;
+        }
+
+        try {
+            $detected = Storage::mimeType($file_with_path);
+        } catch (\Throwable) {
+            return false;
+        }
+
+        return $detected && in_array($detected, $allowed_inline[$extension], true);
     }
 
     public static function getFiletype($file_with_path)

app/Http/Controllers/Api/UploadedFilesController.php

@@ -158,11 +158,19 @@ class UploadedFilesController extends Controller
         }
 
         if (request('inline') == 'true') {
-            $headers = [
-                'Content-Disposition' => 'inline',
-            ];
+            $path = self::$map_storage_path[$object_type];
 
-            return Storage::download(self::$map_storage_path[$object_type].$log->filename, $log->filename, $headers);
+            // Only allowlisted extensions may be served inline. Everything
+            // else (including XML, which can pull an XSLT stylesheet and
+            // execute script in-origin) falls through to a download response.
+            if (! StorageHelper::allowSafeInline($path.$log->filename)) {
+                return StorageHelper::downloader($path.$log->filename);
+            }
+
+            return Storage::download($path.$log->filename, $log->filename, [
+                'Content-Disposition' => 'inline',
+                'X-Content-Type-Options' => 'nosniff',
+            ]);
         }
 
         return StorageHelper::downloader(self::$map_storage_path[$object_type].$log->filename);

config/filesystems.php

@@ -102,7 +102,7 @@ $config = [
             'secret' => env('PRIVATE_AWS_SECRET_ACCESS_KEY'),
             'region' => env('PRIVATE_AWS_DEFAULT_REGION'),
             'bucket' => env('PRIVATE_AWS_BUCKET'),
-            'root'   => env('BACKUP_FILESYSTEM_ROOT', storage_path('app')),
+            'root' => env('BACKUP_FILESYSTEM_ROOT', storage_path('app')),
             'visibility' => 'private',
         ],
 
@@ -126,8 +126,6 @@ if (env('PUBLIC_S3_PROXY', false)) {
 $config['allowed_upload_extensions_array'] = [
     'avif',
     'doc',
-    'doc',
-    'docx',
     'docx',
     'gif',
     'ico',
@@ -184,4 +182,28 @@ $config['allowed_upload_mimetypes'] = implode(',', $config['allowed_upload_mimet
 $config['allowed_upload_extensions_for_validator'] = implode(',', $config['allowed_upload_extensions_array']);
 $config['allowed_upload_extensions'] = '.'.implode(', .', $config['allowed_upload_extensions_array']);
 
+// Strict subset of the upload allowlist that is safe to return with
+// Content-Disposition: inline. The extension gates entry; the server-detected
+// MIME must also match one of the values below before StorageHelper::allowSafeInline
+// hands the response back inline. Anything else falls through to an attachment
+// response so an uploaded XML/HTML/XSLT can't be rendered as active content in the
+// Snipe-IT origin. Keep this list narrower than allowed_upload_extensions_array on
+// purpose: we accept broader file types for storage than we're willing to render.
+$config['allowed_inline_display'] = [
+    'avif' => ['image/avif'],
+    'gif' => ['image/gif'],
+    'jpg' => ['image/jpeg'],
+    'jpeg' => ['image/jpeg'],
+    'mov' => ['video/quicktime'],
+    'mp3' => ['audio/mpeg', 'audio/mp3'],
+    'mp4' => ['video/mp4'],
+    'ogg' => ['audio/ogg', 'video/ogg', 'application/ogg'],
+    'pdf' => ['application/pdf'],
+    'png' => ['image/png'],
+    'svg' => ['image/svg+xml'],
+    'wav' => ['audio/wav', 'audio/x-wav', 'audio/wave', 'audio/vnd.wave'],
+    'webm' => ['video/webm', 'audio/webm'],
+    'webp' => ['image/webp'],
+];
+
 return $config;

tests/Feature/Assets/Api/UploadedFilesInlineTest.php

@@ -0,0 +1,119 @@
+<?php
+
+namespace Tests\Feature\Assets\Api;
+
+use App\Models\Actionlog;
+use App\Models\Asset;
+use App\Models\User;
+use Illuminate\Support\Facades\Storage;
+use Tests\TestCase;
+
+class UploadedFilesInlineTest extends TestCase
+{
+    private User $user;
+
+    private Asset $asset;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        Storage::fake();
+        $this->user = User::factory()->superuser()->create();
+        $this->asset = Asset::factory()->create();
+    }
+
+    private function seedUpload(string $filename, string $contents): void
+    {
+        Storage::put('private_uploads/assets/'.$filename, $contents);
+
+        $log = new Actionlog;
+        $log->item_id = $this->asset->id;
+        $log->item_type = Asset::class;
+        $log->action_type = 'uploaded';
+        $log->filename = $filename;
+        $log->created_by = $this->user->id;
+        $log->save();
+    }
+
+    public function test_xml_upload_requested_inline_is_forced_to_attachment()
+    {
+        $this->seedUpload('malicious.xml', '<?xml version="1.0"?><data>test</data>');
+
+        $log = Actionlog::where('filename', 'malicious.xml')->firstOrFail();
+
+        $response = $this->actingAsForApi($this->user)
+            ->get(route('api.files.show', [
+                'object_type' => 'assets',
+                'id' => $this->asset->id,
+                'file_id' => $log->id,
+                'inline' => 'true',
+            ]))
+            ->assertOk();
+
+        $disposition = $response->headers->get('Content-Disposition');
+        $this->assertStringStartsWith('attachment', (string) $disposition,
+            'XML files must never be served inline via the API — attacker-controlled xml-stylesheet can execute script in-origin.');
+    }
+
+    public function test_html_upload_requested_inline_is_forced_to_attachment()
+    {
+        $this->seedUpload('page.html', '<html><body><script>alert(1)</script></body></html>');
+
+        $log = Actionlog::where('filename', 'page.html')->firstOrFail();
+
+        $response = $this->actingAsForApi($this->user)
+            ->get(route('api.files.show', [
+                'object_type' => 'assets',
+                'id' => $this->asset->id,
+                'file_id' => $log->id,
+                'inline' => 'true',
+            ]))
+            ->assertOk();
+
+        $disposition = $response->headers->get('Content-Disposition');
+        $this->assertStringStartsWith('attachment', (string) $disposition);
+    }
+
+    public function test_png_upload_requested_inline_is_served_inline_with_nosniff()
+    {
+        // 1x1 transparent PNG so Storage::mimeType() detects image/png.
+        $png = base64_decode('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYAAAAAYAAjCB0C8AAAAASUVORK5CYII=');
+        $this->seedUpload('photo.png', $png);
+
+        $log = Actionlog::where('filename', 'photo.png')->firstOrFail();
+
+        $response = $this->actingAsForApi($this->user)
+            ->get(route('api.files.show', [
+                'object_type' => 'assets',
+                'id' => $this->asset->id,
+                'file_id' => $log->id,
+                'inline' => 'true',
+            ]))
+            ->assertOk();
+
+        $disposition = $response->headers->get('Content-Disposition');
+        $this->assertStringStartsWith('inline', (string) $disposition);
+        $this->assertSame('nosniff', $response->headers->get('X-Content-Type-Options'));
+    }
+
+    public function test_xml_disguised_as_png_extension_is_forced_to_attachment()
+    {
+        // Extension says png, contents are XML. The MIME cross-check in
+        // allowSafeInline must catch this and block the inline response.
+        $this->seedUpload('bait.png', '<?xml version="1.0"?><data>test</data>');
+
+        $log = Actionlog::where('filename', 'bait.png')->firstOrFail();
+
+        $response = $this->actingAsForApi($this->user)
+            ->get(route('api.files.show', [
+                'object_type' => 'assets',
+                'id' => $this->asset->id,
+                'file_id' => $log->id,
+                'inline' => 'true',
+            ]))
+            ->assertOk();
+
+        $disposition = $response->headers->get('Content-Disposition');
+        $this->assertStringStartsWith('attachment', (string) $disposition);
+    }
+}