fix(sources/virustotal): cap pagination + clearer 429 message#1788
Conversation
Co-authored-by: Pepijn van der Stap <205847092+x-stp@users.noreply.github.com>
Co-authored-by: Pepijn van der Stap <205847092+x-stp@users.noreply.github.com>
Co-authored-by: Pepijn van der Stap <205847092+x-stp@users.noreply.github.com>
Neo - PR Security ReviewNo security issues found Comment |
WalkthroughThe PR adds a per-source maximum results limit, threads it from CLI/config through passive enumeration into session state, and updates the VirusTotal source to stop at that cap while handling HTTP 429 responses explicitly. New tests cover the new limit and pagination behavior. ChangesResult limit propagation and VirusTotal scraping
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/subscraping/sources/virustotal/virustotal.go (1)
88-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
defer resp.Body.Close()inside aforloop accumulates open bodies until the goroutine exits.Defers run at function exit, not at end of each loop iteration. With
defaultMaxPages = 10and a long cursor chain you can hold up to 10 HTTP response bodies (and their underlying TCP connections) open simultaneously, even though only one is being read at any moment. Close per iteration instead.♻️ Proposed fix — close the body explicitly at the end of each iteration
resp, err := session.Get(ctx, url, "", map[string]string{"x-apikey": randomApiKey}) if err != nil { if resp != nil && resp.StatusCode == http.StatusTooManyRequests { results <- subscraping.Result{Source: s.Name(), Type: subscraping.Error, Error: fmt.Errorf("virustotal free-tier quota exhausted (HTTP 429); some subdomains for %s may be missing", domain)} } else { results <- subscraping.Result{Source: s.Name(), Type: subscraping.Error, Error: err} } s.errors++ session.DiscardHTTPResponse(resp) return } - defer func() { - if err := resp.Body.Close(); err != nil { - results <- subscraping.Result{Source: s.Name(), Type: subscraping.Error, Error: err} - s.errors++ - } - }() var data response err = jsoniter.NewDecoder(resp.Body).Decode(&data) + if cerr := resp.Body.Close(); cerr != nil { + results <- subscraping.Result{Source: s.Name(), Type: subscraping.Error, Error: cerr} + s.errors++ + } if err != nil { results <- subscraping.Result{Source: s.Name(), Type: subscraping.Error, Error: err} s.errors++ return }Alternatively, wrap each iteration in an anonymous function so the
deferfires per page.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/subscraping/sources/virustotal/virustotal.go` around lines 88 - 93, The defer resp.Body.Close() inside the loop (see resp and resp.Body.Close() in virustotal.go) holds response bodies until the goroutine exits; replace the deferred close with an explicit resp.Body.Close() call at the end of each loop iteration (after you've finished reading/parsing the response) or wrap the per-page logic in an anonymous function so the defer runs per-iteration; ensure you still send a subscraping.Result error on close failure (the current results <- subscraping.Result{Source: s.Name(), Type: subscraping.Error, Error: err} handling should remain).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/subscraping/sources/virustotal/virustotal.go`:
- Around line 88-93: The defer resp.Body.Close() inside the loop (see resp and
resp.Body.Close() in virustotal.go) holds response bodies until the goroutine
exits; replace the deferred close with an explicit resp.Body.Close() call at the
end of each loop iteration (after you've finished reading/parsing the response)
or wrap the per-page logic in an anonymous function so the defer runs
per-iteration; ensure you still send a subscraping.Result error on close failure
(the current results <- subscraping.Result{Source: s.Name(), Type:
subscraping.Error, Error: err} handling should remain).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa375abc-affc-4d3d-8f2b-d16780f49539
📒 Files selected for processing (2)
pkg/subscraping/sources/virustotal/virustotal.gopkg/subscraping/sources/virustotal/virustotal_test.go
Mzack9999
force-pushed
the
fix/virustotal-free-tier-limits-1718
branch
from
85e2354 to
76cad6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/runner/options.go`:
- Around line 72-76: The VirusTotal pagination cap still defaults to unlimited
because MaxResults is left at 0 and virustotal.Run reads session.MaxResults
directly. Update the defaulting logic in the runner options/session setup for
MaxResults so VirusTotal gets a nonzero cap out of the box, while keeping the
explicit --max-results override working as expected. Use the MaxResults field
and virustotal.Run as the main points to adjust.
In `@pkg/subscraping/sources/virustotal/virustotal_test.go`:
- Line 109: Fix the errcheck failures in the VirusTotal test-server handlers by
handling or explicitly discarding the return values from the fmt.Fprintf and
fmt.Fprint calls inside the test closures. Update the affected handlers in
virustotal_test.go so each write either checks the returned error with t or
assigns both return values to blank identifiers, keeping the existing test logic
intact.
In `@pkg/subscraping/sources/virustotal/virustotal.go`:
- Around line 51-54: The VirusTotal source currently only honors a result cap
when MaxResults is explicitly set, so standard runs still paginate indefinitely
and can consume quota. Update the default in pkg/runner/options.go to a non-zero
MaxResults value, and ensure virustotal.go’s maxResults handling in the
VirusTotal scraping flow continues to use session.MaxResults as the limit
source.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: acf2c836-f0f1-43e4-b75d-4b15977edfe7
📒 Files selected for processing (8)
README.mdpkg/passive/passive.gopkg/runner/enumerate.gopkg/runner/options.gopkg/runner/validate.gopkg/subscraping/sources/virustotal/virustotal.gopkg/subscraping/sources/virustotal/virustotal_test.gopkg/subscraping/types.go
✅ Files skipped from review due to trivial changes (1)
- README.md
|
|
||
| // MaxResults limits the number of results requested per source. | ||
| // A value of 0 (default) means no limit. Only sources that paginate | ||
| // honor this (currently virustotal), to help stay within API quotas. | ||
| MaxResults int `yaml:"max-results,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether virustotal.go defines a defaultMaxPages / default nonzero cap independent of session.MaxResults
rg -n 'defaultMaxPages|MaxResults' pkg/subscraping/sources/virustotal/virustotal.goRepository: projectdiscovery/subfinder
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pkg/runner/options.go =="
sed -n '1,180p' pkg/runner/options.go | cat -n
echo
echo "== pkg/subscraping/sources/virustotal/virustotal.go =="
sed -n '1,220p' pkg/subscraping/sources/virustotal/virustotal.go | cat -n
echo
echo "== search for max-results flag/help/default wiring =="
rg -n 'max-results|MaxResults|defaultMaxPages|10 pages|400 subdomains|VirusTotal|virustotal' .Repository: projectdiscovery/subfinder
Length of output: 21313
Apply a nonzero default cap to VirusTotal pagination The flag still defaults to 0, and virustotal.Run uses session.MaxResults directly, so the default path remains unlimited. If the goal is to protect the free-tier quota out of the box, set a nonzero default for VirusTotal instead of relying on --max-results.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/runner/options.go` around lines 72 - 76, The VirusTotal pagination cap
still defaults to unlimited because MaxResults is left at 0 and virustotal.Run
reads session.MaxResults directly. Update the defaulting logic in the runner
options/session setup for MaxResults so VirusTotal gets a nonzero cap out of the
box, while keeping the explicit --max-results override working as expected. Use
the MaxResults field and virustotal.Run as the main points to adjust.
| // Honor an optional per-source result limit to avoid unnecessary | ||
| // pagination requests (e.g. to stay within API quotas). 0 = no limit. | ||
| maxResults := session.MaxResults | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether a non-zero default is set for MaxResults in runner options
rg -n -B3 -A3 'MaxResults' pkg/runner/options.go pkg/runner/validate.goRepository: projectdiscovery/subfinder
Length of output: 1871
Set a non-zero default for MaxResults. The cap here only applies when --max-results is set, but pkg/runner/options.go still defaults it to 0 (unlimited). As-is, standard runs will keep paginating VirusTotal and can still burn through quota.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/subscraping/sources/virustotal/virustotal.go` around lines 51 - 54, The
VirusTotal source currently only honors a result cap when MaxResults is
explicitly set, so standard runs still paginate indefinitely and can consume
quota. Update the default in pkg/runner/options.go to a non-zero MaxResults
value, and ensure virustotal.go’s maxResults handling in the VirusTotal scraping
flow continues to use session.MaxResults as the limit source.
3 participants
Closes #1718.
What the OP saw
Running
subfinder -nW -dL FILE -s virustotal -statsagainst ~110 hosts ran the VT free-tier 500-req/day quota dry after roughly twenty hosts. Every subsequent VT call returned HTTP 429, which the source surfaced asunexpected status code 429 received from ...with no indication that the free tier was the cause.The reason a single host can chew through so many requests is the cursor loop:
limit=40per page with no upper bound on pagination. A single popular root domain with thousands of subdomains will issue 50+ requests on its own.Fix
defaultMaxPages), giving up to 400 subdomains per domain on the default code path. That puts a single host at well under 1% of the daily quota even in the worst case.virustotal free-tier quota exhausted (HTTP 429); some subdomains for <domain> may be missingso the operator knows what happened and can either provide an enterprise key or accept the cap.Mirrors the pattern used in #1776 for netlas's Community-tier cap.
Tests
pkg/subscraping/sources/virustotal/virustotal_test.go(5 cases):Metadata/NoApiKeybaseline coveragePaginationCappedOnFreeTiermocks an endpoint that always returns a non-empty cursor and asserts the source stops at exactlydefaultMaxPagesrequestsStopsWhenCursorEmptyasserts the cursor terminator still wins when reached before the capRateLimitedReturnsActionableErrormocks a 429 response and asserts the new "quota exhausted" message is surfaced and pagination stops on the first 429Summary by CodeRabbit
Release Notes
--max-results/-mrto cap the number of results per source (use0for unlimited).--max-results/-mr.