Use a cryptographically random suffix for attachment file names to make them harder guess

That should not cause much security issues, as this is what users can do anyway via attachment creation, and markdown images

docs/configuration.md

@@ -86,6 +86,9 @@ bundled with Part-DB. Set `DATABASE_MYSQL_SSL_VERIFY_CERT` if you want to accept
 * `ATTACHMENT_DOWNLOAD_BY_DEFAULT`: When this is set to 1, the "download external file" checkbox is checked by default
   when adding a new attachment. Otherwise, it is unchecked by default. Use this if you wanna download all attachments
   locally by default. Attachment download is only possible, when `ALLOW_ATTACHMENT_DOWNLOADS` is set to 1.
+* `ATTACHMENT_SHOW_HTML_FILES`: When enabled, user uploaded HTML attachments can be viewed directly in the browser. 
+  Many potential malicious functions are restricted, still this is a potential security risk and should only be enabled,
+  if you trust the users who can upload files. When set to 0, HTML files are rendered as plain text.
 * `USE_GRAVATAR`: Set to `1` to use [gravatar.com](https://gravatar.com/) images for user avatars (as long as they have
   not set their own picture). The users browsers have to download the pictures from a third-party (gravatar) server, so
   this might be a privacy risk.

src/Controller/AttachmentFileController.php

@@ -30,6 +30,7 @@ use App\Form\Filters\AttachmentFilterType;
 use App\Services\Attachments\AttachmentManager;
 use App\Services\Trees\NodesListBuilder;
 use App\Settings\BehaviorSettings\TableSettings;
+use App\Settings\SystemSettings\AttachmentsSettings;
 use Omines\DataTablesBundle\DataTableFactory;
 use RuntimeException;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
