orpiske commented on PR #24473:
URL: https://github.com/apache/camel/pull/24473#issuecomment-4922841323
> ### Discussion: passing tool arguments as an exchange variable instead of
headers
>
> This PR changes how tool arguments reach the route compared to
`camel-langchain4j-tools`: instead of one header per argument, everything is
wrapped in a single `AiToolArguments` object under the `AiTool.TOOL_ARGUMENTS`
exchange variable. We think this is the right call and want to make the
reasoning and consequences explicit before steps 2–5 build on it:
>
> **Behavioral changes**
>
> 1. **Behavioral break in steps 2–5**: every existing tool route
reading `${header.city}` or using sql `:#param` binding will fail at runtime,
not at startup. The upgrade guide entries promised in the roadmap need to spell
out the migration pattern very explicitly.
>
> 2. **Lost binding convenience**: the documented replacement patterns
should be either simple OGNL (`:#${variable.AiToolArguments.parameters[id]}`),
or a one-line processor extracting arguments into route-author-named headers
(safe, since the names are then trusted).
>
>
> **Why variables are safer than headers**
>
> * LLM output is untrusted input under Camel's threat model, and
headers are exactly the surface behind the
[CVE-2025-27636](https://github.com/advisories/GHSA-2c2h-2855-mf97)
header-injection family (the `allowedParams` filter in
`LangChain4jToolsProducer` is a hardening patch over this very risk).
>
> * Headers propagate downstream: a tool route doing `.to("http:...")`
or `.to("jms:...")` forwards LLM-controlled values as HTTP headers / JMS
properties under default filter strategies. Variables are never auto-propagated
and never drive component dispatch.
>
> * Wrapping all arguments under one constant key means even argument
_names_ can't pollute any namespace.
>
>
> **Open question**: keep the single wrapper variable (most conservative),
or additionally set one variable per _declared_ argument (`${variable.city}`)
for ergonomics? Declared names are route-author-chosen, so per-argument
variables would not reopen the injection surface.
>
> _Claude Code on behalf of @Croway_
>
> @zbendhiba @orpiske @davsclaus @oscerd this is something worth discussing,
from a user perspective the header approach is more elegant, `${header.city}`
or using sql `:#param` compared to
`:#${variable.AiToolArguments.parameters[id]}`
I am OK with this change, although:
* it does concern me a bit that processing the tool arguments get harder.
* headers propagating downstream may be a two-edged sword if that leads to
situations where integrations inadvertently leak credentials/PII/etc.
Whether to keep a single wrapper or per variable ... I don't have a strong
opinion. I think the one variable per argument approach would solve one of my
concerns above, but I am unsure how much more complicated it is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
