From a82d7f885a99632d48fcf0a9d379fce89bd9e525 Mon Sep 17 00:00:00 2001 From: Florent Chehab Date: Mon, 1 Jun 2026 14:51:41 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F(backend)=20prevnt=20file?= =?UTF-8?q?=20change=20post=20checks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this commit, post a file check, the policy could be reused to change the verified file. Now, files are uplaoded to a temporary location, then inside a transaction that prevents concurrent calls, the file is copied to its final destination and the checks are run on that one. A new file can still be updated with the policy but it will never be read, etc. As part of this change, all files in the new tmp directory on s3 should have an expiration policy. --- CHANGELOG.md | 4 +- src/backend/core/api/viewsets.py | 172 ++++++++++-------- src/backend/core/models.py | 16 ++ .../core/tests/files/test_api_files_create.py | 2 +- .../files/test_api_files_upload_ended.py | 62 ++++++- src/backend/core/utils.py | 2 +- src/backend/meet/settings.py | 8 + 7 files changed, 180 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd280491..3630a614 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to ### Fixed - 🐛(backend) prevent duplicate pending users on concurrent requests +- 🔒️(backend) prevent file change post checks #1377 ## [1.17.0] - 2026-05-31 @@ -30,7 +31,7 @@ and this project adheres to - ✨(backend) add core.recording.event.parsers.S3Parser - ✨(summary) extended support for all video / audio files #1358 -### Changed +### Changed - ♻️(fullstack) simplify source serialization - ✨(backend) expose room configuration to all API consumers @@ -40,6 +41,7 @@ and this project adheres to - ♻️(backend) prefix Swagger routes with /api ### Fixed + - 🩹(backend) fix swagger and redoc documentation URLs ## [1.16.0] - 2026-05-13 diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 3d80d6c5..3924569f 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -1196,96 +1196,116 @@ class FileViewSet( """ Check the actual uploaded file and mark it as ready. """ - - file = self.get_object() - - if not file.is_pending_upload: - raise drf_exceptions.ValidationError( - {"file": "This action is only available for files in PENDING state."}, - code="file_upload_state_not_pending", - ) + # Ensures we go through authorization checks + self.get_object() s3_client = default_storage.connection.meta.client + validation_error = None - head_response = s3_client.head_object( - Bucket=default_storage.bucket_name, Key=file.file_key - ) - file_size = head_response["ContentLength"] + with transaction.atomic(): + file = self.get_queryset().select_for_update().get(pk=kwargs["pk"]) - if settings.FILE_UPLOAD_APPLY_RESTRICTIONS: - config_for_file_type = settings.FILE_UPLOAD_RESTRICTIONS[file.type] - if file_size > config_for_file_type["max_size"]: - self._complete_file_deletion(file) - logger.info( - "upload_ended: file size (%s) for file %s higher than the allowed max size", - file_size, - file.file_key, - ) + if not file.is_pending_upload: raise drf_exceptions.ValidationError( - detail="The file size is higher than the allowed max size.", - code="file_size_exceeded", + { + "file": "This action is only available for files in PENDING state." + }, + code="file_upload_state_not_pending", ) - # python-magic recommends using at least the first 2048 bytes - # to reduce incorrect identification. - # This is a tradeoff between pulling in the whole file and the most likely relevant bytes - # of the file for mime type identification. - if file_size > 2048: - range_response = s3_client.get_object( - Bucket=default_storage.bucket_name, - Key=file.file_key, - Range="bytes=0-2047", - ) - file_head = range_response["Body"].read() - else: - file_head = s3_client.get_object( - Bucket=default_storage.bucket_name, Key=file.file_key - )["Body"].read() - - # Use improved MIME type detection combining magic bytes and file extension - logger.info("upload_ended: detecting mimetype for file: %s", file.file_key) - mimetype = utils.detect_mimetype(file_head, filename=file.filename) - - if settings.FILE_UPLOAD_APPLY_RESTRICTIONS: - config_for_file_type = settings.FILE_UPLOAD_RESTRICTIONS[file.type] - allowed_file_mimetypes = config_for_file_type["allowed_mimetypes"] - if mimetype not in allowed_file_mimetypes: - self._complete_file_deletion(file) - logger.warning( - "upload_ended: mimetype not allowed %s for file %s", - mimetype, - file.file_key, - ) - raise drf_exceptions.ValidationError( - detail="The file type is not allowed.", - code="file_type_not_allowed", - ) - - file.upload_state = models.FileUploadStateChoices.READY - file.mimetype = mimetype - file.size = file_size - - file.save(update_fields=["upload_state", "mimetype", "size"]) - - if head_response["ContentType"] != mimetype: - logger.info( - "upload_ended: content type mismatch between object storage and file," - " updating from %s to %s", - head_response["ContentType"], - mimetype, - ) + # We copy the file to its final destination, we will run the checks on that + # final file and ignore any updates to the temporary file. (We cannot revoke the policy, + # so the temporary file might still be updated after that.) + # The temporary folders will need to be cleaned periodically s3_client.copy_object( Bucket=default_storage.bucket_name, Key=file.file_key, CopySource={ "Bucket": default_storage.bucket_name, - "Key": file.file_key, + "Key": file.temporary_file_key, }, - ContentType=mimetype, - Metadata=head_response["Metadata"], - MetadataDirective="REPLACE", ) + head_response = s3_client.head_object( + Bucket=default_storage.bucket_name, Key=file.file_key + ) + file_size = head_response["ContentLength"] + # python-magic recommends using at least the first 2048 bytes + # to reduce incorrect identification. + # This is a tradeoff between pulling in the whole file and + # the most likely relevant bytes + # of the file for mime type identification. + if file_size > 2048: + range_response = s3_client.get_object( + Bucket=default_storage.bucket_name, + Key=file.file_key, + Range="bytes=0-2047", + ) + file_head = range_response["Body"].read() + else: + file_head = s3_client.get_object( + Bucket=default_storage.bucket_name, Key=file.file_key + )["Body"].read() + + logger.info("upload_ended: detecting mimetype for file: %s", file.file_key) + mimetype = utils.detect_mimetype(file_head, filename=file.filename) + + if settings.FILE_UPLOAD_APPLY_RESTRICTIONS: + config_for_file_type = settings.FILE_UPLOAD_RESTRICTIONS[file.type] + if file_size > config_for_file_type["max_size"]: + logger.info( + "upload_ended: file size (%s) for file %s higher than the allowed max size", + file_size, + file.file_key, + ) + validation_error = drf_exceptions.ValidationError( + detail="The file size is higher than the allowed max size.", + code="file_size_exceeded", + ) + else: + # Use improved MIME type detection combining magic bytes and file extension + allowed_file_mimetypes = config_for_file_type["allowed_mimetypes"] + if mimetype not in allowed_file_mimetypes: + logger.warning( + "upload_ended: mimetype not allowed %s for file %s", + mimetype, + file.file_key, + ) + validation_error = drf_exceptions.ValidationError( + detail="The file type is not allowed.", + code="file_type_not_allowed", + ) + + if validation_error is not None: + self._complete_file_deletion(file) + else: + file.upload_state = models.FileUploadStateChoices.READY + file.mimetype = mimetype + file.size = file_size + file.save(update_fields=["upload_state", "mimetype", "size"]) + + if head_response["ContentType"] != mimetype: + logger.info( + "upload_ended: content type mismatch between object storage and file," + " updating from %s to %s", + head_response["ContentType"], + mimetype, + ) + s3_client.copy_object( + Bucket=default_storage.bucket_name, + Key=file.file_key, + CopySource={ + "Bucket": default_storage.bucket_name, + "Key": file.file_key, + }, + ContentType=mimetype, + Metadata=head_response["Metadata"], + MetadataDirective="REPLACE", + ) + + if validation_error: + raise validation_error + # Not yet implemented # Change the file.upload_state when this will be done # malware_detection.analyse_file(file.file_key, file_id=file.id) @@ -1298,7 +1318,7 @@ class FileViewSet( """Delete a file completely.""" file.soft_delete() file.hard_delete() - process_file_deletion.delay(file.id) + transaction.on_commit(lambda: process_file_deletion.delay(file.id)) def _authorize_subrequest(self, request, pattern): """ diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 32d17db8..bfdb4643 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -954,6 +954,16 @@ class File(BaseModel): return f"{settings.FILE_UPLOAD_PATH}/{self.pk!s}" + @property + def temporary_key_base(self): + """Temporary key base used while upload is still pending.""" + if not self.pk: + raise RuntimeError( + "The file instance must be saved before requesting a storage key." + ) + + return f"{settings.FILE_UPLOAD_TMP_PATH}/{self.pk!s}" + @property def file_key(self): """Key used to store the file in object storage.""" @@ -962,6 +972,12 @@ class File(BaseModel): # leaking Personal Information in logs, etc. return f"{self.key_base}{extension!s}" + @property + def temporary_file_key(self): + """Temporary key used to upload the file before it is finalized.""" + _, extension = splitext(self.filename) + return f"{self.temporary_key_base}{extension!s}" + def get_abilities(self, user): """ Compute and return abilities for a given user on the file. diff --git a/src/backend/core/tests/files/test_api_files_create.py b/src/backend/core/tests/files/test_api_files_create.py index 942315ae..3bfbae5a 100644 --- a/src/backend/core/tests/files/test_api_files_create.py +++ b/src/backend/core/tests/files/test_api_files_create.py @@ -118,7 +118,7 @@ def test_api_files_create_file_authenticated_success(): assert policy_parsed.scheme == "http" assert policy_parsed.netloc in ["minio:9000", "localhost:9000"] - assert policy_parsed.path == f"/meet-media-storage/files/{file.id!s}.png" + assert policy_parsed.path == f"/meet-media-storage/tmp/files/{file.id!s}.png" query_params = parse_qs(policy_parsed.query) diff --git a/src/backend/core/tests/files/test_api_files_upload_ended.py b/src/backend/core/tests/files/test_api_files_upload_ended.py index 7f238955..a89c578c 100644 --- a/src/backend/core/tests/files/test_api_files_upload_ended.py +++ b/src/backend/core/tests/files/test_api_files_upload_ended.py @@ -1,6 +1,7 @@ """Test related to item upload ended API.""" import logging +from concurrent.futures import ThreadPoolExecutor from io import BytesIO from django.core.files.storage import default_storage @@ -81,7 +82,7 @@ def test_api_file_upload_ended_success(settings): ) default_storage.save( - file.file_key, + file.temporary_file_key, BytesIO(b"my prose"), ) @@ -97,6 +98,7 @@ def test_api_file_upload_ended_success(settings): assert response.json()["mimetype"] == "text/plain" +@pytest.mark.django_db(transaction=True) def test_api_file_upload_ended_mimetype_not_allowed(settings, caplog): """ Test that the API returns a 400 when the mimetype is not allowed. @@ -119,7 +121,7 @@ def test_api_file_upload_ended_mimetype_not_allowed(settings, caplog): ) default_storage.save( - file.file_key, + file.temporary_file_key, BytesIO(b"my prose"), ) @@ -156,7 +158,7 @@ def test_api_file_upload_ended_mimetype_not_allowed_not_checking_mimetype(settin ) default_storage.save( - file.file_key, + file.temporary_file_key, BytesIO(b"my prose"), ) @@ -200,7 +202,7 @@ def test_api_upload_ended_mismatch_mimetype_with_object_storage(settings, caplog s3_client.put_object( Bucket=default_storage.bucket_name, - Key=file.file_key, + Key=file.temporary_file_key, ContentType="text/html", Body=BytesIO( b'' @@ -211,7 +213,7 @@ def test_api_upload_ended_mismatch_mimetype_with_object_storage(settings, caplog ) head_object = s3_client.head_object( - Bucket=default_storage.bucket_name, Key=file.file_key + Bucket=default_storage.bucket_name, Key=file.temporary_file_key ) assert head_object["ContentType"] == "text/html" @@ -234,9 +236,10 @@ def test_api_upload_ended_mismatch_mimetype_with_object_storage(settings, caplog assert head_object["Metadata"] == {"foo": "bar"} +@pytest.mark.django_db(transaction=True) def test_api_upload_ended_file_size_exceeded(settings, caplog): """ - Test when the file size exceed the allowed max upload file size + Test when the file size exceeds the allowed max upload file size should return a 400 and delete the file. """ @@ -256,7 +259,7 @@ def test_api_upload_ended_file_size_exceeded(settings, caplog): ) default_storage.save( - file.file_key, + file.temporary_file_key, BytesIO(b"my prose"), ) @@ -270,3 +273,48 @@ def test_api_upload_ended_file_size_exceeded(settings, caplog): assert not models.File.objects.filter(id=file.id).exists() assert not default_storage.exists(file.file_key) + + +@pytest.mark.django_db(transaction=True) +def test_api_file_upload_ended_concurrent_calls_are_serialized(settings): + """Only one concurrent upload-ended call can finalize a pending upload.""" + settings.FILE_UPLOAD_APPLY_RESTRICTIONS = True + settings.FILE_UPLOAD_RESTRICTIONS = { + "background_image": { + **settings.FILE_UPLOAD_RESTRICTIONS["background_image"], + "allowed_mimetypes": ["text/plain"], + }, + } + + user = factories.UserFactory() + file = factories.FileFactory( + type=FileTypeChoices.BACKGROUND_IMAGE, + filename="my_file.txt", + creator=user, + ) + default_storage.save(file.temporary_file_key, BytesIO(b"my prose")) + + def call_upload_ended(): + client = APIClient() + client.force_login(user) + return client.post(f"/api/v1.0/files/{file.id!s}/upload-ended/") + + with ThreadPoolExecutor(max_workers=2) as executor: + futures = [ + executor.submit(call_upload_ended), + executor.submit(call_upload_ended), + ] + responses = [future.result() for future in futures] + + status_codes = sorted(response.status_code for response in responses) + assert status_codes == [200, 400] + + failed_response = next( + response for response in responses if response.status_code == 400 + ) + assert failed_response.json() == { + "file": "This action is only available for files in PENDING state." + } + + file.refresh_from_db() + assert file.upload_state == FileUploadStateChoices.READY diff --git a/src/backend/core/utils.py b/src/backend/core/utils.py index 39135b37..c7280fc4 100644 --- a/src/backend/core/utils.py +++ b/src/backend/core/utils.py @@ -426,7 +426,7 @@ def generate_upload_policy(file): Originally taken from https://github.com/suitenumerique/drive/blob/564822d31f071c6dfacd112ef4b7146c73077cd9/src/backend/core/api/utils.py#L102 # pylint: disable=line-too-long """ - key = file.file_key + key = file.temporary_file_key # This settings should be used if the backend application and the frontend application # can't connect to the object storage with the same domain. This is the case in the diff --git a/src/backend/meet/settings.py b/src/backend/meet/settings.py index d38025a7..4f7d696a 100755 --- a/src/backend/meet/settings.py +++ b/src/backend/meet/settings.py @@ -186,6 +186,9 @@ class Base(Configuration): FILE_UPLOAD_PATH = values.Value( "files", environ_name="FILE_UPLOAD_PATH", environ_prefix=None ) + FILE_UPLOAD_TMP_PATH = values.Value( + "tmp/files", environ_name="FILE_UPLOAD_TMP_PATH", environ_prefix=None + ) FILE_UPLOAD_APPLY_RESTRICTIONS = values.BooleanValue( default=True, environ_name="FILE_UPLOAD_APPLY_RESTRICTIONS", environ_prefix=None @@ -1087,6 +1090,11 @@ class Base(Configuration): """ super().post_setup() + if cls.FILE_UPLOAD_TMP_PATH == cls.FILE_UPLOAD_PATH: + raise ValueError( + "FILE_UPLOAD_TMP_PATH cannot be the same as FILE_UPLOAD_PATH" + ) + # The SENTRY_DSN setting should be available to activate sentry for an environment if cls.SENTRY_DSN is not None: sentry_sdk.init(