From 759dd0ded421a50eaa93d6cd5a17d2a354ee61ff Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 17:48:04 +0200 Subject: [PATCH] Echo the request id in session-not-found 404s The transparent proxy's unknown-session guard returned a -32001 JSON-RPC error with a null id, while every sibling error path echoes the request id, so a client correlating responses by id got null for this one error class. Thread the parsed id through to NotFoundBody. Closes #5945 Co-Authored-By: Claude Opus 5 --- .../revision_guard_regression_test.go | 3 + .../proxy/transparent/transparent_proxy.go | 7 ++- pkg/transport/session/jsonrpc_errors.go | 11 ++-- pkg/transport/session/jsonrpc_errors_test.go | 55 ++++++++++++++++--- test/e2e/hostile_input_proxy_test.go | 19 ++----- 5 files changed, 66 insertions(+), 29 deletions(-) diff --git a/pkg/transport/proxy/transparent/revision_guard_regression_test.go b/pkg/transport/proxy/transparent/revision_guard_regression_test.go index 8557c5f6e4..02268dc1f1 100644 --- a/pkg/transport/proxy/transparent/revision_guard_regression_test.go +++ b/pkg/transport/proxy/transparent/revision_guard_regression_test.go @@ -104,6 +104,9 @@ func TestGuardUnknownSessionFiresDespiteForgedModernRevision(t *testing.T) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) assert.Contains(t, string(body), `"code":-32001`) + // The 404 echoes the request's JSON-RPC id so a client can correlate it, + // matching the classification-error path (#5945). + assert.Contains(t, string(body), `"id":1`) assert.False(t, backendCalled.Load(), "backend must not be contacted: the unknown-session guard must fire regardless of the classified revision") } diff --git a/pkg/transport/proxy/transparent/transparent_proxy.go b/pkg/transport/proxy/transparent/transparent_proxy.go index 35ccbfa316..f0122a24d8 100644 --- a/pkg/transport/proxy/transparent/transparent_proxy.go +++ b/pkg/transport/proxy/transparent/transparent_proxy.go @@ -598,12 +598,17 @@ func (t *tracingTransport) RoundTrip(req *http.Request) (*http.Response, error) isJSON := strings.Contains(req.Header.Get("Content-Type"), "application/json") sawInitialize := false revision := mcp.RevisionLegacy + // Echoed by the unknown-session guard below so its 404 correlates with the + // request, like every sibling error path (#5945). Stays nil for bodies that + // carry no JSON-RPC id, which NotFoundBody renders as "id":null. + var rpcID json.RawMessage if len(reqBody) > 0 && ((isMCP && isJSON) || t.p.transportType == types.TransportTypeStreamableHTTP.String()) { method, params, id, singleRequest, isInit := t.parseRPCRequest(reqBody) sawInitialize = isInit + rpcID = id if singleRequest { meta := mcp.ExtractMeta(params) rev, cerr := mcp.ClassifyRevision(method, meta, req.Header.Get("MCP-Protocol-Version")) @@ -628,7 +633,7 @@ func (t *tracingTransport) RoundTrip(req *http.Request) (*http.Response, error) slog.Error("session store lookup failed", "error", err) return plainResponse(req, http.StatusServiceUnavailable, "session store unavailable"), nil } - return session.NotFoundResponse(req), nil + return session.NotFoundResponse(req, rpcID), nil } } diff --git a/pkg/transport/session/jsonrpc_errors.go b/pkg/transport/session/jsonrpc_errors.go index 5083642f96..46aee708af 100644 --- a/pkg/transport/session/jsonrpc_errors.go +++ b/pkg/transport/session/jsonrpc_errors.go @@ -54,11 +54,14 @@ func WriteNotFound(w http.ResponseWriter, requestID any) { _, _ = w.Write(NotFoundBody(requestID)) } -// NotFoundResponse constructs an *http.Response with HTTP 404 and a -// JSON-RPC error body. Use this in httputil.ReverseProxy.ModifyResponse +// NotFoundResponse constructs an *http.Response with HTTP 404 and a JSON-RPC +// error body echoing requestID. Use this in httputil.ReverseProxy.ModifyResponse // (transparent proxy) where no http.ResponseWriter is available. -func NotFoundResponse(req *http.Request) *http.Response { - body := NotFoundBody(nil) +// +// Pass nil when the incoming request carries no JSON-RPC id: a GET or DELETE +// (no body), or a body that did not parse as a single JSON-RPC request. +func NotFoundResponse(req *http.Request, requestID any) *http.Response { + body := NotFoundBody(requestID) hdr := make(http.Header) hdr.Set("Content-Type", "application/json") return &http.Response{ diff --git a/pkg/transport/session/jsonrpc_errors_test.go b/pkg/transport/session/jsonrpc_errors_test.go index 9488bc80f4..01d00de466 100644 --- a/pkg/transport/session/jsonrpc_errors_test.go +++ b/pkg/transport/session/jsonrpc_errors_test.go @@ -84,15 +84,52 @@ func TestWriteNotFound(t *testing.T) { func TestNotFoundResponse(t *testing.T) { t.Parallel() - req := httptest.NewRequest(http.MethodPost, "/mcp", nil) - resp := NotFoundResponse(req) + tests := []struct { + name string + requestID any + wantID string + }{ + { + name: "no request id echoes null", + requestID: nil, + wantID: `"id":null`, + }, + { + name: "string request id is echoed", + requestID: "req-1", + wantID: `"id":"req-1"`, + }, + { + // The transparent proxy passes the id straight through as a + // json.RawMessage; a nil one must still render as null rather + // than producing an invalid body. + name: "raw json id is echoed verbatim", + requestID: json.RawMessage(`42`), + wantID: `"id":42`, + }, + { + name: "nil raw json id echoes null", + requestID: json.RawMessage(nil), + wantID: `"id":null`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() - assert.Equal(t, http.StatusNotFound, resp.StatusCode) - assert.Equal(t, "application/json", resp.Header.Get("Content-Type")) - assert.Equal(t, req, resp.Request) + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + resp := NotFoundResponse(req, tt.requestID) - body, err := io.ReadAll(resp.Body) - require.NoError(t, err) - assert.Contains(t, string(body), `"code":-32001`) - assert.Contains(t, string(body), `"id":null`) + assert.Equal(t, http.StatusNotFound, resp.StatusCode) + assert.Equal(t, "application/json", resp.Header.Get("Content-Type")) + assert.Equal(t, req, resp.Request) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Contains(t, string(body), `"code":-32001`) + assert.Contains(t, string(body), tt.wantID) + assert.Equal(t, int64(len(body)), resp.ContentLength) + }) + } } diff --git a/test/e2e/hostile_input_proxy_test.go b/test/e2e/hostile_input_proxy_test.go index d878d938d7..57625d50d5 100644 --- a/test/e2e/hostile_input_proxy_test.go +++ b/test/e2e/hostile_input_proxy_test.go @@ -101,11 +101,7 @@ var _ = Describe("Hostile Input Proxy", Label("proxy", "stateless", "hostile-inp resp, err := client.Send(context.Background(), proxyURL, req) Expect(err).ToNot(HaveOccurred(), tc.name) Expect(resp.StatusCode).To(Equal(tc.wantStatus), tc.name) - if tc.idNotEchoed { - Expect(resp.ID).To(BeNil(), "%s: echoed id", tc.name) - } else { - Expect(resp.ID).To(Equal(json.Number(fmt.Sprintf("%d", tc.id))), "%s: echoed id", tc.name) - } + Expect(resp.ID).To(Equal(json.Number(fmt.Sprintf("%d", tc.id))), "%s: echoed id", tc.name) Expect(resp.Error).ToNot(BeNil(), tc.name) Expect(resp.Error.Code).To(Equal(tc.wantCode), tc.name) if tc.wantData != nil { @@ -188,12 +184,6 @@ type hostileCase struct { wantStatus int wantCode int64 wantData map[string]any // nil means the error must carry no "data" field at all - // idNotEchoed marks a rejection path that -- unlike ClassificationErrorResponse - // -- doesn't echo the request id at all: session.NotFoundResponse hardcodes a - // nil id (see jsonrpc_errors.go's NotFoundBody(nil) call). A real minor - // asymmetry in the current code, left flagged rather than fixed (out of scope - // for a test-only step). - idNotEchoed bool } // hostileRejectionCases enumerates the classification-error and session-guard @@ -273,10 +263,9 @@ func hostileRejectionCases() []hostileCase { // in revision_guard_regression_test.go. req.WithSessionID("foreign-" + uuid.NewString()) }, - wantStatus: http.StatusNotFound, - wantCode: session.CodeSessionNotFound, - wantData: nil, - idNotEchoed: true, + wantStatus: http.StatusNotFound, + wantCode: session.CodeSessionNotFound, + wantData: nil, }, } }