Skip to content

Audit records outcome=success for JSON-RPC error bodies over 512 bytes #6720

Description

@feiiiiii5

Bug description

The audit middleware's JSON-RPC error detection stops working once a response body is larger than its 512-byte detection buffer, and the event is recorded as outcome: success.

errorDetectionBufferSize is 512 (pkg/audit/auditor.go:110-114) and responseWriter.Write copies only the bytes that fit, truncating the rest (pkg/audit/auditor.go:145-153). detectApplicationError then hands that prefix to mcp.ParseMCPResponse (pkg/audit/auditor.go:456-469), which is deliberately lenient and returns HasError=false when json.Unmarshal fails (pkg/mcp/response.go:39-46). A truncated JSON object is not valid JSON, so outcome stays OutcomeSuccess (pkg/audit/auditor.go:300-308) and no jsonrpc_error_code / jsonrpc_error_message metadata is attached.

The config contract says a prefix is buffered "to detect JSON-RPC error fields, independent of the IncludeResponseData setting" (pkg/audit/config.go:45-49, and the same wording in docs/operator/crd-api.md and the vMCP CRD), with DetectApplicationErrors defaulting to true. For error bodies over 512 bytes the operator gets neither the error nor any hint that detection was skipped, which is the failure class #4678 was filed about.

The streamable-HTTP proxy writes a JSON-RPC response in a single Write (pkg/transport/proxy/streamable/utils.go:134), so the cut lands mid-string.

Steps to reproduce

Any tool call whose JSON-RPC error body exceeds 512 bytes. Mirroring the truncation and ParseMCPResponse semantics outside the repo:

message=  440 body=  501 prefix=501 detected=true
message=  450 body=  511 prefix=511 detected=true
message=  460 body=  521 prefix=512 detected=false
message= 2000 body= 2061 prefix=512 detected=false

With an envelope of {"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"…"}} (63 bytes of overhead) the cut is at a ~449-character message. Error bodies that large come from servers that put a stack trace or the offending input in error.data, and from the proxy's own wrapped upstream failures.

Expected behavior

detectApplicationErrors: true should classify any JSON-RPC error response as outcome: application_error, or the record should say why it could not.

Actual behavior

outcome: success, with no error code or message, for every JSON-RPC error body over 512 bytes.

Environment (if relevant)

  • ToolHive version: main @ dfb0713
  • OS/version: darwin (analysis only; the numbers above come from a standalone program that copies the two functions, not from task test)

Additional context

I looked at three shapes for a fix and did not want to pick one unilaterally:

  1. Grow the buffer. Simple, but the size that is "enough" is a guess, and a large error.data would still be missed.
  2. Detect the error field incrementally while streaming, instead of parsing a prefix (e.g. a json.Decoder token walk that stops at the top-level error key). Handles unbounded bodies without buffering them.
  3. Record outcome: unknown (or attach a detection_truncated: true marker) when the body was cut off, so the record is never silently positive.

I lean towards 2, with 3 as a cheap safety net, but 1 would also fix the common case. Happy to be assigned and put up a PR with tests either way — I did not find an existing test that feeds a response body over the buffer size through the middleware.

Activity

  1. breken-ai commented on Sep 28, 2026

    @breken-ai

    I'd like to work on this

  2. feiiiiii5 commented on Sep 28, 2026

    @feiiiiii5
    Author

    Thanks for picking this up — please go ahead, and I am standing back rather than opening a competing PR.

    I have no stake in which of the three shapes wins, so I will not argue for one. Two things from re-reading the report that may save you time, and one correction:

    Correction on my own numbers. I wrote that the int-parameter case gave 1.21e-08 in the pymc thread — that was a different repo, ignore it. Here the figures I can stand behind are the mirror program you ran, and they are enough: the cut lands at 512 and detection goes false, while 511 still works. That is the boundary.

    The outcome question is the part worth getting right. pkg/audit/auditor.go:300-308 has one more path than the three in the report: whatever detectApplicationError returns, an unparseable or truncated body is currently indistinguishable from a genuinely clean response. If you take shape 3, the marker has to be readable by whatever consumes audit records downstream, not just present in the record — otherwise the record still reads as success to the operator who is looking for the error.

    A cheap thing worth checking first: is errorDetectionBufferSize actually configurable at runtime, or only a package constant? pkg/audit/config.go:45-49 describes the contract but the report reads auditor.go:110-114 as a const. If an operator can already raise it, that is a documented workaround worth putting in the issue, and it bounds how urgent shape 1 is.

    The repo rules that will apply to your PR, in case they are not obvious from the template: commits need a DCO sign-off, the PR is capped at 1000 lines, and task is the only way to run build, test and lint — not go test, go build or golangci-lint directly. pkg/audit/auditor.go sits in a v1beta1 operator API surface, so a new field on the recorded event is a breaking change; a marker inside the existing metadata map is not.

    If you land shape 2 or 3, I would like to review the truncation boundary specifically — whether a body of exactly 512 bytes is still detected, and whether a cut landing inside the error key rather than inside the string is handled. Those are the two places a streaming detector usually diverges from the prefix one, and the v1beta1 constraint means getting it wrong is awkward to walk back.

  3. reyortiz3 commented on Oct 8, 2026

    @reyortiz3
    Collaborator

    Thanks @feiiiiii5 for the context and @breken-ai for volunteering to work on this! 🙏 I'm assigning this to you, we can discuss the actual approach on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions