fix(runner): write unsigned-templates WRN to stderr - #7371
Conversation
…#7314) `r.Logger.Print()` is `gologger`'s LevelSilent helper — it bypasses level filtering but routes to stdout. Using it for the unsigned- templates "Use with caution" warning means the line collides with the JSON / JSONL stream when the user runs with `-j`/`-jsonl`, breaking downstream `jq` and any other consumer that expects clean JSON on stdout. Write the warning to os.Stderr directly (keeping the colorized [WRN] prefix and the always-emitted semantics) so the JSON output stays parseable. The integration suite's filterUnsignedTemplatesWarnings helper already strips this message defensively, so no test churn is needed. Closes projectdiscovery#7314.
Neo - PR Security ReviewNo security issues found Hardening Notes
Comment |
WalkthroughStartup/execution warnings in ChangesLogging level and call-site updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/runner/runner.go (1)
399-401: ⚡ Quick winOther
r.Logger.Print()WRN calls are still stdout-bound.If this warning was indeed reaching stdout (corrupting
-joutput), the same issue exists at:
- Line 400:
r.Logger.Print().Msgf("[%v] %v", aurora.BrightYellow("WRN"), "rate limit per minute is deprecated...")- Line 619:
r.Logger.Print().Msgf("[%v] The concurrency value is higher than max-host-error", r.colorizer.BrightYellow("WRN"))- Line 889:
r.Logger.Print().Msgf("[%v] No DAST templates found", aurora.BrightYellow("WRN"))The PR author acknowledges scoping this PR to one site, but all three emit WRN-prefixed messages via Print() that would equally corrupt JSON output in
-jmode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner.go` around lines 399 - 401, Three warning messages currently use r.Logger.Print().Msgf (e.g., the "rate limit per minute is deprecated" call, the "The concurrency value is higher than max-host-error" call, and the "No DAST templates found" call) which writes to stdout and can corrupt JSON `-j` output; change each of those calls to the logger's warning-level API (replace r.Logger.Print().Msgf(...) with r.Logger.Warn().Msgf(...) or equivalent structured Warn method) so the messages are emitted at warning level (preserving the same message text and colorizer/aurora usage) instead of going to stdout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/runner/runner.go`:
- Around line 399-401: Three warning messages currently use
r.Logger.Print().Msgf (e.g., the "rate limit per minute is deprecated" call, the
"The concurrency value is higher than max-host-error" call, and the "No DAST
templates found" call) which writes to stdout and can corrupt JSON `-j` output;
change each of those calls to the logger's warning-level API (replace
r.Logger.Print().Msgf(...) with r.Logger.Warn().Msgf(...) or equivalent
structured Warn method) so the messages are emitted at warning level (preserving
the same message text and colorizer/aurora usage) instead of going to stdout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf5022c9-bd24-47b2-a07f-e0f7e4d45f5f
📒 Files selected for processing (1)
internal/runner/runner.go
CodeRabbit flagged that the rate-limit-deprecated, concurrency-vs-max-host-error, and DAST-no-templates warnings have the same stdout-pollution problem the unsigned-templates one had. Same fix — fmt.Fprintf to os.Stderr — applied to all three so the stdout stream stays clean for -j / -jsonl consumers regardless of which warning fires.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/runner/runner.go (1)
931-935: ⚡ Quick winConsider centralizing WRN-to-stderr formatting in a small helper.
The same
[%v] ...+os.Stderrpattern now appears in multiple places; a helper will reduce drift and future regressions.Proposed refactor sketch
+func (r *Runner) warnStderrf(format string, args ...interface{}) { + _, _ = fmt.Fprintf(os.Stderr, "[%v] %s\n", r.colorizer.BrightYellow("WRN"), fmt.Sprintf(format, args...)) +}- fmt.Fprintf(os.Stderr, "[%v] Loading %d unsigned templates for scan. Use with caution.\n", r.colorizer.BrightYellow("WRN"), value) + r.warnStderrf("Loading %d unsigned templates for scan. Use with caution.", value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runner/runner.go` around lines 931 - 935, Multiple places duplicate the pattern fmt.Fprintf(os.Stderr, "[%v] ...", r.colorizer.BrightYellow(...)) which risks drift; create a small helper (e.g., a method on the runner receiver like warnStderr or a package-level stderrWarn) that accepts a format string and args, formats the prefix using r.colorizer.BrightYellow("WRN") and writes to os.Stderr, then replace the direct fmt.Fprintf calls (including the one in runner.go that logs "Loading %d unsigned templates for scan. Use with caution.") with calls to that helper so all WRN-to-stderr formatting is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/runner/runner.go`:
- Around line 931-935: Multiple places duplicate the pattern
fmt.Fprintf(os.Stderr, "[%v] ...", r.colorizer.BrightYellow(...)) which risks
drift; create a small helper (e.g., a method on the runner receiver like
warnStderr or a package-level stderrWarn) that accepts a format string and args,
formats the prefix using r.colorizer.BrightYellow("WRN") and writes to
os.Stderr, then replace the direct fmt.Fprintf calls (including the one in
runner.go that logs "Loading %d unsigned templates for scan. Use with caution.")
with calls to that helper so all WRN-to-stderr formatting is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7726028d-1f18-40d5-bcf0-c47fee82b753
📒 Files selected for processing (1)
internal/runner/runner.go
dwisiswant0
left a comment
dwisiswant0
left a comment
There was a problem hiding this comment.
You can just use runner.Logger.Warning() instead.
Per the review on projectdiscovery#7371, swap the four `fmt.Fprintf(os.Stderr, ...)` WRN sites for `r.Logger.Warning().Msgf(...)`. Same stderr destination, no duplicated WRN-prefix logic, follows the existing pattern used across the runner package.
|
Done in d596add — swapped all four |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/runner/runner.go (1)
438-440: ⚡ Quick winRemaining
Print()call inClose()is inconsistent with the PR's intent.The honeypot detector summary at Line 439 still uses
r.Logger.Print().Msgf(...), while all the warning sites in this PR were migrated away fromPrint(). If any writer behindr.Loggerroutes-j/-jsonloutput during scanner shutdown.♻️ Proposed fix for consistency
if r.honeypotDetector != nil { - r.Logger.Print().Msgf("%s\n", r.honeypotDetector.Summary()) + r.Logger.Info().Msgf("%s\n", r.honeypotDetector.Summary()) }🤖 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/runner/runner.go` around lines 438 - 440, The Close() method still logs the honeypot detector summary using r.Logger.Print().Msgf(...), which is inconsistent with the PR's migration away from Print(); update the call in the Close() function to use the same non-Print logging level used elsewhere (e.g., r.Logger.Warn().Msgf(...) or the project’s chosen warning method) so the summary goes to the appropriate log level and does not write to stdout/JSONL streams; keep the existing message text and formatting but replace the Print() call on r.Logger with the consistent log level method.
🤖 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.
Nitpick comments:
In `@internal/runner/runner.go`:
- Around line 438-440: The Close() method still logs the honeypot detector
summary using r.Logger.Print().Msgf(...), which is inconsistent with the PR's
migration away from Print(); update the call in the Close() function to use the
same non-Print logging level used elsewhere (e.g., r.Logger.Warn().Msgf(...) or
the project’s chosen warning method) so the summary goes to the appropriate log
level and does not write to stdout/JSONL streams; keep the existing message text
and formatting but replace the Print() call on r.Logger with the consistent log
level method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 140566bb-d28b-4777-8707-abb198c23e99
📒 Files selected for processing (1)
internal/runner/runner.go
|
Pushed d596add. Swapped the four |
dwisiswant0
changed the title
2 participants
Closes #7314.
Background
When the user runs nuclei with
-j/-jsonl, the JSON output stream is corrupted by the[WRN] Loading N unsigned templates for scanmessage:The cause is the call site at
internal/runner/runner.go:931:gologger's(*Logger).Print()returns aLevelSilentevent — its docstring says "Print prints a string on stdout without any extra labels." It's used here to bypass level filtering (so the warning shows even at quiet verbosity), but the side-effect is that it pollutes the JSON output stream.Change
Write the unsigned-templates warning straight to
os.Stderr. That preserves the original intent — always emit, regardless of log level, with the colorized[WRN]prefix — while keepingos.Stdoutclean for JSON / JSONL consumers.fmtandosare already imported in this file.Verification
go build ./...andgo vet ./internal/runner/both clean.The integration suite already has
filterUnsignedTemplatesWarnings()(internal/tests/integration/integration_test.go:269) that defensively strips this message — so no test churn is needed; the helper just becomes a no-op for stdout-only captures.Notes
+5 / -1lines, single file, no public-API change.r.Logger.Print().Msgf("[WRN] ...")call sites in the same file (lines 400, 619, 889) with the same pattern; happy to follow up with those in a separate PR if you'd like consistent stderr routing across all four.Summary by CodeRabbit