Authorize and accept call_tool with nested tool_name - #6150
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6150 +/- ##
==========================================
+ Coverage 72.58% 72.63% +0.04%
==========================================
Files 736 736
Lines 76331 76353 +22
==========================================
+ Hits 55408 55457 +49
+ Misses 17001 16961 -40
- Partials 3922 3935 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Nothing red here is from your diff. The errors you're seeing come from three unrelated things: six jobs never ran any code, they died in checkout with Rebase when you get a chance to clear the checkout failures. Thanks for the PR. I will approve when you have rebased. |
Authorization read the call_tool target from the top level of the request arguments only. A caller that nested tool_name inside parameters missed that lookup and fell through the pass-through branch, reaching the backend with no policy check performed. Resolve the target through a shared helper so authorization and dispatch agree on which tool a request names. Signed-off-by: Tanguille <91473554+Tanguille@users.noreply.github.com>
LLMs frequently nest tool_name inside parameters rather than alongside it. These calls were rejected with a bare "tool_name is required", which gave the model no indication of the expected shape, so retries reproduced the same payload and the turn was lost. Decode such calls by resolving the target the same way authorization does, and report the expected shape and the parameter keys received when the name really is absent. Keys are listed without values so tool arguments stay out of logs. Fixes stacklok#5891 Signed-off-by: Tanguille <91473554+Tanguille@users.noreply.github.com>
The find->call round-trip only exercised the flat payload, so nothing verified that a nested tool_name dispatches through a real vMCP server rather than only through the decode unit tests. Signed-off-by: Tanguille <91473554+Tanguille@users.noreply.github.com>
d23c6cd to
572a960
Compare
|
Thanks for letting me know @aponcedeleonch. I have now rebased my changes. |
Summary
LLMs calling the optimizer's
call_toolfrequently nesttool_nameinsideparametersinstead of alongside it. Handling that turned out to require fixing authorization first, so this PR is two commits.1.
Authorize call_tool when tool_name is nestedhandleToolsCallread the target name from the top level of the request arguments only. A nestedtool_namemissed that lookup, and the pass-through branch fell through to "allow through" and reached the backend with no policy check performed at all. Cedar policy denying a backend tool was enforced for the flat form and silently skipped for the nested one.This is a live gap on
maintoday, though its impact is currently limited: authorization is skipped, then the optimizer rejects the call because it cannot findtool_nameeither. Making the optimizer accept the nested form (commit 2) would have removed that backstop, so this change was required.The resolution now goes through
optimizer.ResolveCallToolTarget, shared by both readers, so authorization and dispatch cannot disagree about which tool a request names.2.
Accept nested tool_name in call_tool inputCallToolInput.UnmarshalJSONresolves through the same helper, so the nested form dispatches instead of erroring. A top-leveltool_namealways wins andparametersis left untouched in that case, so a backend tool with its owntool_nameargument is unaffected.The
tool_name is requirederror now states the expected shape and echoes the received parameter keys. Previously a nesting mistake and a genuinely empty call produced identical text, which is why this went undiagnosed on my side for a while.The advertised tool schema is unchanged.
GenerateSchemareflects over struct fields, so clients are still told the strict contract and the leniency is invisible repair.Fixes #5891
Type of change
Test plan
task test)task test-e2e)task lint-fix)Authorization bypass, via the existing
TestMiddlewareOptimizerMetaToolsharness and its Cedar policy permitting onlyallowed_backend:call_toolarguments{"tool_name": "forbidden_backend", "parameters": {}}{"parameters": {"tool_name": "forbidden_backend", ...}}{"parameters": {"tool_name": "allowed_backend", ...}}The new cases fail against
mainand pass with commit 1.Dispatch, same input decoded through
schema.Translate[CallToolInput], which is the path bothcall_toolhandlers use:Input
{"parameters": {"tool_name": "searxng_web_search", "query": "..."}}tool_nameparametersCallToolresult""{query, tool_name}tool_name is required"searxng_web_search"{query}Input
{"parameters": {"query": "...", "pageno": 1}}, no name anywhereCallToolerrortool_name is requiredtool_name is required: call_tool expects {"tool_name": "<name from find_tool>", "parameters": {<tool arguments>}}, got parameters keys [pageno query]End to end. The Tier-1 find→call round-trip in
test/e2e/vmcp_optimizer_test.goonly exercised the flat payload, so nothing verified the nested form against a real server. It now issues both calls against the same discovered backend tool and asserts each echoes its own input:That is the
optimizer-labelled subset, not the full e2e suite; CI covers the rest.Both handlers decode via
schema.Translate, which round-trips throughjson.Marshal/json.Unmarshal. There is a regression test asserting the hoist survives that path rather than only a directjson.Unmarshal, since a change toTranslatewould otherwise silently stop the unmarshaler from running.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Does this introduce a user-facing change?
Yes.
call_toolinvocations that nesttool_nameinsideparametersare now authorized against the tool they actually target, instead of skipping the authorization check.tool_nameis genuinely missing, the error describes the expected shape and lists the parameter keys received.Special notes for reviewers
ResolveCallToolTargetnever modifies the caller's map; it copies only when a nested name is hoisted.parsedRequest.Argumentsis shared with downstream consumers, so in-place mutation there would not be safe.The error message lists parameter keys only, never values, so tool arguments do not reach error strings or logs.
pkg/authzalready depended onpkg/vmcp/optimizertransitively throughoptimizerdec, so importing it directly adds no new dependency edge.Scope is deliberately limited to the nesting direction reported in #5891. The symmetric malformation, arguments flattened as top-level siblings of
tool_namerather than underparameters, is not handled. If it appears it belongs inResolveCallToolTargetat the same depth. The new error message echoes the received keys, which is the signal needed to decide whether it is worth handling.Happy to split the authorization fix into its own PR, or to route it through a security process instead, if you would prefer that.