Scanning Module Oslo Accords - #36
Merged
Merged
Conversation
Scan enumerates the target's subdomains and runs every installed module against each one, module by module, so a module can rely on its dependencies having reported before it runs.
IdanKoblik
added
type/feature
📝 WalkthroughWalkthroughThe PR adds the ChangesScan feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ScanCommand
participant scanning.Scan
participant Finders
participant dns.Resolve
participant module.RunModule
participant scanning.Render
ScanCommand->>scanning.Scan: start scan
scanning.Scan->>Finders: discover subdomains
scanning.Scan->>dns.Resolve: resolve candidate hosts
scanning.Scan->>module.RunModule: run modules for live hosts
module.RunModule-->>scanning.Scan: return module data
scanning.Scan->>scanning.Render: render serialized results
scanning.Render-->>ScanCommand: return formatted output
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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: 11
🧹 Nitpick comments (9)
internal/logging/log.go (1)
30-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnchecked
fmt.Fprintreturn value.Static analysis (
errcheck) flags Line 37: the error fromfmt.Fprint(out, s+"\r\n")is discarded. Sinceoutcan be replaced viaSetOutputwith an arbitraryio.Writer, a write failure there is silently swallowed.Check the error, or explicitly discard it to satisfy the linter and document the intent.
🔧 Proposed fix
func line(s string) { mu.Lock() defer mu.Unlock() if !enabled { return } - fmt.Fprint(out, s+"\r\n") + _, _ = fmt.Fprint(out, s+"\r\n") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/logging/log.go` around lines 30 - 38, Handle the return value of fmt.Fprint in line, either checking and handling the write error or explicitly discarding it to document the intentional behavior and satisfy errcheck. Keep the existing locking, enabled check, and output formatting unchanged.Source: Linters/SAST tools
internal/scanning/render.go (1)
26-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecommend adding unit tests for the recursive renderer.
This file implements non-trivial recursive JSON-to-text rendering with several distinct branches: top-level scalar vs. object vs. array, empty arrays (Line 114-116), arrays mixing scalar and nested elements (Line 78-112), and empty-string field values (Line 130-137, where
val == ""renders as a barelabel:with no visible value). No test file accompanies this change.Since this renders the final scan output shown to end users, add table-driven tests covering these branches to lock in the intended formatting and catch regressions.
Do you want me to draft a starter test file covering these cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scanning/render.go` around lines 26 - 156, Add table-driven unit tests for the recursive renderer centered on value, covering top-level scalars, objects, arrays, empty arrays, arrays mixing scalar and nested elements, and empty-string field values. Assert the complete rendered output, including indentation, headings, bullets, line endings, and bare label formatting, while reusing the existing rendering entry point and style setup.CHANGELOG.md (1)
18-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the new HackerTarget subdomain source.
This PR registers a third finder,
hackertarget, ininternal/net/subdomain/finder.go. The changelog does not mention it. Users see new subdomain sources and a new failure source in scan output, so add an entry.📝 Proposed changelog entry
- Scans skip subdomains that no longer resolve, so dead names left behind in certificate transparency logs no longer cost a timeout per module. +- HackerTarget as an additional subdomain source, alongside crt.sh and + CertSpotter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 18 - 26, Add a CHANGELOG.md entry documenting the newly registered HackerTarget subdomain finder (`hackertarget`) alongside the existing scan and subdomain-source changes, including that it appears as an additional source and may produce a corresponding failure source in scan output.internal/scanning/scanner.go (3)
66-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the cognitive complexity of
Scan.SonarCloud fails the build with cognitive complexity 18 against a limit of 15. Extract the per-module fan-out at lines 115-149 into a helper, for example
runAcrossHosts(result *Result, mi int, m string, manager *module.Manager, reporter *module.Reporter, sem chan struct{}). That removes the nested loop and the goroutine body fromScanand keeps the reported complexity under the limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scanning/scanner.go` around lines 66 - 77, Reduce Scan’s cognitive complexity by extracting the per-module host fan-out and goroutine logic from its module-processing loop into a helper such as runAcrossHosts(result *Result, mi int, m string, manager *module.Manager, reporter *module.Reporter, sem chan struct{}). Update Scan to invoke the helper while preserving existing concurrency, reporting, and result behavior.Source: Linters/SAST tools
218-223: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the column width in
pad.The literal
38appears three times. Name it once so the alignment cannot drift.♻️ Proposed refactor
+const nameColumnWidth = 38 + func pad(name string) string { - if len(name) >= 38 { + if len(name) >= nameColumnWidth { return style.White(name) } - return style.White(name) + strings.Repeat(" ", 38-len(name)) + return style.White(name) + strings.Repeat(" ", nameColumnWidth-len(name)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scanning/scanner.go` around lines 218 - 223, Update the pad function to define the column width once and reuse that named value in the length comparison, padding calculation, and strings.Repeat call, replacing all repeated 38 literals while preserving the current alignment behavior.
115-149: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftA running scan cannot be cancelled.
Scancreates its own contexts and never accepts one. The module loop runs to completion for every module against every host.cmd/main.gosetsinterruptedon SIGINT, butExecblocks inScanand nothing observes the signal. On a target with many live hosts, the operator must wait for the whole run or kill the process.Accept a
context.Contextparameter inScan, derive the enumeration and resolution contexts from it, and checkctx.Err()before each module iteration. Pass the same context intomodule.RunModuleso external module execution stops as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scanning/scanner.go` around lines 115 - 149, Update Scan to accept a context.Context, derive enumeration and resolution contexts from it, and check ctx.Err() before each module iteration to stop promptly on cancellation. Pass the same context into module.RunModule so external module execution is cancelled, while preserving existing scan behavior when the context remains active.internal/cmd/help.go (1)
46-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFlag descriptions are misaligned because the padding is hardcoded.
The block uses fixed spaces per entry.
--uninstallis two characters longer than--versionand--install, so its description starts two columns further right. The command list above already computes padding from the longest name. Apply the same approach to the flag list.♻️ Proposed refactor to align flag descriptions
b.WriteString("\r\n" + style.Bold("Flags") + "\r\n") - b.WriteString(fmt.Sprintf(" %s %s\r\n", - style.Cyan("--target"), - style.Dim("Determines the site that goscouter will target (requires http/https prefix)."), - )) - - b.WriteString(fmt.Sprintf(" %s %s\r\n", - style.Cyan("--version"), - style.Dim("Returns goscouter cli version"), - )) - - b.WriteString(fmt.Sprintf(" %s %s\r\n", - style.Cyan("--install"), - style.Dim("Installs an external module"), - )) - - b.WriteString(fmt.Sprintf(" %s %s\r\n", - style.Cyan("--uninstall"), - style.Dim("Uninstalls an external module"), - )) + flags := []struct{ name, desc string }{ + {"--target", "Determines the site that goscouter will target (requires http/https prefix)."}, + {"--version", "Returns goscouter cli version"}, + {"--install", "Installs an external module"}, + {"--uninstall", "Uninstalls an external module"}, + } + + flagWidth := 0 + for _, f := range flags { + if len(f.name) > flagWidth { + flagWidth = len(f.name) + } + } + + for _, f := range flags { + pad := strings.Repeat(" ", flagWidth-len(f.name)) + b.WriteString(fmt.Sprintf(" %s%s %s\r\n", + style.Cyan(f.name), pad, style.Dim(f.desc))) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/help.go` around lines 46 - 66, Update the flag-list formatting in the help-generation function to compute padding from the longest flag name, matching the existing command-list alignment approach. Use that calculated width for --target, --version, --install, and --uninstall so every description starts at the same column, rather than relying on differing hardcoded spaces.internal/cmd/scan.go (1)
22-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Execignoresargs.The scan command accepts any arguments and discards them. An operator who types
scan example.comgets a scan of the manager target with no notice. Reject unexpected arguments, or document the flags that scan accepts.♻️ Proposed change to reject unexpected arguments
func (cmd *ScanCommand) Exec(args []string) error { + if len(args) > 0 { + return fmt.Errorf("scan takes no arguments, got %q", strings.Join(args, " ")) + } + output, err := scanning.Scan(cmd.CmdManager.Target, cmd.ModuleManager)Add
"strings"to the import block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/scan.go` around lines 22 - 26, Update ScanCommand.Exec to validate args before calling scanning.Scan, rejecting unexpected positional arguments instead of silently ignoring them; use the existing command error-handling conventions and add the required strings import if needed for the validation. Preserve scanning.Scan behavior when no arguments are supplied.internal/dns/dns.go (1)
16-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDistinguish a lookup timeout from a real resolution failure.
Resolvereturns a single error for both cases.internal/scanning/scanner.goline 201 then logs"%s does not resolve"and drops the host from the scan. WithRESOLVE_TIMEOUTat two seconds and 64 concurrent lookups, a slow or rate-limited resolver produces timeouts, and live hosts silently disappear from the report.Wrap the timeout case so callers can report it correctly.
♻️ Proposed refactor
addrs, err := net.DefaultResolver.LookupHost(ctx, host) if err != nil { + if errors.Is(err, context.DeadlineExceeded) { + return nil, fmt.Errorf("lookup %s timed out after %s: %w", host, RESOLVE_TIMEOUT, err) + } return nil, err }Add
"errors"to the import block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dns/dns.go` around lines 16 - 28, Update Resolve to distinguish context deadline timeouts from other lookup failures: when net.DefaultResolver.LookupHost returns a timeout caused by the function’s context, wrap that error with a recognizable timeout context while preserving the original error for inspection via errors.Is; leave non-timeout resolution errors and successful results unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/module/dns.go`:
- Around line 29-36: Allow DNS lookups for any valid hostname by removing the
target-versus-eTLD+1 rejection in the DNS module while retaining appropriate
invalid-domain handling; update internal/module/dns.go lines 29-36 around
EffectiveTLDPlusOne. Include the apex host as a scanner candidate in
internal/scanning/scanner.go lines 156-171 by removing the logic that skips it,
ensuring the target domain can appear in Found, Live, and Hosts.
- Around line 38-43: Update the target normalization in the DNS lookup flow
around parsed and records so scheme-prefixed URLs use parsed.Hostname() rather
than parsed.Path, while preserving bare-hostname handling. Reuse the existing
scanner host normalization helper, such as hostOf, or extract a shared helper
for both scanner and DNS modules; adjust the EffectiveTLDPlusOne guard as needed
so valid scheme-prefixed targets reach dns.Lookup.
In `@internal/module/http.go`:
- Around line 57-64: The module functions should remain silent because the
scanner owns progress logging. In internal/module/http.go lines 57-64, remove
the logging.Failed and logging.Found calls while preserving return of records
and err; apply the same removal to internal/module/dns.go lines 45-50, leaving
any record-count detail to the scanner log or Render.
In `@internal/module/runner.go`:
- Around line 251-271: The module execution path does not propagate cancellation
from the shell to running scans. In internal/module/runner.go lines 251-271, add
a context.Context parameter to RunModule, pass it to m.Scout, and update
RunInOrder to forward its context; in internal/scanning/scanner.go lines
115-149, accept the context in Scan, derive enumeration and resolution contexts
from it, check ctx.Err() before each module iteration, and pass it to
module.RunModule; in internal/cmd/scan.go lines 22-26, create a cancellable
context in Exec, cancel it when the interrupt signal arrives, and pass it to
scanning.Scan.
- Around line 273-287: Update the RunModule flow and its scanner caller so scans
return only the module data without invoking m.Render(data); preserve rendering
for non-scan callers by adding a distinct scan path or equivalent option around
RunModule.
In `@internal/net/subdomain/finder.go`:
- Around line 159-167: Update the response cleanup in the HTTP request flow
around http.DefaultClient.Do so the return value from resp.Body.Close is
explicitly discarded, satisfying errcheck while preserving the existing status
handling and error propagation.
- Around line 174-195: Update the HackerTarget response parsing around the
scanner loop and latest collection: detect plain-text non-CSV quota/error
responses before processing, and skip individual lines without a comma instead
of returning an error. Preserve already collected hosts and continue scanning
valid CSV lines, while still propagating scanner.Err().
In `@internal/scanning/scanner.go`:
- Around line 156-171: Update candidates so the apex host is retained rather
than skipped when name equals host, while continuing to exclude empty names and
deduplicate repeated names. Ensure the resulting scan includes a HostResult for
the target domain and remains compatible with the effective-registrable-domain
filtering in internal/module/dns.go.
- Around line 79-92: Update the subdomain enumeration call in the scanner flow
to pass the bare-domain variable host to subdomain.FindAll instead of target,
while preserving the existing context, error handling, and candidate resolution
behavior.
- Line 130: Normalize scan targets at the RunModule boundary so every module
receives the canonical bare-host/domain form, while preserving interactive
callers’ raw-target behavior through the same contract. Update RunModule’s
documentation to state the canonical target format, and adjust the scanning call
near module.RunModule so it no longer supplies an inconsistently normalized
value; ensure DnsModule and HttpModule both continue receiving compatible
targets.
In `@internal/style/style.go`:
- Around line 65-71: Normalize embedded newlines for the missing Failure, Found,
and Info style helpers by applying rawLines to their message output, matching
Error and ensuring Failuref, Foundf, and Infof receive CRLF-normalized content.
If normalization is intended for direct style callers as well, update the
corresponding style helpers rather than relying only on logging.line().
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 18-26: Add a CHANGELOG.md entry documenting the newly registered
HackerTarget subdomain finder (`hackertarget`) alongside the existing scan and
subdomain-source changes, including that it appears as an additional source and
may produce a corresponding failure source in scan output.
In `@internal/cmd/help.go`:
- Around line 46-66: Update the flag-list formatting in the help-generation
function to compute padding from the longest flag name, matching the existing
command-list alignment approach. Use that calculated width for --target,
--version, --install, and --uninstall so every description starts at the same
column, rather than relying on differing hardcoded spaces.
In `@internal/cmd/scan.go`:
- Around line 22-26: Update ScanCommand.Exec to validate args before calling
scanning.Scan, rejecting unexpected positional arguments instead of silently
ignoring them; use the existing command error-handling conventions and add the
required strings import if needed for the validation. Preserve scanning.Scan
behavior when no arguments are supplied.
In `@internal/dns/dns.go`:
- Around line 16-28: Update Resolve to distinguish context deadline timeouts
from other lookup failures: when net.DefaultResolver.LookupHost returns a
timeout caused by the function’s context, wrap that error with a recognizable
timeout context while preserving the original error for inspection via
errors.Is; leave non-timeout resolution errors and successful results unchanged.
In `@internal/logging/log.go`:
- Around line 30-38: Handle the return value of fmt.Fprint in line, either
checking and handling the write error or explicitly discarding it to document
the intentional behavior and satisfy errcheck. Keep the existing locking,
enabled check, and output formatting unchanged.
In `@internal/scanning/render.go`:
- Around line 26-156: Add table-driven unit tests for the recursive renderer
centered on value, covering top-level scalars, objects, arrays, empty arrays,
arrays mixing scalar and nested elements, and empty-string field values. Assert
the complete rendered output, including indentation, headings, bullets, line
endings, and bare label formatting, while reusing the existing rendering entry
point and style setup.
In `@internal/scanning/scanner.go`:
- Around line 66-77: Reduce Scan’s cognitive complexity by extracting the
per-module host fan-out and goroutine logic from its module-processing loop into
a helper such as runAcrossHosts(result *Result, mi int, m string, manager
*module.Manager, reporter *module.Reporter, sem chan struct{}). Update Scan to
invoke the helper while preserving existing concurrency, reporting, and result
behavior.
- Around line 218-223: Update the pad function to define the column width once
and reuse that named value in the length comparison, padding calculation, and
strings.Repeat call, replacing all repeated 38 literals while preserving the
current alignment behavior.
- Around line 115-149: Update Scan to accept a context.Context, derive
enumeration and resolution contexts from it, and check ctx.Err() before each
module iteration to stop promptly on cancellation. Pass the same context into
module.RunModule so external module execution is cancelled, while preserving
existing scan behavior when the context remains active.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a182375a-26a9-495b-91da-eca77dbeac2e
📒 Files selected for processing (15)
CHANGELOG.mdcmd/main.gointernal/cmd/command.gointernal/cmd/help.gointernal/cmd/scan.gointernal/dns/dns.gointernal/logging/log.gointernal/module/dns.gointernal/module/http.gointernal/module/runner.gointernal/net/subdomain/finder.gointernal/scanning/render.gointernal/scanning/scanner.gointernal/style/style.gopkg/records/http.go
💤 Files with no reviewable changes (1)
- pkg/records/http.go
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
1 participant
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.
Summary
Add full-target scan with live progress output.
Scan enumerates the target's subdomains and runs every installed module against each one, module by module, so a module can rely on its dependencies having reported before it runs.
Related issues
Fixes #25
Type of change
type/bug)type/feature)type/enhancement)type/refactor)type/performance)type/documentation)type/chore)Breaking changes
None
How this was tested
I got rate limit from one of the finders, from the amount of tests that I did.
Checklist
<type>/<short-description>and commits followConventional Commits
Summary by CodeRabbit
New Features
scancommand for discovering subdomains and running DNS and HTTP checks.--installand--uninstalloptions for managing external modules.Bug Fixes