Emit conformant JSON-RPC on denial and error paths

pkg/authz/middleware.go

@@ -145,8 +145,8 @@ func handleUnauthorized(w http.ResponseWriter, msgID interface{}, err error) {
 	}
 
 	// Create a JSON-RPC error response
-	id, err := mcp.ConvertToJSONRPC2ID(msgID)
-	if err != nil {
+	id, convErr := mcp.ConvertToJSONRPC2ID(msgID)
+	if convErr != nil {
 		id = jsonrpc2.ID{} // Use empty ID if conversion fails
 	}
 
@@ -155,15 +155,21 @@ func handleUnauthorized(w http.ResponseWriter, msgID interface{}, err error) {
 		Error: jsonrpc2.NewError(mcp.JSONRPCCodeDenied, errorMsg),
 	}
 
-	// Set the response headers
+	// Encode before writing any header, so a marshal failure never leaves a
+	// half-written response (e.g. a 403 header followed by a second 500 write).
+	body, encErr := jsonrpc2.EncodeMessage(errorResponse)
+	if encErr != nil {
+		// Unreachable in practice: errorResponse is always a well-formed
+		// jsonrpc2.Response built from strings/ints above. Fall back to a
+		// hardcoded valid JSON-RPC error body rather than writing nothing.
+		slog.Error("failed to encode JSON-RPC unauthorized response", "error", encErr)
+		body = fmt.Appendf(nil, `{"jsonrpc":"2.0","error":{"code":%d,"message":"Internal error"}}`, mcp.JSONRPCCodeDenied)
+	}
+
 	w.Header().Set("Content-Type", "application/json")
 	w.WriteHeader(http.StatusForbidden)
-
-	// Write the error response
-	if err := json.NewEncoder(w).Encode(errorResponse); err != nil {
-		// If we can't encode the error response, log it and return a simple error
-		http.Error(w, "Internal server error", http.StatusInternalServerError)
-	}
+	//nolint:gosec // G104: writing the JSON-RPC denial body to an HTTP client; nothing to do on error
+	_, _ = w.Write(body)
 }
 
 // Middleware creates an HTTP middleware that authorizes MCP requests.

pkg/authz/middleware_test.go

@@ -1245,6 +1245,87 @@ func TestMiddlewareOptimizerCallToolJSONRoundTrip(t *testing.T) {
 	assert.True(t, handlerCalled, "handler should be called for authorized call_tool")
 }
 
