fix: honor -rl (global rate limit) when -rls is not set#1753
Closed
Rhan2020 wants to merge 2 commits intoprojectdiscovery:devfrom
Closed
fix: honor -rl (global rate limit) when -rls is not set#1753Rhan2020 wants to merge 2 commits intoprojectdiscovery:devfrom
Rhan2020 wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
Ensure global -rl applies to all sources by default, even when -rls is not provided. Also guard against nil custom rate limit and clamp negative global rate limits to zero. Adds tests for nil custom rate limit and negative global rate limit.
Neo - PR Security ReviewNo security issues found Highlights
Comment |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughDefaulting for per-source rate limits in the passive package was changed: per-source limits now start from a computed global value (non-zero only when globalRateLimit > 0) and are overridden only when a CustomRateLimit is provided. Tests were added to validate various global and per-source scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/passive/passive_test.go (1)
25-35: Tests verify success but not actual behavior.The test confirms the function doesn't error with a global rate limit of 3 and a custom entry for "crtsh" only, but it doesn't verify that "hackertarget" actually received the global limit of 3. Consider adding assertions on the returned
MultiLimiterstate if the API allows inspection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/passive/passive_test.go` around lines 25 - 35, The test only checks no error from agent.buildMultiRateLimiter but doesn't verify that the non-custom source "hackertarget" received the global limit; update the test to assert the returned MultiLimiter state for "hackertarget" equals the global limit (3) by inspecting whatever accessor exists on the returned value (e.g., a Get/LimitFor(source) method or an exported map inside the MultiLimiter), and if no inspector exists add a small test-only accessor on MultiLimiter (or expose its internal map) so the test can assert limiter value for "hackertarget"; reference New, buildMultiRateLimiter, customRL, and MultiLimiter when locating code to change.pkg/passive/passive.go (2)
89-92: Consider checking before converting for clarity.When
globalRateLimitis negative,uint(globalRateLimit)produces a large wrapped value before line 91 corrects it. The code is functionally correct, but checking first is more readable.Cleaner ordering
- global := uint(globalRateLimit) - if globalRateLimit < 0 { - global = 0 + var global uint + if globalRateLimit > 0 { + global = uint(globalRateLimit) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/passive/passive.go` around lines 89 - 92, Replace the current conversion-first pattern by checking globalRateLimit before casting: instead of doing global := uint(globalRateLimit) and then correcting when globalRateLimit < 0, branch on globalRateLimit and assign global = 0 for negatives, otherwise set global = uint(globalRateLimit); refer to the variables globalRateLimit and global in passive.go to locate the change.
116-121: Note: Explicit per-source 0 cannot override to unlimited when a global limit is set.If a user sets
-rls source=0intending to make that source unlimited while-rlis non-zero,sourceRateLimitOrDefaultreturns the global default instead. This may be intentional (0 means "inherit default"), but worth documenting if users expect explicit 0 to mean "unlimited for this source."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/passive/passive.go` around lines 116 - 121, The helper sourceRateLimitOrDefault currently treats sourceRateLimit==0 as "use default" which prevents an explicit per-source 0 from meaning unlimited; change the logic in sourceRateLimitOrDefault to return sourceRateLimit when it is explicitly provided (including 0) and only fall back to defaultRateLimit when sourceRateLimit is some sentinel like an unset value (e.g., use a pointer or a separate boolean flag) or, simpler, document the behavior and rename the function to make semantics clear; update callers of sourceRateLimitOrDefault and any user docs/comments so that a sourceRateLimit of 0 is either explicitly defined as unlimited or clearly documented as "inherit default."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/passive/passive_test.go`:
- Around line 48-54: The test "UnlimitedGlobalRateLimitBehavesLikeUnlimited" is
misleading because it passes math.MaxInt32 which does not hit the unlimited
branch (addRateLimiter checks for math.MaxUint32); update the test to pass
math.MaxUint32 (or change the test name to reflect a very large limit) so it
actually exercises the unlimited code path in
buildMultiRateLimiter/addRateLimiter — ensure any parameter types are adjusted
to accept the uint value if necessary and keep assertions the same
(require.NoError, require.NotNil) after the change.
---
Nitpick comments:
In `@pkg/passive/passive_test.go`:
- Around line 25-35: The test only checks no error from
agent.buildMultiRateLimiter but doesn't verify that the non-custom source
"hackertarget" received the global limit; update the test to assert the returned
MultiLimiter state for "hackertarget" equals the global limit (3) by inspecting
whatever accessor exists on the returned value (e.g., a Get/LimitFor(source)
method or an exported map inside the MultiLimiter), and if no inspector exists
add a small test-only accessor on MultiLimiter (or expose its internal map) so
the test can assert limiter value for "hackertarget"; reference New,
buildMultiRateLimiter, customRL, and MultiLimiter when locating code to change.
In `@pkg/passive/passive.go`:
- Around line 89-92: Replace the current conversion-first pattern by checking
globalRateLimit before casting: instead of doing global := uint(globalRateLimit)
and then correcting when globalRateLimit < 0, branch on globalRateLimit and
assign global = 0 for negatives, otherwise set global = uint(globalRateLimit);
refer to the variables globalRateLimit and global in passive.go to locate the
change.
- Around line 116-121: The helper sourceRateLimitOrDefault currently treats
sourceRateLimit==0 as "use default" which prevents an explicit per-source 0 from
meaning unlimited; change the logic in sourceRateLimitOrDefault to return
sourceRateLimit when it is explicitly provided (including 0) and only fall back
to defaultRateLimit when sourceRateLimit is some sentinel like an unset value
(e.g., use a pointer or a separate boolean flag) or, simpler, document the
behavior and rename the function to make semantics clear; update callers of
sourceRateLimitOrDefault and any user docs/comments so that a sourceRateLimit of
0 is either explicitly defined as unlimited or clearly documented as "inherit
default."
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
1 participant
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.
Fixes #1434.
The global rate limit flag (
-rl) was effectively ignored for sources that don't have an explicit per-source entry in-rls, causing subfinder to run unthrottled unless-rlswas provided.Changes:
-rlto every source by default.-rlscontains an entry for a source, it overrides the global value.-rlvalues to 0.Tests:
/claim #1434
Summary by CodeRabbit
New Features
Bug Fixes
Tests