search

mbr / tinyrpc

A compact, modular transport and protocol agnostic RPC library. Does jsonrpc v2.
https://tinyrpc.readthedocs.org
MIT License
156 stars 53 forks source link

JSON-RPC error responses should include the request ID if it can be inferred from the request #83

Closed ntamas closed 5 years ago

ntamas commented 5 years ago

I have stumbled upon a subtle bug in the JSON-RPC implementation while I was working on another PR to implement MSGPACK-RPC support.

The problem is as follows. Whenever tinyrpc returns a JSON-RPC error response that inherits FixedErrorMessageMixin, the ID of the related request in the response is always null. The JSON-RPC spec says this about the id field:

This member is REQUIRED. It MUST be the same as the value of the id member in the Request Object. If there was an error in detecting the id in the Request object (e.g. Parse error/Invalid Request), it MUST be Null.

In my opinion, the id field shoud be null only if it cannot be detected from the incoming request. If the incoming request is invalid (e.g., it has a params field whose type is not a list), we can still figure out what the id is, and we should then include it in the error response. This can help client implementations to figure out which request was problematic if the client sent multiple requests at the same time.

The existing unit tests did not catch this bug because it only tested the invalid params field with a notification (not a request).

This PR fixes the problem by parsing the request ID when it is possible to do so and passing it to the error instance so it can add the request ID to the response. The PR also adds an extra unit test for this exact case.

ntamas commented 5 years ago

CI failure is unrelated; the typing module is not present on Python 3.4.

lnoor commented 5 years ago

Good catch! I believe it has been there from the very first version.