feat: added tracking for status code, waf-detection & grouped errors #6028
Conversation
WalkthroughThis change adds an experimental HTTP status tracking feature. A new boolean flag ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Runner
participant Tracker
participant Writer
User->>CLI: Execute with HTTPStats flag enabled
CLI->>Runner: Initialize Runner with HTTPStats option
Runner->>Tracker: Create new Tracker instance
Runner->>Writer: Configure multi-writer with TrackerWriter
Runner->>HTTP Request: Process requests
HTTP Request->>Writer: Log status via RequestStatsLog
Writer->>Tracker: Update stats counters
Runner->>User: On close, display tracked HTTP stats
sequenceDiagram
participant RequestModule as "HTTP Request"
participant Writer
participant Tracker
RequestModule->>Writer: Call RequestStatsLog(statusCode, response)
Writer->>Tracker: Forward and update statistics
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (4)
pkg/output/stats/stats_test.go (2)
7-27: Add edge cases to error tracking tests.The test covers basic increment scenarios but would benefit from additional edge cases:
- Concurrent access to error counters
- Error kinds with special characters
- Very long error kind strings
29-84: Enhance WAF detection test coverage.While the test covers major WAF providers, consider adding:
- Partial WAF signature matches
- Case sensitivity tests
- Multi-line WAF signatures
- WAF signatures with special characters
pkg/output/output.go (1)
564-564: Add TODO comment for empty implementation.The empty implementation suggests future work. Add a TODO comment to track this.
-func (w *StandardWriter) RequestStatsLog(statusCode, response string) {} +// TODO: Implement HTTP stats logging +func (w *StandardWriter) RequestStatsLog(statusCode, response string) {}cmd/nuclei/main.go (1)
449-449: Enhance the flag description.While the flag is correctly added, consider providing a more detailed description that explains:
- What kind of HTTP stats are captured (status codes, WAF detections, errors)
- Where and how the stats are displayed
- The experimental nature of the feature and any limitations
Apply this diff to improve the description:
- flagSet.BoolVarP(&options.HTTPStats, "http-stats", "hps", false, "enable http status capturing (experimental)"), + flagSet.BoolVarP(&options.HTTPStats, "http-stats", "hps", false, "enable http stats capturing including status codes, WAF detections & grouped errors (displayed at the end of scan) (experimental)"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/output/stats/regexes.jsonis excluded by!**/*.json
📒 Files selected for processing (10)
cmd/nuclei/main.go(1 hunks)internal/runner/runner.go(4 hunks)pkg/output/multi_writer.go(2 hunks)pkg/output/output.go(5 hunks)pkg/output/output_stats.go(1 hunks)pkg/output/stats/stats.go(1 hunks)pkg/output/stats/stats_test.go(1 hunks)pkg/protocols/http/request.go(1 hunks)pkg/testutils/testutils.go(2 hunks)pkg/types/types.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/protocols/http/request.go
🧰 Additional context used
🪛 GitHub Check: Lint
pkg/output/stats/stats.go
[failure] 70-70:
Error return value of m.Set is not checked (errcheck)
[failure] 86-86:
Error return value of t.errorCodes.Iterate is not checked (errcheck)
[failure] 90-90:
Error return value of t.statusCodes.Iterate is not checked (errcheck)
[failure] 94-94:
Error return value of t.wafDetected.Iterate is not checked (errcheck)
🪛 GitHub Actions: 🔨 Tests
pkg/output/stats/stats.go
[error] 70-70: Error return value of m.Set is not checked (errcheck)
🔇 Additional comments (10)
pkg/output/output_stats.go (1)
8-51: Implementation ofStatsOutputWriterand integration withTrackerThe
StatsOutputWriterstruct and its methods are correctly implemented, effectively integrating with theTrackerfor statistics observation. The methods fulfill theWriterinterface requirements and appropriately delegate tracking responsibilities.pkg/output/multi_writer.go (2)
11-12: AssertMultiWriterimplements theWriterinterfaceThe declaration
var _ Writer = &MultiWriter{}ensures at compile time thatMultiWritersatisfies theWriterinterface. This is a good practice to catch implementation errors early.
63-67: AddRequestStatsLogmethod toMultiWriterThe
RequestStatsLogmethod is properly added toMultiWriter, forwarding the call to all underlying writers. This ensures that all writers receive and handle the stats logging consistently.pkg/testutils/testutils.go (1)
206-206: LGTM!The empty implementation is appropriate for a mock object, allowing tests to override the behavior when needed.
pkg/output/output.go (1)
53-54: LGTM!Clean addition of the new method to the interface with clear documentation.
pkg/types/types.go (1)
226-227: LGTM!Clean addition of the
HTTPStatsfield with clear documentation. The field's purpose aligns well with the PR objectives.internal/runner/runner.go (4)
22-22: LGTM!The import statement is correctly aliased and properly specified.
96-97: LGTM!The
httpStatsfield is correctly added to theRunnerstruct with proper type.
262-265: LGTM!The HTTP stats tracking initialization is properly implemented:
- Correctly guarded by the
HTTPStatsflag- Properly initializes the tracker
- Correctly wraps the output writer
372-374: LGTM!The HTTP stats display is properly implemented with appropriate nil check and respects the color settings.
pkg/output/stats/stats.go
Outdated
| t.statusCodes.Iterate(func(k string, v *atomic.Int32) error { | ||
| stats.StatusCodeStats[k] = int(v.Load()) | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
Check the error return value of t.statusCodes.Iterate
Similar to the previous comment, the error return value of t.statusCodes.Iterate is not being checked. This could result in missing errors during the iteration over status codes.
Apply this diff to handle the error:
- t.statusCodes.Iterate(func(k string, v *atomic.Int32) error {
+ if err := t.statusCodes.Iterate(func(k string, v *atomic.Int32) error {
stats.StatusCodeStats[k] = int(v.Load())
return nil
- })
+ }); err != nil {
+ // Handle the error appropriately, e.g., log the error
+ }📝 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.
| t.statusCodes.Iterate(func(k string, v *atomic.Int32) error { | |
| stats.StatusCodeStats[k] = int(v.Load()) | |
| return nil | |
| }) | |
| if err := t.statusCodes.Iterate(func(k string, v *atomic.Int32) error { | |
| stats.StatusCodeStats[k] = int(v.Load()) | |
| return nil | |
| }); err != nil { | |
| // Handle the error appropriately, e.g., log the error | |
| } |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 90-90:
Error return value of t.statusCodes.Iterate is not checked (errcheck)
… waf-stats-errors-tracking
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/output/stats/waf/waf.go (4)
6-6: Consider using a more flexible logging approach.Instead of using the standard
logpackage, consider using an interface-based logging approach that allows consumers to provide their own logger implementation. This would make the package more flexible and testable.- "log" + "io"Then update the
NewWafDetectorfunction to accept a logger:-func NewWafDetector() *WafDetector { +func NewWafDetector(logger io.Writer) *WafDetector { var data wafData if err := json.Unmarshal([]byte(wafContentRegexes), &data); err != nil { - log.Printf("could not unmarshal waf content regexes: %s", err) + fmt.Fprintf(logger, "could not unmarshal waf content regexes: %s\n", err) }
10-13: Add documentation for exported struct and fields.Since
WafDetectoris exported, it should have proper documentation explaining its purpose and fields.+// WafDetector provides functionality to detect and identify Web Application Firewalls (WAFs) +// based on response content patterns. type WafDetector struct { + // wafs contains a mapping of WAF IDs to their definitions wafs map[string]waf + // regexCache contains pre-compiled regular expressions for WAF detection regexCache map[string]*regexp.Regexp }
50-57: Add context and input validation.Consider these improvements:
- Add context for timeout control.
- Validate input content.
- Consider case-insensitive matching option.
-func (d *WafDetector) DetectWAF(content string) (string, bool) { +func (d *WafDetector) DetectWAF(ctx context.Context, content string) (string, bool) { + if content == "" { + return "", false + } + for id, regex := range d.regexCache { + select { + case <-ctx.Done(): + return "", false + default: + } if regex.MatchString(content) { return id, true } } return "", false }
59-62: Add input validation.Add validation for the WAF ID parameter.
func (d *WafDetector) GetWAF(id string) (waf, bool) { + if id == "" { + return waf{}, false + } waf, ok := d.wafs[id] return waf, ok }pkg/output/stats/waf/waf_test.go (1)
5-60: Enhance test coverage and performance.Consider these improvements:
- Add edge cases (empty content, very long content, special characters).
- Enable parallel test execution.
- Add benchmarks for performance testing.
- Test error cases from
NewWafDetector.func TestWAFDetection(t *testing.T) { - detector := NewWafDetector() + detector, err := NewWafDetector() + if err != nil { + t.Fatalf("failed to create detector: %v", err) + } if detector == nil { t.Fatal("expected non-nil wafDetector") } tests := []struct { name string content string expectedWAF string shouldMatch bool }{ + { + name: "Empty content", + content: "", + expectedWAF: "", + shouldMatch: false, + }, + { + name: "Very long content", + content: strings.Repeat("a", 1000000), + expectedWAF: "", + shouldMatch: false, + }, + { + name: "Content with special characters", + content: "Response with special chars: !@#$%^&*()", + expectedWAF: "", + shouldMatch: false, + }, // existing test cases... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() waf, matched := detector.DetectWAF(tt.content) if matched != tt.shouldMatch { t.Errorf("expected match=%v, got match=%v", tt.shouldMatch, matched) } if matched && waf != tt.expectedWAF { t.Errorf("expected WAF=%s, got WAF=%s", tt.expectedWAF, waf) } }) } } +func BenchmarkWAFDetection(b *testing.B) { + detector, err := NewWafDetector() + if err != nil { + b.Fatalf("failed to create detector: %v", err) + } + + content := "Attention Required! | Cloudflare" + b.ResetTimer() + for i := 0; i < b.N; i++ { + detector.DetectWAF(content) + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/output/stats/waf/regexes.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
pkg/output/stats/stats.go(1 hunks)pkg/output/stats/stats_test.go(1 hunks)pkg/output/stats/waf/waf.go(1 hunks)pkg/output/stats/waf/waf_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/output/stats/stats_test.go
- pkg/output/stats/stats.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
🔇 Additional comments (1)
pkg/output/stats/waf/waf.go (1)
27-28: Verify regexes.json file.Please ensure that:
- The
regexes.jsonfile exists in the same directory.- The regex patterns are well-tested and optimized.
- The file is properly formatted and validated.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify regexes.json file exists and is valid JSON # Test: Check if file exists and is valid JSON if [ -f "pkg/output/stats/waf/regexes.json" ]; then jq . pkg/output/stats/waf/regexes.json >/dev/null 2>&1 if [ $? -eq 0 ]; then echo "regexes.json exists and is valid JSON" # Show file contents for review cat pkg/output/stats/waf/regexes.json else echo "regexes.json exists but is not valid JSON" fi else echo "regexes.json does not exist" fiLength of output: 60059
Regexes File Verification Complete
The
regexes.jsonfile located inpkg/output/stats/waf/regexes.jsonexists, is well-formatted, and successfully validates as JSON. The file contains a comprehensive set of regex definitions along with their corresponding metadata and signatures.
- The file is present in the expected directory.
- It passes JSON validation (using
jq).- The regex patterns and associated data are clearly structured for further reviews and testing.
|
Ignore test fails; fixed in |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/nuclei/main.go (1)
450-450: LGTM! The flag is well-integrated and properly documented.The new
http-statsflag follows the established patterns:
- Uses consistent naming with other flags
- Includes experimental status in description
- Properly integrated into the stats group
However, since this is an experimental feature, consider adding a warning message when the flag is enabled.
Add a warning message in the
readConfigfunction when the flag is enabled:if options.HTTPStats { + gologger.Warning().Msgf("The http-stats flag is experimental and its behavior may change in the future") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/nuclei/main.go(1 hunks)internal/runner/runner.go(4 hunks)pkg/types/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/types/types.go
- internal/runner/runner.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
* feat: added initial live DAST server implementation * feat: more logging + misc additions * feat: auth file support enhancements for more complex scenarios + misc * feat: added io.Reader support to input providers for http * feat: added stats db to fuzzing + use sdk for dast server + misc * feat: more additions and enhancements * misc changes to live server * misc * use utils pprof server * feat: added simpler stats tracking system * feat: fixed analyzer timeout issue + missing case fix * misc changes fix * feat: changed the logics a bit + misc changes and additions * feat: re-added slope checks + misc * feat: added baseline measurements for time based checks * chore(server): fix typos Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(templates): potential DOM XSS Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(authx): potential NIL deref Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * feat: misc review changes * removed debug logging * feat: remove existing cookies only * feat: lint fixes * misc * misc text update * request endpoint update * feat: added tracking for status code, waf-detection & grouped errors (#6028) * feat: added tracking for status code, waf-detection & grouped errors * lint error fixes * feat: review changes + moving to package + misc --------- Co-authored-by: sandeep <8293321+ehsandeep@users.noreply.github.com> * fix var dump (#5921) * fix var dump * fix dump test * Added filename length restriction for debug mode (-srd flag) (#5931) Co-authored-by: Andrey Matveenko <an.matveenko@vkteam.ru> * more updates * Update pkg/output/stats/waf/waf.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: sandeep <8293321+ehsandeep@users.noreply.github.com> Co-authored-by: Dwi Siswanto <25837540+dwisiswant0@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Dogan Can Bakir <65292895+dogancanbakir@users.noreply.github.com> Co-authored-by: 9flowers <51699499+Lercas@users.noreply.github.com> Co-authored-by: Andrey Matveenko <an.matveenko@vkteam.ru> Co-authored-by: Sandeep Singh <sandeep@projectdiscovery.io>
Proposed changes
http-statsflag allowing exporting grouped per-request Status Codes, Errors & WAF Detections (if any detected in the response)Closes #6045
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests