fix(dns): prevent resolution loop when querying internal nameserver hostnames

[robindiddams]

When resolving a domain that matches an internal nameserver's hostname (e.g., querying for ns1.internal.example.com when that's configured in InternalNameservers), the resolver would attempt to resolve the nameserver using itself, creating an infinite loop.

This fix adds isInternalNameserver() to check if the query target matches any internal nameserver hostname. If so, selectNameservers() returns upstream nameservers instead, breaking the potential loop.

Summary by CodeRabbit

• Bug Fixes

  • Prevents DNS resolution loops when a nameserver’s hostname would otherwise be resolved via that same internal nameserver, ensuring the resolver falls back to upstream servers as appropriate.

• Tests

  • Added tests validating internal nameserver detection and that selection logic avoids resolution loops.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds isInternalNameserver to dns/server.go and updates selectNameservers to short-circuit and return upstream nameservers when the target equals an internal nameserver, preventing resolution loops.

Changes

Cohort / File(s)	Summary
DNS Nameserver Selection Logic dns/server.go	New isInternalNameserver(target string) bool helper (port stripping, IP filtering, case-normalized hostname comparison). selectNameservers now pre-checks the target with this helper and returns upstream nameservers when the target is an internal nameserver to avoid resolution loops.
Tests for Loop Avoidance dns/server_test.go	Added TestDNSResolver_isInternalNameserver and TestDNSResolver_selectNameservers_avoidsLoop validating internal-nameserver detection and that selectNameservers chooses upstreams instead of causing a resolution loop.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant DNSResolver
  participant InternalNS as Internal Nameserver(s)
  participant UpstreamNS as Upstream Nameserver(s)

  Client->>DNSResolver: request resolve(target)
  DNSResolver->>DNSResolver: isInternalNameserver(target)?
  alt target is internal
    DNSResolver-->>UpstreamNS: select Upstream Nameservers (avoid loop)
    DNSResolver-->>Client: return Upstream Nameservers
  else target not internal
    DNSResolver-->>InternalNS: apply normal internal-vs-upstream logic
    DNSResolver-->>Client: return chosen Nameservers
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I sniffed the nameserver trail, so neat,
A loop would catch my tiny feet.
I hop aside, choose upstream light,
DNS hops safe through day and night. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding a prevention mechanism for DNS resolution loops when querying internal nameserver hostnames.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/dns-internal-nameserver-loop

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e85104a and a9d38ab.

📒 Files selected for processing (1)

• dns/server.go

⏰ 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). (2)

• GitHub Check: build
• GitHub Check: Analyze (go)

🔇 Additional comments (1)

dns/server.go (1)

633-639: LGTM! Correct placement and logic for loop prevention.

The pre-check correctly intercepts queries for internal nameserver hostnames before the managed domain check. This ensures that even if an internal nameserver's hostname is within a managed domain, the resolver uses upstream nameservers to break the resolution loop.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

dns/server_test.go (1)

106-113: Strengthen validation to check all returned nameservers.

The current validation at line 108 only checks the first nameserver, which could miss cases where the implementation returns multiple nameservers or orders them differently. Consider adopting the more robust approach used in TestDNSResolver_selectNameservers_ManagedDomainCheck (lines 662-690), which verifies that all returned nameservers belong to the expected list.

🔎 More robust validation approach

 	for _, test := range tests {
 		ns := resolver.selectNameservers(test.target)
-		isUpstream := len(ns) > 0 && ns[0] == "127.0.0.11:53"
-		if isUpstream != test.expectUpstream {
-			t.Errorf("selectNameservers(%q): %s - got upstream=%v, want upstream=%v",
-				test.target, test.description, isUpstream, test.expectUpstream)
+		
+		// Verify all returned nameservers are from the expected source
+		for _, nameserver := range ns {
+			isUpstream := nameserver == "127.0.0.11:53"
+			isInternal := nameserver == "aether:53"
+			
+			if test.expectUpstream && !isUpstream {
+				t.Errorf("selectNameservers(%q): %s - expected upstream nameserver, got %s",
+					test.target, test.description, nameserver)
+			}
+			if !test.expectUpstream && !isInternal {
+				t.Errorf("selectNameservers(%q): %s - expected internal nameserver, got %s",
+					test.target, test.description, nameserver)
+			}
 		}
 	}

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9d38ab and 8a65b6a.

📒 Files selected for processing (2)

• dns/server.go
• dns/server_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• dns/server.go

🧰 Additional context used

🧬 Code graph analysis (1)

dns/server_test.go (3)

logger/test.go (1)

• NewTestLogger (123-127)

dns/config.go (1)

• DNSConfig (11-30)

dns/server.go (1)

• New (55-103)

⏰ 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: build

🔇 Additional comments (1)

dns/server_test.go (1)

46-80: Excellent test coverage for internal nameserver detection.

The test cases comprehensively cover the expected behavior including exact matches, trailing dots, case insensitivity, subdomain exclusion, IP address exclusion, and empty strings. The table-driven approach is clean and makes it easy to verify all edge cases.