Repository navigation
mcp: reject null titled enum entries in elicitation schemas - #1447
jyotir-aditya wants to merge 1 commit into
Conversation
chrikrah
left a comment
chrikrah
left a comment
There was a problem hiding this comment.
With the guard removed and the two new cases kept, go test ./mcp -run TestElicitationSchemaValidation panics in client.go; with it, both pass. Both loops reach validateTitledEnumEntry, so from my perspective this can be merged.
ServerSession.Elicit runs the same validateElicitSchema after an accept (server.go:1842), so by reading the code a Go server sending a nil entry panics there too once a non-Go client accepts. Worth a sentence in the commit message, which names only the client?
validateTitledEnumEntry dereferenced entry.Const without checking entry for nil. A JSON null inside oneOf (single-select) or anyOf (multi-select) unmarshals to a nil *jsonschema.Schema, so the client panicked while validating an elicitation/create request. ServerSession.Elicit runs the same validateElicitSchema on the schema it sent once the client accepts, so a server passing a nil entry could panic there too. Return an invalid-params error instead, as for other invalid schemas, and add oneOf and anyOf regression cases to TestElicitationSchemaValidation.
7dddae9 to
3d4a82e
Compare
|
Thanks for checking. I confirmed the server path too. With a client that accepts without validating (a middleware stand-in for a non-Go client), |
validateTitledEnumEntrydereferencedentry.Constwithout checkingentryfor nil. A JSONnullinsideoneOf(single-select) oranyOf(multi-select) unmarshals to a nil*jsonschema.Schema, so a client with anElicitationHandlerpanicked while validating theelicitation/createrequest, before the handler ran.The function now returns an error for a nil entry. The client then answers
-32602like it does for other invalid elicitation schemas.The panic predates #1312 (the check was added in v1.4.0), but #1312 also routes
"type": "string"items withanyOfthrough the same check, so that shape now reaches it too.ServerSession.Elicitcalls the samevalidateElicitSchemaon the schema it sent once the client accepts, so a server that sends a nil entry would also panic there when paired with a client that doesn't validate the request. This only applies to sessions below protocol 2026-07-28; later versions refuse server-initiated elicitation. I confirmed this with a throwaway test (a client middleware that returns "accept"): without the guard it panics atserver.go:1842, and with itElicitreturns an error.Tests
Two cases added to
TestElicitationSchemaValidation(oneOf: [null]anditems.anyOf: [null]), both expectingCodeInvalidParams. Onmainthey panic with a nil pointer dereference; with this change they pass.gofmt -l .,go vet ./...andgo test ./...are clean.