From dc4d9ef444afa8f335ac4f514d21fecd299263f3 Mon Sep 17 00:00:00 2001 From: Matthias Dellweg Date: Fri, 7 Aug 2026 15:54:17 +0200 Subject: [PATCH] Add relative_path domains This introduces a new shallow database type with a check constraint to maintain that all relative paths are sanitized. This also consolidates on the idea what can be allowed as a relative path. --- .../0155_create_rel_path_domains.py | 23 +++++++++ ..._contentartifact_relative_path_and_more.py | 40 +++++++++++++++ pulpcore/app/models/content.py | 3 +- pulpcore/app/models/fields.py | 5 ++ pulpcore/app/models/publication.py | 15 ++++-- pulpcore/app/serializers/__init__.py | 3 ++ pulpcore/app/serializers/base.py | 33 ------------ pulpcore/app/serializers/content.py | 51 ++++++++++--------- pulpcore/app/serializers/fields.py | 24 ++++++--- pulpcore/app/serializers/publication.py | 9 ++-- pulpcore/tests/unit/models/test_content.py | 4 +- .../tests/unit/serializers/test_fields.py | 36 +++++++++++++ 12 files changed, 170 insertions(+), 76 deletions(-) create mode 100644 pulpcore/app/migrations/0155_create_rel_path_domains.py create mode 100644 pulpcore/app/migrations/0156_alter_contentartifact_relative_path_and_more.py diff --git a/pulpcore/app/migrations/0155_create_rel_path_domains.py b/pulpcore/app/migrations/0155_create_rel_path_domains.py new file mode 100644 index 00000000000..93eff88c24c --- /dev/null +++ b/pulpcore/app/migrations/0155_create_rel_path_domains.py @@ -0,0 +1,23 @@ +# Generated by Django 5.2.17 on 2026-08-07 12:29 + +from django.db import migrations + + +CREATE_REL_PATH_DOMAINS = """ +CREATE DOMAIN "relative_path" AS text CHECK ('/' || VALUE || '/' !~ '[\n\r\s\t\?#]|(/\.{0,2}/)'); +""" + +REMOVE_REL_PATH_DOMAINS = """ +DROP DOMAIN IF EXISTS "relative_path"; +""" + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0154_task_api_version'), + ] + + operations = [ + migrations.RunSQL(sql=CREATE_REL_PATH_DOMAINS, reverse_sql=REMOVE_REL_PATH_DOMAINS), + ] diff --git a/pulpcore/app/migrations/0156_alter_contentartifact_relative_path_and_more.py b/pulpcore/app/migrations/0156_alter_contentartifact_relative_path_and_more.py new file mode 100644 index 00000000000..22fcf266537 --- /dev/null +++ b/pulpcore/app/migrations/0156_alter_contentartifact_relative_path_and_more.py @@ -0,0 +1,40 @@ +# Generated by Django 5.2.15 on 2026-08-10 10:26 + +import django.contrib.postgres.indexes +import django.db.models.expressions +import pulpcore.app.models.fields +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0155_create_rel_path_domains'), + ] + + operations = [ + migrations.AlterField( + model_name='contentartifact', + name='relative_path', + field=pulpcore.app.models.fields.RelativePathField(), + ), + migrations.AlterField( + model_name='distribution', + name='base_path', + field=pulpcore.app.models.fields.RelativePathField(), + ), + migrations.AlterField( + model_name='publishedartifact', + name='relative_path', + field=pulpcore.app.models.fields.RelativePathField(), + ), + migrations.AlterField( + model_name='publishedmetadata', + name='relative_path', + field=pulpcore.app.models.fields.RelativePathField(), + ), + migrations.AddIndex( + model_name='distribution', + index=django.contrib.postgres.indexes.SpGistIndex(django.contrib.postgres.indexes.OpClass("base_path", name='text_ops'), include=('pulp_domain',), name='core_distribution_base_path_text'), + ), + ] diff --git a/pulpcore/app/models/content.py b/pulpcore/app/models/content.py index e73d4b480ed..c8205eaacac 100644 --- a/pulpcore/app/models/content.py +++ b/pulpcore/app/models/content.py @@ -26,6 +26,7 @@ from pulpcore.app import pulp_hashlib from pulpcore.app.models import BaseModel, MasterModel, fields, storage +from pulpcore.app.models.fields import RelativePathField from pulpcore.app.util import get_domain_pk, gpg_verify from pulpcore.constants import ALL_KNOWN_CONTENT_CHECKSUMS from pulpcore.exceptions import ( @@ -656,7 +657,7 @@ class ContentArtifact(BaseModel, QueryMixin): Artifact, on_delete=models.PROTECT, null=True, related_name="content_memberships" ) content = models.ForeignKey(Content, on_delete=models.CASCADE) - relative_path = models.TextField() + relative_path = RelativePathField() objects = BulkCreateManager() diff --git a/pulpcore/app/models/fields.py b/pulpcore/app/models/fields.py index d711559344d..f6dcf31535f 100644 --- a/pulpcore/app/models/fields.py +++ b/pulpcore/app/models/fields.py @@ -155,6 +155,11 @@ def from_db_value(self, value, expression, connection): return value +class RelativePathField(TextField): + def db_type(self, connection): + return "relative_path" + + @Field.register_lookup class NotEqualLookup(Lookup): # this is copied from https://docs.djangoproject.com/en/3.2/howto/custom-lookups/ diff --git a/pulpcore/app/models/publication.py b/pulpcore/app/models/publication.py index b953be30db9..7de105a50b2 100644 --- a/pulpcore/app/models/publication.py +++ b/pulpcore/app/models/publication.py @@ -13,6 +13,7 @@ from aiohttp.web_exceptions import HTTPNotFound from django.conf import settings from django.contrib.postgres.fields import HStoreField +from django.contrib.postgres.indexes import OpClass, SpGistIndex from django.db import DatabaseError, IntegrityError, models, transaction from django.utils import timezone from django_lifecycle import AFTER_CREATE, AFTER_UPDATE, BEFORE_DELETE, hook @@ -21,6 +22,7 @@ from pulpcore.app.files import PulpTemporaryUploadedFile from pulpcore.app.models import AutoAddObjPermsMixin +from pulpcore.app.models.fields import RelativePathField from pulpcore.app.util import cache_key, get_domain_pk, get_url, retain_distributed_pub_enabled from pulpcore.cache import Cache from pulpcore.responses import ArtifactResponse @@ -270,7 +272,7 @@ class PublishedArtifact(BaseModel): publication (models.ForeignKey): The publication in which the artifact is included. """ - relative_path = models.TextField() + relative_path = RelativePathField() content_artifact = models.ForeignKey("ContentArtifact", on_delete=models.CASCADE) publication = models.ForeignKey(Publication, on_delete=models.CASCADE) @@ -293,7 +295,7 @@ class PublishedMetadata(Content): TYPE = "publishedmetadata" - relative_path = models.TextField() + relative_path = RelativePathField() publication = models.ForeignKey(Publication, on_delete=models.CASCADE) @@ -642,7 +644,7 @@ class Distribution(MasterModel): name = models.TextField(db_index=True) pulp_labels = HStoreField(default=dict) - base_path = models.TextField() + base_path = RelativePathField() pulp_domain = models.ForeignKey("Domain", default=get_domain_pk, on_delete=models.PROTECT) hidden = models.BooleanField(default=False, null=True) checkpoint = models.BooleanField(default=False) @@ -657,6 +659,13 @@ class Distribution(MasterModel): class Meta: unique_together = (("name", "pulp_domain"), ("base_path", "pulp_domain")) + indexes = [ + SpGistIndex( + OpClass("base_path", name="text_ops"), + include=("pulp_domain",), + name="%(app_label)s_%(class)s_base_path_text", + ), # Allows fast startswith (^@) lookups. + ] def get_repository_publication_and_version(self): """ diff --git a/pulpcore/app/serializers/__init__.py b/pulpcore/app/serializers/__init__.py index f0a99ee4663..5b8d89a6a80 100644 --- a/pulpcore/app/serializers/__init__.py +++ b/pulpcore/app/serializers/__init__.py @@ -26,6 +26,8 @@ ) from .fields import ( BaseURLField, + ContentArtifactChecksumField, + ContentArtifactsField, ExportsIdentityFromExporterField, ExportRelatedField, ExportIdentityField, @@ -37,6 +39,7 @@ LatestVersionField, PgpKeyFingerprintField, PulpLabelsField, + RelativePathField, SingleContentArtifactField, RepositoryVersionsIdentityFromRepositoryField, RepositoryVersionRelatedField, diff --git a/pulpcore/app/serializers/base.py b/pulpcore/app/serializers/base.py index 9d5538a4ae3..24630223c66 100644 --- a/pulpcore/app/serializers/base.py +++ b/pulpcore/app/serializers/base.py @@ -6,12 +6,10 @@ from gettext import gettext as _ from logging import getLogger from typing import List, TypedDict -from urllib.parse import urljoin from cryptography.x509 import load_pem_x509_certificate from django.conf import settings from django.core.exceptions import ObjectDoesNotExist -from django.core.validators import URLValidator from django.db import IntegrityError from django.db.models import Model from django.urls.exceptions import NoReverseMatch @@ -475,37 +473,6 @@ class Meta: read_only=True, ) - def _validate_relative_path(self, path): - """ - Validate a relative path (eg from a url) to ensure it forms a valid url and does not begin - or end with slashes nor contain spaces - - Args: - path (str): A relative path to validate - - Returns: - str: the validated path - - Raises: - django.core.exceptions.ValidationError: if the relative path is invalid - - """ - # in order to use django's URLValidator we need to construct a full url - base = "http://localhost" # use a scheme/hostname we know are valid - - if " " in path: - raise serializers.ValidationError(detail=_("Relative path cannot contain spaces.")) - - validate = URLValidator() - validate(urljoin(base, path)) - - if path != path.strip("/"): - raise serializers.ValidationError( - detail=_("Relative path cannot begin or end with slashes.") - ) - - return path - def save(self, **kwargs): try: return super().save(**kwargs) diff --git a/pulpcore/app/serializers/content.py b/pulpcore/app/serializers/content.py index 0caef26fa24..fd7a9be3c5e 100644 --- a/pulpcore/app/serializers/content.py +++ b/pulpcore/app/serializers/content.py @@ -6,18 +6,24 @@ from pulpcore.app import models from pulpcore.app.serializers import ( + ContentArtifactChecksumField, + ContentArtifactsField, + DetailIdentityField, DetailRelatedField, + IdentityField, + ModelSerializer, + PulpLabelsField, RelatedField, - base, - fields, + RelativePathField, + SingleContentArtifactField, pulp_labels_validator, ) from pulpcore.app.util import get_domain -class NoArtifactContentSerializer(base.ModelSerializer): - pulp_href = base.DetailIdentityField(view_name_pattern=r"contents(-.*/.*)-detail") - pulp_labels = fields.PulpLabelsField( +class NoArtifactContentSerializer(ModelSerializer): + pulp_href = DetailIdentityField(view_name_pattern=r"contents(-.*/.*)-detail") + pulp_labels = PulpLabelsField( help_text=_( "A dictionary of arbitrary key/value pairs used to describe a specific " "Content instance." @@ -139,7 +145,7 @@ def create(self, validated_data): class Meta: model = models.Content - fields = base.ModelSerializer.Meta.fields + ( + fields = ModelSerializer.Meta.fields + ( "repository", "overwrite", "pulp_labels", @@ -148,13 +154,12 @@ class Meta: class SingleArtifactContentSerializer(NoArtifactContentSerializer): - artifact = fields.SingleContentArtifactField( + artifact = SingleContentArtifactField( help_text=_("Artifact file representing the physical content"), ) - relative_path = serializers.CharField( + relative_path = RelativePathField( help_text=_("Path where the artifact is located relative to distributions base_path"), - validators=[fields.relative_path_validator], write_only=True, ) @@ -183,7 +188,7 @@ class Meta: class MultipleArtifactContentSerializer(NoArtifactContentSerializer): - artifacts = fields.ContentArtifactsField( + artifacts = ContentArtifactsField( help_text=_( "A dict mapping relative paths inside the Content to the corresponding" "Artifact URLs. E.g.: {'relative/path': " @@ -208,39 +213,39 @@ class ContentChecksumSerializer(serializers.Serializer): Content.objects.prefetch_related("_artifacts").all() """ - md5 = fields.ContentArtifactChecksumField( + md5 = ContentArtifactChecksumField( help_text=_("The MD5 checksum if available."), checksum="md5", ) - sha1 = fields.ContentArtifactChecksumField( + sha1 = ContentArtifactChecksumField( help_text=_("The SHA-1 checksum if available."), checksum="sha1", ) - sha224 = fields.ContentArtifactChecksumField( + sha224 = ContentArtifactChecksumField( help_text=_("The SHA-224 checksum if available."), checksum="sha224", ) - sha256 = fields.ContentArtifactChecksumField( + sha256 = ContentArtifactChecksumField( help_text=_("The SHA-256 checksum if available."), checksum="sha256", ) - sha384 = fields.ContentArtifactChecksumField( + sha384 = ContentArtifactChecksumField( help_text=_("The SHA-384 checksum if available."), checksum="sha384", ) - sha512 = fields.ContentArtifactChecksumField( + sha512 = ContentArtifactChecksumField( help_text=_("The SHA-512 checksum if available."), checksum="sha512", ) class Meta: model = models.Content - fields = base.ModelSerializer.Meta.fields + ( + fields = ModelSerializer.Meta.fields + ( "md5", "sha1", "sha224", @@ -250,8 +255,8 @@ class Meta: ) -class ArtifactSerializer(base.ModelSerializer): - pulp_href = base.IdentityField(view_name="artifacts-detail") +class ArtifactSerializer(ModelSerializer): + pulp_href = IdentityField(view_name="artifacts-detail") file = serializers.FileField(help_text=_("The stored file."), allow_empty_file=True) @@ -341,7 +346,7 @@ def validate(self, data): class Meta: model = models.Artifact - fields = base.ModelSerializer.Meta.fields + ( + fields = ModelSerializer.Meta.fields + ( "file", "size", "md5", @@ -353,12 +358,12 @@ class Meta: ) -class SigningServiceSerializer(base.ModelSerializer): +class SigningServiceSerializer(ModelSerializer): """ A serializer for the model declaring a signing service. """ - pulp_href = base.IdentityField(view_name="signing-services-detail") + pulp_href = IdentityField(view_name="signing-services-detail") name = serializers.CharField(help_text=_("A unique name used to recognize a script.")) public_key = serializers.CharField( help_text=_("The value of a public key used for the repository verification.") @@ -370,7 +375,7 @@ class SigningServiceSerializer(base.ModelSerializer): class Meta: model = models.SigningService - fields = base.ModelSerializer.Meta.fields + ( + fields = ModelSerializer.Meta.fields + ( "name", "public_key", "pubkey_fingerprint", diff --git a/pulpcore/app/serializers/fields.py b/pulpcore/app/serializers/fields.py index 08db4fccffc..d6f4443aa5f 100644 --- a/pulpcore/app/serializers/fields.py +++ b/pulpcore/app/serializers/fields.py @@ -1,5 +1,4 @@ import json -import os import re from gettext import gettext as _ from urllib.parse import urljoin @@ -15,16 +14,27 @@ from pulpcore.app.util import reverse from pulpcore.constants import LABEL_KEY_REGEX +# This ought to match the posix regex used in the relative_path domain. +# Matches "//" "/./" and "/../" or any of the forbidden characters. +_RELPATH_FORBIDDEN_REGEX = re.compile(r"[\n\r\s\t?#]|/(?:\.{0,2})?/") + def relative_path_validator(relative_path): - if os.path.isabs(relative_path): - raise serializers.ValidationError( - _("Relative path can't start with '/'. {0}").format(relative_path) - ) - if os.path.normpath(relative_path).startswith("../"): + # Adding "/" front and back makes trailing and leading "/" also trigger the regex. + if _RELPATH_FORBIDDEN_REGEX.search(f"/{relative_path}/"): raise serializers.ValidationError( - _("Relative path must not reach beyond the base path. {0}").format(relative_path) + _("Relative path is not in canonical form. {0}").format(relative_path) ) + return relative_path + + +class RelativePathField(serializers.CharField): + """ + Serializer Field for the base_url field of the Distribution. + """ + + def to_internal_value(self, value): + return relative_path_validator(super().to_internal_value(value)) # Prefer JSONDictField and JSONListField over JSONField: diff --git a/pulpcore/app/serializers/publication.py b/pulpcore/app/serializers/publication.py index eeffe7d6072..6717db1d60f 100644 --- a/pulpcore/app/serializers/publication.py +++ b/pulpcore/app/serializers/publication.py @@ -13,6 +13,7 @@ DomainUniqueValidator, GetOrCreateSerializerMixin, ModelSerializer, + RelativePathField, RepositoryVersionRelatedField, pulp_labels_validator, ) @@ -203,7 +204,7 @@ class DistributionSerializer(ModelSerializer): pulp_href = DetailIdentityField(view_name_pattern=r"distributions(-.*/.*)-detail") pulp_labels = serializers.HStoreField(required=False, validators=[pulp_labels_validator]) - base_path = serializers.CharField( + base_path = RelativePathField( help_text=_( 'The base (relative) path component of the published url. Avoid paths that \ overlap with other distribution base paths (e.g. "foo" and "foo/bar")' @@ -264,7 +265,7 @@ class Meta: "repository_version", ) - def _validate_path_overlap(self, path): + def validate_base_path(self, path): # look for any base paths nested in path search = path.split("/")[0] q = Q(base_path=search) @@ -289,10 +290,6 @@ def _validate_path_overlap(self, path): return path - def validate_base_path(self, path): - self._validate_relative_path(path) - return self._validate_path_overlap(path) - def validate(self, data): super().validate(data) diff --git a/pulpcore/tests/unit/models/test_content.py b/pulpcore/tests/unit/models/test_content.py index c4023fce6f5..a5fbd33bec3 100644 --- a/pulpcore/tests/unit/models/test_content.py +++ b/pulpcore/tests/unit/models/test_content.py @@ -1,7 +1,6 @@ from collections import namedtuple import pytest -from django.core.files.storage import default_storage as storage from django.core.files.uploadedfile import SimpleUploadedFile from pulpcore.plugin.exceptions import ( @@ -26,9 +25,8 @@ def test_create_read_delete_content(tmp_path): artifact.save() content = Content.objects.create() - artifact_file = storage.open(artifact.file.name) content_artifact = ContentArtifact.objects.create( - artifact=artifact, content=content, relative_path=artifact_file.name + artifact=artifact, content=content, relative_path="test/location/in/repository" ) assert Content.objects.filter(pk=content.pk).exists() assert ( diff --git a/pulpcore/tests/unit/serializers/test_fields.py b/pulpcore/tests/unit/serializers/test_fields.py index 3f89fad446e..ce8307ed061 100644 --- a/pulpcore/tests/unit/serializers/test_fields.py +++ b/pulpcore/tests/unit/serializers/test_fields.py @@ -214,9 +214,45 @@ def test_pgp_key_fingerprint_field_normalize(value, expected): @pytest.mark.parametrize( ("path",), [ + pytest.param("path", id="simple"), + pytest.param("relative/path", id="nested"), + pytest.param("...", id="tripple_dot"), + pytest.param("file.", id="doted_filename"), + pytest.param(".file", id="hidden_filename"), + pytest.param("..file", id="secret_agent_filename"), + pytest.param("file.ext", id="filename_extension"), + pytest.param("file..ext", id="weird_filename"), + pytest.param("path/...", id="tripple_dot_nested"), + pytest.param("path/file.", id="doted_filename_nested"), + pytest.param("path/.file", id="hidden_filename_nested"), + pytest.param("path/..file", id="secret_agent_filename_nested"), + pytest.param("path/file.ext", id="filename_extension_nested"), + pytest.param("path/file..ext", id="weird_filename_nested"), + ], +) +def test_relative_path_validator_accepts(path): + relative_path_validator(path) + + +@pytest.mark.parametrize( + ("path",), + [ + pytest.param("", id="empty"), + pytest.param("/", id="slash"), + pytest.param(".", id="dot"), + pytest.param("..", id="dotdot"), pytest.param("/absolute/path", id="absolute"), + pytest.param("suspicious/path/", id="trailing_slash"), pytest.param("../sneaky/path", id="path_traversal"), pytest.param("suspicious/../../sneaky/path", id="hidden_path_traversal"), + pytest.param("suspicious//path", id="unsanitized"), + pytest.param("suspicious/./path", id="unsanitized_dot"), + pytest.param("suspicious path", id="space"), + pytest.param("suspicious\tpath", id="tab"), + pytest.param("suspicious\rpath", id="carriage_return"), + pytest.param("suspicious\npath", id="linefeed"), + pytest.param("suspicious?path", id="questionmark"), + pytest.param("suspicious/path#fragment", id="urlfragment"), ], ) def test_relative_path_validator_rejects(path):