diff --git a/backend/apps/library/signing_urls.py b/backend/apps/library/signing_urls.py index 71bd0c1..a3da44c 100644 --- a/backend/apps/library/signing_urls.py +++ b/backend/apps/library/signing_urls.py @@ -36,18 +36,25 @@ def sign_payload(payload, salt, now=None): def load_payload(signature, salt, legacy_max_age=86400): """Verify a signed payload; ``None`` when missing, tampered with or expired. - Signatures minted before the stable scheme (``TimestampSigner``) are still - accepted for one release so pages open across the deploy keep working. + Legacy ``TimestampSigner`` values are still accepted for one release. + Detect them by their extra separator (``payload:timestamp:signature``): + a plain ``Signer`` accepts the HMAC a ``TimestampSigner`` computed over + ``payload:timestamp`` and then chokes on the embedded timestamp while + decoding the JSON payload, which used to surface as a 500. """ - try: - data = signing.Signer(salt=salt).unsign_object(signature) - except signing.BadSignature: + if not signature: + return None + if signature.count(":") >= 2: try: return signing.TimestampSigner(salt=salt).unsign_object( signature, max_age=legacy_max_age ) - except signing.BadSignature: + except (signing.BadSignature, ValueError): return None + try: + data = signing.Signer(salt=salt).unsign_object(signature) + except (signing.BadSignature, ValueError): + return None if not isinstance(data, dict): return None try: diff --git a/backend/apps/library/tests/test_media_cache.py b/backend/apps/library/tests/test_media_cache.py index d2b5644..03fd816 100644 --- a/backend/apps/library/tests/test_media_cache.py +++ b/backend/apps/library/tests/test_media_cache.py @@ -12,8 +12,10 @@ import shutil import tempfile import time from pathlib import Path +from unittest import mock from django.contrib.auth import get_user_model +from django.core import signing from django.core.files.uploadedfile import SimpleUploadedFile from django.test import Client, TestCase, override_settings @@ -23,7 +25,7 @@ from rest_framework.authtoken.models import Token from apps.library import services from apps.library.models import MediaItem, MediaLocation, TempUpload -from apps.library.signing_urls import sign_payload +from apps.library.signing_urls import load_payload, sign_payload User = get_user_model() @@ -137,3 +139,29 @@ class MediaCacheTests(TestCase): f"max-age={services.TEMP_CACHE_SECONDS}", response["Cache-Control"] ) self.assertNotIn("immutable", response["Cache-Control"]) + + def test_legacy_timestamp_signatures_are_accepted(self): + """URLs minted before the stable scheme must not 500. + + A TimestampSigner HMAC also passes a plain Signer's check, so the + embedded timestamp used to reach the JSON decoder and blow up. + """ + payload = {"item": self.item.id, "user": self.user.id, "action": "raw"} + legacy = signing.dumps(payload, salt=services.MEDIA_FILE_SALT) + self.assertEqual( + load_payload(legacy, services.MEDIA_FILE_SALT)["item"], self.item.id + ) + response = self.client.get( + f"/api/files/J-{self.item.id}/raw/", {"sig": legacy} + ) + self.assertEqual(response.status_code, 200) + + def test_expired_and_malformed_signatures_return_none(self): + payload = {"item": self.item.id, "user": self.user.id, "action": "raw"} + with mock.patch.object(signing, "time") as clock: + clock.time.return_value = time.time() - 3 * 86400 + expired = signing.dumps(payload, salt=services.MEDIA_FILE_SALT) + self.assertIsNone(load_payload(expired, services.MEDIA_FILE_SALT)) + self.assertIsNone(load_payload("bogus", services.MEDIA_FILE_SALT)) + self.assertIsNone(load_payload("a:b", services.MEDIA_FILE_SALT)) + self.assertIsNone(load_payload("", services.MEDIA_FILE_SALT))