Repository navigation
FIX: Stop a scan whose launch scripts would replace a saved entry (part 6) - #3986
Open
varunj-msft wants to merge 170 commits into
Open
varunj-msft wants to merge 170 commits into
varunj-msft wants to merge 170 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
Contributor
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.
A suite launch runs its launch scripts during preflight and sends its scans without them, so the clash is raised there. Map it to 409 with its message, as a single launch does, and drop the per-entry message that a clash can no longer reach.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Scripts a scan runs at launch could replace an entry restored from a saved recipe, for everyone using the app, or leave it in place so the scan ran with it instead of their own. This makes the registries refuse that and the launch stop with 409, naming the script and the entry. Until the earlier PRs merge, the diff here also shows their changes.
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 launch runs its scripts before it creates anything, so it gets the same 409 and message and starts no scan.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.