@@ -41,31 +42,56 @@ use Symfony\Component\Routing\Attribute\Route;
 
 class AttachmentFileController extends AbstractController
 {
+
+    public function __construct(private readonly AttachmentManager $helper)
+    {
+
+    }
+
+    #[Route(path: '/attachment/{id}/sandbox', name: 'attachment_html_sandbox')]
+    public function htmlSandbox(Attachment $attachment, AttachmentsSettings $attachmentsSettings): Response
+    {
+        //Check if the sandbox is enabled in the settings, as it can be a security risk if used without proper precautions, so it should be opt-in
+        if (!$attachmentsSettings->showHTMLAttachments) {
+            throw $this->createAccessDeniedException('The HTML sandbox for attachments is disabled in the settings, as it can be a security risk if used without proper precautions. Please enable it in the settings if you want to use it.');
+        }
+
+        $this->checkPermissions($attachment);
+
+        $file_path = $this->helper->toAbsoluteInternalFilePath($attachment);
+
+        $attachmentContent = file_get_contents($file_path);
+
+        $response = $this->render('attachments/html_sandbox.html.twig', [
+            'attachment' => $attachment,
+            'content' => $attachmentContent,
+        ]);
+
+        //Set an CSP that allows to run inline scripts, styles and images from external ressources, but does not allow any connections or others.
+        //Also set the sandbox CSP directive with only "allow-script" to run basic scripts
+        $response->headers->set('Content-Security-Policy', "default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline' *; img-src data: *; sandbox allow-scripts allow-downloads allow-modals;");
+
+        //Forbid to embed the attachment render page in an iframe to prevent clickjacking, as it is not used anywhere else for now
+        $response->headers->set('X-Frame-Options', 'DENY');
+
+        return $response;
+    }
+
     /**
      * Download the selected attachment.
      */
     #[Route(path: '/attachment/{id}/download', name: 'attachment_download')]
-    public function download(Attachment $attachment, AttachmentManager $helper): BinaryFileResponse
+    public function download(Attachment $attachment): BinaryFileResponse
     {
-        $this->denyAccessUnlessGranted('read', $attachment);
+        $this->checkPermissions($attachment);
 
-        if ($attachment->isSecure()) {
-            $this->denyAccessUnlessGranted('show_private', $attachment);
-        }
-
-        if (!$attachment->hasInternal()) {
-            throw $this->createNotFoundException('The file for this attachment is external and not stored locally!');
-        }
-
-        if (!$helper->isInternalFileExisting($attachment)) {
-            throw $this->createNotFoundException('The file associated with the attachment is not existing!');
-        }
-
-        $file_path = $helper->toAbsoluteInternalFilePath($attachment);
+        $file_path = $this->helper->toAbsoluteInternalFilePath($attachment);
         $response = new BinaryFileResponse($file_path);
 
+        $response = $this->forbidHTMLContentType($response);
+
         //Set header content disposition, so that the file will be downloaded
-        $response->setContentDisposition(ResponseHeaderBag::DISPOSITION_ATTACHMENT);
+        $response->setContentDisposition(ResponseHeaderBag::DISPOSITION_ATTACHMENT, $attachment->getFilename());
 
         return $response;
     }
@@ -74,7 +100,35 @@ class AttachmentFileController extends AbstractController
      * View the attachment.
      */
     #[Route(path: '/attachment/{id}/view', name: 'attachment_view')]
-    public function view(Attachment $attachment, AttachmentManager $helper): BinaryFileResponse
+    public function view(Attachment $attachment): BinaryFileResponse
+    {
+        $this->checkPermissions($attachment);
+
+        $file_path = $this->helper->toAbsoluteInternalFilePath($attachment);
+        $response = new BinaryFileResponse($file_path);
+
+        $response = $this->forbidHTMLContentType($response);
+
+        //Set header content disposition, so that the file will be downloaded
+        $response->setContentDisposition(ResponseHeaderBag::DISPOSITION_INLINE, $attachment->getFilename());
+
+        return $response;
+    }
+
+    private function forbidHTMLContentType(BinaryFileResponse $response): BinaryFileResponse
+    {
+        $mimeType = $response->getFile()->getMimeType();
+
+        if ($mimeType === 'text/html') {
+            $mimeType = 'text/plain';
+        }
+
+        $response->headers->set('Content-Type', $mimeType);
+
+        return $response;
+    }
+
+    private function checkPermissions(Attachment $attachment): void
     {
         $this->denyAccessUnlessGranted('read', $attachment);
 
@@ -86,17 +140,9 @@ class AttachmentFileController extends AbstractController
             throw $this->createNotFoundException('The file for this attachment is external and not stored locally!');
         }
 
-        if (!$helper->isInternalFileExisting($attachment)) {
+        if (!$this->helper->isInternalFileExisting($attachment)) {
             throw $this->createNotFoundException('The file associated with the attachment is not existing!');
         }
-
-        $file_path = $helper->toAbsoluteInternalFilePath($attachment);
-        $response = new BinaryFileResponse($file_path);
-
-        //Set header content disposition, so that the file will be downloaded
-        $response->setContentDisposition(ResponseHeaderBag::DISPOSITION_INLINE);
-
-        return $response;
     }
 
     #[Route(path: '/attachment/list', name: 'attachment_list')]

src/Entity/Attachments/Attachment.php

@@ -296,6 +296,22 @@ abstract class Attachment extends AbstractNamedDBElement
         return in_array(strtolower($extension), static::MODEL_EXTS, true);
     }
 
