[mikemikimike]

Summary

• Validate multimodal image and audio URLs as HTTP(S), reject non-public and mixed DNS destinations, and support exact hostname allowlists through LIGHTLLM_MULTIMODAL_URL_ALLOWLIST.
• Pin requests to the validated public IP, preserve the original Host and TLS SNI, and reject redirects.
• Return generic errors for remote image and audio failures.

Fixes #1608

Issue: #1608

Tests

• python -m pytest unit_tests/utils/test_multimodal_utils.py -q (19 passed)
• python -m pytest unit_tests -q --tb=short could not complete collection on Windows because triton is unavailable; the same 174 collection errors and 7 skips occur on the base commit.

Summary by CodeRabbit

• Improvements
  • Multimodal resource URLs now accept only HTTP(S), reject embedded credentials and non-public destinations, and honor configured host allowlists.
  • Resource requests use the validated public address and do not automatically follow redirects.
• Bug Fixes
  • Audio and image preload failures now show a generic error instead of exposing the resource URL or underlying exception. Other resource types retain their existing error details.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 03653992-da78-4480-982d-c41fd157105d

📥 Commits

Reviewing files that changed from the base of the PR and between b799a58 and e3108a6.

📒 Files selected for processing: 3

• lightllm/server/multimodal_params.py
• lightllm/utils/multimodal_utils.py
• unit_tests/utils/test_multimodal_utils.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Multimodal URL fetching now validates URLs, rejects non-public destinations, and pins requests to resolved public IPs. Image and audio preload failures for URL resources return generic errors without exposing the URL or underlying exception.

Changes: Multimodal URL Fetch Security

Layer / File(s)	Summary
URL validation and destination policy lightllm/utils/multimodal_utils.py, unit_tests/utils/test_multimodal_utils.py	URL validation restricts schemes, credentials, hosts, and DNS results. It supports a hostname allowlist and a resolution timeout. Tests cover rejected destinations, public DNS results, and allowlist behavior.
Validated and pinned resource fetching lightllm/utils/multimodal_utils.py, unit_tests/utils/test_multimodal_utils.py	Fetching validates URLs before cache lookup, disables redirects, and sends requests to the resolved IP while preserving the original hostname for HTTP and TLS. Tests cover validation before client creation and pinned requests.
Sanitized image and audio preload errors lightllm/server/multimodal_params.py, unit_tests/utils/test_multimodal_utils.py	URL-based image and audio preload failures raise generic ValueError messages. Tests check that the messages do not expose the URL or HTTP status details.

Priority: ⬆️ High

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: High

Sequence Diagram(s)

sequenceDiagram
  participant Preload as Image or audio preload
  participant Fetch as fetch_resource
  participant Validate as validate_resource_url
  participant DNS as Host resolver
  participant HTTP as HTTP client
  Preload->>Fetch: Fetch URL resource
  Fetch->>Validate: Validate URL
  Validate->>DNS: Resolve hostname
  DNS-->>Validate: Return resolved addresses
  Validate-->>Fetch: Return validated URL fields and public IP
  Fetch->>HTTP: Stream GET to pinned IP with original hostname
  HTTP-->>Fetch: Return response content
  Fetch-->>Preload: Return resource bytes

Loading

Suggested reviewers: hiworldwzj, sangchengc, shihaobai

Merge Risk: ⚪ Minimal · up to e3108

No confirmed merge-blocking issue remains; complete the normal checks before merging.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 9.68% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title "fix(multimodal): prevent SSRF in remote resource fetching" matches the main change. The PR adds URL validation for multimodal image and audio URLs, blocks non-public addresses, pins request…
Linked Issues check		Issue [#1608] requires SSRF protection for multimodal image and audio URLs and generic remote-fetch errors. validate_resource_url accepts only HTTP(S), rejects credentials, enforces the exact `LIGHT…
Out of Scope Changes check		The changed implementation files and tests support issue [#1608]. They modify multimodal URL validation, safe fetching, error sanitization, and the related automated tests. No unrelated change is esta…

Full details: Title check

Explanation

The title "fix(multimodal): prevent SSRF in remote resource fetching" matches the main change. The PR adds URL validation for multimodal image and audio URLs, blocks non-public addresses, pins requests to a validated IP, disables redirects, and returns generic fetch errors. The title names the vulnerability class and the affected area. It is specific and concise, and a reviewer scanning history can identify the primary change.

Full details: Linked Issues check

Explanation

Issue [#1608] requires SSRF protection for multimodal image and audio URLs and generic remote-fetch errors. validate_resource_url accepts only HTTP(S), rejects credentials, enforces the exact LIGHTLLM_MULTIMODAL_URL_ALLOWLIST, and rejects private or mixed DNS results. fetch_resource pins the validated address, preserves the original Host and TLS SNI, and disables redirects. ImageItem and AudioItem return generic URL-fetch errors. The added tests cover these requirements, including 19 passing tests reported in the PR description.

Full details: Out of Scope Changes check

Explanation

The changed implementation files and tests support issue [#1608]. They modify multimodal URL validation, safe fetching, error sanitization, and the related automated tests. No unrelated change is established by the available PR evidence.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}