Repository navigation
[Bug]: Internal error messages leak into responses #1232
Description
Activity
Hi @d-orm-sat , thanks for reporting this.
When an unexpected crash happens (like a database failure), the server’s error handler runs this line
jsonrpc_error = JSONRPCInternalError(message=str(error))in
As a workaround at application level you may intercept exceptions prior to JSON-RPC formatting by catching unhandled database exceptions.
Would you like to contribute for a fix upstream? I can assign the issue to you.Hi @d-orm-sat , thanks for reporting this. When an unexpected crash happens (like a database failure), the server’s error handler runs this line
jsonrpc_error = JSONRPCInternalError(message=str(error))inAs a workaround at application level you may intercept exceptions prior to JSON-RPC formatting by catching unhandled database exceptions.
Would you like to contribute for a fix upstream? I can assign the issue to you.Hi @rohityan thanks for the response. I'd be happy to contribute a fix - do you have any suggestions for what the preferred approach would be?
A few ideas I can think of:
- Mapping to a predefined, helpful message like I originally suggested seems a bit cumbersome and not very scalable
- Replace the
message=str(error)withmessage=type(error)- This would still somewhat leak application internals, but would be a lot tighter than the full unredacted message like before.
- Fully redact the error message and replace
message=str(error)withmessage="Internal Server Error"- This would probably be my preference - callers will know something went wrong server side, and servers can diagnose from their logs - it seems a little unnecessary to me to communicate server details to caller, but maybe this was a design choice that wants to be maintained?
Hi @d-orm-sat , replacing
message=str(error)with a sanitized"Internal Server Error"while logging the full exception traceback server-side is definitely the right direction to avoid leaking sensitive details. Also add an accompanying test inbuild_error_response
What happened?
When a server raises an exception, such as when supplying a context ID that is > 36 chars, the client receives a JSON RPC error object which includes the full internal error message that the server produced.
This is not good security practice to leak internal error messages like this, and could expose sensitive information.
A suggestion would be to redact any internal error messages, and perhaps create a mapping of common errors to client friendly error messages.
If the intention is for implementations to handle this themselves, with patching or intercepting, which is likely what we will be doing, it should be at least documented.
Relevant log output
Actual response received: {'error': {'code': -32603, 'message': '(sqlalchemy.dialects.postgresql.asyncpg.Error) <class \'asyncpg.exceptions.StringDataRightTruncationError\'>: value too long for type character varying(36)\n[SQL: INSERT INTO tasks (id, context_id, kind, owner, last_updated, status, artifacts, history, protocol_version, metadata) VALUES ($1::VARCHAR, $2::VARCHAR, $3::VARCHAR, $4::VARCHAR, $5::TIMESTAMP WITHOUT TIME ZONE, $6::JSON, $7::JSON, $8::JSON, $9::VARCHAR, $10::JSON)]\n[parameters: (\'2deb8a1b-a091-4388-a7c2-832e579f6ba6\', \'21198763-5774-4cfd-a1da-883d915260ee999999999\', \'task\', \'\', None, \'{"state": "TASK_STATE_SUBMITTED"}\', \'[]\', \'[{"messageId": "e0f25133-9dcf-4cb9-9c32-e1cd63e3eee0", "contextId": "21198763-5774-4cfd-a1da-883d915260ee999999999", "taskId": "2deb8a1b-a091-4388-a7c2-832e579f6ba6", "role": "ROLE_USER", "parts": [{"text": "How\\\'s the weather in London?"}]}]\', \'1.0\', \'null\')]\n(Background on this error at: https://sqlalche.me/e/20/dbapi)'}, 'id': '19783034-a4f6-4a3f-b772-b004eb4b5cc7', 'jsonrpc': '2.0'} Suggested response: {'error': {'code': -32603, 'message': 'Context ID too long, must be <=36 chars.'}, 'id': '19783034-a4f6-4a3f-b772-b004eb4b5cc7', 'jsonrpc': '2.0'} Or: {'error': {'code': -32603, 'message': 'Server Error'}, 'id': '19783034-a4f6-4a3f-b772-b004eb4b5cc7', 'jsonrpc': '2.0'}Code of Conduct