-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix uncaught exception in MCP server #822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
So to clarify, currently this is blocking from using the |
Hmm, this shouldn't be blocking any clients, it is just adding error handling for a previously unhandled exception. Have you observed issues with this PR in certain clients? |
@bruno-oliveira This shouldn't be blocking anyone, but it does fix a huge security issue with remote servers. Currently anyone can lock up a server just by sending a bad payload #820 |
await self._handle_incoming(responder) | ||
if not responder._completed: # type: ignore[reportPrivateUsage] | ||
await self._handle_incoming(responder) | ||
except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is better to handle a specific exceptions before the general one, such as RuntimeError
(Which mentioned in this issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the risk is the server becoming unresponsive, I believe catching all exceptions to isolate errors to a single request is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a valid answer from the package's point of view.
It makes it considerably hard to maintain the package if everything is except Exception
.
I'm a bit baffled that a security issue like this is still open |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
session_message = SessionMessage( | ||
message=JSONRPCMessage(error_response)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How this got merged?
Motivation and Context
This fixes an uncaught exception in the MCP server
How Has This Been Tested?
Via unit tests and locally.
Breaking Changes
None.
Types of changes
Checklist
Additional context