Add more control over custom DNS domains#1288
Add more control over custom DNS domains#1288Vasco-jofra wants to merge 9 commits intoprojectdiscovery:devfrom
Conversation
…overy#1069) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.29.0 to 0.33.0. - [Commits](golang/net@v0.29.0...v0.33.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
interact.sh is no longer in use so updated docs to use one of the current domains
…y#1160) ZAP left OWASP over a year ago. Also there was a weird dangling image tag that seems to now 404, so I trimmed it.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis change restructures custom DNS records from a flat string map to a type-aware, structured format with support for multiple record types (A, AAAA, CNAME, MX, TXT, NS, ANY). The DNS server now prioritizes custom records over defaults, reads from both YAML and legacy formats, and applies domain-aware filtering during query resolution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Areas requiring extra attention:
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: 5
🧹 Nitpick comments (1)
pkg/server/dns_server.go (1)
569-578: MX priority 0 is conflated with "unset".Using
0as the sentinel for "use default priority" means users cannot explicitly set priority 0 (though this is rare in practice). If this becomes a requirement, consider using a pointer type*uint16forPriorityin the config struct to distinguish between unset and zero.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/interactsh-server/example-custom-records.yamlis excluded by!**/*.yaml
📒 Files selected for processing (1)
pkg/server/dns_server.go(8 hunks)
🔇 Additional comments (5)
pkg/server/dns_server.go (5)
56-56: LGTM!Constructor call correctly passes
options.Domainsto enable domain-aware subdomain extraction.
386-401: LGTM!The new type definitions are well-structured with appropriate YAML tags and optional fields marked with
omitempty.
411-428: LGTM!Initialization correctly handles the new domains parameter and converts default records to the structured format. Error handling for file reading is appropriate (log and continue with defaults).
430-472: LGTM!The dual-format parsing logic correctly prioritizes the structured format while maintaining backward compatibility with legacy configs. Required field validation ensures configuration errors are caught early.
474-542: LGTM!The zone matching and subdomain extraction logic is correct. Type filtering properly handles all supported record types including ANY queries.
| if len(customRecords) > 0 { | ||
| for _, record := range customRecords { | ||
| h.addCustomRecordToMessage(record, zone, m) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Error return from addCustomRecordToMessage is ignored.
If an unsupported record type is encountered or if IP parsing fails, the error will be silently discarded. Consider logging or handling the error.
if len(customRecords) > 0 {
for _, record := range customRecords {
- h.addCustomRecordToMessage(record, zone, m)
+ if err := h.addCustomRecordToMessage(record, zone, m); err != nil {
+ gologger.Warning().Msgf("Could not add custom DNS record: %s", err)
+ }
}
return
}📝 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.
| if len(customRecords) > 0 { | |
| for _, record := range customRecords { | |
| h.addCustomRecordToMessage(record, zone, m) | |
| } | |
| return | |
| } | |
| if len(customRecords) > 0 { | |
| for _, record := range customRecords { | |
| if err := h.addCustomRecordToMessage(record, zone, m); err != nil { | |
| gologger.Warning().Msgf("Could not add custom DNS record: %s", err) | |
| } | |
| } | |
| return | |
| } |
🤖 Prompt for AI Agents
In pkg/server/dns_server.go around lines 165–170, the loop calling
addCustomRecordToMessage ignores its error return; capture the error and handle
it instead of discarding it. Change the loop to check err :=
h.addCustomRecordToMessage(...); if err != nil { log the error with context
(record, zone, and the error) and continue to the next record, or return the
error upstream if that fits the function flow — ensure IP parse and
unsupported-type errors are surfaced via the log or returned so they are not
silently dropped.
| if len(customRecords) > 0 { | ||
| for _, record := range customRecords { | ||
| h.addCustomRecordToMessage(record, zone, m) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Same ignored error issue applies here.
For consistency, handle or log errors from addCustomRecordToMessage.
🤖 Prompt for AI Agents
In pkg/server/dns_server.go around lines 194 to 199, the loop calling
h.addCustomRecordToMessage ignores any error returned; update the loop to
capture the error from addCustomRecordToMessage and handle it consistently
(either return the error up the call chain or log it using the server/logger
with contextual details such as the record and zone). Ensure you include the
error in the log message and decide whether to continue processing remaining
records or abort by returning the error according to the function's
error-handling convention.
| if len(customRecords) > 0 { | ||
| for _, record := range customRecords { | ||
| h.addCustomRecordToMessage(record, zone, m) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Same ignored error issue applies here.
For consistency, handle or log errors from addCustomRecordToMessage.
🤖 Prompt for AI Agents
In pkg/server/dns_server.go around lines 215 to 220, the loop calling
h.addCustomRecordToMessage(record, zone, m) ignores its returned error; update
the loop to capture the error for each call and either log it (with context:
record and zone) or handle it appropriately (e.g., continue on non-fatal errors,
or return on fatal ones). Ensure the chosen logging uses the existing logger and
includes the error value so failures in addCustomRecordToMessage are not
silently ignored.
| if len(customRecords) > 0 { | ||
| for _, record := range customRecords { | ||
| h.addCustomRecordToMessage(record, zone, m) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Same ignored error issue applies here.
For consistency, handle or log errors from addCustomRecordToMessage.
🤖 Prompt for AI Agents
In pkg/server/dns_server.go around lines 251 to 256, the loop calling
h.addCustomRecordToMessage ignores returned errors; update the loop to capture
the error from addCustomRecordToMessage for each record and handle it
consistently (either log the error with context including the record and zone,
and continue, or return the error upstream if appropriate). Use the same
logging/error-handling pattern used elsewhere in this file (include zone and
record info) so errors aren't silently dropped.
| case "A": | ||
| m.Answer = append(m.Answer, &dns.A{ | ||
| Hdr: dns.RR_Header{Name: zone, Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: ttl}, | ||
| A: net.ParseIP(record.Value), | ||
| }) | ||
| case "AAAA": | ||
| m.Answer = append(m.Answer, &dns.AAAA{ | ||
| Hdr: dns.RR_Header{Name: zone, Rrtype: dns.TypeAAAA, Class: dns.ClassINET, Ttl: ttl}, | ||
| AAAA: net.ParseIP(record.Value), | ||
| }) |
There was a problem hiding this comment.
Missing IP address validation for A and AAAA records.
net.ParseIP returns nil for invalid IP strings, which would create malformed DNS responses with nil addresses. Validate the parsed IP before adding the record.
case "A":
+ ip := net.ParseIP(record.Value)
+ if ip == nil || ip.To4() == nil {
+ return fmt.Errorf("invalid IPv4 address for A record: %s", record.Value)
+ }
m.Answer = append(m.Answer, &dns.A{
Hdr: dns.RR_Header{Name: zone, Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: ttl},
- A: net.ParseIP(record.Value),
+ A: ip.To4(),
})
case "AAAA":
+ ip := net.ParseIP(record.Value)
+ if ip == nil || ip.To16() == nil {
+ return fmt.Errorf("invalid IPv6 address for AAAA record: %s", record.Value)
+ }
m.Answer = append(m.Answer, &dns.AAAA{
Hdr: dns.RR_Header{Name: zone, Rrtype: dns.TypeAAAA, Class: dns.ClassINET, Ttl: ttl},
- AAAA: net.ParseIP(record.Value),
+ AAAA: ip.To16(),
})📝 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.
| case "A": | |
| m.Answer = append(m.Answer, &dns.A{ | |
| Hdr: dns.RR_Header{Name: zone, Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: ttl}, | |
| A: net.ParseIP(record.Value), | |
| }) | |
| case "AAAA": | |
| m.Answer = append(m.Answer, &dns.AAAA{ | |
| Hdr: dns.RR_Header{Name: zone, Rrtype: dns.TypeAAAA, Class: dns.ClassINET, Ttl: ttl}, | |
| AAAA: net.ParseIP(record.Value), | |
| }) | |
| case "A": | |
| ip := net.ParseIP(record.Value) | |
| if ip == nil || ip.To4() == nil { | |
| return fmt.Errorf("invalid IPv4 address for A record: %s", record.Value) | |
| } | |
| m.Answer = append(m.Answer, &dns.A{ | |
| Hdr: dns.RR_Header{Name: zone, Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: ttl}, | |
| A: ip.To4(), | |
| }) | |
| case "AAAA": | |
| ip := net.ParseIP(record.Value) | |
| if ip == nil || ip.To16() == nil { | |
| return fmt.Errorf("invalid IPv6 address for AAAA record: %s", record.Value) | |
| } | |
| m.Answer = append(m.Answer, &dns.AAAA{ | |
| Hdr: dns.RR_Header{Name: zone, Rrtype: dns.TypeAAAA, Class: dns.ClassINET, Ttl: ttl}, | |
| AAAA: ip.To16(), | |
| }) |
🤖 Prompt for AI Agents
In pkg/server/dns_server.go around lines 554 to 563, the code appends A/AAAA
records using net.ParseIP(record.Value) without checking for nil; this can
produce malformed responses for invalid IPs. Parse the IP into a variable, check
if it's non-nil (and for A vs AAAA ensure correct 4/16-byte family if desired),
and only append the dns.A or dns.AAAA RR when validation passes; if parsing
fails, skip the record and log or return an error as appropriate to avoid adding
nil addresses to the response.
|
@coderabbitai create an issue for this |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12239 🏁 Script executed: Length of output: 10569
🏁 Script executed: Length of output: 58
Issue link: #1336 The issue documents:
The issue has been assigned to you as requested. |
@coderabbitai the issue was supposed to be for the PR not for unrsolved reviews. update the issue accordingly |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 58
Updated issue: #1336 The issue now describes:
|
Neo - PR Security ReviewNo security issues found Highlights
Hardening Notes
Comment |
7 participants
This PR adds more control over custom DNS records that can be served with interatsch.
It keeps support of the legacy format, as explained in the example YAML file.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.