🔒️(backend) address coderabbit findings + squash migrations

Author: Nastaliss

src/backend/core/api/viewsets/draft.py

@@ -428,7 +428,8 @@ def finalize(self, request, pk=None):
                 )
 
             # Antivirus gate (fail closed): a draft only becomes a transfer once
-            # every file is CLEAN. Two hard blocks: a virus, and a file that
+            # every file has a non-blocking status — CLEAN, or scan-exempt
+            # (SKIPPED / TOO_LARGE). Two hard blocks: a virus, and a file that
             # can't be scanned (error_kind="file") — a retry won't help, the
             # user must remove it. A *transient* error (clamd/scanner hiccup) is
             # re-submitted and kept polling so a passing failure doesn't brick

src/backend/core/api/viewsets/webhook.py

@@ -4,8 +4,9 @@
 The endpoint is unauthenticated in the Django sense (the scanner has no
 account) but protected by a per-file opaque secret minted at submission time
 and echoed back in the query string — compared in constant time before any
-state change. The handler is idempotent: replaying the same callback (e.g. a
-scanner retry) simply re-writes the same ``scan_status``.
+state change. Updates are guarded to PENDING files, so a terminal verdict is
+final: a replayed or duplicate callback (a scanner retry, or a second scan job
+from the reaper) is a 200 no-op rather than an overwrite.
 """
 
 import hmac
@@ -60,9 +61,20 @@ def post(self, request):
         new_status = self._status_from_payload(payload)
         error_kind = self._error_kind_from_payload(payload, new_status)
 
-        models.TransferFile.objects.filter(id=transfer_file.id).update(
-            scan_status=new_status, scan_error_kind=error_kind
-        )
+        # Only a PENDING file is mutable — every legitimate result lands while a
+        # scan is in flight. Guarding on it makes a terminal verdict final, so a
+        # stale or duplicate callback (scanner retry, or a second reaper job)
+        # can't overwrite it: fail closed, a virus stays a virus.
+        updated = models.TransferFile.objects.filter(
+            id=transfer_file.id, scan_status=ScanStatus.PENDING
+        ).update(scan_status=new_status, scan_error_kind=error_kind)
+        if not updated:
+            logger.info(
+                "Scan result for file %s ignored — already %s",
+                file_id,
+                transfer_file.scan_status,
+            )
+            return Response(status=200)
         logger.info(
             "Scan result for file %s: %s%s",
             file_id,

src/backend/core/migrations/0004_transferfile_scan_fields.py

@@ -0,0 +1,52 @@
+# Generated by Django 5.2.11 on 2026-06-23 09:21
+
+from django.db import migrations, models
+
+
+def grandfather_existing_files_clean(apps, schema_editor):
+    """Existing *uploaded* files predate antivirus scanning — mark them CLEAN
+    so they stay downloadable. Scope to ``upload_completed_at`` so an in-flight
+    draft (upload not finished) keeps the PENDING default and is scanned for
+    real when it later completes; otherwise it would slip through complete_upload
+    as CLEAN, never scanned."""
+    TransferFile = apps.get_model("core", "TransferFile")
+    TransferFile.objects.filter(upload_completed_at__isnull=False).update(
+        scan_status="clean"
+    )
+
+
+def noop_reverse(apps, schema_editor):
+    pass
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('core', '0004_alter_user_sub'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='transferfile',
+            name='scan_error_kind',
+            field=models.CharField(blank=True, default='', help_text="Set only when scan_status is ERROR. 'transient' = an infrastructure failure that a retry may clear; 'file' = the file itself can't be scanned, so the user must remove it.", max_length=10),
+        ),
+        migrations.AddField(
+            model_name='transferfile',
+            name='scan_job_id',
+            field=models.CharField(blank=True, default='', help_text='Job id returned by the file-scanner service when the scan was submitted. Kept for traceability / debugging.', max_length=64),
+        ),
+        migrations.AddField(
+            model_name='transferfile',
+            name='scan_status',
+            field=models.CharField(choices=[('pending', 'Pending'), ('clean', 'Clean'), ('infected', 'Infected'), ('error', 'Error'), ('skipped', 'Skipped'), ('too_large', 'Too large to scan')], default='pending', help_text="Antivirus scan state. A file is downloadable when CLEAN or scan-exempt (SKIPPED / TOO_LARGE); the download path fails closed on anything else. Driven by the clamav file-scanner service's webhook callback.", max_length=10),
+        ),
+        migrations.AddField(
+            model_name='transferfile',
+            name='webhook_secret',
+            field=models.CharField(blank=True, default='', help_text='Per-file opaque token embedded in the scan callback URL. The scanner echoes it back; the webhook compares it (constant-time) before trusting the result. Generated when the scan is submitted.', max_length=64),
+        ),
+        migrations.RunPython(
+            grandfather_existing_files_clean, reverse_code=noop_reverse
+        ),
+    ]

src/backend/core/migrations/0005_alter_transferfile_scan_status.py

@@ -429,9 +429,10 @@ class TransferFile(BaseModel):
         max_length=10,
         choices=ScanStatus.choices,
         default=ScanStatus.PENDING,
-        help_text="Antivirus scan state. A file is only downloadable once it "
-        "is CLEAN; the download path fails closed on anything else. Driven by "
-        "the clamav file-scanner service's webhook callback.",
+        help_text="Antivirus scan state. A file is downloadable when CLEAN or "
+        "scan-exempt (SKIPPED / TOO_LARGE); the download path fails closed on "
+        "anything else. Driven by the clamav file-scanner service's webhook "
+        "callback.",
     )
     scan_error_kind = models.CharField(
         max_length=10,

src/backend/core/migrations/0005_transferfile_scan_fields.py

@@ -98,13 +98,27 @@ def test_error_with_bogus_kind_defaults_transient(self, api_client):
         f.refresh_from_db()
         assert f.scan_error_kind == "transient"
 
-    def test_recovery_clears_stale_kind(self, api_client):
-        # A file that previously errored (file kind) comes back CLEAN on a
-        # re-scan — the stale kind must be wiped, not left dangling.
-        f = self._file(scan_status=ScanStatus.ERROR, scan_error_kind="file")
+    def test_terminal_state_not_overwritten(self, api_client):
+        # Once a file reaches a terminal verdict it is no longer PENDING, so a
+        # stale or duplicate callback must not move it (fail closed).
+        f = self._file(scan_status=ScanStatus.INFECTED)
         resp = _post(api_client, f.id, "s3cr3t", {"status": "done", "malware": False})
         assert resp.status_code == 200
         f.refresh_from_db()
+        assert f.scan_status == ScanStatus.INFECTED
+
+    def test_duplicate_callback_cannot_flip_clean(self, api_client):
+        # A second scan job (e.g. a reaper re-submit after a slow webhook) that
+        # reports an error must not unset an already-CLEAN file.
+        f = self._file(scan_status=ScanStatus.CLEAN)
+        resp = _post(
+            api_client,
+            f.id,
+            "s3cr3t",
+            {"status": "error", "error_kind": "transient"},
+        )
+        assert resp.status_code == 200
+        f.refresh_from_db()
         assert f.scan_status == ScanStatus.CLEAN
         assert f.scan_error_kind == ""
 

src/backend/core/migrations/0006_alter_transferfile_scan_status.py

@@ -8,6 +8,7 @@
 import dj_database_url
 import sentry_sdk
 from configurations import Configuration, values
+from django.core.exceptions import ImproperlyConfigured
 from sentry_sdk.integrations.django import DjangoIntegration
 
 logger = logging.getLogger(__name__)
@@ -603,6 +604,24 @@ def post_setup(cls):
                 f"must be one of TRANSFER_EXPIRY_CHOICES ({cls.TRANSFER_EXPIRY_CHOICES})."
             )
 
+        # Fail fast: scanning enabled but its endpoints/credentials unset would
+        # otherwise surface only later, as silently-skipped scans in the worker.
+        if cls.CLAMAV_SCAN_ENABLED:
+            missing = [
+                name
+                for name in (
+                    "CLAMAV_SERVICE_URL",
+                    "CLAMAV_API_KEY",
+                    "SCAN_WEBHOOK_BASE_URL",
+                )
+                if not getattr(cls, name)
+            ]
+            if missing:
+                raise ImproperlyConfigured(
+                    "CLAMAV_SCAN_ENABLED is set but these required settings are "
+                    f"empty: {', '.join(missing)}."
+                )
+
 
 class Build(Base):
     """Settings for building the application (not for running)."""