Skip to content

[Security] Reject reserved transfer keys in vllm_xargs - #56814

Open
jperezdealgaba wants to merge 1 commit into
vllm-project:mainfrom
jperezdealgaba:fix/reject-xargs-transfer-params
Open

jperezdealgaba wants to merge 1 commit into
vllm-project:mainfrom
jperezdealgaba:fix/reject-xargs-transfer-params

Conversation

@jperezdealgaba

Copy link
Copy Markdown
Contributor

Summary

OpenAI request conversion copies caller-controlled vllm_xargs into SamplingParams.extra_args and only overlays typed kv_transfer_params / ec_transfer_params when those top-level fields are present. Planting a reserved key inside vllm_xargs (including a non-dict) therefore reaches EngineCore, where connectors call .get() and the process dies. This change rejects reserved transfer keys at the API merge, type-checks them on SamplingParams, and stores only dicts on Request so EngineCore does not crash if extra_args is mutated after construction.

Changes

  • vllm/sampling_params.py: shared merge_request_extra_args that copies xargs, rejects reserved keys, then overlays typed fields; _verify_extra_args requires those keys to be dicts.
  • OpenAI completion, chat, and responses protocols, plus STT transcription/translation: use the shared merge helper.
  • vllm/v1/request.py: keep kv/ec transfer params only when they are dicts (do not raise in EngineCore).
  • Rust HTTP merge helpers: same reserved-key reject for completions, chat, and generate.

Codepath coverage

  • /v1/completions, /v1/chat/completions (including batch), /v1/responses, /v1/messages (via ChatCompletionRequest), /invocations, STT transcription/translation, Rust HTTP completions/chat/generate, offline SamplingParams, and EngineCore Request construction.

Duplicate-work check

Searched open and merged PRs for vllm_xargs, kv_transfer_params, and extra_args merge. #50353 copies vllm_xargs to avoid mutating the request dict but still overlays reserved keys and does not reject them. #55182 type-checks nested P2P role dicts only. Neither closes this sink.

Tests

  • test_completion_rejects_reserved_xargs_key / chat / responses: reserved keys in vllm_xargs raise VLLMValidationError.
  • test_merge_request_extra_args_overlays_typed_fields and test_completion_overlays_typed_kv_transfer_params: typed overlay still works; kv_cache_report_mode is not reserved.
  • test_completion_does_not_mutate_request_xargs: overlay does not mutate the request dict.
  • test_extra_args_rejects_non_dict_transfer_params: SamplingParams rejects non-dict transfer blobs.
  • test_request_ignores_non_dict_transfer_params: EngineCore stores None instead of crashing.
  • Rust: merge_kv_rejects_reserved_xargs_key, merge_ec_rejects_reserved_xargs_key, merge_kv_overlays_typed_field.

Commands:

  • .venv/bin/python -m pytest tests/test_sampling_params.py tests/v1/test_request.py tests/entrypoints/openai/test_xargs_transfer_params.py tests/entrypoints/openai/test_protocol_responses.py tests/entrypoints/anthropic/test_anthropic.py tests/v1/ec_connector/unit/test_ec_connector_routing.py -v — 69 passed (subset of these files).
  • .venv/bin/pre-commit run on the changed files — passed.
  • cargo test -p vllm-server merge_kv / merge_ec — 3 passed.

AI assistance

This PR was developed with AI assistance.

Made with Cursor

Co-authored-by: Cursor Agent
Signed-off-by: Juan Pérez de Algaba <jperezde@redhat.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify

mergify Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @jperezdealgaba.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant