Stop SSE filter from leaking tools/list on undecodable lines

pkg/authz/response_filter.go

@@ -131,17 +131,22 @@ func (rfw *ResponseFilteringWriter) Flush() {
 
 func (rfw *ResponseFilteringWriter) processJSONResponse(rawResponse []byte) error {
 	message, err := jsonrpc2.DecodeMessage(rawResponse)
-	if err != nil {
-		rfw.ResponseWriter.WriteHeader(rfw.statusCode)
-		_, err := rfw.ResponseWriter.Write(rawResponse)
-		return err
-	}
-
 	response, ok := message.(*jsonrpc2.Response)
-	if !ok {
+	if err != nil || !ok {
+		// Not a clean Response. The disguised-result bypass (#5257) is
+		// transport-independent, so apply the same check here as on the SSE
+		// path: if a non-Response, undecodable, or batch frame still carries a
+		// result, fail closed rather than passing the smuggled list through.
+		if carriesResult(rawResponse) {
+			slog.Warn("JSON response carried a result outside a clean Response frame; dropping as a protocol violation",
+				"method", rfw.method)
+			rfw.ResponseWriter.WriteHeader(rfw.statusCode)
+			_, werr := rfw.ResponseWriter.Write([]byte("{}"))
+			return werr
+		}
 		rfw.ResponseWriter.WriteHeader(rfw.statusCode)
-		_, err := rfw.ResponseWriter.Write(rawResponse)
-		return err
+		_, werr := rfw.ResponseWriter.Write(rawResponse)
+		return werr
 	}
 
 	filteredResponse, err := rfw.filterListResponse(response)
@@ -187,35 +192,49 @@ func (rfw *ResponseFilteringWriter) processSSEResponse(rawResponse []byte) error
 		var written bool
 		if data, ok := bytes.CutPrefix(line, []byte("data:")); ok {
 			message, err := jsonrpc2.DecodeMessage(data)
-			if err != nil {
-				rfw.ResponseWriter.WriteHeader(rfw.statusCode)
-				_, err := rfw.ResponseWriter.Write(rawResponse)
-				return err
-			}
-
-			response, ok := message.(*jsonrpc2.Response)
-			if !ok {
-				rfw.ResponseWriter.WriteHeader(rfw.statusCode)
-				_, err := rfw.ResponseWriter.Write(rawResponse)
-				return err
+			response, isResponse := message.(*jsonrpc2.Response)
+			if isResponse {
+				filteredResponse, err := rfw.filterListResponse(response)
+				if err != nil {
+					return rfw.writeErrorResponse(response.ID, err)
+				}
+
+				filteredData, err := jsonrpc2.EncodeMessage(filteredResponse)
+				if err != nil {
+					return rfw.writeErrorResponse(response.ID, err)
+				}
+
+				// The loop writes linesep after this line, so do not append a
+				// terminator here. A hardcoded "\n" desyncs \r\n streams.
+				if _, err := rfw.ResponseWriter.Write([]byte("data: " + string(filteredData))); err != nil {
+					return fmt.Errorf("%w: %w", errBug, err)
+				}
+
+				written = true
+			} else if carriesResult(data) {
+				// The frame is not a clean Response but still carries a result.
+				// This covers a non-Response type (a request/notification frame
+				// smuggling a result), a decode error (missing or invalid jsonrpc
+				// tag, a response frame with no or a non-scalar id), and a batch
+				// array. All are upstream-controlled shapes that would otherwise
+				// fall through and leak the very list this filter scrubs (#5257).
+				// Fail closed and drop the line.
+				slog.Warn("SSE data line carried a result outside a clean Response frame; dropping as a protocol violation",
+					"method", rfw.method)
+				written = true
+			} else if err != nil {
+				// Genuinely undecodable and no smuggled result. Pass this line
+				// through unfiltered (this line only).
+				slog.Warn("SSE data line could not be decoded as JSON-RPC; passing through unfiltered",
+					"method", rfw.method, "error", err)
+			} else {
+				// Genuine non-Response frame (e.g. an interleaved notifications/*
+				// message) with no result payload. Routine SSE traffic, so log at
+				// Debug to keep the suspicious branches above from being buried.
+				// Pass through this line only.
+				slog.Debug("SSE data line was not a JSON-RPC Response; passing through unfiltered",
+					"method", rfw.method)
 			}
-
-			filteredResponse, err := rfw.filterListResponse(response)
-			if err != nil {
-				return rfw.writeErrorResponse(response.ID, err)
-			}
-
-			filteredData, err := jsonrpc2.EncodeMessage(filteredResponse)
-			if err != nil {
-				return rfw.writeErrorResponse(response.ID, err)
-			}
-
-			_, err = rfw.ResponseWriter.Write([]byte("data: " + string(filteredData) + "\n"))
-			if err != nil {
-				return fmt.Errorf("%w: %w", errBug, err)
-			}
-
-			written = true
 		}
 
 		if !written {
@@ -254,6 +273,39 @@ func requiresResponseFiltering(method string) bool {
 		method == optimizerdec.FindToolName
 }
 
+// carriesResult reports whether a data payload contains a JSON-RPC "result"
+// field, either directly on an object or in any element of a batch array. A
+// frame that is not a clean Response (a request/notification smuggling a
+// result, a shape DecodeMessage rejects, or a batch array) but still carries a
+// result must not pass through: it would leak the list the filter exists to
+// scrub. See issue #5257.
+func carriesResult(data []byte) bool {
+	trimmed := bytes.TrimSpace(data)
+	if len(trimmed) == 0 {
+		return false
+	}
+	switch trimmed[0] {
+	case '{':
+		var probe struct {
+			Result json.RawMessage `json:"result"`
+		}
+		return json.Unmarshal(trimmed, &probe) == nil && probe.Result != nil
+	case '[':
+		var batch []struct {
+			Result json.RawMessage `json:"result"`
+		}
+		if json.Unmarshal(trimmed, &batch) != nil {
+			return false
+		}
+		for i := range batch {
+			if batch[i].Result != nil {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // filterListResponse filters the list response based on authorization policies
 func (rfw *ResponseFilteringWriter) filterListResponse(response *jsonrpc2.Response) (*jsonrpc2.Response, error) {
 	if response.Error != nil {