Harden the dispatch-context routing paths (#265) by gophergogo · Pull Request #265 · GopherSecurity/gopher-mcp

gophergogo · 2026-07-07T05:54:44Z

Summary

Fixes every confirmed finding from the review of #264 (the dispatch-context refactor), one commit per finding:

In-chain handlers no longer strip the context. ProtocolCallbackBridge — the handler installed by the config-driven json_rpc.dispatcher chain — and the four chainable message filters (metrics, circuit breaker, request validation, request logging) forwarded only the context-free hooks. On a config-driven listener that sent every request to the degraded legacy path where a listener-only server has no reply route: responses were never written. All now forward the context-carrying hooks, with shared work factored into helpers so the entries cannot drift. The circuit breaker additionally uses the context's reply path to answer blocked requests with the circuit-open error instead of a silent timeout, and its admission lock no longer covers downstream dispatch.
Stdio server push restored. Stdio sessions are connection-keyed now that the context supplies an origin for every transport, which routed them into the "no push channel" error. Connection-keyed sessions owned by a connection manager deliver through that manager; McpServerConfig gains an optional stdio transport override so the regression test drives a real server over real pipes.
ODR violation resolved. The stdio factory's private duplicate of DirectJsonRpcCallbacks (same mangled name, diverging vtable layout after the context hooks) is deleted in favor of the public definition; the private wrapper filter moves into an anonymous namespace.
Context-free dispatch bounded. The legacy path used to leak one unretrievable null-keyed session per message until max_sessions starved every transport; it now keeps exactly one shared session, and the per-message WARN fires once per server.
Reply sinks fail loud on closed origins. Both context sinks now require the origin connection to be open; a client that hangs up mid-dispatch gets its failed reply logged and counted instead of reported as sent.
Callback-proxy 202 leak fixed. The dispatch-time 202 for notifications no longer fires in callback-proxy mode, where the header-time 202 already answered the POST and the unguarded write was captured by onWrite and shipped down the client's SSE stream as event data.
Cleanups. Dead sendHttpResponse helper removed; LegacyDispatchContext derives from NullMessageDispatchContext so the no-origin contract exists once.

Testing

Full suite: 137/137 pass. New coverage: identity-preserving context forwarding for all four chainable filters and the config-driven bridge; circuit-open reply through the context; stdio subscribe→push round trip over real pipes (fails without the routing fix — verified); one shared session and no capacity starvation for context-free dispatches; sink error on a connection closed mid-dispatch; exactly one 202 on a callback POST with a clean SSE stream.

ProtocolCallbackBridge — the handler the config-driven constructor installs — and the four chainable message filters (metrics, circuit breaker, request validation, request logging) forwarded messages only through the context-free hooks, stripping the per-message origin mid-chain. On a config-driven listener that sent every request to the degraded context-free path, where a listener-only server has no reply route at all: the response was never written and each message leaked a fresh null-keyed session. Each handler now forwards the context-carrying hooks, with the shared work factored into helpers so the two entries cannot drift. The circuit breaker additionally uses the reply path the context provides to answer a blocked request with the circuit-open error instead of letting it time out silently — the old code built that error and had nowhere to send it. Its admission lock is now scoped so downstream dispatch no longer runs under the filter's mutex. Tests pin identity-preserving context forwarding for all four filters, the circuit-open reply, and the config-driven bridge path end to end.

Stdio sessions are keyed on the pipe connection now that the dispatch context supplies an origin for every transport, so sendNotificationToSession classified them as connection-keyed HTTP sessions with no push channel and returned an error — server-initiated notifications (resource updates) silently stopped reaching stdio clients. A connection-keyed session whose connection is owned by a connection manager now routes through that manager, which frames and writes on the long-lived pipe; the no-channel error remains for genuine plain-HTTP connections, and the keyless branch stays as the fallback for context-free legacy sessions. McpServerConfig gains an optional stdio transport override so embedders and tests can point the stdio transport at pipe fds. The new integration test drives a real server over real pipes end to end — subscribe on the stdin pipe, push a resource update, require the notification on the stdout pipe — and fails without the routing fix.

stdio_filter_chain_factory.cc defined its own namespace-scope class named DirectJsonRpcCallbacks while json_rpc_filter_factory.h publishes another class with the same mangled name — an ODR violation, and since the context hooks were added at different declaration positions the two definitions also diverged in vtable layout, which a linker merging the weak symbols could turn into misrouted virtual calls. The stdio factory now reuses the public definition, and its private wrapper filter moves into an anonymous namespace so the collision pattern cannot recur.

A message arriving through the context-free legacy hooks has no origin connection, so getOrCreateSessionFor keyed a fresh session on nullptr per message. Null-keyed sessions are unretrievable by any lookup, so each one leaked until max_sessions starved every transport on the server, and the legacy client's state never persisted across its own messages. The server now keeps exactly one shared session for context-free dispatches, re-created only if the expiry sweep removed it. The context-free warning also fires once per server instead of per message — a legacy producer hits that path on every message, and repeating the same warning is flooding, not signal; later hits log at debug. Tests drive the exact context-free entry an un-migrated producer would use: many messages share one session, and a server with a small max_sessions never exhausts capacity from legacy dispatches.

Both dispatch-context reply sinks guarded only against a null write_callbacks_, which is set once at chain init and never cleared — while a connection write in any non-open state is silently discarded by the connection. A client that hung up mid-dispatch therefore got its reply reported as sent while nothing went out, making the server's failed-send log and error counter unreachable for exactly the case they were added for. Both sinks now also require the origin connection to be open before writing. The regression test wires the filter to a real connection over a socketpair, closes the connection during dispatch, and requires the in-flight reply to surface an error while the pre-close reply succeeds.

A notification POSTed to /callback/{id} is answered with a 202 at header time, under the handshake-write guard. onNotification then wrote a second, unguarded 202 — and in callback-proxy mode the composite's own onWrite captures unguarded writes and ships them through the SSE registry, so the raw status line arrived on the client's SSE stream as event data ("data: HTTP/1.1 202 Accepted..."), corrupting the stream. The dispatch-time 202 now fires only outside callback-proxy mode, where plain HTTP genuinely needs it. The regression test opens a real SSE stream, POSTs a notification to its callback URL from a second connection on the same factory, and requires exactly one 202 on the POST connection and no HTTP status line on the SSE stream.

…#265) HttpSseFilterChainFactory::sendHttpResponse lost its last caller when responses moved to the dispatch context's reply path; its body was already a log-and-drop stub, so delete declaration and definition. LegacyDispatchContext now derives from NullMessageDispatchContext, inheriting the null-origin/empty-id contract instead of restating it, so the 'message with no origin' semantics are defined in exactly one place and only the reply-path fallback differs.

ProtocolCallbackBridge — the handler the config-driven constructor installs — and the four chainable message filters (metrics, circuit breaker, request validation, request logging) forwarded messages only through the context-free hooks, stripping the per-message origin mid-chain. On a config-driven listener that sent every request to the degraded context-free path, where a listener-only server has no reply route at all: the response was never written and each message leaked a fresh null-keyed session. Each handler now forwards the context-carrying hooks, with the shared work factored into helpers so the two entries cannot drift. The circuit breaker additionally uses the reply path the context provides to answer a blocked request with the circuit-open error instead of letting it time out silently — the old code built that error and had nowhere to send it. Its admission lock is now scoped so downstream dispatch no longer runs under the filter's mutex. Tests pin identity-preserving context forwarding for all four filters, the circuit-open reply, and the config-driven bridge path end to end.

Stdio sessions are keyed on the pipe connection now that the dispatch context supplies an origin for every transport, so sendNotificationToSession classified them as connection-keyed HTTP sessions with no push channel and returned an error — server-initiated notifications (resource updates) silently stopped reaching stdio clients. A connection-keyed session whose connection is owned by a connection manager now routes through that manager, which frames and writes on the long-lived pipe; the no-channel error remains for genuine plain-HTTP connections, and the keyless branch stays as the fallback for context-free legacy sessions. McpServerConfig gains an optional stdio transport override so embedders and tests can point the stdio transport at pipe fds. The new integration test drives a real server over real pipes end to end — subscribe on the stdin pipe, push a resource update, require the notification on the stdout pipe — and fails without the routing fix.

stdio_filter_chain_factory.cc defined its own namespace-scope class named DirectJsonRpcCallbacks while json_rpc_filter_factory.h publishes another class with the same mangled name — an ODR violation, and since the context hooks were added at different declaration positions the two definitions also diverged in vtable layout, which a linker merging the weak symbols could turn into misrouted virtual calls. The stdio factory now reuses the public definition, and its private wrapper filter moves into an anonymous namespace so the collision pattern cannot recur.

A message arriving through the context-free legacy hooks has no origin connection, so getOrCreateSessionFor keyed a fresh session on nullptr per message. Null-keyed sessions are unretrievable by any lookup, so each one leaked until max_sessions starved every transport on the server, and the legacy client's state never persisted across its own messages. The server now keeps exactly one shared session for context-free dispatches, re-created only if the expiry sweep removed it. The context-free warning also fires once per server instead of per message — a legacy producer hits that path on every message, and repeating the same warning is flooding, not signal; later hits log at debug. Tests drive the exact context-free entry an un-migrated producer would use: many messages share one session, and a server with a small max_sessions never exhausts capacity from legacy dispatches.

Both dispatch-context reply sinks guarded only against a null write_callbacks_, which is set once at chain init and never cleared — while a connection write in any non-open state is silently discarded by the connection. A client that hung up mid-dispatch therefore got its reply reported as sent while nothing went out, making the server's failed-send log and error counter unreachable for exactly the case they were added for. Both sinks now also require the origin connection to be open before writing. The regression test wires the filter to a real connection over a socketpair, closes the connection during dispatch, and requires the in-flight reply to surface an error while the pre-close reply succeeds.

A notification POSTed to /callback/{id} is answered with a 202 at header time, under the handshake-write guard. onNotification then wrote a second, unguarded 202 — and in callback-proxy mode the composite's own onWrite captures unguarded writes and ships them through the SSE registry, so the raw status line arrived on the client's SSE stream as event data ("data: HTTP/1.1 202 Accepted..."), corrupting the stream. The dispatch-time 202 now fires only outside callback-proxy mode, where plain HTTP genuinely needs it. The regression test opens a real SSE stream, POSTs a notification to its callback URL from a second connection on the same factory, and requires exactly one 202 on the POST connection and no HTTP status line on the SSE stream.

gophergogo added 7 commits July 6, 2026 21:32

caleb2h approved these changes Jul 7, 2026

View reviewed changes

gophergogo merged commit e0e6060 into main Jul 7, 2026
1 check passed

Navigation Menu

Uh oh!

Harden the dispatch-context routing paths (#265) - #265

Harden the dispatch-context routing paths (#265)#265
gophergogo merged 7 commits into
mainfrom
dispatch-context-hardening

gophergogo commented Jul 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

gophergogo commented Jul 7, 2026

Summary

Testing

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants