Skip to content

Decode the call_tool target as dispatch does - #6175

Merged
aponcedeleonch merged 1 commit into
mainfrom
fix-authz-call-tool-key-case
Aug 3, 2026
Merged

Decode the call_tool target as dispatch does#6175
aponcedeleonch merged 1 commit into
mainfrom
fix-authz-call-tool-key-case

Conversation

@aponcedeleonch

@aponcedeleonch aponcedeleonch commented Aug 3, 2026

Copy link
Copy Markdown
Member

Summary

  • Authorize and accept call_tool with nested tool_name #6150 routed authorization and dispatch through one helper so they could not disagree about which tool a call_tool request names. It closed the nested-tool_name case, but the top-level name is still read two different ways: authorization indexes the arguments map on "tool_name", while dispatch decodes the map into a CallToolInput. encoding/json prefers an exact field match but falls back to a case-insensitive one, so the two do not match the same set of keys.
  • {"Tool_Name": "forbidden_backend", "parameters": {}} therefore resolves and runs under dispatch, while the map index sees nothing, toolName stays empty, and the pass-through branch serves the request to the backend with no policy check performed. A case-variant "parameters" key drops the inner arguments the same way, so a policy with a when clause on context.arg_* denies a call it should permit.
  • Resolve the target through the same schema.Translate[optimizer.CallToolInput] call both dispatch sites use (serve_optimizer.go, optimizerdec/decorator.go). The two sides now match keys identically because it is the same call, not two implementations that agree today.
  • Arguments that fail to decode are denied instead of passed through. Dispatch decodes the same map with the same call and rejects it too, so nothing legitimate is lost, and the middleware never waves through a request whose target it could not establish.
  • Leave decoding as the only way in. CallToolArgToolName / CallToolArgParameters and their drift test are gone, and ResolveCallToolTarget is unexported to resolveCallToolTarget now that authorization, its only caller outside the package, no longer needs it. Both existed to serve the map-index pattern this PR removes, and keeping them exported invites it back. The one remaining raw-key read is the nested lookup inside that function, now a named constant guarded against the struct tag by TestCallToolArgToolNameMatchesStructTag.

This is pre-existing, not a regression from #6150, and not reachable today: server.New rejects Config.Authz together with Config.OptimizerConfig, and the Serve path never applies AuthzMiddleware, so the pass-through branch is dead in-tree. It becomes live with the optimizer-admission work deferred in pkg/vmcp/core/admission.go, which is why it is worth closing before then rather than after.

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

Three cases added to TestMiddlewareOptimizerMetaTools, which runs against a real Cedar authorizer. All three fail on main and pass here:

call_tool arguments Expected On main Here
{"Tool_Name": "forbidden_backend", "parameters": {}} 403 200, backend reached 403
{"tool_name": "args_backend", "PARAMETERS": {"query": "x"}} 200 403, arguments dropped 200
{"tool_name": "allowed_backend", "parameters": "not an object"} 403 200, backend reached 403

The second case needs a policy that reads the inner arguments, so the suite gains permit(... resource == Tool::"args_backend") when { context.arg_query == "x" }. Asserting on status alone would not catch dropped arguments.

TestCallToolInput_TranslateResolvesTarget records what the shared decoder resolves for each shape, including that a case-variant nested key is not hoisted. That one is a map index on both sides, so both name no tool and dispatch fails closed; the property under test is parity, not leniency.

Full pkg/authz/..., pkg/vmcp/optimizer/..., pkg/vmcp/session/optimizerdec/..., pkg/vmcp/server/... and pkg/vmcp/schema/... suites pass with -race.

Not covered by e2e: reaching this branch requires Cedar authz plus the optimizer, which server.New refuses to construct.

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

File Change
pkg/authz/middleware.go Resolve the target via schema.Translate; deny undecodable arguments
pkg/vmcp/optimizer/optimizer.go Unexport resolveCallToolTarget; name the nested lookup key
pkg/vmcp/session/optimizerdec/decorator.go Remove the now-unused raw argument-key constants

Does this introduce a user-facing change?

No. The affected branch cannot be reached through thv vmcp serve today, since Cedar authz and the optimizer are mutually exclusive at construction.

Special notes for reviewers

The deny-on-undecodable choice is the one real judgement call. The alternative was to fall back to the old map lookup so that no shape loses an authorization it gets today; the shape in question is {"tool_name": "x", "parameters": "oops"}, which authorizes on main and would now be refused. Denying seemed right for an authz middleware that cannot establish a target, and it costs nothing because dispatch rejects that payload regardless. Happy to switch to the fallback if you would rather keep the branch strictly additive.

handleUnauthorized is called with a nil error because the specific decode failure is already logged with the tool name just above; passing it again would emit a second, less useful line.

Three exported symbols go away: the two CallToolArg* constants and ResolveCallToolTarget. All three were added by #6150 so pkg/authz could resolve the target from a raw map, which is exactly what this PR stops doing, and none has another caller in tree. optimizerdec is dead on the Serve path besides. Technically a Go API break for an out-of-tree importer, and a clean revert if you would rather keep the surface stable.

Verified the advertised tool schema is untouched: it is built by reflection over CallToolInput's struct tags, not from those constants, and OptimizerTools() output is byte-identical to main for both find_tool and call_tool.

Generated with Claude Code

@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Aug 3, 2026
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.62%. Comparing base (798f37c) to head (fba6787).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6175      +/-   ##
==========================================
+ Coverage   72.59%   72.62%   +0.03%     
==========================================
  Files         736      736              
  Lines       76381    76384       +3     
==========================================
+ Hits        55447    55477      +30     
+ Misses      17008    16967      -41     
- Partials     3926     3940      +14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Authorization read the target name by indexing the request arguments map, while
dispatch decodes the same arguments into a CallToolInput. encoding/json matches
struct fields case-insensitively, so a request carrying "Tool_Name" resolved and
ran under dispatch but was invisible to the map index, and the pass-through
branch waved it to the backend with no policy check. A case-variant "parameters"
key dropped the inner arguments from the policy context the same way.

Resolve the target through the same schema.Translate call both dispatch sites
use, so the two cannot match keys differently. Arguments that fail to decode are
denied rather than passed through: dispatch decodes them the same way and
rejects them too, so no legitimate invocation is lost.

Leave decoding as the only way in. Drop the raw argument-key constants and
unexport resolveCallToolTarget, which authorization was the sole outside caller
of. Both existed to serve the map-index pattern this change removes, and keeping
them exported invites it back.
@aponcedeleonch
aponcedeleonch force-pushed the fix-authz-call-tool-key-case branch from 01a006d to fba6787 Compare August 3, 2026 09:48
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Aug 3, 2026
@aponcedeleonch
aponcedeleonch merged commit d5e8996 into main Aug 3, 2026
49 checks passed
@aponcedeleonch
aponcedeleonch deleted the fix-authz-call-tool-key-case branch August 3, 2026 10:41
@github-actions github-actions Bot mentioned this pull request Aug 5, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants