Repository navigation
FIX: Keep backend media and upload inputs inside managed storage - #2958
varunj-msft wants to merge 9 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. |
Resolve imported formats from response and caller MIME types or URL suffixes without changing bytes or defaulting unknown media to WAV. Remove global HTTP logging filters and preserve download exception causes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
I recommend closing this PR and breaking up to smaller. I made #3061 as an example. Chatted offline but let's chat more later! |
2 participants
Description
Status: The separate media-import endpoint and the related simplifications below are implemented locally but are not yet committed or pushed. The GitHub PR head is still
2d4aa44b8. That head already removes the HTTP logging filters, restores exception chaining, and corrects imported format extensions.The backend accepts input across an API boundary. Callers are trusted operators, but a request must not let the server read arbitrary local files or load caller-selected model code. Operator-selected URLs, target endpoints, and adversarial prompt content remain supported.
Changes already on the PR branch:
prompt-memory-entriesorseed-prompt-entriesunder the memory results directory. The media route uses the same containment helper. Data URIs and raw base64 uploads remain supported.urlpieces pass through unchanged.PathandPath | strparameters accept a data URI or an http(s) URL. Downloads use converter-owned temporary files and cleanup, not durable media storage.Path | stralso accepts a managed results blob reference.HuggingFaceChatTargetcannot be created through the API. Targets that upload local files, such asHTTPXAPITarget, require the server's configuredtarget_upload_directory; callers cannot set that directory. Other target path parameters cannot be set through the API. The target catalog omits types and parameters that creation would reject.Local redesign, pending push:
POST /api/media/importtakes oneurl, one mediadata_type, and an optionalmime_typehint. The request model checks the modality once. Its response has exactlyvalue,data_type, andmime_type; it does not echo the source URL or add source metadata.import_urloption and its special rejection validators are removed; it was not a released API..bin, not a modality default such as.wav.Accept-Encoding: identityis used, and API request credentials are not forwarded.allow_media_url_import: falseturns off both explicit imports and constructor URL downloads. The existing GET media route remains public and limited to managed media files.Coordination and intentional limits:
GridCompositeConverter.innocuous_images; this PR handles scalar path parameters. Keep its external-input registry checks and this PR's path validation and upload-directory rules when merging. Keep both runtime lifecycle imports when integrating FEAT: Add ScenarioPreset #2931.prompt_metadata; the backend does not inject it.HTTPXAPITargetinstances retain the existing deprecated unrestricted-upload behavior when no upload directory is set.Tests and Documentation
Updated tests cover the import response shape, invalid modalities, authentication, compatibility headers, the off switch, unchanged bytes and downstream audio formats, reference reuse, independent original and converted MIME hints, and cleanup after write or filename failures and cancellation. Obsolete inline-import rejection and source-response assertions are removed. The README and configuration examples describe explicit import and its limits.
Local validation on the uncommitted redesign:
uv run --no-sync python -m pytest tests\unit\backend\test_media_persistence.py tests\unit\backend\test_media_route.py tests\unit\backend\test_converter_service.py -k "not test_symlink_escaping_results_is_rejected" -q: passed, 339 passed, 5 skipped, 1 deselected. The deselected symlink test requires a Windows privilege not available on this account.uv run --no-sync ruff format pyrit\backend\models\attacks.py pyrit\backend\models\converters.py pyrit\backend\models\media.py pyrit\backend\services\media_persistence.py pyrit\backend\routes\media.py tests\unit\backend\test_media_persistence.py tests\unit\backend\test_converter_service.py tests\unit\backend\test_media_route.py: passed.uv run --no-sync ruff check pyrit\backend\models\attacks.py pyrit\backend\models\converters.py pyrit\backend\models\media.py pyrit\backend\services\media_persistence.py pyrit\backend\routes\media.py tests\unit\backend\test_media_persistence.py tests\unit\backend\test_converter_service.py tests\unit\backend\test_media_route.py: passed.uv run --no-sync ty check pyrit\backend\models\attacks.py pyrit\backend\models\converters.py pyrit\backend\models\media.py pyrit\backend\services\media_persistence.py pyrit\backend\routes\media.py: passed.uv.lockis unchanged.No notebooks changed. JupyText was not run.