fix: don't respond to unparseable messages#940
Conversation
| @@ -143,16 +140,12 @@ where | |||
| Ok(Some(msg)) => return Some(msg), | |||
| Ok(None) => continue, | |||
| Err(JsonRpcMessageCodecError::Serde(e)) => { | |||
There was a problem hiding this comment.
I think we should narrow the ignore path because serde_json::Error is not limited to invalid JSON syntax. It can also represent valid JSON that fails to deserialize into the role-specific RxJsonRpcMessage<Role> shape, which should remain a protocol error rather than disappearing.
Classify the serde error in the receive loop: syntax/EOF errors are unparsable input with no correlatable id (issue modelcontextprotocol#938) and stay silent, while data errors (valid JSON that doesn't match the message shape) are real protocol errors and get an error response instead of being dropped. Add a test covering the protocol-error path.
|
Good catch, thanks. You're right that One open question: for the data-error case I kept the existing |
DaleSeo
left a comment
There was a problem hiding this comment.
One open question: for the data-error case I kept the existing
-32700 Parse errorresponse for a minimal change, but-32600 Invalid Requestis arguably the more accurate code for well-formed JSON with the wrong shape. Happy to switch to that if you'd prefer.
@tsouth89 That sounds like the right fix. Could you make that change before we merge this? Thanks!
|
Done. Switched the data-error case to |
Fixes #938.
When rmcp receives invalid JSON on the stdio / async-read-write transport, it replies with
{"jsonrpc":"2.0","id":null,"error":{"code":-32700,"message":"Parse error"}}. As reported in the issue, rmcp is the only official MCP SDK that responds to unparseable input, and responding can cause an error storm: if the peer treats the response itself as invalid data and answers with another error, the two sides bounce parse errors back and forth. The reporter hit this against a stdio server that accidentally wrote a log line to stdout.This makes the transport ignore unparseable messages instead of responding. It still logs at
debugand continues reading the next message, which matches the behavior of the other official SDKs. The now-unusedErrorDataimport is removed, and the existing test is updated to assert that no response is sent (and that the following valid message is still delivered).Verified locally:
cargo test -p rmcp --features server,transport-async-rw(the updatedreceive_ignores_parse_errortest passes)cargo clippy -p rmcp --all-features --all-targets -- -D warningscargo +nightly fmt --check