Skip to content
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

Validate JSON-RPC response id field more strictly #3095

Closed
Tracked by #2854
fselmo opened this issue Sep 12, 2023 · 0 comments · Fixed by #3359
Closed
Tracked by #2854

Validate JSON-RPC response id field more strictly #3095

fselmo opened this issue Sep 12, 2023 · 0 comments · Fixed by #3359
Labels
Breaking Change v7 breaking changes considered for v7

Comments

@fselmo
Copy link
Collaborator

fselmo commented Sep 12, 2023

What was wrong?

The new WebsocketProviderV2 depends on the JSON-RPC 2.0 specification to be correct for providers. This means it depends on a request id matching with a response id. Because of the asynchronous nature of a websocket connection, and sometimes many-to-one response-to-request relationship (e.g. eth_subscribe), this is the only way to know how to format a response based on its request and to know which response from the socket to return for a particular request.

We don't currently validate that a response has an id very strictly. Starting with v7 of web3.py, we should check if an id is present and if it isn't we should make sure that the response matches that of a subscription. If it doesn't, it should be an invalid response.

How can it be fixed?

Validate whether an RPCResponse contains an id and if it doesn't, it is required to match all fields of an eth_subscribe subscription. There should be no reason for our library to make any assumptions based on malformed responses.

@fselmo fselmo added Breaking Change v7 breaking changes considered for v7 labels Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change v7 breaking changes considered for v7
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant