Compare commits

...
Author SHA1 Message Date
Trenton Holmes 8ac8dd7a04 Fix: _validate_owner crashes on wrong-typed owner, not just missing id
Review caught that the previous fix only caught User.DoesNotExist,
but parameters is a bare DictField, so a list/dict/non-numeric-string
owner reaches User.objects.get(pk=owner) and raises an uncaught
TypeError/ValueError from Django's AutoField.get_prep_value instead.
2026-09-13 15:09:06 -07:00
Trenton HolmesandClaude Sonnet 5 f1b21628a9 Fix: bulk-edit rotate/split/owner validators crash on wrong-typed input
BulkEditSerializer's hand-parsed parameter validators only caught the
exception types their happy-path callers happened to raise, not what
untrusted input can actually produce:

- _validate_parameters_rotate: float(None) raises TypeError, only
  ValueError was caught.
- _validate_parameters_split: parameters["pages"].split(",") assumed a
  string; a null value raised AttributeError.
- _validate_owner: User.objects.get(pk=owner) raises DoesNotExist for a
  nonexistent id with no try/except at all (the `if ownerUser is None`
  check below it was dead code, since .get() never returns None).

All three surfaced as an uncaught 500 instead of a 400.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-13 14:58:27 -07:00
Trenton H 05b7697c35 Fix: avoid IntegrityError when a retried task republishes with the same task_id (#14096) 2026-09-13 21:05:54 +00:00
4 changed files with 134 additions and 10 deletions
+6 -4
View File
@@ -2037,10 +2037,10 @@ class BulkEditSerializer(
raise serializers.ValidationError("remove_custom_fields not specified")
def _validate_owner(self, owner):
ownerUser = User.objects.get(pk=owner)
if ownerUser is None:
try:
return User.objects.get(pk=owner)
except (User.DoesNotExist, TypeError, ValueError):
raise serializers.ValidationError("Specified owner cannot be found")
return ownerUser
def _validate_parameters_set_permissions(self, parameters) -> None:
if "set_permissions" not in parameters:
@@ -2060,7 +2060,7 @@ class BulkEditSerializer(
or not float(parameters["degrees"]).is_integer()
):
raise serializers.ValidationError("invalid rotation degrees")
except ValueError:
except (TypeError, ValueError):
raise serializers.ValidationError("invalid rotation degrees")
def _validate_source_mode(self, parameters) -> None:
@@ -2073,6 +2073,8 @@ class BulkEditSerializer(
def _validate_parameters_split(self, parameters) -> None:
if "pages" not in parameters:
raise serializers.ValidationError("pages not specified")
if not isinstance(parameters["pages"], str):
raise serializers.ValidationError("invalid pages specified")
try:
pages = []
docs = parameters["pages"].split(",")
+11 -6
View File
@@ -1189,13 +1189,18 @@ def before_task_publish_handler(
trigger_source = _determine_trigger_source(headers)
owner_id = _extract_owner_id(task_type, task_kwargs)
PaperlessTask.objects.create(
# A retried task is republished with the same task_id, so this fires
# again for it; get_or_create keeps the original PENDING record
# instead of raising a duplicate-key IntegrityError on the retry.
PaperlessTask.objects.get_or_create(
task_id=task_id,
task_type=task_type,
trigger_source=trigger_source,
status=PaperlessTask.Status.PENDING,
input_data=input_data,
owner_id=owner_id,
defaults={
"task_type": task_type,
"trigger_source": trigger_source,
"status": PaperlessTask.Status.PENDING,
"input_data": input_data,
"owner_id": owner_id,
},
)
except Exception: # pragma: no cover
logger.exception("Creating PaperlessTask failed")
+106
View File
@@ -1165,6 +1165,65 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
self.assertIn(b"set_permissions not specified", response.content)
m.assert_not_called()
@mock.patch("documents.serialisers.bulk_edit.set_permissions")
def test_set_permissions_rejects_nonexistent_owner(self, m) -> None:
"""
BulkEditSerializer._validate_owner called User.objects.get(pk=owner)
with no try/except, so a syntactically valid but nonexistent user
id raised an uncaught User.DoesNotExist instead of a clean 400.
"""
self.setup_mock(m, "set_permissions")
response = self.client.post(
"/api/documents/bulk_edit/",
json.dumps(
{
"documents": [self.doc2.id],
"method": "set_permissions",
"parameters": {
"set_permissions": {"view": {"users": [self.user.id]}},
"owner": 999999,
},
},
),
content_type="application/json",
)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
m.assert_not_called()
@mock.patch("documents.serialisers.bulk_edit.set_permissions")
def test_set_permissions_rejects_wrong_typed_owner(self, m) -> None:
"""
_validate_owner only caught User.DoesNotExist -- a wrong-typed
owner (list/dict/non-numeric string) reaches
User.objects.get(pk=owner) and raises an uncaught TypeError or
ValueError instead, since `parameters` is a bare DictField with
no type checking on "owner" at that level.
"""
self.setup_mock(m, "set_permissions")
for bad_owner in (["not", "an", "id"], {"nested": "dict"}, "not-a-number"):
with self.subTest(owner=bad_owner):
response = self.client.post(
"/api/documents/bulk_edit/",
json.dumps(
{
"documents": [self.doc2.id],
"method": "set_permissions",
"parameters": {
"set_permissions": {
"view": {"users": [self.user.id]},
},
"owner": bad_owner,
},
},
),
content_type="application/json",
)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
m.assert_not_called()
@mock.patch("documents.serialisers.bulk_edit.set_permissions")
def test_set_permissions_merge(self, m) -> None:
self.setup_mock(m, "set_permissions")
@@ -1438,6 +1497,53 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase):
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
m.assert_not_called()
@mock.patch("documents.serialisers.bulk_edit.rotate")
def test_bulk_edit_rotate_rejects_null_degrees(self, m) -> None:
"""
BulkEditSerializer._validate_parameters_rotate's
`float(parameters["degrees"])` raised an uncaught TypeError for
None (only ValueError was caught), reachable via the legacy
generic /api/documents/bulk_edit/ method="rotate" path (the
dedicated /api/documents/rotate/ endpoint isn't affected, its
`degrees` field is a typed IntegerField).
"""
self.setup_mock(m, "rotate")
response = self.client.post(
"/api/documents/bulk_edit/",
json.dumps(
{
"documents": [self.doc2.id],
"method": "rotate",
"parameters": {"degrees": None},
},
),
content_type="application/json",
)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
m.assert_not_called()
@mock.patch("documents.serialisers.bulk_edit.split")
def test_bulk_edit_split_rejects_null_pages(self, m) -> None:
"""
BulkEditSerializer._validate_parameters_split called
parameters["pages"].split(",") with no type check, so a null
value raised an uncaught AttributeError instead of a clean 400.
"""
self.setup_mock(m, "split")
response = self.client.post(
"/api/documents/bulk_edit/",
json.dumps(
{
"documents": [self.doc2.id],
"method": "split",
"parameters": {"pages": None},
},
),
content_type="application/json",
)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
m.assert_not_called()
@mock.patch("documents.views.bulk_edit.rotate")
def test_rotate_insufficient_permissions(self, m) -> None:
self.doc1.owner = User.objects.get(username="temp_admin")
+11
View File
@@ -106,6 +106,17 @@ class TestBeforeTaskPublishHandler:
assert task.task_type == PaperlessTask.TaskType.TRAIN_CLASSIFIER
assert task.trigger_source == PaperlessTask.TriggerSource.MANUAL
# A Celery retry republishes with the same task_id; this must not
# raise a duplicate-key IntegrityError, and must leave the original
# PENDING record alone.
send_publish(
"documents.tasks.train_classifier",
(),
{},
headers={"id": task_id},
)
assert PaperlessTask.objects.filter(task_id=task_id).count() == 1
def test_creates_task_for_sanity_check(self) -> None:
task_id = send_publish("documents.tasks.sanity_check", (), {})
task = PaperlessTask.objects.get(task_id=task_id)