feat: Support reduced nonce lengths and subdomain separation#1347
feat: Support reduced nonce lengths and subdomain separation#1347jentfoo wants to merge 14 commits intoprojectdiscovery:devfrom
Conversation
…ion-strategy feat(server) added eviction strategy
…-1275-feature/eviction-strategy Revert "feat(server) added eviction strategy"
interactsh v1.3.0
Bumps [github.com/refraction-networking/utls](https://github.com/refraction-networking/utls) from 1.8.0 to 1.8.2. - [Release notes](https://github.com/refraction-networking/utls/releases) - [Commits](refraction-networking/utls@v1.8.0...v1.8.2) --- updated-dependencies: - dependency-name: github.com/refraction-networking/utls dependency-version: 1.8.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
…abot/go_modules/github.com/refraction-networking/utls-1.8.2 chore(deps): bump github.com/refraction-networking/utls from 1.8.0 to 1.8.2
release interactsh v1.3.1
Simplify interaction storage to only retain the correlation ID, since the nonce is not needed for routing interactions to clients. The biggest concern in this change is accurately choosing the correlation id. IDs with suffixed nonces are preferred if available, but this change is willing to accept also isolated correlationIDs. This allows the correlation id and nonce to be split into different subdomain components (or orders).
|
🔧 Hit a snag — please try again. |
|
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 ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughImplements two-tier correlation-ID matching across DNS, HTTP, LDAP, and SMTP: prefer corrID+nonce within a label, fallback to bare corrID across label parts; introduces storeInteraction helpers, refactors handlers to accept matchedChunk, updates ID-length utilities, and adds tests for matching behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as "App Server\n(DNS/HTTP/LDAP/SMTP)"
participant Matcher as "ID Matching\nLogic"
participant Store as "Storage\nLayer"
Client->>Server: Send request (host/domain, payload)
Server->>Server: Extract host/parts (and qtype for DNS)
Server->>Matcher: Scan for corrID+nonce (within label)
alt corrID+nonce found
Matcher-->>Server: matchedChunk + fullID
else not found
Server->>Matcher: Scan for bare corrID (across parts)
Matcher-->>Server: matchedChunk + fullID or empty
end
alt matchedChunk non-empty
Server->>Server: Derive correlationID from matchedChunk
Server->>Store: storeInteraction(matchedChunk, fullID, payload,...)
Store-->>Server: Ack / error
else no match
Server->>Server: Log raw/legacy interaction
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
pkg/server/dns_server.go (2)
394-396: Redundant condition:i+1 <= len(parts)is always true.🔧 Proposed simplification
- if i+1 <= len(parts) { - fullID = strings.Join(parts[:i+1], ".") - } + fullID = strings.Join(parts[:i+1], ".")🤖 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 394 - 396, The condition `i+1 <= len(parts)` is redundant because slice upper bounds are always <= len(parts) in this loop; simplify by removing the if-check and directly set fullID = strings.Join(parts[:i+1], ".") where `fullID`, `parts`, and loop index `i` are used (in the same block in dns_server.go) so the assignment always runs without the unnecessary guard.
302-307: Consider adding a defensive check for emptymatchedChunk.Consistent with observations in HTTP and SMTP servers - adding a length guard would align with LDAP server's defensive approach.
🛡️ Proposed defensive check
func (h *DNSServer) storeInteraction(matchedChunk, fullID, qtype, requestMsg, responseMsg, remoteAddr string) { + if len(matchedChunk) < h.options.CorrelationIdLength { + return + } correlationID := matchedChunk[:h.options.CorrelationIdLength]🤖 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 - 307, Add a defensive length check at the start of DNSServer.storeInteraction to ensure matchedChunk is at least h.options.CorrelationIdLength bytes long before slicing; if len(matchedChunk) < h.options.CorrelationIdLength, avoid the slice panic by returning early (or set correlationID to matchedChunk/empty) and skip creating the Interaction that uses UniqueID = correlationID; update the function around the matchedChunk[:h.options.CorrelationIdLength] usage to reference this guard.pkg/server/http_server.go (2)
193-194: Redundant condition:i+1 <= len(parts)is always true.Same issue as in LDAP server - since
iranges from 0 tolen(parts)-1, the condition is always satisfied.🔧 Proposed simplification
- if i+1 <= len(parts) { - fullID = strings.Join(parts[:i+1], ".") - } + fullID = strings.Join(parts[:i+1], ".")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/http_server.go` around lines 193 - 194, The if-check "if i+1 <= len(parts)" is redundant because i iterates 0..len(parts)-1; remove the conditional branch and directly set fullID = strings.Join(parts[:i+1], ".") in the loop (where variables i, parts, and fullID are used) to simplify the code and match the LDAP server fix.
222-227: Consider adding a defensive check for emptymatchedChunk.Unlike
handleInteractionin LDAP server (which checksif matchedChunk != ""), this function directly slicesmatchedChunk[:h.options.CorrelationIdLength]without validation. While current call sites guaranteematchedChunkpassesisCorrelationID(ensuring sufficient length), adding a guard would improve robustness against future refactoring.🛡️ Proposed defensive check
func (h *HTTPServer) handleInteraction(r *http.Request, matchedChunk, fullID, reqString, respString, hostPort string) { + if len(matchedChunk) < h.options.CorrelationIdLength { + return + } correlationID := matchedChunk[:h.options.CorrelationIdLength]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/http_server.go` around lines 222 - 227, The code in HTTPServer.handleInteraction slices matchedChunk[:h.options.CorrelationIdLength] without validating matchedChunk length; add a defensive check (similar to the LDAP handleInteraction check) to ensure len(matchedChunk) >= h.options.CorrelationIdLength before slicing, and if the check fails set a safe fallback for correlationID (e.g., use matchedChunk as-is, empty string, or generate a placeholder) and optionally log the unexpected condition to avoid a panic; update the logic constructing the Interaction.UniqueID to use the validated/fallback correlationID.pkg/server/smtp_server.go (2)
154-156: Redundant condition:i+1 <= len(parts)is always true.🔧 Proposed simplification
- if i+1 <= len(parts) { - fullID = strings.Join(parts[:i+1], ".") - } + fullID = strings.Join(parts[:i+1], ".")🤖 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 154 - 156, The condition checking "if i+1 <= len(parts)" before setting fullID is redundant because i is already within the loop bounds; remove the if and always assign fullID = strings.Join(parts[:i+1], ".") where fullID, parts and i are used (the block containing the current assignment) so the code is simpler and avoids a tautological check.
83-87: Consider adding a defensive check for emptymatchedChunk.Same observation as HTTP server - the function slices
matchedChunk[:h.options.CorrelationIdLength]without validation. Adding a length check would provide consistency with LDAP server's approach.🛡️ Proposed defensive check
func (h *SMTPServer) storeInteraction(matchedChunk, fullID, dataString, from, remoteAddr string) { + if len(matchedChunk) < h.options.CorrelationIdLength { + return + } correlationID := matchedChunk[:h.options.CorrelationIdLength]🤖 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 - 87, The storeInteraction method slices matchedChunk without validating its length; add a defensive check before computing correlationID (in function storeInteraction) to ensure len(matchedChunk) >= h.options.CorrelationIdLength—if not, handle it the same way as the LDAP server (e.g., set correlationID to matchedChunk or a fallback value, or log and return) so you don't panic on short or empty matchedChunk; update any downstream assumptions accordingly (references: storeInteraction, matchedChunk, h.options.CorrelationIdLength).pkg/server/ldap_server.go (1)
129-131: Redundant condition:i+1 <= len(partChunks)is always true.Since
icomes fromrange partChunks, its maximum value islen(partChunks)-1, soi+1is at mostlen(partChunks). This condition will always evaluate to true.🔧 Proposed simplification
- if i+1 <= len(partChunks) { - fullID = strings.Join(partChunks[:i+1], ".") - } + fullID = strings.Join(partChunks[:i+1], ".")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/ldap_server.go` around lines 129 - 131, Remove the redundant conditional guarding assignment to fullID: within the loop over partChunks (where i is the range index and fullID and partChunks are used), drop the always-true check `if i+1 <= len(partChunks)` and unconditionally set fullID = strings.Join(partChunks[:i+1], "."); this simplifies the code and keeps the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/server/dns_server.go`:
- Around line 394-396: The condition `i+1 <= len(parts)` is redundant because
slice upper bounds are always <= len(parts) in this loop; simplify by removing
the if-check and directly set fullID = strings.Join(parts[:i+1], ".") where
`fullID`, `parts`, and loop index `i` are used (in the same block in
dns_server.go) so the assignment always runs without the unnecessary guard.
- Around line 302-307: Add a defensive length check at the start of
DNSServer.storeInteraction to ensure matchedChunk is at least
h.options.CorrelationIdLength bytes long before slicing; if len(matchedChunk) <
h.options.CorrelationIdLength, avoid the slice panic by returning early (or set
correlationID to matchedChunk/empty) and skip creating the Interaction that uses
UniqueID = correlationID; update the function around the
matchedChunk[:h.options.CorrelationIdLength] usage to reference this guard.
In `@pkg/server/http_server.go`:
- Around line 193-194: The if-check "if i+1 <= len(parts)" is redundant because
i iterates 0..len(parts)-1; remove the conditional branch and directly set
fullID = strings.Join(parts[:i+1], ".") in the loop (where variables i, parts,
and fullID are used) to simplify the code and match the LDAP server fix.
- Around line 222-227: The code in HTTPServer.handleInteraction slices
matchedChunk[:h.options.CorrelationIdLength] without validating matchedChunk
length; add a defensive check (similar to the LDAP handleInteraction check) to
ensure len(matchedChunk) >= h.options.CorrelationIdLength before slicing, and if
the check fails set a safe fallback for correlationID (e.g., use matchedChunk
as-is, empty string, or generate a placeholder) and optionally log the
unexpected condition to avoid a panic; update the logic constructing the
Interaction.UniqueID to use the validated/fallback correlationID.
In `@pkg/server/ldap_server.go`:
- Around line 129-131: Remove the redundant conditional guarding assignment to
fullID: within the loop over partChunks (where i is the range index and fullID
and partChunks are used), drop the always-true check `if i+1 <= len(partChunks)`
and unconditionally set fullID = strings.Join(partChunks[:i+1], "."); this
simplifies the code and keeps the same behavior.
In `@pkg/server/smtp_server.go`:
- Around line 154-156: The condition checking "if i+1 <= len(parts)" before
setting fullID is redundant because i is already within the loop bounds; remove
the if and always assign fullID = strings.Join(parts[:i+1], ".") where fullID,
parts and i are used (the block containing the current assignment) so the code
is simpler and avoids a tautological check.
- Around line 83-87: The storeInteraction method slices matchedChunk without
validating its length; add a defensive check before computing correlationID (in
function storeInteraction) to ensure len(matchedChunk) >=
h.options.CorrelationIdLength—if not, handle it the same way as the LDAP server
(e.g., set correlationID to matchedChunk or a fallback value, or log and return)
so you don't panic on short or empty matchedChunk; update any downstream
assumptions accordingly (references: storeInteraction, matchedChunk,
h.options.CorrelationIdLength).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bcf93a4-5a66-4ba6-9131-9ca4db5d0805
📒 Files selected for processing (7)
pkg/server/dns_server.gopkg/server/http_server.gopkg/server/ldap_server.gopkg/server/server.gopkg/server/server_test.gopkg/server/smtp_server.gopkg/server/util.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/server/http_server.go (1)
223-240:⚠️ Potential issue | 🟠 MajorDon't turn
Interaction.UniqueIDinto the storage key.Line 227 writes
correlationIDback intoInteraction.UniqueID.pollreturns these stored interaction blobs verbatim, so this changes the wire contract, not just the storage lookup: same-correlation probes that used different nonces now report the sameunique-id. Keep usingcorrelationIDonly forStorage.AddInteraction; populateUniqueIDwith the richest matched identifier you recovered. The same assignment exists in the DNS/SMTP/LDAP helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/http_server.go` around lines 223 - 240, The code assigns the short correlationID into Interaction.UniqueID, which changes the wire contract; instead leave UniqueID set to the richest identifier you recovered (e.g., use fullID or the original matched identifier) and use correlationID only as the storage key when calling h.options.Storage.AddInteraction(correlationID, data). Update the assignment in http_server.go (and mirror the same fix in the DNS/SMTP/LDAP helpers) so Interaction.UniqueID retains fullID (or the fuller matched identifier) while Storage.AddInteraction continues to use correlationID.
🤖 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/dns_server.go`:
- Around line 390-409: fullID is currently built as strings.Join(parts[:i+1],
".") which drops any labels to the right of the matched correlation ID; change
it to include the entire left-hand side of the interactsh domain instead of
stopping at the matched label. Concretely, compute the suffix length (number of
labels in the configured interactsh domain) and build fullID from the
left-hand-side labels: strings.Join(parts[:len(parts)-suffixLen], ".") (use the
appropriate accessor on h.options or derive it from the configured domain), then
pass that fullID into h.storeInteraction(normalizedPartChunk, fullID, ...) and
h.storeInteraction(normalizedPart, fullID, ...) ; apply the same fix to the
duplicated logic in the HTTP/SMTP/LDAP handlers so FullId consistently includes
all left-side labels.
---
Outside diff comments:
In `@pkg/server/http_server.go`:
- Around line 223-240: The code assigns the short correlationID into
Interaction.UniqueID, which changes the wire contract; instead leave UniqueID
set to the richest identifier you recovered (e.g., use fullID or the original
matched identifier) and use correlationID only as the storage key when calling
h.options.Storage.AddInteraction(correlationID, data). Update the assignment in
http_server.go (and mirror the same fix in the DNS/SMTP/LDAP helpers) so
Interaction.UniqueID retains fullID (or the fuller matched identifier) while
Storage.AddInteraction continues to use correlationID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: caf9c9e0-ba34-4280-94fa-83b7f2965ec4
📒 Files selected for processing (4)
pkg/server/dns_server.gopkg/server/http_server.gopkg/server/ldap_server.gopkg/server/smtp_server.go
| // match corrID+nonce in same label (higher confidence) | ||
| var matched bool | ||
| parts := strings.Split(domain, ".") | ||
| for i, part := range parts { | ||
| for partChunk := range stringsutil.SlideWithLength(part, h.options.GetIdLength()) { | ||
| for partChunk := range stringsutil.SlideWithLength(part, h.options.getMinIdLength()) { | ||
| normalizedPartChunk := strings.ToLower(partChunk) | ||
| if h.options.isCorrelationID(normalizedPartChunk) { | ||
| fullID = part | ||
| if i+1 <= len(parts) { | ||
| fullID = strings.Join(parts[:i+1], ".") | ||
| } | ||
| uniqueID = normalizedPartChunk | ||
| fullID := strings.Join(parts[:i+1], ".") | ||
| h.storeInteraction(normalizedPartChunk, fullID, qtype, requestMsg, responseMsg, host) | ||
| matched = true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if uniqueID != "" { | ||
| correlationID := uniqueID[:h.options.CorrelationIdLength] | ||
| host, _, _ := net.SplitHostPort(w.RemoteAddr().String()) | ||
| interaction := &Interaction{ | ||
| Protocol: "dns", | ||
| UniqueID: uniqueID, | ||
| FullId: fullID, | ||
| QType: toQType(r.Question[0].Qtype), | ||
| RawRequest: requestMsg, | ||
| RawResponse: responseMsg, | ||
| RemoteAddress: host, | ||
| Timestamp: time.Now(), | ||
| } | ||
| data, err := jsoniter.Marshal(interaction) | ||
| if err != nil { | ||
| gologger.Warning().Msgf("Could not encode dns interaction: %s\n", err) | ||
| } else { | ||
| gologger.Debug().Msgf("DNS Interaction: \n%s\n", string(data)) | ||
| if err := h.options.Storage.AddInteraction(correlationID, data); err != nil { | ||
| gologger.Warning().Msgf("Could not store dns interaction: %s\n", err) | ||
| // match bare corrID (no nonce, possibly split corrID and nonce in different subdomain parts) | ||
| if !matched { | ||
| for i, part := range parts { | ||
| normalizedPart := strings.ToLower(part) | ||
| if len(normalizedPart) == h.options.CorrelationIdLength && h.options.isCorrelationID(normalizedPart) { | ||
| fullID := strings.Join(parts[:i+1], ".") | ||
| h.storeInteraction(normalizedPart, fullID, qtype, requestMsg, responseMsg, host) |
There was a problem hiding this comment.
FullId drops labels that appear after the matched correlation ID.
Line 397 and Line 408 build fullID with strings.Join(parts[:i+1], "."), so corrid.nonce.example.com is persisted as just corrid. After this PR, UniqueID is also reduced to the bare correlation ID, so different split/reordered payloads can collapse to the same (UniqueID, FullId) pair in poll results. Build FullId from the entire left-hand side of the interactsh domain instead. The same parts[:i+1] rule is duplicated in the HTTP/SMTP/LDAP handlers.
🤖 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 390 - 409, fullID is currently built
as strings.Join(parts[:i+1], ".") which drops any labels to the right of the
matched correlation ID; change it to include the entire left-hand side of the
interactsh domain instead of stopping at the matched label. Concretely, compute
the suffix length (number of labels in the configured interactsh domain) and
build fullID from the left-hand-side labels:
strings.Join(parts[:len(parts)-suffixLen], ".") (use the appropriate accessor on
h.options or derive it from the configured domain), then pass that fullID into
h.storeInteraction(normalizedPartChunk, fullID, ...) and
h.storeInteraction(normalizedPart, fullID, ...) ; apply the same fix to the
duplicated logic in the HTTP/SMTP/LDAP handlers so FullId consistently includes
all left-side labels.
|
The most recent CodeRabbit feedback does not seem accurate to me, let me know if you want additional changes. Thank you! |
Prior commits enable the nonce to be split from the correlation id. However they were only being reprorted correctly as a subdomain when put to the left of the correlation id. We now include up to the server domain.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/server/server_test.go (1)
87-175: Add a ScanEverywhere regression test forFullIdpreservation.These cases validate non-ScanEverywhere tiering, but they don’t cover the ScanEverywhere path. Adding one case here would catch
FullIdtruncation regressions when sliding-window matches are shorter than the actual ID token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/server_test.go` around lines 87 - 175, Add a new subtest in TestTwoTierMatching that enables the ScanEverywhere path on the Options struct (set options.ScanEverywhere = true) and constructs a domain where the sliding-window match would be shorter than the full ID (e.g., corrID + nonce with a nonce length shorter than the sliding window), then run the same sliding/label checks and assert that the preserved FullId equals the original corrID (or corrID+nonce as appropriate) rather than the truncated window; reference Options, ScanEverywhere, isCorrelationID, and FullId when locating where to adjust the test logic and assertions so this regression (FullId truncation) is caught.
🤖 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/dns_server.go`:
- Around line 382-386: The storeInteraction call is passing the sliding window
chunk slice `part` as the full ID, truncating long nonces; change the argument
so the full token `chunk` (the complete sliding window input) is passed as the
FullId instead of `part` — update the call in the loop that uses
stringsutil.SlideWithLength so h.storeInteraction(normalizedPart, chunk, qtype,
requestMsg, responseMsg, host) (keeping normalizedPart from isCorrelationID) and
ensure SlideWithLength/getMinIdLength logic remains unchanged.
In `@pkg/server/http_server.go`:
- Around line 177-181: The sliding window `part` is a truncated token but
`FullId` should be the full token `chunk`; update the call to
h.handleInteraction so the FullId argument uses the full token rather than the
min-length slice. Specifically, in pkg/server/http_server.go where you loop over
stringsutil.SlideWithLength(chunk, h.options.getMinIdLength()) and call
h.handleInteraction(r, normalizedPart, part, reqString, respString, host), pass
`chunk` as the FullId argument (keep the isCorrelationID check using
normalizedPart) so the function h.handleInteraction receives the complete token.
In `@pkg/server/util.go`:
- Around line 40-43: The subdomain extraction in subdomainOf currently returns
on the first suffix match which can pick the wrong domain when configured
domains overlap; change subdomainOf to scan all options.Domains, track the
longest domain that matches stringsutil.HasSuffixI(hostname, "."+domain), and
after the loop strip that longest matching domain (using
hostname[:len(hostname)-len(longest)-1]) and return the result (or the original
hostname if no match). Ensure you reference the options.Domains loop and the
HasSuffixI check when implementing the longest-match logic.
---
Nitpick comments:
In `@pkg/server/server_test.go`:
- Around line 87-175: Add a new subtest in TestTwoTierMatching that enables the
ScanEverywhere path on the Options struct (set options.ScanEverywhere = true)
and constructs a domain where the sliding-window match would be shorter than the
full ID (e.g., corrID + nonce with a nonce length shorter than the sliding
window), then run the same sliding/label checks and assert that the preserved
FullId equals the original corrID (or corrID+nonce as appropriate) rather than
the truncated window; reference Options, ScanEverywhere, isCorrelationID, and
FullId when locating where to adjust the test logic and assertions so this
regression (FullId truncation) is caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9586c415-8f88-4707-a232-d2ce5e05981a
📒 Files selected for processing (6)
pkg/server/dns_server.gopkg/server/http_server.gopkg/server/ldap_server.gopkg/server/server_test.gopkg/server/smtp_server.gopkg/server/util.go
| for part := range stringsutil.SlideWithLength(chunk, h.options.getMinIdLength()) { | ||
| normalizedPart := strings.ToLower(part) | ||
| if h.options.isCorrelationID(normalizedPart) { | ||
| uniqueID = normalizedPart | ||
| fullID = part | ||
| h.storeInteraction(normalizedPart, part, qtype, requestMsg, responseMsg, host) | ||
| } |
There was a problem hiding this comment.
ScanEverywhere stores truncated FullId for longer nonce values.
At Line 385, fullID is set to part (the sliding-window chunk), not the full token (chunk). With getMinIdLength(), this can truncate FullId to corrID+minimum nonce.
Suggested fix
- h.storeInteraction(normalizedPart, part, qtype, requestMsg, responseMsg, host)
+ h.storeInteraction(normalizedPart, chunk, qtype, requestMsg, responseMsg, host)📝 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.
| for part := range stringsutil.SlideWithLength(chunk, h.options.getMinIdLength()) { | |
| normalizedPart := strings.ToLower(part) | |
| if h.options.isCorrelationID(normalizedPart) { | |
| uniqueID = normalizedPart | |
| fullID = part | |
| h.storeInteraction(normalizedPart, part, qtype, requestMsg, responseMsg, host) | |
| } | |
| for part := range stringsutil.SlideWithLength(chunk, h.options.getMinIdLength()) { | |
| normalizedPart := strings.ToLower(part) | |
| if h.options.isCorrelationID(normalizedPart) { | |
| h.storeInteraction(normalizedPart, chunk, qtype, requestMsg, responseMsg, host) | |
| } |
🤖 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 382 - 386, The storeInteraction call
is passing the sliding window chunk slice `part` as the full ID, truncating long
nonces; change the argument so the full token `chunk` (the complete sliding
window input) is passed as the FullId instead of `part` — update the call in the
loop that uses stringsutil.SlideWithLength so h.storeInteraction(normalizedPart,
chunk, qtype, requestMsg, responseMsg, host) (keeping normalizedPart from
isCorrelationID) and ensure SlideWithLength/getMinIdLength logic remains
unchanged.
There was a problem hiding this comment.
This is pre-existing behavior I prefer to avoid bringing into scope of this PR
There was a problem hiding this comment.
@jentfoo, understood — I'll defer to your judgment on scoping. Would you like me to open a follow-up GitHub issue to track this pre-existing ScanEverywhere truncation of FullId so it isn't lost?
✏️ Learnings added
Learnt from: jentfoo
Repo: projectdiscovery/interactsh PR: 1347
File: pkg/server/dns_server.go:382-386
Timestamp: 2026-03-15T23:59:09.824Z
Learning: In `pkg/server/dns_server.go`, the `ScanEverywhere` path passes the sliding-window slice `part` (not the full token `chunk`) as `fullID` to `storeInteraction`, which can truncate `FullId` for longer nonce values. This is pre-existing behavior intentionally left out of scope of PR `#1347` (feat/support-flexible-nonce).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| for part := range stringsutil.SlideWithLength(chunk, h.options.getMinIdLength()) { | ||
| normalizedPart := strings.ToLower(part) | ||
| if h.options.isCorrelationID(normalizedPart) { | ||
| h.handleInteraction(r, normalizedPart, part, reqString, respString, host) | ||
| } |
There was a problem hiding this comment.
ScanEverywhere passes a truncated value into FullId.
At Line 180, part is a min-length sliding window, so FullId can be shortened (especially with nonce lengths > minimum). Use the full token (chunk) instead.
Suggested fix
- h.handleInteraction(r, normalizedPart, part, reqString, respString, host)
+ h.handleInteraction(r, normalizedPart, chunk, reqString, respString, host)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/server/http_server.go` around lines 177 - 181, The sliding window `part`
is a truncated token but `FullId` should be the full token `chunk`; update the
call to h.handleInteraction so the FullId argument uses the full token rather
than the min-length slice. Specifically, in pkg/server/http_server.go where you
loop over stringsutil.SlideWithLength(chunk, h.options.getMinIdLength()) and
call h.handleInteraction(r, normalizedPart, part, reqString, respString, host),
pass `chunk` as the FullId argument (keep the isCorrelationID check using
normalizedPart) so the function h.handleInteraction receives the complete token.
There was a problem hiding this comment.
This is pre-existing behavior I prefer to avoid bringing into scope of this PR
There was a problem hiding this comment.
@jentfoo, understood — happy to keep this out of scope. Would you like me to open a follow-up issue to track the FullId truncation in the ScanEverywhere path so it doesn't get lost?
(╯°□°）╯
| for _, domain := range options.Domains { | ||
| if stringsutil.HasSuffixI(hostname, "."+domain) { | ||
| return hostname[:len(hostname)-len(domain)-1] | ||
| } |
There was a problem hiding this comment.
Use the longest matching configured domain in subdomainOf.
Line 40 currently returns on the first suffix match. With overlapping domains, this can strip the wrong suffix and produce incorrect FullId values.
Suggested fix
func (options *Options) subdomainOf(hostname string, isFQDN bool) string {
if isFQDN {
hostname = strings.TrimSuffix(hostname, ".")
}
+ longestMatch := ""
for _, domain := range options.Domains {
if stringsutil.HasSuffixI(hostname, "."+domain) {
- return hostname[:len(hostname)-len(domain)-1]
+ if len(domain) > len(longestMatch) {
+ longestMatch = domain
+ }
}
}
+ if longestMatch != "" {
+ return hostname[:len(hostname)-len(longestMatch)-1]
+ }
return hostname
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/server/util.go` around lines 40 - 43, The subdomain extraction in
subdomainOf currently returns on the first suffix match which can pick the wrong
domain when configured domains overlap; change subdomainOf to scan all
options.Domains, track the longest domain that matches
stringsutil.HasSuffixI(hostname, "."+domain), and after the loop strip that
longest matching domain (using hostname[:len(hostname)-len(longest)-1]) and
return the result (or the original hostname if no match). Ensure you reference
the options.Domains loop and the HasSuffixI check when implementing the
longest-match logic.
There was a problem hiding this comment.
Technically true, but I don't personally think the case of a domain.com + subdomain.domain.com both being registered is likely (and thus not worth the complexity to fix).
There was a problem hiding this comment.
@jentfoo, that's a fair trade-off — overlapping configured domains would be an unusual setup, and keeping the implementation simple is a reasonable call. Acknowledged!
Prior to this change clients registered correlation ids are stored exactly, but if the server was configured shorter it would fail to match. This change truncates those user supplied ids so they will match. Allowing the server to operate with it's shorter configured correlation ids, and improving client configuration flexibility.
4 participants
This change simplifies the interaction storage to only retain the correlation ID, since the nonce is not needed for routing interactions to clients.
The biggest concern in this change is accurately choosing the correlation id. IDs with suffixed nonces are preferred if available, but this change is willing to accept isolated correlationIDs. This allows the correlation id and nonce to be split into different subdomain components (or orders).
This PR is presented as an alternative to #1345
#1345 provides a minimal fix to support clients able to configure nonce lengths LONGER than the server, but still requires the client to meet at least that minimum length.
This PR makes the server and client nonce lengths fully independent (client can configure any length, or no length, as a subdomain, or a suffix to the correlation id). This offers a lot of flexibility for different use cases without the need to adjust the server configuration.
Summary by CodeRabbit
Improvements
Tests
Documentation