From c298f5ccdcf0eef65a2ffc401632517cfde500e5 Mon Sep 17 00:00:00 2001 From: varunj-msft Date: Fri, 2 Oct 2026 20:54:38 +0000 Subject: [PATCH] FIX: Register created converters and targets only after the response is built The backend registered a new converter or target before mapping it to the API response, so a mapping failure left the object registered and, for converters, left its uploaded files behind. Create now constructs the object, builds the response, and registers it last. Any failure removes the request's uploads and leaves nothing registered. --- pyrit/backend/services/converter_service.py | 21 +++---- pyrit/backend/services/target_service.py | 14 ++--- tests/unit/backend/test_converter_service.py | 63 ++++++++++++++++++++ tests/unit/backend/test_target_service.py | 16 +++++ 4 files changed, 97 insertions(+), 17 deletions(-) diff --git a/pyrit/backend/services/converter_service.py b/pyrit/backend/services/converter_service.py index e85f91be47..54d463bb75 100644 --- a/pyrit/backend/services/converter_service.py +++ b/pyrit/backend/services/converter_service.py @@ -173,8 +173,9 @@ async def create_converter_async(self, *, request: CreateConverterRequest) -> Co """ Create a new converter instance from API request. - Instantiates the converter with the given type and params, - then registers it in the registry. + Instantiates the converter with the given type and params and builds the + response before registering it, so a request that fails at any step leaves + no registered converter and removes its uploaded files. Args: request: The create converter request with type and params. @@ -194,20 +195,20 @@ async def create_converter_async(self, *, request: CreateConverterRequest) -> Co params=request.params, ) try: - converter_obj = self._registry.create_named_instance( + # Uploads may have yielded to another request that took the name. + self._registry.instances.validate_name_available(request.name) + converter_obj = self._registry.create_instance(request.type, **params) + converter = self._build_instance_from_object(converter_id=request.name, converter_obj=converter_obj) + self._registry.instances.register( + converter_obj, name=request.name, - type_name=request.type, - params=params, - registry_metadata={_OWNED_ARTIFACT_PATHS_KEY: [str(path) for path in owned_paths]}, + metadata={_OWNED_ARTIFACT_PATHS_KEY: [str(path) for path in owned_paths]}, ) except (Exception, asyncio.CancelledError): await self._remove_owned_artifacts_async(paths=owned_paths) raise - return self._build_instance_from_object( - converter_id=request.name, - converter_obj=converter_obj, - ) + return converter async def preview_conversion_async(self, *, request: ConverterPreviewRequest) -> ConverterPreviewResponse: """ diff --git a/pyrit/backend/services/target_service.py b/pyrit/backend/services/target_service.py index d57188f2c7..192bda7253 100644 --- a/pyrit/backend/services/target_service.py +++ b/pyrit/backend/services/target_service.py @@ -183,7 +183,8 @@ async def create_target_async(self, *, request: CreateTargetRequest) -> TargetIn by the target classes themselves. This service only enforces the request-level auth contract: for ``identity`` it confirms the target supports it and omits the api_key so the target validates its own - endpoint and authenticates itself. + endpoint and authenticates itself. The response is built before the + target is registered, so a failed request leaves no registered target. Args: request: The create target request with type, params, and auth_mode. @@ -214,12 +215,11 @@ async def create_target_async(self, *, request: CreateTargetRequest) -> TargetIn # LEGACY COMPATIBILITY: The current configuration UI omits the name. # Remove this generated fallback after that UI sends an explicit name. target_registry_name = request.name or f"compat_{uuid.uuid4().hex}" - target_obj = self._registry.create_named_instance( - name=target_registry_name, - type_name=request.type, - params=params, - ) - return self._build_instance_from_object(target_registry_name=target_registry_name, target_obj=target_obj) + self._registry.instances.validate_name_available(target_registry_name) + target_obj = self._registry.create_instance(request.type, **params) + target = self._build_instance_from_object(target_registry_name=target_registry_name, target_obj=target_obj) + self._registry.instances.register(target_obj, name=target_registry_name) + return target @lru_cache(maxsize=1) diff --git a/tests/unit/backend/test_converter_service.py b/tests/unit/backend/test_converter_service.py index 26497ac54b..036c111bc1 100644 --- a/tests/unit/backend/test_converter_service.py +++ b/tests/unit/backend/test_converter_service.py @@ -783,6 +783,69 @@ async def test_create_converter_cleans_upload_when_construction_fails( assert service._registry.instances.get("invalid-pdf") is None assert list(service._upload_path.iterdir()) == [] + async def test_create_converter_registers_nothing_when_response_mapping_fails( + self, upload_service: ConverterService + ) -> None: + request = CreateConverterRequest( + name="unmapped-pdf", + type="PDFConverter", + params={"existing_pdf": _make_data_uri(mime_type="application/pdf", content=b"%PDF-1.4\n")}, + ) + + with ( + patch( + "pyrit.backend.services.converter_service.converter_object_to_instance", + side_effect=RuntimeError("mapping failed"), + ), + pytest.raises(RuntimeError, match="mapping failed"), + ): + await upload_service.create_converter_async(request=request) + + assert upload_service._registry.instances.get("unmapped-pdf") is None + assert list(upload_service._upload_path.iterdir()) == [] + + async def test_create_converter_rechecks_name_after_upload(self, upload_service: ConverterService) -> None: + persist_async = upload_service._persist_data_uri_params_async + + async def persist_then_take_name_async(**kwargs: object) -> object: + result = await persist_async(**kwargs) + upload_service._registry.instances.register(Base64Converter(), name="taken") + return result + + request = CreateConverterRequest( + name="taken", + type="PDFConverter", + params={"existing_pdf": _make_data_uri(mime_type="application/pdf", content=b"%PDF-1.4\n")}, + ) + with ( + patch.object(upload_service, "_persist_data_uri_params_async", side_effect=persist_then_take_name_async), + patch.object(upload_service._registry, "create_instance") as create_instance, + pytest.raises(ValueError, match="already exists"), + ): + await upload_service.create_converter_async(request=request) + + create_instance.assert_not_called() + assert list(upload_service._upload_path.iterdir()) == [] + + async def test_create_converter_removes_upload_when_registration_fails( + self, upload_service: ConverterService + ) -> None: + request = CreateConverterRequest( + name="raced-pdf", + type="PDFConverter", + params={"existing_pdf": _make_data_uri(mime_type="application/pdf", content=b"%PDF-1.4\n")}, + ) + + with ( + patch.object( + upload_service._registry.instances, "register", side_effect=ValueError("'raced-pdf' already exists") + ), + pytest.raises(ValueError, match="already exists"), + ): + await upload_service.create_converter_async(request=request) + + assert list(upload_service._upload_path.iterdir()) == [] + @pytest.mark.parametrize("error", [OSError("write failed"), asyncio.CancelledError()]) async def test_persist_data_uri_cleans_partial_write( self, upload_service: ConverterService, error: BaseException diff --git a/tests/unit/backend/test_target_service.py b/tests/unit/backend/test_target_service.py index ac997198b2..ccf2e7d2c4 100644 --- a/tests/unit/backend/test_target_service.py +++ b/tests/unit/backend/test_target_service.py @@ -415,6 +415,22 @@ async def test_create_target_rejects_reserved_route_name(self, sqlite_instance, request=CreateTargetRequest(name=name, type="TextTarget", params={}), ) + async def test_create_target_registers_nothing_when_response_mapping_fails(self, sqlite_instance) -> None: + service = TargetService() + + with ( + patch( + "pyrit.backend.services.target_service.target_object_to_instance", + side_effect=RuntimeError("mapping failed"), + ), + pytest.raises(RuntimeError, match="mapping failed"), + ): + await service.create_target_async( + request=CreateTargetRequest(name="unmapped", type="TextTarget", params={}), + ) + + assert service.get_target_object(target_registry_name="unmapped") is None + async def test_create_target_delegates_construction_to_registry(self, sqlite_instance) -> None: """Every target construction path is owned by the registry.""" service = TargetService()