Repository navigation
FIX: Stop a scan whose launch scripts would replace a saved entry (part 6) - #3986
varunj-msft wants to merge 169 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/middleware/error_handlers.py # pyrit/backend/routes/scenario_presets.py # pyrit/backend/routes/scenario_suites.py # pyrit/registry/file_document_storage.py
…y the other storage tests do
The runtime status keeps its short summary. The restore log now also lists each field's full location and message, escaped, without the stored values, and with URL credentials removed. Credentials are removed before a location is shortened, so the summary can't keep part of one either.
Its text can carry a SAS URL. The 503 handler no longer logs it as a debug traceback, and the scenario suite service no longer attaches it to its warning; both log the storage message, which has no credentials.
Richard Lundeen (richlundeen)
left a comment
Richard Lundeen (richlundeen)
left a comment
There was a problem hiding this comment.
Reviewed this PR's own layer over #3909 (18 files; +218/-13 production lines), separately from the inherited persistence stack.
I do not recommend replacing the refusal tracking with just an exception. A custom initializer can catch the ordinary ValueError raised by create_named_instance for a saved name, return successfully, and leave the next scan to resolve the wrong saved component. This can happen on a fresh launch; disabling interrupted-scan resume does not remove it. The bounded collector detects that handled refusal before run admission.
One remaining P2 issue is noted inline: suite preflight does not map that saved-entry conflict to 409. This is a focused layer review, not approval of the inherited stack.
| str: The message to show for the entry. | ||
| """ | ||
| # A clash with a saved entry names only the launch script and the entry, so it's safe to show. | ||
| if isinstance(error, SavedEntryLaunchConflictError): |
There was a problem hiding this comment.
[P2] Handle saved-entry conflicts at suite preflight
Handle this conflict during suite preflight too. launch_async runs the initializers before creating an execution, then clears them from child requests, so a real saved-name clash never reaches this handler. For example, a valid suite launched with initializers=["converters"] and a saved Base64Converter named rot13 raises SavedEntryLaunchConflictError during preflight. The suite route does not catch that type, so the global ValueError handler returns 400 instead of the direct-launch 409. Map it at the suite launch boundary, preserving the message and refusal before submission, and cover that real path rather than only injecting the exception from a mocked start_run_async.
Description
This is one of the changes for saving Registry entries across restarts. It builds on #3909. Until the earlier PRs merge, the diff here also shows their changes.
Problem. Scripts a scan runs at launch (
initializersonPOST /api/scenarios/runs) register their targets, converters and scorers withreplace=True. One that used the name of an entry restored from a saved recipe replaced that entry for everyone using the app. Scripts that skip a name that's already taken (the technique and tools scripts, round-robin grouping, and the scorer script's preferred scorers) left the saved entry in place, so the scan ran with it instead of their own.Change.
SAVED_ENTRY_METADATA_KEY, holding the kind).DefaultInstanceRegistry.register(..., replace=True)refuses to replace a marked entry withSavedEntryReplacementError. It's aRuntimeError, because the converter and scorer startup scripts skip any entry that raises aValueError.register_from_factories(techniques), the tools script, the target script's round-robin grouping, and the scorer script's two picks by name (the preferred scorer in each category andscale_and_refusal) refuse instead of using a marked entry in place of their own.register, from the name checkcreate_named_instanceruns first, or from the technique registry's case-insensitive name check. A suite shows the same message for that entry instead of the generic one.What stays the same. Startup and apply start from empty registries, run the scripts, then restore, so nothing is marked while the scripts run. Launches whose scripts don't touch a saved name, and scripts replacing startup entries, work as before. Creating entries through the API already refuses taken names.
Not covered. A custom script that changes registry contents without going through the registry methods. Picking a scorer by tag or by metrics still considers every registered scorer, saved ones included.
Tests and Documentation
tests/unit/registry/test_instance_registry.py: replacing a marked entry is refused and changes nothing; a refusal is recorded even when the caller catches it, including a caught name check against a marked entry.tests/unit/registry/test_attack_technique_registry.py:register_from_factoriesrefuses to skip a saved technique and still skips a startup one; a caught case-insensitive clash with a saved technique is recorded.tests/unit/registry/test_converter_registry.py: a caught name clash with a marked entry throughcreate_named_instanceis recorded.tests/unit/registry/test_registry_recipe_restore.py: restore marks the entries it rebuilds, with their kind, and leaves startup entries unmarked.tests/unit/backend/test_scenario_run_service.py: a launch whose script would replace a saved converter returns the conflict and keeps the saved object, for the real converter startup script, a script that raises, a script that catches the error itself, and a script that wraps it in its own error; a script error with no clash passes through unchanged; the tools script stops on a saved technique or converter with its name; a script can still replace a startup entry.tests/unit/setup/test_converter_initializer.py: the converter startup script doesn't swallow the refusal (it would if the error were aValueError).tests/unit/setup/test_scorer_initializer.py: the scorer script refuses to tag a saved scorer under a preferred name or asscale_and_refusal.tests/unit/setup/test_targets_initializer.py: round-robin grouping refuses to leave a saved target under the group's name.tests/unit/backend/test_scenario_suite_execution_service.py: a suite entry shows the clash message.pytest -n 4 --dist=loadfile tests/unit/backend tests/unit/setup tests/unit/registry tests/unit/models: 8,071 passed, 9 skipped, 4 xfailed.No JupyText changes.