From 1ff18656b7351dcf61993b5d70762c544b9d8ef2 Mon Sep 17 00:00:00 2001 From: Trenton Holmes <797416+stumpylog@users.noreply.github.com> Date: Sun, 13 Sep 2026 14:46:52 -0700 Subject: [PATCH] Fix: type edit_pdf operations via a nested serializer operations was a plain ListField(required=True) with no child=, so each element was untyped, and both EditPdfDocumentsSerializer.validate and BulkEditSerializer._validate_parameters_edit_pdf hand-checked page/rotate/doc with isinstance(). This let a negative doc index through silently (used as a wrapping Python list index instead of being rejected), an empty operations list crashed `max(op.get("doc", 0) for op in operations)` with update_document=True, and an out-of-range doc index could drive pikepdf.new() to allocate an unbounded number of objects. Added PdfEditOperationSerializer (page: IntegerField(min_value=1), rotate/doc: IntegerField, doc: min_value=0) and used it as ListField(child=..., allow_empty=False) in both places, plus a doc-index bound of len(operations) shared by both validate() methods. Co-Authored-By: Claude Sonnet 5 --- src/documents/serialisers.py | 56 +++++----- src/documents/tests/test_api_bulk_edit.py | 130 +++++++++++++++++++++- 2 files changed, 154 insertions(+), 32 deletions(-) diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index 8afa2c53d..6f9e4a48f 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -1749,8 +1749,18 @@ class MergeDocumentsAsVersionsSerializer(DocumentListSerializer): return attrs +class PdfEditOperationSerializer(serializers.Serializer[dict[str, int]]): + page = serializers.IntegerField(min_value=1) + rotate = serializers.IntegerField(required=False) + doc = serializers.IntegerField(required=False, min_value=0) + + class EditPdfDocumentsSerializer(DocumentListSerializer, SourceModeValidationMixin): - operations = serializers.ListField(required=True) + operations = serializers.ListField( + child=PdfEditOperationSerializer(), + required=True, + allow_empty=False, + ) delete_original = serializers.BooleanField(required=False, default=False) update_document = serializers.BooleanField(required=False, default=False) include_metadata = serializers.BooleanField(required=False, default=True) @@ -1768,18 +1778,9 @@ class EditPdfDocumentsSerializer(DocumentListSerializer, SourceModeValidationMix ) operations = attrs["operations"] - if not isinstance(operations, list): - raise serializers.ValidationError("operations must be a list") - for op in operations: - if not isinstance(op, dict): - raise serializers.ValidationError("invalid operation entry") - if "page" not in op or not isinstance(op["page"], int): - raise serializers.ValidationError("page must be an integer") - if "rotate" in op and not isinstance(op["rotate"], int): - raise serializers.ValidationError("rotate must be an integer") - if "doc" in op and not isinstance(op["doc"], int): - raise serializers.ValidationError("doc must be an integer") + if any(op.get("doc", 0) >= len(operations) for op in operations): + raise serializers.ValidationError("doc index is out of bounds") if attrs["update_document"]: max_idx = max(op.get("doc", 0) for op in operations) @@ -1797,7 +1798,7 @@ class EditPdfDocumentsSerializer(DocumentListSerializer, SourceModeValidationMix doc = Document.objects.get(id=documents[0]) if doc.page_count: for op in operations: - if op["page"] < 1 or op["page"] > doc.page_count: + if op["page"] > doc.page_count: raise serializers.ValidationError( f"Page {op['page']} is out of bounds for document with {doc.page_count} pages.", ) @@ -2128,17 +2129,15 @@ class BulkEditSerializer( def _validate_parameters_edit_pdf(self, parameters, document_id) -> None: if "operations" not in parameters: raise serializers.ValidationError("operations not specified") - if not isinstance(parameters["operations"], list): - raise serializers.ValidationError("operations must be a list") - for op in parameters["operations"]: - if not isinstance(op, dict): - raise serializers.ValidationError("invalid operation entry") - if "page" not in op or not isinstance(op["page"], int): - raise serializers.ValidationError("page must be an integer") - if "rotate" in op and not isinstance(op["rotate"], int): - raise serializers.ValidationError("rotate must be an integer") - if "doc" in op and not isinstance(op["doc"], int): - raise serializers.ValidationError("doc must be an integer") + operations_field = serializers.ListField( + child=PdfEditOperationSerializer(), + allow_empty=False, + ) + parameters["operations"] = operations_field.run_validation( + parameters["operations"], + ) + operations = parameters["operations"] + if "update_document" in parameters: if not isinstance(parameters["update_document"], bool): raise serializers.ValidationError("update_document must be a boolean") @@ -2150,8 +2149,11 @@ class BulkEditSerializer( else: parameters["include_metadata"] = True + if any(op.get("doc", 0) >= len(operations) for op in operations): + raise serializers.ValidationError("doc index is out of bounds") + if parameters["update_document"]: - max_idx = max(op.get("doc", 0) for op in parameters["operations"]) + max_idx = max(op.get("doc", 0) for op in operations) if max_idx > 0: raise serializers.ValidationError( "update_document only allowed with a single output document", @@ -2166,8 +2168,8 @@ class BulkEditSerializer( doc = Document.objects.get(id=document_id) # doc existence is already validated if doc.page_count: - for op in parameters["operations"]: - if op["page"] < 1 or op["page"] > doc.page_count: + for op in operations: + if op["page"] > doc.page_count: raise serializers.ValidationError( f"Page {op['page']} is out of bounds for document with {doc.page_count} pages.", ) diff --git a/src/documents/tests/test_api_bulk_edit.py b/src/documents/tests/test_api_bulk_edit.py index c4f26f877..436208d3a 100644 --- a/src/documents/tests/test_api_bulk_edit.py +++ b/src/documents/tests/test_api_bulk_edit.py @@ -1728,7 +1728,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): content_type="application/json", ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertIn(b"invalid operation entry", response.content) + self.assertIn(b"Expected a dictionary", response.content) response = self.client.post( "/api/documents/edit_pdf/", @@ -1741,7 +1741,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): content_type="application/json", ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertIn(b"page must be an integer", response.content) + self.assertIn(b"valid integer is required", response.content) response = self.client.post( "/api/documents/edit_pdf/", @@ -1754,7 +1754,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): content_type="application/json", ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertIn(b"rotate must be an integer", response.content) + self.assertIn(b"valid integer is required", response.content) response = self.client.post( "/api/documents/edit_pdf/", @@ -1767,7 +1767,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): content_type="application/json", ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertIn(b"doc must be an integer", response.content) + self.assertIn(b"valid integer is required", response.content) for doc_index in (-1, 2**32): with self.subTest(doc_index=doc_index): @@ -1790,7 +1790,7 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): { "documents": [self.doc2.id], "update_document": True, - "operations": [{"page": 1, "doc": 1}, {"page": 2, "doc": 2}], + "operations": [{"page": 1, "doc": 0}, {"page": 2, "doc": 1}], }, ), content_type="application/json", @@ -1815,6 +1815,86 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) self.assertIn(b"Invalid source_mode", response.content) + @mock.patch("documents.views.bulk_edit.edit_pdf") + def test_edit_pdf_rejects_empty_operations(self, m) -> None: + """ + An empty operations list previously reached bulk_edit.edit_pdf() + and crashed with `ValueError: max() iterable argument is empty` + (via `max(op.get("doc", 0) for op in operations)`) whenever + update_document was true. Must now be rejected up front. + """ + self.setup_mock(m, "edit_pdf") + response = self.client.post( + "/api/documents/edit_pdf/", + json.dumps( + { + "documents": [self.doc2.id], + "operations": [], + "update_document": True, + }, + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + m.assert_not_called() + + @mock.patch("documents.views.bulk_edit.edit_pdf") + def test_edit_pdf_rejects_negative_doc_index(self, m) -> None: + """ + A negative `doc` index was previously silently accepted and used + as a wrapping Python list index instead of being rejected. + """ + self.setup_mock(m, "edit_pdf") + response = self.client.post( + "/api/documents/edit_pdf/", + json.dumps( + { + "documents": [self.doc2.id], + "operations": [{"page": 1, "doc": -1}], + }, + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + m.assert_not_called() + + @mock.patch("documents.views.bulk_edit.edit_pdf") + def test_edit_pdf_rejects_out_of_bounds_doc_index(self, m) -> None: + """ + A `doc` index far larger than the number of operations previously + drove `pdf_docs = [pikepdf.new() for _ in range(max_idx + 1)]` to + attempt allocating an enormous number of real objects. + """ + self.setup_mock(m, "edit_pdf") + response = self.client.post( + "/api/documents/edit_pdf/", + json.dumps( + { + "documents": [self.doc2.id], + "operations": [{"page": 1, "doc": 2**33}], + }, + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + m.assert_not_called() + + @mock.patch("documents.views.bulk_edit.edit_pdf") + def test_edit_pdf_rejects_non_positive_page(self, m) -> None: + self.setup_mock(m, "edit_pdf") + response = self.client.post( + "/api/documents/edit_pdf/", + json.dumps( + { + "documents": [self.doc2.id], + "operations": [{"page": 0}], + }, + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + m.assert_not_called() + @mock.patch("documents.views.bulk_edit.edit_pdf") def test_edit_pdf_page_out_of_bounds(self, m) -> None: self.setup_mock(m, "edit_pdf") @@ -1832,6 +1912,46 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): self.assertIn(b"out of bounds", response.content) m.assert_not_called() + @mock.patch("documents.serialisers.bulk_edit.edit_pdf") + def test_bulk_edit_edit_pdf_rejects_empty_operations(self, m) -> None: + """ + Same validation gap as test_edit_pdf_rejects_empty_operations, but + via the legacy generic /api/documents/bulk_edit/ method="edit_pdf" + path, which hand-parses `parameters["operations"]` independently + in BulkEditSerializer._validate_parameters_edit_pdf. + """ + self.setup_mock(m, "edit_pdf") + response = self.client.post( + "/api/documents/bulk_edit/", + json.dumps( + { + "documents": [self.doc2.id], + "method": "edit_pdf", + "parameters": {"operations": [], "update_document": True}, + }, + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + m.assert_not_called() + + @mock.patch("documents.serialisers.bulk_edit.edit_pdf") + def test_bulk_edit_edit_pdf_rejects_out_of_bounds_doc_index(self, m) -> None: + self.setup_mock(m, "edit_pdf") + response = self.client.post( + "/api/documents/bulk_edit/", + json.dumps( + { + "documents": [self.doc2.id], + "method": "edit_pdf", + "parameters": {"operations": [{"page": 1, "doc": 2**33}]}, + }, + ), + content_type="application/json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + m.assert_not_called() + @mock.patch("documents.views.bulk_edit.edit_pdf") def test_edit_pdf_insufficient_permissions(self, m) -> None: self.doc1.owner = User.objects.get(username="temp_admin")