fix(authbridge): Handle json.Marshal failures in MCP rejection rendering

The MCP JSON-RPC rejection helpers ignored marshal errors but always
returned HTTP 200 with Content-Type application/json. If a plugin
populated Violation.Details with an unmarshalable value (e.g. a
channel) or the request id failed to marshal, the MCP client would
receive an empty body on a 200 transport — a parse error rather than
a properly framed JSON-RPC error.

Both call sites now share httpx.MarshalMCPRejectionBody, which:
1. tries the full frame,
2. on marshal error, retries with a minimal frame (drops optional
   data; falls back to id=null per JSON-RPC 2.0 §5.1 if the id
   itself is unmarshalable),
3. on further error, returns a constant precomputed parseable
   payload.

Addresses CodeRabbit review feedback on PR #501.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Kelly Abuelsaad <kaymar@gmail.com>

Author: kellyaa

authbridge/authlib/listener/extproc/server.go

@@ -22,6 +22,7 @@ import (
 	"google.golang.org/grpc/status"
 
 	"github.com/kagenti/kagenti-extensions/authbridge/authlib/auth"
+	"github.com/kagenti/kagenti-extensions/authbridge/authlib/listener/httpx"
 	"github.com/kagenti/kagenti-extensions/authbridge/authlib/listener/internal/sseframe"
 	"github.com/kagenti/kagenti-extensions/authbridge/authlib/listener/skiphost"
 	"github.com/kagenti/kagenti-extensions/authbridge/authlib/pipeline"
@@ -859,7 +860,7 @@ func replaceTokenResponse(token string) *extprocv3.ProcessingResponse {
 func rejectFromActionForRequest(action pipeline.Action, pctx *pipeline.Context) *extprocv3.ProcessingResponse {
 	if pctx != nil && pctx.Extensions.MCP != nil &&
 		pctx.Extensions.MCP.Method != "" && pctx.Extensions.MCP.RPCID != nil {
-		body := mcpRejectionBody(action, pctx.Extensions.MCP.RPCID)
+		body := httpx.MarshalMCPRejectionBody(action, pctx.Extensions.MCP.RPCID)
 		return &extprocv3.ProcessingResponse{
 			Response: &extprocv3.ProcessingResponse_ImmediateResponse{
 				ImmediateResponse: &extprocv3.ImmediateResponse{
@@ -875,47 +876,6 @@ func rejectFromActionForRequest(action pipeline.Action, pctx *pipeline.Context)
 	return rejectFromAction(action)
 }
 
-// JSON-RPC 2.0 server-error code; see authlib/listener/httpx/render.go for
-// rationale on -32000 specifically.
-const jsonRPCServerError = -32000
-
-func mcpRejectionBody(action pipeline.Action, id any) []byte {
-	v := action.Violation
-	message := "request rejected"
-	var data map[string]any
-	if v != nil {
-		if v.Reason != "" {
-			message = v.Reason
-		}
-		data = map[string]any{}
-		if v.Code != "" {
-			data["error"] = v.Code
-		}
-		if v.PluginName != "" {
-			data["plugin"] = v.PluginName
-		}
-		if v.Description != "" {
-			data["description"] = v.Description
-		}
-		if len(v.Details) > 0 {
-			data["details"] = v.Details
-		}
-		if len(data) == 0 {
-			data = nil
-		}
-	}
-	body, _ := json.Marshal(map[string]any{
-		"jsonrpc": "2.0",
-		"id":      id,
-		"error": map[string]any{
-			"code":    jsonRPCServerError,
-			"message": message,
-			"data":    data,
-		},
-	})
-	return body
-}
-
 // rejectFromAction turns a pipeline Reject into an Envoy ImmediateResponse,
 // preserving the plugin's status/headers/body. Replaces the old
 // denyResponse helper which hardcoded {"error":...,"message":...} at each

authbridge/authlib/listener/httpx/render.go

@@ -76,6 +76,28 @@ func shouldRenderMCPError(pctx *pipeline.Context) bool {
 const jsonRPCServerError = -32000
 
 func writeMCPRejection(w http.ResponseWriter, action pipeline.Action, id any) {
+	body := MarshalMCPRejectionBody(action, id)
+	w.Header().Set("Content-Type", "application/json")
+	w.WriteHeader(http.StatusOK)
+	_, _ = w.Write(body)
+}
+
+// mcpRejectionFallback is a precomputed JSON-RPC 2.0 error frame used as
+// the last-resort body when both the full and minimal marshal attempts
+// fail. It is guaranteed to round-trip through json.Unmarshal so MCP
+// clients always see a parseable frame on a 200 application/json
+// response.
+var mcpRejectionFallback = []byte(`{"jsonrpc":"2.0","id":null,"error":{"code":-32000,"message":"request rejected"}}`)
+
+// MarshalMCPRejectionBody renders a Reject Action as a JSON-RPC 2.0
+// error frame body. It is guaranteed to return a non-empty, parseable
+// JSON byte slice so callers can safely send it on a 200
+// application/json response: marshal the full frame first; on error
+// (e.g. a plugin populated Violation.Details with an unmarshalable
+// value, or id is unmarshalable), retry with a minimal frame that omits
+// optional data and falls back to id=null; on further error, return a
+// constant precomputed frame.
+func MarshalMCPRejectionBody(action pipeline.Action, id any) []byte {
 	v := action.Violation
 	message := "request rejected"
 	var data map[string]any
@@ -100,16 +122,35 @@ func writeMCPRejection(w http.ResponseWriter, action pipeline.Action, id any) {
 			data = nil
 		}
 	}
-	body, _ := json.Marshal(map[string]any{
+	if body, err := json.Marshal(map[string]any{
 		"jsonrpc": "2.0",
 		"id":      id,
 		"error": map[string]any{
 			"code":    jsonRPCServerError,
 			"message": message,
 			"data":    data,
 		},
-	})
-	w.Header().Set("Content-Type", "application/json")
-	w.WriteHeader(http.StatusOK)
-	_, _ = w.Write(body)
+	}); err == nil {
+		return body
+	}
+	// Full frame failed to marshal — retry without optional data, and
+	// keep the original id only if it survives marshaling on its own
+	// (otherwise drop to null per JSON-RPC 2.0 §5.1).
+	safeID := any(nil)
+	if id != nil {
+		if _, err := json.Marshal(id); err == nil {
+			safeID = id
+		}
+	}
+	if body, err := json.Marshal(map[string]any{
+		"jsonrpc": "2.0",
+		"id":      safeID,
+		"error": map[string]any{
+			"code":    jsonRPCServerError,
+			"message": message,
+		},
+	}); err == nil {
+		return body
+	}
+	return mcpRejectionFallback
 }

authbridge/authlib/listener/httpx/render_test.go

@@ -184,3 +184,64 @@ func TestWriteRejection_Unchanged(t *testing.T) {
 		t.Errorf("error = %v, want ibac.blocked", body["error"])
 	}
 }
+
+// TestMarshalMCPRejectionBody_BadDetailsFallsBackToMinimalFrame: a
+// plugin can populate Violation.Details with anything (it's
+// map[string]any). If the value is unmarshalable (e.g. a channel),
+// the full frame fails. The body MUST still be a parseable JSON-RPC
+// 2.0 error frame so the MCP client surfaces this as a tool-call
+// failure rather than a parse error on a 200 application/json
+// response. The fallback drops optional data; everything else
+// (jsonrpc version, id, error.code, error.message) survives.
+func TestMarshalMCPRejectionBody_BadDetailsFallsBackToMinimalFrame(t *testing.T) {
+	action := pipeline.DenyWithDetails("ibac.blocked", "intent does not align", map[string]any{
+		"unmarshalable": make(chan int), // json.Marshal returns UnsupportedTypeError
+	})
+	body := MarshalMCPRejectionBody(action, "call-1")
+
+	var parsed map[string]any
+	if err := json.Unmarshal(body, &parsed); err != nil {
+		t.Fatalf("body must round-trip even when details fail to marshal: %v\n%s", err, body)
+	}
+	if parsed["jsonrpc"] != "2.0" {
+		t.Errorf("jsonrpc = %v, want 2.0", parsed["jsonrpc"])
+	}
+	if parsed["id"] != "call-1" {
+		t.Errorf("id = %v, want call-1 (id alone marshals fine, must be preserved)", parsed["id"])
+	}
+	errObj, ok := parsed["error"].(map[string]any)
+	if !ok {
+		t.Fatalf("error field missing or wrong type: %v", parsed["error"])
+	}
+	if errObj["code"] != float64(-32000) {
+		t.Errorf("error.code = %v, want -32000", errObj["code"])
+	}
+	if errObj["message"] != "intent does not align" {
+		t.Errorf("error.message = %v, want IBAC reason", errObj["message"])
+	}
+	if _, hasData := errObj["data"]; hasData {
+		t.Errorf("error.data must be omitted on fallback; got %v", errObj["data"])
+	}
+}
+
+// TestMarshalMCPRejectionBody_BadIDFallsBackToNullID: if the request
+// id itself can't be marshaled (defensive — mcp-parser only stores
+// string/number/null today, but RPCID is `any`), we fall back to
+// id=null per JSON-RPC 2.0 §5.1, which permits null when the
+// original id can't be detected. The body must still be a valid
+// JSON-RPC error frame.
+func TestMarshalMCPRejectionBody_BadIDFallsBackToNullID(t *testing.T) {
+	body := MarshalMCPRejectionBody(mcpAction(), make(chan int))
+
+	var parsed map[string]any
+	if err := json.Unmarshal(body, &parsed); err != nil {
+		t.Fatalf("body must round-trip even when id fails to marshal: %v\n%s", err, body)
+	}
+	if parsed["id"] != nil {
+		t.Errorf("id = %v, want null (unmarshalable id must drop to null)", parsed["id"])
+	}
+	errObj, _ := parsed["error"].(map[string]any)
+	if errObj["message"] != "intent does not align" {
+		t.Errorf("error.message = %v, want IBAC reason (preserved across fallback)", errObj["message"])
+	}
+}