fix(api): hide jetty internals in oversized request 413 response#38
fix(api): hide jetty internals in oversized request 413 response#38bladehan1 wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
6 issues found across 29 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="chainbase/src/main/java/org/tron/common/utils/Commons.java">
<violation number="1" location="chainbase/src/main/java/org/tron/common/utils/Commons.java:24">
P1: BASE58_ADDRESS_LENGTH = 34 is wrong; valid TRON Base58Check addresses can be 34 or 35 chars. Strict != 34 check rejects valid 35-char addresses. Existing codebase uses 35 (framework/…/Base58.java BASE58CHECK_ADDRESS_SIZE).</violation>
</file>
<file name="framework/src/main/java/org/tron/common/application/HttpService.java">
<violation number="1" location="framework/src/main/java/org/tron/common/application/HttpService.java:118">
P2: Missing Cache-Control header on 413 path. Default ErrorHandler sets must-revalidate,no-cache,no-store; the early return before super.handle() skips it, allowing intermediaries to cache the error response.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/services/event/RealtimeEventService.java">
<violation number="1" location="framework/src/main/java/org/tron/core/services/event/RealtimeEventService.java:87">
P2: Direct block trigger processing is not exception-isolated; a plugin/runtime exception can abort flush and drop remaining realtime events.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| public static final int ASSET_ISSUE_COUNT_LIMIT_MAX = 1000; | ||
|
|
||
| public static final int BASE58_ADDRESS_LENGTH = 34; |
There was a problem hiding this comment.
P1: BASE58_ADDRESS_LENGTH = 34 is wrong; valid TRON Base58Check addresses can be 34 or 35 chars. Strict != 34 check rejects valid 35-char addresses. Existing codebase uses 35 (framework/…/Base58.java BASE58CHECK_ADDRESS_SIZE).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At chainbase/src/main/java/org/tron/common/utils/Commons.java, line 24:
<comment>BASE58_ADDRESS_LENGTH = 34 is wrong; valid TRON Base58Check addresses can be 34 or 35 chars. Strict != 34 check rejects valid 35-char addresses. Existing codebase uses 35 (framework/…/Base58.java BASE58CHECK_ADDRESS_SIZE).</comment>
<file context>
@@ -21,6 +21,8 @@ public class Commons {
public static final int ASSET_ISSUE_COUNT_LIMIT_MAX = 1000;
+ public static final int BASE58_ADDRESS_LENGTH = 34;
+
public static byte[] decode58Check(String input) {
</file context>
| Throwable cause = (Throwable) request.getAttribute(RequestDispatcher.ERROR_EXCEPTION); | ||
| logger.info("Reject oversized request, uri: {}, detail: {}", | ||
| request.getRequestURI(), cause == null ? "413" : cause.getMessage()); | ||
| baseRequest.setHandled(true); |
There was a problem hiding this comment.
P2: Missing Cache-Control header on 413 path. Default ErrorHandler sets must-revalidate,no-cache,no-store; the early return before super.handle() skips it, allowing intermediaries to cache the error response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/common/application/HttpService.java, line 118:
<comment>Missing Cache-Control header on 413 path. Default ErrorHandler sets must-revalidate,no-cache,no-store; the early return before super.handle() skips it, allowing intermediaries to cache the error response.</comment>
<file context>
@@ -88,4 +99,30 @@ protected ServletContextHandler initContextHandler() {
+ Throwable cause = (Throwable) request.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
+ logger.info("Reject oversized request, uri: {}, detail: {}",
+ request.getRequestURI(), cause == null ? "413" : cause.getMessage());
+ baseRequest.setHandled(true);
+ ByteBuffer body = badMessageError(HttpStatus.PAYLOAD_TOO_LARGE_413,
+ HttpStatus.getMessage(HttpStatus.PAYLOAD_TOO_LARGE_413),
</file context>
| baseRequest.setHandled(true); | |
| baseRequest.setHandled(true); | |
| response.setHeader("Cache-Control", "must-revalidate,no-cache,no-store"); |
| } else { | ||
| manager.getTriggerCapsuleQueue().offer(blockEvent.getBlockLogTriggerCapsule()); | ||
| blockEvent.getBlockLogTriggerCapsule().setRemoved(isRemove); | ||
| blockEvent.getBlockLogTriggerCapsule().processTrigger(); |
There was a problem hiding this comment.
P2: Direct block trigger processing is not exception-isolated; a plugin/runtime exception can abort flush and drop remaining realtime events.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/event/RealtimeEventService.java, line 87:
<comment>Direct block trigger processing is not exception-isolated; a plugin/runtime exception can abort flush and drop remaining realtime events.</comment>
<file context>
@@ -77,25 +73,31 @@ public synchronized void work() {
} else {
- manager.getTriggerCapsuleQueue().offer(blockEvent.getBlockLogTriggerCapsule());
+ blockEvent.getBlockLogTriggerCapsule().setRemoved(isRemove);
+ blockEvent.getBlockLogTriggerCapsule().processTrigger();
}
}
</file context>
| blockEvent.getBlockLogTriggerCapsule().processTrigger(); | |
| try { | |
| blockEvent.getBlockLogTriggerCapsule().processTrigger(); | |
| } catch (Throwable t) { | |
| logger.error("Process block trigger failed. {}", blockEvent.getBlockId().getString(), t); | |
| } |
The default Jetty ErrorHandler renders the 413 thrown by SizeLimitHandler into an HTML page exposing the full exception stack, Jetty version and the internal request size. Add a private OversizedRequestErrorHandler that logs the detail server-side and returns the short bad-message page for 413 only; other errors keep Jetty's default handling. Tests assert the 413 body renders the short bad-message page and leaks no Jetty stack/class/size, and that other 4xx (malformed Content-Length 400, oversized URI 414) stay on Jetty's default path.
2631a12 to
cb8fdbd
Compare
|
Superseded by upstream PR tronprotocol#6843 (same change). |
What does this PR do?
Installs a custom Jetty
ErrorHandler(privateOversizedRequestErrorHandlerinHttpService) so that for an oversized request — the413thrown bySizeLimitHandlerduring dispatch — the node logs the detail server-side and returns the short, uniform bad-message page. Previously the default Jetty error page exposed the full exception stack, the Jetty version and the internal request size in the413response body.The handler special-cases
413only; all other errors fall through tosuper(Jetty's default handling).Why are these changes required?
The
413response body leaked Jetty internals (CWE-209 information disclosure / server fingerprinting): exact Jetty version, internal class names and line numbers, thread-pool internals, the JDK build, and the rejected request size, e.g.:Only the
413path is affected, becauseSizeLimitHandlerthrows during dispatch (Server.handle) and is rendered viaErrorHandler.handle()->writeErrorPageStacks. Parser-stage errors (400/414/431) already render viabadMessageError()without a stack, so they are intentionally left untouched.This PR has been tested by:
SizeLimitHandlerTest#testHttpBodyExceedsLimit:413body renders the short bad-message page and leaks no Jetty stack / class / size.SizeLimitHandlerTest#testBadContentLengthReturnsDefault400,testOversizedUriReturnsDefault414: other 4xx stay on Jetty's default path (not rerouted through the413branch).curlwith an oversized body returns413with the short page; the response no longer contains the exception stack, class name or internal size.Follow up
Extra details
Jetty
9.4.58. The fix lives in the base classHttpService#initServer()so it applies to every HTTP service (FullNode HTTP, Solidity, PBFT, JSON-RPC). Thereasonreturned to the client is the genericHttpStatus.getMessage(413)("Payload Too Large"); the full detail (including the rejected size) is kept only in the server log.