Emit conformant JSON-RPC on denial and error paths

[jhrozek]

Summary

Authorization denials, and three sibling error paths, emitted a body that is not valid JSON-RPC 2.0. All four built a *jsonrpc2.Response (golang.org/x/exp/jsonrpc2) and serialized it with encoding/json directly. That struct carries no json tags and no MarshalJSON, so reflection produced:

{"Result":null,"Error":{"code":403,"message":"unknown MCP method: ... "},"ID":{}}

Go-capitalized keys, no mandatory "jsonrpc":"2.0", and — the part the issue under-reported — the request id destroyed on every response. jsonrpc2.ID's only field is unexported, so encoding/json renders it {} unconditionally, even for a perfectly valid id. A client could not correlate a denial to the request that caused it. That is the more likely explanation for a Modern go-sdk client failing to surface these denials than the key casing alone.

The bug survived e2e for a subtle reason worth recording: only the outer envelope was reflected. The inner error object is a jsonrpc2.WireError, which does carry json tags, so assertions on error.code kept passing while the envelope around them was malformed.

What changed

• Encode via jsonrpc2.EncodeMessage at all four sites. It marshals through the library's wireCombined, which has the correct lowercase tags, stamps the version tag unconditionally, and tags id omitempty.
• Encode before writing any header. This removes a live double-write in handleUnauthorized (a 403 header followed, on encode failure, by http.Error(..., 500) — a second header plus garbage appended to the body), and fixes the webhook paths, which wrote the header first and swallowed the encode error, leaving a header with no body at all on failure.
• Restore the request id on the protocol-violation path in response_filter.go, which hardcoded an empty id, so fixing the encoder alone would have left that response uncorrelatable.
• Five test assertions pinned the broken shape and so held it in place; two were unchecked type assertions that would have panicked rather than failed. One was worse than useless: a require.NotNil on errResp["ID"] under a comment claiming the string id round-tripped. It never did — the value was {}, and NotNil passes on a non-nil empty map.
• The e2e harness could not detect this bug at all: its response struct had no jsonrpc field. It now has one, read case-sensitively, because encoding/json field matching falls back to case-insensitive and would have let "JSONRPC":"2.0" satisfy a json:"jsonrpc" tag — a hole in exactly the place the guard was added.

Fixes #5950

Type of change

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

Test plan

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

Unit tests: every touched package is green (pkg/authz/..., pkg/webhook/...), re-verified after rebasing onto main. Note task test could not be used locally — it panics on a gotestfmt v2.5.0 bug ("BUG: Empty package name encountered") that reproduces on a clean tree, so the underlying go test invocation was used instead. CI runs the real thing.

Each new test was proven to be a real regression guard by reverting the production change and confirming it fails, then restoring it. Worth calling out for one case specifically: the miscased-key assertion in mcp_raw_client_test.go fails before the case-sensitivity fix and passes after, whereas the absent-key case passes either way and is there to pin a different property.

