mcp: handle bad trailing stdio input graciously (#179)#192
mcp: handle bad trailing stdio input graciously (#179)#192jba merged 5 commits intomodelcontextprotocol:mainfrom
Conversation
mcp/transport.go
Outdated
| if err := in.Decode(&raw); err != nil { | ||
| return nil, err | ||
| } | ||
| // Read remaining data in the buffer. |
There was a problem hiding this comment.
I'd think this would be simpler. Doesn't it suffice to check that the next byte is a newline?
There was a problem hiding this comment.
As per my debugging, raw is definitely holding only valid json bytes after Decode call. Anything beyond that, needs to be read from in.Buffered. We can check only for newline in the next byte as you are suggesting and return err. I just think, providing actual garbage trail may just be informative
There was a problem hiding this comment.
I see your point, but let's keep it simple for now.
mcp/transport.go
Outdated
|
|
||
| // Read the next byte to check if there is trailing data. | ||
| tr := make([]byte, 1) | ||
| _, err := in.Buffered().Read(tr) |
There was a problem hiding this comment.
And you should check the first return value. From the io.Reader doc:
Callers should always process the n > 0 bytes returned before considering the error err. Doing so correctly handles I/O errors that happen after reading some bytes and also both of the allowed EOF behaviors.
There was a problem hiding this comment.
Also, it's possible that the reader is not at EOF, but there is no more data (yet) on the input stream. For example, the other end writes {...} with no newline and no more data. Then I think this would hang waiting for data, or maybe it would return 0; I'm not sure. Write a test case.
mcp/transport_test.go
Outdated
| t.Errorf("ioConn.Read() error = %v, wantErr %v", err, tt.wantErr) | ||
| return | ||
| } | ||
| if !reflect.DeepEqual(err.Error(), tt.want) { |
There was a problem hiding this comment.
You can just use ==. These are strings.
mcp/transport_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func Test_ioConn_Read_BadTrailingData(t *testing.T) { |
mcp/transport_test.go
Outdated
| } | ||
| tests := []struct { | ||
| name string | ||
| fields fields |
There was a problem hiding this comment.
Only the input would vary across tests, so this can be input string
mcp/transport_test.go
Outdated
| tests := []struct { | ||
| name string | ||
| fields fields | ||
| args args |
mcp/transport_test.go
Outdated
| fields fields | ||
| args args | ||
| want string | ||
| wantErr bool |
There was a problem hiding this comment.
an empty want can show that you don't want an error, so remove wantErr
|
@jba could you please take a look at the changes again |
…extprotocol#179) - graciously handled json lazy loading validation
|
@jba Could you please take a look at this |
2 participants
Issue
You can make MCP server log error, send response and then abruptly exit, with a json input malformed at the end
Why this change
json.RawMessage, once a valid first json is decoded, trailing bad input is silently ignored.Status