Fix: Render MCP-protocol error frame on outbound deny#501
Conversation
When an outbound gate (IBAC, etc.) rejects an MCP JSON-RPC request, the proxy used to emit a transport-level 4xx/5xx with a non-MCP body. The agent's MCP client sees that as a session break instead of a single failed tool call. This change adds an MCP-aware rejection helper for both proxy-sidecar (httpx.WriteRejectionForRequest) and envoy-sidecar (rejectFromActionForRequest) listeners. When pctx.Extensions.MCP carries a real JSON-RPC method + non-nil id, the deny renders as HTTP 200 with a JSON-RPC 2.0 error frame echoing the original id and carrying the violation's reason/code as error.message and error.data.error. JSON-RPC notifications (no id), non-MCP traffic, and nil pctx all fall through to today's HTTP-level shape unchanged. Wired in at the outbound request reject sites in forwardproxy (HTTP and CONNECT) and extproc (header- and body-phase outbound). Inbound and response-phase rejects keep the existing shape. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Kelly Abuelsaad <kaymar@gmail.com>
github-project-automation
Bot
moved this to New /:ToDo
in Rossoctl Issue Prioritization
📝 WalkthroughWalkthroughThis PR adds MCP JSON-RPC aware rejection handling across listener services. When an MCP JSON-RPC request is denied, rejections now render as HTTP 200 responses containing JSON-RPC 2.0 error objects, preserving the request id and mapping violation details into the error frame. Non-MCP and notification requests continue using HTTP-level rejection semantics (403 Forbidden). ChangesMCP request rejection rendering
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@authbridge/authlib/listener/httpx/render.go`:
- Around line 103-114: The current json.Marshal calls in writeMCPRejection and
mcpRejectionBody ignore errors and may send invalid bodies while returning
http.StatusOK application/json; update both functions to check the marshal
error, and on failure construct a minimal deterministic JSON-RPC error frame
(omit optional "data", set "id" to null if original id is invalid) and try
marshaling that; if marshaling still fails, fall back to a constant, precomputed
valid JSON payload (e.g.
{"jsonrpc":"2.0","id":null,"error":{"code":<server_error>,"message":"internal
error"}}) so the handler (writeMCPRejection) and rejectFromActionForRequest
callers always write a parseable JSON response. Ensure you reference
writeMCPRejection, mcpRejectionBody and rejectFromActionForRequest when making
the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9fab18f-ef8a-4f2f-aee6-90a4974b3cbf
📒 Files selected for processing (5)
authbridge/authlib/listener/extproc/mcpreject_test.goauthbridge/authlib/listener/extproc/server.goauthbridge/authlib/listener/forwardproxy/server.goauthbridge/authlib/listener/httpx/render.goauthbridge/authlib/listener/httpx/render_test.go
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 rossoctl#501. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Kelly Abuelsaad <kaymar@gmail.com>
kellyaa
added
ready-for-ai-review
kellyaa
moved this from New /:ToDo
to In review / Needs Testing
in Rossoctl Issue Prioritization
There was a problem hiding this comment.
🧹 Nitpick comments (1)
authbridge/authlib/listener/httpx/render.go (1)
100-156: ⚡ Quick winPast review comment properly addressed with robust 3-tier fallback.
The implementation correctly handles the scenarios flagged in the prior review:
- Full frame marshal with data (lines 125-135)
- Minimal frame fallback omitting optional data and using safe id (lines 136-154)
- Constant precomputed payload as final fallback (line 155)
The id marshalability check (lines 140-144) is correct: it tests whether
idcan be marshaled independently before including it in the minimal frame, falling back tonullper JSON-RPC 2.0 §5.1 when the original id is unmarshalable.Optional observability enhancement: Consider adding structured logging (slog.Warn) when fallback tiers are triggered, to aid production debugging if marshal failures occur. The current implementation prioritizes reliability (always returns valid JSON) over observability, which is a reasonable trade-off for error paths.
🔍 Optional: Add observability for fallback paths
}); err == nil { return body } + slog.Warn("httpx: MCP rejection full frame marshal failed, retrying minimal", "error", err) // 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 + } else { + slog.Warn("httpx: MCP rejection id unmarshalable, falling back to null", "error", err) } } 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 } + slog.Error("httpx: MCP rejection minimal frame marshal failed, using constant fallback", "error", err) return mcpRejectionFallback🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@authbridge/authlib/listener/httpx/render.go` around lines 100 - 156, Add structured warning logs when the MarshalMCPRejectionBody fallback tiers are exercised: inside MarshalMCPRejectionBody, log a slog.Warn (including the error value and context) when the initial full-frame json.Marshal fails, and again when the minimal-frame json.Marshal (or the id marshalability check) fails and you must return mcpRejectionFallback; include fields like "stage" ("full" or "minimal"), the jsonRPCServerError constant, and the id type/value where safe to do so to aid debugging without exposing sensitive payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@authbridge/authlib/listener/httpx/render.go`:
- Around line 100-156: Add structured warning logs when the
MarshalMCPRejectionBody fallback tiers are exercised: inside
MarshalMCPRejectionBody, log a slog.Warn (including the error value and context)
when the initial full-frame json.Marshal fails, and again when the minimal-frame
json.Marshal (or the id marshalability check) fails and you must return
mcpRejectionFallback; include fields like "stage" ("full" or "minimal"), the
jsonRPCServerError constant, and the id type/value where safe to do so to aid
debugging without exposing sensitive payloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e05e3c5e-f318-480a-91aa-2cde01a219c6
📒 Files selected for processing (3)
authbridge/authlib/listener/extproc/server.goauthbridge/authlib/listener/httpx/render.goauthbridge/authlib/listener/httpx/render_test.go
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Clawgenti Code Review
Solid fix for a real MCP session-stability problem: outbound IBAC denials now surface as scoped tool-call failures rather than transport breaks. The three-tier marshal fallback (full → minimal → static) is a nice defensive touch. One nit below.
Reviewed by clawgenti using github:pr-review
| "error": map[string]any{ | ||
| "code": jsonRPCServerError, | ||
| "message": message, | ||
| "data": data, |
There was a problem hiding this comment.
Nit: "data": data is included unconditionally, so when action.Violation is nil (or all Violation fields are empty and data stays nil), the wire response contains "data":null. JSON-RPC 2.0 §5.1 says the data member SHOULD be omitted when there are no additional details. Consider a conditional include:
errorObj := map[string]any{
"code": jsonRPCServerError,
"message": message,
}
if data != nil {
errorObj["data"] = data
}Not a blocker — the current shape is spec-valid — but it avoids a superfluous null field that some strict MCP clients might warn on.
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Clean, well-scoped fix: outbound MCP JSON-RPC rejections now render as an HTTP 200 JSON-RPC 2.0 error frame echoing the original id, so an IBAC/policy deny fails a single tools/call instead of breaking the MCP session. Detection is conservative (Method + non-nil RPCID), and notifications / non-MCP / nil-pctx all fall through to today's shape unchanged. Body rendering (MarshalMCPRejectionBody) is correctly shared across both the extproc and forwardproxy sidecars, with guaranteed-parseable marshal fallbacks. Tests are thorough — id-type round-trip, notification fall-back, non-MCP fall-back, nil-pctx safety, 503-still-200, and marshal-failure fallbacks.
Two minor, non-blocking observations inline (1 suggestion, 1 nit). All CI green, DCO passing.
Assisted-By: Claude Code
| // caller's MCP client surfaces this as one failed tool call rather than a | ||
| // transport break. All other shapes fall through to rejectFromAction. | ||
| func rejectFromActionForRequest(action pipeline.Action, pctx *pipeline.Context) *extprocv3.ProcessingResponse { | ||
| if pctx != nil && pctx.Extensions.MCP != nil && |
There was a problem hiding this comment.
suggestion — The MCP-detection predicate here (MCP != nil && Method != "" && RPCID != nil) is reimplemented inline, while httpx.shouldRenderMCPError encodes the identical condition. They're logically equal today but can drift independently. Consider exporting the httpx predicate (e.g. httpx.ShouldRenderMCPError(pctx)) and calling it from both listeners so the "is this an MCP request?" rule lives in one place.
| // 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"}}`) | ||
|
|
There was a problem hiding this comment.
nit — mcpRejectionFallback hardcodes -32000 in the raw string literal instead of referencing the jsonRPCServerError const just below. If the const ever changes, this last-resort frame silently diverges. A // keep in sync with jsonRPCServerError comment (or building it via fmt.Sprintf at init) would close the gap.
github-project-automation
Bot
moved this from In review / Needs Testing
to Done
in Rossoctl Issue Prioritization
…ering After merging PR rossoctl#501 (shared MCP rejection renderer, httpx/render.go), cpex denials on the HTTP proxies render as an MCP JSON-RPC 2.0 error frame at HTTP 200 for classified MCP requests (code -32000, error.data.error=cpex.<code>), and fall back to plain 403/502 only for non-MCP requests. The demo and docs still described the old transport-level 403. Truth them up: - Chat agent (agent.py): format_tool_response reads the cpex code from error.data.error (was error.data.violation, which the renderer never emits). Verified live in-container against a real deny frame. Fix a stale '-> 403' comment. - Scenarios 02/05/06/07/08/09: headers + notes now expect HTTP 200 + JSON-RPC error frame (error.data.error=cpex.*), not HTTP 403. Re-ran all 9: deny frames match. - README: taint S3 line now says denied via MCP JSON-RPC error frame. - CHAT-WALKTHROUGH.md: fix all four deny sections (code -32001 -> -32000, data.violation -> data.error with the real cpex codes). - Plugin docs (plugins/cpex/README.md, docs/cpex-plugin.md): the deny/error wire shape is now described as listener-dependent (MCP frame at 200 vs plain 403/502). Also correct the stale 'body modifications aren't re-serialized' limitation: re-serialization is implemented; the real limitation is that multi-part rewrites fail closed. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
4 participants
Summary
idinstead of a transport-level 4xx/5xx with a non-MCP body. The agent's MCP client surfaces this as one failed tool call rather than a session break.httpx.WriteRejectionForRequest(proxy-sidecar) andrejectFromActionForRequest(envoy-sidecar/extproc). Detection is conservative — only rewrites whenpctx.Extensions.MCPhas a non-emptyMethodAND a non-nilRPCID. Notifications (no id), non-MCP traffic, and nil pctx fall through to today's shape unchanged.Why
IBAC denies (
ibac.blocked,ibac.judge_unavailable,ibac.no_session,ibac.no_intent, etc.) currently surface to MCP clients as 403/503 transport errors. MCP clients treat that as the HTTP transport failing, which can break the whole MCP session — even though the only thing that should fail is the singletools/callrequest. By rendering a JSON-RPC error frame the way an MCP server would, the failure stays scoped to one tool call.The error body shape:
{ "jsonrpc": "2.0", "id": <original id>, "error": { "code": -32000, "message": "<violation reason>", "data": { "error": "ibac.blocked", "plugin": "ibac", "details": { ... } } } }-32000is the JSON-RPC 2.0 implementation-defined server-error code; operators readerror.data.error(the violation code) anderror.message(the human reason) for context.Test plan
go test ./...inauthlib/— all packages passgo build ./...for all threecmd/authbridge-{proxy,envoy,lite}binariesauthbridge/authlib/listener/httpx/render_test.go(7 cases: MCP id round-trip for numeric+string ids, notifications fall back, non-MCP falls back, nil pctx safe, 503 still renders as JSON-RPC, plainWriteRejectionunchanged)authbridge/authlib/listener/extproc/mcpreject_test.go(3 cases: MCP request renders JSON-RPC, non-MCP falls back, notification falls back)Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
Refactor
Tests