E2E: compile-verified only. The dual-era suite needs a built binary and live proxy/backend containers. Because of that, the new RawResponse.JSONRPC field also got hermetic httptest coverage in TestRawClientSend, which runs in every shard — otherwise the detector added to catch this bug would itself have been unverified before merge.

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/authz/middleware.go	handleUnauthorized: EncodeMessage, encode before header, fix double-write and a clobbered err parameter
pkg/authz/response_filter.go	writeErrorResponse: EncodeMessage; new requestID() recovers the real id for the protocol-violation path
pkg/webhook/validating/middleware.go	sendErrorResponse: EncodeMessage, encode before header
pkg/webhook/mutating/middleware.go	same (10 call sites)
pkg/authz/middleware_test.go	New envelope test through the real parsing+authz chain
pkg/authz/response_filter_test.go	Envelope assertions on the existing disguised-frame test
pkg/webhook/*/middleware_test.go	Fix 5 assertions pinning the old shape; de-vacuum the id assertion; int/float64 → int64 fixtures; one envelope test per package
test/e2e/mcp_raw_client.go	Surface jsonrpc on RawResponse, read case-sensitively
test/e2e/mcp_raw_client_test.go	Hermetic coverage incl. a miscased-key guard
test/e2e/dual_era_functional_test.go	Replace a comment deferring conformance with the assertion it deferred

Does this introduce a user-facing change?

Yes. Denial and error responses from the authorization middleware, the response filter, and both webhook middlewares are now valid JSON-RPC 2.0: lowercase jsonrpc/id/error, the version tag present, and the request id echoed back so clients can correlate. Clients that previously tolerated the malformed body via case-insensitive unmarshal are unaffected; spec-strict clients can now parse these responses at all.

Special notes for reviewers

On absent ids: MCP deliberately overrides base JSON-RPC here, and this PR follows MCP. Base JSON-RPC 2.0 §5 says a Response id "MUST be Null" when it cannot be determined. MCP cannot express that: schema/2025-11-25/schema.ts types the error response as id?: RequestId where RequestId = string | number, and both transport pages say the body "has no id" — including for the 403 case specifically. So an absent id is omitted, not emitted as null.

This is not theoretical. The reference TypeScript SDK enforces it: JSONRPCErrorResponseSchema (packages/core/src/schemas.ts:168) is a .strict() object with id: RequestIdSchema.optional(), which admits undefined but not null — and JSONRPCMessageSchema.parse(), the throwing variant, gates every inbound message in the client transports. A response carrying "id":null makes that client throw inside its transport, before the application sees the error. Please read the two paragraphs above before filing this as a bug; one review pass already flagged it, applying base JSON-RPC rather than MCP.

Note id: 0 still round-trips correctly. wireCombined.ID is an interface{}, so omitempty tests IsNil() rather than zero-ness — a hand-rolled map plus omitempty would have silently dropped a zero id.

Deliberately out of scope, each tracked:

• Filter-failure errors on the SSE path are silently discarded #6037 — On the SSE path, writeErrorResponse writes a bare JSON object into a text/event-stream with no data: prefix. Per the WHATWG grammar that parses as an unrecognized field and is silently discarded, so the client hangs to its own timeout. This PR makes that payload conformant, which means it now reads as correct while remaining unreadable by any SSE client — please don't take the green bytes there as evidence the path works. Fixing it needs a design decision, not a mechanical change.
• Error responses emit "id":null, which the TS SDK client rejects #6038 — Resolved on main already, by Omit absent JSON-RPC ids instead of emitting null #6068 ("Omit absent JSON-RPC ids instead of emitting null"), so nothing outstanding here. It covered three hand-built envelopes that emitted "id":null (session/jsonrpc_errors.go, ratelimit/middleware.go, mcp/classification_response.go) — the same rule as above, opposite behavior. Listed only because this branch is where the rule was established and verified against the SDK.
• Clean up JSON-RPC error codes and leaked error text on denial paths #6039 — HTTP statuses used as JSON-RPC error codes (500/413/422). Left untouched on purpose: client-visible, asserted across pkg/vmcp/server/*_test.go and documented in docs/arch/10-virtual-mcp-architecture.md. 403 stays regardless — the draft spec blesses allocating outside the reserved range.

No shared helper, deliberately. The four sites keep their own encode/write blocks. A shared map-building helper was designed and rejected: the tree already has five hand-rolled envelope builders, a sixth absorbs none of them without an options struct, and an any-typed id parameter is precisely how a caller reintroduces the {} bug at the one site holding a typed jsonrpc2.ID. The single rule that prevents a fifth occurrence is greppable instead: never json.Marshal a jsonrpc2 type. A narrower helper taking a jsonrpc2.Message would avoid that objection and is worth a follow-up, but it was not folded in here.

A stale detail in the issue. #5950's repro uses server/discover, which no longer reproduces — #5953 made it always-allowed (middleware.go), after the issue was filed. Tests use tasks/list, which is genuinely absent from MCPMethodToFeatureOperation and which the map's own comment already names as denied by default.

[codecov]

Codecov Report

❌ Patch coverage is 51.42857% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.24%. Comparing base (ca4eda4) to head (0e7fc58).

Files with missing lines	Patch %	Lines
pkg/authz/response_filter.go	50.00%	4 Missing and 3 partials ⚠️
pkg/authz/middleware.go	42.85%	2 Missing and 2 partials ⚠️
pkg/webhook/mutating/middleware.go	57.14%	2 Missing and 1 partial ⚠️
pkg/webhook/validating/middleware.go	57.14%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6055      +/-   ##
==========================================
- Coverage   72.29%   72.24%   -0.06%     
==========================================
  Files         724      724              
  Lines       75330    75348      +18     
==========================================
- Hits        54461    54432      -29     
- Misses      16973    17034      +61     
+ Partials     3896     3882      -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.