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(