fix(jsonrpc2): decode requests when method key is present (#1000)
## Summary Fixes #976. `DecodeMessage` used `msg.Method != ""` to classify messages. A wire payload with `"method":""` was decoded as a **response**, so stdio servers dropped it without a correlated JSON-RPC error. ## Approach Decode into a `wireDecode` struct whose `method` field is `json.RawMessage`. Treat the message as a **request** when the `method` key is **present on the wire** (including `""`), which matches JSON-RPC semantics. This revision: - Uses a **single JSON parse** per message (dedicated decode struct, no secondary fields-map unmarshal) - Documents why `json.RawMessage` is used for `method` - Adds regression coverage so normal responses and id-only payloads without a `method` key stay on the response path ## Test plan - [x] `go test ./internal/jsonrpc2/... -v` - [x] `go test ./mcp/... -run TestIOConnRead_EmptyMethod -v` - [x] `go test ./... -count=1` - [x] `go test -race ./internal/jsonrpc2/... -count=1` - [x] Server conformance (`npx @modelcontextprotocol/conformance server --url http://127.0.0.1:3000/mcp`) — 40 passed, 0 failed - [x] Client conformance (`./scripts/client-conformance.sh`) — 217 passed, 0 failed ## Scope note This PR addresses the primary repro in #976 (empty `method` with valid `id`). Other cases listed in the issue (`id: null`, structurally invalid envelopes) are unchanged. Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
This commit is contained in:
parent
4dc99b48e1
commit
bcbfe0ce36
3 changed files with 89 additions and 5 deletions
|
|
@ -171,8 +171,20 @@ func EncodeIndent(msg Message, prefix, indent string) ([]byte, error) {
|
||||||
return bytes.TrimRight(buf.Bytes(), "\n"), nil
|
return bytes.TrimRight(buf.Bytes(), "\n"), nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// wireDecode is the decode form of [wireCombined]. Method is a [json.RawMessage]
|
||||||
|
// so we can tell whether the "method" key was present on the wire, including
|
||||||
|
// when its value is the empty string (see go-sdk#976).
|
||||||
|
type wireDecode struct {
|
||||||
|
VersionTag string `json:"jsonrpc"`
|
||||||
|
ID any `json:"id,omitempty"`
|
||||||
|
Method json.RawMessage `json:"method"`
|
||||||
|
Params json.RawMessage `json:"params,omitempty"`
|
||||||
|
Result json.RawMessage `json:"result,omitempty"`
|
||||||
|
Error *WireError `json:"error,omitempty"`
|
||||||
|
}
|
||||||
|
|
||||||
func DecodeMessage(data []byte) (Message, error) {
|
func DecodeMessage(data []byte) (Message, error) {
|
||||||
msg := wireCombined{}
|
msg := wireDecode{}
|
||||||
if err := internaljson.Unmarshal(data, &msg); err != nil {
|
if err := internaljson.Unmarshal(data, &msg); err != nil {
|
||||||
return nil, fmt.Errorf("unmarshaling jsonrpc message: %w", err)
|
return nil, fmt.Errorf("unmarshaling jsonrpc message: %w", err)
|
||||||
}
|
}
|
||||||
|
|
@ -183,15 +195,19 @@ func DecodeMessage(data []byte) (Message, error) {
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
if msg.Method != "" {
|
if len(msg.Method) > 0 {
|
||||||
// has a method, must be a call
|
// The "method" key was present. Decode its value (including "").
|
||||||
|
var method string
|
||||||
|
if err := internaljson.Unmarshal(msg.Method, &method); err != nil {
|
||||||
|
return nil, fmt.Errorf("unmarshaling jsonrpc message: %w", err)
|
||||||
|
}
|
||||||
return &Request{
|
return &Request{
|
||||||
Method: msg.Method,
|
Method: method,
|
||||||
ID: id,
|
ID: id,
|
||||||
Params: msg.Params,
|
Params: msg.Params,
|
||||||
}, nil
|
}, nil
|
||||||
}
|
}
|
||||||
// no method, should be a response
|
// no method key, should be a response
|
||||||
if !id.IsValid() {
|
if !id.IsValid() {
|
||||||
return nil, ErrInvalidRequest
|
return nil, ErrInvalidRequest
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -102,6 +102,51 @@ func newResponse(id any, result any, rerr error) jsonrpc2.Message {
|
||||||
return msg
|
return msg
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// go-sdk#976: empty method must decode as a request (encode omits empty method).
|
||||||
|
func TestDecodeEmptyMethodRequest(t *testing.T) {
|
||||||
|
encoded := []byte(`{"jsonrpc":"2.0","id":5,"method":"","params":{}}`)
|
||||||
|
msg, err := jsonrpc2.DecodeMessage(encoded)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
req, ok := msg.(*jsonrpc2.Request)
|
||||||
|
if !ok {
|
||||||
|
t.Fatalf("message type = %T, want *jsonrpc2.Request", msg)
|
||||||
|
}
|
||||||
|
if req.Method != "" {
|
||||||
|
t.Errorf("Method = %q, want empty string", req.Method)
|
||||||
|
}
|
||||||
|
if req.ID != jsonrpc2.Int64ID(5) {
|
||||||
|
t.Errorf("ID = %v, want 5", req.ID.Raw())
|
||||||
|
}
|
||||||
|
if !req.IsCall() {
|
||||||
|
t.Error("empty method with id=5 should be a call")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestDecodeResponseUnchanged(t *testing.T) {
|
||||||
|
encoded := []byte(`{"jsonrpc":"2.0","id":2,"result":{}}`)
|
||||||
|
msg, err := jsonrpc2.DecodeMessage(encoded)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if _, ok := msg.(*jsonrpc2.Response); !ok {
|
||||||
|
t.Fatalf("message type = %T, want *jsonrpc2.Response", msg)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Messages with an id but no "method" key are responses, not malformed requests.
|
||||||
|
func TestDecodeIDOnlyMessageIsResponse(t *testing.T) {
|
||||||
|
encoded := []byte(`{"jsonrpc":"2.0","id":5}`)
|
||||||
|
msg, err := jsonrpc2.DecodeMessage(encoded)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if _, ok := msg.(*jsonrpc2.Response); !ok {
|
||||||
|
t.Fatalf("message type = %T, want *jsonrpc2.Response", msg)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func checkJSON(t *testing.T, got, want []byte) {
|
func checkJSON(t *testing.T, got, want []byte) {
|
||||||
// compare the compact form, to allow for formatting differences
|
// compare the compact form, to allow for formatting differences
|
||||||
g := &bytes.Buffer{}
|
g := &bytes.Buffer{}
|
||||||
|
|
|
||||||
|
|
@ -124,3 +124,26 @@ func TestIOConnRead(t *testing.T) {
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// go-sdk#976: stdio must surface empty-method calls as requests, not responses.
|
||||||
|
func TestIOConnRead_EmptyMethod(t *testing.T) {
|
||||||
|
tr := newIOConn(rwc{
|
||||||
|
rc: io.NopCloser(strings.NewReader(`{"jsonrpc":"2.0","id":5,"method":"","params":{}}`)),
|
||||||
|
})
|
||||||
|
t.Cleanup(func() { tr.Close() })
|
||||||
|
|
||||||
|
msg, err := tr.Read(context.Background())
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("ioConn.Read() error = %v", err)
|
||||||
|
}
|
||||||
|
req, ok := msg.(*jsonrpc.Request)
|
||||||
|
if !ok {
|
||||||
|
t.Fatalf("message type = %T, want *jsonrpc.Request", msg)
|
||||||
|
}
|
||||||
|
if req.Method != "" {
|
||||||
|
t.Errorf("Method = %q, want empty string", req.Method)
|
||||||
|
}
|
||||||
|
if req.ID != jsonrpc2.Int64ID(5) {
|
||||||
|
t.Errorf("ID = %v, want 5", req.ID.Raw())
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue