From e80751d157b26b0d3dcd702afb055f4a44a491dc Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Wed, 7 Oct 2026 16:26:16 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F(backend)=20fix=20race=20c?= =?UTF-8?q?onditions=20between=20actions=20updating=20a=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If two requests at the exact same time update a document and link-configuration, a race condition can happen and one action can overwrite the other one. If a document link_reach is changed from public to restricted, this change can be overwritten and the owner does not have this information. To fix this issue we set the updated_fields for every save. --- src/backend/core/api/serializers.py | 18 +- src/backend/core/api/viewsets.py | 7 +- .../test_api_documents_attachment_upload.py | 44 ++++- .../test_api_documents_link_configuration.py | 88 +++++++++ .../documents/test_api_documents_update.py | 167 ++++++++++++++++++ 5 files changed, 320 insertions(+), 4 deletions(-) diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 9265a812d..2d1d5d901 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -289,10 +289,26 @@ class DocumentSerializer(ListDocumentSerializer): """ When no data is sent on the update, skip making the update in the database and return directly the instance unchanged. + + We don't rely on the update methof from the ModelSerializer to only save + validated_data fields to not override updated value from other + serializer like the LinkDocumentSerializer. A race condition triggering both + serializer in the same time can lead to an overlap and data can be lost. """ if not validated_data: return instance # No data provided, skip the update - return super().update(instance, validated_data) + for attr, value in validated_data.items(): + setattr(instance, attr, value) + try: + # `updated_at` is explicit because it is an `auto_now` field: it has + # no validated_data entry and would not be written otherwise. + instance.save(update_fields=[*validated_data, "updated_at"]) + except Exception: + # The in-memory instance may hold values the database has not + # accepted: reload them so the response reports the row as it is. + instance.refresh_from_db(fields=validated_data.keys()) + raise + return instance class SearchDocumentSerializer(ListDocumentSerializer): diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 2a7d929f3..930dce20c 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -1897,7 +1897,7 @@ class DocumentViewSet( # saving a changed link definition is what tells the collaboration # server to re-check the connections of the document and its descendants - serializer.save() + serializer.save(update_fields=["link_reach", "link_role", "updated_at"]) return drf.response.Response(serializer.data, status=drf.status.HTTP_200_OK) @@ -2024,7 +2024,10 @@ class DocumentViewSet( # Make the attachment readable by document readers document.attachments.append(key) - document.save() + # Only the attachments column is written: the instance was loaded at the + # start of the request and an overlapping change must not be undone + # (e.g. a withdrawal of the link configuration committed in between). + document.save(update_fields=["attachments", "updated_at"]) malware_detection.analyse_file(key, document_id=document.id) diff --git a/src/backend/core/tests/documents/test_api_documents_attachment_upload.py b/src/backend/core/tests/documents/test_api_documents_attachment_upload.py index 9634a163e..a65eb47b3 100644 --- a/src/backend/core/tests/documents/test_api_documents_attachment_upload.py +++ b/src/backend/core/tests/documents/test_api_documents_attachment_upload.py @@ -13,7 +13,7 @@ from django.core.files.uploadedfile import SimpleUploadedFile import pytest from rest_framework.test import APIClient -from core import factories +from core import factories, models from core.api.viewsets import malware_detection from core.tests.conftest import TEAM, USER, VIA @@ -492,3 +492,45 @@ def test_api_documents_attachment_upload_unsafe_mime_types_disabled(settings): "application/octet-stream", ] assert file_head["ContentDisposition"] == 'attachment; filename="script.exe"' + + +def test_api_documents_attachment_upload_stale_instance_does_not_revert_link_configuration(): + """ + An attachment upload must only write the attachments column. + + A request that loaded the document before an owner withdrew the link + configuration must not put the old configuration back when it saves the + attachment. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory( + link_reach="public", link_role="editor", users=[(user, "editor")] + ) + + # The upload request loads the document while it is still public. + stale_document = models.Document.objects.get(pk=document.pk) + + # The owner withdraws the link in between. + withdrawal = models.Document.objects.get(pk=document.pk) + withdrawal.link_reach = models.LinkReachChoices.RESTRICTED + withdrawal.link_role = models.LinkRoleChoices.READER + withdrawal.save(update_fields=["link_reach", "link_role"]) + + # The upload commits after the withdrawal. + file = SimpleUploadedFile(name="test.png", content=PIXEL, content_type="image/png") + url = f"/api/v1.0/documents/{document.id!s}/attachment-upload/" + with mock.patch.object(malware_detection, "analyse_file"): + response = client.post(url, {"file": file}, format="multipart") + + assert response.status_code == 201 + + document.refresh_from_db() + assert len(document.attachments) == 1 + assert document.link_reach == models.LinkReachChoices.RESTRICTED + assert document.link_role == models.LinkRoleChoices.READER + + # The stale in-memory instance is left untouched for reference. + assert stale_document.link_reach == "public" diff --git a/src/backend/core/tests/documents/test_api_documents_link_configuration.py b/src/backend/core/tests/documents/test_api_documents_link_configuration.py index 7a516afea..384c329b5 100644 --- a/src/backend/core/tests/documents/test_api_documents_link_configuration.py +++ b/src/backend/core/tests/documents/test_api_documents_link_configuration.py @@ -419,3 +419,91 @@ def test_api_documents_link_configuration_update_invalid_role_for_reach_validati in error_message ) assert "Allowed roles: editor" in error_message + + +def test_api_documents_link_configuration_survives_concurrent_title_patch( + mock_reset_connections, +): + """ + A title patch loaded before a withdrawal of the link configuration and + saved after it must not put the withdrawn link configuration back. + """ + owner = factories.UserFactory() + client = APIClient() + client.force_login(owner) + + document = factories.DocumentFactory( + link_reach=models.LinkReachChoices.PUBLIC, + link_role=models.LinkRoleChoices.EDITOR, + title="before", + ) + factories.UserDocumentAccessFactory( + document=document, user=owner, role=models.RoleChoices.OWNER + ) + + # The concurrent writer (e.g. an anonymous editor) loads the document + # before the withdrawal: the request holds an instance in memory. + stale_document = models.Document.objects.get(pk=document.pk) + + # The owner withdraws the link in between. + with mock_reset_connections(document.id): + response = client.put( + f"/api/v1.0/documents/{document.id!s}/link-configuration/", + {"link_reach": models.LinkReachChoices.RESTRICTED}, + format="json", + ) + assert response.status_code == 200 + + # The concurrent write commits after the withdrawal. + stale_document.title = "written by an anonymous visitor" + stale_document.save(update_fields=["title"]) + + # The withdrawal holds: the write did not write the sharing columns back. + # The withdrawal only changed the reach, so the editor role of the link is + # kept in the database while the reach is now restricted. + document.refresh_from_db() + assert document.link_reach == models.LinkReachChoices.RESTRICTED + assert document.title == "written by an anonymous visitor" + + +def test_api_documents_link_configuration_save_writes_link_fields_only( + mock_reset_connections, +): + """ + The link-configuration endpoint must only write the link fields: a stale + instance must not be able to rewrite the rest of the row. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory( + link_reach=models.LinkReachChoices.PUBLIC, + link_role=models.LinkRoleChoices.READER, + title="untouched", + ) + factories.UserDocumentAccessFactory( + document=document, user=user, role=models.RoleChoices.OWNER + ) + + # Corrupt the in-memory instance the request will come from to prove that + # none of the other columns are written by the endpoint. + stale_document = models.Document.objects.get(pk=document.pk) + stale_document.title = "title the endpoint must not save" + stale_document.path = "000000" # a stale tree path + + with mock_reset_connections(document.id): + response = client.put( + f"/api/v1.0/documents/{document.id!s}/link-configuration/", + { + "link_reach": models.LinkReachChoices.AUTHENTICATED, + "link_role": models.LinkRoleChoices.READER, + }, + format="json", + ) + assert response.status_code == 200 + + document.refresh_from_db() + assert document.link_reach == models.LinkReachChoices.AUTHENTICATED + assert document.title == "untouched" + assert document.path != "000000" diff --git a/src/backend/core/tests/documents/test_api_documents_update.py b/src/backend/core/tests/documents/test_api_documents_update.py index 1b89996b6..4977e4ab6 100644 --- a/src/backend/core/tests/documents/test_api_documents_update.py +++ b/src/backend/core/tests/documents/test_api_documents_update.py @@ -660,3 +660,170 @@ def test_api_documents_patch_empty_body(): new_document_values = serializers.DocumentSerializer(instance=document).data assert new_document_values == old_document_values assert document_updated_at == document.updated_at + + +@pytest.mark.parametrize("via", VIA) +def test_api_documents_patch_stale_instance_does_not_revert_link_configuration( + via, mock_user_teams +): + """ + A write must only save the fields the request carries. + + An editor patching a document from an instance loaded before an owner + withdrew the link configuration must not put the old link configuration + back: the patch saves the title only. This is the interleaving of the + reported race condition, reproduced deterministically by loading the + stale instance before the withdrawal and saving after it. + """ + owner = factories.UserFactory() + editor = factories.UserFactory(with_owned_document=True) + + client = APIClient() + client.force_login(editor) + + document = factories.DocumentFactory( + link_reach="public", link_role="editor", title="before" + ) + factories.UserDocumentAccessFactory(document=document, user=owner, role="owner") + + if via == USER: + factories.UserDocumentAccessFactory( + document=document, user=editor, role="editor" + ) + elif via == TEAM: + mock_user_teams.return_value = ["lasuite", "unknown"] + factories.TeamDocumentAccessFactory( + document=document, team="lasuite", role="editor" + ) + + # The editor's request loads the document while it is still public. + stale_document = models.Document.objects.get(pk=document.pk) + + # The owner withdraws the link in between. + withdrawal = models.Document.objects.get(pk=document.pk) + withdrawal.link_reach = models.LinkReachChoices.RESTRICTED + withdrawal.link_role = models.LinkRoleChoices.READER + withdrawal.save(update_fields=["link_reach", "link_role"]) + + # The editor's write commits after the withdrawal. + response = client.patch( + f"/api/v1.0/documents/{document.id!s}/", + {"title": "written while revoked"}, + format="json", + ) + assert response.status_code == 200 + + # The withdrawal holds: the patch did not write the sharing columns back. + document.refresh_from_db() + assert document.title == "written while revoked" + assert document.link_reach == models.LinkReachChoices.RESTRICTED + assert document.link_role == models.LinkRoleChoices.READER + + # The stale instance the patch came from is left untouched for reference. + assert stale_document.link_reach == models.LinkReachChoices.PUBLIC + + +def test_api_documents_patch_stale_instance_does_not_resurrect_soft_deletion(): + """ + A write loaded before a soft deletion must not undo the deletion by + writing the deleted_at and ancestors_deleted_at columns back. + """ + user = factories.UserFactory() + + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory(title="to delete", users=[(user, "owner")]) + + # The request loads the document while it is alive. + stale_document = models.Document.objects.get(pk=document.pk) + + # The owner soft deletes it in between. + document.soft_delete() + + # A write admitted before the deletion commits after it. + response = client.patch( + f"/api/v1.0/documents/{document.id!s}/", + {"title": "written while deleted"}, + format="json", + ) + assert response.status_code in [200, 403, 404] + + document.refresh_from_db() + assert document.deleted_at is not None + assert document.ancestors_deleted_at is not None + # The title may have been saved on the deleted document: what must hold is + # that the deletion itself was not reverted. + assert stale_document.deleted_at is None + + +def test_api_documents_patch_stale_instance_does_not_revert_path(): + """ + A write loaded before a move must not write the stale tree path back, + which would leave the document under one parent and counted by another. + """ + user = factories.UserFactory() + + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory(title="to move", users=[(user, "owner")]) + old_path = document.path + + # The request loads the document before the move. + models.Document.objects.get(pk=document.pk) + + # The owner moves it under another parent in between. + new_parent = factories.DocumentFactory() + document.move(new_parent, pos="last-child") + + # The write commits after the move. + response = client.patch( + f"/api/v1.0/documents/{document.id!s}/", + {"title": "written after the move"}, + format="json", + ) + assert response.status_code == 200 + + document.refresh_from_db() + assert document.title == "written after the move" + assert document.path != old_path + assert document.get_parent().id == new_parent.id + + +def test_api_documents_update_stale_instance_does_not_revert_link_configuration(): + """ + Same lost update through a full PUT: an update carrying only writable + fields must not rewrite the read-only link columns of the row. + """ + user = factories.UserFactory() + + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory( + link_reach="public", link_role="editor", title="old title" + ) + factories.UserDocumentAccessFactory(document=document, user=user, role="owner") + + # The request loads the document while it is still public. + models.Document.objects.get(pk=document.pk) + + # The owner withdraws the link in between. + withdrawal = models.Document.objects.get(pk=document.pk) + withdrawal.link_reach = models.LinkReachChoices.RESTRICTED + withdrawal.link_role = models.LinkRoleChoices.READER + withdrawal.save(update_fields=["link_reach", "link_role"]) + + # The write commits after the withdrawal: PUT with the writable fields only. + response = client.put( + f"/api/v1.0/documents/{document.id!s}/", + {"title": "written while revoked", "excerpt": "excerpt"}, + format="json", + ) + assert response.status_code == 200 + + document.refresh_from_db() + assert document.title == "written while revoked" + assert document.link_reach == models.LinkReachChoices.RESTRICTED + assert document.link_role == models.LinkRoleChoices.READER