+// TestHandleUnauthorizedIsConformantJSONRPC drives the real middleware chain (parsing +
+// authorization) for a denied request and asserts the 403 body is a spec-conformant
+// JSON-RPC error response: lowercase "jsonrpc"/"id"/"error" keys and, for notifications,
+// no "id" key at all. assert.JSONEq is case-sensitive, so it alone would catch "ID"/"Error"
+// substituted for "id"/"error" and would fail on an unexpected "id" key — the id-presence
+// check is asserted separately anyway, as a belt-and-braces, self-documenting assertion.
+func TestHandleUnauthorizedIsConformantJSONRPC(t *testing.T) {
+	t.Parallel()
+
+	// The policy content is irrelevant: tasks/list is unknown to
+	// MCPMethodToFeatureOperation and denied before any policy is consulted. If a
+	// future PR adds authorization for tasks/* and this test starts failing, swap
+	// in any other method still absent from that map rather than reading it as an
+	// envelope regression.
+	authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{
+		Policies:     []string{`permit(principal, action == Action::"call_tool", resource == Tool::"weather");`},
+		EntitiesJSON: `[]`,
+	}, "")
+	require.NoError(t, err)
+
+	handler := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {
+		t.Fatal("next handler must not be called for a denied request")
+	})
+	middleware := mcpparser.ParsingMiddleware(Middleware(authorizer, handler, nil))
+
+	const wantErr = `"error":{"code":403,"message":"unknown MCP method: tasks/list (not configured for authorization)"}`
+
+	testCases := []struct {
+		name      string
+		reqIDJSON string // raw "id" field in the request; "" omits it (notification)
+		wantBody  string
+	}{
+		{
+			name:      "int64 id",
+			reqIDJSON: `"id":42,`,
+			wantBody:  `{"jsonrpc":"2.0","id":42,` + wantErr + `}`,
+		},
+		{
+			name:      "string id",
+			reqIDJSON: `"id":"abc",`,
+			wantBody:  `{"jsonrpc":"2.0","id":"abc",` + wantErr + `}`,
+		},
+		{
+			name:      "nil id (notification) omits id entirely",
+			reqIDJSON: "",
+			wantBody:  `{"jsonrpc":"2.0",` + wantErr + `}`,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			reqBody := `{"jsonrpc":"2.0",` + tc.reqIDJSON + `"method":"tasks/list"}`
+			req, err := http.NewRequest(http.MethodPost, "/messages", bytes.NewBufferString(reqBody))
+			require.NoError(t, err)
+			req.Header.Set("Content-Type", "application/json")
+
+			identity := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{
+				Subject: "test-user",
+				Claims:  jwt.MapClaims{"sub": "test-user"},
+			}}
+			req = req.WithContext(auth.WithIdentity(req.Context(), identity))
+
+			rr := httptest.NewRecorder()
+			middleware.ServeHTTP(rr, req)
+
+			require.Equal(t, http.StatusForbidden, rr.Code)
+			assert.Equal(t, "application/json", rr.Header().Get("Content-Type"))
+			assert.JSONEq(t, tc.wantBody, rr.Body.String())
+
+			var decoded map[string]any
+			require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &decoded))
+			_, hasID := decoded["id"]
+			assert.Equal(t, tc.reqIDJSON != "", hasID, "id key presence mismatch")
+			_, hasResult := decoded["result"]
+			assert.False(t, hasResult, "denied response must not contain a result key")
+		})
+	}
+}
+
 // TestConvertToJSONRPC2ID tests the convertToJSONRPC2ID function with various ID types
 func TestConvertToJSONRPC2ID(t *testing.T) {
 	t.Parallel()

pkg/authz/response_filter.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/stacklok/toolhive-core/mcpcompat/mcp"
 	"github.com/stacklok/toolhive/pkg/authz/authorizers"
+	mcpparser "github.com/stacklok/toolhive/pkg/mcp"
 	"github.com/stacklok/toolhive/pkg/vmcp/optimizer"
 	"github.com/stacklok/toolhive/pkg/vmcp/session/optimizerdec"
 )
