Authorization and audit evaluate the pre-mutation MCP request

[jhrozek]

Authorization and audit evaluate the pre-mutation MCP request

Labels: bug

Bug description

When a mutating webhook and authorization are both configured, ToolHive authorizes the
pre-mutation MCP request while the backend executes the post-mutation body.

ParsingMiddleware (pkg/mcp/parser.go:87) parses the request body once and publishes the
resulting *ParsedMCPRequest in two places: into the request context under
MCPRequestContextKey, and into a ParsedRequestHolder so middleware that wraps the parser
(audit) can observe it. It then deliberately refuses to parse again (:91-94).

The mutating webhook middleware (pkg/webhook/mutating/middleware.go:133) replaces r.Body
with the patched body but calls next.ServeHTTP(w, r) with the request unchanged — no new
context, no holder refresh. Because the mutating webhook sits inside the parser and outside
authorization (pkg/runner/middleware.go:164/:177/:226, and
pkg/runner/config_builder.go:691/:707/:723), every downstream consumer of the cached parse
decides on bytes the backend will not execute:

Consumer	Reads	Effect
pkg/authz/middleware.go:196	.ResourceID, .Arguments	Cedar authorizes tool A with arguments A′; the backend runs tool B with arguments B′
pkg/audit/auditor.go:361,373	.Method, .ResourceID	The audit event's type and target.name describe a request that never ran
pkg/telemetry/middleware.go:174,227,300,405,417,687,723	span name, attributes, metric labels	Spans and metrics attribute the call to the pre-mutation tool; sanitizeArguments exports pre-redaction argument values
pkg/usagemetrics/middleware.go:39	.Method	Miscounted tool calls

The audit case is the one that ships in the default configuration: the event type and target name
are not gated on includeRequestData (default false, pkg/audit/config.go:90). Disabling
request-data capture does not make the audit trail accurate.

docs/middleware.md:31 documents the opposite behavior:

Mutating webhooks: Transform the parsed MCP request before later policy evaluation.

By contrast the validating webhook is correct — it re-reads r.Body itself
(pkg/webhook/validating/middleware.go:104), so it genuinely sees post-mutation bytes.

Steps to reproduce

Configure a mutating webhook that patches /mcp_request/params/name (or anything under
/mcp_request/params/arguments/) together with a Cedar authorization policy that refers to the
tool name or argument values. IsPatchScopedToMCPRequest
(pkg/webhook/mutating/patch.go:66) permits both paths, i.e. exactly the fields authorization
decides on.

Covered by regression tests in pkg/webhook/mutating/middleware_test.go
(TestMutatingMiddleware_ParsedRequestReflectsRenamedTool, ...ReflectsPatchedArgument,
...HolderReflectsMutatedResourceID), each of which fails without the fix.

Expected behavior

Policy evaluation, audit and telemetry see the request the backend will actually execute, as
docs/middleware.md:31 promises.

Actual behavior

They see the request as it arrived. The two controls disagree silently, with nothing logged.

Severity

High as a correctness-and-contract defect with security consequences — deliberately not
"authorization can be bypassed".

A mutating webhook is operator-supplied trusted infrastructure, already executing
operator-authorized code in the request path. "A webhook rewrites read_file to delete_file and
gets read_file authorized" is not a privilege escalation for whoever wrote the webhook.

The realistic failure is an operator's two controls disagreeing without warning:

• A benign webhook rewrites arguments for legitimate reasons (injecting a tenant ID, normalizing
  a path, redacting a field) and the operator writes Cedar policy against the normalized value.
  Policy evaluates the un-normalized value. This fails open or closed depending on the policy,
  with no record of the divergence.
• The audit trail names a request that was never executed, undermining the exact artifact an
  operator reaches for during an incident.

Preconditions: mutating webhooks and authorization both configured, plus a webhook that
touches method or params.

Not affected

• Batch smuggling is not possible. mcpRequestPathPrefix has a trailing slash
  (pkg/webhook/mutating/patch.go:19), so a wholesale /mcp_request replacement fails the prefix
  check, and RFC 6902 cannot otherwise turn a JSON object into an array. Any fix must keep this
  closed, which is why the re-parse routes through the same IsBatchRequest guard.
• Rate limiting is consistent by position (pkg/runner/middleware.go:169, outside the
  mutating webhook), though see Tool filtering and rate limiting are enforced against the pre-mutation request #6134 on per-tool quota semantics.
• vMCP never wires the mutating middleware, so pkg/vmcp/server/classification.go,
  call_gate.go and serve_handlers.go are unaffected today.

Additional context

Found by review, verified by reading each link in the chain against main @ 0a0cbd949.

A related but distinct gap is #6134: the tool-call filter enforces --tools against
the raw pre-mutation body from outside the parser, so republishing the parse does not fix it.