Repository navigation
[BugFix] security: restrict untrusted image decoding formats - #9114
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for routing these through the vLLM allowlist, the call-site swap itself looks correct (open_image is a thin Image.open(fp, formats=ALLOWED_IMAGE_FORMATS), and denied formats still surface as UnidentifiedImageError/OSError, so the existing except clauses keep working).
A few things worth addressing before merge:
- Ascend image compatibility:
docker/Dockerfile.npustill builds onvllm-ascend:v0.31.0, andvllm/multimodal/image.pyin vLLM v0.31.0 has noopen_image(it was added in v0.32.0). If that image still ships vLLM 0.31, the new module-level imports inserving_chat.py,images/helpers.py,video_api_utils.pyand the duplex plugins would fail at import time and the NPU server would not start. Could you confirm the NPU image, or add a small local fallback (Image.open(fp, formats=...)with the same allowlist) until the Ascend base moves to 0.32? - MiniMax H3 HEIC/HEIF: see inline. The allowlist has no
HEIF, so these formats can no longer be decoded even withpillow-heifinstalled, but they are still advertised. - Tests: none of the listed tests exercises a denied format. A small parametrized case that feeds an EPS header (
b"%!PS-Adobe-3.0 EPSF-3.0...") into_decode_image_bytes,_load_input_imagesand the H3 upload validator, and asserts a 400 /UnidentifiedImageError, would lock in the security property so a futureImage.openregression gets caught.
For reference, the remaining Image.open call sites I checked look fine to leave as-is: the diffusion pipelines only open str/path inputs from offline callers, and experimental/fullduplex/video_stacking.py is client-side (used by clients/duplex.py and the demo).
amy-why-3459
left a comment
There was a problem hiding this comment.
Inline notes for points 1 and 2 above.
|
This PR appears to belong to: docs/design/module/ar_runtime.md, docs/design/module/observability.md, docs/design/module/model_integration.md. Module owners: @fake0fan @Gaohan123 @gcanlin Routing: @fake0fan via module of the changed files; @Gaohan123 via module of the changed files; @gcanlin via module of the changed files @NickCao, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
|
Also added some regression test for the denial case. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. Looked at the two new commits (8a2934b, e3c212e): the HEIC/HEIF cleanup in both MINIMAX_H3_REFERENCE_IMAGE_FORMATS constants and error messages looks right, and the new test_untrusted_image.py is a good regression guard. Patching EpsImageFile._open to raise proves the EPS decoder is never reached, not just that some error came back, and since Pillow re-raises non-syntax exceptions from plugin _open, a future bare Image.open would fail these tests loudly.
A few remaining items:
- Docs still advertise HEIC/HEIF (outside the diff, so noting here):
docs/serving/videos_api.md("Images use JPEG/PNG/WEBP/HEIC/HEIF (30 MiB)") andrecipes/MiniMaxAI/MiniMax-H3.md("Reference images accept JPG/JPEG, PNG, WEBP, HEIC, or HEIF up to 30 MiB"). Worth updating them in this PR so the docs match the new behavior. open_imageon the Ascend image: this push doesn't touch theserving_chat.pyimport thread, so the vllm-ascend v0.31.0 question from the earlier review is still open. Just flagging it so it doesn't get lost before merge, since the import now sits at module level in the OpenAI entrypoints as well as the duplex plugins.- Minor, see inline: HEIC/HEIF uploads now get the generic "Invalid uploaded image reference" error instead of the format list.
amy-why-3459
left a comment
There was a problem hiding this comment.
Checked d7d5342f and 9f5581ee. The docs and the H3 recipe no longer advertise HEIC/HEIF, and a failed decode now returns the supported-format list, with a test for the .heic case. Thanks. The one item from my earlier review that's still open is the module-level from vllm.multimodal.image import open_image (in helpers.py and serving_chat.py). docker/Dockerfile.npu is still on vllm-ascend:v0.31.0 at this head, so that import path depends on the NPU base moving to vLLM 0.32 first.
Route public image and video ingress through the vLLM audited Pillow format allowlist. Although listed by MiniMax H3 validation, HEIC/HEIF were not supported in a clean install because the project does not include a HEIF decoder dependency. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Nick Cao <ncao@redhat.com>
HEIC and HEIF are excluded by vLLM's image decoder allowlist, so remove them from H3's accepted format sets and error text. Keep their suffixes classified as images so uploads use image validation and size limits. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Nick Cao <ncao@redhat.com>
Cover EPS rejection at the public image, data URL, and MiniMax H3 upload paths. Assert that rejected image formats never reach the EPS decoder and that MiniMax H3 error messages list only supported formats. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Nick Cao <ncao@redhat.com>
Report the accepted formats for MiniMax H3 image decode failures and update the serving docs to match the decoder allowlist. HEIC/HEIF decoding was not supported in this path because no decoder dependency is installed. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Nick Cao <ncao@redhat.com>
Assert that image decode failures return the supported MiniMax H3 reference formats, including uploads whose names use an unsupported image suffix. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Nick Cao <ncao@redhat.com>
9f5581e to
ce024e0
Compare
NPU updated in #9143 |
hsliuustc0106
left a comment
There was a problem hiding this comment.
- Please attach execution evidence for the new EPS-denial regressions at
ce024e07931ddf683fcf086a3d9380865a8ab0a9. The Test Plan currently pins47916310and does not listtest_untrusted_image.py. Please include the command (pytest -q tests/entrypoints/openai_api/test_untrusted_image.py), Python/vLLM/Pillow versions and actual result, or link the existing job/log if these tests already ran. For the claimed security property, show the same three EPS cases fail with the unrestricted baseline decoder and pass with the reviewed allowlist path; identify the baseline revision or controlled revert used. Acceptance: the EPS decoder is not entered on the reviewed head, and the negative control catches restoring unrestricted decoding.
The regression code and public-ingress call-site changes have been inspected statically; no tests were executed here. Current CUDA is green, but its aggregate status does not identify whether these named tests ran. This is a specific verification gap, not a newly demonstrated code defect.
Updated and retested. Negative control: Failing as expected. |
linyueqian
left a comment
There was a problem hiding this comment.
Reviewed ce024e0. Every Image.open on a request path under vllm_omni/ now routes through open_image, covering the OpenAI image, chat, and video entrypoints, the streaming video handler, the MiniMax-H3 upload and preprocessing paths, and the Qwen3-Omni, MiniCPM-o 4.5, JoyAI, and Aura duplex plugins. Current main (88c9ccd) has no other raw Image.open on those paths; the remaining calls are in diffusion pipelines that open str/path inputs from offline callers, the benchmarks, and the client-side video_stacking.py.
Existing except clauses still catch the rejection because a disallowed format raises UnidentifiedImageError, which is an OSError. Patching EpsImageFile._open to fail in the new EPS tests proves the decoder is never entered, rather than only checking that an error was returned. The earlier NPU concern is resolved on main because docker/Dockerfile.npu moved to vllm-ascend v0.32.0 in #9143, which includes open_image.
Static review only; I did not run the tests locally. Main CUDA (build 17678) and the NPU lane pass at ce024e0.
vllm-project/vllm-omni@main:recipes 17bd19f3d775 → cbe4cc4b7c11 [latest.vllm-omni] vllm-project/vllm-omni@17bd19f...cbe4cc4 Commits: - vllm-project/vllm-omni@45d7c0051ec6 [Feat] Add Cosmos3-Nano Pipeline Parallel support (vllm-project/vllm-omni#4637) - vllm-project/vllm-omni@ea719bc6b99b [BugFix] security: restrict untrusted image decoding formats (vllm-project/vllm-omni#9114) Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
vllm-project/vllm-omni@main:recipes c829af446829 → 0131ae1e7330 [latest.vllm-omni] vllm-project/vllm-omni@c829af4...0131ae1 Commits: - vllm-project/vllm-omni@45d7c0051ec6 [Feat] Add Cosmos3-Nano Pipeline Parallel support (vllm-project/vllm-omni#4637) - vllm-project/vllm-omni@ea719bc6b99b [BugFix] security: restrict untrusted image decoding formats (vllm-project/vllm-omni#9114) Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…mni@78610269b446 vllm-project/vllm@main:recipes c3117c7441dd → 37cf71255259 [latest.vllm] vllm-project/vllm@c3117c7...37cf712 Commits: - vllm-project/vllm@74ba6384c2f0 [Doc][ROCm] Document DeepSeek-V4.1-Flash MI355X AgentX TP2 profiles (vllm-project/vllm#63344) - vllm-project/vllm@ecd1e12553e6 [Recipe] MiniMax-M3: add B200 NVFP4 AgentX aggregated profile (vllm-project/vllm#63163) Recipes: - MiniMaxAI/MiniMax-M3 (modified): https://recipes.vllm.ai/MiniMaxAI/MiniMax-M3 - deepseek-ai/DeepSeek-V4.1-Flash (modified): https://recipes.vllm.ai/deepseek-ai/DeepSeek-V4.1-Flash vllm-project/vllm-omni@main:recipes c829af446829 → 78610269b446 [latest.vllm-omni] vllm-project/vllm-omni@c829af4...7861026 Commits: - vllm-project/vllm-omni@45d7c0051ec6 [Feat] Add Cosmos3-Nano Pipeline Parallel support (vllm-project/vllm-omni#4637) - vllm-project/vllm-omni@ea719bc6b99b [BugFix] security: restrict untrusted image decoding formats (vllm-project/vllm-omni#9114) Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…oject#9114) Signed-off-by: Nick Cao <ncao@redhat.com> Co-authored-by: Codex <noreply@openai.com> (cherry picked from commit ea719bc) Signed-off-by: tzhouam <tzhouam@connect.ust.hk>
#9339 (#9352) Signed-off-by: Chendi Xue <chendi.xue@intel.com> Signed-off-by: tzhouam <tzhouam@connect.ust.hk> Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Signed-off-by: Nick Cao <ncao@redhat.com> Signed-off-by: Cao Xuanning <2107254305@qq.com> Signed-off-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com> Signed-off-by: lm10-arg <347490427+lm10-arg@users.noreply.github.com> Co-authored-by: Chendi.Xue <chendi.xue@intel.com> Co-authored-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Nick Cao <ncao@redhat.com> Co-authored-by: Codex <noreply@openai.com> Co-authored-by: Cao Xuanning <109304380+advanced233@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Khairul Kabir <250204754+khairulkabir1661@users.noreply.github.com> Co-authored-by: khairulkabir1661 <khairulkabir1661@users.noreply.github.com> Co-authored-by: lm10-arg <linluo@icloud.com> Co-authored-by: lm10-arg <347490427+lm10-arg@users.noreply.github.com>
Contributor intent (required human answers)
All four questions must be answered by the human contributor before submission.
An agent may reuse explicit human answers with a public-safe source reference;
it must not invent, infer, or prefill missing answers. If an answer is missing
or materially ambiguous, ask the human and wait. Do not publish private
conversation content without authorization.
1. What outcome do you want to achieve?
Reduce the risk that crafted images trigger unsafe decoder behavior in publicly accessible vLLM-Omni services.
2. What is the highest-leverage variable for that outcome?
Whether untrusted image bytes reach Pillow through unrestricted plugin selection.
3. How will this work move that variable?
Route public image decoding through vLLM’s open_image, which restricts Pillow’s plugin choices at open time.
4. Whose customer or user needs does this address, and what are those needs?
Operators of public omni services need to reduce exposure to malicious uploads.
Purpose
Route public image and video ingress through the vLLM audited Pillow format allowlist.
Although listed by MiniMax H3 validation, HEIC/HEIF were not supported in a clean install because the project does not include a HEIF decoder dependency.
Test Plan
vLLM Version: 0.32.0
vLLM-Omni Commit: ce024e0
Test Result
passed
BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
Note
To avoid agent spam, vLLM-Omni limits contributors without write access to 5 open PRs. If this blocks well-intentioned critical work, please contact a committer to request bypass list access.
(anything written below this line will be removed by GitHub Actions)