Repository navigation
FIX: Keep backend media and upload inputs inside managed storage - #2958
varunj-msft wants to merge 8 commits into
Conversation
Media values sent to the backend (message pieces, converter previews, and prepended conversations) could name any existing local file or any URL, and converter file parameters could name any Azure Blob URL. The server then read or forwarded those values. They must now be uploaded content or point into this server's media storage: the media folders under the memory results path, or, when results are stored in Azure Blob Storage, those folders in the configured results container. Blob URLs must start with the results URL and a media folder, the way the serializer builds them, both as the storage reader reads them and as HTTP clients decode them. Prepended content is checked before attack or conversation rows are written. Converter previews and sent messages now reject url pieces, so the backend no longer downloads URLs outside its result storage. Stored history can still hold them. Targets that read local files or load model code now declare it with uses_host_resources, and the create-target API rejects those types (HTTPXAPITarget, HuggingFaceChatTarget). They can still be registered in Python or with an initializer, where the operator controls their settings. The media route and media persistence now share one containment helper, and the backend README documents the checks and the intentional exceptions.
|
I agree with keeping backend local paths and host-resource targets behind an operator-controlled boundary. A client-supplied path is interpreted on the server, so it should not allow access to arbitrary host files or model loading. I do not think we should apply the same blanket rejection to URLs. CoPyRIT users are trusted operators who own the infrastructure, and the backend already permits operator-selected target endpoints and raw HTTP requests. Under that trust model, rejecting media URLs does not remove a meaningful capability; it mainly makes valid media workflows harder. Could we change the policy to import URL-backed media into managed storage instead?
This keeps the useful invariant introduced by this PR—downstream components operate only on managed media—while also giving us stable content for retries and replay, avoiding repeated downloads, and preventing signed URLs from being forwarded to external model providers. A stricter deployment could still disable URL import or configure an allowlist, but I do not think rejection should be the only supported behavior. Can you update this PR to support controlled URL ingestion while retaining the local-path and host-resource restrictions? |
Media values, url pieces, and converter file parameters that are http(s) URLs are now downloaded once into prompt-memory-entries with a time limit, a size limit, and a redirect limit, and only the stored copy reaches converters and targets. Redirects are followed one hop at a time, so every hop must be a plain http(s) URL and redirect bodies are never read. url pieces take the media type of their content. Blob URLs inside the results container stay as references, without their query string. allow_media_url_import in .pyrit_conf turns URL import off.
Agreed, blanket rejection was too blunt. URLs are now imported instead: media URLs and url pieces are fetched once (60s, 100 MiB, max 3 redirects, each hop checked) into prompt-memory-entries, and converters and targets only see the stored copy, so signed URLs don't get forwarded to providers. url pieces are retyped to whatever media type came back, and converter file params accept URLs the same way. I added allow_media_url_import in .pyrit_conf for deployments that want it off and left an allowlist for a follow-up. Local paths and the HTTPX/HF target restrictions are unchanged. |
…rove-Backend-Input-Validation # Conflicts: # pyrit/backend/services/attack_service.py # pyrit/backend/services/message_send_service.py # tests/unit/backend/test_message_send_service.py
|
After another review, I would revise my earlier recommendation: I support the shared ingestion code, bounded downloads, and checks on backend file access, but import should be an explicit capability rather than a requirement for every URL. Our callers are trusted operators, and URL references are useful. 1. Preserve URL references and make type changes explicitCould we preserve URL-reference behavior and let callers explicitly choose import? Location: 2. Use the same source bytes for preview and sendPreview uses the imported copy, but its response still returns the original URL and type. Sending that URL with the preview's converted result downloads the source again. If the URL changes, the stored original can differ from the bytes used for conversion. Could preview return the managed source reference and resolved type for imported inputs, so preview and send use the same copy? Location: 3. Keep download failures useful for debuggingCould download errors identify the failure reason without exposing credentials? Connect timeout, read timeout, the total deadline, and other network failures all become "could not be downloaded." The converter and create-attack routes return that message without logging the chained exception. Please include a safe failure category and the timeout limit where applicable, while keeping credentials and query strings redacted. Location: 4. Cover file references inside collectionsThe constructor-file policy only covers scalar Location: 5. Separate file access from local code executionCould we separate file access from model/code loading rather than reject both through Location: 6. Retain safe source information after importImport replaces the source URL with a generated storage path, but does not retain source information in the stored message. That makes it harder to trace which input produced an artifact. Could we record the credential-redacted source reference and the declared/resolved type in metadata? Please omit credentials and query strings by default. This belongs in ingestion metadata, not in each converter or target. Location: Scope notesThese are scope notes, not claims that this PR already adds these features:
Review drafted with assistance from GitHub Copilot. |
…ing from file access - Keep http(s) media values and url pieces as references by default. A caller imports a URL by setting import_url on a piece or preview, and the stored copy keeps the declared media path type. - Reject Azure Blob URLs outside the result storage as references, since the storage layer would read them from the server's own container. - Return the stored copy and its source metadata from a preview that imports, and record the redacted source URL and content type on imported pieces. - Name the reason and limit in download errors, and log each failure with the classes in its cause chain. - Replace uses_host_resources with loads_local_code and upload_directory_parameter: code-loading targets stay out of the API, HTTPXAPITarget uploads only from the operator's target_upload_directory, and target parameters that name server paths cannot be set through the API.
…PI can create Import failures are raised without the httpx exception chain, since httpx messages quote the full URL and background sends log the traceback. A URL httpx cannot parse is a 400 instead of a 500. A blank target_upload_directory is rejected rather than resolving to the working directory, and the target type catalog leaves out types and path parameters that creation always rejects.
httpx logs every request URL at INFO, query string included. While a media URL downloads, a filter on the httpx logger replaces those URLs with the redacted form; other httpx logging is unchanged.
httpcore logs response headers at DEBUG, so a signed redirect Location or Content-Location would reach the log. Those traces are dropped during imports; other httpcore logging is unchanged.
Thanks for taking another look. Going through them in order:
On the scope notes, agreed on all three. No cache, no host or private-network rules, and constructor uploads keep the storage and cleanup they have today. |
This branch has not been deployed
2 participants
Description
The backend is the part of PyRIT that takes input across a trust boundary. Some media and file inputs were not checked before use:
Path | strparameters accepted any Azure Blob URL.HTTPXAPITarget(uploads files named in params or message text) andHuggingFaceChatTarget(model_path,trust_remote_code) could be created through the API with caller-chosen files and code.Changes:
services/media_persistence.py, shared with the media route. Local media must resolve inside theprompt-memory-entries/seed-prompt-entriesfolders under the memory results path. When results are in Azure Blob Storage, a media URL under the configured results URL and a media folder is kept as a reference, without its query string. Other Azure Blob URLs are rejected as media references, because the storage layer would read them from this server's own container; those needimport_url. Data URIs and raw base64 uploads are unchanged.urlpieces pass through untouched.import_url: trueon a message piece or a converter preview request downloads the URL once withservices/media_url_import.pyand stores it underprompt-memory-entrieswith the declared type (image_path,audio_path,video_path, orbinary_path). Content from a different media family than the declared type is rejected; generic content types keep the declared type. Aurlpiece can't be imported, since it has no media type. Limits:Accept-Encoding: identity, and no request credentials forwarded.original_value, its type, and the source metadata. Sending those reuses the stored copy without downloading again.media_source_url(no credentials, query string, or fragment) and, when known,media_source_content_type; converted values useconverted_media_source_*. This is recorded at ingestion, including attack creation.Location, are dropped; other HTTP client logging is unchanged. A URL httpx can't parse is a 400.PathandPath | strparameters take an upload (data URI) or an http(s) URL. A URL is downloaded into the converter's upload directory and removed with the converter.Path | stralso accepts a managed results blob URL as a reference.allow_media_url_import: falsein.pyrit_conf(defaulttrue) turns imports off.create_attackmaps "not found" to 404 and other validation errors to 400 (same asadd_message).PromptTarget.loads_local_codemarks targets that load model code.HuggingFaceChatTargetsets it and is never created through the API.PromptTarget.upload_directory_parameternames the parameter for a target's upload directory.HTTPXAPITargetsets it (allowed_upload_directory) and can be created through the API only whentarget_upload_directoryis set in.pyrit_conf. The server passes that directory to the target, and callers can't set it. A blank value is rejected when the config loads.PathorPath | strcan't be set through the API. This also coversGitHubCopilotTarget.working_directory./api/targets/typesleaves out the target types and parameters that creation would reject. Registry metadata keeps them.pyrit/backend/README.md"Input Validation" covers what is checked and the intentional exceptions (operator-chosen target destinations and media URLs, unfiltered prompt content, any payload file type, public/api/media, admin-only custom initializers)..pyrit_conf_exampledocumentsallow_media_url_importandtarget_upload_directory.Coordination and known limits:
GridCompositeConverter.innocuous_images. When resolvingtarget_service.pyandconverter_service.py, keep both sides: FIX: Accept only supported external inputs when building components #2959'screate_instance_from_external_inputand external catalog filter, and this PR's_reject_server_resources,_can_create_through_api, path-parameter filtering, and upload directory injection.runtime_lifecycle.py; keep both.*_pathvalues from disk can't read an http(s) reference; setimport_urlfor those.Path.is_file()before containment, so the error tells an existing outside file apart from a missing one. File contents are never returned. This matches the shared-team trust model from FIX: Sanitize externally visible API errors #2237.\\host\share\x.pngis rejected, but only afterPath.is_file()looks it up. Rejecting outside paths before any filesystem call is a follow-up; it has to keep accepting raw base64 that looks like a path (JPEG base64 starts with/9j/).HTTPXAPITarget: instances registered in Python still upload files named in message text whenallowed_upload_directoryis unset. That behavior is deprecated, with removal planned for 1.3.0.Tests and Documentation
test_media_url_import.py: size, redirect, and credential limits; failure reasons and limits; redacted logging; no chained httpx exception; redacted httpx request lines and dropped httpcore traces during imports only; unparseable URLs; the off switch.test_media_persistence.py: containment, traversal, symlinks, remote results root, encoded separators checked against the real storage reader; references kept by default; import keeping the declared type; blob reference rules; source metadata.test_message_send_service.py: a failed import in a background send (count 1 and 2, real SQLite) logs no query string at INFO.test_converter_service.py(preview import returns the stored copy and metadata),test_target_service.py(code-loading and upload targets, server path parameters, configured upload directory, catalog),test_attack_service.py(source metadata on attack creation),test_api_routes.py,test_conversation_editor.py,test_runtime_lifecycle.py,tests/unit/setup/test_configuration_loader.py(blank and non-stringtarget_upload_directory).python -m pytest tests/unit/backend tests/unit/setup tests/unit/prompt_target tests/unit/converter -n 4: 6379 passed, 162 skipped. Four MCP/tool test modules fail to import in this environment, on main too.ruff format/ruff check: passed.ty check pyrit/: same diagnostics as main.805d843a9: all checks passed, including unit tests on ubuntu, windows, and macOS across Python 3.11–3.14.pyrit_backendwith a local config and.env) against a local listener:urlpieces kept, explicit import with the declared type and source metadata, preview-then-send reusing one download, failure reasons without query strings, converter file parameters from URLs, Hugging Face and caller-chosen directories rejected, HTTPX uploading only from the configured directory.target_upload_directoryfails at startup with a clear error.LocationorContent-Locationheader. Requests outside an import log as before.