Merge commit from fork

src/documents/consumer.py

@@ -25,6 +25,7 @@ from documents.data_models import DocumentMetadataOverrides
 from documents.file_handling import create_source_path_directory
 from documents.file_handling import generate_filename
 from documents.file_handling import generate_unique_filename
+from documents.file_handling import validate_path_in_root
 from documents.loggers import LoggingMixin
 from documents.models import Correspondent
 from documents.models import CustomField
@@ -695,6 +696,10 @@ class ConsumerPlugin(
                                     use_format=False,
                                 )
                             document.filename = generated_filename
+                            validate_path_in_root(
+                                document.source_path,
+                                settings.ORIGINALS_DIR,
+                            )
                             create_source_path_directory(document.source_path)
 
                             self._write(
@@ -727,6 +732,10 @@ class ConsumerPlugin(
                                         use_format=False,
                                     )
                                 document.archive_filename = generated_archive_filename
+                                validate_path_in_root(
+                                    document.archive_path,
+                                    settings.ARCHIVE_DIR,
+                                )
                                 create_source_path_directory(document.archive_path)
                                 self._write(
                                     archive_path,

src/documents/file_handling.py

@@ -1,12 +1,33 @@
+import logging
 import os
 from pathlib import Path
 
 from django.conf import settings
 
 from documents.models import Document
+from documents.templating.filepath import is_safe_relative_path
 from documents.templating.filepath import validate_filepath_template_and_render
 from documents.templating.utils import convert_format_str_to_template_format
 
+logger = logging.getLogger("paperless.filehandling")
+
+
+class UnsafeFilePathError(Exception):
+    """
+    Raised when a path generated for a document would land outside of its root.
+    """
+
+
+def validate_path_in_root(path: Path, root: Path) -> None:
+    """
+    Ensures the given absolute path is contained within root, the
+    equivalent guard for the later move.
+    """
+    if not path.resolve().is_relative_to(root.resolve()):
+        msg = f"Refusing to write file outside of root {root}: {path}."
+        logger.warning(msg)
+        raise UnsafeFilePathError(msg)
+
 
 def create_source_path_directory(source_path: Path) -> None:
     source_path.parent.mkdir(parents=True, exist_ok=True)
@@ -121,6 +142,14 @@ def format_filename(document: Document, template_str: str) -> str | None:
         "none",
     )  # backward compatibility
 
+    # Validate again after remove none
+    if not is_safe_relative_path(rendered_filename):
+        logger.warning(
+            "Filename became unsafe after placeholder removal, "
+            "falling back to default naming",
+        )
+        return None
+
     return rendered_filename
 
 

src/documents/templating/filepath.py

@@ -340,7 +340,7 @@ def get_custom_fields_context(
     return field_data
 
 
-def _is_safe_relative_path(value: str) -> bool:
+def is_safe_relative_path(value: str) -> bool:
     if value == "":
         return True
 
@@ -398,7 +398,7 @@ def validate_filepath_template_and_render(
         )
         rendered_template = template.render(context)
 
-        if not _is_safe_relative_path(rendered_template):
+        if not is_safe_relative_path(rendered_template):
             logger.warning(
                 "Template rendered an unsafe path (absolute or containing traversal).",
             )

src/documents/tests/test_consumer.py

@@ -1179,6 +1179,29 @@ class TestConsumer(
                 produce_archive=True,
             )
 
+    @mock.patch("documents.consumer.generate_unique_filename")
+    def test_consume_refuses_to_write_outside_originals_dir(
+        self,
+        m: mock.Mock,
+    ) -> None:
+        """
+        GIVEN:
+            - Filename generation produces a path outside of the originals directory
+        WHEN:
+            - The document is consumed
+        THEN:
+            - The consumption fails and no file is written outside of the root
+        """
+        m.return_value = Path("../../pwned.pdf")
+        escaped = (settings.ORIGINALS_DIR / ".." / ".." / "pwned.pdf").resolve()
+
+        with self.get_consumer(self.get_test_file()) as consumer:
+            with self.assertRaises(ConsumerError):
+                consumer.run()
+
+        self.assertIsNotFile(escaped)
+        self.assertEqual(Document.objects.count(), 0)
+
 
 @mock.patch("documents.consumer.magic.from_file", fake_magic_from_file)
 class TestConsumerCreatedDate(DirectoriesMixin, GetConsumerMixin, TestCase):

src/documents/tests/test_file_handling.py

@@ -16,10 +16,12 @@ from django.test import override_settings
 from django.test.utils import CaptureQueriesContext
 from django.utils import timezone
 
+from documents.file_handling import UnsafeFilePathError
 from documents.file_handling import create_source_path_directory
 from documents.file_handling import delete_empty_directories
 from documents.file_handling import generate_filename
 from documents.file_handling import generate_unique_filename
+from documents.file_handling import validate_path_in_root
 from documents.models import Correspondent
 from documents.models import CustomField
 from documents.models import CustomFieldInstance
@@ -1511,6 +1513,101 @@ class TestFilenameGeneration(DirectoriesMixin, TestCase):
         document.filename = generate_filename(document)
         self.assertEqual(document.filename, Path("XX/doc1.pdf"))
 
+    @override_settings(FILENAME_FORMAT_REMOVE_NONE=True)
+    def test_remove_none_cannot_create_traversal(self) -> None:
+        """
+        GIVEN:
+            - A storage path whose components are safe when validated, but become
+              ".." once the -none- placeholder is stripped out
+            - FILENAME_FORMAT_REMOVE_NONE is True
+        WHEN:
+            - the filename is generated for the document
+        THEN:
+            - The unsafe filename is rejected and the default naming is used
+        """
+        sp = StoragePath.objects.create(
+            name="sp1",
+            path=".-none-./.-none-./tmp/pwned",
+        )
+        document = Document.objects.create(
+            title="doc1",
+            mime_type="application/pdf",
+            storage_path=sp,
+        )
+
+        filename = generate_filename(document)
+
+        self.assertNotIn("..", filename.parts)
+        self.assertEqual(filename, Path(f"{document.pk:07}.pdf"))
+
+    @override_settings(FILENAME_FORMAT_REMOVE_NONE=True)
+    def test_remove_none_still_removes_placeholder(self) -> None:
+        """
+        GIVEN:
+            - A storage path with a placeholder for a value the document does not have
+            - FILENAME_FORMAT_REMOVE_NONE is True
+        WHEN:
+            - the filename is generated for the document
+        THEN:
+            - The placeholder is still removed as before
+        """
+        sp = StoragePath.objects.create(
+            name="sp1",
+            path="{{ correspondent }}/{{ title }}",
+        )
+        document = Document.objects.create(
+            title="doc1",
+            mime_type="application/pdf",
+            storage_path=sp,
+        )
+
+        self.assertEqual(generate_filename(document), Path("doc1.pdf"))
+
+    @override_settings(
+        FILENAME_FORMAT="{{ correspondent }}/{{ title }}/{{ doc_pk }}",
+        FILENAME_FORMAT_REMOVE_NONE=True,
+    )
+    def test_remove_none_cannot_create_traversal_from_metadata(self) -> None:
+        """
+        GIVEN:
+            - A global filename format with directory components
+            - A document whose title becomes ".." once -none- is stripped out
+            - FILENAME_FORMAT_REMOVE_NONE is True
+        WHEN:
+            - the filename is generated for the document
+        THEN:
+            - The unsafe filename is rejected and the default naming is used
+        """
+        document = Document.objects.create(
+            title=".-none-.",
+            mime_type="application/pdf",
+        )
+
+        filename = generate_filename(document)
+
+        self.assertNotIn("..", filename.parts)
+        self.assertEqual(filename, Path(f"{document.pk:07}.pdf"))
+
+    def test_validate_path_in_root(self) -> None:
+        """
+        GIVEN:
+            - A path inside of the root and a path outside of it
+        WHEN:
+            - The path is validated against the root
+        THEN:
+            - Only the path outside of the root is rejected
+        """
+        validate_path_in_root(
+            settings.ORIGINALS_DIR / "0000001.pdf",
+            settings.ORIGINALS_DIR,
+        )
+
+        with self.assertRaises(UnsafeFilePathError):
+            validate_path_in_root(
+                (settings.ORIGINALS_DIR / ".." / ".." / "pwned.pdf"),
+                settings.ORIGINALS_DIR,
+            )
+
     def test_complex_template_strings(self) -> None:
         """
         GIVEN: