Repository navigation
FEAT: Save targets created through the API (part 8) - #4026
varunj-msft wants to merge 155 commits into
Conversation
…API input External construction now rejects a parameter whose type includes Path unless the calling service passes the name in allowed_path_params after making the value safe. The converter service allows its upload parameters once they are stored, and the target service allows the server-set upload folder and the GitHubCopilotTarget working directory. The scorer-only check is removed, and the "Valid parameters" list leaves out path parameters callers cannot set.
The target catalog now applies the path rule to required parameters too, so a target that needs a disallowed path is left out instead of listed without the field, matching the scorer catalog. allowed_path_params is typed frozenset[str] so a bare string cannot turn the allow-list into a substring match.
The target catalog still lists a target whose required path parameter is its server-set upload folder when the server has one, and leaves it out otherwise.
# Conflicts: # tests/unit/backend/test_converter_service.py
…benchmark's recipes folder
# Conflicts: # pyrit/backend/mappers/target_mappers.py # pyrit/backend/models/targets.py # pyrit/backend/routes/targets.py # pyrit/backend/services/target_service.py # tests/unit/backend/test_api_routes.py # tests/unit/backend/test_mappers.py # tests/unit/backend/test_runtime_lifecycle.py # tests/unit/backend/test_target_service.py # tests/unit/registry/test_instance_registry.py
main now declares that TextTarget takes no credentials, with an empty supported_auth_modes. The saved-entry check read that as supporting no mode at all, so it refused saved TextTargets and left the saveability check for the target types with no mode to report. A class that declares no auth modes is now checked as the default api_key mode, which it has no key for.
varunj-msft
changed the title
| self._enforce_server_credential_permission( | ||
| target_type=request.type, params=permission_params, is_admin=is_admin | ||
| ) | ||
| if request.persist: |
There was a problem hiding this comment.
[AI assisted]: [High] Any user who can create targets (not just admins) can save entries that leave the backend not ready after a restart, or that break scans, and this PR has no API path to undo it.
persist isn't limited to admins; only classes with a server_credential_parameter, or ones that run model repository code, need an admin.
- Saving a target whose class only a scan's launch scripts register makes restore fail on the next start, so the runtime stays not ready for everyone until an operator edits storage.
- Saving a name that a scan's launch scripts register stops those scans.
Both are listed under "Not covered", but delete and retry only arrive in #4071. Suggest applying the same "comes back after a restart" rule to the class that _refuse_temporary_references applies to referenced targets, and limiting persist to admins until #4071 lands.
| ValueError: If the target would hold or use a credential, or names a Temporary target. | ||
| """ | ||
| saved_params = dict(params) | ||
| for name, turned_off in target_cls.server_fallback_parameters.items(): |
There was a problem hiding this comment.
[AI assisted]: [Medium] The "saved targets can't use this server's credentials" contract has a documented gap. Setting headers to "{}" here stops PyRIT's OPENAI_ADDITIONAL_REQUEST_HEADERS, but the README says OPENAI_CUSTOM_HEADERS still applies, and the OpenAI SDK (e.g. openai 2.54, openai/_client.py) merges that variable's headers into every client it creates. If an operator puts an auth header there (for example an APIM subscription key), a saved no-key target pointed at any host sends it on every request.
Temporary targets already have this exposure, but saving makes it permanent and unattended. Suggest refusing persist while that variable is set, or at least a prominent warning in pyrit_conf.md.
| ) | ||
|
|
||
|
|
||
| class SavedEntryOwner(BaseModel): |
There was a problem hiding this comment.
[AI assisted]: [Medium] owner looks like a placeholder in this PR, and it stores personal data. It's written to every saved recipe, but nothing here reads it (the description says it's recorded but not shown yet; the first reader is in #4071). Since it's already optional (None when sign-in is off), adding it in the PR that uses it wouldn't break anything.
If it stays, consider storing only oid: the display name or email goes out of date and is kept in plain text, possibly in shared Blob storage. Related: _saved_entry_owner() runs on every create, not only when saving, and create_recipe accepts TechniqueRecipe, which nothing in this PR uses.
|
|
||
|
|
||
| def target_object_to_instance(target_registry_name: str, target_obj: PromptTarget) -> TargetInstance: | ||
| def target_origin(metadata: Mapping[str, Any]) -> TargetOrigin: |
There was a problem hiding this comment.
[AI assisted]: [Medium] Origin logic sits in the mapper layer, and the public API will churn. The save rule _refuse_temporary_references in the service depends on this mapper helper, and TargetOrigin is a new public export of pyrit.models.catalog. #4071 currently deletes both, replacing them with RegistryEntryOrigin and registry_entry_origin() in pyrit/registry/instance_registry.py.
Suggest using the final name and location in this PR: if a release ships between the two, it becomes a breaking rename that needs a deprecation.
| self._metadata_cache = None | ||
| self._instance_version += 1 | ||
|
|
||
| def mark_configuration_entries(self) -> None: |
There was a problem hiding this comment.
[AI assisted]: [Medium] The configuration origin is inferred after the fact rather than recorded at registration. Right before restore, this marks everything that isn't saved as configuration. That's correct only because the API rejects requests until the runtime is ready and apply resets the registries first, and #4071 already calls it from a retry path too. It also adds a backend-only concept to the generic InstanceRegistry protocol. Suggest marking entries when the startup initializers register them, e.g. inside a scope like the existing record_saved_entry_refusals.
[Low] The mark is also kept on replace=True: a target that a launch script swapped in is listed as configuration and can be referenced by a saved target, but after a restart that name resolves to the startup target instead, without warning.
| try: | ||
| self._registry.instances.validate_name_available(target_registry_name) | ||
| except ValueError as error: | ||
| if request.persist and target_registry_name in self._registry.instances: |
There was a problem hiding this comment.
[AI assisted]: [Low] Error types are inferred from ValueError: a name conflict here is detected as "a ValueError was raised and the name is now registered", and in _save_and_register_async any ValueError from register becomes a 409. A dedicated name-taken error from the registry would make both precise.
A taken name also returns 409 with persist but 400 without it; that keeps existing behaviour, but is worth unifying in a separate PR. Separately, the cleanup after a failed register deletes the recipe unconditionally even though create_recipe returns a version; deleting only if that version still matches avoids removing another writer's file.
| if turned_off is not None and saved_params.get(name) in (None, ""): | ||
| saved_params[name] = turned_off | ||
| metadata = await asyncio.to_thread(self._registry.get_registered_class_metadata, target_type) | ||
| parameters = metadata.parameters if metadata is not None else () |
There was a problem hiding this comment.
[AI assisted]: [Low] When class metadata is missing, this falls back to (), which quietly skips the stored-credential and temporary-reference checks (restore would then refuse the recipe on the next start). Restore uses registry.get_class_metadata(cls); using the same call here makes saving and restoring run exactly the same check and never fail open.
| - `GET /api/targets/{id}` - Get target details | ||
| - `POST /api/targets` - Create a target | ||
|
|
||
| A created target is Temporary: a restart or reinitialization removes it. With `"persist": true`, `register` left on, |
There was a problem hiding this comment.
[AI assisted]: [Low] This paragraph packs many rules and status codes into one block; a short list or table (rule → status code) would be easier to scan. Also, the origin field description (shown in the API docs) says temporary means "created through the API without saving", leaving out targets from a scan's launch scripts and loss on reinitialization, which the mapper and this README both mention.
| * or writes the ones saved on the machine running it. | ||
| */ | ||
| function createSeededBackendConfigFile(): string { | ||
| const folder = mkdtempSync(path.join(tmpdir(), "pyrit-seeded-backend-")); |
There was a problem hiding this comment.
[AI assisted]: [Low] The folder created by mkdtempSync is never deleted, so each seeded run leaves one behind in the temp directory. Removing it in a global teardown would clean it up.
There was a problem hiding this comment.
Could we make the persisted target definition a stable configuration contract rather than coupling it directly to the current Python class/module and constructor parameters?
Presets and suites offer a useful comparison: they persist versioned domain definitions and registered-name references. Presets still have some coupling through scenario_params, but their overall format isn't simply a runtime constructor recipe.
Here, renaming a constructor parameter or moving a class between modules can invalidate saved targets—and a restore failure blocks backend readiness. The description acknowledges that possibility after upgrades, but schema_version alone doesn't provide a compatibility strategy.
I'd prefer a versioned target definition resolved through the registry, with an explicit compatibility boundary between stored configuration and construction. This needn't mean a separate model for every target: retaining compatible parameter aliases or applying versioned transformations could suffice. Constructor-only refactors should leave saved definitions working; actual format changes should have explicit migrations and old-definition regression tests.
Was the direct constructor coupling intentional? If so, could we document the trade-off and the expected migration policy?
FileDocumentStorage now raises DocumentStorageUnavailableError when the folder or container can't be used, and DocumentUnreadableError when one document exists but can't be read. The message names the location, with any SAS token hidden, and a short reason. One backend handler maps both to 503 and logs the location and reason, with the traceback at debug level, so the per-route storage helpers are gone. A single read, a conditional save and a delete now follow the listing's rules: a folder, pipe or broken link named like a document is unreadable rather than missing, and on Blob only a deleted blob counts as missing. Blob sign-in, permission, network and missing-container errors stop the listing instead of marking every file unreadable. Each unreadable report keeps the real file name and the original error, and the listing's documents are read-only. Initializer delete removes a script that can't be read or decoded, and registering one no longer turns a storage write failure into a 400. Tests patch module-level aliases instead of os.
List recipes escapes the OS text it reports, such as a file name that isn't UTF-8, and a recipe whose JSON holds an unpaired surrogate escape is reported as invalid, so the models no longer need a JSON-only serializer. Recipes may start with a byte-order mark. A document without schema_version reads as version 1, and each file is checked against the format version of the model that reads it. Validation summaries shorten each location part and the whole message, since a location can repeat a stored key. Recipes and StoredRecipe are no longer frozen, since their params and tags never were, and StoredRecipe checks that its recipe type matches its registry. The listing's recipes are read-only, and it keeps the error behind each recipe that couldn't be read, for the log.
Listing catches only storage errors, so a bug is raised instead of being reported as unreadable storage, and a bug while planning is a build error rather than an invalid recipe. Each failure is logged once, with the error behind it and URL credentials removed. The report shows the error's text with each URL's user name, query and fragment removed, any SAS signature hidden, and the length capped. A class that moved within PyRIT is restored; one now registered from outside PyRIT still fails. A technique whose name or tag clashes is a name clash, and anything else registration refuses is an invalid recipe. A recipe that sets a server file or folder is refused before it is built. Class metadata comes from the registry's cache. The class docstring lists the steps the API's create runs that restore skips.
The runtime becomes ready even when some saved entries fail; the failures stay in the runtime status. Each failed entry keeps its name in its registry: looking it up, or taking the name, raises SavedEntryUnavailableError, which the backend answers with 409, so nothing else stands in for it. Administrators see the full message and everyone else a fixed explanation of the reason. A folder that can't be listed holds every unregistered name of its kind, and each entry it left unbuilt is reported. While a saved technique is held, a launch or estimate that picks techniques by all, default or a tag is refused with 409 for scenarios whose techniques come from the registry, since the broken technique's tags may be unknown. Naming techniques still works; naming the held one fails with its reason. Route catch-alls let the error through instead of turning it into a 500.
A launch script that registers a name held for a saved entry that couldn't be restored is refused, and the refusal is recorded even when the script catches it, so the launch stops instead of running with the script's entry in the saved one's place. A suite entry that fails on a held entry, or on a broad technique selection, shows the reason without the error's details.
The OpenAI SDK adds that variable's headers to every request, so a saved OpenAI target would send whatever the server puts there, which can be a credential. While it's set, saving an OpenAI target is refused and a saved one isn't restored; the message names the variable, never its value. Targets declare such variables in server_header_variables.
Description
This is one of the changes for saving Registry entries across restarts. It builds on #4025, which refuses saved targets that would authenticate with a key. Until the earlier PRs merge, the diff here also shows their changes.
Problem. A target created through
POST /api/targetsis gone when the backend restarts. Restore (#3909) brings saved entries back at every start, but nothing saves one yet, and the target list doesn't say which targets will come back.Change.
POST /api/targetstakes"persist": true, which needsregisterleft on and a name that starts with a lowercase letter and has at most 64 lowercase letters, digits and underscores (otherwise 422). The target is built as before, saved as a recipe underregistry_recipes_source(by default~/.pyrit/registry_recipes, so no Azure is needed) with the signed-in user recorded as the one who saved it, and then registered. If registering fails, the file is deleted again; if that delete fails too, the request gets 503 and the server log names the target and its folder, so the file can be removed.headersis set to"{}", so a saved one doesn't send this server'sOPENAI_ADDITIONAL_REQUEST_HEADERS. Then the request goes through the same check restore uses, so a target that would hold or use a key, or leave an address to this server's settings, gets 400, and the message says it can still be created without saving it. A saved target can use saved targets and targets from configuration, which keep their own sign-in, but not a Temporary target, such as a round-robin member that won't come back after a restart.origin:saved,configuration(registered by the server's startup scripts), ortemporary(gone after a restart or reinitialization, such as a target created withoutpersistor registered by a scan's launch scripts).registry_recipes_sourceat a new temp folder, so they don't read or write the targets saved on the machine running them.What stays the same. Creating a target without
persistworks as before, and permission checks are unchanged and come first, so a non-admin whose target would use a server credential still gets 403. A save runs inside the create request, and apply already reports busy while a request is in flight, so no new lock is needed. Restore is unchanged, except that it first marks the targets the startup scripts registered asconfiguration.Not covered.
Tests and Documentation
tests/unit/backend/test_target_service.py: an Entra ID target is saved and restore rebuilds it; a no-key target is saved without an owner when sign-in is off; a target that would hold or use a key isn't saved or registered; a taken name is a conflict only when saving; a saved file with the name is a conflict and stays as it was; the file is deleted when the target can't be registered; storage failures are reported as unavailable, and the log names the target, its folder and the error type without the error's text; two saves of one name save one target; a round-robin is saved only over targets that come back after a restart; a class registered under another name is saved so restore finds it; a failed save that can't be undone is reported as storage trouble; a reserved name is refused as invalid, not as taken; the list shows where each target comes from.tests/unit/backend/test_api_routes.py: a save conflict maps to 409 and a storage failure to 503; the saved-entries source and the signed-in owner (or none when sign-in is off) reach the service;persistneedsregisterand a valid name.tests/unit/backend/test_mappers.py: the origin of each registered target, and on the instance.tests/unit/backend/test_runtime_lifecycle.py: startup marks the targets its scripts register but not saved ones; a save in flight makes apply report busy.tests/unit/registry/test_instance_registry.pyandtest_registry_recipe_storage.py: startup entries are marked and keep the mark when replaced; a recipe is created once, lists back with its owner and version, and is deleted once.uv run --frozen --extra all python -m pytest -n 8 --dist=loadfile tests/unit/registry tests/unit/backend tests/unit/prompt_target tests/unit/models tests/unit/setup tests/unit/converter tests/unit/score: 19,488 passed, 9 skipped, 4 xfailednpm run lintandnpm run type-check(forplaywright.config.ts): passed.E2E_SEEDED_MODE=true npx playwright test --project=seeded --workers=1: 108 passed, with the backend started from the generated temp config.saved); the same name gets 409; a typed key, an empty key and an endpoint with?sig=get 400 without echoing the key; exactly one recipe file is written, with no key. After a restart the saved target is back and answers a chat message, and the Temporary one is gone.pyrit/backend/README.mdanddoc/getting_started/pyrit_conf.md: how to save a target, the name rule, what it can hold, theheadersdefault, the OpenAI SDK settings that still apply, and the status codes.No JupyText changes.