diff --git a/baseapp/files/README.md b/baseapp/files/README.md index cd9d4f68..ecabba02 100644 --- a/baseapp/files/README.md +++ b/baseapp/files/README.md @@ -56,6 +56,9 @@ This adds the following fields to the File model: - `uploaded_parts` - JSON tracking ETags - `upload_expires_at` - Expiration timestamp +`file_size` is a `PositiveBigIntegerField`, so files over 2 GB fit. A project with its own concrete +`File` model needs `python manage.py makemigrations files` after upgrading to pick that up. + ### 2. URLs Already Configured The URLs are automatically included in your project via `testproject/urls.py`. diff --git a/baseapp/files/UPLOAD_FLOW.md b/baseapp/files/UPLOAD_FLOW.md index 82e3854c..7e2e57d3 100644 --- a/baseapp/files/UPLOAD_FLOW.md +++ b/baseapp/files/UPLOAD_FLOW.md @@ -189,9 +189,10 @@ Without it uploads fail in the browser with a CORS error and nothing reaches the ### Common Errors - **400 Bad Request**: Invalid parameters, part numbers, or file size +- **400 Bad Request** on complete: the assembled file's size differs from the declared `file_size` (e.g. a part carried extra bytes). The stored object is deleted and the upload is not completed. - **403 Forbidden**: User doesn't own the file/upload - **404 Not Found**: File or upload not found -- **413 Payload Too Large**: File exceeds `MAX_FILE_UPLOAD_SIZE` +- **400 Bad Request** `{"file_size": ["File is too large. The maximum size is 5.0 GB."]}`: File exceeds `MAX_FILE_UPLOAD_SIZE`. Clients can show this message as is. ### Upload Status - `pending`: Upload initiated but not started diff --git a/baseapp/files/models.py b/baseapp/files/models.py index 7ad9b3a4..031fcdb3 100644 --- a/baseapp/files/models.py +++ b/baseapp/files/models.py @@ -300,7 +300,7 @@ class AbstractFile(*file_inheritances, DocumentIdMixin, RelayModel, TimeStampedM verbose_name=_("file name"), help_text=_("Original name of the uploaded file"), ) - file_size = models.PositiveIntegerField( + file_size = models.PositiveBigIntegerField( null=True, verbose_name=_("file size"), help_text=_("File size in bytes"), diff --git a/baseapp/files/rest_framework/uploads/serializers.py b/baseapp/files/rest_framework/uploads/serializers.py index f323b953..6802054a 100644 --- a/baseapp/files/rest_framework/uploads/serializers.py +++ b/baseapp/files/rest_framework/uploads/serializers.py @@ -1,4 +1,6 @@ import swapper +from django.conf import settings +from django.template.defaultfilters import filesizeformat from django.utils.translation import gettext_lazy as _ from rest_framework import serializers @@ -19,6 +21,20 @@ class InitiateUploadSerializer(serializers.Serializer): # Optional parent relationship via DocumentId public_id parent_id = serializers.UUIDField(required=False, allow_null=True) + def validate_file_size(self, value: int) -> int: + """Reject a declared size above MAX_FILE_UPLOAD_SIZE, naming the limit. + + Validated here, not only in UploadService, so the client gets the reason + instead of the view's generic "Invalid upload parameters". + """ + max_file_size = getattr(settings, "MAX_FILE_UPLOAD_SIZE", 5 * 1024 * 1024 * 1024) + if value > max_file_size: + raise serializers.ValidationError( + _("File is too large. The maximum size is %(max)s.") + % {"max": filesizeformat(max_file_size)} + ) + return value + def validate_parent_id(self, value): """Validate parent_id exists as a DocumentId public_id.""" if not value: diff --git a/baseapp/files/services/upload_service.py b/baseapp/files/services/upload_service.py index 4c6ee531..fd695b0c 100644 --- a/baseapp/files/services/upload_service.py +++ b/baseapp/files/services/upload_service.py @@ -3,6 +3,7 @@ import swapper from django.conf import settings +from django.core.files.storage import default_storage from django.db import transaction from django.utils import timezone @@ -109,6 +110,7 @@ def complete_multipart_upload(self, file_id: int, parts: list): # Complete with storage handler (network I/O — outside the transaction) try: file_path = self.handler.complete_upload(file_obj, file_obj.upload_id, parts) + self._verify_stored_size(file_obj, file_path) except Exception: File.objects.filter(id=file_id).update(upload_status=File.UploadStatus.FAILED) raise @@ -152,6 +154,26 @@ def abort_multipart_upload(self, file_id: int): locked.save() return locked + def _verify_stored_size(self, file_obj, file_path: str) -> None: + """Reject an assembled file whose bytes differ from the declared size. + + MAX_FILE_UPLOAD_SIZE is checked against the declared ``file_size`` at + initiation, but neither the local part endpoint nor a presigned S3 part URL + bounds how many bytes are actually sent. Both handlers store through + ``default_storage`` (it is what selects the handler), so it can measure + either. An oversized object is deleted rather than kept. + """ + stored_size = default_storage.size(file_path) + if stored_size == file_obj.file_size: + return + try: + default_storage.delete(file_path) + except Exception: + logger.exception("Could not delete mismatched upload %s", file_path) + raise ValueError( + f"Stored size {stored_size} does not match declared size {file_obj.file_size}" + ) + def _validate_file_params(self, file_size, num_parts, part_size): """Validate file upload parameters.""" max_file_size = getattr(settings, "MAX_FILE_UPLOAD_SIZE", 5 * 1024 * 1024 * 1024) # 5GB diff --git a/baseapp/files/tests/test_graphql.py b/baseapp/files/tests/test_graphql.py index 5350a419..a3a5dda0 100644 --- a/baseapp/files/tests/test_graphql.py +++ b/baseapp/files/tests/test_graphql.py @@ -1,8 +1,10 @@ import pytest import swapper from django.contrib.contenttypes.models import ContentType +from django.test import Client as DjangoClient from baseapp_comments.tests.factories import CommentFactory +from baseapp_core.graphql.testing.fixtures import GraphQLClient from baseapp_core.models import DocumentId File = swapper.load_model("baseapp_files", "File") @@ -795,3 +797,31 @@ def test_file_node_query_returns_file_for_owner(graphql_user_client, django_user assert "errors" not in content assert content["data"]["file"]["id"] == file.relay_id assert content["data"]["file"]["fileName"] == "mine.txt" + + +FILE_SIZE_QUERY = """ + query FileSize($id: ID!) { + node(id: $id) { + ... on File { + fileSize + } + } + } +""" + + +def test_file_size_larger_than_32_bit_int( + graphql_user_client: GraphQLClient, django_user_client: DjangoClient +) -> None: + file_size = 3 * 1024 * 1024 * 1024 + file = File.objects.create( + file_name="long-video.mp4", + file_size=file_size, + upload_status=File.UploadStatus.COMPLETED, + created_by=django_user_client.user, + ) + + content = graphql_user_client(FILE_SIZE_QUERY, variables={"id": file.relay_id}).json() + + assert "errors" not in content + assert content["data"]["node"]["fileSize"] == file_size diff --git a/baseapp/files/tests/test_presigned_uploads.py b/baseapp/files/tests/test_presigned_uploads.py index b6fa67d1..1de96777 100644 --- a/baseapp/files/tests/test_presigned_uploads.py +++ b/baseapp/files/tests/test_presigned_uploads.py @@ -8,12 +8,11 @@ from datetime import timedelta from typing import Any, Dict -from unittest.mock import patch from urllib.parse import urlparse import pytest import swapper -from django.conf import settings +from django.test import override_settings from django.utils import timezone from freezegun import freeze_time from rest_framework import status @@ -29,8 +28,11 @@ @pytest.fixture def temp_media_root(tmp_path): - """Use a temporary directory for media root.""" - with patch.object(settings, "MEDIA_ROOT", str(tmp_path)): + """Use a temporary directory for media root. + + override_settings (not patch.object) so default_storage drops its cached location. + """ + with override_settings(MEDIA_ROOT=str(tmp_path)): yield tmp_path @@ -99,6 +101,26 @@ def test_full_upload_flow(self, client, user_client, temp_media_root, initiated_ assert file_obj.upload_status == File.UploadStatus.COMPLETED assert (temp_media_root / file_obj.file.name).read_bytes() == PART_DATA + def test_complete_rejects_more_bytes_than_declared( + self, client, user_client, temp_media_root, initiated_upload + ) -> None: + """Declaring a small size and PUTting more must not yield a COMPLETED, oversized file.""" + path = presigned_path(initiated_upload["presigned_urls"][0]) + etag = put_part(client, path, data=PART_DATA * 4).json()["etag"] + + response = user_client.post( + f"/v1/files/uploads/{initiated_upload['id']}/complete", + {"parts": [{"part_number": 1, "etag": etag}]}, + format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + file_obj = File.get_by_public_id(initiated_upload["id"]) + # ATOMIC_REQUESTS rolls back the FAILED flag with the 400; what matters is no COMPLETED file. + assert file_obj.upload_status != File.UploadStatus.COMPLETED + assert not file_obj.file + assert not list((temp_media_root / "files").rglob("*")) + def test_missing_token(self, client, initiated_upload): """A request without a token is rejected.""" path = presigned_path(initiated_upload["presigned_urls"][0]).split("?")[0] diff --git a/baseapp/files/tests/test_rest_api.py b/baseapp/files/tests/test_rest_api.py index c41fa27a..ea0d870b 100644 --- a/baseapp/files/tests/test_rest_api.py +++ b/baseapp/files/tests/test_rest_api.py @@ -5,6 +5,7 @@ import swapper from django.contrib.auth import get_user_model from django.contrib.contenttypes.models import ContentType +from django.test import override_settings from django.utils import timezone from rest_framework import status @@ -210,6 +211,48 @@ def test_initiate_upload_file_size_mismatch(self, user_client, mock_s3_handler): error_message = str(response_data) assert "doesn't match" in error_message + @override_settings(MAX_FILE_UPLOAD_SIZE=10 * 1024 * 1024) + def test_initiate_upload_too_large_explains_the_limit( + self, user_client: Client, mock_s3_handler: MagicMock + ) -> None: + response = user_client.post( + "/v1/files/uploads", + { + "file_name": "big.pdf", + "file_size": 15 * 1024 * 1024, + "file_content_type": "application/pdf", + "num_parts": 3, + "part_size": 5242880, + }, + format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == { + "file_size": ["File is too large. The maximum size is 10.0\xa0MB."] + } + mock_s3_handler.initiate_upload.assert_not_called() + + def test_initiate_upload_larger_than_32_bit_size( + self, user_client: Client, mock_s3_handler: MagicMock + ) -> None: + file_size = 3 * 1024 * 1024 * 1024 + response = user_client.post( + "/v1/files/uploads", + { + "file_name": "long-video.mp4", + "file_size": file_size, + "file_content_type": "video/mp4", + "num_parts": 31, + "part_size": 100 * 1024 * 1024, + }, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + file_obj = File.get_by_public_id(response.json()["id"]) + assert file_obj.file_size == file_size + def test_initiate_upload_too_many_parts(self, user_client, mock_s3_handler): """Test validation for maximum parts (10,000).""" response = user_client.post( @@ -264,6 +307,36 @@ def pending_upload(self, user_client): upload_expires_at=timezone.now() + timedelta(hours=24), ) + @pytest.fixture(autouse=True) + def stored_storage(self): + """The handler is mocked, so nothing is stored; report the declared size by default.""" + with patch("baseapp.files.services.upload_service.default_storage") as storage: + storage.size.return_value = 10485760 + yield storage + + def test_complete_upload_rejects_bytes_beyond_the_declared_size( + self, + user_client: Client, + pending_upload: "File", + mock_s3_handler: MagicMock, + stored_storage: MagicMock, + ) -> None: + """A client that declares a small size and sends more must not get a COMPLETED file.""" + stored_storage.size.return_value = pending_upload.file_size + 1 + + response = user_client.post( + f"/v1/files/uploads/{pending_upload.public_id}/complete", + {"parts": [{"part_number": 1, "etag": "a"}, {"part_number": 2, "etag": "b"}]}, + format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + stored_storage.delete.assert_called_once_with("files/test-file.mp4") + pending_upload.refresh_from_db() + # ATOMIC_REQUESTS rolls back the FAILED flag with the 400; what matters is no COMPLETED file. + assert pending_upload.upload_status != File.UploadStatus.COMPLETED + assert not pending_upload.file + def test_complete_upload_success(self, user_client, pending_upload, mock_s3_handler): """Test successful upload completion.""" response = user_client.post( diff --git a/testproject/files/migrations/0004_file_size_big_integer.py b/testproject/files/migrations/0004_file_size_big_integer.py new file mode 100644 index 00000000..1d0d828f --- /dev/null +++ b/testproject/files/migrations/0004_file_size_big_integer.py @@ -0,0 +1,20 @@ +# Generated by Django 5.2.17 on 2026-10-13 00:20 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("files", "0003_alter_file_options_alter_filetarget_options_and_more"), + ] + + operations = [ + migrations.AlterField( + model_name="file", + name="file_size", + field=models.PositiveBigIntegerField( + help_text="File size in bytes", null=True, verbose_name="file size" + ), + ), + ]