-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(jsonrpc): add resource restrict for jsonrpc #6728
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
base: develop
Are you sure you want to change the base?
Changes from all commits
b32fa58
02a588f
eff49b9
19217b8
0351c22
acd51cb
fa87c7e
01da427
7b2585d
a0f51f8
38edfda
e70ea6b
bf9a5a1
9eb44ce
d65f064
801e7ae
2bb35a8
d09d720
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -303,6 +303,9 @@ public void setHttpPBFTPort(int v) { | |
| private int maxBlockRange = 5000; | ||
| private int maxSubTopics = 1000; | ||
| private int maxBlockFilterNum = 50000; | ||
| private int maxBatchSize = 100; | ||
| private int maxResponseSize = 25 * 1024 * 1024; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SHOULD] Use a memory-size config type for maxResponseSize
Suggestion: change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using |
||
| private int maxAddressSize = 1000; | ||
| } | ||
|
|
||
| @Getter | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| package org.tron.core.services.filter; | ||
|
|
||
| import java.io.ByteArrayOutputStream; | ||
| import java.io.IOException; | ||
| import java.io.PrintWriter; | ||
| import javax.servlet.ServletOutputStream; | ||
| import javax.servlet.WriteListener; | ||
| import javax.servlet.http.HttpServletResponse; | ||
| import javax.servlet.http.HttpServletResponseWrapper; | ||
| import lombok.Getter; | ||
|
|
||
| /** | ||
| * Buffers the response body without writing to the underlying response, | ||
| * so the caller can replay it after the handler returns. | ||
| * | ||
| * <p>If {@code maxBytes > 0} and the response would exceed that limit, the | ||
| * {@link #isOverflow()} flag is set instead of throwing. The caller should check this flag after | ||
| * the handler returns and write its own error response when true. | ||
| * | ||
| * <p>Header-mutating methods ({@code setStatus}, {@code setContentType}) are buffered here and | ||
| * only forwarded to the real response via {@link #commitToResponse()}. | ||
| */ | ||
| public class BufferedResponseWrapper extends HttpServletResponseWrapper { | ||
|
|
||
| private final HttpServletResponse actual; | ||
| private final ByteArrayOutputStream buffer = new ByteArrayOutputStream(); | ||
| private final int maxBytes; | ||
| private int status = HttpServletResponse.SC_OK; | ||
| private String contentType; | ||
| private boolean committed = false; | ||
| @Getter | ||
| private volatile boolean overflow = false; | ||
|
|
||
| private final ServletOutputStream outputStream = new ServletOutputStream() { | ||
| @Override | ||
| public void write(int b) { | ||
| if (overflow) { | ||
| return; | ||
| } | ||
| if (maxBytes > 0 && buffer.size() >= maxBytes) { | ||
| markOverflow(); | ||
| return; | ||
| } | ||
| buffer.write(b); | ||
| } | ||
|
|
||
| @Override | ||
| public void write(byte[] b, int off, int len) { | ||
| if (overflow) { | ||
| return; | ||
| } | ||
| if (maxBytes > 0 && buffer.size() + len > maxBytes) { | ||
| markOverflow(); | ||
| return; | ||
| } | ||
| buffer.write(b, off, len); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isReady() { | ||
| return true; | ||
| } | ||
|
|
||
| @Override | ||
| public void setWriteListener(WriteListener writeListener) { | ||
| } | ||
| }; | ||
|
|
||
| private final PrintWriter writer = new PrintWriter(outputStream, true); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [NIT] BufferedResponseWrapper PrintWriter uses platform default charset Suggestion: private final PrintWriter writer =
new PrintWriter(new OutputStreamWriter(outputStream, StandardCharsets.UTF_8), true); |
||
|
|
||
| /** | ||
| * @param response the wrapped response | ||
| * @param maxBytes max allowed response bytes; {@code 0} means no limit | ||
| */ | ||
| public BufferedResponseWrapper(HttpServletResponse response, int maxBytes) { | ||
| super(response); | ||
| this.actual = response; | ||
| this.maxBytes = maxBytes; | ||
| } | ||
|
|
||
| private void markOverflow() { | ||
| overflow = true; | ||
| buffer.reset(); | ||
| } | ||
|
|
||
| /** | ||
| * Early-detection path: if the framework reports the full content length before writing any | ||
| * bytes, we can flag overflow without buffering anything. | ||
| */ | ||
| @Override | ||
| public void setContentLength(int len) { | ||
| if (maxBytes > 0 && len > maxBytes) { | ||
| markOverflow(); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void setContentLengthLong(long len) { | ||
| if (maxBytes > 0 && len > maxBytes) { | ||
| markOverflow(); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public int getStatus() { | ||
| return this.status; | ||
| } | ||
|
|
||
| @Override | ||
| public void setStatus(int sc) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SHOULD] Override getStatus and intercept setHeader/addHeader for Content-Length Header capture currently only covers
Suggestion: override
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks four your review:
|
||
| this.status = sc; | ||
| } | ||
|
|
||
| @Override | ||
| public void setHeader(String name, String value) { | ||
| if ("content-length".equalsIgnoreCase(name)) { | ||
| try { | ||
| setContentLengthLong(Long.parseLong(value)); | ||
| } catch (NumberFormatException ignored) { | ||
| // malformed value, skip overflow check | ||
| } | ||
| } else { | ||
| super.setHeader(name, value); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void addHeader(String name, String value) { | ||
| if ("content-length".equalsIgnoreCase(name)) { | ||
| try { | ||
| setContentLengthLong(Long.parseLong(value)); | ||
| } catch (NumberFormatException ignored) { | ||
| // malformed value, skip overflow check | ||
| } | ||
| } else { | ||
| super.addHeader(name, value); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void setContentType(String type) { | ||
| this.contentType = type; | ||
| } | ||
|
|
||
| @Override | ||
| public ServletOutputStream getOutputStream() { | ||
| return outputStream; | ||
| } | ||
|
|
||
| @Override | ||
| public PrintWriter getWriter() { | ||
| return writer; | ||
| } | ||
|
|
||
| public void commitToResponse() throws IOException { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [NIT] Make commitToResponse idempotent or fail-fast on second call After Suggestion: either clear the buffer at the end of
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a varible |
||
| if (committed) { | ||
| throw new IllegalStateException("commitToResponse() already called"); | ||
| } | ||
| committed = true; | ||
| if (contentType != null) { | ||
| actual.setContentType(contentType); | ||
| } | ||
| actual.setStatus(status); | ||
| actual.setContentLength(buffer.size()); | ||
| buffer.writeTo(actual.getOutputStream()); | ||
| actual.getOutputStream().flush(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package org.tron.core.services.filter; | ||
|
|
||
| import java.io.BufferedReader; | ||
| import java.io.ByteArrayInputStream; | ||
| import java.io.InputStreamReader; | ||
| import java.nio.charset.Charset; | ||
| import java.nio.charset.StandardCharsets; | ||
| import javax.servlet.ReadListener; | ||
| import javax.servlet.ServletInputStream; | ||
| import javax.servlet.http.HttpServletRequest; | ||
| import javax.servlet.http.HttpServletRequestWrapper; | ||
|
|
||
| /** | ||
| * Wraps a request and replays a pre-read body from a byte array. | ||
| */ | ||
| public class CachedBodyRequestWrapper extends HttpServletRequestWrapper { | ||
|
|
||
| private enum BodyAccessor { NONE, STREAM, READER } | ||
|
|
||
| private final byte[] body; | ||
| private BodyAccessor accessor = BodyAccessor.NONE; | ||
|
|
||
| public CachedBodyRequestWrapper(HttpServletRequest request, byte[] body) { | ||
| super(request); | ||
| this.body = body; | ||
| } | ||
|
|
||
| @Override | ||
| public ServletInputStream getInputStream() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SHOULD] getInputStream() and getReader() should be mutually exclusive per servlet spec Servlet 3.1 spec (§ 5.4 / § 5.5) requires that once one of Suggestion: track which accessor was used first (boolean field) and throw
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At present, jsonrpc4j only invokes one of them; this is a potential issue rather than an existing bug. Adding the relevant checks is actually redundant, but i will try it. |
||
| if (accessor == BodyAccessor.READER) { | ||
| throw new IllegalStateException("getReader() has already been called on this request"); | ||
| } | ||
| accessor = BodyAccessor.STREAM; | ||
| final ByteArrayInputStream bais = new ByteArrayInputStream(body); | ||
| return new ServletInputStream() { | ||
| @Override | ||
| public int read() { | ||
| return bais.read(); | ||
| } | ||
|
|
||
| @Override | ||
| public int read(byte[] b, int off, int len) { | ||
| return bais.read(b, off, len); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isFinished() { | ||
| return bais.available() == 0; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isReady() { | ||
| return true; | ||
| } | ||
|
|
||
| @Override | ||
| public void setReadListener(ReadListener readListener) { | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public BufferedReader getReader() { | ||
| if (accessor == BodyAccessor.STREAM) { | ||
| throw new IllegalStateException("getInputStream() has already been called on this request"); | ||
| } | ||
| accessor = BodyAccessor.READER; | ||
| String encoding = getCharacterEncoding(); | ||
| Charset charset = encoding != null ? Charset.forName(encoding) : StandardCharsets.UTF_8; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [NIT] CachedBodyRequestWrapper.getReader does not handle malformed charset Suggestion: Charset charset;
try {
charset = encoding != null ? Charset.forName(encoding) : StandardCharsets.UTF_8;
} catch (IllegalCharsetNameException | UnsupportedCharsetException ex) {
charset = StandardCharsets.UTF_8;
} |
||
| return new BufferedReader(new InputStreamReader(new ByteArrayInputStream(body), charset)); | ||
| } | ||
| } | ||
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.
[SHOULD] Validate non-negative range for the new size-limit fields at config load
The three new fields (
jsonRpcMaxBatchSize,jsonRpcMaxResponseSize,jsonRpcMaxAddressSize) are read viaArgs.applyNodeConfigwith no range validation. The> 0guards in the call sites mean a negative value silently becomes a permanent 'no limit' state — that is fine if<= 0is the documented contract, but neitherreference.confnorconfig.confsays so explicitly, only> 0 otherwise no limit. Operators reading the comment may assume only0disables the limit; setting-1(a common 'unset' sentinel) silently has the same effect, whileInteger.MIN_VALUEis also accepted with no warning.Suggestion: validate
value >= 0inArgs.applyNodeConfig(reject startup with a clear error on negative values), and update the reference/config comments to spell out the exact 'disabled' semantics — e.g.# 0 disables the limit; negative values are rejected at startup.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.
The comment specifies it already, but I can optimze it as
<=0 means no limitin config.conf.