Validate release-file URL metadata (#3014)

Author: e-q

apps/downloads/api.py

@@ -1,12 +1,14 @@
 """REST API endpoints for downloads using Tastypie and Django REST Framework."""
 
+from django.core.exceptions import ValidationError as DjangoValidationError
 from rest_framework import status, viewsets
 from rest_framework.authentication import TokenAuthentication
 from rest_framework.decorators import action
 from rest_framework.response import Response
 from tastypie import fields
 from tastypie.constants import ALL, ALL_WITH_RELATIONS
 from tastypie.exceptions import BadRequest
+from tastypie.validation import Validation
 
 from apps.downloads.models import OS, Release, ReleaseFile
 from apps.downloads.serializers import OSSerializer, ReleaseFileSerializer, ReleaseSerializer
@@ -15,6 +17,20 @@
 from pydotorg.resources import GenericResource, OnlyPublishedAuthorization
 
 
+class ReleaseFileValidation(Validation):
+    """Tastypie validation for release-file URL relationships."""
+
+    def is_valid(self, bundle, request=None):
+        """Return validation errors for hydrated release-file writes."""
+        if bundle.obj is None:
+            return {}
+        try:
+            bundle.obj.clean()
+        except DjangoValidationError as exc:
+            return exc.message_dict
+        return {}
+
+
 class OSResource(GenericResource):
     """Tastypie resource for operating systems."""
 
@@ -90,6 +106,7 @@ class Meta(GenericResource.Meta):
         queryset = ReleaseFile.objects.all()
         resource_name = "downloads/release_file"
         list_allowed_methods = ["get", "post", "delete"]
+        validation = ReleaseFileValidation()
         fields = [
             "name",
             "slug",

apps/downloads/models.py

@@ -19,6 +19,24 @@
 from fastly.utils import purge_surrogate_key, purge_url
 
 DEFAULT_MARKUP_TYPE = getattr(settings, "DEFAULT_MARKUP_TYPE", "markdown")
+PYTHON_DOT_ORG_HTTPS_PREFIX = "https://www.python.org/"
+PYTHON_DOT_ORG_HTTP_PREFIX = "http://www.python.org/"
+RELEASE_FILE_URL_FIELDS = (
+    "url",
+    "gpg_signature_file",
+    "sigstore_signature_file",
+    "sigstore_cert_file",
+    "sigstore_bundle_file",
+    "sbom_spdx2_file",
+)
+RELEASE_FILE_SIDECAR_SUFFIXES = {
+    "gpg_signature_file": ".asc",
+    "sigstore_signature_file": ".sig",
+    "sigstore_cert_file": ".crt",
+    "sigstore_bundle_file": ".sigstore",
+    "sbom_spdx2_file": ".spdx.json",
+}
+RELEASE_FILE_HTTPS_ERROR = "Release file URLs must begin with 'https://www.python.org/'."
 
 
 class OS(ContentManageable, NameSlugModel):
@@ -361,13 +379,46 @@ def condition_url_is_blank_or_python_dot_org(column: str):
     """Conditions for a URLField column to force 'http[s]://python.org'."""
     return (
         models.Q(**{f"{column}__exact": ""})
-        | models.Q(**{f"{column}__startswith": "https://www.python.org/"})
+        | models.Q(**{f"{column}__startswith": PYTHON_DOT_ORG_HTTPS_PREFIX})
         # Older releases allowed 'http://'. 'https://' is required at
         # the API level, so shouldn't show up in newer releases.
-        | models.Q(**{f"{column}__startswith": "http://www.python.org/"})
+        | models.Q(**{f"{column}__startswith": PYTHON_DOT_ORG_HTTP_PREFIX})
     )
 
 
+def validate_release_file_urls(release_file):
+    """Validate current ReleaseFile URL writes without rejecting unchanged legacy rows."""
+    values = {}
+    for field_name in RELEASE_FILE_URL_FIELDS:
+        values[field_name] = getattr(release_file, field_name) or ""
+
+    previous_values = None
+    if release_file.pk is not None:
+        release_file_model = type(release_file)
+        previous_values_qs = release_file_model.objects.filter(pk=release_file.pk)
+        previous_values = previous_values_qs.values(*RELEASE_FILE_URL_FIELDS).first()
+    errors = {}
+
+    for field_name, value in values.items():
+        if not value or value.startswith(PYTHON_DOT_ORG_HTTPS_PREFIX):
+            continue
+        if previous_values is None or value != (previous_values[field_name] or ""):
+            errors.setdefault(field_name, []).append(RELEASE_FILE_HTTPS_ERROR)
+
+    artifact_url = values["url"]
+    if artifact_url:
+        for field_name, suffix in RELEASE_FILE_SIDECAR_SUFFIXES.items():
+            sidecar_url = values[field_name]
+            expected_url = f"{artifact_url}{suffix}"
+            if not sidecar_url or sidecar_url == expected_url:
+                continue
+            message = f"Sidecar URL must match the artifact URL plus '{suffix}'."
+            errors.setdefault(field_name, []).append(message)
+
+    if errors:
+        raise ValidationError(errors)
+
+
 class ReleaseFile(ContentManageable, NameSlugModel):
     """Individual files in a release.
 
@@ -395,6 +446,11 @@ class ReleaseFile(ContentManageable, NameSlugModel):
     filesize = models.IntegerField(default=0)
     download_button = models.BooleanField(default=False, help_text="Use for the supernav download button for this OS")
 
+    def clean(self):
+        """Validate release-file URL relationships."""
+        super().clean()
+        validate_release_file_urls(self)
+
     def validate_unique(self, exclude=None):
         """Ensure only one release file per OS has the download button enabled."""
         if self.download_button and self.release_id:

apps/downloads/serializers.py

@@ -1,5 +1,8 @@
 """DRF serializers for the downloads API."""
 
+import copy
+
+from django.core.exceptions import ValidationError as DjangoValidationError
 from rest_framework import serializers
 
 from apps.downloads.models import OS, Release, ReleaseFile
@@ -40,6 +43,24 @@ class Meta:
 class ReleaseFileSerializer(serializers.HyperlinkedModelSerializer):
     """Serializer for release file data."""
 
+    def validate(self, attrs):
+        """Validate release-file URL relationships."""
+        attrs = super().validate(attrs)
+        release_file = self._release_file_for_validation(attrs)
+        try:
+            release_file.clean()
+        except DjangoValidationError as exc:
+            raise serializers.ValidationError(exc.message_dict) from exc
+        return attrs
+
+    def _release_file_for_validation(self, attrs):
+        if self.instance is None:
+            return ReleaseFile(**attrs)
+        release_file = copy.copy(self.instance)
+        for attr, value in attrs.items():
+            setattr(release_file, attr, value)
+        return release_file
+
     class Meta:
         """Meta configuration for ReleaseFileSerializer."""
 

apps/downloads/tests/test_models.py

@@ -1,13 +1,28 @@
 import datetime as dt
 from unittest.mock import patch
 
+from django.core.exceptions import ValidationError
 from django.db import IntegrityError, transaction
 from django.db.models import URLField
 
-from apps.downloads.models import OS, Release, ReleaseFile
+from apps.downloads.models import (
+    OS,
+    RELEASE_FILE_SIDECAR_SUFFIXES,
+    RELEASE_FILE_URL_FIELDS,
+    Release,
+    ReleaseFile,
+)
 from apps.downloads.tests.base import BaseDownloadTests
 
 
+def release_file_url_field_names():
+    return tuple(
+        field.name
+        for field in ReleaseFile._meta.get_fields()  # noqa: SLF001
+        if isinstance(field, URLField)
+    )
+
+
 class DownloadModelTests(BaseDownloadTests):
     def test_stringification(self):
         self.assertEqual(str(self.osx), "macOS")
@@ -300,7 +315,7 @@ def test_release_file_urls_not_python_dot_org(self):
             with self.subTest(field.name), transaction.atomic():
                 kwargs = {
                     "url": "https://www.python.org/ftp/python/9.7.2/python-9.7.2.exe",
-                    # field.name may be 'url', but will replace the default value.
+                    # field.name may be "url", but will replace the default value.
                     field.name: "https://notpython.com/python-9.7.2.txt",
                 }
 
@@ -311,3 +326,96 @@ def test_release_file_urls_not_python_dot_org(self):
                         name="Windows installer draft",
                         **kwargs,
                     )
+
+    def test_release_file_rejects_new_http_urls(self):
+        for field_name in RELEASE_FILE_URL_FIELDS:
+            with self.subTest(field_name):
+                kwargs = {
+                    "url": "https://www.python.org/ftp/python/9.7.2/python-9.7.2.exe",
+                    # field_name may be 'url', but will replace the default value.
+                    field_name: "http://www.python.org/ftp/python/9.7.2/python-9.7.2.exe",
+                }
+                release_file = ReleaseFile(
+                    os=self.windows,
+                    release=self.draft_release,
+                    name="Windows installer draft",
+                    slug=f"windows-installer-draft-{field_name}",
+                    **kwargs,
+                )
+
+                with self.assertRaises(ValidationError) as cm:
+                    release_file.full_clean()
+                self.assertIn(field_name, cm.exception.message_dict)
+
+    def test_release_file_url_fields_cover_model_url_fields(self):
+        self.assertEqual(RELEASE_FILE_URL_FIELDS, release_file_url_field_names())
+
+    def test_release_file_sidecar_suffixes_cover_sidecar_url_fields(self):
+        sidecar_field_names = set(RELEASE_FILE_URL_FIELDS) - {"url"}
+
+        self.assertEqual(set(RELEASE_FILE_SIDECAR_SUFFIXES), sidecar_field_names)
+
+    def test_release_file_allows_existing_http_urls_to_be_edited(self):
+        release_file = ReleaseFile.objects.create(
+            os=self.windows,
+            release=self.draft_release,
+            name="Windows installer draft",
+            url="http://www.python.org/ftp/python/9.7.2/python-9.7.2.exe",
+        )
+
+        release_file.description = "Updated legacy metadata"
+
+        release_file.full_clean()
+
+    def test_release_file_rejects_existing_mismatched_sidecar_urls(self):
+        artifact_url = "https://www.python.org/ftp/python/9.7.2/Python-9.7.2-sidecar.tgz"
+        release_file = ReleaseFile.objects.create(
+            os=self.linux,
+            release=self.draft_release,
+            name="Source tarball draft",
+            slug="source-tarball-draft-mismatch",
+            url=artifact_url,
+            gpg_signature_file=artifact_url.replace("9.7.2", "9.7.1") + ".asc",
+        )
+
+        release_file.description = "Updated metadata"
+
+        with self.assertRaises(ValidationError) as cm:
+            release_file.full_clean()
+        self.assertIn("gpg_signature_file", cm.exception.message_dict)
+
+    def test_release_file_sidecar_urls_must_extend_artifact_url(self):
+        artifact_url = "https://www.python.org/ftp/python/9.7.2/Python-9.7.2-sidecar.tgz"
+
+        for field_name, suffix in RELEASE_FILE_SIDECAR_SUFFIXES.items():
+            with self.subTest(field_name):
+                wrong_artifact_url = artifact_url.replace("9.7.2", "9.7.1")
+                release_file = ReleaseFile(
+                    os=self.linux,
+                    release=self.draft_release,
+                    name="Source tarball draft",
+                    slug=f"source-tarball-draft-{field_name}",
+                    url=artifact_url,
+                    **{field_name: f"{wrong_artifact_url}{suffix}"},
+                )
+
+                with self.assertRaises(ValidationError) as cm:
+                    release_file.full_clean()
+                self.assertIn(field_name, cm.exception.message_dict)
+
+    def test_release_file_accepts_sidecar_urls_for_same_artifact(self):
+        artifact_url = "https://www.python.org/ftp/python/9.7.2/Python-9.7.2-sidecar.tgz"
+        sidecar_urls = {}
+        for field_name, suffix in RELEASE_FILE_SIDECAR_SUFFIXES.items():
+            sidecar_urls[field_name] = f"{artifact_url}{suffix}"
+
+        release_file = ReleaseFile(
+            os=self.linux,
+            release=self.draft_release,
+            name="Source tarball draft",
+            slug="source-tarball-draft",
+            url=artifact_url,
+            **sidecar_urls,
+        )
+
+        release_file.full_clean()