@@ -140,7 +141,7 @@ func (rfw *ResponseFilteringWriter) processJSONResponse(rawResponse []byte) erro
 		if carriesResult(rawResponse) {
 			slog.Warn("JSON response carried a result outside a clean Response frame; dropping as a protocol violation",
 				"method", rfw.method)
-			return rfw.writeErrorResponse(jsonrpc2.ID{},
+			return rfw.writeErrorResponse(rfw.requestID(),
 				fmt.Errorf("dropped a frame carrying a result outside a clean Response"))
 		}
 		rfw.ResponseWriter.WriteHeader(rfw.statusCode)
@@ -532,19 +533,38 @@ func (rfw *ResponseFilteringWriter) writeErrorResponse(id jsonrpc2.ID, err error
 		Error: jsonrpc2.NewError(500, fmt.Sprintf("Error filtering response: %v", err)),
 	}
 
-	errorData, marshalErr := json.Marshal(errorResponse)
-	if marshalErr != nil {
-		// If we can't even marshal the error, write a simple error
-		rfw.ResponseWriter.WriteHeader(http.StatusInternalServerError)
-		_, writeErr := rfw.ResponseWriter.Write([]byte(`{"error": "Internal server error"}`))
-		return writeErr
+	// Encode before writing any header, so an encode failure never leaves a
+	// half-written response.
+	body, encErr := jsonrpc2.EncodeMessage(errorResponse)
+	if encErr != nil {
+		// Unreachable in practice: errorResponse is always a well-formed
+		// jsonrpc2.Response built from a valid ID and message above. Fall back
+		// to a hardcoded valid JSON-RPC error body rather than writing nothing.
+		slog.Error("failed to encode JSON-RPC error response", "error", encErr)
+		body = []byte(`{"jsonrpc":"2.0","error":{"code":500,"message":"Internal server error"}}`)
 	}
 
 	rfw.ResponseWriter.WriteHeader(http.StatusInternalServerError)
-	_, writeErr := rfw.ResponseWriter.Write(errorData)
+	_, writeErr := rfw.ResponseWriter.Write(body)
 	return writeErr
 }
 
+// requestID recovers the JSON-RPC id of the original request so an error
+// response written mid-filtering can still be correlated by the client.
+// Falls back to an empty jsonrpc2.ID (which EncodeMessage omits from the
+// wire entirely) if the request was never parsed or its id doesn't convert.
+func (rfw *ResponseFilteringWriter) requestID() jsonrpc2.ID {
+	parsed := mcpparser.GetParsedMCPRequest(rfw.request.Context())
+	if parsed == nil {
+		return jsonrpc2.ID{}
+	}
+	id, err := mcpparser.ConvertToJSONRPC2ID(parsed.ID)
+	if err != nil {
+		return jsonrpc2.ID{}
+	}
+	return id
+}
+
 // filterFindToolResponse filters the tools list embedded in a find_tool tools/call
 // response. The response is a CallToolResult whose first text content item contains
 // a JSON-encoded optimizer.FindToolOutput. Only tools the caller is authorized to

pkg/authz/response_filter_test.go

@@ -1138,6 +1138,11 @@ func TestResponseFilteringWriter_SSE_DisguisedResponseFrame(t *testing.T) {
 // TestResponseFilteringWriter_JSON_DisguisedResponseFrame is the application/json
 // counterpart: the disguised-result bypass is transport-independent, so the JSON
 // path must also fail closed on a smuggled result rather than write it through.
+//
+// It also pins the writeErrorResponse envelope: the error body written on this
+// path must be conformant JSON-RPC (lowercase "jsonrpc"/"id"/"error" keys, no
+// "id" key for a notification) and must carry the *original request's* id, not
+// an empty one, so the client can still correlate the denial.
 func TestResponseFilteringWriter_JSON_DisguisedResponseFrame(t *testing.T) {
 	t.Parallel()
 
@@ -1156,20 +1161,83 @@ func TestResponseFilteringWriter_JSON_DisguisedResponseFrame(t *testing.T) {
 
 	// A batch array smuggling a tools/list result. DecodeMessage rejects the
 	// array, so the pre-fix JSON path wrote it through raw.
-	smuggled := `[{"jsonrpc":"2.0","id":1,"result":{"tools":[{"name":"admin_tool"}]}}]`
+	const smuggled = `[{"jsonrpc":"2.0","id":1,"result":{"tools":[{"name":"admin_tool"}]}}]`
 
-	req, err := http.NewRequest(http.MethodPost, "/messages", nil)
-	require.NoError(t, err)
-	req = req.WithContext(auth.WithIdentity(req.Context(), identity))
+	// writeErrorResponse wraps the carriesResult error message with a fixed
+	// prefix and reports code 500 — an HTTP status, not a JSON-RPC error code.
+	// This test pins that current behavior; it does not assert the code is
+	// correct.
+	const wantErr = `"error":{"code":500,"message":"Error filtering response: ` +
+		`dropped a frame carrying a result outside a clean Response"}`
 
-	rr := httptest.NewRecorder()
-	rfw := NewResponseFilteringWriter(rr, authorizer, req, string(mcp.MethodToolsList), nil, nil)
-	rfw.ResponseWriter.Header().Set("Content-Type", "application/json")
+	testCases := []struct {
+		name      string
+		reqIDJSON string // raw "id" field on the *incoming* request; "" omits it (notification)
+		wantIDKey string // expected "id" fragment in the error envelope; "" if id must be absent
+	}{
+		{
+			name:      "int id is recovered onto the error envelope",
+			reqIDJSON: `"id":42,`,
+			wantIDKey: `"id":42,`,
+		},
+		{
+			name:      "string id is recovered onto the error envelope",
+			reqIDJSON: `"id":"abc",`,
+			wantIDKey: `"id":"abc",`,
+		},
+		{
+			name:      "notification (no id) omits id entirely",
+			reqIDJSON: "",
+			wantIDKey: "",
+		},
+	}
 
-	_, err = rfw.Write([]byte(smuggled))
-	require.NoError(t, err)
-	require.NoError(t, rfw.FlushAndFilter())
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			reqBody := `{"jsonrpc":"2.0",` + tc.reqIDJSON + `"method":"tools/list"}`
+			req, err := http.NewRequest(http.MethodPost, "/messages", strings.NewReader(reqBody))
+			require.NoError(t, err)
+			req.Header.Set("Content-Type", "application/json")
+			req = req.WithContext(auth.WithIdentity(req.Context(), identity))
+
+			// Run the real MCP parsing middleware so the request context carries a
+			// ParsedMCPRequest, matching how the request would look at this point
+			// in the real middleware chain. writeErrorResponse's id-recovery path
+			// reads the request id back out of that context.
+			var parsedReq *http.Request
+			mcpparser.ParsingMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
+				parsedReq = r
+			})).ServeHTTP(httptest.NewRecorder(), req)
+			require.NotNil(t, parsedReq)
+
+			rr := httptest.NewRecorder()
+			rfw := NewResponseFilteringWriter(rr, authorizer, parsedReq, string(mcp.MethodToolsList), nil, nil)
+			rfw.ResponseWriter.Header().Set("Content-Type", "application/json")
 
-	assert.NotContains(t, rr.Body.String(), "admin_tool",
-		"smuggled result on the application/json transport leaked past the filter")
+			_, err = rfw.Write([]byte(smuggled))
+			require.NoError(t, err)
+			require.NoError(t, rfw.FlushAndFilter())
+
+			assert.Equal(t, http.StatusInternalServerError, rr.Code)
+
+			body := rr.Body.String()
+			assert.NotContains(t, body, "admin_tool",
+				"smuggled result on the application/json transport leaked past the filter")
+
+			// assert.JSONEq is key-case-sensitive, so this single assertion catches
+			// "Error"/"ID" substituted for "error"/"id" (the historical bug), a
+			// missing "jsonrpc" tag, and a wrong or missing id all at once.
+			wantBody := `{"jsonrpc":"2.0",` + tc.wantIDKey + wantErr + `}`
+			assert.JSONEq(t, wantBody, body)
+
+			var decoded map[string]json.RawMessage
+			require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &decoded))
+			_, hasID := decoded["id"]
+			assert.Equal(t, tc.wantIDKey != "", hasID, "id key presence mismatch")
+			_, hasResult := decoded["result"]
+			assert.False(t, hasResult, "error response must not contain a result key")
+		})
+	}
 }

