fix: DNS and SMTP drop interactions when nonce lengths differ

[jentfoo]

DNS and SMTP accumulated sliding-window matches in a single variable (last match wins) and stored only once after the loop. This silently dropped valid interactions when the server's correlation-id-nonce-length was shorter than a client's.

This change updates both to dispatch per match inside the loop, mirroring the working HTTP/LDAP pattern. SMTP also gains the missing SlideWithLength and ToLower normalization.

Summary by CodeRabbit

• Tests

  • Added unit tests to validate correlation ID detection, including exact, short, and embedded-ID scenarios.

• Refactor

  • Centralized interaction storage for DNS and SMTP into reusable helpers.
  • Improved correlation ID detection by using a sliding-window approach and ensured interactions are consistently stored when IDs are found.

[neo-by-projectdiscovery-dev]

Neo - PR Security Review

No security issues found

Highlights

• Fixes bug where DNS and SMTP dropped valid interactions when correlation ID nonce lengths differed
• Updates both protocols to dispatch per match inside loop, mirroring HTTP/LDAP pattern
• Adds SlideWithLength and ToLower normalization to SMTP for consistency
• Includes comprehensive tests validating correlation ID detection with sliding windows

_{Comment @pdneo help for available commands. · Open in Neo}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9f8fe0c1-dae9-4210-af20-c2ac36a84a62

📥 Commits

Reviewing files that changed from the base of the PR and between 7fc0b1f and f97c742.

📒 Files selected for processing (1)

• pkg/server/server_test.go

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

• pkg/server/server_test.go

---

Walkthrough

The changes extract and centralize interaction encoding/storage into new storeInteraction helpers for DNS and SMTP servers, update code paths to use chunked correlation-ID detection, and add tests validating correlation ID detection including sliding-window cases.

Changes

Cohort / File(s)	Summary
DNS Server Refactoring pkg/server/dns_server.go	Adds storeInteraction(uniqueID, fullID, qtype, requestMsg, responseMsg, remoteAddr) to centralize JSON encoding and Storage.AddInteraction calls; replaces multiple inlined Interaction constructions and storage calls across correlation-ID scanning and root-TLD branches.
SMTP Server Refactoring pkg/server/smtp_server.go	Adds (*SMTPServer).storeInteraction(uniqueID, fullID, dataString, from, remoteAddr) and refactors defaultHandler to use sliding-window chunk extraction (via stringsutil.SlideWithLength) to find correlation IDs, delegating storage to the new helper.
Correlation ID Tests pkg/server/server_test.go	Adds TestIsCorrelationID with subtests for exact-length match, undersized input rejection, and detection of embedded IDs using sliding-window checks; introduces new imports (strings, stringsutil, xid).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through code with keen delight,
Found IDs hidden out of sight,
I packed them up and stored them neat,
DNS and SMTP — tidy and sweet,
Tests cheer as data meets the light.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ 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%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main fix: addressing a bug where DNS and SMTP incorrectly drop interactions when correlation ID nonce lengths differ from expected values.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

Tip

CodeRabbit can generate a title for your PR based on the changes.

Add @coderabbitai placeholder anywhere in the title of your PR and CodeRabbit will replace it with a title based on the changes in the PR. You can change the placeholder by changing the reviews.auto_title_placeholder setting.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

pkg/server/smtp_server.go (1)

83-85: Add a length guard before slicing uniqueID.

This helper assumes validated input; adding a guard prevents accidental panic if another call path is introduced later.