+    /**
+     * Returns true if this is a locally stored HTML file, which can be shown by the sandbox viewer.
+     * This is the case if we have an internal path with a html extension.
+     * @return bool
+     */
+    public function isLocalHTMLFile(): bool
+    {
+        if($this->hasInternal()){
+
+            $extension = pathinfo($this->getFilename(), PATHINFO_EXTENSION);
+
+            return in_array(strtolower($extension), ['html', 'htm'], true);
+        }
+        return false;
+    }
+
     /**
      * Checks if this attachment has a path to an external file
      *

src/Services/Attachments/AttachmentSubmitHandler.php

@@ -137,7 +137,10 @@ class AttachmentSubmitHandler
             $attachment->getName()
         );
 
-        return $safeName.'-'.uniqid('', false).'.'.$extension;
+        // Generate a 12-character URL-safe random string, which should avoid collisions and prevent from guessing file paths.
+        $random = str_replace(['+', '/', '='], ['0', '1', '2'], base64_encode(random_bytes(9)));
+
+        return $safeName.'-'.$random.'.'.$extension;
     }
 
     /**

src/Services/Attachments/AttachmentURLGenerator.php

@@ -22,6 +22,7 @@ declare(strict_types=1);
 
 namespace App\Services\Attachments;
 
+use App\Settings\SystemSettings\AttachmentsSettings;
 use Imagine\Exception\RuntimeException;
 use App\Entity\Attachments\Attachment;
 use InvalidArgumentException;
@@ -40,7 +41,7 @@ class AttachmentURLGenerator
 
     public function __construct(protected Packages $assets, protected AttachmentPathResolver $pathResolver,
                                 protected UrlGeneratorInterface $urlGenerator, protected AttachmentManager $attachmentHelper,
-        protected CacheManager $thumbnailManager, protected LoggerInterface $logger)
+        protected CacheManager $thumbnailManager, protected LoggerInterface $logger, private readonly AttachmentsSettings $attachmentsSettings)
     {
         //Determine a normalized path to the public folder (assets are relative to this folder)
         $this->public_path = $this->pathResolver->parameterToAbsolutePath('public');
@@ -99,6 +100,10 @@ class AttachmentURLGenerator
             return null;
         }
 
+        if ($this->attachmentsSettings->showHTMLAttachments && $attachment->isLocalHTMLFile()) {
+            return $this->urlGenerator->generate('attachment_html_sandbox', ['id' => $attachment->getID()]);
+        }
+
         $asset_path = $this->absolutePathToAssetPath($absolute_path);
         //If path is not relative to public path or marked as secure, serve it via controller
         if (null === $asset_path || $attachment->isSecure()) {

src/Services/InfoProviderSystem/Providers/CanopyProvider.php

@@ -202,6 +202,10 @@ class CanopyProvider implements InfoProviderInterface
         $product = $response->toArray()['data']['amazonProduct'];
 
 
+        if ($product === null) {
+            throw new \RuntimeException("Product with ASIN $id not found");
+        }
+
         return new PartDetailDTO(
             provider_key: $this->getProviderKey(),
             provider_id: $product['asin'],

src/Settings/SystemSettings/AttachmentsSettings.php

@@ -58,4 +58,11 @@ class AttachmentsSettings
         envVar: "bool:ATTACHMENT_DOWNLOAD_BY_DEFAULT", envVarMode: EnvVarMode::OVERWRITE
     )]
     public bool $downloadByDefault = false;
-}
+
+    #[SettingsParameter(
+        label: new TM("settings.system.attachments.showHTMLAttachments"),
+        description: new TM("settings.system.attachments.showHTMLAttachments.help"),
+        envVar: "bool:ATTACHMENT_SHOW_HTML_FILES", envVarMode: EnvVarMode::OVERWRITE
+    )]
+    public bool $showHTMLAttachments = false;
+}

templates/attachments/html_sandbox.html.twig

@@ -0,0 +1,78 @@
+<!DOCTYPE html>
+<html lang="{{ app.request.locale | replace({"_": "-"}) }}"
+<head>
+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
+
+    <link rel="icon" href="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 100 100'><text y='.9em' font-size='90'>️⚠️️</text></svg>">
+
+    {# The content block is already escaped. so we must not escape it again. #}
+    <title>{% trans %}attachment.sandbox.title{% endtrans %}: {{ attachment.filename }}</title>
+
+    <style>
+        /* Reset margins and stop the page from scrolling */
+        body, html {
+            margin: 0;
+            padding: 0;
+            height: 100%;
+            overflow: hidden;
+            font-family: sans-serif;
+        }
+
+        /* The Flex Container */
+        .wrapper {
+            display: flex;
+            flex-direction: column;
+            height: 100vh;
+        }
+
+        /* The Warning Header */
+        .warning-bar {
+            background-color: #ff4d4d;
+            color: white;
+            padding: 10px 20px;
+            text-align: center;
+            box-shadow: 0 2px 5px rgba(0,0,0,0.2);
+            z-index: 10; /* Keep it above the iframe */
+        }
+
+        /* The Iframe: The 'flex: 1' makes it fill all remaining space */
+        .content-frame {
+            flex: 1;
+            border: none;
+            width: 100%;
+        }
+    </style>
+</head>
+<body>
+{% block body %}
+    {# We have a fullscreen iframe, with an warning on top #}
+
+    <div class="wrapper">
+
+        <header>
+            <header class="warning-bar">
+                <b>⚠️ {% trans%}attachment.sandbox.warning{% endtrans %}</b>
+
+                <br>
+                <small>
+                    {% trans%}[Attachment]{% endtrans%}: <b>{{ attachment.name }}</b> /  <b>{{ attachment.filename ?? "" }}</b> ({% trans%}id.label{% endtrans %}: {{ attachment.id }})
+                    <a href="{{ path("attachment_view", {id: attachment.id}) }}" style="color: white; margin-left: 15px;">{% trans%}attachment.sandbox.as_plain_text{% endtrans %}</a>
+                    <a href="{{ path("homepage") }}" style="color: white; margin-left: 15px;">{% trans%}attachment.sandbox.back_to_partdb{% endtrans %}</a>
+                </small>
+            </header>
+        </header>
+
+        <iframe  referrerpolicy="no-referrer" class="content-frame"
+                 {# When changing this sandbox, also change the sandbox CSP in the controller #}
+                 sandbox="allow-scripts allow-downloads allow-modals"
+                 srcdoc="{{ content|e('html_attr') }}"
+        ></iframe>
+
+
+    </div>
+
+{% endblock %}
+
+</body>
+</html>

tests/Entity/Attachments/AttachmentTest.php

@@ -279,4 +279,32 @@ final class AttachmentTest extends TestCase
         $reflection_property->setAccessible(true);
         $reflection_property->setValue($object, $value);
     }
+
+    public function testIsLocalHTMLFile(): void
+    {
+        $attachment = new PartAttachment();
+
+        $attachment->setExternalPath('https://google.de');
+        $this->assertFalse($attachment->isLocalHTMLFile());
+
+        $attachment->setExternalPath('https://google.de/test.html');
+        $this->assertFalse($attachment->isLocalHTMLFile());
+
+        $attachment->setInternalPath('%MEDIA%/test.html');
+        $this->assertTrue($attachment->isLocalHTMLFile());
+
+        $attachment->setInternalPath('%MEDIA%/test.htm');
+        $this->assertTrue($attachment->isLocalHTMLFile());
+
+        $attachment->setInternalPath('%MEDIA%/test.txt');
+        $this->assertFalse($attachment->isLocalHTMLFile());
+
+        //It works however, if the file is stored as txt, and the internal filename ends with .html
+        $attachment->setInternalPath('%MEDIA%/test.txt');
+        $this->setProtectedProperty($attachment, 'original_filename', 'test.html');
+        $this->assertTrue($attachment->isLocalHTMLFile());
+
+        $this->setProtectedProperty($attachment, 'original_filename', 'test.htm');
+        $this->assertTrue($attachment->isLocalHTMLFile());
+    }
 }

