gsadmin/meet
1
0
Fork 0
mirror of https://github.com/suitenumerique/meet.git synced 2026-08-07 09:23:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

🔒️(backend) prevnt file change post checks

Browse Source 
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.
This commit is contained in:
Florent Chehab
2026-06-01 14:51:41 +02:00
parent 5ef6e8b5ea
commit a82d7f885a
7 changed files with 180 additions and 86 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+3 -1
View File
@@ -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
src/backend/core/api/viewsets.py
+96 -76
View File
@@ -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):
"""
src/backend/core/models.py
+16
View File
@@ -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.
src/backend/core/tests/files/test_api_files_create.py
+1 -1
View File
@@ -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)
src/backend/core/tests/files/test_api_files_upload_ended.py
+55 -7
View File
@@ -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'<meta http-equiv="refresh" content="0; url=https://fichiers.numerique.gouv.fr">'
@@ -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
src/backend/core/utils.py
+1 -1
View File
@@ -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
src/backend/meet/settings.py
+8
View File
@@ -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(