Add automatic wildcard DNS detection and filtering via a new -aw/--auto-wildcard flag#947
Open
Aqil-Ahmad wants to merge 2 commits intoprojectdiscovery:devfrom
Open
Add automatic wildcard DNS detection and filtering via a new -aw/--auto-wildcard flag#947Aqil-Ahmad wants to merge 2 commits intoprojectdiscovery:devfrom
Aqil-Ahmad wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAuto-wildcard detection was added: a new CLI flag enables runtime discovery of wildcard DNS across multiple domains, a wildcard resolver and store were introduced, and the runner integrates detection to skip/filter wildcard-derived hosts during processing. Changes
Sequence DiagramsequenceDiagram
actor User
participant Runner as "Runner\n(runner.go)"
participant Input as "InputProcessor\n(prepareInput)"
participant Resolver as "WildcardResolver\n(pkg/wildcards)"
participant DNS as "DNS Server\n(external)"
User->>Runner: Start with --auto-wildcard
Runner->>Input: prepareInput(hosts)
Input->>Input: collect root domains
Input-->>Runner: processed hosts + domains
Runner->>Resolver: NewResolver(domains)
Resolver->>DNS: query wildcard permutations
DNS-->>Resolver: responses (IPs)
Resolver->>Resolver: cache wildcard IPs
loop per host
Runner->>Resolver: LookupHost(host, observedIP)
Resolver-->>Runner: isWildcard (bool), wildcardIPs
alt isWildcard
Runner->>Runner: increment wildcardFilteredCount
Runner-->>DNS: skip further probes for host
else not wildcard
Runner->>DNS: continue normal DNS probes
end
end
Runner-->>User: results + wildcard filter stats
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 6
🧹 Nitpick comments (5)
pkg/wildcards/resolver_test.go (1)
36-66: Integration test depends on external DNS infrastructure — will be flaky in CI.
Test_Resolver_LookupHostmakes live DNS queries to8.8.8.8and depends oncampaigns.google.combeing a wildcard. If network access is unavailable or Google changes their DNS configuration, this test will fail. Consider gating it behind a build tag (e.g.,//go:build integration) or atesting.Short()skip, and adding a mock-based unit test for the core logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/wildcards/resolver_test.go` around lines 36 - 66, Test_Resolver_LookupHost performs live DNS queries (via NewResolver and r.client.Lookup) and so is flaky in CI; modify the test to avoid external dependency by either adding a build tag (e.g., //go:build integration) so it only runs in integration builds or early-skipping it when testing.Short() is true, and add a new unit test that mocks Resolver.client.Lookup to exercise Resolver.LookupHost logic deterministically (create a fake client that returns controlled IPs and verify LookupHost behavior).pkg/wildcards/resolver.go (1)
107-118: Usingerrors.New("found domain")as a control-flow signal is fragile.If
Eachever starts inspecting/logging these errors, this pattern will produce misleading diagnostics. A dedicated sentinel (e.g.,var errFound = errors.New("found")) or abreak-supporting iteration pattern would be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/wildcards/resolver.go` around lines 107 - 118, The LookupHost function uses errors.New("found domain") as a control-flow signal inside w.Domains.Each; replace that ad-hoc error with a package-level sentinel (e.g., var errFound = errors.New("domain found")) and return errFound from the Each callback when the domain is matched, then after Each finishes check if the returned error equals errFound and ignore it (treat as normal early-exit) while propagating any other real errors; reference Resolver.LookupHost, w.Domains.Each, and the new errFound sentinel when making the change.internal/runner/runner.go (3)
499-519: Wildcard resolver is set up with a separate DNSX client — verify this is intentional.A new DNSX client is created specifically for wildcard resolution rather than reusing
r.dnsx. This doubles resolver connections but isolates wildcard probe traffic from main resolution. If intentional, this is fine. If not,NewResolverWithClient(r.autoWildcardDomains, r.dnsx)would be simpler and avoid extra resource usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner.go` around lines 499 - 519, The code creates a separate DNSX client for wildcard detection (using dnsx.DefaultOptions, dnsx.New and assigning to wcClient) instead of reusing the existing r.dnsx client; decide whether this is intentional and either (A) keep it but add a clear comment explaining why isolation is required (e.g., to separate probe traffic and retries/timeouts) or (B) reuse the existing resolver by passing r.dnsx into wildcards.NewResolverWithClient(r.autoWildcardDomains, r.dnsx) and remove the dnsx.DefaultOptions/dnsx.New block to avoid extra connections; update the code around r.options.AutoWildcard, r.autoWildcardDomains, NewResolverWithClient and r.wildcardResolver accordingly.
1032-1043: Auto-wildcard filtering only checks A records — AAAA wildcards will pass through undetected.
isAutoWildcarditerates overdnsData.Aonly, and the underlyingLookupHostin the resolver also only inspectsin.Aresponses. If a host resolves exclusively via AAAA, wildcard filtering won't apply. This matches the forcedTypeAaddition on line 126, but it's a known limitation worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner.go` around lines 1032 - 1043, isAutoWildcard currently only inspects IPv4 A records (dnsData.A) so AAAA-only wildcard responses bypass filtering; update isAutoWildcard to also iterate dnsData.AAAA and call r.wildcardResolver.LookupHost for each IPv6 address, and if the resolver's LookupHost or underlying logic only examines in.A responses, update it to also consider in.AAAA (or provide a separate lookup path) so both IPv4 and IPv6 wildcard detections are applied; reference isAutoWildcard, dnsData.A, dnsData.AAAA and wildcardResolver.LookupHost when making the changes.
809-815: Inline auto-wildcard check issues a DNS query per IP per host — could amplify request volume.Each resolved host triggers
LookupHost, which may issue random-subdomain DNS probes for each permutation level (until a cache hit or NXDOMAIN). For large input lists, this can significantly increase total DNS traffic beyond what the rate limiter accounts for, since the wildcard resolver uses its own DNSX client. Consider documenting this behavior or accounting for these probes in rate-limit calculations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner.go` around lines 809 - 815, The inline auto-wildcard check currently calls r.isAutoWildcard (which uses LookupHost via the wildcard resolver's DNSX client) per resolved host/IP, causing extra DNS probes outside the main rate limiter; update the logic so that wildcard detection is either memoized per domain (cache results in a map keyed by domain and consult before calling isAutoWildcard) or re-use the main rate-limited DNS client instead of the wildcardResolver's separate client, and ensure any DNS calls from isAutoWildcard are passed through the same rate-limiter used by the runner; adjust r.isAutoWildcard, r.wildcardResolver usage, and where LookupHost is invoked to implement caching or shared client so per-IP/stub probes stop amplifying request volume.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runner/runner.go`:
- Around line 292-304: The code currently adds every normalized item from sc
into awDomainSet when r.options.AutoWildcard and r.options.Domains are set,
which wrongly includes FUZZ patterns; update the loop in the runner (where
awDomainSet, normalize, sc are used) to skip entries containing the literal
"FUZZ" (or other placeholder tokens used for bruteforce) before inserting into
awDomainSet so only real root domains are collected for auto-wildcard detection;
i.e., check strings.Contains(item, "FUZZ") (or your placeholder list) and
continue the loop if true, otherwise add item to awDomainSet.
In `@pkg/wildcards/resolver_test.go`:
- Line 65: Remove the debug fmt.Printf call that prints the variable wildcards
in the test; either delete the line fmt.Printf("wildcards: %v\n", wildcards) or
replace it with t.Logf("wildcards: %v", wildcards) so the output is only shown
via the testing framework (use the test's *testing.T instance `t`).
In `@pkg/wildcards/resolver.go`:
- Around line 179-188: The code has a TOCTOU race when initializing
wildcardAnswersCache entries: if !cachedValueOk two goroutines can each create a
new cachedValue.IPS and Set it, losing one goroutine's additions; update the
logic around wildcardAnswersCache, cachedValue and IPS to atomically
get-or-create the entry instead of blind Set — e.g., implement a GetOrCreate or
LoadOrStore pattern for wildcardAnswersCache keyed by original, or use a per-key
mutex to ensure only one goroutine initializes cachedValue.IPS (check after
creating whether another goroutine stored a value and merge into that existing
IPS or reuse it), then populate IPS and call Get/Set/GetSyncLockMapValues
(referencing wildcardAnswersCache, cachedValue, IPS, mapsutil.NewSyncLockMap,
Set, Get, and getSyncLockMapValues) so initialization and population are atomic
and no entries are overwritten.
- Around line 199-211: GetAllWildcardIPs is reading value.IPS.Map directly which
bypasses SyncLockMap's concurrency protection and causes a data race; update
Resolver.GetAllWildcardIPs to iterate the thread-safe map via the SyncLockMap
API (e.g., call value.IPS.Iterate or reuse the existing getSyncLockMapValues
helper) instead of accessing the underlying Map field, and collect IPs into ips
using those safe accessors when walking wildcardAnswersCache entries
(referencing wildcardAnswersCache, wildcardAnswerCacheValue, and value.IPS).
In `@pkg/wildcards/store.go`:
- Around line 80-87: The LoadFromFile loop currently ignores scanner.Err(),
allowing read errors to be swallowed and returning partial data; after the for
scanner.Scan() loop in pkg/wildcards/store.go (the code that calls
s.wildcards.Set(item, struct{}{})), call scanner.Err() and if non-nil return
that error (or wrap it for context) so file read failures surface instead of
returning nil.
In `@pkg/wildcards/util.go`:
- Around line 17-26: The loop that builds the servers slice fails to check
scanner.Err() after scanning and doesn't sanitize lines; update the scanner loop
to TrimSpace each scanned line (use strings.TrimSpace), skip if trimmed line is
empty, and only append ":53" when the trimmed line does not already include a
port (e.g., check strings.Contains(trimmed, ":") or use net.SplitHostPort).
After the loop, call scanner.Err() and return that error if non-nil so partial
results aren't silently accepted. Add the "strings" import (and net if you
choose SplitHostPort) and update uses of scanner/Text/servers accordingly.
---
Nitpick comments:
In `@internal/runner/runner.go`:
- Around line 499-519: The code creates a separate DNSX client for wildcard
detection (using dnsx.DefaultOptions, dnsx.New and assigning to wcClient)
instead of reusing the existing r.dnsx client; decide whether this is
intentional and either (A) keep it but add a clear comment explaining why
isolation is required (e.g., to separate probe traffic and retries/timeouts) or
(B) reuse the existing resolver by passing r.dnsx into
wildcards.NewResolverWithClient(r.autoWildcardDomains, r.dnsx) and remove the
dnsx.DefaultOptions/dnsx.New block to avoid extra connections; update the code
around r.options.AutoWildcard, r.autoWildcardDomains, NewResolverWithClient and
r.wildcardResolver accordingly.
- Around line 1032-1043: isAutoWildcard currently only inspects IPv4 A records
(dnsData.A) so AAAA-only wildcard responses bypass filtering; update
isAutoWildcard to also iterate dnsData.AAAA and call
r.wildcardResolver.LookupHost for each IPv6 address, and if the resolver's
LookupHost or underlying logic only examines in.A responses, update it to also
consider in.AAAA (or provide a separate lookup path) so both IPv4 and IPv6
wildcard detections are applied; reference isAutoWildcard, dnsData.A,
dnsData.AAAA and wildcardResolver.LookupHost when making the changes.
- Around line 809-815: The inline auto-wildcard check currently calls
r.isAutoWildcard (which uses LookupHost via the wildcard resolver's DNSX client)
per resolved host/IP, causing extra DNS probes outside the main rate limiter;
update the logic so that wildcard detection is either memoized per domain (cache
results in a map keyed by domain and consult before calling isAutoWildcard) or
re-use the main rate-limited DNS client instead of the wildcardResolver's
separate client, and ensure any DNS calls from isAutoWildcard are passed through
the same rate-limiter used by the runner; adjust r.isAutoWildcard,
r.wildcardResolver usage, and where LookupHost is invoked to implement caching
or shared client so per-IP/stub probes stop amplifying request volume.
In `@pkg/wildcards/resolver_test.go`:
- Around line 36-66: Test_Resolver_LookupHost performs live DNS queries (via
NewResolver and r.client.Lookup) and so is flaky in CI; modify the test to avoid
external dependency by either adding a build tag (e.g., //go:build integration)
so it only runs in integration builds or early-skipping it when testing.Short()
is true, and add a new unit test that mocks Resolver.client.Lookup to exercise
Resolver.LookupHost logic deterministically (create a fake client that returns
controlled IPs and verify LookupHost behavior).
In `@pkg/wildcards/resolver.go`:
- Around line 107-118: The LookupHost function uses errors.New("found domain")
as a control-flow signal inside w.Domains.Each; replace that ad-hoc error with a
package-level sentinel (e.g., var errFound = errors.New("domain found")) and
return errFound from the Each callback when the domain is matched, then after
Each finishes check if the returned error equals errFound and ignore it (treat
as normal early-exit) while propagating any other real errors; reference
Resolver.LookupHost, w.Domains.Each, and the new errFound sentinel when making
the change.
Author
|
FYI, this is the only pr i currently have open. |
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 #924
/claim #924
Proposed changes
Instead of re-implementing wildcard logic, this imports and extends the stable wildcard resolver from
https://github.com/projectdiscovery/shuffledns/tree/dev/pkg/wildcards into pkg/wildcards/.
When enabled:
wildcard IPs, with caching for performance
Checklist
Summary by CodeRabbit