Use standard JSON-RPC error codes and scrub leaked error text

[jhrozek]

Summary

Two consistency problems in the same handful of denial/error functions, both found while fixing #5950 and deliberately left out of that PR because neither is the envelope bug.

1. HTTP statuses used as JSON-RPC error codes. Three paths passed the HTTP status straight through as error.code: pkg/authz/response_filter.go emitted a literal 500, and both webhook middlewares emitted whatever status they were called with — 413, 422, or 500. Those values are legal, since they sit outside the reserved -32768..-32000 range, but they are not the codes the draft MCP spec designates for general protocol failures ("MCP uses the standard JSON-RPC 2.0 error codes (-32700, -32600 to -32603) for general protocol failures"). A client that branches on error.code cannot recognize 500 as internal-error or 422 as a denial.

The status and the code are independent dimensions, so mcp.JSONRPCCodeForStatus is now the single place the mapping lives: 413 → -32600 (an oversized body is a malformed request), 500 → -32603, and 422 → JSONRPCCodeDenied. That last one matters: the mutating path relays a webhook's HTTP 422 always-deny response downstream, while the validating path relays the identical condition as 403 — so emitting 422 as the code made one denial look unlike every other denial, defeating the stated purpose of the shared constant.

Deriving the code inside sendErrorResponse leaves all 13 call sites untouched. An explicit second parameter was considered and rejected: each status here already corresponds to exactly one failure class with exactly one correct code, so passing a redundant constant at 13 sites is 13 more places to get it wrong.

403 is unchanged. It is the one case where status and code legitimately coincide, deliberately outside the reserved range so it never collides with an SDK-generated code. It stays asserted across pkg/vmcp/server/*_test.go and documented at docs/arch/10-virtual-mcp-architecture.md:631.

2. Denial paths leaked internal error text. handleUnauthorized used err.Error() as the client-visible message, so a Cedar evaluation error and a default-deny message disclosing "(not configured for authorization)" both reached the caller; writeErrorResponse sent "Error filtering response: %v", where the error can originate in policy evaluation and name tools. Meanwhile pkg/webhook/validating/middleware.go does the exact opposite two directories over, with the comment "Prevent information leaks by ignoring the webhook's message" — so the tree contradicted itself in adjacent files.

Standardized on scrub-and-log, following writeModernListError in pkg/vmcp/server/modern_dispatch.go. The client-visible string is unchanged from what a clean deny already produced, so an authorizer error and a policy deny are now byte-identical to the caller rather than distinguishable. Scoping this honestly, as the issue does: the recipient is already authenticated, just not authorized, and what leaked was policy-engine structure — not credentials and not another user's data. This is a hardening and consistency item, not a vulnerability.

Fixes #6039

Type of change

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

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

task test passed with exit code 0, plus scoped go test ./pkg/mcp/... ./pkg/authz/... ./pkg/webhook/... ./pkg/runner/... green on every touched package.

One caveat worth stating rather than hiding: task test's command is go test ... | gotestfmt -hide "all", with no pipefail set, so its exit code is a weaker signal than it appears and the run produces no per-package output at all. The scoped runs are the real per-package evidence.

Every new assertion was proven to be a real regression guard by reverting the production line it covers, confirming the test fails, then restoring. Two worth calling out:

• Reverting sendErrorResponse to int64(statusCode) makes the 422 test fail with expected: 403, actual: 422 — that assertion is what pins "a denial looks like every other denial".
• Reverting authorizeAndServe's err != nil || disjunct makes the new allowed=true cases fail with the next handler having been called and a 200 returned. Nothing in the suite covered that before (see reviewer notes below).

Linting not run locally; leaving it to CI.

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/mcp/errors.go	New JSONRPCCodeForStatus; records why JSONRPCCodeDenied is deliberately untyped
pkg/mcp/revision.go	CodeInternalError = -32603 added to the existing standard-code block; two comments resynced
pkg/webhook/validating/middleware.go	sendErrorResponse maps status → code; encode-failure fallback body fixed
pkg/webhook/mutating/middleware.go	Same (10 call sites, all untouched)
pkg/authz/response_filter.go	writeErrorResponse: 500 → CodeInternalError, message scrubbed + logged; processSSEResponse no longer interpolates the raw body
pkg/authz/middleware.go	handleUnauthorized always sends "Unauthorized" and logs the error; unknown-method deny logs at Debug from the call site
pkg/mcp/errors_test.go	New table-driven mapping test incl. the default case
pkg/webhook/*/middleware_test.go	413 assertions updated; new 422→403, 500→-32603, and a validating-side 500 path that had no coverage
pkg/authz/middleware_test.go	New authorizer-error scrub test; new allowed=true fail-closed case
pkg/authz/response_filter_test.go	Disguised-result test now pins the mapped code and the generic message

