security(gateway): anchor api_server MEDIA tag resolution to safe paths - #57479
Closed
srojk34 wants to merge 1 commit into
Closed
security(gateway): anchor api_server MEDIA tag resolution to safe paths#57479srojk34 wants to merge 1 commit into
srojk34 wants to merge 1 commit into
Conversation
_resolve_media_to_data_urls's ad-hoc _MEDIA_TAG_RE matched any bare
token after MEDIA: (no absolute-path anchor) and read the resolved
path directly with no denylist. A relative/traversal path like
MEDIA:../../../../etc/passwd.png slipped through, and any image-
suffixed file the process could read (including under ~/.ssh, ~/.aws,
etc.) was base64-inlined into the API response if its path merely
appeared in the model's own final reply text.
Every other platform adapter's MEDIA: handling already goes through
two shared primitives in gateway/platforms/base.py:
- MEDIA_TAG_CLEANUP_RE, which anchors the path to ~/, /, or a
Windows drive letter plus a known deliverable extension.
- validate_media_delivery_path, which resolves symlinks and rejects
paths under the credential/system-path denylist.
Reuse both here instead of the local unanchored pattern and naive
Path().expanduser() resolution.
Contributor
|
obsolete The issue this PR closes appears to be resolved already. Please reopen with a fresh target if this still covers a distinct gap. Signed: GPT-5.5-low in Codex |
Contributor
|
Merged via PR #57660 — your commit was cherry-picked onto current main with your authorship preserved in git log (rebase-merge). Thanks for the fix! |
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.
Summary
_resolve_media_to_data_urls(added by #56959,feat(api-server): inline MEDIA: image tags as base64 data URLs for remote frontends) scans the model's own final response text forMEDIA:<path>tags and inlines matching local images as base64 data URLs, since remote OpenAI-compatible frontends can't read local file paths.Its matcher,
_MEDIA_TAG_RE, had no absolute-path anchor — it matched any non-whitespace token (or quoted string) afterMEDIA:, including a relative/traversal path likeMEDIA:../../../../etc/passwd.png. The resolved path was then read directly withPath(...).expanduser()+is_file()+ a 5MB size cap and no denylist at all.This recreates, from scratch, a vulnerability class this codebase has fixed independently at least 8 times on other platforms (issue #16721 and others). Every other platform adapter's
MEDIA:handling already goes through two shared, hardened primitives ingateway/platforms/base.py:MEDIA_TAG_CLEANUP_RE— anchors the path to~/,/, or a Windows drive letter (X:\/X:/), plus a known deliverable extension, with a proper trailing terminator.validate_media_delivery_path— resolves symlinks (Path.resolve(strict=True)), requires an absolute path, and rejects anything under the credential/system-path denylist (/etc,/proc,~/.ssh,~/.aws, Hermes credential stores, etc.) unless it's under an operator-allowlisted root.Impact before this fix: any image-suffixed file the hermes process could read — reachable via a relative path, or an absolute credential path if it happened to have (or was renamed/symlinked to have) an allowed extension — could be exfiltrated as base64 to a remote API client if its path merely appeared in the model's own reply text (e.g. via prompt injection, an echoed example string, or stale tool output).
Fix
Replace the local
_MEDIA_TAG_REpattern and naive path handling in_to_data_urlwith the sameMEDIA_TAG_CLEANUP_RE/validate_media_delivery_pathpair every other adapter uses. The existing_MEDIA_IMG_EXT/_MEDIA_MIMEmaps are kept as-is — they narrow the broaderMEDIA_DELIVERY_EXTSallowlist down to the specific image types this function inlines (PDFs/videos/etc. still correctly fall through untouched, matching current behavior).Test plan
main: absolute non-denylisted path inlines correctly;MEDIA:../../../../tmp/x/secret.pngis left as literal text (not absolute, regex doesn't match);MEDIA:~/.ssh/id_rsa.pngis left as literal text (denylist rejects it).test_relative_traversal_path_not_inlined,test_credential_path_not_inlined_even_with_image_extension, andtest_symlink_escaping_to_denylisted_target_not_inlined(symlink target under/etcmust not be inlined —validate_media_delivery_pathresolves symlinks before the denylist check) totests/gateway/test_api_server_media_data_urls.py.uv run --frozen --extra dev --extra messaging python -m pytest tests/gateway/test_api_server_media_data_urls.py -q— all 10 tests (7 existing + 3 new) pass.uv run --frozen --extra dev --extra messaging python -m pytest tests/gateway/ -k "api_server or media" -q— 709 passed, 4 skipped, 1 pre-existing unrelated failure (test_background_command.py::test_media_files_routed_by_type, a/varvs/private/varmacOS symlink assertion in an unrelated file — confirmed present on a cleanmaincheckout before this change, not a regression).ruff checkon both changed files — clean.