translations/messages.en.xlf

@@ -12593,5 +12593,41 @@ Buerklin-API Authentication server:
         <target>When selected, more details will be fetched from canopy when creating a part. This causes an additional API request, but gives product bullet points and category info.</target>
       </segment>
     </unit>
+    <unit id="D055xh8" name="attachment.sandbox.warning">
+      <segment>
+        <source>attachment.sandbox.warning</source>
+        <target>WARNING: You are viewing an user uploaded attachment. This is untrusted content. Proceed with care.</target>
+      </segment>
+    </unit>
+    <unit id="bRcdnJK" name="attachment.sandbox.back_to_partdb">
+      <segment>
+        <source>attachment.sandbox.back_to_partdb</source>
+        <target>Back to Part-DB</target>
+      </segment>
+    </unit>
+    <unit id="MzyA7N8" name="settings.system.attachments.showHTMLAttachments">
+      <segment>
+        <source>settings.system.attachments.showHTMLAttachments</source>
+        <target>Show uploaded HTML file attachments (sandboxed)</target>
+      </segment>
+    </unit>
+    <unit id="V_LJkRy" name="settings.system.attachments.showHTMLAttachments.help">
+      <segment>
+        <source>settings.system.attachments.showHTMLAttachments.help</source>
+        <target>⚠️ When enabled, user uploaded HTML attachments can be viewed directly in the browser. Many potential malicious functions are restricted, still this is a potential security risk and should only be enabled, if you trust the users who can upload files.</target>
+      </segment>
+    </unit>
+    <unit id="BQo2xWi" name="attachment.sandbox.title">
+      <segment>
+        <source>attachment.sandbox.title</source>
+        <target>HTML [Attachment]</target>
+      </segment>
+    </unit>
+    <unit id="sJ6v9uJ" name="attachment.sandbox.as_plain_text">
+      <segment>
+        <source>attachment.sandbox.as_plain_text</source>
+        <target>View as plain text</target>
+      </segment>
+    </unit>
   </file>
 </xliff>