diff --git a/CHANGELOG.md b/CHANGELOG.md index 86c286a5e..53dd6ff37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ and this project adheres to - 🐛(backend) ignore CSPs for API docs in development - 🐛(frontend) export images embedded with a relative url #2573 - 🐛(y-provider) fix sentry init #2579 +- 🐛(backend) handle object storage metadata keys case-insensitively #2576 - 🐛(keycloak) fix database env variables in the self-hosting example #2572 - 🐛(helm) show the database error while jobs wait for it to be ready #2578 diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 5d9991bcb..e63fb726d 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -71,6 +71,7 @@ from core.services.search_indexers import ( from core.tasks.access import reset_service_connections_in_cascade from core.tasks.mail import send_ask_for_access_mail from core.utils.analytics import PosthogEventName, posthog_capture +from core.utils.dicts import lowercase_keys from core.utils.paths import filter_descendants from core.utils.s3_response_stream import content_stream from core.utils.treebeard import create_tree_node_with_retry @@ -1884,7 +1885,7 @@ class DocumentViewSet( extra_args = { "Metadata": { "owner": str(request.user.id), - "status": enums.DocumentAttachmentStatus.PROCESSING, + "status": enums.DocumentAttachmentStatus.PROCESSING.value, }, "ContentType": serializer.validated_data["content_type"], } @@ -2034,7 +2035,7 @@ class DocumentViewSet( head_resp = s3_client.head_object(Bucket=bucket_name, Key=key) except ClientError as err: raise drf.exceptions.PermissionDenied() from err - metadata = head_resp.get("Metadata", {}) + metadata = lowercase_keys(head_resp.get("Metadata", {})) # In order to be compatible with existing upload without `status` metadata, # we consider them as ready. if ( @@ -2238,7 +2239,7 @@ class DocumentViewSet( {"detail": "Media not found"}, status=drf.status.HTTP_404_NOT_FOUND, ) - metadata = head_resp.get("Metadata", {}) + metadata = lowercase_keys(head_resp.get("Metadata", {})) body = { "status": metadata.get("status", enums.DocumentAttachmentStatus.PROCESSING), diff --git a/src/backend/core/malware_detection.py b/src/backend/core/malware_detection.py index 9b1ef3a72..e126fed1f 100644 --- a/src/backend/core/malware_detection.py +++ b/src/backend/core/malware_detection.py @@ -8,6 +8,7 @@ from lasuite.malware_detection.enums import ReportStatus from core.enums import DocumentAttachmentStatus from core.models import Document +from core.utils.dicts import lowercase_keys logger = logging.getLogger(__name__) security_logger = logging.getLogger("docs.security") @@ -22,8 +23,8 @@ def malware_detection_callback(file_path, status, error_info, **kwargs): s3_client = default_storage.connection.meta.client bucket_name = default_storage.bucket_name head_resp = s3_client.head_object(Bucket=bucket_name, Key=file_path) - metadata = head_resp.get("Metadata", {}) - metadata.update({"status": DocumentAttachmentStatus.READY}) + metadata = lowercase_keys(head_resp.get("Metadata", {})) + metadata.update({"status": DocumentAttachmentStatus.READY.value}) # Update status in metadata s3_client.copy_object( Bucket=bucket_name, diff --git a/src/backend/core/management/commands/update_files_content_type_metadata.py b/src/backend/core/management/commands/update_files_content_type_metadata.py index bb2e5253a..2373dfb16 100644 --- a/src/backend/core/management/commands/update_files_content_type_metadata.py +++ b/src/backend/core/management/commands/update_files_content_type_metadata.py @@ -6,6 +6,7 @@ from django.core.management.base import BaseCommand import magic from core.models import Document +from core.utils.dicts import lowercase_keys # pylint: disable=too-many-locals, broad-exception-caught @@ -74,7 +75,7 @@ class Command(BaseCommand): CopySource={"Bucket": bucket_name, "Key": key}, Key=key, ContentType=magic_mime_type, - Metadata=head_resp.get("Metadata", {}), + Metadata=lowercase_keys(head_resp.get("Metadata", {})), MetadataDirective="REPLACE", ) total_updated += 1 diff --git a/src/backend/core/tests/documents/test_api_documents_media_auth.py b/src/backend/core/tests/documents/test_api_documents_media_auth.py index 35ea8b2b6..40514dad2 100644 --- a/src/backend/core/tests/documents/test_api_documents_media_auth.py +++ b/src/backend/core/tests/documents/test_api_documents_media_auth.py @@ -3,6 +3,7 @@ Test media-auth authorization API endpoint in docs core app. """ from io import BytesIO +from unittest.mock import patch from urllib.parse import urlparse from uuid import uuid4 @@ -369,6 +370,34 @@ def test_api_documents_media_auth_not_ready_status(): assert response.status_code == 403 +def test_api_documents_media_auth_uppercase_status_metadata(): + """ + Object storage metadata keys are case insensitive, and some S3 implementations give them + back capitalized. A ready attachment should still be served in that case. + """ + document_id = uuid4() + filename = f"{uuid4()!s}.jpg" + key = f"{document_id!s}/attachments/{filename:s}" + + factories.DocumentFactory(id=document_id, link_reach="public", attachments=[key]) + + head_resp = { + "ContentType": "text/plain", + "Metadata": {"Status": DocumentAttachmentStatus.READY.value}, + } + + original_url = f"http://localhost/media/{key:s}" + with patch.object( + default_storage.connection.meta.client, "head_object", return_value=head_resp + ): + response = APIClient().get( + "/api/v1.0/documents/media-auth/", HTTP_X_ORIGINAL_URL=original_url + ) + + assert response.status_code == 200 + assert "AWS4-HMAC-SHA256 Credential=" in response["Authorization"] + + def test_api_documents_media_auth_missing_status_metadata(settings): """Attachments without status metadata should be considered as ready""" document_id = uuid4() diff --git a/src/backend/core/tests/documents/test_api_documents_media_check.py b/src/backend/core/tests/documents/test_api_documents_media_check.py index 81cec061e..b5a472c97 100644 --- a/src/backend/core/tests/documents/test_api_documents_media_check.py +++ b/src/backend/core/tests/documents/test_api_documents_media_check.py @@ -1,6 +1,7 @@ """Test the "media_check" endpoint.""" from io import BytesIO +from unittest.mock import patch from uuid import uuid4 from django.core.files.storage import default_storage @@ -116,6 +117,39 @@ def test_api_documents_media_check_anonymous_public_document_ready(): } +def test_api_documents_media_check_uppercase_status_metadata(): + """ + Object storage metadata keys are case insensitive, and some S3 implementations give them + back capitalized. The "media_check" endpoint should still report the attachment as ready. + """ + document = factories.DocumentFactory(link_reach="public") + + filename = f"{uuid4()!s}.jpg" + key = f"{document.id!s}/attachments/{filename:s}" + document.attachments = [key] + document.save(update_fields=["attachments"]) + + head_resp = { + "ContentType": "text/plain", + "Metadata": {"Status": DocumentAttachmentStatus.READY.value}, + } + + client = APIClient() + + with patch.object( + default_storage.connection.meta.client, "head_object", return_value=head_resp + ): + response = client.get( + f"/api/v1.0/documents/{document.id!s}/media-check/", {"key": key} + ) + + assert response.status_code == 200 + assert response.json() == { + "status": DocumentAttachmentStatus.READY, + "file": f"/media/{key:s}", + } + + @pytest.mark.parametrize("link_reach", ["restricted", "authenticated"]) def test_api_documents_media_check_anonymous_non_public_document(link_reach): """ diff --git a/src/backend/core/tests/test_malware_detection.py b/src/backend/core/tests/test_malware_detection.py index 57da7643d..5fb017ad6 100644 --- a/src/backend/core/tests/test_malware_detection.py +++ b/src/backend/core/tests/test_malware_detection.py @@ -1,6 +1,7 @@ """Test malware detection callback.""" import random +from unittest.mock import patch from django.core.files.base import ContentFile from django.core.files.storage import default_storage @@ -56,6 +57,41 @@ def test_malware_detection_callback_safe_status(safe_file): assert metadata["status"] == DocumentAttachmentStatus.READY +def test_malware_detection_callback_safe_status_uppercase_metadata(safe_file): + """ + Object storage metadata keys are case insensitive, and some S3 implementations give them + back capitalized. The callback should update the existing "status" entry instead of adding + a second one, which would end up as two "x-amz-meta-status" headers on the copy request. + """ + document = DocumentFactory(attachments=[safe_file]) + + s3_client = default_storage.connection.meta.client + head_resp = { + "ContentType": "text/plain", + "Metadata": { + "Owner": str(document.id), + "Status": DocumentAttachmentStatus.PROCESSING.value, + }, + } + + with ( + patch.object(s3_client, "head_object", return_value=head_resp), + patch.object(s3_client, "copy_object") as mock_copy_object, + ): + malware_detection_callback( + safe_file, + ReportStatus.SAFE, + error_info={}, + document_id=document.id, + ) + + metadata = mock_copy_object.call_args.kwargs["Metadata"] + assert metadata == { + "owner": str(document.id), + "status": DocumentAttachmentStatus.READY.value, + } + + def test_malware_detection_callback_unsafe_status(unsafe_file): """Test malware detection callback with unsafe status.""" diff --git a/src/backend/core/tests/test_utils.py b/src/backend/core/tests/test_utils.py index 33c7c643e..99c2f4d2b 100644 --- a/src/backend/core/tests/test_utils.py +++ b/src/backend/core/tests/test_utils.py @@ -9,7 +9,7 @@ import pycrdt import pytest from core import factories -from core.utils.dicts import get_value_by_pattern +from core.utils.dicts import get_value_by_pattern, lowercase_keys from core.utils.paths import get_ancestor_to_descendants_map from core.utils.users import ( get_users_sharing_documents_with_cache_key, @@ -251,3 +251,35 @@ def test_utils_get_value_by_pattern_no_match(): result = get_value_by_pattern(data, r"^title\.") assert result == [] + + +def test_utils_lowercase_keys_empty(): + """Test that an empty dictionary is returned untouched.""" + assert lowercase_keys({}) == {} + + +def test_utils_lowercase_keys_mixed_case(): + """Test that keys are lowercased while values are left untouched.""" + data = {"Owner": "42", "STATUS": "Ready", "is_unsafe": "true"} + + assert lowercase_keys(data) == { + "owner": "42", + "status": "Ready", + "is_unsafe": "true", + } + + +def test_utils_lowercase_keys_collision(): + """Test that keys differing only by their case are collapsed into a single one.""" + data = {"Status": "processing", "status": "ready"} + result = lowercase_keys(data) + + assert result == {"status": "ready"} + + +def test_utils_lowercase_keys_does_not_mutate_source(): + """Test that the source dictionary is left untouched.""" + data = {"Status": "processing"} + lowercase_keys(data) + + assert data == {"Status": "processing"} diff --git a/src/backend/core/utils/dicts.py b/src/backend/core/utils/dicts.py index 8cc7aa4bc..faf8b13fb 100644 --- a/src/backend/core/utils/dicts.py +++ b/src/backend/core/utils/dicts.py @@ -22,3 +22,25 @@ def get_value_by_pattern(data, pattern): """ regex = re.compile(pattern) return [value for key, value in data.items() if regex.match(key)] + + +def lowercase_keys(data): + """ + Get a copy of a dictionary with all its keys lowercased. + + Useful for object storage metadata: keys are case insensitive per the S3 specification, + but implementations don't agree on the case they give back, so reading or updating a + metadata entry by its lowercase name is only reliable after this normalization. + + Args: + data (dict): Source dictionary + + Returns: + dict: New dictionary with lowercased keys. When several keys only differ by their + case, the last one encountered wins. + + Example: + >>> lowercase_keys({"Owner": "42", "Status": "ready"}) + {"owner": "42", "status": "ready"} + """ + return {key.lower(): value for key, value in data.items()}