Draft
Conversation
Neo - PR Security ReviewNo security issues found Highlights
Hardening Notes
Comment |
WalkthroughIntegrates NERVA library for service fingerprinting in naabu, improving signal handling for graceful shutdown, enabling service discovery and versioning features, and enhancing output formatting with CSV support and detailed service information display. Changes
Sequence DiagramsequenceDiagram
participant Runner as Runner
participant Scanner as Scanner
participant NERVA as NERVA Library
participant Results as Results Aggregator
Runner->>Scanner: Scan hosts & collect open ports
Scanner-->>Runner: Return initial scan results
Runner->>NERVA: Extract TCP/UDP targets from open ports
Note over Runner,NERVA: Collect target list (IP:port pairs)
NERVA->>NERVA: Fingerprint TCP targets
NERVA->>NERVA: Fingerprint UDP targets
NERVA-->>Runner: Return scanned services<br/>(name, version, protocol)
Runner->>Results: Convert service data to Port structures
Note over Runner,Results: Integrate protocol, TLS, version info
Runner->>Results: Merge enriched ports back into HostResult
Runner->>Runner: Format & emit results<br/>(CSV/JSON with service details)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 6
🧹 Nitpick comments (1)
pkg/runner/runner.go (1)
215-218: Avoid active fingerprinting inonReceiveunless it is strictly needed.
onReceiveis a hot path; callingenrichHostResultPortshere adds active probes per callback, then a full fingerprint pass runs again later. This can significantly increase runtime and probe volume.🔧 Suggested guard to reduce unnecessary work
portsToEmit := hostResult.Ports - if r.options.ServiceDiscovery || r.options.ServiceVersion { + if (r.options.ServiceDiscovery || r.options.ServiceVersion) && + (!r.options.DisableStdout || r.options.OnResult != nil) { portsToEmit = r.enrichHostResultPorts(hostResult) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runner/runner.go` around lines 215 - 218, The onReceive hot path currently calls enrichHostResultPorts (via portsToEmit = r.enrichHostResultPorts(hostResult)), triggering active probes per callback; remove that direct call from onReceive and instead defer enrichment to the later fingerprinting/pass that already performs full fingerprinting. Concretely, stop invoking enrichHostResultPorts inside onReceive: leave portsToEmit = hostResult.Ports, and mark the hostResult (e.g., set a flag or enqueue the hostResult) so the fingerprinting stage will call r.enrichHostResultPorts(hostResult) only once when needed (controlled by r.options.ServiceDiscovery or r.options.ServiceVersion). Ensure the new flow still respects those option checks but avoids active probing inside onReceive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/naabu/main.go`:
- Around line 103-125: The detached goroutine performing shutdown (calling
cancel(), saving resume via options.ResumeCfg.SaveResumeConfig(), logging
runner.DefaultResumeFilePath(), invoking naabuRunner.ShowScanResultOnExit() and
naabuRunner.Close(), then os.Exit(1)) races with process exit; move these steps
out of the anonymous go func and execute them directly in the signal-handler
goroutine so shutdown runs synchronously and deterministically: call cancel(),
then if options.ResumeCfg != nil && options.ResumeCfg.ShouldSaveResume() save
and log the resume file, then if naabuRunner != nil call ShowScanResultOnExit()
and Close() (logging any errors), and finally call os.Exit(1).
In `@pkg/runner/nerva.go`:
- Around line 109-111: The current port validation only checks for non-positive
values (if service.Port <= 0) and allows ports > 65535; update the validation to
reject ports outside the valid 1–65535 range (e.g., if service.Port <= 0 ||
service.Port > 65535) so the function that converts the service (the block using
service.Port and returning nil, "", false) returns the same error path for
out-of-range ports and prevents invalid data from entering results.
- Line 238: The assignment to the deprecated field p.TLS should be replaced with
the new compatibility path: stop writing to p.TLS and instead call the new
setter or assign the new field (e.g., use p.SetTLS(enhanced.TLS) or p.TLSConfig
= enhanced.TLS) depending on the p type; if neither exists, add a small
compatibility method on p’s type named SetTLS(tls) that stores the value into
the new field (TLSConfig) and use that here to avoid the deprecated SA1019 usage
while preserving behavior.
- Around line 163-165: The nil-check for hostResult is wrong because it
dereferences hostResult when returning hostResult.Ports; update the guard in the
function containing hostResult so that if hostResult == nil you return nil (or
an empty slice) immediately, and only access hostResult.Ports after confirming
hostResult != nil; i.e., change the conditional branch to return a safe zero
value instead of hostResult.Ports and then proceed to use hostResult.Ports when
non-nil.
In `@pkg/runner/output.go`:
- Line 179: formatOutput currently concatenates host and port with ":" (host +
":" + strconv.Itoa(p.Port)), which breaks for IPv6; change it to use
net.JoinHostPort(host, strconv.Itoa(p.Port)) to produce bracketed IPv6 host:port
pairs and import net if missing, keeping the rest of formatOutput and any
callers (e.g., the bufwriter.WriteString call) unchanged.
In `@pkg/runner/util_test.go`:
- Around line 43-47: The test branch where tt.args == "localhost" currently
accepts "::127" which is not an IPv6 loopback; update the assertions to ensure
the returned addresses are actual loopback addresses by parsing each value from
gotV6 with net.ParseIP(ip) and checking ip != nil and ip.IsLoopback() rather
than matching a hardcoded list (replace the assert.Contains check against
{"::1","::127"}). Keep the existing assert.NotNil(net.ParseIP(...)) check but
follow it with a loopback check using the parsed IP's IsLoopback method to
robustly validate localhost answers.
---
Nitpick comments:
In `@pkg/runner/runner.go`:
- Around line 215-218: The onReceive hot path currently calls
enrichHostResultPorts (via portsToEmit = r.enrichHostResultPorts(hostResult)),
triggering active probes per callback; remove that direct call from onReceive
and instead defer enrichment to the later fingerprinting/pass that already
performs full fingerprinting. Concretely, stop invoking enrichHostResultPorts
inside onReceive: leave portsToEmit = hostResult.Ports, and mark the hostResult
(e.g., set a flag or enqueue the hostResult) so the fingerprinting stage will
call r.enrichHostResultPorts(hostResult) only once when needed (controlled by
r.options.ServiceDiscovery or r.options.ServiceVersion). Ensure the new flow
still respects those option checks but avoids active probing inside onReceive.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/naabu/main.gogo.modpkg/runner/nerva.gopkg/runner/nerva_test.gopkg/runner/options.gopkg/runner/output.gopkg/runner/output_test.gopkg/runner/runner.gopkg/runner/util_test.gopkg/runner/validate.go
💤 Files with no reviewable changes (1)
- pkg/runner/validate.go
Comment on lines
+103
to
+125
| go func() { | ||
| // Cancel context to stop ongoing tasks | ||
| cancel() | ||
|
|
||
| // Show scan result if runner is available | ||
| if naabuRunner != nil { | ||
| naabuRunner.ShowScanResultOnExit() | ||
| // Try to save resume config if needed | ||
| if options.ResumeCfg != nil && options.ResumeCfg.ShouldSaveResume() { | ||
| gologger.Info().Msgf("Creating resume file: %s\n", runner.DefaultResumeFilePath()) | ||
| if err := options.ResumeCfg.SaveResumeConfig(); err != nil { | ||
| gologger.Error().Msgf("Couldn't create resume file: %s\n", err) | ||
| } | ||
| } | ||
|
|
||
| if err := naabuRunner.Close(); err != nil { | ||
| gologger.Error().Msgf("Couldn't close runner: %s\n", err) | ||
| } | ||
| } | ||
| // Show scan result if runner is available | ||
| if naabuRunner != nil { | ||
| naabuRunner.ShowScanResultOnExit() | ||
|
|
||
| // Final flush if gologger has a Close method (placeholder if exists) | ||
| // Example: gologger.Close() | ||
| if err := naabuRunner.Close(); err != nil { | ||
| gologger.Error().Msgf("Couldn't close runner: %s\n", err) | ||
| } | ||
| } | ||
|
|
||
| os.Exit(1) | ||
| os.Exit(1) | ||
| }() |
There was a problem hiding this comment.
Graceful-shutdown work is detached, which can race with process exit.
Starting a second goroutine for cleanup makes resume-save/result-flush/close ordering non-deterministic under interruption. Keep shutdown steps in the signal-handler goroutine to make completion deterministic.
🔧 Suggested fix
- go func() {
- // Cancel context to stop ongoing tasks
- cancel()
-
- // Try to save resume config if needed
- if options.ResumeCfg != nil && options.ResumeCfg.ShouldSaveResume() {
- gologger.Info().Msgf("Creating resume file: %s\n", runner.DefaultResumeFilePath())
- if err := options.ResumeCfg.SaveResumeConfig(); err != nil {
- gologger.Error().Msgf("Couldn't create resume file: %s\n", err)
- }
- }
-
- // Show scan result if runner is available
- if naabuRunner != nil {
- naabuRunner.ShowScanResultOnExit()
-
- if err := naabuRunner.Close(); err != nil {
- gologger.Error().Msgf("Couldn't close runner: %s\n", err)
- }
- }
-
- os.Exit(1)
- }()
+ // Cancel context to stop ongoing tasks
+ cancel()
+
+ // Try to save resume config if needed
+ if options.ResumeCfg != nil && options.ResumeCfg.ShouldSaveResume() {
+ gologger.Info().Msgf("Creating resume file: %s\n", runner.DefaultResumeFilePath())
+ if err := options.ResumeCfg.SaveResumeConfig(); err != nil {
+ gologger.Error().Msgf("Couldn't create resume file: %s\n", err)
+ }
+ }
+
+ // Show scan result if runner is available
+ if naabuRunner != nil {
+ naabuRunner.ShowScanResultOnExit()
+
+ if err := naabuRunner.Close(); err != nil {
+ gologger.Error().Msgf("Couldn't close runner: %s\n", err)
+ }
+ }
+
+ os.Exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/naabu/main.go` around lines 103 - 125, The detached goroutine performing
shutdown (calling cancel(), saving resume via
options.ResumeCfg.SaveResumeConfig(), logging runner.DefaultResumeFilePath(),
invoking naabuRunner.ShowScanResultOnExit() and naabuRunner.Close(), then
os.Exit(1)) races with process exit; move these steps out of the anonymous go
func and execute them directly in the signal-handler goroutine so shutdown runs
synchronously and deterministically: call cancel(), then if options.ResumeCfg !=
nil && options.ResumeCfg.ShouldSaveResume() save and log the resume file, then
if naabuRunner != nil call ShowScanResultOnExit() and Close() (logging any
errors), and finally call os.Exit(1).
Comment on lines
+109
to
+111
| if service.Port <= 0 { | ||
| return nil, "", false | ||
| } |
There was a problem hiding this comment.
Missing upper-bound port validation in Nerva service conversion.
Ports above 65535 are currently accepted and can leak invalid data into results.
🔧 Suggested fix
- if service.Port <= 0 {
+ if service.Port <= 0 || service.Port > 65535 {
return nil, "", false
}📝 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.
Suggested change
| if service.Port <= 0 { | |
| return nil, "", false | |
| } | |
| if service.Port <= 0 || service.Port > 65535 { | |
| return nil, "", false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runner/nerva.go` around lines 109 - 111, The current port validation only
checks for non-positive values (if service.Port <= 0) and allows ports > 65535;
update the validation to reject ports outside the valid 1–65535 range (e.g., if
service.Port <= 0 || service.Port > 65535) so the function that converts the
service (the block using service.Port and returning nil, "", false) returns the
same error path for out-of-range ports and prevents invalid data from entering
results.
Comment on lines
+163
to
+165
| if hostResult == nil || len(hostResult.Ports) == 0 { | ||
| return hostResult.Ports | ||
| } |
There was a problem hiding this comment.
Nil guard still dereferences hostResult on the return path.
When hostResult == nil, returning hostResult.Ports will panic.
🔧 Suggested fix
func (r *Runner) enrichHostResultPorts(hostResult *result.HostResult) []*port.Port {
- if hostResult == nil || len(hostResult.Ports) == 0 {
+ if hostResult == nil {
+ return nil
+ }
+ if len(hostResult.Ports) == 0 {
return hostResult.Ports
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runner/nerva.go` around lines 163 - 165, The nil-check for hostResult is
wrong because it dereferences hostResult when returning hostResult.Ports; update
the guard in the function containing hostResult so that if hostResult == nil you
return nil (or an empty slice) immediately, and only access hostResult.Ports
after confirming hostResult != nil; i.e., change the conditional branch to
return a safe zero value instead of hostResult.Ports and then proceed to use
hostResult.Ports when non-nil.
|
|
||
| for _, p := range hostResult.Ports { | ||
| if enhanced, ok := resultsByPort[key(p.Protocol, p.Port)]; ok { | ||
| p.TLS = enhanced.TLS |
There was a problem hiding this comment.
CI blocker: deprecated TLS field assignment is failing staticcheck (SA1019).
This line is currently breaking the pipeline.
🔧 Suggested fix (temporary compatibility path)
if enhanced, ok := resultsByPort[key(p.Protocol, p.Port)]; ok {
- p.TLS = enhanced.TLS
+ //nolint:staticcheck // keep deprecated field populated for backward compatibility.
+ p.TLS = enhanced.TLS
p.Service = enhanced.Service
}📝 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.
Suggested change
| p.TLS = enhanced.TLS | |
| if enhanced, ok := resultsByPort[key(p.Protocol, p.Port)]; ok { | |
| //nolint:staticcheck // keep deprecated field populated for backward compatibility. | |
| p.TLS = enhanced.TLS | |
| p.Service = enhanced.Service | |
| } |
🧰 Tools
🪛 GitHub Actions: 🔨 Build Test
[error] 238-238: SA1019: p.TLS is deprecated: TLS field will be removed in a future version (staticcheck)
🪛 GitHub Check: Lint
[failure] 238-238:
SA1019: p.TLS is deprecated: TLS field will be removed in a future version (staticcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runner/nerva.go` at line 238, The assignment to the deprecated field
p.TLS should be replaced with the new compatibility path: stop writing to p.TLS
and instead call the new setter or assign the new field (e.g., use
p.SetTLS(enhanced.TLS) or p.TLSConfig = enhanced.TLS) depending on the p type;
if neither exists, add a small compatibility method on p’s type named
SetTLS(tls) that stores the value into the new field (TLSConfig) and use that
here to avoid the deprecated SA1019 usage while preserving behavior.
| } | ||
| sb.WriteString("\n") | ||
| _, err := bufwriter.WriteString(sb.String()) | ||
| _, err := bufwriter.WriteString(formatOutput(host, p, outputCDN, cdnName) + "\n") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current formatting implementation and whether IPv6 formatting is covered by tests.
rg -nP 'host\s*\+\s*":"\s*\+\s*strconv\.Itoa\(p\.Port\)|net\.JoinHostPort\(' pkg/runner/output.go
rg -nP 'formatOutput|WriteHostOutput|IPv6|2001:db8|::1|\[[0-9a-fA-F:]+\]:' pkg/runner/output_test.go || trueRepository: projectdiscovery/naabu
Length of output: 421
🏁 Script executed:
#!/bin/bash
# Let me also examine the formatOutput function directly to understand its current state
cat -n pkg/runner/output.go | sed -n '180,210p'Repository: projectdiscovery/naabu
Length of output: 1021
🏁 Script executed:
#!/bin/bash
# Check the imports to see if net is already imported
head -20 pkg/runner/output.goRepository: projectdiscovery/naabu
Length of output: 427
🏁 Script executed:
#!/bin/bash
# Look at the full test file to see what inputs are being tested
cat -n pkg/runner/output_test.goRepository: projectdiscovery/naabu
Length of output: 2773
Use IPv6-safe host:port formatting in formatOutput.
Line 189 currently builds host:port via concatenation (host + ":" + strconv.Itoa(p.Port)), which is ambiguous for IPv6 addresses (e.g., 2001:db8::1:443 cannot be parsed correctly without brackets). Since line 179 routes all host output through this helper, this is the right place to fix it.
Use net.JoinHostPort() which handles both IPv4 and IPv6 correctly:
Proposed fix
import (
"bufio"
"encoding/csv"
"encoding/json"
"fmt"
"io"
+ "net"
"reflect"
"strconv"
"strings"
"time"
@@
func formatOutput(host string, p *port.Port, outputCDN bool, cdnName string) string {
- line := host + ":" + strconv.Itoa(p.Port)
+ line := net.JoinHostPort(host, strconv.Itoa(p.Port))
if outputCDN && cdnName != "" {
line += " [" + cdnName + "]"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runner/output.go` at line 179, formatOutput currently concatenates host
and port with ":" (host + ":" + strconv.Itoa(p.Port)), which breaks for IPv6;
change it to use net.JoinHostPort(host, strconv.Itoa(p.Port)) to produce
bracketed IPv6 host:port pairs and import net if missing, keeping the rest of
formatOutput and any callers (e.g., the bufwriter.WriteString call) unchanged.
Comment on lines
+43
to
+47
| if tt.args == "localhost" { | ||
| for _, ip := range gotV6 { | ||
| assert.NotNil(t, net.ParseIP(ip), "localhost ipv6 answer should be a valid IP") | ||
| assert.Contains(t, []string{"::1", "::127"}, ip) | ||
| } |
There was a problem hiding this comment.
localhost IPv6 assertion currently accepts a non-loopback address.
::127 is not a loopback IPv6 address, so this can mask incorrect resolver behavior in the localhost test.
🔧 Suggested fix
if tt.args == "localhost" {
for _, ip := range gotV6 {
- assert.NotNil(t, net.ParseIP(ip), "localhost ipv6 answer should be a valid IP")
- assert.Contains(t, []string{"::1", "::127"}, ip)
+ parsed := net.ParseIP(ip)
+ require.NotNil(t, parsed, "localhost ipv6 answer should be a valid IP")
+ assert.True(t, parsed.IsLoopback(), "localhost ipv6 answer should be a loopback IP")
}
} else {📝 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.
Suggested change
| if tt.args == "localhost" { | |
| for _, ip := range gotV6 { | |
| assert.NotNil(t, net.ParseIP(ip), "localhost ipv6 answer should be a valid IP") | |
| assert.Contains(t, []string{"::1", "::127"}, ip) | |
| } | |
| if tt.args == "localhost" { | |
| for _, ip := range gotV6 { | |
| parsed := net.ParseIP(ip) | |
| require.NotNil(t, parsed, "localhost ipv6 answer should be a valid IP") | |
| assert.True(t, parsed.IsLoopback(), "localhost ipv6 answer should be a loopback IP") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runner/util_test.go` around lines 43 - 47, The test branch where tt.args
== "localhost" currently accepts "::127" which is not an IPv6 loopback; update
the assertions to ensure the returned addresses are actual loopback addresses by
parsing each value from gotV6 with net.ParseIP(ip) and checking ip != nil and
ip.IsLoopback() rather than matching a hardcoded list (replace the
assert.Contains check against {"::1","::127"}). Keep the existing
assert.NotNil(net.ParseIP(...)) check but follow it with a loopback check using
the parsed IP's IsLoopback method to robustly validate localhost answers.
dogancanbakir
requested changes
Mar 5, 2026
Member
dogancanbakir
left a comment
dogancanbakir
left a comment
There was a problem hiding this comment.
- merge conflict
- lint err
- CLI example to run and verify
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Close #1647
Summary by CodeRabbit
Release Notes
New Features
Improvements