From 0096d00759e82a7b7ac134e39bc1e8cca24ea788 Mon Sep 17 00:00:00 2001 From: Daniel Alley Date: Tue, 27 Oct 2026 00:48:03 -0400 Subject: [PATCH] Fix artifact-prefixed uploads with empty media root Check file sources against the active storage path and artifact directory boundary so fresh uploads keep their client filenames. Closes #8041 Assisted By: Codex --- CHANGES/8041.bugfix | 1 + pulpcore/app/models/fields.py | 50 +++++++++++++------ .../functional/api/test_crd_artifacts.py | 11 ++++ pulpcore/tests/unit/models/test_fields.py | 47 +++++++++++++++++ 4 files changed, 95 insertions(+), 14 deletions(-) create mode 100644 CHANGES/8041.bugfix create mode 100644 pulpcore/tests/unit/models/test_fields.py diff --git a/CHANGES/8041.bugfix b/CHANGES/8041.bugfix new file mode 100644 index 00000000000..a778c2a8f19 --- /dev/null +++ b/CHANGES/8041.bugfix @@ -0,0 +1 @@ +Fixed uploads of files whose names start with `artifact` when the media root is empty. diff --git a/pulpcore/app/models/fields.py b/pulpcore/app/models/fields.py index f6dcf31535f..17157c66b55 100644 --- a/pulpcore/app/models/fields.py +++ b/pulpcore/app/models/fields.py @@ -44,15 +44,17 @@ def pre_save(self, model_instance, add): There are two ways to get artifact into Pulp: sync and upload. - The upload case - - file is not stored yet, aka file._committed = False - - nothing to do here in addition to Django pre_save actions + The upload case: + - assigning a File object sets _committed=False, meaning this FieldFile has not saved it + - the File object may still refer to a file already in artifact storage, so check any + absolute source path before saving + - Django pre_save writes the File object to upload_to without extra handling here The sync case: - - file is already stored in a temporary location, aka file._committed = True - - it needs to be moved into Pulp artifact storage if it's not there - - TemporaryDownloadedFile takes care of correctly set storage path - - only then Django pre_save actions should be performed + - assigning a path sets _committed=True, even when the file is in a temporary location + - a file already at upload_to stays in place; other files in artifact storage are rejected + - TemporaryDownloadedFile wraps an outside source so storage can move or copy it + - when needed, mark the file uncommitted so Django pre_save stores it at upload_to Args: model_instance (`class::pulpcore.plugin.Artifact`): The instance this field belongs to. @@ -64,13 +66,30 @@ def pre_save(self, model_instance, add): """ file = model_instance.file + # A path assignment creates a committed FieldFile whose name is the source path. + # A File assignment is uncommitted, so inspect the wrapped File's source name. + source_path = file.name if file._committed else file.file.name + if not file._committed and (not source_path or not os.path.isabs(source_path)): + # A relative File name does not identify a location in artifact storage. + return super().pre_save(model_instance, add) + artifact_storage_path = self.upload_to(model_instance, "") - already_in_place = file.name in [ - artifact_storage_path, - os.path.join(settings.MEDIA_ROOT, artifact_storage_path), - ] - is_in_artifact_storage = file.name.startswith(os.path.join(settings.MEDIA_ROOT, "artifact")) + stored_file_path = None + stored_artifact_dir = None + try: + stored_file_path = file.storage.path(artifact_storage_path) + stored_artifact_dir = file.storage.path("artifact") + except NotImplementedError: + # Object storage backends do not have absolute filesystem paths. + pass + + already_in_place = source_path in (artifact_storage_path, stored_file_path) + # Match the artifact directory, not other names beginning with "artifact". + artifact_storage_prefixes = [os.path.join("artifact", "")] + if stored_artifact_dir is not None: + artifact_storage_prefixes.append(os.path.join(stored_artifact_dir, "")) + is_in_artifact_storage = source_path.startswith(tuple(artifact_storage_prefixes)) if not already_in_place and is_in_artifact_storage: raise ValueError( @@ -80,10 +99,13 @@ def pre_save(self, model_instance, add): "prior to Artifact creation." ) ) - move = file._committed and file.name != artifact_storage_path - if move: + if not file._committed: + return super().pre_save(model_instance, add) + + if file.name != artifact_storage_path: if not already_in_place: file._file = TemporaryDownloadedFile(open(file.name, "rb")) + # Let Django save the source and record its storage-relative name. file._committed = False return super().pre_save(model_instance, add) diff --git a/pulpcore/tests/functional/api/test_crd_artifacts.py b/pulpcore/tests/functional/api/test_crd_artifacts.py index 2ce1a1f958e..4d0e033595f 100644 --- a/pulpcore/tests/functional/api/test_crd_artifacts.py +++ b/pulpcore/tests/functional/api/test_crd_artifacts.py @@ -55,6 +55,17 @@ def test_upload_valid_attrs(pulpcore_bindings, pulpcore_random_file, monitor_tas _do_upload_valid_attrs(pulpcore_bindings, pulpcore_random_file["name"], data) +@pytest.mark.parallel +def test_upload_artifact_prefixed_filename(pulpcore_bindings, tmp_path): + file = tmp_path / f"artifact-{uuid.uuid4()}.txt" + contents = os.urandom(1024) + file.write_bytes(contents) + + artifact = pulpcore_bindings.ArtifactsApi.create(str(file)) + + assert artifact.sha256 == hashlib.sha256(contents).hexdigest() + + def test_upload_empty_file(pulpcore_bindings, tmp_path, monitor_task): """Upload an empty file. diff --git a/pulpcore/tests/unit/models/test_fields.py b/pulpcore/tests/unit/models/test_fields.py new file mode 100644 index 00000000000..a2f2dac1b8c --- /dev/null +++ b/pulpcore/tests/unit/models/test_fields.py @@ -0,0 +1,47 @@ +import hashlib + +import pytest +from django.core.files import File +from django.core.files.uploadedfile import SimpleUploadedFile + +from pulpcore.app.models import Artifact, Domain +from pulpcore.app.models.storage import FileSystem + + +@pytest.fixture +def artifact_storage_root(monkeypatch, tmp_path): + storage_root = tmp_path / "media" + storage = FileSystem(location=str(storage_root)) + monkeypatch.setattr(Domain, "get_storage", lambda self: storage) + return storage_root + + +@pytest.mark.django_db +def test_artifact_upload_with_empty_media_root(settings, artifact_storage_root): + settings.MEDIA_ROOT = "" + + contents = b"artifact upload" + artifact = Artifact.objects.create( + file=SimpleUploadedFile("artifact-upload.txt", contents), + size=len(contents), + **{digest: hashlib.new(digest, contents).hexdigest() for digest in Artifact.DIGEST_FIELDS}, + ) + + artifact.refresh_from_db() + assert artifact.file.read() == contents + + +@pytest.mark.django_db +def test_artifact_file_object_from_artifact_storage_is_rejected(artifact_storage_root): + contents = b"artifact already in storage" + source_path = artifact_storage_root / "artifact" / "other" + source_path.parent.mkdir(parents=True) + source_path.write_bytes(contents) + digests = { + digest: hashlib.new(digest, contents).hexdigest() for digest in Artifact.DIGEST_FIELDS + } + + with source_path.open("rb") as source: + artifact = Artifact(file=File(source), size=len(contents), **digests) + with pytest.raises(ValueError, match="already present in Artifact storage"): + artifact.save()