Skip to content

Commit 43f9748

Browse files
authored
🔒️(backend) bind comment thread to the path document on creation
Bind the thread to the document named in the URL path: a comment must never be stored in a thread that belongs to another document, even when the caller is allowed to comment on that other document.
1 parent 5bb089e commit 43f9748

3 files changed

Lines changed: 133 additions & 2 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@ and this project adheres to
1717
- 💄(frontend) update upload loader and error ui #2778
1818
- ⬆️(yhub) upgrade @y/hub to 0.10.1 #2853
1919

20+
### Fixed
21+
22+
- 🔒️(backend) bind comment thread to the path document on creation #2838
23+
2024
## [v6.0.1] - 2026-10-15
2125

2226
### Added

‎src/backend/core/api/serializers.py‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -927,8 +927,22 @@ class Meta:
927927
]
928928

929929
def validate(self, attrs):
930-
"""Validate comment data."""
931-
attrs["thread_id"] = self.context["thread_id"]
930+
"""Validate comment data.
931+
932+
Bind the thread to the document named in the URL path: a comment must
933+
never be stored in a thread that belongs to another document, even when
934+
the caller is allowed to comment on that other document.
935+
"""
936+
thread_id = self.context["thread_id"]
937+
938+
if not models.Thread.objects.filter(
939+
pk=thread_id,
940+
document_id=self.context["document_id"],
941+
).exists():
942+
raise serializers.ValidationError(
943+
{"thread_id": "This thread does not belong to this document."}
944+
)
945+
attrs["thread_id"] = thread_id
932946
return attrs
933947

934948
def get_abilities(self, obj):

‎src/backend/core/tests/documents/test_api_documents_comments.py‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
# pylint: disable=too-many-lines
44
import random
5+
import uuid
56
from unittest import mock
67

78
from django.contrib.auth.models import AnonymousUser
@@ -302,6 +303,118 @@ def test_create_comment_authenticated_user_not_enough_access():
302303
assert response.status_code == 403
303304

304305

306+
def test_create_comment_thread_from_another_document():
307+
"""
308+
A user allowed to comment on a document should not be able to store a
309+
comment into a thread belonging to another document by naming their own
310+
document in the URL path.
311+
"""
312+
user = factories.UserFactory()
313+
other_document = factories.DocumentFactory(
314+
link_reach="restricted", users=[(user, models.LinkRoleChoices.COMMENTER)]
315+
)
316+
thread = factories.ThreadFactory() # belongs to another document
317+
318+
client = APIClient()
319+
client.force_login(user)
320+
with mock.patch("core.api.viewsets.posthog_capture") as mock_capture:
321+
response = client.post(
322+
f"/api/v1.0/documents/{other_document.id!s}/threads/{thread.id!s}/comments/",
323+
{"body": "test"},
324+
)
325+
326+
assert response.status_code == 400
327+
assert response.json() == {
328+
"thread_id": ["This thread does not belong to this document."]
329+
}
330+
# No comment must have been written anywhere
331+
assert models.Comment.objects.count() == 0
332+
mock_capture.assert_not_called()
333+
334+
335+
def test_create_comment_nonexistent_thread():
336+
"""
337+
A nonexistent thread id should be refused with a 400, not crash with a 500
338+
as it did before the thread-to-document binding was enforced.
339+
"""
340+
user = factories.UserFactory()
341+
document = factories.DocumentFactory(
342+
link_reach="restricted", users=[(user, models.LinkRoleChoices.COMMENTER)]
343+
)
344+
345+
client = APIClient()
346+
client.force_login(user)
347+
response = client.post(
348+
f"/api/v1.0/documents/{document.id!s}/threads/{uuid.uuid4()!s}/comments/",
349+
{"body": "test"},
350+
)
351+
352+
assert response.status_code == 400
353+
assert response.json() == {
354+
"thread_id": ["This thread does not belong to this document."]
355+
}
356+
assert models.Comment.objects.count() == 0
357+
358+
359+
@pytest.mark.parametrize(
360+
"link_reach",
361+
[models.LinkReachChoices.RESTRICTED, models.LinkReachChoices.AUTHENTICATED],
362+
)
363+
def test_create_comment_anonymous_user_cross_document_thread(link_reach):
364+
"""
365+
An anonymous user should not be able to write into a thread of a private
366+
document by naming a public document they are allowed to comment on in the
367+
URL path.
368+
"""
369+
public_document = factories.DocumentFactory(
370+
link_reach="public", link_role=models.LinkRoleChoices.EDITOR
371+
)
372+
thread = factories.ThreadFactory(
373+
document__link_reach=link_reach
374+
) # belongs to another document
375+
376+
client = APIClient()
377+
response = client.post(
378+
f"/api/v1.0/documents/{public_document.id!s}/threads/{thread.id!s}/comments/",
379+
{"body": "test"},
380+
)
381+
382+
assert response.status_code == 400
383+
assert models.Comment.objects.count() == 0
384+
385+
386+
def test_create_comment_reader_cross_document_thread():
387+
"""
388+
A reader of a document should not be able to escalate to writing comments
389+
by naming another document they can comment on in the URL path.
390+
"""
391+
reader = factories.UserFactory()
392+
owner = factories.UserFactory()
393+
victim_document = factories.DocumentFactory(users=[(owner, "owner")])
394+
factories.UserDocumentAccessFactory(
395+
document=victim_document, user=reader, role=models.RoleChoices.READER
396+
)
397+
thread = factories.ThreadFactory(document=victim_document)
398+
399+
attacker_document = factories.DocumentFactory(
400+
link_reach="restricted", users=[(reader, models.LinkRoleChoices.COMMENTER)]
401+
)
402+
403+
client = APIClient()
404+
client.force_login(reader)
405+
response = client.post(
406+
f"/api/v1.0/documents/{attacker_document.id!s}/threads/{thread.id!s}/comments/",
407+
{"body": "test"},
408+
)
409+
410+
assert response.status_code == 400
411+
assert response.json() == {
412+
"thread_id": ["This thread does not belong to this document."]
413+
}
414+
# The victim's thread was not polluted
415+
assert models.Comment.objects.count() == 0
416+
417+
305418
# Retrieve comment
306419

307420

0 commit comments

Comments
 (0)