pkg/webhook/mutating/middleware.go

@@ -316,9 +316,6 @@ func readSourceIP(r *http.Request) string {
 }
 
 func sendErrorResponse(w http.ResponseWriter, statusCode int, message string, msgID interface{}) {
-	w.Header().Set("Content-Type", "application/json")
-	w.WriteHeader(statusCode)
-
 	id, err := mcp.ConvertToJSONRPC2ID(msgID)
 	if err != nil {
 		id = jsonrpc2.ID{} // Use empty ID if conversion fails.
@@ -329,5 +326,20 @@ func sendErrorResponse(w http.ResponseWriter, statusCode int, message string, ms
 		ID:    id,
 		Error: jsonrpc2.NewError(int64(statusCode), message),
 	}
-	_ = json.NewEncoder(w).Encode(errResp)
+
+	// Encode before writing any header, so an encode failure never leaves a
+	// half-written response (e.g. a status header followed by a second write).
+	body, encErr := jsonrpc2.EncodeMessage(errResp)
+	if encErr != nil {
+		// Unreachable in practice: errResp is always a well-formed jsonrpc2.Response
+		// built from strings/ints above. Fall back to a hardcoded valid JSON-RPC
+		// error body rather than writing nothing.
+		slog.Error("failed to encode JSON-RPC error response", "error", encErr)
+		body = fmt.Appendf(nil, `{"jsonrpc":"2.0","error":{"code":%d,"message":"Internal error"}}`, statusCode)
+	}
+
+	w.Header().Set("Content-Type", "application/json")
+	w.WriteHeader(statusCode)
+	//nolint:gosec // G104: writing the JSON-RPC denial body to an HTTP client; nothing to do on error
+	_, _ = w.Write(body)
 }