Skip to content

fix: sanitize remote media safety errors#490

Merged
waybarrios merged 2 commits intowaybarrios:mainfrom
Thump604:fix/issue-488-ssrf-review-notes
May 8, 2026
Merged

fix: sanitize remote media safety errors#490
waybarrios merged 2 commits intowaybarrios:mainfrom
Thump604:fix/issue-488-ssrf-review-notes

Conversation

@Thump604
Copy link
Copy Markdown
Collaborator

@Thump604 Thump604 commented May 2, 2026

Closes #488.

This is a narrow follow-up for the SSRF review notes on remote media handling. It does not attempt to implement pinned-IP transport; instead it makes the current behavior explicit and removes the client-visible leakage from URL-safety failures.

Changes:

  • add a public-safe UnsafeRemoteURLError.public_message
  • validate remote media URLs during chat/Responses request preparation so unsafe streaming requests fail before emitting SSE data
  • return a generic 400 detail for blocked media URLs while logging the internal diagnostic server-side
  • document the remaining DNS rebinding / split-horizon DNS limitation and recommend egress controls or a trusted proxy for environments where that is in scope
  • add regression coverage for safe public errors and endpoint-level sanitization

Validation:

  • AI_RUNTIME_BYPASS_SAFETY_GATE=1 /opt/ai-runtime/venv-live/bin/python -m pytest tests/test_mllm.py tests/test_server.py -q -k 'unsafe_remote_url_error_has_safe_public_message or validate_url_safety or request_with_safe_redirects or blocks_unsafe_url_before_request or chat_completion_sanitizes_remote_media_safety_errors'
  • AI_RUNTIME_BYPASS_SAFETY_GATE=1 /opt/ai-runtime/venv-live/bin/python -m pytest tests/test_mllm.py -q
  • AI_RUNTIME_BYPASS_SAFETY_GATE=1 /opt/ai-runtime/venv-live/bin/python -m pytest tests/test_server.py -q
  • AI_RUNTIME_BYPASS_SAFETY_GATE=1 /opt/ai-runtime/venv-live/bin/python -m compileall -q vllm_mlx/models/mllm.py vllm_mlx/server.py
  • black --target-version py312 --check vllm_mlx/models/mllm.py vllm_mlx/server.py tests/test_mllm.py tests/test_server.py
  • uvx ruff check vllm_mlx/models/mllm.py vllm_mlx/server.py tests/test_mllm.py tests/test_server.py --select E,F,W --ignore E402,E501,E731,F811,F841
  • git diff --check

Copy link
Copy Markdown
Collaborator

@janhilgard janhilgard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good SSRF hardening. The key insight — validation must happen before any SSE data is emitted — is correctly implemented.

UnsafeRemoteURLError.public_message — clean separation between internal diagnostics (IP, hostname, reason) and client-facing message. The test explicitly verifies 169.254.169.254 appears in the internal error but NOT in public_message — exactly the right security boundary assertion.

Early validation placement:

  • _prepare_chat_messages() → catches both streaming and non-streaming chat completions before any response bytes are sent
  • create_response() streaming path → _validate_remote_media_urls(chat_request.messages) runs before StreamingResponse() constructor
  • _prepare_responses_request() → catches non-streaming Responses path

All three entry points validate before the point of no return (SSE start or response body).

_raise_remote_media_http_error — logs full details server-side via logger.warning, returns only the generic public_message via HTTPException(status_code=400). No information leakage to the client.

_iter_remote_media_urls — handles all multimodal content types (image_url, video_url, audio_url, image, video, audio) plus Pydantic model objects. The is_url() filter correctly skips base64 data URIs and local paths that don't need SSRF validation.

Documentation — honest about the DNS rebinding / split-horizon limitation. Recommending egress controls or a trusted proxy is the right advice rather than claiming complete protection.

Endpoint test — the AWS metadata endpoint (169.254.169.254) is the classic SSRF target, good choice. The assertion "169.254.169.254" not in response.text verifies no internal detail leaks in the full response body.

Single commit, CI green. LGTM.

@waybarrios
Copy link
Copy Markdown
Owner

The Anthropic /v1/messages endpoint is unprotected. The patch wraps _prepare_chat_completion_invocation and _run_responses_request with except UnsafeRemoteURLError, but create_anthropic_message calls _prepare_anthropic_invocation outside any try block, and that path goes through _prepare_chat_messages_validate_remote_media_urls. UnsafeRemoteURLError is a ValueError subclass with no app-level handler, so a failure surfaces as a 500 with a stack trace that contains the resolved IP.

# vllm_mlx/server.py:4900-4906 (current PR head)
release_on_exit = True
prepared = _prepare_anthropic_invocation(    # raises UnsafeRemoteURLError
    engine,
    openai_request,
    effective_max_tokens,
)

vllm-mlx/vllm_mlx/server.py

Lines 4895 to 4912 in d486a8d

total_timeout=total_timeout,
deadline=deadline,
model=openai_request.model,
)
if engine is None:
return Response(status_code=499)
release_on_exit = True
prepared = _prepare_anthropic_invocation(
engine,
openai_request,
effective_max_tokens,
)
try:
if anthropic_request.stream:
anthropic_terminal = (
f"event: message_stop\ndata: {json.dumps({'type': 'message_stop'})}\n\n"
)

The fix is the same pattern already used twice in this PR:

try:
    prepared = _prepare_anthropic_invocation(
        engine,
        openai_request,
        effective_max_tokens,
    )
except UnsafeRemoteURLError as exc:
    _raise_remote_media_http_error(exc)

Today this is partly masked because anthropic_to_openai._convert_message silently drops image blocks (only text / tool_use / tool_result are converted), so multimodal Anthropic requests never reach the validator in the first place. As soon as that converter is completed for image support, the gap becomes a live IP-leak path. Worth closing now while the patch is in flight.

The second thing worth flagging: the /v1/responses streaming path runs the validator twice. Once eagerly here:

# vllm_mlx/server.py:4692-4699
try:
    if request.stream:
        chat_request = _responses_request_to_chat_request(request)
        _validate_remote_media_urls(chat_request.messages)
        return StreamingResponse(
            _disconnect_guard(_stream_responses_request(request), raw_request),
            media_type="text/event-stream",
        )

vllm-mlx/vllm_mlx/server.py

Lines 4690 to 4708 in d486a8d

async def create_response(request: ResponsesRequest, raw_request: Request):
"""Create a Responses API response."""
try:
if request.stream:
chat_request = _responses_request_to_chat_request(request)
_validate_remote_media_urls(chat_request.messages)
return StreamingResponse(
_disconnect_guard(_stream_responses_request(request), raw_request),
media_type="text/event-stream",
)
response_object, _persisted_messages = await _run_responses_request(
request, raw_request
)
except UnsafeRemoteURLError as exc:
_raise_remote_media_http_error(exc)
if response_object is None:
return Response(status_code=499)

And once again, lazily, inside the generator via _prepare_responses_request:

# vllm_mlx/server.py:2034-2046
_validate_model_name(request.model)
engine = get_engine()
chat_request = _responses_request_to_chat_request(request)
...
_validate_remote_media_urls(chat_request.messages)

vllm-mlx/vllm_mlx/server.py

Lines 2030 to 2048 in d486a8d

def _prepare_responses_request(
request: ResponsesRequest,
) -> tuple[BaseEngine, ChatCompletionRequest, list[dict], dict]:
"""Prepare a Responses request for execution on the chat engine."""
_validate_model_name(request.model)
engine = get_engine()
chat_request = _responses_request_to_chat_request(request)
if chat_request.messages:
logger.info(
f"[REQUEST] POST /v1/responses stream={request.stream} "
f"model={request.model!r} items="
f"{len(request.input) if isinstance(request.input, list) else 1} "
f"tools={len(request.tools)}"
)
_validate_remote_media_urls(chat_request.messages)
messages, images, videos, audios = extract_multimodal_content(

The second call runs after StreamingResponse(...) has already returned, so any UnsafeRemoteURLError raised there is outside the outer except and either corrupts the SSE stream or surfaces as a generic 500. Concrete consequences:

  • Latency: every streaming /v1/responses request with media pays a duplicate DNS-resolving safety check.
  • Failure mode is mode-dependent: if the eager call ever gets removed or moved, the late call silently no-ops the security guarantee instead of failing closed.
  • The eager call already does the right thing in isolation, so the inner one is dead-as-a-guard.

Pick one site. Either drop the call inside _prepare_responses_request for the streaming path, or move all validation inside _prepare_responses_request and remove the eager copy — but document why if both stay.

There's also a third item worth noting but not blocking: the Responses API path runs _validate_remote_media_urls against messages that already passed through _response_content_to_text (server.py:1649-1666), which strips everything except text/input_text/output_text into plain text. Any image_url carried in ResponsesRequest.input is dropped before the validator sees it, so the validator on this path never has anything to validate. Either Responses is text-only by design (in which case the _validate_remote_media_urls call there is dead code) or it should preserve media blocks before validation.

vllm-mlx/vllm_mlx/server.py

Lines 1647 to 1670 in d486a8d

def _response_content_to_text(content) -> str:
"""Normalize Responses API content items into plain text."""
if content is None:
return ""
if isinstance(content, str):
return content
text_parts = []
for part in content:
if isinstance(part, dict):
part_type = part.get("type")
text = part.get("text", "")
else:
part_type = getattr(part, "type", None)
text = getattr(part, "text", "")
if part_type in {"text", "input_text", "output_text"}:
text_parts.append(text)
return "\n".join(part for part in text_parts if part)
def _responses_tools_to_chat_tools(
tools: list[ResponseFunctionTool | dict],

@Thump604
Copy link
Copy Markdown
Collaborator Author

Thump604 commented May 7, 2026

@waybarrios I addressed the follow-ups you flagged and pushed dda431be993758b234a0ebf4dd37c6f3a4b94e71.

Changes:

  • Anthropic /v1/messages now converts UnsafeRemoteURLError from request preparation into the same sanitized 400 response as the other endpoints.
  • Streaming /v1/responses now validates remote media before constructing StreamingResponse and skips the duplicate late validation inside the SSE generator path.
  • Non-streaming /v1/responses still validates through _prepare_responses_request.
  • I left the current Responses API media-shape note as-is rather than broadening this patch, since the current Responses model is text/tool focused and media preservation there is a separate design surface.

Validation:

  • AI_RUNTIME_BYPASS_SAFETY_GATE=1 PYTHONPATH=/opt/ai-runtime/worktrees/vllm-mlx/issue-488-ssrf-review-notes /opt/ai-runtime/venv-live/bin/python -m pytest tests/test_responses_api.py tests/test_server.py::TestReasoningAndToolCallsNonStreaming tests/test_mllm.py -q59 passed, 6 deselected
  • ruff check vllm_mlx/server.py tests/test_server.py tests/test_responses_api.py
  • black --check --target-version py312 vllm_mlx/server.py tests/test_server.py tests/test_responses_api.py
  • git diff --check

CI is green on the pushed commit as well.

@waybarrios
Copy link
Copy Markdown
Owner

Ok this looks better now. You got my approval!

@waybarrios waybarrios merged commit d1cc766 into waybarrios:main May 8, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow up PR #325 SSRF review notes

3 participants