Skip to content

Add filtering for chat template kwargs#25794

Merged
DarkLight1337 merged 7 commits intovllm-project:mainfrom
russellb:vllm-ghsa-6fvq-23cw-5628
Sep 27, 2025
Merged

Add filtering for chat template kwargs#25794
DarkLight1337 merged 7 commits intovllm-project:mainfrom
russellb:vllm-ghsa-6fvq-23cw-5628

Conversation

@russellb
Copy link
Member

@russellb russellb commented Sep 27, 2025

Isotr0py and others added 4 commits September 19, 2025 22:21
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses a critical security vulnerability (GHSA-6fvq-23cw-5628) related to arbitrary code execution via malicious chat templates. The core of the fix is the new resolve_chat_template_kwargs function, which intelligently filters keyword arguments passed to the Jinja2 template renderer. It ensures that only arguments both supported by the apply_chat_template function and explicitly declared as variables within the template are passed, effectively blocking the injection of a malicious chat_template. The changes also introduce a --trust-request-chat_template flag for defense-in-depth, preventing user-supplied templates from being used unless explicitly allowed. The implementation is robust, and the accompanying tests correctly validate the fix. Overall, this is a solid and important security patch.

@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 27, 2025
@DarkLight1337
Copy link
Member

@Isotr0py PTAL at the V1 test failure

@DarkLight1337
Copy link
Member

Let's also reject the request if it passes an untrusted chat template, to avoid silent regressions

Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) September 27, 2025 09:12
@DarkLight1337 DarkLight1337 merged commit 7977e50 into vllm-project:main Sep 27, 2025
44 checks passed
simon-mo pushed a commit that referenced this pull request Sep 28, 2025
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Co-authored-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Signed-off-by: simon-mo <simon.mo@hey.com>
pdasigi pushed a commit to pdasigi/vllm that referenced this pull request Oct 2, 2025
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Co-authored-by: Isotr0py <mozf@mail2.sysu.edu.cn>
yewentao256 pushed a commit that referenced this pull request Oct 3, 2025
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Co-authored-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Signed-off-by: yewentao256 <zhyanwentao@126.com>
Comment on lines +1621 to +1625
resolved_kwargs = resolve_chat_template_kwargs(
tokenizer=tokenizer,
chat_template=hf_chat_template,
chat_template_kwargs=kwargs,
)
Copy link
Member

Choose a reason for hiding this comment

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

is it called by each request? do we need to cache it?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, it's called by each request. This function won't compile the chat template, so I think it won't introduces too much overhead.

Given that kwargs can be various depending on the request's extra body, it's not really applicable to cache it.

Copy link
Member

Choose a reason for hiding this comment

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

Add cache in #26227

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, it's called by each request. This function won't compile the chat template, so I think it won't introduces too much overhead.

Given that kwargs can be various depending on the request's extra body, it's not really applicable to cache it.

using npu(ascend 910c) it costs about 10-20ms, it is great to cache it

Choose a reason for hiding this comment

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

It seems this change might be linked to a ~10x latency jump we saw in our benchmarks on 0.11.0 (where CPU hit 100%).
The caching in 0.11.1 appears to solve it, but upgrading is a bit tricky since it requires CUDA ≥ 12.9.

Copy link
Member

Choose a reason for hiding this comment

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

You can still use older CUDA versions if you install vLLM from source.

@youkaichao
Copy link
Member

in which case do we ever use chat template from users? I thought we should just disallow it.

@bbrowning
Copy link
Contributor

We may also want a similar trust_request_chat_template around

chat_template=request.chat_template or self.chat_template,

choprahetarth pushed a commit to Tandemn-Labs/vllm that referenced this pull request Oct 11, 2025
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Co-authored-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Signed-off-by: simon-mo <simon.mo@hey.com>
shyeh25 pushed a commit to shyeh25/vllm that referenced this pull request Oct 14, 2025
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Co-authored-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Signed-off-by: simon-mo <simon.mo@hey.com>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Co-authored-by: Isotr0py <mozf@mail2.sysu.edu.cn>
alhridoy pushed a commit to alhridoy/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Co-authored-by: Isotr0py <mozf@mail2.sysu.edu.cn>
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
Signed-off-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Co-authored-by: Isotr0py <mozf@mail2.sysu.edu.cn>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants