8000 jsonrpc: add a batch size limit for RPC requests · Issue #2867 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

jsonrpc: add a batch size limit for RPC requests #2867

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.

Sign up for GitHub

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

Jump to bottom
Closed
cason opened this issue Apr 22, 2024 · 3 comments · Fixed by #2939
Closed

jsonrpc: add a batch size limit for RPC requests #2867

cason opened this issue Apr 22, 2024 · 3 comments · Fixed by #2939
Assignees
@andynog
Labels
config rpc
Milestone
2024-Q2

Comments

@cason
Copy link
Contributor
cason commented Apr 22, 2024 •
edited
Loading

Our homemade RPC implementation, jsonrpc, allows users to invoke multiple RCP methods using a single request, using batching. While this is not a problem, it may constitute a form of attack vector.

By investigating the code, batching is implemented by this method:

cometbft/rpc/jsonrpc/server/http_json_handler.go

Line 41 in 6da2cf3

// first try to unmarshal the incoming request as an array of RPC requests

Not sure if this is the only method implementing batched requests, but this one was identified by running a test client.

We should consider adding a configuration parameter to limit the size/length of batched RPC requests submitted to Comet. To preserve backwards compatibility, the default value for this parameter can be 0, meaning no limit.
@cason cason added this to CometBFT Apr 22, 2024
@github-project-automation github-project-automation bot moved this to Todo in CometBFT Apr 22, 2024
@cason
Copy link
Contributor Author
cason commented Apr 22, 2024

This solution was adopted in a fork, using a hard-coded batch size limit:

https://github.com/sei-protocol/sei-tendermint/blob/5e8e9a2c60a3989e48b600cebe524c475d34bed4/rpc/jsonrpc/server/http_json_handler.go#L46-L50

@ValarDragon
Copy link
Contributor

I'm very supportive of doing this w/ a config option!

@adizere adizere added this to the 2024-Q2 milestone Apr 22, 2024
@andynog andynog self-assigned this Apr 23, 2024
@andynog
Copy link
Contributor
andynog commented Apr 24, 2024

I'll look into this and try to push a PR in the next couple of days

