Closed
Conversation
Squashed commits: [6bd4faa] fix(API): add whitelist rate limiter adapter
📝 WalkthroughWalkthroughThree files were updated to improve robustness and security: resource processor gained a clarifying comment, transaction trace added zero-size guards to prevent division by zero, rate limiter servlet replaced dynamic reflection-based adapter loading with a fixed whitelist map, and a new test suite was added to validate the whitelist logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java (1)
33-87: Please add one behavior-level test around servlet initialization.This suite only inspects the private map. If
RateLimiterServlet.addRateContainer()later bypassed the whitelist, these tests would still stay green. One happy-path init test and one unknown-strategy rejection test would cover the security behavior this PR is actually changing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java` around lines 33 - 87, Add two integration-style tests that exercise servlet initialization rather than just the private whitelist map: 1) a happy-path test that instantiates RateLimiterServlet, calls init(...) (using a Mock/Minimal ServletConfig/ServletContext) with init-params that reference a known key such as DEFAULT_BASE_QPS or QPS_RATE_LIMITER and then asserts that addRateContainer() behavior occurred (e.g., the servlet exposes the configured container or a non-null internal container/state after init); 2) a negative test that calls init(...) with an unknown/invalid strategy name and asserts the servlet rejects it (either by throwing ServletException or by leaving the rate-container unregistered/null). Target the RateLimiterServlet.init(...) and RateLimiterServlet.addRateContainer(...) code paths so the tests will fail if init bypasses the ALLOWED_ADAPTERS whitelist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@chainbase/src/main/java/org/tron/core/db/TransactionTrace.java`:
- Around line 304-305: Persisted zero window sizes in TransactionTrace lead to
divide-by-zero later in ResourceProcessor.increase() when divideCeil(lastUsage *
precision, oldWindowSize) is called; modify the code that writes window sizes
(the expressions that set newUsage/newSize and newUsage/newSize2 in
TransactionTrace) so that when usage is zero you persist a positive fallback
(e.g., use currentSize > 0 ? currentSize : 1L and currentSize2 > 0 ?
currentSize2 : 1L) instead of 0L, ensuring divideCeil never receives a zero
denominator; update the logic around the newUsage computations to use these
non-zero fallbacks while keeping the existing semantics when usage is non-zero.
In
`@framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java`:
- Around line 43-58: The testWhitelistContents() assertions are too permissive:
replace the isAssignableFrom() checks on
allowedAdapters.get(GLOBAL_PREEMPTIBLE), get(QPS_RATE_LIMITER),
get(IP_QPS_RATE_LIMITER), and get(DEFAULT_BASE_QPS) with assertSame(...) against
the exact classes GlobalPreemptibleAdapter.class, QpsRateLimiterAdapter.class,
IPQPSRateLimiterAdapter.class, and DefaultBaseQqsAdapter.class respectively, and
add an assertEquals(expectedSize, allowedAdapters.size()) to verify the map size
for the whitelist contract.
---
Nitpick comments:
In
`@framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java`:
- Around line 33-87: Add two integration-style tests that exercise servlet
initialization rather than just the private whitelist map: 1) a happy-path test
that instantiates RateLimiterServlet, calls init(...) (using a Mock/Minimal
ServletConfig/ServletContext) with init-params that reference a known key such
as DEFAULT_BASE_QPS or QPS_RATE_LIMITER and then asserts that addRateContainer()
behavior occurred (e.g., the servlet exposes the configured container or a
non-null internal container/state after init); 2) a negative test that calls
init(...) with an unknown/invalid strategy name and asserts the servlet rejects
it (either by throwing ServletException or by leaving the rate-container
unregistered/null). Target the RateLimiterServlet.init(...) and
RateLimiterServlet.addRateContainer(...) code paths so the tests will fail if
init bypasses the ALLOWED_ADAPTERS whitelist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b517087-2d82-4604-864c-0e57c5de76cb
📒 Files selected for processing (4)
chainbase/src/main/java/org/tron/core/db/ResourceProcessor.javachainbase/src/main/java/org/tron/core/db/TransactionTrace.javaframework/src/main/java/org/tron/core/services/http/RateLimiterServlet.javaframework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java
Comment on lines
+304
to
+305
| long newUsage = newSize == 0 ? 0 | ||
| : max(0, newArea / newSize, dynamicPropertiesStore.disableJavaLangMath()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify zero-size persistence and denominator usage paths.
rg -n -C2 --type=java \
'newUsage == 0 \? 0L : newSize|newUsage == 0 \? 0L : newSize2|divideCeil\(lastUsage \* this\.precision, oldWindowSize\)|divideCeil\(lastUsage \* precision, windowSize\)'Repository: Sunny6889/java-tron
Length of output: 3476
🏁 Script executed:
#!/bin/bash
# Get divideCeil implementation to check for zero-handling
rg -A10 "^\s*long divideCeil\(" --type=java chainbase/src/main/java/org/tron/core/db/Repository: Sunny6889/java-tron
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check for guards before divideCeil in ResourceProcessor.increase() method (lines 47-50)
sed -n '40,60p' chainbase/src/main/java/org/tron/core/db/ResourceProcessor.javaRepository: Sunny6889/java-tron
Length of output: 1022
🏁 Script executed:
#!/bin/bash
# Check the full context of line 78-80 in ResourceProcessor
sed -n '70,90p' chainbase/src/main/java/org/tron/core/db/ResourceProcessor.javaRepository: Sunny6889/java-tron
Length of output: 941
🏁 Script executed:
#!/bin/bash
# Check variables available at line 308 (currentSize context)
sed -n '290,310p' chainbase/src/main/java/org/tron/core/db/TransactionTrace.javaRepository: Sunny6889/java-tron
Length of output: 1127
🏁 Script executed:
#!/bin/bash
# Find divideCeil implementation
rg -B2 -A8 "private.*divideCeil|protected.*divideCeil|public.*divideCeil" --type=java chainbase/src/main/java/org/tron/core/db/Repository: Sunny6889/java-tron
Length of output: 1117
🏁 Script executed:
#!/bin/bash
# Check V2 version around line 322
sed -n '310,340p' chainbase/src/main/java/org/tron/core/db/TransactionTrace.javaRepository: Sunny6889/java-tron
Length of output: 1392
🏁 Script executed:
#!/bin/bash
# Check for any guards before divideCeil calls in ResourceProcessor - full increase method
sed -n '73,85p' chainbase/src/main/java/org/tron/core/db/ResourceProcessor.javaRepository: Sunny6889/java-tron
Length of output: 755
Persisted zero window sizes cause division by zero in ResourceProcessor.
The zero window size persistence at line 308 (newUsage == 0 ? 0L : newSize) and line 326 (newUsage == 0 ? 0L : newSize2) creates a critical issue. When ResourceProcessor.increase() retrieves the stored window size and calls divideCeil(lastUsage * precision, oldWindowSize) at line 78, a persisted zero will cause ArithmeticException: / by zero since divideCeil performs direct division without checking the denominator.
The available variables (currentSize at line 295, currentSize2 at line 313) enable the proposed fallback: persist a positive value (e.g., currentSize > 0 ? currentSize : 1L) when usage is zero, rather than persisting 0L.
Proposed fix
accountCap.setEnergyUsage(newUsage);
- accountCap.setNewWindowSize(ENERGY, newUsage == 0 ? 0L : newSize);
+ long persistedSize = newUsage == 0 ? (currentSize > 0 ? currentSize : 1L) : newSize;
+ accountCap.setNewWindowSize(ENERGY, persistedSize);and
accountCap.setEnergyUsage(newUsage);
- accountCap.setNewWindowSizeV2(ENERGY, newUsage == 0 ? 0L : newSize2);
+ long persistedSize2 = newUsage == 0 ? (currentSize2 > 0 ? currentSize2 : 1L) : newSize2;
+ accountCap.setNewWindowSizeV2(ENERGY, persistedSize2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chainbase/src/main/java/org/tron/core/db/TransactionTrace.java` around lines
304 - 305, Persisted zero window sizes in TransactionTrace lead to
divide-by-zero later in ResourceProcessor.increase() when divideCeil(lastUsage *
precision, oldWindowSize) is called; modify the code that writes window sizes
(the expressions that set newUsage/newSize and newUsage/newSize2 in
TransactionTrace) so that when usage is zero you persist a positive fallback
(e.g., use currentSize > 0 ? currentSize : 1L and currentSize2 > 0 ?
currentSize2 : 1L) instead of 0L, ensuring divideCeil never receives a zero
denominator; update the logic around the newUsage computations to use these
non-zero fallbacks while keeping the existing semantics when usage is non-zero.
Comment on lines
+43
to
+58
| public void testWhitelistContents() { | ||
| assertNotNull(allowedAdapters.get(GLOBAL_PREEMPTIBLE)); | ||
| assertTrue(allowedAdapters.get(GLOBAL_PREEMPTIBLE) | ||
| .isAssignableFrom(GlobalPreemptibleAdapter.class)); | ||
|
|
||
| assertNotNull(allowedAdapters.get(QPS_RATE_LIMITER)); | ||
| assertTrue(allowedAdapters.get(QPS_RATE_LIMITER) | ||
| .isAssignableFrom(QpsRateLimiterAdapter.class)); | ||
|
|
||
| assertNotNull(allowedAdapters.get(IP_QPS_RATE_LIMITER)); | ||
| assertTrue(allowedAdapters.get(IP_QPS_RATE_LIMITER) | ||
| .isAssignableFrom(IPQPSRateLimiterAdapter.class)); | ||
|
|
||
| assertNotNull(allowedAdapters.get(DEFAULT_BASE_QPS)); | ||
| assertTrue(allowedAdapters.get(DEFAULT_BASE_QPS) | ||
| .isAssignableFrom(DefaultBaseQqsAdapter.class)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find framework -name "RateLimiterServletWhitelistTest.java" -type fRepository: Sunny6889/java-tron
Length of output: 151
🏁 Script executed:
cat -n framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.javaRepository: Sunny6889/java-tron
Length of output: 4556
🏁 Script executed:
# Check if the adapter classes exist
find framework -name "GlobalPreemptibleAdapter.class" -o -name "GlobalPreemptibleAdapter.java" | head -20Repository: Sunny6889/java-tron
Length of output: 159
🏁 Script executed:
# Search for the adapter class definitions
rg "class GlobalPreemptibleAdapter|class QpsRateLimiterAdapter|class IPQPSRateLimiterAdapter|class DefaultBaseQqsAdapter" --type javaRepository: Sunny6889/java-tron
Length of output: 703
🏁 Script executed:
# Verify the current imports in the test file
head -25 framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.javaRepository: Sunny6889/java-tron
Length of output: 1066
Use exact assertions for the whitelist contract.
The isAssignableFrom() checks are too permissive. If the map accidentally stored IRateLimiter.class instead of the specific adapter class, the test would still pass because IRateLimiter.class.isAssignableFrom(GlobalPreemptibleAdapter.class) returns true. Use assertSame() to enforce exact class identity, and use assertEquals() for the size comparison to provide clearer assertion semantics.
🧪 Tighten the assertions
+import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
@@
- assertNotNull(allowedAdapters.get(GLOBAL_PREEMPTIBLE));
- assertTrue(allowedAdapters.get(GLOBAL_PREEMPTIBLE)
- .isAssignableFrom(GlobalPreemptibleAdapter.class));
+ assertSame(GlobalPreemptibleAdapter.class, allowedAdapters.get(GLOBAL_PREEMPTIBLE));
@@
- assertNotNull(allowedAdapters.get(QPS_RATE_LIMITER));
- assertTrue(allowedAdapters.get(QPS_RATE_LIMITER)
- .isAssignableFrom(QpsRateLimiterAdapter.class));
+ assertSame(QpsRateLimiterAdapter.class, allowedAdapters.get(QPS_RATE_LIMITER));
@@
- assertNotNull(allowedAdapters.get(IP_QPS_RATE_LIMITER));
- assertTrue(allowedAdapters.get(IP_QPS_RATE_LIMITER)
- .isAssignableFrom(IPQPSRateLimiterAdapter.class));
+ assertSame(IPQPSRateLimiterAdapter.class, allowedAdapters.get(IP_QPS_RATE_LIMITER));
@@
- assertNotNull(allowedAdapters.get(DEFAULT_BASE_QPS));
- assertTrue(allowedAdapters.get(DEFAULT_BASE_QPS)
- .isAssignableFrom(DefaultBaseQqsAdapter.class));
+ assertSame(DefaultBaseQqsAdapter.class, allowedAdapters.get(DEFAULT_BASE_QPS));
@@
- assertTrue("Whitelist must contain exactly 4 adapters", allowedAdapters.size() == 4);
+ assertEquals("Whitelist must contain exactly 4 adapters", 4, allowedAdapters.size());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@framework/src/test/java/org/tron/core/services/http/RateLimiterServletWhitelistTest.java`
around lines 43 - 58, The testWhitelistContents() assertions are too permissive:
replace the isAssignableFrom() checks on
allowedAdapters.get(GLOBAL_PREEMPTIBLE), get(QPS_RATE_LIMITER),
get(IP_QPS_RATE_LIMITER), and get(DEFAULT_BASE_QPS) with assertSame(...) against
the exact classes GlobalPreemptibleAdapter.class, QpsRateLimiterAdapter.class,
IPQPSRateLimiterAdapter.class, and DefaultBaseQqsAdapter.class respectively, and
add an assertEquals(expectedSize, allowedAdapters.size()) to verify the map size
for the whitelist contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
Summary by CodeRabbit
Bug Fixes
Refactor
Tests