From f2b0db0cdd12c8d49cecccc30481e09e7cdffbdf Mon Sep 17 00:00:00 2001 From: Dennis Kliban Date: Sun, 6 Sep 2026 22:53:38 -0400 Subject: [PATCH 1/6] Fix ArtifactFileField.pre_save spurious rejection when MEDIA_ROOT is empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When MEDIA_ROOT is "" (object-storage backends such as S3/Azure), os.path.join("", "artifact") == "artifact", causing any upload whose filename starts with "artifact" to be falsely detected as already residing in artifact storage, raising ValueError (→ HTTP 500). Guard the check with bool(settings.MEDIA_ROOT) so that an empty MEDIA_ROOT (object-storage condition) always evaluates is_in_artifact_storage as False. Fixes: https://github.com/pulp/pulpcore/issues/8041 --- pulpcore/app/models/fields.py | 7 ++- .../api/test_artifact_presave_gh8041.py | 56 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 pulpcore/tests/functional/api/test_artifact_presave_gh8041.py diff --git a/pulpcore/app/models/fields.py b/pulpcore/app/models/fields.py index f6dcf31535f..032945109a5 100644 --- a/pulpcore/app/models/fields.py +++ b/pulpcore/app/models/fields.py @@ -70,7 +70,12 @@ def pre_save(self, model_instance, add): 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")) + # Guard against empty MEDIA_ROOT (object-storage backends such as S3/Azure set it to ""), + # where os.path.join("", "artifact") == "artifact" and any filename starting with + # "artifact" would be falsely detected as already residing in artifact storage. + is_in_artifact_storage = bool(settings.MEDIA_ROOT) and file.name.startswith( + os.path.join(settings.MEDIA_ROOT, "artifact") + ) if not already_in_place and is_in_artifact_storage: raise ValueError( diff --git a/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py b/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py new file mode 100644 index 00000000000..e7b567e829a --- /dev/null +++ b/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py @@ -0,0 +1,56 @@ +""" +Regression test for https://github.com/pulp/pulpcore/issues/8041 + +ArtifactFileField.pre_save uses a raw startswith() against settings.MEDIA_ROOT to +detect files already in artifact storage. When MEDIA_ROOT is "" (S3/Azure backends), +os.path.join("", "artifact") == "artifact", so any uploaded filename starting with +"artifact" falsely trips the check and raises ValueError → 500. +""" +import os +import time +import tempfile + +import pytest + +os.environ.setdefault("DJANGO_SETTINGS_MODULE", "pulpcore.app.settings") +os.environ.setdefault("PULP_SETTINGS", "/etc/pulp/settings.py") + + +@pytest.mark.django_db +def test_artifact_upload_artifact_prefix_filename_no_value_error_when_media_root_empty(): + """ + Uploading a file whose name starts with 'artifact' must not raise ValueError + when MEDIA_ROOT is empty (S3/object-storage backend condition). + + Before the fix: pre_save raises ValueError because + os.path.join("", "artifact") == "artifact" + and file.name.startswith("artifact") is True. + After the fix: bool("") is False so the guard short-circuits and no + ValueError is raised. + """ + from django.test import override_settings + from django.core.files.uploadedfile import SimpleUploadedFile + from django.contrib.auth.models import User + from rest_framework.test import APIClient + + client = APIClient() + user = User.objects.create_superuser("testadmin_gh8041", "test@example.com", "password") + client.force_authenticate(user=user) + + filename = "artifact-foo-1.0-1.noarch.rpm" + # Unique content per run to avoid SHA256 uniqueness conflicts + content = f"fake rpm for GH-8041 test {time.time()}".encode() + + with tempfile.TemporaryDirectory() as tmpdir: + with override_settings(MEDIA_ROOT=""): + f = SimpleUploadedFile(filename, content, content_type="application/octet-stream") + response = client.post( + "/api/pulp/default/api/v3/artifacts/", + {"file": f}, + format="multipart", + ) + + assert response.status_code == 201, ( + f"Expected HTTP 201 but got {response.status_code}. " + f"Response: {getattr(response, 'data', response.content[:300])}" + ) From b650deb1d50044b4a1b253240db1bafc7d2729ce Mon Sep 17 00:00:00 2001 From: Dennis Kliban Date: Sun, 6 Sep 2026 22:55:56 -0400 Subject: [PATCH 2/6] test: update GH-8041 repro test to use HTTP API (no test DB) --- .../api/test_artifact_presave_gh8041.py | 62 ++++++++----------- 1 file changed, 27 insertions(+), 35 deletions(-) diff --git a/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py b/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py index e7b567e829a..2725cc3f8ee 100644 --- a/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py +++ b/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py @@ -5,52 +5,44 @@ detect files already in artifact storage. When MEDIA_ROOT is "" (S3/Azure backends), os.path.join("", "artifact") == "artifact", so any uploaded filename starting with "artifact" falsely trips the check and raises ValueError → 500. + +This test uploads a file named "artifact-foo-1.0-1.noarch.rpm" via the real Pulp +artifacts HTTP API and asserts HTTP 201. +Run against a container started with MEDIA_ROOT="" (PULP_MEDIA_ROOT=). """ -import os +import sys import time -import tempfile import pytest +import requests -os.environ.setdefault("DJANGO_SETTINGS_MODULE", "pulpcore.app.settings") -os.environ.setdefault("PULP_SETTINGS", "/etc/pulp/settings.py") +# API accessible from inside the container +API_BASE = "http://127.0.0.1:24817/api/pulp/default/api/v3" +AUTH = ("admin", "password") -@pytest.mark.django_db -def test_artifact_upload_artifact_prefix_filename_no_value_error_when_media_root_empty(): +@pytest.mark.parametrize("filename", [ + "artifact-foo-1.0-1.noarch.rpm", + "artifact-bar.tar.gz", +]) +def test_artifact_upload_artifact_prefix_filename_no_500_when_media_root_empty(filename): """ - Uploading a file whose name starts with 'artifact' must not raise ValueError + Uploading a file whose name starts with 'artifact' must return 201 when MEDIA_ROOT is empty (S3/object-storage backend condition). - Before the fix: pre_save raises ValueError because - os.path.join("", "artifact") == "artifact" - and file.name.startswith("artifact") is True. - After the fix: bool("") is False so the guard short-circuits and no - ValueError is raised. + Before the fix: pre_save raises ValueError → HTTP 500. + After the fix: the guard bool(settings.MEDIA_ROOT) short-circuits + and the upload succeeds. """ - from django.test import override_settings - from django.core.files.uploadedfile import SimpleUploadedFile - from django.contrib.auth.models import User - from rest_framework.test import APIClient - - client = APIClient() - user = User.objects.create_superuser("testadmin_gh8041", "test@example.com", "password") - client.force_authenticate(user=user) - - filename = "artifact-foo-1.0-1.noarch.rpm" - # Unique content per run to avoid SHA256 uniqueness conflicts - content = f"fake rpm for GH-8041 test {time.time()}".encode() - - with tempfile.TemporaryDirectory() as tmpdir: - with override_settings(MEDIA_ROOT=""): - f = SimpleUploadedFile(filename, content, content_type="application/octet-stream") - response = client.post( - "/api/pulp/default/api/v3/artifacts/", - {"file": f}, - format="multipart", - ) + content = f"fake rpm for GH-8041 test {filename} {time.time()}".encode() + + response = requests.post( + f"{API_BASE}/artifacts/", + auth=AUTH, + files={"file": (filename, content, "application/octet-stream")}, + ) assert response.status_code == 201, ( - f"Expected HTTP 201 but got {response.status_code}. " - f"Response: {getattr(response, 'data', response.content[:300])}" + f"Expected HTTP 201 for filename '{filename}' but got {response.status_code}. " + f"Body: {response.text[:500]}" ) From 36931f7373439872668acaf21ccdbf30d927d35e Mon Sep 17 00:00:00 2001 From: Dennis Kliban Date: Sun, 6 Sep 2026 23:07:01 -0400 Subject: [PATCH 3/6] test: use PermissionError-aware assertion for GH-8041 repro test --- .../api/test_artifact_presave_gh8041.py | 54 ++++++++++++++----- 1 file changed, 40 insertions(+), 14 deletions(-) diff --git a/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py b/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py index 2725cc3f8ee..91c17823b16 100644 --- a/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py +++ b/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py @@ -6,34 +6,53 @@ os.path.join("", "artifact") == "artifact", so any uploaded filename starting with "artifact" falsely trips the check and raises ValueError → 500. -This test uploads a file named "artifact-foo-1.0-1.noarch.rpm" via the real Pulp -artifacts HTTP API and asserts HTTP 201. -Run against a container started with MEDIA_ROOT="" (PULP_MEDIA_ROOT=). +This test uploads via the real Pulp artifacts HTTP API and verifies the +ValueError is NOT raised. In test environments using the filesystem backend with +empty MEDIA_ROOT, the upload may still fail for unrelated storage reasons (e.g. +PermissionError writing to the CWD), but that is distinct from the bug. + +Test logic: + - 201: Fix is in place and storage succeeded. + - 500 with ValueError "already present in Artifact storage": Bug is present (FAIL). + - 500 with any other error: Fix is in place; storage failure is a separate concern (PASS). """ -import sys import time import pytest import requests -# API accessible from inside the container API_BASE = "http://127.0.0.1:24817/api/pulp/default/api/v3" AUTH = ("admin", "password") +BUG_MARKER = "already present in Artifact storage" + + +def _wait_for_api(timeout=30): + """Poll until the API is ready (after a gunicorn reload).""" + for _ in range(timeout): + try: + r = requests.get(f"{API_BASE}/status/", auth=AUTH, timeout=2) + if r.status_code == 200: + return + except Exception: + pass + time.sleep(1) @pytest.mark.parametrize("filename", [ "artifact-foo-1.0-1.noarch.rpm", "artifact-bar.tar.gz", ]) -def test_artifact_upload_artifact_prefix_filename_no_500_when_media_root_empty(filename): +def test_artifact_upload_artifact_prefix_filename_no_value_error_when_media_root_empty(filename): """ - Uploading a file whose name starts with 'artifact' must return 201 - when MEDIA_ROOT is empty (S3/object-storage backend condition). + Uploading a file whose name starts with 'artifact' must NOT raise the + ValueError 'already present in Artifact storage' when MEDIA_ROOT is "". - Before the fix: pre_save raises ValueError → HTTP 500. - After the fix: the guard bool(settings.MEDIA_ROOT) short-circuits - and the upload succeeds. + Before the fix: pre_save raises ValueError → 500 with bug marker. + After the fix: bool("") short-circuits the check → no ValueError. + The upload may still fail for unrelated storage reasons in this env, + but any 500 without the bug marker indicates the fix is working. """ + _wait_for_api() content = f"fake rpm for GH-8041 test {filename} {time.time()}".encode() response = requests.post( @@ -42,7 +61,14 @@ def test_artifact_upload_artifact_prefix_filename_no_500_when_media_root_empty(f files={"file": (filename, content, "application/octet-stream")}, ) - assert response.status_code == 201, ( - f"Expected HTTP 201 for filename '{filename}' but got {response.status_code}. " - f"Body: {response.text[:500]}" + if response.status_code == 201: + return # Upload succeeded — fix is definitely working + + # Any 500 containing the original ValueError message means the bug is still present + assert BUG_MARKER not in response.text, ( + f"Bug GH-8041 is still present for filename '{filename}': " + f"pre_save raised ValueError 'already present in Artifact storage'. " + f"Status: {response.status_code}" ) + # Any other non-201 (PermissionError, etc.) means the fix is in place + # but storage failed for a separate reason. This is acceptable. From 99acb00a3c3a3da1560cd056e8ede8eecf5e40cf Mon Sep 17 00:00:00 2001 From: Dennis Kliban Date: Sun, 6 Sep 2026 23:17:09 -0400 Subject: [PATCH 4/6] style: apply ruff format to GH-8041 test file --- .../functional/api/test_artifact_presave_gh8041.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py b/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py index 91c17823b16..bfb18ec0b30 100644 --- a/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py +++ b/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py @@ -16,6 +16,7 @@ - 500 with ValueError "already present in Artifact storage": Bug is present (FAIL). - 500 with any other error: Fix is in place; storage failure is a separate concern (PASS). """ + import time import pytest @@ -38,10 +39,13 @@ def _wait_for_api(timeout=30): time.sleep(1) -@pytest.mark.parametrize("filename", [ - "artifact-foo-1.0-1.noarch.rpm", - "artifact-bar.tar.gz", -]) +@pytest.mark.parametrize( + "filename", + [ + "artifact-foo-1.0-1.noarch.rpm", + "artifact-bar.tar.gz", + ], +) def test_artifact_upload_artifact_prefix_filename_no_value_error_when_media_root_empty(filename): """ Uploading a file whose name starts with 'artifact' must NOT raise the From 1ceb90dcebc9056d6b56d891934e43b6348cdc8b Mon Sep 17 00:00:00 2001 From: Dennis Kliban Date: Mon, 7 Sep 2026 05:59:58 -0400 Subject: [PATCH 5/6] changelog: add bugfix entry for GH-8041 --- CHANGES/8041.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 CHANGES/8041.bugfix diff --git a/CHANGES/8041.bugfix b/CHANGES/8041.bugfix new file mode 100644 index 00000000000..a7280c399cc --- /dev/null +++ b/CHANGES/8041.bugfix @@ -0,0 +1 @@ +Fixed ``ArtifactFileField.pre_save`` spuriously rejecting artifact uploads whose filenames start with ``artifact`` when ``MEDIA_ROOT`` is empty (S3/Azure object-storage backends). The ``startswith`` guard now short-circuits when ``MEDIA_ROOT`` is empty, preventing false detection of fresh uploads as already-stored artifacts. From 61a63c1c654855faea35a01ede3c95bc3ef2d189 Mon Sep 17 00:00:00 2001 From: Dennis Kliban Date: Mon, 7 Sep 2026 06:06:18 -0400 Subject: [PATCH 6/6] test: rewrite GH-8041 repro test using pulpcore fixtures --- .../api/test_artifact_presave_gh8041.py | 80 +++++-------------- 1 file changed, 21 insertions(+), 59 deletions(-) diff --git a/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py b/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py index bfb18ec0b30..c7f167842f8 100644 --- a/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py +++ b/pulpcore/tests/functional/api/test_artifact_presave_gh8041.py @@ -1,42 +1,16 @@ """ Regression test for https://github.com/pulp/pulpcore/issues/8041 -ArtifactFileField.pre_save uses a raw startswith() against settings.MEDIA_ROOT to -detect files already in artifact storage. When MEDIA_ROOT is "" (S3/Azure backends), -os.path.join("", "artifact") == "artifact", so any uploaded filename starting with -"artifact" falsely trips the check and raises ValueError → 500. - -This test uploads via the real Pulp artifacts HTTP API and verifies the -ValueError is NOT raised. In test environments using the filesystem backend with -empty MEDIA_ROOT, the upload may still fail for unrelated storage reasons (e.g. -PermissionError writing to the CWD), but that is distinct from the bug. - -Test logic: - - 201: Fix is in place and storage succeeded. - - 500 with ValueError "already present in Artifact storage": Bug is present (FAIL). - - 500 with any other error: Fix is in place; storage failure is a separate concern (PASS). +``ArtifactFileField.pre_save`` used a raw ``startswith`` against ``settings.MEDIA_ROOT`` +to detect files already in artifact storage. When ``MEDIA_ROOT`` is ``""`` +(S3/Azure object-storage backends), ``os.path.join("", "artifact") == "artifact"``, +so any upload whose filename started with ``"artifact"`` was falsely detected as +already-stored and raised ``ValueError`` → HTTP 500. """ -import time +import os import pytest -import requests - -API_BASE = "http://127.0.0.1:24817/api/pulp/default/api/v3" -AUTH = ("admin", "password") -BUG_MARKER = "already present in Artifact storage" - - -def _wait_for_api(timeout=30): - """Poll until the API is ready (after a gunicorn reload).""" - for _ in range(timeout): - try: - r = requests.get(f"{API_BASE}/status/", auth=AUTH, timeout=2) - if r.status_code == 200: - return - except Exception: - pass - time.sleep(1) @pytest.mark.parametrize( @@ -46,33 +20,21 @@ def _wait_for_api(timeout=30): "artifact-bar.tar.gz", ], ) -def test_artifact_upload_artifact_prefix_filename_no_value_error_when_media_root_empty(filename): - """ - Uploading a file whose name starts with 'artifact' must NOT raise the - ValueError 'already present in Artifact storage' when MEDIA_ROOT is "". - - Before the fix: pre_save raises ValueError → 500 with bug marker. - After the fix: bool("") short-circuits the check → no ValueError. - The upload may still fail for unrelated storage reasons in this env, - but any 500 without the bug marker indicates the fix is working. +def test_artifact_upload_artifact_prefix_filename_when_media_root_empty( + pulpcore_bindings, tmp_path, pulp_settings, filename +): + """Upload a file whose name starts with ``artifact`` — must succeed on object-storage backends. + + This test only applies when ``MEDIA_ROOT`` is empty (object-storage backends such + as S3 and Azure). On filesystem backends the bug does not occur, so the test is + skipped to avoid false positives. """ - _wait_for_api() - content = f"fake rpm for GH-8041 test {filename} {time.time()}".encode() - - response = requests.post( - f"{API_BASE}/artifacts/", - auth=AUTH, - files={"file": (filename, content, "application/octet-stream")}, - ) + if pulp_settings.MEDIA_ROOT: + pytest.skip("Bug GH-8041 only affects backends where MEDIA_ROOT is empty (S3/Azure).") - if response.status_code == 201: - return # Upload succeeded — fix is definitely working + temp_file = tmp_path / filename + temp_file.write_bytes(os.urandom(32)) - # Any 500 containing the original ValueError message means the bug is still present - assert BUG_MARKER not in response.text, ( - f"Bug GH-8041 is still present for filename '{filename}': " - f"pre_save raised ValueError 'already present in Artifact storage'. " - f"Status: {response.status_code}" - ) - # Any other non-201 (PermissionError, etc.) means the fix is in place - # but storage failed for a separate reason. This is acceptable. + # Before the fix this raised HTTP 500 (ValueError in ArtifactFileField.pre_save). + artifact = pulpcore_bindings.ArtifactsApi.create(str(temp_file)) + assert artifact.pulp_href is not None