stacklok / stacklok/toolhive

Tool filtering and rate limiting are enforced against the pre-mutation request

Open
#6,134 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Go
Stars
2.2k
Forks
300
Avg merge
1d 15h
Merged PRs (30d)
184

Description

Tool filtering and rate limiting are enforced against the pre-mutation request

Labels: bug

Bug description

A mutating webhook can rename a tool call into a tool that --tools filtering excluded from the
server, because the tool-call filter decides against the raw body before the mutation happens.

The tool filter and tool-call filter middleware read r.Body directly
(pkg/mcp/tool_filter.go:249) and are appended to the chain before the MCP parser
(pkg/runner/middleware.go:148 and :155, versus the parser at :164 and the mutating webhook at
:177; the CLI path is the same, pkg/runner/config_builder.go:688 versus :707). Because the
chain wraps in reverse append order, the filters are the outer wrappers and run first — so they see
the request as it arrived, and a mutating webhook rewrites the body afterwards.

Consequence: a webhook patching /mcp_request/params/name can turn an allowed tool call into a
call for a tool that --tools filtered out, and the filter has already passed the request.
IsPatchScopedToMCPRequest (pkg/webhook/mutating/patch.go:66) permits that path.

The filters' position is deliberatedocs/middleware.md:1063 states "Tool filters should
come before MCP Parser to operate on raw requests" — so this is a known gap in an intentional
design, not a regression.

Relationship to the parse-staleness fix

This is the same bug family as the mutating-webhook parse staleness (#6133), but a
different mechanism, and the fix for that one does not fix this one. That fix republishes the
cached *ParsedMCPRequest so consumers inside the parser (authorization, audit, telemetry,
usage metrics) see post-mutation data. The tool filters sit outside the parser and read the raw
body, so there is no cached parse to refresh — they have simply already run.

Rate limiting has the same shape

Rate limiting is registered at pkg/runner/middleware.go:169, outside the mutating webhook at
:177, and makes its decision on parsed.ResourceID (pkg/ratelimit/middleware.go:138). It is
therefore consistent by position — it is not reading a stale cached parse — but the semantics
still surprise: it debits the bucket for the tool the caller requested while the backend runs
the tool the webhook produced. A per-tool quota is bypassable by a rename.

Note that the CLI path (pkg/runner/config_builder.go:666-730) has no rate limiting at all.

Expected behavior

Undecided, and that is why this needs its own issue rather than being folded into the other fix.
The options trade off against the reason the filters are positioned early:

  1. Re-run tool filtering after mutation. Requires either moving the filters inside the parser
    (losing the "operate on raw requests" property they were placed early for) or running them
    twice.
  2. Reject any mutation that changes the tool name, i.e. narrow IsPatchScopedToMCPRequest so
    /mcp_request/params/name and /mcp_request/method are not patchable. This closes the filter
    gap, the rate-limit gap, and the stale-Mcp-Name limitation in one move, at the cost of
    forbidding a rewrite the webhook API currently advertises.
  3. Document the gap and treat the tool filter as an ingress-only control.

Option 2 is worth serious consideration: renaming is the mutation that breaks the most invariants,
and no known use case requires it.

Actual behavior

Filtering and per-tool rate limits are enforced against the requested tool, not the executed one.

Additional context

Verified against main @ 0a0cbd949. Preconditions: mutating webhooks configured together with
--tools filtering or rate limiting, plus a webhook that patches method or params/name.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the middleware ordering in pkg/runner/middleware.go and pkg/runner/config_builder.go, then read pkg/mcp/tool_filter.go, pkg/ratelimit/middleware.go, and IsPatchScopedToMCPRequest in pkg/webhook/mutating/patch.go. Decide which stated behavior should govern mutations of method or params/name, and make filtering and rate limiting consistent with that decision without overlooking the documented raw-request ordering.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
api, backend, security
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.