From 624b7911e5694f15ed9c6b7070f138d79cd7c59e Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Tue, 1 Sep 2026 07:56:38 -0700 Subject: [PATCH] Merge commit from fork --- src/documents/consumer.py | 9 +++ src/documents/file_handling.py | 29 +++++++ src/documents/templating/filepath.py | 4 +- src/documents/tests/test_consumer.py | 23 ++++++ src/documents/tests/test_file_handling.py | 97 +++++++++++++++++++++++ 5 files changed, 160 insertions(+), 2 deletions(-) diff --git a/src/documents/consumer.py b/src/documents/consumer.py index 79d0cfa8b..edb48c348 100644 --- a/src/documents/consumer.py +++ b/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, diff --git a/src/documents/file_handling.py b/src/documents/file_handling.py index 6bc202733..aeed27b01 100644 --- a/src/documents/file_handling.py +++ b/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 diff --git a/src/documents/templating/filepath.py b/src/documents/templating/filepath.py index d827b7c89..a4de6b274 100644 --- a/src/documents/templating/filepath.py +++ b/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).", ) diff --git a/src/documents/tests/test_consumer.py b/src/documents/tests/test_consumer.py index 69c5afb9d..7c98f3552 100644 --- a/src/documents/tests/test_consumer.py +++ b/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): diff --git a/src/documents/tests/test_file_handling.py b/src/documents/tests/test_file_handling.py index 0a4c13cd2..081ffdcbd 100644 --- a/src/documents/tests/test_file_handling.py +++ b/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: