Repository navigation
fix: Validate multimodal media URLs and load paths - #8282
Conversation
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
# Conflicts: # components/src/dynamo/common/multimodal/image_loader.py # components/src/dynamo/common/tests/multimodal/test_image_loader.py
zhongdaor-nv
changed the title
WalkthroughThis pull request introduces SSRF (Server-Side Request Forgery) protection across multimodal loaders by implementing a centralized URL validation framework. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/src/dynamo/common/multimodal/image_loader.py (1)
130-142:⚠️ Potential issue | 🟡 MinorAsymmetric local-path handling across loaders — document or align.
ImageLoader has an explicit early check at lines 133–136 that unconditionally rejects
file://and bare paths, even whenDYN_MM_LOCAL_PATHis set. In contrast, AudioLoader's docstring (line 107–108) explicitly states it "Supports http(s), data: URIs, file:// paths, and bare filesystem paths," with no early rejection. This meansvalidate_media_url(called at line 137) is unreachable forfile://inputs in ImageLoader, whereas AudioLoader/VideoLoader allowvalidate_media_urlto apply the policy normally.The early check also causes minor inefficiencies: the URL is parsed twice (lines 132 and 138), and
validate_media_urlcan never process thefile://case it was designed to handle (per lines 235–238 in url_validator.py).If this asymmetry is intentional (e.g., images cannot be served from local disk in the HTTP-fetch path), add a docstring to
load_imageexplaining the supported schemes and why local files are excluded. Otherwise, align with AudioLoader/VideoLoader by removing the early check and lettingvalidate_media_urlenforce the policy consistently across all media types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/common/multimodal/image_loader.py` around lines 130 - 142, The load_image method currently preemptively rejects file:// and bare paths before calling validate_media_url, causing asymmetry with AudioLoader/VideoLoader and skipping url_policy enforcement; either remove the early check and let validate_media_url(handle file:// and bare paths per DYN_MM_LOCAL_PATH) enforce policy—i.e., delete the ValueError block in load_image, call normalized_url = validate_media_url(image_url, self._url_policy) first, then parse normalized_url—or, if local-file rejection is intentional, add a clear docstring to load_image describing supported schemes and why file:///bare paths are disallowed; also avoid double-parsing by parsing only after normalization.
🧹 Nitpick comments (9)
components/src/dynamo/common/tests/multimodal/test_audio_loader.py (2)
103-105: Prefer constructor-injected policy overloader._url_policy = ....
AudioLoader(url_policy=UrlValidationPolicy())avoids reaching into a private attribute.📝 Proposed fix
- loader = AudioLoader() - loader._url_policy = UrlValidationPolicy() + loader = AudioLoader(url_policy=UrlValidationPolicy())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/common/tests/multimodal/test_audio_loader.py` around lines 103 - 105, The test directly assigns a private attribute loader._url_policy; instead, construct the AudioLoader with the policy by passing UrlValidationPolicy into its constructor (use AudioLoader(url_policy=UrlValidationPolicy())) inside test_load_audio_uses_vllm_media_connector so the test uses the public API rather than touching the private _url_policy field.
36-99: Nit: test names still reference the removed_normalize_audio_url.Same as the video test file — the
test_normalize_audio_url_*prefix is now misleading since these exercisevalidate_media_url. Consider renaming in a follow-up pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/common/tests/multimodal/test_audio_loader.py` around lines 36 - 99, The test functions in this file still use the old prefix test_normalize_audio_url_* even though the code exercised is validate_media_url (and related UrlValidationPolicy/AudioLoader behavior); rename each test function (e.g., test_normalize_audio_url_converts_local_paths → test_validate_media_url_converts_local_paths, test_normalize_audio_url_preserves_data_urls → test_validate_media_url_preserves_data_urls, etc.) to reflect validate_media_url (and similarly align any other test files that reference the `_normalize_audio_url` naming) so names match the current API being tested (keep the assertions and fixtures unchanged, only update the test function names and any references to them).components/src/dynamo/common/tests/multimodal/test_image_loader.py (1)
251-251: Nit: use a raw string formatchsince it's a regex with alternation.Ruff RUF043: the
|is a regex metacharacter. Switch to a raw string to make the intent explicit and silence the lint.📝 Proposed fix
- with pytest.raises(ValueError, match="scheme|not allowed"): + with pytest.raises(ValueError, match=r"scheme|not allowed"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/common/tests/multimodal/test_image_loader.py` at line 251, The regex alternation in the pytest.raises call uses a normal string "scheme|not allowed" which treats the pipe as a regex metacharacter but triggers RUF043; update the match argument to a raw string (e.g., r"scheme|not allowed") in the with pytest.raises(...) line in test_image_loader.py so the intent is explicit and the linter warning is silenced.components/src/dynamo/common/tests/multimodal/test_video_loader.py (2)
24-75: Nit: test names still reference the removed_normalize_video_url.These tests now exercise
validate_media_url; renaming totest_validate_media_url_*(ortest_video_url_validation_*) would keep discovery intuitive. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/common/tests/multimodal/test_video_loader.py` around lines 24 - 75, The test names still reference the removed _normalize_video_url; rename the functions to reflect the exercised API (validate_media_url) to improve test discovery and clarity. Update each test function name in components/src/dynamo/common/tests/multimodal/test_video_loader.py (e.g., change test_normalize_video_url_converts_local_paths -> test_validate_media_url_converts_local_paths, test_normalize_video_url_preserves_data_urls -> test_validate_media_url_preserves_data_urls, etc.) while leaving the bodies unchanged; ensure names consistently reference validate_media_url (and keep references to UrlValidationPolicy and UrlValidationError intact).
88-92: Prefer constructor-injected policy over private-attribute mutation.
loader._url_policy = UrlValidationPolicy()on Line 92 reaches into the private state. SinceVideoLoader(url_policy=...)is already available, passing the policy at construction time is cleaner and less brittle to future refactors.📝 Proposed fix
- loader = VideoLoader() - # data: scheme is in the default allowlist regardless of env flags. - loader._url_policy = UrlValidationPolicy() + # data: scheme is in the default allowlist regardless of env flags. + loader = VideoLoader(url_policy=UrlValidationPolicy())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/common/tests/multimodal/test_video_loader.py` around lines 88 - 92, The test mutates the private attribute loader._url_policy; instead construct the loader with the policy instead: instantiate VideoLoader(url_policy=UrlValidationPolicy()) in test_load_video_uses_vllm_media_connector so you avoid touching the private _url_policy field and keep the test robust to refactors.components/src/dynamo/common/tests/multimodal/test_url_validator.py (2)
206-206: Nit: escape regex metacharacters inmatch=.The
.characters are regex wildcards. Ruff RUF043 flags this; also, in principle it could false-match a non-literal169X254X169X254.📝 Proposed fix
- with pytest.raises(UrlValidationError, match="169.254.169.254"): + with pytest.raises(UrlValidationError, match=r"169\.254\.169\.254"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/common/tests/multimodal/test_url_validator.py` at line 206, The test currently uses pytest.raises(UrlValidationError, match="169.254.169.254") which treats dots as regex wildcards; update the assertion to match the literal IP by escaping regex metacharacters (e.g., change the match to "169\\.254\\.169\\.254" or build the pattern with re.escape) so the test asserts the exact string; locate the pytest.raises call referencing UrlValidationError in test_url_validator.py and replace the match argument accordingly.
399-416: Couple the test to_MAX_REDIRECTSrather than the literal3.The comment embeds the invariant "
_MAX_REDIRECTSis hardcoded at 3", but the list hardcodes 4 responses. If the constant ever changes, this test will silently stop exercising the limit (or over-trip it). Import the constant and build the list dynamically so the intent survives refactors.📝 Proposed fix
-from dynamo.common.multimodal.url_validator import ( +from dynamo.common.multimodal.url_validator import ( + _MAX_REDIRECTS, UrlValidationError, ... ) ... - # _MAX_REDIRECTS is hardcoded at 3; we need 4 redirect responses to trip it. + # Build _MAX_REDIRECTS + 1 hops to trip the limit. policy = UrlValidationPolicy(allow_private_ips=True) def _hop(src: str, dst: str) -> MagicMock: return _mock_response(status_code=302, location=dst, request_url=src) - client = _mock_client( - [ - _hop("https://example.com/a", "https://example.com/b"), - _hop("https://example.com/b", "https://example.com/c"), - _hop("https://example.com/c", "https://example.com/d"), - _hop("https://example.com/d", "https://example.com/e"), - ] - ) + hops = [ + _hop(f"https://example.com/{i}", f"https://example.com/{i + 1}") + for i in range(_MAX_REDIRECTS + 1) + ] + client = _mock_client(hops)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/common/tests/multimodal/test_url_validator.py` around lines 399 - 416, The test test_fetch_with_revalidation_enforces_redirect_limit is currently hardcoded to 4 redirect responses based on a comment that _MAX_REDIRECTS == 3; instead import the actual _MAX_REDIRECTS constant and generate N = _MAX_REDIRECTS + 1 hops so the test triggers the redirect limit regardless of changes to the constant. Specifically, update the setup that builds the list passed to _mock_client (used with _mock_response and the helper _hop) to programmatically create a chain of redirect responses of length _MAX_REDIRECTS + 1, leaving the rest of the test (UrlValidationPolicy, fetch_with_revalidation and the pytest.raises assertion) unchanged.components/src/dynamo/common/multimodal/url_validator.py (2)
191-194: Nit: adjacent string literals with a stray split.
"Local media paths are not permitted; set " "DYN_MM_LOCAL_PATH to enable"is two fragments concatenated at parse time — works, but looks like a leftover from a wrap. Collapse to a single literal.- raise UrlValidationError( - "Local media paths are not permitted; set " "DYN_MM_LOCAL_PATH to enable" - ) + raise UrlValidationError( + "Local media paths are not permitted; set DYN_MM_LOCAL_PATH to enable" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/common/multimodal/url_validator.py` around lines 191 - 194, The raise in UrlValidationError uses two adjacent string literals split awkwardly; update the raise in the URL validation code (the block that checks policy.allowed_local_path and raises UrlValidationError) to use a single consolidated string literal like "Local media paths are not permitted; set DYN_MM_LOCAL_PATH to enable" so the message is not split across two fragments.
183-217: Consider caching the resolvedallowed_local_path.
validate_local_pathre-resolvespolicy.allowed_local_pathon every call (stat + symlink walk). BecauseUrlValidationPolicyisfrozen=True, this is a safe candidate for memoization (e.g.functools.lru_cacheon a helper keyed by the policy'sallowed_local_path), or resolving once at policy construction. Low impact, but media loaders call this per request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/common/multimodal/url_validator.py` around lines 183 - 217, The function validate_local_path currently calls Path(policy.allowed_local_path).expanduser().resolve(strict=True) on every invocation; cache the resolved allowed path instead to avoid repeated stat/symlink walks by either resolving and storing it once when constructing UrlValidationPolicy (e.g. add a resolved_allowed_path attribute during policy init) or by introducing a small helper (e.g. _resolve_allowed_local_path) decorated with functools.lru_cache keyed by policy.allowed_local_path and use that from validate_local_path; update validate_local_path to use the cached resolved path and keep the same error behavior if the configured path cannot be resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/src/dynamo/common/multimodal/url_validator.py`:
- Around line 169-180: validate_url currently calls the blocking
socket.getaddrinfo which stalls the event loop when validate_url is invoked from
the async fetch_with_revalidation path; change validate_url to provide a
non-blocking async resolution path used by fetch_with_revalidation by replacing
the direct socket.getaddrinfo call with an async resolve (e.g., await
asyncio.get_running_loop().getaddrinfo(...) or offload to
anyio.to_thread.run_sync) and then reuse the existing is_blocked_ip checks and
UrlValidationError handling so DNS resolution happens off the event loop while
keeping the same failure messages and behavior used in validate_url and
fetch_with_revalidation.
- Around line 235-238: The code currently treats "file" URIs by using
parsed.path and silently drops any authority; update the branch that handles
scheme == "file" to explicitly check parsed.netloc (or parsed.hostname) and
reject non-empty authorities instead of normalizing—e.g., if scheme == "file"
and parsed.netloc is not empty, raise an error (or return a policy rejection)
with a clear message; otherwise continue to call validate_local_path(raw_path,
policy) and return resolved.as_uri(). Ensure you modify the same conditional
that references parsed, scheme, raw_path, and validate_local_path so the
authority check happens before calling validate_local_path.
- Around line 246-292: In fetch_with_revalidation, before each redirect request
(inside the while loop after computing next_url but before calling
client.build_request/send), compute and compare the origin (scheme + host +
port) of current_url and next_url (use response.url.join(location) result or
parse both URLs) and if they differ create a per-request filtered_headers copy
that removes sensitive headers ("Authorization", "Cookie",
"Proxy-Authorization") instead of mutating the original headers variable; pass
that filtered_headers into client.build_request for this single hop only so
cross-origin redirects don't leak credentials while preserving headers for
same-origin hops, and keep validate_url, visited, hops logic unchanged.
---
Outside diff comments:
In `@components/src/dynamo/common/multimodal/image_loader.py`:
- Around line 130-142: The load_image method currently preemptively rejects
file:// and bare paths before calling validate_media_url, causing asymmetry with
AudioLoader/VideoLoader and skipping url_policy enforcement; either remove the
early check and let validate_media_url(handle file:// and bare paths per
DYN_MM_LOCAL_PATH) enforce policy—i.e., delete the ValueError block in
load_image, call normalized_url = validate_media_url(image_url,
self._url_policy) first, then parse normalized_url—or, if local-file rejection
is intentional, add a clear docstring to load_image describing supported schemes
and why file:///bare paths are disallowed; also avoid double-parsing by parsing
only after normalization.
---
Nitpick comments:
In `@components/src/dynamo/common/multimodal/url_validator.py`:
- Around line 191-194: The raise in UrlValidationError uses two adjacent string
literals split awkwardly; update the raise in the URL validation code (the block
that checks policy.allowed_local_path and raises UrlValidationError) to use a
single consolidated string literal like "Local media paths are not permitted;
set DYN_MM_LOCAL_PATH to enable" so the message is not split across two
fragments.
- Around line 183-217: The function validate_local_path currently calls
Path(policy.allowed_local_path).expanduser().resolve(strict=True) on every
invocation; cache the resolved allowed path instead to avoid repeated
stat/symlink walks by either resolving and storing it once when constructing
UrlValidationPolicy (e.g. add a resolved_allowed_path attribute during policy
init) or by introducing a small helper (e.g. _resolve_allowed_local_path)
decorated with functools.lru_cache keyed by policy.allowed_local_path and use
that from validate_local_path; update validate_local_path to use the cached
resolved path and keep the same error behavior if the configured path cannot be
resolved.
In `@components/src/dynamo/common/tests/multimodal/test_audio_loader.py`:
- Around line 103-105: The test directly assigns a private attribute
loader._url_policy; instead, construct the AudioLoader with the policy by
passing UrlValidationPolicy into its constructor (use
AudioLoader(url_policy=UrlValidationPolicy())) inside
test_load_audio_uses_vllm_media_connector so the test uses the public API rather
than touching the private _url_policy field.
- Around line 36-99: The test functions in this file still use the old prefix
test_normalize_audio_url_* even though the code exercised is validate_media_url
(and related UrlValidationPolicy/AudioLoader behavior); rename each test
function (e.g., test_normalize_audio_url_converts_local_paths →
test_validate_media_url_converts_local_paths,
test_normalize_audio_url_preserves_data_urls →
test_validate_media_url_preserves_data_urls, etc.) to reflect validate_media_url
(and similarly align any other test files that reference the
`_normalize_audio_url` naming) so names match the current API being tested (keep
the assertions and fixtures unchanged, only update the test function names and
any references to them).
In `@components/src/dynamo/common/tests/multimodal/test_image_loader.py`:
- Line 251: The regex alternation in the pytest.raises call uses a normal string
"scheme|not allowed" which treats the pipe as a regex metacharacter but triggers
RUF043; update the match argument to a raw string (e.g., r"scheme|not allowed")
in the with pytest.raises(...) line in test_image_loader.py so the intent is
explicit and the linter warning is silenced.
In `@components/src/dynamo/common/tests/multimodal/test_url_validator.py`:
- Line 206: The test currently uses pytest.raises(UrlValidationError,
match="169.254.169.254") which treats dots as regex wildcards; update the
assertion to match the literal IP by escaping regex metacharacters (e.g., change
the match to "169\\.254\\.169\\.254" or build the pattern with re.escape) so the
test asserts the exact string; locate the pytest.raises call referencing
UrlValidationError in test_url_validator.py and replace the match argument
accordingly.
- Around line 399-416: The test
test_fetch_with_revalidation_enforces_redirect_limit is currently hardcoded to 4
redirect responses based on a comment that _MAX_REDIRECTS == 3; instead import
the actual _MAX_REDIRECTS constant and generate N = _MAX_REDIRECTS + 1 hops so
the test triggers the redirect limit regardless of changes to the constant.
Specifically, update the setup that builds the list passed to _mock_client (used
with _mock_response and the helper _hop) to programmatically create a chain of
redirect responses of length _MAX_REDIRECTS + 1, leaving the rest of the test
(UrlValidationPolicy, fetch_with_revalidation and the pytest.raises assertion)
unchanged.
In `@components/src/dynamo/common/tests/multimodal/test_video_loader.py`:
- Around line 24-75: The test names still reference the removed
_normalize_video_url; rename the functions to reflect the exercised API
(validate_media_url) to improve test discovery and clarity. Update each test
function name in
components/src/dynamo/common/tests/multimodal/test_video_loader.py (e.g., change
test_normalize_video_url_converts_local_paths ->
test_validate_media_url_converts_local_paths,
test_normalize_video_url_preserves_data_urls ->
test_validate_media_url_preserves_data_urls, etc.) while leaving the bodies
unchanged; ensure names consistently reference validate_media_url (and keep
references to UrlValidationPolicy and UrlValidationError intact).
- Around line 88-92: The test mutates the private attribute loader._url_policy;
instead construct the loader with the policy instead: instantiate
VideoLoader(url_policy=UrlValidationPolicy()) in
test_load_video_uses_vllm_media_connector so you avoid touching the private
_url_policy field and keep the test robust to refactors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 304ab61e-8e62-44b1-9bc2-ccca3df804b2
📒 Files selected for processing (9)
components/src/dynamo/common/multimodal/audio_loader.pycomponents/src/dynamo/common/multimodal/http_client.pycomponents/src/dynamo/common/multimodal/image_loader.pycomponents/src/dynamo/common/multimodal/url_validator.pycomponents/src/dynamo/common/multimodal/video_loader.pycomponents/src/dynamo/common/tests/multimodal/test_audio_loader.pycomponents/src/dynamo/common/tests/multimodal/test_image_loader.pycomponents/src/dynamo/common/tests/multimodal/test_url_validator.pycomponents/src/dynamo/common/tests/multimodal/test_video_loader.py
|
Audio and video still have a redirect-based SSRF path. The new per-hop revalidation only exists in the image path (components/src/dynamo/common/multimodal/image_loader.py:95), but audio/video still do a one-time validate_media_url(...) and then hand the original hostname to vLLM’s MediaConnector.load_from_url_async(...) (components/src/dynamo/common/multimodal/audio_loader.py:94, components/src/dynamo/common/multimodal/video_loader.py:93). vLLM is pinned at pyproject.toml:53 0.19.0; vLLM’s official docs say VLLM_MEDIA_URL_ALLOW_REDIRECTS defaults to 1, and allowed_media_domains defaults to empty. That means a public URL can still 302 to 169.254.169.254 or another internal target for audio/video, so the PR leaves the main SSRF primitive open for 2 of the 3 loaders. (codex) |
|
The new hostname/IP check is only pre-flight and does not pin the validated IP to the actual connection. validate_url() resolves the hostname with socket.getaddrinfo(...) (components/src/dynamo/common/multimodal/url_validator.py:172), but fetch_with_revalidation() then sends the request using the hostname again (components/src/dynamo/common/multimodal/url_validator.py:273). The code comment explicitly calls this “best-effort” rebinding protection, so a rebinding-capable hostname can still pass validation on a public IP and connect on a private one. The same gap applies before audio/video are handed to vLLM. (codex) |
There was a problem hiding this comment.
Some Issues caught by my Local Code Reviewer
- Audio/video loaders bypass the new redirect revalidation — they call validate_media_url once then hand off to vLLM's MediaConnector, which follows redirects per VLLM_MEDIA_URL_ALLOW_REDIRECTS=1. Only the image path is actually protected, defeating the PR's stated objective.
here each loader ended up
Image loader (image_loader.py:189) does it right:
response = await fetch_with_revalidation(http_client, image_url, self._url_policy)
Dynamo owns the socket, so every hop is revalidated.
Audio loader (audio_loader.py:74-77):
normalized_url = validate_media_url(audio_url, self._url_policy)
return await connector.load_from_url_async(normalized_url, ...)
Video loader (video_loader.py:93-96):
normalized_url = validate_media_url(video_url, self._url_policy)
return await connector.load_from_url_async(normalized_url, ...)
The audio/video paths check the initial URL only, then hand the string to vLLM's MediaConnector.load_from_url_async. vLLM opens its own httpx client and follows redirects
according to its own env var (VLLM_MEDIA_URL_ALLOW_REDIRECTS, default 1). Dynamo's per-hop validator never runs.
Why that's a real bug, not theoretical
Attack, step by step:
1. User submits https://totally-legit.example.com/song.wav.
2. validate_media_url resolves totally-legit.example.com → 93.184.216.34 (public). ✅ passes.
3. String gets passed to vLLM's MediaConnector.
4. vLLM GETs the URL. Server responds 302 Location: http://169.254.169.254/latest/meta-data/iam/security-credentials/.
5. vLLM follows the redirect (default on). Fetches AWS instance-metadata creds from inside the VPC.
6. vLLM returns bytes-that-are-actually-IAM-credentials as "audio," which gets echoed back to the caller through the model pipeline / error path.
The image loader is immune because fetch_with_revalidation would reject the hop at step 5. Audio and video are wide open.
@grahamking wdyt abt this ? [ updated: This one is similar to ur codex review ]
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
Signed-off-by: zhongdaor <zhongdaor@nvidia.com>
github-actions
Bot
added
the
documentation
Claude suggest: Full DNS-rebinding protection in Python requires per-connection IP pinning — doable but non-trivial (~100 lines of custom aiohttp resolver or httpx/httpcore network-backend surgery), and it's a separate design decision with its own testing burden. Out of scope for this PR. The pragmatic recommendation for deployments concerned about rebinding is network-layer egress restriction (iptables / K8s NetworkPolicy blocking). |
…lcalling
Two legacy VLLMConfig entries in tests/serve/test_vllm.py started
failing in today's nightly vllm amd64 job with:
Exception: Failed to load image from http://...:
http:// URLs are not allowed; set DYN_MM_ALLOW_INTERNAL=1 to enable
The strict URL validator landed in #8282 and correctly plumbed the
env var through the factory for VLLM_MULTIMODAL_PROFILES-generated
configs in tests/utils/multimodal.py, but missed these two hand-
authored configs that still use http:// URLs (cocodataset HTTP image
and the local test fixture server at localhost:8806).
Set DYN_MM_ALLOW_INTERNAL=1 on both configs so they can load http://
payloads like before, matching what the factory already does for the
mm_* generated configs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
Signed-off-by: zhongdaor <zhongdaor@nvidia.com> Signed-off-by: zhongdaor-nv <zhongdaor@nvidia.com>
Signed-off-by: zhongdaor <zhongdaor@nvidia.com> Signed-off-by: zhongdaor-nv <zhongdaor@nvidia.com>
This branch was previously deployed
3 participants
What changed
This branch hardens multimodal media loading by centralizing URL validation into a new
url_validatormodule and routing the audio, image, and video loaders through that policy.The change adds:
file://and bare filesystem pathsWhy it changed
The multimodal loaders accepted user-controlled media locations without a shared validation layer. That left path handling and remote fetch behavior inconsistent across loaders and made it harder to enforce a single policy for allowed schemes, local-path access, and redirect handling.
Impact
Developers now have one place to control multimodal media URL policy, with env-driven configuration for internal network access and allowed local media paths.
Validation
Ran:
timeout 120s env PYTHONPATH=components/src ~/dynamo/bin/python -m pytest -q components/src/dynamo/common/tests/multimodal/test_audio_loader.py components/src/dynamo/common/tests/multimodal/test_image_loader.py components/src/dynamo/common/tests/multimodal/test_url_validator.py components/src/dynamo/common/tests/multimodal/test_video_loader.pyCloses DIS-1800
Summary by CodeRabbit
New Features
Bug Fixes