diff --git a/apps/downloads/api.py b/apps/downloads/api.py index 4d8f3010c..a01638d6a 100644 --- a/apps/downloads/api.py +++ b/apps/downloads/api.py @@ -1,11 +1,13 @@ """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.validation import Validation from apps.downloads.models import OS, Release, ReleaseFile from apps.downloads.serializers import OSSerializer, ReleaseFileSerializer, ReleaseSerializer @@ -14,6 +16,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.""" @@ -88,6 +104,7 @@ class Meta(GenericResource.Meta): queryset = ReleaseFile.objects.all() resource_name = "downloads/release_file" + validation = ReleaseFileValidation() fields = [ "name", "slug", diff --git a/apps/downloads/models.py b/apps/downloads/models.py index 8af0a1c1d..e7cfe30f2 100644 --- a/apps/downloads/models.py +++ b/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: diff --git a/apps/downloads/serializers.py b/apps/downloads/serializers.py index 14e74ef06..64cf86025 100644 --- a/apps/downloads/serializers.py +++ b/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.""" diff --git a/apps/downloads/tests/test_models.py b/apps/downloads/tests/test_models.py index d1d4c97b3..bdd04d5ef 100644 --- a/apps/downloads/tests/test_models.py +++ b/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() diff --git a/apps/downloads/tests/test_views.py b/apps/downloads/tests/test_views.py index 1a05dfd40..6ff57c4e5 100644 --- a/apps/downloads/tests/test_views.py +++ b/apps/downloads/tests/test_views.py @@ -6,7 +6,11 @@ from django.urls import reverse from rest_framework.test import APITestCase -from apps.downloads.models import Release +from apps.downloads.models import ( + RELEASE_FILE_HTTPS_ERROR, + RELEASE_FILE_SIDECAR_SUFFIXES, + Release, +) from apps.downloads.tests.base import BaseDownloadTests, DownloadMixin from apps.pages.factories import PageFactory from apps.users.factories import UserFactory @@ -183,6 +187,17 @@ def get_json(self, response): return json_response["objects"] return json_response + def assert_release_file_validation_error(self, response, field_name, message): + content = self.get_json(response) + if self.api_version == "v1": + content = content["downloads/release_file"] + + self.assertIn(field_name, content) + errors = content[field_name] + if isinstance(errors, str): + errors = [errors] + self.assertIn(message, [str(error) for error in errors]) + def test_invalid_token(self): url = self.create_url("os", self.linux.pk) response = self.json_client( @@ -412,6 +427,102 @@ def test_post_release_file(self): self.assertIn(data["release"], content["release"]) self.assertEqual(content["description"], data["description"]) + def test_post_release_file_rejects_http_urls(self): + url = self.create_url("release_file") + data = { + "name": "HTTP file", + "slug": "http-file", + "os": self.create_url("os", self.linux.pk), + "release": self.create_url("release", self.release_275.pk), + "description": "This is a description.", + "is_source": True, + "url": "http://www.python.org/ftp/python/2.7.5/Python-2.7.5-http.tgz", + "md5_sum": "098f6bcd4621d373cade4e832627b4f6", + "filesize": 123456, + "download_button": False, + } + + response = self.json_client("post", url, data, HTTP_AUTHORIZATION=self.Authorization) + + self.assertEqual(response.status_code, 400) + self.assert_release_file_validation_error(response, "url", RELEASE_FILE_HTTPS_ERROR) + + def test_post_release_file_rejects_sidecars_for_other_artifacts(self): + url = self.create_url("release_file") + artifact_url = "https://www.python.org/ftp/python/2.7.5/Python-2.7.5-api.tgz" + wrong_gpg_signature_url = "https://www.python.org/ftp/python/2.7.5/Python-2.7.4-api.tgz.asc" + data = { + "name": "File with wrong sidecar", + "slug": "file-with-wrong-sidecar", + "os": self.create_url("os", self.linux.pk), + "release": self.create_url("release", self.release_275.pk), + "description": "This is a description.", + "is_source": True, + "url": artifact_url, + "gpg_signature_file": wrong_gpg_signature_url, + "md5_sum": "098f6bcd4621d373cade4e832627b4f6", + "filesize": 123456, + "download_button": False, + } + + response = self.json_client("post", url, data, HTTP_AUTHORIZATION=self.Authorization) + + self.assertEqual(response.status_code, 400) + self.assert_release_file_validation_error( + response, + "gpg_signature_file", + "Sidecar URL must match the artifact URL plus '.asc'.", + ) + + def test_update_release_file_rejects_changed_http_urls(self): + url = self.create_url("release_file", self.release_275_linux.pk) + data = { + "name": self.release_275_linux.name, + "slug": self.release_275_linux.slug, + "os": self.create_url("os", self.linux.pk), + "release": self.create_url("release", self.release_275.pk), + "description": self.release_275_linux.description, + "is_source": self.release_275_linux.is_source, + "url": "http://www.python.org/ftp/python/2.7.5/Python-2.7.5.tgz", + "md5_sum": self.release_275_linux.md5_sum, + "sha256_sum": self.release_275_linux.sha256_sum, + "filesize": self.release_275_linux.filesize, + "download_button": self.release_275_linux.download_button, + } + + response = self.json_client("put", url, data, HTTP_AUTHORIZATION=self.Authorization) + + self.assertEqual(response.status_code, 400) + self.assert_release_file_validation_error(response, "url", RELEASE_FILE_HTTPS_ERROR) + + def test_update_release_file_rejects_changed_sidecars_for_other_artifacts(self): + url = self.create_url("release_file", self.release_275_linux.pk) + artifact_url = self.release_275_linux.url + data = { + "name": self.release_275_linux.name, + "slug": self.release_275_linux.slug, + "os": self.create_url("os", self.linux.pk), + "release": self.create_url("release", self.release_275.pk), + "description": self.release_275_linux.description, + "is_source": self.release_275_linux.is_source, + "url": artifact_url, + "gpg_signature_file": artifact_url.replace("2.7.5", "2.7.4") + ".asc", + "md5_sum": self.release_275_linux.md5_sum, + "sha256_sum": self.release_275_linux.sha256_sum, + "filesize": self.release_275_linux.filesize, + "download_button": self.release_275_linux.download_button, + } + + response = self.json_client("put", url, data, HTTP_AUTHORIZATION=self.Authorization) + + self.assertEqual(response.status_code, 400) + gpg_signature_suffix = RELEASE_FILE_SIDECAR_SUFFIXES["gpg_signature_file"] + self.assert_release_file_validation_error( + response, + "gpg_signature_file", + f"Sidecar URL must match the artifact URL plus '{gpg_signature_suffix}'.", + ) + def test_delete_release_file(self): url = self.create_url("release_file", self.release_275_linux.pk) response = self.json_client("delete", url)