🛡️ Proposed guard

 func (h *SMTPServer) storeInteraction(uniqueID, fullID, dataString, from, remoteAddr string) {
+	if len(uniqueID) < h.options.CorrelationIdLength {
+		gologger.Warning().Msgf("Skipping smtp interaction with short uniqueID: %q", uniqueID)
+		return
+	}
 	correlationID := uniqueID[:h.options.CorrelationIdLength]
 	interaction := &Interaction{

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/server/smtp_server.go` around lines 83 - 85, The storeInteraction helper
slices uniqueID into correlationID without checking length, which can panic; add
a guard in SMTPServer.storeInteraction that verifies len(uniqueID) >=
h.options.CorrelationIdLength before slicing and, if the check fails, fall back
to a safe value (e.g., use uniqueID as the correlationID or an empty string) or
early-return; reference the unique symbols uniqueID,
h.options.CorrelationIdLength and the function storeInteraction when making the
change so the slice operation is protected.

pkg/server/dns_server.go (1)

302-304: Harden helper against future short-input panics.

uniqueID[:h.options.CorrelationIdLength] will panic if a future caller passes malformed input. Current callers are safe, but this helper is a good place for a defensive guard.

🛡️ Proposed guard

 func (h *DNSServer) storeInteraction(uniqueID, fullID, qtype, requestMsg, responseMsg, remoteAddr string) {
+	if len(uniqueID) < h.options.CorrelationIdLength {
+		gologger.Warning().Msgf("Skipping dns interaction with short uniqueID: %q", uniqueID)
+		return
+	}
 	correlationID := uniqueID[:h.options.CorrelationIdLength]
 	interaction := &Interaction{

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/server/dns_server.go` around lines 302 - 304, The slice operation
uniqueID[:h.options.CorrelationIdLength] in DNSServer.storeInteraction can panic
if uniqueID is shorter than h.options.CorrelationIdLength; add a defensive guard
in storeInteraction that checks len(uniqueID) and uses a safe fallback (e.g.,
use uniqueID itself or an empty string) when it's too short, then proceed to
build the Interaction normally (referencing DNSServer.storeInteraction,
Interaction struct, and h.options.CorrelationIdLength); optionally emit a
debug/warn log when the fallback is used to aid future debugging.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/server/server_test.go`:
- Around line 21-33: The tests create IDs that don't match the configured length
rules, causing the sliding-window case to pass on generic alphanumerics rather
than an actual embedded correlation ID; update the test fixtures so they use
Options.GetIdLength() (or compute expected total length from
Options.CorrelationIdLength and Options.CorrelationIdNonceLength) when building
id strings, and construct the "longer" string so the nonce differs but the
embedded xid segment remains exactly the expected CorrelationIdLength;
specifically adjust the "exact length match" and "sliding window finds embedded
ID" cases to derive lengths from Options (or call GetIdLength()) and ensure
Options.isCorrelationID is exercised with an actual valid xid substring rather
than a random alphanumeric window.

---

Nitpick comments:
In `@pkg/server/dns_server.go`:
- Around line 302-304: The slice operation
uniqueID[:h.options.CorrelationIdLength] in DNSServer.storeInteraction can panic
if uniqueID is shorter than h.options.CorrelationIdLength; add a defensive guard
in storeInteraction that checks len(uniqueID) and uses a safe fallback (e.g.,
use uniqueID itself or an empty string) when it's too short, then proceed to
build the Interaction normally (referencing DNSServer.storeInteraction,
Interaction struct, and h.options.CorrelationIdLength); optionally emit a
debug/warn log when the fallback is used to aid future debugging.

In `@pkg/server/smtp_server.go`:
- Around line 83-85: The storeInteraction helper slices uniqueID into
correlationID without checking length, which can panic; add a guard in
SMTPServer.storeInteraction that verifies len(uniqueID) >=
h.options.CorrelationIdLength before slicing and, if the check fails, fall back
to a safe value (e.g., use uniqueID as the correlationID or an empty string) or
early-return; reference the unique symbols uniqueID,
h.options.CorrelationIdLength and the function storeInteraction when making the
change so the slice operation is protected.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c75cb439-e1ac-430f-9592-c3d3629118cd

📥 Commits

Reviewing files that changed from the base of the PR and between 810180a and 7fc0b1f.

📒 Files selected for processing (3)

• pkg/server/dns_server.go
• pkg/server/server_test.go
• pkg/server/smtp_server.go

[jentfoo]

This would be a valuable fix, allowing servers to configure a lower nonce length to allow for client flexibility. However if willing to accept a larger change I just submitted #1347 which I believe is a more complete and flexible solution.