Does this introduce a user-facing change?

Yes, and it is client-visible on two axes.

Error codes. Denial and error bodies from the authorization middleware, the response filter, and both webhook middlewares now carry standard JSON-RPC codes instead of HTTP statuses: 500 → -32603, 413 → -32600, 422 → 403. Clients that branch on error.code can now recognize these as internal-error / invalid-request / denied. 403 is unchanged. No HTTP status changes, so anything keying off the status — including audit outcomes, which are derived from the status and not the code — behaves identically.

Error messages. Authorization denials now always read Unauthorized, and response-filter failures read internal error, instead of carrying the underlying error text. Operators lose nothing: the full error is logged server-side with more context than the client ever had. Anyone who was scraping the old message text for diagnostics will need to read the logs instead.

Implementation plan

Approved implementation plan

Implemented in three delegated steps (Sonnet worker, Opus reviewer per step, iterating until the reviewer had no blocking findings), landing as two commits.

Step 0 — branch setup. Branch off fix-nonconformant-jsonrpc-denials (#6055), which rewrites these exact four functions to encode via jsonrpc2.EncodeMessage. #6055 also de-shadowed the err parameter in handleUnauthorized, which is what makes the original error available to log in step 3.

Step 1 — the mapping. Reuse the existing standard-code family in pkg/mcp/revision.go (which already exports CodeInvalidRequest = -32600) rather than inventing a new home; add CodeInternalError alongside it and JSONRPCCodeForStatus next to JSONRPCCodeDenied. Acceptance: no constant duplicated, default fails safe to CodeInternalError, no new import beyond net/http.

Step 2 — apply at the call sites. Derive the code inside both sendErrorResponse functions and use CodeInternalError in writeErrorResponse; fix the encode-failure fallback bodies, which spliced the raw status into the code field and would otherwise reintroduce the bug they stand in for. Acceptance: no HTTP status changed anywhere, the 403 assertions pass unmodified, and the 422 assertion is a proven regression guard. → commit 1.

Step 3 — scrub the error text. handleUnauthorized logs and always sends "Unauthorized"; writeErrorResponse logs with the method and sends "internal error". Fixing the shared handler is the root-cause fix — it covers both callers at once. Acceptance: the message string stays exactly "Unauthorized" so pkg/runner/authz_audit_integration_test.go passes unmodified, fail-closed semantics unchanged, and no scrubbed error is silently dropped. → commit 2.

Deliberately out of scope, each tracked: #6037 (SSE data: prefix), #6038 (three hand-built envelopes still emitting "id":null).

Special notes for reviewers

Was stacked on #6055; now standalone. This was originally built on fix-nonconformant-jsonrpc-denials because parts of it — notably the encode-failure fallback bodies — only exist after that refactor. #6055 has since merged (9b0d83af0), so this is rebased onto main and the diff is just its own two commits.

Three things the review pass caught that are worth knowing about, because two of them were defects in this PR's own tests:

1. A test that did not test what it claimed. The first version of TestHandleUnauthorizedScrubsAuthorizerError used stubAuthorizer{allowed: false, err: sensitiveErr}. authorizeAndServe's condition is err != nil || !authorized, so allowed: false takes the branch via the !authorized disjunct — every assertion passed identically with err: nil, which made the NotContains vacuous. Now allowed: true, so the denial can only come from the error. This also closed a real coverage gap: nothing in the suite pinned that an authorizer returning (true, err) fails closed, so dropping the err != nil disjunct would have served an erroring authorizer with the whole suite still green.

2. A sibling leak the scrub initially missed. processSSEResponse interpolated the entire buffered upstream body — for a tools/list, the full pre-Cedar tool list — into an error that is logged upstream by FlushAndFilter's caller. Not client-visible, but it routed the exact payload this filter exists to scrub into an unbounded, caller-influenceable log line. Now reports the length only.

3. Log-level conflation. handleUnauthorized's slog.Warn served two very different events: the unknown-method default deny (expected traffic from any newer-spec client hitting tasks/list, sampling/createMessage, …) and an actual authorizer failure. Since parsedRequest.Method is unbounded caller-controlled input, that was one attacker-chosen WARN line per request. The routine deny now logs at Debug from the call site — matching how this package already logs per-item denials, and how the webhook middlewares split Info for denials from Error for operational failures — leaving Warn for genuine authorizer errors.

One scoping caveat on "standardize", so nobody reads more into it than is there. This aligns pkg/authz with the nearest precedent, not with a tree-wide rule. writeModernListError scrubs because its errors carry aggregation and routing plumbing that security.md forbids exposing — the same category as the policy-engine detail pkg/authz was leaking, which is why it is the right precedent here. Its sibling writeModernDispatchError (pkg/vmcp/server/modern_dispatch.go:618-623) deliberately does the opposite, relaying err.Error() verbatim, on the documented grounds that the SDK path already exposes the raw text for the identical failure. That exception survives this PR by design. So the split that remains is a reasoned one between error categories, not the adjacent-files contradiction this PR removes.

A note for whoever touches the logging next. handleUnauthorized's doc comment now warns that the logged error can embed decoded JWT claim values: preprocessClaims puts every claim into the Cedar evaluation context, and cedar-go's decimal()/ip()/datetime() constructors echo the offending value into Diagnostic.Errors. That directly contradicts the deliberate "claim keys only — never claim values, which hold user PII" rule in authorizers/cedar/core.go. Nothing leaks today, but this payload should not be promoted to a structured field that ships to an aggregator.

A same-defect sibling remains, tracked as a follow-up. processSSEResponse here and processEventStream in pkg/mcp/tool_filter.go carry the byte-identical fmt.Errorf("unsupported separator: %s", <whole buffer>) — splicing the full pre-filter payload (the unfiltered tool list, for a tools/list) into an error that is logged upstream. Neither is client-visible, so neither is a leak in the scrub's sense, but both route the exact payload the filter exists to scrub into an unbounded, caller-influenceable log line. This PR fixes the one in its own blast radius; the tool_filter.go sibling is fixed in a follow-up rather than retro-fitted here.

Two items deliberately banked, not fixed here:

• pkg/authz/middleware.go:330 — a passThroughTools entry with an empty or non-string tool_name is served with no authorization check at all, relying on the optimizer decorator to gate downstream. Pre-existing, deliberate, and pinned by an existing test, but it deserves its own issue confirming it against the decorator's actual behavior.
• convErr in handleUnauthorized and requestID()'s error in response_filter.go are still swallowed. Both are commented, so the go-style rule is satisfied, but the consequence is a client-visible symptom — an error response with no id, uncorrelatable — with zero server-side trace. Those lines come from Emit conformant JSON-RPC on denial and error paths #6055, so they belong there or in a follow-up.

Why no shared envelope helper, in case it comes up: #6055 already argued this and it holds here too. The tree has several hand-rolled envelope builders; a shared one absorbs none of them without an options struct, and an any-typed id parameter is exactly how a caller reintroduces the id bug #6055 fixed. The greppable rule is the cheaper guard.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 78.26087% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.25%. Comparing base (9b0d83a) to head (bf310dd).

Files with missing lines	Patch %	Lines
pkg/authz/response_filter.go	50.00%	2 Missing ⚠️
pkg/authz/middleware.go	80.00%	1 Missing ⚠️
pkg/webhook/mutating/middleware.go	66.66%	1 Missing ⚠️
pkg/webhook/validating/middleware.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6066      +/-   ##
==========================================
- Coverage   72.25%   72.25%   -0.01%     
==========================================
  Files         724      725       +1     
  Lines       75348    75358      +10     
==========================================
+ Hits        54445    54448       +3     
- Misses      17022    17030       +8     
+ Partials     3881     3880       -1     

☔ 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.

The base branch was changed.