Feat: Add IP to results in DNS protocol #6563
Conversation
WalkthroughThe DNS protocol now resolves and propagates IP addresses through request execution, DSL mapping, and result events. A new internal resolver helper was added and tests were updated to pass and assert the propagated IP value. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Request Handler
participant Resolver as tryToResolveHost
participant Executor as execute()
participant Operators as responseToDSLMap
participant DSL as DSL Event Map
rect rgb(240,248,255)
Client->>Resolver: ask for IP (domain or input)
Resolver-->>Client: returns ip (or error)
end
rect rgb(245,255,240)
Client->>Executor: execute(domain, ip, ...)
Executor->>Operators: responseToDSLMap(req, resp, host, ip, matched)
Operators->>DSL: inject fields (host, matched, ip, records...)
DSL-->>Operators: DSL event with ip field
Operators-->>Executor: return event
Executor-->>Client: final ResultEvent (includes IP)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/protocols/dns/request.go (1)
186-193: Use the same dnsClient instance for Trace as for Do()You build/use a local dnsClient for requests, but Trace calls request.dnsClient. This can diverge or be nil.
- traceData, err = request.dnsClient.Trace(domain, request.question, request.TraceMaxRecursion) + traceData, err = dnsClient.Trace(domain, request.question, request.TraceMaxRecursion)
🧹 Nitpick comments (8)
pkg/protocols/dns/utils.go (2)
20-24: Deterministic selection when multiple A/AAAA are presentUsing the “first” element can be nondeterministic across resolvers. Optionally sort before selection to stabilize results (especially useful for tests).
- if ips.A != nil && len(ips.A) > 0 { - return ips.A[0], nil - } + if n := len(ips.A); n > 0 { + sort.Strings(ips.A) + return ips.A[0], nil + } - if ips.AAAA != nil && len(ips.AAAA) > 0 { - return ips.AAAA[0], nil - } + if n := len(ips.AAAA); n > 0 { + sort.Strings(ips.AAAA) + return ips.AAAA[0], nil + }(Remember to add import "sort".)
8-8: Exported sentinel should have a doc commentAdd a brief GoDoc so it shows up in package docs.
-var IPNotFoundError = errors.New("no A or AAAA record found") +// IPNotFoundError is returned when neither A nor AAAA records are found. +var IPNotFoundError = errors.New("no A or AAAA record found")pkg/protocols/dns/operators.go (1)
84-101: Include “ip” only when available (optional)To keep events compact, consider setting "ip" only when non‑empty. Current tests expect the key; do this only if you also update tests.
ret := output.InternalEvent{ "host": host, - "ip": ip, + } + if ip != "" { + ret["ip"] = ip + } + for k, v := range map[string]interface{}{ + "matched": matched, + "request": req.String(), + "rcode": resp.Rcode, + "question": questionToString(resp.Question), + "extra": rrToString(resp.Extra), + "answer": rrToString(resp.Answer), + "ns": rrToString(resp.Ns), + "raw": resp.String(), + "template-id": request.options.TemplateID, + "template-info": request.options.TemplateInfo, + "template-path": request.options.TemplatePath, + "type": request.Type().String(), + "trace": traceToString(traceData, false), + } { + ret[k] = v }pkg/protocols/dns/operators_test.go (2)
48-51: Strengthen assertions: check “ip” explicitly; avoid brittle length checksAssert the "ip" field value directly and drop reliance on total map size which changes when event shape evolves.
- event := request.responseToDSLMap(req, resp, "one.one.one.one", "1.1.1.1", "one.one.one.one", nil) - require.Len(t, event, 16, "could not get correct number of items in dsl map") + event := request.responseToDSLMap(req, resp, "one.one.one.one", "1.1.1.1", "one.one.one.one", nil) + require.Equal(t, "1.1.1.1", event["ip"], "ip must be propagated")
82-92: Add an assertion for “ip” in each scenarioEach test constructs the DSL map with an explicit IP; assert it’s present to guard regressions.
Example:
event := request.responseToDSLMap(req, resp, "one.one.one.one", "1.1.1.1", "one.one.one.one", nil) +require.Equal(t, "1.1.1.1", event["ip"])Apply similarly in other test blocks.
Also applies to: 149-163, 194-205, 266-275
pkg/protocols/dns/utils_tests.go (3)
12-18: Avoid hard dependency on public resolvers in unit testsTests hitting 1.1.1.1:53 can flake in CI. Prefer a stub/mocked resolver, an integration-test build tag, or gate by env var.
Example gating:
func newTestResolver(t *testing.T) *retryabledns.Client { + if os.Getenv("NUCLEI_ENABLE_DNS_NET_TESTS") == "" { + t.Skip("networked DNS tests disabled (set NUCLEI_ENABLE_DNS_NET_TESTS=1 to enable)") + }(Remember import "os".)
35-46: Use a more stable AAAA targetipv6.google.com has changed historically. Consider one.one.one.one which has well-known AAAA records.
- ip, err := tryToResolveHost("ipv6.google.com", resolver) + ip, err := tryToResolveHost("one.one.one.one", resolver)
67-75: Prefer error return over panic for nil resolverPanic-based testing couples callers to crash behavior. If you adopt a nil guard in tryToResolveHost, update this test to assert an error instead.
-func TestTryToResolveHost_NilResolver(t *testing.T) { - defer func() { - if r := recover(); r == nil { - t.Fatal("expected panic with nil resolver, but did not panic") - } - }() - _, _ = tryToResolveHost("example.com", nil) - require.Fail(t, "expected panic due to nil resolver") -} +func TestTryToResolveHost_NilResolver(t *testing.T) { + _, err := tryToResolveHost("example.com", nil) + require.Error(t, err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/protocols/dns/operators.go(2 hunks)pkg/protocols/dns/operators_test.go(5 hunks)pkg/protocols/dns/request.go(5 hunks)pkg/protocols/dns/utils.go(1 hunks)pkg/protocols/dns/utils_tests.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/protocols/dns/utils.gopkg/protocols/dns/operators_test.gopkg/protocols/dns/request.gopkg/protocols/dns/utils_tests.gopkg/protocols/dns/operators.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()
Files:
pkg/protocols/dns/utils.gopkg/protocols/dns/operators_test.gopkg/protocols/dns/request.gopkg/protocols/dns/utils_tests.gopkg/protocols/dns/operators.go
🧬 Code graph analysis (3)
pkg/protocols/dns/request.go (4)
pkg/protocols/common/contextargs/metainput.go (1)
MetaInput(18-30)pkg/protocols/dns/dns.go (1)
Request(20-99)pkg/protocols/common/contextargs/contextargs.go (1)
Context(22-33)pkg/output/output.go (1)
InternalEvent(93-93)
pkg/protocols/dns/utils_tests.go (1)
pkg/protocols/dns/utils.go (1)
IPNotFoundError(8-8)
pkg/protocols/dns/operators.go (2)
pkg/protocols/protocols.go (1)
Request(308-333)pkg/output/output.go (1)
InternalEvent(93-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (2)
pkg/protocols/dns/request.go (1)
195-205: responseToDSLMap propagation looks goodIP is now propagated into the event and downstream. No issues spotted here.
pkg/protocols/dns/operators.go (1)
114-131: No issues found—IP field is correctly definedThe ResultEvent struct in pkg/output/output.go (line 187) includes
IP stringwith the JSON tagjson:"ip,omitempty". The code in pkg/protocols/dns/operators.go correctly assigns this field.
| func tryToResolveHost(domain string, resolver *retryabledns.Client) (string, error) { | ||
| // resolve is recursive so CNAMEs are resolved automatically | ||
| ips, err := resolver.Resolve(domain) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| // It returns first A or AAAA record found in DNS chain | ||
| // This means that domains with multiple A records will return the first one only, also ipv4 is preferred over ipv6 | ||
| if ips.A != nil && len(ips.A) > 0 { | ||
| return ips.A[0], nil | ||
| } | ||
| if ips.AAAA != nil && len(ips.AAAA) > 0 { | ||
| return ips.AAAA[0], nil | ||
| } | ||
| return "", IPNotFoundError |
There was a problem hiding this comment.
Guard against nil resolver to avoid panics in callers
tryToResolveHost assumes resolver is non‑nil. A nil resolver will panic at runtime. Either validate at call site or guard here.
Minimal defensive fix:
func tryToResolveHost(domain string, resolver *retryabledns.Client) (string, error) {
+ if resolver == nil {
+ return "", errors.New("nil resolver")
+ }
// resolve is recursive so CNAMEs are resolved automatically
ips, err := resolver.Resolve(domain)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func tryToResolveHost(domain string, resolver *retryabledns.Client) (string, error) { | |
| // resolve is recursive so CNAMEs are resolved automatically | |
| ips, err := resolver.Resolve(domain) | |
| if err != nil { | |
| return "", err | |
| } | |
| // It returns first A or AAAA record found in DNS chain | |
| // This means that domains with multiple A records will return the first one only, also ipv4 is preferred over ipv6 | |
| if ips.A != nil && len(ips.A) > 0 { | |
| return ips.A[0], nil | |
| } | |
| if ips.AAAA != nil && len(ips.AAAA) > 0 { | |
| return ips.AAAA[0], nil | |
| } | |
| return "", IPNotFoundError | |
| func tryToResolveHost(domain string, resolver *retryabledns.Client) (string, error) { | |
| if resolver == nil { | |
| return "", errors.New("nil resolver") | |
| } | |
| // resolve is recursive so CNAMEs are resolved automatically | |
| ips, err := resolver.Resolve(domain) | |
| if err != nil { | |
| return "", err | |
| } | |
| // It returns first A or AAAA record found in DNS chain | |
| // This means that domains with multiple A records will return the first one only, also ipv4 is preferred over ipv6 | |
| if ips.A != nil && len(ips.A) > 0 { | |
| return ips.A[0], nil | |
| } | |
| if ips.AAAA != nil && len(ips.AAAA) > 0 { | |
| return ips.AAAA[0], nil | |
| } | |
| return "", IPNotFoundError |
🤖 Prompt for AI Agents
In pkg/protocols/dns/utils.go around lines 12 to 26, guard against a nil
resolver to avoid panics: at the start of tryToResolveHost check if resolver ==
nil and return a clear error (e.g., a new ErrNilResolver or a wrapped error with
context) instead of proceeding; keep the rest of the logic unchanged so callers
receive an explicit error rather than causing a runtime panic.
There was a problem hiding this comment.
should not happen
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites of tryToResolveHost to verify resolver is never nil
rg -n -A5 -B5 'tryToResolveHost\(' --type=go
Length of output: 4095
@matejsmycka You're right! I can see the only production call site in pkg/protocols/dns/request.go (line 63-65) guards with else if request.dnsClient != nil before calling tryToResolveHost, so the resolver will never be nil in practice. Since this is a private function with a single protected call site, the nil check would be redundant. Thanks for the clarification!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/protocols/dns/utils_test.go (5)
20-32: Simplify by removing redundant error checks.Lines 24-29 duplicate the validation performed by
requireassertions on lines 30-31. Usingrequireconsistently throughout would be cleaner.Apply this diff to simplify:
func TestTryToResolveHost_SuccessARecord(t *testing.T) { resolver := newTestResolver(t) ip, err := tryToResolveHost("example.com", resolver) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - if ip == "" { - t.Fatal("expected non-empty IP for example.com") - } + require.NoError(t, err) require.NotEmpty(t, ip) require.Contains(t, ip, ".") }
34-46: Simplify by removing redundant error checks.Same pattern as the previous test—manual checks duplicate the
requireassertions.Apply this diff:
func TestTryToResolveHost_SuccessAAAARecord(t *testing.T) { resolver := newTestResolver(t) ip, err := tryToResolveHost("ipv6.google.com", resolver) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - if ip == "" { - t.Fatal("expected non-empty IPv6 address for ipv6.google.com") - } + require.NoError(t, err) require.NotEmpty(t, ip) require.Contains(t, ip, ":") }
48-55: Clarify error assertion logic.The condition on line 52 is confusing. Since the test expects either
IPNotFoundErroror any DNS error, simply asserting that an error occurred would be clearer.Apply this diff:
func TestTryToResolveHost_IPNotFound(t *testing.T) { resolver := newTestResolver(t) _, err := tryToResolveHost("nonexistent-subdomain.ef37979f-9fff-43f8-b267-822108d4291c.com", resolver) - if !errors.Is(err, IPNotFoundError) && err == nil { - t.Fatalf("expected IPNotFoundError or DNS error, got: %v", err) - } + require.Error(t, err, "expected IPNotFoundError or DNS error for non-existent domain") }
57-65: Remove redundant error check.Lines 61-63 duplicate the
require.Errorassertion on line 64.Apply this diff:
func TestTryToResolveHost_InvalidDomain(t *testing.T) { resolver := newTestResolver(t) _, err := tryToResolveHost("invalid_domain_###", resolver) - if err == nil { - t.Fatal("expected error for invalid domain name, got nil") - } require.Error(t, err) }
67-75: Remove unreachable code.Line 74 is unreachable: if the panic occurs (expected path), execution never reaches line 74; if no panic occurs, the deferred function fails the test at line 70 before line 74 can execute.
Apply this diff:
func TestTryToResolveHost_NilResolver(t *testing.T) { defer func() { if r := recover(); r == nil { t.Fatal("expected panic with nil resolver, but did not panic") } }() _, _ = tryToResolveHost("example.com", nil) - require.Fail(t, "expected panic due to nil resolver") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/protocols/dns/utils.go(1 hunks)pkg/protocols/dns/utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/protocols/dns/utils.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/protocols/dns/utils_test.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()
Files:
pkg/protocols/dns/utils_test.go
🧬 Code graph analysis (1)
pkg/protocols/dns/utils_test.go (1)
pkg/protocols/dns/utils.go (1)
IPNotFoundError(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests (ubuntu-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (macOS-latest)
🔇 Additional comments (2)
pkg/protocols/dns/utils_test.go (2)
1-9: LGTM!Package declaration and imports are appropriate for the test functionality.
11-18: External DNS dependency noted.The helper uses Cloudflare's public DNS server (1.1.1.1:53), creating an external dependency that could make tests flaky in restricted network environments. This is acceptable for DNS protocol testing but be aware tests may fail without internet access.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/protocols/dns/utils_test.go (1)
37-42: Consider checking for specific error type.The test validates error handling but doesn't verify that the returned error is specifically
IPNotFoundError(mentioned in the AI summary). While checking for any error is acceptable for this "best effort" approach, asserting the specific error type would provide stronger guarantees.If you want to verify the specific error type, you could update the test:
func TestTryToResolveHost_IPNotFound(t *testing.T) { resolver := newTestResolver(t) _, err := tryToResolveHost("nonexistent-subdomain.ef37979f-9fff-43f8-b267-822108d4291c.com", resolver) - require.Error(t, err, "expected IPNotFoundError or DNS error for non-existent domain") + require.Error(t, err) + require.ErrorIs(t, err, IPNotFoundError, "expected IPNotFoundError for non-existent domain") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/protocols/dns/request.go(4 hunks)pkg/protocols/dns/utils_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/protocols/dns/utils_test.gopkg/protocols/dns/request.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()
Files:
pkg/protocols/dns/utils_test.gopkg/protocols/dns/request.go
🧬 Code graph analysis (1)
pkg/protocols/dns/request.go (4)
pkg/protocols/common/contextargs/metainput.go (1)
MetaInput(18-30)pkg/protocols/dns/dns.go (1)
Request(20-99)pkg/protocols/common/contextargs/contextargs.go (1)
Context(22-33)pkg/output/output.go (1)
InternalEvent(93-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests (windows-latest)
🔇 Additional comments (9)
pkg/protocols/dns/utils_test.go (5)
11-17: LGTM! Helper function is well-structured.The test resolver helper properly initializes the DNS client with appropriate error handling.
19-26: Test coverage for A record resolution looks good.The test appropriately validates successful IPv4 resolution. Note that this test has an external dependency on DNS infrastructure and example.com availability, which is acceptable for integration testing but may occasionally be flaky in restricted network environments.
44-49: LGTM! Invalid domain test is appropriate.The test correctly validates error handling for malformed domain names.
51-58: Design relies on implicit nil contracts via defensive caller checks rather than explicit validation.Verification confirms defensive nil-checks exist in
request.go(line 63) and DO guard before callingtryToResolveHost. The panic behavior occurs via nil pointer dereference atresolver.Resolve()(line 14 ofutils.go), not explicit validation. The function has no internal nil guard—it delegates this responsibility entirely to callers.While this pattern works (all production calls are guarded), it's implicit rather than explicit. Consider whether the function should include an explicit nil check with a meaningful error message, or add documentation clarifying that
resolvermust never be nil. This would make the contract explicit rather than relying on caller discipline.
28-35: Document IPv6 requirement or add conditional skip for robustness.The test uses real DNS resolution via Cloudflare's 1.1.1.1 to query ipv6.google.com for AAAA records. It will fail in environments where IPv6 is disabled or unavailable. No skip conditions exist in the codebase to handle this. Either document that full test suite requires IPv6 support, or add a skip condition when IPv6 is unavailable.
pkg/protocols/dns/request.go (4)
59-69: IP resolution logic correctly implements "best effort" approach.The implementation properly handles the key scenarios:
- Direct IP passthrough when input is already an IP
- Defensive nil-check for
dnsClientbefore resolution (addressing previous review feedback)- Trailing dot normalization for resolver compatibility (addressing previous review feedback)
- Silent failure on resolution errors, leaving
ipemptyThe placement before the payload loop is correct since the
domainvariable is set from the original input and doesn't vary per payload iteration.
103-103: LGTM! IP parameter propagation is consistent and concurrency-safe.The
ipvariable is correctly passed toexecute()in both execution paths:
- Threaded payload execution (line 103): The
ipvariable is safely captured in the goroutine closure since it's read-only and set before the loop begins.- Non-payload execution (line 116): Direct call with the resolved IP.
This ensures consistent IP propagation regardless of whether payloads are used.
Also applies to: 116-116
121-121: Function signature update is clean and backward-compatible.The addition of the
ipparameter to the internalexecute()function is well-placed between related target parameters (domainandmetadata). Since this is an internal function, there's no public API impact, and all call sites have been properly updated.
193-193: IP successfully propagated to DSL mapping for matcher/extractor access.The updated call to
responseToDSLMapcompletes the IP propagation chain from resolution through to the output event. According to the related changes inoperators.go, this enables matchers and extractors to reference the resolved IP in templates.
|
@matejsmycka Thanks for your PR! Would it be possible to provide more context about the use case? In other protocols the |
Hi, we are using it for asset discovery, and we are using IP addresses as assets (for example, like the Shodan). This makes it easy to track back each finding to IP hosts. I could create my own resolver for Nuclei |
|
If this feature is not wanted, we will add a resolver on our side. |
2 participants
Proposed changes
This PR implements IP in the DNS findings. I see reasons why this was not implemented. There is no easy, perfect solution. However, I would go for a "best effort" solution, see changes.
If you have any recommendations that I should implement, please say so.
Checklist
Summary by CodeRabbit
New Features
Tests