From 8c622bbf9df5de0445f13d6e9cdb2a0a57abe24a Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Wed, 14 Oct 2026 11:01:23 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F(backend)=20bind=20comment?= =?UTF-8?q?=20thread=20to=20the=20path=20document=20on=20creation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 1 + src/backend/core/api/serializers.py | 17 ++- .../documents/test_api_documents_comments.py | 107 ++++++++++++++++++ 3 files changed, 123 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc73d7c17..fda3a97f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to ### Fixed +- 🔒️(backend) bind comment thread to the path document on creation - 🐛(frontend) stop the http fallback when the collaboration server refuses it - 🐛(backend) bypass throttling on access/me action from yhub - 🐛(frontend) switch language when the current one is untranslated #2790 diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 2d1d5d901..d4a2a507b 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -927,8 +927,21 @@ class CommentSerializer(serializers.ModelSerializer): ] def validate(self, attrs): - """Validate comment data.""" - attrs["thread_id"] = self.context["thread_id"] + """Validate comment data. + + 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. + """ + thread = models.Thread.objects.filter( + pk=self.context["thread_id"], + document_id=self.context["document_id"], + ).first() + if thread is None: + raise serializers.ValidationError( + {"thread_id": "This thread does not belong to this document."} + ) + attrs["thread_id"] = thread.pk return attrs def get_abilities(self, obj): diff --git a/src/backend/core/tests/documents/test_api_documents_comments.py b/src/backend/core/tests/documents/test_api_documents_comments.py index 722120243..c24779cbe 100644 --- a/src/backend/core/tests/documents/test_api_documents_comments.py +++ b/src/backend/core/tests/documents/test_api_documents_comments.py @@ -2,6 +2,7 @@ # pylint: disable=too-many-lines import random +import uuid from unittest import mock from django.contrib.auth.models import AnonymousUser @@ -302,6 +303,112 @@ def test_create_comment_authenticated_user_not_enough_access(): assert response.status_code == 403 +def test_create_comment_thread_from_another_document(): + """ + A user allowed to comment on a document should not be able to store a + comment into a thread belonging to another document by naming their own + document in the URL path. + """ + user = factories.UserFactory() + other_document = factories.DocumentFactory( + link_reach="restricted", users=[(user, models.LinkRoleChoices.COMMENTER)] + ) + thread = factories.ThreadFactory() # belongs to another document + + client = APIClient() + client.force_login(user) + with mock.patch("core.api.viewsets.posthog_capture") as mock_capture: + response = client.post( + f"/api/v1.0/documents/{other_document.id!s}/threads/{thread.id!s}/comments/", + {"body": "test"}, + ) + + assert response.status_code == 400 + assert response.json() == { + "thread_id": ["This thread does not belong to this document."] + } + # No comment must have been written anywhere + assert models.Comment.objects.count() == 0 + mock_capture.assert_not_called() + + +def test_create_comment_nonexistent_thread(): + """ + A nonexistent thread id should be refused with a 400, not crash with a 500 + as it did before the thread-to-document binding was enforced. + """ + user = factories.UserFactory() + document = factories.DocumentFactory( + link_reach="restricted", users=[(user, models.LinkRoleChoices.COMMENTER)] + ) + + client = APIClient() + client.force_login(user) + response = client.post( + f"/api/v1.0/documents/{document.id!s}/threads/{uuid.uuid4()!s}/comments/", + {"body": "test"}, + ) + + assert response.status_code == 400 + assert response.json() == { + "thread_id": ["This thread does not belong to this document."] + } + assert models.Comment.objects.count() == 0 + + +def test_create_comment_anonymous_user_cross_document_thread(): + """ + An anonymous user should not be able to write into a thread of a private + document by naming a public document they are allowed to comment on in the + URL path. + """ + public_document = factories.DocumentFactory( + link_reach="public", link_role=models.LinkRoleChoices.EDITOR + ) + thread = factories.ThreadFactory() # belongs to another document + + client = APIClient() + response = client.post( + f"/api/v1.0/documents/{public_document.id!s}/threads/{thread.id!s}/comments/", + {"body": "test"}, + ) + + assert response.status_code == 400 + assert models.Comment.objects.count() == 0 + + +def test_create_comment_reader_cross_document_thread(): + """ + A reader of a document should not be able to escalate to writing comments + by naming another document they can comment on in the URL path. + """ + reader = factories.UserFactory() + owner = factories.UserFactory() + victim_document = factories.DocumentFactory(users=[(owner, "owner")]) + factories.UserDocumentAccessFactory( + document=victim_document, user=reader, role=models.RoleChoices.READER + ) + thread = factories.ThreadFactory(document=victim_document) + + attacker_document = factories.DocumentFactory( + link_reach="restricted", users=[(reader, models.LinkRoleChoices.COMMENTER)] + ) + + client = APIClient() + client.force_login(reader) + response = client.post( + f"/api/v1.0/documents/{attacker_document.id!s}/threads/{thread.id!s}/comments/", + {"body": "test"}, + ) + + assert response.status_code == 400 + assert response.json() == { + "thread_id": ["This thread does not belong to this document."] + } + # The victim's thread was not polluted + assert models.Comment.objects.count() == 0 + + # Retrieve comment