fix: restore global rate limit fallback for passive sources#1761
fix: restore global rate limit fallback for passive sources#1761hriszc wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughExtracted per-source rate-limit logic into a new helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
🧹 Nitpick comments (2)
pkg/passive/passive_test.go (2)
41-47: Consider adding a test case for case-insensitivity.The
resolveSourceRateLimitfunction appliesstrings.ToLower(sourceName)to ensure case-insensitive matching. Adding an explicit test case (e.g.,sourceName: "SiteDossier"expecting to match the"sitedossier"key) would help protect this behavior against future regressions.🤖 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 41 - 47, Add a test case to passive_test.go that verifies resolveSourceRateLimit is case-insensitive: in the existing table-driven tests (the one using customRates and globalRateLimit), add an entry with sourceName set to "SiteDossier" (mixed case) and expected equal to the same value as the "sitedossier" customRates entry (e.g., 2); this ensures resolveSourceRateLimit (which calls strings.ToLower(sourceName)) continues to match keys regardless of case.
11-18: UseNewSyncLockMapconstructor instead of direct struct literal initialization.The test initializes
SyncLockMapvia struct literal (Map: map[string]uint{...}), which bypasses the constructor and its initialization logic. The codebase already demonstrates the correct pattern inpkg/passive/sources.gowithNewSyncLockMap[string, string](mapsutil.WithMap(...)). This same pattern also appears inpkg/runner/runner.goand should be refactored in both places for consistency.Use:
customRates := &subscraping.CustomRateLimit{ Custom: *mapsutil.NewSyncLockMap[string, uint]( mapsutil.WithMap(mapsutil.Map[string, uint]{ "sitedossier": 2, "github": 0, }), ), }🤖 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 11 - 18, The test constructs a mapsutil.SyncLockMap via a struct literal which bypasses its constructor; update the customRates initialization in passive_test.go to use mapsutil.NewSyncLockMap with mapsutil.WithMap (e.g., create CustomRateLimit with Custom set to *mapsutil.NewSyncLockMap[string, uint](mapsutil.WithMap(mapsutil.Map[string, uint]{...}))) so the SyncLockMap is properly initialized; modify the customRates variable (and any similar direct initializations) to use NewSyncLockMap instead of the Map: struct literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/passive/passive_test.go`:
- Around line 41-47: Add a test case to passive_test.go that verifies
resolveSourceRateLimit is case-insensitive: in the existing table-driven tests
(the one using customRates and globalRateLimit), add an entry with sourceName
set to "SiteDossier" (mixed case) and expected equal to the same value as the
"sitedossier" customRates entry (e.g., 2); this ensures resolveSourceRateLimit
(which calls strings.ToLower(sourceName)) continues to match keys regardless of
case.
- Around line 11-18: The test constructs a mapsutil.SyncLockMap via a struct
literal which bypasses its constructor; update the customRates initialization in
passive_test.go to use mapsutil.NewSyncLockMap with mapsutil.WithMap (e.g.,
create CustomRateLimit with Custom set to *mapsutil.NewSyncLockMap[string,
uint](mapsutil.WithMap(mapsutil.Map[string, uint]{...}))) so the SyncLockMap is
properly initialized; modify the customRates variable (and any similar direct
initializations) to use NewSyncLockMap instead of the Map: struct literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85708726-d29f-41c7-b080-be1dd5ea7f90
📒 Files selected for processing (2)
pkg/passive/passive.gopkg/passive/passive_test.go
Neo - PR Security ReviewNo security issues found Highlights
Comment |
|
Addressed the review nits:
The PR head has been updated accordingly. |
|
Follow-up: the CodeRabbit suggestions are already addressed, checks are green, and the regression coverage is in place. This should be ready for human review when convenient. |
1 participant
/claim #1434
Summary
Restore the global
-rlfallback semantics for passive sources when no provider-specific entry is present in-rls.-rlsoverrides intact when a provider limit is explicitly set-rlnor-rlsprovides a limitValidation
Ran:
Result: passed.
Summary by CodeRabbit
Refactor
Tests