@andynog andynog moved this from Todo to In Progress in CometBFT Apr 25, 2024
andynog added a commit that referenced this issue Apr 25, 2024
@andynog
adding config parameter logic (#2867)
3edf740
andynog added a commit that referenced this issue Apr 25, 2024
@andynog
adding rpc batch size parameter to config template (#2867)
0dad2ea
andynog added a commit that referenced this issue Apr 25, 2024
@andynog
added info to the config manual (#2867)
9a206b3
andynog added a commit that referenced this issue Apr 26, 2024
@andynog
implemented Middleware to check batch size (#2867)
be4108f
andynog added a commit that referenced this issue Apr 29, 2024
@andynog
adding changelog entry (#2867)
77d8eb6
@andynog andynog mentioned this issue Apr 29, 2024
Merged
4 tasks
andynog added a commit that referenced this issue Apr 30, 2024
@andynog
set the default to 0 for backwards compatibility (#2867)
989eb73
andynog added a commit that referenced this issue Apr 30, 2024
@andynog
check if maxBatchSize gt 0 from the beginning (#2867)
0d03eb3
andynog added a commit that referenced this issue Apr 30, 2024
@andynog
refactoring middleware logic (#2867)
40da0d3
andynog added a commit that referenced this issue Apr 30, 2024
@andynog
adding test for max batch size (#2867)
bde430f
andynog added a commit that referenced this issue May 1, 2024
@andynog
improve test checking error message (#2867)
236382b
andynog added a commit that referenced this issue May 1, 2024
@andynog
improve test check error message (#2867)
0d65507
andynog added a commit that referenced this issue May 1, 2024
@andynog
reverting default value for max req batch size to 10 (#2867)
d4d82b1
@andynog andynog moved this from In Progress to Ready for Review in CometBFT May 1, 2024
github-merge-queue bot pushed a commit that referenced this issue May 2, 2024
@andynog
feat: add a configurable maximum batch size for RPC requests (#2939)
aa1caba 
close: #2867 

Adds middleware for the JSONRPC server to enforce a configurable maximum
batch size for RPC requests.

---

#### PR checklist

- [ ] Tests written/updated
- [X] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [X] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in CometBFT May 2, 2024
mergify bot pushed a commit that referenced this issue May 2, 2024
@andynog @mergify
feat: add a configurable maximum batch size for RPC requests (#2939)
62ddf31 
close: #2867

Adds middleware for the JSONRPC server to enforce a configurable maximum
batch size for RPC requests.

---

#### PR checklist

- [ ] Tests written/updated
- [X] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [X] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit aa1caba)
mergify bot pushed a commit that referenced this issue May 2, 2024
@andynog @mergify
feat: add a configurable maximum batch size for RPC requests (#2939)
ef93acc 
close: #2867

Adds middleware for the JSONRPC server to enforce a configurable maximum
batch size for RPC requests.

---

#### PR checklist

- [ ] Tests written/updated
- [X] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [X] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit aa1caba)

# Conflicts:
#	docs/references/config/config.toml.md
@mergify mergify bot mentioned this issue May 2, 2024
Merged
4 tasks
@mergify mergify bot mentioned this issue May 2, 2024
Merged
4 tasks
andynog added a commit that referenced this issue May 3, 2024
@mergify @andynog @melekes
feat: add a configurable maximum batch size for RPC requests (backport …
bce83b3 
…#2939) (#2980)

close: #2867 

Adds middleware for the JSONRPC server to enforce a configurable maximum
batch size for RPC requests.

---

#### PR checklist

- [ ] Tests written/updated
- [X] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [X] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #2939 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
andynog added a commit that referenced this issue May 3, 2024
@andynog
remove comment about default value (#2867)
6f2fc74
@andynog andynog mentioned this issue May 3, 2024
Merged
4 tasks
github-merge-queue bot pushed a commit that referenced this issue May 6, 2024
@andynog
fix: remove comment about default value (#2867) (#3000)
cac1fd8 
Related to #2867 

Quick fix for a comment in the code added about a default value for a
config parameter.

During backport to `v0.38.x`, @melekes made a suggestion that the
default value in a comment was inaccurate. Instead of updating the
default value in the comment, I've decided to remove the comment about
the default value.

In my opinion, we should not have default value for config parameters in
comments if it's clear specified in the configuration docs what are the
default and possible values. Realized that having the default values in
comments is not beneficial and updating in the comment might be a not as
straightforward as changing it in code/config.


#### PR checklist

- [ ] ~~Tests written/updated~~
- [ ] ~~Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)~~
- [X] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
mergify bot pushed a commit that referenced this issue May 6, 2024
@andynog @mergify
fix: remove comment about default value (#2867) (#3000)
7de7de6 
Related to #2867

Quick fix for a comment in the code added about a default value for a
config parameter.

During backport to `v0.38.x`, @melekes made a suggestion that the
default value in a comment was inaccurate. Instead of updating the
default value in the comment, I've decided to remove the comment about
the default value.

In my opinion, we should not have default value for config parameters in
comments if it's clear specified in the configuration docs what are the
default and possible values. Realized that having the default values in
comments is not beneficial and updating in the comment might be a not as
straightforward as changing it in code/config.

#### PR checklist

- [ ] ~~Tests written/updated~~
- [ ] ~~Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)~~
- [X] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit cac1fd8)
mergify bot pushed a commit that referenced this issue May 6, 2024
@andynog @mergify
fix: remove comment about default value (#2867) (#3000)
745c1a3 
Related to #2867

Quick fix for a comment in the code added about a default value for a
config parameter.

During backport to `v0.38.x`, @melekes made a suggestion that the
default value in a comment was inaccurate. Instead of updating the
default value in the comment, I've decided to remove the comment about
the default value.

In my opinion, we should not have default value for config parameters in
comments if it's clear specified in the configuration docs what are the
default and possible values. Realized that having the default values in
comments is not beneficial and updating in the comment might be a not as
straightforward as changing it in code/config.

#### PR checklist

- [ ] ~~Tests written/updated~~
- [ ] ~~Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)~~
- [X] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit cac1fd8)

# Conflicts:
#	rpc/jsonrpc/server/http_server.go
This was referenced May 6, 2024
Merged
Merged
melekes added a commit that referenced this issue May 6, 2024
@mergify @andynog @melekes
fix: remove comment about default value (#2867) (backport #3000) (#3013)
887a9ff 
Related to #2867 

Quick fix for a comment in the code added about a default value for a
config parameter.

During backport to `v0.38.x`, @melekes made a suggestion that the
default value in a comment was inaccurate. Instead of updating the
default value in the comment, I've decided to remove the comment about
the default value.

In my opinion, we should not have default value for config parameters in
comments if it's clear specified in the configuration docs what are the
default and possible values. Realized that having the default values in
comments is not beneficial and updating in the comment might be a not as
straightforward as changing it in code/config.


#### PR checklist

- [ ] ~~Tests written/updated~~
- [ ] ~~Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)~~
- [X] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3000 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
melekes added a commit that referenced this issue May 6, 2024
@mergify @andynog @melekes
fix: remove comment about default value (#2867) (backport #3000) (#3012)
933203b 
Related to #2867 

Quick fix for a comment in the code added about a default value for a
config parameter.

During backport to `v0.38.x`, @melekes made a suggestion that the
default value in a comment was inaccurate. Instead of updating the
default value in the comment, I've decided to remove the comment about
the default value.

In my opinion, we should not have default value for config parameters in
comments if it's clear specified in the configuration docs what are the
default and possible values. Realized that having the default values in
comments is not beneficial and updating in the comment might be a not as
straightforward as changing it in code/config.


#### PR checklist

- [ ] ~~Tests written/updated~~
- [ ] ~~Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)~~
- [X] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [X] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3000 done by
[Mergify](https://mergify.com).

Co-authored-by: Andy Nogueira <me@andynogueira.dev>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
@faddat faddat mentioned this issue Oct 15, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@andynog andynog

Labels
config rpc
Projects
No open projects
CometBFT
Status: Done
Milestone
2024-Q2
Development

Successfully merging a pull request may close this issue.

feat: add a configurable maximum batch size for RPC requests cometbft/cometbft
4 participants
@cason @adizere @ValarDragon @andynog
0